Skip to content

feat(retry): transient transport failures back off and retry - #171

Merged
tobert merged 3 commits into
mainfrom
rate-limits
Sep 1, 2026
Merged

feat(retry): transient transport failures back off and retry#171
tobert merged 3 commits into
mainfrom
rate-limits

Conversation

@tobert

@tobert tobert commented Aug 31, 2026

Copy link
Copy Markdown
Owner

Transient provider failures — rate limits and overloads — now back off and retry at the Retried seam instead of killing the whole call. M1 of the plan in docs/rate-limits.md, which rides this PR.

The incident behind it

2026-08-27: two deliberate calls on gpt-deliberate died in the DOSSIER phase when the explorer hit a 429 TPM limit on three attached files. kaibo neither waited nor asked again, and each whole investigation was lost — the failure mode this fixes is a many-turn phase forfeited to one transient refusal.

Why this lives in kaibo and not rig

Verified against the vendored rig 0.41 sources before building: rig-core's RetryPolicy/ExponentialBackoff is wired only into the SSE event source (streaming reconnects — kaibo's loop is non-streaming); rig-agent's ModelTurnAction::Retry re-rolls completed turns and never sees a transport error, which exits the run as Err. The one thing rig does hand back is provider_response_status() — and its error path drops response headers (non_success_status_error keeps status + body), so Retry-After is unrecoverable on this path. That is why the delay hint is body-parsed and best-effort, with computed backoff as the floor.

The design

  • One module owns "kaibo sends a provider request twice." Retried<M> already retried malformed generations at the CompletionModel seam, underneath rig's loop where the transcript still exists; the transient class extends it rather than adding a sibling wrapper. Two independent counters — a turn that fumbles once and then gets rate-limited spends both allowances.
  • Classification is status-primary: 429, 500, 502, 503, 529 via provider_response_status(), matched as specific codes (never !is_success()). Conservative phrase markers (overloaded_error, rate limit, too many requests) fire only when no status survived; a bare "429" substring deliberately cannot.
  • Full jitter, because kaibo's own bursts (rig runs a turn's tool calls with buffer_unordered) would otherwise re-collide on a synchronized clock. Floor 1s, factor 2, per-wait cap 60s, 4 further attempts — constants until evidence says a deployment needs config.
  • Timeouts deliberately excluded: a timed-out request may be a request that is too large, and retrying it doubles the spend to fail twice. Added only on evidence, the same rule the malformed-marker neighbors follow.

Validation

Failing-first: the two behavioral tests were run against the pre-M1 loop and failed for the right reason (a 429 passed straight through). 11 module tests including paused-clock timing bounds; full suite green post-rebase; clippy clean; rand/http (dev-only) were already transitive — Cargo.lock gained zero new crates; cargo tree -i aws-lc-rs / -i mimalloc both empty.

M2 (a governor-based per-backend requests-per-minute limiter, opt-in) and M3 (Retry-After on kaibo-owned reqwest surfaces, deferred) are scoped in the plan doc.

A cross-family kaibo review follows as a PR comment.

Co-authored-by: Claude Sonnet 5 (implementation), reviewed in-session by Claude Fable 5.

🤖 Generated with Claude Code

tobert and others added 2 commits August 31, 2026 11:19
The 2026-08-27 gpt-deliberate failure (explorer 429 TPM, no backoff, whole
investigation lost) is the motivating case. Verified against vendored rig 0.41
that nothing upstream covers the non-streaming path: RetryPolicy is SSE-only,
agent hooks never see transport errors, and Retry-After is dropped before the
error reaches us. The existing Retried<M> wrapper (completion_retry.rs) is the
seam; M1 adds a transient-transport class with backoff there, M2 adds a
governor-based per-backend requests-per-minute limiter, M3 (Retry-After on
owned reqwest surfaces) is recorded and deferred.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…etried seam

