mirror of
https://github.com/garrytan/gbrain.git
synced 2026-07-27 22:15:33 +00:00
fix(mcp): skip stdin EOF handlers when MCP_STDIO=1
OpenClaw's bundle-mcp gateway and similar wrappers pipe the JSON-RPC handshake on stdin then close their stdin half. Pre-fix, both stdin 'end' and 'close' listeners (server.ts:65-66 and serve.ts:204-206) treated this as a permanent disconnect and shut the server down before the first tool call arrived. Guard both sites with `process.env.MCP_STDIO !== '1'`. Signal handlers (SIGTERM/SIGINT/SIGHUP), transport.onclose, and the parent-process watchdog still cover legitimate shutdown paths. The serve.ts site threads the env read through an injectable `mcpStdio?: boolean` on ServeOptions so tests stay isolated (no process.env mutation per scripts/check-test-isolation.sh R1). Tests: 3 new cases in test/serve-stdio-lifecycle.test.ts pin the guard's invariants — mcpStdio=true must NOT trigger shutdown on stdin EOF, signals must still drive shutdown with mcpStdio=true, and mcpStdio=false (default) preserves existing CLI behavior. 25/25 pass. Origin: PR #870. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 4.7
parent
d71fcf6f65
commit
4f533c721b
+16
-1
@@ -56,6 +56,13 @@ export interface ServeOptions {
|
||||
// tick fell through to the cached `process.ppid` and the watchdog
|
||||
// never fired, while still claiming to be installed.
|
||||
probeWatchdog?: () => boolean;
|
||||
// v0.34.0 (#870): test seam for the MCP_STDIO=1 piped-stdin guard.
|
||||
// When true, runServe skips the stdin 'end'/'close' shutdown hooks
|
||||
// because the wrapping gateway (OpenClaw bundle-mcp, others) pipes the
|
||||
// JSON-RPC handshake and closes stdin immediately. Signal handlers and
|
||||
// transport.onclose still cover legitimate shutdown.
|
||||
// Defaults to `process.env.MCP_STDIO === '1'` when omitted.
|
||||
mcpStdio?: boolean;
|
||||
}
|
||||
|
||||
export async function runServe(
|
||||
@@ -201,7 +208,15 @@ function installStdioLifecycle(
|
||||
// Skip when stdin is a TTY: interactive `gbrain serve` use shouldn't
|
||||
// terminate just because the user hasn't typed anything. Signal /
|
||||
// watchdog paths still cover that case if needed.
|
||||
if (!deps.stdin.isTTY) {
|
||||
// v0.34.0 (#870): when MCP_STDIO=1, the wrapping gateway pipes the
|
||||
// JSON-RPC handshake then closes its stdin half. Treating that as a
|
||||
// permanent disconnect kills the server before the first tool call.
|
||||
// Signal handlers (SIGTERM/SIGINT/SIGHUP), transport.onclose, and the
|
||||
// parent-process watchdog below still cover legitimate shutdown paths.
|
||||
// `mcpStdio` is the injectable form; default reads the env once at
|
||||
// install time so tests stay isolated (no process.env mutation).
|
||||
const mcpStdioMode = opts.mcpStdio ?? (process.env.MCP_STDIO === '1');
|
||||
if (!deps.stdin.isTTY && !mcpStdioMode) {
|
||||
deps.stdin.once('end', () => beginShutdown('stdin-end'));
|
||||
deps.stdin.once('close', () => beginShutdown('stdin-close'));
|
||||
}
|
||||
|
||||
+9
-2
@@ -62,8 +62,15 @@ export async function startMcpServer(engine: BrainEngine) {
|
||||
.catch(() => {})
|
||||
.finally(() => process.exit(code));
|
||||
};
|
||||
process.stdin.on('end', () => shutdown('stdin end'));
|
||||
process.stdin.on('close', () => shutdown('stdin close'));
|
||||
// v0.34.0 (#870): when MCP_STDIO=1, the wrapping gateway (OpenClaw's
|
||||
// bundle-mcp layer, others) often pipes the JSON-RPC handshake then
|
||||
// closes its stdin half. Treating that as a permanent disconnect kills
|
||||
// the server before the first tool call arrives. Signal handlers and
|
||||
// transport.onclose still cover the legitimate shutdown paths.
|
||||
if (process.env.MCP_STDIO !== '1') {
|
||||
process.stdin.on('end', () => shutdown('stdin end'));
|
||||
process.stdin.on('close', () => shutdown('stdin close'));
|
||||
}
|
||||
// @ts-ignore — SDK exposes onclose on transport
|
||||
transport.onclose = () => shutdown('transport close');
|
||||
process.on('SIGTERM', () => shutdown('SIGTERM'));
|
||||
|
||||
@@ -81,6 +81,7 @@ function makeHarness(opts: {
|
||||
isTTY?: boolean;
|
||||
initialParentPid?: number;
|
||||
probeWatchdog?: boolean;
|
||||
mcpStdio?: boolean;
|
||||
} = {}): Harness {
|
||||
const engine = new StubEngine();
|
||||
const stdin = new EventEmitter() as EventEmitter & { isTTY?: boolean };
|
||||
@@ -120,6 +121,7 @@ function makeHarness(opts: {
|
||||
setInterval: timers.setInterval,
|
||||
clearInterval: timers.clearInterval,
|
||||
probeWatchdog: () => probeWatchdogResult,
|
||||
mcpStdio: opts.mcpStdio,
|
||||
};
|
||||
|
||||
return {
|
||||
@@ -439,4 +441,63 @@ describe('runServe stdio lifecycle', () => {
|
||||
expect(code).toBe(0);
|
||||
expect(h.logs.some(l => l.includes('cleanup error: synthetic disconnect failure'))).toBe(true);
|
||||
});
|
||||
|
||||
// v0.34.0 (#870): OpenClaw gateway / bundle-mcp wrappers pipe the
|
||||
// JSON-RPC handshake on stdin then close their stdin half. Without
|
||||
// MCP_STDIO=1 the server treats that as a permanent disconnect and
|
||||
// exits before handling tools/call. The guard skips the stdin 'end' /
|
||||
// 'close' hooks when MCP_STDIO=1; signals and parent watchdog still
|
||||
// cover legitimate shutdown.
|
||||
describe('MCP_STDIO=1 piped-stdin guard (#870)', () => {
|
||||
test('stdin end with mcpStdio=true does NOT trigger shutdown', async () => {
|
||||
const h = makeHarness({ mcpStdio: true });
|
||||
await startInBackground(h.engine, [], h.opts);
|
||||
|
||||
// Without the guard this would shutdown; with the guard it must not.
|
||||
h.stdin.emit('end');
|
||||
|
||||
// Give the event loop a microtask turn to catch any erroneous shutdown
|
||||
// path. We assert NO exit was registered.
|
||||
await new Promise<void>((r) => setTimeout(r, 10));
|
||||
expect(h.engine.disconnectCalls).toBe(0);
|
||||
|
||||
// Then trigger SIGTERM to drive the test to completion; signal handlers
|
||||
// remain active even with mcpStdio=true (codex would catch if they didn't).
|
||||
h.signals.emit('SIGTERM');
|
||||
const code = await h.exited;
|
||||
expect(code).toBe(0);
|
||||
expect(h.engine.disconnectCalls).toBe(1);
|
||||
expect(h.logs.some(l => l.includes('graceful exit (SIGTERM)'))).toBe(true);
|
||||
});
|
||||
|
||||
test('stdin close with mcpStdio=true does NOT trigger shutdown', async () => {
|
||||
const h = makeHarness({ mcpStdio: true });
|
||||
await startInBackground(h.engine, [], h.opts);
|
||||
|
||||
h.stdin.emit('close');
|
||||
|
||||
await new Promise<void>((r) => setTimeout(r, 10));
|
||||
expect(h.engine.disconnectCalls).toBe(0);
|
||||
|
||||
h.signals.emit('SIGINT');
|
||||
const code = await h.exited;
|
||||
expect(code).toBe(0);
|
||||
expect(h.engine.disconnectCalls).toBe(1);
|
||||
});
|
||||
|
||||
test('mcpStdio=false (default) preserves stdin EOF shutdown', async () => {
|
||||
// Regression guard: the guard must not over-trigger. With the env
|
||||
// unset, stdin EOF must still drive shutdown so existing CLI usage
|
||||
// (gbrain serve under launchd, claude-desktop's stdio MCP) is
|
||||
// unchanged.
|
||||
const h = makeHarness({ mcpStdio: false });
|
||||
await startInBackground(h.engine, [], h.opts);
|
||||
|
||||
h.stdin.emit('end');
|
||||
const code = await h.exited;
|
||||
expect(code).toBe(0);
|
||||
expect(h.engine.disconnectCalls).toBe(1);
|
||||
expect(h.logs.some(l => l.includes('graceful exit (stdin-end)'))).toBe(true);
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user