Repository navigation
notify every listen held open on a session - #1490
Merged
guglielmo-san merged 2 commits intoNov 3, 2026
Merged
guglielmo-san merged 2 commits into
guglielmo-san merged 2 commits into
Conversation
Comment on lines
+80
to
+83
| toolChangeSubscriptions map[*ServerSession]jsonrpc.ID // session -> requestID for "tools/changed" | ||
| promptChangeSubscriptions map[*ServerSession]jsonrpc.ID // session -> requestID for "prompts/changed" | ||
| resourceChangeSubscriptions map[*ServerSession]jsonrpc.ID // session -> requestID for "resources/changed" | ||
| resourceSubscriptions map[string]map[*ServerSession][]jsonrpc.ID // uri -> session -> requestID of each subscription |
Contributor
There was a problem hiding this comment.
what do you think about adding the same change to toolChangeSubscriptions, promptChangeSubscriptions and resourceChangeSubscriptions ?
Contributor
Author
There was a problem hiding this comment.
Done in 655fc9f: the three list-changed registries now hold the ID of every open listen per session, as resourceSubscriptions does, through the same addListen and removeListen, and notifySubscribedSessions delivers on each of them. TestSubscriptionsListen_EveryListenIsNotifiedOfListChanges fails for each of the three on main and on the first commit, and passes with this one. I updated the description to match.
resourceSubscriptions holds one request ID per URI and session, so a second subscriptions/listen for a URI the session already holds replaced the first listen's ID with its own. Both listens were acknowledged with the URI, but ResourceUpdated delivered only on the second, and once the second ended its teardown removed the entry and the first, still open, received nothing. The session's entry now holds the ID of every open listen for the URI, and ResourceUpdated delivers the update on each of them under its own subscription ID. A listen's teardown removes only its own ID, and the entry with the last one. resources/subscribe and resources/unsubscribe keep one ID per URI and session, as before, and the SubscribeHandler and UnsubscribeHandler are still called once for each listen. Fixes modelcontextprotocol#1489
toolChangeSubscriptions, promptChangeSubscriptions and resourceChangeSubscriptions held one request ID per session, so a second subscriptions/listen asking for the same list-changed notification replaced the first listen's ID with its own. Only the second was notified, and once it ended its teardown removed the entry and the first, still open, received nothing. Each of them now holds the ID of every open listen, as resourceSubscriptions does, and notifySubscribedSessions takes the same map of session to IDs for both kinds of notification, delivering on each listen under its own subscription ID. addListen, removeListen and cloneListens keep the four registries in step. Updates modelcontextprotocol#1489
jmrplens
force-pushed
the
jmrp-listen-keeps-every-subscription
branch
from
November 3, 2026 11:41
655fc9f to
5bf666d
Compare
guglielmo-san
approved these changes
Nov 3, 2026
Tangerg
added a commit
to Tangerg/go-sdk
that referenced
this pull request
Nov 5, 2026
Sync 109 upstream commits through f91620f, including the same-origin redirect restriction (modelcontextprotocol#1440), linear-time WWW-Authenticate parsing (modelcontextprotocol#1456), OIDC issuer validation (modelcontextprotocol#1442), per-listen notification (modelcontextprotocol#1490) and caller-params preservation (modelcontextprotocol#1473). The fork keeps its own module path, exact encoded-JSON Meta, strict ResourceContents encoding and the transport cancellation and frame budget fixes. Upstream tests that used map-based Meta read it through decodeMetaValue. The protocol number precision suite from the retired preserve-protocol-json-numbers branch now guards the exact-number contract against future syncs. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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.
resourceSubscriptionsholds one request ID per URI and session, so on a 2026-07-28 session a secondsubscriptions/listenfor a URI the session already holds replaced the first listen's ID with its own. Both listens are acknowledged with the URI, butResourceUpdateddelivered only on the second, and once the second ended the first, still open, received nothing. The three list-changed registries did the same to two listens asking for one list-changed notification.This is the second shape from #1359, keeping the keying by session that #1275 kept:
resourceSubscriptions), throughaddListenandremoveListen, andnotifySubscribedSessionsdelivers on each with its ownsubscriptionId.resources/subscribeandresources/unsubscribekeep one ID per URI and session, as before.SubscribeHandlerandUnsubscribeHandlerare still called once for each listen, as mcp: retire a listen's subscriptions only while they are its own #1360 left them, so a handler that counts them still holds the session subscribed while one listen remains.There are no exported API changes.
TestSubscriptionsListen_EveryListenOnAURIIsNotifiedholds two listens open on one URI and checks, as it ends them one at a time (later first in one subtest, earlier in the other), which IDs each update reaches, the handler counts, and that the session ends unsubscribed. Onmainat ae26252 both subtests fail withboth listens open: update delivered under subscription IDs [3], want [2 3].TestSubscriptionsListen_EveryListenIsNotifiedOfListChangesdoes the same for a tools, a prompts and a resources list change, and fails the same way for each onmain.TestSubscriptionsListen_URINamedTwiceIsNotifiedOnceandTestServerSubscribe_LegacyResubscribeReplacesIDkeep whatmaindid for a listen naming its URI twice and for a secondresources/subscribe, and each fails if its path always appends.gofmt -l,go vet ./...andstaticcheckv0.6.1 are clean,go test -race ./...passes on Go 1.26.10 andgo test ./...on Go 1.25.14 and 1.26.10, and the listen and subscribe tests pass 20 runs in a row under-race.Three existing tests are flaky under load, on
mainas well as with this change: in batches of 50 on Go 1.25.14TestStreamableStateful_DiscoverDoesNotLeakSessionfailed 0 to 13 times onmainand 0 to 8 with the change, and in batches of 160 under-raceTestResourceSubscriptions_InMemoryandTestResourceSubscriptions_MultipleURIsfailed 1 to 25 times onmainand 1 to 29 with the change. Those two callResourceUpdatedas soon as theSubscribeHandlerruns, before the listen is registered.Fixes #1489