Handle closed connection in StreamTransport to avoid stuck worker

[Gregory-Gerard]

Q	A
Bug fix?	yes
New feature?	yes
Docs?	no
Issues	Fix #283
License	MIT

WIP: I'll update this description with full details as I progress 👍

[OskarStark]

Hmm one is called runtime, the other one transport, having a runtime interface on a transport feels weird 😐 any ideas?

[Gregory-Gerard]

Yes it feels weird to me too. But I don't have other ideas for now for testing StreamTransport and mocking connection_aborted for example.

I could override connection_aborted (I found an example in Slim framework) in StreamTransport in my test, but I don't really like this idea too.

If someone have guidance, I'll happily modify this!

[Copilot]

Pull Request Overview

This PR abstracts the StreamTransport I/O operations behind a RuntimeInterface to enable testing and improve testability. The main purpose is to handle closed connections in the transport layer to avoid stuck workers.

• Introduces RuntimeInterface and Runtime implementation to wrap PHP's built-in functions
• Adds InMemoryRuntime test fixture to enable unit testing of transport behavior
• Refactors StreamTransport to use the runtime interface instead of direct PHP function calls

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
src/mcp-sdk/src/Server/Transport/Sse/RuntimeInterface.php	Defines interface for runtime operations (connection status, output buffering, etc.)
src/mcp-sdk/src/Server/Transport/Sse/Runtime.php	Production implementation wrapping PHP's built-in functions
src/mcp-sdk/src/Server/Transport/Sse/StreamTransport.php	Updated to use runtime interface with default parameter for backward compatibility
src/mcp-sdk/tests/Fixtures/InMemoryRuntime.php	Test implementation capturing output for assertions
src/mcp-sdk/tests/Server/Transport/Sse/StreamTransportTest.php	Comprehensive unit tests for StreamTransport behavior

[Gregory-Gerard]

@OskarStark would you be okay if I try the namespaced function override approach just for connection_aborted via a test-only polyfill? We can keep flush/write as-is and assert via PHP’s output buffering.

I couldn’t find other example testing this; even HttpFoundation’s EventStreamResponse doesn’t check specifically connection_aborted.

This would avoids the Runtime dependency and any constructor changes. If you’re fine with it, I’ll add a dev-only polyfill that shadows Symfony\AI\McpSdk\Server\Transport\Sse\connection_aborted() in tests.

[OskarStark]

I would say give it a try, why not

[Gregory-Gerard]

Re @OskarStark! I implemented the namespaced override only for connection_aborted. StreamTransport keeps native calls; tests assert via output capture; and no new RuntimeInterface. If you’re good with this, I’ll proceed with the concrete SSE ping/heartbeat fix.

[OskarStark]

Yes why not 🙌🙂

[Gregory-Gerard]

Hi @OskarStark @chr-hertel (friendly tag because you were tagged in the issue originally)!

Quick update:

• Still WIP. Next up: make ping interval and request timeouts configurable per MCP spec (https://modelcontextprotocol.io/specification/2025-06-18/basic/utilities/ping). I may introduce a small immutable ServerConfig injected into Server to avoid constructor bloat.
• Ping (or rather the timeout) is non-strict for now because some clients don't reply explicitly to pings. I'll add a "strict" mode (default off) so we can enforce spec behavior without breaking common clients (spec: https://modelcontextprotocol.io/specification/2025-06-18/basic/lifecycle#timeouts, e.g. issue with a strict timeout When using it in VSCode with Github Copilot, a timeout error is thrown. punkpeye/fastmcp#38).
• Major changes so far:
  • Added PendingResponse/PendingResponseBag to track outgoing requests, resolve inbound responses/errors, and GC on timeout. This currently requires JsonRpcHandler to yield Response|Error. Longer-term I'd prefer splitting JSON encode/decode from processing handlers as proposed in PR Draft: Streamable Http #240, but seems out of scope here, but happy to adjust later if you prefer.
  • Added KeepAliveSessionInterface + KeepAliveSession to automate periodic pings.
  • Server::sendRequest(...) with callback and timeout handling is in; next I'll add per-request timeout overrides and emit a cancel notification to the client on timeout (which implies adding sendNotification), still per the spec.
  • Injected a ClockInterface to make testing easier and remove a direct dependency on system clock with usleep
• If everything is okay, I will of course update the mcp-bundle with new configuration, docs, etc.
• Also, last thing but not least, we may need to hardcode the ping capability in RequestHandlers (because most implementation does), and client/server SHOULD always respond to a ping, any ideas about this?

WDYT about my directions? Happy to have feedback before polishing thing!

[Gregory-Gerard]

Thinking about adding a default Clock like in Symfony Messenger, wdyt?