fix(client): make the Streamable HTTP no-reply failure diagnosable and cover chunked application/json replies - #1875
Open
ump45nose wants to merge 1 commit into
Conversation
The no-reply failure of the Streamable HTTP client transport only reported the request ID, even when the peer had responded with a different ID, an empty body, or something that was not a JSON-RPC reply. Response bodies are only logged at Trace level through LogTransportReceivedMessageSensitive, so the failure was not diagnosable at default logging. Track non-sensitive shape facts about the uncorrelated response (media type, empty body, deserialization failures, the ID the peer actually replied with, a network-terminated event stream) and append them to the exception message. No payload is copied into the message, so the existing sensitive-logging boundary is preserved and the original message prefix stays intact. Add production-equivalent coverage for a chunked application/json reply: a loopback socket answers HTTP 200 with Transfer-Encoding: chunked, no Content-Length, and a correlated JSON-RPC error, driven through the transport's normal ResponseHeadersRead path. The correlated error is delivered to the session and drives the existing server/discover -> initialize fallback. Related to modelcontextprotocol#1862
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.
Summary
Addresses the two asks in #1862 for the Streamable HTTP client transport: production-equivalent coverage for a chunked
application/jsonreply, and diagnostics on the no-reply failure.1. Coverage for a chunked
application/jsonreply (newStreamableHttpResponseCorrelationTests)The existing coverage for this path uses an in-memory
StringContent, which is already buffered and carries aContent-Length. The two new tests drive a raw loopback socket instead, answering with HTTP 200,Content-Type: application/json; charset=utf-8,Transfer-Encoding: chunked, noContent-Length, and the body split across 16-byte chunks — consumed through the transport's normalResponseHeadersReadflow (the request body is written chunked as well, sinceJsonContenthas no known length). They assert that a correlated JSON-RPC error is (a) delivered to the session and (b) drives the existingserver/discover→initializefallback to2025-06-18.Finding worth recording for the original report: both tests pass on current
mainwith no transport change. I could not reproduce the loss described in the issue for a correlated error in a chunkedapplication/json200 response, including when theserver/discoverprobe is answered with a-32600error. So the framing itself does not appear to be the cause; the failure mode that remains reachable is a reply the transport cannot correlate.2. The no-reply failure now reports what actually happened
McpException: Streamable HTTP POST response completed without a reply to request with ID: 1was the only signal, even when the peer had clearly answered. Because response bodies are only logged atTracelevel viaLogTransportReceivedMessageSensitive, everything needed to diagnose the failure was invisible at default logging.The transport now accumulates non-sensitive shape facts about the uncorrelated response and appends them to that message:
...without a reply to request with ID: 1...without a reply to request with ID: 1. The response Content-Type was 'application/json', and the response contained a JSON-RPC response or error with ID '42', which does not match the request ID.The reported cases are the four requested in the issue plus two cheap neighbours: empty JSON body; JSON-RPC deserialization failure; a message that is not a response/error for this request; a response or error whose ID differs from the request ID; a response or error with a null ID (JSON-RPC permits this when the peer could not determine the request ID); and a
text/event-streamthat ended with a network error. The response media type is always included.Only message shape and IDs are reported. No message payload is copied into the exception message, so the existing sensitive-logging boundary is preserved, and the original
...without a reply to request with ID: Xprefix is kept intact.Verification
dotnet test tests/ModelContextProtocol.Tests -f net10.0 --filter "FullyQualifiedName~StreamableHttpResponseCorrelationTests"→ 6 passed, 0 failed.src/ModelContextProtocol.Core/Client/StreamableHttpClientSessionTransport.cstemporarily restored toHEAD→ 4 failed, 2 passed. The four failures are exactly the new diagnostics assertions (NoReply_ResponseWithDifferentId_ExceptionNamesTheObservedId,NoReply_NullIdErrorResponse_ExceptionReportsTheNullId,NoReply_EmptyJsonBody_ExceptionReportsTheEmptyBody,NoReply_NonJsonContentType_ExceptionReportsTheMediaType); the two chunked-response tests pass on unmodifiedmain, which is the finding above.dotnet test tests/ModelContextProtocol.Tests -f net10.0 --filter "FullyQualifiedName~Tests.Transport|FullyQualifiedName~Tests.Client|FullyQualifiedName~Tests.Protocol"→ 1358 passed, 0 failed, 2 skipped.Related to #1862.