Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion src/sessions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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") {
Expand Down
51 changes: 51 additions & 0 deletions src/spawn.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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));
Expand Down Expand Up @@ -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}".`);
Expand All @@ -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")
Expand Down
67 changes: 67 additions & 0 deletions tests/spawn-already-published.test.ts
Original file line number Diff line number Diff line change
@@ -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<void> =>
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<number>((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);
});
Loading