fix(producer): extend DE stall watchdog to the single-worker streaming path

[vanceingalls]

Summary

The no-frame-progress stall watchdog added for the parallel DE streaming path only guarded workerCount > 1. The single-worker DE-inversion path (both the worker-encode pipeline and the plain per-frame loop) had no equivalent protection — a wedged capture call would sit until the ~5min CDP protocolTimeout, producing capture_error fallback stalls up to ~92 minutes. Telemetry showed ~88% of these stalls occur on the single-worker path, not the parallel one the existing watchdog covers.

Changes

• Extend the watchdog to the sequential capture path. Since captureFrameToBuffer / captureFrameToBufferPipelined / captureFramesBatchPipelined take no AbortSignal (unlike executeParallelCapture), a wedged call can't be cancelled — instead each capture call is raced against a stall deadline (raceAgainstStall) anchored to the last frame that made progress. A trip abandons the in-flight capture (same orphaned-promise contract already used for encodeResult) and rejects fast so the render falls back to screenshot instead of hanging.
• Applied to all three sequential capture call sites: the worker-encode batch path, the worker-encode per-frame path, and the plain (non-worker-encode) per-frame loop.
• Renamed the shared config from HF_DE_PARALLEL_STALL_MS/resolveParallelStallTimeoutMs to HF_DE_STALL_MS/resolveDeStallTimeoutMs since it now covers both paths, not just parallel.

Test plan

• New tests: stall trips on the single-worker worker-encode pipeline, and on the single-worker plain capture loop (captureStreamingStage.test.ts)
• Existing parallel-path watchdog tests updated for the renamed env var, still pass
• bun run test:unit in packages/producer — full pass except one pre-existing, unrelated flaky timeout in hdrImageTransferCache.test.ts (untouched by this change)
• typecheck, oxlint, oxfmt clean

[vanceingalls]

• fix(producer): extend DE stall watchdog to the single-worker streaming path #2473 👈 (View in Graphite)
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[miga-heygen]

Review — #2473 fix(producer): extend DE stall watchdog to the single-worker streaming path

Verdict: LGTM — well-designed extension of the existing watchdog. Clean mechanics, correct timeout behavior, good test coverage.

SSOT check

Item	Owner	Duplicates?
Stall timeout config	resolveDeStallTimeoutMs() / HF_DE_STALL_MS (single source)	Renamed from PARALLEL-scoped — clean
Parallel detection	Poll-based setInterval + AbortController	Correct — parallel path has AbortSignal support
Sequential detection	raceAgainstStall per-capture call	Correct — sequential path has NO signal support, race is the right mechanism

Two mechanisms for two genuinely different constraints. The PR description explains the split clearly.

raceAgainstStall review

Clean generic implementation:

• clearTimeout on both resolve and reject paths — no timer leak
• Math.max(0, deadlineMs) handles the edge case where elapsed time already exceeds the stall window (fires on next tick → immediate fail, which is correct)
• Orphaned-promise contract for the abandoned capture call is explicitly acknowledged and matches the existing encodeResult pattern

Deadline computation

stallTimeoutMs - (Date.now() - lastProgressAt) at each call site. lastProgressAt is updated AFTER the frame is written to the encoder + reorder buffer + framesRendered incremented — "progress" means "committed," not "captured." Correct placement.

The captureWithStallGuard closure in runWorkerEncodePipelineLoop captures lastProgressAt by reference (let in outer scope), so each invocation sees the latest value. Clean.

Coverage

Three sequential call sites wrapped:

1. Worker-encode batch path (captureFramesBatchPipelined)
2. Worker-encode per-frame path (captureFrameToBufferPipelined)
3. Plain per-frame loop (captureFrameToBuffer)

Two new tests: worker-encode pipeline stall + plain capture loop stall. Both use new Promise(() => {}) (never resolves) with a 50ms timeout. Existing parallel tests updated for the renamed env var.

Mock fix

captureFrameToBufferPipelined mock now returns { encodeResult: Promise.resolve(...) } instead of { encodeResult: { buffer: ... } } — looks like a correction to match the real interface where encodeResult is a Promise.

No blockers. Ship it.

[james-russo-rames-d-jusso]

Reviewed at 3ea435ae. CI is green. Reviewing as first pass; no prior reviews to layer on.

Blockers

(none)

Concerns

🟠 Env var rename HF_DE_PARALLEL_STALL_MS → HF_DE_STALL_MS has no backwards-compat shim. On origin/main the old name only appears in code (captureStreamingStage.ts + captureStreamingStage.test.ts), so nothing in the repo drifts. But if any external ops surface — a deploy manifest, a feature-flag rollout gate, a runbook that suggests tuning it for the ≥24GB macOS trial cohort the original watchdog was scoped to, a Kubernetes ConfigMap, a .env under HF ops — sets HF_DE_PARALLEL_STALL_MS at runtime, it will silently no-op after this PR merges and the code will fall back to the 60s default. Two ways out:

