Skip to content

Re-anchor custom queued deadline references when a Dag run is cleared - #71850

Open
rjgoyln wants to merge 4 commits into
apache:mainfrom
rjgoyln:deadline-custom-queued-reanchor
Open

rjgoyln wants to merge 4 commits into
apache:mainfrom
rjgoyln:deadline-custom-queued-reanchor

Conversation

@rjgoyln

@rjgoyln rjgoyln commented Aug 19, 2026 •

Copy link
Copy Markdown
Contributor

Summary

A custom deadline reference registered as DeadlineReference.TYPES.DAGRUN_QUEUED is never re-anchored when its Dag run is cleared — the built-in DAGRUN_QUEUED_AT deadline moves to the new queued_at, the custom one keeps pointing at the old one.

The registered timing only ever existed in the in-memory DeadlineReference.TYPES tuples of the process that ran the Dag file, so the clear path could recognise a queued-anchored deadline only by matching the built-in class name, which no custom class satisfies.

  • Record the registered timing on the reference class and store it on the alert row.
  • Select queued-anchored deadlines on the stored timing, keeping the class-name match as a fallback.
  • Compare a stored reference only on the fields it has, and refresh a matched row in place.

The serializer adds the field rather than serialize_reference(), because custom references are expected to override that method and an override would drop it.

Compatibility

An alert serialized before this change keeps its id and gains the timing on the next parse rather than being replaced, which would strand the deadlines of running Dag runs.

closes: #71747


Was generative AI tooling used to co-author this PR?
  • Yes — Claude Code (Opus 5)

Generated-by: Claude Code (Opus 5) following the guidelines

@ferruzzi

Copy link
Copy Markdown
Contributor

Cross-linking, since this overlaps two other open PRs on the same code and I do not think the three authors are aware of each other.

#70714 (opened 30 July) changes deadline references to receive the in-memory DagRun instead of dag_id and run_id, and deprecates required_kwargs. It overlaps this PR across a bunch of files. Worth a look before you rebase, particularly _evaluate_with's signature, which it narrows from **kwargs to a keyword-only dagrun.

On the interval workaround: once #70370 lands, storing numeric intervals in the tests to stay clear of the JSON bug is no longer necessary, and storing serialize(interval) instead would then hit the production write path. That legacy-float shortcut is what hid the TypeError in the existing test in the
first place, so it is worth removing rather than inheriting once the fix is in.

@ferruzzi

Copy link
Copy Markdown
Contributor

#71802 also edits serialization/definitions/deadline.py, so it conflicts here too.

When you rebase on top of #70714, the required_kwargs: set[str] = set() line your diff has above the new evaluation_timing is deleted by that PR, and the deletion is importnt. Its replacement treats a present required_kwargs as an opt-in to the deprecated path, so keeping that line silently routes every reference including the built-ins through it rather than failing loudly. Just be careful when you do a rebase or merge conflict.

@rjgoyln

rjgoyln commented Aug 26, 2026

Copy link
Copy Markdown
Contributor Author

Cross-linking, since this overlaps two other open PRs on the same code and I do not think the three authors are aware of each other.

#70714 (opened 30 July) changes deadline references to receive the in-memory DagRun instead of dag_id and run_id, and deprecates required_kwargs. It overlaps this PR across a bunch of files. Worth a look before you rebase, particularly _evaluate_with's signature, which it narrows from **kwargs to a keyword-only dagrun.

On the interval workaround: once #70370 lands, storing numeric intervals in the tests to stay clear of the JSON bug is no longer necessary, and storing serialize(interval) instead would then hit the production write path. That legacy-float shortcut is what hid the TypeError in the existing test in the first place, so it is worth removing rather than inheriting once the fix is in.

Thanks for the cross-links! I’d only accounted for #70370.

For #70714, I’ll keep the evaluation_timing deletions when rebasing and follow the keyword-only dagrun signature.

For the interval tests, agreed — I’ll switch them to serialize(interval) once #70370 lands and remove the note from the description.

#71802 noted as well. It should be a textual conflict in the interval handling rather than a behavioral one.

@ferruzzi

Copy link
Copy Markdown
Contributor

It's going to be a bit of a mess untangling them all as we merge them, but we'll sort it out.

Comment thread airflow-core/src/airflow/models/taskinstance.py
Comment thread airflow-core/src/airflow/serialization/encoders.py
Comment thread airflow-core/tests/unit/models/test_taskinstance.py
@rjgoyln

rjgoyln commented Oct 8, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the review — all three are fair.

I've addressed the reference matching issue: matching now ignores fields the stored row lacks, and a matched row is refreshed in place so evaluation_timing is populated without changing the ID.

For recomputing from the user's reference and the missing _evaluate_with coverage, I haven't made those changes yet. They depend on #70714's new interface, where the reference takes an in-memory DagRun and _evaluate_with takes dagrun as a keyword-only argument. Since #70714 is still open, I'll follow it once that interface is available — happy to rebase onto your branch and do it here instead if you prefer.

@rjgoyln
rjgoyln force-pushed the deadline-custom-queued-reanchor branch from 2b35a1c to 7078bac Compare October 10, 2026 06:21
@ferruzzi

Copy link
Copy Markdown
Contributor

I've been working through a long chain of PRs which are all editing the same section of code, with the goal of merging them in the order that will minimize conflicts and this one is up next. If you can get it rebased and green, I'll merge it. This is not going to be a simple "click to fast-forward" rebase, but I tried to make it as painless as could. Be careful not to lose the dagrun being explicitly passed into _evaluate_with and the rest should flow pretty easily from there.

The evaluation timing a reference is registered with only lived in the
in-memory registry of the process that ran the Dag file, so the scheduler
had no way to tell a queued-anchored reference from any other and fell
back to matching the built-in class name. Custom references registered as
DeadlineReference.TYPES.DAGRUN_QUEUED were silently left pointing at the
old queued_at.

closes: apache#71747
Comparing the whole reference dict made every stored alert look changed as
soon as an upgrade added a field to the serialized form. All alert UUIDs were
then regenerated, and because Deadline.deadline_alert_id is ON DELETE SET
NULL, the deadlines of running Dag runs lost their alert and stopped being
re-anchored entirely.
Now that the clear path decodes the alert to resolve its interval (apache#70370), a
bare number and an unregistered reference class no longer stand in for what the
Dag processor actually writes: decoding refuses a custom reference that no
plugin advertises, so the deadline was silently left alone.
…ence

The recompute now asks the reference for the new time (apache#70714), so a reference
whose evaluation returns something other than the bare queued_at is what
separates re-anchoring through the reference from the built-in formula.
@rjgoyln
rjgoyln force-pushed the deadline-custom-queued-reanchor branch from 7078bac to b12a9e8 Compare October 15, 2026 19:16
@rjgoyln

rjgoyln commented Oct 15, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the review! I've rebased onto main and updated the test to distinguish reference re-anchoring from the built-in formula. Intervals now use serialize(interval).

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Custom deadline references registered as TYPES.DAGRUN_QUEUED are not re-anchored when a DagRun is cleared

3 participants