diff --git a/llm-docs/axe-scan-architecture.md b/llm-docs/axe-scan-architecture.md index f1b077f2f8..2f7c5a8fad 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 @@ -282,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/src/command/call/axe/scan.ts b/src/command/call/axe/scan.ts index f237c8c2e3..63866ba260 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 { debug } from "../../../deno_ral/log.ts"; +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 { 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 cdp from "../../../core/cri/deno-cri/index.js"; import { AxeScanConfig, AxeViewport } from "./config.ts"; import { AxeMode, AxePage } from "./discover.ts"; @@ -101,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( @@ -157,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)); + }, + ); }); } @@ -177,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)); + } } } @@ -265,122 +242,30 @@ 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 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); - client = await CdpClient.connect(wsUrl); + client = await CdpClient.connect(browser.port); } 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}` : ""), + asError(e).message + (detail ? `\nChrome said: ${detail}` : ""), ); } @@ -388,7 +273,7 @@ export async function launchScanBrowser(port: number): Promise { client, close: async () => { client.close(); - await shutdown(); + await browser.close(); }, }; } 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, diff --git a/src/core/cri/launch.ts b/src/core/cri/launch.ts new file mode 100644 index 0000000000..f78c7e9ac0 --- /dev/null +++ b/src/core/cri/launch.ts @@ -0,0 +1,219 @@ +/* + * launch.ts + * + * 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 + * (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. + * + * 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 + * 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; 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"), + ], +});