kaxil commented on code in PR #66573:
URL: https://github.com/apache/airflow/pull/66573#discussion_r3262693277
##########
task-sdk/tests/task_sdk/execution_time/test_task_runner.py:
##########
@@ -475,6 +475,40 @@ def send_side_effect(msg=None, **kwargs):
assert error is None
+def test_run_exits_nonzero_when_failure_terminal_send_fails(create_runtime_ti,
mock_supervisor_comms):
Review Comment:
The test only asserts `SystemExit(1)`. It doesn't check that
`on_failure_callback` / listener `on_task_instance_failed` / `email_on_failure`
actually ran for the failed task. Given that `sys.exit(1)` from inside `run()`
bypasses `finalize()` in `main()` (SystemExit isn't caught by `except
Exception:`), a regression test that locks in the new behavior should also
verify the failure callbacks/listeners still fire, otherwise this change
silently drops them.
##########
task-sdk/tests/task_sdk/execution_time/test_task_runner.py:
##########
@@ -475,6 +475,40 @@ def send_side_effect(msg=None, **kwargs):
assert error is None
+def test_run_exits_nonzero_when_failure_terminal_send_fails(create_runtime_ti,
mock_supervisor_comms):
+ """
+ When the task FAILS and the terminal-state send to the supervisor fails too
+ (e.g. broken Unix socket / supervisor crashed / IPC channel dead), `run()`
+ must not silently swallow it: the supervisor's `final_state` property
+ defaults exit_code-0-with-no-terminal-state to SUCCESS, which would turn
+ a transient IPC blip into a silent data-quality bug downstream. Exit
+ non-zero so the supervisor classifies this as FAILED / UP_FOR_RETRY.
+ """
+ from airflow.sdk.execution_time.task_runner import run
Review Comment:
`run` is already imported at module level (line 163), so this inline import
is redundant. Same nit applies to
`test_run_swallows_supervisor_terminal_send_failure` above which has the same
pattern, but worth not propagating it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
