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

[miguel-heygen]

Root-caused from a detailed post-release feedback report, and confirms/resolves a vaguer report of the same race from the prior loop run.

"render (draft quality, default --browser-gpu auto->hardware) produced a fully black 15s mp4 (117KB, all frames black) despite lint/validate/inspect/snapshot all passing and Studio preview playing the composition correctly. [...] Root cause suspected: chrome-headless-shell binary was manually re-extracted from the cached zip (CLI's own 'browser ensure' download got stuck mid-extraction after two concurrent invocations raced on the same cache dir - had to kill both and manually unzip + chmod +x + xattr -dr quarantine). The manual extraction likely lost some GPU/Metal entitlement that the official install flow sets, causing headless GPU frame capture to silently return black canvases. Passing --no-browser-gpu fixed it completely."

Root cause

@puppeteer/browsers' install() has no concurrency guard — 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 (common on a fresh machine, or right after browser clear) race on the same extract target. A killed/interrupted extraction from that race leaves a binary that merely exists — every health check (doctor, lint, validate, inspect, snapshot) reported healthy — while missing bits a clean install sets, causing headless GPU frame capture to silently return black frames.

Fix

mkdirSync as an atomic cross-process mutex around the download. recursive:false is load-bearing — it's what makes mkdirSync throw EEXIST when another process already holds the lock instead of silently no-op'ing like mkdir -p. 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 second time. 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

The reporter's second suggestion — a deeper doctor check that actually captures a test frame rather than checking binary existence. Real gap, but a separate, larger feature. This fix prevents the corruption that caused the symptom, which matters more than detecting it after the fact.

Tests

2 new cases in manager.test.ts: 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 — simpler and more reliable than mocking Date.now()/setTimeout 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.

[james-russo-rames-d-jusso]

