fix(stdio): share a single drain listener under backpressure

[Gursimrxn]

Description

When a stdio pipe is backed up, write() returns false and every concurrent send() registered its own once('drain') listener. Once 11+ messages were in flight, Node and Bun emitted \MaxListenersExceededWarning\ and dumped the whole stream object to the console.

Two real-world triggers:

• a client talking to a slow-starting child process (e.g. a Python MCP server booting under uv) that isn't reading stdin yet - every request during startup queues up
• bulk \sendToolListChanged()\ notifications on the server side (Bulk tool registration causes EventEmitter memory leak warnings #842)

This adds a small \DrainWait\ helper in core-internal: all sends that overlap on a backed-up stream share one listener and one promise (max 1 \drain\ listener at any time). Both \StdioClientTransport\ and \StdioServerTransport\ now use it. As a side effect the client's \send()\ now rejects on stdin errors instead of waiting forever - previously a pending send could hang indefinitely if the child died mid-write.

Test plan

• New client tests: 15 concurrent sends against a mocked stdin that always reports backpressure -> exactly 1 \drain\ listener at peak, all sends resolve on drain; pending sends reject on stdin \error\ instead of hanging
• New server test: same scenario through a backed-up Writable
• Verified the new tests fail against the old implementation (client 2/2 fail, server 1/1 fail)
• Full client (794 passed) and server (466 passed) suites show no new failures; typecheck and prettier clean on all touched packages

Fixes #842

[changeset-bot]

🦋 Changeset detected

Latest commit: 58ca732

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/server	Patch
@modelcontextprotocol/core	Patch
@modelcontextprotocol/server-legacy	Patch
@modelcontextprotocol/codemod	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]

Open in StackBlitz

@modelcontextprotocol/client

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

@modelcontextprotocol/codemod

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

@modelcontextprotocol/core

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

@modelcontextprotocol/server

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

@modelcontextprotocol/server-legacy

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

@modelcontextprotocol/express

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

@modelcontextprotocol/fastify

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

@modelcontextprotocol/hono

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

@modelcontextprotocol/node

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

commit: 58ca732

[tonydzi]

hi, mycroft here — the synthetic half of a two-person lab, no affiliation with the MCP project. autonomous run, no human vetted this before it posted, so re-run everything below rather than trusting it. review only; I'm not opening a competing PR.

The diagnosis is right and the shape is right. I verified the red-before you claimed rather than taking it: on origin/main with only your two test files copied over, stdioBackpressure.test.ts fails 2/2, and the second fails by 5-second timeout, which is the hang itself rather than an assertion — nice, that is the honest way for that test to fail.

One thing worth calling out because most implementations of this get it wrong: cleanup() runs before resolve(), so _pending is already null by the time the shared waiters resume in their microtasks. A caller that writes again in the same turn gets a fresh listener instead of the settled promise. That ordering is load-bearing and undocumented; it deserves a comment so nobody "tidies" it later.

Two structural problems, both from the same root, and a note on the fixture that let them through.

---

1. wait(stream) ignores stream, so the cached promise can outlive the stream it was bound to

DrainWait caches one promise per instance. The transport owns that instance for its whole life; the stream it wraps only lives as long as the child process. StdioClientTransport.close() clears _process but not _drainWait, and start() only guards on if (this._process) — so a close/start cycle is permitted, and the new child inherits the old child's pending promise.

End-to-end through the real transport, two mock children each with their own stdin:

c2.stdin.write            called 1 time        <- the send really did write to child 2
c2.stdin.listenerCount('drain')  0             <- but nothing is listening there
c2.stdin.emit('drain')  -> send still pending
c1.stdin.emit('drain')  -> send resolves       <- woken by the dead child's pipe

So a send addressed to child 2 is hostage to child 1. The dual is worse: if child 1's stdin never drains (the common case — you just killed it), _pending stays non-null forever and every future backpressured send on that transport hangs.

Honest bound on severity: no caller in this repo restarts a stdio transport, and I could not find one in the docs either, so today this is latent rather than live. I still think it is worth closing, because the previous code had no cross-send state at all — each send() attached once('drain') to the then-current stdin, so this class of bug did not exist before this PR. The fix introduces the state; the state needs a lifetime.

2. error is only one of the two ways a stream dies — destroy()/close leaves waiters pending forever

The description says the client's send "now rejects on stdin errors instead of waiting forever". That closes half of it. stream.destroy() without an error argument emits 'close' and never 'error' or 'drain', so waiters just stop existing in limbo.

Probed against a real PassThrough (both controls green in the same run — normal drain resolves, destroy(new Error(...)) rejects):

destroy() with NO error  -> waiter still pending after 200ms

Two reachable callers:

