diff --git a/packages/cli/src/commands/render.test.ts b/packages/cli/src/commands/render.test.ts index f0a5ff7cc..59b22283f 100644 --- a/packages/cli/src/commands/render.test.ts +++ b/packages/cli/src/commands/render.test.ts @@ -509,33 +509,79 @@ describe("renderLocal — DE parallel-router CLI trial", () => { }; it("enables the trial (sets the env var) on a fresh install with telemetry on", async () => { - configState.config = { telemetryEnabled: true, deParallelRouterTrialFired: false }; + configState.config = { + telemetryEnabled: true, + deParallelRouterTrialFired: false, + telemetryNoticeShown: true, + }; await renderLocal("/tmp/project", "/tmp/out.mp4", baseOptions); expect(process.env.HF_DE_PARALLEL_ROUTER).toBe("true"); }); it("does not override an env var the user already set themselves", async () => { - configState.config = { telemetryEnabled: true, deParallelRouterTrialFired: false }; + configState.config = { + telemetryEnabled: true, + deParallelRouterTrialFired: false, + telemetryNoticeShown: true, + }; 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("does not enable the trial once it has already fired for this install", async () => { - configState.config = { telemetryEnabled: true, deParallelRouterTrialFired: true }; + configState.config = { + 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 telemetry isn't actually trackable (shouldTrack() false — dev mode / DO_NOT_TRACK / disabled)", async () => { - configState.config = { telemetryEnabled: true, deParallelRouterTrialFired: false }; + it("does not enable the trial when shouldTrack() is false (dev mode / DO_NOT_TRACK)", async () => { + configState.config = { + 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(); }); + 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 () => { + configState.config = { + 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.config = { + telemetryEnabled: true, + deParallelRouterTrialFired: false, + telemetryNoticeShown: false, + }; + await renderLocal("/tmp/project", "/tmp/out.mp4", baseOptions); + expect(process.env.HF_DE_PARALLEL_ROUTER).toBeUndefined(); + }); + 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 }; + configState.config = { + telemetryEnabled: true, + deParallelRouterTrialFired: false, + telemetryNoticeShown: true, + }; producerState.executeImpl = async (job) => { job.perfSummary = { resolution: { width: 100, height: 100 }, @@ -554,7 +600,11 @@ describe("renderLocal — DE parallel-router CLI trial", () => { }); it("persists the trial as fired when the router's own safety net actually reverted", async () => { - configState.config = { telemetryEnabled: true, deParallelRouterTrialFired: false }; + configState.config = { + telemetryEnabled: true, + deParallelRouterTrialFired: false, + telemetryNoticeShown: true, + }; producerState.executeImpl = async (job) => { job.perfSummary = { resolution: { width: 100, height: 100 }, @@ -567,17 +617,33 @@ describe("renderLocal — DE parallel-router CLI trial", () => { ); }); - it("does not persist the trial as fired when the router never became eligible for this render", async () => { - configState.config = { telemetryEnabled: true, deParallelRouterTrialFired: false }; + it("does not persist the trial as fired or increment the render count when the router never became eligible for this render", async () => { + configState.config = { + telemetryEnabled: true, + deParallelRouterTrialFired: false, + telemetryNoticeShown: true, + }; producerState.executeImpl = async (job) => { - job.perfSummary = { resolution: { width: 100, height: 100 }, drawElement: {} }; + // aggregateDrawElement (perfSummary.ts) ALWAYS defaults parallelRouter + // to the string "none" for every render, whether or not drawElement + // ever ran — never undefined. This fixture must match that shape, not + // an unrealistic empty object, or the test doesn't actually exercise + // the "none"-vs-undefined distinction (review finding). + job.perfSummary = { + resolution: { width: 100, height: 100 }, + drawElement: { parallelRouter: "none" }, + }; }; await renderLocal("/tmp/project", "/tmp/out.mp4", baseOptions); expect(configState.writeConfigCalls).toHaveLength(0); }); 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 }; + configState.config = { + telemetryEnabled: true, + deParallelRouterTrialFired: false, + telemetryNoticeShown: true, + }; producerState.executeImpl = async (job) => { job.errorDetails = { observability: { capture: { deParallelRouter: "routed" } } }; throw new Error("render cancelled"); @@ -597,7 +663,11 @@ describe("renderLocal — DE parallel-router CLI trial", () => { }); 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 }; + configState.config = { + telemetryEnabled: true, + deParallelRouterTrialFired: false, + telemetryNoticeShown: true, + }; producerState.executeImpl = async (job) => { job.errorDetails = { observability: { capture: { deParallelRouter: "reverted" } } }; throw new Error("worker crashed even after fallback"); @@ -616,7 +686,11 @@ describe("renderLocal — DE parallel-router CLI trial", () => { // 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 }; + configState.config = { + telemetryEnabled: true, + deParallelRouterTrialFired: false, + telemetryNoticeShown: true, + }; producerState.executeImpl = async (job) => { job.perfSummary = { @@ -642,8 +716,31 @@ describe("renderLocal — DE parallel-router CLI trial", () => { expect(process.env.HF_DE_PARALLEL_ROUTER).toBeUndefined(); }); + it("does not arm the trial when disableDeParallelRouterTrial is set (real batch concurrency, --batch-concurrency N>=2)", async () => { + // Concurrent renderLocal calls share one process-wide env var and one + // module-level flag — safe for sequential --batch rows (every other + // test in this block), not for genuinely concurrent ones (review + // finding). render.ts sets this option to true whenever batchConcurrency + // > 1; verify that gate actually prevents arming. + configState.config = { + telemetryEnabled: true, + deParallelRouterTrialFired: false, + telemetryNoticeShown: true, + }; + await renderLocal("/tmp/project", "/tmp/out.mp4", { + ...baseOptions, + disableDeParallelRouterTrial: true, + }); + expect(process.env.HF_DE_PARALLEL_ROUTER).toBeUndefined(); + expect(configState.writeConfigCalls).toHaveLength(0); + }); + it("does not override an env var the user set between two renders in the same process", async () => { - configState.config = { telemetryEnabled: true, deParallelRouterTrialFired: false }; + configState.config = { + telemetryEnabled: true, + deParallelRouterTrialFired: false, + telemetryNoticeShown: true, + }; producerState.executeImpl = async (job) => { job.perfSummary = { resolution: { width: 100, height: 100 }, @@ -662,7 +759,11 @@ describe("renderLocal — DE parallel-router CLI trial", () => { }); it("caps exposure at DE_PARALLEL_ROUTER_TRIAL_MAX_RENDERS even when the router never reverts", async () => { - configState.config = { telemetryEnabled: true, deParallelRouterTrialFired: false }; + configState.config = { + telemetryEnabled: true, + deParallelRouterTrialFired: false, + telemetryNoticeShown: true, + }; producerState.executeImpl = async (job) => { job.perfSummary = { resolution: { width: 100, height: 100 }, diff --git a/packages/cli/src/commands/render.ts b/packages/cli/src/commands/render.ts index 80ede962c..9e3bf6e9d 100644 --- a/packages/cli/src/commands/render.ts +++ b/packages/cli/src/commands/render.ts @@ -906,6 +906,10 @@ export default defineCommand({ exitAfterComplete: false, throwOnError: true, skipFeedback: true, + // Real concurrent workers (batchConcurrency > 1) can't safely share + // the trial's process-wide env var/flag — see disableDeParallelRouterTrial's + // own doc comment. + disableDeParallelRouterTrial: batchConcurrency > 1, }; const manifest = await batchModule.runBatchRender({ prepared: preparedBatch, @@ -1048,6 +1052,19 @@ interface RenderOptions { throwOnError?: boolean; /** Skip the interactive feedback prompt after a successful render. */ skipFeedback?: boolean; + /** + * Disable the DE parallel-router CLI trial (`maybeEnableDeParallelRouterTrial`) + * for this render. Set by `--batch --batch-concurrency N>=2`: that mechanism + * shares one process-wide env var and one module-level flag across every + * `renderLocal` call in the process, which is safe for SEQUENTIAL calls + * (the ordinary single-worker batch case) but not for genuinely concurrent + * ones — two rows racing on the same global env var/flag could tear down + * or misattribute each other's outcome (review finding). Rather than + * attempt to make shared process-global state safe under real concurrency, + * simply don't offer the trial when it can't be — batch concurrency is an + * explicit opt-in, not the common case. + */ + disableDeParallelRouterTrial?: boolean; } /** @@ -1427,7 +1444,10 @@ export async function renderLocal( } const producer = await loadProducer(); - const deParallelRouterTrialArmed = maybeEnableDeParallelRouterTrial(options.quiet); + const deParallelRouterTrialArmed = maybeEnableDeParallelRouterTrial( + options.quiet, + options.disableDeParallelRouterTrial === true, + ); const startTime = Date.now(); const logger = createRenderTelemetryLogger( @@ -1646,28 +1666,34 @@ export function __resetDeParallelRouterTrialStateForTests(): void { } /** - * 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 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. + * + * 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 — + * `config.telemetryEnabled` is read fresh from `readConfig()` on every call + * here instead, closing that gap (review finding). */ -/** 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(); + return ( + 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 + ); } /** Shared cleanup for both `maybeEnableDeParallelRouterTrial` (this process @@ -1680,7 +1706,27 @@ function stopManagingDeParallelRouterTrial(): void { deParallelRouterTrialManagedByUs = false; } -function maybeEnableDeParallelRouterTrial(quiet: boolean): boolean { +/** + * 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 if `disabled` (set for `--batch-concurrency N>=2`, + * where real concurrent workers can't safely share this process-wide state + * — see `RenderOptions.disableDeParallelRouterTrial`), 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). + */ +function maybeEnableDeParallelRouterTrial(quiet: boolean, disabled: boolean): boolean { + if (disabled) return false; const userSetIt = process.env.HF_DE_PARALLEL_ROUTER !== undefined && !deParallelRouterTrialManagedByUs; if (userSetIt) return false; @@ -1725,27 +1771,59 @@ function maybeEnableDeParallelRouterTrial(quiet: boolean): boolean { * 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). + * the in-process read cache) and verifies the write actually stuck against + * another fresh read, retrying against whatever a concurrent writer left + * behind if not (up to a few attempts) — this repo has no real cross-process + * file locking, so it's still possible for two truly simultaneous writers to + * race past each other, but it closes the common case where a concurrently- + * running CLI process (another terminal, a parallel script) would otherwise + * silently revert this write with its own stale snapshot (review finding). */ -function maybeConsumeDeParallelRouterTrial(trialArmed: boolean, job: RenderJob): void { - if (!trialArmed) return; - const outcome = - job.perfSummary?.drawElement?.parallelRouter ?? - job.errorDetails?.observability?.capture.deParallelRouter; - if (outcome === undefined) return; - const config = readConfigFresh(); +/** + * Apply this render's outcome to a fresh config snapshot: increment the + * render-count backstop, and flip `deParallelRouterTrialFired` (+ stop + * managing the env var) if the router's own safety net fired or the cap was + * reached. Pure mutation, no I/O — kept separate from the retry loop below + * so each stays simple enough for the repo's own complexity gate. + */ +function applyDeParallelRouterOutcome(config: HyperframesConfig, outcome: string): void { 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); +} + +function maybeConsumeDeParallelRouterTrial(trialArmed: boolean, job: RenderJob): void { + if (!trialArmed) return; + const outcome = + job.perfSummary?.drawElement?.parallelRouter ?? + job.errorDetails?.observability?.capture.deParallelRouter; + // perfSummary.drawElement.parallelRouter is NEVER undefined on the success + // path — aggregateDrawElement (perfSummary.ts) defaults it to the string + // "none" for every render, whether or not drawElement/the router ever + // engaged. Treating "none" the same as undefined here is required, not + // optional: without it, ordinary renders below the router's own frame + // threshold (the common case) would tick the render-count backstop on + // every single render and trip DE_PARALLEL_ROUTER_TRIAL_MAX_RENDERS after + // 25 completely unrelated renders that never touched the router at all + // (review finding). + if (outcome === undefined || outcome === "none") return; + + const MAX_WRITE_ATTEMPTS = 3; + for (let attempt = 0; attempt < MAX_WRITE_ATTEMPTS; attempt++) { + const config = readConfigFresh(); + applyDeParallelRouterOutcome(config, outcome); + writeConfig(config); + const verify = readConfigFresh(); + const stuck = + verify.deParallelRouterTrialRenderCount === config.deParallelRouterTrialRenderCount && + verify.deParallelRouterTrialFired === config.deParallelRouterTrialFired; + if (stuck) return; + // A concurrent writer landed between our write and this verify read — + // retry against whatever they left, re-applying our own mutation on top. + } } function handleRenderError( diff --git a/packages/cli/src/telemetry/config.test.ts b/packages/cli/src/telemetry/config.test.ts new file mode 100644 index 000000000..7ee9670be --- /dev/null +++ b/packages/cli/src/telemetry/config.test.ts @@ -0,0 +1,96 @@ +import { beforeEach, describe, expect, it, vi } from "vitest"; + +// In-memory fake filesystem so these tests exercise the REAL config.ts +// module (parsing, caching, readConfigFresh's cache-bypass) without ever +// touching the developer/CI machine's actual ~/.hyperframes/config.json — +// homedir() is resolved once at config.ts's module-load time, so faking +// HOME via env var would only work in a fresh process, not inside a shared +// vitest worker. +const fsState = vi.hoisted(() => ({ + files: new Map(), +})); + +vi.mock("node:fs", () => ({ + existsSync: vi.fn((path: string) => fsState.files.has(path)), + mkdirSync: vi.fn(() => undefined), + readFileSync: vi.fn((path: string) => { + const content = fsState.files.get(path); + if (content === undefined) throw new Error(`ENOENT: ${path}`); + return content; + }), + writeFileSync: vi.fn((path: string, content: string) => { + fsState.files.set(path, content); + }), +})); + +describe("config.ts — readConfig / readConfigFresh / writeConfig (real module, faked fs)", () => { + let readConfig: typeof import("./config.js").readConfig; + let readConfigFresh: typeof import("./config.js").readConfigFresh; + let writeConfig: typeof import("./config.js").writeConfig; + let CONFIG_PATH: typeof import("./config.js").CONFIG_PATH; + + beforeEach(async () => { + fsState.files.clear(); + // Fresh module instance per test — config.ts's `cachedConfig` is + // module-scoped, so without this, a later test would silently inherit + // an earlier test's cached read. + vi.resetModules(); + ({ readConfig, readConfigFresh, writeConfig, CONFIG_PATH } = await import("./config.js")); + }); + + it("creates a default config with a fresh anonymousId when no file exists", () => { + const config = readConfig(); + expect(config.telemetryEnabled).toBe(true); + expect(config.anonymousId).toBeTruthy(); + expect(fsState.files.has(CONFIG_PATH)).toBe(true); + }); + + it("caches the read — a second readConfig() call does not see a file mutated out from under it", () => { + const first = readConfig(); + fsState.files.set(CONFIG_PATH, JSON.stringify({ ...first, deParallelRouterTrialFired: true })); + const second = readConfig(); + expect(second.deParallelRouterTrialFired).toBeUndefined(); + }); + + it("readConfigFresh bypasses the cache and picks up a file written by another process", () => { + const first = readConfig(); + fsState.files.set(CONFIG_PATH, JSON.stringify({ ...first, deParallelRouterTrialFired: true })); + const fresh = readConfigFresh(); + expect(fresh.deParallelRouterTrialFired).toBe(true); + }); + + it("writeConfig updates the in-process cache so a subsequent readConfig() sees the write immediately", () => { + const config = readConfig(); + config.deParallelRouterTrialRenderCount = 5; + writeConfig(config); + const reread = readConfig(); + expect(reread.deParallelRouterTrialRenderCount).toBe(5); + }); + + it('treats a non-boolean deParallelRouterTrialFired (e.g. the JSON string "false") as unset, not truthy', () => { + const base = readConfig(); + fsState.files.set( + CONFIG_PATH, + JSON.stringify({ ...base, deParallelRouterTrialFired: "false" }), + ); + const fresh = readConfigFresh(); + expect(fresh.deParallelRouterTrialFired).toBeUndefined(); + }); + + it("treats a non-number deParallelRouterTrialRenderCount as unset", () => { + const base = readConfig(); + fsState.files.set( + CONFIG_PATH, + JSON.stringify({ ...base, deParallelRouterTrialRenderCount: "5" }), + ); + const fresh = readConfigFresh(); + expect(fresh.deParallelRouterTrialRenderCount).toBeUndefined(); + }); + + it("resets to defaults with a fresh anonymousId when the file is corrupted JSON", () => { + fsState.files.set(CONFIG_PATH, "{not valid json"); + const config = readConfig(); + expect(config.telemetryEnabled).toBe(true); + expect(config.anonymousId).toBeTruthy(); + }); +});