From d71da7da4ff45a3ca7d4e5bfa43d0d8a7d3430aa Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 28 Sep 2026 14:58:31 +0000 Subject: [PATCH 1/2] fix(core): await the notification send so a failed send is never briefly unhandled Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_012VRbFCp41otcScXE1YY3es --- .changeset/await-notification-send.md | 6 ++ ...egacyHandshakeCloseAfterInitialize.test.ts | 82 +++++++++++++++++++ packages/core-internal/src/shared/protocol.ts | 2 +- .../shared/notificationSendRejection.test.ts | 80 ++++++++++++++++++ .../server/notificationSendRejection.test.ts | 52 ++++++++++++ 5 files changed, 221 insertions(+), 1 deletion(-) create mode 100644 .changeset/await-notification-send.md create mode 100644 packages/client/test/client/legacyHandshakeCloseAfterInitialize.test.ts create mode 100644 packages/core-internal/test/shared/notificationSendRejection.test.ts create mode 100644 packages/server/test/server/notificationSendRejection.test.ts diff --git a/.changeset/await-notification-send.md b/.changeset/await-notification-send.md new file mode 100644 index 0000000000..2df131c33c --- /dev/null +++ b/.changeset/await-notification-send.md @@ -0,0 +1,6 @@ +--- +'@modelcontextprotocol/client': patch +'@modelcontextprotocol/server': patch +--- + +Sending a notification on a closed connection no longer produces a briefly unhandled promise rejection (seen as `unhandledrejection` on Cloudflare Workers) in addition to the returned rejection. diff --git a/packages/client/test/client/legacyHandshakeCloseAfterInitialize.test.ts b/packages/client/test/client/legacyHandshakeCloseAfterInitialize.test.ts new file mode 100644 index 0000000000..5288d0b8d9 --- /dev/null +++ b/packages/client/test/client/legacyHandshakeCloseAfterInitialize.test.ts @@ -0,0 +1,82 @@ +/** + * The trigger reported in #2864: the transport closes during the legacy + * `initialize` handshake, right after the server's result is delivered, so the + * client's `notifications/initialized` is sent on a connection that is already + * gone. `connect()` must reject through its returned promise with the + * not-connected send failure, and nothing else may escape. + * + * Node's rejection tracker does not report the one-microtask handler gap that + * workerd does (see core-internal's notificationSendRejection test for the + * timing observation itself); this end-to-end test pins the trigger path and + * the error `connect()` rejects with. + */ +import type { JSONRPCMessage, Transport } from '@modelcontextprotocol/core-internal'; +import { isJSONRPCRequest, SdkError, SdkErrorCode } from '@modelcontextprotocol/core-internal'; +import { afterEach, beforeEach, describe, expect, test } from 'vitest'; + +import { Client } from '../../src/client/client'; + +/** Answers `initialize`, then closes in the same tick — before the client can send `notifications/initialized`. */ +class ReplyThenCloseTransport implements Transport { + onclose?: () => void; + onerror?: (error: Error) => void; + onmessage?: (message: JSONRPCMessage) => void; + + sent: JSONRPCMessage[] = []; + closeCalls = 0; + + async start(): Promise {} + + async send(message: JSONRPCMessage): Promise { + this.sent.push(message); + if (!isJSONRPCRequest(message) || message.method !== 'initialize') return; + queueMicrotask(() => { + this.onmessage?.({ + jsonrpc: '2.0', + id: message.id, + result: { protocolVersion: '2025-03-26', capabilities: {}, serverInfo: { name: 'flaky', version: '0' } } + }); + this.onclose?.(); + }); + } + + async close(): Promise { + this.closeCalls++; + } +} + +describe('legacy handshake: transport closes between the initialize result and notifications/initialized', () => { + const unhandled: unknown[] = []; + const onUnhandled = (reason: unknown): void => { + unhandled.push(reason); + }; + + beforeEach(() => { + unhandled.length = 0; + process.on('unhandledRejection', onUnhandled); + }); + + afterEach(() => { + process.off('unhandledRejection', onUnhandled); + }); + + test('connect() rejects with SdkError NotConnected from the initialized-notification send and no rejection escapes', async () => { + const transport = new ReplyThenCloseTransport(); + const client = new Client({ name: 'c', version: '0' }, { versionNegotiation: { mode: 'legacy' } }); + + const rejection = await client.connect(transport).then( + () => undefined, + (error: unknown) => error + ); + + expect(rejection).toBeInstanceOf(SdkError); + expect((rejection as SdkError).code).toBe(SdkErrorCode.NotConnected); + // The handshake got as far as the send that failed: initialize went out, + // the initialized notification never did. + expect(transport.sent.map(m => ('method' in m ? m.method : 'response'))).toEqual(['initialize']); + + // Let any stray rejection surface before asserting none did. + await new Promise(resolve => setTimeout(resolve, 0)); + expect(unhandled).toEqual([]); + }); +}); diff --git a/packages/core-internal/src/shared/protocol.ts b/packages/core-internal/src/shared/protocol.ts index 637be389aa..8e0bc04034 100644 --- a/packages/core-internal/src/shared/protocol.ts +++ b/packages/core-internal/src/shared/protocol.ts @@ -1593,7 +1593,7 @@ export abstract class Protocol { * Emits a notification, which is a one-way message that does not expect a response. */ async notification(notification: Notification, options?: NotificationOptions): Promise { - return this._notificationViaCodec(this._resolveOutboundCodec(notification.method), notification, options); + return await this._notificationViaCodec(this._resolveOutboundCodec(notification.method), notification, options); } /** diff --git a/packages/core-internal/test/shared/notificationSendRejection.test.ts b/packages/core-internal/test/shared/notificationSendRejection.test.ts new file mode 100644 index 0000000000..69ec61cc0a --- /dev/null +++ b/packages/core-internal/test/shared/notificationSendRejection.test.ts @@ -0,0 +1,80 @@ +/** + * `Protocol.notification()` must hand its caller the ONLY rejection a failed + * send produces. The send funnel (`_notificationViaCodec`) is an async method + * that throws synchronously when there is no transport, so the promise it + * returns is already rejected. If `notification()` returns that promise + * instead of awaiting it, the async-function resolution goes through the + * thenable job: the inner rejection sits with no handler for one microtask + * before the job reads and calls its `then`. Node's tracker forgives that; + * workerd (Cloudflare Workers) reports it as `unhandledrejection` followed by + * `rejectionhandled`, which surfaces as noise in Vitest runs on that platform + * (#2864). + * + * The observation below is the handler-attachment timing itself: with + * `return await`, `await` attaches its reaction through the internal + * promise-then path and the inner promise's own `then` property is never + * read; with a bare `return`, the thenable job reads it one microtask later. + */ +import { describe, expect, test } from 'vitest'; + +import { SdkError, SdkErrorCode } from '../../src/errors/sdkErrors'; +import type { BaseContext } from '../../src/shared/protocol'; +import { Protocol } from '../../src/shared/protocol'; + +class TestProtocolImpl extends Protocol { + protected assertCapabilityForMethod(): void {} + protected assertNotificationCapability(): void {} + protected assertRequestHandlerCapability(): void {} + protected buildContext(ctx: BaseContext): BaseContext { + return ctx; + } +} + +/** + * A natively rejected promise whose `then` property records every read. + * The thenable-resolution job reaches `then` via property lookup; `await` + * on a native promise does not. + */ +function instrumentedRejection(error: Error): { promise: Promise; thenReads: () => number } { + const promise = Promise.reject(error); + const originalThen = promise.then.bind(promise); + let reads = 0; + Object.defineProperty(promise, 'then', { + get() { + reads++; + return originalThen; + } + }); + return { promise, thenReads: () => reads }; +} + +describe('Protocol.notification(): a failed send rejects only through the returned promise', () => { + test('when not connected, the inner send rejection is handled in the same microtask (no thenable-job hop)', async () => { + const protocol = new TestProtocolImpl(); + const notConnected = new SdkError(SdkErrorCode.NotConnected, 'Not connected'); + const inner = instrumentedRejection(notConnected); + + // Stand in for the real funnel with a rejection we can observe. The real + // one throws synchronously on `!this._transport`, i.e. it also returns an + // already-rejected promise — the shape that matters here. + // Plain instance override rather than `vi.spyOn`: the spy wrapper itself + // reads `then` on any promise a spied call returns, which would mask the + // observation below. + (protocol as unknown as { _notificationViaCodec: () => Promise })._notificationViaCodec = () => inner.promise; + + await expect(protocol.notification({ method: 'notifications/initialized' })).rejects.toBe(notConnected); + + // A bare `return innerPromise` from the async method resolves via the + // thenable job, which reads `then` one microtask after the inner promise + // rejected — the window in which workerd reports it unhandled. + expect(inner.thenReads()).toBe(0); + }); + + test('when not connected, the returned promise still rejects with SdkError NotConnected (unstubbed path)', async () => { + const protocol = new TestProtocolImpl(); + + await expect(protocol.notification({ method: 'notifications/initialized' })).rejects.toSatisfy( + (error: unknown) => error instanceof SdkError && error.code === SdkErrorCode.NotConnected + ); + }); +}); diff --git a/packages/server/test/server/notificationSendRejection.test.ts b/packages/server/test/server/notificationSendRejection.test.ts new file mode 100644 index 0000000000..fbf0560d14 --- /dev/null +++ b/packages/server/test/server/notificationSendRejection.test.ts @@ -0,0 +1,52 @@ +/** + * Server-side twin of core-internal's notificationSendRejection test: a + * `sendLoggingMessage()` on a server with no transport must reject ONLY + * through the returned promise. The send funnel returns an already-rejected + * promise; if `Protocol.notification()` returned it instead of awaiting it, + * the inner rejection would sit unhandled for one microtask (reported by + * workerd as `unhandledrejection` + `rejectionhandled`, #2864). + */ +import { SdkError, SdkErrorCode } from '@modelcontextprotocol/core-internal'; +import { describe, expect, test } from 'vitest'; + +import { Server } from '../../src/server/server'; + +function instrumentedRejection(error: Error): { promise: Promise; thenReads: () => number } { + const promise = Promise.reject(error); + const originalThen = promise.then.bind(promise); + let reads = 0; + Object.defineProperty(promise, 'then', { + get() { + reads++; + return originalThen; + } + }); + return { promise, thenReads: () => reads }; +} + +describe('Server notification sends on a closed connection', () => { + test('sendLoggingMessage() when not connected: the inner send rejection is handled in the same microtask', async () => { + const server = new Server({ name: 'test', version: '1.0.0' }, { capabilities: { logging: {} } }); + const notConnected = new SdkError(SdkErrorCode.NotConnected, 'Not connected'); + const inner = instrumentedRejection(notConnected); + + // Plain instance override rather than `vi.spyOn`: the spy wrapper itself + // reads `then` on any promise a spied call returns, which would mask the + // observation below. + (server as unknown as { _notificationViaCodec: () => Promise })._notificationViaCodec = () => inner.promise; + + await expect(server.sendLoggingMessage({ level: 'info', data: 'hello' })).rejects.toBe(notConnected); + + // Read by the thenable job only when `notification()` returns the inner + // promise without awaiting it. + expect(inner.thenReads()).toBe(0); + }); + + test('sendLoggingMessage() when not connected still rejects with SdkError NotConnected (unstubbed path)', async () => { + const server = new Server({ name: 'test', version: '1.0.0' }, { capabilities: { logging: {} } }); + + await expect(server.sendLoggingMessage({ level: 'info', data: 'hello' })).rejects.toSatisfy( + (error: unknown) => error instanceof SdkError && error.code === SdkErrorCode.NotConnected + ); + }); +}); From 72f07e026fc11c842756d8177907a348e8508e3a Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 28 Sep 2026 15:34:01 +0000 Subject: [PATCH 2/2] test: correct the comments on when the inner promise's then is read Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_012VRbFCp41otcScXE1YY3es --- .../shared/notificationSendRejection.test.ts | 24 ++++++++++--------- .../server/notificationSendRejection.test.ts | 4 ++-- 2 files changed, 15 insertions(+), 13 deletions(-) diff --git a/packages/core-internal/test/shared/notificationSendRejection.test.ts b/packages/core-internal/test/shared/notificationSendRejection.test.ts index 69ec61cc0a..88129278bb 100644 --- a/packages/core-internal/test/shared/notificationSendRejection.test.ts +++ b/packages/core-internal/test/shared/notificationSendRejection.test.ts @@ -3,17 +3,19 @@ * send produces. The send funnel (`_notificationViaCodec`) is an async method * that throws synchronously when there is no transport, so the promise it * returns is already rejected. If `notification()` returns that promise - * instead of awaiting it, the async-function resolution goes through the - * thenable job: the inner rejection sits with no handler for one microtask - * before the job reads and calls its `then`. Node's tracker forgives that; + * instead of awaiting it, the async function resolves with a thenable: its + * `then` is read synchronously at return, but the call is deferred to the + * thenable job, so the inner rejection sits with no handler for one + * microtask. Node's tracker forgives that; * workerd (Cloudflare Workers) reports it as `unhandledrejection` followed by * `rejectionhandled`, which surfaces as noise in Vitest runs on that platform * (#2864). * * The observation below is the handler-attachment timing itself: with - * `return await`, `await` attaches its reaction through the internal - * promise-then path and the inner promise's own `then` property is never - * read; with a bare `return`, the thenable job reads it one microtask later. + * `return await`, `await` attaches its reaction synchronously through the + * internal promise path and the inner promise's own `then` property is never + * read; with a bare `return`, `then` is read at return and called one + * microtask later. */ import { describe, expect, test } from 'vitest'; @@ -32,8 +34,8 @@ class TestProtocolImpl extends Protocol { /** * A natively rejected promise whose `then` property records every read. - * The thenable-resolution job reaches `then` via property lookup; `await` - * on a native promise does not. + * Resolving an async function with a thenable reaches `then` via property + * lookup; `await` on a native promise does not. */ function instrumentedRejection(error: Error): { promise: Promise; thenReads: () => number } { const promise = Promise.reject(error); @@ -64,9 +66,9 @@ describe('Protocol.notification(): a failed send rejects only through the return await expect(protocol.notification({ method: 'notifications/initialized' })).rejects.toBe(notConnected); - // A bare `return innerPromise` from the async method resolves via the - // thenable job, which reads `then` one microtask after the inner promise - // rejected — the window in which workerd reports it unhandled. + // A bare `return innerPromise` from the async method reads `then` + // synchronously at return and calls it one microtask later — the window + // in which workerd reports the inner rejection unhandled. expect(inner.thenReads()).toBe(0); }); diff --git a/packages/server/test/server/notificationSendRejection.test.ts b/packages/server/test/server/notificationSendRejection.test.ts index fbf0560d14..3ff02f0909 100644 --- a/packages/server/test/server/notificationSendRejection.test.ts +++ b/packages/server/test/server/notificationSendRejection.test.ts @@ -37,8 +37,8 @@ describe('Server notification sends on a closed connection', () => { await expect(server.sendLoggingMessage({ level: 'info', data: 'hello' })).rejects.toBe(notConnected); - // Read by the thenable job only when `notification()` returns the inner - // promise without awaiting it. + // `then` is read (synchronously at return, called one microtask later) + // only when `notification()` returns the inner promise without awaiting it. expect(inner.thenReads()).toBe(0); });