Skip to content

fix(producer): fall back fast when the parallel DE streaming capture stalls#2298

Merged
vanceingalls merged 1 commit into
mainfrom
07-12-fix-de-router-stall-watchdog
Jul 12, 2026
Merged

fix(producer): fall back fast when the parallel DE streaming capture stalls#2298
vanceingalls merged 1 commit into
mainfrom
07-12-fix-de-router-stall-watchdog

Conversation

@vanceingalls

@vanceingalls vanceingalls commented Jul 12, 2026

Copy link
Copy Markdown
Collaborator

Stack

  1. fix(producer): fall back fast when the parallel DE streaming capture stalls #2298 — DE router stall watchdog ← you are here
  2. fix(producer): stop clipping the bottom edge of software-GPU video renders #2300 — video bottom-edge clip
  3. docs(skills): tell agents to file reproducible render bugs, not paraphrases #2301 — feedback bug-report guidance

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:

  • Ticks off executeParallelCapture's progress callback. If no new frame lands within HF_DE_PARALLEL_STALL_MS (default 60 s — well under the 5-min protocol timeout, ≫ the 15–32 ms/frame budget), it fires.
  • On trip: aborts the reorder buffer (so peer workers parked in waitForFrame reject instead of deadlocking the Promise.all pool) and aborts the pool via a separate AbortController linked to the parent abort.
  • The parent abortSignal stays un-aborted, so 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.

Test

  • Watchdog trips on no progress → rethrows a stall error (routes to fallback).
  • A genuine parent-abort is not relabeled as a stall (stays a cancellation).

…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>

vanceingalls commented Jul 12, 2026

Copy link
Copy Markdown
Collaborator Author

@miga-heygen miga-heygen left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Stall trips (short timeout, no progress) → rethrows stall error, parent signal NOT aborted
  2. 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 james-russo-rames-d-jusso left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 through renderOrchestrator.ts:2607's classifier, which maps !isVerifyError && !isMemoryExhaustion to "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 the deFallbackReason union 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 a ponytail:.

Questions

  • 🟡 Manual HF_DE_PARALLEL_STREAM=true opt-in path. shouldRetryViaPinnedFallback (renderOrchestrator.ts:1291) requires deWorkerInversion === "inverted" or deParallelRouter === "routed" for the non-verify, non-cancellation branch to fall back. The auto-router cohort (the ≥24 GB macOS trial target) is fine — it sets deParallelRouter = "routed". But a dev running HF_DE_PARALLEL_STREAM=true manually 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 > lastCapturedFrames guard (which resets lastProgressAt only on strictly-increasing frame counts) doesn't have a direct assertion — worth a third case that ticks progress a couple of times, waits past stallTimeoutMs, 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_MS if data comes back showing false trips.
  • deParallelStreamForced = false reset on retry at renderOrchestrator.ts:2636 — I read it but didn't trace all downstream call sites to confirm no lingering forceParallelStream re-enters the parallel path on the retry attempt.

Review by Rames D Jusso

@miguel-heygen miguel-heygen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Independent current-head review.

Strengths

  • captureStreamingStage.ts:612-638 keeps watchdog aborts separate from the parent cancellation signal and pairs the interval/listener with cleanup.
  • captureStreamingStage.ts:649-695 advances 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 jrusso1020 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

@vanceingalls vanceingalls merged commit 534286a into main Jul 12, 2026
72 checks passed
@vanceingalls vanceingalls deleted the 07-12-fix-de-router-stall-watchdog branch July 12, 2026 18:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants