diff --git a/packages/cli/src/commands/render.test.ts b/packages/cli/src/commands/render.test.ts index 48f0ce7c20..a4345f2aa3 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,75 @@ 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(); + expect(process.env.HF_DE_PARALLEL_ROUTER).toBe("false"); }); - it("does not enable the trial when shouldTrack() is false (dev mode / DO_NOT_TRACK)", async () => { + 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, }; - trackingState.shouldTrack = false; + 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).toBeUndefined(); + expect(process.env.HF_DE_PARALLEL_ROUTER).toBe("true"); }); - 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 +954,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 +970,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 +984,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 +1000,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 +1019,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 +1029,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 +1045,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 +1094,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..fa8cedd6a3 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,109 @@ 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); } -/** 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; +/** + * 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"); +} + +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. + // + // 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 ?? "").trim() !== ""; + 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 +1287,62 @@ 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.", - ), - ); - } + // 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); +} + +/** + * 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() === ""