Repository navigation
Conversation
The timeout branches in `StreamableHttpTransport` and `StdioTransport` resumed the suspended fiber with an error but left the request in the session's pending requests. A fiber that suspended again was then resumed with that stale timeout once more, and the entry stayed in the session until it ended. `checkResponse()` now answers a request past its timeout with the `Request timed out` error and drops its pending entry, like an answer does, and the duplicated timeout logic is gone from both transports. Expiry reloads the session before saving it back, so it does not undo what other streams stored since the poll started. `StreamableHttpTransport` loses its `$clock` constructor argument, which only served the removed check. Fixes modelcontextprotocol#561
Arslan-TR
requested review from
CodeWithKyrian,
Nyholm,
chr-hertel and
soyuka
as code owners
October 10, 2026 08:49
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #561
The unknown-ID half of the issue is already handled by
handleResponse()onmain; this covers the other half.Problem
When a request to the client (elicitation, sampling) timed out, the polling loops of
StreamableHttpTransportandStdioTransportresumed the fiber with aRequest timed outerror but never removed the entry from_mcp.pending_requests. OnlyProtocol::checkResponse()did that, and only for answers. So the entry stayed in the session until it ended, and a fiber that suspended again was resumed with the same stale timeout once more.Change
Protocol::checkResponse()now returns theRequest timed outerror for a request past its timeout and drops its pending entry, like an answer does. An answer that is already stored still wins over the timeout.checkForResponse()keeps its signature.ProtocolSessionRaceTestcovers that case; its timed out request now returns the error instead ofnull). The general read-modify-write race stays with [Server][Streamable HTTP] Concurrent requests in the same session can overwrite queued responses and cause stale/unknown message IDs #275.StreamableHttpTransportloses its$clockconstructor argument, as the transport no longer compares timestamps itself. It was added after v0.8.1, so it is not in a release yet.Protocolusestime(), likehandleResponse()already does.Tests
New in
ProtocolTest: the timed out request is reported once and only its entry is dropped, a request within its timeout stays pending, a stored answer beats the timeout, a late answer after the reported timeout is dropped, and a stream stops polling a request that timed out. TheStreamableHttpTransporttest that relied on the injected clock now checks that the polling loop resumes the fiber with the error the response finder returns.ProtocolTest::testCheckResponseReportsTimedOutRequestAndDropsItsPendingEntry,testAnswerAfterReportedTimeoutIsDropped,testStreamStopsPollingTimedOutRequestand the updatedProtocolSessionRaceTestcase fail without thesrc/change.Ran locally:
phpunitunit suite,phpstan(no errors),php-cs-fixeron the changed files. The unit suite still has failures that are the same without this change (FileSessionStoreTestpermission tests,JwtTokenValidatorTest,HttpTransportListenTest, on Windows). The integration tests that spawn a stdio server did not finish here, andDualEraEndpointTest::testAsksForInputfails identically onmain; I did not run the interop/conformance suites.