Report an MCP OAuth callback failure before a retry

A callback listener can fail before waitForAuthCode() starts to wait.
The provider then started a new listener and hid the original error
for up to five minutes.

- Keep a failure that occurs while no caller waits.
- Report that failure once on the next waitForAuthCode() call.
- Start a new listener on the call after that.
- Close each failed listener. Ignore errors from that cleanup.
- Start a new listener only after the old listener closes.
- Discard a kept failure when a new listener starts or close() runs.

Author: SoccerField24x7

src/services/mcp/McpOAuthClientProvider.ts

@@ -93,6 +93,15 @@ export class McpOAuthClientProvider implements OAuthClientProvider {
 	private _effectiveResourceIndicator: string | null
 	/** Deduplicates concurrent _ensureCallbackServer() calls. */
 	private _ensureServerPromise: Promise<void> | null = null
+	/** Callback promise most recently returned to a waitForAuthCode() caller. */
+	private _awaitedAuthCodePromise: Promise<string> | null = null
+	/**
+	 * Rejected callback promise of a listener that failed while no caller was waiting.
+	 * The next waitForAuthCode() call reports it once instead of starting another listener.
+	 */
+	private _unreportedAuthCodeFailure: Promise<string> | null = null
+	/** Stops a detached listener. A replacement listener waits for it because it may reuse the port. */
+	private _listenerCleanup: Promise<void> | null = null
 
 	private constructor(
 		private readonly _serverUrl: string,
@@ -243,6 +252,11 @@ export class McpOAuthClientProvider implements OAuthClientProvider {
 
 	private async _doStartCallbackServer(): Promise<void> {
 		this._closed = false
+		// A new listener belongs to a new authorization attempt, so an earlier failure no longer applies.
+		this._unreportedAuthCodeFailure = null
+		if (this._listenerCleanup) {
+			await this._listenerCleanup
+		}
 		const { port: configuredPort, host } = getCallbackAddress(this._configuredRedirectUri)
 		const { server, port, result, cancel } = await startCallbackServer(
 			configuredPort ?? this._port,
@@ -260,18 +274,31 @@ export class McpOAuthClientProvider implements OAuthClientProvider {
 		this._authCodePromise = authCodePromise
 
 		// Static clients can start this promise before waitForAuthCode() observes it.
-		// Observe rejection now to prevent an unhandled rejection, but keep the original
-		// promise so waitForAuthCode() still receives the callback error. Stop only this
-		// promise's listener before clearing it so a later flow can bind a fresh listener.
-		void authCodePromise.catch(async () => {
-			if (this._authCodePromise === authCodePromise && this._server === server) {
-				await stopCallbackServer(server, cancel).catch(() => {})
-				if (this._authCodePromise === authCodePromise && this._server === server) {
-					this._server = null
-					this._cancelCallbackServer = null
-				}
+		// Observe rejection now to prevent an unhandled rejection. A caller that is already
+		// waiting receives the error from this promise; otherwise keep the rejected promise so
+		// the next waitForAuthCode() call reports it instead of waiting on a new listener.
+		void authCodePromise.catch(() => {
+			// close() detaches this promise before it cancels the listener and stops it itself.
+			if (this._authCodePromise !== authCodePromise) return
+			if (this._awaitedAuthCodePromise !== authCodePromise) {
+				this._unreportedAuthCodeFailure = authCodePromise
+			}
+			// Detach only the failed listener so a later authorization attempt starts a fresh one.
+			// Keep the failed promise current for callers that are still resuming from startup.
+			this._server = null
+			this._cancelCallbackServer = null
+			this._stopDetachedListener(server, cancel)
+		})
+	}
+
+	/** Stops a listener that is no longer attached. A replacement listener waits because it may reuse the port. */
+	private _stopDetachedListener(server: http.Server, cancel: () => void): void {
+		const cleanup = stopCallbackServerIgnoringErrors(server, cancel).finally(() => {
+			if (this._listenerCleanup === cleanup) {
+				this._listenerCleanup = null
 			}
 		})
+		this._listenerCleanup = cleanup
 	}
 
 	state(): string {
@@ -520,12 +547,24 @@ export class McpOAuthClientProvider implements OAuthClientProvider {
 	 * Resolves with the authorization code once the user completes the OAuth
 	 * browser flow and the local callback server receives the redirect.
 	 * Rejects on error or 5-minute timeout.
+	 *
+	 * If the listener failed before any caller waited, the first call rejects with
+	 * that failure without starting another listener. A later call starts a fresh one.
 	 */
 	async waitForAuthCode(): Promise<string> {
-		if (!this._authCodePromise || !this._server) {
+		if (!this._unreportedAuthCodeFailure && (!this._authCodePromise || !this._server)) {
 			await this._ensureCallbackServer()
 		}
-		return this._authCodePromise!
+		const unreportedFailure = this._unreportedAuthCodeFailure
+		if (unreportedFailure) {
+			this._unreportedAuthCodeFailure = null
+			return unreportedFailure
+		}
+		// A caller that waited for listener startup receives that listener's result, even when
+		// another caller already took the failure while this caller was resuming.
+		const authCodePromise = this._authCodePromise!
+		this._awaitedAuthCodePromise = authCodePromise
+		return authCodePromise
 	}
 
 	/**
@@ -654,13 +693,30 @@ export class McpOAuthClientProvider implements OAuthClientProvider {
 		if (this._ensureServerPromise) {
 			await this._ensureServerPromise.catch(() => {})
 		}
-		if (!this._closed && this._server) {
+		// A closed flow must not report its failure to a later flow.
+		this._unreportedAuthCodeFailure = null
+		const server = this._server
+		if (!this._closed && server) {
 			this._closed = true
-			await stopCallbackServer(this._server, this._cancelCallbackServer ?? (() => {})).catch(() => {})
+			const cancel = this._cancelCallbackServer ?? (() => {})
+			// Detach before cancelling so the rejection handler does not record the cancellation.
 			this._server = null
 			this._cancelCallbackServer = null
 			this._authCodePromise = null
+			this._stopDetachedListener(server, cancel)
 		}
+		if (this._listenerCleanup) {
+			await this._listenerCleanup
+		}
+	}
+}
+
+/** Stops a callback listener. A cleanup error must not replace the callback error. */
+async function stopCallbackServerIgnoringErrors(server: http.Server, cancel: () => void): Promise<void> {
+	try {
+		await stopCallbackServer(server, cancel)
+	} catch {
+		// The callback result, not the cleanup result, is the error that callers report.
 	}
 }
 

src/services/mcp/__tests__/McpHub.spec.ts

@@ -3211,6 +3211,29 @@ describe("McpHub", () => {
 			).resolves.toBeUndefined()
 		})
 
+		it("should record the callback error and disconnect when waitForAuthCode rejects", async () => {
+			vsc.window.showInformationMessage.mockResolvedValueOnce(t("mcp:oauth.flow.authenticateButton"))
+			// The provider reports a callback listener failure, such as a timeout before the user authenticated.
+			mockAuthProvider.waitForAuthCode.mockRejectedValueOnce(new Error("Callback timeout"))
+
+			await mcpHub["_initiateOAuthFlow"](
+				serverName,
+				source,
+				config,
+				mockAuthProvider,
+				mockTransport,
+				mockConnection,
+			)
+
+			expect(mockAuthProvider.openBrowser).toHaveBeenCalledTimes(1)
+			expect(mockAuthProvider.exchangeCodeForTokens).not.toHaveBeenCalled()
+			expect(mockAuthProvider.close).toHaveBeenCalled()
+			expect(mockConnection.server.status).toBe("disconnected")
+			expect(mcpHub["appendErrorMessage"]).toHaveBeenCalledTimes(1)
+			expect(mcpHub["appendErrorMessage"]).toHaveBeenCalledWith(mockConnection, "Callback timeout")
+			expect(mcpHub["connectToServer"]).not.toHaveBeenCalled()
+		})
+
 		it("should disconnect and flag error when Cancel is pressed after Authenticate is clicked", async () => {
 			// Simulate: user clicks Authenticate in the toast, then cancels via the progress bar
 			// while waitForAuthCode is still pending.