feat(producer,cli): full telemetry visibility for DE parallel-router/inversion failures

[vanceingalls]

Summary

Follow-up to #2095. Started as failure-path telemetry visibility for the DE parallel router, then grew into hardening the retry/fallback path itself as testing (including live dogfooding) turned up real gaps — including three defects caught by a max-effort code review of the branch — and finally into solving how the router ever gets real-traffic telemetry at all, since HF_DE_PARALLEL_ROUTER has no self-serve opt-in path.

1. Failure-path telemetry visibility. render_error carried zero DE-cohort context, so a hard failure while routed/inverted was indistinguishable from any unrelated failure in PostHog.

• RenderCaptureObservability now records dePreInversionWorkers/dePreRouterWorkers (the worker count calibration would have picked absent the experiment), deFallbackReason, mirrored alongside the existing deSelfVerifyFallback — set live at the same point the router/inversion state is set, so it survives into job.errorDetails on a hard failure, not just into the success-path perfSummary.
• Added capture-sourced de_* fields to the shared RenderObservabilityTelemetryPayload, consumed by both trackRenderComplete and trackRenderError. On render_complete, the existing explicit perfSummary-sourced fields still win — this addition is purely a failure-path fallback.

2. Widen the self-verify retry to generic capture failures on a pinned worker count. The router/inversion pin a fixed worker count regardless of calibration or host load — exactly the scenario a contention timeout or worker crash is most likely under. Previously only a drawElement self-verify failure (blank/PSNR) triggered the existing fallback to the calibrated, non-DE parallel-screenshot path; any other capture-stage failure on a pinned render just hard-failed instead of reusing that tested safety net. shouldRetryViaPinnedFallback (new, pure, unit-tested) widens the retry.

3. Drop to a single worker specifically on OOM. Reusing the pre-pinned worker count on retry is fine for ordinary failures, but calibration's own pick can be higher than the pin (e.g. calibration wanted 5, the router pinned to 3) — retrying an OOM at the same or higher count just OOMs again. resolveInversionRetryPlan/resolveParallelRouterRetryPlan now drop to workerCount=1 specifically when the retry is OOM-triggered.

4. Recognize Bun/JavaScriptCore's OOM message. The producer's deployed runtime is Bun, not Node — isMemoryExhaustionError's patterns were all V8-specific, so #3 would never have engaged in production. Fixed with an exact-match check for Bun's "Out of memory" string.

5. Code-review fixes (3 confirmed findings from a 17-agent max-effort review):

• The Bun OOM regex from feat(studio): consolidate into single OSS-ready NLE editor #4 was itself defeated by this codebase's own parallel-worker error wrapping ("[Parallel] Capture failed: Worker N: Out of memory") — added a second pattern that survives the wrapping.
• shouldRetryViaPinnedFallback didn't exclude cancellation — aborting mid-render on the pinned cohort would detour through a wasted encoder spin-up before the cancellation was honored. Added an isCancellation check, checked first.
• deFallbackReason was set locally but never reached failure-path telemetry — a render that fails after a fallback attempt was indistinguishable from one that never attempted a fallback. Threaded through the same path as deSelfVerifyFallback.

6. CLI trial to actually get real-traffic router telemetry. HF_DE_PARALLEL_ROUTER is a producer env var with no self-serve path — nobody was ever going to manually turn it on, so the soak this whole feature needs would never start. renderLocal now enables the router for free on EVERY eligible CLI render for a fresh install — not just once — to maximize successful ("routed") telemetry volume across the fleet. It only turns off, permanently for that install, the first time the router's OWN safety net actually fires (deParallelRouter === "reverted") — a clean "routed" success keeps the trial running on the next render. A render that merely "routed" then crashed for an unrelated reason (e.g. cancellation) does NOT count as a router failure. Persists to ~/.hyperframes/config.json. A render that never crosses the router's own frame threshold doesn't touch the trial state either way. Never overrides an explicit user-set env var, and only engages when telemetry is enabled. Scoped to the in-process CLI path (--docker doesn't thread perfSummary/errorDetails back to the CLI process, so trial consumption can't be detected there).

Failure modes this retry/fallback path now handles

