Skip to content

[Server] Add outgoing (elicitation, sampling) request and client response events - #564

Merged
chr-hertel merged 2 commits into
modelcontextprotocol:mainfrom
chr-hertel:pr-386-elicit-lifecycle-events
Oct 10, 2026
Merged

chr-hertel merged 2 commits into
modelcontextprotocol:mainfrom
chr-hertel:pr-386-elicit-lifecycle-events

Conversation

@chr-hertel

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

Copy link
Copy Markdown
Member

Takes over #386, rebased onto main; the 2026-07-28 part already landed with #528.

  • Add ServerRequestEvent and ClientResponseEvent for handshake-era server-initiated requests and the client's replies.
  • Dispatch ResponseEvent / ErrorEvent when a suspended fiber completes, from inside the fiber, so concurrent calls on one session each report under their own request. No transport callback, no TransportInterface BC break.
  • Fix a handler throwing after its fiber resumed escaping to the transport (stdio listen loop dies, SSE stream ends without a reply): it is now answered with an error response and ErrorEvent, like a handler throwing before it suspends. Same for a throwing result or error listener.
  • Drop a client response to an ID the server is not waiting on, or whose timeout passed: no ClientResponseEvent, and no longer stored in the session, where it would pile up unconsumed. Read-only check against the pending entry's deadline, so no extra session write that could race a concurrent worker.

Closes #386

🤖 Generated with Claude Code

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

Deferred listener exceptions can terminate connections, and unsolicited responses are incorrectly reported as replies.

3 open findings
What changed in this PR

Adds observability events for handshake-era server/client exchanges and deferred Fiber completion.

Changes:

  • Adds server-request and client-response events.
  • Dispatches result events when suspended Fibers complete.
  • Updates tests, documentation, and changelog.
File Description
src/​Server/​Protocol.php Dispatches new lifecycle events and deferred results.
src/​Event/​ServerRequestEvent.php Defines outgoing request events.
src/​Event/​ClientResponseEvent.php Defines incoming client response events.
tests/​Unit/​Server/​ProtocolTest.php Tests event dispatch and concurrent Fibers.
docs/​advanced/​events.md Documents the expanded event API.
CHANGELOG.md Records the new event behavior.

🧠 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
Comment thread src/Server/Protocol.php
Comment thread docs/advanced/events.md Outdated
@chr-hertel
chr-hertel force-pushed the pr-386-elicit-lifecycle-events branch from dbdaff8 to 93a9a20 Compare October 10, 2026 01:01
@chr-hertel
chr-hertel requested a balanced review from Copilot October 10, 2026 01:05

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

Unknown client responses remain persisted indefinitely, allowing unbounded session growth.

1 open finding
3 resolved since last review

🧠 Review effort: Balanced

Comment thread src/Server/Protocol.php Outdated
@chr-hertel chr-hertel added this to the 0.9.0 milestone Oct 10, 2026
@chr-hertel chr-hertel added the Server Issues & PRs related to the Server component label Oct 10, 2026
@chr-hertel
chr-hertel force-pushed the pr-386-elicit-lifecycle-events branch from 93a9a20 to 31db107 Compare October 10, 2026 01:14
@chr-hertel
chr-hertel requested a balanced review from Copilot October 10, 2026 01:15

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

Timed-out and duplicate responses can still be stored, and throwing error listeners can escape resumed Fibers.

1 open finding
1 resolved since last review

🧠 Review effort: Balanced

Comment thread src/Server/Protocol.php Outdated
@chr-hertel
chr-hertel force-pushed the pr-386-elicit-lifecycle-events branch from 31db107 to d145a7c Compare October 10, 2026 01:24
@chr-hertel
chr-hertel requested a balanced review from Copilot October 10, 2026 01:26

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.

🔵 Needs a closer look

A throwing ErrorEvent listener can still escape a resumed fiber and terminate its transport loop.

0 open findings

1 resolved since last review

🧠 Review effort: Balanced

@chr-hertel
chr-hertel force-pushed the pr-386-elicit-lifecycle-events branch from d145a7c to 9e1cf30 Compare October 10, 2026 01:33
@chr-hertel
chr-hertel requested a balanced review from Copilot October 10, 2026 01:34

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

Timeout cleanup can overwrite responses or other session updates saved concurrently by another worker.

1 open finding

🧠 Review effort: Balanced

Comment thread src/Server/Protocol.php Outdated
@chr-hertel
chr-hertel force-pushed the pr-386-elicit-lifecycle-events branch from 9e1cf30 to 36297e4 Compare October 10, 2026 01:42
@chr-hertel
chr-hertel requested a balanced review from Copilot October 10, 2026 01:43

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

Rejected responses can overwrite concurrent session updates, and numeric-string IDs can incorrectly match integer requests.

3 open findings

🧠 Review effort: Balanced

Comment thread src/Server/Protocol.php
Comment on lines +426 to +429
if (!\is_array($pending) || $this->hasTimedOut($pending)) {
$this->logger->warning('Received a client response for an unknown or timed out request ID.', ['message_id' => $messageId]);

return;

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Filed #578 for this

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.

🟢 Approval recommended

The implementation matches the stated lifecycle behavior and includes focused coverage for its concurrency and error-handling paths.

1 open finding
2 resolved since last review

🧠 Review effort: Balanced

@chr-hertel
chr-hertel merged commit b35b52d into modelcontextprotocol:main Oct 10, 2026
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Server Issues & PRs related to the Server component

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants