Repository navigation
Conversation
…ery start create_fake_temporary_node_executable deleted /tmp/bun-node-debug before recreating it in debug builds. The directory is shared by every concurrently running debug bun, so one process starting up removed the node/bun links a sibling had already put on its scripts' PATH, and the sibling's script failed with "node: not found" or ran a real node instead. The EEXIST path already re-points a link that targets a different binary, which is all the cleanup the directory needs. The two lifecycle-script tests that run with an empty PATH also drop NODE/npm_node_execpath from the environment. Under "bun bd test" the runner is itself started through "bun run", which sets those to the real node, and bun honors them instead of creating the shims, so the tests failed for an unrelated reason.
WalkthroughThe change removes destructive temporary-link cleanup, adds identity-based Windows link recovery, and documents POSIX collision handling. It adds concurrent shim-race coverage and isolates test subprocess environments from inherited Node and npm metadata. ChangesConcurrent link and test-environment safety
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Status: fix pushed (both platforms), waiting on CI. Reproduced on linux-x64 with a debug build: 16 concurrent The three |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — debug-only removal of a racy directory wipe, with a well-constructed regression test.
What was reviewed:
- Confirmed the removed
delete_tree_absoluteis redundant: the EEXIST branch alreadyreadlinks and re-points stale links, so debug now matches release behavior. - Checked the
envWithoutNoderefactor fixes the latent bug where the waiter-thread variant'stestEnvwas spread beforeenv.PATH = "", so it never actually got an empty PATH; the second test now also correctly receivesBUN_FEATURE_FLAG_FORCE_WAITER_THREAD. - Verified the new race test drains all pipes, uses
await using, awaits the observable condition (first stdout line) before spawning siblings, and is stable underdescribe.concurrentsince all processes use the samebunExe()and the fix guarantees no unlink of a matching link.
Extended reasoning...
Overview
This PR removes a 5-line #[cfg(bun_debug)] block in src/install/lib.rs that called delete_tree_absolute on the shared /tmp/bun-node-debug shim directory before every --bun run. The wipe raced with sibling debug processes that had already put that directory on their scripts' PATH, causing intermittent node: not found or fall-through to the real node. The EEXIST branch below the removed block already verifies the existing symlink target via readlink and re-points it if stale, so the wipe served no purpose. A comment is updated to document that a matching link must never be removed. The Windows debug wipe is intentionally left in place (hardlinks cannot be verified with readlink).
The two lifecycle-script tests that clear PATH are refactored to use a new envWithoutNode helper that also clears NODE/npm_node_execpath (which bun bd's bun run wrapper exports). This fixes a latent test bug: the waiter-thread variant's testEnv = { ...env, ... } was spread before env.PATH = "" was assigned, so it never actually ran with empty PATH; and ensureTempNodeGypScript works was passing raw env instead of testEnv, missing the waiter-thread flag entirely. A new test in bun-run.test.ts reproduces the race with a busy-polling watcher script plus four concurrent --bun siblings.
Security risks
None. The change is debug-build-only and strictly removes a destructive filesystem operation. Release builds are byte-identical. No auth, crypto, or untrusted-input paths are touched.
Level of scrutiny
Low-to-moderate. The native change is a 7-line deletion gated behind #[cfg(bun_debug)], aligning debug behavior with release. The test changes are larger by line count but are straightforward: a helper extraction, two call-site refactors that fix pre-existing test-only bugs (well explained in the PR description), and one new regression test. I traced the testEnv spread ordering to confirm the PR description's claim about the waiter-thread variant.
Other factors
The new race test follows harness conventions: tempDir, await using, forEachLine for line-buffered stdout, all pipes drained via Promise.all, awaits the observable readiness signal (first stdout line = the resolved shim path) before spawning siblings, and asserts a combined { stdout, stderr, exitCode } object. The busy-loop in watch.sh is justified inline (the missing-link window is only a few syscalls wide). The PR description reports 5/5 failures against main and 10/10 passes with the fix, plus both affected test files green under bun bd test. Skipped on Windows since the watcher is a sh script and the Windows path is unchanged.
…npm_* in the test harness The Windows arm of create_fake_temporary_node_executable had the same per-start delete_tree of the shared bun-node-debug directory, and deleting a hard link to a running image succeeds on NTFS, so concurrent debug processes lost each other's node.exe/bun.exe there as well. Instead of wiping, compare the identity (st_dev/st_ino) of an existing link with the running image on ALREADY_EXISTS and replace only a link that is a different file, mirroring the readlink check in the POSIX arm. This keeps the protection against links left by a previous build that the wipe provided, and also stops release builds from blindly reusing a link made by another binary at the same commit. The regression test now uses a JS watcher instead of a sh script so it runs on Windows as well. harness bunEnv now strips NODE and npm_* the same way it strips NODE_ENV and BUN_DEBUG_*: `bun run` exports them to its script, so a runner started via `bun bd test` leaks them into every spawned bun, where NODE/npm_node_execpath suppress shim creation and most npm_* values are only set when absent. This replaces the per-file helper in bun-install-lifecycle-scripts.test.ts and also fixes bunx.test.ts's npm_config_user_agent test under `bun bd test`.
|
Updated 3:29 AM PT - Aug 11th, 2026
✅ @robobun, your commit b5437a265270fcb4db0a24db1b0694a9a5e952dd passed in 🧪 To try this PR locally: bunx bun-pr 37384That installs a local version of the PR into your bun-37384 --bun |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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.
Inline comments:
In `@src/install/lib.rs`:
- Around line 692-699: Update the hard-link replacement logic around image_stat
and the existing-link handling so an unavailable executable identity is treated
as a match/keep condition. When openat_os_path or stat returns None, do not
remove or recreate an existing hard link; retain the current replacement
behavior only when the image identity is known and confirmed different.
In `@test/cli/install/bun-run.test.ts`:
- Around line 1144-1155: Update the watch.js loop in the install test to include
the sibling-process exit state as an additional termination condition, ensuring
the child stops if the parent fails before writing the stop file. Preserve the
existing Bun.which("node") validation and success behavior while preventing an
unbounded busy loop.
In `@test/harness.ts`:
- Around line 121-124: Update the environment-key filtering in the harness
cleanup block to match the “npm_” prefix case-insensitively, removing both
lower- and upper-case npm-prefixed variables such as NPM_CONFIG_REGISTRY while
preserving the existing NODE handling.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e6721d5a-c1fc-4604-85c5-3732adea8306
📒 Files selected for processing (4)
src/install/lib.rstest/cli/install/bun-install-lifecycle-scripts.test.tstest/cli/install/bun-run.test.tstest/harness.ts
…stat'ed Only a link known to be a different file is replaced; if the identity of the running executable is unavailable, the existing link is left alone, as before. The watcher fixture in the race test also gets a deadline so a killed test run cannot leave it spinning.
There was a problem hiding this comment.
My earlier comment at src/install/lib.rs:727 is resolved — 0ac0025 switched the check to is_none_or, so an unknown image_stat now keeps the existing link. I reviewed this pass and found no bugs. A human look would still be worthwhile: the Windows ALREADY_EXISTS arm is now a stat-and-maybe-replace on release builds too (previously a pure no-op), and the bunEnv strip in test/harness.ts touches every test file.
What was reviewed:
- Windows loop state machine (
made_dir/replaced) — every path terminates;replacedbounds delete/retry to once per name. is_none_or+ inner.is_ok_and: link kept when image stat fails; only replaced when identity is known-different.bunEnvstrip scoped toNODE/lowercasenpm_*— matches whatbun runexports; CI never had these so no CI behavior change.- New test: watcher awaits its own shim line before siblings start;
await usingkills it on failure; assertions cover both watcher and sibling exit codes.
Extended reasoning...
Overview
This PR removes the #[cfg(bun_debug)] delete_tree_absolute wipe of the shared bun-node-* shim directory in both the POSIX and Windows arms of RunCommand::create_fake_temporary_node_executable, and replaces the Windows ALREADY_EXISTS => {} no-op with a file-identity check (st_dev/st_ino) that deletes and recreates only a link known to point at a different binary. It also strips NODE and lowercase npm_* from bunEnv/ciEnv in test/harness.ts, fixes two lifecycle-script tests to spawn with {...testEnv, PATH: ""} instead of mutating the shared env, and adds a concurrent shim-race test in bun-run.test.ts.
Prior review
I left an inline comment on an earlier commit noting that is_some_and on a None image_stat would still delete-and-recreate. That was fixed in 0ac0025: the check is now let keep = image_stat.as_ref().is_none_or(|image| ...), so keep = true when the running image cannot be stat'ed and the existing link is left alone. The current diff reflects this. The comment-cop and CodeRabbit threads are all resolved.
Security risks
None identified. The shim directory ownership/mode check on POSIX is unchanged; the Windows path adds a read-only open + stat of an existing hard link. No user input reaches the changed code.
Level of scrutiny
Medium-high. The POSIX side is a strict deletion of debug-only code plus a one-line comment. The Windows side is new control flow (a loop with two boolean guards) that now runs on release builds as well — before this PR, release Windows hit ALREADY_EXISTS => {} and never touched an existing link. The PR description argues this is a correctness improvement (stops release Windows reusing a link at a different binary of the same commit), and the replaced guard bounds the delete to once per name, but it is a runtime behavior change on a path every bun run --bun and lifecycle script goes through. The test/harness.ts change affects the environment of every spawned subprocess in the test suite; the reasoning (only strips what bun run leaks, CI never had these) is sound and was verified on both platforms per the PR thread.
Other factors
The new test is well-constructed: it awaits the watcher's first stdout line (the shim path, asserted to match /bun-node.../node) before starting siblings, uses await using for cleanup, drains stderr, and asserts both the watcher's and siblings' exit codes. The PR description documents 6/6 fail-before on both platforms with bun bd and a manual fsutil hardlink list check of the Windows identity path. The two lifecycle-script test fixes correct a real bug (one variant spread testEnv before mutating env.PATH, the other passed raw env).
Given the release-Windows behavior change and the harness-wide env change, I'm deferring rather than approving so a maintainer can confirm both are intended.
|
The same race showed up again at 5ff2d9f. Main still has both Probe: a package.json script This branch now conflicts with main in |
|
This is Claude (an AI assistant) writing on Jarred's behalf. Included in #44968. |
Repro
Debug builds only (
bun bd). With a package.json scriptprobe: node -e "console.log(typeof Bun)", start a handful ofbun-debug run --bun probeat the same time. About 1-2% of them printundefined(the script fell through to the real node) or fail withnode: not found:Any test file that runs several
--bunprocesses or PATH-less lifecycle scripts concurrently (bun-run.test.tsandbun-install-lifecycle-scripts.test.tsare bothdescribe.concurrent) can hit this underbun bd test, on Linux and on Windows. Release builds never wiped the directory, so the race itself is debug-only.Cause
RunCommand::create_fake_temporary_node_executable(src/install/lib.rs) puts thenodeandbunlinks in one directory shared by every running bun of the same build:<tmp>/bun-node-<sha>, or the single fixed<tmp>/bun-node-debugfor debug builds. Under#[cfg(bun_debug)]both the POSIX and the Windows arm randelete_tree_absoluteon that directory before recreating it, on every process start. A process starting up therefore removed the links a sibling had already planted and put on its scripts' PATH. (On NTFS, deleting a hard link to a running image succeeds as long as another link to it remains, so the Windows wipe removes live links as well; the test below fails on Windows against main too.) Depending on timing the sibling's script found nonodeat all, or the sibling's ownsymlink()failed with ENOENT because the directory was gone between itsmkdirandsymlink, in which case it silently went on without the shim directory on PATH.The wipe existed so that links left behind by an earlier debug binary would not be reused blindly.
Fix
Drop the wipe in both arms and have the "link already exists" path verify the link instead, so the only thing ever removed from the directory is a link that belongs to some other binary:
readlinks the link and re-points it once if it targets a different path, and a rebuilt binary at the same path is picked up through the symlink. Only the wipe is removed.st_dev/st_ino, volume serial plus file index, the same checknode_fs.rsuses to detect copying a file onto itself) of the existing link with the running image, reuses it on a match, and otherwise deletes that one link and creates it again, mirroring the POSIXreplacedloop. A rebuilt binary is a new file, so this keeps the protection the wipe provided, and it also stops Windows release builds from reusing a link made by a different binary at the same commit. The mkdir-and-retry on other errors is unchanged.Concurrent starts of the same binary now never remove anything. Two different binaries sharing the directory still re-point the links back and forth, which is the pre-existing release behavior on POSIX.
Test environment
The
bun-install-lifecycle-scripts.test.tsfailures that prompted this had a second, unrelated cause.bun bdgoes throughbun run, which exportsNODE,npm_node_execpathand a set ofnpm_*variables to its script, and the harnessbunEnvspreadsprocess.env, so every bun spawned by a test inherits them.EnvLoader::get_node_pathhonorsNODE/npm_node_execpathbefore looking at PATH, sobun installwithPATH=""never created the shims. Mostnpm_*values are only set when absent, which is also why thenpm_config_user_agenttest inbunx.test.tsfails underbun bd teston main.bunEnvnow stripsNODEandnpm_*next to the existingNODE_ENV/BUN_DEBUG_*strips. CI startsbun testdirectly and never had these variables, so this changes local runs only. The two PATH-less lifecycle tests now spawn with{ ...testEnv, PATH: "" }; before, one variant spread the env before PATH was cleared and the other spawned with the raw env, so neither waiter-thread variant tested what it said.Verification
New test in
test/cli/install/bun-run.test.ts: onebun run --bunwhose script runs asnodethrough the shim, prints whatnoderesolves to, and keeps re-resolving it until told to stop, while four morebun run --bunprocesses start up and runnodethrough the shims. The watcher re-resolves about 40k times a second in a debug build and a wipe leaves the link missing for a whole delete-tree, mkdir and relink cycle, so every wipe is caught: against main'slib.rsit failed 6/6 on Linux and 6/6 on Windows (plus roughly 200 runs of earlier variants of the same shape, some under load) withand with this branch it passes 6/6 on both (debug builds). Release builds have no wipe, so in CI the test passes before and after; the fail-before is specific to
bun bd.The Windows identity check was also exercised directly: with
%TEMP%\bun-node-debug\node.exereplaced by a hard link tocmd.exe, onebun-debug run --bunre-pointed it atbun-debug.exe(checked withfsutil hardlink list) and left the still-correctbun.exelink alone.Also green with
bun bd test:bun-run.test.ts(Linux 293, Windows 403),bun-install-lifecycle-scripts.test.ts(118 on Linux, 3 of which failed before; the two PATH-less tests on Windows), thenpm_config_user_agenttest inbunx.test.tson both platforms (failed locally before), andtest/regression/issue/26207.test.ts.Related: #35565 also removes the POSIX wipe, as one part of re-keying the directory per user. This PR is only the race fix and can land independently of that design change.
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-lifecycle-scripts.test.ts test/cli/install/bun-run.test.ts