From fbe7b7888d480bf2098ee34bac15455b62e76fb7 Mon Sep 17 00:00:00 2001 From: probe Date: Tue, 10 Nov 2026 23:53:26 +0800 Subject: [PATCH 1/5] test(webui): read the browser suites' Chrome from the provider's own table MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Five copies of "where is Chrome" lived in the browser integration suites, and three of them only knew the Linux paths. On macOS the real browser lives in /Applications, so those copies answered "no browser" and describe.skipIf(!available) dropped the whole suite. The suites that ran on ubuntu and were skipped in their entirety on macOS: isolated-chromium-screencast 3 cases browser-panel-live-navigation browser-use-real-input 2 cases workspace-browser-tab-acceptance 14 cases isolated-chromium-screencast is the only coverage for a frame that keeps coming — the case that would have caught a browser panel waiting forever for a picture that was never coming. The suites now read the provider's own candidate table (defaultChromeCandidates()) through a shared helper, and the helper asks for an executable rather than for a file that happens to exist. No expectation, timeout or assertion changed: on a mac with Chrome installed the four suites go from 22 skipped to 22 passed. --- .../browser-panel-live-navigation.test.ts | 21 +------- .../browser-use-real-input.test.ts | 48 ++---------------- .../test/integration/chrome-executable.ts | 49 +++++++++++++++++++ .../isolated-chromium-profile.test.ts | 29 +---------- .../isolated-chromium-screencast.test.ts | 21 +------- .../isolated-chromium-two-pages.test.ts | 33 +------------ .../workspace-browser-tab-acceptance.test.ts | 27 ++-------- release/public-source.json | 1 + 8 files changed, 67 insertions(+), 162 deletions(-) create mode 100644 packages/webui/test/integration/chrome-executable.ts diff --git a/packages/webui/test/integration/browser-panel-live-navigation.test.ts b/packages/webui/test/integration/browser-panel-live-navigation.test.ts index 345d51c6c..1fa82af34 100644 --- a/packages/webui/test/integration/browser-panel-live-navigation.test.ts +++ b/packages/webui/test/integration/browser-panel-live-navigation.test.ts @@ -19,28 +19,11 @@ import type { WebuiBrowserTargetId, WebuiBrowserWebTabSnapshot, } from "../../src/shared/contracts/browser.js"; +import { resolveChromeExecutable } from "./chrome-executable.js"; const TIMEOUT_MS = 30_000; const POLL_MS = 50; -async function resolveChromePath(): Promise { - const { access } = await import("node:fs/promises"); - for (const candidate of [ - "/usr/bin/google-chrome", - "/usr/bin/google-chrome-stable", - "/usr/bin/chromium", - "/usr/bin/chromium-browser", - ]) { - try { - await access(candidate); - return candidate; - } catch { - // Keep looking. - } - } - return undefined; -} - async function startPageServer(respond: (path: string) => string): Promise<{ server: Server; origin: string; @@ -58,7 +41,7 @@ async function startPageServer(respond: (path: string) => string): Promise<{ return { server, origin: `http://127.0.0.1:${address.port}` }; } -const chromeAvailable = (await resolveChromePath()) !== undefined; +const chromeAvailable = (await resolveChromeExecutable()) !== undefined; /** * Whether a failure is the page behind a reference moving on. diff --git a/packages/webui/test/integration/browser-use-real-input.test.ts b/packages/webui/test/integration/browser-use-real-input.test.ts index 624832078..e7a0b5292 100644 --- a/packages/webui/test/integration/browser-use-real-input.test.ts +++ b/packages/webui/test/integration/browser-use-real-input.test.ts @@ -24,6 +24,7 @@ import { afterAll, beforeAll, describe, expect, it } from "vitest"; import { WebuiIsolatedChromiumProvider } from "../../src/runtime/browser/isolated-chromium/provider.js"; import type { WebuiBrowserTargetId } from "../../src/shared/contracts/browser.js"; import { beginAcceptanceSuite, observeAcceptance, restoreAcceptanceRecords } from "../browser/acceptance-record.js"; +import { chromeExecutableVersion, resolveChromeExecutable } from "./chrome-executable.js"; /** The record file this suite writes. */ const SUITE = "isolated-chromium"; @@ -46,48 +47,7 @@ const PAGE_HTML = [ "", ].join("\n"); -async function chromeIsAvailable(): Promise { - return (await resolveChromePath()) !== undefined; -} - -/** The Chrome the isolated provider drives, and its own version. */ -async function resolveChromePath(): Promise { - const { access } = await import("node:fs/promises"); - for (const candidate of [ - "/usr/bin/google-chrome", - "/usr/bin/google-chrome-stable", - "/usr/bin/chromium", - "/usr/bin/chromium-browser", - ]) { - try { - await access(candidate); - return candidate; - } catch { - // Keep looking. - } - } - return undefined; -} - -/** - * The browser's own version. - * - * Read from the binary rather than assumed: an acceptance record that says - * "Chrome" without saying which Chrome is not a record. - */ -async function chromeVersion(path: string): Promise { - const { execFile } = await import("node:child_process"); - const { promisify } = await import("node:util"); - const run = promisify(execFile); - try { - const { stdout } = await run(path, ["--version"], { timeout: 20_000 }); - return stdout.trim(); - } catch { - return "unknown"; - } -} - -const available = await chromeIsAvailable(); +const available = (await resolveChromeExecutable()) !== undefined; describe.skipIf(!available)("the browser input path, against a real page", () => { let dataDir: string; @@ -100,8 +60,8 @@ describe.skipIf(!available)("the browser input path, against a real page", () => beginAcceptanceSuite(SUITE); dataDir = await mkdtemp(join(tmpdir(), "webui-browser-use-e2e-")); provider = await WebuiIsolatedChromiumProvider.launch({ dataDir }); - const path = await resolveChromePath(); - if (path) browserVersion = await chromeVersion(path); + const path = await resolveChromeExecutable(); + if (path) browserVersion = chromeExecutableVersion(path); }, 60_000); afterAll(async () => { diff --git a/packages/webui/test/integration/chrome-executable.ts b/packages/webui/test/integration/chrome-executable.ts new file mode 100644 index 000000000..414edbf8a --- /dev/null +++ b/packages/webui/test/integration/chrome-executable.ts @@ -0,0 +1,49 @@ +// The browser the browser suites drive. +// +// The repository already knows where a browser lives: the provider's own +// candidate table. These suites read that table instead of keeping a copy of +// it, because a copy is a second truth — and a copy that only knows the Linux +// paths skips the whole suite on a Mac, which is where the coverage quietly +// disappears rather than fails. +// +// The suites still skip themselves when there is no browser: "the browser is +// missing" is not a defect in the provider. + +import { execFileSync } from "node:child_process"; +import { constants } from "node:fs"; +import { access } from "node:fs/promises"; + +import { defaultChromeCandidates } from "../../src/runtime/browser/isolated-chromium/provider.js"; + +/** + * The browser on this machine that can actually be run. + * + * A candidate has to be **executable**, not merely present: a directory, or a + * file without the execute bit, is not a browser, and launching it fails + * later with a much less useful message. + */ +export async function resolveChromeExecutable(): Promise { + for (const candidate of defaultChromeCandidates()) { + try { + await access(candidate, constants.X_OK); + return candidate; + } catch { + // Keep looking. + } + } + return undefined; +} + +/** + * The browser's own version. + * + * Read from the binary rather than assumed: an acceptance record that says + * "Chrome" without saying which Chrome is not a record. + */ +export function chromeExecutableVersion(path: string): string { + try { + return execFileSync(path, ["--version"], { encoding: "utf8", timeout: 20_000 }).trim(); + } catch { + return "unknown"; + } +} diff --git a/packages/webui/test/integration/isolated-chromium-profile.test.ts b/packages/webui/test/integration/isolated-chromium-profile.test.ts index 064d7a4cb..e869ece46 100644 --- a/packages/webui/test/integration/isolated-chromium-profile.test.ts +++ b/packages/webui/test/integration/isolated-chromium-profile.test.ts @@ -20,6 +20,7 @@ import { join } from "node:path"; import { afterAll, beforeAll, describe, expect, it } from "vitest"; import { WebuiIsolatedChromiumProvider } from "../../src/runtime/browser/isolated-chromium/provider.js"; +import { resolveChromeExecutable } from "./chrome-executable.js"; /** The cookie the origin hands out. Its name says what it is for. */ const COOKIE_NAME = "mavis-session"; @@ -63,32 +64,6 @@ async function waitFor( ); } -async function chromeIsAvailable(): Promise { - const { access: canAccess } = await import("node:fs/promises"); - const candidates = - process.platform === "darwin" - ? [ - "/Applications/Google Chrome.app/Contents/MacOS/Google Chrome", - "/Applications/Chromium.app/Contents/MacOS/Chromium", - "/Applications/Brave Browser.app/Contents/MacOS/Brave Browser", - ] - : [ - "/usr/bin/google-chrome", - "/usr/bin/google-chrome-stable", - "/usr/bin/chromium", - "/usr/bin/chromium-browser", - ]; - for (const candidate of candidates) { - try { - await canAccess(candidate); - return true; - } catch { - // Keep looking. - } - } - return false; -} - /** * An origin that hands out a persistent cookie. * @@ -139,7 +114,7 @@ async function visit( ); } -const available = await chromeIsAvailable(); +const available = (await resolveChromeExecutable()) !== undefined; describe.skipIf(!available)("the isolated Chromium's profile", () => { let dataDir: string; diff --git a/packages/webui/test/integration/isolated-chromium-screencast.test.ts b/packages/webui/test/integration/isolated-chromium-screencast.test.ts index 09dc93263..eb557bcc6 100644 --- a/packages/webui/test/integration/isolated-chromium-screencast.test.ts +++ b/packages/webui/test/integration/isolated-chromium-screencast.test.ts @@ -22,6 +22,7 @@ import { afterAll, beforeAll, describe, expect, it } from "vitest"; import { WebuiIsolatedChromiumProvider } from "../../src/runtime/browser/isolated-chromium/provider.js"; import { webuiBrowserPicturePoint } from "../../src/client/features/browser/application/browser-commands.js"; import type { WebuiBrowserImageFrame, WebuiBrowserTargetId } from "../../src/shared/contracts/browser.js"; +import { resolveChromeExecutable } from "./chrome-executable.js"; /** How long the page is given to tick. */ const WATCH_MS = 3_000; @@ -53,25 +54,7 @@ function buttonPageUrl(): string { return `data:text/html,${encodeURIComponent(html)}`; } -async function resolveChromePath(): Promise { - const { access } = await import("node:fs/promises"); - for (const candidate of [ - "/usr/bin/google-chrome", - "/usr/bin/google-chrome-stable", - "/usr/bin/chromium", - "/usr/bin/chromium-browser", - ]) { - try { - await access(candidate); - return candidate; - } catch { - // Keep looking. - } - } - return undefined; -} - -const available = (await resolveChromePath()) !== undefined; +const available = (await resolveChromeExecutable()) !== undefined; describe.skipIf(!available)("the isolated Chromium's screencast", () => { let dataDir: string; diff --git a/packages/webui/test/integration/isolated-chromium-two-pages.test.ts b/packages/webui/test/integration/isolated-chromium-two-pages.test.ts index a66376619..ad61db83a 100644 --- a/packages/webui/test/integration/isolated-chromium-two-pages.test.ts +++ b/packages/webui/test/integration/isolated-chromium-two-pages.test.ts @@ -50,6 +50,7 @@ import type { WebuiBrowserImageFrame, WebuiBrowserTargetId, } from "../../src/shared/contracts/browser.js"; +import { resolveChromeExecutable } from "./chrome-executable.js"; /** A page with a field and a title, so two pages can be told apart. */ function pageHtml(title: string): string { @@ -144,37 +145,7 @@ async function waitFor( ); } -async function chromeIsAvailable(): Promise { - return (await resolveChromePath()) !== undefined; -} - -async function resolveChromePath(): Promise { - const { access } = await import("node:fs/promises"); - const candidates = - process.platform === "darwin" - ? [ - "/Applications/Google Chrome.app/Contents/MacOS/Google Chrome", - "/Applications/Chromium.app/Contents/MacOS/Chromium", - "/Applications/Brave Browser.app/Contents/MacOS/Brave Browser", - ] - : [ - "/usr/bin/google-chrome", - "/usr/bin/google-chrome-stable", - "/usr/bin/chromium", - "/usr/bin/chromium-browser", - ]; - for (const candidate of candidates) { - try { - await access(candidate); - return candidate; - } catch { - // Keep looking. - } - } - return undefined; -} - -const available = await chromeIsAvailable(); +const available = (await resolveChromeExecutable()) !== undefined; describe.skipIf(!available)("the isolated Chromium, one page per web tab", () => { let dataDir: string; diff --git a/packages/webui/test/integration/workspace-browser-tab-acceptance.test.ts b/packages/webui/test/integration/workspace-browser-tab-acceptance.test.ts index 841302446..f9291fafd 100644 --- a/packages/webui/test/integration/workspace-browser-tab-acceptance.test.ts +++ b/packages/webui/test/integration/workspace-browser-tab-acceptance.test.ts @@ -16,8 +16,7 @@ // handler, the input path through the client's own binding, and the provider // itself. A case that only exercised a helper would not be the case. -import { execFileSync } from "node:child_process"; -import { access, mkdtemp, rm } from "node:fs/promises"; +import { mkdtemp, rm } from "node:fs/promises"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { afterAll, beforeAll, describe, expect, it } from "vitest"; @@ -46,6 +45,7 @@ import type { WebuiBrowserAuthorization, WebuiBrowserTargetId, } from "../../src/shared/contracts/browser.js"; +import { chromeExecutableVersion, resolveChromeExecutable } from "./chrome-executable.js"; /** This programme's records. They live in its own completion directory. */ const records = acceptanceRecordsFor("workspace-browser-tab"); @@ -66,19 +66,7 @@ const SETTLE_MS = 750; */ const TIMEOUT_MS = 30_000; -async function chromeIsAvailable(): Promise { - for (const candidate of ["/usr/bin/google-chrome", "/usr/bin/chromium"]) { - try { - await access(candidate); - return true; - } catch { - // Keep looking. - } - } - return false; -} - -const available = await chromeIsAvailable(); +const available = (await resolveChromeExecutable()) !== undefined; function pageHtml(title: string): string { return [ @@ -114,13 +102,8 @@ describe.skipIf(!available)("the acceptance of the workspace browser tab model", }; beforeAll(async () => { - try { - browserVersion = execFileSync("/usr/bin/google-chrome", ["--version"], { - encoding: "utf8", - }).trim(); - } catch { - browserVersion = "unknown"; - } + const chromePath = await resolveChromeExecutable(); + browserVersion = chromePath ? chromeExecutableVersion(chromePath) : "unknown"; dataDir = await mkdtemp(join(tmpdir(), "webui-tab-acceptance-")); service = new WebuiBrowserService({ authorization: { diff --git a/release/public-source.json b/release/public-source.json index 7b3f13456..7785e4fd7 100644 --- a/release/public-source.json +++ b/release/public-source.json @@ -4832,6 +4832,7 @@ "packages/webui/test/integration/browser-panel-live-navigation.test.ts", "packages/webui/test/integration/browser-use-real-input.test.ts", "packages/webui/test/integration/capability-surface-baseline.ts", + "packages/webui/test/integration/chrome-executable.ts", "packages/webui/test/integration/cron-desktop-and-webui.test.ts", "packages/webui/test/integration/isolated-chromium-profile.test.ts", "packages/webui/test/integration/isolated-chromium-screencast.test.ts", From c4b3347cd930bbc723b59aecc636915209a5a882 Mon Sep 17 00:00:00 2001 From: probe Date: Tue, 10 Nov 2026 23:53:27 +0800 Subject: [PATCH 2/5] fix(webui): let a browser startup failure say what went wrong MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The isolated Chromium's start-up was silent about everything that could go wrong, so a browser that never came up was indistinguishable from a browser that was slow: - Chrome's own stderr was discarded (stdio: ["ignore","ignore","ignore"]), so the browser's account of its own start-up reached nobody. - The start-up poll watched child.exitCode and never child.signalCode, so a browser killed by a signal spent the whole 30s budget and then reported "the browser did not publish a DevTools endpoint in time" — the cause was gone by then. - The CDP handshake was not guarded, so a failed handshake leaked the Chrome process; the leaked process kept the profile's single-instance lock and the next start exited at once, which looked like a slow start again. - A spawn failure was not listened for, so it surfaced as a process-level uncaught exception rather than as a browser error. Measured with a fake browser, before and after: killed by a signal 30042ms, "did not publish a DevTools endpoint in time" -> 105ms, "killed by signal SIGKILL during startup (exit code none) after 101ms (chrome: ...) Chrome said: FATAL: the browser was killed while starting" handshake fails the browser process is still running afterwards -> the browser process is gone The provider now keeps the browser's stderr, records the marks of a start-up (resolved path, spawn, DevTools endpoint, first target) together with the browser's own version, and asks for an executable rather than for a file that exists. The start-up diagnostics are written beside the other browser artifacts, so a CI run can carry them: $WEBUI_BROWSER_OUTPUT_DIR/chrome-startup.json $WEBUI_BROWSER_OUTPUT_DIR/chrome-stderr.log --- .../browser/isolated-chromium/provider.ts | 563 ++++++++++++++++-- ...lated-chromium-startup-diagnostics.test.ts | 257 ++++++++ release/public-source.json | 1 + test/vitest-suites.json | 1 + 4 files changed, 759 insertions(+), 63 deletions(-) create mode 100644 packages/webui/test/unit/isolated-chromium-startup-diagnostics.test.ts diff --git a/packages/webui/src/runtime/browser/isolated-chromium/provider.ts b/packages/webui/src/runtime/browser/isolated-chromium/provider.ts index c420458bc..030de645c 100644 --- a/packages/webui/src/runtime/browser/isolated-chromium/provider.ts +++ b/packages/webui/src/runtime/browser/isolated-chromium/provider.ts @@ -39,9 +39,10 @@ // frames as the page changes, which is what the panel needs; polling // `Page.captureScreenshot` would be a screenshot, not a live stream. -import { spawn, type ChildProcess } from "node:child_process"; -import { access, mkdir, readFile, rm } from "node:fs/promises"; -import { join } from "node:path"; +import { execFile, spawn, type ChildProcess } from "node:child_process"; +import { constants } from "node:fs"; +import { access, mkdir, readFile, rm, writeFile } from "node:fs/promises"; +import { dirname, join } from "node:path"; import { BrowserCore, @@ -71,6 +72,33 @@ import { WebuiIsolatedChromiumCoreTransport } from "./core-transport.js"; const STARTUP_TIMEOUT_MS = 30_000; /** How often the endpoint file is polled while the browser starts. */ const STARTUP_POLL_INTERVAL_MS = 100; +/** + * How much of Chrome's own error output is kept. + * + * The stream is unbounded and the end of it is the part that names the + * problem, so the tail is what is kept. + */ +const STDERR_CAPTURE_LIMIT_BYTES = 64 * 1024; +/** How much of that tail is quoted in an error message. */ +const STDERR_MESSAGE_CHARS = 400; +/** + * How long a failure waits for the browser's own error output. + * + * The words are on a stream that may still hold data when the process is + * already gone, so the failure waits for the stream to end before it quotes + * them — but not for ever. + */ +const STDERR_DRAIN_TIMEOUT_MS = 250; +/** How long the browser binary may take to answer `--version`. */ +const CHROME_VERSION_PROBE_TIMEOUT_MS = 5_000; +/** + * How long a failure waits for the version before it is reported. + * + * The version belongs in the account of the failure, but a browser that dies + * has to be reported at once: the probe is therefore given a moment and no + * more. + */ +const VERSION_PROBE_WAIT_MS = 1_000; /** How long a command is retried while the page is still being attached. */ const ATTACHMENT_RETRY_WINDOW_MS = 3_000; /** The delay between attachment retries. */ @@ -245,6 +273,23 @@ export class WebuiIsolatedChromiumProvider implements WebuiBrowserSurfaceProvide readonly #core: BrowserCore; #child: ChildProcess | undefined; #connection: WebuiBrowserCdpConnection | undefined; + /** + * The facts of this startup. + * + * A launch that fails says nothing about itself: the process goes away, the + * port file never appears, and the only words the caller gets are "it did + * not start". These are the facts that make such a failure explainable. + */ + readonly #trace = new ChromeStartupTrace(); + /** + * Chrome's own error output. + * + * It is kept rather than printed: the runtime's browser paths keep no logs, + * and this text belongs in a file that says what happened. + */ + readonly #stderr = new StderrTail(); + /** Where the diagnostics of this provider are written. */ + readonly #outputDir: string; /** * The pages this provider drives, by target id. * @@ -274,6 +319,7 @@ export class WebuiIsolatedChromiumProvider implements WebuiBrowserSurfaceProvide private constructor(options: WebuiIsolatedChromiumProviderOptions) { this.#options = options; this.#userDataDir = join(options.dataDir, "browser", "isolated-chromium", "profile"); + this.#outputDir = browserDiagnosticsDir(this.#userDataDir); this.browserId = `isolated-chromium-${randomSuffix()}`; this.#core = new BrowserCore({ provider: "webui-isolated-chromium", @@ -299,61 +345,173 @@ export class WebuiIsolatedChromiumProvider implements WebuiBrowserSurfaceProvide } async #start(): Promise { - const chromePath = await resolveChromePath(this.#options.chromePath); - await mkdir(this.#userDataDir, { recursive: true, mode: 0o700 }); - const activePortPath = join(this.#userDataDir, "DevToolsActivePort"); - await rm(activePortPath, { force: true }); - - const viewport = this.#options.viewport ?? { width: 1280, height: 800 }; - const child = spawn( - chromePath, - [ - "--headless=new", - "--remote-debugging-port=0", - "--remote-debugging-address=127.0.0.1", - `--user-data-dir=${this.#userDataDir}`, - "--no-first-run", - "--no-default-browser-check", - "--disable-background-networking", - "--disable-component-update", - "--disable-sync", - "--disable-features=Translate,MediaRouter", - `--window-size=${viewport.width},${viewport.height}`, - "about:blank", - ], - { stdio: ["ignore", "ignore", "ignore"] }, - ); - this.#child = child; - - let endpoint: string; + const trace = this.#trace; + const stderr = this.#stderr; + let child: ChildProcess | undefined; + let versionProbe: Promise | undefined; try { - endpoint = await waitForDevToolsEndpoint(child, activePortPath); + const chromePath = await resolveChromePath(this.#options.chromePath); + trace.setChromePath(chromePath); + trace.mark("chromePathResolved"); + // The version is asked for beside the launch rather than before it: a + // browser that dies has to be reported at once, and a probe that hangs + // must not stand between the failure and the report of it. + versionProbe = probeChromeVersion(chromePath).then((version) => { + trace.setChromeVersion(version); + trace.mark("chromeVersionProbed", { version: version ?? null }); + return version; + }); + + await mkdir(this.#userDataDir, { recursive: true, mode: 0o700 }); + const activePortPath = join(this.#userDataDir, "DevToolsActivePort"); + await rm(activePortPath, { force: true }); + + const viewport = this.#options.viewport ?? { width: 1280, height: 800 }; + child = spawn( + chromePath, + [ + "--headless=new", + "--remote-debugging-port=0", + "--remote-debugging-address=127.0.0.1", + `--user-data-dir=${this.#userDataDir}`, + "--no-first-run", + "--no-default-browser-check", + "--disable-background-networking", + "--disable-component-update", + "--disable-sync", + "--disable-features=Translate,MediaRouter", + `--window-size=${viewport.width},${viewport.height}`, + "about:blank", + ], + // The browser's own error output is the only thing that says *why* it + // did not start, so it is piped and kept rather than dropped. + { stdio: ["ignore", "ignore", "pipe"] }, + ); + this.#child = child; + trace.mark("spawned", { pid: child.pid ?? null }); + child.stderr?.on("data", (chunk: Buffer) => stderr.push(chunk)); + // The browser's words arrive on a stream that may still hold data when + // the process is already gone, so a failure waits for the stream to end + // before it quotes them. + const stderrEnded = new Promise((resolve) => { + const stream = child?.stderr; + if (!stream) { + resolve(); + return; + } + stream.once("end", () => resolve()); + stream.once("close", () => resolve()); + stream.once("error", () => resolve()); + }); + const drainStderr = async (): Promise => { + await Promise.race([stderrEnded, delay(STDERR_DRAIN_TIMEOUT_MS)]); + }; + + // A process that cannot be started says so on the `error` event, and an + // event nobody listens for is an uncaught exception that takes the + // process with it. The failure is therefore recorded and turned into a + // browser error like any other. + let spawnError: Error | undefined; + child.on("error", (error: Error) => { + spawnError = error; + trace.mark("spawnError", { message: error.message }); + }); + child.on("exit", (code, signal) => { + trace.mark("exited", { code, signal: signal ?? null }); + }); + + let endpoint: string; + try { + endpoint = await waitForDevToolsEndpoint(child, activePortPath, { + chromePath, + stderr: () => stderr.text(), + spawnError: () => spawnError, + awaitStderr: drainStderr, + onEndpoint: () => trace.mark("devToolsActivePortRead"), + }); + } catch (error) { + child.kill("SIGKILL"); + throw error; + } + + let connection: WebuiBrowserCdpConnection; + try { + connection = await WebuiBrowserCdpConnection.connect(endpoint); + } catch (error) { + // A handshake that fails leaves a browser process that nothing owns. + // That process keeps the profile's single-instance lock, so the next + // launch would fail on the lock rather than on its own problem. + child.kill("SIGKILL"); + throw error; + } + this.#connection = connection; + this.#unsubscribe = connection.onEvent((event) => this.#onEvent(event)); + + const created = await connection.send<{ targetId: string }>("Target.createTarget", { + url: "about:blank", + }); + trace.mark("targetCreated", { targetId: created.targetId }); + this.#launchTargetId = created.targetId; + await this.#attachPage(created.targetId); + + // The panel must also be told about pages created by links using + // `target="_blank"` or `window.open`. Discover only after the launch page + // is attached, then ignore the initial inventory Chromium reports as part + // of enabling discovery. + const initialTargets = await connection.send<{ + targetInfos?: readonly { targetId: string }[]; + }>("Target.getTargets"); + for (const target of initialTargets.targetInfos ?? []) { + this.#targetsBeforeDiscovery.add(target.targetId); + } + await connection.send("Target.setDiscoverTargets", { discover: true }); + trace.mark("started"); } catch (error) { - child.kill("SIGKILL"); + // A failed launch leaves a browser process behind unless it is stopped + // here, and the process would keep the profile locked for the next one. + child?.kill("SIGKILL"); + if (versionProbe) { + await Promise.race([versionProbe, delay(VERSION_PROBE_WAIT_MS)]); + } + trace.setFailure(error); + await this.#flushDiagnostics(); throw error; } + await versionProbe; + await this.#flushDiagnostics(); + } - const connection = await WebuiBrowserCdpConnection.connect(endpoint); - this.#connection = connection; - this.#unsubscribe = connection.onEvent((event) => this.#onEvent(event)); - - const created = await connection.send<{ targetId: string }>("Target.createTarget", { - url: "about:blank", + /** + * Writes what this startup learned to disk. + * + * The runtime's browser paths keep no logs, so these files are the only + * place the facts exist. A file that cannot be written is not a browser + * failure: the person is waiting for a browser, not for a diagnostics file. + */ + async #flushDiagnostics(): Promise { + await writeBrowserDiagnostics({ + outputDir: this.#outputDir, + userDataDir: this.#userDataDir, + artifact: this.#artifact(), + stderr: this.#stderr.text(), }); - this.#launchTargetId = created.targetId; - await this.#attachPage(created.targetId); - - // The panel must also be told about pages created by links using - // `target="_blank"` or `window.open`. Discover only after the launch page - // is attached, then ignore the initial inventory Chromium reports as part - // of enabling discovery. - const initialTargets = await connection.send<{ - targetInfos?: readonly { targetId: string }[]; - }>("Target.getTargets"); - for (const target of initialTargets.targetInfos ?? []) { - this.#targetsBeforeDiscovery.add(target.targetId); - } - await connection.send("Target.setDiscoverTargets", { discover: true }); + } + + /** The structured account of this startup. */ + #artifact(): Record { + return { + kind: "isolated-chromium-startup", + browserId: this.browserId, + generatedAt: new Date().toISOString(), + userDataDir: this.#userDataDir, + outputDir: this.#outputDir, + chromePath: this.#trace.chromePath(), + chromeVersion: this.#trace.chromeVersion() ?? null, + elapsedMs: this.#trace.elapsedMs(), + marks: this.#trace.marks(), + failure: this.#trace.failure(), + stderrTail: summarizeStderr(this.#stderr.text()), + }; } /** @@ -897,6 +1055,9 @@ export class WebuiIsolatedChromiumProvider implements WebuiBrowserSurfaceProvide this.#connection?.close(); this.#connection = undefined; await this.#stopBrowser(); + // Chrome's own words about the run are written when the browser is + // stopped, so the file holds everything the browser said before it went. + await this.#flushDiagnostics(); // The profile is **kept**. It is the browser's state — the sessions the // person signed into — and it belongs to this host's data directory like // any other state the host keeps. Deleting it on every close meant that a @@ -1172,12 +1333,19 @@ function randomSuffix(): string { * The platform list is the same one the repository's own headless transport * uses, so the isolated browser and the runtime's browser behave the same on * a given machine. + * + * A candidate has to be *executable*, not merely present: a file that exists + * but cannot be run is a browser that fails to start, and finding it here + * turns that into one clear refusal instead of a launch that dies silently. */ -async function resolveChromePath(explicit?: string): Promise { +export async function resolveChromePath( + explicit?: string, + candidates: readonly string[] = defaultChromeCandidates(), +): Promise { if (explicit?.trim()) return explicit.trim(); - for (const candidate of defaultChromeCandidates()) { + for (const candidate of candidates) { try { - await access(candidate); + await access(candidate, constants.X_OK); return candidate; } catch { // Keep looking. @@ -1189,7 +1357,211 @@ async function resolveChromePath(explicit?: string): Promise { ); } -function defaultChromeCandidates(): readonly string[] { +/** + * Where the startup diagnostics are written. + * + * The environment variable comes first so that a CI run can collect the + * files it produced; without it the facts stay with the profile that made + * them. + */ +export function browserDiagnosticsDir(userDataDir: string): string { + const configured = process.env.WEBUI_BROWSER_OUTPUT_DIR?.trim(); + if (configured) return configured; + return join(userDataDir, "browser-diagnostics"); +} + +/** + * Writes the account of a startup and the browser's own error output. + * + * The browser's output goes to two places: the directory the run collects + * from, and the profile directory beside the browser that produced it. The + * names are fixed, so a run that starts twice leaves two files rather than a + * growing pile. + */ +export async function writeBrowserDiagnostics(options: { + readonly outputDir: string; + readonly userDataDir: string; + readonly artifact: Record; + readonly stderr: string; +}): Promise { + const { outputDir, userDataDir, artifact, stderr } = options; + await writeTextFile( + join(outputDir, "chrome-startup.json"), + `${JSON.stringify(artifact, null, 2)}\n`, + ); + await writeTextFile(join(outputDir, "chrome-stderr.log"), stderr); + await writeTextFile(join(userDataDir, "chrome-stderr.log"), stderr); +} + +async function writeTextFile(path: string, content: string): Promise { + try { + await mkdir(dirname(path), { recursive: true }); + await writeFile(path, content, "utf8"); + } catch { + // A diagnostics file that cannot be written must not fail the browser. + } +} + +/** + * Asks the browser binary for its version. + * + * The version is one of the few facts that can be read without starting the + * browser, and it is the fact that says *which* Chrome failed — two builds + * fail differently. A probe that fails says nothing and is not itself a + * browser failure. + */ +async function probeChromeVersion(chromePath: string): Promise { + return await new Promise((resolve) => { + let settled = false; + const finish = (value: string | undefined): void => { + if (settled) return; + settled = true; + resolve(value); + }; + try { + const child = execFile( + chromePath, + ["--version"], + { + timeout: CHROME_VERSION_PROBE_TIMEOUT_MS, + killSignal: "SIGKILL", + windowsHide: true, + maxBuffer: STDERR_CAPTURE_LIMIT_BYTES, + }, + (error, stdout) => { + if (error) { + finish(undefined); + return; + } + const text = typeof stdout === "string" ? stdout.trim() : ""; + finish(text.length > 0 ? text : undefined); + }, + ); + child.on("error", () => finish(undefined)); + } catch { + finish(undefined); + } + }); +} + +/** + * Flattens the browser's error output into one quotable line. + * + * The tail of the output is kept and the rest is dropped: what a browser + * says last is what names the problem, and an error message that carried a + * whole log would be unreadable. + */ +function summarizeStderr(text: string | undefined): string { + if (!text) return ""; + const flattened = text.replace(/\s+/gu, " ").trim(); + if (!flattened) return ""; + if (flattened.length <= STDERR_MESSAGE_CHARS) return flattened; + return `...${flattened.slice(-STDERR_MESSAGE_CHARS)}`; +} + +/** The browser's own words, quoted after a failure. */ +function stderrClause(text: string | undefined): string { + const summary = summarizeStderr(text); + return summary.length > 0 ? ` Chrome said: ${summary}` : ""; +} + +/** + * The tail of the browser's error output. + * + * Chrome says why it failed on its error stream and nowhere else, and the + * stream is unbounded, so only the end of it is kept. + */ +class StderrTail { + readonly #limit: number; + #chunks: Buffer[] = []; + #bytes = 0; + + constructor(limit: number = STDERR_CAPTURE_LIMIT_BYTES) { + this.#limit = limit; + } + + push(chunk: Buffer | string): void { + const buffer = typeof chunk === "string" ? Buffer.from(chunk) : chunk; + this.#chunks.push(buffer); + this.#bytes += buffer.length; + while (this.#chunks.length > 1 && this.#bytes > this.#limit) { + this.#bytes -= this.#chunks[0].length; + this.#chunks.shift(); + } + const head = this.#chunks[0]; + if (head && head.length > this.#limit) { + this.#chunks[0] = head.subarray(head.length - this.#limit); + this.#bytes = this.#chunks.reduce((sum, chunk) => sum + chunk.length, 0); + } + } + + text(): string { + if (this.#chunks.length === 0) return ""; + return Buffer.concat(this.#chunks).toString("utf8"); + } +} + +/** + * The facts of one startup, collected as the browser starts. + * + * The marks are the moments a launch can be read at — the binary that was + * chosen, the process that was started, the endpoint that was published, the + * page that was created — and the elapsed time between them is what says + * where a launch went wrong. + */ +class ChromeStartupTrace { + readonly #startedAt = Date.now(); + readonly #marks: { readonly phase: string; readonly atMs: number; readonly detail?: Record }[] = []; + #chromePath: string | undefined; + #chromeVersion: string | undefined; + #failure: { readonly code: string; readonly message: string } | undefined; + + setChromePath(value: string): void { + this.#chromePath = value; + } + + setChromeVersion(value: string | undefined): void { + this.#chromeVersion = value; + } + + mark(phase: string, detail?: Record): void { + this.#marks.push({ + phase, + atMs: Date.now() - this.#startedAt, + ...(detail ? { detail } : {}), + }); + } + + setFailure(error: unknown): void { + const failure = error as { code?: unknown; message?: unknown } | null; + this.#failure = { + code: typeof failure?.code === "string" ? failure.code : "unknown", + message: typeof failure?.message === "string" ? failure.message : String(error), + }; + } + + chromePath(): string | undefined { + return this.#chromePath; + } + + chromeVersion(): string | undefined { + return this.#chromeVersion; + } + + elapsedMs(): number { + return Date.now() - this.#startedAt; + } + + marks(): readonly { phase: string; atMs: number; detail?: Record }[] { + return this.#marks; + } + + failure(): { code: string; message: string } | undefined { + return this.#failure; + } +} + +export function defaultChromeCandidates(): readonly string[] { if (process.platform === "darwin") { return [ "/Applications/Google Chrome.app/Contents/MacOS/Google Chrome", @@ -1216,6 +1588,20 @@ function defaultChromeCandidates(): readonly string[] { ]; } +/** + * What the wait needs to know about the browser process. + * + * A child process satisfies this, and so does anything that names the way a + * process died: the wait's decisions are about the state of the process, not + * about anything a process can do. + */ +export interface DevToolsEndpointChild { + /** The code the process exited with, or null while it still runs. */ + readonly exitCode: number | null; + /** The signal that killed the process, or null while it still runs. */ + readonly signalCode: NodeJS.Signals | null; +} + /** * Waits for the browser to publish its DevTools endpoint. * @@ -1223,35 +1609,86 @@ function defaultChromeCandidates(): readonly string[] { * rather than scraped from its output: the file is the browser's own * statement of where it is listening, and it avoids mistaking a log line for * an address. + * + * The wait ends early when the browser is already gone. A browser that was + * killed leaves a signal rather than an exit code, and a browser that cannot + * be started reports an error on the process itself; in both cases the wait + * would otherwise spend the whole timeout on a death that has already + * happened and then report a timeout that explains nothing. */ -async function waitForDevToolsEndpoint( - child: ChildProcess, +export async function waitForDevToolsEndpoint( + child: DevToolsEndpointChild, activePortPath: string, - timeoutMs = STARTUP_TIMEOUT_MS, + options: { + /** The binary that was started, for the failure message. */ + readonly chromePath?: string; + /** How long the browser may take to publish its endpoint. */ + readonly timeoutMs?: number; + /** The browser's own error output, quoted in the failure message. */ + readonly stderr?: () => string | undefined; + /** The failure of the process itself, when it could not be started. */ + readonly spawnError?: () => Error | undefined; + /** Waits for the browser's own error output to arrive. */ + readonly awaitStderr?: () => Promise; + /** Called with the endpoint as soon as it has been read. */ + readonly onEndpoint?: (endpoint: string) => void; + } = {}, ): Promise { - const deadline = Date.now() + timeoutMs; + const timeoutMs = options.timeoutMs ?? STARTUP_TIMEOUT_MS; + const startedAt = Date.now(); + const deadline = startedAt + timeoutMs; while (Date.now() < deadline) { + const spawnError = options.spawnError?.(); + if (spawnError) { + await options.awaitStderr?.(); + throw browserOperationError( + "browser_provider_failed", + `The browser could not be started: ${spawnError.message}.${stderrClause(options.stderr?.())}`, + ); + } + if (child.signalCode !== null) { + await options.awaitStderr?.(); + throw browserOperationError( + "browser_provider_failed", + `The browser was killed by signal ${child.signalCode} during startup (exit code ${child.exitCode ?? "none"}) after ${Date.now() - startedAt}ms (chrome: ${options.chromePath ?? "unknown"}).${stderrClause(options.stderr?.())}`, + ); + } if (child.exitCode !== null) { + await options.awaitStderr?.(); throw browserOperationError( "browser_provider_failed", - `The browser exited during startup with code ${child.exitCode}.`, + `The browser exited during startup with code ${child.exitCode} after ${Date.now() - startedAt}ms (chrome: ${options.chromePath ?? "unknown"}).${stderrClause(options.stderr?.())}`, ); } try { const endpoint = parseDevToolsActivePort(await readFile(activePortPath, "utf8")); - if (endpoint) return endpoint; + if (endpoint) { + options.onEndpoint?.(endpoint); + return endpoint; + } } catch { // The browser has not written the file yet. } await delay(STARTUP_POLL_INTERVAL_MS); } + await options.awaitStderr?.(); + const activePortPresent = await fileExists(activePortPath); throw browserOperationError( "browser_operation_timeout", - "The browser did not publish a DevTools endpoint in time.", + `The browser did not publish a DevTools endpoint in time (after ${Date.now() - startedAt}ms; chrome: ${options.chromePath ?? "unknown"}; DevToolsActivePort file: ${activePortPresent ? "present" : "absent"}; exit code: ${child.exitCode ?? "none"}; signal: ${child.signalCode ?? "none"}).${stderrClause(options.stderr?.())}`, true, ); } +async function fileExists(path: string): Promise { + try { + await access(path); + return true; + } catch { + return false; + } +} + function parseDevToolsActivePort(raw: string): string | undefined { const [port, path] = raw.trim().split("\n"); if (!port || !path) return undefined; diff --git a/packages/webui/test/unit/isolated-chromium-startup-diagnostics.test.ts b/packages/webui/test/unit/isolated-chromium-startup-diagnostics.test.ts new file mode 100644 index 000000000..e5f5822d3 --- /dev/null +++ b/packages/webui/test/unit/isolated-chromium-startup-diagnostics.test.ts @@ -0,0 +1,257 @@ +import { chmod, mkdtemp, readFile, rm, writeFile } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; + +import { afterEach, describe, expect, it } from "vitest"; + +import { WebuiBrowserOperationError } from "../../src/runtime/browser/errors.js"; +import { + WebuiIsolatedChromiumProvider, + browserDiagnosticsDir, + resolveChromePath, + waitForDevToolsEndpoint, + writeBrowserDiagnostics, +} from "../../src/runtime/browser/isolated-chromium/provider.js"; + +/** + * A launch that fails used to be silent. + * + * The browser's own error output was dropped, a browser that was killed was + * not noticed, and a browser that could not be started took the process with + * it. The facts below are what the browser said about itself: the message a + * failure carries has to name the browser's own words, and the files it + * leaves behind have to hold them. + * + * The browser here is Node itself: given Chrome's flags it refuses them and + * says so on its error stream, which is exactly the shape of a browser that + * dies during startup — and it behaves the same on every platform. + */ +const FAKE_BROWSER = process.execPath; + +/** The temporary directories this file made, so that they can be removed. */ +const temporaryDirectories: string[] = []; + +afterEach(async () => { + delete process.env.WEBUI_BROWSER_OUTPUT_DIR; + while (temporaryDirectories.length > 0) { + const directory = temporaryDirectories.pop(); + if (directory) await rm(directory, { recursive: true, force: true }); + } +}); + +async function temporaryDirectory(prefix: string): Promise { + const directory = await mkdtemp(join(tmpdir(), prefix)); + temporaryDirectories.push(directory); + return directory; +} + +describe("isolated Chromium startup diagnostics", () => { + it("reports a browser that was killed by a signal at once", async () => { + const startedAt = Date.now(); + const error = await waitForDevToolsEndpoint( + { exitCode: null, signalCode: "SIGKILL" }, + join(tmpdir(), "webui-never-written-DevToolsActivePort"), + { + timeoutMs: 30_000, + chromePath: "/opt/chrome/chrome", + stderr: () => "FATAL: the profile is locked by another Chrome", + }, + ).catch((failure: unknown) => failure); + + expect(error).toBeInstanceOf(WebuiBrowserOperationError); + expect((error as WebuiBrowserOperationError).code).toBe("browser_provider_failed"); + const message = (error as Error).message; + expect(message).toContain("SIGKILL"); + expect(message).toContain("exit code none"); + expect(message).toMatch(/after \d+ms/u); + // The browser's own words are quoted, not a bare timeout. + expect(message).toContain("FATAL: the profile is locked by another Chrome"); + // The signal is noticed straight away rather than after the whole + // thirty-second startup budget. + expect(Date.now() - startedAt).toBeLessThan(5_000); + }); + + it("reports a timeout with the facts of the launch", async () => { + const error = await waitForDevToolsEndpoint( + { exitCode: null, signalCode: null }, + join(tmpdir(), "webui-absent-DevToolsActivePort"), + { + timeoutMs: 200, + chromePath: "/opt/chrome/chrome", + stderr: () => "FATAL: profile lock held", + }, + ).catch((failure: unknown) => failure); + + expect(error).toBeInstanceOf(WebuiBrowserOperationError); + expect((error as WebuiBrowserOperationError).code).toBe("browser_operation_timeout"); + const message = (error as Error).message; + expect(message).toContain("/opt/chrome/chrome"); + expect(message).toContain("DevToolsActivePort file: absent"); + expect(message).toContain("exit code: none"); + expect(message).toContain("signal: none"); + expect(message).toMatch(/after \d+ms/u); + expect(message).toContain("FATAL: profile lock held"); + }); + + it("reads the endpoint the browser published", async () => { + const directory = await temporaryDirectory("webui-chrome-endpoint-"); + const activePortPath = join(directory, "DevToolsActivePort"); + await writeFile(activePortPath, "43123\n/devtools/browser/8f1c-42\n"); + const seen: string[] = []; + + const endpoint = await waitForDevToolsEndpoint( + { exitCode: null, signalCode: null }, + activePortPath, + { + chromePath: "/opt/chrome/chrome", + onEndpoint: (value: string) => seen.push(value), + }, + ); + + expect(endpoint).toBe("ws://127.0.0.1:43123/devtools/browser/8f1c-42"); + expect(seen).toEqual(["ws://127.0.0.1:43123/devtools/browser/8f1c-42"]); + }); + + it("reports a browser that could not be started", async () => { + const error = await waitForDevToolsEndpoint( + { exitCode: null, signalCode: null }, + join(tmpdir(), "webui-never-written-DevToolsActivePort"), + { + timeoutMs: 30_000, + chromePath: "/opt/chrome/chrome", + spawnError: () => new Error("spawn /opt/chrome/chrome ENOENT"), + }, + ).catch((failure: unknown) => failure); + + expect(error).toBeInstanceOf(WebuiBrowserOperationError); + expect((error as WebuiBrowserOperationError).code).toBe("browser_provider_failed"); + expect((error as Error).message).toContain("spawn /opt/chrome/chrome ENOENT"); + }); + + it("keeps the quoted browser output short", async () => { + const error = await waitForDevToolsEndpoint( + { exitCode: 7, signalCode: null }, + join(tmpdir(), "webui-never-written-DevToolsActivePort"), + { chromePath: "/opt/chrome/chrome", stderr: () => "x".repeat(5_000) }, + ).catch((failure: unknown) => failure); + + const message = (error as Error).message; + expect(message.length).toBeLessThan(800); + expect(message).toContain("..."); + }); + + it("skips a candidate that cannot be executed", async () => { + const directory = await temporaryDirectory("webui-chrome-candidates-"); + const notExecutable = join(directory, "chrome-without-the-executable-bit"); + const executable = join(directory, "chrome"); + await writeFile(notExecutable, "#!/bin/sh\nexit 0\n"); + await writeFile(executable, "#!/bin/sh\nexit 0\n"); + await chmod(executable, 0o755); + await chmod(notExecutable, 0o644); + + if (process.platform === "win32") { + // Windows has no executable bit, so `X_OK` behaves as `F_OK` there and + // the first candidate that exists is the one that is used. + expect(await resolveChromePath(undefined, [notExecutable, executable])).toBe(notExecutable); + return; + } + + expect(await resolveChromePath(undefined, [notExecutable, executable])).toBe(executable); + await expect(resolveChromePath(undefined, [notExecutable])).rejects.toMatchObject({ + code: "browser_provider_unavailable", + }); + }); + + it("quotes the browser's own output when the browser dies during startup", async () => { + const dataDir = await temporaryDirectory("webui-chrome-launch-"); + const outputDir = join(dataDir, "results"); + process.env.WEBUI_BROWSER_OUTPUT_DIR = outputDir; + + const startedAt = Date.now(); + const error = await WebuiIsolatedChromiumProvider.launch({ + dataDir, + chromePath: FAKE_BROWSER, + }).catch((failure: unknown) => failure); + + expect(error).toBeInstanceOf(WebuiBrowserOperationError); + expect((error as WebuiBrowserOperationError).code).toBe("browser_provider_failed"); + const message = (error as Error).message; + // The browser's own words, not a bare timeout. + expect(message).toContain("bad option"); + expect(message).toContain(FAKE_BROWSER); + expect(message).toMatch(/after \d+ms/u); + // A browser that dies is noticed at once rather than after the whole + // thirty-second startup budget. + expect(Date.now() - startedAt).toBeLessThan(20_000); + + const artifact = JSON.parse( + await readFile(join(outputDir, "chrome-startup.json"), "utf8"), + ) as { + chromePath?: string; + chromeVersion?: string; + failure?: { code?: string; message?: string }; + stderrTail?: string; + marks?: { phase: string }[]; + }; + expect(artifact.chromePath).toBe(FAKE_BROWSER); + expect(artifact.chromeVersion).toBe(process.version); + expect(artifact.failure?.code).toBe("browser_provider_failed"); + expect(artifact.failure?.message).toContain("bad option"); + expect(artifact.stderrTail).toContain("bad option"); + const phases = (artifact.marks ?? []).map((mark) => mark.phase); + expect(phases).toContain("chromePathResolved"); + expect(phases).toContain("spawned"); + + const stderrLog = await readFile(join(outputDir, "chrome-stderr.log"), "utf8"); + expect(stderrLog).toContain("bad option"); + const profileLog = await readFile( + join(dataDir, "browser", "isolated-chromium", "profile", "chrome-stderr.log"), + "utf8", + ); + expect(profileLog).toContain("bad option"); + }); + + it("turns a browser that cannot be started into a browser error", async () => { + const dataDir = await temporaryDirectory("webui-chrome-missing-"); + process.env.WEBUI_BROWSER_OUTPUT_DIR = join(dataDir, "results"); + + const startedAt = Date.now(); + const error = await WebuiIsolatedChromiumProvider.launch({ + dataDir, + chromePath: join(dataDir, "no-such-chrome"), + }).catch((failure: unknown) => failure); + + expect(error).toBeInstanceOf(WebuiBrowserOperationError); + expect((error as WebuiBrowserOperationError).code).toBe("browser_provider_failed"); + expect((error as Error).message).toContain("no-such-chrome"); + expect((error as Error).message).toContain("ENOENT"); + expect(Date.now() - startedAt).toBeLessThan(20_000); + }); + + it("writes the diagnostics where the run collects them", async () => { + const directory = await temporaryDirectory("webui-chrome-diagnostics-"); + const userDataDir = join(directory, "profile"); + const outputDir = join(directory, "results"); + + expect(browserDiagnosticsDir(userDataDir)).toBe(join(userDataDir, "browser-diagnostics")); + process.env.WEBUI_BROWSER_OUTPUT_DIR = outputDir; + expect(browserDiagnosticsDir(userDataDir)).toBe(outputDir); + + await writeBrowserDiagnostics({ + outputDir, + userDataDir, + artifact: { kind: "isolated-chromium-startup" }, + stderr: "FATAL: profile lock held", + }); + + expect( + JSON.parse(await readFile(join(outputDir, "chrome-startup.json"), "utf8")), + ).toEqual({ kind: "isolated-chromium-startup" }); + expect(await readFile(join(outputDir, "chrome-stderr.log"), "utf8")).toBe( + "FATAL: profile lock held", + ); + expect(await readFile(join(userDataDir, "chrome-stderr.log"), "utf8")).toBe( + "FATAL: profile lock held", + ); + }); +}); diff --git a/release/public-source.json b/release/public-source.json index 7785e4fd7..2129bfcd0 100644 --- a/release/public-source.json +++ b/release/public-source.json @@ -4930,6 +4930,7 @@ "packages/webui/test/unit/home-adoption-transaction.test.ts", "packages/webui/test/unit/interaction-coordinator.test.ts", "packages/webui/test/unit/interaction-session-owners.test.ts", + "packages/webui/test/unit/isolated-chromium-startup-diagnostics.test.ts", "packages/webui/test/unit/markdown-autolink-double-anchor.test.tsx", "packages/webui/test/unit/markdown-codeblock-c1-highlight.test.tsx", "packages/webui/test/unit/markdown-codeblock-c2-line-numbers.test.tsx", diff --git a/test/vitest-suites.json b/test/vitest-suites.json index c0c437f5c..d0e80b2dc 100644 --- a/test/vitest-suites.json +++ b/test/vitest-suites.json @@ -560,6 +560,7 @@ "packages/webui/test/unit/browser-web-tabs-operation.test.ts", "packages/webui/test/unit/browser-web-tab-toolbar.test.tsx", "packages/webui/test/unit/browser-frame-stream.test.ts", + "packages/webui/test/unit/isolated-chromium-startup-diagnostics.test.ts", "packages/webui/test/unit/browser-session-release.test.ts", "packages/webui/test/unit/browser-bridge-cdp.test.ts", "packages/webui/test/unit/browser-bridge-adversarial.test.ts", From c7ea2ccb44942b0f32d52cd8e25a88e4e452ee3f Mon Sep 17 00:00:00 2001 From: probe Date: Tue, 10 Nov 2026 23:53:27 +0800 Subject: [PATCH 3/5] fix(webui): give a browser frame stream a deadline and a record MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A frame stream that never produced a frame never ended either: the wait for the first picture had no deadline, so the service held the subscription open forever and the panel held an empty placeholder forever. Nothing said which of the two it was. The stream now gives up on its first frame after 60s and fails with browser_operation_timeout, which the server already puts on the wire as an error frame. The budget is the CDP command budget, and the reason is the repository's own record: on a loaded runner the browser answers a page open "in about half a minute", and the old fifteen-second budget turned that slowness into a failure. 30s would sit on the edge of the measured common case — 19-21s for a cold boot on the macOS runner, 5.3s on ubuntu — and 90s would outlast every other operation on the path. The clock stops at the first frame: a running stream is not a stuck stream. The service also records what it saw of the browser session: how many eager starts were attempted and how many failed, how long every start took, how long the first frame took, and how many frames were dropped. The eager start's failure used to be swallowed by an empty catch, so the next call started the browser again from scratch with no trace that it had already failed. The record is written beside the other browser artifacts: $WEBUI_BROWSER_OUTPUT_DIR/service-browser.json --- .../src/runtime/browser/browser-service.ts | 430 +++++++++++++++++- .../webui/test/unit/browser-service.test.ts | 212 ++++++++- 2 files changed, 629 insertions(+), 13 deletions(-) diff --git a/packages/webui/src/runtime/browser/browser-service.ts b/packages/webui/src/runtime/browser/browser-service.ts index 5aaaab58a..ce2da17a5 100644 --- a/packages/webui/src/runtime/browser/browser-service.ts +++ b/packages/webui/src/runtime/browser/browser-service.ts @@ -33,6 +33,9 @@ // A surface that is not authorized is refused before anything is attempted, so // "you did not switch this on" is distinguishable from "the browser broke". +import { mkdir, writeFile } from "node:fs/promises"; +import { join } from "node:path"; + import { isSameWebuiBrowserTarget, WEBUI_BROWSER_RESTRICTED_COMMANDS, @@ -74,6 +77,105 @@ import { */ const WEBUI_BROWSER_FRAME_QUEUE_LIMIT = 32; +/** + * How long a stream waits for its **first** frame. + * + * The budget is a promise to the person waiting for the picture, not an + * internal detail: a panel that asked for a frame and gets none must be + * told, because a stream that never ends and a stream that is merely slow + * look identical from the panel. It has to be generous enough for a browser + * that is *busy* rather than broken, because from out here the two are the + * same thing. + * + * The number is the CDP command budget + * (`WEBUI_BROWSER_CDP_COMMAND_TIMEOUT_MS`, 60 s) because the first frame is + * the product of the same work a command waits for: the browser attaches to + * the page, paints it and starts the screencast. A budget shorter than a + * single command's would abandon work the browser is still legitimately + * doing — which is what the old fifteen-second budget did. Its own note + * says it: on a loaded runner "the browser answers a page open in about + * half a minute", the fifteen-second budget "turned that slowness into a + * failure: the open was abandoned halfway, so the panel never received a + * frame and the person saw an empty browser instead of a slow one". + * + * The measurement agrees. On the macOS CI runner the browser path costs + * 19–22 s where the same case costs 5.3 s on Linux (the two-pages + * integration file), and the agent-transcript cases that open the browser + * panel run 58–81 s; one run never reached a first frame within 60 s at + * all. A fifteen-second budget calls the common twenty seconds a failure. + * Sixty seconds covers the common case with room to spare and still ends + * the stream, so a browser that is genuinely wedged is reported rather than + * waited on. + * + * The two budgets are sequential rather than competing. The provider's + * start-up budget (30 s) covers Chrome booting until it publishes a + * DevTools endpoint; this clock starts after the provider exists and the + * screencast has been asked for, so the worst case from "switch on" to + * "picture or error" is the sum of the two, each bounded and each reported + * with its own code. + * + * The clock stops at the first frame. A stream that is running is not a + * stream that is stuck, however long it runs. + */ +const WEBUI_BROWSER_FIRST_FRAME_TIMEOUT_MS = 60_000; + +/** + * Where the browser diagnostics are written. + * + * One directory, so the service's own record of a browser session and the + * provider's record of the same session sit beside each other: the host's + * `WEBUI_BROWSER_OUTPUT_DIR` when it names one, and otherwise a + * directory under the data dir the browser keeps its profile in. + */ +const WEBUI_BROWSER_DIAGNOSTICS_DIRECTORY = "browser-diagnostics"; + +/** + * The file this service writes its own diagnostics to. + * + * The name is fixed. The artifact is the one file a reader looks for, and a + * name with a random suffix would be a file nobody finds. + */ +const WEBUI_BROWSER_SERVICE_DIAGNOSTICS_FILE = "service-browser.json"; + +/** + * The shortest gap between two diagnostics writes. + * + * The dropped-frame counter can move on every frame, so its writes are + * throttled; every other event writes at once, because those are the events + * somebody is waiting to see. + */ +const WEBUI_BROWSER_DIAGNOSTICS_WRITE_INTERVAL_MS = 500; + +/** + * One failure the service saw, as the diagnostics record it. + * + * The code is the browser's own when the failure carries one — the same code + * the caller would have been given — and the message is what the failure + * said. A failure that names nothing is still a failure, and is recorded as + * such. + */ +interface WebuiBrowserFailureRecord { + readonly at: string; + readonly message: string; + readonly code: string | undefined; +} + +/** + * One attempt to start the isolated browser, as the diagnostics record it. + * + * The attempt carries the failure that came before it. A start that follows + * a failed start is a retry, and a retry that leaves no trace of what it + * retries is indistinguishable from a first start. + */ +interface WebuiBrowserLaunchRecord { + readonly at: string; + readonly trigger: "warmup" | "demand"; + readonly outcome: "started" | "failed"; + readonly durationMs: number; + readonly failure: WebuiBrowserFailureRecord | undefined; + readonly previousFailure: WebuiBrowserFailureRecord | undefined; +} + /** * The run whose selection the panel is showing when no run is active. * @@ -155,6 +257,15 @@ export interface WebuiBrowserServiceOptions { }; /** The Chrome executable the isolated browser should use. */ readonly chromePath?: string; + /** + * How long a stream waits for its first frame. + * + * The default is the service's own budget. It is injectable so a test can + * exercise the deadline without waiting for it — the deadline is a product + * behaviour, and a test that shortened the wait must not shorten what + * production waits for. + */ + readonly firstFrameTimeoutMs?: number; } /** @@ -203,6 +314,48 @@ export class WebuiBrowserService { >(); /** How many frames each surface dropped, per surface. */ readonly #droppedFrames = new Map(); + /** + * What this service has seen of the browser session. + * + * The record is what the diagnostics artifact is written from. It exists + * because the failures this service sees are otherwise invisible: a start + * that failed and left nothing behind is indistinguishable from a start + * that never happened, and a stream that never produced a frame is + * indistinguishable from a stream that is merely quiet. + */ + readonly #diagnostics = { + /** How many eager starts were attempted, and how many of them failed. */ + warmupAttempts: 0, + warmupFailures: 0, + /** How many starts were attempted, and how many of them failed. */ + launchAttempts: 0, + launchFailures: 0, + /** How long every start took, added up. */ + launchTotalDurationMs: 0, + /** The last few attempts, oldest first. */ + launches: [] as WebuiBrowserLaunchRecord[], + /** The last attempt, whatever its outcome. */ + lastLaunch: undefined as WebuiBrowserLaunchRecord | undefined, + /** The failure the last eager start left behind. */ + lastWarmupFailure: undefined as WebuiBrowserFailureRecord | undefined, + /** The last failure of any kind, which the next attempt carries. */ + lastFailure: undefined as WebuiBrowserFailureRecord | undefined, + /** The first frame of the last stream, and how long it took. */ + firstFrame: undefined as + | { + readonly surface: WebuiBrowserSurface; + readonly targetId: string; + readonly latencyMs: number; + readonly at: string; + } + | undefined, + /** How many streams gave up waiting for their first frame. */ + firstFrameTimeouts: 0, + }; + /** The diagnostics write that is in flight. */ + #diagnosticsWrite: Promise = Promise.resolve(); + /** The diagnostics write that is scheduled, if any. */ + #diagnosticsTimer: ReturnType | undefined; /** * The web tabs each session has open, per surface and then per run. * @@ -597,10 +750,18 @@ export class WebuiBrowserService { // Starting eagerly is what makes "turn it on and it works" true. A // failure here is not fatal: the surface stays unauthorized-but-on, and // the next list request reports the provider failure with its own code. + this.#diagnostics.warmupAttempts += 1; try { - await this.#ensureProvider(surface); - } catch { - // Reported by the call that needs the provider. + await this.#ensureProvider(surface, { trigger: "warmup" }); + } catch (error) { + // The failure is not thrown: the switch is on, and the next call + // that needs the browser starts it again. But it must not vanish + // with the catch that swallowed it either. A start that failed and + // left nothing behind is indistinguishable from a start that never + // happened, so the failure is recorded in the session's diagnostics + // and the next attempt carries it — the retry is a visible retry. + this.#recordWarmupFailure(error); + await this.#writeDiagnostics(); } } } @@ -608,6 +769,7 @@ export class WebuiBrowserService { async #ensureProvider( surface: WebuiBrowserSurface, + options: { readonly trigger?: "warmup" | "demand" } = {}, ): Promise { if (this.#closed) return undefined; const existing = this.#providers.get(surface); @@ -642,13 +804,13 @@ export class WebuiBrowserService { const provider = await pending.promise; if (pending.revoked && !this.#closed) { const authorization = await this.readAuthorization(); - if (isBrowserSurfaceUsable(authorization, surface)) return this.#ensureProvider(surface); + if (isBrowserSurfaceUsable(authorization, surface)) return this.#ensureProvider(surface, options); } return provider; } const acquisition: { revoked: boolean; promise: Promise } = { revoked: false, - promise: Promise.resolve().then(() => this.#launchIsolatedProvider(acquisition)).finally(() => { + promise: Promise.resolve().then(() => this.#launchIsolatedProvider(acquisition, options.trigger ?? "demand")).finally(() => { if (this.#acquisition === acquisition) this.#acquisition = undefined; }), }; @@ -656,16 +818,30 @@ export class WebuiBrowserService { return acquisition.promise; } - async #launchIsolatedProvider(acquisition: { revoked: boolean }): Promise { + async #launchIsolatedProvider( + acquisition: { revoked: boolean }, + trigger: "warmup" | "demand", + ): Promise { const surface = "isolated-chromium"; const create = this.#options.createIsolatedProvider ?? ((options: { readonly dataDir: string; readonly chromePath?: string }) => WebuiIsolatedChromiumProvider.launch(options)); - const provider = await create({ - dataDir: this.#options.dataDir, - ...(this.#options.chromePath ? { chromePath: this.#options.chromePath } : {}), - }); + const startedAt = Date.now(); + let provider: WebuiBrowserSurfaceProvider; + try { + provider = await create({ + dataDir: this.#options.dataDir, + ...(this.#options.chromePath ? { chromePath: this.#options.chromePath } : {}), + }); + } catch (error) { + // The browser did not start. The attempt is recorded before the failure + // travels on, so the record of what went wrong outlives the call that + // would otherwise have swallowed it. + await this.#recordLaunch(trigger, "failed", Date.now() - startedAt, error); + throw error; + } + await this.#recordLaunch(trigger, "started", Date.now() - startedAt); let authorization: WebuiBrowserAuthorization; try { authorization = await this.readAuthorization(); @@ -991,6 +1167,9 @@ export class WebuiBrowserService { // sees a picture of a page it did not ask for, not even briefly. if (!this.#frameMatchesTarget(target, frame)) { this.#droppedFrames.set(surface, (this.#droppedFrames.get(surface) ?? 0) + 1); + // The count has an outlet now: it is written into the session's + // diagnostics, where the drop can be read rather than guessed at. + this.#scheduleDiagnostics(); return; } const index = queue.findIndex( @@ -1039,22 +1218,67 @@ export class WebuiBrowserService { // page whoever looks at it. if (viewport) await provider.setViewport?.(targetKey, viewport); if (first) await provider.startImageStream(targetKey, handler); + + // The first frame's deadline. + // + // A stream that never produces a frame is a panel that waits forever, and + // the person cannot tell that apart from a slow page. The clock runs from + // the moment the screencast is asked for, and stops at the first frame + // that reaches the subscriber: a stream that is running is not a stream + // that is stuck, however long it runs. + const firstFrameBudgetMs = + typeof this.#options.firstFrameTimeoutMs === "number" && + this.#options.firstFrameTimeoutMs > 0 + ? this.#options.firstFrameTimeoutMs + : WEBUI_BROWSER_FIRST_FRAME_TIMEOUT_MS; + const firstFrameStartedAt = Date.now(); + let firstFrameSeen = false; + let firstFrameDeadlinePassed = false; + let firstFrameDeadline: ReturnType | undefined; + const firstFrameDeadlinePromise = new Promise((resolve) => { + firstFrameDeadline = setTimeout(() => { + firstFrameDeadlinePassed = true; + resolve(); + }, firstFrameBudgetMs); + }); try { while (!signal?.aborted) { while (queue.length > 0) { const frame = queue.shift(); if (!frame) continue; + if (!firstFrameSeen) { + firstFrameSeen = true; + this.#recordFirstFrame(surface, target, Date.now() - firstFrameStartedAt); + // The deadline is over: the frame arrived, so nothing is waiting + // for one any more. + if (firstFrameDeadline) clearTimeout(firstFrameDeadline); + void this.#writeDiagnostics(); + } yield frame; } + if (!firstFrameSeen && firstFrameDeadlinePassed) { + // The picture is not coming. The stream says so rather than + // waiting: the server turns this into the error frame the client + // is waiting for, with the browser's own timeout code. + this.#diagnostics.firstFrameTimeouts += 1; + await this.#writeDiagnostics(); + throw browserOperationError( + "browser_operation_timeout", + `The browser delivered no frame for "${target.targetId}" within ${firstFrameBudgetMs} ms.`, + true, + ); + } await Promise.race([ new Promise((resolve) => { notify = resolve; }), new Promise((resolve) => setTimeout(resolve, 250)), + firstFrameDeadlinePromise, ]); notify = undefined; } } finally { + if (firstFrameDeadline) clearTimeout(firstFrameDeadline); subscribers.delete(deliver); if (subscribers.size === 0) { // Cancelling the stream releases the provider-side resources it held: @@ -1132,6 +1356,169 @@ export class WebuiBrowserService { return this.#droppedFrames.get(surface) ?? 0; } + /** + * The directory the browser diagnostics are written under. + * + * The host's own directory when it names one, and otherwise a directory + * under the data dir the browser keeps its profile in. + */ + #diagnosticsDirectory(): string { + const configured = process.env.WEBUI_BROWSER_OUTPUT_DIR; + if (typeof configured === "string" && configured.trim().length > 0) return configured; + return join(this.#options.dataDir, WEBUI_BROWSER_DIAGNOSTICS_DIRECTORY); + } + + /** + * The session's diagnostics, as the artifact holds them. + * + * The shape is fixed rather than incidental: a reader who knows the file + * can find the numbers in it without knowing when it was written. + */ + #diagnosticsDocument(): Record { + const droppedFrames: Record = {}; + for (const [surface, count] of this.#droppedFrames) droppedFrames[surface] = count; + return { + schema: "webui.browser.service.diagnostics.v1", + updatedAt: new Date().toISOString(), + dataDir: this.#options.dataDir, + warmup: { + attempts: this.#diagnostics.warmupAttempts, + failures: this.#diagnostics.warmupFailures, + lastFailure: this.#diagnostics.lastWarmupFailure ?? null, + }, + launches: { + attempts: this.#diagnostics.launchAttempts, + failures: this.#diagnostics.launchFailures, + totalDurationMs: this.#diagnostics.launchTotalDurationMs, + last: this.#diagnostics.lastLaunch ?? null, + history: this.#diagnostics.launches, + }, + firstFrame: { + last: this.#diagnostics.firstFrame ?? null, + timeouts: this.#diagnostics.firstFrameTimeouts, + }, + droppedFrames, + }; + } + + /** + * Writes the session's diagnostics. + * + * The write is best-effort: a diagnostics file that cannot be written must + * not stop the browser from being used. The writes are serialized, so two + * events cannot tear the file, and each write holds the state as it was + * when it ran — the last write therefore holds the state as it is. + */ + async #writeDiagnostics(): Promise { + const write = this.#diagnosticsWrite.then( + () => this.#writeDiagnosticsFile(), + () => this.#writeDiagnosticsFile(), + ); + this.#diagnosticsWrite = write.catch(() => undefined); + await write; + } + + /** Writes the diagnostics file itself. */ + async #writeDiagnosticsFile(): Promise { + try { + const directory = this.#diagnosticsDirectory(); + await mkdir(directory, { recursive: true }); + await writeFile( + join(directory, WEBUI_BROWSER_SERVICE_DIAGNOSTICS_FILE), + `${JSON.stringify(this.#diagnosticsDocument(), null, 2)}\n`, + "utf8", + ); + } catch { + // A diagnostics file that cannot be written is a missing file, not a + // broken browser. + } + } + + /** + * Schedules the diagnostics to be written. + * + * The dropped-frame counter can move on every frame, so its write is + * throttled rather than immediate. Every other event writes at once: those + * are the events somebody is waiting to see. + */ + #scheduleDiagnostics(): void { + if (this.#diagnosticsTimer) return; + const timer = setTimeout(() => { + this.#diagnosticsTimer = undefined; + void this.#writeDiagnostics(); + }, WEBUI_BROWSER_DIAGNOSTICS_WRITE_INTERVAL_MS); + this.#diagnosticsTimer = timer; + // The timer must not hold the process open: a diagnostics write is never + // worth a host that will not shut down. + (timer as { unref?: () => void }).unref?.(); + } + + /** + * Records one attempt to start the isolated browser. + * + * The attempt carries the failure that came before it: a start that + * follows a failed start is a retry, and a retry that leaves no trace of + * what it retries is a start that looks like the first one. + */ + async #recordLaunch( + trigger: "warmup" | "demand", + outcome: "started" | "failed", + durationMs: number, + error?: unknown, + ): Promise { + const failure = outcome === "failed" ? describeBrowserFailure(error) : undefined; + const record: WebuiBrowserLaunchRecord = { + at: new Date().toISOString(), + trigger, + outcome, + durationMs, + failure, + previousFailure: this.#diagnostics.lastFailure, + }; + this.#diagnostics.launchAttempts += 1; + this.#diagnostics.launchTotalDurationMs += durationMs; + if (failure) { + this.#diagnostics.launchFailures += 1; + this.#diagnostics.lastFailure = failure; + } + this.#diagnostics.lastLaunch = record; + this.#diagnostics.launches.push(record); + // The history is a tail, not a ledger: it is here to show the sequence + // of attempts, and the sequence that matters is the recent one. + if (this.#diagnostics.launches.length > 10) this.#diagnostics.launches.shift(); + await this.#writeDiagnostics(); + } + + /** + * Records that the eager start failed. + * + * The eager start is not allowed to fail the switch: the surface stays on, + * and the next call that needs the browser starts it again. But the failure + * must not disappear with the catch that swallowed it. + */ + #recordWarmupFailure(error: unknown): void { + const failure = describeBrowserFailure(error); + this.#diagnostics.warmupFailures += 1; + this.#diagnostics.lastWarmupFailure = failure; + this.#diagnostics.lastFailure = failure; + } + + /** + * Records that a stream delivered its first frame, and how long it took. + */ + #recordFirstFrame( + surface: WebuiBrowserSurface, + target: WebuiBrowserTargetId, + latencyMs: number, + ): void { + this.#diagnostics.firstFrame = { + surface, + targetId: target.targetId, + latencyMs, + at: new Date().toISOString(), + }; + } + /** * Whether a frame still describes the page behind the target. * @@ -1517,9 +1904,32 @@ export class WebuiBrowserService { for (const [surface, provider] of [...this.#providers]) { await this.#stopProvider(surface, provider); } + // The session's record is written out one last time, so the numbers the + // artifact holds are the numbers the session ended with. + if (this.#diagnosticsTimer) { + clearTimeout(this.#diagnosticsTimer); + this.#diagnosticsTimer = undefined; + } + await this.#writeDiagnostics(); } } +/** + * Describes a failure the way the diagnostics record it. + * + * The code is the browser's own when the failure carries one — the same code + * the caller would have been given — and the message is what the failure + * said. A failure that names nothing is still a failure, and is recorded as + * one. + */ +function describeBrowserFailure(error: unknown): WebuiBrowserFailureRecord { + return { + at: new Date().toISOString(), + message: error instanceof Error ? error.message : String(error), + code: error instanceof WebuiBrowserOperationError ? error.code : undefined, + }; +} + function sameTab(left: WebuiBrowserTargetId, right: WebuiBrowserTargetId): boolean { return left.surface === right.surface && left.browserId === right.browserId && left.targetId === right.targetId; } diff --git a/packages/webui/test/unit/browser-service.test.ts b/packages/webui/test/unit/browser-service.test.ts index 770f07806..50a749e9a 100644 --- a/packages/webui/test/unit/browser-service.test.ts +++ b/packages/webui/test/unit/browser-service.test.ts @@ -1,4 +1,8 @@ -import { describe, expect, it, vi } from "vitest"; +import { mkdtemp, readFile } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; + +import { afterEach, describe, expect, it, vi } from "vitest"; import { WebuiBrowserService, @@ -183,6 +187,7 @@ function createService( initial: WebuiBrowserAuthorization, tabIds: readonly string[] = ["tab-1"], edgeProvider?: WebuiBrowserSurfaceProvider, + overrides: Partial = {}, ): { service: WebuiBrowserService; provider: RecordingProvider; @@ -203,6 +208,7 @@ function createService( ...(edgeProvider ? { createBridgeProvider: async (surface: string) => (surface === "edge" ? edgeProvider : undefined) } : {}), + ...overrides, }; return { service: new WebuiBrowserService(options), provider, state }; } @@ -279,7 +285,7 @@ describe("browser service", () => { const starting = new Promise((resolve) => { started = resolve; }); const create = vi.fn(() => { started(); return new Promise((resolve) => { release = resolve; }); }); const authorization = { ...ALL_OFF, isolatedChromium: true }; - const service = new WebuiBrowserService({ authorization: { read: async () => authorization, write: async () => authorization }, dataDir: "unused", createIsolatedProvider: create }); + const service = new WebuiBrowserService({ authorization: { read: async () => authorization, write: async () => authorization }, dataDir: "/tmp/webui-browser-unused-data-dir", createIsolatedProvider: create }); const first = service.listTargets("isolated-chromium"); const second = service.listTargets("isolated-chromium"); await starting; @@ -333,7 +339,7 @@ describe("browser service", () => { let authorization = { ...ALL_OFF, isolatedChromium: true }; const service = new WebuiBrowserService({ authorization: { read: async () => authorization, write: async (input) => (authorization = { ...authorization, ...input }) }, - dataDir: "unused-recording-provider", + dataDir: "/tmp/webui-browser-unused-recording-provider", createIsolatedProvider: create, }); const first = service.listTargets("isolated-chromium"); @@ -1513,3 +1519,203 @@ async function stop( await new Promise((resolve) => setTimeout(resolve, 20)); await service.close(); } +/** + * The failures the service sees must be visible. + * + * A start that failed and left nothing behind is indistinguishable from a + * start that never happened, and a stream that never produced a frame is + * indistinguishable from a stream that is merely quiet. The session's + * diagnostics are where both are written down. + */ +describe("the browser session's diagnostics", () => { + afterEach(() => { + vi.unstubAllEnvs(); + }); + + /** A directory of its own, so the artifact under test is this session's. */ + async function diagnosticsDirectory(): Promise { + const directory = await mkdtemp(join(tmpdir(), "webui-browser-diagnostics-")); + vi.stubEnv("WEBUI_BROWSER_OUTPUT_DIR", directory); + return directory; + } + + /** The artifact the session wrote. */ + async function readArtifact(directory: string): Promise { + const raw = await readFile(join(directory, "service-browser.json"), "utf8"); + return JSON.parse(raw) as BrowserDiagnosticsArtifact; + } + + it("gives up on a stream whose first frame never arrives", async () => { + const directory = await diagnosticsDirectory(); + const { service, provider } = createService( + { ...ALL_OFF, isolatedChromium: true }, + ["tab-1"], + undefined, + // The budget is the service's own; the test shortens the wait so the + // case runs quickly without changing what production waits for. + { firstFrameTimeoutMs: 150 }, + ); + await service.listTargets("isolated-chromium"); + await service.selectTarget(tab("tab-1"), "run-panel"); + + const startedAt = Date.now(); + // The provider never pushes a frame: the panel asked for a picture and + // the browser never sent one. + const failure = await streamFailure(service, tab("tab-1")); + const elapsed = Date.now() - startedAt; + + // The stream ended with the browser's own timeout code, which is what + // the server puts on the wire for the client. + expect(failure).toBeInstanceOf(WebuiBrowserOperationError); + expect((failure as WebuiBrowserOperationError).code).toBe("browser_operation_timeout"); + // The screencast really was running: the deadline ended a live stream, + // it did not stop one from starting. + expect(provider.startedStreams).toContain("tab-1"); + // The stream gave up rather than hanging: it ended at the deadline, and + // it ended at all. + expect(elapsed).toBeGreaterThanOrEqual(140); + expect(elapsed).toBeLessThan(5_000); + // Giving up released what the stream held: the screencast was stopped, + // not left running for a panel that has already been told it failed. + expect(provider.stoppedStreams).toContain("tab-1"); + + const artifact = await readArtifact(directory); + expect(artifact.firstFrame.timeouts).toBe(1); + await service.close(); + }); + + it("records an eager start that could not launch the browser", async () => { + const directory = await diagnosticsDirectory(); + const authorization = { ...ALL_OFF, isolatedChromium: true }; + const service = new WebuiBrowserService({ + authorization: { read: async () => authorization, write: async () => authorization }, + dataDir: "/tmp/webui-browser-warmup-test", + createIsolatedProvider: async () => { + throw new Error("the browser did not start"); + }, + }); + + // The eager start fails and the switch stays on: the failure is not the + // switch's to report, and the switch must not be torn down over it. + await expect(service.initialize()).resolves.toBeUndefined(); + + const artifact = await readArtifact(directory); + expect(artifact.warmup.attempts).toBe(1); + expect(artifact.warmup.failures).toBe(1); + expect(artifact.warmup.lastFailure?.message).toContain("the browser did not start"); + expect(artifact.launches.attempts).toBe(1); + expect(artifact.launches.failures).toBe(1); + expect(artifact.launches.history[0]?.trigger).toBe("warmup"); + expect(artifact.launches.history[0]?.outcome).toBe("failed"); + // Nothing came before the first attempt. + expect(artifact.launches.history[0]?.previousFailure).toBeUndefined(); + + // The next call that needs the browser starts it again, and the retry + // carries the failure that came before it: a retry that leaves no trace + // of what it retries is a start that looks like the first one. + await expect(service.listTargets("isolated-chromium")).rejects.toThrow("the browser did not start"); + const retried = await readArtifact(directory); + expect(retried.launches.attempts).toBe(2); + expect(retried.launches.failures).toBe(2); + expect(retried.launches.history[1]?.trigger).toBe("demand"); + expect(retried.launches.history[1]?.previousFailure?.message).toContain("the browser did not start"); + await service.close(); + }); + + it("records how long the first frame took", async () => { + const directory = await diagnosticsDirectory(); + const { service, provider } = createService({ ...ALL_OFF, isolatedChromium: true }); + await service.listTargets("isolated-chromium"); + + const stream = collectFrames(service, tab("tab-1")); + await waitUntil(() => provider.startedStreams.includes("tab-1")); + provider.pushFrame(1, "tab-1"); + await settleFrames(); + await stop(service, [stream]); + + // The first frame's cost is written down: a picture that took a second + // and a picture that took a quarter of one are not the same thing, and + // only one of them can be read from the record. + const artifact = await readArtifact(directory); + expect(artifact.firstFrame.last?.targetId).toBe("tab-1"); + expect(artifact.firstFrame.last?.surface).toBe("isolated-chromium"); + expect(artifact.firstFrame.last?.latencyMs).toBeGreaterThanOrEqual(0); + expect(artifact.firstFrame.timeouts).toBe(0); + }); + + it("writes the dropped frames into the session's diagnostics", async () => { + const directory = await diagnosticsDirectory(); + const { service, provider } = createService({ ...ALL_OFF, isolatedChromium: true }); + await service.listTargets("isolated-chromium"); + + const stream = collectFrames(service, tab("tab-1")); + await waitUntil(() => provider.startedStreams.includes("tab-1")); + // A frame that names another page is dropped rather than put on screen, + // and the drop is counted. + provider.frameHandlers.get("tab-1")?.({ + target: { ...tab("tab-1"), targetId: "tab-2" }, + data: "QlRSQVk=", + format: "jpeg", + sequence: 1, + }); + await settleFrames(); + expect(service.droppedFrameCount("isolated-chromium")).toBe(1); + await stop(service, [stream]); + + // The count is not a number nobody can reach: it is written down. + const artifact = await readArtifact(directory); + expect(artifact.droppedFrames["isolated-chromium"]).toBe(1); + }); +}); + +/** The shape the session's diagnostics artifact holds. */ +interface BrowserDiagnosticsArtifact { + readonly schema: string; + readonly warmup: { + readonly attempts: number; + readonly failures: number; + readonly lastFailure?: { readonly message: string }; + }; + readonly launches: { + readonly attempts: number; + readonly failures: number; + readonly totalDurationMs: number; + readonly history: readonly { + readonly trigger: string; + readonly outcome: string; + readonly durationMs: number; + readonly failure?: { readonly message: string }; + readonly previousFailure?: { readonly message: string }; + }[]; + }; + readonly firstFrame: { + readonly last?: { + readonly surface: string; + readonly targetId: string; + readonly latencyMs: number; + }; + readonly timeouts: number; + }; + readonly droppedFrames: Record; +} + +/** + * The error a stream ended with, if it ended with one. + * + * A stream that never produces a frame does not end by itself, so the + * failure can only be read by asking the stream for it. + */ +async function streamFailure( + service: WebuiBrowserService, + target: WebuiBrowserTargetId, +): Promise { + try { + for await (const _frame of service.watchFrames("isolated-chromium", target)) { + return undefined; + } + return undefined; + } catch (error) { + return error; + } +} + From 88f5c472d81a859a3fa2e494538e33cfee462cc1 Mon Sep 17 00:00:00 2001 From: probe Date: Tue, 10 Nov 2026 23:53:27 +0800 Subject: [PATCH 4/5] fix(webui): show a failed browser stream instead of an empty panel The client's browser frame watcher accepted event frames and dropped error frames, so the refusal the server does put on the wire never reached the panel. The panel's empty placeholder then stood for both "the picture is still coming" and "the picture is not coming", and the person looking at it could not tell which. The watch now reports the refusal the way the other watches in this package report theirs, and the panel says so: a stream that failed shows a notice that the picture is not coming, beside the reason the service gave. The notice renders only when the panel failed, so the default markup is byte for byte what it was. --- .../browser/contracts/browser-port.ts | 9 +++ .../browser/ui/BrowserPanel.public.tsx | 17 +++++ .../browser/ui/BrowserPanelHost.public.tsx | 6 ++ .../webui/src/client/platform/transport.ts | 4 +- packages/webui/src/client/platform/wire.ts | 10 +++ .../test/unit/browser-panel-view.test.tsx | 61 +++++++++++++++++- .../test/unit/webui-transport-wire.test.ts | 64 +++++++++++++++++++ 7 files changed, 168 insertions(+), 3 deletions(-) diff --git a/packages/webui/src/client/features/browser/contracts/browser-port.ts b/packages/webui/src/client/features/browser/contracts/browser-port.ts index 7e33f8772..0e2214bd6 100644 --- a/packages/webui/src/client/features/browser/contracts/browser-port.ts +++ b/packages/webui/src/client/features/browser/contracts/browser-port.ts @@ -239,6 +239,15 @@ export interface BrowserPort { readonly viewport?: WebuiBrowserViewport; }, onFrame: (frame: WebuiBrowserImageFrame) => void, + /** + * The service's refusal, when the stream gave up. + * + * The frames themselves say nothing about a stream that stopped, so the + * refusal is reported here rather than left to the panel to infer: a + * panel that is waiting and a panel whose stream failed look identical + * until one of them is told. + */ + onFailure?: (error: Error) => void, ) => () => void; /** * The session's web tabs, as a snapshot, on a stream. diff --git a/packages/webui/src/client/features/browser/ui/BrowserPanel.public.tsx b/packages/webui/src/client/features/browser/ui/BrowserPanel.public.tsx index e4c7828de..86edc8093 100644 --- a/packages/webui/src/client/features/browser/ui/BrowserPanel.public.tsx +++ b/packages/webui/src/client/features/browser/ui/BrowserPanel.public.tsx @@ -76,6 +76,10 @@ const PANEL_COPY = { absent: "当前运行时没有浏览器能力，浏览器面板不可用。", notReady: "浏览器能力尚未就绪（例如扩展未连接），面板暂时无法显示页面。", unknown: "当前运行时没有报告浏览器能力，面板按未授权处理。", + // The stream gave up. The reason is said once, beside this notice, as a + // toast; this sentence says the half that is about the panel — that the + // picture is not coming, and that opening the page again is the way back. + failed: "浏览器画面流已失败，画面不会再来。重新打开页面可以重试。", } as const; export interface BrowserPanelProps { @@ -411,6 +415,19 @@ export function BrowserPanel({ ) : null} + {/* + A stream that gave up is not a page that is still loading. The empty + placeholder below looks the same in both cases, so the failure says + so — and it says it here, in the panel, rather than only in a toast + that a person can miss. It renders only when the panel failed, so the + default DOM is the one that was there before. + */} + {state?.status === "error" ? ( + + {PANEL_COPY.failed} + + ) : null} +
{frameSrc ? ( { + dispatch({ type: "failed", error: toBrowserError(error) }); + }, ); return () => { stop(); diff --git a/packages/webui/src/client/platform/transport.ts b/packages/webui/src/client/platform/transport.ts index dfa6b614b..68c494c75 100644 --- a/packages/webui/src/client/platform/transport.ts +++ b/packages/webui/src/client/platform/transport.ts @@ -325,8 +325,8 @@ export function createWebuiTransport(options: WebuiTransportOptions): Required request(SEND_BROWSER_COMMAND_OPERATION_NAME, body), - watchBrowserFrames: (body, onFrame) => - watchBrowserFrames(WATCH_BROWSER_FRAMES_OPERATION_NAME, body, onFrame), + watchBrowserFrames: (body, onFrame, onFailure) => + watchBrowserFrames(WATCH_BROWSER_FRAMES_OPERATION_NAME, body, onFrame, onFailure), watchBrowserWebTabs: (body, onSnapshot) => watchBrowserWebTabs(WATCH_BROWSER_WEB_TABS_OPERATION_NAME, body, onSnapshot), openBrowserWebTab: (body) => request(OPEN_BROWSER_WEB_TAB_OPERATION_NAME, body), diff --git a/packages/webui/src/client/platform/wire.ts b/packages/webui/src/client/platform/wire.ts index 5b40e8385..44a0f4f44 100644 --- a/packages/webui/src/client/platform/wire.ts +++ b/packages/webui/src/client/platform/wire.ts @@ -194,6 +194,7 @@ export interface WebuiWire { operation: string, requestBody: { readonly surface: WebuiBrowserSurface; readonly target: WebuiBrowserTargetId }, onFrame: (frame: WebuiBrowserImageFrame) => void, + onFailure?: (error: Error) => void, ): () => void; /** * The web-tab snapshot stream. @@ -614,6 +615,7 @@ export function createWebuiWire({ readonly viewport?: WebuiBrowserViewport; }, onFrame: (frame: WebuiBrowserImageFrame) => void, + onFailure?: (error: Error) => void, ): () => void { let stopped = false; const session = openWireSession(operation, requestBody, { @@ -621,6 +623,14 @@ export function createWebuiWire({ // A stopped watch must not deliver, and `close()` is not a promise that // the socket stops emitting: the caller's flag is what guarantees it. if (stopped) return; + // An error frame is the service's answer: it says the stream gave up + // and why. Dropping it is what leaves a panel waiting for a picture + // that is never coming — the watcher has no way to tell "still + // loading" from "the service said it stopped", so it says neither. + if (frame.kind === "error") { + onFailure?.(new WebuiWireError(frame.code, frame.message, frame.requestId)); + return; + } if (frame.kind !== "event" || !frame.body || typeof frame.body !== "object") return; onFrame(frame.body as WebuiBrowserImageFrame); }, diff --git a/packages/webui/test/unit/browser-panel-view.test.tsx b/packages/webui/test/unit/browser-panel-view.test.tsx index 703b24885..15bd8bdfc 100644 --- a/packages/webui/test/unit/browser-panel-view.test.tsx +++ b/packages/webui/test/unit/browser-panel-view.test.tsx @@ -14,7 +14,7 @@ // asserts the three-button row. import { describe, expect, it } from "vitest"; -import { readFileSync } from "node:fs"; +import { readFileSync, writeFileSync } from "node:fs"; import { renderToStaticMarkup } from "react-dom/server"; import { @@ -65,6 +65,19 @@ function loadedState() { return state; } +/** + * A panel whose frame stream gave up. + * + * The state says so the way the service does: a failed event carries the + * code, and the panel shows that it failed rather than showing nothing. + */ +function failedState() { + return reduceWebuiBrowserPanelState(loadedState(), { + type: "failed", + error: { code: "browser_operation_timeout", message: "The browser delivered no frame.", retryable: true }, + }); +} + /** All-optional, like every port in this package: presence is the assertion. */ const wiredPort: BrowserPort = { getBrowserAuthorization: () => Promise.resolve(WEBUI_BROWSER_AUTHORIZATION_DEFAULT), @@ -118,6 +131,25 @@ describe("the Browser panel", () => { expect(source).not.toContain("dangerouslySetInnerHTML"); }); + it("says so when the frame stream failed, and says nothing extra", () => { + // A stream that gave up and a page that is still loading are the same + // picture on screen: an empty placeholder. The failure is what tells them + // apart, so it has to be on the panel rather than only in a toast. + const healthy = renderToStaticMarkup( + , + ); + const failed = renderToStaticMarkup( + , + ); + + expect(healthy).not.toContain("browser-panel-failed"); + expect(failed).toContain('data-testid="browser-panel-failed"'); + // The failure adds that one notice and changes nothing else — the + // picture, the empty placeholder and the rest of the panel are byte for + // byte what they were. A panel that failed must not repaint the world. + expect(failed.replace(/]*data-testid="browser-panel-failed"[^>]*>[^<]*<\/div>/u, "")).toBe(healthy); + }); + it("is the window's page and nothing beside it", () => { // A browser window is a page. A list of the browser's pages beside it is // what made this read as a media panel with a sidebar, and the strip is @@ -426,3 +458,30 @@ describe("the installation key a Settings page mounts", () => { expect(markup).not.toContain('data-testid="browser-installation-key-value"'); }); }); + + +describe("baseline probe", () => { + it("prints the default-path markup", () => { + const loaded = renderToStaticMarkup( + , + ); + const empty = renderToStaticMarkup( + , + ); + console.log("BASELINE_LOADED>>>" + loaded + "<<<"); + console.log("BASELINE_EMPTY>>>" + empty + "<<<"); + }); +}); + + +describe("baseline probe", () => { + it("writes the default-path markup", () => { + const loaded = renderToStaticMarkup( + , + ); + const empty = renderToStaticMarkup( + , + ); + writeFileSync("/tmp/browser-panel-baseline.json", JSON.stringify({ loaded, empty }, null, 2)); + }); +}); diff --git a/packages/webui/test/unit/webui-transport-wire.test.ts b/packages/webui/test/unit/webui-transport-wire.test.ts index 3e5e18fd6..ca1ff699a 100644 --- a/packages/webui/test/unit/webui-transport-wire.test.ts +++ b/packages/webui/test/unit/webui-transport-wire.test.ts @@ -654,3 +654,67 @@ describe("an inbound frame is judged before it is believed", () => { await expect(pending).resolves.toEqual({ sessions: [], hasMore: false }); }); }); + +describe("the browser frame connection", () => { + const target = { + surface: "isolated-chromium" as const, + browserId: "browser-1", + targetId: "tab-1", + generation: 1, + }; + + it("reports the service's refusal instead of dropping it", async () => { + // An error frame is the service saying the stream gave up. Dropping it + // leaves the panel waiting for a picture that is never coming, and the + // panel cannot tell "still loading" from "the service stopped" on its + // own — so the refusal has to reach it. + const transport = transportWith(ControllableSocket); + const frames: unknown[] = []; + const failures: Error[] = []; + const stop = transport.watchBrowserFrames( + { surface: "isolated-chromium", target }, + (frame) => frames.push(frame), + (error) => failures.push(error), + ); + await tick(); + const socket = ControllableSocket.latest!; + socket.deliver({ + kind: "error", + requestId: socket.request.requestId, + code: "browser_operation_timeout", + message: "The browser delivered no frame for \"tab-1\" within 60000 ms.", + }); + await tick(); + + expect(frames).toHaveLength(0); + expect(failures).toHaveLength(1); + expect(failures[0]?.message).toContain("delivered no frame"); + // The code is the branchable half; the message carries the budget and + // changes with it. + expect((failures[0] as unknown as { code?: string }).code).toBe("browser_operation_timeout"); + stop(); + }); + + it("keeps delivering frames after the watch was opened", async () => { + const transport = transportWith(ControllableSocket); + const frames: unknown[] = []; + const failures: Error[] = []; + const stop = transport.watchBrowserFrames( + { surface: "isolated-chromium", target }, + (frame) => frames.push(frame), + (error) => failures.push(error), + ); + await tick(); + const socket = ControllableSocket.latest!; + socket.deliver({ + kind: "event", + requestId: socket.request.requestId, + body: { target, data: "QUJD", format: "jpeg", sequence: 1 }, + }); + await tick(); + + expect(frames).toHaveLength(1); + expect(failures).toHaveLength(0); + stop(); + }); +}); From 5d7d1e8cc80a28e68e13464d3e2cbac2d5c627cc Mon Sep 17 00:00:00 2001 From: probe Date: Wed, 11 Nov 2026 00:36:43 +0800 Subject: [PATCH 5/5] test(webui): drop the scratch probes left in the panel view test Two "baseline probe" blocks were left in this file from the work that compared the healthy markup with the failed one: one printed the markup to the console, the other wrote it to a hard-coded /tmp path. The second one is what failed the Windows runner. On Windows a /tmp path resolves to D:\tmp on the current drive, the write ended in ENOENT, and the file was reported as failed. The comparison the probes were written for is already a test in this file: "says so when the frame stream failed, and says nothing extra" removes the one notice from the failed markup and compares what is left with the healthy markup. The probes were scaffolding for writing that case; they have no place in the suite. --- .../test/unit/browser-panel-view.test.tsx | 28 +------------------ 1 file changed, 1 insertion(+), 27 deletions(-) diff --git a/packages/webui/test/unit/browser-panel-view.test.tsx b/packages/webui/test/unit/browser-panel-view.test.tsx index 15bd8bdfc..45dd02413 100644 --- a/packages/webui/test/unit/browser-panel-view.test.tsx +++ b/packages/webui/test/unit/browser-panel-view.test.tsx @@ -14,7 +14,7 @@ // asserts the three-button row. import { describe, expect, it } from "vitest"; -import { readFileSync, writeFileSync } from "node:fs"; +import { readFileSync } from "node:fs"; import { renderToStaticMarkup } from "react-dom/server"; import { @@ -459,29 +459,3 @@ describe("the installation key a Settings page mounts", () => { }); }); - -describe("baseline probe", () => { - it("prints the default-path markup", () => { - const loaded = renderToStaticMarkup( - , - ); - const empty = renderToStaticMarkup( - , - ); - console.log("BASELINE_LOADED>>>" + loaded + "<<<"); - console.log("BASELINE_EMPTY>>>" + empty + "<<<"); - }); -}); - - -describe("baseline probe", () => { - it("writes the default-path markup", () => { - const loaded = renderToStaticMarkup( - , - ); - const empty = renderToStaticMarkup( - , - ); - writeFileSync("/tmp/browser-panel-baseline.json", JSON.stringify({ loaded, empty }, null, 2)); - }); -});