fix(client): make streamable HTTP auth awaits abortable by requestSignal - #2644
fix(client): make streamable HTTP auth awaits abortable by requestSignal#2644claude[bot] wants to merge 8 commits into
Conversation
token(), onUnauthorized 401 recovery, and step-up authorization were awaited with no path for the per-request or transport abort signal to reach them, so a hung auth flow parked send() forever. Race those awaits against the combined signal and offer it to onUnauthorized via the new optional UnauthorizedContext.signal field. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DThnF41VH9fXQPjGmRySxa
🦋 Changeset detectedLatest commit: 53c393c The changes in this PR will be included in the next version bump. This PR includes changesets to release 6 packages
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/codemod
@modelcontextprotocol/core
@modelcontextprotocol/server
@modelcontextprotocol/server-legacy
@modelcontextprotocol/express
@modelcontextprotocol/fastify
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| // Per-request abort: when the caller supplies a request-scoped | ||
| // signal (the `subscriptions/listen` driver), aborting it cancels | ||
| // this POST and its SSE response stream without closing the | ||
| // transport. Combined BEFORE header acquisition so the auth chain | ||
| // (token(), 401 recovery, step-up) is raced against it too. | ||
| const transportSignal = this._abortController?.signal; | ||
| const signal = | ||
| options?.requestSignal !== undefined && transportSignal !== undefined | ||
| ? anySignal(transportSignal, options.requestSignal) | ||
| : (options?.requestSignal ?? transportSignal); | ||
|
|
||
| const headers = await this._commonHeaders(signal); |
There was a problem hiding this comment.
🟣 Pre-existing issue (not introduced by this PR): the #2641 listen() teardown wedge this PR cites as motivation still survives on the awaited McpSubscription.close() path — wireTeardown() awaits the notifications/cancelled send, which carries no requestSignal, so with a hung token() the raced _commonHeaders only sees the transport-lifetime signal and await subscription.close() parks until the whole transport closes. Fix locus is client.ts's wireTeardown plumbing: don't await the cancelled notification on the public close() path, bound it, or give notification sends an abort channel.
Extended reasoning...
What survives
This PR makes the auth chain abortable wherever a requestSignal reaches _send — which fixes the listen POST itself. But the teardown path the PR description names ("Since Client.listen()'s teardown relies on requestSignal reaching the transport, these parks are also one of the ways the #2641 listen() wedge stays stuck") has a second await that never gets a requestSignal: the notifications/cancelled send.
Code path
McpSubscription.close()(packages/client/src/client/client.ts:2060-2064) awaitswireTeardown().wireTeardown()(client.ts:2055-2058) callsrequestAbort.abort()— which, thanks to this PR, now settles the parked listen POST — and then doesawait this.notification({ method: 'notifications/cancelled', ... }).catch(() => {}).Protocol.notification→_notificationViaCodec→await this._transport.send(jsonrpcNotification, options)(protocol.ts:1645).NotificationOptionscarries onlyrelatedRequestId— there is norequestSignalchannel for notifications.- In the patched
_send(streamableHttp.ts:1045-1056), a signal-less send collapses the combined signal tooptions?.requestSignal ?? transportSignal— i.e. the transport-lifetime signal alone. _commonHeaders(signal)therefore races the hungtoken()only againstthis._abortController.signal, which by design never aborts on a subscription-scoped close.raceWithSignalnever settles.
The .catch(() => {}) on the notification only swallows a rejection; it cannot unpark a promise that never settles. So await subscription.close() hangs until the entire transport is closed.
Step-by-step proof (the PR's own #2643 scenario)
- Modern-era Streamable HTTP connection with an
AuthProviderwhosetoken()refresh wedges (returns a never-settling promise), an opensubscriptions/listensubscription. - App calls
await subscription.close()intending to tear down just the subscription while keeping the transport. wireTeardown()runsrequestAbort.abort()→ the listen POST settles (this PR's fix works here).wireTeardown()then awaits thenotifications/cancelledsend →_sendcomputessignal = transportSignal(norequestSignal) →_commonHeadersawaitsraceWithSignal(token(), transportSignal)→ neither side ever settles.await subscription.close()never resolves. The user-visible [v2] Client.listen() rejections escape as process-level unhandledRejection while the send is in flight; a parked send hangs listen() past its ack timeout #2641 symptom survives on exactly the teardown path the PR names.
Note the sibling teardown callers — the ack-timeout (client.ts:2077) and caller-signal abort (client.ts:2092) — use void wireTeardown().catch(...) and are unaffected; only the awaited public close() path wedges. Client.close() is also safe because _resetConnectionState() settles listen state without awaiting wireTeardown.
Why this is pre-existing, and why it's still worth noting
On main the un-raced await token() parked this same notification send forever — and even a transport close didn't settle it. This PR strictly improves the path (transport close now unparks it) and doesn't touch client.ts at all; the PR description also carefully claims only that these parks are 'one of the ways' the #2641 wedge stays stuck. So this isn't a regression and shouldn't block the PR. It's flagged because the PR interacts with the exact site where the send now parks (the raced _commonHeaders) and cites the listen-teardown wedge as motivation, so the residual gap is a natural follow-up.
Suggested fix (follow-up, in client.ts)
Any of: don't await the cancelled-notification send on the public close() path (mirror the void wireTeardown().catch(...) pattern the sibling callers use); bound the await with a timeout; or add an abort channel to notification sends (e.g. thread a requestSignal through NotificationOptions) so the subscription's requestAbort signal reaches the transport for the cancelled notification too.
| /** | ||
| * Abort signal for the request (or transport) whose 401 triggered this | ||
| * recovery. The transport stops waiting for `onUnauthorized` when it | ||
| * aborts; cooperative implementations should pass it to their own fetches | ||
| * so the recovery work stops too. Optional — absent when the transport | ||
| * has no lifetime signal to offer. | ||
| */ | ||
| signal?: AbortSignal; |
There was a problem hiding this comment.
🟣 The legacy SSE client transport (packages/client/src/client/sse.ts) still contains the exact un-raced auth-await pattern this PR fixes for streamable HTTP: _commonHeaders() awaits token() with no signal, and both onUnauthorized awaits (the _send 401 recovery and the start-path handler) are un-raced and never receive the new UnauthorizedContext.signal, so a hung provider parks send() past close() there. This is a pre-existing issue in code the PR does not touch — flagging as follow-up material, not blocking.
Extended reasoning...
What survives in the sibling transport. This PR races every auth await in streamableHttp.ts against the combined request/transport signal, so a hung AuthProvider.token() or onUnauthorized() can no longer park send() past an abort (#2643). But the legacy SSE transport in the same package retains all three un-raced sites:
sse.ts:162-171—_commonHeaders()awaitsthis._authProvider?.token()with no signal and no race.sse.ts:375-386—_send's 401 recovery awaitsthis._authProvider.onUnauthorized({ response, serverUrl, fetchFn })un-raced, and without the newctx.signalfield.sse.ts:216-223— the_startOrAuthonerror handler callsonUnauthorizedthe same way.
Why the transport-lifetime wedge applies. SSEClientTransport has no requestSignal, so the per-request half of #2643 is out of scope — but the transport-lifetime half is not. close() (sse.ts:341) aborts _abortController, and _send's fetch does carry this._abortController?.signal (sse.ts:363). The problem is sequencing: the abort only reaches the fetch's init.signal, which is never hit while the send is parked before the fetch in a hung token(), or after a 401 in a hung onUnauthorized(). Nothing observes the abort in those states.
Step-by-step proof. (1) transport.send(msg) calls _send, which awaits _commonHeaders(); (2) _commonHeaders() awaits this._authProvider.token(), which returns a promise that never settles (wedged refresh, hung broker); (3) the caller invokes transport.close(), which aborts _abortController and fires onclose; (4) the parked send() never reaches line 363 where the signal would matter, has no race against the signal, and therefore never settles — the returned promise hangs forever. This is exactly the shape the PR's new test "transport close() settles a send parked in a hung token()" verifies is fixed for streamable HTTP; the same scenario against SSEClientTransport still hangs.
Why this is in scope to mention. The repo's Completeness convention says: when a PR replaces a pattern, grep the package for surviving instances of the old form — partial migrations leave sibling code paths with the very bug the PR claims to fix (#1657, #1761, #1595). Additionally, the PR modifies the shared UnauthorizedContext interface (auth.ts:58-65, adding signal?) that both transports consume — after this PR, only one of its two in-repo callers populates the field, so providers used with the SSE transport never see ctx.signal.
Why it is not blocking. All three verifiers agreed on pre-existing severity: sse.ts is untouched by this PR, it is the deprecated legacy transport, #2643 is explicitly scoped to streamable HTTP, and the PR describes itself as a focused fix. The fix — the same raceWithSignal(…, this._abortController?.signal) treatment (the helper could move somewhere shared), passing signal into the onUnauthorized context, and the same unstamped-abort discipline in the catch blocks — is a natural, mechanical follow-up.
How to fix (follow-up). Give _commonHeaders() a signal? parameter raced around token(), race both onUnauthorized awaits against this._abortController?.signal, and pass that signal as ctx.signal so cooperative providers can cancel their own recovery fetches — mirroring the streamable HTTP changes in this PR.
- suppress onerror for transport-lifetime aborts too in _send's catch and guard the resumptionToken fire-and-forget like reconnect() does - reject already-aborted sends before invoking authProvider.token() - pass the raced await's rejection through verbatim so the auth-seam stamp (a property on the thrown value, Error or not) and cause chains survive; lock in with an identity-preservation test Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DThnF41VH9fXQPjGmRySxa
|
Review addressed in bdacc42 — all three actionable findings were correct on inspection:
On the two pre-existing findings, both real but out of scope here: the awaited Generated by Claude Code |
…eWithSignal - guard terminateSession's catch and the 202/initialized standalone-GET fire-and-forget against transport-lifetime aborts, matching _send - normalize raceWithSignal's input via Promise.resolve so a plain-JS provider returning a bare value keeps working (the pre-race await tolerated it) and an executor throw cannot leak the abort listener - fold the pre-existing StartSSEOptions drop on the resume-via-send path: thread onresumptiontoken and onRequestStreamEnd like the fresh-POST path so resumed streams keep the token-persistence chain and report their terminal end Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DThnF41VH9fXQPjGmRySxa
| const isIntentionalAbort = (): boolean => this._abortController?.signal.aborted === true || requestSignal?.aborted === true; | ||
|
|
||
| try { | ||
| // Combined BEFORE header acquisition so the auth chain (token(), | ||
| // 401 recovery, step-up) is raced against the same abort as the | ||
| // GET itself. | ||
| const transportSignal = this._abortController?.signal; | ||
| const signal = | ||
| requestSignal !== undefined && transportSignal !== undefined | ||
| ? anySignal(transportSignal, requestSignal) | ||
| : (requestSignal ?? transportSignal); |
There was a problem hiding this comment.
🟣 Pre-existing issue (not introduced by this PR, which only relocates the call sites): the non-native anySignal fallback (Node 20.0–20.2, where AbortSignal.any is absent) removes its listeners only from cleanup(), which runs solely when one of the input signals aborts — so on the common path where a request completes normally, the onA listener (whose closure pins the per-request signal and controller) stays on the long-lived transport signal until close(), accumulating one listener per completed request and hitting MaxListenersExceededWarning after ~10. Fixing it needs a disposal channel — e.g. return { signal, dispose } from anySignal and dispose in a finally in _send/_startOrAuthSse.
Extended reasoning...
What the bug is. The fallback branch of anySignal (streamableHttp.ts:283–304, taken when AbortSignal.any is absent — the function's own comment notes the package's engines floor is >=20, so Node 20.0–20.2 must be served by it) registers onA on the transport-lifetime signal a and onB on the per-request signal b, both {once: true}. The only listener removal lives in cleanup(), which is invoked exclusively from onA/onB — i.e. only when one of the two input signals actually aborts. On the overwhelmingly common path — the request completes normally and neither signal ever aborts — nothing removes onA from a. Its closure references cleanup, which captures b and the fresh AbortController, so the entire per-request signal graph stays pinned on the long-lived transport signal until close() finally aborts a.\n\nThe code path that triggers it. On a modern-era Streamable HTTP session, Protocol.request supplies a requestSignal for every request: protocol.ts:1417–1418 creates requestAbort whenever the era is modern and transport.hasPerRequestStream === true (which StreamableHTTPClientTransport declares), and protocol.ts:1560 passes requestAbort?.signal into every transport.send(). So both call sites this PR relocates — _startOrAuthSse (~line 617) and _send (~line 1092) — take the anySignal(transportSignal, requestSignal) branch on every request. requestAbort.abort() runs only from cancel() (timeout / explicit cancel, protocol.ts:1473) and is skipped once the response is received, so a successfully-completed request never aborts either input signal.\n\nStep-by-step proof (Node 20.0–20.2): (1) client issues request #1 → _send calls anySignal(transportSignal, requestSignal) → fallback registers onA on transportSignal and onB on requestSignal; (2) the request completes normally → Protocol resolves, requestAbort.abort() is never called → neither onA nor onB fires, cleanup() never runs; (3) onA remains on transportSignal, its closure pinning requestSignal, onB, and the per-request AbortController; (4) repeat for requests #2–#11 → after the 11th listener Node emits MaxListenersExceededWarning: Possible EventTarget memory leak detected. 11 abort listeners added to [AbortSignal]; (5) a long-lived session (a gateway, a daemon) grows memory linearly with request count until the transport closes.\n\nWhy existing code doesn't prevent it. The function's own comment (lines 284–288) recognizes exactly this leak class, but only for the case where b aborts ("every request-scoped b that aborts would otherwise leave one listener + closure pinned on a") — the mutual-removal fix it describes never executes when neither signal aborts, which is the normal-completion case. Notably, the PR's own new raceWithSignal was built specifically to avoid this class ("The abort listener is removed as soon as the promise settles so a long-lived transport signal does not accumulate closures per request") — the sibling combinator it sits next to violates that stated discipline. The native AbortSignal.any path (Node ≥20.3) is unaffected: the spec defines GC-safe dependent signals.\n\nImpact. Bounded to runtimes lacking AbortSignal.any (Node 20.0–20.2): one leaked listener + closure per completed request, MaxListenersExceededWarning after ~10 requests, and unbounded linear memory growth over a long-lived session. No behavioral incorrectness — aborts still propagate — so this is a resource leak, not a correctness failure, on a narrow runtime range.\n\nHow to fix. A bare AbortSignal has no completion hook, so the fallback needs the consumer to release it: e.g. have anySignal return { signal, dispose } (native path returns a no-op dispose) and call dispose() in a finally in _send and _startOrAuthSse once the request settles — mirroring the remove-on-settlement discipline raceWithSignal already implements. This is pre-existing: anySignal and both call sites exist on main unchanged; this diff only moves the combined-signal computation above header acquisition (same one anySignal call per request), so it relocates but neither introduces nor worsens the leak. Flagged because the PR touches both call sites and explicitly adopts the opposite discipline in raceWithSignal — a natural follow-up, not a blocker.
…onerror, extract _combinedSignal - export UnauthorizedContext from the client barrel and document ctx.signal forwarding in machine-auth.md and upgrade-to-v2.md - changeset now mentions the folded resume-observer fix - drop the duplicate onerror emission in the resume-via-send and 202/initialized catch blocks (_startOrAuthSse already emits before rethrowing); fire onRequestStreamEnd on a genuine resume failure so the observed per-request stream does not dead-end silently - extract _combinedSignal() for the duplicated transport+request signal computation (used by _send, _startOrAuthSse, terminateSession) - use isAborted() in the two onUnauthorized catch guards Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DThnF41VH9fXQPjGmRySxa
|
Round addressed in f4e9e33: On the pre-existing non-native Generated by Claude Code |
There was a problem hiding this comment.
Beyond the inline notes (all non-blocking nits or pre-existing items), this run also examined whether auth-phase aborts reject send() with a different error shape than fetch-phase aborts — they do not: abortReasonError passes an Error reason through identity-preserved (including the default DOMException AbortError), so both phases surface the same abort reason; only non-Error reasons are normalized, per the helper's documented contract.
Extended reasoning...
This run's six findings are all nits (duplication/consolidation follow-ups, a narrow already-aborted gating gap on the step-up path, a pre-existing reconnect double-emit) or explicitly pre-existing (withOAuth middleware's un-raced auth awaits), so they do not block the PR — but because findings were posted as inline comments, no approval or deferral body is appropriate. The one candidate issue examined and refuted this run (abort-rejection shape divergence between the raced auth awaits and the fetch path) is recorded above so a later pass does not re-derive it. The PR itself remains auth-sensitive transport-lifecycle work that warrants human review regardless.
|
|
||
| if (this._authProvider.onUnauthorized && !isAuthRetry) { | ||
| try { | ||
| await this._authProvider.onUnauthorized({ | ||
| response, | ||
| serverUrl: this._url, | ||
| fetchFn: this._fetchWithInit | ||
| }); | ||
| // Raced against the per-request/transport abort so | ||
| // a hung 401 recovery cannot park the send (#2643); | ||
| // the signal is also handed to the provider so a | ||
| // cooperative implementation can cancel its own work. | ||
| await raceWithSignal( | ||
| this._authProvider.onUnauthorized({ | ||
| response, | ||
| serverUrl: this._url, | ||
| fetchFn: this._fetchWithInit, | ||
| signal | ||
| }), | ||
| signal | ||
| ); | ||
| } catch (error) { | ||
| // An abort is an intentional teardown, not an auth | ||
| // failure — leave it unstamped. | ||
| if (isAborted(signal)) { | ||
| throw error; | ||
| } | ||
| // Auth-seam stamp: covers the SDK's OAuth flow and | ||
| // custom onUnauthorized callbacks alike. | ||
| throw markAuthSeamEscape(error); |
There was a problem hiding this comment.
🟡 The ~40-line 401-recovery block (raced onUnauthorized with ctx construction, the abort-vs-stamp two-branch catch, the response drain, and the isAuthRetry/UnauthorizedError tail) plus the 403 insufficient_scope step-up race are duplicated near-verbatim between the GET path (_startOrAuthSse) and the POST path (_send) — and this PR's own history had to edit both copies in lockstep twice (c37f361 added the race wrapper + ctx.signal + catch to both; f4e9e33 swapped isAborted() into both). Extracting a shared private helper (e.g. _recoverUnauthorized(response, signal), and/or folding the race+signal into _stepUpAuthorize which already owns method-level stamping) would make the announced follow-up — threading a signal through auth() — a one-site change instead of a four-site lockstep edit.
Extended reasoning...
What the duplication is. The 401-recovery block is character-identical between the GET path (_startOrAuthSse, ~lines 655–698) and the POST path (_send, ~lines 1150–1198) except for one word in a comment ('park the GET' vs 'park the send') and the path-specific retry-recursion target. The shared unit comprises: the WWW-Authenticate extraction + computeScopeUnion preservation, the raced await raceWithSignal(this._authProvider.onUnauthorized({ response, serverUrl: this._url, fetchFn: this._fetchWithInit, signal }), signal), the two-branch catch (if (isAborted(signal)) throw error; else throw markAuthSeamEscape(error)), the response.text?.().catch(() => {}) drain, and the isAuthRetry → SdkHttpError / terminal UnauthorizedError tail. The two 403 insufficient_scope step-up races are equally parallel: identical raceWithSignal(this._stepUpAuthorize({...}, stepUpRetries), signal) + 'AUTHORIZED' check + stepUpRetries + 1 recursion (~704–716 vs ~1206–1218).\n\nWhy this PR concretely demonstrates the cost. The pre-PR duplication was a plain un-raced await. This PR grew each copy in lockstep, twice, in its own commit history:\n\n1. c37f361 added the raceWithSignal wrapper, the ctx.signal field, and the two-branch abort/stamp catch — identically to both 401 sites and both step-up sites (four hunks performing the same edit).\n2. f4e9e33 (round three) swapped the inline signal?.aborted === true checks for isAborted(signal) — again applied to both copies.\n\nEach round required remembering both sites by hand. The PR's own review history shows exactly this discipline failing for path-symmetric code: the onerror-suppression guards needed fixes at four separate sites spread across bdacc42/2d4fbe7/f4e9e33 because each path carried its own copy.\n\nWhy existing consolidations don't cover it. The two extractions already performed on review are narrower: _combinedSignal() unified only the five-line signal derivation, and isAborted() unified only the abort-check expression. Neither touches the ~40-line recovery control flow, which remains fully duplicated. Nothing — no type, no test — enforces that the GET and POST paths keep identical auth-recovery semantics; they are held in sync only by author vigilance.\n\nThe drift hazard is live, not hypothetical. The PR description names the natural follow-up: threading the combined signal through the SDK's internal auth() metadata-discovery fetches ("a natural follow-up but touches the OAuth plumbing more broadly"). As the code stands, that follow-up — or any other change to the recovery contract (a second ctx field, a change to the abort/stamp classification) — must land identically at both 401 sites and both step-up sites: a four-site lockstep edit with no mechanism to catch a missed site.\n\nStep-by-step proof of the drift scenario. (1) The follow-up PR adds signal to AuthOptions and threads it through _stepUpAuthorize → auth(). (2) The author edits the POST-path step-up call (the site named in the issue repro) but misses the GET-path copy in _startOrAuthSse — plausible, since the two blocks live ~500 lines apart and are exercised by different test groups. (3) A resumed GET stream hitting a 403 now runs an uncancellable step-up while the fresh POST honors the signal: the GET and POST paths have silently diverged in abort semantics. (4) No test fails, because nothing asserts the paths behave identically. With a shared helper, step (2) is impossible — the change lands once.\n\nHow to fix. Extract a private helper, e.g. private async _recoverUnauthorized(response: Response, signal: AbortSignal | undefined): Promise<void> containing the WWW-Authenticate extraction/scope-union, the raced onUnauthorized call with ctx, the abort-vs-stamp catch, and the drain; both call sites reduce to one call plus their path-specific retry recursion (which stays at the call site — the asymmetries sit outside the extractable unit). For the step-up, either a sibling helper or fold the raceWithSignal(…, signal) into _stepUpAuthorize itself, which already takes method-level responsibility for auth-seam stamping. The helper is private, so the repo's public-API-minimalism principle is not implicated; if anything, REVIEW.md's "one way to do things" supports it.\n\nSeverity. Nit — no behavior difference exists today; both copies are currently correct and identical. The cost is the demonstrated lockstep-maintenance burden (two rounds in this PR alone) and the drift risk for the announced follow-up work. Non-blocking.
|
|
||
| const headers = await this._commonHeaders(); | ||
| // Per-request abort: when the caller supplies a request-scoped | ||
| // signal (the `subscriptions/listen` driver), aborting it cancels | ||
| // this POST and its SSE response stream without closing the | ||
| // transport. | ||
| const signal = this._combinedSignal(options?.requestSignal); | ||
|
|
||
| const headers = await this._commonHeaders(signal); |
There was a problem hiding this comment.
🟣 🟣 Pre-existing issue (this PR does not touch middleware.ts): when withOAuth(provider) is composed in via the transport's fetch option, its await provider.tokens() and 401-branch await auth(provider, ...) never consult init.signal — only the inner next() call forwards it — so while the middleware is parked there, aborting requestSignal or calling close() rejects nothing and the await fetch(url, init) in _send never settles: the exact #2643 hang survives through this documented configuration, from inside the fetch call this PR made abortable. Follow-up material for the broader auth()-signal-threading work the PR description names: withOAuth should observe init?.signal around its tokens()/auth() awaits and thread it into auth()'s fetchFn requests.
Extended reasoning...
What the bug is. withOAuth(provider) (packages/client/src/client/middleware.ts:38-96) is exported public API that users compose into a transport via StreamableHTTPClientTransportOptions.fetch. When it is the transport's fetch function, the promise awaited at this hunk's await (this._fetch ?? fetch)(this._url, init) is the middleware's, and the middleware has three await points that never consult init.signal:
await provider.tokens()inmakeRequest(middleware.ts:46) — before any fetch is in flight;await auth(provider, { serverUrl, resourceMetadataUrl, scope, fetchFn: next })in the 401 branch (middleware.ts:64-69) —auth()runs metadata discovery and token exchange throughfetchFnwith requests it constructs itself, which never carry the caller'sinit.signal;- the retry
makeRequest()(middleware.ts:80).
Only the inner next(input, {...init, headers}) call forwards init.signal to a real fetch. While the middleware is parked in (1) or (2), no in-flight fetch is listening to the combined signal — aborting requestSignal or calling close() rejects nothing, and _send's fetch await never settles.
Why the PR's fix doesn't cover it. This PR's raceWithSignal wraps the transport's own auth chain (_authProvider.token(), onUnauthorized, _stepUpAuthorize). In the withOAuth configuration the user typically sets no authProvider at all — the auth chain lives inside the fetch function, past the point where the transport hands off to fetch(url, init). The transport assumes an aborted init.signal makes the fetch reject, which is true for real fetches but false for this middleware. The PR description itself concedes the underlying gap: "threading the signal all the way through the SDK's internal auth() metadata-discovery fetches is a natural follow-up."
Step-by-step proof.
new StreamableHTTPClientTransport(url, { fetch: applyMiddlewares(withOAuth(provider), fetch) })— noauthProvider, so none of the PR's raced awaits engage.send()computes the combined signal and calls the middleware withinit.signalset to it.- The server returns 401; the middleware enters
await auth(provider, ...), whose token-endpoint fetch (viafetchFn: next, but withauth()'s own signal-less requests) wedges. - The caller aborts
requestSignal(or the app callstransport.close()). The combined signal fires — but no fetch is in flight with that signal attached, and nothing in the middleware observes it. send()stays pending forever — the exact [v2] StreamableHTTPClientTransport: auth awaits in _send are un-abortable — requestSignal cannot cancel token()/onUnauthorized/metadata-discovery parks #2643 symptom, including the [v2] Client.listen() rejections escape as process-level unhandledRejection while the send is in flight; a parked send hangs listen() past its ack timeout #2641listen()-teardown reliance the PR description cites.
Why existing code doesn't prevent it. raceWithSignal never wraps the fetch await itself (fetches are assumed abort-responsive), and withOAuth reads nothing from init besides headers. Nothing in the middleware chain propagates abort to the tokens()/auth() parks.
How to fix (follow-up, not this PR). In withOAuth: race the tokens() and auth() awaits against init?.signal (the same raceWithSignal shape this PR introduces), and thread the signal into auth()'s fetchFn calls so discovery/token-exchange requests actually cancel. This is a mechanically distinct site from the sse.ts transport's un-raced auth awaits flagged earlier — a fix there cannot touch middleware.ts, and vice versa.
Why pre-existing. middleware.ts is untouched by this diff, its JSDoc notes MCP transports "should not need this wrapper", and merging this PR strictly improves the abort story. But the wrapper remains exported and documented for exactly this composition, making it (besides sse.ts) the last supported client-package configuration where a hung auth flow still parks send() past its abort — worth tracking in the auth()-signal-threading follow-up.
…reconnect dedup, predicate extraction - raceWithSignal now takes a producer and runs its aborted fast-path BEFORE invoking it, so an abort landing ahead of any of the five auth stages (token(), 401 recovery x2, step-up x2) never starts side-effectful work; subsumes _commonHeaders' special-case gate - terminateSession uses _combinedSignal() for the DELETE fetch too - reconnect() no longer re-reports a failure _startOrAuthSse already routed to onerror; one report per failed attempt, retry kept - extract _isIntentionalAbort(requestSignal?) and use it at all seven guard sites (three hand-written variants unified) - tests: pre-aborted requestSignal never invokes token(); a failed reconnect attempt reports onerror exactly once Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DThnF41VH9fXQPjGmRySxa
|
Round addressed in ce28a32: Two findings are deferred rather than folded in, to keep this PR reviewable:
Generated by Claude Code |
ce28a32 deliberately dropped the 'Failed to reconnect SSE stream:' re-wrap that duplicated each attempt's failure report; the reconnect-failure-onerror scenario pinned that duplicate's message. Each failed attempt still reaches onerror exactly once — now as the underlying failure itself — and the scenario asserts both that and the wrapper's absence. Retry budget and GET counts unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DThnF41VH9fXQPjGmRySxa
| this._startOrAuthSse({ | ||
| resumptionToken, | ||
| replayMessageId: isJSONRPCRequest(message) ? message.id : undefined, | ||
| requestSignal: options?.requestSignal | ||
| }).catch(error => this.onerror?.(error)); | ||
| requestSignal: options?.requestSignal, | ||
| // Keep the caller's stream observers across the resume, | ||
| // matching the fresh-POST path below: without them a | ||
| // resume-via-send() dropped later resumption tokens (the | ||
| // persistence chain) and never reported the resumed | ||
| // stream's terminal end. | ||
| onresumptiontoken, | ||
| onRequestStreamEnd: options?.onRequestStreamEnd | ||
| }).catch(() => { | ||
| // `_startOrAuthSse`'s own catch already routed the | ||
| // failure to onerror (suppressed for intentional aborts) | ||
| // before rethrowing — no second emission here. Same | ||
| // abort guard as `_scheduleReconnection`'s reconnect(): | ||
| // an abort of either signal during the resume (now | ||
| // reachable mid-auth-chain too) is intentional teardown. | ||
| if (this._isIntentionalAbort(options?.requestSignal)) { | ||
| return; | ||
| } | ||
| // An outright resume failure (network, non-401/403/405 | ||
| // HTTP error) is TERMINAL for the per-request stream the | ||
| // caller is observing — this is the only channel that | ||
| // reports it (the 405 case fires inside _startOrAuthSse). | ||
| options?.onRequestStreamEnd?.(); | ||
| }); | ||
| return; | ||
| } |
There was a problem hiding this comment.
🟣 Pre-existing issue at the protocol↔transport seam (this PR rewrites this branch but does not introduce the interaction): on a legacy-era connection, Protocol's cancel path delivers cancellation by POSTing notifications/cancelled with the original request's send options forwarded verbatim — including resumptionToken (protocol.ts:1464) — and this branch short-circuits on resumptionToken before looking at the message, so the cancellation is silently discarded (the server keeps executing the cancelled request) and a rogue resume GET is opened instead. Fix either by sending the cancellation with { relatedRequestId } only in protocol.ts, or by gating this branch on the outbound message actually being the request being resumed.
Extended reasoning...
What the bug is. On a legacy-era streamable HTTP connection (any pre-2026-07-28 negotiated protocol version), cancelling a request that was resumed via resumptionToken never delivers notifications/cancelled to the server. Instead, the cancellation notification is silently swallowed by this branch of _send, and a second GET+Last-Event-ID resume stream is opened for a request the protocol layer has already rejected.
The code path. In Protocol._requestWithSchemaViaCodec (packages/core-internal/src/shared/protocol.ts), streamCloseCancels (protocol.ts:1417) requires codec.era === MODERN_WIRE_REVISION && hasPerRequestStream, so on any legacy-era connection requestAbort === undefined and cancel() takes the notification-POST branch (protocol.ts:1453-1466). That branch forwards the original request's send options verbatim: this._transport.send(cancelledNotification, { relatedRequestId, resumptionToken, onresumptiontoken }) (protocol.ts:1464) — resumptionToken/onresumptiontoken were destructured from the caller's request options at protocol.ts:1367. On the transport side, _send short-circuits on if (resumptionToken) (streamableHttp.ts:1092) before ever inspecting the message: it fire-and-forgets a GET resume via _startOrAuthSse and returns. The notifications/cancelled payload is never POSTed.
Step-by-step proof. (1) A client on a legacy-era connection resumes a long-running request: client.request(req, schema, { resumptionToken: lastEventId, onresumptiontoken }). (2) The request hits DEFAULT_REQUEST_TIMEOUT_MSEC (or the caller aborts options.signal). (3) cancel() runs; requestAbort is undefined, so the notifications/cancelled send goes out carrying the caller's resumptionToken in its options. (4) _send's resumption branch triggers: the cancellation is discarded — the server keeps executing the cancelled request, violating the spec's cancellation contract and the protocol.ts comment's promise that "every other era×transport combination keeps today's notifications/cancelled POST path unchanged" — and a rogue GET with Last-Event-ID is opened for a request whose response handler was just deleted. Since isJSONRPCRequest(notification) is false, replayMessageId is undefined, so nothing remaps the replayed stream.
Why existing code doesn't prevent it. The resumptionToken short-circuit predates this PR (v1 behavior) and assumes the message being sent is the request being resumed; the protocol cancel path blindly forwards the options object's resumptionToken onto a different message. No test covers cancel-during-resume on the legacy era. Modern-era connections are unaffected (cancel is requestAbort.abort(), the else branch).
Why it surfaces on this PR. The PR rewrites exactly this branch and newly threads onresumptiontoken/onRequestStreamEnd into the rogue GET (streamableHttp.ts:1101-1102) — so the caller's token-persistence callback and terminal-stream callback now actively fire for a stream opened by a discarded cancellation, after request() has already rejected; previously the rogue GET at least ran observer-free. Both sides of the root interaction predate the diff, though, which is why this is filed as pre-existing rather than blocking.
How to fix. Two candidate loci: (a) in protocol.ts, send the cancellation notification with { relatedRequestId } only — do not forward the original request's resumptionToken/onresumptiontoken onto a different message; or (b) in _send, only take the resume branch when the outbound message is the request being resumed (i.e. a JSON-RPC request), not a notification. Option (a) is the smaller, more targeted change and keeps the transport branch's contract intact.
… drop; doc the reconnect emission contract - a reconnect scheduled before the resumed stream delivered any ID-bearing event now falls back to options.resumptionToken at both _scheduleReconnection sites, so Last-Event-ID is not silently lost (a token-less GET starts a fresh stream and the pending response never replays); pinned by a fake-timers test - changeset + upgrade-to-v2.md now state the reconnect emission contract: one onerror per failed attempt carrying the underlying error, wrapper gone, exhaustion message-text guarantee unchanged Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DThnF41VH9fXQPjGmRySxa
|
Round addressed in 61062b0: the reconnect emission-contract change is now documented in the changeset and in upgrade-to-v2.md (the exhaustion message-text guarantee is explicitly scoped, with migration guidance for monitors that matched the removed wrapper), and the pre-first-event resumption-token loss is fixed at both The cross-package cancellation finding (legacy-era Generated by Claude Code |
| --- | ||
| '@modelcontextprotocol/client': patch | ||
| --- |
There was a problem hiding this comment.
🟡 The changeset declares patch, but this PR adds new public API to @modelcontextprotocol/client at stable 2.0.0: UnauthorizedContext is newly exported from the package barrel, and the new optional UnauthorizedContext.signal field is a runtime-populated extension point the docs teach provider authors to adopt — under semver that is a minor bump. Consider changing the front-matter to minor (or splitting the API addition into its own minor changeset) so the new extension point isn't buried in a patch entry.
Extended reasoning...
What the issue is. .changeset/abortable-auth-awaits.md declares '@modelcontextprotocol/client': patch, but the PR's final shape adds consumer-visible API surface to a package sitting at stable 2.0.0 (packages/client/package.json):
packages/client/src/index.tsnewly exports theUnauthorizedContexttype (added in review round f4e9e33). Per CLAUDE.md § Public API Exports, "Adding a symbol to a packageindex.tsmakes it public API" — andgit show HEAD~10confirms the barrel had zero matches before this PR, so this is the type's nameable public debut.packages/client/src/client/auth.tsadds the optionalUnauthorizedContext.signalfield, and both transport paths (_sendand_startOrAuthSse) now actually populate it. This is not a type-only tweak: it is a new runtime capability thatdocs/clients/machine-auth.mdanddocs/migration/upgrade-to-v2.mdboth explicitly teach provider authors to adopt ("forward it to your own fetches so the recovery work stops with it").
Step-by-step proof that this is the standard minor-bump marker. (1) A provider author writes onUnauthorized: ctx => { myFetch(url, { signal: ctx.signal }) }. (2) Against 2.0.0, this fails to typecheck — UnauthorizedContext has no signal property and the type isn't even importable from the barrel. (3) Against the version this changeset produces, it typechecks and works at runtime. (4) Code that compiles against 2.0.x-latest but not against 2.0.0 is precisely semver's definition of "new, backwards compatible functionality" — a MINOR bump. With the current changeset, 2.0.1 would ship with an API surface that differs from 2.0.0, which API-diff tooling and consumers treating patch releases as surface-identical will misclassify.
Repo precedent cuts both ways — here is the honest accounting. The package's own changelog records PR #1710 (which introduced the AuthProvider surface, including the line "New UnauthorizedContext type.") under Minor Changes. A refuting verifier correctly countered that packages/client/CHANGELOG.md's 2.0.0 Patch Changes section contains bugfix PRs with incidental new API declared as patch: #2441 shipped the new isJsonContentType() export, and #2384 shipped the new X.isInstance() static guards. Both claims verify. However, both of those patch declarations were made during the 2.0.0-beta prerelease phase (they first shipped in 2.0.0-beta.3), where the bump type had no user-visible consequence — every prerelease bump lands as another -beta.N regardless. The same is true of #1710's minor entry (2.0.0-alpha.1). This PR is the first such incidental API addition landing against a stable 2.0.0, where patch vs. minor is the difference between 2.0.1 and 2.1.0 and actually governs what consumers may assume. So the prerelease-era "bump tracks the change's primary character" precedent doesn't straightforwardly transfer, and strict semver applies with full force for the first time.
Impact. Beyond tooling misclassification, the release notes generated from a patch changeset bury the one thing provider authors must adopt for cooperative cancellation — the ctx.signal extension point — in a patch entry, where nobody scanning Minor Changes for new capabilities will find it.
How to fix. One word in the front-matter: patch → minor. Alternatively, split the additive API (UnauthorizedContext export + signal field) into its own minor changeset and keep the abort-race bugfix as patch — changesets takes the max, so the release lands as minor either way, with cleaner notes.
Why nit. Release-metadata correctness only — nothing breaks at runtime if this merges as-is, and given the (prerelease-era) patch precedent the maintainers may reasonably make the opposite call. Non-blocking either way.
| /** | ||
| * Normalize an aborted signal's `reason` to an `Error` (matching how the | ||
| * listen driver and `fetch` surface aborts) so a raced auth await rejects | ||
| * with something callers can inspect. | ||
| */ | ||
| function abortReasonError(signal: AbortSignal): Error { | ||
| const reason: unknown = signal.reason; | ||
| return reason instanceof Error ? reason : new Error(String(reason ?? 'Aborted')); | ||
| } |
There was a problem hiding this comment.
🟡 The new module-private abortReasonError helper duplicates, byte-for-byte, the abort-reason normalization the listen driver hand-writes inline at client.ts:2091 (reason instanceof Error ? reason : new Error(String(reason ?? 'Aborted'))), and the helper's own JSDoc says the two sites must agree — but the agreement is enforced only by prose, so drift would be silent. Consider hoisting the normalization into a client-internal shared helper (taking the reason rather than the signal, so both sites can use it) and calling it from both places — the same copy-drift consolidation this PR's review rounds already performed within streamableHttp.ts.
Extended reasoning...
What the finding is. This PR adds abortReasonError(signal) (packages/client/src/client/streamableHttp.ts:315-323), whose body is reason instanceof Error ? reason : new Error(String(reason ?? 'Aborted')). The listen driver in packages/client/src/client/client.ts:2091 contains the identical expression inline: settle({ cause: 'local', error: reason instanceof Error ? reason : new Error(String(reason ?? 'Aborted')) }). The helper's JSDoc explicitly declares the pairing intentional — "matching how the listen driver and fetch surface aborts" — i.e. a send() aborted in the auth chain and a listen() caller-signal abort are meant to reject with the same normalized shape. Yet nothing enforces that: the two copies are lexically unrelated (the client.ts inline never matches a grep for abortReasonError), so a future change to one would silently strand the other.\n\nConcrete verification. A grep across packages/client/src for the instanceof Error ? … : new Error(String(…)) shape finds nine sites, but only two carry the distinctive ?? 'Aborted' fallback: the new helper (streamableHttp.ts:322) and the listen driver (client.ts:2091). The other seven (client.ts:1218/1267/1317/1825/2120/2593, streamableHttp.ts:828) are the plain String(x) idiom with no fallback and no documented coupling. So this is not merely "the idiom appears everywhere" — these two sites are a semantically coupled pair (same fallback string, same purpose, JSDoc-declared agreement) sitting in different files.\n\nStep-by-step drift scenario. (1) A future PR improves abortReasonError to preserve non-Error reasons, e.g. new Error('Aborted', { cause: reason }), or stamps name = 'AbortError' so callers can branch on it. (2) Every raced auth await in the transport now rejects with the new shape; the author greps for abortReasonError, finds only streamableHttp.ts call sites, and ships. (3) listen()'s local-abort settle() at client.ts:2091 keeps the old shape — the same user-initiated abort now surfaces two differently-shaped errors depending on which await the abort happened to land in. (4) No type or test catches it: the copies share no symbol, and no test asserts cross-site shape agreement.\n\nAddressing the refutation. One verifier argued the extraction serves only one extra call site and that the general normalization idiom is pervasive and inline throughout the codebase. The pervasiveness point is fair for the plain String(x) form, but the ?? 'Aborted' variant exists at exactly these two sites and the helper's own JSDoc claims they agree — that documented coupling is what distinguishes this pair from the surrounding idiom. The refutation is also right that the practical cost of drift is bounded (both sites still reject with an Error), and that the shared helper must not create backwards coupling — which is why the fix should be a reason-taking helper in a client-internal util module (or exported module-level from a neutral file, not added to index.ts), never an import of the transport module from client.ts. These caveats shape the fix; they don't erase the finding. On balance — three verifiers confirmed, one refuted — this is a plausible-but-optional cleanup, which is exactly what nit severity encodes.\n\nWhy the PR's own history supports acting. This PR's review rounds performed precisely this class of consolidation within streamableHttp.ts: isAborted(), _combinedSignal(), and _isIntentionalAbort() were each extracted because hand-copied predicates had drifted or nearly drifted across sites. This pair spans two files, where drift is strictly harder to notice than the intra-file cases already fixed.\n\nHow to fix (mechanical, no behavior change). Add a small shared helper in a client-internal module, taking the raw reason (the client.ts site already has the reason in hand; the transport wraps it for signals):\n\nts\nexport function normalizeAbortReason(reason: unknown): Error {\n return reason instanceof Error ? reason : new Error(String(reason ?? 'Aborted'));\n}\n\n\nThen abortReasonError becomes normalizeAbortReason(signal.reason) and client.ts:2091 becomes settle({ cause: 'local', error: normalizeAbortReason(reason) }). Keep it out of the package index.ts (internal helper, per CLAUDE.md export rules). No runtime behavior changes today; the existing test suites pass unchanged.\n\nSeverity. Quality/consistency finding only — no incorrect behavior, no crash, nothing blocks merging: nit.
A custom reconnectionScheduler that throws while scheduling attempt N+1 ends the retry chain — terminal for an observed per-request stream, exactly like budget exhaustion, but the catch fired only onerror so a listen() observer waited forever. Fire the stream-end callback too, matching the sibling terminal paths. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DThnF41VH9fXQPjGmRySxa
|
Latest round: the reconnect re-arm gap is fixed in 53c393c — a Two findings are dispositioned rather than changed:
Generated by Claude Code |
| // Honour BOTH the transport-wide abort and the per-request abort | ||
| // (a listen subscription closed during the backoff delay): do not | ||
| // resurrect a stream the caller already tore down. | ||
| if (this._abortController?.signal.aborted || options.requestSignal?.aborted) return; | ||
| this._startOrAuthSse(options).catch(error => { | ||
| if (this._abortController?.signal.aborted || options.requestSignal?.aborted) return; | ||
| this.onerror?.(new Error(`Failed to reconnect SSE stream: ${error instanceof Error ? error.message : String(error)}`)); | ||
| if (this._isIntentionalAbort(options.requestSignal)) return; | ||
| this._startOrAuthSse(options).catch(() => { | ||
| if (this._isIntentionalAbort(options.requestSignal)) return; | ||
| // `_startOrAuthSse`'s own catch already routed the failure to | ||
| // onerror before rethrowing — re-emitting here would report | ||
| // every failed attempt twice. Just schedule the next try. | ||
| try { | ||
| this._scheduleReconnection(options, attemptCount + 1); | ||
| } catch (scheduleError) { |
There was a problem hiding this comment.
🟣 Pre-existing (this PR rewrites the enclosing reconnect() closure but not the slot mechanics): _cancelReconnection is a single instance field shared by all concurrent per-stream reconnect chains (the standalone GET stream plus every primed POST-SSE stream), so every _scheduleReconnection call overwrites the previous chain's cancel handle and close() can cancel at most one pending attempt — a superseded chain's non-unref'd setTimeout stays live up to maxReconnectionDelay (30s default) after close(), and with a custom reconnectionScheduler the JSDoc promise that the cancel fn 'will be called on transport.close()' is violated for all but the last-scheduled chain. Fix: track cancel handles in a Set (add on schedule, remove when the corresponding reconnect() runs) and cancel all members in close().
Extended reasoning...
What the bug is. Reconnection chains in StreamableHTTPClientTransport are per-stream: the standalone GET stream is always isReconnectable, and every per-request POST-SSE stream that received a priming event (canResume = isReconnectable || hasPrimingEvent) runs its own chain through _scheduleReconnection. But the cancel handle lives in one instance field, this._cancelReconnection. Both scheduling branches overwrite it unconditionally — this._cancelReconnection = typeof cancel === 'function' ? cancel : undefined for a custom reconnectionScheduler, this._cancelReconnection = () => clearTimeout(handle) for the default setTimeout — and reconnect()'s first statement (streamableHttp.ts:815) unconditionally clears the slot even when the handle currently stored belongs to a different chain whose timer is still pending. close() then calls the single this._cancelReconnection?.() exactly once.
The code path that triggers it. Two chains pending at once is a routine state — one network drop severs the standalone GET stream and a primed POST-SSE stream simultaneously, and each schedules its own reconnect. Whichever schedules second wins the slot; the first chain's cancel handle is silently lost. There is also a worse mid-flight variant: when chain A's reconnect() fires while chain B's timer is still pending, A's first statement sets the slot to undefined, so a subsequent close() cancels nothing.
Step-by-step proof. (1) The standalone GET stream drops → chain A calls _scheduleReconnection, setTimeout fires later, slot = cancelA. (2) Before A's timer fires, a primed per-request POST-SSE stream drops → chain B schedules, slot = cancelB; cancelA is unreachable. (3) The caller calls transport.close() → close() runs this._cancelReconnection?.(), clearing only B's timer, then aborts the transport signal. (4) A's timer — up to maxReconnectionDelay (30 s with defaults), and never unref()'d — stays armed. When it fires, reconnect()'s _isIntentionalAbort guard prevents any network traffic, but the guard runs only when the timer fires: it cannot release the timer at close time. A Node CLI or test process that closed its transport keeps the event loop pinned for up to 30 s before it can exit.
Why existing code doesn't prevent it. The _isIntentionalAbort check inside reconnect() is the only safeguard, and it is post-fire — it stops stream resurrection, not the timer itself. No test creates two concurrent reconnect chains, so the interleaving is uncovered. With a custom reconnectionScheduler the failure is a documented-contract violation rather than just an event-loop pin: the ReconnectionScheduler JSDoc (~line 137) promises the returned cancel function 'will be called on transport.close() to abort the pending reconnection' — exactly the mobile/serverless platform-background-task use case it names — yet with two concurrent chains only the last-scheduled chain's platform task is cancelled. The other stays scheduled on the platform scheduler after close() and wakes into a dead transport.
Why this is pre-existing. git show cc4b416 (main, pre-PR) has the identical single-slot structure; this PR rewrote only the error-reporting body of the reconnect() closure (the double-onerror dedup) and left the slot/cancel mechanics unchanged. It is flagged here because the PR touches this exact closure, making it the natural place to notice — but it should not block this PR.
Impact. Bounded: no network resurrection, no protocol misbehavior, no data loss. The consequences are (a) an event-loop pin of up to maxReconnectionDelay after close() with the default setTimeout path, (b) close() cancelling nothing at all in the mid-flight variant, and (c) a leaked platform background task with a custom scheduler, violating the scheduler's documented cancel contract.
How to fix. Replace the single slot with a Set<() => void>: add each chain's cancel handle when it schedules, have each reconnect() remove its own handle (captured in the closure, instead of nulling a shared field), and have close() iterate the set and cancel every member before aborting. That preserves the one-handle-per-chain lifecycle and makes the ReconnectionScheduler cancel contract hold for every pending chain.
|
Confirmed real and pre-existing: Generated by Claude Code |
There was a problem hiding this comment.
I re-reviewed the final revision (through 53c393c) and found no new bugs — all findings from the earlier review rounds are addressed or explicitly dispositioned in the thread. Given the scope, a human maintainer look is still worthwhile before merge.
What was reviewed this run:
raceWithSignalsemantics: aborted fast-path before the producer runs, listener removal on settlement, identity-preserved rejections (auth-seam stamp survives), and no-op handlers on the losing promise.- Abort classification (unstamped abort vs
markAuthSeamEscape) across_commonHeaders, bothonUnauthorizedsites, and both step-up awaits — POST and GET paths are symmetric. - The reconnect emission contract, the pre-first-event resumption-token fallback, and the terminal
onRequestStreamEndpaths now agree with the updated e2e assertion and are pinned by unit tests.
One item deliberately left open for a maintainer: the changeset patch vs minor call for the new UnauthorizedContext export + signal field (see the fixed-group/middleware peer-dep note in the thread).
Extended reasoning...
Overview
This PR makes the streamable HTTP transport's auth awaits (token(), onUnauthorized 401 recovery, insufficient-scope step-up) abortable by the combined request/transport signal, adds the optional UnauthorizedContext.signal extension point (newly exported from the client barrel), and — through four review rounds — also changed the reconnect onerror emission contract, fixed a pre-first-event resumption-token loss, preserved stream observers across resume-via-send(), and fixed a terminal-notification gap when a custom reconnection scheduler throws. Files touched: streamableHttp.ts (substantial), auth.ts, index.ts, docs, changeset, unit and e2e tests.
Security risks
The changes sit in the OAuth/auth-recovery seam of the client transport. No new credential handling or validation logic is introduced — the racing only changes when the transport stops waiting, not what it trusts — and the abort-vs-auth-failure classification is guarded consistently at every raced site. The raced-out auth work continuing in the background is a documented limitation, not an exposure. Still, auth-path code warrants human eyes by default.
Level of scrutiny
High. This is production-critical transport lifecycle code with subtle async semantics (signal combination, listener cleanup, unhandledRejection suppression, identity-preserved rejections for the auth-seam stamp), a user-visible onerror contract change, and new public API on a stable 2.0.0 package. The earlier review rounds surfaced real issues (a CI-breaking e2e assertion, a token-loss bug, two terminal-notification gaps), all of which were fixed with pinning tests — but the density of findings itself argues against shadow-approval.
Other factors
This run of the bug hunting system found no new issues on the final revision, and the prior rounds' findings are all resolved or dispositioned in the thread. The one open item is a semver judgment call (changeset patch vs minor) with a repo-specific fixed-group/peer-dependency trap that the thread documents — that is precisely a maintainer decision, not something an automated approval should make. Test coverage is strong: failing-first unit tests for every new abort path plus behavior-preservation controls, and the gated e2e assertion was updated to the new contract.
1 participant
Requested by Felix Weinberger · Slack thread
Fixes #2643.
Root cause
StreamableHTTPClientTransportthreadsTransportSendOptions.requestSignalthrough its fetches and the SSE resume chain, but the auth awaits on the send path never see it:_commonHeaders()awaitsauthProvider.token()before the per-request signal is even combined — a hung token refresh parkssend()forever, and abortingrequestSignaldoes nothing (repro in [v2] StreamableHTTPClientTransport: auth awaits in _send are un-abortable — requestSignal cannot cancel token()/onUnauthorized/metadata-discovery parks #2643 shows the send still pending 2s after the abort).authProvider.onUnauthorized({ response, serverUrl, fetchFn })with no signal, on both the POST (_send) and GET (_startOrAuthSse) paths._stepUpAuthorize, which runs metadata discovery and token exchange) is awaited un-raced on both paths.Since
Client.listen()'s teardown relies onrequestSignalreaching the transport, these parks are also one of the ways the #2641 listen() wedge stays stuck.Fix
A
raceWithSignal(promise, signal)helper (next to the existinganySignal) races each auth await against the combined request/transport signal, which is now computed before header acquisition on both the POST and GET paths:_commonHeaders(signal?)racestoken();terminateSessionpasses the transport signal too.onUnauthorizedawaits are raced, and the combined signal is offered to the provider via a new optionalUnauthorizedContext.signalfield (additive — existing implementations are unaffected) so cooperative providers can cancel their own recovery fetches._stepUpAuthorizeawaits are raced.Abort semantics match the existing discipline: an abort rejects with the signal's reason, is not stamped as an auth-seam escape (it is an intentional teardown, so the negotiation-probe classifier and the send-catch treat it as a plain abort), and the
_sendcatch keeps suppressingonerrorforrequestSignalaborts. The losing promise gets a no-op rejection handler so it cannot escape as anunhandledRejection; the race listener is removed on settlement so a long-lived transport signal does not accumulate closures per request.Known limitation (called out in #2643): the raced-out auth work itself keeps running — the
AuthProvidercontract has no mandatory cancellation channel.ctx.signalgives providers the hook; threading the signal all the way through the SDK's internalauth()metadata-discovery fetches is a natural follow-up but touches the OAuth plumbing more broadly, so it is left out of this focused fix.Tests
Written failing-first against
main(cc4b416); newdescribeblock inpackages/client/test/client/streamableHttp.test.ts:token()+requestSignalabort → send rejects with the abort reason, noonerror, no fetch issued (fails onmain: send still pending after 500ms).token()+transport.close()→ send rejects (fails onmain).onUnauthorized+requestSignalabort → send rejects, noonerror, and the provider receivedctx.signalin the aborted state (fails onmain).token()rejection without an abort → still surfaces as the auth failure (behavior-preservation control; passes before and after).pnpm --filter @modelcontextprotocol/client test: 33 files, 801 tests passed (805 including the new block on the final run). Lint (ESLint + Prettier) and typecheck clean. Changeset included (patch,@modelcontextprotocol/client).Generated by Claude Code