1. Bridge both names for one release: const raw = process.env.HF_DE_STALL_MS ?? process.env.HF_DE_PARALLEL_STALL_MS; + a one-shot log.warn when the old name is picked up. Deletable in a follow-up.
2. Confirm in the PR body that no external config surface uses the old name. If you've already checked (ops configs, dashboard queries, runbooks), a one-line note there closes this.

The rename itself reads correctly given the widened scope; this is purely a rollout-safety ask.

🟠 Sequential path can mislabel a parent-abort-during-wedge as "stalled". The parallel path was carefully written to disambiguate — see packages/producer/src/services/render/stages/captureStreamingStage.ts:685 — if (stalled && abortSignal?.aborted !== true) — with a dedicated test at captureStreamingStage.test.ts:260 (does not relabel a genuine parent-abort as a stall). The sequential path's raceAgainstStall is a pure setTimeout race: if the parent aborts DURING a wedged sequential captureFrameToBuffer / …Pipelined call, the wedged promise doesn't reject (no AbortSignal plumbed into those APIs — as the PR body notes), so raceAgainstStall's timer fires after stallTimeoutMs and the caller sees "Sequential drawElement capture stalled: no frame progress for 60000ms …" even though the root cause was a user-initiated abort.

Downstream this still hits capture_error classification (renderOrchestrator.ts:2741) → screenshot-fallback retry, so the FUNCTIONAL behavior is fine (both stall and abort take the same next-step branch). But the observability signal is misleading: a cancelled render will emit log.warn("… re-rendering via screenshot", { error: "…stalled…" }) and a capture_streaming checkpoint that reads as a stall to whoever is watching those logs / metrics for real stall rates.

Two low-cost fixes:

1. Thread the parent signal into raceAgainstStall and, on trip, check signal.aborted before rejecting with the stall message — otherwise reject with an abort error. Keeps the utility narrow: raceAgainstStall(promise, deadlineMs, message, signal?).
2. Inside the setTimeout callback, check abortSignal?.aborted at trip time, and if aborted, skip the message rewrite (let the caller's assertNotAborted() fire on the next iteration instead). Slightly hackier since the utility doesn't know about the outer signal — the first option is cleaner.

Nits

🟡 No sequential-path counterpart to does not relabel a genuine parent-abort as a stall. Once (2) above is addressed, worth adding a single-worker: does not relabel a genuine parent-abort as a stall test that mirrors the parallel-path shape. Locks the abort-vs-stall labeling for both branches under one test file.

🟡 captureWithStallGuard(idxs[0] ?? i, ...) — the idxs[0] ?? i fallback covers an empty-batch case that shouldn't happen (batchSize is clamped >= 1 upstream). Fine as defensive coding, but if it ever fires, the error message anchors to i — the outer-loop iteration variable — which may or may not correspond to the batch that stalled. Minor.

Green notes

🟢 raceAgainstStall is a clean utility. Isolates the deadline race behind a tight signature; the "orphaned, never awaited" contract borrowed from the encode-pipeline is well-documented in the docstring. Math.max(0, deadlineMs) clamp handles the edge where a batch consumed most of the window before returning (next call's deadline goes near-zero → tick-immediate trip → screenshot fallback, which is the correct behavior when the pipeline is that far behind).

🟢 lastProgressAt update sites are exhaustive. Both drain paths in runWorkerEncodePipelineLoop (drainPrev + drainBatch) update it, and the sequential plain-loop's frame-write also updates it. So every actual frame that lands anywhere in the sequential family refreshes the window.

🟢 Both new sequential paths pinned by red-first tests. hangSequentialUntilStall = true makes each mocked capture return new Promise(() => {}) (never resolves) — pre-PR that would hang the loop until the outer test-runner timeout; post-PR it trips within the 50ms window. Both workerEncodeEnabled: true and the plain path get their own test.

🟢 Stall error reaches the orchestrator's existing capture_error classification at renderOrchestrator.ts:2741 (via err !== isVerifyError && !isMemoryExhaustion). That path already fires log.warn + observability.checkpoint("capture_streaming", …) + updateCaptureObservability({ deFallbackReason: "capture_error" }), so this PR doesn't need new emission wiring — the observability is inherited via classification. No dashboard-side change needed.

🟢 Docstring rewrite ("worker (parallel path) or the single in-flight capture (sequential path, worker-encode or plain) can wedge mid-capture") correctly widens the invariant's scope to match the new coverage.

What I didn't verify

• Whether any HF-ops surface currently sets HF_DE_PARALLEL_STALL_MS (that's what concern (1) is asking you to check — you have the visibility here that I don't).
• Whether a downstream metric consumer of deFallbackReason === "capture_error" cares about parallel-vs-sequential provenance. Message text includes it, but the classified reason is the same for both.

— Review by Rames D Jusso