Skip to content

fix(producer): render telemetry reports beyond-viewport capture as the workers used it - #6702

Merged
miguel-heygen merged 3 commits into
mainfrom
fix/telemetry-beyond-viewport-used
Oct 31, 2026
Merged

miguel-heygen merged 3 commits into
mainfrom
fix/telemetry-beyond-viewport-used

Conversation

@miguel-heygen

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

Copy link
Copy Markdown
Collaborator

Description

Render telemetry can say a render captured beyond the viewport when none of its frames did. A render with a native video asks for beyond-viewport capture, and each capture session turns that request off at init when the page fits the frame. On a parallel render, though, the value reported after capture came from the probe session, not from the workers that captured. A 4-worker render of a page that fits reported captureBeyondViewport: true, while all four workers logged captureBeyondViewport downgraded and every screenshot was taken with it off. That makes the field useless for telling whether renders pay for beyond-viewport capture.

Each session's perf summary now carries the value it captured with. Every multi-worker capture path (streaming, disk and segmented) already collects those summaries, and the render's observability summary already merges per-worker facts from them. That summary, on success and on failure alike, now applies the sessions' value: on if any session kept it. When no session reported one, the value recorded during the render stands, as before. Doing it at the summary covers every capture path, including any added later.

The capture_strategy checkpoint is unchanged. It records the plan before any session starts, so it still shows the request.

Testing

fails without the fix: getCapturePerfSummary > reports the beyond-viewport value the session captured with (engine) and the capturedBeyondViewport and withCapturedBeyondViewport tests in perfSummary.test.ts (producer) fail on main.

  • frameCapture-discardWarmup.test.ts and perfSummary.test.ts: 10 and 12 passed, 3 runs in a row.
  • tsc --noEmit for engine and producer: clean. Pre-commit (lint, format, fallow, typecheck): clean.

What I did NOT exercise

No real render on this branch. The before state (trace true, four downgrades, every screenshot with it off) comes from a 4-worker render on a Linux box on main.

Why under 100 lines

The workers already report a perf summary to the orchestrator on both parallel paths; this adds one field to it and applies it at the two places the summary is built.

Size

A lone fix: no open PR of ours touches this code, so it ships on its own.

@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: REQUEST CHANGES · 1 blocker · 0 important · 0 nits · head 276e3cf2

  • blocker The segmented path still reports the requested value, not the one the workers used. In renderOrchestrator.ts:4899-4904 the segmentedRes.success branch never updates captureBeyondViewport. So observability keeps resolvedCaptureBeyondViewport, which :3661 set to the video request. Segmented workers already push their summaries into dedupPerfs (captureSegmentedStage.ts:435, :611), and those summaries now carry the field. Repro: a long-form render with a native video, on a page that fits, planned as sdr_segmented, still reports captureBeyondViewport: true even though every worker logged downgraded. That is the same bug on a third multi-worker path. Fix: after updateCaptureObservability({ workerCount }), set the field from capturedBeyondViewport(dedupPerfs, undefined) when it isn't undefined, and add a test that covers it.
Scope

Read the full diff, the three orchestrator capture branches, the stages that push into dedupPerfs, and the init downgrade (frameCapture.ts:2951). Ran it locally: the 4 named tests pass at head, and the engine test plus the 3 capturedBeyondViewport tests fail with src reverted to main, so the 'fails without the fix' line holds. CI was still in progress when I posted this. Retry accumulation in dedupPerfs on the streaming path predates this PR, and the downgrade depends only on the page, so attempts agree. Not flagged.

@miguel-heygen

Copy link
Copy Markdown
Collaborator Author

Fixed in 95994ee, at the root rather than per branch: the observability summary (built once on success and once on failure) now applies the sessions' value from dedupPerfs, the same place it already merges per-worker init facts. That covers the streaming, disk and segmented paths, and any capture path added later, so no branch can miss it again. The per-branch changes from the first head are reverted. withCapturedBeyondViewport has its own tests, red on main.

@github-actions

github-actions Bot commented Oct 29, 2026 •

Copy link
Copy Markdown
Contributor

Edit accuracy: accurate 271 (base branch 271), smooth 114 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 (2)

  • seqplainkeys-none-center-r0-nested-z100: 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 / tracking 0.06, pressJump 0, drop 0, reload 0, render 0.03, renderKey -, undo true, teleport true
  • seqplainkeys-none-px-r0-nested-z100: tracking 0.07, pressJump 0, drop 0, reload 0, render 0.03, renderKey -, undo true, teleport true / tracking 0.07, pressJump 0, drop 0, reload 0, render 0.03, renderKey -, undo true, teleport true / tracking 0.07, 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 · 1 resolved, 0 open, 0 new · 276e3cf → 95994ee

The segmented-path blocker is fixed: withCapturedBeyondViewport is applied in both observability.summary calls (success and failure), and those are the only places captureObservability is read, so it covers every capture path. Tests pass 12 + 10 locally. Required CI is green after a rerun: the Windows engine-cli lane hit its 16-minute deadline with every engine test passing. Comments fails, but it isn't required.

@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 resolved, 0 open, 0 new · 95994ee → b9d040c. The only change is one doc comment removed from types.ts. All CI is green.

@miguel-heygen
miguel-heygen force-pushed the fix/telemetry-beyond-viewport-used branch from b9d040c to aa7ca08 Compare October 31, 2026 21:00

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

Approve. No blockers at aa7ca084.

  • Fails without the fix: checked locally. At this head, frameCapture-discardWarmup.test.ts passes 10/10. I put engine/src/services/frameCapture.ts back to the merge-base version and getCapturePerfSummary > reports the beyond-viewport value the session captured with fails with expected undefined to be false.
  • Where the summaries come from:
    • Every summary in dedupPerfs comes from a session that captured frames: the parallel workers through parallelCoordinator and pushWorkerDedupPerfs, the segmented workers, and the sequential or streaming session.
    • The probe session's summary is never added, so a probe that kept it on can't make the result read on.
    • "On if any session kept it" is the correct way to combine them.
  • When no session reported: if no summary carries the field, withCapturedBeyondViewport returns the capture facts unchanged. Renders that report nothing look the same as before.
  • Both summaries: the success and failure observability summaries both apply it. The capture_strategy checkpoint still records the plan, as the body says.
  • CI at review time: preview-regression, Player perf, PR captures and Comments pass. CI, regression, Windows render verification and CodeQL were still running.

@miguel-heygen
miguel-heygen added this pull request to the merge queue Oct 31, 2026
Merged via the queue into main with commit bb3c3b2 Oct 31, 2026
140 of 142 checks passed
@miguel-heygen
miguel-heygen deleted the fix/telemetry-beyond-viewport-used branch October 31, 2026 21:58
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