kaxil commented on code in PR #63355:
URL: https://github.com/apache/airflow/pull/63355#discussion_r2957813446
##########
airflow-core/src/airflow/api_fastapi/execution_api/routes/task_instances.py:
##########
@@ -294,6 +294,7 @@ def ti_run(
"/{task_instance_id}/state",
status_code=status.HTTP_204_NO_CONTENT,
responses={
+ status.HTTP_200_OK: {"description": "The TI was already in the
requested state"},
Review Comment:
The 200 description says "The TI was already in the requested state" and the
409 on line 299 says "The TI is already in the requested state". These describe
different scenarios but read almost identically. Consider updating the 409
description to something like "The TI is not in a valid state for this
transition" to differentiate.
##########
airflow-core/src/airflow/api_fastapi/execution_api/routes/task_instances.py:
##########
@@ -362,6 +363,20 @@ def ti_update_state(
},
)
+ requested_state = ti_patch_payload.state
+ requested_state_value = (
Review Comment:
The `hasattr(..., "value")` guards are unnecessary. Both
`ti_patch_payload.state` and `previous_state` are `str` enum types
(`TerminalTIState`/`TerminalStateNonSuccess` and `TaskInstanceState`), all of
which always have `.value`. This can be simplified to
`str(ti_patch_payload.state) == str(previous_state)`, or even just compare the
`.value` attributes directly.
##########
airflow-core/src/airflow/api_fastapi/execution_api/routes/task_instances.py:
##########
@@ -362,6 +363,20 @@ def ti_update_state(
},
)
+ requested_state = ti_patch_payload.state
+ requested_state_value = (
+ requested_state.value if hasattr(requested_state, "value") else
str(requested_state)
+ )
+ previous_state_value = previous_state.value if hasattr(previous_state,
"value") else str(previous_state)
+
Review Comment:
This check runs before the `previous_state != RUNNING` guard. If a RUNNING
task receives a state update request where the requested state happens to match
RUNNING, this would return 200 instead of proceeding. In practice this can't
happen because the `TIStateUpdate` discriminated union doesn't include a
RUNNING variant, but worth adding a brief comment clarifying this guard is for
duplicate terminal state updates specifically.
##########
airflow-core/tests/unit/api_fastapi/execution_api/versions/head/test_task_instances.py:
##########
@@ -1539,6 +1539,31 @@ def test_ti_update_state_not_running(self, client,
session, create_task_instance
session.refresh(ti)
assert ti.state == State.SUCCESS
Review Comment:
This covers the duplicate-success scenario from issue #63183, which is good.
Worth also adding a test for a different mismatch (TI in SUCCESS, request sends
FAILED) to confirm it still returns 409. The existing
`test_ti_update_state_not_running` covers this implicitly, but having an
explicit test next to the new idempotent test would make the contract clearer:
only *same-state* duplicates get 200, not all non-running transitions.
--
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]
