Skip to content

Commit f2c32e8

Browse files
fix: address round-4 review comments on 401 handling
Four fixes from claude[bot] review on the AuthProvider approach: 1. Drain 401 response body after onUnauthorized() succeeds, before the retry. Unconsumed bodies block socket recycling in undici. All three 401 sites now drain before return. 2. _startOrAuthSse() 401 retry was return await, causing onerror to fire twice (recursive call's catch + outer catch both fire). Changed to return (not awaited) matching the send() pattern. Removed the try/finally, added flag reset to success path + outer catch instead. 3. Migration docs still referenced SdkErrorCode.ClientHttpAuthentication for the 401-after-auth case, but that throw site was replaced by _authRetryInFlight which throws UnauthorizedError. Updated both migration.md and migration-SKILL.md. 4. Pre-existing: 403 upscoping auth() call passed this._fetch instead of this._fetchWithInit, dropping custom requestInit options during token requests. All other auth() calls in this transport already used _fetchWithInit.
1 parent 65b5099 commit f2c32e8

4 files changed

Lines changed: 18 additions & 18 deletions

File tree

docs/migration-SKILL.md

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -116,7 +116,7 @@ Two error classes now exist:
116116
| Invalid params (server response) | `McpError` with `ErrorCode.InvalidParams` | `ProtocolError` with `ProtocolErrorCode.InvalidParams` |
117117
| HTTP transport error | `StreamableHTTPError` | `SdkError` with `SdkErrorCode.ClientHttp*` |
118118
| Failed to open SSE stream | `StreamableHTTPError` | `SdkError` with `SdkErrorCode.ClientHttpFailedToOpenStream` |
119-
| 401 after auth flow | `StreamableHTTPError` | `SdkError` with `SdkErrorCode.ClientHttpAuthentication` |
119+
| 401 after auth flow | `StreamableHTTPError` | `UnauthorizedError` |
120120
| 403 after upscoping | `StreamableHTTPError` | `SdkError` with `SdkErrorCode.ClientHttpForbidden` |
121121
| Unexpected content type | `StreamableHTTPError` | `SdkError` with `SdkErrorCode.ClientHttpUnexpectedContent` |
122122
| Session termination failed | `StreamableHTTPError` | `SdkError` with `SdkErrorCode.ClientHttpFailedToTerminateSession` |
@@ -131,7 +131,6 @@ New `SdkErrorCode` enum values:
131131
- `SdkErrorCode.ConnectionClosed` = `'CONNECTION_CLOSED'`
132132
- `SdkErrorCode.SendFailed` = `'SEND_FAILED'`
133133
- `SdkErrorCode.ClientHttpNotImplemented` = `'CLIENT_HTTP_NOT_IMPLEMENTED'`
134-
- `SdkErrorCode.ClientHttpAuthentication` = `'CLIENT_HTTP_AUTHENTICATION'`
135134
- `SdkErrorCode.ClientHttpForbidden` = `'CLIENT_HTTP_FORBIDDEN'`
136135
- `SdkErrorCode.ClientHttpUnexpectedContent` = `'CLIENT_HTTP_UNEXPECTED_CONTENT'`
137136
- `SdkErrorCode.ClientHttpFailedToOpenStream` = `'CLIENT_HTTP_FAILED_TO_OPEN_STREAM'`

docs/migration.md

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -585,7 +585,7 @@ The new `SdkErrorCode` enum contains string-valued codes for local SDK errors:
585585
| `SdkErrorCode.ConnectionClosed` | Connection was closed |
586586
| `SdkErrorCode.SendFailed` | Failed to send message |
587587
| `SdkErrorCode.ClientHttpNotImplemented` | HTTP POST request failed |
588-
| `SdkErrorCode.ClientHttpAuthentication` | Server returned 401 after successful auth |
588+
| `UnauthorizedError` (thrown, not `SdkError`) | Server returned 401 after re-auth attempt |
589589
| `SdkErrorCode.ClientHttpForbidden` | Server returned 403 after trying upscoping |
590590
| `SdkErrorCode.ClientHttpUnexpectedContent` | Unexpected content type in HTTP response |
591591
| `SdkErrorCode.ClientHttpFailedToOpenStream` | Failed to open SSE stream |
@@ -617,11 +617,10 @@ import { SdkError, SdkErrorCode } from '@modelcontextprotocol/core';
617617
try {
618618
await transport.send(message);
619619
} catch (error) {
620-
if (error instanceof SdkError) {
620+
if (error instanceof UnauthorizedError) {
621+
console.log('Token rejected — reconnect with fresh credentials');
622+
} else if (error instanceof SdkError) {
621623
switch (error.code) {
622-
case SdkErrorCode.ClientHttpAuthentication:
623-
console.log('Auth failed after completing auth flow');
624-
break;
625624
case SdkErrorCode.ClientHttpForbidden:
626625
console.log('Forbidden after upscoping attempt');
627626
break;

packages/client/src/client/sse.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -275,6 +275,7 @@ export class SSEClientTransport implements Transport {
275275
serverUrl: this._url,
276276
fetchFn: this._fetchWithInit
277277
});
278+
await response.text?.().catch(() => {});
278279
// Purposely _not_ awaited, so we don't call onerror twice
279280
return this.send(message);
280281
}

packages/client/src/client/streamableHttp.ts

Lines changed: 12 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -220,16 +220,14 @@ export class StreamableHTTPClientTransport implements Transport {
220220

221221
if (this._authProvider?.onUnauthorized && !this._authRetryInFlight) {
222222
this._authRetryInFlight = true;
223-
try {
224-
await this._authProvider.onUnauthorized({
225-
response,
226-
serverUrl: this._url,
227-
fetchFn: this._fetchWithInit
228-
});
229-
return await this._startOrAuthSse(options);
230-
} finally {
231-
this._authRetryInFlight = false;
232-
}
223+
await this._authProvider.onUnauthorized({
224+
response,
225+
serverUrl: this._url,
226+
fetchFn: this._fetchWithInit
227+
});
228+
await response.text?.().catch(() => {});
229+
// Purposely _not_ awaited, so we don't call onerror twice
230+
return this._startOrAuthSse(options);
233231
}
234232
if (this._authProvider) {
235233
await response.text?.().catch(() => {});
@@ -251,8 +249,10 @@ export class StreamableHTTPClientTransport implements Transport {
251249
});
252250
}
253251

252+
this._authRetryInFlight = false;
254253
this._handleSseStream(response.body, options, true);
255254
} catch (error) {
255+
this._authRetryInFlight = false;
256256
this.onerror?.(error as Error);
257257
throw error;
258258
}
@@ -510,6 +510,7 @@ export class StreamableHTTPClientTransport implements Transport {
510510
serverUrl: this._url,
511511
fetchFn: this._fetchWithInit
512512
});
513+
await response.text?.().catch(() => {});
513514
// Purposely _not_ awaited, so we don't call onerror twice
514515
return this.send(message);
515516
}
@@ -549,7 +550,7 @@ export class StreamableHTTPClientTransport implements Transport {
549550
serverUrl: this._url,
550551
resourceMetadataUrl: this._resourceMetadataUrl,
551552
scope: this._scope,
552-
fetchFn: this._fetch
553+
fetchFn: this._fetchWithInit
553554
});
554555

555556
if (result !== 'AUTHORIZED') {

0 commit comments

Comments
 (0)