Skip to content

fix(api): retry 429/5xx with backoff, and stop replaying streamed responses - #20

Merged
ForkedInTime merged 2 commits into
mainfrom
fix/api-retry-backoff
Aug 4, 2026
Merged

fix(api): retry 429/5xx with backoff, and stop replaying streamed responses#20
ForkedInTime merged 2 commits into
mainfrom
fix/api-retry-backoff

Conversation

@ForkedInTime

Copy link
Copy Markdown
Owner

Closes cross-cutting item X.6 (429 retry/backoff) and a latent bug found while doing it (X.7).

The gap

There was no retry anywhere on the request path. 529 was handled — it falls back to a secondary model once — but 429 was not, and retry-after was ignored entirely. A rate-limited user just got an error and lost the turn. Under enterprise load that is the failure mode you are guaranteed to hit.

What landed

New src/api/retry.rs:

  • Retries 408/429/500/502/503/504 and connect/timeout transport errors.
  • Honours retry-after verbatim (numeric seconds, plus the retry-after-ms extension some OpenAI-compat providers send). No jitter, no exponential growth — the server named the time; second-guessing it just earns another 429.
  • Exponential backoff with equal jitter otherwise. Full jitter can retry almost immediately, which defeats the point under a rate limit; no jitter synchronises every client.
  • Bounded three ways: 5 attempts, 150s total, and a 120s cap on retry-after above which it fails fast with a clear message rather than freezing the UI for minutes.
  • Every backoff is reported to the user. A silent 30s sleep reads as a hang.

Wired into all three send sites: Anthropic streaming, Anthropic non-streaming, and OpenAI-compat (where Groq and OpenRouter throttle hardest).

Two deliberate scope limits, both documented in the module:

  • 529 is still not retried here. It belongs to the model-switch fast path — switching to a cheaper model immediately beats sleeping 30s first.
  • send_with_retry returns the final response even on an error status, so the OpenAI-compat "does not support tools" 400 sniff and the 529 detection keep working byte-for-byte.

The bug this uncovered

The TUI already had its own retry loop, which I had not seen when I scoped this. It re-issued messages_stream on any error matching reset by peer / connection / timed out.

But AppEvent::TextChunk does self.streaming.push_str(&chunk) and that buffer is never rewound. So a mid-stream connection reset replayed the whole response: the user saw a truncated answer followed by a complete one, and the doubled text was persisted into the session.

Fixed with a streamed_any guard, extracted as retry::may_retry_stream so it is reachable from tests — the streaming loop itself is binary-only. 429/5xx were also removed from that loop now that the client owns them; keeping both meant 3 x 5 = 15 attempts that ignored retry-after.

Testing — 25 new tests

Split so each layer proves something the other cannot:

  • Pure policy: attempt/time/retry-after caps, jitter bounds and clamping, header parsing. Includes an HTTP-date retry-after being ignored rather than parsed as zero — that would turn a polite backoff into a hot retry loop.
  • Scripted HTTP server (hand-rolled on TcpListener, no new dependency): a 429 is retried then succeeds, a 400 is not retried, a permanent 429 stops at the cap, every retry notifies, and giving up explains why.
  • Wiring, per phase-1's lesson that a passing test is not proof the wiring is exercised: one test per backend proving messages_stream / messages / the OpenAI-compat client actually reach the retry path. Each fails if that call is swapped back to a plain send().
  • Cancellation: Esc aborts the task, and up to 32s of backoff now sits inside it. A test asserts a cancelled turn does not fire its queued retry — verified with a negative control (removing the abort makes it fail).
  • retry-after on the wire: the other server tests use retry-after: 0 and would still pass if the header were ignored, so one test uses a 1s delay that is distinguishable from the 250-500ms exponential step.

Every fix was verified by reintroducing the bug and confirming the test fails — the method used throughout this audit. All six reintroductions were caught; removing the attempt cap hung until timeout, which is the intended proof.

The #[cfg(test)] base-URL override exists because a runtime override is a credential-exfiltration vector — the same reason ANTHROPIC_BASE_URL is excluded from the .env allowlist. It is not compiled into release builds.

QA

  • 661 tests, 0 failures (was 611)
  • Clippy clean under the exact CI command (--all-targets --all-features -D warnings)
  • Release builds, 19.17 MB

ForkedInTime and others added 2 commits August 4, 2026 09:56
…ponses

There was no retry anywhere on the request path. A single 429 failed the
whole turn — the one failure mode guaranteed to happen under load. 529 was
handled (fallback model) but 429 was not, and `retry-after` was ignored.

New `api/retry.rs`:
  - retries 408/429/500/502/503/504 and connect/timeout transport errors
  - honours `retry-after` (numeric seconds, plus the `retry-after-ms`
    extension) verbatim — no jitter, no growth; the server named the time
  - exponential backoff with equal jitter otherwise
  - bounded three ways: 5 attempts, 150s total, and a 120s cap on
    `retry-after` above which it fails fast rather than freezing the UI
  - reports every backoff to the user; a silent 30s sleep reads as a hang

Wired into all three send sites (Anthropic streaming + non-streaming, and
OpenAI-compat, where Groq/OpenRouter throttle hardest). 529 is deliberately
still excluded: it is owned by the model-switch fast path, which beats
sleeping. `send_with_retry` returns the final response even on an error
status, so the OpenAI-compat "does not support tools" 400 sniff and the 529
detection keep working unchanged.

Also fixes a latent bug found in the TUI's own retry loop. It re-issued
`messages_stream` on any error matching `reset by peer`/`connection`, but
`AppEvent::TextChunk` appends to `app.streaming` and is never rewound — so a
*mid-stream* failure replayed the entire response and the user saw a
truncated answer followed by a complete one, with the doubled text persisted
to the session. Now guarded by `may_retry_stream`, which refuses to retry
once any output has been streamed. 429/5xx were also removed from that loop
since the client now owns them; keeping both meant 3 x 5 = 15 attempts that
ignored `retry-after`.

The base URL override used by the tests is `#[cfg(test)]` on purpose: a
runtime override is a credential-exfiltration vector, which is why
ANTHROPIC_BASE_URL is excluded from the .env allowlist.

25 tests: pure policy (caps, jitter bounds, header parsing, HTTP-date
ignored rather than read as zero) plus scripted-server tests proving each
backend actually reaches the retry path, that a cancelled turn does not fire
its queued retry, and that `retry-after` is obeyed on the wire.

Co-Authored-By: Arch Linux <noreply@archlinux.org>
The cancellation test asserted the first request had landed after a flat
250ms, which a loaded CI runner can miss. Polling keeps the assertion but
removes the timing assumption; the negative control (dropping the abort)
still fails it.

Co-Authored-By: Arch Linux <noreply@archlinux.org>
@ForkedInTime
ForkedInTime merged commit 7ea3d2b into main Aug 4, 2026
5 checks passed
@ForkedInTime
ForkedInTime deleted the fix/api-retry-backoff branch August 4, 2026 17:07
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