From 8fcab7e44f1b7f0242985833009e8a5c3d8cbb33 Mon Sep 17 00:00:00 2001 From: Dustin Healy <54083382+dustinhealy@users.noreply.github.com> Date: Sun, 30 Aug 2026 03:55:30 -0700 Subject: [PATCH] =?UTF-8?q?=F0=9F=94=84=20fix:=20Recover=20Missing=20MCP?= =?UTF-8?q?=20Marketplace=20Catalogs=20(#15323)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix: recover missing MCP marketplace catalogs * fix: make MCP catalog recovery passive * test: type MCP catalog recovery fixtures * 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 Claude-Session: https://claude.ai/code/session_01Xr1Dabvdn1mzzyYgpJgU5B * fix: bound passive MCP recovery by deadline, key cooldowns by config Both follow-ups address the same mistake: recovery expressed its own request-level constraints in terms borrowed from other layers. `connectionTimeout` bounds one connection attempt, and `MCPConnectionFactory.discoverToolsInternal` spends it twice — once on the authenticated connection, then again in `attemptUnauthenticatedToolListing` — so capping it bounded no total this layer could reason about. Recovery now enforces its own wall-clock deadline per server with `withTimeout`, which holds however many attempts the factory makes; `connectionTimeout` is left to do only its own job, still honouring a shorter operator `initTimeout`. An attempt abandoned by the deadline disposes its own connection when it settles, and `Promise.race` keeps a handler on it, so a late rejection is not unhandled. A per-request budget now caps total recovery regardless of server count. A server is dialed only if the remaining budget can fund a full deadline; never dialing one is not evidence against it, so a skipped server records no cooldown and a later request reaches it once those ahead are cached or cooling down. Cooldown identity now includes the publication generation — the same effective-config identity the tool caches fence on — instead of just user and server name. Correcting a server's URL or transport keys a new entry, so the refetch the client issues on update is no longer skipped for up to a minute by the previous configuration's failure. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Xr1Dabvdn1mzzyYgpJgU5B * refactor: keep passive MCP recovery stateless and bounded by its own work Reverts the cooldown, request budget and deadline race added in 257d5cf and fc3e3c9, and keeps only the three stateless limits. The tool cache refuses unfenced writes (`tools.ts`), and a discovery connection owns no publication generation and is disposed, so a recovered catalog cannot be retained by design. Those commits responded by building a cache-shaped memory in front of it — per-process failure state, a scheduling budget, an identity, an eviction sweep — and each round of review found another way that hand-rolled cache differed from a real one: wrong identity for configuration, wrong identity for credentials, no fairness across requests, and a limiter slot released while its network operation was still running. None of that machinery was asked for; all of it was compensation for a result the architecture does not allow keeping. Recovery is now stateless. It skips only what configuration alone proves pointless — a server the config tier already marked `inspectionFailed`, and one whose declared `customUserVars` are unset — and bounds the work itself rather than racing it, so a limiter slot is held for exactly as long as its network operation runs and the concurrency limit of three is real. The attempt timeout is not a compromise: recovery exists for a server that is reachable and authorized but whose catalog cache expired, and such a server answers tools/list well inside 1.5s. Anything slower cannot be rescued here, so failing fast costs nothing. The factory spends that value per attempt, so a server's ceiling is it times the attempts made; the constant documents that rather than hiding it behind a number tuned to today's attempt count. Consequences that were bugs are now gone by construction: every cold server is attempted on every request, so none is starved by those ahead of it, and correcting a server's configuration or credentials takes effect on the next refetch instead of waiting out a stale cooldown. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Xr1Dabvdn1mzzyYgpJgU5B * fix: correct the inspection-failure skip and bound catalog fan-out Three fixes that belong to this layer; a fourth issue does not, and is described below. The `inspectionFailed` skip was too broad. `MCPServersInitializer` stores a YAML server that was unreachable at startup via `addServerStub`, which stamps `source: 'yaml'`, and only config-tier entries get the timed retry in `ensureSingleConfigServer`. Skipping every failed stub therefore hid a recoverable server from the marketplace permanently — the exact state this recovery exists to escape. It now defers only `source === 'config'`, matching what `reinitMCPServer` already does. Plugin auth is read only when some cold server actually declares `customUserVars`, and only for those servers. The common unauthenticated case no longer pays a MongoDB round trip whose result nothing can consume. Snapshot refreshes are now bounded by the same limiter as discovery. They are not local reads: both connection paths reach `fetchOrderedToolsSnapshot` and issue a real `tools/list`, so a cache reset across many servers previously burst unbounded outbound requests while discovery was capped at three. Not fixed here, because it cannot be: `connectionTimeout` does not bound discovery. It covers `connection.connect()` only, and `fetchToolsSnapshot` then applies its own `TOOLS_LIST_TIMEOUT_MS` (30s) to `tools/list`, so a server that connects fast and stalls while listing still holds its slot for that window. The factory also does not cancel a timed-out connect before starting the unauthenticated fallback. Bounding this end to end needs a deadline threaded through `MCPConnectionFactory` into both `connect()` and `fetchToolsSnapshot()`, which is a change to shared connection machinery rather than to this caller. The constant's comment now states what it does and does not bound instead of implying an end-to-end guarantee. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Xr1Dabvdn1mzzyYgpJgU5B * fix: thread live-session OBO context into passive catalog discovery The merge of #15334 sources OBO tokens from the live OpenID session via request-boundary closures. Passive catalog recovery is a discovery call site too; without these options an OBO server whose stored token went stale fails recovery — the exact cold-catalog class this PR fixes. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Xr1Dabvdn1mzzyYgpJgU5B --------- Co-authored-by: Claude --- api/server/controllers/mcp.js | 78 +-- api/server/routes/__tests__/mcp.spec.js | 180 ++++--- api/server/services/Tools/mcp.js | 59 ++- api/server/services/Tools/mcp.spec.js | 101 +++- packages/api/src/index.ts | 1 + packages/api/src/mcp/MCPManager.ts | 26 +- .../api/src/mcp/__tests__/MCPManager.test.ts | 35 ++ packages/api/src/mcp/catalog/recovery.spec.ts | 484 ++++++++++++++++++ packages/api/src/mcp/catalog/recovery.ts | 262 ++++++++++ packages/api/src/mcp/tools.ts | 65 ++- 10 files changed, 1103 insertions(+), 188 deletions(-) create mode 100644 packages/api/src/mcp/catalog/recovery.spec.ts create mode 100644 packages/api/src/mcp/catalog/recovery.ts diff --git a/api/server/controllers/mcp.js b/api/server/controllers/mcp.js index 19ea8d90b5..1d1f6eed7c 100644 --- a/api/server/controllers/mcp.js +++ b/api/server/controllers/mcp.js @@ -6,10 +6,11 @@ * @import { MCPServerDocument } from 'librechat-data-provider' */ const { randomUUID } = require('crypto'); -const { logger, SystemCapabilities } = require('@librechat/data-schemas'); +const { logger, getTenantId, SystemCapabilities } = require('@librechat/data-schemas'); const { checkAccess, isUserSourced, + createAuthIdentityContext, MCPConnection, MCPErrorCodes, splitMCPToolKey, @@ -35,6 +36,8 @@ const { resolveMcpConfigNames, resolveAllMcpConfigs, } = require('~/server/services/MCP'); +const { loadMCPServerCatalogs } = require('~/server/services/Tools/mcp'); +const { createOpenIDSessionTokenProvider } = require('~/server/services/OpenIDSessionRefresh'); const { cacheMCPServerTools, getMCPServerTools, @@ -193,65 +196,26 @@ const getMCPTools = async (req, res) => { return res.status(200).json({ servers: {} }); } - const mcpManager = getMCPManager(); const mcpServers = {}; - - const serverToolsMap = new Map(); - const serversWithoutTools = []; - const cacheResults = await Promise.all( - configuredServers.map(async (serverName) => { - try { - return { - serverName, - tools: await getMCPServerTools(userId, serverName, mcpConfig[serverName]), - }; - } catch (error) { - logger.error(`[getMCPTools] Error fetching cached tools for ${serverName}:`, error); - return { serverName, tools: null }; - } - }), - ); - for (const { serverName, tools } of cacheResults) { - if (tools) { - serverToolsMap.set(serverName, tools); - continue; - } - - let serverTools; - let publicationGeneration; - let publicationRevision; - try { - ({ - tools: serverTools, - publicationGeneration, - publicationRevision, - } = await mcpManager.getServerToolFunctionsSnapshot( - userId, - serverName, - mcpConfig[serverName], - )); - } catch (error) { - logger.error(`[getMCPTools] Error fetching tools for server ${serverName}:`, error); - continue; - } - if (!serverTools) { - serversWithoutTools.push(serverName); - continue; - } - serverToolsMap.set(serverName, serverTools); - - // Empty is an authoritative catalog too; re-cache it after TTL expiry to avoid polling. - cacheMCPServerTools({ - userId, + const oboIdentityContext = createAuthIdentityContext({ + user: req.user, + tenantId: getTenantId(), + }); + const { serverTools: serverToolsMap, serversWithoutTools } = await loadMCPServerCatalogs({ + user: req.user, + servers: configuredServers.map((serverName) => ({ serverName, - serverTools, serverConfig: mcpConfig[serverName], - publicationGeneration, - publicationRevision, - }).catch((err) => - logger.error(`[getMCPTools] Failed to cache tools for ${serverName}:`, err), - ); - } + })), + upstreamTokenProvider: createOpenIDSessionTokenProvider({ + req, + res, + user: req.user, + identityContext: oboIdentityContext, + tokenPreference: 'access_token', + }), + oboIdentityContext, + }); if (serversWithoutTools.length > 0) { logger.debug( `[getMCPTools] No tools (${serversWithoutTools.length}): ${serversWithoutTools.join(', ')}`, diff --git a/api/server/routes/__tests__/mcp.spec.js b/api/server/routes/__tests__/mcp.spec.js index 17b6d4e678..fe5ca60df5 100644 --- a/api/server/routes/__tests__/mcp.spec.js +++ b/api/server/routes/__tests__/mcp.spec.js @@ -33,6 +33,10 @@ const mockRegistryInstance = { }), }; let mockMCPUseAllowed = true; +const mockLoadMCPServerCatalogs = jest.fn().mockResolvedValue({ + serverTools: new Map(), + serversWithoutTools: [], +}); jest.mock('@librechat/api', () => { const actual = jest.requireActual('@librechat/api'); @@ -174,6 +178,7 @@ jest.mock('~/server/middleware', () => ({ jest.mock('~/server/services/Tools/mcp', () => ({ reinitMCPServer: jest.fn(), + loadMCPServerCatalogs: (...args) => mockLoadMCPServerCatalogs(...args), })); const mockOAuthCompletion = (tokens) => { @@ -225,6 +230,10 @@ describe('MCP Routes', () => { beforeEach(() => { jest.clearAllMocks(); + mockLoadMCPServerCatalogs.mockResolvedValue({ + serverTools: new Map(), + serversWithoutTools: [], + }); currentUser = undefined; mockResolveAllMcpConfigs.mockResolvedValue({}); mockResolveMcpConfigNames.mockResolvedValue([]); @@ -3284,9 +3293,8 @@ describe('MCP Routes', () => { expect(mockResolveAllMcpConfigs).not.toHaveBeenCalled(); }); - it('caches a live user snapshot with its connection-bound publication generation', async () => { + it('delegates catalog loading and renders the returned tools', async () => { const { Constants } = require('librechat-data-provider'); - const { cacheMCPServerTools, getMCPServerTools } = require('~/server/services/Config'); const pluginKey = `search${Constants.mcp_delimiter}user-server`; const serverTools = { [pluginKey]: { @@ -3300,58 +3308,80 @@ describe('MCP Routes', () => { }; const serverConfig = { type: 'sse', url: 'https://user.example.com/sse' }; mockResolveAllMcpConfigs.mockResolvedValueOnce({ 'user-server': serverConfig }); - getMCPServerTools.mockResolvedValueOnce(null); - cacheMCPServerTools.mockResolvedValueOnce(); - require('~/config').getMCPManager.mockReturnValue({ - getServerToolFunctionsSnapshot: jest.fn().mockResolvedValue({ - tools: serverTools, - publicationGeneration: 'connection-generation', - }), + mockLoadMCPServerCatalogs.mockResolvedValueOnce({ + serverTools: new Map([['user-server', serverTools]]), + serversWithoutTools: [], }); const response = await request(app).get('/api/mcp/tools'); expect(response.status).toBe(200); - expect(cacheMCPServerTools).toHaveBeenCalledWith({ - userId: 'test-user-id', - serverName: 'user-server', - serverTools, - serverConfig, - publicationGeneration: 'connection-generation', + expect(response.body.servers['user-server'].tools).toEqual([ + { name: 'search', pluginKey, description: 'Search' }, + ]); + expect(mockLoadMCPServerCatalogs).toHaveBeenCalledWith({ + user: { id: 'test-user-id' }, + servers: [{ serverName: 'user-server', serverConfig }], + upstreamTokenProvider: expect.any(Function), + oboIdentityContext: expect.any(Object), }); }); - it('re-caches an authoritative empty live snapshot after a cache miss', async () => { - const { cacheMCPServerTools, getMCPServerTools } = require('~/server/services/Config'); + it('renders an authoritative empty catalog as a configured server', async () => { const serverConfig = { type: 'sse', url: 'https://empty.example.com/sse' }; mockResolveAllMcpConfigs.mockResolvedValueOnce({ 'empty-server': serverConfig }); - getMCPServerTools.mockResolvedValueOnce(null); - cacheMCPServerTools.mockResolvedValueOnce(); - const getServerToolFunctionsSnapshot = jest.fn().mockResolvedValue({ - tools: {}, - publicationGeneration: undefined, + mockLoadMCPServerCatalogs.mockResolvedValueOnce({ + serverTools: new Map([['empty-server', {}]]), + serversWithoutTools: [], }); - require('~/config').getMCPManager.mockReturnValue({ getServerToolFunctionsSnapshot }); const response = await request(app).get('/api/mcp/tools'); expect(response.status).toBe(200); expect(response.body.servers['empty-server'].tools).toEqual([]); - expect(cacheMCPServerTools).toHaveBeenCalledWith({ - userId: 'test-user-id', - serverName: 'empty-server', - serverTools: {}, - serverConfig, - publicationGeneration: undefined, + }); + + it('renders a passively recovered catalog returned by the loader', async () => { + const { Constants } = require('librechat-data-provider'); + const pluginKey = `search${Constants.mcp_delimiter}connected-server`; + const serverTools = { + [pluginKey]: { + type: 'function', + function: { + name: pluginKey, + description: 'Search', + parameters: { type: 'object' }, + }, + }, + }; + const serverConfig = { type: 'sse', url: 'https://connected.example.com/sse' }; + mockResolveAllMcpConfigs.mockResolvedValueOnce({ 'connected-server': serverConfig }); + mockLoadMCPServerCatalogs.mockResolvedValueOnce({ + serverTools: new Map([['connected-server', serverTools]]), + serversWithoutTools: [], + }); + + const response = await request(app).get('/api/mcp/tools'); + + expect(response.status).toBe(200); + expect(response.body.servers['connected-server'].tools).toEqual([ + { + name: 'search', + pluginKey, + description: 'Search', + }, + ]); + expect(mockLoadMCPServerCatalogs).toHaveBeenCalledWith({ + user: { id: 'test-user-id' }, + servers: [{ serverName: 'connected-server', serverConfig }], + upstreamTokenProvider: expect.any(Function), + oboIdentityContext: expect.any(Object), }); }); - it('should continue returning MCP tools when one server cache lookup fails', async () => { + it('continues rendering available tools when another server has no catalog', async () => { const { Constants } = require('librechat-data-provider'); - const { logger } = require('@librechat/data-schemas'); - const { getMCPServerTools } = require('~/server/services/Config'); - - mockResolveAllMcpConfigs.mockResolvedValueOnce({ + const configs = { 'bad-server': { type: 'sse', url: 'https://bad.example.com/sse', @@ -3361,45 +3391,30 @@ describe('MCP Routes', () => { url: 'https://good.example.com/sse', iconPath: '/icons/good.svg', }, - }); - - // Mock order matches Object.keys() order from the config above. - getMCPServerTools - .mockRejectedValueOnce(new Error('cache unavailable')) - .mockResolvedValueOnce({ - [`search${Constants.mcp_delimiter}good-server`]: { - type: 'function', - function: { - name: `search${Constants.mcp_delimiter}good-server`, - description: 'Search good server', - parameters: { type: 'object' }, + }; + mockResolveAllMcpConfigs.mockResolvedValueOnce(configs); + mockLoadMCPServerCatalogs.mockResolvedValueOnce({ + serverTools: new Map([ + [ + 'good-server', + { + [`search${Constants.mcp_delimiter}good-server`]: { + type: 'function', + function: { + name: `search${Constants.mcp_delimiter}good-server`, + description: 'Search good server', + parameters: { type: 'object' }, + }, + }, }, - }, - }); - - const mockGetServerToolFunctionsSnapshot = jest.fn().mockResolvedValue({ - tools: null, - publicationGeneration: 'test-generation', - }); - require('~/config').getMCPManager.mockReturnValue({ - getServerToolFunctionsSnapshot: mockGetServerToolFunctionsSnapshot, + ], + ]), + serversWithoutTools: ['bad-server'], }); const response = await request(app).get('/api/mcp/tools'); expect(response.status).toBe(200); - expect(logger.error).toHaveBeenCalledWith( - '[getMCPTools] Error fetching cached tools for bad-server:', - expect.any(Error), - ); - expect(mockGetServerToolFunctionsSnapshot).toHaveBeenCalledWith( - 'test-user-id', - 'bad-server', - { - type: 'sse', - url: 'https://bad.example.com/sse', - }, - ); expect(response.body.servers['good-server']).toMatchObject({ name: 'good-server', icon: '/icons/good.svg', @@ -3419,9 +3434,7 @@ describe('MCP Routes', () => { it('should return configured servers when all cache lookups fail', async () => { const { logger } = require('@librechat/data-schemas'); - const { getMCPServerTools } = require('~/server/services/Config'); - - mockResolveAllMcpConfigs.mockResolvedValueOnce({ + const configs = { 'first-server': { type: 'sse', url: 'https://first.example.com/sse', @@ -3430,16 +3443,11 @@ describe('MCP Routes', () => { type: 'sse', url: 'https://second.example.com/sse', }, - }); - - getMCPServerTools.mockRejectedValue(new Error('cache unavailable')); - - const mockGetServerToolFunctionsSnapshot = jest.fn().mockResolvedValue({ - tools: null, - publicationGeneration: 'test-generation', - }); - require('~/config').getMCPManager.mockReturnValue({ - getServerToolFunctionsSnapshot: mockGetServerToolFunctionsSnapshot, + }; + mockResolveAllMcpConfigs.mockResolvedValueOnce(configs); + mockLoadMCPServerCatalogs.mockResolvedValueOnce({ + serverTools: new Map(), + serversWithoutTools: ['first-server', 'second-server'], }); const response = await request(app).get('/api/mcp/tools'); @@ -3457,8 +3465,16 @@ describe('MCP Routes', () => { expect(logger.debug).toHaveBeenCalledWith( '[getMCPTools] No tools (2): first-server, second-server', ); - expect(logger.error).toHaveBeenCalledTimes(2); - expect(mockGetServerToolFunctionsSnapshot).toHaveBeenCalledTimes(2); + expect(mockLoadMCPServerCatalogs).toHaveBeenCalledTimes(1); + expect(mockLoadMCPServerCatalogs).toHaveBeenCalledWith({ + user: { id: 'test-user-id' }, + servers: Object.entries(configs).map(([serverName, serverConfig]) => ({ + serverName, + serverConfig, + })), + upstreamTokenProvider: expect.any(Function), + oboIdentityContext: expect.any(Object), + }); }); }); diff --git a/api/server/services/Tools/mcp.js b/api/server/services/Tools/mcp.js index c74b80ce10..b1eb82f303 100644 --- a/api/server/services/Tools/mcp.js +++ b/api/server/services/Tools/mcp.js @@ -1,16 +1,30 @@ const { logger } = require('@librechat/data-schemas'); const { + formatMCPServerTools, + getUserMCPAuthMap, getMissingCustomUserVars, + loadMCPServerCatalogs: loadCatalogs, requiresEphemeralUserConnection, getMissingRuntimeBodyPlaceholderFields, } = require('@librechat/api'); const { CacheKeys, Constants } = require('librechat-data-provider'); const { getMCPManager, getMCPServersRegistry, getFlowStateManager } = require('~/config'); -const { findToken, createToken, updateToken, deleteTokens } = require('~/models'); +const { + findToken, + createToken, + updateToken, + deleteTokens, + findPluginAuthsByKeys, +} = require('~/models'); const { getGraphApiToken } = require('~/server/services/GraphTokenService'); const { exchangeOboToken } = require('~/server/services/OboTokenService'); const { createOboTrustChecker } = require('~/server/services/OboPolicyService'); -const { getMCPToolsCacheGeneration, updateMCPServerTools } = require('~/server/services/Config'); +const { + getMCPServerTools, + cacheMCPServerTools, + getMCPToolsCacheGeneration, + updateMCPServerTools, +} = require('~/server/services/Config'); const { getLogStores } = require('~/cache'); const MCP_REINITIALIZE_FAILURE_REASONS = { @@ -20,6 +34,46 @@ const MCP_REINITIALIZE_FAILURE_REASONS = { INITIALIZATION_FAILED: 'initialization_failed', }; +/** Wires application dependencies into the passive, request-local catalog recovery service. + * @param {Object} params + * @param {IUser} params.user + * @param {Array<{ serverName: string, serverConfig: object }>} params.servers + * @param {import('@librechat/api').UpstreamTokenProvider} [params.upstreamTokenProvider] - Live upstream-token closure for OBO discovery, built at the request boundary so this layer never receives the raw Express request. + * @param {import('@librechat/api').AuthIdentityContext} [params.oboIdentityContext] - Non-template-visible OBO identity context built from the real request user. + */ +async function loadMCPServerCatalogs({ user, servers, upstreamTokenProvider, oboIdentityContext }) { + const flowManager = getFlowStateManager(getLogStores(CacheKeys.FLOWS)); + const tokenMethods = { findToken, updateToken, createToken, deleteTokens }; + const mcpManager = getMCPManager(); + return loadCatalogs( + { user, servers }, + { + loadUserMCPAuthMap: (userId, serverNames) => + getUserMCPAuthMap({ + userId, + servers: serverNames, + findPluginAuthsByKeys, + }), + discoverServerTools: (options) => + mcpManager.discoverServerTools({ + ...options, + flowManager, + tokenMethods, + graphTokenResolver: getGraphApiToken, + oboTokenResolver: exchangeOboToken, + oboTrustChecker: createOboTrustChecker(), + upstreamTokenProvider, + oboIdentityContext, + }), + formatServerTools: formatMCPServerTools, + getCachedServerTools: getMCPServerTools, + getServerToolFunctionsSnapshot: (userId, serverName, serverConfig) => + mcpManager.getServerToolFunctionsSnapshot(userId, serverName, serverConfig), + cacheServerTools: cacheMCPServerTools, + }, + ); +} + /** * Reinitializes an MCP server connection and discovers available tools. * When OAuth is required, uses discovery mode to list tools without full authentication @@ -383,4 +437,5 @@ async function reinitMCPServer({ module.exports = { reinitMCPServer, + loadMCPServerCatalogs, }; diff --git a/api/server/services/Tools/mcp.spec.js b/api/server/services/Tools/mcp.spec.js index d5bdeff5f9..8eb8bac327 100644 --- a/api/server/services/Tools/mcp.spec.js +++ b/api/server/services/Tools/mcp.spec.js @@ -7,11 +7,25 @@ const mockGetGraphApiToken = jest.fn(); const mockUpdateMCPServerTools = jest.fn(); const mockGetMCPToolsCacheGeneration = jest.fn().mockResolvedValue('generation-current'); const mockGetToolPublicationGeneration = jest.fn().mockReturnValue('generation-current'); +const mockLoadCatalogs = jest.fn(); +const mockGetUserMCPAuthMap = jest.fn(); +const mockFormatMCPServerTools = jest.fn(); +const mockGetMCPServerTools = jest.fn(); +const mockCacheMCPServerTools = jest.fn(); +const mockGetServerToolFunctionsSnapshot = jest.fn(); + +jest.mock('@librechat/api', () => ({ + ...jest.requireActual('@librechat/api'), + loadMCPServerCatalogs: (...args) => mockLoadCatalogs(...args), + getUserMCPAuthMap: (...args) => mockGetUserMCPAuthMap(...args), + formatMCPServerTools: (...args) => mockFormatMCPServerTools(...args), +})); jest.mock('~/config', () => ({ getMCPManager: jest.fn(() => ({ getConnection: mockGetConnection, discoverServerTools: mockDiscoverServerTools, + getServerToolFunctionsSnapshot: mockGetServerToolFunctionsSnapshot, getToolPublicationGeneration: mockGetToolPublicationGeneration, })), getMCPServersRegistry: jest.fn(() => ({ getServerConfig: jest.fn() })), @@ -22,10 +36,13 @@ jest.mock('~/models', () => ({ createToken: jest.fn(), updateToken: jest.fn(), deleteTokens: jest.fn(), + findPluginAuthsByKeys: jest.fn(), })); jest.mock('~/server/services/Config', () => ({ updateMCPServerTools: mockUpdateMCPServerTools, getMCPToolsCacheGeneration: mockGetMCPToolsCacheGeneration, + getMCPServerTools: mockGetMCPServerTools, + cacheMCPServerTools: mockCacheMCPServerTools, })); jest.mock('~/server/services/GraphTokenService', () => ({ getGraphApiToken: mockGetGraphApiToken, @@ -34,7 +51,89 @@ jest.mock('~/cache', () => ({ getLogStores: jest.fn(() => ({})), })); -const { reinitMCPServer } = require('./mcp'); +const { reinitMCPServer, loadMCPServerCatalogs } = require('./mcp'); + +describe('loadMCPServerCatalogs', () => { + beforeEach(() => { + jest.clearAllMocks(); + }); + + it('wires batched auth and passive discovery without opening a managed connection', async () => { + const user = { id: 'user-123' }; + const servers = [ + { + serverName: 'config-only', + serverConfig: { type: 'sse', url: 'https://config.example.com/sse' }, + }, + { + serverName: 'user-server', + serverConfig: { type: 'sse', url: 'https://user.example.com/sse' }, + }, + ]; + mockGetUserMCPAuthMap.mockResolvedValue({}); + mockDiscoverServerTools.mockResolvedValue({ tools: [] }); + mockFormatMCPServerTools.mockReturnValue({}); + mockLoadCatalogs.mockImplementation(async (params, deps) => { + await deps.loadUserMCPAuthMap( + user.id, + servers.map(({ serverName }) => serverName), + ); + await deps.discoverServerTools({ + user, + serverName: 'config-only', + configServers: { 'config-only': servers[0].serverConfig }, + }); + deps.formatServerTools('config-only', []); + await deps.getCachedServerTools(user.id, 'config-only', servers[0].serverConfig); + await deps.getServerToolFunctionsSnapshot(user.id, 'config-only', servers[0].serverConfig); + await deps.cacheServerTools({ serverName: 'config-only' }); + return { serverTools: new Map([['config-only', {}]]), serversWithoutTools: [] }; + }); + + const upstreamTokenProvider = jest.fn(); + const oboIdentityContext = { appUserId: 'user-123' }; + const result = await loadMCPServerCatalogs({ + user, + servers, + upstreamTokenProvider, + oboIdentityContext, + }); + + expect(mockGetUserMCPAuthMap).toHaveBeenCalledTimes(1); + expect(mockGetUserMCPAuthMap).toHaveBeenCalledWith({ + userId: user.id, + servers: ['config-only', 'user-server'], + findPluginAuthsByKeys: require('~/models').findPluginAuthsByKeys, + }); + expect(mockDiscoverServerTools).toHaveBeenCalledWith( + expect.objectContaining({ + user, + serverName: 'config-only', + configServers: { 'config-only': servers[0].serverConfig }, + flowManager: expect.any(Object), + tokenMethods: expect.any(Object), + upstreamTokenProvider, + oboIdentityContext, + }), + ); + expect(mockGetConnection).not.toHaveBeenCalled(); + expect(mockGetMCPServerTools).toHaveBeenCalledWith( + user.id, + 'config-only', + servers[0].serverConfig, + ); + expect(mockGetServerToolFunctionsSnapshot).toHaveBeenCalledWith( + user.id, + 'config-only', + servers[0].serverConfig, + ); + expect(mockCacheMCPServerTools).toHaveBeenCalledWith({ serverName: 'config-only' }); + expect(result).toEqual({ + serverTools: new Map([['config-only', {}]]), + serversWithoutTools: [], + }); + }); +}); describe('reinitMCPServer — customUserVars gating (issue #10969)', () => { const user = { id: 'user-123' }; diff --git a/packages/api/src/index.ts b/packages/api/src/index.ts index d6beb61e67..ddd8cd516c 100644 --- a/packages/api/src/index.ts +++ b/packages/api/src/index.ts @@ -23,6 +23,7 @@ export * from './mcp/errors'; export * from './mcp/cache'; export * from './mcp/tools'; export * from './mcp/catalog/store'; +export * from './mcp/catalog/recovery'; export * from './mcp/assistants'; export * from './mcp/request'; /* Utilities */ diff --git a/packages/api/src/mcp/MCPManager.ts b/packages/api/src/mcp/MCPManager.ts index 5a465d44be..8d658bdecc 100644 --- a/packages/api/src/mcp/MCPManager.ts +++ b/packages/api/src/mcp/MCPManager.ts @@ -264,9 +264,21 @@ export class MCPManager extends UserConnectionManager { */ public async discoverServerTools(args: t.ToolDiscoveryOptions): Promise { const { serverName, user } = args; + const registry = MCPServersRegistry.getInstance(); + const serverConfig = await registry.getServerConfig(serverName, user?.id, args.configServers); + + if (!serverConfig) { + logger.warn('[MCP][Discovery] Server configuration not found'); + return { tools: null, oauthRequired: false, oauthUrl: null }; + } try { - const existingAppConnection = await this.appConnections?.get(serverName); + const useAppConnection = + canUseAppConnection(serverConfig) && + (await registry.isAppServerConfig(serverName, serverConfig)); + const existingAppConnection = useAppConnection + ? await this.appConnections?.get(serverName) + : null; if (existingAppConnection && (await existingAppConnection.isConnected())) { const snapshot = await existingAppConnection.fetchOrderedToolsSnapshot(); return { @@ -279,17 +291,6 @@ export class MCPManager extends UserConnectionManager { logger.debug('[MCP][Discovery] App connection unavailable; trying discovery mode'); } - const serverConfig = await MCPServersRegistry.getInstance().getServerConfig( - serverName, - user?.id, - args.configServers, - ); - - if (!serverConfig) { - logger.warn('[MCP][Discovery] Server configuration not found'); - return { tools: null, oauthRequired: false, oauthUrl: null }; - } - const missingBodyFields = getMissingRuntimeBodyPlaceholderFields( serverConfig, args.requestBody, @@ -301,7 +302,6 @@ export class MCPManager extends UserConnectionManager { return { tools: null, oauthRequired: false, oauthUrl: null }; } - const registry = MCPServersRegistry.getInstance(); const { allowedDomains, allowedAddresses, useSSRFProtection } = await registry.resolveAllowlists({ userId: user?.id, role: user?.role }); await this.assertResolvedRuntimeConfigAllowed({ diff --git a/packages/api/src/mcp/__tests__/MCPManager.test.ts b/packages/api/src/mcp/__tests__/MCPManager.test.ts index 64e1dc7131..2caa1ab518 100644 --- a/packages/api/src/mcp/__tests__/MCPManager.test.ts +++ b/packages/api/src/mcp/__tests__/MCPManager.test.ts @@ -2957,6 +2957,41 @@ describe('MCPManager', () => { expect(MCPConnectionFactory.discoverTools).not.toHaveBeenCalled(); }); + it('does not reuse an app connection for a tenant-scoped config override', async () => { + const configOverride = { + type: 'streamable-http' as const, + url: 'https://tenant.example.com/mcp', + source: 'config' as const, + }; + const appConnections = { get: jest.fn().mockResolvedValue(mockConnection) }; + mockAppConnections(appConnections); + (mockRegistryInstance.getServerConfig as jest.Mock).mockResolvedValue(configOverride); + (mockRegistryInstance.isAppServerConfig as jest.Mock).mockResolvedValue(false); + (MCPConnectionFactory.discoverTools as jest.Mock).mockResolvedValue({ + tools: mockTools, + connection: null, + oauthRequired: false, + oauthUrl: null, + }); + + const manager = await MCPManager.createInstance(newMCPServersConfig()); + const result = await manager.discoverServerTools({ + serverName, + user: { id: 'tenant-user' } as IUser, + configServers: { [serverName]: configOverride }, + }); + + expect(result.tools).toEqual(mockTools); + expect(mockRegistryInstance.getServerConfig).toHaveBeenCalledWith(serverName, 'tenant-user', { + [serverName]: configOverride, + }); + expect(appConnections.get).not.toHaveBeenCalled(); + expect(MCPConnectionFactory.discoverTools).toHaveBeenCalledWith( + expect.objectContaining({ serverConfig: configOverride }), + expect.objectContaining({ user: expect.objectContaining({ id: 'tenant-user' }) }), + ); + }); + it('should use MCPConnectionFactory.discoverTools when no app connection available', async () => { const discoveryConnection = { disconnect: jest.fn().mockResolvedValue(undefined), diff --git a/packages/api/src/mcp/catalog/recovery.spec.ts b/packages/api/src/mcp/catalog/recovery.spec.ts new file mode 100644 index 0000000000..255b606e51 --- /dev/null +++ b/packages/api/src/mcp/catalog/recovery.spec.ts @@ -0,0 +1,484 @@ +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'; + +const user = { id: 'user-1' } as IUser; +const serverConfig = (name: string): ParsedServerConfig => + ({ type: 'streamable-http', url: `https://${name}.example.com/mcp` }) as ParsedServerConfig; +const withUserVars = (config: ParsedServerConfig): ParsedServerConfig => + ({ + ...config, + customUserVars: { API_KEY: { title: 'API key', description: 'Server API key' } }, + }) as ParsedServerConfig; +const availableTools = (name: string): LCAvailableTools => ({ + [name]: { + type: 'function', + function: { + name, + description: '', + parameters: { type: 'object', properties: {} }, + }, + }, +}); + +describe('recoverMCPServerCatalogs', () => { + it('loads user auth once and preserves config-only lookup context for each server', async () => { + const servers = [ + { serverName: 'alpha', serverConfig: withUserVars(serverConfig('alpha')) }, + { serverName: 'beta', serverConfig: withUserVars(serverConfig('beta')) }, + ]; + const loadUserMCPAuthMap = jest.fn().mockResolvedValue({ + [`${Constants.mcp_prefix}alpha`]: { API_KEY: 'alpha-secret' }, + [`${Constants.mcp_prefix}beta`]: { API_KEY: 'beta-secret' }, + }); + const discoverServerTools = jest.fn(async ({ serverName }: ToolDiscoveryOptions) => ({ + tools: [{ name: `${serverName}-tool`, inputSchema: { type: 'object' as const } }], + })); + const formatServerTools = jest.fn((serverName: string) => + availableTools(`tool${Constants.mcp_delimiter}${serverName}`), + ); + + const result = await recoverMCPServerCatalogs( + { user, servers }, + { loadUserMCPAuthMap, discoverServerTools, formatServerTools }, + ); + + expect(loadUserMCPAuthMap).toHaveBeenCalledTimes(1); + expect(loadUserMCPAuthMap).toHaveBeenCalledWith('user-1', ['alpha', 'beta']); + expect(discoverServerTools).toHaveBeenCalledWith( + expect.objectContaining({ + user, + serverName: 'alpha', + configServers: { alpha: servers[0].serverConfig }, + customUserVars: { API_KEY: 'alpha-secret' }, + }), + ); + expect(discoverServerTools).toHaveBeenCalledWith( + expect.objectContaining({ + user, + serverName: 'beta', + configServers: { beta: servers[1].serverConfig }, + customUserVars: { API_KEY: 'beta-secret' }, + }), + ); + expect(result.size).toBe(2); + }); + + it('limits passive discovery to three concurrent servers', async () => { + const servers = Array.from({ length: 7 }, (_, index) => ({ + serverName: `server-${index}`, + serverConfig: serverConfig(`server-${index}`), + })); + let active = 0; + let maxActive = 0; + const discoverServerTools = jest.fn(async () => { + active += 1; + maxActive = Math.max(maxActive, active); + await new Promise((resolve) => setTimeout(resolve, 5)); + active -= 1; + return { tools: [] }; + }); + + const result = await recoverMCPServerCatalogs( + { user, servers }, + { + loadUserMCPAuthMap: jest.fn().mockResolvedValue({}), + discoverServerTools, + formatServerTools: jest.fn().mockReturnValue({}), + }, + ); + + expect(discoverServerTools).toHaveBeenCalledTimes(7); + expect(maxActive).toBe(3); + expect(result.size).toBe(7); + }); + + it('keeps successful catalogs when another server fails or has no authoritative tools', async () => { + const servers = ['good', 'failed', 'missing'].map((serverName) => ({ + serverName, + serverConfig: serverConfig(serverName), + })); + + const result = await recoverMCPServerCatalogs( + { user, servers }, + { + loadUserMCPAuthMap: jest.fn().mockResolvedValue({}), + discoverServerTools: jest.fn(async ({ serverName }: ToolDiscoveryOptions) => { + if (serverName === 'failed') { + throw new Error('offline'); + } + return { tools: serverName === 'missing' ? null : [] }; + }), + formatServerTools: jest.fn().mockReturnValue({}), + }, + ); + + expect([...result.keys()]).toEqual(['good']); + }); +}); + +describe('loadMCPServerCatalogs', () => { + it('loads cache hits and connected snapshots in parallel, then caches only the snapshot', async () => { + const cachedTools = availableTools('cached'); + const snapshotTools = availableTools('live'); + const servers = [ + { serverName: 'cached-server', serverConfig: serverConfig('cached') }, + { serverName: 'live-server', serverConfig: serverConfig('live') }, + ]; + const getCachedServerTools = jest.fn( + async ( + _userId: string, + serverName: string, + _serverConfig: ParsedServerConfig, + ): Promise => (serverName === 'cached-server' ? cachedTools : null), + ); + const getServerToolFunctionsSnapshot = jest.fn().mockResolvedValue({ + tools: snapshotTools, + publicationGeneration: 'generation-1', + }); + const cacheServerTools = jest.fn().mockResolvedValue(undefined); + const loadUserMCPAuthMap = jest.fn(); + + const result = await loadMCPServerCatalogs( + { user, servers }, + { + getCachedServerTools, + getServerToolFunctionsSnapshot, + cacheServerTools, + loadUserMCPAuthMap, + discoverServerTools: jest.fn(), + formatServerTools: jest.fn(), + }, + ); + + expect(getCachedServerTools).toHaveBeenCalledTimes(2); + expect(getServerToolFunctionsSnapshot).toHaveBeenCalledTimes(1); + expect(loadUserMCPAuthMap).not.toHaveBeenCalled(); + expect(cacheServerTools).toHaveBeenCalledWith({ + userId: user.id, + serverName: 'live-server', + serverTools: snapshotTools, + serverConfig: servers[1].serverConfig, + publicationGeneration: 'generation-1', + publicationRevision: undefined, + }); + expect(result.serverTools).toEqual( + new Map([ + ['cached-server', cachedTools], + ['live-server', snapshotTools], + ]), + ); + expect(result.serversWithoutTools).toEqual([]); + }); + + it('serves passive recovery only to the request and does not cache it without a fence', async () => { + const servers = [{ serverName: 'cold-server', serverConfig: serverConfig('cold') }]; + const recoveredTools = availableTools('recovered'); + const cacheServerTools = jest.fn(); + + const result = await loadMCPServerCatalogs( + { user, servers }, + { + getCachedServerTools: jest.fn().mockResolvedValue(null), + getServerToolFunctionsSnapshot: jest.fn().mockResolvedValue({ tools: null }), + cacheServerTools, + loadUserMCPAuthMap: jest.fn().mockResolvedValue({}), + discoverServerTools: jest.fn().mockResolvedValue({ + tools: [{ name: 'recovered', inputSchema: { type: 'object' as const } }], + }), + formatServerTools: jest.fn().mockReturnValue(recoveredTools), + }, + ); + + expect(result.serverTools).toEqual(new Map([['cold-server', recoveredTools]])); + expect(result.serversWithoutTools).toEqual([]); + expect(cacheServerTools).not.toHaveBeenCalled(); + }); + + it('isolates cache and snapshot failures and reports only unresolved servers', async () => { + const servers = [ + { serverName: 'recovered', serverConfig: serverConfig('recovered') }, + { serverName: 'missing', serverConfig: serverConfig('missing') }, + ]; + + const result = await loadMCPServerCatalogs( + { user, servers }, + { + getCachedServerTools: jest.fn().mockRejectedValue(new Error('cache unavailable')), + getServerToolFunctionsSnapshot: jest + .fn() + .mockRejectedValueOnce(new Error('connection unavailable')) + .mockResolvedValueOnce({ tools: null }), + cacheServerTools: jest.fn(), + loadUserMCPAuthMap: jest.fn().mockResolvedValue({}), + discoverServerTools: jest.fn(async ({ serverName }: ToolDiscoveryOptions) => ({ + tools: serverName === 'recovered' ? [] : null, + })), + formatServerTools: jest.fn().mockReturnValue({}), + }, + ); + + expect(result.serverTools).toEqual(new Map([['recovered', {}]])); + expect(result.serversWithoutTools).toEqual(['missing']); + }); +}); + +describe('recoverMCPServerCatalogs — bounded, skippable discovery', () => { + const recoveryDeps = ( + discoverServerTools: jest.Mock, + userMCPAuthMap: Record> = {}, + ) => ({ + loadUserMCPAuthMap: jest.fn().mockResolvedValue(userMCPAuthMap), + discoverServerTools, + formatServerTools: jest.fn().mockReturnValue({}), + }); + + it('bounds each connection attempt the factory makes', 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: 1500 }), + ); + }); + + 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: 900 } as ParsedServerConfig; + + await recoverMCPServerCatalogs( + { user, servers: [{ serverName: 'impatient', serverConfig: impatient }] }, + recoveryDeps(discoverServerTools), + ); + + expect(discoverServerTools).toHaveBeenCalledWith( + expect.objectContaining({ connectionTimeout: 900 }), + ); + }); + + 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, + source: 'config', + } as ParsedServerConfig; + + const result = await recoverMCPServerCatalogs( + { + user, + servers: [ + { serverName: 'failed', serverConfig: failed }, + { serverName: 'healthy', serverConfig: serverConfig('healthy') }, + ], + }, + deps, + ); + + 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 = withUserVars(serverConfig('needs-vars')); + + 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 = withUserVars(serverConfig('needs-vars')); + + 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('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, + source: 'config', + } 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); + }); + + it('still attempts a yaml stub, which has no retry timer of its own', async () => { + const discoverServerTools = jest.fn().mockResolvedValue({ tools: [] }); + const stub = { + ...serverConfig('yaml-stub'), + inspectionFailed: true, + source: 'yaml', + } as ParsedServerConfig; + + await recoverMCPServerCatalogs( + { user, servers: [{ serverName: 'yaml-stub', serverConfig: stub }] }, + recoveryDeps(discoverServerTools), + ); + + expect(discoverServerTools).toHaveBeenCalledWith( + expect.objectContaining({ serverName: 'yaml-stub' }), + ); + }); + + it('reads plugin auth only when a cold server actually declares user variables', async () => { + const discoverServerTools = jest.fn().mockResolvedValue({ tools: [] }); + const deps = recoveryDeps(discoverServerTools); + + await recoverMCPServerCatalogs( + { + user, + servers: [ + { serverName: 'open-a', serverConfig: serverConfig('open-a') }, + { serverName: 'open-b', serverConfig: serverConfig('open-b') }, + ], + }, + deps, + ); + + expect(deps.loadUserMCPAuthMap).not.toHaveBeenCalled(); + expect(discoverServerTools).toHaveBeenCalledTimes(2); + }); + + it('asks plugin auth only for the credential-bearing servers in a mixed list', async () => { + const discoverServerTools = jest.fn().mockResolvedValue({ tools: [] }); + const deps = recoveryDeps(discoverServerTools, { + [`${Constants.mcp_prefix}guarded`]: { API_KEY: 'set' }, + }); + + await recoverMCPServerCatalogs( + { + user, + servers: [ + { serverName: 'open', serverConfig: serverConfig('open') }, + { serverName: 'guarded', serverConfig: withUserVars(serverConfig('guarded')) }, + ], + }, + deps, + ); + + expect(deps.loadUserMCPAuthMap).toHaveBeenCalledWith('user-1', ['guarded']); + expect(discoverServerTools).toHaveBeenCalledTimes(2); + }); + + it('holds a limiter slot until its discovery settles, so concurrency stays honest', async () => { + /** A slot released while its network operation is still running would let a fourth + * discovery start; awaiting the work rather than racing it keeps the limit real. */ + let active = 0; + let maxActive = 0; + const discoverServerTools = jest.fn(async () => { + active += 1; + maxActive = Math.max(maxActive, active); + await new Promise((resolve) => setTimeout(resolve, 20)); + active -= 1; + return { tools: null }; + }); + const servers = Array.from({ length: 9 }, (_, index) => ({ + serverName: `slow-${index}`, + serverConfig: serverConfig(`slow-${index}`), + })); + + const result = await recoverMCPServerCatalogs( + { user, servers }, + { + loadUserMCPAuthMap: jest.fn().mockResolvedValue({}), + discoverServerTools, + formatServerTools: jest.fn().mockReturnValue({}), + }, + ); + + expect(discoverServerTools).toHaveBeenCalledTimes(9); + expect(maxActive).toBe(3); + expect(result.size).toBe(0); + }); + + it('attempts every cold server, so none is starved by those ahead of it', async () => { + const discoverServerTools = jest.fn().mockResolvedValue({ tools: [] }); + const servers = Array.from({ length: 12 }, (_, index) => ({ + serverName: `server-${index}`, + serverConfig: serverConfig(`server-${index}`), + })); + + await recoverMCPServerCatalogs({ user, servers }, recoveryDeps(discoverServerTools)); + await recoverMCPServerCatalogs({ user, servers }, recoveryDeps(discoverServerTools)); + + expect(discoverServerTools).toHaveBeenCalledTimes(24); + for (const { serverName } of servers) { + expect(discoverServerTools).toHaveBeenCalledWith(expect.objectContaining({ serverName })); + } + }); + + it('bounds snapshot refreshes, which each issue a real tools/list', async () => { + let active = 0; + let maxActive = 0; + const servers = Array.from({ length: 9 }, (_, index) => ({ + serverName: `server-${index}`, + serverConfig: serverConfig(`server-${index}`), + })); + + await loadMCPServerCatalogs( + { user, servers }, + { + getCachedServerTools: jest.fn().mockResolvedValue(null), + getServerToolFunctionsSnapshot: jest.fn(async () => { + active += 1; + maxActive = Math.max(maxActive, active); + await new Promise((resolve) => setTimeout(resolve, 20)); + active -= 1; + return { tools: null }; + }), + cacheServerTools: jest.fn(), + loadUserMCPAuthMap: jest.fn().mockResolvedValue({}), + discoverServerTools: jest.fn().mockResolvedValue({ tools: null }), + formatServerTools: jest.fn().mockReturnValue({}), + }, + ); + + expect(maxActive).toBe(3); + }); +}); diff --git a/packages/api/src/mcp/catalog/recovery.ts b/packages/api/src/mcp/catalog/recovery.ts new file mode 100644 index 0000000000..06d61b8b0f --- /dev/null +++ b/packages/api/src/mcp/catalog/recovery.ts @@ -0,0 +1,262 @@ +import { logger } from '@librechat/data-schemas'; +import type { Tool } from '@modelcontextprotocol/sdk/types.js'; +import type { IUser } from '@librechat/data-schemas'; +import type { LCAvailableTools, ParsedServerConfig, ToolDiscoveryOptions } from '../types'; +import { hasCustomUserVars, getMissingCustomUserVars } from '../utils'; +import { createConcurrencyLimiter } from '~/utils/promise'; +import { getServerCustomUserVars } from '../auth'; + +/** Bounds outbound MCP fan-out from one catalog request: snapshot refreshes issue a real + * `tools/list`, so they burst as readily as passive discovery does. */ +const CATALOG_FANOUT_CONCURRENCY = 3; +/** + * Bounds `connection.connect()` only — it is the sole segment of discovery a caller can bound + * today. It does NOT bound the whole operation: `discoverToolsInternal` spends this value once + * per connection attempt (authenticated, then unauthenticated), and `fetchToolsSnapshot` then + * applies its own `TOOLS_LIST_TIMEOUT_MS` (30s) to `tools/list`. A server that connects quickly + * and stalls while listing therefore still holds its slot for that longer window. + * + * Bounding discovery end to end needs a deadline threaded through `MCPConnectionFactory` into + * both `connect()` and `fetchToolsSnapshot()`; until that exists, this keeps the common + * unreachable case cheap, because recovery targets a server that is reachable and authorized but + * whose catalog cache expired, and such a server connects well inside this window. + */ +const RECOVERY_ATTEMPT_TIMEOUT_MS = 1500; + +export interface MCPServerCatalogRecoveryInput { + serverName: string; + serverConfig: ParsedServerConfig; +} + +export interface MCPServerCatalogRecoveryDeps { + loadUserMCPAuthMap: ( + userId: string, + serverNames: readonly string[], + ) => Promise>>; + discoverServerTools: (options: ToolDiscoveryOptions) => Promise<{ tools: Tool[] | null }>; + formatServerTools: (serverName: string, tools: Tool[]) => LCAvailableTools; +} + +export interface MCPServerCatalogSnapshot { + tools: LCAvailableTools | null; + publicationGeneration?: string; + publicationRevision?: string; +} + +export interface MCPServerCatalogLoaderDeps extends MCPServerCatalogRecoveryDeps { + getCachedServerTools: ( + userId: string, + serverName: string, + serverConfig: ParsedServerConfig, + ) => Promise; + getServerToolFunctionsSnapshot: ( + userId: string, + serverName: string, + serverConfig: ParsedServerConfig, + ) => Promise; + cacheServerTools: (params: { + userId: string; + serverName: string; + serverTools: LCAvailableTools; + serverConfig: ParsedServerConfig; + publicationGeneration?: string; + publicationRevision?: string; + }) => Promise; +} + +export interface MCPServerCatalogLoaderResult { + serverTools: Map; + serversWithoutTools: string[]; +} + +interface RecoveryCandidate extends MCPServerCatalogRecoveryInput { + customUserVars?: Record; +} + +/** Bounds one connection attempt, honouring a shorter operator `initTimeout`. */ +function resolveAttemptTimeout(serverConfig: ParsedServerConfig): number { + const { initTimeout } = serverConfig; + if (typeof initTimeout === 'number') { + return Math.min(initTimeout, RECOVERY_ATTEMPT_TIMEOUT_MS); + } + return RECOVERY_ATTEMPT_TIMEOUT_MS; +} + +async function discoverCandidate( + user: IUser, + { serverName, serverConfig, customUserVars }: RecoveryCandidate, + deps: MCPServerCatalogRecoveryDeps, +): Promise<[string, LCAvailableTools | null]> { + try { + const result = await deps.discoverServerTools({ + user, + serverName, + configServers: { [serverName]: serverConfig }, + customUserVars, + connectionTimeout: resolveAttemptTimeout(serverConfig), + }); + return [ + serverName, + result.tools == null ? null : deps.formatServerTools(serverName, result.tools), + ]; + } catch (error) { + logger.error(`[MCP catalog recovery] Failed to discover tools for ${serverName}:`, error); + return [serverName, null]; + } +} + +/** + * Passively discovers cold MCP catalogs for one request. + * + * A recovered catalog cannot be retained: a discovery connection owns no publication generation + * and is disposed, and the tool cache refuses unfenced writes, so the result is served only to + * the requesting user. Recovery therefore stays stateless and individually cheap rather than + * scheduling around a result it is not allowed to keep — it skips only what configuration alone + * proves pointless, and fails fast on everything else. + */ +export async function recoverMCPServerCatalogs( + params: { user: IUser; servers: readonly MCPServerCatalogRecoveryInput[] }, + deps: MCPServerCatalogRecoveryDeps, +): Promise> { + const { user, servers } = params; + /** Only the config tier retries a failed stub on its own clock. A `yaml`- or `user`-sourced + * stub has no such timer, so skipping it unconditionally would hide the server for good — + * exactly the state this recovery exists to escape. */ + const recoverable = servers.filter(({ serverName, serverConfig }) => { + if (!serverConfig.inspectionFailed || serverConfig.source !== 'config') { + return true; + } + logger.debug(`[MCP catalog recovery] Skipping ${serverName}: awaiting config-tier retry`); + return false; + }); + if (recoverable.length === 0) { + return new Map(); + } + + /** Only credential-bearing servers can consume the auth map, so a list without any avoids + * the plugin-auth round trip entirely. */ + const credentialServers = recoverable.filter(({ serverConfig }) => + hasCustomUserVars(serverConfig), + ); + const userMCPAuthMap = credentialServers.length + ? await deps.loadUserMCPAuthMap( + user.id, + credentialServers.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: RecoveryCandidate[] = []; + for (const candidate of recoverable) { + const customUserVars = getServerCustomUserVars(userMCPAuthMap, candidate.serverName); + const missingUserVars = getMissingCustomUserVars(candidate.serverConfig, customUserVars); + if (missingUserVars.length > 0) { + logger.debug( + `[MCP catalog recovery] Skipping ${candidate.serverName}: ${missingUserVars.length} user-provided variable(s) unset`, + ); + continue; + } + authorized.push({ ...candidate, customUserVars }); + } + if (authorized.length === 0) { + return new Map(); + } + + const recover = createConcurrencyLimiter(CATALOG_FANOUT_CONCURRENCY); + const results = await Promise.all( + authorized.map((candidate) => recover(() => discoverCandidate(user, candidate, deps))), + ); + + return new Map(results.filter((entry): entry is [string, LCAvailableTools] => entry[1] != null)); +} + +/** Loads cached, connected, then passive MCP catalogs for a marketplace-style list request. */ +export async function loadMCPServerCatalogs( + params: { user: IUser; servers: readonly MCPServerCatalogRecoveryInput[] }, + deps: MCPServerCatalogLoaderDeps, +): Promise { + const { user, servers } = params; + const cached = await Promise.all( + servers.map(async ({ serverName, serverConfig }) => { + try { + const tools = await deps.getCachedServerTools(user.id, serverName, serverConfig); + return { serverName, serverConfig, tools, source: 'cache' as const }; + } catch (error) { + logger.error(`[MCP catalog loader] Failed to read cached tools for ${serverName}:`, error); + return { serverName, serverConfig, tools: null, source: 'cache' as const }; + } + }), + ); + + /** A snapshot is not a local read — both connection paths issue a fresh `tools/list` — so a + * cache reset across many servers would otherwise burst unbounded outbound requests. */ + const refresh = createConcurrencyLimiter(CATALOG_FANOUT_CONCURRENCY); + const snapshots = await Promise.all( + cached.map((entry) => { + if (entry.tools != null) { + return entry; + } + return refresh(async () => { + try { + const snapshot = await deps.getServerToolFunctionsSnapshot( + user.id, + entry.serverName, + entry.serverConfig, + ); + return { ...entry, ...snapshot, source: 'snapshot' as const }; + } catch (error) { + logger.error( + `[MCP catalog loader] Failed to read connected tools for ${entry.serverName}:`, + error, + ); + return { ...entry, tools: null, source: 'snapshot' as const }; + } + }); + }), + ); + + const coldServers = snapshots + .filter(({ tools }) => tools == null) + .map(({ serverName, serverConfig }) => ({ serverName, serverConfig })); + let recovered = new Map(); + if (coldServers.length > 0) { + try { + recovered = await recoverMCPServerCatalogs({ user, servers: coldServers }, deps); + } catch (error) { + logger.error('[MCP catalog loader] Failed to recover cold server catalogs:', error); + } + } + + const serverTools = new Map(); + const serversWithoutTools: string[] = []; + for (const snapshot of snapshots) { + const tools = snapshot.tools ?? recovered.get(snapshot.serverName); + if (tools == null) { + serversWithoutTools.push(snapshot.serverName); + continue; + } + serverTools.set(snapshot.serverName, tools); + + if (snapshot.source !== 'snapshot' || snapshot.tools == null) { + continue; + } + void deps + .cacheServerTools({ + userId: user.id, + serverName: snapshot.serverName, + serverTools: snapshot.tools, + serverConfig: snapshot.serverConfig, + publicationGeneration: snapshot.publicationGeneration, + publicationRevision: snapshot.publicationRevision, + }) + .catch((error) => + logger.error( + `[MCP catalog loader] Failed to cache tools for ${snapshot.serverName}:`, + error, + ), + ); + } + + return { serverTools, serversWithoutTools }; +} diff --git a/packages/api/src/mcp/tools.ts b/packages/api/src/mcp/tools.ts index 5f58d4bc28..cc68e53ba8 100644 --- a/packages/api/src/mcp/tools.ts +++ b/packages/api/src/mcp/tools.ts @@ -83,6 +83,35 @@ export interface MCPToolCacheService { ) => Promise; } +/** Converts an MCP tools/list response into LibreChat's server-qualified catalog format. */ +export function formatMCPServerTools(serverName: string, tools: MCPToolInput[]): LCAvailableTools { + const serverTools: LCAvailableTools = {}; + const keyServerName = normalizeServerName(serverName); + const keyToolNames = stripServerNamePrefixes( + tools.map((tool) => tool.name), + keyServerName, + ); + for (const tool of tools) { + const keyToolName = keyToolNames.get(tool.name) ?? tool.name; + const name = `${keyToolName}${Constants.mcp_delimiter}${keyServerName}`; + const entry: LCFunctionTool = { + type: 'function', + ['function']: { + name, + description: tool.description ?? '', + parameters: tool.inputSchema + ? (normalizeJsonSchema(resolveJsonSchemaRefs(tool.inputSchema)) as JsonSchemaType) + : ({ type: 'object', properties: {} } as JsonSchemaType), + }, + }; + if (keyToolName !== tool.name) { + entry.serverToolName = tool.name; + } + serverTools[name] = entry; + } + return serverTools; +} + interface AppServerBoundary { serverName: string; suffix: string; @@ -222,42 +251,12 @@ export function createMCPToolCacheService(deps: MCPToolCacheDeps): MCPToolCacheS const { userId, serverName, tools, serverConfig, publicationGeneration, publicationRevision } = params; try { - const serverTools: LCAvailableTools = {}; - const mcpDelimiter = Constants.mcp_delimiter; - if (tools == null) { logger.debug('[MCP Cache] No tools to update'); - return serverTools; - } - - /** Cache keys are MODEL-FACING: they become builder tool ids, agent.tools - * entries, tool_options keys, and definition names, and must equal the - * runtime instance name (`createToolInstance` in MCP.js), which embeds - * `normalizeServerName(serverName)`. The cache STORE itself stays keyed - * by the raw config name. */ - const keyServerName = normalizeServerName(serverName); - const keyToolNames = stripServerNamePrefixes( - tools.map((tool) => tool.name), - keyServerName, - ); - for (const tool of tools) { - const keyToolName = keyToolNames.get(tool.name) ?? tool.name; - const name = `${keyToolName}${mcpDelimiter}${keyServerName}`; - const entry: LCFunctionTool = { - type: 'function', - ['function']: { - name, - description: tool.description ?? '', - parameters: tool.inputSchema - ? (normalizeJsonSchema(resolveJsonSchemaRefs(tool.inputSchema)) as JsonSchemaType) - : ({ type: 'object', properties: {} } as JsonSchemaType), - }, - }; - if (keyToolName !== tool.name) { - entry.serverToolName = tool.name; - } - serverTools[name] = entry; + return {}; } + /** Cache keys are model-facing and must match runtime tool instance names. */ + const serverTools = formatMCPServerTools(serverName, tools); const resolvedConfig = await resolveCacheConfig(userId, serverName, serverConfig); const configGeneration = resolvedConfig