Repository navigation
[v1.x] fix(server): close StdioServerTransport when stdin ends or closes - #2911
maxymlyskov wants to merge 4 commits into
Conversation
StdioServerTransport listened only for 'data' and 'error' on stdin, so a server whose client hung up its end of the pipe never closed: onclose never fired, and a server holding a keep-alive handle kept running with no parent (modelcontextprotocol#2002). Close the transport on stdin 'end' and 'close', also when the stream had already ended or been destroyed before start(), and fire onclose once however many of those arrive. This ports the stdin part of modelcontextprotocol#2494 (6a05402) and its tests; stdout error handling stays with modelcontextprotocol#2579.
🦋 Changeset detectedLatest commit: c83162f The changes in this PR will be included in the next version bump. 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 |
commit: |
|
@claude review |
Closing on stdin 'end' removed the transport's only 'error' listener from the stream. With one socket for both directions (new StdioServerTransport(socket, socket)) a write that was still pending could then fail with EPIPE and, with no listener left, end the process. The listener now stays until the stream closes, so the error reaches onerror as before. Also documents on the class that requests in flight when stdin ends are not answered, as on main.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| _onstdinclose = () => { | ||
| // stdin EOF means the client hung up and nothing more can arrive. | ||
| this.close().catch(() => {}); | ||
| }; |
There was a problem hiding this comment.
🔴 Scripted callers that pipe requests in and close stdin (half-close, e.g. printf '…' | node server.js) now get no response for any handler still running at EOF. src/server/stdio.ts:52 closes the transport on 'end', and Protocol._onclose (src/shared/protocol.ts:657) aborts every in-flight handler, so its response is skipped at src/shared/protocol.ts:788. The changeset calls this accepted; in fact it hits every one-shot pipe whose tool handler awaits anything, since the writer closes the pipe immediately. Fix: on stdin 'end' (as opposed to 'close'/'error'), let request handlers already received before EOF finish and write their responses to stdout before onclose fires and handlers are aborted; keep immediate close on destroy. A Protocol-level drain is the durable fix.
Why this was flagged
A client or shell script writes one or more requests to the server's stdin and then closes its write end while still reading stdout, e.g. printf '…' | node server.js with a tool handler that awaits file or network I/O. The data chunk arrives, Protocol._onrequest schedules the handler, then the EOF read callback fires 'end' and src/server/stdio.ts:73 invokes _onstdinclose, which calls close() and onclose synchronously (src/server/stdio.ts:114). Protocol._onclose aborts every controller in _requestHandlerAbortControllers (src/shared/protocol.ts:657). When the handler resolves, src/shared/protocol.ts:788 sees signal.aborted and returns without sending the JSON-RPC response. On the base branch the transport ignored 'end', the handler completed and the response was written to stdout; after this change the caller reads nothing on stdout and the request silently vanishes. The new test 'should still deliver messages that arrived before stdin ended' only asserts onmessage was called, not that a response is written, so it does not catch this.
Verification: src/server/stdio.ts:73 registers this._stdin.on('end', this._onstdinclose), and close() invokes this.onclose?.() at line 114. Protocol's _onclose at src/shared/protocol.ts:657-660 aborts every in-flight handler; lines 788-791 then return on abortController.signal.aborted, so no response is written to stdout. On c2ce003 start() only listened for 'data' and 'error', so the response was written.
| await closed; | ||
| await server.close(); | ||
| // Allow any late 'close' event from the stream teardown to be delivered | ||
| await new Promise(resolve => setTimeout(resolve, 10)); |
There was a problem hiding this comment.
🟡 nit (optional): this new test waits on a real 10ms setTimeout, which CLAUDE.md asks tests to avoid in favour of vi.useFakeTimers(). Fix: replace the real delay in test/server/stdio.test.ts:306 with fake timers (vi.useFakeTimers() plus await vi.runAllTimersAsync()) or a tick-based flush such as awaiting setImmediate, since the late 'close' event is delivered via nextTick, not a timer. [also at: test/server/stdio.test.ts:306 - nit: test/server/stdio.test.ts — CLAUDE.md asks tests to use vi.useFakeTimers() instead of real setTimeout/await delays: the new 'should not fire onclose twice when close() is called after stdin ends' test waits on a real await new Promise(resolve => setTimeout(resolve, 10)) to let a late… Nothing fails at runtime. The instruction guards against…]
Why this was flagged
The test 'should not fire onclose twice when close() is called after stdin ends' at test/server/stdio.test.ts:306 does await new Promise(resolve => setTimeout(resolve, 10)) to let a late stream 'close' event arrive. CLAUDE.md line 25 asks for vi.useFakeTimers() instead of real setTimeout/await delays in tests. Nothing breaks at runtime; the instruction guards against slow and timing-flaky suites. Two pre-existing tests in the same file (lines 121 and 146) use the same pattern, which is precedent but not what the instruction permits.
Verification: nit. CLAUDE.md line 25 reads "- Testing: Co-locate tests with source files, use descriptive test names. Use vi.useFakeTimers() instead of real setTimeout/await delays in tests". Line 306 of test/server/stdio.test.ts, in a test the diff adds, is await new Promise(resolve => setTimeout(resolve, 10));. Nothing breaks at runtime; the cost is a real wait and timing-based flakiness.
| // A stream that already ended or was destroyed will never emit 'end'/'close' again. | ||
| // Deferred so an onclose assigned right after connect() still sees it. | ||
| if (this._stdin.readableEnded || this._stdin.destroyed) { | ||
| setImmediate(this._onstdinclose); |
There was a problem hiding this comment.
🟡 (optional) Servers that reconnect a fresh transport from onclose now spin forever at 100% CPU once the client hangs up, where the base left them idle. src/server/stdio.ts:68-69 sees process.stdin already readableEnded on every new start() and schedules _onstdinclose via setImmediate, which closes, fires onclose, and the handler connects again, with no yield between iterations. Fix: do not re-arm a dead stdin on each start(); either throw from start() when stdin is already ended/destroyed, or fire onclose once per stream and refuse further starts on the same ended stream.
Why this was flagged
Trigger: user code that resets the server on close, e.g. server.onclose = () => server.connect(new StdioServerTransport()). Entry point is onclose fired from close() at src/server/stdio.ts:114 after stdin EOF. On the second connect, start() at src/server/stdio.ts:68 finds process.stdin.readableEnded true and calls setImmediate(this._onstdinclose) at line 69; that runs close() (line 92), which fires onclose (line 114), which connects again, and so on. setImmediate callbacks run every event-loop turn with no delay, so the process busy-loops and never exits. On the base branch the second transport only attached 'data'/'error' listeners to an ended stream and nothing fired, so the process sat idle and exited when its other handles were released. Remedy: make start() on an already-ended or destroyed stdin reject (or fire onclose only once per Readable, tracked on the stream), so a reconnect loop terminates with an error instead of spinning.
Verification: src/server/stdio.ts:68-69 fires setImmediate(this._onstdinclose) on every start() against an already-ended stdin; _onstdinclose calls close(), which calls this.onclose?.() at line 114. Protocol's wrapper sets this._transport = undefined at src/shared/protocol.ts:664 before invoking the user's onclose at 665, so the reconnect passes the already-connected guard and reaches start() again, with no terminating condition.
…d stream start() defers its own close by one turn when stdin had already ended. If close() ran in between, that deferred handler attached the error listener again on a transport that was already closed. It now does so only when it is the one that closes. The changeset also names two more effects of closing on stdin end: a notification sent afterwards rejects with 'Not connected', and an onclose that calls process.exit() can cut off output that is still being written.
|
The three findings apply to main as well. This PR is a straight backport of #2494, and main's If letting in-flight handlers finish after a half-close is the behavior you want, I think that change belongs on main first, then here. |
This branch has not been deployed
2 participants
Backport of #2494 for #2002 on v1.x.
On v1.x,
StdioServerTransport.start()listens for stdindataanderrorand nothing else (src/server/stdio.ts#L53-L63). So when the client hangs up the pipe, the transport never closes andonclosenever fires. A server that frees a timer or pool inonclosekeeps running with no parent. #2002 has 6 of them piling up in ~17 hours (all PPID=1) and another report of 7 in 5 days on 1.29.0, and its triage comment reproduced the same code on v1.x.Driver: a real child server holding a
setIntervalit clears inonclose. It completes initialize, thenchild.stdin.end(), no signal:The fix is the stdin half of #2494:
endandcloselisteners that close the transport, removed again inclose(), a_closedguard (today a secondclose()firesonclosetwice), and asetImmediateclose when stdin already ended or was destroyed beforestart().One change a v1 user can notice: requests still in flight at stdin EOF are aborted and their responses aren't written. Same as main, and the changeset says so.
Left out: stdout
errorhandling,send()rejecting after close and the swallow-listener sweep. On v1.x that's open #2579, which edits this file and adds its own_closedguard, so whichever lands second gets a small rebase.Tests
test/server/stdio.test.ts: fix(server): close StdioServerTransport when stdin ends or closes #2494's seven stdin tests, unchanged, plus one checkingclose()removes theend/closelisteners. stdin defaults to the sharedprocess.stdinand the_closedguard would hide a leftover listener, so nothing else catches it.test/integration-tests/processCleanup.test.ts+src/__fixtures__/serverWithKeepAlive.tsfrom fix(server): close StdioServerTransport when stdin ends or closes #2494: the child exits on its own when the client closes stdin. It takes its own 15 s timeout, since the describe uses 5 s and the zombie bound is 8 s.On untouched v1.x, 8 of the 9 fail and the listener test passes on both:
npm test: 1818 passed (Windows 11 / Node 22.12)npm run test:e2e: 1114 passed, 1 failed.transport:stdio:shutdown-escalationfails the same way on untouched v1.x (Windows delivers no catchable SIGTERM). On WSL2 Linux / Node 22.22: 1115 passed.npm run typecheck,npm run lint: cleanChangeset included.