Repository navigation
Clear retry policy state when a TI is cleared - #73889
amoghrajesh wants to merge 1 commit into
Conversation
|
@kaxil 3.4 item, can I get a review here? |
| scheduled_dttm=timezone.utcnow(), | ||
| try_number=next_try_number, | ||
| # Already archived with the finished try; the new one must not inherit it. | ||
| retry_reason=None, |
There was a problem hiding this comment.
A task with start_from_trigger=True never reaches this UPDATE: defer_task() at line 2267 sends it straight to DEFERRED and doesn't touch either column. So a retry that starts in the triggerer still carries the previous try's values. If that trigger then fails with retries left, the task-end-event handler in trigger.py archives the stale reason under the new try number, and next_retry_datetime() times the following retry from the stale override, which is the case this PR is fixing. Clearing both in defer_task() next to where it sets the DEFERRED state would cover it, along with a start_from_trigger case in the schedule_tis test.
| ti.external_executor_id = None | ||
| # retry_delay_override is the functional one: next_retry_datetime() prefers it over | ||
| # the task's own retry_delay, so a stale value would retime the next retry. | ||
| ti.retry_reason = None |
There was a problem hiding this comment.
The state_reason description in core_api/datamodels/task_instances.py still says the reason "is cleared only when the task next starts running, so a task waiting to be retried or re-run can still carry the reason". After this change a cleared task never carries it, and it's gone as soon as the next try is scheduled. That text needs updating (and the generated OpenAPI, UI and airflowctl copies regenerating), along with the "Cleared on task start (ti_run)" comment on the columns at line 695 and the one at execution_api/routes/task_instances.py:741.
| clear_task_instances([ti], session) | ||
| session.flush() | ||
|
|
||
| assert ti.retry_reason is None |
There was a problem hiding this comment.
These assertions read back the same in-memory ti that clear_task_instances just changed, so they pass as long as the two assignments exist. Could you also check the TaskInstanceHistory row for the old try? It should still have the seeded reason and override, and that check would fail if the reset ever got moved above prepare_db_for_next_try.
| dr = dag_maker.create_dagrun(session=session) | ||
| ti = dr.get_task_instance("task", session=session) | ||
| ti.refresh_from_task(dag.get_task("task")) | ||
| ti.state = None |
There was a problem hiding this comment.
This only covers a None-state TI, but the case the PR is after is up_for_retry moving to scheduled (a None-state TI with these set mostly comes from a clear, which now resets them anyway). test_schedule_tis_preserves_allocated_attempt just above already parametrizes None / up_for_retry / up_for_reschedule with the same setup, so seeding the two columns there and asserting on them would cover all three without the copy.
This branch has not been deployed
2 participants
Was generative AI tooling used to co-author this PR?
Follow up from #73030
A retry policy's decision describes one attempt, but both columns holding it outlived that attempt. Clearing a task reset its state and left them behind, and scheduling the next try bumped try_number without touching them, so a task that failed before it started reported the previous attempt's reason under the new attempt's number.
retry_delay_overrideis the one that changes behaviour rather than display:next_retry_datetime()prefers it over the task's own retry_delay, so a value left over from an earlier attempt silently retimed the next retry.Clearing at both points is safe because the retry path archives the finished try, with its reason, into task_instance_history first.
{pr_number}.significant.rst, in airflow-core/newsfragments. You can add this file in a follow-up commit after the PR is created so you know the PR number.