Repository navigation
fix(core): a stuck seek in check or snapshot fails named before the protocol timeout - #6596
miguel-heygen wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
Verdict: REQUEST CHANGES · 1 blocker · 0 important · 0 nits · head c74bb142
The 30 s default also applies to export renders, not just check/snapshot. That turns renders that are slow but valid into failures. Details inline.
Scope and checks
- Read all 7 files. Also checked every caller of
__hfWaitForSeekCompletion: engineframeCapture/drawElementService/riskFrames/videoFrameInjector, clicaptureCompositionFrame/checkBrowser/motionShot, and studiouseShadowPreviewReload. None of them passtimeoutMs. - The producer's virtual-time shim freezes
Date.nowwhile the barrier waits, sodeadline - Date.now()still works out to 30 s of real time. That part is fine. - Ran locally on the head:
seek-dispatch,init.seekOrderandframeSourcestests (35 pass); coretscis clean. CI was still running when I posted.
| (completion) => options?.videoFrames !== false || !_videoFrameCompletions.has(completion), | ||
| ([, hold]) => options?.videoFrames !== false || hold !== "video-frame", | ||
| ); | ||
| const timeoutMs = options?.timeoutMs ?? SEEK_SETTLE_CAP_MS; |
There was a problem hiding this comment.
blocker: every engine/producer render goes through this default. waitForPendingSeekCompletion (engine frameCapture.ts:3190) calls __hfWaitForSeekCompletion() with no options. The 180 s in the PR body is the CLI's Puppeteer default (captureTimeout.ts). Renders run under protocolTimeout, which is 300 s by default, scales with canvas area up to 30 min (scaleProtocolTimeoutForComposition exists because slow-but-valid seek/paint calls on large canvases were being killed), and can be raised by the user.
So with this change, a render that used to finish now fails after 30 s, with no way to raise the limit. Example: a frame source whose ready loads a heavy model or scene for 45 s on a cold worker. docs/guides/frame-sources.md says "ready holds initialization and export capture" and documents no limit. The same goes for a slow software-GL vfx/motion-blur paint at 4K.
Fix: have the engine pass timeoutMs derived from its effective protocol timeout, minus a margin so the named error still wins the race. Keep 30 s, or the CLI's budget, for check/snapshot. If 30 s is meant to be the new contract for renders too, document it in the frame-sources guide and make it configurable.
There was a problem hiding this comment.
Agreed, fixed in ce1d96b. The core wait no longer has a default limit: without timeoutMs it waits as before, so renders keep their scaled protocolTimeout. Only the CLI's check grid and settleCompositionSeek (snapshot) pass a limit, 170 s, 10 s under the 180 s protocol timeout their browser runs with. Anything that finished before still finishes; a seek that hit the opaque 180 s timeout now fails at 170 s and names the stuck hold.
There was a problem hiding this comment.
✅ Fixed in ce1d96b. The core wait has no default limit anymore, so renders keep their scaled protocolTimeout. Only check and snapshot pass 170 s.
Edit accuracy: accurate 271 (base branch 271), smooth 143 of thoseThe gate passes. Quarantined, measured but not gated (0) Unstable (2)
|
… end it before their timeout
What changes
A seek waits for everything it started (video frames, frame sources, scene windows, motion blur, effects, image decodes) before a frame is captured. If one of those holds never settled,
checkandsnapshotsat until Puppeteer's protocol timeout (180 s) fired, with a message that said nothing about why.Now the caller can bound the wait, and the error names what is still pending:
__hfWaitForSeekCompletion({ timeoutMs })ends at the caller's limit. WithouttimeoutMsit waits as before, so export renders keep the limits they already had:protocolTimeout, scaled by canvas size and settable by the user.check(the overlap grid) andsnapshot/settleCompositionSeekpassSEEK_SETTLES_BEFORE_PROTOCOL_TIMEOUT_MS, which is 170 s, 10 s under the protocol timeout their browser runs with. A seek that used to finish still finishes; one that used to die at 180 s with an opaque timeout now fails at 170 s, named. The grid runs all its seeks in one page call, so it spends one 170 s budget across all of them.registerSeekCompletion(promise, hold)), so the error can say which one is stuck.What it does not fix
Tests
seek-dispatch.test.ts:timeoutMs, a seek waits as long as its holds take (fake timers, 30 min).The first two fail on main, where the wait had no deadline. The CLI capture tests now expect the limit to be passed.
Results on Linux: