fix(mux): bound the process-tree walk — it can take a machine down

`getChildPids` ran `pgrep -P <pid>` 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/<pid>/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) <noreply@anthropic.com>
This commit is contained in:
Claudia
2026-08-07 01:34:47 +02:00
parent d41f28bc14
commit 2e69e28e71
4 changed files with 421 additions and 40 deletions
+17
View File
@@ -0,0 +1,17 @@
---
'aicodeman': patch
---
Bound the process-tree walk that could take a machine down.
`getChildPids` ran `pgrep -P <pid>` 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/<pid>/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.
+87
View File
@@ -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 <pid>` 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/<pid>/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<number, readonly number[]>,
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<number>([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;
}
+117 -40
View File
@@ -22,7 +22,8 @@
*/ */
import { EventEmitter } from 'node:events'; 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'; import { promisify } from 'node:util';
const execAsync = promisify(exec); const execAsync = promisify(exec);
@@ -100,6 +101,16 @@ import {
// ============================================================================ // ============================================================================
import { EXEC_TIMEOUT_MS } from './config/exec-timeout.js'; 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'; 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 /** One `ps` snapshot of the whole process table, cached briefly. */
private getChildPids(pid: number): number[] { private static procSnapshot: { at: number; byParent: Map<number, number[]> } | null = null;
const pids: number[] = []; /** Single-flight guard so a hung `ps` cannot pile up parallel refreshes. */
try { private static procRefresh: { started: number; promise: Promise<Map<number, number[]>> } | null = null;
const output = execSync(`pgrep -P ${pid}`, {
encoding: 'utf-8', /**
timeout: EXEC_TIMEOUT_MS, * Fork ONE `ps` asynchronously and cache the parent -> children map.
}).trim(); *
if (output) { * Async on purpose: a synchronous fork here would block the event loop on every
for (const childPid of output * stats tick, and under the procfs pathology this module exists to survive,
.split('\n') * `execSync`'s timeout cannot return at all (spawnSync waits for the unkillable
.map((p) => parseInt(p, 10)) * child) — freezing the whole server where a hung async poll only costs staleness.
.filter((p) => !Number.isNaN(p))) { */
pids.push(childPid); private static refreshProcSnapshot(): Promise<Map<number, number[]>> {
pids.push(...this.getChildPids(childPid)); 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<Map<number, number[]>>((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;
} }
} const byParent = new Map<number, number[]>();
} catch { for (const line of String(out).split('\n')) {
// No children or command failed 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<number, number[]> {
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<number[]> {
let byParent: ReadonlyMap<number, readonly number[]>;
try {
byParent = await Promise.race([
TmuxManager.refreshProcSnapshot(),
new Promise<never>((_, 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<number, number[]>();
}
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 // Check if a process is still alive
@@ -2221,7 +2307,7 @@ export class TmuxManager extends EventEmitter implements TerminalMultiplexer {
const allPids: number[] = [currentPid]; const allPids: number[] = [currentPid];
// Strategy 1: Kill all child processes recursively // Strategy 1: Kill all child processes recursively
let childPids = this.getChildPids(currentPid); let childPids = await this.getChildPidsFresh(currentPid);
if (childPids.length > 0) { if (childPids.length > 0) {
console.log(`[TmuxManager] Found ${childPids.length} child processes to kill`); console.log(`[TmuxManager] Found ${childPids.length} child processes to kill`);
allPids.push(...childPids); allPids.push(...childPids);
@@ -2238,7 +2324,7 @@ export class TmuxManager extends EventEmitter implements TerminalMultiplexer {
await new Promise((resolve) => setTimeout(resolve, TMUX_KILL_WAIT_MS)); await new Promise((resolve) => setTimeout(resolve, TMUX_KILL_WAIT_MS));
childPids = this.getChildPids(currentPid); childPids = await this.getChildPidsFresh(currentPid);
for (const childPid of childPids) { for (const childPid of childPids) {
if (this.isProcessAlive(childPid)) { if (this.isProcessAlive(childPid)) {
try { try {
@@ -2452,17 +2538,13 @@ export class TmuxManager extends EventEmitter implements TerminalMultiplexer {
const [rss, cpu] = psOutput.split(/\s+/).map((x) => parseFloat(x) || 0); 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; let childCount = 0;
try { try {
const childOutput = ( childCount = (this.childrenByParent().get(session.pid) ?? []).length;
await execAsync(`pgrep -P ${session.pid} | wc -l`, {
encoding: 'utf-8',
timeout: EXEC_TIMEOUT_MS,
})
).stdout.trim();
childCount = parseInt(childOutput, 10) || 0;
} catch { } catch {
// No children or command failed // No children or snapshot unavailable
} }
return { return {
@@ -2496,17 +2578,12 @@ export class TmuxManager extends EventEmitter implements TerminalMultiplexer {
// Step 1: Get descendant PIDs // Step 1: Get descendant PIDs
const descendantMap = new Map<number, number[]>(); const descendantMap = new Map<number, number[]>();
const pgrepOutput = ( // Derived from the ONE snapshot instead of a shell loop that forks a pgrep
await execAsync( // per session — the shape that turned into a fork storm under load.
`for p in ${sessionPids.join(' ')}; do children=$(pgrep -P $p 2>/dev/null | tr '\\n' ','); echo "$p:$children"; done`, const byParent = this.childrenByParent();
{ const childLines = sessionPids.map((p) => `${p}:${(byParent.get(p) ?? []).join(',')}`).join('\n');
encoding: 'utf-8',
timeout: EXEC_TIMEOUT_MS,
}
)
).stdout.trim();
for (const line of pgrepOutput.split('\n')) { for (const line of childLines.split('\n')) {
const [pidStr, childrenStr] = line.split(':'); const [pidStr, childrenStr] = line.split(':');
const sessionPid = parseInt(pidStr, 10); const sessionPid = parseInt(pidStr, 10);
if (!Number.isNaN(sessionPid)) { if (!Number.isNaN(sessionPid)) {
+200
View File
@@ -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 <pid>` 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/<pid>/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<number, number[]> {
const m = new Map<number, number[]>();
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<number, number[]> {
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<number, number[]>([[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<number, number[]>();
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<number, number[]> {
const t = new Map<number, number[]>();
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<number, number[]>): Promise<number[]> {
const { TmuxManager } = await import('../src/tmux-manager.js');
const Klass = TmuxManager as unknown as {
refreshProcSnapshot(): Promise<Map<number, number[]>>;
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<number[]> }).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<number, number[]>();
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);
});
});