Fail task instances whose stored next_kwargs cannot be processed - #70685
Conversation
|
A few notes on the description text rather than the code:
|
ab4ad64 to
a28161c
Compare
a28161c to
004dcc8
Compare
|
The diff has changed materially since both of your reviews, so flagging for a fresh look rather than letting the existing approval carry it. @amoghrajesh — your approval predates @kaxil's review. Since then:
@kaxil — the one finding I did not implement is the HITL write-side validation. It is filed as #71036 and linked from a comment on None of this is a trivial rewording, so I would rather both of you look again before it goes in. Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting |
amoghrajesh
left a comment
There was a problem hiding this comment.
Thanks, looks fine to me.
handle_event_submit decoded a task instance's stored next_kwargs and assumed the result was a dict. Neither assumption held: the decode caught only four exception types, so anything the BaseSerialization fallback raised escaped, and the isinstance check sat under TYPE_CHECKING, so it never ran at runtime. Both escape as exceptions from a function whose callers walk every waiting task instance in one pass — the scheduler's timeout sweep and two API routes — so a single unusable payload aborted the whole batch. Decode through a helper that checks its result, and guard decode, event insertion and re-encode together. A task instance whose payload cannot be processed is re-queued to fail through the existing __fail__ path, so its normal retry and callback handling still runs, instead of being left parked for the next sweep to trip over again. Generated-by: Claude Opus 5 (1M context) following the guidelines at https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#gen-ai-assisted-contributions
The Dag author sees only the task log, so the traceback has to travel in next_kwargs the way submit_failure already sends it; the process log where this runs is often not theirs to read. Decode and re-encode also fail for different reasons: blaming the stored kwargs for a payload the trigger just yielded points the author at database state that was never at fault. The sweep's summary counted an unresumable task as resolved.
Co-authored-by: Amogh Desai <amoghrajesh1999@gmail.com>
One-line summary on the second line + blank line before the description, so the docstring satisfies both D205 (blank between summary and description) and D213 (summary on second line). Static checks were failing on ruff for this.
0764deb to
738251c
Compare
Backport failed to create: v3-3-test. View the failure log Run detailsNote: As of Merging PRs targeted for Airflow 3.X In matter of doubt please ask in #release-management Slack channel.
You can attempt to backport this manually by running: cherry_picker 2b7a0be v3-3-testThis should apply the commit to the v3-3-test branch and leave the commit in conflict state marking After you have resolved the conflicts, you can continue the backport process by running: cherry_picker --continueIf you don't have cherry-picker installed, see the installation guide. |
) (#71183) * Fail task instances whose stored next_kwargs cannot be processed handle_event_submit decoded a task instance's stored next_kwargs and assumed the result was a dict. Neither assumption held: the decode caught only four exception types, so anything the BaseSerialization fallback raised escaped, and the isinstance check sat under TYPE_CHECKING, so it never ran at runtime. Both escape as exceptions from a function whose callers walk every waiting task instance in one pass — the scheduler's timeout sweep and two API routes — so a single unusable payload aborted the whole batch. Decode through a helper that checks its result, and guard decode, event insertion and re-encode together. A task instance whose payload cannot be processed is re-queued to fail through the existing __fail__ path, so its normal retry and callback handling still runs, instead of being left parked for the next sweep to trip over again. Generated-by: Claude Opus 5 (1M context) following the guidelines at https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#gen-ai-assisted-contributions * Report why a task instance could not be resumed, and to whom The Dag author sees only the task log, so the traceback has to travel in next_kwargs the way submit_failure already sends it; the process log where this runs is often not theirs to read. Decode and re-encode also fail for different reasons: blaming the stored kwargs for a payload the trigger just yielded points the author at database state that was never at fault. The sweep's summary counted an unresumable task as resolved. * Update airflow-core/src/airflow/models/trigger.py * Fix ruff D205/D213 on _fail_unresumable_task_instance docstring One-line summary on the second line + blank line before the description, so the docstring satisfies both D205 (blank between summary and description) and D213 (summary on second line). Static checks were failing on ruff for this. --------- (cherry picked from commit 2b7a0be) Co-authored-by: Jarek Potiuk <jarek@potiuk.com> Co-authored-by: Amogh Desai <amoghrajesh1999@gmail.com>
) (#71183) * Fail task instances whose stored next_kwargs cannot be processed handle_event_submit decoded a task instance's stored next_kwargs and assumed the result was a dict. Neither assumption held: the decode caught only four exception types, so anything the BaseSerialization fallback raised escaped, and the isinstance check sat under TYPE_CHECKING, so it never ran at runtime. Both escape as exceptions from a function whose callers walk every waiting task instance in one pass — the scheduler's timeout sweep and two API routes — so a single unusable payload aborted the whole batch. Decode through a helper that checks its result, and guard decode, event insertion and re-encode together. A task instance whose payload cannot be processed is re-queued to fail through the existing __fail__ path, so its normal retry and callback handling still runs, instead of being left parked for the next sweep to trip over again. Generated-by: Claude Opus 5 (1M context) following the guidelines at https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#gen-ai-assisted-contributions * Report why a task instance could not be resumed, and to whom The Dag author sees only the task log, so the traceback has to travel in next_kwargs the way submit_failure already sends it; the process log where this runs is often not theirs to read. Decode and re-encode also fail for different reasons: blaming the stored kwargs for a payload the trigger just yielded points the author at database state that was never at fault. The sweep's summary counted an unresumable task as resolved. * Update airflow-core/src/airflow/models/trigger.py * Fix ruff D205/D213 on _fail_unresumable_task_instance docstring One-line summary on the second line + blank line before the description, so the docstring satisfies both D205 (blank between summary and description) and D213 (summary on second line). Static checks were failing on ruff for this. --------- (cherry picked from commit 2b7a0be) Co-authored-by: Jarek Potiuk <jarek@potiuk.com> Co-authored-by: Amogh Desai <amoghrajesh1999@gmail.com>
handle_event_submitdecoded a task instance's storednext_kwargsand thenassumed the result was a dict. The decode caught only four exception types, so
anything the
BaseSerializationcompat fallback raised escaped; and theisinstancecheck was underif TYPE_CHECKING:, so it never ran at runtime.Both surface as exceptions from a function whose callers process every waiting
task instance in a single pass — the scheduler's
AWAITING_INPUTtimeout sweepand two API routes — so one task instance with an unusable stored payload took
the whole batch down with it.
This decodes through a helper that validates its own result, and guards decode,
event insertion and re-encode as one unit. A task instance whose payload cannot
be processed is re-queued to fail via the existing
__fail__path, so retriesand
on_failurecallbacks run normally, rather than being left parked for thenext sweep to trip over again.
Behaviour change
The HITL response endpoint and the execution API's
AWAITING_INPUTbranchpreviously returned HTTP 500 for such a payload; they now succeed, with the task
routed to fail. No request shape that previously worked is newly rejected.
Test plan
test_handle_event_submit_fails_task_with_unusable_next_kwargs— parametrized over an undecodable payload and one that decodes to a non-dicttest_awaiting_input_timeout_sweep_survives_unusable_next_kwargs— end-to-end through the sweep, with a malformed and a healthy task instance in the same batchmainwithout this changeruff check/ruff format/mypycleanDeferred
Failing a task instance rather than raising is not free for the Human-in-the-loop path: a response whose
params_inputcarries a serde-reserved key (__classname__,__schema_id__) is now recorded and discarded rather than rejected, and theresponse_receivedguard blocks a corrected resubmission. The fix belongs on the write side inhitl.py; tracked in #71036, and linked from a comment onhandle_event_submit.Was generative AI tooling used to co-author this PR?
Generated-by: Claude Opus 5 (1M context) following the guidelines at
https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#gen-ai-assisted-contributions