Skip to content

fix(producer): a render whose capture stops advancing fails within minutes - #6257

Open
miguel-heygen wants to merge 4 commits into
mainfrom
fix/producer-stall-progress
Open

miguel-heygen wants to merge 4 commits into
mainfrom
fix/producer-stall-progress

Conversation

@miguel-heygen

@miguel-heygen miguel-heygen commented Oct 24, 2026 •

Copy link
Copy Markdown
Collaborator

What changes for users

A render whose capture stops landing frames now fails in minutes instead of sitting silent, and a request that is alive but quiet now says so. When a capture attempt stalls, the existing retry (fewer workers, fresh session) still runs; the render fails only when every attempt has stalled.

How

  • One stall rule in observeRenderStage, fed by job.framesRendered (every capture path writes it). A capture attempt is aborted after 300 s without a first frame, then 60 s without a new frame (HF_DE_STALL_MS overrides the 60 s). It covers parallel and sequential disk capture, HDR sequential and hybrid, and distributed chunk capture. Stages that already had their own stall guard (sequential streaming, worker-encode, segmented, parallel streaming) are unchanged.
  • The trip aborts only that attempt, and is classified exactly like today's per-call capture timeout, so the existing ladder runs: parallel disk capture halves its workers (4 to 2 to 1) when the stalled attempt landed at least one frame. Sequential disk and HDR have no retry for this, as with the timeout today. Each attempt gets a fresh window. When the last attempt also stalls the render fails with RENDER_STAGE_STALLED (owner system, retryable: false), added to the server's safe error codes so it reaches the wire. An aborted attempt that neither retries nor settles within one more window fails the stage.
  • 30 s keepalive on /v1/render-stream: from before the queue wait until just before the final event, it re-sends the normal progress event with the live stage and frame counts. It never sends after a terminal event.
  • Bounded ends: the final event flush gives up after 10 s, closing a capture session no longer waits forever on the last browser memory sample (5 s cap), and the render slot is released when the render ends rather than after the final write, so a client that stops reading cannot hold a slot.
  • Two stale fallow comments in frameCapture.ts removed (the gate fails on them).

Package size

scripts/package-size-budgets.json: the producer budget goes from 781312/2840135 to 787935/2862300 (packed/unpacked), main's budget plus this change's measured delta. Measured: main 787145/2860280, this change 793768/2882445. The change sits inside the 1% tolerance of the new budget.

Verification

  • New tests go through executeRenderJob (real disk capture stage and ladder, only the parallel capture call scripted), renderChunk, and createRenderHandlers. Run against main's production code they fail; with the change they pass three runs in a row (engine 7, server 8 + 2, chunk 1, orchestrator 6).
  • Cases: attempt 1 stalls, attempt 2 on two workers lands its first frame 200 s in, the render completes; every attempt stalls, the render fails typed within about the summed windows; a capture landing a frame every 50 s for 2,200 s is never killed; keepalive during the queue wait, none after a terminal event; a never-settling flush and a never-settling memory sample both give up.
  • Sixteen single-line mutations of the production code each turn their test red. Format, lint, both typechecks, comment ratchet, fallow pass.

Not exercised

No real browser render. verify:packed-manifests needs every package built (size measured on the producer pack directly). A pool that lands no frame in 300 s now fails typed instead of taking the single "Network.enable timed out" retry at half the workers.

@github-actions

github-actions Bot commented Oct 24, 2026 •

Copy link
Copy Markdown
Contributor

Edit accuracy: accurate 271 (base branch 271), smooth 163 of those

The gate passes.
Smoothness is reported in the artifact, not gated. A case fails only if it fails 2 of 3 runs.

Quarantined, measured but not gated (0)

Unstable (1)

  • seqplainkeys-none-center-r0-root-z100: tracking 0.06, pressJump 0, drop 0, reload 0, render 0.03, renderKey -, undo false, teleport true / tracking 0.06, pressJump 0, drop 0, reload 0, render 0.03, renderKey -, undo true, teleport true / tracking 0.06, pressJump 0, drop 0, reload 0, render 0.03, renderKey -, undo true, teleport true

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

Verdict: APPROVE · 0 blocker · 1 important · 0 nits · head 8aa52db2

8aa52db brings back main's evaluateWithinBound, PROBE_READ_TIMEOUT_MS and the log params exactly, so renderOrchestrator.ts no longer drops anything relative to the merge base. A stalled attempt reaches the existing ladder as protocol_timeout, stalledBy() makes a cancel caused by the stall fail as RENDER_STAGE_STALLED, and the keepalive is cleared before each terminal write. One gap is ↓ inline.

