Conversation
…ld is still not found `make_path_with` steps back one component on ENOENT and forward on EEXIST or success. When a parent exists but cannot hold children (a dangling symlink, procfs, a regular file on Windows), the walk repeats the same two mkdir calls forever. Every caller of `make_path` and `make_open_path` spins: `bun build --outdir`, `Bun.build`, `--compile --outfile`, `NODE_COMPILE_CACHE`, `enableCompileCache`, and `BUN_RUNTIME_TRANSPILER_CACHE_PATH`. Once the walk has advanced forward, a `NotFound` is now final and the child's error is returned.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 5 included reviews per hour; 3 remain after this review. WalkthroughThe path walker now terminates after forward progress when it encounters ChangesDangling symlink error handling
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Status: diff is green; ready for review. #36162 (same guard plus an install cache directory policy change) is closed in favor of this PR. CI build 106806 (head 1edaa7e): 180 of 182 jobs passed. The two red jobs do not touch this diff:
Both are reported to main-break triage. Everything else that failed passed on retry or when run alone (flake). Earlier builds: 106564 (181/182, url.test.ts), 106623 (179/181, url.test.ts and a Windows RSS flake in require-cache.test.ts). Reproduced with: USE_SYSTEM_BUN=1 bun test test/bundler/cli.test.ts -t dangling # 1.4.1: times out (spins)
USE_SYSTEM_BUN=1 bun test test/bundler/bun-build-api.test.ts -t dangling # 1.4.1: times out (spins)
USE_SYSTEM_BUN=1 bun test test/js/node/module/node-module-module.test.js -t dangling # 1.4.1: times out (spins)
USE_SYSTEM_BUN=1 bun test test/cli/install/bun-install-cache-dir.test.ts # 1.4.1: times out (spins)
bun bd test <same four files> # passThe same |
`<&str>::from(E)` yields the bare variant name (`NOENT`) on Windows, where `E` is not an alias of `SystemErrno`. Route through `get_error_code_tag_name` so the message reads `ENOENT` on every platform.
|
Updated 4:35 AM PT - Aug 27th, 2026
❌ @robobun, your commit 1edaa7e has 2 failures in
🧪 To try this PR locally: bunx bun-pr 40600That installs a local version of the PR into your bun-40600 --bun |
There was a problem hiding this comment.
LGTM — the advanced guard is the minimal fix and the walk still terminates correctly on every path I could trace.
Checked: traced make_path_with through the normal back-then-forward case, the dangling-symlink Exists→NotFound case, the libarchive FILE_OPEN_IF Created-only caller, and the concurrent-delete race — all terminate with the expected result. Verified all three make_path_with call sites (sys/lib.rs posix + windows, libarchive/lib.rs) map their errno results consistently with the new guard. Confirmed get_error_code_tag_name returns the SystemErrno variant name ("ENOENT") so the Windows message fix is right. Tests follow harness conventions (tempDir, subprocess spawns for the hang repros, pipes drained concurrently, stdout asserted before exitCode) and all referenced imports are present.
Extended reasoning...
Overview
This PR fixes an infinite-loop regression in bun_paths::make_path_with, the shared back-then-forward recursive-mkdir walk behind Dir::make_path, --outdir, NODE_COMPILE_CACHE, and libarchive extraction. When a path component reports EEXIST but its child reports ENOENT (dangling symlink, /proc, file-in-path on Windows), the walk previously oscillated forever. The fix adds a single advanced flag: once any prefix has reported Created or Exists, a subsequent NotFound returns the error immediately instead of stepping back. A secondary one-line fix in NodeCompileCache.rs switches errno_name to get_error_code_tag_name() so Windows renders ENOENT instead of the bare enum-variant name NOENT. Four Rust unit tests exercise the walk directly with mock closures; three JS integration tests spawn subprocesses to cover Bun.build({ outdir }), bun build --outdir, and enableCompileCache() under a dangling symlink.
Security risks
None. This is a filesystem directory-creation helper; the change only makes it terminate sooner on a case that previously spun forever. No new inputs are accepted, no path validation is loosened, and the error is surfaced rather than swallowed. The errno_name change is a display-string lookup with no behavioral effect beyond the message text.
Level of scrutiny
Low-to-moderate. The production change is ~3 lines of logic in a well-factored helper with a closed set of three callers, all of which I read and confirmed map their syscall results onto MakePathStep in the way the new guard expects (in particular, the libarchive Windows caller never returns Exists, which is exactly why the guard keys on advanced rather than "saw Exists"). I traced the algorithm through the happy path (back up N times, then create forward N times), the dangling-symlink case, the first-call-succeeds case, and the missing-root case — all terminate correctly. The one behavioral edge is a concurrent-delete race (parent created, then deleted by another process before the child mkdir): this now errors instead of retrying, which is acceptable and matches the PR's stated intent.
Other factors
Test quality is high: Rust unit tests assert the exact call sequence and include a < 100 runaway-loop guard so a regression fails rather than hangs cargo test; JS tests spawn the hang-prone repros in child processes so the runner survives, drain stdout/stderr/exited concurrently, and assert output before exit code per repo convention. All imports used by the new tests (symlinkSync, join, tempDir, fs, path) are already present in each file. No CODEOWNER paths are touched. The github-actions[bot] inline comments on earlier revisions were followed by two commits before the current head. Exit reason was dry_streak.
bun install goes through the same recursive mkdir walk for its cache directory. The release binary spins there forever. With the guard the walk fails with ENOENT and the install falls back to node_modules/.cache as 1.3.x did.
6885583 to
ed44efa
Compare
There was a problem hiding this comment.
LGTM — the advanced guard is a strict improvement (infinite loop → error), and the termination argument checks out.
Checked the walk against the normal back-then-forward case and the create-race the doc comment mentions — both still succeed; only a second NotFound after a Created/Exists parent is now fatal, which is exactly the case that used to oscillate. Verified get_error_code_tag_name() returns the SystemErrno variant name so Windows now yields ENOENT. The new bun-install-cache-dir.test.ts uses a file: folder dep so it stays off the network, and the standalone file matches the per-feature pattern already used in test/cli/install/.
Extended reasoning...
Overview
The PR fixes an infinite loop in bun_paths::make_path_with, the shared back-then-forward recursive-mkdir walker behind Dir::make_path, Dir::make_open_path, and the libarchive extraction path. When a parent reports Exists/Created but its child still reports NotFound (dangling symlink, /proc/nonexistent/x, a regular file on Windows), the old walk stepped back on NotFound and forward on Exists forever. The fix adds a single advanced flag: once the walk has moved forward past any Created/Exists prefix, a subsequent NotFound returns the error immediately. A secondary one-line fix in NodeCompileCache.rs swaps <&str>::from(e.get_errno()) for e.get_error_code_tag_name() so the Windows E enum yields ENOENT rather than NOENT. Four Rust unit tests exercise the walker directly (including a runaway-loop assertion) and four JS integration tests spawn subprocesses for each user-visible entry point that previously hung.
Security risks
None. This is a pure control-flow termination fix in a filesystem helper; no new inputs are trusted, no validation is loosened, and the only behavioral change replaces an infinite loop with the same ENOENT error 1.3.x produced. The libarchive caller is unaffected in the success direction — extraction that previously worked still works; only the previously-nonterminating case now returns an error.
Level of scrutiny
Moderate. make_path_with is a shared primitive with three callers (src/sys/lib.rs, src/paths/lib.rs, src/libarchive/lib.rs), but the change is three lines whose correctness is verifiable by case analysis: before advanced is set the walk behaves identically to the old code; after it is set, the immediate parent of the current component just reported Created/Exists, so stepping back on NotFound would deterministically revisit that same parent and re-advance — returning the child's error is the only progress-making choice. The unit test make_path_creates_missing_parents pins the exact attempt sequence for the healthy path, and the two loop tests bound attempts at 100 so a regression fails fast rather than hanging the test binary.
Other factors
The new commit since the prior review (ed44efa1) only adds the bun-install-cache-dir.test.ts integration test; the core fix is unchanged. No CODEOWNERS cover the touched paths. Tests follow harness conventions (tempDir, bunExe/bunEnv, await using, concurrent pipe drains, output asserted before exit code, subprocess isolation so a regression to the hang cannot take down the runner). The install test uses a local file: dependency to stay off the network per the hermeticity rule. The github-actions inline threads on component_iterator.rs were author-self-resolved, but the subsequent 8582e308 "shorten the make_path_with docs" commit plausibly addressed them and no human reviewer has an outstanding CHANGES_REQUESTED.
The guard fires on the first missing component after the walk has advanced, not only on the original target. /proc/nonexistent/out and dangling/cc/<tag> both take that route: the target and its parent are missing, and the first existing ancestor cannot hold children. A guard that only made the original target final would pass the other tests and still spin here.
…NOENT (#40602) ### Problem - On Windows, user-visible messages spell the errno without the `E` prefix. `bun install` with `"workspaces": ["C:/missing/*"]` prints `Failed to run workspace pattern C:/missing/* due to error NOENT`. POSIX prints `ENOENT`. - Cause: on Windows `bun_errno::E` is its own enum with unprefixed variant names (`NOENT`), and its `strum::IntoStaticStr` derive made `<&str>::from(e)` return that name (`src/errno/windows_errno.rs:95`). On POSIX `E` is `type E = SystemErrno`, so the same expression returns `ENOENT`. Nine messages built their name with `<&'static str>::from(err.get_errno())`. ### Fix - Replace the derive with `impl From<E> for &'static str` that returns `SystemErrno`'s name for the same discriminant. The two enums share one discriminant set (`SystemErrno::to_e` casts the other way, and all 138 dense variants match). Every `<&str>::from(E)` on Windows now prints `ENOENT`. The unused `strum::EnumString` derive goes too. - The nine sites switch to `bun_sys::Error::name()` (`src/sys/Error.rs:321`), the accessor the rest of the tree uses. It also translates libuv-sourced errnos and reports an unmapped value as `UNKNOWN`, which `get_errno()` cannot. - The two `bun:internal-for-testing` hooks that print an `E` (`src/sys_jsc/error_jsc.rs`) keep doing so. Their Windows-only tests now expect `EPERM`, `ENOENT`, `EUNKNOWN`. - Verified on windows-x64: `bad-workspace.test.ts` (new test), `translate-uv-error-windows.test.ts` and `rm-windows-ntstatus.test.ts` fail with bun 1.4.1 and pass with the debug build. Also `error-name-from-libuv.test.ts`, and `cron.test.ts` on Linux. ### Background - `bun_sys::Error::get_errno()` returns the errno as `E` for `match` arms. `name()` returns the user-visible code. - On Windows, `E` (bare names) and `SystemErrno` (E-prefixed names) are two `#[repr(u16)]` enums with the same discriminants. `pub const ENOENT: E = E::NOENT` aliases let cross-platform code compile. POSIX has no second enum. - strum's `IntoStaticStr` derive turns an enum value into its Rust variant name. <details><summary>Notes</summary> - Repro with the released bun 1.4.1 on windows-x64: ``` > echo { "name": "root", "workspaces": ["C:/nonexistent-dir-xyz/*"] } > package.json > bun install error: Failed to run workspace pattern C:/nonexistent-dir-xyz/* due to error NOENT ``` With this change the same command prints `due to error ENOENT`, as Linux does. - The nine sites: `src/install/lockfile/Package/WorkspaceMap.rs:456,472,488`, `src/install/lifecycle_script_runner.rs:389`, `src/runtime/api/cron.rs:192,396,1128`, `src/runtime/cli/test/parallel/Coordinator.rs:934`, `src/runtime/api/bun/spawn/stdio.rs:175`. A tenth, `src/jsc/NodeCompileCache.rs:162`, is left as is: the `From<E>` impl corrects its output, and #40600 switches it to `get_error_code_tag_name()`. - `stdio.rs:175` is inside `#[cfg(linux)]`, so its output was already right. It is changed for consistency with the other sites. - `EUNKNOWN` in the hook tests is `SystemErrno::EUNKNOWN`'s current name, the same string `err.code` carries today. #39340 renames it to `UNKNOWN` and should update these assertions with the rest. - `cargo test -p bun_errno` gains `e_spells_like_system_errno`. It is trivially true on POSIX and checks the new impl on a Windows host. `cargo test` for one crate does not link on Windows today (bun_core needs the C++ symbols), so the hook tests are the Windows coverage in CI. - The bug is Windows-only. On Linux every test in this PR passes with and without the change. - Edge cases where `name()` and the old expression differed on Windows: an error with `from_libuv` set printed `UV_ENOENT`, `name()` prints `ENOENT`. An unmapped discriminant printed `SUCCESS`, `name()` prints `UNKNOWN`. - `bun-install-lifecycle-scripts.test.ts` on Linux: 119 pass, 3 fail because `node` is not on PATH in the container (`node: command not found`). Unrelated to this change. - A first version of this PR added a source lint that banned `str>::from(...get_errno())`. A review pointed out that the spelling is a property of the type and that the lint had no positive control and missed the let-bound form, so the type-level fix above replaced it. </details> <!-- robobun:evidence:begin --> --- **no test proof** · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/node/fs/translate-uv-error-windows.test.ts, test/js/node/fs/rm-windows-ntstatus.test.ts, test/cli/install/bad-workspace.test.ts <!-- robobun:evidence:end -->
Problem
/proc, or a regular file on Windows.bun build --outdir=./dangling/outnever exits.NODE_COMPILE_CACHE=./dangling/cc bun app.jshangs before user code runs.Bun.build,--compile --outfile,enableCompileCache(), andbun installwith such aBUN_INSTALL_CACHE_DIRhit the same loop.make_path_withinsrc/paths/component_iterator.rs:203. It steps back on ENOENT and forward on EEXIST or success, with no check that the retry can do better. strace showsmkdirat("dangling/out") = ENOENTthenmkdirat("dangling") = EEXIST, 22,000 times per second.Fix
NotFoundis final. The parent just reportedCreatedorExists, so stepping back can only repeat the same two results. The walk returns the child's ENOENT, as 1.3.14 did.Exists: the Windows libarchive variant usesNtCreateFile(FILE_OPEN_IF)and reportsCreatedfor an existing directory too.test/bundler/cli.test.ts,test/bundler/bun-build-api.test.ts,test/js/node/module/node-module-module.test.js, and the newtest/cli/install/bun-install-cache-dir.test.ts. 1.4.1 times out on all four. Also thebun_pathsunit tests,archive.test.ts,transpiler-cache.test.ts.Background
make_path_withis the onemkdir -pwalk behindDir::make_path,Dir::make_open_path, and the libarchive UTF-16 variant. The caller supplies the per-prefix mkdir as a closure that returnsCreated,Exists, orNotFound.node:fsmkdirSync({ recursive: true })is not affected. It has its own forward pass innode_fs.rs.Notes
paths: terminate recursive mkdir walk when a confirmed parent still yields ENOENT #36162 carried the same three line change to
make_path_withplus a policy change to thebun installcache directory: error out when an explicitBUN_INSTALL_CACHE_DIRor bunfiginstall.cache.dircannot be created, warn and fall back for an implicit one. It was conflicting with main and is closed in favor of this PR. The policy change is a separate proposal. This PR keeps the existing behavior: the walk returns ENOENT andensure_cache_directoryfalls back tonode_modules/.cache, as 1.3.x did.test/cli/install/bun-install-cache-dir.test.tscovers that entry point with afile:folder dependency, so it stays off the network.Reproduction with the released binary (Linux and Windows both spin):
With the fix, each form terminates at once. 1.3.14 printed
Failed to create output directory FileNotFoundfor the first one.BUN_RUNTIME_TRANSPILER_CACHE_PATHgoes through the samemake_open_pathand is fixed too.The hang in Windows:
bun installinfinite-loops (isolated) or silently corrupts bun.lock (hoisted) when the CWD drive letter differs from the real path #39357 (bun installthrough a subst drive or cross-drive junction on Windows) is the same loop: the isolated linker builds a path with a bogusC:component in the middle,NtCreateFilereportsSTATUS_OBJECT_NAME_INVALID, which maps to ENOENT, and the walk bounces against the existing parent. This guard turns that hang into an error. It does not close Windows:bun installinfinite-loops (isolated) or silently corrupts bun.lock (hoisted) when the CWD drive letter differs from the real path #39357: the root cause there, the project root resolved through a different drive letter, is thePackageManager.rschange in Fix bun install through subst drives, cross-drive junctions, and symlinked package.json #39361.Fix bun install through subst drives, cross-drive junctions, and symlinked package.json #39361 also carries a guard in
make_path_with: alast_not_foundmarker that is cleared onCreated. The libarchive Windows walker (make_path_u16insrc/libarchive/lib.rs) usesNtCreateFile(FILE_OPEN_IF)and reportsCreatedfor a directory that already exists, so that marker is cleared on every step back and the walk does not terminate there. Theadvancedflag here is set onCreatedandExistsalike. This PR should land first and Fix bun install through subst drives, cross-drive junctions, and symlinked package.json #39361 should drop itscomponent_iterator.rshunk and themake_path_terminates_on_uncreatable_componenttest.The unit test
make_path_stops_on_a_middle_component_after_two_steps_backpins the/proc/nonexistent/outanddangling/cc/<tag>shape: the target and its parent are both missing, and the first existing ancestor cannot hold children. The guard fires on the middle component, so the returned error is that component's. A guard that only made the original target final passes the other unit tests and still spins on this shape.The compile cache test exposed a second, Windows-only defect:
enableCompileCachereportedCannot create cache directory: NOENT. On WindowsEis its own enum with bare variant names, so<&str>::from(e.get_errno())drops theE.errno_nameinNodeCompileCache.rsnow usesget_error_code_tag_name, the accessor behindsys::Error::name(). errno(windows): spell E like SystemErrno so messages say ENOENT, not NOENT #40602 fixes theFrom<E> for &strimpl itself and the other nine call sites. The two merge in either order.The old Zig
std.fs.Dir.makePathstat'ed the path on EEXIST and returned the stat error, which is how 1.3.14 reported FileNotFound. A stat does not help for/proc/nonexistent/x(the parent is a real directory), so this PR detects the lack of progress in the walk itself. It also saves the extra syscall on every EEXIST.A race where another process creates the missing parent between the child's ENOENT and the parent's mkdir still works: the parent reports
Exists, the walk moves forward, and the child is created on the retry. Only a second ENOENT on the same child after that is fatal.The
bun-build-api.test.tsrun also shows two bytecode tests that time out at 5 s under this debug+ASAN build. Their fixture completes in 28 s when run by hand, and the output directory is created fine. That is the slow machine, not this change.no test proof · iteration 4 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/node/module/node-module-module.test.js, test/bundler/cli.test.ts, test/bundler/bun-build-api.test.ts