fix(producer): fall back fast when the parallel DE streaming capture stalls#2298
Conversation
…stalls The DE parallel router auto-enables the interleaved parallel-streaming capture for the >=24GB macOS trial cohort. If a worker wedges mid-capture (a hung seek/screenshot at an early frame) the render made no frame progress yet sat until the per-frame CDP protocolTimeout (~5 min) fired before the pinned self-verify fallback could run — a silent multi-minute hang shipped to real users (report: stuck at frame 2/2031 for 6+ min, no fallback, until HF_DE_PARALLEL_ROUTER=false). Add a no-frame-progress watchdog to the parallel branch of runCaptureStreamingStage. It ticks off executeParallelCapture's progress callback; if no NEW frame lands within HF_DE_PARALLEL_STALL_MS (default 60s, well under protocolTimeout and >> the 15-32ms/frame budget), it aborts the reorder buffer (unsticking peer workers parked in waitForFrame so the pool doesn't deadlock) and aborts the pool via a SEPARATE controller linked to the parent abort. Because the parent abortSignal stays un-aborted, the orchestrator reads the failure as a generic capture_error (not a cancellation) and re-renders on the pinned screenshot path — the same fallback a verify failure already uses. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This stack of pull requests is managed by Graphite. Learn more about stacking. |
miga-heygen
left a comment
There was a problem hiding this comment.
Review — fix(producer): fall back fast when the parallel DE streaming capture stalls
SSOT check: clean. Stall timeout is canonical in resolveParallelStallTimeoutMs(), env-overridable via HF_DE_PARALLEL_STALL_MS, default 60s.
The separate AbortController design is correct and important. The orchestrator gates its pinned screenshot fallback on abortSignal.aborted — if the watchdog used the same signal, a stall would look like a cancellation and the fallback would be skipped. The stallController linked to the parent via forwardParentAbort keeps the two abort domains distinct:
- Parent aborts → both controllers abort → orchestrator sees cancellation (no fallback)
- Watchdog trips → only stallController aborts → parent signal stays clean → orchestrator sees capture_error → screenshot fallback fires
Race safety: if parent aborts just after the stall fires, stalled=true but abortSignal.aborted=true too — the if (stalled && abortSignal?.aborted !== true) check correctly lets the parent-abort error propagate instead of relabeling it as a stall.
Reorder buffer abort: reorderBuffer.abort(stallErr) unblocks peer workers parked in waitForFrame so the Promise.all pool drains instead of deadlocking. The mock exercises this contract.
Cleanup: finally block clears the interval and removes the listener. No leaks.
Tests: two tests cover the critical branches:
- Stall trips (short timeout, no progress) → rethrows stall error, parent signal NOT aborted
- Parent abort (huge timeout, no stall) → error does NOT contain "stalled"
Both tests properly restore process.env.HF_DE_PARALLEL_STALL_MS in finally blocks.
Default threshold: 60s is well above any real per-frame budget (15-32ms) and well below the 5-min protocol timeout — a false trip only costs the (slower, never-wrong) screenshot fallback. Good tradeoff.
LGTM — no issues found.
— Miga
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Reviewed at 07fb35c3.
Nice fix. The design decisions all land right: separate AbortController linked to the parent (so a watchdog trip stays distinguishable from a real cancellation), reorder-buffer abort so peers parked in waitForFrame unstick, stalled && abortSignal?.aborted !== true gate before relabeling (parent-abort wins the tie), progress-only counter advance (spurious 0→0 pings don't reset the clock), and finally-block cleanup on both the interval and the parent-abort listener. FrameReorderBuffer.abort is idempotent (if (aborted) return) so the double-call path (drain-error + watchdog) is safe. Classification per feedback_race_fix_preempt_vs_narrow is a clean PREEMPT — new early exit that was previously waiting on the 5-min protocolTimeout. And per feedback_illusory_defense_in_depth_same_source, this is genuine layered defense-in-depth vs #2249 (RAM-floor withhold sources from host config; this sources from runtime progress — different signals).
Concerns
- 🟠 Observability blind spot in
deFallbackReason. The stall error routes throughrenderOrchestrator.ts:2607's classifier, which maps!isVerifyError && !isMemoryExhaustionto"capture_error". Post-fix ops sees "wait, we're falling back to screenshot at X% rate" but can't distinguish a stall-abort from any other generic capture-side failure without grepping error strings. Trivial to extend: add"stall"(or"parallel_stall") to thedeFallbackReasonunion and pattern-match on the/stalled/substring in the classifier. The whole point of this fix is that the stall was silent — losing the signal after the fix would leave ops in the same "we didn't know it was happening" spot for the rate, just not the individual render. Fine to defer if you'd rather ship this first, but worth aponytail:.
Questions
- 🟡 Manual
HF_DE_PARALLEL_STREAM=trueopt-in path.shouldRetryViaPinnedFallback(renderOrchestrator.ts:1291) requiresdeWorkerInversion === "inverted"ordeParallelRouter === "routed"for the non-verify, non-cancellation branch to fall back. The auto-router cohort (the ≥24 GB macOS trial target) is fine — it setsdeParallelRouter = "routed". But a dev runningHF_DE_PARALLEL_STREAM=truemanually gets the watchdog trip but not the pinned fallback — the stall bubbles as an unhandled render failure. Intentional (dev-only opt-in, not a shipped surface) or worth broadening the fallback gate?
Nits
- 🟡 Test coverage is two cases: stall trip → non-cancellation error, and genuine parent-abort → not relabeled. The
progress.capturedFrames > lastCapturedFramesguard (which resetslastProgressAtonly on strictly-increasing frame counts) doesn't have a direct assertion — worth a third case that ticks progress a couple of times, waits paststallTimeoutMs, and asserts no trip fires. Ratchet-style unit for the timer.
What I didn't verify
- Real-hardware behavior: whether 60s default is comfortably above worst-case legitimate per-frame budget on the ≥24 GB macOS trial cohort under heavy comp shapes (bimodal frame costs, sub-composition-video, etc.). The 15–32 ms budget claimed in the comment is a comfortable margin for the fast case; the tail is what could false-trip. Fine to tune the default later via
HF_DE_PARALLEL_STALL_MSif data comes back showing false trips. deParallelStreamForced = falsereset on retry atrenderOrchestrator.ts:2636— I read it but didn't trace all downstream call sites to confirm no lingeringforceParallelStreamre-enters the parallel path on the retry attempt.
miguel-heygen
left a comment
There was a problem hiding this comment.
Independent current-head review.
Strengths
captureStreamingStage.ts:612-638keeps watchdog aborts separate from the parent cancellation signal and pairs the interval/listener with cleanup.captureStreamingStage.ts:649-695advances the watchdog only on real frame progress, aborts the reorder buffer to release waiting workers, and preserves parent-abort semantics.- Focused stall/parent-abort suite passed 6/6 locally; all GitHub checks are terminal and green, including Windows tests/render. Branch is fresh against
main.
No material blockers found.
Verdict: APPROVE
Reasoning: The watchdog preserves cancellation versus fallback semantics, cleans up acquired resources, and has direct regression coverage for both branches.
— deepwork
jrusso1020
left a comment
There was a problem hiding this comment.
Approving on Rames-D's go. Rames-D reviewed + Magi APPROVED at head 07fb35c, CI green, no changes-requested. (Rames-D's observability nit re the capture_error bucket is non-blocking.)

Stack
Problem
The DE parallel router auto-enables the interleaved parallel-streaming capture for the ≥24 GB macOS trial cohort. If a worker wedges mid-capture (a hung seek/screenshot at an early frame), the render makes zero frame progress yet sits until the per-frame CDP
protocolTimeout(~5 min) fires before the pinned self-verify fallback can run — a silent multi-minute hang shipped to real users.Reported: stuck at frame 2/2031 for 6+ min, no fallback, until the user manually set
HF_DE_PARALLEL_ROUTER=false(71 s clean).Fix
Add a no-frame-progress watchdog to the parallel branch of
runCaptureStreamingStage:executeParallelCapture's progress callback. If no new frame lands withinHF_DE_PARALLEL_STALL_MS(default 60 s — well under the 5-min protocol timeout, ≫ the 15–32 ms/frame budget), it fires.waitForFramereject instead of deadlocking thePromise.allpool) and aborts the pool via a separateAbortControllerlinked to the parent abort.abortSignalstays un-aborted, so the orchestrator reads the failure as a genericcapture_error(not a cancellation) and re-renders on the pinned screenshot path — the same fallback a verify failure already uses.Test