Handle missed deadline alerts when DagRuns fail#68961
Conversation
5299bc4 to
bf27fda
Compare
bf27fda to
48d9021
Compare
|
@shivaam — the Static checks CI job is failing here, which needs a code fix on your side (a rerun won't clear it). You can reproduce and fix locally with: Once those pass and CI is green, it'll be ready for a maintainer to pick up. Thanks! See the PR quality criteria. Automated first-pass triage note drafted by an AI-assisted tool — may get things wrong; once addressed, a real Apache Airflow maintainer takes the next look. (why automated) Drafted-by: Claude Code (Opus 4.8); reviewed by @potiuk before posting |
48d9021 to
3eb066b
Compare
|
Thanks for the PR and the thorough testing. This had a pretty long discussion in the Issue and I still don't think short-circuiting by default is the right answer. Let's say you have a Dag which generates a report which you need for your morning meeting so you set a Deadline Alert for 9AM which emails you saying your report won't be ready. Case 1) You work at Big Company. The Dag fails at 7AM, an admin sees this, determines it's just a network hiccup and clears the failing tasks which then pass at 7:30. If the deadline fired on failure, you would have your report and a notice saying you won't. Case 2) Your Dag may have an Case 3) You work at Small Company and don't have an admin team, you do it all yourself, so a Dag failing likely won't be cleared and the missed deadline is inevitable. I'd argue that Case 4) You realize something is wrong with the input and you manually mark the Dag as FAILED so the report doesn't get generated with bad data. Are you going to fix the data and re-trigger the Dag, or is that the end of the story? We don't know and shouldn't guess. Worth noting: this PR wouldn't cover this case either, which would introduce a difference in behavior between an automatic failure and a manual one. Overall, always short-circuiting is not the answer. I would propose three alternatives which are not mutually exclusive:
To be clear, I do agree that there is a real user need here, I'm not trying to kill this or dissuade you. I just want to make sure we solve it without breaking the existing use cases. It's one very valid use case, but that doesn't invalidate the others. That said, here's my suggestion: I'd also ask that the ((tagging @ramitkataria since he was also involved in the original discussion and may have thoughts.)) |
|
@ferruzzi thanks, this makes sense. I split the independent For this PR, I agree that always firing the Deadline Alert on failure is too broad as the default. I will rework this toward an opt-in The prune-hook and UI/manual-failure options also make sense to me, but I think those can be tracked as follow-up issues unless you think they should be included in this PR. |
3eb066b to
4bf50ea
Compare
|
The bugfix portion is approved and merged; let me know when you are ready for a re-review on this one or if you want to discuss it more, otherwise I'll just let you work on it in peace. 👍 |
4bf50ea to
9e0d761
Compare
@ferruzzi here is the updated pr with |
Thanks for case-by-case anaysis - I agree with your proposals |
Thank you for looking into this. I have implemented the first point from @ferruzi's proposal in this PR and will track the other two as new issues. Please let me know if you will have any feedback |
9e0d761 to
cf16975
Compare
ferruzzi
left a comment
There was a problem hiding this comment.
IMHO, this looks like it is working how I would expect it to work. I know there is still some discussion on whether this is the right approach, so I'll let that discussion play out before approving.
Also, looks like a merge conflict needs to be resolved.
@ferruzzi One note: since this PR adds a migration, this kind of conflict may keep coming back whenever another migration lands on main first. To avoid churn, would it be okay to get the functional/design review settled first, and then I can do one final rebase of the migration metadata right before merge? |
|
@ramitkataria do you have any other feedback comments on it? |
cf16975 to
c229f08
Compare
Nope, looks good to me |
DeadlineAlert.fire_on_failure, defaulting toFalse.fire_on_failure=True.fire_on_failure=Truestill fire only whendeadline_timeis reached.Closes: #60927
Details
Failed DagRuns currently leave pending deadline alerts untouched until the scheduler's expired-deadline loop reaches
deadline_time. For long deadline intervals, that can delay the alert by hours after the DagRun has already failed.This PR adds an opt-in flag,
DeadlineAlert.fire_on_failure, for users who want deadline alerts to fire as soon as an automated scheduler-driven DagRun failure is finalized.The default is
Falsefor backward compatibility. Existing Deadline Alerts keep their current behavior and fire atdeadline_time.The independent callback data persistence bug was split into #69073 and has already merged, so this PR is now focused on the failure-time opt-in behavior only.
Also tested with a local Airflow cluster:
fire_on_failure=True sync/executor callback fired immediately on DagRun failure.
fire_on_failure=True async/triggerer callback fired immediately on DagRun failure.
fire_on_failure=False stayed pending after failure and fired only after deadline_time.
Tests
Local focused validation passed:
6 passed21 passed11 passedAlso tested with a real local Airflow/Postgres daemon harness:
fire_on_failure=Truesync/executor callback fired immediately on DagRun failure.fire_on_failure=Trueasync/triggerer callback fired immediately on DagRun failure.fire_on_failure=Falsestayed pending after failure and fired only afterdeadline_time.Was generative AI tooling used to co-author this PR?
Important
🛠️ Maintainer triage note for @shivaam · by
@potiuk· 2026-07-08 15:38 UTCHelpful heads-up from the maintainers — please address before this PR can be reviewed:
CI image checks / Static checks). See the contributor guide.Full criteria: Pull Request quality criteria.
The ball is in your court — you've been assigned to this PR. Fix the above, then mark it Ready for review.
Automated triage — may be imperfect; a maintainer takes the next look.