Skip to content

[Client] Check HTTP status codes in HttpTransport - #425

Open
ez-lbz wants to merge 2 commits into
modelcontextprotocol:mainfrom
ez-lbz:client-http-status-checks
Open

[Client] Check HTTP status codes in HttpTransport#425
ez-lbz wants to merge 2 commits into
modelcontextprotocol:mainfrom
ez-lbz:client-http-status-checks

Conversation

@ez-lbz

@ez-lbz ez-lbz commented Aug 16, 2026

Copy link
Copy Markdown

HttpTransport::send() never looked at the HTTP status code of the response. A 404 JSON error body was parsed as a regular message, and non-JSON error bodies (e.g. text/plain) were dropped silently, leaving the caller waiting on the request timeout.

This change checks the status code before dispatching the body:

  • 404 with a session id set means the session is gone: the local session id is cleared and SessionExpiredException is thrown so the application can re-initialize.
  • any other non-2xx status throws HttpTransportException (new, in Mcp\Client\Exception) carrying the status code and a snippet of the body.

It also sends the MCP-Protocol-Version header on every POST once the initialize handshake has negotiated a version, as the streamable HTTP spec requires; before negotiation the header is omitted.

Tests in HttpTransportTest cover the 404-with-session, 404-without-session, 500 text/plain, 200 application/json and header-present/absent cases using mocked HTTP clients.

HttpTransport::send() never looked at the HTTP status code of the
response. A 404 JSON error body was parsed as a regular message, and
non-JSON error bodies (e.g. text/plain) were dropped silently, leaving
the caller waiting on the request timeout.

Check the status code before dispatching the body:
- 404 with a session id set means the session is gone: clear the local
  session id and throw SessionExpiredException so the application can
  re-initialize.
- any other non-2xx status throws HttpTransportException carrying the
  status code and a snippet of the body.

Also send the MCP-Protocol-Version header once the initialize handshake
has negotiated a version, as the streamable HTTP spec requires.

Tests cover the 404-with-session, 404-without-session, 500, 200 and
header-present/absent cases with mocked HTTP clients.

@chr-hertel chr-hertel left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @ez-lbz, thanks for tackling this! I ran my review skill on this and it came back with quite some hits, so i'll drop that here for you - at least what i could follow as well.


Silently dropping non-2xx responses is a real bug and worth fixing. I checked the change against the spec and against the TypeScript and Python SDKs, and the core logic needs restructuring before this can go in.

Critical

Non-2xx bodies are discarded. HttpTransport::send() throws before looking at the body, but the spec requires a JSON-RPC error body on several non-2xx responses:

  • 404 Not Found + -32601 when the server doesn't implement the RPC method — and the spec says outright that "the JSON-RPC error body distinguishes this case from a 404 returned by a legacy HTTP+SSE server" (2026-07-28 streamable-http)
  • 400 Bad Request + -32020 HeaderMismatch on header-validation failure
  • 400 Bad Request + UnsupportedProtocolVersionError, listing the server's supported versions

Both reference SDKs parse the body first and only fall back to a status-derived error:

  • Python: streamable_http.py — on status >= 400, if the content type is JSON it parses the body and forwards a JSONRPCError to the caller (re-stamping the id so correlation works); only if that fails does it synthesize from the status.
  • TypeScript: streamableHttp.ts — same for 400: parse, match the id against an outstanding request, deliver as a message, otherwise fall through to SdkHttpError.

This also makes the SDK self-inconsistent: our own server returns 404 and 400 with JSON-RPC error bodies (src/Server/Protocol.php:671-681), and our own client would now throw them away. The test fixture in this PR is the proof — it sends {"jsonrpc":"2.0","id":1,"error":{"code":-32000,"message":"Session not found"}} and nothing ever parses it.

Most immediately, an UnsupportedProtocolVersionError becomes an opaque string exactly where the new negotiation code needs to read the server's version list.

