fix(browser): recover abandoned install locks

[miguel-heygen]

What

Recover Chrome installation when a crashed process leaves a stale .chrome.install.reclaim.lock, including the reported case where both install lock directories remain.

Why

On Windows, a timed-out render could leave both browser lock directories behind. Every later render waited forever at browser setup because the reclaim gate was never aged out. HYPERFRAMES_BROWSER_PATH bypassed the stale cache, but the default managed-browser path remained blocked.

How

• Detect and remove an abandoned reclaim gate after the configured stale interval.
• Reclaim an already-stale install lock immediately instead of waiting through a fresh timeout.
• Preserve the existing reclaim gate that prevents concurrent waiters from deleting a newly acquired lock.
• Add a fake-timer regression test covering both stale lock directories left by a crashed reclaimer.

Test plan

• Unit tests added/updated
• Manual testing performed
• Documentation updated (not applicable)

Verification:

• Reproduced with published 0.7.55 under an isolated HOME containing both stale lock directories; render remained blocked at browser setup until host timeout.
• packages/cli manager suite: 27 passed.
• packages/cli full suite: 129 files and 1650 tests passed.
• packages/cli typecheck passed.
• Lint and formatting passed.
• Built the fixed local CLI and rendered a 48-frame MP4 under the same pre-aged two-lock setup; render exited 0 and produced the output.

[james-russo-rames-d-jusso]

Reviewed at a3bb558.

Clean fix for a real production bug (Windows two-lock deadlock reproducible on 0.7.55 per the PR body). Two orthogonal problems, two orthogonal fixes, both correct:

• Abandoned reclaim gate — reclaimAbandonedReclaimLock at the top of each poll iteration ages out the reclaim gate itself when the process holding it died before releasing. Correctly narrower than the install-lock reclaim (single rmSync, no gate-of-gate needed because concurrent rmSync of a non-existent path is a no-op under force: true).
• Already-stale install lock short-circuit — isDirLockStale(INSTALL_LOCK_DIR, timings.staleMs) || Date.now() > deadline skips the full-timeout wait when the lock is already stale on entry. The reclaim-gate serialization in reclaimStaleInstallLock still prevents concurrent reclaimers from racing, so the earlier trigger doesn't reintroduce the multi-waiter reclaim hazard.

Test coverage is on-point — the fake-timer regression test exercises the both-locks-present path with pre-aged mtime, and the existing progress-reporting test correctly pins initialMtimeMs: Date.now() to keep its lock non-stale so the new fast-path doesn't skip the wait it wants to observe.

What I didn't verify

• Windows-specific mtimeMs behavior on a directory (statSync on a lock dir) — trusting Node's cross-platform surface. Miguel reproduced under Windows in the PR body, so this is empirically confirmed.

— Review by Rames D Jusso

[vanceingalls]

R1 by Via — posted after Rames-D's parallel review; adds a distinct flake-mechanism diagnosis.

Peer-lens with Rames-D: convergent on the core fix (reclaim-gate + immediate-stale-lock reclaim, both correct, blast-radius contained, real production dispatch in tests). Unique from this review: a specific diagnosis of the Test lane flake on manager.test.ts:428 and a suggested tightening. Verdict downgraded to 🟡 LGTM-conditional on that basis.

PR #2328 — fix(browser): recover abandoned install locks
Verdict: 🟡 LGTM-conditional
Head: a3bb5583 on main (single-commit branch, miguel-heygen)
Status: CI green after retry (Test failed on attempt 1 at job/86790562063, passed on attempt 2 at job/86922281176); mergeable_state=blocked (awaiting review approval, not CI).

Ship-blocker to flag (flake, not deterministic)

• Test lane failed on attempt 1 with an assertion inside a test THIS PR MODIFIED:
  manager.test.ts:428 — expected false to be true on the assertion
  warnSpy.mock.calls.some(([msg]) => String(msg).includes("Waiting for another hyperframes process"))
  in withInstallLock reports progress while waiting instead of staying silent.
• The identical run passed on GitHub's rerun (attempt 2 at 20:10:23Z, all 27 tests green). So the failure is a flake, but the flake was introduced by this PR — see Finding 1. Retries hide the problem; the flake will re-emerge on every subsequent PR that pushes CLI test load.

Summary of change
Adds reclaimAbandonedReclaimLock + isDirLockStale helpers to packages/cli/src/browser/manager.ts so that (a) a crashed process holding the .chrome.install.reclaim.lock gate no longer wedges every future installer forever (the reported "both lock directories left behind" case), and (b) a waiter that walks into an already-stale .chrome.install.lock reclaims it immediately instead of waiting through a fresh staleMs=120s timeout. Adds one regression test using fake timers for the two-lock-dir scenario, and modifies one existing test's mtime seeding to keep it viable under the new mtime-check semantics.

Findings

