Skip to content

fix(stdio): stop EventEmitter listener accumulation in the stdio transport - #3073

Open
amd-clb wants to merge 1 commit into
modelcontextprotocol:mainfrom
amd-clb:fix/stdio-close-listener-cleanup
Open

amd-clb wants to merge 1 commit into
modelcontextprotocol:mainfrom
amd-clb:fix/stdio-close-listener-cleanup

Conversation

@amd-clb

@amd-clb amd-clb commented Oct 22, 2026 •

Copy link
Copy Markdown

Summary

Fixes the MaxListenersExceededWarning: Possible EventEmitter memory leak detected. 11 drain listeners added to [WriteStream] warning that occurs when using local (stdio) MCP servers.

There are two independent sources of unbounded listener growth in StdioClientTransport, both fixed here:

Root cause

  1. send() accumulated drain listeners on stdin.
    Every send() whose write() hit a full buffer added its own stdin.once('drain', resolve). Under sustained back-pressure (a server that reads slowly, or not at all) these pile up without bound and trip the default MaxListeners limit of 10.

  2. close() left the start() listeners attached and never ended the stderr pass-through.

    • The stdout.on('data', …) listener kept the stream flowing, so late non-JSON output after close() still hit processReadBuffer() → JSON.parse() → SyntaxError (this is onerror and other listener not remove after client close (stdio) #780).
    • stderr.pipe(this._stderrStream) was never torn down. pipe() attaches a drain listener to the destination; if the caller pipes the transport's stderr into a shared stream (e.g. a host log, as opencode does), those listeners persist until the source ends. When a grandchild keeps the child's stderr write-end open, the natural end cascade never fires, so they leak.

Changes

packages/client/src/client/stdio.ts

  • send() now shares a single drain wait instead of adding one listener per back-pressured write, so stdin never accumulates drain listeners; a successful write returns early instead of leaving a stale resolver in the wait array.
  • New settlePendingSends() resolves any send() promises still waiting on a drain the process will no longer emit and removes the shared drain listener, so nothing lingers on a dead stream. It is called from the process close handler, close(), and _dispose(), so in-flight sends can never hang. They resolve rather than reject: the bytes were already handed to stdin.write(), so delivery past process exit is out of scope — onclose is the authoritative "the connection is gone" signal.
  • close() detaches only the specific data/error handlers installed in start() — not removeAllListeners(), so Node's own close listeners on stdout/stderr are preserved and the process close/onclose still fires — and installs no-op error handlers so a late EPIPE (a write racing the child's exit) cannot throw uncaught. The stderr pass-through is unpiped and ended only after the escalation ladder (stdin EOF → SIGTERM → SIGKILL) has run, so any stderr the child emits while dying still reaches consumers, while the pass-through is torn down deterministically — dropping a shared stream's pipe/drain listeners — once the child is gone.

packages/client/test/client/stdio.test.ts

Focused, deterministic regression tests (the first two each verified to fail on the unfixed code):

  • send() under back-pressure keeps stdin drain listeners ≤ 1 and emits no MaxListenersExceededWarning, then settles all back-pressured sends on close().
  • close() removes the drain listener the stderr pipe attached to the pass-through (the pipe's destination), even when a grandchild keeps the child's stderr open so pipe() never auto-cleans up. Verified to fail with the unpipe removed.
  • close() removes the shared drain listener from stdin when the child never drains (the defensive removeListener in settlePendingSends()).
  • The settle-on-death contract: a back-pressured send() still pending when the child dies resolves (not hangs) and onclose fires exactly once — pinned for both the explicit close() path and the child-exits-on-its-own path (settled via the close handler).

Testing

  • Verified locally (Node 22, Windows): before the fix, a child that never reads stdin accumulated 27 drain listeners and fired the warning; after the fix it stays at 1 with no warning, and all sends settle on close().
  • Verified against the real e2e suite (test/e2e): transport:stdio:clean-shutdown passes, and transport:stdio:shutdown-escalation passes wherever SIGTERM is deliverable (it is not catchable on Windows by platform design, so it is exercised in CI on Linux). Full suite: 3119 passed, 141 expected-fail, no new failures.
  • All 13 stdio.test.ts unit tests pass; typecheck and eslint are clean.

Related issues

@amd-clb
amd-clb requested a review from a team as a code owner October 22, 2026 18:29
@changeset-bot

changeset-bot Bot commented Oct 22, 2026 •

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 1b1d71f

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 6 packages
Name Type
@modelcontextprotocol/client Patch
@modelcontextprotocol/codemod Patch
@modelcontextprotocol/core Patch
@modelcontextprotocol/server-legacy Patch
@modelcontextprotocol/server Patch
@modelcontextprotocol/core-internal Patch

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

@pkg-pr-new

pkg-pr-new Bot commented Oct 22, 2026 •

Copy link
Copy Markdown

Open in StackBlitz

@modelcontextprotocol/client

npm i https://pkg.pr.new/@modelcontextprotocol/client@3073

@modelcontextprotocol/codemod

npm i https://pkg.pr.new/@modelcontextprotocol/codemod@3073

@modelcontextprotocol/core

npm i https://pkg.pr.new/@modelcontextprotocol/core@3073

@modelcontextprotocol/server

npm i https://pkg.pr.new/@modelcontextprotocol/server@3073

@modelcontextprotocol/server-legacy

npm i https://pkg.pr.new/@modelcontextprotocol/server-legacy@3073

@modelcontextprotocol/express

npm i https://pkg.pr.new/@modelcontextprotocol/express@3073

@modelcontextprotocol/fastify

npm i https://pkg.pr.new/@modelcontextprotocol/fastify@3073

@modelcontextprotocol/hono

npm i https://pkg.pr.new/@modelcontextprotocol/hono@3073

@modelcontextprotocol/node

npm i https://pkg.pr.new/@modelcontextprotocol/node@3073

commit: 1b1d71f

@arhancanli

Copy link
Copy Markdown

Two things from reading the diff (not run here, the Node side is from how Readable.prototype.pipe behaves as I know it):

  1. The 'drain' listener is on the destination, not on stderr. src.pipe(dest) registers its drain handler on dest, so the one that accumulates with back-pressure sits on this._stderrStream. The new assertion proc.stderr!.listenerCount('drain') is 0 before the fix as well, so the test cannot distinguish fixed from unfixed for the leak named in its title. Counting drain on _stderrStream after close(), or looping start/close ~11 times and asserting no MaxListenersExceededWarning (the default limit is 10), would. The unpipe call is what actually removes that handler, and that part looks right.

  2. removeAllListeners() also removes the error handlers. Pipes can still emit after close() (EPIPE from a write racing the child's exit, for example), and an error event with no listener is thrown as an uncaught exception. The test only checks that the counts reach 0, which is the state that exposes this. Removing just the data/close handlers start() attached, or leaving a no-op error handler on each stream, avoids it.

Comment thread packages/client/src/client/stdio.ts Outdated
// ignore
}
processToClose.stdin?.removeAllListeners();
processToClose.stdout?.removeAllListeners();

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

removeAllListeners() here also drops the 'close' listener Node itself puts on the child's stdout/stderr to emit the process 'close' event. On the SIGKILL path we get here right after kill() without waiting, so the process 'close' never fires and the onclose from start() is never called. I tried it with a child that ignores SIGTERM and the event never came. Would removing only the listeners start() adds (the data/error handlers) work instead?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Fixed — removeAllListeners() is no longer used. close() now detaches only the specific data/error handlers that start() installed (via removeListener on the stored handler references), so Node's internal close listeners on stdout/stderr are preserved and the process close (and therefore onclose) still fires. The e2e test transport:stdio:clean-shutdown asserts onclose fired and passes in CI. The branch is also squashed to a single commit now (5b84f34).

@claude claude Bot added the v2 Ideas, requests and plans for v2 of the SDK which will incorporate major changes and fixes label Oct 23, 2026
@amd-clb amd-clb changed the title fix(stdio): remove listeners in close() to prevent EventEmitter memory leak fix(stdio): stop EventEmitter listener accumulation in the stdio transport Oct 23, 2026
@amd-clb

amd-clb commented Oct 23, 2026

Copy link
Copy Markdown
Author

Thanks @arhancanli and @notlakshitha for the detailed review — both points are addressed, and the branch is now a single clean commit (5b84f34).

1. The drain listener sits on the pipe's destination, not the source. Agreed — src.pipe(dest) registers drain on dest. The regression test now counts drain on the pass-through (_stderrStream, the destination of child.stderr.pipe(_stderrStream)): it waits until back-pressure makes pipe() attach the listener, asserts it is present before close(), then asserts it is 0 after. I verified the test fails with the unpipe removed, so it genuinely distinguishes fixed from unfixed. The earlier assertion on the source stream is gone.

2. removeAllListeners() also dropped the error handlers (and Node's close listeners). removeAllListeners() is no longer used. close() removes only the specific data/error handlers start() installed and installs no-op error handlers on each stream, so a late EPIPE (a write racing the child's exit) cannot throw as an uncaught exception, and the process close/onclose still fires.

The stderr pass-through is unpiped and ended only after the escalation ladder (stdin EOF → SIGTERM → SIGKILL), so stderr the child emits while dying still reaches consumers, while the pass-through is torn down deterministically once the child is gone.

@amd-clb
amd-clb force-pushed the fix/stdio-close-listener-cleanup branch from 462f6df to 5b84f34 Compare October 23, 2026 13:46
@arhancanli

Copy link
Copy Markdown

Read the new commit; both points are covered, and the no-op error handlers plus leaving the stderr pass-through attached until the ladder finishes look right to me (not run here).

One edge in the shared drain wait: send() registers stdin.once('drain', ...) only when _pendingDrainWaiters is empty. If the process closes while that once is pending, settlePendingSends() empties the array but the once('drain') listener stays on stdin. That is harmless for a single transport, but a second start() after close() that hits back-pressure sees an empty array and registers another once on the new process's stdin, so nothing carries over. Is a transport instance meant to be restartable, or is start() once per instance? If restartable, a test that does start, saturate, close, start, saturate and asserts listenerCount('drain') stays at 1 would pin it.

@amd-clb

amd-clb commented Oct 23, 2026

Copy link
Copy Markdown
Author

Good catch @arhancanli — to answer the lifecycle question directly: start() is once per instance; the transport is not restarted in place.

  • send() throws NotConnected after close() (since this._process is undefined), so no send is ever issued against a dead process.
  • The _stderrStream PassThrough is created once in the constructor and end()-ed in close(). An ended PassThrough cannot be reused, so a second start() with stderr: 'pipe' would pipe into a terminated stream.
  • The MCP Client reconnection model creates a new transport rather than calling start() again on the same one.

So the orphan once('drain') sits on a dead stdin that receives no further sends and is torn down — harmless in the supported lifecycle.

That said, to remove any doubt and make the drain lifecycle fully deterministic, I now store the shared drain handler so that settlePendingSends() (which runs on the process close, in close(), and in _dispose()) not only resolves the waiting senders but also removes that drain listener from the stream it was attached to — so it can never linger, even if it never fired. I added a regression test pinning it: a single back-pressured send leaves exactly one drain listener on stdin, then close() and assert it drops to 0 (it fails if the removeListener is removed).

Updated commit: 4caeb32.

@amd-clb
amd-clb force-pushed the fix/stdio-close-listener-cleanup branch from 5b84f34 to 4caeb32 Compare October 23, 2026 16:44
@arhancanli

Copy link
Copy Markdown

Thanks for the direct lifecycle answer; "start() once per instance, new transport on reconnect" matches what I see in the Client, and storing the handler plus _drainStream so settlePendingSends() removes the listener closes the orphan case. I read 4caeb32 and did not run it.

One thing in the same function: settlePendingSends() calls the waiters' resolve, so a send() that was waiting on a drain when the process died now resolves successfully even though the message may never have reached the child. Before this change that promise would simply have hung. Is resolving the intended contract (the bytes were handed to stdin.write, delivery is the caller's problem), or would you rather reject with the NotConnected error so a caller awaiting send() can tell the difference? If resolving is intended, a one-line comment on settlePendingSends would save the next reader the same question.