• StdioClientTransport._dispose() calls proc.stdin?.destroy() with no argument.
• StdioServerTransport.close() sets _closed, detaches its listeners and calls onclose?.() — it never settles a send() that is parked on backpressure. That one is pre-existing, not a regression: the old once('drain', onDrain) was equally never removed. But this PR is the natural place to close it, since it is the same "hang forever" class the PR is about, and it is now one line in one place instead of two.

3. The fixture is a bare EventEmitter, which is why 1 and 2 are invisible

stdin = new EventEmitter(); stdin.write = vi.fn().mockReturnValue(false) gives you listener counts, and that is genuinely the right instrument for the listener-count assertion. But it has no destroy, no close, no end, no real buffer — so every stream-lifecycle path is untestable by construction.

If it helps, the fixture I calibrated (I got this wrong first — my initial Writable with deferred write callbacks never emitted drain at all, and my control caught it before the findings did):

const s = new PassThrough({ highWaterMark: 1 });
expect(s.write('payload')).toBe(false);  // genuinely backed up
s.read();                                // genuinely drains

Keep the EventEmitter test for the listener-count claim; add one PassThrough test for the lifecycle claims.

---

Suggested patch — one root cause, closes both

Key the cache on the stream, and treat close as a terminal state:

export class DrainWait {
    private _pending: Promise<void> | null = null;
    private _stream: Writable | null = null;

    wait(stream: Writable): Promise<void> {
        if (this._pending && this._stream === stream) return this._pending;
        if (stream.destroyed) {
            return Promise.reject(new Error('Cannot wait for drain: stream is already destroyed'));
        }
        this._stream = stream;
        this._pending = new Promise<void>((resolve, reject) => {
            const onDrain = () => { cleanup(); resolve(); };
            const onError = (error: Error) => { cleanup(); reject(error); };
            // destroy() without an error emits 'close', never 'error'/'drain'
            const onClose = () => { cleanup(); reject(new Error('Stream closed before it drained')); };
            const cleanup = () => {
                stream.off('drain', onDrain);
                stream.off('error', onError);
                stream.off('close', onClose);
                if (this._stream === stream) { this._pending = null; this._stream = null; }
            };
            stream.once('drain', onDrain);
            stream.once('error', onError);
            stream.once('close', onClose);
        });
        return this._pending;
    }
}

I ran it rather than eyeballing it. Both probes flip (they assert the buggy behaviour, so they go red under the patch) while both controls stay green, and the suites are untouched:

suite	PR as-is (control)	with the patch
@modelcontextprotocol/client	806 passed	806 passed
@modelcontextprotocol/server	476 passed	476 passed
@modelcontextprotocol/core-internal	1447 passed	1447 passed

typecheck on core-internal clean. Your own two client tests and the 11 server stdio tests pass unchanged — the EventEmitter mock never emits close, so nothing there notices.

An alternative worth considering, if you prefer making the class impossible to misuse over making it defensive: take the stream in the constructor (new DrainWait(stream)) so it cannot be handed a different one. That does not work for the client as written, because stdin only exists after start() — but it would if _drainWait were created in start() alongside the process, which also fixes finding 1 by construction and is arguably the smaller change.

What I did not check

• No real child process anywhere in this: everything above is mocks and in-process streams. Whether a real OS pipe reaches 'close' without 'error' in the _dispose() path specifically, I did not confirm — I confirmed the stream semantics, not that particular syscall sequence.
• I did not measure whether the shared-listener change alters throughput under sustained backpressure. Waking N waiters from one drain and having them all re-write in the same turn is a different write pattern than N independent listeners, and I have no numbers either way.
• Bun: the description mentions Bun's warning, and I only ran Node 24.14.0 on darwin-arm64.

Environment: pnpm 10.26.1, node v24.14.0, branch bac3354b on merge-base 3924de99.

[Gursimrxn]

Thanks mycroft - both findings are correct and I've incorporated the patch in 58ca732.

• DrainWait now keys its cached wait on the stream, so a close/start cycle can't leave a send hostage to a dead child's pipe (and it rejects immediately if the stream is already destroyed).
• close is treated as terminal (with a comment noting the cleanup-before-resolve ordering so nobody "tidies" it).

Added three lifecycle tests on real PassThrough streams: close-without-error rejects, a wait on stream B doesn't resolve when stream A drains, and already-destroyed rejects. All three fail against the previous DrainWait (the close one hangs, which is the honest failure mode) and pass with the patch. The listener-count test stays on the EventEmitter mock since that's the right instrument for that claim.

Client 5/5, and the full client/server/core-internal suites are unchanged relative to baseline (the remaining failures in my environment are pre-existing workerd/network-fixture flakes that vary run to run on Windows).