From 38d9984eed1e53b7f38f24d1a74003b350fcd277 Mon Sep 17 00:00:00 2001 From: Julius Marminge <51714798+juliusmarminge@users.noreply.github.com> Date: Wed, 30 Sep 2026 09:52:42 -0700 Subject: [PATCH] fix(server): process-group kills never reach every process the user owns A fake spawner in tests reports pid 1, and the OpenCode runtime cleans up with process.kill(-pid). That became kill(-1, SIGKILL), which signals every process the user owns: running the provider tests killed the T3 server, cloudflared and every agent on the machine. signalProcessGroup refuses a pid of 0, 1 or a non-integer with ESRCH, like a group that already exited, and every server process-group signal (OpenCode runtime and server ledger, ACP, Pi) goes through it. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../src/orchestration-v2/Adapters/PiRpc.ts | 7 ++- apps/server/src/process/processGroup.test.ts | 45 +++++++++++++++++++ apps/server/src/process/processGroup.ts | 16 +++++++ .../src/provider/OpenCodeServerLedger.ts | 5 ++- .../src/provider/acp/AcpSessionRuntime.ts | 3 +- apps/server/src/provider/opencodeRuntime.ts | 5 ++- 6 files changed, 74 insertions(+), 7 deletions(-) create mode 100644 apps/server/src/process/processGroup.test.ts create mode 100644 apps/server/src/process/processGroup.ts diff --git a/apps/server/src/orchestration-v2/Adapters/PiRpc.ts b/apps/server/src/orchestration-v2/Adapters/PiRpc.ts index 9b1da49e027b..bc6d09d6b421 100644 --- a/apps/server/src/orchestration-v2/Adapters/PiRpc.ts +++ b/apps/server/src/orchestration-v2/Adapters/PiRpc.ts @@ -30,6 +30,8 @@ import { ChildProcess, ChildProcessSpawner } from "effect/unstable/process"; import { HostProcessPlatform } from "@t3tools/shared/hostProcess"; import { resolveSpawnCommand } from "@t3tools/shared/shell"; +import { signalProcessGroup } from "../../process/processGroup.ts"; + export class PiRpcError extends Schema.TaggedError()("PiRpcError", { operation: Schema.String, detail: Schema.optional(Schema.String), @@ -238,7 +240,7 @@ export const makePiRpcConnection = Effect.fnUntraced(function* (options: PiRpcSp if (platform === "win32") { process.kill(Number(child.pid), signal); } else { - process.kill(-Number(child.pid), signal); + signalProcessGroup(Number(child.pid), signal); } return true; } catch { @@ -250,7 +252,8 @@ export const makePiRpcConnection = Effect.fnUntraced(function* (options: PiRpcSp const hasExited = (): boolean => { if (childExited) return true; try { - process.kill(platform === "win32" ? Number(child.pid) : -Number(child.pid), 0); + if (platform === "win32") process.kill(Number(child.pid), 0); + else signalProcessGroup(Number(child.pid), 0); return false; } catch { return true; diff --git a/apps/server/src/process/processGroup.test.ts b/apps/server/src/process/processGroup.test.ts new file mode 100644 index 000000000000..9694d9b7ab94 --- /dev/null +++ b/apps/server/src/process/processGroup.test.ts @@ -0,0 +1,45 @@ +// @effect-diagnostics nodeBuiltinImport:off +import * as NodeChildProcess from "node:child_process"; + +import { describe, expect, it } from "@effect/vitest"; +import { HostProcessPlatform } from "@t3tools/shared/hostProcess"; + +import { signalProcessGroup } from "./processGroup.ts"; + +const errorCode = (run: () => void) => { + try { + run(); + return undefined; + } catch (cause) { + return (cause as NodeJS.ErrnoException).code; + } +}; + +describe.skipIf(HostProcessPlatform.defaultValue() === "win32")("signalProcessGroup", () => { + // Signal 0 only probes, so this is safe to run unguarded: `kill(-1, 0)` and + // `kill(0, 0)` succeed, which is how a fake spawner's pid 1 became + // `kill(-1, SIGKILL)` and took down every process the user owned. + it("never reaches this server's group or every process the user owns", () => { + for (const pid of [1, 0, -1, Number.NaN, 1.5]) { + expect( + errorCode(() => signalProcessGroup(pid, 0)), + `pid ${pid}`, + ).toBe("ESRCH"); + } + }); + + it("signals a spawned process group", async () => { + const child = NodeChildProcess.spawn("/bin/sh", ["-c", "sleep 600 & wait"], { + detached: true, + stdio: "ignore", + }); + const exited = new Promise((resolve) => + child.once("exit", (_code, signal) => resolve(signal)), + ); + const pid = child.pid!; + + expect(errorCode(() => signalProcessGroup(pid, 0))).toBeUndefined(); + signalProcessGroup(pid, "SIGKILL"); + expect(await exited).toBe("SIGKILL"); + }); +}); diff --git a/apps/server/src/process/processGroup.ts b/apps/server/src/process/processGroup.ts new file mode 100644 index 000000000000..ee501e0fdd0b --- /dev/null +++ b/apps/server/src/process/processGroup.ts @@ -0,0 +1,16 @@ +/** + * Signals a POSIX process group this server spawned, as `process.kill(-pid)`. + * + * `kill(0)` signals this server's own group and `kill(-1)` every process the + * user owns, and a fake spawner in tests reports pid 1. So a pid of 0 or 1, or + * one that is not an integer, fails with ESRCH like a group that has already + * exited, and callers keep their existing handling. + */ +export function signalProcessGroup(pid: number, signal: NodeJS.Signals | 0): void { + if (!Number.isSafeInteger(pid) || pid <= 1) { + throw Object.assign(new Error(`kill ESRCH: not a spawned process group (${pid})`), { + code: "ESRCH", + }); + } + process.kill(-pid, signal); +} diff --git a/apps/server/src/provider/OpenCodeServerLedger.ts b/apps/server/src/provider/OpenCodeServerLedger.ts index 6ae07ba9e789..654e4c0c2889 100644 --- a/apps/server/src/provider/OpenCodeServerLedger.ts +++ b/apps/server/src/provider/OpenCodeServerLedger.ts @@ -9,6 +9,7 @@ import * as Schema from "effect/Schema"; import { ChildProcess, ChildProcessSpawner } from "effect/unstable/process"; import * as ServerConfig from "../config.ts"; +import { signalProcessGroup } from "../process/processGroup.ts"; const ProcessIdentity = Schema.Struct({ pid: Schema.Int, startTime: Schema.String }); type ProcessIdentity = typeof ProcessIdentity.Type; @@ -91,7 +92,7 @@ const parseDarwinPs = (output: string): ReadonlyArray => const signalGroup = (pgid: number, signal: NodeJS.Signals) => { try { - process.kill(-pgid, signal); + signalProcessGroup(pgid, signal); } catch { // The group may already be gone. } @@ -99,7 +100,7 @@ const signalGroup = (pgid: number, signal: NodeJS.Signals) => { const groupExists = (pgid: number) => { try { - process.kill(-pgid, 0); + signalProcessGroup(pgid, 0); return true; } catch (cause) { return (cause as NodeJS.ErrnoException | undefined)?.code !== "ESRCH"; diff --git a/apps/server/src/provider/acp/AcpSessionRuntime.ts b/apps/server/src/provider/acp/AcpSessionRuntime.ts index 50438c274137..dbb78eec8031 100644 --- a/apps/server/src/provider/acp/AcpSessionRuntime.ts +++ b/apps/server/src/provider/acp/AcpSessionRuntime.ts @@ -29,6 +29,7 @@ import type * as EffectAcpProtocol from "effect-acp/protocol"; import { resolveSpawnCommand } from "@t3tools/shared/shell"; import { HostProcessPlatform } from "@t3tools/shared/hostProcess"; +import { signalProcessGroup } from "../../process/processGroup.ts"; import { appendAcpStderrTail, sanitizeAcpStderrExcerpt } from "./AcpStderr.ts"; import { collectSessionConfigOptionValues, @@ -1647,7 +1648,7 @@ export const make = ( const signalOwnedProcessGroup = (signal: NodeJS.Signals) => Effect.try({ try: () => { - process.kill(-Number(child.pid), signal); + signalProcessGroup(Number(child.pid), signal); return true; }, catch: (cause) => diff --git a/apps/server/src/provider/opencodeRuntime.ts b/apps/server/src/provider/opencodeRuntime.ts index 192a15076a9a..f1dfda3e6abd 100644 --- a/apps/server/src/provider/opencodeRuntime.ts +++ b/apps/server/src/provider/opencodeRuntime.ts @@ -30,6 +30,7 @@ import * as Schema from "effect/Schema"; import * as Stream from "effect/Stream"; import { ChildProcess, ChildProcessSpawner } from "effect/unstable/process"; +import { signalProcessGroup } from "../process/processGroup.ts"; import { isWindowsCommandNotFound } from "../processRunner.ts"; import * as OpenCodeServerLedger from "./OpenCodeServerLedger.ts"; import { collectStreamAsString } from "./providerSnapshot.ts"; @@ -609,7 +610,7 @@ const makeOpenCodeRuntime = Effect.gen(function* () { ? child.kill({ killSignal: "SIGKILL" }).pipe(Effect.asVoid) : Effect.sync(() => { try { - process.kill(-Number(child.pid), "SIGKILL"); + signalProcessGroup(Number(child.pid), "SIGKILL"); } catch { // The command and its process group may already have exited. } @@ -731,7 +732,7 @@ const makeOpenCodeRuntime = Effect.gen(function* () { ? child.kill({ killSignal: signal, forceKillAfter: "1 second" }).pipe(Effect.asVoid) : Effect.sync(() => { try { - process.kill(-Number(child.pid), signal); + signalProcessGroup(Number(child.pid), signal); } catch { // The direct child may already have exited after starting the // server; the process group kill is best-effort cleanup for