From 7d6186ff46b63cdcc00f1c237752994ba27e742b Mon Sep 17 00:00:00 2001 From: Ousama Ben Younes Date: Sat, 26 Sep 2026 23:27:08 +0000 Subject: [PATCH] [Client] Fail fast when the stdio server process exits before responding When a StdioTransport server process dies before answering, connect() spun until the init timeout and reported a generic "Request timed out", while the child's stderr was logged at debug level and discarded. Detect the dead child in processFiber() and resume the waiting fiber at once with the exit code and a bounded tail of its stderr. Drain stdout fully within a tick so a response arriving just before exit still wins, and reset the per-connection state on respawn so a retried connection starts clean. --- CHANGELOG.md | 1 + src/Client/Transport/StdioTransport.php | 106 ++++++++++++- tests/Integration/IntegrationTestCase.php | 4 +- .../Client/Transport/StdioTransportTest.php | 144 ++++++++++++++++++ 4 files changed, 248 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e77f0621..1b5b1ad7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,7 @@ All notable changes to `mcp/sdk` will be documented in this file. * [BC Break] Remove the `providerClass` argument of `#[CompletionProvider]`. Use `provider:`, which takes the same class-string and is now the first positional argument. * Add `HttpTransport::getSessionId()` to read the server-minted `Mcp-Session-Id`: a request-scoped caller can persist it and pass it back through the constructor's `$headers` on a later transport. Always `null` on `2026-07-28`, which removed protocol-level sessions. * Fix OIDC discovery rejecting issuers with a trailing slash (e.g. Authentik, Auth0). +* Fail a `StdioTransport` connection immediately when the server process exits before responding, reporting its exit code and a bounded tail of its stderr instead of waiting out the request timeout with a generic "Request timed out". 0.8.0 ----- diff --git a/src/Client/Transport/StdioTransport.php b/src/Client/Transport/StdioTransport.php index f1029619..9866e763 100644 --- a/src/Client/Transport/StdioTransport.php +++ b/src/Client/Transport/StdioTransport.php @@ -46,11 +46,20 @@ class StdioTransport extends BaseTransport private string $inputBuffer = ''; + private string $stderrBuffer = ''; + + private ?int $processExitCode = 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 a failed start. + */ + public const MAX_STDERR_BUFFER_SIZE = 8 * 1024; + /** @var McpFiber|null */ private ?\Fiber $activeFiber = null; @@ -164,6 +173,13 @@ public function close(): void private function spawnProcess(): void { + // A transport may be respawned on a retry, so clear anything the + // previous process left behind — otherwise a fresh child would inherit + // the old one's exit code and stderr. + $this->processExitCode = null; + $this->stderrBuffer = ''; + $this->inputBuffer = ''; + $descriptors = [ 0 => ['pipe', 'r'], // stdin 1 => ['pipe', 'w'], // stdout @@ -238,8 +254,13 @@ private function processInput(): void return; } - $data = fread($this->stdout, 8192); - if (false !== $data && '' !== $data) { + // Drain everything currently available, not just one 8 KiB chunk, so a + // response larger than the read size is fully read within the tick. + // Otherwise a process that emits a big frame and exits could be seen as + // dead by processFiber() before its response has finished arriving. + // Complete frames are dispatched after each chunk so the buffer cap + // still bounds a single unterminated frame, not a burst of whole ones. + while (false !== ($data = fread($this->stdout, 8192)) && '' !== $data) { if (\strlen($this->inputBuffer) + \strlen($data) > $this->maxBufferSize) { $this->abortInput(\sprintf('buffered %d bytes without a newline, exceeding the %d byte limit', \strlen($this->inputBuffer) + \strlen($data), $this->maxBufferSize)); @@ -247,8 +268,16 @@ private function processInput(): void } $this->inputBuffer .= $data; + $this->dispatchCompleteFrames(); } + } + /** + * Hand every newline-delimited frame currently in the buffer to the message + * handler, leaving any trailing partial frame behind. + */ + private function dispatchCompleteFrames(): void + { while (false !== ($pos = strpos($this->inputBuffer, "\n"))) { $line = substr($this->inputBuffer, 0, $pos); $this->inputBuffer = substr($this->inputBuffer, $pos + 1); @@ -314,6 +343,24 @@ private function processFiber(): void return; } + // Fail fast if the server process is already gone: no response can + // arrive from a dead child, so waiting out the timeout only hides + // why it died. + $exitCode = $this->checkProcessExit(); + if (null !== $exitCode) { + $this->logger->warning('Server process exited before responding', [ + 'request_id' => $requestId, + 'exit_code' => $exitCode, + ]); + // A dead child will never answer this request, so drop it: a + // reused transport must not carry it into the next attempt. + $this->state->removePendingRequest($requestId); + $error = Error::forInternalError($this->processExitMessage($exitCode), $requestId); + $this->activeFiber->resume($error); + + return; + } + // Check timeout if (time() - $timestamp >= $timeout) { $this->logger->warning('Request timed out', ['request_id' => $requestId]); @@ -326,14 +373,63 @@ private function processFiber(): void } private function processStderr(): void + { + $this->drainStderr(); + } + + /** + * Read whatever stderr is currently available, log it, and keep a bounded + * tail so a failed start can be explained. Non-blocking, so it returns as + * soon as the pipe is drained. + */ + private function drainStderr(): 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 .= $chunk; + if (\strlen($this->stderrBuffer) > self::MAX_STDERR_BUFFER_SIZE) { + $this->stderrBuffer = substr($this->stderrBuffer, -self::MAX_STDERR_BUFFER_SIZE); + } + } + } + + /** + * Return the child's exit code once it has terminated, or null while it is + * still running. proc_get_status() only reports a real exit code the first + * time it is called after the process ends, so it is captured and cached + * here, together with the final stderr. + */ + private function checkProcessExit(): ?int + { + if (null !== $this->processExitCode) { + return $this->processExitCode; + } + + if (null === $this->process || !\is_resource($this->process)) { + return null; + } + + $status = proc_get_status($this->process); + if ($status['running']) { + return null; } + + $this->drainStderr(); + + return $this->processExitCode = $status['exitcode']; + } + + private function processExitMessage(int $exitCode): string + { + $message = \sprintf('Server process exited with code %d before responding', $exitCode); + + $stderr = trim($this->stderrBuffer); + + return '' !== $stderr ? $message.': '.$stderr : $message.'.'; } } diff --git a/tests/Integration/IntegrationTestCase.php b/tests/Integration/IntegrationTestCase.php index 4ad33d42..7d17fdb8 100644 --- a/tests/Integration/IntegrationTestCase.php +++ b/tests/Integration/IntegrationTestCase.php @@ -60,8 +60,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 surfaces here with its exit code and + // stderr, so the message below is 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 fb314083..c39161fb 100644 --- a/tests/Unit/Client/Transport/StdioTransportTest.php +++ b/tests/Unit/Client/Transport/StdioTransportTest.php @@ -15,6 +15,7 @@ use Mcp\Client\Transport\StdioTransport; use Mcp\Exception\InvalidArgumentException; use Mcp\Schema\JsonRpc\Error; +use Mcp\Schema\JsonRpc\Response; use PHPUnit\Framework\Attributes\TestDox; use PHPUnit\Framework\TestCase; @@ -78,6 +79,149 @@ public function testRejectsNonPositiveCap(): void new StdioTransport(command: 'true', maxBufferSize: 0); } + #[TestDox('a child that exits before responding fails the request fast with its exit code and stderr')] + public function testProcessExitBeforeResponseFailsFast(): void + { + $transport = new StdioTransport(command: \PHP_BINARY, args: ['-r', 'fwrite(\STDERR, "boot failure detail"); exit(7);']); + $state = new ClientState(); + $state->addPendingRequest(1, 30); + $transport->setState($state); + + $fiber = $this->suspendedFiber(); + $this->setPrivate($transport, 'activeFiber', $fiber); + + $this->invokeSpawn($transport); + // Pumps processFiber() alone; it captures the final stderr itself, so + // the assertions do not depend on a prior processStderr() tick. + $this->pumpUntilResolved($transport, $fiber); + + $this->assertTrue($fiber->isTerminated(), 'the waiting fiber must be resolved once the process is gone'); + $error = $fiber->getReturn(); + $this->assertInstanceOf(Error::class, $error); + $this->assertSame(Error::INTERNAL_ERROR, $error->code); + $this->assertSame(1, $error->id); + $this->assertStringContainsString('code 7', $error->message, 'the exit code belongs in the message'); + $this->assertStringContainsString('boot failure detail', $error->message, 'captured stderr belongs in the message'); + + $transport->close(); + } + + #[TestDox('a response that arrived before the process exited still wins over the exit error')] + public function testResponseBeforeExitIsNotOverwritten(): void + { + $transport = new StdioTransport(command: \PHP_BINARY, args: ['-r', 'exit(0);']); + $state = new ClientState(); + $state->addPendingRequest(1, 30); + $state->storeResponse(1, ['jsonrpc' => '2.0', 'id' => 1, 'result' => ['ok' => true]]); + $transport->setState($state); + + $fiber = $this->suspendedFiber(); + $this->setPrivate($transport, 'activeFiber', $fiber); + + $this->invokeSpawn($transport); + $this->pumpUntilResolved($transport, $fiber); + + $this->assertTrue($fiber->isTerminated()); + $this->assertInstanceOf(Response::class, $fiber->getReturn(), 'the buffered response must not be replaced by the exit error'); + + $transport->close(); + } + + #[TestDox('a response larger than one read that arrives just before the process exits is not clobbered')] + public function testLargeResponseArrivingBeforeExitIsNotClobbered(): void + { + // 9000 zero bytes on one line: larger than the 8 KiB read size, so the + // frame only completes if stdout is fully drained within the tick. + $transport = new StdioTransport(command: \PHP_BINARY, args: ['-r', 'echo str_repeat("0", 9000), "\n"; exit(0);']); + $state = new ClientState(); + $state->addPendingRequest(1, 30); + $transport->setState($state); + + // Stand in for the session: a delivered frame becomes the stored response. + $transport->onMessage(static function (string $line) use ($state): void { + if (\strlen($line) > 8192) { + $state->storeResponse(1, ['jsonrpc' => '2.0', 'id' => 1, 'result' => ['ok' => true]]); + } + }); + + $fiber = $this->suspendedFiber(); + $this->setPrivate($transport, 'activeFiber', $fiber); + + $this->invokeSpawn($transport); + $this->pumpTicksUntilResolved($transport, $fiber); + + $this->assertTrue($fiber->isTerminated()); + $this->assertInstanceOf(Response::class, $fiber->getReturn(), 'a fully-arrived response must win over the exit error'); + + $transport->close(); + } + + /** + * A fiber that suspends once and returns whatever value resumes it — a + * stand-in for a request fiber awaiting its response. + * + * @return \Fiber + */ + private function suspendedFiber(): \Fiber + { + $fiber = new \Fiber(static fn () => \Fiber::suspend()); + $fiber->start(); + + return $fiber; + } + + private function invokeSpawn(StdioTransport $transport): void + { + (new \ReflectionMethod($transport, 'spawnProcess'))->invoke($transport); + } + + private function invokeProcessFiber(StdioTransport $transport): void + { + (new \ReflectionMethod($transport, 'processFiber'))->invoke($transport); + } + + private function invokeTick(StdioTransport $transport): void + { + (new \ReflectionMethod($transport, 'tick'))->invoke($transport); + } + + private function setPrivate(StdioTransport $transport, string $property, mixed $value): void + { + (new \ReflectionProperty($transport, $property))->setValue($transport, $value); + } + + /** + * Drive processFiber() until it resolves the waiting fiber or a deadline + * elapses. The transport itself makes the first proc_get_status() call + * after the child exits, so the exit code stays readable for the code + * under test. + * + * @param \Fiber $fiber + */ + private function pumpUntilResolved(StdioTransport $transport, \Fiber $fiber): void + { + $deadline = microtime(true) + 2.0; + while (!$fiber->isTerminated() && microtime(true) < $deadline) { + $this->invokeProcessFiber($transport); + usleep(2000); + } + } + + /** + * Like {@see pumpUntilResolved()} but drives the full tick(), so stdout is + * read and parsed before the exit check runs. + * + * @param \Fiber $fiber + */ + private function pumpTicksUntilResolved(StdioTransport $transport, \Fiber $fiber): void + { + $deadline = microtime(true) + 2.0; + while (!$fiber->isTerminated() && microtime(true) < $deadline) { + $this->invokeTick($transport); + usleep(2000); + } + } + /** * @return resource */