fix(core): resync ReadBuffer at the next message boundary after an oversized message - #2793
Draft
BerkantACUN wants to merge 1 commit into
Draft
Conversation
…ersized message On overflow the buffer was cleared and reading continued, but the rest of the oversized message was still arriving: it landed in the empty buffer and was fed to the parser as if it were a new message, and a large enough remainder overflowed a second time. The remainder is now dropped, unbuffered, up to and including its newline, and parsing resumes with whatever follows. One oversized message, one error. Second point of modelcontextprotocol#2775.
🦋 Changeset detectedLatest commit: 2b0fed1 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 |
9 tasks
@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.
After an oversized message,
ReadBuffercleared itself and kept reading — but the rest of that message was still arriving. It landed in the empty buffer and was fed to the parser as if it were the start of a new message. The remainder is now dropped, unbuffered, up to and including the newline that ends it, and parsing resumes with whatever follows.Motivation and Context
Second point of #2775. On overflow,
append()calledclear()and threw. Two things followed from that:SyntaxErrorskip hides it; onv1.xit surfaces as a secondonerrorfor the same message (Unexpected token 'A', "AAAAAAAAAA"... is not valid JSONin the issue's output), pointing at nothing real.maxBufferSizeof memory for a message already rejected. ThroughStdioClientTransportthis is usually masked, sinceclose()ends the child before the second half lands (I checked with the issue's 20 MB repro: oneonerroronmaintoo), so the evidence for it is at theReadBufferlevel, in the tests.A parser that resumes at a message boundary has neither problem: one oversized message, one error, and the next real message parses.
How it works
_discardingToNewlineis set and subsequent chunks are skipped without being stored until one contains it.maxBufferSizereaches the parser, as before.clear()also cancels the resync, so a transport that resets the buffer on close does not skip the first line of a new stream.onerror, thenclose()) is unchanged. No public API change;ReadBufferiscore-internal.How Has This Been Tested?
Five unit tests on
ReadBuffer: resumes at the next boundary rather than mid-message; drops a multi-chunk remainder without buffering it (the second-overflow case — fails onmain); keeps what follows the boundary inside the chunk that overflowed (fails onmain); never hands the parser more than the limit even after the boundary (fails onmain);clear()abandons the resync. The existing overflow tests still pass, includingshould clear buffer before throwing on overflow, which exercises the same "append again afterwards" contract.core-internal, client and server stdio suites pass;pnpm typecheck:allandpnpm lint:allpass.Companion to #2792, which carries the overflow error into the caller's rejection; independent of it.
Breaking Changes
None.
Types of changes
Checklist
Disclosure: written with Claude Code; I reviewed the diff and ran the suites myself.