Repository navigation
[Server][Streamable HTTP] Concurrent SSE streams on one session can consume each other's client responses #544
Description
Activity
I confirmed the ownership problem at
a5ed85fand found a second state-lifecycle requirement that seems important for the fix.Today
checkResponse()is the only server path that removes an entry from_mcp.pending_requests. The timeout branch inStreamableHttpTransport::createStreamedResponse()resumes the fiber with an error but does not remove that request. Therefore even an otherwise correctly owned timeout leaves a stale, already-expired pending entry in the session; a later stream can encounter it and immediately resume the wrong fiber with that old timeout. In the other direction,handleResponse()stores any keyed response without first checking that the request is still pending, so a response arriving after timeout can remain orphaned.I would make response/timeout resolution one atomic operation rather than adding only a read-side filter:
- Each pending record gets an opaque stream/fiber owner at creation.
- A loop may atomically claim and remove only a pending record with its owner.
- Response completion consumes both the response and pending record; timeout completion removes the pending record before resuming.
- A late response with no pending owner is discarded/logged, never left for a future fiber.
The owner cannot safely be inferred by diffing the session-wide list:
handleFiberYield()currently discards the request ID returned byProtocol::sendRequest(), and the whole-session read/modify/write race already tracked in #275 could overwrite that inference. Returning the created ID from the yield callback, or passing a transport-generated owner token intosendRequest(), would make the association explicit.Regression cases I would add:
- A and B pending; B responds first: only B resumes and A remains pending.
- A responds while B expires: each fiber receives only its own outcome.
- After timeout, B is absent from pending state.
- A late B response is ignored and cannot resume a later fiber.
- Repeat the cases with an interleaving session save so the fix does not rely on non-atomic whole-session mutation.
AI disclosure: ChatGPT helped inspect the current source and draft this review. This is source-level verification on a clean checkout; I did not execute a PHP concurrency harness.
Summary
On Streamable HTTP (handshake era, SSE), two tool calls on one session that both send a server-to-client request, such as
elicitation/create, can each receive the other's answer.Cause
Protocolkeeps pending server-to-client requests in one session-wide list,_mcp.pending_requests. The SSE loop inStreamableHttpTransport::createStreamedResponse()walks that whole list, not only the requests its own fiber sent:With tool calls A and B open on one session, each waiting in
ClientGateway::elicit():The timeout branch has the same problem: A's loop can resume A's fiber with a timeout error for B's request ID.
I found this by reading the source on
main(a5ed85f) and 0.8.1. I have not reproduced it end to end. Running two streams on one session at the same time needs a server that doesn't serialize requests per session.Suggested fix
Record which stream sent each pending request, and have each loop check only its own. For example, track the request IDs the fiber yielded in the transport instance, and filter
getPendingRequests()by them.Context
In Drupal's
mcp_serverwe lock the session for the length of a stream. That lock made a second elicitation wait, which hid this bug. We're changing the lock to cover each session write instead of the whole stream, so elicitation answers don't wait on the lease (mcp_server!88). That makes this race reachable.