feat(server): closeOnRefusedHandshake option for per-handshake transports#2439
feat(server): closeOnRefusedHandshake option for per-handshake transports#2439felixweinberger wants to merge 2 commits into
Conversation
🦋 Changeset detectedLatest commit: a634cc8 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}
…ansports A sessionful WebStandardStreamableHTTPServerTransport created per handshake attempt (the session-map recipe: a fresh transport and connected server pair per expected initialize) leaked the pair when the handshake was refused: the 4xx went out, onclose never fired, and the consumer's onclose-keyed cleanup never ran. With closeOnRefusedHandshake enabled, handleRequest enforces one invariant at the single public entry: the transport's first completed request either establishes the session or ends the transport. Its finally schedules the close chain on a microtask whenever a settling request leaves a sessionful transport without a session and no other request in flight — covering every refusal path by construction: POST handshake refusals, thrown handshake attempts, pre-session GET/DELETE, and unsupported methods. An active-request counter defers the close while a concurrent request (an initialize still reading its body) is in flight, and the microtask re-checks both the counter and the session state before closing, so a completing initialize is never torn down. The option defaults to false: a long-lived sessionful endpoint answers pre-session refusals (for example the SDK client's version-negotiation probe) without ending the transport, exactly as before. Stateless transports never close on refusals regardless of the option. The sessions guide's session-map recipe adopts the option.
The entry's classification step takes a web-standard Request, extracts the HTTP method and the MCP-Protocol-Version / Mcp-Method / Mcp-Name headers, performs the single body read (distinguishing an unreadable stream from a non-JSON body), and classifies with classifyInboundRequest. It was internal, so hybrid deployments had to hand-assemble the classifier's fields — or settle for the boolean isLegacyRequest and lose the routing reason. Export it, together with its EntryClassification outcome type, as the Request-shaped sibling of classifyInboundRequest. Hybrid compositions can now route on the full outcome (for example: legacy initialize to a sessionful host, everything else to the stateless handler) while reusing the exact code path behind createMcpHandler and isLegacyRequest, including the body-preserving forwardRequest for whichever handler wins. Internal call sites and behavior are unchanged.
felixweinberger
force-pushed
the
fweinberger/refused-handshake-and-classifier
branch
from
273854c to
a634cc8
Compare
felixweinberger
changed the title
| } | ||
| } | ||
| default: { | ||
| return this.handleUnsupportedRequest(); | ||
| } finally { | ||
| // The awaits above mean this runs once the response (or throw) has | ||
| // settled — after a successful initialize has assigned its state. | ||
| this._activeRequests -= 1; | ||
| if ( | ||
| this._closeOnRefusedHandshake && | ||
| this._activeRequests === 0 && | ||
| this.sessionIdGenerator !== undefined && | ||
| !this._initialized && | ||
| this.sessionId === undefined | ||
| ) { | ||
| queueMicrotask(() => { | ||
| if (this._activeRequests === 0 && !this._initialized && this.sessionId === undefined) { | ||
| void this.close().catch(() => {}); | ||
| } | ||
| }); |
There was a problem hiding this comment.
🟡 With closeOnRefusedHandshake: true, a handshake that throws after session state is assigned — e.g. an async onsessioninitialized or eventStore.storeEvent rejecting during the initialize — returns a 400 with no mcp-session-id header, yet the transport considers itself established, so the close chain never runs and the connected server pair leaks. Consider rolling back sessionId/_initialized in handlePostRequest's outer catch when the failure happened while serving the initialization request (or extending the finally predicate), or narrowing the 'a refused or throwing handshake' claim in the changeset and option JSDoc, whose only tested throwing case is a throw before state assignment (throwing sessionIdGenerator).
Extended reasoning...
What the gap is. The new finally in handleRequest schedules the refused-handshake close only when the transport is still never-established: !this._initialized && this.sessionId === undefined. But handlePostRequest assigns that state before two awaits inside the same handshake that can reject: this.sessionId = this.sessionIdGenerator?.(); this._initialized = true; is followed by await Promise.resolve(this._onsessioninitialized(this.sessionId)), and later in the same request by await this.writePrimingEvent(...) → eventStore.storeEvent(). Both onsessioninitialized and storeEvent are Promise-typed user hooks precisely so they can hit an async session store or database, which can transiently reject.
The code path. If either await rejects, the throw lands in handlePostRequest's outer catch, which answers with createJsonErrorResponse(400, -32700, 'Parse error', ...). That response is built before the initialize is ever dispatched via onmessage and carries no mcp-session-id header, so the client's handshake has failed and — in the session-map recipe this option targets — nothing can ever route another request to this transport instance; the client will re-initialize on a fresh transport. Yet the finally predicate now sees _initialized === true and sessionId assigned, so no close is scheduled: onclose never fires and the just-connected Server (plus anything the app paired with it) leaks — the exact leak class the option was added to eliminate. If onsessioninitialized rejected before completing its sessions.set, there is not even a map entry for the documented shutdown loop to close.
Why existing coverage doesn't catch it. The PR's throwing-handshake test uses a throwing sessionIdGenerator, which throws before the state assignment — the predicate still matches there, so the close fires. There is no test for a rejection after state assignment. Meanwhile the changeset (.changeset/refused-handshake-fires-close.md) and the option JSDoc claim that 'a refused or throwing handshake' schedules the close chain, which the code does not back for post-assignment throws — a prose-vs-implementation mismatch.
Step-by-step proof. 1) Consumer builds a per-handshake transport with { sessionIdGenerator, closeOnRefusedHandshake: true, onsessioninitialized: async id => { await sessionStore.put(id, ...) } } and connects a fresh server. 2) The client's initialize POST arrives; handlePostRequest passes header checks, parses the body, recognizes the initialization request, assigns this.sessionId and sets this._initialized = true. 3) await Promise.resolve(this._onsessioninitialized(this.sessionId)) rejects (transient store outage). 4) The outer catch returns 400 Parse error — no mcp-session-id header, so the client never learns the id and re-initializes elsewhere. 5) handleRequest's finally evaluates the predicate: _initialized is true → no close scheduled. 6) onclose never fires; the transport + connected server pair is orphaned until process exit. The same sequence applies to a rejecting eventStore.storeEvent() inside writePrimingEvent on the initialize's response stream.
Impact and mitigations. The leak requires the opt-in flag plus an async onsessioninitialized/eventStore rejection at exactly the handshake; the documented recipe uses a synchronous sessions.set that cannot reject, and pre-PR every refused handshake leaked anyway — so this is a residual coverage gap in the new mitigation plus an over-broad claim, not a regression.
How to fix. Either (a) roll back sessionId/_initialized in handlePostRequest's outer catch when the failure happened while serving the initialization request (or extend the finally predicate to treat an initialize whose own request ended in the error path as not-established), and add a test with a rejecting async onsessioninitialized; or (b) narrow the changeset/JSDoc wording so 'throwing handshake' is scoped to throws before session state is assigned.
| if (!sessionId && isInitializeRequest(req.body)) { | ||
| const transport = new NodeStreamableHTTPServerTransport({ | ||
| sessionIdGenerator: () => randomUUID(), | ||
| // This pair exists for one expected handshake: if the | ||
| // initialize is refused, close the transport (onclose below | ||
| // runs) so the just-connected server never leaks. | ||
| closeOnRefusedHandshake: true, | ||
| onsessioninitialized: id => { | ||
| sessions.set(id, transport); | ||
| } |
There was a problem hiding this comment.
🟡 The runnable session-map examples that use this exact per-handshake recipe (examples/legacy-routing/server.ts, examples/repl/server.ts, examples/standalone-get/server.ts, examples/sse-polling/server.ts, examples/elicitation/server.ts) were not updated with closeOnRefusedHandshake and carry no manual close-on-refused-handshake guard, so they still leak the just-connected server on a refused handshake and now diverge from the guide snippet documented as mined from legacy-routing/server.ts. Consider adding closeOnRefusedHandshake: true to those examples (or noting why they were left out); the PR description's claim that the examples currently carry a manual guard for this is not backed by anything in the tree.
Extended reasoning...
What the finding is. This PR adds closeOnRefusedHandshake and applies it to the session-map recipe in docs/serving/sessions-state-scaling.md and its typecheck-only companion examples/guides/serving/sessions-state-scaling.examples.ts (whose sessions_routing region is documented as "mined from examples/legacy-routing/server.ts"). However, the runnable examples that use the exact same per-handshake recipe were not updated: examples/legacy-routing/server.ts:39-48, examples/repl/server.ts:266-276, examples/standalone-get/server.ts:65-74, examples/sse-polling/server.ts:105-117, and examples/elicitation/server.ts:254-263 all construct a fresh NodeStreamableHTTPServerTransport with a sessionIdGenerator, connect a fresh McpServer, and serve one expected initialize — with neither closeOnRefusedHandshake nor any manual close-on-refused-handshake guard.\n\nThe code path. In each of those examples, the !sessionId && isInitializeRequest(req.body) branch connects a just-built server to a just-built transport before handing the request to transport.handleRequest. If that first request is refused (a schema-invalid initialize, a missing Accept header, a batch initialize), the transport answers 4xx but never runs its close chain, so onclose never fires and the connected server — and the sessions-map bookkeeping around it — leaks until process exit. That is precisely the leak this PR's changeset and docs describe fixing for this recipe.\n\nStep-by-step example (legacy-routing/server.ts). (1) A client POSTs an initialize with Accept: application/json only. (2) The route sees no session id and isInitializeRequest(req.body) is true, so it builds a transport (no closeOnRefusedHandshake), connects a fresh server, and calls handleRequest. (3) The transport refuses with 406; onsessioninitialized never runs and onclose never fires. (4) The server/transport pair stays connected in memory forever — the option that would close it on this path exists after this PR, but the example doesn't set it.\n\nWhy this diverges from the docs. The updated guide snippet now recommends closeOnRefusedHandshake: true for exactly this recipe and explains that without it "the just-connected server never leaks" — while docs/serving/legacy-clients.md links legacy-routing/server.ts as running "this exact branch" and the guide companion documents itself as mined from that file. So after this PR the guide and the runnable examples it points at disagree, and a reader following the linked example ships the leak the PR set out to fix. Additionally, the PR description's statement that "the examples currently carry a manual 'close on refused handshake' guard for exactly this" is not backed by anything in the tree — a repo-wide grep for closeOnRefusedHandshake (or any equivalent manual guard) in examples/ hits only the guide companion.\n\nImpact and severity. Nothing breaks if merged as-is: the option defaults to false, the examples still compile and serve, their e2e legs never exercise a refused handshake, and the leak is pre-existing behavior rather than a regression. This is a completeness/consistency follow-up, not a blocker.\n\nHow to fix. Add closeOnRefusedHandshake: true to the transport options in the five runnable session-map examples (matching the guide snippet, ideally with the same short comment), or state in the PR why they were intentionally left out. Also consider correcting the PR description's claim about a pre-existing manual guard.
| // A throwing sessionIdGenerator lands the handshake in | ||
| // handlePostRequest's outer catch before any session state was | ||
| // written — the 400 there is a refusal like the explicit paths. | ||
| const throwingTransport = new WebStandardStreamableHTTPServerTransport({ | ||
| sessionIdGenerator: () => { | ||
| throw new Error('generator boom'); | ||
| }, | ||
| closeOnRefusedHandshake: true | ||
| }); | ||
| const throwingServer = new McpServer({ name: 'test-server', version: '1.0.0' }, { capabilities: {} }); | ||
| await throwingServer.connect(throwingTransport); | ||
| const protocolOnClose = throwingTransport.onclose; | ||
| const closeSpy = vi.fn<() => void>(); | ||
| throwingTransport.onclose = () => { | ||
| closeSpy(); | ||
| protocolOnClose?.(); | ||
| }; | ||
|
|
||
| const response = await throwingTransport.handleRequest(createRequest('POST', TEST_MESSAGES.initialize)); | ||
|
|
||
| expect(response.status).toBe(400); | ||
| await flushCloseChain(); | ||
| expect(closeSpy).toHaveBeenCalledTimes(1); | ||
| }); | ||
|
|
||
| it('fires onclose when a pre-session GET is refused on a never-established transport', async () => { | ||
| const response = await transport.handleRequest(createRequest('GET', undefined, { accept: 'application/json' })); |
There was a problem hiding this comment.
🟡 The new "handshake attempt itself throws" test pins a pre-existing misclassification: handlePostRequest's outer catch answers a throwing sessionIdGenerator (a server-internal failure) with 400 / -32700 Parse error, and the test's expect(response.status).toBe(400) plus the comment calling it "a refusal like the explicit paths" codify that as intended, so a later fix to 500 / -32603 would break this test. Consider softening the assertion/comment (assert only the onclose behavior, or match on a range), or — better — discriminating the thrown cause in the outer catch; the catch itself predates this PR.
Extended reasoning...
What the finding is. handlePostRequest in packages/server/src/server/streamableHttp.ts wraps its entire body in one broad try/catch whose catch hard-codes createJsonErrorResponse(400, -32_700, 'Parse error', { data: String(error) }) for any thrown cause. That includes server-internal failures — a throwing sessionIdGenerator, a rejecting onsessioninitialized, or a failing eventStore.storeEvent in writePrimingEvent — none of which are client parse faults. Per JSON-RPC semantics, -32700 (Parse error) tells the client its bytes were malformed; a server-side configuration or callback failure belongs to -32603 Internal error over HTTP 500, so clients don't pointlessly reformat/retry a request that was fine.\n\nHow this PR interacts with it. The catch itself is untouched by this PR — it predates it. But the new test fires onclose when the handshake attempt itself throws on a never-established transport (packages/server/test/server/streamableHttp.test.ts:1436-1462) constructs a transport whose sessionIdGenerator throws, asserts expect(response.status).toBe(400), and its comment explicitly frames the outcome: "the 400 there is a refusal like the explicit paths." That turns an incidental artifact of the broad catch into documented, test-pinned intended behavior. The changeset text likewise advertises the "throwing handshake" path as one of the covered refusals.\n\nThe specific code path (step-by-step proof).\n1. The test POSTs a valid initialize (TEST_MESSAGES.initialize) with correct Accept/Content-Type headers to the throwing transport.\n2. handlePostRequest passes header checks, parses the body successfully, and passes the JSON-RPC schema parse — so neither of the two intentional -32700 branches (invalid JSON, invalid JSON-RPC message) fires.\n3. isInitializationRequest is true, so it reaches this.sessionId = this.sessionIdGenerator?.() — the generator throws Error('generator boom').\n4. The throw unwinds to the outer catch, which returns 400 / -32700 Parse error with data: "Error: generator boom" — a client-fault answer to a purely server-side failure. The request bytes were perfectly well-formed.\n5. The new test asserts response.status === 400, so any future change of this branch to 500 / -32603 becomes a test-breaking change to this PR's test, even though the fix would not affect the feature under test (the finally-based close fires regardless of status).\n\nWhy nothing existing prevents it. The intentional parse-error branches inside the try return early with their own -32700 responses; everything after them that throws — session-state assignment, the onsessioninitialized await, priming-event storage — falls through to the same undiscriminated catch. There is no check on the thrown cause.\n\nImpact. A client whose handshake hits a server-internal failure receives "Parse error," implying its request was malformed — misleading for debugging and for any client-side retry/reformat logic. And now the wrong code is pinned by a test and rationalized by a comment, raising the cost of the eventual fix.\n\nHow to fix. Preferably in a follow-up (or here, since it's small): make the outer catch discriminate — return 500 / -32603 Internal error for non-parse failures, keeping -32700 only for the explicit body/schema-parse branches (which already answer before reaching the catch). At minimum in this PR: soften the new test so it doesn't pin the exact 400/-32700 shape (e.g. assert the close chain fired and that the response is an error status, or drop the "refusal like the explicit paths" framing from the comment), so a later correction of the error classification isn't blocked by this test.\n\nWhy this is a nit, not blocking. No behavior regression is introduced by this PR — the misclassification is pre-existing and the PR's feature (the onclose chain on refused handshakes) works identically whichever status the throw path returns. The only cost of merging as-is is that the wrong error code becomes test-enshrined.
1 participant
When a consumer creates a
WebStandardStreamableHTTPServerTransportper handshake attempt — the session-map recipe: classify the request, mint a fresh transport+server pair for a legacyinitialize, register it on success — a refused handshake leaks the pair. The transport answers the 4xx (schema-invalid initialize, wrongAccept, a handshake that throws) but never runs its close chain,onclosenever fires, and the connectedServerplus anything the app paired with it (an event-bus subscription, a database handle) lives until process exit. The sessions guide and the examples that follow this wiring have nothing to hook: no session was ever minted, so no teardown path runs.This PR adds
closeOnRefusedHandshake?: boolean(defaultfalse): when enabled, a sessionful transport whose first completed request does not establish its session closes and firesonclose. The rule is enforced once, at the single public entry (handleRequest'stry/finally), so every refusal path — including thrown handshakes and paths added in the future — is covered by construction rather than by per-branch instrumentation.Why an option and not the default: long-lived sessionful transports must survive pre-session refusals — the SDK's own client version negotiation sends a pre-session probe, expects the 400, then initializes on the same endpoint (pinned by
test/integration'sversionNegotiationsuite, which runs green with the default). Only the consumer knows whether a transport serves one handshake or an endpoint; the option is that declaration. Stateless transports never close regardless (covered by a test with the option enabled).Concurrency: the close is scheduled only when the settling request leaves the transport quiescent (an active-request counter, re-checked with the session state inside the scheduled microtask), so a refusal settling beside an in-flight
initializedefers to it — if that initialize establishes the session, no close fires. The counter's necessity is mutation-tested: removing it makes the race test fail.The second commit exports
classifyEntryRequest(andEntryClassification): hybrid deployments that route the legacyinitializehandshake to a session host and everything else to the stateless entry need the classification reason, and previously had to either settle for the booleanisLegacyRequestor hand-assembleclassifyInboundRequest's raw-field input, re-deriving header extraction and the single body read the entry already performs. The function the entry itself uses is now public, unchanged, and the legacy-clients guide's hand-wired routing section now shows it.Motivation and Context
See above: silent resource leak in the per-handshake wiring, with no SDK hook to fix it consumer-side, plus a re-derivation trap for hand-wired hybrid routing.
How Has This Been Tested?
Twelve tests in the new transport describe: with the option — refused
initialize(schema-invalid, bad Accept, batch) firesoncloseand tears down a connected server; a throwing handshake closes; pre-sessionGETrefusal closes; a pre-session request on a path no refusal branch individually handles (405 unsupported method) closes; established sessions receiving refused requests do NOT close and keep serving; stateless transports never close even with the option enabled; the refusal-beside-in-flight-initialize race defers and the handshake completes. With the default — a long-lived sessionful transport survives a pre-session refusal and then establishes normally (the version-negotiation shape). Classifier: field-parity againstclassifyInboundRequestacross request shapes, malformed-JSON byte preservation, the single-read invariant, and a live hybrid-routing round trip. Full workspace suite includingtest/integration(348) — the previously failingversionNegotiationtests pass with the default-off semantics. Docs gates (sync:snippets --check,docs:check) clean.Breaking Changes
None. The option defaults to off; all existing wirings are byte-for-byte unchanged.
Types of changes
Checklist
Additional context
The sessions guide's session-map recipe now constructs its per-handshake transport with the option and explains when to leave it off. An earlier revision of this PR applied the close unconditionally; the integration suite's version-negotiation tests caught that long-lived endpoints depend on surviving pre-session refusals, which is exactly why the behavior is a consumer declaration rather than a default.