fix(client): persist SSE reconnection attempt count across idle-close cycles

[claude]

Requested by Felix Weinberger · Slack thread

Fixes #2682

Problem

StreamableHTTPClientTransport's standby GET/SSE stream reconnects forever (~once per initialReconnectionDelay, indefinitely) when the server gracefully idle-closes it — which is spec-compliant server behavior. maxRetries (default 2) never trips.

Root cause in packages/client/src/client/streamableHttp.ts: both reconnect branches of _handleSseStream hardcode the attempt count to 0 when scheduling a reconnection, and _scheduleReconnection only advances the counter inside its failed-fetch .catch(). So every successful-connect-then-close cycle re-enters _handleSseStream and schedules with 0 again — maxRetries and reconnectionDelayGrowFactor are dead for this path, and each cycle re-runs the authenticated fetch path (continuous auth/token churn).

Reproduced live on main with a local server that opens then idle-closes the standby stream after 50ms: 20 GETs in 5 seconds at a fixed delay, no onerror, with maxRetries: 2.

Fix

Persist the attempt count across connect-then-close cycles that make no progress, and reset it only when a stream did:

• _scheduleReconnection's reconnect closure threads attemptCount + 1 into _startOrAuthSse → _handleSseStream, so the stream produced by attempt n knows its place in the chain (threaded through private method parameters — no public API change; StartSSEOptions is untouched).
• _handleSseStream hoists a single scheduleNext() closure used by both the graceful-close and mid-stream-error branches (the previous duplicated block is how the two hardcoded 0s happened), scheduling with madeProgress ? 0 : reconnectAttempt.
• Progress = the stream delivered a message, or it stayed open for at least maxReconnectionDelay before ending. The lifetime rule keeps healthy-but-quiet sessions alive: a standby stream that an intermediary (e.g. an ALB with a 60s idle timeout) periodically closes reconnects indefinitely, and resetting for long-lived streams can never produce a reconnect rate faster than the maximum backoff already permits. Priming events alone deliberately do not reset the count — a server can send one and still close immediately, which would re-arm exactly the [v2] StreamableHTTPClientTransport standby GET/SSE stream reconnects forever when the server gracefully idle-closes it ( maxRetries  never trips) #2682 loop.
• Also fixed at the same (now single) scheduling site: resumptionToken: lastEventId ?? options.resumptionToken — a reconnected stream that ended before any event arrived previously dropped the Last-Event-ID token it was opened with, silently downgrading a resume into a fresh stream.

Resulting semantics: maxRetries bounds consecutive fruitless reconnection attempts — ones that fail outright (unchanged behavior) or that connect but end quickly without delivering anything (new). After the limit trips, the transport surfaces the existing onerror ("Maximum reconnection attempts exceeded") and stops, matching the documented maxRetries contract. The maxRetries JSDoc and the migration guide's reconnection-exhaustion bullet (which claimed the v1 accounting carried over) are updated to state this.

With the fix, the same live repro performs exactly 1 original + 2 reconnect GETs with growing backoff, then stops with onerror.

Tests

