From 0749cd9ff83ad41c72c7dc6be1b224e9bc27731b Mon Sep 17 00:00:00 2001 From: Vance Ingalls Date: Wed, 29 Jul 2026 02:36:40 -0700 Subject: [PATCH 1/6] feat(producer): open the DE single-worker inversion to short comps under an element ceiling MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 31% of fleet renders (24h, v0.7.78+) are DE-eligible comps clamped to parallel screenshot purely because they sit under the 900-frame inversion floor — the median fleet render is ~250-600 frames, below every DE entry threshold. This opens a 250-899 frame band, gated on composition size. Measured, not assumed. A controlled sweep (fixed synthetic content, {250,400,600,900}f, single-DE vs parallel-screenshot-W4, 3 reps, capture mode verified per row, AC power, load-gated) showed single-DE winning 1.16-1.24x at every size — but only for content in constant motion. A follow-up 2x2 found motion and DOM size pull in OPPOSITE directions, so neither alone predicts the winner (ratio = ss4/de1, >1 means DE wins): 24 movers / 0 nodes -> 1.05 320 movers / 0 nodes -> 1.24 320 movers / 7000 nodes -> 1.09 24 movers / 7000 nodes -> 0.96 24 movers / 20000 nodes -> 0.71 24 movers / 40000 nodes -> 0.55 DE's wall-clock scales ~0.50ms/element against parallel screenshot's ~0.22ms — drawElement repaints the whole tree per frame while fan-out amortizes it — so the downside is NOT bounded and a bare floor drop would have handed a 1.8x regression to large comps. Since motion only ever helps DE, an element ceiling calibrated at the lowest-motion case is safe at every motion level; crossover there is ~3.9k, and the default sits at 2500. The predicate is untouched; the call site picks the floor. Above the ceiling, or at 900+ frames, behaviour is bit-identical to today — the change can only add inversions in the new band, never remove one. Instrumentation, since this ships at full exposure rather than cohorted: `composition_element_count` on EVERY render (the fleet distribution of the gate variable is unknown — without it we cannot tell whether 2500 opens the band for most short comps or almost none, nor re-derive the threshold from real content), and `de_short_band` = applied | skipped_elements, unset when the frame count made the band irrelevant, so a fleet perf shift is attributable to this change rather than to content mix. Safety is unchanged and already proven on this path: per-frame PSNR self-verify with screenshot fallback, exactly as the 900+ band has shipped default-on. Knobs: HF_DE_SHORT_MIN_FRAMES, HF_DE_SHORT_MAX_ELEMENTS (0 disables the band). Co-Authored-By: Claude Opus 5 (1M context) --- packages/cli/src/commands/render.ts | 2 + packages/cli/src/telemetry/events.ts | 8 ++ .../cli/src/telemetry/renderObservability.ts | 2 + .../src/services/render/observability.ts | 20 ++++ .../src/services/render/perfSummary.ts | 6 + .../src/services/renderOrchestrator.test.ts | 104 ++++++++++++++++++ .../src/services/renderOrchestrator.ts | 84 +++++++++++++- 7 files changed, 225 insertions(+), 1 deletion(-) diff --git a/packages/cli/src/commands/render.ts b/packages/cli/src/commands/render.ts index 5c8fb75310..6dcb77be88 100644 --- a/packages/cli/src/commands/render.ts +++ b/packages/cli/src/commands/render.ts @@ -1433,6 +1433,8 @@ function trackRenderMetrics( deClampReason: perf?.drawElement?.clampReason, deWorkerInversion: perf?.drawElement?.workerInversion, dePreInversionWorkers: perf?.drawElement?.preInversionWorkers, + compositionElementCount: perf?.drawElement?.compositionElementCount, + deShortBand: perf?.drawElement?.shortBand, deParallelRouter: perf?.drawElement?.parallelRouter, dePreRouterWorkers: perf?.drawElement?.preRouterWorkers, deGateReason: perf?.drawElement?.gateReason, diff --git a/packages/cli/src/telemetry/events.ts b/packages/cli/src/telemetry/events.ts index cd68747569..b0d37aabbf 100644 --- a/packages/cli/src/telemetry/events.ts +++ b/packages/cli/src/telemetry/events.ts @@ -72,6 +72,8 @@ export interface RenderObservabilityTelemetryPayload { // the more authoritative perfSummary value wins when both are present. captureDeWorkerInversion?: string; captureDePreInversionWorkers?: number; + captureCompositionElementCount?: number; + captureDeShortBand?: string; captureDeParallelRouter?: string; captureDeGpuRenderer?: string; captureDePreRouterWorkers?: number; @@ -130,6 +132,8 @@ function renderObservabilityEventProperties(props: RenderObservabilityTelemetryP capture_memory_exhaustion_detected: props.captureMemoryExhaustionDetected, de_worker_inversion: props.captureDeWorkerInversion, de_pre_inversion_workers: props.captureDePreInversionWorkers, + composition_element_count: props.captureCompositionElementCount, + de_short_band: props.captureDeShortBand, de_parallel_router: props.captureDeParallelRouter, gpu_renderer: props.captureDeGpuRenderer, de_pre_router_workers: props.captureDePreRouterWorkers, @@ -204,6 +208,8 @@ export function trackRenderComplete( deClampReason?: string; deWorkerInversion?: string; dePreInversionWorkers?: number; + compositionElementCount?: number; + deShortBand?: string; deParallelRouter?: string; dePreRouterWorkers?: number; deGateReason?: string; @@ -303,6 +309,8 @@ export function trackRenderComplete( de_clamp_reason: props.deClampReason, de_worker_inversion: props.deWorkerInversion, de_pre_inversion_workers: props.dePreInversionWorkers, + composition_element_count: props.compositionElementCount, + de_short_band: props.deShortBand, de_parallel_router: props.deParallelRouter, de_pre_router_workers: props.dePreRouterWorkers, de_gate_reason: props.deGateReason, diff --git a/packages/cli/src/telemetry/renderObservability.ts b/packages/cli/src/telemetry/renderObservability.ts index 87554d6939..10a1ad22f4 100644 --- a/packages/cli/src/telemetry/renderObservability.ts +++ b/packages/cli/src/telemetry/renderObservability.ts @@ -42,6 +42,8 @@ export function renderObservabilityTelemetryPayload( captureMemoryExhaustionDetected: capture.memoryExhaustionDetected, captureDeWorkerInversion: capture.deWorkerInversion, captureDePreInversionWorkers: capture.dePreInversionWorkers, + captureCompositionElementCount: capture.compositionElementCount, + captureDeShortBand: capture.deShortBand, captureDeParallelRouter: capture.deParallelRouter, captureDeGpuRenderer: capture.deGpuRenderer, captureDePreRouterWorkers: capture.dePreRouterWorkers, diff --git a/packages/producer/src/services/render/observability.ts b/packages/producer/src/services/render/observability.ts index 39111960cd..dfe0bc2a40 100644 --- a/packages/producer/src/services/render/observability.ts +++ b/packages/producer/src/services/render/observability.ts @@ -70,6 +70,26 @@ export interface RenderCaptureObservability { deWorkerInversion?: "inverted" | "reverted"; /** Worker count the resolver would have used absent the inversion; undefined if it never fired. */ dePreInversionWorkers?: number; + /** + * Rough element count of the compiled composition (`countElementTags`). + * + * Emitted on every render, not just inverted ones — this is the variable the + * short-comp inversion band is gated on, and the fleet distribution of it is + * unknown. Without it there is no way to tell whether the 2500 ceiling opens + * the band for most short comps or almost none, and no way to re-derive the + * threshold from real content instead of synthetic sweeps. + */ + compositionElementCount?: number; + /** + * Why the short-comp band did or did not apply to this render: + * "applied" (frames landed in 250-899 and the element count cleared the + * ceiling), "skipped_elements" (band was open by frame count but the comp + * was too large), or undefined when the frame count made the band + * irrelevant either way. Distinguishes "the new routing chose this" from + * "the pre-existing 900 floor chose this" — otherwise a fleet perf shift is + * unattributable. + */ + deShortBand?: "applied" | "skipped_elements"; /** DE parallel-router outcome: "routed" (fired, held) | "reverted" (fired, self-verify retry rolled back). */ deParallelRouter?: "routed" | "reverted"; /** diff --git a/packages/producer/src/services/render/perfSummary.ts b/packages/producer/src/services/render/perfSummary.ts index fe72e1e1c0..0770d58ee7 100644 --- a/packages/producer/src/services/render/perfSummary.ts +++ b/packages/producer/src/services/render/perfSummary.ts @@ -80,6 +80,10 @@ export interface DrawElementPerfInput { workerInversion?: "inverted" | "reverted"; /** Auto-resolved worker count before the inversion pinned it to 1 (set only when the inversion fired). */ preInversionWorkers?: number; + /** Rough compiled-composition element count — gate variable for the short-comp inversion band. */ + compositionElementCount?: number; + /** Short-comp band attribution: "applied" | "skipped_elements"; unset when the band was irrelevant. */ + shortBand?: string; parallelRouter?: "routed" | "reverted"; /** Auto-resolved worker count before the router pinned it to 3 (set only when the router fired). */ preRouterWorkers?: number; @@ -121,6 +125,8 @@ function aggregateDrawElement( clampReason: de.clampReason, workerInversion: de.workerInversion ?? "none", preInversionWorkers: de.preInversionWorkers, + compositionElementCount: de.compositionElementCount, + shortBand: de.shortBand, parallelRouter: de.parallelRouter ?? "none", preRouterWorkers: de.preRouterWorkers, gateReason: gateReasons.length > 0 ? gateReasons.join("|") : undefined, diff --git a/packages/producer/src/services/renderOrchestrator.test.ts b/packages/producer/src/services/renderOrchestrator.test.ts index 791a17389b..16536f213f 100644 --- a/packages/producer/src/services/renderOrchestrator.test.ts +++ b/packages/producer/src/services/renderOrchestrator.test.ts @@ -34,6 +34,8 @@ import { resolveParallelRouterRetryPlan, resetCaptureAttemptProgress, shouldRetryViaPinnedFallback, + countElementTags, + envInt, shouldPreferParallelDrawElement, shouldPreferSingleWorkerDrawElement, shouldStreamParallelCapture, @@ -1693,6 +1695,108 @@ describe("shouldPreferSingleWorkerDrawElement (DE priority inversion)", () => { expect(shouldPreferSingleWorkerDrawElement(eligible)).toBe(true); }); + // ── Short-comp band ──────────────────────────────────────────────────── + // The band lowers the effective floor from 900 to 250 for SMALL comps only. + // The predicate itself is unchanged — the call site picks the floor — so + // these pin the arithmetic the call site performs. + // + // Measured basis (400f, single-DE vs parallel-screenshot-W4, 2 reps, ratio + // = ss4/de1 so >1 means DE wins): + // 24 movers / 0 nodes -> 1.05 DE wins + // 320 movers / 0 nodes -> 1.24 DE wins + // 320 movers / 7000 nodes -> 1.09 DE wins + // 24 movers / 7000 nodes -> 0.96 DE LOSES + // 24 movers / 20000 nodes -> 0.71 DE loses badly + // 24 movers / 40000 nodes -> 0.55 DE loses very badly + // Motion helps DE, DOM size punishes it; the ceiling is calibrated at the + // lowest-motion case so every higher-motion comp is covered too. + describe("short-comp band floor arithmetic", () => { + const shortBandFloor = (elementCount: number, maxElements = 2500): number => + elementCount <= maxElements ? Math.min(900, 250) : 900; + + it("opens the 250-frame floor for a small comp", () => { + expect(shortBandFloor(800)).toBe(250); + expect( + shouldPreferSingleWorkerDrawElement({ + ...eligible, + totalFrames: 400, + minFrames: shortBandFloor(800), + }), + ).toBe(true); + }); + + it("keeps the 900-frame floor for a large comp — the measured 1.8x regression case", () => { + expect(shortBandFloor(40000)).toBe(900); + expect( + shouldPreferSingleWorkerDrawElement({ + ...eligible, + totalFrames: 400, + minFrames: shortBandFloor(40000), + }), + ).toBe(false); + }); + + it("never RAISES the floor: a large comp at 900+ frames still inverts as it did before", () => { + expect( + shouldPreferSingleWorkerDrawElement({ + ...eligible, + totalFrames: 2380, + minFrames: shortBandFloor(40000), + }), + ).toBe(true); + }); + + it("leaves comps below the band floor alone", () => { + expect( + shouldPreferSingleWorkerDrawElement({ + ...eligible, + totalFrames: 200, + minFrames: shortBandFloor(800), + }), + ).toBe(false); + }); + }); + + describe("countElementTags", () => { + it("counts closing tags", () => { + expect(countElementTags("
a
")).toBe(2); + }); + + it("undercounts void elements — biases the count DOWN, so the ceiling must stay conservative", () => { + expect(countElementTags("
")).toBe(0); + }); + + it("is stable on empty and malformed input rather than throwing", () => { + expect(countElementTags("")).toBe(0); + expect(countElementTags("<<<>>>")).toBe(0); + }); + + it("scales to a large document without a full parse", () => { + expect(countElementTags("

x

".repeat(40000))).toBe(40000); + }); + }); + + describe("envInt", () => { + afterEach(() => { + delete process.env.HF_TEST_ENV_INT; + }); + + it("falls back when unset, empty, or non-numeric — a typo must not disable a guard", () => { + expect(envInt("HF_TEST_ENV_INT", 2500)).toBe(2500); + process.env.HF_TEST_ENV_INT = ""; + expect(envInt("HF_TEST_ENV_INT", 2500)).toBe(2500); + process.env.HF_TEST_ENV_INT = "lots"; + expect(envInt("HF_TEST_ENV_INT", 2500)).toBe(2500); + }); + + it("reads an explicit value, including 0 as a real disable", () => { + process.env.HF_TEST_ENV_INT = "700"; + expect(envInt("HF_TEST_ENV_INT", 2500)).toBe(700); + process.env.HF_TEST_ENV_INT = "0"; + expect(envInt("HF_TEST_ENV_INT", 2500)).toBe(0); + }); + }); + it("honors explicitly requested workers", () => { expect(shouldPreferSingleWorkerDrawElement({ ...eligible, requestedWorkers: 3 })).toBe(false); }); diff --git a/packages/producer/src/services/renderOrchestrator.ts b/packages/producer/src/services/renderOrchestrator.ts index 232bc29acc..2369bf2c02 100644 --- a/packages/producer/src/services/renderOrchestrator.ts +++ b/packages/producer/src/services/renderOrchestrator.ts @@ -492,6 +492,10 @@ export interface RenderPerfSummary { workerInversion?: string; /** Worker count the auto-resolution chose BEFORE the inversion pinned it to 1 — the parallel counterfactual for speedup math. Only set when the inversion fired. */ preInversionWorkers?: number; + /** Rough compiled-composition element count — the variable the short-comp inversion band is gated on. Always set. */ + compositionElementCount?: number; + /** Short-comp band attribution: "applied" | "skipped_elements"; unset when the frame count made the band irrelevant. */ + shortBand?: string; /** DE parallel-router outcome: "routed" (fired, held), "reverted" (fired, self-verify retry rolled back), "none". Mutually exclusive with workerInversion. */ parallelRouter?: string; /** Worker count the auto-resolution chose BEFORE the router pinned it to 3 — the single-worker-inversion counterfactual. Only set when the router fired. */ @@ -1222,6 +1226,37 @@ export function shouldUseStreamingEncode( return workerCount === 1; } +/** + * Integer tuning knob from the environment. Matches the convention the + * surrounding DE thresholds already use: unset OR set-but-empty falls back to + * the default (a blank var is not a kill switch), and so does anything + * non-numeric — a typo must never silently disable a routing guard. + */ +export function envInt(name: string, fallback: number): number { + const raw = process.env[name]; + if (raw === undefined || raw.trim() === "") return fallback; + const parsed = Number(raw); + return Number.isFinite(parsed) ? parsed : fallback; +} + +/** + * Rough element count for compiled composition HTML. + * + * Deliberately a string scan and not a `parseHTML` + `querySelectorAll` (the + * `countAuthoredTimedClips` approach): this runs on EVERY render before the + * routing decision, and a full linkedom parse of the exact documents that + * matter here — the 20k-40k node ones — is the most expensive case. Precision + * is not needed. It feeds a threshold whose measured crossover is ~3.9k and + * whose default sits at 2500, so counting closing tags is comfortably inside + * the margin. Undercounts void elements (``, `
`) and self-closing + * SVG nodes, which biases the count DOWN — the direction that opens the band + * — so the ceiling is the thing to keep conservative. + */ +export function countElementTags(html: string): number { + const matches = html.match(/<\/[a-zA-Z]/g); + return matches === null ? 0 : matches.length; +} + /** * DE priority inversion predicate: should an AUTO-resolved multi-worker render * drop to single-worker verified drawElement streaming? @@ -2405,6 +2440,38 @@ async function executeRenderPipeline(input: { ? 900 : Number(deSingleMinFramesRaw); const deSingleMinFrames = Number.isFinite(deSingleMinFramesNum) ? deSingleMinFramesNum : 900; + // Short-comp band: 31% of fleet renders (24h, 0.7.78+) are DE-eligible + // comps clamped to parallel screenshot purely because they sit under this + // floor. A controlled sweep (fixed synthetic content, {250,400,600,900}f, + // single-DE vs parallel-screenshot-W4, 3 reps, capture modes verified per + // run) showed single-DE winning 1.16-1.24x at EVERY size — but only for + // content in constant motion. A follow-up 2x2 (movers x static DOM nodes) + // found the two variables pull in opposite directions: motion favours DE, + // DOM size punishes it, and DE's wall-clock scales ~0.50ms/node against + // parallel screenshot's ~0.22ms (drawElement repaints the whole tree per + // frame; fan-out amortizes it). At 24 movers / 400f the measured curve is + // +5% for DE at 0 nodes, -4% at 7k, -41% at 20k, -80% at 40k — crossover + // near ~3.9k. Since motion only ever helps DE, a node ceiling calibrated + // at the LOWEST-motion case is safe for every motion level, so the short + // band opens only below `deShortBandMaxElements`. Above it the original + // 900 floor stands, unchanged. + const deShortBandMinFrames = envInt("HF_DE_SHORT_MIN_FRAMES", 250); + const deShortBandMaxElements = envInt("HF_DE_SHORT_MAX_ELEMENTS", 2500); + const compositionElementCount = countElementTags(compiled.html); + const deShortBandOpen = + deShortBandMinFrames > 0 && + deShortBandMaxElements > 0 && + compositionElementCount <= deShortBandMaxElements; + const deEffectiveMinFrames = deShortBandOpen + ? Math.min(deSingleMinFrames, deShortBandMinFrames) + : deSingleMinFrames; + // Does the band even matter for this render? Only when the frame count + // falls in the newly-opened window — at or above the original floor the + // inversion fires regardless, and below `deShortBandMinFrames` nothing + // fires either way. Keeps the telemetry reason from claiming credit (or + // blame) on renders the change could not have affected. + const deShortBandApplies = + totalFrames >= deShortBandMinFrames && totalFrames < deSingleMinFrames; // "Would ANY multi-worker resolution be inverted?" — if workers resolve // to 1 naturally the outcome is identical either way. const WOULD_RESOLVE_MULTI_WORKER = 2; @@ -2416,7 +2483,7 @@ async function executeRenderPipeline(input: { forceScreenshot: captureForceScreenshot, outputFormat, totalFrames, - minFrames: deSingleMinFrames, + minFrames: deEffectiveMinFrames, singleWorkerStreamingOk: shouldUseStreamingEncode(cfg, outputFormat, 1, job.duration), layeredOrEffectRoute: hasHdrContent || compiled.hasShaderTransitions, supersampling: deviceScaleFactor > 1, @@ -2678,6 +2745,15 @@ async function executeRenderPipeline(input: { // any resource-pressure failure unique to this cohort. dePreInversionWorkers: deWorkerInversion ? preRoutingWorkerCount : undefined, dePreRouterWorkers: deParallelRouter ? preRoutingWorkerCount : undefined, + // Short-comp band attribution — see the field docs. Emitted on every + // render so the fleet element-count distribution is readable, and so a + // perf shift can be split into "the new band did it" vs "unchanged". + compositionElementCount, + deShortBand: deShortBandApplies + ? deShortBandOpen + ? "applied" + : "skipped_elements" + : undefined, // Same rationale as the counters above: carried on live capture // observability, not only the success-path perfSummary, so a crash / // OOM / timeout still reports which GPU backend it happened on. That @@ -3524,6 +3600,12 @@ async function executeRenderPipeline(input: { clampReason: deClampReason, workerInversion: deWorkerInversion, preInversionWorkers: deWorkerInversion ? preRoutingWorkerCount : undefined, + compositionElementCount, + shortBand: deShortBandApplies + ? deShortBandOpen + ? "applied" + : "skipped_elements" + : undefined, parallelRouter: deParallelRouter, preRouterWorkers: deParallelRouter ? preRoutingWorkerCount : undefined, selfVerifyFallback: deSelfVerifyFallback, From e9de2fa14f61f503287c9d87ec0487c165ecbcba Mon Sep 17 00:00:00 2001 From: Vance Ingalls Date: Wed, 29 Jul 2026 10:02:05 -0700 Subject: [PATCH 2/6] refactor(producer): make the short-comp band baseline-first and its attribution decisive MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Recut after pre-registering the read exposed two flaws in the first cut: 1. Attribution was wrong. de_short_band keyed on frame count + element ceiling alone, so a webm render, a compile-gated comp, or a forced screenshot at 400f reported "applied" while its routing was untouched — poisoning the measurement cohort with unaffected renders and diluting any effect toward zero. Now the predicate is evaluated twice (900 floor vs band floor) and the band is DECISIVE only when the calls disagree: every other eligibility condition passed and only the floor differed. The cohort contains exactly the renders whose routing the band decides. 2. A same-release flip is unfalsifiable. composition_element_count ships WITH the routing change, so the before-period cannot be filtered to the same cohort as the after-period — the comparison would show a speedup even if the change did nothing (the after-cohort excludes big comps by construction; the before-cohort includes them). Routing is therefore gated behind HF_DE_SHORT_BAND_ROUTE, default OFF: this release computes and emits the full band decision on every render ("applied" is the counterfactual "would have inverted"), a follow-up flips the default. Identical cohort selector on both sides of the boundary, and the skipped/oversize renders in the same frame band form a concurrent control — a difference-in-differences that absorbs secular drift (content mix, version-correlated populations, hardware), which a plain before/after cannot. Also: countElementTags now counts HTML void elements. Counting only closers read an image gallery as a tiny comp and opened the band on exactly the content most likely to lose it (images skew expensive to paint). Opening tags stay uncounted — inline scripts' `a < b` would false-positive. Counter semantics are frozen while the baseline is read: the distribution the baseline release records must be measured by the same counter that later gates. Revert-rate baseline for the pre-registered read, measured over 14d fleet-wide: the 900+ inversion runs 31,756 inverted / 1,705 reverted = 5.1%. At the benched 1.16-1.24x win and ~1.8x revert cost, expected net for the band is ~12%. Kill criteria for the flip release: DiD <= 0, in-band revert rate > 5.1% baseline, or band fallback rate > DE baseline. Co-Authored-By: Claude Opus 5 (1M context) --- .../src/services/render/observability.ts | 18 +-- .../src/services/render/perfSummary.ts | 2 +- .../src/services/renderOrchestrator.test.ts | 112 ++++++++++-------- .../src/services/renderOrchestrator.ts | 78 +++++++----- 4 files changed, 126 insertions(+), 84 deletions(-) diff --git a/packages/producer/src/services/render/observability.ts b/packages/producer/src/services/render/observability.ts index dfe0bc2a40..55b96961f1 100644 --- a/packages/producer/src/services/render/observability.ts +++ b/packages/producer/src/services/render/observability.ts @@ -81,13 +81,17 @@ export interface RenderCaptureObservability { */ compositionElementCount?: number; /** - * Why the short-comp band did or did not apply to this render: - * "applied" (frames landed in 250-899 and the element count cleared the - * ceiling), "skipped_elements" (band was open by frame count but the comp - * was too large), or undefined when the frame count made the band - * irrelevant either way. Distinguishes "the new routing chose this" from - * "the pre-existing 900 floor chose this" — otherwise a fleet perf shift is - * unattributable. + * Short-comp band decision, emitted only when the band is DECISIVE — every + * other inversion-eligibility condition passed and only the floor (250 vs + * 900) differed. "applied": the element count cleared the ceiling too, so + * with routing enabled (HF_DE_SHORT_BAND_ROUTE) this render inverts; in the + * baseline release the same value is the COUNTERFACTUAL "would have + * inverted". "skipped_elements": the element ceiling was the only blocker. + * Unset: the band could not have affected this render (ineligible for some + * other reason, or already inverting at 900+). The selector is computed + * identically before and after the routing flip, and the skipped/oversize + * renders form the concurrent control for the difference-in-differences + * read — that is the entire point of the field. */ deShortBand?: "applied" | "skipped_elements"; /** DE parallel-router outcome: "routed" (fired, held) | "reverted" (fired, self-verify retry rolled back). */ diff --git a/packages/producer/src/services/render/perfSummary.ts b/packages/producer/src/services/render/perfSummary.ts index 0770d58ee7..2735329301 100644 --- a/packages/producer/src/services/render/perfSummary.ts +++ b/packages/producer/src/services/render/perfSummary.ts @@ -82,7 +82,7 @@ export interface DrawElementPerfInput { preInversionWorkers?: number; /** Rough compiled-composition element count — gate variable for the short-comp inversion band. */ compositionElementCount?: number; - /** Short-comp band attribution: "applied" | "skipped_elements"; unset when the band was irrelevant. */ + /** Short-comp band decision when the band was DECISIVE: "applied" (inverts once HF_DE_SHORT_BAND_ROUTE is on; counterfactual in the baseline release) | "skipped_elements" (element ceiling was the only blocker); unset when the band could not have affected this render. */ shortBand?: string; parallelRouter?: "routed" | "reverted"; /** Auto-resolved worker count before the router pinned it to 3 (set only when the router fired). */ diff --git a/packages/producer/src/services/renderOrchestrator.test.ts b/packages/producer/src/services/renderOrchestrator.test.ts index 16536f213f..2b25e5e4ae 100644 --- a/packages/producer/src/services/renderOrchestrator.test.ts +++ b/packages/producer/src/services/renderOrchestrator.test.ts @@ -1695,63 +1695,67 @@ describe("shouldPreferSingleWorkerDrawElement (DE priority inversion)", () => { expect(shouldPreferSingleWorkerDrawElement(eligible)).toBe(true); }); - // ── Short-comp band ──────────────────────────────────────────────────── - // The band lowers the effective floor from 900 to 250 for SMALL comps only. - // The predicate itself is unchanged — the call site picks the floor — so - // these pin the arithmetic the call site performs. + // ── Short-comp band ────────────────────────────────────────────────────────────────────── + // The call site evaluates the predicate TWICE — once at the 900 floor, once + // at the 250 band floor — and the band is DECISIVE only when the calls + // disagree. These pin that arithmetic, including the property the design + // depends on: the band can only ADD inversions, never remove one. // - // Measured basis (400f, single-DE vs parallel-screenshot-W4, 2 reps, ratio - // = ss4/de1 so >1 means DE wins): - // 24 movers / 0 nodes -> 1.05 DE wins - // 320 movers / 0 nodes -> 1.24 DE wins - // 320 movers / 7000 nodes -> 1.09 DE wins - // 24 movers / 7000 nodes -> 0.96 DE LOSES - // 24 movers / 20000 nodes -> 0.71 DE loses badly - // 24 movers / 40000 nodes -> 0.55 DE loses very badly - // Motion helps DE, DOM size punishes it; the ceiling is calibrated at the - // lowest-motion case so every higher-motion comp is covered too. - describe("short-comp band floor arithmetic", () => { - const shortBandFloor = (elementCount: number, maxElements = 2500): number => - elementCount <= maxElements ? Math.min(900, 250) : 900; - - it("opens the 250-frame floor for a small comp", () => { - expect(shortBandFloor(800)).toBe(250); - expect( - shouldPreferSingleWorkerDrawElement({ - ...eligible, - totalFrames: 400, - minFrames: shortBandFloor(800), - }), - ).toBe(true); + // Measured basis (400f, single-DE vs parallel-screenshot-W4, ratio = ss4/de1 + // so >1 means DE wins): + // 24 movers / 0 nodes -> 1.05 | 24 movers / 7000 nodes -> 0.96 + // 320 movers / 0 nodes -> 1.24 | 24 movers / 20000 nodes -> 0.71 + // 320 movers / 7000 nodes -> 1.09 | 24 movers / 40000 nodes -> 0.55 + // Motion helps DE, DOM size punishes it; the element ceiling is calibrated + // at the lowest-motion case so every higher-motion comp is covered too. + describe("short-comp band decisiveness (two-floor evaluation)", () => { + const BAND_FLOOR = Math.min(900, 250); + const atBase = (totalFrames: number, over?: Partial) => + shouldPreferSingleWorkerDrawElement({ ...eligible, ...over, totalFrames, minFrames: 900 }); + const atBand = (totalFrames: number, over?: Partial) => + shouldPreferSingleWorkerDrawElement({ + ...eligible, + ...over, + totalFrames, + minFrames: BAND_FLOOR, + }); + + it("is decisive exactly in the 250-899 window for an otherwise-eligible render", () => { + expect(atBand(400) && !atBase(400)).toBe(true); + expect(atBand(250) && !atBase(250)).toBe(true); + expect(atBand(899) && !atBase(899)).toBe(true); }); - it("keeps the 900-frame floor for a large comp — the measured 1.8x regression case", () => { - expect(shortBandFloor(40000)).toBe(900); - expect( - shouldPreferSingleWorkerDrawElement({ - ...eligible, - totalFrames: 400, - minFrames: shortBandFloor(40000), - }), - ).toBe(false); + it("is NOT decisive below the band floor — nothing fires either way", () => { + expect(atBand(200)).toBe(false); + expect(atBase(200)).toBe(false); }); - it("never RAISES the floor: a large comp at 900+ frames still inverts as it did before", () => { - expect( - shouldPreferSingleWorkerDrawElement({ - ...eligible, - totalFrames: 2380, - minFrames: shortBandFloor(40000), - }), - ).toBe(true); + it("is NOT decisive at 900+ — the pre-existing floor already inverts, unchanged", () => { + expect(atBase(2380)).toBe(true); + expect(atBand(2380) && !atBase(2380)).toBe(false); + }); + + it("is NOT decisive when the render is ineligible for any other reason — the attribution cohort must exclude renders the band cannot affect", () => { + for (const over of [ + { requestedWorkers: 3 as const }, + { useDrawElement: false }, + { deCompileGate: "css_effect:filter" }, + { forceScreenshot: true }, + { outputFormat: "webm" as const }, + { singleWorkerStreamingOk: false }, + { probeDeGated: true }, + ]) { + expect(atBand(400, over)).toBe(false); + } }); - it("leaves comps below the band floor alone", () => { + it("HF_DE_SHORT_MIN_FRAMES=0 disables via the predicate's own minFrames guard", () => { expect( shouldPreferSingleWorkerDrawElement({ ...eligible, - totalFrames: 200, - minFrames: shortBandFloor(800), + totalFrames: 400, + minFrames: Math.min(900, 0), }), ).toBe(false); }); @@ -1762,8 +1766,18 @@ describe("shouldPreferSingleWorkerDrawElement (DE priority inversion)", () => { expect(countElementTags("
a
")).toBe(2); }); - it("undercounts void elements — biases the count DOWN, so the ceiling must stay conservative", () => { - expect(countElementTags("
")).toBe(0); + it("counts void elements — an image gallery must not read as a tiny comp", () => { + expect(countElementTags("
")).toBe(3); + expect(countElementTags('')).toBe(2); + }); + + it("does not false-positive on inline-script comparisons or void-prefixed words", () => { + // Only the closer counts: "if (a < b && x ")).toBe( + 1, + ); }); it("is stable on empty and malformed input rather than throwing", () => { diff --git a/packages/producer/src/services/renderOrchestrator.ts b/packages/producer/src/services/renderOrchestrator.ts index 2369bf2c02..acb8cd74a9 100644 --- a/packages/producer/src/services/renderOrchestrator.ts +++ b/packages/producer/src/services/renderOrchestrator.ts @@ -1247,13 +1247,22 @@ export function envInt(name: string, fallback: number): number { * routing decision, and a full linkedom parse of the exact documents that * matter here — the 20k-40k node ones — is the most expensive case. Precision * is not needed. It feeds a threshold whose measured crossover is ~3.9k and - * whose default sits at 2500, so counting closing tags is comfortably inside - * the margin. Undercounts void elements (``, `
`) and self-closing - * SVG nodes, which biases the count DOWN — the direction that opens the band - * — so the ceiling is the thing to keep conservative. + * whose default sits at 2500, so tag counting is comfortably inside the + * margin. Closing tags plus HTML void elements (``, `
`, …) — voids + * matter because they skew EXPENSIVE to paint (images), and counting only + * closers would read an image gallery as a tiny comp and open the band on + * exactly the content most likely to lose it. Opening tags are deliberately + * NOT counted: compiled comps embed inline scripts where `a < b` would + * false-positive. Self-closing SVG children still undercount; acceptable. + * + * These semantics are FROZEN while the short-band baseline is being read — + * the fleet distribution recorded by the baseline release must be measured + * by the same counter that later gates routing, or the baseline is invalid. */ export function countElementTags(html: string): number { - const matches = html.match(/<\/[a-zA-Z]/g); + const matches = html.match( + /<\/[a-zA-Z]|<(?:img|br|hr|input|source|track|area|base|col|embed|link|meta|param|wbr)\b/gi, + ); return matches === null ? 0 : matches.length; } @@ -2462,20 +2471,21 @@ async function executeRenderPipeline(input: { deShortBandMinFrames > 0 && deShortBandMaxElements > 0 && compositionElementCount <= deShortBandMaxElements; - const deEffectiveMinFrames = deShortBandOpen - ? Math.min(deSingleMinFrames, deShortBandMinFrames) - : deSingleMinFrames; - // Does the band even matter for this render? Only when the frame count - // falls in the newly-opened window — at or above the original floor the - // inversion fires regardless, and below `deShortBandMinFrames` nothing - // fires either way. Keeps the telemetry reason from claiming credit (or - // blame) on renders the change could not have affected. - const deShortBandApplies = - totalFrames >= deShortBandMinFrames && totalFrames < deSingleMinFrames; + // Baseline-first sequencing: this release EVALUATES the band on every + // render and emits the decision, but only routes on it when + // HF_DE_SHORT_BAND_ROUTE=true (flipped by default in a follow-up release). + // The point is a difference-in-differences read: the cohort selector + // (`de_short_band`) is computed identically before and after the flip — + // "applied" is counterfactual in the baseline release and factual after — + // and the skipped/oversize renders in the same frame band form a + // concurrent control that absorbs secular drift (content mix, version- + // correlated populations, hardware). A plain before/after cannot + // attribute a fleet perf shift to this change; this can. + const deShortBandRoute = process.env.HF_DE_SHORT_BAND_ROUTE === "true"; // "Would ANY multi-worker resolution be inverted?" — if workers resolve // to 1 naturally the outcome is identical either way. const WOULD_RESOLVE_MULTI_WORKER = 2; - const deInversionEligible = shouldPreferSingleWorkerDrawElement({ + const deInversionArgs = { workerCount: WOULD_RESOLVE_MULTI_WORKER, requestedWorkers: job.config.workers, useDrawElement: cfg.useDrawElement, @@ -2483,7 +2493,7 @@ async function executeRenderPipeline(input: { forceScreenshot: captureForceScreenshot, outputFormat, totalFrames, - minFrames: deEffectiveMinFrames, + minFrames: deSingleMinFrames, singleWorkerStreamingOk: shouldUseStreamingEncode(cfg, outputFormat, 1, job.duration), layeredOrEffectRoute: hasHdrContent || compiled.hasShaderTransitions, supersampling: deviceScaleFactor > 1, @@ -2495,7 +2505,29 @@ async function executeRenderPipeline(input: { process.env.PRODUCER_EXPERIMENTAL_FAST_CAPTURE === "true" || // Verified parallel DE streaming (opt-in) wants its parallelism kept. process.env.HF_DE_PARALLEL_STREAM === "true", + }; + const invertAtBaseFloor = shouldPreferSingleWorkerDrawElement(deInversionArgs); + // Same render, same eligibility, band floor instead of 900. Math.min so a + // user override of HF_DE_SINGLE_MIN_FRAMES below the band floor keeps + // winning; HF_DE_SHORT_MIN_FRAMES=0 disables via the predicate's own + // minFrames > 0 check. + const invertAtBandFloor = shouldPreferSingleWorkerDrawElement({ + ...deInversionArgs, + minFrames: Math.min(deSingleMinFrames, deShortBandMinFrames), }); + // The band is DECISIVE only when it alone flips the decision — every + // other eligibility condition already passed and only the floor differed. + // Renders the band could not have affected (ineligible for any other + // reason, or already inverting at 900+) stay unset, so the telemetry + // cohort contains exactly the renders whose routing this change decides. + const deShortBandDecisive = invertAtBandFloor && !invertAtBaseFloor; + const deShortBand = deShortBandDecisive + ? deShortBandOpen + ? ("applied" as const) + : ("skipped_elements" as const) + : undefined; + const deInversionEligible = + deShortBandRoute && deShortBand === "applied" ? invertAtBandFloor : invertAtBaseFloor; // DE parallel-router eligibility — see shouldPreferParallelDrawElement. // Default-off (HF_DE_PARALLEL_ROUTER); HF_DE_PARALLEL_MIN_FRAMES default // 700, re-calibrated 2026-07-27 from the original safe-high 2000. A @@ -2749,11 +2781,7 @@ async function executeRenderPipeline(input: { // render so the fleet element-count distribution is readable, and so a // perf shift can be split into "the new band did it" vs "unchanged". compositionElementCount, - deShortBand: deShortBandApplies - ? deShortBandOpen - ? "applied" - : "skipped_elements" - : undefined, + deShortBand, // Same rationale as the counters above: carried on live capture // observability, not only the success-path perfSummary, so a crash / // OOM / timeout still reports which GPU backend it happened on. That @@ -3601,11 +3629,7 @@ async function executeRenderPipeline(input: { workerInversion: deWorkerInversion, preInversionWorkers: deWorkerInversion ? preRoutingWorkerCount : undefined, compositionElementCount, - shortBand: deShortBandApplies - ? deShortBandOpen - ? "applied" - : "skipped_elements" - : undefined, + shortBand: deShortBand, parallelRouter: deParallelRouter, preRouterWorkers: deParallelRouter ? preRoutingWorkerCount : undefined, selfVerifyFallback: deSelfVerifyFallback, From 23854f7c6a328ef1f6913160af345c9fda70b036 Mon Sep 17 00:00:00 2001 From: Vance Ingalls Date: Wed, 29 Jul 2026 10:54:24 -0700 Subject: [PATCH 3/6] =?UTF-8?q?feat(producer):=20surface=20init=20telemetr?= =?UTF-8?q?y=20from=20parallel=20workers=20=E2=80=94=20the=20band's=20miss?= =?UTF-8?q?ing=20motion=20axis?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The routing surface the short-comp benchmarks validated is (motion x DOM size x frames). After the baseline release, fleet telemetry carries DOM size (composition_element_count) and frames on every render — but the motion proxy, observability_init_tween_count, has 0% coverage on the exact renders the band routes: parallel workers' console buffers (and so the [FrameCapture:INIT] line the summary parses) only propagate to the orchestrator on FAILURE. Single-worker screenshot renders report it; the multi-worker clamp bucket never does. Verified against 7d of fleet data: 35k screenshot renders carry tween counts, 0 of 9,600 band renders. Fix rides the one channel parallel workers already return on success — the per-worker CapturePerfSummary. Sessions record initTelemetry on every init path; the perf summary now carries it; the orchestrator max-merges across workers (same multi-session semantics the console parser uses) and feeds it to the observability summary as a structured fallback, console lines still refining when present. With this, every band render carries full coordinates — (elements, tweens, frames, path, speed) — which buys two reads: regressing wild DE speed against element count on the existing 900+ inversions validates the bench's 0.50ms/element slope BEFORE the routing flip, and any post-flip misroute can be reproduced locally by feeding its telemetry row straight into gen-crossover-comp's knobs (--movers ~ tween count, --static ~ element count) and re-benching. (Also drops a now-stale fallow suppression in render.ts — the test-only reset export it guarded gained real test importers, so the issue it suppressed no longer exists and the gate flags the leftover.) Co-Authored-By: Claude Opus 5 (1M context) --- packages/cli/src/commands/render.ts | 1 - packages/engine/src/services/frameCapture.ts | 2 ++ packages/engine/src/types.ts | 12 +++++++ .../src/services/render/observability.test.ts | 31 +++++++++++++++++ .../src/services/render/observability.ts | 34 ++++++++++++------- .../src/services/renderOrchestrator.test.ts | 18 ++++++++++ .../src/services/renderOrchestrator.ts | 31 +++++++++++++++++ 7 files changed, 115 insertions(+), 14 deletions(-) diff --git a/packages/cli/src/commands/render.ts b/packages/cli/src/commands/render.ts index 6dcb77be88..f80711fa61 100644 --- a/packages/cli/src/commands/render.ts +++ b/packages/cli/src/commands/render.ts @@ -1109,7 +1109,6 @@ let deParallelRouterTrialFiredThisProcess = false; * resetting outside a test process where many independent test cases share * one imported module instance. */ -// fallow-ignore-next-line unused-export export function __resetDeParallelRouterTrialStateForTests(): void { deParallelRouterTrialManagedByUs = false; deParallelRouterTrialFiredThisProcess = false; diff --git a/packages/engine/src/services/frameCapture.ts b/packages/engine/src/services/frameCapture.ts index 4751f7a053..0a8e9bc55c 100644 --- a/packages/engine/src/services/frameCapture.ts +++ b/packages/engine/src/services/frameCapture.ts @@ -3786,6 +3786,8 @@ export function getCapturePerfSummary(session: CaptureSession): CapturePerfSumma p95TotalMs: percentileOf(session.capturePerf.frameMs, 0.95), p99TotalMs: percentileOf(session.capturePerf.frameMs, 0.99), subTimelineWaitOutcome: session.subTimelineWaitOutcome, + initDurationMs: session.initTelemetry?.initDurationMs, + initTweenCount: session.initTelemetry?.tweenCount, warnings: cloneCaptureWarnings(session.warnings), staticDedupReused: session.staticDedupCount ?? 0, staticDedupEnabled: session.staticDedupEnabled ?? false, diff --git a/packages/engine/src/types.ts b/packages/engine/src/types.ts index 4f30c1efba..dd5faa5e6b 100644 --- a/packages/engine/src/types.ts +++ b/packages/engine/src/types.ts @@ -241,6 +241,18 @@ export interface CapturePerfSummary { p99TotalMs: number; /** Sub-composition timeline wait outcome (absent pre-init). */ subTimelineWaitOutcome?: SubTimelineWaitOutcome; + /** + * Session init telemetry, mirrored from the `[FrameCapture:INIT]` console + * line so PARALLEL workers report it too: worker sessions' console buffers + * only propagate to the orchestrator on failure, which left the + * multi-worker path — the short-comp band's entire population — with 0% + * coverage of the motion axis (`observability_init_tween_count`) in fleet + * telemetry. Riding the perf summary reuses the one channel that already + * flows back per worker on success. + */ + initDurationMs?: number; + /** GSAP tween count at init — the motion-axis signal for capture routing analysis. */ + initTweenCount?: number; /** Correctness warnings observed before or during capture. */ warnings?: CaptureWarning[]; /** diff --git a/packages/producer/src/services/render/observability.test.ts b/packages/producer/src/services/render/observability.test.ts index 7e52898609..3e9070c63d 100644 --- a/packages/producer/src/services/render/observability.test.ts +++ b/packages/producer/src/services/render/observability.test.ts @@ -407,3 +407,34 @@ describe("RenderObservabilityRecorder", () => { ); }); }); + +describe("init observability fallback (parallel workers)", () => { + const makeRecorder = () => + new RenderObservabilityRecorder({ renderJobId: "render-par", pipelineStartMs: Date.now() }); + + it("uses the structured fallback when the console has no INIT line — the parallel success path", () => { + const summary = makeRecorder().summary({ + lastBrowserConsole: ["[FrameCapture:NAV] page.goto start"], + capture: { forceScreenshot: false, captureMode: "screenshot" }, + initFallback: { initDurationMs: 850, tweenCount: 1200 }, + }); + expect(summary.init).toEqual({ initDurationMs: 850, tweenCount: 1200 }); + }); + + it("max-merges console INIT lines over the fallback, matching multi-session semantics", () => { + const summary = makeRecorder().summary({ + lastBrowserConsole: ["[FrameCapture:INIT] complete initDurationMs=1234 tweenCount=42"], + capture: { forceScreenshot: false, captureMode: "screenshot" }, + initFallback: { initDurationMs: 850, tweenCount: 1200 }, + }); + expect(summary.init).toEqual({ initDurationMs: 1234, tweenCount: 1200 }); + }); + + it("stays undefined when neither source has anything", () => { + const summary = makeRecorder().summary({ + lastBrowserConsole: [], + capture: { forceScreenshot: false, captureMode: "screenshot" }, + }); + expect(summary.init).toBeUndefined(); + }); +}); diff --git a/packages/producer/src/services/render/observability.ts b/packages/producer/src/services/render/observability.ts index 55b96961f1..0996f60007 100644 --- a/packages/producer/src/services/render/observability.ts +++ b/packages/producer/src/services/render/observability.ts @@ -270,20 +270,26 @@ function readUnsignedIntAfter(line: string, prefix: string): number | undefined return digits > 0 ? value : undefined; } -function summarizeInitObservability(lines: string[]): RenderInitObservability | undefined { - let initDurationMs: number | undefined; - let tweenCount: number | undefined; +/** Max of two optional readings — multiple worker/session INIT records can appear; keep the worst. */ +function maxReading(current: number | undefined, next: number | undefined): number | undefined { + if (next === undefined) return current; + return current === undefined ? next : Math.max(current, next); +} + +function summarizeInitObservability( + lines: string[], + fallback?: RenderInitObservability, +): RenderInitObservability | undefined { + // Console parsing only sees THIS process's session buffer, so parallel + // workers' INIT lines never reach it — their init telemetry arrives + // structured via the per-worker perf summaries instead. Seed with that and + // let the console parse (same max semantics) refine it. + let initDurationMs: number | undefined = fallback?.initDurationMs; + let tweenCount: number | undefined = fallback?.tweenCount; for (const line of lines) { if (!line.includes("[FrameCapture:INIT]")) continue; - const duration = readUnsignedIntAfter(line, "initDurationMs="); - const tweens = readUnsignedIntAfter(line, "tweenCount="); - // Multiple worker/session INIT records can appear; keep the worst observed startup cost. - if (duration !== undefined) { - initDurationMs = initDurationMs === undefined ? duration : Math.max(initDurationMs, duration); - } - if (tweens !== undefined) { - tweenCount = tweenCount === undefined ? tweens : Math.max(tweenCount, tweens); - } + initDurationMs = maxReading(initDurationMs, readUnsignedIntAfter(line, "initDurationMs=")); + tweenCount = maxReading(tweenCount, readUnsignedIntAfter(line, "tweenCount=")); } if (initDurationMs === undefined && tweenCount === undefined) return undefined; return { initDurationMs, tweenCount }; @@ -402,6 +408,8 @@ export class RenderObservabilityRecorder { summary(input: { lastBrowserConsole: string[]; capture: RenderCaptureObservability; + /** Structured init telemetry from per-worker perf summaries — the only success-path channel parallel workers have (their console buffers propagate on failure only). */ + initFallback?: RenderInitObservability; extraction?: RenderExtractionObservability; compositionHash?: string; }): RenderObservabilitySummary { @@ -416,7 +424,7 @@ export class RenderObservabilityRecorder { browserDiagnostics: summarizeBrowserDiagnostics(input.lastBrowserConsole), capture: { ...input.capture }, extraction: input.extraction ? { ...input.extraction } : undefined, - init: summarizeInitObservability(input.lastBrowserConsole), + init: summarizeInitObservability(input.lastBrowserConsole, input.initFallback), }; } diff --git a/packages/producer/src/services/renderOrchestrator.test.ts b/packages/producer/src/services/renderOrchestrator.test.ts index 2b25e5e4ae..942299337a 100644 --- a/packages/producer/src/services/renderOrchestrator.test.ts +++ b/packages/producer/src/services/renderOrchestrator.test.ts @@ -36,6 +36,7 @@ import { shouldRetryViaPinnedFallback, countElementTags, envInt, + mergeWorkerInitObservability, shouldPreferParallelDrawElement, shouldPreferSingleWorkerDrawElement, shouldStreamParallelCapture, @@ -1761,6 +1762,23 @@ describe("shouldPreferSingleWorkerDrawElement (DE priority inversion)", () => { }); }); + describe("mergeWorkerInitObservability", () => { + it("max-merges across workers and ignores workers that reported nothing", () => { + expect( + mergeWorkerInitObservability([ + { initDurationMs: 400, initTweenCount: 900 }, + {}, + { initDurationMs: 1250, initTweenCount: 880 }, + ]), + ).toEqual({ initDurationMs: 1250, tweenCount: 900 }); + }); + + it("returns undefined when no worker reported — summary.init must stay absent, not zeroed", () => { + expect(mergeWorkerInitObservability([])).toBeUndefined(); + expect(mergeWorkerInitObservability([{}, {}])).toBeUndefined(); + }); + }); + describe("countElementTags", () => { it("counts closing tags", () => { expect(countElementTags("
a
")).toBe(2); diff --git a/packages/producer/src/services/renderOrchestrator.ts b/packages/producer/src/services/renderOrchestrator.ts index acb8cd74a9..114f37fcf3 100644 --- a/packages/producer/src/services/renderOrchestrator.ts +++ b/packages/producer/src/services/renderOrchestrator.ts @@ -1266,6 +1266,35 @@ export function countElementTags(html: string): number { return matches === null ? 0 : matches.length; } +/** + * Max-merge init telemetry across per-worker capture perf summaries — the + * success-path channel for PARALLEL renders, whose worker console buffers + * (and so the `[FrameCapture:INIT]` line) only propagate on failure. Max + * matches summarizeInitObservability's own multi-session semantics: keep the + * worst observed startup cost, and tween count is per-composition so any + * worker's reading is the reading. + */ +export function mergeWorkerInitObservability( + perfs: ReadonlyArray<{ initDurationMs?: number; initTweenCount?: number }>, +): { initDurationMs?: number; tweenCount?: number } | undefined { + let initDurationMs: number | undefined; + let tweenCount: number | undefined; + for (const perf of perfs) { + if (perf.initDurationMs !== undefined) { + initDurationMs = + initDurationMs === undefined + ? perf.initDurationMs + : Math.max(initDurationMs, perf.initDurationMs); + } + if (perf.initTweenCount !== undefined) { + tweenCount = + tweenCount === undefined ? perf.initTweenCount : Math.max(tweenCount, perf.initTweenCount); + } + } + if (initDurationMs === undefined && tweenCount === undefined) return undefined; + return { initDurationMs, tweenCount }; +} + /** * DE priority inversion predicate: should an AUTO-resolved multi-worker render * drop to single-worker verified drawElement streaming? @@ -3600,6 +3629,7 @@ async function executeRenderPipeline(input: { const observabilitySummary = observability.summary({ lastBrowserConsole, capture: captureObservability, + initFallback: mergeWorkerInitObservability(dedupPerfs), extraction: extractionObservability, compositionHash, }); @@ -3761,6 +3791,7 @@ async function executeRenderPipeline(input: { const observabilitySummary = observability.summary({ lastBrowserConsole, capture: captureObservability, + initFallback: mergeWorkerInitObservability(dedupPerfs), extraction: extractionObservability, compositionHash, }); From 7bb9e3cbf920e04d01854a839d01619cb3615ab4 Mon Sep 17 00:00:00 2001 From: Vance Ingalls Date: Wed, 29 Jul 2026 11:26:41 -0700 Subject: [PATCH 4/6] =?UTF-8?q?fix(producer):=20address=20R2=20review=20fi?= =?UTF-8?q?ndings=20=E2=80=94=20element=20undercount=20and=20kill-switch?= =?UTF-8?q?=20attribution?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two review-blocking issues from Miguel's R2 pass (both confirmed by running the counterexamples directly): 1. countElementTags still undercounted unboundedly. The void-element fix covered HTML tags but SVG elements (, , ...) are neither closing-tag-shaped nor in the HTML void list, so "".repeat(40000) reported 0 — the same failure class as the original counterexample, and the exact shape of comp the measured 1.8x regression case is made of. A 2500 ceiling cannot bound an error with no bound of its own. Added a third alternative matching any self-closing tag; verified it doesn't false-positive on the adversarial minified-JS case (unspaced "", which reads like a tag open but never contains the literal two-char "/>" the alt requires). 2. HF_DE_SHORT_MAX_ELEMENTS=0 (the documented kill switch) still reported deShortBand: "skipped_elements" for every in-band render instead of undefined — attributing "comp too large" when the real cause was "band disabled," which would have polluted the DiD control cohort with kill-switched renders and made the post-flip read look like the ceiling was too tight. Extracted the attribution logic into resolveDeShortBand(), a pure function gated on bandEnabled (deShortBandMaxElements > 0) as well as decisiveness — and made it independently unit-testable, since the inline version could only be exercised by a full render pipeline run. Also from this review round: the inversion log line could report "400 frames >= 900" for a band-routed inversion; it now names the floor that actually decided the render. Tightened shortBand's type to match its peer fields' unions (workerInversion, parallelRouter) instead of a bare string. Clarified the tween-count merge docblock, which claimed workers always agree (semantically true) while the code takes a defensive max (in case one doesn't) — the two aren't in conflict, but the comment read as if they were. Co-Authored-By: Claude Opus 5 (1M context) --- .../src/services/render/perfSummary.ts | 2 +- .../src/services/renderOrchestrator.test.ts | 88 +++++++++++++++ .../src/services/renderOrchestrator.ts | 100 ++++++++++++++---- 3 files changed, 166 insertions(+), 24 deletions(-) diff --git a/packages/producer/src/services/render/perfSummary.ts b/packages/producer/src/services/render/perfSummary.ts index 2735329301..d3f224267d 100644 --- a/packages/producer/src/services/render/perfSummary.ts +++ b/packages/producer/src/services/render/perfSummary.ts @@ -83,7 +83,7 @@ export interface DrawElementPerfInput { /** Rough compiled-composition element count — gate variable for the short-comp inversion band. */ compositionElementCount?: number; /** Short-comp band decision when the band was DECISIVE: "applied" (inverts once HF_DE_SHORT_BAND_ROUTE is on; counterfactual in the baseline release) | "skipped_elements" (element ceiling was the only blocker); unset when the band could not have affected this render. */ - shortBand?: string; + shortBand?: "applied" | "skipped_elements"; parallelRouter?: "routed" | "reverted"; /** Auto-resolved worker count before the router pinned it to 3 (set only when the router fired). */ preRouterWorkers?: number; diff --git a/packages/producer/src/services/renderOrchestrator.test.ts b/packages/producer/src/services/renderOrchestrator.test.ts index 942299337a..6f3a198e56 100644 --- a/packages/producer/src/services/renderOrchestrator.test.ts +++ b/packages/producer/src/services/renderOrchestrator.test.ts @@ -37,6 +37,7 @@ import { countElementTags, envInt, mergeWorkerInitObservability, + resolveDeShortBand, shouldPreferParallelDrawElement, shouldPreferSingleWorkerDrawElement, shouldStreamParallelCapture, @@ -1762,6 +1763,67 @@ describe("shouldPreferSingleWorkerDrawElement (DE priority inversion)", () => { }); }); + describe("resolveDeShortBand", () => { + it("reports applied only when decisive and the element ceiling cleared", () => { + expect( + resolveDeShortBand({ + invertAtBaseFloor: false, + invertAtBandFloor: true, + bandEnabled: true, + bandOpen: true, + }), + ).toBe("applied"); + }); + + it("reports skipped_elements when decisive but the comp is oversized", () => { + expect( + resolveDeShortBand({ + invertAtBaseFloor: false, + invertAtBandFloor: true, + bandEnabled: true, + bandOpen: false, + }), + ).toBe("skipped_elements"); + }); + + it("is undefined when the base floor already inverts — the band changed nothing", () => { + expect( + resolveDeShortBand({ + invertAtBaseFloor: true, + invertAtBandFloor: true, + bandEnabled: true, + bandOpen: true, + }), + ).toBeUndefined(); + }); + + it("is undefined when neither floor inverts — the render was ineligible for some other reason", () => { + expect( + resolveDeShortBand({ + invertAtBaseFloor: false, + invertAtBandFloor: false, + bandEnabled: true, + bandOpen: true, + }), + ).toBeUndefined(); + }); + + // Review finding (R1 + R2, both reviewers): HF_DE_SHORT_MAX_ELEMENTS=0 + // must read as "band disabled" (undefined), never "comp too large" + // (skipped_elements) — the latter would poison the DiD control cohort by + // mislabeling a kill-switch event as a real oversize measurement. + it("HF_DE_SHORT_MAX_ELEMENTS=0 (bandEnabled=false) reports undefined even when the render would otherwise be decisive", () => { + expect( + resolveDeShortBand({ + invertAtBaseFloor: false, + invertAtBandFloor: true, // an otherwise-eligible in-band render + bandEnabled: false, // the kill switch + bandOpen: false, // deShortBandOpen also false when the switch is off + }), + ).toBeUndefined(); + }); + }); + describe("mergeWorkerInitObservability", () => { it("max-merges across workers and ignores workers that reported nothing", () => { expect( @@ -1789,6 +1851,32 @@ describe("shouldPreferSingleWorkerDrawElement (DE priority inversion)", () => { expect(countElementTags('')).toBe(2); }); + // Review-flagged blocker (v1): SVG elements are neither closing-tag-shaped + // nor in the HTML void list, so a self-closing-SVG-heavy comp read as + // element count 0 — an UNBOUNDED undercount, the same failure class as + // the original counterexample, and the exact shape of comp the + // measured 1.8x regression case is made of. The ceiling cannot bound an + // error that has no bound of its own. + it("counts self-closing SVG elements — the 40k-node regression case must not read as empty", () => { + expect(countElementTags("".repeat(40000))).toBe(40000); + expect(countElementTags('')).toBe(1); + expect(countElementTags("")).toBe(1); + }); + + it("does not double-count a self-closed void element (still just 1)", () => { + expect(countElementTags('')).toBe(1); + expect(countElementTags('')).toBe(1); + }); + + it("does not false-positive on minified JS division-after-comparison (the self-closing alt's real risk)", () => { + // Unspaced "" is the adversarial case: "<" IS immediately + // followed by a letter, so the generic self-closing alt gets as far as + // starting a match — but it still requires the literal two-char "/>" + // sequence, and here a "c" sits between the "/" and the ">", so + // backtracking never finds one and it correctly fails to match. + expect(countElementTags("if(ad){}")).toBe(0); + }); + it("does not false-positive on inline-script comparisons or void-prefixed words", () => { // Only the closer counts: "`, `
`, …) — voids - * matter because they skew EXPENSIVE to paint (images), and counting only - * closers would read an image gallery as a tiny comp and open the band on - * exactly the content most likely to lose it. Opening tags are deliberately - * NOT counted: compiled comps embed inline scripts where `a < b` would - * false-positive. Self-closing SVG children still undercount; acceptable. + * margin — PROVIDED the count is not unboundedly low for some real content + * shape. Three sources are counted, each catching a case the others miss: + * + * 1. Closing tags (``) — the base count for ordinary HTML. + * 2. Named HTML void elements (``, `
`, …), bare or self-closed — + * voids matter because they skew EXPENSIVE to paint (images), and + * counting only closers would read an image gallery as a tiny comp and + * open the band on exactly the content most likely to lose it. + * 3. Any self-closing tag (``, ``) — SVG's own + * elements are neither closing-tag-shaped nor in the void list, so + * without this a self-closing-SVG-heavy composition (`` x 40k) + * counted as ZERO — an unbounded undercount, not a rounding error, and + * exactly the shape of comp the 1.8x regression case is made of + * (review finding: the ceiling cannot compensate for an error with no + * bound). + * + * Opening (non-self-closing, non-void) tags are deliberately NOT counted: + * compiled comps embed inline scripts, and `a < b` or `x `), so + * ordinary JS comparisons and divisions don't qualify — verified by test. * * These semantics are FROZEN while the short-band baseline is being read — * the fleet distribution recorded by the baseline release must be measured @@ -1261,7 +1276,7 @@ export function envInt(name: string, fallback: number): number { */ export function countElementTags(html: string): number { const matches = html.match( - /<\/[a-zA-Z]|<(?:img|br|hr|input|source|track|area|base|col|embed|link|meta|param|wbr)\b/gi, + /<\/[a-zA-Z]|<(?:img|br|hr|input|source|track|area|base|col|embed|link|meta|param|wbr)\b|<[a-zA-Z][-a-zA-Z0-9]*\b[^>]*\/>/gi, ); return matches === null ? 0 : matches.length; } @@ -1271,8 +1286,9 @@ export function countElementTags(html: string): number { * success-path channel for PARALLEL renders, whose worker console buffers * (and so the `[FrameCapture:INIT]` line) only propagate on failure. Max * matches summarizeInitObservability's own multi-session semantics: keep the - * worst observed startup cost, and tween count is per-composition so any - * worker's reading is the reading. + * worst observed startup cost for duration. Tween count is per-composition, + * so workers should agree — max is a defensive read against a worker that + * initializes before the timeline is fully wired, not an expected disagreement. */ export function mergeWorkerInitObservability( perfs: ReadonlyArray<{ initDurationMs?: number; initTweenCount?: number }>, @@ -1295,6 +1311,34 @@ export function mergeWorkerInitObservability( return { initDurationMs, tweenCount }; } +/** + * The short-comp band's attribution decision, extracted as a pure function so + * the gating fixes below are independently testable rather than living inline + * where only a full render pipeline run could exercise them. + * + * "applied" / "skipped_elements" are emitted ONLY when the band is DECISIVE — + * every other inversion-eligibility condition already passed (both floor + * evaluations agree on everything except which floor they used) and the band + * floor alone flipped the answer. `bandEnabled` gates that decisiveness + * itself: `HF_DE_SHORT_MAX_ELEMENTS=0` is a documented kill switch (symmetric + * with `HF_DE_SHORT_MIN_FRAMES=0`, which already disables via the predicate's + * own `minFrames > 0` guard), and without this gate a fired kill switch left + * every in-band render decisive against a real floor comparison — reporting + * "skipped_elements" (comp too large) instead of undefined (band disabled) + * and corrupting the DiD control cohort with kill-switched renders (review + * finding). + */ +export function resolveDeShortBand(args: { + invertAtBaseFloor: boolean; + invertAtBandFloor: boolean; + bandEnabled: boolean; + bandOpen: boolean; +}): "applied" | "skipped_elements" | undefined { + const decisive = args.bandEnabled && args.invertAtBandFloor && !args.invertAtBaseFloor; + if (!decisive) return undefined; + return args.bandOpen ? "applied" : "skipped_elements"; +} + /** * DE priority inversion predicate: should an AUTO-resolved multi-worker render * drop to single-worker verified drawElement streaming? @@ -2496,9 +2540,17 @@ async function executeRenderPipeline(input: { const deShortBandMinFrames = envInt("HF_DE_SHORT_MIN_FRAMES", 250); const deShortBandMaxElements = envInt("HF_DE_SHORT_MAX_ELEMENTS", 2500); const compositionElementCount = countElementTags(compiled.html); + // HF_DE_SHORT_MAX_ELEMENTS=0 is the documented kill switch (symmetric + // with HF_DE_SHORT_MIN_FRAMES=0, which disables via the predicate's own + // minFrames > 0 guard). Gated explicitly here too — without it, a fired + // max-elements kill switch left every in-band render decisive against a + // real floor comparison, so it reported "skipped_elements" (comp too + // large) instead of undefined (band disabled), corrupting the DiD + // control cohort with kill-switched renders (review finding). + const deShortBandEnabled = deShortBandMaxElements > 0; const deShortBandOpen = + deShortBandEnabled && deShortBandMinFrames > 0 && - deShortBandMaxElements > 0 && compositionElementCount <= deShortBandMaxElements; // Baseline-first sequencing: this release EVALUATES the band on every // render and emits the decision, but only routes on it when @@ -2544,19 +2596,21 @@ async function executeRenderPipeline(input: { ...deInversionArgs, minFrames: Math.min(deSingleMinFrames, deShortBandMinFrames), }); - // The band is DECISIVE only when it alone flips the decision — every - // other eligibility condition already passed and only the floor differed. - // Renders the band could not have affected (ineligible for any other - // reason, or already inverting at 900+) stay unset, so the telemetry - // cohort contains exactly the renders whose routing this change decides. - const deShortBandDecisive = invertAtBandFloor && !invertAtBaseFloor; - const deShortBand = deShortBandDecisive - ? deShortBandOpen - ? ("applied" as const) - : ("skipped_elements" as const) - : undefined; + const deShortBand = resolveDeShortBand({ + invertAtBaseFloor, + invertAtBandFloor, + bandEnabled: deShortBandEnabled, + bandOpen: deShortBandOpen, + }); const deInversionEligible = deShortBandRoute && deShortBand === "applied" ? invertAtBandFloor : invertAtBaseFloor; + // The floor that actually decided this render — for the human-facing log + // below, so it never claims e.g. "400 frames >= 900" for a band-routed + // inversion (review finding). + const deInversionEffectiveMinFrames = + deShortBandRoute && deShortBand === "applied" + ? Math.min(deSingleMinFrames, deShortBandMinFrames) + : deSingleMinFrames; // DE parallel-router eligibility — see shouldPreferParallelDrawElement. // Default-off (HF_DE_PARALLEL_ROUTER); HF_DE_PARALLEL_MIN_FRAMES default // 700, re-calibrated 2026-07-27 from the original safe-high 2000. A @@ -2790,7 +2844,7 @@ async function executeRenderPipeline(input: { log.info( "[Render] Fast capture: single-worker drawElement streaming preferred over " + `${workerCount}-worker screenshot capture (${totalFrames} frames >= ` + - `${deSingleMinFrames}; verified path, measured faster at every worker count). ` + + `${deInversionEffectiveMinFrames}; verified path, measured faster at every worker count). ` + "Set HF_DE_SINGLE_MIN_FRAMES=0 or --workers N to override.", ); workerCount = 1; From def98f79c3fa746e805c6fbe9ee51392c267ea1e Mon Sep 17 00:00:00 2001 From: Vance Ingalls Date: Wed, 29 Jul 2026 16:40:27 -0700 Subject: [PATCH 5/6] fix(producer): count live DOM, not source markup, for the short-band gate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit R3 review finding: a string scan of compiled.html — however the regex is tuned — cannot see elements a composition's own script creates at runtime. The repo already has a production shape that hits this exactly: style-10-prod's per-transcript-word caption generator builds one per word via document.createElement, measuring 2 source tags against thousands of live nodes after init. That's the same unbounded-undercount failure class as the earlier /SVG counterexamples, but this one has no static-scan fix — the elements simply don't exist as tags in the string. resolveCompositionElementCount() now prefers the live DOM size, queried from the probe session that's already running for every render at this point in the pipeline (its Chrome gets reused for capture on the common single-worker path, so this costs one extra CDP evaluate, not a browser launch) once that session's init sequence has completed — session.page .evaluate(() => document.querySelectorAll("*").length) sees runtime- generated DOM the source scan never could. countElementTags remains as the fallback for the rare case with no initialized probe session (evaluate throws, session absent, or not yet initialized). Co-Authored-By: Claude Opus 5 (1M context) --- .../src/services/render/observability.ts | 6 ++- .../src/services/renderOrchestrator.test.ts | 41 +++++++++++++++ .../src/services/renderOrchestrator.ts | 50 +++++++++++++++++-- 3 files changed, 92 insertions(+), 5 deletions(-) diff --git a/packages/producer/src/services/render/observability.ts b/packages/producer/src/services/render/observability.ts index 0996f60007..d802ae12f0 100644 --- a/packages/producer/src/services/render/observability.ts +++ b/packages/producer/src/services/render/observability.ts @@ -71,7 +71,11 @@ export interface RenderCaptureObservability { /** Worker count the resolver would have used absent the inversion; undefined if it never fired. */ dePreInversionWorkers?: number; /** - * Rough element count of the compiled composition (`countElementTags`). + * Element count for the short-comp band gate (`resolveCompositionElementCount`): + * the LIVE DOM size from the already-running probe session when one is + * initialized, falling back to a static scan of the compiled HTML + * (`countElementTags`) otherwise. Live is authoritative — a static scan + * cannot see elements a composition's own script creates at runtime. * * Emitted on every render, not just inverted ones — this is the variable the * short-comp inversion band is gated on, and the fleet distribution of it is diff --git a/packages/producer/src/services/renderOrchestrator.test.ts b/packages/producer/src/services/renderOrchestrator.test.ts index 6f3a198e56..fe83d25f78 100644 --- a/packages/producer/src/services/renderOrchestrator.test.ts +++ b/packages/producer/src/services/renderOrchestrator.test.ts @@ -37,6 +37,7 @@ import { countElementTags, envInt, mergeWorkerInitObservability, + resolveCompositionElementCount, resolveDeShortBand, shouldPreferParallelDrawElement, shouldPreferSingleWorkerDrawElement, @@ -1896,6 +1897,46 @@ describe("shouldPreferSingleWorkerDrawElement (DE priority inversion)", () => { }); }); + describe("resolveCompositionElementCount", () => { + // Review finding (R3): a static scan of SOURCE markup cannot see DOM a + // composition's own script creates at runtime (document.createElement) — + // an unbounded undercount no regex can close. style-10-prod's real + // per-transcript-word caption generator is exactly this shape: 2 source + // tags, thousands of live nodes after init. These pin the fix: prefer the + // live count from an initialized probe session, static scan only as + // fallback. + it("uses the live DOM count from an initialized probe session, ignoring the (much smaller) source scan", async () => { + const session = { isInitialized: true, page: { evaluate: async () => 40001 } }; + expect(await resolveCompositionElementCount(session, "
")).toBe(40001); + }); + + it("falls back to the static scan when there is no probe session", async () => { + expect(await resolveCompositionElementCount(null, "
")).toBe(2); + }); + + it("falls back to the static scan when the probe session is not yet initialized", async () => { + const session = { isInitialized: false, page: { evaluate: async () => 999 } }; + expect(await resolveCompositionElementCount(session, "
")).toBe(1); + }); + + it("falls back to the static scan when page.evaluate throws (detached frame, mid-navigation)", async () => { + const session = { + isInitialized: true, + page: { + evaluate: async () => { + throw new Error("Execution context was destroyed"); + }, + }, + }; + expect(await resolveCompositionElementCount(session, "
")).toBe(2); + }); + + it("falls back to the static scan when evaluate resolves a non-finite value", async () => { + const session = { isInitialized: true, page: { evaluate: async () => Number.NaN } }; + expect(await resolveCompositionElementCount(session, "
")).toBe(1); + }); + }); + describe("envInt", () => { afterEach(() => { delete process.env.HF_TEST_ENV_INT; diff --git a/packages/producer/src/services/renderOrchestrator.ts b/packages/producer/src/services/renderOrchestrator.ts index 03fe724e40..6b88a9b29b 100644 --- a/packages/producer/src/services/renderOrchestrator.ts +++ b/packages/producer/src/services/renderOrchestrator.ts @@ -1270,9 +1270,13 @@ export function envInt(name: string, fallback: number): number { * literal closing marker (``), so * ordinary JS comparisons and divisions don't qualify — verified by test. * - * These semantics are FROZEN while the short-band baseline is being read — - * the fleet distribution recorded by the baseline release must be measured - * by the same counter that later gates routing, or the baseline is invalid. + * FALLBACK ONLY as of the live-DOM fix below — a string scan of the SOURCE + * markup cannot see elements a composition's own script creates at runtime + * (`document.createElement`), which is an unbounded undercount no regex can + * close: `style-10-prod`'s per-transcript-word caption generator measures 2 + * source tags against thousands of live nodes after init (review finding). + * `resolveCompositionElementCount` prefers the initialized probe session's + * live count and uses this only when no such session exists. */ export function countElementTags(html: string): number { const matches = html.match( @@ -1281,6 +1285,41 @@ export function countElementTags(html: string): number { return matches === null ? 0 : matches.length; } +/** + * Element count for the short-comp band's gate — prefers the LIVE DOM of the + * probe session that's already running for every render at this point in the + * pipeline (its Chrome is reused for capture on the common single-worker + * path, so this costs one extra CDP round-trip, not a browser launch) over + * the static `countElementTags` scan of `compiled.html`. The live count is + * the only one that sees runtime-generated DOM — a composition whose script + * builds its own elements after load (the caption-word-span pattern above) + * is otherwise measured as near-empty regardless of how the static scanner + * is tuned, admitting an arbitrarily large live DOM below the routing + * ceiling (review finding, R3). + * + * These semantics are FROZEN while the short-band baseline is being read — + * the fleet distribution recorded by the baseline release must be measured + * by the same resolver that later gates routing, or the baseline is invalid. + */ +export async function resolveCompositionElementCount( + probeSession: Pick | null, + html: string, +): Promise { + if (probeSession?.isInitialized) { + try { + const liveCount = await probeSession.page.evaluate( + () => document.querySelectorAll("*").length, + ); + if (typeof liveCount === "number" && Number.isFinite(liveCount)) return liveCount; + } catch { + // Probe page evaluate can fail (navigation mid-flight, detached frame, + // page crash) — fall through to the static scan rather than block the + // render on a routing-gate measurement. + } + } + return countElementTags(html); +} + /** * Max-merge init telemetry across per-worker capture perf summaries — the * success-path channel for PARALLEL renders, whose worker console buffers @@ -2539,7 +2578,10 @@ async function executeRenderPipeline(input: { // 900 floor stands, unchanged. const deShortBandMinFrames = envInt("HF_DE_SHORT_MIN_FRAMES", 250); const deShortBandMaxElements = envInt("HF_DE_SHORT_MAX_ELEMENTS", 2500); - const compositionElementCount = countElementTags(compiled.html); + const compositionElementCount = await resolveCompositionElementCount( + probeSession, + compiled.html, + ); // HF_DE_SHORT_MAX_ELEMENTS=0 is the documented kill switch (symmetric // with HF_DE_SHORT_MIN_FRAMES=0, which disables via the predicate's own // minFrames > 0 guard). Gated explicitly here too — without it, a fired From 4dbf0d90b05de2c3db7c983641c8785fd9988148 Mon Sep 17 00:00:00 2001 From: Vance Ingalls Date: Wed, 29 Jul 2026 17:02:26 -0700 Subject: [PATCH 6/6] fix(producer): fail closed when no live element count is available MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit R4 review finding, and the comment I wrote in R3 was simply wrong: the probe session is NOT running for every render. probeStage's needsBrowser gate launches one only for unknown duration, unresolved compositions, or specific media cases — and hasRuntimeInsertedMedia matches only createElement("video"|"audio"), never createElement("span"). So the exact shape that motivated the live-DOM fix (a known-duration, media-free caption comp building thousands of nodes in script) gets NO probe, falls back to the static source scan, reads as ~2 elements, and could enter the applied cohort at 40k live nodes. The R3 fix measured the right thing but only for the population that already had a probe. Now the count carries provenance and the band fails closed: - resolveCompositionElementCount returns { count, source: "live" | "static" }. Only "live" — an actual DOM measurement — may open the band. - resolveDeShortBand gains a third decisive outcome, "unmeasured", for the static case. It deliberately does NOT report skipped_elements: a static undercount is not a real oversize observation, and putting it in the control arm would contaminate the DiD just as putting it in the treatment arm would. Neither cohort; never routes. - composition_element_count_source ships alongside the count, so the fleet rate of "static" sizes the population a future conditional-probe-launch would unlock — which is the data PR B needs to decide whether that launch cost is worth paying. Regression coverage walks the real chain rather than a full render, using the production functions in pipeline order: probeRequiresBrowser (newly extracted from the inline needsBrowser expression, so the gate is testable at all) returns false for the caption-comp shape → the resolver reports static and a count under the ceiling → the band reports unmeasured, not applied. Fault injection confirms it bites: removing the one guard line fails exactly these three tests. Co-Authored-By: Claude Opus 5 (1M context) --- packages/cli/src/commands/render.ts | 1 + packages/cli/src/telemetry/events.ts | 4 + .../cli/src/telemetry/renderObservability.ts | 1 + .../src/services/render/observability.ts | 11 +- .../src/services/render/perfSummary.ts | 5 +- .../src/services/render/stages/probeStage.ts | 46 +++++- .../src/services/renderOrchestrator.test.ts | 153 ++++++++++++++++-- .../src/services/renderOrchestrator.ts | 88 ++++++---- 8 files changed, 259 insertions(+), 50 deletions(-) diff --git a/packages/cli/src/commands/render.ts b/packages/cli/src/commands/render.ts index f80711fa61..d832746369 100644 --- a/packages/cli/src/commands/render.ts +++ b/packages/cli/src/commands/render.ts @@ -1433,6 +1433,7 @@ function trackRenderMetrics( deWorkerInversion: perf?.drawElement?.workerInversion, dePreInversionWorkers: perf?.drawElement?.preInversionWorkers, compositionElementCount: perf?.drawElement?.compositionElementCount, + compositionElementCountSource: perf?.drawElement?.compositionElementCountSource, deShortBand: perf?.drawElement?.shortBand, deParallelRouter: perf?.drawElement?.parallelRouter, dePreRouterWorkers: perf?.drawElement?.preRouterWorkers, diff --git a/packages/cli/src/telemetry/events.ts b/packages/cli/src/telemetry/events.ts index b0d37aabbf..3bc3db5e84 100644 --- a/packages/cli/src/telemetry/events.ts +++ b/packages/cli/src/telemetry/events.ts @@ -73,6 +73,7 @@ export interface RenderObservabilityTelemetryPayload { captureDeWorkerInversion?: string; captureDePreInversionWorkers?: number; captureCompositionElementCount?: number; + captureCompositionElementCountSource?: string; captureDeShortBand?: string; captureDeParallelRouter?: string; captureDeGpuRenderer?: string; @@ -133,6 +134,7 @@ function renderObservabilityEventProperties(props: RenderObservabilityTelemetryP de_worker_inversion: props.captureDeWorkerInversion, de_pre_inversion_workers: props.captureDePreInversionWorkers, composition_element_count: props.captureCompositionElementCount, + composition_element_count_source: props.captureCompositionElementCountSource, de_short_band: props.captureDeShortBand, de_parallel_router: props.captureDeParallelRouter, gpu_renderer: props.captureDeGpuRenderer, @@ -209,6 +211,7 @@ export function trackRenderComplete( deWorkerInversion?: string; dePreInversionWorkers?: number; compositionElementCount?: number; + compositionElementCountSource?: string; deShortBand?: string; deParallelRouter?: string; dePreRouterWorkers?: number; @@ -310,6 +313,7 @@ export function trackRenderComplete( de_worker_inversion: props.deWorkerInversion, de_pre_inversion_workers: props.dePreInversionWorkers, composition_element_count: props.compositionElementCount, + composition_element_count_source: props.compositionElementCountSource, de_short_band: props.deShortBand, de_parallel_router: props.deParallelRouter, de_pre_router_workers: props.dePreRouterWorkers, diff --git a/packages/cli/src/telemetry/renderObservability.ts b/packages/cli/src/telemetry/renderObservability.ts index 10a1ad22f4..362837ae8a 100644 --- a/packages/cli/src/telemetry/renderObservability.ts +++ b/packages/cli/src/telemetry/renderObservability.ts @@ -43,6 +43,7 @@ export function renderObservabilityTelemetryPayload( captureDeWorkerInversion: capture.deWorkerInversion, captureDePreInversionWorkers: capture.dePreInversionWorkers, captureCompositionElementCount: capture.compositionElementCount, + captureCompositionElementCountSource: capture.compositionElementCountSource, captureDeShortBand: capture.deShortBand, captureDeParallelRouter: capture.deParallelRouter, captureDeGpuRenderer: capture.deGpuRenderer, diff --git a/packages/producer/src/services/render/observability.ts b/packages/producer/src/services/render/observability.ts index d802ae12f0..925f9fcfb0 100644 --- a/packages/producer/src/services/render/observability.ts +++ b/packages/producer/src/services/render/observability.ts @@ -84,6 +84,15 @@ export interface RenderCaptureObservability { * threshold from real content instead of synthetic sweeps. */ compositionElementCount?: number; + /** + * Provenance of `compositionElementCount`: "live" (measured from the probe + * session's real DOM — sees runtime-generated elements) or "static" (source + * markup scan, which does not). The probe is CONDITIONAL, so this is not a + * detail: only a `live` count may open the short band, and the fleet rate of + * "static" sizes the population a future conditional-probe-launch would + * unlock for the band. + */ + compositionElementCountSource?: "live" | "static"; /** * Short-comp band decision, emitted only when the band is DECISIVE — every * other inversion-eligibility condition passed and only the floor (250 vs @@ -97,7 +106,7 @@ export interface RenderCaptureObservability { * renders form the concurrent control for the difference-in-differences * read — that is the entire point of the field. */ - deShortBand?: "applied" | "skipped_elements"; + deShortBand?: "applied" | "skipped_elements" | "unmeasured"; /** DE parallel-router outcome: "routed" (fired, held) | "reverted" (fired, self-verify retry rolled back). */ deParallelRouter?: "routed" | "reverted"; /** diff --git a/packages/producer/src/services/render/perfSummary.ts b/packages/producer/src/services/render/perfSummary.ts index d3f224267d..e4f4db3fc8 100644 --- a/packages/producer/src/services/render/perfSummary.ts +++ b/packages/producer/src/services/render/perfSummary.ts @@ -82,8 +82,10 @@ export interface DrawElementPerfInput { preInversionWorkers?: number; /** Rough compiled-composition element count — gate variable for the short-comp inversion band. */ compositionElementCount?: number; + /** Provenance of the element count: "live" (probe DOM, trusted to gate) | "static" (source scan, not). */ + compositionElementCountSource?: "live" | "static"; /** Short-comp band decision when the band was DECISIVE: "applied" (inverts once HF_DE_SHORT_BAND_ROUTE is on; counterfactual in the baseline release) | "skipped_elements" (element ceiling was the only blocker); unset when the band could not have affected this render. */ - shortBand?: "applied" | "skipped_elements"; + shortBand?: "applied" | "skipped_elements" | "unmeasured"; parallelRouter?: "routed" | "reverted"; /** Auto-resolved worker count before the router pinned it to 3 (set only when the router fired). */ preRouterWorkers?: number; @@ -126,6 +128,7 @@ function aggregateDrawElement( workerInversion: de.workerInversion ?? "none", preInversionWorkers: de.preInversionWorkers, compositionElementCount: de.compositionElementCount, + compositionElementCountSource: de.compositionElementCountSource, shortBand: de.shortBand, parallelRouter: de.parallelRouter ?? "none", preRouterWorkers: de.preRouterWorkers, diff --git a/packages/producer/src/services/render/stages/probeStage.ts b/packages/producer/src/services/render/stages/probeStage.ts index 58142a30f0..8d19667509 100644 --- a/packages/producer/src/services/render/stages/probeStage.ts +++ b/packages/producer/src/services/render/stages/probeStage.ts @@ -184,6 +184,37 @@ function hasRuntimeInsertedMedia(html: string): boolean { ); } +/** + * Does this render need a browser probe at all? + * + * Extracted as a pure predicate because whether a probe runs decides whether + * a LIVE DOM element count is available downstream, and the short-comp + * inversion band fails closed without one (see + * `resolveCompositionElementCount` / `resolveDeShortBand`). Notably NONE of + * these conditions fire for a known-duration, media-free composition that + * builds thousands of `div`/`span` nodes in its own init script — + * `hasRuntimeInsertedMedia` matches only `createElement("video"|"audio")` — + * so that shape is measured statically and must never reach the band's + * `applied` cohort (review finding, R4). + */ +export function probeRequiresBrowser(args: { + durationSeconds: number; + unresolvedCompositionCount: number; + hasAutoStart: boolean; + hasScriptedAudio: boolean; + hasVariableMedia: boolean; + hasInsertedMedia: boolean; +}): boolean { + return ( + args.durationSeconds <= 0 || + args.unresolvedCompositionCount > 0 || + args.hasAutoStart || + args.hasScriptedAudio || + args.hasVariableMedia || + args.hasInsertedMedia + ); +} + export async function runProbeStage(input: ProbeStageInput): Promise { const { projectDir, @@ -217,13 +248,14 @@ export async function runProbeStage(input: ProbeStageInput): Promise 0 || - hasAutoStart || - hasScriptedAudio || - hasVariableMedia || - hasInsertedMedia; + const needsBrowser = probeRequiresBrowser({ + durationSeconds: composition.duration, + unresolvedCompositionCount: compiled.unresolvedCompositions.length, + hasAutoStart, + hasScriptedAudio, + hasVariableMedia, + hasInsertedMedia, + }); if (needsBrowser) { const reasons = []; diff --git a/packages/producer/src/services/renderOrchestrator.test.ts b/packages/producer/src/services/renderOrchestrator.test.ts index fe83d25f78..752e65d95d 100644 --- a/packages/producer/src/services/renderOrchestrator.test.ts +++ b/packages/producer/src/services/renderOrchestrator.test.ts @@ -44,6 +44,7 @@ import { shouldStreamParallelCapture, shouldUseStreamingEncode, } from "./renderOrchestrator.js"; +import { probeRequiresBrowser } from "./render/stages/probeStage.js"; import { ensureFrameWritten } from "./render/stages/captureHdrFrameShared.js"; import { resolveCompositeTransfer, shouldUseLayeredComposite } from "./hdrCompositor.js"; import { @@ -1765,9 +1766,12 @@ describe("shouldPreferSingleWorkerDrawElement (DE priority inversion)", () => { }); describe("resolveDeShortBand", () => { - it("reports applied only when decisive and the element ceiling cleared", () => { + const live = { elementCountSource: "live" as const }; + + it("reports applied only when decisive, live-measured, and under the ceiling", () => { expect( resolveDeShortBand({ + ...live, invertAtBaseFloor: false, invertAtBandFloor: true, bandEnabled: true, @@ -1776,9 +1780,10 @@ describe("shouldPreferSingleWorkerDrawElement (DE priority inversion)", () => { ).toBe("applied"); }); - it("reports skipped_elements when decisive but the comp is oversized", () => { + it("reports skipped_elements when live-measured and over the ceiling", () => { expect( resolveDeShortBand({ + ...live, invertAtBaseFloor: false, invertAtBandFloor: true, bandEnabled: true, @@ -1790,6 +1795,7 @@ describe("shouldPreferSingleWorkerDrawElement (DE priority inversion)", () => { it("is undefined when the base floor already inverts — the band changed nothing", () => { expect( resolveDeShortBand({ + ...live, invertAtBaseFloor: true, invertAtBandFloor: true, bandEnabled: true, @@ -1801,6 +1807,7 @@ describe("shouldPreferSingleWorkerDrawElement (DE priority inversion)", () => { it("is undefined when neither floor inverts — the render was ineligible for some other reason", () => { expect( resolveDeShortBand({ + ...live, invertAtBaseFloor: false, invertAtBandFloor: false, bandEnabled: true, @@ -1816,6 +1823,7 @@ describe("shouldPreferSingleWorkerDrawElement (DE priority inversion)", () => { it("HF_DE_SHORT_MAX_ELEMENTS=0 (bandEnabled=false) reports undefined even when the render would otherwise be decisive", () => { expect( resolveDeShortBand({ + ...live, invertAtBaseFloor: false, invertAtBandFloor: true, // an otherwise-eligible in-band render bandEnabled: false, // the kill switch @@ -1823,6 +1831,105 @@ describe("shouldPreferSingleWorkerDrawElement (DE priority inversion)", () => { }), ).toBeUndefined(); }); + + // Review finding (R4): the probe supplying the live count is conditional, + // so a static count is an UNBOUNDED undercount on runtime-generated DOM. + // It must never produce "applied" (would route a 40k-node comp) and must + // never produce "skipped_elements" either (would contaminate the DiD + // control cohort with a number that isn't a real oversize observation). + it("fails closed to unmeasured when the count came from the static scan, never applied", () => { + expect( + resolveDeShortBand({ + elementCountSource: "static", + invertAtBaseFloor: false, + invertAtBandFloor: true, + bandEnabled: true, + bandOpen: true, // static scan said "small" — must NOT be believed + }), + ).toBe("unmeasured"); + }); + + it("reports unmeasured (not skipped_elements) for a static count over the ceiling — it is not a real observation either", () => { + expect( + resolveDeShortBand({ + elementCountSource: "static", + invertAtBaseFloor: false, + invertAtBandFloor: true, + bandEnabled: true, + bandOpen: false, + }), + ).toBe("unmeasured"); + }); + + it("stays undefined for a static count when the band was not decisive anyway", () => { + expect( + resolveDeShortBand({ + elementCountSource: "static", + invertAtBaseFloor: true, + invertAtBandFloor: true, + bandEnabled: true, + bandOpen: true, + }), + ).toBeUndefined(); + }); + }); + + // Review finding (R4), end-to-end: the review asked for a regression with a + // known duration, no media / unresolved compositions, and >2500 + // script-created nodes, asserting it cannot enter `applied` without a live + // count. This walks the real decision chain rather than a full render — + // the probe gate, the count resolver, and the band attribution are each the + // production function, wired in the same order the pipeline wires them. + describe("no-probe dynamic-DOM composition cannot enter the applied cohort (R4 regression)", () => { + // A caption-style comp: known duration, no media, builds 4000 spans in + // its own init script. Mirrors packages/producer/tests/style-10-prod. + const DYNAMIC_DOM_HTML = [ + '
', + "", + ].join("\n"); + + it("gets no browser probe — none of the probe conditions fire for this shape", () => { + expect( + probeRequiresBrowser({ + durationSeconds: 13.3, // known + unresolvedCompositionCount: 0, // resolved + hasAutoStart: false, // no media + hasScriptedAudio: false, + hasVariableMedia: false, + // createElement("span") is not createElement("video"|"audio") + hasInsertedMedia: false, + }), + ).toBe(false); + }); + + it("therefore measures statically, and the static count wildly understates the live DOM", async () => { + const resolved = await resolveCompositionElementCount(null, DYNAMIC_DOM_HTML); + expect(resolved.source).toBe("static"); + // Source markup has a handful of tags; the live DOM would have 4000+. + expect(resolved.count).toBeLessThan(2500); + }); + + it("and therefore reports unmeasured — never applied — so it cannot route or join either cohort", async () => { + const { count, source } = await resolveCompositionElementCount(null, DYNAMIC_DOM_HTML); + const bandOpen = source === "live" && count <= 2500; + const band = resolveDeShortBand({ + // A 400-frame render that would otherwise be perfectly eligible. + invertAtBaseFloor: false, + invertAtBandFloor: true, + bandEnabled: true, + bandOpen, + elementCountSource: source, + }); + expect(band).toBe("unmeasured"); + expect(band).not.toBe("applied"); + }); }); describe("mergeWorkerInitObservability", () => { @@ -1902,24 +2009,36 @@ describe("shouldPreferSingleWorkerDrawElement (DE priority inversion)", () => { // composition's own script creates at runtime (document.createElement) — // an unbounded undercount no regex can close. style-10-prod's real // per-transcript-word caption generator is exactly this shape: 2 source - // tags, thousands of live nodes after init. These pin the fix: prefer the - // live count from an initialized probe session, static scan only as - // fallback. + // tags, thousands of live nodes after init. + // + // Review finding (R4): the probe that provides the live count is + // CONDITIONAL, so the fallback cases below are not merely "less precise" + // — they are UNSAFE to gate on, and every one of them must report + // provenance "static" so the caller can fail closed. it("uses the live DOM count from an initialized probe session, ignoring the (much smaller) source scan", async () => { const session = { isInitialized: true, page: { evaluate: async () => 40001 } }; - expect(await resolveCompositionElementCount(session, "
")).toBe(40001); + expect(await resolveCompositionElementCount(session, "
")).toEqual({ + count: 40001, + source: "live", + }); }); - it("falls back to the static scan when there is no probe session", async () => { - expect(await resolveCompositionElementCount(null, "
")).toBe(2); + it("reports static provenance when there is no probe session", async () => { + expect(await resolveCompositionElementCount(null, "
")).toEqual({ + count: 2, + source: "static", + }); }); - it("falls back to the static scan when the probe session is not yet initialized", async () => { + it("reports static provenance when the probe session is not yet initialized", async () => { const session = { isInitialized: false, page: { evaluate: async () => 999 } }; - expect(await resolveCompositionElementCount(session, "
")).toBe(1); + expect(await resolveCompositionElementCount(session, "
")).toEqual({ + count: 1, + source: "static", + }); }); - it("falls back to the static scan when page.evaluate throws (detached frame, mid-navigation)", async () => { + it("reports static provenance when page.evaluate throws (detached frame, mid-navigation)", async () => { const session = { isInitialized: true, page: { @@ -1928,12 +2047,18 @@ describe("shouldPreferSingleWorkerDrawElement (DE priority inversion)", () => { }, }, }; - expect(await resolveCompositionElementCount(session, "
")).toBe(2); + expect(await resolveCompositionElementCount(session, "
")).toEqual({ + count: 2, + source: "static", + }); }); - it("falls back to the static scan when evaluate resolves a non-finite value", async () => { + it("reports static provenance when evaluate resolves a non-finite value", async () => { const session = { isInitialized: true, page: { evaluate: async () => Number.NaN } }; - expect(await resolveCompositionElementCount(session, "
")).toBe(1); + expect(await resolveCompositionElementCount(session, "
")).toEqual({ + count: 1, + source: "static", + }); }); }); diff --git a/packages/producer/src/services/renderOrchestrator.ts b/packages/producer/src/services/renderOrchestrator.ts index 6b88a9b29b..a24e36eda8 100644 --- a/packages/producer/src/services/renderOrchestrator.ts +++ b/packages/producer/src/services/renderOrchestrator.ts @@ -494,8 +494,10 @@ export interface RenderPerfSummary { preInversionWorkers?: number; /** Rough compiled-composition element count — the variable the short-comp inversion band is gated on. Always set. */ compositionElementCount?: number; - /** Short-comp band attribution: "applied" | "skipped_elements"; unset when the frame count made the band irrelevant. */ - shortBand?: "applied" | "skipped_elements"; + /** Rough compiled-composition element-count provenance: "live" (probe DOM) | "static" (source scan, not trusted to open the band). */ + compositionElementCountSource?: "live" | "static"; + /** Short-comp band attribution: "applied" | "skipped_elements" | "unmeasured"; unset when the frame count made the band irrelevant. */ + shortBand?: "applied" | "skipped_elements" | "unmeasured"; /** DE parallel-router outcome: "routed" (fired, held), "reverted" (fired, self-verify retry rolled back), "none". Mutually exclusive with workerInversion. */ parallelRouter?: string; /** Worker count the auto-resolution chose BEFORE the router pinned it to 3 — the single-worker-inversion counterfactual. Only set when the router fired. */ @@ -1286,16 +1288,22 @@ export function countElementTags(html: string): number { } /** - * Element count for the short-comp band's gate — prefers the LIVE DOM of the - * probe session that's already running for every render at this point in the - * pipeline (its Chrome is reused for capture on the common single-worker - * path, so this costs one extra CDP round-trip, not a browser launch) over - * the static `countElementTags` scan of `compiled.html`. The live count is - * the only one that sees runtime-generated DOM — a composition whose script - * builds its own elements after load (the caption-word-span pattern above) - * is otherwise measured as near-empty regardless of how the static scanner - * is tuned, admitting an arbitrarily large live DOM below the routing - * ceiling (review finding, R3). + * Element count for the short-comp band's gate, WITH PROVENANCE. + * + * `live` — measured from the initialized probe session's real DOM. This is + * the only trustworthy source: it sees elements a composition's own script + * created after load, which no scan of the source markup can (the + * caption-word-span pattern above builds thousands of nodes from two source + * tags). + * + * `static` — the `countElementTags` fallback. Emitted for diagnostics, but + * NOT trusted to open the band: the probe is conditional (see + * `probeStage.ts`'s `needsBrowser` — only unknown duration, unresolved + * compositions, or specific media cases launch one), so a known-duration, + * media-free composition that builds 40k nodes in script gets no probe, and + * a static count that says "2". Treating that as measured would admit + * exactly the regression case the ceiling exists to exclude (review finding, + * R4). The caller fails closed on anything but `live`. * * These semantics are FROZEN while the short-band baseline is being read — * the fleet distribution recorded by the baseline release must be measured @@ -1304,20 +1312,22 @@ export function countElementTags(html: string): number { export async function resolveCompositionElementCount( probeSession: Pick | null, html: string, -): Promise { +): Promise<{ count: number; source: "live" | "static" }> { if (probeSession?.isInitialized) { try { const liveCount = await probeSession.page.evaluate( () => document.querySelectorAll("*").length, ); - if (typeof liveCount === "number" && Number.isFinite(liveCount)) return liveCount; + if (typeof liveCount === "number" && Number.isFinite(liveCount)) { + return { count: liveCount, source: "live" }; + } } catch { // Probe page evaluate can fail (navigation mid-flight, detached frame, // page crash) — fall through to the static scan rather than block the // render on a routing-gate measurement. } } - return countElementTags(html); + return { count: countElementTags(html), source: "static" }; } /** @@ -1355,26 +1365,43 @@ export function mergeWorkerInitObservability( * the gating fixes below are independently testable rather than living inline * where only a full render pipeline run could exercise them. * - * "applied" / "skipped_elements" are emitted ONLY when the band is DECISIVE — - * every other inversion-eligibility condition already passed (both floor - * evaluations agree on everything except which floor they used) and the band - * floor alone flipped the answer. `bandEnabled` gates that decisiveness - * itself: `HF_DE_SHORT_MAX_ELEMENTS=0` is a documented kill switch (symmetric - * with `HF_DE_SHORT_MIN_FRAMES=0`, which already disables via the predicate's - * own `minFrames > 0` guard), and without this gate a fired kill switch left + * A value is emitted ONLY when the band is DECISIVE — every other + * inversion-eligibility condition already passed (both floor evaluations + * agree on everything except which floor they used) and the band floor alone + * flipped the answer. `bandEnabled` gates that decisiveness itself: + * `HF_DE_SHORT_MAX_ELEMENTS=0` is a documented kill switch (symmetric with + * `HF_DE_SHORT_MIN_FRAMES=0`, which already disables via the predicate's own + * `minFrames > 0` guard), and without this gate a fired kill switch left * every in-band render decisive against a real floor comparison — reporting * "skipped_elements" (comp too large) instead of undefined (band disabled) * and corrupting the DiD control cohort with kill-switched renders (review * finding). + * + * Three decisive outcomes, and the distinction between the last two is the + * point: + * "applied" — measured LIVE and under the ceiling. Only this + * routes (once HF_DE_SHORT_BAND_ROUTE is on) and only + * this joins the treatment cohort. + * "skipped_elements" — measured live, over the ceiling. A real oversize + * observation; the DiD control group. + * "unmeasured" — no live DOM count available (no probe session ran; + * see `resolveCompositionElementCount`). FAILS CLOSED: + * never routes, and kept out of BOTH cohorts so a + * static undercount cannot masquerade as a small comp + * (review finding, R4). Emitted rather than dropped + * because its fleet rate sizes the population a + * future conditional-probe-launch would unlock. */ export function resolveDeShortBand(args: { invertAtBaseFloor: boolean; invertAtBandFloor: boolean; bandEnabled: boolean; bandOpen: boolean; -}): "applied" | "skipped_elements" | undefined { + elementCountSource: "live" | "static"; +}): "applied" | "skipped_elements" | "unmeasured" | undefined { const decisive = args.bandEnabled && args.invertAtBandFloor && !args.invertAtBaseFloor; if (!decisive) return undefined; + if (args.elementCountSource !== "live") return "unmeasured"; return args.bandOpen ? "applied" : "skipped_elements"; } @@ -2578,10 +2605,13 @@ async function executeRenderPipeline(input: { // 900 floor stands, unchanged. const deShortBandMinFrames = envInt("HF_DE_SHORT_MIN_FRAMES", 250); const deShortBandMaxElements = envInt("HF_DE_SHORT_MAX_ELEMENTS", 2500); - const compositionElementCount = await resolveCompositionElementCount( - probeSession, - compiled.html, - ); + // `source` is load-bearing, not diagnostic: the probe is CONDITIONAL + // (probeStage's `needsBrowser` — unknown duration, unresolved + // compositions, or specific media cases), so a known-duration media-free + // comp that builds its DOM in script has no live count available and the + // static scan reads it as tiny. Only a `live` count may open the band. + const { count: compositionElementCount, source: compositionElementCountSource } = + await resolveCompositionElementCount(probeSession, compiled.html); // HF_DE_SHORT_MAX_ELEMENTS=0 is the documented kill switch (symmetric // with HF_DE_SHORT_MIN_FRAMES=0, which disables via the predicate's own // minFrames > 0 guard). Gated explicitly here too — without it, a fired @@ -2593,6 +2623,7 @@ async function executeRenderPipeline(input: { const deShortBandOpen = deShortBandEnabled && deShortBandMinFrames > 0 && + compositionElementCountSource === "live" && compositionElementCount <= deShortBandMaxElements; // Baseline-first sequencing: this release EVALUATES the band on every // render and emits the decision, but only routes on it when @@ -2643,6 +2674,7 @@ async function executeRenderPipeline(input: { invertAtBandFloor, bandEnabled: deShortBandEnabled, bandOpen: deShortBandOpen, + elementCountSource: compositionElementCountSource, }); const deInversionEligible = deShortBandRoute && deShortBand === "applied" ? invertAtBandFloor : invertAtBaseFloor; @@ -2906,6 +2938,7 @@ async function executeRenderPipeline(input: { // render so the fleet element-count distribution is readable, and so a // perf shift can be split into "the new band did it" vs "unchanged". compositionElementCount, + compositionElementCountSource, deShortBand, // Same rationale as the counters above: carried on live capture // observability, not only the success-path perfSummary, so a crash / @@ -3755,6 +3788,7 @@ async function executeRenderPipeline(input: { workerInversion: deWorkerInversion, preInversionWorkers: deWorkerInversion ? preRoutingWorkerCount : undefined, compositionElementCount, + compositionElementCountSource, shortBand: deShortBand, parallelRouter: deParallelRouter, preRouterWorkers: deParallelRouter ? preRoutingWorkerCount : undefined,