Repository navigation
Separate task outcomes from report delivery in TaskSDK Supervisor - #73249
Merged
Merged
Conversation
ashb
commented
Sep 16, 2026
jedcunningham
approved these changes
Sep 16, 2026
kaxil
approved these changes
Sep 16, 2026
kaxil
reviewed
Sep 16, 2026
ashb
force-pushed
the
supervisor-state-reporting
branch
from
September 22, 2026 14:20
fa5e82d to
9b40fb8
Compare
kaxil
approved these changes
Sep 22, 2026
ashb
force-pushed
the
supervisor-state-reporting
branch
2 times, most recently
from
September 23, 2026 14:31
eab420e to
831a790
Compare
The supervisor uses _terminal_state inconsistently: failed and skipped outcomes are recorded when the worker reports them, while other outcomes are recorded only after the API call succeeds. This couples the known task outcome to API availability and makes deferred reporting harder to reason about. This changes things so we record the worker’s outcome immediately on an instance field and use the existing pending message to track delivery. Failed API calls retain that message and keep heartbeats running. Upon task process exit pending reports take precedence over exit-code inference. Server-directed termination (zombie detection, external clearing, etc.) overrides pending reporting and any worker outcomes received during shutdown. The previous tests required _terminal_state to remain unset after an API failure because setting it stopped heartbeats. That assertion reflected the old coupling between outcome and delivery. The updated tests check the underlying guarantees: retain the report, continue heartbeats, and recover through the correct endpoint. Post-task finalization timeouts now depend on the recorded completion timestamp, independently of whether reporting succeeds. This also bounds finalization when the initial API call fails.
ashb
force-pushed
the
supervisor-state-reporting
branch
from
September 23, 2026 16:20
831a790 to
f8564c7
Compare
seanmuth
added a commit
to seanmuth/airflow
that referenced
this pull request
Sep 25, 2026
A subprocess can report a terminal state via message (SucceedTask, TaskState(SKIPPED), etc.) and then exit with a genuinely non-zero code afterward -- e.g. an OOM kill, or `_handle_process_overtime_if_needed()` sending SIGTERM once `_terminal_state` is already set. `final_state` previously derived its result from the exit code whenever one was observed, ignoring an already-confirmed terminal state. The clearest reachable case is SUCCESS: `SucceedTask` writes the row directly and clears the pending-message slot on success, so a later non-zero exit code re-triggers `update_task_state_if_needed()` -> `.finish()` against a row `succeed()` already wrote, which 409s. A terminal state can also be set directly by the supervisor rather than reported by the subprocess (SERVER_TERMINATED via `_send_heartbeat_if_needed`), so the precedence applies to any already-set `_terminal_state`, not only ones delivered by message -- this subsumes needing SERVER_TERMINATED/UP_FOR_RETRY called out as special cases, since every terminal state now gets the same precedence. Rebased over apache#73249/apache#73253's rework of pending-message delivery: those changes mean TaskState-reported outcomes (SKIPPED, FAILED, etc.) are now normally dispatched using the message's own state directly, without consulting `final_state` at all while a message is pending, so this precedence is defense-in-depth for that path rather than the reachable bug it was in the pre-rework code -- the SUCCESS case above remains the live, reachable one this fixes. Confirmed via live instrumentation against a reproduced crash, see apache#65708 (comment).
1 task done
seanmuth
added a commit
to seanmuth/airflow
that referenced
this pull request
Oct 28, 2026
A subprocess can report a terminal state via message (SucceedTask, TaskState(SKIPPED), etc.) and then exit with a genuinely non-zero code afterward -- e.g. an OOM kill, or `_handle_process_overtime_if_needed()` sending SIGTERM once `_terminal_state` is already set. `final_state` previously derived its result from the exit code whenever one was observed, ignoring an already-confirmed terminal state. The clearest reachable case is SUCCESS: `SucceedTask` writes the row directly and clears the pending-message slot on success, so a later non-zero exit code re-triggers `update_task_state_if_needed()` -> `.finish()` against a row `succeed()` already wrote, which 409s. A terminal state can also be set directly by the supervisor rather than reported by the subprocess (SERVER_TERMINATED via `_send_heartbeat_if_needed`), so the precedence applies to any already-set `_terminal_state`, not only ones delivered by message -- this subsumes needing SERVER_TERMINATED/UP_FOR_RETRY called out as special cases, since every terminal state now gets the same precedence. Rebased over apache#73249/apache#73253's rework of pending-message delivery: those changes mean TaskState-reported outcomes (SKIPPED, FAILED, etc.) are now normally dispatched using the message's own state directly, without consulting `final_state` at all while a message is pending, so this precedence is defense-in-depth for that path rather than the reachable bug it was in the pre-rework code -- the SUCCESS case above remains the live, reachable one this fixes. Confirmed via live instrumentation against a reproduced crash, see apache#65708 (comment).
kaxil
pushed a commit
to seanmuth/airflow
that referenced
this pull request
Nov 4, 2026
A subprocess can report a terminal state via message (SucceedTask, TaskState(SKIPPED), etc.) and then exit with a genuinely non-zero code afterward -- e.g. an OOM kill, or `_handle_process_overtime_if_needed()` sending SIGTERM once `_terminal_state` is already set. `final_state` previously derived its result from the exit code whenever one was observed, ignoring an already-confirmed terminal state. The clearest reachable case is SUCCESS: `SucceedTask` writes the row directly and clears the pending-message slot on success, so a later non-zero exit code re-triggers `update_task_state_if_needed()` -> `.finish()` against a row `succeed()` already wrote, which 409s. A terminal state can also be set directly by the supervisor rather than reported by the subprocess (SERVER_TERMINATED via `_send_heartbeat_if_needed`), so the precedence applies to any already-set `_terminal_state`, not only ones delivered by message -- this subsumes needing SERVER_TERMINATED/UP_FOR_RETRY called out as special cases, since every terminal state now gets the same precedence. Rebased over apache#73249/apache#73253's rework of pending-message delivery: those changes mean TaskState-reported outcomes (SKIPPED, FAILED, etc.) are now normally dispatched using the message's own state directly, without consulting `final_state` at all while a message is pending, so this precedence is defense-in-depth for that path rather than the reachable bug it was in the pre-rework code -- the SUCCESS case above remains the live, reachable one this fixes. Confirmed via live instrumentation against a reproduced crash, see apache#65708 (comment).
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
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.
The supervisor uses _terminal_state inconsistently: failed and skipped
outcomes are recorded when the worker reports them, while other outcomes
are recorded only after the API call succeeds. This couples the known
task outcome to API availability and makes deferred reporting harder to
reason about.
This changes things so we record the worker’s outcome immediately on an
instance field and use the existing pending message to track delivery.
Failed API calls retain that message and keep heartbeats running. Upon
task process exit pending reports take precedence over exit-code
inference. Server-directed termination (zombie detection, external
clearing, etc.) overrides pending reporting and any worker outcomes
received during shutdown.
The previous tests required _terminal_state to remain unset after an API
failure because setting it stopped heartbeats. That assertion reflected
the old coupling between outcome and delivery. The updated tests check
the underlying guarantees: retain the report, continue heartbeats, and
recover through the correct endpoint.
Post-task finalization timeouts now depend on the recorded completion
timestamp, independently of whether reporting succeeds. This also bounds
finalization when the initial API call fails.
Was generative AI tooling used to co-author this PR?
{pr_number}.significant.rst, in airflow-core/newsfragments. You can add this file in a follow-up commit after the PR is created so you know the PR number.