From 26513984f361811bb2407bbc0fecf5f5ce8ab666 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miguel=20=C3=81ngel?= Date: Fri, 11 Sep 2026 17:06:32 -0400 Subject: [PATCH] fix(producer): non-DE parallel-stream router silently refused on macOS multi-worker renders (#3886) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(producer): read useDrawElement post-clamp for the parallel-stream router The non-DE parallel-streaming router read cfg.useDrawElement before the default-on drawElement clamp ran, so on every macOS/GPU-default host with multi-worker renders it saw a stale true and refused to stream even though the render was about to fall back to non-DE screenshot capture anyway. * refactor: trim fix-narrating comments to durable invariants Adversarial simplify pass flagged two comments that described the diff itself (what used to happen, what changed) instead of the invariant that matters going forward. Kept only the forward-looking reasoning. * fix(producer): capture probe console before closing on the DE clamp path; add regression coverage - Extract the default-on drawElement clamp condition into a named, exported, unit-tested pure predicate (shouldClampDefaultDrawElement), matching this file's existing convention for the sibling router predicate. - Preserve lastBrowserConsole when the clamp closes a drawElement probe session, matching every other probe-close site in this function (an adversarial review caught this: on the auto-worker-calibration path, a render that throws after the clamp would report the calibration browser's console instead of the probe's). - Add tests proving the router requires the clamp's post-clamp output, not its input — the exact ordering bug PRINFRA-689 was about. * test: drop internal link and fix-narrating comment from new regression test A test comment I added carried an internal Slack archive link (this is a public repo) and described the diff instead of the invariant — the user caught it in review after my own simplify pass had already run and missed it, since that pass ran before this test file existed. * refactor: collapse duplicated ordering-invariant comment into one place Two comment blocks explained the same 'why deParallelStreamForced alone is safe here' reasoning, one forward-referencing the other. Stated once. --- .../src/services/renderOrchestrator.test.ts | 89 ++++++++++++ .../src/services/renderOrchestrator.ts | 128 ++++++++++++------ 2 files changed, 178 insertions(+), 39 deletions(-) diff --git a/packages/producer/src/services/renderOrchestrator.test.ts b/packages/producer/src/services/renderOrchestrator.test.ts index 48afd01c29..fcb634f7d5 100644 --- a/packages/producer/src/services/renderOrchestrator.test.ts +++ b/packages/producer/src/services/renderOrchestrator.test.ts @@ -47,6 +47,7 @@ import { mergeWorkerInitObservability, resolveCompositionElementCount, resolveDeShortBand, + shouldClampDefaultDrawElement, shouldPreferParallelDrawElement, shouldPreferSingleWorkerDrawElement, shouldStreamParallelCapture, @@ -2841,6 +2842,94 @@ describe("shouldStreamParallelCapture (non-DE parallel streaming router)", () => }); }); +describe("shouldClampDefaultDrawElement (default-on drawElement clamp)", () => { + const unverifiedParallel = { + useDrawElement: true, + fastCaptureExplicitOptIn: false, + useStreamingEncode: false, + workerCount: 2, + deParallelStreamVerified: false, + }; + + it("clamps default-on drawElement for unverified multi-worker capture", () => { + expect(shouldClampDefaultDrawElement(unverifiedParallel)).toBe(true); + }); + + it("leaves useDrawElement alone once it is already false", () => { + expect(shouldClampDefaultDrawElement({ ...unverifiedParallel, useDrawElement: false })).toBe( + false, + ); + }); + + it("an explicit opt-in overrides the clamp", () => { + expect( + shouldClampDefaultDrawElement({ ...unverifiedParallel, fastCaptureExplicitOptIn: true }), + ).toBe(false); + }); + + it("does not clamp a verified multi-worker streaming render", () => { + expect( + shouldClampDefaultDrawElement({ ...unverifiedParallel, deParallelStreamVerified: true }), + ).toBe(false); + }); + + it("clamps a single-worker render with streaming off (the disk-path case)", () => { + expect(shouldClampDefaultDrawElement({ ...unverifiedParallel, workerCount: 1 })).toBe(true); + }); + + it("leaves a single-worker streaming render unclamped (self-verified by the drain)", () => { + expect( + shouldClampDefaultDrawElement({ + ...unverifiedParallel, + workerCount: 1, + useStreamingEncode: true, + }), + ).toBe(false); + }); +}); + +describe("default-on drawElement clamp feeds the non-DE parallel-stream router", () => { + // The router requires the drawElement clamp's OUTPUT, not its input. Both + // predicates below are individually correct; a caller wiring them together + // in the wrong order still reproduces the bug these two tests bound. + const macOsDefaultOnMultiWorker = { + useDrawElement: true, + fastCaptureExplicitOptIn: false, + useStreamingEncode: false, + workerCount: 2, + deParallelStreamVerified: false, + }; + + it("routes once the clamp's post-clamp value feeds the router", () => { + const clamped = shouldClampDefaultDrawElement(macOsDefaultOnMultiWorker); + const postClampUseDrawElement = clamped ? false : macOsDefaultOnMultiWorker.useDrawElement; + + expect( + shouldStreamParallelCapture({ + routerEnabled: true, + workerCount: macOsDefaultOnMultiWorker.workerCount, + useDrawElement: postClampUseDrawElement, + outputFormat: "mp4", + streamingOk: true, + layeredOrEffectRoute: false, + }), + ).toBe(true); + }); + + it("never routes if the router instead reads the PRE-clamp value (the bug)", () => { + expect( + shouldStreamParallelCapture({ + routerEnabled: true, + workerCount: macOsDefaultOnMultiWorker.workerCount, + useDrawElement: macOsDefaultOnMultiWorker.useDrawElement, + outputFormat: "mp4", + streamingOk: true, + layeredOrEffectRoute: false, + }), + ).toBe(false); + }); +}); + describe("closeOrphanedProbeForRetry (probe cleanup before verify-triggered retry)", () => { // Enough of a CaptureSession stand-in to exercise the closer path — the // helper never inspects the object; it just hands it to the injected closer. diff --git a/packages/producer/src/services/renderOrchestrator.ts b/packages/producer/src/services/renderOrchestrator.ts index ad4abc2d35..2f0319c680 100644 --- a/packages/producer/src/services/renderOrchestrator.ts +++ b/packages/producer/src/services/renderOrchestrator.ts @@ -2008,6 +2008,33 @@ export function shouldStreamParallelCapture(args: { ); } +/** + * Default-on drawElement clamp: is this render's runtime self-verification + * net absent, so `useDrawElement` must fall back to the screenshot/beginframe + * baseline? The disk path and unverified parallel capture ship frames no + * drain verifies — see the call site's comment on why this clamp's own + * outcome is independent of running before or after + * {@link shouldStreamParallelCapture} in the caller. Pure; exported for tests. + */ +export function shouldClampDefaultDrawElement(args: { + useDrawElement: boolean; + /** PRODUCER_EXPERIMENTAL_FAST_CAPTURE === "true" — explicit opt-in always wins. */ + fastCaptureExplicitOptIn: boolean; + useStreamingEncode: boolean; + workerCount: number; + /** (deParallelStreamForced || HF_DE_PARALLEL_STREAM === "true") && + * useStreamingEncode && workerCount > 1 — the multi-worker streaming + * self-verification net is present. */ + deParallelStreamVerified: boolean; +}): boolean { + return ( + args.useDrawElement && + !args.fastCaptureExplicitOptIn && + (!args.useStreamingEncode || args.workerCount > 1) && + !args.deParallelStreamVerified + ); +} + export function resolveCaptureForceScreenshotForPageSideCompositing(args: { forceScreenshot: boolean; usePageSideCompositing: boolean; @@ -3231,9 +3258,66 @@ async function executeRenderPipeline(input: { deParallelRouter: deParallelRouter ?? "none", }); + // Streaming encode pipes captured frames through ffmpeg's stdin to produce + // a single video file. Keep the default enabled for sequential capture, but + // let auto-parallel renders use disk frames: the current ordered streaming + // writer would otherwise stall later workers behind earlier frame ranges. + // png-sequence has no encoded video output, so streaming is always bypassed. + let useStreamingEncode = shouldUseStreamingEncode( + cfg, + outputFormat, + workerCount, + job.duration, + deParallelStreamForced, + ); + // Default-on drawElement is only safe where the runtime self-verification + // net actually runs: the single-worker streaming worker-encode drain, or + // (HF_DE_PARALLEL_STREAM) a verified multi-worker streaming render. The + // disk path and unverified parallel capture ship frames no drain + // verifies, so they clamp to the screenshot baseline unless drawElement + // was explicitly opted into. + // + // This must run before the non-DE parallel-streaming router below, which + // needs `cfg.useDrawElement` post-clamp to know whether this render will + // actually use non-DE capture. The router's own force flag + // (`captureParallelStreamForced`) requires `!useDrawElement`, so it can + // only be true once this clamp has already fired — meaning the router's + // flag is always false while this clamp's guard is still evaluating, + // and `deParallelStreamForced` above is the only flag that can affect it. + const deParallelStreamVerified = + (deParallelStreamForced || process.env.HF_DE_PARALLEL_STREAM === "true") && + useStreamingEncode && + workerCount > 1; + if ( + shouldClampDefaultDrawElement({ + useDrawElement: cfg.useDrawElement, + fastCaptureExplicitOptIn: process.env.PRODUCER_EXPERIMENTAL_FAST_CAPTURE === "true", + useStreamingEncode, + workerCount, + deParallelStreamVerified, + }) + ) { + cfg.useDrawElement = false; + deClampReason = workerCount > 1 ? "parallel" : "disk_path"; + log.info( + "[Render] Fast capture: default-on drawElement disabled for this render — " + + (workerCount > 1 ? "parallel capture" : "the disk capture path") + + " has no runtime self-verification. Set PRODUCER_EXPERIMENTAL_FAST_CAPTURE=true to override.", + ); + // The probe session already initialized in drawElement mode (canvas + // injected); it must not be reused by the unverified path. + if (probeSession && probeSession.captureMode === "drawelement") { + lastBrowserConsole = probeSession.browserConsoleBuffer; + await closeCaptureSession(probeSession); + probeSession = null; + } + } + // Non-DE parallel-streaming router — see shouldStreamParallelCapture. // Mutually exclusive with the DE inversion/router above by construction // (both DE predicates require useDrawElement; this requires its negation). + // Reads `cfg.useDrawElement` after the clamp above, so it sees the + // capture mode this render will actually use. const captureParallelStreamRouterEnabled = process.env.HF_CAPTURE_PARALLEL_STREAM === "true"; const captureParallelStreamArgs = { workerCount, @@ -3282,12 +3366,11 @@ async function executeRenderPipeline(input: { probeSession = null; } - // Streaming encode pipes captured frames through ffmpeg's stdin to produce - // a single video file. Keep the default enabled for sequential capture, but - // let auto-parallel renders use disk frames: the current ordered streaming - // writer would otherwise stall later workers behind earlier frame ranges. - // png-sequence has no encoded video output, so streaming is always bypassed. - let useStreamingEncode = shouldUseStreamingEncode( + // Re-resolve now that the non-DE router above may have forced streaming + // on for this multi-worker render (same formula as the early value above, + // now including `captureParallelStreamForced`). This is the value the + // rest of the pipeline (encode/writer selection, logging) uses. + useStreamingEncode = shouldUseStreamingEncode( cfg, outputFormat, workerCount, @@ -3302,39 +3385,6 @@ async function executeRenderPipeline(input: { durationSeconds: job.duration, maxDurationSeconds: cfg.streamingEncodeMaxDurationSeconds, }); - // Default-on drawElement is only safe where the runtime self-verification - // net actually runs: the single-worker streaming worker-encode drain. The - // disk path (png-sequence / over the streaming duration cap) and parallel - // capture ship frames no drain verifies — route those renders to the - // screenshot baseline unless drawElement was explicitly opted into. - // HF_DE_PARALLEL_STREAM: multi-worker STREAMING renders now carry the - // full drain-time self-verification (per-worker ground truth + the shared - // drain guard), so the confinement rule is satisfied and the parallel - // clamp does not apply. The disk path stays clamped. - const deParallelStreamVerified = - (deParallelStreamForced || process.env.HF_DE_PARALLEL_STREAM === "true") && - useStreamingEncode && - workerCount > 1; - if ( - cfg.useDrawElement && - process.env.PRODUCER_EXPERIMENTAL_FAST_CAPTURE !== "true" && - (!useStreamingEncode || workerCount > 1) && - !deParallelStreamVerified - ) { - cfg.useDrawElement = false; - deClampReason = workerCount > 1 ? "parallel" : "disk_path"; - log.info( - "[Render] Fast capture: default-on drawElement disabled for this render — " + - (workerCount > 1 ? "parallel capture" : "the disk capture path") + - " has no runtime self-verification. Set PRODUCER_EXPERIMENTAL_FAST_CAPTURE=true to override.", - ); - // The probe session already initialized in drawElement mode (canvas - // injected); it must not be reused by the unverified path. - if (probeSession && probeSession.captureMode === "drawelement") { - await closeCaptureSession(probeSession); - probeSession = null; - } - } // png-sequence is "no container" — outputPath is treated as a directory and // the encode/mux/faststart stages are skipped entirely. The empty extension