Skip to content

[Server] Fix parallel elicitations on one session getting each other's answers - #581

Merged
chr-hertel merged 8 commits into
modelcontextprotocol:mainfrom
chr-hertel:fix-parallel-client-requests
Oct 14, 2026
Merged

chr-hertel merged 8 commits into
modelcontextprotocol:mainfrom
chr-hertel:fix-parallel-client-requests

Conversation

@chr-hertel

@chr-hertel chr-hertel commented Oct 10, 2026 •

Copy link
Copy Markdown
Member

Two tool calls on one session that both elicit got the same request id (1000), so one call resumed with the other one's answer. On top, an elicitation could go out on another call's SSE stream, since all streams drained one session-wide queue.

  • requests to the client get a UUID as id instead of a counter read-modify-written in the session - the spec allows string ids, and clients echo them as is
  • [BC Break] Protocol::sendRequest(), checkResponse() and handleFiberYield() take and return a Uuid instead of an int, as does a transport's response finder
  • what a suspended fiber sends is queued per transport in Protocol, so it goes out on the stream of its own call - the session-wide queue stays for messages not tied to a call, like ResourceUpdatedNotification

Fixes chr-hertel#56

🤖 Generated with Claude Code

@chr-hertel chr-hertel added this to the 0.9.0 milestone Oct 10, 2026
@chr-hertel chr-hertel added bug Something isn't working Server Issues & PRs related to the Server component labels Oct 10, 2026
@chr-hertel
chr-hertel requested a balanced review from Copilot October 12, 2026 20:08

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Concurrent session writes can still discard pending requests, and the request-ID bound fails on supported 32-bit PHP builds.

2 open findings
What changed in this PR

Fixes parallel session calls sharing request IDs and sending messages over the wrong stream.

Changes:

  • Generates random JavaScript-safe request IDs.
  • Queues fiber messages per transport.
  • Adds concurrency regression tests and documentation.
File Description
src/​Server/​Protocol.php Adds random IDs and transport-specific queues.
tests/​Unit/​Server/​ProtocolTest.php Tests concurrent streams and workers.
tests/​Unit/​Fixtures/​PollingLoopTransport.php Exposes outgoing messages for tests.
CHANGELOG.md Documents the concurrency fix.

🧠 Review effort: Balanced


💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/Server/Protocol.php Outdated
Comment thread src/Server/Protocol.php Outdated
@chr-hertel
chr-hertel force-pushed the fix-parallel-client-requests branch from 1d66c37 to 0e5331f Compare October 12, 2026 20:58
Comment thread src/Server/Protocol.php Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

A concurrency test sends a UUID through an integer formatter, and the documented request-ID contract conflicts with the implementation.

2 open findings
2 resolved since last review

🧠 Review effort: Balanced

Comment thread tests/Unit/Server/ProtocolTest.php Outdated
Comment thread src/Server/Protocol.php Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

A failed session save can still send an untracked transport-local request and incorrectly resume its fiber.

1 open finding
2 resolved since last review

🧠 Review effort: Balanced

Comment thread src/Server/Protocol.php

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The implementation addresses both reported concurrency causes with focused regression and race-condition coverage.

0 open findings

1 resolved since last review

🧠 Review effort: Balanced

@chr-hertel
chr-hertel force-pushed the fix-parallel-client-requests branch from 6a892cf to 723e550 Compare October 14, 2026 22:17
@chr-hertel
chr-hertel merged commit d9bd9cc into modelcontextprotocol:main Oct 14, 2026
53 of 54 checks passed
@chr-hertel
chr-hertel deleted the fix-parallel-client-requests branch October 14, 2026 22:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working Server Issues & PRs related to the Server component

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Server] Parallel elicitations on one session get each other's answers

3 participants