404 + session id is not enough to conclude "session expired". In 2026-07-28 method not found is also 404, and a session id is set on every post-handshake request. So the first call to an unsupported method clears a perfectly live session and raises SessionExpiredException. Python guards against this by parsing the body first, and its status-derived fallback is deliberately narrow — its comment on the no-session branch is literally "'Session terminated' would be a lie here". TypeScript doesn't special-case 404 at all.

The new exceptions break Client::connect()'s retry loop. Both extend Mcp\Exception\Exception, not ConnectionException. send() is called inside the fiber that HttpTransport::connect() starts, and Protocol::request() only has a finally, so the exception propagates out of Fiber::start() and out of connect() — where Client.php:99-108 catches only ConnectionException. A 503 during the handshake now skips the retries, the setInitialized(false) reset, and $transport->close(). Before this PR that same 503 timed out into a retried ConnectionException, so this is a regression for exactly the transient errors the retry exists for.

Suggested shape

  1. On non-2xx with a JSON content type: parse the body; if it's a JSON-RPC error, dispatch it via handleMessage() so the waiting fiber resolves and the caller gets RequestException, as before.
  2. Only on parse failure use the status-derived path — and there, restrict the session-expiry conclusion to a 404 whose body was not a JSON-RPC error.
  3. Have the remaining exceptions extend ConnectionException.

That also removes most of the exception-type churn, since the common case resolves through the existing RequestException path.

Improvement

  • close()'s DELETE doesn't get the protocol-version header. The spec says "all subsequent requests", and both SDKs build headers once and reuse them everywhere — Python's _prepare_headers() is explicitly documented as covering "transport-internal GET/DELETE", TS's _commonHeaders() is used by terminateSession(). Extracting a shared buildHeaders() for send() and close() fixes this and drops the duplicated $this->headers loop.
  • SessionExpiredException leaves state->initialized === true, so Client::isConnected() keeps returning true and the application silently sends session-less requests instead of reconnecting.
  • runRequest() leaks state on throw. The cleanup at HttpTransport.php:206-208 is skipped when send() throws, so activeFiber, activeProgressCallback and activeStream survive — a previously-open SSE stream in particular will be read by the next tick(). Needs a try/finally.
  • New Mcp\Client\Exception namespace. All 28 existing exceptions live in Mcp\Exception, including transport-level ones like ConnectionException and TimeoutException. Two namespaces for exceptions is a maintenance trap.
  • SessionExpiredException doesn't extend HttpTransportException, so no single type catches "any HTTP-level transport failure".
  • testOmitsProtocolVersionHeaderBeforeNegotiation tests the wrong path. It builds a transport with no state, so $this->state?-> short-circuits on the null-safe operator. In production Protocol::connect() calls setState() before connect(), so the real pre-negotiation state is non-null with a null version. The test passes for the wrong reason and would keep passing if the null !== $protocolVersion guard were removed.
  • Missing: a CHANGELOG.md entry (two new public exception classes plus a behaviour change), the "Error Handling" section in docs/client.md, and the @throws annotation on Client::sendRequest().
  • Test duplication: six near-identical anonymous ClientInterface classes and six inline new HttpTransport(...) calls, while the file already has a createTransport() helper using named arguments.
  • Test gaps: nothing asserts the session id is preserved on a non-404 failure, and there's no case for a 2xx with an empty body (202/204), which the notification path actually produces.

Nitpick

  • $statusCode is passed to parent::__construct() as the exception code and stored in a readonly property, so getCode() and getStatusCode() return the same value two ways.
  • Header spelled MCP-Protocol-Version; the rest of the codebase uses Mcp-Protocol-Version / Mcp-Session-Id. Case-insensitive on the wire, but inconsistent.
  • The 500-character snippet cap is a bare magic number, and substr() can split a multi-byte sequence mid-character — mb_substr() avoids a mangled tail.
  • f0cfaf7 ("Retry CI: composer network error") is an empty commit and should be dropped before merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants