fix(llm): back off + retry mid-stream provider failures; typed transient errors (ENG-673)

[alecantu7]

Part 1 of 3 for ENG-673 (cowork-server + cowork companions to follow — this is the upstream that raises the typed error). Fixes the anton-side of BUG-CM-001.

Problem

A mid-stream provider overload arrives inside an HTTP-200 stream — the status was already sent, so the error is smuggled into the body as an SSE error event, and the SDK raises APIStatusError(status_code=200, body=…). anton's status-only classifier turned that into the nonsensical "Server returned 200 — the LLM endpoint may be temporarily unavailable", and the session loop retried it instantly with zero backoff — burning all attempts within seconds of a minutes-long incident (Anthropic "elevated errors", 2026-07-08). BYOK/direct users hit this; MindsHub-routed users are shielded by the router's failover.

What changed

• Typed errors + shared classifier (provider.py): new TransientProviderError / ProviderOverloadedError, and classify_transient() that reads the body, not the status — overloaded_error/api_error, 5xx, plain-429, connection drops, truncated streams.
• anthropic.py: the two byte-identical status-only blocks refactored into a shared _raise_for_status_error (mirrors the ENG-598 openai mapper). openai.py: that mapper extended with the transient branch (covers all four call paths).
• session.py: budget-bounded backoff-and-retry (30s/turn, cancellation-aware, jittered ~2/10/18s) for the mid-stream case; on exhaustion raise ProviderOverloadedError (carries model+provider) for the downstream provider_overloaded card. Completed tool_results are never re-executed on retry — only dangling tool_use is sealed.
• Scrubbed logging of the error body on every transient occurrence (via ENG-583 scrub_credentials).

Design decisions (aligned on the ticket)

• Classify by body, not status — the only way to see the truth once a 200 is on the wire.
• Split by prior-retry — the 30s budget is spent only on the mid-stream case that had no prior retry (overload in a 200). Request-time 5xx/429, connection errors, and truncated streams were already retried by the SDK (or would loop on a hard-bad endpoint), so they carry session_backoff=False: honest typed message, but fail fast instead of stacking another 30s. This is what keeps a persistently-broken endpoint from hanging 30s (and the e2e error tests fast).
• Idempotency — retry re-issues only the failed LLM call; history (with completed tool_results) is unchanged, so a tool that already published/sent can't fire twice.
• Per-turn 30s budget from the first transient error; cancellation aborts the backoff immediately.

Tests

tests/test_transient_retry.py (new, 30 cases): the classifier matrix, both provider mappers (incl. the mid-stream-200 → not "Server returned 200"), the session_backoff split, backoff cadence + retry_after + cancel-awareness, and turn-level behavior — recovers after N transient retries, exhausts the budget to ProviderOverloadedError, cancels on stop, and does not inject a recovery note (no-replay). Updated the two ENG-598 mapper tests (429/500 are now typed-transient) and the two e2e error-handling tests (honest message + fast fail).

