Skip to content

Fail a task whose execute is interrupted by SIGTERM - #73395

Closed
namanjain24-sudo wants to merge 2 commits into
apache:mainfrom
namanjain24-sudo:fix-sigterm-marks-task-success
Closed

namanjain24-sudo wants to merge 2 commits into
apache:mainfrom
namanjain24-sudo:fix-sigterm-marks-task-success

Conversation

@namanjain24-sudo

Copy link
Copy Markdown
Contributor

closes: #73006

What happened

The task runner installs a SIGTERM handler that calls on_kill() and returns:

def _on_term(signum, frame):
    pid = os.getpid()
    if pid != parent_pid:
        return

    ti.task.on_kill()

Nothing raises, so execute() resumes from wherever the signal interrupted it and runs
to its end. For an operator whose on_kill destroys the work it is waiting on, the
operator then returns normally and the task instance is committed success even
though the work never happened.

KubernetesPodOperator is the reported case: on_kill deletes the pod, and cleanup()
— the only place that raises for a pod that did not reach Succeeded — returns early
because _killed is set. That guard comes from #36749, whose premise is that something
else is already marking the task failed. That holds when a user marks a task failed in
the UI. It does not hold when the SIGTERM comes from a pod eviction, a drained node or
a restarted worker, so nothing fails the task at all.

AirflowTaskTerminated is already caught in _run_task_and_map_outcome, but grepping
the tree shows it is never raised anywhere — the raise was dropped when the handler was
ported in #50141, and only the except survived.

Reproduced on main at 61db43fba9 with the operator shape from the issue: the task
instance ends state=TaskInstanceState.SUCCESS and execute() is observed running past
the point where the signal arrived.

What is in this PR

_on_term raises AirflowTaskTerminated after on_kill(), with two guards:

  • Only while the task's own body is on the stack. The supervisor also sends SIGTERM
    once a task has already reported success and is sitting in task_success_overtime, and
    also 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_task by raising a BaseException
    — AirflowTaskTimeout, SystemExit — leave it too.
  • Once. A second SIGTERM arriving while the first is still unwinding is ignored.
    Without this, an on_kill that is itself interrupted re-enters the handler: the test
    for it records 478 on_kill calls on main against 1 with this change.

An on_kill that raises no longer hides the termination either — the raise is in a
finally, so the task still ends as AirflowTaskTerminated rather than as whatever
on_kill failed with.

This is deliberately in the task runner rather than in KubernetesPodOperator. The
missing raise is the regression, and it affects any operator whose on_kill tears down
the work in flight; fixing cleanup() alone would leave that hole open for the rest.

Testing

TestSigtermTerminatesTask in task-sdk/tests/task_sdk/execution_time/test_task_runner.py,
six cases: a SIGTERM mid-execute; the KubernetesPodOperator shape, with an on_kill
that sets _killed and a cleanup() that therefore declines to raise; a second SIGTERM
delivered from inside on_kill; an on_kill that raises; and two negative controls — a
SIGTERM delivered after execute() has returned, which must stay success, and a task
that gets no signal at all.

Ablation, with only task_runner.py reverted to main and the tests kept (they live in a
different file, so the revert does not delete them): the four positive cases fail and the
two controls pass. On main the first case reports state=SUCCESS with execute() having
run to its end, the pod-shaped one reports SUCCESS, and the re-signalling one reports 478
on_kill calls.

test_task_runner.py and test_supervisor.py were run with and without the change and the
failure sets compared: no regressions. 15 tests fail identically either way in this
local environment (8 TestTriggerDagRunOperator, 7 in test_supervisor.py); the only
difference between the two runs is the four new tests.

ruff check, ruff format --check and mypy are clean on both changed files. Three
pre-push hooks could not run locally — generate-openapi-spec, check-default-configuration
and provider mypy all need a Breeze CI image, and the local Docker daemon fails to build
one; they touch no file in this PR and will run in CI.

Behaviour change

A task whose process is sent SIGTERM while execute() is running now ends failed
instead 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.

AirflowTaskTerminated is classified as terminal today, so such a task fails without
consuming 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?
  • Yes

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.

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
@potiuk

potiuk commented Sep 25, 2026

Copy link
Copy Markdown
Member

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 gh pr reopen <PR_NUMBER> --repo apache/airflow. Reopen the ones you are ready to follow through - keep them rebased, respond to review comments and fix failing checks.

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

@potiuk potiuk closed this Sep 25, 2026
@namanjain24-sudo

Copy link
Copy Markdown
Contributor Author

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 gh pr reopen <PR_NUMBER> --repo apache/airflow and the "Reopen pull request" button in the UI fail with a generic Could not open the pull request error on all four, even though I'm well under the 5-PR cap (only #73104 is currently open). Could someone with write access reopen these for me, or point me at what I'm missing? Happy to provide any more detail that's useful.


Drafted-by: Claude Code (Sonnet 5) (no human review before posting)

@namanjain24-sudo

Copy link
Copy Markdown
Contributor Author

Superseded by #73809 — this PR kept failing to reopen (both gh pr reopen and the UI button errored with a generic "Could not open the pull request"), so per @potiuk's guidance on Slack I pushed/opened fresh from the same branch instead.

namanjain24-sudo added a commit to namanjain24-sudo/airflow that referenced this pull request Sep 28, 2026
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.
namanjain24-sudo added a commit to namanjain24-sudo/airflow that referenced this pull request Sep 28, 2026
namanjain24-sudo added a commit to namanjain24-sudo/airflow that referenced this pull request Sep 28, 2026
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:task-sdk closed because of open PR limit Closed as a one-time step of introducing the open pull request limit

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants