From acdae81d2250a70098237629249e02227007fe7f Mon Sep 17 00:00:00 2001 From: Vance Ingalls Date: Tue, 28 Jul 2026 01:31:23 -0700 Subject: [PATCH 1/2] feat(producer): enable the parallel-DE router by default, behind a per-install circuit breaker MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The DE parallel router (HF_DE_PARALLEL_ROUTER) becomes default-ON. The soak answered the safety question it was gated on: zero damaged frames shipped — every fallback was the self-verification net catching a bad frame and recovering on the screenshot path. Verify PSNR p10 sits flat near 40 dB against a 32 dB floor. The residual 2.31% revert rate is an efficiency cost (a revert forfeits the speedup, never the output), accepted in exchange for parallelizing the >=700-frame band — roughly 80% of all DE capture wall-clock, frame-weighted. Default-ON is safe because the per-install circuit breaker stays underneath it. That distinction matters: 9.8% of installs hit a revert, and they are latched off permanently after the first one. Without the breaker those installs would go from "one slow render, then protected" to "every eligible render is slow". The breaker, adapted for a default-ON flag: - Writes an explicit HF_DE_PARALLEL_ROUTER=false and persists it to ~/.hyperframes/config.json, so the install stays off across processes. Absent no longer means off, so the switch has to be written, not unset. - Trips only on a real fallback, never on render count — a healthy install keeps the speedup indefinitely. - Independent of telemetry state: opting out of analytics must not cost a user the faster renderer. Telemetry governs reporting, not behavior. - An explicit user value wins in both directions, latched before the breaker can write the var and make the two indistinguishable. - The user is told when it trips and how to re-enable. isDeParallelRouterEnabled() parses the kill switch properly: false/0/off/no (case- and space-insensitive) disable; unset or empty is the default. A bare `!== "false"` would silently ignore every spelling but one and hand parallel DE to a user who asked for none. Refs PRINFRA-384 Co-Authored-By: Claude Opus 5 (1M context) --- packages/cli/src/commands/render.test.ts | 120 +++---- packages/cli/src/commands/render.ts | 317 +++++++++--------- packages/cli/src/commands/render/execute.ts | 5 +- .../src/services/renderOrchestrator.test.ts | 22 ++ .../src/services/renderOrchestrator.ts | 50 ++- 5 files changed, 274 insertions(+), 240 deletions(-) diff --git a/packages/cli/src/commands/render.test.ts b/packages/cli/src/commands/render.test.ts index 48f0ce7c20..c7553b545e 100644 --- a/packages/cli/src/commands/render.test.ts +++ b/packages/cli/src/commands/render.test.ts @@ -698,7 +698,7 @@ describe("renderLocal browser GPU config", () => { }); }); -describe("renderLocal — DE parallel-router CLI trial", () => { +describe("renderLocal — DE parallel-router circuit breaker", () => { let renderLocal: typeof import("./render.js").renderLocal; let resetTrialState: typeof import("./render.js").__resetDeParallelRouterTrialStateForTests; const savedEnv = new Map(); @@ -747,19 +747,22 @@ describe("renderLocal — DE parallel-router CLI trial", () => { browserGpuMode: "software" as const, hdrMode: "auto" as const, quiet: true, - // The trial is OPT-IN (review): only the CLI's own sequential call sites - // set this. These tests simulate those call sites. - enableDeParallelRouterTrial: true, + // Breaker management is OPT-IN (review): only the CLI's own sequential + // call sites set it. These tests simulate those call sites. + manageDeParallelRouterBreaker: true, }; - it("enables the trial (sets the env var) on a fresh install with telemetry on", async () => { + it("leaves the env var untouched on a fresh install — the router is default-ON", async () => { + // Under the old opt-in trial this armed HF_DE_PARALLEL_ROUTER="true". + // The router now ships on, so the breaker's job is to stay out of the + // way until something actually fails. configState.disk = { telemetryEnabled: true, deParallelRouterTrialFired: false, telemetryNoticeShown: true, }; await renderLocal("/tmp/project", "/tmp/out.mp4", baseOptions); - expect(process.env.HF_DE_PARALLEL_ROUTER).toBe("true"); + expect(process.env.HF_DE_PARALLEL_ROUTER).toBeUndefined(); }); it("does not override an env var the user already set themselves", async () => { @@ -773,49 +776,30 @@ describe("renderLocal — DE parallel-router CLI trial", () => { expect(process.env.HF_DE_PARALLEL_ROUTER).toBe("false"); }); - it("does not enable the trial once it has already fired for this install", async () => { + it("writes an explicit false once the breaker has tripped for this install", async () => { + // THE regression this rework exists for: the old code disabled the + // router by DELETING the var. With a default-ON router, absent means ON, + // so deleting would silently re-enable it on the very host that just + // failed. Only an explicit "false" is a real off-switch. configState.disk = { telemetryEnabled: true, deParallelRouterTrialFired: true, telemetryNoticeShown: true, }; await renderLocal("/tmp/project", "/tmp/out.mp4", baseOptions); - expect(process.env.HF_DE_PARALLEL_ROUTER).toBeUndefined(); - }); - - it("does not enable the trial when shouldTrack() is false (dev mode / DO_NOT_TRACK)", async () => { - configState.disk = { - telemetryEnabled: true, - deParallelRouterTrialFired: false, - telemetryNoticeShown: true, - }; - trackingState.shouldTrack = false; - await renderLocal("/tmp/project", "/tmp/out.mp4", baseOptions); - expect(process.env.HF_DE_PARALLEL_ROUTER).toBeUndefined(); + expect(process.env.HF_DE_PARALLEL_ROUTER).toBe("false"); }); - it("does not enable the trial when config.telemetryEnabled is false, even if shouldTrack() is stale-true (e.g. `hyperframes telemetry off` mid-batch)", async () => { + it("keeps the router on for a telemetry opt-out — analytics choice must not cost performance", async () => { + // The old trial refused to arm without recordable telemetry (no point + // running an experiment you can't measure). Now that the router is a + // shipped default, gating it on telemetry would punish a privacy choice + // with a slower renderer. configState.disk = { telemetryEnabled: false, deParallelRouterTrialFired: false, telemetryNoticeShown: true, }; - trackingState.shouldTrack = true; - await renderLocal("/tmp/project", "/tmp/out.mp4", baseOptions); - expect(process.env.HF_DE_PARALLEL_ROUTER).toBeUndefined(); - }); - - it("does not enable the trial before the first-run telemetry disclosure has been shown at least once", async () => { - // cli.ts shows this notice via a fire-and-forget, unawaited dynamic - // import — there's no guarantee it printed before renderLocal runs on a - // brand-new install's very first invocation. Requiring - // telemetryNoticeShown means the trial never races an opt-in message - // against the disclosure it depends on. - configState.disk = { - telemetryEnabled: true, - deParallelRouterTrialFired: false, - telemetryNoticeShown: false, - }; await renderLocal("/tmp/project", "/tmp/out.mp4", baseOptions); expect(process.env.HF_DE_PARALLEL_ROUTER).toBeUndefined(); }); @@ -925,11 +909,9 @@ describe("renderLocal — DE parallel-router CLI trial", () => { }); it("persists a later --batch row's revert even though this process already armed the trial on an earlier row", async () => { - // Regression test for the exact scenario a --batch run hits: multiple - // renderLocal calls in one process. Before the fix, row 2's - // maybeEnableDeParallelRouterTrial saw process.env.HF_DE_PARALLEL_ROUTER - // already "true" (set by row 1) and mistook that for "the user set it", - // returning trialArmed=false — silently dropping row 2's revert. + // The --batch scenario: multiple renderLocal calls in one process. Row 1 + // succeeds (breaker stays out of the way, env untouched); row 2 reverts + // and must still be recorded and trip the breaker. configState.disk = { telemetryEnabled: true, deParallelRouterTrialFired: false, @@ -943,7 +925,7 @@ describe("renderLocal — DE parallel-router CLI trial", () => { }; }; await renderLocal("/tmp/project", "/tmp/out.mp4", baseOptions); - expect(process.env.HF_DE_PARALLEL_ROUTER).toBe("true"); + expect(process.env.HF_DE_PARALLEL_ROUTER).toBeUndefined(); expect(configState.disk.deParallelRouterTrialFired).toBe(false); producerState.executeImpl = async (job) => { @@ -957,12 +939,14 @@ describe("renderLocal — DE parallel-router CLI trial", () => { expect(configState.writeConfigCalls).toContainEqual( expect.objectContaining({ deParallelRouterTrialFired: true }), ); - expect(process.env.HF_DE_PARALLEL_ROUTER).toBeUndefined(); + // Explicit "false", not deleted: with a default-ON router, unsetting the + // var would re-enable it on the host that just reverted. + expect(process.env.HF_DE_PARALLEL_ROUTER).toBe("false"); }); it("does not arm the trial for programmatic callers that never opted in (opt-in polarity — also covers --batch-concurrency N>=2, which leaves it unset)", async () => { // The trial's process-wide env var and module-level flags are only safe - // under sequential invocation, so enableDeParallelRouterTrial is OPT-IN + // under sequential invocation, so manageDeParallelRouterBreaker is OPT-IN // (review): a programmatic renderLocal consumer that doesn't know about // the trial must get no trial. The CLI's concurrent-batch path relies on // the same default by leaving the option unset. @@ -971,7 +955,7 @@ describe("renderLocal — DE parallel-router CLI trial", () => { deParallelRouterTrialFired: false, telemetryNoticeShown: true, }; - const { enableDeParallelRouterTrial: _omitted, ...programmaticOptions } = baseOptions; + const { manageDeParallelRouterBreaker: _omitted, ...programmaticOptions } = baseOptions; await renderLocal("/tmp/project", "/tmp/out.mp4", programmaticOptions); expect(process.env.HF_DE_PARALLEL_ROUTER).toBeUndefined(); expect(configState.writeConfigCalls).toHaveLength(0); @@ -990,7 +974,7 @@ describe("renderLocal — DE parallel-router CLI trial", () => { }; }; await renderLocal("/tmp/project", "/tmp/out.mp4", baseOptions); - expect(process.env.HF_DE_PARALLEL_ROUTER).toBe("true"); + expect(process.env.HF_DE_PARALLEL_ROUTER).toBeUndefined(); // A real interactive user can't do this mid-batch, but a wrapper script // invoking the CLI programmatically in the same process could — the @@ -1000,7 +984,10 @@ describe("renderLocal — DE parallel-router CLI trial", () => { expect(process.env.HF_DE_PARALLEL_ROUTER).toBe("false"); }); - it("caps exposure at DE_PARALLEL_ROUTER_TRIAL_MAX_RENDERS even when the router never reverts", async () => { + it("never trips on healthy renders, however many — the old 25-render cap is gone", async () => { + // The cap was sampling logic for an opt-in experiment. Under a shipped + // default it would switch the feature off behind the user's back after + // 25 good renders. configState.disk = { telemetryEnabled: true, deParallelRouterTrialFired: false, @@ -1013,40 +1000,14 @@ describe("renderLocal — DE parallel-router CLI trial", () => { }; }; - for (let i = 0; i < 25; i++) { + for (let i = 0; i < 30; i++) { await renderLocal("/tmp/project", "/tmp/out.mp4", baseOptions); } - expect(configState.writeConfigCalls).toContainEqual( - expect.objectContaining({ - deParallelRouterTrialFired: true, - deParallelRouterTrialRenderCount: 25, - }), - ); - expect(process.env.HF_DE_PARALLEL_ROUTER).toBeUndefined(); - - // The 26th eligible render must not re-arm it. - await renderLocal("/tmp/project", "/tmp/out.mp4", baseOptions); - expect(process.env.HF_DE_PARALLEL_ROUTER).toBeUndefined(); - }); - - it("observes a telemetry opt-out written by another process mid-batch (arm site reads fresh, not cached)", async () => { - configState.disk = { - telemetryEnabled: true, - deParallelRouterTrialFired: false, - telemetryNoticeShown: true, - }; - // Row 1 arms and primes the config cache. - await renderLocal("/tmp/project", "/tmp/out.mp4", baseOptions); - expect(process.env.HF_DE_PARALLEL_ROUTER).toBe("true"); - - // Another process runs `hyperframes telemetry off`, writing straight to - // "disk" — this process's cache still says telemetryEnabled: true, so a - // cached read at the arm site would keep arming (review finding). - configState.disk = { ...configState.disk, telemetryEnabled: false }; - - await renderLocal("/tmp/project", "/tmp/out.mp4", baseOptions); expect(process.env.HF_DE_PARALLEL_ROUTER).toBeUndefined(); + expect( + configState.writeConfigCalls.some((call) => call.deParallelRouterTrialFired === true), + ).toBe(false); }); it("re-asserts the fired flag when the write is lost (concurrent clobber / transient failure), without re-counting the render", async () => { @@ -1088,10 +1049,11 @@ describe("renderLocal — DE parallel-router CLI trial", () => { // Nothing could persist... expect(configState.disk.deParallelRouterTrialFired).toBe(false); // ...but the in-process latch still blocks the next render from - // re-running the experiment that just failed (review finding). + // re-running the path that just failed (review finding) — and now does + // it by writing an explicit "false", since absent means ON. producerState.executeImpl = async () => undefined; await renderLocal("/tmp/project", "/tmp/out.mp4", baseOptions); - expect(process.env.HF_DE_PARALLEL_ROUTER).toBeUndefined(); + expect(process.env.HF_DE_PARALLEL_ROUTER).toBe("false"); }); }); diff --git a/packages/cli/src/commands/render.ts b/packages/cli/src/commands/render.ts index a53cbfbc68..7cfabc6a96 100644 --- a/packages/cli/src/commands/render.ts +++ b/packages/cli/src/commands/render.ts @@ -62,7 +62,6 @@ import { } from "../telemetry/events.js"; import { maybePromptRenderFeedback } from "../telemetry/feedback.js"; import { readConfigFresh, writeConfig, type HyperframesConfig } from "../telemetry/config.js"; -import { shouldTrack } from "../telemetry/client.js"; import { renderJobObservabilityTelemetryPayload } from "../telemetry/renderObservability.js"; import { bytesToMb } from "../telemetry/system.js"; import { VERSION } from "../version.js"; @@ -414,21 +413,26 @@ export interface RenderOptions { /** Skip the interactive feedback prompt after a successful render. */ skipFeedback?: boolean; /** - * OPT IN to the DE parallel-router CLI trial - * (`maybeEnableDeParallelRouterTrial`) for this render. Default OFF — + * OPT IN to managing the DE parallel-router circuit breaker + * (`applyDeParallelRouterCircuitBreaker`) for this render. Default OFF — * only the top-level CLI render command's own call sites should ever set - * this (review): the trial mechanism shares one process-wide env var and - * two module-level flags across every `renderLocal` call in the process, + * this (review): the mechanism shares one process-wide env var and two + * module-level flags across every `renderLocal` call in the process, * which is safe for SEQUENTIAL calls (single render, single-concurrency * batch rows) but not for genuinely concurrent ones — racing invocations * could tear down or misattribute each other's outcome. Programmatic * consumers importing `renderLocal` (a future studio-server path, test - * harnesses, distributed runners) therefore get NO trial unless they - * explicitly opt in AND guarantee sequential invocation. The CLI sets - * this for single renders and for `--batch` at concurrency 1; it leaves - * it unset for `--batch-concurrency N>=2`. + * harnesses, distributed runners) therefore do not manage the breaker + * unless they explicitly opt in AND guarantee sequential invocation. The + * CLI sets this for single renders and for `--batch` at concurrency 1; it + * leaves it unset for `--batch-concurrency N>=2`. + * + * NOTE the asymmetry: the ROUTER itself is default-on for every consumer + * (the producer decides that). This flag only governs whether we + * additionally enforce the per-install breaker, because that is the part + * with process-wide state. */ - enableDeParallelRouterTrial?: boolean; + manageDeParallelRouterBreaker?: boolean; } /** @@ -828,10 +832,12 @@ export async function renderLocal( } const producer = await loadProducer(); - const deParallelRouterTrialArmed = maybeEnableDeParallelRouterTrial( - options.quiet, - options.enableDeParallelRouterTrial === true, - ); + const deParallelRouterActive = + options.manageDeParallelRouterBreaker === true + ? applyDeParallelRouterCircuitBreaker(options.quiet) + : // Not managing the breaker: the router still runs (producer default), + // we just don't enforce or record the per-install trip. + false; const startTime = Date.now(); const logger = createRenderTelemetryLogger( @@ -881,7 +887,7 @@ export async function renderLocal( try { await producer.executeRenderJob(job, projectDir, outputPath, onProgress); } catch (error: unknown) { - maybeConsumeDeParallelRouterTrial(deParallelRouterTrialArmed, job, options.quiet); + maybeConsumeDeParallelRouterTrial(deParallelRouterActive, job, options.quiet); handleRenderError( error, options, @@ -900,7 +906,7 @@ export async function renderLocal( // (win32/x64, CLI 0.7.58): valid MP4 on disk, exited 1 with no error print. markRenderSucceeded(); - maybeConsumeDeParallelRouterTrial(deParallelRouterTrialArmed, job, options.quiet); + maybeConsumeDeParallelRouterTrial(deParallelRouterActive, job, options.quiet); const elapsed = Date.now() - startTime; if (job.outcome === "completed_with_warnings") { for (const warning of job.warnings) { @@ -1059,37 +1065,40 @@ function createNoopProducerLogger(): ProducerLogger { }; } -/** Backstop cap: even absent an actual router failure, stop offering the - * trial after this many engaged (routed or reverted) renders for an - * install. Without this, a healthy router that never reverts would stay - * force-enabled on every eligible render forever (review finding). */ -const DE_PARALLEL_ROUTER_TRIAL_MAX_RENDERS = 25; +/** + * The 25-render exposure cap that bounded the old opt-in TRIAL is gone: the + * router is default-ON as of 2026-07-27, so "stop offering it after N + * renders" would mean switching a shipped default off behind the user's + * back. What survives is the half that was always safety rather than + * sampling — the per-install circuit breaker below, which latches the router + * off for good the first time a render has to fall back. + */ /** - * True across every `renderLocal` call in THIS process once the trial has - * armed `HF_DE_PARALLEL_ROUTER` here — distinct from the env var's own - * value, which stays "true" across an entire `--batch` run. Without this, - * a second batch row's `process.env.HF_DE_PARALLEL_ROUTER !== undefined` - * check can't tell "we set this ourselves on row 1" from "the user set - * this" and would wrongly treat itself as un-armed, silently dropping that - * row's outcome from ever reaching `maybeConsumeDeParallelRouterTrial` - * (review finding). + * The user set `HF_DE_PARALLEL_ROUTER` themselves (either polarity), latched + * once at first observation. Their choice wins over the circuit breaker in + * BOTH directions: we never overwrite an explicit opt-in with `"false"` on a + * fallback, and never overwrite an explicit opt-out either. Latched rather + * than re-read because the breaker itself writes the var — after the first + * write a live `process.env` read could no longer tell "the user set this" + * from "we set this" (the same distinction the old trial needed for + * `--batch` rows sharing one process). */ -let deParallelRouterTrialManagedByUs = false; +let deParallelRouterUserManaged = false; +let deParallelRouterUserManagedResolved = false; /** - * In-process latch mirroring `deParallelRouterTrialFired`: set the moment we - * DECIDE the trial is over, independent of whether persisting that decision - * to `~/.hyperframes/config.json` succeeds. `writeConfig` swallows all fs + * In-process latch mirroring the persisted `deParallelRouterTrialFired`: set + * the moment the breaker trips, independent of whether persisting that to + * `~/.hyperframes/config.json` succeeds. `writeConfig` swallows all fs * errors (by design — telemetry must never break the CLI), so on an - * unwritable config (root-owned file, disk full) the fired flag can never - * stick on disk; without this latch the trial would silently re-arm and - * re-fail on every subsequent render in this process forever (review - * finding). Later processes still re-arm — disk is the only cross-process - * channel — but each process now stops after at most one failure it - * couldn't record. + * unwritable config (root-owned file, disk full) the flag can never stick on + * disk; without this latch the router would re-enable and re-fail on every + * subsequent render in this process. Later processes re-arm — disk is the + * only cross-process channel — but each process now stops after at most one + * failure it couldn't record. */ -let deParallelRouterTrialFiredThisProcess = false; +let deParallelRouterBreakerTrippedThisProcess = false; /** * Test-only reset for the module-level trial state — a real CLI process @@ -1099,111 +1108,101 @@ let deParallelRouterTrialFiredThisProcess = false; */ // fallow-ignore-next-line unused-export export function __resetDeParallelRouterTrialStateForTests(): void { - deParallelRouterTrialManagedByUs = false; - deParallelRouterTrialFiredThisProcess = false; + deParallelRouterBreakerTrippedThisProcess = false; + deParallelRouterUserManaged = false; + deParallelRouterUserManagedResolved = false; } /** - * True once the trial should stop offering itself: already failed (on disk - * or via this process's in-memory latch), hit the render-count backstop, or - * telemetry isn't actually recordable right now. + * Has this install's router circuit breaker already tripped — on disk, or via + * this process's in-memory latch? * - * Checks BOTH `shouldTrack()` and `config.telemetryEnabled` directly, not - * `shouldTrack()` alone: `shouldTrack()` (`../telemetry/client.js`) memoizes - * its verdict once per process and never invalidates, so during a long - * `--batch` run (all rows share one process) a `hyperframes telemetry off` - * issued from another terminal mid-batch would never be observed. The - * caller must pass a `readConfigFresh()` snapshot for the same reason — - * `readConfig()` serves a process-lifetime cache that is exactly as stale - * as the `shouldTrack()` memoization this check exists to bypass (review - * finding). + * Deliberately does NOT consider telemetry state. The old opt-in trial did: + * there was no point running an experimental path if the resulting signal + * couldn't be recorded. Now that the router is a shipped default, gating it + * on telemetry would mean users who opted out of analytics silently get a + * slower renderer — punishing a privacy choice with a performance penalty + * (review finding). Telemetry state governs REPORTING, never behavior. */ -function isDeParallelRouterTrialBlocked(config: HyperframesConfig): boolean { - const overRenderCap = - (config.deParallelRouterTrialRenderCount ?? 0) >= DE_PARALLEL_ROUTER_TRIAL_MAX_RENDERS; - return ( - deParallelRouterTrialFiredThisProcess || - Boolean(config.deParallelRouterTrialFired) || - overRenderCap || - !config.telemetryEnabled || - !shouldTrack() || - // cli.ts shows the first-run telemetry disclosure via a fire-and-forget, - // unawaited dynamic import — there's no guarantee it has printed before - // this render command reaches this point. Requiring telemetryNoticeShown - // means the trial simply never offers itself on a fresh install's very - // first invocation (before the disclosure is guaranteed to have run at - // least once), rather than racing an experimental opt-in message against - // the disclosure it depends on (review finding). - !config.telemetryNoticeShown - ); +function hasDeParallelRouterBreakerTripped(config: HyperframesConfig): boolean { + return deParallelRouterBreakerTrippedThisProcess || Boolean(config.deParallelRouterTrialFired); +} + +/** + * Latch the router OFF for the rest of this process by writing an explicit + * `"false"`. + * + * Under the old default-OFF flag this deleted the var, because absent meant + * off. With the router default-ON, deleting means ON — the same call would + * silently RE-ENABLE the router on exactly the host that just failed + * (review finding). Writing the explicit value is what makes the breaker a + * breaker. No-op when the user set the var themselves: their choice wins in + * both directions. + */ +/** + * Mirror of the producer's `isDeParallelRouterEnabled`. Deliberately + * duplicated rather than imported: `@hyperframes/producer` is lazily loaded + * (`loadProducer()`) to keep CLI startup fast, and this runs on the startup + * path. Keep the two in sync — the producer copy is the source of truth. + */ +function userValueEnablesDeParallelRouter(): boolean { + const raw = process.env.HF_DE_PARALLEL_ROUTER?.trim().toLowerCase(); + if (raw === undefined || raw === "") return true; + return !(raw === "false" || raw === "0" || raw === "off" || raw === "no"); } -/** Shared cleanup for both `maybeEnableDeParallelRouterTrial` (this process - * should stop offering the trial) and `maybeConsumeDeParallelRouterTrial` - * (the trial just failed/hit its cap) — a no-op unless WE were the ones - * managing the env var. */ -function stopManagingDeParallelRouterTrial(): void { - if (!deParallelRouterTrialManagedByUs) return; - delete process.env.HF_DE_PARALLEL_ROUTER; - deParallelRouterTrialManagedByUs = false; +function applyDeParallelRouterBreaker(): void { + if (deParallelRouterUserManaged) return; + process.env.HF_DE_PARALLEL_ROUTER = "false"; } /** - * Enable the DE parallel-router experiment (`HF_DE_PARALLEL_ROUTER`, default - * off) for this render, on every eligible render for this install (up to - * `DE_PARALLEL_ROUTER_TRIAL_MAX_RENDERS`), so we get real-traffic router - * telemetry (revert rate, verify-db distribution) without requiring anyone - * to manually set the env var — see `HyperframesConfig.deParallelRouterTrialFired`. - * See `maybeConsumeDeParallelRouterTrial` for what turns it off. Returns - * whether this call armed it (so the caller knows to check for consumption - * afterward) — false unless the caller explicitly opted in (`enabled` — - * OPT-IN polarity, review: only the top-level CLI render command's own - * sequential call sites set it; programmatic `renderLocal` consumers get no - * trial by default because the mechanism's process-wide state is unsafe - * under concurrent invocation — see - * `RenderOptions.enableDeParallelRouterTrial`), if it's already failed (or - * hit the render cap) for this install, if the user already set the env var - * themselves (never override an explicit choice — see - * `deParallelRouterTrialManagedByUs` for how a later `--batch` row - * distinguishes that from our own earlier arm), or if telemetry isn't - * actually recordable right now (see `isDeParallelRouterTrialBlocked`; no - * point risking the experimental path if we can't even record the - * resulting signal). + * Apply this install's router circuit breaker before a render. + * + * The router is default-ON, so the normal path does NOTHING here — the + * producer's own default takes over. This exists for the one case that must + * survive a shipped default: an install that already had a render fall back + * stays off, permanently, across processes (the verdict is persisted to + * `~/.hyperframes/config.json`). See `maybeConsumeDeParallelRouterTrial` for + * what trips it. + * + * Returns whether the router is active for this render, so the caller knows + * to inspect the outcome afterward. */ -function maybeEnableDeParallelRouterTrial(quiet: boolean, enabled: boolean): boolean { - if (!enabled) return false; - // The in-process latch alone decides once it's set — short-circuit before - // the disk read so post-fired batch rows don't pay a config read + parse + - // shared-cache invalidation per row for an answer module state already - // knows (review finding). - if (deParallelRouterTrialFiredThisProcess) { - stopManagingDeParallelRouterTrial(); - return false; +function applyDeParallelRouterCircuitBreaker(quiet: boolean): boolean { + // Latch the user's own choice on first observation, BEFORE the breaker can + // write the var itself and make the two indistinguishable. + if (!deParallelRouterUserManagedResolved) { + deParallelRouterUserManaged = process.env.HF_DE_PARALLEL_ROUTER !== undefined; + deParallelRouterUserManagedResolved = true; } - const userSetIt = - process.env.HF_DE_PARALLEL_ROUTER !== undefined && !deParallelRouterTrialManagedByUs; - if (userSetIt) return false; - - // readConfigFresh, NOT readConfig: the cached read is exactly as stale as - // the shouldTrack() memoization the blocked-check exists to bypass — a - // mid-batch `hyperframes telemetry off` (or another process persisting - // fired=true) would never be observed through the cache (review finding). - if (isDeParallelRouterTrialBlocked(readConfigFresh())) { - stopManagingDeParallelRouterTrial(); - return false; + if (deParallelRouterUserManaged) { + // Explicit choice, either polarity — report whether it enables the + // router so outcomes are still consumed, but never override it. + return userValueEnablesDeParallelRouter(); } - if (deParallelRouterTrialManagedByUs) return true; - deParallelRouterTrialManagedByUs = true; - process.env.HF_DE_PARALLEL_ROUTER = "true"; - if (!quiet) { - console.log( - c.dim( - " Trying the experimental parallel drawElement capture path for this install " + - "(disabled automatically if it ever needs to fall back; opt out anytime: " + - "HF_DE_PARALLEL_ROUTER=false)", - ), - ); + // The in-process latch decides once set — short-circuit before the disk + // read so post-trip batch rows don't pay a config read + parse per row for + // an answer module state already knows. + if (deParallelRouterBreakerTrippedThisProcess) { + applyDeParallelRouterBreaker(); + return false; + } + // readConfigFresh, NOT readConfig: the cached read is process-lifetime, so + // another process persisting a trip mid-`--batch` would never be observed. + if (hasDeParallelRouterBreakerTripped(readConfigFresh())) { + deParallelRouterBreakerTrippedThisProcess = true; + applyDeParallelRouterBreaker(); + if (!quiet) { + console.log( + c.dim( + " Parallel drawElement capture stays off for this install (a previous render " + + "had to fall back). Re-enable with HF_DE_PARALLEL_ROUTER=true.", + ), + ); + } + return false; } return true; } @@ -1280,40 +1279,58 @@ function persistDeParallelRouterTrialFired(): boolean { * `persistDeParallelRouterTrialFired`. */ function maybeConsumeDeParallelRouterTrial( - trialArmed: boolean, + routerActive: boolean, job: RenderJob, quiet: boolean, ): void { - if (!trialArmed) return; + if (!routerActive) return; const outcome = resolveDeParallelRouterOutcome(job); if (outcome === undefined) return; const config = readConfigFresh(); const renderCount = (config.deParallelRouterTrialRenderCount ?? 0) + 1; config.deParallelRouterTrialRenderCount = renderCount; - const fired = outcome === "reverted" || renderCount >= DE_PARALLEL_ROUTER_TRIAL_MAX_RENDERS; + // Trip ONLY on an actual fallback. The old trial also tripped at a + // 25-render exposure cap, which was sampling logic: bound how long an + // experiment force-enables itself. Under a shipped default that would + // switch the feature off behind the user's back after 25 good renders. + const fired = outcome === "reverted"; if (fired) { config.deParallelRouterTrialFired = true; // Latch BEFORE attempting persistence — the decision holds for this // process even if the disk write never sticks (unwritable config). - deParallelRouterTrialFiredThisProcess = true; - stopManagingDeParallelRouterTrial(); + deParallelRouterBreakerTrippedThisProcess = true; + applyDeParallelRouterBreaker(); } writeConfig(config); - // `!quiet`-gated like every other trial message: quiet/batch-json renders - // must produce no unexpected terminal output — CI wrappers asserting - // empty stderr would misread the warning as a render failure (review - // finding). The in-process latch above already guarantees the safety - // behavior the warning describes, whether or not it prints. - if (fired && !persistDeParallelRouterTrialFired() && !quiet) { - console.warn( - c.warn( - " Could not persist the parallel drawElement trial's off-switch to " + - "~/.hyperframes/config.json (unwritable?). The experiment stays off for this " + - "process; future runs may retry it. Set HF_DE_PARALLEL_ROUTER=false to opt out.", - ), - ); - } + if (fired) reportDeParallelRouterBreakerTrip(quiet); +} + +/** + * Tell the user the breaker tripped, and warn if the verdict couldn't be + * persisted. All output is `!quiet`-gated: quiet/batch-json renders must + * produce no unexpected terminal output — CI wrappers asserting empty stderr + * would misread a warning as a render failure (review finding). The + * in-process latch guarantees the safety behavior either way. + */ +function reportDeParallelRouterBreakerTrip(quiet: boolean): void { + const persisted = persistDeParallelRouterTrialFired(); + if (quiet) return; + console.log( + c.dim( + " A frame failed verification, so parallel drawElement capture fell back to the " + + "screenshot path and is now off for this install. Re-enable: " + + "HF_DE_PARALLEL_ROUTER=true", + ), + ); + if (persisted) return; + console.warn( + c.warn( + " Could not persist the parallel drawElement circuit breaker to " + + "~/.hyperframes/config.json (unwritable?). It stays off for this process; " + + "future runs may retry it. Set HF_DE_PARALLEL_ROUTER=false to opt out for good.", + ), + ); } function handleRenderError( diff --git a/packages/cli/src/commands/render/execute.ts b/packages/cli/src/commands/render/execute.ts index e35748c24a..a2a3b4482a 100644 --- a/packages/cli/src/commands/render/execute.ts +++ b/packages/cli/src/commands/render/execute.ts @@ -34,7 +34,6 @@ export interface RenderExecutionDependencies { } // Exported only through render.ts so command tests can lock the user-facing guidance. -// fallow-ignore-next-line unused-export export function renderLintContinuationHint(strictErrors: boolean): string { return strictErrors ? " Continuing render despite lint warnings. Use --strict-all to block warnings." @@ -109,7 +108,7 @@ export async function executeRenderPlan( protocolTimeout: plan.protocolTimeout, playerReadyTimeout: plan.playerReadyTimeout, exitAfterComplete: true, - enableDeParallelRouterTrial: true, + manageDeParallelRouterBreaker: true, }; if (plan.useDocker) { options.pageSideCompositing = plan.pageSideCompositing; @@ -259,7 +258,7 @@ async function executeBatchRender( exitAfterComplete: false, throwOnError: true, skipFeedback: true, - enableDeParallelRouterTrial: plan.batchConcurrency <= 1, + manageDeParallelRouterBreaker: plan.batchConcurrency <= 1, }; const manifest = await batchModule.runBatchRender({ prepared: preparedBatch, diff --git a/packages/producer/src/services/renderOrchestrator.test.ts b/packages/producer/src/services/renderOrchestrator.test.ts index ff34b7e17f..2b2686b70f 100644 --- a/packages/producer/src/services/renderOrchestrator.test.ts +++ b/packages/producer/src/services/renderOrchestrator.test.ts @@ -33,6 +33,7 @@ import { resolveParallelRouterRetryPlan, resetCaptureAttemptProgress, shouldRetryViaPinnedFallback, + isDeParallelRouterEnabled, shouldPreferParallelDrawElement, shouldPreferSingleWorkerDrawElement, shouldStreamParallelCapture, @@ -1827,6 +1828,27 @@ describe("resolveInversionRetryPlan (self-verify retry rollback)", () => { }); }); +describe("isDeParallelRouterEnabled (kill switch parsing)", () => { + it("defaults ON when unset or set-but-empty", () => { + expect(isDeParallelRouterEnabled({})).toBe(true); + expect(isDeParallelRouterEnabled({ HF_DE_PARALLEL_ROUTER: "" })).toBe(true); + expect(isDeParallelRouterEnabled({ HF_DE_PARALLEL_ROUTER: " " })).toBe(true); + }); + + it("honours every conventional spelling of off — an opt-out must never fail OPEN", () => { + // A naive `!== "false"` would enable the router for all of these, handing + // 3-worker parallel DE to a user who explicitly asked for none. + for (const v of ["false", "FALSE", "False", "0", "off", "OFF", "no", "No", " false "]) { + expect(isDeParallelRouterEnabled({ HF_DE_PARALLEL_ROUTER: v })).toBe(false); + } + }); + + it("treats any other value as enabled", () => { + expect(isDeParallelRouterEnabled({ HF_DE_PARALLEL_ROUTER: "true" })).toBe(true); + expect(isDeParallelRouterEnabled({ HF_DE_PARALLEL_ROUTER: "1" })).toBe(true); + }); +}); + describe("shouldPreferParallelDrawElement (DE parallel router)", () => { const eligible = { workerCount: 5, diff --git a/packages/producer/src/services/renderOrchestrator.ts b/packages/producer/src/services/renderOrchestrator.ts index 79f4e6494e..5282147d19 100644 --- a/packages/producer/src/services/renderOrchestrator.ts +++ b/packages/producer/src/services/renderOrchestrator.ts @@ -1300,11 +1300,12 @@ export function resolveInversionRetryPlan(args: { * clear 1.25x (3,600f, 39% static/dedup-heavy) still didn't LOSE to single- * worker (1.16x) — dedup already skips the capture work parallelism would * split, so there's mechanically less headroom, not a regression. No comp - * anywhere showed par3 < single. Default-off (HF_DE_PARALLEL_ROUTER): this - * promotes the opt-in mechanism from #2056 into the auto-routing decision, - * but the decision itself stays gated behind its own flag pending the - * telemetry soak (revert rate, de_verify_min_db distribution) on real wild - * traffic — there is currently none, since nothing routes here by default. + * anywhere showed par3 < single. Default ON since 2026-07-27 + * (HF_DE_PARALLEL_ROUTER=false is the kill switch): the default-off soak + * proved the safety half (zero shipped damage, 100% revert recovery), so the + * flip trades an accepted ~2.3% revert rate for parallelizing the ≥700f + * band. This promotes the opt-in mechanism from #2056 into the auto-routing + * decision. * Takes priority over the single-worker inversion when both would fire. * Re-calibrated 2026-07-27: a controlled crossover sweep (three content * profiles including a genuinely init-expensive 24-sub-composition comp; @@ -1314,6 +1315,29 @@ export function resolveInversionRetryPlan(args: { * dropped below the inversion's threshold (700 vs 900): where both fire, * parallel wins over the inversion's single-worker pick (+17–21% at 700f). */ +/** + * Is the DE parallel router enabled for this process? + * + * Default ON since 2026-07-27; `HF_DE_PARALLEL_ROUTER` is the kill switch. + * Every conventional spelling of "off" disables it — a naive + * `!== "false"` would silently ignore `0`, `off`, `no`, `FALSE`, and an + * exported-but-empty var, i.e. an opt-out that FAILS OPEN and hands the user + * 3-worker parallel DE anyway (review finding). A set-but-empty value means + * "unset" here, matching how the sibling HF_DE_* numeric knobs treat it. + * + * The CLI's circuit breaker relies on this accepting an explicit "false": + * once an install trips the breaker it writes that value rather than + * unsetting the var, because under a default-ON flag unsetting means ON. + * Pure; exported for tests. + */ +export function isDeParallelRouterEnabled( + env: Readonly>, +): boolean { + const raw = env.HF_DE_PARALLEL_ROUTER?.trim().toLowerCase(); + if (raw === undefined || raw === "") return true; + return !(raw === "false" || raw === "0" || raw === "off" || raw === "no"); +} + export function shouldPreferParallelDrawElement(args: { workerCount: number; /** job.config.workers — a number means the user explicitly chose. */ @@ -1329,7 +1353,7 @@ export function shouldPreferParallelDrawElement(args: { supersampling: boolean; probeDeGated: boolean; experimentalParallelDeOptIn: boolean; - /** HF_DE_PARALLEL_ROUTER === "true" — the router's own kill switch, default off. */ + /** HF_DE_PARALLEL_ROUTER !== "false" — default ON since 2026-07-27; env var is the kill switch. */ routerEnabled: boolean; /** * Whether verified parallel DE STREAMING can actually run for this render @@ -2326,7 +2350,17 @@ async function executeRenderPipeline(input: { process.env.HF_DE_PARALLEL_STREAM === "true", }); // DE parallel-router eligibility — see shouldPreferParallelDrawElement. - // Default-off (HF_DE_PARALLEL_ROUTER); HF_DE_PARALLEL_MIN_FRAMES default + // Default ON since 2026-07-27 (kill switch: HF_DE_PARALLEL_ROUTER=false). + // The soak that gated this flip answered the safety question: zero + // damaged frames shipped across the entire default-off window — every + // revert was the self-verify net catching a bad frame and recovering via + // screenshot. The residual metric (revert rate ~2.3% vs the 2% goal) is + // an efficiency cost (a revert forfeits the speedup, never correctness), + // accepted in exchange for parallelizing the ≥700-frame band (~80% of + // all DE capture wall-clock). Post-flip tripwire on dashboard 1807532: + // sustained revert >10% or any verify-missed damage rolls this back — + // one env default, decoupled from the floor change one release earlier. + // HF_DE_PARALLEL_MIN_FRAMES default // 700, re-calibrated 2026-07-27 from the original safe-high 2000. A // controlled frame-count sweep (fixed content-per-frame, three synthetic // profiles × {350..3000f} × {single,par2,par3} × 3 reps, worker counts + @@ -2338,7 +2372,7 @@ async function executeRenderPipeline(input: { // duplicated init costs CPU, not wall-clock. Below ~700f the win thins // toward ~+10% while still paying 3 hardware-GPU browsers, so the floor // stays. Harness: plans/drawelement-fast-capture/de-crossover-bench.sh. - const deParallelRouterEnabled = process.env.HF_DE_PARALLEL_ROUTER === "true"; + const deParallelRouterEnabled = isDeParallelRouterEnabled(process.env); const deParallelMinFramesRaw = process.env.HF_DE_PARALLEL_MIN_FRAMES; const deParallelMinFramesNum = deParallelMinFramesRaw === undefined || deParallelMinFramesRaw.trim() === "" From 0fed335aa25d3b9ac08e6b0f3dc7ea11369d634c Mon Sep 17 00:00:00 2001 From: Vance Ingalls Date: Tue, 28 Jul 2026 01:33:46 -0700 Subject: [PATCH 2/2] fix(cli): keep a set-but-empty router env var breaker-managed (review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Ownership detection classified ANY defined HF_DE_PARALLEL_ROUTER as a user choice, but both parsers read empty/whitespace as "unset -> default ON". Launching with `HF_DE_PARALLEL_ROUTER=` therefore routed the render (empty parses as ON) while exempting the install from its circuit breaker: after a verified fallback applyDeParallelRouterBreaker() no-op'd, so the install kept retrying the failing router instead of latching off. That is the exact first-fallback protection this PR exists to provide, lost on a documented default path. Ownership now uses the same normalization as the parsers. Also: only announce a trip the breaker could act on. With an explicit user opt-in the breaker is deliberately a no-op, so "now off for this install" was factually wrong — and reprinted on every later revert, since the user's value keeps the router active. Tests: set-but-empty and whitespace both latch off and persist the fired flag (fault-injection verified — restoring the old check fails both); explicit "true" survives a fallback. Co-Authored-By: Claude Opus 5 (1M context) --- packages/cli/src/commands/render.test.ts | 45 ++++++++++++++++++++++++ packages/cli/src/commands/render.ts | 16 +++++++-- 2 files changed, 59 insertions(+), 2 deletions(-) diff --git a/packages/cli/src/commands/render.test.ts b/packages/cli/src/commands/render.test.ts index c7553b545e..a4345f2aa3 100644 --- a/packages/cli/src/commands/render.test.ts +++ b/packages/cli/src/commands/render.test.ts @@ -790,6 +790,51 @@ describe("renderLocal — DE parallel-router circuit breaker", () => { expect(process.env.HF_DE_PARALLEL_ROUTER).toBe("false"); }); + for (const emptyish of ["", " "]) { + it(`treats a set-but-empty env var (${JSON.stringify(emptyish)}) as default, not a user choice`, async () => { + // Both parsers read empty/whitespace as "unset → default ON", so the + // producer routes. If ownership instead treated any defined value as a + // user choice, the breaker would no-op and this install would keep + // retrying a failing router forever — losing the first-fallback + // protection that is the point of the breaker. + configState.disk = { + telemetryEnabled: true, + deParallelRouterTrialFired: false, + telemetryNoticeShown: true, + }; + process.env.HF_DE_PARALLEL_ROUTER = emptyish; + producerState.executeImpl = async (job) => { + job.perfSummary = { + resolution: { width: 100, height: 100 }, + drawElement: { parallelRouter: "reverted" }, + }; + }; + await renderLocal("/tmp/project", "/tmp/out.mp4", baseOptions); + expect(process.env.HF_DE_PARALLEL_ROUTER).toBe("false"); + expect(configState.writeConfigCalls).toContainEqual( + expect.objectContaining({ deParallelRouterTrialFired: true }), + ); + }); + } + + it("does not override an explicit user opt-in even after a fallback", async () => { + // "Explicit user choice wins in both directions" — the opt-in half. + configState.disk = { + telemetryEnabled: true, + deParallelRouterTrialFired: false, + telemetryNoticeShown: true, + }; + process.env.HF_DE_PARALLEL_ROUTER = "true"; + producerState.executeImpl = async (job) => { + job.perfSummary = { + resolution: { width: 100, height: 100 }, + drawElement: { parallelRouter: "reverted" }, + }; + }; + await renderLocal("/tmp/project", "/tmp/out.mp4", baseOptions); + expect(process.env.HF_DE_PARALLEL_ROUTER).toBe("true"); + }); + it("keeps the router on for a telemetry opt-out — analytics choice must not cost performance", async () => { // The old trial refused to arm without recordable telemetry (no point // running an experiment you can't measure). Now that the router is a diff --git a/packages/cli/src/commands/render.ts b/packages/cli/src/commands/render.ts index 7cfabc6a96..fa8cedd6a3 100644 --- a/packages/cli/src/commands/render.ts +++ b/packages/cli/src/commands/render.ts @@ -1172,8 +1172,16 @@ function applyDeParallelRouterBreaker(): void { function applyDeParallelRouterCircuitBreaker(quiet: boolean): boolean { // Latch the user's own choice on first observation, BEFORE the breaker can // write the var itself and make the two indistinguishable. + // + // Ownership uses the SAME normalization as the two parsers: a set-but-empty + // (or whitespace) value means "unset / default ON", so it is NOT a user + // choice and must stay breaker-managed. Treating any defined value as + // user-managed would let `HF_DE_PARALLEL_ROUTER=` route the render (empty + // parses as ON) while exempting that install from the breaker — it would + // keep retrying a failing router forever, losing exactly the first-fallback + // protection this PR exists to provide (review finding). if (!deParallelRouterUserManagedResolved) { - deParallelRouterUserManaged = process.env.HF_DE_PARALLEL_ROUTER !== undefined; + deParallelRouterUserManaged = (process.env.HF_DE_PARALLEL_ROUTER ?? "").trim() !== ""; deParallelRouterUserManagedResolved = true; } if (deParallelRouterUserManaged) { @@ -1303,7 +1311,11 @@ function maybeConsumeDeParallelRouterTrial( applyDeParallelRouterBreaker(); } writeConfig(config); - if (fired) reportDeParallelRouterBreakerTrip(quiet); + // Only announce a trip the breaker could actually act on. With an explicit + // user opt-in the breaker is a no-op, so "now off for this install" would + // be false — and would reprint on every subsequent revert, since the user's + // value keeps the router active (review finding). + if (fired && !deParallelRouterUserManaged) reportDeParallelRouterBreakerTrip(quiet); } /**