From 6342361a41191bfe39c15e2bac7ca49227bbcc38 Mon Sep 17 00:00:00 2001 From: Nathan Herald Date: Thu, 3 Sep 2026 14:43:16 +0200 Subject: [PATCH 1/2] Say "is already running" at once instead of after thirty seconds MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A `pty run` that loses a creation race waited out the whole start budget and then reported a generic publication timeout. `wait_for_publication` compares the published metadata against its OWN pid, so for a loser that check is false for the rest of the budget. Its only other way out of the loop is noticing its own daemon die. When that is slow — a loaded machine, a daemon still starting up — the loop spends the entire thirty second default and reports a timeout, when the true answer was on disk in the first iteration. Measured on a Mac by Silber.pty on 2026-09-03: 30.06 s against a 30 s budget, saying "Timed out waiting for daemon publication" instead of "is already running". The loop now checks whether the name is published by a live process that is not us, and stops with the sentence `pty run` already prints when it sees a running session before it spawns. Losing the race later should not produce a different explanation of the same situation. All three conditions matter and each is tested. Published, or a name whose metadata is still being written would be refused. A different pid, or a successful spawn would refuse itself. A live one, or stale metadata from a dead daemon would make the name permanently unusable. The safety property was never in question. The test that found this asserts exactly one winner before it checks the loser message, and that assertion passed every time. What was wrong is what the loser said, and how long it took. --- src/spawn.ts | 41 +++++++++++++++++++++++++++ tests/spawn-already-published.test.ts | 38 +++++++++++++++++++++++++ 2 files changed, 79 insertions(+) create mode 100644 tests/spawn-already-published.test.ts diff --git a/src/spawn.ts b/src/spawn.ts index 0f77ac9..d32b25d 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, + isProcessAlive, } from "./sessions.ts"; const __dirname = path.dirname(fileURLToPath(import.meta.url)); @@ -228,6 +229,27 @@ 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". + if (publishedElsewhere(metadata?.daemonPid ?? null, child.pid ?? -1, isProcessAlive, () => + 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 +264,25 @@ 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. + * + * 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..4c59fc1 --- /dev/null +++ b/tests/spawn-already-published.test.ts @@ -0,0 +1,38 @@ +// 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 { publishedElsewhere } from "../src/spawn.ts"; + +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); + }); +}); From 56fd09ef0499597a83423bead2d85ad4b3403f59 Mon Sep 17 00:00:00 2001 From: Nathan Herald Date: Thu, 3 Sep 2026 14:52:41 +0200 Subject: [PATCH 2/2] Do not call a zombie daemon a live owner The new check refuses a session name when it is published by a live process that is not us. It asked `pid_alive` / `isProcessAlive`, and a zombie answers `kill(pid, 0)`. So an unreaped daemon would have counted as live and the name would have been refused for as long as the corpse went unreaped. That is the exact failure the liveness condition exists to prevent, arrived at by the check that was supposed to prevent it. An unreaped daemon is the precise case that matters here: dead, not reaped, still in the process list. Both now ask `has_process_exited_for_reap` / `hasProcessExitedForReap`, which reads the process state and counts a zombie as gone. Tested against a real corpse in both languages, with the predicate the production path actually passes. Reverting to the cheap predicate fails both. Node also exports `hasProcessExitedForReap`, which was private. Silber.cos asked what this check does with a zombie daemon on macOS. The answer was worse than the question: it was wrong on both platforms. --- src/sessions.ts | 7 ++++++- src/spawn.ts | 14 +++++++++++-- tests/spawn-already-published.test.ts | 29 +++++++++++++++++++++++++++ 3 files changed, 47 insertions(+), 3 deletions(-) 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 d32b25d..c045598 100644 --- a/src/spawn.ts +++ b/src/spawn.ts @@ -6,7 +6,7 @@ import { fileURLToPath } from "node:url"; import { acquireLock, getEventsPath, getSocketPath, readMetadata, releaseLock, validateDisplayName, - isProcessAlive, + hasProcessExitedForReap, } from "./sessions.ts"; const __dirname = path.dirname(fileURLToPath(import.meta.url)); @@ -240,7 +240,12 @@ async function spawnViaNode(options: SpawnDaemonOptions, serverModule: string): // 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". - if (publishedElsewhere(metadata?.daemonPid ?? null, child.pid ?? -1, isProcessAlive, () => + // **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 @@ -271,6 +276,11 @@ async function spawnViaNode(options: SpawnDaemonOptions, serverModule: string): * 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( diff --git a/tests/spawn-already-published.test.ts b/tests/spawn-already-published.test.ts index 4c59fc1..7ad0cef 100644 --- a/tests/spawn-already-published.test.ts +++ b/tests/spawn-already-published.test.ts @@ -6,7 +6,12 @@ // 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; @@ -35,4 +40,28 @@ describe("deciding that somebody else owns the name", () => { 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); });