#	Failure mode	Trigger	Remedy
1	DE self-verify blank frame	Drain-time blank-guard trips	Retry via screenshot at pre-pinned worker count, deFallbackReason="blank"
2	DE self-verify PSNR breach	Captured frame vs ground truth < HF_DE_VERIFY_MIN_DB	Same, deFallbackReason="psnr"
3	Generic capture failure on a pinned worker count	Host contention timeout, worker crash while inverted/routed	Retry at pre-pinned (calibrated) worker count, deFallbackReason="capture_error"
4	OOM on a pinned worker count	Memory-exhaustion error while inverted/routed	Retry at workerCount=1 (not the pinned count — that can be equal/higher), deFallbackReason="oom"
5	OOM message not recognized (V8-only patterns)	Producer runs on Bun/JSC, not Node/V8	isMemoryExhaustionError also matches Bun's exact "Out of memory" message
6	OOM message mangled by this codebase's own worker-error wrapping	executeParallelCapture wraps as "[Parallel] Capture failed: Worker N: Out of memory"	Second regex matches "out of memory" as the whole content of a "Worker N: ..." segment
7	Cancellation mid-retry	User aborts while on the pinned cohort	Excluded entirely — propagates immediately instead of spinning up a fresh encoder first
8	True unbounded-growth OOM (heap hard limit)	Continuous allocation past the VM ceiling	No remedy — not catchable. SIGKILL (exit 137) before any catch runs; documented ceiling, not fixed
9	Retry poisoning a concurrent render (pre-fix)	Router mutated process.env.HF_DE_PARALLEL_STREAM globally; renders can share a process (PRODUCER_MAX_CONCURRENT_RENDERS)	Router decision threaded as a per-render local (deParallelStreamForced), not an env var
10	Failure invisible in telemetry	Hard failure while routed/inverted carried no DE-cohort tags in render_error	Mirrored router/inversion/fallback state from live capture-observability into the failure path
11	Router never gets real-traffic telemetry	HF_DE_PARALLEL_ROUTER has no self-serve opt-in — nobody manually flips it	CLI's per-install trial (item 6) runs on every eligible render until a real failure (not first engagement), maximizing successful-routing telemetry volume; no default-flip or env var needed

Known gaps, not remedied by design:

• No host-load/contention check before routing — the pin fires regardless of current machine state; failures are only caught reactively (rows 3–6), not avoided proactively.
• True heap-exhaustion OOM (row 8) is fundamentally uncatchable in this runtime.
• The CLI trial (row 11) only covers the in-process (--docker-less) CLI path — Docker renders and Studio server renders aren't included.

Test plan

• bunx vitest run across renderOrchestrator.test.ts, observability.test.ts, events.test.ts, renderObservability.test.ts, frameCapture-transientErrors.test.ts, render.test.ts — 269/269 pass
• Tests covering: failure-path de_* telemetry on render_error; explicit perfSummary values winning over the capture-observability fallback on render_complete; shouldRetryViaPinnedFallback's verify/generic/OOM/cancellation/reverted-cohort matrix; OOM-drops-to-1 for both retry-plan functions; Bun OOM detection (bare message + wrapped-worker-message forms, including negative substring cases); CLI trial enable/skip/no-consume-on-clean-routed-success/no-consume-on-unrelated-crash/consume-only-on-reverted matrix (40 tests total in render.test.ts)
• Live dogfood: real HF_DE_PARALLEL_ROUTER=true render confirmed router fires, self-verify passes 4/4, preRouterWorkers populates correctly
• End-to-end verification (not just unit tests) of the OOM path from inside the workspace: a real Bun RangeError/"Out of memory", both bare and wrapped in the real executeParallelCapture format, correctly resolves to workerCount: 1 on retry
• End-to-end verification of the CLI trial's config persistence against a real file (not the mocked unit tests): fresh install → deParallelRouterTrialFired undefined → write true → persists across reread
• tsc --noEmit, oxlint, oxfmt clean on all touched files

🤖 Generated with Claude Code

[vanceingalls]

Added a second commit based on live testing this afternoon.

While dogfooding the router with a real render (HF_DE_PARALLEL_ROUTER=true, workers left auto), calibration on this machine picked 5 workers — the router still pinned to 3 and it worked fine, but it surfaced a real gap: the router/inversion pin a fixed worker count regardless of calibration or host load, and I separately watched this same machine's load spike to 12+ from an unrelated process mid-benchmark. If a capture-stage failure (timeout, worker crash) happens on that pinned count under contention, today it just hard-fails the whole render — only a drawElement self-verify failure (blank/PSNR) gets the existing fallback to the calibrated, non-DE parallel-screenshot path.

df57eb0fd widens that retry: shouldRetryViaPinnedFallback now retries via the same tested fallback for any capture failure while deWorkerInversion="inverted" or deParallelRouter="routed", not just verify failures — reusing the exact retry machinery already built and tested for #2026/#2095, just broadening the trigger.

