From 71028b20630aa01567fe9d6993493e6f0ada5f66 Mon Sep 17 00:00:00 2001 From: JF Date: Sat, 22 Aug 2026 15:45:55 -0400 Subject: [PATCH] fix(js-debug): tag vsDebugServer argv and reap stranded instances (#431) js-debug DAP servers are spawned detached+unref'd by the proxy worker, so a hard-killed worker (win32 TerminateProcess) stranded them outside every tree-kill path; they accumulated as orphans holding file locks on the vendor tree. Two-prong fix: - Tag the vsDebugServer argv with the same inert --mcp-owner-pid / --mcp-session-id markers the proxy worker carries (vsDebugServer reads only argv[2]/argv[3]; trailing tokens are ignored), and teach the startup janitor a third matcher over its existing process scan that reaps marked instances whose owner is dead - taskkill /T /F also sweeps the debuggee/watchdog children while the vsDebugServer parent is still alive. Marker constants move to @debugmcp/shared so the adapter package can import them. - On win32, ProxyProcessAdapter.kill() now tree-kills the worker first (taskkill /PID /T /F) while it is still alive, so ProxyManager's force-kill escalations no longer strand the detached adapter subtree. Guarded by the adapter's tracked exit state to avoid PID-reuse kills. Closes #431 Co-Authored-By: Claude Fable 5 --- .../src/javascript-debug-adapter.ts | 21 +++++- .../javascript-debug-adapter.command.test.ts | 35 +++++++++ packages/shared/src/index.ts | 8 +++ packages/shared/src/utils/process-markers.ts | 31 ++++++++ src/implementations/process-launcher-impl.ts | 49 +++++++++++-- src/utils/proxy-orphan-reaper.ts | 45 +++++++++--- src/utils/startup-janitor.ts | 31 ++++++-- .../process-launcher-impl.test.ts | 72 +++++++++++++++++++ .../proxy-orphan-reaper-internals.test.ts | 61 ++++++++++++++++ tests/unit/utils/startup-janitor.test.ts | 44 +++++++++++- 10 files changed, 369 insertions(+), 28 deletions(-) create mode 100644 packages/shared/src/utils/process-markers.ts diff --git a/packages/adapter-javascript/src/javascript-debug-adapter.ts b/packages/adapter-javascript/src/javascript-debug-adapter.ts index e3f2f1cb..439e276f 100644 --- a/packages/adapter-javascript/src/javascript-debug-adapter.ts +++ b/packages/adapter-javascript/src/javascript-debug-adapter.ts @@ -25,7 +25,9 @@ import { type LanguageSpecificAttachConfig, type FeatureRequirement, type AdapterCapabilities, - type AdapterLaunchBarrier + type AdapterLaunchBarrier, + OWNER_PID_ARG_PREFIX, + SESSION_ID_ARG_PREFIX } from '@debugmcp/shared'; import { DebugLanguage } from '@debugmcp/shared'; import type { AdapterDependencies } from '@debugmcp/shared'; @@ -280,7 +282,22 @@ export class JavascriptDebugAdapter extends EventEmitter implements IDebugAdapte typeof config?.adapterHost === 'string' && config.adapterHost.trim().length > 0 ? config.adapterHost : '127.0.0.1'; - const args = [adapterPath, String(port), host]; + // Reaper markers (issue #431): vsDebugServer.cjs reads only argv[2] (port) + // and argv[3] (host) and ignores trailing tokens, so these tags are inert + // at runtime — but they let the startup janitor recognize an instance + // stranded by a hard-killed proxy worker (spawned detached+unref'd, it + // survives every tree-kill path) and reap it by owner pid. Constraints: + // tokens must stay whitespace-free (the win32 process scan splits + // CommandLine on whitespace) and must never contain `--help` (vsDebugServer + // prints usage and exits if --help appears anywhere in argv). + const ownerPid = Number(process.env.MCP_DEBUGGER_MAIN_PID) || process.pid; + const args = [ + adapterPath, + String(port), + host, + `${OWNER_PID_ARG_PREFIX}${ownerPid}`, + `${SESSION_ID_ARG_PREFIX}${config.sessionId}` + ]; // Environment: clone from process.env (string values only), safely ensure NODE_OPTIONS memory flag const env: Record = {}; diff --git a/packages/adapter-javascript/tests/unit/javascript-debug-adapter.command.test.ts b/packages/adapter-javascript/tests/unit/javascript-debug-adapter.command.test.ts index cb460358..12b8af82 100644 --- a/packages/adapter-javascript/tests/unit/javascript-debug-adapter.command.test.ts +++ b/packages/adapter-javascript/tests/unit/javascript-debug-adapter.command.test.ts @@ -73,6 +73,41 @@ describe('JavascriptDebugAdapter.buildAdapterCommand (stdio)', () => { expect(path.isAbsolute(adapterPath as string)).toBe(true); }); + it('tags the adapter argv with owner-pid and session-id reaper markers (issue #431)', () => { + const adapter = new JavascriptDebugAdapter(deps); + + vi.stubEnv('MCP_DEBUGGER_MAIN_PID', '4242'); + const cmd = adapter.buildAdapterCommand(defaultConfig); + + // vsDebugServer.cjs reads only argv[2] (port) and argv[3] (host); trailing + // tokens are inert, which is what makes these markers safe to append. + expect(cmd.args[3]).toBe('--mcp-owner-pid=4242'); + expect(cmd.args[4]).toBe('--mcp-session-id=test-session'); + }); + + it('falls back to process.pid for the owner marker when MCP_DEBUGGER_MAIN_PID is unset', () => { + const adapter = new JavascriptDebugAdapter(deps); + + vi.stubEnv('MCP_DEBUGGER_MAIN_PID', undefined); + const cmd = adapter.buildAdapterCommand(defaultConfig); + + expect(cmd.args[3]).toBe(`--mcp-owner-pid=${process.pid}`); + }); + + it('marker tokens contain no whitespace and never the string --help', () => { + // Whitespace would fragment under the win32 CommandLine split; --help + // anywhere in argv makes vsDebugServer print usage instead of serving. + const adapter = new JavascriptDebugAdapter(deps); + + const cmd = adapter.buildAdapterCommand(defaultConfig); + + for (const token of cmd.args.slice(3)) { + expect(token).not.toMatch(/\s/); + expect(token).not.toContain('--help'); + } + expect(cmd.args.slice(3).length).toBeGreaterThan(0); + }); + it('environment includes NODE_OPTIONS and does not mutate process.env', () => { const adapter = new JavascriptDebugAdapter(deps); diff --git a/packages/shared/src/index.ts b/packages/shared/src/index.ts index 134d5e3e..cf4eb0b1 100644 --- a/packages/shared/src/index.ts +++ b/packages/shared/src/index.ts @@ -234,3 +234,11 @@ export { export type { SecretRule, RedactionHit, RedactionResult } from './utils/secret-redaction.js'; export { LineBuffer } from './utils/line-buffer.js'; export { toSourceBreakpoint, type BreakpointFields, toFunctionBreakpoint, type FunctionBreakpointFields } from './utils/to-source-breakpoint.js'; +// Argv marker constants shared by spawn-time tagging and the startup orphan +// reapers (issues #343, #431). +export { + PROXY_BOOTSTRAP_MARKER, + JS_DEBUG_ADAPTER_MARKER, + OWNER_PID_ARG_PREFIX, + SESSION_ID_ARG_PREFIX +} from './utils/process-markers.js'; diff --git a/packages/shared/src/utils/process-markers.ts b/packages/shared/src/utils/process-markers.ts new file mode 100644 index 00000000..02797cf5 --- /dev/null +++ b/packages/shared/src/utils/process-markers.ts @@ -0,0 +1,31 @@ +/** + * Argv marker constants shared between the processes that tag child argv at + * spawn time (the server's proxy launcher, adapter packages) and the startup + * reapers that later recognize those tags in system-wide process scans + * (issues #343, #431). + * + * Marker constraints, imposed by the scan/matcher machinery: + * - Tokens must be whitespace-free: the win32 scan splits a process's + * CommandLine on whitespace, so a marker containing a space would fragment + * and never match. + * - Identity markers are matched with `String.includes` against every token + * (paths may fragment), so they must be substrings that cannot appear in + * unrelated cmdlines by accident. + * - No token may be or contain `--help`: vsDebugServer.cjs prints usage and + * exits when `--help` appears anywhere in its argv. + */ + +/** Substring of the proxy worker's script-path argv token used as its identity marker. */ +export const PROXY_BOOTSTRAP_MARKER = 'proxy-bootstrap'; + +/** + * Substring of the js-debug DAP server's script-path argv token used as its + * identity marker (issue #431). On its own this also matches VS Code's own + * js-debug instances — matchers must additionally require the owner-pid + * marker below, which only our spawns carry. + */ +export const JS_DEBUG_ADAPTER_MARKER = 'vsDebugServer'; + +/** Records the PID of the mcp-debugger server that owned the session. */ +export const OWNER_PID_ARG_PREFIX = '--mcp-owner-pid='; +export const SESSION_ID_ARG_PREFIX = '--mcp-session-id='; diff --git a/src/implementations/process-launcher-impl.ts b/src/implementations/process-launcher-impl.ts index d62e2fc5..4ce3b5bb 100644 --- a/src/implementations/process-launcher-impl.ts +++ b/src/implementations/process-launcher-impl.ts @@ -32,7 +32,9 @@ class ProxyProcessAdapter extends EventEmitter implements IProxyProcess { constructor( public readonly childProcess: IChildProcess, - public readonly sessionId: string + public readonly sessionId: string, + private readonly platform: NodeJS.Platform = process.platform, + private readonly treeKill?: (pid: number) => void ) { super(); @@ -377,12 +379,32 @@ class ProxyProcessAdapter extends EventEmitter implements IProxyProcess { if (this.killed || this.disposed) { return false; // Already killed or disposed } - + // If waiting for initialization, fail it if (this.initializationState === 'waiting') { this.failInitialization(new Error('Process killed during initialization')); } - + + // win32: any cross-process signal is TerminateProcess on the single PID — + // the worker's JS never runs, so its own adapter tree-kill cascade cannot + // fire, stranding detached adapter children like js-debug's vsDebugServer + // (issue #431). Sweep the tree FIRST, while the worker is still alive: + // taskkill /T can only discover children through a live parent. Skipped + // once the worker has exited — its PID may already be recycled. + if ( + this.platform === 'win32' && + this.treeKill && + this._exitCode === null && + this._signalCode === null && + typeof this.childProcess.pid === 'number' + ) { + try { + this.treeKill(this.childProcess.pid); + } catch { + // Best-effort: fall through to the plain kill below. + } + } + try { return this.childProcess.kill(signal); } catch { @@ -396,7 +418,9 @@ class ProxyProcessAdapter extends EventEmitter implements IProxyProcess { */ export class ProxyProcessLauncherImpl implements IProxyProcessLauncher { constructor( - private processManager: IProcessManager + private processManager: IProcessManager, + private platform: NodeJS.Platform = process.platform, + private treeKill?: (pid: number) => void ) {} launchProxy( @@ -474,8 +498,21 @@ export class ProxyProcessLauncherImpl implements IProxyProcessLauncher { options ); - // Create the proxy process adapter with the raw child process - return new ProxyProcessAdapter(childProcess, sessionId); + // Create the proxy process adapter with the raw child process. The + // win32 tree-kill (issue #431) defaults to spawning taskkill through the + // same process manager; injectable for tests. + const treeKill = + this.treeKill ?? + ((pid: number) => { + const killer = this.processManager.spawn( + 'taskkill', + ['/PID', String(pid), '/T', '/F'], + { stdio: 'ignore', windowsHide: true } as IProcessOptions + ); + // Fire-and-forget: a missing taskkill must not crash the server. + killer.on('error', () => {}); + }); + return new ProxyProcessAdapter(childProcess, sessionId, this.platform, treeKill); } } diff --git a/src/utils/proxy-orphan-reaper.ts b/src/utils/proxy-orphan-reaper.ts index 233dfba0..34216de8 100644 --- a/src/utils/proxy-orphan-reaper.ts +++ b/src/utils/proxy-orphan-reaper.ts @@ -32,15 +32,21 @@ */ import { execFile } from 'node:child_process'; import { promisify } from 'node:util'; +import { + PROXY_BOOTSTRAP_MARKER, + JS_DEBUG_ADAPTER_MARKER, + OWNER_PID_ARG_PREFIX, + SESSION_ID_ARG_PREFIX +} from '@debugmcp/shared'; import { isPidAlive, SignalFn } from './jvm-orphan-reaper.js'; import { scanLinux, scanDarwin, scanWindows, scanProcessArgs, type ScannedProcess } from './process-scan.js'; const execFileAsync = promisify(execFile); -/** Substring of the worker's script-path argv token used as the identity marker. */ -export const PROXY_BOOTSTRAP_MARKER = 'proxy-bootstrap'; -export const OWNER_PID_ARG_PREFIX = '--mcp-owner-pid='; -export const SESSION_ID_ARG_PREFIX = '--mcp-session-id='; +// Marker constants live in @debugmcp/shared so adapter packages can tag their +// own spawns with the same markers this reaper matches (issue #431); re-exported +// here to keep this module the reaper-side import point. +export { PROXY_BOOTSTRAP_MARKER, JS_DEBUG_ADAPTER_MARKER, OWNER_PID_ARG_PREFIX, SESSION_ID_ARG_PREFIX }; const LIST_TIMEOUT_MS = 5000; /** How often the POSIX escalation path polls for the SIGTERM cascade to finish. */ @@ -171,18 +177,17 @@ export async function listWindowsProxies(): Promise { } /** - * Two-factor identification: a token containing the proxy-bootstrap script - * name AND a valid --mcp-owner-pid marker. Untagged bootstrap processes - * (pre-upgrade workers) and malformed tags are ignored, never killed. - * - * @internal Exposed for unit tests; not part of the public module API. + * Two-factor identification: a token containing the identity marker AND a + * valid --mcp-owner-pid marker. Untagged processes (pre-upgrade workers, + * VS Code's own js-debug instances) and malformed tags are ignored, never + * killed. */ -export function parseProxyArgs(pid: number, args: string[]): TaggedProxy | null { +function parseTaggedArgs(pid: number, args: string[], identityMarker: string): TaggedProxy | null { let hasMarker = false; let ownerPid = -1; let sessionId = ''; for (const a of args) { - if (a.includes(PROXY_BOOTSTRAP_MARKER)) { + if (a.includes(identityMarker)) { hasMarker = true; } else if (a.startsWith(OWNER_PID_ARG_PREFIX)) { const v = Number(a.slice(OWNER_PID_ARG_PREFIX.length)); @@ -195,6 +200,24 @@ export function parseProxyArgs(pid: number, args: string[]): TaggedProxy | null return { pid, ownerPid, sessionId }; } +/** @internal Exposed for unit tests; not part of the public module API. */ +export function parseProxyArgs(pid: number, args: string[]): TaggedProxy | null { + return parseTaggedArgs(pid, args, PROXY_BOOTSTRAP_MARKER); +} + +/** + * Matcher for js-debug DAP server processes (vsDebugServer.cjs) tagged at + * spawn time by JavascriptDebugAdapter.buildAdapterCommand (issue #431). + * These are spawned detached+unref'd by the proxy worker, so a hard-killed + * worker strands them outside every tree-kill path; the startup janitor + * feeds scan rows through this matcher to find them. + * + * @internal Exposed for unit tests; not part of the public module API. + */ +export function parseJsDebugAdapterArgs(pid: number, args: string[]): TaggedProxy | null { + return parseTaggedArgs(pid, args, JS_DEBUG_ADAPTER_MARKER); +} + const defaultSignal: SignalFn = (pid, signal) => process.kill(pid, signal); /** Injectable so escalation tests run instantly instead of really sleeping. */ diff --git a/src/utils/startup-janitor.ts b/src/utils/startup-janitor.ts index 182faf2e..7e0d9a31 100644 --- a/src/utils/startup-janitor.ts +++ b/src/utils/startup-janitor.ts @@ -5,8 +5,9 @@ * transport from coming up. * * Covers: - * - Orphan debuggee JVMs and proxy workers, via ONE shared process scan - * feeding both reapers' matchers (previously two independent walks). + * - Orphan debuggee JVMs, proxy workers, and js-debug DAP servers (#431), via + * ONE shared process scan feeding all reaper matchers (previously + * 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 @@ -22,7 +23,7 @@ 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'; +import { reapOrphanProxies, parseProxyArgs, parseJsDebugAdapterArgs, type ReapResult as ProxyReapResult, type ReapOptions as ProxyReapOptions } from './proxy-orphan-reaper.js'; export interface JanitorLogger { info?: (msg: string) => void; @@ -39,6 +40,7 @@ export interface StartupJanitorOptions { scan?: (options: ProcessScanOptions) => Promise; reapJvms?: (opts: JvmReapOptions) => Promise; reapProxies?: (opts: ProxyReapOptions) => Promise; + reapJsDebugAdapters?: (opts: ProxyReapOptions) => Promise; sweep?: (opts: SweepOptions) => Promise; } @@ -77,16 +79,24 @@ async function runOrphanReapers(opts: StartupJanitorOptions, selfPid: number): P return; } - // Matchers are marker-based, so feeding every row to both is harmless — - // JVM -D markers only appear in java cmdlines and vice versa. + // Matchers are marker-based, so feeding every row to all three is harmless — + // each identity marker only appears in its own family's cmdlines. 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([ + // js-debug DAP servers are spawned detached+unref'd, so a hard-killed worker + // strands them outside every tree-kill path (issue #431). Reusing the proxy + // reaper works because its win32 kill is taskkill /T /F — while the stranded + // vsDebugServer is alive, the tree kill also reaches the debuggee/watchdog + // children js-debug spawned beneath it. + const jsDebugAdapterLister = async () => + processes.map((p) => parseJsDebugAdapterArgs(p.pid, p.args)).filter((t): t is NonNullable => t !== null); + + const [jvmOutcome, proxyOutcome, jsDebugOutcome] = await Promise.allSettled([ (opts.reapJvms ?? reapOrphanJvms)({ selfPid, logger: log, lister: jvmLister }), (opts.reapProxies ?? reapOrphanProxies)({ selfPid, logger: log, lister: proxyLister }), + (opts.reapJsDebugAdapters ?? reapOrphanProxies)({ selfPid, logger: log, lister: jsDebugAdapterLister }), ]); if (jvmOutcome.status === 'fulfilled') { @@ -103,6 +113,13 @@ async function runOrphanReapers(opts: StartupJanitorOptions, selfPid: number): P } else { log.warn?.(`[startup-janitor] Orphan proxy reaper failed: ${(proxyOutcome.reason as Error)?.message ?? String(proxyOutcome.reason)}`); } + if (jsDebugOutcome.status === 'fulfilled') { + if (jsDebugOutcome.value.killed.length > 0) { + log.info?.(`[startup-janitor] Reaped ${jsDebugOutcome.value.killed.length} orphan js-debug adapter(s) from prior runs`); + } + } else { + log.warn?.(`[startup-janitor] Orphan js-debug adapter reaper failed: ${(jsDebugOutcome.reason as Error)?.message ?? String(jsDebugOutcome.reason)}`); + } } /** Delete stale per-session run log dirs after this long (matches logger.ts). */ diff --git a/tests/unit/implementations/process-launcher-impl.test.ts b/tests/unit/implementations/process-launcher-impl.test.ts index 34c1f666..3641c5c5 100644 --- a/tests/unit/implementations/process-launcher-impl.test.ts +++ b/tests/unit/implementations/process-launcher-impl.test.ts @@ -172,4 +172,76 @@ describe('ProxyProcessLauncherImpl', () => { const result = proxyProcess.kill('SIGTERM'); expect(result).toBe(false); }); + + // On win32 any cross-process signal is TerminateProcess on the single PID, + // which strands the detached js-debug adapter subtree (issue #431). kill() + // must sweep the worker's tree first, while the worker is still alive. + describe('win32 tree-kill on kill()', () => { + it('tree-kills via the injected treeKill before terminating the worker on win32', () => { + const treeKill = vi.fn(); + const launcher = new ProxyProcessLauncherImpl(processManager, 'win32', treeKill); + const proxyProcess = launcher.launchProxy('./dist/proxy.js', 'session-tree'); + + const result = proxyProcess.kill('SIGKILL'); + + expect(result).toBe(true); + expect(treeKill).toHaveBeenCalledWith(2222); + expect(child.kill).toHaveBeenCalledWith('SIGKILL'); + // Tree-kill must strike first: taskkill /T can only discover children + // while the parent is alive. + expect(treeKill.mock.invocationCallOrder[0]).toBeLessThan( + (child.kill as ReturnType).mock.invocationCallOrder[0] + ); + }); + + it('does not tree-kill when the worker already exited (PID may be recycled)', () => { + const treeKill = vi.fn(); + const launcher = new ProxyProcessLauncherImpl(processManager, 'win32', treeKill); + const proxyProcess = launcher.launchProxy('./dist/proxy.js', 'session-tree-exited'); + + child.emit('exit', 0, null); + proxyProcess.kill('SIGKILL'); + + expect(treeKill).not.toHaveBeenCalled(); + }); + + it('does not tree-kill on POSIX platforms', () => { + const treeKill = vi.fn(); + const launcher = new ProxyProcessLauncherImpl(processManager, 'linux', treeKill); + const proxyProcess = launcher.launchProxy('./dist/proxy.js', 'session-tree-posix'); + + proxyProcess.kill('SIGKILL'); + + expect(treeKill).not.toHaveBeenCalled(); + expect(child.kill).toHaveBeenCalledWith('SIGKILL'); + }); + + it('still terminates the worker when treeKill throws', () => { + const treeKill = vi.fn(() => { + throw new Error('taskkill missing'); + }); + const launcher = new ProxyProcessLauncherImpl(processManager, 'win32', treeKill); + const proxyProcess = launcher.launchProxy('./dist/proxy.js', 'session-tree-throw'); + + const result = proxyProcess.kill('SIGKILL'); + + expect(result).toBe(true); + expect(child.kill).toHaveBeenCalledWith('SIGKILL'); + }); + + it('spawns taskkill /PID /T /F via the process manager by default on win32', () => { + const spawnSpy = vi.spyOn(processManager, 'spawn'); + const launcher = new ProxyProcessLauncherImpl(processManager, 'win32'); + const proxyProcess = launcher.launchProxy('./dist/proxy.js', 'session-tree-default'); + spawnSpy.mockClear(); + + proxyProcess.kill('SIGKILL'); + + expect(spawnSpy).toHaveBeenCalledWith( + 'taskkill', + ['/PID', '2222', '/T', '/F'], + expect.objectContaining({ windowsHide: true }) + ); + }); + }); }); diff --git a/tests/unit/utils/proxy-orphan-reaper-internals.test.ts b/tests/unit/utils/proxy-orphan-reaper-internals.test.ts index 6adaaac7..f3684223 100644 --- a/tests/unit/utils/proxy-orphan-reaper-internals.test.ts +++ b/tests/unit/utils/proxy-orphan-reaper-internals.test.ts @@ -23,6 +23,7 @@ import { execFile } from 'node:child_process'; import * as fsp from 'node:fs/promises'; import { parseProxyArgs, + parseJsDebugAdapterArgs, killPosixWithEscalation, killWindows, listLinuxProxies, @@ -112,6 +113,66 @@ describe('parseProxyArgs', () => { }); }); +describe('parseJsDebugAdapterArgs', () => { + /** A js-debug adapter cmdline exactly as spawned via buildAdapterCommand (issue #431). */ + const adapterArgs = (ownerPid: number | string, sessionId = 'sess-1') => [ + 'C:\\nodejs\\node.exe', + 'C:\\repo\\packages\\adapter-javascript\\vendor\\js-debug\\vsDebugServer.cjs', + '5679', + '127.0.0.1', + `--mcp-owner-pid=${ownerPid}`, + `--mcp-session-id=${sessionId}`, + ]; + + it('parses the full spawn shape including session id', () => { + expect(parseJsDebugAdapterArgs(9001, adapterArgs(42, 'abc-123'))).toEqual({ + pid: 9001, + ownerPid: 42, + sessionId: 'abc-123', + }); + }); + + it("returns null for VS Code's own js-debug instances (no owner marker)", () => { + expect( + parseJsDebugAdapterArgs(9001, [ + 'C:\\nodejs\\node.exe', + 'c:\\Users\\x\\.vscode\\extensions\\ms-vscode.js-debug\\src\\vsDebugServer.cjs', + '5680', + ]), + ).toBeNull(); + }); + + it('returns null when the owner marker is present but the vsDebugServer token is absent', () => { + // e.g. a tagged proxy worker row — must stay matched by parseProxyArgs only + expect( + parseJsDebugAdapterArgs(9001, [ + 'node', + '/app/dist/proxy/proxy-bootstrap.js', + '--mcp-owner-pid=42', + '--mcp-session-id=s', + ]), + ).toBeNull(); + }); + + it('returns null when owner pid is non-numeric, zero, or negative', () => { + expect(parseJsDebugAdapterArgs(9001, adapterArgs('not-a-pid'))).toBeNull(); + expect(parseJsDebugAdapterArgs(9001, adapterArgs(0))).toBeNull(); + expect(parseJsDebugAdapterArgs(9001, adapterArgs(-5))).toBeNull(); + }); + + it('tolerates a missing session id by leaving it empty', () => { + expect( + parseJsDebugAdapterArgs(9001, [ + 'node', + '/repo/vendor/js-debug/vsDebugServer.cjs', + '5679', + '127.0.0.1', + '--mcp-owner-pid=42', + ]), + ).toEqual({ pid: 9001, ownerPid: 42, sessionId: '' }); + }); +}); + /** Injected signal fn that throws an ErrnoException with the given code (issue #183). */ function throwWith(code: string): () => never { return () => { diff --git a/tests/unit/utils/startup-janitor.test.ts b/tests/unit/utils/startup-janitor.test.ts index e22102ef..2c6dd1ab 100644 --- a/tests/unit/utils/startup-janitor.test.ts +++ b/tests/unit/utils/startup-janitor.test.ts @@ -24,11 +24,33 @@ describe('runStartupJanitor', () => { args: ['node', 'dist/proxy/proxy-bootstrap.js', '--mcp-owner-pid=7', '--mcp-session-id=s1'] }; const noiseRow: ScannedProcess = { pid: 300, args: ['bash'] }; + // A js-debug DAP server tagged at spawn by buildAdapterCommand (issue #431) + const vsDebugServerRow: ScannedProcess = { + pid: 400, + args: [ + 'C:\\nodejs\\node.exe', + 'C:\\repo\\packages\\adapter-javascript\\vendor\\js-debug\\vsDebugServer.cjs', + '5679', + '127.0.0.1', + '--mcp-owner-pid=7', + '--mcp-session-id=s2' + ] + }; + // VS Code's own js-debug instance — unmarked, must never be reaped + const foreignVsDebugServerRow: ScannedProcess = { + pid: 500, + args: [ + 'C:\\nodejs\\node.exe', + 'c:\\Users\\x\\.vscode\\extensions\\ms-vscode.js-debug\\src\\vsDebugServer.cjs', + '5680' + ] + }; - it('scans once and feeds both reapers from the same result', async () => { - const scan = vi.fn().mockResolvedValue([jvmRow, proxyRow, noiseRow]); + it('scans once and feeds all three reapers from the same result', async () => { + const scan = vi.fn().mockResolvedValue([jvmRow, proxyRow, noiseRow, vsDebugServerRow, foreignVsDebugServerRow]); const reapJvms = vi.fn().mockResolvedValue({ scanned: 1, killed: [], skipped: [], errors: [] }); const reapProxies = vi.fn().mockResolvedValue({ scanned: 1, killed: [], skipped: [], errors: [] }); + const reapJsDebugAdapters = vi.fn().mockResolvedValue({ scanned: 1, killed: [], skipped: [], errors: [] }); await runStartupJanitor({ logger: makeLogger(), @@ -37,6 +59,7 @@ describe('runStartupJanitor', () => { scan, reapJvms, reapProxies, + reapJsDebugAdapters, sweep: vi.fn().mockResolvedValue({ removedRuns: 0 }) }); @@ -52,12 +75,19 @@ describe('runStartupJanitor', () => { await expect(proxyLister()).resolves.toEqual([ { pid: 200, ownerPid: 7, sessionId: 's1' } ]); + // The marked vsDebugServer row reaches only the adapter reaper; VS Code's + // unmarked instance reaches none. + const adapterLister = reapJsDebugAdapters.mock.calls[0][0].lister; + await expect(adapterLister()).resolves.toEqual([ + { pid: 400, ownerPid: 7, sessionId: 's2' } + ]); }); it('skips the reapers entirely when MCP_SKIP_ORPHAN_REAPERS=1', async () => { const scan = vi.fn(); const reapJvms = vi.fn(); const reapProxies = vi.fn(); + const reapJsDebugAdapters = vi.fn(); await runStartupJanitor({ logger: makeLogger(), @@ -66,12 +96,14 @@ describe('runStartupJanitor', () => { scan, reapJvms, reapProxies, + reapJsDebugAdapters, sweep: vi.fn().mockResolvedValue({ removedRuns: 0 }) }); expect(scan).not.toHaveBeenCalled(); expect(reapJvms).not.toHaveBeenCalled(); expect(reapProxies).not.toHaveBeenCalled(); + expect(reapJsDebugAdapters).not.toHaveBeenCalled(); }); it('logs kill counts and never throws when a reaper rejects', async () => { @@ -84,6 +116,12 @@ describe('runStartupJanitor', () => { errors: [] }); const reapProxies = vi.fn().mockRejectedValue(new Error('reaper exploded')); + const reapJsDebugAdapters = vi.fn().mockResolvedValue({ + scanned: 1, + killed: [{ pid: 400, ownerPid: 7, sessionId: 's2' }], + skipped: [], + errors: [] + }); await expect(runStartupJanitor({ logger, @@ -92,10 +130,12 @@ describe('runStartupJanitor', () => { scan, reapJvms, reapProxies, + reapJsDebugAdapters, sweep: vi.fn().mockResolvedValue({ removedRuns: 0 }) })).resolves.toBeUndefined(); expect(logger.info).toHaveBeenCalledWith(expect.stringContaining('Reaped 1 orphan JVM')); + expect(logger.info).toHaveBeenCalledWith(expect.stringContaining('Reaped 1 orphan js-debug adapter')); expect(logger.warn).toHaveBeenCalledWith(expect.stringContaining('reaper exploded')); }); });