fix(compact): try one alternate account on a pool 429/402, and keep the backoff headers - #927
Conversation
…he backoff headers Closes #913. `/v1/responses` already answers a pre-stream 429/402 by trying one eligible alternate account inside the same logical request (`retryCodexPoolOnAlternateAccount()`, core.ts:319-423). Compact did not: it resolved one context, sent once, and returned the rejection. The client then retried the compact task OUTSIDE the logical request, which is how a session reports exhausted retries while another pool account sits idle. Three things had to change together. The recorder took its account from a closure, so it could not express "record this against A and promote B", let alone record anything against B. It now takes the context explicitly; every existing call site passes the same `authCtx` it used before. The two accounts need different recovery. Compact's send goes through `fetchWithTransientRetry()`, which makes up to three status attempts each wrapping its own reset retries. "Send the alternate exactly once" and "keep A's existing recovery" only coexist if the modes differ — so A keeps the full ladder and B gets a single direct send, exactly as the regular path does at core.ts:396. The asymmetry is deliberate there too: A's retries happen before any alternate is considered, and the alternate is a last bounded try rather than a second ladder. `bufferCompactResponse()` rebuilt the response with only Content-Type, dropping `Retry-After` and the reset hints from the very rejection a client needs them for. It now carries a narrow allowlist and the upstream statusText. The alternate is built completely before A's body is cancelled, so a failure during construction still leaves A's rejection returnable. Six regressions, with the send count as the activation proof throughout — one send means the branch never fired, three means it recursed: 429 and 402 each try exactly one alternate; no eligible alternate returns A's rejection with its headers intact after exactly one send; a rejecting alternate produces two sends and returns the second rejection; a 400 does not trigger an alternate; an abort between attempts prevents the second send.
|
Warning Review limit reached
Next review available in: 1 minute Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughChangesCodex compact failover
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant CompactHandler
participant PrimaryAccount
participant AlternateAccount
participant PoolHealth
Client->>CompactHandler: Send compact request
CompactHandler->>PrimaryAccount: Attempt compact request
PrimaryAccount-->>CompactHandler: 429 or 402
CompactHandler->>PoolHealth: Record primary rejection
CompactHandler->>AlternateAccount: Send one alternate attempt
AlternateAccount-->>CompactHandler: Response
CompactHandler->>PoolHealth: Record alternate outcome
CompactHandler-->>Client: Return buffered response
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
…ly fires The suite asserted send counts for the 429/402 path, the no-alternate path, the both-reject path, and the abort path — but nothing drove the alternate into a transient 5xx, which is the only case that distinguishes `recovery: "single"` from `recovery: "normal"`. A wiring mistake that left the alternate on the transient ladder would have passed every existing test. Now it does not. Ablated by flipping the alternate back to "normal": the send count goes from 2 to 6 (one for A, three for B, across both rejection codes) and the test fails.
|
Added one test ( The existing matrix asserts send counts for the 429/402 success path, the no-alternate path, the both-reject path, and the abort path — all good — but none of them drives the alternate into a transient 5xx, and that is the only case where Ablation: flipping the alternate's mode back to Everything else here matches the plan in |
…e abort race Two review blockers on the alternate-account attempt. A's quota cache went stale. The regular path applies the rejected account's upstream quota headers before recording it (core.ts:349-357) — a 429 carries the snapshot that produced it, so skipping that leaves quota-strategy routing and the dashboard reading numbers from before the account ran out. Compact now does the same thing in the same order. Cancellation could race alternate resolution. The only abort check sat before the await, and resolution can wait on a credential refresh. A client that went away during that window still got A recorded, A's body cancelled, and B's fetch invoked. Native fetch rejects an aborted signal, but a custom executor need not, and B's quota is not ours to spend on a request nobody is waiting for. Re-checked after resolution, releasing B's probe lease on the way out.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/server/responses/compact.ts`:
- Around line 332-446: Add a regression test in
responses-compaction-routing.test.ts for a bound thread at 100% local quota
where the upstream responds successfully; assert the request sends exactly once
through the expected affinity account and does not resolve or attempt an
alternate account. Keep the retry condition in the compact response flow,
including sendCompactAttempt handling around the existing status check,
restricted to upstream status 429 or 402.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: a2706b09-595a-42f9-a07d-007ec744c557
📒 Files selected for processing (2)
src/server/responses/compact.tstests/responses-compaction-routing.test.ts
…al quota CodeRabbit asked for the case the plan called the scope guard: a bound thread whose affined account reads 100% locally, with the upstream answering normally. Nothing in the file covered it. It matters because a cached 100% is what the account looked like on its last WHAM read, not a rejection — the alternate attempt has to trigger on an actual upstream 429/402. If the gate ever widened to consult quota, this request would resolve an alternate and send twice, and every other test in the file would still pass. One send, 200 returned, no rotation.
|
Added CodeRabbit's requested case ( It is the scope guard the plan named, and the reason it earns its place is that no existing test would catch its absence. A cached 100% is what the account looked like on its last WHAM read, not a rejection — if the alternate gate ever widened to consult local quota, this request would send twice and every other test in the file would still pass. Verified against the existing quota-strategy behavior rather than assumed: the plan explicitly rules out changing the rebinding at
|
1 participant
Closes #913. Reported by @luvs01.
/v1/responsesalready answers a pre-stream 429/402 by trying one eligible alternate account inside the same logical request —retryCodexPoolOnAlternateAccount()atsrc/server/responses/core.ts:319-423, activated before streaming at:1679-1721, recognizing exactly 429 and 402./v1/responses/compacthad none of it. It resolved one context, sent once, and returned the rejection. The client then retried the compact task outside the logical request, which is how a session ends up reporting exhausted retries while another pool account sat idle.Three things had to change together
The recorder took its account from a closure.
recordCompactPoolOutcome()captured a singleauthCtx, so it could not express "record this against A and promote B", and could not record anything against B at all. It now takes the context explicitly; every existing call site passes the sameauthCtxit used before, so that part is a mechanical widening — but it is a prerequisite, not a detail.The two accounts need different recovery, and this is the part worth reviewing. Compact's send goes through
fetchWithTransientRetry(), which makes up to three status attempts, each wrapping its own reset retries. So "send the alternate exactly once" and "preserve A's existing recovery" contradict each other unless the modes differ. A keeps the full ladder; B gets a single directfetchWithHeaderTimeout(). That is exactly what the regular path does atcore.ts:396, and the asymmetry is deliberate there too — A's retries happen before any alternate is considered, and the alternate is a last bounded try rather than a second retry ladder.bufferCompactResponse()dropped the backoff headers. It rebuilt the response with onlyContent-Type, discardingRetry-Afterand the reset hints from the very rejection a client needs them for. It now carries a narrow allowlist plus the upstreamstatusText.Ordering detail: the alternate is built completely before A's body is cancelled. If construction throws, A's rejection is still intact and returnable.
Not in scope
The quota strategy deliberately rebinds an over-threshold thread (
src/codex/routing.ts:1179-1200), and tests cover that. The issue's framing suggests a 100% WHAM snapshot should never move an existing thread under any strategy — that is a policy change with its own tradeoffs, not part of endpoint parity, so it is not smuggled in here.Evidence
Six regressions, with the send count as the activation proof throughout — one send means the branch never fired, three means it recursed:
Retry-Afterand reset headers intactAblation: reverting
src/server/responses/compact.tstodevwhile keeping the tests fails exactly 4 of the 6 — the two negative controls still pass, which is what makes them controls.bun run test: 7581 pass / 0 fail / 8 skip across 504 filesbun x tsc --noEmitexit 0bun run privacy:scanpassedSummary by CodeRabbit
Review round
An adversarial review found three blockers, all fixed in
361b329a3and20f4f6dd0.A's quota cache went stale. The regular path applies the rejected account's upstream quota headers before recording it (
core.ts:349-357) — a 429 carries the snapshot that produced it. Compact skipped that, so quota-strategy routing and the dashboard kept reading numbers from before the account ran out. Now done in the same order.Cancellation could race alternate resolution. The only abort check sat before the await, and resolution can wait on a credential refresh. A client that went away during that window still got A recorded, A's body cancelled, and B's fetch invoked. Native fetch rejects an aborted signal, but a custom executor need not, and B's quota is not ours to spend on a request nobody is waiting for. Re-checked after resolution, releasing B's probe lease on the way out.
The tests did not lock the crux they were written for. This is the one worth naming: all six original tests could pass with B switched from
"single"back to"normal"recovery, because B always returned 200 and never stimulated the retry ladder. Three more tests now cover it — B returning a transient 5xx must still be one send, A returning a transient 5xx must still be retried in place, and each account's health must record its own outcome.Verified by mutation: forcing B onto the normal ladder fails exactly those three single-send assertions and nothing else.
Updated totals: 9 regressions,
bun run test7586 pass / 0 fail across 504 files, typecheck exit 0, privacy scan passed.