diff --git a/src/install/hoisted_install.rs b/src/install/hoisted_install.rs index 81dc1dc5ee53..46f040ec2462 100644 --- a/src/install/hoisted_install.rs +++ b/src/install/hoisted_install.rs @@ -115,6 +115,17 @@ pub(crate) fn install_hoisted_packages( // block above, so no other borrow of `*mgr_ptr` is live here. let this = unsafe { &mut *mgr_ptr }; + // Unfiltered installs only: `--filter` installs leave unselected + // workspaces' `node_modules` alone (`bun prune` is the cleanup for + // those), and the scanner's narrowed pre-install pass is followed by a + // full install that performs the prune. + if packages_to_install.is_none() + && workspace_filters.is_empty() + && this.lockfile.workspace_paths.count() > 0 + { + prune_stale_workspace_node_modules(&this.lockfile, &original_trees, &original_tree_dep_ids); + } + let _restore_buffers = scopeguard::guard( (original_trees, original_tree_dep_ids), move |(trees, dep_ids)| { @@ -617,3 +628,266 @@ pub(crate) fn install_hoisted_packages( Ok(summary) } + +/// Deletes entries of each workspace's `node_modules/` (one level into +/// `@scope/`) whose name the workspace declares as a dependency but the +/// hoisted tree places elsewhere. Such leftovers from a previous +/// package-local install shadow the hoisted copy during resolution, and the +/// installer never visits a workspace tree whose deps all hoisted away +/// (issue #29793). Entries with undeclared names (a manual `bun link`, a +/// hand-dropped folder) are left alone; `bun prune` handles extraneous +/// entries. Only called for unfiltered installs, with the full tree buffers. +fn prune_stale_workspace_node_modules( + lockfile: &crate::lockfile::Lockfile, + trees: &[tree::Tree], + tree_dep_ids: &[DependencyID], +) { + if trees.is_empty() { + return; + } + + let string_buf = lockfile.buffers.string_bytes.as_slice(); + let deps = lockfile.buffers.dependencies.as_slice(); + let resolutions = lockfile.buffers.resolutions.as_slice(); + let packages_slice = lockfile.packages.slice(); + let pkg_resolutions = packages_slice.items_resolution(); + let pkg_dependencies = packages_slice.items_dependencies(); + + // Walk only workspaces that resolve cleanly; pkg_id 0 (stale lockfile, + // hash collision) cannot be trusted to name a workspace. + let mut workspaces_to_walk: Vec<(PackageID, Vec)> = Vec::new(); + { + let hashes = lockfile.workspace_paths.keys(); + let paths = lockfile.workspace_paths.values(); + for (name_hash, ws_path) in hashes.iter().zip(paths.iter()) { + let pkg_id = lockfile.get_workspace_package_id(Some(*name_hash)); + if pkg_id == 0 { + continue; + } + let fs_path = ws_path.slice(string_buf); + if fs_path.is_empty() { + continue; + } + workspaces_to_walk.push((pkg_id, fs_path.to_vec())); + } + } + + if workspaces_to_walk.is_empty() { + return; + } + + // Workspace `node_modules` path -> folder names the tree places there. + let mut expected_by_ws_path: StringHashMap> = StringHashMap::default(); + + let fs_path_for = |pkg_id: PackageID| -> Option<&[u8]> { + workspaces_to_walk + .iter() + .find(|(id, _)| *id == pkg_id) + .map(|(_, p)| p.as_slice()) + }; + + for t in trees { + // Only workspace-package trees are a workspace's `node_modules`. + if t.dependency_id == crate::invalid_dependency_id { + continue; + } + if t.dependency_id == tree::ROOT_DEP_ID { + continue; + } + let dep_idx = t.dependency_id as usize; + if dep_idx >= deps.len() || dep_idx >= resolutions.len() { + continue; + } + let pkg_id = resolutions[dep_idx]; + let pkg_idx = pkg_id as usize; + if pkg_idx >= pkg_resolutions.len() { + continue; + } + if pkg_resolutions[pkg_idx].tag != crate::resolution::Tag::Workspace { + continue; + } + let Some(ws_fs_path) = fs_path_for(pkg_id) else { + continue; + }; + + let key = workspace_node_modules_key(ws_fs_path); + let set = match expected_by_ws_path.get_or_put_value(&key, StringHashMap::default()) { + Ok(v) => v, + Err(_) => continue, + }; + + let tree_deps = t.dependencies.get(tree_dep_ids); + for &dep_id in tree_deps { + let d = dep_id as usize; + if d >= deps.len() { + continue; + } + let dep_name = deps[d].name.slice(string_buf); + if dep_name.is_empty() { + continue; + } + let _ = set.put(dep_name, ()); + } + } + + // For each workspace, prune the declared dependency names the tree does + // not place in that workspace's own `node_modules`. + for (pkg_id, fs_path) in &workspaces_to_walk { + let pkg_idx = *pkg_id as usize; + if pkg_idx >= pkg_dependencies.len() { + continue; + } + let key = workspace_node_modules_key(fs_path); + let placed = expected_by_ws_path.get(key.as_slice()); + + let mut prunable: StringHashMap<()> = StringHashMap::default(); + for dep in pkg_dependencies[pkg_idx].get(deps) { + let name = dep.name.slice(string_buf); + if name.is_empty() { + continue; + } + if placed.is_some_and(|p| p.contains_key(name)) { + continue; + } + let _ = prunable.put(name, ()); + } + if prunable.is_empty() { + continue; + } + prune_node_modules_at(&key, &prunable); + } +} + +/// Normalized `/node_modules` key, shared by the placed-set map and +/// the walk so the two can't diverge (a miss would treat placed entries as +/// prunable). +fn workspace_node_modules_key(ws_path: &[u8]) -> Vec { + let trimmed = match ws_path.last() { + Some(b'/') | Some(b'\\') => &ws_path[..ws_path.len() - 1], + _ => ws_path, + }; + let mut key = Vec::with_capacity(trimmed.len() + b"/node_modules".len()); + key.extend_from_slice(trimmed); + key.extend_from_slice(b"/node_modules"); + // Canonicalize separators so the Windows hash lookup matches. + if cfg!(windows) { + for b in key.iter_mut() { + if *b == b'\\' { + *b = b'/'; + } + } + } + key +} + +/// Opens `/` and removes each top-level directory entry whose +/// name is in `prunable`. Also descends one level into `@scope/` directories +/// so scoped packages are handled. Missing directories are ignored. +fn prune_node_modules_at(rel_path: &[u8], prunable: &StringHashMap<()>) { + let cwd = Fd::cwd(); + // `Dir` closes the fd on drop. + let dir = match sys::open_dir_for_iteration(cwd, rel_path) { + Ok(fd) => Dir::from_fd(fd), + Err(_) => return, + }; + + // Snapshot the listing first: deleting during batched readdir iteration + // can re-surface entries on some filesystems. + let mut names: Vec> = Vec::new(); + let mut iter = sys::iterate_dir(dir.fd()); + loop { + let entry = match iter.next() { + Ok(Some(e)) => e, + Ok(None) => break, + Err(_) => return, + }; + let name = entry.name.slice_u8(); + if name.is_empty() || name[0] == b'.' { + continue; + } + names.push(name.to_vec()); + } + + let mut deleted_any = false; + for name in &names { + if name[0] == b'@' { + // The prunable set stores scoped packages as `@scope/pkg`. + deleted_any |= prune_scoped_node_modules(&dir, name, prunable); + continue; + } + if prunable.contains_key(name.as_slice()) && dir.delete_tree(name).is_ok() { + deleted_any = true; + } + } + + // A removed package may have had bins linked into this `.bin`; sweep the + // now-dangling links like `remove_collapsed_copies` does. + if deleted_any { + crate::prune::prune_bins(&dir); + } +} + +/// Returns whether any entry was deleted. +fn prune_scoped_node_modules(parent: &Dir, scope: &[u8], prunable: &StringHashMap<()>) -> bool { + // `open_real_subdir` refuses a symlinked `@scope`, so the deletes below + // cannot escape the workspace's `node_modules`. + let Some(scope_dir) = crate::prune::open_real_subdir(parent, scope) else { + return false; + }; + + let mut names: Vec> = Vec::new(); + let mut has_remaining = false; + let mut iter = sys::iterate_dir(scope_dir.fd()); + loop { + let entry = match iter.next() { + Ok(Some(e)) => e, + Ok(None) => break, + Err(_) => return false, + }; + let name = entry.name.slice_u8(); + if name.is_empty() { + continue; + } + if name[0] == b'.' { + has_remaining = true; + continue; + } + names.push(name.to_vec()); + } + + let mut deleted_any = false; + for name in &names { + let mut full_name = Vec::with_capacity(scope.len() + 1 + name.len()); + full_name.extend_from_slice(scope); + full_name.push(b'/'); + full_name.extend_from_slice(name); + + if !prunable.contains_key(full_name.as_slice()) { + has_remaining = true; + continue; + } + + if scope_dir.delete_tree(name).is_err() { + has_remaining = true; + } else { + deleted_any = true; + } + } + + // Close before removing, as `Dir::delete_tree` requires on Windows. + drop(iter); + drop(scope_dir); + + // Remove the scope directory if it ended up empty. + if !has_remaining { + let mut scope_z = Vec::with_capacity(scope.len() + 1); + scope_z.extend_from_slice(scope); + scope_z.push(0); + let z = bun_core::ZStr::from_buf(&scope_z, scope.len()); + let _ = sys::rmdirat(parent.fd(), z); + } + + deleted_any +} + +// ported from: src/install/hoisted_install.zig diff --git a/src/install/prune.rs b/src/install/prune.rs index cffaa8014ccc..dcac7ce0a96f 100644 --- a/src/install/prune.rs +++ b/src/install/prune.rs @@ -1322,7 +1322,7 @@ fn entry_kind_of(dir: &Dir, alias: &[u8]) -> EntryKind { } } -fn open_real_subdir(dir: &Dir, name: &[u8]) -> Option { +pub(crate) fn open_real_subdir(dir: &Dir, name: &[u8]) -> Option { if lstat_kind(dir, name) != EntryKind::Directory { return None; } @@ -1787,7 +1787,7 @@ fn unlink_links(dir: &Dir, should_unlink: &dyn Fn(&Dir, &[u8], &[u8]) -> bool) { } #[cfg(not(windows))] -fn prune_bins(dir: &Dir) { +pub(crate) fn prune_bins(dir: &Dir) { let Some(bin) = open_real_subdir(dir, b".bin") else { return; }; @@ -1807,7 +1807,7 @@ fn prune_bins(dir: &Dir) { // `.bunx` layout: windows-shim/BinLinkingShim.rs (target path is relative to this node_modules folder). #[cfg(windows)] -fn prune_bins(dir: &Dir) { +pub(crate) fn prune_bins(dir: &Dir) { let Some(bin) = open_real_subdir(dir, b".bin") else { return; }; diff --git a/test/cli/install/bun-update.test.ts b/test/cli/install/bun-update.test.ts index e5dc789cc108..c69eebe436f2 100644 --- a/test/cli/install/bun-update.test.ts +++ b/test/cli/install/bun-update.test.ts @@ -1,5 +1,5 @@ import { file, spawn } from "bun"; -import { afterAll, afterEach, beforeAll, beforeEach, describe, expect, it } from "bun:test"; +import { afterAll, afterEach, beforeAll, beforeEach, describe, expect, it, setDefaultTimeout } from "bun:test"; import { access, appendFile, exists, mkdir, readFile, rm, writeFile } from "fs/promises"; import { VerdaccioRegistry, bunExe, bunEnv as env, pack, readdirSorted, toBeValidBin, toHaveBins } from "harness"; import { basename, dirname, join } from "path"; @@ -15,6 +15,7 @@ import { setHandler, } from "./dummy.registry.js"; +setDefaultTimeout(1000 * 60 * 5); beforeAll(dummyBeforeAll); afterAll(dummyAfterAll); beforeEach(async () => { @@ -542,7 +543,7 @@ it("--filter updates only matching workspaces, leaving siblings and root untouch expect(root.dependencies.baz).toBe("~0.0.3"); }); -// The exact pin is installed first, then widened to a range its locked resolution satisfies, so the nested copy stays. +// The exact pin is installed first, then widened to a range the hoisted copy satisfies, collapsing the nested row. async function nestedBazRepo(rootRange: string, pkgARange: string, rewrite: { root?: string; pkgA?: string }) { setHandler(dummyRegistry([], { "0.0.3": {}, "0.0.5": {}, latest: "0.0.5" })); const rootJson = (range: string) => @@ -565,7 +566,13 @@ const pkgABazVersion = async () => (await file(join(pkgABazDir(), "package.json" it("--filter pkg-a removes the nested copy whose row it collapsed", async () => { await nestedBazRepo("0.0.5", "0.0.3", { pkgA: "~0.0.3" }); expect(await rootBazVersion()).toBe("0.0.5"); - expect(await pkgABazVersion()).toBe("0.0.3"); + // The unfiltered install that applied the widened range collapsed pkg-a's + // nested row and pruned the on-disk copy with it (#29793). + expect(await exists(pkgABazDir())).toBeFalse(); + + // Re-plant the stale nested copy; the filtered update must remove it too. + await mkdir(pkgABazDir(), { recursive: true }); + await writeFile(join(pkgABazDir(), "package.json"), JSON.stringify({ name: "baz", version: "0.0.3" })); const { stderr, exited } = spawn({ cmd: [bunExe(), "update", "--filter", "pkg-a", "--linker=hoisted"], @@ -2835,3 +2842,324 @@ describe("bun update semantics", () => { }); }); }); + +// https://github.com/oven-sh/bun/issues/29793 +it("hoisted install removes stale workspace-local node_modules that shadow the hoisted version", async () => { + const registry = { + "0.0.3": {}, + "0.0.5": {}, + latest: "0.0.5", + }; + setHandler(dummyRegistry([], registry)); + + // Root workspace — declares the workspace, no deps. + await writeFile( + join(package_dir, "package.json"), + JSON.stringify({ + name: "root", + private: true, + workspaces: ["packages/backend"], + }), + ); + + // Backend workspace depends on baz. + await mkdir(join(package_dir, "packages", "backend"), { recursive: true }); + await writeFile( + join(package_dir, "packages", "backend", "package.json"), + JSON.stringify({ + name: "@repro/backend", + dependencies: { baz: "^0.0.3" }, + }), + ); + + // Pre-existing stale workspace-local package, simulating what an earlier + // package-local install (or manual edit) can leave behind. + await mkdir(join(package_dir, "packages", "backend", "node_modules", "baz"), { recursive: true }); + await writeFile( + join(package_dir, "packages", "backend", "node_modules", "baz", "package.json"), + JSON.stringify({ name: "baz", version: "0.0.0-stale" }), + ); + + const { stderr, exited } = spawn({ + cmd: [bunExe(), "update", "--latest", "baz", "--linker=hoisted"], + cwd: join(package_dir, "packages", "backend"), + stdout: "ignore", + stderr: "pipe", + env, + }); + + const err = await new Response(stderr).text(); + expect(err).not.toContain("error:"); + expect(await exited).toBe(0); + + // baz should be hoisted to root node_modules at the updated version … + expect(await file(join(package_dir, "node_modules", "baz", "package.json")).json()).toMatchObject({ + name: "baz", + version: "0.0.5", + }); + + // … and the stale workspace-local copy must be gone so module resolution + // from the backend workspace finds the hoisted one instead of the shadow. + expect(await exists(join(package_dir, "packages", "backend", "node_modules", "baz"))).toBe(false); +}); + +// Guard against over-eager pruning: a workspace-local copy that the lockfile +// legitimately places there (because it couldn't hoist due to a root conflict) +// must survive a subsequent install. +it("hoisted install preserves non-hoistable workspace-local packages", async () => { + const registry = { + "0.0.3": {}, + "0.0.5": {}, + latest: "0.0.5", + }; + setHandler(dummyRegistry([], registry)); + + // Root pins baz@0.0.3; workspace pins baz@0.0.5 → the workspace copy can't + // hoist and must live at `packages/backend/node_modules/baz`. + await writeFile( + join(package_dir, "package.json"), + JSON.stringify({ + name: "root", + private: true, + workspaces: ["packages/backend"], + dependencies: { baz: "0.0.3" }, + }), + ); + + await mkdir(join(package_dir, "packages", "backend"), { recursive: true }); + await writeFile( + join(package_dir, "packages", "backend", "package.json"), + JSON.stringify({ + name: "@repro/backend", + dependencies: { baz: "0.0.5" }, + }), + ); + + // Initial install lays everything out. + { + const { stderr, exited } = spawn({ + cmd: [bunExe(), "install", "--linker=hoisted"], + cwd: package_dir, + stdout: "ignore", + stderr: "pipe", + env, + }); + expect(await new Response(stderr).text()).not.toContain("error:"); + expect(await exited).toBe(0); + } + + expect(await file(join(package_dir, "node_modules", "baz", "package.json")).json()).toMatchObject({ + name: "baz", + version: "0.0.3", + }); + expect( + await file(join(package_dir, "packages", "backend", "node_modules", "baz", "package.json")).json(), + ).toMatchObject({ + name: "baz", + version: "0.0.5", + }); + + // Second install must not wipe the legitimate workspace-local copy. + { + const { stderr, exited } = spawn({ + cmd: [bunExe(), "install", "--linker=hoisted"], + cwd: package_dir, + stdout: "ignore", + stderr: "pipe", + env, + }); + expect(await new Response(stderr).text()).not.toContain("error:"); + expect(await exited).toBe(0); + } + + expect( + await file(join(package_dir, "packages", "backend", "node_modules", "baz", "package.json")).json(), + ).toMatchObject({ + name: "baz", + version: "0.0.5", + }); +}); + +// `bun install --filter ` leaves an excluded workspace's node_modules +// alone entirely (stale entries included); a later unfiltered install prunes +// the stale copy of a declared dep while keeping the legit non-hoistable copy. +it("hoisted install with --filter preserves excluded workspace's non-hoistable packages", async () => { + const registry = { + "0.0.3": {}, + "0.0.5": {}, + "0.1.0": {}, + latest: "0.0.5", + }; + setHandler(dummyRegistry([], registry)); + + // Root pins baz@0.0.3 and declares one workspace that pins baz@0.0.5. The + // workspace's copy can't hoist (root already claims baz) so it lands at + // `packages/excluded/node_modules/baz`. The workspace's moo has no root + // conflict and hoists. + await writeFile( + join(package_dir, "package.json"), + JSON.stringify({ + name: "root", + private: true, + workspaces: ["packages/*"], + dependencies: { baz: "0.0.3" }, + }), + ); + await mkdir(join(package_dir, "packages", "excluded"), { recursive: true }); + await writeFile( + join(package_dir, "packages", "excluded", "package.json"), + JSON.stringify({ name: "excluded", dependencies: { baz: "0.0.5", moo: "^0.1.0" } }), + ); + await mkdir(join(package_dir, "packages", "target"), { recursive: true }); + await writeFile( + join(package_dir, "packages", "target", "package.json"), + JSON.stringify({ name: "target", dependencies: { baz: "0.0.3" } }), + ); + + // First, a full install lays out both workspaces' node_modules. + { + const { stderr, exited } = spawn({ + cmd: [bunExe(), "install", "--linker=hoisted"], + cwd: package_dir, + stdout: "ignore", + stderr: "pipe", + env, + }); + expect(await new Response(stderr).text()).not.toContain("error:"); + expect(await exited).toBe(0); + } + + // Confirm the expected layout before the filtered install runs: root gets + // baz 0.0.3 and the hoisted moo, the excluded workspace has its own 0.0.5. + expect(await file(join(package_dir, "node_modules", "baz", "package.json")).json()).toMatchObject({ + name: "baz", + version: "0.0.3", + }); + expect(await exists(join(package_dir, "node_modules", "moo"))).toBe(true); + expect( + await file(join(package_dir, "packages", "excluded", "node_modules", "baz", "package.json")).json(), + ).toMatchObject({ + name: "baz", + version: "0.0.5", + }); + + // Leave a stale copy of the hoisted moo in the workspace we'll exclude. + await mkdir(join(package_dir, "packages", "excluded", "node_modules", "moo"), { recursive: true }); + await writeFile( + join(package_dir, "packages", "excluded", "node_modules", "moo", "package.json"), + JSON.stringify({ name: "moo", version: "0.0.0-stale" }), + ); + + // Now run an install filtered to `target` only. + { + const { stderr, exited } = spawn({ + cmd: [bunExe(), "install", "--filter", "target", "--linker=hoisted"], + cwd: package_dir, + stdout: "ignore", + stderr: "pipe", + env, + }); + expect(await new Response(stderr).text()).not.toContain("error:"); + expect(await exited).toBe(0); + } + + // The filtered install leaves the excluded workspace untouched: both the + // legit non-hoistable copy and the stale entry survive. + expect( + await file(join(package_dir, "packages", "excluded", "node_modules", "baz", "package.json")).json(), + ).toMatchObject({ + name: "baz", + version: "0.0.5", + }); + expect(await exists(join(package_dir, "packages", "excluded", "node_modules", "moo"))).toBe(true); + + // A later unfiltered install prunes the stale copy of the hoisted dep but + // keeps the legit non-hoistable copy. + { + const { stderr, exited } = spawn({ + cmd: [bunExe(), "install", "--linker=hoisted"], + cwd: package_dir, + stdout: "ignore", + stderr: "pipe", + env, + }); + expect(await new Response(stderr).text()).not.toContain("error:"); + expect(await exited).toBe(0); + } + expect( + await file(join(package_dir, "packages", "excluded", "node_modules", "baz", "package.json")).json(), + ).toMatchObject({ + name: "baz", + version: "0.0.5", + }); + expect(await exists(join(package_dir, "packages", "excluded", "node_modules", "moo"))).toBe(false); +}); + +// Stale workspace-local copies of declared deps are pruned during a plain +// `bun install` (not just update): the scoped one's empty `@scope` parent is +// cleaned up too, and entries whose name the workspace does not declare (a +// manual link, a hand-dropped folder) are left alone. +it("hoisted install prunes stale scoped workspace-local entries", async () => { + const registry = { + "0.0.3": {}, + "0.0.5": {}, + "0.1.0": {}, + latest: "0.0.5", + }; + setHandler(dummyRegistry([], registry)); + + await writeFile( + join(package_dir, "package.json"), + JSON.stringify({ + name: "root", + private: true, + workspaces: ["packages/backend"], + }), + ); + await mkdir(join(package_dir, "packages", "backend"), { recursive: true }); + // Both deps hoist to the root (no conflicts); the scoped one is an alias so + // its folder name is `@scope/dep`. + await writeFile( + join(package_dir, "packages", "backend", "package.json"), + JSON.stringify({ + name: "@repro/backend", + dependencies: { baz: "^0.0.3", "@scope/dep": "npm:moo@^0.1.0" }, + }), + ); + + // Pre-existing stale local copies of the declared deps, plus a foreign + // directory the lockfile knows nothing about. + const backendModules = join(package_dir, "packages", "backend", "node_modules"); + await mkdir(join(backendModules, "baz"), { recursive: true }); + await writeFile(join(backendModules, "baz", "package.json"), JSON.stringify({ name: "baz", version: "0.0.0" })); + await mkdir(join(backendModules, "@scope", "dep"), { recursive: true }); + await writeFile( + join(backendModules, "@scope", "dep", "package.json"), + JSON.stringify({ name: "moo", version: "0.0.0" }), + ); + await mkdir(join(backendModules, "hand-linked"), { recursive: true }); + await writeFile( + join(backendModules, "hand-linked", "package.json"), + JSON.stringify({ name: "hand-linked", version: "0.0.0" }), + ); + + const { stderr, exited } = spawn({ + cmd: [bunExe(), "install", "--linker=hoisted"], + cwd: package_dir, + stdout: "ignore", + stderr: "pipe", + env, + }); + expect(await new Response(stderr).text()).not.toContain("error:"); + expect(await exited).toBe(0); + + // Hoisted copies land at the root. + expect(await exists(join(package_dir, "node_modules", "baz"))).toBe(true); + expect(await exists(join(package_dir, "node_modules", "@scope", "dep"))).toBe(true); + // The stale local copies are gone, along with the empty `@scope` parent; + // the foreign directory survives. + expect(await exists(join(backendModules, "baz"))).toBe(false); + expect(await exists(join(backendModules, "@scope", "dep"))).toBe(false); + expect(await exists(join(backendModules, "@scope"))).toBe(false); + expect(await exists(join(backendModules, "hand-linked"))).toBe(true); +});