fix(producer,engine): stop mislabelling capture mode, and name the silent drawElement refusals (#3151)

* fix(producer,engine): stop mislabelling capture mode, and name the silent drawElement refusals

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 <noreply@anthropic.com>

* fix(producer): re-derive captureMode through the platform gate on every observability patch

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 <noreply@anthropic.com>

---------

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
Vance Ingalls
2026-08-10 08:02:00 -07:00
committed by GitHub
co-authored by Claude Fable 5
parent 68205dbbc1
commit 33ac86fd38
5 changed files with 271 additions and 10 deletions
+80
View File
@@ -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,