Uh oh!
There was an error while loading. Please reload this page.
Fail closed when supervisor IPC fails on a non-success terminal state - #66573
Conversation
607e582 to
2acf7f0Comparepotiuk
commented
May 17, 2026
I'd love to get this one merged — and would love it in 3.2.2 if it's not too late. cc @vatsrahul1001 (3.2.2 RM) Drafted-by: Claude Code (Opus 4.7); reviewed by @potiuk before posting |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
vatsrahul1001
commented
May 18, 2026
@potiuk can you address comments? |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
When a task FAILED / SKIPPED / etc. and the IPC send of the terminal-state message to the supervisor itself raised, the existing finally block logged the exception and let run() return normally. The task subprocess then exited with code 0, which the supervisor final_state property maps to SUCCESS for an exit_code-0 process without a _terminal_state (the supervisor never received the message). A genuine task FAILURE was silently being upgraded to SUCCESS on transient IPC failures, breaking downstream pipeline correctness without any signal. Exit non-zero from the finally block when the terminal state is anything other than SUCCESS, so the supervisor's final_state correctly classifies as FAILED (or UP_FOR_RETRY when retries are configured). The SUCCESS exemption preserves the existing softening for the legitimate scenario where the supervisor rejects the terminal-state send with a 409 because the server already terminalised the TI -- covered by test_run_swallows_supervisor_terminal_send_failure, which continues to pass. New regression test: when the task fails and the supervisor IPC send raises (BrokenPipeError simulating a dead Unix socket), run() now raises SystemExit(1). Reported by the L3 ASVS sweep at apache/tooling-agents#24 (FINDING-006).
- Narrow the fail-closed guard from `state != SUCCESS` to FAILED / UP_FOR_RETRY only (kaxil). SKIPPED / UP_FOR_RESCHEDULE / DEFERRED would otherwise be mismapped to FAILED by supervisor's final_state, which is strictly worse than the default mapping. - Stop calling sys.exit(1) inside run()'s finally — SystemExit is a BaseException so main()'s `except Exception:` would not catch it and finalize() would be skipped, silently dropping on_failure_callback / on_retry_callback / listener hooks / email_on_failure / email_on_retry on the same IPC failure (kaxil). Signal via `_terminal_state_send_failed` on the ti and let main() sys.exit(1) after finalize() has run. - Remove the redundant inline `from ... import run` in the test (Lee-W, kaxil) — `run` is already imported at module level. - Rework the existing regression test to assert the new contract (run() returns FAILED + sets the flag) and add a listener-based test that locks in callbacks/listeners still firing on the IPC-broken path (kaxil).
The fail-closed path in _handle_current_task_failure set ti._terminal_state_send_failed = True dynamically without a class-level declaration, so mypy raised [attr-defined]. Add the field as a Pydantic PrivateAttr (bool, default False) matching the existing _cached_template_context pattern. No runtime behavior change — getattr(ti, '_terminal_state_send_failed', False) still returns False for the unset case, now via the PrivateAttr default instead of the getattr fallback.
be3187f to
d1a32abCompareUh oh!
There was an error while loading. Please reload this page.
Backport successfully created: v3-2-testNote: As of Merging PRs targeted for Airflow 3.X In matter of doubt please ask in #release-management Slack channel.
|
…rminal state (apache#66573) * Fail closed when supervisor IPC fails on a non-success terminal state When a task FAILED / SKIPPED / etc. and the IPC send of the terminal-state message to the supervisor itself raised, the existing finally block logged the exception and let run() return normally. The task subprocess then exited with code 0, which the supervisor final_state property maps to SUCCESS for an exit_code-0 process without a _terminal_state (the supervisor never received the message). A genuine task FAILURE was silently being upgraded to SUCCESS on transient IPC failures, breaking downstream pipeline correctness without any signal. Exit non-zero from the finally block when the terminal state is anything other than SUCCESS, so the supervisor's final_state correctly classifies as FAILED (or UP_FOR_RETRY when retries are configured). The SUCCESS exemption preserves the existing softening for the legitimate scenario where the supervisor rejects the terminal-state send with a 409 because the server already terminalised the TI -- covered by test_run_swallows_supervisor_terminal_send_failure, which continues to pass. New regression test: when the task fails and the supervisor IPC send raises (BrokenPipeError simulating a dead Unix socket), run() now raises SystemExit(1). Reported by the L3 ASVS sweep at apache/tooling-agents#24 (FINDING-006). * Address review comments: defer fail-closed exit, narrow guard - Narrow the fail-closed guard from `state != SUCCESS` to FAILED / UP_FOR_RETRY only (kaxil). SKIPPED / UP_FOR_RESCHEDULE / DEFERRED would otherwise be mismapped to FAILED by supervisor's final_state, which is strictly worse than the default mapping. - Stop calling sys.exit(1) inside run()'s finally — SystemExit is a BaseException so main()'s `except Exception:` would not catch it and finalize() would be skipped, silently dropping on_failure_callback / on_retry_callback / listener hooks / email_on_failure / email_on_retry on the same IPC failure (kaxil). Signal via `_terminal_state_send_failed` on the ti and let main() sys.exit(1) after finalize() has run. - Remove the redundant inline `from ... import run` in the test (Lee-W, kaxil) — `run` is already imported at module level. - Rework the existing regression test to assert the new contract (run() returns FAILED + sets the flag) and add a listener-based test that locks in callbacks/listeners still firing on the IPC-broken path (kaxil). * Declare _terminal_state_send_failed on RuntimeTaskInstance for mypy The fail-closed path in _handle_current_task_failure set ti._terminal_state_send_failed = True dynamically without a class-level declaration, so mypy raised [attr-defined]. Add the field as a Pydantic PrivateAttr (bool, default False) matching the existing _cached_template_context pattern. No runtime behavior change — getattr(ti, '_terminal_state_send_failed', False) still returns False for the unset case, now via the PrivateAttr default instead of the getattr fallback. --------- (cherry picked from commit 7e91517) Co-authored-by: Jarek Potiuk <jarek@potiuk.com> Co-authored-by: vatsrahul1001 <rah.sharma11@gmail.com>
…rminal state (apache#66573) * Fail closed when supervisor IPC fails on a non-success terminal state When a task FAILED / SKIPPED / etc. and the IPC send of the terminal-state message to the supervisor itself raised, the existing finally block logged the exception and let run() return normally. The task subprocess then exited with code 0, which the supervisor final_state property maps to SUCCESS for an exit_code-0 process without a _terminal_state (the supervisor never received the message). A genuine task FAILURE was silently being upgraded to SUCCESS on transient IPC failures, breaking downstream pipeline correctness without any signal. Exit non-zero from the finally block when the terminal state is anything other than SUCCESS, so the supervisor's final_state correctly classifies as FAILED (or UP_FOR_RETRY when retries are configured). The SUCCESS exemption preserves the existing softening for the legitimate scenario where the supervisor rejects the terminal-state send with a 409 because the server already terminalised the TI -- covered by test_run_swallows_supervisor_terminal_send_failure, which continues to pass. New regression test: when the task fails and the supervisor IPC send raises (BrokenPipeError simulating a dead Unix socket), run() now raises SystemExit(1). Reported by the L3 ASVS sweep at apache/tooling-agents#24 (FINDING-006). * Address review comments: defer fail-closed exit, narrow guard - Narrow the fail-closed guard from `state != SUCCESS` to FAILED / UP_FOR_RETRY only (kaxil). SKIPPED / UP_FOR_RESCHEDULE / DEFERRED would otherwise be mismapped to FAILED by supervisor's final_state, which is strictly worse than the default mapping. - Stop calling sys.exit(1) inside run()'s finally — SystemExit is a BaseException so main()'s `except Exception:` would not catch it and finalize() would be skipped, silently dropping on_failure_callback / on_retry_callback / listener hooks / email_on_failure / email_on_retry on the same IPC failure (kaxil). Signal via `_terminal_state_send_failed` on the ti and let main() sys.exit(1) after finalize() has run. - Remove the redundant inline `from ... import run` in the test (Lee-W, kaxil) — `run` is already imported at module level. - Rework the existing regression test to assert the new contract (run() returns FAILED + sets the flag) and add a listener-based test that locks in callbacks/listeners still firing on the IPC-broken path (kaxil). * Declare _terminal_state_send_failed on RuntimeTaskInstance for mypy The fail-closed path in _handle_current_task_failure set ti._terminal_state_send_failed = True dynamically without a class-level declaration, so mypy raised [attr-defined]. Add the field as a Pydantic PrivateAttr (bool, default False) matching the existing _cached_template_context pattern. No runtime behavior change — getattr(ti, '_terminal_state_send_failed', False) still returns False for the unset case, now via the PrivateAttr default instead of the getattr fallback. --------- (cherry picked from commit 7e91517) Co-authored-by: Jarek Potiuk <jarek@potiuk.com> Co-authored-by: vatsrahul1001 <rah.sharma11@gmail.com>
…rminal state (#66573) (#67183) * Fail closed when supervisor IPC fails on a non-success terminal state When a task FAILED / SKIPPED / etc. and the IPC send of the terminal-state message to the supervisor itself raised, the existing finally block logged the exception and let run() return normally. The task subprocess then exited with code 0, which the supervisor final_state property maps to SUCCESS for an exit_code-0 process without a _terminal_state (the supervisor never received the message). A genuine task FAILURE was silently being upgraded to SUCCESS on transient IPC failures, breaking downstream pipeline correctness without any signal. Exit non-zero from the finally block when the terminal state is anything other than SUCCESS, so the supervisor's final_state correctly classifies as FAILED (or UP_FOR_RETRY when retries are configured). The SUCCESS exemption preserves the existing softening for the legitimate scenario where the supervisor rejects the terminal-state send with a 409 because the server already terminalised the TI -- covered by test_run_swallows_supervisor_terminal_send_failure, which continues to pass. New regression test: when the task fails and the supervisor IPC send raises (BrokenPipeError simulating a dead Unix socket), run() now raises SystemExit(1). Reported by the L3 ASVS sweep at apache/tooling-agents#24 (FINDING-006). * Address review comments: defer fail-closed exit, narrow guard - Narrow the fail-closed guard from `state != SUCCESS` to FAILED / UP_FOR_RETRY only (kaxil). SKIPPED / UP_FOR_RESCHEDULE / DEFERRED would otherwise be mismapped to FAILED by supervisor's final_state, which is strictly worse than the default mapping. - Stop calling sys.exit(1) inside run()'s finally — SystemExit is a BaseException so main()'s `except Exception:` would not catch it and finalize() would be skipped, silently dropping on_failure_callback / on_retry_callback / listener hooks / email_on_failure / email_on_retry on the same IPC failure (kaxil). Signal via `_terminal_state_send_failed` on the ti and let main() sys.exit(1) after finalize() has run. - Remove the redundant inline `from ... import run` in the test (Lee-W, kaxil) — `run` is already imported at module level. - Rework the existing regression test to assert the new contract (run() returns FAILED + sets the flag) and add a listener-based test that locks in callbacks/listeners still firing on the IPC-broken path (kaxil). * Declare _terminal_state_send_failed on RuntimeTaskInstance for mypy The fail-closed path in _handle_current_task_failure set ti._terminal_state_send_failed = True dynamically without a class-level declaration, so mypy raised [attr-defined]. Add the field as a Pydantic PrivateAttr (bool, default False) matching the existing _cached_template_context pattern. No runtime behavior change — getattr(ti, '_terminal_state_send_failed', False) still returns False for the unset case, now via the PrivateAttr default instead of the getattr fallback. --------- (cherry picked from commit 7e91517) Co-authored-by: Jarek Potiuk <jarek@potiuk.com> Co-authored-by: vatsrahul1001 <rah.sharma11@gmail.com>
…rminal state (#66573) (#67183) * Fail closed when supervisor IPC fails on a non-success terminal state When a task FAILED / SKIPPED / etc. and the IPC send of the terminal-state message to the supervisor itself raised, the existing finally block logged the exception and let run() return normally. The task subprocess then exited with code 0, which the supervisor final_state property maps to SUCCESS for an exit_code-0 process without a _terminal_state (the supervisor never received the message). A genuine task FAILURE was silently being upgraded to SUCCESS on transient IPC failures, breaking downstream pipeline correctness without any signal. Exit non-zero from the finally block when the terminal state is anything other than SUCCESS, so the supervisor's final_state correctly classifies as FAILED (or UP_FOR_RETRY when retries are configured). The SUCCESS exemption preserves the existing softening for the legitimate scenario where the supervisor rejects the terminal-state send with a 409 because the server already terminalised the TI -- covered by test_run_swallows_supervisor_terminal_send_failure, which continues to pass. New regression test: when the task fails and the supervisor IPC send raises (BrokenPipeError simulating a dead Unix socket), run() now raises SystemExit(1). Reported by the L3 ASVS sweep at apache/tooling-agents#24 (FINDING-006). * Address review comments: defer fail-closed exit, narrow guard - Narrow the fail-closed guard from `state != SUCCESS` to FAILED / UP_FOR_RETRY only (kaxil). SKIPPED / UP_FOR_RESCHEDULE / DEFERRED would otherwise be mismapped to FAILED by supervisor's final_state, which is strictly worse than the default mapping. - Stop calling sys.exit(1) inside run()'s finally — SystemExit is a BaseException so main()'s `except Exception:` would not catch it and finalize() would be skipped, silently dropping on_failure_callback / on_retry_callback / listener hooks / email_on_failure / email_on_retry on the same IPC failure (kaxil). Signal via `_terminal_state_send_failed` on the ti and let main() sys.exit(1) after finalize() has run. - Remove the redundant inline `from ... import run` in the test (Lee-W, kaxil) — `run` is already imported at module level. - Rework the existing regression test to assert the new contract (run() returns FAILED + sets the flag) and add a listener-based test that locks in callbacks/listeners still firing on the IPC-broken path (kaxil). * Declare _terminal_state_send_failed on RuntimeTaskInstance for mypy The fail-closed path in _handle_current_task_failure set ti._terminal_state_send_failed = True dynamically without a class-level declaration, so mypy raised [attr-defined]. Add the field as a Pydantic PrivateAttr (bool, default False) matching the existing _cached_template_context pattern. No runtime behavior change — getattr(ti, '_terminal_state_send_failed', False) still returns False for the unset case, now via the PrivateAttr default instead of the getattr fallback. --------- (cherry picked from commit 7e91517) Co-authored-by: Jarek Potiuk <jarek@potiuk.com> Co-authored-by: vatsrahul1001 <rah.sharma11@gmail.com>
…rminal state (#66573) (#67183) * Fail closed when supervisor IPC fails on a non-success terminal state When a task FAILED / SKIPPED / etc. and the IPC send of the terminal-state message to the supervisor itself raised, the existing finally block logged the exception and let run() return normally. The task subprocess then exited with code 0, which the supervisor final_state property maps to SUCCESS for an exit_code-0 process without a _terminal_state (the supervisor never received the message). A genuine task FAILURE was silently being upgraded to SUCCESS on transient IPC failures, breaking downstream pipeline correctness without any signal. Exit non-zero from the finally block when the terminal state is anything other than SUCCESS, so the supervisor's final_state correctly classifies as FAILED (or UP_FOR_RETRY when retries are configured). The SUCCESS exemption preserves the existing softening for the legitimate scenario where the supervisor rejects the terminal-state send with a 409 because the server already terminalised the TI -- covered by test_run_swallows_supervisor_terminal_send_failure, which continues to pass. New regression test: when the task fails and the supervisor IPC send raises (BrokenPipeError simulating a dead Unix socket), run() now raises SystemExit(1). Reported by the L3 ASVS sweep at apache/tooling-agents#24 (FINDING-006). * Address review comments: defer fail-closed exit, narrow guard - Narrow the fail-closed guard from `state != SUCCESS` to FAILED / UP_FOR_RETRY only (kaxil). SKIPPED / UP_FOR_RESCHEDULE / DEFERRED would otherwise be mismapped to FAILED by supervisor's final_state, which is strictly worse than the default mapping. - Stop calling sys.exit(1) inside run()'s finally — SystemExit is a BaseException so main()'s `except Exception:` would not catch it and finalize() would be skipped, silently dropping on_failure_callback / on_retry_callback / listener hooks / email_on_failure / email_on_retry on the same IPC failure (kaxil). Signal via `_terminal_state_send_failed` on the ti and let main() sys.exit(1) after finalize() has run. - Remove the redundant inline `from ... import run` in the test (Lee-W, kaxil) — `run` is already imported at module level. - Rework the existing regression test to assert the new contract (run() returns FAILED + sets the flag) and add a listener-based test that locks in callbacks/listeners still firing on the IPC-broken path (kaxil). * Declare _terminal_state_send_failed on RuntimeTaskInstance for mypy The fail-closed path in _handle_current_task_failure set ti._terminal_state_send_failed = True dynamically without a class-level declaration, so mypy raised [attr-defined]. Add the field as a Pydantic PrivateAttr (bool, default False) matching the existing _cached_template_context pattern. No runtime behavior change — getattr(ti, '_terminal_state_send_failed', False) still returns False for the unset case, now via the PrivateAttr default instead of the getattr fallback. --------- (cherry picked from commit 7e91517) Co-authored-by: Jarek Potiuk <jarek@potiuk.com> Co-authored-by: vatsrahul1001 <rah.sharma11@gmail.com>
…rminal state (#66573) (#67183) * Fail closed when supervisor IPC fails on a non-success terminal state When a task FAILED / SKIPPED / etc. and the IPC send of the terminal-state message to the supervisor itself raised, the existing finally block logged the exception and let run() return normally. The task subprocess then exited with code 0, which the supervisor final_state property maps to SUCCESS for an exit_code-0 process without a _terminal_state (the supervisor never received the message). A genuine task FAILURE was silently being upgraded to SUCCESS on transient IPC failures, breaking downstream pipeline correctness without any signal. Exit non-zero from the finally block when the terminal state is anything other than SUCCESS, so the supervisor's final_state correctly classifies as FAILED (or UP_FOR_RETRY when retries are configured). The SUCCESS exemption preserves the existing softening for the legitimate scenario where the supervisor rejects the terminal-state send with a 409 because the server already terminalised the TI -- covered by test_run_swallows_supervisor_terminal_send_failure, which continues to pass. New regression test: when the task fails and the supervisor IPC send raises (BrokenPipeError simulating a dead Unix socket), run() now raises SystemExit(1). Reported by the L3 ASVS sweep at apache/tooling-agents#24 (FINDING-006). * Address review comments: defer fail-closed exit, narrow guard - Narrow the fail-closed guard from `state != SUCCESS` to FAILED / UP_FOR_RETRY only (kaxil). SKIPPED / UP_FOR_RESCHEDULE / DEFERRED would otherwise be mismapped to FAILED by supervisor's final_state, which is strictly worse than the default mapping. - Stop calling sys.exit(1) inside run()'s finally — SystemExit is a BaseException so main()'s `except Exception:` would not catch it and finalize() would be skipped, silently dropping on_failure_callback / on_retry_callback / listener hooks / email_on_failure / email_on_retry on the same IPC failure (kaxil). Signal via `_terminal_state_send_failed` on the ti and let main() sys.exit(1) after finalize() has run. - Remove the redundant inline `from ... import run` in the test (Lee-W, kaxil) — `run` is already imported at module level. - Rework the existing regression test to assert the new contract (run() returns FAILED + sets the flag) and add a listener-based test that locks in callbacks/listeners still firing on the IPC-broken path (kaxil). * Declare _terminal_state_send_failed on RuntimeTaskInstance for mypy The fail-closed path in _handle_current_task_failure set ti._terminal_state_send_failed = True dynamically without a class-level declaration, so mypy raised [attr-defined]. Add the field as a Pydantic PrivateAttr (bool, default False) matching the existing _cached_template_context pattern. No runtime behavior change — getattr(ti, '_terminal_state_send_failed', False) still returns False for the unset case, now via the PrivateAttr default instead of the getattr fallback. --------- (cherry picked from commit 7e91517) Co-authored-by: Jarek Potiuk <jarek@potiuk.com> Co-authored-by: vatsrahul1001 <rah.sharma11@gmail.com>
Summary
When a task FAILED / SKIPPED / etc. and the IPC send of the terminal-state message to the supervisor itself raised, the existing
finallyblock logged the exception and letrun()return normally. The task subprocess then exited with code 0, which the supervisor'sfinal_stateproperty maps to SUCCESS for anexit_code == 0process without a_terminal_state(the supervisor never received the message).A genuine task FAILURE was silently being upgraded to SUCCESS on transient IPC failures, breaking downstream pipeline correctness without any signal.
Fix
Exit non-zero from the
finallyblock when the terminal state is anything other than SUCCESS, so the supervisor'sfinal_statecorrectly classifies as FAILED (or UP_FOR_RETRY when retries are configured).The SUCCESS exemption preserves the existing softening for the legitimate scenario where the supervisor rejects the terminal-state send with a 409 because the server already terminalised the TI — covered by
test_run_swallows_supervisor_terminal_send_failure, which continues to pass.Test
New regression test: when the task fails and the supervisor IPC
sendraises (BrokenPipeErrorsimulating a dead Unix socket),run()raisesSystemExit(1).Reported by
L3 ASVS sweep — apache/tooling-agents#24 (FINDING-006).
Was generative AI tooling used to co-author this PR?
Generated-by: Claude Code (Opus 4.7) following the guidelines