Skip to content
Open
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
54 changes: 50 additions & 4 deletions prelude/bootstrap-shared.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
// SEA bootstrap: bundled by esbuild via require('./bootstrap-shared').

var childProcess = require('child_process');
var { createHash } = require('crypto');
var { createHash, randomBytes } = require('crypto');
var fs = require('fs');
var path = require('path');
var zlib = require('zlib');
Expand Down Expand Up @@ -85,6 +85,52 @@ function pickDecompressorAsync(compression) {
// NATIVE ADDON EXTRACTION /////////////////////////////////////////
// /////////////////////////////////////////////////////////////////

// Publish a complete file without truncating a cached addon that another
// process may already have mapped into memory. The sibling temp file keeps
// rename on the same filesystem; randomness also separates worker threads
// that share a process.pid.
function writeFileAtomic(dest, content) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Minor Readability

About 14 lines of comments sit on a roughly 45-line private helper, and several of them restate the code. The try blocks also nest 4 deep, with a renamed flag.

Why: Someone reading this for the first time has to keep renamed, identical and the rethrow in their head at once.

Fix: Cut the header to one line, and keep only the NAME_MAX, Windows-rename and EEXIST comments. Pulling the "is it already the same bytes" check into a small named predicate would remove one nesting level.

// Do not extend the destination basename: it may already be near NAME_MAX.
var temporary = path.join(
path.dirname(dest),
'.pkg-' + process.pid + '-' + randomBytes(6).toString('hex') + '.tmp',

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Minor Correctness

If the process is killed between openSync and renameSync, a .pkg-<pid>-<hex>.tmp file is left in the cache. Nothing ever removes it, and in the node_modules layout it sits inside the copied package tree.

Why: The cache slowly takes up more disk space. Loading still works.

Fix: Accepting this is fine. A sentence in the PR or a short comment would be enough.

);
// Only enter cleanup once we own the file. An EEXIST must not remove
// another writer's temporary file.
var fd = fs.openSync(temporary, 'wx');

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Minor Simplicity

The open with 'wx', then write and close through the fd, exists so cleanup never deletes another writer's temp file after EEXIST. With pid + 48 random bits, that collision essentially can't happen.

Why: Two reviewers pulled in opposite directions here. Treating 'wx' as O_EXCL protection against a planted symlink at the temp path is reasonable, and it costs little, so keeping it is fine.

Fix: Optional. A plain writeFileSync(temporary, content) would save about 6 lines, plus the EEXIST test and its fixture branch.

var renamed = false;
try {
try {
fs.writeFileSync(fd, content);
} finally {
fs.closeSync(fd);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nit Correctness

If writeFileSync throws and then closeSync also throws in the inner finally, the close error replaces the more useful write error (for example ENOSPC).

}
try {
fs.renameSync(temporary, dest);
renamed = true;
} catch (error) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Minor Correctness

On Windows, Defender or the search indexer often holds a newly written file open for a moment, so MoveFileEx fails with EPERM/EACCES/EBUSY. When the bytes differ (the cache-repair path), that error is rethrown right away. graceful-fs retries renames for this reason.

Why: This is rare, and the old writeFileSync could fail the same way, so it isn't a clear regression.

Fix: Optionally retry a few times on those codes before comparing the bytes.

// Windows can refuse to replace an addon another process has loaded.
// Accept a concurrent winner only if it published the exact same bytes.
var identical = false;
try {
identical = fs.readFileSync(dest).equals(content);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

FYI Performance

On the rename-failure path this reads the whole addon back from disk, which can be 1-20 MB. Comparing fs.statSync(dest).size first would skip that read when the sizes differ. It only runs on failure, so this isn't urgent.

} catch (_) {
// Keep the original rename error if the destination cannot be read.
}
if (!identical) throw error;
}
} finally {
// Also remove partial files left by a failed write, not just failed renames.
if (!renamed) {
try {
fs.unlinkSync(temporary);
} catch (_) {
// Best-effort cleanup must not hide the original write or rename error.
}
}
}
}

