fix: make doctor's Chrome check and browser path predict render's resolution - #3921
Open
miga-heygen wants to merge 3 commits into
Open
miga-heygen wants to merge 3 commits into
miga-heygen wants to merge 3 commits into
Conversation
…olution
doctor's Chrome check and `hyperframes browser path` both called the
public findBrowser() with no options, which resolves loosely: any
puppeteer-cached chrome-headless-shell version, then the
hyperframes-managed cache (exact CHROME_VERSION pin), then system
Chrome. Render's actual resolution (ensureBrowser({ preferManagedChrome:
true })) is stricter -- it only accepts the exact-pinned managed-cache
build, skipping both the puppeteer-cache and system-Chrome fallbacks.
So doctor/browser path could report a "cache hit" that render doesn't
honor at all, immediately before an unexpected re-download.
Adds an optional preferManagedChrome option to findBrowser(), mirroring
ensureBrowser's existing conditional shape, and threads it through
doctor's underlying checkChrome() and browser path's lookup (plus its
own not-found fallback). Skipped on Linux ARM64, which has no managed
chrome-headless-shell build at all -- forcing preferManagedChrome there
would report "not found" even with a correctly installed system
Chromium, the exact false-negative this check must avoid producing.
Co-Authored-By: Miguel Angel <miguel.sierra@heygen.com>
The "Linux ARM64 has no managed chrome-headless-shell, so resolve
unqualified there" rule was spelled three ways: explicitly in doctor's
checkChrome, explicitly in `browser ensure`'s ARM64 branch, and only
implicitly in `browser path`, which reached system Chromium on ARM64 by
falling into ensureBrowser's download path and being rerouted. Move the
decision into manager.ts (`resolvesManagedOnly`), shared by findBrowser
and ensureBrowser, so callers pass the plain `preferManagedChrome` option
and a render on ARM64 no longer takes the install lock just to find the
system Chromium it could have found up front.
findBrowser is documented as "find without downloading", but its
stale-cache branch (manifest present, executable missing) triggered a
full re-download — from doctor. It now reports that state as not found;
ensureBrowser owns the re-download, and both `browser path` and doctor
already route a miss there.
Tests: findBrowser({ preferManagedChrome }) on Linux ARM64 still finds
system Chromium; ensureBrowser does too without taking the install lock;
`browser path` on ARM64 prints the system path without entering
ensureBrowser; env-var override still wins under preferManagedChrome; a
stale managed-cache entry makes findBrowser return undefined without
calling install().
Co-Authored-By: Miguel Ángel <miguel.sierra@heygen.com>
Contributor
Author
|
Addressed in 0cb180f. |
The `browser path` suite only covered Linux ARM64, where
`preferManagedChrome` is a no-op by design, so dropping the option from
either `runPath` resolution survived the tests. Add an x64 case with a
puppeteer-cache binary and an empty managed cache: the command must not
print the puppeteer path, and both `findBrowser` and `ensureBrowser`
must be called with `{ preferManagedChrome: true }`.
Pin `ensureBrowser`'s own first cache lookup the same way: on x64 with
both caches populated it returns the pinned managed binary without
calling `install()`, where the unqualified call returns the
puppeteer-cache one.
Fold the three `managedOnly ? findFromHyperframesCache() :
findFromCache()` sites into a `lookupCache(managedOnly)` helper next to
`resolvesManagedOnly`, and reword the preflight comment: both cache
lookups now swallow their own read errors, so the remaining throw out of
`findBrowser` is `@puppeteer/browsers` failing to load. Rename the
preflight test that described the old corrupt-cache throw.
Co-Authored-By: Miguel Ángel <miguel.sierra@heygen.com>
Contributor
Author
|
Addressed in 24a611a. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
doctor's Chrome check andhyperframes browser pathboth called the publicfindBrowser()(inpackages/cli/src/browser/manager.ts) with no options, which resolves loosely: any puppeteer-cachedchrome-headless-shellversion, then the hyperframes-managed cache (exactCHROME_VERSIONpin), then system Chrome. Render's actual resolution (ensureBrowser({ preferManagedChrome: true }), used byrender/execute.ts,studioServer.ts, andbrowser ensure) is stricter — it only accepts the exact-pinned managed-cache build, skipping both the puppeteer-cache and system-Chrome fallbacks entirely.So
doctor/browser pathcould report a "cache hit" against a wrong-version puppeteer-cache build or system Chrome, while the very nextrendercall doesn't honor that at all and re-downloads the full pinned build — a false "you're good" signal immediately before an unexpected multi-minute download.Fix
Added an optional
preferManagedChromeoption to the previously zero-argfindBrowser(), mirroringensureBrowser's existing internal conditional shape: when set, it checks only the exact-pin hyperframes-managed cache and returnsundefinedinstead of falling back to system Chrome.doctor's underlyingcheckChrome()andbrowser path's lookup (including its own not-foundensureBrowser()fallback) now pass this option, so a reported hit actually predicts render's behavior.The four other pre-existing
findBrowser()call sites (three insideensureLinuxArmBrowserinmanager.tsand one inbrowser ensure's ARM64 branch) stay unqualified: they only run on Linux ARM64, where the option is a no-op anyway (see below).Linux ARM64: Chrome for Testing publishes no
linux-arm64chrome-headless-shell, so there is no managed cache to check against —ensureBrowser({ preferManagedChrome: true })reroutes to system Chromium there (ensureLinuxArmBrowser). An early version of this fix passed the option unconditionally fromcheckChrome(), which would have madedoctorreport "Chrome not found" on every ARM64 machine, even one with a correctly installed system Chromium. Rather than have each caller carry that exception, the decision now lives once inmanager.ts(resolvesManagedOnly:preferManagedChrome && !isLinuxArm()), shared byfindBrowserandensureBrowser.doctor,browser path, and any future caller get the right answer with the plain option, and apreferManagedChromerender on ARM64 resolves system Chromium directly instead of taking the install lock and detouring through the download path to reach it.Stale cache:
findBrowseris documented as "find without downloading", but its stale-cache branch (manifest present, executable missing) used to trigger a full re-download — fromdoctor. It now reports that state as "not found" and leaves the re-download toensureBrowser;browser pathalready falls through toensureBrowseron a miss, anddoctorpoints athyperframes browser ensure.Test plan
manager.test.ts:preferManagedChromeignores a puppeteer-cache hit and resolves to the pinned managed cache when both are populated; does not report a false hit against an off-pin puppeteer-cache build (the exact reported scenario); does not fall back to system Chrome; still resolvesHYPERFRAMES_BROWSER_PATHfirst; still finds system Chromium on Linux ARM64 (bothfindBrowserandensureBrowser, the latter asserting the install lock is never taken).ensureBrowser({ preferManagedChrome: true })on x64 with both caches populated returns the pinned managed binary without callinginstall(), where the unqualified call returns the puppeteer-cache one.findBrowserreports a stale managed-cache entry as not found without callinginstall(); the existing stale-cache re-download test now exercisesensureBrowser, which owns that path.preflight.test.ts:checkChromecallsfindBrowserwith{ preferManagedChrome: true }.commands/browser.test.ts(new):hyperframes browser pathon Linux ARM64 prints the system Chromium path without enteringensureBrowser. On x64 with a puppeteer-cache binary present and an empty managed cache,browser pathdoes not print the puppeteer-cache path; bothfindBrowserandensureBrowserare called with{ preferManagedChrome: true }.bunx tsc --noEmit -p packages/cliclean;oxlint/oxfmt --checkclean on all touched files;manager.test.ts+preflight.test.ts+commands/browser.test.ts+doctor.test.ts— 94/94 pass.!isLinuxArm()guard fails the ARM64findBrowsertest and thebrowser pathtest; bypassingresolvesManagedOnlyinensureBrowseralone fails its ARM64 test; ignoring the env var fails the env-first test (plus three pre-existing ones). Making eitherrunPathresolution unqualified fails the x64browser pathtest; keyingensureBrowser's first cache lookup off the unqualified path fails its x64 managed-only test; invertinglookupCachefails eight cache-resolution tests.