Skip to content

Commit db74ddf

Browse files
committed
Let the HITL review plugin address one loop pass or mapped slot
The plugin located a task instance by (task_id, map_index) and stored its review state as XCom under that address. With loop passes sharing a task id and map index that picks an arbitrary pass, so approving or rejecting could resolve another pass's session and write the human action to it. The plugin endpoints now take region_id and region_index and resolve them by the same live-producer rules as the public API, so an ambiguous selection is rejected instead of guessed. Reading, writing and checking completion all use that exact attempt. The review UI forwards the selected task instance's region coordinates when the host provides them. The plugin also runs on hosts that predate regions, where an unknown region_id would break the request, so a host without task instance context keeps the map-only request shape. A modern host that has not resolved a selection renders no session rather than opening another task's. Tests that re-fetch a task instance to run it need the region too, otherwise they run another pass's row.
1 parent f34b417 commit db74ddf

12 files changed

Lines changed: 547 additions & 47 deletions

File tree

‎airflow-core/tests/unit/api_fastapi/core_api/routes/public/test_extra_links.py‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -143,8 +143,8 @@ def body():
143143
loop = create_loop(body, max_iterations=4)
144144
run = dag_maker.create_dagrun()
145145
sibling = next(ti for ti in run.task_instances if ti.task_id == "body.work" and ti.region_index == 0)
146-
selected_region = DynamicRegion.get_or_create(
147-
dag_id=run.dag_id, run_id=run.run_id, node_id=loop.group_id, session=session
146+
selected_region = DynamicRegion(
147+
dag_id=run.dag_id, run_id=run.run_id, node_id=loop.group_id, forked_from_region_id=sibling.region_id
148148
)
149149
session.add(selected_region)
150150
session.flush()

‎airflow-core/tests/unit/api_fastapi/core_api/routes/public/test_hitl.py‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -71,8 +71,8 @@ def body():
7171
loop = create_loop(body, max_iterations=4)
7272
run = dag_maker.create_dagrun()
7373
region = session.scalar(select(DynamicRegion).where(DynamicRegion.node_id == loop.group_id))
74-
sibling_region = DynamicRegion.get_or_create(
75-
dag_id=run.dag_id, run_id=run.run_id, node_id=loop.group_id, session=session
74+
sibling_region = DynamicRegion(
75+
dag_id=run.dag_id, run_id=run.run_id, node_id=loop.group_id, forked_from_region_id=region.id
7676
)
7777
session.add(sibling_region)
7878
session.flush()

‎devel-common/src/tests_common/test_utils/taskinstance.py‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -20,15 +20,15 @@
2020

2121
import asyncio
2222
import copy
23-
from typing import TYPE_CHECKING
23+
from typing import TYPE_CHECKING, Any
2424

2525
from airflow import settings
2626
from airflow.models.taskinstance import TaskInstance
2727
from airflow.utils.session import NEW_SESSION
2828

2929
from tests_common.test_utils.compat import SerializedBaseOperator, SerializedMappedOperator
3030
from tests_common.test_utils.dag import create_scheduler_dag
31-
from tests_common.test_utils.version_compat import AIRFLOW_V_3_0_PLUS, AIRFLOW_V_3_2_PLUS
31+
from tests_common.test_utils.version_compat import AIRFLOW_V_3_0_PLUS, AIRFLOW_V_3_2_PLUS, AIRFLOW_V_3_4_PLUS
3232

3333
try:
3434
from airflow.serialization.serialized_objects import create_scheduler_operator
@@ -146,11 +146,15 @@ def run_task_instance(
146146
from airflow.sdk.definitions.dag import _run_task
147147

148148
# Session handling is a mess in tests; use a fresh ti to run the task.
149+
# Without a region, get_task_instance applies public_region_filter, which excludes loop regions, so
150+
# a loop pass would not be found and the caller's ti would run instead of this pass's row.
151+
region_kwargs: dict[str, Any] = {"region_id": ti.region_id} if AIRFLOW_V_3_4_PLUS else {}
149152
new_ti = TaskInstance.get_task_instance(
150153
dag_id=ti.dag_id,
151154
run_id=ti.run_id,
152155
task_id=ti.task_id,
153156
map_index=ti.map_index,
157+
**region_kwargs,
154158
**session_kwargs,
155159
)
156160
# Some tests don't even save the ti at all, in which case new_ti is None.

‎providers/common/ai/docs/hitl_review.rst‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -164,6 +164,10 @@ The plugin exposes a FastAPI app at ``/hitl-review``. Base URL:
164164
- ``run_id``: Dag run ID.
165165
- ``task_id``: Task ID.
166166
- ``map_index``: Map index for mapped tasks. Use ``-1`` for non-mapped tasks or index for dynamic mapping.
167+
- ``region_id`` and ``region_index``: Select one pass of a loop. Send them together; ``region_index``
168+
without ``region_id`` is rejected. Only supported on Airflow 3.4 or later; earlier hosts return 400 when
169+
either is sent. A task inside a loop addressed without them returns 400, and a selection that matches
170+
more than one task instance returns 409.
167171

168172
Endpoints
169173
^^^^^^^^^

0 commit comments

Comments
 (0)