diff --git a/.changeset/render-chrome-start-retries.md b/.changeset/render-chrome-start-retries.md new file mode 100644 index 00000000..b37e140b --- /dev/null +++ b/.changeset/render-chrome-start-retries.md @@ -0,0 +1,5 @@ +--- +"@cloudhal/cli": patch +--- + +`render_page` on Node gives a slow Chrome more chances to start. A launch that hangs past its start budget or exits is killed, and the renderer tries again up to three times. Each try uses a new profile, and the pause doubles from 1 s. The error says how long a launch may take and how many tries were made. A Chrome that crashes after it started is launched again by the next render, and its profile is removed (#207). diff --git a/packages/cli/src/render-chrome.ts b/packages/cli/src/render-chrome.ts index b464d07a..7474c127 100644 --- a/packages/cli/src/render-chrome.ts +++ b/packages/cli/src/render-chrome.ts @@ -44,88 +44,150 @@ function nodeSocket(ws: WebSocket): CdpSocket { }; } +export interface ChromeRendererOptions { + /** Stop Chrome this long after the last session closes (default 60 s). */ + idleMs?: number; + /** How long one launch may take to print its DevTools endpoint (default 20 s). */ + startMs?: number; + /** Launches before giving up, each with a fresh profile (default 3, #207). */ + launches?: number; + /** Wait before the second launch, doubled before each later one (default 1 s). */ + backoffMs?: number; +} + +/** A Chrome process this renderer started. */ +interface Launch { + readonly process: ChildProcess; + readonly endpoint: Promise; + readonly dir: string; +} + +const sleep = (ms: number) => new Promise((r) => setTimeout(r, ms)); + +/** Starts one headless Chrome on a new temporary profile. */ +function launchChrome(executable: string, startMs: number): Launch { + const dir = mkdtempSync(path.join(tmpdir(), "cloudhal-render-")); + const child = spawn( + executable, + [ + "--headless=new", + "--remote-debugging-port=0", + `--user-data-dir=${dir}`, + "--no-first-run", + "--no-default-browser-check", + "--hide-scrollbars", + "--mute-audio", + "about:blank", + ], + { stdio: ["ignore", "ignore", "pipe"] }, + ); + const endpoint = new Promise((resolve, reject) => { + let err = ""; + // A warm start is under a second; a cold one on a busy CI runner can take over 10 s. A launch + // that hangs past this is killed and the renderer launches again (session()). + const timer = setTimeout( + () => reject(new Error(`Chrome did not start within ${startMs / 1000} s`)), + startMs, + ); + child.stderr?.on("data", (chunk: Buffer) => { + err += chunk.toString(); + const m = /DevTools listening on (ws:\/\/\S+)/.exec(err); + if (m) { + clearTimeout(timer); + resolve(m[1] as string); + } + }); + child.once("exit", () => { + clearTimeout(timer); + reject(new Error("Chrome exited")); + }); + }); + endpoint.catch(() => {}); + const killOnExit = () => child.kill(); + process.once("exit", killOnExit); + child.once("exit", () => process.off("exit", killOnExit)); + return { process: child, endpoint, dir }; +} + +/** Stops a launch (forcefully if it ignores SIGTERM) and removes its profile. */ +function killChrome({ process: child, dir }: Launch): void { + // Chrome's helpers can still be writing into the profile as the main process exits: retry, and + // never let a leftover temporary directory become an uncaught exception. + const removeProfile = () => { + try { + rmSync(dir, { recursive: true, force: true, maxRetries: 5, retryDelay: 100 }); + } catch { + // the OS cleans its temporary directory + } + }; + if (child.exitCode !== null || child.signalCode !== null) { + removeProfile(); + return; + } + child.kill(); + // A Chrome still starting up can ignore SIGTERM and outlive this process: force it. + const force = setTimeout(() => child.kill("SIGKILL"), 2000); + force.unref?.(); + child.once("exit", () => clearTimeout(force)); + child.once("exit", removeProfile); +} + export function chromeRenderer( executable: string, - options: { idleMs?: number } = {}, -): PageRenderer { - let chrome: { process: ChildProcess; endpoint: Promise; dir: string } | undefined; + options: ChromeRendererOptions = {}, +): PageRenderer & { stop(): void } { + const startMs = options.startMs ?? 20_000; + const launches = Math.max(1, options.launches ?? 3); + const backoffMs = options.backoffMs ?? 1000; + let chrome: Launch | undefined; let active = 0; let idle: ReturnType | undefined; const stop = () => { - if (!chrome) return; - const { process: child, dir } = chrome; + const current = chrome; chrome = undefined; - child.kill(); - // A Chrome still starting up can ignore SIGTERM and outlive this process: force it. - const force = setTimeout(() => child.kill("SIGKILL"), 2000); - force.unref?.(); - child.once("exit", () => clearTimeout(force)); - // Chrome's helpers can still be writing into the profile as the main process exits: retry, - // and never let a leftover temporary directory become an uncaught exception. - child.once("exit", () => { - try { - rmSync(dir, { recursive: true, force: true, maxRetries: 5, retryDelay: 100 }); - } catch { - // the OS cleans its temporary directory - } - }); + if (current) killChrome(current); }; - const start = () => { - const dir = mkdtempSync(path.join(tmpdir(), "cloudhal-render-")); - const child = spawn( - executable, - [ - "--headless=new", - "--remote-debugging-port=0", - `--user-data-dir=${dir}`, - "--no-first-run", - "--no-default-browser-check", - "--hide-scrollbars", - "--mute-audio", - "about:blank", - ], - { stdio: ["ignore", "ignore", "pipe"] }, - ); - const endpoint = new Promise((resolve, reject) => { - let err = ""; - // A warm start is under a second, a cold one on a busy CI runner can take over 10 s; one that - // hangs past this is killed and launched once more (session()). - const timer = setTimeout(() => reject(new Error("Chrome did not start")), 20_000); - child.stderr?.on("data", (chunk: Buffer) => { - err += chunk.toString(); - const m = /DevTools listening on (ws:\/\/\S+)/.exec(err); - if (m) { - clearTimeout(timer); - resolve(m[1] as string); - } - }); - child.once("exit", () => { - clearTimeout(timer); - reject(new Error("Chrome exited")); - if (chrome?.process === child) chrome = undefined; - }); - }); - endpoint.catch(() => {}); - chrome = { process: child, endpoint, dir }; - const killOnExit = () => child.kill(); - process.once("exit", killOnExit); - child.once("exit", () => process.off("exit", killOnExit)); - return chrome; + // The running Chrome's endpoint, launching it when needed. A stuck or crashed cold start is + // killed and launched again with a new profile, with a growing pause, so a busy machine gets + // several chances before render_page fails (#207). The error names the last failure. + const endpoint = async (): Promise => { + let last: unknown; + for (let attempt = 0; attempt < launches; attempt++) { + if (attempt > 0) await sleep(backoffMs * 2 ** (attempt - 1)); + if (!chrome) { + const launched = launchChrome(executable, startMs); + // A Chrome that exits on its own (a crash) is launched again by the next session. + launched.process.once("exit", () => { + if (chrome !== launched) return; + chrome = undefined; + killChrome(launched); + }); + chrome = launched; + } + const current = chrome; + try { + return await current.endpoint; + } catch (error) { + last = error; + // Another session may already have replaced it: only drop this launch. + if (chrome === current) chrome = undefined; + killChrome(current); + } + } + const reason = last instanceof Error ? last.message : String(last); + throw new Error(`${reason} (${launches} ${launches === 1 ? "launch" : "launches"})`); }; return { + stop() { + clearTimeout(idle); + stop(); + }, async session() { clearTimeout(idle); - let url: string; - try { - url = await (chrome ?? start()).endpoint; - } catch { - // A stuck or crashed cold start: one fresh launch with a new profile. - stop(); - url = await start().endpoint; - } + const url = await endpoint(); const ws = new WebSocket(url); await new Promise((resolve, reject) => { ws.addEventListener("open", () => resolve(), { once: true }); diff --git a/packages/cli/test/render-chrome.test.ts b/packages/cli/test/render-chrome.test.ts index 33b5a3da..8be1f259 100644 --- a/packages/cli/test/render-chrome.test.ts +++ b/packages/cli/test/render-chrome.test.ts @@ -9,7 +9,7 @@ import os from "node:os"; import path from "node:path"; import { pathToFileURL } from "node:url"; import { renderPage } from "@cloudhal/mcp-server/render"; -import { afterAll, describe, expect, it } from "vitest"; +import { afterAll, beforeAll, describe, expect, it } from "vitest"; import { CAROUSEL_SCRIPT } from "../../astro-renderer/src/carousel-client.ts"; import { renderPagePreview } from "../../astro-renderer/src/preview.ts"; import { chromeRenderer, findChrome, localRender } from "../src/render-chrome.ts"; @@ -30,7 +30,16 @@ const PAGE = ` { let dir: string; + // One Chrome for every test here, started before them with the launch's own budget (#207): a + // cold start on a busy CI runner can take most of a minute, which must not eat a render's 50 s. + // Chrome that cannot start at all still fails every test, with the launch error. + let renderer: ReturnType; + beforeAll(async () => { + renderer = chromeRenderer(chrome as string, { idleMs: 300_000, startMs: 45_000 }); + await (await renderer.session()).close(); + }, 200_000); afterAll(async () => { + renderer?.stop(); if (dir) await rm(dir, { recursive: true, force: true }); }); @@ -39,7 +48,7 @@ describe.skipIf(!chrome)("render_page checks in Chrome", () => { const file = path.join(dir, "page.html"); await writeFile(file, PAGE); const r = await renderPage( - chromeRenderer(chrome as string, { idleMs: 10 }), + renderer, pathToFileURL(file).href, // Two widths, full page: the 20 s default is tight on a loaded CI runner (#35). { widths: [1440, 390], fullPage: true, timeoutMs: 50_000 }, @@ -74,7 +83,6 @@ describe.skipIf(!chrome)("render_page checks in Chrome", () => { expect(check("landmarks")?.examples).toEqual( expect.arrayContaining(["@1440: the page — 0 page headers (one is right)"]), ); - // The first test starts Chrome: up to two 20 s launch attempts (render-chrome.ts) plus the render. }, 120_000); it("checks the chrome: skip link, landmarks, a transparent header's contrast, focus under it", async () => { @@ -96,11 +104,11 @@ header{position:sticky;top:0;height:80px;color:#fff;margin-bottom:-80px}header a const run = async (name: string, html: string, widths: (1440 | 768 | 390)[] = [1440]) => { const file = path.join(dir, name); await writeFile(file, html); - const r = await renderPage( - chromeRenderer(chrome as string, { idleMs: 10 }), - pathToFileURL(file).href, - { widths, fullPage: false, timeoutMs: 50_000 }, - ); + const r = await renderPage(renderer, pathToFileURL(file).href, { + widths, + fullPage: false, + timeoutMs: 50_000, + }); return (id: string) => r.checks.find((c) => c.id === id); }; const b = await run("bad.html", bad); @@ -281,11 +289,11 @@ header{position:sticky;top:0;height:80px;color:#fff;margin-bottom:-80px}header a const render = async (name: string, html: string) => { const file = path.join(dir, name); await writeFile(file, html); - const r = await renderPage( - chromeRenderer(chrome as string, { idleMs: 10 }), - pathToFileURL(file).href, - { widths: [1440], fullPage: false, timeoutMs: 50_000 }, - ); + const r = await renderPage(renderer, pathToFileURL(file).href, { + widths: [1440], + fullPage: false, + timeoutMs: 50_000, + }); return (id: string) => r.checks.find((c) => c.id === id); }; await writeFile(path.join(dir, "theme.js"), "document.body.dataset.ran = '1';"); @@ -320,11 +328,11 @@ header{position:sticky;top:0;height:80px;color:#fff;margin-bottom:-80px}header a await new Promise((r) => server.listen(0, "127.0.0.1", r)); try { const { port } = server.address() as AddressInfo; - const r = await renderPage( - chromeRenderer(chrome as string, { idleMs: 10 }), - `http://127.0.0.1:${port}/`, - { widths: [1440], fullPage: false, timeoutMs: 50_000 }, - ); + const r = await renderPage(renderer, `http://127.0.0.1:${port}/`, { + widths: [1440], + fullPage: false, + timeoutMs: 50_000, + }); expect(r.checks.find((c) => c.id === "script-blocked")?.examples).toEqual([ expect.stringMatching(/an inline script .*would be blocked on the site by script-src-elem/), ]); @@ -341,23 +349,83 @@ describe("localRender", () => { }); }); +const STARTS_SECOND_TIME = `n=$(wc -l < "$(dirname "$0")/launches" | tr -d ' ') +[ "$n" -lt 2 ] && exit 1 +echo "DevTools listening on ws://127.0.0.1:9/devtools/browser/x" >&2 +exec sleep 30`; + describe("chromeRenderer", () => { - it.skipIf(process.platform === "win32")( - "launches a Chrome that fails to start once more, with a new profile", + const unix = process.platform !== "win32"; + // A fake Chrome: a shell script that logs each launch's arguments, then does `body`. + const fakeChrome = async (body: string) => { + const tmp = await mkdtemp(path.join(os.tmpdir(), "cloudhal-render-retry-")); + const log = path.join(tmp, "launches"); + const fake = path.join(tmp, "chrome"); + await writeFile(fake, `#!/bin/sh\necho "$$ $@" >> "${log}"\n${body}\n`, { mode: 0o755 }); + const launches = async () => (await readFile(log, "utf8")).trim().split("\n"); + return { tmp, fake, log, launches }; + }; + const profile = (line?: string) => /--user-data-dir=(\S+)/.exec(line ?? "")?.[1]; + const alive = (pid: number) => { + try { + process.kill(pid, 0); + return true; + } catch { + return false; + } + }; + + it.skipIf(!unix)( + "launches a Chrome that keeps exiting three times, each with a new profile", async () => { - const tmp = await mkdtemp(path.join(os.tmpdir(), "cloudhal-render-retry-")); - const log = path.join(tmp, "launches"); - const fake = path.join(tmp, "chrome"); - await writeFile(fake, `#!/bin/sh\necho "$@" >> "${log}"\nexit 1\n`, { mode: 0o755 }); + const f = await fakeChrome("exit 1"); try { - await expect(chromeRenderer(fake).session()).rejects.toThrow("Chrome exited"); - const launches = (await readFile(log, "utf8")).trim().split("\n"); - expect(launches).toHaveLength(2); - const profile = (line?: string) => /--user-data-dir=(\S+)/.exec(line ?? "")?.[1]; - expect(profile(launches[0])).not.toBe(profile(launches[1])); + await expect(chromeRenderer(f.fake, { backoffMs: 10 }).session()).rejects.toThrow( + "Chrome exited (3 launches)", + ); + const launches = await f.launches(); + expect(launches).toHaveLength(3); + expect(new Set(launches.map(profile)).size).toBe(3); } finally { - await rm(tmp, { recursive: true, force: true }); + await rm(f.tmp, { recursive: true, force: true }); } }, ); + + it.skipIf(!unix)( + "kills a launch that hangs past startMs and says how long it waited", + async () => { + const f = await fakeChrome("exec sleep 30"); + try { + // 1 s: a fake's shell can take a few hundred ms to start while vitest is busy, and it must + // have logged its pid before the renderer gives up on it. + const renderer = chromeRenderer(f.fake, { startMs: 1000, launches: 2, backoffMs: 10 }); + await expect(renderer.session()).rejects.toThrow( + "Chrome did not start within 1 s (2 launches)", + ); + const pids = (await f.launches()).map((line) => Number(line.split(" ")[0])); + expect(pids).toHaveLength(2); + for (let i = 0; i < 50 && pids.some(alive); i++) { + await new Promise((r) => setTimeout(r, 50)); + } + expect(pids.filter(alive)).toEqual([]); + } finally { + await rm(f.tmp, { recursive: true, force: true }); + } + }, + ); + + it.skipIf(!unix)("recovers when a later launch starts", async () => { + // The second launch prints an endpoint (on a closed port), so the renderer gets past the + // launch and fails only at connecting: the retry reached a started Chrome. + const f = await fakeChrome(STARTS_SECOND_TIME); + const renderer = chromeRenderer(f.fake, { backoffMs: 10 }); + try { + await expect(renderer.session()).rejects.toThrow("could not reach Chrome"); + expect(await f.launches()).toHaveLength(2); + } finally { + renderer.stop(); + await rm(f.tmp, { recursive: true, force: true }); + } + }); });