install: remove the node_modules links of a renamed or removed workspace

src/install/PackageManager.rs

@@ -78,6 +78,8 @@ pub mod populate_manifest_cache;
 pub mod process_dependency_list;
 #[path = "PackageManager/ProgressStrings.rs"]
 pub mod progress_strings;
+#[path = "PackageManager/remove_stale_workspace_links.rs"]
+pub(crate) mod remove_stale_workspace_links;
 #[path = "PackageManager/runTasks.rs"]
 pub mod run_tasks;
 #[path = "PackageManager/security_scanner.rs"]

src/install/PackageManager/install_with_manager.rs

@@ -38,6 +38,7 @@ use bun_install_types::NodeLinker::NodeLinker;
 
 // Free-function "methods" on `PackageManager` hosted in sibling modules
 // to avoid one giant `impl PackageManager` block.
+use crate::package_manager_real::remove_stale_workspace_links::remove_stale_workspace_links;
 use crate::package_manager_real::run_tasks::{RunTasksCallbacks, run_tasks};
 use crate::package_manager_real::{
     UpdateRequest, enqueue_dependency_list, enqueue_dependency_with_main, enqueue_patch_task_pre,
@@ -878,6 +879,11 @@ pub fn install_with_manager(
             break 'install_summary PackageInstallSummary::default();
         }
 
+        // Every workspace is a root dependency, so without a diff the workspace set is unchanged.
+        if had_any_diffs {
+            remove_stale_workspace_links(&lockfile_before_clean, &manager.lockfile);
+        }
+
         let mut linker = manager.options.node_linker;
         loop {
             match linker {

src/install/PackageManager/remove_stale_workspace_links.rs

@@ -0,0 +1,80 @@
+use bstr::BStr;
+use bun_paths::AutoAbsPathChecked;
+
+use crate::lockfile::Lockfile;
+use crate::lockfile::package::PackageColumns as _;
+use crate::package_installer::alias_is_safe_install_target;
+use crate::package_manager_real::PackageManager;
+use crate::resolution::Tag as ResolutionTag;
+
+/// Linkers only visit names in the new lockfile; a workspace name that left it is unlinked here.
+pub(crate) fn remove_stale_workspace_links(previous: &Lockfile, current: &Lockfile) {
+    let previous_string_buf = previous.buffers.string_bytes.as_slice();
+    let previous_packages = previous.packages.slice();
+    let resolutions = previous_packages.items_resolution();
+    let name_hashes = previous_packages.items_name_hash();
+    let names = previous_packages.items_name();
+
+    for pkg_id in 0..previous_packages.len() {
+        if resolutions[pkg_id].tag != ResolutionTag::Workspace
+            || current.workspace_paths.contains_key(&name_hashes[pkg_id])
+        {
+            continue;
+        }
+
+        let name = names[pkg_id].slice(previous_string_buf);
+        if !alias_is_safe_install_target(name) {
+            continue;
+        }
+
+        remove_links_named(current, name);
+    }
+}
+
+#[cold]
+#[inline(never)]
+fn remove_links_named(current: &Lockfile, name: &[u8]) {
+    let current_string_buf = current.buffers.string_bytes.as_slice();
+
+    let mut path = AutoAbsPathChecked::init_top_level_dir();
+    let top_level_len = path.len();
+
+    remove_link(&mut path, b"", name);
+    for workspace_path in current.workspace_paths.values() {
+        path.set_length(top_level_len);
+        remove_link(&mut path, workspace_path.slice(current_string_buf), name);
+    }
+}
+
+/// `readlink` is the symlink (or junction) check; a real directory or file there is left alone.
+fn remove_link(path: &mut AutoAbsPathChecked, package_dir: &[u8], name: &[u8]) {
+    if path.append(package_dir).is_err()
+        || path.append(b"node_modules").is_err()
+        || path.append(name).is_err()
+    {
+        return;
+    }
+
+    let mut link_target = bun_paths::path_buffer_pool::get();
+    if bun_sys::readlink(path.slice_z(), &mut link_target).is_err() {
+        return;
+    }
+
+    bun_output::scoped_log!(
+        PackageManager,
+        "removing stale workspace link {}",
+        BStr::new(path.slice())
+    );
+
+    #[cfg(windows)]
+    {
+        // Directory symlinks and junctions need rmdir; a file symlink needs unlink.
+        if bun_sys::rmdir(path.slice_z()).is_err() {
+            let _ = bun_sys::unlink(path.slice_z());
+        }
+    }
+    #[cfg(not(windows))]
+    {
+        let _ = bun_sys::unlink(path.slice_z());
+    }
+}

test/cli/install/bun-workspaces.test.ts

@@ -2629,3 +2629,179 @@ describe("packages whose version label is longer than 512 bytes", () => {
     );
   });
 });
