From 84b65da9a657a616fc7858f28ca60b55f7479193 Mon Sep 17 00:00:00 2001 From: ciregenz Date: Mon, 7 Sep 2026 18:33:00 -0700 Subject: [PATCH] [eric] retry pill: a CLI retry carries its kind (unreachable, provider error, rate limit, login) onto the crumb, the wire, the pill and the recovered envelope; half the fleet's provider busy was a router giving no answer Co-Authored-By: Claude Fable 5.1 --- backend/apps/agents/core/flight_recorder.py | 4 +- .../manager/streaming/note_provider_retry.py | 31 +++++++++++++- .../apps/agents/manager/streaming/state.py | 1 + backend/tests/test_provider_retry_ledger.py | 42 +++++++++++++++++++ .../pages/AgentChat/shell/RateLimitPill.tsx | 11 +++-- .../src/shared/providerRetryLabel.test.ts | 21 ++++++++++ frontend/src/shared/providerRetryLabel.ts | 23 ++++++++++ frontend/src/shared/state/agentsSlice.ts | 5 ++- frontend/src/shared/statusLabel.test.ts | 6 +++ frontend/src/shared/statusLabel.ts | 4 +- frontend/src/shared/ws/WebSocketManager.ts | 1 + 11 files changed, 139 insertions(+), 10 deletions(-) create mode 100644 frontend/src/shared/providerRetryLabel.test.ts create mode 100644 frontend/src/shared/providerRetryLabel.ts diff --git a/backend/apps/agents/core/flight_recorder.py b/backend/apps/agents/core/flight_recorder.py index 07171eb0..2189267b 100644 --- a/backend/apps/agents/core/flight_recorder.py +++ b/backend/apps/agents/core/flight_recorder.py @@ -132,7 +132,8 @@ def build_envelope( @typechecked -def record_recovery(session_id: str, net: str, model: Optional[str], attempts: int, sessions: Optional[Dict[str, object]] = None) -> None: +def record_recovery(session_id: str, net: str, model: Optional[str], attempts: int, sessions: Optional[Dict[str, object]] = None, + detail: Optional[Dict[str, object]] = None) -> None: """The near-miss ledger: a silent recovery the user never saw still counts in analytics, so 'how often do the nets fire' has a denominator. Fire-and-forget; failures never block the turn.""" crumb(session_id, "recovered", net=net, attempts=attempts) @@ -147,6 +148,7 @@ def record_recovery(session_id: str, net: str, model: Optional[str], attempts: i "attempts": attempts, "journey": journey_auth_context(), "concurrency": concurrency_snapshot(sessions or {}), + **(detail or {}), }) except Exception: pass diff --git a/backend/apps/agents/manager/streaming/note_provider_retry.py b/backend/apps/agents/manager/streaming/note_provider_retry.py index 458de59d..3b6f5854 100644 --- a/backend/apps/agents/manager/streaming/note_provider_retry.py +++ b/backend/apps/agents/manager/streaming/note_provider_retry.py @@ -6,7 +6,8 @@ dies carries "the provider 500'd four times first" in its envelope instead of an timeout. Counting it as a RECOVERED near-miss happens later, at turn end, because a retry that is still in flight has not recovered anything yet.""" -from typing import Optional +from collections import Counter +from typing import Literal, Optional from typeguard import typechecked @@ -14,6 +15,25 @@ from backend.apps.agents.core import flight_recorder from backend.apps.agents.manager.streaming.state import TurnState +RetryKind = Literal["unreachable", "auth", "rate_limit", "provider_error", "other"] + + +@typechecked +def retry_kind(status: object, error: object) -> RetryKind: + """What the CLI is actually waiting on. Fleet, 7 days to 2026-09-07: 121 of 245 retries had NO status at + all (the request never got an answer: our router down or restarting, or the network gone), 98 were the + router's own 502 for an upstream it could not reach, 13 were 429s. One word for all of them hid that.""" + if not isinstance(status, int): + return "unreachable" + if status in (401, 403) or error == "authentication_failed": + return "auth" + if status == 429: + return "rate_limit" + if status >= 500: + return "provider_error" + return "other" + + @typechecked def note_provider_retry(session_id: str, raw: object, turn: TurnState) -> None: """Record one CLI-internal provider retry. Never raises; diagnostics must not break a turn.""" @@ -24,11 +44,14 @@ def note_provider_retry(session_id: str, raw: object, turn: TurnState) -> None: turn.provider_retries += 1 delay_ms = data.get("retry_delay_ms") turn.provider_retry_wait_ms += int(delay_ms) if isinstance(delay_ms, int) else 0 + p_kind = retry_kind(data.get("error_status"), data.get("error")) + turn.provider_retry_kinds.append(p_kind) flight_recorder.crumb( session_id, "provider-retry", status=data.get("error_status"), error=str(data.get("error", ""))[:40], + kind=p_kind, attempt=data.get("attempt"), delay_ms=delay_ms, ) @@ -39,6 +62,7 @@ def note_provider_retry(session_id: str, raw: object, turn: TurnState) -> None: "session_id": session_id, "attempt": data.get("attempt"), "delay_ms": delay_ms if isinstance(delay_ms, int) else None, + "kind": p_kind, })) except Exception: pass @@ -50,4 +74,7 @@ def settle_provider_retries(session_id: str, turn: TurnState, model: Optional[st a denominator in the near-miss ledger.""" if turn.provider_retries <= 0: return - flight_recorder.record_recovery(session_id, "provider-retry", model, turn.provider_retries, sessions) + flight_recorder.record_recovery( + session_id, "provider-retry", model, turn.provider_retries, sessions, + detail={"retry_kinds": dict(Counter(turn.provider_retry_kinds))}, + ) diff --git a/backend/apps/agents/manager/streaming/state.py b/backend/apps/agents/manager/streaming/state.py index 3b220f17..99e082fa 100644 --- a/backend/apps/agents/manager/streaming/state.py +++ b/backend/apps/agents/manager/streaming/state.py @@ -66,6 +66,7 @@ class TurnState(BaseModel): # Provider 500s/429s the CLI retried on its own; the user sees only a long silence, so these are counted rather than lost. provider_retries: int = 0 provider_retry_wait_ms: int = 0 + provider_retry_kinds: List[str] = [] # Mid-turn context breaker: fires once per turn, and only after a below-trigger reading (a turn that STARTS over the trigger must run, or a failed shrink would break-loop forever). context_break_fired: bool = False saw_input_below_trigger: bool = False diff --git a/backend/tests/test_provider_retry_ledger.py b/backend/tests/test_provider_retry_ledger.py index a62e5014..8e53e617 100644 --- a/backend/tests/test_provider_retry_ledger.py +++ b/backend/tests/test_provider_retry_ledger.py @@ -93,3 +93,45 @@ def test_the_turn_loop_actually_dispatches_api_retry(): assert 'p_subtype == "api_retry"' in src, "the SystemMessage branch must recognise the retry subtype" assert "note_provider_retry(session_id, raw, turn)" in src assert "settle_provider_retries(session_id, turn, resolved_model, self.sessions)" in src + + +def test_the_kind_names_what_the_cli_waits_on(): + """Fleet, 7 days to 2026-09-07: 121 of 245 retries carried no status (our router down or restarting), 98 the + router's own 502 for an upstream it could not reach, 13 a 429. "Provider busy" covered all three.""" + from backend.apps.agents.manager.streaming.note_provider_retry import retry_kind + assert retry_kind(None, "unknown") == "unreachable" + assert retry_kind(502, "server_error") == "provider_error" + assert retry_kind(503, "server_error") == "provider_error" + assert retry_kind(429, "rate_limit") == "rate_limit" + assert retry_kind(401, "authentication_failed") == "auth" + assert retry_kind(400, "invalid_request") == "other" + + +def test_the_kind_rides_the_crumb_the_wire_and_the_recovered_envelope(monkeypatch): + import asyncio + from backend.apps.agents.core import ws_manager as wsm + sid = "provretry05" + flight_recorder.drop_session(sid) + sent_ws: List[Dict[str, Any]] = [] + + async def fake_send(session_id: str, event: str, payload: Dict[str, Any]) -> None: + sent_ws.append({"event": event, **payload}) + + monkeypatch.setattr(wsm.ws_manager, "send_to_session", fake_send) + sent: List[Dict[str, Any]] = [] + import backend.apps.service.client as service_client + monkeypatch.setattr(service_client, "submit_diagnostic", lambda payload: sent.append(payload)) + + async def run() -> None: + turn = TurnState() + note_provider_retry(sid, LIVE_RETRY, turn) + note_provider_retry(sid, {"subtype": "api_retry", "data": {"attempt": 2, "retry_delay_ms": 29000, "error_status": None, "error": "unknown"}}, turn) + await asyncio.sleep(0) + settle_provider_retries(sid, turn, "gpt-5.6", {}) + + asyncio.run(run()) + crumbs = [c for c in flight_recorder.breadcrumbs(sid) if c.get("l") == "provider-retry"] + assert [c["kind"] for c in crumbs] == ["provider_error", "unreachable"] + assert [w["kind"] for w in sent_ws if w["event"] == "agent:provider_retrying"] == ["provider_error", "unreachable"] + assert sent[0]["retry_kinds"] == {"provider_error": 1, "unreachable": 1}, "the recovered envelope finally says what was retried" + flight_recorder.drop_session(sid) diff --git a/frontend/src/app/pages/AgentChat/shell/RateLimitPill.tsx b/frontend/src/app/pages/AgentChat/shell/RateLimitPill.tsx index 0e6a13ca..af488f8f 100644 --- a/frontend/src/app/pages/AgentChat/shell/RateLimitPill.tsx +++ b/frontend/src/app/pages/AgentChat/shell/RateLimitPill.tsx @@ -7,6 +7,7 @@ import AutorenewIcon from '@mui/icons-material/Autorenew'; import { useAppDispatch, useAppSelector } from '@/shared/hooks'; import { clearProviderRetrying, clearRateLimited, clearReconnectWait } from '@/shared/state/agentsSlice'; import { useClaudeTokens } from '@/shared/styles/ThemeContext'; +import { providerRetryHint, providerRetryLabel } from '@/shared/providerRetryLabel'; /** Mid-turn CLI backoff pill (ENG-178): the provider 500/429'd and the CLI is silently waiting up * to tens of seconds; without this the card just sits dead. Auto-clears after the announced delay @@ -23,14 +24,18 @@ export const ProviderRetryPill: React.FC<{ sessionId: string }> = ({ sessionId } return () => clearTimeout(t); }, [pr, sessionId, dispatch]); - const label = pr?.attempt ? `Provider busy, retrying (attempt ${pr.attempt})` : 'Provider busy, retrying'; + const label = providerRetryLabel(pr?.kind, pr?.attempt); const lastLabel = useRef(label); - if (pr) lastLabel.current = label; + const lastHint = useRef(providerRetryHint(pr?.kind)); + if (pr) { + lastLabel.current = label; + lastHint.current = providerRetryHint(pr.kind); + } return ( { + assert.equal(providerRetryLabel('unreachable', 3), "Can't reach the model, retrying (attempt 3)"); + assert.equal(providerRetryLabel('provider_error', 1), 'Provider error, retrying (attempt 1)'); + assert.equal(providerRetryLabel('rate_limit', null), 'Rate limited, retrying'); + assert.equal(providerRetryLabel('auth', 2), 'Login rejected, retrying (attempt 2)'); +}); + +test('an older backend that sends no kind still reads as before', () => { + assert.equal(providerRetryLabel(null, 4), 'Provider busy, retrying (attempt 4)'); + assert.equal(providerRetryLabel(undefined, null), 'Provider busy, retrying'); +}); + +test('the hover says whose fault it is', () => { + assert.match(providerRetryHint('unreachable'), /router on this machine/); + assert.match(providerRetryHint('auth'), /reconnect/); + assert.match(providerRetryHint(null), /hiccup/); +}); diff --git a/frontend/src/shared/providerRetryLabel.ts b/frontend/src/shared/providerRetryLabel.ts new file mode 100644 index 00000000..7d6ee144 --- /dev/null +++ b/frontend/src/shared/providerRetryLabel.ts @@ -0,0 +1,23 @@ +/** What the pill says while the CLI retries. One word ("busy") used to cover a router that was not + * answering at all, which is half of the fleet's retries and nothing the provider did. */ +export function providerRetryLabel(kind: string | null | undefined, attempt: number | null | undefined): string { + const head = (() => { + switch (kind) { + case 'unreachable': return "Can't reach the model, retrying"; + case 'rate_limit': return 'Rate limited, retrying'; + case 'auth': return 'Login rejected, retrying'; + case 'provider_error': return 'Provider error, retrying'; + default: return 'Provider busy, retrying'; + } + })(); + return attempt ? `${head} (attempt ${attempt})` : head; +} + +export function providerRetryHint(kind: string | null | undefined): string { + switch (kind) { + case 'unreachable': return 'The request got no answer at all: the model router on this machine is restarting or the network dropped. The agent keeps trying on its own.'; + case 'rate_limit': return 'The provider asked us to slow down; the agent waits it out and continues on its own.'; + case 'auth': return 'The provider rejected the login on this attempt; if it keeps happening, reconnect the model in Settings.'; + default: return 'The AI provider had a hiccup; the agent is waiting it out and will continue on its own'; + } +} diff --git a/frontend/src/shared/state/agentsSlice.ts b/frontend/src/shared/state/agentsSlice.ts index c3e8b6e7..e2b313f8 100644 --- a/frontend/src/shared/state/agentsSlice.ts +++ b/frontend/src/shared/state/agentsSlice.ts @@ -124,7 +124,7 @@ export interface AgentSession { rate_limited?: { retry_after_s: number | null; at: string } | null; // Parked waiting for the connection back; unlike the pills above this can last minutes, so the UI has to say so. reconnect_wait?: { retry_in_s: number | null; attempt: number | null; at: string } | null; - provider_retrying?: { attempt: number | null; delay_ms: number | null; at: string } | null; + provider_retrying?: { attempt: number | null; delay_ms: number | null; kind: string | null; at: string } | null; // Set when a view-builder turn installed/changed deps, so the app card does a HARD reload (Vite restart) at turn-finish instead of the soft one. Reset when the next turn starts. app_deps_changed?: boolean; mcp_suggestions?: Array<{ id: string; title: string; description: string; reason?: string }>; @@ -1062,13 +1062,14 @@ const agentsSlice = createSlice({ setProviderRetrying( state, - action: PayloadAction<{ sessionId: string; attempt: number | null; delayMs: number | null }> + action: PayloadAction<{ sessionId: string; attempt: number | null; delayMs: number | null; kind: string | null }> ) { const session = state.sessions[action.payload.sessionId]; if (session) { session.provider_retrying = { attempt: action.payload.attempt, delay_ms: action.payload.delayMs, + kind: action.payload.kind, at: new Date().toISOString(), }; } diff --git a/frontend/src/shared/statusLabel.test.ts b/frontend/src/shared/statusLabel.test.ts index 28f0eaed..e4ffff23 100644 --- a/frontend/src/shared/statusLabel.test.ts +++ b/frontend/src/shared/statusLabel.test.ts @@ -27,3 +27,9 @@ test('stale pill state on a finished session never overrides its real status', ( assert.equal(cardStatusWord({ status: 'completed', reconnect_wait: { at }, queued: true }), 'done'); assert.equal(cardStatusWord({ status: 'error', rate_limited: { at } }), 'needs attention'); }); + +test('a router that is not answering is not the provider being busy', () => { + assert.equal(cardStatusWord({ status: 'running', provider_retrying: { at, kind: 'unreachable' } }), 'no answer from the model'); + assert.equal(cardStatusWord({ status: 'running', provider_retrying: { at, kind: 'provider_error' } }), 'provider busy'); + assert.equal(cardStatusWord({ status: 'running', provider_retrying: { at } }), 'provider busy'); +}); diff --git a/frontend/src/shared/statusLabel.ts b/frontend/src/shared/statusLabel.ts index 80c5050a..09c52d8f 100644 --- a/frontend/src/shared/statusLabel.ts +++ b/frontend/src/shared/statusLabel.ts @@ -14,7 +14,7 @@ interface CardStatusSource { queued?: boolean; reconnect_wait?: { at: string } | null; rate_limited?: { at: string } | null; - provider_retrying?: { at: string } | null; + provider_retrying?: { at: string; kind?: string | null } | null; } /** The collapsed card's one status word. A running turn that is really waiting on something says what, so "working" never covers a lost connection or a throttle the expanded chat's pills would show. */ @@ -22,7 +22,7 @@ export function cardStatusWord(s: CardStatusSource): string { if (s.status === 'running') { if (s.reconnect_wait) return 'waiting for connection'; if (s.rate_limited) return 'rate limited'; - if (s.provider_retrying) return 'provider busy'; + if (s.provider_retrying) return s.provider_retrying.kind === 'unreachable' ? 'no answer from the model' : 'provider busy'; // "queued" already means an unsent message in the composer chip; the admission gate gets its own words. if (s.queued) return 'waiting to start'; } diff --git a/frontend/src/shared/ws/WebSocketManager.ts b/frontend/src/shared/ws/WebSocketManager.ts index c46408e3..0369ebe2 100644 --- a/frontend/src/shared/ws/WebSocketManager.ts +++ b/frontend/src/shared/ws/WebSocketManager.ts @@ -790,6 +790,7 @@ class WebSocketManager { sessionId: session_id, attempt: typeof data.attempt === 'number' ? data.attempt : null, delayMs: typeof data.delay_ms === 'number' ? data.delay_ms : null, + kind: typeof data.kind === 'string' ? data.kind : null, })); } break;