fix(storage): fail export when current pool is empty#320
Conversation
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
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:
📝 WalkthroughWalkthroughrefactored export/migration wiring. added Changes
Sequence Diagram(s)estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes notes and review focus
suggested labels
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![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 1475-1484: The export path currently re-throws EBUSY/EPERM from
loadAccountsFromPath (seen in the catch branch near
createEmptyStorageWithMetadata), which differs from loadAccountsInternal's
fallback behavior; add a regression test that simulates a locked storage file
(Windows-style EBUSY/EPERM) and asserts that the export code either falls back
to the backup/WAL path (matching loadAccountsInternal) or explicitly documents
and handles the error; specifically, add a test in test/account-port.test.ts (or
a new test file) that stubs/throws an error with code "EBUSY" and "EPERM" from
loadAccountsFromPath and verifies the behavior of the export routine (calling
loadAccountsForExport or the function that invokes loadAccountsFromPath) to
ensure consistent fallback behavior or explicit error handling, updating the
implementation in lib/storage.ts where necessary to mirror
loadAccountsInternal's fallback logic.
In `@test/account-port.test.ts`:
- Around line 7-81: Add a third test for exportAccountsSnapshot that passes
transactionState: undefined and stubs readCurrentStorage to return a storage
with a unique token (e.g., "locked-read-token"); assert readCurrentStorage was
called once, readCurrentStorageUnlocked was not called, and exportAccountsToFile
was invoked with storage containing that token. Locate the test by the existing
describe "account port helpers" and mimic the existing test patterns,
referencing exportAccountsSnapshot, transactionState, readCurrentStorage,
readCurrentStorageUnlocked, and exportAccountsToFile to implement the new case.
In `@test/experimental-sync-target-entry.test.ts`:
- Around line 69-73: Add deterministic regression tests that assert runtime
retry vs fail-fast behavior of readFileWithRetry rather than only checking
argument wiring: create two vitest tests in
experimental-sync-target-entry.test.ts that mock the underlying file-read to
throw an error object with errno set to a Windows lock code and use a
controllable sleep mock/timer—(1) for retryable codes (the Set used in existing
expectation), assert readFileWithRetry retries up to maxAttempts and eventually
resolves or exhausts retries (assert call count, sleeps called, and
result/throw), and (2) for fail-fast codes (EACCES and ENOTEMPTY), assert
readFileWithRetry immediately throws on first call with no sleeps or retries;
stub/spy readFileWithRetry invocation and the sleep function and use thrown
Error objects with errno properties to simulate race conditions so tests are
deterministic and bounded.
🪄 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: 5edac6fd-b737-4226-bdf2-5f577ccc9549
📒 Files selected for processing (4)
lib/storage.tslib/storage/account-port.tstest/account-port.test.tstest/experimental-sync-target-entry.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/experimental-sync-target-entry.test.tstest/account-port.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/account-port.tslib/storage.ts
🔇 Additional comments (6)
lib/storage/account-port.ts (1)
14-14: lgtm on the rename and branch logic.the rename from
loadAccountsInternaltoreadCurrentStorageUnlockedbetter communicates intent. the three-way branch correctly handles:
- active transaction with matching path → reuse snapshot (no io)
- active transaction with mismatched path → call unlocked loader (avoids deadlock since lock already held)
- no active transaction → call locked reader
this aligns with the non-reentrant
withStorageLockshown inlib/storage/transactions.ts:7-13.Also applies to: 30-32
lib/storage.ts (3)
1435-1485: new export-specific loader looks correct for the stated goal.
loadAccountsForExport()intentionally avoids WAL/backup recovery, ensuring export fails if the current pool is genuinely empty. this aligns with the pr objective to prevent silent fallback to stale data during export.key behavior differences from
loadAccountsInternal:
- no WAL journal recovery (lines 1475-1484 just re-throw)
- no backup candidate iteration
- no synthetic fixture promotion
1843-1844: wiring looks correct.
readCurrentStorageUnlockedbypasses the lock (caller already holds it via transaction), whilereadCurrentStoragewraps withwithStorageLockfor the no-transaction case. this matches the decision logic inlib/storage/account-port.ts:26-32.
1438-1439: missing regression test: migration writes during path-mismatch transaction.
loadAccountsForExport(lib/storage.ts:1435) callsmigrateLegacyProjectStorageIfNeeded(saveAccountsUnlocked)at line 1439. when this function is used asreadCurrentStorageUnlockedinside a transaction wheretransactionState.storagePathdiffers fromcurrentStoragePath, the migration writes to the current path while the transaction holds a snapshot for a different path. test/account-port.test.ts:58 exercises the path-mismatch scenario but mocksreadCurrentStorageUnlocked, so the actual migration path is never tested. add a regression test that callsexportAccountsSnapshotwith a real transaction state and validates that legacy migration succeeds without corrupting the transaction snapshot.test/account-port.test.ts (2)
8-40: test correctly verifies snapshot reuse when paths match.assertions at lines 33-39 confirm that when
transactionState.activeand paths match, no loader is invoked and the snapshot is passed directly toexportAccountsToFile. deterministic, no real secrets.
42-81: test correctly verifies live storage read when paths differ.assertions at lines 72-80 confirm
readCurrentStorageUnlockedis called (notreadCurrentStorage) and the live token is used, not the stale snapshot token. good coverage for the path-mismatch branch.
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 1444-1500: loadAccountsForExport calls
migrateLegacyProjectStorageIfNeeded and saveAccountsUnlocked without holding the
storage lock, allowing a race with concurrent saves; fix by performing migration
and persisting under the same locked path used for normal writes: either (A)
replace the unlocked migration+save flow with the locked
readCurrentStorage/saveAccounts flow so migrations use the storage lock, or (B)
wrap both migrateLegacyProjectStorageIfNeeded and the subsequent
saveAccountsUnlocked call in the existing storage lock helper so they run
atomically; update loadAccountsForExport to call the locked variant (or call
saveAccounts instead of saveAccountsUnlocked) and ensure
migrateLegacyProjectStorageIfNeeded persistence branch uses the lock as well, or
add a concise comment in loadAccountsForExport stating that unlocked best-effort
behavior is intentional if you choose not to lock.
🪄 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: ac5e8dc1-8baf-4b72-b61a-9362b220ff6b
📒 Files selected for processing (4)
lib/storage.tstest/account-port.test.tstest/experimental-sync-target-entry.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/account-port.test.tstest/experimental-sync-target-entry.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/storage.ts
🔇 Additional comments (10)
test/storage.test.ts (4)
1389-1436: ebusy/eperm rethrow test addresses past review comment.
test/storage.test.ts:1389-1436mocksloadAccountsFromPathto throw EBUSY/EPERM for the locked path, then asserts the error propagates with matching code (line 1429).this directly covers the windows lock contention scenario flagged in the past review comment at
lib/storage.ts:1490-1499. the parameterized approach ensures both codes are tested.
1301-1343: test confirms export does not persist legacy migration during transaction.
test/storage.test.ts:1301-1343sets up a legacy storage file, invokes export withinwithAccountStorageTransaction, and asserts:
- exported content matches legacy storage (line 1336)
currentStoragePathremains absent (line 1338)legacyStoragePathis not unlinked (line 1339)this validates the
{ commit: false }behavior inloadAccountsForExport({ persistMigrations: false }).
1345-1387: test confirms v3 normalization is not persisted during unlocked export read.
test/storage.test.ts:1345-1387creates a v1 storage file, exports within a transaction, and asserts:
- on-disk version stays 1 (line 1379)
- exported version is 3 (line 1380)
this confirms the export path does not write back migrations when
persistMigrations: false.
7-7: import added forsetStoragePathState.the import at
test/storage.test.ts:7supports the transaction-state export tests below. this is correctly scoped tolib/storage/path-state.js.lib/storage.ts (3)
1858-1860: export wiring correctly selects unlocked vs locked reader based on transaction state.the wiring at
lib/storage.ts:1858-1860passesloadAccountsForExport({ persistMigrations: false })toreadCurrentStorageUnlockedand wraps the locked variant inwithStorageLock. this matches the logic inlib/storage/account-port.ts:26-32where the snapshot is only reused when the transaction path matches the current storage path.the
persistMigrations: falseflag ensures no side-effect writes occur during unlocked reads, which is the correct design for export under a different transaction context.
712-717: migration options refactor looks good.the
{ persist, commit }options object atlib/storage.ts:712-717cleanly separates persistence behavior from commit semantics. defaultingpersisttosaveAccountsandcommittotruepreserves backward compatibility forloadAccountsInternal.
788-830: commit flag controls whether migration writes are persisted.the conditional at
lib/storage.ts:788correctly gates the persist/unlink logic. whencommit: false, only in-memory state is updated (lines 829-830), which is appropriate for export-time migration detection without side effects.test/experimental-sync-target-entry.test.ts (2)
43-91: test now verifies retry policy wiring and simulated retry behavior.the updated test at
test/experimental-sync-target-entry.test.ts:43-91asserts:
retryableCodescontains exactly EBUSY, EPERM, EAGAIN (lines 47-49)retryableCodesexcludes EACCES, ENOTEMPTY (lines 50-51)maxAttemptsis 4 (line 52)sleepis called with exponential delays 25 and 50 (lines 88-89)this addresses the past review comment requesting runtime retry behavior verification rather than just argument wiring.
93-124: fail-fast test for non-retryable EACCES is deterministic and complete.the new test at
test/experimental-sync-target-entry.test.ts:93-124correctly verifies:
- EACCES throws immediately (line 119)
readFileWithRetryis called exactly once (line 121)sleepis never called (line 122)normalizeAccountStorageis never reached (line 123)this confirms windows lock-race fail-fast policy as required by the coding guidelines.
test/account-port.test.ts (1)
8-113: all three branches of snapshot selection logic are now tested.
test/account-port.test.ts:8-113covers:
- active transaction + matching path → uses snapshot (lines 33-34 assert readers not called)
- active transaction + mismatched path → uses
readCurrentStorageUnlocked(lines 72-73)- no active transaction → uses
readCurrentStorage(lines 104-105)this matches the branching logic in
lib/storage/account-port.ts:26-32. the past review comment requesting the third test case is now addressed.
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 788-824: The migration (migrateLegacyProjectStorageIfNeeded) is
running too early and repopulates an empty current store before
loadAccountsForExport("locked") runs; move the call to
migrateLegacyProjectStorageIfNeeded({ persist: saveAccountsUnlocked }) out of
the immediate startup path and instead invoke it inside the ENOENT branch where
the code handles missing current storage (the branch around the enoent handling
at the earlier read logic), so migration only occurs after an export read miss;
update the call sites referenced (the current invocations near the earlier lines
calling migrateLegacyProjectStorageIfNeeded and the export call
loadAccountsForExport("locked")) and add vitest cases for “current exists + 0
accounts + legacy exists” and “reset-marker + legacy exists” to prevent
regressions and ensure concurrency/Windows IO behaviors are covered.
🪄 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: fc529852-b3fa-4ca2-ad8c-eeda31c083f6
📒 Files selected for processing (1)
lib/storage.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)
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
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
test/storage.test.ts (1)
1410-1457: 🛠️ Refactor suggestion | 🟠 Majormissing the unlocked windows-lock regression.
the unlocked happy path is covered at
test/storage.test.ts:1300-1408, and this case covers the locked failure path, but nothing drives the cross-path transaction branch atlib/storage.ts:1864-1866through anEBUSYorEPERMread failure. that is the branch most exposed to windows lock contention because it bypasseswithStorageLockwhile another transaction owns the mutex. please add a vitest that opens an unrelatedwithAccountStorageTransaction(), forcesloadAccountsFromPath()on the live path to throw, and assertsexportAccounts()rejects without writingexportPath.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.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/storage.test.ts` around lines 1410 - 1457, Add a deterministic vitest that reproduces the unlocked-windows-lock regression by starting a concurrent withAccountStorageTransaction() to hold the in-memory transaction path, then forcing loadAccountsFromPath() on the real storage path to throw (simulate { code: "EBUSY" } or "EPERM") and assert that exportAccounts(exportPath) rejects and that exportPath was not written; use vi.doMock/vi.importActual to stub loadAccountsFromPath for the live path only, ensure you open the transaction before calling exportAccounts, and clean up mocks/resetModules and restore storage path with setStoragePathDirect so the test is deterministic and exercises the cross-path branch in lib/storage.ts (the branch that bypasses withStorageLock).
🤖 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 1453-1455: The locked-export path has a race where
saveAccountsUnlocked() can clear the reset marker and a concurrent process can
recreate it before the locked persist branches do writes/unlinks (the
migrationOptions branch and subsequent v1 normalization and legacy-migration
writes in the export flow), so change the locked export to be read-only or make
the locked persist path check the reset-marker again and refuse to write/unlink
before performing any filesystem mutation; specifically, update the code around
migrationOptions/migration commit logic and the v1-normalize and legacy-migrate
code paths to re-read the reset marker (or use a read-only export branch) before
any write/unlink, add or adjust tests in test/storage.test.ts to cover the
interleaving case, and ensure new code follows lib/** guidelines for auth
rotation, Windows IO and concurrency (including handling EBUSY/429 retries).
---
Duplicate comments:
In `@test/storage.test.ts`:
- Around line 1410-1457: Add a deterministic vitest that reproduces the
unlocked-windows-lock regression by starting a concurrent
withAccountStorageTransaction() to hold the in-memory transaction path, then
forcing loadAccountsFromPath() on the real storage path to throw (simulate {
code: "EBUSY" } or "EPERM") and assert that exportAccounts(exportPath) rejects
and that exportPath was not written; use vi.doMock/vi.importActual to stub
loadAccountsFromPath for the live path only, ensure you open the transaction
before calling exportAccounts, and clean up mocks/resetModules and restore
storage path with setStoragePathDirect so the test is deterministic and
exercises the cross-path branch in lib/storage.ts (the branch that bypasses
withStorageLock).
🪄 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: dd4edecf-42c7-49b4-a27f-25b7ccbc3d52
📒 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
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/storage.ts (1)
746-785:⚠️ Potential issue | 🟠 Majordo not merge legacy after the live file reappears.
after the initial miss on
lib/storage.ts:1455-1476, the fallback callsmigrateLegacyProjectStorageIfNeeded({ commit: false }). that helper then re-readsstate.currentStoragePathatlib/storage.ts:746-759and merges it with legacy data atlib/storage.ts:781-785. if another process recreates the current file in that window—especially as an empty store—export can still return the merged legacy snapshot instead of honoring the live file. that reopens the empty-current non-revival bug under a race.suggested fix
if (code === "ENOENT") { const migratedLegacyStorage = await migrateLegacyProjectStorageIfNeeded({ commit: false }); if (existsSync(resetMarkerPath)) { return createEmptyStorageWithMetadata(false, "intentional-reset"); } + if (existsSync(path)) { + const { normalized, schemaErrors } = await loadAccountsFromPath(path, { + normalizeAccountStorage, + isRecord, + }); + if (schemaErrors.length > 0) { + log.warn("Account storage schema validation warnings", { + errors: schemaErrors.slice(0, 5), + }); + } + return normalized; + } return migratedLegacyStorage; }please add a vitest that forces
enoenton the first read and then makes the current file appear empty before the legacy merge;test/storage.test.ts:1580-1673only covers the steady-state empty/missing cases.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: 1474-1480
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/storage.ts` around lines 746 - 785, The migration currently re-reads state.currentStoragePath and proceeds to merge legacy data even if another process recreated the current file in the window; to fix, in migrateLegacyProjectStorageIfNeeded after calling loadNormalizedStorageFromPath for state.currentStoragePath (the targetStorage read at lib/storage.ts around the loadNormalizedStorageFromPath call) re-check that the on-disk current storage is still "absent" or unchanged before calling mergeStorageForMigration(targetStorage, legacyStorage,...); if the current file now exists (or its contents/mtime indicate a non-empty live store), abort the legacy merge and return without applying legacy data. Update logic around loadNormalizedStorageFromPath, migrateLegacyProjectStorageIfNeeded, and the mergeStorageForMigration call to short-circuit when the current file reappears, and add a vitest that forces an initial ENOENT then creates an empty/current file before the legacy merge (extend test/storage.test.ts around 1580-1673) and verify behavior under concurrent IO (EBUSY/429) as per guidelines.
🤖 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 1445-1492: Update the test matrix to include the "EAGAIN" error
code as a retryable read-lock case: add "EAGAIN" to the it.each array alongside
"EBUSY" and "EPERM" (the matrix that drives the mocked loadAccountsFromPath
behavior), so the mocked loadAccountsFromPath throws an error with code "EAGAIN"
for the lockedStoragePath and the assertion on
isolatedStorageModule.exportAccounts(exportPath) expects { code: "EAGAIN" };
ensure the same addition is applied to the corresponding matrix further down
(the block covering lines 1494-1578) so both test cases exercise EAGAIN, and
keep the rest of the test flow (setStoragePathDirect, doMock/doUnmock,
resetModules) unchanged.
---
Outside diff comments:
In `@lib/storage.ts`:
- Around line 746-785: The migration currently re-reads state.currentStoragePath
and proceeds to merge legacy data even if another process recreated the current
file in the window; to fix, in migrateLegacyProjectStorageIfNeeded after calling
loadNormalizedStorageFromPath for state.currentStoragePath (the targetStorage
read at lib/storage.ts around the loadNormalizedStorageFromPath call) re-check
that the on-disk current storage is still "absent" or unchanged before calling
mergeStorageForMigration(targetStorage, legacyStorage,...); if the current file
now exists (or its contents/mtime indicate a non-empty live store), abort the
legacy merge and return without applying legacy data. Update logic around
loadNormalizedStorageFromPath, migrateLegacyProjectStorageIfNeeded, and the
mergeStorageForMigration call to short-circuit when the current file reappears,
and add a vitest that forces an initial ENOENT then creates an empty/current
file before the legacy merge (extend test/storage.test.ts around 1580-1673) and
verify behavior under concurrent IO (EBUSY/429) as per guidelines.
🪄 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: ee4f670a-631d-4043-82fa-6be3b44cbef4
📒 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
🔇 Additional comments (2)
lib/storage.ts (1)
1842-1843: good locked vs unlocked export reader split.
lib/storage.ts:1842-1843cleanly separates the cross-path transaction read from the normal locked read, which keeps export side-effect free without dropping the serialized no-transaction path.test/storage.test.ts:1300-1432andtest/storage.test.ts:1494-1578exercise both sides well.test/storage.test.ts (1)
1580-1715: good regression coverage for legacy non-revival.
test/storage.test.ts:1580-1715pins both cases that mattered here: empty current storage and reset-marker presence both reject export and leave legacy state untouched.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
1 participant
Summary
What Changed
loadAccountsForExport()stays side-effect free and reset-marker awaremigrateLegacyProjectStorageIfNeeded({ commit: false })is only used for in-memory export fallback and never writes or unlinks files on that pathEBUSY/EPERM/EAGAINlock races instead of collapsing them into an empty resultexportAccountsSnapshot()still reuses the transaction snapshot only when it matches the current storage pathValidation
npm run lintnpm run typechecknpm run buildnode_modules\\.bin\\vitest.cmd run test/account-port.test.ts test/storage.test.ts test/project-migration.test.ts test/experimental-sync-target-entry.test.tsnpm teststill hits a Vitest worker OOM on Windows in this environment after220/221files and3161/3164tests, even withNODE_OPTIONS=--max-old-space-size=8192Risk
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
this PR fixes a class of export correctness bugs where an empty current storage pool could silently revive legacy, WAL, or reset-cleared accounts — the fix is well-scoped:
loadAccountsForExportis a new side-effect-free read path (reset-marker-aware, no normalize-and-persist side effects), andmigrateLegacyProjectStorageIfNeeded({ commit: false })provides a safe in-memory-only fallback for the legacy migration path used only during export.key changes:
exportAccountsnow wiresreadCurrentStorageUnlocked(no lock, used when a transaction is active on a different path) andreadCurrentStorage(withStorageLock, used when no transaction is active), replacing the previousloadAccountsInternal/withAccountStorageTransactionpairing that could return a stale snapshot from a different storage pathmigrateLegacyProjectStorageIfNeededgainscommit: falsemode that skips allpersistandunlinkcalls, adds two TOCTOU re-read checkpoints per loop iteration (liveCurrentStorageBeforeMergeandliveCurrentStorageAfterLegacyRead), and rethrowsEBUSY/EPERM/EAGAINlock errors instead of collapsing them into an empty result — important on windows where AV or the atomic-replace pattern can hold transient locksEACCESandENOTEMPTYremoved from the sync-target retryable codes set, correcting a pre-existing over-broad retry policy;EACCESnow propagates fail-fast, matching the existing test for permanent permission denialsConfidence Score: 4/5
commit: falsepath which removes all write/unlink side-effectsreadLiveCurrentStorageIfExportModeclosure; specifically theexistsSync→return path when no legacy candidates exist or when the file appears mid-second-iterationImportant Files Changed
loadAccountsForExport(side-effect-free, reset-marker-aware), refactorsmigrateLegacyProjectStorageIfNeededwithcommit: falseoption and two TOCTOU re-read checkpoints inside the loop; the logic is sound but the doubleexistsSync→async-read sequence inreadLiveCurrentStorageIfExportModeleaves a narrow window where a reappearing file can be missed across both loop checkpoints if the timing is unlucky — covered in practice by the post-loopexistsSyncguard, but no explicit multi-legacy-candidate test exercises that pathloadAccountsInternalreplaced byreadCurrentStorageUnlocked; the ternary now correctly routes stale-snapshot avoidance through the unlocked reader when transaction is on a different pathliveCurrentStorageAfterLegacyReadon the first candidate has already returned false)Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[exportAccounts] --> B{transaction active?} B -- "yes, same path" --> C[use transaction snapshot] B -- "yes, different path" --> D[readCurrentStorageUnlocked\nloadAccountsForExport] B -- no --> E[withStorageLock\nloadAccountsForExport] D --> F{reset marker?} E --> F F -- yes --> G[return empty intentional-reset] F -- no --> H[loadAccountsFromPath current] H -- ok --> I{reset marker?} I -- yes --> G I -- no --> J[return normalized storage] H -- ENOENT --> K{reset marker?} K -- yes --> G K -- no --> L[migrateLegacyProjectStorageIfNeeded\ncommit=false] L --> M[loop over legacy candidates] M --> N{TOCTOU: existsSync before merge?} N -- current reappeared --> O[return live storage] N -- not present --> P[loadNormalizedStorageFromPath legacy] P --> Q{TOCTOU: existsSync after legacy read?} Q -- current reappeared --> O Q -- not present --> R[mergeStorageForMigration in-memory] R --> S[targetStorage = merged, migrated = true] S --> M L --> T{migrated?} T -- yes --> U[return targetStorage] T -- no --> V{targetStorage AND no current file?} V -- yes --> U V -- no --> W[return null] O --> X{reset marker after migrate?} U --> X W --> X X -- yes --> G X -- no --> Y[exportAccountsToFile] Y --> Z{accounts empty?} Z -- yes --> AA[throw No accounts to export] Z -- no --> AB[write export file]Prompt To Fix All With AI
Reviews (11): Last reviewed commit: "fix-export-reset-fallback" | Re-trigger Greptile