Deliberately excluded: OOM. The fallback's worker count is calibration's own pick, which can be higher than the pin (my dogfood run: calibration wanted 5, router pinned to 3) — retrying at the same or higher count would likely just OOM again, so that still fails fast instead of masking a real resource problem behind a doomed retry.

7 new unit tests on the extracted predicate, 122/122 total pass, typecheck/lint/format clean.

[vanceingalls]

Fixed a real gap in the OOM-retry commit — fair pushback: reusing `preInversionWorkerCount`/`preRouterWorkerCount` unmodified on an OOM retry was retrying at the same or higher parallelism than what just OOM'd (calibration can pick more workers than the pin, e.g. 5 vs the router's 3) — that's not a remedy, it's the same bet again, and worse for anything else sharing the host process.

`51353a7ed`: `resolveInversionRetryPlan`/`resolveParallelRouterRetryPlan` now drop to `workerCount=1` specifically when the retry is OOM-triggered — one Chrome page, the actual leanest configuration available, not just a different capture mode at the same worker count. Ordinary self-verify (blank/PSNR) retries are untouched — those aren't memory-related, so they keep behaving exactly as before.

2 new tests covering the OOM-drops-to-1 case for both retry-plan functions, 124/124 total pass.

[vanceingalls]

You asked if I actually tested the OOM-drops-to-1 fix — I hadn't beyond unit tests, so I went and did it live. Good thing I did: the fix as shipped would never have engaged in production.

`isMemoryExhaustionError` only matches V8-specific error text (`"JavaScript heap out of memory"`, `"Set maximum size exceeded"`, etc.) — but the producer's deployed runtime is Bun/JavaScriptCore (`packages/gcp-cloud-run/Dockerfile`: `CMD ["bun", "dist/server.js"]`), not Node/V8. I forced a real catchable OOM under Bun (`new Uint8Array(Number.MAX_SAFE_INTEGER)`) and the actual thrown message is the bare string `"Out of memory"` — none of the 7 existing patterns match it. So in the real deployed runtime, `isMemoryExhaustion` would always resolve `false`, and the whole worker-count-reduction from the previous commit would silently never fire — every OOM would just fall through to the generic `capture_error` retry (still retries, per the widened fallback, but without ever dropping to 1 worker).

Fixed in `b3f244a7e`: added an exact-message match for Bun's `"Out of memory"` (trimmed, case-insensitive) — deliberately an EXACT match, not a substring check, same reasoning as the existing patterns' comment (a bare "out of memory" substring shows up in benign WebGL/GPU console noise and must not misclassify).

