fix: stabilize oauth wait/redirect deferred lifecycle (#70) (thanks @monotykamary)

CHANGELOG.md

@@ -7,6 +7,7 @@
 - OAuth: avoid crashing on headless Linux when `xdg-open` is unavailable; clear stale dynamic-port client registrations; close callback server if stale-client persistence reads fail. (PR #72, thanks @mgonto)
 - Added optional `oauthScope`/`oauth_scope` config override as an escape hatch for providers that require explicit scopes.
 - `createCallResult().json()` now collects all parseable JSON entries from MCP content arrays (single item stays backward-compatible), and raw inspect depth now stays readable without unbounded traversal. (PR #91, thanks @Blankdlh)
+- OAuth wait/redirect now share one deferred to eliminate authorization race windows and preserve stable close-path errors, including wait-before-redirect and repeated-redirect flows. (PR #70, thanks @monotykamary)
 
 ### Tooling / Dependencies
 - Updated dependencies to latest releases (including MCP SDK, Rolldown RC, Zod, Biome, Oxlint, Vitest, Bun types).

src/oauth.ts

@@ -66,7 +66,6 @@ class PersistentOAuthClientProvider implements OAuthClientProvider {
   private readonly persistence: OAuthPersistence;
   private redirectUrlValue: URL;
   private authorizationDeferred: Deferred<string> | null = null;
-  private redirectInitiatedDeferred: Deferred<void> | null = null;
   private server?: http.Server;
 
   private constructor(
@@ -254,11 +253,8 @@ class PersistentOAuthClientProvider implements OAuthClientProvider {
 
   async redirectToAuthorization(authorizationUrl: URL): Promise<void> {
     this.logger.info(`Authorization required for ${this.definition.name}. Opening browser...`);
-    this.authorizationDeferred = createDeferred<string>();
-    // Resolve the redirect initiated deferred so waitForAuthorizationCode can proceed
-    this.redirectInitiatedDeferred?.resolve();
-    this.redirectInitiatedDeferred = null;
-    openExternal(authorizationUrl.toString());
+    this.ensureAuthorizationDeferred();
+    __oauthInternals.openExternal(authorizationUrl.toString());
     this.logger.info(`If the browser did not open, visit ${authorizationUrl.toString()} manually.`);
   }
 
@@ -280,22 +276,9 @@ class PersistentOAuthClientProvider implements OAuthClientProvider {
   }
 
   // waitForAuthorizationCode resolves once the local callback server captures a redirect.
-  // This must use the same deferred created by redirectToAuthorization; do not create
-  // a new one here to avoid race conditions where the callback resolves a different promise.
-  // If called before redirectToAuthorization, this method waits for it to be initiated.
+  // The same deferred is shared with redirectToAuthorization so callback resolution is stable.
   async waitForAuthorizationCode(): Promise<string> {
-    // If redirectToAuthorization hasn't been called yet, wait for it
-    if (!this.authorizationDeferred) {
-      if (!this.redirectInitiatedDeferred) {
-        this.redirectInitiatedDeferred = createDeferred<void>();
-      }
-      await this.redirectInitiatedDeferred.promise;
-    }
-    // Now authorizationDeferred must exist
-    if (!this.authorizationDeferred) {
-      throw new Error('OAuth authorization deferred was not created after redirect initiation.');
-    }
-    return this.authorizationDeferred.promise;
+    return this.ensureAuthorizationDeferred().promise;
   }
 
   // close stops the temporary callback server created for the OAuth session.
@@ -305,11 +288,6 @@ class PersistentOAuthClientProvider implements OAuthClientProvider {
       this.authorizationDeferred.reject(new Error('OAuth session closed before receiving authorization code.'));
       this.authorizationDeferred = null;
     }
-    if (this.redirectInitiatedDeferred) {
-      // Clean up the redirect initiated deferred to prevent memory leaks
-      this.redirectInitiatedDeferred.reject(new Error('OAuth session closed before redirect was initiated.'));
-      this.redirectInitiatedDeferred = null;
-    }
     if (!this.server) {
       return;
     }
@@ -318,6 +296,13 @@ class PersistentOAuthClientProvider implements OAuthClientProvider {
     });
     this.server = undefined;
   }
+
+  private ensureAuthorizationDeferred(): Deferred<string> {
+    if (!this.authorizationDeferred) {
+      this.authorizationDeferred = createDeferred<string>();
+    }
+    return this.authorizationDeferred;
+  }
 }
 
 export interface OAuthSession {

tests/oauth-session.test.ts

@@ -4,12 +4,39 @@ import os from 'node:os';
 import path from 'node:path';
 import { afterEach, describe, expect, it, vi } from 'vitest';
 import type { ServerDefinition } from '../src/config.js';
-import { createOAuthSession } from '../src/oauth.js';
+import { __oauthInternals, createOAuthSession } from '../src/oauth.js';
+
+type StatefulProvider = {
+  redirectUrl: string | URL;
+  state: () => Promise<string>;
+  redirectToAuthorization: (authorizationUrl: URL) => Promise<void>;
+};
+
+const requestStatus = (target: URL): Promise<number> =>
+  new Promise((resolve, reject) => {
+    const req = http.request(
+      {
+        hostname: target.hostname,
+        port: target.port,
+        path: `${target.pathname}${target.search}`,
+        family: 4,
+        method: 'GET',
+      },
+      (res) => {
+        const status = res.statusCode ?? 0;
+        res.resume();
+        resolve(status);
+      }
+    );
+    req.on('error', reject);
+    req.end();
+  });
 
 describe('FileOAuthClientProvider session lifecycle', () => {
   const tempDirs: string[] = [];
 
   afterEach(async () => {
+    vi.restoreAllMocks();
     await Promise.all(tempDirs.splice(0).map((dir) => fs.rm(dir, { recursive: true, force: true })));
   });
 
@@ -123,4 +150,67 @@ describe('FileOAuthClientProvider session lifecycle', () => {
       createServerSpy.mockRestore();
     }
   });
+
+  it('resolves waiters created before redirectToAuthorization', async () => {
+    const tokenCacheDir = await fs.mkdtemp(path.join(os.tmpdir(), 'mcporter-oauth-test-'));
+    tempDirs.push(tokenCacheDir);
+    const definition: ServerDefinition = {
+      name: 'test-oauth-wait-before-redirect',
+      description: 'Test OAuth server',
+      command: { kind: 'http', url: new URL('https://example.com/mcp') },
+      auth: 'oauth',
+      tokenCacheDir,
+    };
+    const logger = {
+      info: vi.fn(),
+      warn: vi.fn(),
+      error: vi.fn(),
+    };
+
+    const session = await createOAuthSession(definition, logger);
+    const provider = session.provider as StatefulProvider;
+    vi.spyOn(__oauthInternals, 'openExternal').mockImplementation(() => {});
+    const waitPromise = session.waitForAuthorizationCode();
+    await provider.redirectToAuthorization(new URL('https://example.com/auth'));
+
+    const callback = new URL(String(provider.redirectUrl));
+    callback.hostname = '127.0.0.1';
+    callback.searchParams.set('code', 'prewait-code');
+    const status = await requestStatus(callback);
+    expect(status).toBe(200);
+    await expect(waitPromise).resolves.toBe('prewait-code');
+    await session.close();
+  });
+
+  it('does not replace the pending authorization deferred on repeated redirect calls', async () => {
+    const tokenCacheDir = await fs.mkdtemp(path.join(os.tmpdir(), 'mcporter-oauth-test-'));
+    tempDirs.push(tokenCacheDir);
+    const definition: ServerDefinition = {
+      name: 'test-oauth-repeat-redirect',
+      description: 'Test OAuth server',
+      command: { kind: 'http', url: new URL('https://example.com/mcp') },
+      auth: 'oauth',
+      tokenCacheDir,
+    };
+    const logger = {
+      info: vi.fn(),
+      warn: vi.fn(),
+      error: vi.fn(),
+    };
+
+    const session = await createOAuthSession(definition, logger);
+    const provider = session.provider as StatefulProvider;
+    vi.spyOn(__oauthInternals, 'openExternal').mockImplementation(() => {});
+    const waitPromise = session.waitForAuthorizationCode();
+    await provider.redirectToAuthorization(new URL('https://example.com/auth-one'));
+    await provider.redirectToAuthorization(new URL('https://example.com/auth-two'));
+
+    const callback = new URL(String(provider.redirectUrl));
+    callback.hostname = '127.0.0.1';
+    callback.searchParams.set('code', 'stable-deferred-code');
+    const status = await requestStatus(callback);
+    expect(status).toBe(200);
+    await expect(waitPromise).resolves.toBe('stable-deferred-code');
+    await session.close();
+  });
 });