1. 🟡 Flake risk (nit — should tighten before merge) — packages/cli/src/browser/manager.test.ts:413-429 ("withInstallLock reports progress while waiting instead of staying silent"). The test now seeds initialMtimeMs: Date.now() at setup time, then does await import("./manager.js") (a vi.resetModules()'d dynamic import per beforeEach) before calling withInstallLock. Under the new isDirLockStale(INSTALL_LOCK_DIR, timings.staleMs) check on manager.ts:154 (short-circuited ahead of the wait-notice branch on lines 147-153), if the dynamic import takes more than staleMs=50ms — plausible on a busy shared-runner with vitest resetting modules between tests — iteration 1 fires reclaimStaleInstallLock immediately, the loop breaks a few ms later, and waitedMs never reaches waitNoticeMs=20. No warn ever emits, and the assertion at line 428 fails with exactly the observed expected false to be true. The pre-PR test tolerated arbitrary import latency because the old code only checked Date.now() > deadline from waitStart (post-import), not mtime seeded pre-import. Suggested fix: mirror the pattern the PR already adopted for the new test at line 352-367 — vi.useFakeTimers() with now: 1000 and drive time with vi.advanceTimersByTimeAsync, or move the initialMtimeMs capture inside a helper that runs after the import. Refute attempt: I looked for other reasons the warn wouldn't fire — spy setup, ordering with previous useFakeTimers in the sibling test bleeding through (afterEach vi.useRealTimers() handles that), module-cache warmth reducing import cost (each test does vi.resetModules(), so re-execution is always required). None hold up. The observed 123ms duration is consistent with iter-1 immediate-reclaim + break; a healthy run should be ~50-60ms (11-12 poll iters at ~5ms each). Not-refuted.

2. 🟢 Reclaim-gate abandonment fix is well-shaped — packages/cli/src/browser/manager.ts:103-111 + 134-142. reclaimAbandonedReclaimLock reads mtime via isDirLockStale (correctly non-strict >), rmSync's with force:true (ENOENT-safe by itself, plus a redundant ENOENT catch — belt and suspenders), and swallows any other errno type via the catch. Called only inside the existsSync(RECLAIM_LOCK) branch, so no cost when the gate is absent. Refute attempt: race with reclaimStaleInstallLock's legitimate holder — that holder just mkdirSync'd the gate, so its mtime is fresh (< staleMs old), and isDirLockStale returns false. Two waiters both racing to clean an actually-abandoned gate — both call rmSync with force:true, idempotent. Not-refuted.

3. 🟢 Immediate-reclaim of already-stale install lock is correct — manager.ts:154. Adding isDirLockStale(INSTALL_LOCK_DIR, staleMs) || Date.now() > deadline before the reclaim path means a fresh waiter walking into a lock whose mtime is already 3 hours old no longer waits its own 120s deadline before acting. The reclaim gate on line 89-101 still serializes reclaimers via tryAcquireDirLock(RECLAIM_LOCK) and re-checks the mtime after acquiring the gate, so waiter B can't delete waiter A's freshly-won lock. Refute attempt: what if the holder's event loop is blocked for >120s so heartbeat setInterval can't fire, and a waiter reads the resulting stale mtime and reclaims a still-live install? downloadBrowser calls @puppeteer/browsers.install() which is async (extraction on a worker), and purgeStaleInstall is a sync rmSync of one version dir (seconds, not minutes). The comment at line 40-41 already acknowledges this concurrency contract. Not-refuted.

4. 🟢 Blast radius covered — withInstallLock has 3 call sites, all in manager.ts (lines 474, 562, 577: findBrowser's stale-cache path, ensureBrowser's stale-cache path, and ensureBrowser's main install path). All three go through the same function, so the reclaim-gate protection is uniform. No parallel install paths (grep across the repo confirms INSTALL_RECLAIM_LOCK_DIR is defined and used only here). Sibling-of telemetry-gap-on-parallel-branch concern doesn't apply.

5. 🟢 New test is real, not aspirational — manager.test.ts:352-367 imports the production withInstallLock from ./manager.js, exercises the actual reclaimAbandonedReclaimLock code path (would loop forever without the fix; Promise.race against Promise.resolve("still waiting") correctly asserts progress within pollMs * 2 = 10ms fake time). Under fake timers, Date.now() returns a real wall-clock timestamp frozen at useFakeTimers() time — so Date.now() - 0 > 50ms is trivially true, which is a mild characterization-test smell (any staleMs value would pass), but the reclaim-of-abandoned-gate semantics are still verified via the paths.has(HF_RECLAIM_LOCK) === false assertion. Signature markers checked: no local reclaimAbandonedReclaimLock shadow in the test file, no un-resolvable SHA references, direct dispatch through the production import.

6. 🟢 Rollout — no flag needed — Unconditional bug fix in the lock lifecycle. Correct choice: a feature-flagged rollout would leave broken installs behind for the disabled cohort, which is the opposite of the fix's purpose. No env-var scaffolding required.

Cross-checks

• Head SHA current: yes (a3bb5583d67ab8a908f38f2853df438c525e70fb, single commit, no post-review author pushes).
• Prior reviewers on record: none (0 reviews; 0 requested_reviewers; 0 issue comments).
• Mergeability: mergeable=true, mergeable_state=blocked (awaiting approval; no CI block after retry).
• Test lane state: currently GREEN via attempt 2 rerun; RED on attempt 1 with the flake described above.
• Failing-test root cause: PR-caused flake, not pre-existing/unrelated. The failure fired inside a test THIS PR MODIFIED, with a failure mode traceable to the new isDirLockStale short-circuit interacting with mtime seeded before the dynamic import. Retry cleared it, but the flake will re-emerge under sufficient CI load.

R1 by Via

[jrusso1020]

Stamped — verified before approving: CI green (35 pass) at a3bb5583, author miguel-heygen, no standing CHANGES_REQUESTED. Confirmed at source: reclaimAbandonedReclaimLock ages out the reclaim gate itself (rmSync when stale) and the install-lock wait short-circuits via isDirLockStale(INSTALL_LOCK_DIR, …) — closing the Windows two-lock deadlock where a process dying after acquiring the reclaim gate stalled every future installer. ENOENT handled (treated as "not stale" / re-thrown otherwise).

— Rames Jusso