From 8a68ab47d551b0a6d34388c93d988addc16f5733 Mon Sep 17 00:00:00 2001 From: Wbert Adrian Castro Vera Date: Wed, 4 Nov 2026 10:34:58 -0500 Subject: [PATCH 1/2] fix: wait for Darwin process group reaping --- packages/core/src/mcp/mcp-runtime-runner.ts | 12 +++- .../test/unit/mcp/mcp-runtime-runner.test.ts | 57 +++++++++++++++++++ 2 files changed, 67 insertions(+), 2 deletions(-) diff --git a/packages/core/src/mcp/mcp-runtime-runner.ts b/packages/core/src/mcp/mcp-runtime-runner.ts index 21765fc..f40e9c8 100644 --- a/packages/core/src/mcp/mcp-runtime-runner.ts +++ b/packages/core/src/mcp/mcp-runtime-runner.ts @@ -254,8 +254,16 @@ export async function cancelManagedTree ( process.kill(groupId, 0) await sleep(25) // group still exists (e.g. a grandchild) — keep polling } catch (err) { - if ((err as NodeJS.ErrnoException).code === 'ESRCH') return { confirmed: true } - return { confirmed: false, error: `process group check failed (${(err as NodeJS.ErrnoException).code})` } + const code = (err as NodeJS.ErrnoException).code + if (code === 'ESRCH') return { confirmed: true } + // Darwin skips zombies when probing a group and can return EPERM + // until they are reaped. Keep waiting for ESRCH within the same + // deadline; EPERM alone never proves that cleanup is safe. + if (platform === 'darwin' && code === 'EPERM') { + await sleep(25) + continue + } + return { confirmed: false, error: `process group check failed (${code})` } } } return { confirmed: false, error: 'managed npm process group still exists after the confirmation deadline' } diff --git a/packages/core/test/unit/mcp/mcp-runtime-runner.test.ts b/packages/core/test/unit/mcp/mcp-runtime-runner.test.ts index c0736e4..7532d16 100644 --- a/packages/core/test/unit/mcp/mcp-runtime-runner.test.ts +++ b/packages/core/test/unit/mcp/mcp-runtime-runner.test.ts @@ -45,6 +45,63 @@ function killTree (pid: number): void { } } +describe('cancelManagedTree (Unix termination confirmation)', () => { + it('waits for a transient Darwin EPERM to become ESRCH before confirming termination', async (t) => { + let probes = 0 + t.mock.method(process, 'kill', (pid: number, signal: NodeJS.Signals | number) => { + assert.strictEqual(pid, -1234) + if (signal === 0) { + probes++ + throw Object.assign(new Error('group probe'), { code: probes === 1 ? 'EPERM' : 'ESRCH' }) + } + assert.ok(signal === 'SIGTERM' || signal === 'SIGKILL') + return true + }) + + const outcome = await cancelManagedTree({ pid: 1234 }, Promise.resolve(), { + platform: 'darwin', + confirmMs: 5_000, + }) + + assert.deepStrictEqual(outcome, { confirmed: true }) + assert.strictEqual(probes, 2, 'confirmation requires a later ESRCH, not EPERM alone') + }) + + it('does not confirm termination when Darwin EPERM persists to the deadline', async (t) => { + t.mock.method(process, 'kill', (pid: number, signal: NodeJS.Signals | number) => { + assert.strictEqual(pid, -1234) + if (signal === 0) throw Object.assign(new Error('permission denied'), { code: 'EPERM' }) + return true + }) + const start = Date.now() + const outcome = await cancelManagedTree({ pid: 1234 }, Promise.resolve(), { + platform: 'darwin', + confirmMs: 50, + }) + + assert.strictEqual(outcome.confirmed, false) + assert.ok(outcome.error) + assert.ok(Date.now() - start < 5_000, 'confirmation remains bounded') + }) + + for (const { platform, code } of [ + { platform: 'linux', code: 'EPERM' }, + { platform: 'darwin', code: 'EACCES' }, + ] as const) { + it(`does not reinterpret ${code} on ${platform} as confirmed termination`, async (t) => { + t.mock.method(process, 'kill', (_pid: number, signal: NodeJS.Signals | number) => { + if (signal === 0) throw Object.assign(new Error('probe failed'), { code }) + return true + }) + + const outcome = await cancelManagedTree({ pid: 1234 }, Promise.resolve(), { platform }) + + assert.strictEqual(outcome.confirmed, false) + assert.match(outcome.error ?? '', new RegExp(`process group check failed \\(${code}\\)`)) + }) + } +}) + describe('cancelManagedTree (Windows termination branch)', () => { it('returns a bounded terminationError when taskkill never exits', async () => { const killer = fakeKiller() From eb30b9a2c51502da2afdbe6521fb8bcb6f3f0ea2 Mon Sep 17 00:00:00 2001 From: Wbert Adrian Castro Vera Date: Wed, 4 Nov 2026 10:38:43 -0500 Subject: [PATCH 2/2] test: strengthen Darwin termination checks and diagnostics --- packages/core/src/mcp/mcp-runtime-runner.ts | 13 ++++++------- .../core/test/unit/mcp/mcp-runtime-runner.test.ts | 7 ++++--- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/packages/core/src/mcp/mcp-runtime-runner.ts b/packages/core/src/mcp/mcp-runtime-runner.ts index f40e9c8..fa682d7 100644 --- a/packages/core/src/mcp/mcp-runtime-runner.ts +++ b/packages/core/src/mcp/mcp-runtime-runner.ts @@ -249,24 +249,23 @@ export async function cancelManagedTree ( if (!(await waitFor(closed, deadline))) { return { confirmed: false, error: 'npm process did not exit after SIGKILL' } } + let probeError: string | undefined while (Date.now() < deadline) { try { process.kill(groupId, 0) - await sleep(25) // group still exists (e.g. a grandchild) — keep polling + probeError = undefined } catch (err) { const code = (err as NodeJS.ErrnoException).code if (code === 'ESRCH') return { confirmed: true } + probeError = `process group check failed (${code})` // Darwin skips zombies when probing a group and can return EPERM // until they are reaped. Keep waiting for ESRCH within the same // deadline; EPERM alone never proves that cleanup is safe. - if (platform === 'darwin' && code === 'EPERM') { - await sleep(25) - continue - } - return { confirmed: false, error: `process group check failed (${code})` } + if (platform !== 'darwin' || code !== 'EPERM') return { confirmed: false, error: probeError } } + await sleep(25) // group still exists or its state is unconfirmed — keep polling } - return { confirmed: false, error: 'managed npm process group still exists after the confirmation deadline' } + return { confirmed: false, error: probeError ?? 'managed npm process group still exists after the confirmation deadline' } } catch (err) { return { confirmed: false, error: (err as Error).message } } diff --git a/packages/core/test/unit/mcp/mcp-runtime-runner.test.ts b/packages/core/test/unit/mcp/mcp-runtime-runner.test.ts index 7532d16..5df607d 100644 --- a/packages/core/test/unit/mcp/mcp-runtime-runner.test.ts +++ b/packages/core/test/unit/mcp/mcp-runtime-runner.test.ts @@ -48,13 +48,13 @@ function killTree (pid: number): void { describe('cancelManagedTree (Unix termination confirmation)', () => { it('waits for a transient Darwin EPERM to become ESRCH before confirming termination', async (t) => { let probes = 0 + const calls: Array<[number, NodeJS.Signals | number]> = [] t.mock.method(process, 'kill', (pid: number, signal: NodeJS.Signals | number) => { - assert.strictEqual(pid, -1234) + calls.push([pid, signal]) if (signal === 0) { probes++ throw Object.assign(new Error('group probe'), { code: probes === 1 ? 'EPERM' : 'ESRCH' }) } - assert.ok(signal === 'SIGTERM' || signal === 'SIGKILL') return true }) @@ -65,6 +65,7 @@ describe('cancelManagedTree (Unix termination confirmation)', () => { assert.deepStrictEqual(outcome, { confirmed: true }) assert.strictEqual(probes, 2, 'confirmation requires a later ESRCH, not EPERM alone') + assert.deepStrictEqual(calls, [[-1234, 'SIGTERM'], [-1234, 'SIGKILL'], [-1234, 0], [-1234, 0]]) }) it('does not confirm termination when Darwin EPERM persists to the deadline', async (t) => { @@ -80,7 +81,7 @@ describe('cancelManagedTree (Unix termination confirmation)', () => { }) assert.strictEqual(outcome.confirmed, false) - assert.ok(outcome.error) + assert.match(outcome.error ?? '', /process group check failed \(EPERM\)/) assert.ok(Date.now() - start < 5_000, 'confirmation remains bounded') })