Add rotating account backup fallback recovery#29
Conversation
Keep three backup snapshots for account storage (.bak, .bak.1, .bak.2), rotate them on save, and scan backups in recovery order during load. Also clear historical backups in clearAccounts and add regression tests for historical backup restore and backup rotation behavior. Co-authored-by: Codex <noreply@openai.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughintroduces multi-slot rotating backups for account storage and robust windows-aware home-dir resolution. load tries multiple backup candidates and persists recovered payloads; saves rotate backups with retry and staging; clear removes all backup slots. windows path resolution gains explicit fallbacks. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
review notes
🚥 Pre-merge checks | ❌ 3❌ Failed checks (3 warnings)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
Prefer user-scoped environment variables before os.homedir() so account storage resolves consistently across users on Windows.\n\nAdd runtime-path tests covering USERPROFILE, HOME, and HOMEDRIVE/HOMEPATH fallback behavior. Co-authored-by: Codex <noreply@openai.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/storage.ts`:
- Around line 208-224: The rotating backup routine createRotatingAccountsBackup
mutates backup slots sequentially and is concurrency-sensitive—add a vitest
regression that fires parallel saveAccounts calls to reproduce races and assert
deterministic final ordering of candidates returned by
getAccountsBackupRecoveryCandidates (e.g., .bak, .bak.1, .bak.2). Fix
createRotatingAccountsBackup by serializing backup rotations (introduce a
per-path mutex/queue or use atomic temp-file writes + atomic renames for each
slot) and ensure copyFileWithRetry handles and retries transient EBUSY/429
errors; update logging in these paths to avoid leaking tokens/emails and ensure
the test asserts both ordering and that no sensitive data is logged. Ensure the
new test references createRotatingAccountsBackup,
getAccountsBackupRecoveryCandidates, copyFileWithRetry, and saveAccounts so
changes are covered.
In `@test/storage-recovery-paths.test.ts`:
- Around line 96-122: Add a regression test that verifies fallback to the third
backup file by extending the existing "falls back to historical backup snapshots
when the latest backup is unreadable" test: create broken JSON in storagePath
and storagePath+".bak" and storagePath+".bak.1", write a valid JSON payload to
storagePath+".bak.2" (mirroring the structure used for historicalBackupPayload),
call loadAccounts(), assert recovered.accounts contains the expected accountId
from the .bak.2 payload, and assert the persisted storagePath file was updated
to that payload; use the same helper variables (storagePath, loadAccounts) and
assertion patterns as the existing test to keep it deterministic under vitest.
In `@test/storage.test.ts`:
- Around line 1574-1605: The test only checks .bak and .bak.1 but must assert
full rotation to .bak.2 and also exercise the Windows EPERM copy-retry branch;
update the test that calls saveAccounts to perform one additional save (so three
rotations produce .bak, .bak.1, .bak.2) and assert refreshToken ordering: .bak
-> token-3, .bak.1 -> token-2, .bak.2 -> token-1. To hit the EPERM retry branch
in lib/storage.ts (the copy/rotation logic), mock fs.copyFile (using vitest
vi.spyOn or vi.stub) to throw an error with code "EPERM" the first time it's
called and succeed on subsequent calls, then restore the mock after the test so
you deterministically exercise the retry path. Ensure you reference the test
helper saveAccounts and the copy/rotation code in lib/storage.ts when locating
where to mock.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (3)
lib/storage.tstest/storage-recovery-paths.test.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 (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/storage.test.tstest/storage-recovery-paths.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/storage.ts
🧬 Code graph analysis (2)
test/storage.test.ts (1)
lib/storage.ts (3)
getStoragePath(282-287)saveAccounts(939-943)clearAccounts(949-974)
test/storage-recovery-paths.test.ts (1)
lib/storage.ts (1)
loadAccounts(684-686)
🔇 Additional comments (3)
lib/storage.ts (2)
784-807: backup recovery scan order looks correct
lib/storage.ts:784-807(line 784) iterates backup candidates newest-to-oldest and persists the first normalized recovery candidate. this aligns with the fallback design.
953-970: artifact cleanup now covers all configured backups
lib/storage.ts:953-970(line 953) clears primary, wal, and all recovery candidate backups in one pass. this matches the rotation lifecycle.test/storage.test.ts (1)
1620-1635: good regression coverage for backup artifact cleanup
test/storage.test.ts:1620-1635(line 1620) now asserts.bak.1and.bak.2deletion alongside primary,.bak, and.wal. this directly guards the new cleanup behavior inlib/storage.ts:953-970.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/runtime-paths.test.ts (1)
11-25:⚠️ Potential issue | 🟠 Majorrestore original env values after each test to keep the suite deterministic
test/runtime-paths.test.ts:11-25deletes global env keys but never restores prior values. this can bleed into other test files when workers are reused.proposed fix
+const originalEnv = { + CODEX_HOME: process.env.CODEX_HOME, + CODEX_MULTI_AUTH_DIR: process.env.CODEX_MULTI_AUTH_DIR, + USERPROFILE: process.env.USERPROFILE, + HOME: process.env.HOME, + HOMEDRIVE: process.env.HOMEDRIVE, + HOMEPATH: process.env.HOMEPATH, +}; + describe("runtime-paths", () => { beforeEach(() => { vi.resetModules(); vi.clearAllMocks(); delete process.env.CODEX_HOME; delete process.env.CODEX_MULTI_AUTH_DIR; delete process.env.USERPROFILE; delete process.env.HOME; delete process.env.HOMEDRIVE; delete process.env.HOMEPATH; homedir.mockReturnValue("/home/neil"); }); afterEach(() => { + process.env.CODEX_HOME = originalEnv.CODEX_HOME; + process.env.CODEX_MULTI_AUTH_DIR = originalEnv.CODEX_MULTI_AUTH_DIR; + process.env.USERPROFILE = originalEnv.USERPROFILE; + process.env.HOME = originalEnv.HOME; + process.env.HOMEDRIVE = originalEnv.HOMEDRIVE; + process.env.HOMEPATH = originalEnv.HOMEPATH; vi.restoreAllMocks(); });As per coding guidelines,
test/**: tests must stay deterministic and use vitest.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/runtime-paths.test.ts` around lines 11 - 25, The tests delete several environment variables in the beforeEach block (CODEX_HOME, CODEX_MULTI_AUTH_DIR, USERPROFILE, HOME, HOMEDRIVE, HOMEPATH) but never restore their original values; modify the setup so beforeEach captures the original process.env values (e.g., store a map of the current values) and then in afterEach restore those originals (reassign process.env entries or delete keys that were originally absent), while still calling vi.restoreAllMocks() and keeping homedir.mockReturnValue("/home/neil") behavior; update the beforeEach/afterEach around vi.resetModules(), vi.clearAllMocks(), homedir.mockReturnValue, and the env deletions to ensure test determinism and no cross-test pollution.
🤖 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/runtime-paths.ts`:
- Around line 17-27: The concatenation of homeDrive + homePath can produce
drive-relative paths when HOMEPATH lacks a leading separator; update the logic
that builds drivePathHome (variables homeDrive, homePath, drivePathHome) to
normalize by ensuring homePath begins with path.sep (or a backslash on Windows)
before concatenation (e.g., if homePath does not start with "/" or "\" prepend
"\"), and then return the firstNonEmpty([...]) as before; also add a vitest
regression in the runtime-paths tests to cover HOMEPATH without a leading
separator so the new behavior is verified.
In `@test/runtime-paths.test.ts`:
- Around line 101-112: Add a new regression test that mirrors the existing
Windows case but uses HOMEPATH="Users\\Carol" (no leading slash) to ensure
getCodexHomeDir() handles drive-relative paths correctly: spy/mock
process.platform to "win32" (reuse platformSpy pattern), set
homedir.mockReturnValue(...) as in the existing test, set process.env.HOMEDRIVE
= "E:" and process.env.HOMEPATH = "Users\\Carol", import
"../lib/runtime-paths.js" and assert mod.getCodexHomeDir() returns
"E:\\Users\\Carol\\.codex"; ensure you restore platformSpy in finally like the
existing test.
---
Outside diff comments:
In `@test/runtime-paths.test.ts`:
- Around line 11-25: The tests delete several environment variables in the
beforeEach block (CODEX_HOME, CODEX_MULTI_AUTH_DIR, USERPROFILE, HOME,
HOMEDRIVE, HOMEPATH) but never restore their original values; modify the setup
so beforeEach captures the original process.env values (e.g., store a map of the
current values) and then in afterEach restore those originals (reassign
process.env entries or delete keys that were originally absent), while still
calling vi.restoreAllMocks() and keeping homedir.mockReturnValue("/home/neil")
behavior; update the beforeEach/afterEach around vi.resetModules(),
vi.clearAllMocks(), homedir.mockReturnValue, and the env deletions to ensure
test determinism and no cross-test pollution.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (2)
lib/runtime-paths.tstest/runtime-paths.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/runtime-paths.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/runtime-paths.ts
🔇 Additional comments (2)
lib/runtime-paths.ts (1)
43-43: good consolidation of home resolution pathsrouting
getCodexHomeDir, fallback dirs, and legacy dir through one resolver (lib/runtime-paths.ts:43,lib/runtime-paths.ts:124,lib/runtime-paths.ts:221) removes split behavior and matches the new windows precedence tests intest/runtime-paths.test.ts:76-112.Also applies to: 124-129, 221-221
test/runtime-paths.test.ts (1)
76-99: windows precedence coverage is solidthe new cases in
test/runtime-paths.test.ts:76-99clearly validateUSERPROFILEthenHOMEfallback behavior underwin32and keep assertions explicit.
Normalize Windows HOMEDRIVE/HOMEPATH resolution, restore env vars in runtime path tests, add HOMEPATH no-leading-slash regression, extend backup recovery coverage to .bak.2, and strengthen backup rotation tests with EPERM retry and parallel-save ordering checks. Co-authored-by: Codex <noreply@openai.com>
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 `@test/storage.test.ts`:
- Around line 1587-1604: The test leaves fs.copyFile mocked if saveAccounts
throws, risking cross-test contamination; wrap the spy restoration in a finally
block so copySpy.mockRestore() runs regardless of errors — locate the test
around the saveAccounts invocation (the const copySpy = vi.spyOn(fs, "copyFile")
mock and the await saveAccounts(...) call) and move or add the
copySpy.mockRestore() call into a finally that always executes after the await
to ensure the original fs.copyFile is restored.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (4)
lib/runtime-paths.tstest/runtime-paths.test.tstest/storage-recovery-paths.test.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 (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/storage-recovery-paths.test.tstest/runtime-paths.test.tstest/storage.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/runtime-paths.ts
🧬 Code graph analysis (2)
test/storage-recovery-paths.test.ts (1)
lib/storage.ts (1)
loadAccounts(684-686)
test/storage.test.ts (1)
lib/storage.ts (3)
saveAccounts(939-943)getStoragePath(282-287)clearAccounts(949-974)
🔇 Additional comments (6)
lib/runtime-paths.ts (2)
15-33: good windows fallback normalization and ordering.the resolver at
lib/runtime-paths.ts:15(Line 15) correctly handlesUSERPROFILE,HOME, andHOMEDRIVE/HOMEPATH, including the no-leading-slash case viawin32.resolveatlib/runtime-paths.ts:21(Line 21). this directly addresses a windows edge case without adding token/email logging.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.
45-45: centralizing home resolution across call sites is a solid hardening change.using
getResolvedUserHomeDir()inlib/runtime-paths.ts:45(Line 45),lib/runtime-paths.ts:126(Line 126), andlib/runtime-paths.ts:223(Line 223) removes divergence between primary/fallback/legacy path derivation.Also applies to: 126-130, 223-223
test/runtime-paths.test.ts (2)
10-42: env isolation setup is clean and deterministic.the setup/teardown at
test/runtime-paths.test.ts:27(Line 27) andtest/runtime-paths.test.ts:35(Line 35) prevents cross-test env contamination and keeps module reload behavior stable.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.
95-144: nice windows regression coverage, including the homedrive/homepath edge case.the added cases at
test/runtime-paths.test.ts:95(Line 95) throughtest/runtime-paths.test.ts:144(Line 144) explicitly coverUSERPROFILE,HOME,HOMEDRIVE+HOMEPATH, andHOMEPATHwithout leading slash.test/storage-recovery-paths.test.ts (1)
96-122: backup fallback regressions are well targeted.the new tests at
test/storage-recovery-paths.test.ts:96(Line 96) andtest/storage-recovery-paths.test.ts:124(Line 124) validate.bak.1and.bak.2recovery paths and verify persistence after recovery, which closes a real corruption-recovery gap.Also applies to: 124-151
test/storage.test.ts (1)
1606-1695: strong regression coverage for rotation depth, parallel ordering, and artifact cleanup.
test/storage.test.ts:1606(Line 1606),test/storage.test.ts:1649(Line 1649), andtest/storage.test.ts:1710(Line 1710) now cover.bak/.bak.1/.bak.2ordering, concurrency serialization expectations, and clear-path cleanup breadth.Also applies to: 1710-1725
Add transient 429 retry support for backup copy operations and harden retry tests by always restoring fs.copyFile spies in finally blocks to avoid cross-test contamination. Co-authored-by: Codex <noreply@openai.com>
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (2)
lib/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 (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/storage.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/storage.ts
🧬 Code graph analysis (1)
test/storage.test.ts (1)
lib/storage.ts (3)
saveAccounts(939-943)getStoragePath(282-287)clearAccounts(949-974)
🔇 Additional comments (2)
test/storage.test.ts (2)
1576-1642: good retry coverage and mock cleanup discipline.
test/storage.test.ts:1576-1642cleanly exercises transientEPERM/429copy retries and restoresfs.copyFileinfinally, which keeps suites deterministic.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.
1687-1733: solid concurrency regression for rotating backups.
test/storage.test.ts:1687-1733validates parallelsaveAccountsserialization and deterministic.bak/.bak.1/.bak.2ordering.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.
Limit filesystem backup copy retries to real Node fs transient codes (EPERM, EBUSY) and remove synthetic 429 retry test coverage. Co-authored-by: Codex <noreply@openai.com>
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 `@test/storage.test.ts`:
- Around line 1653-1699: The test "keeps rotating backup order deterministic
across parallel saves" currently asserts only the .bak rotations; add a
primary-file assertion by reading and JSON-parsing the main storage file (the
same storagePath variable used in the test) and assert that its
accounts[0].refreshToken equals "token-4" to catch a lost-latest-write
regression triggered by concurrent saveAccounts calls (see
lib/storage.ts:938-942); update the test to read await fs.readFile(storagePath,
"utf-8"), parse it, and add
expect(primary.accounts?.[0]?.refreshToken).toBe("token-4").
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
test/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 (1)
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
🧬 Code graph analysis (1)
test/storage.test.ts (1)
lib/storage.ts (3)
saveAccounts(939-943)getStoragePath(282-287)clearAccounts(949-974)
🔇 Additional comments (3)
test/storage.test.ts (3)
1564-1574: good retry-path isolation and cleanup.the
try/finallyrestoration aroundvi.spyOn(fs, "copyFile")intest/storage.test.ts:1564andtest/storage.test.ts:1598keeps tests deterministic and prevents cross-test contamination while exercising transient copy failures tied to storage backup retries inlib/storage.ts:938.Also applies to: 1576-1608
1610-1651: rotation regression now validates full three-generation retention.this now verifies
.bak,.bak.1, and.bak.2ordering after multiple saves intest/storage.test.ts:1610, which is the right depth for the rotating backup contract.
1714-1715: backup artifact cleanup coverage is solid.including
.bak.1and.bak.2create-and-delete assertions intest/storage.test.ts:1714andtest/storage.test.ts:1728matches the expanded cleanup behavior inlib/storage.ts:948-973.Also applies to: 1720-1721, 1728-1729
Stage rotation copies to temp files before applying backup slot updates to avoid partial backup-chain rewrites when the latest backup copy fails.\n\nAdd regression coverage for preserving historical backups on latest-copy failure and assert primary snapshot in parallel rotation test. Co-authored-by: Codex <noreply@openai.com>
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/storage.ts`:
- Around line 239-241: The staged-commit loop that performs fs.rename on each
stagedWrite (the stagedWrites loop around fs.rename in lib/storage.ts) lacks
retry/backoff for transient Windows locks and 429-like transient errors; wrap
the fs.rename call for each stagedWrite in a retry-with-exponential-backoff
(configurable max attempts and base delay), catch and retry on syscall EBUSY
(and any transient 429-style errors your IO layer surfaces), fail-fast for
permanent errors, and ensure retries are awaited per-item; add a small jitter to
avoid thundering and add tests in test/storage.test.ts (extend the existing
copy-retry coverage around lines ~1542-1608) that simulate EBUSY and ensure
rename eventually succeeds; avoid logging any token/email contents when emitting
retry or error logs.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (2)
lib/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 (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/storage.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
🧬 Code graph analysis (2)
lib/storage.ts (1)
scripts/repo-hygiene.js (1)
code(21-21)
test/storage.test.ts (1)
lib/storage.ts (3)
saveAccounts(969-973)getStoragePath(312-317)clearAccounts(979-1004)
🔇 Additional comments (6)
lib/storage.ts (3)
179-206: copy retry path looks solid.
lib/storage.ts: Line 179-206correctly retries transienteperm/ebusywith exponential backoff and safely tolerates missing source files during slot shifts.
814-847: backup recovery cascade is correctly ordered and resilient.
lib/storage.ts: Line 814-847walks.bak→.bak.1→.bak.2, continues past unreadable candidates, and persists the first valid normalized snapshot.
983-999: artifact cleanup now matches rotating backup depth.
lib/storage.ts: Line 983-999expandsclearAccountsto clear wal plus all backup tiers, which matches the new recovery candidate set.test/storage.test.ts (3)
1564-1608: good vitest hygiene and windows retry coverage.
test/storage.test.ts: Line 1564-1608usesfinallyfor spy restoration and adds explicitebusy+epermcopy retry coverage in deterministic form.
1610-1766: rotation regressions are well covered, including parallel ordering.
test/storage.test.ts: Line 1610-1766validates.bak/.bak.1/.bak.2retention, preserves history on latest-copy failure, and asserts primary snapshot under parallel saves.
1781-1797: clearaccounts cleanup assertions now match rotating backups.
test/storage.test.ts: Line 1781-1797now verifies.bak.1and.bak.2removal alongside primary and wal artifacts.
Add retry/backoff for staged backup rename commits to tolerate transient filesystem locks during rotation, and add regression coverage for EBUSY staged rename retry. Co-authored-by: Codex <noreply@openai.com>
Delete orphaned backup rotation staged files during account load to prevent leftover token-bearing temp artifacts after interrupted writes. Add recovery-path regression coverage that verifies .bak/.bak.1/.bak.2 rotate temp files are removed while unrelated temp files remain untouched. Co-authored-by: Codex <noreply@openai.com>
1 participant
Summary
.bak,.bak.1,.bak.2) on each saveclearAccountsand add regression coverageValidation
npx vitest run test/storage-recovery-paths.test.ts test/storage.test.tsnpm run typechecknote: greptile review for oc-chatgpt-multi-auth. cite files like
lib/foo.ts:123. confirm regression tests + windows concurrency/token redaction coverage.Greptile Summary
added three-snapshot rotating backups (
.bak,.bak.1,.bak.2) with staged write pattern to prevent corruption during windows antivirus locks. extends recovery to scan historical backups when latest is corrupted. fixes previous review issues: replaced http status codes (E429, 429) with proper filesystem codes (EPERM, EBUSY, EAGAIN) in retry logic, and addedcleanupStaleRotatingBackupArtifactson load to remove orphaned.rotate.<nonce>.<label>.tmpfiles from crashed processes.changes
os.homedir()to handle system profile edge caseswindows filesystem safety
.rotate.artifacts on every load to prevent token leakagetest/storage.test.ts:654-694covers retry scenarios,test/storage-recovery-paths.test.ts:563-595validates artifact cleanuprisks
.rotate.suffix - any deviation in temp file naming would bypass detectionConfidence Score: 4/5
lib/storage.ts:229-275(createRotatingAccountsBackup) - verify staged write logic handles all partial failure modes correctly during windows antivirus locksImportant Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[saveAccounts] --> B{backup enabled?} B -->|yes| C[createRotatingAccountsBackup] B -->|no| Z[write primary] C --> D[stage: copy .bak → .bak.1.rotate.nonce.tmp] D --> E[stage: copy .bak.1 → .bak.2.rotate.nonce.tmp] E --> F[stage: copy primary → .bak.rotate.nonce.tmp] F --> G{all staged?} G -->|yes| H[atomic rename batch] G -->|no| J[finally: cleanup temps] H --> K[rename .bak.2.rotate → .bak.2] K --> L[rename .bak.1.rotate → .bak.1] L --> M[rename .bak.rotate → .bak] M --> N[finally: cleanup remaining temps] N --> Z Z --> DONE[done] CRASH[process crash] -.->|orphaned temps| LOAD[loadAccounts] LOAD --> CLEANUP[cleanupStaleRotatingBackupArtifacts] CLEANUP --> SCAN[scan dir for .rotate.*.tmp] SCAN --> UNLINK[unlink stale artifacts]Last reviewed commit: a6e6a75
Context used:
dashboard- What: Every code change must explain how it defends against Windows filesystem concurrency bugs and ... (source)