From 1938c020bb62d93cb5354f3a0d33b78c5b31d45f Mon Sep 17 00:00:00 2001 From: Travis Bonnet Date: Thu, 19 Mar 2026 13:44:15 -0500 Subject: [PATCH 1/5] feat(core): add opt-in periodic ping for connection health monitoring Implements the missing periodic ping functionality specified in issue #1000. Per the MCP specification, implementations SHOULD periodically issue pings to detect connection health, with configurable frequency. Changes: - Add `pingIntervalMs` option to `ProtocolOptions` (disabled by default) - Implement `startPeriodicPing()` and `stopPeriodicPing()` in Protocol - Client starts periodic ping after successful initialization - Server starts periodic ping after receiving initialized notification - Timer uses `unref()` so it does not prevent clean process exit - Ping failures are reported via `onerror` without stopping the timer - Timer is automatically cleaned up on close or unexpected disconnect Fixes #1000 Co-authored-by: Br1an67 <932039080@qq.com> --- .changeset/periodic-ping.md | 16 ++ packages/client/src/client/client.ts | 3 + packages/core/src/shared/protocol.ts | 68 +++++ .../core/test/shared/periodicPing.test.ts | 260 ++++++++++++++++++ packages/server/src/server/server.ts | 5 +- 5 files changed, 351 insertions(+), 1 deletion(-) create mode 100644 .changeset/periodic-ping.md create mode 100644 packages/core/test/shared/periodicPing.test.ts diff --git a/.changeset/periodic-ping.md b/.changeset/periodic-ping.md new file mode 100644 index 000000000..b7c9e3ac7 --- /dev/null +++ b/.changeset/periodic-ping.md @@ -0,0 +1,16 @@ +--- +"@modelcontextprotocol/core": minor +"@modelcontextprotocol/client": minor +"@modelcontextprotocol/server": minor +--- + +feat: add opt-in periodic ping for connection health monitoring + +Adds a `pingIntervalMs` option to `ProtocolOptions` that enables automatic +periodic pings to verify the remote side is still responsive. Per the MCP +specification, implementations SHOULD periodically issue pings to detect +connection health, with configurable frequency. + +The feature is disabled by default. When enabled, pings begin after +initialization completes and stop automatically when the connection closes. +Failures are reported via the `onerror` callback without stopping the timer. diff --git a/packages/client/src/client/client.ts b/packages/client/src/client/client.ts index 21a43bd15..be004b9f0 100644 --- a/packages/client/src/client/client.ts +++ b/packages/client/src/client/client.ts @@ -541,6 +541,9 @@ export class Client extends Protocol { this._setupListChangedHandlers(this._pendingListChangedConfig); this._pendingListChangedConfig = undefined; } + + // Start periodic ping after successful initialization + this.startPeriodicPing(); } catch (error) { // Disconnect if initialization fails. void this.close(); diff --git a/packages/core/src/shared/protocol.ts b/packages/core/src/shared/protocol.ts index 57eab6932..149c08ead 100644 --- a/packages/core/src/shared/protocol.ts +++ b/packages/core/src/shared/protocol.ts @@ -33,6 +33,7 @@ import type { TaskCreationParams } from '../types/index.js'; import { + EmptyResultSchema, getNotificationSchema, getRequestSchema, getResultSchema, @@ -92,6 +93,21 @@ export type ProtocolOptions = { * so they should NOT be included here. */ tasks?: TaskManagerOptions; + + /** + * Interval (in milliseconds) between periodic ping requests sent to the remote side + * to verify connection health. If set, pings will begin after {@linkcode Protocol.connect | connect()} + * completes and stop automatically when the connection closes. + * + * Per the MCP specification, implementations SHOULD periodically issue pings to + * detect connection health, with configurable frequency. + * + * Disabled by default (no periodic pings). Typical values: 15000-60000 (15s-60s). + * + * Ping failures are reported via the {@linkcode Protocol.onerror | onerror} callback + * and do not stop the periodic timer. + */ + pingIntervalMs?: number; }; /** @@ -308,6 +324,9 @@ export abstract class Protocol { private _taskManager: TaskManager; + private _pingTimer?: ReturnType; + private _pingIntervalMs?: number; + protected _supportedProtocolVersions: string[]; /** @@ -336,6 +355,7 @@ export abstract class Protocol { constructor(private _options?: ProtocolOptions) { this._supportedProtocolVersions = _options?.supportedProtocolVersions ?? SUPPORTED_PROTOCOL_VERSIONS; + this._pingIntervalMs = _options?.pingIntervalMs; // Create TaskManager from protocol options this._taskManager = _options?.tasks ? new TaskManager(_options.tasks) : new NullTaskManager(); @@ -490,6 +510,8 @@ export abstract class Protocol { } private _onclose(): void { + this.stopPeriodicPing(); + const responseHandlers = this._responseHandlers; this._responseHandlers = new Map(); this._progressHandlers.clear(); @@ -728,10 +750,56 @@ export abstract class Protocol { return this._transport; } + /** + * Starts sending periodic ping requests at the configured interval. + * Pings are used to verify that the remote side is still responsive. + * Failures are reported via the {@linkcode onerror} callback but do not + * stop the timer; pings continue until the connection is closed. + * + * This is called automatically at the end of {@linkcode connect} when + * `pingIntervalMs` is set. Subclasses that override `connect()` and + * perform additional initialization (e.g., the MCP handshake) may call + * this method after their initialization is complete instead. + * + * Has no effect if periodic ping is already running or if no interval + * is configured. + */ + protected startPeriodicPing(): void { + if (this._pingTimer || !this._pingIntervalMs) { + return; + } + + this._pingTimer = setInterval(async () => { + try { + await this._requestWithSchema({ method: 'ping' }, EmptyResultSchema, { + timeout: this._pingIntervalMs + }); + } catch (error) { + this._onerror(error instanceof Error ? error : new Error(`Periodic ping failed: ${String(error)}`)); + } + }, this._pingIntervalMs); + + // Allow the process to exit even if the timer is still running + if (typeof this._pingTimer === 'object' && 'unref' in this._pingTimer) { + this._pingTimer.unref(); + } + } + + /** + * Stops periodic ping requests. Called automatically when the connection closes. + */ + protected stopPeriodicPing(): void { + if (this._pingTimer) { + clearInterval(this._pingTimer); + this._pingTimer = undefined; + } + } + /** * Closes the connection. */ async close(): Promise { + this.stopPeriodicPing(); await this._transport?.close(); } diff --git a/packages/core/test/shared/periodicPing.test.ts b/packages/core/test/shared/periodicPing.test.ts new file mode 100644 index 000000000..eeabce3a7 --- /dev/null +++ b/packages/core/test/shared/periodicPing.test.ts @@ -0,0 +1,260 @@ +import { vi, beforeEach, afterEach, describe, test, expect } from 'vitest'; + +import type { BaseContext } from '../../src/shared/protocol.js'; +import { Protocol } from '../../src/shared/protocol.js'; +import type { Transport, TransportSendOptions } from '../../src/shared/transport.js'; +import type { JSONRPCMessage } from '../../src/types/types.js'; + +class MockTransport implements Transport { + onclose?: () => void; + onerror?: (error: Error) => void; + onmessage?: (message: unknown) => void; + + async start(): Promise {} + async close(): Promise { + this.onclose?.(); + } + async send(_message: JSONRPCMessage, _options?: TransportSendOptions): Promise {} +} + +function createProtocol(options?: { pingIntervalMs?: number }) { + return new (class extends Protocol { + protected assertCapabilityForMethod(): void {} + protected assertNotificationCapability(): void {} + protected assertRequestHandlerCapability(): void {} + protected assertTaskCapability(): void {} + protected buildContext(ctx: BaseContext): BaseContext { + return ctx; + } + protected assertTaskHandlerCapability(): void {} + // Expose protected methods for testing + public testStartPeriodicPing(): void { + this.startPeriodicPing(); + } + public testStopPeriodicPing(): void { + this.stopPeriodicPing(); + } + })(options); +} + +describe('Periodic Ping', () => { + let transport: MockTransport; + + beforeEach(() => { + vi.useFakeTimers(); + transport = new MockTransport(); + }); + + afterEach(() => { + vi.useRealTimers(); + }); + + test('should not send periodic pings when pingIntervalMs is not set', async () => { + const protocol = createProtocol(); + const sendSpy = vi.spyOn(transport, 'send'); + + await protocol.connect(transport); + + // Advance time well past any reasonable interval + await vi.advanceTimersByTimeAsync(120_000); + + // No ping requests should have been sent (only no messages at all) + const pingMessages = sendSpy.mock.calls.filter(call => { + const msg = call[0] as { method?: string }; + return msg.method === 'ping'; + }); + expect(pingMessages).toHaveLength(0); + }); + + test('should send periodic pings when pingIntervalMs is set and startPeriodicPing is called', async () => { + const protocol = createProtocol({ pingIntervalMs: 10_000 }); + const sendSpy = vi.spyOn(transport, 'send'); + + await protocol.connect(transport); + + // Respond to each ping with a success result + sendSpy.mockImplementation(async (message: JSONRPCMessage) => { + const msg = message as { id?: number; method?: string }; + if (msg.method === 'ping' && msg.id !== undefined) { + // Simulate the server responding with a pong + transport.onmessage?.({ + jsonrpc: '2.0', + id: msg.id, + result: {} + }); + } + }); + + // Start periodic ping (in real usage, Client.connect() calls this after init) + protocol.testStartPeriodicPing(); + + // No ping yet (first fires after one interval) + const pingsBeforeAdvance = sendSpy.mock.calls.filter(call => { + const msg = call[0] as { method?: string }; + return msg.method === 'ping'; + }); + expect(pingsBeforeAdvance).toHaveLength(0); + + // Advance past one interval + await vi.advanceTimersByTimeAsync(10_000); + + const pingsAfterOne = sendSpy.mock.calls.filter(call => { + const msg = call[0] as { method?: string }; + return msg.method === 'ping'; + }); + expect(pingsAfterOne).toHaveLength(1); + + // Advance past another interval + await vi.advanceTimersByTimeAsync(10_000); + + const pingsAfterTwo = sendSpy.mock.calls.filter(call => { + const msg = call[0] as { method?: string }; + return msg.method === 'ping'; + }); + expect(pingsAfterTwo).toHaveLength(2); + }); + + test('should stop periodic pings on close', async () => { + const protocol = createProtocol({ pingIntervalMs: 5_000 }); + const sendSpy = vi.spyOn(transport, 'send'); + + await protocol.connect(transport); + + sendSpy.mockImplementation(async (message: JSONRPCMessage) => { + const msg = message as { id?: number; method?: string }; + if (msg.method === 'ping' && msg.id !== undefined) { + transport.onmessage?.({ + jsonrpc: '2.0', + id: msg.id, + result: {} + }); + } + }); + + protocol.testStartPeriodicPing(); + + // One ping fires + await vi.advanceTimersByTimeAsync(5_000); + const pingsBeforeClose = sendSpy.mock.calls.filter(call => { + const msg = call[0] as { method?: string }; + return msg.method === 'ping'; + }); + expect(pingsBeforeClose).toHaveLength(1); + + // Close the connection + await protocol.close(); + + // Advance more time; no new pings should be sent + sendSpy.mockClear(); + await vi.advanceTimersByTimeAsync(20_000); + + const pingsAfterClose = sendSpy.mock.calls.filter(call => { + const msg = call[0] as { method?: string }; + return msg.method === 'ping'; + }); + expect(pingsAfterClose).toHaveLength(0); + }); + + test('should report ping errors via onerror without stopping the timer', async () => { + const protocol = createProtocol({ pingIntervalMs: 5_000 }); + const errors: Error[] = []; + protocol.onerror = error => { + errors.push(error); + }; + + await protocol.connect(transport); + + // Make send reject to simulate a failed ping + const sendSpy = vi.spyOn(transport, 'send'); + sendSpy.mockImplementation(async (message: JSONRPCMessage) => { + const msg = message as { id?: number; method?: string }; + if (msg.method === 'ping' && msg.id !== undefined) { + transport.onmessage?.({ + jsonrpc: '2.0', + id: msg.id, + error: { + code: -32000, + message: 'Server error' + } + }); + } + }); + + protocol.testStartPeriodicPing(); + + // First ping fails + await vi.advanceTimersByTimeAsync(5_000); + expect(errors).toHaveLength(1); + + // Second ping also fails, proving the timer was not stopped + await vi.advanceTimersByTimeAsync(5_000); + expect(errors).toHaveLength(2); + }); + + test('should not start duplicate timers if startPeriodicPing is called multiple times', async () => { + const protocol = createProtocol({ pingIntervalMs: 5_000 }); + const sendSpy = vi.spyOn(transport, 'send'); + + await protocol.connect(transport); + + sendSpy.mockImplementation(async (message: JSONRPCMessage) => { + const msg = message as { id?: number; method?: string }; + if (msg.method === 'ping' && msg.id !== undefined) { + transport.onmessage?.({ + jsonrpc: '2.0', + id: msg.id, + result: {} + }); + } + }); + + // Call startPeriodicPing multiple times + protocol.testStartPeriodicPing(); + protocol.testStartPeriodicPing(); + protocol.testStartPeriodicPing(); + + await vi.advanceTimersByTimeAsync(5_000); + + // Should only have one ping, not three + const pings = sendSpy.mock.calls.filter(call => { + const msg = call[0] as { method?: string }; + return msg.method === 'ping'; + }); + expect(pings).toHaveLength(1); + }); + + test('should stop periodic pings when transport closes unexpectedly', async () => { + const protocol = createProtocol({ pingIntervalMs: 5_000 }); + const sendSpy = vi.spyOn(transport, 'send'); + + await protocol.connect(transport); + + sendSpy.mockImplementation(async (message: JSONRPCMessage) => { + const msg = message as { id?: number; method?: string }; + if (msg.method === 'ping' && msg.id !== undefined) { + transport.onmessage?.({ + jsonrpc: '2.0', + id: msg.id, + result: {} + }); + } + }); + + protocol.testStartPeriodicPing(); + + // One ping fires + await vi.advanceTimersByTimeAsync(5_000); + + // Simulate transport closing unexpectedly + transport.onclose?.(); + + sendSpy.mockClear(); + await vi.advanceTimersByTimeAsync(20_000); + + const pingsAfterTransportClose = sendSpy.mock.calls.filter(call => { + const msg = call[0] as { method?: string }; + return msg.method === 'ping'; + }); + expect(pingsAfterTransportClose).toHaveLength(0); + }); +}); diff --git a/packages/server/src/server/server.ts b/packages/server/src/server/server.ts index 4361f3e1e..3bc448145 100644 --- a/packages/server/src/server/server.ts +++ b/packages/server/src/server/server.ts @@ -133,7 +133,10 @@ export class Server extends Protocol { } this.setRequestHandler('initialize', request => this._oninitialize(request)); - this.setNotificationHandler('notifications/initialized', () => this.oninitialized?.()); + this.setNotificationHandler('notifications/initialized', () => { + this.startPeriodicPing(); + this.oninitialized?.(); + }); if (this._capabilities.logging) { this._registerLoggingHandler(); From 41a36b3289887d887a796e7af02f3fa6b0017e41 Mon Sep 17 00:00:00 2001 From: Felix Weinberger Date: Thu, 26 Mar 2026 15:35:19 +0000 Subject: [PATCH 2/5] refactor: extract _handleInitialized method for clearer lifecycle hook Mirrors the Go SDK pattern where the initialized notification handler is a named method rather than an inline closure. --- packages/server/src/server/server.ts | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/packages/server/src/server/server.ts b/packages/server/src/server/server.ts index 3bc448145..8c8c844b6 100644 --- a/packages/server/src/server/server.ts +++ b/packages/server/src/server/server.ts @@ -133,16 +133,18 @@ export class Server extends Protocol { } this.setRequestHandler('initialize', request => this._oninitialize(request)); - this.setNotificationHandler('notifications/initialized', () => { - this.startPeriodicPing(); - this.oninitialized?.(); - }); + this.setNotificationHandler('notifications/initialized', () => this._handleInitialized()); if (this._capabilities.logging) { this._registerLoggingHandler(); } } + private _handleInitialized(): void { + this.startPeriodicPing(); + this.oninitialized?.(); + } + private _registerLoggingHandler(): void { this.setRequestHandler('logging/setLevel', async (request, ctx) => { const transportSessionId: string | undefined = From d9d31e1df65d0154e0ebfb85ac9eb7bf7ddc1ccb Mon Sep 17 00:00:00 2001 From: Travis Bonnet Date: Thu, 2 Apr 2026 00:32:51 -0500 Subject: [PATCH 3/5] fix: restart periodic ping after client reconnection The reconnection path (when transport.sessionId is set) returns early, skipping the startPeriodicPing() call that runs after normal initialization. Since _onclose() clears the ping timer during disconnection, connection health monitoring silently stops working after any reconnect. Adding startPeriodicPing() before the early return ensures pings resume on reconnection. Co-Authored-By: Claude Opus 4.6 (1M context) --- packages/client/src/client/client.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/client/src/client/client.ts b/packages/client/src/client/client.ts index be004b9f0..4de82cfc5 100644 --- a/packages/client/src/client/client.ts +++ b/packages/client/src/client/client.ts @@ -498,6 +498,8 @@ export class Client extends Protocol { if (this._negotiatedProtocolVersion !== undefined && transport.setProtocolVersion) { transport.setProtocolVersion(this._negotiatedProtocolVersion); } + // Restart periodic ping since _onclose() clears the timer on disconnect + this.startPeriodicPing(); return; } try { From 2a6dc38cb611bc7fec894d6e0ea11d6bef752d9c Mon Sep 17 00:00:00 2001 From: Travis Bonnet Date: Sat, 4 Apr 2026 19:42:51 -0500 Subject: [PATCH 4/5] fix(core): address review feedback on periodic ping - Fix shutdown race condition: add _closing flag so in-flight pings do not fire spurious onerror when close() is called concurrently. The flag is set in close() before stopping the timer and closing the transport, and reset in connect() for clean reconnection. - Prevent concurrent ping overlap: replace setInterval with self-rescheduling setTimeout so the next ping is only scheduled after the current one completes (success or failure). This strictly enforces one-at-a-time behavior instead of relying on timeout alignment. - Fix inaccurate JSDoc: startPeriodicPing() is not called by the base Protocol.connect(). Updated docs to reflect that Client and Server call it after their respective initialization steps, and that custom subclasses must call it explicitly. - Add test coverage for the shutdown race condition and strict sequential ping enforcement. Co-Authored-By: Claude Opus 4.6 (1M context) --- packages/core/src/shared/protocol.ts | 61 ++++-- .../core/test/shared/periodicPing.test.ts | 193 +++++++++--------- 2 files changed, 141 insertions(+), 113 deletions(-) diff --git a/packages/core/src/shared/protocol.ts b/packages/core/src/shared/protocol.ts index 149c08ead..306a24bc1 100644 --- a/packages/core/src/shared/protocol.ts +++ b/packages/core/src/shared/protocol.ts @@ -96,8 +96,9 @@ export type ProtocolOptions = { /** * Interval (in milliseconds) between periodic ping requests sent to the remote side - * to verify connection health. If set, pings will begin after {@linkcode Protocol.connect | connect()} - * completes and stop automatically when the connection closes. + * to verify connection health. When set, pings begin after initialization completes + * ({@linkcode Client} starts them after the MCP handshake; {@linkcode Server} starts + * them on `notifications/initialized`) and stop automatically when the connection closes. * * Per the MCP specification, implementations SHOULD periodically issue pings to * detect connection health, with configurable frequency. @@ -105,7 +106,7 @@ export type ProtocolOptions = { * Disabled by default (no periodic pings). Typical values: 15000-60000 (15s-60s). * * Ping failures are reported via the {@linkcode Protocol.onerror | onerror} callback - * and do not stop the periodic timer. + * and do not stop the periodic loop. */ pingIntervalMs?: number; }; @@ -324,8 +325,9 @@ export abstract class Protocol { private _taskManager: TaskManager; - private _pingTimer?: ReturnType; + private _pingTimer?: ReturnType; private _pingIntervalMs?: number; + private _closing = false; protected _supportedProtocolVersions: string[]; @@ -474,6 +476,7 @@ export abstract class Protocol { */ async connect(transport: Transport): Promise { this._transport = transport; + this._closing = false; const _onclose = this.transport?.onclose; this._transport.onclose = () => { try { @@ -754,12 +757,13 @@ export abstract class Protocol { * Starts sending periodic ping requests at the configured interval. * Pings are used to verify that the remote side is still responsive. * Failures are reported via the {@linkcode onerror} callback but do not - * stop the timer; pings continue until the connection is closed. + * stop the loop; pings continue until the connection is closed. * - * This is called automatically at the end of {@linkcode connect} when - * `pingIntervalMs` is set. Subclasses that override `connect()` and - * perform additional initialization (e.g., the MCP handshake) may call - * this method after their initialization is complete instead. + * This is not called automatically by the base {@linkcode Protocol.connect | connect()} + * method. {@linkcode Client} calls it after the MCP initialization handshake + * (and on reconnection), and {@linkcode Server} calls it when the + * `notifications/initialized` notification is received. Custom `Protocol` + * subclasses must call this explicitly after their own initialization. * * Has no effect if periodic ping is already running or if no interval * is configured. @@ -769,20 +773,32 @@ export abstract class Protocol { return; } - this._pingTimer = setInterval(async () => { - try { - await this._requestWithSchema({ method: 'ping' }, EmptyResultSchema, { - timeout: this._pingIntervalMs - }); - } catch (error) { - this._onerror(error instanceof Error ? error : new Error(`Periodic ping failed: ${String(error)}`)); + const schedulePing = (): void => { + this._pingTimer = setTimeout(async () => { + try { + await this._requestWithSchema({ method: 'ping' }, EmptyResultSchema, { + timeout: this._pingIntervalMs + }); + } catch (error) { + // Suppress errors caused by intentional shutdown + if (!this._closing) { + this._onerror(error instanceof Error ? error : new Error(`Periodic ping failed: ${String(error)}`)); + } + } finally { + // Schedule the next ping only if we have not been stopped + if (this._pingTimer) { + schedulePing(); + } + } + }, this._pingIntervalMs); + + // Allow the process to exit even if the timer is still running + if (typeof this._pingTimer === 'object' && 'unref' in this._pingTimer) { + this._pingTimer.unref(); } - }, this._pingIntervalMs); + }; - // Allow the process to exit even if the timer is still running - if (typeof this._pingTimer === 'object' && 'unref' in this._pingTimer) { - this._pingTimer.unref(); - } + schedulePing(); } /** @@ -790,7 +806,7 @@ export abstract class Protocol { */ protected stopPeriodicPing(): void { if (this._pingTimer) { - clearInterval(this._pingTimer); + clearTimeout(this._pingTimer); this._pingTimer = undefined; } } @@ -799,6 +815,7 @@ export abstract class Protocol { * Closes the connection. */ async close(): Promise { + this._closing = true; this.stopPeriodicPing(); await this._transport?.close(); } diff --git a/packages/core/test/shared/periodicPing.test.ts b/packages/core/test/shared/periodicPing.test.ts index eeabce3a7..2cf836881 100644 --- a/packages/core/test/shared/periodicPing.test.ts +++ b/packages/core/test/shared/periodicPing.test.ts @@ -37,6 +37,28 @@ function createProtocol(options?: { pingIntervalMs?: number }) { })(options); } +/** Configure the spy to auto-respond to pings with a success result. */ +function autoRespondPings(transport: MockTransport, sendSpy: ReturnType): void { + sendSpy.mockImplementation(async (message: JSONRPCMessage) => { + const msg = message as { id?: number; method?: string }; + if (msg.method === 'ping' && msg.id !== undefined) { + transport.onmessage?.({ + jsonrpc: '2.0', + id: msg.id, + result: {} + }); + } + }); +} + +/** Count ping messages in spy call history. */ +function countPings(sendSpy: ReturnType): number { + return sendSpy.mock.calls.filter((call: unknown[]) => { + const msg = call[0] as { method?: string }; + return msg.method === 'ping'; + }).length; +} + describe('Periodic Ping', () => { let transport: MockTransport; @@ -58,12 +80,7 @@ describe('Periodic Ping', () => { // Advance time well past any reasonable interval await vi.advanceTimersByTimeAsync(120_000); - // No ping requests should have been sent (only no messages at all) - const pingMessages = sendSpy.mock.calls.filter(call => { - const msg = call[0] as { method?: string }; - return msg.method === 'ping'; - }); - expect(pingMessages).toHaveLength(0); + expect(countPings(sendSpy)).toBe(0); }); test('should send periodic pings when pingIntervalMs is set and startPeriodicPing is called', async () => { @@ -71,47 +88,21 @@ describe('Periodic Ping', () => { const sendSpy = vi.spyOn(transport, 'send'); await protocol.connect(transport); - - // Respond to each ping with a success result - sendSpy.mockImplementation(async (message: JSONRPCMessage) => { - const msg = message as { id?: number; method?: string }; - if (msg.method === 'ping' && msg.id !== undefined) { - // Simulate the server responding with a pong - transport.onmessage?.({ - jsonrpc: '2.0', - id: msg.id, - result: {} - }); - } - }); + autoRespondPings(transport, sendSpy); // Start periodic ping (in real usage, Client.connect() calls this after init) protocol.testStartPeriodicPing(); // No ping yet (first fires after one interval) - const pingsBeforeAdvance = sendSpy.mock.calls.filter(call => { - const msg = call[0] as { method?: string }; - return msg.method === 'ping'; - }); - expect(pingsBeforeAdvance).toHaveLength(0); + expect(countPings(sendSpy)).toBe(0); // Advance past one interval await vi.advanceTimersByTimeAsync(10_000); - - const pingsAfterOne = sendSpy.mock.calls.filter(call => { - const msg = call[0] as { method?: string }; - return msg.method === 'ping'; - }); - expect(pingsAfterOne).toHaveLength(1); + expect(countPings(sendSpy)).toBe(1); // Advance past another interval await vi.advanceTimersByTimeAsync(10_000); - - const pingsAfterTwo = sendSpy.mock.calls.filter(call => { - const msg = call[0] as { method?: string }; - return msg.method === 'ping'; - }); - expect(pingsAfterTwo).toHaveLength(2); + expect(countPings(sendSpy)).toBe(2); }); test('should stop periodic pings on close', async () => { @@ -119,27 +110,13 @@ describe('Periodic Ping', () => { const sendSpy = vi.spyOn(transport, 'send'); await protocol.connect(transport); - - sendSpy.mockImplementation(async (message: JSONRPCMessage) => { - const msg = message as { id?: number; method?: string }; - if (msg.method === 'ping' && msg.id !== undefined) { - transport.onmessage?.({ - jsonrpc: '2.0', - id: msg.id, - result: {} - }); - } - }); + autoRespondPings(transport, sendSpy); protocol.testStartPeriodicPing(); // One ping fires await vi.advanceTimersByTimeAsync(5_000); - const pingsBeforeClose = sendSpy.mock.calls.filter(call => { - const msg = call[0] as { method?: string }; - return msg.method === 'ping'; - }); - expect(pingsBeforeClose).toHaveLength(1); + expect(countPings(sendSpy)).toBe(1); // Close the connection await protocol.close(); @@ -148,14 +125,10 @@ describe('Periodic Ping', () => { sendSpy.mockClear(); await vi.advanceTimersByTimeAsync(20_000); - const pingsAfterClose = sendSpy.mock.calls.filter(call => { - const msg = call[0] as { method?: string }; - return msg.method === 'ping'; - }); - expect(pingsAfterClose).toHaveLength(0); + expect(countPings(sendSpy)).toBe(0); }); - test('should report ping errors via onerror without stopping the timer', async () => { + test('should report ping errors via onerror without stopping the loop', async () => { const protocol = createProtocol({ pingIntervalMs: 5_000 }); const errors: Error[] = []; protocol.onerror = error => { @@ -164,7 +137,7 @@ describe('Periodic Ping', () => { await protocol.connect(transport); - // Make send reject to simulate a failed ping + // Respond with an error to simulate a failed ping const sendSpy = vi.spyOn(transport, 'send'); sendSpy.mockImplementation(async (message: JSONRPCMessage) => { const msg = message as { id?: number; method?: string }; @@ -186,7 +159,7 @@ describe('Periodic Ping', () => { await vi.advanceTimersByTimeAsync(5_000); expect(errors).toHaveLength(1); - // Second ping also fails, proving the timer was not stopped + // Second ping also fails, proving the loop was not stopped await vi.advanceTimersByTimeAsync(5_000); expect(errors).toHaveLength(2); }); @@ -196,17 +169,7 @@ describe('Periodic Ping', () => { const sendSpy = vi.spyOn(transport, 'send'); await protocol.connect(transport); - - sendSpy.mockImplementation(async (message: JSONRPCMessage) => { - const msg = message as { id?: number; method?: string }; - if (msg.method === 'ping' && msg.id !== undefined) { - transport.onmessage?.({ - jsonrpc: '2.0', - id: msg.id, - result: {} - }); - } - }); + autoRespondPings(transport, sendSpy); // Call startPeriodicPing multiple times protocol.testStartPeriodicPing(); @@ -216,11 +179,7 @@ describe('Periodic Ping', () => { await vi.advanceTimersByTimeAsync(5_000); // Should only have one ping, not three - const pings = sendSpy.mock.calls.filter(call => { - const msg = call[0] as { method?: string }; - return msg.method === 'ping'; - }); - expect(pings).toHaveLength(1); + expect(countPings(sendSpy)).toBe(1); }); test('should stop periodic pings when transport closes unexpectedly', async () => { @@ -228,17 +187,7 @@ describe('Periodic Ping', () => { const sendSpy = vi.spyOn(transport, 'send'); await protocol.connect(transport); - - sendSpy.mockImplementation(async (message: JSONRPCMessage) => { - const msg = message as { id?: number; method?: string }; - if (msg.method === 'ping' && msg.id !== undefined) { - transport.onmessage?.({ - jsonrpc: '2.0', - id: msg.id, - result: {} - }); - } - }); + autoRespondPings(transport, sendSpy); protocol.testStartPeriodicPing(); @@ -251,10 +200,72 @@ describe('Periodic Ping', () => { sendSpy.mockClear(); await vi.advanceTimersByTimeAsync(20_000); - const pingsAfterTransportClose = sendSpy.mock.calls.filter(call => { - const msg = call[0] as { method?: string }; - return msg.method === 'ping'; + expect(countPings(sendSpy)).toBe(0); + }); + + test('should not fire onerror when close() races with an in-flight ping', async () => { + const protocol = createProtocol({ pingIntervalMs: 5_000 }); + const errors: Error[] = []; + protocol.onerror = error => { + errors.push(error); + }; + + await protocol.connect(transport); + + // Do NOT auto-respond: the ping will remain in-flight so close() races with it + protocol.testStartPeriodicPing(); + + // Advance to fire the first ping (it is now awaiting a response) + await vi.advanceTimersByTimeAsync(5_000); + + // Close while the ping is in-flight; this should NOT produce an onerror + await protocol.close(); + + // Drain any pending microtasks + await vi.advanceTimersByTimeAsync(0); + + expect(errors).toHaveLength(0); + }); + + test('pings are strictly sequential (no concurrent overlap)', async () => { + const protocol = createProtocol({ pingIntervalMs: 5_000 }); + const sendSpy = vi.spyOn(transport, 'send'); + let inFlightPings = 0; + let maxConcurrent = 0; + + await protocol.connect(transport); + + sendSpy.mockImplementation(async (message: JSONRPCMessage) => { + const msg = message as { id?: number; method?: string }; + if (msg.method === 'ping' && msg.id !== undefined) { + inFlightPings++; + if (inFlightPings > maxConcurrent) { + maxConcurrent = inFlightPings; + } + // Simulate a slow response: resolve after a short delay + setTimeout(() => { + inFlightPings--; + transport.onmessage?.({ + jsonrpc: '2.0', + id: msg.id, + result: {} + }); + }, 2_000); + } }); - expect(pingsAfterTransportClose).toHaveLength(0); + + protocol.testStartPeriodicPing(); + + // Advance enough for multiple ping cycles + for (let i = 0; i < 5; i++) { + await vi.advanceTimersByTimeAsync(5_000); + // Let the delayed response resolve + await vi.advanceTimersByTimeAsync(2_000); + } + + // With setTimeout-based scheduling, pings are strictly sequential + expect(maxConcurrent).toBe(1); + // Verify pings were actually sent + expect(countPings(sendSpy)).toBeGreaterThanOrEqual(3); }); }); From 6ec1f9cd8b1521b6c96f04adf6def677c46de026 Mon Sep 17 00:00:00 2001 From: Travis Bonnet Date: Fri, 10 Apr 2026 00:53:52 -0500 Subject: [PATCH 5/5] fix: replace cross-package {@linkcode} refs with backtick code in JSDoc TypeDoc cannot resolve {@linkcode Client} and {@linkcode Server} from the core package because those classes live in separate packages (@modelcontextprotocol/client and @modelcontextprotocol/server). Replace with backtick-quoted code spans so the docs build passes without warnings. Co-Authored-By: Claude Opus 4.6 (1M context) --- packages/core/src/shared/protocol.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/core/src/shared/protocol.ts b/packages/core/src/shared/protocol.ts index 306a24bc1..ceda59dbe 100644 --- a/packages/core/src/shared/protocol.ts +++ b/packages/core/src/shared/protocol.ts @@ -97,7 +97,7 @@ export type ProtocolOptions = { /** * Interval (in milliseconds) between periodic ping requests sent to the remote side * to verify connection health. When set, pings begin after initialization completes - * ({@linkcode Client} starts them after the MCP handshake; {@linkcode Server} starts + * (`Client` starts them after the MCP handshake; `Server` starts * them on `notifications/initialized`) and stop automatically when the connection closes. * * Per the MCP specification, implementations SHOULD periodically issue pings to @@ -760,8 +760,8 @@ export abstract class Protocol { * stop the loop; pings continue until the connection is closed. * * This is not called automatically by the base {@linkcode Protocol.connect | connect()} - * method. {@linkcode Client} calls it after the MCP initialization handshake - * (and on reconnection), and {@linkcode Server} calls it when the + * method. `Client` calls it after the MCP initialization handshake + * (and on reconnection), and `Server` calls it when the * `notifications/initialized` notification is received. Custom `Protocol` * subclasses must call this explicitly after their own initialization. *