install: fix ENOENT race on shared cache when RENAME_EXCHANGE is unsupported (NFS) - #36229
install: fix ENOENT race on shared cache when RENAME_EXCHANGE is unsupported (NFS)#36229robobun wants to merge 9 commits into
Conversation
WalkthroughChangesCache commit paths now use exclusive atomic renames and preserve existing destinations during concurrent installs. The system fallback renames destinations aside before replacement, and a Linux regression test simulates unsupported rename exchange operations. Concurrent install cache handling
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
On the bot findings above:
If #33979 or #33884 lands first I will rebase this onto it; the pieces compose (the fallback hardening and the NFS test remain relevant either way). |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/patch_install.rs`:
- Around line 613-621: Update the keep-existing branch in the patch installation
flow around rename handling so it only preserves an existing cache entry when
the directory exists and its final bun_hash_tag completeness marker is present.
Replace the bare directory_exists_at check with the appropriate sys::exists_at
check for that marker, allowing incomplete entries to remain replaceable while
preserving complete concurrent installs.
In `@src/sys/lib.rs`:
- Around line 9293-9310: Update the final Err branch of the rename retry logic
to delete the aside tree when the best-effort renameat restoration fails,
including when retries are exhausted and the destination remains occupied.
Preserve the existing successful restore behavior and return the original error
after cleanup.
In `@test/cli/install/bun-install-cache-race.test.ts`:
- Around line 24-67: Update the SHIM_C C source to guard all
SYS_renameat2-dependent logic with an `#ifdef` SYS_renameat2 check, so compilation
succeeds on older headers and the test can self-skip through its existing setup.
Preserve the current failure behavior when SYS_renameat2 is available, including
both syscall interception and renameat2 handling.
- Around line 123-160: Update makeRegistry to use Bun.serve with port: 0 instead
of node:http createServer. Adapt the request handler to Bun’s server API, obtain
the assigned port from the returned server for the tarball URL, and remove the
manual listen promise and createServer-specific lifecycle handling while
preserving the existing responses and async disposal.
🪄 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: 2d942bf1-2476-48dc-88e3-f22c2d0ec531
📒 Files selected for processing (4)
src/install/extract_tarball.rssrc/install/patch_install.rssrc/sys/lib.rstest/cli/install/bun-install-cache-race.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/sys/lib.rs`:
- Around line 9270-9274: Remove the delete_tree_at call from the no-space branch
in the rename flow around rename_aside_name. Ensure the destination is never
deleted in place: allocate or use sufficient storage to generate an aside name,
and if the path-length limit prevents that, return a catchable path-length error
before any filesystem side effect; otherwise preserve the aside-based rename
behavior.
🪄 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: 30209186-b5b2-496d-ae27-8a42b408b55f
📒 Files selected for processing (3)
src/install/extract_tarball.rssrc/install/patch_install.rssrc/sys/lib.rs
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/install/patch_install.rs (1)
604-629: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftRevalidate completeness inside the concurrent fallback.
Line 615 checks the tag before
renameat_concurrently. Two repairers can both see an incomplete entry; after one publishes a complete entry, the other's fallback can move it aside, replace it, and delete it. This breaks the first-wins contract and can again disrupt readers of the winning tree.Make the fallback re-check the displaced destination's tag after claiming it, restoring/discarding the source when it is complete, or add per-entry coordination that makes this decision atomic.
🤖 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 `@src/install/patch_install.rs` around lines 604 - 629, Update the concurrent fallback around renameat2 and the subsequent renameat_concurrently flow to re-check the displaced destination’s completion tag after claiming it. If the destination is complete, restore or discard the source without replacing the published tree; otherwise proceed with replacement and cleanup. Preserve the first-wins contract and avoid deleting or disrupting the winning entry.
🤖 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-race.test.ts`:
- Around line 182-186: Update the concurrent-install test around “concurrent
installs sharing a cache survive a filesystem without RENAME_EXCHANGE” to seed
the shared cache with an incomplete entry before starting installs, ensuring
concurrent repair exercises the rename-aside fallback. Configure the preload
shim to inject and assert at least one EOPNOTSUPP, and verify the test observes
that injected failure while still completing successfully.
---
Outside diff comments:
In `@src/install/patch_install.rs`:
- Around line 604-629: Update the concurrent fallback around renameat2 and the
subsequent renameat_concurrently flow to re-check the displaced destination’s
completion tag after claiming it. If the destination is complete, restore or
discard the source without replacing the published tree; otherwise proceed with
replacement and cleanup. Preserve the first-wins contract and avoid deleting or
disrupting the winning entry.
🪄 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: f9738e46-904e-4868-8dbd-cc9f3265a3b2
📒 Files selected for processing (3)
src/install/patch_install.rssrc/sys/lib.rstest/cli/install/bun-install-cache-race.test.ts
|
On the outside-diff review note about re-validating completeness inside the patch fallback: declined as out of scope. The repair path (entry exists but its completeness marker is missing) deliberately keeps last-wins semantics, since replacement is the only repair mechanism without per-entry locking. Two repairers racing over an already-corrupt entry was racy before this PR (in-place delete_tree) and is strictly narrower now (rename-aside). Making that decision atomic requires per-entry coordination (lock files), which the issue discussion notes is its own can of worms on NFS, and the healthy-entry path this PR fixes never touches a published tree. |
There was a problem hiding this comment.
I didn't find any bugs this pass (both of my earlier inline findings — the NAME_MAX guard in rename_aside_name and the isMusl skip on the shim test — were addressed), but this changes concurrency semantics in a shared syscall helper and the cache-population design, so it's worth a human look. Note also the flagged potential-duplicate PRs #33979 / #33884.
What was reviewed:
- First-writer-wins on rename collision in
extract_tarball.rs/patch_install.rs, with completeness gates (package.json/.bun-tag-<hash>) — matches the precedent incommit_global_store_entry. - The rename-aside fallback loop in
renameat_concurrently_without_fallback: retry bound, ENOENT/EEXIST handling, restore-on-error, and cleanup of the aside tree on every exit path. .is_ok()on the initialrenameat2(exclude)swallows non-EEXIST errors, but the fallthrough torenameat_concurrentlyre-surfaces them, so no error is lost.- LD_PRELOAD shim test: hermetic registry, per-process pipe drain, marker-file assertion in the repair test proving the fallback ran.
Extended reasoning...
Overview
Fixes #36227 — concurrent bun install processes sharing a cache on filesystems without RENAME_EXCHANGE (NFS/FUSE) hit ENOENT mid-copy because the loser's fallback deleted the winner's cache entry in place. Touches src/install/extract_tarball.rs and src/install/patch_install.rs (first-writer-wins on collision, gated by a completeness marker so corrupt entries stay repairable), src/sys/lib.rs (renameat_concurrently_without_fallback now renames the destination aside before deleting, with a bounded retry loop and best-effort restore on error), and adds a Linux-only LD_PRELOAD regression test.
Security risks
None identified. Paths are Bun-generated cache/temp names, not untrusted input; no auth, crypto, or network parsing changes.
Level of scrutiny
High. renameat_concurrently_without_fallback is a shared helper reached by move_file_z (bun build --compile, bun upgrade, Tmpfile::finish), the extract repair path, and patch commit/restore, on both POSIX and Windows. The concurrency reasoning (open dir fds surviving rename, first-writer-wins vs. rename-aside) is subtle and cross-process. This is a design decision — the PR chose keep-existing over per-entry lockfiles or delete-in-place — that a maintainer should sign off on, especially given github-actions flagged #33979 and #33884 as potential duplicates addressing the same race.
Other factors
All prior review threads (CodeRabbit and my two inline nits) are resolved: NAME_MAX guard added to rename_aside_name, isMusl added to the test skip, patch keep-existing gated on the .bun-tag marker, aside tree deleted when restore fails, Bun.serve({port:0}) used for the stub registry, and a repair test with a shim-injection marker was added. The PR states rust:check-all and the install/patch suites pass; the new test is Linux-glibc-only and the evidence block notes it was deferred to CI.
…ame collision Concurrent bun install processes sharing BUN_INSTALL_CACHE_DIR raced when the filesystem does not support renameat2(RENAME_EXCHANGE) (NFS, FUSE): losing the RENAME_NOREPLACE race fell back to delete_tree + rename, which unlinked the winner's cache entry files while another process was copying them into node_modules, failing with ENOENT. Cache entries only appear via an atomic rename of a complete tree, so on a rename collision keep the existing entry and discard the freshly extracted copy. An entry that exists but is missing package.json (a crashed copy) is still replaced, since replacement is its only repair path. The generic renameat_concurrently fallback also no longer deletes the live destination in place: it moves the destination aside, renames the source into place, then deletes the old tree under its aside name, restoring it if the rename fails. Fixes #36227
…estore, use Bun.serve in test
2504c93 to
5b1d90a
Compare
There was a problem hiding this comment.
No issues found this pass; the two nits I raised earlier (NAME_MAX guard in rename_aside_name, isMusl skip for the LD_PRELOAD test) are both addressed, and the repair test now asserts the shim actually injected EOPNOTSUPP via the marker file. That said, this reworks the fallback semantics of a shared syscall helper (renameat_concurrently_without_fallback — every non-Linux caller and every NFS/FUSE Linux caller now goes through the new rename-aside/retry loop) and flips cache publication from last-wins to first-wins, so it's worth a human look.
Checked: the retry loop cleans up the aside tree on every exit (success, EEXIST retry, error-with-restore, restore-failed); rename_aside_name guards both PATH_MAX and NAME_MAX; the .is_ok() on the initial renameat2(NOREPLACE) swallowing non-EEXIST errors is fine because the fallback renameat_concurrently_a re-attempts and surfaces the real error; patch_install.rs buntag-buffer hoist preserves the same bytes/NUL as before.
Extended reasoning...
Overview
Four files: src/install/extract_tarball.rs and src/install/patch_install.rs change cache-entry publication from unconditional renameat_concurrently to renameat2(NOREPLACE) → keep-existing-if-complete → fallback; src/sys/lib.rs replaces the sad-path in renameat_concurrently_without_fallback (delete-tree + rename) with a rename-aside/retry loop and adds rename_aside_name; a new 300-line Linux-only test uses an LD_PRELOAD shim to force RENAME_EXCHANGE → EOPNOTSUPP.
Security risks
None identified. Paths are Bun-generated cache names, not untrusted input; no new parsing or privilege boundaries.
Level of scrutiny
High. renameat_concurrently_without_fallback is a shared helper reached from move_file_z (bun build --compile, upgrade, test snapshot commit, Tmpfile::finish), the tarball repair path, and bun patch commit/restore. On Windows and on any Linux filesystem lacking RENAME_EXCHANGE, every one of those callers now takes the new loop instead of the old delete+rename. The first-writer-wins switch also changes an invariant other code may depend on. github-actions flagged two possibly-overlapping open PRs (#33979, #33884) that a human should reconcile.
Other factors
All prior review threads (CodeRabbit, comment-cop, my earlier nits) are resolved in the current diff. The test is hermetic (in-process registry, port: 0, tempDir), skips on musl, and the repair case asserts the shim fired via a marker file. The PR description states rust:check-all and the streaming-extract/patch suites pass; the new test itself is Linux-glibc-only so CI is the proof point.
|
Updated 7:11 PM PT - Jul 28th, 2026
❌ @robobun, your commit 2f400c3 has some failures in 🧪 To try this PR locally: bunx bun-pr 36229That installs a local version of the PR into your bun-36229 --bun |
There was a problem hiding this comment.
No blocking issues found, but this reworks the shared renameat_concurrently_without_fallback fallback (reached from ~10 call sites including move_file_z, Tmpfile::finish, bun build --compile, and upgrade) and changes cache-publish semantics to first-wins — worth a human look at the concurrency reasoning.
Reviewed: the rename-aside loop (retry bound, restore-on-error, aside cleanup on every exit); rename_aside_name NAME_MAX/PATH_MAX guards (my earlier NAME_MAX note is addressed); the keep-existing completeness checks in both extract_tarball.rs and patch_install.rs; and the LD_PRELOAD test (musl skip applied, marker file proves shim injection in the repair test).
Extended reasoning...
Overview
The PR touches four files: src/sys/lib.rs (rewrites the sad-path fallback in renameat_concurrently_without_fallback from delete-then-rename to rename-aside-then-rename-then-delete, adds rename_aside_name), src/install/extract_tarball.rs and src/install/patch_install.rs (both add a first-wins RENAME_NOREPLACE + completeness check ahead of the existing concurrent-rename call), and a new Linux-only LD_PRELOAD-based regression test.
Security risks
None identified. The paths involved are Bun-controlled cache/tempdir names, not user input; the new helper only appends a fixed-shape suffix and both length limits (buffer and NAME_MAX) are guarded. No auth, crypto, or network-trust surface is touched.
Level of scrutiny
High. renameat_concurrently_without_fallback is a shared syscall helper reached from ~10 call sites (move_file_z → StandaloneModuleGraph, upgrade_command, test_command, Tmpfile::finish; patchPackage.rs commit/restore; the extract/patch repair paths). The new fallback introduces a retry loop, a random aside name, and best-effort restore semantics that run on every platform without atomic exchange (NFS/FUSE on Linux, and always on Windows since EXCHANGE is cfg-gated out). Changing the cache publish from last-wins to first-wins is a semantic decision that a maintainer should sign off on, even though the PR's argument (entries only appear via atomic rename of a fully extracted tree, so an existing entry is content-identical) is sound and mirrors commit_global_store_entry.
Other factors
Both nits I raised on the previous revision (NAME_MAX guard on the aside basename; isMusl skip so the shim test doesn't vacuously pass on the static-linked lane) have been addressed, and all inline threads from other reviewers are resolved. The bug-hunting pass on the current head found nothing new. The test is hermetic (in-process registry, port: 0, tempDir), skips correctly on non-Linux/musl/no-cc, and the repair test asserts the shim actually injected EOPNOTSUPP via a marker file. What I did not independently verify: behavior on Windows (where the aside fallback now always runs) and macOS (where renameat2(exclude) maps through renameatx_np); those are covered only by the author's rust:check-all and CI.
|
CI status on the latest run (build 84403): 193 of 196 jobs passed. The only hard failure is the darwin 14 x64 test lane hitting its job timeout; every test failure in the annotations is marked flaky and passed when re-run alone (parallel-batch timeouts and timing assertions in suites this PR does not touch: MySQL 28004, spawn-streaming-stdout, fetch-tls-abortsignal-timeout, bunshell, svelte, and similar). The new cache-race and repair tests pass on the Linux lanes. The diff is ready for review; I am not going to keep re-rolling CI for unrelated flake. |
|
One interaction to be aware of when reviewing this: keeping the existing destination unconditionally is not right for #38702 fixes that leak for tarballs by deciding before extraction whether the folder already existed ( |
1 participant
Fixes #36227
Problem
Two or more concurrent
bun installprocesses sharingBUN_INSTALL_CACHE_DIRon a filesystem withoutrenameat2(RENAME_EXCHANGE)support (NFS, FUSE) could fail with:When a process lost the
RENAME_NOREPLACErace to publish a cache entry,renameat_concurrently_without_fallbacktriedRENAME_EXCHANGE(alwaysEOPNOTSUPPon NFS) and then fell back todelete_tree(dest)+renameat. That recursive delete unlinked the winner's cache entry files while another process was mid-copy from the cache intonode_modules. On local filesystemsRENAME_EXCHANGEsucceeds and, crucially, the displaced tree is left intact in the temp dir, so readers never see files disappear.Moving the destination aside before deleting (as suggested in the issue) is not sufficient: a reader holds an open fd on the cache entry directory for the whole copy, and renaming a directory does not invalidate that fd, so deleting the displaced tree under any name still unlinks files out from under the reader. Verified by the new test, which still failed with an aside-then-delete approach.
Fix
Cache entries only ever appear via an atomic rename of a fully extracted tree, so an existing entry at the destination is complete and content-identical to ours. On a rename collision, keep the existing entry and discard the freshly extracted temp copy (which no other process reads). This mirrors what
commit_global_store_entryalready does for isolated installs. Applied to both cache-population sites:extract_tarball.rs: first-wins, except when the existing entry is incomplete (missingpackage.jsonfor npm packages, matchingpackage_missing_from_cache). An incomplete entry is still replaced because that is the only repair path for a corrupt entry, preserving the re-extraction repair flow.patch_install.rs: first-wins; the entry name embeds the patch hash, so a same-named entry is equivalent.The generic
renameat_concurrentlyfallback (still used by the repair path,move_file_z, and patch commit/restore) no longer deletes the live destination in place: it renames the destination aside, renames the source into place, then deletes the old tree under its aside name, restoring the destination if the rename fails. This removes the wide partially-deleted-directory window for any remaining replace-semantics caller.Test
test/cli/install/bun-install-cache-race.test.ts(Linux): anLD_PRELOADshim makesrenameat2(RENAME_EXCHANGE)fail withEOPNOTSUPPthe way NFS does, then runs 4 concurrentbun install --backend=copyfileprocesses sharing a fresh cache, installing a 600-file package from a local in-process registry, for 3 iterations.ENOENT: failed copying files from cache to destination(the exact failure from the issue).cargo checkpasses on all 10 targets (bun run rust:check-all);bun-install-streaming-extract,bun-install-patch, andbun-patchsuites pass with the debug build.no test proof · iteration 1 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/cli/install/bun-install-cache-race.test.ts