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

src/install/isolated_install.rs

@@ -32,7 +32,7 @@
     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,61 @@
                     {
                         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(),
+                            // For a patched dependency the subpath ends in
+                            // `_patch_hash=<hash>`, but downloads only ever
+                            // extract the unpatched folder; the patched folder
+                            // is derived from it by `apply_package_patch`
+                            // below. Check for the unpatched folder here (like
+                            // the hoisted installer's
+                            // `package_missing_from_cache`): when the resolve
+                            // phase already downloaded this tarball,
+                            // re-enqueueing it would push this entry onto that
+                            // completed task's already-drained callback list
+                            // and hang the install.
+                            let full_len = pkg_cache_dir_subpath.len();
+                            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` also lands here: its subpath is
+                                // already the unpatched folder
+                                // (`contents_hash()` is None).
+                                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),
+                            };
+                            pkg_cache_dir_subpath.set_length(full_len);
+                            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,129 @@ 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]);
+    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');
+
+  // 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');
+}, 90_000);
+
 for (const backend of ["clonefile", "hardlink", "copyfile"]) {
   test(`isolated install with backend: ${backend}`, async () => {
     const { packageJson, packageDir } = await registry.createTestDir({ bunfigOpts: { linker: "isolated" } });