diff --git a/packages/server/src/server/managed-processes/managed-processes.ts b/packages/server/src/server/managed-processes/managed-processes.ts index 1de767586f..01eecf893d 100644 --- a/packages/server/src/server/managed-processes/managed-processes.ts +++ b/packages/server/src/server/managed-processes/managed-processes.ts @@ -158,7 +158,7 @@ class SystemManagedProcessTable implements ManagedProcessTable { // The identity query can fail on every call on some machines (a broken Windows PowerShell, // a policy that refuses to start it). Whether the pid is running at all needs no query, so // a gone process still reads as gone there and its records can be retired. - if (inspection.status === "error" && !processExists(pid)) return { status: "not-found" }; + if (inspection.status === "error" && !isProcessAlive(pid)) return { status: "not-found" }; return inspection; } @@ -448,18 +448,9 @@ export function createPidTarget(pid: number): TreeKillTarget { }; } -function isProcessAlive(pid: number): boolean { - try { - process.kill(pid, 0); - return true; - } catch (error) { - return isNodeErrorWithCode(error, "EPERM"); - } -} - // Signal 0 checks that the pid exists without signaling it: ESRCH means no such process, EPERM // means one that belongs to someone else. -function processExists(pid: number): boolean { +function isProcessAlive(pid: number): boolean { try { process.kill(pid, 0); return true; diff --git a/packages/server/src/terminal/terminal-worker-client.test.ts b/packages/server/src/terminal/terminal-worker-client.test.ts index 01a4dd91d3..119d339b18 100644 --- a/packages/server/src/terminal/terminal-worker-client.test.ts +++ b/packages/server/src/terminal/terminal-worker-client.test.ts @@ -4,7 +4,13 @@ import { tmpdir } from "node:os"; import path from "node:path"; import { isPlatform } from "../test-utils/platform.js"; import { encodeFrame, FrameDecoder, TerminalWorkerFrameError } from "./terminal-worker-frames.js"; -import { connectTerminalWorker, TerminalWorkerConnectError } from "./terminal-worker-client.js"; +import { createConnection, createServer, type Server } from "node:net"; +import { randomUUID } from "node:crypto"; +import { + connectTerminalWorker, + probeTerminalWorkerSocket, + TerminalWorkerConnectError, +} from "./terminal-worker-client.js"; import { launchTerminalWorker } from "./terminal-worker-launch.js"; import { terminalWorkerRecordPath } from "./terminal-worker-record.js"; @@ -74,7 +80,6 @@ describe("terminal worker authentication", () => { }); it("bounds a stalled framed writer to one chunk and resumes in order", async () => { - const { createServer, createConnection } = await import("node:net"); const { attachFramedSocket } = await import("./terminal-worker-frames.js"); const server = createServer(); await new Promise((resolve) => server.listen(0, "127.0.0.1", resolve)); @@ -115,3 +120,41 @@ it("bounds a stalled framed writer to one chunk and resumes in order", async () await new Promise((resolve) => server.close(() => resolve())); } }); + +// The probe decides whether a live pid named by an unidentifiable worker record is still the +// worker. A refused answer retires the record, so it must hold for a Windows named pipe too. +describe("terminal worker socket probe", () => { + function socketPath(): string { + if (isPlatform("win32")) return `\\\\.\\pipe\\paseo-tw-probe-${randomUUID()}`; + const directory = mkdtempSync(path.join(tmpdir(), "tw-probe-")); + temporaryDirs.push(directory); + return path.join(directory, "w.sock"); + } + + async function serve(at: string): Promise { + const server = createServer((socket) => socket.on("error", () => {})); + await new Promise((resolve) => server.listen(at, resolve)); + return server; + } + + it("answers accepting while a worker serves the socket", async () => { + const at = socketPath(); + const server = await serve(at); + try { + expect(await probeTerminalWorkerSocket(at)).toBe("accepting"); + } finally { + await new Promise((resolve) => server.close(resolve)); + } + }); + + it("answers refused for a socket nobody ever served", async () => { + expect(await probeTerminalWorkerSocket(socketPath())).toBe("refused"); + }); + + it("answers refused once the worker that served the socket closed it", async () => { + const at = socketPath(); + const server = await serve(at); + await new Promise((resolve) => server.close(resolve)); + expect(await probeTerminalWorkerSocket(at)).toBe("refused"); + }); +}); diff --git a/packages/server/src/terminal/terminal-worker-reaper.test.ts b/packages/server/src/terminal/terminal-worker-reaper.test.ts index b691afcbf7..f46ed07b1f 100644 --- a/packages/server/src/terminal/terminal-worker-reaper.test.ts +++ b/packages/server/src/terminal/terminal-worker-reaper.test.ts @@ -724,6 +724,26 @@ describe.skipIf(isPlatform("win32"))("terminal worker reaper without process ide expect(existsSync(recordPath)).toBe(false); }); + it("does not look up the children of a gone program recorded without a birth time", async () => { + const home = makeHome(); + const recordPath = writeRecord(home, { + pid: await goneProcessPid(), + identity: { unavailable: "ps unavailable" }, + terminals: [{ id: "t1", pid: await goneProcessPid(), startedAt: null }], + }); + // Linux reports sessions, so a gone program with a birth time would be walked through the + // process table, which this machine cannot read. + const linuxWithoutPs = createTerminalWorkerProcessInspector({ + platform: "linux", + commandRunner: { exec: () => Promise.reject(new Error("spawn ps ENOENT")) }, + }); + + const result = await reapTerminalWorkers({ paseoHome: home, inspector: linuxWithoutPs }); + + expect(result).toEqual({ records: 1, retired: 1, signaled: [] }); + expect(existsSync(recordPath)).toBe(false); + }); + it("never signals a live terminal program recorded without a birth time", async () => { const home = makeHome(); const program = spawnIdle(); diff --git a/packages/server/src/terminal/terminal-worker-reaper.ts b/packages/server/src/terminal/terminal-worker-reaper.ts index e29f5da97a..9ea2e64013 100644 --- a/packages/server/src/terminal/terminal-worker-reaper.ts +++ b/packages/server/src/terminal/terminal-worker-reaper.ts @@ -39,7 +39,8 @@ import { // table cannot be read, nothing in the record is signaled: ending a program would reparent // the children only the table can find, and a retry could never reach them again. // 2. An owned worker, or one its process cannot identify, gets an authenticated `shutdown`: it -// kills each terminal and waits for it. +// kills each terminal and waits for it. A worker that cannot be identified gets it even when +// the caller skips the grace period, since nothing else can stop it. // 3. After `gracefulMs`, SIGKILL whatever owned process is left, re-inspecting right before // each signal. // 4. Succeed only when every candidate is gone or proven unrelated. Otherwise keep a record of @@ -266,14 +267,11 @@ async function reapRecord( return await keepUnresolved(input, tracked, [], `not signaled: ${discovery.tableError}`); } - // The shutdown request authenticates with the record's secret, so a worker that cannot be - // identified by its process is asked too: only the worker that wrote the record can answer. - // It is never signaled. - const askShutdown = isOpen(tracked.worker) && input.gracefulMs > 0; - if (askShutdown) { - await requestWorkerShutdown(record, input.gracefulMs, log); + const shutdownMs = shutdownWindow(tracked.worker, input.gracefulMs); + if (shutdownMs > 0) { + await requestWorkerShutdown(record, shutdownMs, log); } - await waitUntilSettled(tracked, Date.now() + (askShutdown ? input.gracefulMs : 0)); + await waitUntilSettled(tracked, Date.now() + shutdownMs); let signaled: number[] = []; if (!isSettled(tracked)) { @@ -284,6 +282,17 @@ async function reapRecord( return await keepUnresolved(input, tracked, signaled, null); } +// How long the worker gets to answer an authenticated `shutdown` and exit. An owned worker gets +// the grace period before SIGKILL, and none when the caller skips it. The request authenticates +// with the record's secret, so a worker its process cannot identify is asked too: only the worker +// that wrote the record can answer. It is never signaled, so it is asked on every cleanup, +// retries without a grace period included. +function shutdownWindow(worker: ProcessOwnership, gracefulMs: number): number { + if (worker.status === "owned") return gracefulMs; + if (worker.status === "unknown") return Math.max(gracefulMs, DEFAULT_GRACEFUL_MS); + return 0; +} + // Retires the record when every process it names is settled; otherwise writes the cleanup // record, marked closing, with only the unresolved entries. A live worker keeps rewriting its // own record, so the closing state never goes there. `notSignaled` explains open processes that @@ -460,8 +469,12 @@ async function collectCandidates( // already gone: the kernel keeps a session id reserved while a member lives, so a job the // shell left behind is still found. A pid reused by another process (unrelated) leads someone // else's session and is never walked. + // A program recorded without a birth time is never walked: the worker could not read the + // process table when it started it, so the walk would only keep the cleanup unconfirmed. const roots = Array.from(candidates.values()).filter( - (c) => c.state.status === "owned" || (c.state.status === "gone" && inspector.reportsSessions), + (c) => + c.state.status === "owned" || + (c.state.status === "gone" && c.startedAt !== null && inspector.reportsSessions), ); if (roots.length === 0) return { candidates: Array.from(candidates.values()), tableError: null }; diff --git a/packages/server/src/terminal/worker-terminal-manager.test.ts b/packages/server/src/terminal/worker-terminal-manager.test.ts index 72f0ae6692..adf7ce6517 100644 --- a/packages/server/src/terminal/worker-terminal-manager.test.ts +++ b/packages/server/src/terminal/worker-terminal-manager.test.ts @@ -61,6 +61,7 @@ import { listTerminalWorkerRecords, readTerminalWorkerRecord, terminalWorkerDirectory, + terminalWorkerLogPath, terminalWorkerRecordPath, writeTerminalWorkerRecord, type TerminalWorkerRecord, @@ -74,7 +75,7 @@ import { TerminalWorkerIncompatibleError, } from "./terminal-worker-pool.js"; import { connectTerminalWorker } from "./terminal-worker-client.js"; -import { launchTerminalWorker } from "./terminal-worker-launch.js"; +import { launchTerminalWorker, TerminalWorkerLaunchError } from "./terminal-worker-launch.js"; import { currentBootId } from "../server/pid-lock.js"; import { spawn } from "node:child_process"; @@ -2678,6 +2679,24 @@ it.each([false, true])( 45000, ); +// #8810: a worker that cannot start must not take the daemon down. The launcher cannot open its +// log here (a directory sits at its path), so the launch fails before any worker runs. +it("starts without a worker when the worker cannot start, and launches one once it can", async () => { + const paseoHome = createTemporaryHome(); + const logPath = terminalWorkerLogPath(paseoHome); + mkdirSync(logPath); + + manager = await startRealWorkerManager({ paseoHome }); + expect(await workerRecords(paseoHome)).toEqual([]); + await expect( + manager.createTerminal({ workspaceId: "ws-no-worker", cwd: paseoHome }), + ).rejects.toThrow(TerminalWorkerLaunchError); + + rmSync(logPath, { recursive: true }); + trackTerminal(await manager.createTerminal({ workspaceId: "ws-no-worker", cwd: paseoHome })); + expect(await workerRecords(paseoHome)).toHaveLength(1); +}, 30000); + // Both Windows reports (#8766, #8784): the process identity query (Windows PowerShell there, // `ps` here) cannot run at all. Terminals work anyway; only what cleanup may signal degrades. // The fake `ps` dies by a signal, so the query fails the way a process that cannot be created @@ -2804,6 +2823,37 @@ it.skipIf(isPlatform("win32"))( 30000, ); +// A retry of an unconfirmed cleanup, and startup cleanup, run without a grace period. For a +// worker that cannot be identified the shutdown request is the only way it can be stopped. +it.skipIf(isPlatform("win32"))( + "closes a worker whose identity is unavailable from a cleanup without a grace period", + async () => { + const paseoHome = createTemporaryHome(); + const restorePath = withoutProcessIdentity(); + try { + const previous = await startRealWorkerManager({ paseoHome }); + await previous.createTerminal({ + workspaceId: "ws-no-identity", + cwd: paseoHome, + ...nodeTerminalCommand("setInterval(() => {}, 1000);"), + }); + const [record] = await workerRecords(paseoHome); + const programPid = record!.terminals[0]!.pid; + await previous.detach(); + + const result = await reapTerminalWorkers({ paseoHome, gracefulMs: 0 }); + + expect(result.signaled).toEqual([]); + expect(isProcessAlive(programPid)).toBe(false); + expect(await listTerminalWorkerRecords(paseoHome)).toEqual([]); + await waitForCondition(() => !isProcessAlive(record!.pid), 5000); + } finally { + restorePath(); + } + }, + 30000, +); + it.skipIf(isPlatform("win32"))( "launches a worker past records of workers that are gone, when no identity can be read", async () => {