fix(client): deduplicate concurrent OAuth refreshes

.changeset/deduplicate-concurrent-oauth-refresh.md

@@ -0,0 +1,16 @@
+---
+'@modelcontextprotocol/client': patch
+---
+
+Deduplicate concurrent OAuth flows only when the provider and request options match. Ordinary
+calls with different resources, scopes, discovery validation settings, metadata URLs, or fetch
+functions run serially and read credentials after the previous operation settles, including failures.
+Authorization-code exchanges and forced reauthorization still bypass this queue.
+
+Add the optional `OAuthClientProvider.withAuthTransaction` hook so hosts can protect a complete
+auth operation, including credential invalidation and recovery retries, with their own lock.
+Code exchanges and forced reauthorization also invoke the hook independently.
+
+Treat `error_description: null` as an omitted description when parsing an OAuth error response.
+This allows `invalid_grant` recovery without relaxing the public OAuth error schema or accepting
+malformed values in other fields.

packages/client/src/client/auth.ts

@@ -241,6 +241,21 @@ export function adaptOAuthProvider(
  * No changes are needed to existing implementations.
  */
 export interface OAuthClientProvider {
+    /**
+     * Optionally runs a complete {@linkcode auth} operation inside a host-managed
+     * credential transaction (for example, a cross-process lock). The operation
+     * includes credential reads, writes, invalidation, and recovery retries.
+     * Authorization-code exchanges and forced reauthorization also use this hook,
+     * but are never deduplicated by the SDK.
+     *
+     * Implementations must invoke and await `operation`, returning its result and
+     * releasing any lock in `finally`. Do not release the lock when an outer wait
+     * is cancelled while the underlying operation is still running. The operation
+     * returns `REDIRECT` after initiating authorization; it does not hold a lock
+     * across the human/browser authorization wait.
+     */
+    withAuthTransaction?(operation: () => Promise<AuthResult>): Promise<AuthResult>;
+
     /**
      * The URL to redirect the user agent to after authorization.
      * Return `undefined` for non-interactive flows that don't require user interaction
@@ -950,7 +965,19 @@ export async function parseErrorResponse(input: Response | string): Promise<OAut
     const body = input instanceof Response ? await input.text() : input;
 
     try {
-        const result = OAuthErrorResponseSchema.parse(JSON.parse(body));
+        const parsed: unknown = JSON.parse(body);
+        // Some servers send null for an absent description. Normalize only at the
+        // wire boundary; the public schema and all other fields remain strict.
+        if (
+            parsed !== null &&
+            typeof parsed === 'object' &&
+            !Array.isArray(parsed) &&
+            'error_description' in parsed &&
+            parsed.error_description === null
+        ) {
+            delete parsed.error_description;
+        }
+        const result = OAuthErrorResponseSchema.parse(parsed);
         return OAuthError.fromResponse(result);
     } catch (error) {
         // Not a valid OAuth error response, but try to inform the user of the raw data anyway
@@ -1036,13 +1063,51 @@ function warnCredentialInvalidation(provider: OAuthClientProvider, error: OAuthE
     console.warn(`[mcp-sdk] OAuth ${JSON.stringify(error.code)} — ${action}. Cause: ${JSON.stringify(error.message)}`);
 }
 
+const pendingAuthRequests = new WeakMap<OAuthClientProvider, { options: AuthOptions; request: Promise<AuthResult> }>();
+
+function equivalentAuthOptions(a: AuthOptions, b: AuthOptions): boolean {
+    return (
+        String(a.serverUrl) === String(b.serverUrl) &&
+        a.resourceMetadataUrl?.toString() === b.resourceMetadataUrl?.toString() &&
+        a.scope === b.scope &&
+        (a.skipIssuerMetadataValidation ?? false) === (b.skipIssuerMetadataValidation ?? false) &&
+        a.fetchFn === b.fetchFn
+    );
+}
+
 /**
  * Orchestrates the full auth flow with a server.
  *
  * This can be used as a single entry point for all authorization functionality,
  * instead of linking together the other lower-level functions in this module.
  */
 export async function auth(provider: OAuthClientProvider, options: AuthOptions): Promise<AuthResult> {
+    const execute = (): Promise<AuthResult> => {
+        const operation = (): Promise<AuthResult> => authWithErrorHandling(provider, options);
+        return provider.withAuthTransaction ? provider.withAuthTransaction(operation) : operation();
+    };
+    if (options.authorizationCode !== undefined || options.forceReauthorization === true) {
+        return execute();
+    }
+
+    const pending = pendingAuthRequests.get(provider);
+    if (pending && equivalentAuthOptions(pending.options, options)) return pending.request;
+
+    // Incompatible ordinary calls must use their own options and read credentials
+    // after the previous operation settles, even if it failed. Track the queue tail
+    // so equivalent callers can also share an operation that has not started yet.
+    const request = pending ? pending.request.then(execute, execute) : Promise.resolve().then(execute);
+    pendingAuthRequests.set(provider, { options, request });
+    try {
+        return await request;
+    } finally {
+        if (pendingAuthRequests.get(provider)?.request === request) {
+            pendingAuthRequests.delete(provider);
+        }
+    }
+}
+
+async function authWithErrorHandling(provider: OAuthClientProvider, options: AuthOptions): Promise<AuthResult> {
     try {
         return await authInternal(provider, options);
     } catch (error) {

packages/client/test/client/auth.test.ts

@@ -3049,6 +3049,214 @@ describe('OAuth Authorization', () => {
             expect(body.get('refresh_token')).toBe('refresh123');
         });
 
+        describe('concurrent auth deduplication', () => {
+            const configureRefresh = (tokenResponse: (body: URLSearchParams) => Promise<Response> | Response): void => {
+                mockFetch.mockImplementation((url, init) => {
+                    const urlString = url.toString();
+                    if (urlString.includes('/.well-known/oauth-protected-resource')) {
+                        return Promise.resolve(
+                            Response.json({
+                                resource: 'https://api.example.com/mcp-server',
+                                authorization_servers: ['https://auth.example.com']
+                            })
+                        );
+                    }
+                    if (urlString.includes('/.well-known/oauth-authorization-server')) {
+                        return Promise.resolve(
+                            Response.json({
+                                issuer: 'https://auth.example.com',
+                                authorization_endpoint: 'https://auth.example.com/authorize',
+                                token_endpoint: 'https://auth.example.com/token',
+                                response_types_supported: ['code'],
+                                code_challenge_methods_supported: ['S256']
+                            })
+                        );
+                    }
+                    if (urlString.includes('/token')) {
+                        return Promise.resolve(tokenResponse(init?.body as URLSearchParams));
+                    }
+                    return Promise.resolve(Response.json({}, { status: 404 }));
+                });
+                (mockProvider.clientInformation as Mock).mockResolvedValue({ client_id: 'test-client', client_secret: 'test-secret' });
+                (mockProvider.tokens as Mock).mockResolvedValue({
+                    access_token: 'expired-access',
+                    refresh_token: 'current-refresh',
+                    issuer: 'https://auth.example.com'
+                });
+                (mockProvider.saveTokens as Mock).mockResolvedValue(undefined);
+                (mockProvider.codeVerifier as Mock).mockResolvedValue('test-verifier');
+            };
+
+            it('deduplicates concurrent refreshes for the same provider', async () => {
+                let tokenRequests = 0;
+                let release!: () => void;
+                let markStarted!: () => void;
+                const started = new Promise<void>(resolve => {
+                    markStarted = resolve;
+                });
+                const gate = new Promise<void>(resolve => {
+                    release = resolve;
+                });
+                configureRefresh(async () => {
+                    tokenRequests++;
+                    markStarted();
+                    await gate;
+                    return Response.json({
+                        access_token: 'new-access',
+                        refresh_token: 'new-refresh',
+                        token_type: 'Bearer',
+                        expires_in: 3600
+                    });
+                });
+
+                const first = auth(mockProvider, { serverUrl: 'https://api.example.com/mcp-server' });
+                await started;
+                const requests = [
+                    first,
+                    auth(mockProvider, { serverUrl: 'https://api.example.com/mcp-server' }),
+                    auth(mockProvider, { serverUrl: 'https://api.example.com/mcp-server' })
+                ];
+                release();
+
+                await expect(Promise.all(requests)).resolves.toEqual(['AUTHORIZED', 'AUTHORIZED', 'AUTHORIZED']);
+                expect(tokenRequests).toBe(1);
+                expect(mockProvider.saveTokens).toHaveBeenCalledTimes(1);
+            });
+
+            it('allows a new refresh after the previous one completes', async () => {
+                let tokenRequests = 0;
+                configureRefresh(() => {
+                    tokenRequests++;
+                    return Response.json({
+                        access_token: `access-${tokenRequests}`,
+                        refresh_token: `refresh-${tokenRequests}`,
+                        token_type: 'Bearer',
+                        expires_in: 3600
+                    });
+                });
+
+                await auth(mockProvider, { serverUrl: 'https://api.example.com/mcp-server' });
+                await auth(mockProvider, { serverUrl: 'https://api.example.com/mcp-server' });
+
+                expect(tokenRequests).toBe(2);
+            });
+
+            it('does not coalesce an authorization-code exchange with an in-flight refresh', async () => {
+                let releaseRefresh!: () => void;
+                let refreshStarted!: () => void;
+                const refreshGate = new Promise<void>(resolve => {
+                    releaseRefresh = resolve;
+                });
+                const started = new Promise<void>(resolve => {
+                    refreshStarted = resolve;
+                });
+                let codeRequests = 0;
+                configureRefresh(async body => {
+                    if (body.get('grant_type') === 'refresh_token') {
+                        refreshStarted();
+                        await refreshGate;
+                    } else {
+                        codeRequests++;
+                    }
+                    return Response.json({ access_token: 'new-access', refresh_token: 'new-refresh', token_type: 'Bearer' });
+                });
+
+                const refresh = auth(mockProvider, { serverUrl: 'https://api.example.com/mcp-server' });
+                await started;
+                const codeExchange = auth(mockProvider, {
+                    serverUrl: 'https://api.example.com/mcp-server',
+                    authorizationCode: 'auth-code'
+                });
+
+                await expect(codeExchange).resolves.toBe('AUTHORIZED');
+                expect(codeRequests).toBe(1);
+                releaseRefresh();
+                await expect(refresh).resolves.toBe('AUTHORIZED');
+            });
+
+            it('does not coalesce forced reauthorization with an in-flight refresh', async () => {
+                let releaseRefresh!: () => void;
+                let refreshStarted!: () => void;
+                const refreshGate = new Promise<void>(resolve => {
+                    releaseRefresh = resolve;
+                });
+                const started = new Promise<void>(resolve => {
+                    refreshStarted = resolve;
+                });
+                configureRefresh(async body => {
+                    if (body.get('grant_type') === 'refresh_token') {
+                        refreshStarted();
+                        await refreshGate;
+                    }
+                    return Response.json({ access_token: 'new-access', refresh_token: 'new-refresh', token_type: 'Bearer' });
+                });
+
+                const refresh = auth(mockProvider, { serverUrl: 'https://api.example.com/mcp-server' });
+                await started;
+                await expect(
+                    auth(mockProvider, {
+                        serverUrl: 'https://api.example.com/mcp-server',
+                        forceReauthorization: true
+                    })
+                ).resolves.toBe('REDIRECT');
+                expect(mockProvider.redirectToAuthorization).toHaveBeenCalledTimes(1);
+                releaseRefresh();
+                await expect(refresh).resolves.toBe('AUTHORIZED');
+            });
+
+            it('does not deduplicate different providers', async () => {
+                let tokenRequests = 0;
+                configureRefresh(() => {
+                    tokenRequests++;
+                    return Response.json({ access_token: 'new-access', refresh_token: 'new-refresh', token_type: 'Bearer' });
+                });
+                const otherProvider: OAuthClientProvider = {
+                    ...mockProvider,
+                    clientInformation: vi.fn().mockResolvedValue({ client_id: 'other-client', client_secret: 'other-secret' }),
+                    tokens: vi.fn().mockResolvedValue({
+                        access_token: 'other-expired-access',
+                        refresh_token: 'other-current-refresh',
+                        issuer: 'https://auth.example.com'
+                    }),
+                    saveTokens: vi.fn().mockResolvedValue(undefined)
+                };
+
+                await expect(
+                    Promise.all([
+                        auth(mockProvider, { serverUrl: 'https://api.example.com/mcp-server' }),
+                        auth(otherProvider, { serverUrl: 'https://api.example.com/mcp-server' })
+                    ])
+                ).resolves.toEqual(['AUTHORIZED', 'AUTHORIZED']);
+                expect(tokenRequests).toBe(2);
+            });
+
+            it('propagates one refresh failure to every concurrent caller', async () => {
+                const persistError = new Error('credential store unavailable');
+                let tokenRequests = 0;
+                configureRefresh(() => {
+                    tokenRequests++;
+                    return Response.json({ access_token: 'new-access', refresh_token: 'new-refresh', token_type: 'Bearer' });
+                });
+                (mockProvider.saveTokens as Mock).mockRejectedValueOnce(persistError);
+
+                const results = await Promise.allSettled([
+                    auth(mockProvider, { serverUrl: 'https://api.example.com/mcp-server' }),
+                    auth(mockProvider, { serverUrl: 'https://api.example.com/mcp-server' }),
+                    auth(mockProvider, { serverUrl: 'https://api.example.com/mcp-server' })
+                ]);
+
+                expect(results).toEqual([
+                    { status: 'rejected', reason: persistError },
+                    { status: 'rejected', reason: persistError },
+                    { status: 'rejected', reason: persistError }
+                ]);
+                expect(tokenRequests).toBe(1);
+
+                await expect(auth(mockProvider, { serverUrl: 'https://api.example.com/mcp-server' })).resolves.toBe('AUTHORIZED');
+                expect(tokenRequests).toBe(2);
+            });
+        });
+
         // The #2034 tests below differ only in how the token endpoint answers, so the
         // discovery fixture is shared. `tokenResponse` is invoked per POST to /token.
         const mockDiscoveryWithTokenEndpoint = (tokenResponse: () => unknown): void => {