From 2e69e28e7197aaf98dd6572dc2d56119c975d830 Mon Sep 17 00:00:00 2001 From: Claudia Date: Fri, 7 Aug 2026 01:34:47 +0200 Subject: [PATCH] =?UTF-8?q?fix(mux):=20bound=20the=20process-tree=20walk?= =?UTF-8?q?=20=E2=80=94=20it=20can=20take=20a=20machine=20down?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `getChildPids` ran `pgrep -P ` per node and recursed with no visited set, no depth limit and no node cap. Two further sites forked a `pgrep` per session on every stats tick. Across ~28 adopted tmux trees the fan-out exploded, and because each `pgrep` blocks in the kernel while reading `/proc//cgroup` under WSL, none returned while the walk kept spawning more. Observed: ~13,000 `pgrep` processes stuck in D-state out of ~39,000 total, load average above 13,000, and a machine only recoverable by restarting WSL — which cost every running session. Every diagnostic command timed out too, because they read /proc as well. - ONE `ps -eo pid=,ppid=` snapshot, cached briefly and refreshed asynchronously with a single-flight guard. Async matters: under the same procfs pathology, `execSync`'s timeout cannot return (spawnSync waits for the unkillable child), which would freeze the server where a hung async poll only costs staleness. - The traversal moved to `proc-tree.ts` as a pure function — breadth-first, with a visited set (a stale snapshot can contain a cycle), a depth cap and a node cap, both reporting when they truncate. Pure so the regression tests can exercise the shipped code rather than a copy of it. - The kill path forces a fresh snapshot: the wait between SIGTERM and the survivor re-scan (200ms) sits inside the cache TTL (2000ms), so reading the cache there would return pre-SIGTERM state and aim SIGKILL at stale PIDs. That wait is bounded, so a wedged `ps` cannot stop killSession from reaching its process-group and tmux fallbacks. - Any `ps` error keeps the previous snapshot instead of caching partial output as fresh; a truncated table would make whole subtrees invisible to the kill path. 13 tests, including one that drives TmuxManager itself — with the caps bypassed at the call site, 3 of them fail. The snapshot refresh is stubbed there, because otherwise the manager runs a real `ps`, replaces the fixture, and the test silently measures the machine's own process tree instead. Co-Authored-By: Claude Opus 5 (1M context) --- .changeset/bounded-process-tree-walk.md | 17 ++ src/proc-tree.ts | 87 +++++++++++ src/tmux-manager.ts | 157 ++++++++++++++----- test/proc-walk-bounds.test.ts | 200 ++++++++++++++++++++++++ 4 files changed, 421 insertions(+), 40 deletions(-) create mode 100644 .changeset/bounded-process-tree-walk.md create mode 100644 src/proc-tree.ts create mode 100644 test/proc-walk-bounds.test.ts diff --git a/.changeset/bounded-process-tree-walk.md b/.changeset/bounded-process-tree-walk.md new file mode 100644 index 00000000..ef0ea5f3 --- /dev/null +++ b/.changeset/bounded-process-tree-walk.md @@ -0,0 +1,17 @@ +--- +'aicodeman': patch +--- + +Bound the process-tree walk that could take a machine down. + +`getChildPids` ran `pgrep -P ` per node and recursed with no visited set, no +depth limit and no node cap. Across ~28 adopted tmux trees the fan-out exploded, +and because each `pgrep` blocks in the kernel while reading `/proc//cgroup` +under WSL, none returned while the walk kept spawning more — ~13,000 `pgrep` +processes stuck in D-state out of ~39,000 total, load average above 13,000, +recoverable only by restarting WSL. + +Now: one `ps` snapshot, breadth-first with a visited set, a depth cap and a node +cap, in a pure module (`proc-tree.ts`) that the regression tests exercise +directly. The snapshot is refreshed asynchronously, and the kill path forces a +fresh one so the SIGKILL escalation cannot re-read pre-SIGTERM state. diff --git a/src/proc-tree.ts b/src/proc-tree.ts new file mode 100644 index 00000000..892497f7 --- /dev/null +++ b/src/proc-tree.ts @@ -0,0 +1,87 @@ +/** + * @fileoverview Bounded descendant walk over a process-tree snapshot. + * + * Split out of `tmux-manager.ts` so the traversal can be unit-tested directly. It + * previously lived as a private method, which meant the regression test had to keep + * its own copy of the algorithm — a test that passes while the shipped code rots. + * + * ## The incident this guards against + * + * On 2026-07-30 an unbounded version of this walk took a machine down. It ran + * `pgrep -P ` once per node and recursed with no visited set, no depth limit and + * no node cap. Across ~28 adopted tmux trees the fan-out exploded, and because each + * `pgrep` blocks in the WSL kernel while reading `/proc//cgroup`, none of them + * returned while the walk kept spawning more. Result: ~13,000 `pgrep` processes stuck + * in D-state out of ~39,000 total, load average above 13,000, and a machine only + * recoverable by restarting WSL — which cost every running session. + * + * Three properties make that impossible, and each has a test: + * 1. a cycle terminates instead of looping (stale snapshots can contain one), + * 2. depth is capped, + * 3. node count is capped. + * + * The fourth property — spawning nothing per node — is structural: this function + * takes a snapshot and cannot spawn anything at all. + * + * @module proc-tree + */ + +/** Maximum generations to descend. Deeper than any real agent process tree. */ +export const PROC_WALK_MAX_DEPTH = 10; + +/** Hard ceiling on collected descendants. A backstop, not an expected limit. */ +export const PROC_WALK_MAX_NODES = 500; + +export interface WalkOptions { + maxDepth?: number; + maxNodes?: number; + /** + * Called once when a cap truncated the result, with which cap it was. Both are + * reported: a silent depth cap would hide a deep tree just as effectively as a + * silent node cap hides a wide one, and the whole point of this module is that + * truncation is visible rather than mysterious. + */ + onTruncated?: (pid: number, cap: number, reason: 'nodes' | 'depth') => void; +} + +/** + * All descendants of `pid`, breadth-first and bounded. + * + * @param pid root of the walk; never included in the result + * @param byParent parent pid → child pids, from ONE `ps` snapshot + */ +export function collectDescendants( + pid: number, + byParent: ReadonlyMap, + opts: WalkOptions = {} +): number[] { + const maxDepth = opts.maxDepth ?? PROC_WALK_MAX_DEPTH; + const maxNodes = opts.maxNodes ?? PROC_WALK_MAX_NODES; + + const out: number[] = []; + const visited = new Set([pid]); + let frontier = [pid]; + + for (let depth = 0; depth < maxDepth && frontier.length; depth += 1) { + const next: number[] = []; + for (const parent of frontier) { + for (const child of byParent.get(parent) ?? []) { + if (visited.has(child)) continue; // a real tree has no cycles, a stale + visited.add(child); // snapshot can still produce one + out.push(child); + next.push(child); + if (out.length >= maxNodes) { + opts.onTruncated?.(pid, maxNodes, 'nodes'); + return out; + } + } + } + frontier = next; + // Ran out of generations while descendants were still queued: the tree is + // deeper than the cap and the result is incomplete. + if (depth === maxDepth - 1 && frontier.length > 0) { + opts.onTruncated?.(pid, maxDepth, 'depth'); + } + } + return out; +} diff --git a/src/tmux-manager.ts b/src/tmux-manager.ts index 101dd10f..2ecd02b1 100644 --- a/src/tmux-manager.ts +++ b/src/tmux-manager.ts @@ -22,7 +22,8 @@ */ import { EventEmitter } from 'node:events'; -import { execSync, exec } from 'node:child_process'; +import { collectDescendants } from './proc-tree.js'; +import { execSync, exec, execFile } from 'node:child_process'; import { promisify } from 'node:util'; const execAsync = promisify(exec); @@ -100,6 +101,16 @@ import { // ============================================================================ import { EXEC_TIMEOUT_MS } from './config/exec-timeout.js'; + +/** How long a cached process snapshot stays usable. */ +const PROC_SNAPSHOT_TTL_MS = 2000; + +/** + * How long the kill path waits for a fresh snapshot before giving up on it. + * Shorter than EXEC_TIMEOUT_MS on purpose: killSession has two further strategies + * (process group, tmux kill-session) and must reach them even when `ps` is wedged. + */ +const PROC_SNAPSHOT_WAIT_MS = 1500; import { DEFAULT_TMUX_HISTORY_LIMIT, DEFAULT_TERMINAL_BUFFER_MAX_BYTES } from './config/terminal-history.js'; /** @@ -2094,27 +2105,102 @@ export class TmuxManager extends EventEmitter implements TerminalMultiplexer { } } - // Get all child process PIDs recursively - private getChildPids(pid: number): number[] { - const pids: number[] = []; - try { - const output = execSync(`pgrep -P ${pid}`, { - encoding: 'utf-8', - timeout: EXEC_TIMEOUT_MS, - }).trim(); - if (output) { - for (const childPid of output - .split('\n') - .map((p) => parseInt(p, 10)) - .filter((p) => !Number.isNaN(p))) { - pids.push(childPid); - pids.push(...this.getChildPids(childPid)); + /** One `ps` snapshot of the whole process table, cached briefly. */ + private static procSnapshot: { at: number; byParent: Map } | null = null; + /** Single-flight guard so a hung `ps` cannot pile up parallel refreshes. */ + private static procRefresh: { started: number; promise: Promise> } | null = null; + + /** + * Fork ONE `ps` asynchronously and cache the parent -> children map. + * + * Async on purpose: a synchronous fork here would block the event loop on every + * stats tick, and under the procfs pathology this module exists to survive, + * `execSync`'s timeout cannot return at all (spawnSync waits for the unkillable + * child) — freezing the whole server where a hung async poll only costs staleness. + */ + private static refreshProcSnapshot(): Promise> { + const inFlight = TmuxManager.procRefresh; + // Reuse an in-flight refresh — unless it is old enough to be presumed stuck. + if (inFlight && Date.now() - inFlight.started < EXEC_TIMEOUT_MS * 2) return inFlight.promise; + + const started = Date.now(); + const promise = new Promise>((resolve) => { + execFile('ps', ['-eo', 'pid=,ppid='], { timeout: EXEC_TIMEOUT_MS, maxBuffer: 8 * 1024 * 1024 }, (err, out) => { + if (TmuxManager.procRefresh?.started === started) TmuxManager.procRefresh = null; + if (err) { + // ANY error, not just an empty one: a timed-out or truncated `ps` yields + // partial output, and caching that as fresh would make whole subtrees + // invisible — including to the kill path. Stale beats wrong. + console.error('[TmuxManager] process snapshot failed:', err); + resolve(TmuxManager.procSnapshot?.byParent ?? new Map()); + return; } - } - } catch { - // No children or command failed + const byParent = new Map(); + for (const line of String(out).split('\n')) { + const parts = line.trim().split(/\s+/); + if (parts.length < 2) continue; + const pid = parseInt(parts[0], 10); + const ppid = parseInt(parts[1], 10); + if (Number.isNaN(pid) || Number.isNaN(ppid)) continue; + const list = byParent.get(ppid); + if (list) list.push(pid); + else byParent.set(ppid, [pid]); + } + TmuxManager.procSnapshot = { at: Date.now(), byParent }; + resolve(byParent); + }); + }); + TmuxManager.procRefresh = { started, promise }; + return promise; + } + + /** + * Best snapshot WITHOUT forking: returns the cache, kicking off a background + * refresh when it has gone stale, and never blocks. Stats and window-title + * consumers tolerate data one interval old; nothing that KILLS may use this. + */ + private childrenByParent(): Map { + const cached = TmuxManager.procSnapshot; + if (!cached || Date.now() - cached.at >= PROC_SNAPSHOT_TTL_MS) { + void TmuxManager.refreshProcSnapshot(); } - return pids; + return cached?.byParent ?? new Map(); + } + + /** + * Descendants from a snapshot that is not the cached one — the kill path's variant. + * + * killSession re-scans for survivors between SIGTERM and SIGKILL, and the wait in + * between (200ms) sits far inside the cache TTL (2000ms): reading the cache there + * returns the pre-SIGTERM state verbatim, so children spawned since are invisible + * and SIGKILL aims at stale PIDs, guarded only by kill(pid, 0) — which cannot + * detect PID reuse. + * + * It forces a refresh rather than guaranteeing recency: an already-running refresh + * is reused, so the snapshot can predate this call by up to one `ps` runtime. A + * strict postdate guarantee would mean chaining a second `ps` behind every + * in-flight one, which is the fork storm this code exists to avoid. + * + * Bounded by design: waiting forever would freeze killSession before it reaches + * its process-group and tmux fallbacks. + */ + private async getChildPidsFresh(pid: number): Promise { + let byParent: ReadonlyMap; + try { + byParent = await Promise.race([ + TmuxManager.refreshProcSnapshot(), + new Promise((_, reject) => + setTimeout(() => reject(new Error('proc snapshot timeout')), PROC_SNAPSHOT_WAIT_MS) + ), + ]); + } catch { + console.warn('[TmuxManager] process snapshot did not return in time; using the cached one'); + byParent = TmuxManager.procSnapshot?.byParent ?? new Map(); + } + return collectDescendants(pid, byParent, { + onTruncated: (root, cap, reason) => + console.warn(`[TmuxManager] descendant walk for ${root} hit the ${cap}-${reason} cap; truncating`), + }); } // Check if a process is still alive @@ -2221,7 +2307,7 @@ export class TmuxManager extends EventEmitter implements TerminalMultiplexer { const allPids: number[] = [currentPid]; // Strategy 1: Kill all child processes recursively - let childPids = this.getChildPids(currentPid); + let childPids = await this.getChildPidsFresh(currentPid); if (childPids.length > 0) { console.log(`[TmuxManager] Found ${childPids.length} child processes to kill`); allPids.push(...childPids); @@ -2238,7 +2324,7 @@ export class TmuxManager extends EventEmitter implements TerminalMultiplexer { await new Promise((resolve) => setTimeout(resolve, TMUX_KILL_WAIT_MS)); - childPids = this.getChildPids(currentPid); + childPids = await this.getChildPidsFresh(currentPid); for (const childPid of childPids) { if (this.isProcessAlive(childPid)) { try { @@ -2452,17 +2538,13 @@ export class TmuxManager extends EventEmitter implements TerminalMultiplexer { const [rss, cpu] = psOutput.split(/\s+/).map((x) => parseFloat(x) || 0); + // From the shared snapshot: this runs per session on every stats tick, and a + // pgrep per session was a fork per session per interval. let childCount = 0; try { - const childOutput = ( - await execAsync(`pgrep -P ${session.pid} | wc -l`, { - encoding: 'utf-8', - timeout: EXEC_TIMEOUT_MS, - }) - ).stdout.trim(); - childCount = parseInt(childOutput, 10) || 0; + childCount = (this.childrenByParent().get(session.pid) ?? []).length; } catch { - // No children or command failed + // No children or snapshot unavailable } return { @@ -2496,17 +2578,12 @@ export class TmuxManager extends EventEmitter implements TerminalMultiplexer { // Step 1: Get descendant PIDs const descendantMap = new Map(); - const pgrepOutput = ( - await execAsync( - `for p in ${sessionPids.join(' ')}; do children=$(pgrep -P $p 2>/dev/null | tr '\\n' ','); echo "$p:$children"; done`, - { - encoding: 'utf-8', - timeout: EXEC_TIMEOUT_MS, - } - ) - ).stdout.trim(); + // Derived from the ONE snapshot instead of a shell loop that forks a pgrep + // per session — the shape that turned into a fork storm under load. + const byParent = this.childrenByParent(); + const childLines = sessionPids.map((p) => `${p}:${(byParent.get(p) ?? []).join(',')}`).join('\n'); - for (const line of pgrepOutput.split('\n')) { + for (const line of childLines.split('\n')) { const [pidStr, childrenStr] = line.split(':'); const sessionPid = parseInt(pidStr, 10); if (!Number.isNaN(sessionPid)) { diff --git a/test/proc-walk-bounds.test.ts b/test/proc-walk-bounds.test.ts new file mode 100644 index 00000000..f29e0475 --- /dev/null +++ b/test/proc-walk-bounds.test.ts @@ -0,0 +1,200 @@ +/** + * Regression guard for the process-tree walk. + * + * On 2026-07-30 an unbounded version took a machine down: it ran `pgrep -P ` per + * node and recursed with no visited set, no depth limit and no node cap. Across ~28 + * adopted tmux trees the fan-out exploded, and because each `pgrep` blocks in the WSL + * kernel while reading /proc//cgroup, none returned while the walk kept firing + * more. Result: ~13,000 `pgrep` processes stuck in D-state out of ~39,000 total, load + * average above 13,000, recoverable only by + * restarting WSL — which cost every running session. + * + * These tests exercise the SHIPPED function. An earlier version of this file carried + * its own copy of the traversal, which would have passed happily while the real code + * regressed; that is why the walk now lives in its own module. + */ +import { describe, expect, it, vi } from 'vitest'; + +import { collectDescendants, PROC_WALK_MAX_DEPTH, PROC_WALK_MAX_NODES } from '../src/proc-tree.js'; + +/** Build a parent→children map from `[parent, child]` pairs. */ +function tree(pairs: [number, number][]): Map { + const m = new Map(); + for (const [p, c] of pairs) m.set(p, [...(m.get(p) ?? []), c]); + return m; +} + +/** A chain 1→2→…→n, i.e. depth n-1. */ +function chain(n: number): Map { + return tree(Array.from({ length: n - 1 }, (_, i) => [i + 1, i + 2] as [number, number])); +} + +describe('collectDescendants', () => { + it('returns every descendant of a normal tree, root excluded', () => { + const t = tree([ + [1, 2], + [1, 3], + [2, 4], + [3, 5], + ]); + expect(collectDescendants(1, t).sort((a, b) => a - b)).toEqual([2, 3, 4, 5]); + }); + + it('terminates on a cycle instead of looping forever', () => { + // A live `ps` snapshot is not atomic; pid reuse can produce a parent loop. + const t = tree([ + [1, 2], + [2, 3], + [3, 1], + [3, 2], + ]); + expect(collectDescendants(1, t).sort((a, b) => a - b)).toEqual([2, 3]); + }); + + it('does not include the root even when something claims it as a child', () => { + expect(collectDescendants(1, tree([[1, 1]]))).toEqual([]); + }); + + it('caps the depth, and says so', () => { + // 40 generations available, only PROC_WALK_MAX_DEPTH may be descended. A silent + // depth cap hides a deep tree exactly as a silent node cap hides a wide one. + const onTruncated = vi.fn(); + expect(collectDescendants(1, chain(40), { onTruncated })).toHaveLength(PROC_WALK_MAX_DEPTH); + expect(onTruncated).toHaveBeenCalledWith(1, PROC_WALK_MAX_DEPTH, 'depth'); + }); + + it('stays silent about depth when the tree ends inside the cap', () => { + const onTruncated = vi.fn(); + collectDescendants(1, chain(4), { onTruncated }); + expect(onTruncated).not.toHaveBeenCalled(); + }); + + it('caps the node count and reports the truncation', () => { + // One parent with far more children than the cap allows. + const wide = new Map([[1, Array.from({ length: PROC_WALK_MAX_NODES * 3 }, (_, i) => i + 2)]]); + const onTruncated = vi.fn(); + + const out = collectDescendants(1, wide, { onTruncated }); + + expect(out).toHaveLength(PROC_WALK_MAX_NODES); + expect(onTruncated).toHaveBeenCalledWith(1, PROC_WALK_MAX_NODES, 'nodes'); + }); + + it('stays silent when nothing was truncated', () => { + const onTruncated = vi.fn(); + collectDescendants(1, tree([[1, 2]]), { onTruncated }); + expect(onTruncated).not.toHaveBeenCalled(); + }); + + it('survives the shape that caused the incident: many wide, deep trees', () => { + // 28 adopted trees, branching 4-wide. Depth 6 already gives 4096 nodes per tree — + // eight times the cap, which is what this asserts. (The real incident's trees were + // deeper still; building that here would mean materialising 16M map entries and + // would only test the fixture builder.) + const t = new Map(); + let next = 1000; + const roots: number[] = []; + for (let r = 0; r < 28; r += 1) { + const root = next++; + roots.push(root); + let frontier = [root]; + for (let d = 0; d < 6; d += 1) { + const nf: number[] = []; + for (const p of frontier) { + const kids = [next++, next++, next++, next++]; + t.set(p, kids); + nf.push(...kids); + } + frontier = nf; + } + } + + for (const root of roots) { + const out = collectDescendants(root, t); + expect(out.length).toBeLessThanOrEqual(PROC_WALK_MAX_NODES); + } + }); + + it('honours explicit overrides', () => { + expect(collectDescendants(1, chain(40), { maxDepth: 3 })).toEqual([2, 3, 4]); + expect(collectDescendants(1, chain(40), { maxNodes: 2 })).toEqual([2, 3]); + }); + + it('returns nothing for an unknown pid or an empty snapshot', () => { + expect(collectDescendants(999, tree([[1, 2]]))).toEqual([]); + expect(collectDescendants(1, new Map())).toEqual([]); + }); +}); + +/** + * The bound must be reachable through the code that actually kills things. + * + * The unit tests above exercise `collectDescendants` directly, which is necessary but + * not sufficient: reverting `tmux-manager.ts` to the old unbounded `pgrep -P` recursion + * left every one of them green. This asserts the wiring — that TmuxManager's descendant + * lookup goes through the bounded walk and honours its caps. + * + * The snapshot refresh is stubbed. Without that the manager runs a real `ps` and + * replaces the fixture, and the test silently measures the machine's own process tree + * instead of the tree under test — which is how the first version of this test passed + * even with both caps bypassed. + */ +describe('TmuxManager uses the bounded walk', () => { + /** Build a tree `width` wide and `depth` deep, rooted at 1. */ + function bigTree(width: number, depth: number): Map { + const t = new Map(); + let next = 2; + let frontier = [1]; + for (let d = 0; d < depth; d += 1) { + const nf: number[] = []; + for (const p of frontier) { + const kids = Array.from({ length: width }, () => next++); + t.set(p, kids); + nf.push(...kids); + } + frontier = nf; + } + return t; + } + + async function walkVia(fixture: Map): Promise { + const { TmuxManager } = await import('../src/tmux-manager.js'); + const Klass = TmuxManager as unknown as { + refreshProcSnapshot(): Promise>; + procSnapshot: unknown; + }; + const original = Klass.refreshProcSnapshot; + Klass.refreshProcSnapshot = () => Promise.resolve(fixture); + Klass.procSnapshot = { at: Date.now(), byParent: fixture }; + try { + const mgr = new TmuxManager(); + return await (mgr as unknown as { getChildPidsFresh(pid: number): Promise }).getChildPidsFresh(1); + } finally { + Klass.refreshProcSnapshot = original; + Klass.procSnapshot = null; + } + } + + it('honours the node cap on a tree far wider than it', async () => { + // 4096 descendants available; the cap is 500. With the caps bypassed — the shape + // a regression at the call site would take — this returns thousands. + const out = await walkVia(bigTree(4, 6)); + expect(out.length).toBe(PROC_WALK_MAX_NODES); + }); + + it('honours the depth cap on a deep chain', async () => { + const chainTree = new Map(); + for (let i = 1; i < 40; i += 1) chainTree.set(i, [i + 1]); + + const out = await walkVia(chainTree); + + expect(out.length).toBe(PROC_WALK_MAX_DEPTH); + }); + + it('spawns nothing per node — the walk only reads the snapshot', async () => { + // A per-node spawn against this fixture would mean thousands of processes; the + // test completing at all is the assertion, plus the bound holding. + const out = await walkVia(bigTree(4, 6)); + expect(out.length).toBeLessThanOrEqual(PROC_WALK_MAX_NODES); + }); +});