diff --git a/src/sessions.ts b/src/sessions.ts index da677a8..ca931d4 100644 --- a/src/sessions.ts +++ b/src/sessions.ts @@ -798,7 +798,12 @@ type ReapObservedResult = signalled: boolean; }; -function hasProcessExitedForReap(pid: number): boolean { +/** Is `pid` gone for reaping purposes? A zombie counts as exited. + * + * Exported because the spawner needs the same question answered. + * `isProcessAlive` is not a substitute: an unreaped process still answers + * `kill(pid, 0)`, so the cheap predicate calls a corpse live. */ +export function hasProcessExitedForReap(pid: number): boolean { if (!isProcessAlive(pid)) return true; try { if (process.platform === "linux") { diff --git a/src/spawn.ts b/src/spawn.ts index 0f77ac9..c045598 100644 --- a/src/spawn.ts +++ b/src/spawn.ts @@ -6,6 +6,7 @@ import { fileURLToPath } from "node:url"; import { acquireLock, getEventsPath, getSocketPath, readMetadata, releaseLock, validateDisplayName, + hasProcessExitedForReap, } from "./sessions.ts"; const __dirname = path.dirname(fileURLToPath(import.meta.url)); @@ -228,6 +229,32 @@ async function spawnViaNode(options: SpawnDaemonOptions, serverModule: string): metadata.daemonPid === child.pid && hasPublishedSessionStart(options.name, metadata.createdAt); if (startPublished) break; + // **Read the fact that is already there before waiting for one that is + // not.** If somebody else has published this name, this attempt can never + // win: the check above compares against our own pid and stays false for + // the rest of the budget. The only other way out of this loop is noticing + // our own daemon die, so when that is slow — a loaded machine, a daemon + // still starting up — the loop spends the whole start timeout and then + // reports a timeout, when the true answer was on disk in the first pass. + // + // Measured on a Mac by Silber.pty on 2026-09-03: the losing `pty run` + // took 30.06 s against a 30 s budget and said "Timed out waiting for + // daemon publication" instead of "is already running". + // **NOT `isProcessAlive`.** A zombie answers `kill(pid, 0)`, so the cheap + // predicate calls a corpse live — and a corpse recorded as the owner would + // make this session name refuse every future `pty run`, which is precisely + // the failure this check exists to avoid. + const ownerLive = (pid: number) => !hasProcessExitedForReap(pid); + if (publishedElsewhere(metadata?.daemonPid ?? null, child.pid ?? -1, ownerLive, () => + metadata !== null && hasPublishedSessionStart(options.name, metadata.createdAt), + )) { + // Deliberately the same sentence `pty run` prints when it sees a + // running session before it spawns. Losing the race later should not + // produce a different explanation of the same situation. + throw new Error( + `Session "${options.name}" is already running. Use "pty attach ${options.name}" to connect.`, + ); + } checkEarlyExit(); if (Date.now() - startedAt >= timeoutMs) { throw new Error(`Timed out waiting for daemon publication for session "${options.name}".`); @@ -242,6 +269,30 @@ async function spawnViaNode(options: SpawnDaemonOptions, serverModule: string): } } +/** Has this session been published by a live process that is not us? + * + * All three conditions matter. **Published**, or we would refuse a name whose + * metadata is still being written. **By a different pid**, or we would refuse + * our own success. **By a live one**, or stale metadata from a daemon that died + * would make the name permanently unusable. + * + * "Live" means `hasProcessExitedForReap`, never `isProcessAlive`. **A zombie + * answers `kill(pid, 0)`**, measured on Linux 2026-09-03, so the cheap + * predicate calls a corpse live. An unreaped daemon is the precise case this + * has to get right. + * + * Kept separate from the registry so all four ways of answering "no" can be + * tested rather than raced for. */ +export function publishedElsewhere( + owner: number | null, + mine: number, + alive: (pid: number) => boolean, + published: () => boolean, +): boolean { + if (owner === null) return false; + return owner !== mine && alive(owner) && published(); +} + function hasPublishedSessionStart(name: string, createdAt: string): boolean { try { return fs.readFileSync(getEventsPath(name), "utf8") diff --git a/tests/spawn-already-published.test.ts b/tests/spawn-already-published.test.ts new file mode 100644 index 0000000..7ad0cef --- /dev/null +++ b/tests/spawn-already-published.test.ts @@ -0,0 +1,67 @@ +// A `pty run` that loses a creation race waited out the whole 30 second start +// budget and then reported a generic publication timeout, when the true answer +// was on disk in the first pass. +// +// The safety property was never in question: exactly one process wins. What was +// wrong is what the loser said, and how long it took to say it. + +import { describe, expect, it } from "vitest"; +import { spawn } from "node:child_process"; +import { publishedElsewhere } from "../src/spawn.ts"; +import { hasProcessExitedForReap, isProcessAlive } from "../src/sessions.ts"; + +const sleep = (ms: number): Promise => + new Promise((resolve) => setTimeout(resolve, ms)); + +describe("deciding that somebody else owns the name", () => { + const live = () => true; + const dead = () => false; + const yes = () => true; + const no = () => false; + + // Every way of answering "no", so the one way of answering "yes" means + // something. Raced for, only the last of these would ever be exercised. + it("needs a live, different, published owner", () => { + expect(publishedElsewhere(200, 100, live, yes)).toBe(true); + }); + + it("does not count our own pid as somebody else", () => { + expect(publishedElsewhere(100, 100, live, yes)).toBe(false); + }); + + it("does not count a dead owner, which would make the name unusable forever", () => { + expect(publishedElsewhere(200, 100, dead, yes)).toBe(false); + }); + + it("does not count metadata that is still being written", () => { + expect(publishedElsewhere(200, 100, live, no)).toBe(false); + }); + + it("does not count a missing owner", () => { + expect(publishedElsewhere(null, 100, live, yes)).toBe(false); + }); + + // The predicate the production path actually passes, against a real corpse. + // `isProcessAlive` says true for a zombie, which would refuse the name for as + // long as the corpse went unreaped. An unreaped daemon is the precise case. + it("does not count a zombie daemon as a live owner", async () => { + const sh = spawn("sh", ["-c", "sleep 0.1 & echo $! ; kill -STOP $$"], { + stdio: ["ignore", "pipe", "ignore"], + }); + try { + const corpse = await new Promise((resolve) => + sh.stdout!.once("data", (d) => resolve(Number(String(d).trim()))), + ); + for (let i = 0; i < 500 && !hasProcessExitedForReap(corpse); i++) await sleep(10); + + expect(isProcessAlive(corpse), "precondition: a zombie answers kill(pid, 0)").toBe(true); + const ownerLive = (pid: number) => !hasProcessExitedForReap(pid); + expect( + publishedElsewhere(corpse, 100, ownerLive, yes), + "a zombie daemon must not make the session name unusable", + ).toBe(false); + } finally { + sh.kill("SIGKILL"); + } + }, 20_000); +});