Also worth flagging while I was testing: a genuine full-heap-exhaustion OOM (unbounded growth until the VM's hard limit) is not catchable at all — I forced one with `bun --max-old-space-size=64` and it SIGKILLs the process (exit 137) before any `catch` runs, no retry code ever executes. So this whole OOM-remedy path only ever helps the catchable flavor — a single oversized allocation hitting RangeError/Out of memory — which is the realistic failure mode for a large frame buffer, not a slow accumulating leak. Worth knowing as a ceiling on what this fix (and any future OOM handling) can do.

Verified end-to-end (not just unit tests) from a script inside the producer workspace, importing the real workspace source: real Bun `RangeError` → `isMemoryExhaustionError` true → `shouldRetryViaPinnedFallback` true → both `resolveInversionRetryPlan` and `resolveParallelRouterRetryPlan` correctly resolve `workerCount: 1`. Also confirmed the "no pinned cohort" case correctly still fails fast (no false-positive retry).

39/39 tests pass on the classifier, 124/124 on the orchestrator, typecheck/lint/format clean.

[vanceingalls]

Ran a max-effort automated code review of this branch (17 finder/verifier agents). 3 findings survived verification, all fixed in `a355fb2f6`:

1. The Bun OOM regex was defeated by this codebase's own error wrapping. `executeParallelCapture`/`formatWorkerFailure` (`parallelCoordinator.ts`) wrap a worker's error as `"[Parallel] Capture failed: Worker N: "` before it ever reaches `isMemoryExhaustionError` — the exact-match check from the previous commit never survives that wrapping, so a real OOM on the router-routed cohort (the exact case the drop-to-1 fix targets) would have retried at the SAME worker count that just OOM'd. Added a second pattern that recovers the signal by requiring `"out of memory"` be the WHOLE content of a `"Worker N: ..."` segment (bounded by end-of-string or `"; "`) — preserves the same no-bare-substring-match property, survives the wrapping. Verified against the actual wrapping logic (not a guess), and re-verified end-to-end from inside the workspace with the exact wrapped message string, confirming `workerCount: 1` now comes out correctly.

2. Cancellation wasn't excluded from the retry. `shouldRetryViaPinnedFallback` would have caught a mid-render `RenderCancelledError` on the pinned cohort and detoured through spawning a fresh encoder/capture session before the outer catch's cancellation branch ever ran — delaying "stop" with a pointless resource cycle. Added `isCancellation` (checked first), using the same `err instanceof RenderCancelledError || abortSignal?.aborted` check the outer catch already uses.

3. `deFallbackReason` never reached failure-path telemetry. Set locally but never mirrored into `RenderCaptureObservability`, so a render that fails AFTER attempting a fallback was indistinguishable from one that never attempted any fallback — directly undercutting this branch's own stated goal of measuring how often the OOM retry fires on a render that still ultimately fails. Threaded through the same path as the existing `deSelfVerifyFallback`.

10 new tests added across the 3 fixes, 199/199 pass, typecheck/lint/format clean.

[vanceingalls]

Ran a second max-effort code review focused on the CLI trial mechanism (last 2 commits, unreviewed at the time). 4 confirmed findings, all fixed in `532dad7cc`:

1. Batch re-entrancy bug (highest severity). `maybeEnableDeParallelRouterTrial`'s guard (`process.env.HF_DE_PARALLEL_ROUTER !== undefined`) couldn't distinguish "the user set this" from "row 1 of this same `--batch` run already armed it." Every row after the first silently got `trialArmed=false`, so a real revert on row 2+ was never persisted — permanently defeating the "one real failure turns it off" guarantee for that install. Fixed with a module-level flag that tracks whether we set the env var, distinct from its current value, plus a direct regression test reproducing the exact two-row scenario.

2. Config file race across concurrent CLI processes. `writeConfig` is a non-atomic whole-file overwrite with an unbounded read cache — a second concurrently-running CLI process (even just any other command, since all of them call `incrementCommandCount`) could clobber a just-persisted `deParallelRouterTrialFired: true` with its own stale snapshot. Added `readConfigFresh` (bypasses the cache) and use it immediately before the trial's read-modify-write, narrowing the race window without a full locking rewrite.

3. Regression: no exposure cap. The previous commit's semantics flip (only "reverted" turns it off) removed the one-render cap entirely — a healthy router that never reverts would stay force-enabled forever. Added `DE_PARALLEL_ROUTER_TRIAL_MAX_RENDERS = 25` as a backstop.

4. Missing `shouldTrack()` gate. Only checked `config.telemetryEnabled`, not `shouldTrack()` — so a dev-mode run or `DO_NOT_TRACK`/`HYPERFRAMES_NO_TELEMETRY` user got the experimental path silently armed while telemetry was simultaneously blocked underneath it. Now gates on `shouldTrack()` (a strict superset).

Also fixed a lower-severity stringly-typed-boolean parsing gap, and refactored the enable function into smaller pieces to clear the repo's own complexity gate (which the added logic had pushed over threshold).

14 new/updated tests (43 total in `render.test.ts`), verified the config primitives end-to-end against a real file, typecheck/lint/format/fallow-audit all clean.

[vanceingalls]

Third max-effort review, focused on the previous commit's own fixes. 6 confirmed findings, all fixed in `dc6df93de`:

1. `--batch-concurrency N>=2` breaks the re-entrancy fix. Real concurrent `renderLocal()` workers (via `Promise.all` in `batchRender.ts`) can't safely share one process-wide env var + module flag — a row finishing first could tear down the env var mid-render for a sibling row still in flight. Rather than try to make shared global state safe under true concurrency, added `RenderOptions.disableDeParallelRouterTrial` and set it whenever `batchConcurrency > 1` — the trial just isn't offered when it can't be evaluated safely.

2. The `outcome === undefined` no-op guard almost never fired. `aggregateDrawElement` defaults `parallelRouter` to the string `"none"` for every render, never `undefined` — so ordinary renders below the router's frame threshold (the common case) were ticking the render-count backstop and tripping the 25-render cap after zero real router engagements. Now treats `"none"` the same as `undefined`. Also fixed the test fixture that was supposed to cover this but used an unrealistic shape.

3. `shouldTrack()`'s process-lifetime memoization vs. a long batch. If a user runs `hyperframes telemetry off` in another terminal mid-batch, the cached `shouldTrack()` verdict would never reflect it. Restored a direct `config.telemetryEnabled` check (read fresh every call) alongside `shouldTrack()`.

4. Config write race could still flip `fired` back to `false`, not just lose an increment. Added a verify-and-retry loop (write, re-read fresh, retry up to 3x if a concurrent writer landed in between) — narrows further without a full file-locking rewrite.

5. Trial could arm before the first-run telemetry disclosure was guaranteed to have printed (that notice runs via an unawaited dynamic import in `cli.ts`). Rather than touch that pre-existing bootstrap chain, gated the trial on `config.telemetryNoticeShown` — it simply never offers itself on a fresh install's very first invocation.

6. `readConfigFresh` and the type-guarded parsing had zero coverage through the real implementation. Added a dedicated `config.test.ts` (node:fs mocked with an in-memory fake, not a HOME-env hack) exercising the actual module.

11 new/updated tests in `render.test.ts` (56 total) + 7 new in `config.test.ts`, typecheck/lint/format/fallow-audit all clean.

[vanceingalls]

Fourth max-effort review round (15 raw findings, deduped to 5 root causes — the review's own synthesize step died on a session limit, so dedup was manual). All real ones fixed in `2542e9427`:

1. The previous telemetryEnabled fix was ineffective (5 findings, one root cause). The arm site passed `readConfig()` — the process-lifetime cache — into the blocked-check, making it exactly as stale as the `shouldTrack()` memoization it claimed to bypass. Mid-batch `hyperframes telemetry off` was still never observed. Now reads `readConfigFresh()` at the arm site. The prior test passed only because the mock made `readConfig`/`readConfigFresh` behaviorally identical — see fix 4.

2. Verify-and-retry loop double-counted renders (5 findings, one root cause). When our write landed but a concurrent writer advanced the file before our verify read, the retry re-applied the increment — two renders → three counts → exposure cap trips early → trial permanently killed with less telemetry than designed. Reworked: the counter is written exactly once, unverified (lost increment = benign undercount); only the fired flag is verified/re-asserted, which is idempotent so retries can't corrupt anything.

3. Unwritable config = silent infinite re-arm (2 findings). `writeConfig` swallows all fs errors, so a reverted outcome could never persist on a read-only `~/.hyperframes` — re-arm and re-fail forever. Added an in-process fired latch (set at decision time, before persistence is attempted) + a one-time warning when persistence exhausts. Later processes still re-arm (disk is the only cross-process channel), but each process stops after at most one unrecordable failure.

4. Test mock erased the cache-vs-disk distinction (2 findings). `readConfig`/`readConfigFresh`/`writeConfig` were three views over one shared object, making findings 1 and 2 untestable by construction. The mock now models disk vs cache separately with a `failWrites` hook simulating the real `writeConfig`'s silent error swallowing. 3 new regression tests cover the mid-batch opt-out, the fired-flag re-assert-without-recount, and the unwritable-config latch.

5. Not fixed, by design: one finding flagged that the widened pinned-fallback retry (from the earlier producer commits) pays a doubled render on deterministic mid-stream failures like ENOSPC before surfacing the error. That's the accepted tradeoff of the fallback design discussed earlier in this PR — cancellation and OOM are already special-cased; further error-classification heuristics aren't worth the whack-a-mole.

56 tests across `render.test.ts` + `config.test.ts`, typecheck/lint/format/fallow-audit clean.

[vanceingalls]

Fifth (final scoped) review round, focused solely on the previous commit. 5 findings, all local — none structural, so per the pre-committed exit rule this stays a patch, and it's the last one. Fixed in `6172d79dc`:

1. Atomic config writes (the round's most consequential find). `writeConfig` was a plain `writeFileSync` — not atomic — while `readConfig`'s corrupted-file catch RESETS the whole config to defaults (telemetry re-enabled, anonymousId rotated, trial off-switch wiped). A concurrent reader catching a write mid-flight would silently destroy the user's config, and the previous commit's per-render `readConfigFresh()` multiplied exposure to exactly that window. Now writes to a pid-suffixed temp file + `renameSync` (atomic within a directory on POSIX). Verified against a real filesystem.

2. `writeConfig` reports success. Errors still swallowed (telemetry must never break the CLI), but `persistDeParallelRouterTrialFired` now stops immediately on a genuine fs failure instead of 3 blind writes + 4 reads, reserving retries for actual concurrent clobbers — the distinction the previous design couldn't make.

3. The persistence-failure warning is now `!quiet`-gated like every other trial message — no more unexpected stderr on quiet/batch-json renders that strict CI wrappers would misread as failure. The in-process latch already guarantees the safety behavior either way.

4. Arm site short-circuits on the in-process latch before the fresh disk read — post-fired batch rows no longer pay per-row config I/O for an answer module state already knows.

5. Removed the new `as T` assertions in the test factory (repo TS convention) via an explicitly typed `vi.hoisted` return.

57 tests across render.test.ts + config.test.ts (one new: writeConfig success/failure return + no-temp-file-left-behind), typecheck/lint/format/fallow clean, real-fs verification of the atomic write and the read-only-dir failure path.

[miga-heygen]

SSOT Review — #2115 feat(producer,cli): full telemetry visibility for DE parallel-router/inversion failures

What this PR does

Six interlocking changes, each building on the last:

1. Failure-path telemetry visibility — RenderCaptureObservability now records dePreInversionWorkers/dePreRouterWorkers/deFallbackReason so a hard failure while routed/inverted is distinguishable from an unrelated crash in PostHog.
2. Widen self-verify retry — shouldRetryViaPinnedFallback (new, pure, unit-tested) extends the existing retry path to any capture-stage failure on a pinned worker count, not just drawElement self-verify trips.
3. OOM drops to single worker — resolveInversionRetryPlan/resolveParallelRouterRetryPlan accept isMemoryExhaustion and drop to workerCount=1 when true (the actual memory remedy, not the same-or-higher count).
4. Bun OOM recognition — isMemoryExhaustionError now matches JSC's bare "Out of memory" AND the same message wrapped by this codebase's own executeParallelCapture error formatting.
5. Three confirmed code-review fixes — cancellation exclusion in retry path, deFallbackReason threading to failure telemetry, wrapped-worker regex for Bun OOM.
6. CLI trial — maybeEnableDeParallelRouterTrial/maybeConsumeDeParallelRouterTrial enable the router on every eligible CLI render for fresh installs, turning off only on a real safety-net revert (or a 25-render backstop cap).

SSOT analysis

Ran the full 7-step loop against the diff:

Helpers — every new function carries a fact the caller doesn't know:

• shouldRetryViaPinnedFallback owns the retry-eligibility decision (cancellation exclusion, verify-error always, pinned-cohort generic failures). Called exactly once in the catch block.
• isDeParallelRouterTrialBlocked owns the full blocking criteria (fired flag, render cap, telemetry state, notice shown). No duplication with the arm site.
• resolveDeParallelRouterOutcome normalizes "none" → undefined — this normalization is required (documented: without it, ordinary sub-threshold renders would tick the render counter).
• persistDeParallelRouterTrialFired owns the verify-and-re-assert retry loop — idempotent on the boolean flag, deliberately does NOT re-assert the render counter (double-counting would trip the cap early).

No duplicated decisions found. The retry decision lives in shouldRetryViaPinnedFallback only. The trial-gating decision lives in isDeParallelRouterTrialBlocked only. The OOM detection lives in isMemoryExhaustionError only.

Side-effect invariant:

Condition	Actor	Side Effect	Expected Count
Trial eligible + not blocked	maybeEnableDeParallelRouterTrial	Sets process.env.HF_DE_PARALLEL_ROUTER	Once per render
Router engaged (routed/reverted)	maybeConsumeDeParallelRouterTrial	Increments deParallelRouterTrialRenderCount	Once per render, fire-and-forget
Router reverted OR cap hit	maybeConsumeDeParallelRouterTrial → persistDeParallelRouterTrialFired	Writes deParallelRouterTrialFired: true	Once, verified with up to 3 retry attempts
Trial over	stopManagingDeParallelRouterTrial	Deletes env var + clears managedByUs flag	Once per render

No double mutations, no missed paths. Both success and error paths call maybeConsumeDeParallelRouterTrial — verified in the diff.

Sentinel-value scan:

• deParallelRouterTrialFired parsed with === true (not truthy) — guards against string "false" in hand-edited JSON. ✓
• deParallelRouterTrialRenderCount parsed with typeof === "number". ✓
• resolveDeParallelRouterOutcome normalizes "none" → undefined. ✓
• ?? 0 for render count is correct (undefined means 0). ✓

Stale concept grep: No remnants of earlier designs visible. The old isDrawElementVerificationError-only catch was fully replaced by the shouldRetryViaPinnedFallback gate.

Code observations

The writeConfig atomicity fix is a quiet gem. The temp-file + renameSync change protects against a scenario the comment identifies: readConfig's corrupted-JSON catch resets to defaults (new anonymousId, all optional fields wiped), so a torn read of a non-atomic write would silently destroy the user's config. Good defensive change that benefits all config consumers, not just the trial.

The readConfigFresh vs readConfig distinction is well-motivated. The arm site and consume site both use readConfigFresh because readConfig's process-lifetime cache is exactly as stale as shouldTrack()'s memoization — a mid-batch hyperframes telemetry off from another terminal would never be observed. The short-circuit on deParallelRouterTrialFiredThisProcess (before the disk read) is a nice optimization for post-fired batch rows.

The Bun OOM regex pair is carefully scoped. BUN_MEMORY_EXHAUSTION_EXACT_MESSAGE matches the bare message (/^out of memory\.?$/i), while BUN_MEMORY_EXHAUSTION_WRAPPED_WORKER_MESSAGE matches through executeParallelCapture's "Worker N: <message>" wrapping — requiring "out of memory" as the whole worker-segment content ((?:;|$) boundary), not a substring. The negative test case ("Worker 2: WebGL context lost, out of memory reported by driver") confirms the substring-exclusion property holds.

The disableDeParallelRouterTrial option for batchConcurrency > 1 is the right call. The process-wide env var + module-level flag are safe for sequential batch rows but not genuinely concurrent ones. Rather than making shared mutable state safe under concurrency, just don't offer the trial. Simple and correct.

Test coverage is excellent — 40+ new tests covering the full CLI trial matrix (fresh install, already fired, telemetry off, notice not shown, user-set env var, clean routed success, reverted, unrelated crash, batch row sequencing, concurrency disable, mid-batch opt-out, lost writes, unwritable config, render cap), OOM detection (Bun bare + wrapped + negative), retry logic (shouldRetryViaPinnedFallback verify/generic/OOM/cancellation/reverted-cohort), and config module behavior (cache/fresh/write/corrupt/atomic).

Verdict

No SSOT violations. No stale plumbing. No sentinel-value issues. The failure-mode table in the PR description is thorough and the "known gaps" are honest. The code-review findings are already incorporated. CI is 46/47 green (one regression shard still running).

This is a well-structured hardening PR. Ship it.

— Miga

[james-russo-rames-d-jusso]

Reviewed at 6172d79dc223.

Clean, high-effort PR — the failure-mode matrix in the body reads like a design doc, and 5 self-review rounds already scrubbed the interior. The retry-plan predicates (shouldRetryViaPinnedFallback, resolveInversionRetryPlan, resolveParallelRouterRetryPlan) are all pure and tested end-to-end (verify / generic / OOM / cancellation matrix); deParallelStreamForced correctly threaded as a per-render local instead of process.env mutation closes real cross-render cross-talk in the concurrent-producer scenario; Bun OOM regex handles the wrapped-worker case as exact-match-per-segment (verified against the substring-negative test); CLI trial mechanics (in-process latch surviving unwritable disk, readConfigFresh bypassing the stale-cache bug, resolveDeParallelRouterOutcome normalizing "none" → undefined to avoid ticking the render-count cap on unrelated renders) all handle the concurrency / persistence edge cases the self-review rounds surfaced.

Two non-blocking notes inline:

1. deSelfVerifyFallback semantic narrowing — the field used to effectively mean any fallback fired; with the widened retry it now means verify-triggered specifically. Downstream PostHog dashboards keyed on that as any fallback will silently under-count OOM / capture_error retries. Coordinate with the observability rebuild.
2. Trial disable is opt-out from CLI batch code only — renderLocal is exported; any programmatic consumer sharing the process picks up the trial + its module-level latch. Doc-comment could name the assumption explicitly, or invert to opt-in.

The one gap I'd flag structurally is host-load / contention awareness before routing (called out in the PR body's Known gaps) — accepting that as noted, since the retry safety net catches it reactively.

— Review by Rames D Jusso

[james-russo-rames-d-jusso]

🟠 Downstream metric semantic narrowing. Before this PR, this line ran unconditionally inside the verify-error branch, so de_self_verify_fallback = true was effectively any DE fallback fired. With the widened retry, this flips to deSelfVerifyFallback = isVerifyError, and OOM / capture_error-triggered fallbacks now emit de_self_verify_fallback = false while carrying de_fallback_reason ∈ {oom, capture_error}.

Any PostHog dashboard / alert / cohort filter that used de_self_verify_fallback = true as the any fallback fired signal will silently under-count post-merge — exactly the failure mode this PR exists to close, but for the observability side rather than the code side. Given this is the very cohort the PR is expanding failure-path visibility for, worth either:

1. Preserving deSelfVerifyFallback = true for any fallback (keep the compat, downgrade the field to just "fallback fired" and let deFallbackReason carry the sub-reason), or
2. Explicitly calling out in the PR body that dashboards should migrate to de_fallback_reason IS NOT NULL — Miguel's observability rebuild (project_hf_observability_rebuild) is the natural coordination point here.

The name deSelfVerifyFallback also reads narrower than the field's original meaning now — a follow-up rename to something like deVerifyFallback (with deFallbackReason being the general signal) would help future readers, non-blocking.

— Review by Rames D Jusso

[james-russo-rames-d-jusso]

🟡 Trial safety is opt-out, and only from CLI batch code. renderLocal is exported from this file, so anything else in the codebase (or an external SDK consumer) that imports and drives it — e.g. a future studio-server render path, a scripted test harness, or a distributed-plan runner — inherits the trial and the process-wide env-var / module-level latch state without knowing to pass disableDeParallelRouterTrial: true. The doc-comment names --batch-concurrency N>=2 as the specific case, but the underlying issue is broader: any concurrent invocation of renderLocal in the same process races on deParallelRouterTrialManagedByUs / deParallelRouterTrialFiredThisProcess / process.env.HF_DE_PARALLEL_ROUTER.

Safer polarity: default the trial off for programmatic callers and add enableDeParallelRouterTrial?: boolean that the CLI top-level render command explicitly opts into (batch code stays off, current tests unchanged). Non-blocking — the CLI-only assumption is a reasonable scoping call — but the doc-comment could name the assumption explicitly ("only the top-level CLI render command should ever leave this false") so future callers hit a clear signpost.

— Review by Rames D Jusso

[miga-heygen]

Re-review — #2115

Checked the delta since my last review (3 new commits). The changes are minor — the core architecture is unchanged. The new commit b02703b7 appears to be doc/polarity tweaks. No new SSOT concerns.

Previous verdict stands: no blockers, ship it.

— Miga

[james-russo-rames-d-jusso]

Reviewed delta 6172d79d → b02703b7.

Both R1 items landed cleanly, and the resolution shapes are exactly the ones I'd have picked — appreciated that you took the polarity flip on the trial flag rather than the "add another guard on the concurrent path" workaround.

🟠 deSelfVerifyFallback semantic narrowing — resolved via docs + downstream migration hint. observability.ts:44-51 and renderOrchestrator.ts:406-412 now explicitly state that deSelfVerifyFallback narrowed to "verify-triggered" only, and name the compensating "any-fallback" signal (de_fallback_reason IS NOT NULL) that dashboard consumers should migrate to. Not renaming the field is the right call — a rename is its own migration hop through PostHog + any BI reports — so a doc-level pointer + explicit new-signal name is the cleanest bridge. Coordination note if it's useful: worth a heads-up in the HF observability rebuild thread (Miguel's #1231/#1233/#1248 lineage) so anyone tuning fallback dashboards there knows the two signals now split.

🟡 Trial-flag polarity flip — resolved as OPT-IN. render.ts:1068 — enableDeParallelRouterTrial?: boolean default OFF. Both CLI call sites migrated: single top-level render sets enableDeParallelRouterTrial: true, batch path sets batchConcurrency <= 1 (sequential opts in, concurrent leaves unset). maybeEnableDeParallelRouterTrial short-circuits when !enabled at line 1752. Doc comment fully rewritten and (nice touch) explicitly names studio-server / test harnesses / distributed runners as the "programmatic consumers who get no trial by default" — exactly the class of caller the R1 concern was about. Test at render.test.ts:755-766 inverted correctly — the const { enableDeParallelRouterTrial: _omitted, ...programmaticOptions } destructure is a clean way to simulate a programmatic caller in the fixture.

Ship it. LGTM from my side — leaving as a comment.

— Review by Rames D Jusso

[miguel-heygen]

Final approval pass at current head 392dd410a582e74622cdacb5cfd1d7a67878defc.

Verified the current diff and the b02703b7 follow-up: trial routing is opt-in at intended CLI call sites, and deSelfVerifyFallback documentation points consumers to de_fallback_reason IS NOT NULL. No new functional drift found. All required CI checks are green. Remaining review threads are non-blocking documentation/coordination notes only.

[jrusso1020]

APPROVE ✅

CI is now fully green on the merge head 392dd410a (44 checks pass, 0 fail — including all 8 regression shards, Windows render/tests, and CodeQL). That was the specific thing I was holding for: a render-path change on a fresh merge-of-main needs the visual-regression shards to clear on the actual merge head, and they did.

The R1/R2 review is thorough (Miga SSOT + tai/pr-review + Rames-D) with all non-blockers addressed: opt-in trial polarity (enableDeParallelRouterTrial, default OFF, programmatic consumers get no trial) and the deSelfVerifyFallback semantic narrowing documented with the de_fallback_reason IS NOT NULL migration hint. Head is the reviewed merge commit, no post-review push.

Approving per Vance's request. — Rames Jusso