fix(cli,producer): opt-in trial polarity + narrowed-fallback-flag docs (review)

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) <noreply@anthropic.com>

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);
   });

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

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

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;