fix(server): archiving a thread stops only its own agent-browser daemon, over its socket

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<string, string[]> = { "t3-a": [], "t3-b": [] };
+    const servers: NodeNet.Server[] = [];
+    const daemon = (session: "t3-a" | "t3-b") =>
+      new Promise<void>((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<void>((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");
   });
 });

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/<pid>/cmdline`, NUL-separated. */
-  commandLine?: (pid: number) => Promise<string>;
-  terminate?: (pid: number) => void;
   read?: (path: string) => Promise<string>;
+  /** Sends one command line to a socket and answers the reply line. */
+  ask?: (socketPath: string, line: string) => Promise<DaemonAnswer>;
+  /** Whether a file is still there, polled while the daemon exits. */
+  exists?: (path: string) => Promise<boolean>;
+  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<DaemonAnswer> {
+  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
+ * (`<socket dir>/<session>.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.