Skip to content

feat(cli): add --require-beginframe to fail instead of falling back to screenshot capture - #4585

Open
crmne wants to merge 3 commits into
heygen-com:mainfrom
crmne:fix/require-beginframe
Open

crmne wants to merge 3 commits into
heygen-com:mainfrom
crmne:fix/require-beginframe

Conversation

@crmne

@crmne crmne commented Sep 27, 2026

Copy link
Copy Markdown

What

hyperframes render --require-beginframe (env PRODUCER_REQUIRE_BEGINFRAME=true) fails the render when a capture session would not run BeginFrame, instead of continuing in screenshot capture. The error names the reason:

✗  Render failed

   BeginFrame capture is required, but the HeadlessExperimental.beginFrame probe failed after 2000ms (beginFrame probe timeout during warm-up beginFrame). Browsers starting together on one GPU can miss this deadline, so fewer --workers may help; not falling back to screenshot capture.

Without the flag nothing changes.

Why

A BeginFrame render can drop to screenshot capture and still exit 0. In #4584 every worker's probe times out once 6 workers start on one hardware GPU, the render takes twice as long, and only the summary line shows it. Scripts and CI that expect BeginFrame have no way to find out other than parsing that line. A render that should be fast or not happen at all can now say so.

Related work

Refs #4584. The cause there (a probe deadline shorter than a GPU start under contention) is a separate fix; this PR only makes the fallback detectable. #2810 made the effective capture mode visible for distributed chunks; this does the same for local renders by failing early.

How

  • EngineConfig.requireBeginFrame (default false), read from PRODUCER_REQUIRE_BEGINFRAME. The CLI flag sets that variable through the render plan's environment, as --low-memory-mode does.
  • createCaptureSession throws BeginFrameRequiredError when the session's capture mode resolves to screenshot: not Linux, no chrome-headless-shell, supersampling, transparent drawElement, a software GPU, or a caller that set forceScreenshot (alpha formats, render-mode hints, low-memory mode, the retry after a failed capture). This runs before any browser starts, so the producer's own screenshot retries also stop there.
  • launchBrowser throws the same error when the BeginFrame probe fails, instead of relaunching the browser without BeginFrame flags. requireBeginFrame is part of the launch fingerprint, so a pooled browser that fell back is never handed to a session that requires BeginFrame.
  • BeginFrameRequiredError is exported from @hyperframes/engine.

The flag does not change which capture mode a render asks for; it only refuses to continue in screenshot mode. It does not apply to --docker renders, which use SwiftShader.

Test plan

  • Unit tests added/updated

    • browserManager.test.ts: a failing probe relaunches in screenshot mode by default, and with requireBeginFrame rejects with BeginFrameRequiredError after a single launch (Linux only, like the launch path it covers). The second test fails without the change.
    • frameCapture-requireBeginFrame.test.ts: a session forced to screenshot capture, and one on a software GPU, are refused with their reason before Chrome launches.
    • config.test.ts: the environment variable; render/plan.test.ts: the flag maps to it.
  • Manual testing performed, on Linux with an RTX 3090 (--browser-gpu --gpu, chrome-headless-shell 152.0.7977.30, the 10 s composition from the issue):

    Command Exit Summary
    --workers 6 --require-beginframe 1 fails at "Starting browsers" with the error above, no output file
    --workers 3 --require-beginframe 0 beginframe capture · hardware gpu
    --workers 1 --require-beginframe 0 beginframe capture · hardware gpu
    --workers 6 0 screenshot capture · hardware gpu (unchanged)
  • Documentation updated: the flag table in docs/packages/cli.mdx

  • Comments follow CONTRIBUTING.md "Comments"

bun run lint, bun run format:check, bun run --filter '*' typecheck, fallow audit, check-comment-citations and comment-ratchet pass. The engine and CLI suites pass except 4 engine tests (videoFrameExtractor, ffprobe: FFmpeg 9 no longer accepts -vsync, and the HDR PNG fixtures) and 3 CLI tests (skillsManifest pruneOrphanedLockEntries), which fail the same way on main on this machine.

…o screenshot capture

When the BeginFrame probe fails, or a capture session is set to screenshot
capture, a render silently continues in screenshot mode and exits 0. With
--require-beginframe (or PRODUCER_REQUIRE_BEGINFRAME=true) the capture
session throws BeginFrameRequiredError instead, naming the reason, and no
screenshot browser is launched.

@miguel-heygen miguel-heygen 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 4833d83: complete diff, CLI plan/executor/Docker forwarding, engine capture-mode transitions, failed-probe cleanup and pooled-browser fingerprinting.

The engine path is solid in focused checks: screenshot preselection rejects, a failed probe closes Chrome and throws without relaunching, and the strict bit prevents reuse of a permissive screenshot-fallback browser. DrawElement fallback returns to immutable launchCaptureMode, so it preserves BeginFrame for a successfully strict-launched session.

