diff --git a/docs/design/f2-mcp-transport-pool.md b/docs/design/f2-mcp-transport-pool.md index 6b54771c799..4da25cf8a0f 100644 --- a/docs/design/f2-mcp-transport-pool.md +++ b/docs/design/f2-mcp-transport-pool.md @@ -75,14 +75,14 @@ PR #4336 shipped F2 as 6 atomic commits + 6 fix commits over ~4 hours. Wenshao r #### Declined-with-reply (filed as F2 follow-ups) -| # | Site | Reason for declining | -| --- | --------------------------------------------------- | --------------------------------------------------------------------------------------------------------------------------------------- | -| W7 | Test coverage gaps (4 untested critical paths) | 1/4 added (W6 regression test); rest deferred to focused test-coverage PR after F2 series merges | -| W8 | `maxReconnectAttempts` / `reconnectStrategy` unused | Forward-compat placeholders for the deferred health-monitor-driven reconnect (design §6.6); removing + re-adding churns the public type | -| W11 | Duplicate fast-path / in-flight-path attach blocks | Refactor opportunity touching the entire `acquire` signature; not blocking F2 merge | -| W12 | `passesSessionFilter` O(M×N) per `applyTools` | Micro-perf optimization measurable only with hundreds of tools × large filter lists | -| R9 | `McpClientManager` ctor 7-positional sentinels | Refactor to options-object touches every call site (test fixtures, ToolRegistry); out of F2 scope | -| R10 | `pgrep -P ` per-PID-per-level cost | Single `ps -eo pid,ppid` requires platform-specific column flags + tree builder; current 16s worst-case bound is acceptable | +| # | Site | Reason for declining | +| --- | --------------------------------------------------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| W7 | Test coverage gaps (4 untested critical paths) | 1/4 added (W6 regression test); rest deferred to focused test-coverage PR after F2 series merges | +| W8 | `maxReconnectAttempts` / `reconnectStrategy` unused | Forward-compat placeholders for the deferred health-monitor-driven reconnect (design §6.6); removing + re-adding churns the public type | +| W11 | Duplicate fast-path / in-flight-path attach blocks | ✅ Done in PR A: `attachPooledSession` + `rollbackReservationOnSpawnFailure` private helpers (commit `2d546efca`) | +| W12 | `passesSessionFilter` O(M×N) per `applyTools` | ✅ Done in PR A: `applyTools` / `applyPrompts` precompute filter `Set`s once per pass; predicate becomes O(1) per tool (commit `a4a855ab3`) | +| R9 | `McpClientManager` ctor 7-positional sentinels | ✅ Done in PR A: options-object ctor + `mkManager` test factory (commit `0cb1eaa27`) | +| R10 | `pgrep -P ` per-PID-per-level cost | ✅ Done in PR A: single `ps -A -o pid=,ppid=` snapshot + in-memory BFS walk; pgrep BFS retained as fallback for BusyBox ` / `Get-CimInstance -Filter` subprocess per node) to a single process-table snapshot followed by in-memory tree walk. Two motivations: (1) one fork instead of B^D forks on the hot pool-shutdown path; (2) snapshot consistency — pre-fix BFS could miss descendants that forked between adjacent BFS levels. Per-pid path retained as fallback for BusyBox `ps` { - if (process.platform === 'win32') return listDescendantPidsWin(rootPid); - return listDescendantPidsUnix(rootPid); + if (!Number.isInteger(rootPid) || rootPid <= 0) return []; + try { + if (process.platform === 'win32') + return await listDescendantPidsWin(rootPid); + return await listDescendantPidsUnix(rootPid); + } catch { + return []; // OS reaps orphans; pool shutdown still proceeds. + } } async function listDescendantPidsUnix(root: number): Promise { - const all: number[] = []; - const queue = [root]; - while (queue.length) { - const parent = queue.shift()!; - const { stdout } = await execFile('pgrep', ['-P', String(parent)], { - timeout: 2000, - }).catch(() => ({ stdout: '' })); - const children = stdout.split('\n').map(Number).filter(Number.isFinite); - all.push(...children); - queue.push(...children); + let tree: Map | undefined; + try { + tree = await snapshotProcessTreeUnix(); // ps -A -o pid=,ppid= + } catch { + /* fall through to fallback */ } - return all; + if (tree) return walkDescendants(tree, root); // O(descendants), 1 fork + return await listDescendantPidsUnixPgrepFallback(root); // legacy BFS } -async function listDescendantPidsWin(root: number): Promise { - // PowerShell CIM query — wmic deprecated on modern Windows - const script = `Get-CimInstance Win32_Process | Where-Object { $_.ParentProcessId -eq } | Select-Object -ExpandProperty ProcessId`; - // ... recursive walk, return aggregated pids +async function snapshotProcessTreeUnix(): Promise> { + // -A: all processes (POSIX, equivalent to -e but unambiguous on BSD). + // -o pid=,ppid=: pid + ppid columns, trailing `=` suppresses headers. + const { stdout } = await execFile('ps', ['-A', '-o', 'pid=,ppid='], { + timeout: 2000, + maxBuffer: 8 * 1024 * 1024, // covers >250k-process pathological hosts + }); + const childrenByPpid = new Map(); + for (const line of stdout.split('\n')) { + const m = line.trim().match(/^(\d+)\s+(\d+)$/); + if (!m) continue; + /* parse, push into childrenByPpid */ + } + return childrenByPpid; } + +// Windows: single Get-CimInstance Win32_Process | ConvertTo-Csv snapshot +// of all (ProcessId, ParentProcessId) rows + in-memory walk; per-pid +// `Get-CimInstance -Filter "ParentProcessId=$p"` retained as fallback. ``` -Called from `PoolEntry.shutdown()` before `client.disconnect()`. Handles `npx @modelcontextprotocol/server-X`, `uvx ...`, `pnpm dlx ...` wrapper leaks. +Called from `PoolEntry.shutdown()` before `client.disconnect()`. Handles `npx @modelcontextprotocol/server-X`, `uvx ...`, `pnpm dlx ...` wrapper leaks. MAX_DESCENDANTS=256 / MAX_DEPTH=8 caps preserved. ### 6.5 Spawn failure handling diff --git a/packages/core/src/tools/mcp-client-manager.test.ts b/packages/core/src/tools/mcp-client-manager.test.ts index 9220bb3b853..c86ac31abc0 100644 --- a/packages/core/src/tools/mcp-client-manager.test.ts +++ b/packages/core/src/tools/mcp-client-manager.test.ts @@ -5,7 +5,10 @@ */ import { afterEach, describe, expect, it, vi } from 'vitest'; -import { McpClientManager } from './mcp-client-manager.js'; +import { + McpClientManager, + type McpClientManagerOptions, +} from './mcp-client-manager.js'; import { McpClient } from './mcp-client.js'; import type { ToolRegistry } from './tool-registry.js'; import type { Config } from '../config/config.js'; @@ -22,6 +25,37 @@ vi.mock('./mcp-client.js', async () => { }; }); +/** + * F2 (#4175 commit 6 review fix — wenshao R9 / PR A): test factory + * for `McpClientManager`. Pre-fix the 80 construction sites in this + * file each repeated a 7-positional call with 4 `undefined` sentinels + * to reach the trailing `pool` arg. With the options-object ctor + + * this factory, each site names only the fields it overrides; default + * `mockConfig` + `{} as ToolRegistry` cover the no-arg case. + */ +function mkManager( + overrides: { + config?: Config; + toolRegistry?: ToolRegistry; + options?: McpClientManagerOptions; + } = {}, +): McpClientManager { + const config = + overrides.config ?? + ({ + isTrustedFolder: () => true, + getMcpServers: () => ({}), + getMcpServerCommand: () => undefined, + getPromptRegistry: () => ({}), + getWorkspaceContext: () => ({}), + getDebugMode: () => false, + getSessionId: () => 'sid-1', + isMcpServerDisabled: () => false, + } as unknown as Config); + const toolRegistry = overrides.toolRegistry ?? ({} as ToolRegistry); + return new McpClientManager(config, toolRegistry, overrides.options ?? {}); +} + describe('McpClientManager', () => { afterEach(() => { vi.restoreAllMocks(); @@ -59,15 +93,10 @@ describe('McpClientManager', () => { getSessionId: () => 'sid-1', isMcpServerDisabled: () => false, } as unknown as Config; - const manager = new McpClientManager( - mockConfig, - {} as ToolRegistry, - undefined, - undefined, - undefined, - undefined, - fakePool, - ); + const manager = mkManager({ + config: mockConfig, + options: { pool: fakePool }, + }); await manager.discoverAllMcpTools(mockConfig); expect(acquireSpy).toHaveBeenCalledTimes(1); expect(acquireSpy).toHaveBeenCalledWith( @@ -125,15 +154,10 @@ describe('McpClientManager', () => { getSessionId: () => 'sid-1', isMcpServerDisabled: () => false, } as unknown as Config; - const manager = new McpClientManager( - mockConfig, - {} as ToolRegistry, - undefined, - undefined, - undefined, - undefined, - fakePool, - ); + const manager = mkManager({ + config: mockConfig, + options: { pool: fakePool }, + }); // Should resolve without throwing — the BudgetExhaustedError on // srvB is caught and downgraded to a debug log. await manager.discoverAllMcpTools(mockConfig); @@ -184,15 +208,10 @@ describe('McpClientManager', () => { getSessionId: () => 'sid-1', isMcpServerDisabled: () => false, } as unknown as Config; - const manager = new McpClientManager( - mockConfig, - {} as ToolRegistry, - undefined, - undefined, - undefined, - undefined, - fakePool, - ); + const manager = mkManager({ + config: mockConfig, + options: { pool: fakePool }, + }); // Kick off discovery; it enters in-flight (acquire awaits the gate). const discoveryPromise = manager.discoverAllMcpTools(mockConfig); @@ -247,15 +266,10 @@ describe('McpClientManager', () => { getSessionId: () => 'sid-1', isMcpServerDisabled: () => false, } as unknown as Config; - const manager = new McpClientManager( - mockConfig, - {} as ToolRegistry, - undefined, - undefined, - undefined, - undefined, - fakePool, - ); + const manager = mkManager({ + config: mockConfig, + options: { pool: fakePool }, + }); // Inject a rejecting in-flight promise (the only way to hit the // W108 catch block — internal per-server catches mean the natural // path always resolves). @@ -297,15 +311,10 @@ describe('McpClientManager', () => { getSessionId: () => 'sid-1', isMcpServerDisabled: () => false, } as unknown as Config; - const manager = new McpClientManager( - mockConfig, - {} as ToolRegistry, - undefined, - undefined, - undefined, - undefined, - fakePool, - ); + const manager = mkManager({ + config: mockConfig, + options: { pool: fakePool }, + }); // Inject a never-settling in-flight promise. ( manager as unknown as { discoveryInFlight?: Promise } @@ -354,15 +363,10 @@ describe('McpClientManager', () => { getSessionId: () => 'sid-1', isMcpServerDisabled: () => false, } as unknown as Config; - const manager = new McpClientManager( - mockConfig, - {} as ToolRegistry, - undefined, - undefined, - undefined, - undefined, - fakePool, - ); + const manager = mkManager({ + config: mockConfig, + options: { pool: fakePool }, + }); // Simulate a prior timed-out shutdown that set the sticky flag. (manager as unknown as { stopTimedOut: boolean }).stopTimedOut = true; @@ -417,15 +421,10 @@ describe('McpClientManager', () => { getSessionId: () => 'sid-1', isMcpServerDisabled: () => false, } as unknown as Config; - const manager = new McpClientManager( - mockConfig, - {} as ToolRegistry, - undefined, - undefined, - undefined, - undefined, - fakePool, - ); + const manager = mkManager({ + config: mockConfig, + options: { pool: fakePool }, + }); await manager.discoverAllMcpToolsIncremental(mockConfig); expect(acquireSpy).toHaveBeenCalledTimes(1); expect(McpClient).not.toHaveBeenCalled(); @@ -454,15 +453,10 @@ describe('McpClientManager', () => { getSessionId: () => 'sid-1', isMcpServerDisabled: () => false, } as unknown as Config; - const manager = new McpClientManager( - mockConfig, - {} as ToolRegistry, - undefined, - undefined, - undefined, - undefined, - fakePool, - ); + const manager = mkManager({ + config: mockConfig, + options: { pool: fakePool }, + }); await manager.discoverMcpToolsForServer('srv', mockConfig); @@ -500,15 +494,10 @@ describe('McpClientManager', () => { getSessionId: () => 'sid-1', isMcpServerDisabled: () => false, } as unknown as Config; - const manager = new McpClientManager( - mockConfig, - {} as ToolRegistry, - undefined, - undefined, - undefined, - undefined, - fakePool, - ); + const manager = mkManager({ + config: mockConfig, + options: { pool: fakePool }, + }); await manager.discoverAllMcpTools(mockConfig); const result = await manager.readResource('srv', 'mcp://srv/doc'); @@ -565,15 +554,10 @@ describe('McpClientManager', () => { getSessionId: () => 'sid-1', isMcpServerDisabled: () => false, } as unknown as Config; - const manager = new McpClientManager( - mockConfig, - {} as ToolRegistry, - undefined, - undefined, - undefined, - undefined, - fakePool, - ); + const manager = mkManager({ + config: mockConfig, + options: { pool: fakePool }, + }); await manager.discoverAllMcpTools(mockConfig); // Sanity: healthy fast-path still works. await expect(manager.readResource('srv', 'mcp://srv/doc')).resolves.toEqual( @@ -636,15 +620,13 @@ describe('McpClientManager', () => { getSessionId: () => 'sid-1', isMcpServerDisabled: () => false, } as unknown as Config; - const manager = new McpClientManager( - mockConfig, - { removeMcpToolsByServer: vi.fn() } as unknown as ToolRegistry, - undefined, - undefined, - undefined, - undefined, - fakePool, - ); + const manager = mkManager({ + config: mockConfig, + toolRegistry: { + removeMcpToolsByServer: vi.fn(), + } as unknown as ToolRegistry, + options: { pool: fakePool }, + }); await manager.discoverAllMcpTools(mockConfig); expect(releaseSpy).not.toHaveBeenCalled(); await manager.disconnectServer('srv'); @@ -687,15 +669,10 @@ describe('McpClientManager', () => { getSessionId: () => 'sid-1', isMcpServerDisabled: () => false, } as unknown as Config; - const manager = new McpClientManager( - mockConfig, - {} as ToolRegistry, - undefined, - undefined, - undefined, - undefined, - fakePool, - ); + const manager = mkManager({ + config: mockConfig, + options: { pool: fakePool }, + }); const p1 = manager.discoverAllMcpTools(mockConfig); const p2 = manager.discoverAllMcpTools(mockConfig); // Both passes block on the in-flight `pool.acquire`. Pre-fix @@ -732,7 +709,7 @@ describe('McpClientManager', () => { getDebugMode: () => false, isMcpServerDisabled: () => false, } as unknown as Config; - const manager = new McpClientManager(mockConfig, {} as ToolRegistry); + const manager = mkManager({ config: mockConfig }); await manager.discoverAllMcpTools(mockConfig); expect(McpClient).toHaveBeenCalledOnce(); expect(mockedMcpClient.connect).toHaveBeenCalledOnce(); @@ -757,7 +734,7 @@ describe('McpClientManager', () => { getDebugMode: () => false, isMcpServerDisabled: () => false, } as unknown as Config; - const manager = new McpClientManager(mockConfig, {} as ToolRegistry); + const manager = mkManager({ config: mockConfig }); await manager.discoverAllMcpTools(mockConfig); expect(mockedMcpClient.connect).toHaveBeenCalledOnce(); expect(mockedMcpClient.discover).toHaveBeenCalledOnce(); @@ -782,7 +759,7 @@ describe('McpClientManager', () => { getDebugMode: () => false, isMcpServerDisabled: () => false, } as unknown as Config; - const manager = new McpClientManager(mockConfig, {} as ToolRegistry); + const manager = mkManager({ config: mockConfig }); await manager.discoverAllMcpTools(mockConfig); expect(mockedMcpClient.connect).not.toHaveBeenCalled(); expect(mockedMcpClient.discover).not.toHaveBeenCalled(); @@ -812,7 +789,7 @@ describe('McpClientManager', () => { getDebugMode: () => false, isMcpServerDisabled: () => false, } as unknown as Config; - const manager = new McpClientManager(mockConfig, {} as ToolRegistry); + const manager = mkManager({ config: mockConfig }); // First connect to create the clients await manager.discoverAllMcpTools({ isTrustedFolder: () => true, @@ -848,7 +825,7 @@ describe('McpClientManager', () => { getDebugMode: () => false, isMcpServerDisabled: () => false, } as unknown as Config; - const manager = new McpClientManager(mockConfig, {} as ToolRegistry); + const manager = mkManager({ config: mockConfig }); await manager.discoverAllMcpTools({ isTrustedFolder: () => true, isMcpServerDisabled: () => false, @@ -879,7 +856,7 @@ describe('McpClientManager', () => { getWorkspaceContext: () => ({}) as WorkspaceContext, getDebugMode: () => false, } as unknown as Config; - const manager = new McpClientManager(mockConfig, {} as ToolRegistry); + const manager = mkManager({ config: mockConfig }); await manager.discoverMcpToolsForServer( 'test-server', @@ -919,7 +896,7 @@ describe('McpClientManager', () => { getWorkspaceContext: () => ({}) as WorkspaceContext, getDebugMode: () => false, } as unknown as Config; - const manager = new McpClientManager(mockConfig, {} as ToolRegistry); + const manager = mkManager({ config: mockConfig }); await manager.discoverMcpToolsForServer( 'test-server', @@ -979,7 +956,7 @@ describe('McpClientManager', () => { getWorkspaceContext: () => ({}) as WorkspaceContext, getDebugMode: () => false, } as unknown as Config; - const manager = new McpClientManager(mockConfig, {} as ToolRegistry); + const manager = mkManager({ config: mockConfig }); await manager.discoverMcpToolsForServer( 'test-server', @@ -1048,18 +1025,17 @@ describe('McpClientManager', () => { getWorkspaceContext: () => ({}) as WorkspaceContext, getDebugMode: () => false, } as unknown as Config; - const manager = new McpClientManager( - mockConfig, - {} as ToolRegistry, - undefined, - undefined, - { - autoReconnect: true, - checkIntervalMs: 10, - maxConsecutiveFailures: 1, - reconnectDelayMs: 10, + const manager = mkManager({ + config: mockConfig, + options: { + healthConfig: { + autoReconnect: true, + checkIntervalMs: 10, + maxConsecutiveFailures: 1, + reconnectDelayMs: 10, + }, }, - ); + }); try { await manager.discoverMcpToolsForServer( @@ -1116,7 +1092,7 @@ describe('McpClientManager', () => { getWorkspaceContext: () => ({}) as WorkspaceContext, getDebugMode: () => false, } as unknown as Config; - const manager = new McpClientManager(mockConfig, {} as ToolRegistry); + const manager = mkManager({ config: mockConfig }); const discovery = manager.discoverMcpToolsForServer( 'test-server', @@ -1165,7 +1141,7 @@ describe('McpClientManager', () => { getWorkspaceContext: () => ({}) as WorkspaceContext, getDebugMode: () => false, } as unknown as Config; - const manager = new McpClientManager(mockConfig, {} as ToolRegistry); + const manager = mkManager({ config: mockConfig }); await manager.discoverMcpToolsForServer('unknown-server', { isTrustedFolder: () => true, @@ -1203,9 +1179,12 @@ describe('McpClientManager', () => { getDebugMode: () => false, isMcpServerDisabled: () => false, } as unknown as Config; - const manager = new McpClientManager(mockConfig, { - removeMcpToolsByServer: vi.fn(), - } as unknown as ToolRegistry); + const manager = mkManager({ + config: mockConfig, + toolRegistry: { + removeMcpToolsByServer: vi.fn(), + } as unknown as ToolRegistry, + }); const t0 = Date.now(); await manager.discoverAllMcpToolsIncremental(mockConfig); @@ -1254,7 +1233,7 @@ describe('McpClientManager', () => { getDebugMode: () => false, isMcpServerDisabled: (name: string) => name === 'disabled', } as unknown as Config; - const manager = new McpClientManager(mockConfig, {} as ToolRegistry); + const manager = mkManager({ config: mockConfig }); await manager.discoverAllMcpToolsIncremental(mockConfig); @@ -1297,7 +1276,10 @@ describe('McpClientManager', () => { getDebugMode: () => false, isMcpServerDisabled: (name: string) => name === 'foo' && disabled, } as unknown as Config; - const manager = new McpClientManager(mockConfig, toolRegistryStub); + const manager = mkManager({ + config: mockConfig, + toolRegistry: toolRegistryStub, + }); // First pass: server enabled, gets connected. await manager.discoverAllMcpToolsIncremental(mockConfig); @@ -1352,7 +1334,7 @@ describe('McpClientManager', () => { getDebugMode: () => false, isMcpServerDisabled: () => false, } as unknown as Config; - const manager = new McpClientManager(mockConfig, {} as ToolRegistry); + const manager = mkManager({ config: mockConfig }); await manager.discoverAllMcpToolsIncremental(mockConfig); // Cleanup the global sink so it doesn't leak into other tests. @@ -1409,9 +1391,12 @@ describe('McpClientManager', () => { getDebugMode: () => false, isMcpServerDisabled: () => false, } as unknown as Config; - const manager = new McpClientManager(mockConfig, { - removeMcpToolsByServer: vi.fn(), - } as unknown as ToolRegistry); + const manager = mkManager({ + config: mockConfig, + toolRegistry: { + removeMcpToolsByServer: vi.fn(), + } as unknown as ToolRegistry, + }); await manager.discoverAllMcpToolsIncremental(mockConfig); spy.mockRestore(); @@ -1461,9 +1446,12 @@ describe('McpClientManager', () => { getDebugMode: () => false, isMcpServerDisabled: () => false, } as unknown as Config; - const manager = new McpClientManager(mockConfig, { - removeMcpToolsByServer: vi.fn(), - } as unknown as ToolRegistry); + const manager = mkManager({ + config: mockConfig, + toolRegistry: { + removeMcpToolsByServer: vi.fn(), + } as unknown as ToolRegistry, + }); await manager.discoverAllMcpToolsIncremental(mockConfig); spy.mockRestore(); @@ -1510,9 +1498,10 @@ describe('McpClientManager', () => { isMcpServerDisabled: () => false, } as unknown as Config; const removeMcpToolsByServer = vi.fn(); - const manager = new McpClientManager(mockConfig, { - removeMcpToolsByServer, - } as unknown as ToolRegistry); + const manager = mkManager({ + config: mockConfig, + toolRegistry: { removeMcpToolsByServer } as unknown as ToolRegistry, + }); await manager.discoverAllMcpToolsIncremental(mockConfig); @@ -1562,9 +1551,12 @@ describe('McpClientManager', () => { getDebugMode: () => false, isMcpServerDisabled: () => false, } as unknown as Config; - const manager = new McpClientManager(mockConfig, { - removeMcpToolsByServer: vi.fn(), - } as unknown as ToolRegistry); + const manager = mkManager({ + config: mockConfig, + toolRegistry: { + removeMcpToolsByServer: vi.fn(), + } as unknown as ToolRegistry, + }); await manager.discoverAllMcpToolsIncremental(mockConfig); @@ -1625,11 +1617,10 @@ describe('McpClientManager', () => { getDebugMode: () => false, isMcpServerDisabled: () => false, } as unknown as Config; - const manager = new McpClientManager( - mockConfig, - {} as ToolRegistry, - events, - ); + const manager = mkManager({ + config: mockConfig, + options: { eventEmitter: events }, + }); await manager.discoverAllMcpToolsIncremental(mockConfig); @@ -1742,14 +1733,10 @@ describe('McpClientManager — PR 14 guardrails', () => { c: { command: 'node' }, d: { command: 'node' }, }); - const manager = new McpClientManager( + const manager = mkManager({ config, - {} as ToolRegistry, - undefined, - undefined, - undefined, - { clientBudget: 2, budgetMode: 'enforce' }, - ); + options: { budgetConfig: { clientBudget: 2, budgetMode: 'enforce' } }, + }); await manager.discoverAllMcpTools(config); expect(created).toHaveLength(2); // only 2 McpClient instances created const accounting = manager.getMcpClientAccounting(); @@ -1772,14 +1759,10 @@ describe('McpClientManager — PR 14 guardrails', () => { b: { command: 'node' }, c: { command: 'node' }, }); - const manager = new McpClientManager( + const manager = mkManager({ config, - {} as ToolRegistry, - undefined, - undefined, - undefined, - { clientBudget: 2, budgetMode: 'warn' }, - ); + options: { budgetConfig: { clientBudget: 2, budgetMode: 'warn' } }, + }); await manager.discoverAllMcpTools(config); // warn mode: all 3 connect; reservedSlots grows past budget; no refusals. expect(created).toHaveLength(3); @@ -1797,14 +1780,10 @@ describe('McpClientManager — PR 14 guardrails', () => { a: { command: 'node' }, b: { command: 'node' }, }); - const manager = new McpClientManager( + const manager = mkManager({ config, - {} as ToolRegistry, - undefined, - undefined, - undefined, - { budgetMode: 'off' }, - ); + options: { budgetConfig: { budgetMode: 'off' } }, + }); await manager.discoverAllMcpTools(config); const accounting = manager.getMcpClientAccounting(); expect(accounting.total).toBe(2); @@ -1826,14 +1805,10 @@ describe('McpClientManager — PR 14 guardrails', () => { alpha: { command: 'node' }, mike: { command: 'node' }, }); - const manager = new McpClientManager( + const manager = mkManager({ config, - {} as ToolRegistry, - undefined, - undefined, - undefined, - { clientBudget: 2, budgetMode: 'enforce' }, - ); + options: { budgetConfig: { clientBudget: 2, budgetMode: 'enforce' } }, + }); await manager.discoverAllMcpTools(config); expect(created).toEqual(['zulu', 'alpha']); expect(manager.getMcpClientAccounting().refusedServerNames).toEqual([ @@ -1849,14 +1824,10 @@ describe('McpClientManager — PR 14 guardrails', () => { a: { command: 'node' }, b: { command: 'node' }, }); - const manager = new McpClientManager( + const manager = mkManager({ config, - {} as ToolRegistry, - undefined, - undefined, - undefined, - { clientBudget: 1, budgetMode: 'enforce' }, - ); + options: { budgetConfig: { clientBudget: 1, budgetMode: 'enforce' } }, + }); await manager.discoverAllMcpTools(config); expect(manager.getMcpClientAccounting().refusedServerNames).toEqual(['b']); @@ -1877,14 +1848,10 @@ describe('McpClientManager — PR 14 guardrails', () => { a: { command: 'node' }, b: { command: 'node' }, }); - const manager = new McpClientManager( + const manager = mkManager({ config, - {} as ToolRegistry, - undefined, - undefined, - undefined, - { clientBudget: 1, budgetMode: 'enforce' }, - ); + options: { budgetConfig: { clientBudget: 1, budgetMode: 'enforce' } }, + }); await manager.discoverAllMcpTools(config); // `a` was reserved; `b` was refused. A `readResource('b', ...)` would // lazy-spawn — must throw rather than silently exceed the cap. @@ -1901,14 +1868,10 @@ describe('McpClientManager — PR 14 guardrails', () => { a: { command: 'node' }, b: { command: 'node' }, }); - const manager = new McpClientManager( + const manager = mkManager({ config, - {} as ToolRegistry, - undefined, - undefined, - undefined, - { clientBudget: 1, budgetMode: 'enforce' }, - ); + options: { budgetConfig: { clientBudget: 1, budgetMode: 'enforce' } }, + }); await manager.discoverAllMcpTools(config); expect(manager.getMcpClientAccounting().reservedSlots).toEqual(['a']); await manager.disconnectServer('a'); @@ -1920,7 +1883,7 @@ describe('McpClientManager — PR 14 guardrails', () => { process.env['QWEN_SERVE_MCP_CLIENT_BUDGET'] = '7'; process.env['QWEN_SERVE_MCP_BUDGET_MODE'] = 'enforce'; const config = configWithServers({}); - const manager = new McpClientManager(config, {} as ToolRegistry); + const manager = mkManager({ config }); expect(manager.getMcpClientBudget()).toBe(7); expect(manager.getMcpBudgetMode()).toBe('enforce'); }); @@ -1929,7 +1892,7 @@ describe('McpClientManager — PR 14 guardrails', () => { process.env['QWEN_SERVE_MCP_CLIENT_BUDGET'] = '5'; // No mode env var. Resolved mode is `warn` (the safe default). const config = configWithServers({}); - const manager = new McpClientManager(config, {} as ToolRegistry); + const manager = mkManager({ config }); expect(manager.getMcpClientBudget()).toBe(5); expect(manager.getMcpBudgetMode()).toBe('warn'); }); @@ -1937,7 +1900,7 @@ describe('McpClientManager — PR 14 guardrails', () => { it('env var fallback rejects non-positive budgets silently', async () => { process.env['QWEN_SERVE_MCP_CLIENT_BUDGET'] = '-3'; const config = configWithServers({}); - const manager = new McpClientManager(config, {} as ToolRegistry); + const manager = mkManager({ config }); // Invalid values fall through to `undefined` budget + `off` mode — // no enforcement, no boot-time crash. Validation lives in the CLI // flag handler (`packages/cli/src/commands/serve.ts`). @@ -1965,14 +1928,10 @@ describe('McpClientManager — PR 14 guardrails', () => { name === 'b') as Config['isMcpServerDisabled'], }, ); - const manager = new McpClientManager( + const manager = mkManager({ config, - {} as ToolRegistry, - undefined, - undefined, - undefined, - { clientBudget: 2, budgetMode: 'enforce' }, - ); + options: { budgetConfig: { clientBudget: 2, budgetMode: 'enforce' } }, + }); await manager.discoverAllMcpTools(config); expect(created.sort()).toEqual(['a', 'c']); expect(manager.getMcpClientAccounting().reservedSlots.sort()).toEqual([ @@ -1994,14 +1953,10 @@ describe('McpClientManager — PR 14 guardrails', () => { a: { command: 'node' }, b: { command: 'node' }, }); - const manager = new McpClientManager( + const manager = mkManager({ config, - {} as ToolRegistry, - undefined, - undefined, - undefined, - { clientBudget: 1, budgetMode: 'enforce' }, - ); + options: { budgetConfig: { clientBudget: 1, budgetMode: 'enforce' } }, + }); await manager.discoverAllMcpTools(config); // `b` was refused at startup. A manual `/mcp reconnect b` (which goes // through `discoverMcpToolsForServer` → `...Internal`) would have @@ -2023,14 +1978,10 @@ describe('McpClientManager — PR 14 guardrails', () => { a: { command: 'node' }, b: { command: 'node' }, }); - const manager = new McpClientManager( + const manager = mkManager({ config, - {} as ToolRegistry, - undefined, - undefined, - undefined, - { clientBudget: 1, budgetMode: 'enforce' }, - ); + options: { budgetConfig: { clientBudget: 1, budgetMode: 'enforce' } }, + }); await manager.discoverAllMcpTools(config); expect(manager.getMcpClientAccounting().refusedServerNames).toEqual(['b']); // Operator action: explicit disconnect of `b` should drop it from @@ -2059,16 +2010,13 @@ describe('McpClientManager — PR 14 guardrails', () => { getDebugMode: () => false, isMcpServerDisabled: () => false, } as unknown as Config; - const manager = new McpClientManager( + const manager = mkManager({ config, - { + toolRegistry: { removeMcpToolsByServer: () => undefined, } as unknown as ToolRegistry, - undefined, - undefined, - undefined, - { clientBudget: 2, budgetMode: 'enforce' }, - ); + options: { budgetConfig: { clientBudget: 2, budgetMode: 'enforce' } }, + }); await manager.discoverAllMcpToolsIncremental(config); expect(manager.getMcpClientAccounting().reservedSlots.sort()).toEqual([ 'a', @@ -2103,14 +2051,10 @@ describe('McpClientManager — PR 14 guardrails', () => { a: { command: 'node' }, b: { command: 'node' }, }); - const manager = new McpClientManager( + const manager = mkManager({ config, - {} as ToolRegistry, - undefined, - undefined, - undefined, - { budgetMode: 'off' }, - ); + options: { budgetConfig: { budgetMode: 'off' } }, + }); await manager.discoverAllMcpTools(config); const accounting = manager.getMcpClientAccounting(); expect(accounting.total).toBe(2); @@ -2140,14 +2084,10 @@ describe('McpClientManager — PR 14 guardrails', () => { a: { command: 'node' }, // will fail b: { command: 'node' }, // would be refused pre-fix }); - const manager = new McpClientManager( + const manager = mkManager({ config, - {} as ToolRegistry, - undefined, - undefined, - undefined, - { clientBudget: 1, budgetMode: 'enforce' }, - ); + options: { budgetConfig: { clientBudget: 1, budgetMode: 'enforce' } }, + }); await manager.discoverAllMcpTools(config); // `a` failed → slot freed → `b` ought to fit (budget=1, current=0 // after `a` released). But discoverAllMcpTools walks all servers @@ -2186,14 +2126,10 @@ describe('McpClientManager — PR 14 guardrails', () => { const config = configWithServers({ a: { command: 'node' }, }); - const manager = new McpClientManager( + const manager = mkManager({ config, - {} as ToolRegistry, - undefined, - undefined, - undefined, - { clientBudget: 1, budgetMode: 'enforce' }, - ); + options: { budgetConfig: { clientBudget: 1, budgetMode: 'enforce' } }, + }); // No discovery yet → `a` not in clients → lazy spawn path. await expect(manager.readResource('a', 'file:///x')).rejects.toThrow( 'lazy connect boom', @@ -2216,7 +2152,7 @@ describe('McpClientManager — PR 14 guardrails', () => { // `tryReserveSlot` returns 'reserved' when `clientBudget === undefined`, // so an "enforce" daemon would let unlimited servers through. const config = configWithServers({}); - const manager = new McpClientManager(config, {} as ToolRegistry); + const manager = mkManager({ config }); expect(manager.getMcpClientBudget()).toBeUndefined(); // Downgraded — not 'enforce' — because enforce requires a budget. expect(manager.getMcpBudgetMode()).toBe('off'); @@ -2238,7 +2174,7 @@ describe('McpClientManager — PR 14 guardrails', () => { name === 'a') as Config['isMcpServerDisabled'], }, ); - const manager = new McpClientManager(config, {} as ToolRegistry); + const manager = mkManager({ config }); await expect(manager.readResource('a', 'file:///x')).rejects.toThrow( /'a' is disabled/, ); @@ -2261,14 +2197,10 @@ describe('McpClientManager — PR 14 guardrails', () => { name === 'b') as Config['isMcpServerDisabled'], }, ); - const manager = new McpClientManager( + const manager = mkManager({ config, - {} as ToolRegistry, - undefined, - undefined, - undefined, - { clientBudget: 1, budgetMode: 'enforce' }, - ); + options: { budgetConfig: { clientBudget: 1, budgetMode: 'enforce' } }, + }); await manager.discoverAllMcpTools(config); // Even though `b` would be budget-refused if not disabled, the // disabled gate must trip first. @@ -2300,14 +2232,10 @@ describe('McpClientManager — PR 14 guardrails', () => { }) as unknown as McpClient, ); const config = configWithServers({ x: { command: 'node' } }); - const manager = new McpClientManager( + const manager = mkManager({ config, - {} as ToolRegistry, - undefined, - undefined, - undefined, - { clientBudget: 1, budgetMode: 'enforce' }, - ); + options: { budgetConfig: { clientBudget: 1, budgetMode: 'enforce' } }, + }); // Server `x` not previously reserved; this call freshly reserves // then connect() throws. Pre-fix the slot leaked permanently // under enforce mode, blocking any later server in `clients.size=1`. @@ -2345,19 +2273,21 @@ describe('McpClientManager — PR 14 guardrails', () => { }) as unknown as McpClient, ); const config = configWithServers({ a: { command: 'node' } }); - const manager = new McpClientManager( + const manager = mkManager({ config, - { removeMcpToolsByServer: () => undefined } as unknown as ToolRegistry, - undefined, - undefined, - { - autoReconnect: false, - checkIntervalMs: 100, - maxConsecutiveFailures: 1, - reconnectDelayMs: 100, + toolRegistry: { + removeMcpToolsByServer: () => undefined, + } as unknown as ToolRegistry, + options: { + healthConfig: { + autoReconnect: false, + checkIntervalMs: 100, + maxConsecutiveFailures: 1, + reconnectDelayMs: 100, + }, + budgetConfig: { clientBudget: 2, budgetMode: 'enforce' }, }, - { clientBudget: 2, budgetMode: 'enforce' }, - ); + }); const discoveryPromise = manager.discoverAllMcpToolsIncremental(config); // Advance past the stdio default discovery timeout (30s). await vi.advanceTimersByTimeAsync(31_000); @@ -2383,14 +2313,13 @@ describe('McpClientManager — PR 14 guardrails', () => { second: { command: 'node' }, third: { command: 'node' }, }); - const manager = new McpClientManager( + const manager = mkManager({ config, - { removeMcpToolsByServer: () => undefined } as unknown as ToolRegistry, - undefined, - undefined, - undefined, - { clientBudget: 2, budgetMode: 'enforce' }, - ); + toolRegistry: { + removeMcpToolsByServer: () => undefined, + } as unknown as ToolRegistry, + options: { budgetConfig: { clientBudget: 2, budgetMode: 'enforce' } }, + }); await manager.discoverAllMcpToolsIncremental(config); // First two declared servers fit; third refused. Refusal-order // determinism preserved (config-declaration order) — the inner @@ -2418,14 +2347,10 @@ describe('McpClientManager — PR 14 guardrails', () => { a: { command: 'node' }, b: { command: 'node' }, }); - const manager = new McpClientManager( + const manager = mkManager({ config, - {} as ToolRegistry, - undefined, - undefined, - undefined, - { clientBudget: 1, budgetMode: 'enforce' }, - ); + options: { budgetConfig: { clientBudget: 1, budgetMode: 'enforce' } }, + }); await manager.discoverAllMcpTools(config); expect(manager.getMcpClientAccounting().refusedServerNames).toEqual(['b']); // Free a slot. @@ -2450,14 +2375,10 @@ describe('McpClientManager — PR 14 guardrails', () => { a: { command: 'node' }, b: { command: 'node' }, }); - const manager = new McpClientManager( + const manager = mkManager({ config, - {} as ToolRegistry, - undefined, - undefined, - undefined, - { clientBudget: 1, budgetMode: 'enforce' }, - ); + options: { budgetConfig: { clientBudget: 1, budgetMode: 'enforce' } }, + }); await manager.discoverAllMcpTools(config); expect(manager.getMcpClientAccounting().refusedServerNames).toEqual(['b']); // Free a slot. @@ -2485,7 +2406,7 @@ describe('McpClientManager — PR 14 guardrails', () => { name === 'a') as Config['isMcpServerDisabled'], }, ); - const manager = new McpClientManager(config, {} as ToolRegistry); + const manager = mkManager({ config }); await manager.discoverMcpToolsForServer('a', config); expect(createdCount).toBe(0); }); @@ -2508,14 +2429,10 @@ describe('McpClientManager — PR 14 guardrails', () => { }) as unknown as McpClient, ); const config = configWithServers({ x: { command: 'node' } }); - const manager = new McpClientManager( + const manager = mkManager({ config, - {} as ToolRegistry, - undefined, - undefined, - undefined, - { clientBudget: 1, budgetMode: 'enforce' }, - ); + options: { budgetConfig: { clientBudget: 1, budgetMode: 'enforce' } }, + }); await manager.discoverMcpToolsForServer('x', config); // Slot released on weReservedSlot+catch path AND the transport // was closed before dropping the client reference. @@ -2528,7 +2445,7 @@ describe('McpClientManager — PR 14 guardrails', () => { process.env['QWEN_SERVE_MCP_CLIENT_BUDGET'] = 'abc'; try { const config = configWithServers({}); - const manager = new McpClientManager(config, {} as ToolRegistry); + const manager = mkManager({ config }); expect(manager.getMcpClientBudget()).toBeUndefined(); // Operator-visible breadcrumb landed on stderr. const calls = writeSpy.mock.calls.map((c) => String(c[0])); @@ -2560,7 +2477,7 @@ describe('McpClientManager — PR 14 guardrails', () => { name === 'a' && disabled) as Config['isMcpServerDisabled'], }, ); - const manager = new McpClientManager(config, {} as ToolRegistry); + const manager = mkManager({ config }); // First connect while NOT disabled. await manager.discoverAllMcpTools(config); // Now operator disables 'a' mid-session. @@ -2594,14 +2511,10 @@ describe('McpClientManager — PR 14 guardrails', () => { }) as unknown as McpClient, ); const config = configWithServers({ x: { command: 'node' } }); - const manager = new McpClientManager( + const manager = mkManager({ config, - {} as ToolRegistry, - undefined, - undefined, - undefined, - { clientBudget: 1, budgetMode: 'enforce' }, - ); + options: { budgetConfig: { clientBudget: 1, budgetMode: 'enforce' } }, + }); await expect(manager.readResource('x', 'file:///a')).rejects.toThrow( /mid-handshake failure/, ); @@ -2615,7 +2528,7 @@ describe('McpClientManager — PR 14 guardrails', () => { // No budget → downgrade fires try { const config = configWithServers({}); - const manager = new McpClientManager(config, {} as ToolRegistry); + const manager = mkManager({ config }); expect(manager.getMcpBudgetMode()).toBe('off'); const calls = writeSpy.mock.calls.map((c) => String(c[0])); expect( @@ -2650,14 +2563,10 @@ describe('McpClientManager — PR 14 guardrails', () => { }) as unknown as McpClient, ); const config = configWithServers({ a: { command: 'node' } }); - const manager = new McpClientManager( + const manager = mkManager({ config, - {} as ToolRegistry, - undefined, - undefined, - undefined, - { clientBudget: 1, budgetMode: 'enforce' }, - ); + options: { budgetConfig: { clientBudget: 1, budgetMode: 'enforce' } }, + }); await manager.discoverAllMcpTools(config); // Transport closed before client reference dropped + slot released. expect(disconnectCalls).toBeGreaterThanOrEqual(1); @@ -2669,7 +2578,7 @@ describe('McpClientManager — PR 14 guardrails', () => { // No budget — pre-fix this passed through with mode='warn', // reaching emitBudgetTelemetry with clientBudget=undefined. const config = configWithServers({}); - const manager = new McpClientManager(config, {} as ToolRegistry); + const manager = mkManager({ config }); expect(manager.getMcpClientBudget()).toBeUndefined(); expect(manager.getMcpBudgetMode()).toBe('off'); }); @@ -2681,15 +2590,11 @@ describe('McpClientManager — PR 14 guardrails', () => { // path's downgrade so a future caller that bypasses validation // can't silently fail-open. const config = configWithServers({}); - const manager = new McpClientManager( + const manager = mkManager({ config, - {} as ToolRegistry, - undefined, - undefined, - undefined, // Invalid combination: enforce mode without a budget. - { budgetMode: 'enforce' }, - ); + options: { budgetConfig: { budgetMode: 'enforce' } }, + }); // Downgraded to off so tryReserveSlot doesn't masquerade as enforce. expect(manager.getMcpBudgetMode()).toBe('off'); }); @@ -2718,14 +2623,10 @@ describe('McpClientManager — PR 14 guardrails', () => { }) as unknown as McpClient, ); const config = configWithServers({ a: { command: 'node' } }); - const manager = new McpClientManager( + const manager = mkManager({ config, - {} as ToolRegistry, - undefined, - undefined, - undefined, - { clientBudget: 1, budgetMode: 'enforce' }, - ); + options: { budgetConfig: { clientBudget: 1, budgetMode: 'enforce' } }, + }); // First pass: a connects successfully, slot reserved. await manager.discoverAllMcpTools(config); expect(manager.getMcpClientAccounting().reservedSlots).toEqual(['a']); @@ -2803,18 +2704,16 @@ describe('McpClientManager — PR 14b push events + hysteresis', () => { c: { command: 'node' }, d: { command: 'node' }, }); - const manager = new McpClientManager( + const manager = mkManager({ config, - {} as ToolRegistry, - undefined, - undefined, - undefined, - { - clientBudget: 4, - budgetMode: 'warn', - onBudgetEvent: (e) => events.push(e), + options: { + budgetConfig: { + clientBudget: 4, + budgetMode: 'warn', + onBudgetEvent: (e) => events.push(e), + }, }, - ); + }); await manager.discoverAllMcpTools(config); const warnings = events.filter( (e) => (e as { kind: string }).kind === 'budget_warning', @@ -2845,18 +2744,16 @@ describe('McpClientManager — PR 14b push events + hysteresis', () => { a: { command: 'node' }, b: { command: 'node' }, }); - const manager = new McpClientManager( + const manager = mkManager({ config, - {} as ToolRegistry, - undefined, - undefined, - undefined, - { - clientBudget: 4, - budgetMode: 'warn', - onBudgetEvent: (e) => events.push(e), + options: { + budgetConfig: { + clientBudget: 4, + budgetMode: 'warn', + onBudgetEvent: (e) => events.push(e), + }, }, - ); + }); await manager.discoverAllMcpTools(config); expect( events.filter((e) => (e as { kind: string }).kind === 'budget_warning'), @@ -2882,18 +2779,16 @@ describe('McpClientManager — PR 14b push events + hysteresis', () => { const config = configWithServers({}, { getMcpServers: cfgGetter, } as Partial); - const manager = new McpClientManager( + const manager = mkManager({ config, - {} as ToolRegistry, - undefined, - undefined, - undefined, - { - clientBudget: 4, - budgetMode: 'warn', - onBudgetEvent: (e) => events.push(e), + options: { + budgetConfig: { + clientBudget: 4, + budgetMode: 'warn', + onBudgetEvent: (e) => events.push(e), + }, }, - ); + }); await manager.discoverAllMcpTools(config); expect( events.filter((e) => (e as { kind: string }).kind === 'budget_warning'), @@ -2943,14 +2838,15 @@ describe('McpClientManager — PR 14b push events + hysteresis', () => { a: { command: 'node' }, b: { command: 'node' }, }); - const manager = new McpClientManager( + const manager = mkManager({ config, - {} as ToolRegistry, - undefined, - undefined, - undefined, - { budgetMode: 'off', onBudgetEvent: (e) => events.push(e) }, - ); + options: { + budgetConfig: { + budgetMode: 'off', + onBudgetEvent: (e) => events.push(e), + }, + }, + }); await manager.discoverAllMcpTools(config); expect(events).toEqual([]); }); @@ -2966,18 +2862,16 @@ describe('McpClientManager — PR 14b push events + hysteresis', () => { b: { httpUrl: 'http://b' }, c: { url: 'http://c' }, }); - const manager = new McpClientManager( + const manager = mkManager({ config, - {} as ToolRegistry, - undefined, - undefined, - undefined, - { - clientBudget: 1, - budgetMode: 'enforce', - onBudgetEvent: (e) => events.push(e), + options: { + budgetConfig: { + clientBudget: 1, + budgetMode: 'enforce', + onBudgetEvent: (e) => events.push(e), + }, }, - ); + }); await manager.discoverAllMcpTools(config); const batches = events.filter( (e) => (e as { kind: string }).kind === 'refused_batch', @@ -3003,18 +2897,16 @@ describe('McpClientManager — PR 14b push events + hysteresis', () => { a: { command: 'node' }, b: { command: 'node' }, }); - const manager = new McpClientManager( + const manager = mkManager({ config, - {} as ToolRegistry, - undefined, - undefined, - undefined, - { - clientBudget: 5, - budgetMode: 'enforce', - onBudgetEvent: (e) => events.push(e), + options: { + budgetConfig: { + clientBudget: 5, + budgetMode: 'enforce', + onBudgetEvent: (e) => events.push(e), + }, }, - ); + }); await manager.discoverAllMcpTools(config); expect( events.filter((e) => (e as { kind: string }).kind === 'refused_batch'), @@ -3031,18 +2923,16 @@ describe('McpClientManager — PR 14b push events + hysteresis', () => { a: { command: 'node' }, b: { command: 'node' }, }); - const manager = new McpClientManager( + const manager = mkManager({ config, - {} as ToolRegistry, - undefined, - undefined, - undefined, - { - clientBudget: 1, - budgetMode: 'enforce', - onBudgetEvent: (e) => events.push(e), + options: { + budgetConfig: { + clientBudget: 1, + budgetMode: 'enforce', + onBudgetEvent: (e) => events.push(e), + }, }, - ); + }); // First pass fills the budget with `a`. `b` is refused — that's // the bulk refusal (length-1 batch). await manager.discoverAllMcpTools(config); @@ -3080,14 +2970,15 @@ describe('McpClientManager — PR 14b push events + hysteresis', () => { a: { command: 'node' }, b: { command: 'node' }, }); - const manager = new McpClientManager( + const manager = mkManager({ config, - {} as ToolRegistry, - undefined, - undefined, - undefined, - { budgetMode: 'off', onBudgetEvent: (e) => events.push(e) }, - ); + options: { + budgetConfig: { + budgetMode: 'off', + onBudgetEvent: (e) => events.push(e), + }, + }, + }); await manager.discoverAllMcpTools(config); // Force discovery refusal would be impossible in off mode (no // budget). Disconnect-then-rediscover also no-ops the state @@ -3110,18 +3001,16 @@ describe('McpClientManager — PR 14b push events + hysteresis', () => { d: { tcp: 'ws://d' }, // websocket (refused) e: { type: 'sdk', command: 'sdk' }, // sdk (refused) }); - const manager = new McpClientManager( + const manager = mkManager({ config, - {} as ToolRegistry, - undefined, - undefined, - undefined, - { - clientBudget: 1, - budgetMode: 'enforce', - onBudgetEvent: (e) => events.push(e), + options: { + budgetConfig: { + clientBudget: 1, + budgetMode: 'enforce', + onBudgetEvent: (e) => events.push(e), + }, }, - ); + }); await manager.discoverAllMcpTools(config); const batches = events.filter( (e) => (e as { kind: string }).kind === 'refused_batch', @@ -3142,18 +3031,16 @@ describe('McpClientManager — PR 14b push events + hysteresis', () => { b: { command: 'node' }, c: { command: 'node' }, }); - const manager = new McpClientManager( + const manager = mkManager({ config, - {} as ToolRegistry, - undefined, - undefined, - undefined, - { - clientBudget: 1, - budgetMode: 'warn', - onBudgetEvent: (e) => events.push(e), + options: { + budgetConfig: { + clientBudget: 1, + budgetMode: 'warn', + onBudgetEvent: (e) => events.push(e), + }, }, - ); + }); await manager.discoverAllMcpTools(config); // warn mode: no refusals, but the warning may fire (3/1 ratio crosses 0.75). expect( @@ -3172,18 +3059,16 @@ describe('McpClientManager — PR 14b push events + hysteresis', () => { c: { command: 'node' }, d: { command: 'node' }, }); - const manager = new McpClientManager( + const manager = mkManager({ config, - {} as ToolRegistry, - undefined, - undefined, - undefined, - { - clientBudget: 4, - budgetMode: 'warn', - onBudgetEvent: (e) => events.push(e), + options: { + budgetConfig: { + clientBudget: 4, + budgetMode: 'warn', + onBudgetEvent: (e) => events.push(e), + }, }, - ); + }); await manager.discoverAllMcpTools(config); // First crossing fired one warning. expect( @@ -3218,18 +3103,16 @@ describe('McpClientManager — PR 14b push events + hysteresis', () => { c: { command: 'node' }, d: { command: 'node' }, }); - const manager = new McpClientManager( + const manager = mkManager({ config, - {} as ToolRegistry, - undefined, - undefined, - undefined, - { - clientBudget: 1, - budgetMode: 'enforce', - onBudgetEvent: (e) => events.push(e), + options: { + budgetConfig: { + clientBudget: 1, + budgetMode: 'enforce', + onBudgetEvent: (e) => events.push(e), + }, }, - ); + }); await manager.discoverAllMcpToolsIncremental(config); const batches = events.filter( (e) => (e as { kind: string }).kind === 'refused_batch', @@ -3261,18 +3144,16 @@ describe('McpClientManager — PR 14b push events + hysteresis', () => { c: { command: 'node' }, d: { command: 'node' }, }); - const manager = new McpClientManager( + const manager = mkManager({ config, - {} as ToolRegistry, - undefined, - undefined, - undefined, - { - clientBudget: 4, - budgetMode: 'warn', - onBudgetEvent: (e) => events.push(e), + options: { + budgetConfig: { + clientBudget: 4, + budgetMode: 'warn', + onBudgetEvent: (e) => events.push(e), + }, }, - ); + }); await manager.discoverAllMcpTools(config); expect( events.filter((e) => (e as { kind: string }).kind === 'budget_warning'), diff --git a/packages/core/src/tools/mcp-client-manager.ts b/packages/core/src/tools/mcp-client-manager.ts index 25f231027d1..53a4dff7836 100644 --- a/packages/core/src/tools/mcp-client-manager.ts +++ b/packages/core/src/tools/mcp-client-manager.ts @@ -343,6 +343,26 @@ function readBudgetFromEnv(): McpBudgetConfig { return { clientBudget, budgetMode }; } +/** + * F2 (#4175 commit 6 review fix — wenshao R9 / PR A): options bag for + * `McpClientManager` construction, replacing the prior 5 trailing + * positional parameters (`eventEmitter`, `sendSdkMcpMessage`, + * `healthConfig`, `budgetConfig`, `pool`). Pre-fix every test site + * threaded 4 explicit `undefined`s to reach the trailing `pool` arg — + * the fixed positions also blocked future option additions without + * re-ordering. The options-object form lets each caller name only the + * fields it cares about and keeps the constructor signature stable + * across future additions (e.g. when the W8 health-monitor wire-up + * lands a new `reconnectStrategy` knob). + */ +export interface McpClientManagerOptions { + eventEmitter?: EventEmitter; + sendSdkMcpMessage?: SendSdkMcpMessage; + healthConfig?: Partial; + budgetConfig?: McpBudgetConfig; + pool?: import('./mcp-transport-pool.js').McpTransportPool; +} + /** * Manages the lifecycle of multiple MCP clients, including local child processes. * This class is responsible for starting, stopping, and discovering tools from @@ -513,19 +533,16 @@ export class McpClientManager { constructor( config: Config, toolRegistry: ToolRegistry, - eventEmitter?: EventEmitter, - sendSdkMcpMessage?: SendSdkMcpMessage, - healthConfig?: Partial, - budgetConfig?: McpBudgetConfig, - pool?: import('./mcp-transport-pool.js').McpTransportPool, + options: McpClientManagerOptions = {}, ) { this.cliConfig = config; this.toolRegistry = toolRegistry; - this.pool = pool; + this.pool = options.pool; - this.eventEmitter = eventEmitter; - this.sendSdkMcpMessage = sendSdkMcpMessage; - this.healthConfig = { ...DEFAULT_HEALTH_CONFIG, ...healthConfig }; + this.eventEmitter = options.eventEmitter; + this.sendSdkMcpMessage = options.sendSdkMcpMessage; + this.healthConfig = { ...DEFAULT_HEALTH_CONFIG, ...options.healthConfig }; + const budgetConfig = options.budgetConfig; // Tests inject `budgetConfig` directly; production reads env vars // set by `qwen serve --mcp-client-budget=N --mcp-budget-mode=X` diff --git a/packages/core/src/tools/mcp-transport-pool.ts b/packages/core/src/tools/mcp-transport-pool.ts index a4cdb4f6b95..e1d671cdd74 100644 --- a/packages/core/src/tools/mcp-transport-pool.ts +++ b/packages/core/src/tools/mcp-transport-pool.ts @@ -237,13 +237,6 @@ export class McpTransportPool { // session caller. Also handles the leftover stale entry case // (any pre-existing zombie not yet evicted by `onClosed`). if (existing && !existing.isTerminated()) { - const view = new SessionMcpView( - sessionToolRegistry, - sessionPromptRegistry, - sessionId, - serverName, - cfg, - ); // F2 (#4175 commit 6 review fix — wenshao W10): index update // happens AFTER `attach` succeeds. Pre-fix the order was // reversed; an `attach` rejection (e.g., entry transitioned @@ -251,11 +244,20 @@ export class McpTransportPool { // `attach` call) left a stale `sessionToEntries[sessionId]` // mapping with no matching `entry.refs.has(sessionId)` — // `releaseSession` would later iterate the stale id and call - // `entry.detach` on a non-attached session. + // `entry.detach` on a non-attached session. W11/PR A: + // `attachPooledSession` is the shared view+attach helper; + // call-site ordering (indexAttach AFTER attach, terminal-state + // self-heal in catch) stays here, not in the helper. try { - const conn = existing.attach(sessionId, view, { - release: () => this.release(id, sessionId), - }); + const conn = this.attachPooledSession( + existing, + id, + serverName, + cfg, + sessionId, + sessionToolRegistry, + sessionPromptRegistry, + ); this.indexAttach(sessionId, id); return conn; } catch (err) { @@ -355,16 +357,11 @@ export class McpTransportPool { sessionPromptRegistry, ); } catch (err) { - // R24 T17: only release if THIS acquire actually reserved a - // new slot. `'already_held'` means the sibling holds it; not - // ours to release. - if ( - this.opts.budget !== undefined && - reservationResult === 'reserved' && - !this.hasNameSibling(serverName) - ) { - this.opts.budget.release(serverName); - } + // R24 T17 (codified by W11/PR A as a shared helper): only + // release if THIS acquire actually reserved a new slot. + // `'already_held'` means the sibling holds it; not ours to + // release. + this.rollbackReservationOnSpawnFailure(reservationResult, serverName); throw err; } } @@ -397,20 +394,14 @@ export class McpTransportPool { // spawn failure (V21-4) so a transient connect failure // doesn't leak the slot until daemon restart. // - // R24 T17: only release if THIS acquire actually reserved a - // new slot (`reservationResult === 'reserved'`). When - // `tryReserve` returned `'already_held'`, a same-name - // sibling held the slot; this acquire reserved nothing, so - // a release here would phantom-decrement the budget counter - // if the sibling were concurrently evicted between - // `tryReserve` and this catch. - if ( - this.opts.budget !== undefined && - reservationResult === 'reserved' && - !this.hasNameSibling(serverName) - ) { - this.opts.budget.release(serverName); - } + // R24 T17 contract (codified as `rollbackReservationOnSpawnFailure` + // helper in W11/PR A): only release if THIS acquire actually + // reserved a new slot (`reservationResult === 'reserved'`). + // `'already_held'` means a sibling holds the slot — phantom- + // releasing here would decrement the counter if the sibling + // were concurrently evicted between `tryReserve` and this + // catch. + this.rollbackReservationOnSpawnFailure(reservationResult, serverName); throw err; }); this.spawnInFlight.set(id, inFlight); @@ -455,17 +446,16 @@ export class McpTransportPool { ); } - const view = new SessionMcpView( - sessionToolRegistry, - sessionPromptRegistry, - sessionId, - serverName, - cfg, - ); try { - const conn = entry.attach(sessionId, view, { - release: () => this.release(id, sessionId), - }); + const conn = this.attachPooledSession( + entry, + id, + serverName, + cfg, + sessionId, + sessionToolRegistry, + sessionPromptRegistry, + ); // F2 (#4175 commit 6 review fix — qwen-latest W111): re-index // AFTER attach succeeds. Pre-fix the early `indexAttach` at the // top of this branch was enough on the unpooled path (W77) @@ -831,6 +821,79 @@ export class McpTransportPool { // ---------- internals ---------- + /** + * F2 (#4175 commit 6 review fix — wenshao W11 / PR A): shared + * view+attach helper for the two POOLED `acquire()` branches (the + * fast-path for an existing entry, and the post-spawn attach after + * `await inFlight`). Pre-fix both branches inlined the same 3-step + * pattern (build view → entry.attach → return) with identical + * release-callback wiring; PR A's stated cleanup goal is to dedupe + * without losing the per-call-site race-window invariant comments + * that explain WHY each branch's surrounding ordering is what it is. + * + * NOT used by `createUnpooledConnection` — the unpooled release + * callback runs `entry.forceShutdown('manual')` directly (no pool + * refcount accounting since unpooled entries are per-session) and + * also calls `indexDetach` from the release callback itself. + * + * Caller is responsible for: + * - Terminal-state pre-check (`!entry.isTerminated()`) + race- + * window self-heal (`evictEntry` on the W125 catch path). + * - Reverse-index ordering (early `indexAttach` BEFORE await on + * the post-spawn branch per W90; AFTER attach on the fast-path + * per W10; W111 re-indexAttach AFTER attach on post-spawn). + * The race-window comments live at the call sites because they + * describe the surrounding ordering, not the attach itself. + */ + private attachPooledSession( + entry: PoolEntry, + id: ConnectionId, + serverName: string, + cfg: MCPServerConfig, + sessionId: string, + sessionToolRegistry: ToolRegistry, + sessionPromptRegistry: PromptRegistry, + ): PooledConnection { + const view = new SessionMcpView( + sessionToolRegistry, + sessionPromptRegistry, + sessionId, + serverName, + cfg, + ); + return entry.attach(sessionId, view, { + release: () => this.release(id, sessionId), + }); + } + + /** + * F2 (#4175 commit 6 review fix — wenshao W11 / PR A; codifies the + * R24 T17 contract): roll back THIS acquire's slot reservation on + * spawn failure. Used by both the unpooled-spawn catch and the + * pooled-spawn-in-flight catch — both decisions are identical: + * - `'reserved'` → THIS acquire newly held the slot; release + * if no sibling holds it + * - `'already_held'` → sibling holds it; never release here (the + * sibling's own onClosed / evictEntry will + * handle it). Pre-R24 the bare + * `!hasNameSibling()` check would phantom- + * release a slot this acquire never reserved + * when the sibling was concurrently evicted. + * - `undefined` → no budget configured; nothing to do. + */ + private rollbackReservationOnSpawnFailure( + reservationResult: 'reserved' | 'already_held' | undefined, + serverName: string, + ): void { + if ( + this.opts.budget !== undefined && + reservationResult === 'reserved' && + !this.hasNameSibling(serverName) + ) { + this.opts.budget.release(serverName); + } + } + /** * F2 (#4175 commit 6 review fix — wenshao R22 W125-followup A+B): * Single source of truth for evicting a pooled entry from diff --git a/packages/core/src/tools/pid-descendants.test.ts b/packages/core/src/tools/pid-descendants.test.ts index 1346a567575..cb21d7169c9 100644 --- a/packages/core/src/tools/pid-descendants.test.ts +++ b/packages/core/src/tools/pid-descendants.test.ts @@ -59,13 +59,19 @@ describe('pid-descendants', () => { // Cross-platform integration test: spawn a wrapper that itself // spawns a child, verify listDescendantPids finds both levels. - // Gated on POSIX availability of `pgrep` (skipped on Windows + on - // CI without pgrep). + // + // F2 (#4175 commit 6 review fix — wenshao R10 / R23 T7 / PR A): + // Pre-fix gate skipped on `CI === '1'` (pgrep not always available + // on minimal CI runners). Post-fix the snapshot path uses + // `ps -A -o pid=,ppid=` (POSIX standard, available on every + // non-distroless Linux/macOS), so we keep only the Windows skip; + // the snapshot's per-pid pgrep fallback covers the rare BusyBox + // { - it('enumerates one level of children via pgrep', async () => { + it('enumerates one level of children via process-tree snapshot', async () => { // Parent process spawns a node child with `--eval` that sleeps. // Use spawn directly so we control the lifecycle. const parent = spawn('/bin/sh', [ diff --git a/packages/core/src/tools/pid-descendants.ts b/packages/core/src/tools/pid-descendants.ts index 3b028cdfb4d..363e5683e37 100644 --- a/packages/core/src/tools/pid-descendants.ts +++ b/packages/core/src/tools/pid-descendants.ts @@ -12,11 +12,22 @@ const debugLogger = createDebugLogger('PidDescendants'); const execFileAsync = promisify(execFile); /** - * Wall-clock budget for each individual `pgrep` / `Get-CimInstance` call. + * Wall-clock budget for each individual snapshot / per-pid query call. * Bounded so a hung process-table walk can't stall pool shutdown. */ const QUERY_TIMEOUT_MS = 2_000; +/** + * F2 (#4175 commit 6 review fix — wenshao R10 / R23 T7 / PR A): cap + * for `execFile`'s internal stdout buffer on the snapshot path. Default + * is 1MB, which is enough for ~30k-process hosts (~30 bytes/line) but + * an 8MB cap covers >250k-process pathological cases without forcing + * the truncation-or-fallback branch on real machines. The cap applies + * only to the snapshot family of calls; the per-pid `pgrep -P` fallback + * has a tiny output (just the children of one pid) and uses the default. + */ +const SNAPSHOT_MAXBUFFER_BYTES = 8 * 1024 * 1024; + /** * Hard cap on recursion depth + total descendants returned. Defense * against runaway process trees (forkbomb-style) or pathological @@ -35,15 +46,28 @@ const MAX_DEPTH = 8; * `uvx ...`, `pnpm dlx ...`) that would otherwise leak when the * pool entry's primary child is killed. * + * F2 (#4175 commit 6 review fix — wenshao R10 / R23 T7 / PR A): + * the implementation switched from per-pid `pgrep -P ` BFS + * (Linux/macOS) / per-pid `Get-CimInstance -Filter "ParentProcessId=$p"` + * BFS (Windows) — which forked one subprocess per node visited — to + * a single process-table snapshot followed by an in-memory tree walk. + * Two motivations: (1) ~B^D fork count → 1 fork per call, on the + * hot pool-shutdown path; (2) snapshot consistency — pre-fix BFS + * could miss descendants that forked between adjacent BFS levels. + * * Behavior: - * - Linux/macOS: `pgrep -P ` walked recursively, BFS order - * - Windows: PowerShell `Get-CimInstance Win32_Process` filtered - * by `ParentProcessId`, walked recursively - * - Either platform: graceful degradation if the tool is missing - * or the query times out — returns whatever was collected so far - * and logs a warning. Pool shutdown still proceeds; orphan - * processes will be reaped by the OS eventually (Linux init, - * Windows job objects). + * - Linux/macOS: `ps -A -o pid=,ppid=` snapshot, in-memory BFS walk + * over the parsed `Map`. + * - Windows: PowerShell `Get-CimInstance Win32_Process` → + * `ConvertTo-Csv` snapshot of all `(ProcessId, ParentProcessId)` + * rows, in-memory walk. + * - Either platform: graceful degradation if the snapshot tool is + * missing / blocked / times out — falls back to per-pid BFS + * (preserves the pre-fix code path so BusyBox `ps` { } async function listDescendantPidsUnix(root: number): Promise { + let tree: Map | undefined; + try { + tree = await snapshotProcessTreeUnix(); + } catch (err) { + debugLogger.warn( + `Unix snapshot via 'ps -A' failed (${ + err instanceof Error ? err.message : String(err) + }); falling back to per-pid pgrep BFS`, + ); + } + if (tree) { + return walkDescendants(tree, root); + } + return await listDescendantPidsUnixPgrepFallback(root); +} + +async function snapshotProcessTreeUnix(): Promise> { + // `ps -A -o pid=,ppid=` + // -A: all processes (POSIX, equivalent to -e; -A is unambiguous + // across BSD/SysV — BSD historically used -e for env display). + // -o pid=,ppid=: pid + ppid columns; trailing `=` suppresses each + // column header (POSIX standard). + // Output is " " per line, no header. + const { stdout } = await execFileAsync('ps', ['-A', '-o', 'pid=,ppid='], { + timeout: QUERY_TIMEOUT_MS, + maxBuffer: SNAPSHOT_MAXBUFFER_BYTES, + }); + const childrenByPpid = new Map(); + let parsedRows = 0; + for (const line of stdout.split('\n')) { + const m = line.trim().match(/^(\d+)\s+(\d+)$/); + if (!m) continue; + const pid = Number.parseInt(m[1], 10); + const ppid = Number.parseInt(m[2], 10); + if (!Number.isFinite(pid) || pid <= 0) continue; + if (!Number.isFinite(ppid) || ppid < 0) continue; + parsedRows += 1; + const arr = childrenByPpid.get(ppid); + if (arr) arr.push(pid); + else childrenByPpid.set(ppid, [pid]); + } + if (parsedRows === 0) { + // Snapshot tool ran but produced no parseable lines (e.g. BusyBox + // `ps` without `-o` support echoing usage). Treat as failure so + // the caller's catch falls back to per-pid pgrep. + throw new Error( + `'ps -A -o pid=,ppid=' returned no parseable rows (stdout length=${stdout.length})`, + ); + } + return childrenByPpid; +} + +async function listDescendantPidsUnixPgrepFallback( + root: number, +): Promise { const all: number[] = []; const queue: Array<{ pid: number; depth: number }> = [ { pid: root, depth: 0 }, @@ -107,6 +186,75 @@ async function listDescendantPidsUnix(root: number): Promise { } async function listDescendantPidsWin(root: number): Promise { + let tree: Map | undefined; + try { + tree = await snapshotProcessTreeWin(); + } catch (err) { + debugLogger.warn( + `Windows snapshot via Get-CimInstance failed (${ + err instanceof Error ? err.message : String(err) + }); falling back to per-pid filter BFS`, + ); + } + if (tree) { + return walkDescendants(tree, root); + } + return await listDescendantPidsWinPerPidFallback(root); +} + +async function snapshotProcessTreeWin(): Promise> { + // Single-shot CIM query for ALL processes' (ProcessId, + // ParentProcessId), CSV-formatted for stable parsing. + // F2 (#4175 commit 5 review fix — wenshao R5): no integer + // interpolation into the script; this query takes no parameters + // (we filter in-memory after the snapshot returns). + // + // F2 (#4175 commit 6 review fix — wenshao PR-A-R3 T3): explicit + // `-Delimiter ","` on `ConvertTo-Csv`. Pre-fix PowerShell 5.1 + // honored the system locale's list separator (semicolon on + // German / French / Dutch / etc.), so the regex + // `^"(\d+)","(\d+)"$` below never matched on those locales → + // snapshot threw → fell back to the slower per-pid CIM path + // (~0.5-1s extra PowerShell startup latency per descendant on + // every shutdown). Forcing comma normalizes the output across + // locales / PS versions. + const script = + 'Get-CimInstance -ClassName Win32_Process ' + + '| Select-Object ProcessId,ParentProcessId ' + + '| ConvertTo-Csv -NoTypeInformation -Delimiter ","'; + const { stdout } = await execFileAsync( + 'powershell.exe', + ['-NoProfile', '-NonInteractive', '-Command', script], + { timeout: QUERY_TIMEOUT_MS, maxBuffer: SNAPSHOT_MAXBUFFER_BYTES }, + ); + const childrenByPpid = new Map(); + let parsedRows = 0; + // CSV: first line is header `"ProcessId","ParentProcessId"`, + // subsequent lines are `"",""`. + const lines = stdout.split(/\r?\n/); + for (let i = 1; i < lines.length; i++) { + const m = lines[i].match(/^"(\d+)","(\d+)"$/); + if (!m) continue; + const pid = Number.parseInt(m[1], 10); + const ppid = Number.parseInt(m[2], 10); + if (!Number.isFinite(pid) || pid <= 0) continue; + if (!Number.isFinite(ppid) || ppid < 0) continue; + parsedRows += 1; + const arr = childrenByPpid.get(ppid); + if (arr) arr.push(pid); + else childrenByPpid.set(ppid, [pid]); + } + if (parsedRows === 0) { + throw new Error( + `Get-CimInstance snapshot returned no parseable rows (stdout length=${stdout.length})`, + ); + } + return childrenByPpid; +} + +async function listDescendantPidsWinPerPidFallback( + root: number, +): Promise { const all: number[] = []; const queue: Array<{ pid: number; depth: number }> = [ { pid: root, depth: 0 }, @@ -161,6 +309,49 @@ async function listDescendantPidsWin(root: number): Promise { return all; } +/** + * F2 (#4175 commit 6 review fix — wenshao R10 / R23 T7 / PR A): + * shared in-memory BFS over a snapshot tree. Replaces both + * platforms' per-node subprocess forks once the snapshot has been + * obtained. Same MAX_DESCENDANTS / MAX_DEPTH caps as the legacy + * fallback path. Returns BFS order — children before grandchildren. + * + * F2 (#4175 commit 6 review fix — wenshao PR-A-R2 #1): `visited` + * set prevents BFS revisits when the snapshot captures a PID-reuse + * cycle (rare but possible on busy hosts with rapid pid churn + * between snapshot start and parse — Linux pid wraparound can make + * `ps -A` show a freed pid in a different parent's children list, + * producing an A→B / B→A cycle). Pre-fix the cycle would fill the + * MAX_DESCENDANTS=256 quota with duplicate entries and starve + * legitimate descendants. The per-pid `pgrep` BFS fallback had the + * same theoretical issue but was less exposed because each + * `pgrep -P pid` call only returns DIRECT children; the snapshot + * captures the whole tree at once. `root` is seeded into `visited` + * so a malformed snapshot listing root as a descendant of one of + * its own children doesn't re-enqueue root. + */ +function walkDescendants(tree: Map, root: number): number[] { + const all: number[] = []; + const visited = new Set([root]); + const queue: Array<{ pid: number; depth: number }> = [ + { pid: root, depth: 0 }, + ]; + while (queue.length && all.length < MAX_DESCENDANTS) { + const { pid, depth } = queue.shift()!; + if (depth >= MAX_DEPTH) continue; + const children = tree.get(pid); + if (!children) continue; + for (const child of children) { + if (visited.has(child)) continue; + visited.add(child); + if (all.length >= MAX_DESCENDANTS) break; + all.push(child); + queue.push({ pid: child, depth: depth + 1 }); + } + } + return all; +} + /** * Send SIGTERM to a list of pids, tolerating per-pid failures * (already exited, permission denied, etc.). On Windows, Node's diff --git a/packages/core/src/tools/session-mcp-view.ts b/packages/core/src/tools/session-mcp-view.ts index 9ae2311ddb1..c6b37535c22 100644 --- a/packages/core/src/tools/session-mcp-view.ts +++ b/packages/core/src/tools/session-mcp-view.ts @@ -13,19 +13,55 @@ import type { ToolRegistry } from './tool-registry.js'; const debugLogger = createDebugLogger('McpPool:View'); -function passesNameFilter( - name: string, +/** + * F2 (#4175 commit 6 review fix — wenshao W12 / PR A; PR-A-R2 #2 + * folded the exports to delegate here): precompute lookup `Set`s + * once per `applyTools` / `applyPrompts` pass so the per-tool + * predicate is O(1) instead of repeating an array scan for every + * snapshot entry. Same semantics: `excludeTools` is direct-equality + * match (parens form not stripped — intentional pre-F2 behavior + * preserved); `includeTools` strips the first `(...)` suffix so + * `toolName(args)` matches `toolName`. + * + * PR-A-R2 #2: `passesSessionFilter` / `passesSessionPromptFilter` + * (exported below for unit-testability) now route THROUGH + * `compiledFilterAccepts(compileNameFilter(...))` so there is a + * single source of truth for the predicate. Pre-fix the exports + * called a separate `passesNameFilter` array-based implementation + * with the same semantics, creating a drift risk where a future + * change to one impl wouldn't be caught by tests of the other. + * The Set construction is per-call for these exports (cheap for + * tests / one-off probes); the bulk paths in + * `applyTools`/`applyPrompts` still construct ONE filter per pass. + */ +interface CompiledNameFilter { + excludeSet?: ReadonlySet; + includeSet?: ReadonlySet; +} + +function compileNameFilter( includeTools?: readonly string[], excludeTools?: readonly string[], +): CompiledNameFilter { + return { + excludeSet: excludeTools ? new Set(excludeTools) : undefined, + includeSet: includeTools + ? new Set( + includeTools.map((entry) => + entry.includes('(') ? entry.slice(0, entry.indexOf('(')) : entry, + ), + ) + : undefined, + }; +} + +function compiledFilterAccepts( + filter: CompiledNameFilter, + name: string, ): boolean { - if (excludeTools?.includes(name)) return false; - if (!includeTools) return true; - return includeTools.some((entry) => { - const stripped = entry.includes('(') - ? entry.slice(0, entry.indexOf('(')) - : entry; - return stripped === name; - }); + if (filter.excludeSet?.has(name)) return false; + if (!filter.includeSet) return true; + return filter.includeSet.has(name); } /** @@ -46,13 +82,21 @@ function passesNameFilter( * support, intentionally matching the existing pre-F2 behavior so * operators don't see semantic divergence between the two filter * lists when migrating sessions through pool mode. + * + * PR-A-R2 #2: routes through `compiledFilterAccepts(compileNameFilter(...))` + * so the bulk-path predicate and the exported per-name predicate + * share one implementation. Set construction is paid per call here + * (negligible for unit tests / one-off audit-path probes). */ export function passesSessionFilter( tool: DiscoveredMCPTool, includeTools?: readonly string[], excludeTools?: readonly string[], ): boolean { - return passesNameFilter(tool.serverToolName, includeTools, excludeTools); + return compiledFilterAccepts( + compileNameFilter(includeTools, excludeTools), + tool.serverToolName, + ); } /** @@ -68,13 +112,19 @@ export function passesSessionFilter( * parens form `excludeTools: ['toolName(args)']` which only matches * tools (the parens-stripping in `passesSessionFilter` matches * `toolName` in the include list, not the exclude list). + * + * PR-A-R2 #2: same delegation to the compiled path as + * `passesSessionFilter`. */ export function passesSessionPromptFilter( promptName: string, includeTools?: readonly string[], excludeTools?: readonly string[], ): boolean { - return passesNameFilter(promptName, includeTools, excludeTools); + return compiledFilterAccepts( + compileNameFilter(includeTools, excludeTools), + promptName, + ); } /** @@ -127,11 +177,17 @@ export class SessionMcpView { */ applyTools(snapshot: readonly DiscoveredMCPTool[]): void { this.sessionToolRegistry.removeMcpToolsByServer(this.serverName); + // W12/PR A: precompute filter Sets once per pass so the per-tool + // predicate is O(1). Pre-fix `passesSessionFilter` re-scanned the + // includeTools / excludeTools arrays inside every iteration — + // O(M tools × N filter entries) per pass. Same semantics applied. + const filter = compileNameFilter( + this.cfg.includeTools, + this.cfg.excludeTools, + ); let registered = 0; for (const tool of snapshot) { - if ( - !passesSessionFilter(tool, this.cfg.includeTools, this.cfg.excludeTools) - ) { + if (!compiledFilterAccepts(filter, tool.serverToolName)) { continue; } // V21 C7: per-session trust copy. `withTrust` returns the same @@ -176,15 +232,14 @@ export class SessionMcpView { */ applyPrompts(snapshot: readonly DiscoveredMCPPrompt[]): void { this.sessionPromptRegistry.removePromptsByServer(this.serverName); + // W12/PR A: same Set precompute as applyTools. + const filter = compileNameFilter( + this.cfg.includeTools, + this.cfg.excludeTools, + ); let registered = 0; for (const prompt of snapshot) { - if ( - !passesSessionPromptFilter( - prompt.name, - this.cfg.includeTools, - this.cfg.excludeTools, - ) - ) { + if (!compiledFilterAccepts(filter, prompt.name)) { continue; } try { diff --git a/packages/core/src/tools/tool-registry.ts b/packages/core/src/tools/tool-registry.ts index a9dcb637b95..ab79c8b04c8 100644 --- a/packages/core/src/tools/tool-registry.ts +++ b/packages/core/src/tools/tool-registry.ts @@ -196,21 +196,19 @@ export class ToolRegistry { sendSdkMcpMessage?: SendSdkMcpMessage, ) { this.config = config; - this.mcpClientManager = new McpClientManager( - this.config, - this, + // F2 (#4175 commit 6 review fix — wenshao R9 / PR A): options-bag + // ctor; previously 7 positional args with `undefined, undefined` + // sentinels for `healthConfig` / `budgetConfig`. `pool` is + // forwarded from Config (set by daemon-mode QwenAgent in + // `newSessionConfig`); when undefined the manager keeps its pre-F2 + // per-session spawn behavior, when defined non-SDK MCP discovery + // goes through `pool.acquire` so N sessions in the same workspace + // share one transport per unique server config. + this.mcpClientManager = new McpClientManager(this.config, this, { eventEmitter, sendSdkMcpMessage, - // F2 (#4175 commit 4): forward the workspace-shared MCP transport - // pool from Config (set by daemon-mode QwenAgent in - // newSessionConfig). When undefined, McpClientManager keeps its - // pre-F2 per-session spawn behavior. When defined, non-SDK MCP - // discovery goes through pool.acquire so N sessions in the same - // workspace share one transport per unique server config. - undefined, // healthConfig: keep manager defaults - undefined, // budgetConfig: keep manager defaults - this.config.getMcpTransportPool(), - ); + pool: this.config.getMcpTransportPool(), + }); } /**