diff --git a/packages/engine/src/config.test.ts b/packages/engine/src/config.test.ts index ce683547e3..95ee5096b0 100644 --- a/packages/engine/src/config.test.ts +++ b/packages/engine/src/config.test.ts @@ -4,6 +4,7 @@ import { tmpdir } from "node:os"; import { resolveConfig, resolveDefaultDrawElement, + explainDrawElementDisabled, DEFAULT_CONFIG, scaleProtocolTimeoutForComposition, shouldClampToScreenshotForConcreteGpu, @@ -233,6 +234,85 @@ describe("resolveConfig", () => { }); }); + // Every resolveDefaultDrawElement branch returns a bare `false`, so a render + // that never became a DE candidate reached telemetry with no reason at all + // and landed in the dashboard's `other` bucket. These pin that each silent + // refusal now has a name, and that the names stay in the same ORDER as the + // resolver's branches — if the two drift, the reason is a plausible lie, + // which is worse than no reason. + describe("explainDrawElementDisabled (names the silent refusals)", () => { + const base = { browserGpuMode: "hardware" as const, workerEncode: true }; + + it("names each refusal", () => { + expect(explainDrawElementDisabled({ ...base, platform: "linux" })).toBe( + "unsupported_platform", + ); + expect( + explainDrawElementDisabled({ ...base, platform: "darwin", browserGpuMode: "software" }), + ).toBe("software_gpu"); + expect(explainDrawElementDisabled({ ...base, platform: "win32", workerEncode: false })).toBe( + "worker_encode_off", + ); + }); + + // The Windows case this shipped for: hardware GPU, supported platform, + // worker-encode on — nothing environmental explains it, so it was an + // explicit opt-out. Must NOT masquerade as one of the other three. + // The caller-side path: resolveDefaultDrawElement never even runs when the + // feature is off at a higher config level, so the orchestrator seeds from + // the environment alone and must land on `disabled` rather than inventing + // an environmental cause. + it("reports `disabled` for a config-time refusal on a healthy host", () => { + expect( + resolveDefaultDrawElement({ ...base, useDrawElement: false, platform: "darwin" }), + ).toBe(false); + expect(explainDrawElementDisabled({ ...base, platform: "darwin" })).toBe("disabled"); + }); + + it("falls back to `disabled` when nothing environmental explains it", () => { + expect(explainDrawElementDisabled({ ...base, platform: "win32" })).toBe("disabled"); + expect(explainDrawElementDisabled({ ...base, platform: "darwin" })).toBe("disabled"); + }); + + // Platform is checked BEFORE gpu mode, matching the resolver. A linux + // software host is reported as unsupported_platform, not software_gpu: + // fixing the GPU would not help. + it("orders platform ahead of gpu mode, like the resolver", () => { + expect( + explainDrawElementDisabled({ + platform: "linux", + browserGpuMode: "software", + workerEncode: false, + }), + ).toBe("unsupported_platform"); + }); + + // The contract that keeps the two functions honest: whenever the resolver + // says false, the explainer must produce a reason, and whenever it says + // true the caller must not ask. + it("covers every input where the resolver refuses", () => { + const platforms: NodeJS.Platform[] = ["darwin", "win32", "linux"]; + const gpuModes = ["hardware", "software", "auto"] as const; + for (const platform of platforms) { + for (const browserGpuMode of gpuModes) { + for (const workerEncode of [true, false]) { + const on = resolveDefaultDrawElement({ + useDrawElement: true, + explicitOptIn: false, + platform, + browserGpuMode, + workerEncode, + }); + if (on) continue; + expect(explainDrawElementDisabled({ platform, browserGpuMode, workerEncode })).not.toBe( + "disabled", + ); + } + } + } + }); + }); + describe("resolveDefaultDrawElement (pure host clamp)", () => { const base = { useDrawElement: true, diff --git a/packages/engine/src/config.ts b/packages/engine/src/config.ts index fb06796fe3..127a98ffc1 100644 --- a/packages/engine/src/config.ts +++ b/packages/engine/src/config.ts @@ -741,6 +741,39 @@ export function resolveDefaultDrawElement(args: { return args.workerEncode; } +/** + * Why {@link resolveDefaultDrawElement} said no. Call ONLY when the resolved + * `useDrawElement` is false — the branches mirror that resolver's, in order. + * + * Every branch there returns a bare `false` and records nothing, so a render + * that never became a drawElement candidate reaches telemetry with no + * `de_compile_gate`, no `de_clamp_reason` and no `de_gate_reason`. Those land + * in the "Why not drawElement" dashboard's catch-all `other` bucket, which + * measured 56,507 renders over 14 days — its second-largest bar, explaining + * nothing. The orchestrator's own clamp only fires while `useDrawElement` is + * still true, so it cannot cover a config-time refusal by construction. + * + * Takes only the environmental inputs on purpose: the caller holds the + * POST-resolution `useDrawElement`, from which the pre-resolution request is + * no longer recoverable. So "none of these three explain it" is itself the + * answer — the feature was switched off explicitly. + * + * Kept separate from the resolver rather than widening its return type: it sits + * on the config hot path and several call sites want a plain boolean. Mirror + * any branch change in both. + */ +export function explainDrawElementDisabled(args: { + platform: NodeJS.Platform; + browserGpuMode: EngineConfig["browserGpuMode"]; + workerEncode: boolean; +}): "unsupported_platform" | "software_gpu" | "worker_encode_off" | "disabled" { + // Platform first: on an unsupported host the GPU mode is beside the point. + if (!isDrawElementPlatform(args.platform)) return "unsupported_platform"; + if (args.browserGpuMode === "software") return "software_gpu"; + if (!args.workerEncode) return "worker_encode_off"; + return "disabled"; +} + export function resolveConfig(overrides?: Partial): EngineConfig { const env = (key: string): string | undefined => process.env[key]; const envNum = (key: string, fallback: number): number => { diff --git a/packages/engine/src/index.ts b/packages/engine/src/index.ts index b6fe19cdcf..fe29c8bfa9 100644 --- a/packages/engine/src/index.ts +++ b/packages/engine/src/index.ts @@ -53,6 +53,7 @@ export { scaleProtocolTimeoutForComposition, shouldClampToScreenshotForConcreteGpu, applyConcreteGpuScreenshotClamp, + explainDrawElementDisabled, resolveExtractCacheDir, defaultExtractCacheDir, EXTRACT_CACHE_DIR_DISABLED_ALIASES, diff --git a/packages/producer/src/services/renderOrchestrator.test.ts b/packages/producer/src/services/renderOrchestrator.test.ts index d5283ab356..93c04e51d0 100644 --- a/packages/producer/src/services/renderOrchestrator.test.ts +++ b/packages/producer/src/services/renderOrchestrator.test.ts @@ -44,6 +44,8 @@ import { shouldPreferSingleWorkerDrawElement, shouldStreamParallelCapture, shouldUseStreamingEncode, + resolveObservedCaptureMode, + createCaptureObservabilityUpdater, } from "./renderOrchestrator.js"; import { probeRequiresBrowser } from "./render/stages/probeStage.js"; import { ensureFrameWritten } from "./render/stages/captureHdrFrameShared.js"; @@ -2660,3 +2662,71 @@ describe("closeOrphanedProbeForRetry (probe cleanup before verify-triggered retr expect((log.warn.mock.calls[0][1] as { error: string }).error).toBe("string-only rejection"); }); }); + +// BeginFrame is Linux-only in both real entry points, but the observability +// field derived its mode from `forceScreenshot` alone. That mislabelled 30,625 +// Windows renders as `beginframe` over 14 days — a fifth of the fast-capture +// dashboard's capture-mode data — for captures that were really screenshot. +describe("resolveObservedCaptureMode", () => { + it("only ever reports beginframe on linux", () => { + expect(resolveObservedCaptureMode(false, "linux")).toBe("beginframe"); + expect(resolveObservedCaptureMode(false, "win32")).toBe("screenshot"); + expect(resolveObservedCaptureMode(false, "darwin")).toBe("screenshot"); + }); + + // The case the old code got right, kept so a future simplification back to + // a bare boolean fails here rather than in production telemetry. + it("reports screenshot whenever screenshot was forced, linux included", () => { + expect(resolveObservedCaptureMode(true, "linux")).toBe("screenshot"); + expect(resolveObservedCaptureMode(true, "win32")).toBe("screenshot"); + }); +}); + +// The blocker found in review: seeding `captureMode` at construction is not +// enough. `updateCaptureObservability` fires at 23 sites, and the post-compile +// `{ forceScreenshot }` patch runs on EVERY render — the old closure re-derived +// from `forceScreenshot` alone and put `beginframe` back before capture began, +// so both telemetry emits read the reverted value. These go through the closure +// rather than the helper, which is the only way to catch that. +describe("createCaptureObservabilityUpdater", () => { + const seed = (platform: NodeJS.Platform, forceScreenshot: boolean) => { + const observability = { + forceScreenshot, + captureMode: resolveObservedCaptureMode(forceScreenshot, platform), + browserGpuMode: "hardware" as const, + } as Parameters[0]; + return { observability, update: createCaptureObservabilityUpdater(observability, platform) }; + }; + + // The exact production case: Windows, hardware GPU, screenshot not forced. + it("keeps screenshot on win32 across an unrelated patch", () => { + const { observability, update } = seed("win32", false); + expect(observability.captureMode).toBe("screenshot"); + update({ transientRetries: 1 }); + expect(observability.captureMode).toBe("screenshot"); + }); + + // The unconditional post-compile patch — the one that guaranteed the revert. + it("keeps screenshot on win32 when forceScreenshot is re-patched false", () => { + const { observability, update } = seed("win32", false); + update({ forceScreenshot: false }); + expect(observability.captureMode).toBe("screenshot"); + }); + + // Linux must still be able to report beginframe, or the fix would have + // silently disabled the field everywhere instead of correcting it. + it("still reports beginframe on linux, and honours a later force", () => { + const { observability, update } = seed("linux", false); + expect(observability.captureMode).toBe("beginframe"); + update({ transientRetries: 2 }); + expect(observability.captureMode).toBe("beginframe"); + update({ forceScreenshot: true }); + expect(observability.captureMode).toBe("screenshot"); + }); + + it("applies the patch itself, not only the derived mode", () => { + const { observability, update } = seed("win32", false); + update({ workerCount: 4 }); + expect(observability.workerCount).toBe(4); + }); +}); diff --git a/packages/producer/src/services/renderOrchestrator.ts b/packages/producer/src/services/renderOrchestrator.ts index 840d17277d..69100e1e1a 100644 --- a/packages/producer/src/services/renderOrchestrator.ts +++ b/packages/producer/src/services/renderOrchestrator.ts @@ -80,6 +80,7 @@ import { resolveBrowserGpuMode, resolveHeadlessShellPath, applyConcreteGpuScreenshotClamp, + explainDrawElementDisabled, scaleProtocolTimeoutForComposition, classifyCaptureFailure, cloneCaptureWarning, @@ -807,6 +808,72 @@ export function buildMissingFrameRetryBatches( return batches; } +/** + * The capture mode this render will REPORT, pre-capture. + * + * BeginFrame is Linux-only. Both real entry points enforce that — + * `frameCapture`'s preMode (`headlessShell && isLinux && !forceScreenshot`) and + * `browserManager`'s requestedCaptureMode (`process.platform === "linux"`) — + * but the observability field derived the mode from `forceScreenshot` alone, + * with no platform test. Every non-Linux render that did not force screenshot + * therefore reported `beginframe` for a capture that was really screenshot: + * 30,625 Windows renders over 14 days, a fifth of the fast-capture dashboard's + * capture-mode data. + * + * `config.ts` documents this same failure for "darwin + software" and adds a + * `forceScreenshot` clamp as defence-in-depth — but that clamp only fires on + * software GPU, so Windows-on-hardware slipped straight past it (41,102 of the + * mislabelled renders). + * + * NECESSARY, NOT SUFFICIENT — read this before trusting the value on Linux. + * The platform test is the only condition modelled here. Linux BeginFrame + * additionally requires a headless-shell binary, no supersampling, no + * transparent drawElement route (`frameCapture.ts` preMode) and the + * `--enable-begin-frame-control` flag (`browserManager.ts`). Any of those can + * make the ACTUAL mode screenshot while this still reports `beginframe`, so a + * Linux `beginframe` reading is an upper bound, not a fact. The authoritative + * value is the session's own `launchCaptureMode` — the same field the runtime + * video gate already falls back to. Deriving this field from the resolved + * session instead of from config is the real fix and is deliberately NOT in + * this change: it closes the Windows mislabel, which is platform-only and + * needs no session plumbing. + * + * Pure; exported for tests. + */ +export function resolveObservedCaptureMode( + forceScreenshot: boolean, + platform: NodeJS.Platform = process.platform, +): "screenshot" | "beginframe" { + return forceScreenshot || platform !== "linux" ? "screenshot" : "beginframe"; +} + +/** + * Build the observability patcher, re-deriving `captureMode` on every patch. + * + * Extracted and exported because the previous inline closure was where the + * Windows mislabel actually lived. Seeding `captureMode` correctly at + * construction is NOT sufficient: this updater is invoked at 23 sites through + * the pipeline, and one of them — + * `updateCaptureObservability({ forceScreenshot: captureForceScreenshot })` + * straight after compile — runs unconditionally on every render. The old body + * re-derived from `forceScreenshot` alone, so the seeded value was overwritten + * with `beginframe` again before capture began, and both the success and error + * telemetry emits read the reverted object. A helper-only test cannot catch + * that: it never round-trips through this closure. Hence the export. + */ +export function createCaptureObservabilityUpdater( + observability: RenderCaptureObservability, + platform: NodeJS.Platform = process.platform, +): (patch: Partial) => void { + return (patch: Partial): void => { + Object.assign(observability, patch); + observability.captureMode = resolveObservedCaptureMode( + Boolean(observability.forceScreenshot), + platform, + ); + }; +} + export function getNextRetryWorkerCount(currentWorkers: number): number { return Math.max(1, Math.floor(currentWorkers / 2)); } @@ -2017,7 +2084,7 @@ async function executeRenderPipeline(input: { const chunkedEncodeSize = cfg.chunkSizeFrames; const captureObservability: RenderCaptureObservability = { forceScreenshot: Boolean(cfg.forceScreenshot), - captureMode: cfg.forceScreenshot ? "screenshot" : "beginframe", + captureMode: resolveObservedCaptureMode(Boolean(cfg.forceScreenshot)), browserGpuMode: cfg.browserGpuMode, protocolTimeoutMs: cfg.protocolTimeout, pageNavigationTimeoutMs: cfg.pageNavigationTimeout, @@ -2025,12 +2092,7 @@ async function executeRenderPipeline(input: { }; let extractionObservability: RenderExtractionObservability | undefined; let compositionHash: string | undefined; - const updateCaptureObservability = (patch: Partial): void => { - Object.assign(captureObservability, patch); - captureObservability.captureMode = captureObservability.forceScreenshot - ? "screenshot" - : "beginframe"; - }; + const updateCaptureObservability = createCaptureObservabilityUpdater(captureObservability); // Function-scoped (not inside the try) so both the success path AND the catch // can read it — the catch records transient-retry burn on renders that still // failed, which is the more actionable signal for tuning the retry cap. @@ -2187,7 +2249,21 @@ async function executeRenderPipeline(input: { // drawElement release telemetry: why default DE disengaged (if it did), // whether self-verify fell back, and the drain-side counters. const deCompileGate = compileResult.deCompileGate; - let deClampReason: string | undefined; + // Seed with the CONFIG-TIME refusal, if there was one. The clamp further + // down only runs `if (cfg.useDrawElement && ...)`, so a render that never + // became a drawElement candidate at all could never acquire a reason — + // it reached telemetry with every DE field empty and landed in the + // dashboard's `other` bucket (56,507 renders / 14d, second-largest bar on + // "Why not drawElement", explaining nothing). Re-derived from the same + // inputs `resolveConfig` used, so it cannot disagree with the decision. + // Later clamps overwrite this: a more specific reason always wins. + let deClampReason: string | undefined = cfg.useDrawElement + ? undefined + : explainDrawElementDisabled({ + platform: process.platform, + browserGpuMode: cfg.browserGpuMode, + workerEncode: cfg.enableDrawElementWorkerEncode, + }); // "inverted" = fired and held; "reverted" = fired but the self-verify // retry rolled back to the parallel path; undefined = never fired. let deWorkerInversion: "inverted" | "reverted" | undefined; @@ -3052,8 +3128,9 @@ async function executeRenderPipeline(input: { // Which mode will stream: the engine picks beginframe only on Linux with // headless-shell and no forced screenshot (frameCapture.ts preMode); // everything else is screenshot. Recorded for telemetry cohorting. - const captureParallelStream = - process.platform === "linux" && !captureForceScreenshot ? "beginframe" : "screenshot"; + // Same predicate as the observability field — use the one helper so the + // two cannot drift if the router's modes ever change. + const captureParallelStream = resolveObservedCaptureMode(captureForceScreenshot); log.info( `[Render] Parallel ${captureParallelStream} capture will stream to the encoder ` + `(interleaved, ${workerCount} workers) instead of the disk path. ` +