fix(client): drain piped stderr so unread pipe cannot deadlock - #2788
fix(client): drain piped stderr so unread pipe cannot deadlock#2788tiagovilasboas wants to merge 4 commits into
Conversation
Put the stderr PassThrough in flowing mode after piping so a chatty child cannot fill the unread pipe and hang the session. Listeners attached before start() still receive chunks. Fixes modelcontextprotocol#2776 Co-authored-by: Tiago Vilas Boas <tcarvalhovb@gmail.com>
Assert listTools() completes when stderr is piped with no reader, that a pre-start listener still receives chunks, and that a late listener does not deadlock and sees only post-attach output. Co-authored-by: Tiago Vilas Boas <tcarvalhovb@gmail.com>
Co-authored-by: Tiago Vilas Boas <tcarvalhovb@gmail.com>
🦋 Changeset detectedLatest commit: 910cb45 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: |
Avoid racing stdout completion against flowing-mode stderr delivery, and keep flood size large enough to fill a paused pipe without dumping megabytes into assertion output. Co-authored-by: Tiago Vilas Boas <tcarvalhovb@gmail.com>
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.
BerkantACUN
left a comment
BerkantACUN
left a comment
There was a problem hiding this comment.
Ran the repro from the issue against this branch rather than reading the diff alone — npm i https://pkg.pr.new/@modelcontextprotocol/client@2788, the same chatty server (200 × 45 KB lines to stderr on the first request), Node 22.22.1, Windows.
| listener | @modelcontextprotocol/client@2.0.0 |
this PR |
|---|---|---|
| none | hangs (killed at 5 s) | 1 tool in 218 ms, 0 bytes seen |
data before start() |
— | 1 tool, all 9,000,213 bytes, STARTUP + POST markers |
.pipe() before start() |
— | same |
readable before start() |
— | same |
data after connect() |
all bytes, STARTUP included | 1 tool, POST only, STARTUP gone |
overlapped, none / early |
— | same as pipe |
So it does what the description says, and the late-listener row is exactly the trade the docstring now describes. Early listeners lose nothing.
One nuance in the docstring, not a blocker: the readable row works because Node's resume() is a no-op when a 'readable' listener is attached, so the paused-mode API keeps working as long as the listener was there before start(). The only shape that actually changes is a bare read() loop with no 'readable' listener. "Not supported" is stricter than the behaviour; fine to leave as-is if you prefer the conservative wording.
The one thing I'd keep an eye on is the negative assertion in the late-listener test (STARTUP_MARKER absent). It relies on the child's startup stderr chunk being drained before setImmediate fires; stdout and stderr are separate pipes with no ordering guarantee between them. The initialize round trip is a large margin so I don't expect it to flake, but if it ever does, dropping that one expect is the fix — the session-completes assertion plus the POST marker are what pin the bug.
The v1.x backport is prepared as #2794 (draft until this lands, and it will follow whatever review changes here), since 1.30.0 is where I hit it and where most installs still are. Same repro against that branch: 229 ms with no reader.
2 participants
Problem
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, then the OS pipe), blocks onwrite(2), stops reading stdin, and the session hangs — noonerror, no rejection.await client.listTools()never returns.Solution
Issue option (1): after
child.stderr.pipe(passThrough), callpassThrough.resume()so the stream is in flowing mode. Unread chunks are drained; listeners attached beforestart()/connect()still receive every chunk.Late listeners (after
connect()) see only post-attach data — lost log lines beat a hung session. Documented onStdioServerParameters.stderrand thestderrgetter, including that paused-mode.read()is not supported afterstart().Test plan
stderr: 'pipe', no reader →listTools()completes (timeout = fail)start()still receives startup + post-request chunkspnpm --filter @modelcontextprotocol/client exec vitest run test/client/stdio.test.ts— 9/9 passed locally (1.21s)Follow-up
v1.x backport (
src/client/stdio.ts) is out of scope for this PR.Fixes #2776