Repository navigation
fix(mcp): recover failed pooled connections without replaying calls - #11392
destire-mio wants to merge 12 commits into
Conversation
doudouOUC
left a comment
There was a problem hiding this comment.
Reviewed at ea4c678. I spent most of this review trying to break the security-relevant claims in the design and could not — the parts I was most worried about are handled correctly, and I want to record that explicitly since it is the useful output here.
What I verified holds:
- A tool that carries the bootstrap
Configcannot spawn a private client.mcp-tool.tsfails closed on!this.reconnectOnErrorfor connection errors — it throws the shared-connection message so the model learns the outcome is unknown, andshouldAttemptReconnectreturnsfalsebefore any client work.reconnectOnErroris genuinely populated, not a dead default:session-mcp-view.tspassesfalseexplicitly when cloning a tool for a session. - Guarded invocations fail closed on reconnect, with the right reasoning in place — one authorization must not become several execution attempts when a transport error leaves the side effect ambiguous.
- The per-session tool clone does not bake in the first subscriber's trust or eager-load choice;
withSessionConfigtakes them per session. - The recovery cooldown does not have the "an earlier concurrent success clears a later failure" bug, and the expiry timer checks identity before deleting, so an older failure cannot clear a newer cooldown for the same key.
Two findings, neither Critical.
Route ownership — a process-global read drives a live-session-owner decision. isConnectionError treats getAllMCPServerStatuses().get(this.serverName) === DISCONNECTED as proof that this session's error is a connection error. That status map is keyed by server name only and is process-global, so in the daemon, one session's server going DISCONNECTED will make an unrelated error in another session's tool for a same-named server get reported to the model as "connection error, outcome unknown." Bounded and self-correcting, but it is the wrong scope for the decision — this should consult the status of the connection this tool actually holds.
Cross-session coupling in the recovery cooldown. recoveryRetryAfter is keyed by connectionIdOf(serverName, cfg), which is process-global rather than session-scoped, so a session with a locally broken environment puts every other session's recovery for that config into a 5s cooldown. Mitigated by the fact that explicit discovery/restart goes through the normal acquire path and bypasses the cooldown, which the code notes — so users have an escape hatch. Worth stating the intended scope in the design doc either way.
The main thing I want to raise is scope, and it is a maintainer call rather than a review finding.
This is roughly +1100/−270 production lines across packages/core/src/tools/**, packages/core/src/agents/**, packages/core/src/subagents/**, and packages/cli/src/acp-integration/** — core infrastructure, cross-package, and it introduces two new modules (mcp-pool-entry.ts, mcp-workspace-budget.ts) alongside the recovery change itself. Under our two-tier gate a fix of this size is not size-blocked, but it does escalate for awareness, and I cannot claim to name every downstream consumer of the pool and registry surfaces being changed here with 100% confidence. So: escalating to maintainer review, with the note that the security-critical paths check out.
Concretely, I think this would be easier to land as two changes — the pooling/budget infrastructure first, then the recovery-without-replay behavior on top. Against Simplicity First, a single PR that both restructures pool ownership and changes recovery semantics is hard to review as one unit, and the test additions (+3800 lines) suggest the same.
The design doc is present in both English and Chinese, which is appreciated.
CI: the Test (ubuntu-latest, Node 22.x) lane is failing on this SHA and the branch is BLOCKED. Please get that green before the next round — I did not attempt to attribute the failure, since I could not run tests locally (the worktree lacks node_modules/packages/core/dist, and the vitest globalSetup guard blocks runs until npm run build from the repo root). Everything above is read-verified against ea4c678.
doudouOUC
left a comment
There was a problem hiding this comment.
Agent-assisted review at fb125a154b51b227371bdcb49c916bcb1602f9d0 — Partial review — coverage gaps; no additional Critical independently confirmed. This is not a C=0/all-clear conclusion.
Current-head reconciliation
- R3-12 / R5-1:
mcp-client-manager.ts:3489-3504rolls back the tentative overlay by object identity;canRollback()now gates only restoration of recovery authority. The same-fingerprint path preserves an active entry during restart. Current lifecycle tests cover failed replacement plus remove/disconnect and queued-add invalidation; the old stale-overlay report should not be repeated against this implementation. - R4-2:
mcp-tool.ts:563-575reads the owning McpClient status callback before legacy name-only fallback, while separately detecting a closed SDK transport. Discovery supplies that callback and projections/clones/retry construction preserve it. This addresses both cross-connection contamination and the later onerror-with-transport-present regression. - R4-3: raw recipe lookup via
getMcpServers(false)preserves extension object identity through Config and subagent overrides; independent overrides still compare effective transport/session metadata. - R5-2: ACP now synchronizes tools and commands on every pooled pre-send, even with no diagnostic notice. R5-5's definitely-not-sent direct-client case now rejects before
callTool(mcp-tool.ts:624-636); pooled failures never enter standalone reconnect/replay. JSON-RPC application errors remain distinct from transport loss. - R5-4: cleanup is published before terminal notifications, and acquire has a five-second caller deadline (
mcp-transport-pool.ts:284-305). Timeout intentionally retains the barrier; it is not evidence the old process exited. The previous unbounded-caller-wait mechanism is changed, not a reason to permit overlapping replacement processes. Selected tests verify no spawn before cleanup, timeout, and later recovery. - R2-2's stale-handle mechanism is addressed by expected-handle release plus registry seats; parent/child registries sharing a logical session have separate seats. R3-2's empty refusal event is guarded by pending refusal count. The Chinese design now includes the previously omitted qualifications. Shared cooldown coupling remains explicitly documented; deferred suggestions are not reopened.
Coverage: reviewed the current production changes across invocation, client close/discovery, pool entry/transport lifetime, manager recovery/runtime serialization, registry inheritance/resource ownership, budget, Config/subagents, agent pre-send, and ACP synchronization; sampled current lifecycle/cleanup tests. Existing selected-workspace routes use the selected runtime bridge; session mutations resolve the live session's Config, while legacy workspace fan-out remains a separate existing surface. No new management route is added.
Not fully cleared: the full historical thread inventory was fetched (108 threads, no nested pagination remaining), but earlier R1/R2/R3 discovery/exclusion/resource interleavings and R4-9's budget test-versus-real-pool claim were not exhaustively re-adjudicated. Nor were all changed tests, every existing daemon dispatch/auth consumer, native transport late-close/restart races, all callable-only paths, real OAuth, or Linux/Windows process cleanup. Static safeguards are not an end-to-end security or liveness proof. The roughly 1,544 changed production-source lines warrant large-fix advisory review, not the external-refactor hard block.
Validation: static pinned-source/dataflow and selected-test inspection only; no tests, builds, daemon runs, or PR code executed. Comment only; no approval implied.
|
@qwen-code /resolve |
Both this branch and main inserted new Session fields at the same anchor after `private turn`, so the declaration block conflicted. The two sets are independent: the recovery notice fields gate per-turn MCP recovery diagnostics, and main's nested-sequence counter names code-mode sub-calls. Keep both rather than choosing a side.
|
Qwen Code resolved the merge conflicts and pushed the branch update. PR #11392 — conflict resolutionMerged Root causemain's #10607 ( Textual, not semanticAdjacent only; neither side reads the other's field. Union: private turn: number = 0;
private mcpRecoveryNoticeTurn?: string;
private readonly mcpRecoveryNotices = new Map<string, string>();
private codeModeNestedSequence = 0;Both are live ( Load-bearing
Not verifiedNo build, typecheck, or tests run.
中文说明仅 根因:main 的 #10607( 纯文本冲突,非语义冲突:两侧仅相邻,互不读写对方字段,故取并集。两字段均在用( 关键约束:三字段须保留声明;顺序不重要(无相互依赖),勿以为位置有意义而重排。另两处 git 静默产出、易被后续修改破坏的顺序: 未能验证:未跑构建、类型检查或测试。CodeModeOnly 与连接池恢复的组合两侧均无覆盖:#10607 令 |
|
Splitting this per the review guidance (thank you @doudouOUC for the detailed scope analysis).
Closing the combined PR so each piece can be reviewed on its own. The reproduction/design notes remain in the thread and in the new bilingual design note under docs/design/. |
Infrastructure half of QwenLM#11392, split out per review guidance. Covers the pool ownership and teardown paths that demand recovery will build on. - PoolEntry: publish a cleanup barrier, expose waitForCleanup(), and scope detach/release to a handle identity so a superseded handle cannot detach its replacement. - McpTransportPool: index attachments by (logical session, registry seat) so one session's registries release independently; track retiring entries and wait, under a deadline covering the teardown budget, for in-flight cleanup before a new process is spawned for the same fingerprint. - McpClient: reflect an unexpected SDK close in a pool-managed client's status, retire the previous transport on disconnect, and bound transport.close()/client.close() so the cleanup barrier always settles. Behaviour change: a pool-managed client now reports DISCONNECTED on an unexpected transport close, which evicts the entry and removes the session's MCP tools. Before, an EOF-killed server was invisible. This is the detection half of QwenLM#11272; re-acquisition is a follow-up. No recovery trigger, retry cooldown, budget refusal accounting, or raw-recipe access is included; those land with the follow-up that calls them.
Bilingual design note for the infrastructure half of QwenLM#11392: the pool ownership and teardown invariants, the unexpected-close behaviour change, risks, and validation plan. English and Simplified Chinese are aligned.
What this PR does
Restores previously acquired MCP connections and session tool, prompt and resource registrations before later model sends or resource reads in daemon/ACP sessions and their agents. Recovery uses the existing workspace pool, current permissions and normal authentication. It performs no tool invocation or replay.
Sessions retain their subscriptions while a shared connection is restarting. Discovery, recovery and runtime additions coordinate within the session; the pool coordinates process creation and old-connection cleanup across sessions. Removal invalidates an older queued addition before it writes configuration, and queued discovery rechecks folder trust when it starts. Failed runtime replacements roll back their own tentative in-memory configuration without restoring connection authority revoked by disconnect, removal or shutdown.
Pooled ACP sessions synchronize model tool declarations and prompt commands before each model send, including recovery that produces no diagnostic. Inherited agent tools retain their source, while independent overrides retain their own connection and resource ownership. Error handling reads the owning connection: a missing transport is rejected before sending, a returned JSON-RPC business error keeps its meaning, and a lost response retains an unknown-outcome/no-replay warning.
Why it's needed
A tool invocation, its process/transport and a session's pool reference have separate lifetimes. Cancelling one invocation can leave the shared process healthy. When the transport does fail, removing the pool entry previously left active sessions without a replacement path. Recovery must restore connection ownership and declarations without treating an interrupted operation as safe to repeat.
The counted local fixture establishes these recovery gaps, not the original reporter's cancellation mechanism. Ordinary cancellation left the fixture process alive; exit and protocol corruption were controlled faults. The runtime configuration rollback concerns in-memory state, not disk settings. A transport object can remain assigned after a network error, and an active pool entry can temporarily have a disconnected client during restart; neither state alone determines an invocation's outcome.
Reviewer Test Plan
How to verify
Evidence (Before & After)
The repair is committed as
fb125a154b51b227371bdcb49c916bcb1602f9d0, based on reviewed head82b635b0e58044a8345ccdf62447469e765559be, including the maintainer's main merge. The tested CLI entrypoint SHA256 is1f6de4cb4679d4c47add276e433feeea657616d9d33c0d351bff720419e02ef9. Its five-file production delta from the reviewed head has SHA2567cc812236bc68f4e0967b1a91cda669f7801764d8d64634adcd8566d5051085b.The full workspace baseline build, repaired core/CLI builds, bundle, repository-wide typecheck, final core typecheck, changed-file ESLint/Prettier and diff checks pass on Node 22.22.3. The CLI compiler used a 6 GiB heap. Two final source self-review passes and two independent review passes found no additional actionable defect within the examined ownership and consumer paths. These are bounded review results, separate from hosted CI and maintainer acceptance.
The post-verification built-output manifest covers 1,061 files (118,342,322 bytes), with manifest SHA256
e6e8eba23b352510b1aa1261a97407698687c18e98bf218526e7d2d28ae8fe41; the entrypoint hash above is not a substitute for the full output tree. No rebuild occurred between daemon verification and this snapshot.The manager approval-revocation producer and the next-send ACP declaration consumer were verified separately; this is not a single end-to-end revocation experiment. Never-settling cleanup was tested by fault injection. With the installed SDK 2.0.0, a real stdio child ignoring EOF and SIGTERM exited in about four seconds; a permanent built-in-transport cleanup hang was not reproduced. Early invalid fixture parameters and a nonexistent test directory were corrected and are not counted as production failures. The bot's previously inconclusive whole-diff test-efficacy probe is not claimed as completed by these targeted checks.
Tested on
Environment
Loopback daemon, WebSocket ACP, deterministic local model endpoint, counted stdio and random-port HTTP fixtures, isolated test homes and runtime directories. Raw request, cancellation, process and pool-state evidence is retained locally; results above use wire counts and actual tool outcomes, not model prose or end_turn.
Risk & Scope
Design documents: English · 中文. Both versions cover the same decisions, constraints, validation scenarios and evidence boundaries.
Linked Issues
Related to #11272. This implements connection recovery, not the proposed in-chat named restart command.
中文说明
本 PR 的改动
在 daemon/ACP 会话及其 Agent 的后续模型发送或资源读取前,恢复曾取得的 MCP 连接与会话工具、提示词和资源注册。恢复使用现有工作区连接池、当前权限和原有认证流程,不执行工具调用、不重放旧调用。
共享连接重启期间保留会话订阅。发现、恢复和运行时添加在会话内协调;连接池协调跨会话进程创建与旧连接清理。移除会使尚未写入配置的旧排队添加失效,排队发现开始时复查目录信任。运行时替换失败回滚本次临时内存配置,但不恢复被断开、移除或停止撤销的连接权限。
每次池化 ACP 模型发送前同步模型工具声明和提示词命令,包括恢复没有诊断提示的情况。继承工具保持来源归属,独立覆盖保留自己的连接和资源。错误处理读取所属连接:transport 缺失时在发送前拒绝,收到的 JSON-RPC 业务错误保持原义,响应丢失保留结果未知与没有重放的提示。
为什么需要
工具调用、进程/传输、会话池引用具有不同生命周期。取消一个调用可以保留健康共享进程。传输失效时,旧实现清理池条目却没有给活跃会话提供获取替代连接的路径。恢复需要重建连接归属和声明,不能推断被打断的操作可以重复执行。
本地计数服务证明这些恢复缺口,不能证明原报告中的取消机制。普通取消保留了服务进程;退出和协议损坏是受控故障。运行时配置回滚处理内存状态,不修改磁盘设置。网络错误后 transport 对象可能存在;重启中的 active 池条目也可能暂时持有断开的客户端,两种状态不能独自决定调用结果。
审查者测试计划
如何验证
前后证据
修复提交为
fb125a154b51b227371bdcb49c916bcb1602f9d0,基于评审 head82b635b0e58044a8345ccdf62447469e765559be,包含维护者的 main 合并。被测 CLI 入口文件 SHA256 为1f6de4cb4679d4c47add276e433feeea657616d9d33c0d351bff720419e02ef9。相对评审 head 的五文件生产差异 SHA256 为7cc812236bc68f4e0967b1a91cda669f7801764d8d64634adcd8566d5051085b。Node 22.22.3 下,基线全工作区构建、修后 core/CLI 构建、bundle、全仓类型检查、最终 core 类型检查、变更文件 ESLint/Prettier 和 diff 检查通过。CLI 编译使用 6 GiB 堆。最终两轮源码自审和两轮独立复审,在所检查的归属及消费路径内未发现额外可处理缺陷。该结论有范围限制,区别于托管 CI 和维护者接受状态。
验收后的构建产物清单包含 1,061 个文件(118,342,322 字节),清单 SHA256 为
e6e8eba23b352510b1aa1261a97407698687c18e98bf218526e7d2d28ae8fe41;上述入口文件指纹不代替完整产物目录。daemon 验证与该快照之间没有重新构建。审批撤销的 manager 生产端和下次发送的 ACP 声明消费端分开验证,不是一次完整的撤销端到端实验。永不完成的清理通过故障注入测试。已安装 SDK 2.0.0 的真实 stdio 进程忽略 EOF 和 SIGTERM 后约四秒退出,没有复现内置传输永久清理挂起。早期无效夹具参数与不存在的测试目录经过修正,不计入产品失败。此前 bot 未得出结论的全差异测试效能探针,不由本轮定向检查宣称完成。
测试平台
环境
本机 daemon、WebSocket ACP、确定性本地模型端点、计数 stdio 和随机端口 HTTP 夹具、隔离的测试用户目录和运行目录。原始请求、取消、进程及连接池状态证据保存在本地;上述结果依据 wire 计数和实际工具输出,不依据模型文字或 end_turn。
风险与范围
设计文档:English · 中文。两版覆盖相同决策、约束、验证场景与证据边界。
关联 Issue
关联 #11272。本改动实现连接恢复,不包含所提议的聊天内按名称重启命令。