The 2026-08-27 failure is the motivating case: a gpt-deliberate explorer hit a
429 TPM limit and the whole investigation died — kaibo neither waited nor asked
again. The Retried wrapper already owned 'kaibo sends a provider request twice'
for malformed generations, so the transient class lives there rather than in a
sibling: statuses 429/500/502/503/529 via provider_response_status(), with
conservative body markers only when no status survived. Exponential backoff
with full jitter (rig's buffer_unordered fan-out means synchronized retries
would re-collide), a body-parsed delay hint as a floor (rig drops the
Retry-After header before the error reaches us), and independent bounds per
failure class. Timeouts deliberately excluded: a timed-out request may be a
request that is too large, and retrying it doubles the spend to fail twice.

Plan: docs/rate-limits.md (M1). Failing-first: the two behavioral tests were
run against the pre-M1 loop and failed for the right reason before the
implementation landed. 1237 tests green, clippy clean, aws-lc-rs and mimalloc
trees still empty.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@tobert

tobert commented Aug 31, 2026

Copy link
Copy Markdown
Owner Author

Cross-family review

kaibo cast crusoe (explorer deepseek-ai/Deepseek-V4-Flash, synth zai/GLM-5.2), five review areas, citations verified before posting. One blocking finding, now fixed on the branch (c9329c3); everything else correct.

The blocking finding — a panic on a huge delay hint. transient_wait converted the body-parsed hint with Duration::from_secs_f64(...) and applied .min(60s) to the result — but a finite-yet-huge hint (twenty digits of seconds parses to ~1e20, past Duration's ~1.8e19-second ceiling) panics inside the conversion before the cap can run. A hostile or buggy 429 body saying "try again in 99999999999999999999s" would have killed the consult phase with an opaque panic instead of waiting 60s. Reproduced failing-first (the new test panicked exactly as predicted against the old code); fixed by capping the number before the conversion.

The rest, confirmed correct:

  1. Retry loop bounds. Two independent counters; maximum requests = 1 + 2 (malformed) + 4 (transient) = 7; a non-retryable error returns after one attempt; the final error is the provider's own unwrapped words. When an error matches both classes, malformed (the more specific diagnosis) is checked first.
  2. Classification is conservative. Status-primary with specific codes; the 2xx-with-error-envelope trap is handled (a 200 carrying "overloaded_error" in the body is NOT retried — the status branch is authoritative, and there's a test pinning it). Marker fallback fires only with no status, phrases only — "request id 4293001" cannot trigger it. Noted, not a bug: rate_limit (underscore) isn't a marker; in practice a real 429 carries its status, and the retry vocabulary is deliberately narrower than classify_failure's advice vocabulary.
  3. Paused-clock tests. The delay-hint test deterministically distinguishes hint-honored from hint-ignored (a jittered sub-1s backoff cannot reach the 1s floor the assertion demands; the 2s hint can). The bound test alone wouldn't catch a zero-wait regression, but the hint test covers that behavior — noted as acceptable overlap.
  4. tracing::warn content. %error carries the provider's response body (org id, limit details) to the operator's logs — no API keys (response bodies never carry them), operator surface only, and the same text reaches the caller on final failure anyway.
  5. Boundaries. stream deliberately forwards without retry (kaibo is non-streaming); composes_native_output_with_tools forwards verbatim; wrap order watched(retried(model)) puts recovery below observation, both below rig's loop where the transcript survives.

Reviewed-by: kaibo cast crusoe (DeepSeek-V4-Flash explorer, GLM-5.2 synth)

The cross-family review's one blocking finding, reproduced failing-first: a
finite-but-huge hint (twenty digits of seconds) passes the finiteness guard,
and Duration::from_secs_f64 panics on overflow before the .min(cap) on the
converted value can run. The cap now applies to the number, then the
conversion — a hostile or buggy 429 body waits 60s instead of killing the
phase with a panic.

Reviewed-by: kaibo cast crusoe (DeepSeek-V4-Flash explorer, GLM-5.2 synth)
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@tobert
tobert merged commit f19d3f3 into main Sep 1, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant