Skip to content

Commit 3aa0cd6

Browse files
felixweinbergerfelixweinberger
committed
fix: restore SdkError(ClientHttpAuthentication) for circuit-breaker case
The 401-after-re-auth case (circuit breaker trips) should throw a distinct error from the normal 'token rejected' case: - First 401 with no onUnauthorized → UnauthorizedError — caller re-auths externally and reconnects - Second 401 after onUnauthorized succeeded → SdkError with ClientHttpAuthentication — server is misbehaving, don't blindly retry, escalate The previous commit collapsed these into UnauthorizedError, which risks callers catching it, re-authing, and looping. Restored the SdkError throw at all three 401 sites when _authRetryInFlight is already set. Reverted migration doc changes — ClientHttpAuthentication is not dead code.
1 parent f2c32e8 commit 3aa0cd6

7 files changed

Lines changed: 64 additions & 40 deletions

File tree

‎docs/migration-SKILL.md‎

Lines changed: 14 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -107,19 +107,19 @@ Two error classes now exist:
107107
- **`ProtocolError`** (renamed from `McpError`): Protocol errors that cross the wire as JSON-RPC responses
108108
- **`SdkError`** (new): Local SDK errors that never cross the wire
109109

110-
| Error scenario | v1 type | v2 type |
111-
| -------------------------------- | -------------------------------------------- | ----------------------------------------------------------------- |
112-
| Request timeout | `McpError` with `ErrorCode.RequestTimeout` | `SdkError` with `SdkErrorCode.RequestTimeout` |
113-
| Connection closed | `McpError` with `ErrorCode.ConnectionClosed` | `SdkError` with `SdkErrorCode.ConnectionClosed` |
114-
| Capability not supported | `new Error(...)` | `SdkError` with `SdkErrorCode.CapabilityNotSupported` |
115-
| Not connected | `new Error('Not connected')` | `SdkError` with `SdkErrorCode.NotConnected` |
116-
| Invalid params (server response) | `McpError` with `ErrorCode.InvalidParams` | `ProtocolError` with `ProtocolErrorCode.InvalidParams` |
117-
| HTTP transport error | `StreamableHTTPError` | `SdkError` with `SdkErrorCode.ClientHttp*` |
118-
| Failed to open SSE stream | `StreamableHTTPError` | `SdkError` with `SdkErrorCode.ClientHttpFailedToOpenStream` |
119-
| 401 after auth flow | `StreamableHTTPError` | `UnauthorizedError` |
120-
| 403 after upscoping | `StreamableHTTPError` | `SdkError` with `SdkErrorCode.ClientHttpForbidden` |
121-
| Unexpected content type | `StreamableHTTPError` | `SdkError` with `SdkErrorCode.ClientHttpUnexpectedContent` |
122-
| Session termination failed | `StreamableHTTPError` | `SdkError` with `SdkErrorCode.ClientHttpFailedToTerminateSession` |
110+
| Error scenario | v1 type | v2 type |
111+
| --------------------------------- | -------------------------------------------- | ----------------------------------------------------------------- |
112+
| Request timeout | `McpError` with `ErrorCode.RequestTimeout` | `SdkError` with `SdkErrorCode.RequestTimeout` |
113+
| Connection closed | `McpError` with `ErrorCode.ConnectionClosed` | `SdkError` with `SdkErrorCode.ConnectionClosed` |
114+
| Capability not supported | `new Error(...)` | `SdkError` with `SdkErrorCode.CapabilityNotSupported` |
115+
| Not connected | `new Error('Not connected')` | `SdkError` with `SdkErrorCode.NotConnected` |
116+
| Invalid params (server response) | `McpError` with `ErrorCode.InvalidParams` | `ProtocolError` with `ProtocolErrorCode.InvalidParams` |
117+
| HTTP transport error | `StreamableHTTPError` | `SdkError` with `SdkErrorCode.ClientHttp*` |
118+
| Failed to open SSE stream | `StreamableHTTPError` | `SdkError` with `SdkErrorCode.ClientHttpFailedToOpenStream` |
119+
| 401 after re-auth (circuit break) | `StreamableHTTPError` | `SdkError` with `SdkErrorCode.ClientHttpAuthentication` |
120+
| 403 after upscoping | `StreamableHTTPError` | `SdkError` with `SdkErrorCode.ClientHttpForbidden` |
121+
| Unexpected content type | `StreamableHTTPError` | `SdkError` with `SdkErrorCode.ClientHttpUnexpectedContent` |
122+
| Session termination failed | `StreamableHTTPError` | `SdkError` with `SdkErrorCode.ClientHttpFailedToTerminateSession` |
123123

124124
New `SdkErrorCode` enum values:
125125

@@ -131,6 +131,7 @@ 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'`
134135
- `SdkErrorCode.ClientHttpForbidden` = `'CLIENT_HTTP_FORBIDDEN'`
135136
- `SdkErrorCode.ClientHttpUnexpectedContent` = `'CLIENT_HTTP_UNEXPECTED_CONTENT'`
136137
- `SdkErrorCode.ClientHttpFailedToOpenStream` = `'CLIENT_HTTP_FAILED_TO_OPEN_STREAM'`

