stack 5/5: probe reset-derived cooldowns without waiting to be selected (#915) - #955
Conversation
…ted (#915) A reset-derived cooldown is a prediction, and OpenAI can reset earlier than predicted. The probe that would notice was unreachable: cooled accounts are filtered out before every strategy runs, and resolveCodexAuthContext() selects an account first and only then checks that account's lease. With another account eligible, the cooled one was never selected, never probed, and never recovered. Adds a background claim/settle pair that enumerates accounts independently of selection, on the state sweeper's existing tick rather than a new timer. Claims require reset-derived exactly — a default cooldown is the 60s headerless fallback, so there is no prediction to be early against — and are ordered oldest-first so a bounded pass cannot starve later accounts. Settle clears only the exact scope entry, and only while the lease, cooldown generation, and credential generation all still match; any mismatch releases the lease and retains the cooldown. Recovery does not route through recordCodexUpstreamOutcome(200), which would also mutate account-wide failure state, and never uses clearCodexAccountCooldown(), which clears every scope. Spark is skipped at the claim site: WHAM takes no scope parameter, so a generic result can never prove a spark recovery. Claiming it would spend the account's one claim per pass to settle false and leave the shared scope cooled behind it. The main account is excluded from this first cut — it has no WHAM single-flight, so a sweeper probe could race a dashboard refresh into parallel requests. Fixes #915
… load-bearing The recovery predicate assumed weekly-window semantics for every plan except go/free, so an unfamiliar plan_type with a finite weekly percentage cleared the cooldown on evidence we could not interpret — routing traffic to an account that may still be restricted in a window we never read. Recognized plans are now enumerated and anything else retains the cooldown, which only costs a delay since it expires on its own. Three tests also passed vacuously. Both lease-release cases asserted only that the cooldown survived, never that the lease came back — a stranded lease means that account is never probed again, which is worse than the bug. They now require a later pass to reach WHAM and clear it. The fairness test spaced its passes 60s apart, inside the 5-minute probe interval, so already-probed accounts dropped out on their own and the ordering was never exercised; it now spaces past the interval so served and unserved accounts genuinely compete, and it fails when the oldest-first sort is removed.
The hand-written weekly set missed prolite, which the upstream model snapshot enumerates alongside the other eight plans. A recovered prolite account would have failed the completeness check and stayed cooled — reintroducing the exact defect this change exists to fix, through an incomplete list rather than a wrong rule. The test now loops every weekly plan the snapshot names.
…list (#915) The allowlist was the wrong shape, not just incomplete. The upstream snapshot carries 21 distinct plan strings — edu_plus, finserv, k12, quorum, self_serve_business_usage_based and eight more — and CodexAccount.plan is an unrestricted string, so any hand-written list is a list of the plans someone remembered. Twelve real plans would have been refused recovery and stayed cooled forever: the exact defect this unit exists to fix, reintroduced as a typo-shaped hole. Adding prolite fixed one name and left eleven. Extracted codexQuotaWindowForPlan() as the single rule and routed parsing, exhaustion, and recovery through it, so the three cannot drift. Recovery still fails closed on missing EVIDENCE — credits-only and windowless payloads never clear a cooldown — which is the guard that actually protects a restricted account. Failing closed on an unfamiliar plan NAME only ever meant cooled forever. The test now derives its plan set from the snapshot instead of restating a list, so a newly added plan is covered without anyone remembering to update it.
… under test The data-derived loop computed its own expectation from codexQuotaWindowForPlan(), so it only proved the function agrees with itself — ablating the rule to always return weekly still passed 18/18. The contract is now stated in literals first (go/free monthly; plus, pro, prolite, team, business, enterprise, edu, finserv, k12, absent, empty weekly; free_workspace is not free), and that ablation now fails.
|
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 |
![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: 493329df01
ℹ️ 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 required = codexQuotaWindowForPlan(plan) === "monthly" | ||
| ? quota.monthlyPercent | ||
| : quota.weeklyPercent; |
There was a problem hiding this comment.
For Team and other non-Free/Go plans, WHAM can legitimately return only an explicitly monthly primary window; parseUsageQuota() then produces only monthlyPercent, as covered by tests/rate-limit-reset-credits.test.ts:121-132. This helper nevertheless always requires weeklyPercent for those plans, so every successful background probe is rejected and the reset-derived cooldown remains until its predicted expiry, potentially keeping a single-account pool unavailable for a long monthly window. Determine completeness from the window actually classified in the fresh response rather than from the plan name alone.
Useful? React with 👍 / 👎.
| && claim.credentialGeneration === proof.credentialGeneration | ||
| && isCodexAccountGenerationLive(claim.accountId, claim.credentialGeneration); |
There was a problem hiding this comment.
When a cooled account's access token is near expiry, getValidCodexToken() refreshes it and increments the credential generation before performing the WHAM request (src/codex/account-store.ts:415-421,467). The returned fresh quota is therefore proven under the new live generation, but this equality compares it with the pre-refresh claim generation and rejects the recovery, leaving the account cooled for another probe interval despite a successful reading. Preserve fencing against external credential replacement while recognizing a generation transition performed by this probe's own refresh.
Useful? React with 👍 / 👎.
1 participant
Stack
5/5 — cooldown early-recovery probe
Base:
codex/545-classifier-thinking-disabled(#954)Summary
A reset-derived cooldown is a prediction, and OpenAI can reset earlier than predicted. The account should come back — but the probe that would notice was unreachable.
The chain: cooled accounts are filtered out before every strategy runs (
src/codex/routing.ts:740-768), andresolveCodexAuthContext()selects an account first and only then checks that account's probe lease (src/codex/auth-context.ts:238-247,:276-298). So with account B eligible, A is never selected, never reaches the lease code, and never recovers. The recovery mechanism was not broken; it was unreachable.A forced WHAM read did not help either:
setAccountQuotaFromParsed()writes the quota cache and never touches routing state.claimDueCodexQuotaRecoveryProbes()/settleCodexQuotaRecoveryProbe()— enumerate accounts independently of selectionsrc/codex/auth-context.tsunchanged; request admission is untouchedDeliberate narrowings
reset-derivedonly. The existing request-path predicate also admitsdefault, but adefaultcooldown is the 60-second headerless fallback — there is no prediction to be early against.retry-afterstays authoritative.Oldest-first ordering. Bounded claims with stable config order starve: if more accounts are due than the per-pass limit and the early ones keep failing, they become due again and consume the budget forever. Claims sort by oldest
lastProbeAt ?? cooldownSince, so a probed account rotates to the back whether or not it recovered.Spark is never claimed.
GET /backend-api/wham/usagetakes no scope parameter and returns generic windows, so its result can never prove a spark recovery. Claiming it would spend the account's one claim per pass to settlefalseand leave the shared scope — which that evidence can clear — cooled behind it.Main account excluded from this first cut. Pool accounts have generation-aware single-flight;
fetchMainAccountInfo()has none, so a sweeper probe could race a dashboard refresh into parallel WHAM requests. Giving main the same admission semantics is its own change.Settle is narrow. It clears only the exact map entry, only while the lease, cooldown generation, and credential generation all still match; any mismatch releases the lease and retains the cooldown. It does not use
clearCodexAccountCooldown()(clears every scope) and does not route throughrecordCodexUpstreamOutcome(200)(also mutates account-wide failure state).What the audit changed
Five rounds, four FAIL. Every finding was reproduced before fixing.
plan_typewith a finite weekly percentage cleared the cooldown on evidence we could not interpret.prolite— then the snapshot turned out to carry 21 distinct plan strings (edu_plus,finserv,k12,quorum,self_serve_business_usage_based, …) with 12 unclassified, andCodexAccount.planis an unrestricted string. Every omission meant an account cooled forever: the defect this PR exists to fix, reintroduced as a typo-shaped hole. Replaced withcodexQuotaWindowForPlan(), the parser's own binary rule, now shared by parsing, exhaustion, and recovery so the three cannot drift.Verification
bun x tsc --noEmit— exit 0bun run test— 7729 pass, 8 skip, 0 fail, 508 filesbun run privacy:scan— passedclaimDueCodexQuotaRecoveryProbes()fails 5 tests; ablating the oldest-first sort fails the fairness test; ablating the window rule fails the plan matrix; ablating lease release fails both retention testsThe defining case: account A cooled reset-derived, B eligible, ordinary routing selects B — after the interval the worker probes A's credential and clears A's cooldown, with no routing call ever having selected A.
Fixes #915