Repository navigation
fix(producer): a BeginFrame capture failure re-renders on a screenshot session - #7070
Merged
Merged
Conversation
Contributor
There was a problem hiding this comment.
Approve at 6b2dcfed. No blocker found.
Verified
- The body names the test that fails without the fix:
renderOrchestrator.beginFrameRetry.test.ts. I checked it against main'srenderOrchestrator.ts. On main, "re-renders a BeginFrame attempt…" and "does not retry a second failure…" both fail: one attempt, then the render rejects with the first capture error. All 3 pass at this head. renderOrchestrator.test.ts,.stall,.segmentedDeRetryand.beginFrameRetrypass: 314/314.- The retry is bounded. The re-render runs inside the
catch, so a second failure propagates.replanAfterFailuresetsforceScreenshot: trueon everysdr_streamingbranch, so the restarted attempt evaluatesisBeginFrameCaptureas false. isBeginFrameCapturematches the attempt's own label predicate (!plan.forceScreenshot && !useDrawElement && parallelCaptureMode === "beginframe"). It is checked after the cancellation and encoder-interrupted guards, so those still never restart.
Non-blocking notes
- The scope is the streaming route only, as the body says. A BeginFrame failure on the plain
sdr_diskcapture path still fails the render: the diskcatchonly retries drawElement errors. Routes that go there include webm, png sequences, and BeginFrame with the stream router off. Worth a follow-up if "any BeginFrame failure costs one re-render" is meant to hold everywhere. - "Whatever the failure was" includes failures that will happen again no matter the capture mode, such as a composition that throws, or ENOSPC. Those now take one extra full screenshot render before failing. That is a reasonable trade for a single bounded retry; just noting the cost.
resolveParallelCaptureModecannot see the engine's software-GPU clamp (its own docstring says so). A render that the clamp moved to screenshot is still labelled BeginFrame and gets one extra screenshot retry. This is harmless.
Contributor
Edit accuracy: accurate 267 (base branch 267), smooth 106 of thoseThe gate passes. Quarantined, measured but not gated (8)
Unstable (1)
|
Merged
1 task done
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
When a render's streaming capture fails in BeginFrame mode, the render restarts once on a fresh screenshot session, whatever the failure was. Before, only stalls, encoder death, transient browser errors and drawElement failures restarted; any other BeginFrame capture failure failed the render.
Why
On Linux with chrome-headless-shell, BeginFrame capture is noticeably faster than screenshot capture, and hosts that opt into it (
PRODUCER_FORCE_SCREENSHOT=false) want that speed without a new way to fail. Screenshot capture is the baseline every other fallback already returns to, so a BeginFrame-only failure should cost one re-render, not the render.Related work
Refs #7066 (a render request can force screenshot capture). Together they let a caller retry in screenshot mode, and make the engine do the same inside one attempt.
How
shouldRetryViaPinnedFallbacktakesisBeginFrameCapture: true means the failed attempt captured through BeginFrame, and that alone allows the restart. Cancellation and encoder interruption still never restart.resolveParallelCaptureModeresolvedbeginframefor this render.replanAfterFailure, which always setsforceScreenshot: true, so the restarted attempt is not BeginFrame and a second failure is final.Scope: the streaming route. Distributed chunks already have their own whole-chunk screenshot retry (
runCaptureWithScreenshotFallback).Test plan
renderOrchestrator.beginFrameRetry.test.tsrunsexecuteRenderJobwith the streaming stage scripted per attempt.forceScreenshot: true. Fails without the fix: the render rejects with the capture error after one attempt.isBeginFrameCapturebranch, or passingfalseat the call site, each fail the two restart tests.renderOrchestrator.test.ts(295),renderOrchestrator.stall.test.ts(9),renderOrchestrator.segmentedDeRetry.test.ts(7) pass; new file passes three runs in a row;tsc --noEmit, oxlint, oxfmt, comment ratchet clean.PRODUCER_FORCE_SCREENSHOT=false, a composition that froze the page tripped the BeginFrame watchdog and the engine restarted it on a screenshot session within the same render.