Repository navigation
[Server] Expire timed out client requests in Protocol::checkResponse() - #585
Merged
chr-hertel merged 2 commits intoOct 13, 2026
Conversation
Arslan-TR
requested review from
CodeWithKyrian,
Nyholm,
chr-hertel and
soyuka
as code owners
October 10, 2026 08:49
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Expiration can still overwrite a response saved concurrently between the session reload and whole-session save.
1 open finding
What changed in this PR
Moves timed-out client-request cleanup into Protocol::checkResponse() and removes duplicate transport timeout handling.
Changes:
- Expire pending requests centrally while preserving stored responses.
- Simplify HTTP and stdio polling loops.
- Add timeout, late-response, and concurrency tests.
| File | Description |
|---|---|
src/Server/Protocol.php |
Implements centralized timeout expiration. |
src/Server/Transport/StreamableHttpTransport.php |
Removes transport timeout logic and clock injection. |
src/Server/Transport/StdioTransport.php |
Removes duplicate timeout handling. |
tests/Unit/Server/ProtocolTest.php |
Tests timeout and response behavior. |
tests/Unit/Server/ProtocolSessionRaceTest.php |
Updates concurrent-session coverage. |
tests/Unit/Server/Transport/StreamableHttpTransportTest.php |
Tests forwarding timeout errors to fibers. |
CHANGELOG.md |
Documents the fix. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
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
chr-hertel
force-pushed
the
fix/561-timed-out-requests-cleanup
branch
from
October 13, 2026 21:43
ea41404 to
674a865
Compare
…ng one keeps the others
chr-hertel
added a commit
to chr-hertel/php-sdk
that referenced
this pull request
Oct 13, 2026
chr-hertel
added a commit
to chr-hertel/php-sdk
that referenced
this pull request
Oct 14, 2026
chr-hertel
added a commit
that referenced
this pull request
Oct 14, 2026
…s answers (#581) * [Server] Fix parallel elicitations on one session getting each other's answers * [Server] Address review: cap client request ids at PHP_INT_MAX on 32-bit PHP * [Server] Read the random client request ids in the tests of #580 and #585 * [Server] Address review: send requests to the client with UUID ids * [Server] Address review: pass client request ids as Uuid instances * [Server] Address review: answer the second stream's request by its UUID in the test * [Server] Address review: don't send a client request the session could not store on a fiber yield * [Server] Read the client request ids in the tests of #584
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.