diff --git a/docs/terminal-worker.md b/docs/terminal-worker.md index be9321fb4f..195c2f798c 100644 --- a/docs/terminal-worker.md +++ b/docs/terminal-worker.md @@ -21,11 +21,14 @@ Which triggers keep terminals and which close them is owned by the stop reason ( ## Record, socket and secret -The worker writes `$PASEO_HOME/terminal-worker/.json` (owner-only) with its identity (birth time + command line carrying `--paseo-terminal-worker `), protocol version, socket path, a random secret, the copy it runs from, and each terminal program's pid and birth time. +The worker writes `$PASEO_HOME/terminal-worker/.json` (owner-only) with its identity (birth time + command line carrying `--paseo-terminal-worker `), protocol version, socket path, a random secret, the copy it runs from, and each terminal program's pid and birth time. The secret is what lets a daemon drive the worker; identity is what lets cleanup signal it. - The first frame on the socket must be `hello` with the secret; anything else closes the connection. Anyone who connects can type into every shell, so the record is `0600`, the unix socket `0600` in a `0700` directory. - Unix socket paths have a byte budget (103 on macOS, 107 on Linux). Dev homes under worktrees blow it, so `terminal-worker-socket-path.ts` tries `$PASEO_HOME/terminal-worker/sock/`, then `$TMPDIR/paseo-/`, then `/tmp/paseo-/`, and accepts an existing directory only if it is a real `0700` directory owned by you. It never chmods a shared root. - Cleanup signals a process only when its birth time matches the record. Birth times come from `ps -o lstart` with `LC_ALL=C` (it follows `LC_TIME` otherwise) and from `Win32_Process.CreationDate`. A missing birth time is "unknown", never matched by command text: a shell that ran `exec` keeps its pid and birth time but not its argv. +- Identity is best effort. Where the query cannot run (on Windows, a Windows PowerShell that cannot load `Get-CimInstance` or that the machine refuses to start: #8766, #8784), the worker logs why to `terminal-worker.log`, records `identity: { unavailable: }` and its programs with `startedAt: null`, and serves terminals as usual. Daemons up to 0.15.1 cannot parse such a record and report it unreadable. +- Without identity, cleanup never signals the worker or those programs. A stop asks the worker to `shutdown` over its socket, which the secret authenticates, and reports whatever survives as unconfirmed. Children of a program without a birth time are not looked up. +- Whether a pid is running needs no identity query (`process.kill(pid, 0)`), so records of workers that are gone are retired on every machine. When the query cannot tell who holds a live pid, the worker's socket decides: a worker listens until it exits, so a refused connection means the pid now runs something else. A socket that accepts keeps the worker counted, and adoption authenticates it with the secret. - A kill cleanup that cannot confirm every process gone writes `closing-.json` beside the worker record. The worker rewrites its own record whenever a terminal starts or exits, so the kill intent has to live in a file it never writes. A closing record is never adopted; the next stop or start retries it. - An unreadable record of either kind makes `daemon status` and a kill restart fail with its path: a kill cleanup that met it could not check what it names, and leaves nothing else behind. A read that fails with `EPERM`, `EBUSY` or `EACCES` is retried for 250ms first: on Windows, opening a record while the worker renames its new copy over it fails that way for a moment. @@ -40,7 +43,7 @@ The worker writes `$PASEO_HOME/terminal-worker/.json` (owner-only) with its | two or more older | shuts it down; its terminals close (logged) | | newer, or a version it cannot read | leaves it running, undriven; `list_terminals` and `create_terminal` answer `rpc_error` `terminal_worker_incompatible`, and `paseo daemon status` prints why | -Never more than two workers run. A new worker launches only when none is current, none is incompatible and fewer than two run. The count comes from the records, so an unreachable or unidentified worker counts, and so does one a cleanup could not confirm gone. When two run, the daemon starts with no current worker, and the first terminal created after one of them exits launches it. +Never more than two workers run. A new worker launches only when none is current, none is incompatible and fewer than two run. The count comes from the records, so an unreachable worker counts, as does one that cannot be identified while its socket still accepts connections, and one a cleanup could not confirm gone. When two run, the daemon starts with no current worker, and the first terminal created after one of them exits launches it. **The frozen envelope never changes across versions:** the frame format (4-byte length + `v8.serialize`), `hello`/`helloAck`, the `listTerminals` info fields and `shutdown`. That is what lets any daemon close any worker: a kill stop sends `shutdown` to a newer worker it cannot otherwise drive. Protocol 5 hard cuts unreleased protocols 1 through 4, including the acknowledgement-based replay contract: those workers are obsolete and close. For future versions, keep a previous-version adapter so the draining worker can still be driven, and drop the adapter for the version before that. diff --git a/packages/server/src/server/managed-processes/managed-processes.test.ts b/packages/server/src/server/managed-processes/managed-processes.test.ts index b4be4d16bd..7d1f2cb6f6 100644 --- a/packages/server/src/server/managed-processes/managed-processes.test.ts +++ b/packages/server/src/server/managed-processes/managed-processes.test.ts @@ -344,8 +344,34 @@ describe("system managed process table", () => { }, ]); }); + + // #8766 (Get-CimInstance cannot load) and #8784 (powershell.exe cannot be created): the + // identity query fails on every call, and a process that is gone must still read as gone. + test.each(["win32", "linux"] as const)( + "tells a gone process from a live one when the %s identity query cannot run", + async (platform) => { + const processTable = createSystemManagedProcessTable({ + platform, + commandRunner: { exec: () => Promise.reject(new Error("spawn UNKNOWN")) }, + }); + + expect(await processTable.inspect(await exitedProcessPid())).toEqual({ + status: "not-found", + }); + expect(await processTable.inspect(process.pid)).toMatchObject({ + status: "error", + error: expect.objectContaining({ message: "spawn UNKNOWN" }), + }); + }, + ); }); +async function exitedProcessPid(): Promise { + const child = spawnProcess(process.execPath, ["-e", ""], { stdio: "ignore" }); + await new Promise((resolve) => child.once("exit", resolve)); + return child.pid!; +} + describe("windowsPowerShellPath", () => { test("names Windows PowerShell under SystemRoot, whatever PATH holds", () => { expect(windowsPowerShellPath({ SystemRoot: "D:\\Win", PATH: "" })).toBe( diff --git a/packages/server/src/server/managed-processes/managed-processes.ts b/packages/server/src/server/managed-processes/managed-processes.ts index 43c1b3c810..1de767586f 100644 --- a/packages/server/src/server/managed-processes/managed-processes.ts +++ b/packages/server/src/server/managed-processes/managed-processes.ts @@ -148,13 +148,18 @@ class SystemManagedProcessTable implements ManagedProcessTable { return { status: "not-found" }; } + let inspection: ManagedProcessInspection; try { - return this.platform === "win32" - ? await this.inspectWindows(pid) - : await this.inspectPosix(pid); + inspection = + this.platform === "win32" ? await this.inspectWindows(pid) : await this.inspectPosix(pid); } catch (error) { - return { status: "error", error }; + inspection = { status: "error", error }; } + // 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" }; + return inspection; } private async inspectPosix(pid: number): Promise { @@ -452,6 +457,17 @@ function isProcessAlive(pid: number): boolean { } } +// 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 { + try { + process.kill(pid, 0); + return true; + } catch (error) { + return !isNodeErrorWithCode(error, "ESRCH"); + } +} + function isCommandExitFailure(error: unknown): boolean { // execFile rejects with a numeric `code` (the process exit status) when the // command ran and exited non-zero; a string `code` (e.g. "ENOENT") means it diff --git a/packages/server/src/terminal/terminal-worker-client.ts b/packages/server/src/terminal/terminal-worker-client.ts index 532f3075c6..ac48293879 100644 --- a/packages/server/src/terminal/terminal-worker-client.ts +++ b/packages/server/src/terminal/terminal-worker-client.ts @@ -1,4 +1,5 @@ -import { EventEmitter } from "node:events"; +import { EventEmitter, once } from "node:events"; +import { setTimeout as delay } from "node:timers/promises"; import { createConnection } from "node:net"; import { attachFramedSocket, type FramedSocket } from "./terminal-worker-frames.js"; import { @@ -139,3 +140,34 @@ export function connectTerminalWorker(input: { }); }); } + +export type TerminalWorkerSocketProbe = "accepting" | "refused" | "unknown"; + +const PROBE_TIMEOUT_MS = 1_000; +// What a connect to a socket nobody listens on fails with: the unix socket file is left behind +// (ECONNREFUSED) or gone, as a Windows pipe is once its server closes (ENOENT). +const NO_LISTENER_ERRORS = new Set(["ECONNREFUSED", "ENOENT"]); + +/** + * Whether anything accepts connections on a worker's socket. It never authenticates, so the + * daemon controlling the worker keeps control: the worker drops a connection that closes + * before its hello. + */ +export async function probeTerminalWorkerSocket( + socketPath: string, +): Promise { + const socket = createConnection(socketPath); + const timeout = new AbortController(); + try { + return await Promise.race([ + once(socket, "connect").then(() => "accepting" as const), + delay(PROBE_TIMEOUT_MS, "unknown" as const, { signal: timeout.signal }), + ]); + } catch (error) { + const code = (error as NodeJS.ErrnoException).code; + return code && NO_LISTENER_ERRORS.has(code) ? "refused" : "unknown"; + } finally { + timeout.abort(); + socket.destroy(); + } +} diff --git a/packages/server/src/terminal/terminal-worker-ownership.ts b/packages/server/src/terminal/terminal-worker-ownership.ts index 9bd02b01cd..82c8179b78 100644 --- a/packages/server/src/terminal/terminal-worker-ownership.ts +++ b/packages/server/src/terminal/terminal-worker-ownership.ts @@ -1,4 +1,6 @@ import type { ManagedProcessInspection } from "../server/managed-processes/managed-processes.js"; +import { probeTerminalWorkerSocket } from "./terminal-worker-client.js"; +import type { TerminalWorkerProcessInspector } from "./terminal-worker-processes.js"; import { TERMINAL_WORKER_MARKER_ARG, type TerminalWorkerRecord } from "./terminal-worker-record.js"; // Who a pid named in a terminal worker record belongs to, before anything signals it. @@ -15,10 +17,36 @@ export type ProcessOwnership = | { status: "owned" } | { status: "unknown"; reason: string }; +/** + * Who the pid a worker record names belongs to now. Only a matching birth time and worker + * marker make it owned, the one state cleanup may signal. When the process table cannot tell + * (the query failed, or the worker could not read its own identity), the worker's socket + * decides between unknown and unrelated: a live worker listens until it exits, so a refused + * connection means the pid now runs something else, or a worker already on its way out. + */ +export async function identifyTerminalWorker( + record: Pick, + inspector: Pick, +): Promise { + const ownership = classifyTerminalWorkerProcess(record, await inspector.inspect(record.pid)); + if (ownership.status !== "unknown") return ownership; + if ((await probeTerminalWorkerSocket(record.socketPath)) === "refused") { + return { status: "unrelated", reason: "nothing listens on its socket" }; + } + return ownership; +} + export function classifyTerminalWorkerProcess( record: Pick, inspection: ManagedProcessInspection, ): ProcessOwnership { + if ("unavailable" in record.identity) { + if (inspection.status === "not-found") return { status: "gone" }; + return { + status: "unknown", + reason: `the worker could not read its own identity: ${record.identity.unavailable}`, + }; + } const birth = classifyBirth(record.identity.startedAt, inspection); if (birth.status !== "owned" || inspection.status !== "alive") return birth; const commandLine = inspection.snapshot.commandLine; diff --git a/packages/server/src/terminal/terminal-worker-process.ts b/packages/server/src/terminal/terminal-worker-process.ts index 855c39da59..5378190494 100644 --- a/packages/server/src/terminal/terminal-worker-process.ts +++ b/packages/server/src/terminal/terminal-worker-process.ts @@ -16,6 +16,7 @@ import { terminalWorkerLogPath, terminalWorkerRecordPath, writeTerminalWorkerRecord, + type TerminalWorkerIdentity, type TerminalWorkerRecord, } from "./terminal-worker-record.js"; import { createTerminalWorkerRecordPublisher } from "./terminal-worker-record-publisher.js"; @@ -403,9 +404,10 @@ function runWorker(argv: string[]): void { ]); } - // A terminal enters the record only with its program's birth time, so cleanup can always - // tell the program from a later process that reuses its pid. A program that already exited - // needs no entry; one whose birth time cannot be read is killed and the create fails. + // A terminal enters the record with its program's birth time, so cleanup can tell the + // program from a later process that reuses its pid. A program that already exited needs no + // entry. One whose birth time cannot be read is recorded without it: it still runs, and + // cleanup never signals it. async function recordTerminalProgram(session: LocalTerminalSession): Promise { if (session.getExitInfo()) return; // The manager drops a session as soon as its program exits, so only a live one has a pid. @@ -417,10 +419,13 @@ function runWorker(argv: string[]): void { if (inspection.status === "not-found" || session.getExitInfo()) { return; } - if (inspection.status === "error" || !inspection.snapshot.startedAt) { + const startedAt = inspection.status === "alive" ? inspection.snapshot.startedAt : null; + if (!startedAt) { const reason = inspection.status === "error" ? errorMessage(inspection.error) : "no birth time reported"; - throw new Error(`Cannot identify terminal program ${pid}: ${reason}`); + console.error( + `Terminal worker cannot read the birth time of terminal program ${pid}; cleanup will never signal it: ${reason}`, + ); } if (!record) { throw new Error("Terminal worker record is not written yet"); @@ -429,12 +434,30 @@ function runWorker(argv: string[]): void { ...record, terminals: [ ...record.terminals.filter((entry) => entry.id !== session.id), - { id: session.id, pid, startedAt: inspection.snapshot.startedAt }, + { id: session.id, pid, startedAt }, ], }; await persistRecord(); } + // The birth time and command line cleanup matches before it signals this worker. Where they + // cannot be read the worker runs anyway, reachable only over its authenticated socket. + async function readOwnIdentity(): Promise { + const self = await inspector.inspect(process.pid); + if ( + self.status === "alive" && + self.snapshot.startedAt && + self.snapshot.commandLine?.includes(args.recordId) + ) { + return { startedAt: self.snapshot.startedAt, commandLine: self.snapshot.commandLine }; + } + const reason = self.status === "error" ? errorMessage(self.error) : "no usable snapshot"; + console.error( + `Terminal worker cannot read its own process identity; cleanup will never signal it: ${reason}`, + ); + return { unavailable: reason }; + } + function enqueueCreateTerminalRequest(message: TerminalCreateRequest): Promise { const nextRequest = createTerminalQueue.then(() => handleCreateTerminalRequest(message)); createTerminalQueue = nextRequest.catch(() => {}); @@ -711,21 +734,12 @@ function runWorker(argv: string[]): void { chmodSync(socketPath, PRIVATE_FILE_MODE); } - const self = await inspector.inspect(process.pid); - if ( - self.status !== "alive" || - !self.snapshot.startedAt || - !self.snapshot.commandLine?.includes(args.recordId) - ) { - const reason = self.status === "error" ? errorMessage(self.error) : "no usable snapshot"; - throw new Error(`Terminal worker cannot read its own process identity: ${reason}`); - } record = { recordId: args.recordId, pid: process.pid, bootId: currentBootId(), createdAt: new Date().toISOString(), - identity: { startedAt: self.snapshot.startedAt, commandLine: self.snapshot.commandLine }, + identity: await readOwnIdentity(), protocolVersion, paseoVersion: args.paseoVersion, socketPath, diff --git a/packages/server/src/terminal/terminal-worker-reaper.test.ts b/packages/server/src/terminal/terminal-worker-reaper.test.ts index a20d571b7f..b691afcbf7 100644 --- a/packages/server/src/terminal/terminal-worker-reaper.test.ts +++ b/packages/server/src/terminal/terminal-worker-reaper.test.ts @@ -643,3 +643,102 @@ describe.skipIf(isPlatform("win32"))("terminal worker reaper", () => { }); }); }); + +// #8766 and #8784: on these Windows machines every Windows PowerShell query fails. The +// inspector runs the Windows queries (no session walk) against real local processes. Cleanup +// signals nothing it cannot identify, and still retires what is gone. +describe.skipIf(isPlatform("win32"))("terminal worker reaper without process identity", () => { + const noIdentity = createTerminalWorkerProcessInspector({ + platform: "win32", + commandRunner: { exec: () => Promise.reject(new Error("spawn UNKNOWN")) }, + }); + + // A live process standing in for a worker that cannot answer: it listens on the record's + // socket and never replies to the hello. + async function spawnSilentListener(socketPath: string): Promise { + const listener = spawnIdle(` + require("node:net").createServer(() => {}).listen(${JSON.stringify(socketPath)}, () => { + process.stdout.write("listening\\n"); + }); + `); + await new Promise((resolve) => listener.stdout!.once("data", resolve)); + return listener; + } + + it("retires the record of a gone worker and its gone programs without the identity query", async () => { + const home = makeHome(); + const recordPath = writeRecord(home, { + pid: await goneProcessPid(), + identity: { unavailable: "spawn UNKNOWN" }, + terminals: [{ id: "t1", pid: await goneProcessPid(), startedAt: null }], + }); + + const result = await reapTerminalWorkers({ paseoHome: home, inspector: noIdentity }); + + expect(result).toEqual({ records: 1, retired: 1, signaled: [] }); + expect(existsSync(recordPath)).toBe(false); + }); + + it("never signals a live worker it cannot identify that does not answer, and keeps the cleanup unconfirmed", async () => { + const home = makeHome(); + const socketPath = path.join(home, "w.sock"); + const listener = await spawnSilentListener(socketPath); + writeRecord(home, { + pid: listener.pid!, + identity: { unavailable: "spawn UNKNOWN" }, + socketPath, + }); + + await expect( + reapTerminalWorkers({ paseoHome: home, inspector: noIdentity, gracefulMs: 300 }), + ).rejects.toBeInstanceOf(TerminalCleanupNotConfirmedError); + + expect(isAlive(listener.pid!)).toBe(true); + expect(await readUnconfirmedTerminalCleanup(home)).toEqual([ + expect.objectContaining({ + pid: listener.pid!, + role: "worker", + reason: "inspection failed", + detail: expect.stringContaining("could not read its own identity: spawn UNKNOWN"), + }), + ]); + // The next stop or start retries it the same way. + await expect(retryUnconfirmedTerminalCleanup(home)).rejects.toBeInstanceOf( + TerminalCleanupNotConfirmedError, + ); + expect(isAlive(listener.pid!)).toBe(true); + }); + + it("treats a live pid with nothing on the worker's socket as another process, signals it never, and retires the record", async () => { + const home = makeHome(); + const reusedWorkerPid = spawnIdle().pid!; + const recordPath = writeRecord(home, { + pid: reusedWorkerPid, + identity: { unavailable: "spawn UNKNOWN" }, + }); + + const result = await reapTerminalWorkers({ paseoHome: home, inspector: noIdentity }); + + expect(result).toEqual({ records: 1, retired: 1, signaled: [] }); + expect(isAlive(reusedWorkerPid)).toBe(true); + expect(existsSync(recordPath)).toBe(false); + }); + + it("never signals a live terminal program recorded without a birth time", async () => { + const home = makeHome(); + const program = spawnIdle(); + writeRecord(home, { + pid: await goneProcessPid(), + terminals: [{ id: "t1", pid: program.pid!, startedAt: null }], + }); + + await expect( + reapTerminalWorkers({ paseoHome: home, inspector, confirmMs: 200 }), + ).rejects.toBeInstanceOf(TerminalCleanupNotConfirmedError); + + expect(isAlive(program.pid!)).toBe(true); + expect(await readUnconfirmedTerminalCleanup(home)).toEqual([ + expect.objectContaining({ pid: program.pid!, role: "terminal program" }), + ]); + }); +}); diff --git a/packages/server/src/terminal/terminal-worker-reaper.ts b/packages/server/src/terminal/terminal-worker-reaper.ts index 2647d4b25e..e29f5da97a 100644 --- a/packages/server/src/terminal/terminal-worker-reaper.ts +++ b/packages/server/src/terminal/terminal-worker-reaper.ts @@ -6,7 +6,7 @@ import { connectTerminalWorker } from "./terminal-worker-client.js"; import { pruneTerminalWorkerRuntimes } from "./terminal-worker-runtime.js"; import { classifyTerminalProgramProcess, - classifyTerminalWorkerProcess, + identifyTerminalWorker, type ProcessOwnership, } from "./terminal-worker-ownership.js"; import { @@ -38,7 +38,8 @@ import { // program's session joins only when its environment carries that terminal's id. When the // 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 gets an authenticated `shutdown`: it kills each terminal and waits for it. +// 2. An owned worker, or one its process cannot identify, gets an authenticated `shutdown`: it +// kills each terminal and waits for 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 @@ -161,7 +162,7 @@ export interface ReapTerminalWorkersOptions { interface Candidate { pid: number; - startedAt: string; + startedAt: string | null; role: UnresolvedTerminalProcess["role"]; terminalId: string; state: ProcessOwnership; @@ -252,7 +253,7 @@ async function reapRecord( return { signaled: [], unresolved: [] }; } - const worker = classifyTerminalWorkerProcess(record, await inspector.inspect(record.pid)); + const worker = await identifyTerminalWorker(record, inspector); const discovery = await collectCandidates(record, inspector, log); const tracked: TrackedProcesses = { record, @@ -265,13 +266,14 @@ async function reapRecord( return await keepUnresolved(input, tracked, [], `not signaled: ${discovery.tableError}`); } - if (tracked.worker.status === "owned" && input.gracefulMs > 0) { + // 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); } - await waitUntilSettled( - tracked, - Date.now() + (tracked.worker.status === "owned" ? input.gracefulMs : 0), - ); + await waitUntilSettled(tracked, Date.now() + (askShutdown ? input.gracefulMs : 0)); let signaled: number[] = []; if (!isSettled(tracked)) { @@ -337,10 +339,7 @@ function isSettled(tracked: TrackedProcesses): boolean { async function refreshTracked(tracked: TrackedProcesses): Promise { if (isOpen(tracked.worker)) { - tracked.worker = classifyTerminalWorkerProcess( - tracked.record, - await tracked.inspector.inspect(tracked.record.pid), - ); + tracked.worker = await identifyTerminalWorker(tracked.record, tracked.inspector); } for (const candidate of tracked.candidates) { if (isOpen(candidate.state)) { @@ -369,7 +368,7 @@ async function forceKillOwned( const { record, inspector } = tracked; const signaled: number[] = []; if (tracked.worker.status === "owned") { - tracked.worker = classifyTerminalWorkerProcess(record, await inspector.inspect(record.pid)); + tracked.worker = await identifyTerminalWorker(record, inspector); if (tracked.worker.status === "owned") { await forceKillWorker(record.pid); signaled.push(record.pid); diff --git a/packages/server/src/terminal/terminal-worker-record.ts b/packages/server/src/terminal/terminal-worker-record.ts index 3176d2ee8e..49f8a7db50 100644 --- a/packages/server/src/terminal/terminal-worker-record.ts +++ b/packages/server/src/terminal/terminal-worker-record.ts @@ -7,13 +7,26 @@ import { writePrivateFileAtomicSync, writePrivateFileAtomic } from "../server/pr // Owner-only metadata a detached terminal worker leaves in $PASEO_HOME so the next daemon can // find, authenticate and adopt it, and so cleanup can identify every process it is allowed to // signal. Process identity is the birth time `ps`/Win32_Process report, never the pid alone. +// Identity is best effort: where the query cannot run, the worker still records itself and +// its terminals, and cleanup never signals what it cannot identify. const TerminalWorkerTerminalEntrySchema = z.object({ id: z.string().min(1), pid: z.number().int().positive(), - startedAt: z.string().min(1), + // Null when the program's birth time could not be read: it is never signaled. + startedAt: z.string().min(1).nullable(), }); +const TerminalWorkerIdentitySchema = z.union([ + z.object({ + startedAt: z.string().min(1), + commandLine: z.string().min(1), + }), + // Why the worker could not read its own identity. Such a worker is driven and shut down only + // over its authenticated socket. + z.object({ unavailable: z.string().min(1) }), +]); + // One process a cleanup could not confirm gone, kept so a later caller (a retry, or a remote // client reading daemon status) can name it. const TerminalCleanupUnresolvedSchema = z.object({ @@ -28,10 +41,7 @@ const TerminalWorkerRecordSchema = z.object({ pid: z.number().int().positive(), bootId: z.string().nullable(), createdAt: z.string().min(1), - identity: z.object({ - startedAt: z.string().min(1), - commandLine: z.string().min(1), - }), + identity: TerminalWorkerIdentitySchema, // Read loosely: a version this daemon cannot interpret marks a worker from a newer Paseo // (classifyTerminalWorkerProtocol), not an unreadable record. protocolVersion: z.union([z.number(), z.string()]), @@ -48,6 +58,7 @@ const TerminalWorkerRecordSchema = z.object({ }); export type TerminalWorkerTerminalEntry = z.infer; +export type TerminalWorkerIdentity = z.infer; export type TerminalCleanupUnresolved = z.infer; export type TerminalWorkerRecord = z.infer; diff --git a/packages/server/src/terminal/worker-terminal-manager.test.ts b/packages/server/src/terminal/worker-terminal-manager.test.ts index 0bcc2acf92..72f0ae6692 100644 --- a/packages/server/src/terminal/worker-terminal-manager.test.ts +++ b/packages/server/src/terminal/worker-terminal-manager.test.ts @@ -62,15 +62,21 @@ import { readTerminalWorkerRecord, terminalWorkerDirectory, terminalWorkerRecordPath, + writeTerminalWorkerRecord, type TerminalWorkerRecord, } from "./terminal-worker-record.js"; -import { TERMINAL_WORKER_PROTOCOL_OVERRIDE_ENV } from "./terminal-worker-protocol.js"; +import { + TERMINAL_WORKER_PROTOCOL_OVERRIDE_ENV, + TERMINAL_WORKER_PROTOCOL_VERSION, +} from "./terminal-worker-protocol.js"; import { NoTerminalWorkerAvailableError, TerminalWorkerIncompatibleError, } from "./terminal-worker-pool.js"; import { connectTerminalWorker } from "./terminal-worker-client.js"; -import { launchTerminalWorker, TerminalWorkerLaunchError } from "./terminal-worker-launch.js"; +import { launchTerminalWorker } from "./terminal-worker-launch.js"; +import { currentBootId } from "../server/pid-lock.js"; +import { spawn } from "node:child_process"; type TerminalRow = TerminalState["grid"][number]; @@ -2672,33 +2678,176 @@ it.each([false, true])( 45000, ); -// Both Windows reports in #8766: the worker's process identity query (PowerShell there, `ps` here) -// fails, the worker exits before writing its record, and the daemon used to exit with it. +// 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 +// does, not the way `ps` reports a missing pid. +function withoutProcessIdentity(): () => void { + const binDir = mkdtempSync(join(tmpdir(), "worker-terminal-manager-no-ps-")); + temporaryDirs.push(binDir); + writeFileSync(join(binDir, "ps"), "#!/bin/sh\necho 'ps: unavailable' >&2\nkill -KILL $$\n", { + mode: 0o755, + }); + const originalPath = process.env.PATH; + process.env.PATH = `${binDir}${delimiter}${originalPath ?? ""}`; + return () => { + process.env.PATH = originalPath; + }; +} + +function echoTerminalCommand(prefix: string) { + return nodeTerminalCommand(` + process.stdin.on("data", (chunk) => { + process.stdout.write(${JSON.stringify(prefix)} + chunk.toString()); + }); + setInterval(() => {}, 1000); + `); +} + +async function expectEcho(session: TerminalSession, prefix: string, input: string): Promise { + const messages: string[] = []; + const unsubscribe = session.subscribe((message) => { + if (message.type === "output") messages.push(message.data); + }); + session.send({ type: "input", data: `${input}\r` }); + try { + await waitForCondition(() => messages.join("").includes(`${prefix}${input}`), 10000); + } finally { + unsubscribe(); + } +} + it.skipIf(isPlatform("win32"))( - "starts without a worker when the worker cannot read its own identity, and launches one once it can", + "runs terminals when no process identity can be read, recording the identity as unavailable", async () => { const paseoHome = createTemporaryHome(); - const binDir = mkdtempSync(join(tmpdir(), "worker-terminal-manager-no-ps-")); - temporaryDirs.push(binDir); - writeFileSync(join(binDir, "ps"), "#!/bin/sh\nexit 1\n", { mode: 0o755 }); - const originalPath = process.env.PATH; - process.env.PATH = `${binDir}${delimiter}${originalPath ?? ""}`; + const restorePath = withoutProcessIdentity(); try { manager = await startRealWorkerManager({ paseoHome }); - expect(await workerRecords(paseoHome)).toEqual([]); - await expect( - manager.createTerminal({ workspaceId: "ws-no-identity", cwd: paseoHome }), - ).rejects.toThrow(TerminalWorkerLaunchError); - expect(await workerRecords(paseoHome)).toEqual([]); - } finally { - process.env.PATH = originalPath; - } + const session = trackTerminal( + await manager.createTerminal({ + workspaceId: "ws-no-identity", + cwd: paseoHome, + ...echoTerminalCommand("no-identity:"), + }), + ); + await expectEcho(session, "no-identity:", "hello"); - const terminal = trackTerminal( - await manager.createTerminal({ workspaceId: "ws-no-identity", cwd: paseoHome }), - ); - expect(terminal.id).toBeTruthy(); - expect((await workerRecords(paseoHome)).map((record) => record.pid)).toHaveLength(1); + const [record, ...others] = await workerRecords(paseoHome); + expect(others).toEqual([]); + expect(record!.identity).toEqual({ + unavailable: expect.stringContaining("ps: unavailable"), + }); + expect(record!.terminals).toEqual([ + { id: session.id, pid: expect.any(Number), startedAt: null }, + ]); + } finally { + restorePath(); + } + }, + 30000, +); + +it.skipIf(isPlatform("win32"))( + "adopts the terminals of a worker whose identity is unavailable after a daemon restart", + async () => { + const paseoHome = createTemporaryHome(); + const restorePath = withoutProcessIdentity(); + try { + const first = await startRealWorkerManager({ paseoHome }); + const created = await first.createTerminal({ + workspaceId: "ws-no-identity", + cwd: paseoHome, + ...echoTerminalCommand("adopted:"), + }); + await first.detach(); + + manager = await startRealWorkerManager({ paseoHome }); + + const adopted = manager.getTerminal(created.id); + expect(adopted).toBeDefined(); + await expectEcho(adopted!, "adopted:", "after-restart"); + expect(await workerRecords(paseoHome)).toHaveLength(1); + } finally { + restorePath(); + } + }, + 30000, +); + +it.skipIf(isPlatform("win32"))( + "closes the terminals and the worker on a stop without signaling, when no identity can be read", + async () => { + const paseoHome = createTemporaryHome(); + const restorePath = withoutProcessIdentity(); + try { + const stopping = await startRealWorkerManager({ paseoHome }); + manager = stopping; + await stopping.createTerminal({ + workspaceId: "ws-no-identity", + cwd: paseoHome, + ...nodeTerminalCommand("setInterval(() => {}, 1000);"), + }); + const [record] = await workerRecords(paseoHome); + const programPid = record!.terminals[0]!.pid; + + await stopping.closeTerminals(); + + // The worker closed its terminals before it answered; it stops listening as it exits. + 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 () => { + const paseoHome = createTemporaryHome(); + // One worker died; another died and its pid now runs an unrelated process, so nothing + // listens on its socket. Neither may count against the two-worker limit or be signaled. + const exited = spawn(process.execPath, ["-e", ""], { stdio: "ignore" }); + await new Promise((resolve) => exited.once("exit", resolve)); + const reused = spawn(process.execPath, ["-e", "setInterval(() => {}, 1e9)"], { + stdio: "ignore", + }); + try { + for (const pid of [exited.pid!, reused.pid!]) { + writeTerminalWorkerRecord(paseoHome, { + recordId: `dead-worker-${pid}`, + pid, + bootId: currentBootId(), + createdAt: new Date().toISOString(), + identity: { unavailable: "spawn UNKNOWN" }, + protocolVersion: TERMINAL_WORKER_PROTOCOL_VERSION, + paseoVersion: "0.0.0-test", + socketPath: join(paseoHome, `gone-${pid}.sock`), + secret: "secret-of-a-gone-worker", + terminals: [], + }); + } + const restorePath = withoutProcessIdentity(); + try { + manager = await startRealWorkerManager({ paseoHome }); + trackTerminal( + await manager.createTerminal({ workspaceId: "ws-no-identity", cwd: paseoHome }), + ); + } finally { + restorePath(); + } + + const records = await workerRecords(paseoHome); + expect(records.map((record) => record.pid)).not.toContain(exited.pid); + expect(records.map((record) => record.pid)).not.toContain(reused.pid); + expect(records).toHaveLength(1); + expect(isProcessAlive(reused.pid!)).toBe(true); + } finally { + reused.kill("SIGKILL"); + } }, 30000, ); diff --git a/packages/server/src/terminal/worker-terminal-manager.ts b/packages/server/src/terminal/worker-terminal-manager.ts index ea91d54a66..19a581dc26 100644 --- a/packages/server/src/terminal/worker-terminal-manager.ts +++ b/packages/server/src/terminal/worker-terminal-manager.ts @@ -21,7 +21,7 @@ import { resolvePaseoCliExecutablePath } from "./paseo-cli-path.js"; import type { CaptureTerminalLinesResult } from "./terminal-capture.js"; import { connectTerminalWorker, type TerminalWorkerConnection } from "./terminal-worker-client.js"; import { launchTerminalWorker } from "./terminal-worker-launch.js"; -import { classifyTerminalWorkerProcess } from "./terminal-worker-ownership.js"; +import { identifyTerminalWorker } from "./terminal-worker-ownership.js"; import { createTerminalWorkerProcessInspector, type TerminalWorkerProcessInspector, @@ -1038,18 +1038,21 @@ export async function startWorkerTerminalManager( toReap.push(file.path); continue; } - const worker = classifyTerminalWorkerProcess(record, await inspector.inspect(record.pid)); - if (worker.status === "owned") { + const worker = await identifyTerminalWorker(record, inspector); + if (worker.status === "owned" || worker.status === "unknown") { + if (worker.status === "unknown") { + // Its secret still proves it: only the worker that wrote the record can answer the + // hello below. Cleanup will not signal it, only ask it to shut down. + logger.warn( + { recordPath: file.path, pid: record.pid, reason: worker.reason }, + "Cannot identify terminal worker by its process; reaching it over its socket", + ); + } live.push({ path: file.path, record, fit: classifyTerminalWorkerProtocol(record.protocolVersion), }); - } else if (worker.status === "unknown") { - logger.warn( - { recordPath: file.path, pid: record.pid, reason: worker.reason }, - "Cannot identify terminal worker; leaving its record", - ); } else { // Its programs lost their PTY with the worker and cannot be adopted. toReap.push(file.path); @@ -1142,10 +1145,7 @@ export async function startWorkerTerminalManager( const reconnect = () => { recovery ??= (async () => { while (!recoveryState.stopped) { - const ownership = classifyTerminalWorkerProcess( - record, - await inspector.inspect(record.pid), - ); + const ownership = await identifyTerminalWorker(record, inspector); if (ownership.status === "gone" || ownership.status === "unrelated") { await reapTerminalWorkers({ paseoHome: options.paseoHome, @@ -1273,8 +1273,8 @@ export async function startWorkerTerminalManager( logger.warn(error.message); } else { // The daemon starts without terminals rather than not at all. A worker that cannot start - // here (its process identity query failed, say) is launched again by the first terminal - // creation, which reports the same error if it still cannot. + // here is launched again by the first terminal creation, which reports the same error + // if it still cannot. logger.error( { err: error }, "Terminal worker did not start; terminals are unavailable until one starts", @@ -1298,7 +1298,8 @@ export async function startWorkerTerminalManager( // Every worker that may still run in this home, one per record: a worker and its cleanup record // are listed as one. Unreadable records and workers that cannot be identified count; a worker -// confirmed gone, or a pid reused by another process, does not. +// confirmed gone, or a pid now running something else (another birth time, or nothing listening +// on the worker's socket), does not. async function countRunningTerminalWorkers( paseoHome: string, inspector: TerminalWorkerProcessInspector, @@ -1311,7 +1312,7 @@ async function countRunningTerminalWorkers( } const { record } = file; if (!writtenDuringThisBoot({ startedAt: record.createdAt, bootId: record.bootId })) continue; - const worker = classifyTerminalWorkerProcess(record, await inspector.inspect(record.pid)); + const worker = await identifyTerminalWorker(record, inspector); if (worker.status === "owned" || worker.status === "unknown") running += 1; } return running;