Skip to content

[Server] Keep inbound request intact when queueing an outbound one #486

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
chr-hertel wants to merge 1 commit into
base: main
Choose a base branch
from
Open

[Server] Keep inbound request intact when queueing an outbound one #486

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions src/Server/Protocol.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -303,9 +303,11 @@ private function handleRequest(TransportInterface $transport, Request $request,
$notification = $result['notification'];
$this->sendNotification($notification, $session);
} elseif ('request' === $result['type']) {
$request = $result['request'];
// Keep $request untouched: it is the inbound request the catch
// blocks below answer under, not this outbound one.
$outboundRequest = $result['request'];
$timeout = $result['timeout'] ?? 120;
$this->sendRequest($request, $timeout, $session);
$this->sendRequest($outboundRequest, $timeout, $session);
}
}

Expand Down
59 changes: 59 additions & 0 deletions tests/Unit/Server/ProtocolTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,9 @@
use Mcp\Server\Handler\Notification\NotificationHandlerInterface;
use Mcp\Server\Handler\Request\RequestHandlerInterface;
use Mcp\Server\Protocol;
use Mcp\Server\Session\InMemorySessionStore;
use Mcp\Server\Session\SessionInterface;
use Mcp\Server\Session\SessionManager;
use Mcp\Server\Session\SessionManagerInterface;
use Mcp\Server\Transport\TransportInterface;
use Mcp\Tests\Unit\Fixtures\ThrowingRequest;
Expand Down Expand Up @@ -793,6 +795,63 @@ public function testHandlerUnexpectedExceptionReturnsInternalError(): void
$this->assertStringNotContainsString('Unexpected error', $message['error']['message']);
}

#[TestDox('Failure while dispatching an outbound request is answered under the inbound request id')]
public function testOutboundRequestFailureIsAnsweredUnderInboundRequestId(): void
{
$handler = $this->createMock(RequestHandlerInterface::class);
$handler->method('supports')->willReturn(true);
$handler->method('handle')->willReturnCallback(static function (): Response {
// Suspend with an outbound, id-less request, as sampling/elicitation handlers do.
\Fiber::suspend(['type' => 'request', 'request' => new PingRequest(), 'timeout' => 5]);

return new Response(1, []);
});

$exception = new \RuntimeException('Transport unavailable');
$this->transport->method('attachFiberToSession')->willThrowException($exception);

$sessionManager = new SessionManager(new InMemorySessionStore());

$events = [];
$dispatcher = $this->createMock(EventDispatcherInterface::class);
$dispatcher->method('dispatch')->willReturnCallback(static function (object $event) use (&$events): object {
$events[] = $event;

return $event;
});

$protocol = new Protocol(
requestHandlers: [$handler],
notificationHandlers: [],
messageFactory: MessageFactory::make(),
sessionManager: $sessionManager,
eventDispatcher: $dispatcher,
);

$session = $sessionManager->create();
$session->save();
$sessionId = $session->getId();
$protocol->processInput(
$this->transport,
'{"jsonrpc": "2.0", "id": 1, "method": "ping"}',
$sessionId
);

$errorEvents = array_values(array_filter($events, static fn (object $event): bool => $event instanceof ErrorEvent));
$this->assertCount(1, $errorEvents);
$this->assertSame($exception, $errorEvents[0]->getThrowable());
$this->assertSame(1, $errorEvents[0]->getError()->getId());

$outgoing = $protocol->consumeOutgoingMessages($sessionId);
$errors = array_values(array_filter(
array_map(static fn (array $outgoingMessage): array => json_decode($outgoingMessage['message'], true), $outgoing),
static fn (array $message): bool => isset($message['error']),
));
$this->assertCount(1, $errors);
$this->assertSame(1, $errors[0]['id']);
$this->assertSame(Error::INTERNAL_ERROR, $errors[0]['error']['code']);
}

#[TestDox('Notification handler exceptions are caught and logged')]
public function testNotificationHandlerExceptionsAreCaught(): void
{
Expand Down