paths: terminate recursive mkdir walk when a confirmed parent still yields ENOENT - #36162
paths: terminate recursive mkdir walk when a confirmed parent still yields ENOENT#36162robobun wants to merge 13 commits into
Conversation
…ENOENT The back-then-forward mkdir walk assumed ENOENT on a component always means "parent missing". When the parent is a dangling symlink (or any path that exists but cannot host children), the leaf returns ENOENT, the parent returns EEXIST, and the walk oscillates between the two forever. bun install with BUN_INSTALL_CACHE_DIR under a dangling symlink burned a full core issuing ~50k mkdirat/s with no output and no timeout. Once the walk has advanced forward (parent confirmed Created/Exists), treat a subsequent NotFound as terminal and return the error. This matches the distinct forward pass in node_fs::mkdir_recursive_os_path_impl, where ENOENT after a confirmed parent is already fatal.
|
Status: diff is green; ready for review. Reproduced with: USE_SYSTEM_BUN=1 bun test test/cli/install/bun-install-cache-dir.test.ts # all 4 spin → timeout
bun bd test test/cli/install/bun-install-cache-dir.test.ts # all 4 pass in ~200-300msBehavior on an uncreatable cache dir:
CI (build 83770): the only failed job is |
|
Updated 9:18 PM PT - Jul 27th, 2026
❌ @robobun, your commit 2c2c34b has 1 failures in
🧪 To try this PR locally: bunx bun-pr 36162That installs a local version of the PR into your bun-36162 --bun |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Warning Review limit reached
Next review available in: 1 minute Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Walkthrough
ChangesPath walk termination
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@test/cli/install/bun-install-cache-dir.test.ts`:
- Line 15: Remove the per-test timeout configuration from the test declaration
for “bun install exits instead of spinning in mkdirat”. Keep the child-process
timeout guard unchanged and rely on the test runner’s default timeout.
- Around line 6-13: Update the regression-test comments around the
dangling-symlink coverage to contain only the confirmed issue URL; remove the
explanatory behavior, fallback, and Windows-skip comments while preserving the
test implementation.
🪄 Autofix (Beta)
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: 44680663-270b-4f10-9dad-7dfd5c494d9a
📒 Files selected for processing (2)
src/paths/component_iterator.rstest/cli/install/bun-install-cache-dir.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — tight 3-line termination guard on the shared mkdir walk, with unit + integration coverage. The doc-comment nit inline is a comment-cop artifact, not a blocker.
Extended reasoning...
Overview
This PR fixes an infinite mkdirat loop in bun_paths::make_path_with, the shared back-then-forward walk driving bun_sys::mkdir_recursive_at (POSIX + Windows) and the libarchive u16 variant. When a path component exists (e.g. a dangling symlink) but its child returns ENOENT, the old walk oscillated forever between "parent exists → step forward" and "child ENOENT → step back". The fix adds a single advanced flag: once any step has moved forward, a subsequent NotFound returns the error instead of stepping back. Net logic change is 3 lines; the rest is two Rust unit tests and a spawned-install integration test.
Security risks
None. Pure path-walk state machine; no untrusted input parsing changed. The fix strictly reduces the state space — it converts an unbounded loop into an error return. It cannot make a previously-succeeding case loop.
Level of scrutiny
Medium — this is a shared low-level helper with several callers, so I traced them: mkdir_recursive_at_mode (posix at src/sys/lib.rs:2404, windows at :4025), Dir::make_path/make_open_path, File::open_mkdirs_at, and the libarchive Windows extractor. All map EEXIST → Exists and ENOENT → NotFound, so the new guard applies uniformly. The only behavioral delta beyond the bug case is a theoretical TOCTOU race (another process deletes a just-created intermediate dir mid-walk) — previously that would retry once, now it returns ENOENT. That matches Node's mkdir({recursive:true}) forward-pass semantics and is unfixable in general anyway; the infinite spin it replaces is strictly worse.
Other factors
- The
make_path_walks_back_then_forwardunit test explicitly proves the normal deep-create path is unchanged, andmake_path_terminates_...asserts exactly 3 calls for the dangling-symlink shape. - The integration test is a proper hang-guard: spawns with a 15s subprocess timeout, asserts
signalCode === nullfirst (so a regression shows as SIGTERM, not a generic timeout), drains both pipes concurrently, usestempDir/bunEnv, and points at127.0.0.1:1so no external network.skipIf(isWindows)is reasonable since POSIX symlink semantics don't map cleanly and the Windows path is covered by the shared unit test. - All prior bot feedback (comment-cop, coderabbit) is resolved. The one remaining nit — stale doc comments on
MakePathStep::NotFound/make_path_with— is the direct result of comment-cop rejecting three successive attempts to update them; the code is ~10 lines below and self-explanatory.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/cli/install/bun-install-cache-dir.test.ts (1)
18-36: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftMake the regression assertion specific to the cache-path failure.
Because the registry is intentionally unreachable, any ordinary registry/network failure can satisfy the generic
"error"and non-zero-exit assertions—even ifBUN_INSTALL_CACHE_DIRis ignored or the dangling-symlink mkdir path is never exercised. Use a hermetic fixture that proves cache initialization was reached, or assert a cache-specific error/fallback filesystem effect.As per coding guidelines, tests must prove they fail for the intended reason.
🤖 Prompt for 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. In `@test/cli/install/bun-install-cache-dir.test.ts` around lines 18 - 36, Strengthen the regression test around the Bun.spawn install flow so it verifies the dangling BUN_INSTALL_CACHE_DIR path was actually initialized, rather than relying on generic registry errors and a nonzero exit. Add a hermetic cache-path assertion or fixture that distinguishes cache-directory handling from the intentionally unreachable BUN_CONFIG_REGISTRY failure, while preserving the existing process cleanup and outcome checks.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@test/cli/install/bun-install-cache-dir.test.ts`:
- Around line 18-36: Strengthen the regression test around the Bun.spawn install
flow so it verifies the dangling BUN_INSTALL_CACHE_DIR path was actually
initialized, rather than relying on generic registry errors and a nonzero exit.
Add a hermetic cache-path assertion or fixture that distinguishes
cache-directory handling from the intentionally unreachable BUN_CONFIG_REGISTRY
failure, while preserving the existing process cleanup and outcome checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f69d0010-aee2-417d-a79b-265fcb38116b
📒 Files selected for processing (2)
src/paths/component_iterator.rstest/cli/install/bun-install-cache-dir.test.ts
ensure_cache_directory previously swallowed the error and silently fell back to node_modules/.cache. With the mkdir walk now returning ENOENT on a dangling-symlink parent, surface that error to the user: print 'error: cache directory "<path>" is not creatable: <errno>' and exit. The fallback loop is dropped; when Enable::CACHE is already off the node_modules/.cache branch is unchanged. Adds a runtime auto-install test variant alongside bun install.
…lback for implicit defaults An unwritable $HOME/.bun/install/cache (root-owned after a sudo install, read-only $HOME in containers) previously fell back silently to node_modules/.cache. Keep that working: CacheDir now carries is_explicit, set only for BUN_INSTALL_CACHE_DIR and bunfig install.cache.dir. On open failure: explicit path -> error + exit; implicit default -> warn naming the path and errno, then fall through to node_modules/.cache. Also switch to bun_fmt::quote for the path per local convention.
There was a problem hiding this comment.
All prior review feedback has been addressed and the bug-hunting pass found nothing new. Deferring rather than approving because this touches a shared mkdir -p primitive (make_path_with, used by bun_sys::mkdir_recursive_at on POSIX/Windows and by libarchive extraction) and introduces a user-facing behavior change (explicit BUN_INSTALL_CACHE_DIR/--cache-dir/bunfig now hard-fails instead of silently falling back) — worth a human sign-off.
What was reviewed:
- Traced the
advancedguard through the normal back-then-forward walk, the dangling-symlink case, and a concurrent-delete race — the only behavior lost is retry-on-TOCTOU-delete, which is acceptable. - Checked all
make_path_withcallers (src/sys/lib.rs:2409,:4133,src/libarchive/lib.rs:1118) — all map ENOENT→NotFound/EEXIST→Exists, so the guard applies uniformly. - Verified
CacheDirhas no other construction sites and the secondfetch_cache_directory_pathcaller (bun pm cache rm) doesn't readis_explicit.
Extended reasoning...
Overview
Three files: (1) src/paths/component_iterator.rs adds a 3-line advanced guard to make_path_with so a NotFound after the walk has confirmed a parent returns the error instead of oscillating forever, plus two unit tests; (2) src/install/PackageManager/PackageManagerDirectories.rs restructures ensure_cache_directory from a loop/continue to straight-line, adds CacheDir::is_explicit, and branches error handling on it (explicit → error: + Global::crash(); implicit → warn: gated on log_level != Silent + fall through to node_modules/.cache); (3) a new 4-test file exercising env-var, bunfig, runtime auto-install, and the implicit-$HOME fallback.
Security risks
None. No untrusted-input parsing was added; the path is already user-controlled and only flows into mkdirat as before. The change tightens termination and error reporting.
Level of scrutiny
Medium-high. make_path_with is the single back-then-forward walk used by every recursive-mkdir path in the runtime — POSIX mkdir_recursive_at, the Windows NtCreateFile variant, and libarchive tar extraction. The logic change is small and I traced it against the normal walk (unchanged), the dangling-symlink case (now terminates in 3 calls, matching the unit test), and the concurrent-delete race (now returns ENOENT instead of retrying — arguably correct, but a behavior change on a shared primitive). Separately, the ensure_cache_directory restructure is a deliberate user-facing behavior change: an explicitly-configured-but-uncreatable cache directory now aborts bun install where it previously fell back silently. That matches REVIEW.md's "operations the user explicitly requested fail" rule and I raised the implicit-path-regression concern earlier (now addressed via is_explicit), but a maintainer should confirm the explicit-path hard-fail is the desired product behavior.
Other factors
This PR has been through five rounds of review feedback, all addressed: implicit-path fallback preserved (d2a2740), bun_fmt::quote for path quoting (d2a2740), dead cache_directory_path = b"" write removed (38ee46c), bunfig-spelling test added (a23676c), --silent gate on the warn (5456aa1), and doc-comment enumeration completed (2c2c34b). Tests use test.concurrent, tempDir, timeout: 15_000 as a hang guard with signalCode === null assertions, and a dead registry so no network is contacted. The suite is skipIf(isWindows) (dangling POSIX symlinks); Windows coverage of the primitive change relies on the cargo test unit tests plus CI.
|
Two more ways into the same loop, for the record. Both go through
Unrelated to the above: the branch currently conflicts with main in |
|
One more trigger for the same Seen on the windows-2019-x64 and windows-11-aarch64 lanes of #39295 (build 99109): a bake production test pre-created The |
2 participants
Problem
bun installand the runtime auto-installer spin forever at ~50kmkdirat/s, burning a full core with no output, when the resolved install cache directory is inside a path whose parent exists but cannot host children (e.g. a dangling symlink to an unmounted disk, or a procfs path).Repro
The raw loop:
Cause
bun_paths::make_path_withdrives the shared back-then-forwardmkdir -pwalk used bybun_sys::mkdir_recursive_at(POSIX and Windows) and the libarchiveu16variant. OnENOENTit steps back to the parent; onCreated/EEXISTit steps forward. When the parent is a dangling symlink the walk oscillates forever.fs.mkdirSync(path, {recursive: true})already throwsENOENTcleanly on the same path becausenode_fs::mkdir_recursive_os_path_implruns a distinct forward pass whereENOENTis fatal.Fix
make_path_with: once the walk has advanced forward (parent confirmedCreated/Exists), a subsequentNotFoundreturns the error instead of stepping back. The normal "walk back to the first existing ancestor, then forward" path is unchanged.ensure_cache_directory: whenmake_open_pathon the resolved cache directory fails, surface the error instead of silently disabling the cache.CacheDirnow carriesis_explicit, true only when the path came fromBUN_INSTALL_CACHE_DIRor bunfig'sinstall.cache.dir:error: cache directory "<path>" is not creatable: <errno>and exit.$BUN_INSTALL,$XDG_CACHE_HOME,$HOME, or the in-tree fallback): printwarn: cache directory "<path>" is not creatable: <errno>, falling back to node_modules/.cacheand continue. This preserves the existing fallback for unwritable$HOME/.bun(root-owned aftersudo, read-only$HOMEin containers).After:
Verification
USE_SYSTEM_BUN=1 bun test test/cli/install/bun-install-cache-dir.test.ts: all three tests fail (processes spin, test-runner timeout).bun bd test test/cli/install/bun-install-cache-dir.test.ts: all three pass in ~150-300ms, assertingsignalCode === nulland the exact error/warn text. The implicit-path test also assertsnode_modules/.cacheexists on disk and that resolution continued past cache init.cargo test -p bun_paths component_iterator: newmake_path_terminates_when_parent_exists_but_child_is_enoentunit test passes; positive-path walk test unchanged.bun bd test test/js/node/fs/fs-mkdir.test.tsandtest/cli/install/bun-install-retry.test.ts: still green.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-cache-dir.test.ts