🔧 fix: Fix rampant pings and rate limiting

- Skips idle connection checks in `getMCPManager` to avoid unnecessary pings.
- Introduces a skipOAuthTimeout flag during initial connection to prevent timeouts during server discovery.
- Uses a lightweight connection state check instead of ping to avoid rate limits.
- Prevents refetch spam and rate limit errors when checking connection status.
- Fixes an issue where the server connection was not being disconnected.
This commit is contained in:
Dustin Healy 2025-07-21 12:53:28 -07:00
parent 1da5365397
commit 10e06f2221
9 changed files with 90 additions and 39 deletions

View file

@ -11,12 +11,13 @@ let flowManager = null;
/** /**
* @param {string} [userId] - Optional user ID, to avoid disconnecting the current user. * @param {string} [userId] - Optional user ID, to avoid disconnecting the current user.
* @param {boolean} [skipIdleCheck] - Skip idle connection checking to avoid unnecessary pings.
* @returns {MCPManager} * @returns {MCPManager}
*/ */
function getMCPManager(userId) { function getMCPManager(userId, skipIdleCheck = false) {
if (!mcpManager) { if (!mcpManager) {
mcpManager = MCPManager.getInstance(); mcpManager = MCPManager.getInstance();
} else { } else if (!skipIdleCheck) {
mcpManager.checkIdleConnections(userId); mcpManager.checkIdleConnections(userId);
} }
return mcpManager; return mcpManager;

View file

@ -97,7 +97,6 @@ function createServerToolsCallback() {
return; return;
} }
await mcpToolsCache.set(serverName, serverTools); await mcpToolsCache.set(serverName, serverTools);
logger.warn(`MCP tools for ${serverName} added to cache.`);
} catch (error) { } catch (error) {
logger.error('Error retrieving MCP tools from cache:', error); logger.error('Error retrieving MCP tools from cache:', error);
} }
@ -240,7 +239,6 @@ const getAvailableTools = async (req, res) => {
// Check if the app-level connection is active // Check if the app-level connection is active
try { try {
const mcpManager = getMCPManager(); const mcpManager = getMCPManager();
const allConnections = mcpManager.getAllConnections();
const appConnection = mcpManager.getConnection(serverName); const appConnection = mcpManager.getConnection(serverName);

View file

@ -182,7 +182,7 @@ const updateUserPluginsController = async (req, res) => {
`[updateUserPluginsController] Disconnecting MCP connection for user ${user.id} and server ${serverName} after plugin auth update for ${pluginKey}.`, `[updateUserPluginsController] Disconnecting MCP connection for user ${user.id} and server ${serverName} after plugin auth update for ${pluginKey}.`,
); );
// Don't kill the server connection on revoke anymore, user can just reinitialize the server if thats what they want // Don't kill the server connection on revoke anymore, user can just reinitialize the server if thats what they want
// await mcpManager.disconnectUserConnection(user.id, serverName); await mcpManager.disconnectUserConnection(user.id, serverName);
} }
} catch (disconnectError) { } catch (disconnectError) {
logger.error( logger.error(

View file

@ -219,7 +219,7 @@ router.get('/connection/status', requireJwtAuth, async (req, res) => {
return res.status(401).json({ error: 'User not authenticated' }); return res.status(401).json({ error: 'User not authenticated' });
} }
const mcpManager = getMCPManager(); const mcpManager = getMCPManager(null, true); // Skip idle checks to avoid ping spam
const connectionStatus = {}; const connectionStatus = {};
// Get all MCP server names from custom config // Get all MCP server names from custom config
@ -237,12 +237,14 @@ router.get('/connection/status', requireJwtAuth, async (req, res) => {
const userConnection = mcpManager.getUserConnectionIfExists(user.id, serverName); const userConnection = mcpManager.getUserConnectionIfExists(user.id, serverName);
const hasUserConnection = !!userConnection; const hasUserConnection = !!userConnection;
// Determine if connected based on actual connection state // Use lightweight connection state check instead of ping-based isConnected()
let connected = false; let connected = false;
if (hasAppConnection) { if (hasAppConnection) {
connected = await appConnection.isConnected(); // Check connection state without ping to avoid rate limits
connected = appConnection.connectionState === 'connected';
} else if (hasUserConnection) { } else if (hasUserConnection) {
connected = await userConnection.isConnected(); // Check connection state without ping to avoid rate limits
connected = userConnection.connectionState === 'connected';
} }
// Determine if this server requires user authentication // Determine if this server requires user authentication

View file

@ -59,6 +59,8 @@ export default function MCPPanel() {
await Promise.all([ await Promise.all([
queryClient.invalidateQueries([QueryKeys.tools]), queryClient.invalidateQueries([QueryKeys.tools]),
queryClient.refetchQueries([QueryKeys.tools]), queryClient.refetchQueries([QueryKeys.tools]),
queryClient.invalidateQueries([QueryKeys.mcpAuthValues]),
queryClient.refetchQueries([QueryKeys.mcpAuthValues]),
queryClient.invalidateQueries([QueryKeys.mcpConnectionStatus]), queryClient.invalidateQueries([QueryKeys.mcpConnectionStatus]),
queryClient.refetchQueries([QueryKeys.mcpConnectionStatus]), queryClient.refetchQueries([QueryKeys.mcpConnectionStatus]),
]); ]);

View file

@ -45,13 +45,8 @@ export default function MCPConfigDialog({
}: MCPConfigDialogProps) { }: MCPConfigDialogProps) {
const localize = useLocalize(); const localize = useLocalize();
// Get connection status to determine OAuth requirements with aggressive refresh // Get connection status to determine OAuth requirements
const { data: statusQuery } = useMCPConnectionStatusQuery({ const { data: statusQuery } = useMCPConnectionStatusQuery();
refetchOnMount: true,
refetchOnWindowFocus: true,
staleTime: 0,
cacheTime: 0,
});
const mcpServerStatuses = statusQuery?.connectionStatus || {}; const mcpServerStatuses = statusQuery?.connectionStatus || {};
// Derive real-time connection status and OAuth requirements // Derive real-time connection status and OAuth requirements

View file

@ -51,9 +51,10 @@ export const useMCPConnectionStatusQuery = <TData = t.TMCPConnectionStatusRespon
[QueryKeys.mcpConnectionStatus], [QueryKeys.mcpConnectionStatus],
() => dataService.getMCPConnectionStatus(), () => dataService.getMCPConnectionStatus(),
{ {
// refetchOnWindowFocus: false, refetchOnWindowFocus: false, // Stop window focus spam
// refetchOnReconnect: false, refetchOnReconnect: false, // Stop reconnect spam
// refetchOnMount: true, refetchOnMount: false, // Only fetch when explicitly needed
staleTime: 2000, // 2 second cache to prevent excessive calls
...config, ...config,
}, },
); );

View file

@ -57,7 +57,7 @@ export class MCPConnection extends EventEmitter {
private static instance: MCPConnection | null = null; private static instance: MCPConnection | null = null;
public client: Client; public client: Client;
private transport: Transport | null = null; // Make this nullable private transport: Transport | null = null; // Make this nullable
private connectionState: t.ConnectionState = 'disconnected'; public connectionState: t.ConnectionState = 'disconnected';
private connectPromise: Promise<void> | null = null; private connectPromise: Promise<void> | null = null;
private readonly MAX_RECONNECT_ATTEMPTS = 3; private readonly MAX_RECONNECT_ATTEMPTS = 3;
public readonly serverName: string; public readonly serverName: string;
@ -70,6 +70,7 @@ export class MCPConnection extends EventEmitter {
private oauthTokens?: MCPOAuthTokens | null; private oauthTokens?: MCPOAuthTokens | null;
private oauthRequired = false; private oauthRequired = false;
private oauthTimeoutId: NodeJS.Timeout | null = null; private oauthTimeoutId: NodeJS.Timeout | null = null;
private skipOAuthTimeout = false; // Skip OAuth timeouts during discovery
iconPath?: string; iconPath?: string;
timeout?: number; timeout?: number;
url?: string; url?: string;
@ -79,6 +80,7 @@ export class MCPConnection extends EventEmitter {
private readonly options: t.MCPOptions, private readonly options: t.MCPOptions,
userId?: string, userId?: string,
oauthTokens?: MCPOAuthTokens | null, oauthTokens?: MCPOAuthTokens | null,
skipOAuthTimeout = false, // Skip OAuth timeouts during discovery
) { ) {
super(); super();
this.serverName = serverName; this.serverName = serverName;
@ -86,6 +88,7 @@ export class MCPConnection extends EventEmitter {
this.iconPath = options.iconPath; this.iconPath = options.iconPath;
this.timeout = options.timeout; this.timeout = options.timeout;
this.lastPingTime = Date.now(); this.lastPingTime = Date.now();
this.skipOAuthTimeout = skipOAuthTimeout;
if (oauthTokens) { if (oauthTokens) {
this.oauthTokens = oauthTokens; this.oauthTokens = oauthTokens;
} }
@ -208,10 +211,7 @@ export class MCPConnection extends EventEmitter {
this.emit('connectionChange', 'disconnected'); this.emit('connectionChange', 'disconnected');
}; };
transport.onerror = (error) => { // Error handling is done by setupTransportErrorHandlers
logger.error(`${this.getLogPrefix()} SSE transport error:`, error);
this.emitError(error, 'SSE transport error:');
};
transport.onmessage = (message) => { transport.onmessage = (message) => {
logger.info(`${this.getLogPrefix()} Message received: ${JSON.stringify(message)}`); logger.info(`${this.getLogPrefix()} Message received: ${JSON.stringify(message)}`);
@ -250,10 +250,7 @@ export class MCPConnection extends EventEmitter {
this.emit('connectionChange', 'disconnected'); this.emit('connectionChange', 'disconnected');
}; };
transport.onerror = (error: Error | unknown) => { // Error handling is done by setupTransportErrorHandlers
logger.error(`${this.getLogPrefix()} Streamable-http transport error:`, error);
this.emitError(error, 'Streamable-http transport error:');
};
transport.onmessage = (message: JSONRPCMessage) => { transport.onmessage = (message: JSONRPCMessage) => {
logger.info(`${this.getLogPrefix()} Message received: ${JSON.stringify(message)}`); logger.info(`${this.getLogPrefix()} Message received: ${JSON.stringify(message)}`);
@ -411,6 +408,24 @@ export class MCPConnection extends EventEmitter {
const serverUrl = this.url; const serverUrl = this.url;
logger.debug(`${this.getLogPrefix()} Server URL for OAuth: ${serverUrl}`); logger.debug(`${this.getLogPrefix()} Server URL for OAuth: ${serverUrl}`);
// In startup mode, immediately emit oauthRequired and fail without timeout
if (this.skipOAuthTimeout) {
logger.info(
`${this.getLogPrefix()} Skip OAuth timeout: OAuth required, failing immediately without timeout`,
);
// Emit the event for discovery purposes
this.emit('oauthRequired', {
serverName: this.serverName,
error,
serverUrl,
userId: this.userId,
});
// Immediately throw to avoid timeout
throw error;
}
const oauthTimeout = this.options.initTimeout ?? 60000; const oauthTimeout = this.options.initTimeout ?? 60000;
/** Promise that will resolve when OAuth is handled */ /** Promise that will resolve when OAuth is handled */
const oauthHandledPromise = new Promise<void>((resolve, reject) => { const oauthHandledPromise = new Promise<void>((resolve, reject) => {
@ -545,6 +560,12 @@ export class MCPConnection extends EventEmitter {
private setupTransportErrorHandlers(transport: Transport): void { private setupTransportErrorHandlers(transport: Transport): void {
transport.onerror = (error) => { transport.onerror = (error) => {
// Suppress error logging if we're already disconnected or disconnecting
if (this.connectionState === 'disconnected' || this.shouldStopReconnecting) {
logger.debug(`${this.getLogPrefix()} Transport error during disconnect (expected):`, error);
return;
}
logger.error(`${this.getLogPrefix()} Transport error:`, error); logger.error(`${this.getLogPrefix()} Transport error:`, error);
// Check if it's an OAuth authentication error // Check if it's an OAuth authentication error
@ -556,6 +577,15 @@ export class MCPConnection extends EventEmitter {
} }
} }
// Check if it's a rate limit error (429) - don't trigger reconnection
const errorMessage = error?.toString() || '';
if (errorMessage.includes('429') || errorMessage.includes('too many requests')) {
logger.warn(
`${this.getLogPrefix()} Rate limit error detected, not triggering reconnection`,
);
return; // Don't emit error state for rate limits
}
this.emit('connectionChange', 'error'); this.emit('connectionChange', 'error');
}; };
} }
@ -593,14 +623,14 @@ export class MCPConnection extends EventEmitter {
this.oauthTimeoutId = null; this.oauthTimeoutId = null;
} }
// Set disconnected state early to suppress error logging during cleanup
this.connectionState = 'disconnected';
if (this.transport) { if (this.transport) {
await this.client.close(); await this.client.close();
this.transport = null; this.transport = null;
} }
if (this.connectionState === 'disconnected') {
return;
}
this.connectionState = 'disconnected';
this.emit('connectionChange', 'disconnected'); this.emit('connectionChange', 'disconnected');
} finally { } finally {
this.connectPromise = null; this.connectPromise = null;
@ -643,6 +673,11 @@ export class MCPConnection extends EventEmitter {
return false; return false;
} }
// Don't ping if we're disconnecting or should stop reconnecting
if (this.shouldStopReconnecting) {
return false;
}
try { try {
// Try ping first as it's the lightest check // Try ping first as it's the lightest check
await this.client.ping(); await this.client.ping();
@ -656,6 +691,13 @@ export class MCPConnection extends EventEmitter {
(error as Error)?.message.includes('method not found')); (error as Error)?.message.includes('method not found'));
if (!pingUnsupported) { if (!pingUnsupported) {
// Check if it's a rate limit error - don't log as error
const errorMessage = (error as Error)?.message || '';
if (errorMessage.includes('429') || errorMessage.includes('too many requests')) {
logger.warn(`${this.getLogPrefix()} Ping rate limited, assuming connected`);
return true; // Assume still connected during rate limiting
}
logger.error(`${this.getLogPrefix()} Ping failed:`, error); logger.error(`${this.getLogPrefix()} Ping failed:`, error);
return false; return false;
} }

View file

@ -160,6 +160,7 @@ export class MCPManager extends EventEmitter {
}): Promise<void> { }): Promise<void> {
const processedConfig = processMCPEnv(config); const processedConfig = processMCPEnv(config);
let tokens: MCPOAuthTokens | null = null; let tokens: MCPOAuthTokens | null = null;
if (tokenMethods?.findToken) { if (tokenMethods?.findToken) {
try { try {
/** Refresh function for app-level connections */ /** Refresh function for app-level connections */
@ -204,10 +205,13 @@ export class MCPManager extends EventEmitter {
logger.debug(`[MCP][${serverName}] No existing tokens found`); logger.debug(`[MCP][${serverName}] No existing tokens found`);
} }
} }
if (tokens) { if (tokens) {
logger.info(`[MCP][${serverName}] Loaded OAuth tokens`); logger.info(`[MCP][${serverName}] Loaded OAuth tokens`);
} }
const connection = new MCPConnection(serverName, processedConfig, undefined, tokens);
// Create connection in startup mode to prevent OAuth timeouts
const connection = new MCPConnection(serverName, processedConfig, undefined, tokens, true);
// Track OAuth skipped state explicitly // Track OAuth skipped state explicitly
let oauthSkipped = false; let oauthSkipped = false;
@ -347,7 +351,8 @@ export class MCPManager extends EventEmitter {
while (attempts < maxAttempts) { while (attempts < maxAttempts) {
try { try {
await connection.connect(); await connection.connect();
if (await connection.isConnected()) { // Use lightweight connection state check instead of ping
if (connection.connectionState === 'connected') {
return; return;
} }
throw new Error('Connection attempt succeeded but status is not connected'); throw new Error('Connection attempt succeeded but status is not connected');
@ -493,7 +498,8 @@ export class MCPManager extends EventEmitter {
} }
connection = undefined; // Force creation of a new connection connection = undefined; // Force creation of a new connection
} else if (connection) { } else if (connection) {
if (await connection.isConnected()) { // Use lightweight connection state check instead of ping
if (connection.connectionState === 'connected') {
logger.debug(`[MCP][User: ${userId}][${serverName}] Reusing active connection`); logger.debug(`[MCP][User: ${userId}][${serverName}] Reusing active connection`);
this.updateUserLastActivity(userId); this.updateUserLastActivity(userId);
return connection; return connection;
@ -636,7 +642,8 @@ export class MCPManager extends EventEmitter {
}); });
await Promise.race([connectionAttempt, connectionTimeout]); await Promise.race([connectionAttempt, connectionTimeout]);
if (!(await connection?.isConnected())) { // Use lightweight connection state check instead of ping
if (connection?.connectionState !== 'connected') {
throw new Error('Failed to establish connection after initialization attempt.'); throw new Error('Failed to establish connection after initialization attempt.');
} }
@ -747,7 +754,8 @@ export class MCPManager extends EventEmitter {
flowManager: FlowStateManager<MCPOAuthTokens | null>; flowManager: FlowStateManager<MCPOAuthTokens | null>;
skipReconnect?: boolean; skipReconnect?: boolean;
}): Promise<boolean> { }): Promise<boolean> {
if (await connection.isConnected()) { // Use lightweight connection state check instead of ping
if (connection.connectionState === 'connected') {
return true; return true;
} }
@ -775,7 +783,8 @@ export class MCPManager extends EventEmitter {
flowManager, flowManager,
}); });
if (await connection.isConnected()) { // Use lightweight connection state check instead of ping
if (connection.connectionState === 'connected') {
logger.info(`[MCP][${serverName}] App-level connection successfully reconnected`); logger.info(`[MCP][${serverName}] App-level connection successfully reconnected`);
return true; return true;
} else { } else {
@ -959,7 +968,8 @@ export class MCPManager extends EventEmitter {
} }
} }
if (!(await connection.isConnected())) { // Use lightweight connection state check instead of ping
if (connection.connectionState !== 'connected') {
/** May happen if getUserConnection failed silently or app connection dropped */ /** May happen if getUserConnection failed silently or app connection dropped */
throw new McpError( throw new McpError(
ErrorCode.InternalError, // Use InternalError for connection issues ErrorCode.InternalError, // Use InternalError for connection issues