diff --git a/CHANGELOG.md b/CHANGELOG.md index 581026eb..baf62a2e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -43,6 +43,7 @@ All notable changes to `mcp/sdk` will be documented in this file. * [BC Break] `TransportInterface::send()` receives every response of a handler that does not suspend, not only errors raised before a session exists; the session's outgoing queue only carries server-initiated requests and notifications. A custom transport must deliver each message it gets, as a batch gets several. * Fix the client request timeout firing up to a second early and throwing a generic `RequestException`: it now throws `TimeoutException`, which extends `RequestException`, and sends `notifications/cancelled` for the abandoned request. * [BC Break] `TimeoutException` extends `RequestException` instead of `Exception`, so a `catch (RequestException)` now also catches timeouts. `ClientStateInterface::getPendingRequests()` reports `timestamp` as a `float` from `microtime(true)` instead of an `int`. +* Fix the client's `HttpTransport` waiting out the request timeout when a non-streamed response to a request ends without its answer (truncated, empty, unreadable or not JSON): the call now fails at once with a `RequestException`, also when the body of an error status fails to read. * Serve both protocol eras over stdio: `StdioTransport` settles the era on the client's first request and serves `2026-07-28` requests, `subscriptions/listen` and `notifications/cancelled` on the one channel. * [BC Break] `StatelessAwareTransportInterface` declares `setHandshakeVersions()`, so a server without the modern era names only the revisions it negotiates when refusing a `2026-07-28` request, e.g. the one set with `Builder::setProtocolVersion()`. * Answer a bare `initialize` on a `2026-07-28`-only endpoint with `-32022` naming the served revisions, and a request without a session on the handshake leg with its id. diff --git a/src/Client/Transport/HttpTransport.php b/src/Client/Transport/HttpTransport.php index bed19e65..4435113b 100644 --- a/src/Client/Transport/HttpTransport.php +++ b/src/Client/Transport/HttpTransport.php @@ -239,19 +239,54 @@ public function send(string $data): void $contentType = strtolower($response->getHeaderLine('Content-Type')); if ($response->getStatusCode() >= 400) { - $this->handleErrorStatus($data, $response->getStatusCode(), $response->getReasonPhrase(), $response->getBody()->getContents()); + try { + $body = $response->getBody()->getContents(); + } catch (\RuntimeException) { + // The status alone still tells what went wrong. + $body = ''; + } + + $this->handleErrorStatus($data, $response->getStatusCode(), $response->getReasonPhrase(), $body); return; } if (str_contains($contentType, 'text/event-stream')) { $this->startSseStream($this->responseStream($response->getBody()), self::requestIdOf($data)); - } elseif (str_contains($contentType, 'application/json')) { + + return; + } + + try { $body = $response->getBody()->getContents(); - if (!empty($body)) { - $this->handleMessage($body); - } + } catch (\RuntimeException $e) { + $this->failUnanswered($data, $e->getMessage()); + + return; + } + + $dispatched = str_contains($contentType, 'application/json') && '' !== trim($body); + if ($dispatched) { + $this->handleMessage($body); } + + if (null !== ($requestId = self::requestIdOf($data)) && !($dispatched && self::answers($body, $requestId))) { + $this->failUnanswered($data, '' === trim($body) ? 'empty body' : 'no answer in the body'); + } + } + + /** + * Fails a request whose response ended without its answer at once, rather than at its timeout. + */ + private function failUnanswered(string $sent, string $reason): void + { + $requestId = self::requestIdOf($sent); + if (null === $requestId || null === $this->state) { + return; + } + + $this->logger->warning('Response ended without answering the request', ['request_id' => $requestId, 'reason' => $reason]); + $this->state->storeResponse($requestId, Error::forInternalError('The response ended without answering the request: '.$reason, $requestId)->jsonSerialize()); } /** @@ -893,10 +928,11 @@ private function dispatchSseData(array $event): void private function isResponseForCurrentSseRequest(string $data): bool { - if (null === $this->sseRequestId || '' === $data) { - return false; - } + return null !== $this->sseRequestId && self::answers($data, $this->sseRequestId); + } + private static function answers(string $data, int|string $requestId): bool + { $decoded = json_decode($data, true); if (!\is_array($decoded)) { return false; @@ -904,7 +940,7 @@ private function isResponseForCurrentSseRequest(string $data): bool foreach (array_is_list($decoded) ? $decoded : [$decoded] as $message) { // One the protocol cannot read is dropped there, so the request is still waiting. - if (\is_array($message) && !isset($message['method']) && ($message['id'] ?? null) === $this->sseRequestId + if (\is_array($message) && !isset($message['method']) && ($message['id'] ?? null) === $requestId && self::isWellFormedAnswer($message)) { return true; } diff --git a/tests/Unit/Client/Transport/HttpTransportTest.php b/tests/Unit/Client/Transport/HttpTransportTest.php index 303ce2c6..3ed9af57 100644 --- a/tests/Unit/Client/Transport/HttpTransportTest.php +++ b/tests/Unit/Client/Transport/HttpTransportTest.php @@ -21,6 +21,7 @@ use Mcp\Exception\ConnectionException; use Mcp\Exception\InvalidArgumentException; use Mcp\Exception\RequestCancelledException; +use Mcp\Exception\RequestException; use Mcp\Exception\TimeoutException; use Mcp\Schema\ClientCapabilities; use Mcp\Schema\Enum\ProtocolVersion; @@ -1019,6 +1020,86 @@ public function isCancellationRequested(): bool $client->disconnect(); } + /** + * A null body is one whose read fails, like a connection reset. + * + * @return iterable + */ + public static function unansweredResponseProvider(): iterable + { + yield 'a truncated JSON body' => ['application/json', '{"jsonrpc":"2.0","id":%d,"result":{"cont']; + yield 'an empty JSON body' => ['application/json', '']; + yield 'a JSON body failing to read' => ['application/json', null]; + yield 'a malformed JSON answer' => ['application/json', '{"jsonrpc":"2.0","id":%d,"result":"done"}']; + yield 'a body that is no JSON' => ['text/html', 'Gateway says hi']; + yield 'an answer not sent as JSON' => ['text/plain', '{"jsonrpc":"2.0","id":%d,"result":{"content":[]}}']; + yield 'no content type and no body' => ['', '']; + } + + #[TestDox('an error status whose body fails to read still fails the call at once')] + public function testUnreadableErrorBodyFailsTheCallAtOnce(): void + { + $httpClient = new RecordingHttpClient(function (array $message): ?ResponseInterface { + if ('cut' !== ($message['params']['name'] ?? null)) { + return null; + } + + $stream = $this->createStub(StreamInterface::class); + $stream->method('getContents')->willThrowException(new \RuntimeException('Connection reset by peer')); + + return new Response(500, ['Content-Type' => 'application/json'], $stream); + }); + + $client = Client::builder()->setClientInfo('test', '1')->setProtocolVersion(ProtocolVersion::V2025_11_25)->setRequestTimeout(30)->build(); + $client->connect(new HttpTransport('http://localhost/mcp', [], $httpClient, $this->factory, $this->factory)); + + try { + $client->callTool('cut'); + $this->fail('Expected the call to fail.'); + } catch (RequestException $e) { + $this->assertStringContainsString('HTTP 500', $e->getMessage()); + } + + $this->assertSame('next call', $client->callTool('fast')->content[0]->text ?? null); + $client->disconnect(); + } + + #[DataProvider('unansweredResponseProvider')] + #[TestDox('a response that ends without the answer fails the call at once: $_dataName')] + public function testUnansweredResponseFailsTheCallAtOnce(string $contentType, ?string $body): void + { + $httpClient = new RecordingHttpClient(function (array $message) use ($contentType, $body): ?ResponseInterface { + if ('cut' !== ($message['params']['name'] ?? null)) { + return null; + } + + $headers = '' === $contentType ? [] : ['Content-Type' => $contentType]; + if (null !== $body) { + return new Response(200, $headers, \sprintf($body, $message['id'])); + } + + $stream = $this->createStub(StreamInterface::class); + $stream->method('getContents')->willThrowException(new \RuntimeException('Connection reset by peer')); + + return new Response(200, $headers, $stream); + }); + + $client = Client::builder()->setClientInfo('test', '1')->setProtocolVersion(ProtocolVersion::V2025_11_25)->setRequestTimeout(30)->build(); + $client->connect(new HttpTransport('http://localhost/mcp', [], $httpClient, $this->factory, $this->factory)); + + $started = microtime(true); + try { + $client->callTool('cut'); + $this->fail('Expected the call to fail.'); + } catch (RequestException $e) { + $this->assertStringContainsString('without answering the request', $e->getMessage()); + } + + $this->assertLessThan(1, microtime(true) - $started, 'the cut must not be waited out like a slow answer'); + $this->assertSame('next call', $client->callTool('fast')->content[0]->text ?? null); + $client->disconnect(); + } + /** @return iterable */ public static function revisionProvider(): iterable {