kaxil commented on code in PR #72155:
URL: https://github.com/apache/airflow/pull/72155#discussion_r3972717396
##########
providers/standard/src/airflow/providers/standard/triggers/hitl.py:
##########
@@ -121,14 +121,18 @@ async def _handle_timeout(self) -> TriggerEvent:
# Case 1: Response arrived just before timeout
if resp.response_received and resp.chosen_options:
if TYPE_CHECKING:
- assert resp.responded_by_user is not None
assert resp.responded_at is not None
chosen_options_list = list(resp.chosen_options or [])
+ responded_by_user = (
+ HITLUser(id=resp.responded_by_user.id,
name=resp.responded_by_user.name)
+ if resp.responded_by_user
+ else None
+ )
self.log.info(
"[HITL] responded_by=%s (id=%s) options=%s at %s (timeout
fallback skipped)",
- resp.responded_by_user.name,
- resp.responded_by_user.id,
+ responded_by_user["name"] if responded_by_user else None,
Review Comment:
On the replay branch this guard exists for, the message now states the
opposite of what happened: `[HITL] responded_by=None (id=None)
options=['Approve'] at 2026-09-09 12:00:00+00:00 (timeout fallback skipped)`.
The fallback was not skipped. It ran in an earlier incarnation of this trigger,
which is exactly why the row has a response with no responder, and this run is
resuming with the result it recorded.
`test_run_replayed_after_timeout_fallback_has_no_responder` asserts that
literal string (`test_hitl.py:245-251`), so merging pins the wrong text behind
a test that reads as deliberate.
Reachability is settled by the guard itself, and it needs only a lapsed
heartbeat rather than a crash: `Trigger.assign_unassigned` reassigns any
trigger whose triggerer's `latest_heartbeat` is older than
`health_check_threshold` (`models/trigger.py:403-424`), and `run()` then
restarts from the top and lands back in Case 1. It reaches shipped standard
operators too, not just `common.ai`, since `HITLEntryOperator` defaults to
`defaults=[OK]` (`operators/hitl.py:555-560`).
Branching on the nullness already computed just above says the right thing
and drops both `... if responded_by_user else None` re-tests:
```python
if responded_by_user is None:
self.log.info(
"[HITL] resuming with the timeout default %s recorded at %s "
"(applied by an earlier run of this trigger)",
chosen_options_list,
resp.responded_at,
)
else:
self.log.info(
"[HITL] responded_by=%s (id=%s) options=%s at %s (timeout fallback
skipped)",
responded_by_user["name"],
responded_by_user["id"],
chosen_options_list,
resp.responded_at,
)
```
This is the trigger-side instance of the earlier point about an unattended
approval being indistinguishable from a human one: this line is the only place
an on-call reader could tell them apart, and right now it names no actor and
gives the wrong cause.
While you are in here, `_handle_response` still has the same shape at
`:207-208` behind an `if TYPE_CHECKING: assert resp.responded_by_user is not
None` (`:185`). `HITLDetailResponse.responded_by_user` is `HITLUser | None =
None` (`task-sdk/src/airflow/sdk/api/datamodels/_generated.py:694`), so that
assert is false, and it is what let the Case 1 dereference past mypy in the
first place. I cannot reach it without clock skew between two triggerers, so it
is not a defect on its own, but a shared `_responder(resp)` helper would cover
both call sites and let the assert go.
##########
providers/standard/tests/unit/standard/triggers/test_hitl.py:
##########
@@ -206,6 +206,50 @@ async def
test_run_should_check_response_in_timeout_handler(
action_datetime,
)
+ @pytest.mark.db_test
+ @pytest.mark.asyncio
+ @mock.patch.object(HITLTrigger, "log")
+ @mock.patch("airflow.sdk.execution_time.hitl.update_hitl_detail_response")
Review Comment:
This patch target never binds, so `mock_update.assert_not_called()` at
`:244` cannot fail. `triggers/hitl.py:36-40` imports
`update_hitl_detail_response` at module load and `:159` calls the trigger
module's own global, so patching the attribute on the source module leaves the
caller pointing at the real function. Reproducing the import shape confirms it:
the source module rebinds to the mock while the consumer's global does not, and
the consumer still calls the original.
That matters here more than in the tests above, because "the replay must not
write a second default" is the invariant this test exists to document. The
other two assertions do fail on main, so the regression coverage is real, but
the headline guard is decorative. Either
`@mock.patch("airflow.providers.standard.triggers.hitl.update_hitl_detail_response")`,
or assert no `UpdateHITLDetail` shows up in
`mock_supervisor_comms.send.call_args_list`. Worth doing in the same pass as
the message change, since that also rewrites the `:245-251` assertion.
##########
providers/common/ai/src/airflow/providers/common/ai/mixins/approval.py:
##########
@@ -179,14 +189,13 @@ def defer_for_approval(
trigger=HITLTrigger(
ti_id=ti_id,
options=[LLMApprovalMixin.APPROVE, LLMApprovalMixin.REJECT],
- defaults=None,
+ defaults=timeout_defaults,
Review Comment:
This line makes `HITLTrigger` Case 3 reachable from `common.ai` for the
first time, so the provider now leans on the Case 1 fix in the same PR, but
`pyproject.toml:72` still floors `apache-airflow-providers-standard>=1.12.1`. I
checked the released tags: 1.12.1, 1.15.0 and 1.18.0 all still carry the
`TYPE_CHECKING`-only assert and the unguarded `resp.responded_by_user.name` /
`.id`. So someone on a 3.1 or 3.2 core who upgrades only `common-ai` gets
`on_approval_timeout` together with the unfixed trigger, and in the replay
window the task fails in precisely the scenario `"approve"` exists to rescue.
Bumping the floor to whichever standard release carries the trigger fix
would close it, with the precedent one line up in the same block
(`apache-airflow-providers-common-compat>=1.15.0`).
##########
providers/common/ai/src/airflow/providers/common/ai/mixins/approval.py:
##########
@@ -219,10 +229,16 @@ def execute_complete(self, context: Context,
generated_output: str, event: dict[
responded_by_user = event.get("responded_by_user")
chosen = event["chosen_options"]
if self.APPROVE not in chosen:
+ if event.get("timedout"):
+ raise HITLRejectException(
+ "Output was rejected automatically: approval_timeout
expired with "
+ "on_approval_timeout='reject'."
+ )
raise HITLRejectException(f"Output was rejected by the reviewer
{responded_by_user}.")
+ log.info("Output approved by %s.", responded_by_user or "the approval
timeout default")
Review Comment:
`responded_by_user` is a `HITLUser` TypedDict, so on the reviewer path this
prints `Output approved by {'id': 'u1', 'name': 'alice'}.` rather than the
name, and `llm_branch.py:160-163` has the same shape. The trigger change in
this PR already formats it as `responded_by_user["name"]`
(`triggers/hitl.py:134`); the new test passes a bare `"admin"` string, which is
a shape no production path produces, so it does not surface there.
##########
providers/common/ai/src/airflow/providers/common/ai/operators/llm.py:
##########
@@ -135,8 +141,18 @@ def __init__(
self._serialize_model_output = serialize_output or not _CORE_WALKER
self.agent_params = agent_params or {}
self.usage_limits = usage_limits
+ if on_approval_timeout not in ("fail",
*LLMApprovalMixin.TIMEOUT_DEFAULTS):
+ raise ValueError(
+ f"on_approval_timeout must be 'fail', 'approve', or 'reject',
got {on_approval_timeout!r}."
+ )
+ if on_approval_timeout != "fail" and not (require_approval and
approval_timeout):
Review Comment:
`timedelta(0)` is falsy, so `approval_timeout=timedelta(0)` trips this check
with a message saying the value is unset when it was passed. On 3.3+ that is a
deadline the core honours: `TaskAwaitingInput(timeout=...)` reaches the
execution API as `if ti_patch_payload.timeout is not None`
(`execution_api/routes/task_instances.py:766-768`), so `trigger_timeout` lands
on `utcnow()` and `check_awaiting_input_timeouts` applies the default on the
next tick. Pre-3.3 the same value means the opposite, because line 195 tests it
for truthiness and leaves `timeout_datetime=None`, so the review waits forever.
With the default `"fail"` this guard never fires, so that split pre-dates
the PR and is not yours to fix here. Swapping in `is not None` alone would
widen it, so the call is which meaning `timedelta(0)` should carry. At minimum
the message should not claim `approval_timeout` is missing when it is set.
--
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]
