mirror of
https://github.com/danny-avila/LibreChat.git
synced 2026-09-07 23:18:26 +00:00
fix: bound and back off passive MCP catalog recovery
Passive recovery runs inline on `GET /api/mcp/tools` and its results are request-local by design, so every list request re-dialed the same cold servers with the default connection timeout. Three limits keep that cost proportional to what recovery can actually recover: - Cap the discovery timeout at 5s instead of inheriting the connection default (`initTimeout ?? 30s`); a server configured to connect faster keeps its own shorter limit. - Skip a server the config tier already marked `inspectionFailed`, leaving it to that tier's retry window rather than re-dialing it per request. - Skip a server whose declared `customUserVars` are unset, matching the gate `reinitMCPServer` applies for issue #10969 — connecting without them fails auth, so the attempt is spent for nothing. Servers that still fail discovery enter a one-minute per-process cooldown, which is what stops an unreachable server from being re-dialed by every subsequent list request. A server that recovers clears its own entry, and expired entries are swept at most once per window so the map stays bounded. Skipped servers render exactly as they did before recovery existed: present in the catalog with an empty tool list. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Xr1Dabvdn1mzzyYgpJgU5B
This commit is contained in:
parent
82208c7fbc
commit
257d5cf3e0
4 changed files with 360 additions and 10 deletions
|
|
@ -5,6 +5,7 @@ const {
|
|||
getMissingCustomUserVars,
|
||||
loadMCPServerCatalogs: loadCatalogs,
|
||||
requiresEphemeralUserConnection,
|
||||
createMCPCatalogRecoveryCooldown,
|
||||
getMissingRuntimeBodyPlaceholderFields,
|
||||
} = require('@librechat/api');
|
||||
const { CacheKeys, Constants } = require('librechat-data-provider');
|
||||
|
|
@ -34,6 +35,10 @@ const MCP_REINITIALIZE_FAILURE_REASONS = {
|
|||
INITIALIZATION_FAILED: 'initialization_failed',
|
||||
};
|
||||
|
||||
/** Recovered catalogs are request-local, so this process-wide cooldown is what keeps an
|
||||
* unreachable server from being re-dialed by every catalog list request. */
|
||||
const recoveryCooldown = createMCPCatalogRecoveryCooldown();
|
||||
|
||||
/** Wires application dependencies into the passive, request-local catalog recovery service. */
|
||||
async function loadMCPServerCatalogs({ user, servers }) {
|
||||
const flowManager = getFlowStateManager(getLogStores(CacheKeys.FLOWS));
|
||||
|
|
@ -62,6 +67,7 @@ async function loadMCPServerCatalogs({ user, servers }) {
|
|||
getServerToolFunctionsSnapshot: (userId, serverName, serverConfig) =>
|
||||
mcpManager.getServerToolFunctionsSnapshot(userId, serverName, serverConfig),
|
||||
cacheServerTools: cacheMCPServerTools,
|
||||
recoveryCooldown,
|
||||
},
|
||||
);
|
||||
}
|
||||
|
|
|
|||
|
|
@ -124,6 +124,28 @@ describe('loadMCPServerCatalogs', () => {
|
|||
serversWithoutTools: [],
|
||||
});
|
||||
});
|
||||
|
||||
it('shares one failure cooldown across requests so a cold server is not re-dialed', async () => {
|
||||
const user = { id: 'user-123' };
|
||||
const servers = [
|
||||
{ serverName: 'offline', serverConfig: { type: 'sse', url: 'https://offline.example/sse' } },
|
||||
];
|
||||
const cooldowns = [];
|
||||
mockLoadCatalogs.mockImplementation(async (_params, deps) => {
|
||||
cooldowns.push(deps.recoveryCooldown);
|
||||
return { serverTools: new Map(), serversWithoutTools: ['offline'] };
|
||||
});
|
||||
|
||||
await loadMCPServerCatalogs({ user, servers });
|
||||
await loadMCPServerCatalogs({ user, servers });
|
||||
|
||||
expect(cooldowns[0]).toBeDefined();
|
||||
expect(cooldowns[1]).toBe(cooldowns[0]);
|
||||
expect(cooldowns[0].isCoolingDown(user.id, 'offline')).toBe(false);
|
||||
cooldowns[0].recordFailure(user.id, 'offline');
|
||||
expect(cooldowns[0].isCoolingDown(user.id, 'offline')).toBe(true);
|
||||
cooldowns[0].recordSuccess(user.id, 'offline');
|
||||
});
|
||||
});
|
||||
|
||||
describe('reinitMCPServer — customUserVars gating (issue #10969)', () => {
|
||||
|
|
|
|||
|
|
@ -1,7 +1,11 @@
|
|||
import { Constants } from 'librechat-data-provider';
|
||||
import type { IUser } from '@librechat/data-schemas';
|
||||
import type { LCAvailableTools, ParsedServerConfig, ToolDiscoveryOptions } from '../types';
|
||||
import { loadMCPServerCatalogs, recoverMCPServerCatalogs } from './recovery';
|
||||
import {
|
||||
loadMCPServerCatalogs,
|
||||
recoverMCPServerCatalogs,
|
||||
createMCPCatalogRecoveryCooldown,
|
||||
} from './recovery';
|
||||
|
||||
const user = { id: 'user-1' } as IUser;
|
||||
const serverConfig = (name: string): ParsedServerConfig =>
|
||||
|
|
@ -217,3 +221,203 @@ describe('loadMCPServerCatalogs', () => {
|
|||
expect(result.serversWithoutTools).toEqual(['missing']);
|
||||
});
|
||||
});
|
||||
|
||||
describe('recoverMCPServerCatalogs — bounded, skippable discovery', () => {
|
||||
const recoveryDeps = (
|
||||
discoverServerTools: jest.Mock,
|
||||
userMCPAuthMap: Record<string, Record<string, string>> = {},
|
||||
) => ({
|
||||
loadUserMCPAuthMap: jest.fn().mockResolvedValue(userMCPAuthMap),
|
||||
discoverServerTools,
|
||||
formatServerTools: jest.fn().mockReturnValue({}),
|
||||
});
|
||||
|
||||
it('caps the discovery timeout so an unreachable server cannot hold the request', async () => {
|
||||
const discoverServerTools = jest.fn().mockResolvedValue({ tools: [] });
|
||||
|
||||
await recoverMCPServerCatalogs(
|
||||
{ user, servers: [{ serverName: 'slow', serverConfig: serverConfig('slow') }] },
|
||||
recoveryDeps(discoverServerTools),
|
||||
);
|
||||
|
||||
expect(discoverServerTools).toHaveBeenCalledWith(
|
||||
expect.objectContaining({ serverName: 'slow', connectionTimeout: 5000 }),
|
||||
);
|
||||
});
|
||||
|
||||
it('keeps a shorter configured initTimeout instead of raising it to the cap', async () => {
|
||||
const discoverServerTools = jest.fn().mockResolvedValue({ tools: [] });
|
||||
const impatient = { ...serverConfig('impatient'), initTimeout: 1500 } as ParsedServerConfig;
|
||||
|
||||
await recoverMCPServerCatalogs(
|
||||
{ user, servers: [{ serverName: 'impatient', serverConfig: impatient }] },
|
||||
recoveryDeps(discoverServerTools),
|
||||
);
|
||||
|
||||
expect(discoverServerTools).toHaveBeenCalledWith(
|
||||
expect.objectContaining({ connectionTimeout: 1500 }),
|
||||
);
|
||||
});
|
||||
|
||||
it('leaves a server the config tier marked unreachable to that tier’s retry window', async () => {
|
||||
const discoverServerTools = jest.fn().mockResolvedValue({ tools: [] });
|
||||
const deps = recoveryDeps(discoverServerTools);
|
||||
const failed = { ...serverConfig('failed'), inspectionFailed: true } as ParsedServerConfig;
|
||||
|
||||
const result = await recoverMCPServerCatalogs(
|
||||
{
|
||||
user,
|
||||
servers: [
|
||||
{ serverName: 'failed', serverConfig: failed },
|
||||
{ serverName: 'healthy', serverConfig: serverConfig('healthy') },
|
||||
],
|
||||
},
|
||||
deps,
|
||||
);
|
||||
|
||||
expect(deps.loadUserMCPAuthMap).toHaveBeenCalledWith('user-1', ['healthy']);
|
||||
expect(discoverServerTools).toHaveBeenCalledTimes(1);
|
||||
expect(discoverServerTools).toHaveBeenCalledWith(
|
||||
expect.objectContaining({ serverName: 'healthy' }),
|
||||
);
|
||||
expect([...result.keys()]).toEqual(['healthy']);
|
||||
});
|
||||
|
||||
it('skips a server whose user-provided variables are unset and recovers its siblings', async () => {
|
||||
const discoverServerTools = jest.fn().mockResolvedValue({ tools: [] });
|
||||
const needsVars = {
|
||||
...serverConfig('needs-vars'),
|
||||
customUserVars: { API_KEY: { title: 'API key', description: 'Server API key' } },
|
||||
} as ParsedServerConfig;
|
||||
|
||||
const result = await recoverMCPServerCatalogs(
|
||||
{
|
||||
user,
|
||||
servers: [
|
||||
{ serverName: 'needs-vars', serverConfig: needsVars },
|
||||
{ serverName: 'open', serverConfig: serverConfig('open') },
|
||||
],
|
||||
},
|
||||
recoveryDeps(discoverServerTools),
|
||||
);
|
||||
|
||||
expect(discoverServerTools).toHaveBeenCalledTimes(1);
|
||||
expect(discoverServerTools).toHaveBeenCalledWith(
|
||||
expect.objectContaining({ serverName: 'open' }),
|
||||
);
|
||||
expect([...result.keys()]).toEqual(['open']);
|
||||
});
|
||||
|
||||
it('discovers a server whose user-provided variables are satisfied', async () => {
|
||||
const discoverServerTools = jest.fn().mockResolvedValue({ tools: [] });
|
||||
const needsVars = {
|
||||
...serverConfig('needs-vars'),
|
||||
customUserVars: { API_KEY: { title: 'API key', description: 'Server API key' } },
|
||||
} as ParsedServerConfig;
|
||||
|
||||
await recoverMCPServerCatalogs(
|
||||
{ user, servers: [{ serverName: 'needs-vars', serverConfig: needsVars }] },
|
||||
recoveryDeps(discoverServerTools, {
|
||||
[`${Constants.mcp_prefix}needs-vars`]: { API_KEY: 'set' },
|
||||
}),
|
||||
);
|
||||
|
||||
expect(discoverServerTools).toHaveBeenCalledWith(
|
||||
expect.objectContaining({
|
||||
serverName: 'needs-vars',
|
||||
customUserVars: { API_KEY: 'set' },
|
||||
}),
|
||||
);
|
||||
});
|
||||
|
||||
it('does not re-dial a server that just failed discovery', async () => {
|
||||
const discoverServerTools = jest
|
||||
.fn()
|
||||
.mockRejectedValueOnce(new Error('offline'))
|
||||
.mockResolvedValue({ tools: [] });
|
||||
const recoveryCooldown = createMCPCatalogRecoveryCooldown();
|
||||
const servers = [{ serverName: 'offline', serverConfig: serverConfig('offline') }];
|
||||
const deps = { ...recoveryDeps(discoverServerTools), recoveryCooldown };
|
||||
|
||||
await recoverMCPServerCatalogs({ user, servers }, deps);
|
||||
const second = await recoverMCPServerCatalogs({ user, servers }, deps);
|
||||
|
||||
expect(discoverServerTools).toHaveBeenCalledTimes(1);
|
||||
expect(second.size).toBe(0);
|
||||
});
|
||||
|
||||
it('keeps re-dialing a server that recovers successfully', async () => {
|
||||
const discoverServerTools = jest.fn().mockResolvedValue({ tools: [] });
|
||||
const recoveryCooldown = createMCPCatalogRecoveryCooldown();
|
||||
const servers = [{ serverName: 'healthy', serverConfig: serverConfig('healthy') }];
|
||||
const deps = { ...recoveryDeps(discoverServerTools), recoveryCooldown };
|
||||
|
||||
await recoverMCPServerCatalogs({ user, servers }, deps);
|
||||
await recoverMCPServerCatalogs({ user, servers }, deps);
|
||||
|
||||
expect(discoverServerTools).toHaveBeenCalledTimes(2);
|
||||
});
|
||||
|
||||
it('skips the auth lookup entirely when every cold server is ineligible', async () => {
|
||||
const discoverServerTools = jest.fn();
|
||||
const deps = recoveryDeps(discoverServerTools);
|
||||
const failed = { ...serverConfig('failed'), inspectionFailed: true } as ParsedServerConfig;
|
||||
|
||||
const result = await recoverMCPServerCatalogs(
|
||||
{ user, servers: [{ serverName: 'failed', serverConfig: failed }] },
|
||||
deps,
|
||||
);
|
||||
|
||||
expect(deps.loadUserMCPAuthMap).not.toHaveBeenCalled();
|
||||
expect(discoverServerTools).not.toHaveBeenCalled();
|
||||
expect(result.size).toBe(0);
|
||||
});
|
||||
});
|
||||
|
||||
describe('createMCPCatalogRecoveryCooldown', () => {
|
||||
it('holds a failure for the cooldown window and releases it afterwards', () => {
|
||||
jest.useFakeTimers();
|
||||
try {
|
||||
const cooldown = createMCPCatalogRecoveryCooldown(60_000);
|
||||
cooldown.recordFailure('user-1', 'alpha');
|
||||
|
||||
expect(cooldown.isCoolingDown('user-1', 'alpha')).toBe(true);
|
||||
jest.advanceTimersByTime(59_000);
|
||||
expect(cooldown.isCoolingDown('user-1', 'alpha')).toBe(true);
|
||||
jest.advanceTimersByTime(2_000);
|
||||
expect(cooldown.isCoolingDown('user-1', 'alpha')).toBe(false);
|
||||
} finally {
|
||||
jest.useRealTimers();
|
||||
}
|
||||
});
|
||||
|
||||
it('scopes failures per user and per server, and clears them on success', () => {
|
||||
const cooldown = createMCPCatalogRecoveryCooldown(60_000);
|
||||
cooldown.recordFailure('user-1', 'alpha');
|
||||
|
||||
expect(cooldown.isCoolingDown('user-1', 'beta')).toBe(false);
|
||||
expect(cooldown.isCoolingDown('user-2', 'alpha')).toBe(false);
|
||||
|
||||
cooldown.recordSuccess('user-1', 'alpha');
|
||||
expect(cooldown.isCoolingDown('user-1', 'alpha')).toBe(false);
|
||||
});
|
||||
|
||||
it('sweeps expired entries so the failure map cannot grow without bound', () => {
|
||||
jest.useFakeTimers();
|
||||
try {
|
||||
const cooldown = createMCPCatalogRecoveryCooldown(60_000);
|
||||
for (let i = 0; i < 50; i++) {
|
||||
cooldown.recordFailure(`user-${i}`, 'alpha');
|
||||
}
|
||||
jest.advanceTimersByTime(61_000);
|
||||
cooldown.recordFailure('user-current', 'alpha');
|
||||
|
||||
expect(cooldown.isCoolingDown('user-current', 'alpha')).toBe(true);
|
||||
for (let i = 0; i < 50; i++) {
|
||||
expect(cooldown.isCoolingDown(`user-${i}`, 'alpha')).toBe(false);
|
||||
}
|
||||
} finally {
|
||||
jest.useRealTimers();
|
||||
}
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -3,15 +3,34 @@ import type { Tool } from '@modelcontextprotocol/sdk/types.js';
|
|||
import type { IUser } from '@librechat/data-schemas';
|
||||
import type { LCAvailableTools, ParsedServerConfig, ToolDiscoveryOptions } from '../types';
|
||||
import { createConcurrencyLimiter } from '~/utils/promise';
|
||||
import { getMissingCustomUserVars } from '../utils';
|
||||
import { getServerCustomUserVars } from '../auth';
|
||||
|
||||
const RECOVERY_CONCURRENCY = 3;
|
||||
/**
|
||||
* Passive recovery runs inline on a catalog list request, so an unreachable server must not
|
||||
* hold the response for the connection default (`initTimeout ?? 30s`). A server configured to
|
||||
* connect faster keeps its own shorter limit.
|
||||
*/
|
||||
const RECOVERY_TIMEOUT_MS = 5000;
|
||||
/** How long a server that failed passive recovery is skipped before it is dialed again. */
|
||||
const RECOVERY_COOLDOWN_MS = 60 * 1000;
|
||||
|
||||
export interface MCPServerCatalogRecoveryInput {
|
||||
serverName: string;
|
||||
serverConfig: ParsedServerConfig;
|
||||
}
|
||||
|
||||
/**
|
||||
* Per-process record of servers whose passive recovery just failed. Recovered catalogs are
|
||||
* request-local, so without it every list request re-dials the same unreachable servers.
|
||||
*/
|
||||
export interface MCPCatalogRecoveryCooldown {
|
||||
isCoolingDown: (userId: string, serverName: string) => boolean;
|
||||
recordFailure: (userId: string, serverName: string) => void;
|
||||
recordSuccess: (userId: string, serverName: string) => void;
|
||||
}
|
||||
|
||||
export interface MCPServerCatalogRecoveryDeps {
|
||||
loadUserMCPAuthMap: (
|
||||
userId: string,
|
||||
|
|
@ -19,6 +38,7 @@ export interface MCPServerCatalogRecoveryDeps {
|
|||
) => Promise<Record<string, Record<string, string>>>;
|
||||
discoverServerTools: (options: ToolDiscoveryOptions) => Promise<{ tools: Tool[] | null }>;
|
||||
formatServerTools: (serverName: string, tools: Tool[]) => LCAvailableTools;
|
||||
recoveryCooldown?: MCPCatalogRecoveryCooldown;
|
||||
}
|
||||
|
||||
export interface MCPServerCatalogSnapshot {
|
||||
|
|
@ -53,6 +73,74 @@ export interface MCPServerCatalogLoaderResult {
|
|||
serversWithoutTools: string[];
|
||||
}
|
||||
|
||||
/**
|
||||
* Creates the failure cooldown shared by every request in one process. Entries expire on their
|
||||
* own, and an expired sweep runs at most once per window so lookups stay amortized constant.
|
||||
*/
|
||||
export function createMCPCatalogRecoveryCooldown(
|
||||
cooldownMs: number = RECOVERY_COOLDOWN_MS,
|
||||
): MCPCatalogRecoveryCooldown {
|
||||
const failedAt = new Map<string, number>();
|
||||
const cooldownKey = (userId: string, serverName: string): string => `${userId}:${serverName}`;
|
||||
let lastSweptAt = 0;
|
||||
|
||||
const sweep = (now: number): void => {
|
||||
if (now - lastSweptAt < cooldownMs) {
|
||||
return;
|
||||
}
|
||||
lastSweptAt = now;
|
||||
for (const [key, timestamp] of failedAt) {
|
||||
if (now - timestamp >= cooldownMs) {
|
||||
failedAt.delete(key);
|
||||
}
|
||||
}
|
||||
};
|
||||
|
||||
return {
|
||||
isCoolingDown: (userId, serverName) => {
|
||||
const now = Date.now();
|
||||
sweep(now);
|
||||
const timestamp = failedAt.get(cooldownKey(userId, serverName));
|
||||
return timestamp != null && now - timestamp < cooldownMs;
|
||||
},
|
||||
recordFailure: (userId, serverName) => {
|
||||
failedAt.set(cooldownKey(userId, serverName), Date.now());
|
||||
},
|
||||
recordSuccess: (userId, serverName) => {
|
||||
failedAt.delete(cooldownKey(userId, serverName));
|
||||
},
|
||||
};
|
||||
}
|
||||
|
||||
function resolveRecoveryTimeout(serverConfig: ParsedServerConfig): number {
|
||||
const { initTimeout } = serverConfig;
|
||||
if (typeof initTimeout === 'number') {
|
||||
return Math.min(initTimeout, RECOVERY_TIMEOUT_MS);
|
||||
}
|
||||
return RECOVERY_TIMEOUT_MS;
|
||||
}
|
||||
|
||||
/**
|
||||
* A server the config tier already marked unreachable is left to that tier's retry window, and
|
||||
* one that just failed recovery is left to its cooldown; dialing either again on this request
|
||||
* only repeats a known failure.
|
||||
*/
|
||||
function isRecoverable(
|
||||
userId: string,
|
||||
{ serverName, serverConfig }: MCPServerCatalogRecoveryInput,
|
||||
cooldown?: MCPCatalogRecoveryCooldown,
|
||||
): boolean {
|
||||
if (serverConfig.inspectionFailed) {
|
||||
logger.debug(`[MCP catalog recovery] Skipping ${serverName}: awaiting config-tier retry`);
|
||||
return false;
|
||||
}
|
||||
if (cooldown?.isCoolingDown(userId, serverName)) {
|
||||
logger.debug(`[MCP catalog recovery] Skipping ${serverName}: recent discovery failure`);
|
||||
return false;
|
||||
}
|
||||
return true;
|
||||
}
|
||||
|
||||
/**
|
||||
* Passively discovers cold MCP catalogs for one request. Results are intentionally not cached:
|
||||
* discovery connections do not own a publication generation, so publishing them could overwrite
|
||||
|
|
@ -63,28 +151,58 @@ export async function recoverMCPServerCatalogs(
|
|||
deps: MCPServerCatalogRecoveryDeps,
|
||||
): Promise<Map<string, LCAvailableTools>> {
|
||||
const { user, servers } = params;
|
||||
if (servers.length === 0) {
|
||||
const recoverable = servers.filter((server) =>
|
||||
isRecoverable(user.id, server, deps.recoveryCooldown),
|
||||
);
|
||||
if (recoverable.length === 0) {
|
||||
return new Map();
|
||||
}
|
||||
|
||||
const userMCPAuthMap = await deps.loadUserMCPAuthMap(
|
||||
user.id,
|
||||
recoverable.map(({ serverName }) => serverName),
|
||||
);
|
||||
|
||||
/** A server missing its user-provided credentials fails auth on connect (see issue #10969),
|
||||
* so discovering it would spend a doomed connection on every request. */
|
||||
const authorized: Array<
|
||||
MCPServerCatalogRecoveryInput & { customUserVars?: Record<string, string> }
|
||||
> = [];
|
||||
for (const server of recoverable) {
|
||||
const customUserVars = getServerCustomUserVars(userMCPAuthMap, server.serverName);
|
||||
const missingUserVars = getMissingCustomUserVars(server.serverConfig, customUserVars);
|
||||
if (missingUserVars.length > 0) {
|
||||
logger.debug(
|
||||
`[MCP catalog recovery] Skipping ${server.serverName}: ${missingUserVars.length} user-provided variable(s) unset`,
|
||||
);
|
||||
continue;
|
||||
}
|
||||
authorized.push({ ...server, customUserVars });
|
||||
}
|
||||
if (authorized.length === 0) {
|
||||
return new Map();
|
||||
}
|
||||
|
||||
const serverNames = servers.map(({ serverName }) => serverName);
|
||||
const userMCPAuthMap = await deps.loadUserMCPAuthMap(user.id, serverNames);
|
||||
const recover = createConcurrencyLimiter(RECOVERY_CONCURRENCY);
|
||||
const results = await Promise.all(
|
||||
servers.map(({ serverName, serverConfig }) =>
|
||||
authorized.map(({ serverName, serverConfig, customUserVars }) =>
|
||||
recover(async (): Promise<[string, LCAvailableTools | null]> => {
|
||||
try {
|
||||
const result = await deps.discoverServerTools({
|
||||
user,
|
||||
serverName,
|
||||
configServers: { [serverName]: serverConfig },
|
||||
customUserVars: getServerCustomUserVars(userMCPAuthMap, serverName),
|
||||
customUserVars,
|
||||
connectionTimeout: resolveRecoveryTimeout(serverConfig),
|
||||
});
|
||||
return [
|
||||
serverName,
|
||||
result.tools == null ? null : deps.formatServerTools(serverName, result.tools),
|
||||
];
|
||||
if (result.tools == null) {
|
||||
deps.recoveryCooldown?.recordFailure(user.id, serverName);
|
||||
return [serverName, null];
|
||||
}
|
||||
deps.recoveryCooldown?.recordSuccess(user.id, serverName);
|
||||
return [serverName, deps.formatServerTools(serverName, result.tools)];
|
||||
} catch (error) {
|
||||
deps.recoveryCooldown?.recordFailure(user.id, serverName);
|
||||
logger.error(`[MCP catalog recovery] Failed to discover tools for ${serverName}:`, error);
|
||||
return [serverName, null];
|
||||
}
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue