Skip to content

feat(engine): first-frame warmup capture helper for distributed chunks#771

Merged
jrusso1020 merged 1 commit into
mainfrom
feat/engine-discard-warmup-capture
May 13, 2026
Merged

feat(engine): first-frame warmup capture helper for distributed chunks#771
jrusso1020 merged 1 commit into
mainfrom
feat/engine-discard-warmup-capture

Conversation

@jrusso1020
Copy link
Copy Markdown
Collaborator

@jrusso1020 jrusso1020 commented May 13, 2026

What

New discardWarmupCapture(session, frameIndex=0, time=0, innerCapture?) helper in packages/engine/src/services/frameCapture.ts. Runs one capture through the standard captureFrameCore path, throws the buffer away, and restores the session's perf and BeginFrame damage counters. Re-exported from packages/engine/src/index.ts.

Why

Part of Phase 2, §5.2 (lastFrameCache row) and §17.2 (gating table).

Chrome's BeginFrame screenshot pipeline maintains a per-process lastFrameCache: when a captured frame's hasDamage reports false, the screenshot path returns the previously captured buffer. For chunk N (N > 0) the distributed worker has no prior frame in its cache, so the very first capture's hasDamage reporting diverges from what an in-process render at the same absolute frame index would see — the in-process renderer always has frame N-1 cached.

Running a discarded warmup capture before the first real capture primes the cache, so chunk output is byte-identical to in-process output.

