diff --git a/gitnexus/src/mcp/server.ts b/gitnexus/src/mcp/server.ts index f6061e3bc..088043eaf 100644 --- a/gitnexus/src/mcp/server.ts +++ b/gitnexus/src/mcp/server.ts @@ -433,10 +433,37 @@ export function installSignalShutdown( on('SIGTERM', () => void shutdown(SHUTDOWN_EXIT_CODES.SIGTERM)); } +/** Stdio can outlive its launcher when another process holds the pipe open. */ +export function installParentShutdown( + shutdown: (exitCode?: number) => unknown, + parentPid = process.ppid, +): () => void { + if (parentPid <= 1) return () => {}; + const timer = setInterval(() => { + // Reparenting also detects death when the original PID has been reused. + let parentGone = process.ppid !== parentPid; + if (!parentGone) { + try { + process.kill(parentPid, 0); + } catch (error) { + // EPERM (or any unrecognized failure) is not proof of parent death. + parentGone = (error as NodeJS.ErrnoException).code === 'ESRCH'; + } + } + if (parentGone) { + clearInterval(timer); + void shutdown(0); + } + }, 1_000); + timer.unref(); + return () => clearInterval(timer); +} + export async function startMCPServer( backend: LocalBackend, repositoryPolicy?: McpRepositoryPolicy, ): Promise { + const parentPid = process.ppid; const validatedRepositoryPolicy = repositoryPolicy ?? (await createMcpRepositoryPolicy(backend)); const server = createMCPServer(backend, { repositoryPolicy: validatedRepositoryPolicy }); @@ -473,6 +500,7 @@ export async function startMCPServer( const shutdown = async (exitCode = 0) => { if (shuttingDown) return; shuttingDown = true; + stopParentWatch(); // Safety net: if backend.disconnect()/server.close() hangs, still exit so a // SIGINT/SIGTERM reliably terminates the process. Unref'd so the timer alone // never keeps the event loop alive. @@ -489,6 +517,7 @@ export async function startMCPServer( clearTimeout(forceExit); process.exit(exitCode); }; + const stopParentWatch = installParentShutdown(shutdown, parentPid); // Handle graceful shutdown. Node invokes signal listeners with the signal // NAME (e.g. 'SIGTERM') as the first argument; registering `shutdown` diff --git a/gitnexus/test/integration/mcp/server-startup.test.ts b/gitnexus/test/integration/mcp/server-startup.test.ts index bc3e214c6..c041b2910 100644 --- a/gitnexus/test/integration/mcp/server-startup.test.ts +++ b/gitnexus/test/integration/mcp/server-startup.test.ts @@ -21,7 +21,9 @@ */ import { describe, it, expect } from 'vitest'; -import { spawn, type ChildProcessWithoutNullStreams } from 'node:child_process'; +import { execFileSync, spawn, type ChildProcessWithoutNullStreams } from 'node:child_process'; +import { once } from 'node:events'; +import { tmpdir } from 'node:os'; import path from 'node:path'; import fs from 'node:fs'; import { fileURLToPath } from 'node:url'; @@ -172,6 +174,7 @@ function spawnMcpServer(): SpawnedServer { proc.stdin.write(frame); }, close: async () => { + if (proc.exitCode !== null || proc.signalCode !== null) return; proc.stdin.end(); // Give the server a moment to clean up; force-kill if it hangs. await new Promise((resolve) => { @@ -315,3 +318,147 @@ describe('MCP server end-to-end startup', () => { } }, 60_000); }); + +async function within(promise: Promise, milliseconds: number): Promise { + let timer: ReturnType; + try { + return await Promise.race([ + promise, + new Promise((_, reject) => { + timer = setTimeout( + () => reject(new Error('MCP lifecycle operation timed out')), + milliseconds, + ); + }), + ]); + } finally { + clearTimeout(timer!); + } +} + +const initializeMessage = { + jsonrpc: '2.0', + id: 1, + method: 'initialize', + params: { + protocolVersion: '2025-06-18', + capabilities: {}, + clientInfo: { name: 'gitnexus-lifecycle-test', version: '0.0.0' }, + }, +}; + +describe('MCP stdio process lifecycle', () => { + it.each(['before', 'after'] as const)('exits cleanly on EOF %s the handshake', async (when) => { + const server = spawnMcpServer(); + try { + if (when === 'after') { + server.send(initializeMessage); + expect(await server.nextMessage()).toMatchObject({ id: 1, result: {} }); + } + const exited = once(server.proc, 'exit'); + server.proc.stdin.end(); + expect(await within(exited, TOTAL_BUDGET_MS)).toEqual([0, null]); + } finally { + await server.close(); + } + }); + + it('keeps a second MCP client usable when the first client disconnects', async () => { + const first = spawnMcpServer(); + const second = spawnMcpServer(); + try { + first.send(initializeMessage); + second.send(initializeMessage); + expect(await first.nextMessage()).toMatchObject({ id: 1, result: {} }); + expect(await second.nextMessage()).toMatchObject({ id: 1, result: {} }); + const firstExited = once(first.proc, 'exit'); + first.proc.stdin.end(); + expect(await within(firstExited, TOTAL_BUDGET_MS)).toEqual([0, null]); + second.send({ jsonrpc: '2.0', id: 2, method: 'ping' }); + expect(await second.nextMessage()).toMatchObject({ id: 2, result: {} }); + } finally { + await first.close(); + await second.close(); + } + }); + + // A FIFO lets the client keep stdin open independently of the launching + // parent. ChildProcess.stdin would close automatically when that parent dies, + // accidentally testing EOF again. Windows has no mkfifo; unit tests cover its + // unchanged-PPID / missing-parent liveness probe instead. + it.skipIf(process.platform === 'win32')( + 'exits when its parent dies even if stdin stays open', + async () => { + const home = fs.mkdtempSync(path.join(tmpdir(), 'gitnexus-parent-exit-')); + const fifo = path.join(home, 'stdin'); + execFileSync('mkfifo', [fifo]); + const fd = fs.openSync(fifo, fs.constants.O_RDWR); + const parent = spawn( + process.execPath, + [ + '-e', + ` + const { spawn } = require('node:child_process'); + const child = spawn(process.execPath, [process.argv[1], 'mcp'], { stdio: [0, 1, 2] }); + process.send({ pid: child.pid }); + setInterval(() => {}, 1000); + `, + DIST_CLI, + ], + { + cwd: REPO_ROOT, + env: { ...process.env, GITNEXUS_HOME: home, NODE_OPTIONS: '' }, + stdio: [fd, 'pipe', 'pipe', 'ipc'], + }, + ); + let serverPid: number | undefined; + let output = ''; + let errors = ''; + parent.stdout!.setEncoding('utf8'); + parent.stdout!.on('data', (chunk: string) => { + output += chunk; + }); + parent.stderr!.on('data', (chunk: Buffer) => { + errors += chunk.toString(); + }); + try { + const [message] = await within(once(parent, 'message'), TOTAL_BUDGET_MS); + serverPid = (message as { pid: number }).pid; + fs.writeSync(fd, JSON.stringify(initializeMessage) + '\n'); + await expect.poll(() => output, { timeout: FIRST_FRAME_BUDGET_MS }).toContain('"id":1'); + expect(JSON.parse(output.trim())).toMatchObject({ id: 1, result: {} }); + + // A live, idle parent must not be mistaken for a disconnected client. + await new Promise((resolve) => setTimeout(resolve, 1_200)); + expect(parent.stdout!.readableEnded).toBe(false); + output = ''; + fs.writeSync(fd, JSON.stringify({ jsonrpc: '2.0', id: 2, method: 'ping' }) + '\n'); + await expect.poll(() => output, { timeout: FIRST_FRAME_BUDGET_MS }).toContain('"id":2'); + + const parentExited = once(parent, 'exit'); + parent.kill('SIGKILL'); + await within(parentExited, TOTAL_BUDGET_MS); + await expect + .poll(() => parent.stdout!.readableEnded, { timeout: TOTAL_BUDGET_MS, message: errors }) + .toBe(true); + expect(errors).not.toMatch(/uncaughtException|unhandledRejection/); + } finally { + if (!parent.stdout!.readableEnded && serverPid !== undefined) { + try { + process.kill(serverPid, 'SIGKILL'); + } catch { + /* already exited */ + } + } + if (parent.exitCode === null && parent.signalCode === null) { + const exited = once(parent, 'exit'); + parent.kill('SIGKILL'); + await within(exited, TOTAL_BUDGET_MS); + } + fs.closeSync(fd); + fs.rmSync(home, { recursive: true, force: true }); + } + }, + 60_000, + ); +}); diff --git a/gitnexus/test/unit/server.test.ts b/gitnexus/test/unit/server.test.ts index 5b48201f3..f806fb7fe 100644 --- a/gitnexus/test/unit/server.test.ts +++ b/gitnexus/test/unit/server.test.ts @@ -12,7 +12,7 @@ * NOTE: We test the server handler logic by calling the request handlers * directly through the MCP Server's handler dispatch. */ -import { describe, it, expect, vi } from 'vitest'; +import { afterEach, beforeEach, describe, it, expect, vi } from 'vitest'; import { Client } from '@modelcontextprotocol/sdk/client/index.js'; import { Server } from '@modelcontextprotocol/sdk/server/index.js'; import { InMemoryTransport } from '@modelcontextprotocol/sdk/inMemory.js'; @@ -21,6 +21,7 @@ import { createMCPServer, getNextStepHint, installSignalShutdown, + installParentShutdown, startMCPServer, SHUTDOWN_EXIT_CODES, } from '../../src/mcp/server.js'; @@ -682,6 +683,7 @@ describe('prompt registration', () => { describe('startMCPServer', () => { it('registers stdin shutdown listeners before awaiting Server.prototype.connect', async () => { + vi.useFakeTimers(); // P1 finding: startMCPServer must register process.stdin end/close/error // shutdown listeners before calling server.connect(), so that a stdin // closure during transport setup is safely handled. @@ -718,6 +720,8 @@ describe('startMCPServer', () => { try { await startMCPServer(backend); } finally { + vi.clearAllTimers(); + vi.useRealTimers(); connectSpy.mockRestore(); exitStub.mockRestore(); // Remove only listeners added by startMCPServer so no side effects leak. @@ -762,6 +766,81 @@ describe('startMCPServer', () => { }); }); +describe('installParentShutdown', () => { + beforeEach(() => { + vi.useFakeTimers(); + vi.spyOn(process, 'ppid', 'get').mockReturnValue(1234); + vi.spyOn(process, 'kill').mockReturnValue(true); + }); + + afterEach(() => { + vi.clearAllTimers(); + vi.useRealTimers(); + vi.restoreAllMocks(); + }); + + it('leaves a live parent alone and stops probing when disposed', () => { + const shutdown = vi.fn(); + const stop = installParentShutdown(shutdown); + vi.advanceTimersByTime(1_000); + expect(process.kill).toHaveBeenCalledWith(1234, 0); + expect(shutdown).not.toHaveBeenCalled(); + stop(); + vi.advanceTimersByTime(2_000); + expect(process.kill).toHaveBeenCalledTimes(1); + }); + + it('shuts down once after reparenting, including adoption by a subreaper', () => { + const shutdown = vi.fn(); + installParentShutdown(shutdown); + vi.spyOn(process, 'ppid', 'get').mockReturnValue(4321); + vi.advanceTimersByTime(3_000); + expect(shutdown).toHaveBeenCalledExactlyOnceWith(0); + expect(process.kill).not.toHaveBeenCalled(); + }); + + it('detects a missing parent even when the OS retains the original PPID', () => { + const shutdown = vi.fn(); + installParentShutdown(shutdown); + vi.mocked(process.kill).mockImplementation(() => { + throw Object.assign(new Error('No such process'), { code: 'ESRCH' }); + }); + vi.advanceTimersByTime(3_000); + expect(shutdown).toHaveBeenCalledExactlyOnceWith(0); + }); + + it.each(['EPERM', 'EACCES', undefined])( + 'keeps serving after an inconclusive probe (%s)', + (code) => { + const shutdown = vi.fn(); + installParentShutdown(shutdown); + vi.mocked(process.kill).mockImplementation(() => { + throw Object.assign(new Error('Cannot inspect parent'), { code }); + }); + vi.advanceTimersByTime(2_000); + expect(shutdown).not.toHaveBeenCalled(); + expect(process.kill).toHaveBeenCalledTimes(2); + }, + ); + + it('does not keep the event loop alive', () => { + const interval = vi.spyOn(globalThis, 'setInterval'); + const stop = installParentShutdown(vi.fn()); + expect(interval.mock.results[0].value.hasRef()).toBe(false); + stop(); + }); + + it.each([0, 1])( + 'does not probe a process group or init when started under PPID %s', + (parentPid) => { + const shutdown = vi.fn(); + installParentShutdown(shutdown, parentPid)(); + expect(vi.getTimerCount()).toBe(0); + expect(shutdown).not.toHaveBeenCalled(); + }, + ); +}); + // ─── Graceful shutdown signal handling (#1132) ──────────────────────── describe('installSignalShutdown (#1132)', () => {