feat(producer): short-comp DE inversion band — baseline release (telemetry only, routing off) - #2875
Conversation
…der an element ceiling
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) <noreply@anthropic.com>
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Reviewed at 0749cd9.
The measurement-first framing is exactly the shape this kind of change should have: 2×2 isolation to attribute the flip, ceiling calibrated at the lowest-motion cell (so higher-motion cells are covered for free), a "predicate untouched, call site picks the floor" delta that can only ADD inversions inside the new band, and a pre-registered read + kill-by-patch escape hatch. Instrumentation on both perfSummary and RenderCaptureObservability (both DE observability producers per prior HF pattern) — matches the dual-path contract. LGTM overall — two concerns worth surfacing:
Concerns
Merge-order dependency on #2874 is load-bearing. #2874 mirrors deParallelRouterTrialFired into a wipe-surviving state file because #2875 needs it for the newly-exposed population. If this PR lands before #2874, a machine that trips the breaker inside the new short-comp band loses that safety fact on the next rm -rf ~/.hyperframes, and gets re-enrolled into the failing path — the exact case #2874 exists to prevent, applied to a population that didn't have it before. Vance calls this out in #2874's body but not here. Consider adding an explicit "do not merge until #2874 is in main" gate to this PR body, or block via GitHub's merge queue if that's how the team stacks these. Not a code change — just a merge-time invariant.
Kill-switch attribution reports skipped_elements incorrectly. In renderOrchestrator.ts around line 2745 (perfSummary path) and 3600 (RenderCaptureObservability path), deShortBand: deShortBandApplies ? (deShortBandOpen ? "applied" : "skipped_elements") : undefined. If an operator sets HF_DE_SHORT_MAX_ELEMENTS=0 (or HF_DE_SHORT_MIN_FRAMES=0) to kill-switch the band, deShortBandOpen becomes false via the > 0 guard, and every render inside the frame band reports deShortBand: "skipped_elements" — telemetry attributes the miss to "comp too large" when the real cause is "band disabled." Small blast radius (only matters if the kill switch actually fires in the wild, and if it does fire fleet-wide everyone knows why), but the pre-registered read compares applied vs skipped_elements cohorts, and skipped-by-kill-switch would poison the "skipped" bucket in a way the doc doesn't cover. Consider a third "disabled" bucket, or gating deShortBand on deShortBandMinFrames > 0 && deShortBandMaxElements > 0 at the outer conditional so kill-switched renders report as undefined (band irrelevant) instead of skipped_elements.
Nits
countElementTagscounts closing tags via/<\/[a-zA-Z]/g— great choice for cost. Worth noting the noise sources you're bounded against: HTML comments containing</tag>and<script>blocks containing tag strings both inflate the count (biases toward NOT opening the band = safe direction). Fine given the ~3.9k measured crossover vs 2500 default ceiling.envIntacceptsNumber("2500.5") === 2500.5as valid (finite non-integer). Element-count comparisons still work, but the docblock says "integer tuning knob." Not worth guarding; the surface is dev-only.- The new test block on
renderOrchestrator.test.tsre-implements the call-site arithmetic via a localshortBandFloorhelper — since the PR's stated invariant is "the predicate is untouched, the call site picks the floor," anchoring the test in the identicalMath.min(deSingleMinFrames, deShortBandMinFrames)expression fromexecuteRenderPipeline(rather than a helper the site doesn't export) leaves a small drift surface if the call-site formula ever changes. Minor.
What I didn't verify
- The 31% fleet number itself — trusted per PR body's
v0.7.78+24h claim. - The
1.8×regression math end-to-end; walked through the 2×2 table and the "0.50ms/element vs 0.22ms/element" wall-clock claim and it's directionally consistent. - Any downstream dashboard querying
composition_element_count/de_short_band— the wiring emits them correctly; consumption side is out of scope for this diff.
miga-heygen
left a comment
There was a problem hiding this comment.
SSOT — Single Source of Truth
Predicate unchanged. shouldPreferSingleWorkerDrawElement is character-for-character identical to its pre-PR state. The mechanism is confirmed: the predicate checks args.totalFrames >= args.minFrames, and the call site now passes deEffectiveMinFrames (which can be 250 instead of 900) as minFrames. The routing decision stays in one place.
countElementTags is the sole element counter for DE routing. No prior element/node counting function existed for this purpose. The only related counter is countAuthoredCompositionIds in compositionLoader.ts (counts host elements by ID — unrelated). Clean SSOT.
envInt is new but not yet the single parser. The existing HF_DE_SINGLE_MIN_FRAMES, HF_DE_PARALLEL_MIN_FRAMES, and HF_DE_PARALLEL_MIN_MEM_MB still use the inline 3-line pattern. The new short-band vars use envInt. Behavior is identical — consolidation candidate for follow-up.
Telemetry flow. compositionElementCount and deShortBand flow through the same chain as every other DE property (verified at all 6 hops, see Telemetry section).
No SSOT findings.
Correctness
countElementTags undercount bias. Counts closing tags via /<\/[a-zA-Z]/g. Void elements (<img>, <br>) are missed. Bias direction is DOWN → opens the band for borderline comps. The ceiling (default 2500) compensates — measured crossover is ~3900, so a 2500 ceiling on a downward-biased count has margin. Documented in JSDoc. ✓
envInt fallback behavior. Unset → fallback. Empty → fallback. Non-numeric → fallback. Explicit 0 → passes Number.isFinite, returns 0. All tested. ✓
Short band arithmetic — Math.min never raises the floor. deEffectiveMinFrames = deShortBandOpen ? Math.min(deSingleMinFrames, deShortBandMinFrames) : deSingleMinFrames. With defaults: Math.min(900, 250) = 250. If someone sets HF_DE_SHORT_MIN_FRAMES=1200: Math.min(900, 1200) = 900 — floor can never exceed original. ✓
Kill switches. Both work correctly:
HF_DE_SHORT_MAX_ELEMENTS=0: fails> 0check. Band never opens. ✓HF_DE_SHORT_MIN_FRAMES=0: fails> 0check. Band never opens. ✓
deShortBandApplies boundary. totalFrames >= 250 && totalFrames < 900 (defaults). At 900: false (existing path). At 899: true. At 250: true. At 249: false. Correctly identifies the newly-opened window only. ✓
Duplicate ternary. The deShortBand/shortBand ternary appears at two sites (observability + perfSummary). Both read deShortBandApplies and deShortBandOpen, which are const. Structurally cannot diverge. ✓
No correctness findings.
Telemetry Chain
Traced both fields through all six hops:
| Hop | File | compositionElementCount |
shortBand / deShortBand |
|---|---|---|---|
| 1 | observability.ts | compositionElementCount?: number |
deShortBand?: "applied" | "skipped_elements" |
| 2 | perfSummary.ts | compositionElementCount?: number |
shortBand?: string |
| 3 | renderOrchestrator.ts | compositionElementCount?: number |
shortBand?: string |
| 4 | renderObservability.ts | captureCompositionElementCount |
captureDeShortBand |
| 5 | events.ts | composition_element_count |
de_short_band |
| 6 | render.ts | perf?.drawElement?.compositionElementCount |
perf?.drawElement?.shortBand |
No gaps. Every hop present, names consistent through each transformation.
No shadowing. Both fields follow the established dual-emission pattern: spread first (fallback from observability), explicit properties after (win by object-literal override). Same layout as every other DE property in trackRenderComplete. No shadowing bug.
compositionElementCount is unconditional. Computed before any routing decision, emitted without gating. Every render reports element count. ✓
deShortBand is conditional only on band relevance. Set only when deShortBandApplies is true (frame count in 250-899 window). Outside that window: undefined. Avoids attributing unchanged-path renders to the new band. ✓
Standards
- No bare
as Tassertions in the diff. ✓ - No non-null assertions. ✓
Type widening (nit): shortBand?: string in DrawElementPerfInput — peer fields workerInversion and parallelRouter keep their union types ("inverted" | "reverted", "routed" | "reverted"). Could tighten to "applied" | "skipped_elements" for consistency. No runtime impact — field is only ever assigned those values or undefined.
Tests
Band arithmetic (4 tests): opens 250-frame floor for small comp, holds 900 for large comp (the 1.8x regression case), never raises the floor, ignores sub-band comps. ✓
countElementTags (4 tests): closing tags, void element undercount (documents bias), malformed input stability, scale test (40k elements). ✓
envInt (2 tests): fallback on unset/empty/non-numeric, explicit value including 0 as real disable. ✓
No significant missing edge cases.
CI
All checks pass (52 completed: 44 success, 8 expected-skipped). All 9 regression shards pass. ✓
Non-blocking notes
- Type tightening:
shortBand?: stringinDrawElementPerfInput→"applied" | "skipped_elements"to match peer field convention. envIntconsolidation: The three pre-existing inline env-var parsers (HF_DE_SINGLE_MIN_FRAMES,HF_DE_PARALLEL_MIN_FRAMES,HF_DE_PARALLEL_MIN_MEM_MB) are candidates for migration toenvInt— reduces parser drift surface.
Verdict
Approve. The safety argument is sound. The element ceiling gates DE's short-band expansion to compositions where the measured performance advantage holds, and the kill switches work correctly. The telemetry chain is complete with no gaps or shadowing — fleet element-count distribution readable from day one, perf shifts attributable. The predicate is genuinely untouched; only the floor it receives changed.
Clean infrastructure for opening DE to 31% more of the fleet with the right safety rails.
-- Miga
miguel-heygen
left a comment
There was a problem hiding this comment.
R1 at 0749cd9ff83ad41c72c7dc6be1b224e9bc27731b. Two correctness issues prevent a stamp.
-
The element ceiling is not actually a ceiling.
countElementTags()counts only closing tags (/<\/[a-zA-Z]/g). Therefore<img>.repeat(40_000) — or a void/self-closing-heavy SVG/HTML composition — reports0elements and opens the short band. This is an unbounded undercount, while the PR's own measurements say a 40k-node composition is the ~1.8× regression case. A 2500 threshold cannot compensate for an error whose size is unbounded; the current test explicitly locks in the unsafe counterexample with<img><br><hr> === 0. Please use a cheap conservative count that includes void/self-closing elements (overcounting is safe here), or otherwise prove a bounded error below the measured margin. -
de_short_band=applieddoes not mean the band changed routing. It is derived only from frame range + element count, so it reportsappliedeven whendeInversionEligibleis false because of an explicit worker count, screenshot/DE/compile gates, format, supersampling, etc. Both kill switches also turn the same range intoskipped_elements, mislabeling “disabled” as “too large.” That contaminates the pre-registered applied-vs-skipped read the telemetry was added to support. Please derive attribution from the actual routing eligibility/outcome and make disabled either explicit or absent.
Non-blocking: the inversion log still prints ${totalFrames} frames >= ${deSingleMinFrames}; in the new band that can literally say 400 >= 900. It should report the effective floor.
All exact-head checks are green (45 success, 8 expected skips), so these are code/measurement-contract findings rather than CI noise.
Verdict: REQUEST CHANGES
Reasoning: The current counter can admit arbitrarily large DOMs into the measured regression path, and the rollout telemetry cannot reliably distinguish affected renders from unaffected or disabled ones.
— Magi
…ttribution decisive
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) <noreply@anthropic.com>
…nd's missing motion axis 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) <noreply@anthropic.com>
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Reviewed at 23854f7 (delta from R1's 0749cd9ff).
Substantive restructure and a much stronger measurement plan. Working through R1 status at the new SHA:
R1 status at head
- Merge-order dependency on #2874. Decoupled. With
HF_DE_SHORT_BAND_ROUTE=falsedefault, no machine gets routed through the new band, so no new trip surface exists on this release. The dependency migrates cleanly to the follow-up "flip the default" PR — that one still needs #2874 in main. Baseline-first sequencing eliminates the concern here. - Kill-switch attribution reporting
skipped_elements. Resolved forHF_DE_SHORT_MIN_FRAMES=0, residual forHF_DE_SHORT_MAX_ELEMENTS=0— see one item below. The new decisiveness check (invertAtBandFloor && !invertAtBaseFloor) is a much stronger design than my suggested"disabled"bucket for the more common case; keeping it.
New concerns
Residual attribution asymmetry between the two kill switches. In renderOrchestrator.ts around line 2560, HF_DE_SHORT_MIN_FRAMES=0 correctly reports deShortBand: undefined (because Math.min(deSingleMinFrames, 0) === 0 fails the predicate's own minFrames > 0 guard → invertAtBandFloor = false → deShortBandDecisive = false). But HF_DE_SHORT_MAX_ELEMENTS=0 still reports "skipped_elements" for every in-band render: deShortBandOpen goes false via the > 0 guard, but invertAtBandFloor still evaluates at the band floor (250) and stays true for a 400-frame render → decisive+open-false → "skipped_elements". Small blast radius (the max-elements kill switch is documented as break-glass, and if fired fleet-wide the operator knows why), but the DiD read compares applied vs skipped_elements cohorts, and skipped-by-kill-switch pollutes the "skipped" bucket in a way that would make the post-flip analysis look like the ceiling is too tight. Minimal fix: gate the decisiveness check on deShortBandMaxElements > 0 too, symmetric with the min-frames path, so both kill switches land deShortBand: undefined.
What's new and worth acknowledging
countElementTagssemantic reversal + freeze. Void elements now count (image galleries no longer read as tiny comps), so the bias flips from DOWN (opening more) to UP for void-heavy content. The frozen-semantics comment is the right invariant to advertise here — a follow-up that "improves" the counter would silently invalidate the baseline it's meant to be measured against. If the counter ever does need to change, the read/flip cadence has to reset with it; worth naming in the follow-up PR body.- Decisiveness check as the attribution primitive. Evaluating the predicate at both floors and calling the band "decisive only when they disagree" is exactly right. The ineligibility-cases test loop (webm, forceScreenshot, compile-gated, requestedWorkers, useDrawElement=false, singleWorkerStreamingOk=false, probeDeGated) locks the cohort semantics in place — future edits to the predicate can't quietly drift the meaning of
de_short_band. - Parallel-worker init telemetry via the perf-summary channel. Verified the write side is real at head (
session.initTelemetry = telemetryinframeCapture.ts:442under this SHA), sodedupPerfsactually carries the fallback values intomergeWorkerInitObservability. This closes the "0% motion axis on the exact renders the band routes" gap you described.summarizeInitObservabilitynow seeds from the structured fallback and then max-refines via console, matching multi-session semantics — nice compositional shape.
Nits
- Tween-count merge across workers uses
Math.max, but the docblock says "any worker's reading is the reading" — max is safe (workers should agree on a per-composition value), but the code and comment disagree slightly. Not worth changing; noted. initTweenCount(input, fromCapturePerfSummary) →tweenCount(output, frommergeWorkerInitObservability) →tweenCount(inRenderInitObservability) — the single rename at the perf-summary boundary is fine, but future readers will grep both names. Minor.
What I didn't verify
- The 5.1%-revert-rate correction and the 12% expected net win — trusted per the message + PR body.
- Whether any prod dashboards already key off the pre-refactor
de_short_bandsemantics (frame-band-range attribution). The delta is invisible to consumers that only read the valuesapplied/skipped_elements/absent; only downstream logic that checked "is this in the 250-899 range" would need updating.
Solid iteration — LGTM from where I sit with the residual max-elements kill switch attribution flagged for follow-up cleanup.
miga-heygen
left a comment
There was a problem hiding this comment.
Re-review after v2 rework (baseline-first release).
SSOT -- Decisiveness Logic (Two-Floor Evaluation)
Core architectural improvement: the call site now invokes shouldPreferSingleWorkerDrawElement TWICE — once at the 900 floor, once at Math.min(900, 250) — and the band is decisive only when the calls disagree (invertAtBandFloor && !invertAtBaseFloor). This structurally cannot diverge from the production predicate because it IS the production predicate, not a separate predicate that duplicates eligibility checks.
All 13 eligibility conditions (worker count, requestedWorkers, useDrawElement, compile gate, forceScreenshot, outputFormat, minFrames > 0, totalFrames >= minFrames, singleWorkerStreamingOk, layered/effect route, supersampling, probeDeGated, experimental parallel opt-in) are naturally excluded without the call site enumerating them. This was v1's main weakness — fixed.
No SSOT findings.
Correctness — Void Element Regex
Regex: /<\/[a-zA-Z]|<(?:img|br|hr|input|source|track|area|base|col|embed|link|meta|param|wbr)\b/gi
All 14 HTML spec void elements present. \b correctly prevents false positives: <imaginary>, <breadth>, <colombia>, <metadata>, <sourceCode>, <inputField>, <tracking> — none match. Inline script if (a < b) is safe (neither branch fires). The test explicitly pins this: <script>if (a < b && x <breadth && y <imgWidth) {}</script> counts exactly 1 (the </script> closer). ✓
Routing Correctness — Truth Table
| Scenario | invertAtBase |
invertAtBand |
decisive |
bandOpen |
deShortBand |
route=off | route=on |
|---|---|---|---|---|---|---|---|
| >=900, eligible | true | true | false | * | undefined | true | true |
| 250-899, elts<=2500, eligible | false | true | true | true | "applied" | false | true |
| 250-899, elts>2500, eligible | false | true | true | false | "skipped_elements" | false | false |
| <250, eligible | false | false | false | * | undefined | false | false |
| Any frames, ineligible | false | false | false | * | undefined | false | false |
Critical checks:
- DiD integrity (route=off):
deShortBandRoute && deShortBand === "applied"short-circuits tofalse.deInversionEligibleALWAYS equalsinvertAtBaseFloor. Bit-identical to main. PASS. - No phantom tagging (route=on): Every "applied" render has
invertAtBandFloor=trueby construction (decisiveness requires it). PASS. - Operator precedence:
===(10) >&&(5) >?:(3). Parses as(deShortBandRoute && (deShortBand === "applied")) ? invertAtBandFloor : invertAtBaseFloor. PASS. - Cohort exclusion: Ineligible renders (webm, compile-gated, forced-screenshot, etc.) have BOTH floors return
false→decisive=false→deShortBand=undefined. Cohort contains ONLY renders whose routing this change decides. PASS.
The DiD design is airtight.
Init Telemetry Plumbing
mergeWorkerInitObservability solves a real coverage gap: parallel workers' console buffers only propagate on failure → 0% motion-axis coverage on the exact renders the band routes. Fix: piggyback on per-worker CapturePerfSummary, which already flows back on success.
Field name mapping verified at every hop: session.initTelemetry.tweenCount → CapturePerfSummary.initTweenCount → mergeWorkerInit output .tweenCount → summarizeInitObservability fallback seed. Max-merge semantics consistent with the existing maxReading helper used by summarizeInitObservability.
Both observability.summary() call sites (success path AND error/retry path) receive initFallback: mergeWorkerInitObservability(dedupPerfs). ✓
Telemetry Chain
Traced both fields through all hops — no gaps, no shadowing. compositionElementCount is unconditional (every render). deShortBand conditional only on decisiveness. Both follow the established spread-first + explicit-override pattern in trackRenderComplete. ✓
Tests
- Decisiveness tests (4 tests): Call the actual predicate at both floors instead of duplicating production logic. Pin: decisive exactly in 250-899 for eligible renders, NOT decisive below/above/when ineligible (comprehensive — 7 ineligibility reasons tested), kill switch via
HF_DE_SHORT_MIN_FRAMES=0. ✓ mergeWorkerInitObservability(2 tests): Max-merge across workers, undefined when no worker reported. ✓countElementTags(5 tests): Closers, void elements, false-positive prevention on inline scripts, malformed input stability, scale (40k). ✓envInt(2 tests): Unset/empty/non-numeric fallback, explicit 0. ✓- Init observability fallback (3 tests): Structured fallback (parallel success path), max-merge console over fallback, neither source. ✓
No significant missing edge cases.
Standards
- No bare
as Tassertions. ✓ - No non-null assertions. ✓
- Fallow audit fix: removed stale
fallow-ignore-next-lineon__resetDeParallelRouterTrialStateForTests. ✓
CI
All checks pass (47 completed, 2 regression shards pending, 1 expected-skip). 7/9 regression shards pass — confirm last 2 before merge.
Non-blocking note (carried from v1)
shortBand?: string in DrawElementPerfInput and RenderPerfSummary should be "applied" | "skipped_elements" to match peer field convention (workerInversion, parallelRouter keep their unions at this layer).
Verdict
Approve. The v2 rework addresses both structural flaws from v1: (a) the decisiveness logic reuses the production predicate itself instead of a separate check that could diverge, structurally excluding ineligible renders from the measurement cohort; (b) the HF_DE_SHORT_BAND_ROUTE gate makes this release a pure baseline where "applied" is counterfactual telemetry, not a routing change. The DiD design is airtight — the control group (skipped/oversize renders in the same frame band) absorbs secular drift, and the kill criteria are pre-registered before data exists.
The void element counter fix, init telemetry plumbing, and the full telemetry chain are all correct. This is how you ship a 31% fleet expansion safely.
-- Miga
miguel-heygen
left a comment
There was a problem hiding this comment.
R2 at 23854f7c6a328ef1f6913160af345c9fda70b036. The baseline-first redesign is materially stronger: renderOrchestrator.ts:2534-2560 reuses the production predicate at both floors, default-off routing preserves main behavior, and the structured worker-perf fallback closes the parallel motion-axis gap. Two pieces of the original measurement contract remain unresolved.
-
blocker —
packages/producer/src/services/renderOrchestrator.ts:1260: self-closing SVG remains an unbounded undercount. The new regex fixes HTML void elements, but<circle/>.repeat(40_000) still reports0(verified directly), as do other self-closing SVG-heavy compositions. That is the same failure class as the original<img>counterexample: an arbitrarily large DOM is measured below the 2,500 ceiling. Because this PR explicitly freezes the counter and collects the baseline that PR B will later route on, shipping the undercount now makes that baseline/gate unsafe rather than merely imprecise. Count self-closing elements conservatively too, and pin the 40k-SVG case. -
important —
packages/producer/src/services/renderOrchestrator.ts:2553-2560: max-elements kill-switch attribution is still false. As Rames noted,HF_DE_SHORT_MAX_ELEMENTS=0leavesinvertAtBandFloor=true, marks every otherwise-eligible in-band renderskipped_elements, and pollutes the DiD control; only the min-frames kill switch becomes absent. Gate decisiveness/attribution ondeShortBandMaxElements > 0so both documented disables have symmetric no-cohort semantics. This is also the disabled-state half of my R1 attribution request, not a new preference.
Exact-head CI currently has 54 successes, 1 expected skip, and regression shard 4 still running.
Verdict: REQUEST CHANGES
Reasoning: The decision selector is now architecturally sound, but the frozen baseline counter still maps arbitrarily large SVG DOMs to zero and one documented kill switch still corrupts the pre-registered control cohort.
— Magi
…ll-switch attribution 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 (<circle/>, <path/>, ...) are neither closing-tag-shaped nor in the HTML void list, so "<circle/>".repeat(40000) reported 0 — the same failure class as the original <img> 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 "<b/c>", 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) <noreply@anthropic.com>
miguel-heygen
left a comment
There was a problem hiding this comment.
R3 delta at 7bb9e3cbf920e04d01854a839d01619cb3615ab4.
The two R2 findings are fixed cleanly:
packages/producer/src/services/renderOrchestrator.ts:1277-1281now counts self-closing SVG tags, with the 40k SVG counterexample pinned.packages/producer/src/services/renderOrchestrator.ts:1311-1338,2540-2604makes band attribution a pure function and gates it onHF_DE_SHORT_MAX_ELEMENTS > 0, so the max-elements kill switch no longer enters the DiD control cohort. The typedshortBandfield and effective-floor log correction also align the surrounding contract.
One contract-level blocker remains after auditing other real composition shapes:
- blocker —
packages/producer/src/services/renderOrchestrator.ts:1277-1281,2542-2554: the ceiling counts source markup, not the initialized DOM, so runtime-generated DOM is still an unbounded undercount. The repository already has this production shape:packages/producer/tests/style-10-prod/output/compiled.html:824-845creates one group plus onespanper transcript word withdocument.createElement, whilecountElementTags(compiled.html)sees only literal tags in the source. A direct 40,000-nodecreateElementloop measuressourceCount=2versusliveNodesAfterInit=40,001. That admits the same 40k-node regression case below the 2,500 ceiling even though the new SVG test passes. Because this PR explicitly freezes the baseline counter and PR B will route on it, this is a safety-gate failure rather than telemetry imprecision. Please derive the gate from initialized live DOM size, or conservatively exclude runtime-generated/dynamic DOM from theappliedcohort and future route.
Exact-head CI is fully settled: 56 successes, 1 expected neutral/skip, 0 pending, 0 failed.
Verdict: REQUEST CHANGES
Reasoning: Both requested R2 mechanisms are correct, but the frozen source-tag selector can still map arbitrarily large real DOMs below the routing ceiling, so the planned follow-up flip is not safely bounded yet.
— Magi
…gate
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 <span>
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 <img>/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) <noreply@anthropic.com>
miguel-heygen
left a comment
There was a problem hiding this comment.
R4 delta at def98f79c3fa746e805c6fbe9ee51392c267ea1e.
The initialized-probe path now measures the right thing: document.querySelectorAll("*") closes the source-vs-live-DOM gap from R3. One lifecycle blocker keeps that fix from covering the selector's actual population:
- P1 — the live-DOM count is conditional on a probe that is not universal.
packages/producer/src/services/render/stages/probeStage.ts:220-228launches the probe only for unknown duration, unresolved compositions, or specific media cases. A known-duration, media-free composition whose initialization script creates thousands ofdiv/spannodes therefore returnsprobeSession: null.packages/producer/src/services/renderOrchestrator.ts:1308-1320then falls back to the same static source scan, and that value opens the band at:2581-2596;probeDeGatedis also false when the probe is absent (:2623-2627). So a 400-frame composition that creates 40,000 nodes at runtime can still be measured below the 2,500 ceiling and enter theappliedcohort. This directly contradicts the comment at:1289-1292that the probe is already running for every render.
The new tests at packages/producer/src/services/renderOrchestrator.test.ts:1913-1936 currently pin the unsafe fallback for absent, uninitialized, and failed probes rather than exercising the no-probe dynamic-DOM pipeline.
Please fail closed when a live measurement is unavailable: carry count provenance (live vs static) and allow only live to produce applied/route through the short band. Keep static count as diagnostic telemetry, and make missing live measurement absent or explicitly unmeasured so it does not contaminate the DiD control cohort. If full coverage is required, initialize a session only for renders whose two-floor predicate could be decisive, then recompute; forcing a probe for every render would add launch cost and weaken the route-off performance baseline.
Add an end-to-end regression with known duration, no media/unresolved composition, and script-created >2500 nodes, asserting it cannot enter applied without a live count.
Exact-head core CI, CodeQL, and producer tests are green; Windows and regression shards are still running. This is a measurement-contract finding, not CI noise.
Verdict: REQUEST CHANGES
— Magi
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) <noreply@anthropic.com>
miguel-heygen
left a comment
There was a problem hiding this comment.
R5 at exact head 4dbf0d90b05de2c3db7c983641c8785fd9988148 (delta from R4 def98f79).
The R4 blocker is resolved at mechanism:
packages/producer/src/services/renderOrchestrator.ts:1312-1330returns explicitliveversusstaticprovenance. Only an initialized probe whose page evaluation returns a finite DOM count can belive; absent, uninitialized, failed, and invalid measurements all fall back tostatic.packages/producer/src/services/renderOrchestrator.ts:2622-2627,2672-2680makes provenance load-bearing: onlylivecan open the band, and routing still requires both the default-off route flag anddeShortBand === "applied".packages/producer/src/services/renderOrchestrator.ts:1395-1405maps a decisive static measurement tounmeasured, keeping it out of both treatment and oversize-control cohorts rather than silently admitting it or contaminatingskipped_elements.packages/producer/src/services/renderOrchestrator.test.ts:1877-1933pins the exact known-duration, media-free, runtime-generated 4,000-node shape: no probe, static undercount, thenunmeasuredand neverapplied. The fallback matrix also covers uninitialized sessions, page-evaluate failure, and invalid results.- The new provenance and
unmeasuredvalue survive both telemetry paths: producer perf summary for success and capture observability for failure, then CLI event mapping. No shadowing gap found.
Non-blocking documentation drift: the PR body still describes de_short_band as only applied | skipped_elements and composition_element_count as the static countElementTags result. Before merge, please add unmeasured plus composition_element_count_source, and state that ceiling/read analyses must filter to source=live; otherwise the public pre-registered analysis contract no longer matches the safer implementation.
Exact-head CI is fully terminal: 57 successful, 0 failed, 0 pending.
Verdict: APPROVE
Reasoning: The conditional-probe gap now fails closed with explicit provenance, the unsafe dynamic-DOM shape cannot route or join either measured cohort, telemetry plumbing is complete, and all exact-head gates are green.
— Magi
|
Closing the loop on the review rounds here, since this merged without a written reply — @miguel-heygen in particular went four rounds on the same underlying question and each round was right. R1 → R2 → self-closing SVG undercount, and kill-switch attribution. R3 → source markup vs live DOM. The one no regex could fix. R4 → the probe isn't universal. My R3 code comment asserted a probe runs for every render; How it turned out. The first six hours of v0.7.83 data say R4 was the decisive catch: probe coverage is 17%, not the ">=28%" I'd projected — so without failing closed, 83% of renders would have been gated on a blind static count. The same data also shows fleet element counts topping out around 1,420 against a ceiling I calibrated at 2,500 using 7k/20k/40k synthetic sweeps, and |
PR A of two. This release changes NO routing — it computes and emits the short-comp band decision on every render, so the follow-up one-line flip (PR B) is measurable. Why the split is load-bearing is at the bottom.
Why
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.
What the measurements said
Controlled sweep ({250,400,600,900}f, single-DE vs parallel-screenshot-W4, capture mode verified per row, AC power, load-gated): single-DE wins 1.16-1.24x at every size — for content in constant motion. A 2x2 (movers x static DOM) isolated the variables (ratio = ss4/de1, >1 means DE wins):
Motion helps DE; DOM size punishes it (~0.50 vs ~0.22 ms/element — drawElement repaints the whole tree per frame, fan-out amortizes it). The downside is unbounded, so the band carries an element ceiling. Motion only ever helps DE, so a ceiling calibrated at the lowest-motion case (crossover ~3.9k; default 2500) is safe at every motion level.
Expected net, honestly: the existing 900+ inversion reverts on 5.1% of engaged renders (31,756 inverted / 1,705 reverted, 14d fleet-wide), and a revert pays ~1.8x. At the benched win that nets out to ~12%, not 20.
What this PR ships
HF_DE_SHORT_BAND_ROUTE(default off): the band decision is fully computed but does not route. PR B flips the default.de_short_bandemitted only when the band is decisive — the inversion predicate is evaluated at both floors and every other eligibility condition passed; only the floor differed.applied= element ceiling cleared too (counterfactual "would have inverted" in this release, factual after PR B);skipped_elements= ceiling was the only blocker. A webm render or compile-gated comp at 400f reports nothing — the cohort contains exactly the renders whose routing the band decides.composition_element_counton every render (countElementTags, now counting HTML void elements — closers-only read an image gallery as a tiny comp, opening the band on exactly the content most likely to lose it). Counter semantics frozen until PR B lands: the baseline distribution must be measured by the gate that later routes.observability_init_tween_counthad 0% coverage on the exact renders the band routes: parallel workers' console buffers (where the[FrameCapture:INIT]line lives) only propagate on failure — 35k single-worker renders/7d carry tween counts, 0 of 9,600 clamp-bucket renders. Fixed by riding the per-workerCapturePerfSummary(the one channel that returns on success) and max-merging into the observability summary, console lines still refining when present. With it, every band render carries full coordinates: elements x tweens x frames x path x speed.Pre-registered read (before data exists)
composition_element_countchecks the bench's ~0.50ms/element slope on real content BEFORE the flip. Any post-flip misroute is locally reproducible by feeding its telemetry row into the bench generator (--movers≈ tween count,--static≈ element count,--frames).speed_ratioofappliedrenders minus Δ ofskipped_elements/oversize renders across the flip boundary. The control slice absorbs secular drift (content mix, version-correlated populations — both observed in this project's data — and hardware). Install-weighted, heavy-installs excluded, within-version.Why two releases instead of one
Shipping counter + routing together makes the change unfalsifiable: the before-period can't be filtered to ≤2500 elements (the property doesn't exist yet), so the after-cohort excludes big comps by construction while the before-cohort includes them — the comparison shows a "speedup" even if the change does nothing. The paired-comp-hash alternative is dead on arrival: only 0.4% of band comps ever appear on more than one CLI version. Baseline-first is what makes a 100%-exposure ship readable at all.
Safety (unchanged, applies to PR B)
Per-frame PSNR self-verify with screenshot fallback, exactly as the 900+ band ships today. Knobs:
HF_DE_SHORT_MIN_FRAMES,HF_DE_SHORT_MAX_ELEMENTS,HF_DE_SHORT_BAND_ROUTE.Harnesses:
de-short-bench.sh,de-discriminator-bench.sh(plans/drawelement-fast-capture).🤖 Generated with Claude Code