Copilot commented on code in PR #63355:
URL: https://github.com/apache/airflow/pull/63355#discussion_r3066474121
##########
airflow-core/src/airflow/api_fastapi/execution_api/routes/task_instances.py:
##########
@@ -300,8 +300,9 @@ 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 route declares a default `204 No Content` success status, but the
handler now also returns `200 OK` for idempotent requests. To reduce client and
OpenAPI ambiguity, consider either (a) returning `204` for the idempotent no-op
as well (still “no content”), or (b) changing the route’s default `status_code`
to `200` and documenting `204` only if it’s actually returned.
##########
airflow-core/tests/unit/api_fastapi/execution_api/versions/head/test_task_instances.py:
##########
@@ -1547,6 +1547,62 @@ def test_ti_update_state_not_running(self, client,
session, create_task_instance
session.refresh(ti)
assert ti.state == State.SUCCESS
+ def test_ti_update_state_same_state_is_idempotent(self, client, session,
create_task_instance):
+ """Test that setting a TI to its current state is treated as an
idempotent no-op."""
+ ti = create_task_instance(
+ task_id="test_ti_update_state_same_state_is_idempotent",
+ state=State.SUCCESS,
+ session=session,
+ start_date=DEFAULT_START_DATE,
+ )
+ ti.end_date = DEFAULT_END_DATE
+ session.commit()
+
+ response = client.patch(
+ f"/execution/task-instances/{ti.id}/state",
+ json={
+ "state": "success",
+ "end_date": timezone.parse("2024-10-31T13:00:00Z").isoformat(),
+ },
+ )
+ assert response.status_code == 200
+ assert response.text == ""
Review Comment:
Asserting `response.text == ""` can be brittle across response
classes/content-types; asserting on `response.content == b""` is a more direct
check for an empty body and avoids any text decoding nuances.
##########
airflow-core/src/airflow/api_fastapi/execution_api/routes/task_instances.py:
##########
@@ -368,6 +369,19 @@ def ti_update_state(
},
)
+ requested_state_value = str(ti_patch_payload.state)
+ previous_state_value = str(previous_state)
+
+ # TIStateUpdate only allows terminal states, so this idempotency check
handles duplicate
+ # terminal updates (for example SUCCESS -> SUCCESS) and cannot match
RUNNING.
+ if requested_state_value == previous_state_value:
+ log.info(
+ "Duplicate state update request received; state already set",
+ requested_state=requested_state_value,
+ previous_state=previous_state_value,
Review Comment:
Using `str(Enum)` for state comparison is unreliable; for `Enum`/`str, Enum`
types it typically renders `"TaskInstanceState.SUCCESS"` rather than the value,
so the idempotency check may never trigger. Compare the enum members directly
(e.g., `ti_patch_payload.state == previous_state`) or compare their `.value`
fields to ensure duplicate terminal updates are correctly detected.
--
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]
