feat(producer): enable the parallel-DE router by default, behind a real circuit breaker

[vanceingalls]

What

Router flips to default-ON — but not as an env-polarity change. Review found the naive !== "false" version was broken twice over, so this ships default-on with the per-install circuit breaker kept and made correct.

Stacked on #2838 (floor 700 + power telemetry).

What was wrong with the one-line flip

The CLI already had a rollout mechanism I hadn't found when I wrote it: an opt-in trial that arms the router per install and — critically — disables it permanently the first time a render falls back, printing "disabled automatically if it ever needs to fall back". It disables by deleting HF_DE_PARALLEL_ROUTER.

Under !== "false", undefined means ON. So the flip silently turned that circuit breaker into a no-op — on exactly the hosts it exists to protect — while the banner kept promising otherwise. It also failed open on 0, off, no, FALSE, and set-but-empty: every spelling of the kill switch except the literal one enabled the router.

The soak number was measured under that breaker, which also reframes the justification. Version-gated, install-level:

installs that engaged the router	975
never reverted	879
reverted exactly once	92
reverted more than once	4
installs hitting ≥1 revert	9.8%
avg engaged renders/install	4.4
per-render revert rate	2.31%

92 of 96 reverting installs stop at exactly one — that's the breaker working. Remove it and ~10% of installs go from "one slow render, then protected" to "every eligible render is slow."

What this does instead

• isDeParallelRouterEnabled() parses the kill switch properly: false/0/off/no (case- and space-insensitive) disable; unset or empty = default.
• The breaker writes an explicit "false" instead of deleting — that's what makes it survive a default-ON flag — and persists across processes via the existing config flag.
• Trips only on an actual fallback. The old 25-render exposure cap was sampling logic for an experiment; under a shipped default it would switch the feature off behind the user's back after 25 good renders.
• No longer keys on telemetry state. The trial refused to arm without recordable telemetry (fair for an experiment); as a default that would mean opting out of analytics silently costs you a slower renderer.
• Explicit user value wins in both directions, latched before the breaker can write the var and make the two indistinguishable.
• Users are told when it trips and how to re-enable.

Verification

Fault-injection: restoring the delete fails 3 tests; restoring the naive parse fails 1. 66 CLI + 146 producer tests pass, typecheck + lint + fallow clean.

Tracked: PRINFRA-384.

🤖 Generated with Claude Code

[miga-heygen]

SSOT -- circuit breaker state machine audit

Traced every transition through the module-level state (deParallelRouterUserManaged, deParallelRouterUserManagedResolved, deParallelRouterBreakerTrippedThisProcess) and the on-disk deParallelRouterTrialFired flag. All five advertised transitions hold:

Initial state	Event	Result	Verified
unset (fresh install)	render succeeds	stays ON (env untouched, producer default)	yes
unset (fresh install)	render falls back	breaker writes explicit "false", persists deParallelRouterTrialFired: true	yes
explicit "true" by user	render falls back	applyDeParallelRouterBreaker() is a no-op (user-managed latch), env stays "true", router stays ON	yes
explicit "false" by user	any render	userValueEnablesDeParallelRouter() returns false, router OFF, breaker never consulted	yes
breaker wrote "false" (prior process)	user overrides with "true"	user-managed latch fires, userValueEnablesDeParallelRouter() returns true, router ON	yes

Cross-process persistence: the in-process latch (deParallelRouterBreakerTrippedThisProcess) guards within a process; the on-disk flag (deParallelRouterTrialFired via readConfigFresh) guards across processes. Both paths converge on writing process.env.HF_DE_PARALLEL_ROUTER = "false". The persistDeParallelRouterTrialFired retry loop (3 attempts, verify-after-write) handles concurrent-writer clobbers. Correct.

Kill switch parsing

isDeParallelRouterEnabled (producer, exported, takes env param) and userValueEnablesDeParallelRouter (CLI, private, reads process.env) are deliberately duplicated with identical logic. Both:

trim().toLowerCase()
undefined or "" -> true (ON)
"false" | "0" | "off" | "no" -> false (OFF)
everything else -> true (ON)

