fix: #780 onerror and other listener not remove after client close#1322
fix: #780 onerror and other listener not remove after client close#1322shellRaining wants to merge 3 commits intomodelcontextprotocol:mainfrom
Conversation
🦋 Changeset detectedLatest commit: b67ce5e The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
@modelcontextprotocol/client
@modelcontextprotocol/server
@modelcontextprotocol/express
@modelcontextprotocol/fastify
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
|
@claude review |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
The fix is correct and well-structured, but since this touches a core transport implementation in a widely-used SDK, I'd recommend a human maintainer give it a quick look before merging.
Extended reasoning...
Overview
This PR fixes issue #780 by properly detaching stdout/stdin listeners when StdioClientTransport.close() is called. The implementation stores handler references as instance fields (_onServerDataHandler, _onServerErrorHandler) and adds a cleanupListeners() method that calls .off() on close. It also improves on('spawn') and on('close') to use once() since these are one-time events. The changeset and regression test are included.
Security risks
No security risks identified. The change only affects listener lifecycle management on stdio streams — no new inputs are accepted, no permissions or auth logic is affected.
Level of scrutiny
This warrants moderate scrutiny. While the fix follows a well-established Node.js pattern (storing handler refs for later .off()), it touches the stdio transport layer of the MCP client SDK, which is a critical communication path. The refactoring reorders listener setup and changes on to once for spawn/close events, which are correct but worth a human confirming. The PR has been open since December 2025 with no human reviews.
Other factors
The regression test structurally verifies the fix by asserting .off('data', handler) was called. The only bug found is a minor nit: the behavioral assertion (onerror not called) passes trivially due to ReadBuffer swallowing SyntaxError from non-JSON lines (commit 64897f7), rather than due to listener detachment. This doesn't affect the production code correctness — just means the test's behavioral assertion is weaker than it appears. The structural assertions adequately cover the fix.
…fter client close (stdio)
- Add test verifying stdout listeners are detached on close() so late non-JSON output from the child process no longer fires onerror - Add patch-level changeset
|
@claude review |
|
@claude review |
| this._process.once('close', _code => { | ||
| if (this._process) { | ||
| this.cleanupListeners(this._process); | ||
| } | ||
| this._process = undefined; | ||
| this.onclose?.(); | ||
| }); |
There was a problem hiding this comment.
🔴 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
- Caller invokes
const p = transport.start(). Inside the Promise executor,_onProcessErrorHandler = error => { reject(error); onerror?.(error); }is registered on the process. - Caller immediately invokes
transport.close(). Before anyawait,close()callscleanupListeners(this._process), which callsprocess.off("error", this._onProcessErrorHandler)— removing the onlyreject()pathway. close()setsthis._process = undefined.close()hitsawait Promise.race([closePromise, ...])and yields to the event loop.- The OS reports ENOENT: the process emits
"error"._onProcessErrorHandlerwas removed in step 2, so no listener fires.reject()is never called. - The process emits
"close". Theonce("close")handler instart()runs:if (this._process)isfalse(set in step 3), socleanupListenersis skipped. Onlythis.onclose?.()is called. Still noreject(). closePromiseresolves.close()returns.await phangs forever — thestart()Promise is permanently pending.
| async close(): Promise<void> { | ||
| if (this._process) { | ||
| this.cleanupListeners(this._process); | ||
| const processToClose = this._process; | ||
| this._process = undefined; | ||
|
|
There was a problem hiding this comment.
🔴 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
- Child process exits — its stdin read end closes, pipe is broken
- Caller invokes
transport.close() close()callscleanupListeners(this._process)—process.stdin?.off('error', ...)removes the only error listener from stdinprocessToClose.stdin?.end()is called — attempts to flush/close the write side of the broken pipe- OS returns EPIPE; Node.js queues an
errorevent on stdin's EventEmitter - Event loop delivers the
errorevent —stdin.listenerCount('error') === 0 - Node.js throws since
errorwas 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.
| 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); | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 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
- File header:
import process from 'node:process'—processrefers to Node global. cleanupListeners(process: ChildProcess)is declared — the parameterprocessshadows the import within this scope.- Inside the method,
process.stdout?.off(...)andprocess.off(...)all resolve to theChildProcessargument. - If a developer adds
process.env.SOME_VARinside this method expecting the Node global, TypeScript raises a type error (ChildProcess has noenv) — partial protection, but the naming confusion still exists. - If a developer adds
process.off('exit', someHandler)intending the globalprocess, it would compile successfully (both shareoff()) 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.
2 participants
Added logic to clean up residual listeners after calling transport.close().
This is my third attempt to submit a PR to the repository. I hope anybody can pay some attention to review it, I'd be truly grateful. and also point out any mistakes if there are any.
Motivation and Context
see #780
How Has This Been Tested?
After making this modification, residual listeners will not be triggered after closing the transport, and only "close" will be printed in the console.
and past all test suits
Breaking Changes
No breaking changes
Types of changes
Checklist
Additional context