Persist retry_reason not just for retries but even when a task fails - #73027
amoghrajesh wants to merge 8 commits into
Conversation
retry_reason not just for retries but even when a task fails
|
@kaxil can I get a round of review on this? |
|
Round 3, verified at 524f00d against
The same file is the one conflict
Two housekeeping notes: the PR description still describes the |
|
@kaxil thanks. Docs: That paragraph described #73030's work, not this PR's so I removed it, it'll go there instead. Merge conflict resolved by taking main's text and keeping only what this PR changes: the reason is recorded on a FAIL too, and stays on the row since a FAIL is terminal. Also fixed the two other spots you found: step 4 of "How it works" and tasks.rst:300. The log line. Agreed, two decisions disagreeing is worse than none. Now a separate event with the numbers an reader needs: Ironic, given we just took counts out of the stored reason, but this is where they belong. Test asserts Retry policy decision appears once; checked it fails if the old line returns. The import: Now from I'd like to land this first, then rebase #73030 on top and handle comments there. The PR desc has been edited too. |
kaxil
left a comment
There was a problem hiding this comment.
Round 4, verified at 21033cb. The round 3 items landed: the state_reason paragraph is gone, step 4 of "How it works" and tasks.rst:299-300 now match the code, the exhausted-budget path logs its own Retry policy requested a retry but no attempts remain event with try_number and max_tries, and the migrator test imports from airflow.sdk. Task SDK and core tests pass on this push; the three red checks are not from this change (constraints and compat 2.11.1 hit a PyPI 503 during the distribution build, and the docs build failed in the ibm-mq provider's docs). Nothing here blocks. Two stale sentences are worth fixing before merge, plus two notes for the rebase and #73030:
providers/common/ai/docs/retry_policies.rst:423-426 says "The retry_reason is only recorded on a RETRY ... On a FAIL it is not written anywhere" again. You removed it in d9d3a00, and the latest merge from main brought it back, since main's ClassifierRetryPolicy rewrite of the page carries the same paragraph. It now contradicts the paragraph this PR adds at :408-413 on the same page. Dropping it, or folding the "truncated to 500 characters" detail into :408, fixes it.
The same claim is in the ChainRetryPolicy docstring at task-sdk/src/airflow/sdk/definitions/retry_policy.py:381-382: "The worker stores it as retry_reason on a RETRY and logs it otherwise." That is the rendered API reference, and after this PR the worker stores it on a FAIL too. The comment at :431 ("it is not stored, since nothing is retried by it") now gives the wrong reason for the same thing; "since no policy took a position" would be accurate.
For the rebase: git merge-tree against current main reports a conflict in supervisor.py, because #73290 moved TaskState handling into _handle_task_state. The self._retry_reason = msg.retry_reason capture needs to move into that handler, and test_task_state_retry_reason_forwarded_to_finish will fail if it gets dropped in the resolution, so that part is covered.
For #73030: a FAIL reason now outlives the FAIL. Only ti_run clears retry_reason, so mark-success leaves a SUCCESS row carrying it, and a cleared TI keeps it until the next run starts. Nothing reads the column yet, so this belongs with the UI work there (render it only for FAILED or UP_FOR_RETRY, or clear it in clear_task_instances).
Was generative AI tooling used to co-author this PR?
Motivation
Today, when a task goes into "FAILED" it alone tells you nothing about why it stopped and that's something a retry policy in the first place should surface best. A few examples:
Auth error, policy said don't bother retrying.
Your task hits an API and gets a 403 because the key expired. The policy is smart enough to know retrying won't help (it'll just fail 3 more times the same way), so it fails immediately after try 1. Without a reason, you just see "FAILED after 1 try" and think something's broken with retries. With a reason, you see: "auth error, policy chose not to retry" now you know exactly what to go fix (the API key), not the code.
Rate limit, retries all used up.
Your task keeps hitting a rate limit. The policy says "retry" each time, so it retries 3 times, but the budget (retries=3) runs out. Without a reason, you just see 3 red X's and no explanation. With a reason: "rate limit" and you immediately know it's not a bug; it's just that the external service was too slow to respond, and maybe you should bump retries or add a delay. The attempt counts are already on the same row as
try_numberandmax_tries, so whatever displays the reason can say "3 of 3" itself.Different failures on different tries.
Try 1 failed for one reason, try 2 for a totally different reason. Right now all you see is a row of identical red icons with no way to tell if its the same recurring problem or three unrelated ones. Reasons attached to each try let someone debugging a flaky task actually see the story, instead of guessing.
The underlying idea: today, all that classification work the policy does (LLM or exception-based) happens, gets logged once in task logs. Only the "it retried" case kept the reason. This change makes sure the reason survives for the FAILED case too, so a future screen can show a plain sentence like "Stopped at try 2: auth error, no retry" instead of just a bare failure with no story behind it.
What
If you've set a
retry_policyon a task, it can decide things like "this looks like an auth error, dont bother retrying" or "retries are exhausted, this was a rate limit," but today that explanation is only ever logged to a log line and thrown away otherwise. It never reaches the database, so no API response or UI screen(I am proposing we build it to provide a better UX to users and its more "in the face") can ever show it, no matter how much we build on top later.This PR is the first, necessary step toward fixing that: make sure the reason actually gets saved whenever a task fails, not just when it retries. Once its reliably in the database, a future PR can expose it through the API and the UI, so a Dag author looking at a failed task instance can see a plain reason instead of just "FAILED" with no explanation.
Current behaviour
retry_reasonis already written to the database, but only when a retry policy chooses to retry. The two failure outcomes people most want explained never save anything: a policy deciding FAIL outright, and a policy deciding RETRY but the retry budget being exhausted. Both currently build a bareTaskState(state=FAILED)with no reason, so the classification text is logged and discarded.TaskState(state=FAILED), reason dropped.RetryTask(retry_reason=...), already persisted (unchanged).Proposed change
Thread
retry_reasonthrough both FAILED paths, end to end:_handle_current_task_failed's FAIL branch and_finalize_task_failure's exhausted budget branch now attach a reason to theTaskStatethey build. Only the policy's own words are stored; attempt counts are left to whatever displays the reason, which hastry_numberandmax_triesalongside it.Retry policy requested a retry but no attempts remain, withtry_numberandmax_tries) rather than a secondRetry policy decisionline contradicting the first.TaskState(task-sdk message) gains aretry_reasonfield.retry_reasonoff theTaskStatemessage and forwards it to the deferredfinish()call.TITerminalStatePayload(execution API request schema) gainsretry_reason, gated behind a new Cadwyn version change added to the existing unreleased2026-10-30version (tentative date)task_instance.retry_reason.providers/common/ai/docs/retry_policies.rstandairflow-core/docs/core-concepts/tasks.rst.Only tasks with a configured retry policy are affected; a plain
retries=Ntask with no policy gets no reason and no behaviour change, since there's nothing meaningful to attach.Testing
After running the
example_llm_retry_policydag, earlier if I ran:I would get:
Now we see:
The two
failedrows are the change: before this PR both were blank.What's next
retry_reasonstill isn't exposed anywhere outside the database. Adding it toTaskInstanceResponse(API) and rendering it in the Task Instance UI panel are the next two steps, so this data can actually reach a Dag author or surface it on the UI.{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.