+
+// Both linkers only create or repoint the `node_modules/<name>` links the current
+// lockfile asks for. When a workspace is renamed or removed, its name leaves the
+// lockfile and the links bun had created under that name (in the root's node_modules
+// and, with the isolated linker, in the node_modules of the workspaces depending on
+// it) used to stay behind, still resolving to the workspace folder.
+describe("links of a renamed or removed workspace", () => {
+  type Linker = "hoisted" | "isolated";
+  const linkers: Linker[] = ["hoisted", "isolated"];
+
+  async function install(ctx: TestCtx, linker: Linker, ...args: string[]): Promise<string> {
+    await using proc = spawn({
+      cmd: [bunExe(), "install", "--linker", linker, ...args],
+      cwd: ctx.packageDir,
+      stdout: "pipe",
+      stderr: "pipe",
+      env: ctx.env,
+    });
+    const [out, err, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
+    expect(err).not.toContain("error:");
+    expect(exitCode).toBe(0);
+    return out;
+  }
+
+  function writeProject(
+    packageDir: string,
+    rootDependencies: Record<string, string>,
+    packages: Record<string, Record<string, unknown>>,
+  ) {
+    return Promise.all([
+      write(
+        join(packageDir, "package.json"),
+        JSON.stringify({ name: "foo", workspaces: ["packages/*"], devDependencies: rootDependencies }),
+      ),
+      ...Object.entries(packages).map(([dir, packageJson]) =>
+        write(join(packageDir, "packages", dir, "package.json"), JSON.stringify({ version: "1.0.0", ...packageJson })),
+      ),
+    ]);
+  }
+
+  // Package entries of a node_modules directory (no `.bun`/`.bin`), or [] when it does
+  // not exist. A listing (rather than `exists`) also sees links whose target is gone.
+  async function entries(nodeModules: string): Promise<string[]> {
+    try {
+      return (await readdirSorted(nodeModules)).filter(name => !name.startsWith("."));
+    } catch (err: any) {
+      if (err.code !== "ENOENT") throw err;
+      return [];
+    }
+  }
+
+  for (const linker of linkers) {
+    test.concurrent(`${linker}: renaming a workspace removes the links under its old name`, async () => {
+      using ctx = await setupTest();
+      const { packageDir } = ctx;
+      const rootNodeModules = join(packageDir, "node_modules");
+      const bNodeModules = join(packageDir, "packages", "b", "node_modules");
+
+      await writeProject(
+        packageDir,
+        { a: "*", b: "*" },
+        { a: { name: "a" }, b: { name: "b", dependencies: { a: "*" } } },
+      );
+      await install(ctx, linker);
+      expect(await entries(rootNodeModules)).toEqual(["a", "b"]);
+      expect(await entries(bNodeModules)).toEqual(linker === "isolated" ? ["a"] : []);
+
+      await writeProject(
+        packageDir,
+        { "a-renamed": "*", b: "*" },
+        { a: { name: "a-renamed" }, b: { name: "b", dependencies: { "a-renamed": "*" } } },
+      );
+      await install(ctx, linker);
+      expect(await entries(rootNodeModules)).toEqual(["a-renamed", "b"]);
+      expect(await entries(bNodeModules)).toEqual(linker === "isolated" ? ["a-renamed"] : []);
+      expect(await file(join(rootNodeModules, "a-renamed", "package.json")).json()).toMatchObject({
+        name: "a-renamed",
+      });
+
+      expect(await install(ctx, linker)).toContain("(no changes)");
+      expect(await entries(rootNodeModules)).toEqual(["a-renamed", "b"]);
+      expect(await entries(bNodeModules)).toEqual(linker === "isolated" ? ["a-renamed"] : []);
+    });
+
+    test.concurrent(`${linker}: renaming a scoped workspace removes the link under its old name`, async () => {
+      using ctx = await setupTest();
+      const { packageDir } = ctx;
+      const scopeDir = join(packageDir, "node_modules", "@repo");
+
+      await writeProject(
+        packageDir,
+        { "@repo/a": "*", "@repo/b": "*" },
+        { a: { name: "@repo/a" }, b: { name: "@repo/b" } },
+      );
+      await install(ctx, linker);
+      expect(await entries(scopeDir)).toEqual(["a", "b"]);
+
+      await writeProject(
+        packageDir,
+        { "@repo/a-renamed": "*", "@repo/b": "*" },
+        { a: { name: "@repo/a-renamed" }, b: { name: "@repo/b" } },
+      );
+      await install(ctx, linker);
+      expect(await entries(scopeDir)).toEqual(["a-renamed", "b"]);
+      expect(await file(join(scopeDir, "a-renamed", "package.json")).json()).toMatchObject({ name: "@repo/a-renamed" });
+    });
+
+    test.concurrent(`${linker}: removing a workspace from the project removes its link`, async () => {
+      using ctx = await setupTest();
+      const { packageDir } = ctx;
+      const rootNodeModules = join(packageDir, "node_modules");
+
+      await writeProject(packageDir, { a: "*", b: "*" }, { a: { name: "a" }, b: { name: "b" } });
+      await install(ctx, linker);
+      expect(await entries(rootNodeModules)).toEqual(["a", "b"]);
+
+      await rm(join(packageDir, "packages", "a"), { recursive: true });
+      await writeProject(packageDir, { b: "*" }, { b: { name: "b" } });
+      await install(ctx, linker);
+      expect(await entries(rootNodeModules)).toEqual(["b"]);
+    });
+
+    // The old name is unlinked before the linker runs, so a dependency that still
+    // wants that name (here an alias of the renamed workspace) is linked again.
+    test.concurrent(`${linker}: the old name is linked again when a dependency still uses it`, async () => {
+      using ctx = await setupTest();
+      const { packageDir } = ctx;
+      const rootNodeModules = join(packageDir, "node_modules");
+
+      await writeProject(packageDir, { a: "*" }, { a: { name: "a" } });
+      await install(ctx, linker);
+      expect(await entries(rootNodeModules)).toEqual(["a"]);
+
+      await writeProject(packageDir, { a: "workspace:a-renamed@*" }, { a: { name: "a-renamed" } });
+      await install(ctx, linker);
+      expect(await entries(rootNodeModules)).toEqual(linker === "isolated" ? ["a"] : ["a", "a-renamed"]);
+      expect(await file(join(rootNodeModules, "a", "package.json")).json()).toMatchObject({ name: "a-renamed" });
+    });
+  }
+
+  // The isolated linker only links the workspaces the root depends on, so nothing is
+  // ever linked for `a` here and the directory under its name is not bun's to remove.
+  test.concurrent("a directory under the old name is left alone", async () => {
+    using ctx = await setupTest();
+    const { packageDir } = ctx;
+    const rootNodeModules = join(packageDir, "node_modules");
+    const marker = join(rootNodeModules, "a", "marker.txt");
+
+    await writeProject(packageDir, { b: "*" }, { a: { name: "a" }, b: { name: "b" } });
+    await install(ctx, "isolated");
+    expect(await entries(rootNodeModules)).toEqual(["b"]);
+
+    await write(marker, "not a workspace link");
+    await writeProject(packageDir, { b: "*" }, { a: { name: "a-renamed" }, b: { name: "b" } });
+    await install(ctx, "isolated");
+    expect(await entries(rootNodeModules)).toEqual(["a", "b"]);
+    expect(await file(marker).text()).toBe("not a workspace link");
+  });
+
+  test.concurrent("--dry-run does not touch the links", async () => {
+    using ctx = await setupTest();
+    const { packageDir } = ctx;
+    const rootNodeModules = join(packageDir, "node_modules");
+
+    await writeProject(packageDir, { a: "*", b: "*" }, { a: { name: "a" }, b: { name: "b" } });
+    await install(ctx, "hoisted");
+    expect(await entries(rootNodeModules)).toEqual(["a", "b"]);
+
+    await writeProject(packageDir, { "a-renamed": "*", b: "*" }, { a: { name: "a-renamed" }, b: { name: "b" } });
+    await install(ctx, "hoisted", "--dry-run");
+    expect(await entries(rootNodeModules)).toEqual(["a", "b"]);
+
+    await install(ctx, "hoisted");
+    expect(await entries(rootNodeModules)).toEqual(["a-renamed", "b"]);
+  });
+});