seanmuth commented on issue #65708:
URL: https://github.com/apache/airflow/issues/65708#issuecomment-5683506198
## Correction and new evidence: the mechanism I described earlier was wrong
My comment above (and the framing in PR #73142's current docstring)
described this as "an unobserved/defaulted exit code overriding an
already-confirmed terminal state." **That explanation was inference from source
reading, not something I'd actually observed, and it doesn't hold up.** I built
live instrumentation (wrapping the relevant `Client`/`ActivitySubprocess` call
sites in a plugin, no application behavior changed) and ran it under a
deliberately CPU-saturated burst workload (10k mapped tasks, worker concurrency
pushed past comfortable capacity) to catch a real occurrence. Retracting the
earlier explanation and posting what actually happens, with full tracebacks.
### What's actually happening
Two independent, separately-caused duplicate writes hit the same task
instance, 20 seconds apart. Full apiserver-side timeline for one occurrence
(times UTC, anonymized deployment):
```
14:16:41.554 PATCH .../run -> 200
14:16:44.129 PUT .../heartbeat -> 204
14:16:44.404 PUT .../rtif -> 201
14:16:55.466 "Task instance state updated, new_state=success,
rows_affected=1"
14:16:55.538 PATCH .../state -> 204 (the real, successful
.succeed() write)
14:16:58.254 "Cannot update Task Instance in invalid state,
previous_state=success"
14:16:58.256 PATCH .../state -> 409 (duplicate retry of the
SAME .succeed() call, 2.7s later)
...
14:17:18.386 "Cannot update Task Instance in invalid state,
previous_state=success"
14:17:18.406 PATCH .../state -> 409 (a SEPARATE, later
.finish() call)
```
**First duplicate (the 204→409 pair, 2.7s apart):** this is client-side
tenacity retrying `.succeed()`'s PATCH after a timeout/connection hiccup on the
*client* side, even though the server had already fully processed and committed
the first physical request. `Client.request`'s retry predicate only retries on
`httpx.RequestError` or a `5xx` — a `409` is neither, so once the retry lands
on an already-`success` row, tenacity gives up immediately (`reraise=True`) and
raises. This happens entirely inside `Client.request()`, upstream of any
application-level state tracking. **Nothing in this issue or in PR #73142 fixes
this** — it's a retry-into-a-non-idempotent-guard problem at the HTTP/tenacity
layer.
**Second duplicate (the later 409, and the one that actually crashes the
worker):** full traceback, identical across all 7 live occurrences I captured
in one run (only `ti_id` and timestamp differ):
```
ServerResponseError: Server returned error
File ".../astro_agent_client/execution/isolated/_astro/procs/worker.py",
line 351, in _child_supervise_task
File ".../airflow/sdk/execution_time/supervisor.py", line 1993, in
supervise
File ".../airflow/sdk/execution_time/supervisor.py", line 1004, in wait
File ".../airflow/sdk/execution_time/supervisor.py", line 1018, in
update_task_state_if_needed
File ".../airflow/sdk/api/client.py", line 226, in finish
File ".../httpx/_client.py", line 1218, in patch
File ".../tenacity/__init__.py", line 338, in wrapped_f
File ".../tenacity/__init__.py", line 477, in __call__
File ".../tenacity/__init__.py", line 378, in iter
File ".../tenacity/__init__.py", line 400, in <lambda>
File ".../concurrent/futures/_base.py", line 449, in result
File ".../concurrent/futures/_base.py", line 401, in __get_result
File ".../tenacity/__init__.py", line 480, in __call__
File ".../airflow/sdk/api/client.py", line 887, in request
File ".../httpx/_client.py", line 825, in request
File ".../httpx/_client.py", line 914, in send
File ".../httpx/_client.py", line 942, in _send_handling_auth
File ".../httpx/_client.py", line 999, in _send_handling_redirects
File ".../httpx/_client.py", line 982, in _send_handling_redirects
File ".../airflow/sdk/api/client.py", line 186, in
raise_on_4xx_5xx_with_note
File ".../airflow/sdk/api/client.py", line 176, in get_json_error
```
`update_task_state_if_needed()` computes `final_state` from `exit_code` in a
way that doesn't correctly defer to the already-known
`_terminal_state='success'` here, decides it needs to call `.finish()`, and
that call hits the exact same "already success" guard — except this exception
is genuinely uncaught in `wait()`/`supervise()`, and it's this one that kills
task supervision. This is exactly the scenario PR #73142 targets. I traced it
end to end against #73142's fix: with correct precedence, `final_state`
resolves to `SUCCESS`, lands in `STATES_SENT_DIRECTLY`, and `.finish()` is
never called — zero calls, not a caught failure.
I also want to flag PR #66574 ("Recover stuck TIs when direct terminal-state
API call fails," in 3.3.0+) as related but distinct: it defers when
`_terminal_state` gets set and gives `update_task_state_if_needed()` a safe
replay path instead of the buggy fallback. In my traced scenario it would
prevent the *crash* (the replay's failure is caught and logged rather than
propagated) but it does **not** prevent the duplicate send — the replay itself
re-issues `.succeed()` again, hitting the same 409 a third time. #73142 is the
one that actually eliminates that second call outright.
**Net: two independent bugs, three total duplicate/redundant `PATCH
.../state` calls for one task completion, on one confirmed live occurrence.**
#73142 fixes the second. Neither #73142 nor #66574 (not yet backported to any
3.1.x/3.2.x line I've checked) fixes the first — that would need a narrower fix
at the retry layer itself (read-before-retry against current server state, or
treating "requested state == current state" as idempotent success rather than a
conflict).
---
Drafted-by: Claude Code (Sonnet 5); reviewed by @seanmuth 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
