Skip to content

Fail task instances whose stored next_kwargs cannot be processed - #70685

Merged
vatsrahul1001 merged 4 commits into
apache:mainfrom
potiuk:fail-task-instance-with-unusable-next-kwargs
Aug 5, 2026
Merged

Fail task instances whose stored next_kwargs cannot be processed#70685
vatsrahul1001 merged 4 commits into
apache:mainfrom
potiuk:fail-task-instance-with-unusable-next-kwargs

Conversation

@potiuk

@potiuk potiuk commented Jul 29, 2026

Copy link
Copy Markdown
Member

handle_event_submit decoded a task instance's stored next_kwargs and then
assumed the result was a dict. The decode caught only four exception types, so
anything the BaseSerialization compat fallback raised escaped; and the
isinstance check was under if 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_INPUT timeout sweep
and 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 retries
and on_failure callbacks run normally, rather than being left parked for the
next sweep to trip over again.

Behaviour change

The HITL response endpoint and the execution API's AWAITING_INPUT branch
previously 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-dict
  • test_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 batch
  • Both fail on main without this change
  • ruff check / ruff format / mypy clean

Deferred

Failing a task instance rather than raising is not free for the Human-in-the-loop path: a response whose params_input carries a serde-reserved key (__classname__, __schema_id__) is now recorded and discarded rather than rejected, and the response_received guard blocks a corrected resubmission. The fix belongs on the write side in hitl.py; tracked in #71036, and linked from a comment on handle_event_submit.

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

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

@potiuk
potiuk requested review from XD-DENG and ashb as code owners July 29, 2026 13:56
@boring-cyborg boring-cyborg Bot added area:Scheduler including HA (high availability) scheduler area:Triggerer labels Jul 29, 2026
@potiuk potiuk added the backport-to-v3-3-test Backport to v3-3-test label Jul 29, 2026
@potiuk potiuk added this to the Airflow 3.3.1 milestone Jul 29, 2026

@amoghrajesh amoghrajesh left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @potiuk. I do not see any backcompat concerns too here, but @kaxil if you could take another look.

Comment thread airflow-core/src/airflow/models/trigger.py Outdated
Comment thread airflow-core/src/airflow/models/trigger.py Outdated
Comment thread airflow-core/tests/unit/jobs/test_scheduler_job.py
Comment thread airflow-core/tests/unit/models/test_trigger.py
Comment thread airflow-core/src/airflow/models/trigger.py
Comment thread airflow-core/src/airflow/models/trigger.py Outdated
Comment thread airflow-core/src/airflow/models/trigger.py Outdated
Comment thread airflow-core/src/airflow/models/trigger.py Outdated
Comment thread airflow-core/src/airflow/models/trigger.py Outdated
Comment thread airflow-core/tests/unit/jobs/test_scheduler_job.py Outdated
@kaxil

kaxil commented Aug 3, 2026

Copy link
Copy Markdown
Member

A few notes on the description text rather than the code:

  1. "the execution API's AWAITING_INPUT branch previously returned HTTP 500" -- it didn't. That route catches everything escaping the state-update helper and silently marks the TI FAILED with a direct UPDATE (task_instances.py, lines 465-476 on main), bypassing retries and worker-run on_failure callbacks. So the PR improves that caller more than the body claims: the task now goes through the real __fail__ path instead of a hard FAILED write. The HITL half of that sentence is accurate.

  2. The caller list ("the scheduler's AWAITING_INPUT timeout sweep and two API routes") omits the triggerer: Trigger.submit_event also lands in handle_event_submit, and pre-fix it had the worst failure mode of the four -- handle_events only confirms an event's persist seq after submit_event returns, so an escaping decode error meant the poison event was redelivered indefinitely (triggerer_job_runner.py, lines 764-776).

  3. One tradeoff worth a sentence in the behaviour-change section: when decode fails for environment reasons rather than bad data (say an allowed_deserialization_classes mismatch or a provider missing on the scheduler mid rolling-upgrade), waiting tasks now fail and consume retries, where main halted and preserved them until the environment was fixed. Failing is the right call, but the body currently reads as if nothing previously-working is newly affected, and this is the one case where the old crash-loop was accidentally protective.

@potiuk
potiuk force-pushed the fail-task-instance-with-unusable-next-kwargs branch 2 times, most recently from ab4ad64 to a28161c Compare August 3, 2026 23:32
@potiuk
potiuk force-pushed the fail-task-instance-with-unusable-next-kwargs branch from a28161c to 004dcc8 Compare August 3, 2026 23:37
@potiuk

potiuk commented Aug 4, 2026

Copy link
Copy Markdown
Member Author

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:

  • the traceback now travels in next_kwargs via format_exception, the way submit_failure already does it, because that is the only channel that reaches the task log;
  • decoding and re-encoding are separate try blocks with distinct messages, so a payload the trigger just yielded is no longer reported as unreadable stored kwargs;
  • the docstrings now say this runs in the triggerer too, and record why the except has to stay broad;
  • the shadowing local TaskInstanceState import is gone;
  • the AWAITING_INPUT sweep counts a task it could not resume separately rather than reporting it as resolved, and the test asserts (1, 0, 1).

@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 handle_event_submit; the reasoning is in that thread. Everything else is addressed above.

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

@potiuk
potiuk requested review from amoghrajesh and kaxil August 4, 2026 00:32

@amoghrajesh amoghrajesh left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, looks fine to me.

Comment thread airflow-core/src/airflow/models/trigger.py
potiuk and others added 4 commits August 5, 2026 15:21
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.
@vatsrahul1001
vatsrahul1001 force-pushed the fail-task-instance-with-unusable-next-kwargs branch from 0764deb to 738251c Compare August 5, 2026 09:55
@vatsrahul1001
vatsrahul1001 merged commit 2b7a0be into apache:main Aug 5, 2026
78 checks passed
@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Backport failed to create: v3-3-test. View the failure log Run details

Note: As of Merging PRs targeted for Airflow 3.X
the committer who merges the PR is responsible for backporting the PRs that are bug fixes (generally speaking) to the maintenance branches.

In matter of doubt please ask in #release-management Slack channel.

Status Branch Result
v3-3-test Commit Link

You can attempt to backport this manually by running:

cherry_picker 2b7a0be v3-3-test

This should apply the commit to the v3-3-test branch and leave the commit in conflict state marking
the files that need manual conflict resolution.

After you have resolved the conflicts, you can continue the backport process by running:

cherry_picker --continue

If you don't have cherry-picker installed, see the installation guide.

vatsrahul1001 added a commit that referenced this pull request Aug 5, 2026
) (#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>
vatsrahul1001 added a commit that referenced this pull request Aug 5, 2026
) (#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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:Scheduler including HA (high availability) scheduler area:Triggerer backport-to-v3-3-test Backport to v3-3-test

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants