Repository navigation
fix(engine): keep beginframe when a passed probe's page close is slow - #6037
Conversation
jrusso1020
left a comment
There was a problem hiding this comment.
Reviewed exact head e76d7897e3f5838b1a4e04dd22af98463f6f2ca8; no prior reviews or inline findings to duplicate.
packages/engine/src/services/browserManager.ts:513preserves the capability result instead of treating page-close latency as a failed BeginFrame probe. The existing deadline, PNG validation and negative probe result remain intact.packages/engine/src/services/browserManager.test.ts:115reproduces the successful-PNG/never-settling-close case. Browser launch still falls back on a genuinely negative probe; worker teardown and browser lease ownership are unchanged.
Validation: independently executed the exact probe/helper source across successful and failed probes with resolving, rejecting, hanging and late-rejecting cleanup (eight scenarios). All retained the appropriate capability result; restoring the old return reproduced the false unsupported result. Also checked the pool close/drain and parallel-worker cleanup integration.
A fresh independent adversarial pass executed 36 exact-source failure/cleanup combinations without a classification or late-rejection failure. It also used real headless-shell with page close delayed 400 ms: the probe returned supported with the cleanup note after the 250 ms cleanup bound, the page subsequently closed, and the next real page produced a PNG. This validates the local boundary, not cloud render throughput.
No full repository test suite or cloud 4K render was run by me; the production reproduction and three author test runs are reported evidence.
Verdict: APPROVE
Reasoning: Cleanup latency no longer changes a measured capability, while real probe failures retain their fallback behavior. No blocker found in this focused change.
— hf-review
Edit accuracy: accurate 271 (base branch 271), smooth 140 of thoseThe gate passes. Quarantined, measured but not gated (0) Unstable (1)
|
What
A browser whose BeginFrame probe passed now stays in BeginFrame mode even when closing the probe page is slow. Before, a slow close turned a passing probe into a fallback to screenshot mode.
Why
The probe checks that
HeadlessExperimental.beginFramereturns a real PNG frame, then closes its page with a short timeout. When the machine is busy (several capture browsers starting while ffmpeg extracts video frames, as on a cloud render chunk), that close can take longer than the timeout even though the frame came back fine. The probe then reported "unsupported", that one browser fell back to screenshot mode, and a distributed chunk whose other browsers used BeginFrame failed its "all workers use one capture mode" check and had to be retried.A slow page close says nothing about whether BeginFrame works. If a browser really is broken, the capture itself fails and the existing screenshot retry handles it.
How
probeBeginFrameSupportkeeps the probe's own result when page cleanup times out and only appends "probe page cleanup timed out" to the detail. A probe that failed stays failed.Test plan
browserManager.test.ts: a probe whose PNG frame succeeds but whose page close never settles stays supported, with the cleanup note in its detail. Passed 3 runs in a row; fails with the old rule restored.