stack 5/7: probe reset-derived cooldowns without waiting to be selected (#915) - #955
Merged
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.
📝 WalkthroughWalkthroughCodex adds bounded recovery probes for due reset-derived pool-account cooldowns. Probes use leases and generation fencing, validate plan-specific quota snapshots, run as a single-flight background worker, and clear only matching recovered cooldowns. Tests cover failure, scope, concurrency, credential, gating, and fairness cases. ChangesCodex cooldown recovery
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant StateSweep
participant AuthAPI
participant Routing
participant QuotaAPI
StateSweep->>AuthAPI: Run cooldown recovery worker
AuthAPI->>Routing: Claim due pool-account probes
Routing-->>AuthAPI: Return leased claims
AuthAPI->>QuotaAPI: Refresh quota concurrently
QuotaAPI-->>AuthAPI: Return quota snapshots
AuthAPI->>Routing: Settle each probe with recovery proof
Routing-->>AuthAPI: Clear matching cooldown or release lease
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
This was referenced Aug 3, 2026
Owner
Author
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".
Comment on lines
+106
to
+108
| 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 👍 / 👎.
Comment on lines
+511
to
+512
| && 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 👍 / 👎.
This was referenced Aug 4, 2026
Owner
Author
chrisae9
pushed a commit
to chrisae9/opencodex
that referenced
this pull request
Aug 4, 2026
Two defects share one symptom report, and only one of them is about stacked PRs. `ci.yml` filters `pull_request` on `branches: [main, dev]`, which GitHub matches against the BASE ref. A stacked child's base is another open PR's head, so the workflow is never queued: the lidge-jun#951-lidge-jun#955 stack carried 24 changed files under `src/` and 748 added lines with `enforce-target`, `label`, and `react-doctor` as its only check-runs. AGENTS.md calls stacked children an intentional review workflow and `enforce-target` already exempts them from the wrong-base gate, so the test gate is the one place that never got the memo. The labeler is a separate bug the stack happened to expose. `stack 1/5:` fails the conventional-commit regex — the `1/5` sits between the word and the colon — and the sentence-case fallback then extracts `stack`, which has no entry in PREFIX_TO_LABEL. The sync skips, a skip is not a failure, and the `label` check stays green while all four PRs carry no type label. The audit killed the first design. It chose `branches: [main, dev, "codex/**"]` on the theory that stacked bases live in the `codex/` namespace; listing real open PR head refs falsified that (codex/ 14, but also fix/ 4, feat/ 3, agent/ 3, split/, ingw/). Any of those can become a stacked base, and a contributor stack is precisely the case that most needs CI — the allowlist would have fixed the maintainer half and left the contributor half silently unverified. So the filter goes and the untouched `paths:` stays the scope gate, which is already how issue-quality-tests.yml is written. The audit also caught 020 asserting that the conventional regex matches `stack 1/5:`. It does not; both docs now name the sentence-case path. Docs only — no workflow or script changes in this commit.
chrisae9
pushed a commit
to chrisae9/opencodex
that referenced
this pull request
Aug 4, 2026
`ci.yml` filtered `pull_request` on `branches: [main, dev]`, and GitHub matches that against the BASE ref. A stacked child's base is another open PR's head branch, so the workflow was never queued: the lidge-jun#951-lidge-jun#955 stack merged with `enforce-target`, `label`, and `react-doctor` as its only check-runs, and no test job at all, for 24 changed files under `src/`. That contradicts the repository's own design. AGENTS.md calls stacked children an intentional review workflow, and `enforce-target` implements the exemption by listing open PRs and matching `other.head.ref === pr.base.ref`. The gate accepts them; the check workflow never saw them. The filter is removed rather than extended. An allowlist cannot express "base is another PR's head": open PR head refs today are `codex/` (14) but also `fix/` (4), `feat/` (3), `agent/` (3), `split/`, and `ingw/`, and any of them can become a stacked base. A `codex/**` glob — the first draft of this change — would have fixed the maintainer's own stacks and left contributor stacks silently unverified, which is the worse half of the bug. `paths:` is untouched and is the real scope gate, so docs-only and devlog-only PRs still queue nothing. The precedent is already in this repository: `issue-quality-tests.yml` runs `pull_request` with `paths:` and no `branches:`. Widening is safe here specifically because this workflow is `pull_request` (not `pull_request_target`), declares `contents: read`, and reads no secrets — the same change would not be safe on `enforce-pr-target.yml` or `pr-labeler.yml`, which are untouched. `push:` stays pinned to the integration lines. The existing "PR checks reach every branch the target gate accepts" test asserted the opposite contract, which is how this survived: it pinned both workflows to `["dev", "main"]` and asserted the trigger key set as `["branches", "paths"]`. Its INTENT is exactly this fix — commit 5229717 wrote it so "an accepted PR is also a checked one" — so it is extended to the gate's stacked exemption rather than deleted: `ci.yml` is now asserted to carry no base filter, `service-lifecycle.yml` keeps its list (it gates the release service path, not review), and the gate itself is pinned as base-unfiltered. The block's type annotation did not even model `branches` on the PR trigger, so the second assertion adds it and pins its absence. Driven red: restoring `branches: [main, dev]` fails exactly that one test. Verified: bun test tests/ci-workflows.test.ts 83 pass; typecheck and privacy:scan pass.
chrisae9
pushed a commit
to chrisae9/opencodex
that referenced
this pull request
Aug 4, 2026
All four PRs of the lidge-jun#951-lidge-jun#955 stack carried no type label while the `label` check reported success. `stack 1/5:` fails the conventional-commit regex — the `1/5` sits between the word and the colon — and then reaches the sentence-case fallback, which extracts `stack`. That has no entry in PREFIX_TO_LABEL, so `planTypeLabelSync` returns `{skip: true, reason: "no-prefix"}`, and a skip is not a failure. The check stays green and nothing is labeled. This is not a stacked-PR bug; the labeler has no branch filter and ran fine on all four. It is a title-vocabulary bug that the stack happened to expose: any title with an unrecognised prefix word is silently unlabeled. The commits underneath are conventional even when the title is not, so they answer what the title cannot. Adding `stack` to PREFIX_TO_LABEL was rejected — a stack PR can carry fixes, features, or docs, so any fixed mapping would be a lie. The unanimity rule this started with was falsified by running it on the real data. lidge-jun#952 gives `{bug: 1}` and labels, but lidge-jun#955 gives `{bug: 4, chore: 1}` and would abstain — four `fix(codex):` commits plus one `test(codex):`, which is a bug fix by any honest reading. A rule that abstains there abstains on most real PRs, since nearly every substantial change carries a test or chore commit. So `chore` is supporting, not competing: `test:`, `ci:`, `chore:`, `style:`, `refactor:`, and `build:` all map to it, and none of them says what a PR is FOR. It drops out of the tally when a non-chore type is present. An all-chore PR still gets `chore`, and a genuine `fix:`-plus-`feat:` mix is still left unlabeled rather than guessed. The title stays authoritative when it classifies, so a well-formed title is never overridden by what happens to be committed under it, and the existing human-override gate still runs first. No permission change: `pulls.listCommits` is covered by the existing `contents: read`. Both rules were driven red: removing the fallback fails the stack-PR test and nothing else; removing the chore-demotion fails the lidge-jun#955-shape test and nothing else. The docs now also state the promotion model, which was only a code comment before: `enforce-target` and `label` run on `pull_request_target` and are loaded from the default branch, so merging either to `dev` does not change live behavior until promotion to `main`. Verified: node --test .github/scripts/pr-labeler.test.cjs 24 pass; bun test tests/ci-workflows.test.ts 83 pass; typecheck and privacy:scan pass.
Owner
Author
Stack navigation — 7 layers, review and merge bottom-up
Each layer targets the branch below it, so its diff only makes sense on that base — Note for the merge sequence: retargeting a child after its parent merges emits an |
lidge-jun
changed the base branch from
codex/545-classifier-thinking-disabled
to
dev
August 4, 2026 04:18
# Conflicts: # src/codex/auth-api.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/auth-api.ts`:
- Around line 849-855: Record the boolean result returned by
settleCodexQuotaRecoveryProbe in both the per-claim catch path and the
worker-level failure path within the recovery worker. Add a minimal counter or
debug-level signal keyed only by the settle outcome, without including account
IDs, plans, or token material; preserve the existing best-effort error handling
and cooldown cleanup.
🪄 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: 83da6c8e-5ffe-4786-8d30-fb7a8b99aa8e
📒 Files selected for processing (5)
src/codex/auth-api.tssrc/codex/quota.tssrc/codex/routing.tssrc/server/index.tstests/codex-cooldown-recovery.test.ts
Comment on lines
+849
to
+855
| } catch { | ||
| settleCodexQuotaRecoveryProbe(claim, false, {}, now); | ||
| } | ||
| }); | ||
| })().catch(() => { | ||
| // Background recovery is best-effort; routing keeps the cooldown on failure. | ||
| }).finally(() => { cooldownRecoveryInFlight = null; }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Add a minimal failure signal to the recovery worker.
The worker swallows every failure. Line 849 catches per-claim errors, and line 853 catches worker-level errors. Neither path records anything. The worker runs on the 60-second sweep tick and issues upstream WHAM calls.
The failure mode is diagnostic, not functional. If an account stays cooled, an operator cannot distinguish these cases from outside:
- No claim was made, because the cooldown is
retry-afteror default-derived. - A claim was made and the WHAM call failed.
- A claim was made, WHAM succeeded, and the generation fence rejected the result.
That is the exact question issue #915 raises, so the fix for it should be observable. settleCodexQuotaRecoveryProbe already returns a boolean, and the return value is currently discarded at lines 846 and 850.
A counter or a debug-level line keyed on the settle outcome is enough. Do not include the account id, the plan, or any token material in the signal.
📈 Sketch: capture settle outcomes without logging credentials
cooldownRecoveryInFlight = (async () => {
const claims = claimDueCodexQuotaRecoveryProbes(config, POOL_QUOTA_REFRESH_CONCURRENCY, now);
+ let cleared = 0;
+ let retained = 0;
await mapWithConcurrency(claims, POOL_QUOTA_REFRESH_CONCURRENCY, async claim => {
const account = configuredPoolAccount(config, claim.accountId);
if (!account) {
settleCodexQuotaRecoveryProbe(claim, false, {}, now);
+ retained += 1;
return;
}
try {
const result = await fetchPoolAccountQuota(claim.accountId, true, account.plan);
const recovered = claim.scope !== "spark"
&& isCompleteCodexQuotaRecoverySnapshot(result.freshQuota ?? null, result.freshPlan ?? account.plan);
- settleCodexQuotaRecoveryProbe(claim, recovered, {
+ const settled = settleCodexQuotaRecoveryProbe(claim, recovered, {
credentialGeneration: result.freshCredentialGeneration,
}, now);
+ if (settled) cleared += 1; else retained += 1;
} catch {
settleCodexQuotaRecoveryProbe(claim, false, {}, now);
+ retained += 1;
}
});
+ recordCodexCooldownRecoveryPass({ claimed: claims.length, cleared, retained });
})().catch(() => {Do you want me to wire this into the existing metrics surface instead of a local counter?
🤖 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/auth-api.ts` around lines 849 - 855, Record the boolean result
returned by settleCodexQuotaRecoveryProbe in both the per-claim catch path and
the worker-level failure path within the recovery worker. Add a minimal counter
or debug-level signal keyed only by the settle outcome, without including
account IDs, plans, or token material; preserve the existing best-effort error
handling and cooldown cleanup.
2 tasks
7 tasks
lidge-jun
added a commit
that referenced
this pull request
Aug 4, 2026
stack 6/7: triage the overnight PRs and fix the #955 defects they found
chrisae9
pushed a commit
to chrisae9/opencodex
that referenced
this pull request
Aug 4, 2026
lidge-jun#967 found two real defects in my own lidge-jun#955 code and both verify at runtime: a Team account with a monthly window could never recover because the predicate picked its window by plan name while the parser picks by window duration, and the probe's own token refresh was mistaken for an external credential replacement. lidge-jun#963 and lidge-jun#965 both claim lidge-jun#962; lidge-jun#965 wins because it inherits from the row it actually replaces rather than recomputing config hints, and because lidge-jun#963 rewrites an existing regression contract to justify a broader change. lidge-jun#966 is a fifth design for lidge-jun#914 that survives two of the four prior falsifications but not all: mixed 5xx-then-rejection still loses the attributable failure, and five newly-classified sidecar paths keep default redirects, so a credential-visible 307 to a dead host still reads as neutral.
chrisae9
pushed a commit
to chrisae9/opencodex
that referenced
this pull request
Aug 4, 2026
…ned refresh generations Addresses the two unresolved Codex review threads on lidge-jun#955: - isCompleteCodexQuotaRecoverySnapshot() required weeklyPercent for every non-Go/Free plan by plan name, but the parser classifies windows by duration: a Team response with an explicitly monthly primary window parses to monthlyPercent only, so those accounts could never recover early and stayed cooled until their predicted expiry. - settleCodexQuotaRecoveryProbe() required the claim-time credential generation to match exactly. A probe-owned token refresh inside getValidCodexToken() advances the generation by one before WHAM completes, so a successful fresh reading was rejected and the account waited another probe interval. replacedAt is preserved by refresh and stamped by external replacement, so it fences the +1 transition. (cherry picked from commit 79d2164)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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
Summary by CodeRabbit
New Features
Bug Fixes