New Reconnection attempt accounting (#2682) describe block in packages/client/test/client/streamableHttp.test.ts:

1. Bounded idle-close loop — a standby stream that always closes empty immediately triggers exactly 1 + maxRetries fetches, then onerror, and stays stopped.
2. Productive streams don't count — streams that deliver messages keep reconnecting far past maxRetries with no error (existing behavior preserved).
3. Reset then bound again — empty close → productive stream (count resets) → empty closes trip the limit on the fresh count.
4. Long-lived idle streams are progress — streams that outlive maxReconnectionDelay before idle-closing reconnect indefinitely with no exhaustion error (the healthy quiet-session case).
5. Resumption token survives empty reconnects — after a priming event, a reconnected stream that ends empty still sends the same Last-Event-ID on the next attempt.

Existing suites pass unchanged (@modelcontextprotocol/client: 802/802 tests, typecheck, lint all green), including the prior graceful-close/priming-event reconnection tests, and the retry: field still controls the delay.

Changeset

@modelcontextprotocol/client: patch — behavior fix only, no API surface change.

---

Generated by Claude Code

[changeset-bot]

🦋 Changeset detected

Latest commit: f4a5e03

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 6 packages

Name	Type
@modelcontextprotocol/client	Patch
@modelcontextprotocol/core	Patch
@modelcontextprotocol/server	Patch
@modelcontextprotocol/server-legacy	Patch
@modelcontextprotocol/codemod	Patch
@modelcontextprotocol/core-internal	Patch

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

[pkg-pr-new]

Open in StackBlitz

@modelcontextprotocol/client

npm i https://pkg.pr.new/@modelcontextprotocol/client@2684

@modelcontextprotocol/codemod

npm i https://pkg.pr.new/@modelcontextprotocol/codemod@2684

@modelcontextprotocol/core

npm i https://pkg.pr.new/@modelcontextprotocol/core@2684

@modelcontextprotocol/server

npm i https://pkg.pr.new/@modelcontextprotocol/server@2684

@modelcontextprotocol/server-legacy

npm i https://pkg.pr.new/@modelcontextprotocol/server-legacy@2684

@modelcontextprotocol/express

npm i https://pkg.pr.new/@modelcontextprotocol/express@2684

@modelcontextprotocol/fastify

npm i https://pkg.pr.new/@modelcontextprotocol/fastify@2684

@modelcontextprotocol/hono

npm i https://pkg.pr.new/@modelcontextprotocol/hono@2684

@modelcontextprotocol/node

npm i https://pkg.pr.new/@modelcontextprotocol/node@2684

commit: f4a5e03

[claude]

Additional findings (outside current diff — PR may have been updated during review):

• 🟣 packages/client/src/client/streamableHttp.ts — Pre-existing, amplified by this change: the send()-resume branch forwards only {resumptionToken, replayMessageId, requestSignal} to _startOrAuthSse, dropping the caller's onresumptiontoken and never accepting onRequestStreamEnd — so when the NEW fruitless-reconnect exhaustion trips for a resumed request, options.onRequestStreamEnd?.() at line 683 is a no-op and the pending request gets no per-request signal at all.

  Extended reasoning...

  A client resumes a long-running request via protocol.request(..., { resumptionToken, onresumptiontoken }) (protocol.ts:1572 passes both to transport.send). transport.send hits the resumption branch at streamableHttp.ts:1003-1015, which calls _startOrAuthSse({ resumptionToken, replayMessageId, requestSignal }) — contrast resumeStream() at lines 1296-1300, which does thread onresumptiontoken. Two consequences: (1) new event IDs delivered on the resumed stream never reach the caller's onresumptiontoken, so an app persisting tokens for crash-resume keeps a stale token; (2) under this PR's new accounting, when the resumed GET stream is gracefully idle-closed maxRetries consecutive times without a message (server has nothing to replay yet because the tool is still running), _scheduleReconnection exhausts at line 680-684 and fires only transport-level onerror — options.onRequestStreamEnd is undefined on this path, so the request never settles and hangs until its timeout (or indefinitely with resetTimeoutOnProgress/long timeouts, the typical resumable-request configuration). Be

  Verification: pre-existing — the claim is factually accurate and the reachable half is concrete, though the defective lines predate this PR (the diff modifies their callee and makes the exhaustion consequence reachable). Decisive citations, all in /home/claude/typescript-sdk/packages/client/src/client/streamableHttp.ts at HEAD: line 1001 destructures `const { resumptionToken, onresumptiontoken } = options || {}

[claude]

Beyond the inline findings, this run also examined and ruled out three candidates in the newest commits: onerror firing on intentional aborts in the resumed-send failure catch (pre-existing behavior, not introduced by the new abort guard), the madeProgress duration threshold's interaction with a server-supplied SSE retry: field, and a throwing onRequestStreamEnd callback being re-invoked by the settlement guards (edge case in caller-owned code, nit-level at most).

Extended reasoning...

This is a findings-present run (three confirmed inline comments will be posted), so the body is limited to the brief ruled-out note. All three ruled-out candidates concern code added in the latest pushes (the resume-failure abort guard from f83507f, the madeProgress threshold from 1d296bd, and the settlement guards from b3d8c77), so they were not covered by any prior review round on this PR — I verified the referenced code exists at HEAD in /home/claude/typescript-sdk/packages/client/src/client/streamableHttp.ts (lines 783, 1033-1045). The hunt exited on max_bugs rather than running dry, and confirmed findings are being posted, so approval is off the table; the note is informational only and does not restate the inline findings.

Additional findings (outside current diff — PR may have been updated during review):

• 🟡 packages/client/src/client/streamableHttp.ts — nit: maxReconnectionDelay gained a second, unrelated semantic in this diff — it is now also the progress threshold for the fruitless-reconnect accounting (madeProgress = receivedMessage || Date.now() - streamOpenedAt >= this._reconnectionOptions.maxReconnectionDelay, line 783) — but its own JSDoc (lines 100-104) still describes it solely as 'Maximum backoff time between reconnection attempts in milliseconds.' The new role is documented only on maxRetries's JSDoc and in the changeset.

  Extended reasoning...

  Concrete cost: a user reading the maxReconnectionDelay doc to tune backoff pacing (e.g. raising it to 300_000 to reconnect gently, or lowering it to 2_000 for snappy recovery) has no way to know they are simultaneously retuning when an idle standby stream counts as 'made progress': raising it makes healthy sessions behind an intermediary with a shorter idle timeout count every cycle as fruitless (exhausting the notification channel after maxRetries), and lowering it makes rapid idle-close loops count as progress and reconnect forever — the exact #2682 loop the PR fixes. Fix is a two-line JSDoc addition on maxReconnectionDelay (lines 100-104) cross-referencing the progress-threshold role the diff already documented on maxRetries (lines 119-130), so both knobs' docs describe the coupling.

  Verification: nit — the factual claim checks out. This diff gives maxReconnectionDelay a second semantic: /home/claude/typescript-sdk/packages/client/src/client/streamableHttp.ts:783 (new in this PR) reads const madeProgress = receivedMessage || Date.now() - streamOpenedAt >= this._reconnectionOptions.maxReconnectionDelay;, making the option the threshold that decides whether a reconnected stream "made pro

• 🟣 packages/client/src/client/streamableHttp.ts — Pre-existing, made reachable/consequential by this diff: the exhaustion branch of _scheduleReconnection calls this.onerror before options.onRequestStreamEnd without try/finally, and the third route into it — the reconnect-failure chain's catch at lines 702-710 — has no caller-side compensation, so a throwing user onerror handler skips settlement and the resumed request hangs. The PR guarded the other two routes (graceful-close at 868-873 and mid-stream-error at 897-902) against exactly this throwing-handler scenario (and ships a test for it) but left this sibling path with the very failure the settlement threading claims to eliminate; fixing at depth (try/finally around lines 681-683) would cover all three routes and remove the duplicated caller-side catches.

  Extended reasoning...

  A caller resumes a long-running request or listen subscription with transport.send(msg, { resumptionToken, onRequestStreamEnd }) — the settlement contract this PR completes (client.ts:2112 relies on onRequestStreamEnd to settle the subscription state machine). The resumed stream ends fruitlessly once (scheduled via scheduleNext), then the next reconnect attempts fail outright (network flake), driving the chain at streamableHttp.ts:702-710: each failure calls this._scheduleReconnection(options, attemptCount + 1) from inside the catch. When attemptCount+1 reaches maxRetries, line 681 invokes this.onerror('Maximum reconnection attempts (N) exceeded.'); if the app's onerror handler throws (the same scenario the PR's own new test 'does not double-fire exhaustion callbacks when a user onerror handler throws on graceful close' exercises for the graceful route), the throw skips options.onRequestStreamEnd?.() at line 683 and lands in the catch at lines 707-709, which only re-emits onerror and never settles. Unlike the graceful-close route (line 872) and the error route (line 901), nothing fir

  Verification: pre-existing — the defect is real, and the diff directly interacts with the defective closure. In /home/claude/typescript-sdk/packages/client/src/client/streamableHttp.ts the exhaustion branch runs the user callback before the settlement callback with no try/finally (lines 680-684): this.onerror?.(new Error(\Maximum reconnection attempts (${maxRetries}) exceeded.`)); options.onRequestStreamEn

