feat(client): add reconnectionScheduler to StreamableHTTPClientTransport#1763
feat(client): add reconnectionScheduler to StreamableHTTPClientTransport#1763felixweinberger wants to merge 1 commit intomainfrom
Conversation
Adds a ReconnectionScheduler callback option so non-persistent environments can override the default setTimeout-based SSE reconnection scheduling. The scheduler receives (reconnect, delay, attemptCount) and may return a cancel function that is invoked on transport.close(). This ensures pending custom-scheduled reconnections can be aborted the same way the built-in setTimeout is cleared. Replaces the _reconnectionTimeout field with a unified _cancelReconnection callback that works for both the default and custom scheduler paths. Fixes #1162 Closes #1177 Co-authored-by: CHOIJEWON <alsrn6040@kakao.com>
🦋 Changeset detectedLatest commit: 2d1622d The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
@modelcontextprotocol/client
@modelcontextprotocol/server
@modelcontextprotocol/express
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — clean, minimal API addition with good test coverage and no change to default behavior.
Extended reasoning...
Overview
This PR adds a single optional reconnectionScheduler callback to StreamableHTTPClientTransportOptions, allowing non-persistent environments (serverless, mobile, desktop sleep/wake) to override the default setTimeout-based SSE reconnection. The implementation touches one source file (streamableHttp.ts) and its test file, plus a changeset. The internal _reconnectionTimeout field is replaced by a unified _cancelReconnection callback that works for both the default setTimeout path and custom schedulers.
Security risks
None. The change is a scheduling mechanism for reconnection — it does not touch auth, credentials, permissions, or data handling. The scheduler callback receives only a reconnect function, a delay number, and an attempt count.
Level of scrutiny
Low-to-moderate. This is a new optional configuration option with no change to default behavior. The API surface is small (ReconnectionScheduler type + one optional property). The refactoring from _reconnectionTimeout to _cancelReconnection is purely internal (private fields). The implementation correctly handles void returns from the scheduler, clears _cancelReconnection before invoking reconnect(), and the close() method properly calls and nullifies the cancel function.
Other factors
- 6 new focused tests cover: scheduler invocation with correct arguments,
setTimeoutfallback, cancel-on-close, void-tolerant schedulers, andclearTimeouton close without a scheduler. - 2 existing tests were updated to reference the renamed private field (
_cancelReconnectioninstead of_reconnectionTimeout). - No CODEOWNERS-specific paths are affected beyond the default wildcard.
- The changeset correctly marks this as a minor version bump.
- No outstanding reviewer comments or unaddressed feedback in the timeline.
|
Picked this up as something we discussed previously @mattzcarey |
bhosmer-ant
left a comment
bhosmer-ant
left a comment
There was a problem hiding this comment.
Nice feature — the API design is clean (passing the computed delay as a suggestion is a good call) and the _cancelReconnection abstraction is cleaner than the old timeout field. A few things to tighten up around the weaker cancellation contract that a user-supplied scheduler introduces:
Also: ReconnectionScheduler isn't re-exported from packages/client/src/index.ts — users can't import the type to annotate their scheduler implementation without reaching into the internal path.
| this._reconnectionTimeout = setTimeout(() => { | ||
| // Use the last event ID to resume where we left off | ||
| const reconnect = (): void => { | ||
| this._cancelReconnection = undefined; |
There was a problem hiding this comment.
The reconnect closure should guard against the transport already being closed:
const reconnect = (): void => {
this._cancelReconnection = undefined;
if (this._abortController?.signal.aborted) return; // ← add this
this._startOrAuthSse(options).catch(...);
};With setTimeout, clearTimeout is a hard guarantee — the callback won't fire after clear. A user-supplied cancel function has no such guarantee (or may return void, meaning no cancel at all). A late-firing reconnect would call _startOrAuthSse with an aborted signal → rejects → .catch schedules another reconnection, emitting spurious onerror("Failed to reconnect SSE stream") events on a closed transport (bounded by maxRetries, but noisy).
The existing _handleSseStream call sites already have this guard (lines ~432, ~451) — just need it here too.
| clearTimeout(this._reconnectionTimeout); | ||
| this._reconnectionTimeout = undefined; | ||
| if (this._cancelReconnection) { | ||
| this._cancelReconnection(); |
There was a problem hiding this comment.
If the user-provided cancel function throws, _abortController.abort() and onclose never fire — the transport stays half-open. Worth wrapping in try/finally (same theme as the try/finally additions in #1735):
try {
this._cancelReconnection?.();
} finally {
this._cancelReconnection = undefined;
this._abortController?.abort();
this.onclose?.();
}| this._startOrAuthSse(options).catch(error => { | ||
| this.onerror?.(new Error(`Failed to reconnect SSE stream: ${error instanceof Error ? error.message : String(error)}`)); | ||
| // Schedule another attempt if this one failed, incrementing the attempt counter | ||
| this._scheduleReconnection(options, attemptCount + 1); |
There was a problem hiding this comment.
If _scheduleReconnection throws here (e.g., the user's scheduler throws), it's inside a .catch handler of a fire-and-forget promise chain — it becomes an unhandled rejection. Worth a try/catch that routes to onerror:
this._startOrAuthSse(options).catch(error => {
this.onerror?.(new Error(`Failed to reconnect SSE stream: ...`));
try {
this._scheduleReconnection(options, attemptCount + 1);
} catch (scheduleError) {
this.onerror?.(scheduleError instanceof Error ? scheduleError : new Error(String(scheduleError)));
}
});2 participants
Adds a
ReconnectionSchedulercallback option toStreamableHTTPClientTransportOptionsso non-persistent environments can override the defaultsetTimeout-based SSE reconnection scheduling.Motivation and Context
Fixes #1162. The current
_scheduleReconnectionusessetTimeout, which doesn't work well for:Supersedes #1177 with one API addition: the scheduler may return a cancel function that is invoked on
transport.close(), so pending custom-scheduled reconnections can be aborted the same way the built-insetTimeoutis cleared. Thanks @CHOIJEWON for the original implementation.How Has This Been Tested?
6 new tests in
packages/client/test/client/streamableHttp.test.ts:(reconnect, delay, attemptCount)setTimeoutwhen no scheduler providedsetTimeoutnot used when scheduler providedclose()void(no cancel)setTimeoutstill cleared onclose()pnpm --filter @modelcontextprotocol/client testpasses (317 tests).Breaking Changes
None. New optional option, default behavior unchanged. Internal
_reconnectionTimeoutfield renamed to_cancelReconnection(private, not part of public API).Types of changes
Checklist
Additional context
API:
The
_reconnectionTimeout?: ReturnType<typeof setTimeout>field is replaced by_cancelReconnection?: () => void, unifying cleanup for both the default and custom scheduler paths.