fix(engine): pre-create __render_frame__ siblings in initializeSession

[varo-yang]

Fixes #2007.

Chunk-lambda renders have been throwing a periodic near-black frame — one bad frame every chunk_frames / worker_count frames. On a single-video 4-worker chunk that's every 60 frames, and signalstats catches it cleanly: YAVG drops to ~22 while YMAX stays ~240. Local single-process renders never show it, which is the tell — they don't run under BeginFrame.

I tracked it to the isNewImage branch in injectVideoFramesBatch. The first time a session paints a given videoId there's no __render_frame__ sibling yet, so it creates the <img> on the spot — createElement + insertBefore. captureFrameCore fires beginFrameCapture right after, and under HeadlessExperimental.BeginFrame the compositor doesn't have that freshly-inserted layer in the immediately-next frame, so the first captured frame per session paints only body background + whatever was already composited (the YAVG ~22 signature). Each lambda worker is its own session, so it recurs at every worker boundary.

The fix pre-creates the hidden sibling at the end of initializeSession, then drives one explicit non-capture visual HeadlessExperimental.beginFrame (noDisplayUpdates: false) to composite the new layers before frame 0. The warmup ticks can't do that — they're noDisplayUpdates: true, so they advance the clock but don't paint, and the seek in prepareFrameForCapture doesn't tick either; without the explicit commit frame the first display-producing BeginFrame would be the capture itself. The commit tick sits one interval below beginFrameTimeTicks (which already carries +10 intervals of headroom over the last warmup tick), so it stays monotonic and doesn't eat a render frame, and the warmup loop is now drained unconditionally before it so it can't race an in-flight warmup frame. Every subsequent inject then takes the hasImg = true (src-update) path; the isNewImage branch stays as a fallback for callers that don't go through initializeSession.

[miguel-heygen]

Focused review of the BeginFrame ordering fix. No prior reviews or inline comments were present when checked.

Strengths: packages/engine/src/services/screenshotService.ts:566 keeps the sibling creation idempotent and preserves the existing injectVideoFramesBatch fallback path, and packages/engine/src/services/screenshotService.test.ts:749 adds direct coverage for that helper's DOM behavior.

Blockers

• blocker packages/engine/src/services/frameCapture.ts:1637 - in the BeginFrame init path, ensureRenderFrameSiblings() runs after warmupState.running = false at packages/engine/src/services/frameCapture.ts:1616. The PR's fix relies on warmup/readiness BeginFrame ticks committing the new <img> layer before capture, but no compositor-driving tick happens after the sibling is inserted and before beginFrameCapture at packages/engine/src/services/frameCapture.ts:2288. That means the first real compositor tick after insertion can still be the captured frame, leaving the original worker-boundary near-black flash race in place. Please move the creation before warmup can stop, or explicitly drive a non-capture BeginFrame after creating the siblings and before session.isInitialized = true.

Verdict: REQUEST CHANGES
Reasoning: The core timing guarantee for the compositor workaround is not established in the BeginFrame path, so the bug this PR is meant to fix can still reproduce.

-- Codex

[varo-yang]

@miguel-heygen good catch, you're right. I traced the BeginFrame path end to end and there really is no compositor tick between the insert and the first beginFrameCapture on main: warmup has already stopped by the time the siblings are created, the seek in prepareFrameForCapture doesn't drive a frame, and the paint flush is skipped in beginframe mode. So the first captured frame can still miss the layer, exactly as you called out.

What threw me is that this same pre-create already kills the flash in our production render — we've been shipping it as a patch on 0.7.37 for a while. Turns out that's because on 0.7.37 it happens to land inside the warmup window, so the readiness-poll ticks commit the layer before capture; the port to main moved it past where warmup stops and quietly lost that guarantee.

7d28cf8 moves the creation before the readiness polls (while warmup is still ticking) instead of at the end of init, which puts it back where it effectively sits in the version we've validated in prod. Mind taking another look?

[miguel-heygen]

Thanks for tracing this through. I rechecked the new 7d28cf8 placement against the BeginFrame capture path.

Strengths: packages/engine/src/services/frameCapture.ts:1562 moves sibling creation much earlier than the previous version, and packages/engine/src/services/screenshotService.ts:566 keeps the helper idempotent so the normal injection fallback remains intact. I also checked the image-readiness side effect: the placeholder <img>s have no src, and pollImagesReady ignores those at packages/engine/src/services/frameCapture.ts:1197, so this should not add a readiness timeout.

Blockers

• blocker packages/engine/src/services/frameCapture.ts:1525 / packages/engine/src/services/frameCapture.ts:1570 - the new fix still relies on warmup frames with noDisplayUpdates: true to commit the newly inserted render-frame layers before capture. CDP's own HeadlessExperimental.beginFrame schema says that with noDisplayUpdates true, visual updates may not be visible in screenshots. In this path, after ensureRenderFrameSiblings(page) at packages/engine/src/services/frameCapture.ts:1574, I don't see any noDisplayUpdates: false / visual BeginFrame before the first captured beginFrameCapture at packages/engine/src/services/frameCapture.ts:2301. So the first display-producing BeginFrame after the insert can still be the screenshot frame itself, which leaves the same compositor-layer timing race in place. Please either issue one non-capture HeadlessExperimental.beginFrame with noDisplayUpdates: false after creating the siblings and before session.isInitialized = true, or otherwise make the post-insert warmup tick a real display update while preserving monotonic ticks.

