mcp: notify sessions concurrently so one stalled peer cannot starve the rest - #1228
Open
yhxlele wants to merge 2 commits into
Open
mcp: notify sessions concurrently so one stalled peer cannot starve the rest#1228yhxlele wants to merge 2 commits into
yhxlele wants to merge 2 commits into
Conversation
…he rest notifySessions and Server.notifySubscribedSessions delivered a broadcast to its subscribers one session at a time, all under a single shared 10s context. A session whose write did not return promptly held up every session after it, and once the shared context expired the remaining sessions failed with the deadline error without ever being attempted. Because the streamable transport's stream write is a plain http.ResponseWriter write that does not observe the context, a peer that had stopped reading could hold the loop far longer than 10s. Send to each session on its own goroutine with its own deadline, and wait for all attempts before returning, preserving the existing "returns after attempting every session" behaviour. A stalled peer now delays or fails only its own delivery. The test stalls one in-memory peer by never reading its end of the pipe and checks that a second, healthy session still receives the notification while the first is blocked. It fails against the serial implementation. Fixes modelcontextprotocol#1227
yhxlele
force-pushed
the
fix-concurrent-notify
branch
from
September 2, 2026 07:44
29f3fe2 to
cf71822
Compare
| defer wg.Done() | ||
| ctx, cancel := context.WithTimeout(context.Background(), notifyTimeout) | ||
| defer cancel() | ||
| params := makeParams() |
Contributor
There was a problem hiding this comment.
executing this in parallel now introduces a race, as makeParams returns a shallow copy
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
2 participants
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.
notifySessionsandServer.notifySubscribedSessionsdelivered a broadcast to its subscribers one session at a time, all under a single shared 10s context. A session whose write did not return promptly held up every session after it, and once the shared context expired the remaining sessions failed with the deadline error without ever being attempted. Because the streamable transport's stream write is a plainhttp.ResponseWriterwrite that does not observe the context, a peer that had stopped reading could hold the loop far longer than 10s. Details in #1227.This sends to each session on its own goroutine with its own deadline (the same 10s, now a named constant), and waits for all attempts before returning, so
ResourceUpdatedand the list-changed broadcasts keep today's "returns after attempting every session" behaviour. A stalled peer now delays or fails only its own delivery.Not changed here: the stalled write itself is still unbounded (the stream write does not honour the context). That is a transport-level change and can be taken separately; this PR removes the coupling between subscribers, which is the part that affected healthy peers.
Test plan
go test ./mcp/ -count=1 -raceTestNotifySessionsIsolatesStalledPeer— one in-memory peer never reads its end of the pipe so the server's write to it blocks; a second, healthy session must still receive the notification while the first is blocked. Fails against the serial implementation (healthy session was not notified while another session's write was stalled).go vet ./mcp/Fixes #1227