[v1.x] fix(client): drain piped stderr so unread pipe cannot deadlock - #2794
Draft
BerkantACUN wants to merge 1 commit into
Draft
[v1.x] fix(client): drain piped stderr so unread pipe cannot deadlock#2794BerkantACUN wants to merge 1 commit into
BerkantACUN wants to merge 1 commit into
Conversation
v1.x backport of modelcontextprotocol#2788. `StdioClientTransport` with `stderr: 'pipe'` (or `'overlapped'`) pipes child stderr into a PassThrough that stays paused until a reader attaches. A server that logs a normal amount fills that buffer and the OS pipe, blocks on write(2), stops reading stdin, and the session hangs with no error, no rejection and no timeout. After piping, the PassThrough is put in flowing mode so unread chunks drain; listeners attached before start() still receive every chunk, a late listener sees only post-attach data. Fixes modelcontextprotocol#2776 on v1.x.
|
commit: |
4 tasks
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.
v1.x backport of #2788 by @tiagovilasboas. Opened as a draft until #2788 lands; I will follow whatever changes review makes there.
StdioClientTransportwithstderr: 'pipe'(or'overlapped') pipes child stderr into aPassThroughthat stays paused until a reader attaches. A server that logs a normal amount to stderr fills that buffer (16 KiBhighWaterMark) and then the OS pipe, blocks onwrite(2), stops reading stdin, and the session hangs — noonerror, no rejection, no timeout.await client.listTools()never returns. #2776 was reported on this line (@modelcontextprotocol/sdk@1.30.0).Motivation and Context
Same as #2788: after
child.stderr.pipe(passThrough), callpassThrough.resume()so the stream is in flowing mode and unread chunks drain. Listeners attached beforestart()/connect()still receive every chunk. A late listener sees only post-attach data — lost log lines beat a hung session. Both docstrings say so, including that paused-moderead()is not supported afterstart().Differences from #2788
src/client/stdio.tsandtest/client/stdio.test.ts(single-package layout)..js-suffixed relative style; the three test cases are otherwise identical.How Has This Been Tested?
The three regression tests from #2788 (no reader →
listTools()completes; listener beforestart()receives startup and post-request chunks; late listener → session completes, sees only post-attach chunks). The first and third hang onv1.xbefore the change and fail on the 8 s test timeout.npm run checkpasses;test/client/stdio.test.ts8/8.Also ran the issue's repro (a server writing 200 × 45 KB lines to stderr on the first request, no reader) against this branch on Windows/Node 22: hangs on 1.30.0, returns in ~220 ms here.
Breaking Changes
None to the API. Behavioural: a listener attached to
transport.stderrafterstart()no longer sees data written before it attached.Types of changes
Checklist
Disclosure: prepared with Claude Code; I reviewed the diff against #2788 and ran the tests and the repro myself.