Skip to content

fix(engine): keep beginframe when a passed probe's page close is slow - #6037

Merged
miguel-heygen merged 1 commit into
mainfrom
cloudchunk/probe-cleanup-keeps-beginframe
Oct 21, 2026
Merged

miguel-heygen merged 1 commit into
mainfrom
cloudchunk/probe-cleanup-keeps-beginframe

Conversation

@miguel-heygen

Copy link
Copy Markdown
Collaborator

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.beginFrame returns 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

probeBeginFrameSupport keeps 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

  • Unit tests added/updated
    • 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.
  • Manual testing performed
    • Seen on a heavy 4K cloud render: one chunk failed with mixed capture modes after a passing probe whose page close timed out; that is the case the test reproduces.
  • Documentation updated (if applicable): not applicable.
  • Comments follow CONTRIBUTING.md "Comments"

@jrusso1020 jrusso1020 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed exact head e76d7897e3f5838b1a4e04dd22af98463f6f2ca8; no prior reviews or inline findings to duplicate.

  • packages/engine/src/services/browserManager.ts:513 preserves 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:115 reproduces 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

@github-actions

Copy link
Copy Markdown
Contributor

Edit accuracy: accurate 271 (base branch 271), smooth 140 of those

The gate passes.
Smoothness is reported in the artifact, not gated. A case fails only if it fails 2 of 3 runs.

Quarantined, measured but not gated (0)

Unstable (1)

  • seqplainkeys-none-px-r0-root-z100: tracking 0.06, pressJump 0, drop 0, reload 0, render 0.03, renderKey -, undo false, teleport true / tracking 0.06, pressJump 0, drop 0, reload 0, render 0.03, renderKey -, undo true, teleport true / tracking 0.06, pressJump 0, drop 0, reload 0, render 0.03, renderKey -, undo true, teleport true

@miguel-heygen
miguel-heygen added this pull request to the merge queue Oct 21, 2026
Merged via the queue into main with commit a8b576d Oct 21, 2026
74 checks passed
@miguel-heygen
miguel-heygen deleted the cloudchunk/probe-cleanup-keeps-beginframe branch October 21, 2026 16:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants