diff --git a/examples/javascript/fork_attach_target.js b/examples/javascript/fork_attach_target.js new file mode 100644 index 00000000..5f520560 --- /dev/null +++ b/examples/javascript/fork_attach_target.js @@ -0,0 +1,85 @@ +/** + * Forking attach target for the JavaScript attach smoke tests (issue #501). + * + * Started as: node --inspect=127.0.0.1: fork_attach_target.js + * + * The parent ticks forever and fork()s a child every 2s; each child announces + * itself over the IPC channel and exits, and the parent logs + * `child-handshake pid=` on receipt — mirroring the fork + init-ACK + * pattern of mcp-debugger's own ProxyManager. A child that never completes the + * handshake (parked in waitForDebugger by a debugger's auto-attach bootloader) + * is logged as `child-wedged pid=` and killed so wedges cannot pile up. + */ +import { fork } from 'child_process'; +import { fileURLToPath } from 'url'; + +const selfPath = fileURLToPath(import.meta.url); + +if (process.send) { + // Child branch: announce over IPC, then exit once the parent acks (or after + // a short grace so an unacked child never lingers) + process.send({ type: 'child-ready', pid: process.pid }); + process.on('message', (msg) => { + if (msg && msg.type === 'ack') { + process.exit(0); + } + }); + setTimeout(() => process.exit(0), 2000); +} else { + const FORK_INTERVAL_MS = 2000; + const HANDSHAKE_TIMEOUT_MS = 5000; + const MAX_CONCURRENT_CHILDREN = 5; + + let tickCounter = 0; + let handshakeCounter = 0; + const pendingChildren = new Set(); + + function tick() { + tickCounter += 1; + if (tickCounter % 10 === 0) { + console.log(`tick ${tickCounter}`); + } + } + + function spawnChild() { + if (pendingChildren.size >= MAX_CONCURRENT_CHILDREN) { + return; + } + // execArgv: [] so children don't inherit the parent's --inspect flag and + // fight over its port. The #501 wedge mechanism is unaffected: js-debug's + // auto-attach bootloader rides NODE_OPTIONS (env), which fork() inherits. + const child = fork(selfPath, [], { execArgv: [], stdio: ['inherit', 'inherit', 'inherit', 'ipc'] }); + pendingChildren.add(child); + + const wedgeTimer = setTimeout(() => { + console.log(`child-wedged pid=${child.pid}`); + child.kill('SIGKILL'); + }, HANDSHAKE_TIMEOUT_MS); + + child.on('message', (msg) => { + if (msg && msg.type === 'child-ready') { + clearTimeout(wedgeTimer); + handshakeCounter += 1; + console.log(`child-handshake ${handshakeCounter} pid=${msg.pid}`); + try { + child.send({ type: 'ack' }); + } catch { + // Child may already have exited on its own grace timer + } + } + }); + child.on('exit', () => { + clearTimeout(wedgeTimer); + pendingChildren.delete(child); + }); + child.on('error', () => { + clearTimeout(wedgeTimer); + pendingChildren.delete(child); + }); + } + + setInterval(tick, 100); + setInterval(spawnChild, FORK_INTERVAL_MS); + spawnChild(); + console.log('fork attach target started'); +} diff --git a/packages/adapter-javascript/src/javascript-debug-adapter.ts b/packages/adapter-javascript/src/javascript-debug-adapter.ts index 5e750fa5..2e3e920f 100644 --- a/packages/adapter-javascript/src/javascript-debug-adapter.ts +++ b/packages/adapter-javascript/src/javascript-debug-adapter.ts @@ -68,7 +68,8 @@ export class JavascriptDebugAdapter extends EventEmitter implements IDebugAdapte 'continueOnAttach', 'trace', 'websocketAddress', - 'attachExistingChildren' + 'attachExistingChildren', + 'autoAttachChildProcesses' ] as const; private state: AdapterState = AdapterState.UNINITIALIZED; @@ -680,6 +681,15 @@ export class JavascriptDebugAdapter extends EventEmitter implements IDebugAdapte name: 'Attach to Node.js process', host: (host as string | undefined) || '127.0.0.1', port: port as number | undefined, + // js-debug's pwa-node attach defaults this to true, injecting its + // NODE_OPTIONS bootloader into the inspected process; every fork() then + // parks under waitForDebugger and only one child can be adopted (#501). + // Default it off like launch mode does; an explicit caller value wins + // (never silently override a supported key — cf. #499). + autoAttachChildProcesses: + typeof rest.autoAttachChildProcesses === 'boolean' + ? rest.autoAttachChildProcesses + : false, } as LanguageSpecificAttachConfig; } diff --git a/packages/adapter-javascript/tests/unit/javascript-debug-adapter.transform.test.ts b/packages/adapter-javascript/tests/unit/javascript-debug-adapter.transform.test.ts index 6fce1e02..7cf7a535 100644 --- a/packages/adapter-javascript/tests/unit/javascript-debug-adapter.transform.test.ts +++ b/packages/adapter-javascript/tests/unit/javascript-debug-adapter.transform.test.ts @@ -376,5 +376,39 @@ describe('JavascriptDebugAdapter.transformLaunchConfig', () => { expect(cfg.skipFiles).toEqual(['/**']); expect(cfg.continueOnAttach).toBe(true); }); + + it('defaults autoAttachChildProcesses to false on attach (issue #501)', () => { + const adapter = new JavascriptDebugAdapter(deps); + const cfg = adapter.transformAttachConfig({ + request: 'attach', + port: 9229 + } as any) as Record; + + // js-debug's pwa-node attach defaults this to true, which parks every + // fork() of the inspected process in waitForDebugger + expect(cfg.autoAttachChildProcesses).toBe(false); + }); + + it('respects a caller-supplied autoAttachChildProcesses (issue #501)', () => { + const adapter = new JavascriptDebugAdapter(deps); + const optIn = adapter.transformAttachConfig({ + request: 'attach', + port: 9229, + autoAttachChildProcesses: true + } as any) as Record; + expect(optIn.autoAttachChildProcesses).toBe(true); + + const optOut = adapter.transformAttachConfig({ + request: 'attach', + port: 9229, + autoAttachChildProcesses: false + } as any) as Record; + expect(optOut.autoAttachChildProcesses).toBe(false); + }); + + it('lists autoAttachChildProcesses as a supported attach key (issue #501)', () => { + const adapter = new JavascriptDebugAdapter(deps); + expect(adapter.supportedAttachKeys).toContain('autoAttachChildProcesses'); + }); }); }); diff --git a/packages/shared/src/interfaces/adapter-policy-js.ts b/packages/shared/src/interfaces/adapter-policy-js.ts index 6a3b7225..655b98b0 100644 --- a/packages/shared/src/interfaces/adapter-policy-js.ts +++ b/packages/shared/src/interfaces/adapter-policy-js.ts @@ -448,6 +448,20 @@ export const JsDebugAdapterPolicy: AdapterPolicy = { if (typeof stopOnEntryValue === 'boolean') { attachArgs.stopOnEntry = stopOnEntryValue; } + // js-debug's pwa-node attach defaults autoAttachChildProcesses to true, + // which bootloads every fork() of the inspected process into + // waitForDebugger; with single-child adoption those forks wedge (#501). + // The MCP path already defaults this off in transformAttachConfig; this + // guard makes the policy self-contained for embedders that bypass the + // adapter transform. A caller-supplied boolean is respected, sourced + // like stopOnEntry above: launchConfig (via callerAttachExtras), then + // dapLaunchArgs. + if (typeof attachArgs.autoAttachChildProcesses !== 'boolean') { + attachArgs.autoAttachChildProcesses = + typeof a.autoAttachChildProcesses === 'boolean' + ? (a.autoAttachChildProcesses as boolean) + : false; + } try { console.info(`[JsDebugAdapterPolicy] [JS] Sending 'attach' to ${attachPort} (address=${attachHost})`); await pm.sendDapRequest('attach', attachArgs); @@ -748,11 +762,17 @@ export const JsDebugAdapterPolicy: AdapterPolicy = { const cfg = args?.configuration ?? {}; const pendingId: string | undefined = cfg?.__pendingTargetId; - // Send acknowledgment + // Send acknowledgment. The early success ack is correct: js-debug + // ignores the response body — a pending target is resolved only by + // a fresh DAP connection attaching with its __pendingTargetId. context.sendResponse(request, {}); - + if (pendingId && typeof pendingId === 'string') { - // Check if not already adopted + // Bookkeeping invariant (issues #249/#501): the id is added here, + // before the adoption/release runs; MinimalDapClient removes it + // again when adoption throws or the release fails, so a re-sent + // startDebugging can retry. Adopted and released targets stay + // recorded — both are settled server-side. if (!context.adoptedTargets.has(pendingId)) { context.adoptedTargets.add(pendingId); return { diff --git a/src/proxy/child-session-manager.ts b/src/proxy/child-session-manager.ts index 394a63ee..67831ec2 100644 --- a/src/proxy/child-session-manager.ts +++ b/src/proxy/child-session-manager.ts @@ -19,11 +19,26 @@ import path from 'path'; const logger = createLogger('child-session-manager'); +// Deferred, cached import of MinimalDapClient — deferred to break the static +// import cycle with minimal-dap.ts, cached so concurrent first uses (an +// adoption racing a release, issue #501) share one import() call: concurrent +// dynamic imports of the same module can resolve inconsistently under +// vitest's module mocker, handing one caller the real module and the other +// the mock. +let minimalDapModule: Promise | undefined; +function loadMinimalDap(): Promise { + minimalDapModule ??= import('./minimal-dap.js'); + return minimalDapModule; +} + function createInstanceId(): string { return randomBytes(4).toString('hex'); } -function createChildSafePolicy(policy: AdapterPolicy): AdapterPolicy { +function createChildSafePolicy( + policy: AdapterPolicy, + onUnadoptableChild?: (config: ChildSessionConfig) => void +): AdapterPolicy { if (!policy.supportsReverseStartDebugging) { return policy; } @@ -48,7 +63,13 @@ function createChildSafePolicy(policy: AdapterPolicy): AdapterPolicy { if (!result.handled) { return result; } - // Do not spawn grandchildren; acknowledge and stop. + // Do not spawn grandchildren — but do not strand them either + // (issue #501): js-debug delivers fork auto-attach startDebugging + // requests on the adopted child's connection, so hand the target + // to the owning manager, which releases it to run undebugged. + if (result.createChildSession && result.childConfig && onUnadoptableChild) { + onUnadoptableChild(result.childConfig); + } return { handled: true }; }; } @@ -66,6 +87,17 @@ export interface ChildSessionOptions { cdpBridgeFactory?: () => CdpFunctionBreakpointBridge; } +/** + * How a startDebugging reverse request was resolved (issue #501): + * - 'adopted': became the active child session + * - 'duplicate': already adopted or already released; nothing to do + * - 'released': could not be adopted (single-child limitation) — attached and + * immediately detached so the pending target runs undebugged + * - 'release-failed': the release attempt failed; the target may still be + * parked and a re-sent startDebugging will retry the release + */ +export type ChildSessionOutcome = 'adopted' | 'duplicate' | 'released' | 'release-failed'; + /** * Settle-once death signal for a child client during adoption (issue #248). * Modeled on JsDebugLaunchBarrier: pre-caught so an unused latch never becomes @@ -127,6 +159,8 @@ export class ChildSessionManager extends EventEmitter { private adoptedTargets = new Set(); private childSessions = new Map(); private activeChild: MinimalDapClient | null = null; + // Targets resumed undebugged because they could not be adopted (issue #501) + private releasedTargets = new Set(); // Breakpoint mirroring private storedBreakpoints = new Map(); @@ -401,25 +435,43 @@ export class ChildSessionManager extends EventEmitter { /** * Create and configure a child session */ - async createChildSession(config: ChildSessionConfig): Promise { + async createChildSession(config: ChildSessionConfig): Promise { const { pendingId, parentConfig } = config; - + // Check if already adopted if (this.adoptedTargets.has(pendingId)) { logger.warn(`Pending target ${pendingId} already adopted`); - return; + return 'duplicate'; } - + // Check if adoption is in progress or we already have a child if (this.adoptionInProgress || this.hasActiveChildren()) { - logger.info(`[ChildSessionManager:${this.instanceId}] Ignoring child session request; adoption in progress or child active`, { + if (this.releasedTargets.has(pendingId)) { + logger.info(`[ChildSessionManager:${this.instanceId}] Pending target ${pendingId} already released; ignoring`); + return 'duplicate'; + } + // Single-child limitation: this target cannot be adopted. Silently + // dropping the request leaves the forked process parked forever in + // waitForDebugger (issue #501) — instead, attach a throwaway connection + // and immediately detach so the child runs undebugged. + logger.info(`[ChildSessionManager:${this.instanceId}] Cannot adopt child session request; releasing target to run undebugged`, { + pendingId, adoptionInProgress: this.adoptionInProgress, hasActiveChild: !!this.activeChild, childSessionCount: this.childSessions.size }); - return; + // Stamped before the first await so a concurrent request for the same + // target cannot double-release; rolled back only on failure + this.releasedTargets.add(pendingId); + const released = await this.releaseUndebugged(config); + if (!released) { + this.releasedTargets.delete(pendingId); + return 'release-failed'; + } + logger.warn(`[ChildSessionManager:${this.instanceId}] startDebugging target ${pendingId} could not be adopted (one child session at a time); released to run UNDEBUGGED — breakpoints will not bind in that child process`); + return 'released'; } - + this.adoptionInProgress = true; logger.info(`[ChildSessionManager:${this.instanceId}] Setting adoptionInProgress = true for ${pendingId}`); this.adoptedTargets.add(pendingId); @@ -428,10 +480,10 @@ export class ChildSessionManager extends EventEmitter { let death: ChildDeathLatch | null = null; try { // Import MinimalDapClient dynamically to avoid circular dependency - const { MinimalDapClient } = await import('./minimal-dap.js'); + const { MinimalDapClient } = await loadMinimalDap(); // Create child client with a policy that disables recursive reverse debugging - const childPolicy = createChildSafePolicy(this.policy); + const childPolicy = this.buildChildSafePolicy(); child = new MinimalDapClient(this.host, this.port, childPolicy); await child.connect(); @@ -491,6 +543,7 @@ export class ChildSessionManager extends EventEmitter { logger.info(`[ChildSessionManager:${this.instanceId}] Child session created successfully for ${pendingId}`); this.emit('childCreated', pendingId, child); + return 'adopted'; } catch (error) { this.adoptionInProgress = false; @@ -636,6 +689,120 @@ export class ChildSessionManager extends EventEmitter { } } + /** + * Child-safe policy wired back to this manager: a startDebugging arriving + * on a child (or release) connection is handed to createChildSession, + * which — with a child already active — releases it (issue #501). + * Fire-and-forget: the DAP ack was already sent by the base policy, and a + * release must not block the child's message dispatch. + */ + private buildChildSafePolicy(): AdapterPolicy { + return createChildSafePolicy(this.policy, (config) => { + void this.createChildSession(config).catch((err) => { + const msg = err instanceof Error ? err.message : String(err); + logger.error(`[ChildSessionManager:${this.instanceId}] Forwarded unadoptable child ${config.pendingId} failed: ${msg}`); + }); + }); + } + + /** + * Resume a pending target that cannot be adopted, without debugging it + * (issue #501): the debug server only unparks a pending target when a DAP + * connection attaches with its __pendingTargetId — for js-debug the attach + * response means initAdapter completed and runIfWaitingForDebugger is about + * to run. Attach with a throwaway connection, give the resume a moment to + * land, then detach with terminateDebuggee: false. + * + * Never throws, never touches adoption state (activeChild/childSessions/ + * adoptedTargets) — a failed release must leave the parent session intact. + */ + private async releaseUndebugged(config: ChildSessionConfig): Promise { + const { pendingId, parentConfig } = config; + let releaseClient: MinimalDapClient | null = null; + try { + // Import MinimalDapClient dynamically to avoid circular dependency + const { MinimalDapClient } = await loadMinimalDap(); + // Child-safe policy: a grandchild startDebugging arriving on this + // socket is forwarded back to this manager (released) rather than + // recursing into adoption + const client = new MinimalDapClient(this.host, this.port, this.buildChildSafePolicy()); + releaseClient = client; + await this.withTimeout((async () => { + await client.connect(); + // js-debug rejects launch/attach before initialize; the pending-target + // attach handler additionally awaits configurationDone + await client.sendRequest('initialize', { + clientID: `mcp-release-${pendingId}`, + adapterID: this.policy.getDapAdapterConfiguration().type, + pathFormat: 'path', + linesStartAt1: true, + columnsStartAt1: true + }, 5000); + try { + await client.sendRequest('configurationDone', {}, 5000); + } catch { + logger.warn(`[release:${pendingId}] configurationDone failed or not required`); + } + const startArgs = this.policy.buildChildStartArgs(pendingId, parentConfig); + logger.info(`[release:${pendingId}] ${startArgs.command} (throwaway connection)`); + await client.sendRequest(startArgs.command, startArgs.args, 15000); + // The resume runs just after the attach response resolves; wait for + // the policy's ready signal so a fast disconnect cannot detach from a + // target that is still parked + await this.waitForReadySignal(client, 1500); + try { + await client.sendRequest('disconnect', { terminateDebuggee: false }, 3000); + } catch { + // Best effort — detaching is what matters, and shutdown() below + // closes the socket either way + } + })(), 20000, `release of pending target ${pendingId} timed out`); + return true; + } catch (error) { + const msg = error instanceof Error ? error.message : String(error); + logger.error(`[ChildSessionManager:${this.instanceId}] Failed to release pending target ${pendingId}: ${msg}`); + return false; + } finally { + try { + releaseClient?.shutdown('release complete'); + } catch { + // Socket may already be gone + } + } + } + + /** + * Wait for the policy's child-ready signal (e.g. js-debug posts 'thread' + * or an early 'stopped'); resolves false on timeout or client death. + */ + private waitForReadySignal(client: MinimalDapClient, timeoutMs: number): Promise { + return new Promise((resolve) => { + let done = false; + + const settle = (value: boolean) => { + if (done) return; + done = true; + client.off('event', onEvent); + client.off('close', onDeath); + client.off('error', onDeath); + clearTimeout(timer); + resolve(value); + }; + + const onEvent = (evt: DebugProtocol.Event) => { + if (evt && this.policy.isChildReadyEvent(evt)) { + settle(true); + } + }; + const onDeath = () => settle(false); + const timer = setTimeout(() => settle(false), timeoutMs); + + client.on('event', onEvent); + client.on('close', onDeath); + client.on('error', onDeath); + }); + } + /** * Bound a step with its own timer (used to cap attach attempts at the * remaining total deadline regardless of per-request timeouts) @@ -846,6 +1013,7 @@ export class ChildSessionManager extends EventEmitter { this.childSessions.clear(); this.activeChild = null; this.adoptedTargets.clear(); + this.releasedTargets.clear(); this.storedBreakpoints.clear(); } } diff --git a/src/proxy/minimal-dap.ts b/src/proxy/minimal-dap.ts index 2b03c1bf..eb52639a 100644 --- a/src/proxy/minimal-dap.ts +++ b/src/proxy/minimal-dap.ts @@ -261,9 +261,17 @@ export class MinimalDapClient extends EventEmitter { }, createChildSession: async (config: ChildSessionConfig) => { if (this.childSessionManager) { - await this.childSessionManager.createChildSession(this.enrichChildConfig(config)); + const outcome = await this.childSessionManager.createChildSession(this.enrichChildConfig(config)); // Update active child reference from manager this.activeChild = this.childSessionManager.getActiveChild(); + // A failed release keeps the target parked; forget it so a + // re-sent startDebugging can retry (parity with the #249 + // rollback below). 'released'/'duplicate' stay recorded — a + // released target's server-side deferred has settled and can + // never be adopted (issue #501) + if (outcome === 'release-failed') { + this.adoptedTargets.delete(config.pendingId); + } } }, activeChildren: this.childSessions as Map, @@ -278,10 +286,17 @@ export class MinimalDapClient extends EventEmitter { // Create child session through the manager logger.info(`[MinimalDapClient] Creating child session via ChildSessionManager`); try { - await this.childSessionManager.createChildSession(this.enrichChildConfig(result.childConfig)); + const outcome = await this.childSessionManager.createChildSession(this.enrichChildConfig(result.childConfig)); // Update active child reference from manager this.activeChild = this.childSessionManager.getActiveChild(); + // A failed release keeps the target parked; forget it so a + // re-sent startDebugging can retry (same reasoning as the + // #249 rollback in the catch). 'released'/'duplicate' stay + // recorded — a released target can never be adopted (#501) + if (outcome === 'release-failed' && typeof result.childConfig.pendingId === 'string') { + this.adoptedTargets.delete(result.childConfig.pendingId); + } } catch (err) { const msg = err instanceof Error ? err.message : String(err); logger.error(`[MinimalDapClient] Failed to create child session: ${msg}`); diff --git a/src/server.ts b/src/server.ts index 3e952277..d7d256cb 100644 --- a/src/server.ts +++ b/src/server.ts @@ -1195,7 +1195,7 @@ export class DebugMcpServer { stopOnEntry: { type: 'boolean', description: 'Stop on entry after attaching' }, justMyCode: { type: 'boolean', description: 'Only debug user code (skip library code)' }, breakOnExceptions: { type: 'string', enum: ['uncaught', 'all', 'none'], description: 'Break when exceptions are thrown: "uncaught" pauses at uncaught exceptions at the crash site; "all" also pauses on caught/raised exceptions (language-dependent). Default "none" — attach sessions never apply a language default (unlike launch)' }, - adapterConfig: { type: 'object', description: 'Adapter-specific attach configuration merged into the attach config before the adapter transforms it (e.g. C/C++/LLDB: program — the binary path for symbol resolution when /proc//maps paths are not openable, as in a kubectl-debug ephemeral container — or initCommands like "settings set target.exec-search-paths /proc//root"). Reserved keys request/__attachMode are ignored; set stopOnEntry via the top-level parameter. Keys the adapter does not recognize are still forwarded to the debugger and named in the response warning — a near-miss of a supported key gets a did-you-mean suggestion. js-debug pins its attach orchestration keys (address/port/continueOnAttach/attachExistingChildren) over caller values', additionalProperties: true } + adapterConfig: { type: 'object', description: 'Adapter-specific attach configuration merged into the attach config before the adapter transforms it (e.g. C/C++/LLDB: program — the binary path for symbol resolution when /proc//maps paths are not openable, as in a kubectl-debug ephemeral container — or initCommands like "settings set target.exec-search-paths /proc//root"). Reserved keys request/__attachMode are ignored; set stopOnEntry via the top-level parameter. Keys the adapter does not recognize are still forwarded to the debugger and named in the response warning — a near-miss of a supported key gets a did-you-mean suggestion. js-debug pins its attach orchestration keys (address/port/continueOnAttach/attachExistingChildren) over caller values; its autoAttachChildProcesses defaults to false on attach — child processes the target forks run undebugged (set it true to auto-attach children; only one child can be adopted at a time, further children are resumed undebugged)', additionalProperties: true } }, required: ['sessionId'] } diff --git a/tests/e2e/mcp-server-smoke-javascript-attach.test.ts b/tests/e2e/mcp-server-smoke-javascript-attach.test.ts index a583e8db..901fe48f 100644 --- a/tests/e2e/mcp-server-smoke-javascript-attach.test.ts +++ b/tests/e2e/mcp-server-smoke-javascript-attach.test.ts @@ -16,6 +16,12 @@ * Test 3 — stopOnEntry:false: attaching must NOT pause the target (the * js-debug child adoption path must not force an entry stop), and detach * must leave it alive. + * + * Tests 5/6 — forking targets (issue #501): attaching must not wedge child + * processes the target fork()s. By default the auto-attach bootloader is off, + * so forks run untouched; with autoAttachChildProcesses:true each fork parks + * under waitForDebugger and js-debug requests adoption — the single-child + * limitation means it must be released to run undebugged, not dropped. */ import { describe, it, expect, beforeAll, afterAll, afterEach } from 'vitest'; @@ -32,6 +38,7 @@ const __filename = fileURLToPath(import.meta.url); const __dirname = path.dirname(__filename); const ROOT = path.resolve(__dirname, '../..'); const TARGET_SCRIPT = path.resolve(ROOT, 'examples', 'javascript', 'attach_target.js'); +const FORK_TARGET_SCRIPT = path.resolve(ROOT, 'examples', 'javascript', 'fork_attach_target.js'); const BREAKPOINT_LINE = 11; // `counter += 1;` inside tick() function getFreePort(): Promise { @@ -56,12 +63,12 @@ interface Target { stdout: () => string; } -/** Spawn the tick target with an open inspector port and wait until it listens. */ -async function spawnTarget(): Promise { +/** Spawn a tick target with an open inspector port and wait until it listens. */ +async function spawnTarget(script: string = TARGET_SCRIPT): Promise { const port = await getFreePort(); const proc = spawn( process.execPath, - [`--inspect=127.0.0.1:${port}`, TARGET_SCRIPT], + [`--inspect=127.0.0.1:${port}`, script], { stdio: ['ignore', 'pipe', 'pipe'] } ); @@ -412,4 +419,101 @@ describe('MCP Server JavaScript Attach-Mode Smoke Tests', () => { await new Promise(r => setTimeout(r, 500)); expect(targetProcess!.exitCode, 'detach must leave the target process alive').toBeNull(); }, 120000); + + /** Count completed fork→parent IPC handshakes in the fork target's stdout. */ + function countHandshakes(target: Target): number { + return (target.stdout().match(/child-handshake /g) ?? []).length; + } + + /** Poll until the fork target completes a NEW handshake, or the deadline. */ + async function waitForHandshakeProgress(target: Target, baseline: number, deadlineMs: number): Promise { + const deadline = Date.now() + deadlineMs; + while (Date.now() < deadline) { + if (countHandshakes(target) > baseline) { + return true; + } + await new Promise(r => setTimeout(r, 500)); + } + return false; + } + + it('forked children keep completing their IPC handshake while attached (issue #501)', async () => { + const target = await spawnTarget(FORK_TARGET_SCRIPT); + targetProcess = target.proc; + + const attachResponse = await createSessionAndAttach(target.port, { stopOnEntry: false }); + expect(attachResponse.success, `attach failed: ${JSON.stringify(attachResponse)}`).toBe(true); + + // Forks started AFTER the attach must complete their fork→IPC→ack round + // trip. Pre-fix, js-debug's auto-attach bootloader (attach default: on) + // parked every fork in waitForDebugger and only one target could ever be + // adopted — each fork wedged until the fixture's 5s diagnostic timeout. + const baseline = countHandshakes(target); + const progressed = await waitForHandshakeProgress(target, baseline, 45000); + expect( + progressed, + `no fork completed its IPC handshake within 45s of attach (issue #501: children ` + + `parked by the auto-attach bootloader); target stdout:\n${target.stdout()}` + ).toBe(true); + expect( + target.stdout(), + 'a forked child hit the 5s wedge diagnostic while attached (issue #501)' + ).not.toContain('child-wedged'); + + const detachResult = await callToolSafely(mcpClient!, 'detach_from_process', { + sessionId: sessionId!, + terminateProcess: false + }); + expect(detachResult.success, `detach_from_process failed: ${JSON.stringify(detachResult)}`).toBe(true); + await new Promise(r => setTimeout(r, 500)); + expect(targetProcess!.exitCode, 'detach must leave the target process alive').toBeNull(); + }, 120000); + + it('releases forks it cannot adopt when autoAttachChildProcesses is opted in (issue #501)', async () => { + const target = await spawnTarget(FORK_TARGET_SCRIPT); + targetProcess = target.proc; + + const attachResponse = await createSessionAndAttach(target.port, { + stopOnEntry: false, + adapterConfig: { autoAttachChildProcesses: true } + }); + expect(attachResponse.success, `attach failed: ${JSON.stringify(attachResponse)}`).toBe(true); + + // The key is declared in supportedAttachKeys — opting in must not trip + // the unrecognized-adapterConfig-key warning (issue #466 mechanism) + const warning = String(attachResponse.warning ?? ''); + expect( + warning, + `autoAttachChildProcesses is a supported attach key but the attach warned about it: ${warning}` + ).not.toContain('autoAttachChildProcesses'); + + // With the bootloader ON, every fork parks under waitForDebugger and + // js-debug requests adoption for it. The single-child limitation means it + // cannot be adopted — it must be RELEASED to run undebugged (attach with + // its __pendingTargetId, then detach), so handshakes keep completing. + const baseline = countHandshakes(target); + const progressed = await waitForHandshakeProgress(target, baseline, 45000); + expect( + progressed, + `no fork completed its IPC handshake within 45s of attach with ` + + `autoAttachChildProcesses:true — unadoptable forks are not being released ` + + `(issue #501); target stdout:\n${target.stdout()}` + ).toBe(true); + + // Debugging the parent must remain intact after releases happened + const threadsResult = await callToolSafely(mcpClient!, 'list_threads', { sessionId: sessionId! }); + const threads = (threadsResult.threads as unknown[] | undefined) ?? []; + expect( + threads.length, + `list_threads failed after fork releases: ${JSON.stringify(threadsResult)}` + ).toBeGreaterThan(0); + + const detachResult = await callToolSafely(mcpClient!, 'detach_from_process', { + sessionId: sessionId!, + terminateProcess: false + }); + expect(detachResult.success, `detach_from_process failed: ${JSON.stringify(detachResult)}`).toBe(true); + await new Promise(r => setTimeout(r, 500)); + expect(targetProcess!.exitCode, 'detach must leave the target process alive').toBeNull(); + }, 120000); }); diff --git a/tests/proxy/child-session-manager.test.ts b/tests/proxy/child-session-manager.test.ts index 6f263f32..0a10307c 100644 --- a/tests/proxy/child-session-manager.test.ts +++ b/tests/proxy/child-session-manager.test.ts @@ -12,6 +12,9 @@ import { ChildSessionManager } from '../../src/proxy/child-session-manager.js'; class MockMinimalDapClient extends EventEmitter { // Knobs for death-aware adoption tests (issue #248); reset in beforeEach static lastInstance: MockMinimalDapClient | null = null; + // All clients created in order — a release (issue #501) creates a second + // client, so lastInstance alone cannot address the adoption child + static instances: MockMinimalDapClient[] = []; static hangCommands = new Set(); static failCommands = new Map(); static suppressInitialized = false; @@ -46,6 +49,7 @@ class MockMinimalDapClient extends EventEmitter { this.port = port; this.policy = policy; MockMinimalDapClient.lastInstance = this; + MockMinimalDapClient.instances.push(this); } async connect(): Promise { @@ -127,6 +131,7 @@ describe('ChildSessionManager', () => { beforeEach(() => { MockMinimalDapClient.lastInstance = null; + MockMinimalDapClient.instances = []; MockMinimalDapClient.hangCommands.clear(); MockMinimalDapClient.failCommands.clear(); MockMinimalDapClient.suppressInitialized = false; @@ -567,21 +572,21 @@ describe('ChildSessionManager', () => { }); - it('should handle adoption in progress correctly', async () => { + it('releases a target that arrives while adoption is in progress (issue #501)', async () => { const config1 = { pendingId: 'pending-1', host: 'localhost', port: 9229, parentConfig: {} }; - + const config2 = { pendingId: 'pending-2', host: 'localhost', port: 9229, parentConfig: {} }; - + vi.useFakeTimers(); try { // Start first adoption @@ -591,11 +596,194 @@ describe('ChildSessionManager', () => { const promise2 = manager.createChildSession(config2); await vi.advanceTimersByTimeAsync(20000); - await Promise.all([promise1, promise2]); + const [outcome1, outcome2] = await Promise.all([promise1, promise2]); - // Only one should succeed + // First is adopted; second cannot be, but instead of being silently + // dropped (leaving the forked process parked in waitForDebugger) it + // is attached-and-detached so it runs undebugged + expect(outcome1).toBe('adopted'); + expect(outcome2).toBe('released'); expect(manager.getActiveChild()).toBeDefined(); expect(manager.hasActiveChildren()).toBe(true); + + // The release rode a separate throwaway client with the minimal + // unpark sequence, then closed its socket + const releaseClient = MockMinimalDapClient.instances.find(c => + c.requests.some(r => + r.command === 'attach' && + (r.args as Record).__pendingTargetId === 'pending-2' + ) + ); + expect(releaseClient).toBeDefined(); + const commands = releaseClient!.requests.map(r => r.command); + expect(commands).toEqual(['initialize', 'configurationDone', 'attach', 'disconnect']); + const attachArgs = releaseClient!.requests[2].args as Record; + expect(attachArgs.continueOnAttach).toBe(true); + const disconnectArgs = releaseClient!.requests[3].args as Record; + expect(disconnectArgs.terminateDebuggee).toBe(false); + expect(releaseClient!.shutdownCalls).toEqual(['release complete']); + + // Adoption state is untouched by the release + expect(releaseClient).not.toBe(manager.getActiveChild()); + expect(manager.isAdopted('pending-2')).toBe(false); + expect((manager as any).childSessions.size).toBe(1); + } finally { + vi.useRealTimers(); + } + }); + + it('releases a target when a child is already active, exactly once (issue #501)', async () => { + vi.useFakeTimers(); + try { + const adoption = manager.createChildSession({ + pendingId: 'first-child', + host: 'localhost', + port: 9229, + parentConfig: {} + }); + await vi.advanceTimersByTimeAsync(20000); + expect(await adoption).toBe('adopted'); + const activeChild = manager.getActiveChild(); + + const release = manager.createChildSession({ + pendingId: 'forked-child', + host: 'localhost', + port: 9229, + parentConfig: {} + }); + await vi.advanceTimersByTimeAsync(20000); + expect(await release).toBe('released'); + + // A repeat request for the released target is a no-op duplicate + const clientCount = MockMinimalDapClient.instances.length; + const repeat = manager.createChildSession({ + pendingId: 'forked-child', + host: 'localhost', + port: 9229, + parentConfig: {} + }); + await vi.advanceTimersByTimeAsync(20000); + expect(await repeat).toBe('duplicate'); + expect(MockMinimalDapClient.instances.length).toBe(clientCount); + + // Active child undisturbed throughout + expect(manager.getActiveChild()).toBe(activeChild); + } finally { + vi.useRealTimers(); + } + }); + + it('reports release-failed and stays retryable when the release attach fails (issue #501)', async () => { + vi.useFakeTimers(); + try { + const adoption = manager.createChildSession({ + pendingId: 'first-child', + host: 'localhost', + port: 9229, + parentConfig: {} + }); + await vi.advanceTimersByTimeAsync(20000); + expect(await adoption).toBe('adopted'); + const activeChild = manager.getActiveChild(); + + MockMinimalDapClient.failCommands.set('attach', new Error('target gone')); + const failed = manager.createChildSession({ + pendingId: 'forked-child', + host: 'localhost', + port: 9229, + parentConfig: {} + }); + await vi.advanceTimersByTimeAsync(30000); + expect(await failed).toBe('release-failed'); + expect(manager.getActiveChild()).toBe(activeChild); + + // The failed release rolled its bookkeeping back: a re-sent + // startDebugging retries the release and can now succeed + MockMinimalDapClient.failCommands.clear(); + const retried = manager.createChildSession({ + pendingId: 'forked-child', + host: 'localhost', + port: 9229, + parentConfig: {} + }); + await vi.advanceTimersByTimeAsync(20000); + expect(await retried).toBe('released'); + } finally { + vi.useRealTimers(); + } + }); + + it('bounds a hung release and settles without touching the active child (issue #501)', async () => { + vi.useFakeTimers(); + try { + const adoption = manager.createChildSession({ + pendingId: 'first-child', + host: 'localhost', + port: 9229, + parentConfig: {} + }); + await vi.advanceTimersByTimeAsync(20000); + expect(await adoption).toBe('adopted'); + + MockMinimalDapClient.hangCommands.add('attach'); + const hung = manager.createChildSession({ + pendingId: 'forked-child', + host: 'localhost', + port: 9229, + parentConfig: {} + }); + // The release flow is bounded at 20s overall + await vi.advanceTimersByTimeAsync(25000); + expect(await hung).toBe('release-failed'); + expect(manager.getActiveChild()).toBeDefined(); + + // The hung throwaway socket was still torn down + const releaseClient = MockMinimalDapClient.instances.find(c => + c.requests.some(r => + r.command === 'attach' && + (r.args as Record).__pendingTargetId === 'forked-child' + ) + ); + expect(releaseClient!.shutdownCalls).toEqual(['release complete']); + } finally { + vi.useRealTimers(); + } + }); + + it('never mirrors breakpoints to a release client (issue #501)', async () => { + vi.useFakeTimers(); + try { + const adoption = manager.createChildSession({ + pendingId: 'first-child', + host: 'localhost', + port: 9229, + parentConfig: {} + }); + await vi.advanceTimersByTimeAsync(20000); + expect(await adoption).toBe('adopted'); + + const release = manager.createChildSession({ + pendingId: 'forked-child', + host: 'localhost', + port: 9229, + parentConfig: {} + }); + await vi.advanceTimersByTimeAsync(20000); + expect(await release).toBe('released'); + + const storePromise = manager.storeBreakpoints('/abs/app.js', [{ line: 5 }]); + await vi.advanceTimersByTimeAsync(0); + await storePromise; + + const releaseClient = MockMinimalDapClient.instances.find(c => + c.requests.some(r => + r.command === 'attach' && + (r.args as Record).__pendingTargetId === 'forked-child' + ) + ); + expect(releaseClient!.requests.filter(r => r.command === 'setBreakpoints')).toHaveLength(0); + const child = manager.getActiveChild() as unknown as MockMinimalDapClient; + expect(child.requests.filter(r => r.command === 'setBreakpoints').length).toBeGreaterThan(0); } finally { vi.useRealTimers(); } @@ -1134,6 +1322,46 @@ describe('ChildSessionManager', () => { await mgr.shutdown(); }); + it('releases a fork startDebugging arriving on the child connection (issue #501)', async () => { + const mgr = new ChildSessionManager({ policy: JsDebugAdapterPolicy, host: 'localhost', port: 9229 }); + await createChild(mgr); + + // js-debug delivers fork auto-attach startDebugging requests on the + // ADOPTED CHILD's connection, so the child-safe policy must hand them + // back to the manager for release instead of dropping them + const childPolicy = MockMinimalDapClient.lastInstance!.policy!; + const behavior = childPolicy.getDapClientBehavior(); + + vi.useFakeTimers(); + try { + const result = await behavior.handleReverseRequest!( + { + seq: 3, + type: 'request', + command: 'startDebugging', + arguments: { configuration: { __pendingTargetId: 'grandchild-1' } } + } as never, + { sendResponse: vi.fn(), adoptedTargets: new Set(), activeChildren: new Map() } as never + ); + expect(result).toEqual({ handled: true }); + // The forward is fire-and-forget; drive the release flow to completion + await vi.advanceTimersByTimeAsync(25000); + } finally { + vi.useRealTimers(); + } + + const releaseClient = MockMinimalDapClient.instances.find(c => + c.requests.some(r => + r.command === 'attach' && + (r.args as Record).__pendingTargetId === 'grandchild-1' + ) + ); + expect(releaseClient, 'the forwarded fork target must be released via a throwaway client').toBeDefined(); + expect(releaseClient!.shutdownCalls).toEqual(['release complete']); + + await mgr.shutdown(); + }); + it('survives failing configuration requests during adoption', async () => { MockMinimalDapClient.failCommands.set('setExceptionBreakpoints', new Error('no exception filters')); MockMinimalDapClient.failCommands.set('configurationDone', new Error('not required')); diff --git a/tests/unit/proxy/minimal-dap.test.ts b/tests/unit/proxy/minimal-dap.test.ts index 6306cae4..5167fca4 100644 --- a/tests/unit/proxy/minimal-dap.test.ts +++ b/tests/unit/proxy/minimal-dap.test.ts @@ -1033,6 +1033,51 @@ describe('MinimalDapClient', () => { ); }); + it('rolls back adoptedTargets when the release of an unadoptable target fails (issue #501)', async () => { + const stubManager = createChildSessionManagerStub(); + stubManager.createChildSession.mockResolvedValue('release-failed'); + const client = new MinimalDapClient('localhost', 5678, JsDebugAdapterPolicy, { + childSessionManagerFactory: () => stubManager as unknown as ChildSessionManager + }); + + const request: DebugProtocol.Request = { + seq: 1, + type: 'request', + command: 'startDebugging', + arguments: { configuration: { __pendingTargetId: 'parked-target' } } + }; + + await (client as any).handleProtocolMessage(request); + + // A re-sent startDebugging must be able to retry the release + expect((client as any).adoptedTargets.has('parked-target')).toBe(false); + await (client as any).handleProtocolMessage({ ...request, seq: 2 }); + expect(stubManager.createChildSession).toHaveBeenCalledTimes(2); + }); + + it('keeps a released target in adoptedTargets so it is not re-processed (issue #501)', async () => { + const stubManager = createChildSessionManagerStub(); + stubManager.createChildSession.mockResolvedValue('released'); + const client = new MinimalDapClient('localhost', 5678, JsDebugAdapterPolicy, { + childSessionManagerFactory: () => stubManager as unknown as ChildSessionManager + }); + + const request: DebugProtocol.Request = { + seq: 1, + type: 'request', + command: 'startDebugging', + arguments: { configuration: { __pendingTargetId: 'released-target' } } + }; + + await (client as any).handleProtocolMessage(request); + + // A released target's server-side deferred has settled — it can never + // be adopted; the policy short-circuits any re-sent request + expect((client as any).adoptedTargets.has('released-target')).toBe(true); + await (client as any).handleProtocolMessage({ ...request, seq: 2 }); + expect(stubManager.createChildSession).toHaveBeenCalledTimes(1); + }); + it('marks child breakpoint events with the child-origin key (issues #500/#495)', () => { const stubManager = createChildSessionManagerStub(); const client = new MinimalDapClient('localhost', 5678, JsDebugAdapterPolicy, { diff --git a/tests/unit/shared/adapter-policy-js.test.ts b/tests/unit/shared/adapter-policy-js.test.ts index ad71d67a..096baec6 100644 --- a/tests/unit/shared/adapter-policy-js.test.ts +++ b/tests/unit/shared/adapter-policy-js.test.ts @@ -334,5 +334,50 @@ describe('JsDebugAdapterPolicy', () => { ); expect(sendDapRequest.mock.calls.some(([cmd]) => cmd === 'launch')).toBe(false); }); + + it('defaults autoAttachChildProcesses to false in attach args, keeping a caller value (issue #501)', async () => { + const runAttachHandshake = async (dapLaunchArgs: Record) => { + vi.useFakeTimers(); + try { + const events = new EventEmitter(); + const sendDapRequest = vi.fn().mockResolvedValue({}); + const proxyManager = Object.assign(events, { + isRunning: () => true, + sendDapRequest, + removeListener: events.removeListener.bind(events) + }); + const context = { + proxyManager, + sessionId: 'session-501', + dapLaunchArgs, + scriptPath: '/workspace/app.js', + scriptArgs: [], + breakpoints: new Map() + }; + + const handshakePromise = JsDebugAdapterPolicy.performHandshake(context as any); + await Promise.resolve(); + events.emit('dap-event', 'initialized'); + await vi.advanceTimersByTimeAsync(0); + await handshakePromise; + + const attachCall = sendDapRequest.mock.calls.find(([cmd]) => cmd === 'attach'); + return attachCall?.[1] as Record; + } finally { + vi.useRealTimers(); + } + }; + + const defaulted = await runAttachHandshake({ + request: 'attach', attachSimplePort: 9229, type: 'pwa-node' + }); + expect(defaulted.autoAttachChildProcesses).toBe(false); + + const optedIn = await runAttachHandshake({ + request: 'attach', attachSimplePort: 9229, type: 'pwa-node', + autoAttachChildProcesses: true + }); + expect(optedIn.autoAttachChildProcesses).toBe(true); + }); }); });