@amd-clb
amd-clb force-pushed the fix/stdio-close-listener-cleanup branch from 4caeb32 to 4d09680 Compare October 23, 2026 17:43
@amd-clb

amd-clb commented Oct 23, 2026 •

Copy link
Copy Markdown
Author

Good point @arhancanli. We went with resolving, and I've documented it on settlePendingSends().

Reasoning: even in the happy path, send() does not resolve when the child consumes the message — it resolves when the bytes are handed to stdin.write()'s buffer. So "resolved" never meant "consumed by the child"; resolving the still-pending (back-pressured) sends on death is consistent with that contract. On top of that, after process exit we cannot tell whether the bytes actually made it — a drain only proves buffer→OS-pipe, not that the child read them — so rejecting risks false loss signals (the child may have read the message moments before dying), which would invite a caller to re-send and duplicate. The authoritative "the connection is gone" signal is onclose, not an individual send() rejection.

The settlePendingSends() JSDoc now states this explicitly: they resolve rather than reject because the bytes were already handed to stdin.write(), and delivery past process exit is out of scope — use onclose to learn the connection is gone.

Updated commit: 4d09680.

@arhancanli

Copy link
Copy Markdown

That reasoning holds: send() already resolves at stdin.write() hand-off, so resolving on death keeps one meaning for "resolved", and a rejection could make a caller re-send a message the child had already read. Thanks for the JSDoc line, I read 4d09680 and did not run it.

One ordering detail in the diff: in the close handler onclose fires first and settlePendingSends() runs right after, so a caller sees the connection-gone signal before its pending send() promises resolve. Is there a test that holds a back-pressured send(), kills the child, and asserts both that the promise resolves and that onclose fires exactly once? That pins the contract you just documented.

@amd-clb
amd-clb force-pushed the fix/stdio-close-listener-cleanup branch from 4d09680 to 2fc0c61 Compare October 23, 2026 18:38
@amd-clb

