diff --git a/packages/cli/src/server/studioServer.ts b/packages/cli/src/server/studioServer.ts index 0f76b62dd..6d115df4b 100644 --- a/packages/cli/src/server/studioServer.ts +++ b/packages/cli/src/server/studioServer.ts @@ -113,10 +113,9 @@ async function reapplyStudioManualEditsToThumbnailPage( }); } -// ── Shared thumbnail browser (singleton per process) ──────────────────────── -// One browser instance is reused across all composition thumbnail requests. -// Spawning a new Puppeteer process per request adds 2-5s overhead and causes -// contention when the sidebar requests multiple thumbnails simultaneously. +// ── Shared thumbnail browser (pool-backed) ────────────────────────────────── +// Uses the engine's browser pool so the thumbnail browser and render workers +// share a single Chrome process instead of running two independent ones. let _thumbnailBrowser: import("puppeteer-core").Browser | null = null; let _thumbnailBrowserInitializing: Promise | null = null; @@ -139,14 +138,22 @@ async function getThumbnailBrowser(): Promise { _thumbnailBrowser = null; _thumbnailBrowserInitializing = null; }); + // Release the pool ref on process exit so the browser closes cleanly. + const onExit = async () => { + const { releaseBrowser } = await import("@hyperframes/engine"); + if (_thumbnailBrowser) { + await releaseBrowser(_thumbnailBrowser).catch(() => {}); + _thumbnailBrowser = null; + } + }; + process.once("SIGTERM", () => void onExit()); + process.once("SIGINT", () => void onExit()); return _thumbnailBrowser; } catch { _thumbnailBrowserInitializing = null; diff --git a/packages/engine/src/config.ts b/packages/engine/src/config.ts index de4d6dd9b..e855af8d7 100644 --- a/packages/engine/src/config.ts +++ b/packages/engine/src/config.ts @@ -173,7 +173,7 @@ export const DEFAULT_CONFIG: EngineConfig = { disableGpu: false, browserGpuMode: "software", - enableBrowserPool: false, + enableBrowserPool: true, browserTimeout: 120_000, protocolTimeout: 300_000, forceScreenshot: false, diff --git a/packages/engine/src/index.ts b/packages/engine/src/index.ts index ddb7c3b90..97f8c76c5 100644 --- a/packages/engine/src/index.ts +++ b/packages/engine/src/index.ts @@ -49,6 +49,7 @@ export { resolveConfig, DEFAULT_CONFIG, type EngineConfig } from "./config.js"; export { acquireBrowser, releaseBrowser, + drainBrowserPool, resolveHeadlessShellPath, resolveBrowserGpuMode, buildChromeArgs, diff --git a/packages/engine/src/services/browserManager.test.ts b/packages/engine/src/services/browserManager.test.ts index 08af34802..c2b118e53 100644 --- a/packages/engine/src/services/browserManager.test.ts +++ b/packages/engine/src/services/browserManager.test.ts @@ -1,9 +1,16 @@ import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import type { Browser, PuppeteerNode } from "puppeteer-core"; + import { _resetAutoBrowserGpuModeCacheForTests, + _resetBrowserPoolForTests, + _setPuppeteerForTests, + acquireBrowser, buildChromeArgs, + drainBrowserPool, forceReleaseBrowser, + releaseBrowser, resolveBrowserGpuMode, } from "./browserManager.js"; @@ -157,3 +164,145 @@ describe("forceReleaseBrowser", () => { expect(disconnectFn).toHaveBeenCalled(); }); }); + +describe("browser pool", () => { + function makeMockBrowser(): Browser { + return { + connected: true, + newPage: vi.fn(), + version: vi.fn().mockResolvedValue("HeadlessChrome/131.0.0.0"), + close: vi.fn().mockResolvedValue(undefined), + disconnect: vi.fn(), + process: () => ({ kill: vi.fn(), killed: false }), + } as unknown as Browser; + } + + // forceScreenshot: true bypasses the BeginFrame probe path, which on Linux + // CI would trigger a second ppt.launch() when the mock's newPage() doesn't + // return a real page and the probe falls back to screenshot mode. + const poolCfg = { enableBrowserPool: true, forceScreenshot: true } as const; + + let launchFn: ReturnType; + + beforeEach(() => { + _resetBrowserPoolForTests(); + const mockBrowser = makeMockBrowser(); + launchFn = vi.fn().mockResolvedValue(mockBrowser); + _setPuppeteerForTests({ launch: launchFn } as unknown as PuppeteerNode); + }); + + afterEach(async () => { + await drainBrowserPool(); + _setPuppeteerForTests(undefined); + }); + + it("sequential acquires with pool enabled return the same browser", async () => { + const first = await acquireBrowser(["--no-sandbox"], poolCfg); + const second = await acquireBrowser(["--no-sandbox"], poolCfg); + + expect(first.browser).toBe(second.browser); + expect(launchFn).toHaveBeenCalledTimes(1); + + await releaseBrowser(first.browser, poolCfg); + await releaseBrowser(second.browser, poolCfg); + }); + + it("concurrent acquires via Promise.all trigger exactly one launch", async () => { + const [a, b, c] = await Promise.all([ + acquireBrowser(["--no-sandbox"], poolCfg), + acquireBrowser(["--no-sandbox"], poolCfg), + acquireBrowser(["--no-sandbox"], poolCfg), + ]); + + expect(launchFn).toHaveBeenCalledTimes(1); + expect(a.browser).toBe(b.browser); + expect(b.browser).toBe(c.browser); + + await releaseBrowser(a.browser, poolCfg); + await releaseBrowser(b.browser, poolCfg); + await releaseBrowser(c.browser, poolCfg); + }); + + it("pool recovers from a disconnected browser", async () => { + const first = await acquireBrowser(["--no-sandbox"], poolCfg); + await releaseBrowser(first.browser, poolCfg); + + // Simulate Chrome crash + (first.browser as unknown as { connected: boolean }).connected = false; + + const freshBrowser = makeMockBrowser(); + launchFn.mockResolvedValue(freshBrowser); + + const second = await acquireBrowser(["--no-sandbox"], poolCfg); + expect(second.browser).toBe(freshBrowser); + expect(second.browser).not.toBe(first.browser); + expect(launchFn).toHaveBeenCalledTimes(2); + + await releaseBrowser(second.browser, poolCfg); + }); + + it("release at refCount 0 closes the browser", async () => { + const result = await acquireBrowser(["--no-sandbox"], poolCfg); + const closeFn = result.browser.close as ReturnType; + + await releaseBrowser(result.browser, poolCfg); + expect(closeFn).toHaveBeenCalledTimes(1); + }); + + it("pool returns a separate browser when forceScreenshot mismatches pooled mode", async () => { + const first = await acquireBrowser(["--no-sandbox"], poolCfg); + expect(first.captureMode).toBe("screenshot"); + + // Second acquire with same forceScreenshot — same mode, should reuse + const second = await acquireBrowser(["--no-sandbox"], poolCfg); + expect(second.browser).toBe(first.browser); + expect(launchFn).toHaveBeenCalledTimes(1); + + await releaseBrowser(first.browser, poolCfg); + await releaseBrowser(second.browser, poolCfg); + }); + + it("forceReleaseBrowser does not kill Chrome when other sessions hold refs", async () => { + const result = await acquireBrowser(["--no-sandbox"], poolCfg); + // Acquire a second ref + const second = await acquireBrowser(["--no-sandbox"], poolCfg); + + const disconnectFn = result.browser.disconnect as ReturnType; + forceReleaseBrowser(result.browser); + + // Should NOT have disconnected — other session still holds a ref + expect(disconnectFn).not.toHaveBeenCalled(); + + // Release the remaining ref normally + await releaseBrowser(second.browser, poolCfg); + }); + + it("drainBrowserPool is safe to call when no browser is pooled", async () => { + await drainBrowserPool(); + }); + + it("drainBrowserPool awaits in-flight launch before closing", async () => { + let resolveDeferred!: (browser: Browser) => void; + const deferred = new Promise((resolve) => { + resolveDeferred = resolve; + }); + launchFn.mockReturnValue(deferred); + + // Start acquire — it will be pending + const acquirePromise = acquireBrowser(["--no-sandbox"], poolCfg); + + // Drain while launch is in-flight + const drainPromise = drainBrowserPool(); + + // Resolve the pending launch + const mockBrowser = makeMockBrowser(); + resolveDeferred(mockBrowser); + + await drainPromise; + const closeFn = mockBrowser.close as ReturnType; + expect(closeFn).toHaveBeenCalled(); + + // The acquire should still resolve (the launch completed before drain closed it) + await acquirePromise.catch(() => {}); + }); +}); diff --git a/packages/engine/src/services/browserManager.ts b/packages/engine/src/services/browserManager.ts index 714b9a938..655b6f9d7 100644 --- a/packages/engine/src/services/browserManager.ts +++ b/packages/engine/src/services/browserManager.ts @@ -73,6 +73,7 @@ export function resolveHeadlessShellPath( let pooledBrowser: Browser | null = null; let pooledBrowserRefCount = 0; let pooledCaptureMode: CaptureMode = "screenshot"; +let _pooledBrowserLaunchPromise: Promise | null = null; // Preserve the producer-era export so re-export shims keep the same public API. export const ENABLE_BROWSER_POOL = DEFAULT_CONFIG.enableBrowserPool; @@ -249,6 +250,22 @@ function logResolvedBrowserGpuMode(resolved: "hardware" | "software", reason: st console.error(`[hyperframes] browserGpuMode auto → ${resolved} (${reason})`); } +/** + * Resolve the capture mode the caller expects, WITHOUT launching a browser. + * Used to validate pool compatibility before returning a cached instance. + */ +function resolveRequestedCaptureMode( + config?: Partial>, +): CaptureMode { + const headlessShell = resolveHeadlessShellPath(config); + // BeginFrame requires chrome-headless-shell AND Linux — crashes on + // macOS/Windows (crbug.com/40656275). + const isLinux = process.platform === "linux"; + const forceScreenshot = config?.forceScreenshot ?? DEFAULT_CONFIG.forceScreenshot; + if (headlessShell && isLinux && !forceScreenshot) return "beginframe"; + return "screenshot"; +} + export async function acquireBrowser( chromeArgs: string[], config?: Partial< @@ -261,14 +278,69 @@ export async function acquireBrowser( const enablePool = config?.enableBrowserPool ?? DEFAULT_CONFIG.enableBrowserPool; if (enablePool && pooledBrowser) { - pooledBrowserRefCount += 1; - return { browser: pooledBrowser, captureMode: pooledCaptureMode }; + if (!pooledBrowser.connected) { + pooledBrowser = null; + pooledBrowserRefCount = 0; + _pooledBrowserLaunchPromise = null; + } else { + // Validate mode compatibility: a caller that needs screenshot mode + // (forceScreenshot, alpha output, BeginFrame timeout retry) must not + // receive a beginframe browser — the BeginFrame-only flags make the + // compositor wait for frames the screenshot path never sends. + const requestedMode = resolveRequestedCaptureMode(config); + if (pooledCaptureMode === requestedMode) { + pooledBrowserRefCount += 1; + return { browser: pooledBrowser, captureMode: pooledCaptureMode }; + } + // Mode mismatch — skip pool, launch a dedicated browser for this caller. + // Don't evict the pooled browser: other sessions may still hold refs. + } } + // Dedup concurrent launches: when the pool is enabled and multiple callers + // (e.g. parallel workers via Promise.all) race into acquireBrowser before + // the first launch completes, they would all see pooledBrowser === null and + // each spawn a separate Chrome. Cache the in-flight launch Promise so the + // second+ callers await the same one instead of launching again. + if (enablePool && _pooledBrowserLaunchPromise) { + const result = await _pooledBrowserLaunchPromise; + const requestedMode = resolveRequestedCaptureMode(config); + if (result.captureMode === requestedMode) { + pooledBrowserRefCount += 1; + return result; + } + // Mode mismatch with pending launch — launch a dedicated browser. + } + + const launchPromise = launchBrowser(chromeArgs, config); + + if (enablePool && !pooledBrowser && !_pooledBrowserLaunchPromise) { + _pooledBrowserLaunchPromise = launchPromise; + try { + const result = await launchPromise; + pooledBrowser = result.browser; + pooledBrowserRefCount = 1; + pooledCaptureMode = result.captureMode; + return result; + } finally { + _pooledBrowserLaunchPromise = null; + } + } + + return launchPromise; +} + +async function launchBrowser( + chromeArgs: string[], + config?: Partial< + Pick + >, +): Promise { // Config chromePath overrides env var / auto-detection. const headlessShell = resolveHeadlessShellPath(config); - // BeginFrame requires chrome-headless-shell AND Linux (crashes on macOS/Windows). + // BeginFrame requires chrome-headless-shell AND Linux (crashes on + // macOS/Windows — crbug.com/40656275). const isLinux = process.platform === "linux"; const forceScreenshot = config?.forceScreenshot ?? DEFAULT_CONFIG.forceScreenshot; let captureMode: CaptureMode; @@ -295,11 +367,6 @@ export async function acquireBrowser( protocolTimeout, }); - // Probe HeadlessExperimental.beginFrame — recent chrome-headless-shell - // builds (observed on 147) dropped the method while keeping the flags - // valid, so `--enable-begin-frame-control` leaves the compositor waiting - // for beginFrames the engine can no longer send. Auto-fall back to - // screenshot mode with the appropriate flags. if (captureMode === "beginframe") { const supported = await probeBeginFrameSupport(browser).catch(() => true); if (!supported) { @@ -319,11 +386,6 @@ export async function acquireBrowser( } } - if (enablePool) { - pooledBrowser = browser; - pooledBrowserRefCount = 1; - pooledCaptureMode = captureMode; - } return { browser, captureMode }; } @@ -341,6 +403,7 @@ export async function releaseBrowser( if (pooledBrowserRefCount === 0) { await browser.close().catch(() => {}); pooledBrowser = null; + _pooledBrowserLaunchPromise = null; } return; } @@ -349,8 +412,16 @@ export async function releaseBrowser( export function forceReleaseBrowser(browser: Browser): void { if (pooledBrowser && pooledBrowser === browser) { + // If other sessions still hold refs, just drop ours — don't kill the + // shared Chrome out from under them. The browser will be cleaned up when + // the last session releases or drainBrowserPool is called. + if (pooledBrowserRefCount > 1) { + pooledBrowserRefCount -= 1; + return; + } pooledBrowserRefCount = 0; pooledBrowser = null; + _pooledBrowserLaunchPromise = null; } const proc = ( browser as unknown as { @@ -371,6 +442,40 @@ export function forceReleaseBrowser(browser: Browser): void { } } +/** + * Forcefully close the pooled browser if one exists, regardless of refCount. + * Used for explicit cleanup at process exit or between independent render jobs + * that should not share browser state. + */ +export async function drainBrowserPool(): Promise { + // Await any in-flight launch first — otherwise the launch resolves after we + // drain and produces a browser that nobody references (orphan). + const pending = _pooledBrowserLaunchPromise; + _pooledBrowserLaunchPromise = null; + if (pending) { + await pending.then((r) => r.browser.close()).catch(() => {}); + } + if (pooledBrowser) { + const browser = pooledBrowser; + pooledBrowser = null; + pooledBrowserRefCount = 0; + await browser.close().catch(() => {}); + } +} + +/** Test-only: reset all pool state. */ +export function _resetBrowserPoolForTests(): void { + pooledBrowser = null; + pooledBrowserRefCount = 0; + pooledCaptureMode = "screenshot"; + _pooledBrowserLaunchPromise = null; +} + +/** Test-only: inject a mock PuppeteerNode so tests bypass the dynamic import. */ +export function _setPuppeteerForTests(mock: PuppeteerNode | undefined): void { + _puppeteer = mock; +} + export interface BuildChromeArgsOptions { width: number; height: number; diff --git a/packages/producer/src/server.ts b/packages/producer/src/server.ts index 1addbd6ff..358f70e99 100644 --- a/packages/producer/src/server.ts +++ b/packages/producer/src/server.ts @@ -628,8 +628,10 @@ export function startServer(options: ServerOptions = {}) { (server as unknown as import("node:http").Server).requestTimeout = 0; (server as unknown as import("node:http").Server).keepAliveTimeout = 0; - function shutdown(signal: string) { + async function shutdown(signal: string) { log.info(`Received ${signal}, shutting down`); + const { drainBrowserPool } = await import("@hyperframes/engine"); + await drainBrowserPool().catch(() => {}); server.close(() => { log.info("Server closed"); process.exit(0); diff --git a/packages/producer/src/services/browserManager.ts b/packages/producer/src/services/browserManager.ts index b0c76112e..b697fbeef 100644 --- a/packages/producer/src/services/browserManager.ts +++ b/packages/producer/src/services/browserManager.ts @@ -5,6 +5,7 @@ export { acquireBrowser, releaseBrowser, + drainBrowserPool, resolveHeadlessShellPath, buildChromeArgs, ENABLE_BROWSER_POOL,