diff --git a/docs/design/f2-mcp-transport-pool.md b/docs/design/f2-mcp-transport-pool.md index 6b54771c79..4da25cf8a0 100644 --- a/docs/design/f2-mcp-transport-pool.md +++ b/docs/design/f2-mcp-transport-pool.md @@ -75,14 +75,14 @@ PR #4336 shipped F2 as 6 atomic commits + 6 fix commits over ~4 hours. Wenshao r #### Declined-with-reply (filed as F2 follow-ups) -| # | Site | Reason for declining | -| --- | --------------------------------------------------- | --------------------------------------------------------------------------------------------------------------------------------------- | -| W7 | Test coverage gaps (4 untested critical paths) | 1/4 added (W6 regression test); rest deferred to focused test-coverage PR after F2 series merges | -| W8 | `maxReconnectAttempts` / `reconnectStrategy` unused | Forward-compat placeholders for the deferred health-monitor-driven reconnect (design §6.6); removing + re-adding churns the public type | -| W11 | Duplicate fast-path / in-flight-path attach blocks | Refactor opportunity touching the entire `acquire` signature; not blocking F2 merge | -| W12 | `passesSessionFilter` O(M×N) per `applyTools` | Micro-perf optimization measurable only with hundreds of tools × large filter lists | -| R9 | `McpClientManager` ctor 7-positional sentinels | Refactor to options-object touches every call site (test fixtures, ToolRegistry); out of F2 scope | -| R10 | `pgrep -P ` per-PID-per-level cost | Single `ps -eo pid,ppid` requires platform-specific column flags + tree builder; current 16s worst-case bound is acceptable | +| # | Site | Reason for declining | +| --- | --------------------------------------------------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| W7 | Test coverage gaps (4 untested critical paths) | 1/4 added (W6 regression test); rest deferred to focused test-coverage PR after F2 series merges | +| W8 | `maxReconnectAttempts` / `reconnectStrategy` unused | Forward-compat placeholders for the deferred health-monitor-driven reconnect (design §6.6); removing + re-adding churns the public type | +| W11 | Duplicate fast-path / in-flight-path attach blocks | ✅ Done in PR A: `attachPooledSession` + `rollbackReservationOnSpawnFailure` private helpers (commit `2d546efca`) | +| W12 | `passesSessionFilter` O(M×N) per `applyTools` | ✅ Done in PR A: `applyTools` / `applyPrompts` precompute filter `Set`s once per pass; predicate becomes O(1) per tool (commit `a4a855ab3`) | +| R9 | `McpClientManager` ctor 7-positional sentinels | ✅ Done in PR A: options-object ctor + `mkManager` test factory (commit `0cb1eaa27`) | +| R10 | `pgrep -P ` per-PID-per-level cost | ✅ Done in PR A: single `ps -A -o pid=,ppid=` snapshot + in-memory BFS walk; pgrep BFS retained as fallback for BusyBox ` / `Get-CimInstance -Filter` subprocess per node) to a single process-table snapshot followed by in-memory tree walk. Two motivations: (1) one fork instead of B^D forks on the hot pool-shutdown path; (2) snapshot consistency — pre-fix BFS could miss descendants that forked between adjacent BFS levels. Per-pid path retained as fallback for BusyBox `ps` { - if (process.platform === 'win32') return listDescendantPidsWin(rootPid); - return listDescendantPidsUnix(rootPid); + if (!Number.isInteger(rootPid) || rootPid <= 0) return []; + try { + if (process.platform === 'win32') + return await listDescendantPidsWin(rootPid); + return await listDescendantPidsUnix(rootPid); + } catch { + return []; // OS reaps orphans; pool shutdown still proceeds. + } } async function listDescendantPidsUnix(root: number): Promise { - const all: number[] = []; - const queue = [root]; - while (queue.length) { - const parent = queue.shift()!; - const { stdout } = await execFile('pgrep', ['-P', String(parent)], { - timeout: 2000, - }).catch(() => ({ stdout: '' })); - const children = stdout.split('\n').map(Number).filter(Number.isFinite); - all.push(...children); - queue.push(...children); + let tree: Map | undefined; + try { + tree = await snapshotProcessTreeUnix(); // ps -A -o pid=,ppid= + } catch { + /* fall through to fallback */ } - return all; + if (tree) return walkDescendants(tree, root); // O(descendants), 1 fork + return await listDescendantPidsUnixPgrepFallback(root); // legacy BFS } -async function listDescendantPidsWin(root: number): Promise { - // PowerShell CIM query — wmic deprecated on modern Windows - const script = `Get-CimInstance Win32_Process | Where-Object { $_.ParentProcessId -eq } | Select-Object -ExpandProperty ProcessId`; - // ... recursive walk, return aggregated pids +async function snapshotProcessTreeUnix(): Promise> { + // -A: all processes (POSIX, equivalent to -e but unambiguous on BSD). + // -o pid=,ppid=: pid + ppid columns, trailing `=` suppresses headers. + const { stdout } = await execFile('ps', ['-A', '-o', 'pid=,ppid='], { + timeout: 2000, + maxBuffer: 8 * 1024 * 1024, // covers >250k-process pathological hosts + }); + const childrenByPpid = new Map(); + for (const line of stdout.split('\n')) { + const m = line.trim().match(/^(\d+)\s+(\d+)$/); + if (!m) continue; + /* parse, push into childrenByPpid */ + } + return childrenByPpid; } + +// Windows: single Get-CimInstance Win32_Process | ConvertTo-Csv snapshot +// of all (ProcessId, ParentProcessId) rows + in-memory walk; per-pid +// `Get-CimInstance -Filter "ParentProcessId=$p"` retained as fallback. ``` -Called from `PoolEntry.shutdown()` before `client.disconnect()`. Handles `npx @modelcontextprotocol/server-X`, `uvx ...`, `pnpm dlx ...` wrapper leaks. +Called from `PoolEntry.shutdown()` before `client.disconnect()`. Handles `npx @modelcontextprotocol/server-X`, `uvx ...`, `pnpm dlx ...` wrapper leaks. MAX_DESCENDANTS=256 / MAX_DEPTH=8 caps preserved. ### 6.5 Spawn failure handling diff --git a/packages/core/src/tools/pid-descendants.test.ts b/packages/core/src/tools/pid-descendants.test.ts index 1346a56757..cb21d7169c 100644 --- a/packages/core/src/tools/pid-descendants.test.ts +++ b/packages/core/src/tools/pid-descendants.test.ts @@ -59,13 +59,19 @@ describe('pid-descendants', () => { // Cross-platform integration test: spawn a wrapper that itself // spawns a child, verify listDescendantPids finds both levels. - // Gated on POSIX availability of `pgrep` (skipped on Windows + on - // CI without pgrep). + // + // F2 (#4175 commit 6 review fix — wenshao R10 / R23 T7 / PR A): + // Pre-fix gate skipped on `CI === '1'` (pgrep not always available + // on minimal CI runners). Post-fix the snapshot path uses + // `ps -A -o pid=,ppid=` (POSIX standard, available on every + // non-distroless Linux/macOS), so we keep only the Windows skip; + // the snapshot's per-pid pgrep fallback covers the rare BusyBox + // { - it('enumerates one level of children via pgrep', async () => { + it('enumerates one level of children via process-tree snapshot', async () => { // Parent process spawns a node child with `--eval` that sleeps. // Use spawn directly so we control the lifecycle. const parent = spawn('/bin/sh', [ diff --git a/packages/core/src/tools/pid-descendants.ts b/packages/core/src/tools/pid-descendants.ts index 3b028cdfb4..cbb789f60c 100644 --- a/packages/core/src/tools/pid-descendants.ts +++ b/packages/core/src/tools/pid-descendants.ts @@ -12,11 +12,22 @@ const debugLogger = createDebugLogger('PidDescendants'); const execFileAsync = promisify(execFile); /** - * Wall-clock budget for each individual `pgrep` / `Get-CimInstance` call. + * Wall-clock budget for each individual snapshot / per-pid query call. * Bounded so a hung process-table walk can't stall pool shutdown. */ const QUERY_TIMEOUT_MS = 2_000; +/** + * F2 (#4175 commit 6 review fix — wenshao R10 / R23 T7 / PR A): cap + * for `execFile`'s internal stdout buffer on the snapshot path. Default + * is 1MB, which is enough for ~30k-process hosts (~30 bytes/line) but + * an 8MB cap covers >250k-process pathological cases without forcing + * the truncation-or-fallback branch on real machines. The cap applies + * only to the snapshot family of calls; the per-pid `pgrep -P` fallback + * has a tiny output (just the children of one pid) and uses the default. + */ +const SNAPSHOT_MAXBUFFER_BYTES = 8 * 1024 * 1024; + /** * Hard cap on recursion depth + total descendants returned. Defense * against runaway process trees (forkbomb-style) or pathological @@ -35,15 +46,28 @@ const MAX_DEPTH = 8; * `uvx ...`, `pnpm dlx ...`) that would otherwise leak when the * pool entry's primary child is killed. * + * F2 (#4175 commit 6 review fix — wenshao R10 / R23 T7 / PR A): + * the implementation switched from per-pid `pgrep -P ` BFS + * (Linux/macOS) / per-pid `Get-CimInstance -Filter "ParentProcessId=$p"` + * BFS (Windows) — which forked one subprocess per node visited — to + * a single process-table snapshot followed by an in-memory tree walk. + * Two motivations: (1) ~B^D fork count → 1 fork per call, on the + * hot pool-shutdown path; (2) snapshot consistency — pre-fix BFS + * could miss descendants that forked between adjacent BFS levels. + * * Behavior: - * - Linux/macOS: `pgrep -P ` walked recursively, BFS order - * - Windows: PowerShell `Get-CimInstance Win32_Process` filtered - * by `ParentProcessId`, walked recursively - * - Either platform: graceful degradation if the tool is missing - * or the query times out — returns whatever was collected so far - * and logs a warning. Pool shutdown still proceeds; orphan - * processes will be reaped by the OS eventually (Linux init, - * Windows job objects). + * - Linux/macOS: `ps -A -o pid=,ppid=` snapshot, in-memory BFS walk + * over the parsed `Map`. + * - Windows: PowerShell `Get-CimInstance Win32_Process` → + * `ConvertTo-Csv` snapshot of all `(ProcessId, ParentProcessId)` + * rows, in-memory walk. + * - Either platform: graceful degradation if the snapshot tool is + * missing / blocked / times out — falls back to per-pid BFS + * (preserves the pre-fix code path so BusyBox `ps` { } async function listDescendantPidsUnix(root: number): Promise { + let tree: Map | undefined; + try { + tree = await snapshotProcessTreeUnix(); + } catch (err) { + debugLogger.warn( + `Unix snapshot via 'ps -A' failed (${ + err instanceof Error ? err.message : String(err) + }); falling back to per-pid pgrep BFS`, + ); + } + if (tree) { + return walkDescendants(tree, root); + } + return await listDescendantPidsUnixPgrepFallback(root); +} + +async function snapshotProcessTreeUnix(): Promise> { + // `ps -A -o pid=,ppid=` + // -A: all processes (POSIX, equivalent to -e; -A is unambiguous + // across BSD/SysV — BSD historically used -e for env display). + // -o pid=,ppid=: pid + ppid columns; trailing `=` suppresses each + // column header (POSIX standard). + // Output is " " per line, no header. + const { stdout } = await execFileAsync('ps', ['-A', '-o', 'pid=,ppid='], { + timeout: QUERY_TIMEOUT_MS, + maxBuffer: SNAPSHOT_MAXBUFFER_BYTES, + }); + const childrenByPpid = new Map(); + let parsedRows = 0; + for (const line of stdout.split('\n')) { + const m = line.trim().match(/^(\d+)\s+(\d+)$/); + if (!m) continue; + const pid = Number.parseInt(m[1], 10); + const ppid = Number.parseInt(m[2], 10); + if (!Number.isFinite(pid) || pid <= 0) continue; + if (!Number.isFinite(ppid) || ppid < 0) continue; + parsedRows += 1; + const arr = childrenByPpid.get(ppid); + if (arr) arr.push(pid); + else childrenByPpid.set(ppid, [pid]); + } + if (parsedRows === 0) { + // Snapshot tool ran but produced no parseable lines (e.g. BusyBox + // `ps` without `-o` support echoing usage). Treat as failure so + // the caller's catch falls back to per-pid pgrep. + throw new Error( + `'ps -A -o pid=,ppid=' returned no parseable rows (stdout length=${stdout.length})`, + ); + } + return childrenByPpid; +} + +async function listDescendantPidsUnixPgrepFallback( + root: number, +): Promise { const all: number[] = []; const queue: Array<{ pid: number; depth: number }> = [ { pid: root, depth: 0 }, @@ -107,6 +186,65 @@ async function listDescendantPidsUnix(root: number): Promise { } async function listDescendantPidsWin(root: number): Promise { + let tree: Map | undefined; + try { + tree = await snapshotProcessTreeWin(); + } catch (err) { + debugLogger.warn( + `Windows snapshot via Get-CimInstance failed (${ + err instanceof Error ? err.message : String(err) + }); falling back to per-pid filter BFS`, + ); + } + if (tree) { + return walkDescendants(tree, root); + } + return await listDescendantPidsWinPerPidFallback(root); +} + +async function snapshotProcessTreeWin(): Promise> { + // Single-shot CIM query for ALL processes' (ProcessId, + // ParentProcessId), CSV-formatted for stable parsing. + // F2 (#4175 commit 5 review fix — wenshao R5): no integer + // interpolation into the script; this query takes no parameters + // (we filter in-memory after the snapshot returns). + const script = + 'Get-CimInstance -ClassName Win32_Process ' + + '| Select-Object ProcessId,ParentProcessId ' + + '| ConvertTo-Csv -NoTypeInformation'; + const { stdout } = await execFileAsync( + 'powershell.exe', + ['-NoProfile', '-NonInteractive', '-Command', script], + { timeout: QUERY_TIMEOUT_MS, maxBuffer: SNAPSHOT_MAXBUFFER_BYTES }, + ); + const childrenByPpid = new Map(); + let parsedRows = 0; + // CSV: first line is header `"ProcessId","ParentProcessId"`, + // subsequent lines are `"",""`. + const lines = stdout.split(/\r?\n/); + for (let i = 1; i < lines.length; i++) { + const m = lines[i].match(/^"(\d+)","(\d+)"$/); + if (!m) continue; + const pid = Number.parseInt(m[1], 10); + const ppid = Number.parseInt(m[2], 10); + if (!Number.isFinite(pid) || pid <= 0) continue; + if (!Number.isFinite(ppid) || ppid < 0) continue; + parsedRows += 1; + const arr = childrenByPpid.get(ppid); + if (arr) arr.push(pid); + else childrenByPpid.set(ppid, [pid]); + } + if (parsedRows === 0) { + throw new Error( + `Get-CimInstance snapshot returned no parseable rows (stdout length=${stdout.length})`, + ); + } + return childrenByPpid; +} + +async function listDescendantPidsWinPerPidFallback( + root: number, +): Promise { const all: number[] = []; const queue: Array<{ pid: number; depth: number }> = [ { pid: root, depth: 0 }, @@ -161,6 +299,32 @@ async function listDescendantPidsWin(root: number): Promise { return all; } +/** + * F2 (#4175 commit 6 review fix — wenshao R10 / R23 T7 / PR A): + * shared in-memory BFS over a snapshot tree. Replaces both + * platforms' per-node subprocess forks once the snapshot has been + * obtained. Same MAX_DESCENDANTS / MAX_DEPTH caps as the legacy + * fallback path. Returns BFS order — children before grandchildren. + */ +function walkDescendants(tree: Map, root: number): number[] { + const all: number[] = []; + const queue: Array<{ pid: number; depth: number }> = [ + { pid: root, depth: 0 }, + ]; + while (queue.length && all.length < MAX_DESCENDANTS) { + const { pid, depth } = queue.shift()!; + if (depth >= MAX_DEPTH) continue; + const children = tree.get(pid); + if (!children) continue; + for (const child of children) { + if (all.length >= MAX_DESCENDANTS) break; + all.push(child); + queue.push({ pid: child, depth: depth + 1 }); + } + } + return all; +} + /** * Send SIGTERM to a list of pids, tolerating per-pid failures * (already exited, permission denied, etc.). On Windows, Node's