Expire exhaustion marks at the upstream's own reset time#38
Conversation
Caught live during a capacity crunch: aziz@manaflow.ai's weekly window had reset (57% quota available) but its scheduler score stayed zeroed, because a markAccountExhausted mark only cleared on a SUCCESSFUL usage refresh and the loaded usage endpoint kept falling back to stale data. Failover then burned its 6 attempts on genuinely-cooked accounts ranked above the recovered one, and real users got 429s while real quota sat unroutable — the exact failed-reroute condition this service exists to prevent. MarkExhaustedUntil records an expiry per mark; Get() lazily prunes lapsed marks back to the optimistic default so routing retries the account and request-time truth decides. The expiry comes from the rejected response itself: anthropic-ratelimit-unified-reset (authoritative window reset epoch), else Retry-After, else a 10-minute default; clamped to [now+1m, now+8d]. A full usage refresh (Set/FinishRefresh) clears pending expiries so a prune can never delete refreshed scores. Tests: expired marks lapse while unexpired hold; refresh supersedes marks; expiry parsing (unified-reset, Retry-After, default, floor, cap); and the live regression end-to-end — failover reaches a recovered account after its mark lapses. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
📝 WalkthroughWalkthroughClaude account exhaustion marking now uses upstream-reported reset times instead of a fixed default TTL. SchedulerRef adds a time-bounded ChangesTime-bounded Claude exhaustion marking
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant ClaudeAPI
participant Proxy
participant SchedulerRef
participant Failover
ClaudeAPI-->>Proxy: response with rate-limit headers
Proxy->>Proxy: claudeExhaustionExpiry(headers)
Proxy->>SchedulerRef: markAccountExhaustedFromResponse(expiry)
SchedulerRef->>SchedulerRef: MarkExhaustedUntil(account, expiry)
Note over SchedulerRef: time passes, expiry lapses
Failover->>SchedulerRef: Get()
SchedulerRef->>SchedulerRef: pruneExpired(now)
SchedulerRef-->>Failover: account available again
Failover->>ClaudeAPI: retry request to recovered account
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Addresses autoreview P1: refreshes seed from the current scheduler and carry an account's zero score forward when its own usage fetch fails, so blanket-clearing exhaustedUntil on every refresh made request-time marks permanent again in the common mixed-refresh case. Now an expiry is dropped only when the incoming score actually supersedes the mark (shows headroom); a carried-forward zero score keeps its expiry. Keeping an expiry for a genuinely-cooked account is safe: on lapse routing tries it once and the upstream reject re-marks it. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
internal/proxy/claude_ratelimit_routing_test.go (1)
926-954: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd edge cases for fallback parsing.
Please extend this table with malformed
anthropic-ratelimit-unified-reset+ validRetry-After, and HTTP-dateRetry-After, so the expiry parser’s fallback contract stays covered.🤖 Prompt for 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. In `@internal/proxy/claude_ratelimit_routing_test.go` around lines 926 - 954, Extend TestClaudeExhaustionExpiry to cover the fallback behavior in claudeExhaustionExpiry: add a case where Anthropic-Ratelimit-Unified-Reset is malformed but Retry-After is valid, and verify the parser falls back to Retry-After; also add a case where Retry-After is an HTTP-date and confirm it is parsed correctly. Keep the coverage centered on claudeExhaustionExpiry’s header precedence and fallback parsing contract.
🤖 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 `@internal/proxy/proxy.go`:
- Around line 1688-1692: `markAccountExhaustedFromResponse` currently applies
`claudeExhaustionExpiry` for every provider because it ignores `provider`, which
can make the Codex retry path honor `Retry-After` instead of the default TTL.
Update the helper in `Server.markAccountExhaustedFromResponse` to branch on the
`accounts.Provider` value and only derive the expiry from Claude response
headers for Claude; for all other providers, keep using the scheduler’s default
exhaustion TTL. Make sure the caller path around the Codex retry logic continues
to route through this helper so the provider-specific behavior is centralized.
- Around line 1703-1710: The fallback logic in the header parsing block should
use Retry-After whenever anthropic-ratelimit-unified-reset is missing or
invalid, not just when it is absent. Update the parsing around claudeHeaderGet,
strconv.ParseInt, and strconv.Atoi so that a malformed reset value does not
short-circuit the Retry-After path; only keep until from the reset header when
parsing succeeds, otherwise continue to the Retry-After branch.
---
Nitpick comments:
In `@internal/proxy/claude_ratelimit_routing_test.go`:
- Around line 926-954: Extend TestClaudeExhaustionExpiry to cover the fallback
behavior in claudeExhaustionExpiry: add a case where
Anthropic-Ratelimit-Unified-Reset is malformed but Retry-After is valid, and
verify the parser falls back to Retry-After; also add a case where Retry-After
is an HTTP-date and confirm it is parsed correctly. Keep the coverage centered
on claudeExhaustionExpiry’s header precedence and fallback parsing contract.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 86e58d4c-8d8e-4bc6-9954-5051d693604e
📒 Files selected for processing (4)
internal/proxy/claude_ratelimit_routing_test.gointernal/proxy/proxy.gointernal/selectacct/scheduler_ref.gointernal/selectacct/scheduler_ref_test.go
| func (s Server) markAccountExhaustedFromResponse(provider accounts.Provider, accountID string, header http.Header) { | ||
| if s.SchedulerRef == nil { | ||
| return | ||
| } | ||
| s.SchedulerRef.MarkExhaustedUntil(provider, accountID, claudeExhaustionExpiry(header, time.Now())) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Keep non-Claude exhaustion marks on the default TTL.
markAccountExhaustedFromResponse ignores provider, so the Codex retry path at Line 2950 can honor a Retry-After header and diverge from the stated default-TTL behavior. Branch inside the helper and only use Claude header-derived expiry for Claude.
Proposed fix
func (s Server) markAccountExhaustedFromResponse(provider accounts.Provider, accountID string, header http.Header) {
if s.SchedulerRef == nil {
return
}
+ if provider != accounts.ProviderClaude {
+ s.SchedulerRef.MarkExhausted(provider, accountID)
+ return
+ }
s.SchedulerRef.MarkExhaustedUntil(provider, accountID, claudeExhaustionExpiry(header, time.Now()))
}Also applies to: 2947-2950
🤖 Prompt for 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.
In `@internal/proxy/proxy.go` around lines 1688 - 1692,
`markAccountExhaustedFromResponse` currently applies `claudeExhaustionExpiry`
for every provider because it ignores `provider`, which can make the Codex retry
path honor `Retry-After` instead of the default TTL. Update the helper in
`Server.markAccountExhaustedFromResponse` to branch on the `accounts.Provider`
value and only derive the expiry from Claude response headers for Claude; for
all other providers, keep using the scheduler’s default exhaustion TTL. Make
sure the caller path around the Codex retry logic continues to route through
this helper so the provider-specific behavior is centralized.
| if raw := strings.TrimSpace(claudeHeaderGet(header, "anthropic-ratelimit-unified-reset")); raw != "" { | ||
| if epoch, err := strconv.ParseInt(raw, 10, 64); err == nil && epoch > 0 { | ||
| until = time.Unix(epoch, 0) | ||
| } | ||
| } else if ra := strings.TrimSpace(claudeHeaderGet(header, "Retry-After")); ra != "" { | ||
| if secs, err := strconv.Atoi(ra); err == nil && secs > 0 { | ||
| until = now.Add(time.Duration(secs) * time.Second) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the relevant function and nearby uses of the headers.
FILE="internal/proxy/proxy.go"
echo "== File size =="
wc -l "$FILE"
echo
echo "== Outline =="
ast-grep outline "$FILE" --view expanded || true
echo
echo "== Relevant region =="
sed -n '1670,1735p' "$FILE" | cat -n
echo
echo "== Search for header parsing and Retry-After handling =="
rg -n 'anthropic-ratelimit-unified-reset|Retry-After|ParseTime|strconv\.Atoi|time\.Unix|DefaultExhaustedTTL' "$FILE"Repository: manaflow-ai/subrouter
Length of output: 22700
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Search for Retry-After parsing across repo =="
rg -n 'Retry-After|ParseTime\(' .
echo
echo "== Relevant nearby code for overload backoff =="
sed -n '2728,2748p' internal/proxy/proxy.go | cat -n
echo
echo "== Search for tests around claudeExhaustionExpiry / overload backoff =="
rg -n 'claudeExhaustionExpiry|claudeOverloadBackoff|Retry-After' internal -g '*_test.go'Repository: manaflow-ai/subrouter
Length of output: 4202
Fall back to Retry-After when reset parsing fails.
A malformed anthropic-ratelimit-unified-reset currently bypasses the Retry-After path; run the fallback when reset parsing doesn’t succeed, not only when the header is absent.
🤖 Prompt for 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.
In `@internal/proxy/proxy.go` around lines 1703 - 1710, The fallback logic in the
header parsing block should use Retry-After whenever
anthropic-ratelimit-unified-reset is missing or invalid, not just when it is
absent. Update the parsing around claudeHeaderGet, strconv.ParseInt, and
strconv.Atoi so that a malformed reset value does not short-circuit the
Retry-After path; only keep until from the reset header when parsing succeeds,
otherwise continue to the Retry-After branch.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8df07ce22f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if s.SchedulerRef == nil { | ||
| return | ||
| } | ||
| s.SchedulerRef.MarkExhaustedUntil(provider, accountID, claudeExhaustionExpiry(header, time.Now())) |
There was a problem hiding this comment.
When Claude returns a 401 authentication_error, claudeAccountExhaustedByResponse routes it through this helper with no reset headers, so claudeExhaustionExpiry stores only DefaultExhaustedTTL; after 10 minutes SchedulerRef.Get prunes the mark and the scheduler can pick the same dead OAuth credential again. A 401 is not a quota window that will recover at reset time, so these auth failures should keep using a refresh/reauth-driven exhaustion mark rather than the new rate-limit expiry.
Useful? React with 👍 / 👎.
Autoreview round 3 flagged that a lapsed expiry lets a still-cooked account (fresh-zero and carried-zero scores are indistinguishable without provenance) go optimistic once. That is the intended tradeoff, consistent with the scheduler-wide optimistic-routing philosophy: the probe's reject re-marks the account with the new authoritative reset, bounding the cost at one attempt per account per expiry window, while the opposite choice (trusting zero scores without expiry) is exactly the stranded-recovered-quota failure this fixes. Documented on pruneExpired and covered by TestLapsedMarkRemarksOnNextReject. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1618082579
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const DefaultExhaustedTTL = 10 * time.Minute | ||
|
|
||
| func (r *SchedulerRef) MarkExhausted(provider accounts.Provider, accountID string) { | ||
| r.MarkExhaustedUntil(provider, accountID, time.Now().Add(DefaultExhaustedTTL)) |
There was a problem hiding this comment.
Separately from the 401 response path, MarkExhausted is still used for terminal credential failures in proxy.go's oauthRetryAccount and refreshSelectedAccount, but this change makes every such mark expire after 10 minutes. When a refresh token is permanently invalid or missing, the next SchedulerRef.Get after that TTL prunes the zero score and Pick treats the account as fully healthy again, so new requests can keep selecting a dead credential and incur refresh-failover or upstream 401s instead of staying out of rotation until reauth/account reload. The expiring path should be limited to quota-window responses with reset times, not permanent credential failures.
Useful? React with 👍 / 👎.
Addresses autoreview round 4: (1) the passive body-inspect path called the plain default-TTL mark, which could overwrite a header-derived reset expiry set moments earlier — it now recomputes from the same response headers. (2) Dead credentials (401 / terminal invalid_grant refresh failures) get a 1h credential TTL instead of the 10m rate-limit default: they only heal via human re-auth, so frequent probes are overhead, but the mark still lapses so a re-authed account rejoins within the hour without needing a successful usage refresh. Transient refresh failures keep the short default. Adds ExhaustedUntilFor for verification and TestMarkTTLSelection pinning expiry per class + the no-shortening re-mark. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="internal/proxy/claude_ratelimit_routing_test.go">
<violation number="1" location="internal/proxy/claude_ratelimit_routing_test.go:1048">
P1: The inverted comparison for the transient-refresh assertion (`blip@refresh-failure`) is reversed: it checks `time.Until(blipUntil) > 15*time.Minute` but should check `< 15*time.Minute`. Because transient marks default to ~10m remaining, the current condition causes a false fatal and breaks the test. Swap the comparison so the test validates the expected TTL bound.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| } | ||
| server.markAccountExhaustedRefreshFailure(accounts.ProviderClaude, "blip@example.com", fmt.Errorf("dial tcp: connection refused")) | ||
| blipUntil, _ := server.SchedulerRef.ExhaustedUntilFor(accounts.ProviderClaude, "blip@example.com") | ||
| if time.Until(blipUntil) > 15*time.Minute { |
There was a problem hiding this comment.
P1: The inverted comparison for the transient-refresh assertion (blip@refresh-failure) is reversed: it checks time.Until(blipUntil) > 15*time.Minute but should check < 15*time.Minute. Because transient marks default to ~10m remaining, the current condition causes a false fatal and breaks the test. Swap the comparison so the test validates the expected TTL bound.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At internal/proxy/claude_ratelimit_routing_test.go, line 1048:
<comment>The inverted comparison for the transient-refresh assertion (`blip@refresh-failure`) is reversed: it checks `time.Until(blipUntil) > 15*time.Minute` but should check `< 15*time.Minute`. Because transient marks default to ~10m remaining, the current condition causes a false fatal and breaks the test. Swap the comparison so the test validates the expected TTL bound.</comment>
<file context>
@@ -1003,3 +1004,48 @@ func TestClaudeFailoverTriesRecoveredAccount(t *testing.T) {
+ }
+ server.markAccountExhaustedRefreshFailure(accounts.ProviderClaude, "blip@example.com", fmt.Errorf("dial tcp: connection refused"))
+ blipUntil, _ := server.SchedulerRef.ExhaustedUntilFor(accounts.ProviderClaude, "blip@example.com")
+ if time.Until(blipUntil) > 15*time.Minute {
+ t.Fatalf("transient refresh mark in %v, want ~10m default", time.Until(blipUntil))
+ }
</file context>
…est evidence Addresses autoreview round 5: fresh-confirmed exhaustion and a carried-forward zero seed were indistinguishable, so an older request-time expiry could lapse a freshly-observed zero back to optimistic. Score.Fresh now tags scores computed from a successful current usage fetch (set at the single write site in scoreAccounts; seeds explicitly carry Fresh=false). Expiry reconciliation: superseded (headroom) drops the expiry; carried zero keeps it (the stranded- recovered-account fix); fresh zero re-anchors it to the fetch's own window reset (floored at the default TTL, capped 8d) — extend-only, so an authoritative long reset from a rejected response still holds. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Exhaustion marks now expire at the upstream's own reset time (with per-failure- class TTLs and fresh-evidence re-anchoring), fixing recovered accounts staying unroutable while users got 429s (#38). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7b619e099b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if r.exhaustedUntil == nil { | ||
| r.exhaustedUntil = make(map[string]time.Time) | ||
| } | ||
| r.exhaustedUntil[ScoreKey(provider, accountID)] = until |
There was a problem hiding this comment.
When an account already has a later expiry from Anthropic-Ratelimit-Unified-Reset, a subsequent mark without reset headers (for example the WebSocket usageLimitJSON path calling MarkExhausted) passes now+DefaultExhaustedTTL, and this assignment replaces the later timestamp. That shortens an hourly/weekly reset window to 10 minutes, so Get() can prune the zero score early and route traffic back to an account that upstream still says is exhausted; keep the later of the existing and new expiries unless the mark is being explicitly cleared or reclassified.
Useful? React with 👍 / 👎.
1 participant
Caught live (the core failed-reroute condition)
During tonight's capacity crunch,
aziz@manaflow.ai's weekly window reset (57% quota available, 88% session headroom) — yet failing sessions burned all 6 failover attempts on genuinely-cooked accounts and never tried it. Real users got 429s while real quota sat unroutable.Root cause:
markAccountExhaustedzero-scores an account permanently — the mark only clears on a successful usage refresh. Under load the usage endpoint rate-limits and the refresh falls back to stale "known-good" data, so a recovered account stays buried indefinitely, ranked below stale-scored cooked accounts.Fix
Exhaustion marks now expire at the moment the account actually recovers:
MarkExhaustedUntil(provider, id, until)records an expiry per mark;Get()lazily prunes lapsed marks back to the optimistic default, so routing retries the account and request-time truth decides.anthropic-ratelimit-unified-reset(authoritative reset epoch) →Retry-After→ 10-minute default; clamped to[now+1m, now+8d].Set/FinishRefresh) clears pending expiries so a prune can never delete refreshed scores.Tests
🤖 Generated with Claude Code
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Expire exhaustion marks at the upstream’s reset so recovered accounts get retried and failover reaches real quota. Preserve expiries through partial refreshes, extend them when a fresh fetch re-confirms exhaustion, and use longer TTLs for dead credentials.
MarkExhaustedUntil; proxy now sets expiry viaclaudeExhaustionExpiryusinganthropic-ratelimit-unified-reset→Retry-After→ 10m default, clamped to [now+1m, now+8d] (codex falls back to the default).Score.Freshto reconcile refreshes: headroom clears the mark; a carried zero keeps its expiry; a fresh zero re-anchors/extends it to at least the window reset (floored at the default TTL, capped at 8d).Get()to restore optimistic scores; a lapsed mark allows one probe and a reject re-marks it with the new reset; body-inspect re-mark uses headers so it doesn’t shorten a header-derived expiry.Written for commit 7b619e0. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Tests