🚏 feat: Rate Limit Error handling in MCPConnection (#10921)

* 🚏 feat: Rate Limit Error handling in MCPConnection

* chore: Added detailed logging for rate limit scenarios to improve debugging.
- Updated comments to clarify the behavior during rate limiting and reconnection attempts.
- Ensured that the connection state is properly managed when encountering rate limit errors.

* fix: Enhance error handling for rate limiting in MCPConnection

- Updated comments to clarify the rationale for throwing errors in the connectClient() method during rate limit scenarios.
- Ensured consistency in error handling between public API and internal methods.

packages/api/src/mcp/__tests__/MCPConnection.test.ts

@@ -0,0 +1,178 @@
+/**
+ * Tests for MCPConnection error detection methods.
+ *
+ * These tests use standalone implementations that mirror the private methods in MCPConnection.
+ * This approach was chosen because MCPConnection requires complex dependencies (Client, transport)
+ * that are difficult to mock properly. The standalone implementations are kept in sync with
+ * the actual implementation in connection.ts.
+ *
+ * Alternative approaches considered:
+ * 1. Reflection/type casting - fragile and breaks with refactoring
+ * 2. Protected methods with test subclass - changes public API for testing
+ * 3. Integration tests - tested separately in the full MCP test suite
+ */
+describe('MCPConnection Error Detection', () => {
+  /**
+   * Standalone implementation of isRateLimitError for testing.
+   * This mirrors the private method in MCPConnection (connection.ts).
+   * Keep in sync with the actual implementation.
+   */
+  function isRateLimitError(error: unknown): boolean {
+    if (!error || typeof error !== 'object') {
+      return false;
+    }
+
+    // Check for error code
+    if ('code' in error) {
+      const code = (error as { code?: number }).code;
+      if (code === 429) {
+        return true;
+      }
+    }
+
+    // Check message for rate limit indicators
+    if ('message' in error && typeof error.message === 'string') {
+      const message = error.message.toLowerCase();
+      if (
+        message.includes('429') ||
+        message.includes('rate limit') ||
+        message.includes('too many requests')
+      ) {
+        return true;
+      }
+    }
+
+    return false;
+  }
+
+  /**
+   * Standalone implementation of isOAuthError for testing.
+   * This mirrors the private method in MCPConnection (connection.ts).
+   * Keep in sync with the actual implementation.
+   */
+  function isOAuthError(error: unknown): boolean {
+    if (!error || typeof error !== 'object') {
+      return false;
+    }
+
+    // Check for error code
+    if ('code' in error) {
+      const code = (error as { code?: number }).code;
+      if (code === 401 || code === 403) {
+        return true;
+      }
+    }
+
+    // Check message for various auth error indicators
+    if ('message' in error && typeof error.message === 'string') {
+      const message = error.message.toLowerCase();
+      // Check for 401 status
+      if (message.includes('401') || message.includes('non-200 status code (401)')) {
+        return true;
+      }
+      // Check for invalid_token (OAuth servers return this for expired/revoked tokens)
+      if (message.includes('invalid_token')) {
+        return true;
+      }
+      // Check for authentication required
+      if (message.includes('authentication required') || message.includes('unauthorized')) {
+        return true;
+      }
+    }
+
+    return false;
+  }
+
+  describe('isRateLimitError', () => {
+    it('should detect rate limit error by code 429', () => {
+      const error = { code: 429, message: 'Too many requests' };
+      expect(isRateLimitError(error)).toBe(true);
+    });
+
+    it('should detect rate limit error by message containing 429', () => {
+      const error = { message: 'Error POSTing to endpoint (HTTP 429): Too many requests' };
+      expect(isRateLimitError(error)).toBe(true);
+    });
+
+    it('should detect rate limit error by message containing "rate limit"', () => {
+      const error = { message: 'Rate limit exceeded, please try again later' };
+      expect(isRateLimitError(error)).toBe(true);
+    });
+
+    it('should detect rate limit error by message containing "too many requests"', () => {
+      const error = { message: 'Too many requests - slow down!' };
+      expect(isRateLimitError(error)).toBe(true);
+    });
+
+    it('should not detect rate limit for 401 errors', () => {
+      const error = { code: 401, message: 'Unauthorized' };
+      expect(isRateLimitError(error)).toBe(false);
+    });
+
+    it('should not detect rate limit for 500 errors', () => {
+      const error = { code: 500, message: 'Internal server error' };
+      expect(isRateLimitError(error)).toBe(false);
+    });
+
+    it('should not detect rate limit for null/undefined', () => {
+      expect(isRateLimitError(null)).toBe(false);
+      expect(isRateLimitError(undefined)).toBe(false);
+    });
+
+    it('should not detect rate limit for non-object errors', () => {
+      expect(isRateLimitError('string error')).toBe(false);
+      expect(isRateLimitError(123)).toBe(false);
+    });
+
+    it('should handle real-world StackOverflow rate limit error', () => {
+      const error = {
+        code: 429,
+        message:
+          'Streamable HTTP error: Error POSTing to endpoint: <!DOCTYPE html><html>Too Many Requests</html>',
+      };
+      expect(isRateLimitError(error)).toBe(true);
+    });
+  });
+
+  describe('isOAuthError', () => {
+    it('should detect OAuth error by code 401', () => {
+      const error = { code: 401, message: 'Unauthorized' };
+      expect(isOAuthError(error)).toBe(true);
+    });
+
+    it('should detect OAuth error by code 403', () => {
+      const error = { code: 403, message: 'Forbidden' };
+      expect(isOAuthError(error)).toBe(true);
+    });
+
+    it('should detect OAuth error by message containing 401', () => {
+      const error = { message: 'Error POSTing to endpoint (HTTP 401): Unauthorized' };
+      expect(isOAuthError(error)).toBe(true);
+    });
+
+    it('should not detect OAuth error for 429 rate limit', () => {
+      const error = { code: 429, message: 'Too many requests' };
+      expect(isOAuthError(error)).toBe(false);
+    });
+
+    it('should detect OAuth error for invalid_token', () => {
+      const error = { message: 'The access token is invalid_token or expired' };
+      expect(isOAuthError(error)).toBe(true);
+    });
+  });
+
+  describe('error type differentiation', () => {
+    it('should correctly differentiate between rate limit and OAuth errors', () => {
+      const rateLimitError = { code: 429, message: 'Too many requests' };
+      const oauthError = { code: 401, message: 'Unauthorized' };
+
+      // Rate limit error should be detected as rate limit, not OAuth
+      expect(isRateLimitError(rateLimitError)).toBe(true);
+      expect(isOAuthError(rateLimitError)).toBe(false);
+
+      // OAuth error should be detected as OAuth, not rate limit
+      expect(isOAuthError(oauthError)).toBe(true);
+      expect(isRateLimitError(oauthError)).toBe(false);
+    });
+  });
+});

packages/api/src/mcp/connection.ts

@@ -415,6 +415,24 @@ export class MCPConnection extends EventEmitter {
         } catch (error) {
           logger.error(`${this.getLogPrefix()} Reconnection attempt failed:`, error);
 
+          // Stop immediately if rate limited - retrying will only make it worse
+          if (this.isRateLimitError(error)) {
+            /**
+             * Rate limiting sets shouldStopReconnecting to prevent hammering the server.
+             * Silent return here (vs throw in connectClient) because we're already in
+             * error recovery mode - throwing would just add noise. The connection
+             * must be recreated to retry after rate limit lifts.
+             */
+            logger.warn(
+              `${this.getLogPrefix()} Rate limited (429), stopping reconnection attempts`,
+            );
+            logger.debug(
+              `${this.getLogPrefix()} Rate limit block is permanent for this connection instance`,
+            );
+            this.shouldStopReconnecting = true;
+            return;
+          }
+
           if (
             this.reconnectAttempts === this.MAX_RECONNECT_ATTEMPTS ||
             (this.shouldStopReconnecting as boolean)
@@ -475,6 +493,25 @@ export class MCPConnection extends EventEmitter {
         this.emit('connectionChange', 'connected');
         this.reconnectAttempts = 0;
       } catch (error) {
+        // Check if it's a rate limit error - stop immediately to avoid making it worse
+        if (this.isRateLimitError(error)) {
+          /**
+           * Rate limiting sets shouldStopReconnecting to prevent hammering the server.
+           * This is a permanent block for this connection instance - the connection
+           * must be recreated (e.g., by user re-initiating) to retry after rate limit lifts.
+           *
+           * We throw here (unlike handleReconnection which returns silently) because:
+           * - connectClient() is a public API - callers expect async errors to throw
+           * - Other errors in this catch block also throw for consistency
+           * - handleReconnection is private/internal error recovery, different context
+           */
+          logger.warn(`${this.getLogPrefix()} Rate limited (429), stopping connection attempts`);
+          this.shouldStopReconnecting = true;
+          this.connectionState = 'error';
+          this.emit('connectionChange', 'error');
+          throw error;
+        }
+
         // Check if it's an OAuth authentication error
         if (this.isOAuthError(error)) {
           logger.warn(`${this.getLogPrefix()} OAuth authentication required`);
@@ -786,4 +823,36 @@ export class MCPConnection extends EventEmitter {
 
     return false;
   }
+
+  /**
+   * Checks if an error indicates rate limiting (HTTP 429).
+   * Rate limited requests should stop reconnection attempts to avoid making the situation worse.
+   */
+  private isRateLimitError(error: unknown): boolean {
+    if (!error || typeof error !== 'object') {
+      return false;
+    }
+
+    // Check for error code
+    if ('code' in error) {
+      const code = (error as { code?: number }).code;
+      if (code === 429) {
+        return true;
+      }
+    }
+
+    // Check message for rate limit indicators
+    if ('message' in error && typeof error.message === 'string') {
+      const message = error.message.toLowerCase();
+      if (
+        message.includes('429') ||
+        message.includes('rate limit') ||
+        message.includes('too many requests')
+      ) {
+        return true;
+      }
+    }
+
+    return false;
+  }
 }