kaxil commented on code in PR #73030:
URL: https://github.com/apache/airflow/pull/73030#discussion_r4062322903
##########
task-sdk/src/airflow/sdk/execution_time/task_runner.py:
##########
@@ -1933,9 +1935,19 @@ 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
+ if max_tries > 0:
+ # max_tries is the retry count, not the attempt count -- total
attempts is max_tries + 1.
+ suffix = f"; retries exhausted ({ti.try_number} of {max_tries +
1})"
Review Comment:
This prints `(3 of 3)`, and the banner title added in the same push already
reads "Stopped on try 3 of 3", built from the same `try_number` and `max_tries
+ 1`
([Details.tsx:127](https://github.com/apache/airflow/blob/d6bc665a89959d66bc2ad3f3b990bd69140905b3/airflow-core/src/airflow/ui/src/pages/TaskInstance/Details.tsx#L127)).
The alert ends up stating the counts twice.
There is a second reason to drop the suffix rather than reword it. `retries`
defaults to 0, and `_is_eligible_to_retry` is `max_tries != 0 and try_number <=
max_tries`, so a task without an explicit `retries=` reaches this branch with
the suffix skipped. Where a policy returned RETRY, what gets stored is the bare
reason: "Transient rate limit, backing off for 60s" on a task that is not going
to retry. All four new tests pass `retries=2`, so the default case is untested.
Leaving the counts to the UI would cover both.
##########
airflow-core/src/airflow/ui/src/pages/TaskInstance/Details.tsx:
##########
@@ -162,6 +202,12 @@ export const Details = () => {
</Flex>
</Table.Cell>
</Table.Row>
+ {tryInstance?.state_reason === null || tryInstance?.state_reason ===
undefined ? undefined : (
Review Comment:
The banner is gated on `failed`/`up_for_retry` now, but this row isn't; it
renders on a non-null reason alone. After a clear the column survives, so the
same staleness comes back here: `clear_task_instances` resets `state`,
`external_executor_id`, the next-method args and `max_tries` and never touches
`retry_reason`
([taskinstance.py:444-447](https://github.com/apache/airflow/blob/d6bc665a89959d66bc2ad3f3b990bd69140905b3/airflow-core/src/airflow/models/taskinstance.py#L444-L447)),
and the page then shows `State: (no status)` with `Reason for state: auth
error, do not retry` directly underneath it. Applying the same state gate here
closes it without waiting on the column-clearing PR you described. The four
cases at `Details.test.tsx:129-137` set the reason on both objects but assert
only the banner's absence, so they walk straight past this row; adding
`expect(screen.queryByText(i18n.t("common:taskInstance.stateReason"))).not.toBeInTheDocument()`
to them fails against today's code.
##########
task-sdk/src/airflow/sdk/execution_time/schema/schema.json:
##########
@@ -4203,6 +4203,18 @@
],
"default": null,
"title": "Rendered Map Index"
+ },
+ "retry_reason": {
Review Comment:
The generated SDK mirrors of this schema didn't get regenerated, and static
checks are red on one of them: `check-ts-sdk-supervisor-schema` exits 1 with
"files were modified by this hook" ([run
35580411680](https://github.com/apache/airflow/actions/runs/35580411680/job/106276494112)).
That hook is deliberately skipped for schema-only changes so regeneration can
be the ts-sdk follow-up's job, but `skip_prek_hooks` returns early once
`full_tests_needed` is set
([selective_checks.py:1708-1711](https://github.com/apache/airflow/blob/d6bc665a89959d66bc2ad3f3b990bd69140905b3/dev/breeze/src/airflow_breeze/utils/selective_checks.py#L1708-L1711)),
and this PR sets it by touching `v2-rest-api-generated.yaml`, which matches
the API-codegen file group. The CI log confirms it: `skip-prek-hooks:
identity,update-uv-lock`. `cd ts-sdk && pnpm run generate:supervisor` should be
a two-line diff, and `retry_reason` is optional so nothing in
`ts-sdk/src/coordinator/` needs to change with it. The other
two mirrors are stale the same way with no hook firing on this PR:
`go-sdk/pkg/execution/genmodels/models.gen.go` and
`java-sdk/sdk/schema/schema.json` both still have `TaskState` as
state/end_date/type/rendered_map_index. Worth deciding whether they ride along
here or in the follow-up.
##########
providers/common/ai/docs/retry_policies.rst:
##########
@@ -140,10 +140,12 @@ four fields: ``category``, ``should_retry``,
``suggested_delay_seconds``, and
``reasoning``. Only ``should_retry`` and ``suggested_delay_seconds`` affect
the run.
-``category`` and ``reasoning`` are only recorded on a RETRY. They are written
+``category`` and ``reasoning`` are recorded on both outcomes. They are written
to the task instance's ``retry_reason`` (truncated to 500 characters, see
-below), then cleared once the next attempt starts running. On a FAIL they are
-not written anywhere -- they only show up in the task log.
+below). On a RETRY the value is cleared once the next attempt starts running.
+A FAIL is terminal, so there is no next attempt to clear it and the reason
+stays on the row. When the model asked to retry but no attempts were left, the
+stored reason ends with a ``; retries exhausted (N of M)`` note.
Review Comment:
The `; retries exhausted (N of M)` note is promised here without
qualification, but `task_runner.py` skips it when `max_tries <= 0`, and that is
the default `retries=0` case.
The `Requires Airflow >= 3.3.0` note at the top of the page has also gone
stale for the FAIL path. That path needs `TITerminalStatePayload.retry_reason`,
which lands in 3.4.0 (`airflow-core/src/airflow/__init__.py` reads `3.4.0`,
latest tag is `3.3.2`), while the provider floors at `apache-airflow>=3.0.0`.
On a 3.3.x deployment the sentence being removed here is still the accurate one.
One other gap: the page names only `retry_reason`, which is the name a user
cannot see anywhere. `grep -r state_reason providers/common/ai/docs
airflow-core/docs` comes back empty. Since the point of this change is making
the reason discoverable, a line pointing at the REST field and the Details page
would finish it.
##########
task-sdk/tests/task_sdk/execution_time/schema/test_migrator.py:
##########
@@ -470,3 +470,37 @@ def test_head_version_keeps_arg_bindings(self,
real_migrator, startup_details):
assert isinstance(defaulted, LiteralArgBinding)
assert defaulted.from_default is True
assert defaulted.value_schema.root == {"type": "integer", "format":
"int64"}
+
+
+class TestRealBundleRetryReasonUpgrade:
+ """
+ Drive the *real* supervisor bundle through the ``retry_reason`` migration.
+
+ ``TaskState`` flows foreign-runtime -> supervisor, the opposite direction
from
+ ``arg_bindings`` above, so a runtime pinned to an older schema is exercised
+ through ``upgrade`` rather than ``downgrade``.
+ """
+
+ @pytest.fixture
+ def real_migrator(self) -> SchemaVersionMigrator:
+ return get_schema_version_migrator()
+
+ def test_upgrade_fills_missing_retry_reason_with_none(self, real_migrator):
+ from airflow.sdk.execution_time.comms import TaskState
+
+ body = {"type": "TaskState", "state": "failed", "end_date": None,
"rendered_map_index": None}
+ out = real_migrator.upgrade(body, TaskState, "2026-06-16")
+ assert out["retry_reason"] is None
+
+ def test_upgrade_keeps_retry_reason_at_head(self, real_migrator):
+ from airflow.sdk.execution_time.comms import TaskState
Review Comment:
Not reopening the core-side boundary test you closed. This is narrower: the
gate here does work, but neither test in this class exercises the direction it
governs. I checked both halves rather than reasoning from the code. Deleting
`AddRetryReasonToTaskState` from the bundle leaves both tests green. A
downgrade probe on the same build shows the instruction is live:
`downgrade(TaskState(..., retry_reason="auth error"), "2026-06-16")` comes back
without the key, while `"2026-10-30"` keeps it.
The mechanism matches that.
`schema(TaskState).field("retry_reason").didnt_exist` is filed under cadwyn's
`alter_schema_instructions`, not the `alter_request_by_schema_instructions`
that `SchemaVersionMigrator.upgrade` iterates, and `upgrade` then validates
against the head class, which always carries the field; the comment at
[migrator.py:159-162](https://github.com/apache/airflow/blob/d6bc665a89959d66bc2ad3f3b990bd69140905b3/task-sdk/src/airflow/sdk/execution_time/schema/migrator.py#L159-L162)
says as much. `test_upgrade_keeps_retry_reason_at_head` also hits the
`source_version == supervisor_version` early return, so it reduces to a
pydantic round-trip.
A `downgrade(TaskState(..., retry_reason="x"), "2026-06-16")` asserting the
key is absent would pin the version change, and fails once it is deleted.
`TestRealBundleArgBindingsDowngrade` just above is the template. Minor: the two
function-level `from airflow.sdk.execution_time.comms import TaskState` imports
can move to the top of the file.
--
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]
