Reply with JSON-RPC parse error for over-nested stdio requests#1629
Open
adityasingh2400 wants to merge 3 commits into
Open
Reply with JSON-RPC parse error for over-nested stdio requests#1629adityasingh2400 wants to merge 3 commits into
adityasingh2400 wants to merge 3 commits into
Conversation
When a stdio JSON-RPC request arrived with params nested deeper than the JSON reader's default MaxDepth of 64, full deserialization threw a JsonException that the read loop logged and swallowed. The request id was never recovered, so no response reached the client and the id-bearing request stayed pending until the caller timed out, even though the server process kept handling later requests. The read loop now recovers the request id from the raw line when full parsing fails and replies with a JSON-RPC parse error for that id, so the caller's pending request completes promptly. Id recovery walks only the top-level object with an elevated reader depth and skips other values, so a deeply nested params value cannot make recovery itself fail. Requests without a recoverable id keep the previous behavior of logging and continuing. Fixes modelcontextprotocol#1614
The net472 target lacks the TextReader.ReadLineAsync(CancellationToken) overload, so the new test failed to compile on the Windows build legs. Wrap the token in an #if NET directive to match the existing pattern in SseResponseStreamTransportTests, falling back to the parameterless overload on .NET Framework.
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.
Fixes #1614
A stdio JSON-RPC request whose params nest deeper than the JSON reader's default MaxDepth of 64 left the id-bearing request pending forever while the server stayed healthy. In StreamServerTransport.ReadMessagesAsync the whole line is deserialized in one shot, and the polymorphic JsonRpcMessage converter reads params as a JsonNode. Once nesting exceeds MaxDepth that read throws a JsonException, which the read loop logged and then swallowed with a continue. The request id was parsed off the wire but lost when the exception unwound, so no response ever reached the client and the caller waited until it timed out, even though later shallow requests still succeeded. The session-level error handling that normally turns a thrown JsonException into a parse-error response never runs here because the message never deserializes into a JsonRpcRequest, so it never reaches the session.
This change makes the read loop recover the request id from the raw line when full parsing fails and reply with a JSON-RPC parse error for that id, so the caller's pending request completes promptly instead of hanging. Id recovery walks only the top-level object with an elevated reader depth and skips every other value, including a deeply nested params, so recovery cannot fail for the same depth reason the original parse did. A message with no recoverable id, such as a notification or a line too malformed to correlate, keeps the previous behavior of logging and continuing.
This mirrors the prompt rejection the HTTP transports already give for the same payload, where depth 64 returns immediately rather than leaving the request pending.
Added a regression test in StdioServerTransportTests that feeds a ping request with params nested 100 levels deep and asserts the transport writes back a JsonRpcError carrying the original id and the ParseError code, and that the transport stays connected for further reads. Verified the test hangs to a timeout without the fix and passes with it. Ran the StdioServerTransportTests suite on net10.0, all 13 tests pass.