‎docs/migration.md‎

Lines changed: 19 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -575,21 +575,21 @@ try {
575575

576576
The new `SdkErrorCode` enum contains string-valued codes for local SDK errors:
577577

578-
| Code | Description |
579-
| ------------------------------------------------- | ------------------------------------------ |
580-
| `SdkErrorCode.NotConnected` | Transport is not connected |
581-
| `SdkErrorCode.AlreadyConnected` | Transport is already connected |
582-
| `SdkErrorCode.NotInitialized` | Protocol is not initialized |
583-
| `SdkErrorCode.CapabilityNotSupported` | Required capability is not supported |
584-
| `SdkErrorCode.RequestTimeout` | Request timed out waiting for response |
585-
| `SdkErrorCode.ConnectionClosed` | Connection was closed |
586-
| `SdkErrorCode.SendFailed` | Failed to send message |
587-
| `SdkErrorCode.ClientHttpNotImplemented` | HTTP POST request failed |
588-
| `UnauthorizedError` (thrown, not `SdkError`) | Server returned 401 after re-auth attempt |
589-
| `SdkErrorCode.ClientHttpForbidden` | Server returned 403 after trying upscoping |
590-
| `SdkErrorCode.ClientHttpUnexpectedContent` | Unexpected content type in HTTP response |
591-
| `SdkErrorCode.ClientHttpFailedToOpenStream` | Failed to open SSE stream |
592-
| `SdkErrorCode.ClientHttpFailedToTerminateSession` | Failed to terminate session |
578+
| Code | Description |
579+
| ------------------------------------------------- | ------------------------------------------- |
580+
| `SdkErrorCode.NotConnected` | Transport is not connected |
581+
| `SdkErrorCode.AlreadyConnected` | Transport is already connected |
582+
| `SdkErrorCode.NotInitialized` | Protocol is not initialized |
583+
| `SdkErrorCode.CapabilityNotSupported` | Required capability is not supported |
584+
| `SdkErrorCode.RequestTimeout` | Request timed out waiting for response |
585+
| `SdkErrorCode.ConnectionClosed` | Connection was closed |
586+
| `SdkErrorCode.SendFailed` | Failed to send message |
587+
| `SdkErrorCode.ClientHttpNotImplemented` | HTTP POST request failed |
588+
| `SdkErrorCode.ClientHttpAuthentication` | Server returned 401 after re-authentication |
589+
| `SdkErrorCode.ClientHttpForbidden` | Server returned 403 after trying upscoping |
590+
| `SdkErrorCode.ClientHttpUnexpectedContent` | Unexpected content type in HTTP response |
591+
| `SdkErrorCode.ClientHttpFailedToOpenStream` | Failed to open SSE stream |
592+
| `SdkErrorCode.ClientHttpFailedToTerminateSession` | Failed to terminate session |
593593

594594
#### `StreamableHTTPError` removed
595595

@@ -617,10 +617,11 @@ import { SdkError, SdkErrorCode } from '@modelcontextprotocol/core';
617617
try {
618618
await transport.send(message);
619619
} catch (error) {
620-
if (error instanceof UnauthorizedError) {
621-
console.log('Token rejected — reconnect with fresh credentials');
622-
} else if (error instanceof SdkError) {
620+
if (error instanceof SdkError) {
623621
switch (error.code) {
622+
case SdkErrorCode.ClientHttpAuthentication:
623+
console.log('Auth failed — server rejected token after re-auth');
624+
break;
624625
case SdkErrorCode.ClientHttpForbidden:
625626
console.log('Forbidden after upscoping attempt');
626627
break;

‎packages/client/src/client/sse.ts‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -279,8 +279,13 @@ export class SSEClientTransport implements Transport {
279279
// Purposely _not_ awaited, so we don't call onerror twice
280280
return this.send(message);
281281
}
282+
await response.text?.().catch(() => {});
283+
if (this._authRetryInFlight) {
284+
throw new SdkError(SdkErrorCode.ClientHttpAuthentication, 'Server returned 401 after re-authentication', {
285+
status: 401
286+
});
287+
}
282288
if (this._authProvider) {
283-
await response.text?.().catch(() => {});
284289
throw new UnauthorizedError();
285290
}
286291
}

‎packages/client/src/client/streamableHttp.ts‎

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -229,8 +229,13 @@ export class StreamableHTTPClientTransport implements Transport {
229229
// Purposely _not_ awaited, so we don't call onerror twice
230230
return this._startOrAuthSse(options);
231231
}
232+
await response.text?.().catch(() => {});
233+
if (this._authRetryInFlight) {
234+
throw new SdkError(SdkErrorCode.ClientHttpAuthentication, 'Server returned 401 after re-authentication', {
235+
status: 401
236+
});
237+
}
232238
if (this._authProvider) {
233-
await response.text?.().catch(() => {});
234239
throw new UnauthorizedError();
235240
}
236241
}
@@ -514,8 +519,13 @@ export class StreamableHTTPClientTransport implements Transport {
514519
// Purposely _not_ awaited, so we don't call onerror twice
515520
return this.send(message);
516521
}
522+
await response.text?.().catch(() => {});
523+
if (this._authRetryInFlight) {
524+
throw new SdkError(SdkErrorCode.ClientHttpAuthentication, 'Server returned 401 after re-authentication', {
525+
status: 401
526+
});
527+
}
517528
if (this._authProvider) {
518-
await response.text?.().catch(() => {});
519529
throw new UnauthorizedError();
520530
}
521531
}

‎packages/client/test/client/sse.test.ts‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@ import { createServer } from 'node:http';
33
import type { AddressInfo } from 'node:net';
44

55
import type { JSONRPCMessage, OAuthTokens } from '@modelcontextprotocol/core';
6-
import { OAuthError, OAuthErrorCode } from '@modelcontextprotocol/core';
6+
import { OAuthError, OAuthErrorCode, SdkError, SdkErrorCode } from '@modelcontextprotocol/core';
77
import { listenOnRandomPort } from '@modelcontextprotocol/test-helpers';
88
import type { Mock, Mocked, MockedFunction, MockInstance } from 'vitest';
99

@@ -1575,7 +1575,7 @@ describe('SSEClientTransport', () => {
15751575
await expect(transport.send(message)).rejects.toThrow(UnauthorizedError);
15761576
});
15771577

1578-
it('enforces circuit breaker on double-401: onUnauthorized called once, then throws', async () => {
1578+
it('enforces circuit breaker on double-401: onUnauthorized called once, then throws SdkError', async () => {
15791579
postResponses = [401, 401];
15801580
await setupServer();
15811581

@@ -1586,7 +1586,9 @@ describe('SSEClientTransport', () => {
15861586
transport = new SSEClientTransport(resourceBaseUrl, { authProvider });
15871587
await transport.start();
15881588

1589-
await expect(transport.send(message)).rejects.toThrow(UnauthorizedError);
1589+
const error = await transport.send(message).catch(e => e);
1590+
expect(error).toBeInstanceOf(SdkError);
1591+
expect((error as SdkError).code).toBe(SdkErrorCode.ClientHttpAuthentication);
15901592
expect(authProvider.onUnauthorized).toHaveBeenCalledTimes(1);
15911593
expect(postCount).toBe(2);
15921594
});

‎packages/client/test/client/streamableHttp.test.ts‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1678,7 +1678,9 @@ describe('StreamableHTTPClientTransport', () => {
16781678
// Retry the original request - still 401 (broken server)
16791679
.mockResolvedValueOnce(unauthedResponse);
16801680

1681-
await expect(transport.send(message)).rejects.toThrow(UnauthorizedError);
1681+
const error = await transport.send(message).catch(e => e);
1682+
expect(error).toBeInstanceOf(SdkError);
1683+
expect((error as SdkError).code).toBe(SdkErrorCode.ClientHttpAuthentication);
16821684
expect(mockAuthProvider.saveTokens).toHaveBeenCalledWith({
16831685
access_token: 'new-access-token',
16841686
token_type: 'Bearer',

‎packages/client/test/client/tokenProvider.test.ts‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
import type { JSONRPCMessage } from '@modelcontextprotocol/core';
2+
import { SdkError, SdkErrorCode } from '@modelcontextprotocol/core';
23
import type { Mock } from 'vitest';
34

45
import type { AuthProvider } from '../../src/client/auth.js';
@@ -81,7 +82,7 @@ describe('StreamableHTTPClientTransport with AuthProvider', () => {
8182
expect(retryInit.headers.get('Authorization')).toBe('Bearer new-token');
8283
});
8384

84-
it('should throw UnauthorizedError if retry after onUnauthorized also gets 401', async () => {
85+
it('should throw SdkError(ClientHttpAuthentication) if retry after onUnauthorized also gets 401', async () => {
8586
const authProvider: AuthProvider = {
8687
token: vi.fn(async () => 'still-bad'),
8788
onUnauthorized: vi.fn(async () => {})
@@ -93,7 +94,9 @@ describe('StreamableHTTPClientTransport with AuthProvider', () => {
9394
.mockResolvedValueOnce({ ok: false, status: 401, headers: new Headers(), text: async () => 'unauthorized' })
9495
.mockResolvedValueOnce({ ok: false, status: 401, headers: new Headers(), text: async () => 'unauthorized' });
9596

96-
await expect(transport.send(message)).rejects.toThrow(UnauthorizedError);
97+
const error = await transport.send(message).catch(e => e);
98+
expect(error).toBeInstanceOf(SdkError);
99+
expect((error as SdkError).code).toBe(SdkErrorCode.ClientHttpAuthentication);
97100
expect(authProvider.onUnauthorized).toHaveBeenCalledTimes(1);
98101
});
99102

0 commit comments

Comments
 (0)