Verdict: REQUEST CHANGES
Reasoning: The new ordering is closer, but the code still does not establish a guaranteed visual/compositor tick between inserting the fallback layers and the first captured BeginFrame.

-- Codex

[miguel-heygen]

Thanks, b98f77f fixes the actual noDisplayUpdates issue from my last review: the sibling insert is now followed by a non-capture visual BeginFrame before frame 0.

Strengths: packages/engine/src/services/frameCapture.ts:1650 now pre-creates the siblings and packages/engine/src/services/frameCapture.ts:1652 sends the explicit noDisplayUpdates: false BeginFrame in the pre-frame-0 tick gap, which is the right compositor contract. packages/engine/src/services/screenshotService.ts:558 also documents the warmup-vs-visual tick distinction clearly.

Blockers

• blocker packages/engine/src/services/frameCapture.ts:1616 / packages/engine/src/services/frameCapture.ts:1652 - the new commit tick is sent with a raw commitCdp.send("HeadlessExperimental.beginFrame", ...) immediately after warmupState.running = false, but the unlocked warmup path does not await warmupLoopPromise before sending it. If the warmup loop is currently inside its own HeadlessExperimental.beginFrame when init reaches this point, this visual commit tick can race the in-flight warmup tick and fail with the same Another frame is pending class that beginFrameCapture already has retry handling for in packages/engine/src/services/screenshotService.ts:123. Please drain the warmup loop for both locked and unlocked modes before deriving/sending the commit tick, or route the commit tick through the same pending-frame retry helper.

Verdict: REQUEST CHANGES
Reasoning: The compositor commit is now conceptually correct, but it is issued on an undrained BeginFrame session and can turn the warmup/capture boundary into an init-time flake under load.

-- Codex

[varo-yang]

@miguel-heygen good catch — the unlocked path cleared the flag but never waited for the loop, so the commit tick could race an in-flight warmup beginFrame ("Another frame is pending"). Made the warmup drain unconditional in 3f49507 so the loop is fully settled before the commit tick. Thanks!

[miguel-heygen]

Re-reviewed latest head 3f49507 after the warmup-drain update.

Strengths: packages/engine/src/services/frameCapture.ts:1621 now drains the warmup loop unconditionally before any further BeginFrame, closing the pending-frame race from my last review. packages/engine/src/services/frameCapture.ts:1653 then pre-creates the render-frame siblings and packages/engine/src/services/frameCapture.ts:1655 drives one visual non-capture BeginFrame before frame 0, so the compositor-layer timing guarantee is now explicit. The helper remains idempotent in packages/engine/src/services/screenshotService.ts:567, and the new unit coverage in packages/engine/src/services/screenshotService.test.ts:771 covers creation/idempotency/filtering.

Notes: CI was still running when I checked, but no failing check runs were present. The PR body still describes the old warmup/readiness-tick mechanism; it would be worth updating it to mention the explicit visual commit tick and unconditional warmup drain.

Verdict: APPROVE
Reasoning: The previous BeginFrame ordering and pending-frame blockers are addressed in code; remaining items are CI completion and PR-description cleanup.

-- Codex

[varo-yang]

@miguel-heygen — Heads-up before this gets merged — the render-regression shards are failing, and it looks like a real regression from this change rather than flakiness.

I tracked it to sub-composition-video (shard-7). On main that composition renders straight through BeginFrame — no liveness-probe timeout, no relaunch, passes clean (Total 6 | Passed 6). With this PR it stalls instead: the hidden __render_frame__ siblings that ensureRenderFrameSiblings pre-creates add enough compositor layers to push that comp (sub-comps + video, already layer-heavy) past the SwiftShader "heavy-layer stalls BeginFrame" threshold. So the liveness probe times out at 30s, the render relaunches into screenshot mode, and the relaunch dies with Attempted to use detached Frame at the browser_probe phase. The suite fails — and then the process never exits, so the shard just sits there until the 60-minute job timeout cancels it. Same on shard-6, and shard-1 joined on the latest run.

So the pre-create-siblings approach is trading the black-frame fix for a compositor-load regression on the heavier comps. I don't think it's mergeable as-is: the sibling layers need to be scoped to only the case where the black frame actually shows up, or the approach reworked so it doesn't leave persistent layers on compositions that are already sitting at the BeginFrame limit. Digging into that.

[varo-yang]

@miguel-heygen — pushed a fix; my earlier read was wrong. It's not the sibling layers — I bisected it on a native x86 SwiftShader box and it's the commit tick's tick ordering. It fires at beginFrameTimeTicks - 1·interval, but the producer's liveness probe then fires at - 5·interval (an earlier tick), so BeginFrame time runs backwards and stalls. Moving the commit tick to - 6·interval keeps warmup < commit < probe < capture monotonic and clears it on every affected comp. Regression is green on all shards now — the one red check is a flaky bun install I can't re-run myself. Another look when you get a chance?

[miguel-heygen]

@varo-yang fix the failed ci check