diff --git a/src/install/extract_tarball.rs b/src/install/extract_tarball.rs index b8d3a8b091b5..0fd75aeae14f 100644 --- a/src/install/extract_tarball.rs +++ b/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, diff --git a/test/regression/issue/28062.test.ts b/test/regression/issue/28062.test.ts new file mode 100644 index 000000000000..8a3729e6f8f6 --- /dev/null +++ b/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, +);