fix: scope Spark cooldowns to its native quota - #599
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReset-derived Codex cooldowns are tracked by confirmed quota scope, while explicit ChangesQuota-scoped Codex cooldowns
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Request
participant resolveCodexAuthContext
participant CodexUpstream
participant recordCodexUpstreamOutcome
Request->>resolveCodexAuthContext: Resolve account for modelId
resolveCodexAuthContext-->>Request: quotaScope and probe metadata
Request->>CodexUpstream: Forward model request
CodexUpstream-->>Request: Response and reset headers
Request->>recordCodexUpstreamOutcome: Record modelId, resetAt, and probe scope
recordCodexUpstreamOutcome-->>Request: Updated scoped or account-wide health
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
✅ Target branch corrected This pull request now targets The |
fa47cd1 to
a2b2b06
Compare
There was a problem hiding this comment.
Pull request overview
This PR updates the Codex pool cooldown system so that reset-derived 429 cooldowns are scoped to confirmed independent native quota groups (notably keeping gpt-5.3-codex-spark separate), allowing same-account failover combos to continue to later models when appropriate. It threads model + recovery-probe metadata through the Responses and native compaction pipelines, adds regression coverage, and documents the intended quota-group behavior.
Changes:
- Introduce quota-scoped upstream health for reset-derived cooldowns (Spark vs shared native group), while keeping
Retry-After/default cooldowns account-wide. - Propagate
modelIdand probe metadata through Responses and native compaction outcome recording to ensure correct scoping and probe handling. - Add focused tests for same-account combo failover and native compaction, and update user/structure documentation to match the new semantics.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/server-combo-failover-e2e.test.ts | Adds E2E regression test ensuring Spark reset-derived cooldown doesn’t block same-account Terra fallback. |
| tests/responses-compaction-routing.test.ts | Adds regression test ensuring Spark cooldown doesn’t block Terra native compaction on same pool account. |
| tests/codex-routing.test.ts | Verifies clearCodexAccountCooldown clears all live per-scope cooldown entries. |
| tests/codex-auth-context.test.ts | Adds unit tests confirming scoped cooldown behavior and scoped recovery probe interactions. |
| structure/08_openai-provider-tiers.md | Documents the quota-group rule: Spark independent vs shared native group; account-wide handling for Retry-After/default. |
| src/server/responses/core.ts | Threads modelId + probe metadata through auth resolution and upstream outcome recording for Responses. |
| src/server/responses/compact.ts | Threads modelId + probe metadata and now carries reset headers into compaction outcome recording. |
| src/codex/routing.ts | Adds quota-scope model mapping and scoped-health storage/leases; implements getCodexQuotaHealthSnapshot. |
| src/codex/auth-context.ts | Routes cooldown checks + probe acquisition through quota-scope snapshot/leases; carries quota scope in auth contexts. |
| docs-site/src/content/docs/guides/providers.md | Updates user docs to explain scoped reset-derived cooldown behavior and account-wide exceptions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a2b2b065b7
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/server/responses/compact.ts (1)
303-318: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRedundant if/else: both branches call
recordCompactPoolOutcomeidentically.Lines 309-318 branch on
upstream.ok && buffered.status >= 500but both theifandelsebodies execute the exact samerecordCompactPoolOutcome(upstream.status, { retryAfter, resetAt }). The comment above explains whyupstream.status(notbuffered.status) is intentionally used, but since that's true in both branches, the conditional is a no-op that could mislead future maintainers into thinking there's a behavioral difference.♻️ Proposed simplification
const buffered = await bufferCompactResponse(upstream, req.signal); // Record pool health only after the body is fully delivered (or definitively failed). // A premature 200 would clear soft-avoid while the client still sees a buffer 502. if (buffered.status === 499) { return buffered; } - if (upstream.ok && buffered.status >= 500) { - // The upstream account returned 200 — it is healthy. The buffering failure - // (oversized body exceeding COMPACT_RESPONSE_MAX_BYTES, or a rare mid-read - // reset on a small JSON payload) is a local proxy issue, not account flakiness. - // Record the upstream status so a deterministic payload-size limit does not - // soft-avoid a healthy account and rotate a thread for 30s. - recordCompactPoolOutcome(upstream.status, { retryAfter, resetAt }); - } else { - recordCompactPoolOutcome(upstream.status, { retryAfter, resetAt }); - } + // Always record the real upstream status (not the possibly-munged buffered + // status): a local buffering failure on a 200 upstream response must not + // soft-avoid a healthy account and rotate a thread for 30s. + recordCompactPoolOutcome(upstream.status, { retryAfter, resetAt }); return buffered;🤖 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 `@src/server/responses/compact.ts` around lines 303 - 318, Remove the redundant if/else conditional after the buffered.status === 499 early return in the compact response flow, and call recordCompactPoolOutcome(upstream.status, { retryAfter, resetAt }) once. Preserve the existing upstream-status recording behavior and surrounding comments.
🤖 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/codex/routing.ts`:
- Around line 906-933: The reset-derived quota branch in the quota outcome
handling must also perform shared-scope failover cleanup. When quotaScope ===
"shared", clear the account’s thread affinity via
clearThreadAccountMapForAccount(accountId) and rotate activeCodexAccountId
before returning; preserve the existing scoped cooldown behavior for other quota
scopes.
---
Outside diff comments:
In `@src/server/responses/compact.ts`:
- Around line 303-318: Remove the redundant if/else conditional after the
buffered.status === 499 early return in the compact response flow, and call
recordCompactPoolOutcome(upstream.status, { retryAfter, resetAt }) once.
Preserve the existing upstream-status recording behavior and surrounding
comments.
🪄 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: d4157ce6-2139-439f-9baf-855c667f7098
📒 Files selected for processing (10)
docs-site/src/content/docs/guides/providers.mdsrc/codex/auth-context.tssrc/codex/routing.tssrc/server/responses/compact.tssrc/server/responses/core.tsstructure/08_openai-provider-tiers.mdtests/codex-auth-context.test.tstests/codex-routing.test.tstests/responses-compaction-routing.test.tstests/server-combo-failover-e2e.test.ts
Maintainer review — bug + security (
|
| Source | Item | Status on tip |
|---|---|---|
| Codex P2 | Scoped reset-derived returns before affinity clear / active rotation; selectors only check account-wide cooldown → same account keeps being chosen for the cooled scope | Still valid — confirmed |
| CodeRabbit (Major) | Shared-scope reset-derived also skips clearThreadAccountMapForAccount + activeCodexAccountId rotation |
Still valid — same early-return at routing.ts:912-932 |
| Copilot | CodexAccountCooldownError / cooldownErrorMessage still reads account-wide when only a scope is cool |
Still valid — UX polish, non-blocking |
Bug review
| Severity | Location | Finding |
|---|---|---|
| high | src/codex/routing.ts:912-932 |
Reset-derived + known quotaScope stores scoped health and returns before the account-wide quota branch that clears thread affinity and rotates activeCodexAccountId. Pool selectors still use account-wide isCodexAccountInCooldown only, so a cooled Spark (or shared) account stays selectable for that same scope. In a 2-account pool, later Spark turns fail locally on A until probe/expiry even when B has Spark quota. Same-request alternate retry (#585) only helps the first upstream 429, not subsequent turns. |
| medium | src/codex/auth-context.ts + cooldownErrorMessage |
Scoped rejection still says the selected Codex account is cooling down / clear cooldown or switch accounts, with no scope/model hint. Misleading when Terra/Luna on the same account remain usable. |
What this PR does get right for #590: Spark reset-derived no longer becomes account-wide, so same-account combo failover Spark → Terra/Luna can proceed. core.ts / compact wiring of modelId + probeQuotaScope looks complete. Account-wide Retry-After / default paths still rotate.
Required before merge: make same-scope multi-account selection/failover aware of scoped cooldowns (Codex P2), with a focused regression. Prefer scope-aware eligibility over blindly rotating activeCodexAccountId on every Spark-only cooldown (that would fight the intentional “keep account usable for other scopes” design). At minimum, shared-scope reset-derived should get the existing affinity-clear + active rotation (Rabbit). Copilot message polish can land with that or as a tiny follow-up.
Security
No medium+ issues on the cooldown/auth path:
- Account-wide throttles still win over scoped state
- Scoped probes stay account+scope+generation bound
- No new credential/token logging
- Same-owner pool behavior only; client
modelonly picks which local quota bucket is checked
Summary
Ship-blocker is the scoped early-return vs pool rotation gap (Codex + Rabbit + Bugbot agree). #590’s single-account combo case is largely fixed; multi-account same-scope Spark/shared still fail-closed locally. Re-request review after that fix (+ CI approval/green).
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 306-308: Update the buffered 499 branch in the response handling
flow to call recordCodexUpstreamOutcome with status 499 before returning
buffered, ensuring any admitted Spark/shared recovery probe lease is released
while preserving the existing response.
In `@tests/codex-auth-context.test.ts`:
- Around line 569-578: Update the test for cooldownErrorMessage(err) to verify
the complete model-scoped message contract: retain the expected Spark quota
wording and assert that account-wide blocking language is absent, ensuring the
CodexAccountCooldownError does not present as an account-wide cooldown.
🪄 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: 79cbfdee-5a51-42ae-8d77-8c95e588aa4f
📒 Files selected for processing (5)
src/codex/auth-context.tssrc/codex/routing.tssrc/server/responses/compact.tstests/codex-auth-context.test.tstests/codex-routing.test.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/codex/routing.ts (1)
627-758: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftThread quota scope through Codex account selection
src/codex/routing.ts:722-758andsrc/codex/routing.ts:973-1058still pick/bind accounts with scope-blind cooldown checks. That can leave thread affinity andactiveCodexAccountIdpointing at an account that is reset-cooled for the current model scope; the later auth-context swap hides the failure from the request, but the routing state remains stale. PassquotaScopeinto the selection helpers and makeisCodexAccountSelectable/isCodexAccountInCooldownconsultquotaScopedHealthwhen the scope is known.🤖 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 `@src/codex/routing.ts` around lines 627 - 758, Thread quotaScope through pickUnboundStrategyAccount, pickFillFirstCodexAccount, pickNextFillFirstCodexAccount, and the account-selection/binding flow around the later routing helpers. When a scope is provided, make isCodexAccountSelectable and isCodexAccountInCooldown evaluate quotaScopedHealth for that scope, so selections and active/thread affinity never retain reset-cooled accounts; preserve existing behavior when no scope is known.
🤖 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.
Outside diff comments:
In `@src/codex/routing.ts`:
- Around line 627-758: Thread quotaScope through pickUnboundStrategyAccount,
pickFillFirstCodexAccount, pickNextFillFirstCodexAccount, and the
account-selection/binding flow around the later routing helpers. When a scope is
provided, make isCodexAccountSelectable and isCodexAccountInCooldown evaluate
quotaScopedHealth for that scope, so selections and active/thread affinity never
retain reset-cooled accounts; preserve existing behavior when no scope is known.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: d0025fe6-a9ca-4029-aca2-da7dff59d7c8
📒 Files selected for processing (5)
docs-site/src/content/docs/guides/providers.mdsrc/codex/auth-context.tssrc/codex/routing.tssrc/server/responses/core.tstests/codex-pool-rotation.test.ts
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/codex/routing.ts`:
- Around line 612-616: Update pruneLruThreadAffinities() and its callers to
avoid scanning every thread on each bind: maintain a running affinity-entry
counter, incrementing only when bindThreadAffinity() adds a new scope key and
decrementing in every deletion/pruning path. Use this counter for capacity
checks and eviction bookkeeping, removing reliance on threadAffinityEntryCount()
or ensuring it is not recomputed during normal binds.
In `@tests/codex-routing.test.ts`:
- Around line 350-356: Update the assertion in the scoped cooldown test around
resolveCodexAccountForThread to verify the effective active account via
getEffectiveActiveCodexAccountId(config), rather than reading
config.activeCodexAccountId directly. Preserve the expected value of "a" and the
existing Spark/shared routing assertions.
In `@tests/responses-compaction-routing.test.ts`:
- Around line 221-245: Ensure both compaction cancellation tests place all
global-state setup inside their existing try blocks: for
tests/responses-compaction-routing.test.ts lines 221-245 and 288-309, move the
environment assignments, Date.now stubs, clearCodexUpstreamHealth,
saveCodexAccountCredential, recordCodexUpstreamOutcome, and globalThis.fetch
stub so the corresponding finally teardown always executes.
🪄 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: fa3113e5-32fa-4d3a-94b1-34108134a5d8
📒 Files selected for processing (7)
src/codex/auth-context.tssrc/codex/routing.tssrc/server/responses/compact.tstests/codex-auth-context.test.tstests/codex-pool-rotation.test.tstests/codex-routing.test.tstests/responses-compaction-routing.test.ts
|
Follow-up review update
Validation completed:
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
src/codex/routing.ts:515
isCodexAccountInCooldownonly checks the account-wide cooldown and therefore ignores model-scoped reset-derived cooldowns. There is at least one runtime call site that gates model selection on this helper (src/codex/subagent-model-fallback.ts:192), which means a Spark-scoped cooldown could be missed and the model may continue to be selected despite an active scoped cooldown. To avoid regressions, callers that are deciding availability for a specific model should usegetCodexQuotaHealthSnapshot(accountId, codexQuotaScopeForModel(modelId), now)and the matching scoped probe-lease functions.
export function isCodexAccountInCooldown(accountId: string, now = Date.now()): boolean {
return getCodexAccountCooldownUntil(accountId, now) !== null;
}
src/codex/routing.ts:484
- Reset-derived native cooldowns (including the shared native quota group) are now stored in
quotaScopedHealth, but the account-level snapshot helpers (getCodexAccountCooldownUntil/getCodexAccountHealthSnapshot) only read fromupstreamHealth. That can make shared native cooldowns invisible to status surfaces that still rely on the account snapshot (e.g. OAuth health projection), even though routing correctly blocks viagetCodexQuotaHealthSnapshot.
};
}
e57aa39 to
dbf7cde
Compare
dbf7cde to
ada5219
Compare
Maintainer review (bug + security)Verdict: APPROVE (merge after full CI on a trusted run / maintainer push if fork CI is incomplete) SecurityNo medium+ issues in the Spark/shared quota scoping surface. Account identifiers in cooldown messages stay masked; no new credential exposure. Bug reviewScoped cooldown + affinity retention looks coherent. Focused coverage includes shared-scope rotation ( Open Codex / CodeRabbit threads
Notes
|
|
Will get merged after interal code work lands on dev. |
Transition workflow completed
|
|
Go port tracking issue: #663 |
Summary
gpt-5.3-codex-sparkwithin Spark's confirmed independent quota scope.Retry-Afteror default cooldowns.Closes #590.
Verification
bun run testbun run typecheckbun run privacy:scancd docs-site && bun run buildTest Coverage
Pre-Landing Review
No issues found. The diff was reviewed for quota-state isolation, auth propagation, and compact endpoint behavior.
Scope Drift
Scope Check: CLEAN — implementation is limited to the accepted cooldown-scoping behavior, its tests, and related documentation.
Plan Completion
No plan file detected.
Checklist
Summary by CodeRabbit
429handling) while keepingRetry-Afteraccount-wide./responses/compactnow enforces a hard payload size limit (32MB) and returns clearer errors for oversized vs aborted requests; outcome recording now includesmodelId,probeQuotaScope, and computedresetAt.