[Client] Check HTTP status codes in HttpTransport - #425
Conversation
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.
There was a problem hiding this comment.
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+-32601when the server doesn't implement the RPC method — and the spec says outright that "the JSON-RPC error body distinguishes this case from a404returned by a legacy HTTP+SSE server" (2026-07-28 streamable-http)400 Bad Request+-32020 HeaderMismatchon header-validation failure400 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— onstatus >= 400, if the content type is JSON it parses the body and forwards aJSONRPCErrorto the caller (re-stamping the id so correlation works); only if that fails does it synthesize from the status. - TypeScript:
streamableHttp.ts— same for400: parse, match the id against an outstanding request, deliver as a message, otherwise fall through toSdkHttpError.
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
- 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 getsRequestException, as before. - 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.
- 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 byterminateSession(). Extracting a sharedbuildHeaders()forsend()andclose()fixes this and drops the duplicated$this->headersloop.SessionExpiredExceptionleavesstate->initialized === true, soClient::isConnected()keeps returningtrueand the application silently sends session-less requests instead of reconnecting.runRequest()leaks state on throw. The cleanup atHttpTransport.php:206-208is skipped whensend()throws, soactiveFiber,activeProgressCallbackandactiveStreamsurvive — a previously-open SSE stream in particular will be read by the nexttick(). Needs atry/finally.- New
Mcp\Client\Exceptionnamespace. All 28 existing exceptions live inMcp\Exception, including transport-level ones likeConnectionExceptionandTimeoutException. Two namespaces for exceptions is a maintenance trap. SessionExpiredExceptiondoesn't extendHttpTransportException, so no single type catches "any HTTP-level transport failure".testOmitsProtocolVersionHeaderBeforeNegotiationtests the wrong path. It builds a transport with no state, so$this->state?->short-circuits on the null-safe operator. In productionProtocol::connect()callssetState()beforeconnect(), 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 thenull !== $protocolVersionguard were removed.- Missing: a
CHANGELOG.mdentry (two new public exception classes plus a behaviour change), the "Error Handling" section indocs/client.md, and the@throwsannotation onClient::sendRequest(). - Test duplication: six near-identical anonymous
ClientInterfaceclasses and six inlinenew HttpTransport(...)calls, while the file already has acreateTransport()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
$statusCodeis passed toparent::__construct()as the exception code and stored in a readonly property, sogetCode()andgetStatusCode()return the same value two ways.- Header spelled
MCP-Protocol-Version; the rest of the codebase usesMcp-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.
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:
SessionExpiredExceptionis thrown so the application can re-initialize.HttpTransportException(new, inMcp\Client\Exception) carrying the status code and a snippet of the body.It also sends the
MCP-Protocol-Versionheader 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
HttpTransportTestcover the 404-with-session, 404-without-session, 500 text/plain, 200 application/json and header-present/absent cases using mocked HTTP clients.