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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
