fix: preserve HTTP status codes in streamable HTTP transport error responses#2176
Closed
giulio-leone wants to merge 2 commits intomodelcontextprotocol:mainfrom
Closed
Conversation
…sponses
The streamable HTTP transport had two error propagation issues:
1. Non-2xx responses used generic error messages ('Session terminated',
'Server returned an error response') that discarded the HTTP status
code, making it impossible to distinguish auth errors (401/403) from
server errors (500) or not-found (404).
2. Connection/transport errors (timeouts, DNS failures, connection
refused) raised inside _handle_post_request were not caught, causing
callers to hang indefinitely waiting for a response on the read
stream.
Fix:
- Consolidate 404 and general >= 400 handlers into a single block that
reads the response body and includes 'HTTP {status}: {body}' in the
error message
- Wrap _handle_post_request in try/except to catch transport errors and
forward them as JSONRPCError through the read stream before re-raising
Fixes modelcontextprotocol#2110
8d879bb to
0176b50
Compare
Author
|
Closing — the test failures indicate this branch has diverged too far from main. The HTTP status code preservation approach needs to be reworked to align with recent upstream changes. |
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.
Problem
The streamable HTTP transport has two error propagation issues that cause clients to hang or receive uninformative error messages:
1. HTTP status codes are discarded in error responses
Non-2xx responses produce generic error messages that lose the original HTTP status:
404→ErrorData(code=INVALID_REQUEST, message="Session terminated")401/403/500/...→ErrorData(code=INTERNAL_ERROR, message="Server returned an error response")Callers cannot distinguish auth errors (401/403) from server errors (500) or other failures.
2. Transport errors cause caller to hang
Connection-level errors (timeouts, DNS failures, refused connections) raised inside
_handle_post_requestare not caught. When called viatg.start_soon()forJSONRPCRequestmessages, the exception propagates to the task group but noJSONRPCErroris written to the read stream — the caller blocks indefinitely.Fix
Status code preservation
Consolidated the separate 404 and
>= 400handlers into a single block that:HTTP {status}: {body}in the error messageTransport error propagation
Wrapped
_handle_post_requestin atry/exceptthat:JSONRPCErrorthrough theread_stream_writerso the caller gets an error instead of hangingTest
13 fast tests pass (validation, JSON response, content type, session), 2 consecutive clean runs. Integration tests not affected (verified
test_streamable_http_client_json_responsepasses).Fixes #2110