From d830d5c8aeb4896b6b9d0591adf079c0e7da7978 Mon Sep 17 00:00:00 2001 From: JF Date: Fri, 21 Aug 2026 23:15:17 -0400 Subject: [PATCH] perf(startup): scope orphan reapers to server commands, share one process scan, add opt-out MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The two orphan reapers ran before argv parsing (even --version paid the double scan), blocked startup, and walked /proc / ran ps independently over identical data (issue #399). Now: - src/utils/process-scan.ts produces (pid, args) rows once — one bounded /proc walk on linux, one ps -ww -A on darwin; win32 stays one filtered CIM query per process name (unfiltered would cost more). The reapers' platform listers are thin wrappers applying their marker matchers. - src/utils/startup-janitor.ts orchestrates: one scan feeds both reapers via their existing lister seams; kicked fire-and-forget from the stdio/sse/http command actions only, so the transport comes up immediately and non-server invocations never scan. Safe concurrently: reapers only kill processes whose recorded owner pid is dead. - MCP_SKIP_ORPHAN_REAPERS=1 opt-out for PID-namespaced containers. - The janitor also sweeps stale session run dirs (proxy-*.log / dap-trace-*.ndjson older than 7 days) under the tmpdir session log base — the follow-through deferred from #403. Co-Authored-By: Claude Fable 5 --- docs/development/setup-guide.md | 1 + src/index.ts | 49 +++---- src/utils/jvm-orphan-reaper.ts | 113 +++------------ src/utils/process-scan.ts | 134 ++++++++++++++++++ src/utils/proxy-orphan-reaper.ts | 109 +++------------ src/utils/startup-janitor.ts | 169 +++++++++++++++++++++++ tests/unit/index.test.ts | 27 ++++ tests/unit/utils/process-scan.test.ts | 130 +++++++++++++++++ tests/unit/utils/startup-janitor.test.ts | 152 ++++++++++++++++++++ 9 files changed, 664 insertions(+), 220 deletions(-) create mode 100644 src/utils/process-scan.ts create mode 100644 src/utils/startup-janitor.ts create mode 100644 tests/unit/utils/process-scan.test.ts create mode 100644 tests/unit/utils/startup-janitor.test.ts diff --git a/docs/development/setup-guide.md b/docs/development/setup-guide.md index 0d981581..42bc1b89 100644 --- a/docs/development/setup-guide.md +++ b/docs/development/setup-guide.md @@ -314,6 +314,7 @@ TEST_TIMEOUT=30000 | `DEBUG` | Enable debug output (e.g., `DEBUG=debug-mcp:*`) | Not set | | `DAP_TRACE` | Set to `1` to trace every DAP frame to a per-session `dap-trace-.ndjson` (capped at 50 MB) | Not set | | `DAP_TRACE_FILE` | Explicit DAP trace file path (implies tracing on) | Not set | +| `MCP_SKIP_ORPHAN_REAPERS` | Set to `1` to skip the startup orphan-process scans (e.g. PID-namespaced containers where orphans are impossible) | Not set | ## Troubleshooting Setup Issues diff --git a/src/index.ts b/src/index.ts index b0b0f0dc..1ec9d9ee 100644 --- a/src/index.ts +++ b/src/index.ts @@ -39,8 +39,7 @@ process.argv = process.argv.map(arg => ); import { createLogger } from './utils/logger.js'; -import { reapOrphanJvms } from './utils/jvm-orphan-reaper.js'; -import { reapOrphanProxies } from './utils/proxy-orphan-reaper.js'; +import { runStartupJanitor } from './utils/startup-janitor.js'; import { DebugMcpServer } from './server.js'; import { setupErrorHandlers } from './cli/error-handlers.js'; import { @@ -97,30 +96,17 @@ export async function main(): Promise { // below uses this to decide which orphans from prior runs are ours to kill. process.env.MCP_DEBUGGER_MAIN_PID = String(process.pid); - // Best-effort cleanup of debuggee JVMs and proxy worker chains leaked by - // prior crashed runs. Awaited synchronously so a fresh server starts in a - // known-clean state; the two reapers run concurrently so their worst-case - // process-listing timeouts don't stack. Failures here must never block - // startup — both functions are designed not to throw, and allSettled is - // belt-and-suspenders. - const [jvmOutcome, proxyOutcome] = await Promise.allSettled([ - reapOrphanJvms({ selfPid: process.pid, logger }), - reapOrphanProxies({ selfPid: process.pid, logger }) - ]); - if (jvmOutcome.status === 'fulfilled') { - if (jvmOutcome.value.killed.length > 0) { - logger.info(`[startup] Reaped ${jvmOutcome.value.killed.length} orphan JVM(s) from prior runs`); - } - } else { - logger.warn(`[startup] Orphan JVM reaper failed: ${(jvmOutcome.reason as Error)?.message ?? String(jvmOutcome.reason)}`); - } - if (proxyOutcome.status === 'fulfilled') { - if (proxyOutcome.value.killed.length > 0) { - logger.info(`[startup] Reaped ${proxyOutcome.value.killed.length} orphan proxy worker(s) from prior runs`); - } - } else { - logger.warn(`[startup] Orphan proxy reaper failed: ${(proxyOutcome.reason as Error)?.message ?? String(proxyOutcome.reason)}`); - } + // Best-effort cleanup of debris from prior crashed runs (orphan JVMs and + // proxy workers, stale session logs) runs fire-and-forget from the + // server-starting command actions below — never for --version/--help, and + // never blocking the transport from coming up (issue #399). Safe alongside + // the live server: reapers only kill processes whose recorded owner pid is + // dead. MCP_SKIP_ORPHAN_REAPERS=1 skips the process scans entirely. + const kickStartupJanitor = () => { + void runStartupJanitor({ logger }).catch(() => { + // runStartupJanitor logs its own failures; this guard is belt-and-suspenders. + }); + }; // Setup error handlers setupErrorHandlers({ logger }); @@ -129,19 +115,22 @@ export async function main(): Promise { const program = createCLI('debug-mcp-server', 'Step-through debugging MCP server for LLMs', getVersion()); // Setup commands - setupStdioCommand(program, (options) => - handleStdioCommand(options, { logger, serverFactory: createDebugMcpServer }) - ); - + setupStdioCommand(program, (options) => { + kickStartupJanitor(); + return handleStdioCommand(options, { logger, serverFactory: createDebugMcpServer }); + }); + // The SSE/HTTP command modules pull in express and the SDK's HTTP transport // stacks; import them only when their subcommand actually runs so stdio mode // (the common case) never pays for them (issue #400). setupSSECommand(program, async (options) => { + kickStartupJanitor(); const { handleSSECommand } = await import('./cli/sse-command.js'); return handleSSECommand(options, { logger, serverFactory: createDebugMcpServer }); }); setupHttpCommand(program, async (options) => { + kickStartupJanitor(); const { handleHttpCommand } = await import('./cli/http-command.js'); return handleHttpCommand(options, { logger, serverFactory: createDebugMcpServer }); }); diff --git a/src/utils/jvm-orphan-reaper.ts b/src/utils/jvm-orphan-reaper.ts index 403db43c..f3651d15 100644 --- a/src/utils/jvm-orphan-reaper.ts +++ b/src/utils/jvm-orphan-reaper.ts @@ -16,21 +16,12 @@ * Only listing tagged JVMs is platform-divergent. The kill path uses Node's * portable process.kill, which maps to TerminateProcess on Windows. */ -import { execFile } from 'node:child_process'; -import { promisify } from 'node:util'; -import * as fs from 'node:fs/promises'; -import { forEachBounded } from './bounded-concurrency.js'; -import { PROC_SCAN_CONCURRENCY } from './proc-scan-concurrency.js'; - -const execFileAsync = promisify(execFile); +import { scanLinux, scanDarwin, scanWindows, scanProcessArgs, type ScannedProcess } from './process-scan.js'; const JVM_MARKER = '-Dmcp.debugger.jvm=true'; const OWNER_PID_PREFIX = '-Dmcp.debugger.owner_pid='; const SESSION_TAG_PREFIX = '-Dmcp.debugger.session_tag='; -const LIST_TIMEOUT_MS = 5000; -const LIST_MAX_BUFFER = 10 * 1024 * 1024; - export interface TaggedJvm { pid: number; ownerPid: number; @@ -107,106 +98,34 @@ export async function reapOrphanJvms(opts: ReapOptions): Promise { return result; } -export async function listTaggedJvms(): Promise { - switch (process.platform) { - case 'linux': - return listLinux(); - case 'darwin': - return listDarwin(); - case 'win32': - return listWindows(); - default: - return []; +// The platform walks live in process-scan.ts (issue #399: shared with the +// proxy reaper); these wrappers apply the JVM matcher over the scan rows. +function matchTaggedJvms(processes: ScannedProcess[]): TaggedJvm[] { + const result: TaggedJvm[] = []; + for (const p of processes) { + const tagged = parseArgs(p.pid, p.args); + if (tagged) result.push(tagged); } + return result; +} + +export async function listTaggedJvms(): Promise { + return matchTaggedJvms(await scanProcessArgs({ windowsProcessNames: ['java.exe'] })); } /** @internal Exposed for unit tests; not part of the public module API. */ export async function listLinux(): Promise { - let entries: string[]; - try { - entries = await fs.readdir('/proc'); - } catch { - return []; - } - const result: TaggedJvm[] = []; - await forEachBounded(entries, PROC_SCAN_CONCURRENCY, async (entry) => { - if (!/^\d+$/.test(entry)) return; - const pid = Number(entry); - let raw: string; - try { - raw = await fs.readFile(`/proc/${entry}/cmdline`, 'utf8'); - } catch { - return; // disappeared, or permission denied - } - const args = raw.split('\0').filter((s) => s.length > 0); - const tagged = parseArgs(pid, args); - if (tagged) result.push(tagged); - }); - return result; + return matchTaggedJvms(await scanLinux()); } /** @internal Exposed for unit tests; not part of the public module API. */ export async function listDarwin(): Promise { - // -ww disables column truncation; otherwise long java cmdlines lose the - // -D markers we depend on. -A lists all users' processes (we filter by - // owner_pid liveness anyway). - const { stdout } = await execFileAsync('ps', ['-ww', '-A', '-o', 'pid=,command='], { - timeout: LIST_TIMEOUT_MS, - maxBuffer: LIST_MAX_BUFFER, - }); - const result: TaggedJvm[] = []; - for (const line of stdout.split('\n')) { - const trimmed = line.replace(/\s+$/, ''); - if (!trimmed) continue; - const match = trimmed.match(/^\s*(\d+)\s+(.*)$/); - if (!match) continue; - const pid = Number(match[1]); - const args = match[2].split(/\s+/).filter(Boolean); - const tagged = parseArgs(pid, args); - if (tagged) result.push(tagged); - } - return result; + return matchTaggedJvms(await scanDarwin()); } /** @internal Exposed for unit tests; not part of the public module API. */ export async function listWindows(): Promise { - // Get-CimInstance is the modern path; wmic is deprecated and missing on - // fresh Windows 11 installs. ConvertTo-Json -Compress keeps stdout small. - // -NoProfile skips loading user profile scripts (faster, more deterministic). - const ps = `Get-CimInstance Win32_Process -Filter "Name='java.exe'" | Select-Object ProcessId, CommandLine | ConvertTo-Json -Compress`; - let stdout: string; - try { - const r = await execFileAsync('powershell.exe', ['-NoProfile', '-Command', ps], { - timeout: LIST_TIMEOUT_MS, - maxBuffer: LIST_MAX_BUFFER, - windowsHide: true, - }); - stdout = r.stdout; - } catch { - return []; - } - const trimmed = stdout.trim(); - if (!trimmed) return []; - let parsed: unknown; - try { - parsed = JSON.parse(trimmed); - } catch { - return []; - } - const items = Array.isArray(parsed) ? parsed : [parsed]; - const result: TaggedJvm[] = []; - for (const item of items) { - if (!item || typeof item !== 'object') continue; - const obj = item as { ProcessId?: number; CommandLine?: string | null }; - const pid = obj.ProcessId; - const cmdline = obj.CommandLine; - if (typeof pid !== 'number' || typeof cmdline !== 'string') continue; - // -D args don't contain unescaped whitespace, so naive split is enough. - const args = cmdline.split(/\s+/).filter(Boolean); - const tagged = parseArgs(pid, args); - if (tagged) result.push(tagged); - } - return result; + return matchTaggedJvms(await scanWindows(['java.exe'])); } /** @internal Exposed for unit tests; not part of the public module API. */ diff --git a/src/utils/process-scan.ts b/src/utils/process-scan.ts new file mode 100644 index 00000000..3b47b738 --- /dev/null +++ b/src/utils/process-scan.ts @@ -0,0 +1,134 @@ +/** + * Shared cross-platform process scan (issue #399). + * + * Both orphan reapers need the same raw data — (pid, argv) for running + * processes — and used to gather it independently: two /proc walks on Linux, + * two identical `ps -ww -A` execs on Darwin. One scan now produces the rows + * and each reaper contributes a matcher over them. Windows stays one + * name-filtered CIM query per process name (java.exe / node.exe): a single + * unfiltered Win32_Process query would be strictly more expensive than the + * two filtered ones. + */ +import { execFile } from 'node:child_process'; +import { promisify } from 'node:util'; +import * as fs from 'node:fs/promises'; +import { forEachBounded } from './bounded-concurrency.js'; +import { PROC_SCAN_CONCURRENCY } from './proc-scan-concurrency.js'; + +const execFileAsync = promisify(execFile); + +const LIST_TIMEOUT_MS = 5000; +const LIST_MAX_BUFFER = 10 * 1024 * 1024; + +export interface ScannedProcess { + pid: number; + args: string[]; +} + +export interface ProcessScanOptions { + /** win32 only: Win32_Process Name filters, one CIM query per name. */ + windowsProcessNames: string[]; +} + +export async function scanProcessArgs(options: ProcessScanOptions): Promise { + switch (process.platform) { + case 'linux': + return scanLinux(); + case 'darwin': + return scanDarwin(); + case 'win32': + return scanWindows(options.windowsProcessNames); + default: + return []; + } +} + +/** @internal Exposed for unit tests; not part of the public module API. */ +export async function scanLinux(): Promise { + let entries: string[]; + try { + entries = await fs.readdir('/proc'); + } catch { + return []; + } + const result: ScannedProcess[] = []; + await forEachBounded(entries, PROC_SCAN_CONCURRENCY, async (entry) => { + if (!/^\d+$/.test(entry)) return; + const pid = Number(entry); + let raw: string; + try { + raw = await fs.readFile(`/proc/${entry}/cmdline`, 'utf8'); + } catch { + return; // disappeared, or permission denied + } + const args = raw.split('\0').filter((s) => s.length > 0); + result.push({ pid, args }); + }); + return result; +} + +/** @internal Exposed for unit tests; not part of the public module API. */ +export async function scanDarwin(): Promise { + // -ww disables column truncation; otherwise long cmdlines lose the marker + // args the reapers depend on. -A lists all users' processes (matchers + // filter by owner_pid liveness anyway). + const { stdout } = await execFileAsync('ps', ['-ww', '-A', '-o', 'pid=,command='], { + timeout: LIST_TIMEOUT_MS, + maxBuffer: LIST_MAX_BUFFER, + }); + const result: ScannedProcess[] = []; + for (const line of stdout.split('\n')) { + const trimmed = line.replace(/\s+$/, ''); + if (!trimmed) continue; + const match = trimmed.match(/^\s*(\d+)\s+(.*)$/); + if (!match) continue; + const pid = Number(match[1]); + const args = match[2].split(/\s+/).filter(Boolean); + result.push({ pid, args }); + } + return result; +} + +/** @internal Exposed for unit tests; not part of the public module API. */ +export async function scanWindows(processNames: string[]): Promise { + const result: ScannedProcess[] = []; + for (const name of processNames) { + // Get-CimInstance is the modern path; wmic is deprecated and missing on + // fresh Windows 11 installs. ConvertTo-Json -Compress keeps stdout small. + // -NoProfile skips loading user profile scripts (faster, more deterministic). + const ps = `Get-CimInstance Win32_Process -Filter "Name='${name}'" | Select-Object ProcessId, CommandLine | ConvertTo-Json -Compress`; + let stdout: string; + try { + const r = await execFileAsync('powershell.exe', ['-NoProfile', '-Command', ps], { + timeout: LIST_TIMEOUT_MS, + maxBuffer: LIST_MAX_BUFFER, + windowsHide: true, + }); + stdout = r.stdout; + } catch { + continue; // one failing query must not fail the others + } + const trimmed = stdout.trim(); + if (!trimmed) continue; + let parsed: unknown; + try { + parsed = JSON.parse(trimmed); + } catch { + continue; + } + const items = Array.isArray(parsed) ? parsed : [parsed]; + for (const item of items) { + if (!item || typeof item !== 'object') continue; + const obj = item as { ProcessId?: number; CommandLine?: string | null }; + const pid = obj.ProcessId; + const cmdline = obj.CommandLine; + if (typeof pid !== 'number' || typeof cmdline !== 'string') continue; + // Naive whitespace split is enough for the reapers' marker args: they + // never contain whitespace, and a fragmenting path keeps the fragment + // holding the marker intact. + const args = cmdline.split(/\s+/).filter(Boolean); + result.push({ pid, args }); + } + } + return result; +} diff --git a/src/utils/proxy-orphan-reaper.ts b/src/utils/proxy-orphan-reaper.ts index da4e3801..233dfba0 100644 --- a/src/utils/proxy-orphan-reaper.ts +++ b/src/utils/proxy-orphan-reaper.ts @@ -32,10 +32,8 @@ */ import { execFile } from 'node:child_process'; import { promisify } from 'node:util'; -import * as fs from 'node:fs/promises'; import { isPidAlive, SignalFn } from './jvm-orphan-reaper.js'; -import { forEachBounded } from './bounded-concurrency.js'; -import { PROC_SCAN_CONCURRENCY } from './proc-scan-concurrency.js'; +import { scanLinux, scanDarwin, scanWindows, scanProcessArgs, type ScannedProcess } from './process-scan.js'; const execFileAsync = promisify(execFile); @@ -45,7 +43,6 @@ export const OWNER_PID_ARG_PREFIX = '--mcp-owner-pid='; export const SESSION_ID_ARG_PREFIX = '--mcp-session-id='; const LIST_TIMEOUT_MS = 5000; -const LIST_MAX_BUFFER = 10 * 1024 * 1024; /** How often the POSIX escalation path polls for the SIGTERM cascade to finish. */ const TERM_POLL_MS = 200; /** Bounded wait before SIGKILL escalation; must exceed the ~2s #339 cascade. */ @@ -143,108 +140,34 @@ export async function reapOrphanProxies(opts: ReapOptions): Promise return result; } -export async function listTaggedProxies(): Promise { - switch (process.platform) { - case 'linux': - return listLinuxProxies(); - case 'darwin': - return listDarwinProxies(); - case 'win32': - return listWindowsProxies(); - default: - return []; +// The platform walks live in process-scan.ts (issue #399: shared with the +// JVM reaper); these wrappers apply the proxy matcher over the scan rows. +function matchTaggedProxies(processes: ScannedProcess[]): TaggedProxy[] { + const result: TaggedProxy[] = []; + for (const p of processes) { + const tagged = parseProxyArgs(p.pid, p.args); + if (tagged) result.push(tagged); } + return result; +} + +export async function listTaggedProxies(): Promise { + return matchTaggedProxies(await scanProcessArgs({ windowsProcessNames: ['node.exe'] })); } /** @internal Exposed for unit tests; not part of the public module API. */ export async function listLinuxProxies(): Promise { - let entries: string[]; - try { - entries = await fs.readdir('/proc'); - } catch { - return []; - } - const result: TaggedProxy[] = []; - await forEachBounded(entries, PROC_SCAN_CONCURRENCY, async (entry) => { - if (!/^\d+$/.test(entry)) return; - const pid = Number(entry); - let raw: string; - try { - raw = await fs.readFile(`/proc/${entry}/cmdline`, 'utf8'); - } catch { - return; // disappeared, or permission denied - } - const args = raw.split('\0').filter((s) => s.length > 0); - const tagged = parseProxyArgs(pid, args); - if (tagged) result.push(tagged); - }); - return result; + return matchTaggedProxies(await scanLinux()); } /** @internal Exposed for unit tests; not part of the public module API. */ export async function listDarwinProxies(): Promise { - // -ww disables column truncation; otherwise long node cmdlines lose the - // markers we depend on. -A lists all users' processes (we filter by - // owner_pid liveness anyway). - const { stdout } = await execFileAsync('ps', ['-ww', '-A', '-o', 'pid=,command='], { - timeout: LIST_TIMEOUT_MS, - maxBuffer: LIST_MAX_BUFFER, - }); - const result: TaggedProxy[] = []; - for (const line of stdout.split('\n')) { - const trimmed = line.replace(/\s+$/, ''); - if (!trimmed) continue; - const match = trimmed.match(/^\s*(\d+)\s+(.*)$/); - if (!match) continue; - const pid = Number(match[1]); - const args = match[2].split(/\s+/).filter(Boolean); - const tagged = parseProxyArgs(pid, args); - if (tagged) result.push(tagged); - } - return result; + return matchTaggedProxies(await scanDarwin()); } /** @internal Exposed for unit tests; not part of the public module API. */ export async function listWindowsProxies(): Promise { - // Get-CimInstance is the modern path; wmic is deprecated and missing on - // fresh Windows 11 installs. ConvertTo-Json -Compress keeps stdout small. - // -NoProfile skips loading user profile scripts (faster, more deterministic). - const ps = `Get-CimInstance Win32_Process -Filter "Name='node.exe'" | Select-Object ProcessId, CommandLine | ConvertTo-Json -Compress`; - let stdout: string; - try { - const r = await execFileAsync('powershell.exe', ['-NoProfile', '-Command', ps], { - timeout: LIST_TIMEOUT_MS, - maxBuffer: LIST_MAX_BUFFER, - windowsHide: true, - }); - stdout = r.stdout; - } catch { - return []; - } - const trimmed = stdout.trim(); - if (!trimmed) return []; - let parsed: unknown; - try { - parsed = JSON.parse(trimmed); - } catch { - return []; - } - const items = Array.isArray(parsed) ? parsed : [parsed]; - const result: TaggedProxy[] = []; - for (const item of items) { - if (!item || typeof item !== 'object') continue; - const obj = item as { ProcessId?: number; CommandLine?: string | null }; - const pid = obj.ProcessId; - const cmdline = obj.CommandLine; - if (typeof pid !== 'number' || typeof cmdline !== 'string') continue; - // Naive whitespace split is enough: a path containing spaces fragments, - // but the fragment holding 'proxy-bootstrap.js' survives intact, and the - // --mcp-* marker args never contain whitespace. - const args = cmdline.split(/\s+/).filter(Boolean); - const tagged = parseProxyArgs(pid, args); - if (tagged) result.push(tagged); - } - return result; + return matchTaggedProxies(await scanWindows(['node.exe'])); } /** diff --git a/src/utils/startup-janitor.ts b/src/utils/startup-janitor.ts new file mode 100644 index 00000000..182faf2e --- /dev/null +++ b/src/utils/startup-janitor.ts @@ -0,0 +1,169 @@ +/** + * Startup janitor (issue #399): best-effort cleanup of debris from prior + * crashed runs, invoked fire-and-forget from the server-starting CLI commands + * (stdio/http/sse) — never from --version/--help, and never blocking the + * transport from coming up. + * + * Covers: + * - Orphan debuggee JVMs and proxy workers, via ONE shared process scan + * feeding both reapers' matchers (previously two independent walks). + * `MCP_SKIP_ORPHAN_REAPERS=1` skips this part for PID-namespaced containers + * where orphans are impossible. + * - Stale per-session run logs (proxy-.log / dap-trace-.ndjson) under + * the session log base dir (deferred here from issue #403 — the flat + * logger.ts sweeper can't reach these nested dirs). + * + * Safe to run concurrently with a live server: the reapers only kill + * processes whose recorded owner pid is dead, and the sweep only removes run + * dirs whose mtime is older than the age cap (a live session keeps writing). + */ +import * as fsp from 'node:fs/promises'; +import os from 'node:os'; +import path from 'node:path'; +import { scanProcessArgs, type ProcessScanOptions, type ScannedProcess } from './process-scan.js'; +import { reapOrphanJvms, parseArgs, type ReapResult as JvmReapResult, type ReapOptions as JvmReapOptions } from './jvm-orphan-reaper.js'; +import { reapOrphanProxies, parseProxyArgs, type ReapResult as ProxyReapResult, type ReapOptions as ProxyReapOptions } from './proxy-orphan-reaper.js'; + +export interface JanitorLogger { + info?: (msg: string) => void; + warn?: (msg: string) => void; + error?: (msg: string) => void; + debug?: (msg: string) => void; +} + +export interface StartupJanitorOptions { + logger: JanitorLogger; + selfPid?: number; + env?: NodeJS.ProcessEnv; + // Test seams + scan?: (options: ProcessScanOptions) => Promise; + reapJvms?: (opts: JvmReapOptions) => Promise; + reapProxies?: (opts: ProxyReapOptions) => Promise; + sweep?: (opts: SweepOptions) => Promise; +} + +export async function runStartupJanitor(opts: StartupJanitorOptions): Promise { + const log = opts.logger; + const env = opts.env ?? process.env; + const selfPid = opts.selfPid ?? process.pid; + + const skipFlag = (env.MCP_SKIP_ORPHAN_REAPERS ?? '').toLowerCase(); + if (skipFlag === '1' || skipFlag === 'true') { + log.debug?.('[startup-janitor] Orphan reapers skipped (MCP_SKIP_ORPHAN_REAPERS)'); + } else { + await runOrphanReapers(opts, selfPid); + } + + try { + const sweep = opts.sweep ?? sweepStaleSessionRuns; + const swept = await sweep({ logger: log }); + if (swept.removedRuns > 0) { + log.info?.(`[startup-janitor] Removed ${swept.removedRuns} stale session run dir(s)`); + } + } catch (e) { + log.warn?.(`[startup-janitor] Session log sweep failed: ${e instanceof Error ? e.message : String(e)}`); + } +} + +async function runOrphanReapers(opts: StartupJanitorOptions, selfPid: number): Promise { + const log = opts.logger; + const scan = opts.scan ?? scanProcessArgs; + + let processes: ScannedProcess[] = []; + try { + processes = await scan({ windowsProcessNames: ['java.exe', 'node.exe'] }); + } catch (e) { + log.warn?.(`[startup-janitor] Process scan failed: ${e instanceof Error ? e.message : String(e)}`); + return; + } + + // Matchers are marker-based, so feeding every row to both is harmless — + // JVM -D markers only appear in java cmdlines and vice versa. + const jvmLister = async () => + processes.map((p) => parseArgs(p.pid, p.args)).filter((t): t is NonNullable => t !== null); + const proxyLister = async () => + processes.map((p) => parseProxyArgs(p.pid, p.args)).filter((t): t is NonNullable => t !== null); + + const [jvmOutcome, proxyOutcome] = await Promise.allSettled([ + (opts.reapJvms ?? reapOrphanJvms)({ selfPid, logger: log, lister: jvmLister }), + (opts.reapProxies ?? reapOrphanProxies)({ selfPid, logger: log, lister: proxyLister }), + ]); + + if (jvmOutcome.status === 'fulfilled') { + if (jvmOutcome.value.killed.length > 0) { + log.info?.(`[startup-janitor] Reaped ${jvmOutcome.value.killed.length} orphan JVM(s) from prior runs`); + } + } else { + log.warn?.(`[startup-janitor] Orphan JVM reaper failed: ${(jvmOutcome.reason as Error)?.message ?? String(jvmOutcome.reason)}`); + } + if (proxyOutcome.status === 'fulfilled') { + if (proxyOutcome.value.killed.length > 0) { + log.info?.(`[startup-janitor] Reaped ${proxyOutcome.value.killed.length} orphan proxy worker(s) from prior runs`); + } + } else { + log.warn?.(`[startup-janitor] Orphan proxy reaper failed: ${(proxyOutcome.reason as Error)?.message ?? String(proxyOutcome.reason)}`); + } +} + +/** Delete stale per-session run log dirs after this long (matches logger.ts). */ +const STALE_RUN_MAX_AGE_MS = 7 * 24 * 60 * 60 * 1000; + +export interface SweepOptions { + logger?: JanitorLogger; + /** Session log base dir; defaults to SessionManagerCore's default. */ + baseDir?: string; + maxAgeMs?: number; + now?: number; +} + +export interface SweepResult { + removedRuns: number; +} + +/** + * Remove `run-` dirs (holding proxy-.log / dap-trace-.ndjson) + * whose mtime is older than the age cap, then remove session dirs left empty. + * Best-effort throughout: a live session's run dir has a fresh mtime (the + * proxy logger writes to it), and rmdir on a non-empty dir simply fails. + */ +export async function sweepStaleSessionRuns(opts: SweepOptions = {}): Promise { + const baseDir = opts.baseDir ?? path.join(os.tmpdir(), 'debug-mcp-server', 'sessions'); + const maxAgeMs = opts.maxAgeMs ?? STALE_RUN_MAX_AGE_MS; + const now = opts.now ?? Date.now(); + const result: SweepResult = { removedRuns: 0 }; + + let sessionDirs: string[]; + try { + sessionDirs = await fsp.readdir(baseDir); + } catch { + return result; + } + + for (const sessionName of sessionDirs) { + const sessionPath = path.join(baseDir, sessionName); + let runNames: string[]; + try { + runNames = await fsp.readdir(sessionPath); + } catch { + continue; // not a directory, or vanished + } + for (const runName of runNames) { + if (!runName.startsWith('run-')) continue; + const runPath = path.join(sessionPath, runName); + try { + const stat = await fsp.stat(runPath); + if (!stat.isDirectory() || now - stat.mtimeMs < maxAgeMs) continue; + await fsp.rm(runPath, { recursive: true, force: true }); + result.removedRuns++; + } catch { + // vanished or locked (e.g. Windows open handle) — leave it + } + } + try { + await fsp.rmdir(sessionPath); // only succeeds when empty + } catch { + // still has fresh runs or non-run entries — fine + } + } + return result; +} diff --git a/tests/unit/index.test.ts b/tests/unit/index.test.ts index 313c9af9..1bf46b96 100644 --- a/tests/unit/index.test.ts +++ b/tests/unit/index.test.ts @@ -7,8 +7,10 @@ import * as setup from '../../src/cli/setup.js'; import * as stdioCommand from '../../src/cli/stdio-command.js'; import * as sseCommand from '../../src/cli/sse-command.js'; import * as version from '../../src/cli/version.js'; +import { runStartupJanitor } from '../../src/utils/startup-janitor.js'; vi.mock('../../src/utils/logger.js'); +vi.mock('../../src/utils/startup-janitor.js'); vi.mock('../../src/server.js'); vi.mock('../../src/cli/error-handlers.js'); vi.mock('../../src/cli/setup.js'); @@ -49,6 +51,7 @@ describe('index.ts', () => { // Mock command handlers vi.mocked(stdioCommand.handleStdioCommand).mockResolvedValue(undefined); vi.mocked(sseCommand.handleSSECommand).mockResolvedValue(undefined); + vi.mocked(runStartupJanitor).mockResolvedValue(undefined); }); describe('createDebugMcpServer', () => { @@ -111,6 +114,30 @@ describe('index.ts', () => { ); }); + it('does not run the startup janitor for non-server invocations (issue #399)', async () => { + // main() sets up commands but parseAsync is mocked — no command action + // runs, so the janitor (orphan reapers + log sweep) must not either. + // Before #399 the reapers ran before argv parsing, so even --version paid + // the process scans. + await main(); + + expect(runStartupJanitor).not.toHaveBeenCalled(); + }); + + it('kicks the startup janitor when the stdio command action runs (issue #399)', async () => { + let capturedHandler: any; + vi.mocked(setup.setupStdioCommand).mockImplementation((program, handler) => { + capturedHandler = handler; + }); + + await main(); + expect(runStartupJanitor).not.toHaveBeenCalled(); + + await capturedHandler({ logLevel: 'debug' }); + + expect(runStartupJanitor).toHaveBeenCalledTimes(1); + }); + it('should pass correct handlers to setupSSECommand', async () => { let capturedHandler: any; vi.mocked(setup.setupSSECommand).mockImplementation((program, handler) => { diff --git a/tests/unit/utils/process-scan.test.ts b/tests/unit/utils/process-scan.test.ts new file mode 100644 index 00000000..16aeaf8c --- /dev/null +++ b/tests/unit/utils/process-scan.test.ts @@ -0,0 +1,130 @@ +/** + * Unit tests for the shared process scanner (issue #399). + * + * One scan produces (pid, args) pairs that both orphan reapers match over, + * halving the platform I/O the two independent walks used to do. + */ +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; + +vi.mock('node:fs/promises', async (importOriginal: () => Promise) => { + const actual = (await importOriginal()) as Record; + return { + ...actual, + readdir: vi.fn(), + readFile: vi.fn(), + }; +}); + +vi.mock('node:child_process', async (importOriginal: () => Promise) => { + const actual = (await importOriginal()) as Record; + return { + ...actual, + execFile: vi.fn(), + }; +}); + +import { execFile } from 'node:child_process'; +import * as fsp from 'node:fs/promises'; +import { scanLinux, scanDarwin, scanWindows } from '../../../src/utils/process-scan.js'; + +const mockExecFile = execFile as unknown as ReturnType; +const mockReaddir = fsp.readdir as unknown as ReturnType; +const mockReadFile = fsp.readFile as unknown as ReturnType; + +beforeEach(() => { + vi.clearAllMocks(); +}); + +afterEach(() => { + vi.restoreAllMocks(); +}); + +describe('scanLinux', () => { + it('returns (pid, args) for numeric /proc entries, splitting cmdline on NUL', async () => { + mockReaddir.mockResolvedValue(['1', 'self', '42', 'not-a-pid']); + mockReadFile.mockImplementation(async (p: string) => { + if (p === '/proc/1/cmdline') return 'init\0'; + if (p === '/proc/42/cmdline') return 'java\0-Dmcp.debugger.jvm=true\0Main\0'; + throw new Error(`unexpected read: ${p}`); + }); + + const result = await scanLinux(); + + expect(result).toEqual( + expect.arrayContaining([ + { pid: 1, args: ['init'] }, + { pid: 42, args: ['java', '-Dmcp.debugger.jvm=true', 'Main'] }, + ]) + ); + expect(result).toHaveLength(2); + // Non-numeric entries never get a cmdline read + expect(mockReadFile).toHaveBeenCalledTimes(2); + }); + + it('skips pids whose cmdline read fails and survives readdir failure', async () => { + mockReaddir.mockResolvedValue(['7']); + mockReadFile.mockRejectedValue(new Error('EACCES')); + expect(await scanLinux()).toEqual([]); + + mockReaddir.mockRejectedValue(new Error('no /proc')); + expect(await scanLinux()).toEqual([]); + }); +}); + +describe('scanDarwin', () => { + it('parses ps pid/command lines into (pid, args)', async () => { + mockExecFile.mockImplementation( + (_cmd: string, _args: string[], _opts: unknown, cb: (e: Error | null, r?: { stdout: string }) => void) => { + cb(null, { stdout: ' 12 /usr/bin/thing --flag\n 345 node worker.js --mcp-owner-pid=9\n\n' }); + } + ); + + const result = await scanDarwin(); + + expect(result).toEqual([ + { pid: 12, args: ['/usr/bin/thing', '--flag'] }, + { pid: 345, args: ['node', 'worker.js', '--mcp-owner-pid=9'] }, + ]); + }); +}); + +describe('scanWindows', () => { + it('runs one CIM query per process name and concatenates results', async () => { + const byName: Record = { + 'java.exe': JSON.stringify([{ ProcessId: 100, CommandLine: 'java -Dmcp.debugger.jvm=true Main' }]), + 'node.exe': JSON.stringify({ ProcessId: 200, CommandLine: 'node proxy-bootstrap.js --mcp-owner-pid=9' }), + }; + mockExecFile.mockImplementation( + (_cmd: string, args: string[], _opts: unknown, cb: (e: Error | null, r?: { stdout: string }) => void) => { + const psCommand = args[args.length - 1]; + const name = Object.keys(byName).find((n) => psCommand.includes(`Name='${n}'`)); + cb(null, { stdout: name ? byName[name] : '' }); + } + ); + + const result = await scanWindows(['java.exe', 'node.exe']); + + expect(mockExecFile).toHaveBeenCalledTimes(2); + expect(result).toEqual([ + { pid: 100, args: ['java', '-Dmcp.debugger.jvm=true', 'Main'] }, + { pid: 200, args: ['node', 'proxy-bootstrap.js', '--mcp-owner-pid=9'] }, + ]); + }); + + it('tolerates a failing or empty query without failing the others', async () => { + mockExecFile.mockImplementation( + (_cmd: string, args: string[], _opts: unknown, cb: (e: Error | null, r?: { stdout: string }) => void) => { + const psCommand = args[args.length - 1]; + if (psCommand.includes("Name='java.exe'")) { + cb(new Error('powershell exploded')); + } else { + cb(null, { stdout: JSON.stringify({ ProcessId: 5, CommandLine: 'node x.js' }) }); + } + } + ); + + const result = await scanWindows(['java.exe', 'node.exe']); + + expect(result).toEqual([{ pid: 5, args: ['node', 'x.js'] }]); + }); +}); diff --git a/tests/unit/utils/startup-janitor.test.ts b/tests/unit/utils/startup-janitor.test.ts new file mode 100644 index 00000000..e22102ef --- /dev/null +++ b/tests/unit/utils/startup-janitor.test.ts @@ -0,0 +1,152 @@ +/** + * Unit tests for the startup janitor (issue #399): one shared process scan + * feeding both orphan reapers, an env opt-out, and the stale session-run + * log sweep (deferred here from issue #403). + */ +import { describe, it, expect, vi } from 'vitest'; +import fs from 'fs'; +import os from 'os'; +import path from 'path'; +import { runStartupJanitor, sweepStaleSessionRuns } from '../../../src/utils/startup-janitor.js'; +import type { ScannedProcess } from '../../../src/utils/process-scan.js'; + +function makeLogger() { + return { info: vi.fn(), warn: vi.fn(), error: vi.fn(), debug: vi.fn() }; +} + +describe('runStartupJanitor', () => { + const jvmRow: ScannedProcess = { + pid: 100, + args: ['java', '-Dmcp.debugger.jvm=true', '-Dmcp.debugger.owner_pid=7', 'Main'] + }; + const proxyRow: ScannedProcess = { + pid: 200, + args: ['node', 'dist/proxy/proxy-bootstrap.js', '--mcp-owner-pid=7', '--mcp-session-id=s1'] + }; + const noiseRow: ScannedProcess = { pid: 300, args: ['bash'] }; + + it('scans once and feeds both reapers from the same result', async () => { + const scan = vi.fn().mockResolvedValue([jvmRow, proxyRow, noiseRow]); + const reapJvms = vi.fn().mockResolvedValue({ scanned: 1, killed: [], skipped: [], errors: [] }); + const reapProxies = vi.fn().mockResolvedValue({ scanned: 1, killed: [], skipped: [], errors: [] }); + + await runStartupJanitor({ + logger: makeLogger(), + selfPid: 1, + env: {}, + scan, + reapJvms, + reapProxies, + sweep: vi.fn().mockResolvedValue({ removedRuns: 0 }) + }); + + expect(scan).toHaveBeenCalledTimes(1); + expect(scan).toHaveBeenCalledWith({ windowsProcessNames: ['java.exe', 'node.exe'] }); + + // Each reaper received a lister derived from the shared scan + const jvmLister = reapJvms.mock.calls[0][0].lister; + await expect(jvmLister()).resolves.toEqual([ + { pid: 100, ownerPid: 7, sessionTag: '' } + ]); + const proxyLister = reapProxies.mock.calls[0][0].lister; + await expect(proxyLister()).resolves.toEqual([ + { pid: 200, ownerPid: 7, sessionId: 's1' } + ]); + }); + + it('skips the reapers entirely when MCP_SKIP_ORPHAN_REAPERS=1', async () => { + const scan = vi.fn(); + const reapJvms = vi.fn(); + const reapProxies = vi.fn(); + + await runStartupJanitor({ + logger: makeLogger(), + selfPid: 1, + env: { MCP_SKIP_ORPHAN_REAPERS: '1' }, + scan, + reapJvms, + reapProxies, + sweep: vi.fn().mockResolvedValue({ removedRuns: 0 }) + }); + + expect(scan).not.toHaveBeenCalled(); + expect(reapJvms).not.toHaveBeenCalled(); + expect(reapProxies).not.toHaveBeenCalled(); + }); + + it('logs kill counts and never throws when a reaper rejects', async () => { + const logger = makeLogger(); + const scan = vi.fn().mockResolvedValue([]); + const reapJvms = vi.fn().mockResolvedValue({ + scanned: 1, + killed: [{ pid: 100, ownerPid: 7, sessionTag: 't' }], + skipped: [], + errors: [] + }); + const reapProxies = vi.fn().mockRejectedValue(new Error('reaper exploded')); + + await expect(runStartupJanitor({ + logger, + selfPid: 1, + env: {}, + scan, + reapJvms, + reapProxies, + sweep: vi.fn().mockResolvedValue({ removedRuns: 0 }) + })).resolves.toBeUndefined(); + + expect(logger.info).toHaveBeenCalledWith(expect.stringContaining('Reaped 1 orphan JVM')); + expect(logger.warn).toHaveBeenCalledWith(expect.stringContaining('reaper exploded')); + }); +}); + +describe('sweepStaleSessionRuns', () => { + function makeSessionsDir(): string { + return fs.mkdtempSync(path.join(os.tmpdir(), 'janitor-sweep-test-')); + } + + it('removes run dirs older than the age cap and keeps fresh ones', async () => { + const base = makeSessionsDir(); + try { + const staleRun = path.join(base, 'sess-1', 'run-1000'); + const freshRun = path.join(base, 'sess-1', 'run-2000'); + fs.mkdirSync(staleRun, { recursive: true }); + fs.mkdirSync(freshRun, { recursive: true }); + fs.writeFileSync(path.join(staleRun, 'proxy-sess-1.log'), 'old'); + fs.writeFileSync(path.join(freshRun, 'dap-trace-sess-1.ndjson'), 'new'); + const old = new Date(Date.now() - 8 * 24 * 60 * 60 * 1000); + fs.utimesSync(staleRun, old, old); + + const result = await sweepStaleSessionRuns({ baseDir: base }); + + expect(result.removedRuns).toBe(1); + expect(fs.existsSync(staleRun)).toBe(false); + expect(fs.existsSync(freshRun)).toBe(true); + } finally { + fs.rmSync(base, { recursive: true, force: true }); + } + }); + + it('removes a session dir once all its runs are gone', async () => { + const base = makeSessionsDir(); + try { + const staleRun = path.join(base, 'sess-2', 'run-1000'); + fs.mkdirSync(staleRun, { recursive: true }); + fs.writeFileSync(path.join(staleRun, 'proxy-sess-2.log'), 'old'); + const old = new Date(Date.now() - 8 * 24 * 60 * 60 * 1000); + fs.utimesSync(staleRun, old, old); + + await sweepStaleSessionRuns({ baseDir: base }); + + expect(fs.existsSync(path.join(base, 'sess-2'))).toBe(false); + } finally { + fs.rmSync(base, { recursive: true, force: true }); + } + }); + + it('is silent and safe when the base dir does not exist', async () => { + await expect( + sweepStaleSessionRuns({ baseDir: path.join(os.tmpdir(), 'janitor-nonexistent-xyz') }) + ).resolves.toEqual({ removedRuns: 0 }); + }); +});