From b50d80faa9f99cec2b99245d5d50c9fd09d75ade Mon Sep 17 00:00:00 2001 From: Ousama Ben Younes Date: Mon, 12 Oct 2026 23:31:10 +0200 Subject: [PATCH 1/2] [Client] Report the stdio server's exit code and stderr when it dies Fixes #332. Co-authored-by: Christopher Hertel --- CHANGELOG.md | 1 + src/Client/Transport/StdioTransport.php | 79 ++++++++++++++++++- tests/Integration/IntegrationTestCase.php | 4 +- .../Client/Transport/StdioTransportTest.php | 34 +++++++- 4 files changed, 111 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8ad1f8c2..e6c8fb28 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -45,6 +45,7 @@ All notable changes to `mcp/sdk` will be documented in this file. * [BC Break] Bump the client's default protocol version to `2026-07-28`, so a client speaks both protocol eras unless told otherwise. Against a server that speaks both, a default client now settles on `2026-07-28`, where a server can no longer sample or list roots through `ClientGateway`; return those asks as an `InputRequiredResult` instead, or call `Builder::setProtocolVersion(ProtocolVersion::V2025_11_25)` on the client. * On a `2026-07-28` connection, `Client::setLoggingLevel()` stamps the level on every following request, `Client::ping()` sends `server/discover` and `Client::sendRootsListChanged()` sends nothing. * Fail a client request at once when the HTTP server refuses it with an error status or the stdio server process exits, instead of waiting out the timeout. +* Report the exit code and the end of stderr when the stdio server process dies, instead of only that it is no longer running. * [BC Break] Bump `MessageInterface::PROTOCOL_VERSION` to `2026-07-28`. Use `ProtocolVersion::latestHandshake()` where a handshake revision is needed, e.g. in an `initialize` answer. * Add `ServerRequestEvent` and `ClientResponseEvent` for handshake-era server-initiated requests (`elicitation/create`, `sampling/createMessage`) and the client's replies. * Dispatch `ResponseEvent` and `ErrorEvent` when a suspended Fiber completes. diff --git a/src/Client/Transport/StdioTransport.php b/src/Client/Transport/StdioTransport.php index 2108e64e..1c1cacba 100644 --- a/src/Client/Transport/StdioTransport.php +++ b/src/Client/Transport/StdioTransport.php @@ -49,11 +49,26 @@ class StdioTransport extends BaseTransport private string $inputBuffer = ''; + private string $stderrBuffer = ''; + + /** Why the server's output ended, worked out once: the exit code is only reported once. */ + private ?string $exitReason = null; + /** * Default cap on the bytes buffered while waiting for a complete line. */ public const DEFAULT_MAX_BUFFER_SIZE = 4 * 1024 * 1024; + /** + * Bytes of the most recent stderr kept to explain why the server stopped. + */ + public const MAX_STDERR_BUFFER_SIZE = 8 * 1024; + + /** + * How long a server that closed its output is given to exit, so its exit code can be reported. + */ + private const EXIT_GRACE_SECONDS = 0.1; + /** @var McpFiber|null */ private ?\Fiber $activeFiber = null; @@ -172,6 +187,12 @@ public function close(): void private function spawnProcess(): void { + // A transport may be respawned on a retry, so clear anything the + // previous process left behind. + $this->inputBuffer = ''; + $this->stderrBuffer = ''; + $this->exitReason = null; + $descriptors = [ 0 => ['pipe', 'r'], // stdin 1 => ['pipe', 'w'], // stdout @@ -246,7 +267,7 @@ private function processInput(): void // Only on an empty read, so an answer arriving with the end of output is taken first. if (('' === $data || false === $data) && \is_resource($this->stdout) && feof($this->stdout)) { - $this->failPending('The server process closed its output; it is no longer running.', [self::CONNECTION_LOST => true]); + $this->failPending($this->exitReason(), [self::CONNECTION_LOST => true]); } } @@ -339,15 +360,65 @@ private function processFiber(): void } } + /** + * Read what the server wrote to stderr, keeping its tail to explain a later failure. + */ private function processStderr(): void { if (null === $this->stderr || !\is_resource($this->stderr)) { return; } - $stderr = fread($this->stderr, 8192); - if (false !== $stderr && '' !== $stderr) { - $this->logger->debug('Server stderr', ['output' => trim($stderr)]); + while (false !== ($chunk = fread($this->stderr, 8192)) && '' !== $chunk) { + $this->logger->debug('Server stderr', ['output' => trim($chunk)]); + + $this->stderrBuffer = substr($this->stderrBuffer.$chunk, -self::MAX_STDERR_BUFFER_SIZE); + } + } + + /** + * Why the server's output ended: how the process exited, if it did, and the end of its stderr. + */ + private function exitReason(): string + { + if (null !== $this->exitReason) { + return $this->exitReason; + } + + $exitCode = $this->awaitExit(); + $this->processStderr(); + + $reason = null !== $exitCode + ? \sprintf('The server process exited with code %d', $exitCode) + : 'The server process closed its output; it is no longer running'; + + $stderr = trim($this->stderrBuffer); + + return $this->exitReason = '' !== $stderr ? $reason.': '.$stderr : $reason.'.'; + } + + /** + * Give a server that closed its output a moment to exit, which it usually does next. + * + * @return int|null the exit code, or null if it is still running + */ + private function awaitExit(): ?int + { + if (!\is_resource($this->process)) { + return null; } + + $deadline = microtime(true) + self::EXIT_GRACE_SECONDS; + + do { + $status = proc_get_status($this->process); + if (!$status['running']) { + return $status['exitcode']; + } + + usleep(1000); + } while (microtime(true) < $deadline); + + return null; } } diff --git a/tests/Integration/IntegrationTestCase.php b/tests/Integration/IntegrationTestCase.php index 05d05f35..09ddfcd3 100644 --- a/tests/Integration/IntegrationTestCase.php +++ b/tests/Integration/IntegrationTestCase.php @@ -70,8 +70,8 @@ protected function connect(string $fixture, ?ClientBuilder $client = null, array try { $this->client->connect($this->transport($fixture, $env)); } catch (ConnectionException $e) { - // The transport discards the child's stderr, so a fixture dying on - // startup arrives here as a bare timeout. + // A fixture dying on startup arrives here with its exit code and the + // end of its stderr, usually enough to see why. $this->fail(\sprintf('Could not connect to fixture server "%s": %s. Run `%s %s` to see why.', $fixture, $e->getMessage(), \PHP_BINARY, self::script($fixture))); } diff --git a/tests/Unit/Client/Transport/StdioTransportTest.php b/tests/Unit/Client/Transport/StdioTransportTest.php index 5b85a05e..49e1a3db 100644 --- a/tests/Unit/Client/Transport/StdioTransportTest.php +++ b/tests/Unit/Client/Transport/StdioTransportTest.php @@ -159,12 +159,44 @@ public function testExitedServerFailsTheConnection(): void $client->connect(new StdioTransport(command: \PHP_BINARY, args: ['-r', 'exit(1);'])); $this->fail('Connecting to a server that exits must fail.'); } catch (ConnectionException $e) { - $this->assertStringContainsString('no longer running', $e->getMessage()); + $this->assertStringContainsString('exited with code 1', $e->getMessage()); } $this->assertLessThan(5, microtime(true) - $started); } + #[TestDox('a server that dies tells why: its exit code and the end of its stderr')] + public function testExitedServerReportsExitCodeAndStderr(): void + { + $client = Client::builder()->setInitTimeout(10)->setMaxRetries(0)->build(); + + try { + $client->connect(new StdioTransport(command: \PHP_BINARY, args: ['-r', 'fwrite(STDERR, str_repeat("x", 9000)."Fatal: missing extension\n"); exit(7);'])); + $this->fail('Connecting to a server that exits must fail.'); + } catch (ConnectionException $e) { + $this->assertStringContainsString('exited with code 7: ', $e->getMessage()); + $this->assertStringEndsWith('Fatal: missing extension', $e->getMessage()); + $this->assertLessThan(StdioTransport::MAX_STDERR_BUFFER_SIZE + 100, \strlen($e->getMessage()), 'only the tail of stderr is kept'); + } + } + + #[TestDox('a retried connection reports how its own process ended, not the previous one')] + public function testRetryReportsItsOwnProcess(): void + { + $counter = tempnam(sys_get_temp_dir(), 'mcp-stdio-'); + $client = Client::builder()->setInitTimeout(10)->setMaxRetries(1)->build(); + + try { + $client->connect(new StdioTransport(command: \PHP_BINARY, args: ['-r', '$n = (int) file_get_contents($argv[1]) + 1; file_put_contents($argv[1], $n); fwrite(STDERR, "run $n"); exit(10 + $n);', $counter])); + $this->fail('Connecting to a server that exits must fail.'); + } catch (ConnectionException $e) { + $this->assertStringContainsString('exited with code 12: run 2', $e->getMessage()); + $this->assertStringNotContainsString('run 1', $e->getMessage()); + } finally { + unlink($counter); + } + } + #[TestDox('the buffer cap must be a positive number of bytes')] public function testRejectsNonPositiveCap(): void { From be404efb61ab3211b254c90bd841c5a463ffd074 Mon Sep 17 00:00:00 2001 From: Christopher Hertel Date: Tue, 13 Oct 2026 00:03:28 +0200 Subject: [PATCH 2/2] [Client] Harden stdio exit reporting --- src/Client/Transport/StdioTransport.php | 36 +++++++--- tests/Unit/Client/ProtocolTest.php | 4 +- .../Client/Transport/StdioTransportTest.php | 69 ++++++++++++++++++- 3 files changed, 97 insertions(+), 12 deletions(-) diff --git a/src/Client/Transport/StdioTransport.php b/src/Client/Transport/StdioTransport.php index 1c1cacba..5da7762a 100644 --- a/src/Client/Transport/StdioTransport.php +++ b/src/Client/Transport/StdioTransport.php @@ -51,7 +51,11 @@ class StdioTransport extends BaseTransport private string $stderrBuffer = ''; - /** Why the server's output ended, worked out once: the exit code is only reported once. */ + /** + * Why the server's output ended, kept because every later tick sees the end of output + * again and would wait out the exit grace each time, and because before PHP 8.3 + * proc_get_status() reports the exit code only on its first call after the exit. + */ private ?string $exitReason = null; /** @@ -69,6 +73,11 @@ class StdioTransport extends BaseTransport */ private const EXIT_GRACE_SECONDS = 0.1; + /** + * Bytes of stderr read in one go at most: a pipe's usual capacity, so what an exited server left behind is read whole. + */ + private const MAX_STDERR_READ = 64 * 1024; + /** @var McpFiber|null */ private ?\Fiber $activeFiber = null; @@ -369,7 +378,11 @@ private function processStderr(): void return; } - while (false !== ($chunk = fread($this->stderr, 8192)) && '' !== $chunk) { + // A server that never stops writing to stderr must not keep the loop + // from reading its answers and enforcing timeouts. + $read = 0; + while ($read < self::MAX_STDERR_READ && false !== ($chunk = fread($this->stderr, 8192)) && '' !== $chunk) { + $read += \strlen($chunk); $this->logger->debug('Server stderr', ['output' => trim($chunk)]); $this->stderrBuffer = substr($this->stderrBuffer.$chunk, -self::MAX_STDERR_BUFFER_SIZE); @@ -385,12 +398,12 @@ private function exitReason(): string return $this->exitReason; } - $exitCode = $this->awaitExit(); + $ending = $this->awaitExit(); $this->processStderr(); - $reason = null !== $exitCode - ? \sprintf('The server process exited with code %d', $exitCode) - : 'The server process closed its output; it is no longer running'; + $reason = null !== $ending + ? 'The server process '.$ending + : 'The server process closed its output'; $stderr = trim($this->stderrBuffer); @@ -400,9 +413,9 @@ private function exitReason(): string /** * Give a server that closed its output a moment to exit, which it usually does next. * - * @return int|null the exit code, or null if it is still running + * @return string|null how the process ended, or null if it is still running */ - private function awaitExit(): ?int + private function awaitExit(): ?string { if (!\is_resource($this->process)) { return null; @@ -413,7 +426,12 @@ private function awaitExit(): ?int do { $status = proc_get_status($this->process); if (!$status['running']) { - return $status['exitcode']; + // A process ended by a signal has no exit code; PHP reports -1 for it. + if ($status['signaled']) { + return \sprintf('was killed by signal %d', $status['termsig']); + } + + return $status['exitcode'] >= 0 ? \sprintf('exited with code %d', $status['exitcode']) : 'exited'; } usleep(1000); diff --git a/tests/Unit/Client/ProtocolTest.php b/tests/Unit/Client/ProtocolTest.php index ece4b405..b0feecfd 100644 --- a/tests/Unit/Client/ProtocolTest.php +++ b/tests/Unit/Client/ProtocolTest.php @@ -143,12 +143,12 @@ public function testLostConnectionDuringProbeDoesNotFallBack(): void $fiber = new \Fiber(static fn () => $protocol->initialize($config)); $suspended = $fiber->start(); - $fiber->resume(new Error($suspended['request_id'], Error::INTERNAL_ERROR, 'The server process closed its output; it is no longer running.', [TransportInterface::CONNECTION_LOST => true])); + $fiber->resume(new Error($suspended['request_id'], Error::INTERNAL_ERROR, 'The server process closed its output.', [TransportInterface::CONNECTION_LOST => true])); $this->assertTrue($fiber->isTerminated()); $error = $fiber->getReturn(); $this->assertInstanceOf(Error::class, $error); - $this->assertStringContainsString('no longer running', $error->message); + $this->assertSame('The server process closed its output.', $error->message); $this->assertSame(['server/discover'], $transport->methods); } diff --git a/tests/Unit/Client/Transport/StdioTransportTest.php b/tests/Unit/Client/Transport/StdioTransportTest.php index 49e1a3db..30ca0b4d 100644 --- a/tests/Unit/Client/Transport/StdioTransportTest.php +++ b/tests/Unit/Client/Transport/StdioTransportTest.php @@ -95,7 +95,7 @@ public function testClosedOutputFailsPendingRequests(): void $response = $state->consumeResponse(1); $this->assertInstanceOf(Error::class, $response); - $this->assertStringContainsString('no longer running', $response->message); + $this->assertSame('The server process closed its output.', $response->message); $this->assertSame([TransportInterface::CONNECTION_LOST => true], $response->data); } @@ -180,6 +180,34 @@ public function testExitedServerReportsExitCodeAndStderr(): void } } + #[TestDox('a server killed by a signal says so, instead of an exit code it never had')] + public function testKilledServerReportsTheSignal(): void + { + // Where /bin/sh forks the server instead of becoming it, the signal hits + // the shell, so the server lets go of the output itself. + $transport = new StdioTransport(command: \PHP_BINARY, args: ['-r', 'fclose(STDOUT); usleep(500000);']); + $state = new ClientState(); + $transport->setState($state); + $state->addPendingRequest(1, 120); + + (new \ReflectionMethod($transport, 'spawnProcess'))->invoke($transport); + + try { + proc_terminate($this->readPrivate($transport, 'process'), 9); + + $deadline = microtime(true) + 5; + while (null === ($response = $state->consumeResponse(1)) && microtime(true) < $deadline) { + $this->invokeProcessInput($transport); + usleep(1000); + } + } finally { + $transport->close(); + } + + $this->assertInstanceOf(Error::class, $response); + $this->assertStringStartsWith('The server process was killed by signal 9', $response->message); + } + #[TestDox('a retried connection reports how its own process ended, not the previous one')] public function testRetryReportsItsOwnProcess(): void { @@ -197,6 +225,45 @@ public function testRetryReportsItsOwnProcess(): void } } + #[TestDox('a server that never stops writing to stderr does not hold up the transport')] + public function testEndlessStderrDoesNotBlock(): void + { + $endless = new class { + /** @var resource|null */ + public $context; + + public function stream_open(string $path, string $mode, int $options, ?string &$openedPath): bool + { + return true; + } + + public function stream_read(int $count): string + { + return str_repeat('x', $count); + } + + public function stream_eof(): bool + { + return false; + } + }; + + stream_wrapper_register('mcp-endless', $endless::class); + + try { + $transport = new StdioTransport(command: 'true'); + $stream = fopen('mcp-endless://', 'r'); + $this->assertNotFalse($stream); + (new \ReflectionProperty($transport, 'stderr'))->setValue($transport, $stream); + + (new \ReflectionMethod($transport, 'processStderr'))->invoke($transport); + + $this->assertSame(StdioTransport::MAX_STDERR_BUFFER_SIZE, \strlen($this->readPrivate($transport, 'stderrBuffer'))); + } finally { + stream_wrapper_unregister('mcp-endless'); + } + } + #[TestDox('the buffer cap must be a positive number of bytes')] public function testRejectsNonPositiveCap(): void {