fix(server): restore v1 transport lifecycle parity: single-use stateless transports, fail-fast on double connect#2421
Conversation
🦋 Changeset detectedLatest commit: 18747af The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 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}
…less transports, fail-fast on double connect
…dapter single-use propagation, client changeset entry
…d, new SdkErrorCode.StatelessTransportReuse) instead of raw Error
…r so the caller keeps control of the response
…session close() wipes the client's negotiated state, so the documented close-then-connect sequence silently broke session resumption: the resume branches skipped setProtocolVersion and subsequent requests went out without the MCP-Protocol-Version header. Both resume branches now fall back to the version the transport itself carries (new optional Transport.protocolVersion, which StreamableHTTPClientTransport already exposes) and re-adopt it as connection state.
… round-trip Protocol.close() only awaited transport.close(); a transport whose close() resolves without invoking onclose left the instance permanently wedged on the AlreadyConnected guard. close() now runs the teardown itself when the callback has not, and the wrapped onclose gained an identity guard so a deferred onclose from the old transport cannot tear down a replacement connection established afterwards.
…ten comments
- changeset: add @modelcontextprotocol/node — the call-site re-raise
changed its runtime behavior, and server is only a peer/dev dependency
so the adapter would not release otherwise
- migration guide: StatelessTransportReuse row in the SdkErrorCode table;
close-then-resume example carrying { sessionId, protocolVersion }
- reattach the handleRequest JSDoc that the inserted assertNotReused()
had orphaned
- trim mechanism-inventory comments down to their load-bearing lines
felixweinberger
force-pushed
the
fweinberger/stateless-reuse-guard
branch
from
9ec1b2d to
9aaea87
Compare
| async connect(transport: Transport): Promise<void> { | ||
| this.assertNotConnected(); | ||
|
|
||
| this._transport = transport; | ||
| const _onclose = this.transport?.onclose; | ||
| this._transport.onclose = () => { | ||
| try { | ||
| _onclose?.(); | ||
| } finally { | ||
| this._onclose(); | ||
| // Identity guard: close() may have already torn this | ||
| // connection down (and a replacement may be live) by the time | ||
| // a transport fires a deferred onclose. | ||
| if (this._transport === transport) { | ||
| this._onclose(); | ||
| } | ||
| } | ||
| }; | ||
|
|
There was a problem hiding this comment.
🔴 The wrapped onmessage and onerror callbacks installed by connect() lack the this._transport === transport identity guard this PR added to the wrapped onclose, and _onclose() never detaches the old transport's callbacks — so after the sequential close()-then-connect() pattern this PR blesses, a defunct transport's deferred message (e.g. a stdio child's buffered stdout arriving after the SIGKILL-fallback close() resolves) runs a handler whose response is written to the replacement transport, and a deferred error surfaces as a user-facing onerror misattributed to the live connection. Fix: apply the same identity guard to the onmessage/onerror wrappers (or detach/restore the callbacks in _onclose() as the failed-start unwind already does).
Extended reasoning...
What the bug is. Protocol.connect() installs three wrapped callbacks on the transport (protocol.ts:800-831). This PR added a transport-identity guard only to the wrapped onclose (if (this._transport === transport) { this._onclose(); }, lines 807-809). The wrapped onerror (814-817) and onmessage (820-831) dispatch into this._onerror / _onresponse / _onrequest / _onnotification unconditionally. And on the normal lifecycle path nothing ever detaches them: _onclose() (855-884) clears instance state and this._transport but leaves the old transport's callbacks pointing at this instance — the callback restore this PR added runs only in the failed-start unwind (842-844).
Why this PR makes the window part of the supported lifecycle. Protocol.close() now runs _onclose() eagerly when transport.close() resolves without firing onclose — i.e. exactly for transports that have not quiesced when close() returns. StdioClientTransport.close() (stdio.ts:219-258) is the first-party instance: on the SIGKILL-fallback path it resolves without awaiting the child's 'close' event, while the stdout 'data' listener stays attached and keeps firing this.onmessage for late buffered output. The PR then blesses and tests the sequential close() → connect(B) reuse pattern, so a live replacement transport can be bound while the dead transport still holds the instance's unguarded wrapped callbacks.
Step-by-step proof.
protocol.connect(stdioA); the child is slow or has a server→client request (sampling/elicitation) or notification buffered in its stdout.await protocol.close()resolves via the SIGKILL fallback; the PR's new eager teardown inclose()runs_onclose(), clearingthis._transport— butstdioA.onmessage/onerrorare still the instance's wrapped closures.protocol.connect(transportB)— the sequential-reuse pattern the PR's own test 'close() then connect() succeeds (sequential reuse)' pins.- stdioA's buffered stdout delivers a stale JSON-RPC request → the unguarded wrapped
onmessage→this._onrequest(message)._onrequestcapturesconst capturedTransport = this._transportlive at dispatch time (line 981; the comment even says 'to ensure responses go to the correct client') — that is now transport B — creates a fresh, never-abortedAbortController, runs the handler, and sends the response on transport B. - Similarly, a late error from stdioA (child
'error', stdin EPIPE, deferred fetch failure on an HTTP transport) fires the unguardedonerrorwrapper → user-facingonerroron the live connection; and a stalenotifications/cancelledthrough_onnotificationcan abort a handler serving the new connection.
Why the PR's other defenses don't catch it. The new abort gates on ctx.mcpReq.notify/send/elicitInput/requestSampling guard previously aborted handlers; the stale request in step 4 gets a fresh controller that was never aborted, so every gate passes. The onclose identity guard covers only the onclose channel. The failed-start unwind restores callbacks only when start() rejects — the mainline connect-then-close path never restores them. The PR's own test 'close() frees the instance even when the transport never fires onclose' pins the deferred-onclose half of this exact scenario but not the deferred-onmessage/onerror halves.
Impact and why this is worth fixing before merge. The result is a stale message from a dead connection being served on — and its response written to — the replacement connection: precisely the cross-connection contamination this PR's description says it eliminates ('a second connect() silently rebinds the transport out from under in-flight handlers'), reachable on the reuse path the PR documents and tests. It is the same failure family as the failed-start finding already addressed in this PR (comment on line 847), just on the normal lifecycle path, and the onmessage channel is the most damaging of the three. All verifiers confirmed the chain end to end; none refuted it.
How to fix. Mirror the existing guard on the two remaining wrappers — e.g. this._transport.onmessage = (message, extra) => { _onmessage?.(message, extra); if (this._transport !== transport) return; /* dispatch */ } and likewise for onerror — or detach/restore the transport's callbacks in _onclose() the way the failed-start unwind already does. Either is a few lines and closes the last unguarded channel.
| async close(): Promise<void> { | ||
| await this._transport?.close(); | ||
| const transport = this._transport; | ||
| await transport?.close(); | ||
| // A transport whose close() resolves without firing onclose would | ||
| // otherwise leave the instance wedged (still "connected" per the | ||
| // connect() guard). Run the teardown ourselves if it hasn't happened. | ||
| if (transport !== undefined && this._transport === transport) { | ||
| this._onclose(); | ||
| } | ||
| } |
There was a problem hiding this comment.
🔴 In Protocol.close(), the fallback _onclose() teardown runs only after await transport?.close() resolves — if a transport's close() rejects before firing onclose, the teardown is skipped, this._transport stays set, and the new assertNotConnected() guard added by this PR makes every subsequent connect() throw AlreadyConnected, permanently wedging the instance (pre-PR the silent rebind meant a rejecting close() never wedged anything). Wrap the await in try/finally, keeping the existing identity check inside the finally.
Extended reasoning...
What the bug is. Protocol.close() (packages/core-internal/src/shared/protocol.ts:1277-1286) now runs a fallback teardown so that a transport whose close() resolves without firing onclose doesn't leave the instance wedged — but the teardown sits after await transport?.close() with no try/finally. If the transport's close() rejects before firing onclose, the rejection propagates out of Protocol.close() and the manual teardown never runs: this._transport stays set, pending response handlers are never settled, and in-flight handler AbortControllers are never aborted.
Why this PR makes it consequential. Under the new assertNotConnected() guard (added by this same PR), a still-set _transport means every subsequent connect() throws AlreadyConnected. There is no public API to clear _transport, and retrying close() re-invokes the same failing transport.close() — so a persistently-rejecting close wedges the instance permanently. Before this PR, connect() silently rebound the transport, so a throwing close() never wedged anything; the missing try/finally only becomes a real failure mode now that the guard exists.
Step-by-step proof.
- A consumer uses a third-party transport whose
close()does async I/O (e.g. an HTTP session-terminate call) and rejects on failure without invokingonclose— theTransportinterface is public and pluggable, and nothing requiresclose()to fireonclosebefore rejecting. await protocol.close()→transport.close()rejects → theif (transport !== undefined && this._transport === transport) { this._onclose(); }block is skipped →Protocol.close()rejects withthis._transportstill set.- The consumer catches the error and tries to recover:
await protocol.connect(freshTransport)→assertNotConnected()seesthis._transporttruthy → throwsSdkError(AlreadyConnected). - The only escape is calling
close()again, which re-runs the same failingtransport.close(). If it keeps rejecting (e.g. the remote endpoint is gone), the instance is wedged with no recovery path.
Why existing code doesn't prevent it. _onclose() is the only teardown that clears this._transport on this path, and it's reachable here only via the transport's onclose callback or the post-await fallback — both of which the rejection bypasses. Client.close() (packages/client/src/client/client.ts:565-574) confirms the design intent: it wraps super.close() in try/finally with a comment explicitly anticipating a rejecting transport close — but it can only reset the client's own negotiated state, not the private Protocol._transport, leaving the client with wiped negotiated state and a wedged connect guard: the worst combination. The PR's own new test ('close() frees the instance even when the transport never fires onclose') establishes the invariant that close() must always free the instance — the rejection path violates it. Note the SDK's built-in transports are not affected (e.g. StreamableHTTPClientTransport.close() fires onclose in a finally), so the trigger requires a third-party transport — narrow, but Protocol/Transport are public surfaces, and the failure when hit is permanent.
How to fix. One-line restructure — move the fallback into a finally, keeping the existing identity check (which prevents double-teardown when onclose already fired or a replacement connection is live):
async close(): Promise<void> {
const transport = this._transport;
try {
await transport?.close();
} finally {
if (transport !== undefined && this._transport === transport) {
this._onclose();
}
}
}This preserves the rejection propagating to the caller (so failures stay observable) while guaranteeing the instance is freed for a subsequent connect() — the same shape Client.close() already uses one layer up.
| if (transport.sessionId !== undefined) { | ||
| const negotiatedProtocolVersion = this._negotiatedProtocolVersion; | ||
| if (negotiatedProtocolVersion !== undefined) { | ||
| // Resuming keeps the original negotiation: the instance still | ||
| // holds the negotiated version (and with it the wire era) — | ||
| // only the new transport needs the header pushed again. | ||
| transport.setProtocolVersion?.(negotiatedProtocolVersion); | ||
| const version = this._negotiatedProtocolVersion ?? transport.protocolVersion; | ||
| if (version !== undefined) { | ||
| this._negotiatedProtocolVersion = version; | ||
| transport.setProtocolVersion?.(version); | ||
| } | ||
| return; | ||
| } |
There was a problem hiding this comment.
🔴 Both session-resume branches (here and in _connectNegotiated at lines 1047–1055) restore only _negotiatedProtocolVersion after close() wiped the connection state — _serverCapabilities/_serverVersion/_instructions stay undefined, so a client with enforceStrictCapabilities locally rejects every capability-gated request on the migration guide's prescribed close()-then-resume sequence, and the response cache's server identity is never re-set, so the entire resumed session reads/writes the pre-connect '' sentinel partition, violating the cache's documented cross-server isolation on shared responseCacheStores. Fix by restoring the server-derived state the way _connectFromPrior (lines 1205–1213) does — at minimum call this._cache.setServerIdentity(this._deriveServerIdentity(transport)) in both resume branches (its sessionId fallback makes this well-defined) and either carry/restore capabilities or caveat the migration-guide resume bullet.
Extended reasoning...
What the bug is. Client.close() unconditionally runs _resetConnectionState() (client.ts:528–563, invoked from the finally at 565–574), which wipes _negotiatedProtocolVersion, _serverCapabilities, _serverVersion, _instructions, _discoverResult, and calls this._cache.resetForReconnect() — resetting the response cache's server identity to the pre-connect '' sentinel (responseCache.ts:575) while deliberately preserving a user-supplied store. The fix in 652694c taught both session-resume branches (legacy path here at 947–954, negotiated path at 1047–1055) to recover the protocol version from transport.protocolVersion, but nothing else is restored: the branches early-return without re-initializing, and _serverCapabilities is only ever assigned by the three fresh-handshake completions (initialize at ~1001, discover at ~1086, connect({ prior }) at ~1208), and _cache.setServerIdentity(...) is likewise called only at those three sites (1003, 1088, 1210).\n\nConsequence 1 — strict-capability clients reject everything. With enforceStrictCapabilities: true, Protocol.request() calls assertCapabilityForMethod (protocol.ts:1446–1453), and Client's implementation (client.ts:1291+) throws SdkError(CapabilityNotSupported) for tools/*, prompts/*, resources/*, logging/setLevel, and completion/complete whenever _serverCapabilities is undefined. Even without strict mode, getServerCapabilities()/getServerVersion()/getInstructions() return undefined for the whole resumed session, and listChanged handler wiring that gates on _serverCapabilities never re-arms.\n\nConsequence 2 — the response cache runs under the '' sentinel. Every cache read/write/evict derives its partition live: _partitionFor = JSON.stringify([serverIdentity, principal]) (responseCache.ts:368). Since neither resume branch calls setServerIdentity, the entire resumed session reads and writes the ['', ...] partitions. That violates the invariant the class doc states verbatim (responseCache.ts:300–307: two clients sharing one store but connected to different servers never collide; a server cannot read another server's public entries) and the setServerIdentity doc that sentinel entries are 'no longer reachable' after connect.\n\nStep-by-step proof (cache half).\n1. Two Client instances share one persistent responseCacheStore (the documented reason the option exists). Client A resumed to server A, client B resumed to server B, each via the migration guide's exact recipe: save sessionId + getNegotiatedProtocolVersion(), await client.close(), connect(new StreamableHTTPClientTransport(url, { sessionId, protocolVersion })).\n2. Both resumed sessions now operate in the SAME sentinel partitions. A 'public'-scoped tools/list or resources/read entry written on server A's session is served verbatim to server B's session by _serveFromCache (client.ts:~1688) whenever it is fresh.\n3. Independent of TTL, callTool reads _cache.toolDefinition (client.ts:1760, via _probe, which has no freshness gate) for SEP-2243 mirroring and output-schema validation — so tool calls on server B can be validated against server A's tool definitions.\n4. Milder same-server case: on any single-client close()-then-resume, all entries written pre-close under the real identity become unreachable (silent misses), and new writes pollute the sentinel partition. The other resume shape this PR adds and tests — a brand-new Client adopting the transport-carried version — runs under '' from construction for the same reason.\n\nStrict-capabilities proof. (1) Client connects with enforceStrictCapabilities: true, negotiates, gets a session. (2) It follows the migration guide bullet added by this PR: close(), then connect() with { sessionId, protocolVersion }. (3) The resume branch restores only the version and returns; _serverCapabilities is undefined. (4) client.listTools() → Protocol.request() → assertCapabilityForMethod → SdkError(CapabilityNotSupported), locally, before anything reaches the wire — for every capability-gated method, for the life of the resumed session.\n\nWhy this is PR-introduced and why the earlier fix didn't cover it. Pre-PR, same-instance resumption rode the silent rebind without close(), so both _serverCapabilities and the cache identity persisted. The new AlreadyConnected guard forecloses that path, and the error message, changeset, and migration guide all prescribe close()-then-connect. The earlier review thread on this (client.ts:932) was resolved by 652694c, which restored only the protocol-version half; the capabilities/identity halves remain, while the docs present resume as fully working ('Sequential close() then connect() keeps working') with no caveat. The intended complete shape is already in this file: _connectFromPrior (client.ts:1205–1213) restores _serverCapabilities/_serverVersion/_instructions and calls setServerIdentity alongside the version.\n\nHow to fix. In both resume branches: (a) call this._cache.setServerIdentity(this._deriveServerIdentity(transport)) — _deriveServerIdentity (client.ts:1248–1251) already falls back to transport.sessionId when _serverVersion is unavailable, giving a stable per-session surrogate exactly as the responseCache doc describes; and (b) either carry/restore capabilities the same way the version is now carried (e.g. adopt a transport-carried DiscoverResult, or recommend connect({ prior }) for modern-era resumption) or add a migration-guide caveat that strict-capability enforcement and the server-derived getters do not survive close()-then-resume. The cache-identity call is the non-negotiable half: a capabilities-only or docs-only fix would leave the documented cross-server isolation invariant broken on the exact path this PR prescribes and tests.
| try { | ||
| await this._transport.start(); | ||
| } catch (error) { | ||
| // Unwind a failed start so connect() can be retried: restore the | ||
| // transport's own callbacks — a late event from the failed | ||
| // transport must not tear down a successful retry's connection. | ||
| transport.onclose = _onclose; | ||
| transport.onerror = _onerror; | ||
| transport.onmessage = _onmessage; | ||
| this._transport = undefined; | ||
| throw error; | ||
| } |
There was a problem hiding this comment.
🟡 The failed-connect unwind added in this PR wraps only await this._transport.start(), but transport.setSupportedProtocolVersions?.(...) (line 834) runs after this._transport is set and the wrapped callbacks are installed, yet outside the try — so a throw from that optional Transport-interface method skips the unwind, leaving _transport bound to a never-started transport and every direct retry connect() rejecting with AlreadyConnected. Moving that one statement inside the try (or above the callback installation) keeps the PR's own invariant — any failure inside connect() leaves the instance reconnectable with the transport's callbacks restored — true for the whole method.
Extended reasoning...
What the gap is. Protocol.connect() now has a careful failure unwind: if this._transport.start() rejects, the catch block (protocol.ts:836-847) restores the transport's original onclose/onerror/onmessage callbacks and clears this._transport, so the instance stays reconnectable and a late event from the failed transport cannot disturb a retry. But one statement executes between the callback installation (lines 798-831) and that try block: transport.setSupportedProtocolVersions?.(this._supportedProtocolVersions) at line 834. setSupportedProtocolVersions is an optional method on the public Transport interface (packages/core-internal/src/shared/transport.ts), invoked on every server connect — nothing prevents a third-party implementation from throwing (e.g. one that validates the versions list and rejects an unsupported set).\n\nStep-by-step proof.\n1. A server uses a custom Transport whose setSupportedProtocolVersions(versions) throws for an unsupported list (or simply has a bug).\n2. server.connect(t): assertNotConnected() passes, this._transport = t, the wrapped onclose/onerror/onmessage closures are installed on t.\n3. Line 834 throws → connect() rejects before entering the try at line 836. None of the unwind runs: this._transport stays set to a transport whose start() was never called, and the callback restore at lines 842-844 is unreachable for this throw.\n4. The caller retries server.connect(t2) with a fresh transport — the exact recovery pattern the unwind exists to enable (and that the new test 'a failed transport start() leaves the instance reconnectable' pins for the start() path). Under the new assertNotConnected() guard this now throws SdkError(AlreadyConnected) even though no connection was ever established. Pre-PR, the silent rebind made a throw here trivially recoverable, so this is a gap the PR itself introduces in the invariant its own unwind establishes.\n\nWhy the impact is bounded (hence nit). Both first-party implementations cannot throw — WebStandardStreamableHTTPServerTransport.setSupportedProtocolVersions is a plain field assignment (streamableHttp.ts:307-309) and the Node adapter just delegates to it — so the trigger requires a third-party transport. And the instance is not permanently wedged: the AlreadyConnected message's own remedy works, because Protocol.close() now awaits transport.close() and then runs the fallback _onclose() teardown (protocol.ts:1277-1286), clearing _transport and freeing the instance for reconnect. (The tail cases — a close() that rejects on a never-started transport, and the stale un-restored onmessage/onerror wrappers left on the failed transport — are covered by the separate close()-rejection and identity-guard findings already on this PR.) The residual cost when hit: a spurious user-facing onclose for a connection that never established, and a recovery path that contradicts the direct-retry pattern the PR's tests bless for the sibling start() failure one line below.\n\nWhy existing tests miss it. The new tests in protocolConnectGuard.test.ts only exercise a rejecting start(), where the unwind does run; no test drives a throwing setSupportedProtocolVersions.\n\nHow to fix (one line). Move transport.setSupportedProtocolVersions?.(this._supportedProtocolVersions) inside the try block so the same unwind (callback restore + this._transport = undefined) covers it — or hoist it above the callback installation, before any instance state is mutated. Either keeps the whole of connect() under the reconnectable-on-failure invariant rather than only the start() call.\n\nAll three verifiers confirmed the mechanics end to end; all three independently rated it a nit for the reasons above (third-party-only trigger, close() recovery works).
1 participant
Restores transport lifecycle invariants from v1.x that were not carried over in the v2 rewrite.
WebStandardStreamableHTTPServerTransportnow handles a single request exchange and throws on reuse, matching v1 behavior since 1.26.0. Per-request serving viacreateMcpHandleris unaffected, since it already constructs a fresh pair per exchange.NodeStreamableHTTPServerTransportinherits the behavior.Protocol.connect()andClient.connect()now throw when the instance is already connected, instead of silently rebinding the transport. Sequentialclose()thenconnect()keeps working.close()is made consistent with the above: an aborted handler'sctx.mcpReq.notify()resolves as a no-op andctx.mcpReq.send()rejects withConnectionClosed, so it cannot write to a transport connected later.Motivation and Context
Reusing one stateless transport across requests mixes up the request-id to stream routing between exchanges, and a second
connect()silently rebinds the transport out from under in-flight handlers. v1.x fails fast on both since 1.26.0. The v2 rewrite dropped these checks; this restores them with the same semantics and error messages.How Has This Been Tested?
Regression tests ported from the v1 suites for both invariants, plus new tests covering the POST and GET/SSE exchange shapes and post-close handler teardown. Existing tests and examples that reused a stateless transport were moved to per-request pairs, mirroring the v1 change's own test adaptation. Full workspace gates are green (unit, integration, e2e, conformance), and the reuse throw was also exercised against the CJS build output.
Breaking Changes
Code that reused one stateless transport across requests now throws; use a fresh transport per exchange, or
createMcpHandler. This restores documented v1 semantics. Docs, middleware READMEs, and examples that showed a shared stateless transport now show the per-request pattern, and the migration guide has a new entry.Types of changes
Checklist
Additional context
Single commit; changeset included (
@modelcontextprotocol/serverand@modelcontextprotocol/core-internal, patch). The error messages are kept byte-identical to v1.x so existing handling of either message keeps working.