Skip to content

fix: #780 onerror and other listener not remove after client close #1322

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
shellRaining wants to merge 3 commits into from
+107 −26
Closed

fix: #780 onerror and other listener not remove after client close #1322

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e3d2f38
fix: #780 onerror and other listener not remove after client close (s…
shellRaining Oct 24, 2025
064cd99
Add regression test and changeset
felixweinberger Mar 27, 2026
b67ce5e
Remove process error listener in cleanupListeners
felixweinberger Apr 2, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/stdio-cleanup-listeners-on-close.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@modelcontextprotocol/client': patch
---

Detach stdio transport stdout/stdin listeners on `close()`. Previously, if the child process wrote to stdout during shutdown (after `close()` was called), the still-attached data listener would attempt to parse it as JSON-RPC and emit a spurious parse error via `onerror`. Fixes #780.
56 changes: 36 additions & 20 deletions packages/client/src/client/stdio.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,9 @@ export class StdioClientTransport implements Transport {
private _readBuffer: ReadBuffer = new ReadBuffer();
private _serverParams: StdioServerParameters;
private _stderrStream: PassThrough | null = null;
private _onServerDataHandler?: (chunk: Buffer) => void;
private _onServerErrorHandler?: (error: Error) => void;
private _onProcessErrorHandler?: (error: Error) => void;

onclose?: () => void;
onerror?: (error: Error) => void;
Expand Down Expand Up @@ -130,33 +133,32 @@ export class StdioClientTransport implements Transport {
cwd: this._serverParams.cwd
});

this._process.on('error', error => {
reject(error);
this._onServerDataHandler = (chunk: Buffer) => {
this._readBuffer.append(chunk);
this.processReadBuffer();
};
this._onServerErrorHandler = (error: Error) => {
this.onerror?.(error);
});
};

this._process.on('spawn', () => {
resolve();
});
this._process.stdout?.on('data', this._onServerDataHandler);
this._process.stdout?.on('error', this._onServerErrorHandler);
this._process.stdin?.on('error', this._onServerErrorHandler);

