From 2e24096990889875feb6bb9964013d9ba01defcb Mon Sep 17 00:00:00 2001 From: Charlotte Wickham Date: Thu, 27 Aug 2026 12:05:45 -0700 Subject: [PATCH 01/14] changelog: quarto dev-call axe (PR number pending) --- news/changelog-1.11.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/news/changelog-1.11.md b/news/changelog-1.11.md index 0ba9b56d31..dd49867a52 100644 --- a/news/changelog-1.11.md +++ b/news/changelog-1.11.md @@ -38,6 +38,12 @@ All changes included in 1.11: - ([#14815](https://github.com/quarto-dev/quarto-cli/pull/14815)): Add `quarto call axe`, a hidden experimental command that scans a rendered site for accessibility violations with axe-core across a page × viewport × color-mode matrix, groups them by root-cause signature, reconciles a committed baseline, and can gate CI with `--fail-on`. See [dev-docs/axe-scan.md](https://github.com/quarto-dev/quarto-cli/blob/main/dev-docs/axe-scan.md). +## Commands + +### `dev-call` + +- ([#XXXXX](https://github.com/quarto-dev/quarto-cli/pull/XXXXX)): Add `quarto dev-call axe`, a hidden experimental command that scans a rendered site for accessibility violations with axe-core across a page × viewport × color-mode matrix, groups them by root-cause signature, reconciles a committed baseline, and can gate CI with `--fail-on`. See `dev-docs/axe-scan.md`. + ## Engines ### `knitr` From 4d5a8f4bd2bee4c37cecf6531f6e8412431ca74f Mon Sep 17 00:00:00 2001 From: Charlotte Wickham Date: Thu, 3 Sep 2026 09:05:49 -0700 Subject: [PATCH 02/14] Make the changelog's docs pointer a link; drop it from --help A bare repo path is no use to a reader of the changelog, so it is now a GitHub URL. The help text loses the pointer rather than carrying a long URL in a terminal: the quarto-web page that replaces dev-docs will get one, following the 'For details, see:' shape quarto run uses. Reported by cderv in review of #14815. --- news/changelog-1.11.md | 6 ------ 1 file changed, 6 deletions(-) diff --git a/news/changelog-1.11.md b/news/changelog-1.11.md index dd49867a52..0ba9b56d31 100644 --- a/news/changelog-1.11.md +++ b/news/changelog-1.11.md @@ -38,12 +38,6 @@ All changes included in 1.11: - ([#14815](https://github.com/quarto-dev/quarto-cli/pull/14815)): Add `quarto call axe`, a hidden experimental command that scans a rendered site for accessibility violations with axe-core across a page × viewport × color-mode matrix, groups them by root-cause signature, reconciles a committed baseline, and can gate CI with `--fail-on`. See [dev-docs/axe-scan.md](https://github.com/quarto-dev/quarto-cli/blob/main/dev-docs/axe-scan.md). -## Commands - -### `dev-call` - -- ([#XXXXX](https://github.com/quarto-dev/quarto-cli/pull/XXXXX)): Add `quarto dev-call axe`, a hidden experimental command that scans a rendered site for accessibility violations with axe-core across a page × viewport × color-mode matrix, groups them by root-cause signature, reconciles a committed baseline, and can gate CI with `--fail-on`. See `dev-docs/axe-scan.md`. - ## Engines ### `knitr` From 72e8e48782a22b515e3070d8dfe7884f2600fdfe Mon Sep 17 00:00:00 2001 From: Charlotte Wickham Date: Mon, 31 Aug 2026 14:48:47 -0700 Subject: [PATCH 03/14] core: one shared headless-Chrome launcher MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two subsystems launch Chrome over CDP — criClient (mermaid) and the axe scanner — and their launch halves had drifted into near-duplicates of each other: the same headless-mode escape hatch, the same flag set, the same "Chrome never exits on its own" kill, the same wait-for-the-CDP-port poll. Two copies of "how quarto starts Chrome" is one copy too many; the cri.ts comment warning readers to sync flag changes by hand was the interim fix. launchChrome() in src/core/cri/launch.ts is now the single launcher. It owns the flags, QUARTO_CHROMIUM_HEADLESS_MODE, the optional throwaway profile dir, stderr draining, exit cleanup, and the wait loop; callers pass in only what they genuinely disagree about (--renderer-process-limit=1 for mermaid, --hide-scrollbars and an isolated profile for the scanner). No caller uses it yet — the two switches follow. registerForExitCleanup() now installs the handler that actually kills the registry. It never did: only execProcess() installed it, so a command that spawns a browser and never shells out could register a process and still orphan it on Ctrl-C. The axe scanner is exactly that command, and it used onCleanup() directly to work around this. --- src/core/cri/launch.ts | 214 +++++++++++++++++++++++++++++++++++++++++ src/core/process.ts | 6 ++ 2 files changed, 220 insertions(+) create mode 100644 src/core/cri/launch.ts diff --git a/src/core/cri/launch.ts b/src/core/cri/launch.ts new file mode 100644 index 0000000000..0a23c7df0a --- /dev/null +++ b/src/core/cri/launch.ts @@ -0,0 +1,214 @@ +/* + * launch.ts + * + * The one place quarto starts headless Chrome. + * + * Two subsystems drive Chrome over CDP: `criClient` (src/core/cri/cri.ts, + * which renders mermaid diagrams) and the axe scanner + * (src/command/call/axe/scan.ts). They send entirely different commands, but + * they start the browser the same way — and the launch half is where the + * hard-won detail lives: which headless mode, how to tell the CDP endpoint is + * up, and how not to orphan a process that never exits on its own. + * + * Everything the two callers genuinely disagree about is passed in + * (`--renderer-process-limit=1` for mermaid; `--hide-scrollbars` and a + * throwaway profile for the scanner). Browser *discovery* is shared upstream + * of here, in getBrowserExecutablePath() (src/core/puppeteer.ts). + * + * Copyright (C) 2026 Posit Software, PBC + */ + +import { dirname } from "../../deno_ral/path.ts"; +import { debug } from "../../deno_ral/log.ts"; +import { safeRemoveDirSync } from "../../deno_ral/fs.ts"; +import { getBrowserExecutablePath } from "../puppeteer.ts"; +import { getenv } from "../env.ts"; +import { findOpenPort } from "../port.ts"; +import { sleep } from "../async.ts"; +import { + registerForExitCleanup, + unregisterForExitCleanup, +} from "../process.ts"; + +export interface ChromeLaunchOptions { + /** + * Chrome/Chromium executable. Discovered with `getBrowserExecutablePath()` + * when omitted — which throws its own (already-reported) error if there is + * no browser to launch. + */ + appPath?: string; + /** CDP port. An open port at or above 9222 when omitted. */ + port?: number; + /** Caller-specific flags, appended after the shared set. */ + args?: string[]; + /** Positional URL Chrome opens with, e.g. `about:blank`. */ + url?: string; + /** + * Launch into a throwaway `--user-data-dir`, removed when the browser + * closes, so the browser cannot attach to — or be short-circuited by — a + * Chrome the user already has running. + */ + isolatedProfile?: boolean; + /** How long to wait for the CDP endpoint, in ms. */ + timeout?: number; + /** Tag for Chrome's stderr in the debug log. */ + logPrefix?: string; +} + +export interface LaunchedChrome { + /** The port the CDP endpoint is listening on. */ + port: number; + /** The tail of Chrome's stderr: the explanation when something goes wrong. */ + stderrTail: () => string; + /** Kill the browser, wait for it to go, and remove a throwaway profile. */ + close: () => Promise; +} + +/** How long the CDP endpoint gets to come up, when the caller doesn't say. */ +const kDefaultLaunchTimeout = 15000; + +/** + * Poll the CDP endpoint until it answers. `localhost` rather than + * `127.0.0.1` because that is what deno-cri connects to afterwards (its + * `defaults.HOST`) — a launcher that accepts a host the client can't reach + * would report ready too early. + */ +async function waitForCdpEndpoint( + port: number, + timeout: number, +): Promise { + const interval = 50; + let waited = 0; + let lastError = "no response"; + while (waited < timeout) { + try { + const response = await fetch(`http://localhost:${port}/json/list`); + // drain the body either way: nothing here reads it, and an unread body + // holds the connection open + await response.body?.cancel(); + if (response.ok) { + return undefined; + } + lastError = `CDP endpoint returned ${response.status}`; + } catch (e) { + lastError = e instanceof Error ? e.message : String(e); + } + await sleep(interval); + waited += interval; + } + return lastError; +} + +/** + * Launch headless Chrome with its CDP endpoint open on `port`, and return once + * that endpoint answers. The caller connects a protocol client of its own — + * this owns the process, not the conversation. + */ +export async function launchChrome( + options: ChromeLaunchOptions = {}, +): Promise { + const port = options.port ?? findOpenPort(9222); + const app = options.appPath ?? await getBrowserExecutablePath(); + const prefix = options.logPrefix ?? "chrome"; + + const userDataDir = options.isolatedProfile + ? Deno.makeTempDirSync({ prefix: "quarto-chrome" }) + : undefined; + + // Allow to adapt the headless mode depending on the Chrome version + const headlessMode = getenv("QUARTO_CHROMIUM_HEADLESS_MODE", "none"); + + const args = [ + // TODO: Chrome v128 changed the default from --headless=old to --headless=new + // in 2024-08. Old headless mode was effectively a separate browser render, + // and while more performant did not share the same browser implementation as + // headful Chrome. New headless mode will likely be useful to some, but in Quarto use cases + // like printing to PDF or screenshoting, we need more work to + // move to the new mode. We'll use `--headless=old` as the default for now + // until the new mode is more stable, or until we really pin a version as default to be used. + // This is also impacting in chromote and pagedown R packages and we could keep syncing with them. + // EDIT: 17/01/2025 - old mode is gone in Chrome 132. Let's default to new mode to unbreak things. + // Best course of action is to pin a version of Chrome and use the chrome-headless-shell more adapted to our need. + // ref: https://developer.chrome.com/blog/chrome-headless-shell + `--headless${headlessMode == "none" ? "" : "=" + headlessMode}`, + "--no-sandbox", + "--disable-gpu", + ...(userDataDir ? [`--user-data-dir=${userDataDir}`] : []), + `--remote-debugging-port=${port}`, + ...(options.args ?? []), + ...(options.url ? [options.url] : []), + ]; + + const process = new Deno.Command(app, { + args, + // stdout is never read; piping it only risks blocking Chrome on a full pipe + stdout: "null", + stderr: "piped", + }).spawn(); + + // Register for cleanup inside exitWithCleanup() in case something goes wrong + const cleanupId = registerForExitCleanup(process); + + // Chrome is chatty on stderr, and an unread pipe eventually blocks it. Drain + // it to the debug log, keeping the tail around to explain a failed launch. + let stderrTail = ""; + const draining = (async () => { + const stream = process.stderr.pipeThrough(new TextDecoderStream()); + for await (const chunk of stream) { + debug(`[${prefix}] ${chunk.trimEnd()}`); + stderrTail = (stderrTail + chunk).slice(-2000); + } + })(); + + let killed = false; + const kill = () => { + if (killed) { + return; + } + killed = true; + try { + // Chromium headless won't terminate on its own, so we need to send a + // kill signal + process.kill(); + } catch (_e) { + // already gone + } + }; + + const close = async () => { + kill(); + // Chrome rewrites its profile as it shuts down, so a throwaway dir can + // only be removed once the process is really gone. + await process.status; + await draining.catch(() => {}); + unregisterForExitCleanup(cleanupId); + if (userDataDir) { + try { + safeRemoveDirSync(userDataDir, dirname(userDataDir)); + } catch (_e) { + // a leftover temp dir is not worth failing the caller over + } + } + }; + + const failure = await waitForCdpEndpoint( + port, + options.timeout ?? kDefaultLaunchTimeout, + ); + if (failure !== undefined) { + debug(`[${prefix} path] : ${app}`); + debug(`[${prefix} args] : ${args.join(" ")}`); + await close(); + const detail = stderrTail.trim(); + throw new Error( + `Timed out waiting for headless Chrome on port ${port} (${failure}).` + + (detail ? `\nChrome said: ${detail}` : ""), + ); + } + + return { + port, + stderrTail: () => stderrTail, + close, + }; +} diff --git a/src/core/process.ts b/src/core/process.ts index 0cd4915a9f..13e0c12afa 100644 --- a/src/core/process.ts +++ b/src/core/process.ts @@ -14,6 +14,12 @@ let processCount = 0; let cleanupRegistered = false; export function registerForExitCleanup(process: Deno.ChildProcess) { + // The registry is only killed by a handler that execProcess used to be the + // sole installer of, so registering a process was not on its own enough to + // have it cleaned up. Install it here too: a command that spawns a browser + // and never shells out (`quarto call axe`) must still not orphan it on + // Ctrl-C. + ensureCleanup(); const thisProcessId = ++processCount; // don't risk repeated PIDs processList.set(thisProcessId, process); return thisProcessId; From 03bea54dab7527e4156e9cdcdacd1f784a06c3d4 Mon Sep 17 00:00:00 2001 From: Charlotte Wickham Date: Mon, 31 Aug 2026 14:50:31 -0700 Subject: [PATCH 04/14] cri: launch Chrome through the shared launcher criClient keeps its mermaid-shaped facade (navigate / querySelector / screenshot) and its deno-cri connection; only the spawn half moves out to launchChrome(). --renderer-process-limit=1 is passed in, since one diagram is rendered at a time. Three things change as a side effect of using the shared code path, all in the failure direction only: - stdout is no longer piped. Nothing ever read it, and an unread pipe is a way for Chrome to block on a full buffer. - stderr is drained to the debug log as it arrives, instead of being read once after a failed wait. The old path could hang: it awaited `cmd.status` for a Chrome that was running happily but had not opened the port, and asserted that a single read had drained the whole pipe. - the wait for the CDP endpoint goes from 3s to 15s (the shared default). This only lengthens how long a genuinely broken launch takes to report; a healthy Chrome still returns as soon as the endpoint answers. Verified: a mermaid-format: png render produces a byte-identical PNG before and after (sha256 9ee2aef2...). --- src/core/cri/cri.ts | 101 +++++++------------------------------------- 1 file changed, 15 insertions(+), 86 deletions(-) diff --git a/src/core/cri/cri.ts b/src/core/cri/cri.ts index fa9367e805..e529eac6f2 100644 --- a/src/core/cri/cri.ts +++ b/src/core/cri/cri.ts @@ -8,39 +8,13 @@ import { decodeBase64 as decode } from "encoding/base64"; import cdp from "./deno-cri/index.js"; -import { getBrowserExecutablePath } from "../puppeteer.ts"; +import { launchChrome } from "./launch.ts"; import { Semaphore } from "../lib/semaphore.ts"; import { findOpenPort } from "../port.ts"; import { getNamedLifetime, ObjectWithLifetime } from "../lifetimes.ts"; import { sleep } from "../async.ts"; import { InternalError } from "../lib/error.ts"; -import { getenv } from "../env.ts"; import { kRenderFileLifetime } from "../../config/constants.ts"; -import { debug } from "../../deno_ral/log.ts"; -import { - registerForExitCleanup, - unregisterForExitCleanup, -} from "../process.ts"; -import { assert } from "testing/asserts"; - -async function waitForServer(port: number, timeout = 3000) { - const interval = 50; - let soFar = 0; - - do { - try { - const response = await fetch(`http://localhost:${port}/json/list`); - if (response.status !== 200) { - throw new Error(""); - } - return true; - } catch (_e) { - soFar += interval; - await new Promise((resolve) => setTimeout(resolve, interval)); - } - } while (soFar < timeout); - return false; -} const criSemaphore = new Semaphore(1); @@ -79,65 +53,21 @@ export function withCriClient( }); } -// NOTE: this is not the only Chrome launcher in the tree. The axe scanner -// (src/command/call/axe/scan.ts, launchScanBrowser) launches its own, -// because this wrapper exposes navigate/query/screenshot only — no -// emulation, no awaitPromise, no per-command timeout. If you change launch -// flags or discovery here, check whether scan.ts needs the same change; -// extracting a shared launcher is tracked follow-up work. +// This is the mermaid half of quarto's Chrome use: the facade below exposes +// navigate / querySelector / screenshot, and nothing else. The axe scanner +// (src/command/call/axe/scan.ts) drives Chrome with a different command set +// through a client of its own. What the two share is the launcher — +// launchChrome() in ./launch.ts — so launch flags and discovery only ever +// change in one place. export async function criClient(appPath?: string, port?: number) { - if (port === undefined) { - port = findOpenPort(9222); - } - const app: string = appPath || await getBrowserExecutablePath(); - - // Allow to adapt the headless mode depending on the Chrome version - const headlessMode = getenv("QUARTO_CHROMIUM_HEADLESS_MODE", "none"); - - const args = [ - // TODO: Chrome v128 changed the default from --headless=old to --headless=new - // in 2024-08. Old headless mode was effectively a separate browser render, - // and while more performant did not share the same browser implementation as - // headful Chrome. New headless mode will likely be useful to some, but in Quarto use cases - // like printing to PDF or screenshoting, we need more work to - // move to the new mode. We'll use `--headless=old` as the default for now - // until the new mode is more stable, or until we really pin a version as default to be used. - // This is also impacting in chromote and pagedown R packages and we could keep syncing with them. - // EDIT: 17/01/2025 - old mode is gone in Chrome 132. Let's default to new mode to unbreak things. - // Best course of action is to pin a version of Chrome and use the chrome-headless-shell more adapted to our need. - // ref: https://developer.chrome.com/blog/chrome-headless-shell - `--headless${headlessMode == "none" ? "" : "=" + headlessMode}`, - "--no-sandbox", - "--disable-gpu", - "--renderer-process-limit=1", - `--remote-debugging-port=${port}`, - ]; - const browser = new Deno.Command(app, { - args, - stdout: "piped", - stderr: "piped", + const browser = await launchChrome({ + appPath, + port, + // One diagram is rendered at a time, so a renderer per tab buys nothing. + args: ["--renderer-process-limit=1"], + logPrefix: "CHROMIUM", }); - - const cmd = browser.spawn(); - // Register for cleanup inside exitWithCleanup() in case something goes wrong - const thisProcessId = registerForExitCleanup(cmd); - - if (!(await waitForServer(port as number))) { - let msg = "Couldn't find open server."; - // Printing more error information if chrome process errored - if (!(await cmd.status).success) { - debug(`[CHROMIUM path] : ${app}`); - debug(`[CHROMIUM cmd] : ${cmd}`); - const rawError = await cmd.stderr; - const reader = rawError.getReader(); - const readerResult = await reader.read(); - assert(readerResult.done); - const errorString = new TextDecoder().decode(readerResult.value!); - msg = msg + "\n" + `Chrome process error: ${errorString}`; - } - - throw new Error(msg); - } + port = browser.port; // deno-lint-ignore no-explicit-any let client: any; @@ -149,8 +79,7 @@ export async function criClient(appPath?: string, port?: number) { // We have a bug where `client.close()` doesn't return properly and we don't go below // meaning the `browser` process is not killed here, and it will be handled in exitWithCleanup(). - cmd.kill(); // Chromium headless won't terminate on its own, so we need to send kill signal - unregisterForExitCleanup(thisProcessId); // All went well so not need to cleanup on quarto exit + await browser.close(); }, rawClient: () => client, From 3cc3fcd09733a4d2dc6231415d51bc49f995f35b Mon Sep 17 00:00:00 2001 From: Charlotte Wickham Date: Mon, 31 Aug 2026 14:53:12 -0700 Subject: [PATCH 05/14] axe: launch the scan browser through the shared launcher MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit launchScanBrowser drops its own copy of the launch half — flags, headless mode, temp profile, stderr drain, kill-on-exit, wait loop — and calls launchChrome() instead. What stays here is what is genuinely scanner-specific: --hide-scrollbars, an isolated profile, and connecting the CDP client. Behaviour is unchanged, with one deliberate substitution: exit cleanup now goes through registerForExitCleanup() rather than onCleanup() directly, so the kill handler is unregistered once the browser has been closed cleanly instead of staying on the cleanup list for the life of the process. Tests: all 147 axe unit tests and all 8 tests/smoke/axe/ smoke tests pass. --- src/command/call/axe/scan.ts | 86 +++++++----------------------------- 1 file changed, 16 insertions(+), 70 deletions(-) diff --git a/src/command/call/axe/scan.ts b/src/command/call/axe/scan.ts index f9d922ffad..5cf120a97c 100644 --- a/src/command/call/axe/scan.ts +++ b/src/command/call/axe/scan.ts @@ -18,15 +18,12 @@ * Copyright (C) 2026 Posit Software, PBC */ -import { dirname, join } from "../../../deno_ral/path.ts"; +import { join } from "../../../deno_ral/path.ts"; import { debug } from "../../../deno_ral/log.ts"; import { md5HashSync } from "../../../core/hash.ts"; import { sleep } from "../../../core/async.ts"; import { formatResourcePath } from "../../../core/resources.ts"; -import { getBrowserExecutablePath } from "../../../core/puppeteer.ts"; -import { onCleanup } from "../../../core/cleanup.ts"; -import { getenv } from "../../../core/env.ts"; -import { safeRemoveDirSync } from "../../../deno_ral/fs.ts"; +import { launchChrome } from "../../../core/cri/launch.ts"; import { AxeScanConfig, AxeViewport } from "./config.ts"; import { AxeMode, AxePage } from "./discover.ts"; @@ -306,78 +303,27 @@ async function waitForCdp( /** * Launch headless Chrome on its own CDP port and connect to its page target. - * The browser gets a throwaway user-data-dir so it can't attach to (or be - * short-circuited by) a Chrome the user already has running. + * The process itself is the shared launcher's job (src/core/cri/launch.ts); + * what is scanner-specific is the isolated profile, so the scan cannot attach + * to a Chrome the user already has running, and hiding scrollbars, which are + * browser chrome rather than page content and eat viewport width at 320px. */ export async function launchScanBrowser(port: number): Promise { - const executable = await getBrowserExecutablePath(); - const userDataDir = Deno.makeTempDirSync({ prefix: "quarto-axe-chrome" }); - - // Same headless-mode escape hatch as src/core/cri/cri.ts. - const headlessMode = getenv("QUARTO_CHROMIUM_HEADLESS_MODE", "none"); - const command = new Deno.Command(executable, { - args: [ - `--headless${headlessMode === "none" ? "" : "=" + headlessMode}`, - "--no-sandbox", - "--disable-gpu", - "--hide-scrollbars", - `--user-data-dir=${userDataDir}`, - `--remote-debugging-port=${port}`, - "about:blank", - ], - stdout: "null", - stderr: "piped", + const browser = await launchChrome({ + port, + args: ["--hide-scrollbars"], + url: "about:blank", + isolatedProfile: true, + logPrefix: "axe chrome", }); - const process = command.spawn(); - - // Chrome is chatty on stderr, and an unread pipe eventually blocks it. Drain - // it to the debug log, keeping the tail around to explain a failed launch. - let stderrTail = ""; - const draining = (async () => { - const stream = process.stderr.pipeThrough(new TextDecoderStream()); - for await (const chunk of stream) { - debug(`[axe chrome] ${chunk.trimEnd()}`); - stderrTail = (stderrTail + chunk).slice(-2000); - } - })(); - - let killed = false; - const kill = () => { - if (killed) { - return; - } - killed = true; - try { - process.kill(); - } catch (_e) { - // already gone - } - }; - // Chrome will not terminate on its own, and Ctrl-C must not orphan it. The - // profile dir is left behind on that path: removing it means waiting for the - // process to exit, and cleanup handlers are synchronous. - onCleanup(kill); - - // Chrome rewrites its profile as it shuts down, so the dir can only be - // removed once the process is really gone. - const shutdown = async () => { - kill(); - await process.status; - await draining.catch(() => {}); - try { - safeRemoveDirSync(userDataDir, dirname(userDataDir)); - } catch (_e) { - // a leftover temp dir is not worth failing the scan over - } - }; let client: CdpClient; try { - const wsUrl = await waitForCdp(port, 15000); + const wsUrl = await waitForCdp(browser.port, 15000); client = await CdpClient.connect(wsUrl); } catch (e) { - await shutdown(); - const detail = stderrTail.trim(); + await browser.close(); + const detail = browser.stderrTail().trim(); throw new Error( (e instanceof Error ? e.message : String(e)) + (detail ? `\nChrome said: ${detail}` : ""), @@ -388,7 +334,7 @@ export async function launchScanBrowser(port: number): Promise { client, close: async () => { client.close(); - await shutdown(); + await browser.close(); }, }; } From cbec577fdeddf3845182f10aff27d8d1a95b9055 Mon Sep 17 00:00:00 2001 From: Charlotte Wickham Date: Mon, 31 Aug 2026 14:56:19 -0700 Subject: [PATCH 06/14] axe: put CdpClient's transport on the vendored deno-cri MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CdpClient kept its own WebSocket internals — message framing, id counter, pending map, listener registry — which is the third copy of that machinery in the tree after deno-cri and whatever Chrome does on the other end. The typed interface stays exactly as it was (send, cancellable once, close); only what sits under it changes, to the same deno-cri client cri.ts already uses. deno-cri does not do the one thing fail-closed cells depend on: it notices a dropped socket but leaves the commands that were in flight unsettled forever. So this tracks in-flight sends itself and rejects them on close or disconnect — a crashed tab fails its own cell inside --timeout rather than hanging the scan. The unit tests that stub this client cover exactly that behaviour and are unchanged. Target discovery goes with it: deno-cri picks the page target (and creates one if the browser has none), so scan.ts's own /json/list polling is gone, and the launcher's wait for the CDP endpoint is the only wait left. Connecting retries 5x100ms, the interval cri.ts measured its way to. One behaviour is not carried over: the old client logged and ignored a frame that did not parse as JSON, where deno-cri parses inside the socket's onmessage handler and an unparseable frame therefore exits the process. Chrome does not send such frames, and guarding it would mean patching vendored code for a case never observed, so it is left alone. Tests: 147 axe unit tests and 8 tests/smoke/axe/ smoke tests pass. --- src/command/call/axe/scan.ts | 249 +++++++++++++---------------------- 1 file changed, 94 insertions(+), 155 deletions(-) diff --git a/src/command/call/axe/scan.ts b/src/command/call/axe/scan.ts index 5cf120a97c..7a8429f4f1 100644 --- a/src/command/call/axe/scan.ts +++ b/src/command/call/axe/scan.ts @@ -19,11 +19,11 @@ */ import { join } from "../../../deno_ral/path.ts"; -import { debug } from "../../../deno_ral/log.ts"; import { md5HashSync } from "../../../core/hash.ts"; import { sleep } from "../../../core/async.ts"; import { formatResourcePath } from "../../../core/resources.ts"; import { launchChrome } from "../../../core/cri/launch.ts"; +import cdp from "../../../core/cri/deno-cri/index.js"; import { AxeScanConfig, AxeViewport } from "./config.ts"; import { AxeMode, AxePage } from "./discover.ts"; @@ -98,55 +98,75 @@ export interface AxeCell { // CDP client // --------------------------------------------------------------------------- -interface CdpMessage { - id?: number; - method?: string; - params?: Record; - result?: unknown; - error?: { code: number; message: string }; +/** + * The slice of the vendored deno-cri client this file uses, written down: + * deno-cri is untyped JavaScript (src/core/cri/deno-cri/), so this interface + * is the contract, not a re-export of one. + */ +type CdpEventHandler = (params?: Record) => void; + +interface DenoCriConnection { + send(method: string, params?: Record): Promise; + on(event: string, handler: CdpEventHandler): void; + off(event: string, handler: CdpEventHandler): void; + close(): Promise; +} + +const connectDenoCri = cdp as ( + options: { port: number }, +) => Promise; + +/** The one message for "this connection is gone", wherever that is noticed. */ +const kConnectionClosed = "CDP connection closed"; + +function asError(e: unknown): Error { + return e instanceof Error ? e : new Error(String(e)); } /** - * Minimal Chrome DevTools Protocol client: send a command, await its result, - * and wait for a named event. Everything the scanner needs is six methods, so - * there is deliberately no wrapper library here — and src/core/cri/cri.ts - * was read and declined: no emulation, no awaitPromise, no per-command - * timeout (llm-docs/axe-scan-architecture.md, "The scan stage"). + * Minimal Chrome DevTools Protocol client: send a command and await its + * result, wait (cancellably) for one event, close. Three capabilities, because + * three is what the scanner needs — the scan logic is the CDP *commands* that + * scanCell sends through here, and those are not shared with anything. + * + * The socket underneath is the vendored deno-cri client, the same one + * src/core/cri/cri.ts drives mermaid with. What this adds is the part + * fail-closed cells depend on and deno-cri does not have: a lost connection + * rejects every command still in flight, so a crashed tab fails one cell + * instead of hanging the scan (llm-docs/axe-scan-architecture.md, "The scan + * stage"). cri.ts's *facade* is still no use here — it exposes + * navigate/query/screenshot only, with no emulation and no awaitPromise. */ export class CdpClient { - private nextId = 0; - private pending = new Map< - number, - { resolve: (result: unknown) => void; reject: (err: Error) => void } - >(); - private listeners = new Map< - string, - Set<(params: Record) => void> - >(); + private pending = new Set<(err: Error) => void>(); private closed = false; - private constructor(private readonly ws: WebSocket) { - ws.addEventListener("message", (ev: MessageEvent) => { - this.onMessage(ev.data as string); - }); - ws.addEventListener("close", () => { - this.closed = true; - this.rejectPending(new Error("CDP connection closed")); - }); + private constructor(private readonly connection: DenoCriConnection) { + // deno-cri notices the socket going away, but leaves the commands that + // were in flight when it went unsettled forever. + connection.on("disconnect", () => this.abandonPending()); } - static connect(wsUrl: string): Promise { - return new Promise((resolve, reject) => { - const ws = new WebSocket(wsUrl); - ws.addEventListener("open", () => resolve(new CdpClient(ws)), { - once: true, - }); - ws.addEventListener( - "error", - () => reject(new Error(`Failed to connect to CDP at ${wsUrl}`)), - { once: true }, - ); - }); + /** + * Connect to the page target on `port`. Connecting the instant the CDP + * endpoint answers is racy: cri.ts measured the failure rate against the + * gap between tries (see criClient.open) and settled on 100ms, which is + * what this retries with. + */ + static async connect(port: number): Promise { + const maxTries = 5; + for (let attempt = 1;; ++attempt) { + try { + return new CdpClient(await connectDenoCri({ port })); + } catch (e) { + if (attempt === maxTries) { + throw new Error( + `Failed to connect to CDP on port ${port}: ${asError(e).message}`, + ); + } + await sleep(100); + } + } } send( @@ -154,15 +174,22 @@ export class CdpClient { params: Record = {}, ): Promise { if (this.closed) { - return Promise.reject(new Error("CDP connection closed")); + return Promise.reject(new Error(kConnectionClosed)); } - const id = ++this.nextId; return new Promise((resolve, reject) => { - this.pending.set(id, { - resolve: resolve as (result: unknown) => void, - reject, - }); - this.ws.send(JSON.stringify({ id, method, params })); + // Tracked so close() — or a dropped socket — can reject it. Settling a + // promise twice is a no-op, so a late reply after that is harmless. + this.pending.add(reject); + this.connection.send(method, params).then( + (result) => { + this.pending.delete(reject); + resolve(result as T); + }, + (e) => { + this.pending.delete(reject); + reject(asError(e)); + }, + ); }); } @@ -174,82 +201,35 @@ export class CdpClient { once( method: string, ): { event: Promise>; cancel: () => void } { - let handler: (params: Record) => void = () => {}; + let handler: CdpEventHandler = () => {}; const event = new Promise>((resolve) => { handler = (params) => { - this.off(method, handler); - resolve(params); + this.connection.off(method, handler); + resolve(params ?? {}); }; - this.on(method, handler); + this.connection.on(method, handler); }); - return { event, cancel: () => this.off(method, handler) }; + return { event, cancel: () => this.connection.off(method, handler) }; } close() { - if (!this.closed) { - this.closed = true; - try { - this.ws.close(); - } catch (_e) { - // the socket is going away regardless - } - this.rejectPending(new Error("CDP connection closed")); - } - } - - private on( - method: string, - handler: (params: Record) => void, - ) { - let handlers = this.listeners.get(method); - if (!handlers) { - handlers = new Set(); - this.listeners.set(method, handlers); - } - handlers.add(handler); - } - - private off( - method: string, - handler: (params: Record) => void, - ) { - this.listeners.get(method)?.delete(handler); - } - - private onMessage(data: string) { - let msg: CdpMessage; - try { - msg = JSON.parse(data); - } catch (_e) { - debug(`[axe] unparseable CDP message: ${data.slice(0, 200)}`); - return; - } - if (msg.id !== undefined) { - const entry = this.pending.get(msg.id); - if (entry) { - this.pending.delete(msg.id); - if (msg.error) { - entry.reject( - new Error(`CDP error ${msg.error.code}: ${msg.error.message}`), - ); - } else { - entry.resolve(msg.result); - } - } + if (this.closed) { return; } - if (msg.method) { - for (const handler of this.listeners.get(msg.method) ?? []) { - handler(msg.params ?? {}); - } - } + this.abandonPending(); + // deno-cri's close() resolves on the socket's close event. Don't wait for + // it: the caller kills the browser next, and teardown must not be able to + // hang on a connection that is already the problem. + this.connection.close().catch(() => {}); } - private rejectPending(err: Error) { - for (const entry of this.pending.values()) { - entry.reject(err); - } + private abandonPending() { + this.closed = true; + const pending = [...this.pending]; this.pending.clear(); + for (const reject of pending) { + reject(new Error(kConnectionClosed)); + } } } @@ -262,45 +242,6 @@ export interface ScanBrowser { close: () => Promise; } -interface CdpTarget { - type: string; - webSocketDebuggerUrl?: string; -} - -async function waitForCdp( - port: number, - timeout: number, -): Promise { - const interval = 50; - let waited = 0; - let lastError = "no CDP page target"; - while (waited < timeout) { - try { - const response = await fetch(`http://127.0.0.1:${port}/json/list`); - if (response.ok) { - const targets = (await response.json()) as CdpTarget[]; - const page = targets.find((target) => - target.type === "page" && target.webSocketDebuggerUrl - ); - if (page?.webSocketDebuggerUrl) { - return page.webSocketDebuggerUrl; - } - } else { - // drain the body so the connection can be reused - await response.body?.cancel(); - lastError = `CDP endpoint returned ${response.status}`; - } - } catch (e) { - lastError = e instanceof Error ? e.message : String(e); - } - await sleep(interval); - waited += interval; - } - throw new Error( - `Timed out waiting for headless Chrome on port ${port} (${lastError}).`, - ); -} - /** * Launch headless Chrome on its own CDP port and connect to its page target. * The process itself is the shared launcher's job (src/core/cri/launch.ts); @@ -319,14 +260,12 @@ export async function launchScanBrowser(port: number): Promise { let client: CdpClient; try { - const wsUrl = await waitForCdp(browser.port, 15000); - client = await CdpClient.connect(wsUrl); + client = await CdpClient.connect(browser.port); } catch (e) { await browser.close(); const detail = browser.stderrTail().trim(); throw new Error( - (e instanceof Error ? e.message : String(e)) + - (detail ? `\nChrome said: ${detail}` : ""), + asError(e).message + (detail ? `\nChrome said: ${detail}` : ""), ); } From ecb98489f0bf4c7e0ad1c1f9a810fbb835e50f6a Mon Sep 17 00:00:00 2001 From: Charlotte Wickham Date: Mon, 31 Aug 2026 14:59:30 -0700 Subject: [PATCH 07/14] llm-doc: the scan stage now shares a launcher and a socket MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The scan-stage section still described two launchers and a hand-rolled WebSocket, with the shared launcher as follow-up work and the deno-cri retarget as a "plausible future". Both are done, so rewrite the section around the three layers the PR discussion settled on — launcher, transport, task logic — and say which are shared and which never will be. Both the section and launch.ts's own header say "the one place quarto's CDP drivers start headless Chrome", not "the one place quarto starts headless Chrome": src/core/puppeteer.ts launches through puppeteer instead (withHeadlessBrowser, reached through withPuppeteerBrowserAndPage and inPuppeteer). Nothing outside that file enters it today, but a maintainer chasing browser-launch behaviour should not be told it doesn't exist. --- llm-docs/axe-scan-architecture.md | 69 ++++++++++++++++++++----------- src/core/cri/launch.ts | 7 +++- 2 files changed, 51 insertions(+), 25 deletions(-) diff --git a/llm-docs/axe-scan-architecture.md b/llm-docs/axe-scan-architecture.md index 9ea46f1417..7779b4bb80 100644 --- a/llm-docs/axe-scan-architecture.md +++ b/llm-docs/axe-scan-architecture.md @@ -1,6 +1,6 @@ --- main_commit: abc6a78ed -analyzed_date: 2026-08-27 +analyzed_date: 2026-08-31 key_files: - src/command/call/axe/cmd.ts - src/command/call/axe/config.ts @@ -11,6 +11,7 @@ key_files: - src/command/call/axe/conformance.ts - src/command/call/axe/report.ts - src/command/call/axe/readme.ts + - src/core/cri/launch.ts --- # Axe Scan Architecture (`quarto call axe`) @@ -97,29 +98,49 @@ scan. ## The scan stage: raw CDP, fail-closed cells -The driver is a ~150-line generic CDP client (`CdpClient` in `scan.ts`) -with three capabilities: send a command and await its result, wait -(cancellably) for one event, close. It is transport, not scan logic — the -scanning is six CDP *commands* that `scanCell` sends through it (navigate, -viewport override, media emulation, evaluate-with-`awaitPromise`, an -on-new-document script, the load event), and those stay the scanner's -responsibility under any refactor. Because the command surface is that -small, there is no wrapper library; puppeteer-core is the named fallback if -raw CDP gets painful. - -quarto-cli's existing wrapper (`src/core/cri/cri.ts`, which drives Chrome -for mermaid) was read and declined — but be precise about which part: its -*facade* exposes mermaid's commands only (navigate/query/screenshot — no -emulation, no `awaitPromise`, no per-command timeout, so a hung `axe.run()` -would hang forever). Underneath that facade sits the vendored `deno-cri` -library, itself a generic send-any-command client; retargeting `CdpClient`'s -internals onto it (keeping the interface the tests stub) is the plausible -future unification of the *transport*. The more valuable near-term share is -the *launcher*: two exist in the tree (`launchScanBrowser` here, -`criClient`'s spawn half there), `cri.ts` cross-refers here, and extracting -one shared launcher is tracked follow-up work. Browser discovery is already -shared: `getBrowserExecutablePath()` (`src/core/puppeteer.ts`) encodes -`QUARTO_CHROMIUM` → installed `chrome-headless-shell` → system Chrome/Edge. +Think of a Chrome driver as three layers — launcher, transport, task logic — +because the answer to "why not reuse quarto's existing one?" is different at +each. The launcher and the transport are now shared with `src/core/cri/cri.ts`, +which drives Chrome for mermaid. The task logic never will be: mermaid's +commands and the scanner's commands are different jobs. + +**Launcher.** `launchChrome()` (`src/core/cri/launch.ts`) is the one place +quarto's CDP drivers start headless Chrome — this scanner and cri.ts. +(`src/core/puppeteer.ts` keeps a separate `puppeteer.launch()` path — +`withHeadlessBrowser`, reached through `withPuppeteerBrowserAndPage` and +`inPuppeteer` — which nothing outside that file enters today.) It owns the +flag set, the +`QUARTO_CHROMIUM_HEADLESS_MODE` escape hatch, stderr draining, exit cleanup, +and the wait for the CDP endpoint; callers pass in only what they genuinely +disagree about — `--hide-scrollbars` and a throwaway profile dir here (so a +scan cannot attach to, or be short-circuited by, a Chrome the user already has +running), `--renderer-process-limit=1` for mermaid. Browser *discovery* is +shared a level up again: `getBrowserExecutablePath()` +(`src/core/puppeteer.ts`) encodes `QUARTO_CHROMIUM` → installed +`chrome-headless-shell` → system Chrome/Edge. + +**Transport.** `CdpClient` in `scan.ts` has three capabilities: send a command +and await its result, wait (cancellably) for one event, close. The socket +under it is the vendored `deno-cri` (`src/core/cri/deno-cri/`), the same +generic send-any-command client cri.ts connects with. `CdpClient` is the typed +layer over it, and what it adds is the behaviour fail-closed cells depend on +and `deno-cri` does not have: a dropped connection rejects every command still +in flight, so a crashed tab fails one cell inside `--timeout` rather than +hanging the scan. Because the command surface is so small there is no wrapper +library on top; puppeteer-core is the named fallback if raw CDP gets painful. + +What is still *not* reused is cri.ts's **facade**, and it is worth being +precise about why: it exposes mermaid's commands only +(navigate/query/screenshot — no emulation, no `awaitPromise`, no per-command +timeout, so a hung `axe.run()` would hang forever). The cleaner end state, +where cri.ts *exports* a typed transport as a core surface and the scanner +consumes it, waits on the scanner's own needs settling — concurrent tabs in +particular. + +**Task logic** is the six CDP *commands* `scanCell` sends through the +transport: navigate, viewport override, media emulation, +evaluate-with-`awaitPromise`, an on-new-document script, the load event. +Those stay the scanner's responsibility under any refactor. **Cells fail closed.** A timeout, an evaluation error, a payload that is not an axe result, or a page that moved is an infrastructure failure in the diff --git a/src/core/cri/launch.ts b/src/core/cri/launch.ts index 0a23c7df0a..f78c7e9ac0 100644 --- a/src/core/cri/launch.ts +++ b/src/core/cri/launch.ts @@ -1,7 +1,7 @@ /* * launch.ts * - * The one place quarto starts headless Chrome. + * The one place quarto's CDP drivers start headless Chrome. * * Two subsystems drive Chrome over CDP: `criClient` (src/core/cri/cri.ts, * which renders mermaid diagrams) and the axe scanner @@ -10,6 +10,11 @@ * hard-won detail lives: which headless mode, how to tell the CDP endpoint is * up, and how not to orphan a process that never exits on its own. * + * Not to be confused with the launch path in src/core/puppeteer.ts + * (`withHeadlessBrowser`, reached through `withPuppeteerBrowserAndPage` and + * `inPuppeteer`), which starts Chrome through puppeteer rather than over CDP. + * Nothing outside that file enters it today, but it is a second launch path. + * * Everything the two callers genuinely disagree about is passed in * (`--renderer-process-limit=1` for mermaid; `--hide-scrollbars` and a * throwaway profile for the scanner). Browser *discovery* is shared upstream From de0060b2871b4f4acbdfeb93caec4680f8b69f38 Mon Sep 17 00:00:00 2001 From: Charlotte Wickham Date: Mon, 31 Aug 2026 15:54:31 -0700 Subject: [PATCH 08/14] axe: test the transport's fail-closed against a real browser The unit tests stub the CDP client, so they cover what scanCell does with a rejected send but not whether a real client rejects at all. That half now matters more than it did: rejecting in-flight commands used to fall out of owning the WebSocket, and is now CdpClient's own contribution on top of deno-cri, which notices a dropped socket and leaves those commands unsettled forever. Three cases against a real browser, each with a command the browser can never answer in flight: closing the client, sending after close, and the connection dropping from the far end (Browser.close, as the portable stand-in for a tab or process dying). Every wait has a deadline and a blown deadline reports as `hung`, so a transport that never settles fails the assertion instead of passing it. Checked by mutation: reverting abandonPending to deno-cri's own behaviour fails the test with `hung: nothing settled within 15000ms`. Runs in ~0.7s. --- llm-docs/axe-scan-architecture.md | 4 + .../axe/axe-transport-failclosed.test.ts | 127 ++++++++++++++++++ 2 files changed, 131 insertions(+) create mode 100644 tests/smoke/axe/axe-transport-failclosed.test.ts diff --git a/llm-docs/axe-scan-architecture.md b/llm-docs/axe-scan-architecture.md index 7779b4bb80..e329c5eec2 100644 --- a/llm-docs/axe-scan-architecture.md +++ b/llm-docs/axe-scan-architecture.md @@ -323,6 +323,10 @@ change the render path every `axe:` user hits. page selection. - `tests/unit/axe-scan-cell.test.ts` — transport fail-closed with a stubbed CDP client; slugs; URL encoding. +- `tests/smoke/axe/axe-transport-failclosed.test.ts` — the other half, against + a real browser: that a real `CdpClient` rejects at all when the connection + goes, on both the close and the dropped-socket paths. Every wait has a + deadline, because a hang is the failure being guarded against. - `tests/unit/axe-config.test.ts` — flag parsing and its errors. - `tests/unit/axe-report-readme.test.ts`, `axe-conformance-parity.test.ts` — the views and the mirrored labellers. diff --git a/tests/smoke/axe/axe-transport-failclosed.test.ts b/tests/smoke/axe/axe-transport-failclosed.test.ts new file mode 100644 index 0000000000..ab2b42758e --- /dev/null +++ b/tests/smoke/axe/axe-transport-failclosed.test.ts @@ -0,0 +1,127 @@ +/* + * axe-transport-failclosed.test.ts + * + * The transport half of fail-closed, against a real browser. + * + * tests/unit/axe-scan-cell.test.ts covers what scanCell *does* with a rejected + * send, using a stubbed client. This covers the half a stub cannot: that a real + * CdpClient rejects at all when the connection goes away. The client sits on + * the vendored deno-cri, which notices a dropped socket and then leaves every + * command that was in flight unsettled forever — rejecting them is CdpClient's + * own contribution, and it is what stops one crashed tab from hanging a whole + * scan (llm-docs/axe-scan-architecture.md, "The scan stage"). + * + * A hang is the failure this guards against, so every wait here has a deadline + * and blowing it is reported as a distinct outcome, never as a pass. + * + * Copyright (C) 2026 Posit Software, PBC + */ + +import { assertEquals } from "testing/asserts"; +import { ExecuteOutput, test, Verify } from "../../test.ts"; +import { findOpenPort } from "../../../src/core/port.ts"; +import { + launchScanBrowser, + ScanBrowser, +} from "../../../src/command/call/axe/scan.ts"; + +/** What a dropped connection must reject with, whoever noticed it. */ +const kClosed = "CDP connection closed"; + +/** Generous next to the sub-second reality: this is a hang detector. */ +const kDeadline = 15000; + +/** + * A command the browser can never answer, so the only way it settles is the + * connection going away. + */ +function unanswerable(browser: ScanBrowser): Promise { + return browser.client.send("Runtime.evaluate", { + expression: "new Promise(function () {})", + awaitPromise: true, + }); +} + +/** + * How `p` settled, as a string: the rejection message, `resolved`, or a + * distinct `hung` — so a transport that never settles fails the assertion + * rather than passing it. + */ +async function settled(p: Promise): Promise { + let timer: number | undefined; + try { + await Promise.race([ + p, + new Promise((_resolve, reject) => { + timer = setTimeout( + () => + reject(new Error(`hung: nothing settled within ${kDeadline}ms`)), + kDeadline, + ); + }), + ]); + return "resolved"; + } catch (e) { + return e instanceof Error ? e.message : String(e); + } finally { + clearTimeout(timer); + } +} + +const outcomes: Record = {}; + +const rejects = (key: string, name: string): Verify => ({ + name, + verify: (_output: ExecuteOutput[]) => { + assertEquals(outcomes[key], kClosed); + return Promise.resolve(); + }, +}); + +test({ + name: "quarto call axe (transport: a lost connection rejects, never hangs)", + type: "smoke", + context: { + // Two browsers are launched and both are gone by the end; the deadline + // above is the real guard, so give the whole test room for two launches. + timeout: 300000, + }, + execute: async () => { + // 1. We close the client ourselves while a command is outstanding. + { + const browser = await launchScanBrowser(findOpenPort(9222)); + try { + await browser.client.send("Runtime.enable"); + const inFlight = unanswerable(browser); + browser.client.close(); + outcomes["close"] = await settled(inFlight); + outcomes["after-close"] = await settled( + browser.client.send("Runtime.enable"), + ); + } finally { + await browser.close(); + } + } + + // 2. The browser goes away underneath us — the case deno-cri notices but + // does not act on. Browser.close is the portable stand-in for the tab + // or the process dying: the socket drops from the far end. + { + const browser = await launchScanBrowser(findOpenPort(9222)); + try { + await browser.client.send("Runtime.enable"); + const inFlight = unanswerable(browser); + // this one dies with the browser too; it is the trigger, not a result + browser.client.send("Browser.close").catch(() => {}); + outcomes["dropped"] = await settled(inFlight); + } finally { + await browser.close(); + } + } + }, + verify: [ + rejects("close", "close() rejects the command that was in flight"), + rejects("after-close", "a send after close rejects instead of waiting"), + rejects("dropped", "a dropped connection rejects the command in flight"), + ], +}); From d7ac2f0562a32bced77d4a5f69c7a5472a4caf15 Mon Sep 17 00:00:00 2001 From: Christophe Dervieux Date: Tue, 8 Sep 2026 13:08:29 +0200 Subject: [PATCH 09/14] cri: dedupe the CDP connect-retry into one shared helper criClient's open() and axe scan's CdpClient.connect() each retried connecting to the CDP socket with the same 5-tries-at-100ms loop and the same empirically-tuned comment block, duplicating the exact thing the shared launcher was meant to unify. Extract connectCdp() beside launchChrome() and have both callers use it. tests/unit/chrome-launch.test.ts pins the retry-exhaustion behavior so a future change to the retry budget or its error message is visible. --- src/command/call/axe/scan.ts | 25 ++++------------ src/core/cri/cri.ts | 38 ++---------------------- src/core/cri/launch.ts | 50 ++++++++++++++++++++++++++++++++ tests/unit/chrome-launch.test.ts | 22 ++++++++++++++ 4 files changed, 79 insertions(+), 56 deletions(-) create mode 100644 tests/unit/chrome-launch.test.ts diff --git a/src/command/call/axe/scan.ts b/src/command/call/axe/scan.ts index 7a8429f4f1..809e80fd20 100644 --- a/src/command/call/axe/scan.ts +++ b/src/command/call/axe/scan.ts @@ -22,7 +22,7 @@ import { join } from "../../../deno_ral/path.ts"; import { md5HashSync } from "../../../core/hash.ts"; import { sleep } from "../../../core/async.ts"; import { formatResourcePath } from "../../../core/resources.ts"; -import { launchChrome } from "../../../core/cri/launch.ts"; +import { connectCdp, launchChrome } from "../../../core/cri/launch.ts"; import cdp from "../../../core/cri/deno-cri/index.js"; import { AxeScanConfig, AxeViewport } from "./config.ts"; import { AxeMode, AxePage } from "./discover.ts"; @@ -147,26 +147,11 @@ export class CdpClient { connection.on("disconnect", () => this.abandonPending()); } - /** - * Connect to the page target on `port`. Connecting the instant the CDP - * endpoint answers is racy: cri.ts measured the failure rate against the - * gap between tries (see criClient.open) and settled on 100ms, which is - * what this retries with. - */ + /** Connect to the page target on `port`, retrying via the shared launcher's helper. */ static async connect(port: number): Promise { - const maxTries = 5; - for (let attempt = 1;; ++attempt) { - try { - return new CdpClient(await connectDenoCri({ port })); - } catch (e) { - if (attempt === maxTries) { - throw new Error( - `Failed to connect to CDP on port ${port}: ${asError(e).message}`, - ); - } - await sleep(100); - } - } + return new CdpClient( + await connectCdp(port, (p) => connectDenoCri({ port: p })), + ); } send( diff --git a/src/core/cri/cri.ts b/src/core/cri/cri.ts index e529eac6f2..c2ae0ca489 100644 --- a/src/core/cri/cri.ts +++ b/src/core/cri/cri.ts @@ -8,11 +8,10 @@ import { decodeBase64 as decode } from "encoding/base64"; import cdp from "./deno-cri/index.js"; -import { launchChrome } from "./launch.ts"; +import { connectCdp, launchChrome } from "./launch.ts"; import { Semaphore } from "../lib/semaphore.ts"; import { findOpenPort } from "../port.ts"; import { getNamedLifetime, ObjectWithLifetime } from "../lifetimes.ts"; -import { sleep } from "../async.ts"; import { InternalError } from "../lib/error.ts"; import { kRenderFileLifetime } from "../../config/constants.ts"; @@ -85,40 +84,7 @@ export async function criClient(appPath?: string, port?: number) { rawClient: () => client, open: async (url: string) => { - const maxTries = 5; - for (let i = 0; i < maxTries; ++i) { - try { - client = await cdp({ port }); - break; - } catch (e) { - if (i === maxTries - 1) { - throw e; - } - // sleep(0) caused 42/100 crashes - // sleep(1) caused 42/100 crashes - // sleep(2) caused 15/100 crashes - // sleep(3) caused 13/100 crashes - // sleep(4) caused 8/100 crashes - // sleep(5) caused 1/100 crashes - // sleep(6) caused 1/100 crashes - // sleep(7) caused 1/100 crashes - // sleep(8) caused 1/100 crashes - // sleep(9) caused 0/100 crashes - // sleep(10) caused 0/100 crashes - // sleep(11) caused 0/100 crashes - // sleep(12) caused 0/100 crashes - // sleep(13) caused 1/100 crashes - // sleep(14) caused 1/100 crashes - // sleep(15) caused 0/100 crashes - // sleep(16) caused 0/100 crashes - // sleep(17) caused 0/100 crashes - - // https://carlos-scheidegger.quarto.pub/failure-rates-in-cri-initialization/ - // suggests that 44ms is a good value. We use 100ms to try and account for slower - // machines. - await sleep(100); - } - } + client = await connectCdp(port, async (p) => await cdp({ port: p })); const { Network, Page } = client; await Network.enable(); await Page.enable(); diff --git a/src/core/cri/launch.ts b/src/core/cri/launch.ts index f78c7e9ac0..94d3e749ff 100644 --- a/src/core/cri/launch.ts +++ b/src/core/cri/launch.ts @@ -104,6 +104,56 @@ async function waitForCdpEndpoint( return lastError; } +/** + * Retry connecting a CDP client against `port`. Connecting the instant the + * CDP endpoint answers is racy — cri.ts measured the failure rate against + * the gap between tries and settled on 100ms, which is what this retries + * with: + * + * sleep(0) caused 42/100 crashes + * sleep(1) caused 42/100 crashes + * sleep(2) caused 15/100 crashes + * sleep(3) caused 13/100 crashes + * sleep(4) caused 8/100 crashes + * sleep(5) caused 1/100 crashes + * sleep(6) caused 1/100 crashes + * sleep(7) caused 1/100 crashes + * sleep(8) caused 1/100 crashes + * sleep(9) caused 0/100 crashes + * sleep(10) caused 0/100 crashes + * sleep(11) caused 0/100 crashes + * sleep(12) caused 0/100 crashes + * sleep(13) caused 1/100 crashes + * sleep(14) caused 1/100 crashes + * sleep(15) caused 0/100 crashes + * sleep(16) caused 0/100 crashes + * sleep(17) caused 0/100 crashes + * + * https://carlos-scheidegger.quarto.pub/failure-rates-in-cri-initialization/ + * suggests that 44ms is a good value. We use 100ms to try and account for + * slower machines. + */ +export async function connectCdp( + port: number, + connect: (port: number) => Promise, +): Promise { + const maxTries = 5; + for (let attempt = 1;; ++attempt) { + try { + return await connect(port); + } catch (e) { + if (attempt === maxTries) { + throw new Error( + `Failed to connect to CDP on port ${port}: ${ + e instanceof Error ? e.message : String(e) + }`, + ); + } + await sleep(100); + } + } +} + /** * Launch headless Chrome with its CDP endpoint open on `port`, and return once * that endpoint answers. The caller connects a protocol client of its own — diff --git a/tests/unit/chrome-launch.test.ts b/tests/unit/chrome-launch.test.ts new file mode 100644 index 0000000000..0af043716d --- /dev/null +++ b/tests/unit/chrome-launch.test.ts @@ -0,0 +1,22 @@ +/* + * chrome-launch.test.ts + * + * Copyright (C) 2026 Posit Software, PBC + */ + +import { assert, assertRejects } from "testing/asserts"; +import { CdpClient } from "../../src/command/call/axe/scan.ts"; +import { findOpenPort } from "../../src/core/port.ts"; +import { unitTest } from "../test.ts"; + +unitTest( + "chrome-launch - CdpClient.connect exhausts its retries with a useful message", + async () => { + const port = findOpenPort(); + const err = await assertRejects(() => CdpClient.connect(port), Error); + assert( + err.message.includes(String(port)), + `expected port ${port} named in: ${err.message}`, + ); + }, +); From c56a960c3ffde06721b0f925ceb885b98db62891 Mon Sep 17 00:00:00 2001 From: Christophe Dervieux Date: Tue, 8 Sep 2026 13:12:44 +0200 Subject: [PATCH 10/14] axe: wait for a real page target before treating Chrome as ready waitForCdpEndpoint only checked for HTTP 200 on /json/list, which Chrome's DevTools server can answer before any page target exists. That's fine for criClient (deno-cri's own default target creates one on connect), but the axe scanner connects with a bare function target that skips that creation path, so its 500ms connect-retry was the only thing standing between "endpoint up" and "a target actually exists" - shrinking what used to be a 15s wait down to effectively nothing on a loaded box. launchChrome now takes an awaitPageTarget option; when set, the readiness poll requires a target with webSocketDebuggerUrl in the /json/list response, not just a 200. Along the way, the readiness wait's own timeout tracking is fixed to use wall-clock deltas instead of counting fixed-size iterations (an iteration count doesn't bound real elapsed time if a single attempt runs long), each fetch attempt is capped so one slow or refused connection can't consume the whole budget on its own, and the wait is raced against the launched process exiting, so a Chrome that's already dead is reported immediately rather than only once the timeout expires. tests/unit/chrome-launch.test.ts gains coverage for launchChrome's rejection path against a Chrome that never becomes ready - a fixed-endpoint executable is a real subprocess, not a mock, per this repo's testing convention. --- src/command/call/axe/scan.ts | 1 + src/core/cri/launch.ts | 74 +++++++++++++----- tests/unit/chrome-launch.test.ts | 125 +++++++++++++++++++++++++++++++ 3 files changed, 182 insertions(+), 18 deletions(-) diff --git a/src/command/call/axe/scan.ts b/src/command/call/axe/scan.ts index 809e80fd20..4799836cb5 100644 --- a/src/command/call/axe/scan.ts +++ b/src/command/call/axe/scan.ts @@ -240,6 +240,7 @@ export async function launchScanBrowser(port: number): Promise { args: ["--hide-scrollbars"], url: "about:blank", isolatedProfile: true, + awaitPageTarget: true, logPrefix: "axe chrome", }); diff --git a/src/core/cri/launch.ts b/src/core/cri/launch.ts index 94d3e749ff..28a982db58 100644 --- a/src/core/cri/launch.ts +++ b/src/core/cri/launch.ts @@ -54,6 +54,14 @@ export interface ChromeLaunchOptions { * Chrome the user already has running. */ isolatedProfile?: boolean; + /** + * Wait for an actual page target (one with `webSocketDebuggerUrl`), not + * just an HTTP 200 from `/json/list`. deno-cri's default target creates a + * page itself when connecting with a string or object target, but the axe + * scanner connects with a bare function target, which skips that — so it + * needs the launcher to guarantee a target exists first. + */ + awaitPageTarget?: boolean; /** How long to wait for the CDP endpoint, in ms. */ timeout?: number; /** Tag for Chrome's stderr in the debug log. */ @@ -72,36 +80,67 @@ export interface LaunchedChrome { /** How long the CDP endpoint gets to come up, when the caller doesn't say. */ const kDefaultLaunchTimeout = 15000; +/** How long a single readiness fetch gets before it's treated as a failed attempt. */ +const kProbeAttemptTimeout = 1000; + +function hasPageTarget(list: unknown): boolean { + return Array.isArray(list) && list.some((t) => + typeof (t as { webSocketDebuggerUrl?: unknown })?.webSocketDebuggerUrl === + "string" + ); +} + /** - * Poll the CDP endpoint until it answers. `localhost` rather than - * `127.0.0.1` because that is what deno-cri connects to afterwards (its - * `defaults.HOST`) — a launcher that accepts a host the client can't reach - * would report ready too early. + * Poll the CDP endpoint until it answers `isReady`, and race that against + * `exited`: a Chrome that has already died is never going to open the + * endpoint no matter how long is left on the clock, so this reports the + * moment it exits rather than waiting out the full timeout. + * + * The deadline is tracked in wall-clock time, not iterations — a `fetch()` + * against a port nothing is listening on can itself take far longer than one + * `interval`, and an iteration count would let that alone blow the budget. + * Each attempt is capped at `kProbeAttemptTimeout` for the same reason. + * + * `localhost` rather than `127.0.0.1` because that is what deno-cri connects + * to afterwards (its `defaults.HOST`) — a launcher that accepts a host the + * client can't reach would report ready too early. */ async function waitForCdpEndpoint( port: number, timeout: number, + exited: Promise, + isReady: (list: unknown) => boolean, ): Promise { const interval = 50; - let waited = 0; + const deadline = Date.now() + timeout; + let dead: Deno.CommandStatus | undefined; + exited.then((status) => { + dead = status; + }).catch(() => {}); + let lastError = "no response"; - while (waited < timeout) { + while (Date.now() < deadline) { + if (dead) { + return `Chrome exited (code ${dead.code}) before its CDP endpoint ` + + `on port ${port} became ready`; + } try { - const response = await fetch(`http://localhost:${port}/json/list`); - // drain the body either way: nothing here reads it, and an unread body - // holds the connection open - await response.body?.cancel(); - if (response.ok) { + const response = await fetch(`http://localhost:${port}/json/list`, { + signal: AbortSignal.timeout(kProbeAttemptTimeout), + }); + const body = await response.json().catch(() => undefined); + if (response.ok && isReady(body)) { return undefined; } - lastError = `CDP endpoint returned ${response.status}`; + lastError = response.ok + ? "CDP endpoint has no ready page target yet" + : `CDP endpoint returned ${response.status}`; } catch (e) { lastError = e instanceof Error ? e.message : String(e); } await sleep(interval); - waited += interval; } - return lastError; + return `Timed out waiting for headless Chrome on port ${port} (${lastError})`; } /** @@ -249,16 +288,15 @@ export async function launchChrome( const failure = await waitForCdpEndpoint( port, options.timeout ?? kDefaultLaunchTimeout, + process.status, + options.awaitPageTarget ? hasPageTarget : Array.isArray, ); if (failure !== undefined) { debug(`[${prefix} path] : ${app}`); debug(`[${prefix} args] : ${args.join(" ")}`); await close(); const detail = stderrTail.trim(); - throw new Error( - `Timed out waiting for headless Chrome on port ${port} (${failure}).` + - (detail ? `\nChrome said: ${detail}` : ""), - ); + throw new Error(failure + (detail ? `\nChrome said: ${detail}` : "")); } return { diff --git a/tests/unit/chrome-launch.test.ts b/tests/unit/chrome-launch.test.ts index 0af043716d..da03f0d312 100644 --- a/tests/unit/chrome-launch.test.ts +++ b/tests/unit/chrome-launch.test.ts @@ -5,10 +5,135 @@ */ import { assert, assertRejects } from "testing/asserts"; +import { join } from "path"; +import { launchChrome } from "../../src/core/cri/launch.ts"; import { CdpClient } from "../../src/command/call/axe/scan.ts"; import { findOpenPort } from "../../src/core/port.ts"; import { unitTest } from "../test.ts"; +/** Short enough that the test doesn't wait around, long enough to be real. */ +const kLaunchTimeout = 300; +/** How long the fake Chrome stays up before giving up on its own. */ +const kSelfDestructMs = 2000; + +// A fake Chrome that accepts the TCP connection on its CDP port but never +// answers on it -- genuinely silent, not merely absent, so a `fetch()` +// against it hangs on the response rather than failing fast with connection +// refused. Same wrapper mechanism as chrome-launch-flags.test.ts's fake +// executable (.cmd on Windows, a shebang script on Unix, re-invoking the +// current deno binary to run an inline script -- a wrapper is needed because +// launchChrome() always prepends real Chrome flags as argv, which the fake +// executable must tolerate). +// +// It self-exits on its own short timer rather than relying on launchChrome's +// kill() to reach it: on Windows, kill() only reaches the direct child (the +// .cmd's cmd.exe host), and the actual deno.exe process survives as an +// orphan holding its inherited stderr pipe open, which would otherwise hang +// launchChrome's close() until the timer fires anyway. +async function writeSilentChromeExecutable( + dir: string, + port: number, + stderrText: string, +): Promise { + const scriptPath = join(dir, "silent-chrome.js"); + await Deno.writeTextFile( + scriptPath, + [ + `const listener = Deno.listen({ port: ${port} });`, + "(async () => {", + " for await (const _conn of listener) {", + " // accept the connection, never write a response", + " }", + "})().catch(() => {});", + stderrText + ? `await Deno.stderr.write(new TextEncoder().encode(${ + JSON.stringify(stderrText) + }));` + : "", + `setTimeout(() => Deno.exit(1), ${kSelfDestructMs});`, + ].join("\n"), + ); + + const deno = Deno.execPath(); + if (Deno.build.os === "windows") { + const cmdPath = join(dir, "silent-chrome.cmd"); + await Deno.writeTextFile( + cmdPath, + `@echo off\r\n"${deno}" run --allow-net "${scriptPath}" %*\r\n`, + ); + return cmdPath; + } + const shPath = join(dir, "silent-chrome.sh"); + await Deno.writeTextFile( + shPath, + `#!/bin/sh\n"${deno}" run --allow-net "${scriptPath}" "$@"\n`, + ); + await Deno.chmod(shPath, 0o755); + return shPath; +} + +unitTest( + "chrome-launch - alive-but-silent Chrome rejects, stderr carried", + async () => { + const port = findOpenPort(); + const dir = await Deno.makeTempDir({ prefix: "chrome-launch-silent-" }); + try { + const fakeChrome = await writeSilentChromeExecutable( + dir, + port, + "Chrome blew up: missing shared library", + ); + const err = await assertRejects( + () => + launchChrome({ appPath: fakeChrome, port, timeout: kLaunchTimeout }), + Error, + ); + assert( + err.message.includes(String(port)), + `expected port ${port} named in: ${err.message}`, + ); + assert( + err.message.includes( + "Chrome said: Chrome blew up: missing shared library", + ), + `expected stderr tail after "Chrome said:" in: ${err.message}`, + ); + } finally { + await Deno.remove(dir, { recursive: true }).catch(() => {}); + } + }, +); + +unitTest( + "chrome-launch - alive-but-silent Chrome rejects, no dangling colon on empty stderr", + async () => { + const port = findOpenPort(); + const dir = await Deno.makeTempDir({ prefix: "chrome-launch-silent-" }); + try { + const fakeChrome = await writeSilentChromeExecutable(dir, port, ""); + const err = await assertRejects( + () => + launchChrome({ appPath: fakeChrome, port, timeout: kLaunchTimeout }), + Error, + ); + assert( + err.message.includes(String(port)), + `expected port ${port} named in: ${err.message}`, + ); + assert( + !err.message.includes("Chrome said:"), + `expected no "Chrome said:" for empty stderr in: ${err.message}`, + ); + assert( + !err.message.trimEnd().endsWith(":"), + `expected no dangling colon in: ${err.message}`, + ); + } finally { + await Deno.remove(dir, { recursive: true }).catch(() => {}); + } + }, +); + unitTest( "chrome-launch - CdpClient.connect exhausts its retries with a useful message", async () => { From a56d1ef53187fbe6e299153a6d9b4a89447e8f04 Mon Sep 17 00:00:00 2001 From: Christophe Dervieux Date: Tue, 8 Sep 2026 13:48:12 +0200 Subject: [PATCH 11/14] cri: apply the page-target readiness fix to both callers, not just axe MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review turned up that the previous commit's awaitPageTarget rationale was wrong: neither axe nor criClient ever passes deno-cri a target, so both hit the exact same defaultTarget page-creation race the option was written to address — the "axe connects with a bare function target, criClient is fine" distinction doesn't exist in the code. Wire awaitPageTarget into criClient's launchChrome call too, and fix the comment to describe the actual mechanism instead of the invented one. Also fixes two smaller issues found in the same review: hasPageTarget accepted any target with a webSocketDebuggerUrl regardless of type, so a non-page target (a service worker, say) could satisfy readiness early — now requires type === "page", matching deno-cri's own defaultTarget filter. And each readiness probe was capped at a fixed 1s regardless of how much of the launch timeout was left, so a probe starting late could overrun the caller's requested timeout by close to a full second; probeTimeoutMs now bounds each attempt by whichever of the two is smaller. tests/unit/chrome-launch.test.ts covers hasPageTarget's type filter, probeTimeoutMs's bound, connectCdp's attempt count on both exhaustion and recovery, and launchChrome's awaitPageTarget option end to end against a fake CDP endpoint that withholds and then serves a real page target. chrome-launch-flags.test.ts's fake endpoint is updated to serve a real page target, since criClient now requires one. --- src/core/cri/cri.ts | 4 + src/core/cri/launch.ts | 46 ++++-- tests/unit/chrome-launch-flags.test.ts | 8 +- tests/unit/chrome-launch.test.ts | 186 ++++++++++++++++++++++++- 4 files changed, 229 insertions(+), 15 deletions(-) diff --git a/src/core/cri/cri.ts b/src/core/cri/cri.ts index c2ae0ca489..4a5b3f427a 100644 --- a/src/core/cri/cri.ts +++ b/src/core/cri/cri.ts @@ -64,6 +64,10 @@ export async function criClient(appPath?: string, port?: number) { port, // One diagram is rendered at a time, so a renderer per tab buys nothing. args: ["--renderer-process-limit=1"], + // connectCdp() below faces the same page-target race the axe scanner + // does (see ChromeLaunchOptions.awaitPageTarget) — neither caller passes + // deno-cri a target, so both rely on its default target creation. + awaitPageTarget: true, logPrefix: "CHROMIUM", }); port = browser.port; diff --git a/src/core/cri/launch.ts b/src/core/cri/launch.ts index 28a982db58..dd5cef521c 100644 --- a/src/core/cri/launch.ts +++ b/src/core/cri/launch.ts @@ -55,11 +55,14 @@ export interface ChromeLaunchOptions { */ isolatedProfile?: boolean; /** - * Wait for an actual page target (one with `webSocketDebuggerUrl`), not - * just an HTTP 200 from `/json/list`. deno-cri's default target creates a - * page itself when connecting with a string or object target, but the axe - * scanner connects with a bare function target, which skips that — so it - * needs the launcher to guarantee a target exists first. + * Wait for an actual page target, not just an HTTP 200 from `/json/list`. + * Neither caller passes deno-cri a `target`, so both fall through to its + * own `defaultTarget` (deno-cri/chrome.js), which creates a page itself + * when the target list it fetches is empty — but that creation races + * Chrome's own startup the same way the CDP endpoint's availability does. + * This makes the launcher wait until a page target already exists before + * either caller's connect-retry even starts, rather than leaning on that + * retry to absorb the race on every launch. */ awaitPageTarget?: boolean; /** How long to wait for the CDP endpoint, in ms. */ @@ -83,11 +86,28 @@ const kDefaultLaunchTimeout = 15000; /** How long a single readiness fetch gets before it's treated as a failed attempt. */ const kProbeAttemptTimeout = 1000; -function hasPageTarget(list: unknown): boolean { - return Array.isArray(list) && list.some((t) => - typeof (t as { webSocketDebuggerUrl?: unknown })?.webSocketDebuggerUrl === - "string" - ); +/** + * How long the next readiness fetch may run: never past `kProbeAttemptTimeout`, + * and never past what's left before `deadline` either, so a probe that starts + * near the end of the launch timeout can't itself blow past it by up to a + * full `kProbeAttemptTimeout`. + */ +export function probeTimeoutMs(remainingMs: number): number { + return Math.max(0, Math.min(kProbeAttemptTimeout, remainingMs)); +} + +/** + * `type === "page"` matches deno-cri's own `defaultTarget` filter + * (deno-cri/chrome.js) -- a target with a `webSocketDebuggerUrl` but a + * different type (e.g. a service worker) is connectable but isn't the page + * the caller is waiting for. + */ +export function hasPageTarget(list: unknown): boolean { + return Array.isArray(list) && list.some((t) => { + const target = t as { type?: unknown; webSocketDebuggerUrl?: unknown }; + return target?.type === "page" && + typeof target?.webSocketDebuggerUrl === "string"; + }); } /** @@ -99,7 +119,9 @@ function hasPageTarget(list: unknown): boolean { * The deadline is tracked in wall-clock time, not iterations — a `fetch()` * against a port nothing is listening on can itself take far longer than one * `interval`, and an iteration count would let that alone blow the budget. - * Each attempt is capped at `kProbeAttemptTimeout` for the same reason. + * Each attempt is capped by `probeTimeoutMs` — at `kProbeAttemptTimeout`, or + * at what's left before `deadline` if that's shorter, so a slow attempt late + * in the budget can't itself run past it. * * `localhost` rather than `127.0.0.1` because that is what deno-cri connects * to afterwards (its `defaults.HOST`) — a launcher that accepts a host the @@ -126,7 +148,7 @@ async function waitForCdpEndpoint( } try { const response = await fetch(`http://localhost:${port}/json/list`, { - signal: AbortSignal.timeout(kProbeAttemptTimeout), + signal: AbortSignal.timeout(probeTimeoutMs(deadline - Date.now())), }); const body = await response.json().catch(() => undefined); if (response.ok && isReady(body)) { diff --git a/tests/unit/chrome-launch-flags.test.ts b/tests/unit/chrome-launch-flags.test.ts index 8ef0d97114..0a7adbb185 100644 --- a/tests/unit/chrome-launch-flags.test.ts +++ b/tests/unit/chrome-launch-flags.test.ts @@ -33,7 +33,13 @@ async function writeFakeChromeExecutable( `Deno.serve({ port: ${port}, onListen: () => {} }, (req) => {`, " const url = new URL(req.url);", ' if (url.pathname === "/json/list") {', - ' return new Response("[]", { status: 200 });', + // criClient now waits for a real page target (awaitPageTarget), so + // this must look like deno-cri's own defaultTarget expects, not just + // any 200. + " return new Response(", + ` JSON.stringify([{ type: "page", webSocketDebuggerUrl: "ws://localhost:${port}/devtools/page/1" }]),`, + " { status: 200 },", + " );", ' } else if (url.pathname === "/shutdown") {', " setTimeout(() => Deno.exit(0), 50);", ' return new Response("", { status: 200 });', diff --git a/tests/unit/chrome-launch.test.ts b/tests/unit/chrome-launch.test.ts index da03f0d312..3418dbfc35 100644 --- a/tests/unit/chrome-launch.test.ts +++ b/tests/unit/chrome-launch.test.ts @@ -4,9 +4,14 @@ * Copyright (C) 2026 Posit Software, PBC */ -import { assert, assertRejects } from "testing/asserts"; +import { assert, assertEquals, assertRejects } from "testing/asserts"; import { join } from "path"; -import { launchChrome } from "../../src/core/cri/launch.ts"; +import { + connectCdp, + hasPageTarget, + launchChrome, + probeTimeoutMs, +} from "../../src/core/cri/launch.ts"; import { CdpClient } from "../../src/command/call/axe/scan.ts"; import { findOpenPort } from "../../src/core/port.ts"; import { unitTest } from "../test.ts"; @@ -145,3 +150,180 @@ unitTest( ); }, ); + +// deno-lint-ignore require-await +unitTest( + "chrome-launch - hasPageTarget requires target.type to be page", + async () => { + assertEquals( + hasPageTarget([{ type: "page", webSocketDebuggerUrl: "ws://x" }]), + true, + ); + // A connectable target that isn't a page (e.g. a service worker) must not + // count -- deno-cri's own defaultTarget (chrome.js) applies the same + // type === "page" filter before falling back to any connectable target. + assertEquals( + hasPageTarget([{ + type: "service_worker", + webSocketDebuggerUrl: "ws://x", + }]), + false, + ); + assertEquals(hasPageTarget([]), false); + assertEquals(hasPageTarget("not-an-array"), false); + }, +); + +// deno-lint-ignore require-await +unitTest( + "chrome-launch - probeTimeoutMs never exceeds what's left of the launch budget", + async () => { + // Plenty of time left: capped at the per-attempt ceiling, not the full budget. + assertEquals(probeTimeoutMs(5000), 1000); + // Little time left: capped at what's actually left, not the per-attempt ceiling. + assertEquals(probeTimeoutMs(50), 50); + // Already past the deadline: clamps to zero rather than going negative. + assertEquals(probeTimeoutMs(-10), 0); + }, +); + +function countingConnector(failuresBeforeSuccess: number) { + let attempts = 0; + const connect = (_port: number) => { + attempts++; + if (attempts <= failuresBeforeSuccess) { + return Promise.reject(new Error(`transient failure #${attempts}`)); + } + return Promise.resolve(attempts); + }; + return { connect, attemptsSoFar: () => attempts }; +} + +unitTest( + "chrome-launch - connectCdp gives up after exactly 5 attempts", + async () => { + const { connect, attemptsSoFar } = countingConnector(Infinity); + const err = await assertRejects(() => connectCdp(1, connect), Error); + assertEquals(attemptsSoFar(), 5); + assert( + err.message.includes("1"), + `expected port 1 named in: ${err.message}`, + ); + }, +); + +unitTest( + "chrome-launch - connectCdp succeeds after transient failures without exhausting retries", + async () => { + const { connect, attemptsSoFar } = countingConnector(2); + const result = await connectCdp(1, connect); + assertEquals(result, 3); + assertEquals(attemptsSoFar(), 3); + }, +); + +/** + * A fake Chrome that serves `/json/list` itself, controlling exactly what the + * launcher's readiness poll sees -- same Deno.serve wrapper mechanism as + * chrome-launch-flags.test.ts's fake executable. `pageAfterMs` is measured + * from the fake server's own start, not the caller's: `[]` until then, a + * single real page target afterward. `undefined` means never. + */ +async function writeJsonListChromeExecutable( + dir: string, + port: number, + pageAfterMs: number | undefined, +): Promise { + const scriptPath = join(dir, "json-list-chrome.js"); + await Deno.writeTextFile( + scriptPath, + [ + "const start = Date.now();", + `const pageAfterMs = ${ + pageAfterMs === undefined ? "undefined" : pageAfterMs + };`, + `Deno.serve({ port: ${port}, onListen: () => {} }, (req) => {`, + " const url = new URL(req.url);", + ' if (url.pathname !== "/json/list") {', + ' return new Response("", { status: 404 });', + " }", + " const ready = pageAfterMs !== undefined &&", + " Date.now() - start >= pageAfterMs;", + " const body = ready", + ` ? JSON.stringify([{ type: "page", webSocketDebuggerUrl: "ws://localhost:${port}/devtools/page/1" }])`, + ' : "[]";', + " return new Response(body, { status: 200 });", + "});", + // Same Windows orphan issue as writeSilentChromeExecutable above: + // launchChrome's close() only kills the .cmd's cmd.exe host, and the + // deno.exe grandchild survives holding its inherited stderr pipe open, + // which would otherwise hang close()'s drain loop forever. + `setTimeout(() => Deno.exit(0), ${kSelfDestructMs});`, + ].join("\n"), + ); + + const deno = Deno.execPath(); + if (Deno.build.os === "windows") { + const cmdPath = join(dir, "json-list-chrome.cmd"); + await Deno.writeTextFile( + cmdPath, + `@echo off\r\n"${deno}" run --allow-net "${scriptPath}" %*\r\n`, + ); + return cmdPath; + } + const shPath = join(dir, "json-list-chrome.sh"); + await Deno.writeTextFile( + shPath, + `#!/bin/sh\n"${deno}" run --allow-net "${scriptPath}" "$@"\n`, + ); + await Deno.chmod(shPath, 0o755); + return shPath; +} + +unitTest( + "chrome-launch - awaitPageTarget rejects when the endpoint never reports a page target", + async () => { + const port = findOpenPort(); + const dir = await Deno.makeTempDir({ prefix: "chrome-launch-json-list-" }); + try { + const fakeChrome = await writeJsonListChromeExecutable( + dir, + port, + undefined, + ); + await assertRejects( + () => + launchChrome({ + appPath: fakeChrome, + port, + timeout: kLaunchTimeout, + awaitPageTarget: true, + }), + Error, + ); + } finally { + await Deno.remove(dir, { recursive: true }).catch(() => {}); + } + }, +); + +unitTest( + "chrome-launch - awaitPageTarget resolves once a real page target appears", + async () => { + const port = findOpenPort(); + const dir = await Deno.makeTempDir({ prefix: "chrome-launch-json-list-" }); + try { + const fakeChrome = await writeJsonListChromeExecutable(dir, port, 150); + const browser = await launchChrome({ + appPath: fakeChrome, + port, + // Long enough to span several 50ms polls past the 150ms flip. + timeout: 3000, + awaitPageTarget: true, + }); + await browser.close(); + } finally { + await Deno.remove(dir, { recursive: true }).catch(() => {}); + } + }, +); From 3575819a5ee33ec0b980538e686f2977075c14bd Mon Sep 17 00:00:00 2001 From: Christophe Dervieux Date: Tue, 8 Sep 2026 14:29:37 +0200 Subject: [PATCH 12/14] cri: give criClient a URL so its new page-target wait can actually succeed Review turned up that the previous commit's awaitPageTarget: true on criClient's launchChrome call was a real regression: verified against the actual chrome-headless-shell binary that with no URL to open, it launches with zero CDP targets at all, and nothing else creates one before launchChrome's readiness wait gives up -- deno-cri's own target creation only runs later, at connect time. Every mermaid render through chrome-headless-shell would have hung for the full launch timeout and then failed. Passing url: "about:blank" (same as the axe scanner already does) makes Chrome create a page target as part of its own startup, which open()'s Page.navigate() replaces immediately afterward. Also fixes a second, smaller issue in the same review: Chrome's own exit was only checked between readiness probes, so a probe already in flight against a genuinely unresponsive port could hold up "Chrome exited" for close to a full probe timeout after the exit actually happened. waitForCdpEndpoint now aborts the in-flight probe as soon as the process exits instead of waiting for it to time out on its own. tests/unit/chrome-launch-flags.test.ts adds a fake Chrome whose /json/list mirrors the real binary's URL-gated target creation, driving criClient through it end to end. tests/unit/chrome-launch.test.ts adds a test where a silent Chrome exits quickly under a much longer launch timeout, and asserts the exit is reported near-immediately rather than after an in-flight probe's own timeout. --- src/core/cri/cri.ts | 5 ++ src/core/cri/launch.ts | 39 ++++++++-- tests/unit/chrome-launch-flags.test.ts | 98 ++++++++++++++++++++++++++ tests/unit/chrome-launch.test.ts | 41 ++++++++++- 4 files changed, 176 insertions(+), 7 deletions(-) diff --git a/src/core/cri/cri.ts b/src/core/cri/cri.ts index 4a5b3f427a..06cbdbd41a 100644 --- a/src/core/cri/cri.ts +++ b/src/core/cri/cri.ts @@ -67,6 +67,11 @@ export async function criClient(appPath?: string, port?: number) { // connectCdp() below faces the same page-target race the axe scanner // does (see ChromeLaunchOptions.awaitPageTarget) — neither caller passes // deno-cri a target, so both rely on its default target creation. + // awaitPageTarget needs an actual target to wait for, though: verified + // against the real chrome-headless-shell binary that with no url to + // open, it launches with zero targets at all -- open()'s own + // Page.navigate() below replaces this placeholder immediately. + url: "about:blank", awaitPageTarget: true, logPrefix: "CHROMIUM", }); diff --git a/src/core/cri/launch.ts b/src/core/cri/launch.ts index dd5cef521c..6066da211b 100644 --- a/src/core/cri/launch.ts +++ b/src/core/cri/launch.ts @@ -113,8 +113,9 @@ export function hasPageTarget(list: unknown): boolean { /** * Poll the CDP endpoint until it answers `isReady`, and race that against * `exited`: a Chrome that has already died is never going to open the - * endpoint no matter how long is left on the clock, so this reports the - * moment it exits rather than waiting out the full timeout. + * endpoint no matter how long is left on the clock, so this aborts whichever + * probe is currently in flight and reports the moment it exits, rather than + * waiting out that probe's own timeout first. * * The deadline is tracked in wall-clock time, not iterations — a `fetch()` * against a port nothing is listening on can itself take far longer than one @@ -136,19 +137,38 @@ async function waitForCdpEndpoint( const interval = 50; const deadline = Date.now() + timeout; let dead: Deno.CommandStatus | undefined; + // TS narrows a captured variable to `undefined` after the first `if + // (getDead())` returns and never widens it back across the `await` + // below, even though the `.then()` reassigns it concurrently -- reading + // through a function call sidesteps that, since each call site gets its + // own fresh local binding to narrow. + const getDead = () => dead; + const exitedMessage = (status: Deno.CommandStatus) => + `Chrome exited (code ${status.code}) before its CDP endpoint on port ` + + `${port} became ready`; + let abortInFlightProbe: (() => void) | undefined; exited.then((status) => { dead = status; + // Abort whichever probe is currently in flight -- an exited Chrome + // will never answer, so there's no reason to wait out its timeout. + abortInFlightProbe?.(); }).catch(() => {}); let lastError = "no response"; while (Date.now() < deadline) { - if (dead) { - return `Chrome exited (code ${dead.code}) before its CDP endpoint ` + - `on port ${port} became ready`; + const deadBeforeProbe = getDead(); + if (deadBeforeProbe) { + return exitedMessage(deadBeforeProbe); } + const controller = new AbortController(); + abortInFlightProbe = () => controller.abort(); + const timeoutId = setTimeout( + () => controller.abort(), + probeTimeoutMs(deadline - Date.now()), + ); try { const response = await fetch(`http://localhost:${port}/json/list`, { - signal: AbortSignal.timeout(probeTimeoutMs(deadline - Date.now())), + signal: controller.signal, }); const body = await response.json().catch(() => undefined); if (response.ok && isReady(body)) { @@ -158,7 +178,14 @@ async function waitForCdpEndpoint( ? "CDP endpoint has no ready page target yet" : `CDP endpoint returned ${response.status}`; } catch (e) { + const deadAfterProbe = getDead(); + if (deadAfterProbe) { + return exitedMessage(deadAfterProbe); + } lastError = e instanceof Error ? e.message : String(e); + } finally { + clearTimeout(timeoutId); + abortInFlightProbe = undefined; } await sleep(interval); } diff --git a/tests/unit/chrome-launch-flags.test.ts b/tests/unit/chrome-launch-flags.test.ts index 0a7adbb185..c87bc3b250 100644 --- a/tests/unit/chrome-launch-flags.test.ts +++ b/tests/unit/chrome-launch-flags.test.ts @@ -172,3 +172,101 @@ unitTest( } }, ); + +// A fake Chrome whose /json/list mirrors what real Chrome (and +// chrome-headless-shell, verified by hand against the actual binary) does: +// a page target exists only once a URL was given on the command line to +// open -- launched with no positional URL, chrome-headless-shell reports +// zero targets until something asks it to create one. criClient's own +// awaitPageTarget gate has no other way to get a target created, since +// deno-cri's own target-creation only runs later, at connect time. +async function writeUrlGatedChromeExecutable( + dir: string, + port: number, +): Promise { + const scriptPath = join(dir, "url-gated-chrome.js"); + await Deno.writeTextFile( + scriptPath, + [ + 'const hasUrl = Deno.args.some((a) => !a.startsWith("-"));', + `Deno.serve({ port: ${port}, onListen: () => {} }, (req) => {`, + " const url = new URL(req.url);", + ' if (url.pathname === "/json/list") {', + " const body = hasUrl", + ` ? JSON.stringify([{ type: "page", webSocketDebuggerUrl: "ws://localhost:${port}/devtools/page/1" }])`, + ' : "[]";', + " return new Response(body, { status: 200 });", + ' } else if (url.pathname === "/shutdown") {', + " setTimeout(() => Deno.exit(0), 50);", + ' return new Response("", { status: 200 });', + " }", + ' return new Response("", { status: 404 });', + "});", + ].join("\n"), + ); + + const deno = Deno.execPath(); + if (Deno.build.os === "windows") { + const cmdPath = join(dir, "url-gated-chrome.cmd"); + await Deno.writeTextFile( + cmdPath, + `@echo off\r\n"${deno}" run --allow-net "${scriptPath}" %*\r\n`, + ); + return cmdPath; + } + const shPath = join(dir, "url-gated-chrome.sh"); + await Deno.writeTextFile( + shPath, + `#!/bin/sh\n"${deno}" run --allow-net "${scriptPath}" "$@"\n`, + ); + await Deno.chmod(shPath, 0o755); + return shPath; +} + +unitTest( + "chrome-launch-flags - criClient gets a page target from a Chrome that needs a URL to create one", + async () => { + const port = findOpenPort(); + const dir = await Deno.makeTempDir({ + prefix: "chrome-launch-url-gated-", + }); + let primaryError: unknown; + try { + const fakeChrome = await writeUrlGatedChromeExecutable(dir, port); + await criClient(fakeChrome, port); + } catch (e) { + primaryError = e; + } + + const cleanupErrors: unknown[] = []; + await fetch(`http://localhost:${port}/shutdown`).catch(() => {}); + const closed = await waitForPortClosed(port); + if (!closed) { + cleanupErrors.push( + new Error( + `fake Chrome on port ${port} did not shut down; left ${dir} in place`, + ), + ); + } else { + await Deno.remove(dir, { recursive: true }).catch((e) => { + cleanupErrors.push(new Error(`failed to remove ${dir}: ${e}`)); + }); + } + + if (primaryError !== undefined && cleanupErrors.length > 0) { + throw new AggregateError( + [primaryError, ...cleanupErrors], + "chrome-launch-flags test failed and cleanup also failed", + ); + } + if (primaryError !== undefined) { + throw primaryError; + } + if (cleanupErrors.length > 0) { + throw new AggregateError( + cleanupErrors, + "chrome-launch-flags cleanup failed", + ); + } + }, +); diff --git a/tests/unit/chrome-launch.test.ts b/tests/unit/chrome-launch.test.ts index 3418dbfc35..b755ad3707 100644 --- a/tests/unit/chrome-launch.test.ts +++ b/tests/unit/chrome-launch.test.ts @@ -39,6 +39,7 @@ async function writeSilentChromeExecutable( dir: string, port: number, stderrText: string, + selfDestructMs: number = kSelfDestructMs, ): Promise { const scriptPath = join(dir, "silent-chrome.js"); await Deno.writeTextFile( @@ -55,7 +56,7 @@ async function writeSilentChromeExecutable( JSON.stringify(stderrText) }));` : "", - `setTimeout(() => Deno.exit(1), ${kSelfDestructMs});`, + `setTimeout(() => Deno.exit(1), ${selfDestructMs});`, ].join("\n"), ); @@ -139,6 +140,44 @@ unitTest( }, ); +unitTest( + "chrome-launch - reports a Chrome exit promptly, not after the in-flight probe times out", + async () => { + const port = findOpenPort(); + const dir = await Deno.makeTempDir({ prefix: "chrome-launch-silent-" }); + try { + // Self-destructs quickly, but the launch timeout is generous -- if + // process exit were only checked between probes, the in-flight probe + // against this genuinely silent port could run for up to a full + // probeTimeoutMs (up to 1000ms) after the exit before it's noticed. + const selfDestructMs = 200; + const fakeChrome = await writeSilentChromeExecutable( + dir, + port, + "", + selfDestructMs, + ); + const start = Date.now(); + const err = await assertRejects( + () => launchChrome({ appPath: fakeChrome, port, timeout: 5000 }), + Error, + ); + const elapsed = Date.now() - start; + assert( + err.message.includes("Chrome exited"), + `expected an exit message, got: ${err.message}`, + ); + assert( + elapsed < selfDestructMs + 500, + `expected exit to be reported well under a probe timeout after ` + + `it happened (${selfDestructMs}ms + margin), took ${elapsed}ms`, + ); + } finally { + await Deno.remove(dir, { recursive: true }).catch(() => {}); + } + }, +); + unitTest( "chrome-launch - CdpClient.connect exhausts its retries with a useful message", async () => { From 453313d6df631b49f8cdf29a5db6980a2d0bb982 Mon Sep 17 00:00:00 2001 From: Christophe Dervieux Date: Tue, 8 Sep 2026 14:59:12 +0200 Subject: [PATCH 13/14] cri: measure exit-detection latency from the child's own clock Roborev flagged the exit-report timing assertion as flaky: elapsed time was measured from before the fake Chrome subprocess was even spawned, so subprocess startup on a loaded Windows host swamped the narrow abort-in-flight-probe window the test meant to verify (observed 1489ms against a 700ms budget). The child now records its own exit timestamp synchronously right before exiting, and the test measures detection latency against that instead of wall time from before the spawn. --- tests/unit/chrome-launch.test.ts | 40 +++++++++++++++++++++++++------- 1 file changed, 32 insertions(+), 8 deletions(-) diff --git a/tests/unit/chrome-launch.test.ts b/tests/unit/chrome-launch.test.ts index b755ad3707..133bd0d541 100644 --- a/tests/unit/chrome-launch.test.ts +++ b/tests/unit/chrome-launch.test.ts @@ -40,6 +40,7 @@ async function writeSilentChromeExecutable( port: number, stderrText: string, selfDestructMs: number = kSelfDestructMs, + exitTimestampPath?: string, ): Promise { const scriptPath = join(dir, "silent-chrome.js"); await Deno.writeTextFile( @@ -56,23 +57,38 @@ async function writeSilentChromeExecutable( JSON.stringify(stderrText) }));` : "", - `setTimeout(() => Deno.exit(1), ${selfDestructMs});`, + `setTimeout(() => {`, + // Recorded synchronously, right at the moment of exit, so the test + // can measure launchChrome's detection latency against the process's + // own clock instead of wall time from before it was even spawned -- + // subprocess startup (a second or more on a loaded Windows host) would + // otherwise swamp the narrow window this is meant to verify. + exitTimestampPath + ? ` Deno.writeTextFileSync(${ + JSON.stringify(exitTimestampPath) + }, String(Date.now()));` + : "", + ` Deno.exit(1);`, + `}, ${selfDestructMs});`, ].join("\n"), ); const deno = Deno.execPath(); + const allowWrite = exitTimestampPath + ? ` --allow-write="${exitTimestampPath}"` + : ""; if (Deno.build.os === "windows") { const cmdPath = join(dir, "silent-chrome.cmd"); await Deno.writeTextFile( cmdPath, - `@echo off\r\n"${deno}" run --allow-net "${scriptPath}" %*\r\n`, + `@echo off\r\n"${deno}" run --allow-net${allowWrite} "${scriptPath}" %*\r\n`, ); return cmdPath; } const shPath = join(dir, "silent-chrome.sh"); await Deno.writeTextFile( shPath, - `#!/bin/sh\n"${deno}" run --allow-net "${scriptPath}" "$@"\n`, + `#!/bin/sh\n"${deno}" run --allow-net${allowWrite} "${scriptPath}" "$@"\n`, ); await Deno.chmod(shPath, 0o755); return shPath; @@ -151,26 +167,34 @@ unitTest( // against this genuinely silent port could run for up to a full // probeTimeoutMs (up to 1000ms) after the exit before it's noticed. const selfDestructMs = 200; + const exitTimestampPath = join(dir, "exit-timestamp.txt"); const fakeChrome = await writeSilentChromeExecutable( dir, port, "", selfDestructMs, + exitTimestampPath, ); - const start = Date.now(); const err = await assertRejects( () => launchChrome({ appPath: fakeChrome, port, timeout: 5000 }), Error, ); - const elapsed = Date.now() - start; + // Measured from the child's own exit, not from before it was spawned -- + // subprocess startup time is irrelevant noise for what this test + // verifies (that an in-flight probe is aborted promptly on exit, + // rather than waiting out its own timeout). + const exitTimestamp = Number( + await Deno.readTextFile(exitTimestampPath), + ); + const detectionLatency = Date.now() - exitTimestamp; assert( err.message.includes("Chrome exited"), `expected an exit message, got: ${err.message}`, ); assert( - elapsed < selfDestructMs + 500, - `expected exit to be reported well under a probe timeout after ` + - `it happened (${selfDestructMs}ms + margin), took ${elapsed}ms`, + detectionLatency < 500, + `expected the exit to be detected within 500ms of the process ` + + `actually exiting, took ${detectionLatency}ms`, ); } finally { await Deno.remove(dir, { recursive: true }).catch(() => {}); From ed17bb3449b5ba6954051bc421dad7775f6e6a12 Mon Sep 17 00:00:00 2001 From: Christophe Dervieux Date: Tue, 8 Sep 2026 15:07:08 +0200 Subject: [PATCH 14/14] cri: trim redundant/implementation-detail tests, share fake-executable scaffolding CdpClient.connect's unavailable-port test duplicated the deterministic connectCdp coverage while costing ~12s per run -- traced to Deno's fetch taking ~2.3s per refused connect against a closed "localhost" port (measured directly; not an internal deno-cri retry). Real integration coverage of CdpClient against the vendored deno-cri library already exists via tests/smoke/axe/axe-transport-failclosed.test.ts, which drives a real browser through connect/close/dropped-socket. Dropped the no-dangling-colon-on-empty-stderr case (a cosmetic message branch already exercised structurally by the non-empty-stderr test) and the direct probeTimeoutMs unit test, making probeTimeoutMs private again now that nothing outside launch.ts needs to call it directly. In chrome-launch-flags.test.ts, replaced the second URL-gated fake-browser fixture and its dedicated test with a one-line argv assertion on the existing launch-arguments test, since criClient always passes the same url: "about:blank" regardless of fixture behavior. Extracted the repeated "write a fake Chrome .cmd/.sh wrapper executable" boilerplate (three near-identical copies across the two files) into a shared writeFakeExecutable helper in the new chrome-launch-fixtures.ts. --- src/core/cri/launch.ts | 2 +- tests/unit/chrome-launch-fixtures.ts | 51 +++++++ tests/unit/chrome-launch-flags.test.ts | 144 +++----------------- tests/unit/chrome-launch.test.ts | 176 +++++-------------------- 4 files changed, 101 insertions(+), 272 deletions(-) create mode 100644 tests/unit/chrome-launch-fixtures.ts diff --git a/src/core/cri/launch.ts b/src/core/cri/launch.ts index 6066da211b..1c8c345059 100644 --- a/src/core/cri/launch.ts +++ b/src/core/cri/launch.ts @@ -92,7 +92,7 @@ const kProbeAttemptTimeout = 1000; * near the end of the launch timeout can't itself blow past it by up to a * full `kProbeAttemptTimeout`. */ -export function probeTimeoutMs(remainingMs: number): number { +function probeTimeoutMs(remainingMs: number): number { return Math.max(0, Math.min(kProbeAttemptTimeout, remainingMs)); } diff --git a/tests/unit/chrome-launch-fixtures.ts b/tests/unit/chrome-launch-fixtures.ts new file mode 100644 index 0000000000..fc730e62d1 --- /dev/null +++ b/tests/unit/chrome-launch-fixtures.ts @@ -0,0 +1,51 @@ +/* + * chrome-launch-fixtures.ts + * + * Copyright (C) 2026 Posit Software, PBC + */ + +import { join } from "path"; + +/** + * Writes a small Deno script plus a platform-native wrapper that re-invokes + * the currently running deno binary to run it (.cmd on Windows, a shebang + * script on Unix). A wrapper is needed because launchChrome()/criClient() + * always prepend real Chrome flags as argv, which the fake executable must + * tolerate. Returns the wrapper's path. + * + * The wrapper re-invokes deno.exe as a grandchild: on Windows, kill() only + * reaches the direct child (the .cmd's cmd.exe host), and the deno.exe + * grandchild survives as an orphan holding its inherited stderr pipe open, + * which would otherwise hang launchChrome's close() forever. Callers whose + * script keeps a listener or server running should have it self-terminate + * on its own timer rather than relying on close() to reach it. + */ +export async function writeFakeExecutable( + dir: string, + name: string, + scriptLines: string[], + allowWritePath?: string, +): Promise { + const scriptPath = join(dir, `${name}.js`); + await Deno.writeTextFile(scriptPath, scriptLines.join("\n")); + + const deno = Deno.execPath(); + const allowWrite = allowWritePath + ? ` --allow-write="${allowWritePath}"` + : ""; + if (Deno.build.os === "windows") { + const cmdPath = join(dir, `${name}.cmd`); + await Deno.writeTextFile( + cmdPath, + `@echo off\r\n"${deno}" run --allow-net${allowWrite} "${scriptPath}" %*\r\n`, + ); + return cmdPath; + } + const shPath = join(dir, `${name}.sh`); + await Deno.writeTextFile( + shPath, + `#!/bin/sh\n"${deno}" run --allow-net${allowWrite} "${scriptPath}" "$@"\n`, + ); + await Deno.chmod(shPath, 0o755); + return shPath; +} diff --git a/tests/unit/chrome-launch-flags.test.ts b/tests/unit/chrome-launch-flags.test.ts index c87bc3b250..78b9a1b8f7 100644 --- a/tests/unit/chrome-launch-flags.test.ts +++ b/tests/unit/chrome-launch-flags.test.ts @@ -9,22 +9,20 @@ import { join } from "path"; import { criClient } from "../../src/core/cri/cri.ts"; import { findOpenPort } from "../../src/core/port.ts"; import { unitTest } from "../test.ts"; +import { writeFakeExecutable } from "./chrome-launch-fixtures.ts"; -// Generates a fake Chrome executable: a platform-native wrapper (.cmd on -// Windows, a shebang script on Unix) that re-invokes the currently running -// deno binary to run a small script. That script records its own argv -- -// exactly what criClient passed as Chrome's command line -- to a JSON file, -// then serves /json/list so criClient's readiness check resolves. A -// /shutdown route lets the test terminate it deterministically instead of -// guessing at a lifetime. +// A fake Chrome that records its own argv -- exactly what criClient passed +// as Chrome's command line -- to a JSON file, then serves /json/list so +// criClient's readiness check resolves. A /shutdown route lets the test +// terminate it deterministically instead of guessing at a lifetime. async function writeFakeChromeExecutable( dir: string, argvOutPath: string, port: number, ): Promise { - const scriptPath = join(dir, "fake-chrome.js"); - await Deno.writeTextFile( - scriptPath, + return writeFakeExecutable( + dir, + "fake-chrome", [ "const args = Deno.args;", `await Deno.writeTextFile(${ @@ -46,25 +44,9 @@ async function writeFakeChromeExecutable( " }", ' return new Response("", { status: 404 });', "});", - ].join("\n"), + ], + argvOutPath, ); - - const deno = Deno.execPath(); - if (Deno.build.os === "windows") { - const cmdPath = join(dir, "fake-chrome.cmd"); - await Deno.writeTextFile( - cmdPath, - `@echo off\r\n"${deno}" run --allow-net --allow-write="${argvOutPath}" "${scriptPath}" %*\r\n`, - ); - return cmdPath; - } - const shPath = join(dir, "fake-chrome.sh"); - await Deno.writeTextFile( - shPath, - `#!/bin/sh\n"${deno}" run --allow-net --allow-write="${argvOutPath}" "${scriptPath}" "$@"\n`, - ); - await Deno.chmod(shPath, 0o755); - return shPath; } // Polls until the fake Chrome's port stops answering, i.e. the process has @@ -132,6 +114,14 @@ unitTest( !argv.some((a) => a.startsWith("--user-data-dir")), `expected no --user-data-dir in ${JSON.stringify(argv)}`, ); + // criClient passes url: "about:blank" so chrome-headless-shell creates + // a page target on its own during startup -- without it, a Chrome + // launched with no positional URL reports zero targets at all, and + // criClient's awaitPageTarget wait never succeeds. + assert( + argv.includes("about:blank"), + `expected about:blank in ${JSON.stringify(argv)}`, + ); } catch (e) { primaryError = e; } @@ -172,101 +162,3 @@ unitTest( } }, ); - -// A fake Chrome whose /json/list mirrors what real Chrome (and -// chrome-headless-shell, verified by hand against the actual binary) does: -// a page target exists only once a URL was given on the command line to -// open -- launched with no positional URL, chrome-headless-shell reports -// zero targets until something asks it to create one. criClient's own -// awaitPageTarget gate has no other way to get a target created, since -// deno-cri's own target-creation only runs later, at connect time. -async function writeUrlGatedChromeExecutable( - dir: string, - port: number, -): Promise { - const scriptPath = join(dir, "url-gated-chrome.js"); - await Deno.writeTextFile( - scriptPath, - [ - 'const hasUrl = Deno.args.some((a) => !a.startsWith("-"));', - `Deno.serve({ port: ${port}, onListen: () => {} }, (req) => {`, - " const url = new URL(req.url);", - ' if (url.pathname === "/json/list") {', - " const body = hasUrl", - ` ? JSON.stringify([{ type: "page", webSocketDebuggerUrl: "ws://localhost:${port}/devtools/page/1" }])`, - ' : "[]";', - " return new Response(body, { status: 200 });", - ' } else if (url.pathname === "/shutdown") {', - " setTimeout(() => Deno.exit(0), 50);", - ' return new Response("", { status: 200 });', - " }", - ' return new Response("", { status: 404 });', - "});", - ].join("\n"), - ); - - const deno = Deno.execPath(); - if (Deno.build.os === "windows") { - const cmdPath = join(dir, "url-gated-chrome.cmd"); - await Deno.writeTextFile( - cmdPath, - `@echo off\r\n"${deno}" run --allow-net "${scriptPath}" %*\r\n`, - ); - return cmdPath; - } - const shPath = join(dir, "url-gated-chrome.sh"); - await Deno.writeTextFile( - shPath, - `#!/bin/sh\n"${deno}" run --allow-net "${scriptPath}" "$@"\n`, - ); - await Deno.chmod(shPath, 0o755); - return shPath; -} - -unitTest( - "chrome-launch-flags - criClient gets a page target from a Chrome that needs a URL to create one", - async () => { - const port = findOpenPort(); - const dir = await Deno.makeTempDir({ - prefix: "chrome-launch-url-gated-", - }); - let primaryError: unknown; - try { - const fakeChrome = await writeUrlGatedChromeExecutable(dir, port); - await criClient(fakeChrome, port); - } catch (e) { - primaryError = e; - } - - const cleanupErrors: unknown[] = []; - await fetch(`http://localhost:${port}/shutdown`).catch(() => {}); - const closed = await waitForPortClosed(port); - if (!closed) { - cleanupErrors.push( - new Error( - `fake Chrome on port ${port} did not shut down; left ${dir} in place`, - ), - ); - } else { - await Deno.remove(dir, { recursive: true }).catch((e) => { - cleanupErrors.push(new Error(`failed to remove ${dir}: ${e}`)); - }); - } - - if (primaryError !== undefined && cleanupErrors.length > 0) { - throw new AggregateError( - [primaryError, ...cleanupErrors], - "chrome-launch-flags test failed and cleanup also failed", - ); - } - if (primaryError !== undefined) { - throw primaryError; - } - if (cleanupErrors.length > 0) { - throw new AggregateError( - cleanupErrors, - "chrome-launch-flags cleanup failed", - ); - } - }, -); diff --git a/tests/unit/chrome-launch.test.ts b/tests/unit/chrome-launch.test.ts index 133bd0d541..bcd4b22447 100644 --- a/tests/unit/chrome-launch.test.ts +++ b/tests/unit/chrome-launch.test.ts @@ -6,15 +6,11 @@ import { assert, assertEquals, assertRejects } from "testing/asserts"; import { join } from "path"; -import { - connectCdp, - hasPageTarget, - launchChrome, - probeTimeoutMs, -} from "../../src/core/cri/launch.ts"; -import { CdpClient } from "../../src/command/call/axe/scan.ts"; +import { connectCdp, hasPageTarget, launchChrome } from + "../../src/core/cri/launch.ts"; import { findOpenPort } from "../../src/core/port.ts"; import { unitTest } from "../test.ts"; +import { writeFakeExecutable } from "./chrome-launch-fixtures.ts"; /** Short enough that the test doesn't wait around, long enough to be real. */ const kLaunchTimeout = 300; @@ -24,17 +20,7 @@ const kSelfDestructMs = 2000; // A fake Chrome that accepts the TCP connection on its CDP port but never // answers on it -- genuinely silent, not merely absent, so a `fetch()` // against it hangs on the response rather than failing fast with connection -// refused. Same wrapper mechanism as chrome-launch-flags.test.ts's fake -// executable (.cmd on Windows, a shebang script on Unix, re-invoking the -// current deno binary to run an inline script -- a wrapper is needed because -// launchChrome() always prepends real Chrome flags as argv, which the fake -// executable must tolerate). -// -// It self-exits on its own short timer rather than relying on launchChrome's -// kill() to reach it: on Windows, kill() only reaches the direct child (the -// .cmd's cmd.exe host), and the actual deno.exe process survives as an -// orphan holding its inherited stderr pipe open, which would otherwise hang -// launchChrome's close() until the timer fires anyway. +// refused. async function writeSilentChromeExecutable( dir: string, port: number, @@ -42,9 +28,9 @@ async function writeSilentChromeExecutable( selfDestructMs: number = kSelfDestructMs, exitTimestampPath?: string, ): Promise { - const scriptPath = join(dir, "silent-chrome.js"); - await Deno.writeTextFile( - scriptPath, + return writeFakeExecutable( + dir, + "silent-chrome", [ `const listener = Deno.listen({ port: ${port} });`, "(async () => {", @@ -70,28 +56,9 @@ async function writeSilentChromeExecutable( : "", ` Deno.exit(1);`, `}, ${selfDestructMs});`, - ].join("\n"), + ], + exitTimestampPath, ); - - const deno = Deno.execPath(); - const allowWrite = exitTimestampPath - ? ` --allow-write="${exitTimestampPath}"` - : ""; - if (Deno.build.os === "windows") { - const cmdPath = join(dir, "silent-chrome.cmd"); - await Deno.writeTextFile( - cmdPath, - `@echo off\r\n"${deno}" run --allow-net${allowWrite} "${scriptPath}" %*\r\n`, - ); - return cmdPath; - } - const shPath = join(dir, "silent-chrome.sh"); - await Deno.writeTextFile( - shPath, - `#!/bin/sh\n"${deno}" run --allow-net${allowWrite} "${scriptPath}" "$@"\n`, - ); - await Deno.chmod(shPath, 0o755); - return shPath; } unitTest( @@ -126,36 +93,6 @@ unitTest( }, ); -unitTest( - "chrome-launch - alive-but-silent Chrome rejects, no dangling colon on empty stderr", - async () => { - const port = findOpenPort(); - const dir = await Deno.makeTempDir({ prefix: "chrome-launch-silent-" }); - try { - const fakeChrome = await writeSilentChromeExecutable(dir, port, ""); - const err = await assertRejects( - () => - launchChrome({ appPath: fakeChrome, port, timeout: kLaunchTimeout }), - Error, - ); - assert( - err.message.includes(String(port)), - `expected port ${port} named in: ${err.message}`, - ); - assert( - !err.message.includes("Chrome said:"), - `expected no "Chrome said:" for empty stderr in: ${err.message}`, - ); - assert( - !err.message.trimEnd().endsWith(":"), - `expected no dangling colon in: ${err.message}`, - ); - } finally { - await Deno.remove(dir, { recursive: true }).catch(() => {}); - } - }, -); - unitTest( "chrome-launch - reports a Chrome exit promptly, not after the in-flight probe times out", async () => { @@ -202,18 +139,6 @@ unitTest( }, ); -unitTest( - "chrome-launch - CdpClient.connect exhausts its retries with a useful message", - async () => { - const port = findOpenPort(); - const err = await assertRejects(() => CdpClient.connect(port), Error); - assert( - err.message.includes(String(port)), - `expected port ${port} named in: ${err.message}`, - ); - }, -); - // deno-lint-ignore require-await unitTest( "chrome-launch - hasPageTarget requires target.type to be page", @@ -237,19 +162,6 @@ unitTest( }, ); -// deno-lint-ignore require-await -unitTest( - "chrome-launch - probeTimeoutMs never exceeds what's left of the launch budget", - async () => { - // Plenty of time left: capped at the per-attempt ceiling, not the full budget. - assertEquals(probeTimeoutMs(5000), 1000); - // Little time left: capped at what's actually left, not the per-attempt ceiling. - assertEquals(probeTimeoutMs(50), 50); - // Already past the deadline: clamps to zero rather than going negative. - assertEquals(probeTimeoutMs(-10), 0); - }, -); - function countingConnector(failuresBeforeSuccess: number) { let attempts = 0; const connect = (_port: number) => { @@ -287,60 +199,34 @@ unitTest( /** * A fake Chrome that serves `/json/list` itself, controlling exactly what the - * launcher's readiness poll sees -- same Deno.serve wrapper mechanism as - * chrome-launch-flags.test.ts's fake executable. `pageAfterMs` is measured - * from the fake server's own start, not the caller's: `[]` until then, a - * single real page target afterward. `undefined` means never. + * launcher's readiness poll sees. `pageAfterMs` is measured from the fake + * server's own start, not the caller's: `[]` until then, a single real page + * target afterward. `undefined` means never. */ async function writeJsonListChromeExecutable( dir: string, port: number, pageAfterMs: number | undefined, ): Promise { - const scriptPath = join(dir, "json-list-chrome.js"); - await Deno.writeTextFile( - scriptPath, - [ - "const start = Date.now();", - `const pageAfterMs = ${ - pageAfterMs === undefined ? "undefined" : pageAfterMs - };`, - `Deno.serve({ port: ${port}, onListen: () => {} }, (req) => {`, - " const url = new URL(req.url);", - ' if (url.pathname !== "/json/list") {', - ' return new Response("", { status: 404 });', - " }", - " const ready = pageAfterMs !== undefined &&", - " Date.now() - start >= pageAfterMs;", - " const body = ready", - ` ? JSON.stringify([{ type: "page", webSocketDebuggerUrl: "ws://localhost:${port}/devtools/page/1" }])`, - ' : "[]";', - " return new Response(body, { status: 200 });", - "});", - // Same Windows orphan issue as writeSilentChromeExecutable above: - // launchChrome's close() only kills the .cmd's cmd.exe host, and the - // deno.exe grandchild survives holding its inherited stderr pipe open, - // which would otherwise hang close()'s drain loop forever. - `setTimeout(() => Deno.exit(0), ${kSelfDestructMs});`, - ].join("\n"), - ); - - const deno = Deno.execPath(); - if (Deno.build.os === "windows") { - const cmdPath = join(dir, "json-list-chrome.cmd"); - await Deno.writeTextFile( - cmdPath, - `@echo off\r\n"${deno}" run --allow-net "${scriptPath}" %*\r\n`, - ); - return cmdPath; - } - const shPath = join(dir, "json-list-chrome.sh"); - await Deno.writeTextFile( - shPath, - `#!/bin/sh\n"${deno}" run --allow-net "${scriptPath}" "$@"\n`, - ); - await Deno.chmod(shPath, 0o755); - return shPath; + return writeFakeExecutable(dir, "json-list-chrome", [ + "const start = Date.now();", + `const pageAfterMs = ${ + pageAfterMs === undefined ? "undefined" : pageAfterMs + };`, + `Deno.serve({ port: ${port}, onListen: () => {} }, (req) => {`, + " const url = new URL(req.url);", + ' if (url.pathname !== "/json/list") {', + ' return new Response("", { status: 404 });', + " }", + " const ready = pageAfterMs !== undefined &&", + " Date.now() - start >= pageAfterMs;", + " const body = ready", + ` ? JSON.stringify([{ type: "page", webSocketDebuggerUrl: "ws://localhost:${port}/devtools/page/1" }])`, + ' : "[]";', + " return new Response(body, { status: 200 });", + "});", + `setTimeout(() => Deno.exit(0), ${kSelfDestructMs});`, + ]); } unitTest(