From 0241078560f3846f1f825397bd137785db741ceb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miguel=20=C3=81ngel?= Date: Fri, 9 Oct 2026 19:18:27 +0000 Subject: [PATCH] fix(cli): name the unzip errors when every Windows unzip tool fails (#5355) * fix(cli): name the unzip errors when every Windows unzip tool fails * fix(cli): a cached empty or folder Chrome counts as missing, not found * fix(engine): skip an empty cached Chrome the same way the CLI does * fix(engine): keep scanning the Chrome cache past a build it cannot read * test(engine): a folder at the cached Chrome path is skipped --- packages/cli/src/browser/manager.test.ts | 92 +++++++++++++++++-- packages/cli/src/browser/manager.ts | 52 +++++++---- packages/cli/src/commands/browser.test.ts | 18 +++- packages/cli/src/commands/browser.ts | 14 ++- packages/cli/src/renderSetupWorker.test.ts | 8 +- .../cli/src/renderSetupWorkerLifecycle.ts | 2 +- .../src/services/browserManager.test.ts | 81 ++++++++++++++-- .../engine/src/services/browserManager.ts | 13 ++- 8 files changed, 236 insertions(+), 44 deletions(-) diff --git a/packages/cli/src/browser/manager.test.ts b/packages/cli/src/browser/manager.test.ts index 34e8d67520..1bdf9fd452 100644 --- a/packages/cli/src/browser/manager.test.ts +++ b/packages/cli/src/browser/manager.test.ts @@ -94,6 +94,8 @@ interface FsMockOptions { initialMtimeMs?: number; /** what node:os reports; defaults to a Linux host */ osHost?: { platform: string; release: string }; + /** paths whose stat reports 0 bytes */ + emptyFiles?: ReadonlySet; } function installFsMocks({ @@ -102,6 +104,7 @@ function installFsMocks({ touchError, initialMtimeMs = 0, osHost = { platform: "linux", release: "24.0.0" }, + emptyFiles, }: FsMockOptions) { // Mutable, and returned, so tests can pre-seed a "lock already held" path or // assert the lock dir doesn't leak after ensureBrowser resolves. @@ -160,7 +163,7 @@ function installFsMocks({ (err as NodeJS.ErrnoException).code = "ENOENT"; throw err; } - return { mtimeMs: mtimes.get(p) ?? 0, size: 1 }; + return { mtimeMs: mtimes.get(p) ?? 0, size: emptyFiles?.has(p) ? 0 : 1, isFile: () => true }; }, utimesSync: (p: string, _atime: Date, mtime: Date) => { if (touchError) throw touchError; @@ -463,7 +466,7 @@ describe("findBrowser — cache resolution", () => { executablePath: redownloadedBinary, source: "download", }); - expect(warnSpy).toHaveBeenCalledWith(expect.stringContaining("Cached binary missing")); + expect(warnSpy).toHaveBeenCalledWith(expect.stringContaining("Cached binary unusable")); // 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); @@ -533,12 +536,44 @@ describe("findBrowser — cache resolution", () => { } }); + it("installs again instead of reusing an empty cached executable", async () => { + const home = useRealCacheHome(); + installPuppeteerBrowsersMock({ + installImpl: async ({ cacheDir, buildId }) => writeStagedInstall(cacheDir, buildId, "fresh"), + }); + try { + const { ensureBrowser } = await import("./manager.js"); + const cached = await ensureBrowser({ force: true }); + writeFileSync(cached.executablePath, ""); + const { getInstalledBrowsers } = await import("@puppeteer/browsers"); + vi.mocked(getInstalledBrowsers).mockResolvedValue([ + { + browser: "chrome-headless-shell", + buildId: CHROME_VERSION, + platform: "linux", + executablePath: cached.executablePath, + } as never, + ]); + vi.spyOn(console, "warn").mockImplementation(() => {}); + + const again = await ensureBrowser({ preferManagedChrome: true }); + + expect(again.source).toBe("download"); + expect(readFileSync(again.executablePath, "utf8")).toBe("fresh"); + } 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 }) => { + installImpl: async ({ cacheDir, buildId, logger }) => { if (!stageNothing) return writeStagedInstall(cacheDir, buildId, "old"); + logger?.("puppeteer:browsers:fileUtil")?.( + "tar.exe extraction failed: before PowerShell worked", + ); // A real executable passes the unzip check, but its version dir is gone, so only the move fails. const elsewhere = writeStagedInstall(join(cacheDir, "elsewhere"), buildId, "new"); return { @@ -552,7 +587,9 @@ describe("findBrowser — cache resolution", () => { const liveBinary = (await ensureBrowser({ force: true })).executablePath; stageNothing = true; - await expect(ensureBrowser({ force: true })).rejects.toThrow("HYPERFRAMES_BROWSER_PATH"); + const failure = await ensureBrowser({ force: true }).catch((err: unknown) => String(err)); + expect(failure).toContain("HYPERFRAMES_BROWSER_PATH"); + expect(failure).not.toContain("Unzip errors"); expect(readFileSync(liveBinary, "utf8")).toBe("old"); } finally { rmSync(home, { recursive: true, force: true }); @@ -560,15 +597,18 @@ describe("findBrowser — cache resolution", () => { }); it.each([ - { label: "missing", unpacked: undefined, state: "is missing" }, - { label: "empty", unpacked: "", state: "is empty" }, + { label: "missing", unpacked: "none", state: "is missing" }, + { label: "empty", unpacked: "file", state: "is empty" }, + { label: "a folder", unpacked: "folder", state: "is not a file" }, ])("fails instead of reporting a browser the unzip left $label", async ({ unpacked, state }) => { const home = useRealCacheHome(); installPuppeteerBrowsersMock({ installImpl: async ({ cacheDir, buildId, logger }) => { const staged = writeStagedInstall(cacheDir, buildId, ""); rmSync(staged.executablePath); - if (unpacked !== undefined) writeFileSync(staged.executablePath, unpacked); + if (unpacked === "file") writeFileSync(staged.executablePath, ""); + if (unpacked === "folder") + mkdirSync(join(staged.executablePath, "inside"), { recursive: true }); logger?.("puppeteer:browsers:fileUtil")?.("tar.exe extraction failed: Error: boom"); return staged; }, @@ -586,6 +626,30 @@ describe("findBrowser — cache resolution", () => { } }); + it("names the unzip errors when every unzip tool failed", async () => { + const home = useRealCacheHome(); + installPuppeteerBrowsersMock({ + installImpl: async ({ logger }) => { + logger?.("puppeteer:browsers:fileUtil")?.("tar.exe extraction failed: Error: tar boom"); + logger?.("puppeteer:browsers:fileUtil")?.( + "powershell.exe extraction failed: Error: ps boom", + ); + throw new Error("Extraction failed: no zip archiver is available."); + }, + }); + try { + const { ensureBrowser } = await import("./manager.js"); + + const ensured = ensureBrowser({ force: true }); + + await expect(ensured).rejects.toThrow( + "no zip archiver is available. Unzip errors: tar.exe extraction failed: Error: tar boom; powershell.exe extraction failed: Error: ps boom", + ); + } finally { + rmSync(home, { recursive: true, force: true }); + } + }); + it("reports only the last attempt's unzip errors after a corrupt-archive retry", async () => { const home = useRealCacheHome(); let attempt = 0; @@ -949,6 +1013,20 @@ describe("findBrowser — cache resolution", () => { }, ); + it("skips an empty executable in the puppeteer cache", async () => { + installFsMocks({ + existing: new Set([PUPPETEER_CACHE, PUPPETEER_BINARY]), + dirs: { [PUPPETEER_CACHE]: ["linux-148.0.7778.97"] }, + emptyFiles: new Set([PUPPETEER_BINARY]), + }); + installPuppeteerBrowsersMock(); + + const { findBrowser } = await import("./manager.js"); + const result = await findBrowser(); + + expect(result?.executablePath).not.toBe(PUPPETEER_BINARY); + }); + it("prefers the puppeteer cache over the hyperframes cache when BOTH are populated", async () => { // The HF cache is pinned to `CHROME_VERSION` (131-era) which lags upstream // by many releases. The engine's `resolveHeadlessShellPath` scans the diff --git a/packages/cli/src/browser/manager.ts b/packages/cli/src/browser/manager.ts index f75b80b3f7..163afcfeda 100644 --- a/packages/cli/src/browser/manager.ts +++ b/packages/cli/src/browser/manager.ts @@ -378,7 +378,7 @@ async function findFromHyperframesCache(): Promise { b.buildId === managedChromeVersion() && b.platform === hostPlatform, ); - if (match && existsSync(match.executablePath)) { + if (match && isUsableExecutable(match.executablePath)) { return { result: { executablePath: match.executablePath, source: "cache" } }; } if (match) { @@ -499,7 +499,7 @@ function findFromPuppeteerCache(): BrowserResult | undefined { // keep them aligned. If puppeteer ever changes the on-disk layout the two // need to move together. const binary = join(PUPPETEER_CACHE_DIR, version, ...executable); - if (existsSync(binary)) { + if (isUsableExecutable(binary)) { return { executablePath: binary, source: "cache" }; } } @@ -568,14 +568,14 @@ export async function findBrowser(): Promise { if (fromCache.result) return fromCache.result; if (fromCache.staleHyperframesCachePath) { console.warn( - `[browser] Cached binary missing at ${fromCache.staleHyperframesCachePath} — re-downloading...`, + `[browser] Cached binary unusable (missing, empty or not a file) at ${fromCache.staleHyperframesCachePath} — re-downloading...`, ); try { return await withInstallLock(() => downloadBrowser()); } catch (err) { const cause = normalizeErrorMessage(err); throw new Error( - `Cached Chrome binary was missing at ${fromCache.staleHyperframesCachePath}, and re-download failed: ${cause}\n` + + `Cached Chrome binary was unusable at ${fromCache.staleHyperframesCachePath}, and re-download failed: ${cause}\n` + `Run \`hyperframes browser ensure --force\` to re-download.`, ); } @@ -662,7 +662,7 @@ async function ensureBrowserInCurrentProcess( if (fromCache.result) return fromCache.result; if (fromCache.staleHyperframesCachePath) { console.warn( - `[browser] Cached binary missing at ${fromCache.staleHyperframesCachePath} — re-downloading...`, + `[browser] Cached binary unusable (missing, empty or not a file) at ${fromCache.staleHyperframesCachePath} — re-downloading...`, ); return withInstallLock(() => downloadBrowser(options)); } @@ -779,11 +779,15 @@ function browserPathHintForPlatform(): string { return "/usr/bin/google-chrome"; } -function wrapDownloadFailureWithBrowserPathHint(cause: unknown): Error { +function wrapDownloadFailureWithBrowserPathHint( + cause: unknown, + unzipLog: readonly string[], +): Error { const original = normalizeErrorMessage(cause); + const unzipErrors = unzipLog.length > 0 ? ` Unzip errors: ${unzipLog.join("; ")}` : ""; const example = browserPathHintForPlatform(); const message = - `Failed to install chrome-headless-shell ${managedChromeVersion()}: ${original}\n\n` + + `Failed to install chrome-headless-shell ${managedChromeVersion()}: ${original}${unzipErrors}\n\n` + `Point hyperframes at an already-installed Chrome/Chromium instead:\n\n` + ` export HYPERFRAMES_BROWSER_PATH="${example}"\n\n` + `Then re-run your command. Any Chrome build works for the screenshot ` + @@ -839,27 +843,37 @@ async function downloadBrowser(options?: EnsureBrowserOptions): Promise 0 ? "usable" : "is empty"; +} + +function isUsableExecutable(path: string): boolean { + try { + return executableState(path) === "usable"; + } catch { + return false; + } +} + // install() returns without checking that the unzip produced the executable. -function assertExecutableUnpacked( - stagingDir: string, - executablePath: string, - unzipLog: readonly string[], -): void { - const size = statSync(executablePath, { throwIfNoEntry: false })?.size; - if (size) return; - const state = size === 0 ? "is empty" : "is missing"; - const log = unzipLog.length > 0 ? ` Unzip errors: ${unzipLog.join("; ")}` : ""; +function assertExecutableUnpacked(stagingDir: string, executablePath: string): void { + const state = executableState(executablePath); + if (state === "usable") return; throw new Error( - `the archive downloaded, but ${relative(stagingDir, executablePath)} ${state} after unzipping.${log}`, + `the archive downloaded, but ${relative(stagingDir, executablePath)} ${state} after unzipping.`, ); } diff --git a/packages/cli/src/commands/browser.test.ts b/packages/cli/src/commands/browser.test.ts index 3d125573bb..0733fb1f5a 100644 --- a/packages/cli/src/commands/browser.test.ts +++ b/packages/cli/src/commands/browser.test.ts @@ -18,13 +18,15 @@ vi.mock("@clack/prompts", () => ({ })); const ensureBrowser = vi.fn(); +const findBrowser = vi.fn(); +let linuxArm = false; vi.mock("../browser/manager.js", () => ({ ensureBrowser: (...args: unknown[]) => ensureBrowser(...args), - findBrowser: vi.fn(), + findBrowser: (...args: unknown[]) => findBrowser(...args), clearBrowser: vi.fn(), managedChromeVersion: () => "152.0.7977.30", CACHE_DIR: "/cache", - isLinuxArm: () => false, + isLinuxArm: () => linuxArm, })); vi.mock("../telemetry/events.js", () => ({ trackBrowserInstall: vi.fn() })); @@ -44,6 +46,8 @@ describe("browser ensure", () => { spinners.length = 0; logError.mockReset(); ensureBrowser.mockReset(); + findBrowser.mockReset(); + linuxArm = false; vi.spyOn(console, "log").mockImplementation(() => {}); }); @@ -70,4 +74,14 @@ describe("browser ensure", () => { expect(spinners.length).toBeGreaterThan(0); expect(spinners.filter((s) => s.running)).toEqual([]); }); + + it("stops its spinner and fails when the Linux ARM64 lookup throws", async () => { + linuxArm = true; + findBrowser.mockRejectedValue(new Error("EACCES: permission denied")); + + await expect(runEnsure(false)).rejects.toThrow("Command failed"); + + expect(logError).toHaveBeenCalledWith("EACCES: permission denied"); + expect(spinners.filter((s) => s.running)).toEqual([]); + }); }); diff --git a/packages/cli/src/commands/browser.ts b/packages/cli/src/commands/browser.ts index 9bb65263c2..8fb89c19f7 100644 --- a/packages/cli/src/commands/browser.ts +++ b/packages/cli/src/commands/browser.ts @@ -23,6 +23,12 @@ import { import { trackBrowserInstall } from "../telemetry/events.js"; import { normalizeErrorMessage } from "../utils/errorMessage.js"; +function failSpinner(s: ReturnType, label: string, err: unknown): never { + s.stop(c.error(label)); + clack.log.error(normalizeErrorMessage(err)); + failCommand(1, err); +} + function printBrowser(browser: BrowserResult): void { console.log(); console.log(` ${c.dim("Source:")} ${c.bold(browser.source)}`); @@ -38,7 +44,9 @@ async function runEnsure(options?: { force?: boolean }): Promise { if (isLinuxArm()) { const s = clack.spinner(); s.start("Linux ARM64 detected — looking for system Chromium..."); - const existing = await findBrowser(); + const existing = await findBrowser().catch((err: unknown) => + failSpinner(s, "Browser lookup failed", err), + ); if (existing) { s.stop(c.success("System Chromium found")); printBrowser(existing); @@ -88,9 +96,7 @@ async function runEnsure(options?: { force?: boolean }): Promise { }, }); } catch (err) { - s.stop(c.error("Browser not available")); - clack.log.error(normalizeErrorMessage(err)); - failCommand(1, err); + failSpinner(s, "Browser not available", err); } if (result.source === "download") trackBrowserInstall(); diff --git a/packages/cli/src/renderSetupWorker.test.ts b/packages/cli/src/renderSetupWorker.test.ts index 0fd8ce4499..d7ea49803e 100644 --- a/packages/cli/src/renderSetupWorker.test.ts +++ b/packages/cli/src/renderSetupWorker.test.ts @@ -66,15 +66,17 @@ describe("render setup worker failure line", () => { "Failed to install chrome-headless-shell: missing.\n\n export HYPERFRAMES_BROWSER_PATH=x"; const failure = new Error(reason, { cause: new Error("tar.exe extraction failed") }); const stderr = [ - "warning before", - renderSetupErrorLine(failure).trimEnd(), + `warning without a newline${renderSetupErrorLine(failure).trimEnd()}`, "/worker.ts:20", " throw error;", "Error: Failed to install chrome-headless-shell: missing.", " at downloadBrowser (manager.ts:845:11)", ].join("\n"); - expect(renderSetupFailureFrom(stderr)).toEqual({ reason, earlierOutput: "warning before" }); + expect(renderSetupFailureFrom(stderr)).toEqual({ + reason, + earlierOutput: "warning without a newline", + }); }); it.each([ diff --git a/packages/cli/src/renderSetupWorkerLifecycle.ts b/packages/cli/src/renderSetupWorkerLifecycle.ts index 751acdfbe9..9ae9e956e2 100644 --- a/packages/cli/src/renderSetupWorkerLifecycle.ts +++ b/packages/cli/src/renderSetupWorkerLifecycle.ts @@ -61,7 +61,7 @@ export function renderSetupResultFrom(stdout: string): unknown { } export function renderSetupErrorLine(error: unknown): string { - return prefixedLine(RENDER_SETUP_ERROR_PREFIX, normalizeErrorMessage(error)); + return "\n" + prefixedLine(RENDER_SETUP_ERROR_PREFIX, normalizeErrorMessage(error)); } export function renderSetupFailureFrom( diff --git a/packages/engine/src/services/browserManager.test.ts b/packages/engine/src/services/browserManager.test.ts index 08bb47d726..a4c522529a 100644 --- a/packages/engine/src/services/browserManager.test.ts +++ b/packages/engine/src/services/browserManager.test.ts @@ -1,7 +1,7 @@ // fallow-ignore-file code-duplication import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { execFileSync } from "node:child_process"; -import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from "node:fs"; +import { chmodSync, mkdirSync, mkdtempSync, rmSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; @@ -696,7 +696,7 @@ describe("resolveHeadlessShellPath", () => { for (const [directory, executable] of candidates) { const binary = join(cacheVersion, directory, executable); mkdirSync(join(binary, ".."), { recursive: true }); - writeFileSync(binary, ""); + writeFileSync(binary, "shell"); } const expectedBinary = join(cacheVersion, expectedDirectory, expectedExecutable); @@ -737,7 +737,7 @@ describe("resolveHeadlessShellPath", () => { ] as const) { const binary = join(cacheVersion, directory, executable); mkdirSync(join(binary, ".."), { recursive: true }); - writeFileSync(binary, ""); + writeFileSync(binary, "shell"); } const env = { ...process.env, HOME: home, USERPROFILE: home }; @@ -770,10 +770,10 @@ describe("resolveHeadlessShellPath", () => { "chrome-headless-shell", ); mkdirSync(join(binary, ".."), { recursive: true }); - writeFileSync(binary, ""); + writeFileSync(binary, "shell"); const olderBinary = binary.replace("linux-152.0.7928.2", "linux-99.0.1.1"); mkdirSync(join(olderBinary, ".."), { recursive: true }); - writeFileSync(olderBinary, ""); + writeFileSync(olderBinary, "shell"); // os.homedir() reads HOME on POSIX and USERPROFILE on Windows. const env = { ...process.env, HOME: home, USERPROFILE: home }; @@ -787,6 +787,75 @@ describe("resolveHeadlessShellPath", () => { } }); + it("skips a cached shell that is a folder or empty and uses the next older build", () => { + const home = mkdtempSync(join(tmpdir(), "hyperframes-engine-browser-empty-")); + try { + const shell = (version: string) => + join( + home, + ".cache", + "hyperframes", + "chrome", + "chrome-headless-shell", + `linux-${version}`, + "chrome-headless-shell-linux64", + "chrome-headless-shell", + ); + mkdirSync(shell("153.0.7990.1"), { recursive: true }); + for (const [version, content] of [ + ["152.0.7977.30", ""], + ["150.0.7871.124", "shell"], + ]) { + mkdirSync(join(shell(version), ".."), { recursive: true }); + writeFileSync(shell(version), content); + } + const env = { ...process.env, HOME: home, USERPROFILE: home }; + delete env.PRODUCER_HEADLESS_SHELL_PATH; + delete env.HYPERFRAMES_BROWSER_PATH; + const stdout = resolveHeadlessShellInSubprocess(env, { platform: "linux", arch: "x64" }); + + expect(stdout).toBe(shell("150.0.7871.124")); + } finally { + rmSync(home, { recursive: true, force: true }); + } + }); + + // Permission bits do not stop root, and Windows ignores them. + it.skipIf(process.platform === "win32" || process.getuid?.() === 0)( + "keeps scanning past a cached build it cannot read", + () => { + const home = mkdtempSync(join(tmpdir(), "hyperframes-engine-browser-unreadable-")); + const shell = (version: string) => + join( + home, + ".cache", + "hyperframes", + "chrome", + "chrome-headless-shell", + `linux-${version}`, + "chrome-headless-shell-linux64", + "chrome-headless-shell", + ); + const lockedDir = join(shell("152.0.7977.30"), ".."); + try { + for (const version of ["152.0.7977.30", "150.0.7871.124"]) { + mkdirSync(join(shell(version), ".."), { recursive: true }); + writeFileSync(shell(version), "shell"); + } + chmodSync(lockedDir, 0o000); + const env = { ...process.env, HOME: home, USERPROFILE: home }; + delete env.PRODUCER_HEADLESS_SHELL_PATH; + delete env.HYPERFRAMES_BROWSER_PATH; + const stdout = resolveHeadlessShellInSubprocess(env, { platform: "linux", arch: "x64" }); + + expect(stdout).toBe(shell("150.0.7871.124")); + } finally { + chmodSync(lockedDir, 0o755); + rmSync(home, { recursive: true, force: true }); + } + }, + ); + it("skips managed-cache builds newer than Chrome 150 on macOS 12", () => { const home = mkdtempSync(join(tmpdir(), "hyperframes-engine-browser-macos12-")); try { @@ -803,7 +872,7 @@ describe("resolveHeadlessShellPath", () => { ); for (const version of ["152.0.7977.30", "150.0.7871.124"]) { mkdirSync(join(shell(version), ".."), { recursive: true }); - writeFileSync(shell(version), ""); + writeFileSync(shell(version), "shell"); } const env = { ...process.env, HOME: home, USERPROFILE: home }; delete env.PRODUCER_HEADLESS_SHELL_PATH; diff --git a/packages/engine/src/services/browserManager.ts b/packages/engine/src/services/browserManager.ts index 5719f473c3..f4a4efe12e 100644 --- a/packages/engine/src/services/browserManager.ts +++ b/packages/engine/src/services/browserManager.ts @@ -8,7 +8,7 @@ import type { Browser, Page, PuppeteerNode } from "puppeteer-core"; import { execSync } from "child_process"; -import { existsSync, readdirSync } from "fs"; +import { existsSync, readdirSync, statSync } from "fs"; import { join } from "path"; import { homedir } from "os"; import { chromeMajorCeiling, exceedsChromeCeiling } from "./chromeHostCeiling.js"; @@ -168,6 +168,15 @@ function cachedHeadlessShellExecutable( return CACHED_HEADLESS_SHELL_EXECUTABLES[`${hostPlatform}/${hostArch}`]; } +function isNonEmptyFile(path: string): boolean { + try { + const stat = statSync(path, { throwIfNoEntry: false }); + return stat?.isFile() === true && stat.size > 0; + } catch { + return false; + } +} + function findCachedHeadlessShell(baseDir: string): string | undefined { if (!existsSync(baseDir)) return undefined; const executable = cachedHeadlessShellExecutable(); @@ -178,7 +187,7 @@ function findCachedHeadlessShell(baseDir: string): string | undefined { for (const version of versions) { if (exceedsChromeCeiling(version, ceiling)) continue; const binary = join(baseDir, version, ...executable); - if (existsSync(binary)) return binary; + if (isNonEmptyFile(binary)) return binary; } } catch { // Ignore unreadable cache directories and continue browser discovery.