From af535080a250bb150c0a6448517a075cd8076818 Mon Sep 17 00:00:00 2001 From: Vance Ingalls Date: Tue, 28 Jul 2026 01:33:46 -0700 Subject: [PATCH] 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 | 17 +++++++-- 2 files changed, 59 insertions(+), 3 deletions(-) diff --git a/packages/cli/src/commands/render.test.ts b/packages/cli/src/commands/render.test.ts index e684108f8..c3a5ac6d4 100644 --- a/packages/cli/src/commands/render.test.ts +++ b/packages/cli/src/commands/render.test.ts @@ -815,6 +815,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 2a25ad500..ab43e5a66 100644 --- a/packages/cli/src/commands/render.ts +++ b/packages/cli/src/commands/render.ts @@ -69,7 +69,6 @@ import { writeConfigWithResult, 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"; @@ -1185,8 +1184,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) { @@ -1326,7 +1333,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); } /**