Tested spellings in isDeParallelRouterEnabled tests: "false", "FALSE", "False", "0", "off", "OFF", "no", "No", " false " (padded), "", " " (whitespace-only), unset. All produce the correct result. The naive === "true" / !== "false" pitfall that motivated this work is properly eliminated.

SSOT note (reviewed, accepted): The duplication between producer and CLI is documented with a rationale (producer is lazily loaded, CLI needs this on the startup path). The comment names the producer copy as source of truth and says "keep the two in sync." Acceptable trade-off -- flagging it here so a future editor knows to update both.

Side-effect invariant

The breaker writes process.env.HF_DE_PARALLEL_ROUTER = "false" exactly once on the first fallback (in maybeConsumeDeParallelRouterTrial). Subsequent renders in the same process short-circuit at step 3 of applyDeParallelRouterCircuitBreaker (the in-process latch) and call applyDeParallelRouterBreaker which is an idempotent re-write of "false". The disk write (writeConfig) also fires only on the fallback render (subsequent renders return routerActive = false, so maybeConsumeDeParallelRouterTrial exits at line 1). Correct.

Race conditions

Single-process: JS event loop -- no data race possible. Module-level flags are shared across all renderLocal calls in one process, but the manageDeParallelRouterBreaker opt-in polarity ensures only sequential call sites (single render, batch concurrency 1) use the mechanism. Batch concurrency >= 2 leaves the flag unset, so those renders never touch the breaker state. Correct.

Cross-process: two processes both reading deParallelRouterTrialFired: false, both tripping, both writing true -- idempotent, no corruption. The render counter can lose an increment under a race (documented, benign -- it's telemetry, not a decision input anymore). persistDeParallelRouterTrialFired verify-and-retry handles the clobber case for the safety-critical fired flag. Correct.

25-render cap removal

DE_PARALLEL_ROUTER_TRIAL_MAX_RENDERS = 25 is deleted. The only trip condition is now outcome === "reverted". The test that verified the old cap is rewritten to run 30 successful renders and assert the breaker never trips. The comment block replacing the constant explains why: under a shipped default, a render-count cap would silently disable the feature after 25 good renders. Correct and clean.

The deParallelRouterTrialRenderCount field is still incremented on every consumed render -- this is telemetry bookkeeping, not a decision input. Fine to keep.

Standards

No bare as T casts in the diff (only as const for literal types in test fixtures). No non-null assertions. Function signatures are clean. Comments are thorough and accurate -- every "review finding" annotation traces back to a real design constraint.

CI

Preflight (lint + format): pass
Preview parity: pass
Preview regression: pass
Player-perf: pass
Regression shards (1-8): in progress at time of review

---

Findings

1. Misleading user-facing message when explicit opt-in overrides breaker (nit)

When a user explicitly sets HF_DE_PARALLEL_ROUTER=true and a render reverts, reportDeParallelRouterBreakerTrip prints:

A frame failed verification, so parallel drawElement capture fell back to the screenshot path and is now off for this install.

But the user's explicit "true" wins over the breaker (applyDeParallelRouterBreaker is a no-op when deParallelRouterUserManaged), so the router is NOT off. The message is factually incorrect for this path. Low severity (the user explicitly opted in and probably knows what they're doing), but worth gating the message on !deParallelRouterUserManaged so it only prints when the breaker actually took effect.

