Skip to content

fix(producer): trip DE parallel-router circuit breaker on stalls and hangs - #3479

Open
miga-heygen wants to merge 1 commit into
mainfrom
fix/de-parallel-router-hang-breaker
Open

fix(producer): trip DE parallel-router circuit breaker on stalls and hangs#3479
miga-heygen wants to merge 1 commit into
mainfrom
fix/de-parallel-router-hang-breaker

Conversation

@miga-heygen

Copy link
Copy Markdown
Contributor

Summary

  • Root cause: the per-worker capture calls in captureFrameRange (parallelCoordinator.ts) take no abort signal of their own and only checked signal.aborted before starting each frame — a no-op once a worker is already awaiting an in-flight call. On WSL2, the native drawElement/BeginFrame capture call can hang indefinitely at frame 0 with no error. The DE parallel-router's existing stall watchdog (captureStreamingStage.ts) does fire stallController.abort() after HF_DE_STALL_MS, but that abort had no way to reach a worker already wedged inside a hung capture call — so executeParallelCapture's Promise.all waited forever, the render hung indefinitely, and the CLI's circuit breaker (which only runs after executeRenderJob settles) never got a chance to trip.
  • Fix: race each per-frame capture call against the signal actually firing (raceAgainstAbort), the same "can't cancel, only race" pattern already used by the sequential capture path. Once the watchdog's abort is observed, the wedged worker rejects, executeParallelCapture settles, and the existing pinned-fallback retry / "reverted" outcome / circuit-breaker machinery (already correct) runs end to end.
  • Also widen the CLI breaker's trip condition (maybeConsumeDeParallelRouterTrial) from the literal string "reverted" to "not a clean routed success", so any future non-success outcome the observability layer records also latches the breaker instead of silently falling through.
  • Added a regression test (parallelCoordinator-peerAbort.test.ts) that simulates a worker wedged inside a capture call that never settles; confirmed it hangs/times out without the fix and passes with it.

Closes #3441

Test plan

  • vitest run packages/engine/src/services/parallelCoordinator.test.ts packages/engine/src/services/parallelCoordinator-peerAbort.test.ts — 51 passed, including new stall regression test
  • vitest run packages/cli/src/commands/render.test.ts — 73 passed (all DE parallel-router breaker tests)
  • vitest run packages/producer/src/services/renderOrchestrator.test.ts — 195 passed
  • bun test packages/producer/src/services/render/stages/captureStreamingStage.test.ts — 11 passed
  • tsc --noEmit clean on packages/engine and packages/cli
  • Verified the new test times out (reproducing the hang) when the fix is reverted, and passes with it applied

Co-Authored-By: Miga noreply@anthropic.com

…hangs

Root cause: the per-worker capture calls in captureFrameRange
(parallelCoordinator.ts) take no abort signal of their own, and only
checked `signal.aborted` BEFORE starting each frame — a no-op once a
worker is already awaiting an in-flight call. On WSL2, the native
drawElement/BeginFrame capture call can hang indefinitely at frame 0
with no error. The DE parallel-router's existing stall watchdog
(captureStreamingStage.ts) correctly fires `stallController.abort()`
after HF_DE_STALL_MS, but that abort had no way to reach a
worker already wedged inside a hung capture call — so
executeParallelCapture's Promise.all waited forever, the render hung
indefinitely, and the CLI's circuit breaker (which only runs after
executeRenderJob settles) never got a chance to trip.

Fix: race each per-frame capture call against the signal actually
firing (raceAgainstAbort), the same "can't cancel, only race" pattern
already used by the sequential capture path. Once the watchdog's abort
is observed, the wedged worker rejects, executeParallelCapture settles,
and the existing pinned-fallback retry / "reverted" outcome / circuit
breaker machinery (already correct) runs end to end.

Also widen the CLI breaker's trip condition from the literal string
"reverted" to "not a clean routed success", so any future non-success
outcome the observability layer records also latches the breaker
instead of silently falling through.

Closes #3441

Co-Authored-By: Miga <noreply@anthropic.com>

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

Verdict: APPROVE

