From f44bc3a5253af417705b9a373a9f0338a58dae08 Mon Sep 17 00:00:00 2001 From: Vance Ingalls Date: Tue, 14 Jul 2026 03:17:29 +0000 Subject: [PATCH] fix(engine,producer): drive forceScreenshot from one authoritative local MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Miguel R4 blocker on #2359: my R3 fix at renderOrchestrator only updated the observability copy, leaving the authoritative captureForceScreenshot local at compileResult.forceScreenshot (false for auto→software). The frameCapture side clamped its own local and correctly routed screenshot, but downstream orchestrator code overwrote observability back to beginframe from the still-false local at two sites: - Parallel-stream label at renderOrchestrator.ts:2293 mis-labelled the stream as 'beginframe' when actual capture was 'screenshot'. - capture_strategy telemetry at renderOrchestrator.ts:2440-2450 overwrote the earlier observability correction, so the final captureMode observation flipped back to 'beginframe' while the engine actually captured screenshot. Fix: extract the clamp into a caller-facing helper applyConcreteGpuScreenshotClamp(current, resolvedGpuMode, cfg) that returns the (possibly-promoted) new boolean. Callers assign it back to their authoritative local, so routing + telemetry + strategy code read one value. Changes: - packages/engine/src/config.ts: new exported applyConcreteGpuScreenshotClamp; delegates to shouldClampToScreenshotForConcreteGpu but computes the caller's final value, not just the clamp decision. Reads the programmatic opt-out from cfg.forceScreenshotExplicitlyOptedOut. Idempotent on already-true input. - packages/engine/src/index.ts: export the new helper. - packages/engine/src/services/frameCapture.ts: replace the inline OR expression with applyConcreteGpuScreenshotClamp. - packages/producer/src/services/renderOrchestrator.ts: assign result into the AUTHORITATIVE captureForceScreenshot local (was updating only observability). Downstream parallel-stream label at :2293 and capture_strategy telemetry at :2440-2450 now read the corrected value. Tests: 6 new caller-level cases for applyConcreteGpuScreenshotClamp covering the exact matrix Miguel called out: - resolved software + default false → promotes to true (screenshot) - resolved software + programmatic opt-out → stays false (BeginFrame) - resolved hardware + default false → stays false - resolved software + already-true → stays true (idempotent) - resolved software + env PRODUCER_FORCE_SCREENSHOT=false → stays false - resolved software + undefined cfg → promotes to true (frameCapture path) Local: 67/67 engine config tests pass (was 61). oxfmt clean. --- packages/engine/src/config.test.ts | 73 +++++++++++++++++++ packages/engine/src/config.ts | 28 +++++++ packages/engine/src/index.ts | 1 + packages/engine/src/services/frameCapture.ts | 24 +++--- .../src/services/renderOrchestrator.ts | 36 ++++----- 5 files changed, 131 insertions(+), 31 deletions(-) diff --git a/packages/engine/src/config.test.ts b/packages/engine/src/config.test.ts index 5e4fa4108..383b7f4b4 100644 --- a/packages/engine/src/config.test.ts +++ b/packages/engine/src/config.test.ts @@ -6,7 +6,9 @@ import { DEFAULT_CONFIG, scaleProtocolTimeoutForComposition, shouldClampToScreenshotForConcreteGpu, + applyConcreteGpuScreenshotClamp, } from "./config.js"; +import type { EngineConfig } from "./config.js"; import { isLowMemorySystem } from "./services/systemMemory.js"; describe("resolveConfig", () => { @@ -422,6 +424,77 @@ describe("resolveConfig", () => { }); }); + describe("applyConcreteGpuScreenshotClamp (caller-level contract)", () => { + // This is the helper both frameCapture.ts and renderOrchestrator.ts call + // to compute the value the AUTHORITATIVE `forceScreenshot` local should + // hold after the concrete GPU is resolved. Routing AND telemetry read + // from that one value, so this contract must hold across default and + // opt-out combinations. + type OptOutCfg = Pick; + const cleanEnv = {} as NodeJS.ProcessEnv; + + it("resolved software + default false → promotes to true (screenshot route)", () => { + // The core auto→software fix: routing AND downstream telemetry read + // the promoted value, so `updateCaptureObservability({ forceScreenshot: + // captureForceScreenshot })` at the capture_strategy site reports + // screenshot instead of overwriting back to beginframe. + expect(applyConcreteGpuScreenshotClamp(false, "software", {} as OptOutCfg, cleanEnv)).toBe( + true, + ); + }); + + it("resolved software + programmatic opt-out → stays false (BeginFrame preserved)", () => { + // The programmatic escape hatch caller-level contract: setting + // overrides.forceScreenshot=false must keep BeginFrame across BOTH + // routing (frameCapture) and telemetry (renderOrchestrator) — since + // resolveConfig lifts the flag onto the config, both callers converge. + expect( + applyConcreteGpuScreenshotClamp( + false, + "software", + { forceScreenshotExplicitlyOptedOut: true } as OptOutCfg, + cleanEnv, + ), + ).toBe(false); + }); + + it("resolved hardware + default false → stays false (no clamp needed)", () => { + expect(applyConcreteGpuScreenshotClamp(false, "hardware", {} as OptOutCfg, cleanEnv)).toBe( + false, + ); + }); + + it("resolved software + already-true forceScreenshot → stays true (idempotent)", () => { + // Config-time clamp already fired (literal browserGpuMode:"software"), + // so re-applying at the concrete-resolved site is a no-op. + expect(applyConcreteGpuScreenshotClamp(true, "software", {} as OptOutCfg, cleanEnv)).toBe( + true, + ); + }); + + it("resolved software + env PRODUCER_FORCE_SCREENSHOT=false → stays false", () => { + // Env opt-out preserved even when programmatic flag is not set (some + // callers, like debugging BeginFrame-on-software from CI, opt-out via + // env only). + expect( + applyConcreteGpuScreenshotClamp( + false, + "software", + {} as OptOutCfg, + { + PRODUCER_FORCE_SCREENSHOT: "false", + } as NodeJS.ProcessEnv, + ), + ).toBe(false); + }); + + it("resolved software + undefined cfg → default (no programmatic opt-out) → clamps to true", () => { + // Sanity: frameCapture.ts calls with `config` possibly undefined. + // Default case must still promote. + expect(applyConcreteGpuScreenshotClamp(false, "software", undefined, cleanEnv)).toBe(true); + }); + }); + describe("forceScreenshotExplicitlyOptedOut provenance", () => { it("is set to true when programmatic override forceScreenshot=false is passed", () => { setEnv("PRODUCER_BROWSER_GPU_MODE", "hardware"); diff --git a/packages/engine/src/config.ts b/packages/engine/src/config.ts index 2f1805858..bb12af59a 100644 --- a/packages/engine/src/config.ts +++ b/packages/engine/src/config.ts @@ -661,3 +661,31 @@ export function shouldClampToScreenshotForConcreteGpu( if (opts.programmaticOptOut) return false; return env["PRODUCER_FORCE_SCREENSHOT"] !== "false"; } + +/** + * Caller-facing pair to `shouldClampToScreenshotForConcreteGpu`: computes the + * value the *authoritative* `forceScreenshot` local should hold after the + * concrete-resolved-GPU decision fires. Returns the (possibly-promoted) new + * boolean, so the caller can assign it back to its local — driving both + * routing AND telemetry from one source of truth. + * + * Reads the programmatic opt-out from `cfg.forceScreenshotExplicitlyOptedOut` + * (set by `resolveConfig` when EITHER env `PRODUCER_FORCE_SCREENSHOT=false` + * OR programmatic `overrides.forceScreenshot === false` was present). + * + * Idempotent: `applyConcreteGpuScreenshotClamp(true, ...)` returns `true` + * without consulting anything else. + */ +export function applyConcreteGpuScreenshotClamp( + currentForceScreenshot: boolean, + resolvedGpuMode: "software" | "hardware", + cfg: Pick | undefined, + env: NodeJS.ProcessEnv = process.env, +): boolean { + return ( + currentForceScreenshot || + shouldClampToScreenshotForConcreteGpu(resolvedGpuMode, currentForceScreenshot, env, { + programmaticOptOut: cfg?.forceScreenshotExplicitlyOptedOut ?? false, + }) + ); +} diff --git a/packages/engine/src/index.ts b/packages/engine/src/index.ts index 47ce1cf54..72cbf3871 100644 --- a/packages/engine/src/index.ts +++ b/packages/engine/src/index.ts @@ -49,6 +49,7 @@ export { DEFAULT_CONFIG, scaleProtocolTimeoutForComposition, shouldClampToScreenshotForConcreteGpu, + applyConcreteGpuScreenshotClamp, type EngineConfig, } from "./config.js"; export { diff --git a/packages/engine/src/services/frameCapture.ts b/packages/engine/src/services/frameCapture.ts index e25e696c4..c264e5b70 100644 --- a/packages/engine/src/services/frameCapture.ts +++ b/packages/engine/src/services/frameCapture.ts @@ -43,11 +43,7 @@ import { produceDrawElementFrameBatch, } from "./drawElementService.js"; import { initThreeDProjection, detectCssEffectRisk } from "./threeDProjection.js"; -import { - DEFAULT_CONFIG, - shouldClampToScreenshotForConcreteGpu, - type EngineConfig, -} from "../config.js"; +import { DEFAULT_CONFIG, applyConcreteGpuScreenshotClamp, type EngineConfig } from "../config.js"; import type { CaptureOptions, CaptureVideoMetadataHint, @@ -826,15 +822,15 @@ export async function createCaptureSession( // string, so `"auto"` that probes to software would otherwise slip through // and launch BeginFrame + SwiftShader (the exact combination the invariant // 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, process.env, { - programmaticOptOut: config?.forceScreenshotExplicitlyOptedOut ?? false, - }); + // `applyConcreteGpuScreenshotClamp` (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 = applyConcreteGpuScreenshotClamp( + forceScreenshot, + resolvedGpuMode, + config, + ); const preMode: CaptureMode = headlessShell && isLinux && diff --git a/packages/producer/src/services/renderOrchestrator.ts b/packages/producer/src/services/renderOrchestrator.ts index b707f1976..b06d59f10 100644 --- a/packages/producer/src/services/renderOrchestrator.ts +++ b/packages/producer/src/services/renderOrchestrator.ts @@ -70,7 +70,7 @@ import { type SubTimelineWaitOutcome, resolveBrowserGpuMode, resolveHeadlessShellPath, - shouldClampToScreenshotForConcreteGpu, + applyConcreteGpuScreenshotClamp, scaleProtocolTimeoutForComposition, isMemoryExhaustionError, isTransientBrowserError, @@ -1909,25 +1909,27 @@ export async function executeRenderJob( chromePath: resolveHeadlessShellPath(cfg), browserTimeout: cfg.browserTimeout, }); - // Mirror the frameCapture.ts routing invariant here so observability - // reports the actual capture mode on `browserGpuMode: "auto"` renders - // 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. Both env and programmatic opt-outs - // preserved via the shared helper (the programmatic one carried on the + // Apply the software-GPU→screenshot clamp to the AUTHORITATIVE local + // `captureForceScreenshot` (not just the observability copy) so all + // downstream strategy + telemetry code reads the corrected value. + // Otherwise: `frameCapture.ts` clamps its own local and routes + // screenshot, but the still-`false` orchestrator local (a) mislabels the + // parallel-stream logging as "beginframe" below and (b) overwrites the + // earlier observability correction back to BeginFrame at the + // capture_strategy telemetry site. `resolveConfig` couldn't see + // `browserGpuMode:"auto"` resolving to software at config time, so + // `captureForceScreenshot` was still `compileResult.forceScreenshot === false` + // on that path. Both env and programmatic opt-outs preserved via + // `applyConcreteGpuScreenshotClamp` (the programmatic one carried on the // config as `forceScreenshotExplicitlyOptedOut`). - const observabilityForceScreenshot = - captureObservability.forceScreenshot || - shouldClampToScreenshotForConcreteGpu( - resolvedBrowserGpuMode, - captureObservability.forceScreenshot, - process.env, - { programmaticOptOut: cfg.forceScreenshotExplicitlyOptedOut ?? false }, - ); + captureForceScreenshot = applyConcreteGpuScreenshotClamp( + captureForceScreenshot, + resolvedBrowserGpuMode, + cfg, + ); updateCaptureObservability({ browserGpuMode: resolvedBrowserGpuMode, - forceScreenshot: observabilityForceScreenshot, + forceScreenshot: captureForceScreenshot, }); const videoCaptureBeyondViewport = resolveVideoCaptureBeyondViewport(composition.videos.length);