mirror of
https://github.com/rennf93/roboco.git
synced 2026-08-03 07:23:24 +02:00
fix(orchestrator): don't auto-restart on graceful exit; tighten role-status
Smoke-8 surfaced a tight respawn loop: QA failed a PR cleanly, container exited 0, then _check_health bumped error_count and respawned QA with the same task_id. But by then the task was in needs_revision (dev's state), so QA's claim_review was rejected — and the cycle repeated on the next health tick. Token-burning loop. Two layers: 1. _check_health now reads docker's exit code. exit_code == 0 → graceful (intentional handoff via i_am_idle / clean shutdown) → reset error_count, do NOT auto-restart. Non-zero → keep the existing crash-retry behavior. Refactored into _inspect_container_state + _handle_stopped_container to keep xenon's complexity check happy. 2. _readiness_check_role_for_status now includes the dev-owned states (needs_revision, verifying) so a misrouted spawn for QA / PM / board on these statuses fails the readiness gate before the gateway has to reject it. Defense in depth — the right path is #1 (don't respawn on clean exit at all), but if some other code path tries to spawn QA on needs_revision the gate now catches it. Tests: 12 new (5 for _check_health graceful/crash matrix + 7 for the expanded role-status table). Pre-gateway names (none of which were needed here) untouched.
This commit is contained in:
Renn F
+101
-53
@@ -2067,12 +2067,24 @@ class AgentOrchestrator:
|
||||
def _readiness_check_role_for_status(
|
||||
agent_id: str, role: str, status: str
|
||||
) -> str | None:
|
||||
"""Verify agent role matches the role expected for the task status."""
|
||||
"""Verify agent role matches the role expected for the task status.
|
||||
|
||||
Handoff states are role-specific. Dev-owned states (in_progress,
|
||||
verifying, needs_revision, paused, blocked) are restricted to
|
||||
developer/documenter to defang the smoke-8 bug where QA got
|
||||
respawned on a `needs_revision` task via the crash-restart path
|
||||
and immediately hit ``role 'qa' may not claim from status
|
||||
'needs_revision'`` at the gateway.
|
||||
"""
|
||||
role_mismatch: dict[str, str | set[str]] = {
|
||||
"awaiting_qa": "qa",
|
||||
"awaiting_documentation": "documenter",
|
||||
"awaiting_pm_review": {"cell_pm", "main_pm"},
|
||||
"awaiting_ceo_approval": "ceo",
|
||||
# Dev-owned states — only developer/documenter may claim or
|
||||
# resume work here. PMs / QA spawning on these is a misroute.
|
||||
"needs_revision": {"developer", "documenter"},
|
||||
"verifying": {"developer", "documenter"},
|
||||
}
|
||||
required = role_mismatch.get(status)
|
||||
if required is None:
|
||||
@@ -2863,67 +2875,103 @@ Start by:
|
||||
error=str(e),
|
||||
)
|
||||
|
||||
@staticmethod
|
||||
async def _inspect_container_state(
|
||||
container_name: str,
|
||||
) -> tuple[bool, int | None]:
|
||||
"""Return (is_running, exit_code) from `docker inspect`.
|
||||
|
||||
exit_code is None when the output is missing or unparseable; the
|
||||
caller treats None as a crash for safety (smoke-8 fix).
|
||||
"""
|
||||
proc = await asyncio.create_subprocess_exec(
|
||||
"docker",
|
||||
"inspect",
|
||||
"-f",
|
||||
"{{.State.Running}} {{.State.ExitCode}}",
|
||||
container_name,
|
||||
stdout=asyncio.subprocess.PIPE,
|
||||
stderr=asyncio.subprocess.DEVNULL,
|
||||
)
|
||||
stdout, _ = await proc.communicate()
|
||||
parts = stdout.decode().strip().split()
|
||||
is_running = bool(parts) and parts[0] == "true"
|
||||
try:
|
||||
exit_code = int(parts[1]) if len(parts) > 1 and parts[1] else None
|
||||
except ValueError:
|
||||
exit_code = None
|
||||
return is_running, exit_code
|
||||
|
||||
async def _handle_stopped_container(
|
||||
self, agent_id: str, instance: Any, exit_code: int | None
|
||||
) -> None:
|
||||
"""Update state + auto-restart only when the exit was non-zero.
|
||||
|
||||
Smoke-8 evidence: graceful exits (exit 0 — agent called i_am_idle)
|
||||
were treated as crashes by the old logic. The health check bumped
|
||||
error_count and respawned the agent with the prior task_id even if
|
||||
the task had since moved into a state the role can't claim from
|
||||
(e.g. QA → needs_revision). Now: clean exits reset error_count and
|
||||
do nothing; non-zero exits keep the existing crash-retry behaviour.
|
||||
"""
|
||||
cid = instance.container_id[:12] if instance.container_id else None
|
||||
graceful = exit_code == 0
|
||||
if graceful:
|
||||
logger.info(
|
||||
"Agent container exited gracefully",
|
||||
agent_id=agent_id,
|
||||
container_id=cid,
|
||||
exit_code=exit_code,
|
||||
)
|
||||
else:
|
||||
logger.warning(
|
||||
"Agent container stopped unexpectedly",
|
||||
agent_id=agent_id,
|
||||
container_id=cid,
|
||||
exit_code=exit_code,
|
||||
)
|
||||
instance.state = AgentState.OFFLINE
|
||||
instance.container_id = None
|
||||
if graceful:
|
||||
instance.error_count = 0
|
||||
return
|
||||
instance.error_count += 1
|
||||
max_retries = 3
|
||||
if instance.error_count < max_retries:
|
||||
logger.info("Auto-restarting crashed agent", agent_id=agent_id)
|
||||
await self.spawn_agent(
|
||||
agent_id=agent_id,
|
||||
task_id=instance.current_task_id,
|
||||
git_context=(instance.config.git_context if instance.config else None),
|
||||
)
|
||||
elif instance.error_count == max_retries:
|
||||
# Exactly at the threshold — escalate once to humans so a
|
||||
# stranded agent doesn't die silently. Subsequent crashes
|
||||
# stay quiet to avoid notification spam.
|
||||
logger.error(
|
||||
"Agent exceeded max restart attempts; escalating",
|
||||
agent_id=agent_id,
|
||||
error_count=instance.error_count,
|
||||
task_id=instance.current_task_id,
|
||||
)
|
||||
await self._notify_agent_stranded(
|
||||
agent_id=agent_id,
|
||||
error_count=instance.error_count,
|
||||
task_id=instance.current_task_id,
|
||||
)
|
||||
|
||||
async def _check_health(self) -> None:
|
||||
"""Check health of all running agents."""
|
||||
for agent_id, instance in list(self._instances.items()):
|
||||
if instance.state not in (AgentState.ACTIVE, AgentState.WAITING_SHORT):
|
||||
continue
|
||||
|
||||
if instance.container_id is None:
|
||||
continue
|
||||
|
||||
# Check if container is still running
|
||||
container_name = f"roboco-agent-{agent_id}"
|
||||
proc = await asyncio.create_subprocess_exec(
|
||||
"docker",
|
||||
"inspect",
|
||||
"-f",
|
||||
"{{.State.Running}}",
|
||||
container_name,
|
||||
stdout=asyncio.subprocess.PIPE,
|
||||
stderr=asyncio.subprocess.DEVNULL,
|
||||
is_running, exit_code = await self._inspect_container_state(
|
||||
f"roboco-agent-{agent_id}"
|
||||
)
|
||||
stdout, _ = await proc.communicate()
|
||||
|
||||
is_running = stdout.decode().strip() == "true"
|
||||
|
||||
if not is_running:
|
||||
cid = instance.container_id[:12] if instance.container_id else None
|
||||
logger.warning(
|
||||
"Agent container stopped",
|
||||
agent_id=agent_id,
|
||||
container_id=cid,
|
||||
)
|
||||
instance.state = AgentState.OFFLINE
|
||||
instance.error_count += 1
|
||||
instance.container_id = None
|
||||
|
||||
# Auto-restart if not too many errors
|
||||
max_retries = 3
|
||||
if instance.error_count < max_retries:
|
||||
logger.info("Auto-restarting agent", agent_id=agent_id)
|
||||
await self.spawn_agent(
|
||||
agent_id=agent_id,
|
||||
task_id=instance.current_task_id,
|
||||
git_context=(
|
||||
instance.config.git_context if instance.config else None
|
||||
),
|
||||
)
|
||||
elif instance.error_count == max_retries:
|
||||
# Exactly at the threshold — escalate once to humans so a
|
||||
# stranded agent doesn't die silently. Subsequent crashes
|
||||
# stay quiet to avoid notification spam.
|
||||
logger.error(
|
||||
"Agent exceeded max restart attempts; escalating",
|
||||
agent_id=agent_id,
|
||||
error_count=instance.error_count,
|
||||
task_id=instance.current_task_id,
|
||||
)
|
||||
await self._notify_agent_stranded(
|
||||
agent_id=agent_id,
|
||||
error_count=instance.error_count,
|
||||
task_id=instance.current_task_id,
|
||||
)
|
||||
await self._handle_stopped_container(agent_id, instance, exit_code)
|
||||
|
||||
async def _notify_agent_stranded(
|
||||
self,
|
||||
|
||||
@@ -0,0 +1,153 @@
|
||||
"""Smoke-8: _check_health distinguishes graceful (exit 0) from crash exits.
|
||||
|
||||
Original bug: every container stop bumped error_count and triggered
|
||||
spawn_agent(agent_id, task_id=instance.current_task_id). After QA failed a
|
||||
PR and cleanly idled, the health check respawned QA on the (now
|
||||
needs_revision) task — the gateway rejected claim_review every time, and
|
||||
QA respawned again on the next health tick. Token-burning tight loop.
|
||||
|
||||
Fix: read exit code via `docker inspect`. exit_code == 0 → graceful;
|
||||
reset error_count and DO NOT auto-restart. Non-zero → crash; keep
|
||||
existing retry behavior.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
from unittest.mock import AsyncMock, MagicMock, patch
|
||||
from uuid import uuid4
|
||||
|
||||
import pytest
|
||||
from roboco.runtime.orchestrator import AgentOrchestrator, AgentState
|
||||
|
||||
|
||||
def _make_orchestrator() -> AgentOrchestrator:
|
||||
with patch.object(AgentOrchestrator, "__init__", return_value=None):
|
||||
orch = AgentOrchestrator.__new__(AgentOrchestrator)
|
||||
orch._instances = {}
|
||||
orch._lock = MagicMock()
|
||||
return orch
|
||||
|
||||
|
||||
def _instance(task_id: str | None) -> MagicMock:
|
||||
inst = MagicMock()
|
||||
inst.state = AgentState.ACTIVE
|
||||
inst.container_id = "deadbeef1234"
|
||||
inst.current_task_id = task_id
|
||||
inst.error_count = 0
|
||||
inst.config = MagicMock(git_context=None)
|
||||
return inst
|
||||
|
||||
|
||||
async def _docker_inspect_returning(*, running: bool, exit_code: int) -> bytes:
|
||||
return f"{'true' if running else 'false'} {exit_code}\n".encode()
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_graceful_exit_does_not_respawn() -> None:
|
||||
"""Container exit_code=0 means clean shutdown. No auto-restart."""
|
||||
orch = _make_orchestrator()
|
||||
inst = _instance(task_id=str(uuid4()))
|
||||
orch._instances["be-qa"] = inst
|
||||
|
||||
proc = MagicMock()
|
||||
proc.communicate = AsyncMock(return_value=(b"false 0\n", b""))
|
||||
spawn = AsyncMock()
|
||||
orch.spawn_agent = spawn
|
||||
|
||||
with patch("asyncio.create_subprocess_exec", AsyncMock(return_value=proc)):
|
||||
await orch._check_health()
|
||||
|
||||
spawn.assert_not_awaited()
|
||||
assert inst.state == AgentState.OFFLINE
|
||||
assert inst.error_count == 0, (
|
||||
"Graceful exit must reset error_count, not bump it. Otherwise a "
|
||||
"long-running agent that idles clean every time eventually trips "
|
||||
"max_retries and gets flagged as stranded."
|
||||
)
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_crash_exit_triggers_restart() -> None:
|
||||
"""Container exit_code != 0 means crash. Auto-restart (existing behavior)."""
|
||||
orch = _make_orchestrator()
|
||||
task_id = str(uuid4())
|
||||
inst = _instance(task_id=task_id)
|
||||
orch._instances["be-dev-1"] = inst
|
||||
|
||||
proc = MagicMock()
|
||||
proc.communicate = AsyncMock(return_value=(b"false 137\n", b""))
|
||||
spawn = AsyncMock()
|
||||
orch.spawn_agent = spawn
|
||||
|
||||
with patch("asyncio.create_subprocess_exec", AsyncMock(return_value=proc)):
|
||||
await orch._check_health()
|
||||
|
||||
spawn.assert_awaited_once()
|
||||
args = spawn.await_args.kwargs
|
||||
assert args["agent_id"] == "be-dev-1"
|
||||
assert args["task_id"] == task_id
|
||||
assert inst.error_count == 1
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_still_running_no_action() -> None:
|
||||
"""If the container is still running, no state change."""
|
||||
orch = _make_orchestrator()
|
||||
inst = _instance(task_id=str(uuid4()))
|
||||
orch._instances["be-dev-1"] = inst
|
||||
|
||||
proc = MagicMock()
|
||||
proc.communicate = AsyncMock(return_value=(b"true 0\n", b""))
|
||||
spawn = AsyncMock()
|
||||
orch.spawn_agent = spawn
|
||||
|
||||
with patch("asyncio.create_subprocess_exec", AsyncMock(return_value=proc)):
|
||||
await orch._check_health()
|
||||
|
||||
spawn.assert_not_awaited()
|
||||
assert inst.state == AgentState.ACTIVE
|
||||
assert inst.error_count == 0
|
||||
assert inst.container_id == "deadbeef1234"
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_crash_max_retries_does_not_restart() -> None:
|
||||
"""Hit max_retries → don't restart (existing behavior preserved)."""
|
||||
orch = _make_orchestrator()
|
||||
inst = _instance(task_id=str(uuid4()))
|
||||
starting_error_count = 3
|
||||
inst.error_count = starting_error_count
|
||||
orch._instances["be-dev-1"] = inst
|
||||
|
||||
proc = MagicMock()
|
||||
proc.communicate = AsyncMock(return_value=(b"false 1\n", b""))
|
||||
spawn = AsyncMock()
|
||||
orch.spawn_agent = spawn
|
||||
orch._notify_agent_stranded = AsyncMock()
|
||||
|
||||
with patch("asyncio.create_subprocess_exec", AsyncMock(return_value=proc)):
|
||||
await orch._check_health()
|
||||
|
||||
spawn.assert_not_awaited()
|
||||
assert inst.error_count == starting_error_count + 1
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_malformed_inspect_treated_as_crash() -> None:
|
||||
"""If `docker inspect` returns malformed output, default to crash path."""
|
||||
orch = _make_orchestrator()
|
||||
inst = _instance(task_id=str(uuid4()))
|
||||
orch._instances["be-dev-1"] = inst
|
||||
|
||||
proc = MagicMock()
|
||||
# No exit code field at all.
|
||||
proc.communicate = AsyncMock(return_value=(b"false\n", b""))
|
||||
spawn = AsyncMock()
|
||||
orch.spawn_agent = spawn
|
||||
|
||||
with patch("asyncio.create_subprocess_exec", AsyncMock(return_value=proc)):
|
||||
await orch._check_health()
|
||||
|
||||
# exit_code is None → not graceful → counts as crash.
|
||||
spawn.assert_awaited_once()
|
||||
assert inst.error_count == 1
|
||||
@@ -0,0 +1,75 @@
|
||||
"""Smoke-8: _readiness_check_role_for_status covers dev-owned states.
|
||||
|
||||
Original gap: the role-mismatch table only mapped handoff states
|
||||
(awaiting_qa, awaiting_documentation, awaiting_pm_review,
|
||||
awaiting_ceo_approval) to required roles. needs_revision/verifying had
|
||||
no entry, so a QA spawn for a needs_revision task passed the readiness
|
||||
check and the gateway rejected claim_review afterwards. Defense in depth
|
||||
layered behind the _check_health fix.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
from roboco.runtime.orchestrator import AgentOrchestrator
|
||||
|
||||
|
||||
def test_qa_on_needs_revision_blocked() -> None:
|
||||
"""QA cannot be spawned for a needs_revision task."""
|
||||
reason = AgentOrchestrator._readiness_check_role_for_status(
|
||||
agent_id="be-qa", role="qa", status="needs_revision"
|
||||
)
|
||||
assert reason is not None
|
||||
assert "needs_revision" in reason
|
||||
assert "qa" in reason
|
||||
|
||||
|
||||
def test_pm_on_needs_revision_blocked() -> None:
|
||||
"""PM cannot be spawned for a needs_revision task either."""
|
||||
reason = AgentOrchestrator._readiness_check_role_for_status(
|
||||
agent_id="be-pm", role="cell_pm", status="needs_revision"
|
||||
)
|
||||
assert reason is not None
|
||||
|
||||
|
||||
def test_developer_on_needs_revision_allowed() -> None:
|
||||
"""Developer (and documenter) ARE the right roles for needs_revision."""
|
||||
reason = AgentOrchestrator._readiness_check_role_for_status(
|
||||
agent_id="be-dev-1", role="developer", status="needs_revision"
|
||||
)
|
||||
assert reason is None
|
||||
|
||||
|
||||
def test_documenter_on_needs_revision_allowed() -> None:
|
||||
"""Documenter can rework — same dev/doc-owned set."""
|
||||
reason = AgentOrchestrator._readiness_check_role_for_status(
|
||||
agent_id="be-doc", role="documenter", status="needs_revision"
|
||||
)
|
||||
assert reason is None
|
||||
|
||||
|
||||
def test_qa_on_verifying_blocked() -> None:
|
||||
"""Verifying belongs to the dev/doc roles, not QA."""
|
||||
reason = AgentOrchestrator._readiness_check_role_for_status(
|
||||
agent_id="be-qa", role="qa", status="verifying"
|
||||
)
|
||||
assert reason is not None
|
||||
|
||||
|
||||
def test_qa_on_awaiting_qa_allowed() -> None:
|
||||
"""The original handoff case still works — QA on awaiting_qa is fine."""
|
||||
reason = AgentOrchestrator._readiness_check_role_for_status(
|
||||
agent_id="be-qa", role="qa", status="awaiting_qa"
|
||||
)
|
||||
assert reason is None
|
||||
|
||||
|
||||
def test_unmapped_status_allows_any_role() -> None:
|
||||
"""Statuses with no role lock (pending, paused, blocked, etc.) pass."""
|
||||
for status in ("pending", "claimed", "in_progress", "paused", "blocked"):
|
||||
for role in ("developer", "qa", "documenter", "cell_pm"):
|
||||
assert (
|
||||
AgentOrchestrator._readiness_check_role_for_status(
|
||||
agent_id="x", role=role, status=status
|
||||
)
|
||||
is None
|
||||
), f"role={role} status={status} should not be rejected by this gate"
|
||||
Reference in New Issue
Block a user