fix(render): retry drawElement failures on a fresh screenshot page - #3805
fix(render): retry drawElement failures on a fresh screenshot page#3805vanceingalls wants to merge 2 commits into
Conversation
miguel-heygen
left a comment
There was a problem hiding this comment.
The retry architecture reads coherently at d71ad474: all three drawElement entry points stop returning screenshot pixels from the injected-canvas page, the structural error survives the producer wrappers, cancellation and encoder interruption still short-circuit, and both streaming and disk recovery create a force-screenshot attempt. Disk capture also refuses to let a complete-looking prefix mask the untrusted frame set, and the batch prefix promises are observed before the page or worker is torn down.
I found one non-blocking diagnostics mismatch. In captureFrameToBufferPipelined, the new recoverable-error branch throws at frameCapture.ts:3760-3768 before the Mirror captureFrameCore call to captureFrameErrorDiagnostics at :3771-3781. The serial path reaches its outer diagnostics catch, but the ordinary pipelined NCPR/canvas path does not emit the promised frame-error PNG/HTML/JSON/console bundle. That does not make the fresh-page retry unsafe, and it is not a regression from the old per-frame fallback, but either move the diagnostic call ahead of the wrapper throw or narrow the comment/body claim.
I am not stamping a draft over its own unmet merge gate. The current tests prove routing and rejection with mocks; they do not provide the real-Chrome pixel evidence the PR body requires for serial, worker, batch, hard-cut, and induced paint-record failure. Keep the draft until those comparisons exist, especially because a false-positive tiny-JPEG trip now converts one-frame fallback into a full recapture.
Verdict: COMMENT
Reasoning: The implementation has no code-level correctness blocker in the reviewed diff, but its explicit real-browser merge gate remains unmet; one pipelined diagnostics branch should also be reconciled before ready-for-review.
— Magi
…wrapper Review feedback on #3805. captureFrameToBufferPipelined checked isRecoverableDrawElementError and threw DrawElementCaptureError before reaching captureFrameErrorDiagnostics, so the NCPR/canvas failures that now abort the whole attempt produced no frame-error PNG/HTML/JSON bundle — the exact case worth debugging, and the one the adjacent comment promised mirrored the serial path. The serial path was unaffected because its own DrawElementCaptureError throw propagates through captureFrameCore's outer diagnostics catch. Run diagnostics first, then the recoverable wrapper. Bounded to at most one bundle per attempt, since a recoverable error fails the whole attempt, and captureFrameErrorDiagnostics self-catches, so a dead page cannot mask the structural error the producer's fresh-page retry depends on. Covered by a new test asserting the bundle lands for a recoverable pipelined failure; verified by mutation (restoring the old order fails it). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
d71ad47 to
4c7fbde
Compare
Real-Chrome pixel gate: run, and the defect reproducesThe merge gate this PR set for itself is now met. Browser launch was sandbox-blocked on the investigation host; it works on a macOS/hardware-GPU host, so I ran the comparisons there. Pushed alongside this: Setup. macOS arm64, M4 Pro, Chrome 152.0.7977.30, ANGLE/Metal hardware GPU. Purpose-built composition: 1920x1080 @30fps, 6s / 180 frames, three clips with hard cuts at t=2.0s and t=4.0s (11 clip-boundary frames), high-frequency texture so JPEGs clear the tiny-frame floor, no filter/blend/video/3D/webfont. Baseline is the same composition with 1. Clean parity, all three capture paths
67 dB is JPEG-vs-screenshot encode noise. Screenshot baseline: 12.4s. 2. Induced missing paint record — branch vs clean
|
| run | fresh-page retry | frames < 30 dB | worst |
|---|---|---|---|
| serial, branch | yes | 0/180 | bit-identical |
| pipelined, branch | yes | 0/180 | bit-identical |
pipelined, main |
no | 1 (idx 60) | 6.2 dB |
| batch, branch | yes | 0/180 | bit-identical |
batch, main |
no | 4 (idx 60-63) | 13.2 dB |
The stale-surface defect reproduces on main. Its frame 60 scores 77.2 dB against baseline frame 59, and only 6.2 dB against its own correct frame 60 — the same-page screenshot fallback returned the previous frame's canvas bitmap. Because the injection sits on a hard cut, that renders scene ALPHA (red) where scene BRAVO (blue) belongs. On the batch path the whole four-frame batch is corrupted. This branch recovers to bit-identical pixels on all three paths.
Main-side control was run for pipelined and batch, not serial; the serial mechanism is the same same-page fallback surface.
3. Native boundary-screenshot trigger
HF_FAST_CAPTURE_BOUNDARY_SS=true throws DrawElementCaptureError at clip boundaries without any patch: one fresh-page retry, output bit-identical to baseline, zero unhandled promise rejections — which exercises the abandoned-batch-encode drain outside the mocks.
Retry cost
13.5s with one full retry vs 8.3s clean on this 180-frame composition, +63%. That is the cost trade this PR's description flags; it is real and worth stating in the merge decision rather than discovering in the fleet.
What this does not show
macOS / hardware-GPU only. The original Windows compositor trigger is still not reproduced — this exercises the recovery path by injection, not that trigger. The 75% diagnosis confidence in the description stands.
Frame-60 PNG pair (baseline / main / branch) and the full harness are available; happy to attach or re-run any variant.
After drawElement injects its canvas, a same-page screenshot can capture the previous canvas bitmap instead of the newly sought composition frame. Recoverable paint-record/canvas errors, tiny-frame guards, and explicit boundary screenshot fallbacks currently use that unsafe surface.
Reject those captures with a structural
DrawElementCaptureErrorand route the whole attempt through the existing fresh screenshot-session retry. Cover serial, pipelined, and batch paths; consume abandoned batch encode rejections during cleanup. Disk capture propagates the error even when frame files appear complete, clears the untrusted attempt, and forces screenshot capture. Keep cancellation/encoder-interruption exclusions and reportcapture_errorseparately from PSNR/blank verification failures.Related: PRINFRA-600.
Merge gate: met — and the defect is now confirmed, not inferred
The draft gate asked for real Chrome pixel comparisons across serial, worker and batch capture, including a hard scene cut and an induced missing paint record. Browser launch was sandbox-blocked on the investigation host; it works on macOS/hardware-GPU, so the comparisons ran there. Full method, tables and caveats in this comment.
Headline: with the NCPR error injected at the first frame after a hard cut,
mainemits the previous frame's canvas bitmap — its frame 60 scores 77.2 dB against baseline frame 59 and only 6.2 dB against its own correct frame 60 (the batch path corrupts all four frames of the batch, 13.2 dB). This branch returns bit-identical pixels on all three paths. Without injection, all three paths sit at 67.1 dB min with 0/180 frames below the 30 dB damage criterion.So the original 75% diagnosis confidence understated one half and not the other: the stale-surface recovery defect is now reproduced and pixel-confirmed, while the original Windows compositor trigger is still not reproduced. This PR fixes the former and does not claim the latter.
Costs and risks this accepts
Validation
frameCapture-freshFallback.test.ts: 12 tests, covering that no same-page screenshot pixels are returned, pending batch prefixes reject safely, explicit boundary fallback is fresh-page-only, and complete disk files cannot mask an untrusted capture.Rebased onto
main(therenderOrchestrator.test.tsconflict was an import union).4c7fbde5addresses @miguel-heygen's diagnostics finding:captureFrameErrorDiagnosticsnow runs before the recoverable-error wrapper incaptureFrameToBufferPipelined, so NCPR/canvas failures emit the frame-error bundle like the serial path. It is bounded to one bundle per attempt and self-catches, so a dead page cannot mask the structural error the retry depends on. Covered by a test verified by mutation — restoring the old ordering fails it.