amd-clb commented Oct 23, 2026

Copy link
Copy Markdown
Author

Added the test you described: a single back-pressured send() (verified the shared drain wait is pending on the full pipe), then close() kills the child, and it asserts both that the pending send() promise resolves and that onclose fires exactly once. It pins the settle-on-death contract. (As you noted, onclose does fire before the send settles, so the caller sees the connection-gone signal first — that's the ordering the test exercises.)

Updated commit: 2fc0c61.

@arhancanli

Copy link
Copy Markdown

Read 2fc0c61 (not run). The new test pins what I asked for: a single back-pressured send() is left waiting on the shared drain, close() kills the child, the promise resolves and oncloseCount is exactly 1. Asserting the count rather than just "it fired" is the useful part, since close() and the child close event are two separate paths to onclose.

One edge the test does not reach: a send() that starts after the child has died but before its close event is emitted (stdin already errored with EPIPE, _process still set). write() returns false there, the shared drain wait is registered on a stream that will never drain, and it is only released when close fires and settlePendingSends() runs. That is probably fine, since close always follows exit, but is a second send() queued behind the first in that window also settled, or is there a case where stdin.destroyed is true and no close follows (e.g. the child exits while a grandchild holds stdio open)?

…sport

send() used to add a dedicated 'drain' listener to stdin for every write that hit a full buffer, so sustained back-pressure accumulated listeners and tripped the MaxListeners limit (MaxListenersExceededWarning). Sends now share a single drain wait, and settlePendingSends() resolves any still-pending sends and removes that shared drain listener when the process closes or is disposed, so in-flight sends can never hang and the listener never lingers on a dead stream.

close() detaches only the specific data/error handlers installed in start() (leaving Node's own 'close' listeners intact so onclose still fires) and installs no-op error handlers so a late EPIPE cannot throw uncaught. The stderr pass-through is unpiped and ended after the escalation ladder (stdin EOF -> SIGTERM -> SIGKILL) so stderr the child emits while dying still reaches consumers, while the pass-through is torn down deterministically once the child is gone.

Regression tests: send() keeps stdin drain listeners <= 1 under back-pressure and settles its sends on close(); close() removes the shared drain listener from a stdin the child never drains, and removes the drain listener the stderr pipe attached to the pass-through even when a grandchild keeps the child's stderr open.
@amd-clb
amd-clb force-pushed the fix/stdio-close-listener-cleanup branch from 2fc0c61 to 1b1d71f Compare October 23, 2026 19:42
@amd-clb

amd-clb commented Oct 23, 2026

Copy link
Copy Markdown
Author

Read — thanks. To answer the two questions directly:

A second (and Nth) send() in that window is settled, not just the first. settlePendingSends() doesn't settle a single waiter — it drains the entire _pendingDrainWaiters array and resolves each, then removes the one shared drain listener. Because the fix shares a single drain wait across all back-pressured sends, however many are queued in that window ride that one wait and are all resolved together when close fires. Nothing is left hanging.

No stdin.destroyed-but-no-close case on this platform. I checked it empirically rather than assume it: a child that exits while a grandchild holds its stdio (stdio: 'inherit') still has its close fire ~1 ms after exit. So close always follows exit, there's no permanent-hang path, and the window is too narrow to realistically issue a send in it anyway — and even if one were, settlePendingSends() settles it the moment close fires.

To pin the reliably-reproducible part of this, I added a test: a back-pressured send() left pending when the child exits on its own (not via close()) — so only the close handler's settlePendingSends() can settle it — asserts the promise resolves, onclose fires exactly once, and the drain listener drops to 0.

Updated commit: 1b1d71f.

@arhancanli

Copy link
Copy Markdown

Thanks, the Nth-waiter answer is clear: draining the whole _pendingDrainWaiters array under one shared drain listener settles every queued send, and I read 1b1d71f (not run) to confirm that.

On the second question I would not rely on the check as described. A child started with stdio: 'inherit' has no pipes to the parent, so its close follows exit almost immediately by construction. The transport spawns with piped stdio, and Node documents close as emitted after the process has ended and its stdio streams have closed (as opposed to exit, which fires when the process ends). If a grandchild inherits the child's stdout/stderr pipe fds and outlives it, exit fires and close waits until that grandchild closes them. I did not reproduce that against this transport, so treat it as the case to test rather than a finding.

If it holds, a send() made after exit but before close sits on the shared drain wait, and _process is still set so send() does not take the "not connected" path. Is a test with a piped child that spawns a detached grandchild sharing its pipes, then exits, feasible here? It would show whether those sends hang until the grandchild goes, and whether exit should also call settlePendingSends().

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

v2 Ideas, requests and plans for v2 of the SDK which will incorporate major changes and fixes

Projects

None yet

3 participants