diff --git a/roboco/runtime/orchestrator.py b/roboco/runtime/orchestrator.py index cbfdf038..c0906399 100644 --- a/roboco/runtime/orchestrator.py +++ b/roboco/runtime/orchestrator.py @@ -2941,13 +2941,38 @@ Start by: async def _check_parent_branch_ready( self, client: httpx.AsyncClient, task_id: str, parent_id: str ) -> str | None: - """Verify the parent task has a branch; auto-block + return msg if not.""" + """Verify the parent task has a branch; auto-block + return msg if not. + + Race window: the PM's `i_will_plan` claims the parent (transitions + status -> in_progress, sets assigned_to) and then `_finalize_claim` + creates the branch via `_ensure_branch_for_task`. Both actions land + in the same DB transaction but a child dev's spawn dispatch can fire + microseconds before that transaction commits and see branch_name=None. + Without retry we'd auto-block the child unnecessarily. + + When the parent is clearly mid-claim (in_progress + assigned_to set) + re-fetch up to 3 times with a 250ms delay before giving up. Total + worst-case wait is 750ms — well inside the dispatcher's tick budget + and only paid when the race actually triggers. Real misses (parent + still pending or unassigned) auto-block immediately as before. + """ parent_resp = await client.get(f"{self._api_url}/tasks/{parent_id}") if not parent_resp.is_success: return None parent = parent_resp.json() if parent.get("branch_name"): return None + + if parent.get("status") == "in_progress" and parent.get("assigned_to"): + for _ in range(3): + await asyncio.sleep(0.25) + parent_resp = await client.get(f"{self._api_url}/tasks/{parent_id}") + if not parent_resp.is_success: + continue + parent = parent_resp.json() + if parent.get("branch_name"): + return None + await self._auto_block_task( client, task_id, diff --git a/tests/unit/runtime/test_parent_branch_race.py b/tests/unit/runtime/test_parent_branch_race.py new file mode 100644 index 00000000..f9b2a766 --- /dev/null +++ b/tests/unit/runtime/test_parent_branch_race.py @@ -0,0 +1,149 @@ +"""Dispatcher gives the parent's claim transaction a moment to commit before +auto-blocking a child task on a missing parent branch. + +Race scenario: PM's `i_will_plan` and a child dev's spawn dispatch fire in the +same tick. `i_will_plan` claims the parent (transitions to `in_progress`, +populates `assigned_to`, then creates the parent branch via +`_finalize_claim -> _ensure_branch_for_task`). Until that transaction commits, +the dispatcher's `_check_parent_branch_ready` sees `branch_name=None` even +though the branch is microseconds away from landing. + +Without retry the dispatcher auto-blocks the child immediately and the dev +sits idle until the next 30s dispatch tick. With a tight 3x250ms retry — only +when the parent is mid-claim (`status=in_progress` AND `assigned_to` set) — we +absorb the sub-second commit gap without delaying the legitimate-block path. +""" + +from __future__ import annotations + +from typing import Any +from unittest.mock import AsyncMock, MagicMock, patch +from uuid import uuid4 + +import pytest +from roboco.runtime.orchestrator import AgentOrchestrator + + +def _make_orch() -> AgentOrchestrator: + """Build a bare orchestrator without running __init__ (no settings I/O).""" + orch = AgentOrchestrator.__new__(AgentOrchestrator) + return orch + + +def _resp(payload: dict[str, Any]) -> MagicMock: + """httpx.Response stand-in: is_success + .json() match the real API.""" + r = MagicMock() + r.is_success = True + r.json.return_value = payload + return r + + +@pytest.mark.asyncio +async def test_retries_when_parent_is_mid_claim_and_branch_lands() -> None: + """First fetch sees null branch (PM transaction in flight); second fetch + sees the committed branch. We must NOT auto-block in this race.""" + task_id = str(uuid4()) + parent_id = str(uuid4()) + + orch = _make_orch() + + client = AsyncMock() + client.get.side_effect = [ + _resp( + { + "id": parent_id, + "branch_name": None, + "status": "in_progress", + "assigned_to": "main-pm", + } + ), + _resp( + { + "id": parent_id, + "branch_name": "feature/backend/PARENT01", + "status": "in_progress", + "assigned_to": "main-pm", + } + ), + ] + + with ( + patch.object(orch, "_auto_block_task", new=AsyncMock()) as auto_block, + patch( + "roboco.runtime.orchestrator.asyncio.sleep", new=AsyncMock() + ) as sleep_mock, + ): + result = await orch._check_parent_branch_ready(client, task_id, parent_id) + + assert result is None, "branch landed on retry; child must not be blocked" + auto_block.assert_not_awaited() + expected_get_calls = 2 + assert client.get.await_count == expected_get_calls + sleep_mock.assert_awaited_once() + + +@pytest.mark.asyncio +async def test_no_retry_when_parent_not_mid_claim() -> None: + """If the parent is NOT in mid-claim shape, retry is skipped and we + auto-block immediately — preserves today's behavior for real misses.""" + task_id = str(uuid4()) + parent_id = str(uuid4()) + + orch = _make_orch() + + client = AsyncMock() + client.get.return_value = _resp( + { + "id": parent_id, + "branch_name": None, + "status": "pending", + "assigned_to": None, + } + ) + + with ( + patch.object(orch, "_auto_block_task", new=AsyncMock()) as auto_block, + patch( + "roboco.runtime.orchestrator.asyncio.sleep", new=AsyncMock() + ) as sleep_mock, + ): + result = await orch._check_parent_branch_ready(client, task_id, parent_id) + + assert result is not None and "waiting for parent branch" in result + auto_block.assert_awaited_once() + client.get.assert_awaited_once() + sleep_mock.assert_not_awaited() + + +@pytest.mark.asyncio +async def test_blocks_after_max_retries_exhausted() -> None: + """If the parent stays in mid-claim shape with null branch through every + retry, give up and auto-block. 3 retries -> 4 total fetches.""" + task_id = str(uuid4()) + parent_id = str(uuid4()) + + orch = _make_orch() + + mid_claim = { + "id": parent_id, + "branch_name": None, + "status": "in_progress", + "assigned_to": "main-pm", + } + client = AsyncMock() + client.get.return_value = _resp(mid_claim) + + with ( + patch.object(orch, "_auto_block_task", new=AsyncMock()) as auto_block, + patch( + "roboco.runtime.orchestrator.asyncio.sleep", new=AsyncMock() + ) as sleep_mock, + ): + result = await orch._check_parent_branch_ready(client, task_id, parent_id) + + assert result is not None and "waiting for parent branch" in result + auto_block.assert_awaited_once() + expected_get_calls = 4 # 1 initial + 3 retries + expected_sleep_calls = 3 + assert client.get.await_count == expected_get_calls + assert sleep_mock.await_count == expected_sleep_calls