kaxil commented on code in PR #73249:
URL: https://github.com/apache/airflow/pull/73249#discussion_r4031672102
##########
task-sdk/src/airflow/sdk/execution_time/supervisor.py:
##########
@@ -1734,10 +1719,10 @@ def final_state(self):
Not valid before the process has finished.
"""
+ if self._terminal_state == SERVER_TERMINATED:
Review Comment:
Worth a note for later: `final_state` is still exit-code-derived while the
delivered report now uses `msg.state`, so the two can disagree. Harmless today,
since the `finish()` at the end of `update_task_state_if_needed` is unreachable
once a pending msg exists, and the only reader is the "Workload finished"
supervisor log (a task that reported FAILED and was SIGKILLed for overtime logs
`final_state=up_for_retry`). But the function now holds two notions of "the
final state" separated only by branch ordering, which is what the earlier
review caught. A docstring line saying this is exit-code-inferred and shouldn't
be consulted once a report exists would stop it coming back.
##########
task-sdk/tests/task_sdk/execution_time/test_supervisor.py:
##########
@@ -3608,30 +3720,27 @@ def
test_handle_requests_network_exception_does_not_crash_loop(self, watched_sub
),
],
)
- def test_terminal_state_not_set_when_direct_api_fails(
+ def test_worker_outcome_retained_when_direct_api_fails(
Review Comment:
`AwaitInputTask` is missing from this parametrize list, and from the replay
test below it, so nothing asserts `_terminal_state == AWAITING_INPUT` after the
switch to `msg.state`. Of the six types `_send_terminal_state_msg` dispatches,
it's the only one with no coverage here, and you already exercise it in
`test_server_termination_ignores_late_outcome`.
##########
task-sdk/tests/task_sdk/execution_time/test_supervisor.py:
##########
@@ -1088,6 +1088,9 @@ def mock_monotonic():
False,
id="no_terminal_state",
),
+ pytest.param(None, None, 10, False, id="execution_still_running"),
+ pytest.param(None, 9.0, 10, True,
id="finalization_without_reported_state"),
Review Comment:
These pass `terminal_state=None` with `_task_end_time_monotonic` set, which
`_handle_request` can't produce: `_send_terminal_state_msg` sets
`_terminal_state` before the API call that can raise, so a real failure always
leaves it non-None. The behaviour holds (I checked both trees), but the
"finalization stays bounded when the first call fails" case has no test on the
path that delivers it. Same reason `finalization_started_at_zero` doesn't
isolate the zero-timestamp truthiness fix: the old `if not
self._terminal_state` guard returns first, so it would need a reported state
plus a zero `task_end_time_monotonic`.
--
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]
