Repository navigation
Conversation
…reateMcpHandler (modelcontextprotocol#3036) Adds `maxSubscriptionsPerSession` and `getSessionId` to createMcpHandler. Open subscriptions/listen streams are counted per session key returned by `getSessionId(options, request)` and compared against the cap before the SSE ack; refusals return the same -32603 "Subscription limit reached" as the handler-wide `maxSubscriptions` guard. The resolver consumes the pass-through `McpHandlerRequestOptions` (whose `authInfo` reflects what trusted middleware established) and the `Request`, so deployments pick the trust source; the SDK never derives the key from inbound request headers directly. Fail-closed: - `createMcpHandler` throws `TypeError` at construction when `maxSubscriptionsPerSession` is set without `getSessionId`. - The listen router refuses pre-ack when the resolver returns `undefined` for a given listen, instead of silently falling back to the handler-wide cap and claiming per-session isolation that was never applied. Teardown on client cancel, graceful handler.close(), and the immediate completion of an empty-honored filter all release the per-session slot. Closes modelcontextprotocol#3036.
🦋 Changeset detectedLatest commit: ede2be2 The changes in this PR will be included in the next version bump. This PR includes changesets to release 6 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 |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
@modelcontextprotocol/client
@modelcontextprotocol/codemod
@modelcontextprotocol/core
@modelcontextprotocol/server
@modelcontextprotocol/server-legacy
@modelcontextprotocol/express
@modelcontextprotocol/fastify
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 177a109e50
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Deployments decide the trust source for identity (typically a | ||
| // field stamped onto authInfo by their auth middleware); the SDK | ||
| // never derives the key from request headers directly. | ||
| const sessionId = options.getSessionId?.(requestOptions ?? {}, request); |
There was a problem hiding this comment.
When getSessionId is supplied without maxSubscriptionsPerSession (which the option docs say should ignore it), this still calls the resolver for every subscriptions/listen. If it throws for a request, serveModern turns an otherwise valid listen into a 500 despite there being no per-session check; resolve the key only when the cap is configured.
Useful? React with 👍 / 👎.
|
@yaohuangguan this is the (b) fail-closed shape we converged on in #3036 on Oct 20. The four constraints landed verbatim with a test per constraint; build, typecheck, lint, and the 19 subscription tests are green at |
claude
Bot
added
the
v2
…ntextprotocol#3157) The JSDoc on `getSessionId` states the resolver is ignored when `maxSubscriptionsPerSession` is unset, but `serveModern` was calling the resolver unconditionally for every `subscriptions/listen`. A user-supplied resolver that throws would turn an otherwise valid listen into a 500, contradicting the documented contract. Gate the resolver call on `maxSubscriptionsPerSession !== undefined` so the resolver only runs when its result would actually be consumed. Add a test that configures a throwing resolver with no cap set and asserts the listen succeeds and the resolver is never invoked. Flagged by Codex review on PR modelcontextprotocol#3157.
|
Fixed in |
Closes #3036.
Summary
Adds two new options to
createMcpHandler:maxSubscriptionsPerSession?: number: the per-session cap on opensubscriptions/listenstreams, enforced alongside the existing handler-widemaxSubscriptionscap. Whichever fires first wins, and both refuse pre-ack with the same-32603 'Subscription limit reached'in-band JSON-RPC error on HTTP 200.getSessionId?: (options: McpHandlerRequestOptions, request: Request) => string | undefined: the resolver that supplies the trusted session key for bucketing. Signature usesMcpHandlerRequestOptionsso deployments readauthInfo(what trusted middleware established), not raw inbound headers. The SDK does not authenticate the returned value; the deployment picks the trust source.The listen router stays header-agnostic.
createMcpHandler.serveModernresolves the session key via the caller-supplied resolver and passes it tolistenRouter.serve(..., sessionId?); the router enforces the per-session cap on anopenPerSession: Map<string, Set<teardown>>that is grown and released alongside the existingopenset, so the handler-wide accounting is unchanged.Design history
Converged on this contract with @yaohuangguan on 2026-10-20 after the design discussion in #3036 established that the modern
createMcpHandlerpath has no server-managed session context (PerRequestHTTPServerTransportdeliberately drops session-id handling), which rules out the naive "key onMcp-Session-Id" shape as a silent default. The implementation here is the fail-closed (b) option from that thread, with the four constraints yaohuangguan proposed and I committed to on 2026-10-21:createMcpHandlerthrowsTypeErrorat construction whenmaxSubscriptionsPerSessionis set withoutgetSessionId. No silent fallback source for per-session attribution.McpHandlerRequestOptions(whoseauthInfois middleware-stamped) and theRequest, so deployments choose the trust source. The SDK never reads inbound headers directly to derive the key.-32603 'Subscription limit reached'when the resolver returnsundefinedfor a configured per-session cap, instead of silently falling back to the handler-wide cap.Tests
packages/server/test/server/createMcpHandlerListen.test.ts: 19/19 pass, including six new cases for the four constraints above:enforces maxSubscriptionsPerSession per resolved session key, independently of other sessionshandler-wide maxSubscriptions still fires across sessions even when per-session cap has roomthrows TypeError at construction when maxSubscriptionsPerSession is set without getSessionIdrefuses pre-ack with -32603 when getSessionId returns undefined for a configured per-session capreleases a per-session slot when the client cancels the listen streamdoes not consume a per-session slot for a listen whose honored filter narrows to emptyThe existing
handler.close() emits the empty subscriptions/listen resulttest continues to cover the graceful-close release path.Changeset
.changeset/max-subscriptions-per-session.mdbumps@modelcontextprotocol/serverminor, documents the fail-closed contract and the deployment-side trust requirement.Scope
Additive. No change to existing
maxSubscriptionsbehavior, no change to the handler-wide accounting, no change to the listen request/response wire shape.listenRouter.servegains an optional fifth parameter (sessionId?: string); the sole in-tree caller is updated.