diff --git a/electron/main.cjs b/electron/main.cjs index 914eac5..2b46ae0 100644 --- a/electron/main.cjs +++ b/electron/main.cjs @@ -64,7 +64,7 @@ function serverEntry() { // --browser-service // and connects over CDP (system Chrome stays the fallback). Handshake: // the helper prints "BROWSER_SERVICE_CDP " on stdout once the CDP -// endpoint listens. Lifetime: stdin stays open; parent death = EOF = quit. +// endpoint listens. Lifetime: parent IPC disconnect/death = quit. // MUST run before the single-instance lock — a helper is a second // instance of this very binary, and before the LH_DEV_URL redirect — the // helper owns no user data at all. @@ -73,14 +73,15 @@ if (process.argv.includes("--browser-service")) { // Own the helper's entire startup lifetime, including the readiness wait. const lifetime = new AbortController(); const parentClosed = () => { - lifetime.abort(new Error("browser-service parent stdin closed")); + lifetime.abort(new Error("browser-service parent IPC disconnected")); app.quit(); }; - process.stdin.resume(); - process.stdin.on("end", parentClosed); - process.stdin.on("close", parentClosed); (async () => { try { + // Full Windows Electron replaces stdin with synthetic EOF. Use the + // real Node IPC channel, and refuse to start an unowned helper. + if (!process.channel || !process.connected) throw new Error("browser-service requires connected parent IPC"); + process.once("disconnect", parentClosed); app.dock?.hide(); const port = await freePort(); // Must land before app-ready for Chromium to pick it up. @@ -95,7 +96,7 @@ if (process.argv.includes("--browser-service")) { await waitForBrowserService(port, { signal: lifetime.signal }); if (!lifetime.signal.aborted) console.log(`BROWSER_SERVICE_CDP ${port}`); } catch (err) { - if (lifetime.signal.aborted) return; // EOF is a normal shutdown. + if (lifetime.signal.aborted) return; // Parent disconnect is normal. console.error("browser-service failed:", err); app.exit(1); } diff --git a/scripts/smoke-demo-runtime.mjs b/scripts/smoke-demo-runtime.mjs index 04046bb..5fb7e72 100644 --- a/scripts/smoke-demo-runtime.mjs +++ b/scripts/smoke-demo-runtime.mjs @@ -98,6 +98,46 @@ async function importPayload(standalone) { } } +// Explicit IPC disconnect can omit ChildProcess 'close' even after Electron +// exits. Wait for actual exit, boundedly; never infer shutdown intent from +// Node's automatic stdin cleanup after an unexpected child exit. +/** @returns {Promise<{code: number | null, signal: NodeJS.Signals | null, requested: boolean}>} */ +export async function stopBrowserService(helper) { + if (!helper.pid || helper.exitCode !== null || helper.signalCode !== null) { + return { code: helper.exitCode, signal: helper.signalCode, requested: false }; + } + let onExit, onError; + const exited = new Promise((resolve, reject) => { + onExit = (code, signal) => resolve({ code, signal }); + onError = reject; + helper.once("exit", onExit); + helper.once("error", onError); + }); + const wait = async (ms) => { + let timer; + try { + return await Promise.race([exited, new Promise((resolve) => { timer = setTimeout(() => resolve(null), ms); })]); + } finally { clearTimeout(timer); } + }; + try { + const requested = helper.connected; + if (requested) { + console.log("[smoke-demo-runtime] requesting parent IPC disconnect"); + helper.disconnect(); + } + const result = await wait(5000); + if (!result) { + helper.kill("SIGKILL"); + if (!await wait(3000)) throw new Error("browser-service did not exit after forced cleanup"); + throw new Error("browser-service did not exit after IPC disconnect; force-killed"); + } + return { ...result, requested }; + } finally { + helper.removeListener("exit", onExit); + helper.removeListener("error", onError); + } +} + async function browserProbe(puppeteer) { assert.ok(process.versions.electron, "--browser requires a packaged Electron --runtime binary"); const home = process.env.HOME; @@ -109,12 +149,12 @@ async function browserProbe(puppeteer) { "--disable-background-networking", "--disable-component-update", "--no-proxy-server", // Hosted Linux runners disallow Chromium's unprivileged sandbox. ...(process.platform === "linux" ? ["--no-sandbox"] : []), - ], { cwd: home, env, stdio: ["pipe", "pipe", "pipe"] }); + ], { cwd: home, env, stdio: ["ignore", "pipe", "pipe", "ipc"] }); let output = ""; let errors = ""; helper.stderr.on("data", (chunk) => { errors = (errors + chunk).slice(-16_000); }); - const closed = new Promise((resolve) => helper.once("close", (code, signal) => resolve({ code, signal }))); let browser; + let failed = false; try { const port = await new Promise((resolve, reject) => { const timer = setTimeout(() => reject(new Error("browser-service handshake timed out")), 15_000); @@ -152,23 +192,22 @@ async function browserProbe(puppeteer) { assert.deepEqual(failures, [], "demo console/page/network errors"); console.log("[smoke-demo-runtime] packaged browser-service: JS button 0 → 1, clean console/page, page reset ok (no Chrome fallback)"); } catch (err) { + failed = true; throw new Error(`browser-service probe failed at local CDP/browser interaction\n--- browser-service output ---\n${output}\n${errors}`, { cause: err }); } finally { browser?.disconnect(); - helper.stdin.end(); // The shipping lifetime contract: EOF must quit the helper. - let timer; - const result = await Promise.race([ - closed, - new Promise((resolve) => { timer = setTimeout(() => resolve(null), 5000); }), - ]); - clearTimeout(timer); - if (!result) { - helper.kill("SIGKILL"); - await closed; - throw new Error("browser-service did not exit on stdin EOF; force-killed"); + try { + const { code, signal, requested } = await stopBrowserService(helper); + if (requested) console.log(`[smoke-demo-runtime] observed helper exit after requested IPC disconnect: code=${code}, signal=${signal} (no resident helper)`); + else console.log(`[smoke-demo-runtime] helper already exited/disconnected before cleanup: code=${code}, signal=${signal}; no shutdown requested`); + if (!failed) { + assert.ok(requested, "browser-service exited/disconnected before requested cleanup"); + assert.deepEqual({ code, signal }, { code: 0, signal: null }, "browser-service shutdown failed"); + } + } catch (error) { + if (!failed) throw error; + console.error(`[smoke-demo-runtime] cleanup also failed: ${formatProbeError(error)}`); } - assert.deepEqual(result, { code: 0, signal: null }, "browser-service shutdown failed"); - console.log("[smoke-demo-runtime] browser-service exited cleanly on stdin EOF (no resident helper)"); } } diff --git a/src/agent/subagents/headless.ts b/src/agent/subagents/headless.ts index be90352..8cbbfc8 100644 --- a/src/agent/subagents/headless.ts +++ b/src/agent/subagents/headless.ts @@ -116,8 +116,9 @@ const NO_CHROME_MESSAGE = // Demos execute JS and need a browser. PRIMARY: this very app's binary // spawned with --browser-service (main.cjs boots a hidden window with a CDP // port and prints "BROWSER_SERVICE_CDP "). FALLBACK: system Chrome. -// The helper's stdin is held open by this process; server death = EOF = the -// helper quits — no orphans. PDFs and SVGs never come here (native now). +// A Node IPC channel owns its lifetime: parent death/disconnect quits the +// helper on every OS (Windows Electron stdin is synthetic EOF). No orphans. +// PDFs and SVGs never come here (native now). /** Handshake line parser (pure; exported for tests). */ export function parseBrowserServiceLine(line: string): number | null { @@ -180,7 +181,7 @@ async function connectBrowserService(): Promise((resolve) => { @@ -197,7 +198,7 @@ async function connectBrowserService(): Promise { clearTimeout(timer); resolve(null); @@ -215,7 +216,7 @@ async function connectBrowserService(): Promise void; + const appReady = new Promise((resolve) => { allowAppReady = resolve; }); + let allowReadiness!: () => void; + const readiness = new Promise((resolve) => { allowReadiness = resolve; }); + const state = { quits: 0, exits: [] as number[], windows: 0, readinessCalls: 0, stdinReads: 0, signal: undefined as AbortSignal | undefined }; + const logs: string[] = []; + const errors: string[] = []; + // Match Electron 44's Windows override, not a native pipe. + const stdin = new Readable({ read() {} }); + stdin.push(null); + const process = Object.assign(new EventEmitter(), { + platform: "win32", argv: ["electron", "--browser-service"], env: {}, + connected, channel: channel ? {} : undefined, + }); + Object.defineProperty(process, "stdin", { get() { state.stdinReads++; return stdin; } }); + const app = { + setName() {}, commandLine: { appendSwitch() {} }, whenReady: () => appReady, + quit() { state.quits++; }, exit(code: number) { state.exits.push(code); }, + }; + class FakeWindow { + constructor() { state.windows++; } + async loadURL(url: string) { assert.equal(url, "about:blank"); } + } + const net = { + createServer: () => Object.assign(new EventEmitter(), { + listen(_port: number, _host: string, callback: () => void) { callback(); }, + address: () => ({ port: 54321 }), close(callback: () => void) { callback(); }, + }), + }; + const waitForBrowserService = (_port: number, { signal }: { signal: AbortSignal }) => { + state.readinessCalls++; + state.signal = signal; + return new Promise((resolve, reject) => { + if (signal.aborted) return reject(signal.reason); + signal.addEventListener("abort", () => reject(signal.reason), { once: true }); + readiness.then(resolve); + }); + }; + runInNewContext(mainSource, { + require: (id: string) => id === "electron" ? { app, BrowserWindow: FakeWindow } + : id === "node:net" ? net + : id === "./browser-service-readiness.cjs" ? { waitForBrowserService } + : requireMain(id), + process, __dirname: dirname(mainFile), AbortController, + console: { log: (line: string) => logs.push(line), error: (...parts: unknown[]) => errors.push(parts.join(" ")) }, + }, { filename: mainFile }); + return { + state, logs, errors, stdin, process, allowAppReady, allowReadiness, + disconnect() { process.connected = false; process.emit("disconnect"); }, + }; +} + +describe("browser-service IPC-owned lifetime", () => { + it("ignores synthetic Windows stdin EOF while IPC remains connected", async () => { + const helper = helperFixture(); + helper.stdin.resume(); + await turn(); + assert.equal(helper.stdin.readableEnded, true); + assert.equal(helper.state.stdinReads, 0); + assert.equal(helper.state.quits, 0); + helper.allowAppReady(); + await turn(); + assert.equal(helper.state.readinessCalls, 1); + assert.equal(helper.state.signal?.aborted, false); + helper.allowReadiness(); + await turn(); + assert.deepEqual(helper.logs, ["BROWSER_SERVICE_CDP 54321"]); + assert.deepEqual(helper.state.exits, []); + helper.disconnect(); + assert.equal(helper.state.quits, 1); + }); + + it("disconnect during readiness aborts it, quits once, and never announces", async () => { + const helper = helperFixture(); + helper.allowAppReady(); + await turn(); + assert.equal(helper.state.readinessCalls, 1); + helper.disconnect(); + helper.disconnect(); + await turn(); + assert.equal(helper.state.signal?.aborted, true); + assert.equal(helper.state.quits, 1); + assert.deepEqual(helper.logs, []); + assert.deepEqual(helper.errors, []); + assert.deepEqual(helper.state.exits, []); + assert.equal(helper.state.stdinReads, 0); + }); + + it("owns disconnect before app.whenReady resolves", async () => { + const helper = helperFixture(); + helper.disconnect(); + await turn(); + assert.equal(helper.state.quits, 1); + assert.equal(helper.state.windows, 0); + assert.deepEqual(helper.logs, []); + }); + + it("fails closed without both a connected IPC channel and connection state", async () => { + for (const [connected, channel] of [[false, false], [true, false], [false, true]]) { + const helper = helperFixture(connected, channel); + await turn(); + assert.deepEqual(helper.state.exits, [1]); + assert.match(helper.errors.join("\n"), /requires connected parent IPC/); + assert.equal(helper.state.windows, 0); + assert.equal(helper.state.readinessCalls, 0); + assert.equal(helper.state.stdinReads, 0); + assert.equal(helper.process.listenerCount("disconnect"), 0); + } + }); +}); + +describe("browser-service probe cleanup", () => { + it("requests IPC disconnect and accepts actual exit without a close event", async () => { + const helper = Object.assign(new EventEmitter(), { + pid: 123, connected: true, exitCode: null as number | null, signalCode: null, + disconnect() { + helper.connected = false; + queueMicrotask(() => { helper.exitCode = 0; helper.emit("exit", 0, null); }); + }, + }); + assert.deepEqual(await stopBrowserService(helper), { code: 0, signal: null, requested: true }); + assert.equal(helper.listenerCount("exit"), 0); + assert.equal(helper.listenerCount("error"), 0); + }); + + it("does not label an already-exited helper as parent-requested shutdown", async () => { + const helper = { pid: 123, connected: false, exitCode: 0, signalCode: null, + disconnect() { assert.fail("must not request shutdown after early exit"); } }; + assert.deepEqual(await stopBrowserService(helper), { code: 0, signal: null, requested: false }); + }); +});