install: never delete a concurrently published cache entry on Windows

src/install/extract_tarball.rs

@@ -573,59 +573,99 @@ impl ExtractTarball {
                         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
-                                        let mut tempdest_buf = PathBuffer::uninit();
-                                        tempdest_buf[0..tmpname.len()]
-                                            .copy_from_slice(tmpname.as_bytes());
-                                        tempdest_buf[tmpname.len()..][0..4]
-                                            .copy_from_slice(&[b't', b'm', b'p', 0]);
-                                        let tempdest =
-                                            ZStr::from_buf(&tempdest_buf, tmpname.len() + 3);
-                                        let mut folder_name_z_buf = PathBuffer::uninit();
-                                        folder_name_z_buf[0..folder_name.len()]
-                                            .copy_from_slice(folder_name);
-                                        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(
-                                            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());
+                                    | sys::Errno::EXIST
+                            ) {
+                                // The cache path is keyed by package identity, so a valid
+                                // destination is an equivalent entry a concurrent `bun install`
+                                // published first: accept it and drop our copy instead of
+                                // deleting theirs, which would leave a window where neither
+                                // exists. "Valid" here mirrors `package_missing_from_cache()`:
+                                // Npm requires `package.json`; other tags only require the
+                                // directory. An Npm destination without `package.json` is a
+                                // stale entry nothing reads from; move it aside by handle (not
+                                // by path, so a concurrently-published fresh entry is never
+                                // touched) and retry, like the POSIX arm's RENAME_EXCHANGE.
+                                match sys::open_dir_at_windows_a(
+                                    Fd::from_std_dir(cache_dir),
+                                    folder_name,
+                                    sys::WindowsOpenDirOptions {
+                                        can_rename_or_delete: true,
+                                        iterable: false,
+                                        ..Default::default()
+                                    },
+                                ) {
+                                    Ok(dest) => {
+                                        let valid = self.resolution.tag != ResolutionTag::Npm
+                                            || sys::exists_at(
+                                                dest,
+                                                ZStr::from_static(b"package.json\0"),
+                                            );
+                                        if valid {
+                                            let _ = sys::close(dest);
+                                            let _ = tmpdir.delete_tree(tmpname.as_bytes());
+                                            break;
+                                        }
+                                        let evicted = if retries < MAX_RETRIES {
+                                            let mut tempdest_buf = PathBuffer::uninit();
+                                            tempdest_buf[0..tmpname.len()]
+                                                .copy_from_slice(tmpname.as_bytes());
+                                            tempdest_buf[tmpname.len()..][0..3]
+                                                .copy_from_slice(b"tmp");
+                                            let tempdest =
+                                                &tempdest_buf[..tmpname.len() + 3];
+                                            let mut tempdest_w_buf = WPathBuffer::uninit();
+                                            let tempdest_w = strings::to_wpath_normalized(
+                                                &mut tempdest_w_buf,
+                                                tempdest,
+                                            );
+                                            let r = bun_sys::windows::move_opened_file_at(
+                                                dest,
+                                                Fd::from_std_dir(tmpdir),
+                                                tempdest_w,
+                                                true,
+                                            );
+                                            let _ = sys::close(dest);
+                                            if r.is_ok() {
+                                                let _ = tmpdir.delete_tree(tempdest);
+                                                true
+                                            } else {
+                                                false
+                                            }
+                                        } else {
+                                            let _ = sys::close(dest);
+                                            false
+                                        };
+                                        if retries < MAX_RETRIES {
+                                            retries += 1;
+                                            if !evicted {
+                                                std::thread::sleep(
+                                                    std::time::Duration::from_millis(
+                                                        10u64 << (retries - 1),
+                                                    ),
+                                                );
                                             }
+                                            continue;
                                         }
+                                    }
+                                    Err(_) if retries < MAX_RETRIES => {
                                         retries += 1;
-                                        // 10ms, 20ms, 40ms, 80ms — long enough
-                                        // for a concurrent close to land,
-                                        // short enough to not slow a legit
-                                        // failure noticeably.
+                                        // 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;
                                     }
-                                    _ => {}
+                                    Err(_) => {}
                                 }
                             }
-                            let _ = sys::close(dir_to_move);
                             log.add_error_fmt(
                                 None,
                                 bun_ast::Loc::EMPTY,

test/regression/issue/28062.test.ts

@@ -0,0 +1,95 @@
+// 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 { mkdir, rm, writeFile } from "fs/promises";
+import { bunEnv, bunExe, isWindows, tempDir, VerdaccioRegistry } from "harness";
+import { join } from "path";
+
+let verdaccio: VerdaccioRegistry | undefined;
+
+beforeAll(async () => {
+  if (!isWindows) return;
+  verdaccio = new VerdaccioRegistry();
+  await verdaccio.start();
+}, 60_000);
+
+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,
+);