Repository navigation
fix(core): remove pending response entries on request timeout and cancellation - #1134
Lubaoshuai wants to merge 2 commits into
Conversation
Sorry, something went wrong.
…cellation McpClientSession and McpServerSession left the pendingResponses entry in place when the downstream timeout fired or the caller cancelled, so each timed-out request leaked its entry forever (request IDs are unique). Mirror the doOnError/doOnCancel cleanup the streamable session variant already performs after .timeout(requestTimeout). Fixes modelcontextprotocol#1133
|
The cleanup placement looks consistent with the streamable-session pattern, and the idempotent remove makes the duplicate send-error cleanup harmless. One regression-coverage gap seems worth closing before merge: this PR changes both McpClientSession.sendRequest and McpServerSession.sendRequest, and the stated fix covers timeout + downstream cancellation, but the added test exercises only client timeout. I would add at least:
If inexpensive, exercising cancellation on both client and server would pin the exact contract this PR is adding. The existing reflection-based assertion style is sufficient for this focused regression; no public test hook seems necessary. That would protect against a future refactor preserving timeout cleanup while accidentally dropping the doOnCancel behavior, and would also prove the server-side change rather than relying on symmetry with the client. |
Sorry, something went wrong.
…se cleanup Per review: the fix touches both McpClientSession and McpServerSession but only client-side timeout had regression coverage. Add: - McpServerSessionTests: pendingResponses is empty after request timeout, and after the subscriber cancels before a response arrives - McpClientSessionTests: pending entry removed on subscriber cancel spring-javaformat applied; mcp-core targeted suite: 15/15 green. Signed-off-by: Lubaoshuai <128781758+Lubaoshuai@users.noreply.github.com>
|
Thanks for the careful review — agreed on both points. Pushed in 6be0c53:
All three use the same reflection-based pendingResponses assertion as the existing client timeout test. spring-javaformat:apply run on the new file, and the targeted mcp-core suite is green (15/13+2 tests). |
Sorry, something went wrong.
|
Verified the updated head. The new server-side timeout regression is present, and cancellation cleanup is now covered on both client and server with the pending map asserted non-empty before cancellation and empty after disposal. That closes the coverage gap I raised. Thanks for adding the symmetric regressions. |
Sorry, something went wrong.
Fixes #1133
McpClientSession.sendRequest and McpServerSession.sendRequest left their pendingResponses entry in place when the downstream .timeout(...) fired or the caller cancelled — the entry was only removed on a response or a send failure. Since request IDs are unique per request, a timed-out request leaked its entry forever; KeepAliveScheduler pings amplify this into one leaked entry per interval on sessions with a dead peer.
Both methods now mirror the cleanup pattern already used by McpStreamableServerSession.McpStreamableServerSessionStream.sendRequest: doOnError + doOnCancel remove the entry after the timeout boundary. Removal is idempotent, and a late response for a removed id already takes the existing "unexpected response" path.
How verified