mirror of
https://github.com/rennf93/roboco.git
synced 2026-08-03 07:23:24 +02:00
fix(routing): escape hatch out of mix mode — clear-all overrides + pins warning (#663)
This commit is contained in:
@@ -597,18 +597,100 @@ describe("AIRoutingCard", () => {
|
||||
}
|
||||
});
|
||||
|
||||
it("saving the mix with no picks shows an error and never calls applyMode", async () => {
|
||||
it("saving the mix with no picks confirms, then clears every override via an empty per_agent map", async () => {
|
||||
const confirmSpy = vi.spyOn(window, "confirm").mockReturnValue(true);
|
||||
render(withQueryClient(<AIRoutingCard />));
|
||||
await screen.findByText("Per-agent override (mix mode)");
|
||||
|
||||
fireEvent.click(screen.getByRole("button", { name: "Save mix" }));
|
||||
|
||||
await waitFor(() =>
|
||||
expect(toast.error).toHaveBeenCalledWith(
|
||||
"Pick a model for at least one agent",
|
||||
),
|
||||
expect(applyMode).toHaveBeenCalledWith({ mode: "mix", per_agent: {} }),
|
||||
);
|
||||
confirmSpy.mockRestore();
|
||||
});
|
||||
|
||||
it("declining the empty-save confirm never calls applyMode", async () => {
|
||||
const confirmSpy = vi.spyOn(window, "confirm").mockReturnValue(false);
|
||||
render(withQueryClient(<AIRoutingCard />));
|
||||
await screen.findByText("Per-agent override (mix mode)");
|
||||
|
||||
fireEvent.click(screen.getByRole("button", { name: "Save mix" }));
|
||||
|
||||
await waitFor(() => expect(confirmSpy).toHaveBeenCalled());
|
||||
expect(applyMode).not.toHaveBeenCalled();
|
||||
confirmSpy.mockRestore();
|
||||
});
|
||||
|
||||
describe("Clear all overrides", () => {
|
||||
const PINNED_SNAPSHOT = {
|
||||
mode: "mix",
|
||||
assignments: [
|
||||
{
|
||||
scope: "agent_slug",
|
||||
scope_value: "be-dev-1",
|
||||
model_name: "glm-5.2:cloud",
|
||||
},
|
||||
{
|
||||
scope: "agent_slug",
|
||||
scope_value: "auditor",
|
||||
model_name: "glm-5.2:cloud",
|
||||
},
|
||||
],
|
||||
} as Awaited<ReturnType<typeof getMode>>;
|
||||
|
||||
it("is disabled when nothing is picked and nothing is pinned", async () => {
|
||||
render(withQueryClient(<AIRoutingCard />));
|
||||
await screen.findByText("Per-agent override (mix mode)");
|
||||
|
||||
expect(
|
||||
screen.getByRole("button", { name: "Clear all" }),
|
||||
).toBeDisabled();
|
||||
});
|
||||
|
||||
it("clears persisted pins via an empty per_agent map on confirm", async () => {
|
||||
getMode.mockResolvedValueOnce(PINNED_SNAPSHOT);
|
||||
const confirmSpy = vi.spyOn(window, "confirm").mockReturnValue(true);
|
||||
render(withQueryClient(<AIRoutingCard />));
|
||||
await screen.findByText("Per-agent override (mix mode)");
|
||||
await waitFor(() =>
|
||||
expect(
|
||||
screen.getByRole("button", { name: "Clear all" }),
|
||||
).toBeEnabled(),
|
||||
);
|
||||
|
||||
fireEvent.click(screen.getByRole("button", { name: "Clear all" }));
|
||||
|
||||
await waitFor(() =>
|
||||
expect(applyMode).toHaveBeenCalledWith({ mode: "mix", per_agent: {} }),
|
||||
);
|
||||
confirmSpy.mockRestore();
|
||||
});
|
||||
|
||||
it("declining the confirm never calls applyMode", async () => {
|
||||
getMode.mockResolvedValueOnce(PINNED_SNAPSHOT);
|
||||
const confirmSpy = vi.spyOn(window, "confirm").mockReturnValue(false);
|
||||
render(withQueryClient(<AIRoutingCard />));
|
||||
await screen.findByText("Per-agent override (mix mode)");
|
||||
await waitFor(() =>
|
||||
expect(
|
||||
screen.getByRole("button", { name: "Clear all" }),
|
||||
).toBeEnabled(),
|
||||
);
|
||||
|
||||
fireEvent.click(screen.getByRole("button", { name: "Clear all" }));
|
||||
|
||||
await waitFor(() => expect(confirmSpy).toHaveBeenCalled());
|
||||
expect(applyMode).not.toHaveBeenCalled();
|
||||
confirmSpy.mockRestore();
|
||||
});
|
||||
|
||||
it("pinned overrides surface the outrank warning in the Routing-mode section", async () => {
|
||||
getMode.mockResolvedValueOnce(PINNED_SNAPSHOT);
|
||||
render(withQueryClient(<AIRoutingCard />));
|
||||
|
||||
await screen.findByText(/2 per-agent overrides outrank the global mode/);
|
||||
});
|
||||
});
|
||||
|
||||
// -------------------------------------------------------------------------
|
||||
|
||||
@@ -275,6 +275,10 @@ export function AIRoutingCard() {
|
||||
setMixMap(initialMix);
|
||||
}, [initialMix]);
|
||||
|
||||
// Server-persisted pins (not local edits) — these outrank every mode
|
||||
// button, so the Routing-mode section warns when any exist.
|
||||
const pinnedCount = Object.keys(initialMix).length;
|
||||
|
||||
const catalogForMix = catalog;
|
||||
const catalogOllamaOnly = catalog.filter(
|
||||
(c: { provider_type: ModelProvider }) =>
|
||||
@@ -426,6 +430,26 @@ export function AIRoutingCard() {
|
||||
}
|
||||
};
|
||||
|
||||
const clearAllOverrides = async () => {
|
||||
if (
|
||||
!confirm(
|
||||
"Clear every per-agent override? All agents go back to " +
|
||||
"inherit-global (the active routing mode).",
|
||||
)
|
||||
)
|
||||
return;
|
||||
setMixMap({});
|
||||
if (pinnedCount === 0) return; // nothing persisted server-side
|
||||
try {
|
||||
await applyMode.mutateAsync({ mode: "mix", per_agent: {} });
|
||||
toast.success(
|
||||
"Per-agent overrides cleared — all agents follow the global mode",
|
||||
);
|
||||
} catch (e) {
|
||||
toast.error("Clear failed: " + errMsg(e));
|
||||
}
|
||||
};
|
||||
|
||||
const saveMix = async () => {
|
||||
// Filter out empty picks (nothing selected = inherit global).
|
||||
const per_agent: Record<string, string> = {};
|
||||
@@ -433,7 +457,24 @@ export function AIRoutingCard() {
|
||||
if (model) per_agent[slug] = model;
|
||||
}
|
||||
if (Object.keys(per_agent).length === 0) {
|
||||
toast.error("Pick a model for at least one agent");
|
||||
// Empty save = the explicit clear-all. Without it a fully-pinned
|
||||
// fleet has no way back to the global mode (pins survive every mode
|
||||
// switch by design).
|
||||
if (
|
||||
!confirm(
|
||||
"No models are picked — saving clears every per-agent override " +
|
||||
"so all agents follow the global routing mode. Continue?",
|
||||
)
|
||||
)
|
||||
return;
|
||||
try {
|
||||
await applyMode.mutateAsync({ mode: "mix", per_agent: {} });
|
||||
toast.success(
|
||||
"Per-agent overrides cleared — all agents follow the global mode",
|
||||
);
|
||||
} catch (e) {
|
||||
toast.error("Clear failed: " + errMsg(e));
|
||||
}
|
||||
return;
|
||||
}
|
||||
const needsGrok = Object.values(per_agent).some((m) =>
|
||||
@@ -962,6 +1003,15 @@ export function AIRoutingCard() {
|
||||
labelHint="Unlike every button to the left this never wipes existing routing — it's a one-time additive seed you can re-run anytime. Edit or remove individual rows in the Complexity overrides section below."
|
||||
/>
|
||||
</div>
|
||||
{pinnedCount > 0 ? (
|
||||
<p className="text-xs text-amber-600 flex items-center gap-1">
|
||||
<AlertTriangle className="h-3 w-3 shrink-0" />
|
||||
{pinnedCount} per-agent override{pinnedCount === 1 ? "" : "s"}{" "}
|
||||
outrank the global mode — switching modes won't change those
|
||||
agents. Clear rows in the mix table below (an empty save clears
|
||||
them all).
|
||||
</p>
|
||||
) : null}
|
||||
{currentMode === "mix" && !hasOllamaKey ? (
|
||||
<p className="text-xs text-amber-600 flex items-center gap-1">
|
||||
<AlertTriangle className="h-3 w-3" />
|
||||
@@ -1133,13 +1183,27 @@ export function AIRoutingCard() {
|
||||
Per-agent override (mix mode)
|
||||
</Label>
|
||||
</HelpTip>
|
||||
<Button size="sm" onClick={saveMix} disabled={applyMode.isPending}>
|
||||
{applyMode.isPending ? "Saving…" : "Save mix"}
|
||||
</Button>
|
||||
<div className="flex items-center gap-2">
|
||||
<Button
|
||||
size="sm"
|
||||
variant="outline"
|
||||
onClick={clearAllOverrides}
|
||||
disabled={
|
||||
applyMode.isPending ||
|
||||
(pinnedCount === 0 && Object.values(mixMap).every((m) => !m))
|
||||
}
|
||||
>
|
||||
Clear all
|
||||
</Button>
|
||||
<Button size="sm" onClick={saveMix} disabled={applyMode.isPending}>
|
||||
{applyMode.isPending ? "Saving…" : "Save mix"}
|
||||
</Button>
|
||||
</div>
|
||||
</div>
|
||||
<p className="text-xs text-muted-foreground">
|
||||
Leave a row blank to inherit from the global mode. Saving overwrites
|
||||
all per-agent overrides with what's picked here.
|
||||
all per-agent overrides with what's picked here; Clear all
|
||||
resets every agent back to inherit-global in one click.
|
||||
</p>
|
||||
{agentsError ? (
|
||||
<p className="flex items-center gap-1 rounded-md border p-4 text-xs text-amber-600">
|
||||
|
||||
@@ -194,7 +194,9 @@ class ApplyModeRequest(BaseModel):
|
||||
- mode="mix": clear existing per-agent pins; upsert the `per_agent`
|
||||
map verbatim. Role + GLOBAL rows are left untouched so the user can
|
||||
layer with an existing partial setup. Self-hosted model names in
|
||||
`per_agent` are routed to the LOCAL provider automatically.
|
||||
`per_agent` are routed to the LOCAL provider automatically. An empty
|
||||
map is the explicit clear-all (every pin deleted, nothing re-added);
|
||||
omitting the field is still a 400.
|
||||
- mode="self_hosted": clear every assignment; enable LOCAL provider;
|
||||
set GLOBAL default to `default_model` (a self-hosted model name).
|
||||
- mode="cost_tiered": seed the day-1 cost-tiered compound ROLE(":"complexity)
|
||||
|
||||
+56
-3
@@ -3,6 +3,7 @@ Database base configuration and session management.
|
||||
"""
|
||||
|
||||
import asyncio
|
||||
import weakref
|
||||
from collections.abc import AsyncGenerator
|
||||
from contextlib import asynccontextmanager
|
||||
from pathlib import Path
|
||||
@@ -43,14 +44,63 @@ class Base(DeclarativeBase):
|
||||
|
||||
|
||||
class _DbHolder:
|
||||
"""Holder for database engine and session factory singletons."""
|
||||
"""Holder for database engine and session factory singletons.
|
||||
|
||||
``loop`` weakly tracks the event loop the cached engine belongs to.
|
||||
Pooled asyncpg connections are bound to the loop that created them, so
|
||||
handing the cached engine to a DIFFERENT running loop (a second uvicorn
|
||||
thread, a test's ``asyncio.run``, the eval bench's disposable stack)
|
||||
eventually checks out a loop-foreign pooled connection and dies with
|
||||
``RuntimeError: ... Future attached to a different loop``. ``get_engine``
|
||||
therefore discards and rebuilds the cache on a cross-loop access — the
|
||||
automatic form of the manual ``_DbHolder`` resets the e2e harness used
|
||||
to do between tests, now enforced at the single chokepoint every caller
|
||||
routes through. In production exactly one loop exists, so the check
|
||||
never fires and behavior is unchanged.
|
||||
"""
|
||||
|
||||
engine: AsyncEngine | None = None
|
||||
session_factory: async_sessionmaker[AsyncSession] | None = None
|
||||
loop: "weakref.ref[asyncio.AbstractEventLoop] | None" = None
|
||||
|
||||
|
||||
def _running_loop() -> asyncio.AbstractEventLoop | None:
|
||||
try:
|
||||
return asyncio.get_running_loop()
|
||||
except RuntimeError:
|
||||
return None
|
||||
|
||||
|
||||
def _rebind_holder_to_current_loop() -> None:
|
||||
"""Discard the cached engine/factory when accessed from a foreign loop.
|
||||
|
||||
Rules: no cached engine or no running loop — nothing to do; cached with
|
||||
no loop stamp — claim it for the current loop (covers engines injected
|
||||
directly by tests and sync-created engines); stamp matches the current
|
||||
loop — keep; stamp names another loop, alive or dead — drop both caches
|
||||
so this loop builds its own. The old engine is dropped un-disposed by
|
||||
design: its pool cannot be awaited away from a foreign/dead loop, and
|
||||
any in-flight session still holds its own reference to it.
|
||||
"""
|
||||
if _DbHolder.engine is None:
|
||||
return
|
||||
current = _running_loop()
|
||||
if current is None:
|
||||
return
|
||||
if _DbHolder.loop is None:
|
||||
_DbHolder.loop = weakref.ref(current)
|
||||
return
|
||||
if _DbHolder.loop() is current:
|
||||
return
|
||||
logger.debug("Discarding DB engine bound to a different event loop")
|
||||
_DbHolder.engine = None
|
||||
_DbHolder.session_factory = None
|
||||
_DbHolder.loop = None
|
||||
|
||||
|
||||
def get_engine() -> AsyncEngine:
|
||||
"""Get or create the async engine."""
|
||||
"""Get or create the async engine (rebound per event loop — see holder)."""
|
||||
_rebind_holder_to_current_loop()
|
||||
if _DbHolder.engine is None:
|
||||
_DbHolder.engine = create_async_engine(
|
||||
settings.database_url,
|
||||
@@ -61,11 +111,14 @@ def get_engine() -> AsyncEngine:
|
||||
pool_recycle=settings.database_pool_recycle,
|
||||
pool_pre_ping=True,
|
||||
)
|
||||
current = _running_loop()
|
||||
_DbHolder.loop = weakref.ref(current) if current is not None else None
|
||||
return _DbHolder.engine
|
||||
|
||||
|
||||
def get_session_factory() -> async_sessionmaker[AsyncSession]:
|
||||
"""Get or create the async session factory."""
|
||||
"""Get or create the async session factory (rebound with the engine)."""
|
||||
_rebind_holder_to_current_loop()
|
||||
if _DbHolder.session_factory is None:
|
||||
_DbHolder.session_factory = async_sessionmaker(
|
||||
bind=get_engine(),
|
||||
|
||||
+14
-3
@@ -600,7 +600,9 @@ class ModelRoutingService(BaseService):
|
||||
- "mix": apply per-agent map verbatim. Any agent not in the
|
||||
map falls through to the GLOBAL default — which is whatever it
|
||||
was (preserves prior state). Self-hosted model names (not in the
|
||||
catalog) are automatically routed to the LOCAL provider.
|
||||
catalog) are automatically routed to the LOCAL provider. An empty
|
||||
map (not None) is the explicit clear-all — every pin deleted,
|
||||
nothing re-added (see `_apply_mix`).
|
||||
- "cost_tiered": UNLIKE every mode above, this does NOT wipe
|
||||
anything — it seeds/re-upserts the day-1 `_COST_TIERED_SEED`
|
||||
compound ROLE(":"complexity) rows on top of whatever routing is
|
||||
@@ -790,8 +792,17 @@ class ModelRoutingService(BaseService):
|
||||
self.log.info("Mode applied: self_hosted", default_model=default_model)
|
||||
|
||||
async def _apply_mix(self, per_agent: dict[str, str] | None) -> None:
|
||||
"""Apply a per-agent override map; leave role + global rows untouched."""
|
||||
if not per_agent:
|
||||
"""Apply a per-agent override map; leave role + global rows untouched.
|
||||
|
||||
An EMPTY map is the explicit clear-all: every AGENT_SLUG pin is
|
||||
deleted and nothing is re-added, so all agents fall back to the
|
||||
role/global layer. Without this there was no way out of mix mode at
|
||||
all — mode switches deliberately spare pins, and a fully-pinned fleet
|
||||
(the live 2026-07-23 incident: 25/25 agents pinned) made every mode
|
||||
button an effective no-op forever. ``None`` (map not provided) is
|
||||
still refused — only a deliberate empty map clears.
|
||||
"""
|
||||
if per_agent is None:
|
||||
raise ValueError("mix mode requires a per_agent map")
|
||||
# Clear existing agent-slug overrides so the new map is authoritative.
|
||||
await self.session.execute(
|
||||
|
||||
@@ -110,11 +110,15 @@ async def board_gate_setup(
|
||||
# the SAME test database, so the gate's real writes land where we read them.
|
||||
saved_engine = db_base._DbHolder.engine
|
||||
saved_factory = db_base._DbHolder.session_factory
|
||||
saved_loop = db_base._DbHolder.loop
|
||||
handoff_engine = create_async_engine(_test_database_url, future=True)
|
||||
db_base._DbHolder.engine = handoff_engine
|
||||
db_base._DbHolder.session_factory = async_sessionmaker(
|
||||
bind=handoff_engine, class_=AsyncSession, expire_on_commit=False
|
||||
)
|
||||
# Clear the loop stamp so the per-loop rebind guard claims the injected
|
||||
# engine for this test's loop instead of discarding it as foreign.
|
||||
db_base._DbHolder.loop = None
|
||||
|
||||
# The dispatcher receives tasks from the HTTP API, which serializes
|
||||
# assigned_to as the agent UUID; _resolve_agent_slug maps it back to a slug.
|
||||
@@ -134,6 +138,7 @@ async def board_gate_setup(
|
||||
await handoff_engine.dispose()
|
||||
db_base._DbHolder.engine = saved_engine
|
||||
db_base._DbHolder.session_factory = saved_factory
|
||||
db_base._DbHolder.loop = saved_loop
|
||||
|
||||
|
||||
def _make_orch() -> AgentOrchestrator:
|
||||
|
||||
@@ -8,6 +8,7 @@ and drop/close.
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import asyncio
|
||||
import time
|
||||
from typing import TYPE_CHECKING, cast
|
||||
from unittest.mock import AsyncMock, MagicMock, patch
|
||||
@@ -41,10 +42,12 @@ def _reset_holder() -> Generator[None]:
|
||||
"""Snapshot/restore the singleton so tests don't poison the live engine."""
|
||||
saved_engine = _DbHolder.engine
|
||||
saved_factory = _DbHolder.session_factory
|
||||
saved_loop = _DbHolder.loop
|
||||
_InitState.completed_url = None
|
||||
yield
|
||||
_DbHolder.engine = saved_engine
|
||||
_DbHolder.session_factory = saved_factory
|
||||
_DbHolder.loop = saved_loop
|
||||
_InitState.completed_url = None
|
||||
|
||||
|
||||
@@ -65,6 +68,44 @@ def test_get_engine_creates_and_caches() -> None:
|
||||
assert ce.call_count == 1
|
||||
|
||||
|
||||
def test_get_engine_rebinds_on_a_different_event_loop() -> None:
|
||||
"""The cached engine is per-loop: an access from a second loop discards
|
||||
the first loop's engine (whose pooled connections are loop-bound) and
|
||||
builds a fresh one, instead of dying later with 'Future attached to a
|
||||
different loop' — the e2e/eval-bench multi-loop failure mode."""
|
||||
_DbHolder.engine = None
|
||||
_DbHolder.session_factory = None
|
||||
_DbHolder.loop = None
|
||||
engines = [MagicMock(), MagicMock()]
|
||||
with patch("roboco.db.base.create_async_engine", side_effect=engines) as ce:
|
||||
|
||||
async def _grab() -> object:
|
||||
return get_engine()
|
||||
|
||||
loop_a_engine = asyncio.run(_grab())
|
||||
loop_b_engine = asyncio.run(_grab())
|
||||
assert loop_a_engine is engines[0]
|
||||
# Loop B must NOT reuse loop A's engine.
|
||||
assert loop_b_engine is engines[1]
|
||||
assert ce.call_count == len(engines)
|
||||
|
||||
|
||||
def test_get_engine_same_loop_keeps_the_cache() -> None:
|
||||
_DbHolder.engine = None
|
||||
_DbHolder.session_factory = None
|
||||
_DbHolder.loop = None
|
||||
fake_engine = MagicMock()
|
||||
with patch("roboco.db.base.create_async_engine", return_value=fake_engine) as ce:
|
||||
|
||||
async def _grab_twice() -> tuple[object, object]:
|
||||
return get_engine(), get_engine()
|
||||
|
||||
e1, e2 = asyncio.run(_grab_twice())
|
||||
assert e1 is fake_engine
|
||||
assert e2 is fake_engine
|
||||
assert ce.call_count == 1
|
||||
|
||||
|
||||
def test_get_session_factory_creates_and_caches() -> None:
|
||||
_DbHolder.engine = None
|
||||
_DbHolder.session_factory = None
|
||||
|
||||
@@ -426,6 +426,26 @@ async def test_apply_mode_mix_requires_per_agent(llm_setup: dict) -> None:
|
||||
await svc.apply_mode(mode="mix")
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_apply_mode_mix_empty_map_clears_all_pins(llm_setup: dict) -> None:
|
||||
"""An EMPTY per_agent map is the explicit clear-all — the only way out of
|
||||
a fully-pinned fleet, since mode switches deliberately spare pins (the
|
||||
2026-07-23 live incident: 25/25 pins made every mode button a no-op)."""
|
||||
svc = llm_setup["svc"]
|
||||
model = _first_model_for_type(ModelProvider.ANTHROPIC)
|
||||
await svc.apply_mode(mode="mix", per_agent={"be-dev-1": model, "be-qa": model})
|
||||
assert any(
|
||||
r.scope == AssignmentScope.AGENT_SLUG for r in await svc.list_assignments()
|
||||
)
|
||||
|
||||
await svc.apply_mode(mode="mix", per_agent={})
|
||||
|
||||
rows = await svc.list_assignments()
|
||||
assert not any(r.scope == AssignmentScope.AGENT_SLUG for r in rows)
|
||||
# Nothing left at all -> the mode label escapes "mix".
|
||||
assert await svc.derive_mode() == "anthropic"
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_apply_mode_mix_writes_overrides(llm_setup: dict) -> None:
|
||||
svc = llm_setup["svc"]
|
||||
|
||||
Reference in New Issue
Block a user