feat: fall back to same-dir write when rename is cross-device (EXDEV)#6
Conversation
set() writes to a fixed <cacheDir>/.tmp/<uuid> and renames it onto the target for atomicity. When .tmp and the target file are on different filesystems, fs.rename throws EXDEV and the write fails. Add a fallback (on by default) that, on EXDEV, writes a temp file in the target's own directory and renames it there. A same-directory rename stays on one filesystem, so it avoids EXDEV while keeping the atomic replace. - new option `fallback` (default true, set false to keep throwing) - new option `fallbackTmpfileName` (defaults to a random name; a fixed name is only safe without concurrent writes to the same directory) - fallback logic isolated in the private `_renameWithFallback` method - tests for the fallback, custom name, disabled, and fallback-failure paths
|
Warning Review limit reached
Next review available in: 54 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughDiskStore adds configurable handling for ChangesEXDEV fallback handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant DiskStore.set
participant fs.rename
participant DiskStore._renameWithFallback
participant TargetDirectory
DiskStore.set->>fs.rename: rename primary temp file
fs.rename-->>DiskStore.set: EXDEV error
DiskStore.set->>DiskStore._renameWithFallback: retry fallback
DiskStore._renameWithFallback->>TargetDirectory: write fallback temp file
DiskStore._renameWithFallback->>fs.rename: rename fallback temp file
fs.rename-->>DiskStore._renameWithFallback: success or error
DiskStore._renameWithFallback-->>DiskStore.set: result or propagated error
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces an EXDEV fallback mechanism to DiskStore to handle cross-device rename failures. When a rename fails with EXDEV, the store can now fallback to writing a temporary file in the target file's own directory before renaming it, ensuring an atomic replacement. This behavior is configurable via the new fallback and fallbackTmpfileName options. The changes also include comprehensive unit tests. The review feedback highlights a critical issue where using a static, shared fallbackTmpfileName for concurrent writes in the same directory can lead to race conditions and data corruption. It is recommended to append a unique identifier, such as a UUID, to the custom fallback file name to prevent these conflicts.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| async _renameWithFallback(dir, filepath, data) { | ||
| const fallbackTmpfile = path.join( | ||
| dir, | ||
| this.fallbackTmpfileName || `.${path.basename(filepath)}.${crypto.randomUUID()}.tmp` |
There was a problem hiding this comment.
Using a static, shared fallbackTmpfileName across different target files in the same directory can lead to severe race conditions and data corruption during concurrent writes. For example, if store.set('dir/fileA') and store.set('dir/fileB') run concurrently, both will attempt to write to and rename the exact same temporary file path (dir/fallbackTmpfileName), causing one to overwrite the other's data or fail with ENOENT.
To prevent this, we should ensure that the temporary file name is always unique per write operation, even when a custom name/prefix is provided. Appending a unique identifier (like a UUID) to the custom name is a robust way to avoid these conflicts.
this.fallbackTmpfileName
? `${this.fallbackTmpfileName}.${crypto.randomUUID()}`
: `.${path.basename(filepath)}.${crypto.randomUUID()}.tmp`macos-latest is now Apple Silicon (arm64) and Node.js 14 has no macOS arm64 build, so `macos-latest x 14` fails at the setup-node step. Split the matrix so 16/18 keep running on all three OS while 14 runs on Linux and Windows only.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/index.test.js (1)
142-147: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider using
assert.rejectsfor cleaner async error testing.Both sites use a verbose
try/catchblock combined with an intentionalassert(false)to validate that a promise rejects. Consider usingassert.rejectsfor a more idiomatic and concise approach to testing async errors.
test/index.test.js#L142-L147: Simplify the rejection check for the fallback rename error.test/index.test.js#L162-L167: Simplify the rejection check for the EXDEV error.💡 Proposed refactors
For
test/index.test.js#L142-L147:- try { - await diskStore.set('exdev-fail/a', 'v'); - assert(false, 'should not run here'); - } catch (err) { - assert(err.message === 'mock fallback rename error'); - } + await assert.rejects( + diskStore.set('exdev-fail/a', 'v'), + err => err.message === 'mock fallback rename error' + );For
test/index.test.js#L162-L167:- try { - await store.set('exdev-off', 'v'); - assert(false, 'should not run here'); - } catch (err) { - assert(err.code === 'EXDEV'); - } + await assert.rejects( + store.set('exdev-off', 'v'), + err => err.code === 'EXDEV' + );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/index.test.js` around lines 142 - 147, Replace the try/catch plus intentional assert(false) rejection checks in test/index.test.js at lines 142-147 and 162-167 with assert.rejects assertions. Preserve each test’s existing promise operation and expected error message: “mock fallback rename error” at the anchor site and the EXDEV error at the sibling site.
🤖 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.
Nitpick comments:
In `@test/index.test.js`:
- Around line 142-147: Replace the try/catch plus intentional assert(false)
rejection checks in test/index.test.js at lines 142-147 and 162-167 with
assert.rejects assertions. Preserve each test’s existing promise operation and
expected error message: “mock fallback rename error” at the anchor site and the
EXDEV error at the sibling site.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a8806322-f690-4c4d-9d71-b20815839fa1
📒 Files selected for processing (3)
README.mdlib/index.jstest/index.test.js
2 participants
Problem
set()writes to a fixed<cacheDir>/.tmp/<uuid>and thenfs.renames it onto the target file to get an atomic write. When.tmpand the target file are on different filesystems (e.g..tmpis a separate mount),fs.renamethrowsEXDEV: cross-device link not permittedand the write fails outright:Change
Add a fallback (on by default) that, on
EXDEV, writes a temp file in the target's own directory and renames it there. A same-directory rename is always on one filesystem, so it avoidsEXDEVwhile keeping the atomic replace.fallback(defaulttrue; setfalseto keep the old throw behavior)fallbackTmpfileName(defaults to a random name; a fixed name is only safe when concurrent writes to the same directory can't happen)_renameWithFallbackmethodTests
Added cases under
EXDEV fallback:fallbackTmpfileName(asserts the exact fallback path is used)EXDEVwhenfallback: falsenpm test— lint clean, 10 passing.Summary by CodeRabbit
New Features
Bug Fixes
Documentation