You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "namanjain24-sudo (via GitHub)" <gi...@apache.org> on 2026/09/20 06:30:43 UTC
[PR] Fail a task whose execute is interrupted by SIGTERM [airflow]
namanjain24-sudo opened a new pull request, #73395:
URL: https://github.com/apache/airflow/pull/73395
`closes: #73006`
## What happened
The task runner installs a `SIGTERM` handler that calls `on_kill()` and returns:
```python
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?
- [X] Yes
Generative AI tooling was used while authoring this PR, following [the guidelines](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#gen-ai-assisted-contributions). Every claim above — the reproduction, the ablation and the before/after numbers — was produced by running the tests, not by the model asserting them.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fail a task whose execute is interrupted by SIGTERM [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #73395:
URL: https://github.com/apache/airflow/pull/73395#issuecomment-5839401082
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:
* #73129 - Lower cross-provider dependencies in provider lowest-deps tests
* #73172 - Skip executor validation when a worker parses a Dag
* #73213 - Pass the scheduler's session to set_state when processing executor events
* #73356 - Allow DataflowJobStatusSensor to wait on a job name
* #73375 - Reset a mapped task's state store when its expansion is recomputed
* #73395 - Fail a task whose execute is interrupted by SIGTERM
These pull requests stay open because maintainers are already engaged in them - they count towards your limit:
* #73104 - Stop rebuilding the FAB app on every users and roles API call
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
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fail a task whose execute is interrupted by SIGTERM [airflow]
Posted by "kartik00052 (via GitHub)" <gi...@apache.org>.
kartik00052 commented on PR #73395:
URL: https://github.com/apache/airflow/pull/73395#issuecomment-5772913600
Reading through the retry/failure paths after seeing this issue and #73395 — a couple of things worth pointing at directly with code references, plus a possible way to close the remaining gap.
**A bounded, separately-counted infra retry already exists for one case.**
`KubernetesExecutor._is_pre_execution_failure()` (in `kubernetes_executor.py`) already distinguishes a pod that failed *before* the task instance reached `RUNNING` from one that failed after — it checks `ti_state == QUEUED` at failure time, and if that holds, the pod is requeued on its own counter (`pod_launch_failure_max_retries`) without ever touching `task.retries`. That's exactly the "infra failure shouldn't burn the application's retry budget" behavior this issue is asking for — it just only covers pod-launch failures.
**The mid-execution case — the one in this issue's screenshot — doesn't get that treatment.**
Once a task instance is `RUNNING` and gets killed externally (pod eviction, node drain, SIGTERM), that's the case #73395 is fixing: right now on `main`, `_on_term` in the task runner doesn't raise anything, so `execute()` silently resumes and the task can end up reported `SUCCESS` even though the work was interrupted. #73395 fixes that by raising `AirflowTaskTerminated`.
But looking at how `task_runner.py` handles that exception (the except-chain around `except AirflowTaskTerminated`), it goes straight to `state = TaskInstanceState.FAILED` — it never calls `_handle_current_task_failed`, which is the path that checks `TaskInstance.is_eligible_to_retry()`. So after #73395 merges, a mid-execution infra kill won't silently succeed anymore, but it *will* permanently fail with zero retries even if the task has `retries=5` set. #73395's own PR description flags this directly: *"Whether an infrastructure kill should instead be retry-eligible is a real question — it is what #73238 is about... changing that classification also changes the UI 'mark failed' path, so it is left alone here."*
So the gap this issue is really about, narrowed down: **`AirflowTaskTerminated` needs the same kind of separate, bounded retry budget that pod-launch failures already get — not the application's `task.retries`, and not zero.**
**A possible shape for that**, modeled on the existing pattern rather than inventing something new:
- A new config, e.g. `infrastructure_failure_max_retries`, parallel to `pod_launch_failure_max_retries`.
- The real complication: `pod_launch_failure_max_retries` is tracked in `KubernetesExecutor.pod_launch_attempts`, an in-memory counter on the scheduler process. `AirflowTaskTerminated` is raised inside the task runner process itself — it doesn't have access to that. So this can't just reuse the same storage; the counter would need to live somewhere both sides can see it (task instance / task instance history row seems like the natural place, but I haven't traced whether there's already a suitable field, or whether this needs a new column/attempt-tracking concept).
- Rather than making `AirflowTaskTerminated` unconditionally terminal, it would need to go through something like `_handle_current_task_failed`'s retry-eligibility check, but against the new infra-budget instead of (or in addition to) `task.retries`.
- I'd explicitly leave the UI "mark failed" path alone, same as #73395 did — that's a separate behavior change and probably deserves its own discussion if anyone wants to touch it.
Given this touches task-sdk (where `AirflowTaskTerminated` is raised) and core retry semantics (not just the K8s executor), I'd guess this needs at least a short AIP rather than going straight to a PR, but I'm not certain where the line is for a change this size — happy to be corrected.
Open questions I don't have good answers to yet:
- Where should the infra-retry counter actually live given the cross-process constraint above?
- Should this budget be K8s-executor-specific (like today's pod-launch one) or executor-agnostic, since `AirflowTaskTerminated` itself is raised in executor-agnostic code?
- Does this want to be one unified "infrastructure failure retry budget" covering both pre-execution and mid-execution cases, or should they stay as separate mechanisms since they're detected completely differently?
Happy to take a pass at drafting this out further (or an AIP if that's the right next step) if this direction seems reasonable — wanted to sanity-check the approach here first rather than show up with a PR that reopens a decision #73395 intentionally deferred.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fail a task whose execute is interrupted by SIGTERM [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk closed pull request #73395: Fail a task whose execute is interrupted by SIGTERM
URL: https://github.com/apache/airflow/pull/73395
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fail a task whose execute is interrupted by SIGTERM [airflow]
Posted by "namanjain24-sudo (via GitHub)" <gi...@apache.org>.
namanjain24-sudo commented on PR #73395:
URL: https://github.com/apache/airflow/pull/73395#issuecomment-5839696999
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:
- #73395 - Fail a task whose execute is interrupted by SIGTERM
- #73213 - Pass the scheduler's session to set_state when processing executor events
- #73172 - Skip executor validation when a worker parses a Dag
- #73375 - Reset a mapped task's state store when its expansion is recomputed
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)
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fail a task whose execute is interrupted by SIGTERM [airflow]
Posted by "namanjain24-sudo (via GitHub)" <gi...@apache.org>.
namanjain24-sudo commented on PR #73395:
URL: https://github.com/apache/airflow/pull/73395#issuecomment-5858245128
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.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org