feat(server): SDK-owned resource subscription tracking on McpServer#2440
feat(server): SDK-owned resource subscription tracking on McpServer#2440felixweinberger wants to merge 1 commit into
Conversation
🦋 Changeset detectedLatest commit: ab94a4b The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
@modelcontextprotocol/client
@modelcontextprotocol/codemod
@modelcontextprotocol/core
@modelcontextprotocol/server
@modelcontextprotocol/server-legacy
@modelcontextprotocol/express
@modelcontextprotocol/fastify
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
felixweinberger
force-pushed
the
fweinberger/resource-subscription-tracking
branch
from
8cb0563 to
26fc34c
Compare
| this.server.setRequestHandler('resources/subscribe', async (request, ctx) => { | ||
| await options?.onSubscribe?.(request.params.uri, ctx); | ||
| // A request cancelled while the hook ran must not mutate the set — | ||
| // the protocol suppresses the aborted request's response, so the | ||
| // client would never learn a subscription was recorded. | ||
| ctx.mcpReq.signal.throwIfAborted(); | ||
| this._resourceSubscriptions.add(request.params.uri); | ||
| return {}; | ||
| }); |
There was a problem hiding this comment.
🟡 The resources/subscribe handler installed by trackResourceSubscriptions() adds the client-supplied URI to _resourceSubscriptions with no bound on set size or URI length, so a misbehaving 2025-era client on a long-lived sessionful connection can grow server memory without limit — unlike the SDK's own 2026-era listen router, which caps its analog at DEFAULT_MAX_SUBSCRIPTIONS = 1024. Answering the open question in the PR description: yes, a default cap (with an override, mirroring the maxSubscriptions option on serveStdio/createMcpHandler) would be worth adding for symmetry — or at minimum, document using onSubscribe to enforce a bound, since none of the in-repo consumers or docs recipes pass one.
Extended reasoning...
What this is. The resources/subscribe handler installed by trackResourceSubscriptions() (packages/server/src/server/mcp.ts:590-598) does an unconditional this._resourceSubscriptions.add(request.params.uri). The URI is entirely client-controlled input: nothing bounds the set size, nothing bounds the URI length, and nothing requires the URI to correspond to a registered resource (the handler accepts any string). On a sessionful posture (stdio, sessionful Streamable HTTP) the set lives for the lifetime of the connection, so an untrusted or buggy 2025-era client can send an unbounded stream of resources/subscribe requests with unique, arbitrarily long URIs and grow server memory without limit for as long as the session stays open.
Why nothing else prevents it. assertRequestHandlerCapability has no case for resources/subscribe, and the only mitigation the API offers is the optional onSubscribe veto hook — which every consumer this PR ships leaves unset: examples/resources/server.ts, examples/todos-server/todos.ts, the rewritten recipe in docs/servers/resources.md, and the JSDoc example all call trackResourceSubscriptions() bare (the JSDoc example does show a hook, but for URI-prefix filtering, not a count bound). So the default configuration — the one the docs teach — has no guard at all.
The in-package precedent. The SDK's own 2026-era analog treats exactly this class of client-driven bookkeeping as something that must be bounded: packages/server/src/server/listenRouter.ts:36 defines DEFAULT_MAX_SUBSCRIPTIONS = 1024, both createListenRouter (line ~124) and StdioListenRouter (line ~323) refuse further listens past the cap with a -32603 "Subscription limit reached" error, and both serveStdio and createMcpHandler expose this as a maxSubscriptions option defaulting to 1024. Now that the SDK owns the 2025-era bookkeeping too, the same protection belongs here.
Concrete walk-through. 1) A server calls server.trackResourceSubscriptions() (no options) and serves a sessionful stdio or Streamable HTTP connection. 2) A client sends resources/subscribe with params.uri = 'x://' + 'a'.repeat(1_000_000) + i in a loop, incrementing i. 3) Each request passes straight through the handler (no capability gate for the verb, no registered-resource check, no hook), executes _resourceSubscriptions.add(uri), and returns {}. 4) After N requests the Set holds N megabyte-scale strings; nothing evicts them until the connection closes. The equivalent loop against a 2026-era subscriptions/listen connection is refused after 1024 entries by the listen router.
Addressing the counter-argument. One reviewer position is that this merely restates the author's own open question ("Also open to adding a cap on the set... if reviewers want symmetry") and that the pre-PR hand-rolled Sets in the docs and examples had the identical exposure, so nothing regresses. Both points are true — which is exactly why this is filed as a nit rather than a blocker. But the question was explicitly left for reviewers to answer, and moving the bookkeeping from app code into the SDK changes who is responsible for the bound: an app hand-rolling a Set can be expected to own its own limits, while an SDK-owned default should be safe out of the box, the way the listen router already is. So the answer to the open question: yes, add a default cap.
How to fix. Either (a) accept a maxSubscriptions option on trackResourceSubscriptions defaulting to 1024 (matching the listen router's constant and error behavior — refuse with a ProtocolError once _resourceSubscriptions.size >= max), or (b) at minimum, extend the JSDoc/docs recipe to show an onSubscribe hook that enforces a count bound, so the documented default posture isn't unbounded. Option (a) is preferable for symmetry and because it protects the callers who never read that paragraph. This is not a merge blocker: no correct client hits it, and it is not a regression relative to the pattern the PR replaces.
An app that declares resources.subscribe today gets no help from the
high-level server: it must reach through mcpServer.server to register
the resources/subscribe and resources/unsubscribe handlers, keep its
own Set of subscribed URIs, and call server.server.sendResourceUpdated,
because the facade has sendResourceListChanged but no updated sibling.
Declaring the resources.subscribe capability is now the whole opt-in:
at connect() time, McpServer installs the resources/subscribe and
resources/unsubscribe handlers when neither verb has one yet, and
records subscribed URIs in the new read-only per-connection
resourceSubscriptions set. Deferring the install to connect keeps
hand-registered handlers (the previous escape-hatch pattern) winning by
already existing, so servers that answer the verbs themselves keep
their exact behavior; the install is skipped as a pair. connect()
resets the set, so a reused instance cannot leak one connection's
subscriptions into the next, and repeat connects find the first
connect's handlers and only clear the set.
trackResourceSubscriptions({ onSubscribe?, onUnsubscribe? }) is the
explicit configuration path: called before connect it declares the
capability, installs the handlers, and attaches veto hooks that run
before the set changes — a hook throw is returned to the client as the
request's error, and a request cancelled while a hook runs leaves the
set unchanged (the handlers re-check the abort signal). The method is
assert-first: a duplicate call, a manually installed subscribe handler,
or a conflicting low-level resources/list handler throws before any
capability or handler state changes. A sendResourceUpdated facade
completes the notification trio, and Protocol gains a public
hasRequestHandler(method) accessor backing the connect-time check.
On 2026-07-28 connections the two verbs do not exist (clients filter
subscriptions/listen and the serving entries own that bookkeeping), and
the era gate rejects them before any handler runs, so the automatic
install is harmless on dual-era servers.
Apps that declared resources.subscribe without registering handlers
previously advertised the capability while answering -32601; they now
get working subscribe/unsubscribe — documented in the migration guide.
The resources and todos-server examples and the resources guide recipe
now rely on the automatic path; the low-level guide points readers at
it for this pair.
felixweinberger
force-pushed
the
fweinberger/resource-subscription-tracking
branch
from
26fc34c to
ab94a4b
Compare
felixweinberger
changed the title
| async connect(transport: Transport): Promise<void> { | ||
| // A declared `resources.subscribe` capability gets SDK-owned subscription | ||
| // bookkeeping automatically; deferring the install to connect time lets a | ||
| // hand-registered `resources/subscribe` handler win by already existing. | ||
| this._autoTrackResourceSubscriptions(); | ||
| // Per-resource subscriptions are connection state: a reused instance must | ||
| // not carry the previous connection's subscriptions onto a new transport, | ||
| // where they would route unsolicited `notifications/resources/updated`. | ||
| this._resourceSubscriptions.clear(); |
There was a problem hiding this comment.
🟡 The PR description says this ships the explicit opt-in form ("zero behavior change for existing servers", "Breaking Changes: None — the API is opt-in; without it, behavior is byte-for-byte today's"), but the diff ships automatic activation: connect() calls _autoTrackResourceSubscriptions(), which installs the resources/subscribe/resources/unsubscribe handlers for any server that merely declared resources: { subscribe: true } — a behavior change the changeset and migration guide already document. Please update the description (and the Breaking Changes / types-of-changes framing) to reflect the automatic activation, so the auto-vs-explicit design decision — the one the description says was "deliberately left open" — gets reviewed as what actually shipped.
Extended reasoning...
What the mismatch is. The PR description's "Additional context" section states: "One design question deliberately left open for review: whether declaring resources: { subscribe: true } should enable tracking automatically instead of requiring the explicit call. This PR ships the explicit form — zero behavior change for existing servers", and the Breaking Changes section says "None — the API is opt-in; without it, behavior is byte-for-byte today's." The code in the diff does the opposite: McpServer.connect() (packages/server/src/server/mcp.ts:149-157) unconditionally calls _autoTrackResourceSubscriptions(), and that helper (~lines 645-655) installs the resources/subscribe and resources/unsubscribe handlers for any server whose declared capabilities include resources.subscribe — no explicit trackResourceSubscriptions() call required.
Concrete walk-through. 1) An existing app constructs new McpServer({...}, { capabilities: { resources: { subscribe: true } } }) and registers no subscribe handlers (a common posture today — the capability was declared, the verbs answered -32601). 2) On this branch, connect() runs _autoTrackResourceSubscriptions(); the capability check passes, neither verb has a handler, so _installResourceSubscriptionHandlers() runs. 3) A client's resources/subscribe now returns {} and the URI is recorded in resourceSubscriptions, where before it returned -32601 Method not found. That is precisely the automatic form the description claims was not shipped, and precisely the "behavior change for existing servers" it claims doesn't exist. The new test 'a declared resources.subscribe capability auto-installs tracking at connect' pins this behavior explicitly.
The rest of the PR agrees with the code, not the description. The changeset (.changeset/track-resource-subscriptions.md) says "Declaring the resources.subscribe capability now activates it automatically at connect()" and carries an explicit "Behavior change:" paragraph (previously -32601, now working SDK-owned handlers). The migration guide (docs/migration/upgrade-to-v2.md) adds a matching bullet, and docs/servers/resources.md is rewritten around "Declaring resources: { subscribe: true } is the whole opt-in." So the shipped artifacts — code, changeset, migration guide, docs, tests — are all internally consistent; only the PR description contradicts them. (The "Ten new tests" count is also stale; the test file has 13.) The description evidently predates the commit that graduated the feature from explicit to automatic and was never refreshed.
Why it matters. The description misleads reviewers on exactly the design question it flags as "deliberately left open for review": a reviewer reading only the description would believe the automatic-activation decision is still pending and that existing capability-declaring servers are unaffected, when in fact both have been decided in the code. It also means the "Breaking change" checkbox / "non-breaking" framing doesn't match the documented -32601 → {} behavior change for existing servers, which is worth a deliberate maintainer sign-off (including whether a "minor" changeset is the right vehicle for it).
How to fix. Update the PR description: state that the automatic form ships (declaring the capability alone activates tracking at connect(), hand-registered handlers still win), revise the Breaking Changes / Types-of-changes sections to acknowledge the documented behavior change for servers that declared resources.subscribe without handlers, and refresh the test count. Alternatively, if the maintainers prefer the explicit-only design the description describes, drop the _autoTrackResourceSubscriptions() call from connect() — but then the changeset, migration guide, docs, and tests all need reverting to match. Nothing in the merged code is wrong or self-inconsistent, so this is not a merge blocker — it's stale review metadata that should be corrected so the design decision is reviewed as what it actually is.
1 participant
A server that declares
resources: { subscribe: true }gets no help from the high-level API today: it must reach throughmcpServer.serverto register theresources/subscribeandresources/unsubscribehandlers, keep its ownSetof subscribed URIs, and callserver.server.sendResourceUpdated, because the facade offerssendResourceListChangedbut noupdatedsibling. Every consumer that serves 2025-era sessions rewrites the same ~20 lines (theresourcesexample andtodos-serverboth did). The go and java SDKs own this bookkeeping in the SDK; on 2026-07-28 connections our serving entries already own the equivalent via thesubscriptions/listenrouter — this brings the 2025-era path to parity.Declaring the capability is now enough. At
McpServer.connect(), a declaredresources.subscribecapability auto-installs the two request handlers when neither verb has one yet: subscribes are recorded in the per-connection read-onlymcpServer.resourceSubscriptionsset (cleared on every connect), unsubscribes remove them, andsendResourceUpdatedcompletes the notification facade. The install is deliberately deferred to connect so hand-registered handlers win by existing — servers using the low-level escape hatch keep their exact behavior, nothing throws at startup, and the auto arm registers no capabilities so it cannot fail for any pre-existing app.trackResourceSubscriptions({ onSubscribe, onUnsubscribe })is the explicit variant for servers that need veto hooks: the hook runs before the set changes and a throw refuses the request (propagated as the JSON-RPC error, with an abort re-check after the await so a cancelled request never records). It must be called before the firstconnect().Motivation and Context
See above — the resources guide recipe and both in-repo consumers hand-rolled identical bookkeeping; the SDK now owns the mechanics while the app keeps the policy (veto). Auto-on was a deliberate call for the v2 window, with the behavior change documented in the migration guide.
How Has This Been Tested?
Seventeen tests over linked client/server pairs: the auto path (declared capability → subscribe/unsubscribe work at connect, URIs recorded/removed); no capability and no call →
-32601unchanged; a hand-registered handler wins over the auto-install (manual response observed, SDK set stays empty); auto-tracking over low-level servers that answer list/read themselves; veto on subscribe and unsubscribe (client receives the error, set unchanged); a request cancelled during the hook does not record;sendResourceUpdateddeliversnotifications/resources/updated; duplicate explicit call throws; capability auto-declaration and merge on the explicit path; post-connect explicit call throws; conflict atomicity (no capability half-declared); reconnect idempotence (no duplicate-handler throw, set reset each connect). Full workspace suite: server 417, integration 348, e2e 2633 (+155 pre-existing expected-fail), all packages green; docs gates clean; live probes of all four auto semantics through a real client.Breaking Changes
One behavioral change, documented in
docs/migration/upgrade-to-v2.md: servers that declaredresources: { subscribe: true }but registered no handlers previously advertised the capability while answering-32601 Method not found; they now get working SDK-owned subscription semantics. Hand-registered handlers are unaffected. Tests pinning-32601for that misconfiguration need re-baselining.Types of changes
Checklist
Additional context
An earlier revision shipped this as explicit opt-in; it was promoted to capability-driven auto-enable during review (the v2 launch window permits the behavior change, and the connect-time deferral makes it conflict-free). The examples simplify to a constructor capability declaration with no explicit call; the docs recipe leads with the auto path and shows
trackResourceSubscriptionsfor the veto-hook variant.Protocol.hasRequestHandler(method)is added as the public probe the auto-install needs (andassertCanSetRequestHandlernow delegates to it). One sharp edge, documented: callingtrackResourceSubscriptions()after a connect has auto-installed throws the duplicate-registration error — attach hooks before the first connect.