Repository navigation
fix(engine): a render stall names the step Chrome hung in and says what to do - #5450
Conversation
Edit accuracy: accurate 2061 (base branch 2061), smooth 1584 of thoseThe gate passes. Quarantined, measured but not gated (0) Unstable (3)
|
terencecho
left a comment
There was a problem hiding this comment.
Reviewed f7d5adb1d65524103f62d54152d65ca5e2a2c21a against API/effective base 8bca7c4417b59b47fca851183a8d47a4565bad0c: five modified files, +90/−15, no removed or renamed paths. No substantiated blocker found.
Strengths
packages/engine/src/services/frameCapture.ts:2870–2929assigns the phase before each awaited preparation boundary. Motion-blur seek/injection/restore (:4210,4217,4260) and batched capture (:4627) overwrite the preceding frame's label rather than inheriting it.packages/producer/src/services/render/stages/captureStreamingStage.ts:166–229reads the live session when the deadline fires. Streaming, pipelined/batch and segmented callers all pass that session; typed error names, mode prefixes and parallel variants still match the orchestrator's classifiers (renderOrchestrator.ts:2447–2468). Successful and ordinarily rejected work still disarms the timer; cancellation-at-trip remains an abort rather than a stall.
Notes / diagnostic scope
captureStreamingStage.ts:96–98offers conditional GPU/CPU-contention advice. The guard establishes lack of capture progress, not that another application or Chrome itself is necessarily the cause: injection hooks can await non-Chrome work. Batch internals retain the coarse “capturing the frame” label (frameCapture.ts:4627), and secondary error-diagnostic screenshot/content awaits retain the preceding phase (:2769–2803). These are precision limits, not reproduced new failures.- This is diagnostic instrumentation, not a new end-to-end timeout or cancellation guarantee. Startup, encoding, shutdown and cleanup budgets are not changed; the raced native operation is not cancelled. In particular, existing close-time sampling can precede the bounded page/browser close races. I am not approving a universal “no hang” or GPU-causation claim.
Audited: both independent source/conventions and adversarial code passes are complete. The source pass read all five complete effective-base files before the diff, all full head producer/test files, and every head-engine insertion/integration path; byte-equality mapping confirms complete head-engine textual coverage from base plus inserted spans. The adversarial pass separately read all five full head files, including the 5,083-line engine, and relevant legacy/watchdog/capture/retry/cleanup paths. I directly inspected the complete diff, histories, phase/watchdog/classifier source boundaries, parity evidence and both scratch drivers/results. Git/API file identities and decoded API/Git/worktree bytes agree. Histories were exhausted: no prior reviews or inline comments; the sole edit-accuracy bot comment is not substantive code-review clearance.
Validation / Trusting: independent reviewers executed 27 grouped base/head phase, rejection, motion-blur, batch, CSS/LUT and classifier controls, plus ten watchdog controls; all passed. I inspected those executions, rather than claiming a second parent run. They use source-extracted bodies, explicit type erasure, fake timers and inert page/helper promises with Node builtins—no repository imports or renderer/browser integration. An initial adversarial scratch run allowed only 24 microtask drains, insufficient to reach the final screenshot through the extracted async preparation/helper chain. The rerun allowed 200 drains and asserted the intended blocked gate was actually reached before manually firing the fake watchdog. That is a scratch scheduling correction, not a repository fix or a real-time/browser scheduling guarantee. Repository suites, typecheck/build/lint/format, actual Puppeteer/vendor execution, native codecs, browser/Electron, GPU contention and performance were not exercised. Lock/browser launch configuration was inspected, not upstream vendor internals. CI was not a verdict gate and I am not asserting all checks passed.
All checked-in workflow trigger blocks were inspected; none is approval-triggered. This does not establish external merge-App behavior: approval may clear the requested merge prerequisite. No workflow/build/CI rerun, source fix, install, live rendering, release/deploy or direct merge was performed or authorized by this review.
— tai
Verdict: APPROVE
Reasoning: The new phase-at-trip diagnostics preserve the inspected watchdog and error-classification behavior in source and bounded offline controls. No introduced blocking defect was substantiated; the diagnostic and execution limits above remain explicit.
What
When a render stops making progress for 60 s, the error now names the step Chrome was stuck in and tells the user what to do:
Why
A user reported a render that stalled at frame 165, stalled again on the fallback at frame 233, and exited with no video. A heavy GPU render (Blender Cycles) was running at the same time; running the jobs one at a time fixed it. The old error only said "no frame progress for 60000ms", so neither the user nor we could tell which wait hung, and nothing pointed at the other GPU job.
Measured on an Apple Silicon Mac (M4 Pro) with a 1920x1080 image + canvas + GSAP composition (screenshot capture, one worker), using per-step timing marks:
Page.captureScreenshot, 3.1 s on average and 4.0 s at most per frame. Seeking, CSS background decode and page seek handlers never went over 1.5 s. A run with lighter load took 556 s.So under GPU contention the time goes into the one step that needs the GPU to finish and read back a frame, and it grows with how heavy the other GPU job is. A fresh-session retry does not help while that job keeps running, which matches the report (the fallback stalled too).
Related work
None.
How
CaptureSession.captureStep(engine) is set before each per-frame wait: seek, CSS background decode, video frame injection, page seek handlers, LUT load, page compositing, and the frame capture itself (screenshot, BeginFrame or drawElement, including the pipelined and batched drawElement paths). The values are plain phrases, so the message needs no lookup table.captureAccumulatedFrame) seeks and injects outsideprepareFrameForCapture, so it sets the step itself; otherwise a hang there would report the previous frame's step.raceAgainstStallnow takes the session instead of only its capture mode and reads the step when the watchdog trips. The message prefix is unchanged, soisSequentialCaptureStallErrorand the one fresh-session retry behave as before.Test plan
captureStreamingStage.test.tschecks that a wedged screenshot capture reports(stuck at frame 0/10 while capturing the frame). It fails without the producer change (the message ends at(stuck at frame 0/10).) and passes 3 runs in a row with it. The engine and producer typecheck, and oxlint and oxfmt are clean.frameCapture-motionBlur.test.tschecks the step at each seek and video injection, on a frame with motion blur (frame seek, injection, restore seek) and without, starting from a stale step left by the previous frame. Both fail on main and pass with this change (40/40, 3 runs).HF_DE_STALL_MS=2500. Both attempts stalled and the render failed with... (stuck at frame 0/432 while capturing the frame). Chrome stopped answering. If another app is keeping the GPU or CPU busy .... The stall-trace telemetry message carries the same text.