fix(mcp): harden connection recovery

.changeset/mcp-reconnect-hardening.md

@@ -0,0 +1,25 @@
+---
+"agents": patch
+---
+
+MCP client: reconnect hardening — retry failed connections, settle in-flight restores before id migration, close replaced connections
+
+- The per-server `retry` options (and their defaults) now apply to actual
+  connection failures. `connectToServer` reports a failed connect as a
+  resolved value rather than a throw, so the retry wrapper never engaged and a
+  single transient error during hibernation restore or post-OAuth reconnect
+  left the connection failed for the lifetime of the Durable Object. Failed
+  attempts now retry with the configured backoff before settling; connections
+  parked on OAuth (`authenticating`) are not retried.
+- `migrateServerId` now drains in-flight connection work under the old id
+  before renaming, including work registered while an earlier task settles.
+  Previously, adopting a stable id via `addMcpServer(name, url, { id })` on
+  the same wake could rename the connection out from under the restore's
+  discovery step, leaving the server connected but with no tools until the
+  next wake.
+- `connect()` now closes an existing connection before replacing it, instead
+  of dropping it from the connection map with its transport (and any
+  server-side session) still open. Closing terminates the server-side
+  session, so the persisted session id is cleared too — the replacement (and
+  any later restore) starts with a fresh initialize instead of resuming the
+  terminated session.

packages/agents/src/mcp/client.ts

@@ -287,6 +287,17 @@ export type MCPConnectionResult =
       state: typeof MCPConnectionState.CONNECTED;
     };
 
+/**
+ * Thrown internally to make `tryN` retry a connection that failed. Carries the
+ * failure result so it can be returned once attempts are exhausted. Never
+ * escapes {@link MCPClientManager._connectWithRetry}.
+ */
+class ConnectRetryError extends Error {
+  constructor(readonly result: MCPConnectionResult) {
+    super("MCP connection attempt failed");
+  }
+}
+
 /**
  * Result of discovering server capabilities.
  * success indicates whether discovery completed successfully.
@@ -464,6 +475,18 @@ export class MCPClientManager {
   ): Promise<void> {
     if (oldId === newId) return;
 
+    // Connection work closes over the id it starts with
+    // (connectToServer(oldId) → discoverIfConnected(oldId)). Drain it before
+    // the id moves, including work registered while an earlier task settles,
+    // or discovery can land on a key that no longer exists. Tracked
+    // establishConnection promises may reject; their failure must not abort
+    // the rename.
+    while (true) {
+      const pending = this._pendingConnections.get(oldId);
+      if (!pending) break;
+      await pending.catch(() => {});
+    }
+
     const existing = this.sql<MCPServerRow>(
       "SELECT id FROM cf_agents_mcp_servers WHERE id = ?",
       oldId
@@ -1015,6 +1038,44 @@ export class MCPClientManager {
     }
   }
 
+  /**
+   * Connect to a server, retrying transient failures with backoff.
+   *
+   * `connectToServer` reports a connection failure as a resolved
+   * `{ state: FAILED }` value, but `tryN` only retries thrown errors — so
+   * without re-throwing, the configured `retry` options never engage for the
+   * failure they exist to cover. Throw on FAILED to drive the backoff, then
+   * return the last failure result once attempts are exhausted (an
+   * AUTHENTICATING result resolves immediately and is not retried).
+   */
+  private async _connectWithRetry(
+    serverId: string,
+    retry?: RetryOptions
+  ): Promise<MCPConnectionResult> {
+    const maxAttempts = retry?.maxAttempts ?? 3;
+    const baseDelayMs = retry?.baseDelayMs ?? 500;
+    const maxDelayMs = retry?.maxDelayMs ?? 5000;
+
+    try {
+      return await tryN(
+        maxAttempts,
+        async () => {
+          const result = await this.connectToServer(serverId);
+          if (result.state === MCPConnectionState.FAILED) {
+            throw new ConnectRetryError(result);
+          }
+          return result;
+        },
+        { baseDelayMs, maxDelayMs }
+      );
+    } catch (error) {
+      if (error instanceof ConnectRetryError) {
+        return error.result;
+      }
+      throw error;
+    }
+  }
+
   /**
    * Internal method to restore a single server connection and discovery
    */
@@ -1026,21 +1087,12 @@ export class MCPClientManager {
     // If stored OAuth tokens are valid, connection will succeed automatically
     // If tokens are missing/invalid, connection will fail with Unauthorized
     // and state will be set to "authenticating"
-    const maxAttempts = retry?.maxAttempts ?? 3;
-    const baseDelayMs = retry?.baseDelayMs ?? 500;
-    const maxDelayMs = retry?.maxDelayMs ?? 5000;
-
-    const connectResult = await tryN(
-      maxAttempts,
-      async () => this.connectToServer(serverId),
-      { baseDelayMs, maxDelayMs }
-    ).catch((error) => {
-      console.error(
-        `Error connecting to ${serverId} after ${maxAttempts} attempts:`,
-        error
-      );
-      return null;
-    });
+    const connectResult = await this._connectWithRetry(serverId, retry).catch(
+      (error) => {
+        console.error(`Error connecting to ${serverId}:`, error);
+        return null;
+      }
+    );
 
     if (connectResult?.state === MCPConnectionState.CONNECTED) {
       const discoverResult = await this.discoverIfConnected(serverId);
@@ -1099,7 +1151,17 @@ export class MCPClientManager {
     // During OAuth reconnect, reuse existing connection to preserve state;
     // otherwise drop any existing connection and rebuild it.
     if (!options.reconnect?.oauthCode || !this.mcpConnections[id]) {
-      delete this.mcpConnections[id];
+      const replaced = this.mcpConnections[id];
+      if (replaced) {
+        delete this.mcpConnections[id];
+        // Close in the background — once the map entry is gone nothing else
+        // can ever close the replaced transport or its server-side session.
+        void replaced.close().catch(() => {});
+        // close() terminates the session server-side, so the persisted
+        // sessionId is now dead — clear it or the next restore resumes a
+        // session the server no longer knows.
+        this.updateStoredSessionId(id, undefined);
+      }
       this.createConnection(id, url, {
         client: options.client,
         transport: options.transport ?? {}
@@ -1645,15 +1707,7 @@ export class MCPClientManager {
     }
 
     const retry = this.getServerRetryOptions(serverId);
-    const maxAttempts = retry?.maxAttempts ?? 3;
-    const baseDelayMs = retry?.baseDelayMs ?? 500;
-    const maxDelayMs = retry?.maxDelayMs ?? 5000;
-
-    const connectResult = await tryN(
-      maxAttempts,
-      async () => this.connectToServer(serverId),
-      { baseDelayMs, maxDelayMs }
-    );
+    const connectResult = await this._connectWithRetry(serverId, retry);
     this._onServerStateChanged.fire();
 
     if (connectResult.state === MCPConnectionState.CONNECTED) {

packages/agents/src/tests/mcp/client-manager.test.ts

@@ -2569,7 +2569,10 @@ describe("MCPClientManager OAuth Integration", () => {
         name: "Test Server",
         callbackUrl: "http://localhost:3000/callback",
         client: {},
-        transport: { type: "auto" }
+        transport: { type: "auto" },
+        // A failed connect is retried, but the init mock below only resolves
+        // once — pin a single attempt so the failure settles immediately.
+        retry: { maxAttempts: 1 }
       });
 
       let resolveInit!: () => void;