fix(client): surface non-2xx HTTP responses to the waiting caller - #3092
Conversation
Sorry, something went wrong.
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
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
Sorry, something went wrong.
`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
himanshu-commits
left a comment
There was a problem hiding this comment.
done changes
Sorry, something went wrong.
There was a problem hiding this comment.
The core mechanism is exactly right. Non-2xx now produces a JSONRPCError stamped with the original request's id, sent as a SessionMessage — which routes through _handle_response and resolves the specific _response_streams[request_id] future the caller is awaiting.
Suggestions (minor — none of these blocking)
-
3xx edge case. raise_for_status() used to raise for anything non-2xx; the new guard is >= 400. A 3xx now falls through to the content-type dispatch → for a request, likely _handle_unexpected_content_type → bare ValueError → _handle_incoming → the caller hangs until read timeout — the fixed failure mode, on a rarer path. Mitigated in practice: SDK-created clients set follow_redirects=True (_httpx_utils.py), so this only bites user-supplied non-redirecting clients against a redirecting endpoint — and 2.0 has the identical >= 400 guard, so it's parity. Might deserve one line in the PR description, or a >= 300 guard.
-
Resumption GET path still has the same bug class. _handle_resumption_request keeps its bare event_source.response.raise_for_status(). Also unfixed on 2.0 — fine as a follow-up, just noting it so it doesn't get lost.
-
Consider carrying the HTTP status in ErrorData.data. The description already notes callers can't tell a terminal 401 from a retryable 503 — both land as -32603 "Server returned an error response". data is free-form per the JSON-RPC spec, so e.g. data={"http_status": 401} would be fully backward-compatible and would let clients fail fast on auth failures vs. retry on 5xx. Speaking as a downstream consumer (multi-tenant Temporal SDK wrapping this client): this distinction is exactly what we need for retry classification, so if the deferred "carry the status" follow-up happens, data seems like the natural spot on both lines.
Nice work — the compat reasoning around 404 and the HTTPError-vs-StreamError distinction on the body read are exactly the kind of care this path needed.
Sorry, something went wrong.
|
@afterrburn thanks for the review — that's the most thorough one I've had, and the Acting on your suggestions:
On #3094's naming: I went with httpStatus (camelCase, matching MCP's wire conventions like |
Sorry, something went wrong.
Reviewers
At least 1 approving review is required to merge this pull request.
Relates to #3091.
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 — a 500 on one tools/list no longer kills the next call.
Motivation and Context
Reported in #3091: HTTP status errors never reached the caller, and were indistinguishable
from a timeout. This is fixed on the 2.0 line already; per the discussion on the issue, this
PR backports the equivalent behaviour to v1.x, where the bug still reproduces (1.25.0 /
1.28.1).
One deliberate difference from 2.0
2.0 checks the response body before the 404 fallback; this PR checks 404 first.
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. Two existing tests caught this. Behaviour is identical to 2.0
for every status this issue is actually about.
Related: that 32600 is positive, where JSON-RPC codes are negative (2.0 corrected it to
-32600). I've left it alone — changing it would break anyone checking error.code == 32600
on a stable release. Happy to change it if you'd rather.
Note on 3xx
raise_for_status() used to raise for any non-2xx; the new guard is >= 400, so a redirect
falls through to the content-type dispatch rather than the correlated-error path. In practice
create_mcp_http_client sets follow_redirects=True, so this only affects user-supplied
clients with redirects disabled — and 2.0 carries the identical >= 400 guard, so it's
parity, not a regression. Tracked separately rather than widened here.
What this does not fix
Callers still cannot tell a terminal 401 from a retryable 503: both arrive as
-32603 Server returned an error response, exactly as on 2.0. The issue's stated impact
(retry policy) therefore isn't fully addressed here. Carrying the originating status on
error.data would close that, but belongs on the 2.0 line first — happy to follow up.
How Has This Been Tested?
Ten new tests in tests/shared/test_streamable_http.py, driven in-memory via
httpx.MockTransport:
so without the fix they hang rather than pass vacuously — that's the regression pin.
Full suite passes (1149), 100% branch coverage. Six # pragma: no cover markers removed from
the 404 path, which was previously untested and is now exercised.
Breaking Changes
None. 404 handling is preserved byte-for-byte, including its code and message. Every other
non-2xx previously raised an exception that escaped the transport; it now resolves the caller
instead.
Types of changes
Checklist
Additional context
_handle_resumption_request still carries a bare raise_for_status() — the same bug class on
the resumption GET path, unfixed on 2.0 as well. Out of scope here; noting it so it isn't lost.