fix: carry Spark quota scope onto dev2-go (#599) - #662
Conversation
* Merge remote-tracking branch 'origin/dev' into fix/codex-spark-quota-scope-dev * fix: retain affinity per Codex quota scope * fix: address scoped affinity review feedback * fix: restore dev version and harden compact test (cherry picked from commit 20cdc0d)
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (2)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 75be2831c7
ℹ️ 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".
| An explicit `Retry-After` or an unclassified quota 429 is account-wide. A reset-derived native-model | ||
| 429 is advisory and remains within its confirmed quota group: `gpt-5.3-codex-spark` is separate from | ||
| the shared native group (including GPT-5.6 Terra/Luna). This allows a same-account combo to test an | ||
| independent quota without allowing fallbacks that share the exhausted quota. |
There was a problem hiding this comment.
The commit explicitly targets dev2-go, but the diff only changes the TypeScript runtime, tests, structure notes, and documentation; it does not touch go/, the native runtime entrypoint, or Go release-asset tooling. Repository policy requires such changes to target dev, so retarget this change rather than carrying an incomplete TS-only implementation onto the Go integration line.
AGENTS.md reference: AGENTS.md:L84-L89
Useful? React with 👍 / 👎.
| modelId: route.modelId, | ||
| probeLeaseId: codexProbeLeaseId(authCtx), | ||
| probeQuotaScope: codexProbeQuotaScope(authCtx), |
There was a problem hiding this comment.
When a failover combo has another eligible target on the same provider, deferCodexResetDerivedCooldown prevents this newly model-scoped outcome from being recorded, and the combo loop never records it later. For a Spark reset-derived 429 followed by a successful Terra fallback, only the combo target's default 60-second cooldown remains, so subsequent requests retry the known-exhausted Spark quota every minute instead of respecting its reset-derived cooldown and probe schedule. Record the scoped Spark outcome before advancing the combo; doing so no longer blocks the shared Terra/Luna target.
Useful? React with 👍 / 👎.
| if (quotaScope === "shared") { | ||
| clearThreadAccountMapForAccount(accountId); |
There was a problem hiding this comment.
If a thread already has a healthy Spark affinity to account A and a shared Terra/Luna request on A receives a reset-derived 429, this call removes every affinity for A, including the independent Spark binding. Because the same branch can then promote the shared active account to B, the thread's next Spark request may incorrectly move to B even though A's Spark quota remains healthy. Clear only the failed shared affinity here rather than all scoped affinities for the account.
Useful? React with 👍 / 👎.
| recordCodexUpstreamOutcome(config, authCtx.accountId, outcome, { | ||
| threadId, | ||
| modelId, | ||
| probeLeaseId: codexProbeLeaseId(authCtx), | ||
| probeQuotaScope: codexProbeQuotaScope(authCtx), |
There was a problem hiding this comment.
When an HTTP-successful streaming response ends with an SSE response.failed carrying a 429/402 status, this recorder receives the semantic status and model but not the response's Retry-After or x-codex-*-reset-at metadata captured earlier in quotaMeta. Consequently computeQuotaCooldown selects the default source, making a Spark terminal quota failure account-wide and blocking Terra/Luna instead of creating the intended scoped reset-derived cooldown. Pass the captured quota metadata into the terminal recorder.
Useful? React with 👍 / 👎.
| let authCtx: CodexAuthContext; | ||
| if (route.codexAccountMode) { | ||
| authCtx = await resolveCodexAuthContext(req.headers, config, route.codexAccountMode); | ||
| authCtx = await resolveCodexAuthContext(req.headers, config, route.codexAccountMode, { modelId: route.modelId }); |
There was a problem hiding this comment.
Final authentication is now model-scope-aware, but the earlier subagent account preview still calls previewCodexAccountForRequest without a quota scope. If account A has a Spark-scoped cooldown and account B can serve Spark, fallback selection is evaluated against A's account-specific model health even though this call will authenticate against B; that can unnecessarily rewrite Spark to a fallback model or consult the wrong failure state. Pass the primary routed model's quota scope into the preview so fallback selection and final authentication predict the same account.
Useful? React with 👍 / 👎.
| const account = getCodexAccountHealthSnapshot(accountId, now); | ||
| if (account) return account; | ||
| if (!quotaScope) return null; | ||
| const scoped = scopedHealthFor(accountId, quotaScope); |
There was a problem hiding this comment.
Model-less callers return here without consulting scoped health, but resolveFirstUsableOpenAiSidecar still resolves pool authentication without a model ID even though the web-search and vision sidecars use shared native models such as gpt-5.6-luna and gpt-5.4-mini. After account A receives a shared reset-derived cooldown, a routed request needing one of these sidecars can therefore select A and hit the exhausted shared quota instead of selecting an eligible account B. Thread the configured sidecar model through auth resolution and outcome recording so sidecars participate in the shared scope.
Useful? React with 👍 / 👎.
Summary
dev2-go(TS oracle + docs + tests).20cdc0d1ondev.Go port
Go has Codex routing/cooldown (
go/internal/codex/routing*.go,auth_context.go) but not the newCodexQuotaScope/ Spark-independent quota mapping from #599.Tracking issue will follow after this lands. This PR does not claim the Go side is done.