| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
There was a problem hiding this comment.
Good fix for a real problem — I run into hanging clients on auth failures regularly. Two issues I noticed with the error propagation though.
Sorry, something went wrong.
| if response.status_code in (401, 403): | ||
| status_label = "Unauthorized" if response.status_code == 401 else "Forbidden" | ||
| error_message = f"HTTP {response.status_code} {status_label}" | ||
| if isinstance(message, JSONRPCRequest): |
There was a problem hiding this comment.
This raises after already sending a JSONRPCError to the read stream (line 26). But handle_request_async wraps _handle_post_request in a try/except that catches this HttpError and sends it to read_stream_writer again. So for JSONRPCRequest messages hitting 401/403 (and same for the generic >=400 path below), the consumer receives both a SessionMessage(JSONRPCError) and then an HttpError exception from the same stream — two messages for one request.
I think you either want to not raise here when the JSONRPCError was already sent, or skip the JSONRPCError and just raise (letting the wrapper forward it). The current approach of doing both will confuse callers.
Sorry, something went wrong.
| status_label = "Unauthorized" if response.status_code == 401 else "Forbidden" | ||
| exc = HttpError( | ||
| response.status_code, | ||
| f"HTTP {response.status_code} {status_label}", |
There was a problem hiding this comment.
Re-raising HttpError here means it escapes post_writer entirely — nothing above catches it, so it propagates into the task group and crashes the whole SSE client connection. The original code intentionally swallowed all exceptions to keep the writer alive for the connection lifetime.
For the SSE transport I think you want the same pattern as the other exceptions: send it through the read stream but don't re-raise. The caller can handle it from the stream without killing the connection.
Sorry, something went wrong.
There was a problem hiding this comment.
This is exactly the issue I'm having. I've fixed it in this PR: #2273
Sorry, something went wrong.
|
Thanks for your contribution! Closing this in favour of #2124 as that predates this PR. |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
Note: This PR was authored by Claude (AI), operated by @MaxwellCalkin.
Summary
Fixes #2110
When an MCP server returns non-2xx HTTP status codes (401/403/404/5xx), the Streamable HTTP and SSE transports silently log the error but don't propagate it to the caller. The caller hangs indefinitely waiting for a response on the read stream.
Changes
New HttpError exception class (src/mcp/shared/exceptions.py):
Streamable HTTP transport (src/mcp/client/streamable_http.py):
SSE transport (src/mcp/client/sse.py):
Before
After
Test plan