Reasoning: The stall watchdog can now actually release a worker already awaiting each of the three per-frame capture entrypoints. The stage keeps its watchdog controller separate from the parent cancellation signal, rewrites a watchdog abort into a non-cancellation stall error, and therefore reaches the existing pinned fallback / reverted outcome / CLI latch. Listener cleanup and late settlement of the abandoned capture promise are handled. I found no code blocker at this exact head.

The final Windows check was still pending at review time and remains an independent landing gate. No merge action.

— Magi

@terencecho terencecho 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.

Concur with @magi-bot APPROVE at 68028bf9. Independent re-verification at head:

End-to-end mechanism traced clean: stall watchdog (captureStreamingStage.ts:664-669 — separate controller from parent, load-bearing since shouldRetryViaPinnedFallback returns false on isCancellation) → workerSignal = AbortSignal.any([parent, stall]) (parallelCoordinator.ts:924-926) → raceAgainstAbort at all three entrypoints (parallelCoordinator.ts:539,586,591) → workers reject → captureFailure classifies non-fatal cancel → captureStreamingStage rewrites to stall (captureStreamingStage.ts:739-745, gated on abortSignal?.aborted !== true) → renderOrchestrator's shouldRetryViaPinnedFallback returns true → syncCapturePlan flips to "reverted" → CLI sees "reverted" → breaker trips. lastProgressAt is initialized at captureStreamingStage.ts:672 BEFORE executeParallelCapture — frame-0 covered, no off-by-one.

Windows CI resolved since @magi-bot's review: Render on windows-latest SUCCESS 04:43:48Z, Tests on windows-latest SUCCESS 04:50:54Z. Landing gate is clear.

Three non-blocking nits (worth a follow-up commit, not a re-review):

  1. Regression test hits the wrong branch. parallelCoordinator-peerAbort.test.ts:76-116 wedges captureFrame (onFrameBuffer=undefined, parallelCoordinator.ts:591), but the WSL2 field-report path is captureFrameToBuffer (parallelCoordinator.ts:586, onFrameBuffer provided). captureStreamingStage.ts:633,719 always passes onFrameBuffer, and Linux/WSL2 is BeginFrame-mode so workerEncodeEnabled=false (gated in frameCapture.ts:1115-1118 on syncToPaintEvent=true, beginFrameTimeTicks=0 — macOS-hw-GPU-only). The raceAgainstAbort wrapping is identical by inspection so the fix works, but a future refactor of the captureFrameToBuffer branch could silently regress WSL2 while the test still passes. One-line change: add an onFrameBuffer: async () => {} argument to flip the test to the actual path.

  2. CLI-side "widen" at render.ts:1355-1368 is inert for the WSL2 bug. The stall path lands deParallelRouter="reverted" at renderOrchestrator.ts:3400 via syncCapturePlan after replanAfterFailure, and observability.ts:110-111 types the field as strictly "routed" | "reverted" — so resolveDeParallelRouterOutcome already returns exactly "reverted" under both old and new logic. The widen is future-defense against enum growth, not part of the fix; the commit message and inline comment overstate its role. Worth re-wording so a future stall-not-tripping-breaker debugger doesn't misplace the load-bearing bit (which is the syncCapturePlan → "reverted" handoff, not the CLI predicate).

  3. Pipelined-abort path can drop an unhandled rejection. parallelCoordinator.ts:538-547: an abort during the pipelined await throws at 538-541, so the encodeResult.catch(() => {}) guard at 547 never runs. If captureFrameToBufferPipelined later resolves with a value whose inner encodeResult later rejects (worker crash / cleanupDrawElementWorkerEncode teardown, per the pre-existing comment at captureStreamingStage.ts:492-495), that surfaces as an unhandled rejection. macOS-hw-GPU-only, so the WSL2 fix isn't affected — cosmetic teardown noise.

None of the above blocks landing.

— Review by tai (pr-review)

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.

HF_DE_PARALLEL_ROUTER is on by default and hangs native WSL capture at frame 0 with no output; its circuit breaker cannot fire on a hang

3 participants