From 749384ca75ec946a3d1846f968eabd71c19ccb70 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Thu, 3 Sep 2026 02:14:38 +0000 Subject: [PATCH 1/4] install: replace an existing file in the copy backend, do not truncate it The copy backend opened each destination file with O_TRUNC. If the destination was a hardlink of the cache file being copied, the open emptied the cache file, and the copy then wrote 0 bytes. A self-contained workspace reaches this in a normal install. Its tree installs a sibling workspace's nested dependency through the workspace symlink, with the copy backend. The sibling's own tree can hardlink the same package into the same directory first. A hardlink install, then rm -rf node_modules at the root, then a copyfile install also reaches it, without the feature. The copy now opens the destination with O_EXCL. If the file exists, it unlinks the file and opens it again, as the hardlink backend does. On Windows, CopyFileW runs with bFailIfExists. If the file exists, it is deleted, and the copy runs again. --- src/install/PackageInstall.rs | 67 +++++++---- .../bun-workspaces-self-contained.test.ts | 104 +++++++++++++++++- test/cli/install/bun-workspaces.test.ts | 66 ++++++++++- 3 files changed, 212 insertions(+), 25 deletions(-) diff --git a/src/install/PackageInstall.rs b/src/install/PackageInstall.rs index 22a227516ac6..ec7115ff2b49 100644 --- a/src/install/PackageInstall.rs +++ b/src/install/PackageInstall.rs @@ -1356,21 +1356,36 @@ impl<'a> PackageInstall<'a> { } } EntryKind::File => { + // `bFailIfExists`: an existing destination is deleted and + // copied again, never overwritten in place. It can be a + // hardlink of the cache entry being copied (an earlier + // install of the same package into this folder), and + // overwriting it would empty the file in the cache. // SAFETY: FFI — src/dest are valid NUL-terminated WStr buffers. - if unsafe { windows::CopyFileW(src.as_ptr(), dest.as_ptr(), 0) } == 0 { - if let Some(entry_dirname) = - bun_paths::Dirname::dirname_u16(entry.path.as_slice()) - { - let _ = bun_sys::MakePath::make_path_u16( - destination_dir_, - entry_dirname, - ); - // SAFETY: FFI — src/dest are valid NUL-terminated WStr buffers. - if unsafe { windows::CopyFileW(src.as_ptr(), dest.as_ptr(), 0) } - != 0 - { - continue; + let copy_file = + || unsafe { windows::CopyFileW(src.as_ptr(), dest.as_ptr(), 1) } != 0; + if !copy_file() { + let copied = match windows::Win32Error::get() { + windows::Win32Error::FILE_EXISTS + | windows::Win32Error::ALREADY_EXISTS => { + // SAFETY: FFI — dest is a valid NUL-terminated WStr buffer. + unsafe { windows::DeleteFileW(dest.as_ptr()) }; + copy_file() } + _ => match bun_paths::Dirname::dirname_u16(entry.path.as_slice()) + { + Some(entry_dirname) => { + let _ = bun_sys::MakePath::make_path_u16( + destination_dir_, + entry_dirname, + ); + copy_file() + } + None => false, + }, + }; + if copied { + continue; } let err = windows::last_system_errno(); @@ -1409,14 +1424,26 @@ impl<'a> PackageInstall<'a> { destination_dir_.fd(), bstr::BStr::new(entry.path.as_bytes()) ); - // Open O_WRONLY|O_CREAT|O_TRUNC, mode 0o666. + // An existing destination is replaced, never truncated in + // place: it can be a hardlink of the cache entry being copied + // (an earlier install of the same package into this folder), + // and `O_TRUNC` through it would empty the file in the cache. let create = |path: &ZStr| { - sys::openat( - destination_dir_.fd(), - path, - sys::O::WRONLY | sys::O::CREAT | sys::O::TRUNC, - 0o666, - ) + let open = || { + sys::openat( + destination_dir_.fd(), + path, + sys::O::WRONLY | sys::O::CREAT | sys::O::EXCL, + 0o666, + ) + }; + match open() { + Err(err) if err.get_errno() == sys::E::EEXIST => { + let _ = sys::unlinkat(destination_dir_, path); + open() + } + result => result, + } }; let outfile = match create(entry.path) { Ok(f) => f, diff --git a/test/cli/install/bun-workspaces-self-contained.test.ts b/test/cli/install/bun-workspaces-self-contained.test.ts index b1284b245cfe..4e97d37ccf3a 100644 --- a/test/cli/install/bun-workspaces-self-contained.test.ts +++ b/test/cli/install/bun-workspaces-self-contained.test.ts @@ -1,9 +1,9 @@ import { spawn } from "bun"; import { afterAll, afterEach, beforeAll, beforeEach, describe, expect, it, setDefaultTimeout } from "bun:test"; -import { existsSync, lstatSync, readlinkSync, statSync } from "fs"; +import { existsSync, lstatSync, readFileSync, readlinkSync, statSync } from "fs"; import { mkdir, rm, writeFile } from "fs/promises"; import { bunExe, bunEnv as env, isWindows, readdirSorted } from "harness"; -import { join } from "path"; +import { dirname, join } from "path"; import { dummyAfterAll, dummyAfterEach, @@ -37,13 +37,13 @@ beforeEach(async () => { }); afterEach(dummyAfterEach); -async function install(cwd: string, args: string[] = []) { +async function install(cwd: string, args: string[] = [], extraEnv: Record = {}) { // --backend=hardlink so the "physical copy" assertions are meaningful on macOS too // (its default, clonefile, also yields nlink 1) await using proc = spawn({ cmd: [bunExe(), "install", "--backend=hardlink", ...args], cwd, - env, + env: { ...env, ...extraEnv }, stdout: "pipe", stderr: "pipe", }); @@ -176,3 +176,99 @@ it("without either setting the workspace is hoisted normally", async () => { expect(existsSync(join(package_dir, "node_modules", "@barn", "moo", "package.json"))).toBeTrue(); expect(existsSync(join(package_dir, "node_modules", "baz", "package.json"))).toBeTrue(); }); + +// The package.json of `name` that `from` resolves without leaving `root`. It walks up +// from `from` and does not resolve symlinks, the way a tool that copies `root` sees it. +function findWithin(root: string, from: string, name: string): string | undefined { + for (let dir = from; ; dir = dirname(dir)) { + const candidate = join(dir, "node_modules", name, "package.json"); + if (existsSync(candidate)) return candidate; + if (dir === root || dirname(dir) === dir) return undefined; + } +} + +// `c` is self-contained and depends on its siblings `a` and `b`, which need different +// versions of `baz`. The siblings stay symlinks, so the version that cannot hoist into +// c's node_modules goes into the sibling's own node_modules, through the symlink. The +// sibling's own tree can write the same directory. The second write used to truncate +// the hardlinks of the first, and with them the files in the cache. +it.each([ + // baz@0.0.3 hoists to the root, so b's own tree also nests baz@0.0.5 in packages/b + ["one nested in the sibling's own node_modules", {}], + // b uses the root's baz@0.0.5, so only c's tree writes it into packages/b + ["one provided by the root", { baz: "0.0.5" }], +] as const)( + "siblings that need two versions of a package, %s: each resolves its own version inside the self-contained workspace, and the cache stays intact", + async (_label, rootDependencies) => { + setHandler(dummyRegistry([], { "0.0.3": {}, "0.0.5": {} })); + // a cache outside node_modules, like the global one. The variable takes precedence + // over the bunfig `cache` key, and the test runner sets it. + const cacheDir = join(package_dir, ".bun-cache"); + const cacheEnv = { BUN_INSTALL_CACHE_DIR: cacheDir }; + await writeFile( + join(package_dir, "bunfig.toml"), + Bun.TOML.stringify({ + install: { registry: root_url + "/", saveTextLockfile: true, linker: "hoisted" }, + }), + ); + for (const ws of ["a", "b", "c"]) { + await mkdir(join(package_dir, "packages", ws), { recursive: true }); + } + await writeFile( + join(package_dir, "package.json"), + JSON.stringify({ + name: "root", + private: true, + workspaces: { packages: ["packages/*"], selfContained: ["packages/c"] }, + dependencies: rootDependencies, + }), + ); + await writeFile( + join(package_dir, "packages", "a", "package.json"), + JSON.stringify({ name: "@p/a", version: "1.0.0", dependencies: { baz: "0.0.3" } }), + ); + await writeFile( + join(package_dir, "packages", "b", "package.json"), + JSON.stringify({ name: "@p/b", version: "1.0.0", dependencies: { baz: "0.0.5" } }), + ); + await writeFile( + join(package_dir, "packages", "c", "package.json"), + JSON.stringify({ + name: "@p/c", + version: "1.0.0", + dependencies: { "@p/a": "workspace:*", "@p/b": "workspace:*" }, + }), + ); + + const r = await install(package_dir, [], cacheEnv); + expect(r.err).not.toContain("error:"); + expect(r.code).toBe(0); + + const versionOf = (pkgJson: string) => { + expect(statSync(pkgJson).size).toBeGreaterThan(0); + return JSON.parse(readFileSync(pkgJson, "utf8")).version; + }; + const cDir = join(package_dir, "packages", "c"); + const bazFrom = (sibling: string) => { + const found = findWithin(cDir, join(cDir, "node_modules", "@p", sibling), "baz"); + expect(found).toBeDefined(); + return versionOf(found!); + }; + + expect({ a: bazFrom("a"), b: bazFrom("b") }).toEqual({ a: "0.0.3", b: "0.0.5" }); + + for (const version of ["0.0.3", "0.0.5"]) { + const cached = (await readdirSorted(cacheDir)).filter(name => name.startsWith(`baz@${version}@`)); + expect(cached).toHaveLength(1); + expect(versionOf(join(cacheDir, cached[0], "package.json"))).toBe(version); + expect(statSync(join(cacheDir, cached[0], "index.js")).size).toBeGreaterThan(0); + } + + // a repeat install has nothing to do + const again = await install(package_dir, [], cacheEnv); + expect(again.err).not.toContain("error:"); + expect(again.out).toContain("(no changes)"); + expect(again.code).toBe(0); + expect({ a: bazFrom("a"), b: bazFrom("b") }).toEqual({ a: "0.0.3", b: "0.0.5" }); + }, +); diff --git a/test/cli/install/bun-workspaces.test.ts b/test/cli/install/bun-workspaces.test.ts index bbfe6925b439..7f0ddf3f8bed 100644 --- a/test/cli/install/bun-workspaces.test.ts +++ b/test/cli/install/bun-workspaces.test.ts @@ -1,12 +1,13 @@ import { file, spawn, write } from "bun"; import { install_test_helpers } from "bun:internal-for-testing"; import { afterAll, beforeAll, describe, expect, setDefaultTimeout, test } from "bun:test"; -import { mkdirSync, rmSync, writeFileSync } from "fs"; +import { mkdirSync, readFileSync, rmSync, statSync, writeFileSync } from "fs"; import { cp, exists, mkdir, rm } from "fs/promises"; import { assertManifestsPopulated, bunEnv as baseEnv, bunExe, + isWindows, readdirSorted, runBunInstall, toMatchNodeModulesAt, @@ -2670,3 +2671,66 @@ describe("packages whose version label is longer than 512 bytes", () => { ); }); }); + +// A hardlink install leaves hardlinks of cache files in each node_modules it writes. +// After only the root node_modules is removed, the next install does not delete +// anything first, so a workspace's own node_modules still holds those hardlinks. A copy +// over them must replace the files. Writing through them empties the cache files. +test.concurrent("a copyfile install over a workspace's hardlinked files does not empty the cache", async () => { + using ctx = await setupTest(); + const { packageDir, packageJson, env } = ctx; + await Promise.all([ + write( + packageJson, + JSON.stringify({ name: "foo", workspaces: ["packages/*"], dependencies: { "no-deps": "1.0.0" } }), + ), + write( + join(packageDir, "packages", "pkg1", "package.json"), + JSON.stringify({ name: "pkg1", version: "1.0.0", dependencies: { "no-deps": "2.0.0" } }), + ), + ]); + + async function install(backend: "hardlink" | "copyfile") { + await using proc = spawn({ + cmd: [bunExe(), "install", `--backend=${backend}`], + cwd: packageDir, + stdout: "pipe", + stderr: "pipe", + env, + }); + const [, err, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + expect(err).not.toContain("error:"); + expect(exitCode).toBe(0); + } + + const readJson = (path: string) => { + expect(statSync(path).size).toBeGreaterThan(0); + return JSON.parse(readFileSync(path, "utf8")); + }; + + const nestedPkgJson = join(packageDir, "packages", "pkg1", "node_modules", "no-deps", "package.json"); + await install("hardlink"); + expect(readJson(nestedPkgJson)).toEqual({ name: "no-deps", version: "2.0.0" }); + if (!isWindows) { + expect(statSync(nestedPkgJson).nlink).toBeGreaterThan(1); + } + + await rm(join(packageDir, "node_modules"), { recursive: true, force: true }); + await install("copyfile"); + + expect(readJson(nestedPkgJson)).toEqual({ name: "no-deps", version: "2.0.0" }); + expect(readJson(join(packageDir, "node_modules", "no-deps", "package.json"))).toEqual({ + name: "no-deps", + version: "1.0.0", + }); + if (!isWindows) { + // a new file, not the cache file's inode + expect(statSync(nestedPkgJson).nlink).toBe(1); + } + + const cacheDir = join(packageDir, ".bun-cache"); + const cached = (await readdirSorted(cacheDir)).filter(name => name.startsWith("no-deps@2.0.0@")); + expect(cached).toHaveLength(1); + expect(readJson(join(cacheDir, cached[0], "package.json"))).toEqual({ name: "no-deps", version: "2.0.0" }); + expect(statSync(join(cacheDir, cached[0], "index.js")).size).toBeGreaterThan(0); +}); From d07b37f846e97a34b0a2d8ccfdd56a18ad325e1a Mon Sep 17 00:00:00 2001 From: "autofix-ci[bot]" <114827586+autofix-ci[bot]@users.noreply.github.com> Date: Thu, 3 Sep 2026 02:20:21 +0000 Subject: [PATCH 2/4] [autofix.ci] apply automated fixes --- src/install/PackageInstall.rs | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/src/install/PackageInstall.rs b/src/install/PackageInstall.rs index ec7115ff2b49..5381a0ac382d 100644 --- a/src/install/PackageInstall.rs +++ b/src/install/PackageInstall.rs @@ -1372,17 +1372,19 @@ impl<'a> PackageInstall<'a> { unsafe { windows::DeleteFileW(dest.as_ptr()) }; copy_file() } - _ => match bun_paths::Dirname::dirname_u16(entry.path.as_slice()) - { - Some(entry_dirname) => { - let _ = bun_sys::MakePath::make_path_u16( - destination_dir_, - entry_dirname, - ); - copy_file() + _ => { + match bun_paths::Dirname::dirname_u16(entry.path.as_slice()) + { + Some(entry_dirname) => { + let _ = bun_sys::MakePath::make_path_u16( + destination_dir_, + entry_dirname, + ); + copy_file() + } + None => false, } - None => false, - }, + } }; if copied { continue; From a241e91ae1d1d626215d985fddab07d47e00839b Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Thu, 3 Sep 2026 02:25:05 +0000 Subject: [PATCH 3/4] install: shorten the comments on the copy path, use describe.each in the test The Windows comment said that an overwrite would empty the cache file. CopyFileW fails with a sharing violation over a hardlink of its source, so the comment now says that. --- src/install/PackageInstall.rs | 13 ++++--------- .../install/bun-workspaces-self-contained.test.ts | 11 +++++------ 2 files changed, 9 insertions(+), 15 deletions(-) diff --git a/src/install/PackageInstall.rs b/src/install/PackageInstall.rs index 5381a0ac382d..c59d0c54dce6 100644 --- a/src/install/PackageInstall.rs +++ b/src/install/PackageInstall.rs @@ -1356,11 +1356,8 @@ impl<'a> PackageInstall<'a> { } } EntryKind::File => { - // `bFailIfExists`: an existing destination is deleted and - // copied again, never overwritten in place. It can be a - // hardlink of the cache entry being copied (an earlier - // install of the same package into this folder), and - // overwriting it would empty the file in the cache. + // An existing file can be a hardlink of the source, which + // `CopyFileW` cannot overwrite. Delete it and copy again. // SAFETY: FFI — src/dest are valid NUL-terminated WStr buffers. let copy_file = || unsafe { windows::CopyFileW(src.as_ptr(), dest.as_ptr(), 1) } != 0; @@ -1426,10 +1423,8 @@ impl<'a> PackageInstall<'a> { destination_dir_.fd(), bstr::BStr::new(entry.path.as_bytes()) ); - // An existing destination is replaced, never truncated in - // place: it can be a hardlink of the cache entry being copied - // (an earlier install of the same package into this folder), - // and `O_TRUNC` through it would empty the file in the cache. + // An existing file can be a hardlink of the source, and `O_TRUNC` + // would empty the source too. Unlink it and create a new file. let create = |path: &ZStr| { let open = || { sys::openat( diff --git a/test/cli/install/bun-workspaces-self-contained.test.ts b/test/cli/install/bun-workspaces-self-contained.test.ts index 4e97d37ccf3a..0d270a9c24d9 100644 --- a/test/cli/install/bun-workspaces-self-contained.test.ts +++ b/test/cli/install/bun-workspaces-self-contained.test.ts @@ -192,14 +192,13 @@ function findWithin(root: string, from: string, name: string): string | undefine // c's node_modules goes into the sibling's own node_modules, through the symlink. The // sibling's own tree can write the same directory. The second write used to truncate // the hardlinks of the first, and with them the files in the cache. -it.each([ +describe.each([ // baz@0.0.3 hoists to the root, so b's own tree also nests baz@0.0.5 in packages/b ["one nested in the sibling's own node_modules", {}], // b uses the root's baz@0.0.5, so only c's tree writes it into packages/b ["one provided by the root", { baz: "0.0.5" }], -] as const)( - "siblings that need two versions of a package, %s: each resolves its own version inside the self-contained workspace, and the cache stays intact", - async (_label, rootDependencies) => { +] as const)("siblings that need two versions of a package, %s", (_label, rootDependencies) => { + it("each resolves its own version inside the self-contained workspace, and the cache stays intact", async () => { setHandler(dummyRegistry([], { "0.0.3": {}, "0.0.5": {} })); // a cache outside node_modules, like the global one. The variable takes precedence // over the bunfig `cache` key, and the test runner sets it. @@ -270,5 +269,5 @@ it.each([ expect(again.out).toContain("(no changes)"); expect(again.code).toBe(0); expect({ a: bazFrom("a"), b: bazFrom("b") }).toEqual({ a: "0.0.3", b: "0.0.5" }); - }, -); + }); +}); From 1435b66e935b2de52ad73f9feee2b8d348beb699 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Thu, 3 Sep 2026 02:25:47 +0000 Subject: [PATCH 4/4] install: one-line comments on the copy path --- src/install/PackageInstall.rs | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/install/PackageInstall.rs b/src/install/PackageInstall.rs index c59d0c54dce6..82089252218c 100644 --- a/src/install/PackageInstall.rs +++ b/src/install/PackageInstall.rs @@ -1356,8 +1356,7 @@ impl<'a> PackageInstall<'a> { } } EntryKind::File => { - // An existing file can be a hardlink of the source, which - // `CopyFileW` cannot overwrite. Delete it and copy again. + // `dest` can be a hardlink of `src`, which CopyFileW cannot overwrite. // SAFETY: FFI — src/dest are valid NUL-terminated WStr buffers. let copy_file = || unsafe { windows::CopyFileW(src.as_ptr(), dest.as_ptr(), 1) } != 0; @@ -1423,8 +1422,7 @@ impl<'a> PackageInstall<'a> { destination_dir_.fd(), bstr::BStr::new(entry.path.as_bytes()) ); - // An existing file can be a hardlink of the source, and `O_TRUNC` - // would empty the source too. Unlink it and create a new file. + // The file can be a hardlink of `in_file`, and O_TRUNC would empty both. let create = |path: &ZStr| { let open = || { sys::openat(