this._process.on('close', _code => {
this._onProcessErrorHandler = error => {
reject(error);
this.onerror?.(error);
};
this._process.on('error', this._onProcessErrorHandler);
this._process.once('spawn', () => resolve());
this._process.once('close', _code => {
if (this._process) {
this.cleanupListeners(this._process);
}
this._process = undefined;
this.onclose?.();
});
Comment on lines +154 to 160
Copy link
Copy Markdown

@claude claude Bot Apr 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 The start() Promise can hang permanently if close() is called before the spawned process emits spawn and the process subsequently errors (e.g. ENOENT). This is a regression introduced by this PR: the new cleanupListeners() call in close() removes _onProcessErrorHandler — the only function that invokes reject() — before the process has a chance to emit error. The once("close") handler in start() only calls onclose?.() and never reject(), so the Promise leaks. Fix by holding a separate reject reference outside the cleanup lifecycle, or by checking whether the Promise has already settled before clearing the error handler.

Extended reasoning...

What the bug is and how it manifests

This PR introduces cleanupListeners() to fix spurious post-close onerror callbacks (issue #780). As part of that fix, _onProcessErrorHandler is stored as a named field so it can be removed via .off(). However, _onProcessErrorHandler is also the only function that calls reject() inside start()s Promise executor. When close() is called before the process emits spawn, it calls cleanupListeners() which removes _onProcessErrorHandler from the process. If the process then emits error (e.g. ENOENT because the command was not found), there is no longer any listener to call reject(), and the start() Promise is permanently pending.

The specific code path that triggers it

In stdio.ts at lines 154-160, the once("close", ...) handler only calls this.onclose?.() — it never calls resolve() or reject(). The only reject() call is inside _onProcessErrorHandler. So once that handler is removed by cleanupListeners(), the Promise has no settlement path for error cases.

Why existing code does not prevent it

The once("close") handler in start() does fire after the process exits, but it only calls this.onclose?.(). Even though the process closed (and therefore failed), reject() is never invoked. Additionally, the guard if (this._process) inside that handler is already false (because close() set this._process = undefined before yielding), so cleanupListeners is also skipped there — but that is irrelevant since _onProcessErrorHandler is already gone.

What the impact is

Any caller that does const p = transport.start(); await transport.close(); await p; with a command that errors during startup (ENOENT, permission denied, etc.) will hang indefinitely awaiting p. This is a realistic pattern for connection timeouts, cancellation during initialization, and test teardown. Before this PR, the anonymous error handler was never removed by close(), so reject() would always fire when the process errored — the behavior was correct even if imperfect.

How to fix it

Capture reject in a variable that lives outside the cleanup lifecycle. For example:

let startReject: ((e: Error) => void) | undefined;
return new Promise((resolve, reject) => {
    startReject = reject;
    // ... spawn ...
    this._onProcessErrorHandler = error => {
        startReject?.(error);  // call reject even if handler is detached
        this.onerror?.(error);
    };
    // In once("close"):
    this._process.once("close", _code => {
        startReject = undefined;  // settled
        ...
    });
});

Alternatively, track a started boolean and only remove _onProcessErrorHandler after spawn fires.

Step-by-step proof

  1. Caller invokes const p = transport.start(). Inside the Promise executor, _onProcessErrorHandler = error => { reject(error); onerror?.(error); } is registered on the process.
  2. Caller immediately invokes transport.close(). Before any await, close() calls cleanupListeners(this._process), which calls process.off("error", this._onProcessErrorHandler) — removing the only reject() pathway.
  3. close() sets this._process = undefined.
  4. close() hits await Promise.race([closePromise, ...]) and yields to the event loop.
  5. The OS reports ENOENT: the process emits "error". _onProcessErrorHandler was removed in step 2, so no listener fires. reject() is never called.
  6. The process emits "close". The once("close") handler in start() runs: if (this._process) is false (set in step 3), so cleanupListeners is skipped. Only this.onclose?.() is called. Still no reject().
  7. closePromise resolves. close() returns.
  8. await p hangs forever — the start() Promise is permanently pending.


this._process.stdin?.on('error', error => {
this.onerror?.(error);
});

this._process.stdout?.on('data', chunk => {
this._readBuffer.append(chunk);
this.processReadBuffer();
});

this._process.stdout?.on('error', error => {
this.onerror?.(error);
});

if (this._stderrStream && this._process.stderr) {
this._process.stderr.pipe(this._stderrStream);
}
Expand Down Expand Up @@ -202,8 +204,22 @@ export class StdioClientTransport implements Transport {
}
}

private cleanupListeners(process: ChildProcess) {
if (this._onServerDataHandler) {
process.stdout?.off('data', this._onServerDataHandler);
}
if (this._onServerErrorHandler) {
process.stdout?.off('error', this._onServerErrorHandler);
process.stdin?.off('error', this._onServerErrorHandler);
}
if (this._onProcessErrorHandler) {
process.off('error', this._onProcessErrorHandler);
}
}
Comment on lines +207 to +218
Copy link
Copy Markdown

@claude claude Bot Apr 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 The cleanupListeners(process: ChildProcess) parameter at line 207 shadows the module-level import process from 'node:process' at line 2. Inside the method, process.off("error", ...) reads as if it removes a Node.js global process error handler, when it actually operates on the ChildProcess argument — renaming to childProcess or proc eliminates the ambiguity.

Extended reasoning...

What the bug is

In packages/client/src/client/stdio.ts, the method signature private cleanupListeners(process: ChildProcess) at line 207 uses process as a parameter name. This shadows the top-level import process from 'node:process' at line 2. Every reference to process inside the method body therefore resolves to the ChildProcess argument, not Node's global process object.

The specific code path

Inside cleanupListeners, the call process.off('error', this._onProcessErrorHandler) at line 216 looks syntactically identical to removing a handler from Node's global process object — a common pattern for deregistering global uncaught-exception or unhandledRejection handlers. A reader unfamiliar with the local shadowing would reasonably misinterpret this line.

Why existing code doesn't prevent it

The current code is functionally correct: every use of process inside cleanupListeners legitimately targets the ChildProcess argument (.stdout?.off(...), .stdin?.off(...), .off(...)). TypeScript's type system provides some protection — accessing process.env or process.platform would produce a compile error since ChildProcess lacks those properties. However, ChildProcess extends EventEmitter and shares off(), on(), and emit() with the global process object, so TypeScript would not catch an accidental process.off(...) call intended for the global process.

Impact

This is a maintenance trap rather than an active bug. Any developer adding code inside cleanupListeners that intends to read process.env, call process.exit(), or check process.platform would silently receive the ChildProcess object instead, producing a runtime error or silent misbehavior. The naming also creates genuine cognitive overhead every time a reviewer reads process.off("error", ...) in this context.

Step-by-step proof of the shadowing

  1. File header: import process from 'node:process' — process refers to Node global.
  2. cleanupListeners(process: ChildProcess) is declared — the parameter process shadows the import within this scope.
  3. Inside the method, process.stdout?.off(...) and process.off(...) all resolve to the ChildProcess argument.
  4. If a developer adds process.env.SOME_VAR inside this method expecting the Node global, TypeScript raises a type error (ChildProcess has no env) — partial protection, but the naming confusion still exists.
  5. If a developer adds process.off('exit', someHandler) intending the global process, it would compile successfully (both share off()) and silently target the wrong object.

Fix

Rename the parameter to childProcess (or proc) throughout the method:

private cleanupListeners(childProcess: ChildProcess) {
    if (this._onServerDataHandler) {
        childProcess.stdout?.off('data', this._onServerDataHandler);
    }
    // ...
    if (this._onProcessErrorHandler) {
        childProcess.off('error', this._onProcessErrorHandler);
    }
}

This is a one-line rename with zero behavioral impact.


async close(): Promise<void> {
if (this._process) {
this.cleanupListeners(this._process);
const processToClose = this._process;
this._process = undefined;

Comment on lines 220 to 225
Copy link
Copy Markdown

@claude claude Bot Apr 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 In close(), cleanupListeners() removes the stdin error listener before processToClose.stdin?.end() is called, creating a window where async EPIPE errors from stdin have no handler. The try/catch around stdin.end() only catches synchronous throws — async error events emitted by the stream after the call returns are unhandled, causing Node.js to throw an uncaught exception. Fix: call processToClose.stdin?.end() before cleanupListeners(), or attach a temporary no-op error listener to stdin that outlives the end() call.

Extended reasoning...

What the bug is and how it manifests

In close(), the sequence is: (1) cleanupListeners(this._process) removes _onServerErrorHandler from stdin via process.stdin?.off('error', ...), (2) this._process = undefined, then (3) processToClose.stdin?.end() is called with no error listener on stdin.

If the child process has already exited (its read end of the pipe is closed), calling stdin.end() flushes any buffered data and closes the write side. The OS may respond with an EPIPE, which Node.js translates into an asynchronous error event emitted on the stdin Writable stream.

The specific code path that triggers it

Before the PR, the anonymous stdin error listener was registered and never removed, so EPIPE was routed (spuriously) to onerror. After the PR, cleanupListeners() correctly removes all stored listeners, but does so before stdin.end() is called. When the async EPIPE event fires on stdin, Node.js finds zero listeners and throws an uncaught exception per standard EventEmitter semantics.

Why existing safeguards do not prevent it

The try/catch block around processToClose.stdin?.end() only intercepts synchronous exceptions thrown during the end() call itself. EPIPE is an asynchronous OS event delivered after end() returns — the call stack is unwound, so the try/catch is no longer active. The process-level _onProcessErrorHandler lives on the ChildProcess EventEmitter (a different object from the stdin Writable stream) and provides no protection against errors emitted on stdin.

Impact

This is a regression introduced by the PR. Before the PR, EPIPE on stdin was silently routed to onerror (spurious but harmless). After the PR, EPIPE on stdin crashes the process with an unhandled error. This is common: any time a child MCP server exits before the client calls close(), its stdin pipe is broken, and the race between cleanupListeners and async EPIPE delivery is real and platform-dependent.

Step-by-step proof

  1. Child process exits — its stdin read end closes, pipe is broken
  2. Caller invokes transport.close()
  3. close() calls cleanupListeners(this._process) — process.stdin?.off('error', ...) removes the only error listener from stdin
  4. processToClose.stdin?.end() is called — attempts to flush/close the write side of the broken pipe
  5. OS returns EPIPE; Node.js queues an error event on stdin's EventEmitter
  6. Event loop delivers the error event — stdin.listenerCount('error') === 0
  7. Node.js throws since error was emitted with no listener — uncaught exception, process crash

How to fix it

Reorder: call processToClose.stdin?.end() before cleanupListeners(), so the error listener is still active during and after the end() call. Alternatively, attach a temporary no-op error handler to stdin before end() and remove it once the stream closes.

Expand Down
72 changes: 66 additions & 6 deletions packages/client/test/client/crossSpawn.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
import type { ChildProcess } from 'node:child_process';
import { EventEmitter } from 'node:events';

import type { JSONRPCMessage } from '@modelcontextprotocol/core';
import spawn from 'cross-spawn';
import type { Mock, MockedFunction } from 'vitest';
import { vi } from 'vitest';

import { getDefaultEnvironment, StdioClientTransport } from '../../src/client/stdio.js';

Expand All @@ -16,8 +18,9 @@ describe('StdioClientTransport using cross-spawn', () => {
mockSpawn.mockImplementation(() => {
const mockProcess: {
on: Mock;
stdin?: { on: Mock; write: Mock };
stdout?: { on: Mock };
once: Mock;
stdin?: { on: Mock; write: Mock; off: Mock };
stdout?: { on: Mock; off: Mock };
stderr?: null;
} = {
on: vi.fn((event: string, callback: () => void) => {
Expand All @@ -26,12 +29,20 @@ describe('StdioClientTransport using cross-spawn', () => {
}
return mockProcess;
}),
once: vi.fn((event: string, callback: () => void) => {
if (event === 'spawn') {
callback();
}
return mockProcess;
}),
stdin: {
on: vi.fn(),
write: vi.fn().mockReturnValue(true)
write: vi.fn().mockReturnValue(true),
off: vi.fn()
},
stdout: {
on: vi.fn()
on: vi.fn(),
off: vi.fn()
},
stderr: null
};
Expand Down Expand Up @@ -109,13 +120,16 @@ describe('StdioClientTransport using cross-spawn', () => {
// get the mock process object
const mockProcess: {
on: Mock;
once: Mock;
stdin: {
on: Mock;
write: Mock;
once: Mock;
off: Mock;
};
stdout: {
on: Mock;
off: Mock;
};
stderr: null;
} = {
Expand All @@ -125,13 +139,21 @@ describe('StdioClientTransport using cross-spawn', () => {
}
return mockProcess;
}),
once: vi.fn((event: string, callback: () => void) => {
if (event === 'spawn') {
callback();
}
return mockProcess;
}),
stdin: {
on: vi.fn(),
write: vi.fn().mockReturnValue(true),
once: vi.fn()
once: vi.fn(),
off: vi.fn()
},
stdout: {
on: vi.fn()
on: vi.fn(),
off: vi.fn()
},
stderr: null
};
Expand All @@ -153,6 +175,44 @@ describe('StdioClientTransport using cross-spawn', () => {
expect(mockProcess.stdin.write).toHaveBeenCalled();
});

// Regression test for #780: listeners must be detached from the child's
// stdio streams on close(), not left attached to a process we no longer track.
test('should detach stdout/stdin listeners on close', async () => {
const stdout = new EventEmitter();
const stdin = Object.assign(new EventEmitter(), {
write: vi.fn().mockReturnValue(true),
end: vi.fn()
});
const proc = Object.assign(new EventEmitter(), {
stdin,
stdout,
stderr: null,
exitCode: null as number | null,
kill: vi.fn()
});
mockSpawn.mockReturnValue(proc as unknown as ChildProcess);

const transport = new StdioClientTransport({ command: 'test-command' });
const started = transport.start();
proc.emit('spawn');
await started;

expect(stdout.listenerCount('data')).toBe(1);
expect(stdout.listenerCount('error')).toBe(1);
expect(stdin.listenerCount('error')).toBe(1);
expect(proc.listenerCount('error')).toBe(1);

const closed = transport.close();
proc.exitCode = 0;
proc.emit('close');
await closed;

expect(stdout.listenerCount('data')).toBe(0);
expect(stdout.listenerCount('error')).toBe(0);
expect(stdin.listenerCount('error')).toBe(0);
expect(proc.listenerCount('error')).toBe(0);
});

describe('windowsHide', () => {
Comment thread
claude[bot] marked this conversation as resolved.
const originalPlatform = process.platform;

Expand Down
Loading