• 🟣 packages/client/src/client/streamableHttp.ts — Pre-existing, nit (in the function this diff edits): _cancelReconnection is a single slot shared by all concurrent reconnect chains, so when both the standby GET chain and a primed per-request stream chain have backoff timers pending, the later _scheduleReconnection call overwrites the earlier chain's cancel function and close() can only cancel one timer (and with a custom ReconnectionScheduler, the dropped cancel is never invoked).

  Extended reasoning...

  This PR makes resumed sends full participants in the reconnect machinery (threading onRequestStreamEnd/onresumptiontoken through _send's resume branch), so two chains realistically coexist: the standby GET stream is in a backoff delay (timer A, _cancelReconnection = clear A) when a long-running tool call's POST-SSE stream drops after a priming event and its scheduleNext calls _scheduleReconnection, setting _cancelReconnection = clear B and discarding clear A (lines 713-718). The user then calls transport.close(): line 977 cancels only timer B; timer A stays armed for up to maxReconnectionDelay (30s default). Its reconnect no-ops on the aborted guard (line 695), but the un-cancelled, non-unref'd setTimeout keeps a Node process alive up to 30s after close, and with a custom ReconnectionScheduler (line 714) the dropped cancel function means the scheduler's resources for chain A are never released and its callback fires post-close. Fix: keep a set of cancel functions (or per-chain slots) and cancel all of them in close().

  Verification: pre-existing — The claim is factually accurate. _cancelReconnection is a single instance field (packages/client/src/client/streamableHttp.ts:338) and every _scheduleReconnection call unconditionally overwrites it: line 715 (this._cancelReconnection = typeof cancel === 'function' ? cancel : undefined for a custom ReconnectionScheduler) and line 718 (`this._cancelReconnection = () => clearTi

[claude]

Addressed the latest review round's findings in abcf521:

• Settlement at depth: _scheduleReconnection now guarantees the caller settles on every no-reconnection-pending exit — the exhaustion branch wraps onerror in try/finally (so a throwing user handler can't skip onRequestStreamEnd on any of the three routes in), and a throwing custom scheduler settles before rethrowing. The caller-side catches in _handleSseStream are containment-only now, removing the duplicated settlement.
• All pending reconnections tracked: the single _cancelReconnection slot is now a Set, so close() cancels every parked timer across concurrent chains (standby GET plus resumed per-request streams), not just the latest. Regression test asserts both timers are cancelled.
• maxReconnectionDelay JSDoc now documents its second role as the stream-lifetime progress threshold, cross-referencing maxRetries.

Package tests 806/806, typecheck and lint green.

---

Generated by Claude Code

[claude]

🟡 reconnectAttempt is threaded as a 4th optional positional parameter through _startOrAuthSse and must be manually re-passed at every self-recursion site (lines 590, 615, 713); a future call site that omits it silently defaults to 0 and resets the retry accounting — the exact silent-reset bug class this PR exists to fix — with no compiler help.

Extended reasoning...

Concrete cost: fragile manual threading. The PR itself had to edit three recursion sites (401 auth retry at 590, step-up retry at 615, reconnect closure at 713) to keep the count alive; because the parameter is optional with a 0 default, TypeScript cannot flag a fourth future call site (e.g. a new auth branch) that forgets it, and the symptom would be an unbounded reconnect loop indistinguishable from the original #2682 bug. Folding isAuthRetry/stepUpRetries/reconnectAttempt into a single internal state object (or carrying reconnectAttempt inside StartSSEOptions, which is already passed everywhere) makes omission impossible rather than silently wrong.

Verification: nit — the claim is factually accurate. packages/client/src/client/streamableHttp.ts:532 declares private async _startOrAuthSse(options: StartSSEOptions, isAuthRetry = false, stepUpRetries = 0, reconnectAttempt = 0): the new count is a 4th optional positional parameter defaulting to 0. The PR indeed had to hand-thread it at all three self-recursion sites — line 590 (`return this._startOrAuthSse(o

[claude]

Fair structural point — deferring it from this PR. StartSSEOptions is a public exported interface, so folding reconnectAttempt into it either adds an internal knob to the public surface or means introducing a separate internal options type and touching every call site; both go beyond this fix's footprint. The positional threading is currently exercised at all three recursion sites by the regression tests (auth-retry and step-up recursions preserve the count). Happy to do the internal-options-type refactor as a follow-up.

---

Generated by Claude Code

[claude]

Additional findings (outside current diff — PR may have been updated during review):

• 🟡 packages/client/src/client/streamableHttp.ts — Stale JSDoc on _scheduleReconnection: documents a nonexistent lastEventId param and describes attemptCount as "for this specific stream", contradicting the new cross-stream persistent accounting this PR introduces (same stale phrase on _getNextReconnectionDelay at line 654).

  Extended reasoning...

  Concrete cost: the doc block directly above the function this PR heavily rewrites says '@ param lastEventId The ID of the last received event' (the first parameter is options: StartSSEOptions) and '@ param attemptCount Current reconnection attempt count for this specific stream' — but the diff's core change is that the attempt count now persists across connect-then-close stream cycles (threaded via reconnectAttempt through _startOrAuthSse/_handleSseStream). A maintainer reading the JSDoc will assume the pre-PR per-stream-reset semantics, the exact misunderstanding that caused bug #2682. One-line fix: update both JSDoc comments to match the persistent accounting.

  Verification: nit — packages/client/src/client/streamableHttp.ts lines 675-676: JSDoc says "@ param lastEventId The ID of the last received event for resumability" but the signature at line 678 is _scheduleReconnection(options: StartSSEOptions, attemptCount = 0) — no lastEventId param exists. Line 676 says "attempt count for this specific stream", yet the PR's own comment at lines 713-718 states the cou

[claude]

I re-reviewed the latest revision (through f4a5e03) and found no new bugs — all previously raised threads now have corresponding fixes — but given how intricate the reconnection-accounting and settlement semantics became over these iterations, a human look is still worthwhile before merge.

What was reviewed on this pass:

• The two most recent fixes: the at-most-once onRequestStreamEnd wrapper installed at the send() entry point, and the scheduler-throw catch that now settles only when its registry delete proves no attempt was dispatched.
• Progress accounting (message receipt or stream lifetime >= maxReconnectionDelay via performance.now()), resumption-token carry-forward, and close() cancelling every pending reconnection.
• Changeset and migration-guide prose against the shipped behavior — both now describe the new accounting accurately.

Extended reasoning...

Overview

The PR changes StreamableHTTPClientTransport (packages/client/src/client/streamableHttp.ts, +284/-60) so the SSE reconnection attempt count persists across connect-then-idle-close cycles that make no progress, fixing an unbounded reconnect loop against servers that gracefully idle-close the standby GET stream. Over ten commits it grew to also cover: resumption-token carry-forward on empty reconnected streams, a pending-reconnection registry cancelled in close(), threading onRequestStreamEnd through resumed send() so pending requests settle instead of hanging, an at-most-once settlement wrapper, monotonic performance.now() lifetime measurement, and matching migration-guide/changeset updates. Tests add ~477 lines covering the new paths.

Security risks

No meaningful security surface: no auth, crypto, or permission logic is touched. The auth-retry recursion sites in _startOrAuthSse are edited only to thread the attempt count and callback wrapper through; the OAuth flow itself is unchanged. The main risk class here is availability (a healthy session's notification channel being wrongly shut down, or a request hanging), which the iterations specifically addressed and which tests now exercise.

Level of scrutiny

High. This is client-transport lifecycle code with subtle exactly-once settlement and cancellation invariants, and the review history itself demonstrates the failure-mode density: nine earlier review rounds each surfaced real issues (permanent standby-channel shutdown on healthy idle sessions, dropped resumption tokens, double/missed settlement, stale registry entries). The final state is internally consistent — the scheduler-throw catch settles only when its _pendingReconnections.delete() succeeds, and the atMostOnce wrapper enforces the settlement contract at the entry point — but the interaction space (custom schedulers, synchronous dispatch, auth retries, concurrent standby + resumed chains) is large enough that I cannot certify no human needs to look. Approval is therefore not appropriate despite the zero-findings run.

Other factors

This run's hunt exited via dry_streak with no findings after the final commit, which is a genuine state change: every prior round found something, and this is the first clean pass over the completed revision, so a closing verdict adds information rather than noise. The changeset and the rewritten migration-guide bullet (previously flagged as contradicting the change) now accurately describe the new accounting, including the progress-reset conditions. Test coverage for the new behavior is substantial, including hostile-callback and synchronous-scheduler cases added alongside each fix. All prior threads were resolved only by this bot's own session, but each has a plausibly-addressing commit verified against the code in this run.