Repository navigation
Keep task attempt UUIDs and try numbers aligned - #73554
Merged
Merged
Conversation
1 task done
Member
Author
|
This inconsistency was (re)discovered here #65932 (comment) |
1 task
ashb
force-pushed
the
ti-attempt-number
branch
from
September 22, 2026 15:34
1837fe3 to
6540da5
Compare
kaxil
reviewed
Sep 22, 2026
kaxil
reviewed
Sep 22, 2026
ashb
force-pushed
the
ti-attempt-number
branch
2 times, most recently
from
September 25, 2026 09:50
0f4050b to
a500381
Compare
ashb
marked this pull request as ready for review
September 25, 2026 09:52
ashb
requested review from
XD-DENG,
amoghrajesh,
choo121600,
ephraimbuddy,
guan404ming,
pierrejeambrun,
ryanahamilton,
shubhamraj-git and
vatsrahul1001
as code owners
September 25, 2026 09:52
Member
Author
|
Pulled out of review for final test run. |
kaxil
reviewed
Sep 25, 2026
kaxil
approved these changes
Sep 25, 2026
kaxil
left a comment
Member
There was a problem hiding this comment.
LGTM, a few minor comments inline. The migration one is the only one I am not 100% sure on.
An attempt has a UUID A and try number N: (A,N). Previously, we assigned a new UUID B when retrying or clearing, but left the scheduler to increment the number. That left (B,N) in the database: the replacement's UUID paired with the previous attempt's number. We want to always allocate both fields together, archiving (A,N) before creating (B,N+1). The scheduler then uses that allocation: | Retry lifecycle | Previously | Now | |-----------------|------------|---------| | Running | (A,N) | (A,N) | | UP_FOR_RETRY | (B,N) | (B,N+1) | | Scheduled | (B,N+1) | (B,N+1) | When clearing a running task, we must also wait for its process to stop before allowing the replacement to run. We reject heartbeats with 409 when the TI is RESTARTING, which tells the worker to stop. But we cannot tell from that response when the worker has actually stopped. Its child may still be handling SIGTERM, running cleanup, or changing external state. Starting the replacement then could let both attempts act on the same external system. We therefore leave both fields unchanged throughout shutdown: | Running clear | Identity | State | |----------------------|----------|------------| | Before clear | (A,N) | RUNNING | | Clear requested | (A,N) | RESTARTING | | Cleared again | (A,N) | RESTARTING | | Termination reported | (B,N+1) | NULL | By keeping the UUID unchanged as A, we let the worker report against the attempt it owns. We also reject start requests during RESTARTING so another workload cannot start that attempt while shutdown continues. The supervisor knows the child has stopped once it has reaped it. We need the supervisor to report that fact to the API before we can release the replacement without relying on an executor event. It sends SERVER_TERMINATED through the existing finish request to acknowledge the shutdown. We do not store SERVER_TERMINATED as a TI state: the worker is confirming that it stopped as instructed, rather than reporting a task failure. Once the supervisor confirms termination, we make the replacement eligible immediately and restore its retry budget. A clear must allow an immediate rerun even when the task has exhausted its retries, without introducing an ordinary retry delay or failure callback. We still need to support workers back to Task SDK 1.0.0, which cannot send this acknowledgement. The scheduler therefore accepts terminal executor events as a fallback while the matching attempt is RESTARTING. Executor SUCCESS only means the workload finished, so either SUCCESS or FAILED can complete the clear. The API and scheduler both lock and recheck the row before allocating a replacement. If the API completes the clear first, the old executor event no longer matches the live attempt. We do not need to track the worker version. Moving allocation earlier must not change retry timing or which logs users see. During UP_FOR_RETRY, we still calculate backoff and select logs using the failed attempt N, although the live row now identifies N+1. We preserve both fields across deferral and rescheduling because they continue the same attempt. Try zero still means we have not allocated an attempt. We also need to bring existing pending rows into the new convention. Migration 0134 advances UP_FOR_RETRY rows and NULL-state rows with positive try numbers: the old code had already replaced their UUIDs but left scheduling to advance their numbers. Rows at try zero with NULL state remain unallocated. We use Cadwyn to preserve older API contracts. For current clients, the API returns 410 for an archived attempt and 404 for an unknown UUID; older clients retain 404 for both. If a worker loses a response, it can use 410 to establish that its attempt has ended, but cannot infer that the server accepted the particular outcome it reported. We retain the existing heartbeat-timeout and orphan recovery behavior. Those recovery paths cannot prove that the old worker has stopped.
Retries now allocate the next try before it starts, so selecting the last executed try only in Logs makes tab changes misleading. Keep the current try as the default and preserve explicit selections across tabs. Explain why a pending try has no logs, don't try to fetch any logs from the API server, and offer the previous try instead.
…x tests Some tests were manually creating TIs in "incorrect" state, this fixes them.
New clients will usually ack the 409 to by reporting `server_terminated` state, but old clients (and crashed tasks) won't, so we need to make sure that executor events still correctly progress a RESTARTING TI into a new try in UP_FOR_RETRY/None.
ashb
force-pushed
the
ti-attempt-number
branch
from
September 26, 2026 15:08
f486099 to
8f692d6
Compare
ashb
requested review from
bugraoz93,
henry3260,
jason810496 and
rawwar
as code owners
September 26, 2026 15:27
1 task done
namanjain24-sudo
added a commit
to namanjain24-sudo/airflow
that referenced
this pull request
Sep 28, 2026
…ents process_executor_events calls ti.set_state() without a session when the Dag of a finished task instance cannot be loaded. set_state is @provide_session and settings.Session is scoped, so create_session() returned the scheduler's own session and committed and closed it on exit, in the middle of the batch. A second call site with the same bug (a cleared task instance reported terminated) was superseded by apache#73554, which replaced that whole code path with TaskInstance.complete_restart(), already passing the scheduler's session correctly.
1 task done
1 task done
1 of 2 tasks
namanjain24-sudo
added a commit
to namanjain24-sudo/airflow
that referenced
this pull request
Oct 5, 2026
…ents process_executor_events calls ti.set_state() without a session when the Dag of a finished task instance cannot be loaded. set_state is @provide_session and settings.Session is scoped, so create_session() returned the scheduler's own session and committed and closed it on exit, in the middle of the batch. A second call site with the same bug (a cleared task instance reported terminated) was superseded by apache#73554, which replaced that whole code path with TaskInstance.complete_restart(), already passing the scheduler's session correctly.
1 task done
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
An attempt has a UUID A and try number N: (A,N). Previously, we assigned
a new UUID B when retrying or clearing, but left the scheduler to
increment the number. That left (B,N) in the database: the replacement's
UUID paired with the previous attempt's number.
We want to always allocate both fields together, archiving (A,N)
before creating (B,N+1). The scheduler then uses that allocation:
This has UI impacts, as previously try N+1 was only created just as it starts.
In both of these screenshots below, try 1 has failed and the task is eligible for retry.
UI Before: (There is no try selector visible, as there is only a single try)
UI after (try 2 is now visible, in state of "up for retry", logs explicitly never fetched, but a "see here" message shown by the UI instead)
When clearing a running task, we must also wait for its process to stop
before allowing the replacement to run. We reject heartbeats with 409
when the TI is RESTARTING, which tells the worker to stop. But we cannot
tell from that response when the worker has actually stopped. Its child
may still be handling SIGTERM, running cleanup, or changing external
state. Starting the replacement then could let both attempts act on
the same external system.
We therefore leave both fields unchanged throughout shutdown:
By keeping the UUID unchanged as A, we let the worker report against
the attempt it owns. We also reject start requests during RESTARTING
so another workload cannot start that attempt while shutdown continues.
The supervisor knows the child has stopped once it has reaped it.
We need the supervisor to report that fact to the API before we can
release the replacement without relying on an executor event. It sends
SERVER_TERMINATED through the existing finish request to acknowledge
the shutdown. We do not store SERVER_TERMINATED as a TI state: the
worker is confirming that it stopped as instructed, rather than
reporting a task failure.
Once the supervisor confirms termination, we make the replacement
eligible immediately and restore its retry budget. A clear must allow
an immediate rerun even when the task has exhausted its retries,
without introducing an ordinary retry delay or failure callback.
We still need to support workers back to Task SDK 1.0.0, which cannot
send this acknowledgement. The scheduler therefore accepts terminal
executor events as a fallback while the matching attempt is RESTARTING.
Executor SUCCESS only means the workload finished, so either SUCCESS
or FAILED can complete the clear. The API and scheduler both lock and
recheck the row before allocating a replacement. If the API completes
the clear first, the old executor event no longer matches the live
attempt. We do not need to track the worker version.
Moving allocation earlier must not change retry timing or which logs
users see. During UP_FOR_RETRY, we still calculate backoff and select
logs using the failed attempt N, although the live row now identifies
N+1. We preserve both fields across deferral and rescheduling because
they continue the same attempt. Try zero still means we have not
allocated an attempt.
We also need to bring existing pending rows into the new convention.
Migration 0134 advances UP_FOR_RETRY rows and NULL-state rows with
positive try numbers: the old code had already replaced their UUIDs
but left scheduling to advance their numbers. Rows at try zero with
NULL state remain unallocated.
We use Cadwyn to preserve older API contracts. For current clients,
the API returns 410 for an archived attempt and 404 for an unknown UUID;
older clients retain 404 for both. If a worker loses a response, it can
use 410 to establish that its attempt has ended, but cannot infer that
the server accepted the particular outcome it reported.
We retain the existing heartbeat-timeout and orphan recovery behavior.
Those recovery paths cannot prove that the old worker has stopped.
Was generative AI tooling used to co-author this PR?
{pr_number}.significant.rst, in airflow-core/newsfragments. You can add this file in a follow-up commit after the PR is created so you know the PR number.