From ad13e89b2c33c6da2a7df713dbe24cf733610096 Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Fri, 14 Aug 2026 23:45:17 -0400 Subject: [PATCH 1/6] =?UTF-8?q?=F0=9F=93=A1=20fix:=20Publish=20App-Level?= =?UTF-8?q?=20MCP=20Tool=20Catalogs=20Without=20a=20Reserved=20Revision?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Shared MCP servers advertised no tools to agents, so every turn failed with "configured to use MCP tools, but none are available" (#14857). `replaceAppServerTools` returned false whenever a publication carried no `publicationRevision`, but only `refreshChangedTools` reserves one. Every other app-level publisher — the first-connect snapshot, reinitialization, on-demand catalog reads, the retained-catalog restore — was silently dropped. The agent path fails closed on that drop: the skipped write returns null, so reinitialize yields no tools and the turn 503s. Startup hid it. `connectAppServers()` defers the initial refresh and calls `refreshToolList()` itself, which does reserve, so a boot that reaches its MCP servers looks healthy. Only a lazily created app connection — the server not yet up when LibreChat boots, a dropped connection, a cold cache — takes the unreserved path. `ConnectionsRepository` now reserves before its own `tools/list`, matching the list_changed path; a failed reservation publishes unordered rather than failing the connection. Publishers with no pre-fetch reservation point have already fetched by the time they reach the cache, so they take the next revision at write time instead of being discarded. `mergeAppTools` still publishes at revision 0 and stays deferential to a live catalog. --- api/server/services/Config/mcp.js | 1 + packages/api/src/mcp/ConnectionsRepository.ts | 29 +++- .../__tests__/ConnectionsRepository.test.ts | 43 +++++- .../appToolPublication.integration.test.ts | 141 ++++++++++++++++++ packages/api/src/mcp/tools.spec.ts | 47 +++++- packages/api/src/mcp/tools.ts | 22 ++- 6 files changed, 276 insertions(+), 7 deletions(-) create mode 100644 packages/api/src/mcp/__tests__/appToolPublication.integration.test.ts diff --git a/api/server/services/Config/mcp.js b/api/server/services/Config/mcp.js index 2a6a66d8fa7..968de7d7ba6 100644 --- a/api/server/services/Config/mcp.js +++ b/api/server/services/Config/mcp.js @@ -24,6 +24,7 @@ const { setCachedToolsIfCurrent, getCachedAppServerTools, setCachedAppServerTools, + getNextAppToolsPublicationRevision, getServerConfig: (serverName, userId) => MCPServersRegistry.getInstance().getServerConfig(serverName, userId), getAllServerConfigs: () => MCPServersRegistry.getInstance().getAllServerConfigs(), diff --git a/packages/api/src/mcp/ConnectionsRepository.ts b/packages/api/src/mcp/ConnectionsRepository.ts index 63e9396192d..3c2478f8176 100644 --- a/packages/api/src/mcp/ConnectionsRepository.ts +++ b/packages/api/src/mcp/ConnectionsRepository.ts @@ -2,8 +2,9 @@ import { logger } from '@librechat/data-schemas'; import type * as t from './types'; import { cancelMCPToolsChanged, - getMCPAppToolsPublicationGeneration, notifyMCPToolsChanged, + reserveMCPToolsChangedRevision, + getMCPAppToolsPublicationGeneration, } from './toolsChanged'; import { MCPServersRegistry } from '~/mcp/registry/MCPServersRegistry'; import { MCPConnectionFactory } from '~/mcp/MCPConnectionFactory'; @@ -162,12 +163,21 @@ export class ConnectionsRepository { this.connections.set(serverName, connection); if (this.ownerId === undefined && options.refreshTools !== false) { + /** Reserved before `tools/list` runs so a first-connect publication is ordered against + * concurrent replicas exactly as a list_changed refresh is. An app-level catalog write + * carrying no revision cannot be ordered, and agents saw an empty catalog forever when + * this path was the one that populated it (#14857). */ + const publicationRevision = await this.reserveToolsPublicationRevision( + serverName, + serverConfig, + ); if (connection.client.getServerCapabilities()?.tools == null) { await notifyMCPToolsChanged({ tools: [], serverName, serverConfig, publicationGeneration, + publicationRevision, }); return connection; } @@ -182,6 +192,7 @@ export class ConnectionsRepository { serverName, serverConfig, publicationGeneration, + publicationRevision, }); } } else { @@ -191,6 +202,22 @@ export class ConnectionsRepository { return connection; } + /** Orders a first-connect publication; a failed reservation must not fail the connection. */ + private async reserveToolsPublicationRevision( + serverName: string, + serverConfig: t.ParsedServerConfig, + ): Promise { + try { + return await reserveMCPToolsChangedRevision({ serverName, serverConfig }); + } catch (error) { + logger.warn( + `${this.prefix(serverName)} Failed to reserve tool-list publication order; publishing unordered`, + error, + ); + return undefined; + } + } + /** Gets or creates connections for multiple servers concurrently */ async getMany( serverNames: string[], diff --git a/packages/api/src/mcp/__tests__/ConnectionsRepository.test.ts b/packages/api/src/mcp/__tests__/ConnectionsRepository.test.ts index 5105df396ec..3e7dce7e9b3 100644 --- a/packages/api/src/mcp/__tests__/ConnectionsRepository.test.ts +++ b/packages/api/src/mcp/__tests__/ConnectionsRepository.test.ts @@ -1,6 +1,10 @@ import { logger } from '@librechat/data-schemas'; import type * as t from '~/mcp/types'; -import { getMCPAppToolsPublicationGeneration, setMCPToolsChangedHandler } from '~/mcp/toolsChanged'; +import { + setMCPToolsChangedHandler, + setMCPToolsChangedRevisionHandler, + getMCPAppToolsPublicationGeneration, +} from '~/mcp/toolsChanged'; import { ConnectionsRepository } from '~/mcp/ConnectionsRepository'; import { MCPConnectionFactory } from '~/mcp/MCPConnectionFactory'; import { MCPConnection } from '~/mcp/connection'; @@ -101,6 +105,7 @@ describe('ConnectionsRepository', () => { afterEach(() => { setMCPToolsChangedHandler(null); + setMCPToolsChangedRevisionHandler(null); jest.clearAllMocks(); }); @@ -185,6 +190,42 @@ describe('ConnectionsRepository', () => { await expect(load).resolves.toBe(mockConnection); }); + /** Without a revision the catalog write is unordered, and app-level publications that + * cannot be ordered never reach the cache agents read from (#14857). */ + it('orders the initial app publication with a revision reserved before the fetch', async () => { + const sequence: string[] = []; + setMCPToolsChangedRevisionHandler(() => { + sequence.push('reserve'); + return '4'; + }); + mockConnection.fetchToolsSnapshot.mockImplementation(async () => { + sequence.push('fetch'); + return { tools: [], complete: true }; + }); + const handler = jest.fn(); + setMCPToolsChangedHandler(handler); + + await repository.get('server1'); + + expect(sequence).toEqual(['reserve', 'fetch']); + expect(handler).toHaveBeenCalledWith( + expect.objectContaining({ serverName: 'server1', publicationRevision: '4' }), + ); + }); + + it('still publishes the initial app snapshot when the reservation fails', async () => { + setMCPToolsChangedRevisionHandler(() => { + throw new Error('revision store unavailable'); + }); + const handler = jest.fn(); + setMCPToolsChangedHandler(handler); + + await expect(repository.get('server1')).resolves.toBe(mockConnection); + expect(handler).toHaveBeenCalledWith( + expect.objectContaining({ serverName: 'server1', publicationRevision: undefined }), + ); + }); + it('can defer the initial app tool refresh for startup synchronization', async () => { await repository.get('server1', { refreshTools: false }); diff --git a/packages/api/src/mcp/__tests__/appToolPublication.integration.test.ts b/packages/api/src/mcp/__tests__/appToolPublication.integration.test.ts new file mode 100644 index 00000000000..f9fb268a20e --- /dev/null +++ b/packages/api/src/mcp/__tests__/appToolPublication.integration.test.ts @@ -0,0 +1,141 @@ +/** Regression coverage for app-level MCP catalogs that never reach agents (#14857). */ +import { CacheKeys, Constants, normalizeServerName } from 'librechat-data-provider'; +import type { LCAvailableTools, ParsedServerConfig } from '../types'; +import { getMCPAppToolsPublicationGeneration } from '../toolsChanged'; +import { createMCPCatalogStore } from '../catalog/store'; +import { createMCPToolCacheService } from '../tools'; + +const SERVER_NAME = 'shared'; + +const appConfig: ParsedServerConfig = { + type: 'streamable-http', + url: 'https://mcp.example.com/mcp', + source: 'yaml', +}; + +const toolKey = (name: string) => + `${name}${Constants.mcp_delimiter}${normalizeServerName(SERVER_NAME)}`; + +const description = (name: string) => `${name} the corpus`; + +const catalogOf = (...names: string[]): LCAvailableTools => + Object.fromEntries( + names.map((name) => [ + toolKey(name), + { + type: 'function' as const, + ['function']: { + name: toolKey(name), + description: description(name), + parameters: { type: 'object' as const, properties: {} }, + }, + }, + ]), + ); + +function createService() { + const cache = new Map(); + const store = createMCPCatalogStore({ + cacheConfig: { FORCED_IN_MEMORY_CACHE_NAMESPACES: [CacheKeys.TOOL_CACHE] }, + getCache: () => ({ + get: async (key) => cache.get(key), + set: async (key, value) => { + cache.set(key, value); + return true; + }, + delete: async (key) => cache.delete(key), + }), + }); + + const service = createMCPToolCacheService({ + getCachedTools: store.getCachedTools, + updateCachedGlobalTools: store.updateCachedGlobalTools, + setCachedTools: store.setCachedTools, + setCachedToolsIfCurrent: store.setCachedToolsIfCurrent, + getCachedAppServerTools: store.getCachedAppServerTools, + setCachedAppServerTools: store.setCachedAppServerTools, + getNextAppToolsPublicationRevision: store.getNextAppToolsPublicationRevision, + getServerConfig: async () => appConfig, + getAllServerConfigs: async () => ({ [SERVER_NAME]: appConfig }), + isAppServerConfig: async () => true, + }); + + return { service, store }; +} + +describe('app-level tool publication', () => { + const configGeneration = getMCPAppToolsPublicationGeneration(appConfig); + + it('publishes a first-connect snapshot that reserved no revision', async () => { + const { service } = createService(); + + await expect( + service.replaceAppServerTools({ + serverName: SERVER_NAME, + serverTools: catalogOf('search'), + publicationGeneration: configGeneration, + }), + ).resolves.toBe(true); + + await expect(service.getMCPServerTools('user-1', SERVER_NAME, appConfig)).resolves.toEqual( + catalogOf('search'), + ); + }); + + /** The reinitialize path an agent falls back to when the shared catalog is cold. Returning + * null here is what surfaced as "configured to use MCP tools, but none are available". */ + it('returns the catalog when a user request republishes a shared server', async () => { + const { service } = createService(); + + await expect( + service.updateMCPServerTools({ + userId: 'user-1', + serverName: SERVER_NAME, + serverConfig: appConfig, + publicationGeneration: configGeneration, + tools: [{ name: 'search', description: description('search') }], + }), + ).resolves.toEqual(catalogOf('search')); + + await expect(service.getMCPServerTools('user-1', SERVER_NAME, appConfig)).resolves.toEqual( + catalogOf('search'), + ); + }); + + it('caches a shared catalog discovered on demand', async () => { + const { service } = createService(); + + await service.cacheMCPServerTools({ + userId: 'user-1', + serverName: SERVER_NAME, + serverConfig: appConfig, + serverTools: catalogOf('search'), + publicationGeneration: configGeneration, + }); + + await expect(service.getMCPServerTools('user-2', SERVER_NAME, appConfig)).resolves.toEqual( + catalogOf('search'), + ); + }); + + it('keeps a newer catalog when a publication that reserved earlier lands last', async () => { + const { service, store } = createService(); + const stale = await store.getNextAppToolsPublicationRevision(SERVER_NAME, configGeneration); + + await service.replaceAppServerTools({ + serverName: SERVER_NAME, + serverTools: catalogOf('current'), + publicationGeneration: configGeneration, + }); + await service.replaceAppServerTools({ + serverName: SERVER_NAME, + serverTools: catalogOf('stale'), + publicationGeneration: configGeneration, + publicationRevision: stale, + }); + + await expect(service.getMCPServerTools('user-1', SERVER_NAME, appConfig)).resolves.toEqual( + catalogOf('current'), + ); + }); +}); diff --git a/packages/api/src/mcp/tools.spec.ts b/packages/api/src/mcp/tools.spec.ts index 9f314cff916..9179e4607a1 100644 --- a/packages/api/src/mcp/tools.spec.ts +++ b/packages/api/src/mcp/tools.spec.ts @@ -157,7 +157,52 @@ describe('createMCPToolCacheService', () => { ).rejects.toThrow('Redis down'); }); - it('does not publish a live app snapshot without pre-fetch ordering', async () => { + it('orders a publication that could not reserve ordering before its fetch', async () => { + const setCachedAppServerTools = jest.fn().mockResolvedValue(true); + const deps = createMockDeps({ + setCachedAppServerTools, + getNextAppToolsPublicationRevision: jest.fn().mockResolvedValue('7'), + }); + + await expect( + createMCPToolCacheService(deps).replaceAppServerTools({ + serverName: 'dynamic', + serverTools: {}, + publicationGeneration: 'config-generation', + }), + ).resolves.toBe(true); + + expect(deps.getNextAppToolsPublicationRevision).toHaveBeenCalledWith( + 'dynamic', + 'config-generation', + ); + expect(setCachedAppServerTools).toHaveBeenCalledWith('dynamic', 'config-generation', {}, '7'); + }); + + it('keeps a reserved revision instead of allocating a later one', async () => { + const deps = createMockDeps({ + getNextAppToolsPublicationRevision: jest.fn().mockResolvedValue('7'), + }); + + await expect( + createMCPToolCacheService(deps).replaceAppServerTools({ + serverName: 'dynamic', + serverTools: {}, + publicationGeneration: 'config-generation', + publicationRevision: '3', + }), + ).resolves.toBe(true); + + expect(deps.getNextAppToolsPublicationRevision).not.toHaveBeenCalled(); + expect(deps.setCachedAppServerTools).toHaveBeenCalledWith( + 'dynamic', + 'config-generation', + {}, + '3', + ); + }); + + it('does not publish an unordered snapshot when no revision allocator is wired', async () => { const deps = createMockDeps(); await expect( diff --git a/packages/api/src/mcp/tools.ts b/packages/api/src/mcp/tools.ts index 35dfd39fcd6..e90fd98ea7d 100644 --- a/packages/api/src/mcp/tools.ts +++ b/packages/api/src/mcp/tools.ts @@ -41,6 +41,10 @@ export interface MCPToolCacheDeps { tools: LCAvailableTools, publicationRevision?: string, ) => Promise; + getNextAppToolsPublicationRevision?: ( + serverName: string, + configGeneration: string, + ) => Promise; getServerConfig: (serverName: string, userId?: string) => Promise; getAllServerConfigs?: () => Promise>; isAppServerConfig?: (serverName: string, effectiveConfig: ParsedServerConfig) => Promise; @@ -91,6 +95,7 @@ export function createMCPToolCacheService(deps: MCPToolCacheDeps): MCPToolCacheS setCachedToolsIfCurrent, getCachedAppServerTools, setCachedAppServerTools, + getNextAppToolsPublicationRevision, getServerConfig, getAllServerConfigs, isAppServerConfig, @@ -372,19 +377,28 @@ export function createMCPToolCacheService(deps: MCPToolCacheDeps): MCPToolCacheS logger.debug(`[MCP Cache] Skipped unaddressed app-level publication for ${serverName}`); return false; } - if (!publicationRevision) { - logger.debug(`[MCP Cache] Skipped unordered app-level publication for ${serverName}`); + /** Only a list_changed refresh can reserve ordering before its `tools/list` starts. Every + * other publisher — first connect, reinitialization, on-demand catalog reads — has already + * fetched by the time it gets here, so it takes the next revision now rather than being + * dropped; dropping left agents with a permanently empty app catalog (#14857). */ + const revision = + publicationRevision ?? + (await getNextAppToolsPublicationRevision?.(serverName, configGeneration)); + if (!revision) { + logger.debug( + `[MCP Cache] Skipped unordered app-level publication for ${serverName}: no revision allocator is configured`, + ); return false; } const replaced = await setCachedAppServerTools( serverName, configGeneration, serverTools, - publicationRevision, + revision, ); if (replaced === false) { logger.debug( - `[MCP Cache] Ignored superseded app-level tools for ${serverName} at revision ${publicationRevision ?? '0'}`, + `[MCP Cache] Ignored superseded app-level tools for ${serverName} at revision ${revision}`, ); return false; } From eadb8a894166aa02a3a2a5576d56c03bd9beecc4 Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Sat, 15 Aug 2026 08:38:11 -0400 Subject: [PATCH 2/6] =?UTF-8?q?=F0=9F=93=A1=20fix:=20Bind=20App=20Catalog?= =?UTF-8?q?=20Ordering=20to=20the=20Fetch=20That=20Produced=20It?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses review feedback on the previous commit: allocating a revision at publish time lets a slow `tools/list` of an old catalog outrank a newer one that reserved after it started, and it would let the retained-catalog restore — which republishes deliberately pre-mutation data — outrank a live catalog. Ordering now travels with the data. `fetchToolsSnapshot` reserves before its first page and returns the ticket on the snapshot, so every app-level publisher reads the revision belonging to the read it is publishing rather than one allocated at an unrelated moment. `fetchOrderedToolsSnapshot` carries the refresh's revision when it defers to one, since that is whose catalog it returns. With the reservation at the single point where app-level tools are read, no publisher can forget it, so `replaceAppServerTools` goes back to refusing an unordered write: a publication that lost its ticket fetched at an unknown time and cannot be ordered. A failed reservation is reported as `orderingUnavailable` rather than swallowed, which keeps the list_changed path retrying instead of publishing a catalog that would be silently dropped, and leaves inspection unaffected by a transient cache outage. `MCPServerInspector.getToolFunctions` becomes `getToolCatalog` and returns the revision with the tools, so there is no variant that quietly discards ordering. --- api/server/controllers/mcp.js | 17 +- api/server/services/Config/mcp.js | 1 - api/server/services/Tools/mcp.js | 5 + packages/api/src/mcp/ConnectionsRepository.ts | 36 +-- packages/api/src/mcp/MCPManager.ts | 7 +- .../__tests__/ConnectionsRepository.test.ts | 35 ++- .../api/src/mcp/__tests__/MCPManager.test.ts | 37 ++-- .../appToolPublication.integration.test.ts | 206 +++++++++++------- packages/api/src/mcp/assistants.ts | 3 + packages/api/src/mcp/connection.ts | 89 +++++--- .../src/mcp/registry/MCPServerInspector.ts | 18 +- .../__tests__/MCPServerInspector.test.ts | 23 +- packages/api/src/mcp/tools.spec.ts | 49 +---- packages/api/src/mcp/tools.ts | 24 +- 14 files changed, 292 insertions(+), 258 deletions(-) diff --git a/api/server/controllers/mcp.js b/api/server/controllers/mcp.js index 61d6a6bdda3..1eb1b6eb3d0 100644 --- a/api/server/controllers/mcp.js +++ b/api/server/controllers/mcp.js @@ -219,13 +219,17 @@ const getMCPTools = async (req, res) => { let serverTools; let publicationGeneration; + let publicationRevision; try { - ({ tools: serverTools, publicationGeneration } = - await mcpManager.getServerToolFunctionsSnapshot( - userId, - serverName, - mcpConfig[serverName], - )); + ({ + tools: serverTools, + publicationGeneration, + publicationRevision, + } = await mcpManager.getServerToolFunctionsSnapshot( + userId, + serverName, + mcpConfig[serverName], + )); } catch (error) { logger.error(`[getMCPTools] Error fetching tools for server ${serverName}:`, error); continue; @@ -243,6 +247,7 @@ const getMCPTools = async (req, res) => { serverTools, serverConfig: mcpConfig[serverName], publicationGeneration, + publicationRevision, }).catch((err) => logger.error(`[getMCPTools] Failed to cache tools for ${serverName}:`, err), ); diff --git a/api/server/services/Config/mcp.js b/api/server/services/Config/mcp.js index 968de7d7ba6..2a6a66d8fa7 100644 --- a/api/server/services/Config/mcp.js +++ b/api/server/services/Config/mcp.js @@ -24,7 +24,6 @@ const { setCachedToolsIfCurrent, getCachedAppServerTools, setCachedAppServerTools, - getNextAppToolsPublicationRevision, getServerConfig: (serverName, userId) => MCPServersRegistry.getInstance().getServerConfig(serverName, userId), getAllServerConfigs: () => MCPServersRegistry.getInstance().getAllServerConfigs(), diff --git a/api/server/services/Tools/mcp.js b/api/server/services/Tools/mcp.js index 15606944190..2a6dd5e810f 100644 --- a/api/server/services/Tools/mcp.js +++ b/api/server/services/Tools/mcp.js @@ -66,6 +66,7 @@ async function reinitMCPServer({ let oauthExpiresAt; let ephemeralServer = false; let publicationGeneration; + let publicationRevision; try { const registry = getMCPServersRegistry(); @@ -279,6 +280,9 @@ async function reinitMCPServer({ } if (snapshot.complete) { tools = snapshot.tools; + /** Reserved before this snapshot's tools/list; an app-level catalog cannot publish + * without it, and allocating a later one here would outrank fresher tools. */ + publicationRevision = snapshot.publicationRevision; } else { logger.warn( `[MCP Reinitialize] Preserving cached tools for ${serverName} because tools/list returned an incomplete snapshot`, @@ -306,6 +310,7 @@ async function reinitMCPServer({ tools, serverConfig, ...(publicationGeneration && { publicationGeneration }), + ...(publicationRevision && { publicationRevision }), }); if (availableTools == null) { tools = null; diff --git a/packages/api/src/mcp/ConnectionsRepository.ts b/packages/api/src/mcp/ConnectionsRepository.ts index 3c2478f8176..9e1a5ba06b8 100644 --- a/packages/api/src/mcp/ConnectionsRepository.ts +++ b/packages/api/src/mcp/ConnectionsRepository.ts @@ -2,9 +2,8 @@ import { logger } from '@librechat/data-schemas'; import type * as t from './types'; import { cancelMCPToolsChanged, - notifyMCPToolsChanged, - reserveMCPToolsChangedRevision, getMCPAppToolsPublicationGeneration, + notifyMCPToolsChanged, } from './toolsChanged'; import { MCPServersRegistry } from '~/mcp/registry/MCPServersRegistry'; import { MCPConnectionFactory } from '~/mcp/MCPConnectionFactory'; @@ -163,15 +162,12 @@ export class ConnectionsRepository { this.connections.set(serverName, connection); if (this.ownerId === undefined && options.refreshTools !== false) { - /** Reserved before `tools/list` runs so a first-connect publication is ordered against - * concurrent replicas exactly as a list_changed refresh is. An app-level catalog write - * carrying no revision cannot be ordered, and agents saw an empty catalog forever when - * this path was the one that populated it (#14857). */ - const publicationRevision = await this.reserveToolsPublicationRevision( - serverName, - serverConfig, - ); + /** The snapshot carries ordering reserved before its own `tools/list`, so this + * first-connect publication is ordered against concurrent replicas exactly as a + * list_changed refresh is. An app-level write that cannot be ordered is dropped, which + * left agents with a permanently empty catalog when this path populated it (#14857). */ if (connection.client.getServerCapabilities()?.tools == null) { + const { publicationRevision } = await connection.reserveToolsPublicationRevision(); await notifyMCPToolsChanged({ tools: [], serverName, @@ -183,7 +179,7 @@ export class ConnectionsRepository { } const initialGeneration = toolsChangedGeneration; const snapshot = await connection.fetchToolsSnapshot(); - if (snapshot.complete) { + if (snapshot.complete && !snapshot.orderingUnavailable) { if (toolsChangedGeneration !== initialGeneration) { await latestToolsChangedPublication; } else { @@ -192,7 +188,7 @@ export class ConnectionsRepository { serverName, serverConfig, publicationGeneration, - publicationRevision, + publicationRevision: snapshot.publicationRevision, }); } } else { @@ -202,22 +198,6 @@ export class ConnectionsRepository { return connection; } - /** Orders a first-connect publication; a failed reservation must not fail the connection. */ - private async reserveToolsPublicationRevision( - serverName: string, - serverConfig: t.ParsedServerConfig, - ): Promise { - try { - return await reserveMCPToolsChangedRevision({ serverName, serverConfig }); - } catch (error) { - logger.warn( - `${this.prefix(serverName)} Failed to reserve tool-list publication order; publishing unordered`, - error, - ); - return undefined; - } - } - /** Gets or creates connections for multiple servers concurrently */ async getMany( serverNames: string[], diff --git a/packages/api/src/mcp/MCPManager.ts b/packages/api/src/mcp/MCPManager.ts index 6741ded3aa1..df6a1d02acb 100644 --- a/packages/api/src/mcp/MCPManager.ts +++ b/packages/api/src/mcp/MCPManager.ts @@ -422,6 +422,7 @@ export class MCPManager extends UserConnectionManager { ): Promise<{ tools: t.LCAvailableTools | null; publicationGeneration?: string; + publicationRevision?: string; }> { try { const registry = MCPServersRegistry.getInstance(); @@ -434,9 +435,7 @@ export class MCPManager extends UserConnectionManager { ? await this.appConnections?.get(serverName) : null; if (existingAppConnection != null) { - return { - tools: await MCPServerInspector.getToolFunctions(serverName, existingAppConnection), - }; + return MCPServerInspector.getToolCatalog(serverName, existingAppConnection); } let awaitedRecovery: Promise | undefined; @@ -482,7 +481,7 @@ export class MCPManager extends UserConnectionManager { } try { - const tools = await MCPServerInspector.getToolFunctions(serverName, connection); + const { tools } = await MCPServerInspector.getToolCatalog(serverName, connection); const generationAfterFetch = await getMCPToolsChangedGeneration({ userId, serverName }); if ( publicationGeneration != null && diff --git a/packages/api/src/mcp/__tests__/ConnectionsRepository.test.ts b/packages/api/src/mcp/__tests__/ConnectionsRepository.test.ts index 3e7dce7e9b3..9237b2d093f 100644 --- a/packages/api/src/mcp/__tests__/ConnectionsRepository.test.ts +++ b/packages/api/src/mcp/__tests__/ConnectionsRepository.test.ts @@ -87,6 +87,7 @@ describe('ConnectionsRepository', () => { disconnect: jest.fn().mockResolvedValue(undefined), dispose: jest.fn().mockResolvedValue(undefined), fetchToolsSnapshot: jest.fn().mockResolvedValue({ tools: [], complete: true }), + reserveToolsPublicationRevision: jest.fn().mockResolvedValue({}), refreshToolList: jest.fn().mockResolvedValue(undefined), createdAt: Date.now(), isStale: jest.fn().mockReturnValue(false), @@ -190,39 +191,37 @@ describe('ConnectionsRepository', () => { await expect(load).resolves.toBe(mockConnection); }); - /** Without a revision the catalog write is unordered, and app-level publications that - * cannot be ordered never reach the cache agents read from (#14857). */ - it('orders the initial app publication with a revision reserved before the fetch', async () => { - const sequence: string[] = []; - setMCPToolsChangedRevisionHandler(() => { - sequence.push('reserve'); - return '4'; - }); - mockConnection.fetchToolsSnapshot.mockImplementation(async () => { - sequence.push('fetch'); - return { tools: [], complete: true }; + /** An app-level write that carries no revision cannot be ordered and is dropped, so the + * ordering reserved for this snapshot has to reach the publication (#14857). */ + it('publishes the initial app snapshot under the revision it was fetched with', async () => { + mockConnection.fetchToolsSnapshot.mockResolvedValue({ + tools: [], + complete: true, + publicationRevision: '4', }); const handler = jest.fn(); setMCPToolsChangedHandler(handler); await repository.get('server1'); - expect(sequence).toEqual(['reserve', 'fetch']); expect(handler).toHaveBeenCalledWith( expect.objectContaining({ serverName: 'server1', publicationRevision: '4' }), ); }); - it('still publishes the initial app snapshot when the reservation fails', async () => { - setMCPToolsChangedRevisionHandler(() => { - throw new Error('revision store unavailable'); - }); + it('reserves ordering for a server that advertises no tools capability', async () => { + (mockConnection.client.getServerCapabilities as jest.Mock).mockReturnValue({}); + mockConnection.reserveToolsPublicationRevision = jest + .fn() + .mockResolvedValue({ publicationRevision: '9' }); const handler = jest.fn(); setMCPToolsChangedHandler(handler); - await expect(repository.get('server1')).resolves.toBe(mockConnection); + await repository.get('server1'); + + expect(mockConnection.fetchToolsSnapshot).not.toHaveBeenCalled(); expect(handler).toHaveBeenCalledWith( - expect.objectContaining({ serverName: 'server1', publicationRevision: undefined }), + expect.objectContaining({ serverName: 'server1', tools: [], publicationRevision: '9' }), ); }); diff --git a/packages/api/src/mcp/__tests__/MCPManager.test.ts b/packages/api/src/mcp/__tests__/MCPManager.test.ts index 9a07b7e6f47..f9c02e7bb63 100644 --- a/packages/api/src/mcp/__tests__/MCPManager.test.ts +++ b/packages/api/src/mcp/__tests__/MCPManager.test.ts @@ -532,7 +532,7 @@ describe('MCPManager', () => { describe('getServerToolFunctions', () => { it('should catch and handle errors gracefully', async () => { - (MCPServerInspector.getToolFunctions as jest.Mock) = jest.fn(() => { + (MCPServerInspector.getToolCatalog as jest.Mock) = jest.fn(() => { throw new Error('Connection failed'); }); @@ -552,7 +552,7 @@ describe('MCPManager', () => { }); it('should catch synchronous errors from getUserConnections', async () => { - (MCPServerInspector.getToolFunctions as jest.Mock) = jest.fn().mockResolvedValue({}); + (MCPServerInspector.getToolCatalog as jest.Mock) = jest.fn().mockResolvedValue({ tools: {} }); mockAppConnections({ get: jest.fn().mockResolvedValue(null), @@ -586,9 +586,9 @@ describe('MCPManager', () => { }, }; - (MCPServerInspector.getToolFunctions as jest.Mock) = jest + (MCPServerInspector.getToolCatalog as jest.Mock) = jest .fn() - .mockResolvedValue(expectedTools); + .mockResolvedValue({ tools: expectedTools }); mockAppConnections({ has: jest.fn().mockResolvedValue(true), @@ -608,7 +608,9 @@ describe('MCPManager', () => { finishInspection = resolve; }); const connection = {} as MCPConnection; - (MCPServerInspector.getToolFunctions as jest.Mock) = jest.fn().mockReturnValue(inspection); + (MCPServerInspector.getToolCatalog as jest.Mock) = jest + .fn() + .mockReturnValue(inspection.then((tools) => ({ tools }))); mockAppConnections({ get: jest.fn().mockResolvedValue(null), }); @@ -643,7 +645,7 @@ describe('MCPManager', () => { const recovery = new Promise((resolve) => { resolveRecovery = resolve; }); - (MCPServerInspector.getToolFunctions as jest.Mock) = jest.fn().mockResolvedValue({}); + (MCPServerInspector.getToolCatalog as jest.Mock) = jest.fn().mockResolvedValue({ tools: {} }); mockAppConnections({ get: jest.fn().mockResolvedValue(null), }); @@ -665,14 +667,14 @@ describe('MCPManager', () => { const toolsPromise = manager.getServerToolFunctions(userId, serverName); await new Promise((resolve) => setImmediate(resolve)); - expect(MCPServerInspector.getToolFunctions).not.toHaveBeenCalled(); + expect(MCPServerInspector.getToolCatalog).not.toHaveBeenCalled(); internals.userConnections.set(userId, new Map([[serverName, recoveredConnection]])); internals.oauthRecoveries.delete(staleConnection); resolveRecovery?.(); await expect(toolsPromise).resolves.toEqual({}); - expect(MCPServerInspector.getToolFunctions).toHaveBeenCalledWith( + expect(MCPServerInspector.getToolCatalog).toHaveBeenCalledWith( serverName, recoveredConnection, ); @@ -681,7 +683,7 @@ describe('MCPManager', () => { it('should include specific server name in error messages', async () => { const specificServerName = 'github_mcp_server'; - (MCPServerInspector.getToolFunctions as jest.Mock) = jest.fn(() => { + (MCPServerInspector.getToolCatalog as jest.Mock) = jest.fn(() => { throw new Error('Server specific error'); }); @@ -721,7 +723,7 @@ describe('MCPManager', () => { const appGet = jest.fn().mockResolvedValue({} as MCPConnection); mockAppConnections({ get: appGet }); (mockRegistryInstance.isAppServerConfig as jest.Mock).mockResolvedValue(false); - (MCPServerInspector.getToolFunctions as jest.Mock).mockResolvedValue(expectedTools); + (MCPServerInspector.getToolCatalog as jest.Mock).mockResolvedValue({ tools: expectedTools }); const manager = await MCPManager.createInstance(newMCPServersConfig()); const internals = manager as unknown as { @@ -733,10 +735,7 @@ describe('MCPManager', () => { manager.getServerToolFunctionsSnapshot(userId, serverName, overlayConfig), ).resolves.toEqual({ tools: expectedTools, publicationGeneration: undefined }); expect(appGet).not.toHaveBeenCalled(); - expect(MCPServerInspector.getToolFunctions).toHaveBeenCalledWith( - serverName, - overlayConnection, - ); + expect(MCPServerInspector.getToolCatalog).toHaveBeenCalledWith(serverName, overlayConnection); }); }); @@ -3468,7 +3467,9 @@ describe('MCPManager', () => { }, }, }; - (MCPServerInspector.getToolFunctions as jest.Mock).mockResolvedValue(expectedToolFunctions); + (MCPServerInspector.getToolCatalog as jest.Mock).mockResolvedValue({ + tools: expectedToolFunctions, + }); try { const manager = await MCPManager.createInstance(newMCPServersConfig()); @@ -3528,7 +3529,7 @@ describe('MCPManager', () => { await expect( manager.getServerToolFunctionsSnapshot(userId, serverName, serverConfig), ).resolves.toEqual({ tools: null }); - expect(MCPServerInspector.getToolFunctions).not.toHaveBeenCalled(); + expect(MCPServerInspector.getToolCatalog).not.toHaveBeenCalled(); expect(connection.dispose).toHaveBeenCalledTimes(1); } finally { generationSpy.mockRestore(); @@ -3568,7 +3569,7 @@ describe('MCPManager', () => { await expect( manager.getServerToolFunctionsSnapshot(userId, serverName, committedConfig), ).resolves.toEqual({ tools: null }); - expect(MCPServerInspector.getToolFunctions).not.toHaveBeenCalled(); + expect(MCPServerInspector.getToolCatalog).not.toHaveBeenCalled(); expect(connection.dispose).toHaveBeenCalledTimes(1); } finally { generationSpy.mockRestore(); @@ -3605,7 +3606,7 @@ describe('MCPManager', () => { await expect(manager.getServerToolFunctionsSnapshot(userId, serverName)).resolves.toEqual({ tools: null, }); - expect(MCPServerInspector.getToolFunctions).not.toHaveBeenCalled(); + expect(MCPServerInspector.getToolCatalog).not.toHaveBeenCalled(); expect(connection.dispose).toHaveBeenCalledTimes(1); } finally { generationSpy.mockRestore(); diff --git a/packages/api/src/mcp/__tests__/appToolPublication.integration.test.ts b/packages/api/src/mcp/__tests__/appToolPublication.integration.test.ts index f9fb268a20e..016d745a2a0 100644 --- a/packages/api/src/mcp/__tests__/appToolPublication.integration.test.ts +++ b/packages/api/src/mcp/__tests__/appToolPublication.integration.test.ts @@ -1,39 +1,40 @@ -/** Regression coverage for app-level MCP catalogs that never reach agents (#14857). */ +/** Real-SDK coverage for app-level catalogs that never reach agents (#14857). */ +import { Server } from '@modelcontextprotocol/sdk/server/index.js'; +import { InMemoryTransport } from '@modelcontextprotocol/sdk/inMemory.js'; import { CacheKeys, Constants, normalizeServerName } from 'librechat-data-provider'; -import type { LCAvailableTools, ParsedServerConfig } from '../types'; -import { getMCPAppToolsPublicationGeneration } from '../toolsChanged'; +import { ListToolsRequestSchema, type Tool } from '@modelcontextprotocol/sdk/types.js'; +import type { MCPToolsChangedEvent } from '../toolsChanged'; +import type { ParsedServerConfig } from '../types'; +import { + notifyMCPToolsChanged, + setMCPToolsChangedHandler, + getMCPAppToolsPublicationGeneration, + setMCPToolsChangedRevisionHandler, +} from '../toolsChanged'; import { createMCPCatalogStore } from '../catalog/store'; import { createMCPToolCacheService } from '../tools'; +import { MCPConnection } from '../connection'; + +jest.setTimeout(10_000); const SERVER_NAME = 'shared'; const appConfig: ParsedServerConfig = { type: 'streamable-http', url: 'https://mcp.example.com/mcp', - source: 'yaml', }; +const tool = (name: string): Tool => ({ + name, + description: `${name} the corpus`, + inputSchema: { type: 'object', properties: {} }, +}); + const toolKey = (name: string) => `${name}${Constants.mcp_delimiter}${normalizeServerName(SERVER_NAME)}`; -const description = (name: string) => `${name} the corpus`; - -const catalogOf = (...names: string[]): LCAvailableTools => - Object.fromEntries( - names.map((name) => [ - toolKey(name), - { - type: 'function' as const, - ['function']: { - name: toolKey(name), - description: description(name), - parameters: { type: 'object' as const, properties: {} }, - }, - }, - ]), - ); - -function createService() { +/** Wires the store, the cache service, and the publication handlers exactly as startup does. */ +function createHarness() { const cache = new Map(); const store = createMCPCatalogStore({ cacheConfig: { FORCED_IN_MEMORY_CACHE_NAMESPACES: [CacheKeys.TOOL_CACHE] }, @@ -54,88 +55,143 @@ function createService() { setCachedToolsIfCurrent: store.setCachedToolsIfCurrent, getCachedAppServerTools: store.getCachedAppServerTools, setCachedAppServerTools: store.setCachedAppServerTools, - getNextAppToolsPublicationRevision: store.getNextAppToolsPublicationRevision, getServerConfig: async () => appConfig, getAllServerConfigs: async () => ({ [SERVER_NAME]: appConfig }), isAppServerConfig: async () => true, }); - return { service, store }; + setMCPToolsChangedRevisionHandler(({ serverName, configGeneration }) => + store.getNextAppToolsPublicationRevision(serverName, configGeneration), + ); + setMCPToolsChangedHandler(async (event: MCPToolsChangedEvent) => { + await service.updateMCPServerTools({ + userId: event.userId, + serverName: event.serverName, + tools: event.tools, + serverConfig: event.serverConfig as ParsedServerConfig, + publicationGeneration: event.publicationGeneration, + publicationRevision: event.publicationRevision, + }); + }); + + return { store, service }; +} + +async function createConnection(tools: Tool[]) { + const server = new Server( + { name: 'shared-tool-server', version: '1.0.0' }, + { capabilities: { tools: { listChanged: true } } }, + ); + server.setRequestHandler(ListToolsRequestSchema, async () => ({ + tools: tools.map((entry) => ({ ...entry })), + })); + + const [clientTransport, serverTransport] = InMemoryTransport.createLinkedPair(); + await server.connect(serverTransport); + + /** No userId: this is an app-shared connection, the scope agents read from. */ + const connection = new MCPConnection({ serverName: SERVER_NAME, serverConfig: appConfig }); + await connection.client.connect(clientTransport); + connection.emit('connectionChange', 'connected'); + + return { + connection, + notifyChanged: () => server.sendToolListChanged(), + close: async () => { + connection.removeAllListeners(); + await connection.client.close().catch(() => undefined); + await server.close().catch(() => undefined); + }, + }; } describe('app-level tool publication', () => { const configGeneration = getMCPAppToolsPublicationGeneration(appConfig); + let harness: Awaited> | undefined; - it('publishes a first-connect snapshot that reserved no revision', async () => { - const { service } = createService(); - - await expect( - service.replaceAppServerTools({ - serverName: SERVER_NAME, - serverTools: catalogOf('search'), - publicationGeneration: configGeneration, - }), - ).resolves.toBe(true); - - await expect(service.getMCPServerTools('user-1', SERVER_NAME, appConfig)).resolves.toEqual( - catalogOf('search'), - ); + afterEach(async () => { + setMCPToolsChangedHandler(null); + setMCPToolsChangedRevisionHandler(null); + await harness?.close(); + harness = undefined; }); - /** The reinitialize path an agent falls back to when the shared catalog is cold. Returning - * null here is what surfaced as "configured to use MCP tools, but none are available". */ - it('returns the catalog when a user request republishes a shared server', async () => { - const { service } = createService(); - - await expect( - service.updateMCPServerTools({ - userId: 'user-1', - serverName: SERVER_NAME, - serverConfig: appConfig, - publicationGeneration: configGeneration, - tools: [{ name: 'search', description: description('search') }], - }), - ).resolves.toEqual(catalogOf('search')); - - await expect(service.getMCPServerTools('user-1', SERVER_NAME, appConfig)).resolves.toEqual( - catalogOf('search'), - ); - }); + /** The reporter's case: a shared server connecting outside the startup refresh. Its snapshot + * has to carry ordering, or the catalog write is dropped and agents see no tools at all. */ + it('makes a first-connect snapshot readable by agents', async () => { + const { service } = createHarness(); + harness = await createConnection([tool('search')]); - it('caches a shared catalog discovered on demand', async () => { - const { service } = createService(); + const snapshot = await harness.connection.fetchToolsSnapshot(); + expect(snapshot.publicationRevision).toBeDefined(); - await service.cacheMCPServerTools({ - userId: 'user-1', + await notifyMCPToolsChanged({ + tools: snapshot.tools, serverName: SERVER_NAME, serverConfig: appConfig, - serverTools: catalogOf('search'), publicationGeneration: configGeneration, + publicationRevision: snapshot.publicationRevision, }); - await expect(service.getMCPServerTools('user-2', SERVER_NAME, appConfig)).resolves.toEqual( - catalogOf('search'), - ); + const published = await service.getMCPServerTools('user-1', SERVER_NAME, appConfig); + expect(Object.keys(published ?? {})).toEqual([toolKey('search')]); + }); + + it('reserves ordering before the tools/list it describes', async () => { + const { store } = createHarness(); + harness = await createConnection([tool('search')]); + + const before = await store.getNextAppToolsPublicationRevision(SERVER_NAME, configGeneration); + const snapshot = await harness.connection.fetchToolsSnapshot(); + const after = await store.getNextAppToolsPublicationRevision(SERVER_NAME, configGeneration); + + expect(Number(snapshot.publicationRevision)).toBeGreaterThan(Number(before)); + expect(Number(snapshot.publicationRevision)).toBeLessThan(Number(after)); }); - it('keeps a newer catalog when a publication that reserved earlier lands last', async () => { - const { service, store } = createService(); - const stale = await store.getNextAppToolsPublicationRevision(SERVER_NAME, configGeneration); + /** A snapshot fetched earlier must not overwrite a catalog published from a later fetch, + * which is exactly what a revision allocated at publish time would allow. */ + it('cannot overwrite a catalog fetched after it', async () => { + const { service } = createHarness(); + harness = await createConnection([tool('stale')]); + const stale = await harness.connection.fetchToolsSnapshot(); - await service.replaceAppServerTools({ + const current = await harness.connection.reserveToolsPublicationRevision(); + await notifyMCPToolsChanged({ + tools: [tool('current')], serverName: SERVER_NAME, - serverTools: catalogOf('current'), + serverConfig: appConfig, publicationGeneration: configGeneration, + publicationRevision: current.publicationRevision, }); - await service.replaceAppServerTools({ + await notifyMCPToolsChanged({ + tools: stale.tools, serverName: SERVER_NAME, - serverTools: catalogOf('stale'), + serverConfig: appConfig, publicationGeneration: configGeneration, - publicationRevision: stale, + publicationRevision: stale.publicationRevision, }); - await expect(service.getMCPServerTools('user-1', SERVER_NAME, appConfig)).resolves.toEqual( - catalogOf('current'), + const published = await service.getMCPServerTools('user-1', SERVER_NAME, appConfig); + expect(Object.keys(published ?? {})).toEqual([toolKey('current')]); + }); + + /** When a request-time fetch is superseded by a list_changed refresh, it returns the refresh's + * catalog — and must return the refresh's revision with it, not its own superseded ticket. */ + it('carries the refresh revision when a request fetch is superseded', async () => { + createHarness(); + harness = await createConnection([tool('search')]); + const published: Array = []; + harness.connection.on('toolsChanged', (_tools: Tool[], revision?: string) => + published.push(revision), ); + + await harness.notifyChanged(); + await harness.connection.refreshToolList(); + const ordered = await harness.connection.fetchOrderedToolsSnapshot(); + + expect(published.length).toBeGreaterThan(0); + expect(ordered.complete).toBe(true); + expect(ordered.publicationRevision).toBeDefined(); }); }); diff --git a/packages/api/src/mcp/assistants.ts b/packages/api/src/mcp/assistants.ts index 25c123d0fc1..6b0160ce4ac 100644 --- a/packages/api/src/mcp/assistants.ts +++ b/packages/api/src/mcp/assistants.ts @@ -29,6 +29,7 @@ export interface AssistantToolDefinitionsParams { export interface AssistantToolCatalogSnapshot { tools: LCAvailableTools | null; publicationGeneration?: string; + publicationRevision?: string; } export interface AssistantToolDefinitionsDeps { @@ -60,6 +61,7 @@ export interface AssistantToolDefinitionsDeps { serverTools: LCAvailableTools; serverConfig: ParsedServerConfig; publicationGeneration?: string; + publicationRevision?: string; }) => Promise; } @@ -134,6 +136,7 @@ async function loadServerCatalog( serverTools: snapshot.tools, serverConfig, publicationGeneration: snapshot.publicationGeneration, + publicationRevision: snapshot.publicationRevision, }) .catch((error) => logger.error( diff --git a/packages/api/src/mcp/connection.ts b/packages/api/src/mcp/connection.ts index 64eeed4c57f..9254a4db7aa 100644 --- a/packages/api/src/mcp/connection.ts +++ b/packages/api/src/mcp/connection.ts @@ -1002,6 +1002,11 @@ type MCPListToolsResult = Awaited>; export interface MCPToolsSnapshot { tools: MCPListToolsResult['tools']; complete: boolean; + /** Ordering ticket reserved before this snapshot's `tools/list`; app scope only. */ + publicationRevision?: string; + /** Set when reserving that ticket failed, which is retryable — unlike a scope that simply + * has no ordering to reserve, where the ticket is absent because none was ever needed. */ + orderingUnavailable?: boolean; } export class MCPConnection extends EventEmitter { @@ -1042,6 +1047,7 @@ export class MCPConnection extends EventEmitter { epoch: number; generation: number; tools: MCPListToolsResult['tools']; + publicationRevision?: string; } | null = null; private hasConnected = false; @@ -1853,25 +1859,9 @@ export class MCPConnection extends EventEmitter { const refreshEpoch = this.toolListRefreshEpoch; while (this.handledToolListChangeGeneration < this.toolListChangeGeneration) { const targetGeneration = this.toolListChangeGeneration; - let publicationRevision: string | undefined; - try { - publicationRevision = await reserveMCPToolsChangedRevision({ - serverName: this.serverName, - serverConfig: this.options, - userId: this.userId, - }); - } catch (error) { - this.toolListRefreshFailures++; - logger.error( - `${this.getLogPrefix()} Failed to reserve tool-list publication order:`, - error, - ); - this.scheduleToolListRefreshRetry(); - return; - } - const snapshot = + const snapshot: MCPToolsSnapshot = this.client.getServerCapabilities()?.tools == null - ? { tools: [], complete: true } + ? { tools: [], complete: true, ...(await this.reserveToolsPublicationRevision()) } : await this.fetchToolsSnapshot(); if ( this.toolListRefreshEpoch !== refreshEpoch || @@ -1880,7 +1870,8 @@ export class MCPConnection extends EventEmitter { ) { return; } - if (!snapshot.complete) { + /** Publishing unordered would drop this catalog silently; retry until it can be ordered. */ + if (!snapshot.complete || snapshot.orderingUnavailable) { this.toolListRefreshFailures++; this.scheduleToolListRefreshRetry(); return; @@ -1892,8 +1883,9 @@ export class MCPConnection extends EventEmitter { epoch: refreshEpoch, generation: targetGeneration, tools: snapshot.tools, + publicationRevision: snapshot.publicationRevision, }; - this.dispatchToolsChanged(snapshot.tools, publicationRevision); + this.dispatchToolsChanged(snapshot.tools, snapshot.publicationRevision); } } @@ -2390,11 +2382,20 @@ export class MCPConnection extends EventEmitter { const maxPages = mcpConfig.TOOLS_LIST_MAX_PAGES; const maxTools = mcpConfig.TOOLS_LIST_MAX_TOOLS; const maxBytes = mcpConfig.TOOLS_LIST_MAX_BYTES; + /** Reserved before the first page so the resulting catalog can never outrank one published + * from a `tools/list` that started later. Every app-level publisher reads its ordering off + * the snapshot it received, which is the only way to know when the data was actually read. */ + const ordering = await this.reserveToolsPublicationRevision(); const deadline = Date.now() + mcpConfig.TOOLS_LIST_TIMEOUT_MS; const allTools: MCPListToolsResult['tools'] = []; const seenCursors = new Set(); let cursor: string | undefined; let totalBytes = 0; + const snapshot = (complete: boolean): MCPToolsSnapshot => ({ + tools: allTools, + complete, + ...ordering, + }); for (let page = 1; page <= maxPages; page++) { const exhaustedBudget = getToolsListBudgetExceededReason( @@ -2405,31 +2406,31 @@ export class MCPConnection extends EventEmitter { ); if (exhaustedBudget != null) { this.warnToolsListBudgetExceeded(exhaustedBudget, allTools.length); - return { tools: allTools, complete: true }; + return snapshot(true); } const remainingMs = deadline - Date.now(); if (remainingMs <= 0) { this.warnToolsListBudgetExceeded('time', allTools.length); - return { tools: allTools, complete: false }; + return snapshot(false); } const result = await this.listToolsPage(cursor, remainingMs); if (result == null) { /** Request failed mid-pagination: return the pages already fetched instead of discarding them. */ - return { tools: allTools, complete: false }; + return snapshot(false); } for (const tool of result.tools) { if (allTools.length >= maxTools) { this.warnToolsListBudgetExceeded('tool count', allTools.length); - return { tools: allTools, complete: true }; + return snapshot(true); } const toolBytes = getApproximateToolBytes(tool); if (totalBytes + toolBytes > maxBytes) { this.warnToolsListBudgetExceeded('size', allTools.length); - return { tools: allTools, complete: true }; + return snapshot(true); } allTools.push(tool); @@ -2438,7 +2439,7 @@ export class MCPConnection extends EventEmitter { const { nextCursor } = result; if (nextCursor == null) { - return { tools: allTools, complete: true }; + return snapshot(true); } const nextPageBudget = getToolsListBudgetExceededReason( @@ -2449,14 +2450,14 @@ export class MCPConnection extends EventEmitter { ); if (nextPageBudget != null) { this.warnToolsListBudgetExceeded(nextPageBudget, allTools.length); - return { tools: allTools, complete: true }; + return snapshot(true); } if (seenCursors.has(nextCursor)) { logger.warn( `${this.getLogPrefix()} MCP server returned a repeated tools/list cursor; stopping pagination after ${page} page(s).`, ); - return { tools: allTools, complete: false }; + return snapshot(false); } seenCursors.add(nextCursor); @@ -2466,7 +2467,29 @@ export class MCPConnection extends EventEmitter { logger.warn( `${this.getLogPrefix()} Reached the tools/list pagination limit of ${maxPages} page(s); some tools may be omitted. Set MCP_TOOLS_LIST_MAX_PAGES higher if this server legitimately exposes more.`, ); - return { tools: allTools, complete: true }; + return snapshot(true); + } + + /** + * Allocates app-catalog ordering. A reservation failure must not fail the request that asked + * for the tools, so it is reported on the snapshot for publishers to retry on instead. + */ + public async reserveToolsPublicationRevision(): Promise<{ + publicationRevision?: string; + orderingUnavailable?: boolean; + }> { + try { + return { + publicationRevision: await reserveMCPToolsChangedRevision({ + serverName: this.serverName, + serverConfig: this.options, + userId: this.userId, + }), + }; + } catch (error) { + logger.warn(`${this.getLogPrefix()} Failed to reserve tool-list publication order:`, error); + return { orderingUnavailable: true }; + } } /** @@ -2507,7 +2530,13 @@ export class MCPConnection extends EventEmitter { published.generation === this.toolListChangeGeneration && this.handledToolListChangeGeneration === this.toolListChangeGeneration ) { - return { tools: published.tools, complete: true }; + /** Ordering travels with the data: this is the refresh's catalog, so it must publish under + * the refresh's revision rather than the one reserved for the superseded fetch above. */ + return { + tools: published.tools, + complete: true, + publicationRevision: published.publicationRevision, + }; } return { tools: [], complete: false }; diff --git a/packages/api/src/mcp/registry/MCPServerInspector.ts b/packages/api/src/mcp/registry/MCPServerInspector.ts index 3281cdf6bea..b0c1b436e0b 100644 --- a/packages/api/src/mcp/registry/MCPServerInspector.ts +++ b/packages/api/src/mcp/registry/MCPServerInspector.ts @@ -162,22 +162,22 @@ export class MCPServerInspector { } private async fetchToolFunctions(): Promise { - this.config.toolFunctions = await MCPServerInspector.getToolFunctions( - this.serverName, - this.connection!, - ); + this.config.toolFunctions = ( + await MCPServerInspector.getToolCatalog(this.serverName, this.connection!) + ).tools; } /** - * Converts server tools to LibreChat-compatible tool functions format. + * Converts server tools to LibreChat-compatible tool functions format, keeping the ordering + * reserved before the `tools/list` that produced them. App-level publishers need that + * revision — a catalog write that cannot be ordered against concurrent replicas is dropped. * @param serverName - The name of the server * @param connection - The MCP connection - * @returns Tool functions formatted for LibreChat */ - public static async getToolFunctions( + public static async getToolCatalog( serverName: string, connection: MCPConnection, - ): Promise { + ): Promise<{ tools: t.LCAvailableTools; publicationRevision?: string }> { const snapshot = await connection.fetchOrderedToolsSnapshot(); if (!snapshot.complete) { throw new Error(`Incomplete tools/list snapshot for MCP server ${serverName}`); @@ -206,6 +206,6 @@ export class MCPServerInspector { }; }); - return toolFunctions; + return { tools: toolFunctions, publicationRevision: snapshot.publicationRevision }; } } diff --git a/packages/api/src/mcp/registry/__tests__/MCPServerInspector.test.ts b/packages/api/src/mcp/registry/__tests__/MCPServerInspector.test.ts index b3f6aaf1f42..52990606991 100644 --- a/packages/api/src/mcp/registry/__tests__/MCPServerInspector.test.ts +++ b/packages/api/src/mcp/registry/__tests__/MCPServerInspector.test.ts @@ -501,7 +501,7 @@ describe('MCPServerInspector', () => { }); }); - describe('getToolFunctions()', () => { + describe('getToolCatalog()', () => { it('should convert MCP tools to LibreChat tool functions format', async () => { mockConnection.fetchOrderedToolsSnapshot = jest.fn().mockResolvedValue({ complete: true, @@ -528,7 +528,10 @@ describe('MCPServerInspector', () => { ], }); - const result = await MCPServerInspector.getToolFunctions('my_server', mockConnection); + const { tools: result } = await MCPServerInspector.getToolCatalog( + 'my_server', + mockConnection, + ); expect(result).toEqual({ file_read_mcp_my_server: { @@ -564,7 +567,10 @@ describe('MCPServerInspector', () => { .fn() .mockResolvedValue({ tools: [], complete: true }); - const result = await MCPServerInspector.getToolFunctions('my_server', mockConnection); + const { tools: result } = await MCPServerInspector.getToolCatalog( + 'my_server', + mockConnection, + ); expect(result).toEqual({}); }); @@ -581,7 +587,10 @@ describe('MCPServerInspector', () => { ], }); - const result = await MCPServerInspector.getToolFunctions('My Server', mockConnection); + const { tools: result } = await MCPServerInspector.getToolCatalog( + 'My Server', + mockConnection, + ); const key = 'file_read_mcp_My_Server'; expect(Object.keys(result)).toEqual([key]); @@ -594,9 +603,9 @@ describe('MCPServerInspector', () => { complete: false, }); - await expect( - MCPServerInspector.getToolFunctions('my_server', mockConnection), - ).rejects.toThrow('Incomplete tools/list snapshot for MCP server my_server'); + await expect(MCPServerInspector.getToolCatalog('my_server', mockConnection)).rejects.toThrow( + 'Incomplete tools/list snapshot for MCP server my_server', + ); }); }); }); diff --git a/packages/api/src/mcp/tools.spec.ts b/packages/api/src/mcp/tools.spec.ts index 9179e4607a1..74d8a5c68a4 100644 --- a/packages/api/src/mcp/tools.spec.ts +++ b/packages/api/src/mcp/tools.spec.ts @@ -157,52 +157,9 @@ describe('createMCPToolCacheService', () => { ).rejects.toThrow('Redis down'); }); - it('orders a publication that could not reserve ordering before its fetch', async () => { - const setCachedAppServerTools = jest.fn().mockResolvedValue(true); - const deps = createMockDeps({ - setCachedAppServerTools, - getNextAppToolsPublicationRevision: jest.fn().mockResolvedValue('7'), - }); - - await expect( - createMCPToolCacheService(deps).replaceAppServerTools({ - serverName: 'dynamic', - serverTools: {}, - publicationGeneration: 'config-generation', - }), - ).resolves.toBe(true); - - expect(deps.getNextAppToolsPublicationRevision).toHaveBeenCalledWith( - 'dynamic', - 'config-generation', - ); - expect(setCachedAppServerTools).toHaveBeenCalledWith('dynamic', 'config-generation', {}, '7'); - }); - - it('keeps a reserved revision instead of allocating a later one', async () => { - const deps = createMockDeps({ - getNextAppToolsPublicationRevision: jest.fn().mockResolvedValue('7'), - }); - - await expect( - createMCPToolCacheService(deps).replaceAppServerTools({ - serverName: 'dynamic', - serverTools: {}, - publicationGeneration: 'config-generation', - publicationRevision: '3', - }), - ).resolves.toBe(true); - - expect(deps.getNextAppToolsPublicationRevision).not.toHaveBeenCalled(); - expect(deps.setCachedAppServerTools).toHaveBeenCalledWith( - 'dynamic', - 'config-generation', - {}, - '3', - ); - }); - - it('does not publish an unordered snapshot when no revision allocator is wired', async () => { + /** A publisher that lost its snapshot's revision fetched at an unknown time. Allocating a + * fresh one here would let a slow fetch of an old catalog outrank a newer one. */ + it('does not publish a live app snapshot without pre-fetch ordering', async () => { const deps = createMockDeps(); await expect( diff --git a/packages/api/src/mcp/tools.ts b/packages/api/src/mcp/tools.ts index e90fd98ea7d..16a9080d7aa 100644 --- a/packages/api/src/mcp/tools.ts +++ b/packages/api/src/mcp/tools.ts @@ -41,10 +41,6 @@ export interface MCPToolCacheDeps { tools: LCAvailableTools, publicationRevision?: string, ) => Promise; - getNextAppToolsPublicationRevision?: ( - serverName: string, - configGeneration: string, - ) => Promise; getServerConfig: (serverName: string, userId?: string) => Promise; getAllServerConfigs?: () => Promise>; isAppServerConfig?: (serverName: string, effectiveConfig: ParsedServerConfig) => Promise; @@ -95,7 +91,6 @@ export function createMCPToolCacheService(deps: MCPToolCacheDeps): MCPToolCacheS setCachedToolsIfCurrent, getCachedAppServerTools, setCachedAppServerTools, - getNextAppToolsPublicationRevision, getServerConfig, getAllServerConfigs, isAppServerConfig, @@ -377,16 +372,13 @@ export function createMCPToolCacheService(deps: MCPToolCacheDeps): MCPToolCacheS logger.debug(`[MCP Cache] Skipped unaddressed app-level publication for ${serverName}`); return false; } - /** Only a list_changed refresh can reserve ordering before its `tools/list` starts. Every - * other publisher — first connect, reinitialization, on-demand catalog reads — has already - * fetched by the time it gets here, so it takes the next revision now rather than being - * dropped; dropping left agents with a permanently empty app catalog (#14857). */ - const revision = - publicationRevision ?? - (await getNextAppToolsPublicationRevision?.(serverName, configGeneration)); - if (!revision) { + /** Ordering is reserved before the `tools/list` that produced these tools and travels with + * the snapshot, so a publisher that lost it fetched at an unknown time and cannot be + * ordered against concurrent replicas. Allocating one here instead would let a slow fetch + * of an old catalog outrank a newer one that reserved after it started. */ + if (!publicationRevision) { logger.debug( - `[MCP Cache] Skipped unordered app-level publication for ${serverName}: no revision allocator is configured`, + `[MCP Cache] Skipped unordered app-level publication for ${serverName}: its snapshot carried no reserved revision`, ); return false; } @@ -394,11 +386,11 @@ export function createMCPToolCacheService(deps: MCPToolCacheDeps): MCPToolCacheS serverName, configGeneration, serverTools, - revision, + publicationRevision, ); if (replaced === false) { logger.debug( - `[MCP Cache] Ignored superseded app-level tools for ${serverName} at revision ${revision}`, + `[MCP Cache] Ignored superseded app-level tools for ${serverName} at revision ${publicationRevision}`, ); return false; } From cd013053af8b2ffb64b1bc3d4519e4aa7285cfce Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Sat, 15 Aug 2026 08:48:04 -0400 Subject: [PATCH 3/6] =?UTF-8?q?=F0=9F=93=A1=20fix:=20Retry=20an=20Empty=20?= =?UTF-8?q?App=20Catalog=20That=20Could=20Not=20Reserve=20Ordering?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-up. The no-tools-capability branch destructured the reservation result and dropped `orderingUnavailable`, publishing without a revision when the revision store was transiently unavailable. That write is rejected in silence, and unlike the snapshot branch this one returned without reaching `refreshToolList()`, so whatever the server last advertised stayed in place until the connection was recreated or the cache expired. Both branches now route an unreservable catalog through the same retry path. --- packages/api/src/mcp/ConnectionsRepository.ts | 10 ++++-- .../__tests__/ConnectionsRepository.test.ts | 31 +++++++++++++++++++ 2 files changed, 39 insertions(+), 2 deletions(-) diff --git a/packages/api/src/mcp/ConnectionsRepository.ts b/packages/api/src/mcp/ConnectionsRepository.ts index 9e1a5ba06b8..84d67c9f60b 100644 --- a/packages/api/src/mcp/ConnectionsRepository.ts +++ b/packages/api/src/mcp/ConnectionsRepository.ts @@ -167,13 +167,19 @@ export class ConnectionsRepository { * list_changed refresh is. An app-level write that cannot be ordered is dropped, which * left agents with a permanently empty catalog when this path populated it (#14857). */ if (connection.client.getServerCapabilities()?.tools == null) { - const { publicationRevision } = await connection.reserveToolsPublicationRevision(); + const ordering = await connection.reserveToolsPublicationRevision(); + /** The refresh path reserves again under backoff. Publishing unordered instead would be + * dropped in silence, leaving whatever this server last advertised in place. */ + if (ordering.orderingUnavailable) { + await connection.refreshToolList(); + return connection; + } await notifyMCPToolsChanged({ tools: [], serverName, serverConfig, publicationGeneration, - publicationRevision, + publicationRevision: ordering.publicationRevision, }); return connection; } diff --git a/packages/api/src/mcp/__tests__/ConnectionsRepository.test.ts b/packages/api/src/mcp/__tests__/ConnectionsRepository.test.ts index 9237b2d093f..98ebd7f41ba 100644 --- a/packages/api/src/mcp/__tests__/ConnectionsRepository.test.ts +++ b/packages/api/src/mcp/__tests__/ConnectionsRepository.test.ts @@ -225,6 +225,37 @@ describe('ConnectionsRepository', () => { ); }); + /** An unordered publication is dropped in silence, so a server left with no way to order + * its empty catalog has to retry rather than leave the previous one advertised. */ + it('retries an empty catalog it could not reserve ordering for', async () => { + (mockConnection.client.getServerCapabilities as jest.Mock).mockReturnValue({}); + mockConnection.reserveToolsPublicationRevision = jest + .fn() + .mockResolvedValue({ orderingUnavailable: true }); + const handler = jest.fn(); + setMCPToolsChangedHandler(handler); + + await repository.get('server1'); + + expect(mockConnection.refreshToolList).toHaveBeenCalledTimes(1); + expect(handler).not.toHaveBeenCalled(); + }); + + it('retries a fetched catalog it could not reserve ordering for', async () => { + mockConnection.fetchToolsSnapshot.mockResolvedValue({ + tools: [], + complete: true, + orderingUnavailable: true, + }); + const handler = jest.fn(); + setMCPToolsChangedHandler(handler); + + await repository.get('server1'); + + expect(mockConnection.refreshToolList).toHaveBeenCalledTimes(1); + expect(handler).not.toHaveBeenCalled(); + }); + it('can defer the initial app tool refresh for startup synchronization', async () => { await repository.get('server1', { refreshTools: false }); From c7eedfad047491488dded2c51e876fcf98d13bf7 Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Sat, 15 Aug 2026 09:00:28 -0400 Subject: [PATCH 4/6] =?UTF-8?q?=F0=9F=93=A1=20fix:=20Serve=20Tools=20Whose?= =?UTF-8?q?=20Shared=20Catalog=20Write=20Could=20Not=20Be=20Ordered?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-up. Only the shared catalog write needs ordering; the tools themselves were just read from the server and are correct to serve. Discarding them because the write could not be ordered is what turns a cache failure into a server that appears to have no tools at all, which is the reported symptom. `updateMCPServerTools` now returns the tools it built when the publication has no reserved revision, instead of null. A superseded write still discards — there another replica holds something newer. Reinitialization also asks the connection to republish under backoff when its snapshot could not reserve ordering, so the shared catalog does not stay cold until something else triggers a refresh. --- api/server/services/Tools/mcp.js | 11 ++++++++++ packages/api/src/mcp/tools.spec.ts | 33 ++++++++++++++++++++++++++++++ packages/api/src/mcp/tools.ts | 11 ++++++++++ 3 files changed, 55 insertions(+) diff --git a/api/server/services/Tools/mcp.js b/api/server/services/Tools/mcp.js index 2a6dd5e810f..78325d77208 100644 --- a/api/server/services/Tools/mcp.js +++ b/api/server/services/Tools/mcp.js @@ -283,6 +283,17 @@ async function reinitMCPServer({ /** Reserved before this snapshot's tools/list; an app-level catalog cannot publish * without it, and allocating a later one here would outrank fresher tools. */ publicationRevision = snapshot.publicationRevision; + if (snapshot.orderingUnavailable && typeof connection.refreshToolList === 'function') { + /** These tools still serve this request; the connection republishes the shared + * catalog under backoff rather than leaving it cold until the next reinitialize. */ + connection + .refreshToolList() + .catch((err) => + logger.debug( + `[MCP Reinitialize] Could not schedule a catalog republish for ${serverName}: ${err?.message ?? String(err)}`, + ), + ); + } } else { logger.warn( `[MCP Reinitialize] Preserving cached tools for ${serverName} because tools/list returned an incomplete snapshot`, diff --git a/packages/api/src/mcp/tools.spec.ts b/packages/api/src/mcp/tools.spec.ts index 74d8a5c68a4..7c9e7fbd001 100644 --- a/packages/api/src/mcp/tools.spec.ts +++ b/packages/api/src/mcp/tools.spec.ts @@ -173,6 +173,39 @@ describe('createMCPToolCacheService', () => { expect(deps.setCachedAppServerTools).not.toHaveBeenCalled(); }); + /** The catalog write needs ordering; the tools themselves were read from the server and are + * correct to serve. Discarding them is what surfaced as a server with no tools (#14857). */ + it('serves tools it could not publish instead of discarding them', async () => { + const deps = createSharedCacheDeps({ config: cacheableConfig }); + const search = toolName('search', 'dynamic'); + + await expect( + createMCPToolCacheService(deps).updateMCPServerTools({ + userId: 'user-1', + serverName: 'dynamic', + serverConfig: cacheableConfig, + tools: [{ name: 'search' }], + }), + ).resolves.toEqual({ [search]: expect.objectContaining({ type: 'function' }) }); + + expect(deps.setCachedAppServerTools).not.toHaveBeenCalled(); + }); + + it('discards a superseded catalog rather than serving it', async () => { + const deps = createSharedCacheDeps({ config: cacheableConfig }); + deps.setCachedAppServerTools = jest.fn().mockResolvedValue(false); + + await expect( + createMCPToolCacheService(deps).updateMCPServerTools({ + userId: 'user-1', + serverName: 'dynamic', + serverConfig: cacheableConfig, + tools: [{ name: 'search' }], + publicationRevision: '1', + }), + ).resolves.toBeNull(); + }); + it('rejects a tool boundary owned by another app server', async () => { const shadowed = toolName('search', 'foo_bar'); const deps = createMockDeps({ diff --git a/packages/api/src/mcp/tools.ts b/packages/api/src/mcp/tools.ts index 16a9080d7aa..467cf6d6980 100644 --- a/packages/api/src/mcp/tools.ts +++ b/packages/api/src/mcp/tools.ts @@ -288,6 +288,17 @@ export function createMCPToolCacheService(deps: MCPToolCacheDeps): MCPToolCacheS userId == null ? (publicationGeneration ?? configGeneration) : (configGeneration ?? publicationGeneration); + /** Only the shared catalog write needs ordering. These tools were just read from the + * server, so the caller should still serve them; discarding a correct tool list because + * its write could not be ordered is what makes a cache failure look to the user like a + * server with no tools at all (#14857). A superseded write is different — another + * replica holds something newer — and still discards below. */ + if (!publicationRevision) { + logger.debug( + `[MCP Cache] Serving ${tools.length} unpublished tools for ${serverName}: this snapshot reserved no revision`, + ); + return serverTools; + } const replaced = await replaceAppServerTools({ serverName, serverTools, From c8a1787433492b7c6d0fadcf6bdbf9895f33c015 Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Sat, 15 Aug 2026 09:08:22 -0400 Subject: [PATCH 5/6] =?UTF-8?q?=F0=9F=93=A1=20fix:=20Surface=20a=20Discard?= =?UTF-8?q?ed=20App=20Catalog=20Instead=20of=20Debug-Logging=20It?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #14857 went a release without a diagnostic because the only trace of a dropped app-level catalog was a debug line no deployment runs. Operators saw agents fail every turn with nothing in the logs to explain it, and the reporter had to read the source to find the cause. A publication discarded because it cannot be addressed or ordered means this server's tools are unavailable to every agent that selected them, and serving an unpublished catalog means every request re-fetches it. Both are warnings now. A superseded write stays at debug: concurrent replicas produce it routinely and the winner already holds newer tools. Tests pin the level, so a later refactor cannot quietly make the failure silent again. --- packages/api/src/mcp/tools.spec.ts | 37 ++++++++++++++++++++++++++++++ packages/api/src/mcp/tools.ts | 16 +++++++++---- 2 files changed, 48 insertions(+), 5 deletions(-) diff --git a/packages/api/src/mcp/tools.spec.ts b/packages/api/src/mcp/tools.spec.ts index 7c9e7fbd001..03726be831a 100644 --- a/packages/api/src/mcp/tools.spec.ts +++ b/packages/api/src/mcp/tools.spec.ts @@ -1,3 +1,4 @@ +import { logger } from '@librechat/data-schemas'; import { Constants, normalizeServerName } from 'librechat-data-provider'; import type { LCAvailableTools, ParsedServerConfig } from './types'; import type { MCPToolCacheDeps, MCPToolInput } from './tools'; @@ -173,6 +174,42 @@ describe('createMCPToolCacheService', () => { expect(deps.setCachedAppServerTools).not.toHaveBeenCalled(); }); + /** #14857 went a release without a diagnostic because dropping an app catalog only logged + * at debug. A drop means agents lose this server's tools, so it has to be visible by + * default; a superseded write is routine and must stay quiet. */ + describe('visibility of a discarded publication', () => { + const publish = (params: { publicationGeneration?: string; publicationRevision?: string }) => + createMCPToolCacheService( + createMockDeps({ setCachedAppServerTools: jest.fn().mockResolvedValue(false) }), + ).replaceAppServerTools({ serverName: 'dynamic', serverTools: {}, ...params }); + + let warn: jest.SpyInstance; + + beforeEach(() => { + warn = jest.spyOn(logger, 'warn').mockImplementation(() => logger); + }); + + afterEach(() => warn.mockRestore()); + + it('warns when a publication cannot be ordered', async () => { + await publish({ publicationGeneration: 'config-generation' }); + + expect(warn).toHaveBeenCalledWith(expect.stringContaining('Skipped unordered')); + }); + + it('warns when a publication cannot be addressed', async () => { + await publish({ publicationRevision: '1' }); + + expect(warn).toHaveBeenCalledWith(expect.stringContaining('Skipped unaddressed')); + }); + + it('stays quiet when a concurrent replica already published newer tools', async () => { + await publish({ publicationGeneration: 'config-generation', publicationRevision: '1' }); + + expect(warn).not.toHaveBeenCalled(); + }); + }); + /** The catalog write needs ordering; the tools themselves were read from the server and are * correct to serve. Discarding them is what surfaced as a server with no tools (#14857). */ it('serves tools it could not publish instead of discarding them', async () => { diff --git a/packages/api/src/mcp/tools.ts b/packages/api/src/mcp/tools.ts index 467cf6d6980..b1b801e1ca8 100644 --- a/packages/api/src/mcp/tools.ts +++ b/packages/api/src/mcp/tools.ts @@ -294,8 +294,8 @@ export function createMCPToolCacheService(deps: MCPToolCacheDeps): MCPToolCacheS * server with no tools at all (#14857). A superseded write is different — another * replica holds something newer — and still discards below. */ if (!publicationRevision) { - logger.debug( - `[MCP Cache] Serving ${tools.length} unpublished tools for ${serverName}: this snapshot reserved no revision`, + logger.warn( + `[MCP Cache] Serving ${tools.length} unpublished tools for ${serverName}: this snapshot reserved no revision, so every request re-fetches them`, ); return serverTools; } @@ -379,8 +379,13 @@ export function createMCPToolCacheService(deps: MCPToolCacheDeps): MCPToolCacheS const config = await resolveCacheConfig(undefined, serverName); configGeneration = config ? getMCPAppToolsPublicationGeneration(config) : undefined; } + /** Discarding a publication is warned, not debugged: #14857 was invisible for a release + * because the only trace of a dropped app catalog was a debug line no deployment runs. + * A drop here means this server's tools are missing for every agent that needs them. */ if (!configGeneration) { - logger.debug(`[MCP Cache] Skipped unaddressed app-level publication for ${serverName}`); + logger.warn( + `[MCP Cache] Skipped unaddressed app-level publication for ${serverName}; its tools stay unavailable to agents`, + ); return false; } /** Ordering is reserved before the `tools/list` that produced these tools and travels with @@ -388,8 +393,8 @@ export function createMCPToolCacheService(deps: MCPToolCacheDeps): MCPToolCacheS * ordered against concurrent replicas. Allocating one here instead would let a slow fetch * of an old catalog outrank a newer one that reserved after it started. */ if (!publicationRevision) { - logger.debug( - `[MCP Cache] Skipped unordered app-level publication for ${serverName}: its snapshot carried no reserved revision`, + logger.warn( + `[MCP Cache] Skipped unordered app-level publication for ${serverName}: its snapshot carried no reserved revision, so its tools stay unavailable to agents`, ); return false; } @@ -399,6 +404,7 @@ export function createMCPToolCacheService(deps: MCPToolCacheDeps): MCPToolCacheS serverTools, publicationRevision, ); + /** Expected whenever replicas publish concurrently: the winner already holds newer tools. */ if (replaced === false) { logger.debug( `[MCP Cache] Ignored superseded app-level tools for ${serverName} at revision ${publicationRevision}`, From e495fcf6335280cca95be31d4e03dddef8b18211 Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Sat, 15 Aug 2026 09:17:38 -0400 Subject: [PATCH 6/6] =?UTF-8?q?=F0=9F=A7=AA=20test:=20Pin=20the=20Reinitia?= =?UTF-8?q?lize=20Path's=20Catalog=20Ordering?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reinitialization is the path an agent falls back to when the shared catalog is cold, so it is where #14857 surfaced as "configured to use MCP tools, but none are available". Nothing pinned that it forwards the ordering its snapshot was fetched with, nor that it asks the connection to republish a catalog it could not order. Both assertions fail against the pre-fix source. --- api/server/services/Tools/mcp.spec.js | 44 +++++++++++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/api/server/services/Tools/mcp.spec.js b/api/server/services/Tools/mcp.spec.js index 65029bd68b3..f4e6b2629fb 100644 --- a/api/server/services/Tools/mcp.spec.js +++ b/api/server/services/Tools/mcp.spec.js @@ -125,6 +125,50 @@ describe('reinitMCPServer — customUserVars gating (issue #10969)', () => { }); }); + /** An app-level catalog write is dropped unless it carries the ordering reserved before its + * own tools/list. When this path forwarded no revision, every publication was discarded and + * agents were told the server had no tools at all (#14857). */ + it('publishes under the ordering its snapshot was fetched with', async () => { + mockGetConnection.mockResolvedValue({ + fetchOrderedToolsSnapshot: jest.fn().mockResolvedValue({ + tools: [{ name: 'search', inputSchema: { type: 'object' } }], + complete: true, + publicationRevision: '7', + }), + }); + + await reinitMCPServer({ + user, + serverName, + serverConfig: { type: 'streamable-http', url: 'https://thingy.example.com/mcp' }, + }); + + expect(mockUpdateMCPServerTools).toHaveBeenCalledWith( + expect.objectContaining({ serverName, publicationRevision: '7' }), + ); + }); + + it('asks the connection to republish a catalog it could not order', async () => { + const refreshToolList = jest.fn().mockResolvedValue(undefined); + mockGetConnection.mockResolvedValue({ + refreshToolList, + fetchOrderedToolsSnapshot: jest.fn().mockResolvedValue({ + tools: [{ name: 'search', inputSchema: { type: 'object' } }], + complete: true, + orderingUnavailable: true, + }), + }); + + const result = await reinitMCPServer({ + user, + serverName, + serverConfig: { type: 'streamable-http', url: 'https://thingy.example.com/mcp' }, + }); + + expect(refreshToolList).toHaveBeenCalledTimes(1); + expect(result.tools).toHaveLength(1); + }); + it('preserves cached tools when live recovery returns an incomplete snapshot', async () => { const fetchOrderedToolsSnapshot = jest.fn().mockResolvedValue({ tools: [{ name: 'partial', inputSchema: { type: 'object' } }],