diff --git a/CHANGELOG.md b/CHANGELOG.md index a671720a..82f799b6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,8 @@ All notable changes to `mcp/sdk` will be documented in this file. * Fix memory growing with every `tools/call` in long-lived processes like FrankenPHP workers or stdio servers. * Fix OIDC discovery rejecting issuers with a trailing slash (e.g. Authentik, Auth0). * Fix `Client::connect()` rejecting an empty `serverInfo.version`, as the Python SDK sends, and leaving the transport open when initialization fails on a malformed result: the transport is now closed and the failure wrapped in a `ConnectionException`. +* Fix parallel elicitations on one session getting each other's answers: requests to the client get UUID ids instead of a session counter, and a request or notification a handler sends goes out on the stream of its own call. +* [BC Break] Ids of requests to the client are UUIDs: `Protocol::sendRequest()`, `Protocol::checkResponse()` and `Protocol::handleFiberYield()` take and return a `Uuid` instead of an `int`, as does the response finder of a transport. On the wire and in the session they are RFC 4122 strings. * Fix stateless SSE streams holding back frames until close when PHP output buffering is enabled. * Fix a session deleted while one of its requests still ran coming back when that request saved it. * Reject a recognized `Mcp-Param-*` header whose mirrored argument is absent from the body with `-32020`, instead of accepting the request (SEP-2243). diff --git a/src/Server/Protocol.php b/src/Server/Protocol.php index 90f37bd7..0941f119 100644 --- a/src/Server/Protocol.php +++ b/src/Server/Protocol.php @@ -53,9 +53,6 @@ */ class Protocol { - /** Session key for request ID counter */ - private const SESSION_REQUEST_ID_COUNTER = '_mcp.request_id_counter'; - /** Session key for pending outgoing requests */ private const SESSION_PENDING_REQUESTS = '_mcp.pending_requests'; @@ -80,10 +77,18 @@ class Protocol * The client request each suspended fiber waits on. Pending requests are stored in the session, * which concurrent calls share, so a fiber must only be resumed with the answer to the one it sent. * - * @var \WeakMap + * @var \WeakMap */ private \WeakMap $awaitedRequestIds; + /** + * What each transport's fiber sends the client, kept out of the session for the same reason: + * a request or notification must go out on the stream of the call that produced it. + * + * @var \WeakMap, list}>> + */ + private \WeakMap $fiberOutgoingMessages; + /** * @param array>> $requestHandlers * @param array $notificationHandlers @@ -99,6 +104,7 @@ public function __construct( private readonly ?RequestStateCodec $requestStateCodec = null, ) { $this->awaitedRequestIds = new \WeakMap(); + $this->fiberOutgoingMessages = new \WeakMap(); } /** @@ -114,14 +120,20 @@ public function connect(TransportInterface $transport): void $transport->onSessionEnd($this->destroySession(...)); - $transport->setOutgoingMessagesProvider($this->consumeOutgoingMessages(...)); + // The transport keeps these callbacks, so they reference it weakly to not keep it alive. + $transportRef = \WeakReference::create($transport); + + $transport->setOutgoingMessagesProvider(fn (Uuid $sessionId): array => [ + ...$this->takeFiberOutgoingMessages($transportRef->get()), + ...$this->consumeOutgoingMessages($sessionId), + ]); $transport->setPendingRequestsProvider(fn (Uuid $sessionId, \Fiber $fiber): array => $this->getAwaitedPendingRequests($fiber, $sessionId)); $transport->setResponseFinder($this->checkResponse(...)); - $transport->setFiberYieldHandler(function (mixed $yieldedValue, ?Uuid $sessionId, \Fiber $fiber): void { - $this->trackAwaitedRequest($fiber, $this->handleFiberYield($yieldedValue, $sessionId)); + $transport->setFiberYieldHandler(function (mixed $yieldedValue, ?Uuid $sessionId, \Fiber $fiber) use ($transportRef): void { + $this->trackAwaitedRequest($fiber, $this->handleFiberYield($yieldedValue, $sessionId, $transportRef->get())); }); $this->logger->info('Protocol connected to transport', ['transport' => $transport::class]); @@ -355,10 +367,12 @@ private function handleRequest(TransportInterface $transport, Request $request, $beforeSuspension = $session->all(); $awaitedRequestId = null; + $outgoing = null; if ($result instanceof NotificationSuspension) { - $this->sendNotification($result->notification, $session); + $outgoing = $result->notification; } elseif ($result instanceof RequestSuspension) { - $awaitedRequestId = $this->sendRequest($result->request, $result->timeout, $session); + $awaitedRequestId = $this->registerRequest($result->request, $result->timeout, $session); + $outgoing = $result->request->withId($awaitedRequestId->toRfc4122()); } // The transport resumes the fiber from what the session holds: it must @@ -381,6 +395,10 @@ private function handleRequest(TransportInterface $transport, Request $request, $this->trackAwaitedRequest($fiber, $awaitedRequestId); $transport->attachFiberToSession($fiber, $session->getId()); + if (null !== $outgoing) { + $this->queueFiberOutgoing($transport, $outgoing); + } + return; } @@ -416,8 +434,8 @@ private function handleResponse(Response|Error $response, SessionInterface $sess return; } - // Request IDs are ints: a string ID like "1000" would otherwise match through PHP's key coercion. - $pending = \is_int($messageId) ? $session->get(self::SESSION_PENDING_REQUESTS, [])[$messageId] ?? null : null; + // Request IDs are UUIDs: any other ID cannot be one the server sent. + $pending = \is_string($messageId) && Uuid::isValid($messageId) ? $session->get(self::SESSION_PENDING_REQUESTS, [])[$messageId] ?? null : null; if (!\is_array($pending) || $this->hasTimedOut($pending)) { $this->logger->warning('Received a client response for an unknown or timed out request ID.', ['message_id' => $messageId]); @@ -457,30 +475,40 @@ private function handleNotification(Notification $notification, SessionInterface /** * Sends a request to the client and returns the request ID. */ - public function sendRequest(Request $request, int $timeout, SessionInterface $session): int + public function sendRequest(Request $request, int $timeout, SessionInterface $session): Uuid { - $counter = $session->get(self::SESSION_REQUEST_ID_COUNTER, 1000); - $requestId = $counter++; - $session->set(self::SESSION_REQUEST_ID_COUNTER, $counter); + $requestId = $this->registerRequest($request, $timeout, $session); - $requestWithId = $request->withId($requestId); + $this->queueOutgoing($request->withId($requestId->toRfc4122()), ['type' => 'request'], $session); - $this->dispatchEvent(new ServerRequestEvent($requestWithId, $timeout, $session)); + return $requestId; + } + + /** + * Picks the id of a request to the client and stores the request as pending in the session. + * + * The id is a UUID, not counted in the session: concurrent requests of a client load + * the session at the same time and would count up to the same id. + */ + private function registerRequest(Request $request, int $timeout, SessionInterface $session): Uuid + { + $requestId = Uuid::v4(); + $id = $requestId->toRfc4122(); + + $this->dispatchEvent(new ServerRequestEvent($request->withId($id), $timeout, $session)); $this->logger->info('Queueing server request to client', [ - 'request_id' => $requestId, + 'request_id' => $id, 'method' => $request::getMethod(), ]); // Per entry, so saving does not write back what other streams changed in the others. - $session->set(self::SESSION_PENDING_REQUESTS.".{$requestId}", [ - 'request_id' => $requestId, + $session->set(self::SESSION_PENDING_REQUESTS.".{$id}", [ + 'request_id' => $id, 'timeout' => $timeout, 'timestamp' => time(), ]); - $this->queueOutgoing($requestWithId, ['type' => 'request'], $session); - return $requestId; } @@ -544,6 +572,55 @@ private function sendResponse(TransportInterface $transport, Response|Error $res * @param array $context */ private function queueOutgoing(Request|Notification $message, array $context, SessionInterface $session): void + { + if (null === $outgoing = $this->encodeOutgoing($message, $context)) { + return; + } + + $queue = $session->get(self::SESSION_OUTGOING_QUEUE, []); + $queue[] = $outgoing; + $session->set(self::SESSION_OUTGOING_QUEUE, $queue); + } + + /** + * Queues what a transport's fiber sends the client, for that transport alone. + * + * @param TransportInterface $transport + */ + private function queueFiberOutgoing(TransportInterface $transport, Request|Notification $message): void + { + if (null === $outgoing = $this->encodeOutgoing($message, ['type' => $message instanceof Request ? 'request' : 'notification'])) { + return; + } + + $queue = $this->fiberOutgoingMessages[$transport] ?? []; + $queue[] = $outgoing; + $this->fiberOutgoingMessages[$transport] = $queue; + } + + /** + * @param TransportInterface|null $transport + * + * @return list}> + */ + private function takeFiberOutgoingMessages(?TransportInterface $transport): array + { + if (null === $transport || !isset($this->fiberOutgoingMessages[$transport])) { + return []; + } + + $queue = $this->fiberOutgoingMessages[$transport]; + unset($this->fiberOutgoingMessages[$transport]); + + return $queue; + } + + /** + * @param array $context + * + * @return array{message: string, context: array}|null + */ + private function encodeOutgoing(Request|Notification $message, array $context): ?array { try { $encoded = json_encode($message, \JSON_THROW_ON_ERROR); @@ -552,15 +629,13 @@ private function queueOutgoing(Request|Notification $message, array $context, Se 'exception' => $e, ]); - return; + return null; } - $queue = $session->get(self::SESSION_OUTGOING_QUEUE, []); - $queue[] = [ + return [ 'message' => $encoded, 'context' => $context, ]; - $session->set(self::SESSION_OUTGOING_QUEUE, $queue); } /** @@ -592,24 +667,25 @@ public function consumeOutgoingMessages(Uuid $sessionId): array * * @return Response>|Error|null */ - public function checkResponse(int $requestId, Uuid $sessionId): Response|Error|null + public function checkResponse(Uuid $requestId, Uuid $sessionId): Response|Error|null { + $id = $requestId->toRfc4122(); $session = $this->sessionManager->createWithId($sessionId); - $responseData = $session->get(self::SESSION_RESPONSES.".{$requestId}"); + $responseData = $session->get(self::SESSION_RESPONSES.".{$id}"); if (null === $responseData) { - $pending = $session->get(self::SESSION_PENDING_REQUESTS, [])[$requestId] ?? null; + $pending = $session->get(self::SESSION_PENDING_REQUESTS, [])[$id] ?? null; return \is_array($pending) && $this->hasTimedOut($pending) ? $this->expireRequest($requestId, $sessionId) : null; } $this->logger->debug('Found and consuming client response.', [ - 'request_id' => $requestId, + 'request_id' => $id, 'session_id' => $sessionId->toRfc4122(), ]); - $session->set(self::SESSION_RESPONSES.".{$requestId}", null); - $session->forget(self::SESSION_PENDING_REQUESTS.".{$requestId}"); + $session->set(self::SESSION_RESPONSES.".{$id}", null); + $session->forget(self::SESSION_PENDING_REQUESTS.".{$id}"); $session->save(); try { @@ -620,7 +696,7 @@ public function checkResponse(int $requestId, Uuid $sessionId): Response|Error|n return Response::fromArray($responseData); } catch (\Throwable $e) { $this->logger->error('Failed to reconstruct client response from session.', [ - 'request_id' => $requestId, + 'request_id' => $id, 'exception' => $e, 'response_data' => $responseData, ]); @@ -635,25 +711,26 @@ public function checkResponse(int $requestId, Uuid $sessionId): Response|Error|n * The session is loaded again as the caller's copy may be as old as the last * poll, and saving it back would undo what other streams stored since. */ - private function expireRequest(int $requestId, Uuid $sessionId): ?Error + private function expireRequest(Uuid $requestId, Uuid $sessionId): ?Error { + $id = $requestId->toRfc4122(); $session = $this->sessionManager->createWithId($sessionId); $pending = $session->get(self::SESSION_PENDING_REQUESTS, []); // Answered or expired by another stream in the meantime: the next poll sorts it out. - if (!isset($pending[$requestId]) || null !== $session->get(self::SESSION_RESPONSES.".{$requestId}")) { + if (!isset($pending[$id]) || null !== $session->get(self::SESSION_RESPONSES.".{$id}")) { return null; } $this->logger->warning('Client request timed out.', [ - 'request_id' => $requestId, + 'request_id' => $id, 'session_id' => $sessionId->toRfc4122(), ]); - $session->forget(self::SESSION_PENDING_REQUESTS.".{$requestId}"); + $session->forget(self::SESSION_PENDING_REQUESTS.".{$id}"); $session->save(); - return Error::forInternalError('Request timed out', $requestId); + return Error::forInternalError('Request timed out', $id); } /** @@ -667,7 +744,7 @@ private function hasTimedOut(array $pending): bool /** * Get pending requests for a session. * - * @return array The pending requests + * @return array The pending requests */ public function getPendingRequests(Uuid $sessionId): array { @@ -679,11 +756,15 @@ public function getPendingRequests(Uuid $sessionId): array /** * Handle values yielded by Fibers during transport-managed resumes. * - * @param FiberSuspend|null $yieldedValue + * With the transport the fiber runs on, what it sends goes out on that transport alone; + * without one, it is queued for whichever stream of the session asks first. * - * @return int|null the ID of the request sent to the client, which the fiber now waits on + * @param FiberSuspend|null $yieldedValue + * @param TransportInterface|null $transport + * + * @return Uuid|null the ID of the request sent to the client, which the fiber now waits on */ - public function handleFiberYield(mixed $yieldedValue, ?Uuid $sessionId): ?int + public function handleFiberYield(mixed $yieldedValue, ?Uuid $sessionId, ?TransportInterface $transport = null): ?Uuid { if (!$sessionId) { $this->logger->warning('Fiber yielded value without associated session context.'); @@ -718,23 +799,43 @@ public function handleFiberYield(mixed $yieldedValue, ?Uuid $sessionId): ?int ]); } + $requestId = null; + try { if ($yieldedValue instanceof RequestSuspension) { - return $this->sendRequest($yieldedValue->request, $yieldedValue->timeout, $session); + $requestId = $this->registerRequest($yieldedValue->request, $yieldedValue->timeout, $session); + $outgoing = $yieldedValue->request->withId($requestId->toRfc4122()); + } else { + $outgoing = $yieldedValue->notification; } - $this->sendNotification($yieldedValue->notification, $session); + if (null === $transport) { + $this->queueOutgoing($outgoing, ['type' => null === $requestId ? 'notification' : 'request'], $session); + } } finally { - $session->save(); + $saved = $session->save(); } - return null; + // As on the first suspension: a request the session does not hold would never be answered. + if (!$saved) { + $this->logger->warning('Failed to save the session of a suspended request.', [ + 'session_id' => $sessionId->toRfc4122(), + ]); + + return null; + } + + if (null !== $transport) { + $this->queueFiberOutgoing($transport, $outgoing); + } + + return $requestId; } /** * @param McpFiber $fiber */ - private function trackAwaitedRequest(\Fiber $fiber, ?int $requestId): void + private function trackAwaitedRequest(\Fiber $fiber, ?Uuid $requestId): void { if (null === $requestId) { unset($this->awaitedRequestIds[$fiber]); @@ -748,7 +849,7 @@ private function trackAwaitedRequest(\Fiber $fiber, ?int $requestId): void /** * @param McpFiber $fiber * - * @return array + * @return array */ private function getAwaitedPendingRequests(\Fiber $fiber, Uuid $sessionId): array { @@ -757,7 +858,7 @@ private function getAwaitedPendingRequests(\Fiber $fiber, Uuid $sessionId): arra return []; } - return array_intersect_key($this->getPendingRequests($sessionId), [$requestId => true]); + return array_intersect_key($this->getPendingRequests($sessionId), [$requestId->toRfc4122() => true]); } /** diff --git a/src/Server/Transport/BaseTransport.php b/src/Server/Transport/BaseTransport.php index f80b1046..e132f9c3 100644 --- a/src/Server/Transport/BaseTransport.php +++ b/src/Server/Transport/BaseTransport.php @@ -88,7 +88,7 @@ protected function getOutgoingMessages(?Uuid $sessionId): array /** * @param McpFiber $fiber * - * @return array> + * @return array> */ protected function getPendingRequests(?Uuid $sessionId, \Fiber $fiber): array { @@ -102,7 +102,7 @@ protected function getPendingRequests(?Uuid $sessionId, \Fiber $fiber): array /** * @phpstan-return FiberResume */ - protected function checkForResponse(int $requestId, ?Uuid $sessionId): Response|Error|null + protected function checkForResponse(Uuid $requestId, ?Uuid $sessionId): Response|Error|null { if ($sessionId && \is_callable($this->responseFinder)) { return ($this->responseFinder)($requestId, $sessionId); diff --git a/src/Server/Transport/ManagesTransportCallbacks.php b/src/Server/Transport/ManagesTransportCallbacks.php index b5939fe3..39bb7255 100644 --- a/src/Server/Transport/ManagesTransportCallbacks.php +++ b/src/Server/Transport/ManagesTransportCallbacks.php @@ -37,10 +37,10 @@ trait ManagesTransportCallbacks /** @var callable(Uuid): array}> */ protected $outgoingMessagesProvider; - /** @var callable(Uuid, McpFiber): array> */ + /** @var callable(Uuid, McpFiber): array> */ protected $pendingRequestsProvider; - /** @var (callable(int, Uuid): (Response>|Error|null))|null */ + /** @var (callable(Uuid, Uuid): (Response>|Error|null))|null */ protected $responseFinder; /** @var callable(FiberSuspend|null, ?Uuid, McpFiber): void */ @@ -67,7 +67,7 @@ public function setPendingRequestsProvider(callable $provider): void } /** - * @param callable(int, Uuid):(Response>|Error|null) $finder + * @param callable(Uuid, Uuid):(Response>|Error|null) $finder */ public function setResponseFinder(callable $finder): void { diff --git a/src/Server/Transport/StdioTransport.php b/src/Server/Transport/StdioTransport.php index 0b875760..aa2db0e5 100644 --- a/src/Server/Transport/StdioTransport.php +++ b/src/Server/Transport/StdioTransport.php @@ -405,7 +405,7 @@ private function resumeFiber(\Fiber $fiber): void } foreach ($pendingRequests as $pending) { - $requestId = $pending['request_id']; + $requestId = Uuid::fromString($pending['request_id']); $response = $this->checkForResponse($requestId, $this->sessionId); diff --git a/src/Server/Transport/StreamableHttpTransport.php b/src/Server/Transport/StreamableHttpTransport.php index 2008f5a0..79086372 100644 --- a/src/Server/Transport/StreamableHttpTransport.php +++ b/src/Server/Transport/StreamableHttpTransport.php @@ -308,7 +308,7 @@ protected function createStreamedResponse(): ResponseInterface $resumed = false; foreach ($pendingRequests as $pending) { - $requestId = $pending['request_id']; + $requestId = Uuid::fromString($pending['request_id']); $response = $this->checkForResponse($requestId, $this->sessionId); diff --git a/src/Server/Transport/TransportInterface.php b/src/Server/Transport/TransportInterface.php index 40e37418..3ce4e542 100644 --- a/src/Server/Transport/TransportInterface.php +++ b/src/Server/Transport/TransportInterface.php @@ -101,14 +101,14 @@ public function setOutgoingMessagesProvider(callable $provider): void; * * The transport calls this to decide if it should wait for a client response before resuming a Fiber. * - * @param callable(Uuid $sessionId, McpFiber $fiber): array> $provider + * @param callable(Uuid $sessionId, McpFiber $fiber): array> $provider */ public function setPendingRequestsProvider(callable $provider): void; /** * Set a finder function to check for a specific client response. * - * @param callable(int, Uuid):FiberResume $finder + * @param callable(Uuid, Uuid):FiberResume $finder */ public function setResponseFinder(callable $finder): void; diff --git a/tests/Unit/Fixtures/PollingLoopTransport.php b/tests/Unit/Fixtures/PollingLoopTransport.php index ae749305..0019efe3 100644 --- a/tests/Unit/Fixtures/PollingLoopTransport.php +++ b/tests/Unit/Fixtures/PollingLoopTransport.php @@ -25,7 +25,7 @@ final class PollingLoopTransport extends InMemoryTransport { /** - * @return list + * @return list */ public function getPendingRequestIds(): array { @@ -34,6 +34,14 @@ public function getPendingRequestIds(): array return array_keys($this->getPendingRequests($this->sessionId, $this->sessionFiber)); } + /** + * @return array> the messages this stream sends the client next, decoded + */ + public function takeOutgoingMessages(): array + { + return array_map(static fn (array $message): array => json_decode($message['message'], true), $this->getOutgoingMessages($this->sessionId)); + } + /** * @param FiberSuspend $yielded */ diff --git a/tests/Unit/Server/ProtocolSessionRaceTest.php b/tests/Unit/Server/ProtocolSessionRaceTest.php index b29c5501..f6213624 100644 --- a/tests/Unit/Server/ProtocolSessionRaceTest.php +++ b/tests/Unit/Server/ProtocolSessionRaceTest.php @@ -36,7 +36,7 @@ public function testPollingDoesNotOverwriteAConcurrentResponse(): void $sessions = new SessionManager($store, gcProbability: 0); $sessionId = Uuid::v4(); $session = $sessions->createWithId($sessionId); - $session->set('_mcp.pending_requests', [7 => ['request_id' => 7, 'timeout' => 120, 'timestamp' => time()]]); + $session->set('_mcp.pending_requests', ['00000000-0000-4000-8000-000000000007' => ['request_id' => '00000000-0000-4000-8000-000000000007', 'timeout' => 120, 'timestamp' => time()]]); $session->save(); $waiting = new Protocol([], [], MessageFactory::make(), $sessions); @@ -46,13 +46,13 @@ public function testPollingDoesNotOverwriteAConcurrentResponse(): void // The answer lands right after the waiting worker read the session, // before anything it does next could write the session back. $store->interleaveAfterNextRead(static function () use ($answering, $transport, $sessionId): void { - $answering->processInput($transport, '{"jsonrpc": "2.0", "id": 7, "result": {"ok": true}}', $sessionId); + $answering->processInput($transport, '{"jsonrpc": "2.0", "id": "00000000-0000-4000-8000-000000000007", "result": {"ok": true}}', $sessionId); }); // One turn of the waiting worker's loop, with nothing queued to send. $this->assertSame([], $waiting->consumeOutgoingMessages($sessionId)); - $this->assertInstanceOf(Response::class, $waiting->checkResponse(7, $sessionId)); + $this->assertInstanceOf(Response::class, $waiting->checkResponse(Uuid::fromString('00000000-0000-4000-8000-000000000007'), $sessionId)); } #[TestDox('polling a timed out request does not overwrite a client response stored meanwhile')] @@ -63,8 +63,8 @@ public function testPollingATimedOutRequestDoesNotOverwriteAConcurrentResponse() $sessionId = Uuid::v4(); $session = $sessions->createWithId($sessionId); $session->set('_mcp.pending_requests', [ - 7 => ['request_id' => 7, 'timeout' => 5, 'timestamp' => time() - 10], - 8 => ['request_id' => 8, 'timeout' => 120, 'timestamp' => time()], + '00000000-0000-4000-8000-000000000007' => ['request_id' => '00000000-0000-4000-8000-000000000007', 'timeout' => 5, 'timestamp' => time() - 10], + '00000000-0000-4000-8000-000000000008' => ['request_id' => '00000000-0000-4000-8000-000000000008', 'timeout' => 120, 'timestamp' => time()], ]); $session->save(); @@ -74,14 +74,14 @@ public function testPollingATimedOutRequestDoesNotOverwriteAConcurrentResponse() // Another stream's answer lands while this stream polls its timed out request. $store->interleaveAfterNextRead(static function () use ($answering, $transport, $sessionId): void { - $answering->processInput($transport, '{"jsonrpc": "2.0", "id": 8, "result": {"ok": true}}', $sessionId); + $answering->processInput($transport, '{"jsonrpc": "2.0", "id": "00000000-0000-4000-8000-000000000008", "result": {"ok": true}}', $sessionId); }); - $timeout = $waiting->checkResponse(7, $sessionId); + $timeout = $waiting->checkResponse(Uuid::fromString('00000000-0000-4000-8000-000000000007'), $sessionId); $this->assertInstanceOf(Error::class, $timeout); - $this->assertSame(7, $timeout->getId()); + $this->assertSame('00000000-0000-4000-8000-000000000007', $timeout->getId()); - $this->assertInstanceOf(Response::class, $waiting->checkResponse(8, $sessionId)); + $this->assertInstanceOf(Response::class, $waiting->checkResponse(Uuid::fromString('00000000-0000-4000-8000-000000000008'), $sessionId)); } #[TestDox('expiring a timed out request keeps a request another stream sent meanwhile')] @@ -91,7 +91,7 @@ public function testExpiringATimedOutRequestKeepsAConcurrentlySentRequest(): voi $sessions = new SessionManager($store, gcProbability: 0); $sessionId = Uuid::v4(); $session = $sessions->createWithId($sessionId); - $session->set('_mcp.pending_requests', [7 => ['request_id' => 7, 'timeout' => 5, 'timestamp' => time() - 10]]); + $session->set('_mcp.pending_requests', ['00000000-0000-4000-8000-000000000007' => ['request_id' => '00000000-0000-4000-8000-000000000007', 'timeout' => 5, 'timestamp' => time() - 10]]); $session->save(); $waiting = new Protocol([], [], MessageFactory::make(), $sessions); @@ -101,14 +101,14 @@ public function testExpiringATimedOutRequestKeepsAConcurrentlySentRequest(): voi $store->interleaveAfterNextRead(static function () use ($sessions, $sessionId): void { $sending = $sessions->createWithId($sessionId); $pending = $sending->get('_mcp.pending_requests', []); - $pending[9] = ['request_id' => 9, 'timeout' => 120, 'timestamp' => time()]; + $pending['00000000-0000-4000-8000-000000000009'] = ['request_id' => '00000000-0000-4000-8000-000000000009', 'timeout' => 120, 'timestamp' => time()]; $sending->set('_mcp.pending_requests', $pending); $sending->save(); }); }); - $this->assertInstanceOf(Error::class, $waiting->checkResponse(7, $sessionId)); + $this->assertInstanceOf(Error::class, $waiting->checkResponse(Uuid::fromString('00000000-0000-4000-8000-000000000007'), $sessionId)); - $this->assertSame([9], array_keys($sessions->createWithId($sessionId)->get('_mcp.pending_requests'))); + $this->assertSame(['00000000-0000-4000-8000-000000000009'], array_keys($sessions->createWithId($sessionId)->get('_mcp.pending_requests'))); } } diff --git a/tests/Unit/Server/ProtocolTest.php b/tests/Unit/Server/ProtocolTest.php index 85bff04c..38c15ee3 100644 --- a/tests/Unit/Server/ProtocolTest.php +++ b/tests/Unit/Server/ProtocolTest.php @@ -42,6 +42,7 @@ use Mcp\Tests\Unit\Fixtures\PollingLoopTransport; use Mcp\Tests\Unit\Fixtures\RecordingTransport; use Mcp\Tests\Unit\Fixtures\ThrowingRequest; +use Mcp\Tests\Unit\Server\Session\Fixture\InterleavingSessionStore; use PHPUnit\Framework\Attributes\DataProvider; use PHPUnit\Framework\Attributes\TestDox; use PHPUnit\Framework\MockObject\MockObject; @@ -963,10 +964,8 @@ public function testOutboundRequestFailureIsAnsweredUnderInboundRequestId(): voi $this->assertSame($exception, $errorEvents[0]->getThrowable()); $this->assertSame(1, $errorEvents[0]->getError()->getId()); - // The outbound request was queued for the client before the failure; the error is the response. - $outgoing = array_map(static fn (array $outgoingMessage): array => json_decode($outgoingMessage['message'], true), $protocol->consumeOutgoingMessages($sessionId)); - $this->assertCount(1, $outgoing); - $this->assertSame('ping', $outgoing[0]['method']); + // Nobody would resume the fiber, so the outbound request is not sent; the error is the response. + $this->assertSame([], $protocol->consumeOutgoingMessages($sessionId)); $this->assertCount(1, $sent); $this->assertSame(1, $sent[0]['id']); @@ -977,46 +976,125 @@ public function testOutboundRequestFailureIsAnsweredUnderInboundRequestId(): voi public function testConcurrentStreamsPollOnlyTheirOwnPendingRequest(): void { [$protocol, $sessionId, $firstStream, $secondStream] = $this->startTwoStreamsWaitingOnClient(); + $firstIds = $firstStream->getPendingRequestIds(); + $secondIds = $secondStream->getPendingRequestIds(); - $protocol->processInput($secondStream, '{"jsonrpc": "2.0", "id": 1001, "result": {}}', $sessionId); + $protocol->processInput($secondStream, \sprintf('{"jsonrpc": "2.0", "id": %s, "result": {}}', json_encode($secondIds[0])), $sessionId); - $this->assertSame([1000], $firstStream->getPendingRequestIds()); - $this->assertSame([1001], $secondStream->getPendingRequestIds()); + $this->assertCount(1, $firstIds); + $this->assertCount(1, $secondIds); + $this->assertNotSame($firstIds, $secondIds); + $this->assertSame($firstIds, $firstStream->getPendingRequestIds()); + $this->assertSame($secondIds, $secondStream->getPendingRequestIds()); + $this->assertNull($protocol->checkResponse(Uuid::fromString($firstIds[0]), $sessionId)); + $this->assertInstanceOf(Response::class, $protocol->checkResponse(Uuid::fromString($secondIds[0]), $sessionId)); } #[TestDox('A client request a fiber sends after resuming is polled only by its own stream')] public function testRequestYieldedOnResumeIsPolledOnlyByItsOwnStream(): void { [, $sessionId, $firstStream, $secondStream] = $this->startTwoStreamsWaitingOnClient(); + $firstIds = $firstStream->getPendingRequestIds(); + $secondIds = $secondStream->getPendingRequestIds(); $firstStream->yieldFromFiber(new RequestSuspension(new PingRequest(), $sessionId->toRfc4122(), 5)); - $this->assertSame([1002], $firstStream->getPendingRequestIds()); - $this->assertSame([1001], $secondStream->getPendingRequestIds()); + $this->assertCount(1, $firstStream->getPendingRequestIds()); + $this->assertNotSame($firstIds, $firstStream->getPendingRequestIds()); + $this->assertNotSame($secondIds, $firstStream->getPendingRequestIds()); + $this->assertSame($secondIds, $secondStream->getPendingRequestIds()); } #[TestDox('A stream whose fiber resumes and sends a notification no longer polls the request it was waiting on')] public function testNotificationYieldedOnResumeClearsTheAwaitedRequest(): void { [, $sessionId, $firstStream, $secondStream] = $this->startTwoStreamsWaitingOnClient(); + $secondIds = $secondStream->getPendingRequestIds(); $firstStream->yieldFromFiber(new NotificationSuspension(new LoggingMessageNotification(LoggingLevel::Info, 'hello'), $sessionId->toRfc4122())); $this->assertSame([], $firstStream->getPendingRequestIds()); - $this->assertSame([1001], $secondStream->getPendingRequestIds()); + $this->assertSame($secondIds, $secondStream->getPendingRequestIds()); } #[TestDox('A stream whose fiber first suspends on a notification polls none of the session\'s pending requests')] public function testStreamSuspendedOnNotificationPollsNoPendingRequest(): void { [$protocol, $sessionId, , $secondStream] = $this->startTwoStreamsWaitingOnClient(); + $secondIds = $secondStream->getPendingRequestIds(); $thirdStream = new PollingLoopTransport(); $protocol->connect($thirdStream); $protocol->processInput($thirdStream, '{"jsonrpc": "2.0", "id": 3, "method": "ping"}', $sessionId); $this->assertSame([], $thirdStream->getPendingRequestIds()); - $this->assertSame([1001], $secondStream->getPendingRequestIds()); + $this->assertSame($secondIds, $secondStream->getPendingRequestIds()); + } + + #[TestDox('Concurrent streams on one session each send the client request their own fiber sent')] + public function testConcurrentStreamsSendOnlyTheirOwnRequest(): void + { + [, , $firstStream, $secondStream] = $this->startTwoStreamsWaitingOnClient(); + + $firstOutgoing = $firstStream->takeOutgoingMessages(); + $secondOutgoing = $secondStream->takeOutgoingMessages(); + + $this->assertCount(1, $firstOutgoing); + $this->assertSame($firstStream->getPendingRequestIds(), [$firstOutgoing[0]['id']]); + $this->assertCount(1, $secondOutgoing); + $this->assertSame($secondStream->getPendingRequestIds(), [$secondOutgoing[0]['id']]); + } + + #[TestDox('A client request a fiber sends after resuming goes out on its own stream')] + public function testRequestYieldedOnResumeIsSentOnItsOwnStream(): void + { + [, $sessionId, $firstStream, $secondStream] = $this->startTwoStreamsWaitingOnClient(); + $firstStream->takeOutgoingMessages(); + $secondStream->takeOutgoingMessages(); + + $firstStream->yieldFromFiber(new RequestSuspension(new PingRequest(), $sessionId->toRfc4122(), 5)); + + $this->assertSame([], $secondStream->takeOutgoingMessages()); + $firstOutgoing = $firstStream->takeOutgoingMessages(); + $this->assertCount(1, $firstOutgoing); + $this->assertSame($firstStream->getPendingRequestIds(), [$firstOutgoing[0]['id']]); + } + + #[TestDox('Client requests of calls handled by different workers on one session get distinct ids')] + public function testConcurrentWorkersSendClientRequestsUnderDistinctIds(): void + { + $handler = $this->createMock(RequestHandlerInterface::class); + $handler->method('supports')->willReturn(true); + $handler->method('handle')->willReturnCallback(static function (Request $request, SessionInterface $session): Response { + \Fiber::suspend(new RequestSuspension(new PingRequest(), $session->getId()->toRfc4122(), 5)); + + return new Response(1, []); + }); + + $store = new InterleavingSessionStore(); + $sessionManager = new SessionManager($store, gcProbability: 0); + $session = $sessionManager->create(); + $session->save(); + $sessionId = $session->getId(); + + $firstWorker = new Protocol([$handler], [], MessageFactory::make(), $sessionManager); + $secondWorker = new Protocol([$handler], [], MessageFactory::make(), $sessionManager); + $firstStream = new PollingLoopTransport(); + $secondStream = new PollingLoopTransport(); + $firstWorker->connect($firstStream); + $secondWorker->connect($secondStream); + + // The second call is handled after the first worker loaded the session, before it saved it. + $secondIds = null; + $store->interleaveAfterNextRead(static function () use ($secondWorker, $secondStream, $sessionId, &$secondIds): void { + $secondWorker->processInput($secondStream, '{"jsonrpc": "2.0", "id": 2, "method": "ping"}', $sessionId); + $secondIds = $secondStream->getPendingRequestIds(); + }); + $firstWorker->processInput($firstStream, '{"jsonrpc": "2.0", "id": 1, "method": "ping"}', $sessionId); + + $this->assertCount(1, $firstStream->getPendingRequestIds()); + $this->assertCount(1, $secondIds ?? []); + $this->assertNotSame($secondIds, $firstStream->getPendingRequestIds()); } #[TestDox('A stream stops polling a client request once it timed out')] @@ -1047,9 +1125,9 @@ public function testStreamStopsPollingTimedOutRequest(): void $protocol->connect($stream); $protocol->processInput($stream, '{"jsonrpc": "2.0", "id": 1, "method": "ping"}', $sessionId); - $this->assertSame([1000], $stream->getPendingRequestIds()); + $this->assertCount(1, $pendingRequestIds = $stream->getPendingRequestIds()); - $this->assertInstanceOf(Error::class, $protocol->checkResponse(1000, $sessionId)); + $this->assertInstanceOf(Error::class, $protocol->checkResponse(Uuid::fromString($pendingRequestIds[0]), $sessionId)); $this->assertSame([], $stream->getPendingRequestIds()); } @@ -1902,13 +1980,7 @@ public function testServerRequestEventIsDispatched(): void ->willReturnArgument(0); $session = $this->createMock(SessionInterface::class); - $session->method('get')->willReturnCallback(static function ($key, $default = null) { - if ('_mcp.request_id_counter' === $key) { - return 1000; - } - - return $default; - }); + $session->method('get')->willReturnArgument(1); $session->method('getId')->willReturn(Uuid::v4()); $protocol = new Protocol( @@ -1925,13 +1997,13 @@ public function testServerRequestEventIsDispatched(): void 'method' => 'ping', ]); - $protocol->sendRequest($request, 60, $session); + $requestId = $protocol->sendRequest($request, 60, $session); $this->assertInstanceOf(ServerRequestEvent::class, $capturedEvent); $this->assertSame($session, $capturedEvent->getSession()); $this->assertSame(60, $capturedEvent->getTimeout()); $this->assertSame('ping', $capturedEvent->getMethod()); - $this->assertSame(1000, $capturedEvent->getRequest()->getId()); + $this->assertSame($requestId->toRfc4122(), $capturedEvent->getRequest()->getId()); } #[TestDox('ClientResponseEvent is dispatched when a client response is received')] @@ -1950,7 +2022,7 @@ public function testClientResponseEventIsDispatched(): void })) ->willReturnArgument(0); - $session = $this->createSessionWithPendingRequests([1000]); + $session = $this->createSessionWithPendingRequests(['00000000-0000-4000-8000-000000001000']); $this->sessionManager->method('createWithId')->willReturn($session); $this->sessionManager->method('exists')->willReturn(true); @@ -1966,13 +2038,13 @@ public function testClientResponseEventIsDispatched(): void $sessionId = Uuid::v4(); $protocol->processInput( $this->transport, - '{"jsonrpc": "2.0", "id": 1000, "result": {"action": "accept"}}', + '{"jsonrpc": "2.0", "id": "00000000-0000-4000-8000-000000001000", "result": {"action": "accept"}}', $sessionId ); $this->assertInstanceOf(ClientResponseEvent::class, $capturedEvent); $this->assertSame($session, $capturedEvent->getSession()); - $this->assertSame(1000, $capturedEvent->getId()); + $this->assertSame('00000000-0000-4000-8000-000000001000', $capturedEvent->getId()); $this->assertFalse($capturedEvent->isError()); } @@ -1992,7 +2064,7 @@ public function testClientResponseEventIsError(): void return $event; }); - $session = $this->createSessionWithPendingRequests([1000]); + $session = $this->createSessionWithPendingRequests(['00000000-0000-4000-8000-000000001000']); $this->sessionManager->method('createWithId')->willReturn($session); $this->sessionManager->method('exists')->willReturn(true); @@ -2008,13 +2080,13 @@ public function testClientResponseEventIsError(): void $sessionId = Uuid::v4(); $protocol->processInput( $this->transport, - '{"jsonrpc": "2.0", "id": 1000, "error": {"code": -32603, "message": "Client error"}}', + '{"jsonrpc": "2.0", "id": "00000000-0000-4000-8000-000000001000", "error": {"code": -32603, "message": "Client error"}}', $sessionId ); $this->assertInstanceOf(ClientResponseEvent::class, $capturedEvent); $this->assertTrue($capturedEvent->isError()); - $this->assertSame(1000, $capturedEvent->getId()); + $this->assertSame('00000000-0000-4000-8000-000000001000', $capturedEvent->getId()); } #[TestDox('A response to an unknown request ID is neither dispatched nor stored')] @@ -2025,7 +2097,7 @@ public function testClientResponseEventIsNotDispatchedForUnknownId(): void ->expects($this->never()) ->method('dispatch'); - $session = $this->createSessionWithPendingRequests([1000]); + $session = $this->createSessionWithPendingRequests(['00000000-0000-4000-8000-000000001000']); $session->expects($this->never())->method('set'); $this->sessionManager->method('createWithId')->willReturn($session); @@ -2041,18 +2113,18 @@ public function testClientResponseEventIsNotDispatchedForUnknownId(): void $protocol->processInput( $this->transport, - '{"jsonrpc": "2.0", "id": 4711, "result": {"action": "accept"}}', + '{"jsonrpc": "2.0", "id": "00000000-0000-4000-8000-000000004711", "result": {"action": "accept"}}', Uuid::v4() ); } - #[TestDox('A response with a string ID does not match the integer ID of a pending request')] - public function testClientResponseEventIsNotDispatchedForNumericStringId(): void + #[TestDox('A response with an integer ID does not match the numeric string ID of a pending request')] + public function testClientResponseEventIsNotDispatchedForIntegerId(): void { $eventDispatcher = $this->createMock(EventDispatcherInterface::class); $eventDispatcher->expects($this->never())->method('dispatch'); - $session = $this->createSessionWithPendingRequests([1000]); + $session = $this->createSessionWithPendingRequests(['1000']); $session->expects($this->never())->method('set'); $this->sessionManager->method('createWithId')->willReturn($session); @@ -2066,7 +2138,7 @@ public function testClientResponseEventIsNotDispatchedForNumericStringId(): void eventDispatcher: $eventDispatcher, ); - $protocol->processInput($this->transport, '{"jsonrpc": "2.0", "id": "1000", "result": {}}', Uuid::v4()); + $protocol->processInput($this->transport, '{"jsonrpc": "2.0", "id": 1000, "result": {}}', Uuid::v4()); } #[TestDox('A late response to a timed out request is neither dispatched nor stored')] @@ -2085,41 +2157,41 @@ public function testLateResponseToTimedOutRequestIsDropped(): void ); $session = $sessionManager->create(); - $session->set('_mcp.pending_requests', [1000 => ['request_id' => 1000, 'timeout' => 5, 'timestamp' => time() - 10]]); + $session->set('_mcp.pending_requests', ['00000000-0000-4000-8000-000000001000' => ['request_id' => '00000000-0000-4000-8000-000000001000', 'timeout' => 5, 'timestamp' => time() - 10]]); $session->save(); - $protocol->processInput($this->transport, '{"jsonrpc": "2.0", "id": 1000, "result": {}}', $session->getId()); + $protocol->processInput($this->transport, '{"jsonrpc": "2.0", "id": "00000000-0000-4000-8000-000000001000", "result": {}}', $session->getId()); - $this->assertNull($sessionManager->createWithId($session->getId())->get('_mcp.responses.1000')); + $this->assertNull($sessionManager->createWithId($session->getId())->get('_mcp.responses.00000000-0000-4000-8000-000000001000')); } #[TestDox('A timed out client request is reported as an error and its pending entry is dropped')] public function testCheckResponseReportsTimedOutRequestAndDropsItsPendingEntry(): void { [$protocol, $sessionManager, $session] = $this->createProtocolWithPendingRequests([ - 1000 => ['request_id' => 1000, 'timeout' => 5, 'timestamp' => time() - 10], - 1001 => ['request_id' => 1001, 'timeout' => 120, 'timestamp' => time()], + '00000000-0000-4000-8000-000000001000' => ['request_id' => '00000000-0000-4000-8000-000000001000', 'timeout' => 5, 'timestamp' => time() - 10], + '00000000-0000-4000-8000-000000001001' => ['request_id' => '00000000-0000-4000-8000-000000001001', 'timeout' => 120, 'timestamp' => time()], ]); - $result = $protocol->checkResponse(1000, $session->getId()); + $result = $protocol->checkResponse(Uuid::fromString('00000000-0000-4000-8000-000000001000'), $session->getId()); $this->assertInstanceOf(Error::class, $result); - $this->assertSame(1000, $result->getId()); + $this->assertSame('00000000-0000-4000-8000-000000001000', $result->getId()); $this->assertSame(Error::INTERNAL_ERROR, $result->code); $this->assertSame('Request timed out', $result->message); - $this->assertSame([1001], array_keys($protocol->getPendingRequests($session->getId()))); - $this->assertSame([1001], array_keys($sessionManager->createWithId($session->getId())->get('_mcp.pending_requests'))); + $this->assertSame(['00000000-0000-4000-8000-000000001001'], array_keys($protocol->getPendingRequests($session->getId()))); + $this->assertSame(['00000000-0000-4000-8000-000000001001'], array_keys($sessionManager->createWithId($session->getId())->get('_mcp.pending_requests'))); - $this->assertNull($protocol->checkResponse(1000, $session->getId()), 'The timeout is reported once.'); + $this->assertNull($protocol->checkResponse(Uuid::fromString('00000000-0000-4000-8000-000000001000'), $session->getId()), 'The timeout is reported once.'); } #[TestDox('A client request within its timeout stays pending')] public function testCheckResponseKeepsRequestWithinItsTimeout(): void { - $pending = [1000 => ['request_id' => 1000, 'timeout' => 120, 'timestamp' => time() - 10]]; + $pending = ['00000000-0000-4000-8000-000000001000' => ['request_id' => '00000000-0000-4000-8000-000000001000', 'timeout' => 120, 'timestamp' => time() - 10]]; [$protocol, , $session] = $this->createProtocolWithPendingRequests($pending); - $this->assertNull($protocol->checkResponse(1000, $session->getId())); + $this->assertNull($protocol->checkResponse(Uuid::fromString('00000000-0000-4000-8000-000000001000'), $session->getId())); $this->assertSame($pending, $protocol->getPendingRequests($session->getId())); } @@ -2127,15 +2199,15 @@ public function testCheckResponseKeepsRequestWithinItsTimeout(): void public function testCheckResponseReturnsAnswerOfExpiredRequest(): void { [$protocol, , $session] = $this->createProtocolWithPendingRequests([ - 1000 => ['request_id' => 1000, 'timeout' => 5, 'timestamp' => time() - 10], + '00000000-0000-4000-8000-000000001000' => ['request_id' => '00000000-0000-4000-8000-000000001000', 'timeout' => 5, 'timestamp' => time() - 10], ]); - $session->set('_mcp.responses.1000', ['jsonrpc' => '2.0', 'id' => 1000, 'result' => ['action' => 'accept']]); + $session->set('_mcp.responses.00000000-0000-4000-8000-000000001000', ['jsonrpc' => '2.0', 'id' => '00000000-0000-4000-8000-000000001000', 'result' => ['action' => 'accept']]); $session->save(); - $result = $protocol->checkResponse(1000, $session->getId()); + $result = $protocol->checkResponse(Uuid::fromString('00000000-0000-4000-8000-000000001000'), $session->getId()); $this->assertInstanceOf(Response::class, $result); - $this->assertSame(1000, $result->getId()); + $this->assertSame('00000000-0000-4000-8000-000000001000', $result->getId()); $this->assertSame([], $protocol->getPendingRequests($session->getId())); } @@ -2143,15 +2215,15 @@ public function testCheckResponseReturnsAnswerOfExpiredRequest(): void public function testAnswerAfterReportedTimeoutIsDropped(): void { [$protocol, $sessionManager, $session] = $this->createProtocolWithPendingRequests([ - 1000 => ['request_id' => 1000, 'timeout' => 5, 'timestamp' => time() - 10], + '00000000-0000-4000-8000-000000001000' => ['request_id' => '00000000-0000-4000-8000-000000001000', 'timeout' => 5, 'timestamp' => time() - 10], ]); - $this->assertInstanceOf(Error::class, $protocol->checkResponse(1000, $session->getId())); + $this->assertInstanceOf(Error::class, $protocol->checkResponse(Uuid::fromString('00000000-0000-4000-8000-000000001000'), $session->getId())); - $protocol->processInput($this->transport, '{"jsonrpc": "2.0", "id": 1000, "result": {}}', $session->getId()); + $protocol->processInput($this->transport, '{"jsonrpc": "2.0", "id": "00000000-0000-4000-8000-000000001000", "result": {}}', $session->getId()); - $this->assertNull($sessionManager->createWithId($session->getId())->get('_mcp.responses.1000')); - $this->assertNull($protocol->checkResponse(1000, $session->getId())); + $this->assertNull($sessionManager->createWithId($session->getId())->get('_mcp.responses.00000000-0000-4000-8000-000000001000')); + $this->assertNull($protocol->checkResponse(Uuid::fromString('00000000-0000-4000-8000-000000001000'), $session->getId())); } #[TestDox('ClientResponseEvent::getId() is null when the error has no id')] @@ -2506,11 +2578,12 @@ public function testFiberYieldedRequestSuspensionIsQueued(): void $this->assertSame(45, $suspension->timeout); $this->assertNull($suspension->inputKey); - $protocol->handleFiberYield($suspension, $sessionId); + $requestId = $protocol->handleFiberYield($suspension, $sessionId); + $this->assertNotNull($requestId); $pending = $protocol->getPendingRequests($sessionId); $this->assertCount(1, $pending); - $this->assertSame(45, $pending[1000]['timeout']); + $this->assertSame(45, $pending[$requestId->toRfc4122()]['timeout']); $outgoing = $protocol->consumeOutgoingMessages($sessionId); $this->assertCount(1, $outgoing); @@ -2518,7 +2591,7 @@ public function testFiberYieldedRequestSuspensionIsQueued(): void $message = json_decode($outgoing[0]['message'], true); $this->assertSame('roots/list', $message['method']); - $this->assertSame(1000, $message['id']); + $this->assertSame($requestId->toRfc4122(), $message['id']); } #[TestDox('A fiber yield does not bring back a session destroyed while the request ran')] @@ -2538,6 +2611,29 @@ public function testFiberYieldDoesNotResurrectADestroyedSession(): void $this->assertFalse($store->exists($sessionId)); } + #[TestDox('A fiber yield does not send a client request the session could not store')] + public function testFiberYieldDoesNotSendARequestTheSessionCouldNotStore(): void + { + $store = new InterleavingSessionStore(); + $sessions = new SessionManager($store, gcProbability: 0); + $sessionId = Uuid::v4(); + $sessions->createWithId($sessionId)->save(); + + $protocol = new Protocol([], [], MessageFactory::make(), $sessions); + $transport = new PollingLoopTransport(); + $protocol->connect($transport); + $transport->setSessionId($sessionId); + + // A DELETE lands after the yield read the session, before it saves. + $store->interleaveAfterNextRead(static function () use ($sessions, $sessionId): void { + $sessions->destroy($sessionId); + }); + $requestId = $protocol->handleFiberYield(new RequestSuspension(new PingRequest(), $sessionId->toRfc4122(), 60), $sessionId, $transport); + + $this->assertNull($requestId); + $this->assertSame([], $transport->takeOutgoingMessages()); + } + #[TestDox('A fiber yield that is not a suspension object is dropped without touching the session')] public function testFiberYieldedUnexpectedPayloadIsIgnored(): void { @@ -2556,7 +2652,7 @@ public function testFiberYieldedUnexpectedPayloadIsIgnored(): void } /** - * @param array $pending + * @param array $pending * * @return array{Protocol, SessionManager, SessionInterface} */ @@ -2578,7 +2674,7 @@ private function createProtocolWithPendingRequests(array $pending): array } /** - * @param list $requestIds + * @param list $requestIds */ private function createSessionWithPendingRequests(array $requestIds): SessionInterface&MockObject { diff --git a/tests/Unit/Server/Transport/StdioSuspendedFibersTest.php b/tests/Unit/Server/Transport/StdioSuspendedFibersTest.php index 646c05f4..7c3e9957 100644 --- a/tests/Unit/Server/Transport/StdioSuspendedFibersTest.php +++ b/tests/Unit/Server/Transport/StdioSuspendedFibersTest.php @@ -11,6 +11,7 @@ namespace Mcp\Tests\Unit\Server\Transport; +use Mcp\Event\ServerRequestEvent; use Mcp\Schema\Elicitation\ElicitationSchema; use Mcp\Schema\Elicitation\StringSchemaDefinition; use Mcp\Schema\Enum\ProtocolVersion; @@ -19,6 +20,7 @@ use Mcp\Server\Transport\StdioTransport; use PHPUnit\Framework\Attributes\TestDox; use PHPUnit\Framework\TestCase; +use Psr\EventDispatcher\EventDispatcherInterface; /** * Several handshake-era tool calls in flight on one stdio connection, each @@ -46,16 +48,17 @@ public function testConcurrentProgressCallsAllFinish(): void #[TestDox('concurrent elicitations each resume with their own answer')] public function testConcurrentElicitationsResumeWithTheirOwnAnswer(): void { - // The server numbers its requests to the client from 1000 on, in the order the calls ask. + // Request ids are random, so the client answers once both elicitations went out, the second one first. $lines = $this->exchange([ self::call(2, 'ask'), self::call(3, 'ask'), - ['jsonrpc' => '2.0', 'id' => 1001, 'result' => ['action' => 'accept', 'content' => ['name' => 'second']]], - ['jsonrpc' => '2.0', 'id' => 1000, 'result' => ['action' => 'accept', 'content' => ['name' => 'first']]], + ], static fn (int|string $first, int|string $second): array => [ + ['jsonrpc' => '2.0', 'id' => $second, 'result' => ['action' => 'accept', 'content' => ['name' => 'second']]], + ['jsonrpc' => '2.0', 'id' => $first, 'result' => ['action' => 'accept', 'content' => ['name' => 'first']]], ]); $elicitations = array_values(array_filter($lines, static fn (array $line): bool => 'elicitation/create' === ($line['method'] ?? null))); - $this->assertSame([1000, 1001], array_column($elicitations, 'id')); + $this->assertCount(2, $elicitations); $this->assertSame('first', $this->answer($lines, 2)['result']['content'][0]['text'] ?? null); $this->assertSame('second', $this->answer($lines, 3)['result']['content'][0]['text'] ?? null); @@ -93,12 +96,46 @@ private static function call(int $id, string $tool, ?string $progressToken = nul /** * Runs a handshake-era session with the given messages after the handshake. * - * @param list> $messages + * @param list> $messages + * @param (\Closure(int|string, int|string): list>)|null $answer the client's messages once the server sent two requests, given their ids * * @return list> */ - private function exchange(array $messages): array + private function exchange(array $messages, ?\Closure $answer = null): array { + $input = fopen('php://temp', 'r+'); + $this->assertNotFalse($input); + + $requestIds = []; + $dispatcher = new class(static function (ServerRequestEvent $event) use ($input, $answer, &$requestIds): void { + $requestIds[] = $event->getRequest()->getId(); + if (null === $answer || 2 !== \count($requestIds)) { + return; + } + + // Appended behind what the server has yet to read. + $position = ftell($input); + fseek($input, 0, \SEEK_END); + foreach ($answer(...$requestIds) as $message) { + fwrite($input, json_encode($message, \JSON_THROW_ON_ERROR)."\n"); + } + fwrite($input, str_repeat("\n", 5)); + fseek($input, (int) $position); + }) implements EventDispatcherInterface { + public function __construct(private readonly \Closure $onServerRequest) + { + } + + public function dispatch(object $event): object + { + if ($event instanceof ServerRequestEvent) { + ($this->onServerRequest)($event); + } + + return $event; + } + }; + $server = Server::builder() ->setServerInfo('test-server', '1.0.0') ->addTool(static function (RequestContext $context): string { @@ -112,10 +149,9 @@ private function exchange(array $messages): array return $result->content['name'] ?? 'none'; }, name: 'ask', description: 'Asks for a name') + ->setEventDispatcher($dispatcher) ->build(); - $input = fopen('php://temp', 'r+'); - $this->assertNotFalse($input); // A file rather than memory: running the server closes its streams. $outputFile = tempnam(sys_get_temp_dir(), 'mcp-stdio'); $output = fopen($outputFile, 'w'); diff --git a/tests/Unit/Server/Transport/StreamableHttpTransportTest.php b/tests/Unit/Server/Transport/StreamableHttpTransportTest.php index 21ea16b5..aaef1302 100644 --- a/tests/Unit/Server/Transport/StreamableHttpTransportTest.php +++ b/tests/Unit/Server/Transport/StreamableHttpTransportTest.php @@ -432,9 +432,9 @@ public function testPollingLoopResumesFiberWithTimeoutErrorFromResponseFinder(): $transport->attachFiberToSession($fiber, Uuid::v4()); }); $transport->setOutgoingMessagesProvider(static fn (): array => []); - $transport->setResponseFinder(static fn (int $requestId): Error => Error::forInternalError('Request timed out', $requestId)); + $transport->setResponseFinder(static fn (Uuid $requestId): Error => Error::forInternalError('Request timed out', $requestId->toRfc4122())); $transport->setPendingRequestsProvider(static fn (): array => [ - ['request_id' => 1, 'timestamp' => 1_000_000, 'timeout' => 120], + '00000000-0000-4000-8000-000000000001' => ['request_id' => '00000000-0000-4000-8000-000000000001', 'timestamp' => 1_000_000, 'timeout' => 120], ]); $response = $transport->listen(); @@ -477,7 +477,7 @@ protected function isConnectionAborted(): bool $transport->setOutgoingMessagesProvider(static fn (): array => []); $transport->setResponseFinder(static fn () => null); $transport->setPendingRequestsProvider(static fn (): array => [ - ['request_id' => 1, 'timestamp' => time(), 'timeout' => 120], + '00000000-0000-4000-8000-000000000001' => ['request_id' => '00000000-0000-4000-8000-000000000001', 'timestamp' => time(), 'timeout' => 120], ]); $response = $transport->listen();