chore: some suggestions on the PR

Author: kmruiz

src/common/connectionManager.ts

@@ -140,9 +140,7 @@ export class ConnectionManager extends EventEmitter<ConnectionManagerEvents> {
         try {
             const connectionType = ConnectionManager.inferConnectionTypeFromSettings(this.userConfig, connectionInfo);
             if (connectionType.startsWith("oidc")) {
-                // The error here is irrelevant, we only use this ping to force the connection
-                // Errors will be handled by the auth flow.
-                void serviceProvider?.runCommand?.("admin", { hello: 1 }).catch(() => {});
+                void this.pingAndForget(serviceProvider);
 
                 return this.changeState("connection-requested", {
                     tag: "connecting",
@@ -207,9 +205,7 @@ export class ConnectionManager extends EventEmitter<ConnectionManagerEvents> {
 
     private onOidcAuthFailed(error: unknown): void {
         if (this.state.tag === "connecting" && this.state.connectionStringAuthType?.startsWith("oidc")) {
-            void this.disconnect().then(() => {
-                this.changeState("connection-errored", { tag: "errored", errorReason: String(error) });
-            });
+            void this.disconnectOnOidcError(error);
         }
     }
 
@@ -279,31 +275,23 @@ export class ConnectionManager extends EventEmitter<ConnectionManagerEvents> {
         }
     }
 
-    static async waitUntil<T extends ConnectionState>(
-        tag: T["tag"],
-        cm: ConnectionManager,
-        signal: AbortSignal,
-        additionalCondition?: (state: T) => boolean
-    ): Promise<T> {
-        let ts: NodeJS.Timeout | undefined;
-
-        return new Promise<T>((resolve, reject) => {
-            ts = setInterval(() => {
-                if (signal.aborted) {
-                    return reject(new Error(`Aborted: ${signal.reason}`));
-                }
+    private async pingAndForget(serviceProvider: NodeDriverServiceProvider): Promise<void> {
+        try {
+            await serviceProvider?.runCommand?.("admin", { hello: 1 }).catch(() => {});
+        } catch (error: unknown) {
+            // we really don't care about this error
+            void error;
+        }
+    }
 
-                const status = cm.currentConnectionState;
-                if (status.tag === tag) {
-                    if (!additionalCondition || (additionalCondition && additionalCondition(status as T))) {
-                        return resolve(status as T);
-                    }
-                }
-            }, 100);
-        }).finally(() => {
-            if (ts !== undefined) {
-                clearInterval(ts);
-            }
-        });
+    private async disconnectOnOidcError(error: unknown): Promise<void> {
+        try {
+            await this.disconnect();
+        } catch (error: unknown) {
+            // we really don't care about this error
+            void error;
+        } finally {
+            this.changeState("connection-errored", { tag: "errored", errorReason: String(error) });
+        }
     }
 }

tests/integration/common/connectionManager.oidc.test.ts

@@ -7,12 +7,8 @@ import {
     getServerVersion,
     MongoDBIntegrationTestCase,
 } from "../tools/mongodb/mongodbHelpers.js";
-import { defaultTestConfig, responseAsText, timeout } from "../helpers.js";
-import {
-    ConnectionManager,
-    ConnectionStateConnected,
-    ConnectionStateConnecting,
-} from "../../../src/common/connectionManager.js";
+import { defaultTestConfig, responseAsText, timeout, waitUntil } from "../helpers.js";
+import { ConnectionStateConnected, ConnectionStateConnecting } from "../../../src/common/connectionManager.js";
 import { setupDriverConfig, UserConfig } from "../../../src/common/config.js";
 import path from "path";
 import type { OIDCMockProviderConfig } from "@mongodb-js/oidc-mock-provider";
@@ -161,7 +157,7 @@ describe.skipIf(process.platform !== "linux")("ConnectionManager OIDC Tests", as
     for (const { version, nonce } of baseTestMatrix) {
         describeOidcTest(version, `auth-flow;nonce=${nonce}`, { additionalConfig: { oidcNoNonce: !nonce } }, (it) => {
             it("can connect with the expected user", async ({ signal }, integration) => {
-                const state = await ConnectionManager.waitUntil<ConnectionStateConnected>(
+                const state = await waitUntil<ConnectionStateConnected>(
                     "connected",
                     integration.mcpServer().session.connectionManager,
                     signal
@@ -186,7 +182,7 @@ describe.skipIf(process.platform !== "linux")("ConnectionManager OIDC Tests", as
             });
 
             it("can list existing databases", async ({ signal }, integration) => {
-                const state = await ConnectionManager.waitUntil<ConnectionStateConnected>(
+                const state = await waitUntil<ConnectionStateConnected>(
                     "connected",
                     integration.mcpServer().session.connectionManager,
                     signal
@@ -198,7 +194,7 @@ describe.skipIf(process.platform !== "linux")("ConnectionManager OIDC Tests", as
             });
 
             it("can refresh a token once expired", async ({ signal }, integration) => {
-                const state = await ConnectionManager.waitUntil<ConnectionStateConnected>(
+                const state = await waitUntil<ConnectionStateConnected>(
                     "connected",
                     integration.mcpServer().session.connectionManager,
                     signal
@@ -221,7 +217,7 @@ describe.skipIf(process.platform !== "linux")("ConnectionManager OIDC Tests", as
             { additionalConfig: { oidcFlows: "device-auth", browser: false } },
             (it) => {
                 it("gets requested by the agent to connect", async ({ signal }, integration) => {
-                    const state = await ConnectionManager.waitUntil<ConnectionStateConnecting>(
+                    const state = await waitUntil<ConnectionStateConnecting>(
                         "connecting",
                         integration.mcpServer().session.connectionManager,
                         signal,
@@ -236,7 +232,7 @@ describe.skipIf(process.platform !== "linux")("ConnectionManager OIDC Tests", as
                     expect(response).toContain(state.oidcLoginUrl);
                     expect(response).toContain(state.oidcUserCode);
 
-                    await ConnectionManager.waitUntil<ConnectionStateConnected>(
+                    await waitUntil<ConnectionStateConnected>(
                         "connected",
                         integration.mcpServer().session.connectionManager,
                         signal

tests/integration/helpers.ts

@@ -8,7 +8,7 @@ import { Session } from "../../src/common/session.js";
 import { Telemetry } from "../../src/telemetry/telemetry.js";
 import { config, driverOptions } from "../../src/common/config.js";
 import { afterAll, afterEach, beforeAll, describe, expect, it, vi } from "vitest";
-import { ConnectionManager } from "../../src/common/connectionManager.js";
+import { ConnectionManager, ConnectionState } from "../../src/common/connectionManager.js";
 import { CompositeLogger } from "../../src/common/logger.js";
 import { ExportsManager } from "../../src/common/exportsManager.js";
 
@@ -304,3 +304,31 @@ export function resourceChangedNotification(client: Client, uri: string): Promis
 export function responseAsText(response: Awaited<ReturnType<Client["callTool"]>>): string {
     return JSON.stringify(response.content, undefined, 2);
 }
+
+export function waitUntil<T extends ConnectionState>(
+    tag: T["tag"],
+    cm: ConnectionManager,
+    signal: AbortSignal,
+    additionalCondition?: (state: T) => boolean
+): Promise<T> {
+    let ts: NodeJS.Timeout | undefined;
+
+    return new Promise<T>((resolve, reject) => {
+        ts = setInterval(() => {
+            if (signal.aborted) {
+                return reject(new Error(`Aborted: ${signal.reason}`));
+            }
+
+            const status = cm.currentConnectionState;
+            if (status.tag === tag) {
+                if (!additionalCondition || (additionalCondition && additionalCondition(status as T))) {
+                    return resolve(status as T);
+                }
+            }
+        }, 100);
+    }).finally(() => {
+        if (ts !== undefined) {
+            clearInterval(ts);
+        }
+    });
+}