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,61 @@ 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(),
+                            // 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,133 @@
   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" } });