fix(engine): gate software-GPU warn + thread resolved mode (staff review)

Three fixes from PR #822 self-review:

1. Gate the "Software GPU detected" console.warn so it only fires when the
   guard actually changed the outcome. Previously the warn condition
   (captureMode==="screenshot" && isSoftwareRenderer && headlessShell &&
   isLinux) was satisfied even when the caller had explicitly set
   forceScreenshot=true — misleading operators into thinking the guard
   kicked in when they had already picked screenshot mode. Add
   `!forceScreenshot` to the condition and one-shot the warn with a
   process-level latch to avoid spam across multi-worker renders.

2. Thread the already-resolved browserGpuMode from frameCapture.ts into
   acquireBrowser via a new AcquireBrowserOptions.resolvedBrowserGpuMode.
   The Promise cache made the duplicate resolution cheap, but threading
   the value removes the smell of two parallel resolutions of the same
   thing with no static guarantee they agree under future refactors.

3. Add fallback-launch-retry test: simulate probeBeginFrameSupport
   failure via a CDP mock whose `send` rejects, assert the second launch
   strips beginframe-only flags. Also add positive-control tests for
   "Linux + hardware + headless-shell does NOT warn" and "forceScreenshot
   suppresses the software-GPU warn".

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Author: vanceingalls

packages/engine/src/services/browserManager.test.ts

