Skip to content

feat(core): add opt-in periodic ping for connection health monitoring #1717

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
travisbreaks wants to merge 5 commits into
base: main
Choose a base branch
from
Open

feat(core): add opt-in periodic ping for connection health monitoring #1717

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
1938c02
feat(core): add opt-in periodic ping for connection health monitoring
travisbreaks Mar 19, 2026
41a36b3
refactor: extract _handleInitialized method for clearer lifecycle hook
felixweinberger Mar 26, 2026
d9d31e1
fix: restart periodic ping after client reconnection
travisbreaks Apr 2, 2026
2a6dc38
fix(core): address review feedback on periodic ping
travisbreaks Apr 5, 2026
6ec1f9c
fix: replace cross-package {@linkcode} refs with backtick code in JSDoc
travisbreaks Apr 10, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions .changeset/periodic-ping.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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.
5 changes: 5 additions & 0 deletions packages/client/src/client/client.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -498,6 +498,8 @@ export class Client extends Protocol<ClientContext> {
if (this._negotiatedProtocolVersion !== undefined && transport.setProtocolVersion) {
transport.setProtocolVersion(this._negotiatedProtocolVersion);
}
// Restart periodic ping since _onclose() clears the timer on disconnect
this.startPeriodicPing();
return;
}
try {
Expand Down Expand Up @@ -541,6 +543,9 @@ export class Client extends Protocol<ClientContext> {
this._setupListChangedHandlers(this._pendingListChangedConfig);
this._pendingListChangedConfig = undefined;
}

// Start periodic ping after successful initialization
this.startPeriodicPing();
} catch (error) {
// Disconnect if initialization fails.
void this.close();
Expand Down
85 changes: 85 additions & 0 deletions packages/core/src/shared/protocol.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ import type {
TaskCreationParams
} from '../types/index.js';
import {
EmptyResultSchema,
getNotificationSchema,
getRequestSchema,
getResultSchema,
Expand Down Expand Up @@ -92,6 +93,22 @@ 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. When set, pings begin after initialization completes
* (`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
* 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 loop.
*/
pingIntervalMs?: number;
};

/**
Expand Down Expand Up @@ -308,6 +325,10 @@ export abstract class Protocol<ContextT extends BaseContext> {

private _taskManager: TaskManager;

private _pingTimer?: ReturnType<typeof setTimeout>;
private _pingIntervalMs?: number;
private _closing = false;

protected _supportedProtocolVersions: string[];

/**
Expand Down Expand Up @@ -336,6 +357,7 @@ export abstract class Protocol<ContextT extends BaseContext> {

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();
Expand Down Expand Up @@ -454,6 +476,7 @@ export abstract class Protocol<ContextT extends BaseContext> {
*/
async connect(transport: Transport): Promise<void> {
this._transport = transport;
this._closing = false;
const _onclose = this.transport?.onclose;
this._transport.onclose = () => {
try {
Expand Down Expand Up @@ -490,6 +513,8 @@ export abstract class Protocol<ContextT extends BaseContext> {
}

private _onclose(): void {
this.stopPeriodicPing();

const responseHandlers = this._responseHandlers;
this._responseHandlers = new Map();
this._progressHandlers.clear();
Expand Down Expand Up @@ -728,10 +753,70 @@ export abstract class Protocol<ContextT extends BaseContext> {
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 loop; pings continue until the connection is closed.
*
* This is not called automatically by the base {@linkcode Protocol.connect | connect()}
* 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.
*
* Has no effect if periodic ping is already running or if no interval
* is configured.
*/
Comment on lines +756 to +770
Copy link
Copy Markdown

@claude claude bot Apr 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 The JSDoc for startPeriodicPing() claims "This is called automatically at the end of connect() when pingIntervalMs is set," but Protocol.connect() ends with await this._transport.start() and never calls startPeriodicPing(). Any custom Protocol subclass that does not override connect() will silently never start periodic pings — update the JSDoc to accurately describe that only Client and Server call this after their respective initialization steps.

Extended reasoning...

What the bug is

The JSDoc comment on startPeriodicPing() (protocol.ts lines 759-762) contains a false statement: "This is called automatically at the end of connect() when pingIntervalMs is set." This implies the base Protocol.connect() is responsible for starting the ping timer when the option is configured.

The specific code path

Looking at Protocol.connect(), the method sets up transport callbacks and ends with await this._transport.start(). There is no call to startPeriodicPing() anywhere in the base implementation. The actual callers are: (1) Client.connect() calls this.startPeriodicPing() after the full MCP initialization handshake completes (and also on the reconnection fast-path); (2) Server._handleInitialized() calls this.startPeriodicPing() when the notifications/initialized notification is received.

Why existing code does not prevent the confusion

The second sentence of the JSDoc partially acknowledges the real behavior: "Subclasses that override connect() and perform additional initialization may call this method after their initialization is complete instead." However, this reads as an opt-out from the stated automatic behavior, not a correction of it. The false first sentence still stands.

Impact

Any developer implementing a custom Protocol subclass who passes pingIntervalMs and does not override connect() will get no periodic pings, despite the documentation promising automatic invocation. The ping timer simply never starts. This is a silent failure — no error is thrown, and _pingIntervalMs is set, so there is no obvious indication that something is wrong.

Step-by-step proof

  1. Create a class extending Protocol without overriding connect()
  2. Instantiate with new MyProtocol({ pingIntervalMs: 30_000 })
  3. Call await myProtocol.connect(transport) — this runs Protocol.connect(), which calls transport.start() and returns
  4. startPeriodicPing() is never invoked; _pingTimer remains undefined
  5. No pings are ever sent, despite _pingIntervalMs being set to 30,000

Fix

Update the JSDoc to accurately state that Client and Server are the actual callers, and that custom subclasses must call this method explicitly after their own initialization is complete.

Regarding the duplicate refutation

One verifier flagged this as a duplicate of bug_004. Whether or not another report covers the same issue, the bug is independently confirmed and real. The JSDoc is factually wrong regardless of duplication — the duplication concern is a process question, not a technical refutation.

protected startPeriodicPing(): void {
if (this._pingTimer || !this._pingIntervalMs) {
return;
}

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();
}
};

schedulePing();
}

/**
* Stops periodic ping requests. Called automatically when the connection closes.
*/
protected stopPeriodicPing(): void {
if (this._pingTimer) {
clearTimeout(this._pingTimer);
this._pingTimer = undefined;
}
}

/**
* Closes the connection.
*/
async close(): Promise<void> {
this._closing = true;
this.stopPeriodicPing();
await this._transport?.close();
}
Comment on lines 817 to 821
Copy link
Copy Markdown

@claude claude bot Apr 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 When close() is called while a periodic ping is in-flight, _onclose() rejects all pending response handlers with SdkError(ConnectionClosed), causing the ping catch block to fire this._onerror() with that error. Users monitoring onerror for connection health will receive spurious Connection closed errors during normal, intentional shutdowns that are indistinguishable from genuine connection failures. Fix: add a _closing boolean flag set before closing the transport, and check it in the ping catch block to suppress the error.

Extended reasoning...

The Race Condition

The bug lives in startPeriodicPing() at packages/core/src/shared/protocol.ts around lines 778-804. The setInterval callback is async, so once it fires and awaits _requestWithSchema(), JavaScript cannot cancel it. Clearing the interval only prevents future firings; the already-started async function continues running. This is the core of the race.

Exact Code Path

  1. The setInterval fires. The async callback begins executing and calls _requestWithSchema({ method: 'ping' }, ...), which registers a response handler in _responseHandlers and suspends at the await.
  2. Before the server responds, the caller invokes close().
  3. close() calls stopPeriodicPing(), which does clearInterval(this._pingTimer). The in-flight async callback has already started; clearing the interval only prevents future firings.
  4. close() then calls this._transport?.close(), which triggers the transport onclose callback.
  5. That callback invokes _onclose(), which iterates over all entries in _responseHandlers and rejects each one with new SdkError(SdkErrorCode.ConnectionClosed, 'Connection closed').
  6. The pending _requestWithSchema promise now rejects with the ConnectionClosed error.
  7. The catch block in startPeriodicPing executes: since SdkError extends Error, this._onerror(error) fires.
  8. The user's onerror callback receives a SdkError(ConnectionClosed) during a normal, user-initiated shutdown.

Why Existing Code Does Not Prevent It

The close() method does call stopPeriodicPing() before this._transport?.close(), which looks like it should protect against this. But stopPeriodicPing() only prevents new timer firings. It cannot retroactively cancel an async function that has already started and is awaiting a promise. There is no mechanism in the ping catch block to distinguish intentional close from a genuine connection failure.

Impact

Any consumer that sets onerror to monitor connection health (the primary intended use case per the PR description) will see false positive ConnectionClosed errors every time close() is called while a ping happens to be in-flight. Since pings fire on a regular interval, the race is probabilistic but reliably reproducible at scale or in tests. More critically, these errors are indistinguishable from a real remote-side failure, so automated reconnection logic or alerting built on onerror will misfire.

How to Fix

Add a private _closing boolean field to Protocol, initialized to false. In close(), set this._closing = true before calling stopPeriodicPing() and this._transport?.close(). In the ping catch block, guard the onerror call:

} catch (error) {
    if (!this._closing) {
        this._onerror(error instanceof Error ? error : new Error('Periodic ping failed: ' + String(error)));
    }
}

Reset _closing to false in _onclose() after cleanup so that a reconnected protocol instance works correctly. The _closing flag cleanly suppresses the error regardless of what exact error code the transport produces on shutdown.

Step-by-Step Proof

T=0   setInterval fires; pingCallback() begins executing
T=0   _requestWithSchema registers handler H in _responseHandlers, yields at await
T=1   close() is called by user
T=1   stopPeriodicPing() clears the timer -- in-flight pingCallback is unaffected
T=1   this._transport.close() called
T=1   transport triggers onclose -> _onclose() runs
T=1   _onclose(): for each handler in responseHandlers -> handler(SdkError(ConnectionClosed))
T=1   Handler H rejects; pingCallback resumes in catch block
T=1   catch(error): this._onerror(error) -- SPURIOUS ERROR DELIVERED TO USER
T=1   user onerror fires with 'Connection closed' during intentional shutdown


Expand Down
Loading
Loading