Skip to content

fix(client): make streamable HTTP auth awaits abortable by requestSignal #2644

New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Open
claude wants to merge 8 commits into
base: main
Choose a base branch
from
Open

fix(client): make streamable HTTP auth awaits abortable by requestSignal #2644

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
c37f361
fix(client): make streamable HTTP auth awaits abortable by requestSignal
claude Aug 11, 2026
bdacc42
fix(client): address automated review on abortable auth awaits
claude Aug 11, 2026
2d4fbe7
fix(client): finish the no-spurious-onerror discipline and harden rac…
claude Aug 11, 2026
f4e9e33
fix(client): review round three — export UnauthorizedContext, dedupe …
claude Aug 11, 2026
ce28a32
fix(client): review round four — thunked race, full signal adoption, …
claude Aug 11, 2026
123e3e9
test(e2e): pin one onerror per failed reconnect attempt
claude Aug 11, 2026
61062b0
fix(client): keep the prior resumption token across a pre-first-event…
claude Aug 11, 2026
53c393c
fix(client): notify onRequestStreamEnd when re-arming a reconnect throws
claude Aug 11, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/abortable-auth-awaits.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@modelcontextprotocol/client': patch
---
Comment on lines +1 to +3

@claude claude Bot Aug 11, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

🟡 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):

  1. packages/client/src/index.ts newly exports the UnauthorizedContext type (added in review round f4e9e33). Per CLAUDE.md § Public API Exports, "Adding a symbol to a package index.ts makes it public API" — and git show HEAD~10 confirms the barrel had zero matches before this PR, so this is the type's nameable public debut.
  2. packages/client/src/client/auth.ts adds the optional UnauthorizedContext.signal field, and both transport paths (_send and _startOrAuthSse) now actually populate it. This is not a type-only tweak: it is a new runtime capability that docs/clients/machine-auth.md and docs/migration/upgrade-to-v2.md both 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.


Make the streamable HTTP transport's auth awaits abortable. `AuthProvider.token()`, `onUnauthorized()` 401 recovery, and insufficient-scope step-up authorization were awaited with no way for `TransportSendOptions.requestSignal` (or the transport's own lifetime signal) to reach them, so a hung token refresh or recovery flow parked `send()` forever past its abort. These awaits are now raced against the combined request/transport signal, and the signal is offered to `onUnauthorized` via the new optional `UnauthorizedContext.signal` field so cooperative providers can cancel their own recovery work. An abort during the auth chain rejects the send with the abort reason (unstamped, treated as an intentional teardown, no spurious `onerror`). Also fixes resume-via-`send()`: the resumed GET now preserves the caller's `onresumptiontoken`/`onRequestStreamEnd` observers, so resumed streams keep the token-persistence chain, report their terminal end, and an outright resume failure no longer dead-ends silently. Reconnect-attempt failures now reach `onerror` exactly once, as the underlying error — the `Failed to reconnect SSE stream: …` wrapper that duplicated each per-attempt report is gone (the `Maximum reconnection attempts (N) exceeded.` exhaustion message is unchanged). A reconnect scheduled before the resumed stream delivered any ID-bearing event now falls back to the stream's prior resumption token instead of silently dropping `Last-Event-ID`.
2 changes: 1 addition & 1 deletion docs/clients/machine-auth.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ const authProvider: AuthProvider = { token: async () => getStoredToken() };
const transport = new StreamableHTTPClientTransport(new URL('https://api.example.com/mcp'), { authProvider });
```

The transport calls `token()` before every request and sets the `Authorization` header from whatever it returns. Without `onUnauthorized`, a 401 throws `UnauthorizedError`. Add `onUnauthorized(ctx)` to refresh the credential and the transport retries the request once.
The transport calls `token()` before every request and sets the `Authorization` header from whatever it returns. Without `onUnauthorized`, a 401 throws `UnauthorizedError`. Add `onUnauthorized(ctx)` to refresh the credential and the transport retries the request once. `ctx.signal` (when present) aborts once the caller or transport gives up on the request — forward it to your own fetches so the recovery work stops with it.

## Sign with a private key instead of a secret

Expand Down
10 changes: 8 additions & 2 deletions docs/migration/upgrade-to-v2.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1091,7 +1091,8 @@ The transport `authProvider` option is widened to `AuthProvider | OAuthClientPro
**`AuthProvider`** is a new minimal interface — `{ token(): Promise<string | undefined>;
onUnauthorized?(ctx): Promise<void> }` — for static-token / non-OAuth bearer auth.
Transports call `token()` before every request and `onUnauthorized()` on 401 (then retry
once). Existing `OAuthClientProvider` implementations need no changes — transports adapt
once); `ctx.signal` (when present) aborts once the caller or transport gives up — forward
it to your own fetches. Existing `OAuthClientProvider` implementations need no changes — transports adapt
them internally via the new `adaptOAuthProvider()` export. Also exported:
`isOAuthClientProvider()` (type guard) and `handleOAuthUnauthorized()` (the standard
OAuth `onUnauthorized` behavior, for composing your own adapter).
Expand Down Expand Up @@ -1511,7 +1512,12 @@ rewrite required unless noted.
standalone GET-stream reconnection behavior and its exhaustion signal carry over from
v1: when retries run out, the transport emits `onerror` with a plain `Error` whose
message is `Maximum reconnection attempts (N) exceeded.` — there is no typed error
class for this condition, so monitors that match the message text keep working.
class for this condition, so monitors that match the message text keep working. The
message-text guarantee is scoped to that exhaustion message: each failed attempt
before exhaustion now reaches `onerror` exactly once, as the underlying error itself —
the `Failed to reconnect SSE stream: …` wrapper that previously accompanied every
per-attempt report is gone, so monitors matching that wrapper text should match the
underlying error (or the exhaustion message) instead.
- **Also unchanged: elicitation response validation.** `elicitInput`'s local validation
of elicitation responses against `requestedSchema`, the resulting `-32602` error
message wording (`Elicitation response content does not match requested schema: …`),
Expand Down
8 changes: 8 additions & 0 deletions packages/client/src/client/auth.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,14 @@ export interface UnauthorizedContext {
serverUrl: URL;
/** Fetch function configured with the transport's `requestInit`, for making auth requests. */
fetchFn: FetchLike;
/**
* 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;
Comment on lines +58 to +65

@claude claude Bot Aug 11, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

🟣 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() awaits this._authProvider?.token() with no signal and no race.
  • sse.ts:375-386 — _send's 401 recovery awaits this._authProvider.onUnauthorized({ response, serverUrl, fetchFn }) un-raced, and without the new ctx.signal field.
  • sse.ts:216-223 — the _startOrAuth onerror handler calls onUnauthorized the 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.

Comment thread
claude[bot] marked this conversation as resolved.
}

/**
Expand Down
Loading
Loading