seanmuth commented on issue #60858: URL: https://github.com/apache/airflow/issues/60858#issuecomment-6153995895
@eladkal @potiuk following up on the pushback on #74458. I'd like to settle whether this is a bug before anyone writes another PR, because I think the current behavior is inconsistent with what Airflow already does, not just a missing feature. **Airflow already runs `on_failure_callback` for infrastructure failures.** The scheduler builds a `TaskCallbackRequest` for the dag processor in three cases, and none of them is a business failure: - the executor reports the task finished while the DB still says `running` (the [`killed_externally`](https://github.com/apache/airflow/blob/47d929c702be22d289b13316b6173e6a863a4c54/airflow-core/src/airflow/jobs/scheduler_job_runner.py#L1743) path) - the task stops heartbeating ([heartbeat-timeout purge](https://github.com/apache/airflow/blob/47d929c702be22d289b13316b6173e6a863a4c54/airflow-core/src/airflow/jobs/scheduler_job_runner.py#L4109)). The [tasks docs](https://github.com/apache/airflow/blob/47d929c702be22d289b13316b6173e6a863a4c54/airflow-core/docs/core-concepts/tasks.rst?plain=1#L402-L406) use "the Airflow worker ran out of memory and was OOMKilled" as the example for this one - requeue attempts run out ([stuck in queued](https://github.com/apache/airflow/blob/47d929c702be22d289b13316b6173e6a863a4c54/airflow-core/src/airflow/jobs/scheduler_job_runner.py#L3507)) The [callbacks docs](https://github.com/apache/airflow/blob/47d929c702be22d289b13316b6173e6a863a4c54/airflow-core/docs/administration-and-deployment/logging-monitoring/callbacks.rst?plain=1#L23-L24) also pitch callbacks as monitoring ("you may wish to alert when certain tasks have failed"), with the only documented exception being state changes from the CLI/UI. **So the same OOM fires the callback or not, depending on which process the kernel picks.** On the "OOM kills the whole pod" point: that's the kubelet default on cgroup v2 since 1.28, which sets `memory.oom.group` so the whole container goes down together. But it's only a default. Since 1.32 kubelet has `singleProcessOOMKill` to opt out, nodes still on cgroup v1 never had group kill, and end users or managed Airflow providers can and do change it. Outside containers entirely (bare metal, VMs, LocalExecutor on a box) there's no group kill at all. In all of those cases the kernel kills a single process, whichever PID has the highest `oom_score`, and that can be either one: - **Supervisor killed:** nothing records the final state, the TI stays `running`, and one of the scheduler paths above picks it up and the dag processor runs `on_failure_callback`. - **Task process killed, supervisor survives:** the supervisor [reports `FAILED` through `finish()`](https://github.com/apache/airflow/blob/47d929c702be22d289b13316b6173e6a863a4c54/task-sdk/src/airflow/sdk/execution_time/supervisor.py#L1798), and the [`FAILED` branch on the API server](https://github.com/apache/airflow/blob/47d929c702be22d289b13316b6173e6a863a4c54/airflow-core/src/airflow/api_fastapi/execution_api/routes/task_instances.py#L843) only handles fail-fast. The DB is now terminal, so the scheduler sees no mismatch. No callback, no `on_task_instance_failed` listener, no OpenLineage FAIL event. Dag authors can't control or even see which of those happened. That's the bug: whether the callback runs is decided by a race between OOM victims, not by any design intent. It's also a regression: #15537 fixed exactly this OOM case for Airflow 2 in 2021. There's a smaller related gap with retries left. `UP_FOR_RETRY` is in [`STATES_SENT_DIRECTLY`](https://github.com/apache/airflow/blob/47d929c702be22d289b13316b6173e6a863a4c54/task-sdk/src/airflow/sdk/execution_time/supervisor.py#L308), which assumes the task process already sent `RetryTask`. A killed process never did, so the supervisor reports nothing and the TI sits in `running` until the scheduler catches it. **On the "needs a whole new flow" concern:** the flow already exists. The dag processor already runs task callbacks for tasks whose process died, via `TaskCallbackRequest`, and nothing new has to parse the dag. The fix is to use that same path when the supervisor itself reports a signal kill: 1. The supervisor marks the terminal payload as a signal kill when the task's exit code is negative and it didn't initiate the kill (i.e. not an overtime/heartbeat/server-requested termination). 2. In the `FAILED` branch, the API server enqueues the same `TaskCallbackRequest` the scheduler builds (`FAILED` or `UP_FOR_RETRY` by retry eligibility) and fires the failure listener. That works on every executor, keeps the supervisor fast, and avoids the KubernetesExecutor heartbeat-timeout delay (and the pool slot held during it) that #74458 had. It's option 2 from that PR's description. The cost is a small execution API change. If you agree this is a bug, @manipatnam can pick the PR back up, since #74458 already covers most of the supervisor side. If the position is still that `on_failure_callback` shouldn't cover infra failures, I think the scheduler paths above should change to match, because right now the behavior depends on which process loses an OOM race. 🤖 Generated with [Claude Code](https://claude.com/claude-code) — Claude Opus 5.5 -- 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]
