Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion .github/workflows/nodejs.yml
Original file line number Diff line number Diff line change
Expand Up @@ -15,4 +15,13 @@ jobs:
uses: artusjs/github-actions/.github/workflows/node-test.yml@v1
with:
os: 'ubuntu-latest, macos-latest, windows-latest'
version: '14, 16, 18'
version: '16, 18'

# Node.js 14 has no macOS arm64 build (macos-latest is Apple Silicon),
# so run it on Linux and Windows only.
Job-node-14:
name: Node.js 14
uses: artusjs/github-actions/.github/workflows/node-test.yml@v1
with:
os: 'ubuntu-latest, windows-latest'
version: '14'
2 changes: 2 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@ npm install diskstore --save

- `new DiskStore(options)`
- {String} cacheDir - root cache dir
- {Boolean} [fallback=true] - when the internal `rename` fails with `EXDEV` (the `.tmp` dir and the target file are on different filesystems), retry by writing a temp file in the target file's own directory and rename it there, which stays atomic. Set to `false` to let the `EXDEV` error throw.
- {String} [fallbackTmpfileName] - the temp file name used by the fallback, placed in the target file's directory. Defaults to a random name. A fixed name is only safe when concurrent writes to the same directory cannot happen; otherwise keep the default so each write gets a unique temp file.
- `async get(relativePath)` read data from relativePath if the file exists.
- {String} relativePath - file path relative to root cache dir
- `async set(relativePath, data)` write data to relativePath
Expand Down
33 changes: 33 additions & 0 deletions lib/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,9 @@ class DiskStore extends Base {
*
* @param {Object} options
* - {String} cacheDir - cache dir
* - {Boolean} [fallback=true] - on cross-device rename (EXDEV), retry via the target's own dir
* - {String} [fallbackTmpfileName] - temp file name used by the fallback, defaults to a random
* one. A fixed name is only safe when concurrent writes to the same directory cannot happen.
*/
constructor(options = {}) {
assert(options.cacheDir, '[DiskStore] options.cacheDir is required');
Expand All @@ -22,6 +25,15 @@ class DiskStore extends Base {
return this.options.cacheDir;
}

// on by default, only off when explicitly set to false
get fallback() {
return this.options.fallback !== false;
}

get fallbackTmpfileName() {
return this.options.fallbackTmpfileName;
}

async get(relativePath) {
const filepath = path.join(this.cacheDir, relativePath);
try {
Expand All @@ -47,6 +59,27 @@ class DiskStore extends Base {
await fs.rename(tmpfile, filepath);
} catch (err) {
await fs.rm(tmpfile, { force: true });
// rename across filesystems throws EXDEV, retry via the target's own dir
if (err.code === 'EXDEV' && this.fallback) {
await this._renameWithFallback(dir, filepath, data);
return;
}
throw err;
}
}

// write a temp file in the target's own dir then rename, so it stays on one
// filesystem and keeps the atomic replace
async _renameWithFallback(dir, filepath, data) {
const fallbackTmpfile = path.join(
dir,
this.fallbackTmpfileName || `.${path.basename(filepath)}.${crypto.randomUUID()}.tmp`

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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`

);
try {
await fs.writeFile(fallbackTmpfile, data);
await fs.rename(fallbackTmpfile, filepath);
} catch (err) {
await fs.rm(fallbackTmpfile, { force: true });
throw err;
}
}
Expand Down
93 changes: 93 additions & 0 deletions test/index.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,99 @@ describe('test/index.test.js', () => {
assert.deepEqual(data, Buffer.from('a foo bar'));
});

describe('EXDEV fallback', () => {
// simulate .tmp being on a different device than the target: rename from .tmp
// throws EXDEV, everything else falls through to the real rename
function mockCrossDevice() {
const originRename = fs.rename;
mm(fs, 'rename', async (src, dest) => {
if (src.includes(`${path.sep}.tmp${path.sep}`)) {
const err = new Error('EXDEV: cross-device link not permitted');
err.code = 'EXDEV';
throw err;
}
return originRename(src, dest);
});
}

it('should fallback to same-dir write when rename is cross-device', async function() {
mockCrossDevice();
await diskStore.set('exdev/a', 'exdev value');
const data = await diskStore.get('exdev/a');
assert.deepEqual(data, Buffer.from('exdev value'));
// the fallback temp file should be cleaned up, nothing left in the dir
const files = await fs.readdir(path.join(cacheDir, 'exdev'));
assert.deepEqual(files, [ 'a' ]);
});

it('should use custom fallbackTmpfileName', async function() {
const store = new DiskStore({
cacheDir,
fallbackTmpfileName: '.custom.fallback.tmp',
});
await store.ready();
// record the source of the same-dir (fallback) rename to prove the custom name is used
const renamed = [];
const originRename = fs.rename;
mm(fs, 'rename', async (src, dest) => {
if (src.includes(`${path.sep}.tmp${path.sep}`)) {
const err = new Error('EXDEV: cross-device link not permitted');
err.code = 'EXDEV';
throw err;
}
renamed.push(src);
return originRename(src, dest);
});
await store.set('exdev-custom', 'v');
const data = await store.get('exdev-custom');
assert.deepEqual(data, Buffer.from('v'));
assert.deepEqual(renamed, [ path.join(cacheDir, '.custom.fallback.tmp') ]);
// the fallback temp file is removed once used
assert(await store.get('.custom.fallback.tmp') === null);
});

it('should clean up and throw when the fallback rename also fails', async function() {
mm(fs, 'rename', async src => {
if (src.includes(`${path.sep}.tmp${path.sep}`)) {
const err = new Error('EXDEV: cross-device link not permitted');
err.code = 'EXDEV';
throw err;
}
// the fallback (same-dir) rename fails too
throw new Error('mock fallback rename error');
});
try {
await diskStore.set('exdev-fail/a', 'v');
assert(false, 'should not run here');
} catch (err) {
assert(err.message === 'mock fallback rename error');
}
// both the primary and the fallback temp files should be cleaned up
const files = await fs.readdir(path.join(cacheDir, 'exdev-fail'));
assert(files.length === 0);
const tmpFiles = await fs.readdir(path.join(cacheDir, '.tmp'));
assert(tmpFiles.length === 0);
});

it('should throw EXDEV when fallback disabled', async function() {
const store = new DiskStore({
cacheDir,
fallback: false,
});
await store.ready();
mockCrossDevice();
try {
await store.set('exdev-off', 'v');
assert(false, 'should not run here');
} catch (err) {
assert(err.code === 'EXDEV');
}
// the primary temp file should still be cleaned up
const files = await fs.readdir(path.join(cacheDir, '.tmp'));
assert(files.length === 0);
});
});

describe('write atomic', () => {
it('should write be atomic', async function() {
await coffee.fork('write_big_file.js', [])
Expand Down
Loading