Repository navigation
fix(sse): honor native Claude quota scope - #13888
diegosouzapw merged 23 commits into
Conversation
abhisheksharma2411
left a comment
abhisheksharma2411
left a comment
There was a problem hiding this comment.
The per-model direction is right for one of Claude's two auth modes, and the test is a good one — asserting the sibling stays selectable, not just that the lock exists. But I think the OAuth case it actually tests is the case where it may be wrong, and I'd rather raise it before it lands than after.
The message in your test is one the repo already classifies as account-wide.
open-sse/services/quotaTextCooldowns.ts:47:
// Native Claude OAuth uses this otherwise-generic 429 wording for an
// exhausted subscription window. Keep it provider-scoped: other upstreams
// can use the same phrase for a short RPM throttle.
(provider === "claude" && lower.includes("this request would exceed your account's rate limit"))That is isSubscriptionQuotaText — #2321, the Claude Pro/Team 5-hour subscription window — and it carries SUBSCRIPTION_QUOTA_COOLDOWN_MS = 1 hour. Your test sends that exact string with authType: "oauth".
A subscription window is spent per account, not per model. claude-opus-5 does not have its own five-hour bucket, so after the lock the sibling is routable into a quota that is already exhausted — one upstream 429 per remaining model.
Both tests pass on your branch, which is what makes this worth flagging rather than obvious. I ran them on 2a967d7:
tests/unit/account-fallback-anthropic-quota.test.ts 13 pass, 0 fail
"markAccountUnavailable scopes native Claude account..." 1 pass, 0 fail
The existing suite still gets QUOTA_EXHAUSTED with a ~1h cooldown from classifyError, while markAccountUnavailable now locks a model and leaves the connection active — your test asserts rateLimitedUntil === undefined, which is precisely the part that changes. The reason still says account-wide exhaustion; the scope no longer agrees with it.
The codebase already spells out this failure mode for the analogous provider. src/sse/services/auth.ts ~L2800:
agentrouter is a passthroughModels provider (
isPerModelQuotaProvider === true), so without this branch the nextifwould treat it like any other passthrough 429 and lock a SINGLE model — leaving combo routing to burn one upstream call per remaining model of the same exhausted account. Must run BEFORE that block.
That connection-scope branch is keyed on isAgentrouterConnectionQuotaScope, so Claude would not reach it.
Where I think you're right: Anthropic API-key rate limits genuinely are per-model (separate RPM/TPM per model family), so scoping those to the model is the correct fix and siblings really are routable. The issue is only that canonicalId === "claude" covers both auth modes, and the subscription case is the one your test exercises.
Two shapes that would keep the win without the regression — whichever fits the codebase better:
- Gate on auth mode, so OAuth/subscription connections keep the connection-level cooldown and API-key connections get the per-model lock.
- Keep the one-liner but let
isSubscriptionQuotaTexttake precedence, the way the agentrouter connection-scope branch runs ahead of the per-model block.
Worth a regression test either way: a Claude OAuth connection that takes the subscription message should leave the sibling unroutable, which is the inverse of the assertion you have now. Happy to be wrong if OAuth quota is in fact per-model — if you have a 429 body from a real subscription exhaustion showing the model scoped, that settles it.
|
I think you are right this is to broad, the isolated model can be changed depending on their policy, I remembered there is one time that the isolated model was Sonnet and now Fable. I think a way to define the isolated model rather than broad logic like this better: I've pulled this, and I think we can use this as the logic, will try to create changes and let you review again. Thanks @abhisheksharma2411 |
|
That payload is a much better answer than either option I offered — it removes the guesswork entirely. The scope shouldn't come from a hardcoded provider list or from the auth mode; it should come from the response, because the upstream is already telling you: That's exactly the distinction the current Two things worth designing in from the start: 1. 2. Branch on Roughly:
Worth checking against Happy to review the follow-up — and thanks for taking the point rather than defending the one-liner. |
…de-model-quota-scope
…de-model-quota-scope
…de-model-quota-scope
riez
changed the title
|
Thanks @abhisheksharma2411, I revisited the implementation and addressed your concerns. I’ve also updated this branch with the upstream Those help, but #13266 deliberately leaves routing and cooldowns unchanged, while #12957 covers 5xx failures. The remaining issue is the subscription-quota 429: Fable can be exhausted while the shared quota still has room, but a connection-wide cooldown also prevents switching to Opus. This PR now takes the scope from Anthropic’s payload:
The parser also handles the mixed payload shown in #13266, where Added regression coverage for missing percentages, unresolved scopes, cold/stale cache, reset propagation, and sibling-model availability. The focused suite passed 221 tests, and the changed integration suite passed 48. Could you take another look? |
|
@diegosouzapw if you have time kindly take a look as well for code review. Thanks. |
abhisheksharma2411
left a comment
abhisheksharma2411
left a comment
There was a problem hiding this comment.
@riez this is a much better shape than what either of us started with, and the parts I was worried about are genuinely handled. Walking getCachedClaudeQuotaScopeDecision against the table from last week:
- active
weekly_all/session→scope: "connection"✅ - active
weekly_scopedthat resolves →scope: "model"✅ - active
weekly_scopedthat doesn't resolve → falls toscope: "connection"✅ — this is the fail-safe I cared about most and you built it exactly right isActive+severitydecide blocking, percent is display-only ✅- real
resetsAtpropagated ascooldownMsinstead of a fixed window ✅
quotaTextCooldowns also lines up now — isExplicitClaudeQuota429Text runs through isSubscriptionQuotaText, so the reason and the action can't disagree the way they used to. That was the loose end I flagged and it's tied off.
I pulled the branch and probed it rather than just reading. Two things came out, one of which I think matters.
1. Two scoped limits that don't name a model collapse into one — and the survivor can be the inactive one.
normalizeCurrentLimits is careful to keep them distinct in semanticKey:
: (metadata.scopeKey ?? `weekly_scoped:unknown:${index}`),…but the output map at the end of normalizeClaudeUsageQuotas re-keys everything by displayKey, and quotaKey() renders every model-less scoped limit as the same string:
return `weekly ${(displayName || "scoped").toLowerCase()} (7d)`; // -> "weekly scoped (7d)"So the index disambiguation is thrown away one line later, and last-write-wins. With two model-less scoped limits where the first is the active critical one:
PROBE A keys: ["weekly scoped (7d)"]
PROBE A values: [{"active":false,"severity":"normal"}] <- the active/critical one is gone
End to end, with a control:
both model-less : {"scope":"connection","evidence":"none", "resetAt":null, "cooldownMs":null}
active alone : {"scope":"connection","evidence":"blocking","resetAt":"...17:16:51Z","cooldownMs":599999}
Adding an unrelated, inactive limit to the payload erases an active critical one, and the fail-safe you built silently stops firing — evidence drops to "none", so it's back to the generic cooldown with no reset. That's the "an unmatched scoped limit reads as no model limit" failure landing on the wrong side.
Fixture I used (scope.surface non-null, model null) is inferred from the shape in your own paste — "surface": null is right there, so the field exists and can presumably be populated. I can't prove Anthropic sends two of these today. But the collision holds for any two scoped limits that don't resolve to a model, and since you already anticipated duplicates in semanticKey, I'd just carry that through: key the output on semanticKey and keep displayKey for presentation, or fold a surface/index discriminator into quotaKey().
2. Display-name matching crosses model generations.
const requestedTokens = new Set(normalizedTokens(requestedKey));
const displayTokens = normalizedTokens(metadata.modelDisplayName);
return displayTokens.length > 0 && displayTokens.every((token) => requestedTokens.has(token));Subset, not equality — so a display name is satisfied by any model whose tokens are a superset:
display="Claude Opus 4" requested=claude-opus-4-5 -> true
display="Claude Sonnet 4" requested=claude-sonnet-4-5 -> true
display="Opus" requested=claude-opus-5 -> true
display="Fable" requested=claude-fable-5-1 -> true
The last two are the behaviour you want. The first two aren't: an exhausted Opus 4 window captures a request for Opus 4.5, which has its own untouched quota.
PROBE D requested=claude-opus-4 -> scope=model evidence=blocking
PROBE D requested=claude-opus-4-5 -> scope=model evidence=blocking <- locks a model that isn't limited
PROBE D requested=claude-sonnet-5 -> scope=connection evidence=blocking <- correct fail-safe
Three things go wrong at once there: the wrong model gets locked, it gets locked with another window's resetsAt, and whatever actually caused the 429 goes unattributed. claude-sonnet-5 shows the right answer for comparison — unresolved, so cool the connection.
Note the modelId branch is exact (requestedKey === normalizedRequestedModel(metadata.modelId)), so this is only the displayName fallback — the path that exists precisely because id comes back null, which is the case that'll actually be hit.
I don't think plain set-equality is the fix, since it'd break the "Fable" → claude-fable-5-1 case that's the whole point. Two options that both work:
- resolve the display name against
getProviderModels("claude")and only treat it as a match if it lands on the requested model's family and the version segments don't contradict; or - keep the subset rule but reject when the display name carries a version token that isn't the requested model's version token —
["claude","opus","4"]vsclaude-opus-4-5fails,["fable"]vsclaude-fable-5-1still passes because it names no version.
Either way the unresolved case already does the right thing, so the safe answer is available for free.
None of this is a rewrite — (1) is a key choice at one line and (2) is the tail of one predicate. The architecture underneath is right, and the isActive/severity/resetsAt plumbing is cleaner than what I suggested. Happy to re-check once you've had a look.
|
Fixed both in 6b2a0f7. unresolved scoped windows now stay distinct and preserve the active reset regardless of order. versioned display names require an exact version match, while family-only names like Fable still work. added regression coverage for both cases, including the chat path. could you take another look? |
abhisheksharma2411
left a comment
abhisheksharma2411
left a comment
There was a problem hiding this comment.
Both fixed, and I re-ran the exact probes from my last review against 6b2a0f75 rather than reading the diff.
Finding 1 — the collapsing scoped windows. Was one key with the inactive window surviving; now:
keys: ["weekly scoped (7d) #1","weekly scoped (7d) #2"]
active first -> {"scope":"connection","evidence":"blocking","cooldownMs":599999}
active second -> {"scope":"connection","evidence":"blocking","cooldownMs":599999}
Previously the second ordering gave evidence:"none" with no reset. Both orders now preserve the active critical window and its real reset, so the fail-safe fires regardless of the order upstream happens to send them in — which is the property that matters, and you tested it in both directions rather than just the one I demonstrated.
Finding 2 — version-crossing display names. The numeric-token comparison does exactly what was needed:
display="Claude Opus 4" requested=claude-opus-4-5 -> false (was true)
display="Claude Opus 4" requested=claude-opus-4 -> true
display="Claude Sonnet 4" requested=claude-sonnet-4-5 -> false (was true)
display="Fable" requested=claude-fable-5-1 -> true
display="Opus" requested=claude-opus-5 -> true
The two family-only labels still match, which was the constraint that ruled out plain set-equality. Requiring equal length and order on the version tokens is stricter than the "same version prefix" I had in mind and is the safer direction — Opus 4 can't absorb Opus 4.5 and vice versa. The -\d{8} strip before comparing is a good catch I hadn't considered; a dated snapshot id would otherwise have read as an extra version token.
claude-quota-model-matching, claude-quota-normalization, claude-scoped-quota-auth and claude-fable-usage together: 40 pass / 0 fail.
Nothing blocking. Two small notes, both optional:
#1/#2is positional, so it is not stable across polls. It doesn't affect routing — the decision iteratesObject.values(entry.modelQuotas)and never reads the key, andquotaSnapshotChangedonly looks atquotas, notmodelQuotas— so this is a display concern only. But if upstream reorders itslimits[]between polls, the same logical limit moves between#1and#2in the UI.scope.surfaceis sitting right there in the payload and is still unread; keying on it when present (falling back to the index) would give a stable label for free.- Worth a line in the PR body that an unresolved scoped limit now cools the connection rather than being ignored, since that is the user-visible behaviour change for anyone whose account starts receiving surface-scoped limits.
Good iteration on this one — the shape you landed on is better than what either of us started with.
|
Fxed in 27d5161. we now use scope.surface.display_name for stable row keys and readable labels when available. anonymous or duplicate surfaces remain separately numbered so no quota window gets dropped. probed both payload orders locally: labels stay stable, and connection-scoped cooldowns retain the correct reset without creating a model lock. 103 unit/integration tests and 2 rendering tests pass. could you take another look @abhisheksharma2411? |
# Conflicts: # tests/integration/combo-matrix/context-relay-handoff.test.ts # tests/integration/combo-matrix/weighted.test.ts
Co-authored-by: diegosouzapw <8016841+diegosouzapw@users.noreply.github.com>
Reconcile markAccountUnavailable with the tip's OAuth 401 backoff (cheaper-inference#14917): the Claude-scope cooldown now feeds resolvedCooldownMs, the OAuth backoff still overrides it, and a Claude quota resetAt is only used when that backoff did not apply.
The tip moved the quota-cache types/state into quotaCacheState.ts (cheaper-inference#14820); carry this PR's QuotaInfo.claudeQuota and QuotaCacheEntry.modelQuotas fields there (type-only import keeps the leaf free of runtime imports).
d2c950b
into
cheaper-inference:release/v3.8.51
…#14190, proxyLogs import side effect (#15116) Release-captain base-red fix (v3.8.51 release PR #11442, unit shards 1-2 + integration): production defect — OmniRoute's own local-cooldown 429 was classified as CLIProxyAPI quota exhaustion since #14190 and made combos skip same-provider targets (now tagged X-OmniRoute-Local-Cooldown); the proxyLogHost extraction reconciled with #15109; contract propagations for #14627/#14873/#13888/#14876/#15001 and two harness isolation fixes. Focused suites green on the reconciled tip, typecheck clean.
Summary
weekly_scopedlimit identifies only the failed model.Why this PR exists
This PR was opened after Fable 5.1 exhausted its model quota, then switching to Opus 5 still produced the following failure even though the Claude quota dashboard showed roughly 40% remaining overall:
OmniRoute previously interpreted that native Claude 429 without using the upstream quota scope. A model-specific limit could therefore cool the whole OAuth connection and prevent a sibling model from using its remaining quota.
Behavior
The normalizer handles Anthropic payloads that contain either or both quota generations:
limits[], includingweekly_scoped,scope.model,isActiveoris_active, andresetsAtorresets_at.five_hour,seven_day, andseven_day_*fields.limits[], and empty or invalid current data does not erase valid session or weekly data.quotas; scoped model windows are stored separately inmodelQuotas, so an exhausted Fable pool cannot reduce Opus connection headroom.isActiveandseveritydetermine whether a current limit blocks. Percent utilization is display metadata and may be absent.For a native Claude subscription quota 429 with fresh quota evidence:
weekly_scopedmodel limitweekly_scopedlimitsessionor account-wide weekly limitAdditional safeguards:
ccandclaudealiases resolve to the same native Claude identity while request-local combo keys keep their original provider identity.anthropicAPI-key provider is unchanged.Release-base validation repairs
While reconciling with the active release branch, the required routing checks exposed unrelated stale test state. This PR also makes those checks deterministic without changing their product behavior:
claude-sonnet-4-6.ESLint suppression cleanup is intentionally excluded from this PR.
config/quality/eslint-suppressions.jsonhas zero net diff from the current release base. Unrelated upstream comments are retained, and quota policy was extracted into focused owners instead of deleting rationale or raising frozen baselines.Upstream conflict reconciliation (9a2ae7a)
Merged
upstream/release/v3.8.51at6f1d4beabca5a9ffb81857705003caba048fa584into this branch without rewriting history.targetExhaustion.tsby retaining the PR's structured result, effective connection, and Claude quota handling while preserving upstream [BUG] Model-scoped 403 on per-model-quota providers marks the whole connection exhausted in combos (reproduced with alibaba) #14136 model-scoped 403 behavior and Vertex connection-wide denials.quotaCache.tsby retaining Claude metadata/model quotas alongside upstream fix(quota): prevent Vertex spend telemetry from exhausting accounts #14276 unknown-fraction normalization, persistence, and hydration behavior.providerExhausted; assertions are unchanged.modelQuotasmap as well.Verification:
npm run typecheck:core.npm run lint -- --pass-on-unpruned-suppressions.npm run check:file-size -- --base-ref upstream/release/v3.8.51.git diff --check upstream/release/v3.8.51; no unmerged index entries.Known inherited checks:
npm run lintreports only stale suppression entries. They are not pruned in this PR.npm run check:open-sse-typecheckreports eight errors inopen-sse/executors/auggie.ts(TS2769 x4, TS18047 x4). That file, the relevant typecheck configurations, and lockfile are identical to upstream. No baseline widening or unrelated fix was added.Commit gates were run manually with operator approval for a one-commit hook exception, avoiding the repository-prohibited lint-staged auto-stash.
Post-reconciliation regression commands
Previous surface-label follow-up (27d5161)
Addressed the remaining optional surface-label concern:
scope.surface.display_name, the nullish object shape verified in the installed first-party Claude Code 2.1.272 cached-usage schema. Test surface names are synthetic; this is not a claim that a live account returned those names.Verification:
--no-cache --pass-on-unpruned-suppressions, Prettier, andgit diff --check.npm run check:open-sse-typecheck: 0 errors.npm run check:file-size: passed.Surface follow-up regression commands
No live Anthropic requests, account/configuration changes, or deployment. Full lint/build/CI matrix was not rerun for this four-file follow-up; the previously documented release-base lint failures and suppression cleanup remain outside this change.
Previous review follow-up (6b2a0f7)
Addressed both remaining review findings:
weekly_scopedwindows through the final output map, including missing model metadata and non-tokenizable display names. An inactive window can no longer erase the active window's reset, regardless of payload order.Regressions were demonstrated failing before the fixes, including the real chat handler with an isolated database and mocked upstream 429 responses. No live provider quota was consumed.
Tests added or extended:
tests/unit/claude-quota-model-matching.test.ts(new)tests/unit/claude-quota-normalization.test.tstests/integration/claude-quota-chat-ordering.test.tsVerification for 6b2a0f7:
--no-cache --pass-on-unpruned-suppressionsnpm run check:open-sse-typecheck: 0 errorsgit diff HEAD --checknpm run check:file-size -- --base-ref 0d9299df9b0dc85275958e2591ceb8d6eabded82: passedconfig/quality/eslint-suppressions.jsonremains identical to the PR release baseFocused regression command
Full
npm run lintis still base-red. A fresh uncached run also reports 92 existing rule errors in five files unchanged from the PR release base, in addition to stale suppression entries in the plain lint run. Those files and the suppression baseline are intentionally untouched. The full build/CI matrix and a live Anthropic test were not rerun for this follow-up.Earlier validation (before this follow-up)
--no-cacheand--pass-on-unpruned-suppressionsnpm run typecheck:corenpm run check:open-sse-typechecknpm run check:cyclesgit diff --check upstream/release/v3.8.51upstream/release/v3.8.51npm run check:file-size -- --base-ref upstream/release/v3.8.51The file-size command is base-red only for
tests/unit/chatcore-translation-paths.test.ts: checker count 3449 exceeds its frozen 3447 baseline. The PR copy is byte-identical to current upstream (b5ef7a62babcafb6b83251fc881cef929de1f6f0), so this PR does not change that file or its baseline.Plain
npm run lintalso remains base-red because stale suppression entries are still present. This PR leaves that cleanup to upstream or a separate reconciliation PR. All ESLint rules for changed TypeScript files pass with unpruned suppressions tolerated.Reviewer notes