From b02703b7c9b3f5fb002e2df9ee0320208b333f02 Mon Sep 17 00:00:00 2001 From: Vance Ingalls Date: Fri, 10 Jul 2026 17:24:13 -0700 Subject: [PATCH] fix(cli,producer): opt-in trial polarity + narrowed-fallback-flag docs (review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two non-blocking review notes from Rames, both addressed: 1. Trial polarity inverted to OPT-IN: disableDeParallelRouterTrial → enableDeParallelRouterTrial. renderLocal is exported, so any programmatic consumer (future studio-server path, test harness, distributed runner) previously inherited the trial and its process-wide env-var/module-latch state without knowing to disable it — and concurrent invocation races that state. Now only the CLI's own sequential call sites opt in (the single top-level render, and batch at concurrency 1); everyone else gets no trial by default. The doc comment names the sequential-invocation assumption explicitly. 2. deSelfVerifyFallback semantic narrowing documented at both declarations (RenderCaptureObservability + RenderPerfSummary.drawElement): since the pinned-fallback retry was widened, the flag means verify-triggered SPECIFICALLY — OOM/capture_error fallbacks report false with deFallbackReason carrying the reason. Dashboards keyed on de_self_verify_fallback=true as "any fallback fired" must migrate to de_fallback_reason IS NOT NULL (also called out in the PR body for the observability rebuild to pick up). Co-Authored-By: Claude Opus 4.8 (1M context) --- packages/cli/src/commands/render.test.ts | 21 +++---- packages/cli/src/commands/render.ts | 55 +++++++++++-------- .../src/services/render/observability.ts | 10 +++- .../src/services/renderOrchestrator.ts | 9 ++- 4 files changed, 59 insertions(+), 36 deletions(-) diff --git a/packages/cli/src/commands/render.test.ts b/packages/cli/src/commands/render.test.ts index 28b187451..e80ea824c 100644 --- a/packages/cli/src/commands/render.test.ts +++ b/packages/cli/src/commands/render.test.ts @@ -539,6 +539,9 @@ 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, }; it("enables the trial (sets the env var) on a fresh install with telemetry on", async () => { @@ -749,21 +752,19 @@ 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. + 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 + // (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. configState.disk = { telemetryEnabled: true, deParallelRouterTrialFired: false, telemetryNoticeShown: true, }; - await renderLocal("/tmp/project", "/tmp/out.mp4", { - ...baseOptions, - disableDeParallelRouterTrial: true, - }); + const { enableDeParallelRouterTrial: _omitted, ...programmaticOptions } = baseOptions; + await renderLocal("/tmp/project", "/tmp/out.mp4", programmaticOptions); expect(process.env.HF_DE_PARALLEL_ROUTER).toBeUndefined(); expect(configState.writeConfigCalls).toHaveLength(0); }); diff --git a/packages/cli/src/commands/render.ts b/packages/cli/src/commands/render.ts index f68cd32ab..0d91b2cc1 100644 --- a/packages/cli/src/commands/render.ts +++ b/packages/cli/src/commands/render.ts @@ -901,10 +901,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, + // Sequential batch rows may trial; real concurrent workers + // (batchConcurrency > 1) can't safely share the trial's process-wide + // env var/flags — see enableDeParallelRouterTrial's own doc comment. + enableDeParallelRouterTrial: batchConcurrency <= 1, }; const manifest = await batchModule.runBatchRender({ prepared: preparedBatch, @@ -991,6 +991,9 @@ export default defineCommand({ protocolTimeout, playerReadyTimeout, exitAfterComplete: true, + // The single top-level CLI render is sequential by construction — the + // one place the trial's process-wide state is unconditionally safe. + enableDeParallelRouterTrial: true, }); } }, @@ -1048,18 +1051,21 @@ interface RenderOptions { /** 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. + * OPT IN to the DE parallel-router CLI trial + * (`maybeEnableDeParallelRouterTrial`) 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, + * 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`. */ - disableDeParallelRouterTrial?: boolean; + enableDeParallelRouterTrial?: boolean; } /** @@ -1441,7 +1447,7 @@ export async function renderLocal( const producer = await loadProducer(); const deParallelRouterTrialArmed = maybeEnableDeParallelRouterTrial( options.quiet, - options.disableDeParallelRouterTrial === true, + options.enableDeParallelRouterTrial === true, ); const startTime = Date.now(); @@ -1728,19 +1734,22 @@ function stopManagingDeParallelRouterTrial(): void { * 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 + * 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). */ -function maybeEnableDeParallelRouterTrial(quiet: boolean, disabled: boolean): boolean { - if (disabled) return false; +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 diff --git a/packages/producer/src/services/render/observability.ts b/packages/producer/src/services/render/observability.ts index d225d61dd..43771effb 100644 --- a/packages/producer/src/services/render/observability.ts +++ b/packages/producer/src/services/render/observability.ts @@ -40,7 +40,15 @@ export interface RenderCaptureObservability { usePageSideCompositing?: boolean; hasHdrContent?: boolean; browserGpuMode?: string; - /** drawElement per-render self-verification tripped → whole render re-ran via screenshot. */ + /** + * drawElement per-render SELF-VERIFICATION tripped (blank/PSNR) → whole + * render re-ran via screenshot. NARROWED semantics since the pinned-fallback + * retry was widened (review): OOM- and generic-capture-error-triggered + * fallbacks report FALSE here, with `deFallbackReason` ∈ {oom, + * capture_error}. The "any fallback fired" signal is `deFallbackReason` + * being set, NOT this flag — dashboards keyed on `de_self_verify_fallback = + * true` as any-fallback must migrate to `de_fallback_reason IS NOT NULL`. + */ deSelfVerifyFallback?: boolean; /** * Why the capture-stage retry (self-verify OR the pinned-worker-count diff --git a/packages/producer/src/services/renderOrchestrator.ts b/packages/producer/src/services/renderOrchestrator.ts index 3f461fa02..522e9faa6 100644 --- a/packages/producer/src/services/renderOrchestrator.ts +++ b/packages/producer/src/services/renderOrchestrator.ts @@ -403,9 +403,14 @@ export interface RenderPerfSummary { verifyMinDb?: number; /** Init cost of capturing ground truth (ms). */ verifyInitMs: number; - /** Self-verification tripped and the render re-ran via screenshot. */ + /** + * SELF-VERIFICATION tripped (blank/PSNR) and the render re-ran via + * screenshot. Narrowed since the pinned-fallback retry was widened + * (review): OOM/generic-capture-error fallbacks report FALSE here — + * `fallbackReason` being set is the "any fallback fired" signal. + */ selfVerifyFallback: boolean; - /** What tripped it: psnr | blank. */ + /** What tripped the fallback retry: psnr | blank | oom | capture_error. */ fallbackReason?: string; /** Blank-guard counters. */ blankSuspects: number;