Conversation
JosephDoUrden
left a comment
JosephDoUrden
left a comment
There was a problem hiding this comment.
Pulled 9ad6cce and ran it locally, node 22, sdk 1.30.0.
npm test in src/everything gives 108 passing on the PR head and 107 on the base cda92bd. Build passes. tsc with the test files included passes too.
The two resources tests are real. I stubbed cleanupSubscriptions to a no-op and both fail on the assertion.
I also drove it through a real SDK Client over InMemoryTransport, two uris and a second session added by hand. Own session gone, the other kept, empty uri pruned. Same with an undefined session id, which is the stdio case.
The server.test.ts one is the problem. server.server.setRequestHandler is the real McpServer method there, not a mock, so .mock is undefined and subscribeHandler is always undefined. It goes straight to the else branch and just calls cleanup with no expect at all.
I made that else branch throw and the test failed, so that is the only branch that runs. With cleanupSubscriptions stubbed to a no-op it still passes. So it does not test the fix.
It also replaced the "should allow multiple servers to be created" test instead of sitting next to it. I would put that one back.
@AbhiPra24 the cheap fix is to go through the real path, Client plus InMemoryTransport, subscribeResource, then cleanup(transport.sessionId) and check getSubscriptions. Or drop the server test and keep only the resources ones, which already cover it.
Worth saying there are other PRs on #4710. #4712, #4715 and #4716, plus #4711 which #4716 replaces. All of them name it removeSubscriber like the issue asked, this one is cleanupSubscriptions. Only #4712 and this one carry tests. @olaservo your call which shape you want, just flagging so nobody reviews four of these.
Fix is right, the server test needs one more pass before I would call it merge-ready.
syncRoots caches one Root[] per session in the module-level map at server/roots.ts:8, and that cache is the only thing deciding whether a session's roots get fetched again. cleanup(sessionId) at server/index.ts:109 stops the simulated logging and resource-update intervals and clears the task store, but never removes the session's entry, so on an SSE or streamable-HTTP server the map grows by one entry per past connection for the life of the process. The sibling map in resources/subscriptions.ts has the same gap, and five open pull requests fix it: modelcontextprotocol#4798, modelcontextprotocol#4718, modelcontextprotocol#4716, modelcontextprotocol#4715 and modelcontextprotocol#4712. Not one of them touches roots.ts, so fixing subscriptions leaves this identical leak alive next to it. Tests: 108 passed. One new case sets an entry, calls cleanup, and asserts the entry is gone. Reverting roots.ts and index.ts and keeping the test gives 1 failed, 5 passed in server.test.ts. npm run build is clean.
3 participants
Description
Fixes #4710
When a client session closes or disconnects, its session ID remained in the
subscriptionsmap indefinitely. This patch introducescleanupSubscriptions(sessionId)to remove the disconnected session from tracked URI subscriptions (and prune empty URIs from the map) and invokes it during server cleanup (cleanup(sessionId)). Also cleans up empty subscriber sets on explicit unsubscribe requests.Changes
src/everything/resources/subscriptions.ts: AddedcleanupSubscriptionsto remove all subscriptions for a session ID and delete empty URI entries; deleted empty URI sets inUnsubscribeRequestSchemahandler; exportedgetSubscriptionsfor inspection/testing.src/everything/server/index.ts: AddedcleanupSubscriptions(sessionId)tocleanupcallback returned bycreateServer.src/everything/__tests__/resources.test.tsandsrc/everything/__tests__/server.test.tsto verify session subscription cleanup on disconnect.Verification
npm --workspace=@modelcontextprotocol/server-everything testpassed (108 tests passing).npm run buildpassed.