From 0bafbedb30aa2dccf0fb8c261103d798fe240149 Mon Sep 17 00:00:00 2001 From: Renn F Date: Mon, 18 May 2026 05:11:16 +0200 Subject: [PATCH] fix(orchestrator): auto-recover blocked parent at PM closure respawn (#177) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #170 made the closure dispatcher auto-resume a `paused` parent before respawning its PM, but only `paused`. A parent that is `blocked` at closure (every descendant already terminal) is an errant/stale block — a child's i_am_blocked propagated, or a PM blocked it and never unblocked — the real dependency is already done. #170 left it as-is, so the respawned PM landed on a blocked parent it cannot submit_up / complete and had to manually `unblock` it first (needs journal:decision) — which models do not reliably do, wedging the whole closure chain forever (observed end-to-end this run: leaf stuck awaiting_pm_review, cell parent blocked, root paused, PMs cycling indefinitely). Add `_auto_recover_blocked_parent` (mirrors `_auto_resume_paused_parent`) and recover `blocked` symmetrically to `paused` in `_maybe_spawn_pm_closure`. `blocked -> in_progress` is lifecycle-valid — it is exactly what `unblock(restore=True)` performs. Scoped to the closure-spawn point (descendants terminal) so a live dependency block is never auto-cleared. Best-effort, like the paused path. 4 new tests mirror the #170 suite (recovered-before-spawn, mutual exclusivity with paused, patch shape, error-swallowing). make quality green. --- roboco/runtime/orchestrator.py | 43 ++++++++++++++- .../runtime/test_pm_closure_auto_resume.py | 53 ++++++++++++++++++- 2 files changed, 94 insertions(+), 2 deletions(-) diff --git a/roboco/runtime/orchestrator.py b/roboco/runtime/orchestrator.py index 9b61996d..0996cfa9 100644 --- a/roboco/runtime/orchestrator.py +++ b/roboco/runtime/orchestrator.py @@ -3301,6 +3301,41 @@ Start by: error=str(e), ) + async def _auto_recover_blocked_parent( + self, client: httpx.AsyncClient, task_id: str + ) -> None: + """Recover a blocked parent right before its PM is respawned for closure. + + #177: symmetric to ``_auto_resume_paused_parent`` (#170). The + closure dispatcher only reaches this point once every descendant + is terminal, so a parent still ``blocked`` here is an errant / + stale block (e.g. a child's i_am_blocked propagated, or a PM + blocked it and never unblocked) — the real dependency is already + done. #170 auto-resumed only ``paused`` parents, so a ``blocked`` + one wedged the whole chain forever: the respawned PM cannot + submit_up / complete a blocked parent and must first ``unblock`` + it (needs journal:decision), which weak models never reliably do + (smoke-19/this run wedged exactly here). ``blocked -> in_progress`` + is lifecycle-valid — it is precisely what ``unblock(restore=True)`` + performs. Best-effort; a failure must not block the spawn (the PM + can still ``unblock`` manually). + """ + try: + await client.patch( + f"{self._api_url}/tasks/{task_id}", + json={"status": "in_progress"}, + ) + logger.info( + "Auto-recovered blocked parent for PM closure respawn", + task_id=task_id, + ) + except Exception as e: + logger.error( + "Failed to auto-recover blocked parent", + task_id=task_id, + error=str(e), + ) + def _select_agent_for_cell(self, cell: str, role: str) -> str | None: """ Select the best available agent for a cell and role. @@ -4306,8 +4341,14 @@ Start now: evidence(task_id="{task_id}") # directly submit_up / complete / escalate — pre-gateway behaviour the # gateway refactor dropped, which wedged smoke-15 (minimax never # issued resume() itself). - if task.get("status") == "paused": + # #177: a parent that is `blocked` at closure (all descendants + # terminal) is an errant/stale block — recover it symmetrically so + # the chain can't wedge forever waiting for a PM to manually unblock. + parent_status = task.get("status") + if parent_status == "paused": await self._auto_resume_paused_parent(client, task_id) + elif parent_status == "blocked": + await self._auto_recover_blocked_parent(client, task_id) prompt = self._build_pm_closure_prompt(task, descendants) await self.spawn_agent( diff --git a/tests/unit/runtime/test_pm_closure_auto_resume.py b/tests/unit/runtime/test_pm_closure_auto_resume.py index 11b8c196..56eba508 100644 --- a/tests/unit/runtime/test_pm_closure_auto_resume.py +++ b/tests/unit/runtime/test_pm_closure_auto_resume.py @@ -7,6 +7,13 @@ gateway refactor dropped that, so the respawned PM had to issue `resume()` itself — which minimax reliably failed, wedging smoke-15. _maybe_spawn_pm_closure must resume a `paused` parent (and only a paused one) immediately before spawning its PM. + +#177: symmetric handling for a `blocked` parent. At closure all +descendants are terminal, so a still-`blocked` parent is an errant/ +stale block — it must be recovered to in_progress too, else the chain +wedges forever waiting for a PM to manually unblock (this run wedged +exactly there). `paused` and `blocked` are mutually exclusive — each +triggers only its own recovery helper. """ from __future__ import annotations @@ -39,6 +46,7 @@ def _ready_orch() -> AgentOrchestrator: orch._task_git_context = MagicMock(return_value=None) # type: ignore[method-assign] orch.spawn_agent = AsyncMock() # type: ignore[method-assign] orch._auto_resume_paused_parent = AsyncMock() # type: ignore[method-assign] + orch._auto_recover_blocked_parent = AsyncMock() # type: ignore[method-assign] return orch @@ -51,12 +59,28 @@ async def test_paused_parent_is_resumed_before_spawn() -> None: await orch._maybe_spawn_pm_closure(client, task) orch._auto_resume_paused_parent.assert_awaited_once_with(client, "parent-1") + orch._auto_recover_blocked_parent.assert_not_awaited() + orch.spawn_agent.assert_awaited_once() + + +@pytest.mark.asyncio +async def test_blocked_parent_is_recovered_before_spawn() -> None: + """#177: a blocked parent at closure is recovered (not the paused path).""" + orch = _ready_orch() + client = AsyncMock() + task = {"id": "parent-2", "status": "blocked", "team": "backend"} + + await orch._maybe_spawn_pm_closure(client, task) + + orch._auto_recover_blocked_parent.assert_awaited_once_with(client, "parent-2") + orch._auto_resume_paused_parent.assert_not_awaited() orch.spawn_agent.assert_awaited_once() @pytest.mark.asyncio async def test_non_paused_parent_is_not_resumed() -> None: - """awaiting_pm_review / in_progress parents must NOT be touched.""" + """awaiting_pm_review / in_progress parents must NOT be touched by + either recovery path.""" for st in ("awaiting_pm_review", "in_progress"): orch = _ready_orch() client = AsyncMock() @@ -65,6 +89,7 @@ async def test_non_paused_parent_is_not_resumed() -> None: await orch._maybe_spawn_pm_closure(client, task) orch._auto_resume_paused_parent.assert_not_awaited() + orch._auto_recover_blocked_parent.assert_not_awaited() orch.spawn_agent.assert_awaited_once() @@ -105,3 +130,29 @@ async def test_auto_resume_swallows_errors() -> None: # Must not raise. await orch._auto_resume_paused_parent(client, "p") + + +@pytest.mark.asyncio +async def test_auto_recover_blocked_patches_status_in_progress() -> None: + """#177: blocked -> in_progress (same transition unblock(restore=True) + performs).""" + orch = _orch() + client = AsyncMock() + + await orch._auto_recover_blocked_parent(client, "parent-7") + + client.patch.assert_awaited_once() + call = client.patch.await_args + assert call.args[0].endswith("/tasks/parent-7") + assert call.kwargs["json"] == {"status": "in_progress"} + + +@pytest.mark.asyncio +async def test_auto_recover_blocked_swallows_errors() -> None: + """A recovery failure must not block the spawn (best-effort).""" + orch = _orch() + client = AsyncMock() + client.patch = AsyncMock(side_effect=RuntimeError("api down")) + + # Must not raise. + await orch._auto_recover_blocked_parent(client, "p")