install: never delete a concurrently published cache entry on Windows - #33884
install: never delete a concurrently published cache entry on Windows#33884Jarred-Sumner wants to merge 5 commits into
Conversation
|
Updated 1:00 AM PT - Aug 2nd, 2026
❌ @robobun, your commit 45ed0f6 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 33884That installs a local version of the PR into your bun-33884 --bun |
|
Found 3 issues this PR may fix:
🤖 Generated with Claude Code |
WalkthroughChangesWindows cache publish handling
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn't find any bugs and the logic looks sound, but this changes Windows-only cache-publish semantics that were only verified via cargo check (no runtime execution), and the PR explicitly flags a behavioral trade-off around corrupt cache entries — worth a maintainer sign-off, ideally with the requested Windows soak test.
Extended reasoning...
Overview
This PR modifies the Windows-only error-recovery arm in src/install/extract_tarball.rs::move_to_cache_directory. Previously, when renaming a freshly-extracted temp dir into the cache failed with NOTEMPTY/PERM/BUSY/EXIST, the code would rename the destination cache entry out to a temp path and delete_tree it before retrying — which could destroy a concurrent bun install's just-published entry. The new code closes dir_to_move, probes whether the destination now opens, and if so accepts the winner's entry (deleting only its own temp copy and breaking to the shared re-open path). The bounded backoff retry is retained only for the case where the destination is not yet openable. Net: ~50 lines of destructive recovery replaced with ~20 lines of accept-the-winner logic.
Security risks
None identified. This is cache-publish contention handling; no new inputs are parsed, no path construction is added, and the only new filesystem operation is cache_dir.open_at(folder_name) on a name already computed and used by the surrounding code. The returned Dir from the .is_ok() probe is closed via Drop, so no fd leak.
Level of scrutiny
Medium-high. The diff is small and the reasoning is well-argued, but:
- The entire hunk is behind
#[cfg(windows)]and the author states it was only type-checked (cargo check --target x86_64-pc-windows-msvc), never executed. The PR description explicitly requests a maintainer with a Windows machine to soak-test concurrent installs. - This is the package-manager cache-publish path — it runs on every uncached
bun installon Windows. - The PR explicitly calls out a behavioral trade-off: the deleted destructive path incidentally self-healed pre-existing corrupt/incomplete cache directories, and this change will now accept them instead. The author offers a follow-up; a maintainer should decide whether that's acceptable as-is.
Other factors
- No automated test is included (deliberately, with a stated rationale that the racy path is unreachable off-Windows). Per repo guidelines this normally blocks, so a human should confirm the exception is acceptable here.
- The
sys::close(dir_to_move)was hoisted to the top of theErrarm, which is a correct simplification (previously duplicated across the retry and fall-through paths). - No CODEOWNERS entry covers
src/install/.
Given the untested-on-target-platform status and the flagged design trade-off, this warrants human review rather than auto-approval.
…#36613) Fixes the Windows CI flake in `test/regression/issue/36577.test.ts` introduced by #36578, and brings the test under the default timeout on debug builds. ### Repro With the CI runner's env on Windows: ```powershell $env:BUN_INSTALL_CACHE_DIR=$tmp; $env:BUN_TMPDIR=$tmp; $env:TEMP=$tmp bun test test/regression/issue/36577.test.ts ``` fails 12/15 runs with either `expect(r.code).toBe(0)` receiving 1, or stderr containing ``` error: failed to verify cache dir for "f012": ENOENT ``` ### Cause The two `test.concurrent` cases install overlapping package names (`f000`-`f023`, `lib`, `carrier`, `pdep`, `zz-late`). The test set `cache = "cache"` in bunfig to give each case an isolated cache, but `scripts/runner.node.mjs` sets `BUN_INSTALL_CACHE_DIR` on the test process, `bunEnv` inherits it, and `fetch_cache_directory_path` consults that env var before the bunfig setting, so both concurrent `bun install` processes shared one cache directory. On Windows, the tarball-extraction retry path handles an occupied cache slot by renaming the existing entry into the temp dir and deleting it before retrying, so one process's post-rename verify can see `ENOENT` on an entry the other process just moved aside. That underlying race is #28062 (fix in progress in #33884). The POSIX path uses `renameat_concurrently_a`, which does not remove the existing entry, so the flake is Windows-only. Separately, under debug+ASAN each case took ~5s (two subprocess installs on 80-104 packages plus ~100 `Bun.Archive.write` calls for tarball synthesis in the registry server), right at the 5s default timeout. ### Fix - Override `BUN_INSTALL_CACHE_DIR` per test case in the spawn env so the intended isolation holds regardless of the ambient environment; drop the now-dead bunfig `cache` key and `mkdirSync`. - Run both installs with `--lockfile-only`. The `Lockfile::eql` comparison under test only needs a lockfile, so tarball download, extraction and linking are irrelevant. With no tarball fetch the registry server no longer needs to synthesize real tarballs, so the `Bun.Archive.write` path and the tarball route are removed (integrity is never verified on this path). ### Verification - Windows with CI-like env: 0/30 failures (was 12/15). - `bun bd test`: 5/5 pass, 0.8-1.8s per case (was timing out at 5s). - With the `src/install/lockfile.rs` change from #36578 reverted, both cases still fail with `lockfile had changes, but lockfile is frozen`, so the test continues to cover the original regression. <!-- robobun:evidence:begin --> --- **[stamp-90s]** gate passed · iteration 1 · 1 files touched <details><summary>passes on PR (with fix)</summary> ```console Test-only change. Debug/ASAN (expected pass): $ bun bd test 'test/regression/issue/36577.test.ts' $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "test/regression/issue/36577.test.ts" bun test v1.4.0 (6aa1437) test/regression/issue/36577.test.ts: (pass) frozen lockfile accepts a freshly generated lockfile (32 fillers) [2973.65ms] (pass) frozen lockfile accepts a freshly generated lockfile (24 fillers) [3158.88ms] 2 pass 0 fail 8 expect() calls Ran 2 tests across 1 file. [5.50s] Exit: 0 ``` </details> <details><summary>diff hotspot</summary> ``` test/regression/issue/36577.test.ts | 65 +++++++++---------------------------- 1 file changed, 16 insertions(+), 49 deletions(-) ``` </details> **gate history** · 1 passed · 1 rejected · iteration 1 <details><summary>evidence per changed file</summary> ``` file reads edits tests test/regression/issue/36577.test.ts 6 9 0 ``` </details> **self-review** · no surviving concerns 33 concerns were raised and did not survive verification. <!-- robobun:evidence:end -->
|
@robobun rebase this PR |
When two bun install processes share the package cache on Windows and both extract the same package, the loser's rename into the final cache path fails because the winner already published it. The loser then renamed the winner's entry out of the cache and deleted it, leaving a window with no cache entry, so the winner's own install failed with ENOENT. Cache entries are keyed by package identity, so an existing destination is an equivalent entry: accept it, delete only our own temp dir, and succeed. Keep the bounded backoff retry only for a destination that is not yet openable. This mirrors the POSIX arm, which already has these semantics via an atomic exchange. Fixes #28062
3e9114a to
a6b5e63
Compare
|
Rebased onto main (a6b5e63). No conflicts. |
There was a problem hiding this comment.
I didn't find any bugs, but this changes Windows-only cache-publish error recovery with a stated behavioral trade-off (corrupt cache entries are no longer evicted) and ships without a runtime test by design — the author explicitly asks for a Windows soak-test and a maintainer call on the trade-off. Worth a human look, and worth confirming the post-rebase Windows CI is green (the pre-rebase run had bun-install-registry.test.ts failing on 🪟 x64).
What was reviewed
- The new accept-winner path:
cache_dir.open_at(folder_name).is_ok()returns an ownedDirwhoseDropcloses the fd, so the probe does not leak a handle. - Control flow after
break: the loser falls through to the existingcache_dir.open_at(folder_name)re-open, so it proceeds with the winner's directory as described. - Retry path: the temp extraction dir is only deleted in the accept branch, so re-opening
dir_to_moveoncontinuestill finds it;dir_to_moveis closed on every error path before retry/fail.
Extended reasoning...
Overview
This PR rewrites the error-recovery arm of the Windows-only (#[cfg(windows)]) cache-publish loop in src/install/extract_tarball.rs::move_to_cache_directory. Previously, on EXIST/NOTEMPTY/PERM/BUSY from move_opened_file_at, the code renamed the destination cache entry into the temp dir and delete_tree'd it before retrying — destroying an entry a concurrent bun install had just published. The new code instead probes the destination with open_at; if it opens, the loser deletes only its own temp extraction and accepts the winner's entry. If the destination is not yet openable, the bounded exponential backoff (10/20/40/80 ms) is retained before failing. sys::close(dir_to_move) is hoisted to run once at the top of the error arm on every path.
Security risks
None identified. This is filesystem error-recovery inside the package cache; no untrusted input parsing, no path construction changes, and the destructive operation that remains (tmpdir.delete_tree(tmpname)) targets only the process's own randomly-named temp extraction directory.
Level of scrutiny
Medium-high. The diff is small (~30 net lines removed) and the logic is straightforward, but it is production error-handling in the package manager on a platform the author could not execute on (verified only via cargo check --target *-pc-windows-msvc). The PR description explicitly states a behavioral trade-off — a pre-existing corrupt/incomplete cache directory that the old destructive path incidentally evicted will now be accepted — and defers that call to a maintainer. REVIEW.md's "every behavioral change ships an automated test" is deliberately not met here, with a reasoned argument; that's a maintainer judgment, not a bot approval.
Other factors
- I checked for an fd leak from the
.is_ok()probe:Dirhas aDropimpl (src/sys/dir.rs:14) that closes the fd, so the temporary is cleaned up. - The
breakafter accepting the winner correctly falls through to the existing post-loopcache_dir.open_at(folder_name)→get_fd_path_zsequence, soExtractDatais populated from the winner's directory. - On the retry
continue, the temp dir still exists (only deleted in the accept branch), so re-openingdir_to_moveat the top of the loop is sound. - The pre-rebase CI run (#71287) showed
test/cli/install/bun-install-registry.test.tsfailing on Windows x64 and x64-baseline. I can't tell from here whether that's related or pre-existing flake, but it's directly in the changed subsystem and should be confirmed green on the fresh run before merge.
…egression test The accept-existing path must only accept a destination that actually has a package.json. package_missing_from_cache() triggers re-extraction precisely because the cache dir exists without one, and NtSetInformationFile cannot replace a non-empty directory, so the fresh extraction was being discarded and the stale entry kept. That broke the existing 'it should invalid cached package if package.json is missing' test on Windows. Now on collision: if the destination has package.json, accept it (a concurrent install published it); otherwise move the stale entry aside into the temp dir and retry. That matches what the POSIX arm gets from RENAME_EXCHANGE. The new test/regression/issue/28062.test.ts runs four parallel installs over a shared cache for eight rounds on Windows. Without the fix it fails on the first round with ENOENT opening cache/package/version; with the fix all 32 installs succeed.
|
CI on the rebase turned up a real regression in the original diff: 3b5cf84 gates the accept path on Also added |
…issing_from_cache() validity Open the destination with DELETE access and evict via move_opened_file_at on that handle instead of a path-based renameat, so the directory whose package.json was checked is the one moved aside; a concurrently published fresh entry at the same path is never touched. Only gate on package.json for ResolutionTag::Npm. package_missing_from_cache() uses directory_exists_at for Github/LocalTarball/RemoteTarball, and Github entries are allowed to have no package.json, so accept any openable destination for those tags. Keep the backoff sleep before retry when the eviction rename fails (sharing violation from AV/indexer); only skip it when the stale entry was actually moved aside.
There was a problem hiding this comment.
Verified 8353d5b addresses the three concerns from the prior pass: eviction now goes through move_opened_file_at on the opened dest handle (no path re-resolution TOCTOU), non-Npm tags are accepted on directory existence alone so a package.json-less GitHub entry is never evicted, and the backoff sleep is restored on the eviction-failed retry path. Only new finding is the test-file nit below.
Extended reasoning...
The three inline concerns I raised on the previous revision are all resolved in the current diff — I re-read the Windows publish loop and confirmed each fix is present as described, not just claimed. Deferring rather than approving because this is still a subtle Windows-only concurrency change in the installer's cache publish path that has already needed two rounds of correction (one CI-caught regression, then three review-caught edge cases); a human sign-off on the final shape is warranted. The remaining finding is a CI-perf nit only.
File-scope beforeAll runs even when every test is skipped, so non-Windows lanes were forking verdaccio for a test that never runs. Guard the hook body on isWindows and give it a 60s ceiling for cold debug VMs.
There was a problem hiding this comment.
All four prior review threads are addressed in the current diff and no new issues turned up this pass, but this is a subtle Windows-only concurrency fix in the installer's cache-publish path — worth a human look before merging.
What was reviewed:
- Eviction now binds to the opened handle via
move_opened_file_at(dest, …)— verified againstsrc/sys/windows/mod.rs:1860that this isNtSetInformationFileon the fd, not a path re-resolve. - Validity check mirrors
package_missing_from_cache(): non-Npm tags accept on directory-open alone; Npm requirespackage.json. - Backoff sleep restored on the eviction-failed path;
destis closed on every arm (valid/evicted/max-retries/else). - Test hooks now skip the verdaccio fork on non-Windows lanes.
Extended reasoning...
Overview
The PR reworks the Windows #[cfg(windows)] retry loop in ExtractTarball::move_to_cache_directory (src/install/extract_tarball.rs). Pre-PR, a rename collision on the cache destination unconditionally renamed the existing cache entry out to a temp path and delete_tree'd it before retrying — which, when the existing entry belongs to a concurrent bun install, deletes a live cache entry the other process is about to read. The new logic opens the destination with can_rename_or_delete: true, accepts it if it satisfies the same validity predicate package_missing_from_cache() uses, and only evicts (by handle, not path) an Npm entry with no package.json. A new Windows-only regression test spins four concurrent installs against a shared cache for eight rounds.
This is the fourth revision I've looked at. The three earlier rounds each found real issues (path-based check-then-act eviction, wrong validity predicate for Github/tarball tags, lost backoff on eviction failure, unnecessary verdaccio fork on non-Windows), all of which are now fixed and verified in the current diff. The bug-hunting system found nothing new this round.
Security risks
None identified. The change is confined to how a rename collision is handled inside Bun's own package cache; no new inputs are parsed and no trust boundaries are crossed. The only destructive operations (delete_tree) target the process's own temp extraction (tmpname) or a handle-verified stale entry moved into tmpdir.
Level of scrutiny
High. This is production-critical installer code with a failure mode of "silently deletes another process's cache entry", the fix reasons about Windows handle semantics (DELETE access, sharing violations, NtSetInformationFile on an open handle vs. path-based rename), and it's exercised only on Windows CI lanes. The retry/backoff state machine has five distinct exit paths (accept-valid, evict-succeeded, evict-failed-with-backoff, open-failed-with-backoff, max-retries → error) and getting any one wrong either leaks a handle, reintroduces the race, or spins without waiting. I traced each path for handle balance and termination and they look correct — but three prior rounds each found something, so a maintainer familiar with the Windows install path should confirm.
Other factors
- The regression test exercises the concurrent-winner path (the headline bug) but not the stale-entry eviction path under concurrency; that path is covered single-process by the existing
bun-install-registry.test.ts"invalid cached package" test per the PR body. - CI build #87621 for the latest commit is still in progress at the time of this review; Windows lane results should be checked before merge.
- The 120s test timeout and 8-round × 4-process shape is reasonable for a race repro but adds ~4s to the Windows lane; acceptable for a regression test guarding cache corruption.
|
CI on 45ed0f6 (build 87621): no new failures. |
|
Another sighting of the same race, with the other face of it (the loser's error rather than the winner's ENOENT). Buildkite build 94864 on #36300, Linux and macOS lanes passed. With the current code each loser renames the entry that just landed out of the cache and deletes it before retrying, so with more than two installers they keep evicting each other until one of them runs out of the 4 retries and reports the ENOTEMPTY above. The accept path in this PR covers it: every loser that hits NOTEMPTY finds |
2 participants
The race
Two
bun installprocesses sharing the package cache on Windows can corrupt each other: when both extract the same package, the loser's rename into the final cache path fails because the winner already published it, and the loser's error handling then renames the winner's already-published cache entry out of the cache anddelete_trees it before retrying its own rename. During that 10-150 ms window the cache entry does not exist, so the winner, which already moved on to the install phase, fails withENOENT: failed opening cache/package/version dir for package <name>.Full analysis with line references: #28062 (comment)
The fix
Cache entries are keyed by package identity, so the cache is effectively write-once and an existing destination is an equivalent entry that a concurrent
bun installwon the race to publish. OnEXIST/NOTEMPTY/PERM/BUSYthe loser now opens the destination withDELETEaccess and:package_missing_from_cache()'s own check (Npm: haspackage.json; Github/LocalTarball/RemoteTarball: directory exists). Delete only our own temp extraction and proceed with the winner's directory.package.json, i.e. a stale entry nothing reads from as valid.move_opened_file_aton the opened handle renames the directory whosepackage.jsonwas checked, never a concurrently-published fresh entry that landed at the same path in the gap; the POSIX arm already gets this fromRENAME_EXCHANGE.This keeps the existing "re-extract over a corrupt cache entry" behaviour that
bun-install-registry.test.tsexercises while never deleting a concurrently published entry.Verification
test/regression/issue/28062.test.tsruns four parallel installs against a sharedBUN_INSTALL_CACHE_DIRfor eight rounds on Windows.Also on Windows with this build:
bun-install-registry.test.ts -t "invalid cached package if package.json is missing"passes (it failed on a6b5e63 when the destination was accepted unconditionally).Fixes #28062
no test proof · iteration 3 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/regression/issue/28062.test.ts