kaxil commented on code in PR #73030:
URL: https://github.com/apache/airflow/pull/73030#discussion_r3997960819
##########
airflow-core/src/airflow/ui/src/pages/TaskInstance/Details.tsx:
##########
@@ -131,6 +131,16 @@ export const Details = () => {
return (
<Box p={2}>
+ {taskInstance?.retry_reason === null || taskInstance?.retry_reason ===
undefined ? undefined : (
Review Comment:
Nothing clears `retry_reason` when a task is cleared, so this banner
outlives the state it describes. I ran `clear_task_instances` against a failed
ti with the column set and got back `state=None, retry_reason='PROBE-REASON
auth error, do not retry'`:
[clear_task_instances](https://github.com/apache/airflow/blob/e6eb0db0ba07fe8a332366587fcd74eb7829c42c/airflow-core/src/airflow/models/taskinstance.py#L444)
resets `state`, `external_executor_id` and the next-method args but leaves the
retry-policy columns alone, and the only reset is [in
`ti_run`](https://github.com/apache/airflow/blob/e6eb0db0ba07fe8a332366587fcd74eb7829c42c/airflow-core/src/airflow/api_fastapi/execution_api/routes/task_instances.py#L247-L256)
when the task next enters RUNNING. For a paused dag, or a task queued behind a
full pool, that window is indefinite and the page keeps showing an orange
"Reason for state" quoting the previous attempt. Gating the banner on
`failed`/`up_for_retry`, or clearing the column in `c
lear_task_instances`, would close it.
##########
airflow-core/src/airflow/api_fastapi/core_api/datamodels/task_instances.py:
##########
@@ -89,6 +89,7 @@ class TaskInstanceResponse(BaseModel):
queued_by_job: JobResponse | None = Field(alias="triggerer_job")
dag_version: DagVersionResponse | None
team_name: str | None = None
+ retry_reason: str | None = None
Review Comment:
This is the first time `retry_reason` becomes public v2 API surface, so the
name is fixed from here on. I checked main: the field exists today only as the
ORM column and inside the versioned execution API, and is absent from both
`v2-rest-api-generated.yaml` and these datamodels, so this is the point of no
return. #73027 writes it on the terminal-failure path as well, and this PR's
own label is "Reason for state" rather than anything retry-shaped, so the name
is already out of step with what the field carries. Worth naming the response
field for that (`state_reason`?) while it is still free, even if the ORM column
keeps `retry_reason`.
##########
airflow-core/src/airflow/ui/src/pages/TaskInstance/Details.tsx:
##########
@@ -162,6 +172,12 @@ export const Details = () => {
</Flex>
</Table.Cell>
</Table.Row>
+ {tryInstance?.retry_reason === null || tryInstance?.retry_reason ===
undefined ? undefined : (
Review Comment:
This row reads the selected try (`tryInstance`) while the banner at the top
reads the latest try (`taskInstance`), and both are labelled
`taskInstance.retryReason`. On the default view that prints the same string
twice, since `/tries/{n}` returns the live ti for the current try number. Pick
try 1 of 3 in the Tries strip and the table shows try 1's reason while the
banner directly above the selector still shows try 3's, with nothing on screen
distinguishing them. Should the banner follow `tryInstance` too?
##########
airflow-core/src/airflow/ui/src/pages/TaskInstance/Details.tsx:
##########
@@ -131,6 +131,16 @@ export const Details = () => {
return (
<Box p={2}>
+ {taskInstance?.retry_reason === null || taskInstance?.retry_reason ===
undefined ? undefined : (
+ <Alert
+ data-testid="retry-reason-alert"
Review Comment:
No frontend test covers the banner or the row. `TaskInstance.test.tsx` and
`Header.test.tsx` are right next door, and the branches worth pinning are cheap
ones: banner absent when the reason is null, and `error` vs `warning` picked by
state. This `data-testid` is already the hook for it.
##########
task-sdk/tests/task_sdk/execution_time/test_task_runner.py:
##########
@@ -1196,6 +1196,65 @@ def execute(self, context):
assert counted.count("operator_failures") == 1
+def test_retry_policy_fail_persists_reason(create_runtime_ti,
mock_supervisor_comms):
+ class _AlwaysFails(BaseOperator):
+ def execute(self, context):
+ raise RuntimeError("boom")
+
+ task = _AlwaysFails(
+ task_id="fail_with_reason",
+ retry_policy=ExceptionRetryPolicy(
+ rules=[RetryRule(exception=RuntimeError, action=RetryAction.FAIL,
reason="do not retry")]
+ ),
+ )
+ ti = create_runtime_ti(task=task, should_retry=True)
+
+ state, msg, error = run(ti, ti.get_template_context(), mock.MagicMock())
+
+ assert state == TaskInstanceState.FAILED
+ assert isinstance(msg, TaskState)
+ assert msg.retry_reason == "do not retry"
+
+
+def
test_retry_policy_retry_exhausted_persists_combined_reason(create_runtime_ti,
mock_supervisor_comms):
+ """A policy-chosen RETRY that hits an exhausted budget still fails, with
both reasons recorded."""
+
+ class _AlwaysFails(BaseOperator):
+ def execute(self, context):
+ raise RuntimeError("boom")
+
+ task = _AlwaysFails(
+ task_id="retry_exhausted",
+ retry_policy=ExceptionRetryPolicy(
+ rules=[RetryRule(exception=RuntimeError, action=RetryAction.RETRY,
reason="rate limit")]
+ ),
+ )
+ ti = create_runtime_ti(task=task, try_number=2, max_tries=2,
should_retry=False)
+
+ state, msg, error = run(ti, ti.get_template_context(), mock.MagicMock())
+
+ assert state == TaskInstanceState.FAILED
+ assert isinstance(msg, TaskState)
+ assert msg.retry_reason == "rate limit; retries exhausted (2 of 2)"
Review Comment:
This assertion only reads correctly because the fixture forces a combination
the server never sends. `create_runtime_ti(try_number=2, max_tries=2,
should_retry=False)` is unreachable in production:
`_is_eligible_to_retry("running", 2, 2)` returns True, so the real server would
have sent `should_retry=True` here. Actual exhaustion is `try_number ==
max_tries + 1`, which is what exposes the off-by-one in the message above.
`try_number=3, max_tries=2` would match what the server produces, and two
distinct values would also pin the argument order, which `2 of 2` cannot.
##########
task-sdk/src/airflow/sdk/execution_time/task_runner.py:
##########
@@ -1931,9 +1932,15 @@ def _finalize_task_failure(
if retry_reason is not None:
retry_kwargs["retry_reason"] = retry_reason[:500]
return RetryTask(**retry_kwargs), TaskInstanceState.UP_FOR_RETRY
+ if retry_reason is not None and ti._ti_context_from_server is not None:
+ max_tries = ti._ti_context_from_server.max_tries
+ retry_reason = f"{retry_reason}; retries exhausted ({ti.try_number} of
{max_tries})"
Review Comment:
This renders as "retries exhausted (3 of 2)" in production. The branch is
only reached when the server sent `should_retry=False`, and the server computes
that as [`max_tries != 0 and try_number <=
max_tries`](https://github.com/apache/airflow/blob/13f23e5fa11077db051ceb5c45c0e96d05f42185/airflow-core/src/airflow/api_fastapi/execution_api/routes/task_instances.py#L1317-L1326),
so with retries configured you only arrive here once `try_number == max_tries
+ 1`. The denominator wants to be `max_tries + 1`, which is the convention this
same file already uses at [line
2137](https://github.com/apache/airflow/blob/13f23e5fa11077db051ceb5c45c0e96d05f42185/task-sdk/src/airflow/sdk/execution_time/task_runner.py#L2137)
(`Try {{try_number}} out of {{max_tries + 1}}`) and that
[`models/taskinstance.py:1465`](https://github.com/apache/airflow/blob/e6eb0db0ba07fe8a332366587fcd74eb7829c42c/airflow-core/src/airflow/models/taskinstance.py#L1465)
uses for `Starting attempt %s of %s`. There is also a
`max_tries == 0` case: a policy returning RETRY on a task with no retries
configured renders "retries exhausted (1 of 0)", where there was no retry
budget to exhaust.
##########
airflow-core/tests/unit/api_fastapi/execution_api/versions/head/test_task_instances.py:
##########
@@ -2446,6 +2446,72 @@ def test_ti_update_state_to_failed_table_check(self,
client, session, create_tas
assert ti.next_kwargs is None
assert ti.duration == 3600.00
+ def test_ti_update_state_to_failed_persists_retry_reason(self, client,
session, create_task_instance):
Review Comment:
These three tests only exercise the head version, so nothing pins the
version boundary this change introduces. `AddTerminalStateRetryReasonField` was
added to `Version("2026-10-30")`, and `TITerminalStatePayload` is a
`StrictBaseModel` with `extra="forbid"`, so a terminal-state PATCH carrying
`retry_reason` at `Airflow-API-Version: 2026-06-30` should be rejected. The
other two changes in that same version each have a boundary test
(`v2026_10_30/test_task_instances.py::TestArgBindingsFieldBackwardCompat` and
`v2026_10_30/test_callbacks.py::TestRunCallbackEndpointVersioning`); a short
case alongside them would stop this version change from silently going missing.
##########
airflow-core/src/airflow/api_fastapi/execution_api/routes/task_instances.py:
##########
@@ -670,7 +670,11 @@ def _create_ti_state_update_query_and_update_state(
query = query.values(state=updated_state, next_method=None,
next_kwargs=None)
if updated_state == TaskInstanceState.FAILED:
- # This is the only case needs extra handling for
TITerminalStatePayload
+ if isinstance(ti_patch_payload, TITerminalStatePayload) and
ti_patch_payload.retry_reason:
+ failed_retry_reason: str | None =
ti_patch_payload.retry_reason[:500]
Review Comment:
Truncating from the tail drops the part this change adds.
`_finalize_task_failure` composes the terminal reason as `f"{retry_reason};
retries exhausted (...)"` and, unlike the retry branch just below which caps
with `retry_reason[:500]` before sending, never caps the composed string. So
for a policy reason over roughly 470 characters the suffix is the first thing
lost and what lands in the column is a mid-sentence cut of the reason.
`LLMRetryPolicy` builds the reason from an unconstrained `reasoning` field, so
long values are reachable rather than theoretical. Capping the reason before
appending, or eliding the middle instead of the tail, would keep the more
useful half.
--
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]
