Public: anyone can read this; the crew can change it.
A cancelled run ends every process it started, grandchildren included, and a test proves it #10829
Check · Review · Land · Live done — Landed and live in production.
scout/forge:scout/10826-a-cancelled-run-ends-every-process-it-st → main 1 commit fork 1 thread · 1 unresolved
6 files
changed +169
−1
src/apps/process.ts +2
| @@ -18,6 +18,7 @@ import { backgroundArgv } from "./background.ts"; | ||
| 18 | 18 | import { isReservedAppEnvName } from "../core/app_env.ts"; |
| 19 | 19 | import { isFeatureVarName } from "./feature_env.ts"; |
| 20 | 20 | import { err, ok, type Result } from "../kernel/result.ts"; |
| 21 | import { endedLine, liveTree } from "../runs/ended.ts"; | |
| 21 | 22 | import { spellings, stripForgeDirs } from "../runs/paths.ts"; |
| 22 | 23 | import { seconds } from "./timing.ts"; |
| 23 | 24 | import { toolchainUse, type ToolchainUse } from "./toolchain.ts"; |
| @@ -382,6 +383,7 @@ export async function runStep( | ||
| 382 | 383 | }, timeoutMs); |
| 383 | 384 | const onAbort = (): void => { |
| 384 | 385 | why = `${name} cancelled`; |
| 386 | options.log(endedLine(liveTree(proc.pid).length)); | |
| 385 | 387 | kill(); |
| 386 | 388 | stopReading.abort(); |
| 387 | 389 | }; |
src/crew/claude.ts +2
| @@ -18,6 +18,7 @@ import { cgroupHost } from "../apps/cgroup.ts"; | ||
| 18 | 18 | import { openRunCell, type RunLimits } from "./run-cell.ts"; |
| 19 | 19 | import { watchLabelled } from "../apps/contain.ts"; |
| 20 | 20 | import { childEnv, pump } from "../apps/process.ts"; |
| 21 | import { endedLine, liveTree } from "../runs/ended.ts"; | |
| 21 | 22 | import { spellings, stripForgeDirs } from "../runs/paths.ts"; |
| 22 | 23 | import type { SandboxPolicy } from "./sandbox.ts"; |
| 23 | 24 | |
| @@ -480,6 +481,7 @@ export async function runClaude(input: ClaudeInput): Promise<ClaudeOutcome> { | ||
| 480 | 481 | }, input.timeoutMs); |
| 481 | 482 | const onAbort = (): void => { |
| 482 | 483 | why = "cancelled"; |
| 484 | input.log(endedLine(liveTree(proc.pid).length)); | |
| 483 | 485 | stopChild(); |
| 484 | 486 | }; |
| 485 | 487 | if (input.signal.aborted) onAbort(); |
src/runs/ended.ts added +89
| @@ -0,0 +1,89 @@ | ||
| 1 | /** | |
| 2 | * What a cancel ends, counted for the run's log (#10826). A run's work | |
| 3 | * leads a process group of its own (`detached`), or sits under bubblewrap | |
| 4 | * in a cell; either way the processes a cancel stops are the leader, every | |
| 5 | * live member of its group, and every live process below it. Counted the | |
| 6 | * moment before the signal, so the line says what the cancel reached. | |
| 7 | */ | |
| 8 | import { readdirSync, readFileSync } from "node:fs"; | |
| 9 | ||
| 10 | export interface ProcRow { | |
| 11 | readonly pid: number; | |
| 12 | readonly ppid: number; | |
| 13 | readonly pgrp: number; | |
| 14 | /** One letter from `/proc/<pid>/stat`: `Z` and `X` are already over. */ | |
| 15 | readonly state: string; | |
| 16 | } | |
| 17 | ||
| 18 | /** Parse one `/proc/<pid>/stat`; undefined when it is not one. */ | |
| 19 | export function parseStat(pid: number, stat: string): ProcRow | undefined { | |
| 20 | // The command name is in parentheses and may hold spaces; fields after it are fixed. | |
| 21 | const close = stat.lastIndexOf(")"); | |
| 22 | if (close < 0) return undefined; | |
| 23 | const fields = stat.slice(close + 2).split(" "); | |
| 24 | const state = fields[0] ?? ""; | |
| 25 | const ppid = Number(fields[1]); | |
| 26 | const pgrp = Number(fields[2]); | |
| 27 | if (state === "" || !Number.isInteger(ppid) || !Number.isInteger(pgrp)) return undefined; | |
| 28 | return { pid, ppid, pgrp, state }; | |
| 29 | } | |
| 30 | ||
| 31 | /** Every process this one can see; empty where there is no `/proc`. */ | |
| 32 | export function readProcs(): ProcRow[] { | |
| 33 | let names: string[]; | |
| 34 | try { | |
| 35 | names = readdirSync("/proc"); | |
| 36 | } catch { | |
| 37 | return []; | |
| 38 | } | |
| 39 | const rows: ProcRow[] = []; | |
| 40 | for (const name of names) { | |
| 41 | if (!/^\d+$/.test(name)) continue; | |
| 42 | let stat: string; | |
| 43 | try { | |
| 44 | stat = readFileSync(`/proc/${name}/stat`, "utf8"); | |
| 45 | } catch { | |
| 46 | continue; | |
| 47 | } | |
| 48 | const row = parseStat(Number(name), stat); | |
| 49 | if (row !== undefined) rows.push(row); | |
| 50 | } | |
| 51 | return rows; | |
| 52 | } | |
| 53 | ||
| 54 | /** The live pids a stop of `leader` reaches: the leader, its group, its descendants. */ | |
| 55 | export function treeOf(rows: readonly ProcRow[], leader: number): number[] { | |
| 56 | const live = rows.filter((r) => r.state !== "Z" && r.state !== "X"); | |
| 57 | const children = new Map<number, number[]>(); | |
| 58 | for (const r of live) { | |
| 59 | const list = children.get(r.ppid); | |
| 60 | if (list === undefined) children.set(r.ppid, [r.pid]); | |
| 61 | else list.push(r.pid); | |
| 62 | } | |
| 63 | const reached = new Set<number>(); | |
| 64 | if (live.some((r) => r.pid === leader)) reached.add(leader); | |
| 65 | for (const r of live) if (r.pgrp === leader) reached.add(r.pid); | |
| 66 | // A group member may have children outside the group (a `setsid`): walk | |
| 67 | // down from every one reached, each pid once. | |
| 68 | const walked = new Set<number>(); | |
| 69 | const stack = [...reached]; | |
| 70 | while (stack.length > 0) { | |
| 71 | const pid = stack.pop()!; | |
| 72 | if (walked.has(pid)) continue; | |
| 73 | walked.add(pid); | |
| 74 | for (const child of children.get(pid) ?? []) { | |
| 75 | reached.add(child); | |
| 76 | stack.push(child); | |
| 77 | } | |
| 78 | } | |
| 79 | return [...reached].sort((a, b) => a - b); | |
| 80 | } | |
| 81 | ||
| 82 | /** The live processes a cancel of `leader` is about to end, read now. */ | |
| 83 | export const liveTree = (leader: number): number[] => treeOf(readProcs(), leader); | |
| 84 | ||
| 85 | /** The one log line a cancel writes. */ | |
| 86 | export function endedLine(count: number): string { | |
| 87 | const n = Number.isInteger(count) && count > 0 ? count : 0; | |
| 88 | return `cancel ended ${n} ${n === 1 ? "process" : "processes"}`; | |
| 89 | } |
test/crew-cancel-group.test.ts +6 −1
| @@ -89,6 +89,7 @@ sleep 300 | ||
| 89 | 89 | chmodSync(binary, 0o755); |
| 90 | 90 | |
| 91 | 91 | const abort = new AbortController(); |
| 92 | const lines: string[] = []; | |
| 92 | 93 | const run = runClaude({ |
| 93 | 94 | binary, |
| 94 | 95 | cwd: dir, |
| @@ -98,7 +99,7 @@ sleep 300 | ||
| 98 | 99 | env: {}, |
| 99 | 100 | sandbox: sandboxPolicy({ dataDir: dir, agentName: "bot", urls: [] }), |
| 100 | 101 | timeoutMs: 60_000, |
| 101 | log: () => {}, | |
| 102 | log: (line) => lines.push(line), | |
| 102 | 103 | signal: abort.signal, |
| 103 | 104 | settleMs: 1_000, |
| 104 | 105 | }); |
| @@ -116,6 +117,10 @@ sleep 300 | ||
| 116 | 117 | const outcome = await run; |
| 117 | 118 | expect(outcome.ok).toBe(false); |
| 118 | 119 | expect(outcome.reason).toBe("cancelled"); |
| 120 | // One line says how many it ended: at least the three named here (#10826). | |
| 121 | const ended = lines.filter((l) => l.startsWith("cancel ended ")); | |
| 122 | expect(ended).toHaveLength(1); | |
| 123 | expect(Number(/^cancel ended (\d+) process/.exec(ended[0]!)?.[1])).toBeGreaterThanOrEqual(pids.length); | |
| 119 | 124 | // Reaped too, not left as zombies: no pid answers signal 0. |
| 120 | 125 | const deadline = Date.now() + 2_000; |
| 121 | 126 | const answers = (): number[] => |
test/run-cancel-ended.test.ts added +65
| @@ -0,0 +1,65 @@ | ||
| 1 | /** | |
| 2 | * What a cancel counts as ended (#10826): the leader, every live member of | |
| 3 | * its process group, and every live process under it — a `setsid` | |
| 4 | * grandchild included, a zombie or a stranger not. And the one line the | |
| 5 | * run's log gets. | |
| 6 | */ | |
| 7 | import { describe, expect, test } from "bun:test"; | |
| 8 | import { endedLine, liveTree, parseStat, treeOf, type ProcRow } from "../src/runs/ended.ts"; | |
| 9 | ||
| 10 | const row = (pid: number, ppid: number, pgrp: number, state = "S"): ProcRow => ({ pid, ppid, pgrp, state }); | |
| 11 | ||
| 12 | describe("treeOf", () => { | |
| 13 | test("reaches the leader, its group, and descendants that left the group", () => { | |
| 14 | const rows = [ | |
| 15 | row(1, 0, 1), | |
| 16 | row(100, 1, 100), // the leader | |
| 17 | row(101, 100, 100), // its child | |
| 18 | row(102, 101, 100), // grandchild, same group | |
| 19 | row(103, 101, 103), // grandchild that called setsid | |
| 20 | row(104, 103, 103), // and its own child | |
| 21 | row(105, 1, 100), // reparented to init, still in the group | |
| 22 | row(106, 100, 100, "Z"), // over already | |
| 23 | row(200, 1, 200), // a stranger | |
| 24 | ]; | |
| 25 | expect(treeOf(rows, 100)).toEqual([100, 101, 102, 103, 104, 105]); | |
| 26 | }); | |
| 27 | ||
| 28 | test("is empty once the leader and its group are gone", () => { | |
| 29 | expect(treeOf([row(1, 0, 1), row(200, 1, 200)], 100)).toEqual([]); | |
| 30 | }); | |
| 31 | }); | |
| 32 | ||
| 33 | describe("parseStat", () => { | |
| 34 | test("reads state, parent and group past a name with spaces and parentheses", () => { | |
| 35 | expect(parseStat(42, "42 (a (b) c) S 7 42 42 0 -1")).toEqual({ pid: 42, ppid: 7, pgrp: 42, state: "S" }); | |
| 36 | expect(parseStat(42, "garbage")).toBeUndefined(); | |
| 37 | }); | |
| 38 | }); | |
| 39 | ||
| 40 | describe("endedLine", () => { | |
| 41 | test("is one line with the count", () => { | |
| 42 | expect(endedLine(1)).toBe("cancel ended 1 process"); | |
| 43 | expect(endedLine(3)).toBe("cancel ended 3 processes"); | |
| 44 | expect(endedLine(0)).toBe("cancel ended 0 processes"); | |
| 45 | expect(endedLine(Number.NaN)).toBe("cancel ended 0 processes"); | |
| 46 | }); | |
| 47 | }); | |
| 48 | ||
| 49 | describe.skipIf(process.platform !== "linux")("liveTree", () => { | |
| 50 | test("counts a real child and the grandchild it forked", async () => { | |
| 51 | const proc = Bun.spawn(["/bin/sh", "-c", "sleep 30 & wait"], { detached: true, stdout: "ignore" }); | |
| 52 | try { | |
| 53 | const deadline = Date.now() + 5_000; | |
| 54 | while (liveTree(proc.pid).length < 2 && Date.now() < deadline) await Bun.sleep(20); | |
| 55 | expect(liveTree(proc.pid).length).toBe(2); | |
| 56 | } finally { | |
| 57 | try { | |
| 58 | process.kill(-proc.pid, "SIGKILL"); | |
| 59 | } catch { | |
| 60 | proc.kill("SIGKILL"); | |
| 61 | } | |
| 62 | } | |
| 63 | await proc.exited; | |
| 64 | }); | |
| 65 | }); |
test/run-cancel-tree.test.ts +5
| @@ -18,6 +18,7 @@ import { commitToBranch, createRepo } from "../src/actions/repos.ts"; | ||
| 18 | 18 | import { cancelRepoRun } from "../src/actions/runs.ts"; |
| 19 | 19 | import { startChecks, type ChecksHandle } from "../src/checks/service.ts"; |
| 20 | 20 | import { getRun, listRuns } from "../src/core/runs.ts"; |
| 21 | import { readRunLog } from "../src/runs/log.ts"; | |
| 21 | 22 | import { createUser } from "../src/core/users.ts"; |
| 22 | 23 | import { runningRun } from "../src/runs/stoppable.ts"; |
| 23 | 24 | import { quietLogs } from "./helpers/globals.ts"; |
| @@ -148,5 +149,9 @@ describe.skipIf(process.platform !== "linux")("a cancelled check", () => { | ||
| 148 | 149 | const after = getRun(app.db, run.id); |
| 149 | 150 | expect(after?.state).toBe("cancelled"); |
| 150 | 151 | expect(after?.summary).toBe("cancelled by alice"); |
| 152 | // The run's log says, in one line, how many processes the cancel ended (#10826). | |
| 153 | const ended = readRunLog(dataDir, run.id).lines.filter((l) => l.startsWith("cancel ended ")); | |
| 154 | expect(ended).toHaveLength(1); | |
| 155 | expect(Number(/^cancel ended (\d+) process/.exec(ended[0]!)?.[1])).toBeGreaterThanOrEqual(pids.length); | |
| 151 | 156 | }, 45_000); |
| 152 | 157 | }); |
On a host without /proc, readProcs() returns [] and this prints
cancel ended 0 processeseven though processes were stopped. Prefer saying the count is unavailable.