itz-puneet opened a new pull request, #72606:
URL: https://github.com/apache/airflow/pull/72606
closes: #71202
### The bug
Under `KubernetesExecutor` there are two pods: the per-task pod the executor
creates, and the child
pod `KubernetesPodOperator.execute()` creates for the user's workload. When
both are interrupted
within about a second of each other, the task instance is recorded as
**success** even though the
child pod never finished. A second user confirmed the same behaviour on
their production clusters in
the issue thread.
The sequence:
1. SIGTERM reaches the task process, and the runner calls `on_kill()`.
2. `on_kill()` sets `self._killed` and deletes the child pod.
3. The wait inside `execute_sync` — `await_pod_completion`, following
container logs — returns
*normally* rather than raising, because the pod it was watching has
simply gone away.
4. The `finally` block runs `post_complete_action` → `cleanup`, and
`cleanup` returns early on
`self._killed` before reaching any of its failure signalling.
5. `execute_sync` falls through to its `return`, so the operator reports
success.
`cleanup`'s early return is correct on its own terms — it exists so the pod
is not deleted a second
time, since a duplicate delete can raise and trigger a spurious retry. The
gap is that skipping the
deletion also skips the only place that would have failed the task.
### The fix
Raise from `execute_sync` when the body completed while `_killed` is set.
The placement matters and is the reason this is not done inside `cleanup`:
`cleanup` is called from a
`finally` block, so raising there would replace an exception that is already
propagating. When the
body failed on its own, that failure is what the operator should surface —
it already fails the task,
with a more specific reason. Putting the check after the `finally` means it
only fires on the path
that would otherwise have returned success. `cleanup` keeps its early return
unchanged, so the
double-delete it guards against still cannot happen.
### Tests
Two regression tests in `test_pod.py`:
- `test_execute_sync_fails_when_on_kill_ran_during_the_wait` — drives the
exact sequence above, with
the wait returning normally after `on_kill()` fires. Fails on `main` (the
call returns instead of
raising), passes with this change.
- `test_execute_sync_keeps_the_original_error_when_on_kill_ran` — the body
raises after `on_kill()`;
asserts the original exception still propagates rather than being
replaced. This is what pins the
placement of the check.
Neither test needs a cluster.
---
**Gen-AI disclosure:** I used an AI coding assistant while working on this.
I traced the interrupted
path through `execute_sync`, `cleanup` and `on_kill` myself, modelled the
control flow before and
after the change to confirm the success path is the only one that changes
behaviour, and reviewed
the final diff before submitting.
--
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]
