diff --git a/CHANGELOG.md b/CHANGELOG.md index 8034400c8..a8adf784a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -76,6 +76,7 @@ and adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). - A project a session queries through `projectPath` is now let go once it has gone its own 10 minutes without a query. Before, when its index was still being brought up to date as the session went on to query another project, it was held, locked, until that other project's 10 minutes were up, so its own session and `codegraph index` could wait nearly twice as long to take over. Thanks @bompus. (#2087) - An index left incomplete by an indexing run that was stopped partway, by a crash, an out-of-memory kill or the server's own hang check, no longer gets the background server killed with "Main thread unresponsive" every time it starts: on a large project, restoring the database indexes the run had set aside took longer than the hang check allows, so each restart died at the same point and the index was never repaired. They are now restored in the background, and the project is served once they are back. Thanks @cbeaulieu-gt for the report and the first fix. (#1887) - A new agent session no longer waits up to about 12 minutes for its first answer when the background server is stuck: a server whose process still holds its connection open but never responds made the session wait out a long series of retries. The session now gives up within seconds and answers on its own, and switches back to the background server once that has been restarted. +- An MCP session whose agent stops reading its answers, while keeping the session open, now shuts down the normal way the next time it answers: it stops its work, frees the project for the next session and exits cleanly, and other sessions sharing the background server carry on. Before, that answer crashed it with "Uncaught exception: Error: EPIPE". Thanks @iiijiashu for the report and the first fix. (#2991) - The bundled Node runtime moves to 24.22.1, Node's October security release, and every release now checks the Node it bundles against the checksums nodejs.org publishes. The new runtime also fixes a crash on Windows as a command like `codegraph sync` exits, after its work is done, printing `Assertion failed: !(handle->flags & UV_HANDLE_CLOSING)`, which made a script or CI run that had succeeded fail. Thanks @Ancienttwo for the report and the first fix, and @IdealBoy709 for the report on the security release. (#2617, #2780) - Building CodeGraph from source with bun, as in `bun run build`, no longer runs until it is stopped: bun ran one step of the build as the whole build again, so it kept starting itself over. Building with npm is unchanged. Thanks @bompus. - When CodeGraph can't open a project's database, it now says what went wrong: SQLite's error, the file's path, and what to check, such as another program holding the file open, a folder you can't write to, or a network share. Before, every such failure read "CodeGraph requires node:sqlite (Node.js 22.5+)", and an agent was told no project was loaded. That hint now appears only when Node really can't load its built-in SQLite, a case that used to end in an unhandled error instead. diff --git a/__tests__/mcp-stdout-failure.test.ts b/__tests__/mcp-stdout-failure.test.ts new file mode 100644 index 000000000..7b556ff50 --- /dev/null +++ b/__tests__/mcp-stdout-failure.test.ts @@ -0,0 +1,238 @@ +/** + * A host that stops reading an MCP server's output, while keeping its stdin + * open and staying alive itself, must get the server's normal shutdown, not a + * crash (#2991). + * + * The next response the server wrote failed (EPIPE), and with no listener on + * stdout Node handed the error to the last-resort fatal handler, which exits 1 + * at once. That skipped the shutdown that stops the engine, releases the + * writer lock and waits for query workers still starting, which on Windows can + * crash a process that exits under them. Stdin, the host's PID and the + * liveness watchdog all still look healthy, so nothing else notices. + * + * Each case runs a real server or proxy in a child process, destroys the + * test's end of the child's stdout, then sends one more request so its + * response meets the broken pipe. + */ +import { describe, it, expect, afterEach } from 'vitest'; +import { spawn, type ChildProcessWithoutNullStreams } from 'child_process'; +import * as fs from 'fs'; +import * as net from 'net'; +import * as os from 'os'; +import * as path from 'path'; +import CodeGraph from '../src'; +import { WASM_RUNTIME_FLAGS } from '../src/extraction/wasm-runtime-flags'; + +const BIN = path.resolve(__dirname, '../dist/bin/codegraph.js'); +const PROXY = path.resolve(__dirname, '../dist/mcp/proxy.js'); + +const line = (msg: object): string => JSON.stringify({ jsonrpc: '2.0', ...msg }) + '\n'; +const INITIALIZE = line({ + id: 1, + method: 'initialize', + params: { protocolVersion: '2025-06-18', capabilities: {}, clientInfo: { name: 'test', version: '0.0.0' } }, +}); +const call = (id: number, name = 'codegraph_status'): string => + line({ id, method: 'tools/call', params: { name, arguments: {} } }); +/** No watchdogs, prompt hook or handshake timer: only the stdout failure can end these processes. */ +const QUIET = { + CODEGRAPH_TELEMETRY: '0', DO_NOT_TRACK: '1', CODEGRAPH_NO_PROMPT_HOOK: '1', + CODEGRAPH_NO_WATCHDOG: '1', CODEGRAPH_PPID_POLL_MS: '0', CODEGRAPH_STARTUP_HANDSHAKE_TIMEOUT_MS: '0', +}; + +interface Child { + child: ChildProcessWithoutNullStreams; + out: () => string; + err: () => string; + exit: Promise; + write: (s: string) => void; +} + +const delay = (ms: number) => new Promise((resolve) => setTimeout(resolve, ms)); + +async function until(check: () => boolean, what: string, timeout = 20_000): Promise { + const deadline = Date.now() + timeout; + while (!check()) { + if (Date.now() > deadline) throw new Error(`Timed out waiting for ${what}`); + await delay(25); + } +} + +/** Remove `dir` once nothing holds it: Windows can keep it busy a moment after a child exits. */ +async function removeWhenReleased(dir: string, timeoutMs = 5_000): Promise { + const deadline = Date.now() + timeoutMs; + for (;;) { + try { + fs.rmSync(dir, { recursive: true, force: true }); + return; + } catch (error) { + const code = (error as NodeJS.ErrnoException).code ?? ''; + if (!['EBUSY', 'ENOTEMPTY', 'EPERM'].includes(code) || Date.now() > deadline) throw error; + await delay(50); + } + } +} + +function answered(c: Child, id: number): boolean { + return c.out().split('\n').some((l) => { + try { return JSON.parse(l).id === id; } catch { return false; } + }); +} + +describe('MCP stdio output failure (#2991)', () => { + const children: Child[] = []; + const dirs: string[] = []; + const cleanups: Array<() => Promise | void> = []; + + afterEach(async () => { + for (const { child, exit } of children.splice(0)) { + if (child.exitCode === null && child.signalCode === null) child.kill('SIGKILL'); + await exit; + } + for (const cleanup of cleanups.splice(0)) await cleanup(); + for (const dir of dirs.splice(0)) await removeWhenReleased(dir); + }); + + function start(args: string[], env: Record = {}): Child { + const child = spawn(process.execPath, args, { + // Off every fixture, so a child still exiting cannot hold one (Windows EBUSY). + cwd: os.tmpdir(), + env: { ...process.env, ...QUIET, ...env }, + stdio: ['pipe', 'pipe', 'pipe'], + windowsHide: true, + }) as ChildProcessWithoutNullStreams; + let out = ''; + let err = ''; + child.stdout.on('data', (d) => { out += d; }); + child.stdout.on('error', () => { /* destroyed by the test on purpose */ }); + child.stderr.on('data', (d) => { err += d; }); + child.stdin.on('error', () => { /* the child is gone */ }); + const exit = new Promise((resolve) => child.once('exit', (code) => resolve(code))); + const handle: Child = { + child, out: () => out, err: () => err, exit, + write: (s) => { if (child.exitCode === null) child.stdin.write(s); }, + }; + children.push(handle); + return handle; + } + + /** The host stops reading; the next response meets the broken pipe. Stdin stays open. */ + async function breakOutputThenCall(c: Child, id: number, name?: string): Promise { + c.child.stdout.destroy(); + c.write(call(id, name)); + let timer: NodeJS.Timeout | undefined; + const timedOut = new Promise<'timeout'>((resolve) => { timer = setTimeout(() => resolve('timeout'), 30_000); }); + const result = await Promise.race([c.exit, timedOut]); + clearTimeout(timer); + if (result === 'timeout') throw new Error(`still running 30 s after its output failed:\n${c.err()}`); + return result; + } + + it('stops a direct-mode server through its normal shutdown, which releases the writer lock', async () => { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'cg-stdout-direct-')); + dirs.push(dir); + CodeGraph.initSync(dir).close(); + const writerPid = path.join(dir, '.codegraph', 'writer.pid'); + + // The runtime flags make this child the server itself, not a relauncher. + const server = start([...WASM_RUNTIME_FLAGS, BIN, 'serve', '--mcp', '--path', dir], { CODEGRAPH_NO_DAEMON: '1' }); + server.write(INITIALIZE); + await until(() => answered(server, 1), 'the initialize response'); + server.write(line({ method: 'notifications/initialized' })); + server.write(call(2)); + await until(() => answered(server, 2), 'the first tool call'); + expect(JSON.parse(fs.readFileSync(writerPid, 'utf8')).pid).toBe(server.child.pid); + + expect(await breakOutputThenCall(server, 3), server.err()).toBe(0); + expect(server.err()).not.toMatch(/Uncaught exception/); + expect(fs.existsSync(writerPid)).toBe(false); + }, 60_000); + + it('stops the local proxy\'s in-process engine before the proxy exits', async () => { + const proxy = start(['-e', ` + const fs = require('fs'); + const { runLocalHandshakeProxy } = require(${JSON.stringify(PROXY)}); + runLocalHandshakeProxy({ + root: process.cwd(), + getDaemonSocket: async () => null, + makeEngine: () => ({ + ensureInitialized: async () => {}, + isReadOnly: () => true, + getToolHandler: () => ({ execute: async () => ({ content: [{ type: 'text', text: 'ok' }] }) }), + stop: () => new Promise((resolve) => setTimeout(resolve, 100)).then(() => fs.writeSync(2, 'ENGINE STOPPED\\n')), + }), + }); + `], { CODEGRAPH_DAEMON_RETRY_MS: '0' }); + proxy.write(INITIALIZE); + proxy.write(call(2, 'test')); + await until(() => answered(proxy, 2), 'the first tool call'); + + expect(await breakOutputThenCall(proxy, 3, 'test'), proxy.err()).toBe(0); + expect(proxy.err()).toContain('ENGINE STOPPED'); + expect(proxy.err()).not.toMatch(/Uncaught exception/); + }, 60_000); + + it('detaches the local proxy from the shared daemon and leaves the daemon\'s other clients alone', async () => { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'cg-stdout-daemon-')); + dirs.push(dir); + const socketPath = process.platform === 'win32' + ? `\\\\.\\pipe\\cg-stdout-daemon-${process.pid}-${Date.now()}` + : path.join(dir, 'd.sock'); + // Stands in for the shared daemon: answers every request on every connection. + const connections = new Set(); + const daemon = net.createServer((socket) => { + connections.add(socket); + socket.on('close', () => connections.delete(socket)); + socket.on('error', () => { /* only this fixture's clients */ }); + let buffer = ''; + socket.on('data', (chunk) => { + buffer += String(chunk); + let newline: number; + while ((newline = buffer.indexOf('\n')) >= 0) { + const request = buffer.slice(0, newline); + buffer = buffer.slice(newline + 1); + try { + const msg = JSON.parse(request); + if (msg.id !== undefined) socket.write(line({ id: msg.id, result: { content: [{ type: 'text', text: 'daemon' }] } })); + } catch { /* not a request */ } + } + }); + }); + await new Promise((resolve) => daemon.listen(socketPath, resolve)); + cleanups.push(() => new Promise((resolve) => { + for (const socket of connections) socket.destroy(); + daemon.close(() => resolve()); + })); + const other = net.createConnection(socketPath); + cleanups.unshift(() => { other.destroy(); }); + await new Promise((resolve, reject) => { other.once('connect', () => resolve()); other.once('error', reject); }); + await until(() => connections.size === 1, 'the other client to connect'); + + const proxy = start(['-e', ` + const net = require('net'); + const { runLocalHandshakeProxy } = require(${JSON.stringify(PROXY)}); + runLocalHandshakeProxy({ + root: process.cwd(), + getDaemonSocket: async () => { + const socket = net.createConnection(${JSON.stringify(socketPath)}); + await new Promise((resolve, reject) => { socket.once('connect', resolve); socket.once('error', reject); }); + return socket; + }, + makeEngine: () => { throw new Error('no in-process engine while the daemon serves'); }, + }); + `], { CODEGRAPH_DAEMON_RETRY_MS: '0' }); + proxy.write(INITIALIZE); + proxy.write(call(2, 'test')); + await until(() => answered(proxy, 2), 'a tool call answered through the daemon'); + expect(connections.size).toBe(2); + + expect(await breakOutputThenCall(proxy, 3, 'test'), proxy.err()).toBe(0); + expect(proxy.err()).not.toMatch(/Uncaught exception/); + await until(() => connections.size === 1, 'only the proxy\'s own connection to close'); + expect(daemon.listening).toBe(true); + let reply = ''; + other.on('data', (chunk) => { reply += String(chunk); }); + other.write(call(9, 'test')); + await until(() => reply.includes('"id":9'), 'the daemon to answer its other client'); + }, 60_000); +}); diff --git a/__tests__/stdout-teardown.test.ts b/__tests__/stdout-teardown.test.ts new file mode 100644 index 000000000..ec2041a77 --- /dev/null +++ b/__tests__/stdout-teardown.test.ts @@ -0,0 +1,57 @@ +/** + * #2991 — a stdout that fails (the host stopped reading it) must shut the + * server down through its own shutdown, not the fatal handler. + * treatStdoutFailureAsShutdown is the shared guard; the real processes are + * covered by mcp-stdout-failure.test.ts. + */ +import { describe, it, expect } from 'vitest'; +import { PassThrough } from 'stream'; +import { treatStdoutFailureAsShutdown } from '../src/mcp/stdout-teardown'; + +describe('treatStdoutFailureAsShutdown (#2991)', () => { + it.each(['EPIPE', 'ECONNRESET', 'ERR_STREAM_DESTROYED'])("treats a stdout 'error' (%s) as a shutdown signal", (code) => { + const s = new PassThrough(); + let calls = 0; + treatStdoutFailureAsShutdown(() => { calls++; }, s); + + // Without the guard's listener this emit would throw. + s.emit('error', Object.assign(new Error(`write ${code}`), { code })); + expect(calls).toBe(1); + }); + + it("also fires on 'close'", () => { + const s = new PassThrough(); + let calls = 0; + treatStdoutFailureAsShutdown(() => { calls++; }, s); + s.emit('close'); + expect(calls).toBe(1); + }); + + it('destroys the stream so later writes are dropped', () => { + const s = new PassThrough(); + treatStdoutFailureAsShutdown(() => { /* noop */ }, s); + s.emit('error', new Error('write EPIPE')); + expect(s.destroyed).toBe(true); + }); + + it('fires onTerminal at most once, and absorbs a write that fails during the shutdown', async () => { + const s = new PassThrough(); + let calls = 0; + treatStdoutFailureAsShutdown(() => { calls++; }, s); + s.emit('error', new Error('write EPIPE')); // fire() also destroys → emits 'close' + s.write('a response queued before the failure'); + await new Promise((resolve) => setImmediate(resolve)); + s.emit('close'); + expect(calls).toBe(1); + }); + + it('leaves a stream under backpressure alone', async () => { + const s = new PassThrough({ highWaterMark: 1 }); + let calls = 0; + treatStdoutFailureAsShutdown(() => { calls++; }, s); + expect(s.write('a response the host has not read yet')).toBe(false); + await new Promise((resolve) => setImmediate(resolve)); + expect(calls).toBe(0); + expect(s.destroyed).toBe(false); + }); +}); diff --git a/src/mcp/proxy.ts b/src/mcp/proxy.ts index b9fa35d80..5c0e166e3 100644 --- a/src/mcp/proxy.ts +++ b/src/mcp/proxy.ts @@ -26,6 +26,7 @@ import { EARLY_PPID } from './early-ppid'; import { supervisionLostReason } from './ppid-watchdog'; import { armStartupHandshakeTimeout } from './startup-handshake'; import { treatStdinFailureAsShutdown } from './stdin-teardown'; +import { treatStdoutFailureAsShutdown } from './stdout-teardown'; import { CodeGraphPackageVersion, isOlderRelease } from './version'; import { SERVER_INFO, PROTOCOL_VERSION, initializeInstructions } from './session'; import { SERVER_INSTRUCTIONS } from './server-instructions'; @@ -443,6 +444,10 @@ export async function runLocalHandshakeProxy(deps: LocalHandshakeDeps): Promise< // instead of a clean close; destroying the stream stops a hung fd from // busy-spinning the event loop (#799). treatStdinFailureAsShutdown(shutdown); + // A host can also stop reading our output while it keeps stdin open and + // stays alive: shut down the same way, which closes only this session's + // daemon connection and stops its own engines (#2991). + treatStdoutFailureAsShutdown(shutdown); startPpidWatchdogNoSocket(shutdown); // Backstop for a launch abandoned before any of the above can see it: killed // launcher + held-open pipes + reparent that beat the EARLY_PPID capture diff --git a/src/mcp/stdout-teardown.ts b/src/mcp/stdout-teardown.ts new file mode 100644 index 000000000..aee0020e4 --- /dev/null +++ b/src/mcp/stdout-teardown.ts @@ -0,0 +1,42 @@ +/** + * Treat a failed stdout as a shutdown signal — issue #2991. + * + * A host can stop reading an MCP stdio server's output while it keeps the + * server's stdin open and stays alive itself. The next response the server + * writes then fails (EPIPE on a pipe, ECONNRESET on a socket), and with no + * `'error'` listener on stdout Node hands it to the last-resort fatal handler, + * which exits 1 at once (`src/bin/fatal-handler.ts`). That skips the + * shutdown that stops the engine, releases the writer lock and waits for query + * workers still starting (a process that exits under one of those can crash + * on Windows). Nothing else notices this failure: stdin is still open, the + * host's PID is alive and the event loop is healthy, so the stdin teardown + * (#799), the PPID watchdog and the liveness watchdog all stay quiet. + * + * So listen for `'error'`, and for the `'close'` that follows a failed stream, + * destroy stdout so later writes are dropped, and run the owner's shutdown, + * at most once. The listener stays attached: a write queued before the + * failure can still fail while the shutdown runs. A `write()` that returns + * false is backpressure, not failure, and is left alone. A reader that goes + * away while the server is silent is noticed at its next write. + * + * The twin of {@link treatStdinFailureAsShutdown} (`stdin-teardown.ts`). + * `stream` is injectable for tests; it defaults to `process.stdout`. + */ +export function treatStdoutFailureAsShutdown( + onTerminal: () => void, + stream: NodeJS.WritableStream = process.stdout +): void { + let fired = false; + const fire = (): void => { + if (fired) return; + fired = true; + try { + (stream as Partial<{ destroy(): void }>).destroy?.(); + } catch { + /* already gone */ + } + onTerminal(); + }; + stream.on('error', fire); + stream.on('close', fire); +} diff --git a/src/mcp/transport.ts b/src/mcp/transport.ts index 3a2458bb1..78c515215 100644 --- a/src/mcp/transport.ts +++ b/src/mcp/transport.ts @@ -16,6 +16,7 @@ import * as readline from 'readline'; import type { Socket } from 'net'; +import { treatStdoutFailureAsShutdown } from './stdout-teardown'; /** * JSON-RPC 2.0 Request @@ -312,6 +313,9 @@ export class StdioTransport extends LineBasedJsonRpcTransport { }; this.rl.on('close', onStreamEnd); process.stdin.on('error', onStreamEnd); + // A host that stops reading our output, with stdin still open, gets the + // same shutdown rather than the fatal handler's exit 1 (#2991). + treatStdoutFailureAsShutdown(onStreamEnd); } stop(): void {