fix(core): carry the transport's last error into the Connection closed rejection - #2792
Open
BerkantACUN wants to merge 1 commit into
Open
Conversation
…d rejection When a transport reports why it is closing and then closes, pending requests were rejected with a bare `Connection closed` and the reason went only to `onerror`. Protocol now keeps the most recent transport error since the last delivered message and settles pending requests with `Connection closed: <reason>` and that error as `cause`. Plain `Connection closed` stays for a close with no reported reason, a close the caller asked for, and an error the transport recovered from. First point of modelcontextprotocol#2775.
🦋 Changeset detectedLatest commit: 9c35605 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
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.
When a transport reports why it is closing and then closes, every pending request was rejected with a bare
SdkError('Connection closed')and the reason went only toonerror.Protocolnow keeps the most recent transport error and settles pending requests withConnection closed: <reason>and that error ascause.Motivation and Context
First point of #2775. The stdio transport already knows exactly what went wrong —
ReadBuffer exceeded maximum size of 10485760 bytesnames the cause and implies the fix (maxBufferSize) — but it says so ononerrorand then closes, and the awaiting caller gets:onerroris a side channel that code doing the ordinary thing (await client.listTools()) never sees unless it knew in advance to wire it up. Same shape in #1049 from a different cause. With this change the same repro gives:and
error.causeis the transport'sError, so loggers that walk the cause chain get it too.How it works
Protocol.connectrecords the error in itstransport.onerrorwrapper, and forgets it in itstransport.onmessagewrapper: a message delivered after an error means the transport recovered from it, so it is not why the connection closed. Without that, an SSE reconnect error from an hour ago would be blamed for an unrelated close._onclosebuilds the rejection through a new protected_connectionClosedError(), andClient._oncloseuses the same helper for itslisten()state so both settle with one reason instead of one with and one without.Protocol.close()clears the retained error first: a close the caller asked for has no transport-reported reason, whatever the transport complained about earlier.Connection closedprefix andSdkErrorCode.ConnectionClosedis unchanged; nothing in the repo matches the message exactly (checkedpackages/,test/,examples/,docs/). No transport changes and no public API change beyond the error message andcause.The other two points of #2775 are separate: the
ReadBufferresync is #2793, and @errmakov has the BOM case in #2790.How Has This Been Tested?
Five
Protocolunit tests with the mock transport (reason carried; plain fallback; forgotten after a later message; cleared on caller-initiatedclose(); not carried acrossconnect()to a second transport) and one end-to-end through the realStdioClientTransportwithmaxBufferSize: 1024against a server whosetools/listresult is larger — the repro from the issue, scaled down. The positive cases fail onmainbefore the change (Connection closedwith no cause).core-internal,clientandserversuites pass;pnpm typecheck:allandpnpm lint:allpass.I also ran the issue's original 20 MB repro against this branch: one
onerror, and the caller's rejection readsConnection closed: ReadBuffer exceeded maximum size of 10485760 bytes.Breaking Changes
None. The message gains a suffix when a reason is known; the code and the prefix are unchanged.
Types of changes
Checklist
Disclosure: written with Claude Code; I reviewed the diff and ran the repro and the suites myself.