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: f7f843f 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/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.
| // 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.
…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
a634cc8 to
f7f843f
Compare
| /** | ||
| * Close the transport when its first completed request fails to establish | ||
| * a session. | ||
| * | ||
| * Enable this on transports created per handshake attempt — the | ||
| * session-map recipe, where a fresh transport and connected server pair | ||
| * serves one expected `initialize`. When the pair's first completed | ||
| * request does not establish the session (a refused or throwing | ||
| * handshake, a pre-session `GET`/`DELETE`, an unsupported method), the | ||
| * transport closes and fires {@linkcode WebStandardStreamableHTTPServerTransport.onclose | onclose}, | ||
| * so the paired server tears down instead of leaking. A request still in | ||
| * flight defers the close, and an established session is never affected. | ||
| * | ||
| * Leave it off (the default) for a transport that serves an endpoint | ||
| * long-term: pre-session refusals there — for example the SDK client's | ||
| * version-negotiation probe, answered `400` before `initialize` — must | ||
| * not end the transport. Only meaningful with `sessionIdGenerator` set; | ||
| * stateless transports never close on refusals. | ||
| * | ||
| * @default false | ||
| */ | ||
| closeOnRefusedHandshake?: boolean; |
There was a problem hiding this comment.
🔴 The new closeOnRefusedHandshake option (plus the _activeRequests counter and the try/finally + queueMicrotask deferral machinery in handleRequest) solves a problem that the only wiring it's documented for — the per-handshake session-map recipe, where the consumer owns the single handleRequest callsite — can already solve with a two-line userland check: await transport.handleRequest(req, res, req.body); if (!transport.sessionId) await transport.close(); (in a finally to also cover throws). That check keys on the same observable the SDK predicate uses and covers every refusal path the option covers, so the PR's justification that consumers have "nothing to hook" doesn't hold; the lower-cost alternative is adding the two-line teardown to the sessions guide recipe and the five examples this PR already edits, rather than a new public transport option plus a concurrency-deferral protocol that only defends a combination the option's own JSDoc says not to use.
Extended reasoning...
The design question. closeOnRefusedHandshake is documented (option JSDoc, changeset, sessions guide) exclusively for transports created per handshake attempt — the session-map recipe where the consumer builds a fresh transport + server pair, connects it, and makes exactly one transport.handleRequest(req, res, req.body) call. In that shape the consumer owns the callsite, and the transport already exposes everything needed to detect a refused handshake at that callsite: sessionId is a public property (proxied by NodeStreamableHTTPServerTransport at packages/middleware/node/src/streamableHttp.ts), it is assigned synchronously inside handlePostRequest before the initialize's response is produced, and close() is public and idempotent, firing the same onclose the examples already key their session-map cleanup on.
The userland equivalent. The whole feature reduces to:
try {
await transport.handleRequest(req, res, req.body);
} finally {
if (!transport.sessionId) await transport.close(); // refused handshake: tear the pair down
}This keys on exactly the observable the SDK's finally predicate keys on (sessionId === undefined after the settling request), so it covers every refusal path the option covers by construction: schema-invalid initialize, wrong Accept/Content-Type, batch initialize, pre-session GET/DELETE, 405 unsupported method, and the throwing-sessionIdGenerator case (the outer catch answers 400 with sessionId still undefined, so the check still closes). It even shares the same residual gap as the shipped mechanism for throws after state assignment (previous review comment #3), so it is not weaker for the recipe the option targets. The PR description's claim that consumers have "nothing to hook: no session was ever minted, so no teardown path runs" is therefore not accurate — the consumer's own handleRequest call settling with sessionId still undefined is the hook, available today with no SDK change.
Step-by-step (the guide's own recipe, no option). (1) A client POSTs an initialize with Accept: application/json only. (2) The route sees no session id, isInitializeRequest(req.body) is true, so it constructs the transport, connects a fresh server, and calls await transport.handleRequest(req, res, req.body). (3) The transport refuses with 406; handlePostRequest never reaches the state-assignment lines, so transport.sessionId is still undefined when the call settles. (4) The finally check fires await transport.close(); close() runs onclose, which is the Protocol teardown — the connected server detaches and the (never-created) map entry needs no cleanup. Same outcome as the option, in two lines the recipe already has room for.
What the SDK takes on in exchange. A new public option on WebStandardStreamableHTTPServerTransportOptions (lines 109–130), a private _activeRequests counter, a try/finally + queueMicrotask deferral protocol in handleRequest with its own ~30-line JSDoc contract, and ~260 lines of transport tests. Notably, the concurrency machinery exists only because the close was moved into the transport: in the documented per-handshake recipe each transport serves exactly one request (it only becomes reachable via the sessions map after onsessioninitialized), and for long-lived multi-request transports the option's own JSDoc says to leave it off — so the active-request guard defends a combination the feature is documented not to be used in. In the callsite version the race cannot arise at all, because the check runs after the one and only request on that transport.
Why this matters before merge. Under this repo's stated review bar (minimalism, burden of proof on the addition, helpers users can write themselves belong in the cookbook, and removing something from the public API later is far harder than not adding it), the addition needs a scenario where the callsite check is insufficient — and the PR doesn't show one. The lower-cost alternative is exactly the set of files this PR already touches: put the two-line teardown in the docs/serving/sessions-state-scaling.md recipe, its .examples.ts companion, and the five runnable examples (legacy-routing, repl, sse-polling, standalone-get, elicitation), and drop the transport option, the counter, and the deferral protocol.
Mitigations / what would change the call. The option is opt-in, default-off, well tested, and the version-negotiation default-off semantics are pinned by integration tests — nothing breaks by merging it. If there is a consumer shape where the callsite is not owned by the same code that constructs the pair (e.g. the transport is handed to a framework adapter that owns handleRequest), that would be the justification the PR currently lacks; stating it in the description (and ideally in the option JSDoc) would resolve this. Absent that, the cookbook-level fix achieves the same leak elimination with zero new public surface.
| * Where {@linkcode isLegacyRequest} collapses the decision to a boolean, this | ||
| * returns the full routing outcome — for hybrid deployments that need the | ||
| * routing reason. The canonical pattern routes the legacy `initialize` | ||
| * handshake to a sessionful host and everything else (modern traffic, other | ||
| * legacy traffic, rejections) to a stateless or strict handler: | ||
| * | ||
| * ```ts | ||
| * import { classifyEntryRequest, createMcpHandler } from '@modelcontextprotocol/server'; | ||
| * | ||
| * const stateless = createMcpHandler(factory); | ||
| * | ||
| * async function serve(request: Request): Promise<Response> { | ||
| * const classified = await classifyEntryRequest(request); | ||
| * if (classified.step === 'unreadable-body') { | ||
| * return new Response(null, { status: 400 }); | ||
| * } | ||
| * if (classified.step === 'classified' && classified.outcome.kind === 'legacy' && classified.outcome.reason === 'initialize') { | ||
| * // The 2025 handshake: open a session on the sessionful legacy host. | ||
| * return sessionHost(classified.forwardRequest); | ||
| * } | ||
| * // Hand the already-parsed body along so the entry classifies from the | ||
| * // value instead of reading the forwarded clone a second time. | ||
| * return stateless.fetch(classified.forwardRequest, classified.step === 'classified' ? { parsedBody: classified.parsedBody } : undefined); | ||
| * } | ||
| * ``` |
There was a problem hiding this comment.
🔴 The 'canonical pattern' documented for the new classifyEntryRequest export (this JSDoc @example, the changeset, the new sentence in docs/serving/legacy-clients.md, and the new hybrid-routing test) routes only outcome.reason === 'initialize' to the sessionful host — but the classifier never inspects Mcp-Session-Id, so every follow-up from the session that host just opened (claim-less tools/call POSTs, notifications/initialized, the standalone GET SSE stream, DELETE termination, posted responses to server-initiated requests) classifies legacy with a different reason and lands on the stateless/strict leg: session traffic is served statelessly, GET/DELETE get 405, in-flight elicitInput hangs, and the session host's transport/server pair leaks. Route all legacy/session-bound traffic to the session host in the example, changeset, docs sentence, and test (as the elicitation example already does with kind === 'legacy'), or reword the pattern to a scenario where initialize-only routing is sound.
Extended reasoning...
The bug. The JSDoc @example on the newly exported classifyEntryRequest (createMcpHandler.ts:417-441) presents a 'canonical pattern': route classified.outcome.kind === 'legacy' && classified.outcome.reason === 'initialize' to a sessionful legacy host, and 'everything else (modern traffic, other legacy traffic, rejections)' to a stateless or strict createMcpHandler. The same pattern is repeated in .changeset/export-classify-entry-request.md, in the sentence added to docs/serving/legacy-clients.md ("sending only the legacy initialize handshake to a session host and everything else to the stateless handler"), and pinned by the new test "routes on the outcome reason — legacy initialize to a session host, everything else to the entry (the hybrid pattern)". As documented, the pattern strands the very session it opens.
Why. classifyInboundRequest (packages/core-internal/src/shared/inboundClassification.ts) never inspects the Mcp-Session-Id header. Its legacy reasons are 'batch' | 'initialize' | 'no-claim' | 'notification' | 'http-method' | 'response'. Every request a legacy client sends after the handshake — the requests the session exists for — classifies legacy with a reason other than 'initialize': session-bound tools/call POSTs are 'no-claim', notifications/initialized is 'notification', the standalone GET SSE stream and DELETE session termination are 'http-method', and posted responses to server→client requests (elicitation/sampling answers) are 'response'. Under the documented split all of them go to stateless.fetch / the strict handler.
What happens on the stateless leg. legacyStatelessFallback serves each POST from a fresh instance that knows nothing about the session, drops notifications with a 202, and answers GET/DELETE with 405 ("Method not allowed." — behavior pinned by the existing 'answers GET and DELETE like the canonical stateless example (405)' test). So the standalone GET stream — the entire reason to run a sessionful legacy arm (server-initiated elicitation, sampling, notifications) — is refused; DELETE is refused, so the session at the host can never be terminated and its transport/server pair leaks (ironically the exact leak class the first commit's closeOnRefusedHandshake targets, and that option cannot help here because the session was established); and responses the client posts to server-initiated requests never reach the session host, so any in-flight ctx.mcpReq.elicitInput(...) hangs forever.
Step-by-step proof. (1) A 2025-era client POSTs initialize with no _meta envelope → reason 'initialize' → routed to sessionHost, which mints an Mcp-Session-Id and connects a fresh Server. (2) The client sends notifications/initialized with that header → reason 'notification' (the classifier ignores the header) → stateless.fetch → 202, dropped; the session host's Server never learns initialization completed. (3) The client opens the standalone GET SSE stream with the header → 'http-method' → stateless leg → 405; no server→client channel exists. (4) The client calls a tool whose handler awaits elicitInput → the POST is 'no-claim' → served statelessly by an instance that has no session and no client capabilities; even if a call did reach the host, the client's posted elicitation response is 'response' → stateless leg → never delivered → the handler hangs. (5) DELETE → 'http-method' → 405; the host's transport/server pair lives until process exit.
Why nothing in the PR catches it. The new hybrid-routing test only sends a legacy initialize and modern traffic — it never sends a follow-up session-bound request, so the hole is invisible. And the one real in-repo consumer this PR adds (examples/elicitation/server.ts) does not use the reason-based split: it routes classified.step === 'no-json-body' || classified.outcome.kind === 'legacy' — i.e. all legacy traffic — to the sessionful arm and never reads outcome.reason, which is exactly the boolean isLegacyRequest already provides. So the reason-based routing that is the export's stated justification (changeset, docs sentence, this example) has no sound consumer in the tree, while anyone who copies the 'canonical pattern' ships silently broken legacy sessions plus a per-handshake resource leak.
How to fix. Correct the @example, the changeset text, the legacy-clients.md sentence, and the pinning test so all session-bound legacy traffic reaches the session host — e.g. reason === 'initialize' || request.headers.has('mcp-session-id') || the GET/DELETE/notification/response reasons, which collapses to kind === 'legacy' (what the elicitation example and the isLegacyRequest recipe in the same docs paragraph already do). If that collapse weakens the case for exporting the reason, restate the export's justification (e.g. access to parsedBody/forwardRequest from the single body read, which is what the elicitation example actually uses it for), or reword the pattern to a scenario where initialize-only routing is genuinely sound (e.g. session-affinity infrastructure owning all follow-ups). The exported function itself is fine; it's the advertised wiring around it that misleads.
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. Five in-repo examples using the per-handshake session-map wiring (legacy-routing, sse-polling, repl, standalone-get, elicitation) now construct with the option, and the elicitation example's hybrid entry adoptsclassifyEntryRequest— replacing anisLegacyRequest+ second body-read composition whose clone-ordering pitfall its own comment warned about; the full examples runner passes 73/73 legs. 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.