Skip to content

fix(client): retry the SSE connection once after onUnauthorized #2905

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

Merged
felixweinberger merged 2 commits into from
Sep 30, 2026
+92 −14
Merged

fix(client): retry the SSE connection once after onUnauthorized #2905

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e5f42d6
fix(client): retry the SSE connection once after onUnauthorized
felixweinberger Sep 30, 2026
985573c
Merge remote-tracking branch 'origin/main' into fix/sse-connect-retry…
claude Sep 30, 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/sse-connect-retry-once.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@modelcontextprotocol/client': patch
---

`SSEClientTransport` now retries the SSE connection once after `onUnauthorized()` resolves, as documented. If the retry is also answered with 401, `start()` rejects with `SdkHttpError` (`ClientHttpAuthentication`) instead of calling `onUnauthorized()` again. A 401 on a later reconnect of a stream that had opened still gets one refresh.
19 changes: 17 additions & 2 deletions packages/client/src/client/sse.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,8 @@ export class SSEClientTransport implements Transport {
}

private _last401Response?: Response;
// True between a 401-triggered reconnect and the next successful open.
private _connectAuthRetried = false;

@claude claude Bot Sep 30, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

🟡 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.


private async _commonHeaders(): Promise<Headers> {
// Start from the caller-supplied `requestInit.headers` and `set()` the
Expand Down Expand Up @@ -239,9 +241,10 @@ export class SSEClientTransport implements Transport {

this._eventSource.onerror = event => {
if (event.code === 401 && this._authProvider) {
if (this._authProvider.onUnauthorized && this._last401Response) {
if (this._authProvider.onUnauthorized && this._last401Response && !this._connectAuthRetried) {
const response = this._last401Response;
this._last401Response = undefined;
this._connectAuthRetried = true;
this._eventSource?.close();
this._authProvider.onUnauthorized({ response, serverUrl: this._url, fetchFn: this._fetchWithInit }).then(
// onUnauthorized succeeded → retry fresh. Its onerror handles its own onerror?.() + reject.
Expand All @@ -250,14 +253,25 @@ export class SSEClientTransport implements Transport {
// 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()
Comment on lines +264 to +272

@claude claude Bot Sep 30, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

🟡 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.

);
this._last401Response = undefined;
reject(error);
this.onerror?.(error);
return;
Comment on lines 253 to 277

@claude claude Bot Sep 30, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

🔴 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).

Comment on lines +264 to 277

@claude claude Bot Sep 30, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

🟣 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.

Expand All @@ -270,6 +284,7 @@ export class SSEClientTransport implements Transport {

this._eventSource.onopen = () => {
// The connection is open, but we need to wait for the endpoint to be received.
this._connectAuthRetried = false;
Comment on lines 285 to +287

@claude claude Bot Sep 30, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

🟣 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.

};

this._eventSource.addEventListener('endpoint', (event: Event) => {
Expand Down
82 changes: 70 additions & 12 deletions packages/client/test/client/sse.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1783,10 +1783,43 @@ describe('SSEClientTransport', () => {
await expect(transport.finishAuth('auth-code')).rejects.toThrow('finishAuth requires an OAuthClientProvider');
});

it('SSE connect 401 retry does not poison future 401s — onUnauthorized called on each attempt', async () => {
it('SSE connect: a second 401 right after a refresh rejects, onUnauthorized called once', async () => {
await resourceServer.close();

let getAttempt = 0;
resourceServer = createServer((req, res) => {
if (req.method === 'GET') {
getAttempt++;
res.writeHead(401).end(); // always 401
}
});
resourceBaseUrl = await listenOnRandomPort(resourceServer);

// Backstop so an unbounded retry ends the test with a clear failure instead of a hang.
let calls = 0;
const authProvider: AuthProvider = {
token: vi.fn(async () => 'still-bad'),
onUnauthorized: vi.fn(async () => {
if (++calls >= 10) throw new Error('backstop: onUnauthorized called 10 times');
})
};
transport = new SSEClientTransport(resourceBaseUrl, { authProvider });
const onerror = vi.fn();
transport.onerror = onerror;

const error = await transport.start().catch(e => e);
expect(error).toBeInstanceOf(SdkHttpError);
expect((error as SdkHttpError).code).toBe(SdkErrorCode.ClientHttpAuthentication);
expect((error as SdkHttpError).status).toBe(401);
expect(authProvider.onUnauthorized).toHaveBeenCalledTimes(1);
expect(getAttempt).toBe(2);
expect(onerror).toHaveBeenCalledTimes(1);
});

it('SSE connect 401 retry does not poison future 401s — a 401 on a later reconnect refreshes again', async () => {
// Regression: _startOrAuth(true) baked isAuthRetry=true into the retry EventSource's
// onerror closure, so a subsequent 401 (token expiry on reconnect) would throw
// instead of refreshing. Fix: retry always calls _startOrAuth() fresh.
// instead of refreshing. The retry guard resets once the stream opens.
await resourceServer.close();

let getAttempt = 0;
Expand All @@ -1796,7 +1829,8 @@ describe('SSEClientTransport', () => {
return;
}
getAttempt++;
if (getAttempt < 3) {
// 1: 401, 2: opens then drops, 3: 401 on the automatic reconnect, 4: opens and stays.
if (getAttempt === 1 || getAttempt === 3) {
res.writeHead(401).end();
return;
}
Expand All @@ -1805,8 +1839,12 @@ describe('SSEClientTransport', () => {
'Cache-Control': 'no-cache, no-transform',
Connection: 'keep-alive'
});
res.write('retry: 10\n');
res.write('event: endpoint\n');
res.write(`data: ${resourceBaseUrl.href}post\n\n`);
if (getAttempt === 2) {
res.end();
}
});
resourceBaseUrl = await listenOnRandomPort(resourceServer);

Expand All @@ -1816,17 +1854,17 @@ describe('SSEClientTransport', () => {
};
transport = new SSEClientTransport(resourceBaseUrl, { authProvider });

await transport.start(); // should resolve on attempt 3
await transport.start(); // resolves on attempt 2
expect(authProvider.onUnauthorized).toHaveBeenCalledTimes(1);

await vi.waitFor(() => expect(getAttempt).toBe(4), { timeout: 3000 });
expect(authProvider.onUnauthorized).toHaveBeenCalledTimes(2);
expect(getAttempt).toBe(3);
});

it('retry failure during SSE connect fires onerror exactly once', async () => {
// Regression: when the retry EventSource rejected, its onerror fired inside, then
// the outer .then() rejection handler fired onerror AGAIN for the same error.
// Fix: inner retry chains to .then(resolve, reject) — no outer onerror call.
// onUnauthorized's own failure is handled separately and fires onerror once.
await resourceServer.close();

resourceServer = createServer((req, res) => {
Expand All @@ -1836,20 +1874,40 @@ describe('SSEClientTransport', () => {
});
resourceBaseUrl = await listenOnRandomPort(resourceServer);

const onUnauthorized: AuthProvider['onUnauthorized'] = vi
.fn()
.mockResolvedValueOnce(undefined) // first call succeeds → triggers retry
.mockRejectedValueOnce(new Error('refresh failed')); // second call (in retry) throws
const authProvider: AuthProvider = {
token: vi.fn(async () => 'token'),
onUnauthorized
onUnauthorized: vi.fn(async () => {})
};
transport = new SSEClientTransport(resourceBaseUrl, { authProvider });
const onerror = vi.fn();
transport.onerror = onerror;

await expect(transport.start()).rejects.toThrow('Server returned 401 after re-authentication');
expect(authProvider.onUnauthorized).toHaveBeenCalledTimes(1);
expect(onerror).toHaveBeenCalledTimes(1);
expect(onerror.mock.calls[0]![0].message).toBe('Server returned 401 after re-authentication');
});

it('a failing onUnauthorized during SSE connect fires onerror exactly once', async () => {
await resourceServer.close();

resourceServer = createServer((req, res) => {
if (req.method === 'GET') {
res.writeHead(401).end(); // always 401
}
});
resourceBaseUrl = await listenOnRandomPort(resourceServer);

const authProvider: AuthProvider = {
token: vi.fn(async () => 'token'),
onUnauthorized: vi.fn().mockRejectedValueOnce(new Error('refresh failed'))
};
transport = new SSEClientTransport(resourceBaseUrl, { authProvider });
const onerror = vi.fn();
transport.onerror = onerror;

await expect(transport.start()).rejects.toThrow('refresh failed');
expect(authProvider.onUnauthorized).toHaveBeenCalledTimes(2);
expect(authProvider.onUnauthorized).toHaveBeenCalledTimes(1);
expect(onerror).toHaveBeenCalledTimes(1);
expect(onerror.mock.calls[0]![0].message).toBe('refresh failed');
});
Expand Down
Loading