Reviewed at bc481074 (batch review, Group B CLI/renderer; layering on Magi's own vetting).
Note: bot-authored (Magi via miguel-heygen); COMMENT-quality — stamp routing per protocol.

Summary — Adds an mkdirSync-based cross-process mutex around downloadBrowser() at all three call sites, with a 120s reclaim on stale locks and a post-lock cache re-check so the loser reuses the winner's install instead of re-downloading.

The core mechanic is right: mkdirSync(path, { recursive: false }) is genuinely atomic on POSIX + Windows, so it's a legitimate zero-dep mutex, and the comment calling out why recursive:false is load-bearing is correct. Root-cause analysis (race in @puppeteer/browsers install() → interrupted extract → binary exists but is missing GPU entitlements → black frames) matches the reported symptom cleanly.

Concerns

🟠 Reclaim isn't atomic — two-process deadline race (manager.ts:66-73) — if two peers are BOTH waiting on a lock that crosses the 120s deadline, both call rmSync in sequence, then race on the follow-up mkdirSync. The winner takes the lock, the loser's next iteration still sees Date.now() > deadline, calls rmSync again → destroys the new winner's lock → both proceed into downloadBrowser() concurrently, which is exactly what the mutex was supposed to prevent. Narrow window (requires two waiters crossing the deadline simultaneously after a hard crash left the lock in place), but it's the failure mode this PR is trying to solve, arriving at 120s+ instead of 0s. Cheapest fix: after rmSync-on-timeout, try mkdirSync immediately in the same tick (before another peer can) — if it succeeds we hold the lock, if EEXIST we accept someone else won and continue polling with a fresh deadline. Or: write PID + timestamp into a sentinel file inside the lock dir so reclaim can check "did someone else already reclaim in the last poll interval". Not blocking, but worth naming in a comment even if you decide to accept the risk.

Nits

🟡 findBrowser stale-cache path skips the post-lock re-check (manager.ts:335) — the other two withInstallLock call sites just call downloadBrowser directly (or, in ensureBrowser, only the last one has the findFromCache re-check). In the stale-cache branch, a concurrent peer might have already re-downloaded a fresh binary by the time we get the lock — a re-check would let us skip the second download. Small optimization; the staleHyperframesCachePath case is already the uncommon path.

🟡 Test coverage gap for the post-lock re-check branch — the "does not leak install lock" test asserts release, and the "reclaims past timeout" test asserts reclaim, but neither exercises the "loser waits, then reuses winner's install" path that the PR body highlights as the common case. A test that pre-seeds the cache while the mock install() is running would pin the intended behavior.

Questions

↩️ Chosen 120s timeout ceiling — on slow networks (satellite, tethered mobile, dev containers behind restricted proxies), a legitimate chrome-headless-shell download + extract can plausibly cross 120s. Under load the reclaim would kill an in-progress install for a peer that's still healthy, and both peers then race the re-download from scratch. Was 120s calibrated against any measurement, or is it a starting guess? Worth documenting either way — future you (or a future reporter) will otherwise be back here with "why does browser ensure keep looping".

What I didn't verify

• Whether @puppeteer/browsers' internal install() writes a partial binary at the target path or an atomic temp+rename — if it does temp+rename, an interrupted extraction wouldn't leave a "binary exists but is broken" state and the failure mode described in the PR body would be narrower than stated. Reading their source is called out in the PR body, so probably fine, but I didn't reproduce that read.
• Windows behavior of mkdirSync EEXIST semantics under this exact recursive: false shape — POSIX behavior is well-defined, but if this CLI is expected to run on Windows too, worth a manual check.
• Whether the CI test-isolation fix mentioned in the briefing landed cleanly in the current test file (I only inspected the two new tests and the mock updates, not the full pre-existing suite for cross-test state).

— Rames D Jusso

[james-russo-rames-d-jusso]

R2 verification — reviewed at d143f807.

Summary — Magi pushed d143f807 ("fix(cli): guard stale browser lock reclaim") to address the R1 finding: reclaim-on-timeout race where two waiters both crossing the 120s deadline could stomp a fresh lock (waiter A reclaims + acquires new, waiter B's stale deadline also expired, B deletes A's fresh lock).

Verdict — 🟢 Resolved.

Verification details

• Serialized reclaim gate: reclaimStaleInstallLock at packages/cli/src/browser/manager.ts:64-75 gates on tryAcquireDirLock(INSTALL_RECLAIM_LOCK_DIR) (mkdirSync-EEXIST mutex) — only one waiter enters the reclaim body at a time. finally clears the reclaim lock at :74.
• Fresh-lock mtime recheck: at manager.ts:66-68, after acquiring the reclaim gate, it does statSync(INSTALL_LOCK_DIR).mtimeMs and only deletes if Date.now() - mtimeMs > timeoutMs. Waiter A reclaims at t=121s, waiter B enters gate at t≈121+ε, sees the fresh lock's mtime is essentially now (delta ≈ 0ms), condition > 120_000ms is false → does NOT delete A's fresh lock. Exactly the fix.
• Regression test at manager.test.ts:230-253: "does not reclaim another waiter's fresh lock after this waiter timed out" — spawns two withInstallLock(..., 10, 5) waiters concurrently, first holds for 30ms (crosses the 10ms deadline), asserts both resolve with their respective values and lock is clean. Directly exercises the two-waiter race, not just a single-waiter timeout.
• ENOENT handling at :70-71: if the install lock disappeared between gate acquisition and statSync, ENOENT is swallowed — correct behavior (lock is gone, nothing to do).

Residual notes (not blocking)

• existsSync(INSTALL_RECLAIM_LOCK_DIR) at manager.ts:91 is a TOCTOU-ish check-before-acquire, but the subsequent tryAcquireDirLock(INSTALL_LOCK_DIR) is atomic, so worst-case is one extra poll — no safety impact.
• Filesystem mtime granularity: on some filesystems (FAT32, older ext) mtime resolution is 1-2s. With a 120s timeout, this is fine; if timeoutMs were ever dropped below ~5s in prod this would matter. Not a concern at current values.
• CI green at d143f807 (Windows render + all shims + CodeQL). Fallow audit passes.

— Rames D Jusso

[jrusso1020]

APPROVE — verified CI green (0 fail / 0 pending) + no open CR at this head. Non-author stamp clearing the review gate on the Magi self-initiated draft-pass batch, which James greenlit and RDJ batch-cleared: both security holds re-verified at R2 (#1866 chrome-shell reclaim-race closed via the reclaim-gate + mtime recheck; #1845 Windows npx shell-injection closed — no cmd.exe, node <npx-cli.js> with pure argv), zero drift on the other nine, all green.

Merge via Magi's normal path (no admin-merge). Ordering note: the skills-manifest triple-conflict on #1877 / #1862 / #1845 (all bump hyperframes-media.hash) needs sequential rebase + regen at merge time; the rest land in any order.

— Rames Jusso