From 51c75f08e32f946e01289c2b367f334587269fbe Mon Sep 17 00:00:00 2001 From: miga-heygen Date: Sat, 12 Sep 2026 01:01:20 +0000 Subject: [PATCH] fix(producer): retry capture on a CDP Page.captureScreenshot refusal MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Chromium's CDP layer can refuse a screenshot capture outright ("Protocol error (Page.captureScreenshot): Unable to capture screenshot") with no timeout wording at all. classifyCaptureFailure() didn't recognize the message, so it fell through to the fatal "authoring" bucket and neither capture-retry path (the disk-capture retry loop, or the streaming-capture pinned-fallback retry) ever attempted a retry — including under --low-memory-mode, whose single-worker plain-screenshot capture never engages either path's other retry conditions. Adds the literal error text to classifyCaptureFailure()'s transient_browser bucket, and threads the resulting classification into shouldRetryViaPinnedFallback() as a routing-independent retry condition, so the streaming path recovers on a fresh session the same way the disk path already does for other transient browser failures. Co-Authored-By: Miguel Angel --- .../src/services/captureFailure.test.ts | 7 +++ .../engine/src/services/captureFailure.ts | 5 ++ .../src/services/renderOrchestrator.test.ts | 52 +++++++++++++++++-- .../src/services/renderOrchestrator.ts | 12 +++++ 4 files changed, 71 insertions(+), 5 deletions(-) diff --git a/packages/engine/src/services/captureFailure.test.ts b/packages/engine/src/services/captureFailure.test.ts index 264cbef493..b326374dcd 100644 --- a/packages/engine/src/services/captureFailure.test.ts +++ b/packages/engine/src/services/captureFailure.test.ts @@ -17,6 +17,13 @@ describe("classifyCaptureFailure", () => { ["JavaScript heap out of memory", "memory_exhaustion"], ["drawElement self-verify failed", "verification"], ["Composition has zero duration. Runtime ready: true", "authoring"], + ["Protocol error (Page.captureScreenshot): Unable to capture screenshot", "transient_browser"], + [ + "[Parallel] Capture failed: Worker 0: Protocol error (Page.captureScreenshot): Unable to capture screenshot", + "transient_browser", + ], + // The timed-out variant of the same call stays protocol_timeout (checked first). + ["Protocol error (Page.captureScreenshot): waiting for debugger timed out", "protocol_timeout"], ] as const)("classifies %s as %s", (message, kind) => { expect(classifyCaptureFailure(new Error(message)).kind).toBe(kind); }); diff --git a/packages/engine/src/services/captureFailure.ts b/packages/engine/src/services/captureFailure.ts index 99951d5188..c47153b215 100644 --- a/packages/engine/src/services/captureFailure.ts +++ b/packages/engine/src/services/captureFailure.ts @@ -52,6 +52,11 @@ const TRANSIENT_BROWSER_ERROR_PATTERNS = [ /ECONNREFUSED/i, /net::ERR_NETWORK_CHANGED/i, /Composition has zero duration[\s\S]*Runtime ready: false/, + // CDP can refuse a capture call outright with this exact wording; timed-out + // variants of the same call hit PROTOCOL_TIMEOUT_PATTERNS, checked first. + // Anchored to this literal reason (not just the CDP method) so an unrelated, + // genuinely deterministic Page.captureScreenshot error isn't swept in too. + /Protocol error \(Page\.captureScreenshot\): Unable to capture screenshot/i, ]; const PROTOCOL_TIMEOUT_PATTERNS = [ diff --git a/packages/producer/src/services/renderOrchestrator.test.ts b/packages/producer/src/services/renderOrchestrator.test.ts index fcb634f7d5..b2e2252e91 100644 --- a/packages/producer/src/services/renderOrchestrator.test.ts +++ b/packages/producer/src/services/renderOrchestrator.test.ts @@ -302,18 +302,27 @@ describe("executeDiskCaptureWithAdaptiveRetry — transient Target-closed single vi.mocked(mergeWorkerFrames).mockReset(); }); - it("retries ONCE at the same worker count on a transient Target closed with zero progress", async () => { + // Both shapes classify as transient_browser: the tab dying mid-capture, and a + // CDP refusal of the capture call itself (no timeout wording, so it used to + // fall through to the fatal "authoring" bucket and was never retried). + it.each([ + ["a transient Target closed", "Protocol error (Page.captureScreenshot): Target closed"], + [ + "a non-timeout captureScreenshot protocol refusal", + "[Parallel] Capture failed: Worker 0: Protocol error (Page.captureScreenshot): Unable to capture screenshot", + ], + ])("retries ONCE at the same worker count on %s with zero progress", async (_label, message) => { const workDir = mkdtempSync(join(tmpdir(), "hf-transient-work-")); const framesDir = mkdtempSync(join(tmpdir(), "hf-transient-frames-")); const log = makeLog(); let call = 0; - // First attempt: the tab dies before any frame is captured (frame 0) — zero - // forward progress, which the worker-halving retry deliberately bails on. - // The transient retry recovers it without changing the worker count. + // First attempt fails before any frame is captured (frame 0) — zero forward + // progress, which the worker-halving retry deliberately bails on. The + // transient retry recovers it without changing the worker count. vi.mocked(executeParallelCapture).mockImplementation(async () => { call++; if (call === 1) { - throw new Error("Protocol error (Page.captureScreenshot): Target closed"); + throw new Error(message); } writeAllFrames(framesDir, 4); return []; @@ -1629,6 +1638,13 @@ describe("adaptive missing-frame retry helpers", () => { ), ), ).toBe(true); + expect( + isRecoverableParallelCaptureError( + new Error( + "[Parallel] Capture failed: Worker 0: Protocol error (Page.captureScreenshot): Unable to capture screenshot", + ), + ), + ).toBe(true); expect(isRecoverableParallelCaptureError(new Error("Encoding failed: ffmpeg exited"))).toBe( false, ); @@ -2748,6 +2764,32 @@ describe("shouldRetryViaPinnedFallback (widen the self-verify retry to generic c }), ).toBe(false); }); + + // --low-memory-mode is single-worker with no drawElement, so nothing ever + // pins a count and that mode had no whole-render fallback at all. + it("retries a transient capture-call refusal even with no pinned routing", () => { + expect( + shouldRetryViaPinnedFallback({ + isVerifyError: false, + isCancellation: false, + deWorkerInversion: undefined, + deParallelRouter: undefined, + isTransientCaptureError: true, + }), + ).toBe(true); + }); + + it("never retries a transient capture-call refusal after cancellation", () => { + expect( + shouldRetryViaPinnedFallback({ + isVerifyError: false, + isCancellation: true, + deWorkerInversion: undefined, + deParallelRouter: undefined, + isTransientCaptureError: true, + }), + ).toBe(false); + }); }); describe("sequential capture stall recovery", () => { diff --git a/packages/producer/src/services/renderOrchestrator.ts b/packages/producer/src/services/renderOrchestrator.ts index 2f0319c680..fc0fe73f26 100644 --- a/packages/producer/src/services/renderOrchestrator.ts +++ b/packages/producer/src/services/renderOrchestrator.ts @@ -87,6 +87,7 @@ import { classifyCaptureFailure, cloneCaptureWarning, isMemoryExhaustionError, + isTransientBrowserError, isDrawElementVerificationError, isDrawElementCaptureError, getDrawElementVerificationDetails, @@ -1902,10 +1903,20 @@ export function shouldRetryViaPinnedFallback(args: { isDeRendererStall?: boolean; /** The producer's no-progress watchdog tripped around a sequential capture call. */ isSequentialCaptureStall?: boolean; + /** + * A transient browser failure around the capture call itself + * (`classifyCaptureFailure` → `transient_browser`, e.g. a CDP + * `Page.captureScreenshot` refusal). Routing-independent like the stalls + * above: `--low-memory-mode` pins single-worker screenshot capture with no + * drawElement, so neither inversion nor the router ever pins a count, and + * that mode otherwise had no whole-render fallback for a one-off refusal. + */ + isTransientCaptureError?: boolean; }): boolean { if (args.isCancellation || args.isEncoderInterrupted) return false; if (args.isVerifyError || args.isDeCaptureError) return true; if (args.isDeRendererStall === true || args.isSequentialCaptureStall === true) return true; + if (args.isTransientCaptureError === true) return true; return args.deWorkerInversion === "inverted" || args.deParallelRouter === "routed"; } @@ -3741,6 +3752,7 @@ async function executeRenderPipeline(input: { deParallelRouter, isDeRendererStall: isDeStall, isSequentialCaptureStall: isSequentialStall, + isTransientCaptureError: isTransientBrowserError(err), }) ) throw err;