[jeffsee55]

Problem

cpRecursive() and the standalone-addon path in patchDlopen() write directly to shared cache files with fs.writeFileSync(). Concurrent packaged processes or workers can both decide that a file needs extraction. One can then truncate the cached addon after another has loaded or memory-mapped it, potentially causing SIGBUS/SIGSEGV or exposing partial contents.

Change

Add a shared writeFileAtomic() helper for all three regular-file write sites:

• Create a random, bounded-length sibling temporary filename and open it exclusively with wx.
• Write through the owned descriptor, close it, and rename the completed file into place without truncating the published inode.
• Close descriptors and clean up temporary files after failed writes or renames, preserving existing destination contents.
• If Windows refuses a rename after another writer publishes, accept the winner only when its bytes exactly match the expected contents. Otherwise preserve the original rename error.
• Do not remove another writer's temporary file if exclusive creation fails with EEXIST.
• Keep the existing identical-cache fast path, cache layout, and extraction behavior.

The helper is shared by the traditional bootstrap, SEA main bootstrap, and SEA workers. Generated SEA bundles are rebuilt by the existing build script and remain untracked.

Tests

Add 24 deterministic cases using the actual shared prelude, isolated VM contexts, real temporary files, and a fake dlopen that never loads a native library. Both package-directory and standalone-addon extraction paths cover:

• Cold and replacement cache publication without opening the destination for writing
• Identical warm-cache reuse
• Partial ENOSPC writes and descriptor/temp-file cleanup
• Rename failures and preservation of existing bytes
• Matching and mismatched concurrent winners, simulating Windows rename behavior
• Exclusive-open collisions without deleting another writer's file
• Unique sibling filenames and valid 240-byte addon basenames

Before the implementation change, the initial 22-case suite had 20 failures and only the two warm-cache cases passed. All 24 final cases pass with the fix.

Validation

Passed locally:

• yarn build (including regenerated SEA main/worker bundles)
• node -r esbuild-register --test --test-reporter=dot 'test/unit/**/*.test.ts' (full unit suite)
• node -r esbuild-register --test --test-reporter=spec test/unit/native-addon-cache.test.ts (24 passed)
• yarn --silent prettier --check prelude/bootstrap-shared.js test/unit/native-addon-cache.test.ts
• yarn --silent eslint prelude/bootstrap-shared.js test/unit/native-addon-cache.test.ts
• yarn --silent tsc --noEmit --target es2022 --module commonjs --moduleResolution node --esModuleInterop --skipLibCheck --strict --types node test/unit/native-addon-cache.test.ts
• git diff --check

Existing baseline failure: yarn lint passes formatting but fails on prelude/bootstrap.js:597 because homedir is undefined. The same ESLint failure was confirmed against the unchanged file at base commit 43feb8bb. This PR does not modify that file; #309 addresses the separate cache-location/import issue.

Packaged-binary E2E tests and real Windows locking were not run locally; the Windows rename race is covered through deterministic fault injection.

[robertsLando]

Tring to close/reopen to see if ci starts, dunno what happened to GH

[robertsLando]

Code review

Verdict: Ship with minor changes. There are no blocking or major issues.

The approach is right. Writing to a temp file and then renaming it into place fixes the real truncation/SIGBUS bug on POSIX. The Windows fallback, which accepts an existing file only when the bytes are identical, is the right rule for a cache whose folders are named by content hash. The repo has no existing atomic-write helper this could reuse, and prelude/ can't pull in npm packages.

Strengths

• Readers can no longer see a partly written addon.
• The 'wx' open with a random temp name can't follow a planted symlink. Renaming also stops the old writeFileSync(dest) from writing through a symlink at dest.
• Temp cleanup runs on every failure path, and it never deletes a file this process didn't create.

Top risks

1. The symlink branch of cpRecursive (line 152, not changed in this PR) still calls unlinkSync then symlinkSync. If two processes extract a package with versioned .so symlinks on first launch, one can get EEXIST. That code is rethrown, so dlopen fails. This race existed before the PR, but it is now the one cache write the PR doesn't make safe for concurrent processes. A follow-up or a sentence in the PR would cover it. Possible fixes: publish the link under a temp name and rename it into place, or treat EEXIST as success when readlinkSync shows the same target.
2. The test file is about 7x the size of the fix and checks the exact syscall sequence, so harmless refactors will break it. Details are in the inline comments.
3. The compare-then-publish logic now exists in two places (cpRecursive and the standalone patchDlopen branch).

FYI (not from this PR): PKG_NATIVE_CACHE_PATH can point at a folder other users can write to, such as /tmp. Another local user could then swap the addon between the hash check and dlopen. This was the same before the PR. A possible hardening step later: create the cache folder with mode 0o700.

Note on CI: CI on this PR currently fails in build_artifact. The failure is a lint error in prelude/bootstrap.js (homedir is not defined), and it comes from main, not from this PR. A fix PR for main is coming, and CI will be re-run here after it merges.

[robertsLando]

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.

[robertsLando]

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.

[robertsLando]

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.

[robertsLando]

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.

[robertsLando]

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.

[robertsLando]

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.

[robertsLando]

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).

[robertsLando]

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.

[robertsLando]

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.

[robertsLando]

Readability

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