Additionally, reportDeParallelRouterBreakerTrip fires on EVERY subsequent revert for this user (since routerActive stays true via the user's explicit value), producing the same misleading message each time. The first-revert-only semantics that apply to the non-user-managed path don't apply here.

2. Missing test: user explicit "true" survives a breaker trip (nit)

The state transition "explicit true -> render reverts -> breaker can't override -> env stays true" is logically correct (I traced it above) but has no dedicated test. The existing tests cover user-set "false" not being overridden, breaker writing "false" when unset, and the kill switch parsing. A test like this would lock down the "explicit user choice wins in both directions" invariant the comments describe:

it("does not override an explicit user opt-in even after a fallback", async () => {
  savedEnv.set("HF_DE_PARALLEL_ROUTER", process.env.HF_DE_PARALLEL_ROUTER);
  process.env.HF_DE_PARALLEL_ROUTER = "true";
  configState.disk = { /* fresh install */ };
  producerState.executeImpl = async (job) => {
    job.perfSummary = { drawElement: { parallelRouter: "reverted" } };
  };
  await renderLocal("/tmp/project", "/tmp/out.mp4", baseOptions);
  expect(process.env.HF_DE_PARALLEL_ROUTER).toBe("true");
});

3. events.test.ts -- gpu: false addition looks like a cross-PR fix (informational)

Two test calls to trackRenderComplete gain gpu: false. This is likely fixing a type error introduced by the base PR (#2838) that added a gpu property to the event payload. Not a problem, just noting it's a stacked-PR seam rather than a circuit-breaker change.

---

Verdict

Approve. The circuit breaker's state machine is correct and complete. The kill switch parsing closes the exact vulnerability the PR description identifies (naive !== "false" failing open for 0/off/no/FALSE). The 25-render cap removal is well-motivated and cleanly executed. The two nits above are low-severity edge cases -- finding 1 (misleading message for explicit opt-in users) is the only one I'd consider addressing before merge, and even that is a rare user path. The code is well-documented, the test coverage is strong, and the design trade-offs (deliberate duplication, latched user-managed detection) are clearly explained.

CI regression shards are still running. Confirm they pass before merge.

-- Miga

[miguel-heygen]

Additive review at exact head 8d30cdb9af — Miga covered the state machine, kill-switch spellings, and user-message/test nits. One gap remains in the boundary between the producer parser and CLI breaker.

Strengths

• packages/producer/src/services/renderOrchestrator.ts:1332-1339 centralizes default-on parsing and correctly handles the conventional OFF spellings.
• packages/cli/src/commands/render.ts:1185-1207 preserves the persisted breaker across processes without consulting telemetry state.

blocker — set-but-empty enables the router but disables its circuit breaker

packages/cli/src/commands/render.ts:1172-1183 classifies any defined HF_DE_PARALLEL_ROUTER as user-managed (!== undefined). That disagrees with both parsers at render.ts:1148-1152 and renderOrchestrator.ts:1332-1339, where "" / whitespace explicitly mean “unset/default ON.”

Concrete failure: launch with HF_DE_PARALLEL_ROUTER= (or spaces), hit a verified fallback. The producer routes because empty means ON; applyDeParallelRouterCircuitBreaker returns true but latches deParallelRouterUserManaged = true; then applyDeParallelRouterBreaker() no-ops at render.ts:1154-1157. The install keeps retrying the failing router on later renders instead of receiving the advertised first-fallback protection. This is the central safety invariant of the PR.

Normalize before deciding ownership: empty/whitespace should remain breaker-managed, while a non-empty explicit value stays user-managed. Please pin it with a CLI-level regression that starts from HF_DE_PARALLEL_ROUTER="" (and whitespace), injects parallelRouter: "reverted", and asserts the env becomes "false" and the fired flag persists.

CI is still running eight regression shards; this finding is independent of that pending gate.

Verdict: REQUEST CHANGES
Reasoning: Default-on parsing is correct, but the CLI ownership latch exempts the parser's own empty/default state from the per-install breaker, leaving a documented default path unprotected after fallback.

— Magi

[james-russo-rames-d-jusso]

Reviewed at 8d30cdb9.

Kill-switch parser at renderOrchestrator.ts:1333-1339 is clean — .trim().toLowerCase() up-front, then treats false | 0 | off | no as OFF, everything else including undefined/empty/whitespace as ON. Default-inversion trip parity with the breaker's process.env.HF_DE_PARALLEL_ROUTER = "false" write at render.ts:1156 — producer treats that as OFF via the same reader, so the trip signal reaches the router.

Persistence via ~/.hyperframes/config.json:deParallelRouterTrialFired written by persistDeParallelRouterTrialFired() at render.ts:1242-1251 with a fresh re-read + re-assert up to 3 times to survive concurrent clobber — solid pattern. In-memory latch (deParallelRouterBreakerTrippedThisProcess at render.ts:1101) set BEFORE the disk write so the decision holds if fs write fails. Test coverage is comprehensive across kill-switch spellings (renderOrchestrator.test.ts:1831-1850), 15 CLI-side scenarios (render.test.ts:700-1057) including 30-healthy-renders-no-trip, unwritable-config-in-process-latch, and concurrent-write-clobber-re-assertion.

Four concerns — none blocking, all worth thinking about before default-ON ships.

Blockers

None.

Concerns

1. "Trip only on real fallbacks" claim overstates what the predicate actually catches. The PR title says the breaker trips "only on real fallbacks" and the user-facing message at render.ts:1321-1323 says "A frame failed verification, so parallel drawElement capture fell back". But the trip predicate at render.ts:1297 is const fired = outcome === "reverted";, and "reverted" is stamped by renderOrchestrator.ts:2865 for the router-routed path on any shouldRetryViaPinnedFallback outcome (renderOrchestrator.ts:1475-1484 docstring 1442-1466) — OOM, worker crash, host-contention timeout, all trip the breaker permanently. The test at render.test.ts:893-909 ("worker crashed even after fallback") explicitly asserts this is intended.

Failure scenario: a user runs a large render on a hot machine, one worker gets OS-OOM-killed (not a router bug), retry succeeds via pinned fallback. Breaker persists trip=true. Every subsequent render on that install skips the router forever, even after the user upgrades RAM. Only manual HF_DE_PARALLEL_ROUTER=true or deleting ~/.hyperframes/config.json un-trips it.

Two paths to close: (a) narrow the trip predicate to just self-verify-failure (leave OOM/crash/timeout as one-render retry but not a permanent trip), or (b) update the PR title + user-facing message to accurately describe "any capture-stage retry via the pinned path" as the trip condition and consider automatic time-based un-trip.

2. One-shot deParallelRouterUserManaged latch clobbers mid-batch explicit overrides. render.ts:1078-1086 docstring claims "Their choice wins over the circuit breaker in BOTH directions." That's true only for the first applyDeParallelRouterCircuitBreaker call in the process. Programmatic wrapper scenario:

1. Row 1: fresh env, applyDeParallelRouterCircuitBreaker latches deParallelRouterUserManaged = false.
2. Programmatic wrapper sets process.env.HF_DE_PARALLEL_ROUTER = "true" between renders (documented at render.test.ts:979-981 as a legitimate scenario).
3. Row 2: latch skipped (deParallelRouterUserManagedResolved === true), reads stale userManaged = false, router runs.
4. Row 2 reverts → applyDeParallelRouterBreaker → writes "false" OVER the user's explicit "true" because the latch said false.

Not caught by any test — the test at render.test.ts:964-985 sets "false" (which matches what the breaker would write) and uses parallelRouter: "routed" (no revert, so the breaker write never fires). Fix: re-evaluate userValueEnablesDeParallelRouter(process.env.HF_DE_PARALLEL_ROUTER) at trip time in applyDeParallelRouterBreaker, not from a first-row latch.

3. CLI-copy of the kill-switch parser has no unit test. render.ts:1148-1152 (userValueEnablesDeParallelRouter) parses the same set of spellings as the producer, and the docstring at render.ts:1142-1147 warns: "Keep the two in sync — the producer copy is the source of truth." But only the producer copy has parser tests (renderOrchestrator.test.ts:1831-1850). A future refactor that adds "disabled" as a kill spelling to the producer but forgets the CLI copy would silently break trip-persistence for that spelling with no CI signal. Either add a parity test that imports both parsers and iterates the shared spelling list, or extract the parser to a shared module.

4. Disk-persisted trip is CLI-only. renderOrchestrator.ts:1333-1339 reads env only. Any future in-process or subprocess caller that invokes the producer via a path other than packages/cli/src/commands/render.ts bypasses the ~/.hyperframes/config.json trip check — that caller reads the router as default-ON regardless of prior CLI-side trips. Not exploitable today (no other caller), but the "per-install breaker" claim in the PR body is actually "per-install through the CLI." Worth noting explicitly in the docstring at render.ts:1120-1125 for future integrators (studio-server, aws-lambda, gcp-cloud-run).

Nits

• Stale docs: packages/producer/tests/escape-hatch-fatal-fallback/src/README.md:33-35 still describes the router as opt-in via HF_DE_PARALLEL_ROUTER=true. Update to reflect default-ON + kill-switch semantics.
• applyDeParallelRouterCircuitBreaker(options.quiet) at render.ts:837 — options.quiet is boolean | undefined; when undefined, !quiet inside is truthy, so the breaker's user message may print for programmatic callers who didn't explicitly set quiet. Non-critical since callers who opt in also set quiet in execute.ts:105,255.
• The /tmp/pr-2840.diff was cut against 3da31e399 (pre-2838), not against 2838's HEAD, so the diff file conflates 2838's default-flip inline with 2840's parser tightening — easy to misread on GitHub's stack view. Reviewers using the diff file locally should check the git diff 6a50c9f42..HEAD numstat (24+/1- for renderOrchestrator.ts) to see the 2840-alone shape.

Questions

• Is there an intentional reason NOT to auto-untrip after N healthy renders on a bumped-RAM/refactored machine? A 30-healthy-render green streak (already tested at render.test.ts:987-1010) would be a natural signal.
• Was stop gating on telemetry reviewed with the dashboard-1807532 tripwire owner? Removing the telemetry gate means the "sustained revert >10%" signal now excludes the telemetry-opt-out cohort — the gate for that dashboard signal now has an unknown-size blind spot.

What I didn't verify

• Local end-to-end with a real config file — trusting the test coverage.
• Whether other consumers of ~/.hyperframes/config.json (studio, cli auth) could collide with concurrent writes to the trial-fired field during a batch render.

— Review by Rames D Jusso

[vanceingalls]

Fixed in `0fed335aa`. Thanks both — the blocker is a genuine hole and in the same class as the bug this PR exists to close, just through a different door.

@miguel-heygen — blocker: set-but-empty exempts the install from its own breaker. Correct and confirmed. Ownership used `!== undefined` while both parsers normalize empty/whitespace to "unset → default ON", so `HF_DE_PARALLEL_ROUTER=` routed the render and latched `deParallelRouterUserManaged`, making `applyDeParallelRouterBreaker()` a no-op on fallback. The install then keeps retrying a failing router — losing precisely the first-fallback protection the PR is for. Ownership now uses the same normalization as the parsers:

deParallelRouterUserManaged = (process.env.HF_DE_PARALLEL_ROUTER ?? "").trim() !== "";

Pinned with the regression you asked for, parameterized over `""` and `" "`: both assert the env becomes `"false"` and the fired flag persists. Fault-injection verified — restoring !== undefined fails both.

@miga-heygen — nit 1: misleading message on explicit opt-in. Right, and the repeat-every-revert part is the worse half. The notice is now gated on the breaker having actually taken effect (fired && !deParallelRouterUserManaged), so an explicit opt-in no longer gets told the router is off when it is still on.

@miga-heygen — nit 2: missing explicit-"true"-survives-fallback test. Added, close to your sketch. That completes the "explicit user choice wins in both directions" pair — the opt-out half was already covered.

@miga-heygen — nit 3: gpu: false looked like a cross-PR seam. It was. Moved to #2838 where the gpu property originates; this branch no longer carries it. #2838's Typecheck failure was the same root cause and is fixed there.

69 CLI tests pass, typecheck/lint/fallow clean. Regression shards re-running on the new head.

[miguel-heygen]

Re-reviewed at 0fed335aa25d3b9ac08e6b0f3dc7ea11369d634c against the head where I requested changes. The blocker is closed at the contract boundary: ownership now uses the same normalization as both router parsers (trim() !== ""), so absent, empty, and whitespace-only values remain breaker-managed while any explicit non-empty user choice still wins.

The regression coverage is executable rather than declarative: the parameterized empty/whitespace cases drive a real reverted render outcome and assert both the in-process false latch and the persisted fired flag. The explicit-true case locks the opposite polarity, and the trip message is now emitted only when the breaker actually took control. I ran the exact CLI render suite at this head: 69/69 passed. All current PR checks are green.

No blocking findings. The previously noted programmatic mid-process env-mutation/parser-duplication concerns remain follow-up-grade and are unchanged by this delta.

Verdict: APPROVE
Reasoning: The empty-value ownership mismatch is fixed using the parser contract itself, both default-like spellings and explicit override behavior are regression-pinned, targeted tests pass, and CI is green.

— Magi

The base branch was changed.