Fix TypeError in TaskInstance.next_retry_datetime() for None end_date - #70092
Fix TypeError in TaskInstance.next_retry_datetime() for None end_date#70092AdityaBhattacharya1 wants to merge 4 commits into
Conversation
|
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contributors' Guide
|
| if self.task.max_retry_delay: | ||
| delay = min(self.task.max_retry_delay, delay) | ||
| return self.end_date + delay | ||
| base = self.end_date if self.end_date is not None else timezone.utcnow() |
There was a problem hiding this comment.
Nice catch on the TypeError.
I might be missing something, but since this is recomputed each scheduler pass, wouldn't an end_date is None TI always land at utcnow() + delay and stay up_for_retry indefinitely? Curious what you think.
There was a problem hiding this comment.
Solid point, this needs fixing.
I'll change it to set self.end_date = timezone.utcnow() once, instead of a local fallback, so it anchors and the normal backoff math converges the same way it would for a task that just failed. Since this runs inside an active session, the mutation flushes on the caller's next commit without next_retry_datetime() needing a session param of its own.
One trade-off worth flagging explicitly: this persists a synthetic end_date, so duration/Gantt/API will show a "recovery anchor" timestamp rather than a real completion time. That's the same decision #12675 made for the set_state() path. Let me know if that's alright, will implement the changes right away. Thanks!
There was a problem hiding this comment.
Direction sounds right.
Tiny thing, feel free to ignore. Since is_premature calls next_retry_datetime(), would mutating there mean a plain read touches the DB?
If so, set_state() already anchors this case. Either way works.
There was a problem hiding this comment.
Since is_premature calls next_retry_datetime(), would mutating there mean a plain read touches the DB?
Yup, looked into NotInRetryPeriodDep._get_dep_statuses() as an alternative as well but even that is similarly reachable so doesn't really make for a cleaner path. Will go ahead with the aforementioned change 👍🏻
There was a problem hiding this comment.
Made the change and updated the tests to properly cover this edge case
There was a problem hiding this comment.
Looks good. Thanks for the fix!
There was a problem hiding this comment.
Cool! Could you approve the changes then? Can proceed with the merging thereafter
There was a problem hiding this comment.
I'm a contributor, not a committer, so an "Approve" from me won't unblock the merge. It needs a committer to review and merge.
There was a problem hiding this comment.
Ahh that makes sense, thanks for letting me know!
…uting a fresh fallback on every call
6e932c3 to
18f2440
Compare
A task instance can end up in the
up_for_retrystate withend_dateleft asNone, for example if the process running it is killed abruptly before the end date gets recorded, or via any code path that transitions task state without going throughTaskInstance.set_state(). When that happens,next_retry_datetime()unconditionally computesself.end_date + delay, raising:This is called from
NotInRetryPeriodDep._get_dep_statuses(), which the scheduler invokes on every scheduling pass for a retry-eligible task. The exception is unhandled at that call site, so depending on version this either crashes the scheduler process outright, or (after the per-DagRun failure isolation added in #62893) gets caught and logged but causes that DagRun's scheduling pass to fail identically every time it's re-evaluated — the affected task instance never recovers on its own without directly patching itsend_datein the metadata database.Personally I faced this error for a DAG run where a DAG had been transitioned to
up_for_retrystatus and then paused. Later revision of the DAG changed the DAG structurally. When I unpaused the DAG and cleared state for the previous DAG run, the end_date was never repopulated for that task instance: DagRun.verify_integrity() / expand_mapped_task() had to reconcile the stale map_index=-1 row against a task definition that had since become mapped, and that reconciliation left the row up_for_retry with end_date still NULL.Also added a unit test on
next_retry_datetime()covering theend_date=Nonecase directly, and one onNotInRetryPeriodDepreproducing the actual crash path named in the linked issues (state=up_for_retry,end_date=None) to confirm the dependency check no longer raises.closes: #51640
related: #19615
Was generative AI tooling used to co-author this PR?
{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.