fix(client): retry the SSE connection once after onUnauthorized - #2905
Conversation
SSEClientTransport reconnected after every 401 for as long as onUnauthorized() kept resolving, so start() never settled against a server that always answers 401. It now retries once, as documented and as the POST path and the Streamable HTTP transport do, and then rejects with SdkHttpError. The guard resets when the stream opens, so a 401 on a later reconnect still gets one refresh.
🦋 Changeset detectedLatest commit: 985573c 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: |
|
The failed Generated by Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline findings, I also checked whether the statusText read at sse.ts:270 could be stale or empty on the post-refresh 401 — it is not, since the fetch wrapper at sse.ts:229 re-stashes _last401Response on every 401 response before the EventSource onerror fires.
Extended reasoning...
The change adds a single private boolean to the legacy SSE client transport's 401 handling in packages/client/src/client/sse.ts so that a second 401 after onUnauthorized rejects with SdkHttpError rather than looping, plus rewritten auth-retry tests and a patch changeset. It touches the client auth flow, and the inline findings cover the guard's interaction with non-401 errors, close() during a pending refresh, missing onclose on the terminal path, and an unbounded loop when the stream drops before the endpoint event, so a human look is warranted.
Findings marked 🟡 are optional suggestions and need no follow-up push.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟣
packages/client/src/client/sse.ts— pre-existing: a caller that callsclose()whileonUnauthorized()is still pending gets a brand-new SSE connection opened afterwards, delivering messages to a transport it already closed. The resolve handler at sse.ts:251 calls_startOrAuth()without checking that the transport was closed, andclose()at sse.ts:381-385 neither clears_eventSourcenor rejects the pending start. Fix: have the deferred.thenhandlers (both the sse.ts:251 retry and the sse.ts:255 rejection path) check a closed flag or abort signal before touchingthis._eventSource/_abortControlleror starting I/O, and settle the pendingstart()promise instead of reconnecting. [also at: packages/client/src/client/sse.ts:256 - nit: REVIEW.md (Async / Lifecycle) requires deferred callbacks to check closed/aborted state before mutatingthis._*: the newonUnauthorized().then(..., error => { this._connectAuthRetried = false; ... })rejection handler (and the_connectAuthRetriedwrites in the reconnect EventSource's…]Why this was flagged
Trigger: start() gets a 401, sse.ts:249 invokes onUnauthorized (an OAuth token refresh that can take seconds), and during that wait the user calls close() (sse.ts:381), which aborts
_abortControllerand closes the current EventSource. When onUnauthorized later resolves, sse.ts:251 runs_startOrAuth()unconditionally: sse.ts:218 creates a new EventSource with its own fetch (not bound to the aborted_abortController), sse.ts:240 replaces_abortControllerwith a fresh one, and the stream connects and starts deliveringonmessageevents after onclose already fired. The pending start() then resolves after close(). This is pre-existing (base has the same sse.ts:251 line); the diff edits this callback pair (sse.ts:247, sse.ts:256) but adds no closed-state check.Verification: pre-existing — triggered when a caller invokes
close()while the 401-triggeredonUnauthorized()promise at packages/client/src/client/sse.ts:249 is still pending. Mechanism verified: the resolve handler at sse.ts:251 is() => this._startOrAuth().then(resolve, reject)with no closed/aborted check;close()at sse.ts:381-385 only doesthis._abortController?.abort(); this._eventSource?.close(); this.onclose?.()and sets no closed flag (grep forclosedfinds nothing in the file).
| // stamp: covers the SDK's OAuth flow and custom | ||
| // callbacks alike. | ||
| (error: unknown) => { | ||
| this._connectAuthRetried = false; | ||
| markAuthSeamEscape(error); | ||
| this.onerror?.(error as Error); | ||
| reject(error); | ||
| } | ||
| ); | ||
| return; | ||
| } | ||
| const error = markAuthSeamEscape(new UnauthorizedError()); | ||
| const retried = this._connectAuthRetried; | ||
| this._connectAuthRetried = false; | ||
| const error = markAuthSeamEscape( | ||
| retried | ||
| ? new SdkHttpError(SdkErrorCode.ClientHttpAuthentication, 'Server returned 401 after re-authentication', { | ||
| status: 401, | ||
| statusText: this._last401Response?.statusText ?? '' | ||
| }) | ||
| : new UnauthorizedError() | ||
| ); | ||
| this._last401Response = undefined; | ||
| reject(error); | ||
| this.onerror?.(error); | ||
| return; |
There was a problem hiding this comment.
🔴 A connected client whose post-refresh reconnect hits a network outage longer than its new token's lifetime now loses the stream with a terminal auth error instead of refreshing once more. The _connectAuthRetried guard set at sse.ts:247 is only cleared by onopen or by a 401; a non-401 error at sse.ts:280 leaves it set while EventSource keeps auto-reconnecting on network errors, so the next 401 is blocked at sse.ts:244 and rejected at sse.ts:268 as 'Server returned 401 after re-authentication'. Fix: reset the guard whenever the post-refresh attempt fails for a non-401 reason (the sse.ts:280 path), so the guard means "the immediately preceding attempt was the retry" rather than "no open since the refresh", while an always-401 server still gets exactly one refresh per attempt.
Why this was flagged
Trigger: the stream has opened (flag cleared at sse.ts:287), later drops and EventSource auto-reconnects; the server answers 401 because the token expired. sse.ts:244 passes, sse.ts:247 sets _connectAuthRetried = true, onUnauthorized resolves with a new token and sse.ts:251 creates a new EventSource. That fetch fails with a network error (server restart or outage). eventsource ^3 (pnpm-workspace.yaml:39) dispatches an error event with no code and schedules a reconnect on network failures, so sse.ts:280 builds an SseError and nothing clears the flag. The EventSource keeps retrying every reconnect interval; once the outage outlasts the refreshed token's lifetime the server answers 401 again. sse.ts:244 is now false because of !this._connectAuthRetried, so sse.ts:264-276 rejects with SdkHttpError 'Server returned 401 after re-authentication' and calls onerror; eventsource closes on a non-200 status, and no onclose fires. On the base branch sse.ts:244 has no flag, so onUnauthorized runs again and the reconnect succeeds.
Verification: normal — triggered when, after an opened stream drops and the auto-reconnect gets a 401 that is refreshed, the refreshed EventSource's fetch fails at the network level and the token is no longer valid by the time the library's own reconnect loop reaches the server again (server restart/deploy with key rotation, or an outage longer than a short-lived token).
| const retried = this._connectAuthRetried; | ||
| this._connectAuthRetried = false; | ||
| const error = markAuthSeamEscape( | ||
| retried | ||
| ? new SdkHttpError(SdkErrorCode.ClientHttpAuthentication, 'Server returned 401 after re-authentication', { | ||
| status: 401, | ||
| statusText: this._last401Response?.statusText ?? '' | ||
| }) | ||
| : new UnauthorizedError() |
There was a problem hiding this comment.
🟡 nit (optional): readers of the authProvider JSDoc are told a second 401 throws UnauthorizedError, but after this change the connect path rejects with SdkHttpError instead. The doc at packages/client/src/client/sse.ts:75-77 says "If the retry also gets 401 ... UnauthorizedError is thrown", while sse.ts:268 builds SdkHttpError(ClientHttpAuthentication) on that exact path. Fix: update the JSDoc on SSEClientTransportOptions.authProvider (and the same sentence on AuthProvider.onUnauthorized in auth.ts:89-90) to say the post-refresh 401 rejects with SdkHttpError (ClientHttpAuthentication), keeping UnauthorizedError only for the no-onUnauthorized case. The POST path (sse.ts:435) already contradicted this sentence before the change; this diff extends the contradiction to start(). [also at: packages/client/src/client/sse.ts:77 - nit: Readers of the authProvider option docs are told a second 401 throws UnauthorizedError, but after this change the SSE connect retry rejects with SdkHttpError instead.; packages/client/src/client/sse.ts:268 - nit: Readers of the v2 migration guide are not told that SSEClientTransport.start() now rejects a post-refresh 401 with SdkHttpError instead of v1's UnauthorizedError.; +1 more]
Why this was flagged
A caller with an AuthProvider that has onUnauthorized connects via SSEClientTransport.start() against a server that answers 401 both before and after the refresh. packages/client/src/client/sse.ts:264-273 now rejects with SdkHttpError(SdkErrorCode.ClientHttpAuthentication, 'Server returned 401 after re-authentication'); on the base branch this path looped by calling onUnauthorized again and never rejected. The public JSDoc in the same file, sse.ts:73-77, still promises "If the retry also gets 401, or onUnauthorized is not provided, UnauthorizedError is thrown", and auth.ts:88-90 repeats it for AuthProvider.onUnauthorized.
Verification: nit. Triggering condition: a reader relies on the public authProvider JSDoc for the error type after the single post-refresh retry over the SSE connect path. Mechanism verified: /home/claude/typescript-sdk/packages/client/src/client/sse.ts:75-77 (unchanged by the diff) still reads "then the request is retried once.
|
|
||
| private _last401Response?: Response; | ||
| // True between a 401-triggered reconnect and the next successful open. | ||
| private _connectAuthRetried = false; |
There was a problem hiding this comment.
🟡 nit (optional): CLAUDE.md asks for 2-space indentation: every line this PR adds to packages/client/src/client/sse.ts (e.g. the new _connectAuthRetried field at line 179 and the 401 branch at lines 244-275) and to test/client/sse.test.ts is indented with 4 spaces. Fix: the repo's committed .prettierrc.json sets tabWidth: 4 and pnpm lint:all enforces it, so the practical fix is to correct the stale CLAUDE.md line to say 4-space indentation rather than reindent the code; if 2 spaces is really intended, the prettier config must change first so lint and CLAUDE.md agree.
Why this was flagged
Nothing fails at runtime. The instruction guards consistent formatting; here the written rule (2 spaces) and the enforced formatter (.prettierrc.json: tabWidth: 4, useTabs: false) disagree, and the whole existing file already uses 4 spaces, so the added lines match lint but not the CLAUDE.md text. Reindenting the new lines to 2 spaces would fail pnpm lint:all; the only consequence of leaving it is that CLAUDE.md keeps mis-describing the repo's style. Low weight - flagged only because the instruction is written as it is.
Verification: Base CLAUDE.md (git show aca8ba3:CLAUDE.md, "Code Style Guidelines") reads verbatim "- Formatting: 2-space indentation, semicolons required, single quotes preferred"; the diff adds private _connectAuthRetried = false; at packages/client/src/client/sse.ts:179 with 4 leading spaces, and every other added line (e.g. the retried ? new SdkHttpError(...) branch at lines 264-274 and this._connectAuthRetried = false; in onopen at line 287) is indented in multiples of 4, so the added code does not use 2-space indentation as the instruction is written.
| const retried = this._connectAuthRetried; | ||
| this._connectAuthRetried = false; | ||
| const error = markAuthSeamEscape( | ||
| retried | ||
| ? new SdkHttpError(SdkErrorCode.ClientHttpAuthentication, 'Server returned 401 after re-authentication', { | ||
| status: 401, | ||
| statusText: this._last401Response?.statusText ?? '' | ||
| }) | ||
| : new UnauthorizedError() | ||
| ); | ||
| this._last401Response = undefined; | ||
| reject(error); | ||
| this.onerror?.(error); | ||
| return; |
There was a problem hiding this comment.
🟣 pre-existing, not blocking: pre-existing, widened here: after the stream had opened, a post-refresh 401 on an automatic reconnect leaves the transport dead but never fires onclose, so Client requests hang until timeout. The terminal arm at packages/client/src/client/sse.ts:264-277 only calls reject (a no-op once start() resolved) and onerror; the EventSource is closed and nothing reconnects. Fix: when the terminal 401 arrives after the stream had opened (e.g. this._endpoint is set), also run void this.close() as the endpoint handler does at sse.ts:302, so onclose reaches Protocol and pending requests settle; the same applies to the UnauthorizedError arm. On base this population looped through onUnauthorized() instead of going silent.
A small fix can ride a push you are already making; otherwise a short reply is enough.
Why this was flagged
Trigger: an SSE stream that opened (start() resolved at packages/client/src/client/sse.ts:306) drops; the eventsource auto-reconnect gets 401, the transport refreshes and reconnects (sse.ts:244-251), and the new EventSource is answered 401 again — the always-401-after-refresh case this PR targets, now on a live connection rather than during start(). The retried arm at sse.ts:264-277 builds SdkHttpError, calls reject (the start() promise already resolved, so this does nothing) and this.onerror, and returns. The eventsource library fails the connection on a non-200 status, so no further reconnect happens and no other code path closes the transport. Protocol only settles response handlers and marks the connection closed from transport.onclose (packages/core-internal/src/shared/protocol.ts:797-803, 836); transport.onerror just forwards to the user (protocol.ts:806-809, 867-869).
Verification: pre-existing — triggered when an SSE stream that had opened (start() resolved at packages/client/src/client/sse.ts:306) drops, the eventsource auto-reconnect is answered 401, the transport refreshes and reconnects (sse.ts:244-251), and the fresh EventSource is answered 401 again.
| this._eventSource.onopen = () => { | ||
| // The connection is open, but we need to wait for the endpoint to be received. | ||
| this._connectAuthRetried = false; |
There was a problem hiding this comment.
🟣 pre-existing, not blocking: Clients connecting to a server that answers 200 and drops the stream before sending endpoint still get an unbounded refresh loop: start() never settles and onUnauthorized() runs on every cycle, the exact symptom this PR claims to fix. The guard is cleared in onopen at packages/client/src/client/sse.ts:287, which fires on the 200 response before the connect has succeeded. The eventsource auto-reconnect then gets 401 at sse.ts:244 with the flag already false and refreshes again, forever. Fix: bound the connect attempt as a whole, e.g. clear _connectAuthRetried only when start() resolves (the endpoint handler at sse.ts:306) or count refreshes per start() call, so a stream that never delivers endpoint cannot re-arm the retry.
A small fix can ride a push you are already making; otherwise a short reply is enough.
Why this was flagged
Trigger: during start(), the first GET returns 401, onUnauthorized resolves, the retry GET returns 200 text/event-stream but the server (or a proxy/load balancer whose backends disagree on the rotated token) closes the response before the endpoint event; eventsource auto-reconnects and the next GET returns 401. Entry point is SSEClientTransport.start() via Client.connect. At packages/client/src/client/sse.ts:285-288 onopen sets _connectAuthRetried = false on the 200, before endpoint arrives and before resolve() at sse.ts:306. The reconnect's 401 reaches sse.ts:244 with the flag false, so onUnauthorized is called again, the flag is set, another EventSource is opened, and the cycle repeats. The caller's start() promise never settles and the token endpoint is hit once per cycle; on base the loop is identical, so the PR does not fix #2894 for this population, while the changeset (.changeset/sse-connect-retry-once.md:5) promises retry-once. The dismissal accepted the author's intent (reset on open) instead of checking that open is not connect success for this transport.
Verification: pre-existing — the base branch already loops the same way by the same route (it had no guard at all), and this PR's guard does not reach this path. Triggering condition: during start(), a server (external input) answers 401, then 200 text/event-stream but closes the body before sending the endpoint event, then 401 on the automatic reconnect, and so on.
claude
Bot
added
the
v2
2 participants
Requested by Felix Weinberger · Slack thread
Motivation and Context
Before: against a server that keeps answering 401 while
onUnauthorized()resolves,SSEClientTransport.start()reconnects without limit and never settles.After: the first 401 refreshes and reconnects once; a second 401 before the stream opens rejects with
SdkHttpError(ClientHttpAuthentication, "Server returned 401 after re-authentication"); the guard resets once the stream opens, so a later reconnect gets one refresh again. This matches the POST path and the Streamable HTTP transport, and adds no new option.The two existing tests from #1710 that expected a second refresh are updated to the retry-once behaviour:
SSE connect 401 retry does not poison future 401s — onUnauthorized called on each attempt(now… — a 401 on a later reconnect refreshes again) andretry failure during SSE connect fires onerror exactly once.How: one private boolean in
packages/client/src/client/sse.ts, set when a 401 triggers the reconnect and cleared on open.How Has This Been Tested?
packages/client/test/client/sse.test.ts: new test for the always-401 server (rejects withSdkHttpError,onUnauthorizedcalled once, two GETs,onerroronce); the two tests above updated; a new test for a failingonUnauthorizedduring connect (onerrorexactly once).main'ssse.ts, 39 pass with the change.pnpm --filter @modelcontextprotocol/client test(40 files, 939 passed),pnpm typecheck:all,pnpm lint:allall pass.@modelcontextprotocol/clientpatch.Breaking Changes
None.
Types of changes
Checklist
Fixes #2894
🤖 Generated with Claude Code
https://claude.ai/code/session_012VRbFCp41otcScXE1YY3es
Generated by Claude Code