From 72daac2a1d6785749e78f838114aa1283037f933 Mon Sep 17 00:00:00 2001 From: Vance Ingalls Date: Tue, 14 Jul 2026 03:06:34 +0000 Subject: [PATCH] fix(engine): carry programmatic forceScreenshot opt-out to concrete-resolved site MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Miguel R3 blocker on #2359: the runtime helper only checked the env opt-out (PRODUCER_FORCE_SCREENSHOT=false), silently defeating the documented programmatic escape hatch (overrides.forceScreenshot === false) on the browserGpuMode:'auto' → software probe path. At the concrete-resolution site the boolean forceScreenshot === false is ambiguous between default and explicit opt-out — resolveConfig sees the provenance but the runtime helper does not. Fix: persist provenance on the resolved config. - New INTERNAL EngineConfig field forceScreenshotExplicitlyOptedOut, set by resolveConfig when EITHER env or programmatic explicit-false is present. Purpose-documented in the type as 'not intended to be set by callers'. - shouldClampToScreenshotForConcreteGpu gains an opts.programmaticOptOut parameter; returns false early when set. Env stays as the third arg (backward compatibility with existing tests). - frameCapture.ts and renderOrchestrator.ts pass config.forceScreenshotExplicitlyOptedOut through at both call sites, so the auto→software probe path preserves the same escape hatches as literal browserGpuMode:'software'. New tests: 5 additional cases across the helper (programmatic opt-out alone; programmatic beats missing env) and resolveConfig provenance (programmatic sets flag; env sets flag; neither leaves it undefined). Local: 61/61 engine config tests pass (was 56). --- packages/engine/src/config.test.ts | 48 +++++++++++++++++++ packages/engine/src/config.ts | 34 +++++++++++-- packages/engine/src/services/frameCapture.ts | 11 ++++- .../src/services/renderOrchestrator.ts | 7 ++- 4 files changed, 92 insertions(+), 8 deletions(-) diff --git a/packages/engine/src/config.test.ts b/packages/engine/src/config.test.ts index 6ab586993..5e4fa4108 100644 --- a/packages/engine/src/config.test.ts +++ b/packages/engine/src/config.test.ts @@ -395,6 +395,54 @@ describe("resolveConfig", () => { expect(shouldClampToScreenshotForConcreteGpu("software", false, env)).toBe(true); } }); + + it("honors the programmatic opt-out via opts.programmaticOptOut on software", () => { + // The auto→software probe path is what this really guards: `resolveConfig` + // sets `forceScreenshotExplicitlyOptedOut = true` when the caller passed + // `overrides.forceScreenshot === false`, and the helper reads it here so + // the concrete-resolution route matches the config-time behavior. + expect( + shouldClampToScreenshotForConcreteGpu("software", false, {} as NodeJS.ProcessEnv, { + programmaticOptOut: true, + }), + ).toBe(false); + }); + + it("programmatic opt-out beats a missing env opt-out (both escape hatches independent)", () => { + // Even with no env opt-out set, a programmatic opt-out preserves BeginFrame- + // on-software debugging on the auto→software probe path. + expect( + shouldClampToScreenshotForConcreteGpu( + "software", + false, + { PRODUCER_FORCE_SCREENSHOT: "true" } as NodeJS.ProcessEnv, + { programmaticOptOut: true }, + ), + ).toBe(false); + }); + }); + + describe("forceScreenshotExplicitlyOptedOut provenance", () => { + it("is set to true when programmatic override forceScreenshot=false is passed", () => { + setEnv("PRODUCER_BROWSER_GPU_MODE", "hardware"); + unsetEnv("PRODUCER_FORCE_SCREENSHOT"); + const config = resolveConfig({ forceScreenshot: false }); + expect(config.forceScreenshotExplicitlyOptedOut).toBe(true); + }); + + it("is set to true when env PRODUCER_FORCE_SCREENSHOT=false is set", () => { + setEnv("PRODUCER_BROWSER_GPU_MODE", "hardware"); + setEnv("PRODUCER_FORCE_SCREENSHOT", "false"); + const config = resolveConfig(); + expect(config.forceScreenshotExplicitlyOptedOut).toBe(true); + }); + + it("stays unset when neither opt-out is present (default)", () => { + setEnv("PRODUCER_BROWSER_GPU_MODE", "hardware"); + unsetEnv("PRODUCER_FORCE_SCREENSHOT"); + const config = resolveConfig(); + expect(config.forceScreenshotExplicitlyOptedOut).toBeUndefined(); + }); }); describe("lowMemoryMode", () => { diff --git a/packages/engine/src/config.ts b/packages/engine/src/config.ts index 98e95e23f..2f1805858 100644 --- a/packages/engine/src/config.ts +++ b/packages/engine/src/config.ts @@ -88,6 +88,19 @@ export interface EngineConfig { * opt-out. Not intended to be set by callers. */ pageSideCompositingAutoDisabled?: boolean; + /** + * INTERNAL. Set to `true` by `resolveConfig` when the caller explicitly + * opted out of the software-GPU→screenshot clamp — either via env + * `PRODUCER_FORCE_SCREENSHOT=false` or programmatic + * `overrides.forceScreenshot === false`. The concrete-resolved-GPU helper + * (`shouldClampToScreenshotForConcreteGpu`) reads this so the + * `browserGpuMode:"auto"` → software probe path preserves the same + * escape hatch as literal `browserGpuMode:"software"` (the boolean + * `forceScreenshot === false` at that point is otherwise ambiguous — + * default vs explicit opt-out — because the config resolves before + * the runtime probe fires). Not intended to be set by callers. + */ + forceScreenshotExplicitlyOptedOut?: boolean; /** * Low-memory render profile. When `true`, the orchestrator collapses the * pipeline to its cheapest shape on memory-constrained hosts: it skips the @@ -581,6 +594,12 @@ export function resolveConfig(overrides?: Partial): EngineConfig { // on-software debugging remains possible. const explicitForceScreenshotOptOut = env("PRODUCER_FORCE_SCREENSHOT") === "false" || overrides?.forceScreenshot === false; + // Persist provenance so the concrete-resolved-GPU helper can honor the + // programmatic opt-out too — at that point `forceScreenshot === false` is + // otherwise ambiguous between default and explicit opt-out. + if (explicitForceScreenshotOptOut) { + merged.forceScreenshotExplicitlyOptedOut = true; + } if ( merged.browserGpuMode === "software" && !merged.forceScreenshot && @@ -615,23 +634,30 @@ export function resolveConfig(overrides?: Partial): EngineConfig { * Runtime-resolved companion to the software-GPU screenshot clamp in * `resolveConfig`. Returns `true` iff callers should treat this render as * `forceScreenshot=true` even though the config's stored `forceScreenshot` - * is `false`. Fires when the concrete resolved GPU is software AND the - * env-level opt-out (`PRODUCER_FORCE_SCREENSHOT=false`) is NOT set. + * is `false`. Fires when the concrete resolved GPU is software AND neither + * the env opt-out (`PRODUCER_FORCE_SCREENSHOT=false`) nor the programmatic + * opt-out (`overrides.forceScreenshot === false`, carried via + * `cfg.forceScreenshotExplicitlyOptedOut`) is set. * * `resolveConfig`'s clamp only sees `browserGpuMode` as a string, so * `"auto"` that runtime-probes to software slips through. This helper * closes that gap at the concrete-resolution points (`frameCapture` and - * `renderOrchestrator`). Same invariant, same env opt-out, one predicate. + * `renderOrchestrator`). Same invariant, same escape hatches, one predicate. * * Callers should skip when the invariant is already satisfied - * (`currentForceScreenshot === true`) to avoid redundant work. + * (`currentForceScreenshot === true`) to avoid redundant work. Pass + * `cfg.forceScreenshotExplicitlyOptedOut` via `opts.programmaticOptOut` so + * the `browserGpuMode:"auto"` → software probe path honors the same + * programmatic escape hatch as literal `browserGpuMode:"software"`. */ export function shouldClampToScreenshotForConcreteGpu( resolvedGpuMode: "software" | "hardware", currentForceScreenshot: boolean, env: NodeJS.ProcessEnv = process.env, + opts: { programmaticOptOut?: boolean } = {}, ): boolean { if (currentForceScreenshot) return false; if (resolvedGpuMode !== "software") return false; + if (opts.programmaticOptOut) return false; return env["PRODUCER_FORCE_SCREENSHOT"] !== "false"; } diff --git a/packages/engine/src/services/frameCapture.ts b/packages/engine/src/services/frameCapture.ts index 8297a7b5e..e25e696c4 100644 --- a/packages/engine/src/services/frameCapture.ts +++ b/packages/engine/src/services/frameCapture.ts @@ -825,9 +825,16 @@ export async function createCaptureSession( // point too — `resolveConfig` can only see the pre-resolve `browserGpuMode` // string, so `"auto"` that probes to software would otherwise slip through // and launch BeginFrame + SwiftShader (the exact combination the invariant - // is meant to prevent). Env-level opt-out preserved via the shared helper. + // is meant to prevent). Both env and programmatic opt-outs preserved via + // the shared helper (the programmatic one carried on the config as + // `forceScreenshotExplicitlyOptedOut`, since at this point the boolean + // `forceScreenshot === false` is otherwise ambiguous between default and + // explicit opt-out). const effectiveForceScreenshot = - forceScreenshot || shouldClampToScreenshotForConcreteGpu(resolvedGpuMode, forceScreenshot); + forceScreenshot || + shouldClampToScreenshotForConcreteGpu(resolvedGpuMode, forceScreenshot, process.env, { + programmaticOptOut: config?.forceScreenshotExplicitlyOptedOut ?? false, + }); const preMode: CaptureMode = headlessShell && isLinux && diff --git a/packages/producer/src/services/renderOrchestrator.ts b/packages/producer/src/services/renderOrchestrator.ts index ee1a5b77b..b707f1976 100644 --- a/packages/producer/src/services/renderOrchestrator.ts +++ b/packages/producer/src/services/renderOrchestrator.ts @@ -1914,13 +1914,16 @@ export async function executeRenderJob( // that probe to software: `resolveConfig` couldn't see this at config // time, so `captureObservability.forceScreenshot` was still false, // misreporting `captureMode: "beginframe"` for a session that will - // actually take the screenshot path. Env-level opt-out preserved via - // the shared helper. + // actually take the screenshot path. Both env and programmatic opt-outs + // preserved via the shared helper (the programmatic one carried on the + // config as `forceScreenshotExplicitlyOptedOut`). const observabilityForceScreenshot = captureObservability.forceScreenshot || shouldClampToScreenshotForConcreteGpu( resolvedBrowserGpuMode, captureObservability.forceScreenshot, + process.env, + { programmaticOptOut: cfg.forceScreenshotExplicitlyOptedOut ?? false }, ); updateCaptureObservability({ browserGpuMode: resolvedBrowserGpuMode,