// Recursively copy src -> dest. For existing destination files, compare
// SHA-256 hashes and skip identical ones to avoid redundant writes.
//
Expand Down Expand Up @@ -152,7 +198,7 @@ function cpRecursive(src, dest) {
return;
}
}
fs.writeFileSync(dest, srcContent);
writeFileAtomic(dest, srcContent);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

FYI Correctness

Replacing with a rename instead of writing in place means a replaced file gets 0o666 & ~umask rather than keeping the mode, owner and hard links of the existing file. Nothing in the repo depends on that, so this is only a note.

}

/**
Expand Down Expand Up @@ -221,10 +267,10 @@ function patchDlopen(insideSnapshot) {
var dContent = fs.readFileSync(tmpModulePath);
var dHash = createHash('sha256').update(dContent).digest('hex');
if (hash !== dHash) {
fs.writeFileSync(tmpModulePath, moduleContent);
writeFileAtomic(tmpModulePath, moduleContent);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Minor DRY

This branch repeats the "exists → read → compare → write" steps from the end of cpRecursive (lines 192-201). That makes three different ways to check whether the bytes match: hash both sides, compare against a hash computed earlier, and Buffer.equals inside writeFileAtomic.

Why: The atomic guarantee only holds if every cache write goes through the same path. A later fix to the skip-if-identical or publish logic that lands in only one copy would bring the truncation race back.

Fix: Move the compare-then-publish steps into one helper used by both call sites, and settle on one way to compare.

}
} else {
fs.writeFileSync(tmpModulePath, moduleContent);
writeFileAtomic(tmpModulePath, moduleContent);
}

newPath = tmpModulePath;
Expand Down
312 changes: 312 additions & 0 deletions test/unit/native-addon-cache.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,312 @@
import assert from 'node:assert/strict';
import { createHash } from 'node:crypto';
import fs from 'node:fs';
import { createRequire } from 'node:module';
import os from 'node:os';
import path from 'node:path';
import { describe, it } from 'node:test';
import type { TestContext } from 'node:test';
import vm from 'node:vm';

const sharedPath = path.join(__dirname, '../../prelude/bootstrap-shared.js');
const sharedSource = fs.readFileSync(sharedPath, 'utf8');
const requireBuiltin = createRequire(__filename);
// Synthetic bytes only: the fake dlopen below never loads a native library.
const content = Buffer.from([0, 1, 2, 3, 4, 5, 255, 0]);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nit Readability

The names content, previous and different don't say what role each buffer plays. Names like fresh, stale and raced would read better.

const previous = Buffer.from([8, 7, 6, 5, 4, 3, 2, 1]);
const different = Buffer.from([0, 1, 2, 3, 4, 6, 255, 0]);
type Layout = 'node_modules' | 'standalone';

function fixture(

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Minor Readability

fixture() is about 150 lines and returns 12 members. It builds the paths, a wrapped fs with failure modes chosen by a string option, a fake process, and the vm bootstrap.

Why: Understanding what each test sets up takes several jumps into this function.

Fix: Split out the fs wrapper, or name the failure modes after what they simulate.

t: TestContext,
layout: Layout,
options: {
fail?: 'write' | 'rename' | 'open';
winner?: Buffer;
filename?: string;
} = {},
) {
const root = fs.mkdtempSync(path.join(os.tmpdir(), 'pkg-native-cache-'));
const liveDescriptors = new Set<number>();
t.after(() => {
// Also release leaked descriptors when testing a broken implementation.
for (const fd of liveDescriptors) fs.closeSync(fd);
fs.rmSync(root, { recursive: true, force: true });
});

const snapshot = path.join(root, 'snapshot');
const cache = path.join(root, 'cache');
const filename = options.filename ?? 'addon.node';
const relative =
layout === 'node_modules'
? path.join('sample-addon', 'build', 'Release', filename)
: filename;
const source = path.join(
snapshot,
layout === 'node_modules' ? 'node_modules' : '',
relative,
);
const hash = createHash('sha256').update(content).digest('hex');
const destination = path.join(cache, 'pkg', hash, relative);
fs.mkdirSync(path.dirname(source), { recursive: true });
fs.writeFileSync(source, content);

const opens: { file: string; flags: string; fd?: number }[] = [];
const writes: (string | number)[] = [];
const closes: number[] = [];
const renames: { from: string; to: string; before?: Buffer }[] = [];
const loads: { file: string; bytes: Buffer }[] = [];
const error = Object.assign(new Error('injected cache failure'), {
code: options.fail === 'write' ? 'ENOSPC' : 'EPERM',
});
let collision: string | undefined;

// Only the VM sees these wrappers; fixture setup and inspection use real fs.
const wrappedFs = {
...fs,
openSync(file: string, flags: string) {
const attempt: (typeof opens)[number] = { file, flags };
opens.push(attempt);
if (options.fail === 'open') {
// Another writer owns this exact candidate before our exclusive open.
collision = file;
fs.writeFileSync(file, different);
}
const fd = fs.openSync(file, flags);
attempt.fd = fd;
liveDescriptors.add(fd);
return fd;
},
writeFileSync(file: string | number, bytes: Buffer) {
writes.push(file);
if (options.fail === 'write') {
fs.writeFileSync(file, bytes.subarray(0, 3));
throw error;
}
fs.writeFileSync(file, bytes);
},
closeSync(fd: number) {
fs.closeSync(fd);
liveDescriptors.delete(fd);
closes.push(fd);
},
renameSync(from: string, to: string) {
renames.push({
from,
to,
before: fs.existsSync(to) ? fs.readFileSync(to) : undefined,
});
assert.deepEqual(fs.readFileSync(from), content);
assert.equal(liveDescriptors.size, 0, 'close before publishing');
if (options.fail === 'rename') {
// Deterministic Windows-style race: a winner appears, then rename fails.
// No timing, child processes, or host-specific rename semantics needed.
if (options.winner) fs.writeFileSync(to, options.winner);
throw error;
}
fs.renameSync(from, to);
},
};
const fakeProcess = {
env: { PKG_NATIVE_CACHE_PATH: cache },
pid: process.pid,
dlopen(_module: object, file: string) {
loads.push({ file, bytes: fs.readFileSync(file) });
return 'loaded';
},
};
const shared = {
exports: {} as {
patchDlopen: (_inside: (_file: string) => boolean) => void;
},
};
// Evaluate the actual shared prelude, without importing a generated SEA bundle
// or patching the test runner's process.dlopen / filesystem module.
vm.runInNewContext(
sharedSource,
{
Buffer,
process: fakeProcess,
module: shared,
require: (id: string) => (id === 'fs' ? wrappedFs : requireBuiltin(id)),
},
{ filename: sharedPath },
);
shared.exports.patchDlopen((file) => file.startsWith(snapshot + path.sep));

return {
destination,
opens,
writes,
closes,
renames,
loads,
error,
load: () => fakeProcess.dlopen({}, source),
seed(bytes: Buffer) {
fs.mkdirSync(path.dirname(destination), { recursive: true });
fs.writeFileSync(destination, bytes);
},
assertLoaded() {
assert.deepEqual(loads, [{ file: destination, bytes: content }]);
},
assertCache(bytes?: Buffer, extra: string[] = []) {
assert.deepEqual(
fs.readdirSync(path.dirname(destination)).sort(),
[...(bytes ? [path.basename(destination)] : []), ...extra].sort(),
'no temporary files left behind',
);
if (bytes) assert.deepEqual(fs.readFileSync(destination), bytes);
assert.equal(liveDescriptors.size, 0, 'no leaked descriptors');
},
assertStagedWrite() {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Minor Tests

assertStagedWrite and the wrapped fs check exactly which syscalls run: open with 'wx', write through the fd, close exactly once, EBADF afterwards, and no fds open at rename time.

Why: A rewrite that behaves the same, such as writeFileSync(temp, buf, {flag:'wx'}), would fail these tests. The behavior that matters (destination never opened for writing, temp file is a sibling, rename publishes it, nothing left over) is already covered by the end-state checks.

Fix: Keep the "temp is a sibling, not the destination" check and assertCache. Drop the exact syscall checks.

assert.equal(opens.length, 1, 'one exclusive temporary-file open');

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Minor Readability

This helper returns the temp path but also runs about 10 assertions. Most tests call it last, so when it fails, the message points away from the test that broke.

Why: Reading a test doesn't show what it checks.

Fix: Rename it so the name says it asserts, or inline the key checks into each test.

const [{ file, flags, fd }] = opens;
assert.equal(flags, 'wx');
assert.notEqual(
file,
destination,
'never open the destination for writing',
);
assert.equal(path.dirname(file), path.dirname(destination));
assert.equal(typeof fd, 'number');
assert.deepEqual(writes, [fd], 'write only through the temporary fd');
assert.deepEqual(closes, [fd], 'close the temporary fd exactly once');
assert.throws(() => fs.fstatSync(fd!), { code: 'EBADF' });
return file;
},
getCollision() {
assert.ok(collision, 'exclusive open must have been attempted');
return collision;
},
};
}

for (const layout of ['node_modules', 'standalone'] as const) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Minor Simplicity

There are 24 tests for one helper. Every failure-path case runs for both layouts, and some for both cold and replacement states, but each of those runs exercises the same writeFileAtomic code.

Why: 312 lines of tests is about 7x the size of the fix. That is a lot of reading for what it proves.

Fix: Run the happy paths (cold, replacement, warm skip) per layout and each failure path once. That is about 10 tests and roughly 80-100 fewer lines.

describe(`native addon cache (${layout})`, () => {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Minor Tests

The node_modules layout copies a package that has exactly one file, so cpRecursive is only tested on a single file in a single directory.

Why: A bug that only shows up with several files (a shared temp name, or stopping at the first match) would not be caught.

Fix: Add a variant with at least two files, one of them in a nested directory, where one is identical and one is stale. Assert that only the stale one is rewritten and no .pkg-*.tmp files remain.

for (const existing of [false, true]) {
const state = existing ? 'replacement' : 'cold';

it(`publishes a ${state} cache via a complete sibling file`, (t) => {
const f = fixture(t, layout);
if (existing) f.seed(previous);

assert.equal(f.load(), 'loaded');
f.assertLoaded();
const temporary = f.assertStagedWrite();
assert.deepEqual(f.renames, [
{
from: temporary,
to: f.destination,
before: existing ? previous : undefined,
},
]);
f.assertCache(content);
});

it(`cleans up a partial ENOSPC write for a ${state} cache`, (t) => {
const f = fixture(t, layout, { fail: 'write' });
if (existing) f.seed(previous);

assert.throws(f.load, (err) => err === f.error);
assert.deepEqual(f.loads, [], 'must not load after a failed write');
assert.deepEqual(f.renames, [], 'must not publish partial bytes');
f.assertCache(existing ? previous : undefined);
f.assertStagedWrite();
});

it(`cleans up a failed rename for a ${state} cache`, (t) => {
const f = fixture(t, layout, { fail: 'rename' });
if (existing) f.seed(previous);

assert.throws(f.load, (err) => err === f.error);
assert.deepEqual(f.loads, [], 'must not load after a failed rename');
assert.equal(f.renames.length, 1);
f.assertCache(existing ? previous : undefined);
f.assertStagedWrite();
});
}

it('publishes an addon with a 240-byte basename', (t) => {
// A valid basename near NAME_MAX must not be extended with a temp suffix.
const filename = `${'a'.repeat(235)}.node`;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nit Readability

The literals 235, 240 and 255 are linked only by arithmetic in the reader's head. Either name them (for example NAME_MAX) or drop the test. No real addon has a 240-byte basename, and the fixed .pkg-…tmp name already avoids the problem.

const f = fixture(t, layout, { filename });
assert.equal(Buffer.byteLength(path.basename(f.destination)), 240);

assert.equal(f.load(), 'loaded');
f.assertLoaded();
const temporary = f.assertStagedWrite();
assert.ok(Buffer.byteLength(path.basename(temporary)) <= 255);
assert.deepEqual(f.renames, [
{ from: temporary, to: f.destination, before: undefined },
]);
f.assertCache(content);
});

it('uses a fresh sibling filename for each cache repair', (t) => {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Minor Simplicity

This test checks that two cache repairs pick different temp names. That is a property of randomBytes, not of the fix.

Fix: Delete it (17 lines).

const f = fixture(t, layout);
for (let attempt = 0; attempt < 2; attempt += 1) {
f.seed(previous);
assert.equal(f.load(), 'loaded');
}

assert.equal(f.opens.length, 2);
assert.notEqual(f.opens[0].file, f.opens[1].file);
for (const { file, flags } of f.opens) {
assert.equal(flags, 'wx');
assert.notEqual(file, f.destination);
assert.equal(path.dirname(file), path.dirname(f.destination));
}
assert.equal(f.renames.length, 2);
f.assertCache(content);
});

it('does not write an identical warm cache', (t) => {
const f = fixture(t, layout);
f.seed(content);

assert.equal(f.load(), 'loaded');
f.assertLoaded();
assert.deepEqual(f.opens, []);
assert.deepEqual(f.writes, []);
assert.deepEqual(f.closes, []);
assert.deepEqual(f.renames, []);
f.assertCache(content);
});

for (const matching of [true, false]) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Minor Tests

The Windows fallback tests cover "same bytes" and "different bytes". They don't cover a failed rename where the destination can't be read, which is the inner catch (_) that has to rethrow the original rename error. Today the cold-failure test only reaches that branch by accident, because the destination doesn't exist there.

Why: If that branch accidentally set identical = true, every test that names this case would still pass.

Fix: Add a case where the destination is a directory (or the read throws) and assert the original error comes back.

it(`${matching ? 'accepts' : 'rejects'} a Windows-style rename race with ${matching ? 'matching' : 'different'} winner bytes`, (t) => {
const winner = matching ? content : different;
const f = fixture(t, layout, { fail: 'rename', winner });

if (matching) {
assert.equal(f.load(), 'loaded');
f.assertLoaded();
} else {
assert.throws(f.load, (err) => err === f.error);
assert.deepEqual(f.loads, [], 'must not load a different winner');
}
assert.equal(f.renames.length, 1, 'exercise the rename failure');
f.assertCache(winner);
f.assertStagedWrite();
});
}

it("does not remove another writer's temporary file on open EEXIST", (t) => {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Minor Simplicity

This test, together with the fail: 'open' / getCollision fixture branches, only exists for the exclusive-open guard on bootstrap-shared.js line 100.

Fix: If that guard goes, delete this test with it. If the guard stays, keep the test.

const f = fixture(t, layout, { fail: 'open' });
f.seed(previous);

assert.throws(f.load, { code: 'EEXIST' });
const collision = f.getCollision();
assert.notEqual(collision, f.destination);
assert.equal(path.dirname(collision), path.dirname(f.destination));
assert.deepEqual(f.opens, [{ file: collision, flags: 'wx' }]);
assert.deepEqual(fs.readFileSync(collision), different);
assert.deepEqual(f.writes, []);
assert.deepEqual(f.closes, []);
assert.deepEqual(f.renames, []);
assert.deepEqual(f.loads, []);
f.assertCache(previous, [path.basename(collision)]);
});
});
}