diff options
| -rw-r--r-- | CHANGELOG.md | 1 | ||||
| -rw-r--r-- | src/daemon/transport.ts | 9 | ||||
| -rw-r--r-- | test/daemon/fixtures/transport-exit-fixture.ts | 28 | ||||
| -rw-r--r-- | test/daemon/transport-exit-test.ts | 36 |
4 files changed, 72 insertions, 2 deletions
diff --git a/CHANGELOG.md b/CHANGELOG.md index d85ef79..12b4738 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,7 @@ All notable changes to pi-browser-harness will be documented in this file. ### Fixed +- **`pi -p` exits after using browser tools.** The client's socket to the daemon is kept open across sessions by design, and in non-interactive mode it was the last handle keeping Node alive, so a print-mode run that had connected to the browser printed its answer and then hung until killed. The socket no longer holds the event loop; in-flight requests keep their own timers, so nothing awaited is affected. (#39) - **`browser_download` no longer redirects the user's own downloads for good.** `Browser.setDownloadBehavior` was sent without a `browserContextId`, which Chrome applies to its default context — the one the user's own tabs are in — and the override lives on the browser connection, which the daemon keeps open across pi sessions. Every later download in that Chrome, the user's own included, went to the tool's temp directory, and nothing undid it when the session ended. The tool now takes `restore: true` to reset the behavior, and a clean session shutdown restores the default whenever this session's override is still in force (a killed session cannot, so call `restore: true` as soon as you are done). - **Browser state is no longer appended to the session on every callback.** Every ownership change, every `session_tree` event (even one that merely restored the entry already on the branch) and every shutdown appended a `browser-harness-state` entry, so the append-only session file and Pi Web's `+N custom` chain grew without bound. A snapshot is now written only when it differs from the last one persisted or restored; tab order does not count as a difference. (#26) - **The daemon starts from a pi-managed install without `typebox`.** pi installs packages with dev and peer dependencies omitted and maps host-provided packages such as `typebox` only inside the extension host, but the daemon is a separate Node process that resolved `typebox` through `src/cdp/discovery.ts` and died with `Cannot find module 'typebox'` before binding its socket, so `/browser-setup` reported `Could not start the browser daemon`. The daemon's `/json/version` parsing no longer uses `typebox`, and a test walks the daemon's import graph to keep every reachable package a runtime dependency. (#34) diff --git a/src/daemon/transport.ts b/src/daemon/transport.ts index 3c42336..b67610d 100644 --- a/src/daemon/transport.ts +++ b/src/daemon/transport.ts @@ -13,7 +13,7 @@ import { serialize, } from "./protocol"; -export const createDaemonTransport = (clientId: string): CdpTransport => { +export const createDaemonTransport = (clientId: string, socketPath: string = DAEMON_SOCKET_PATH): CdpTransport => { let socket: Socket | null = null; let rl: ReturnType<typeof createInterface> | null = null; let queue = makeEventQueue(); @@ -51,12 +51,17 @@ export const createDaemonTransport = (clientId: string): CdpTransport => { let sock: Socket; try { - sock = createConnection(DAEMON_SOCKET_PATH); + sock = createConnection(socketPath); } catch (e) { settle(err(cdpError("transport_closed", e instanceof Error ? e.message : String(e)))); return; } socket = sock; + // The client is kept connected across sessions on purpose (index.ts session_shutdown), so this + // socket must not be what keeps the process alive: in `pi -p` it was the last handle standing + // and pi never exited (#39). In-flight requests hold their own ref'd timers, so nothing a + // caller is awaiting can be dropped by this. + sock.unref(); const connectTimer = setTimeout(() => { cleanup("Connection timeout"); diff --git a/test/daemon/fixtures/transport-exit-fixture.ts b/test/daemon/fixtures/transport-exit-fixture.ts new file mode 100644 index 0000000..2753866 --- /dev/null +++ b/test/daemon/fixtures/transport-exit-fixture.ts @@ -0,0 +1,28 @@ +// Connects a daemon transport to a fake daemon that lives in this same process, with every handle +// the fake owns unref'd. The only thing that can keep this process alive afterwards is the +// transport's own socket, so "does the process exit on its own" is the behavior under test. +import { createServer } from "node:net"; +import { createInterface } from "node:readline"; +import { createDaemonTransport } from "../../../src/daemon/transport"; +import { deserialize, serialize } from "../../../src/daemon/protocol"; + +const socketPath = process.argv[2]; +if (!socketPath) throw new Error("usage: fixture <socketPath>"); + +const server = createServer((conn) => { + conn.unref(); + const rl = createInterface({ input: conn, crlfDelay: Infinity }); + rl.on("line", (line) => { + const msg = deserialize(line); + if (msg?.type === "control" && msg.action === "register" && msg.clientId !== undefined) { + conn.write(serialize({ type: "control", action: "registered", clientId: msg.clientId }) + "\n"); + } + }); +}); +server.unref(); +server.listen(socketPath, async () => { + const transport = createDaemonTransport("fixture-client", socketPath); + const r = await transport.connect(""); + console.log(r.success ? "connected" : `connect failed: ${r.error.message}`); + // No explicit exit: if the transport socket holds the event loop, this process never ends. +}); diff --git a/test/daemon/transport-exit-test.ts b/test/daemon/transport-exit-test.ts new file mode 100644 index 0000000..f74a2e4 --- /dev/null +++ b/test/daemon/transport-exit-test.ts @@ -0,0 +1,36 @@ +import { test, describe } from "node:test"; +import assert from "node:assert/strict"; +import { spawn } from "node:child_process"; +import { mkdtempSync, rmSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { dirname, join } from "node:path"; +import { fileURLToPath } from "node:url"; + +const fixture = join(dirname(fileURLToPath(import.meta.url)), "fixtures", "transport-exit-fixture.ts"); +const SKIP = process.platform === "win32" ? "named pipes are not filesystem paths; the fixture uses a temp socket file" : false; + +describe("daemon transport and process exit", { skip: SKIP }, () => { + test("a connected transport does not keep the process alive once nothing else is pending", async () => { + const dir = mkdtempSync(join(tmpdir(), "pbh-exit-")); + const socketPath = join(dir, "d.sock"); + try { + const child = spawn(process.execPath, ["--conditions=import", "--import", "tsx", fixture, socketPath], { + stdio: ["ignore", "pipe", "pipe"], + }); + let stdout = ""; + let stderr = ""; + child.stdout.on("data", (c: Buffer) => { stdout += c.toString(); }); + child.stderr.on("data", (c: Buffer) => { stderr += c.toString(); }); + + const outcome = await new Promise<"exited" | "hung">((resolve) => { + const timer = setTimeout(() => { child.kill("SIGKILL"); resolve("hung"); }, 15_000); + child.on("exit", () => { clearTimeout(timer); resolve("exited"); }); + }); + + assert.match(stdout, /^connected/m, `fixture did not connect: ${stderr}`); + assert.equal(outcome, "exited", "process stayed alive after the transport connected: pi -p would never exit"); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }); +}); |