Full suite green except the 2 pre-existing environmental test_chat_scratchpad failures (scratchpad subprocess can't spawn in the sandbox — documented in the ENG-655 PR, unrelated here).

Security pass (convention #8)

• Logged error body is run through scrub_credentials (ENG-583) before hitting logs — no mdb_/sk-/AIza keys leak via a traceback/body echo.
• No new external input is trusted; classification reads only structured status/body.error.type.
• ProviderOverloadedError carries only model+provider (no keys/PII).

Self-review notes (for the reviewer)

• Truncation detection (stop_reason is None after a clean stream iteration) is new and conservative — it raises transient with session_backoff=False (fail-fast), so a false-positive costs a quick retry, not a 30s loop. The _stream_via_responses path is intentionally not truncation-checked (its status semantics are less clear); flagging in case we want it later.
• What triggers the 30s budget is deliberately narrow — only the evidenced mid-stream-overload-in-a-200 case. If we later see real truncation/connection incidents that would benefit, flipping their session_backoff is a one-liner.
• Punted (tracked on the ticket): scheduled/channel auto-fallback, cross-provider switch suggestion, a config kill-switch for the budget.

Merge order

Land this before / with cowork-server #186 (or together). The core fix — surviving the incident via backoff-retry — works standalone. But on budget exhaustion the honest message only reaches the user once cowork-server maps the code: with an old cowork-server, ProviderOverloadedError is unrecognized and degrades to the generic "An unexpected error occurred" (still not misleading, and never the old "Server returned 200" — but NOT the curated provider text until #186 lands). Full sequence: anton (this) → cowork-server #186 (map provider_overloaded → code + extras) → cowork #388 (the card). Each degrades gracefully; no branch stacking.

🤖 Generated with Claude Code

[alecantu7]

Adversarial self-review of the 3-PR stack (anton #246 · cowork-server #186 · cowork #388)

Reviewed the three branches together, across the contract boundaries. Findings ranked; fixed ones addressed in 7f993e5 (this branch) / the PR-body edit.

🔴 #1 — Truncation heuristic regressed no-finish_reason providers. FIXED.
stop_reason is None → raise discarded a complete answer on any OpenAI-compatible endpoint that doesn't report a terminal marker (would fail every turn for such a provider). Now only the truly-empty stream (no content and no tool_calls) is treated as truncated; content-bearing streams log-and-pass-through.

🟡 #2 — "graceful degradation" claim overstated. FIXED (PR body).
Without cowork-server #186, ProviderOverloadedError is unrecognized → generic "An unexpected error occurred", not the curated text. Corrected the Merge-order note; land this before/with #186.

🟡 #3 — Cancel during backoff surfaced a provider-error card. FIXED.
User-stop mid-backoff re-raised the transient error; now breaks cleanly like a normal stop.

🟡 #4 — Card named the planning model even when the coding model failed. FIXED.
TransientProviderError now carries the in-flight model (threaded through classify_transient + both providers); ProviderOverloadedError uses it, falling back to planning.

🟢 Remaining (LOW — not fixed; flagging for reviewers, follow-up if wanted):

• Adding minds with direct query support on Minds in scratchpad #5 (cowork #388): Retry is disabled when the last user message was multimodal (prevUserText only captures string content). Minor UX gap on image turns.
• Remote Code Execution Back-ends #6 (this PR): request-time transients (session_backoff=False) fall into the count-based path, which injects a "you errored, adjust your approach" system note — misattributes a provider blip to the model. Pollutes context; harmless. Left as-is to keep this PR's blast radius tight.
• Bettermemory #7 (cross-PR): "provider_overloaded" is hand-duplicated across all 3 repos with no shared constant (existing ENG-598 pattern); and reconnectable is semantically overloaded (auth = "can reconnect" vs overload = "on managed"). Works; muddy.
• Improve Minds setup recovery flow #8 (cowork #388): App.jsx form-submission onError (line ~3021) threads no structured error fields — can't render any card. Latent inconsistency; provider_overloaded doesn't flow through that path.

Full suite green after the fixes (bar the 2 pre-existing environmental test_chat_scratchpad failures).

[alecantu7]

Update: #6 also fixed (185aebf). A request-time transient (5xx / rate-limit / dropped connection) no longer injects the "adjust your approach" recovery note — it gets a neutral "transient service issue, not a problem with your approach" note instead, so a provider blip doesn't misdirect the model's next attempt. Regression test added.

Remaining open (all LOW, follow-up candidates): #5 multimodal Retry disabled (cowork), #7 stringly-typed cross-repo code + overloaded reconnectable, #8 latent form-path onError gap (cowork). None are correctness bugs on the primary paths.

[SailingSF]

Review — ENG-673 (anton side)

Nicely scoped and the mechanics are sound: stop_reason is safely initialized in both providers before the new is None check (no NameError), the no-history-injection-on-transient-retry (idempotency) is correct and well-covered, the content_text or tool_calls passthrough on stop_reason is None correctly avoids discarding good answers from finish-reason-omitting endpoints, and the logged body is scrubbed before truncation. One design decision is worth pausing on before merge, plus a couple of smaller notes.

🔴 Main point — the session_backoff split (design + spec + simplicity)

The session_backoff: bool flag routes transient errors down two different paths — the new budget-bounded backoff-and-retry (True) vs. the existing count-based path with a new third "transient service issue" recovery-note branch (False). In the current diff only one case sets True: an overloaded_error/api_error arriving inside an HTTP-200 stream (classify_transient, session_backoff = status_code is None or status_code == 200). Connection drops, read timeouts, truncated streams, 5xx and 429 are all hardcoded False and fail fast without ever retrying.

Two concerns:

1. It diverges from the acceptance criteria. The ticket lists "mid-stream overloaded_error or a connection drop / read timeout / truncated stream → turn completes after backed-off retry". Only overloaded_error recovers here; the other three named cases surface an error immediately.

2. The stated rationale doesn't hold for the mid-stream cases. The comments justify False with "the SDK already retries connection errors at the transport layer." That's true for request-establishment failures — but not for mid-stream ones. Once a streaming 200 is received, the SDK's retry wrapper has already returned; a connection drop or a truncated stream during iteration is never SDK-retried. Those are exactly the cases the ticket calls out as "often more common in a real incident ... providers go silent rather than send a tidy error." Notably, the classify_transient docstring states the correct rule ("a mid-stream failure ... the SDK never retried it → the session must") — the truncation/connection paths just don't follow it.

   The clearest instance: truncated_stream is unambiguously mid-stream (we received a 200 and iterated to an empty end) and is never SDK-retried, yet it's marked session_backoff=False.

Simplicity angle: this flag is also the largest source of complexity in the diff — it creates two retry mechanisms plus a third recovery-note branch, and a TransientProviderError now behaves in three different ways depending on the flag. There's a real question of whether a single retry path (bounded by the 30s budget + cancellation, which already cap the downside) would be both simpler and closer to spec, versus keeping the split but moving the boundary so the mid-stream drop/truncation cases land on the retry path.

I don't want to prescribe the answer here — fast-failing genuinely-instant, repeating request-time errors has real merit, so this is a judgment call about where the boundary belongs. But as written, the split optimizes that boundary in a place that misclassifies the spec's priority cases, so I think it needs a deliberate decision (and, whichever way it goes, the ticket's acceptance criteria updated to match). Could you weigh in on the intended behavior for connection-drop / read-timeout / truncated-stream specifically?

🟡 Missing the mock-provider harness

The ticket requires a deterministic Anthropic-shaped and OpenAI-shaped mock ("the fix is not tested if only Anthropic is mocked"). None ships here, and test_transient_retry.py references a test_transient_retry_e2e file and a tests/fixtures/ harness that aren't in the diff. The unit coverage of both mappers is good, but there's no end-to-end test that a real SSE-error-inside-a-200 stream is classified and recovered. Either add the fixture or drop the dangling docstring reference.

🟡 Observability only partially delivered

Acceptance asks for scrubbed logging + metrics (transient-error / retry-success / budget-exhaustion rates) + Langfuse turn tagging. Only the scrubbed logger.warning is here. If metrics/tagging are a deliberate follow-up, worth noting them as out-of-scope on the ticket so the acceptance list stays honest.