From 19f90b0b92aad1bdf03df81c01a9664609fd8065 Mon Sep 17 00:00:00 2001 From: Vance Ingalls Date: Thu, 9 Jul 2026 21:49:21 -0700 Subject: [PATCH] fix(cli): keep the DE parallel-router trial on until a real failure, not first engagement MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Only consuming telemetry from one data point per install badly undersampled the "routed" (successful) outcome — the far more common case. Changed maybeConsumeDeParallelRouterTrial to only turn the trial off when the router's OWN safety net actually fired (deParallelRouter === "reverted"), not on a clean "routed" success. This runs the experiment on every eligible render for an install indefinitely until it hits one real failure, then stops for that install going forward — trading a slightly higher per-install ceiling on experimental-path exposure for dramatically more successful- routing telemetry volume across the fleet. Also fixed a related edge case while updating this: a render that merely "routed" (router fired, self-verify never even tripped) but then crashed for an unrelated reason (e.g. cancellation) no longer counts as a router failure — only "reverted" (the router's fallback path actually engaged) does. Cancelling a render isn't evidence the router is unsafe. Co-Authored-By: Claude Sonnet 5 --- packages/cli/src/commands/render.test.ts | 30 ++++++++++++-- packages/cli/src/commands/render.ts | 52 +++++++++++++++--------- packages/cli/src/telemetry/config.ts | 14 ++++--- 3 files changed, 67 insertions(+), 29 deletions(-) diff --git a/packages/cli/src/commands/render.test.ts b/packages/cli/src/commands/render.test.ts index 551f086e4..f6e1e6931 100644 --- a/packages/cli/src/commands/render.test.ts +++ b/packages/cli/src/commands/render.test.ts @@ -505,7 +505,7 @@ describe("renderLocal — DE parallel-router CLI trial", () => { expect(process.env.HF_DE_PARALLEL_ROUTER).toBeUndefined(); }); - it("persists deParallelRouterTrialFired when the router actually engages on a successful render", async () => { + it("does NOT persist the trial as fired on a clean 'routed' success — keeps trying on future renders", async () => { configState.config = { telemetryEnabled: true, deParallelRouterTrialFired: false }; producerState.executeImpl = async (job) => { job.perfSummary = { @@ -514,6 +514,18 @@ describe("renderLocal — DE parallel-router CLI trial", () => { }; }; await renderLocal("/tmp/project", "/tmp/out.mp4", baseOptions); + expect(configState.writeConfigCalls).toHaveLength(0); + }); + + it("persists the trial as fired when the router's own safety net actually reverted", async () => { + configState.config = { telemetryEnabled: true, deParallelRouterTrialFired: false }; + producerState.executeImpl = async (job) => { + job.perfSummary = { + resolution: { width: 100, height: 100 }, + drawElement: { parallelRouter: "reverted" }, + }; + }; + await renderLocal("/tmp/project", "/tmp/out.mp4", baseOptions); expect(configState.writeConfigCalls).toContainEqual( expect.objectContaining({ deParallelRouterTrialFired: true }), ); @@ -528,11 +540,23 @@ describe("renderLocal — DE parallel-router CLI trial", () => { expect(configState.writeConfigCalls).toHaveLength(0); }); - it("persists the trial as fired from the failure path when the router engaged before a hard crash", async () => { + it("does NOT persist the trial as fired when a render merely 'routed' crashes for an unrelated reason (e.g. cancellation) — not a router failure", async () => { configState.config = { telemetryEnabled: true, deParallelRouterTrialFired: false }; producerState.executeImpl = async (job) => { job.errorDetails = { observability: { capture: { deParallelRouter: "routed" } } }; - throw new Error("worker crashed"); + throw new Error("render cancelled"); + }; + await renderLocal("/tmp/project", "/tmp/out.mp4", { ...baseOptions, throwOnError: true }).catch( + () => {}, + ); + expect(configState.writeConfigCalls).toHaveLength(0); + }); + + it("persists the trial as fired from the failure path when the router's safety net reverted but the retry still failed", async () => { + configState.config = { telemetryEnabled: true, deParallelRouterTrialFired: false }; + producerState.executeImpl = async (job) => { + job.errorDetails = { observability: { capture: { deParallelRouter: "reverted" } } }; + throw new Error("worker crashed even after fallback"); }; await renderLocal("/tmp/project", "/tmp/out.mp4", { ...baseOptions, throwOnError: true }).catch( () => {}, diff --git a/packages/cli/src/commands/render.ts b/packages/cli/src/commands/render.ts index 10a9e9f39..86133395b 100644 --- a/packages/cli/src/commands/render.ts +++ b/packages/cli/src/commands/render.ts @@ -1612,14 +1612,17 @@ function createNoopProducerLogger(): ProducerLogger { /** * Enable the DE parallel-router experiment (`HF_DE_PARALLEL_ROUTER`, default - * off) for this render, one time per install, so we get real-traffic router - * telemetry (revert rate, verify-db distribution) without requiring anyone - * to manually set the env var — see `HyperframesConfig.deParallelRouterTrialFired`. + * off) for this render, on EVERY eligible render for this install, so we get + * real-traffic router telemetry (revert rate, verify-db distribution) + * without requiring anyone to manually set the env var — see + * `HyperframesConfig.deParallelRouterTrialFired`. Deliberately runs + * indefinitely (not just once) to maximize "routed" success-telemetry + * volume; see `maybeConsumeDeParallelRouterTrial` for what turns it off. * Returns whether this call armed it (so the caller knows to check for - * consumption afterward) — false if it's already fired once, or the user - * already set the env var themselves (never override an explicit choice), - * or telemetry is disabled (no point risking the experimental path if we - * can't even record the resulting signal). + * consumption afterward) — false if it's already failed once for this + * install, or the user already set the env var themselves (never override + * an explicit choice), or telemetry is disabled (no point risking the + * experimental path if we can't even record the resulting signal). */ function maybeEnableDeParallelRouterTrial(quiet: boolean): boolean { if (process.env.HF_DE_PARALLEL_ROUTER !== undefined) return false; @@ -1629,8 +1632,9 @@ function maybeEnableDeParallelRouterTrial(quiet: boolean): boolean { if (!quiet) { console.log( c.dim( - " Trying the experimental parallel drawElement capture path once for this install " + - "(opt out: HF_DE_PARALLEL_ROUTER=false)", + " 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)", ), ); } @@ -1638,20 +1642,28 @@ function maybeEnableDeParallelRouterTrial(quiet: boolean): boolean { } /** - * After a trial-armed render, persist that the experiment actually engaged - * (routed or reverted — either produces telemetry) so it's never enabled - * again for this install. Checks both the success path (`perfSummary`) and - * the failure path (`errorDetails.observability.capture`, mutated in place - * before a hard failure throws) — a crash while routed still counts as a - * fired trial. No-ops if the router never actually became eligible for this - * render (e.g. too few frames): the trial stays available for a future run. + * After a trial-armed render, persist that the router's OWN bet actually + * failed — its self-verify/generic-failure safety net fired + * (`deParallelRouter === "reverted"`) — so it's never enabled again for this + * install. A clean "routed" (the render succeeded with no fallback) does + * NOT consume the trial — the whole point is to keep trying on every + * eligible render until we see one real failure signal, maximizing + * successful-routing telemetry volume rather than stopping at the first + * data point. Checks both the success path (`perfSummary`) and the failure + * path (`errorDetails.observability.capture`, mutated in place before a + * hard failure throws) — a render that still failed even after the + * fallback retry counts too. A render that crashed for an unrelated reason + * while merely "routed" (never reached "reverted" — e.g. cancellation) + * does NOT count as a router failure and does not turn the trial off. + * No-ops if the router never became eligible for this render (e.g. too few + * frames): the trial stays available for a future run either way. */ function maybeConsumeDeParallelRouterTrial(trialArmed: boolean, job: RenderJob): void { if (!trialArmed) return; - const engaged = - job.perfSummary?.drawElement?.parallelRouter ?? - job.errorDetails?.observability?.capture.deParallelRouter; - if (engaged === undefined) return; + const failed = + job.perfSummary?.drawElement?.parallelRouter === "reverted" || + job.errorDetails?.observability?.capture.deParallelRouter === "reverted"; + if (!failed) return; const config = readConfig(); config.deParallelRouterTrialFired = true; writeConfig(config); diff --git a/packages/cli/src/telemetry/config.ts b/packages/cli/src/telemetry/config.ts index 4dcab4dae..9dac17146 100644 --- a/packages/cli/src/telemetry/config.ts +++ b/packages/cli/src/telemetry/config.ts @@ -65,12 +65,14 @@ export interface HyperframesConfig { skillsMissingCount?: number; /** * True once the DE parallel-router experiment ("HF_DE_PARALLEL_ROUTER") - * has actually engaged (routed or reverted — either produces telemetry) - * on a render from this install. The CLI enables the experiment for free - * on renders from a fresh install until this fires once, then never - * touches it again — a one-shot trial to get real-traffic router - * telemetry without requiring anyone to manually opt in via env var. - * See `renderLocal`'s `maybeEnableDeParallelRouterTrial`. + * has actually FAILED (its self-verify/generic-failure safety net fired — + * "reverted", not merely "routed") on a render from this install. The CLI + * enables the experiment for free on EVERY eligible render from a fresh + * install — not just once — to maximize real-traffic router telemetry + * (mostly successful "routed" outcomes) without requiring anyone to + * manually opt in via env var; only a real failure turns it off, and only + * for this install going forward. See `renderLocal`'s + * `maybeEnableDeParallelRouterTrial`/`maybeConsumeDeParallelRouterTrial`. */ deParallelRouterTrialFired?: boolean; }