Fix automatic rollover across quota-limited accounts#5247
Conversation
github-actions
Bot
added
vouched
![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: 6ef53c3ed7
ℹ️ 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".
| // per-minute caps) classify as RATE_LIMIT_EXCEEDED in | ||
| // `parseRateLimitReason` and stay in the provider's own backoff layer | ||
| // instead of burning siblings. | ||
| if (AIError.isUsageLimit(error)) return true; |
There was a problem hiding this comment.
When auth-gateway streams a provider error that only carries a structured usage-limit code/errorId, for example ProviderHttpError(..., 429, { code: "insufficient_quota" }) surfaced with generic text, this new branch makes streamSimple ask the gateway resolver to rotate. However refreshGatewayApiKeyAfterAuthError still chooses usage-vs-auth via only isUsageLimitOutcome(extractHttpStatusFromError(error), message), ignoring the attached Flag.UsageLimit; with generic text it falls through to invalidateCredentialMatching instead of markUsageLimitReached. In gateway deployments this burns quota-exhausted accounts as suspect credentials rather than temporarily blocking them, so the gateway resolver path needs to honor AIError.isUsageLimit(error) before enabling these retries.
Useful? React with 👍 / 👎.
Keep ordinary 401 retries bounded while replay-safe quota failures walk every distinct eligible credential. Anchor blocks to the failed credential and stop on cycles, aborts, or 64 attempts.
jeffscottward
force-pushed
the
fix/automatic-account-rollover
branch
from
6ef53c3 to
eb3a990
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eb3a9905bd
ℹ️ 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".
| signal, | ||
| apiKey: previousKey, | ||
| }); | ||
| if (!rotated) return undefined; |
There was a problem hiding this comment.
When an OAuth credential's access token is refreshed by the broker or another session after this request resolved previousKey but before the provider returns a quota/401 error, rotateSessionCredential(... apiKey: previousKey) now treats that stale bearer as an explicit target and returns false if no current row matches it. Because this new guard returns undefined immediately, the ModelRegistry resolver stops the replay-safe rollover instead of re-resolving or switching to a sibling, so multi-account OAuth pools can still fail on the first quota-limited account during normal refresh races.
Useful? React with 👍 / 👎.
jeffscottward
force-pushed
the
fix/automatic-account-rollover
branch
from
eb3a990 to
ceb57b0
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ceb57b0f8f
ℹ️ 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".
| signal, | ||
| apiKey: previousKey, | ||
| }); | ||
| if (!rotated) return undefined; |
There was a problem hiding this comment.
When AuthStorage.resolver is used directly and rotateSessionCredential returns false, this stops the retry immediately. With the new explicit apiKey: previousKey targeting, that false also occurs when a peer/broker refreshed an OAuth row after the failed request was sent, so the old bearer no longer matches storage; in that case getApiKey could return the refreshed bearer or another sibling, and the retry driver will already reject an already-attempted key. The coding-agent resolver now re-resolves for this race, but this storage-backed resolver still gives up for direct users of authStorage.resolver.
Useful? React with 👍 / 👎.
jeffscottward
force-pushed
the
fix/automatic-account-rollover
branch
from
ceb57b0 to
dc816ca
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dc816cac80
ℹ️ 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 (explicit) return undefined; |
There was a problem hiding this comment.
When auth-gateway handles a usage-limit error for an OAuth bearer that a broker/peer refreshed after the request was sent, the supplied apiKey no longer matches the reloaded rows, so this new explicit-target path returns undefined instead of falling back to the session-sticky credential. markUsageLimitReached then reports switched: false, and the gateway aborts the replay-safe retry even when sibling credentials are available. Consider falling back or re-resolving specifically for the stale explicit-target case.
Useful? React with 👍 / 👎.
jeffscottward
force-pushed
the
fix/automatic-account-rollover
branch
from
dc816ca to
8a1f641
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8a1f641332
ℹ️ 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".
| // A declined rotation can mean another session already refreshed the | ||
| // failed bearer. Re-resolve either way; retry drivers reject an | ||
| // already-attempted key when there is genuinely no sibling. | ||
| return registry.getApiKeyForProvider(provider, sessionId, { baseUrl, modelId }); |
There was a problem hiding this comment.
The new multi-credential usage-limit loop makes this unconditional re-resolve unsafe when rotateSessionCredential returns false because all siblings are already blocked, not because of a stale-bearer race. In that case getApiKeyForProvider can deliberately fall back to a blocked OAuth/API-key credential (for example Codex account A is blocked briefly, account B then hits quota), and the retry driver will accept that not-yet-attempted key and replay immediately instead of letting the outer retry/delay-cap use the no-sibling backoff. Preserve the stale-refresh recovery, but don't re-resolve into known-blocked fallbacks when rotation declined for real.
Useful? React with 👍 / 👎.
jeffscottward
force-pushed
the
fix/automatic-account-rollover
branch
from
8a1f641 to
b929ed1
Compare
roboomp
left a comment
roboomp
left a comment
There was a problem hiding this comment.
Rank: review:p3 — already landed / superseded. Thanks for the thorough rollover work, @jeffscottward.
The functional content of this PR is already present on origin/main:
packages/ai/src/auth-retry.tsandpackages/ai/src/error/auth-classify.tsare byte-identical tomain(git diff origin/main..HEADfor both = 0 lines) —resolveNextAuthRetryKey,AUTH_RETRY_MAX_ATTEMPTS,isDirectCredentialRotationError,createAuthRetryKeyStateall already exist there.AuthStorage.#resolveCredentialTarget(auth-storage.ts), theresolveNextAuthRetryKeywiring instream.ts, and thepreviousKey+if (!rotated) return undefinedguard incoding-agent/src/config/api-key-resolver.tsare all inmain.mainhistory shows the rollover semantics landing via22374ab(test(coding-agent): align retry rollover…) and5e74444(fix(test): repair auth-storage-rotation merge resolution).
The PR is closed, so no action is needed. I reviewed the changed source for correctness regardless — the state machine (bounded refresh+one-switch for 401/auth, direct sibling rotation for usage/quota, attemptedKeys/attemptedCredentialIdentities cycle guards, and the AUTH_RETRY_MAX_ATTEMPTS ceiling) is internally consistent and matches the shipped code, and both changelogs carry [Unreleased] entries. No blocking findings.
Open question for the maintainer: nothing — this appears fully superseded by what's already merged; safe to leave closed.
3 participants
Summary
Testing
bun run check:tsbun run checkinpackages/aiandpackages/coding-agentbun test packages/ai/test— 2,817 passed, 337 skippedissue-983-multi-file-extensiontest fails locally, while the other sandbox-only rendering failure passes in a native rerunSafety