Scope

Read the full non-test diff. Traced watchStall, the parallel ladder, the sequential and HDR attempt signals, the chunk path, and how parallel-coordinator progress is emitted (only landed frames write framesRendered; worker-phase events do not, so nothing resets the window by mistake). Tests were not run locally (no bun here). CI was still running at review time.

options.dedupPerfs.length = 0;
const attemptSignal = options.openAttempt?.(options.abortSignal) ?? options.abortSignal;
try {
for (const tasks of batches) {

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.

important: a retry batch after the first gets only the 60 s window to start its browsers, not the 300 s first-frame window. openAttempt runs once per attempt, but buildMissingFrameRetryBatches returns more than one batch when there are more missing ranges than workers. Example: 4 workers stall, leaving 4 ranges, and the retry runs on 2 workers as 2 batches. Batch 2 launches a fresh pool after batch 1 has already landed frames, so the watcher is on noProgressMs. On a heavy comp whose startup takes longer than 60 s (the case the 300 s window exists for), batch 2 trips the stall, the ladder halves to 1 worker, and the render fails RENDER_STAGE_STALLED where main would have finished.

Fix: re-open the first-frame window at the start of each batch (a restartWindow(firstProgressMs) hook on the attempt, or open the attempt per batch), plus a test with a 2-batch retry whose second pool lands its first frame after more than 60 s.

@miguel-heygen
miguel-heygen force-pushed the fix/producer-stall-progress branch from 8aa52db to 0b80e6f Compare October 25, 2026 22:59
@github-actions

Copy link
Copy Markdown
Contributor

Fallow audit report

Quality gate failed

Found 9 findings: Dead code 1, Duplication 5, Suppressions 1, Audit 2.

Dead code (1)
Severity Rule Location Description
major fallow/unused-class-member packages/producer/src/services/render/observability.ts:619 Class member 'RenderStageStalledError.code' is never referenced
Duplication (5)
Severity Rule Location Description
minor fallow/code-duplication packages/cli/src/telemetry/events.ts:387 Code clone dup:728fb1b2 (37 lines, 3 instances)
minor fallow/code-duplication packages/producer/src/services/render/observability.ts:80 Code clone dup:f1608379 (60 lines, 2 instances)
minor fallow/code-duplication packages/producer/src/services/render/observability.ts:103 Code clone dup:728fb1b2 (37 lines, 3 instances)
minor fallow/code-duplication packages/producer/src/services/render/perfSummary.ts:105 Code clone dup:f1608379 (60 lines, 2 instances)
minor fallow/code-duplication packages/producer/src/services/render/perfSummary.ts:109 Code clone dup:728fb1b2 (37 lines, 3 instances)
Suppressions (1)
Severity Rule Location Description
minor fallow/stale-suppression packages/engine/src/services/frameCapture.ts:1412 // fallow-ignore-next-line unit-size ('unit-size' is not a recognized fallow issue kind. Other tokens on this line still apply.)
Audit (2)
Severity Rule Location Description
minor fallow/added-suppression packages/producer/src/services/render/observability.ts:620 The change adds a `fallow-ignore` suppression comment (fallow-ignore)
minor fallow/added-suppression packages/producer/src/services/render/observability.ts:622 The change adds a `fallow-ignore` suppression comment (fallow-ignore)

Generated by fallow.

Gate outcomes: failed audit-verdict.

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

Verdict: APPROVE · 0 blocker · 1 important · 0 nits · 8aa52db2 → 77b24e9c

I checked the rebase onto 0ae48c9f against the approved patch. The stall rule, retry ladder, keepalive and bounded teardown are unchanged. The stall abort composes with #6224's executionSignal (a too-slow abort is the outer signal, so it is treated as cancellation, not a stall). The size budget (+6623 / +22165 B over main) matches the earlier measured delta. All required checks pass.

  • important: Fallow audit (not required) is red on this head but was green on 8aa52db2. The rebase moved the removed stale fallow-ignore-next-line unit-size from createCaptureSession (frameCapture.ts:1412, now flagged stale) to initializeSession, so "code unchanged otherwise" is not quite true. Main's additions to perfSummary.ts and telemetry/events.ts also now push observability.ts over the clone threshold, and RenderStageStalledError.code is flagged unused. None of this changes runtime behavior; fix it if the loop gates on Fallow.

This branch has not been deployed

No deployments
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.

1 participant