fix(sessions): pty list must not reap a live session on a transient pid read

src/sessions.ts

@@ -318,35 +318,48 @@ export async function listSessions(): Promise<SessionInfo[]> {
   const sessions: SessionInfo[] = [];
   const seen = new Set<string>();
 
-  // Find running sessions (have .sock files)
+  // Find running sessions (have .sock files). A live session is destroyed here
+  // ONLY on POSITIVE proof of death — a readable pid whose process is gone AND
+  // an unreachable socket. A transiently-unreadable pid must NOT be mistaken
+  // for a dead process: the daemon creates its .sock (listen) BEFORE it writes
+  // its .pid, and the plain pidfile write can be caught mid-flight, so under
+  // concurrent multi-agent load a `pty list` can momentarily read a null pid
+  // for a perfectly healthy session. Reaping on that (the old behavior) deleted
+  // a live daemon's socket/pid out from under it, making it invisible and
+  // getting it GC'd + re-launched by consumers that reconcile on not-running.
   const sockFiles = entries.filter((e) => e.endsWith(".sock"));
   for (const sockFile of sockFiles) {
     const name = sockFile.replace(/\.sock$/, "");
     seen.add(name);
     const socketPath = getSocketPath(name);
     const pid = readPid(name);
-    const alive =
-      pid !== null &&
-      isProcessAlive(pid) &&
-      (await isSocketReachable(socketPath));
-
-    if (alive) {
+    const pidAlive = pid !== null && isProcessAlive(pid);
+    // A reachable control socket proves the daemon is alive INDEPENDENTLY of
+    // whether we could read the pidfile this instant.
+    const socketReachable = await isSocketReachable(socketPath);
+
+    if (pidAlive || socketReachable) {
+      // Alive: a live process, or a reachable control socket (busy/mid-startup
+      // daemon whose pid we couldn't read). The daemon writes exit metadata
+      // before its cleanup delay, so a reachable socket can briefly coexist
+      // with exitedAt being set.
       const metadata = readMetadata(name);
-      // The daemon writes exit metadata before its cleanup delay, so a
-      // reachable socket can briefly coexist with exitedAt being set.
       const status = metadata?.exitedAt ? "exited" : "running";
       sessions.push({ name, socketPath, pid, status, metadata });
-    } else if (pid !== null && isProcessAlive(pid)) {
-      // Pid is still alive but the socket isn't reachable right now (busy
-      // daemon, transient EAGAIN, race with a service restart). Keep the
-      // socket on disk and report the session as running — deleting the
-      // socket would render the still-alive daemon permanently invisible.
+    } else if (pid !== null) {
+      // Positively dead: the pid read SUCCEEDED and its process is gone, and
+      // the socket is unreachable. Safe to reap the stale socket/pid (keep
+      // metadata so the session stays addressable as exited/vanished below).
+      cleanupSocket(name);
+    } else {
+      // pid UNREADABLE and socket unreachable — we can prove neither life nor
+      // death (most likely a daemon mid-startup, or a pidfile write that raced
+      // our read under load). Do NOT destroy it; report it running defensively
+      // (a .sock exists). A later read resolves the true state once the pidfile
+      // settles or the socket comes up.
       const metadata = readMetadata(name);
       const status = metadata?.exitedAt ? "exited" : "running";
       sessions.push({ name, socketPath, pid, status, metadata });
-    } else {
-      // Process really died — clean up socket/pid but keep metadata
-      cleanupSocket(name);
     }
   }
 

tests/list-live-session-race.test.ts

@@ -0,0 +1,137 @@
+// Concurrency robustness: `pty list` must not destroy a LIVE session when its
+// pidfile is transiently unreadable.
+//
+// The daemon creates its .sock (listen) BEFORE it writes its .pid, and the
+// plain pidfile write can be caught mid-flight, so under concurrent multi-agent
+// load a `pty list` can momentarily read a null pid for a perfectly healthy
+// session. The old listSessions treated "can't read pid" as "process dead" and
+// ran cleanupSocket — deleting the live daemon's socket/pid out from under it,
+// making it invisible and getting it GC'd + re-launched by consumers that
+// reconcile on not-running. Destruction must require POSITIVE proof of death;
+// a reachable control socket alone proves the daemon is alive.
+
+import { describe, it, expect, afterEach, afterAll } from "vitest";
+import * as fs from "node:fs";
+import * as os from "node:os";
+import * as path from "node:path";
+import { fileURLToPath } from "node:url";
+import { spawn, spawnSync } from "node:child_process";
+
+const __dirname = path.dirname(fileURLToPath(import.meta.url));
+const nodeBin = process.execPath;
+const cliPath = path.join(__dirname, "..", "dist", "cli.js");
+const serverModule = path.join(__dirname, "..", "dist", "server.js");
+
+const testRoot = fs.mkdtempSync(path.join(os.tmpdir(), "pty-listrace-"));
+afterAll(() => {
+  fs.rmSync(testRoot, { recursive: true, force: true, maxRetries: 3, retryDelay: 100 });
+});
+
+let bgPids: number[] = [];
+let sessionDirs: string[] = [];
+
+function makeSessionDir(): string {
+  const dir = fs.mkdtempSync(path.join(testRoot, "d-"));
+  sessionDirs.push(dir);
+  return dir;
+}
+
+async function startDaemon(sessionDir: string, name: string): Promise<number> {
+  const config = JSON.stringify({
+    name, command: "cat", args: [], displayCommand: "cat",
+    cwd: os.tmpdir(), rows: 24, cols: 80,
+  });
+  const child = spawn(nodeBin, [serverModule], {
+    detached: true,
+    stdio: ["ignore", "ignore", "pipe"],
+    env: { ...process.env, PTY_SERVER_CONFIG: config, PTY_SESSION_DIR: sessionDir },
+  });
+  let stderr = "";
+  child.stderr?.on("data", (d: Buffer) => { stderr += d.toString(); });
+  let exitCode: number | null = null;
+  child.on("exit", (code) => { exitCode = code; });
+  (child.stderr as any)?.unref?.();
+  child.unref();
+
+  const socketPath = path.join(sessionDir, `${name}.sock`);
+  const start = Date.now();
+  while (Date.now() - start < 5000) {
+    if (exitCode !== null) throw new Error(`Daemon exited: ${stderr}`);
+    try {
+      fs.statSync(socketPath);
+      await new Promise((r) => setTimeout(r, 100));
+      bgPids.push(child.pid!);
+      return child.pid!;
+    } catch {}
+    await new Promise((r) => setTimeout(r, 50));
+  }
+  throw new Error(`Timeout waiting for daemon socket: ${socketPath}`);
+}
+
+function runCli(sessionDir: string, args: string[]) {
+  return spawnSync(nodeBin, [cliPath, ...args], {
+    env: { ...process.env, PTY_SESSION_DIR: sessionDir },
+    encoding: "utf-8",
+    timeout: 15000,
+  });
+}
+
+afterEach(() => {
+  for (const pid of bgPids) { try { process.kill(pid, "SIGTERM"); } catch {} }
+  bgPids = [];
+  for (const dir of sessionDirs) {
+    try {
+      for (const e of fs.readdirSync(dir)) { try { fs.unlinkSync(path.join(dir, e)); } catch {} }
+    } catch {}
+  }
+  sessionDirs = [];
+});
+
+describe("pty list: concurrency robustness (do not reap a live session on a transient pid read)", () => {
+  it("keeps a live session running when its pidfile is transiently missing", async () => {
+    const dir = makeSessionDir();
+    const name = "live-norace";
+    await startDaemon(dir, name);
+
+    // Simulate the startup / concurrent-load window: the pidfile is momentarily
+    // absent while the daemon (and its listening socket) is fully alive.
+    fs.unlinkSync(path.join(dir, `${name}.pid`));
+
+    const list = JSON.parse(runCli(dir, ["list", "--json"]).stdout);
+    const found = list.find((s: any) => s.name === name);
+    // The live session must still be reported — and as running, not dropped.
+    expect(found).toBeDefined();
+    expect(found.status).toBe("running");
+    // Critically: its control socket must NOT have been reaped. Deleting it
+    // would make the still-alive daemon invisible + get it re-launched.
+    expect(fs.existsSync(path.join(dir, `${name}.sock`))).toBe(true);
+  }, 20000);
+
+  it("still reaps a genuinely dead session's stale socket (positive proof of death)", async () => {
+    const dir = makeSessionDir();
+    const name = "dead-reap";
+    const pid = await startDaemon(dir, name);
+
+    // Kill the daemon hard so it leaves a stale socket + a readable (but dead)
+    // pidfile — the positive-proof-of-death case that SHOULD still be reaped.
+    process.kill(pid, "SIGKILL");
+    // Wait for the process to actually leave the table.
+    const start = Date.now();
+    while (Date.now() - start < 5000) {
+      try { process.kill(pid, 0); } catch { break; }
+      await new Promise((r) => setTimeout(r, 50));
+    }
+
+    // First list has positive proof of death (readable dead pid + unreachable
+    // socket) → it reaps the stale socket/pid.
+    runCli(dir, ["list", "--json"]);
+    expect(fs.existsSync(path.join(dir, `${name}.sock`))).toBe(false);
+
+    // The session stays addressable as vanished (a SIGKILLed daemon wrote no
+    // exit record) once only its metadata remains.
+    const list2 = JSON.parse(runCli(dir, ["list", "--json"]).stdout);
+    const found = list2.find((s: any) => s.name === name);
+    expect(found).toBeDefined();
+    expect(found.status).toBe("vanished");
+  }, 20000);
+});