From f16e509832d4fa02bbc9a5f81b59ff78f9d466af Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miguel=20=C3=81ngel?= Date: Fri, 2 Oct 2026 08:36:46 -0700 Subject: [PATCH] fix(cli): stop browser --force from deleting chrome under running renders (#4901) * fix(cli): stop browser --force from deleting chrome under running renders * style(cli): format browser manager test * fix(cli): sweep leftovers of killed browser installs and on browser clear * docs(cli): state what browser clear returns without history wording --- packages/cli/src/browser/manager.test.ts | 168 ++++++++++++++++++++--- packages/cli/src/browser/manager.ts | 130 ++++++++++-------- packages/cli/src/commands/browser.ts | 2 +- 3 files changed, 217 insertions(+), 83 deletions(-) diff --git a/packages/cli/src/browser/manager.test.ts b/packages/cli/src/browser/manager.test.ts index 0490996773..e576f7d7aa 100644 --- a/packages/cli/src/browser/manager.test.ts +++ b/packages/cli/src/browser/manager.test.ts @@ -21,7 +21,9 @@ * `background-removal/manager.test.ts`) so we don't touch the real * `HOME` cache. */ -import { join, sep } from "node:path"; +import { existsSync, mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { dirname, join, relative, sep } from "node:path"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { managedChromeVersion } from "./manager.js"; @@ -59,6 +61,27 @@ const TEST_LOCK_TIMINGS = { waitNoticeMs: 1_000, }; +// Points node:os homedir at a fresh temp dir so the manager runs against the real filesystem. +function useRealCacheHome(): string { + const home = mkdtempSync(join(tmpdir(), "hf-browser-cache-")); + vi.doMock("node:os", async (importOriginal) => ({ + ...(await importOriginal()), + homedir: () => home, + })); + return home; +} + +function writeStagedInstall(cacheDir: string, buildId: string, content: string) { + const path = join(cacheDir, "chrome-headless-shell", `linux-${buildId}`); + const executablePath = join(path, "chrome-headless-shell-linux64", "chrome-headless-shell"); + mkdirSync(dirname(executablePath), { recursive: true }); + writeFileSync(executablePath, content); + return { executablePath, path }; +} + +// The mocked fs path set of the current test, so the install mock can stage files into it. +let mockedPaths: Set | undefined; + interface FsMockOptions { existing: ReadonlySet; /** map of dir path -> entries returned by readdirSync */ @@ -79,13 +102,18 @@ function installFsMocks({ // Mutable, and returned, so tests can pre-seed a "lock already held" path or // assert the lock dir doesn't leak after ensureBrowser resolves. const paths = new Set(existing); + mockedPaths = paths; const mtimes = new Map([...existing].map((p) => [p, initialMtimeMs])); const contents = new Map(); vi.doMock("node:fs", () => ({ existsSync: (p: string) => paths.has(p), readdirSync: (p: string) => { const entries = dirs?.[p]; - if (!entries) throw new Error(`ENOENT: readdirSync mock had no entry for ${p}`); + if (!entries) { + const err = new Error(`ENOENT: readdirSync mock had no entry for ${p}`); + (err as NodeJS.ErrnoException).code = "ENOENT"; + throw err; + } return entries; }, mkdirSync: (p: string, opts?: { recursive?: boolean }) => { @@ -109,6 +137,18 @@ function installFsMocks({ } } }, + renameSync: (from: string, to: string) => { + const moved = [...paths].filter((p) => p === from || p.startsWith(from + sep)); + if (moved.length === 0) { + const err = new Error(`ENOENT: no such file or directory, rename '${from}'`); + (err as NodeJS.ErrnoException).code = "ENOENT"; + throw err; + } + for (const p of moved) { + paths.delete(p); + paths.add(to + p.slice(from.length)); + } + }, statSync: (p: string) => { if (!paths.has(p)) { const err = new Error(`ENOENT: no such file or directory, stat '${p}'`); @@ -160,10 +200,25 @@ function installPuppeteerBrowsersMock( }>; browserPlatform?: string; installedInHfCacheError?: Error; - installResult?: { executablePath: string }; - installImpl?: (options: { buildId: string }) => Promise<{ executablePath: string }>; + installResult?: { executablePath: string; path?: string }; + installImpl?: (options: { + buildId: string; + cacheDir: string; + }) => Promise<{ executablePath: string; path?: string }>; } = {}, ) { + const impl = + opts.installImpl ?? (async () => opts.installResult ?? { executablePath: HF_BINARY }); + // Fixtures name the binary where it lands in HF_CACHE; install() really writes it under its own cacheDir. + const stagedInstall = async (options: { buildId: string; cacheDir: string }) => { + const result = await impl(options); + if (result.path || !result.executablePath.startsWith(HF_CACHE + sep)) return result; + const rel = relative(HF_CACHE, result.executablePath); + const executablePath = join(options.cacheDir, rel); + const path = join(options.cacheDir, ...rel.split(sep).slice(0, 2)); + mockedPaths?.add(path).add(executablePath); + return { executablePath, path }; + }; vi.doMock("@puppeteer/browsers", () => ({ Browser: { CHROMEHEADLESSSHELL: "chrome-headless-shell" }, detectBrowserPlatform: () => opts.browserPlatform ?? "linux", @@ -175,11 +230,7 @@ function installPuppeteerBrowsersMock( ...browser, })), ), - install: vi - .fn() - .mockImplementation( - opts.installImpl ?? (async () => opts.installResult ?? { executablePath: HF_BINARY }), - ), + install: vi.fn().mockImplementation(stagedInstall), })); } @@ -378,8 +429,9 @@ describe("findBrowser — cache resolution", () => { // The stale install DIR is present (extraction got partway through, e.g. an // ABOUT/LICENSE-only extract) even though the exe itself is missing — // exercises the purge-before-redownload fix, not just the redownload path. + const staleLeftover = join(staleInstallDir, "ABOUT"); const paths = installFsMocks({ - existing: new Set([HF_CACHE, staleInstallDir]), + existing: new Set([HF_CACHE, staleInstallDir, staleLeftover]), }); installPuppeteerBrowsersMock({ installedInHfCache: [ @@ -402,13 +454,13 @@ describe("findBrowser — cache resolution", () => { source: "download", }); expect(warnSpy).toHaveBeenCalledWith(expect.stringContaining("Cached binary missing")); - // The stale directory must be gone before @puppeteer/browsers' install() - // sees it — otherwise install() throws "folder exists but exe missing" - // instead of re-extracting (the exact bug both feedback reports hit). - expect(paths.has(staleInstallDir)).toBe(false); + // The partial extract is replaced wholesale, not merged into (install() would + // otherwise throw "folder exists but exe missing", the bug both feedback reports hit). + expect(paths.has(staleLeftover)).toBe(false); + expect(paths.has(redownloadedBinary)).toBe(true); }); - it("ensureBrowser({force: true}) purges the whole cache before downloading, bypassing any cache/system shortcut", async () => { + it("ensureBrowser({force: true}) re-downloads without purging the cache, bypassing any cache/system shortcut", async () => { const staleInstallDir = join(HF_CACHE, "chrome-headless-shell", "linux-131.0.6778.85"); const downloadedBinary = join(HF_CACHE, "chrome-headless-shell", "force-downloaded"); // A HEALTHY cached binary AND system Chrome are both present — force must @@ -436,14 +488,88 @@ describe("findBrowser — cache resolution", () => { executablePath: downloadedBinary, source: "download", }); - // clearBrowser() wipes prior contents; withInstallLock uses a sibling lock - // outside CACHE_DIR, so assert the purge on what was actually INSIDE it, - // not the directory's own existence. - expect(paths.has(staleInstallDir)).toBe(false); - expect(paths.has(HF_BINARY)).toBe(false); + // Only the downloaded version dir is swapped in; other cache entries stay. + expect(paths.has(downloadedBinary)).toBe(true); + expect(paths.has(HF_BINARY)).toBe(true); }); - it("serializes concurrent force downloads so one purge cannot delete another installer's lock", async () => { + it("keeps a live reader's binary valid while a concurrent --force re-downloads it", async () => { + const home = useRealCacheHome(); + let label = "old"; + let seenMidInstall: string | undefined; + let liveBinary: string | undefined; + installPuppeteerBrowsersMock({ + installImpl: async ({ cacheDir, buildId }) => { + if (liveBinary) seenMidInstall = readFileSync(liveBinary, "utf8"); + return writeStagedInstall(cacheDir, buildId, label); + }, + }); + try { + const { ensureBrowser, CACHE_DIR } = await import("./manager.js"); + const otherVersion = join(CACHE_DIR, "chrome-headless-shell", "linux-1.0.0", "marker"); + mkdirSync(dirname(otherVersion), { recursive: true }); + writeFileSync(otherVersion, "other"); + + liveBinary = (await ensureBrowser({ force: true })).executablePath; + label = "new"; + const forced = await ensureBrowser({ force: true }); + + expect(seenMidInstall).toBe("old"); + expect(forced.executablePath).toBe(liveBinary); + expect(readFileSync(liveBinary, "utf8")).toBe("new"); + expect(readFileSync(otherVersion, "utf8")).toBe("other"); + } finally { + rmSync(home, { recursive: true, force: true }); + } + }); + + it("restores the previous version when the staged install cannot be moved in", async () => { + const home = useRealCacheHome(); + let stageNothing = false; + installPuppeteerBrowsersMock({ + installImpl: async ({ cacheDir, buildId }) => { + const staged = writeStagedInstall(cacheDir, buildId, "old"); + if (stageNothing) rmSync(staged.path, { recursive: true, force: true }); + return staged; + }, + }); + try { + const { ensureBrowser } = await import("./manager.js"); + const liveBinary = (await ensureBrowser({ force: true })).executablePath; + stageNothing = true; + + await expect(ensureBrowser({ force: true })).rejects.toThrow("HYPERFRAMES_BROWSER_PATH"); + expect(readFileSync(liveBinary, "utf8")).toBe("old"); + } finally { + rmSync(home, { recursive: true, force: true }); + } + }); + + it("sweeps dirs leaked by a killed install on the next install and on clear", async () => { + const home = useRealCacheHome(); + installPuppeteerBrowsersMock({ + installImpl: async ({ cacheDir, buildId }) => writeStagedInstall(cacheDir, buildId, "fresh"), + }); + const root = join(home, ".cache", "hyperframes"); + const leftovers = [join(root, ".chrome-staging-dead"), join(root, ".chrome-replaced-dead")]; + const seedLeftovers = () => { + for (const dir of leftovers) mkdirSync(join(dir, "partial"), { recursive: true }); + }; + try { + const { ensureBrowser, clearBrowser } = await import("./manager.js"); + seedLeftovers(); + await ensureBrowser({ force: true }); + expect(leftovers.filter((dir) => existsSync(dir))).toEqual([]); + + seedLeftovers(); + expect(clearBrowser()).toBe(true); + expect(leftovers.filter((dir) => existsSync(dir))).toEqual([]); + } finally { + rmSync(home, { recursive: true, force: true }); + } + }); + + it("serializes concurrent force downloads so they never install at the same time", async () => { const downloadedBinary = join(HF_CACHE, "chrome-headless-shell", "force-downloaded"); const paths = installFsMocks({ existing: new Set([CACHE_ROOT, HF_CACHE, HF_BINARY]), diff --git a/packages/cli/src/browser/manager.ts b/packages/cli/src/browser/manager.ts index 6ed2cfd578..6c19d2ba79 100644 --- a/packages/cli/src/browser/manager.ts +++ b/packages/cli/src/browser/manager.ts @@ -6,14 +6,14 @@ import { mkdirSync, readFileSync, readdirSync, + renameSync, rmSync, statSync, utimesSync, writeFileSync, } from "node:fs"; -import { basename } from "node:path"; import { homedir } from "node:os"; -import { join } from "node:path"; +import { basename, dirname, join, relative } from "node:path"; import { chromeMajorCeiling, exceedsChromeCeiling } from "@hyperframes/engine/chrome-host-ceiling"; import { normalizeErrorMessage } from "../utils/errorMessage.js"; import { runRenderSetupWorker } from "../utils/cancellableProcess.js"; @@ -244,8 +244,7 @@ export interface BrowserResult { export interface EnsureBrowserOptions { onProgress?: (downloadedBytes: number, totalBytes: number) => void; - // Purge any cached HF-managed download before resolving, so a stale or - // partially-extracted install can't make the retry look like a no-op. + // Re-download the managed build even when cached; only its version dir is replaced. force?: boolean; // Always resolve to OUR pinned `CHROME_VERSION` build (cached, or freshly // downloaded) — skip both the shared puppeteer-cache preference (some other @@ -264,23 +263,6 @@ export interface EnsureBrowserOptions { interface CacheLookupResult { result?: BrowserResult; staleHyperframesCachePath?: string; - // Root install-folder path for the stale entry (InstalledBrowser#path), NOT - // the missing executablePath above — this is what actually needs deleting. - staleInstallPath?: string; -} - -/** - * Remove one browser version's install directory (not the whole CACHE_DIR). - * @puppeteer/browsers' install() treats an existing-but-incomplete directory - * as already installed and throws "folder exists but executable is missing" - * rather than re-extracting — so an extraction interrupted by a Windows AV - * lock, a sleep/wake cycle, or ctrl-C (left with only alphabetically-early - * files like ABOUT/LICENSE, no exe) wedges every subsequent ensure/render - * with the same error until someone manually deletes the directory. Purging - * it first makes the retry actually retry. - */ -function purgeStaleInstall(installPath: string): void { - rmSync(installPath, { recursive: true, force: true }); } // --- Internal helpers ------------------------------------------------------- @@ -399,7 +381,7 @@ async function findFromHyperframesCache(): Promise { return { result: { executablePath: match.executablePath, source: "cache" } }; } if (match) { - return { staleHyperframesCachePath: match.executablePath, staleInstallPath: match.path }; + return { staleHyperframesCachePath: match.executablePath }; } return {}; } @@ -588,10 +570,7 @@ export async function findBrowser(): Promise { `[browser] Cached binary missing at ${fromCache.staleHyperframesCachePath} — re-downloading...`, ); try { - return await withInstallLock(async () => { - if (fromCache.staleInstallPath) purgeStaleInstall(fromCache.staleInstallPath); - return downloadBrowser(); - }); + return await withInstallLock(() => downloadBrowser()); } catch (err) { const cause = normalizeErrorMessage(err); throw new Error( @@ -684,10 +663,7 @@ async function ensureBrowserInCurrentProcess( console.warn( `[browser] Cached binary missing at ${fromCache.staleHyperframesCachePath} — re-downloading...`, ); - return withInstallLock(async () => { - if (fromCache.staleInstallPath) purgeStaleInstall(fromCache.staleInstallPath); - return downloadBrowser(options); - }); + return withInstallLock(() => downloadBrowser(options)); } if (!options?.preferManagedChrome) { @@ -700,24 +676,15 @@ async function ensureBrowserInCurrentProcess( } return withInstallLock(async () => { - if (options?.force) { - // `--force` means "always get a fresh managed download" — purging the - // whole HF-managed cache after acquiring the install lock keeps two - // concurrent force retries from deleting each other's in-flight lock or - // partially extracted install. - clearBrowser(); - } - // Re-check after acquiring the lock: a concurrent invocation may have // finished installing while we were waiting, in which case reuse its // result instead of downloading and extracting a second time. Skipped - // under --force, which already purged and always wants a fresh download. + // under --force, which always wants a fresh download. if (!options?.force) { const afterLock = await (options?.preferManagedChrome ? findFromHyperframesCache() : findFromCache()); if (afterLock.result) return afterLock.result; - if (afterLock.staleInstallPath) purgeStaleInstall(afterLock.staleInstallPath); } return downloadBrowser(options); }); @@ -837,43 +804,84 @@ async function downloadBrowser(options?: EnsureBrowserOptions): Promise rmSync(stagingDir, { recursive: true, force: true }); const runInstall = () => install({ - cacheDir: CACHE_DIR, + cacheDir: stagingDir, browser: Browser.CHROMEHEADLESSSHELL, buildId: managedChromeVersion(), platform, downloadProgressCallback: options?.onProgress, }); - let installed; try { - installed = await installWithCorruptArchiveRecovery( - runInstall, - () => { - rmSync(CACHE_DIR, { recursive: true, force: true }); - mkdirSync(CACHE_DIR, { recursive: true }); - }, - (err) => - console.warn( - `[hyperframes] Cached browser archive was corrupt (${normalizeErrorMessage(err)}); clearing the cache and re-downloading.`, - ), + const staged = await installWithCorruptArchiveRecovery(runInstall, clearStaging, (err) => + console.warn( + `[hyperframes] Downloaded browser archive was corrupt (${normalizeErrorMessage(err)}); re-downloading.`, + ), ); + return { executablePath: moveStagedInstallIntoCache(stagingDir, staged), source: "download" }; } catch (err) { throw wrapDownloadFailureWithBrowserPathHint(err); + } finally { + clearStaging(); } - - return { executablePath: installed.executablePath, source: "download" }; } -/** - * Remove the cached Chrome download directory. - * Returns true if anything was removed. - */ -export function clearBrowser(): boolean { - if (!existsSync(CACHE_DIR)) { - return false; +// Swap one staged version dir into CACHE_DIR by rename, never deleting the cache under live users: +// a running old binary keeps its inode, and a stale partial extract (exe missing) is replaced too. +function moveStagedInstallIntoCache( + stagingDir: string, + staged: { path: string; executablePath: string }, +): string { + const target = join(CACHE_DIR, relative(stagingDir, staged.path)); + const aside = join(CACHE_ROOT_DIR, `${REPLACED_PREFIX}${randomUUID()}`); + mkdirSync(dirname(target), { recursive: true }); + let replaced = false; + try { + renameSync(target, aside); + replaced = true; + } catch (err) { + if (!isErrno(err, "ENOENT")) throw err; } + try { + renameSync(staged.path, target); + } catch (err) { + if (replaced) renameSync(aside, target); + throw err; + } + if (replaced) rmSync(aside, { recursive: true, force: true }); + return join(target, relative(staged.path, staged.executablePath)); +} + +const STAGING_PREFIX = ".chrome-staging-"; +const REPLACED_PREFIX = ".chrome-replaced-"; + +// Staging and set-aside dirs outlive an install killed by a signal, since `finally` never runs. +function removeInstallLeftovers(): boolean { + let names: string[]; + try { + names = readdirSync(CACHE_ROOT_DIR); + } catch (err) { + if (isErrno(err, "ENOENT")) return false; + throw err; + } + const leftovers = names.filter( + (name) => name.startsWith(STAGING_PREFIX) || name.startsWith(REPLACED_PREFIX), + ); + for (const name of leftovers) + rmSync(join(CACHE_ROOT_DIR, name), { recursive: true, force: true }); + return leftovers.length > 0; +} + +// Deletes the managed Chrome cache and interrupted-install leftovers; true when any of them existed. +export function clearBrowser(): boolean { + const removedLeftovers = removeInstallLeftovers(); + if (!existsSync(CACHE_DIR)) return removedLeftovers; rmSync(CACHE_DIR, { recursive: true, force: true }); return true; } diff --git a/packages/cli/src/commands/browser.ts b/packages/cli/src/commands/browser.ts index 77915b3da3..47bc922582 100644 --- a/packages/cli/src/commands/browser.ts +++ b/packages/cli/src/commands/browser.ts @@ -93,7 +93,7 @@ async function runEnsure(options?: { force?: boolean }): Promise { return; } - s.start("Purging cached download and re-downloading..."); + s.start("Re-downloading the managed browser..."); const downloadSpinner = clack.spinner(); downloadSpinner.start(