fix(client): surface non-2xx HTTP responses to the waiting caller#3092
Open
himanshu-commits wants to merge 2 commits into
Open
Conversation
A 4xx/5xx response to a message POST hit a bare `response.raise_for_status()` inside the POST's background task. Nothing caught the exception and nothing resolved the request, so the caller sat until its read timeout fired: a 401 was indistinguishable from a slow server, and the escaping error tore down the transport's task group with it. Non-2xx statuses are now answered rather than raised, generalising the handling 404 already had: a `JSONRPCError` stamped with the original request's id, sent as a `SessionMessage` over the read stream, so it resolves through the normal response-correlation path. A spec-compliant JSON-RPC error in the body is preferred over the status-derived stand-in. The connection stays usable. This matches the behaviour already shipped on the 2.0 line, with one deliberate difference: 404 is answered ahead of the body check. A terminated session 404s *with* a JSON-RPC error body, so preferring it would replace this line's `Session terminated` / code 32600 — which callers match on for session expiry — with the server's own wording. Callers still cannot tell a terminal 401 from a retryable 503; both remain `-32603 Server returned an error response`, as on 2.0. Carrying the status would be a follow-up on the 2.0 line. Github-Issue: modelcontextprotocol#3091 Reported-by: afterrburn
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 2 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
`httpx.StreamError` derives from `RuntimeError`, not `httpx.HTTPError`, so it does not cover a body that stalls or truncates while being read: `aread()` raises `ReadTimeout`/`RemoteProtocolError` there. Those escaped into the POST's background task and stranded the caller — the bug this path exists to prevent. Github-Issue: modelcontextprotocol#3091
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.
A 4xx/5xx response to a message POST hit a bare
response.raise_for_status()inside the POST's background task. Nothing caught the exception and nothing resolved the request, so the caller sat until its read timeout fired: a 401 was indistinguishable from a slow server, and the escaping error tore down the transport's task group with it.Non-2xx statuses are now answered rather than raised, generalising the handling 404 already had: a
JSONRPCErrorstamped with the original request's id, sent as aSessionMessageover the read stream, so it resolves through the normal response-correlation path. A spec-compliant JSON-RPC error in the body is preferred over the status-derived stand-in. The connection stays usable.This matches the behaviour already shipped on the 2.0 line, with one deliberate difference: 404 is answered ahead of the body check. A terminated session 404s with a JSON-RPC error body, so preferring it would replace this line's
Session terminated/ code 32600 — which callers match on for session expiry — with the server's own wording.Callers still cannot tell a terminal 401 from a retryable 503; both remain
-32603 Server returned an error response, as on 2.0. Carrying the status would be a follow-up on the 2.0 line.Github-Issue: #3091
Reported-by: afterrburn
Motivation and Context
How Has This Been Tested?
Breaking Changes
Types of changes
Checklist
Additional context