Repository navigation
fix(producer): a render whose capture stops advancing fails within minutes - #6257
miguel-heygen wants to merge 4 commits into
Conversation
Edit accuracy: accurate 271 (base branch 271), smooth 163 of thoseThe gate passes. Quarantined, measured but not gated (0) Unstable (1)
|
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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.
8aa52db to
0b80e6f
Compare
Fallow audit reportQuality gate failed Found 9 findings: Dead code 1, Duplication 5, Suppressions 1, Audit 2. Dead code (1)
Duplication (5)
Suppressions (1)
Audit (2)
Generated by fallow.
|
There was a problem hiding this comment.
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 stalefallow-ignore-next-line unit-sizefromcreateCaptureSession(frameCapture.ts:1412, now flagged stale) toinitializeSession, so "code unchanged otherwise" is not quite true. Main's additions toperfSummary.tsandtelemetry/events.tsalso now pushobservability.tsover the clone threshold, andRenderStageStalledError.codeis flagged unused. None of this changes runtime behavior; fix it if the loop gates on Fallow.
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
observeRenderStage, fed byjob.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_MSoverrides 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.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./v1/render-stream: from before the queue wait until just before the final event, it re-sends the normalprogressevent with the live stage and frame counts. It never sends after a terminal event.frameCapture.tsremoved (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
executeRenderJob(real disk capture stage and ladder, only the parallel capture call scripted),renderChunk, andcreateRenderHandlers. 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).Not exercised
No real browser render.
verify:packed-manifestsneeds 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.