fix(cli): stopping the preview server no longer leaves a Chrome running

[miguel-heygen]

What changes

Stopping the preview server could leave a Chrome running. If a render was in progress when the stop signal arrived, its Chrome was killed by that same signal. The render then read that as a transient browser failure and its bounded retry launched a fresh Chrome that nothing tracked, so it outlived the server process.

• createStudioServer now returns a shutdown(). It marks the server as shutting down first, cancels every in-flight render, kills tracked processes, waits for the renders to finish, closes the thumbnail browser (waiting for one that is mid-launch), and drains the browser pool.
• An aborted render is classified as cancelled before the transient-browser check, so the retry does not fire (packages/engine/src/services/captureFailure.ts:158, rethrown at renderOrchestrator.ts:1266).
• Once shutdown has begun, a new render request is answered with a failed state ("Studio server is shutting down"), and a new thumbnail request does not launch a browser. The check lives in one place, in front of the thumbnail launch.
• preview calls shutdown() in its signal handler instead of the previous inline cleanup. closeThumbnailBrowser is no longer exported (nothing else used it).

Verification

• studioServer.test.ts: 17 pass on a remote Linux runner; typecheck and lint clean; comment check clean.
• New tests: shutdown cancels an in-flight render and waits for it before draining; a render started after shutdown began is refused; a thumbnail request after shutdown launches no browser; a thumbnail launch that finishes after shutdown began is released.
• Deliberate mutations, each turning one test red: no wait for the render, no shutdown check in front of the thumbnail launch, closing without waiting for an in-flight launch, no shutdown check in startRender.
• Not exercised: a real Chrome, and the 3 second forced-exit timer in preview. src/server/portUtils.test.ts fails one PID-provenance test on the remote runner; that file is not touched here and I did not compare it against main.
• Follow-up on this head: shutdown() still waited on every render before closing browsers, so a render slower than the 3s exit watchdog left Chrome running when it fired; a second Ctrl+C/SIGTERM also bypassed cleanup by dropping the one-shot signal handlers. Both fixed, plus a lease-reuse gap during the shutdown race window. Two new tests (a render whose done never settles still gets the browser closed and the pool drained within a bound; a thumbnail request landing in that race window is refused instead of reusing a live lease) bring studioServer.test.ts to 19/19; the full packages/cli suite (3442 tests), typecheck, and lint stay green on a remote Linux runner.

Review

An independent review found no blocking issues. Its three should-fix items are closed: the render test settled synchronously (now asynchronous, so it fails without the wait), the thumbnail test passed without Chrome (now asserts the launch is never attempted), and a thumbnail launch could start after shutdown had begun (fixed at the launch, with a test for the window).

[jrusso1020]

Approving at 60215c59. No earlier GitHub reviews. The one open item from the Slack thread (the 1s drain bound reaching producer shutdown) is covered below.

Traced and sound

• shutdown() ordering. The flag is set before any await, aborts go out first, and browser close runs concurrently with the bounded 2s render wait instead of after it. So a render that never settles can't hold Chrome open past preview's 3s watchdog. RENDER_SHUTDOWN_WAIT_MS (2s) sits above the pool's 1s force bound and under the watchdog, so the budget adds up.
• The pool close race. acquire() checks closed, awaits any drain, re-checks closed, then calls reserveEntry synchronously in the same tick. A close() can't land between the re-check and the reserve, so any entry it misses was already in drain()'s snapshot, and requestClose awaits its launchPromise. Nothing launches untracked.
• The thumbnail path.
  • The shutdown check comes before the cached-lease return and again inside the init IIFE, which covers a request queued behind an in-flight init.
  • closeThumbnailBrowser now awaits an init that was mid-launch before reading the lease.
  • A launch that got past the check either hits the closed pool (throws, returns null) or sets _thumbnailBrowserLease, which the awaiting close then releases.
• on instead of once in preview.ts is correct: shuttingDown makes a repeat signal a no-op instead of falling through to the default handler.
• The un-export of closeThumbnailBrowser. Its only external user was the inline cleanup this PR replaces.
• bun.lock. The only change is workspace versions 0.8.59 → 0.8.62, catching up to the package.json versions already on main.

On the MINOR (1s force bound reaching producer/server.ts). I checked its reach. drain() is called only from close() and drainBrowserPool(), and the only non-test caller of the latter is producer's SIGTERM shutdown(). So the bound only ever applies to an exiting process. Force-closing a Chrome after 1s in a pod that's terminating anyway loses nothing, so I agree it's not blocking. It would matter only if someone later calls drainBrowserPool() to recycle the pool mid-process. A one-line note on drain() that it's exit-only would guard against that.

Ran locally (Linux, full build): studioServer.test.ts 20/20, browserLeasePool.test.ts 8/8.

— Rames

[jrusso1020]

Re-approving at 949ccde8. The only change since my approval at 60215c59 is one commit that shortens the closeBrowserPool JSDoc in browserManager.ts (+1/-5, comment only, no code change). My earlier review stands.

— Rames