feat(client): add reconnectionScheduler to StreamableHTTPClientTransport - #1763
feat(client): add reconnectionScheduler to StreamableHTTPClientTransport#1763felixweinberger merged 7 commits into
Conversation
Sorry, something went wrong.
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: 85d4cc0 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 |
Sorry, something went wrong.
|
@modelcontextprotocol/client
npm i https://pkg.pr.new/modelcontextprotocol/typescript-sdk/@modelcontextprotocol/client@1763
npm i https://pkg.pr.new/modelcontextprotocol/typescript-sdk/@modelcontextprotocol/server@1763
npm i https://pkg.pr.new/modelcontextprotocol/typescript-sdk/@modelcontextprotocol/express@1763
npm i https://pkg.pr.new/modelcontextprotocol/typescript-sdk/@modelcontextprotocol/hono@1763
npm i https://pkg.pr.new/modelcontextprotocol/typescript-sdk/@modelcontextprotocol/node@1763 commit: 85d4cc0 |
Sorry, something went wrong.
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, setTimeout fallback, cancel-on-close, void-tolerant schedulers, and clearTimeout on close without a scheduler.
- 2 existing tests were updated to reference the renamed private field (_cancelReconnection instead 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.
Sorry, something went wrong.
|
Picked this up as something we discussed previously @mattzcarey |
Sorry, something went wrong.
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.
Sorry, something went wrong.
- guard reconnect() against late firing after close via aborted-signal check - wrap _cancelReconnection in try/finally so close() always aborts and fires onclose - wrap recursive _scheduleReconnection in try/catch to route scheduler errors to onerror instead of unhandled rejection - export ReconnectionScheduler type from package index - add tests for late-firing reconnect and throwing cancel function
bhosmer-ant
left a comment
There was a problem hiding this comment.
All four findings addressed — the abort-guard, try/finally in close(), try/catch around the recursive _scheduleReconnection, and the type export are all correct and minimal. Two new tests cover the edge cases (late-firing reconnect after close, cancel-throws-but-abort-still-fires). Bughunter ran clean (21 candidates, 0 confirmed). LGTM.
One pre-existing minor inconsistency (not introduced here, not blocking): the _scheduleReconnection call at ~line 438 (graceful-disconnect path) isn't directly wrapped like the one at 459 — but it's inside the outer try, so a throwing scheduler there gets caught at line 447 with a slightly misleading "SSE stream disconnected" message. Optional follow-up at most.
Sorry, something went wrong.
There was a problem hiding this comment.
LGTM
Extended reasoning...Overview
This PR adds an optional reconnectionScheduler callback to StreamableHTTPClientTransportOptions, allowing non-persistent environments (serverless, mobile, desktop sleep/wake) to override the default setTimeout-based SSE reconnection. The change touches one source file (streamableHttp.ts), a new companion examples file, the package index re-export, and comprehensive tests.
Security risks
None. The change is additive — a new optional configuration callback. No auth, crypto, or permissions code is modified. The scheduler receives only a reconnect callback and delay/attempt metadata; it cannot access internal transport state.
Level of scrutiny
Medium. This is a well-scoped, non-breaking feature addition to a transport layer. All three reviewer suggestions from bhosmer-ant (abort guard in reconnect closure, try/finally in close(), try/catch around scheduler in .catch handler) have been incorporated and their comments resolved. My previous nit about the examples convention has also been addressed with a proper streamableHttp.examples.ts companion file. 8 new tests cover the scheduler, fallback, cancel, void-return, late-fire, and cancel-throws scenarios.
Other factors
- Default behavior is unchanged (falls back to setTimeout)
- Internal field rename (_reconnectionTimeout → _cancelReconnection) is private API only
- Changeset included for minor version bump
- Existing tests updated to use the new internal field name
- No CODEOWNERS concerns for these files
Sorry, something went wrong.
bhosmer-ant
left a comment
There was a problem hiding this comment.
Re-approving after main merge + the .examples.ts companion file move (per repo convention). No functional changes to the scheduler code since my previous approval.
Sorry, something went wrong.
| if (this._reconnectionScheduler) { | ||
| const cancel = this._reconnectionScheduler(reconnect, delay, attemptCount); | ||
| if (typeof cancel === 'function') { | ||
| this._cancelReconnection = cancel; | ||
| } |
There was a problem hiding this comment.
🟡 Nit: When the custom reconnectionScheduler returns void (no cancel function), _cancelReconnection is not reset to undefined, so it can retain a stale cancel function from a prior _scheduleReconnection call. The setTimeout path (line 360) always sets _cancelReconnection, but the scheduler path only sets it conditionally. Fix: this._cancelReconnection = typeof cancel === "function" ? cancel : undefined;
Extended reasoning...When _scheduleReconnection is called and a custom reconnectionScheduler is provided, the return value is checked with typeof cancel === "function". If it is a function, _cancelReconnection is set. However, if the scheduler returns void, _cancelReconnection is left untouched — it retains whatever value it held from a previous call.
Compare this with the default setTimeout path, which unconditionally sets _cancelReconnection = () => clearTimeout(handle). The two branches have inconsistent behavior: one always updates the field, the other only conditionally updates it.
Step-by-step proof: Consider two SSE streams (GET + POST) that disconnect concurrently:
- Stream A disconnects. _scheduleReconnection is called. The custom scheduler returns cancelA. Now _cancelReconnection = cancelA.
- Before cancelA fires, Stream B disconnects. _scheduleReconnection is called again. This time the scheduler returns void (e.g., it delegates to a fire-and-forget platform API). Since typeof undefined \!== "function", the if branch is skipped. _cancelReconnection still holds cancelA — a stale reference.
- On close(), this._cancelReconnection?.() calls the stale cancelA instead of having no cancel for Stream B's reconnection. Stream B's pending reconnection cannot be cancelled via this mechanism.
The practical impact is very low. The reconnect closure already guards against a closed transport with if (this._abortController?.signal.aborted) return (line 343), so a late-firing reconnect after close() is safely no-oped. Calling a stale cancel function is typically harmless (a no-op on an already-fired callback). The scenario also requires specific concurrent stream timing that is uncommon in practice.
The fix is a one-line change to unconditionally assign _cancelReconnection:
this._cancelReconnection = typeof cancel === "function" ? cancel : undefined;This maintains the invariant that _cancelReconnection always reflects the currently scheduled reconnection, consistent with the setTimeout path.
Sorry, something went wrong.
bhosmer-ant
left a comment
There was a problem hiding this comment.
Good catch on the void-return edge case — when _scheduleReconnection is invoked from _handleSseStream (not via the reconnect closure which pre-clears), a void-returning scheduler would leave the previous cancel stale. The always-set ternary is the right fix. LGTM.
Sorry, something went wrong.
Adds a ReconnectionScheduler callback option to StreamableHTTPClientTransportOptions so non-persistent environments can override the default setTimeout-based SSE reconnection scheduling.
Motivation and Context
Fixes #1162. The current _scheduleReconnection uses setTimeout, 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-in setTimeout is cleared. Thanks @CHOIJEWON for the original implementation.
How Has This Been Tested?
6 new tests in packages/client/test/client/streamableHttp.test.ts:
pnpm --filter @modelcontextprotocol/client test passes (317 tests).
Breaking Changes
None. New optional option, default behavior unchanged. Internal _reconnectionTimeout field 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.