install: never delete a concurrently published cache entry on Windows

src/install/extract_tarball.rs

@@ -573,21 +573,36 @@
                         true,
                     ) {
                         bun_sys::Result::Err(err) => {
-                            if retries < MAX_RETRIES {
-                                match err.get_errno() {
-                                    sys::Errno::NOTEMPTY
+                            let _ = sys::close(dir_to_move);
+                            if matches!(
+                                err.get_errno(),
+                                sys::Errno::NOTEMPTY
                                     | sys::Errno::PERM
                                     | sys::Errno::BUSY
-                                    | sys::Errno::EXIST => {
-                                        // before we attempt to delete the destination, let's close the source dir.
-                                        let _ = sys::close(dir_to_move);
-
-                                        // We tried to move the folder over
-                                        // but it didn't work!
-                                        // so instead of just simply deleting the folder
-                                        // we rename it back into the temp dir
-                                        // and then delete that temp dir
-                                        // The goal is to make it more difficult for an application to reach this folder
+                                    | sys::Errno::EXIST
+                            ) {
+                                // The cache path is keyed by the package identity, so an
+                                // existing destination with a package.json is an equivalent
+                                // entry that a concurrent `bun install` published first. Accept
+                                // it and drop our copy instead of deleting theirs, which would
+                                // leave a window where neither exists.
+                                //
+                                // A destination without a package.json is a stale entry that
+                                // `package_missing_from_cache()` already rejected. Nothing is
+                                // reading from it as a valid cache entry, so move it aside and
+                                // retry. This mirrors the POSIX arm's RENAME_EXCHANGE, which
+                                // atomically puts the fresh extraction at the cache path.
+                                if let Ok(dest) = cache_dir.open_at(folder_name) {
+                                    if sys::exists_at(
+                                        dest.fd(),
+                                        ZStr::from_static(b"package.json\0"),
+                                    ) {
+                                        drop(dest);
+                                        let _ = tmpdir.delete_tree(tmpname.as_bytes());
+                                        break;
+                                    }
+                                    drop(dest);
+                                    if retries < MAX_RETRIES {
                                         let mut tempdest_buf = PathBuffer::uninit();
                                         tempdest_buf[0..tmpname.len()]
                                             .copy_from_slice(tmpname.as_bytes());
@@ -601,31 +616,30 @@
                                         folder_name_z_buf[folder_name.len()] = 0;
                                         let folder_name_z =
                                             ZStr::from_buf(&folder_name_z_buf, folder_name.len());
-                                        match sys::renameat(
+                                        if sys::renameat(
                                             Fd::from_std_dir(cache_dir),
                                             folder_name_z,
                                             Fd::from_std_dir(tmpdir),
                                             tempdest,
-                                        ) {
-                                            bun_sys::Result::Err(_) => {}
-                                            bun_sys::Result::Ok(_) => {
-                                                let _ = tmpdir.delete_tree(tempdest.as_bytes());
-                                            }
+                                        )
+                                        .is_ok()
+                                        {
+                                            let _ = tmpdir.delete_tree(tempdest.as_bytes());
                                         }
                                         retries += 1;
-                                        // 10ms, 20ms, 40ms, 80ms — long enough
-                                        // for a concurrent close to land,
-                                        // short enough to not slow a legit
-                                        // failure noticeably.
-                                        std::thread::sleep(std::time::Duration::from_millis(
-                                            10u64 << (retries - 1),
-                                        ));
                                         continue;
                                     }
-                                    _ => {}
+                                } else if retries < MAX_RETRIES {
+                                    retries += 1;
+                                    // 10ms, 20ms, 40ms, 80ms — long enough for a
+                                    // concurrent close to land, short enough to not
+                                    // slow a legitimate failure noticeably.
+                                    std::thread::sleep(std::time::Duration::from_millis(
+                                        10u64 << (retries - 1),
+                                    ));
+                                    continue;
                                 }
                             }
-                            let _ = sys::close(dir_to_move);
                             log.add_error_fmt(
                                 None,
                                 bun_ast::Loc::EMPTY,

test/regression/issue/28062.test.ts

@@ -0,0 +1,94 @@
+// https://github.com/oven-sh/bun/issues/28062
+// Windows: two `bun install` processes sharing BUN_INSTALL_CACHE_DIR race to
+// publish the same cache entry. The loser must accept the winner's entry and
+// never delete it; otherwise the winner's install fails with ENOENT opening
+// the cache dir it just published.
+import { afterAll, beforeAll, expect, test } from "bun:test";
+import { rm, mkdir, writeFile } from "fs/promises";
+import { bunEnv, bunExe, isWindows, tempDir, VerdaccioRegistry } from "harness";
+import { join } from "path";
+
+let verdaccio: VerdaccioRegistry;
+
+beforeAll(async () => {
+  verdaccio = new VerdaccioRegistry();
+  await verdaccio.start();
+});
+
+afterAll(() => {
+  verdaccio.stop();
+});
+
+// The destructive rename-out-and-delete lived in the #[cfg(windows)] publish
+// path; POSIX uses an atomic RENAME_EXCHANGE and never had the ENOENT window.
+test.skipIf(!isWindows)(
+  "concurrent installs sharing a cache dir do not delete each other's cache entries",
+  async () => {
+    const dependencies = {
+      "no-deps": "1.0.0",
+      "a-dep": "1.0.1",
+      "basic-1": "1.0.0",
+      "what-bin": "1.0.0",
+      "one-dep": "1.0.0",
+      "two-range-deps": "1.0.0",
+      "dep-with-tags": "1.0.0",
+      "dep-loop-entry": "1.0.0",
+    };
+    const pkg = JSON.stringify({ name: "cache-race", private: true, dependencies });
+
+    using root = tempDir("bun-install-cache-race", {});
+    const cache = join(String(root), "shared-cache");
+    const bunfig = `[install]\nregistry = "${verdaccio.registryUrl()}"\n`;
+
+    const projects: string[] = [];
+    for (let i = 0; i < 4; i++) {
+      const dir = join(String(root), `p${i}`);
+      await mkdir(dir, { recursive: true });
+      await writeFile(join(dir, "package.json"), pkg);
+      await writeFile(join(dir, "bunfig.toml"), bunfig);
+      projects.push(dir);
+    }
+
+    const env = {
+      ...bunEnv,
+      BUN_INSTALL_CACHE_DIR: cache,
+    };
+
+    const install = async (cwd: string) => {
+      await using proc = Bun.spawn({
+        cmd: [bunExe(), "install", "--ignore-scripts"],
+        cwd,
+        env,
+        stdout: "pipe",
+        stderr: "pipe",
+        stdin: "ignore",
+      });
+      const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
+      return { cwd, stdout, stderr, exitCode };
+    };
+
+    // Before the fix, the loser's rename-out + delete of the winner's cache
+    // entry created a guaranteed ENOENT window (>=10ms of backoff) every time
+    // two processes collided on the same package, so a handful of rounds with
+    // a fresh cache is enough to hit it reliably.
+    for (let round = 0; round < 8; round++) {
+      await rm(cache, { recursive: true, force: true });
+      for (const dir of projects) {
+        await rm(join(dir, "node_modules"), { recursive: true, force: true });
+        await rm(join(dir, "bun.lock"), { force: true });
+      }
+
+      const results = await Promise.all(projects.map(install));
+      const failed = results.filter(r => r.exitCode !== 0);
+      if (failed.length) {
+        const detail = failed.map(r => `cwd=${r.cwd}\nstderr:\n${r.stderr}\nstdout:\n${r.stdout}`).join("\n---\n");
+        expect(detail).toBe("");
+      }
+      for (const r of results) {
+        expect(r.stderr).not.toContain("ENOENT");
+        expect(r.exitCode).toBe(0);
+      }
+    }
+  },
+  120_000,
+);