@@ -170,6 +170,9 @@ describe("acquireBrowser — software-renderer guard", () => {
     lastLaunchArgs = undefined;
     lastLaunchExecutablePath = undefined;
     Object.defineProperty(process, "platform", { value: "linux", configurable: true });
+    // `vi.resetModules()` already gives each test a fresh `browserManager.js`
+    // module — so `_softwareGuardWarned` starts at false per test. No
+    // additional reset needed.
   });
 
   afterEach(() => {
@@ -281,6 +284,102 @@ describe("acquireBrowser — software-renderer guard", () => {
     const messages = warnSpy.mock.calls.map((c) => String(c[0]));
     expect(messages.find((m) => m.includes("Software GPU detected"))).toBeUndefined();
   });
+
+  it("does NOT emit the software-GPU warning when caller explicitly set forceScreenshot=true", async () => {
+    // If the caller already picked screenshot mode, the software-GPU guard
+    // didn't change the outcome — warning would misleadingly tell operators
+    // the guard kicked in. Same shape as the macOS test, but here the
+    // platform satisfies all the original warn-firing preconditions and only
+    // forceScreenshot suppresses the warn.
+    installPuppeteerMock();
+    const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {});
+    const { acquireBrowser: acquire, releaseBrowser: release } =
+      await import("./browserManager.js");
+
+    const acquired = await acquire(["--no-sandbox"], {
+      chromePath: "/fake/chrome-headless-shell",
+      browserGpuMode: "software",
+      forceScreenshot: true,
+    });
+    await release(acquired.browser);
+
+    const messages = warnSpy.mock.calls.map((c) => String(c[0]));
+    expect(messages.find((m) => m.includes("Software GPU detected"))).toBeUndefined();
+  });
+
+  it("does NOT emit the software-GPU warning on Linux + hardware + headless-shell (positive control)", async () => {
+    // On the very env where the guard COULD fire (Linux + headless-shell), a
+    // hardware-resolved GPU mode must NOT trip the warning. Catches the
+    // failure mode where the gate condition gets inverted in a refactor.
+    installPuppeteerMock();
+    const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {});
+    const { acquireBrowser: acquire, releaseBrowser: release } =
+      await import("./browserManager.js");
+
+    const acquired = await acquire(["--no-sandbox"], {
+      chromePath: "/fake/chrome-headless-shell",
+      browserGpuMode: "hardware",
+    });
+    await release(acquired.browser);
+
+    const messages = warnSpy.mock.calls.map((c) => String(c[0]));
+    expect(messages.find((m) => m.includes("Software GPU detected"))).toBeUndefined();
+  });
+
+  it("falls back to screenshot mode + strips beginframe flags when probeBeginFrameSupport fails", async () => {
+    // Pre-existing defense-in-depth: even if the software-GPU guard misses
+    // (e.g. resolveBrowserGpuMode returns "hardware" but the binary itself
+    // has dropped HeadlessExperimental.beginFrame — observed on chrome
+    // builds 147+), the post-launch probe should detect it, close the
+    // browser, and re-launch with stripped flags. This test pins that path
+    // via a CDP session whose `send` rejects.
+    const launchCalls: Array<{ args: string[]; executablePath?: string }> = [];
+    const browserStub = {
+      close: vi.fn().mockResolvedValue(undefined),
+      newPage: vi.fn().mockResolvedValue({
+        close: vi.fn().mockResolvedValue(undefined),
+        createCDPSession: vi.fn().mockResolvedValue({
+          // Probe failure: HeadlessExperimental.enable rejects → probe returns
+          // false → acquireBrowser closes browser + re-launches with stripped
+          // flags.
+          send: vi.fn().mockRejectedValue(new Error("HeadlessExperimental not supported")),
+          detach: vi.fn().mockResolvedValue(undefined),
+        }),
+      }),
+    };
+    const launch = vi.fn(async (opts: { args: string[]; executablePath?: string }) => {
+      launchCalls.push({ args: [...opts.args], executablePath: opts.executablePath });
+      return browserStub;
+    });
+    vi.doMock("puppeteer", () => ({ default: { launch } }));
+    vi.doMock("puppeteer-core", () => ({ default: { launch } }));
+
+    const { acquireBrowser: acquire } = await import("./browserManager.js");
+
+    const chromeArgs = [
+      "--no-sandbox",
+      "--enable-begin-frame-control",
+      "--deterministic-mode",
+      "--run-all-compositor-stages-before-draw",
+    ];
+    const result = await acquire(chromeArgs, {
+      chromePath: "/fake/chrome-headless-shell",
+      browserGpuMode: "hardware",
+    });
+
+    // Two launches: first with beginframe flags (initial attempt), then
+    // re-launch with flags stripped after probe failure.
+    expect(launchCalls.length).toBe(2);
+    expect(launchCalls[0]?.args).toContain("--enable-begin-frame-control");
+    expect(launchCalls[1]?.args).not.toContain("--enable-begin-frame-control");
+    expect(launchCalls[1]?.args).not.toContain("--deterministic-mode");
+    expect(launchCalls[1]?.args).not.toContain("--run-all-compositor-stages-before-draw");
+    expect(launchCalls[1]?.args).toContain("--no-sandbox");
+    // First browser closed before the re-launch.
+    expect(browserStub.close).toHaveBeenCalled();
+    // Final captureMode is screenshot, reflecting the fallback.
+    expect(result.captureMode).toBe("screenshot");
+  });
 });
 
 describe("forceReleaseBrowser", () => {

packages/engine/src/services/browserManager.ts

@@ -249,6 +249,29 @@ function logResolvedBrowserGpuMode(resolved: "hardware" | "software", reason: st
   console.error(`[hyperframes] browserGpuMode auto → ${resolved} (${reason})`);
 }
 
+/**
+ * One-shot latch for the "Software GPU detected" warning. The guard fires per
+ * acquire() (each render worker calls acquireBrowser), so without a latch we'd
+ * spam the operator with N copies of the same message on a multi-worker run.
+ * Exported reset for tests.
+ */
+let _softwareGuardWarned = false;
+export function _resetSoftwareGuardWarnedForTests(): void {
+  _softwareGuardWarned = false;
+}
+
+export interface AcquireBrowserOptions {
+  /**
+   * If the caller already resolved `browserGpuMode` (e.g. `frameCapture.ts`
+   * computes it before building chromeArgs), pass the resolved value here so
+   * `acquireBrowser` doesn't redundantly re-resolve from the raw config. The
+   * Promise cache makes the duplicate call cheap, but threading the value
+   * through removes the smell of two parallel resolutions of the same thing
+   * with no static guarantee they agree.
+   */
+  resolvedBrowserGpuMode?: "software" | "hardware";
+}
+
 export async function acquireBrowser(
   chromeArgs: string[],
   config?: Partial<
@@ -262,6 +285,7 @@ export async function acquireBrowser(
       | "browserGpuMode"
     >
   >,
+  options: AcquireBrowserOptions = {},
 ): Promise<AcquiredBrowser> {
   const enablePool = config?.enableBrowserPool ?? DEFAULT_CONFIG.enableBrowserPool;
 
@@ -280,20 +304,27 @@ export async function acquireBrowser(
   // Resolve browserGpuMode. On software-renderer hosts (no hardware GPU /
   // SwiftShader) HeadlessExperimental.beginFrame is unreliable — the
   // compositor stalls indefinitely on shader-heavy frames and the CDP call
-  // times out at protocolTimeout (default 5 min). Force screenshot mode for
-  // the entire process whenever resolveBrowserGpuMode resolves to "software",
-  // independent of platform/binary. Broader than the per-stage HDR guard in
-  // the producer (captureHdrStage's `cfg.forceScreenshot = true`) — that
-  // guard only covered the layered-composite path, not the BeginFrame loop
-  // for SDR content on a software host.
+  // times out at protocolTimeout (default 5 min). Force screenshot mode
+  // whenever resolveBrowserGpuMode resolves to "software", independent of
+  // platform/binary. This is a separate defense from the producer-side
+  // `captureHdrStage` guard (which unconditionally forces screenshot for the
+  // HDR layered-composite path); this guard covers the SDR-render-on-software-
+  // host case that captureHdrStage doesn't touch.
   //
-  // resolveBrowserGpuMode caches its probe Promise for the process lifetime,
-  // so calling it here is cheap after the first invocation.
-  const browserGpuMode = config?.browserGpuMode ?? DEFAULT_CONFIG.browserGpuMode;
-  const resolvedGpuMode = await resolveBrowserGpuMode(browserGpuMode, {
-    chromePath: headlessShell ?? undefined,
-    browserTimeout: config?.browserTimeout,
-  });
+  // If the caller has already resolved the mode (frameCapture.ts does) it
+  // hands us the value via options.resolvedBrowserGpuMode to avoid a second
+  // resolution from raw config. The probe Promise is cached for the process
+  // lifetime so the fallback path is still cheap.
+  let resolvedGpuMode: "software" | "hardware";
+  if (options.resolvedBrowserGpuMode) {
+    resolvedGpuMode = options.resolvedBrowserGpuMode;
+  } else {
+    const browserGpuMode = config?.browserGpuMode ?? DEFAULT_CONFIG.browserGpuMode;
+    resolvedGpuMode = await resolveBrowserGpuMode(browserGpuMode, {
+      chromePath: headlessShell ?? undefined,
+      browserTimeout: config?.browserTimeout,
+    });
+  }
   const isSoftwareRenderer = resolvedGpuMode === "software";
 
   let captureMode: CaptureMode;
@@ -315,11 +346,14 @@ export async function acquireBrowser(
   // them defensively. No-op if caller already built args for screenshot mode.
   const launchArgs = captureMode === "screenshot" ? stripBeginFrameFlags(chromeArgs) : chromeArgs;
 
-  if (captureMode === "screenshot" && isSoftwareRenderer && headlessShell && isLinux) {
-    // Log once when the software-renderer guard actually changed the outcome
-    // (Linux + headless-shell available — the only env where beginframe would
-    // have been the default). Silent on the always-screenshot platforms
-    // (macOS, Windows) and on hardware hosts.
+  // Warn ONLY when the software-renderer guard actually changed the outcome.
+  // Conditions: Linux + headless-shell available + GPU resolved to software +
+  // caller didn't already pick screenshot via forceScreenshot. (If the caller
+  // explicitly set forceScreenshot=true the guard didn't change anything, so
+  // warning would mislead operators into thinking the guard kicked in.)
+  // One-shot per process to avoid spamming multi-worker renders.
+  if (!forceScreenshot && isSoftwareRenderer && headlessShell && isLinux && !_softwareGuardWarned) {
+    _softwareGuardWarned = true;
     console.warn(
       "[BrowserManager] Software GPU detected; forcing screenshot capture mode (HeadlessExperimental.beginFrame stalls on software-rendered compositors).",
     );

packages/engine/src/services/frameCapture.ts

@@ -213,7 +213,12 @@ export async function createCaptureSession(
     { ...config, browserGpuMode: resolvedGpuMode },
   );
 
-  const { browser, captureMode } = await acquireBrowser(chromeArgs, config);
+  // Thread the already-resolved GPU mode into acquireBrowser so it doesn't
+  // re-resolve from raw config. Promise-cached anyway, but removes the smell
+  // of two parallel resolutions that future refactors could let diverge.
+  const { browser, captureMode } = await acquireBrowser(chromeArgs, config, {
+    resolvedBrowserGpuMode: resolvedGpuMode,
+  });
 
   const page = await browser.newPage();
   // Polyfill esbuild's keepNames helper inside the page.