From a355fb2f6be0b6fb98fe0998e56a13e53773624d Mon Sep 17 00:00:00 2001 From: Vance Ingalls Date: Thu, 9 Jul 2026 20:31:24 -0700 Subject: [PATCH] fix(producer,engine,cli): oom wrapping, cancellation, fallback-reason gaps MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three defects found by max-effort code review of this branch: 1. The Bun OOM exact-match regex was defeated by this codebase's own parallel-worker error wrapping. executeParallelCapture/formatWorkerFailure (parallelCoordinator.ts) always wrap a worker's error as "Worker N: ", optionally suffixed and joined with other workers' segments, all prefixed "[Parallel] Capture failed: ". That wrapping defeated the exact-message check for exactly the cohort (deParallelRouter routed, N separate Chrome processes) the OOM-drops-to-1 fix targets — a real OOM there would retry at the SAME worker count instead of dropping to 1. Added a second pattern that recovers the signal by requiring "out of memory" appear as the WHOLE content of a "Worker N: ..." segment (bounded by end-of-string/"; "), preserving the same exact-match property (no bare substring match) while surviving the wrapping. Verified against the real wrapping logic, not a hand-typed guess at its shape. 2. shouldRetryViaPinnedFallback didn't exclude cancellation, so aborting a render mid-capture on the pinned router/inversion cohort would detour through spawning a fresh encoder/capture session before the outer catch's RenderCancelledError branch ended the render — delaying "stop" with a pointless resource spin-up/tear-down. Added an isCancellation param (checked first, before isVerifyError) using the same `err instanceof RenderCancelledError || abortSignal?.aborted` check the outer catch already uses. 3. deFallbackReason (this PR's new "oom"/"capture_error" values) was set locally but never mirrored into RenderCaptureObservability alongside deSelfVerifyFallback, so a render that fails AFTER a fallback attempt (perfSummary never built) was indistinguishable in render_error telemetry from one that never attempted any fallback — undercutting the "how often does the OOM retry fire on a render that still ultimately fails" question this branch exists to answer. Threaded through RenderCaptureObservability → RenderObservabilityTelemetryPayload → renderObservabilityTelemetryPayload, mirroring the existing deSelfVerifyFallback plumbing. Co-Authored-By: Claude Sonnet 5 --- packages/cli/src/telemetry/events.test.ts | 22 +++++++++++ packages/cli/src/telemetry/events.ts | 2 + .../src/telemetry/renderObservability.test.ts | 16 ++++++++ .../cli/src/telemetry/renderObservability.ts | 1 + .../frameCapture-transientErrors.test.ts | 35 ++++++++++++++++++ packages/engine/src/services/frameCapture.ts | 18 +++++++++ .../src/services/render/observability.ts | 10 +++++ .../src/services/renderOrchestrator.test.ts | 37 +++++++++++++++++++ .../src/services/renderOrchestrator.ts | 20 +++++++++- 9 files changed, 160 insertions(+), 1 deletion(-) diff --git a/packages/cli/src/telemetry/events.test.ts b/packages/cli/src/telemetry/events.test.ts index 91bcc8b5b..0977140fd 100644 --- a/packages/cli/src/telemetry/events.test.ts +++ b/packages/cli/src/telemetry/events.test.ts @@ -75,6 +75,28 @@ describe("render telemetry events", () => { ); }); + it("carries de_fallback_reason on render_error so a render that fails AFTER an OOM-triggered fallback attempt is distinguishable from one that never attempted a fallback", () => { + trackRenderError({ + fps: 30, + quality: "standard", + docker: false, + errorMessage: "worker crashed again after fallback", + captureDeParallelRouter: "reverted", + captureDeSelfVerifyFallback: false, + captureDeFallbackReason: "oom", + }); + + expect(trackEvent).toHaveBeenCalledWith( + "render_error", + expect.objectContaining({ + de_parallel_router: "reverted", + de_self_verify_fallback: false, + de_fallback_reason: "oom", + }), + undefined, + ); + }); + it("prefers the explicit perfSummary-sourced de_worker_inversion over the capture-observability fallback on render_complete", () => { trackRenderComplete({ durationMs: 1000, diff --git a/packages/cli/src/telemetry/events.ts b/packages/cli/src/telemetry/events.ts index 93b3d79f4..0db7d7298 100644 --- a/packages/cli/src/telemetry/events.ts +++ b/packages/cli/src/telemetry/events.ts @@ -46,6 +46,7 @@ export interface RenderObservabilityTelemetryPayload { captureDeParallelRouter?: string; captureDePreRouterWorkers?: number; captureDeSelfVerifyFallback?: boolean; + captureDeFallbackReason?: string; observabilityExtractVideoCount?: number; observabilityExtractedVideoCount?: number; observabilityExtractTotalFrames?: number; @@ -96,6 +97,7 @@ function renderObservabilityEventProperties(props: RenderObservabilityTelemetryP de_parallel_router: props.captureDeParallelRouter, de_pre_router_workers: props.captureDePreRouterWorkers, de_self_verify_fallback: props.captureDeSelfVerifyFallback, + de_fallback_reason: props.captureDeFallbackReason, observability_extract_video_count: props.observabilityExtractVideoCount, observability_extracted_video_count: props.observabilityExtractedVideoCount, observability_extract_total_frames: props.observabilityExtractTotalFrames, diff --git a/packages/cli/src/telemetry/renderObservability.test.ts b/packages/cli/src/telemetry/renderObservability.test.ts index d952ba7cb..de5386945 100644 --- a/packages/cli/src/telemetry/renderObservability.test.ts +++ b/packages/cli/src/telemetry/renderObservability.test.ts @@ -65,4 +65,20 @@ describe("renderObservabilityTelemetryPayload — DE inversion/router cohort (fa ); expect(payload.captureDeSelfVerifyFallback).toBe(true); }); + + it("carries deFallbackReason so a render that fails AFTER an OOM-triggered fallback attempt is distinguishable from one that never attempted a fallback", () => { + const payload = renderObservabilityTelemetryPayload( + makeSummary({ + deParallelRouter: "routed", + deSelfVerifyFallback: false, + deFallbackReason: "oom", + }), + ); + expect(payload.captureDeFallbackReason).toBe("oom"); + }); + + it("leaves deFallbackReason undefined when no fallback was ever attempted", () => { + const payload = renderObservabilityTelemetryPayload(makeSummary({})); + expect(payload.captureDeFallbackReason).toBeUndefined(); + }); }); diff --git a/packages/cli/src/telemetry/renderObservability.ts b/packages/cli/src/telemetry/renderObservability.ts index 9f505d63a..45288ce86 100644 --- a/packages/cli/src/telemetry/renderObservability.ts +++ b/packages/cli/src/telemetry/renderObservability.ts @@ -45,6 +45,7 @@ export function renderObservabilityTelemetryPayload( captureDeParallelRouter: capture.deParallelRouter, captureDePreRouterWorkers: capture.dePreRouterWorkers, captureDeSelfVerifyFallback: capture.deSelfVerifyFallback, + captureDeFallbackReason: capture.deFallbackReason, observabilityExtractVideoCount: extraction?.videoCount, observabilityExtractedVideoCount: extraction?.extractedVideoCount, observabilityExtractTotalFrames: extraction?.totalFramesExtracted, diff --git a/packages/engine/src/services/frameCapture-transientErrors.test.ts b/packages/engine/src/services/frameCapture-transientErrors.test.ts index 97434cf64..82cda5e21 100644 --- a/packages/engine/src/services/frameCapture-transientErrors.test.ts +++ b/packages/engine/src/services/frameCapture-transientErrors.test.ts @@ -109,4 +109,39 @@ describe("isMemoryExhaustionError", () => { false, ); }); + + // The parallel-DE capture path (the exact cohort the OOM-aware retry + // targets) never delivers a bare message — executeParallelCapture / + // formatWorkerFailure (parallelCoordinator.ts) wrap it as + // "[Parallel] Capture failed: Worker N: ", optionally joined with + // other workers' segments and/or suffixed "; diagnostics: ...". Confirmed + // against the real wrapping logic (not a hand-typed guess at its shape). + it("recognizes Bun's OOM message through this codebase's own parallel-worker error wrapping", () => { + expect( + isMemoryExhaustionError(new Error("[Parallel] Capture failed: Worker 2: Out of memory")), + ).toBe(true); + expect( + isMemoryExhaustionError( + new Error("[Parallel] Capture failed: Worker 1: net::ERR_FAILED; Worker 2: Out of memory"), + ), + ).toBe(true); + expect( + isMemoryExhaustionError( + new Error( + "[Parallel] Capture failed: Worker 2: Out of memory; diagnostics: ERROR foo | bar", + ), + ), + ).toBe(true); + }); + + // The wrapped-worker-message pattern must stay as exact-match-per-segment + // as the bare-message one — a worker's own error text merely containing + // "out of memory" (e.g. surfaced WebGL/GPU noise) must not misclassify. + it("does not match 'out of memory' as a mere substring inside a wrapped worker segment", () => { + expect( + isMemoryExhaustionError( + new Error("Worker 2: WebGL context lost, out of memory reported by driver"), + ), + ).toBe(false); + }); }); diff --git a/packages/engine/src/services/frameCapture.ts b/packages/engine/src/services/frameCapture.ts index dd76e6a62..08055386c 100644 --- a/packages/engine/src/services/frameCapture.ts +++ b/packages/engine/src/services/frameCapture.ts @@ -3304,8 +3304,26 @@ const MEMORY_EXHAUSTION_ERROR_PATTERNS = [ // it — a compound message with other text around the phrase still misses. const BUN_MEMORY_EXHAUSTION_EXACT_MESSAGE = /^out of memory\.?$/i; +// The parallel-DE capture path — the exact cohort the OOM-aware retry in +// renderOrchestrator.ts targets — never reaches isMemoryExhaustionError with +// a bare message: `executeParallelCapture`/`formatWorkerFailure` +// (parallelCoordinator.ts) always wrap a worker's error as +// "Worker N: ", optionally suffixed "; diagnostics: ..." and joined +// with other failed workers' segments via "; ", all prefixed +// "[Parallel] Capture failed: ". The exact-match check above is defeated by +// that wrapping entirely (verified) — this pattern recovers the Bun OOM +// signal by requiring "out of memory" appear immediately after "Worker N: " +// and immediately before end-of-string, ";", or ".", i.e. as the WHOLE +// worker-segment content, not merely somewhere inside it. This preserves the +// exact-match property (no bare "out of memory" substring inside otherwise- +// unrelated worker text, e.g. "Worker 2: WebGL context lost, out of memory +// reported by driver" does NOT match) while surviving this codebase's own +// error-flattening. +const BUN_MEMORY_EXHAUSTION_WRAPPED_WORKER_MESSAGE = /\bworker \d+: out of memory\.?(?:;|$)/i; + export function isMemoryExhaustionError(error: unknown): boolean { const message = error instanceof Error ? error.message : String(error); if (BUN_MEMORY_EXHAUSTION_EXACT_MESSAGE.test(message.trim())) return true; + if (BUN_MEMORY_EXHAUSTION_WRAPPED_WORKER_MESSAGE.test(message)) return true; return MEMORY_EXHAUSTION_ERROR_PATTERNS.some((pattern) => pattern.test(message)); } diff --git a/packages/producer/src/services/render/observability.ts b/packages/producer/src/services/render/observability.ts index a1fe82c83..d225d61dd 100644 --- a/packages/producer/src/services/render/observability.ts +++ b/packages/producer/src/services/render/observability.ts @@ -42,6 +42,16 @@ export interface RenderCaptureObservability { browserGpuMode?: string; /** drawElement per-render self-verification tripped → whole render re-ran via screenshot. */ deSelfVerifyFallback?: boolean; + /** + * Why the capture-stage retry (self-verify OR the pinned-worker-count + * fallback) fired: "blank"/"psnr" for a real self-verify trip, + * "oom"/"capture_error" for the widened generic-failure retry. Set + * whenever a fallback is attempted, independent of whether that retry + * itself later succeeds — so a render that fails AFTER a fallback attempt + * (perfSummary never built) is still distinguishable in failure-path + * telemetry from one that never attempted any fallback. + */ + deFallbackReason?: string; /** Auto-parallel inversion outcome: "inverted" (fired, held) | "reverted" (fired, self-verify retry rolled back). */ deWorkerInversion?: "inverted" | "reverted"; /** Worker count the resolver would have used absent the inversion; undefined if it never fired. */ diff --git a/packages/producer/src/services/renderOrchestrator.test.ts b/packages/producer/src/services/renderOrchestrator.test.ts index 962b1173b..bf6de7d51 100644 --- a/packages/producer/src/services/renderOrchestrator.test.ts +++ b/packages/producer/src/services/renderOrchestrator.test.ts @@ -1878,6 +1878,7 @@ describe("shouldRetryViaPinnedFallback (widen the self-verify retry to generic c expect( shouldRetryViaPinnedFallback({ isVerifyError: true, + isCancellation: false, deWorkerInversion: undefined, deParallelRouter: undefined, }), @@ -1888,6 +1889,7 @@ describe("shouldRetryViaPinnedFallback (widen the self-verify retry to generic c expect( shouldRetryViaPinnedFallback({ isVerifyError: false, + isCancellation: false, deWorkerInversion: undefined, deParallelRouter: "routed", }), @@ -1898,6 +1900,7 @@ describe("shouldRetryViaPinnedFallback (widen the self-verify retry to generic c expect( shouldRetryViaPinnedFallback({ isVerifyError: false, + isCancellation: false, deWorkerInversion: "inverted", deParallelRouter: undefined, }), @@ -1908,6 +1911,7 @@ describe("shouldRetryViaPinnedFallback (widen the self-verify retry to generic c expect( shouldRetryViaPinnedFallback({ isVerifyError: false, + isCancellation: false, deWorkerInversion: undefined, deParallelRouter: undefined, }), @@ -1918,6 +1922,7 @@ describe("shouldRetryViaPinnedFallback (widen the self-verify retry to generic c expect( shouldRetryViaPinnedFallback({ isVerifyError: false, + isCancellation: false, deWorkerInversion: undefined, deParallelRouter: "routed", }), @@ -1928,6 +1933,7 @@ describe("shouldRetryViaPinnedFallback (widen the self-verify retry to generic c expect( shouldRetryViaPinnedFallback({ isVerifyError: false, + isCancellation: false, deWorkerInversion: "inverted", deParallelRouter: undefined, }), @@ -1938,9 +1944,40 @@ describe("shouldRetryViaPinnedFallback (widen the self-verify retry to generic c expect( shouldRetryViaPinnedFallback({ isVerifyError: false, + isCancellation: false, deWorkerInversion: "reverted", deParallelRouter: undefined, }), ).toBe(false); }); + + it("never retries a cancellation, even on a pinned cohort — must propagate immediately, not detour through a fresh encoder spin-up", () => { + expect( + shouldRetryViaPinnedFallback({ + isVerifyError: false, + isCancellation: true, + deWorkerInversion: "inverted", + deParallelRouter: undefined, + }), + ).toBe(false); + expect( + shouldRetryViaPinnedFallback({ + isVerifyError: false, + isCancellation: true, + deWorkerInversion: undefined, + deParallelRouter: "routed", + }), + ).toBe(false); + }); + + it("cancellation wins even if the error also looks like a self-verify failure", () => { + expect( + shouldRetryViaPinnedFallback({ + isVerifyError: true, + isCancellation: true, + deWorkerInversion: undefined, + deParallelRouter: undefined, + }), + ).toBe(false); + }); }); diff --git a/packages/producer/src/services/renderOrchestrator.ts b/packages/producer/src/services/renderOrchestrator.ts index 726a36aa8..3f461fa02 100644 --- a/packages/producer/src/services/renderOrchestrator.ts +++ b/packages/producer/src/services/renderOrchestrator.ts @@ -1208,12 +1208,20 @@ export function resolveParallelRouterRetryPlan(args: { * the parallel-SS fallback uses the default pooled browser (one shared * process). Retrying at a possibly-higher worker count is still fewer total * Chrome processes than what just failed. + * + * Excludes cancellation (review): a user-initiated abort must propagate + * immediately, not detour through spawning a fresh encoder/capture session + * before the outer catch's `RenderCancelledError` branch ends the render — + * that would delay honoring "stop" with a pointless resource spin-up/ + * tear-down cycle. */ export function shouldRetryViaPinnedFallback(args: { isVerifyError: boolean; + isCancellation: boolean; deWorkerInversion: "inverted" | "reverted" | undefined; deParallelRouter: "routed" | "reverted" | undefined; }): boolean { + if (args.isCancellation) return false; if (args.isVerifyError) return true; return args.deWorkerInversion === "inverted" || args.deParallelRouter === "routed"; } @@ -2341,7 +2349,16 @@ export async function executeRenderJob( // spawns on retry. See shouldRetryViaPinnedFallback for exactly // which errors qualify. const isVerifyError = isDrawElementVerificationError(err); - if (!shouldRetryViaPinnedFallback({ isVerifyError, deWorkerInversion, deParallelRouter })) + const isCancellation = + err instanceof RenderCancelledError || abortSignal?.aborted === true; + if ( + !shouldRetryViaPinnedFallback({ + isVerifyError, + isCancellation, + deWorkerInversion, + deParallelRouter, + }) + ) throw err; const isMemoryExhaustion = !isVerifyError && isMemoryExhaustionError(err); deSelfVerifyFallback = isVerifyError; @@ -2368,6 +2385,7 @@ export async function executeRenderJob( updateCaptureObservability({ forceScreenshot: true, deSelfVerifyFallback, + deFallbackReason, }); probeSession = null; // Must clear BEFORE resolveParallelRouterRetryPlan recomputes