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 */