Proposal: Add StreamableHTTPOptions.SessionTimeout (Ungraceful client disconnects leads to zombie sessions)

[fgrosse]

Describe the bug

The streamable HTTP transport implementation in mcp/streamable.go leaks a goroutine on the server for each client that disconnects ungracefully. Over time, a server can accumulate many "zombie" sessions, consuming resources.

Details

When a new MCP session is established via the connect function in transport.go:141, the jsonrpc2 connection layer spawns a long-running goroutine to read incoming client messages. This goroutine, created in internal/jsonrpc2/conn.go:279, eventually blocks on the streamableServerConn.Read(...) method.

The Read method waits on one of three conditions:

1. The session's context is canceled.
2. A message arrives on the incoming channel.
3. The session's done channel is closed.

When a client disconnects abruptly (e.g., due to network failure), no more HTTP requests can be made, so no new messages will ever arrive on the incoming channel. The session's context is intentionally detached from the underlying HTTP requests, so it does not get canceled. The session is only terminated, and its done channel closed, when a keep-alive fails or the client sends an explicit DELETE request.

In the case of an ungraceful disconnect, the server is never notified, leaving the reading goroutine blocked indefinitely in the Read(...) method, thus leaking the goroutine.

To Reproduce

Checkout the code from #496 (which adds an opt-in leak detector for unit tests) and run the test with the leak detector enabled:

❯ git checkout fgrosse:fgrosse/goleaks

❯ go test -run TestClientReplay/default -leak ./mcp
PASS
goleak: Errors on successful test run: found unexpected goroutines:
[Goroutine 66 in state select, with github.com/modelcontextprotocol/go-sdk/mcp.(*streamableServerConn).Read on top of the stack:
github.com/modelcontextprotocol/go-sdk/mcp.(*streamableServerConn).Read(0x140003ae0c0, {0x10553b1f8, 0x1400038a090})
	/Users/fgroe/src/panw/mcp/official-go-sdk/mcp/streamable.go:869 +0x7c
github.com/modelcontextprotocol/go-sdk/internal/jsonrpc2.(*Connection).readIncoming(0x1400038c410, {0x10553b1f8, 0x1400038a090}, {0x10c77c280, 0x140003ae0c0}, {0x105533c80, 0x140003a2010})
	/Users/fgroe/src/panw/mcp/official-go-sdk/internal/jsonrpc2/conn.go:536 +0x54
created by github.com/modelcontextprotocol/go-sdk/internal/jsonrpc2.NewConnection.(*Connection).start.func1 in goroutine 46
	/Users/fgroe/src/panw/mcp/official-go-sdk/internal/jsonrpc2/conn.go:279 +0xd8
]
exit status 1
FAIL	github.com/modelcontextprotocol/go-sdk/mcp	3.386s

Expected behavior

The server should never leak any goroutines.

Additional context

I found this issue while investigating flaky unit tests via #489

[findleyr]

It's not just a the goroutine that leaks in these cases. There is other session-specific state that is retained. This isn't a problem in stateless mode, or if the server has a keepalive callback (but keepalive only works if the clients issue a hanging GET).

The server should never leak any goroutines.

I could see a world where a different transport API allows us to avoid the goroutine, but the zombie session problem would still persist. The point is that the server doesn't know if these resources are actually zombies. That a goroutine is involved seems like a second order problem--please comment if you disagree.

How about a SessionTimeout time.Duration field on StreamableHTTPOptions, that times out the session if it hasn't received any traffic over a period of time?

[fgrosse]

Yeah, I agree with everything. Technically, a transport could be implemented in a more event-driven way, which would avoid the need for a goroutine for each session, but that's probably a large refactor and still wouldn't solve the underlying problem because we would still leak memory.

A session timeout makes sense to me as well 👍

[findleyr]

Thanks. Yes, Read/Write simplifies the Connection API at the cost of a goroutine. Alternatives were deemed worse:

1. If we used channels, we could have a single goroutine select over all connections, but that's more complicated.
2. If we used callbacks, it takes away control over concurrency from the jsonrpc2 package. I.e. we could invert control and let the transport call the message handler, but then it's much harder to implement a concurrency model or cancellation.

I think we could still do (1) by having a more advanced interface that connections could optionally implement, but it's probably not worth it since there will always be session state.

[fgrosse]

Yes, the underlying issue of leaking memory also sounds more important (because its also an attack vector for DOS) than optimizing away the one goroutine.

[findleyr]

I'm going to implement this in advance of our proposal meeting tomorrow, to ensure we understand it.

[findleyr]

No objections from the proposal meeting. We should be sure to also clean up events using the EventStore interface.

We think the default timeout should be 0 (=infinite). It has to be this anyway for compatibility.