Skip to content

[Server] Drop pending client requests of a vanished stream - #596

Merged
chr-hertel merged 6 commits into
modelcontextprotocol:mainfrom
chr-hertel:fix-abandoned-pending-requests
Oct 14, 2026
Merged

chr-hertel merged 6 commits into
modelcontextprotocol:mainfrom
chr-hertel:fix-abandoned-pending-requests

Conversation

@chr-hertel

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

Copy link
Copy Markdown
Member

Stacked on #581 - merge after it, only the last two commits belong here.

Only the stream that sent a client request clears its pending entry. A stream that is gone, through a client disconnect or a dead worker, leaves the entry and any late answer in the session until it expires.

  • Registering a new client request drops pending entries a minute past their timeout, together with their stored answer
  • Per-key forget(), so it stays compatible with the per-key session saves
  • Within the grace period a live stream still expires its own request and reports "Request timed out"

Fixes chr-hertel#84

🤖 Generated with Claude Code

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

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 stale session snapshot can leave an abandoned request’s concurrently stored response orphaned.

1 open finding
What changed in this PR

Adds cleanup for abandoned client requests and incorporates the prerequisite UUID-based request isolation work from #581.

Changes:

  • Drops timed-out requests and stored answers after a grace period.
  • Uses UUID request IDs and per-transport outgoing queues.
  • Updates transports, tests, fixtures, and changelog documentation.
File Description
src/​Server/​Protocol.php Implements UUID IDs, stream-specific queues, and abandoned-request cleanup.
src/​Server/​Transport/​TransportInterface.php Updates callback types for UUID request IDs.
src/​Server/​Transport/​BaseTransport.php Accepts UUIDs when finding responses.
src/​Server/​Transport/​ManagesTransportCallbacks.php Updates callback type declarations.
src/​Server/​Transport/​StreamableHttpTransport.php Converts pending IDs to UUIDs.
src/​Server/​Transport/​StdioTransport.php Converts pending IDs to UUIDs.
tests/​Unit/​Fixtures/​PollingLoopTransport.php Supports UUID IDs and captured outgoing messages.
tests/​Unit/​Server/​ProtocolTest.php Covers request isolation, cleanup, and UUID behavior.
tests/​Unit/​Server/​ProtocolSessionRaceTest.php Updates concurrency tests for UUID IDs.
tests/​Unit/​Server/​Transport/​StdioSuspendedFibersTest.php Dynamically answers UUID-based requests.
tests/​Unit/​Server/​Transport/​StreamableHttpTransportTest.php Updates pending-request fixtures for UUIDs.
CHANGELOG.md Documents cleanup and UUID API changes.

🧠 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
@chr-hertel
chr-hertel force-pushed the fix-abandoned-pending-requests branch from 551c299 to fae8cb9 Compare October 13, 2026 23:45
@chr-hertel
chr-hertel requested a balanced review from Copilot October 13, 2026 23:47

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

The stacked change alters concurrency-sensitive session persistence and transport routing, warranting final human review.

0 open findings

1 resolved since last review

🧠 Review effort: Balanced

@chr-hertel
chr-hertel force-pushed the fix-abandoned-pending-requests branch from 62c151f to 0410986 Compare October 14, 2026 22:56
@chr-hertel
chr-hertel requested a balanced review from Copilot October 14, 2026 22:57

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

Cleanup can remove requests still awaited by live fibers and leaves persistent null response entries.

2 open findings

🧠 Review effort: Balanced

Comment thread src/Server/Protocol.php Outdated
Comment thread src/Server/Protocol.php
@chr-hertel
chr-hertel force-pushed the fix-abandoned-pending-requests branch from 0410986 to 585572e Compare October 14, 2026 23:13
@chr-hertel
chr-hertel requested a balanced review from Copilot October 14, 2026 23:16

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

Cross-worker cleanup can remove a live request and resume its handler without the expected timeout error.

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.

🟡 Changes recommended

The dropped-request timeout path leaves tracking active, allowing subsequent provider polls to report the same timeout again.

2 open findings
1 resolved since last review

🧠 Review effort: Balanced

Comment thread src/Server/Protocol.php
Comment thread tests/Unit/Server/ProtocolTest.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 focused changes preserve session persistence and timeout handling, with regression coverage and no identified blocking issues.

0 open findings

2 resolved since last review

🧠 Review effort: Balanced

@chr-hertel
chr-hertel merged commit 0bd47e7 into modelcontextprotocol:main Oct 14, 2026
28 checks passed
@chr-hertel
chr-hertel deleted the fix-abandoned-pending-requests branch October 14, 2026 23:37
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] Pending client requests of a vanished stream stay in the session

2 participants