You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "kalluripradeep (via GitHub)" <gi...@apache.org> on 2026/05/23 10:39:11 UTC
[PR] Fix KubernetesExecutor retry losing hostname [airflow]
kalluripradeep opened a new pull request, #67372:
URL: https://github.com/apache/airflow/pull/67372
(no comment)
--
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] Record TaskInstanceHistory when a task fails from a non-running state [airflow]
Posted by "kalluripradeep (via GitHub)" <gi...@apache.org>.
kalluripradeep commented on PR #67372:
URL: https://github.com/apache/airflow/pull/67372#issuecomment-5764210556
@potiuk Thanks — all three applied.
1. Dropped `DEFERRED` from the comment.
2. Reverted the `test_task_instances.py` hunk entirely, including the
hostname/start_date setup that only fed it. That file is now untouched by
this PR.
3. Reverted the `test_adopt_or_reset_resettable_tasks` additions the same way.
Also trimmed the PR description — it described the `record_ti` FAILED forcing
and the Execution API snapshot, both of which were already on main. The diff
is now the newsfragment, the five-line condition, and the regression test.
--
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] Fix KubernetesExecutor retry losing hostname [airflow]
Posted by "kalluripradeep (via GitHub)" <gi...@apache.org>.
kalluripradeep commented on PR #67372:
URL: https://github.com/apache/airflow/pull/67372#issuecomment-4773012973
> This still misses the scheduler executor-event path that triggers the KubernetesExecutor symptom. A failed executor event for a TI still in `SCHEDULED`/`QUEUED` enters the state-mismatch path, then calls `ti.handle_failure()`. But `handle_failure()` only records history when `ti.state == RUNNING`; otherwise it sets `UP_FOR_RETRY` without `prepare_db_for_next_try()`. Please add/fix the scheduler executor-event retry path and add a regression test where a non-running queued/scheduled TI receives a failed executor event and preserves `TaskInstanceHistory.hostname`/`start_date`.
Thanks for the detailed review — you're exactly right.
The root cause is in `fetch_handle_failure_context()` (line 1809 in `taskinstance.py`):
```python
# Before
if ti.state == TaskInstanceState.RUNNING:
ti.prepare_db_for_next_try(session)
```
When the executor sends a FAILED event for a QUEUED/SCHEDULED/DEFERRED TI, `handle_failure()` is called but `ti.state != RUNNING`, so `prepare_db_for_next_try()` — and therefore `TaskInstanceHistory.record_ti()` — was never invoked. History lost, hostname gone.
Fixed by changing the guard to exclude only `RESTARTING` (which already had `prepare_db_for_next_try` called during the clear operation):
```python
# After
if ti.state != TaskInstanceState.RESTARTING:
# Covers RUNNING (normal exception), QUEUED/SCHEDULED/DEFERRED (external kill)
ti.prepare_db_for_next_try(session)
```
Added a parametrized regression test `test_process_executor_events_queued_ti_retry_preserves_history` covering both `QUEUED` and `SCHEDULED` states — verifies that after a FAILED executor event: TI gets a new UUID (confirming `prepare_db_for_next_try` ran), a `TaskInstanceHistory` row exists, and `hostname`/`start_date` are preserved.
--
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] Fix KubernetesExecutor retry losing hostname [airflow]
Posted by "kalluripradeep (via GitHub)" <gi...@apache.org>.
kalluripradeep commented on PR #67372:
URL: https://github.com/apache/airflow/pull/67372#issuecomment-4772968963
Thanks for the detailed review — you're exactly right.
The root cause is in `fetch_handle_failure_context()` (line 1809 in `taskinstance.py`):
```python
# Before
if ti.state == TaskInstanceState.RUNNING:
ti.prepare_db_for_next_try(session)
```
When the executor sends a FAILED event for a QUEUED/SCHEDULED/DEFERRED TI, `handle_failure()` is called but `ti.state != RUNNING`, so `prepare_db_for_next_try()` — and therefore `TaskInstanceHistory.record_ti()` — was never invoked. History lost, hostname gone.
Fixed by changing the guard to exclude only `RESTARTING` (which already had `prepare_db_for_next_try` called during the clear operation):
```python
# After
if ti.state != TaskInstanceState.RESTARTING:
# Covers RUNNING (normal exception), QUEUED/SCHEDULED/DEFERRED (external kill)
ti.prepare_db_for_next_try(session)
```
Added a parametrized regression test `test_process_executor_events_queued_ti_retry_preserves_history` covering both `QUEUED` and `SCHEDULED` states — verifies that after a FAILED executor event: TI gets a new UUID (confirming `prepare_db_for_next_try` ran), a `TaskInstanceHistory` row exists, and `hostname`/`start_date` are preserved.
--
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] Record TaskInstanceHistory when a task fails from a non-running state [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #67372:
URL: https://github.com/apache/airflow/pull/67372#discussion_r4063831219
##########
airflow-core/tests/unit/api_fastapi/execution_api/versions/head/test_task_instances.py:
##########
@@ -1996,6 +1999,12 @@ def test_ti_update_state_handle_retry(self, client, session, create_task_instanc
).one()
assert tih.task_instance_id
assert tih.task_instance_id != ti.id
+ assert tih.state == State.FAILED
+ assert tih.hostname == "random-hostname"
+ assert tih.start_date == DEFAULT_START_DATE
+ assert tih.end_date == DEFAULT_END_DATE
+ assert tih.duration == 3600
+ assert tih.rendered_map_index == "retry_label_abc"
Review Comment:
These six assertions cover the Execution API retry path in `airflow-core/src/airflow/api_fastapi/execution_api/routes/task_instances.py:676-689`, which this PR doesn't touch — that snapshot logic landed in #69248 (`66b803d5ef`, 2026-07-06), and the `state == FAILED` forcing comes from `taskinstancehistory.py:207-213`. They pass on `main` without your change.
> Target exactly 100% coverage of what the PR changes — no more, no less. […] every test must fail without the PR's change. Do not add tests for pre-existing logic that was already present before the PR.
>
> — [AGENTS.md § Testing Standards](https://github.com/apache/airflow/blob/main/AGENTS.md#testing-standards)
Could you drop this hunk, along with the `hostname` / `start_date` setup above that only exists to feed it? Your new `test_process_executor_events_queued_ti_retry_preserves_history` is the test that actually pins this fix, and it's a good one — parametrized over both states, and it asserts the new UUID as well as the archived hostname.
##########
airflow-core/tests/unit/jobs/test_scheduler_job.py:
##########
@@ -5339,19 +5341,69 @@ def test_adopt_or_reset_resettable_tasks(self, dag_maker, adoptable_state, sessi
ti.refresh_from_db(session=session)
assert ti.id != old_ti_id
- assert (
- session.scalar(
- select(TaskInstanceHistory).where(
- TaskInstanceHistory.dag_id == ti.dag_id,
- TaskInstanceHistory.task_id == ti.task_id,
- TaskInstanceHistory.run_id == ti.run_id,
- TaskInstanceHistory.map_index == ti.map_index,
- TaskInstanceHistory.try_number == old_try_number,
- TaskInstanceHistory.task_instance_id == old_ti_id,
- )
+ tih = session.scalar(
+ select(TaskInstanceHistory).where(
+ TaskInstanceHistory.dag_id == ti.dag_id,
+ TaskInstanceHistory.task_id == ti.task_id,
+ TaskInstanceHistory.run_id == ti.run_id,
+ TaskInstanceHistory.map_index == ti.map_index,
+ TaskInstanceHistory.try_number == old_try_number,
+ TaskInstanceHistory.task_instance_id == old_ti_id,
)
- is not None
)
+ assert tih is not None
+ assert tih.hostname == "random-hostname"
Review Comment:
Same point as on the execution-API test. `test_adopt_or_reset_resettable_tasks` exercises `scheduler_job_runner.py:3543`, where `prepare_db_for_next_try()` was already being called before this PR — so this assertion, and the `ti.hostname` / `ti.start_date` setup added at line 5331, pass without your change and sit outside the PR's scope per [AGENTS.md § Testing Standards](https://github.com/apache/airflow/blob/main/AGENTS.md#testing-standards).
The rewrite from `assert session.scalar(...) is not None` into a named `tih` variable reads better than what it replaced — keep it or revert it with the assertion, whichever you prefer.
##########
airflow-core/src/airflow/models/taskinstance.py:
##########
@@ -1895,10 +1895,14 @@ def fetch_handle_failure_context(
if task and fail_fast:
_stop_remaining_tasks(task_instance=ti, session=session)
else:
- if ti.state == TaskInstanceState.RUNNING:
- # If the task instance is in the running state, it means it raised an exception and
- # about to retry so we record the task instance history. For other states, the task
- # instance was cleared and already recorded in the task instance history.
+ if ti.state != TaskInstanceState.RESTARTING:
+ # Record the current attempt and prepare the TI for its next try.
+ # Covers every path eligible for retry reaching handle_failure():
+ # - RUNNING: task raised an exception during execution (normal failure)
+ # - QUEUED/SCHEDULED/DEFERRED: executor killed the task externally before
Review Comment:
`DEFERRED` can't reach this branch. `handle_failure()` has exactly two callers and both gate the state before arriving here:
- `_process_executor_events` — `scheduler_job_runner.py:1566-1571` restricts `ti_queued` to `SCHEDULED`, `QUEUED`, `RUNNING`, `RESTARTING`
- the heartbeat purge — `scheduler_job_runner.py:3779` restricts to `RUNNING`, `RESTARTING`
The reachable set is `{SCHEDULED, QUEUED, RUNNING}`, so the comment promises a case the code will never see.
> Do not narrate code, repeat code, or invent a rationale in comments.
>
> — [AGENTS.md § Coding Standards](https://github.com/apache/airflow/blob/main/AGENTS.md#coding-standards)
Dropping `DEFERRED` from the list is enough.
--
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] Record TaskInstanceHistory when a task fails from a non-running state [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk merged PR #67372:
URL: https://github.com/apache/airflow/pull/67372
--
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] Fix KubernetesExecutor retry losing hostname [airflow]
Posted by "kalluripradeep (via GitHub)" <gi...@apache.org>.
kalluripradeep commented on PR #67372:
URL: https://github.com/apache/airflow/pull/67372#issuecomment-4772969056
@sagu2602 Yes — your scenario (attempt-4 fails while QUEUED, logs tab missing that attempt) is exactly what this PR fixes.
When a task fails from `QUEUED` state, the scheduler's executor-event path calls `handle_failure()`. Previously, `TaskInstanceHistory` was only recorded when `ti.state == RUNNING`, so a QUEUED failure silently dropped the history row → the attempts tab in the UI had no entry → logs showed nothing.
With this fix `prepare_db_for_next_try()` (which writes the `TaskInstanceHistory` row) is now called for `QUEUED`/`SCHEDULED`/`DEFERRED` states too, so attempt-4's hostname and dates will be recorded and the logs tab will show the attempt correctly.
--
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] Fix KubernetesExecutor retry losing hostname [airflow]
Posted by "Vamsi-klu (via GitHub)" <gi...@apache.org>.
Vamsi-klu commented on PR #67372:
URL: https://github.com/apache/airflow/pull/67372#issuecomment-4764226418
This still misses the scheduler executor-event path that triggers the KubernetesExecutor symptom. A failed executor event for a TI still in `SCHEDULED`/`QUEUED` enters the state-mismatch path, then calls `ti.handle_failure()`. But `handle_failure()` only records history when `ti.state == RUNNING`; otherwise it sets `UP_FOR_RETRY` without `prepare_db_for_next_try()`. Please add/fix the scheduler executor-event retry path and add a regression test where a non-running queued/scheduled TI receives a failed executor event and preserves `TaskInstanceHistory.hostname`/`start_date`.
--
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] Fix KubernetesExecutor retry losing hostname [airflow]
Posted by "sagu2602 (via GitHub)" <gi...@apache.org>.
sagu2602 commented on PR #67372:
URL: https://github.com/apache/airflow/pull/67372#issuecomment-4771235882
Apache Airflow version: 3.2.0
So would this also solve logs tab missing attempts on UI when the task fails from QUEUED > FAILED??
For example: attempt-4 failed during QUEUED state
<img width="466" height="119" alt="logs missing for task queued failed" src="https://github.com/user-attachments/assets/10681365-b4bb-49b2-a85d-c2ce41a778ef" />
--
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] Record TaskInstanceHistory when a task fails from a non-running state [airflow]
Posted by "kalluripradeep (via GitHub)" <gi...@apache.org>.
kalluripradeep commented on PR #67372:
URL: https://github.com/apache/airflow/pull/67372#issuecomment-5750876774
> Could you please think of another title for this PR? The issue might be surfaced in KubernetesExecutor, but the fix is not in the Kubernetes provider - which is rather confusing.
@shahar1 Good point — retitled to "Record TaskInstanceHistory when a task fails from a non-running state".
--
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