mirror of
https://github.com/getpaseo/paseo.git
synced 2026-11-06 00:14:54 +00:00
fix(terminal): ask unidentified workers to shut down on every cleanup
Retries of an unconfirmed cleanup and startup cleanup run without a grace period, which skipped the shutdown request. For a worker whose identity is unavailable that request is the only way to stop it, so it is now always sent. A gone program recorded without a birth time is no longer walked through the process table: the table was unreadable when it started, and the walk only kept the cleanup unconfirmed. Also test the socket probe on every platform, restore a test of the daemon starting when the worker cannot, and keep one pid liveness helper.
This commit is contained in:
@@ -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;
|
||||
|
||||
@@ -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<void>((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<void>((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<Server> {
|
||||
const server = createServer((socket) => socket.on("error", () => {}));
|
||||
await new Promise<void>((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");
|
||||
});
|
||||
});
|
||||
|
||||
@@ -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();
|
||||
|
||||
@@ -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 };
|
||||
|
||||
|
||||
@@ -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 () => {
|
||||
|
||||
Reference in New Issue
Block a user