Repository navigation
fix(engine): renders use the browser the CLI picked; Linux system Chrome is not beginframe - #5987
Conversation
Edit accuracy: accurate 271 (base branch 271), smooth 153 of thoseThe gate passes. Quarantined, measured but not gated (0) |
vanceingalls
left a comment
vanceingalls
left a comment
There was a problem hiding this comment.
Reviewed at 85f45309. There are no other reviews yet.
What holds
- The capture-mode fix is right.
resolveHeadlessShellPathreturnsstring | undefined, so!== nullwas always true.resolveParallelCaptureModenow uses the same truthy rule the engine's own launch uses (browserManager.ts:814-827), so the router and the launch agree. browserPathFromEnvalso skips an emptyHYPERFRAMES_BROWSER_PATH="", which the old??infindFromEnvdid not.
Blocker 1: a CLI render still launches PRODUCER_HEADLESS_SHELL_PATH when both are set
resolveConfig fills chromePath from PRODUCER_HEADLESS_SHELL_PATH (engine/src/config.ts:855). resolveHeadlessShellPath returns config.chromePath before it reaches browserPathFromEnv (browserManager.ts:252). The CLI render builds its config with producer.resolveConfig(…) (cli/src/commands/render.ts:1081), and a render with no producerConfig does the same (renderOrchestrator.ts:2872). So the render's resolveHeadlessShellPath(cfg) at :3452 and :4073 still gets the PRODUCER_ path.
Repro at this head, with both vars pointing at different existing files:
resolveHeadlessShellPath({})returnsHYPERFRAMES_BROWSER_PATH's file.resolveHeadlessShellPath(resolveConfig())returnsPRODUCER_HEADLESS_SHELL_PATH's file, becausecfg.chromePathis thePRODUCER_path.
The new test passes {}, so it never sees the config layer. The preflight-vs-render split this PR targets is still there for CLI renders. Two ways to fix it: resolve chromePath in config.ts through browserPathFromEnv()?.path, or drop the env read there and let resolveHeadlessShellPath own it. Either way, a test through resolveConfig() would pin it.
Blocker 2: required Tests on windows-latest: engine-cli is red, caused by this change
browserManager.test.ts:828 ("throws a clear error when PRODUCER_HEADLESS_SHELL_PATH points at a missing binary") fails on Windows. That job exports HYPERFRAMES_BROWSER_PATH (windows-render.yml:509), and the test doesn't clear it. With the new order, the existing HYPERFRAMES_ file wins, so nothing throws (job). Locally, with HYPERFRAMES_BROWSER_PATH set to an existing file, it is 1 failed / 90 passed, and 91/91 pass without it. Adding delete process.env.HYPERFRAMES_BROWSER_PATH to that test fixes it.
That failure also shows a behaviour change the description doesn't mention: a valid HYPERFRAMES_BROWSER_PATH now silently hides a broken PRODUCER_HEADLESS_SHELL_PATH, which used to throw. That's arguably intended by "HYPERFRAMES first", but worth a line in the description.
Verdict: REQUEST CHANGES
Reasoning: The precedence fix doesn't reach a CLI render, because resolveConfig pins chromePath to PRODUCER_HEADLESS_SHELL_PATH first. A required Windows check also fails on a test the new order breaks. The capture-mode fix itself is correct.
— Via
…he engine config's
|
Both fixed at c56e02e:
|
vanceingalls
left a comment
vanceingalls
left a comment
There was a problem hiding this comment.
Re-reviewed at c56e02e9, covering the delta from 85f45309 (config.ts and browserManager.test.ts).
Both blockers are closed
- The config layer no longer pins the browser.
resolveConfigno longer readsPRODUCER_HEADLESS_SHELL_PATHintochromePath, so a CLI render'sresolveHeadlessShellPath(cfg)reachesbrowserPathFromEnvand getsHYPERFRAMES_BROWSER_PATHfirst. Nothing else readcfg.chromePathfor the env value. Its remaining readers areresolveHeadlessShellPathitself (browserManager.ts:252) and callers that pass an already-resolved path (browserManager.ts:632,frameCapture.ts:1293,renderOrchestrator.ts:3452). The new test goes throughresolveConfig(). Putting the removed line back turns it red (1 failed / 91 passed). - The Windows env leak is fixed. The suite's
beforeEachclears both vars. Locally, with both vars exported as the Windows job does,browserManager.test.tsandconfig.test.tspass 205/205.
Nit: the resolveHeadlessShellPath docstring (browserManager.ts:246-247) still says "config.chromePath, then PRODUCER_HEADLESS_SHELL_PATH env var, then the CLI browser override". That order is now HYPERFRAMES_BROWSER_PATH first.
CI: every required check is green at c56e02e9, including Tests on windows-latest and its engine-cli job (CI run 39528131578, Windows run 39528131597). A second pull_request run at the same head skipped its jobs.
Verdict: APPROVE
Reasoning: The precedence now holds through the engine config a render actually uses, and the failing Windows test is isolated from the runner's env. The capture-mode fix was already correct.
— Via
What
Two fixes to how renders pick their browser:
HYPERFRAMES_BROWSER_PATHandPRODUCER_HEADLESS_SHELL_PATHset to different binaries, the CLI choseHYPERFRAMES_BROWSER_PATHand the engine launchedPRODUCER_HEADLESS_SHELL_PATH. The preflight checked one browser while the render ran on the other. Both now resolve through onebrowserPathFromEnv(inchrome-host-ceiling, which both already import), withHYPERFRAMES_BROWSER_PATHfirst, as the CLI documents. The engine'sresolveConfigno longer copiesPRODUCER_HEADLESS_SHELL_PATHintochromePath. That copy was checked before the env, so a CLI render still launched thePRODUCER_binary. An explicitchromePathstill wins. One behaviour change: with a validHYPERFRAMES_BROWSER_PATHset, a brokenPRODUCER_HEADLESS_SHELL_PATHis no longer reported, because it is never used.headlessShell: resolveHeadlessShellPath(cfg) !== null. That function returnsstring | undefined, so the comparison was always true. A Linux render on system Chrome, which the engine runs in screenshot mode, was treated as beginframe, which turns the parallel-stream router on by default for the wrong capture mode.resolveParallelCaptureModenow takes the resolved path and checks forundefineditself.Why
Both make what the CLI decides and what the engine does disagree. The second also sends one cohort of Linux renders down a router path that was only measured for beginframe capture.
Related work
None. A follow-up PR adds a generic seam for a host to supply its own Chromium launcher on top of the same resolvers.
How
chromeHostCeiling.tsgainsbrowserPathFromEnv(env). The engine'sresolveHeadlessShellPathand the CLI'sfindFromEnvboth call it. Each keeps its existing reaction to a missing file: the engine throws, the CLI falls through.config.tsdrops its own env read forchromePath, soresolveHeadlessShellPathowns the env order for every caller.resolveParallelCaptureMode({ headlessShellPath })replaces the boolean, so the "was a shell resolved" check lives in the tested function, not at the call site.findFromEnvnow states the rule without its history.Test plan
browserManager.test.ts: with both env vars set to different existing files,resolveHeadlessShellPath({})andresolveHeadlessShellPath(resolveConfig())both returnHYPERFRAMES_BROWSER_PATH. The block now clears both vars before each case, so a runner that exports one (the Windows job setsHYPERFRAMES_BROWSER_PATH) cannot change a case.renderOrchestrator.test.ts: a Linux render with no resolved shell isscreenshot.browserManager.tsfrom main, the precedence test fails (1 failed, 16 passed);!== nullagain, the screenshot case fails (1 failed, 2 passed);config.ts's env read put back, theresolveConfig()case fails (1 failed, 91 passed);HYPERFRAMES_BROWSER_PATHset as on the Windows runner, the missing-binary case fails (1 failed, 91 passed), which is the Windows job's failure.browserManager.test.tsplusconfig.test.tspassed 3 runs in a row withHYPERFRAMES_BROWSER_PATHset (205 passed). These also pass: the CLI'srender.test.ts(105 passed), the producer'srenderOrchestrator.test.ts(280 passed) and the Lambdahandler.test.ts(25 passed).