[robobun]

Problem

• test/cli/install/bun-lock.test.ts > "peer no published version satisfies" > "declared by a registry package" (from install: stop looping on a peer dependency no published version satisfies #38851) fails on main about every fourth build (build 104990). expect(registry.requests).toEqual([]) at bun-lock.test.ts:1778 receives ["/peer-target"]: the manifest cache has no usable entry for peer-target. It has two causes. install: add a test harness flag that writes manifest cache entries before exit #40073 covers the first, the save_async exit race. This PR fixes the second.
• Serializer::save (src/install/npm.rs:1352) named the temporary file <name hash>.npm-<milliseconds> in the temporary directory every bun process shares. The four concurrent peer tests all save peer-target, and two saves in one millisecond opened one file. Windows, release build, four installs saving one package at once: 77 of 80 lost their entry. Linux uses O_TMPFILE today; install: replace an existing manifest cache entry atomically instead of unlinking it first #38916 starts using the name on Linux too, so this fix has to land first.

Fix

• Serializer::save names the file with FileSystem::tmpname, as TarballStream already does. Correct because the name no longer depends on anything two processes share.
• Two tests in bun-install-registry.test.ts: four synchronized installs keep their own valid entries, and an install still caches its manifest when directories occupy the old temporary names. The second fails 3 of 3 on Windows without the fix and passes 5 of 5 with it. The test registry serves the tarball only after the cache entry exists, so neither test depends on the exit race.
• Verified: bun bd test test/cli/install/bun-install-registry.test.ts (250 pass) on Linux, both new tests on Windows.

Background

• Manifest cache: each fetched manifest is serialized to <cache dir>/<name hash>-<registry hash>.npm. Within the response's max-age, a later install resolves from that file and makes no request.
• write_file opens the temporary file with O_CREAT | O_TRUNC, writes, then renames it into the cache. Two processes on one name write one file. On macOS the second rename fails and the first cache holds the second's bytes, which load_by_file rejects. On Windows rename_at_w moves by handle, so the second rename moves the file out of the first cache again.
• The exit race: save_async writes the entry from a thread pool task that bun install does not wait for, by design (test(security-scanner-matrix): don't depend on the setup install's manifest cache writes having landed #37203). The test change in test(install): establish the warm manifest cache before the unmet peer test re-resolves #39190 works around it for the bun-lock test. The harness flag in install: add a test harness flag that writes manifest cache entries before exit #40073 removes it from every install test.

Notes

Probe (Windows, 16 vCPUs): one registry and one project per install, dependencies: { "peer-target": "2.0.1" }, each registry holds its manifest response until all four have been asked, then all respond at once. After exit, the .npm entry is read and checked for the install's own registry origin.

binary	temp dir	rounds	ok	missing	wrong registry
1.4.1-canary.1 (release, unfixed)	shared %TEMP%	20	3	60	17
1.4.1-canary.1 (release, unfixed)	shared, responses not synchronized	40	135	17	8
1.4.1-canary.1 (release, unfixed)	one per install	40	160	0	0
debug, unfixed	shared	20	74	3	3
debug, this branch	shared	20	80	0	0

The unfixed debug build loses far fewer entries than the release build because its slower parse spreads the four saves over several milliseconds. That is also why the concurrent test detects the bug almost always on release lanes and only sometimes on a debug build. The directory test fails deterministically on a debug build (3 of 3 and, in an earlier shape of the test, 5 of 5 on Windows).

Windows mechanism in detail: open_file_at_windows maps O_CREAT | O_TRUNC to FILE_OVERWRITE_IF with FILE_SHARE_READ | WRITE | DELETE, so every process opens and truncates the same file. rename_at_w (src/sys/windows/mod.rs:1933) opens the source by path and then moves it by handle. When two processes have opened the path before either moves it, both moves succeed: the second one relocates the file out of the first process's cache. That process sees a successful save and has no entry, which is the "no entry, no error" case in the probe.

FileSystem::tmpname (src/resolver/lib.rs:198) formats .<random ^ nanoseconds>-<counter>.<ext>. The temporary directory probe in PackageManagerDirectories.rs and TarballStream::open_destination use it the same way.

Test registry and the exit race: both new tests read the cache after the install exits. To keep them independent of the save_async exit race, the registry answers the tarball request only once a .npm entry exists in the project's cache (polling with a 5 second deadline, after which it answers anyway so a lost write ends as a failed assertion instead of a hang). The tarball is requested after the manifest is parsed, so the install cannot finish before its entry is on disk.

Sequencing: #38916 changes the Linux O_TMPFILE path to link the file into the temporary directory under tmp_path before renaming it over an existing entry, which makes the name load-bearing on Linux. This PR should land before it. #39190 (the bun-lock warm-up loop) and #40073 (harness flag) handle the exit race; an earlier shape of this PR carried #39190's commit, which the self-review flagged as a duplicate of an open PR, so it was dropped.

Gate: the fixed code path is not reachable on Linux today (O_TMPFILE), so the new tests pass on Linux with and without the fix. The fail-before proof is the Windows run above.

Also run: cargo clippy -p bun_install, clean.

---

no test proof · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/cli/install/bun-install-registry.test.ts, test/cli/install/bun-lock.test.ts

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 4d5f61ff-e075-40ee-96c0-bb72bcc16a7c

📥 Commits

Reviewing files that changed from the base of the PR and between bdf4738 and 788d63f.

📒 Files selected for processing (2)

• src/install/npm.rs
• test/cli/install/bun-install-registry.test.ts

Included review availability: Your plan provides up to 5 included reviews per hour; 0 remain after this review.

---

Walkthrough

Changes

The npm manifest cache now uses randomized temporary filenames from FileSystem::tmpname. Bun registry install tests cover concurrent cache writes, manifest integrity, and conflicts with legacy filename patterns.

npm manifest cache writes

Layer / File(s)	Summary
Randomized temporary file naming src/install/npm.rs	The manifest cache replaces timestamp-based temporary paths with FileSystem::tmpname("npm", ...).
Concurrent cache validation test/cli/install/bun-install-registry.test.ts	Tests verify separate registry manifests remain intact and temporary filenames avoid the previous package-hash and millisecond pattern.

Suggested reviewers: jarred-sumner, alii

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title clearly summarizes the main change: using a separate temporary file for each manifest cache write.
Description check	✅ Passed	The description explains the problem, fix, test coverage, verification results, and relevant background. It provides the information required by the template, although it uses Problem and Fix headings…

Full details: Description check

Explanation

The description explains the problem, fix, test coverage, verification results, and relevant background. It provides the information required by the template, although it uses Problem and Fix headings instead of the exact template headings.

---

_{Comment @coderabbitai help to get the list of available commands.}

[robobun]

Status: fix and tests pushed, head is 788d63f. After a self-review the PR is now the single fix commit: the Serializer::save change and its two tests. The bun-lock.test.ts warm-up loop it carried earlier is #39190's change and was dropped here, since #40073 supersedes it.

How it was reproduced: on Windows, four concurrent bun install processes with their own registries and cache directories, all saving the manifest of one package name, with the registries releasing the manifest response at the same moment. With the shared temporary directory and the release canary, 77 of 80 installs ended without a usable cache entry (60 missing, 17 holding another registry's manifest). With one temporary directory per install, 0 of 160. With this branch (debug build), 0 of 80. The new directory test fails 3 of 3 times on Windows without the fix and passes 5 of 5 with it.

What fixes the bun-lock flake completely: this PR (the temporary file collision on macOS and Windows) plus #40073 (the exit race, every platform). #38916 should land after this PR because it makes the temporary file name load-bearing on Linux.

[claude]

LGTM — swaps the hand-rolled <name-hash>.npm-<ms> temp filename for the shared FileSystem::tmpname helper already used across src/install/, removing the cross-process collision.

What was reviewed:

• Serializer::save: buffer size (64) matches repository.rs usage; tmpname output (max ~39 bytes) fits; return type coerces to write_file's &ZStr param; import path matches crate convention.
• New registry tests: hermetic (local Bun.serve, per-project cache, servers cleaned up in finally/await using), pipes drained concurrently, directory-trap test correctly reconstructs the old format (hex_int_lower::<16> → padStart(16, "0")).
• bun-lock.test.ts retry loop: bounded (5 tries), still asserts cachedManifests === 2 after, so it fails rather than silently passing if the entry never lands.

Extended reasoning...

Overview

Three files touched. src/install/npm.rs replaces the temporary filename in Serializer::save — previously <wyhash(name)>.npm-<milliseconds>, which two concurrent bun processes saving the same package in the same millisecond would collide on — with FileSystem::tmpname(b"npm", &mut [0u8; 64], fast_random()), the same helper used by TarballStream, extract_tarball, repository.rs, PackageManagerDirectories, and half a dozen other install-side call sites. The old 512+64 byte buffer, timestamp computation, and FixedBufferStream formatting are deleted. Two new tests in bun-install-registry.test.ts cover concurrent installs and a directory-trap that deterministically fails on the old naming. bun-lock.test.ts gains a bounded retry loop before its cached-resolve assertion to work around the separate fire-and-forget save_async exit race.

Security risks

None. The change makes temp filenames less predictable (random ^ nanoseconds + process counter vs. deterministic hash + millisecond). No new inputs are parsed, no paths are derived from user data, and the file is still opened relative to the process's own tmpdir fd and renamed into the process's own cache dir.

Level of scrutiny

Medium — this is package-manager code, but the native change is a 3-line substitution to an established helper with ~10 identical call sites in the same crate, and the buffer sizing/return-type/error-propagation all check out against those siblings. The test additions are larger but follow harness conventions closely (port 0, await using, drained pipes, env.BUN_TMPDIR already set per-test by the file's beforeEach).

Other factors

The PR description is unusually thorough: measured collision rates before/after on Windows release and debug builds, explains why Linux is unaffected (O_TMPFILE), and is upfront that the concurrent test is probabilistic on debug builds while the directory-trap test is deterministic. The bun-lock.test.ts retry is a de-flake, not a weakening — the post-loop expect(cachedManifests(...)).toBe(2) still fails the test if the cache never fills, and the comment names the source (save_async) and the general fix (#40073). No outstanding reviewer comments; only a rate-limited CodeRabbit stub in the timeline.

[robobun]

^{Updated 7:23 PM PT - Aug 24th, 2026}

✅ @robobun, your commit 788d63fd005f0c9b1fb07d9c8cb5816494dd9032 passed in Build #105230! 🎉

---

🧪   To try this PR locally:

bunx bun-pr 40377

That installs a local version of the PR into your bun-40377 executable, so you can run:

bun-40377 --bun

[claude]

LGTM — the earlier comment-style nit has been addressed at head (31aca28).

What was reviewed:

• Serializer::save now uses FileSystem::tmpname(b"npm", &mut [0u8; 64], fast_random()), matching the pattern already used at 11+ other src/install/ call sites (TarballStream, extract_tarball, PackageManagerDirectories, etc.); the 64-byte buffer is sufficient for tmpname's .{hex}-{counter}.npm output and matches sibling callers.
• Confirmed write_file still consumes tmp_path correctly on both the Windows absolute-path branch and the POSIX openat branch, and that Linux's O_TMPFILE fast path bypasses the name entirely (so the fix is a no-op there, as the PR states).
• New tests are hermetic: local Bun.serve({ port: 0 }), per-test BUN_TMPDIR set by the file's beforeEach, pipes drained concurrently, servers closed in finally/await using; the directory-trap test's mkdir loop is bounded (≤10,100 iterations) and confined to the test's own temp dir.
• The bun-lock.test.ts retry loop is a bounded poll (5 tries) on an observable condition (count of *.npm entries) with the reason documented — the fire-and-forget save_async exit race, deferred to #40073.

Extended reasoning...

Overview

Three files: a 3-line native fix in src/install/npm.rs (replace a hand-rolled <name-hash>.npm-<ms-timestamp> temp file name with the shared FileSystem::tmpname helper), two new tests in bun-install-registry.test.ts (~120 lines) reproducing the cross-process temp-file collision, and a bounded retry loop in bun-lock.test.ts de-flaking an existing test's manifest-cache-warm assertion.

Security risks

None. The change is to how a temporary file inside bun's own temp directory is named before being renamed into the package manager's manifest cache. No user-controlled input reaches the name; the new name is random ^ nanoseconds plus an atomic counter, strictly less predictable than the old package-hash + millisecond scheme.

Level of scrutiny

Low-to-medium. The native change is mechanical: it deletes 14 lines of bespoke FixedBufferStream formatting and replaces them with one call to the in-tree helper that a dozen sibling call sites in src/install/ already use with the same signature and buffer size. This is exactly what REVIEW.md's "grep for the in-tree helper before hand-writing anything" rule prescribes. The return type (&mut ZStr) coerces to write_file's &ZStr parameter; both the Windows absolute-path join and the POSIX openat path consume it the same way as before.

Other factors

• My previous review left one nit (a 4-line change-narrating comment). The author removed the comment entirely at head 31aca28, and comment-cop's threads are resolved. No other reviewer comments are outstanding.
• The bug hunting system found no issues on this revision.
• The PR description is unusually thorough, with a Windows probe table demonstrating 77/80 failures unfixed → 0/80 fixed, and correctly notes the Linux O_TMPFILE path never uses the name (so the new tests pass on Linux with or without the fix — the fail-before proof is Windows-only, which the author states).
• Tests follow harness conventions: port: 0, bunEnv spread, Promise.all on stdout/stderr/exited, await using for servers/processes, per-test BUN_TMPDIR from the file's existing beforeEach. The directory-trap test's loop is bounded by ms < start + 10_000 and writes into the test's own packageDir/.bun-tmp, which is cleaned up with the test dir.

[claude]

LGTM — the earlier comment nit was addressed; the Rust change now just swaps the hand-rolled temp name for FileSystem::tmpname, matching the ten other call sites in src/install/.

What was reviewed:

• Serializer::save: return type of tmpname matches write_file's &ZStr param; the 64-byte buffer is the same size repository.rs uses and comfortably fits the ~31-byte max output.
• Checked that bun_fmt/bun_io::FixedBufferStream are still used elsewhere in npm.rs — no dead imports left behind.
• New tests: local Bun.serve({port: 0}), per-test BUN_TMPDIR (set in this file's beforeEach), bounded polls, pipes drained concurrently, servers stopped in finally/await using. The directory-trap loop is capped at 10 100 iterations.

Extended reasoning...

Overview

The Rust change in src/install/npm.rs replaces a bespoke temporary-file name (<name-hash>.npm-<millisecond-hex>) with FileSystem::tmpname(b"npm", &mut [0u8; 64], bun_core::fast_random()). This is the same helper already used at ten other places in src/install/ (TarballStream, extract_tarball, PackageManagerDirectories, repository.rs with an identical 64-byte buffer, etc.), so the change is a net deletion of 12 lines of hand-rolled formatting in favor of the shared idiom. The ? propagation and crate::bun_fs::FileSystem import path match every sibling caller. bun_fmt::hex_int_lower and bun_io::FixedBufferStream remain in use elsewhere in the file, so no dead imports.

The test additions in bun-install-registry.test.ts add a two-test describe: one races four synchronized installs sharing BUN_TMPDIR and asserts each keeps a valid per-registry cache entry; the other pre-creates directories at every old-format name for the next few seconds and asserts the install still writes its entry (deterministic on Windows without the fix). Both use a local Bun.serve({ port: 0 }) registry, drain subprocess pipes concurrently, poll for the .npm entry with a 5 s deadline rather than sleeping, and clean up via await using/finally. The trap loop is bounded (< start + 10_000) so it can't run away on a slow lane. BUN_TMPDIR is set to join(packageDir, ".bun-tmp") in this file's beforeEach, so the thousands of trap directories land in a per-test dir.

Security risks

None. The change narrows a shared-resource collision to a per-writer unique name; it introduces no new inputs, parsing, or trust boundaries.

Level of scrutiny

Low-to-medium. The production change is three lines that adopt an existing, widely-used in-tree helper — REVIEW.md's "grep for the in-tree helper before hand-writing anything" applied in reverse. The bulk of the diff is test scaffolding. The earlier comment-cop and my prior nit about the narrating code comment have been resolved (the comment is gone from the current diff), and all inline threads are marked resolved.

Other factors

The PR description documents fail-before/pass-after on Windows in detail, and clippy was run clean. The Linux O_TMPFILE fast path bypasses the name entirely, which the tests acknowledge. The added beforeAll import is used by the new describe. Nothing outstanding from other reviewers.