install: fix isolated-linker deadlock installing patched git/github dependencies in workspaces

src/install/isolated_install.rs

@@ -32,7 +32,7 @@ use bun_collections::{
     ArrayHashMap, DynamicBitSet, DynamicBitSetList, DynamicBitSetUnmanaged, HashMap, LinearFifo,
     StringArrayHashMap,
 };
-use bun_core::{Environment, Global, Output, fast_random, fmt as bun_fmt};
+use bun_core::{Environment, Global, Output, fast_random, fmt as bun_fmt, strings};
 use bun_paths::path_options::AssumeOk as _;
 use bun_paths::{self as paths, AutoAbsPath as AbsPath, AutoRelPath, PathBuffer};
 use bun_semver as semver;
@@ -2356,37 +2356,49 @@ pub(crate) fn install_isolated_packages(
                     {
                         install::PreinstallState::Done => false,
                         _ => 'missing_from_cache: {
-                            if matches!(patch_info, installer::PatchInfo::None) {
-                                let exists = match pkg_res_tag {
-                                    ResolutionTag::Npm => {
-                                        // Reshaped for borrowck — capture length
-                                        // instead of `save()` so the path stays unborrowed.
-                                        let cache_dir_path_save = pkg_cache_dir_subpath.len();
-                                        pkg_cache_dir_subpath.append(b"package.json").assume_ok();
-                                        let exists = sys::exists_at(
-                                            cache_dir,
-                                            pkg_cache_dir_subpath.slice_z(),
-                                        );
-                                        pkg_cache_dir_subpath.set_length(cache_dir_path_save);
-                                        exists
-                                    }
-                                    _ => sys::directory_exists_at(
-                                        cache_dir,
-                                        pkg_cache_dir_subpath.slice_z(),
+                            // Downloads only produce the unpatched folder;
+                            // re-enqueueing one the resolve phase already
+                            // extracted deadlocks the install (#37136).
+                            if matches!(patch_info, installer::PatchInfo::Patch(_)) {
+                                let idx = strings::last_index_of(
+                                    pkg_cache_dir_subpath.slice(),
+                                    b"_patch_hash=",
+                                )
+                                .unwrap_or_else(|| {
+                                    panic!(
+                                        "Patched dependency cache dir subpath does not have the \
+                                         \"_patch_hash=HASH\" suffix. This is a bug, please file \
+                                         a GitHub issue."
                                     )
-                                    .unwrap_or(false),
-                                };
-                                if exists {
-                                    installer.manager_mut().set_preinstall_state(
-                                        pkg_id,
-                                        install::PreinstallState::Done,
-                                    );
+                                });
+                                pkg_cache_dir_subpath.set_length(idx);
+                            }
+                            let exists = match pkg_res_tag {
+                                // `Remove`'s subpath is already unpatched.
+                                ResolutionTag::Npm
+                                    if !matches!(patch_info, installer::PatchInfo::Patch(_)) =>
+                                {
+                                    // Reshaped for borrowck — capture length
+                                    // instead of `save()` so the path stays unborrowed.
+                                    let cache_dir_path_save = pkg_cache_dir_subpath.len();
+                                    pkg_cache_dir_subpath.append(b"package.json").assume_ok();
+                                    let exists =
+                                        sys::exists_at(cache_dir, pkg_cache_dir_subpath.slice_z());
+                                    pkg_cache_dir_subpath.set_length(cache_dir_path_save);
+                                    exists
                                 }
-                                break 'missing_from_cache !exists;
+                                _ => sys::directory_exists_at(
+                                    cache_dir,
+                                    pkg_cache_dir_subpath.slice_z(),
+                                )
+                                .unwrap_or(false),
+                            };
+                            if exists {
+                                installer
+                                    .manager_mut()
+                                    .set_preinstall_state(pkg_id, install::PreinstallState::Done);
                             }
-
-                            // TODO: why does this look like it will never work?
-                            break 'missing_from_cache true;
+                            break 'missing_from_cache !exists;
                         }
                     };
 

test/cli/install/isolated-install.test.ts

@@ -724,6 +724,268 @@ index 0000000000000000000000000000000000000000..3b18e512dba79e4c8300dd08aeb37f8e
   await checkInstall();
 });
 
+// Adding a patchedDependencies entry for a github: dependency of a workspace
+// member deadlocked `bun install` forever with the isolated linker:
+// re-resolution re-downloaded the github tarball, and the install phase then
+// re-enqueued the same tarball task and parked the store entry on the
+// completed task's already-drained callback list, so the pending-task count
+// never reached zero.
+test("adding and removing a patch for a github dependency in a workspace completes", async () => {
+  const { packageJson, packageDir } = await registry.createTestDir({ bunfigOpts: { linker: "isolated" } });
+
+  // Minimal gzipped tarball shaped like a github codeload tarball: a single
+  // root directory wrapping the package contents.
+  function tarHeader(name: string, size: number, isDir: boolean): Uint8Array {
+    const header = new Uint8Array(512);
+    const encoder = new TextEncoder();
+    header.set(encoder.encode(name), 0);
+    header.set(encoder.encode(isDir ? "0000755 " : "0000644 "), 100);
+    header.set(encoder.encode("0000000 "), 108);
+    header.set(encoder.encode("0000000 "), 116);
+    header.set(encoder.encode(size.toString(8).padStart(11, "0") + " "), 124);
+    header.set(encoder.encode("00000000000 "), 136);
+    header.set(encoder.encode("        "), 148);
+    header[156] = (isDir ? "5" : "0").charCodeAt(0);
+    header.set(encoder.encode("ustar"), 257);
+    header.set(encoder.encode("00"), 263);
+    let checksum = 0;
+    for (const byte of header) checksum += byte;
+    header.set(encoder.encode(checksum.toString(8).padStart(6, "0") + "\0 "), 148);
+    return header;
+  }
+  const blocks: Uint8Array[] = [];
+  blocks.push(tarHeader("testowner-testrepo-aaaaaaa/", 0, true));
+  for (const [name, contents] of [
+    ["package.json", JSON.stringify({ name: "gh-dep", version: "1.0.0" })],
+    ["index.js", 'console.log("original");\n'],
+  ]) {
+    const bytes = new TextEncoder().encode(contents);
+    blocks.push(tarHeader(`testowner-testrepo-aaaaaaa/${name}`, bytes.length, false));
+    blocks.push(bytes);
+    if (bytes.length % 512 !== 0) blocks.push(new Uint8Array(512 - (bytes.length % 512)));
+  }
+  blocks.push(new Uint8Array(1024));
+  const tarball = Bun.gzipSync(Buffer.concat(blocks));
+
+  using server = Bun.serve({
+    port: 0,
+    fetch: () => new Response(tarball, { headers: { "Content-Type": "application/gzip" } }),
+  });
+
+  const env = {
+    ...bunEnv,
+    GITHUB_API_URL: `http://localhost:${server.port}`,
+    // CI exports BUN_INSTALL_CACHE_DIR; pin it so this test's cache state is
+    // its own.
+    BUN_INSTALL_CACHE_DIR: join(packageDir, ".bun-cache"),
+  };
+
+  async function install() {
+    await using proc = spawn({
+      cmd: [bunExe(), "install"],
+      cwd: packageDir,
+      env,
+      stdout: "pipe",
+      stderr: "pipe",
+    });
+    const [err, exitCode] = await Promise.all([proc.stderr.text(), proc.exited, proc.stdout.text()]);
+    expect(err).not.toContain("error:");
+    expect(exitCode).toBe(0);
+  }
+
+  const rootPackageJson = {
+    name: "patched-github-workspace",
+    workspaces: ["packages/*"],
+  };
+  await write(packageJson, JSON.stringify(rootPackageJson));
+  await write(
+    join(packageDir, "packages", "member", "package.json"),
+    JSON.stringify({
+      name: "member",
+      version: "1.0.0",
+      dependencies: {
+        "gh-dep": "github:testowner/testrepo#aaaaaaa",
+      },
+    }),
+  );
+  await write(
+    join(packageDir, "patches", "gh-dep.patch"),
+    `diff --git a/index.js b/index.js
+index 1f0e8b9f1f9a56799cdbc1a5a2f8cf9f9a3b2f1c..2f0e8b9f1f9a56799cdbc1a5a2f8cf9f9a3b2f1d 100644
+--- a/index.js
++++ b/index.js
+@@ -1 +1 @@
+-console.log("original");
++console.log("patched");
+`,
+  );
+
+  const installedIndexJs = file(join(packageDir, "packages", "member", "node_modules", "gh-dep", "index.js"));
+
+  await install();
+  expect(await installedIndexJs.text()).toBe('console.log("original");\n');
+
+  // Adding the patch triggers a re-resolution; this install hung forever
+  // before the fix.
+  await write(
+    packageJson,
+    JSON.stringify({
+      ...rootPackageJson,
+      patchedDependencies: {
+        "gh-dep@github:testowner/testrepo#aaaaaaa": "patches/gh-dep.patch",
+      },
+    }),
+  );
+  await install();
+  expect(await installedIndexJs.text()).toBe('console.log("patched");\n');
+
+  // Cold cache with the patch still in the lockfile: the install phase itself
+  // downloads the tarball and applies the patch after extraction.
+  await rm(join(packageDir, ".bun-cache"), { recursive: true, force: true });
+  await rm(join(packageDir, "node_modules"), { recursive: true, force: true });
+  await rm(join(packageDir, "packages", "member", "node_modules"), { recursive: true, force: true });
+  await install();
+  expect(await installedIndexJs.text()).toBe('console.log("patched");\n');
+
+  // Removing the patch re-resolves again and rebuilds the store entry from
+  // the unpatched cache folder (the PatchInfo::Remove path, which hung the
+  // same way).
+  await write(packageJson, JSON.stringify(rootPackageJson));
+  await install();
+  expect(await installedIndexJs.text()).toBe('console.log("original");\n');
+});
+
+// Same deadlock through the git: task-id space (clone + checkout tasks
+// instead of a tarball download). The repo is served over git's dumb HTTP
+// protocol: after `git update-server-info`, a bare repo is plain static
+// files. Requires the git executable to build the fixture repository.
+const gitExecutable = Bun.which("git");
+test.skipIf(!gitExecutable)("adding and removing a patch for a git dependency in a workspace completes", async () => {
+  const { packageJson, packageDir } = await registry.createTestDir({ bunfigOpts: { linker: "isolated" } });
+
+  const srcDir = join(packageDir, "git-src");
+  const bareDir = join(packageDir, "repo.git");
+  // Isolate git from system/global config (e.g. core.autocrlf on Windows
+  // would rewrite the checked-out file contents this test asserts on).
+  const gitConfigEnv = {
+    GIT_CONFIG_NOSYSTEM: "1",
+    GIT_CONFIG_GLOBAL: join(packageDir, "gitconfig"),
+  };
+  const gitEnv = {
+    ...bunEnv,
+    ...gitConfigEnv,
+    GIT_AUTHOR_NAME: "bun-test",
+    GIT_AUTHOR_EMAIL: "test@bun.sh",
+    GIT_COMMITTER_NAME: "bun-test",
+    GIT_COMMITTER_EMAIL: "test@bun.sh",
+  };
+  async function git(args: string[], cwd: string): Promise<string> {
+    await using proc = spawn({ cmd: [gitExecutable!, ...args], cwd, env: gitEnv, stdout: "pipe", stderr: "pipe" });
+    const [out, err, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
+    expect(err).not.toContain("fatal:");
+    expect(exitCode).toBe(0);
+    return out;
+  }
+
+  await write(join(packageDir, "gitconfig"), "[core]\n\tautocrlf = false\n");
+  await write(join(srcDir, "package.json"), JSON.stringify({ name: "git-dep", version: "1.0.0" }));
+  await write(join(srcDir, "index.js"), 'console.log("original");\n');
+  await git(["init", "-q"], srcDir);
+  await git(["add", "-A"], srcDir);
+  await git(["commit", "-qm", "init"], srcDir);
+  const sha = (await git(["rev-parse", "HEAD"], srcDir)).trim();
+  await git(["clone", "-q", "--bare", srcDir, bareDir], packageDir);
+  await git(["update-server-info"], bareDir);
+
+  using server = Bun.serve({
+    port: 0,
+    async fetch(req) {
+      const { pathname } = new URL(req.url);
+      if (!pathname.startsWith("/repo.git/")) return new Response("not found", { status: 404 });
+      const f = file(join(bareDir, pathname.slice("/repo.git/".length)));
+      return (await f.exists()) ? new Response(f) : new Response("not found", { status: 404 });
+    },
+  });
+  const repoUrl = `git+http://127.0.0.1:${server.port}/repo.git`;
+
+  const env = {
+    ...bunEnv,
+    ...gitConfigEnv,
+    BUN_INSTALL_CACHE_DIR: join(packageDir, ".bun-cache"),
+  };
+
+  async function install() {
+    await using proc = spawn({
+      cmd: [bunExe(), "install"],
+      cwd: packageDir,
+      env,
+      stdout: "pipe",
+      stderr: "pipe",
+    });
+    const [err, exitCode] = await Promise.all([proc.stderr.text(), proc.exited, proc.stdout.text()]);
+    expect(err).not.toContain("error:");
+    expect(exitCode).toBe(0);
+  }
+
+  const rootPackageJson = {
+    name: "patched-git-workspace",
+    workspaces: ["packages/*"],
+  };
+  await write(packageJson, JSON.stringify(rootPackageJson));
+  await write(
+    join(packageDir, "packages", "member", "package.json"),
+    JSON.stringify({
+      name: "member",
+      version: "1.0.0",
+      dependencies: {
+        "git-dep": repoUrl,
+      },
+    }),
+  );
+  await write(
+    join(packageDir, "patches", "git-dep.patch"),
+    `diff --git a/index.js b/index.js
+index 1f0e8b9f1f9a56799cdbc1a5a2f8cf9f9a3b2f1c..2f0e8b9f1f9a56799cdbc1a5a2f8cf9f9a3b2f1d 100644
+--- a/index.js
++++ b/index.js
+@@ -1 +1 @@
+-console.log("original");
++console.log("patched");
+`,
+  );
+
+  const installedIndexJs = file(join(packageDir, "packages", "member", "node_modules", "git-dep", "index.js"));
+
+  await install();
+  expect(await installedIndexJs.text()).toBe('console.log("original");\n');
+
+  // The patchedDependencies key must carry the resolved commit; a key without
+  // it is silently ignored, which the patched-content assertion would catch.
+  await write(
+    packageJson,
+    JSON.stringify({
+      ...rootPackageJson,
+      patchedDependencies: {
+        [`git-dep@${repoUrl}#${sha}`]: "patches/git-dep.patch",
+      },
+    }),
+  );
+  await install();
+  expect(await installedIndexJs.text()).toBe('console.log("patched");\n');
+
+  // Cold cache with the patch still in the lockfile: the install phase
+  // clones and checks out itself, applying the patch after the checkout.
+  await rm(join(packageDir, ".bun-cache"), { recursive: true, force: true });
+  await rm(join(packageDir, "node_modules"), { recursive: true, force: true });
+  await rm(join(packageDir, "packages", "member", "node_modules"), { recursive: true, force: true });
+  await install();
+  expect(await installedIndexJs.text()).toBe('console.log("patched");\n');
+
+  await write(packageJson, JSON.stringify(rootPackageJson));
+  await install();
+  expect(await installedIndexJs.text()).toBe('console.log("original");\n');
+});
+
 for (const backend of ["clonefile", "hardlink", "copyfile"]) {
   test(`isolated install with backend: ${backend}`, async () => {
     const { packageJson, packageDir } = await registry.createTestDir({ bunfigOpts: { linker: "isolated" } });