Make deadline reads and serialization robust to dynamic/malformed intervals#68919
Open
seanghaeli wants to merge 1 commit into
Open
Make deadline reads and serialization robust to dynamic/malformed intervals#68919seanghaeli wants to merge 1 commit into
seanghaeli wants to merge 1 commit into
Conversation
seanghaeli
requested review from
XD-DENG,
ashb,
bolkedebruin,
bugraoz93,
choo121600,
ephraimbuddy,
jason810496,
pierrejeambrun,
rawwar and
shubhamraj-git
as code owners
boring-cyborg
Bot
added
area:API
seanghaeli
force-pushed
the
feature/deadline-response-serialization
branch
from
12a8e11 to
352702c
Compare
…ervals Hardens the read and (de)serialization paths for deadline alerts so dynamic (``VariableInterval``) and malformed stored data no longer break the UI/API. - UI deadline-alert response: ``DeadlineAlert.interval`` is a JSON column holding the Airflow-serialized interval, not a plain number. Coerce it to seconds for a fixed ``timedelta`` and to ``None`` for a dynamic ``VariableInterval`` (resolved later by the scheduler), instead of letting Pydantic 500 on the dict. The ``interval`` field becomes ``float | None``. - Drop ``interval`` from the sortable columns of the deadline-alerts endpoint: ordering by a JSON column sorts by structure/text, not duration, so the result was arbitrary and misleading. - Deserialization: route by the encoder-stamped ``__class_path`` ahead of the ``reference_type`` name (a custom reference may share a class name with a builtin), and raise a clear error for a reference with no importable ``__class_path`` instead of an opaque ``KeyError``. - ``Deadline.__repr__`` / ``DeadlineAlert.__repr__`` no longer raise: guard the ``dagrun`` relationship (the FK can be set while the relationship is None after a cascade delete) and handle the dict-shaped JSON interval. A ``__repr__`` must never raise. - ``prune_deadlines`` explicitly excludes deadlines already marked ``missed`` so a missed deadline (whose callback is owned by the scheduler/triggerer) and its queued callback are never cascade-deleted. Generated-by: Claude Code (Opus via Claude Code) on behalf of Sean Ghaeli
seanghaeli
force-pushed
the
feature/deadline-response-serialization
branch
from
352702c to
31bd6d5
Compare
potiuk
added
the
ready for maintainer review
Member
There was a problem hiding this comment.
Thanks for the PR.
I would make the code less verbose and keep only relevant comments/pieces. There are too many big comments some of them needs to be removed completely, some of them needs to be trimmed.
Overall looking good, just a few suggestions.
Another pair of eyes would be great on this.
Comment on lines
+76
to
+79
| Without this coercion Pydantic cannot turn that dict into ``float`` and the | ||
| ``/ui/dags/{dag_id}/deadlineAlerts`` endpoint raises a 500, which breaks the | ||
| run-page deadline status badge. Return the seconds for a fixed interval, or | ||
| ``None`` for a dynamic one (resolved later by the scheduler). |
Member
There was a problem hiding this comment.
Suggested change
| Without this coercion Pydantic cannot turn that dict into ``float`` and the | |
| ``/ui/dags/{dag_id}/deadlineAlerts`` endpoint raises a 500, which breaks the | |
| run-page deadline status badge. Return the seconds for a fixed interval, or | |
| ``None`` for a dynamic one (resolved later by the scheduler). | |
| Without this coercion Pydantic cannot turn that dict into ``float`` and the | |
| ``/ui/dags/{dag_id}/deadlineAlerts`` endpoint raises a 500. Return the seconds for a fixed interval, or | |
| ``None`` for a dynamic one (resolved later by the scheduler). |
| @@ -0,0 +1 @@ | |||
| Fixed the deadline UI/API endpoints raising ``500`` errors on deadline alerts whose interval is stored as a serialized object rather than a plain number. ``DeadlineAlert.interval`` is a JSON column holding the Airflow-serialized interval (a fixed ``timedelta`` or a dynamic ``VariableInterval``); the ``/ui/dags/{dag_id}/deadlineAlerts`` response now coerces it to seconds for a fixed interval and to ``null`` for a dynamic one instead of failing to validate. Sorting deadline alerts by ``interval`` (which would sort by JSON structure, not duration) is no longer offered. Deserializing a corrupt or unrecognized deadline reference now raises a clear error instead of an opaque ``KeyError``, custom references that share a class name with a builtin are routed correctly via ``__class_path``, and ``Deadline.__repr__`` no longer raises when the ``dagrun`` relationship has been severed. ``prune_deadlines`` now explicitly skips deadlines already marked ``missed`` so their queued callbacks are never cascade-deleted. | |||
pierrejeambrun
removed
the
ready for maintainer review
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
This re-introduces the deadline read/serialization robustness slice of #66608 (fully reverted in #68909)
The motivating bug:
DeadlineAlert.intervalis a JSON column holding the Airflow-serialized interval (a fixedtimedeltaor a dynamicVariableInterval), not a plain number. The/ui/dags/{dag_id}/deadlineAlertsresponse declaredinterval: float, so any alert with a serialized-dict interval failed Pydantic validation and the endpoint returned 500, breaking the run-page deadline status badge.What
datamodels/ui/deadline.py):intervalbecomesfloat | Nonewith abeforevalidator that returns seconds for a fixedtimedeltaandNonefor a dynamicVariableInterval.routes/ui/deadlines.py): dropintervalfromorder_by— sorting a JSON column sorts by structure, not duration.serialization/decoders.py,serialization/definitions/deadline.py): route by__class_pathahead ofreference_type(custom refs may share a builtin's class name); raise a clear error for a reference with no importable__class_pathinstead of an opaqueKeyError.__repr__safety (models/deadline.py,models/deadline_alert.py): never raise — guard thedagrunrelationship (FK can be set while the relationship isNonepost-cascade-delete) and handle the dict-shaped interval.models/deadline.py):prune_deadlinesexplicitly excludes~Deadline.missedso a missed deadline's queued callback is never cascade-deleted.Tests
test_deadlines.py(interval coercion + order-by rejection),test_deadline_alert.py/test_deadline.py(repr never raises),test_deadline_reference_registry.py(__class_pathprecedence + missing-class-path error),test_prune_deadlines.py(missed deadlines survive prune).Verified locally in Breeze: 113 passed.
Generated-by: Claude Code (Opus via Claude Code) on behalf of Sean Ghaeli