feat(rotation): add rotationStrategy selector and codex-warm tool (#183, #182)#184
Conversation
Adds two account-management features plus a related availability fix. #183 — rotationStrategy config (env CODEX_AUTH_ROTATION_STRATEGY): - hybrid (default, unchanged): stick to the current account while healthy, else score-select (health+tokens+freshness, which spreads load). - sticky: drain-first — stay on one account until it is rate-limited/ cooling down, then pick the lowest-indexed available account so load concentrates and the other accounts keep their quota windows in reserve. Directly addresses weekly-quota cooldowns landing on every account at once. - round-robin: advance through accounts in order. Wired through the request hot path via AccountManager.getAccountForStrategy. #182 — codex-warm tool: - Sends one minimal billable POST /codex/responses per enabled account so the rolling ~5h usage window actually starts at session start, instead of only when rotation lands on each account. (A read-only GET /wham/usage does NOT open the window, so warming must send a real inference request.) - Pure orchestrator (lib/accounts/warm.ts) + isolated network adapter (lib/accounts/warm-request.ts); disabled accounts skipped, per-account failures reported without aborting the batch. Complements codex-switch. fix(rotation): rotate off an account whose LOCAL token bucket is depleted. Previously the request loop drained the bucket and continue'd, but sticky/ hybrid re-pinned the same account (their availability check ignores the local bucket); the attempted-set guard then 503'd while other accounts had quota. Now a short auto-expiring rate-limit window (LOCAL_TOKEN_DEPLETION_COOLDOWN_MS) is applied so every strategy advances and getMinWaitTimeForFamily can wait. Also: sync .codex-plugin/plugin.json version (was drifted 6.3.3 vs 6.3.4) and add release-please extra-files so plugin.json tracks package.json automatically. Tests: 41 feature tests + 19 adversarial chaos stress tests; doc-parity and plugin-config snapshots updated for the new tool and config key. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (27)
Cache: Disabled due to data retention organization setting Knowledge base: Disabled due to data retention organization setting 📝 WalkthroughWalkthroughAdds two features: (1) a configurable ChangesRotation Strategy Configuration
codex-warm Tool
Docs, Parity Tests, and Release Config
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| export function createWarmOne( | ||
| storage: AccountStorageV3, | ||
| ): (account: AccountMetadataV3) => Promise<WarmOutcome> { | ||
| return async (account: AccountMetadataV3): Promise<WarmOutcome> => { | ||
| const { accessToken } = await ensureCodexUsageAccessToken({ storage, account }); | ||
| const accountId = resolveCodexUsageAccountId({ account, accessToken }); | ||
| if (!accountId) { | ||
| return { | ||
| status: "failed", | ||
| detail: "could not resolve account id (re-login may be required)", | ||
| }; | ||
| } | ||
| await warmAccountWindow({ | ||
| accountId, |
There was a problem hiding this comment.
persistRefreshedCredentials
warmAccounts defaults to concurrent: true, so createWarmOne is invoked via Promise.all for every enabled account simultaneously. when any of those accounts has a near-expiry token, ensureCodexUsageAccessToken proceeds past the early-return guard and eventually calls persistRefreshedCredentials, which presumably does a read-modify-write of the shared storage file. two simultaneous refreshes for two different accounts can interleave as: (1) A reads file, (2) B reads file, (3) A writes — account 0 refreshed, account 1 stale, (4) B writes — account 1 refreshed, but the account 0 refresh from step 3 is now overwritten. net effect: one account's new token is silently lost and the next request for it re-enters the refresh path or fails auth. does persistRefreshedCredentials do an atomic in-place update per account, or does it read the full storage file and overwrite? if the latter, concurrent warm calls for multiple expiring accounts will race.
Prompt To Fix With AI
This is a comment left during a code review.
Path: lib/tools/codex-warm.ts
Line: 44-57
Comment:
**concurrent token-refresh race on `persistRefreshedCredentials`**
`warmAccounts` defaults to `concurrent: true`, so `createWarmOne` is invoked via `Promise.all` for every enabled account simultaneously. when any of those accounts has a near-expiry token, `ensureCodexUsageAccessToken` proceeds past the early-return guard and eventually calls `persistRefreshedCredentials`, which presumably does a read-modify-write of the shared storage file. two simultaneous refreshes for two different accounts can interleave as: (1) A reads file, (2) B reads file, (3) A writes — account 0 refreshed, account 1 stale, (4) B writes — account 1 refreshed, but the account 0 refresh from step 3 is now overwritten. net effect: one account's new token is silently lost and the next request for it re-enters the refresh path or fails auth. does `persistRefreshedCredentials` do an atomic in-place update per account, or does it read the full storage file and overwrite? if the latter, concurrent warm calls for multiple expiring accounts will race.
How can I resolve this? If you propose a fix, please make it concise.
| import { describe, it, expect, vi, beforeEach } from "vitest"; | ||
|
|
||
| vi.mock("../lib/prompts/codex.js", async (importOriginal) => { | ||
| const actual = await importOriginal<typeof import("../lib/prompts/codex.js")>(); | ||
| return { | ||
| ...actual, | ||
| getCodexInstructions: vi.fn(async () => "test-instructions"), | ||
| }; | ||
| }); | ||
|
|
||
| import { | ||
| buildWarmRequestBody, | ||
| warmAccountWindow, | ||
| } from "../lib/accounts/warm-request.js"; | ||
| import { CODEX_BASE_URL } from "../lib/constants.js"; | ||
|
|
||
| function fakeResponse(status: number): Response { | ||
| return { | ||
| ok: status >= 200 && status < 300, | ||
| status, |
There was a problem hiding this comment.
warmAccountWindow exposes timeoutMs specifically to make the abort path testable, but no test exercises it. the AbortController.abort() callback inside setTimeout is completely untested — a broken timeout (e.g. clearTimeout accidentally before the request, or signal never wired to fetch) would go undetected. add a test that passes a very short timeoutMs with a fetchImpl that never resolves, and asserts that the call rejects with an abort error.
Prompt To Fix With AI
This is a comment left during a code review.
Path: test/accounts-warm-request.test.ts
Line: 1-20
Comment:
**missing vitest coverage for the timeout/abort path**
`warmAccountWindow` exposes `timeoutMs` specifically to make the abort path testable, but no test exercises it. the `AbortController.abort()` callback inside `setTimeout` is completely untested — a broken timeout (e.g. `clearTimeout` accidentally before the request, or signal never wired to fetch) would go undetected. add a test that passes a very short `timeoutMs` with a `fetchImpl` that never resolves, and asserts that the call rejects with an abort error.
How can I resolve this? If you propose a fix, please make it concise.| const log = createLogger("warm-request"); | ||
|
|
||
| /** Model used for the warm ping. Mirrors the live quota-probe default. */ | ||
| const WARM_MODEL = "gpt-5.4"; |
There was a problem hiding this comment.
WARM_MODEL will silently fail for accounts without gpt-5.4 access
the warm ping is always sent as gpt-5.4. an account that only has access to a different codex model (or is on a plan that doesn't include gpt-5.4) will receive a 4xx, which warmAccountWindow throws on, and warmAccounts records as failed. the account's window is never opened, but the user sees "Failed - HTTP 403" without any signal that the model is wrong rather than the account. since getCodexInstructions(model) already accepts a model parameter, consider at minimum exposing warmModel as a configurable parameter.
Prompt To Fix With AI
This is a comment left during a code review.
Path: lib/accounts/warm-request.ts
Line: 26
Comment:
**hardcoded `WARM_MODEL` will silently fail for accounts without gpt-5.4 access**
the warm ping is always sent as `gpt-5.4`. an account that only has access to a different codex model (or is on a plan that doesn't include gpt-5.4) will receive a 4xx, which `warmAccountWindow` throws on, and `warmAccounts` records as `failed`. the account's window is never opened, but the user sees "Failed - HTTP 403" without any signal that the model is wrong rather than the account. since `getCodexInstructions(model)` already accepts a model parameter, consider at minimum exposing `warmModel` as a configurable parameter.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
1 participant
Summary
Implements two requested account-management features plus an availability fix surfaced during adversarial review.
#183 —
rotationStrategyconfig (envCODEX_AUTH_ROTATION_STRATEGY)hybrid(default, unchanged): stick to the current account while healthy, else score-select (health + tokens + freshness, which spreads load).sticky: drain-first — stay on one account until it's rate-limited/cooling down, then pick the lowest-indexed available account so load concentrates and the other accounts keep their quota windows in reserve. This is the direct fix for the issue's "all accounts share an initiation time → all hit weekly cooldown together" problem under round-robin.round-robin: advance through accounts in order.Routed through the request hot path via a new
AccountManager.getAccountForStrategydispatcher. Defaulthybridis behavior-identical to the prior code → zero regression.#182 —
codex-warmtoolSends one minimal billable
POST /codex/responsesper enabled account so each account's rolling ~5h usage window actually starts at session start, instead of only when rotation eventually lands on it ("send a hi to each account to open all windows").Architecture: pure orchestrator (
lib/accounts/warm.ts, fully unit-tested) + isolated network adapter (lib/accounts/warm-request.ts). Disabled accounts are skipped; per-account failures are reported without aborting the batch. Complements the existingcodex-switch index=N(which already switches the active account on demand).fix(rotation): local token-bucket depletion no longer strands sticky/hybrid
Adversarial review found that when an account's local client-side token bucket is depleted, the request loop drained it and
continued — butsticky/hybridre-pinned the same account (their availability check ignores the local bucket), so theattempted-set guard broke the loop and returned a spurious 503 while other accounts still had quota. Now a short auto-expiring rate-limit window (LOCAL_TOKEN_DEPLETION_COOLDOWN_MS) is applied on depletion so every strategy advances, andgetMinWaitTimeForFamilycan engage the wait path when the whole pool is depleted.Release hygiene
.codex-plugin/plugin.jsonversion (was drifted6.3.3vspackage.json6.3.4), fixing a pre-existingdoc-parityfailure onmain.extra-filessoplugin.jsontrackspackage.jsonautomatically on future releases.Testing
/responsesadapter with injected fetch covering 2xx/429/4xx/5xx/network/timeout).doc-parityandplugin-configsnapshots updated for the new tool (22 tools) and config key.Closes #183
Closes #182
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
codex-warmcommand to warm up all configured accounts and report per-account results.hybrid,sticky, andround-robinbehavior, configurable via settings or environment variable.Bug Fixes
Chores
6.3.4and refreshed related documentation/version tracking.note: greptile review for oc-chatgpt-multi-auth. cite files like
lib/foo.ts:123. confirm regression tests + windows concurrency/token redaction coverage.Greptile Summary
adds
rotationStrategyconfig (hybrid/sticky/round-robin) for account load-balancing, thecodex-warmtool that primes every account's usage window at session start, and a local-token-depletion fix that prevents sticky/hybrid from re-pinning the same depleted account and returning a spurious 503.getAccountForStrategydispatcher routes to sticky (drain-first, lowest-indexed fallback), round-robin, or the pre-existing hybrid; default is hybrid so existing behavior is unchanged; env overrideCODEX_AUTH_ROTATION_STRATEGYwins over config; chaos tests cover 200-account fleets, mid-rotation removal/reindex, strategy-switching mid-session, and simulated restarts.warm.ts) + isolated network adapter (warm-request.ts) send one minimalPOST /codex/responsesper enabled account concurrently; failures are caught per-account and never abort the batch; 429 is treated as a successful warm since the window is already ticking.markRateLimitedWithReasonwithLOCAL_TOKEN_DEPLETION_COOLDOWN_MS(10 s) is applied when the local token bucket is empty, making the account ineligible inisRateLimitedForFamilyso every rotation strategy advances instead of looping on the same account until theattemptedguard fires a 503.Confidence Score: 3/5
the rotation selector and depletion fix are safe to merge; the warm tool's concurrent token-refresh path can silently lose a refreshed credential when two accounts' tokens expire simultaneously.
the rotation strategy wiring, sticky selector, and depletion fix are well-designed and backed by strong chaos tests. the warm tool's default concurrent mode runs ensureCodexUsageAccessToken in parallel for all accounts — if two accounts hit the refresh branch at the same time, their concurrent persistRefreshedCredentials writes can race at the file level, leaving one account's new token silently overwritten. this is a real failure mode when users add all accounts at the same session and run codex-warm immediately (all tokens expire together). the hardcoded WARM_MODEL and missing timeout test are lower-stakes but worth fixing.
lib/tools/codex-warm.ts (concurrent refresh race) and test/accounts-warm-request.test.ts (timeout path not exercised)
Important Files Changed
Sequence Diagram
%%{init: {'theme': 'neutral'}}%% sequenceDiagram participant User participant CodexWarmTool participant warmAccounts participant createWarmOne participant ensureCodexUsageAccessToken participant warmAccountWindow participant Upstream User->>CodexWarmTool: execute codex-warm CodexWarmTool->>warmAccounts: "accounts[], warmOne (concurrent=true)" par for each enabled account warmAccounts->>createWarmOne: warmOne(account, index) createWarmOne->>ensureCodexUsageAccessToken: refresh token if near expiry Note over ensureCodexUsageAccessToken: concurrent refreshes can race on persistRefreshedCredentials ensureCodexUsageAccessToken-->>createWarmOne: accessToken createWarmOne->>warmAccountWindow: POST /codex/responses (gpt-5.4) warmAccountWindow->>Upstream: "minimal billable ping (store=false)" Upstream-->>warmAccountWindow: "2xx / 429 = warmed, other = throws" warmAccountWindow-->>createWarmOne: true createWarmOne-->>warmAccounts: status warmed end warmAccounts-->>CodexWarmTool: WarmSummary CodexWarmTool-->>User: N warmed, M failed, K skipped%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%% sequenceDiagram participant User participant CodexWarmTool participant warmAccounts participant createWarmOne participant ensureCodexUsageAccessToken participant warmAccountWindow participant Upstream User->>CodexWarmTool: execute codex-warm CodexWarmTool->>warmAccounts: "accounts[], warmOne (concurrent=true)" par for each enabled account warmAccounts->>createWarmOne: warmOne(account, index) createWarmOne->>ensureCodexUsageAccessToken: refresh token if near expiry Note over ensureCodexUsageAccessToken: concurrent refreshes can race on persistRefreshedCredentials ensureCodexUsageAccessToken-->>createWarmOne: accessToken createWarmOne->>warmAccountWindow: POST /codex/responses (gpt-5.4) warmAccountWindow->>Upstream: "minimal billable ping (store=false)" Upstream-->>warmAccountWindow: "2xx / 429 = warmed, other = throws" warmAccountWindow-->>createWarmOne: true createWarmOne-->>warmAccounts: status warmed end warmAccounts-->>CodexWarmTool: WarmSummary CodexWarmTool-->>User: N warmed, M failed, K skippedPrompt To Fix All With AI
Reviews (1): Last reviewed commit: "feat(rotation): add rotationStrategy sel..." | Re-trigger Greptile