From 638c33bc01965247614af67875644e2e18a8e4a3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miguel=20=C3=81ngel?= Date: Thu, 2 Jul 2026 17:45:23 -0700 Subject: [PATCH] fix(cli): lock chrome-headless-shell install against concurrent extraction races (#1866) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 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 | 108 ++++++++++++++++++++++- packages/cli/src/browser/manager.ts | 105 +++++++++++++++++++++- 2 files changed, 207 insertions(+), 6 deletions(-) diff --git a/packages/cli/src/browser/manager.test.ts b/packages/cli/src/browser/manager.test.ts index 32953e66a..f19d1993b 100644 --- a/packages/cli/src/browser/manager.test.ts +++ b/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({ diff --git a/packages/cli/src/browser/manager.ts b/packages/cli/src/browser/manager.ts index c7cd49543..a7c6ea14f 100644 --- a/packages/cli/src/browser/manager.ts +++ b/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 { + 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( + fn: () => Promise, + timeoutMs = INSTALL_LOCK_TIMEOUT_MS, + pollMs = INSTALL_LOCK_POLL_MS, +): Promise { + // 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 { `[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 downloadBrowser(options)); } const fromSystem = findFromSystem(); @@ -370,7 +460,14 @@ export async function ensureBrowser(options?: EnsureBrowserOptions): Promise { + // 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 {