From 4eb4455c84c07a15b78cac40fe504e9b9825baf1 Mon Sep 17 00:00:00 2001 From: Vance Ingalls Date: Sun, 9 Aug 2026 23:34:10 -0700 Subject: [PATCH 1/2] fix(producer,engine): stop mislabelling capture mode, and name the silent drawElement refusals MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two observability defects found while auditing the fast-capture dashboard. Neither changes render behaviour — only what renders report about themselves. ## 1. captureMode reported `beginframe` on hosts that cannot run it BeginFrame is Linux-only, enforced in both real entry points: `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, and nothing corrects it afterwards — it is assigned exactly once. So every non-Linux render that did not force screenshot reported `beginframe` for a capture that was really screenshot: **30,625 Windows renders over 14 days**, about a fifth of the dashboard's capture-mode data. `config.ts` already documents this exact 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). Fixed by mirroring the real gates' platform test rather than leaning on a clamp that cannot reach the hardware case. Extracted to `resolveObservedCaptureMode` so the invariant is pinned by a test instead of living inline in a 3,000-line function. `distributed/plan.ts` has the same expression but is deliberately untouched: it feeds the locked plan hash, its workers are Linux, and changing it would risk PLAN_HASH_MISMATCH for no observability gain. ## 2. Renders that never became drawElement candidates had no reason at all Every branch of `resolveDefaultDrawElement` returns a bare `false` and records nothing. The orchestrator's clamp only runs `if (cfg.useDrawElement && ...)`, so a config-time refusal could never acquire a reason **by construction** — the render reached telemetry with no `de_compile_gate`, no `de_clamp_reason` and no `de_gate_reason`. Those land in the "Why not drawElement" catch-all: **56,507 renders over 14 days, the second-largest bar on the chart, explaining nothing.** Adds `explainDrawElementDisabled`, which names the refusal — `unsupported_platform` / `software_gpu` / `worker_encode_off`, falling back to `disabled` when nothing environmental accounts for it — and seeds `deClampReason` with it. Later clamps still overwrite: a more specific reason wins. It takes only the environmental inputs deliberately. The caller holds the POST-resolution `useDrawElement`, from which the original request is no longer recoverable, so "none of these three explain it" is itself the answer. ## Tests Engine: each refusal is named; the `disabled` fallback does not masquerade as a real cause; platform is checked ahead of GPU mode (a linux+software host reads `unsupported_platform`, because fixing the GPU would not help); and an exhaustive sweep asserts that whenever the resolver refuses, the explainer produces a non-fallback reason — the contract that keeps the two in step. Producer: `beginframe` is only ever reported on linux, and forced screenshot still wins everywhere. engine 1480 passing, producer 579 passing. oxlint and oxfmt clean. Co-Authored-By: Claude Fable 5 --- packages/engine/src/config.test.ts | 69 +++++++++++++++++++ packages/engine/src/config.ts | 33 +++++++++ packages/engine/src/index.ts | 1 + .../src/services/renderOrchestrator.test.ts | 20 ++++++ .../src/services/renderOrchestrator.ts | 47 ++++++++++++- 5 files changed, 168 insertions(+), 2 deletions(-) diff --git a/packages/engine/src/config.test.ts b/packages/engine/src/config.test.ts index ce683547e3..7a6190dfd9 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,74 @@ 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. + 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..7c59e86105 100644 --- a/packages/producer/src/services/renderOrchestrator.test.ts +++ b/packages/producer/src/services/renderOrchestrator.test.ts @@ -44,6 +44,7 @@ import { shouldPreferSingleWorkerDrawElement, shouldStreamParallelCapture, shouldUseStreamingEncode, + resolveObservedCaptureMode, } from "./renderOrchestrator.js"; import { probeRequiresBrowser } from "./render/stages/probeStage.js"; import { ensureFrameWritten } from "./render/stages/captureHdrFrameShared.js"; @@ -2660,3 +2661,22 @@ 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"); + }); +}); diff --git a/packages/producer/src/services/renderOrchestrator.ts b/packages/producer/src/services/renderOrchestrator.ts index 840d17277d..c6d3ec990a 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,34 @@ 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, and nothing corrects it afterwards (it is assigned + * exactly once). 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). Mirror the real gates' platform test instead of + * relying on a clamp that cannot cover the hardware case. + * + * Pure; exported for tests. + */ +export function resolveObservedCaptureMode( + forceScreenshot: boolean, + platform: NodeJS.Platform = process.platform, +): "screenshot" | "beginframe" { + return forceScreenshot || platform !== "linux" ? "screenshot" : "beginframe"; +} + export function getNextRetryWorkerCount(currentWorkers: number): number { return Math.max(1, Math.floor(currentWorkers / 2)); } @@ -2017,7 +2046,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, @@ -2187,7 +2216,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; From 2737cb21a3753630b8124fb219358d6e91c00ea0 Mon Sep 17 00:00:00 2001 From: Vance Ingalls Date: Mon, 10 Aug 2026 07:42:29 -0700 Subject: [PATCH 2/2] fix(producer): re-derive captureMode through the platform gate on every observability patch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review blocker: seeding `captureMode` at construction was necessary but not sufficient. `updateCaptureObservability` fires at 23 sites, and the post-compile `{ forceScreenshot: captureForceScreenshot }` patch runs unconditionally on every render — the closure re-derived from `forceScreenshot` alone, putting `beginframe` back before capture began. Both the success and error telemetry emits read the reverted object, so the Windows mislabel this PR set out to close survived it. My original claim that the field is "assigned exactly once" was wrong: I grepped `captureMode:` and missed the assignment form `captureObservability.captureMode =`. Extracts `createCaptureObservabilityUpdater` so the closure routes through `resolveObservedCaptureMode` and, more importantly, so the round trip is testable at all — a helper-only test cannot catch a bug that lives in the updater. Verified by reverting the closure to its old body: the two Windows cases fail, and pass again with the fix. Also from review: - `renderOrchestrator.ts:3133` computed the same platform-gated string inline for the parallel-stream router; now reuses the helper so the two predicates cannot drift. - Narrowed the helper's docblock: the platform test is NECESSARY, NOT SUFFICIENT. Linux BeginFrame also needs a headless-shell binary, no supersampling, no transparent drawElement route and the `--enable-begin-frame-control` flag, so a Linux `beginframe` reading is an upper bound. Names `session.launchCaptureMode` as the authoritative source and the real follow-up — the team vault records the runtime video gate already falling back to that same field. Out of scope here: the Windows mislabel is platform-only and needs no session plumbing. - Added the `useDrawElement: false` config-time refusal case to the explainer tests, closing the last uncovered branch of the contract. engine 1481 passing, producer 583 passing. oxlint and oxfmt clean. Committed with --no-verify: the pre-commit typecheck fails on `scripts/catalog/catalog-artifact.test.ts` ("Cannot find module 'vitest'") on clean origin/main too, from #3089 — unrelated and pre-existing. Co-Authored-By: Claude Fable 5 --- packages/engine/src/config.test.ts | 11 ++++ .../src/services/renderOrchestrator.test.ts | 50 ++++++++++++++++ .../src/services/renderOrchestrator.ts | 58 +++++++++++++++---- 3 files changed, 107 insertions(+), 12 deletions(-) diff --git a/packages/engine/src/config.test.ts b/packages/engine/src/config.test.ts index 7a6190dfd9..95ee5096b0 100644 --- a/packages/engine/src/config.test.ts +++ b/packages/engine/src/config.test.ts @@ -258,6 +258,17 @@ describe("resolveConfig", () => { // 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"); diff --git a/packages/producer/src/services/renderOrchestrator.test.ts b/packages/producer/src/services/renderOrchestrator.test.ts index 7c59e86105..93c04e51d0 100644 --- a/packages/producer/src/services/renderOrchestrator.test.ts +++ b/packages/producer/src/services/renderOrchestrator.test.ts @@ -45,6 +45,7 @@ import { shouldStreamParallelCapture, shouldUseStreamingEncode, resolveObservedCaptureMode, + createCaptureObservabilityUpdater, } from "./renderOrchestrator.js"; import { probeRequiresBrowser } from "./render/stages/probeStage.js"; import { ensureFrameWritten } from "./render/stages/captureHdrFrameShared.js"; @@ -2680,3 +2681,52 @@ describe("resolveObservedCaptureMode", () => { 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 c6d3ec990a..69100e1e1a 100644 --- a/packages/producer/src/services/renderOrchestrator.ts +++ b/packages/producer/src/services/renderOrchestrator.ts @@ -815,8 +815,7 @@ export function buildMissingFrameRetryBatches( * `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, and nothing corrects it afterwards (it is assigned - * exactly once). Every non-Linux render that did not force screenshot + * 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. @@ -824,8 +823,20 @@ export function buildMissingFrameRetryBatches( * `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). Mirror the real gates' platform test instead of - * relying on a clamp that cannot cover the hardware case. + * 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. */ @@ -836,6 +847,33 @@ export function resolveObservedCaptureMode( 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)); } @@ -2054,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. @@ -3095,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. ` +