Conversation
…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
left a comment
There was a problem hiding this comment.
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.
|
Thanks, good catch. Docker renders always capture with screenshots on software GL, so the requirement can never hold there. 3c50439 now rejects |
jrusso1020
left a comment
There was a problem hiding this comment.
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 asrender <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> --dockerexits 1 with the same error. - Control.
render <dir> --dockerwithout 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-beginframewith the env set totrueis accepted and sets the variable tofalse, 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
trueas set, which is what the engine'senvBooldoes (config.ts:809-813). So the CLI can't let through a value the engine would read astrue. - One Docker switch.
useDockercomes only fromargs.docker(plan.ts:456), andcreateRenderPlanis 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.tsandbrowserLeasePool.test.ts, 188 of 188 pass.- The CLI runs above used the built
dist/cli.jswith a throwawayHOMEand 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
|
@crmne looks like a merge conflict needs to be addressed |
# Conflicts: # docs/packages/cli.mdx
What
hyperframes render --require-beginframe(envPRODUCER_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: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(defaultfalse), read fromPRODUCER_REQUIRE_BEGINFRAME. The CLI flag sets that variable through the render plan's environment, as--low-memory-modedoes.createCaptureSessionthrowsBeginFrameRequiredErrorwhen the session's capture mode resolves to screenshot: not Linux, no chrome-headless-shell, supersampling, transparent drawElement, a software GPU, or a caller that setforceScreenshot(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.launchBrowserthrows the same error when the BeginFrame probe fails, instead of relaunching the browser without BeginFrame flags.requireBeginFrameis part of the launch fingerprint, so a pooled browser that fell back is never handed to a session that requires BeginFrame.BeginFrameRequiredErroris 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
--dockerrenders, which use SwiftShader.Test plan
Unit tests added/updated
browserManager.test.ts: a failing probe relaunches in screenshot mode by default, and withrequireBeginFramerejects withBeginFrameRequiredErrorafter 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):--workers 6 --require-beginframe--workers 3 --require-beginframebeginframe capture · hardware gpu--workers 1 --require-beginframebeginframe capture · hardware gpu--workers 6screenshot capture · hardware gpu(unchanged)Documentation updated: the flag table in
docs/packages/cli.mdxComments follow CONTRIBUTING.md "Comments"
bun run lint,bun run format:check,bun run --filter '*' typecheck,fallow audit,check-comment-citationsandcomment-ratchetpass. 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 (skillsManifestpruneOrphanedLockEntries), which fail the same way onmainon this machine.