Replies: 1 comment 1 reply
|
你把"清理没跑"和"作用域不匹配"分开这一点很关键,而且"把确定性错误变成静默失败"这个反对意见我完全同意——那种修法会让下一次故障更难查。 补一个跨帖的观察,可能对你说的"报告里没提到的设计层问题"有用。 「一个 MCP server = 一个插件实例」这个形状,正在制造一族问题
四个症状、一个共同前提:server 的身份、生命周期和故障域,都被绑定在"插件条目"这个粒度上。你分析的作用域不匹配,本质是"server 名字的注册作用域(模块级 / 如果这个观察成立,那么修 (存在性证明:把所有 server 交给单个 MCP 运行时管理的形态是可行的——我维护的桥上跑的 给被 #3984 卡住的人在修好之前:重启 dsh 能解开——那个 WeakMap 是模块级的,进程重启必然清空。不优雅,但能立刻恢复。 利益相关:我维护 pi2dsh, |
1 reply
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Uh oh!
There was an error while loading. Please reload this page.
Related bug report
This analysis is a follow-up to:
[Bug] mcp-client: activeServerNames 生命周期泄漏导致无法创建新会话 — #3984
EN / English
Purpose
This is a follow-up analysis to [#3984](#3984) — not a duplicate bug report. We verified the reported behavior against the engine source, and our reading diverges from the reporter's root-cause guess on two points, plus finds a design-level issue the report did not mention. We are sharing it openly because we believe the current proposed fix would turn a deterministic error into a silent failure.
The problem — recap
From #3984:
@deepseek-ai/dsh-mcp-clientregisters every configuredserverNamein a module-levelWeakMapkeyed byctx.root. After a session using a given MCP server is closed, creating a new session that uses the sameserverNamefails with:The reporter attributes this to the effect cleanup
() => void names.delete(config.serverName)"not being called" when the session closes, and proposes a defensive "delete-then-check" in the registration effect.Correction 1 — "cleanup never runs" is contradicted by the reporter's own repro
The reporter states the cleanup is never invoked, yet also notes: "waiting about 30 seconds before creating a new session succeeds (unstable)". If the cleanup never ran, the
Setentry would persist forever and 30 seconds would not help. The only consistent reading is that the cleanup eventually runs, but late and unreliably — the failure is a delayed teardown, not a missing one.This distinction matters: it moves the defect out of
dsh-mcp-clientinternals (where the cleanup exists and is synchronous) and points upstream — to when the preset plugin instance's fiber is disposed after session close.Correction 2 — mcp-client's own lifecycle is sound
In the version we verified (
0.1.1-rc.2, source matches the master snapshot cited in #3984):dsh-mcp-client/lib/index.js:768-775— readsctx.root, rejects duplicates (:773), registers the name, and returns a synchronous cleanup() => void names.delete(config.serverName)(:775).() => connection.dispose()(:779), andconnection.dispose()(:678-696) is a complete async chain: flush disposers → close the client → await the close signal → settle the connection → flush the sync chain.Nothing in mcp-client leaks by itself. The cleanup is synchronous and correctly paired; the open question is whether the upstream fiber that owns this effect is disposed promptly — and the ~30 s delay suggests a teardown timer (reconnect backoff / idle teardown) somewhere in that chain. We could not pin that exact timer by static reading; it needs a runtime timing repro (close a session, record when
serverNameis actually released).Design-level finding — the scope mismatch (the part the report missed)
The deeper issue is that the uniqueness constraint's semantic scope does not match its implementation scope:
activeServerNamesis keyed onctx.root(:731+:768) — and in Cordis,ctx.rootis the application-level context (cordis/lib/index.js:1681,this.root = self; fiber 0 is the root). The constraint is therefore process-global.dsh-toolsis built onScopedLayers/scopeOf/scopeTargetfromdsh-scope, and tool visibility is scoped to the agent context.So: two parallel sessions (or any two agents in the same process) that legitimately want to attach the same MCP server are rejected on the
serverNameuniqueness check even if teardown were instantaneous and perfect. The comment intent — "multiple apps in one process must not see each other's names" — is implemented as "unique per process", which is wider than the semantics the tool layer actually needs. The reporter's scenario (sequential sessions) is the symptom; the concurrent case is the structural restriction. Whether the per-process width is intentional (e.g. to prevent double-spawning the same stdio server) is a question only the maintainers can answer — but the mismatch between the uniqueness scope (ctx.root) and the visibility scope (per-agent) is a fact worth aligning or documenting.Critique of the proposed fixes
Fix 1 ("defensive delete-then-check") is unsafe and should not be adopted. In the sequential case it papers over the symptom; in the parallel case it actively creates corruption:
serverName "context7"(a legitimate concurrent topology today, since tool registration is per-agent).names.delete("context7")— the check then passes, and both instances register the same name.serverName, and their dispose/reconnect/teardown chains interfere with each other. A deterministic configuration error becomes a silent, harder-to-diagnose failure.Deleting another live instance's registration before checking is exactly the kind of "fix" that trades a loud bug for a quiet one.
Fix 2 ("make sure the disposal fires") is the right direction but underspecified. The delay has to be located first (which fiber owns the preset instance, and which timer delays its dispose) before it can be fixed. Without a runtime timing repro, "ensure disposal" has nothing concrete to attach to.
Suggested direction
ctx.rootto the agent scope where tools actually live — or, if per-process uniqueness is intentional (double-spawn protection), document it and make the error message say so, so the concurrent case is a stated constraint, not an accident.serverNameregister/release events, so teardown timing (and any future leak) is visible instead of discovered via "session create failed".Questions for maintainers
serverNameintentional (e.g. to avoid double-spawning the same stdio/streamable server), or should the scope be per-agent, matching the tool registry?ctx.rootto the agent scope, or a documented + reworded error for the concurrent case, as the first step?Environment
@deepseek-ai/dsh0.1.1-rc.2(source matches the master snapshot cited in #3984; line numbers above are from the rc.2 build).中文版 / ZH
目的
这是对 #3984 的后续分析帖——不是重复报 bug。我们对照引擎源码核验了报告行为,与举报者的根因猜测在两点上不一致,并发现报告未提及的一个设计层问题。公开分享,因为我们判断:现有建议方案会把"确定性报错"变成"静默故障"。
问题回顾
按 #3984:
@deepseek-ai/dsh-mcp-client把每个配置的serverName注册进一个以ctx.root为 key 的模块级WeakMap。使用某 MCP server 的会话关闭后,新建使用相同serverName的会话即失败:举报者将根因归于 effect 清理函数
() => void names.delete(config.serverName)在会话关闭时"未被调用",并建议在注册 effect 里做防御性"先删后查"。修正一——"清理从未执行"与举报者自己的复现数据矛盾
举报者称清理函数从未被调用,但他同时写道:"等待约 30 秒再创建新会话能成功(不稳定)"。如果清理从未运行,
Set里的条目会永久残留,等 30 秒不会有任何帮助。唯一自洽的解释是:清理最终执行了,但延迟、不稳定——这是延迟销毁,不是"从未销毁"。这个区分很关键:它把缺陷从
dsh-mcp-client内部(清理存在且是同步的)指向上游——会话关闭后,preset 插件实例所在的 fiber 何时被 dispose。修正二——mcp-client 自身的生命周期是健全的
在我们验证的版本(
0.1.1-rc.2,源码与 #3984 引用的 master 快照一致)中:dsh-mcp-client/lib/index.js:768-775——读取ctx.root、拒绝重复(:773)、注册名称、返回同步清理() => void names.delete(config.serverName)(:775)。() => connection.dispose()(:779),而connection.dispose()(:678-696)是完整的异步链:flush disposers → 关闭 client → 等待 close 信号 → settle 连接 → flush 同步链。mcp-client 自身没有泄漏点。清理是同步且正确配对的;真正的开放问题是持有该 effect 的上游 fiber 是否被及时 dispose——约 30 秒的延迟暗示销毁链某处有一个 teardown 定时器(reconnect backoff / idle teardown)。静态阅读无法定位该定时器的确切位置,需要运行时计时复现(关闭会话,记录
serverName实际释放的时刻)。设计层发现——作用域不匹配(报告遗漏的部分)
更深的问题是唯一性约束的语义范围与实现范围不一致:
activeServerNames以ctx.root为 key(:731+:768)——在 Cordis 中ctx.root是应用级 context(cordis/lib/index.js:1681,this.root = self;fiber 0 即 root)。因此约束是进程级全局的。dsh-tools基于dsh-scope的ScopedLayers/scopeOf/scopeTarget构建,工具可见性限定在 agent context 内。于是:并行的两个会话(或同进程任意两个 agent)想挂同一个 MCP server 时,即使销毁即时且完美,也会在
serverName唯一性检查处被拒绝。注释意图——"同一进程内多个 app 互不可见"——被实现成"每进程唯一",宽于工具层实际需要的语义。举报者的场景(串行会话)只是症状;并发场景才是结构性限制。per-process 宽度是否刻意(例如防止重复 spawn 同一 stdio server)只有维护者能回答——但"唯一性作用域(ctx.root)与可见性作用域(per-agent)不一致"是事实,值得对齐或至少文档化。对建议方案的批判
方案 1("防御性先删后查")不安全,不应采纳。 串行场景下它只是掩盖症状;并行场景下它会主动制造损坏:
serverName "context7"(今天这是合法并发拓扑——工具注册本就是 per-agent)。names.delete("context7")——检查随即通过,两个实例注册了同一个名字。serverName,其 dispose/reconnect/teardown 链互相干扰。一个确定性的配置错误变成静默、更难诊断的故障。在检查前删除另一个存活实例的注册,正是那种"把响亮的 bug 换成安静 bug"的修复。
方案 2("确保销毁被触发")方向正确但太泛。 必须先定位延迟点(preset 实例挂在哪条 fiber、哪个定时器延迟了它的 dispose),否则"确保销毁"没有可落地的抓手。没有运行时计时复现,"确保销毁"无从谈起。
建议方向
ctx.root收窄到工具真正所在的 agent 作用域;若每进程唯一是有意设计(防双 spawn),则文档化并让错误消息说明这一点——让并发场景成为声明的约束,而非意外。serverName注册/释放事件,让销毁时序(及未来任何泄漏)可见,而不是靠"session create failed"才发现。向维护者提出的问题
serverName的每进程唯一是否有意为之(例如避免重复 spawn 同一 stdio/streamable server)?还是应改为与工具注册表一致的 per-agent 作用域?ctx.root改为 agent 作用域,或对并发场景做文档化 + 措辞更明确的报错?环境
@deepseek-ai/dsh0.1.1-rc.2(源码与 [Bug] mcp-client: activeServerNames 生命周期泄漏导致无法创建新会话 #3984 引用的 master 快照一致;上文行号出自 rc.2 构建)。署名 / About this report
EN: Reported by the OfferKuai (Offer快) Team — an AI startup building full-lifecycle job-application services, guided by the belief that "users need results, not repeated conversations." Founder: Zhaofeng (Yaming). We use DeepSeek Harness as part of our daily development workflow; this report is our way of contributing back to the ecosystem. Website: https://www.offerkuai.com/ | Contact: <contact@offerkuai.com>
ZH: 本报告由 OfferKuai(Offer快)团队提交 —— 一家专注 AI 全流程求职托管的创业团队,核心理念是「用户要的是结果,不是重复对话」。创始人:Zhaofeng(Yaming)。我们将 DeepSeek Harness 用于日常开发工作流,这份报告是我们对社区的回馈。官网:https://www.offerkuai.com/ | 联系:<contact@offerkuai.com>
All reactions