fix(client): reject stdio send() when the write fails instead of waiting for 'drain'#2552
Open
ondraulehla wants to merge 1 commit into
Open
Conversation
A backpressured `StdioClientTransport.send()` waited for a `'drain'` event, but a pipe destroyed by the server process exiting never drains, so the promise stayed pending for the lifetime of the process and the `'drain'` listener leaked. Settle from the `write()` callback instead, which Node invokes on flush or on failure, so the send rejects with the underlying write error. This is what `StdioServerTransport.send()` already does for its own stdout.
🦋 Changeset detectedLatest commit: 25eab69 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: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
StdioClientTransport.send()now settles from thewrite()callback, so a backpressured send can't stay pending forever when the pipe to the server dies.Motivation and Context
I noticed this while reading the two stdio transports side by side. The server one rejects a failed write, the client one has no error path at all: its promise executor only takes
resolve, and a backpressured write resolves on'drain'. If the server process then exits, orclose()escalates to SIGTERM and SIGKILL, the stream is destroyed, and a destroyed stream never drains. The promise just stays pending, and the'drain'listener is never removed.The symptom is that
await client.notification(...)never comes back. The notification path awaits the send with no timeout, and the connection-closed teardown settles pending responses but not pending sends, so nothing rescues it. I tried it against a server that answersinitialize, stops reading stdin and exits, with 8 MB in flight: the await is still pending after 8 seconds, and with this change it rejects in 0.4 seconds. Requests were already covered by the teardown, so for them the only difference is that they now report the actualEPIPEinstead of a generic connection-closed error.You need a write the pipe won't take in one go to get there, which on Linux is between 160 KB and 224 KB depending on the Node version. Base64 image payloads and file contents in tool arguments reach that routinely.
The server transport has rejected on a write failure since #1568, with a test pinning its listener cleanup, so this brings the client side in line. The fast path stays as it was, since a
write()that returns true still resolves right away and only the failure path differs.How Has This Been Tested?
packages/client/test/client/stdio.test.ts, using a server that never reads stdin and then exits with 8 MB of params in flight. Without the change the send never settles and the test reportshung; with it the send rejects and no'drain'listener is left behind, which is the same leak check the server transport test already makes. I bounded the wait so the old behaviour fails fast instead of timing out the suite, and I don't assert the rejection reason because it is platform specific.pnpm test:allis green on Node 24. I also ran the new test ten times to make sure it isn't flaky. Typecheck, eslint and prettier are clean, changeset included.Breaking Changes
None.
send()already returnsPromise<void>and the sibling stdio transport rejects it on a write failure, so callers that awaited it keep working.v1.xhas the same pattern insrc/client/stdio.ts, and insrc/server/stdio.tstoo since #1568 only landed onmain. Happy to send that separately if you want it.Types of changes
Checklist