fix: discard the warmup capture frame in the parallel coordinator#2489
fix: discard the warmup capture frame in the parallel coordinator#2489mvanhorn wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Blocking finding
[P1] The warmup neither targets the failing boundary nor preserves the compositor's monotonic-time contract — packages/engine/src/services/parallelCoordinator.ts:421-428
I reproduced #2477 from the base before evaluating this patch: at e038cc93c, executeWorkerTask initializes a fresh session and immediately enters captureFrameRange; the first operation for worker 1 is therefore the seek/capture for frame 36, with no paint-settlement barrier. That matches the reported transparent frames at 36/72/108.
This change does not warm that failing seek. discardWarmupCapture(session) uses its defaults, frame 0/time 0 (frameCapture.ts:3250-3262), even when the task starts at frame 36/time 1.2. My current-head negative proof asserted the helper call for runWorker(1, 36) and received only [session], not [session, 36, 1.2]. The new test at parallelCoordinator.test.ts:63-80 mocks the helper as a no-op and checks only call order, so it passes without exercising the frame/time contract.
More critically, blindly capturing a warmup frame reinstates a known compositor deadlock. The current production path documents why discardWarmupCapture is intentionally absent: a second beginFrame at an already-used frameTimeTicks wedges Chrome (packages/producer/src/services/distributed/renderChunk.ts:579-582). Commit 24f64f02c removed the same approach after real multi-chunk validation found both same-time and backward-time deadlocks. This PR calls the helper for worker 0 at frame 0/time 0, immediately followed by the real frame 0/time 0 capture, which is exactly the same-time failure mode. Passing each worker's startFrame would still duplicate that timestamp; passing startFrame - 1 was the already-reverted backward-time failure.
Please fix the actual first-seek paint race without issuing an extra screenshot/beginFrame at a duplicate or backward compositor timestamp—for example, a seek/paint-settlement primitive separated from capture. The regression must reproduce the real boundary: render/decode the transparent multi-worker fixture, assert non-transparent output at interior starts, and prove worker 0 completes without a protocol timeout. A mocked “warmup was called” assertion cannot establish either invariant.
What is good
The failure and abort tests at parallelCoordinator.test.ts:91-115 correctly preserve session cleanup if a pre-capture stage fails. That cleanup discipline should remain in the eventual fix.
CI snapshot: core build/lint/typecheck/test, Windows render/tests, producer suites, and CodeQL are green at 82803ad95598a9e690817f5f8162ac095178f2fa; several regression shards are still running. The blocker above is deterministic and independent of those pending lanes.
Verdict: REQUEST CHANGES
Reasoning: #2477 is reproducible in the base call graph, but this patch warms frame 0 rather than the worker boundary and reintroduces a previously validated Chrome compositor deadlock pattern. It needs a monotonic-time-safe paint-settlement fix and a real boundary-render regression test.
|
Two code changes that might address both review findings:
With the current defaults (frame 0 / time 0) the warmup seeks a frame the worker never renders, so the boundary race at
The deadlock documented in That would leave beginFrame mode still needing the seek/paint-settlement primitive the review asks for, but a mode-guarded, startFrame-targeted warmup could fix the screenshot path (where transparent overlay renders live) without reintroducing the reverted failure modes. WDYT? @mvanhorn |
What
Discards the throwaway warmup frame that each parallel worker captures before its first real seek has painted, so no fully transparent frame is emitted at a worker chunk boundary.
Why
In parallel capture, a worker's very first capture runs before the seek it issued has painted, so the first frame of every worker chunk came back fully transparent. Issue #2477 reported a transparent frame at each chunk boundary. The single-worker path already warms up and discards that first frame; the parallel coordinator was missing the same step.
How
parallelCoordinatornow callsdiscardWarmupCapture(session)immediately after a worker session is initialized, mirroring the existing single-worker warmup.frameCapture.tswas left unchanged because its existing helper already restores the frame counters, so accounting stays correct after the discard.Test plan
Added
parallelCoordinatortests covering call ordering (warmup before the first real capture), worker 0 vs non-zero workers, frame accounting after the discard, warmup failure, abort handling, and session cleanup. Targeted Vitest: 30 passed.oxfmtandoxlintare clean on the changed files.Fixes #2477