Blocker — --docker silently discards the requirement. packages/cli/src/commands/render/plan.ts:385-390 accepts --require-beginframe with Docker and only sets the host environment. commands/render/execute.ts:129-175 does not carry it into RenderOptions, and utils/dockerRunArgs.ts neither forwards the flag/environment nor rejects the combination. Thus hyperframes render --docker --require-beginframe can run the screenshot path successfully, defeating the fail-closed assertion advertised in CLI help and docs. The PR body acknowledges Docker exclusion, but silent acceptance is still unsafe for scripts relying on this flag.

If Docker is intentionally unsupported, reject the effective requirement (flag or PRODUCER_REQUIRE_BEGINFRAME=true) with --docker before launching it. Otherwise forward/enforce it in-container. Pin the combination with a plan/execution regression test; no actual Docker execution was performed for this source-traced finding.

Independent validation:201 engine tests passed (config111, pool8, browserManager67, required-session2, reviewer strict/permissive pool witness1, freshFallback12). Mocked Puppeteer/CDP; no hardware-GPU or real BeginFrame render claim. Current CI is green. Temporary sources cleaned; no merge/release/deployment.

Verdict: REQUEST CHANGES
Reasoning: Local capture enforcement is well-covered, but an accepted Docker invocation bypasses the new requirement. Unsupported mode combinations must fail clearly rather than silently dropping it.

— Magi

Docker renders capture with screenshots on software GL, so the requirement
could never hold there. Refuse the combination, whether it comes from the flag
or PRODUCER_REQUIRE_BEGINFRAME=true, instead of dropping it silently.
@crmne

crmne commented Sep 27, 2026

Copy link
Copy Markdown
Author

Thanks, good catch. Docker renders always capture with screenshots on software GL, so the requirement can never hold there. 3c50439 now rejects --docker together with --require-beginframe or PRODUCER_REQUIRE_BEGINFRAME=true before anything launches, with regression tests for both the flag and the environment variable. The help text and CLI docs say so too.

@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.

Review at 3c5043943d05a9e785374748c04d369bc2ef849e.

Verdict: APPROVE

The one blocker in the earlier review at 4833d832 was that --docker silently dropped the requirement. It is fixed. 3c504394 rejects --docker together with --require-beginframe or PRODUCER_REQUIRE_BEGINFRAME=true in createRenderPlan, before anything launches, with a usage error that says why. Docker renders capture with screenshots on software GL, so rejecting the combination is the right choice over forwarding it.

Findings

Blocking: none.

Non-blocking: the branch conflicts with main in one place, the flag table in docs/packages/cli.mdx. The code files merge cleanly (git merge-tree against f361ee4b). It needs a rebase before it can land.

Earlier blocker, checked

  • Flag with --docker. The built CLI, run as render <dir> --docker --require-beginframe, exits 1 with "BeginFrame is local-only" and does not start a Docker build.
  • Env with --docker. PRODUCER_REQUIRE_BEGINFRAME=true render <dir> --docker exits 1 with the same error.
  • Control. render <dir> --docker without the requirement goes on to build the Docker image, so the check only fires when the requirement is set.
  • Explicit opt-out. --docker --no-require-beginframe with the env set to true is accepted and sets the variable to false, because the flag wins over the environment (args["require-beginframe"] ?? env). That matches how the plan maps the flag to the environment.
  • Same parsing as the engine. The CLI check treats only the string true as set, which is what the engine's envBool does (config.ts:809-813). So the CLI can't let through a value the engine would read as true.
  • One Docker switch. useDocker comes only from args.docker (plan.ts:456), and createRenderPlan is the only plan builder the render command calls, so there is no second Docker path that skips the check.
  • Regression tests. Disabling the check fails both new plan tests. Reverted.

Tests run

  • packages/cli: src/commands/render/plan.test.ts, 35 of 35 pass.
  • packages/engine: config.test.ts, browserManager.test.ts, frameCapture-requireBeginFrame.test.ts and browserLeasePool.test.ts, 188 of 188 pass.
  • The CLI runs above used the built dist/cli.js with a throwaway HOME and a one-second composition. No render was completed. I have no hardware GPU here, so the BeginFrame success path is covered only by the unit tests and the author's table.

Checks

Only the Graphite mergeability check and WIP have reported, at this head and at 4833d832. The repository's CI workflows have not run on this fork PR. They need a maintainer to approve the runs, and the docs conflict needs resolving first. So none of the 10 required checks has a result yet.

Gate

reviewDecision CHANGES_REQUESTED from the earlier review at 4833d832, mergeStateStatus DIRTY, mergeable CONFLICTING (the docs conflict above).

— Rames

@jrusso1020

Copy link
Copy Markdown
Collaborator

@crmne looks like a merge conflict needs to be addressed

This branch has not been deployed

No deployments
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.

3 participants