fix(acp-bridge): make detachClient idempotent via per-clientId attach-ref ledger - #7386
Conversation
…-ref ledger detachClient unconditionally decremented attachCount regardless of whether the detach actually released an attach reference. Duplicate detaches, unknown or anonymous clientIds, and a spawn owner detaching with its own clientId all stole another attacher's count, letting spawnOwnerWantedKill or close-on-last-detach kill a session that still had live clients.
|
Thanks for the PR! Template looks good ✓ Problem: observed bug with a linked issue (#7385) and a clear source-level reproduction. Direction: aligned — this is a real correctness bug in the daemon's session lifecycle. Claude Code's CHANGELOG shows a similar class of fix ("pressing back in one window no longer detaches other windows attached to the same session"), so the problem pattern is well-established in this space. Size: not applicable — Approach: the scope feels right. A per-clientId refcount ledger ( Moving on to code review. 🔍 中文说明感谢贡献! 模板完整 ✓ 问题:已观测到的 bug,有关联 issue(#7385)和清晰的源码级复现。 方向:对齐——这是 daemon session 生命周期中的真实正确性 bug。Claude Code 的 CHANGELOG 中有类似修复("在一个窗口按返回不再 detach 其他 attach 到同一 session 的窗口"),说明这类问题在该领域是已知的。 规模:不适用—— 方案:范围合理。按 clientId 记账的引用账本( 进入代码审查 🔍 — Qwen Code · qwen3.7-max Reviewed at |
Code Review —
|
… a live attacher Mutation testing showed the original case also passed on the pre-fix code: with a single attacher the attachCount floor guard masked the double decrement. Add a second attacher so a stolen ref becomes observable through the owner's requireZeroAttaches kill.
Code ReviewIndependent proposal (before reading the diff): given the problem — Comparison with the diff: the PR does exactly this. The implementation matches my independent proposal closely:
No critical blockers found. No AGENTS.md violations. The v2 commit strengthens the duplicate-detach test by adding a third client C, making count theft directly detectable under a live attacher rather than relying on a fresh attacher to bump the count back. Test ResultsUnit tests (against Build: clean. Typecheck: clean. Daemon smoke test (tmux, Health check: 中文说明代码审查独立方案(未看 diff 前):针对 与 diff 对比:PR 的实现与我的独立方案高度一致:
未发现关键阻塞问题。无 AGENTS.md 违规。v2 提交通过引入第三个客户端 C 加强了重复 detach 测试,使计数偷窃在存活 attacher 下可直接检测。 测试结果单元测试(针对 Daemon 冒烟测试(tmux):daemon 正常启动并服务,健康检查返回 — Qwen Code · qwen3.7-max Reviewed at |
|
Confidence: 5/5 This is a clean, well-scoped fix for a real correctness bug. The per-clientId attach-ref ledger is the right abstraction — it directly couples every What I verified:
The two deliberate behavior changes are well-reasoned: anonymous detaches and spawn-owner detaches no longer affect LGTM, approving. ✅ 中文说明置信度:5/5 这是一个干净、范围合理的修复,针对真实的正确性 bug。按 clientId 记账的 attach 引用账本是正确的抽象——它将每次 验证内容:
两个刻意的行为变更推理合理:匿名 detach 和 spawn owner detach 不再影响 LGTM,批准。✅ — Qwen Code · qwen3.7-max Reviewed at |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
— qwen3.7-max via Qwen Code /review
Local build & real-run verification ✅I built this branch locally and verified it two ways — a unit run with a negative control, and a live 1. Unit suite + negative control
2. Live daemon A/B — counter theft observed over real HTTPI bundled
On pre-fix, two pure-noise detaches drove Also checked
EnvironmentLinux, Node 22, single-worktree A/B (fixed = PR head; pre-fix = 中文说明本地构建 + 真机运行验证 ✅我在本地构建了该分支并用两种方式做了验证——带负对照(negative control)的单元测试,以及在真实 HTTP 接口上跑起真正的 1. 单元测试 + 负对照PR head 上
2. 真机 daemon A/B —— 在真实 HTTP 上观测到“计数被偷”我从该 worktree 打出
pre-fix 下,两个纯噪声 detach 就把 另外核对
环境Linux,Node 22,单 worktree A/B(fixed = PR head;pre-fix = 🤖 Generated with Claude Code — Claude Opus 4.8 (1M context) |
|
Released in v0.20.1. |


What this PR does
Makes
detachClientidempotent by introducing a per-clientId attach reference ledger (attachRefs) on each session entry. Every attach that contributes toattachCountrecords one ledger ref for the registered clientId;detachClientmay only decrementattachCountby releasing a ref from that ledger. Detaches that hold no ref — duplicates, unknown clientIds, anonymous requests, and owner-style registrations (spawn owner, restore initiator) — leave the counter untouched, while the client registration itself is still dropped unconditionally (that operation is idempotent, and an owner's explicit goodbye must keep the close-on-last-detach path reachable).rollbackAttachRegistrationis updated to the same ledger discipline so restore-initiator rollbacks and coalesce-reservation rollbacks each subtract exactly what they contributed.Why it's needed
detachClientunconditionally decrementedattachCount, fully decoupled from whether the detach released a real attach reference. A duplicate detach, a stray or anonymousDELETE, or the spawn owner detaching with its own clientId all stole another attacher's count. Once the counter was stolen down to 0 while real attachers were still connected, thespawnOwnerWantedKilldeferred reap or the close-on-last-detach path killed a session that still had live clients, and every subsequent request from those clients 404'd. Fixes #7385.Reviewer Test Plan
How to verify
Run
cd packages/acp-bridge && npx vitest run src/bridge.test.ts. Six new tests cover the failure modes end-to-end: duplicate detach decrements only once; unknown-clientId and anonymous detaches don't decrement; a spawn owner detaching itself doesn't steal an attacher's ref (while its registration is still removed); the deferred reap fires exactly on the real last attacher's detach and not on noise detaches; and repeated attaches under one echoed clientId detach ref-by-ref. Two existing detach tests now pass the clientId, matching all six production call sites ofdetachClient(acp-http index, dispatch, session routes), which all forward a concrete id.Evidence (Before & After)
N/A (daemon-internal counter semantics; covered by unit tests — 416/416 passing in
bridge.test.ts, plusnpm run buildandnpm run typecheckclean at the repo root).Tested on
Environment (optional)
N/A — unit tests only.
Risk & Scope
DELETEdetach withoutX-Qwen-Client-Idno longer decrementsattachCount(still returns 204); attach/spawn responses always hand out a clientId, and clients that lost theirs are covered by the idle-reaper backstop already documented at the detach site. (2) A spawn owner'sDELETEdetach no longer affectsattachCount, but its registration is still removed.entry.attachCount = coalesceState.count(assignment, not accumulation), which can swallow concurrent attacher bumps during the restore window — unrelated to this fix and left for a follow-up. Also out of scope: under a shared clientId, an adversarial duplicate detach can still strip one extraclientIdsrefcount layer (noattachCountimpact, no premature kill); fully eliminating that would require a detach idempotency key, i.e. a protocol change.Linked Issues
Fixes #7385
中文说明
本 PR 做了什么
通过在每个 session entry 上引入按 clientId 记账的 attach 引用账本(
attachRefs),使detachClient幂等化。每次对attachCount有贡献的 attach 都会为注册的 clientId 记录一份账本引用;detachClient只有在成功释放一份账本引用时才允许递减attachCount。不持有引用的 detach——重复 detach、未知 clientId、匿名请求、以及 owner 型注册(spawn owner、restore 发起者)——不再影响计数,但客户端注册本身仍无条件移除(该操作本身幂等,且 owner 的显式告别必须保持 close-on-last-detach 路径可达)。rollbackAttachRegistration也改为同一账本纪律,使 restore 发起者回滚与 coalesce 预留回滚各自精确扣除自己贡献的部分。为什么需要
detachClient原先无条件递减attachCount,与该 detach 是否真实释放了一份 attach 引用完全解耦。重复 detach、伪造或匿名的DELETE、以及 spawn owner 用自身 clientId 的 detach 都会偷减其他 attacher 的计数。一旦计数在仍有真实 attacher 在线时被偷减到 0,spawnOwnerWantedKill延迟回收或 close-on-last-detach 路径就会杀掉仍有存活客户端的 session,这些客户端之后的所有请求都会 404。修复 #7385。审阅者验证方案
运行
cd packages/acp-bridge && npx vitest run src/bridge.test.ts。六个新增用例端到端覆盖各失败模式:重复 detach 只递减一次;未知 clientId 与匿名 detach 不递减;spawn owner 自 detach 不偷减 attacher 引用(其注册仍被移除);延迟回收精确地在真实最后一个 attacher detach 时触发、不被噪声 detach 提前触发;同一回显 clientId 多次 attach 后逐份引用释放。两个现有 detach 用例改为传 clientId,与detachClient全部六处生产调用点(acp-http index、dispatch、session routes)一致——它们都传递具体 id。证据:daemon 内部计数语义,由单元测试覆盖——
bridge.test.ts416/416 通过,仓库根npm run build与npm run typecheck干净。本地在 macOS 验证。风险与范围
X-Qwen-Client-Id的DELETEdetach 不再递减attachCount(仍返回 204);attach/spawn 响应始终下发 clientId,丢失 id 的客户端由 detach 站点已注明的 idle reaper 兜底。(2)spawn owner 的DELETEdetach 不再影响attachCount,但其注册仍被移除。entry.attachCount = coalesceState.count为赋值而非累加的既有问题(可能吞掉 restore 窗口内并发 attacher 的递增),与本修复无关,留待后续处理。同样范围外:共享 clientId 下攻击性的重复 detach 仍可多剥一层clientIdsrefcount(不影响attachCount、无提前 kill);彻底消除需要 detach 幂等键,即协议变更。关联 Issue
Fixes #7385