From 9c3560525098271f43abda745ffea88a877b5d3f Mon Sep 17 00:00:00 2001 From: Berkant Acun Date: Sat, 12 Sep 2026 01:38:16 +0300 Subject: [PATCH] fix(core): carry the transport's last error into the Connection closed rejection When a transport reports why it is closing and then closes, pending requests were rejected with a bare `Connection closed` and the reason went only to `onerror`. Protocol now keeps the most recent transport error since the last delivered message and settles pending requests with `Connection closed: ` and that error as `cause`. Plain `Connection closed` stays for a close with no reported reason, a close the caller asked for, and an error the transport recovered from. First point of #2775. --- .changeset/connection-closed-carries-cause.md | 9 +++ packages/client/src/client/client.ts | 2 +- packages/client/test/client/stdio.test.ts | 39 +++++++++++ packages/core-internal/src/shared/protocol.ts | 36 +++++++++- .../test/shared/protocol.test.ts | 69 +++++++++++++++++++ 5 files changed, 153 insertions(+), 2 deletions(-) create mode 100644 .changeset/connection-closed-carries-cause.md diff --git a/.changeset/connection-closed-carries-cause.md b/.changeset/connection-closed-carries-cause.md new file mode 100644 index 0000000000..4eb92bcd41 --- /dev/null +++ b/.changeset/connection-closed-carries-cause.md @@ -0,0 +1,9 @@ +--- +'@modelcontextprotocol/core-internal': patch +'@modelcontextprotocol/client': patch +'@modelcontextprotocol/server': patch +--- + +Carry the transport's last reported error into the `Connection closed` rejection. When a transport reports why it is closing — a `ReadBuffer` overflow, a stream error, a dropped socket — and then closes, every pending request was rejected with a bare `SdkError('Connection closed')` and the reason went only to `onerror`. Code doing the ordinary thing (`await client.listTools()`) was told the connection dropped and nothing else. + +The rejection now reads `Connection closed: ` with the transport's error as `cause`. Plain `Connection closed` remains the message when the transport reported nothing, when the caller closed the connection itself, and when a message was delivered after the error (the transport recovered, so the error is not why it closed). `SdkErrorCode.ConnectionClosed` is unchanged. diff --git a/packages/client/src/client/client.ts b/packages/client/src/client/client.ts index 0b386a63e8..69bbd750b8 100644 --- a/packages/client/src/client/client.ts +++ b/packages/client/src/client/client.ts @@ -2264,7 +2264,7 @@ export class Client extends Protocol { */ protected override _onclose(): void { if (this._listenState.size > 0) { - const reason = new SdkError(SdkErrorCode.ConnectionClosed, 'Connection closed'); + const reason = this._connectionClosedError(); for (const entry of this._listenState.values()) { entry.settle({ cause: 'remote', error: reason }); } diff --git a/packages/client/test/client/stdio.test.ts b/packages/client/test/client/stdio.test.ts index 315b8a2595..f06eb4aa40 100644 --- a/packages/client/test/client/stdio.test.ts +++ b/packages/client/test/client/stdio.test.ts @@ -3,6 +3,9 @@ import { tmpdir } from 'node:os'; import type { JSONRPCMessage } from '@modelcontextprotocol/core-internal'; +import { SdkError, SdkErrorCode } from '@modelcontextprotocol/core-internal'; + +import { Client } from '../../src/client/client'; import type { StdioServerParameters } from '../../src/client/stdio'; import { StdioClientTransport } from '../../src/client/stdio'; @@ -122,6 +125,42 @@ test('should fire onerror and close when ReadBuffer overflows', async () => { await closed; }); +test('an awaiting caller learns why the connection closed when the read buffer overflows', async () => { + // The shape from #2775: the transport reports the cause on onerror and + // closes; without a reason on the close, `await client.listTools()` was + // rejected with a bare `Connection closed` and the diagnosis was only + // visible to code that had wired up onerror in advance. + const server = String.raw` + const { createInterface } = require('readline'); + const send = (o) => process.stdout.write(JSON.stringify(o) + '\n'); + createInterface({ input: process.stdin }).on('line', (line) => { + const m = JSON.parse(line); + if (m.method === 'initialize') { + return send({ jsonrpc: '2.0', id: m.id, result: { + protocolVersion: '2025-06-18', capabilities: { tools: {} }, + serverInfo: { name: 'big', version: '1.0.0' } } }); + } + if (m.method === 'notifications/initialized') return; + send({ jsonrpc: '2.0', id: m.id, result: { + tools: [{ name: 'big', description: 'A'.repeat(4096), inputSchema: { type: 'object' } }] } }); + }); + `; + const transport = new StdioClientTransport({ + command: process.execPath, + args: ['-e', server], + maxBufferSize: 1024 + }); + const client = new Client({ name: 'demo', version: '1.0.0' }); + await client.connect(transport); + + const error = await client.listTools().catch(e => e); + expect(error).toBeInstanceOf(SdkError); + expect(error.code).toBe(SdkErrorCode.ConnectionClosed); + expect(error.message).toMatch(/^Connection closed: ReadBuffer exceeded maximum size of 1024 bytes/); + expect(error.cause).toBeInstanceOf(Error); + expect((error.cause as Error).message).toMatch(/ReadBuffer exceeded maximum size/); +}, 10_000); + test('_dispose releases the parent-side pipe handles even when a helper process holds the child stdio', async () => { // The rmcp-holding anatomy: the child exits, but a helper it spawned with // stdio: 'inherit' keeps the pipe write ends open. Awaiting 'exit' settles diff --git a/packages/core-internal/src/shared/protocol.ts b/packages/core-internal/src/shared/protocol.ts index 637be389aa..f28ad46fd6 100644 --- a/packages/core-internal/src/shared/protocol.ts +++ b/packages/core-internal/src/shared/protocol.ts @@ -566,6 +566,15 @@ export abstract class Protocol { private _timeoutInfo: Map = new Map(); private _pendingDebouncedNotifications = new Set(); + /** + * The most recent error the transport reported since the last message it + * delivered. Read once, in `_onclose`, so the `Connection closed` rejection + * can name the cause instead of a placeholder. A message arriving after an + * error means the transport recovered from it, so it is not why the + * connection closed and is forgotten. + */ + private _lastTransportError?: Error; + /** * The protocol version negotiated for the current connection (`undefined` * before negotiation completes), which determines the wire era this @@ -784,6 +793,7 @@ export abstract class Protocol { */ async connect(transport: Transport): Promise { this._transport = transport; + this._lastTransportError = undefined; const _onclose = this.transport?.onclose; this._transport.onclose = () => { try { @@ -795,12 +805,14 @@ export abstract class Protocol { const _onerror = this.transport?.onerror; this._transport.onerror = (error: Error) => { + this._lastTransportError = error; _onerror?.(error); this._onerror(error); }; const _onmessage = this._transport?.onmessage; this._transport.onmessage = (message, extra) => { + this._lastTransportError = undefined; _onmessage?.(message, extra); if (isJSONRPCResultResponse(message) || isJSONRPCErrorResponse(message)) { this._onresponse(message); @@ -838,9 +850,10 @@ export abstract class Protocol { const requestHandlerAbortControllers = this._requestHandlerAbortControllers; this._requestHandlerAbortControllers = new Map(); - const error = new SdkError(SdkErrorCode.ConnectionClosed, 'Connection closed'); + const error = this._connectionClosedError(); this._transport = undefined; + this._lastTransportError = undefined; try { this.onclose?.(); @@ -859,6 +872,24 @@ export abstract class Protocol { this.onerror?.(error); } + /** + * The error every pending request is settled with when the connection + * closes. When the transport reported why — a read buffer overflow, a + * stream error, a dropped socket — that error is the `cause` and its + * message is appended, so an awaiting caller learns what happened without + * having wired up `onerror` in advance. Plain `Connection closed` is the + * fallback for a close with no reported reason. Subclasses that settle + * their own pending state on close should use this rather than construct + * a second, reason-less error. + */ + protected _connectionClosedError(): SdkError { + const cause = this._lastTransportError; + if (!cause) { + return new SdkError(SdkErrorCode.ConnectionClosed, 'Connection closed'); + } + return new SdkError(SdkErrorCode.ConnectionClosed, `Connection closed: ${cause.message}`, undefined, { cause }); + } + /** * Inbound-notification dispatch. Subclass overrides MUST delegate * unmatched traffic to `super._onnotification(rawNotification, extra)` — @@ -1225,6 +1256,9 @@ export abstract class Protocol { * Closes the connection. */ async close(): Promise { + // A close the caller asked for has no transport-reported reason, even + // if the transport complained about something earlier. + this._lastTransportError = undefined; await this._transport?.close(); } diff --git a/packages/core-internal/test/shared/protocol.test.ts b/packages/core-internal/test/shared/protocol.test.ts index 2fb0f64813..9cc4851964 100644 --- a/packages/core-internal/test/shared/protocol.test.ts +++ b/packages/core-internal/test/shared/protocol.test.ts @@ -135,6 +135,75 @@ describe('protocol tests', () => { expect((abortReason as SdkError).code).toBe(SdkErrorCode.ConnectionClosed); }); + describe('close reason', () => { + const resultSchema = z.object({}); + + test('rejects pending requests with the last transport error as the cause', async () => { + await protocol.connect(transport); + const pending = testRequest(protocol, { method: 'example', params: {} }, resultSchema); + + const reported = new Error('ReadBuffer exceeded maximum size of 100 bytes'); + transport.onerror?.(reported); + await transport.close(); + + const error = await pending.catch(e => e); + expect(error).toBeInstanceOf(SdkError); + expect((error as SdkError).code).toBe(SdkErrorCode.ConnectionClosed); + expect((error as SdkError).message).toBe('Connection closed: ReadBuffer exceeded maximum size of 100 bytes'); + expect((error as SdkError).cause).toBe(reported); + }); + + test('falls back to a plain Connection closed when the transport reported nothing', async () => { + await protocol.connect(transport); + const pending = testRequest(protocol, { method: 'example', params: {} }, resultSchema); + + await transport.close(); + + const error = await pending.catch(e => e); + expect((error as SdkError).message).toBe('Connection closed'); + expect((error as SdkError).cause).toBeUndefined(); + }); + + test('forgets a transport error once a later message shows the transport recovered', async () => { + await protocol.connect(transport); + const pending = testRequest(protocol, { method: 'example', params: {} }, resultSchema); + + transport.onerror?.(new Error('a parse error the transport skipped past')); + transport.onmessage?.({ jsonrpc: '2.0', method: 'notifications/progress', params: { progressToken: 'x', progress: 1 } }); + await transport.close(); + + const error = await pending.catch(e => e); + expect((error as SdkError).message).toBe('Connection closed'); + expect((error as SdkError).cause).toBeUndefined(); + }); + + test('does not blame an earlier transport error for a close the caller asked for', async () => { + await protocol.connect(transport); + const pending = testRequest(protocol, { method: 'example', params: {} }, resultSchema); + + transport.onerror?.(new Error('an earlier complaint')); + await protocol.close(); + + const error = await pending.catch(e => e); + expect((error as SdkError).message).toBe('Connection closed'); + expect((error as SdkError).cause).toBeUndefined(); + }); + + test('does not carry a reason across reconnects', async () => { + await protocol.connect(transport); + transport.onerror?.(new Error('from the first connection')); + await transport.close(); + + const second = new MockTransport(); + await protocol.connect(second); + const pending = testRequest(protocol, { method: 'example', params: {} }, resultSchema); + await second.close(); + + const error = await pending.catch(e => e); + expect((error as SdkError).message).toBe('Connection closed'); + }); + }); + test('should remove abort listener from caller signal when request settles', async () => { await protocol.connect(transport);