From 532dad7cc78e98654d4552987625b3763fc0486b Mon Sep 17 00:00:00 2001 From: Vance Ingalls Date: Thu, 9 Jul 2026 22:55:29 -0700 Subject: [PATCH] fix(cli): fix batch re-entrancy, config race, exposure cap, and shouldTrack gap in DE trial MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four confirmed findings from a max-effort code review of the CLI trial mechanism: 1. maybeEnableDeParallelRouterTrial's `process.env.HF_DE_PARALLEL_ROUTER !== undefined` guard couldn't distinguish "the user set this" from "an earlier renderLocal() call in this same process already armed it" — so in --batch (all rows share one process), only row 1's outcome could ever reach maybeConsumeDeParallelRouterTrial. A revert on any later row was silently never persisted. Added a module-level deParallelRouterTrialManagedByUs flag to disambiguate, with a test-only reset export since it's process-lifetime state a real CLI invocation never needs to reset but a test suite sharing one module instance does. 2. writeConfig is a non-atomic whole-file overwrite with no locking, and readConfig's cache never invalidates — a concurrently running second CLI process (another terminal, a parallel script; doesn't even need to be a render, any command calls incrementCommandCount) could silently clobber a just-persisted deParallelRouterTrialFired:true with its own stale snapshot. Added readConfigFresh (bypasses the cache) and use it immediately before the trial's read-modify-write, narrowing the race window without a full config-subsystem locking rewrite. 3. The prior commit's semantics flip removed the only exposure cap — a healthy router that never reverts now force-enabled the experimental path on every eligible render forever. Added DE_PARALLEL_ROUTER_TRIAL_MAX_RENDERS (25) as a backstop: the trial turns off after this many engaged renders even absent an actual failure. 4. maybeEnableDeParallelRouterTrial only checked config.telemetryEnabled, not shouldTrack() — so a dev-mode run or a DO_NOT_TRACK/ HYPERFRAMES_NO_TELEMETRY user got the experimental path silently armed while telemetry was simultaneously blocked underneath it. Now gates on shouldTrack() (a strict superset). Also fixed, lower severity: readConfig's deParallelRouterTrialFired/ deParallelRouterTrialRenderCount parsing now validates the JSON type explicitly instead of a bare truthy/nullish read, so a hand-edited or corrupted config can't have the string "false" misread as truthy. Refactored maybeEnableDeParallelRouterTrial into three smaller functions (isDeParallelRouterTrialBlocked, stopManagingDeParallelRouterTrial) to bring cyclomatic/cognitive complexity back under the repo's threshold — also de-duplicates the "stop managing the env var" logic shared with maybeConsumeDeParallelRouterTrial. 14 new/updated tests (43 total in render.test.ts), including a direct regression test for the batch re-entrancy scenario and a loop test for the render-count cap. Verified the config primitives end-to-end against a real file, not just the mocked unit tests. Co-Authored-By: Claude Sonnet 5 --- packages/cli/src/commands/render.test.ts | 133 ++++++++++++++++++++- packages/cli/src/commands/render.ts | 145 +++++++++++++++++------ packages/cli/src/telemetry/config.ts | 30 ++++- 3 files changed, 268 insertions(+), 40 deletions(-) diff --git a/packages/cli/src/commands/render.test.ts b/packages/cli/src/commands/render.test.ts index f6e1e6931..f0a5ff7cc 100644 --- a/packages/cli/src/commands/render.test.ts +++ b/packages/cli/src/commands/render.test.ts @@ -18,6 +18,14 @@ const configState = vi.hoisted(() => ({ writeConfigCalls: [] as Array>, })); +const trackingState = vi.hoisted(() => ({ + // maybeEnableDeParallelRouterTrial gates on the real shouldTrack(), which + // (via isDevMode()) always returns false when this file itself runs as + // `.ts` source under vitest — mocked here so the CLI-trial tests can + // control it directly instead of inheriting that environment quirk. + shouldTrack: true, +})); + const preflightState = vi.hoisted(() => ({ result: { outcomes: [ @@ -59,12 +67,17 @@ vi.mock("../utils/producer.js", () => ({ vi.mock("../telemetry/config.js", () => ({ readConfig: vi.fn(() => ({ ...configState.config })), + readConfigFresh: vi.fn(() => ({ ...configState.config })), writeConfig: vi.fn((config: Record) => { configState.config = { ...config }; configState.writeConfigCalls.push({ ...config }); }), })); +vi.mock("../telemetry/client.js", () => ({ + shouldTrack: vi.fn(() => trackingState.shouldTrack), +})); + vi.mock("../telemetry/events.js", () => ({ trackRenderComplete: vi.fn(), trackRenderError: vi.fn(), @@ -88,9 +101,14 @@ describe("renderLocal browser GPU config", () => { // suites). Importing once in `beforeAll` keeps every test fast and isolated. let renderLocal: typeof import("./render.js").renderLocal; let resolveBrowserGpuForCli: typeof import("./render.js").resolveBrowserGpuForCli; + let resetTrialState: typeof import("./render.js").__resetDeParallelRouterTrialStateForTests; beforeAll(async () => { - ({ renderLocal, resolveBrowserGpuForCli } = await import("./render.js")); + ({ + renderLocal, + resolveBrowserGpuForCli, + __resetDeParallelRouterTrialStateForTests: resetTrialState, + } = await import("./render.js")); }); function setEnv(key: string, value: string) { @@ -104,6 +122,8 @@ describe("renderLocal browser GPU config", () => { producerState.executeImpl = async () => undefined; configState.config = { telemetryEnabled: true, deParallelRouterTrialFired: true }; configState.writeConfigCalls = []; + trackingState.shouldTrack = true; + resetTrialState(); savedEnv.clear(); savedEnv.set("HYPERFRAMES_FFMPEG_PATH", process.env.HYPERFRAMES_FFMPEG_PATH); savedEnv.set("HYPERFRAMES_FFPROBE_PATH", process.env.HYPERFRAMES_FFPROBE_PATH); @@ -441,16 +461,24 @@ describe("renderLocal browser GPU config", () => { describe("renderLocal — DE parallel-router CLI trial", () => { let renderLocal: typeof import("./render.js").renderLocal; + let resetTrialState: typeof import("./render.js").__resetDeParallelRouterTrialStateForTests; const savedEnv = new Map(); beforeAll(async () => { - ({ renderLocal } = await import("./render.js")); + ({ renderLocal, __resetDeParallelRouterTrialStateForTests: resetTrialState } = + await import("./render.js")); }); beforeEach(() => { producerState.createdJobs = []; producerState.executeImpl = async () => undefined; configState.writeConfigCalls = []; + trackingState.shouldTrack = true; + // The "managed by us" flag lives at module scope in render.ts (real CLI + // processes only ever run one --batch sequence, so it never needs + // resetting there) — reset explicitly here so tests don't leak arm/ + // consume state into each other via shared module instance + test order. + resetTrialState(); savedEnv.clear(); savedEnv.set("HF_DE_PARALLEL_ROUTER", process.env.HF_DE_PARALLEL_ROUTER); savedEnv.set("HYPERFRAMES_FFMPEG_PATH", process.env.HYPERFRAMES_FFMPEG_PATH); @@ -499,8 +527,9 @@ describe("renderLocal — DE parallel-router CLI trial", () => { expect(process.env.HF_DE_PARALLEL_ROUTER).toBeUndefined(); }); - it("does not enable the trial when telemetry is disabled", async () => { - configState.config = { telemetryEnabled: false, deParallelRouterTrialFired: false }; + it("does not enable the trial when telemetry isn't actually trackable (shouldTrack() false — dev mode / DO_NOT_TRACK / disabled)", async () => { + configState.config = { telemetryEnabled: true, deParallelRouterTrialFired: false }; + trackingState.shouldTrack = false; await renderLocal("/tmp/project", "/tmp/out.mp4", baseOptions); expect(process.env.HF_DE_PARALLEL_ROUTER).toBeUndefined(); }); @@ -514,7 +543,14 @@ describe("renderLocal — DE parallel-router CLI trial", () => { }; }; await renderLocal("/tmp/project", "/tmp/out.mp4", baseOptions); - expect(configState.writeConfigCalls).toHaveLength(0); + // A write DOES happen — the render-count backstop is tracked on every + // engaged render — but it must not flip deParallelRouterTrialFired. + expect(configState.writeConfigCalls).toContainEqual( + expect.objectContaining({ + deParallelRouterTrialFired: false, + deParallelRouterTrialRenderCount: 1, + }), + ); }); it("persists the trial as fired when the router's own safety net actually reverted", async () => { @@ -549,7 +585,15 @@ describe("renderLocal — DE parallel-router CLI trial", () => { await renderLocal("/tmp/project", "/tmp/out.mp4", { ...baseOptions, throwOnError: true }).catch( () => {}, ); - expect(configState.writeConfigCalls).toHaveLength(0); + // Still counts toward the render-count backstop (the router DID engage), + // but must not flip deParallelRouterTrialFired — the crash wasn't the + // router's own safety net firing. + expect(configState.writeConfigCalls).toContainEqual( + expect.objectContaining({ + deParallelRouterTrialFired: false, + deParallelRouterTrialRenderCount: 1, + }), + ); }); it("persists the trial as fired from the failure path when the router's safety net reverted but the retry still failed", async () => { @@ -565,6 +609,83 @@ describe("renderLocal — DE parallel-router CLI trial", () => { expect.objectContaining({ deParallelRouterTrialFired: true }), ); }); + + 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. + configState.config = { telemetryEnabled: true, deParallelRouterTrialFired: false }; + + producerState.executeImpl = async (job) => { + job.perfSummary = { + resolution: { width: 100, height: 100 }, + drawElement: { parallelRouter: "routed" }, + }; + }; + await renderLocal("/tmp/project", "/tmp/out.mp4", baseOptions); + expect(process.env.HF_DE_PARALLEL_ROUTER).toBe("true"); + expect(configState.config.deParallelRouterTrialFired).toBe(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 }), + ); + expect(process.env.HF_DE_PARALLEL_ROUTER).toBeUndefined(); + }); + + it("does not override an env var the user set between two renders in the same process", async () => { + configState.config = { telemetryEnabled: true, deParallelRouterTrialFired: false }; + producerState.executeImpl = async (job) => { + job.perfSummary = { + resolution: { width: 100, height: 100 }, + drawElement: { parallelRouter: "routed" }, + }; + }; + await renderLocal("/tmp/project", "/tmp/out.mp4", baseOptions); + expect(process.env.HF_DE_PARALLEL_ROUTER).toBe("true"); + + // A real interactive user can't do this mid-batch, but a wrapper script + // invoking the CLI programmatically in the same process could — the + // explicit override must still win on the next call. + process.env.HF_DE_PARALLEL_ROUTER = "false"; + await renderLocal("/tmp/project", "/tmp/out.mp4", baseOptions); + 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 () => { + configState.config = { telemetryEnabled: true, deParallelRouterTrialFired: false }; + producerState.executeImpl = async (job) => { + job.perfSummary = { + resolution: { width: 100, height: 100 }, + drawElement: { parallelRouter: "routed" }, + }; + }; + + for (let i = 0; i < 25; 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(); + }); }); describe("checkRenderResolutionPreflight", () => { diff --git a/packages/cli/src/commands/render.ts b/packages/cli/src/commands/render.ts index 86133395b..80ede962c 100644 --- a/packages/cli/src/commands/render.ts +++ b/packages/cli/src/commands/render.ts @@ -66,7 +66,13 @@ import { trackRenderPreflightRejected, } from "../telemetry/events.js"; import { maybePromptRenderFeedback } from "../telemetry/feedback.js"; -import { readConfig, writeConfig } from "../telemetry/config.js"; +import { + readConfig, + readConfigFresh, + writeConfig, + type HyperframesConfig, +} from "../telemetry/config.js"; +import { shouldTrack } from "../telemetry/client.js"; import { renderJobObservabilityTelemetryPayload } from "../telemetry/renderObservability.js"; import { normalizeSkillSlug } from "../telemetry/skill.js"; import { bytesToMb } from "../telemetry/system.js"; @@ -1610,24 +1616,82 @@ 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; + +/** + * 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). + */ +let deParallelRouterTrialManagedByUs = false; + +/** + * Test-only reset for `deParallelRouterTrialManagedByUs` — a real CLI + * process only ever runs one `--batch` sequence, so this state never needs + * resetting outside a test process where many independent test cases share + * one imported module instance. + */ +// fallow-ignore-next-line unused-export +export function __resetDeParallelRouterTrialStateForTests(): void { + deParallelRouterTrialManagedByUs = false; +} + /** * Enable the DE parallel-router experiment (`HF_DE_PARALLEL_ROUTER`, default - * 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 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 + * 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 if it's already failed (or hit the render cap) for + * this install, or 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 telemetry isn't actually recordable right now (`shouldTrack()` — + * covers dev mode / DO_NOT_TRACK / HYPERFRAMES_NO_TELEMETRY, a strict + * superset of `config.telemetryEnabled` alone; no point risking the * experimental path if we can't even record the resulting signal). */ +/** True once the trial should stop offering itself: already failed, hit the + * render-count backstop, or telemetry isn't actually recordable right now. */ +function isDeParallelRouterTrialBlocked(config: HyperframesConfig): boolean { + const overRenderCap = + (config.deParallelRouterTrialRenderCount ?? 0) >= DE_PARALLEL_ROUTER_TRIAL_MAX_RENDERS; + return Boolean(config.deParallelRouterTrialFired) || overRenderCap || !shouldTrack(); +} + +/** 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 maybeEnableDeParallelRouterTrial(quiet: boolean): boolean { - if (process.env.HF_DE_PARALLEL_ROUTER !== undefined) return false; - const config = readConfig(); - if (config.deParallelRouterTrialFired || !config.telemetryEnabled) return false; + const userSetIt = + process.env.HF_DE_PARALLEL_ROUTER !== undefined && !deParallelRouterTrialManagedByUs; + if (userSetIt) return false; + + if (isDeParallelRouterTrialBlocked(readConfig())) { + stopManagingDeParallelRouterTrial(); + return false; + } + + if (deParallelRouterTrialManagedByUs) return true; + deParallelRouterTrialManagedByUs = true; process.env.HF_DE_PARALLEL_ROUTER = "true"; if (!quiet) { console.log( @@ -1644,28 +1708,43 @@ function maybeEnableDeParallelRouterTrial(quiet: boolean): boolean { /** * 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. + * (`deParallelRouter === "reverted"`) — or that the render-count backstop + * (`DE_PARALLEL_ROUTER_TRIAL_MAX_RENDERS`) was reached, so it's never + * enabled again for this install. A clean "routed" (the render succeeded + * with no fallback) does NOT consume the trial by itself — the whole point + * is to keep trying on every eligible render until we see a real failure + * signal (bounded by the render cap), 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, uncounted. + * + * Re-reads the config fresh from disk immediately before writing (bypassing + * the in-process read cache) rather than reusing whatever was cached at + * `maybeEnableDeParallelRouterTrial` time — narrows, though doesn't + * eliminate, the window for a concurrently-running CLI process (another + * terminal, a parallel script) to clobber this write with its own stale + * snapshot of unrelated config fields (review finding; this repo has no + * cross-process config file locking). */ function maybeConsumeDeParallelRouterTrial(trialArmed: boolean, job: RenderJob): void { if (!trialArmed) return; - const failed = - job.perfSummary?.drawElement?.parallelRouter === "reverted" || - job.errorDetails?.observability?.capture.deParallelRouter === "reverted"; - if (!failed) return; - const config = readConfig(); - config.deParallelRouterTrialFired = true; + const outcome = + job.perfSummary?.drawElement?.parallelRouter ?? + job.errorDetails?.observability?.capture.deParallelRouter; + if (outcome === undefined) return; + const config = readConfigFresh(); + const renderCount = (config.deParallelRouterTrialRenderCount ?? 0) + 1; + config.deParallelRouterTrialRenderCount = renderCount; + if (outcome === "reverted" || renderCount >= DE_PARALLEL_ROUTER_TRIAL_MAX_RENDERS) { + config.deParallelRouterTrialFired = true; + stopManagingDeParallelRouterTrial(); + } writeConfig(config); } diff --git a/packages/cli/src/telemetry/config.ts b/packages/cli/src/telemetry/config.ts index 9dac17146..ad4a7fb8d 100644 --- a/packages/cli/src/telemetry/config.ts +++ b/packages/cli/src/telemetry/config.ts @@ -75,6 +75,12 @@ export interface HyperframesConfig { * `maybeEnableDeParallelRouterTrial`/`maybeConsumeDeParallelRouterTrial`. */ deParallelRouterTrialFired?: boolean; + /** + * Count of engaged (routed or reverted) trial renders so far — the + * backstop that caps exposure even absent an actual failure. See + * `DE_PARALLEL_ROUTER_TRIAL_MAX_RENDERS` in `render.ts`. + */ + deParallelRouterTrialRenderCount?: number; } const DEFAULT_CONFIG: HyperframesConfig = { @@ -120,7 +126,16 @@ export function readConfig(): HyperframesConfig { skillsUpdateAvailable: parsed.skillsUpdateAvailable, skillsOutdatedCount: parsed.skillsOutdatedCount, skillsMissingCount: parsed.skillsMissingCount, - deParallelRouterTrialFired: parsed.deParallelRouterTrialFired, + // Explicit `=== true`/typeof-number checks rather than a truthy/nullish + // read — a hand-edited or corrupted config could plausibly carry a + // non-boolean/non-number JSON value (e.g. the STRING "false", which is + // truthy in JS) for these two fields specifically, since they're read + // with a bare truthy check at the call site (review finding). + deParallelRouterTrialFired: parsed.deParallelRouterTrialFired === true ? true : undefined, + deParallelRouterTrialRenderCount: + typeof parsed.deParallelRouterTrialRenderCount === "number" + ? parsed.deParallelRouterTrialRenderCount + : undefined, }; cachedConfig = config; @@ -133,6 +148,19 @@ export function readConfig(): HyperframesConfig { } } +/** + * Re-read the config from disk, bypassing the in-process cache. Use + * immediately before a targeted single-field read-modify-write (e.g. the DE + * parallel-router trial's render count/fired flag) to narrow — though not + * eliminate, there is no cross-process file locking here — the window for a + * lost update against a concurrently-running CLI process that wrote other + * fields in the meantime. + */ +export function readConfigFresh(): HyperframesConfig { + cachedConfig = null; + return readConfig(); +} + /** * Persist config to disk. Updates the in-memory cache. */