Repository navigation
Conversation
🦋 Changeset detectedLatest commit: 1b1d71f The changes in this PR will be included in the next version bump. This PR includes changesets to release 6 packages
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/codemod
@modelcontextprotocol/core
@modelcontextprotocol/server
@modelcontextprotocol/server-legacy
@modelcontextprotocol/express
@modelcontextprotocol/fastify
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
|
Two things from reading the diff (not run here, the Node side is from how
|
| // ignore | ||
| } | ||
| processToClose.stdin?.removeAllListeners(); | ||
| processToClose.stdout?.removeAllListeners(); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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).
|
Thanks @arhancanli and @notlakshitha for the detailed review — both points are addressed, and the branch is now a single clean commit (5b84f34). 1. The 2. 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. |
462f6df to
5b84f34
Compare
|
Read the new commit; both points are covered, and the no-op One edge in the shared drain wait: |
|
Good catch @arhancanli — to answer the lifecycle question directly:
So the orphan That said, to remove any doubt and make the drain lifecycle fully deterministic, I now store the shared drain handler so that Updated commit: 4caeb32. |
5b84f34 to
4caeb32
Compare
|
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 One thing in the same function: |
4caeb32 to
4d09680
Compare
|
Good point @arhancanli. We went with resolving, and I've documented it on Reasoning: even in the happy path, The Updated commit: 4d09680. |
|
That reasoning holds: One ordering detail in the diff: in the |
4d09680 to
2fc0c61
Compare
|
Added the test you described: a single back-pressured Updated commit: 2fc0c61. |
|
Read 2fc0c61 (not run). The new test pins what I asked for: a single back-pressured One edge the test does not reach: a |
…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.
2fc0c61 to
1b1d71f
Compare
|
Read — thanks. To answer the two questions directly: A second (and Nth) No To pin the reliably-reproducible part of this, I added a test: a back-pressured Updated commit: 1b1d71f. |
|
Thanks, the Nth-waiter answer is clear: draining the whole On the second question I would not rely on the check as described. A child started with If it holds, a |
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
send()accumulateddrainlisteners onstdin.Every
send()whosewrite()hit a full buffer added its ownstdin.once('drain', resolve). Under sustained back-pressure (a server that reads slowly, or not at all) these pile up without bound and trip the defaultMaxListenerslimit of 10.close()left thestart()listeners attached and never ended thestderrpass-through.stdout.on('data', …)listener kept the stream flowing, so late non-JSON output afterclose()still hitprocessReadBuffer()→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 adrainlistener to the destination; if the caller pipes the transport'sstderrinto a shared stream (e.g. a host log, as opencode does), those listeners persist until the source ends. When a grandchild keeps the child'sstderrwrite-end open, the natural end cascade never fires, so they leak.Changes
packages/client/src/client/stdio.tssend()now shares a singledrainwait instead of adding one listener per back-pressured write, sostdinnever accumulatesdrainlisteners; a successful write returns early instead of leaving a stale resolver in the wait array.settlePendingSends()resolves anysend()promises still waiting on adrainthe process will no longer emit and removes the shareddrainlistener, so nothing lingers on a dead stream. It is called from the processclosehandler,close(), and_dispose(), so in-flight sends can never hang. They resolve rather than reject: the bytes were already handed tostdin.write(), so delivery past process exit is out of scope —oncloseis the authoritative "the connection is gone" signal.close()detaches only the specificdata/errorhandlers installed instart()— notremoveAllListeners(), so Node's owncloselisteners on stdout/stderr are preserved and the processclose/onclosestill fires — and installs no-operrorhandlers so a late EPIPE (a write racing the child's exit) cannot throw uncaught. Thestderrpass-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/drainlisteners — once the child is gone.packages/client/test/client/stdio.test.tsFocused, deterministic regression tests (the first two each verified to fail on the unfixed code):
send()under back-pressure keepsstdindrainlisteners ≤ 1 and emits noMaxListenersExceededWarning, then settles all back-pressured sends onclose().close()removes thedrainlistener thestderrpipe attached to the pass-through (the pipe's destination), even when a grandchild keeps the child'sstderropen sopipe()never auto-cleans up. Verified to fail with theunpiperemoved.close()removes the shareddrainlistener fromstdinwhen the child never drains (the defensiveremoveListenerinsettlePendingSends()).send()still pending when the child dies resolves (not hangs) andonclosefires exactly once — pinned for both the explicitclose()path and the child-exits-on-its-own path (settled via theclosehandler).Testing
stdinaccumulated 27drainlisteners and fired the warning; after the fix it stays at 1 with no warning, and all sends settle onclose().test/e2e):transport:stdio:clean-shutdownpasses, andtransport:stdio:shutdown-escalationpasses 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.stdio.test.tsunit tests pass;typecheckandeslintare clean.Related issues
datalistener not removed after client close)MaxListenersExceededWarningondrainlisteners)