From 29d6afdeb903dd34e6a801867d5877a9fa6a3887 Mon Sep 17 00:00:00 2001 From: gengruilin Date: Sun, 27 Sep 2026 21:46:50 +0800 Subject: [PATCH] fix(mcp): isolate status listener failures --- docs/event-producer-consumer.md | 2 +- docs/event-producer-consumer.zh.md | 2 +- docs/subsystems/tools.md | 12 +++++++----- docs/subsystems/tools.zh.md | 12 +++++++----- .../extensions/tool-cordis/src/api-catalog.ts | 6 +++--- packages/mcp/mcp-manager/README.i18n.yaml | 4 ++-- packages/mcp/mcp-manager/README.md | 2 +- packages/mcp/mcp-manager/README.zh.md | 2 +- packages/mcp/mcp-manager/src/index.ts | 10 ++-------- packages/mcp/mcp-manager/src/types.ts | 6 ++++-- .../mcp/mcp-manager/tests/manager.spec.ts | 19 ++++++++++++++----- 11 files changed, 43 insertions(+), 34 deletions(-) diff --git a/docs/event-producer-consumer.md b/docs/event-producer-consumer.md index bf9c2b000c..7309e2eaa8 100644 --- a/docs/event-producer-consumer.md +++ b/docs/event-producer-consumer.md @@ -55,7 +55,7 @@ This matrix shows which packages dispatch each harness-owned event and which pac | `hmr/reload` | `emit` | [`packages/boot/hmr/src/index.ts:35`](../packages/boot/hmr/src/index.ts) | [`hmr`](../packages/boot/hmr) (`emit`) | - | | `llm/adapters-updated` | `emit` | [`packages/llm/llm/src/types.ts:23`](../packages/llm/llm/src/types.ts) | [`llm`](../packages/llm/llm) (`events.dispatch`) | [`acp`](../packages/acp/acp), [`llm`](../packages/llm/llm), `remotes` | | `llm/stream` | `waterfall` | [`packages/llm/llm/src/index.ts:75`](../packages/llm/llm/src/index.ts) | [`llm`](../packages/llm/llm) (`waterfall`) | [`agent-loop`](../packages/core/agent-loop), [`llm`](../packages/llm/llm), [`llm-replay`](../packages/test-support/llm-replay), [`session-checkpoint-policy`](../packages/session/session-checkpoint-policy), [`session-title`](../packages/session/session-title) | -| `mcp-manager/status` | `emit` | [`packages/mcp/mcp-manager/src/types.ts:55`](../packages/mcp/mcp-manager/src/types.ts) | [`mcp-manager`](../packages/mcp/mcp-manager) (`emit`) | [`mcp-manager`](../packages/mcp/mcp-manager) | +| `mcp-manager/status` | `parallel` | [`packages/mcp/mcp-manager/src/types.ts:57`](../packages/mcp/mcp-manager/src/types.ts) | [`mcp-manager`](../packages/mcp/mcp-manager) (`parallel`) | [`mcp-manager`](../packages/mcp/mcp-manager) | | `permission-presets/catalog-changed` | `emit` | [`packages/interaction/permission-presets/src/types.ts:48`](../packages/interaction/permission-presets/src/types.ts) | [`permission-presets`](../packages/interaction/permission-presets) (`events.dispatch`) | `remotes` | | `plugin-manager/changed` | `emit` | [`packages/boot/plugin-manager/src/types.ts:243`](../packages/boot/plugin-manager/src/types.ts) | [`plugin-manager`](../packages/boot/plugin-manager) (`emit`) | `remotes` | | `plugin-manager/install-log` | `emit` | [`packages/boot/plugin-manager/src/types.ts:249`](../packages/boot/plugin-manager/src/types.ts) | [`plugin-manager`](../packages/boot/plugin-manager) (`emit`) | `remotes` | diff --git a/docs/event-producer-consumer.zh.md b/docs/event-producer-consumer.zh.md index b4fd5d9c74..e0ab2e7f8c 100644 --- a/docs/event-producer-consumer.zh.md +++ b/docs/event-producer-consumer.zh.md @@ -57,7 +57,7 @@ | `hmr/reload` | `emit` | [`packages/boot/hmr/src/index.ts:35`](../packages/boot/hmr/src/index.ts) | [`hmr`](../packages/boot/hmr) (`emit`) | - | | `llm/adapters-updated` | `emit` | [`packages/llm/llm/src/types.ts:23`](../packages/llm/llm/src/types.ts) | [`llm`](../packages/llm/llm) (`events.dispatch`) | [`acp`](../packages/acp/acp), [`llm`](../packages/llm/llm), `remotes` | | `llm/stream` | `waterfall` | [`packages/llm/llm/src/index.ts:75`](../packages/llm/llm/src/index.ts) | [`llm`](../packages/llm/llm) (`waterfall`) | [`agent-loop`](../packages/core/agent-loop), [`llm`](../packages/llm/llm), [`llm-replay`](../packages/test-support/llm-replay), [`session-checkpoint-policy`](../packages/session/session-checkpoint-policy), [`session-title`](../packages/session/session-title) | -| `mcp-manager/status` | `emit` | [`packages/mcp/mcp-manager/src/types.ts:55`](../packages/mcp/mcp-manager/src/types.ts) | [`mcp-manager`](../packages/mcp/mcp-manager) (`emit`) | [`mcp-manager`](../packages/mcp/mcp-manager) | +| `mcp-manager/status` | `parallel` | [`packages/mcp/mcp-manager/src/types.ts:57`](../packages/mcp/mcp-manager/src/types.ts) | [`mcp-manager`](../packages/mcp/mcp-manager) (`parallel`) | [`mcp-manager`](../packages/mcp/mcp-manager) | | `permission-presets/catalog-changed` | `emit` | [`packages/interaction/permission-presets/src/types.ts:48`](../packages/interaction/permission-presets/src/types.ts) | [`permission-presets`](../packages/interaction/permission-presets) (`events.dispatch`) | `remotes` | | `plugin-manager/changed` | `emit` | [`packages/boot/plugin-manager/src/types.ts:243`](../packages/boot/plugin-manager/src/types.ts) | [`plugin-manager`](../packages/boot/plugin-manager) (`emit`) | `remotes` | | `plugin-manager/install-log` | `emit` | [`packages/boot/plugin-manager/src/types.ts:249`](../packages/boot/plugin-manager/src/types.ts) | [`plugin-manager`](../packages/boot/plugin-manager) (`emit`) | `remotes` | diff --git a/docs/subsystems/tools.md b/docs/subsystems/tools.md index 035a85ed3e..219894c9d0 100644 --- a/docs/subsystems/tools.md +++ b/docs/subsystems/tools.md @@ -642,11 +642,11 @@ Source: [`packages/core/tools/src/index.ts`](../../packages/core/tools/src/index ### `mcp-manager/*` events - + -#### `mcp-manager/status` — emit +#### `mcp-manager/status` — parallel -One managed server's status transitioned. Emitted for every published change while the entry exists — connection-state transitions mirrored from the instance, `disabled` when the entry is configured off, and the manager-side `failed` reasons. Not emitted for an entry removed from configuration; observers learn removals from plugin configuration. +One managed server's status transitioned. Emitted for every published change while the entry exists — connection-state transitions mirrored from the instance, `disabled` when the entry is configured off, and the manager-side `failed` reasons. Not emitted for an entry removed from configuration; observers learn removals from plugin configuration. Lifecycle operations do not wait for listeners. Synchronous throws and returned promise rejections are logged without skipping other listeners. ```ts cordis-catalog /** @@ -655,11 +655,13 @@ One managed server's status transitioned. Emitted for every published change whi * from the instance, `disabled` when the entry is configured off, and the * manager-side `failed` reasons. Not emitted for an entry removed from * configuration; observers learn removals from plugin configuration. + * Lifecycle operations do not wait for listeners. Synchronous throws and + * returned promise rejections are logged without skipping other listeners. * @param name - the `servers` entry key. * @param status - the status now current for that server. - * @mode emit + * @mode parallel */ -'mcp-manager/status'(name: string, status: McpManagedServerStatus): void +'mcp-manager/status'(name: string, status: McpManagedServerStatus): void | Promise ``` Source: [`packages/mcp/mcp-manager/src/types.ts`](../../packages/mcp/mcp-manager/src/types.ts) diff --git a/docs/subsystems/tools.zh.md b/docs/subsystems/tools.zh.md index 74dabab71a..dabc11fe0e 100644 --- a/docs/subsystems/tools.zh.md +++ b/docs/subsystems/tools.zh.md @@ -642,11 +642,11 @@ Source: [`packages/core/tools/src/index.ts`](../../packages/core/tools/src/index ### `mcp-manager/*` events - + -#### `mcp-manager/status` — emit +#### `mcp-manager/status` — parallel -One managed server's status transitioned. Emitted for every published change while the entry exists — connection-state transitions mirrored from the instance, `disabled` when the entry is configured off, and the manager-side `failed` reasons. Not emitted for an entry removed from configuration; observers learn removals from plugin configuration. +One managed server's status transitioned. Emitted for every published change while the entry exists — connection-state transitions mirrored from the instance, `disabled` when the entry is configured off, and the manager-side `failed` reasons. Not emitted for an entry removed from configuration; observers learn removals from plugin configuration. Lifecycle operations do not wait for listeners. Synchronous throws and returned promise rejections are logged without skipping other listeners. ```ts cordis-catalog /** @@ -655,11 +655,13 @@ One managed server's status transitioned. Emitted for every published change whi * from the instance, `disabled` when the entry is configured off, and the * manager-side `failed` reasons. Not emitted for an entry removed from * configuration; observers learn removals from plugin configuration. + * Lifecycle operations do not wait for listeners. Synchronous throws and + * returned promise rejections are logged without skipping other listeners. * @param name - the `servers` entry key. * @param status - the status now current for that server. - * @mode emit + * @mode parallel */ -'mcp-manager/status'(name: string, status: McpManagedServerStatus): void +'mcp-manager/status'(name: string, status: McpManagedServerStatus): void | Promise ``` Source: [`packages/mcp/mcp-manager/src/types.ts`](../../packages/mcp/mcp-manager/src/types.ts) diff --git a/packages/extensions/tool-cordis/src/api-catalog.ts b/packages/extensions/tool-cordis/src/api-catalog.ts index 04ee870be0..08524d14ce 100644 --- a/packages/extensions/tool-cordis/src/api-catalog.ts +++ b/packages/extensions/tool-cordis/src/api-catalog.ts @@ -4135,10 +4135,10 @@ export const EVENT_API: readonly EventApiEntry[] = [ }, { name: 'mcp-manager/status', - mode: 'emit', - signature: '\'mcp-manager/status\'(name: string, status: McpManagedServerStatus): void', + mode: 'parallel', + signature: '\'mcp-manager/status\'(name: string, status: McpManagedServerStatus): void | Promise', summary: 'One managed server\'s status transitioned.', - description: 'One managed server\'s status transitioned. Emitted for every published change while the entry exists — connection-state transitions mirrored from the instance, `disabled` when the entry is configured off, and the manager-side `failed` reasons. Not emitted for an entry removed from configuration; observers learn removals from plugin configuration.', + description: 'One managed server\'s status transitioned. Emitted for every published change while the entry exists — connection-state transitions mirrored from the instance, `disabled` when the entry is configured off, and the manager-side `failed` reasons. Not emitted for an entry removed from configuration; observers learn removals from plugin configuration. Lifecycle operations do not wait for listeners. Synchronous throws and returned promise rejections are logged without skipping other listeners.', parameters: [{ name: 'name', description: 'the `servers` entry key.' }, { name: 'status', description: 'the status now current for that server.' }], }, { diff --git a/packages/mcp/mcp-manager/README.i18n.yaml b/packages/mcp/mcp-manager/README.i18n.yaml index 4c50da836f..eb751342ef 100644 --- a/packages/mcp/mcp-manager/README.i18n.yaml +++ b/packages/mcp/mcp-manager/README.i18n.yaml @@ -27,8 +27,8 @@ en: fc60ed8e7f85e54f zh: 5bb2a1983992393f /deepseek-ai-dsh-mcp-manager/use-this-package/observing-and-recovering: - en: ec233622e1a97c35 - zh: 73957928c9a11d49 + en: 6560cc7f6d8f0e7c + zh: d69bac5ebaaac7d6 /deepseek-ai-dsh-mcp-manager/understand-the-implementation: en: 8a74186fa5bfe79d zh: c7c952876ca2992b diff --git a/packages/mcp/mcp-manager/README.md b/packages/mcp/mcp-manager/README.md index 0bdd2e2ef0..24bb8ba32c 100644 --- a/packages/mcp/mcp-manager/README.md +++ b/packages/mcp/mcp-manager/README.md @@ -81,7 +81,7 @@ Settings writes persist through ConfigEditor with revision checks; Loader update ### Observing and recovering -`ctx.mcpServers.list()` answers a management surface with one row per configured server: transport, durable intent, deletability, and the live status (`connecting`, `connected` with its tool count, `reconnecting` with the attempt budget, `failed` with the reason and last error, or `disabled`). Every transition also emits the `mcp-manager/status` event. Reconnection itself stays inside mcp-client (backoff from 500 ms to 30 s, ten attempts per outage); `restart(name)` is the manual recovery beyond it — a fresh instance with a fresh budget, which is the only way back from a `failed` terminal state short of a config change. `probe(name)` connects, lists tools, and disconnects without registering anything — the "test this server" affordance, and it works on a disabled entry. +`ctx.mcpServers.list()` answers a management surface with one row per configured server: transport, durable intent, deletability, and the live status (`connecting`, `connected` with its tool count, `reconnecting` with the attempt budget, `failed` with the reason and last error, or `disabled`). Every transition also emits the `mcp-manager/status` event. Lifecycle operations do not wait for listeners; synchronous throws and returned promise rejections are logged without skipping other listeners. Reconnection itself stays inside mcp-client (backoff from 500 ms to 30 s, ten attempts per outage); `restart(name)` is the manual recovery beyond it — a fresh instance with a fresh budget, which is the only way back from a `failed` terminal state short of a config change. `probe(name)` connects, lists tools, and disconnects without registering anything — the "test this server" affordance, and it works on a disabled entry. ----- diff --git a/packages/mcp/mcp-manager/README.zh.md b/packages/mcp/mcp-manager/README.zh.md index 18f6329bae..241c449462 100644 --- a/packages/mcp/mcp-manager/README.zh.md +++ b/packages/mcp/mcp-manager/README.zh.md @@ -81,7 +81,7 @@ Settings 写入经 ConfigEditor 持久化并检查 revision；Loader 更新 vola ### 观察与恢复 -`ctx.mcpServers.list()` 给管理界面每台已配置服务器一行：传输、持久意图、可删性与实时状态（`connecting`、带工具数的 `connected`、带尝试预算的 `reconnecting`、带原因与最近错误的 `failed`，或 `disabled`）。每次状态迁移同时发出 `mcp-manager/status` 事件。重连本身留在 mcp-client 内（500 毫秒到 30 秒退避、每次断线十次尝试）；`restart(name)` 是其外的手动恢复——全新实例带全新预算，也是不改配置时走出 `failed` 终态的唯一途径。`probe(name)` 连接、列工具、断开而不注册任何东西——即「测试该服务器」，对已停用条目同样可用。 +`ctx.mcpServers.list()` 给管理界面每台已配置服务器一行：传输、持久意图、可删性与实时状态（`connecting`、带工具数的 `connected`、带尝试预算的 `reconnecting`、带原因与最近错误的 `failed`，或 `disabled`）。每次状态迁移同时发出 `mcp-manager/status` 事件。生命周期操作不等待监听器完成；监听器同步抛错或返回的 Promise 拒绝都会被记录，其他监听器仍会收到通知。重连本身留在 mcp-client 内（500 毫秒到 30 秒退避、每次断线十次尝试）；`restart(name)` 是其外的手动恢复——全新实例带全新预算，也是不改配置时走出 `failed` 终态的唯一途径。`probe(name)` 连接、列工具、断开而不注册任何东西——即「测试该服务器」，对已停用条目同样可用。 ----- diff --git a/packages/mcp/mcp-manager/src/index.ts b/packages/mcp/mcp-manager/src/index.ts index d03dede6f6..1f61f2663f 100644 --- a/packages/mcp/mcp-manager/src/index.ts +++ b/packages/mcp/mcp-manager/src/index.ts @@ -461,16 +461,10 @@ export default class McpServerManager extends Service { const previous = this.statuses.get(name) if (previous !== undefined && deepEqualJson(previous, status)) return this.statuses.set(name, status) - // A consumer's throwing listener must not break the lifecycle operation - // publishing the status (docs/defensive-patterns.md: contain callback - // exceptions). Invariant companions report through their own reporter, - // not by throwing here, so containment costs them nothing. - try { - this.ctx.emit('mcp-manager/status', name, status) - } catch (error) { + void this.ctx.parallel('mcp-manager/status', name, status).catch((error: unknown) => { this.ctx.logger.error(`mcp-manager: a mcp-manager/status listener failed for "${name}"`) this.ctx.logger.error(error) - } + }) } /** Template lookups: the process environment and the optional credential seam. */ diff --git a/packages/mcp/mcp-manager/src/types.ts b/packages/mcp/mcp-manager/src/types.ts index b52c636dd2..fd7a5b1db0 100644 --- a/packages/mcp/mcp-manager/src/types.ts +++ b/packages/mcp/mcp-manager/src/types.ts @@ -48,10 +48,12 @@ declare module '@deepseek-ai/cordis' { * from the instance, `disabled` when the entry is configured off, and the * manager-side `failed` reasons. Not emitted for an entry removed from * configuration; observers learn removals from plugin configuration. + * Lifecycle operations do not wait for listeners. Synchronous throws and + * returned promise rejections are logged without skipping other listeners. * @param name - the `servers` entry key. * @param status - the status now current for that server. - * @mode emit + * @mode parallel */ - 'mcp-manager/status'(name: string, status: McpManagedServerStatus): void + 'mcp-manager/status'(name: string, status: McpManagedServerStatus): void | Promise } } diff --git a/packages/mcp/mcp-manager/tests/manager.spec.ts b/packages/mcp/mcp-manager/tests/manager.spec.ts index 34db56c7f8..b728db6493 100644 --- a/packages/mcp/mcp-manager/tests/manager.spec.ts +++ b/packages/mcp/mcp-manager/tests/manager.spec.ts @@ -395,19 +395,28 @@ describe('mcp-manager', () => { await expect(manager.probe('ghost')).rejects.toThrow(/unknown server "ghost"/) }) - it('contains a throwing status listener so the lifecycle operation completes', async () => { + it.each(['throws', 'rejects'] as const)('notifies later status listeners and completes the lifecycle when a listener %s', async (failureMode) => { const { ctx, manager, settings } = await mountWorld({ settingsDoc: { mcp: { servers: { srv: stdioEntry() } } }, }) await waitForState(manager, 'srv', 'connected') - const errors: string[] = [] - ctx.logger.error = ((message: unknown) => { errors.push(String(message)) }) as typeof ctx.logger.error + const failure = new Error('listener bomb') + const logError = vi.spyOn(ctx.logger, 'error').mockImplementation(() => {}) + onTestFinished(() => { logError.mockRestore() }) ctx.on('mcp-manager/status', () => { - throw new Error('listener bomb') + if (failureMode === 'throws') throw failure + return Promise.reject(failure) }) + const laterEvents: Array<{ name: string; status: McpManagedServerStatus }> = [] + ctx.on('mcp-manager/status', (name, status) => { laterEvents.push({ name, status }) }) await settings!.update('mcp', { servers: { srv: { enabled: false } } }) await waitForState(manager, 'srv', 'disabled') - expect(errors.some(line => line.includes('mcp-manager/status listener failed'))).toBe(true) + expect(ctx.tools.get('mcp__srv__remote')).toBeUndefined() + expect(laterEvents).toContainEqual({ name: 'srv', status: { state: 'disabled' } }) + await vi.waitFor(() => { + expect(logError).toHaveBeenCalledWith('mcp-manager: a mcp-manager/status listener failed for "srv"') + expect(logError).toHaveBeenCalledWith(expect.objectContaining({ errors: [failure] })) + }) }) it('unwinds every instance and its tools when the manager is disposed, then refuses operations', async () => {