mirror of
https://github.com/rennf93/roboco.git
synced 2026-08-03 07:23:24 +02:00
[F093] serialize concurrent live-chat spawns under a per-agent lock
The intake and secretary agent ids are each a single fixed id, so two concurrent start_intake_session / start_secretary_session calls raced on the container name (docker run --name roboco-agent-<id>) and the _instances[<id>] write: both passed the reap-prior check before either registered, both ran docker run, and the last _instances write won, orphaning the other container + its relay. Add _intake_spawn_lock / _secretary_spawn_lock (asyncio.Lock) and wrap the _spawn_intake_container / _spawn_secretary_container bodies so the second start waits for the first to fully register before its own reap-prior check runs. Distinct from self._lock (which stop_agent takes) to avoid a reentrancy deadlock: the spawn body holds the spawn lock then calls stop_agent (acquires self._lock) — lock order is always spawn_lock -> self._lock, never the reverse.
This commit is contained in:
@@ -37,6 +37,9 @@ def _make_minimal_orchestrator() -> AgentOrchestrator:
|
||||
# (F071); without this the post-docker-run guard would AttributeError on
|
||||
# the constructor-skipped instance.
|
||||
orch._running = True
|
||||
# F093: concurrent intake starts serialize on this lock; the constructor
|
||||
# (skipped here) initializes it.
|
||||
orch._intake_spawn_lock = asyncio.Lock()
|
||||
return orch
|
||||
|
||||
|
||||
@@ -472,6 +475,61 @@ class TestSpawnGuarded:
|
||||
assert closed == ["sess-B"]
|
||||
|
||||
|
||||
class TestConcurrentSpawnSerialization:
|
||||
"""Two concurrent intake starts must serialize — the intake agent id is a
|
||||
single fixed id, so two ``docker run --name roboco-agent-prompter`` calls and
|
||||
two ``_instances[INTAKE_AGENT_ID]`` writes racing orphan a container + relay.
|
||||
The spawn body (reap-prior → clone → docker run → register) must run under
|
||||
a per-agent lock so the second start only begins once the first has fully
|
||||
registered (or been reaped).
|
||||
"""
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_concurrent_intake_spawns_do_not_interleave(
|
||||
self, monkeypatch: pytest.MonkeyPatch
|
||||
) -> None:
|
||||
orch = _make_minimal_orchestrator()
|
||||
_wire_spawn_mocks(monkeypatch, orch, run_calls=[])
|
||||
|
||||
# Reap the prior instance on a concurrent start: mock stop_agent so the
|
||||
# second spawn's reap doesn't need the real self._lock (not set on the
|
||||
# minimal orchestrator). Records that the prior instance was reaped.
|
||||
reaped: list[str] = []
|
||||
|
||||
async def _stop(aid: str, **_kw: Any) -> None:
|
||||
reaped.append(aid)
|
||||
|
||||
monkeypatch.setattr(orch, "stop_agent", _stop)
|
||||
|
||||
# Instrument the first await inside the spawn body (the scope clone) to
|
||||
# measure how many spawns are inside the body at once. With a serializing
|
||||
# lock the second spawn is parked on lock.acquire() and can't enter clone
|
||||
# until the first releases (after fully registering) -> max depth 1.
|
||||
# Without the lock both spawns reach clone concurrently -> max depth 2.
|
||||
in_clone = 0
|
||||
max_depth = 0
|
||||
|
||||
async def _clone(*_a: Any, **_kw: Any) -> tuple[str, list[str]]:
|
||||
nonlocal in_clone, max_depth
|
||||
in_clone += 1
|
||||
max_depth = max(max_depth, in_clone)
|
||||
await asyncio.sleep(0) # yield so the other spawn may enter if not locked
|
||||
in_clone -= 1
|
||||
return "/data/workspaces/roboco/board/intake-1", ["/cwd"]
|
||||
|
||||
monkeypatch.setattr(orch, "_clone_intake_scope", _clone)
|
||||
|
||||
await asyncio.gather(
|
||||
orch.spawn_intake_session("sess-a", project_slug="roboco"),
|
||||
orch.spawn_intake_session("sess-b", project_slug="roboco"),
|
||||
)
|
||||
|
||||
assert max_depth == 1 # serialized: never two spawns in the body at once
|
||||
# The second start reaped the first's registered instance (proves the two
|
||||
# spawns ran in order, not concurrently clobbering the registry).
|
||||
assert reaped == [INTAKE_AGENT_ID]
|
||||
|
||||
|
||||
class TestReapIntakeSession:
|
||||
@pytest.mark.asyncio
|
||||
async def test_reap_closes_session_and_stops_container(
|
||||
|
||||
@@ -16,6 +16,7 @@ and abort WITHOUT registering. The guarded wrapper closes the relay silently
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import asyncio
|
||||
from pathlib import Path
|
||||
from types import SimpleNamespace
|
||||
from typing import Any
|
||||
@@ -37,6 +38,9 @@ def _make_orchestrator() -> AgentOrchestrator:
|
||||
orch._instances = {}
|
||||
orch._bg_tasks = set()
|
||||
orch._running = True
|
||||
# F093: concurrent secretary starts serialize on this lock; the constructor
|
||||
# (skipped here) initializes it.
|
||||
orch._secretary_spawn_lock = asyncio.Lock()
|
||||
return orch
|
||||
|
||||
|
||||
@@ -163,3 +167,64 @@ async def test_running_spawn_registers_normally(
|
||||
# Only the pre-spawn reap remove — the shutdown guard did NOT remove the
|
||||
# just-started container (the orchestrator stayed running).
|
||||
assert removed == [f"roboco-agent-{SECRETARY_AGENT_ID}"]
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# F093 — concurrent Secretary starts must serialize. The Secretary agent id is a
|
||||
# single fixed id, so two concurrent ``spawn_secretary_session`` calls race on
|
||||
# the container name (``docker run --name roboco-agent-secretary``) and the
|
||||
# ``_instances[SECRETARY_AGENT_ID]`` write, orphaning a container + relay. The
|
||||
# spawn body runs under ``_secretary_spawn_lock`` so the second start only begins
|
||||
# once the first has fully registered (so the second's reap-prior sees it).
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_concurrent_secretary_spawns_do_not_interleave(
|
||||
monkeypatch: pytest.MonkeyPatch,
|
||||
) -> None:
|
||||
orch = _make_orchestrator()
|
||||
removed: list[str] = []
|
||||
_wire_secretary_spawn_mocks(monkeypatch, orch, removed, flip_running_on_run=False)
|
||||
|
||||
# Reap the prior instance on a concurrent start: mock stop_agent so the
|
||||
# second spawn's reap doesn't need the real self._lock (not set on the
|
||||
# minimal orchestrator).
|
||||
reaped: list[str] = []
|
||||
|
||||
async def _stop(aid: str, **_kw: Any) -> None:
|
||||
reaped.append(aid)
|
||||
|
||||
monkeypatch.setattr(orch, "stop_agent", _stop)
|
||||
|
||||
# Instrument the first await inside the spawn body (route resolution) to
|
||||
# measure how many spawns are inside the body at once. With a serializing
|
||||
# lock the second spawn is parked on lock.acquire() and can't reach the
|
||||
# route call until the first releases -> max depth 1. Without the lock both
|
||||
# spawns reach it concurrently -> max depth 2.
|
||||
in_route = 0
|
||||
max_depth = 0
|
||||
|
||||
async def _route(_aid: str) -> Any:
|
||||
nonlocal in_route, max_depth
|
||||
in_route += 1
|
||||
max_depth = max(max_depth, in_route)
|
||||
await asyncio.sleep(0) # yield so the other spawn may enter if not locked
|
||||
in_route -= 1
|
||||
return SimpleNamespace(
|
||||
provider_type=SimpleNamespace(value="anthropic"),
|
||||
model_name="opus",
|
||||
base_url=None,
|
||||
auth_token=None,
|
||||
)
|
||||
|
||||
monkeypatch.setattr(orch, "_resolve_agent_route", _route)
|
||||
|
||||
await asyncio.gather(
|
||||
orch.spawn_secretary_session("sess-a", initial_message=None),
|
||||
orch.spawn_secretary_session("sess-b", initial_message=None),
|
||||
)
|
||||
|
||||
assert max_depth == 1 # serialized: never two spawns in the body at once
|
||||
# The second start reaped the first's registered instance (ran in order).
|
||||
assert reaped == [SECRETARY_AGENT_ID]
|
||||
|
||||
Reference in New Issue
Block a user