From 2e0a4e00914110f1f0f09f159bd8cbf65a1d151e Mon Sep 17 00:00:00 2001 From: Charlotte Wickham Date: Mon, 31 Aug 2026 14:48:47 -0700 Subject: [PATCH 1/6] 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 653bb8a79bbdb95dd7ca2c5701c4cb1684f1d788 Mon Sep 17 00:00:00 2001 From: Charlotte Wickham Date: Mon, 31 Aug 2026 14:50:31 -0700 Subject: [PATCH 2/6] 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 6f2e5261c6abb49835930ac6f99e126357bad3e5 Mon Sep 17 00:00:00 2001 From: Charlotte Wickham Date: Mon, 31 Aug 2026 14:53:12 -0700 Subject: [PATCH 3/6] 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 f237c8c2e3..6eccd58ae2 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 d994febcdbf6c53c1b39eaeb49671b2b253a222a Mon Sep 17 00:00:00 2001 From: Charlotte Wickham Date: Mon, 31 Aug 2026 14:56:19 -0700 Subject: [PATCH 4/6] 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 6eccd58ae2..63866ba260 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 142f0f94d9d8950bbb5bc300aaa73d800815a3c2 Mon Sep 17 00:00:00 2001 From: Charlotte Wickham Date: Mon, 31 Aug 2026 14:59:30 -0700 Subject: [PATCH 5/6] 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 f1b077f2f8..25d247d280 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 bbf61ba07b4d739067b0b7997d125cfbae4f55db Mon Sep 17 00:00:00 2001 From: Charlotte Wickham Date: Mon, 31 Aug 2026 15:54:31 -0700 Subject: [PATCH 6/6] 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 25d247d280..2f7c5a8fad 100644 --- a/llm-docs/axe-scan-architecture.md +++ b/llm-docs/axe-scan-architecture.md @@ -303,6 +303,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"), + ], +});