address review: shared connect guard, unwind callback restore, Node adapter single-use propagation, client changeset entry

Author: felixweinberger

.changeset/stateless-transport-reuse-guard.md

@@ -1,6 +1,7 @@
 ---
 '@modelcontextprotocol/server': patch
 '@modelcontextprotocol/core-internal': patch
+'@modelcontextprotocol/client': patch
 ---
 
 Restores two v1 transport lifecycle invariants dropped in the v2 rewrite:

docs/migration/upgrade-to-v2.md

@@ -1578,7 +1578,7 @@ rewrite required unless noted.
 
 #### Transport / connection lifecycle (v1 parity)
 
-Two v1 transport lifecycle invariants (in effect since sdk@1.26.0) apply in v2:
+The following v1 transport lifecycle invariants (in effect since sdk@1.26.0) apply in v2:
 
 - **A stateless `WebStandardStreamableHTTPServerTransport` serves exactly one request.**
   With `sessionIdGenerator: undefined`, the second `handleRequest()` call throws

packages/client/src/client/client.ts

@@ -929,15 +929,10 @@ export class Client extends Protocol<ClientContext> {
     override async connect(transport: Transport, options?: ConnectOptions): Promise<void> {
         // Fail fast while still connected: the negotiated paths below reset
         // per-connection state and run the discover probe BEFORE the base
-        // Protocol.connect() reuse guard would fire — without this early
-        // check, a connect() on a connected instance would corrupt the live
-        // connection's state before throwing. Same guard + message as
-        // Protocol.connect().
-        if (this.transport) {
-            throw new Error(
-                'Already connected to a transport. Call close() before connecting to a new transport, or use a separate Protocol instance per connection.'
-            );
-        }
+        // Protocol.connect() guard would fire — without this early check, a
+        // connect() on a connected instance would corrupt the live
+        // connection's state before throwing.
+        this.assertNotConnected();
         if (options?.prior !== undefined) {
             // Zero-round-trip reconnect from a previously-obtained
             // DiscoverResult: bypasses versionNegotiation resolution entirely.

packages/core-internal/src/shared/protocol.ts

@@ -775,16 +775,26 @@ export abstract class Protocol<ContextT extends BaseContext> {
     }
 
     /**
-     * Attaches to the given transport, starts it, and starts listening for messages.
-     *
-     * The caller assumes ownership of the {@linkcode Transport}, replacing any callbacks that have already been set, and expects that it is the only user of the {@linkcode Transport} instance going forward.
+     * Throws if this instance is already bound to a transport. Called by
+     * {@linkcode Protocol.connect} and, earlier, by subclass connect
+     * overrides whose pre-connect work mutates per-connection state (see
+     * `Client.connect()`): the check must run before any of that work.
      */
-    async connect(transport: Transport): Promise<void> {
+    protected assertNotConnected(): void {
         if (this._transport) {
             throw new Error(
                 'Already connected to a transport. Call close() before connecting to a new transport, or use a separate Protocol instance per connection.'
             );
         }
+    }
+
+    /**
+     * Attaches to the given transport, starts it, and starts listening for messages.
+     *
+     * The caller assumes ownership of the {@linkcode Transport}, replacing any callbacks that have already been set, and expects that it is the only user of the {@linkcode Transport} instance going forward.
+     */
+    async connect(transport: Transport): Promise<void> {
+        this.assertNotConnected();
 
         this._transport = transport;
         const _onclose = this.transport?.onclose;
@@ -825,7 +835,14 @@ export abstract class Protocol<ContextT extends BaseContext> {
             // A transport that failed to start was never connected: unwind so
             // the instance can retry connect() — the reuse guard above would
             // otherwise lock the instance onto a transport that never carried
-            // traffic.
+            // traffic. Restore the transport's own callbacks too: leaving the
+            // wrapped closures installed would let a late event from the
+            // failed transport (e.g. a child process 'close' after a failed
+            // spawn) tear down whatever connection a successful retry
+            // established.
+            transport.onclose = _onclose;
+            transport.onerror = _onerror;
+            transport.onmessage = _onmessage;
             this._transport = undefined;
             throw error;
         }

packages/core-internal/test/shared/protocolConnectGuard.test.ts

@@ -57,6 +57,36 @@ function createProtocol(): Protocol<BaseContext> {
 }
 
 describe('Protocol.connect() reuse guard', () => {
+    test('a transport that failed to start cannot disturb a later connection', async () => {
+        const protocol = createProtocol();
+        const failing = new MockTransport('failing');
+        const userOnClose = (): void => {};
+        failing.onclose = userOnClose;
+        failing.start = async (): Promise<void> => {
+            throw new Error('boot failure');
+        };
+
+        await expect(protocol.connect(failing)).rejects.toThrow('boot failure');
+        expect(protocol.transport).toBeUndefined();
+        // The unwind restored the transport's own callbacks.
+        expect(failing.onclose).toBe(userOnClose);
+
+        // A later connection is undisturbed by events from the failed
+        // transport (e.g. a child process 'close' arriving after a failed
+        // spawn already rejected start()).
+        const transportB = new MockTransport('B');
+        await protocol.connect(transportB);
+        let sawClose = false;
+        protocol.onclose = () => {
+            sawClose = true;
+        };
+
+        failing.onclose?.();
+
+        expect(protocol.transport).toBe(transportB);
+        expect(sawClose).toBe(false);
+    });
+
     test('connect() while already connected throws', async () => {
         const protocol = createProtocol();
         const transportA = new MockTransport('A');

packages/middleware/node/src/streamableHttp.ts

@@ -203,19 +203,35 @@ export class NodeStreamableHTTPServerTransport implements Transport {
         // Create a custom handler that includes our context
         // overrideGlobalObjects: false prevents Hono from overwriting global Response, which would
         // break frameworks like Next.js whose response classes extend the native Response
+        let dispatchError: unknown;
         const handler = getRequestListener(
             async (webRequest: Request) => {
-                return this._webStandardTransport.handleRequest(webRequest, {
-                    authInfo,
-                    parsedBody
-                });
+                try {
+                    return await this._webStandardTransport.handleRequest(webRequest, {
+                        authInfo,
+                        parsedBody
+                    });
+                } catch (error) {
+                    dispatchError = error;
+                    throw error;
+                }
             },
             { overrideGlobalObjects: false }
         );
 
         // Delegate to the request listener which handles all the Node.js <-> Web Standard conversion
         // including proper SSE streaming support
         await handler(req, res);
+
+        // getRequestListener absorbs a rejected dispatch into a generic 500
+        // response, which would swallow the wrapped transport's single-use
+        // throw. Re-raise it so the documented behavior (reusing a stateless
+        // transport across requests throws at the call site) holds for the
+        // Node adapter too. Other dispatch errors keep the existing
+        // 500-response behavior.
+        if (dispatchError instanceof Error && dispatchError.message.includes('Stateless transport cannot be reused across requests')) {
+            throw dispatchError;
+        }
     }
 
     /**

packages/middleware/node/test/streamableHttp.test.ts

@@ -1657,6 +1657,39 @@ describe('Zod v4', () => {
             return { transport, mcpServer };
         }
 
+        it('reusing one stateless transport rejects at the handleRequest call site (single-use invariant)', async () => {
+            // Deliberately share ONE stateless transport across two requests:
+            // the second handleRequest call must reject with the single-use
+            // message at the Node call site (not be absorbed into a bare 500).
+            const mcpServer = new McpServer({ name: 'single-use-test', version: '1.0.0' }, { capabilities: { logging: {} } });
+            const transport = new NodeStreamableHTTPServerTransport({ sessionIdGenerator: undefined });
+            await mcpServer.connect(transport);
+            perRequestServers.push(mcpServer);
+
+            const callSiteErrors: Error[] = [];
+            const sharedServer = createServer(async (req, res) => {
+                try {
+                    await transport.handleRequest(req, res);
+                } catch (error) {
+                    callSiteErrors.push(error as Error);
+                    if (!res.headersSent) res.writeHead(500);
+                    res.end();
+                }
+            });
+            const sharedUrl = await listenOnRandomPort(sharedServer);
+            try {
+                const first = await sendPostRequest(sharedUrl, TEST_MESSAGES.initialize);
+                expect(first.status).toBe(200);
+                expect(callSiteErrors).toHaveLength(0);
+
+                await sendPostRequest(sharedUrl, TEST_MESSAGES.toolsList);
+                expect(callSiteErrors).toHaveLength(1);
+                expect(callSiteErrors[0]!.message).toContain('Stateless transport cannot be reused across requests');
+            } finally {
+                sharedServer.close();
+            }
+        });
+
         it('should operate without session ID validation', async () => {
             // Initialize the server first
             const initResponse = await sendPostRequest(baseUrl, TEST_MESSAGES.initialize);