Repository navigation
fix(codex): answer the main hard lock as a usage limit, clamp retry to pool recovery (#7046) - #7076
Conversation
…y to pool recovery (#7046)
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
|
✅ Deterministic PR hygiene checks passed. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 34d611269e
ℹ️ 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".
There was a problem hiding this comment.
🔇 Additional comments (12)
src/codex/routing/pool-recovery.ts (1)
45-60: Pool recovery estimate can return a time earlier than a still-active hold.Lines 46-50 and 51-52 collect holds only when they are live (
> now). Line 53 collects the usage-limit reset only whenisCodexAccountHeldAtUsageLimitis true. Line 59 then takesMath.max(...holds). These sources are consistent with each other, so the computed maximum is correct for the holds that were detected.One gap remains. Line 53 depends on
isCodexAccountHeldAtUsageLimit, which uses the credit policy. If an account has only a transientsoftAvoidUntilhold (or a probe/transient hold), nothing is added toholds. Line 58 then skips the account. This is a safe, conservative result and matches the documented policy. No defect.The module header documents the intended semantics, and the tests cover cooldown, avoidance, denial, reauth, and pause. The logic matches the documented contract.
tests/codex-integration/main-account-hard-lock-usage-limit.test.ts (1)
189-199: Test compares an absolute timestamp captured after the call setup, which can flake.Line 192 captures
nowafterlockMain(). Line 193 sets the cooldown from thatnow. Line 194 callsresolveCodexAuthContext, which reads its ownDate.now()at line 1095 ofsrc/codex/auth-context.ts. The expected value on line 196 isnow + 2 * 3_600_000. That value matches the storedcooldownUntilexactly, so the equality holds.Line 198 is the weaker assertion.
cooldownErrorResponse(..., now)uses the capturednow. The expectedretry-afteris2 * 3_600. This is deterministic becausenowis the same value used to set the cooldown. No flake risk.No defect found.
src/codex/routing/health-store.ts (1)
166-166: LGTM!src/codex/auth-context.ts (3)
944-959: LGTM!
967-974: LGTM!
1091-1093: 🎯 Functional CorrectnessThe request-scoped main credential remains rejected while main is hard-locked, but
poolRecoveryAtis only a retry hint. A later pool retry can select an eligible sibling account. The code and tests require the main reset to be preserved for an exact main pin, not for every request-scoped credential. No defect remains.src/server/index/websocket-handler.ts (1)
373-379: Theawait refusal.json()call can throw and fall through to the generic error handling.Line 376 awaits
refusal.json()inside the existingtryblock. If it throws, the outercatchat line 391 swallows it. No frame is sent, and the client gets no refusal. The body comes fromformatUsageLimitReachedResponse, which always produces valid JSON, so this is unlikely in practice.The log was finalized with status 429 on line 367, so logging stays consistent. No defect.
structure/providers/openai-accounts.md (1)
544-556: LGTM!scripts/test-layout/layout.json (1)
1175-1175: LGTM!tests/fixtures/test-layout-expected.json (1)
751-751: LGTM!docs-site/src/content/docs/reference/codex-retry-diagnostics.md (1)
44-49: 📐 Maintainability & Code QualityNo locale contradiction is established.
The inspected locale trees do not contain a translated
codex-retry-diagnostics.mdpage. Therefore, this English-only documentation change does not leave an existing locale page describing the old behavior. The proposed locale update is not actionable for this file.src/codex/auth-context.ts-1094-1099 (1)
1094-1099: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
⚠️ Unverified finding
Verification ran but could not confirm this finding. It is shown for review, not as a verified issue.Swallowed errors hide failures of the recovery lookup.
The empty
catchon line 1099 discards every error fromnextCodexPoolRecoveryAtandcachedDeniedCodexAccountIdsForModel. The comment states the hint is optional, so swallowing is acceptable for the response. A systematic bug in the helper would then be invisible, because no log records the failure.Add a debug-level log in the catch block. Do not log credentials or account identifiers beyond what the codebase already masks.
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository: lidge-jun/opencodex/.coderabbit.yaml
- Review profile: ASSERTIVE
- Plan: Advanced
- Run ID:
b07e2789-039b-4510-8eeb-d2c705628988
📒 Files selected for processing (9)
docs-site/src/content/docs/reference/codex-retry-diagnostics.mdscripts/test-layout/layout.jsonsrc/codex/auth-context.tssrc/codex/routing/health-store.tssrc/codex/routing/pool-recovery.tssrc/server/index/websocket-handler.tsstructure/providers/openai-accounts.mdtests/codex-integration/main-account-hard-lock-usage-limit.test.tstests/fixtures/test-layout-expected.json
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 1 remain after this review.
|
Maintainer integration into Head commit: |
Summary
The main-account quota guard (
codexMainAccountHardLock) refused locally with429 rate_limit_error/rate_limit_exceededand aRetry-Afterpointing at main's weekly reset. Codex treats that as transient: in the reported incident it scheduled a retry 5 days out (Retry-After: 446329) and kept the Goal active for hours, even though a sibling pool account had recovered about 1.5 hours after the refusal.After this change:
429witherror.type=error.code=usage_limit_reached, the same policy message, andresets_at(epoch seconds), with noplan_type. Codex treats this as final: the turn fails and the Goal becomesusageLimited(the issue's isolated app-server run showed exactly this transition). It uses the same body shape as the Anthropic pool fix (fix(anthropic-pool): answer spent quota as a usage limit and lift Retry-After quota cooldowns on fresh reads #6995), viaformatUsageLimitReachedResponse. HTTP (cooldownErrorResponse) and the Codex WebSocket error frame both use it.Retry-Afterandresets_atare clamped to the next account-pool recovery time when an unpinned pool request fell through to the locked main and another stored pool account is known to recover sooner. Both name the same instant. When nothing is known, both are omitted as before.rate_limit_errorcontract byte for byte.Before → after for the incident:
rate_limit_exceeded,Retry-After: 446329, Goal active →usage_limit_reached,Retry-After/resets_at≈ the sibling's recovery (~1.5 h after the refusal, about 14:04 local in the issue's timeline), GoalusageLimited.Pool recovery policy (decided in this PR; Jun delegated the call)
nextCodexPoolRecoveryAt(src/codex/routing/pool-recovery.ts) is deliberately conservative. It only predicts a retry time; it selects nothing and changes no state:I chose these rules to keep the existing safety boundaries (eligibility, pins, replay safety, credits consent, the main guard). The fix gives a truthful terminal state plus the earliest honest "try again at" time. Automatic Goal resumption stays a client concern.
Closes #7046
Verification
bun test tests/codex-integration/main-account-hard-lock-usage-limit.test.ts(new, 8 tests): the incident's exact446329deadline →usage_limit_reached+ matchingresets_at; missing/past/NaN/∞ reset → neither header nor field; earlier pool recovery clamps both, later does not; 60 s floor; past hint ignored; pool hint alone when main's reset is unknown; pool cooldown and credits-off keeprate_limit_error; estimator rules (latest hold wins incl. soft avoid, denied/needs-reauth/paused/out-of-roster excluded, no-hold → none); a gated model (Daybreak) takes the hint only from a roster-admitted sibling; end-to-endresolveCodexAuthContextin pool mode carries the sibling's recovery, and a main-pinned request does not.tests/codex-integration/main-account-*.test.ts,codex-credits-after-limit-main,reserve-auth-context, every test file referencingcooldownErrorResponse/ the hard lock /websocket-handler, file-size ratchet, test-layout guards, and every test that readsauth-context.ts,routing/health-store.tsorwebsocket-handler.tsas source (incl.codex-model-entitlements): pass.bun run typecheck,bun run structure:check,bun run privacy:scan: pass.bun run test/test:changed.auth-context.tspulls in nearly the whole suite, and the shared Bun test lock was held by other worktrees on this machine. The ubuntu test shards on this PR cover it.Checklist
structure/providers/openai-accounts.md,docs-site/.../reference/codex-retry-diagnostics.md)