Skip to content

everything: remove disconnected sessions from resource subscriptions - #4716

Open
vishalhabib99 wants to merge 2 commits into
modelcontextprotocol:mainfrom
vishalhabib99:fix/everything-subscription-cleanup
Open

everything: remove disconnected sessions from resource subscriptions#4716
vishalhabib99 wants to merge 2 commits into
modelcontextprotocol:mainfrom
vishalhabib99:fix/everything-subscription-cleanup

Conversation

@vishalhabib99

Copy link
Copy Markdown

subscriptions (Map<uri, Set<sessionId>>) only ever had entries removed via an explicit resources/unsubscribe call. A session that disconnects without unsubscribing stayed in every Set it had joined for the life of the process, since cleanup(sessionId) stopped logging/update intervals and task-store timers but never touched subscriptions.

Adds removeSubscriber(sessionId), which drops the session from every URI's subscriber set and removes any URI entry left with no subscribers, and calls it from cleanup().

Fixes #4710

(Re-opened as a fresh PR — the original #4711 was closed automatically when my fork was deleted; same fix, no changes.)

subscriptions (Map<uri, Set<sessionId>>) only ever had entries removed
via an explicit resources/unsubscribe call. A session that disconnects
without unsubscribing stayed in every Set it had joined for the life
of the process, since cleanup(sessionId) stopped logging/update
intervals and task-store timers but never touched subscriptions.

Add removeSubscriber(sessionId), which drops the session from every
URI's subscriber set and removes any URI entry left with no
subscribers, and call it from cleanup().

Fixes modelcontextprotocol#4710
Covers the fix for modelcontextprotocol#4710: a session removed via removeSubscriber no
longer receives simulated resource-update notifications, and removing
one session's subscription does not affect other sessions still
subscribed to the same URI.
@vishalhabib99

Copy link
Copy Markdown
Author

Pulled 8e2f0fe, 109 passing vs 107 on base (d73f99e), build and tsc --noEmit on src/everything both clean.

Added two regression tests for removeSubscriber: one confirming a removed session stops receiving simulated resource-update notifications, one confirming removing that session doesn't affect other sessions still subscribed to the same URI. Verified both actually catch the regression — stubbing removeSubscriber to a no-op locally fails the first test, restoring the fix passes it again.

@vishalhabib99

Copy link
Copy Markdown
Author

Rebased onto latest main and CI is green (all Python/TypeScript build+test jobs passing). Added two regression tests for the removeSubscriber fix (a removed session stops receiving resource-update notifications; removing one session doesn't affect others still subscribed to the same URI) — verified both actually catch the regression by stubbing the fix back out locally.

Ready for review whenever you have a chance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

everything: subscriptions map never drops a disconnected session

1 participant