How

  • discardWarmupCapture snapshots session.capturePerf (shallow copy of the five number fields) and session.beginFrameHasDamageCount / beginFrameNoDamageCount before delegating to captureFrameCore.
  • Restoration runs in a finally, so failed warmup captures don't leak inflated counters into the real capture summary.
  • The innerCapture parameter is injectable; tests pass a stub that mutates session state, asserts the wrapper restores it.
  • Writes no file to disk (the function has no return value, so the buffer can't escape).

Test plan

  • Unit tests added: frameCapture-discardWarmup.test.ts (7 cases) covers single-invocation with default args, custom (frameIndex, time), perf-counter restoration on success, damage-counter restoration on success, restoration on error (the finally contract), no fs write to outputDir, and the no-return-value invariant.
  • No caller invokes the helper yet — Phase 3's renderChunk() will run it after initializeSession resolves.
  • bun run --cwd packages/engine test — 7 new + all existing pass.
  • bun run --cwd packages/engine typecheck clean.
  • bunx oxlint + bunx oxfmt --check clean.

This is part of a stack of 10 PRs; this is PR 6 of 10. Stacked on top of #770.

🤖 Generated with Claude Code

miguel-heygen
miguel-heygen previously approved these changes May 13, 2026
Copy link
Copy Markdown
Collaborator

@miguel-heygen miguel-heygen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clean warmup-capture helper. The snapshot/restore pattern in finally is the right way to handle perf counter isolation — ensures the discard capture never biases getCapturePerfSummary() averages, even on failure.

Key correctness checks:

  • Shallow {...session.capturePerf} copy is sufficient since all five fields are primitive numbers — no deep-copy needed.
  • beginFrameHasDamageCount and beginFrameNoDamageCount are scalar numbers, direct assignment restoration is correct.
  • Injectable innerCapture with a type alias (DiscardWarmupInnerCapture) is clean — matches the real captureFrameCore signature without tests needing to import internals.
  • The void return type enforces the "buffer can't escape" invariant at the type level.
  • Test coverage hits the finally contract explicitly (throw case), which is the most important path to verify.

The outputDir no-write test (comparing readdirSync before/after) is a nice touch — confirms the buffer is truly discarded, not accidentally flushed by some file-write side effect in the inner capture path.

LGTM.

— Magi

Copy link
Copy Markdown
Collaborator

@vanceingalls vanceingalls left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Strengths:

  • try { … } finally { restore } (frameCapture.ts:867-873) is the right shape — even a failed warmup capture restores perf counters, so a transient first-frame failure doesn't leak inflated averages into getCapturePerfSummary(). Pinned by the "restoration on error" test (frameCapture-discardWarmup.test.ts:147).
  • DiscardWarmupInnerCapture typed injectable (frameCapture.ts:817) allows tests to drive both success and failure of the inner capture without needing real Chrome — keeps the unit tests fast and deterministic.
  • Comment at frameCapture.ts:820-830 correctly captures the lastFrameCache rationale (Chrome's BeginFrame screenshot pipeline returns the previous buffer when hasDamage===false) — this is the load-bearing reason for the warmup, and getting the comment wrong would mislead future readers.
  • Buffer is never returned (Promise<void> signature, line frameCapture.ts:861) so the helper can't accidentally leak the warmup frame into the output pipeline. Pinned by (frameCapture-discardWarmup.test.ts:174 expect(result).toBeUndefined()).

Findings:

nit: frameCapture.ts:866 const perfBefore = { ...session.capturePerf } — shallow clone is correct because all five fields of capturePerf are primitive numbers (per the comment). If capturePerf ever grows a nested object (e.g. histogram buckets), this restoration silently leaks the warmup's mutations into the real summary. Worth a brittleness comment or a Readonly<typeof session.capturePerf> type pin.

nit: frameCapture.ts:864 jsdoc @param frameIndex — frame index to warm up with (default 0). Chunk workers typically pass their chunk's first absolute frame index. — but the wrapper's whole point is that the lastFrameCache primes with whatever frame is captured. Whether to use 0 or the chunk's first absolute frame doesn't matter for the cache-priming effect (chrome doesn't index the cache by frame number). The default-0 is fine, but the jsdoc oversells the parameter's significance. Consider clarifying: "Frame index passed to the inner capture; defaults to 0. The actual cache-priming effect is index-independent — chunk workers may pass their chunk's first absolute index for log clarity."

Verdict: APPROVE
Reasoning: Clean small helper with the right try/finally shape and good test coverage on the restoration contract.

— Vai

Part of Phase 2 of the distributed rendering plan (determinism hardening).
See DISTRIBUTED-RENDERING-PLAN.md §5.2 (lastFrameCache row) and §17.2
(gating table).

Adds `discardWarmupCapture(session, frameIndex=0, time=0, innerCapture?)`
in packages/engine/src/services/frameCapture.ts. Performs one capture
through the standard `captureFrameCore` path, throws the buffer away, and
restores the session's perf and BeginFrame damage counters.

Distributed chunk workers need this because Chrome's BeginFrame screenshot
pipeline maintains a per-process `lastFrameCache`: when a captured frame's
`hasDamage` reports `false`, the screenshot path returns the previously
captured buffer. For chunk N (N > 0) the worker has no prior frame in its
cache, so the very first capture's `hasDamage` reporting diverges from
what an in-process render at the same absolute frame index would see (the
in-process renderer always has frame N-1 cached). Running a discarded
warmup capture before the first real capture primes the cache so chunk
output is byte-identical to in-process output.

The wrapper:
  - Takes an injectable `innerCapture` so tests can stub the Chrome path
    (default is the real `captureFrameCore`).
  - Restores `session.capturePerf`, `beginFrameHasDamageCount`, and
    `beginFrameNoDamageCount` after the inner call — even on error — so
    warmup captures don't pollute `getCapturePerfSummary()` averages.
  - Writes no file to disk.

In-process behavior is unchanged: no caller invokes the new helper yet.
Phase 3's `renderChunk()` will run it as the first step after
`initializeSession` resolves.

Re-exported from packages/engine/src/index.ts.

7 unit tests at packages/engine/src/services/
frameCapture-discardWarmup.test.ts cover the post-conditional contract:
single inner-capture invocation, perf/damage restoration on success,
restoration on error, no-fs-write.

This is part of a stack of 10 PRs; this is PR 6 of 10.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@jrusso1020 jrusso1020 force-pushed the feat/engine-discard-warmup-capture branch from 4af4705 to c139197 Compare May 13, 2026 04:36
@jrusso1020 jrusso1020 force-pushed the feat/producer-freeze-plan-runtime-env branch from 1800f49 to 1d189aa Compare May 13, 2026 04:36
vanceingalls
vanceingalls previously approved these changes May 13, 2026
Copy link
Copy Markdown
Collaborator

@vanceingalls vanceingalls left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review of the new HEAD (c139197b) after prior approval at 4af47059 was dismissed. PR diff vs. base is the three-file shape: discardWarmupCapture helper in frameCapture.ts, an injectable inner-capture type, and 183 lines of unit test stubbing the inner capture.

Strengths

  • frameCapture.ts:861-876try { inner } finally { restore } is the right discipline. The "restores state even when the inner capture throws" test (frameCapture-discardWarmup.test.ts:150) pins that the warmup failure doesn't leak inflated counters into the real capture summary — exactly the failure mode this rule guards.
  • Injectable DiscardWarmupInnerCapture (frameCapture.ts:228) is the right abstraction — it makes the helper testable without spawning Chrome, and the prod default (captureFrameCore) is wired in by default-parameter so the prod call site is just discardWarmupCapture(session).
  • "writes no output file" test (frameCapture-discardWarmup.test.ts:177) catches the post-condition that the buffer can't escape — gives confidence the helper is genuinely a discard.

Findings

importantframeCapture.ts:861 — Side-effect snapshot is incomplete. captureFrameCore also mutates session.browserConsoleBuffer (see frameCapture.ts:437 and :454, where console.log / error events get pushed into a fixed-size ring buffer). The warmup capture's console output will linger in the buffer and bias the diagnostics dump if a later capture fails (see :661browserConsoleTail: session.browserConsoleBuffer.slice(-30) is what gets written into the failure diagnostics file). Not a correctness regression (the warmup itself works fine), but a downstream-failure-diagnosis quality issue: a chunk worker that fails on frame 5 will have the frame-0 warmup's console lines polluting the "what happened around the failure" tail. Either snapshot+restore the buffer, or accept it and document. Marking important, not blocker — diagnostics-only impact.

Notes (not blocking)

  • CI: regression failure is the Docker buildx infra flake (error writing layer blob: ... resource_exhausted) — same root cause cascaded across PR 770/772 at the same time. Not a test signal. All required tests/typecheck/lint pass.

— Vai

Verdict: APPROVE
Reasoning: Wrapper restores the documented counters correctly; the browserConsoleBuffer gap is a diagnostics-quality concern worth a follow-up but doesn't gate. CI fail is buildx infra flake.

Base automatically changed from feat/producer-freeze-plan-runtime-env to main May 13, 2026 18:42
@jrusso1020 jrusso1020 dismissed stale reviews from vanceingalls and miguel-heygen May 13, 2026 18:42

The base branch was changed.

@jrusso1020 jrusso1020 merged commit 3408c3c into main May 13, 2026
68 of 76 checks passed
@jrusso1020 jrusso1020 deleted the feat/engine-discard-warmup-capture branch May 13, 2026 19:08
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.

3 participants