Replies: 9 comments
|
Confirmed, and the root-cause section is exactly right. I have this fixed locally with a test that fails without it. One warning first, because I think the currently favoured remedy is a trap. Suggested fix 2 (replace on duplicate) is worse than the throwThe replacing shim the community plugin ships has the same defect. Replacement satisfies the coexistence case and stays broken on disposal ordering:
A is still mounted, still believes it registered four providers, and now has none. The registrations are interchangeable, which is what makes sharing safeWorth stating explicitly, because it is the premise the fix rests on and it is not obvious:
So no provider's answer depends on which preset won the race to register it. That is what makes sharing correct rather than merely convenient. The shape that worksKeep one entry per id and a set of live holders; remove the entry when the last holder disposes: const existing = this.providers.get(manifest.id)
if (existing !== undefined && canonicalJson(existing.manifest) !== canonicalJson(manifest)) {
throw new Error(`Host Cordis inspect provider "${manifest.id}" is already registered`)
}
if (existing === undefined) this.providers.set(manifest.id, { ...registration, manifest })
const holders = this.holders.get(manifest.id) ?? new Set<object>()
const token = {}
holders.add(token)
this.holders.set(manifest.id, holders)
return () => {
const live = this.holders.get(manifest.id)
if (live === undefined || !live.delete(token)) return
if (live.size === 0) { this.holders.delete(manifest.id); this.providers.delete(manifest.id) }
}Two details that are load-bearing:
Against the pre-fix registry, five of six cases fail; the sixth is the genuine-collision throw, which passes both ways by design. The ones that matter are "one holder disposes while another is still mounted" and "a repeated disposer does not drop a live holder" — a replacing or refcounting registry passes the naive coexistence test and fails those two. On the other two suggestionsSuggestion 1 (per-session runner) is still the real answer for the wider class, and nothing here forecloses it — this only removes the reason a second preset cannot mount today. Suggestion 3 is worth doing regardless, and independently of this bug. Silently falling back to the default preset is what turned a one-line mount error into a reproduction hunt; a mount failure that reaches the preset picker would have made this self-diagnosing. |
|
This reproduction exposes two ownership requirements that are easy to collapse into “make registration idempotent.”
That also covers out-of-order disposal: mount A, mount equivalent B, dispose B, and A must still be able to list/query all four providers. I turned this into a regression matrix and operator runbook, including the separate stale-generation path that reaches the same error: https://sandbaseai.github.io/deepseek-harness-handbook/preset-generation-recovery.html Disclosure: I maintain the SandBase community handbook. |
|
你和 @nokkies、@denial123789 已经把这条挖得很深了(尤其 @nokkies 那条"replace-on-duplicate 是个陷阱"——A 挂着、B 替换、B 卸载时按'还是我的就删'把 A 的也删了,于是一个活着的 preset 的 补一件:同一个缺陷在另一条路径上已经被独立报过一次,而且那边的用户可见形态糟糕得多。 #902:同一个注册表,另一个触发器那位报的是运行中热重载:改
他自己定位到的根因和你这条是同一件事:
后端返回的错误一字不差就是你贴的那句: 两份报告合起来说明的是一件事,但角度互补:
最后一行是我想强调的:#902 那边后端返回了完整的 对你这条的意义:修复这个注册表的同时,那个被吞掉的报错也该单独提。否则即使 provider 冲突修好了,下一个 一个可能有用的角度:这两条其实指向同一个更上游的选择你写的"standing mount 是 process-lifetime,一旦创建就永不 dispose",和 #902 找到的那条 TODO("等最后一个挂在旧代际上的 agent 消失后再回收"),说的是同一个尚未做出的决定:preset 的挂载什么时候结束?
我觉得这个提法对推动有帮助:把"tool-cordis 的注册冲突"归到"preset 挂载生命周期未定义"之下,比作为一个独立 bug 更容易得到一个不会再复发的答案。(否则很可能修完 inspect registry,下一个往进程级注册表写东西的包再撞一次。) 一条通用教训,值得写进结论@nokkies 那个 disposal ordering 的陷阱,本质是:用"替换"来解决"重复",会把一个响亮的失败换成一个静默的失败。 这个交换在这个社区里已经出现过好几次,而且每次都是坏交易——最近一例是 #1697:那边的 你这条的正解形状可能是一样的:让等价的注册共享一个条目(@denial123789 的 canonical manifest),但manifest 不同的同 ID 注册仍然响亮失败——他也正是这么说的。这个对照值得引一下,它能帮评审快速理解为什么"幂等/替换"不够。 边界与利益相关我们不修 DSH 自家组件—— 利益相关:我维护 pi2dsh(Pi 生态兼容层)。这条不推销——但有一句相关的:我们整套挂载都刻意走 agent-local 的 |
|
@weijiafu14 the #902 link is the important addition, and I verified the mechanism against the source rather than taking it on trust. It holds exactly as described: // TODO: reclaim the superseded generation once the last agent joined to
// it is gone. The subtree is not inert — `dsh-skill-filesystem` watches its
// roots — and the settings-page authoring flow turns "a composition
// changed" into a per-save event.
if (this.standing.get(preset.id) === pending) this.standing.delete(preset.id)
return this.ensureStanding(preset)The pointer is dropped and the next generation is mounted; the superseded fiber is never disposed, so its Which means I should flag something against my own fixHolder-set sharing makes #902's error disappear without fixing #902. Once equivalent registrations share one entry, the new generation joins the leaked generation's entry instead of colliding with it, and the mount succeeds. The leak is still there — the superseded subtree still holds its watchers, exactly as that TODO describes. I think that is still the right trade, but it should be a decision rather than a side effect, so here is the argument both ways. Why removing it is still right: the collision is a terrible leak detector. It only fires when a second cordis-based preset exists, so the common single-preset case leaks silently today and reports nothing. When it does fire, it does not say "a generation leaked" — it says a preset failed to mount, and blocks the user. A signal that is absent in the usual case, misleading when present, and user-blocking either way is not one worth preserving for its diagnostic value. What is lost: today, on a two-preset machine, a leaked generation eventually announces itself. After the fix it will not. So the generation leak needs to be tracked on its own merits — the joined-agent count the TODO describes — rather than left to be noticed by a symptom that no longer occurs. That is worth saying plainly because "the error stopped happening" will be the observable outcome for anyone on #902, and it would be easy to read that as the leak being fixed. It is not. On the discarded errorYour last point is the one I would act on first, and it is the same thing as suggestion 3 on this thread. The backend produced a complete Both of the causes on these two threads are worth fixing. But surfacing the error is worth more than either, since it is the thing that makes the next unknown one cheap. @denial123789 your two ownership requirements match what I landed — canonical key-order-independent manifest comparison, and an opaque holder token rather than a numeric refcount precisely because a twice-run disposer must be harmless. Agreed on the out-of-order disposal case being the one that separates a correct fix from a plausible one. |
|
Excellent root-cause analysis. Your trace — a process-wide singleton It is the same root-cause class as the subagent disposal orphan we reported in #4793 — that missing-lifecycle-handoff-contracts angle is exactly what we'd been digging into on our end. The fix direction implied by your analysis — a per-session / per-mount registry scope instead of a process-global singleton — is the correct one; the real defect is where the ownership boundary is drawn, not the registration call itself. Glad to see this independently confirmed. It strengthens the case that these are not isolated bugs but a systemic scope-selection problem in how DSH manages cross-module state. Verified on 中文版非常扎实的根因分析。你追踪到的 —— 它和我们报告的子代理 dispose 孤儿(#4793)属于同一类根因,而那个「生命周期交接契约缺失」的角度,正是我们之前一直在抠的。你分析里暗示的修复方向 —— 把注册表作用域从「进程全局单例」改成「per-session / per-mount」 —— 是对的;真正的缺陷在于所有权边界划在哪里,而不在注册调用本身。 很高兴看到这一点被独立证实。它强化了我们的判断:这些不是孤立 bug,而是 DSH 在管理跨模块状态时一个系统性的作用域选择问题。 验证环境: |
|
A branch carrying a fix for this is available, based directly on https://github.com/nokkies/dsh-upstream-patches/tree/fix/cordis-inspect-provider-sharing It lets two co-resident hosts share an inspect provider id instead of the second registration throwing. Identical manifests share one holder set, and a genuine mismatch still fails loud. Offered as-is, no attribution wanted. Take, adapt, or ignore it freely. |
|
Adding cross-version confirmation, a one-line host-side repro, exact code locations, and a suggested fix. Still reproducible on 0.1.2-alpha.1The environment in the original report was Trigger is unchanged: a user preset copied from the shipped Worth noting for scope: of the 8 presets on this machine, only the two containing the One-line host-side reproNo second session and no UI needed — from a process where one await ctx.agentPresets.standingKeyFor('cordis')
// → throws: Host Cordis inspect provider "Service" is already registeredThe same call on any preset without that row returns normally. This isolates the failure to the second preset's standing mount (a Exact locations
The row registers four first-party ids — Suggested fixThe provider has no scope dimension in its query surface ( for (const provider of hostInspectProviders(ctx))
ctx.effect(() => {
try {
return ctx.cordisInspect.register(provider);
} catch (error) {
// Another preset already registered this process-global provider:
// it describes process state, so reuse it and do not remove it.
if (!String(error?.message ?? "").includes("already registered")) throw error;
return () => {};
}
}, `tool-cordis: inspect ${provider.manifest.id}`);Only the duplicate-registration case is absorbed; every other error still propagates, and the first registrant's behavior is unchanged. Trade-off: the provider now lives until process exit — acceptable here, since "this preset unloaded, so those Services no longer exist" is not a meaningful state. A reference count inside Longer term, this looks like a plane-placement issue: the registration writes into a process-global registry, so it arguably belongs in the host composition rather than being a side effect of a preset row — the preset would then only contribute the Minor, but worth fixing either wayThe current error text ( I have this patch applied locally and can confirm it parses and is idempotent; I have not yet verified the runtime behavior across a host restart, so treat the fix as a proposal rather than a tested result. |
Postable commentConfirming this on Same root cause as #1415 / #1827 / #2035, so closing this as a duplicate is fine — but two things below are not covered by those reports and both matter for how easy this is to hit. Environment
SymptomClicking New Session does nothing at all — no toast, no error, no new session directory on disk. The Network tab shows the create request completing with HTTP 200 (so this is not an auth or transport failure; the host accepts the RPC and then rejects the composition). Browser console: The failure reaches the user only as a // @deepseek-ai/dsh-client-ui-workspace/lib/client.js:93-95
this.openWorkspace(target).catch((reason) => {
console.warn("new session failed:", reason);
});There is a second silent no-op path in the same handler: when Trigger path — the documented preset-copy flow is enoughThis does not require a user to deliberately run two cordis toolkits. It is what happens after following the guidance printed in the shipped
So a single copy-and-rename turns New Session into a dead button, with no user-visible error. Second defect:
|
| File | Relevance |
|---|---|
dsh-tool-cordis/lib/index.js:9095-9113 |
inject, apply, unconditional ctx.cordisInspect.register(...) for 4 providers |
dsh-cordis-host-runner/lib/index.js:716-741 |
process-global cordisInspect registry; throws on duplicate id |
dsh-agent-presets/lib/index.js:1767-1803 |
ensureStanding re-mount without disposing the previous scope |
dsh-agent-presets/lib/index.js:905-938 |
mountPreset, leakedServices guard (publish-only) |
dsh-client-ui-workspace/lib/client.js:81-96 |
startSession; swallowed rejection and silent sessions.clear() |
|
核实结论:报告机制在 master(c291e79)逐点命中,「process-global」论断至今成立;冲突是「preset 挂载生命周期未定义」的必然结果,不是 tool-cordis 的疏忽.
|
Uh oh!
There was an error while loading. Please reload this page.
tool-cordis inspect providers are process-global: two cordis-based presets cannot coexist in one process
Environment
@deepseek-ai/dsh0.1.1-rc.2,webprofile (dsh webvia launchd)Summary
Any two agent presets that both mount
tool-cordis(i.e. a user preset copied from the shippedcordispreset via the officialcopy()authoring path, plus the built-incordispreset) cannot both mount in the same process. The second one fails with:Only a process restart resolves it, and only if the other cordis-based preset's standing mount is never triggered first.
Steps to reproduce
cordis(the "创造模式" / creator-mode preset) through the Agent Preset settings page (agentPresets.copy).tool-cordisin the copied preset (it is copied verbatim).dsh web.cordispreset first (e.g. create a session with it, or merely select it on a blank session), then try to create/select a session with the copied preset.Host Cordis inspect provider "Service" is already registered. Swapping the order flips which preset fails.Root cause
tool-cordisregisters four Host inspect providers —Service,Event,Builtin,Tool— unconditionally at mount time (inapply:for (const provider of hostInspectProviders(ctx)) ctx.effect(() => ctx.cordisInspect.register(provider), ...)).CordisInspectRegistryService.register()(in@deepseek-ai/dsh-cordis-host-runner) keeps a process-wide singletonprovidersMap and throwsHost Cordis inspect provider "<id>" is already registeredon any duplicate id.standingKeyFor), it is never disposed while the process lives (ensureStanding;recomposeonly re-binds, it does not unmount the previous preset).Impact
copy()from a shipped preset) silently produces presets that are mutually exclusive with the shippedcordispreset. The UI gives no hint: the preset merely shows as failing to select / the session falls back to the default preset, while the real error is only visible in the backend.Community confirmation
The same issue is independently documented by a third-party preset plugin, KannaKuron/dsh-ptc-cordis-preset:
That plugin ships a shim that monkey-patches the registration to replace duplicates (same-manifest), which confirms the pain is real and widespread.
Suggested fixes (any one)
cordisInspectregistry (or at least the host-inspect providers) per preset scope instead of process-global, so multiple cordis-based presets can coexist. (This is the "根治" the community plugin author recommends.)Workaround (current)
dsh web, then create/select the desired cordis-based preset before any other cordis-based preset's standing mount is triggered in the process.All reactions