From 1d775c9bf37bedfcbf0d305e5061fd09cac4a549 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Fri, 5 Jun 2026 21:41:53 +0000 Subject: [PATCH 1/8] install: prune stale workspace node_modules in hoisted installs Closes #29793. A previous package-local install (or a manual edit) can leave `packages//node_modules/` behind. When the current hoisted install hoists `` to the root, the leftover workspace-local copy shadows the hoisted one during module resolution, so `bun update --latest --linker hoisted` reports success while the workspace keeps resolving the stale version. The hoisted installer drives off the lockfile tree, which only lists `node_modules` directories that still hold dependencies. When a workspace's deps all hoist to the root, its own tree is dropped and the installer never visits `packages//node_modules`, so the stale entry is never removed (isolated installs rebuild each workspace's `node_modules` from scratch and sidestep this). Before the install loop runs, walk each workspace's `node_modules/` (plus one level into `@scope/`) and delete any entry the tree layout doesn't place there. The expected set is built from the unfiltered tree buffers captured before `Lockfile::filter()`, so `--filter`-excluded workspaces keep the packages they legitimately own: only genuinely stale entries (not placed anywhere by the lockfile) are removed. The prune is skipped on the security scanner's narrowed pre-install pass (`packages_to_install`); the full install that follows performs it. Tests: stale removal + hoist, non-hoistable workspace-local copy preserved across re-install, `--filter` preservation of excluded workspaces, and scoped-package (`@scope/pkg`) pruning with empty-scope cleanup. --- src/install/hoisted_install.rs | 276 ++++++++++++++++++++++++++ test/cli/install/bun-update.test.ts | 287 +++++++++++++++++++++++++++- 2 files changed, 562 insertions(+), 1 deletion(-) diff --git a/src/install/hoisted_install.rs b/src/install/hoisted_install.rs index 81dc1dc5ee53..3cf80b5b4e6f 100644 --- a/src/install/hoisted_install.rs +++ b/src/install/hoisted_install.rs @@ -115,6 +115,27 @@ pub(crate) fn install_hoisted_packages( // block above, so no other borrow of `*mgr_ptr` is live here. let this = unsafe { &mut *mgr_ptr }; + // Remove stale packages from workspace `node_modules` directories before + // the install loop runs. A previous install (especially a package-local + // one) may have placed packages inside `packages//node_modules` + // that the current hoisted layout no longer expects — those directories + // would shadow the hoisted copies during module resolution. The installer + // only visits trees that still have dependencies, so without this sweep the + // stale entries are never seen, let alone removed. + // + // We build the expected set from the *unfiltered* `original_trees` on + // purpose: with `--filter` the filtered tree omits excluded workspaces + // entirely, but those workspaces' `node_modules` still belong to them and + // must not be wiped based on what the _current_ install would re-create. + // (Issue #29793.) + // + // Skipped when `packages_to_install` narrows the pass (the security + // scanner's pre-install of just the scanner package) — the full install + // that follows performs the same prune. + if packages_to_install.is_none() && 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 +638,258 @@ pub(crate) fn install_hoisted_packages( Ok(summary) } + +/// Walks each workspace's `node_modules/` directory on disk and deletes any +/// package folder the current hoisted tree does not list as belonging there. +/// +/// A previous package-local install (or a manual edit) may have left +/// `packages//node_modules/` behind. When the current install +/// hoists `` to the root, the leftover workspace-local copy shadows the +/// hoisted one during module resolution. The tree iterator only visits +/// `node_modules` directories that still contain entries, so those stale +/// folders are otherwise never seen, let alone removed. +/// +/// Only operates on top-level entries of each workspace's `node_modules/` +/// (plus one level into `@scope/` directories). Transitive `node_modules` +/// nested inside surviving packages are handled by the normal install +/// verify/uninstall path. +/// +/// `trees`/`tree_dep_ids` are the **unfiltered** buffers captured before +/// `Lockfile::filter()` runs, so entries a `--filter`-excluded workspace +/// legitimately owns stay put (see the call site / issue #29793). +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(); + + // Resolve every workspace to (package id, filesystem-relative path). Only + // workspaces that resolve cleanly are walked — if `get_workspace_package_id` + // returns 0 (name-hash collision, stale lockfile, workspace dropped from + // `packages`) we leave that directory alone rather than wipe it with an + // empty expected set. + 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; + } + + // Map of workspace filesystem-relative `node_modules` path (posix + // separators) to the set of folder names the tree places directly there. + // Only workspace-scope trees are indexed — transitive nested trees aren't + // workspace node_modules we need to prune. + let mut expected_by_ws_path: StringHashMap> = StringHashMap::default(); + + // Quick lookup from workspace package id → filesystem path for the tree loop. + 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 trees whose `dependency_id` resolves to a workspace package + // correspond to a workspace's `node_modules` directory. + 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, ()); + } + } + + // Walk only the workspaces we resolved above. + for (_pkg_id, fs_path) in &workspaces_to_walk { + let key = workspace_node_modules_key(fs_path); + let expected = expected_by_ws_path.get(key.as_slice()); + prune_node_modules_at(&key, expected); + } +} + +/// Builds the normalized `/node_modules` key used both to index the +/// expected-set map and to look it up during the walk. Single source of truth +/// so the two call sites can't silently diverge — a mismatch would route +/// pruning through the `expected == None` branch and delete legitimate entries. +fn workspace_node_modules_key(ws_path: &[u8]) -> Vec { + // Tolerate a stray trailing separator (either `/` or `\`). + 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"); + // The on-disk walk uses the key as-is; normalize any backslash separators + // to forward slashes so the hash lookup is canonical on Windows. + 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 not present in `expected`. Also descends one level into `@scope/` +/// directories so scoped packages are handled. Missing directories are ignored. +fn prune_node_modules_at(rel_path: &[u8], expected: Option<&StringHashMap<()>>) { + let cwd = Fd::cwd(); + // `Dir` owns the fd (closes on drop) — no separate scopeguard, or we'd + // double-close. + let dir = match sys::open_dir_for_iteration(cwd, rel_path) { + Ok(fd) => Dir::from_fd(fd), + Err(_) => return, + }; + + // Snapshot the directory listing before deleting anything. `delete_tree` + // mutates the parent directory, and the platform readdir iterators read in + // batches — deleting the current entry is fine, but a batch refill after a + // delete can re-surface entries on some filesystems. Iterating an owned + // list keeps the loop independent of the underlying directory shape. + 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()); + } + + for name in &names { + if name[0] == b'@' { + // Scoped package directory. Recurse one level; the expected set + // stores scoped packages as `@scope/pkg`. + prune_scoped_node_modules(dir.fd(), name, expected); + continue; + } + if let Some(exp) = expected { + if exp.contains_key(name.as_slice()) { + continue; + } + } + let _ = dir.delete_tree(name); + } +} + +fn prune_scoped_node_modules(parent_dir: Fd, scope: &[u8], expected: Option<&StringHashMap<()>>) { + let scope_dir = match sys::open_dir_for_iteration(parent_dir, scope) { + Ok(fd) => Dir::from_fd(fd), + Err(_) => return, + }; + + 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, + }; + let name = entry.name.slice_u8(); + if name.is_empty() { + continue; + } + if name[0] == b'.' { + has_remaining = true; + continue; + } + names.push(name.to_vec()); + } + + 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 let Some(exp) = expected { + if exp.contains_key(full_name.as_slice()) { + has_remaining = true; + continue; + } + } + + if scope_dir.delete_tree(name).is_err() { + has_remaining = true; + } + } + + // 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_dir, z); + } +} + +// ported from: src/install/hoisted_install.zig diff --git a/test/cli/install/bun-update.test.ts b/test/cli/install/bun-update.test.ts index e5dc789cc108..8c44393d8498 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,11 @@ import { setHandler, } from "./dummy.registry.js"; +// ASAN + slow CI filesystems can push individual install spawns past the +// default 5s timeout. `setDefaultTimeout` is read at `it()` call time, so it +// must run during module load (before the `it()` declarations below), not +// inside `beforeAll`. Match the convention other install test files use. +setDefaultTimeout(1000 * 60 * 5); beforeAll(dummyBeforeAll); afterAll(dummyAfterAll); beforeEach(async () => { @@ -2835,3 +2840,283 @@ 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 ` must not destroy legitimate nested packages +// in a workspace that was excluded by the filter. The prune walks the +// unfiltered tree layout, so entries the full lockfile expects are kept. +it("hoisted install with --filter preserves excluded workspace's non-hoistable packages", async () => { + const registry = { + "0.0.3": {}, + "0.0.5": {}, + 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`. + 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" } }), + ); + 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 + // 0.0.3, 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 file(join(package_dir, "packages", "excluded", "node_modules", "baz", "package.json")).json(), + ).toMatchObject({ + name: "baz", + version: "0.0.5", + }); + + // Leave a stale directory in the workspace we'll exclude — the prune still + // cleans it up because it's not in the lockfile anywhere. + await mkdir(join(package_dir, "packages", "excluded", "node_modules", "stale"), { recursive: true }); + await writeFile( + join(package_dir, "packages", "excluded", "node_modules", "stale", "package.json"), + JSON.stringify({ name: "stale", version: "0.0.0" }), + ); + + // 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 excluded workspace's legit non-hoistable copy must still be there. + expect( + await file(join(package_dir, "packages", "excluded", "node_modules", "baz", "package.json")).json(), + ).toMatchObject({ + name: "baz", + version: "0.0.5", + }); + // And the genuinely stale entry in that same directory is gone. + expect(await exists(join(package_dir, "packages", "excluded", "node_modules", "stale"))).toBe(false); +}); + +// Stale scoped packages (`@scope/pkg`) must also be pruned, and the empty +// `@scope` directory shouldn't be left behind. +it("hoisted install prunes stale scoped workspace-local entries", async () => { + const registry = { + "0.0.3": {}, + "0.0.5": {}, + 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 }); + await writeFile( + join(package_dir, "packages", "backend", "package.json"), + JSON.stringify({ + name: "@repro/backend", + dependencies: { baz: "^0.0.3" }, + }), + ); + + // Pre-existing stale scoped package. + await mkdir(join(package_dir, "packages", "backend", "node_modules", "@stale", "pkg"), { + recursive: true, + }); + await writeFile( + join(package_dir, "packages", "backend", "node_modules", "@stale", "pkg", "package.json"), + JSON.stringify({ name: "@stale/pkg", 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); + + // The scoped package directory is gone, along with the empty `@stale` parent. + expect(await exists(join(package_dir, "packages", "backend", "node_modules", "@stale", "pkg"))).toBe(false); + expect(await exists(join(package_dir, "packages", "backend", "node_modules", "@stale"))).toBe(false); +}); From 42d14b79d54643e7e2e64ad4a84a2e2933fbee45 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Thu, 13 Aug 2026 22:05:14 +0000 Subject: [PATCH 2/8] Trim oversized comments in the prune pass --- src/install/hoisted_install.rs | 86 +++++++++------------------------- 1 file changed, 23 insertions(+), 63 deletions(-) diff --git a/src/install/hoisted_install.rs b/src/install/hoisted_install.rs index 3cf80b5b4e6f..00460f51557b 100644 --- a/src/install/hoisted_install.rs +++ b/src/install/hoisted_install.rs @@ -115,23 +115,10 @@ pub(crate) fn install_hoisted_packages( // block above, so no other borrow of `*mgr_ptr` is live here. let this = unsafe { &mut *mgr_ptr }; - // Remove stale packages from workspace `node_modules` directories before - // the install loop runs. A previous install (especially a package-local - // one) may have placed packages inside `packages//node_modules` - // that the current hoisted layout no longer expects — those directories - // would shadow the hoisted copies during module resolution. The installer - // only visits trees that still have dependencies, so without this sweep the - // stale entries are never seen, let alone removed. - // - // We build the expected set from the *unfiltered* `original_trees` on - // purpose: with `--filter` the filtered tree omits excluded workspaces - // entirely, but those workspaces' `node_modules` still belong to them and - // must not be wiped based on what the _current_ install would re-create. - // (Issue #29793.) - // - // Skipped when `packages_to_install` narrows the pass (the security - // scanner's pre-install of just the scanner package) — the full install - // that follows performs the same prune. + // Must use the *unfiltered* `original_trees`: the filtered tree omits + // `--filter`-excluded workspaces, whose `node_modules` must not be wiped. + // Skipped on the security scanner's narrowed pre-install pass; the full + // install that follows performs the prune. if packages_to_install.is_none() && this.lockfile.workspace_paths.count() > 0 { prune_stale_workspace_node_modules(&this.lockfile, &original_trees, &original_tree_dep_ids); } @@ -639,24 +626,13 @@ pub(crate) fn install_hoisted_packages( Ok(summary) } -/// Walks each workspace's `node_modules/` directory on disk and deletes any -/// package folder the current hoisted tree does not list as belonging there. -/// -/// A previous package-local install (or a manual edit) may have left -/// `packages//node_modules/` behind. When the current install -/// hoists `` to the root, the leftover workspace-local copy shadows the -/// hoisted one during module resolution. The tree iterator only visits -/// `node_modules` directories that still contain entries, so those stale -/// folders are otherwise never seen, let alone removed. -/// -/// Only operates on top-level entries of each workspace's `node_modules/` -/// (plus one level into `@scope/` directories). Transitive `node_modules` -/// nested inside surviving packages are handled by the normal install -/// verify/uninstall path. -/// -/// `trees`/`tree_dep_ids` are the **unfiltered** buffers captured before -/// `Lockfile::filter()` runs, so entries a `--filter`-excluded workspace -/// legitimately owns stay put (see the call site / issue #29793). +/// Deletes top-level entries of each workspace's `node_modules/` (one level +/// into `@scope/`) that the hoisted tree does not place there. Stale 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). `trees`/`tree_dep_ids` must be the unfiltered +/// buffers captured before `Lockfile::filter()` so `--filter`-excluded +/// workspaces keep the entries they own. fn prune_stale_workspace_node_modules( lockfile: &crate::lockfile::Lockfile, trees: &[tree::Tree], @@ -672,11 +648,8 @@ fn prune_stale_workspace_node_modules( let packages_slice = lockfile.packages.slice(); let pkg_resolutions = packages_slice.items_resolution(); - // Resolve every workspace to (package id, filesystem-relative path). Only - // workspaces that resolve cleanly are walked — if `get_workspace_package_id` - // returns 0 (name-hash collision, stale lockfile, workspace dropped from - // `packages`) we leave that directory alone rather than wipe it with an - // empty expected set. + // Walk only workspaces that resolve cleanly; pkg_id 0 (stale lockfile, + // hash collision) would otherwise wipe the dir with an empty expected set. let mut workspaces_to_walk: Vec<(PackageID, Vec)> = Vec::new(); { let hashes = lockfile.workspace_paths.keys(); @@ -698,13 +671,9 @@ fn prune_stale_workspace_node_modules( return; } - // Map of workspace filesystem-relative `node_modules` path (posix - // separators) to the set of folder names the tree places directly there. - // Only workspace-scope trees are indexed — transitive nested trees aren't - // workspace node_modules we need to prune. + // Workspace `node_modules` path -> folder names the tree places there. let mut expected_by_ws_path: StringHashMap> = StringHashMap::default(); - // Quick lookup from workspace package id → filesystem path for the tree loop. let fs_path_for = |pkg_id: PackageID| -> Option<&[u8]> { workspaces_to_walk .iter() @@ -713,8 +682,7 @@ fn prune_stale_workspace_node_modules( }; for t in trees { - // Only trees whose `dependency_id` resolves to a workspace package - // correspond to a workspace's `node_modules` directory. + // Only workspace-package trees are a workspace's `node_modules`. if t.dependency_id == crate::invalid_dependency_id { continue; } @@ -765,12 +733,10 @@ fn prune_stale_workspace_node_modules( } } -/// Builds the normalized `/node_modules` key used both to index the -/// expected-set map and to look it up during the walk. Single source of truth -/// so the two call sites can't silently diverge — a mismatch would route -/// pruning through the `expected == None` branch and delete legitimate entries. +/// Normalized `/node_modules` key, shared by the expected-set map +/// and the walk so the two can't diverge (a miss routes through the +/// `expected == None` branch and deletes legitimate entries). fn workspace_node_modules_key(ws_path: &[u8]) -> Vec { - // Tolerate a stray trailing separator (either `/` or `\`). let trimmed = match ws_path.last() { Some(b'/') | Some(b'\\') => &ws_path[..ws_path.len() - 1], _ => ws_path, @@ -778,8 +744,7 @@ fn workspace_node_modules_key(ws_path: &[u8]) -> Vec { let mut key = Vec::with_capacity(trimmed.len() + b"/node_modules".len()); key.extend_from_slice(trimmed); key.extend_from_slice(b"/node_modules"); - // The on-disk walk uses the key as-is; normalize any backslash separators - // to forward slashes so the hash lookup is canonical on Windows. + // Canonicalize separators so the Windows hash lookup matches. if cfg!(windows) { for b in key.iter_mut() { if *b == b'\\' { @@ -795,18 +760,14 @@ fn workspace_node_modules_key(ws_path: &[u8]) -> Vec { /// directories so scoped packages are handled. Missing directories are ignored. fn prune_node_modules_at(rel_path: &[u8], expected: Option<&StringHashMap<()>>) { let cwd = Fd::cwd(); - // `Dir` owns the fd (closes on drop) — no separate scopeguard, or we'd - // double-close. + // `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 directory listing before deleting anything. `delete_tree` - // mutates the parent directory, and the platform readdir iterators read in - // batches — deleting the current entry is fine, but a batch refill after a - // delete can re-surface entries on some filesystems. Iterating an owned - // list keeps the loop independent of the underlying directory shape. + // 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 { @@ -824,8 +785,7 @@ fn prune_node_modules_at(rel_path: &[u8], expected: Option<&StringHashMap<()>>) for name in &names { if name[0] == b'@' { - // Scoped package directory. Recurse one level; the expected set - // stores scoped packages as `@scope/pkg`. + // The expected set stores scoped packages as `@scope/pkg`. prune_scoped_node_modules(dir.fd(), name, expected); continue; } From aae6185758db74f4ef2dc80377cc74e96f78e375 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Thu, 13 Aug 2026 22:31:53 +0000 Subject: [PATCH 3/8] Close the scope dir handle before rmdirat, matching Dir::delete_tree --- src/install/hoisted_install.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/install/hoisted_install.rs b/src/install/hoisted_install.rs index 00460f51557b..1f0e21ed8185 100644 --- a/src/install/hoisted_install.rs +++ b/src/install/hoisted_install.rs @@ -842,6 +842,10 @@ fn prune_scoped_node_modules(parent_dir: Fd, scope: &[u8], expected: Option<&Str } } + // 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); From 735d5c51cedc115e590bb9793abb128d3c0bd639 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Fri, 14 Aug 2026 23:09:32 +0000 Subject: [PATCH 4/8] Skip the workspace prune on filtered installs --filter installs leave unselected workspaces' node_modules alone (bun prune handles those), matching the semantics the pnpm-parity change established. Adapt the collapsed-row test: an unfiltered install now prunes the stale nested copy immediately, so re-plant it before exercising the filtered update's removal. --- src/install/hoisted_install.rs | 18 +++++----- test/cli/install/bun-update.test.ts | 54 +++++++++++++++++++++++------ 2 files changed, 53 insertions(+), 19 deletions(-) diff --git a/src/install/hoisted_install.rs b/src/install/hoisted_install.rs index 1f0e21ed8185..3220a8423a8c 100644 --- a/src/install/hoisted_install.rs +++ b/src/install/hoisted_install.rs @@ -115,11 +115,14 @@ pub(crate) fn install_hoisted_packages( // block above, so no other borrow of `*mgr_ptr` is live here. let this = unsafe { &mut *mgr_ptr }; - // Must use the *unfiltered* `original_trees`: the filtered tree omits - // `--filter`-excluded workspaces, whose `node_modules` must not be wiped. - // Skipped on the security scanner's narrowed pre-install pass; the full - // install that follows performs the prune. - if packages_to_install.is_none() && this.lockfile.workspace_paths.count() > 0 { + // 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); } @@ -630,9 +633,8 @@ pub(crate) fn install_hoisted_packages( /// into `@scope/`) that the hoisted tree does not place there. Stale 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). `trees`/`tree_dep_ids` must be the unfiltered -/// buffers captured before `Lockfile::filter()` so `--filter`-excluded -/// workspaces keep the entries they own. +/// hoisted away (issue #29793). Only called for unfiltered installs, with the +/// full tree buffers. fn prune_stale_workspace_node_modules( lockfile: &crate::lockfile::Lockfile, trees: &[tree::Tree], diff --git a/test/cli/install/bun-update.test.ts b/test/cli/install/bun-update.test.ts index 8c44393d8498..3188f2bbc9ce 100644 --- a/test/cli/install/bun-update.test.ts +++ b/test/cli/install/bun-update.test.ts @@ -547,7 +547,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) => @@ -570,7 +570,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"], @@ -2978,9 +2984,9 @@ it("hoisted install preserves non-hoistable workspace-local packages", async () }); }); -// `bun install --filter ` must not destroy legitimate nested packages -// in a workspace that was excluded by the filter. The prune walks the -// unfiltered tree layout, so entries the full lockfile expects are kept. +// `bun install --filter ` leaves an excluded workspace's node_modules +// alone entirely (stale entries included); a later unfiltered install prunes +// the stale entry 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": {}, @@ -3038,8 +3044,7 @@ it("hoisted install with --filter preserves excluded workspace's non-hoistable p version: "0.0.5", }); - // Leave a stale directory in the workspace we'll exclude — the prune still - // cleans it up because it's not in the lockfile anywhere. + // Leave a stale directory in the workspace we'll exclude. await mkdir(join(package_dir, "packages", "excluded", "node_modules", "stale"), { recursive: true }); await writeFile( join(package_dir, "packages", "excluded", "node_modules", "stale", "package.json"), @@ -3059,14 +3064,35 @@ it("hoisted install with --filter preserves excluded workspace's non-hoistable p expect(await exited).toBe(0); } - // The excluded workspace's legit non-hoistable copy must still be there. + // 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", "stale"))).toBe(true); + + // A later unfiltered install prunes the stale entry 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", }); - // And the genuinely stale entry in that same directory is gone. expect(await exists(join(package_dir, "packages", "excluded", "node_modules", "stale"))).toBe(false); }); @@ -3097,7 +3123,7 @@ it("hoisted install prunes stale scoped workspace-local entries", async () => { }), ); - // Pre-existing stale scoped package. + // Pre-existing stale scoped and unscoped packages. await mkdir(join(package_dir, "packages", "backend", "node_modules", "@stale", "pkg"), { recursive: true, }); @@ -3105,6 +3131,11 @@ it("hoisted install prunes stale scoped workspace-local entries", async () => { join(package_dir, "packages", "backend", "node_modules", "@stale", "pkg", "package.json"), JSON.stringify({ name: "@stale/pkg", version: "0.0.0" }), ); + await mkdir(join(package_dir, "packages", "backend", "node_modules", "stale-plain"), { recursive: true }); + await writeFile( + join(package_dir, "packages", "backend", "node_modules", "stale-plain", "package.json"), + JSON.stringify({ name: "stale-plain", version: "0.0.0" }), + ); const { stderr, exited } = spawn({ cmd: [bunExe(), "install", "--linker=hoisted"], @@ -3116,7 +3147,8 @@ it("hoisted install prunes stale scoped workspace-local entries", async () => { expect(await new Response(stderr).text()).not.toContain("error:"); expect(await exited).toBe(0); - // The scoped package directory is gone, along with the empty `@stale` parent. + // Both stale entries are gone, along with the empty `@stale` parent. expect(await exists(join(package_dir, "packages", "backend", "node_modules", "@stale", "pkg"))).toBe(false); expect(await exists(join(package_dir, "packages", "backend", "node_modules", "@stale"))).toBe(false); + expect(await exists(join(package_dir, "packages", "backend", "node_modules", "stale-plain"))).toBe(false); }); From a18eb420888bffdc7c1b665b685e57798e93cfcd Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Fri, 14 Aug 2026 23:47:12 +0000 Subject: [PATCH 5/8] Prune only declared dependency names, leaving foreign entries alone A workspace entry is stale only when its name is a dependency the workspace declares and the tree resolves elsewhere; that is the shadowing case from #29793. Entries the lockfile never placed (a manual bun link, a hand-dropped folder) survive installs, matching the dedupe and add --filter tests that codify that policy. --- src/install/hoisted_install.rs | 78 ++++++++++++++++++----------- test/cli/install/bun-update.test.ts | 71 +++++++++++++++----------- 2 files changed, 91 insertions(+), 58 deletions(-) diff --git a/src/install/hoisted_install.rs b/src/install/hoisted_install.rs index 3220a8423a8c..0c3436f58666 100644 --- a/src/install/hoisted_install.rs +++ b/src/install/hoisted_install.rs @@ -629,12 +629,14 @@ pub(crate) fn install_hoisted_packages( Ok(summary) } -/// Deletes top-level entries of each workspace's `node_modules/` (one level -/// into `@scope/`) that the hoisted tree does not place there. Stale 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). Only called for unfiltered installs, with the -/// full tree buffers. +/// 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], @@ -649,9 +651,10 @@ fn prune_stale_workspace_node_modules( 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) would otherwise wipe the dir with an empty expected set. + // 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(); @@ -727,17 +730,37 @@ fn prune_stale_workspace_node_modules( } } - // Walk only the workspaces we resolved above. - for (_pkg_id, fs_path) in &workspaces_to_walk { + // 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 expected = expected_by_ws_path.get(key.as_slice()); - prune_node_modules_at(&key, expected); + 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 expected-set map -/// and the walk so the two can't diverge (a miss routes through the -/// `expected == None` branch and deletes legitimate entries). +/// 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], @@ -758,9 +781,9 @@ fn workspace_node_modules_key(ws_path: &[u8]) -> Vec { } /// Opens `/` and removes each top-level directory entry whose -/// name is not present in `expected`. Also descends one level into `@scope/` -/// directories so scoped packages are handled. Missing directories are ignored. -fn prune_node_modules_at(rel_path: &[u8], expected: Option<&StringHashMap<()>>) { +/// 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) { @@ -787,20 +810,17 @@ fn prune_node_modules_at(rel_path: &[u8], expected: Option<&StringHashMap<()>>) for name in &names { if name[0] == b'@' { - // The expected set stores scoped packages as `@scope/pkg`. - prune_scoped_node_modules(dir.fd(), name, expected); + // The prunable set stores scoped packages as `@scope/pkg`. + prune_scoped_node_modules(dir.fd(), name, prunable); continue; } - if let Some(exp) = expected { - if exp.contains_key(name.as_slice()) { - continue; - } + if prunable.contains_key(name.as_slice()) { + let _ = dir.delete_tree(name); } - let _ = dir.delete_tree(name); } } -fn prune_scoped_node_modules(parent_dir: Fd, scope: &[u8], expected: Option<&StringHashMap<()>>) { +fn prune_scoped_node_modules(parent_dir: Fd, scope: &[u8], prunable: &StringHashMap<()>) { let scope_dir = match sys::open_dir_for_iteration(parent_dir, scope) { Ok(fd) => Dir::from_fd(fd), Err(_) => return, @@ -832,11 +852,9 @@ fn prune_scoped_node_modules(parent_dir: Fd, scope: &[u8], expected: Option<&Str full_name.push(b'/'); full_name.extend_from_slice(name); - if let Some(exp) = expected { - if exp.contains_key(full_name.as_slice()) { - has_remaining = true; - continue; - } + if !prunable.contains_key(full_name.as_slice()) { + has_remaining = true; + continue; } if scope_dir.delete_tree(name).is_err() { diff --git a/test/cli/install/bun-update.test.ts b/test/cli/install/bun-update.test.ts index 3188f2bbc9ce..38e0ea0c9238 100644 --- a/test/cli/install/bun-update.test.ts +++ b/test/cli/install/bun-update.test.ts @@ -2986,18 +2986,20 @@ it("hoisted install preserves non-hoistable workspace-local packages", async () // `bun install --filter ` leaves an excluded workspace's node_modules // alone entirely (stale entries included); a later unfiltered install prunes -// the stale entry while keeping the legit non-hoistable copy. +// 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`. + // `packages/excluded/node_modules/baz`. The workspace's moo has no root + // conflict and hoists. await writeFile( join(package_dir, "package.json"), JSON.stringify({ @@ -3010,7 +3012,7 @@ it("hoisted install with --filter preserves excluded workspace's non-hoistable p 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" } }), + JSON.stringify({ name: "excluded", dependencies: { baz: "0.0.5", moo: "^0.1.0" } }), ); await mkdir(join(package_dir, "packages", "target"), { recursive: true }); await writeFile( @@ -3032,11 +3034,12 @@ it("hoisted install with --filter preserves excluded workspace's non-hoistable p } // Confirm the expected layout before the filtered install runs: root gets - // 0.0.3, the excluded workspace has its own 0.0.5. + // 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({ @@ -3044,11 +3047,11 @@ it("hoisted install with --filter preserves excluded workspace's non-hoistable p version: "0.0.5", }); - // Leave a stale directory in the workspace we'll exclude. - await mkdir(join(package_dir, "packages", "excluded", "node_modules", "stale"), { recursive: true }); + // 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", "stale", "package.json"), - JSON.stringify({ name: "stale", version: "0.0.0" }), + 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. @@ -3072,10 +3075,10 @@ it("hoisted install with --filter preserves excluded workspace's non-hoistable p name: "baz", version: "0.0.5", }); - expect(await exists(join(package_dir, "packages", "excluded", "node_modules", "stale"))).toBe(true); + expect(await exists(join(package_dir, "packages", "excluded", "node_modules", "moo"))).toBe(true); - // A later unfiltered install prunes the stale entry but keeps the legit - // non-hoistable copy. + // 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"], @@ -3093,15 +3096,18 @@ it("hoisted install with --filter preserves excluded workspace's non-hoistable p name: "baz", version: "0.0.5", }); - expect(await exists(join(package_dir, "packages", "excluded", "node_modules", "stale"))).toBe(false); + expect(await exists(join(package_dir, "packages", "excluded", "node_modules", "moo"))).toBe(false); }); -// Stale scoped packages (`@scope/pkg`) must also be pruned, and the empty -// `@scope` directory shouldn't be left behind. +// 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)); @@ -3115,26 +3121,30 @@ it("hoisted install prunes stale scoped workspace-local entries", async () => { }), ); 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" }, + dependencies: { baz: "^0.0.3", "@scope/dep": "npm:moo@^0.1.0" }, }), ); - // Pre-existing stale scoped and unscoped packages. - await mkdir(join(package_dir, "packages", "backend", "node_modules", "@stale", "pkg"), { - recursive: true, - }); + // 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(package_dir, "packages", "backend", "node_modules", "@stale", "pkg", "package.json"), - JSON.stringify({ name: "@stale/pkg", version: "0.0.0" }), + join(backendModules, "@scope", "dep", "package.json"), + JSON.stringify({ name: "moo", version: "0.0.0" }), ); - await mkdir(join(package_dir, "packages", "backend", "node_modules", "stale-plain"), { recursive: true }); + await mkdir(join(backendModules, "hand-linked"), { recursive: true }); await writeFile( - join(package_dir, "packages", "backend", "node_modules", "stale-plain", "package.json"), - JSON.stringify({ name: "stale-plain", version: "0.0.0" }), + join(backendModules, "hand-linked", "package.json"), + JSON.stringify({ name: "hand-linked", version: "0.0.0" }), ); const { stderr, exited } = spawn({ @@ -3147,8 +3157,13 @@ it("hoisted install prunes stale scoped workspace-local entries", async () => { expect(await new Response(stderr).text()).not.toContain("error:"); expect(await exited).toBe(0); - // Both stale entries are gone, along with the empty `@stale` parent. - expect(await exists(join(package_dir, "packages", "backend", "node_modules", "@stale", "pkg"))).toBe(false); - expect(await exists(join(package_dir, "packages", "backend", "node_modules", "@stale"))).toBe(false); - expect(await exists(join(package_dir, "packages", "backend", "node_modules", "stale-plain"))).toBe(false); + // 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); }); From b2547e6d10cdcd0bed1e65876572070f7e56b94c Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Sat, 15 Aug 2026 00:26:28 +0000 Subject: [PATCH 6/8] Drop the setDefaultTimeout comment, matching sibling install tests --- test/cli/install/bun-update.test.ts | 4 ---- 1 file changed, 4 deletions(-) diff --git a/test/cli/install/bun-update.test.ts b/test/cli/install/bun-update.test.ts index 38e0ea0c9238..c69eebe436f2 100644 --- a/test/cli/install/bun-update.test.ts +++ b/test/cli/install/bun-update.test.ts @@ -15,10 +15,6 @@ import { setHandler, } from "./dummy.registry.js"; -// ASAN + slow CI filesystems can push individual install spawns past the -// default 5s timeout. `setDefaultTimeout` is read at `it()` call time, so it -// must run during module load (before the `it()` declarations below), not -// inside `beforeAll`. Match the convention other install test files use. setDefaultTimeout(1000 * 60 * 5); beforeAll(dummyBeforeAll); afterAll(dummyAfterAll); From 77f1a979fd0e5aa5dbaef84ce6d8ce190f1d9ab5 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Sat, 15 Aug 2026 00:57:10 +0000 Subject: [PATCH 7/8] Refuse symlinked @scope entries in the workspace prune Reuse prune.rs's open_real_subdir (lstat + O_NOFOLLOW) so the scoped descent cannot delete through a symlink outside the workspace's node_modules, matching the guard bun prune uses for the same walk. --- src/install/hoisted_install.rs | 13 +++++++------ src/install/prune.rs | 2 +- 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/src/install/hoisted_install.rs b/src/install/hoisted_install.rs index 0c3436f58666..cf933ac3ce3f 100644 --- a/src/install/hoisted_install.rs +++ b/src/install/hoisted_install.rs @@ -811,7 +811,7 @@ fn prune_node_modules_at(rel_path: &[u8], prunable: &StringHashMap<()>) { for name in &names { if name[0] == b'@' { // The prunable set stores scoped packages as `@scope/pkg`. - prune_scoped_node_modules(dir.fd(), name, prunable); + prune_scoped_node_modules(&dir, name, prunable); continue; } if prunable.contains_key(name.as_slice()) { @@ -820,10 +820,11 @@ fn prune_node_modules_at(rel_path: &[u8], prunable: &StringHashMap<()>) { } } -fn prune_scoped_node_modules(parent_dir: Fd, scope: &[u8], prunable: &StringHashMap<()>) { - let scope_dir = match sys::open_dir_for_iteration(parent_dir, scope) { - Ok(fd) => Dir::from_fd(fd), - Err(_) => return, +fn prune_scoped_node_modules(parent: &Dir, scope: &[u8], prunable: &StringHashMap<()>) { + // `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; }; let mut names: Vec> = Vec::new(); @@ -872,7 +873,7 @@ fn prune_scoped_node_modules(parent_dir: Fd, scope: &[u8], prunable: &StringHash 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_dir, z); + let _ = sys::rmdirat(parent.fd(), z); } } diff --git a/src/install/prune.rs b/src/install/prune.rs index cffaa8014ccc..692ad0c141d2 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; } From f693a65b22e8e8c25d1ff5fa21e012a5855932ac Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Sat, 15 Aug 2026 01:33:20 +0000 Subject: [PATCH 8/8] Sweep dangling .bin links after pruning a workspace package A previously nested dep with bins leaves links in the workspace's node_modules/.bin when the prune removes it; reuse prune_bins (now pub(crate)) the way remove_collapsed_copies' housekeeping does. --- src/install/hoisted_install.rs | 25 +++++++++++++++++++------ src/install/prune.rs | 4 ++-- 2 files changed, 21 insertions(+), 8 deletions(-) diff --git a/src/install/hoisted_install.rs b/src/install/hoisted_install.rs index cf933ac3ce3f..46f040ec2462 100644 --- a/src/install/hoisted_install.rs +++ b/src/install/hoisted_install.rs @@ -808,23 +808,31 @@ fn prune_node_modules_at(rel_path: &[u8], prunable: &StringHashMap<()>) { 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`. - prune_scoped_node_modules(&dir, name, prunable); + deleted_any |= prune_scoped_node_modules(&dir, name, prunable); continue; } - if prunable.contains_key(name.as_slice()) { - let _ = dir.delete_tree(name); + 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); + } } -fn prune_scoped_node_modules(parent: &Dir, scope: &[u8], prunable: &StringHashMap<()>) { +/// 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; + return false; }; let mut names: Vec> = Vec::new(); @@ -834,7 +842,7 @@ fn prune_scoped_node_modules(parent: &Dir, scope: &[u8], prunable: &StringHashMa let entry = match iter.next() { Ok(Some(e)) => e, Ok(None) => break, - Err(_) => return, + Err(_) => return false, }; let name = entry.name.slice_u8(); if name.is_empty() { @@ -847,6 +855,7 @@ fn prune_scoped_node_modules(parent: &Dir, scope: &[u8], prunable: &StringHashMa 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); @@ -860,6 +869,8 @@ fn prune_scoped_node_modules(parent: &Dir, scope: &[u8], prunable: &StringHashMa if scope_dir.delete_tree(name).is_err() { has_remaining = true; + } else { + deleted_any = true; } } @@ -875,6 +886,8 @@ fn prune_scoped_node_modules(parent: &Dir, scope: &[u8], prunable: &StringHashMa 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 692ad0c141d2..dcac7ce0a96f 100644 --- a/src/install/prune.rs +++ b/src/install/prune.rs @@ -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; };