fix(cli): lock chrome-headless-shell install against concurrent extraction races (#1866)

* fix(cli): lock chrome-headless-shell install against concurrent extraction races

A detailed post-release feedback report of `render` producing a fully
black 15s MP4 despite lint/validate/inspect/snapshot all passing and
Studio preview playing correctly. Root cause traced by the reporter:
chrome-headless-shell had been manually re-extracted after `browser
ensure`'s own download got stuck mid-extraction when two concurrent
invocations raced on the same cache dir. The manual extraction lost a
macOS Gatekeeper/quarantine or GPU/Metal entitlement bit that a clean
install sets, so headless GPU frame capture silently returned all-black
frames — invisible to every existing health check, since they only
confirm the binary *exists*, not that it captures real pixels.
`--no-browser-gpu` fixed it completely, confirming the GPU-capture path
specifically. A related, vaguer report of the same race the prior loop
run ("'browser ensure' hung mid-extraction after a race from two
concurrent invocations") was deferred pending a clearer repro; this
report supplied one.

@puppeteer/browsers' install() has no concurrency guard of its own —
confirmed by reading its source: two concurrent installs for the same
browser/buildId both proceed straight to download+unpack with no
existing-install check, no lock. Two ensureBrowser()/findBrowser() calls
that both miss the cache at the same time (the common case on a fresh
machine, or right after `browser clear`) race on the same extract target.

Fix: mkdirSync as an atomic cross-process mutex around the download —
recursive:false makes it throw EEXIST when another process already holds
it (that's load-bearing: recursive:true would silently no-op instead).
Zero new dependencies. A concurrent caller polls until the lock releases,
then re-checks the cache before deciding whether to download at all — the
common case (loser waits, then reuses the winner's completed install)
never re-downloads. A lock held past a generous timeout is reclaimed
rather than left to wedge every future render if the holder crashed
mid-extraction. Applied to both call sites that reach the racy
downloadBrowser() (ensureBrowser's two paths, and findBrowser's stale-
cache re-download — the file already carries a code-duplication
suppression between these two near-identical functions).

Not doing (out of scope for this fix): the reporter's second suggestion,
a deeper `doctor` check that actually captures a test frame rather than
checking binary existence. That's a real gap but a separate, larger
feature — this fix prevents the corruption that caused it, which matters
more than detecting it after the fact.

Tests: two new cases (lock releases after a successful download; a lock
held past its timeout is reclaimed rather than hanging — exercised via
withInstallLock's injectable timeoutMs/pollMs with tiny real waits,
avoiding fake-timer mocking through the full async ensureBrowser call
graph). All 13 tests in manager.test.ts, 22 across packages/cli/src/browser,
and the full CLI suite (1115 tests) pass.

* test(cli): isolate browser install lock test from system chrome

* fix(cli): guard stale browser lock reclaim

packages/cli/src/browser/manager.test.ts

@@ -54,20 +54,45 @@ interface FsMockOptions {
 }
 
 function installFsMocks({ existing, dirs }: FsMockOptions) {
+  // Mutable, and returned, so tests can pre-seed a "lock already held" path or
+  // assert the lock dir doesn't leak after ensureBrowser resolves.
+  const paths = new Set(existing);
+  const mtimes = new Map([...existing].map((p) => [p, 0]));
   vi.doMock("node:fs", () => ({
-    existsSync: (p: string) => existing.has(p),
+    existsSync: (p: string) => paths.has(p),
     readdirSync: (p: string) => {
       const entries = dirs?.[p];
       if (!entries) throw new Error(`ENOENT: readdirSync mock had no entry for ${p}`);
       return entries;
     },
-    rmSync: () => {},
+    mkdirSync: (p: string, opts?: { recursive?: boolean }) => {
+      if (!opts?.recursive && paths.has(p)) {
+        const err = new Error(`EEXIST: file already exists, mkdir '${p}'`);
+        (err as NodeJS.ErrnoException).code = "EEXIST";
+        throw err;
+      }
+      paths.add(p);
+      mtimes.set(p, Date.now());
+    },
+    rmSync: (p: string) => {
+      paths.delete(p);
+      mtimes.delete(p);
+    },
+    statSync: (p: string) => {
+      if (!paths.has(p)) {
+        const err = new Error(`ENOENT: no such file or directory, stat '${p}'`);
+        (err as NodeJS.ErrnoException).code = "ENOENT";
+        throw err;
+      }
+      return { mtimeMs: mtimes.get(p) ?? 0 };
+    },
   }));
   vi.doMock("node:os", () => ({
     homedir: () => FAKE_HOME,
     platform: () => "linux",
     arch: () => "x64",
   }));
+  return paths;
 }
 
 function installPuppeteerBrowsersMock(
@@ -87,6 +112,15 @@ function installPuppeteerBrowsersMock(
   }));
 }
 
+function installChildProcessMocks() {
+  vi.doMock("node:child_process", () => ({
+    execSync: vi.fn(() => {
+      throw new Error("not found");
+    }),
+    spawnSync: vi.fn(),
+  }));
+}
+
 describe("findBrowser — cache resolution", () => {
   const origPlatform = process.platform;
   const origArch = process.arch;
@@ -99,6 +133,7 @@ describe("findBrowser — cache resolution", () => {
     Object.defineProperty(process, "platform", { value: "linux", configurable: true });
     Object.defineProperty(process, "arch", { value: "x64", configurable: true });
     delete process.env["HYPERFRAMES_BROWSER_PATH"];
+    installChildProcessMocks();
   });
 
   afterEach(() => {
@@ -107,6 +142,7 @@ describe("findBrowser — cache resolution", () => {
     vi.restoreAllMocks();
     vi.doUnmock("node:fs");
     vi.doUnmock("node:os");
+    vi.doUnmock("node:child_process");
     vi.doUnmock("@puppeteer/browsers");
   });
 
@@ -147,6 +183,74 @@ describe("findBrowser — cache resolution", () => {
     expect(warnSpy).toHaveBeenCalledWith(expect.stringContaining("Cached binary missing"));
   });
 
+  it("ensureBrowser does not leak the install lock directory after a successful download", async () => {
+    // Regression: @puppeteer/browsers' install() has no concurrency guard —
+    // two CLI invocations that both miss the cache AND system Chrome (the
+    // reported scenario: two `hyperframes browser ensure` runs racing) hit
+    // ensureBrowser's final download-of-last-resort at once, racing on the
+    // same extract target. mkdirSync as an atomic mutex closes that race;
+    // this asserts the lock is actually released afterward (a leaked lock
+    // would permanently wedge every future render on this machine).
+    const HF_LOCK = join(HF_CACHE, ".install.lock");
+    const downloadedBinary = join(HF_CACHE, "chrome-headless-shell", "downloaded");
+    // Cache dir exists but is empty (no manifest entries) — distinct from the
+    // ENOTDIR "cache unreadable" case, which falls back to system instead.
+    const paths = installFsMocks({ existing: new Set([HF_CACHE]) });
+    installPuppeteerBrowsersMock({
+      installedInHfCache: [],
+      installResult: { executablePath: downloadedBinary },
+    });
+
+    const { ensureBrowser } = await import("./manager.js");
+    const result = await ensureBrowser();
+
+    expect(result).toEqual({ executablePath: downloadedBinary, source: "download" });
+    expect(paths.has(HF_LOCK)).toBe(false);
+  });
+
+  it("withInstallLock reclaims a lock held past the timeout instead of hanging forever", async () => {
+    // A crashed/killed process could leave the lock directory behind
+    // permanently. Reclaiming after a timeout (rather than hanging or
+    // refusing forever) is the behavior that makes the lock safe to add at
+    // all — otherwise one bad exit wedges every future render. Exercises
+    // withInstallLock directly with tiny real timeouts (it takes an
+    // injectable timeoutMs/pollMs for exactly this) rather than mocking
+    // Date.now()/setTimeout through the full ensureBrowser call graph.
+    const HF_LOCK = join(HF_CACHE, ".install.lock");
+    const paths = installFsMocks({ existing: new Set([HF_CACHE, HF_LOCK]) });
+
+    const { withInstallLock } = await import("./manager.js");
+    const result = await withInstallLock(async () => "done", 10, 5);
+
+    expect(result).toBe("done");
+    expect(paths.has(HF_LOCK)).toBe(false);
+  });
+
+  it("withInstallLock does not reclaim another waiter's fresh lock after this waiter timed out", async () => {
+    // Regression guard for the timeout-reclaim race: if multiple waiters cross
+    // the stale-lock deadline together, waiter A can reclaim the stale lock and
+    // acquire a fresh one. Waiter B's old deadline is still expired, but it must
+    // not delete A's fresh lock.
+    const HF_LOCK = join(HF_CACHE, ".install.lock");
+    const RECLAIM_LOCK = join(HF_CACHE, ".install.reclaim.lock");
+    const paths = installFsMocks({ existing: new Set([HF_CACHE, HF_LOCK]) });
+
+    const { withInstallLock } = await import("./manager.js");
+    const first = withInstallLock(
+      async () => {
+        await new Promise((resolve) => setTimeout(resolve, 30));
+        return "first";
+      },
+      10,
+      5,
+    );
+    const second = withInstallLock(async () => "second", 10, 5);
+
+    await expect(Promise.all([first, second])).resolves.toEqual(["first", "second"]);
+    expect(paths.has(HF_LOCK)).toBe(false);
+    expect(paths.has(RECLAIM_LOCK)).toBe(false);
+  });
+
   it("warns and falls through when the hyperframes cache cannot be read", async () => {
     installFsMocks({ existing: new Set([HF_CACHE, SYSTEM_CHROME]) });
     installPuppeteerBrowsersMock({

packages/cli/src/browser/manager.ts

@@ -1,6 +1,6 @@
 // fallow-ignore-file code-duplication
 import { execSync, spawnSync } from "node:child_process";
-import { existsSync, readdirSync, rmSync } from "node:fs";
+import { existsSync, mkdirSync, readdirSync, rmSync, statSync } from "node:fs";
 import { basename } from "node:path";
 import { homedir } from "node:os";
 import { join } from "node:path";
@@ -28,6 +28,96 @@ const CACHE_DIR = join(homedir(), ".cache", "hyperframes", "chrome");
 // too or it silently picks system Chrome over a perfectly good headless-shell.
 const PUPPETEER_CACHE_DIR = join(homedir(), ".cache", "puppeteer", "chrome-headless-shell");
 
+// `@puppeteer/browsers`' install() has no concurrency guard of its own — two
+// CLI invocations that both miss the cache at the same time both extract into
+// the same target directory simultaneously. A killed/interrupted extraction
+// from that race can leave a binary that merely *exists* (so doctor/lint/
+// validate all report healthy) while missing bits a clean install sets (e.g.
+// macOS Gatekeeper/quarantine + GPU/Metal entitlements) — reported as headless
+// GPU frame capture silently returning all-black frames despite --browser-gpu
+// auto/hardware, invisible until someone inspects the actual pixels.
+//
+// mkdirSync is atomic (EEXIST if another process already holds it), so it
+// doubles as a zero-dependency cross-process mutex — no lockfile library needed.
+const INSTALL_LOCK_DIR = join(CACHE_DIR, ".install.lock");
+const INSTALL_RECLAIM_LOCK_DIR = join(CACHE_DIR, ".install.reclaim.lock");
+const INSTALL_LOCK_TIMEOUT_MS = 120_000; // generous: a real download+extract can take a while
+const INSTALL_LOCK_POLL_MS = 200;
+
+function isErrno(err: unknown, code: string): boolean {
+  return (err as NodeJS.ErrnoException).code === code;
+}
+
+function sleep(ms: number): Promise<void> {
+  return new Promise((resolve) => setTimeout(resolve, ms));
+}
+
+function tryAcquireDirLock(lockDir: string): boolean {
+  try {
+    // recursive:false is load-bearing: it's what makes this throw EEXIST
+    // (and therefore act as a mutex) instead of silently no-op'ing like
+    // `mkdir -p` when the lock dir already exists.
+    mkdirSync(lockDir, { recursive: false });
+    return true;
+  } catch (err) {
+    if (!isErrno(err, "EEXIST")) throw err;
+    return false;
+  }
+}
+
+function reclaimStaleInstallLock(timeoutMs: number): void {
+  if (!tryAcquireDirLock(INSTALL_RECLAIM_LOCK_DIR)) return;
+  try {
+    const mtimeMs = statSync(INSTALL_LOCK_DIR).mtimeMs;
+    if (Date.now() - mtimeMs > timeoutMs) {
+      rmSync(INSTALL_LOCK_DIR, { recursive: true, force: true });
+    }
+  } catch (err) {
+    if (!isErrno(err, "ENOENT")) throw err;
+  } finally {
+    rmSync(INSTALL_RECLAIM_LOCK_DIR, { recursive: true, force: true });
+  }
+}
+
+// timeoutMs/pollMs are parameters (not just the module constants) so tests can
+// exercise the reclaim-on-timeout branch with real but tiny waits instead of
+// mocking Date.now()/setTimeout through the full ensureBrowser call graph.
+export async function withInstallLock<T>(
+  fn: () => Promise<T>,
+  timeoutMs = INSTALL_LOCK_TIMEOUT_MS,
+  pollMs = INSTALL_LOCK_POLL_MS,
+): Promise<T> {
+  // recursive:false below needs the parent to already exist (unlike `mkdir -p`).
+  if (!existsSync(CACHE_DIR)) mkdirSync(CACHE_DIR, { recursive: true });
+  let deadline = Date.now() + timeoutMs;
+  for (;;) {
+    if (existsSync(INSTALL_RECLAIM_LOCK_DIR)) {
+      await sleep(pollMs);
+      continue;
+    }
+    if (tryAcquireDirLock(INSTALL_LOCK_DIR)) {
+      rmSync(INSTALL_RECLAIM_LOCK_DIR, { recursive: true, force: true });
+      break;
+    }
+    if (Date.now() > deadline) {
+      // The reclaim gate matters when multiple waiters cross the timeout at
+      // once: without it, waiter A can delete the stale lock and acquire a
+      // fresh one, then waiter B (whose old deadline also expired) can delete
+      // A's fresh lock. The gate serializes reclaimers, and the mtime re-check
+      // after the gate prevents deleting a fresh lock another waiter just won.
+      reclaimStaleInstallLock(timeoutMs);
+      deadline = Date.now() + timeoutMs;
+      continue;
+    }
+    await sleep(pollMs);
+  }
+  try {
+    return await fn();
+  } finally {
+    rmSync(INSTALL_LOCK_DIR, { recursive: true, force: true });
+  }
+}
+
 export type BrowserSource = "env" | "cache" | "system" | "download";
 
 export interface BrowserResult {
@@ -281,7 +371,7 @@ export async function findBrowser(): Promise<BrowserResult | undefined> {
       `[browser] Cached binary missing at ${fromCache.staleHyperframesCachePath} — re-downloading...`,
     );
     try {
-      return await downloadBrowser();
+      return await withInstallLock(() => downloadBrowser());
     } catch (err) {
       const cause = normalizeErrorMessage(err);
       throw new Error(
@@ -361,7 +451,7 @@ export async function ensureBrowser(options?: EnsureBrowserOptions): Promise<Bro
     console.warn(
       `[browser] Cached binary missing at ${fromCache.staleHyperframesCachePath} — re-downloading...`,
     );
-    return downloadBrowser(options);
+    return withInstallLock(() => downloadBrowser(options));
   }
 
   const fromSystem = findFromSystem();
@@ -370,7 +460,14 @@ export async function ensureBrowser(options?: EnsureBrowserOptions): Promise<Bro
     return fromSystem;
   }
 
-  return downloadBrowser(options);
+  return withInstallLock(async () => {
+    // Re-check after acquiring the lock: a concurrent invocation may have
+    // finished installing while we were waiting, in which case reuse its
+    // result instead of downloading and extracting a second time.
+    const afterLock = await findFromCache();
+    if (afterLock.result) return afterLock.result;
+    return downloadBrowser(options);
+  });
 }
 
 async function downloadBrowser(options?: EnsureBrowserOptions): Promise<BrowserResult> {