Repository navigation
Fail a task whose execute is interrupted by SIGTERM - #73395
namanjain24-sudo wants to merge 2 commits into
Conversation
The task runner's SIGTERM handler called on_kill() and returned, so execute() resumed from where the signal interrupted it and ran to completion. An operator whose on_kill destroys the work it is waiting on then returned normally and the task instance was committed success. KubernetesPodOperator deletes its pod in on_kill and its cleanup() declines to raise because _killed is set, so a pod evicted mid-run, or a worker that is drained, produced a green task whose work never happened. AirflowTaskTerminated was already caught in the runner but nothing raised it anywhere; the raise was dropped when the handler was ported. It is raised again, after on_kill, but only while the task's own body is on the stack: the supervisor also sends SIGTERM once a task has reported success and is in task_success_overtime, and that must not turn a finished task into a failed one. A second SIGTERM arriving while the first is unwinding is ignored, so on_kill runs once rather than re-entering the handler, and an on_kill that raises no longer hides the termination. Closes apache#73006
|
Hello @namanjain24-sudo - thank you for your contributions to Apache Airflow! The Airflow community has introduced a limit of 5 open pull requests at a time for contributors without write access to the repository. You currently have 7 open pull requests, so - as a one-time step of introducing the limit - we closed the ones where maintainers have not engaged yet:
These pull requests stay open because maintainers are already engaged in them - they count towards your limit:
This is not a judgement of you or of your changes. We never told contributors before that opening many pull requests at once was a problem, so there is nothing to feel bad about - and nothing is lost: your branches, commits and the review history stay where they are. What we ask you to do is to make your first prioritization decision: choose which of the pull requests above matter most to you, and reopen them (up to 5 open at a time, including the ones still open) with the "Reopen pull request" button or While your pull requests are waiting for review, the most valuable thing you can do is help in other ways - reviewing other contributors' pull requests, helping with issues, and taking part in the discussions on the devlist and Slack. Why we introduced the limit, what it means for you and how to reopen or restore a pull request is explained in https://github.com/apache/airflow/blob/main/contributing-docs/32_open_pull_request_limit.rst. Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting |
|
Thanks for the explanation and for the one-time cleanup — makes sense. I've made my prioritization decision — I'd like to reopen these four:
However, both Drafted-by: Claude Code (Sonnet 5) (no human review before posting) |
PR apache#73395 was closed under the open-PR-count cap for non-committers and failed to reopen, so the same branch was pushed as a fresh PR (apache#73809). The newsfragment filename still carried the old PR number, which fails the check-newsfragment-pr-number check on the new PR.
PR apache#73395 was closed under the open-PR-count cap for non-committers and failed to reopen, so the same branch was pushed as a fresh PR (apache#73809). The newsfragment filename still carried the old PR number, which fails the check-newsfragment-pr-number check on the new PR.
closes: #73006What happened
The task runner installs a
SIGTERMhandler that callson_kill()and returns:Nothing raises, so
execute()resumes from wherever the signal interrupted it and runsto its end. For an operator whose
on_killdestroys the work it is waiting on, theoperator then returns normally and the task instance is committed
successeventhough the work never happened.
KubernetesPodOperatoris the reported case:on_killdeletes the pod, andcleanup()— the only place that raises for a pod that did not reach
Succeeded— returns earlybecause
_killedis set. That guard comes from #36749, whose premise is that somethingelse is already marking the task failed. That holds when a user marks a task failed in
the UI. It does not hold when the
SIGTERMcomes from a pod eviction, a drained node ora restarted worker, so nothing fails the task at all.
AirflowTaskTerminatedis already caught in_run_task_and_map_outcome, but greppingthe tree shows it is never raised anywhere — the raise was dropped when the handler was
ported in #50141, and only the
exceptsurvived.Reproduced on
mainat61db43fba9with the operator shape from the issue: the taskinstance ends
state=TaskInstanceState.SUCCESSandexecute()is observed running pastthe point where the signal arrived.
What is in this PR
_on_termraisesAirflowTaskTerminatedafteron_kill(), with two guards:SIGTERMonce a task has already reported success and is sitting in
task_success_overtime, andalso when the server says the task should no longer be running. Raising there would turn
a finished task into a failed one and lose a result the server has already accepted, so
outside that window the handler keeps today's behaviour exactly. The window is closed in
a
finally, so the paths that leave_execute_taskby raising aBaseException—
AirflowTaskTimeout,SystemExit— leave it too.SIGTERMarriving while the first is still unwinding is ignored.Without this, an
on_killthat is itself interrupted re-enters the handler: the testfor it records 478
on_killcalls onmainagainst 1 with this change.An
on_killthat raises no longer hides the termination either — the raise is in afinally, so the task still ends asAirflowTaskTerminatedrather than as whateveron_killfailed with.This is deliberately in the task runner rather than in
KubernetesPodOperator. Themissing raise is the regression, and it affects any operator whose
on_killtears downthe work in flight; fixing
cleanup()alone would leave that hole open for the rest.Testing
TestSigtermTerminatesTaskintask-sdk/tests/task_sdk/execution_time/test_task_runner.py,six cases: a
SIGTERMmid-execute; theKubernetesPodOperatorshape, with anon_killthat sets
_killedand acleanup()that therefore declines to raise; a secondSIGTERMdelivered from inside
on_kill; anon_killthat raises; and two negative controls — aSIGTERMdelivered afterexecute()has returned, which must staysuccess, and a taskthat gets no signal at all.
Ablation, with only
task_runner.pyreverted tomainand the tests kept (they live in adifferent file, so the revert does not delete them): the four positive cases fail and the
two controls pass. On
mainthe first case reportsstate=SUCCESSwithexecute()havingrun to its end, the pod-shaped one reports
SUCCESS, and the re-signalling one reports 478on_killcalls.test_task_runner.pyandtest_supervisor.pywere run with and without the change and thefailure sets compared: no regressions. 15 tests fail identically either way in this
local environment (8
TestTriggerDagRunOperator, 7 intest_supervisor.py); the onlydifference between the two runs is the four new tests.
ruff check,ruff format --checkandmypyare clean on both changed files. Threepre-push hooks could not run locally —
generate-openapi-spec,check-default-configurationand provider
mypyall need a Breeze CI image, and the local Docker daemon fails to buildone; they touch no file in this PR and will run in CI.
Behaviour change
A task whose process is sent
SIGTERMwhileexecute()is running now ends failedinstead of finishing normally. That is the point of the fix, but it is a visible change for
anyone whose tasks were quietly being marked successful after an eviction.
AirflowTaskTerminatedis classified as terminal today, so such a task fails withoutconsuming a retry. Whether an infrastructure kill should instead be retry-eligible is a
real question — it is what #73238 is about — but changing that classification also changes
the UI "mark failed" path, so it is left alone here.
Was generative AI tooling used to co-author this PR?
Generative AI tooling was used while authoring this PR, following the guidelines. Every claim above — the reproduction, the ablation and the before/after numbers — was produced by running the tests, not by the model asserting them.