Repository navigation
mcp: clear a resource subscription when its listen stream ends - #1283
Merged
guglielmo-san merged 4 commits intoOct 6, 2026
Conversation
Under SEP-2575, ClientSession.Subscribe opened a subscriptions/listen stream per resource URI but never observed its completion: the transport call was fire-and-forget, so when the stream ended for any reason other than a client Unsubscribe (server teardown, resource revocation, a dropped connection) the resourceSubs entry was left behind. A later Subscribe for the same URI then saw the stale entry and returned as a no-op, so the subscription could never be re-established. This diverged from the python and typescript SDKs, whose clients await the listen and re-open on a bare re-subscribe. Await the listen call on its own goroutine (Subscribe stays non-blocking) and, when it completes while its listen context has not been client-cancelled, clear the resourceSubs entry so a bare re-Subscribe re-opens the stream. The map value becomes a small *resourceSub carrying a per-session generation, so a listen goroutine only clears the entry it created and never one a racing Unsubscribe->Subscribe (or a re-subscribe from a completing listen) has since installed; context.CancelFunc values are not comparable, so the previous map type could not express this guard. The SDK does not auto-resubscribe: a permanently revoked URI would hot-loop, so reopening is left to the application. The legacy (pre-2575) resources/subscribe path and the wire protocol are unchanged.
…ubscriptionsListen method to unify error handling and cleanup.
guglielmo-san
approved these changes
Oct 5, 2026
Contributor
|
@jeremy I added some code on top of your PR to simplify the overall implementation of subscriptions/listen |
nahapetyan-serob
approved these changes
Oct 6, 2026
This was referenced Oct 8, 2026
Closed
guglielmo-san
added a commit
that referenced
this pull request
Oct 10, 2026
The subscription registries are keyed by session, so a second `subscriptions/listen` that subscribes to a URI the session already holds, or opts in to a list-changed notification the session already receives, replaces the first listen's entry with its own request ID. The first listen's teardown then deleted that entry by session, and the second listen stayed open with nothing delivered to it. This SDK's own client reaches it on 2026-07-28 over stdio or any other persistent transport; over streamable HTTP two listens never share a session. `ClientSession.Unsubscribe` only cancels the listen, so a `Subscribe` for the same URI right after it can be acknowledged before the server has unwound the first listen, whose deferred `unsubscribe` then removes what the second listen has just registered. Over the in-memory transport and without `-race`, `Subscribe`, `Unsubscribe` and `Subscribe` again lost the subscription in 85 to 110 of 200 attempts in each of five runs on `main`, and in none with this change; under `-race` the window narrows to 0 to 4 in 200. #1283 guards the client's own table against the same race with a generation number; this is the server's half. The change follows the guards #1275 added: - The teardown of the three list-changed registries deletes an entry only while it still holds this listen's request ID. - A listen's deferred resource unsubscribe goes through a new internal `endSubscription`, which removes `resourceSubscriptions[uri][session]` only while it holds this listen's request ID. `resources/unsubscribe` calls it with the zero ID and removes the session's subscription whichever request made it, as before. - The `UnsubscribeHandler` is still called for every URI of every listen that ends, so its calls stay paired with the `SubscribeHandler`'s. Skipping it when a later listen holds the URI would leave a handler that counts listens one unsubscribe short. I kept it this way on purpose; say if you would rather it ran only when the session's entry goes away. The other side of it: a handler that keeps a set of subscribed sessions, as the field's doc reads, now sees an unsubscribe for a URI the session is still subscribed to. If you keep this, I can add a sentence to the `UnsubscribeHandler` doc saying it is called once for each listen that ends. There are no exported API changes, and the keying by session stays. What this does not change is two listens held open at once on one URI: the later one receives every update, and if it ends first the earlier one receives nothing, as on `main` today. That needs the table keyed per listen, and #1359 sets out that shape and one more for you to choose from. This change stands on its own if you keep the keying by session, and the per-listen keying would replace it rather than conflict with it. Tests, next to the existing `TestSubscriptionsListen_*` ones and reusing their fixtures: - `TestSubscriptionsListen_ResubscribeOutlivesEarlierTeardown` drives the Go client through `Subscribe`, `Unsubscribe` and `Subscribe`, and holds the first listen's teardown in the `UnsubscribeHandler` until the client has seen the second listen acknowledged, so the losing order happens every time. It then checks that `ResourceUpdated` reaches the second listen with that listen's subscription ID, and that unsubscribing the second listen still retires the subscription. - `TestSubscriptionsListen_ListChangedOutlivesEarlierTeardown` opens a second listen next to the auto-listen for each of tools, prompts and resources list-changed, ends the auto-listen, and checks that the change reaches the second listen. Against `main` at 8dd5d6a both fail, the second in all three subtests (`timed out waiting for "updated"`, then `"tool"`, `"prompt"` and `"resource"`), and with the change they pass. Taking out any one of the four guards fails the test for that registry, making a listen never retire its resource entry fails `TestSubscriptionsListen_TeardownKeepsOtherListens`, `TestSubscriptionsListen_ResubscribeOutlivesEarlierTeardown`, `TestResourceSubscriptions_InMemory` and `TestResourceSubscriptions_MultipleURIs`, and making `resources/unsubscribe` keep the entry fails `TestEndToEnd`. `go test ./...` on Go 1.25.14 and `go test -race ./...` on Go 1.26.8 and 1.27.1 pass, the new tests passed 20 runs in a row under `-race`, `gofmt -l` and `go vet ./...` are clean, and so is `staticcheck` v0.6.1 on Go 1.26.8. @cgair named this case in the Known limitations of #1275 and showed it there with failing tests; this takes the part of it that the Go client reaches. Updates #1359 Co-authored-by: Guglielmo Colombo <guglielmoc@google.com>
This was referenced Oct 27, 2026
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.
Problem
Under SEP-2575 the client treated
subscriptions/listenas fire-and-forget. The default sending handler issued the call, returned an empty result immediately, and only watched the listen context so it could sendnotifications/cancelled. Nothing observed the stream ending, which caused three problems:SubscriberegistersresourceSubs[uri]and opens a listen. When that listen ended for any reason other thanUnsubscribe(the server tearing it down, the resource being revoked, the connection dropping), the entry stayed. Every laterSubscribefor the URI then hit the "already subscribed" guard and did nothing. An entry added bySubscribeon a closing session was never removed either.defer cancel()) cancelled the stream as soon asnextreturned.UnsubscribeorCloseit sentnotifications/cancelledeven for a stream the server had already closed.This brings the Go SDK in line with the Python and TypeScript SDKs, whose clients await the listen and re-open it on a plain re-subscribe.
Fix
defaultSendingMethodHandlernow awaitssubscriptions/listenlike any other call, andcallSubscriptionsListenis removed. Cancelling the listen context still sendsnotifications/cancelled, throughcall.Connect(for list-changed notifications) andSubscribe(one listen per resource URI) run the listen on a goroutine throughsubscriptionsListen. The listen therefore goes through the sending middleware chain, and neither method blocks on it.awaitListenclears theresourceSubsentry, so a plainSubscribere-opens the stream.*resourceSub{cancel, gen}, carrying a per-session generation number. A finishing listen therefore only clears the entry it created, never one installed by a racingUnsubscribe→Subscribe.The SDK does not resubscribe on its own, because a revoked URI would loop forever. Reopening is left to the application calling
Subscribeagain. The legacyresources/subscribe/resources/unsubscribepath is unchanged.Behavior changes
There are no exported API changes. The visible differences on
2026-07-28sessions are:nextforsubscriptions/listennow returns when the stream ends or is cancelled, with the stream's actual result or error, instead of an empty result right away. Middleware that applies a per-request timeout, or holds a resource while callingnext, now does so for the life of the stream.Connectno longer fails withopening subscriptions/listenwhen a sending middleware rejects the list-changed listen, andSubscribeno longer returns such an error. A listen rejected by the server was not reported before either. proposal: ClientOptions.ResourceSubscriptionEndedHandler for observing resource-subscription termination #1284 proposes a way to observe that.notifications/cancelledis no longer sent for a listen that has already ended.Follow-up proposal for the optional ended-handler: #1284.