Repository navigation
Cancel open subscriptions/listen requests when the transport stops delivering messages - #1933
Open
pournasserian wants to merge 1 commit into
Conversation
…livering messages A subscriptions/listen handler only completes when its request is cancelled. Once the transport's input ends (stdin EOF on stdio), no notifications/cancelled can arrive, so ProcessMessagesCoreAsync waited for it forever and RunAsync never returned, leaving the server process alive. Cancel in-flight subscriptions/listen requests before waiting for in-flight handlers; other handlers still run to completion. Fixes modelcontextprotocol#1914
This branch has not been deployed
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 #1914
Problem
When a stdio server's stdin reaches end-of-stream,
McpSessionHandler.ProcessMessagesCoreAsyncleaves its read loop and waits in itsfinallyblock for every in-flight handler to finish. Asubscriptions/listenhandler (built-in or a customSubscriptionsListenHandler) only completes when its request is cancelled. Once the transport has stopped delivering messages, nonotifications/cancelledcan arrive, and the handler's token is linked only to the loop token, which nothing cancels at EOF. So the wait never ends,RunAsyncnever returns, and the server process stays alive. Clients that opensubscriptions/listenon every connection leak one server process per closed session.Fix
McpSessionHandlernow tracks in-flightsubscriptions/listenrequests separately. In the read loop'sfinally, before it waits for in-flight handlers, it cancels those requests. This covers the built-in handler and any custom handler that honours its token.Other in-flight handlers (e.g. a long
tools/call) still run to completion as before, so side-effecting work isn't cut short at EOF. As noted on the issue, cancelling every handler when the transport completes would also cover that case, but it's a broader behaviour change, so I've left it out of this PR.Streamable HTTP is unaffected in practice: its session transports only complete their channel on dispose, and
StreamableHttpSession.DisposeAsyncalready cancels the server right after disposing the transport.Tests
Added to
RawStreamConformanceTests:InputEndOfStream_WithOpenSubscriptionsListen_CompletesRunAsync: sendsserver/discoverandsubscriptions/listen, waits for the acknowledgement, completes the input pipe, and assertsRunAsyncreturns without its token being cancelled.InputEndOfStream_WithOpenCustomSubscriptionsListen_CompletesRunAsync: the same scenario with a customSubscriptionsListenHandlerthat waits for its token.Both time out without the fix and pass with it.
ModelContextProtocol.Testspasses on net10.0, net9.0, net8.0 and net472, andModelContextProtocol.AspNetCore.Testspasses on net10.0 and net9.0, except for tests that need the npm packages (ClientIntegrationTestsagainst the "everything" server and theConformanceTests), which I couldn't run locally withoutnpm ci.