diff --git a/apps/server/src/lazurio/holders.test.ts b/apps/server/src/lazurio/holders.test.ts index 89faeeb20f23..5b85a0c3cfff 100644 --- a/apps/server/src/lazurio/holders.test.ts +++ b/apps/server/src/lazurio/holders.test.ts @@ -1,5 +1,7 @@ // @effect-diagnostics nodeBuiltinImport:off globalTimers:off - the holder files are real files in a temporary runtime directory, and the watch is the operating system's. +import * as NodeChildProcess from "node:child_process"; import * as NodeFSP from "node:fs/promises"; +import * as NodeNet from "node:net"; import * as NodeOS from "node:os"; import * as NodePath from "node:path"; import { afterEach, describe, expect, it } from "vite-plus/test"; @@ -408,53 +410,101 @@ describe("T3 Code's holder files", () => { }); describe("stopping a thread's agent-browser daemon", () => { - const setup = async (pid: string | null) => { + // LazurioPlatform#348: the stop goes to the session's own socket and never signals a process + // id, so a stale .pid whose number another session's daemon (or any process named + // agent-browser) has now is never touched. + it("asks only the session's own daemon, on its socket, and signals no process", async () => { const sockets = await temporary("holders-daemon-"); - if (pid !== null) await NodeFSP.writeFile(NodePath.join(sockets, "t3-a.pid"), pid); - const terminated: number[] = []; - return { sockets, terminated, terminate: (target: number) => terminated.push(target) }; - }; + const heard: Record = { "t3-a": [], "t3-b": [] }; + const servers: NodeNet.Server[] = []; + const daemon = (session: "t3-a" | "t3-b") => + new Promise((resolve) => { + const server = NodeNet.createServer((socket) => { + socket.on("data", async (chunk) => { + const line = chunk.toString("utf8").trim(); + heard[session]?.push(line); + const { id } = JSON.parse(line) as { id: string }; + socket.write(`${JSON.stringify({ id, success: true, data: { closed: true } })}\n`); + await NodeFSP.rm(NodePath.join(sockets, `${session}.pid`), { force: true }); + }); + }); + servers.push(server); + server.listen(NodePath.join(sockets, `${session}.sock`), () => resolve()); + }); + // A live process named agent-browser whose id t3-a's stale .pid names. + const named = NodePath.join(sockets, "agent-browser"); + await NodeFSP.symlink("/bin/sleep", named); + const sleeper = NodeChildProcess.spawn(named, ["30"], { stdio: "ignore" }); + try { + expect(await stopSessionDaemon(sockets, "t3-a")).toBe("none"); + await NodeFSP.writeFile(NodePath.join(sockets, "t3-a.pid"), `${sleeper.pid}\n`); + await daemon("t3-b"); + expect(await stopSessionDaemon(sockets, "t3-a")).toBe("none"); + expect(heard["t3-b"]).toEqual([]); + expect(sleeper.exitCode).toBeNull(); + expect(sleeper.signalCode).toBeNull(); + // A socket file nobody listens on is no daemon either. + await NodeFSP.writeFile(NodePath.join(sockets, "t3-c.sock"), ""); + expect(await stopSessionDaemon(sockets, "t3-c")).toBe("none"); + // t3-a's own daemon: one shutdown line, then its .pid is gone. + await daemon("t3-a"); + expect(await stopSessionDaemon(sockets, "t3-a")).toBe("stopped"); + expect(heard["t3-a"]?.map((line) => JSON.parse(line))).toEqual([ + { id: "lazurio-browser-done", action: "__agent_browser_internal_shutdown" }, + ]); + await expect(NodeFSP.access(NodePath.join(sockets, "t3-a.pid"))).rejects.toThrow(); + expect(sleeper.signalCode).toBeNull(); + } finally { + sleeper.kill(); + for (const server of servers) server.close(); + } + }); - it("sends SIGTERM only to a live agent-browser process", async () => { - const { sockets, terminated, terminate } = await setup("4242\n"); - const commandLine = async () => "/home/u/.local/bin/agent-browser-linux-x64\0"; - expect( - await stopSessionDaemon(sockets, "t3-a", { alive: () => true, commandLine, terminate }), - ).toBe("stopped"); - expect(terminated).toEqual([4242]); + it("is stopped only once the .pid is gone: a refusal changes nothing, a lost answer or a lingering daemon is unconfirmed", async () => { + const sockets = await temporary("holders-daemon-"); + const refusing = { ask: async () => ({ kind: "reply", line: '{"success":false}' }) as const }; + expect(await stopSessionDaemon(sockets, "t3-a", refusing)).toBe("refused"); + const silent = { ask: async () => ({ kind: "no-answer" }) as const }; + expect(await stopSessionDaemon(sockets, "t3-a", silent)).toBe("unconfirmed"); + // Acknowledged, but the daemon has not exited: it may still hold the tab. + await NodeFSP.writeFile(NodePath.join(sockets, "t3-a.pid"), "4242\n"); + const lingering = { + ask: async () => ({ kind: "reply", line: '{"success":true}' }) as const, + exitWithinMs: 150, + }; + expect(await stopSessionDaemon(sockets, "t3-a", lingering)).toBe("unconfirmed"); + expect(await stopSessionDaemon(sockets, "../t3-a")).toBe("none"); }); - it("never signals a reused pid, an ended one or an unclear one", async () => { - const reused = await setup("4242"); - expect( - await stopSessionDaemon(reused.sockets, "t3-a", { - alive: () => true, - commandLine: async () => "/usr/bin/python3\0server.py\0", - terminate: reused.terminate, - }), - ).toBe("not-agent-browser"); - expect(reused.terminated).toEqual([]); - for (const [pid, alive] of [ - ["4242", false], - ["1", true], - ["", true], - ["x", true], - ] as const) { - const fixture = await setup(pid); - expect( - await stopSessionDaemon(fixture.sockets, "t3-a", { - alive: () => alive, - commandLine: async () => "agent-browser\0", - terminate: fixture.terminate, - }), - ).toBe("none"); - expect(fixture.terminated).toEqual([]); - } - // No .pid: no daemon runs. - const none = await setup(null); - expect(await stopSessionDaemon(none.sockets, "t3-a", { terminate: none.terminate })).toBe( - "none", + // Pablo's review of #56: a failure that proves nothing is never "none" or "stopped". + it("a socket or a .pid that cannot be reached is unconfirmed, never none or stopped", async () => { + const sockets = await temporary("holders-daemon-"); + const server = NodeNet.createServer(() => undefined); + await new Promise((resolve) => + server.listen(NodePath.join(sockets, "t3-a.sock"), () => resolve()), ); - expect(none.terminated).toEqual([]); + try { + // A daemon listens, but its socket cannot be opened. + await NodeFSP.chmod(NodePath.join(sockets, "t3-a.sock"), 0o000); + expect(await stopSessionDaemon(sockets, "t3-a")).toBe("unconfirmed"); + } finally { + server.close(); + } + // Acknowledged, and the .pid's directory cannot be read: not proven gone. + const pidDirectory = await temporary("holders-pid-"); + await NodeFSP.writeFile(NodePath.join(pidDirectory, "t3-a.pid"), "4242\n"); + const acknowledged = { + ask: async () => { + await NodeFSP.chmod(pidDirectory, 0o000); + return { kind: "reply", line: '{"success":true}' } as const; + }, + exitWithinMs: 150, + }; + try { + expect(await stopSessionDaemon(pidDirectory, "t3-a", acknowledged)).toBe("unconfirmed"); + } finally { + await NodeFSP.chmod(pidDirectory, 0o700); + } + expect(await NodeFSP.readFile(NodePath.join(pidDirectory, "t3-a.pid"), "utf8")).toBe("4242\n"); }); }); diff --git a/apps/server/src/lazurio/holders.ts b/apps/server/src/lazurio/holders.ts index 12583e7b8bf7..2339be80c80d 100644 --- a/apps/server/src/lazurio/holders.ts +++ b/apps/server/src/lazurio/holders.ts @@ -1,4 +1,4 @@ -// @effect-diagnostics nodeBuiltinImport:off - The holder files are the Platform's, read and written as it does. +// @effect-diagnostics nodeBuiltinImport:off globalDate:off globalTimers:off - The holder files and the daemon's socket are the Platform's, read, written and asked outside the Effect runtime as it does. /** * Lazurio overlay (plan DEV-6646, task 501; LazurioPlatform decision F47 points 4 and 5): who @@ -17,12 +17,14 @@ * parseHolderRecords, replaceOwnerFile, processAlive and `watchDirectories` (the Platform's * `watchHolds`). holders.test.ts runs the Platform's vectors, so a drift between the copies fails * there. Added here: the hub's rule for a daemon that runs (`daemonWorking`, from `readHolds`), - * the bindings of agent-browser's sessions, and the stop of a thread's daemon. + * the bindings of agent-browser's sessions, and the stop of a thread's daemon (the Platform's + * `stopSessionDaemon`, over the session's socket). */ import * as NodeCrypto from "node:crypto"; import * as NodeFS from "node:fs"; import * as NodeFSP from "node:fs/promises"; +import * as NodeNet from "node:net"; import * as NodePath from "node:path"; /** A strip's name is a file name: agent-browser's session alphabet, as T3 @@ -355,44 +357,130 @@ export async function readSessionBindings( return bindings; } -/** What the daemon stop needs of the host: overridable in tests (a fake process table). */ +/** What a session's daemon answered on its socket: nothing listens there (`absent`), a reply + * line, or no reply in time. */ +export type DaemonAnswer = + | Readonly<{ kind: "absent" }> + | Readonly<{ kind: "reply"; line: string }> + | Readonly<{ kind: "no-answer" }>; + +/** What the daemon stop and the bindings need of the host: overridable in tests. */ export type DaemonSeams = Readonly<{ alive?: (pid: number) => boolean; - /** `/proc//cmdline`, NUL-separated. */ - commandLine?: (pid: number) => Promise; - terminate?: (pid: number) => void; read?: (path: string) => Promise; + /** Sends one command line to a socket and answers the reply line. */ + ask?: (socketPath: string, line: string) => Promise; + /** Whether a file is still there, polled while the daemon exits. */ + exists?: (path: string) => Promise; + exitWithinMs?: number; }>; -/** The daemon is agent-browser's own executable, re-run with AGENT_BROWSER_DAEMON=1 and no - * arguments (agent-browser `cli/src/connection.rs`), so its command line names agent-browser. */ -const namesAgentBrowser = (commandLine: string) => - /^agent-browser(?:[-_.][A-Za-z0-9_.-]*)?$/.test( - NodePath.basename(commandLine.split("\0")[0] ?? ""), +/** agent-browser's own graceful shutdown of a session's daemon, as its CLI sends it before + * restarting a daemon (`request_graceful_daemon_shutdown` in agent-browser + * `cli/src/connection.rs`): the daemon runs its close, which only disconnects from a browser it + * did not launch, answers and exits, removing its `.pid`. The Platform's + * `agentBrowserShutdownAction`. */ +export const agentBrowserShutdownAction = "__agent_browser_internal_shutdown"; + +const daemonAnswerMs = 5_000; +const maxDaemonReplyBytes = 64 * 1024; + +/** Connection errors that prove nothing listens on a socket path: no file, a socket nobody + * listens on (Linux), a file that is no socket (macOS). Anything else, a permission above all, + * proves nothing. */ +const absentSocketCodes = new Set(["ENOENT", "ECONNREFUSED", "ENOTSOCK"]); + +/** One command line to a Unix socket and its reply line. Nothing listening (no socket, a stale + * one) is `absent`; no reply within 5 s, or a socket that cannot be reached for any other reason, + * is `no-answer`. The Platform's `askDaemonSocket`. */ +export function askDaemonSocket(socketPath: string, line: string): Promise { + return new Promise((resolve) => { + let settled = false; + let connected = false; + let buffer = ""; + const socket = NodeNet.createConnection({ path: socketPath }); + const done = (answer: DaemonAnswer) => { + if (settled) return; + settled = true; + clearTimeout(timer); + socket.destroy(); + resolve(answer); + }; + const timer = setTimeout(() => done({ kind: "no-answer" }), daemonAnswerMs); + socket.on("connect", () => { + connected = true; + socket.write(line); + }); + socket.on("data", (chunk) => { + buffer += chunk.toString("utf8"); + const end = buffer.indexOf("\n"); + if (end >= 0) done({ kind: "reply", line: buffer.slice(0, end) }); + else if (buffer.length > maxDaemonReplyBytes) done({ kind: "no-answer" }); + }); + socket.on("end", () => + done(buffer === "" ? { kind: "no-answer" } : { kind: "reply", line: buffer }), + ); + socket.on("error", (error: NodeJS.ErrnoException) => + done( + !connected && absentSocketCodes.has(error.code ?? "") + ? { kind: "absent" } + : { kind: "no-answer" }, + ), + ); + }); +} + +/** Whether a file is there: only "no such file" is gone; any other failure is thrown, since it + * proves nothing. */ +const fileExists = (path: string) => + NodeFSP.access(path).then( + () => true, + (error: NodeJS.ErrnoException) => { + if (error.code === "ENOENT") return false; + throw error; + }, ); /** - * Stops a session's agent-browser daemon: SIGTERM to the process its `.pid` names, only when - * that is a well-formed id of a live process whose command line is agent-browser's. On SIGTERM - * the daemon of an attached browser disconnects without closing a tab and removes its `.pid`, so - * its binding no longer counts for the hub. Never `agent-browser close`, which first spawns a - * daemon when none runs. Resolves what happened; a failed signal is thrown. + * Stops a session's agent-browser daemon: asks it on that session's own socket + * (`/.sock`) to shut down, then waits up to 5 s for its `.pid` to go. The + * daemon of an attached browser disconnects without closing a tab, so its binding no longer + * counts for the hub. Nothing is signalled by a process id: a stale `.pid` whose number another + * process now has, even another session's daemon, is never touched (LazurioPlatform#348). Never + * `agent-browser close`, which first starts a daemon when none runs. `stopped`: the daemon + * answered and its `.pid` is gone; `none`: nothing listens on the session's socket; `refused`: + * the daemon answered that it did not shut down; `unconfirmed`: no answer after the command went + * out, or the `.pid` still there after the wait, so the daemon may still hold the tab. The + * Platform's `stopSessionDaemon`. */ export async function stopSessionDaemon( socketDirectory: string, session: string, seams: DaemonSeams = {}, -): Promise<"stopped" | "none" | "not-agent-browser"> { +): Promise<"stopped" | "none" | "refused" | "unconfirmed"> { if (!isStrip(session)) return "none"; - const pidText = await fileText(NodePath.join(socketDirectory, `${session}.pid`), seams.read); - const pid = pidText === null ? null : daemonProcessId(pidText); - if (pid === null || !(seams.alive ?? processAlive)(pid)) return "none"; - const commandLine = await ( - seams.commandLine ?? ((id) => NodeFSP.readFile(`/proc/${id}/cmdline`, "utf8")) - )(pid).catch(() => ""); - if (!namesAgentBrowser(commandLine)) return "not-agent-browser"; - (seams.terminate ?? ((id) => process.kill(id, "SIGTERM")))(pid); - return "stopped"; + const answer = await (seams.ask ?? askDaemonSocket)( + NodePath.join(socketDirectory, `${session}.sock`), + `${JSON.stringify({ id: "lazurio-browser-done", action: agentBrowserShutdownAction })}\n`, + ); + if (answer.kind === "absent") return "none"; + // The command went out: the daemon may have taken it even if its answer was lost. + if (answer.kind === "no-answer") return "unconfirmed"; + let success = false; + try { + success = (JSON.parse(answer.line) as { success?: unknown } | null)?.success === true; + } catch {} + if (!success) return "refused"; + const exists = seams.exists ?? fileExists; + const pid = NodePath.join(socketDirectory, `${session}.pid`); + const deadline = Date.now() + (seams.exitWithinMs ?? daemonAnswerMs); + for (;;) { + // Only a .pid proven gone counts; one that cannot be checked is not. + const present = await exists(pid).catch(() => null); + if (present === false) return "stopped"; + if (present === null || Date.now() >= deadline) return "unconfirmed"; + await new Promise((resolve) => setTimeout(resolve, 100)); + } } /** Calls `changed` whenever something in one of the directories changes. diff --git a/apps/server/src/lazurio/threadTabs.test.ts b/apps/server/src/lazurio/threadTabs.test.ts index 4e4e27a13106..70feea8e2681 100644 --- a/apps/server/src/lazurio/threadTabs.test.ts +++ b/apps/server/src/lazurio/threadTabs.test.ts @@ -54,7 +54,9 @@ async function world(options: { readonly declared?: boolean; readonly ready?: bo const listReads: number[] = []; /** The fake process table: pid → command line. */ const processes = new Map(); - const terminated: number[] = []; + /** Sessions whose agent-browser daemon listens on its socket, and those asked to stop. */ + const listening = new Set(); + const stopped: string[] = []; const logs: string[] = []; const watched: Array> = []; let retries = 0; @@ -92,10 +94,13 @@ async function world(options: { readonly declared?: boolean; readonly ready?: bo }, daemon: { alive: (pid) => processes.has(pid), - commandLine: async (pid) => processes.get(pid) ?? "", - terminate: (pid) => { - terminated.push(pid); - processes.delete(pid); + ask: async (socketPath) => { + const session = NodePath.basename(socketPath, ".sock"); + if (!listening.has(session)) return { kind: "absent" }; + listening.delete(session); + stopped.push(session); + await NodeFSP.rm(NodePath.join(sockets, `${session}.pid`), { force: true }); + return { kind: "reply", line: '{"success":true}' }; }, }, watch: (directories, onChange) => { @@ -132,7 +137,8 @@ async function world(options: { readonly declared?: boolean; readonly ready?: bo events, threads, processes, - terminated, + listening, + stopped, logs, watched, listReads, @@ -501,22 +507,25 @@ describe("holds and releases", () => { }); describe("archive and delete", () => { - it("removes the thread's file and stops only its live agent-browser daemon, never a reused pid", async () => { + it("removes the thread's file and asks only its own agent-browser daemon to stop, on its socket", async () => { const w = await world(); await w.core.hold(threadA, id(1)); await w.core.hold(threadB, id(2)); await w.bind(stripA, id(1), 4242); await w.bind(stripB, id(2), 4343); w.processes.set(4242, "/home/u/.local/bin/agent-browser\0"); - w.processes.set(4343, "/usr/bin/vim\0notes.txt\0"); + // B's .pid is stale: its number is another live agent-browser's, and nothing listens on B's + // socket. + w.processes.set(4343, "/home/u/.local/bin/agent-browser\0"); + w.listening.add(stripA); await w.core.lifecycle({ type: "retired", threadId: threadA }); expect(await w.stored()).toBeNull(); - expect(w.terminated).toEqual([4242]); + expect(w.stopped).toEqual([stripA]); expect(tabIds(w.lastSnapshot())).toEqual([]); - // The pid in B's .pid is another program now: nothing is killed. await w.core.lifecycle({ type: "retired", threadId: threadB }); - expect(w.terminated).toEqual([4242]); - expect(w.logs).toContain("thread-tabs-daemon-not-agent-browser"); + expect(w.stopped).toEqual([stripA]); + expect(w.processes.has(4343)).toBe(true); + expect(w.logs).not.toContain("thread-tabs-daemon-stop-failed"); }); it("closes no tab itself: a tab another file holds stays held there", async () => { @@ -592,10 +601,11 @@ describe("the startup sweep", () => { { id: id(6), recordedAt: at(50) }, ]); for (const pid of [5001, 5002, 5003]) w.processes.set(pid, "agent-browser\0"); + for (const session of [stripGone, stripDaemon, "codex-1"]) w.listening.add(session); await w.list([id(4), id(6)], at(100)); await w.declare(); expect(await w.stored(stripGone)).toBeNull(); - expect(w.terminated.sort()).toEqual([5001, 5002]); + expect(w.stopped.toSorted()).toEqual([stripGone, stripDaemon].toSorted()); expect(await w.stored("launchpad")).toEqual([{ id: id(4), recordedAt: at(50) }]); // Live threads' files are kept and reconciled. expect(await w.stored()).toEqual([{ id: id(6), recordedAt: at(50) }]); diff --git a/apps/server/src/lazurio/threadTabs.ts b/apps/server/src/lazurio/threadTabs.ts index 8f22ac1b980b..c37da31d5314 100644 --- a/apps/server/src/lazurio/threadTabs.ts +++ b/apps/server/src/lazurio/threadTabs.ts @@ -465,7 +465,8 @@ export function createThreadTabs(seams: ThreadTabsCoreSeams): ThreadTabsCore { if (seams.socketDirectory === undefined) return; try { const outcome = await stopSessionDaemon(seams.socketDirectory, session, seams.daemon); - if (outcome === "not-agent-browser") log("thread-tabs-daemon-not-agent-browser", { session }); + if (outcome === "refused" || outcome === "unconfirmed") + log("thread-tabs-daemon-stop-failed", { session, outcome }); } catch (error) { log("thread-tabs-daemon-stop-failed", { session, cause: String(error) }); } diff --git a/docs/operations/lazurio-fork-release.md b/docs/operations/lazurio-fork-release.md index 383acf193e63..f36fc8d93d0e 100644 --- a/docs/operations/lazurio-fork-release.md +++ b/docs/operations/lazurio-fork-release.md @@ -467,12 +467,16 @@ T3 žádný prohlížeč nemá, a tak overlay dělá tohle: `rel=noopener`) na tutéž URL: přihlašovací stránka brány se po vypršení přihlášení v rámu nevykreslí. - **Archivace a smazání.** Archivace i smazání vlákna smaže jeho soubor - držitele (i nečitelný) a démonovi sezení agent-browser vlákna pošle SIGTERM: - jen živému procesu z `/t3-.pid`, jehož `/proc//cmdline` - je agent-browser, nikdy znovu použitému pid. Démon připojeného prohlížeče se - odpojí, žádnou záložku nezavře a smaže svůj `.pid`, takže vazba pro hub - přestane počítat; `agent-browser close` se nevolá, protože by démona nejdřív - spustil. T3 nikdy nezavírá záložku sám: záložka, kterou drží Apps nebo jiné + držitele (i nečitelný) a démona sezení agent-browser vlákna požádá o + ukončení na jeho vlastním socketu `/t3-.sock` + (`__agent_browser_internal_shutdown`, které posílá i CLI agent-browseru před + restartem démona). Žádnému procesu nepošle signál podle pid, takže zastaralý + `.pid`, jehož číslo má teď jiný proces nebo démon jiného sezení, nic + nezastaví (LazurioPlatform#348). Když na socketu nikdo neposlouchá, démon + neběží. Démon připojeného prohlížeče se odpojí, žádnou záložku nezavře a + smaže svůj `.pid`, takže vazba pro hub přestane počítat. Příkaz + `agent-browser close` se nevolá, protože by démona nejdřív spustil. + T3 nikdy nezavírá záložku sám: záložka, kterou drží Apps nebo jiné vlákno nebo ji někdo sleduje, žije dál, jiná skončí po 8 s milosti hubu. Obnovení z archivu nic nevrátí. Podržení a archivace jdou jednou frontou zápisů a podržení se těsně před zápisem znovu ujistí, že vlákno žije a není