Files
sim/apps/sim/lib/mcp/connection-manager.test.ts
Waleed 8b4b3af120 fix(mcp): harden notification system against race conditions (#3168)
* fix(mcp): harden notification system against race conditions

- Guard concurrent connect() calls in connection manager with connectingServers Set
- Suppress post-disconnect notification handler firing in MCP client
- Clean up Redis event listeners in pub/sub dispose()
- Add tests for all three hardening fixes (11 new tests)

* updated tests

* plugged in new mcp event based system and create sse route to publish notifs

* ack commetns

* fix reconnect timer

* cleanup when running onClose

* fixed spacing on mcp settings tab

* keep error listeners before quiet in redis
2026-02-09 19:36:01 -08:00

181 lines
5.4 KiB
TypeScript

/**
* @vitest-environment node
*/
import { loggerMock } from '@sim/testing'
import { afterEach, describe, expect, it, vi } from 'vitest'
interface MockMcpClient {
connect: ReturnType<typeof vi.fn>
disconnect: ReturnType<typeof vi.fn>
hasListChangedCapability: ReturnType<typeof vi.fn>
onClose: ReturnType<typeof vi.fn>
}
/** Deferred promise to control when `client.connect()` resolves. */
function createDeferred<T = void>() {
let resolve!: (value: T) => void
const promise = new Promise<T>((res) => {
resolve = res
})
return { promise, resolve }
}
function serverConfig(id: string, name = `Server ${id}`) {
return {
id,
name,
transport: 'streamable-http' as const,
url: `https://${id}.example.com/mcp`,
}
}
/** Shared setup: resets modules and applies base mocks. */
function setupBaseMocks() {
vi.resetModules()
vi.doMock('@sim/logger', () => loggerMock)
vi.doMock('@/lib/core/config/feature-flags', () => ({ isTest: false }))
vi.doMock('@/lib/mcp/pubsub', () => ({
mcpPubSub: { onToolsChanged: vi.fn(() => vi.fn()), publishToolsChanged: vi.fn() },
}))
}
describe('McpConnectionManager', () => {
let manager: {
connect: (...args: unknown[]) => Promise<{ supportsListChanged: boolean }>
dispose: () => void
} | null = null
afterEach(() => {
manager?.dispose()
manager = null
})
describe('concurrent connect() guard', () => {
it('creates only one client when two connect() calls race for the same serverId', async () => {
setupBaseMocks()
const deferred = createDeferred()
const instances: MockMcpClient[] = []
vi.doMock('./client', () => ({
McpClient: vi.fn().mockImplementation(() => {
const instance: MockMcpClient = {
connect: vi.fn().mockImplementation(() => deferred.promise),
disconnect: vi.fn().mockResolvedValue(undefined),
hasListChangedCapability: vi.fn().mockReturnValue(true),
onClose: vi.fn(),
}
instances.push(instance)
return instance
}),
}))
const { mcpConnectionManager: mgr } = await import('./connection-manager')
manager = mgr
const config = serverConfig('server-1')
const p1 = mgr.connect(config, 'user-1', 'ws-1')
const p2 = mgr.connect(config, 'user-1', 'ws-1')
deferred.resolve()
const [r1, r2] = await Promise.all([p1, p2])
expect(instances).toHaveLength(1)
expect(r1.supportsListChanged).toBe(true)
expect(r2.supportsListChanged).toBe(false)
})
it('allows a new connect() after a previous one completes', async () => {
setupBaseMocks()
const instances: MockMcpClient[] = []
vi.doMock('./client', () => ({
McpClient: vi.fn().mockImplementation(() => {
const instance: MockMcpClient = {
connect: vi.fn().mockResolvedValue(undefined),
disconnect: vi.fn().mockResolvedValue(undefined),
hasListChangedCapability: vi.fn().mockReturnValue(false),
onClose: vi.fn(),
}
instances.push(instance)
return instance
}),
}))
const { mcpConnectionManager: mgr } = await import('./connection-manager')
manager = mgr
const config = serverConfig('server-2')
const r1 = await mgr.connect(config, 'user-1', 'ws-1')
expect(r1.supportsListChanged).toBe(false)
const r2 = await mgr.connect(config, 'user-1', 'ws-1')
expect(r2.supportsListChanged).toBe(false)
expect(instances).toHaveLength(2)
})
it('cleans up connectingServers when connect() throws', async () => {
setupBaseMocks()
let callCount = 0
const instances: MockMcpClient[] = []
vi.doMock('./client', () => ({
McpClient: vi.fn().mockImplementation(() => {
callCount++
const instance: MockMcpClient = {
connect:
callCount === 1
? vi.fn().mockRejectedValue(new Error('Connection refused'))
: vi.fn().mockResolvedValue(undefined),
disconnect: vi.fn().mockResolvedValue(undefined),
hasListChangedCapability: vi.fn().mockReturnValue(true),
onClose: vi.fn(),
}
instances.push(instance)
return instance
}),
}))
const { mcpConnectionManager: mgr } = await import('./connection-manager')
manager = mgr
const config = serverConfig('server-3')
const r1 = await mgr.connect(config, 'user-1', 'ws-1')
expect(r1.supportsListChanged).toBe(false)
const r2 = await mgr.connect(config, 'user-1', 'ws-1')
expect(r2.supportsListChanged).toBe(true)
expect(instances).toHaveLength(2)
})
})
describe('dispose', () => {
it('rejects new connections after dispose', async () => {
setupBaseMocks()
vi.doMock('./client', () => ({
McpClient: vi.fn().mockImplementation(() => ({
connect: vi.fn().mockResolvedValue(undefined),
disconnect: vi.fn().mockResolvedValue(undefined),
hasListChangedCapability: vi.fn().mockReturnValue(true),
onClose: vi.fn(),
})),
}))
const { mcpConnectionManager: mgr } = await import('./connection-manager')
manager = mgr
mgr.dispose()
const result = await mgr.connect(serverConfig('server-4'), 'user-1', 'ws-1')
expect(result.supportsListChanged).toBe(false)
})
})
})