Skip to content

fix(acp-bridge): make detachClient idempotent via per-clientId attach-ref ledger - #7386

Merged
doudouOUC merged 2 commits into
QwenLM:mainfrom
doudouOUC:fix/daemon-detach-attach-ref-ledger
Jul 21, 2026
Merged

fix(acp-bridge): make detachClient idempotent via per-clientId attach-ref ledger#7386
doudouOUC merged 2 commits into
QwenLM:mainfrom
doudouOUC:fix/daemon-detach-attach-ref-ledger

Conversation

@doudouOUC

Copy link
Copy Markdown
Collaborator

What this PR does

Makes detachClient idempotent by introducing a per-clientId attach reference ledger (attachRefs) on each session entry. Every attach that contributes to attachCount records one ledger ref for the registered clientId; detachClient may only decrement attachCount by 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). rollbackAttachRegistration is updated to the same ledger discipline so restore-initiator rollbacks and coalesce-reservation rollbacks each subtract exactly what they contributed.

Why it's needed

detachClient unconditionally decremented attachCount, fully decoupled from whether the detach released a real attach reference. A duplicate detach, a stray or anonymous DELETE, 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, the spawnOwnerWantedKill deferred 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 of detachClient (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, plus npm run build and npm run typecheck clean at the repo root).

Tested on

OS Status
🍏 macOS
🪟 Windows ⚠️
🐧 Linux ⚠️

Environment (optional)

N/A — unit tests only.

Risk & Scope

  • Main risk or tradeoff: two deliberate behavior changes. (1) A DELETE detach without X-Qwen-Client-Id no longer decrements attachCount (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's DELETE detach no longer affects attachCount, but its registration is still removed.
  • Not validated / out of scope: a pre-existing issue where the restore IIFE assigns 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 extra clientIds refcount layer (no attachCount impact, no premature kill); fully eliminating that would require a detach idempotency key, i.e. a protocol change.
  • Breaking changes / migration notes: none.

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.ts 416/416 通过,仓库根 npm run buildnpm run typecheck 干净。本地在 macOS 验证。

风险与范围

  • 主要风险/权衡:两个刻意的行为变更。(1)不带 X-Qwen-Client-IdDELETE detach 不再递减 attachCount(仍返回 204);attach/spawn 响应始终下发 clientId,丢失 id 的客户端由 detach 站点已注明的 idle reaper 兜底。(2)spawn owner 的 DELETE detach 不再影响 attachCount,但其注册仍被移除。
  • 未验证/范围外:restore IIFE 中 entry.attachCount = coalesceState.count 为赋值而非累加的既有问题(可能吞掉 restore 窗口内并发 attacher 的递增),与本修复无关,留待后续处理。同样范围外:共享 clientId 下攻击性的重复 detach 仍可多剥一层 clientIds refcount(不影响 attachCount、无提前 kill);彻底消除需要 detach 幂等键,即协议变更。
  • 破坏性变更/迁移说明:无。

关联 Issue

Fixes #7385

…-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.
@qwen-code-ci-bot

qwen-code-ci-bot commented Jul 21, 2026

Copy link
Copy Markdown
Collaborator

Thanks for the PR!

Template looks good ✓

Problem: observed bug with a linked issue (#7385) and a clear source-level reproduction. detachClient unconditionally decrements attachCount regardless of whether the detaching clientId actually held an attach reference — duplicate detaches, unknown/anonymous clientIds, and spawn-owner self-detaches all steal another attacher's count, triggering premature session kills. Confirmed in the current source at bridge.ts ~line 7588.

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 — packages/acp-bridge is not a core infrastructure path per the two-tier gate. ~60 production lines and ~149 test lines across 2 files.

Approach: the scope feels right. A per-clientId refcount ledger (attachRefs: Map<string, number>) is the natural minimal fix — it directly addresses the root cause (decrement decoupled from actual attach ownership) without over-engineering. The PR correctly updates all five attach call sites, detachClient, and rollbackAttachRegistration to the same discipline. The two deliberate behavior changes (anonymous detach no longer decrements; spawn-owner detach no longer affects attachCount) are well-reasoned and documented. Out-of-scope items (restore IIFE assignment-vs-accumulation, shared-clientId refcount stripping) are correctly deferred.

Moving on to code review. 🔍

中文说明

感谢贡献!

模板完整 ✓

问题:已观测到的 bug,有关联 issue(#7385)和清晰的源码级复现。detachClient 无条件递减 attachCount,不管发起 detach 的 clientId 是否实际持有 attach 引用——重复 detach、未知/匿名 clientId、spawn owner 自行 detach 都会偷走其他 attacher 的计数,导致 session 被提前 kill。已在当前源码 bridge.ts 约第 7588 行确认。

方向:对齐——这是 daemon session 生命周期中的真实正确性 bug。Claude Code 的 CHANGELOG 中有类似修复("在一个窗口按返回不再 detach 其他 attach 到同一 session 的窗口"),说明这类问题在该领域是已知的。

规模:不适用——packages/acp-bridge 不属于核心基础设施路径。2 个文件,约 60 行生产代码、149 行测试代码。

方案:范围合理。按 clientId 记账的引用账本(attachRefs: Map<string, number>)是自然的最小修复——直接解决根因(递减与实际 attach 归属解耦),没有过度设计。PR 正确更新了全部五处 attach 调用点、detachClientrollbackAttachRegistration。两个刻意的行为变更(匿名 detach 不再递减;spawn owner detach 不再影响 attachCount)推理合理且有文档说明。范围外事项(restore IIFE 赋值 vs 累加、共享 clientId refcount 剥离)正确推迟。

进入代码审查 🔍

Qwen Code · qwen3.7-max

Reviewed at a5e289f8bdba0240859316eefe1f5db150d933e6 · re-run with @qwen-code /triage

@wenshao

wenshao commented Jul 21, 2026

Copy link
Copy Markdown
Collaborator

Code Review — fix(acp-bridge): make detachClient idempotent via per-clientId attach-ref ledger

Reviewed the full diff against main plus the surrounding bridge.ts machinery (attach paths, rollbackAttachRegistration, killSession tombstone, the idle reaper, and all production detachClient call sites). This is a well-scoped, correctly-implemented fix for a real 404-causing bug (#7385). LGTM — the notes below are non-blocking.

What it does

Adds a per-clientId attachRefs ledger to each SessionEntry. attachCount may now only be decremented by a detach that actually releases a recorded ref. Duplicate detaches, unknown/anonymous clientIds, and owner-style registrations (spawn owner, restore initiator) hold no ref, so they can no longer steal a live attacher's count and trigger the spawnOwnerWantedKill / close-on-last-detach reap while real clients are still connected.

Correctness — verified

  • Every attachCount increment is paired with exactly one recordAttachRef. Checked all five contributing sites (bridge.ts:3861/3863, 4098/4100, 4574/4576, 4637/4654, and the coalesce-waiter 3936); each records against the returned (post-registerClient) id, so echoed-id refcounting stays consistent. Owner-style registrations (4195 restore initiator; spawn owner) deliberately register a clientId without a ref and contribute 0 to attachCount — internally consistent with entry.attachCount = coalesceState.count (waiters only).
  • rollbackAttachRegistration delta arithmetic balances for both callers. Default delta=1 (via applyApprovalModeForAttach) subtracts exactly the initiator's ref when present. The delta = 1 + coalesceState.count caller (4110) reduces to released(=1) + (delta-1) = the full += 1 + count from 4098; the rejected coalescers unwind via the 3911 catch and never register/record, so attachCountDelta - 1 correctly accounts for reservations that carry no ledger entry. The comment is accurate.
  • detachClient is idempotent and the close paths stay reachable. unregisterClient still runs unconditionally, so clientIds.size → 0 continues to drive close-on-last-detach and an owner's explicit goodbye still works, even when the detach releases no ref. Duplicate detach with the real id is fully idempotent (ref gone → releaseAttachRef false; clientIds entry already gone → unregisterClient no-op).
  • Backstop confirmed. The idle reaper (bridge.ts:1583) reaps purely on idle TTL, gated only on promptActive / subscriberCountnot on attachCount. So the deliberate "anonymous detach no longer decrements" change can at worst delay teardown to sessionIdleTimeoutMs, never leak permanently, and a live SSE subscriber is correctly protected from reaping. This is the right direction of the tradeoff.
  • No signature/typecheck risk. detachClient(sessionId, clientId?) already existed in bridgeTypes.ts and the prior impl already forwarded clientId to unregisterClient; the diff touches only bridge.ts + the test file. All 8 production call sites (acp-http index/dispatch, routes/session) already pass a concrete id — the rollback paths use the just-returned clientId, and the two HTTP detach handlers derive it from parseClientIdHeader, so a conformant client that echoed the id on attach echoes it on detach.

Tests

Genuinely good — the six new cases drive the real bridge API (spawnOrAttach / detachClient / killSession) and assert observable sessionCount outcomes, so they exercise the actual gating rather than mocking around it. They cover duplicate / unknown / anonymous / owner-self / deferred-reap-timing / echoed-id-refcount.

Non-blocking suggestions

  1. Add a multi-waiter restore-coalesce test. The subtlest new accounting — entry.attachCount = coalesceState.count ↔ per-waiter recordAttachRef (3936), and the raced-entry += 1 + count / recordAttachRef (4098/4100) — has no direct coverage. A test that coalesces N restore waiters onto one in-flight restore, then detaches them ref-by-ref (asserting the deferred reap fires only on the last), would lock in that balance and guard the rollbackAttachRegistration(delta = 1 + count) branch.
  2. Out-of-scope races (already flagged in the PR) are correctly deferred. Worth an explicit note in the follow-up: with the ledger now present, the attachCount = coalesceState.count assignment race can leave attachRefs and attachCount momentarily divergent (a swallowed waiter bump still records a ref), which re-opens a narrower window of the same detachClient steals other attachers' attachCount, causing premature session kill in daemon serve mode #7385 class. The ledger doesn't make it worse, but it also doesn't close it — the real fix is accumulation instead of assignment there. Similarly, rollbackAttachRegistration re-reads the mutable coalesceState.count across the applyApprovalMode await (4103→4110), a pre-existing fragility unchanged by this PR.
  3. Micro: releaseAttachRef's refs <= 0 guard is unreachable defensive code (refs are only ever set ≥ 1 then deleted at 1) — harmless, fine to leave.

Review based on static analysis of the diff and surrounding code; I did not run the suite locally, but the logic balances at every site and the signature change is source-compatible.

… 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.
@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Code Review

Independent proposal (before reading the diff): given the problem — detachClient decrements attachCount without checking whether the detaching clientId actually contributed to it — I would add a Map<string, number> refcount ledger to SessionEntry, record a ref at every attach site that increments attachCount, and gate every decrement on releasing a ref from that ledger. Anonymous, unknown, and owner-style detaches would leave the counter untouched while still removing the client registration (which is idempotent).

Comparison with the diff: the PR does exactly this. The implementation matches my independent proposal closely:

  • attachRefs: Map<string, number> on SessionEntry — refcounted per clientId, correctly handling repeated attaches under the same echoed clientId.
  • recordAttachRef / releaseAttachRef helpers — clean, minimal, no over-abstraction.
  • All 4 attachCount increment sites (lines 3817, 4050, 4525, 4587) are paired with recordAttachRef. The coalescer path (line ~3886) correctly records a ref for its pre-folded contribution.
  • Both decrement sites are gated: detachClient checks clientId !== undefined && releaseAttachRef(entry, clientId), and rollbackAttachRegistration uses releaseAttachRef for the initiator's own contribution while handling coalesce reservations (attachCountDelta - 1) that never registered a clientId.
  • All 8 production call sites of detachClient (acp-http index ×2, dispatch ×2, session routes ×4) pass a concrete clientId — verified by grep.
  • unregisterClient remains unconditional — correct, registration removal is idempotent and the owner's explicit goodbye must keep close-on-last-detach reachable.

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 Results

Unit tests (against a5e289f8):

 ✓ src/bridge.test.ts (416 tests) 4219ms

 Test Files  1 passed (1)
      Tests  416 passed (416)
   Duration  9.59s

Build: clean. Typecheck: clean.

Daemon smoke test (tmux, npm run dev -- serve --port 18923):

> @qwen-code/qwen-code@0.20.0 dev
> node scripts/dev.js serve --port 18923

qwen serve: daemon log → /home/github-runner/actions-runner-12/_work/_temp/qwen-home/debug/daemon/daemon.log
qwen serve: Web Shell UI served from .../packages/web-shell/dist
qwen serve listening on http://127.0.0.1:18923 (mode=http-bridge, workspace=...)
qwen serve: bound to workspace "..."
qwen serve: startup timing: processToListenMs=6663 runQwenServeToListenMs=4924
qwen serve: bearer auth disabled (loopback default). Set QWEN_SERVER_TOKEN to enable.
2026-07-21T04:09:06.871Z [INFO] [DAEMON] deferred runtime: scheduling fallback start in 1000ms
2026-07-21T04:09:07.875Z [INFO] [DAEMON] deferred runtime: fallback timer fire, starting
2026-07-21T04:09:09.098Z [INFO] [DAEMON] ideEnvPresent=false primary=... secondary= daemon workspace roots initialized
qwen serve: session reaper started (interval 60000ms, idle threshold 1800000ms)
qwen serve: /acp WebSocket transport enabled on /acp

Health check: {"status":"ok"} — daemon starts and serves correctly with the PR code.

中文说明

代码审查

独立方案(未看 diff 前):针对 detachClient 不检查 clientId 是否贡献了 attachCount 就递减的问题,我会在 SessionEntry 上加一个 Map<string, number> 引用账本,在每个递增 attachCount 的 attach 站点记录引用,并在每次递减前要求释放引用。匿名、未知、owner 型 detach 不动计数器,但仍移除客户端注册(该操作幂等)。

与 diff 对比:PR 的实现与我的独立方案高度一致:

  • attachRefs: Map<string, number>——按 clientId 记账,正确处理同一 clientId 多次 attach 的 refcount。
  • recordAttachRef / releaseAttachRef 辅助函数——简洁、最小化、无过度抽象。
  • 全部 4 处 attachCount 递增站点均配对 recordAttachRef。coalescer 路径正确地为预折叠的贡献记录引用。
  • 两处递减站点均受控:detachClient 检查 clientId !== undefined && releaseAttachRef(entry, clientId)rollbackAttachRegistrationreleaseAttachRef 处理发起者自己的贡献,同时处理从未注册 clientId 的 coalesce 预留(attachCountDelta - 1)。
  • 全部 8 处 detachClient 生产调用点均传递具体 clientId——已通过 grep 验证。
  • unregisterClient 保持无条件——正确,注册移除幂等,owner 的显式告别必须保持 close-on-last-detach 路径可达。

未发现关键阻塞问题。无 AGENTS.md 违规。v2 提交通过引入第三个客户端 C 加强了重复 detach 测试,使计数偷窃在存活 attacher 下可直接检测。

测试结果

单元测试(针对 a5e289f8):416/416 通过。构建:干净。类型检查:干净。

Daemon 冒烟测试(tmux):daemon 正常启动并服务,健康检查返回 {"status":"ok"}

Qwen Code · qwen3.7-max

Reviewed at a5e289f8bdba0240859316eefe1f5db150d933e6 · re-run with @qwen-code /triage

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

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 attachCount decrement to the release of a recorded attach reference, eliminating the entire class of count-theft bugs (duplicate detach, unknown/anonymous clientId, spawn-owner self-detach) in one stroke.

What I verified:

  • All 4 attachCount increment sites paired with recordAttachRef
  • Both decrement sites (detachClient, rollbackAttachRegistration) gated on releaseAttachRef
  • All 8 production call sites of detachClient forward a concrete clientId ✓
  • unregisterClient remains unconditional (idempotent, keeps close-on-last-detach reachable) ✓
  • 416/416 unit tests passing (including 6 new DAEMON-006 regression tests) ✓
  • Build and typecheck clean ✓
  • Daemon starts and serves correctly ✓

The two deliberate behavior changes are well-reasoned: anonymous detaches and spawn-owner detaches no longer affect attachCount, with the idle-reaper backstop covering clients that lost their clientId. Out-of-scope items are correctly identified and deferred.

LGTM, approving. ✅

中文说明

置信度:5/5

这是一个干净、范围合理的修复,针对真实的正确性 bug。按 clientId 记账的 attach 引用账本是正确的抽象——它将每次 attachCount 递减与释放已记录的 attach 引用耦合,一次性消除了整类计数偷窃 bug(重复 detach、未知/匿名 clientId、spawn owner 自行 detach)。

验证内容:

  • 全部 4 处 attachCount 递增站点配对 recordAttachRef
  • 两处递减站点(detachClientrollbackAttachRegistration)受控于 releaseAttachRef
  • 全部 8 处 detachClient 生产调用点传递具体 clientId ✓
  • unregisterClient 保持无条件(幂等,保持 close-on-last-detach 可达)✓
  • 416/416 单元测试通过(含 6 个新增 DAEMON-006 回归测试)✓
  • 构建和类型检查干净 ✓
  • Daemon 正常启动和服务 ✓

两个刻意的行为变更推理合理:匿名 detach 和 spawn owner detach 不再影响 attachCount,丢失 clientId 的客户端由 idle reaper 兜底。范围外事项正确识别并推迟。

LGTM,批准。✅

Qwen Code · qwen3.7-max

Reviewed at a5e289f8bdba0240859316eefe1f5db150d933e6 · re-run with @qwen-code /triage

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, looks ready to ship. ✅

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Downgraded from Approve to Comment: CI still running. Reviewed.

— qwen3.7-max via Qwen Code /review

@doudouOUC
doudouOUC added this pull request to the merge queue Jul 21, 2026
Merged via the queue into QwenLM:main with commit ce803df Jul 21, 2026
60 checks passed
@wenshao

wenshao commented Jul 21, 2026

Copy link
Copy Markdown
Collaborator

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 qwen serve daemon A/B over the real HTTP surface. Both confirm the fix; recommending it as a merge reference.

1. Unit suite + negative control

bridge.test.ts is 416/416 green on the PR head. To prove the six new tests actually discriminate the fix (not just that green is green), I reverted only bridge.ts back to the merge-base and re-ran: 5 of the new tests immediately go red, then pass again once the production hunk is restored.

unit tests + negative control

The one new test that stays green under revert (repeated attach … ref-by-ref) isn't a gap — both of its detaches release a real ref, so pre-fix and post-fix agree on that path by construction. The five theft/noise cases are the discriminating ones.

2. Live daemon A/B — counter theft observed over real HTTP

I bundled dist/cli.js from this worktree and drove the actual REST surface: A spawns (owner), B and C attach onto one single-scope session (attachCount = 2, clientCount = 3), then I fire a bogus-clientId detach and an anonymous detach, followed by B's real detach and a duplicate. attachCount is read from GET /daemon/status?detail=full. The pre-fix column is the same worktree with bridge.ts at merge-base, rebuilt (tsc + esbuild) — identical harness, identical requests.

live daemon A/B

Step (same session, B & C stay connected) pre-fix attachCount fixed attachCount
A(owner) + B attach + C attach 2 2
DELETE detach, bogus X-Qwen-Client-Id 1 ⬅ stolen 2
DELETE detach, no X-Qwen-Client-Id (anon) 0 ⬅ stolen to zero 2
B's real detach (b.clientId) 0 1
B's duplicate detach (b.clientId again) 0 1 ⬅ C's ref intact

On pre-fix, two pure-noise detaches drove attachCount from 2 → 0 while both B and C were still attached (clientCount stayed 3). That zero is precisely the precondition that lets spawnOwnerWantedKill / close-on-last-detach reap a live session — which the deferred reap fires exactly on the real last attacher unit test (red without the fix) exercises directly. On the fixed build the counter is immune to the noise, drops by exactly one per real ref release, and the duplicate is a no-op.

Also checked

  • eslint --max-warnings 0 on both changed files → clean (CI's Test job gates on this before vitest).
  • All six production detachClient call sites forward a concrete clientId, matching the test claim.
  • packages/cli serve suites touching detachClient (server.test.ts, acp-http/transport.test.ts, multi-workspace-sessions.test.ts, …) stay green. Two unrelated failures in workspace-agents/workspace-memory are pre-existing fs.chmod-as-root artifacts of this sandbox, not from this PR.

Environment

Linux, Node 22, single-worktree A/B (fixed = PR head; pre-fix = bridge.ts @ merge-base). macOS-equivalent behavior; the daemon internals are platform-independent.

中文说明

本地构建 + 真机运行验证 ✅

我在本地构建了该分支并用两种方式做了验证——带负对照(negative control)的单元测试,以及在真实 HTTP 接口上跑起真正的 qwen serve daemon 做 A/B 对比。两者都证实了修复有效,作为合并参考推荐。

1. 单元测试 + 负对照

PR head 上 bridge.test.ts 416/416 全绿。为了证明六个新增用例确实能区分“有修复/无修复”(而不只是“绿就是绿”),我bridge.ts 回退到 merge-base 再跑一遍:5 个新用例立刻变红,把生产代码那段改回来后又全绿。见上方第一张图。

唯一在回退后仍保持绿色的新用例(repeated attach … ref-by-ref)不是漏洞——它的两次 detach 都释放了真实引用,因此修复前后在这条路径上本就一致。真正有区分度的是那五个“偷计数/噪声 detach”的用例。

2. 真机 daemon A/B —— 在真实 HTTP 上观测到“计数被偷”

我从该 worktree 打出 dist/cli.js,驱动真正的 REST 接口:A spawn(owner),BC attach 到同一个 single-scope session(attachCount = 2clientCount = 3),然后依次发一个伪造 clientId 的 detach、一个匿名 detach,再发 B 的真实 detach 和一次重复 detach。attachCountGET /daemon/status?detail=full 读取。pre-fix 那一列是同一个 worktree、把 bridge.ts 切到 merge-base 后重新构建(tsc + esbuild)——完全相同的 harness、相同的请求。见上方第二张图。

步骤(同一 session,B 和 C 全程在线) pre-fix attachCount fixed attachCount
A(owner) + B attach + C attach 2 2
DELETE detach,伪造 X-Qwen-Client-Id 1 ⬅ 被偷 2
DELETE detach,无 X-Qwen-Client-Id(匿名) 0 ⬅ 被偷到 0 2
B 的真实 detach(b.clientId 0 1
B 的重复 detach(再发一次 b.clientId 0 1 ⬅ C 的引用完好

pre-fix 下,两个纯噪声 detach 就把 attachCount 从 2 拉到了 0,而此时 B 和 C 都还连着clientCount 仍为 3)。这个 0 正是让 spawnOwnerWantedKill / close-on-last-detach 去回收一个仍有存活客户端的 session 的前置条件——这一点由 deferred reap fires exactly on the real last attacher 单元用例(无修复时为红)直接覆盖。修复后计数对噪声免疫,每释放一个真实引用才减一,重复 detach 为 no-op。

另外核对

  • 两个改动文件的 eslint --max-warnings 0 → 干净(CI 的 Test job 在 vitest 之前会卡这一关)。
  • detachClient 全部六处生产调用点都传了具体 clientId,与用例描述一致。
  • packages/cli 中涉及 detachClient 的 serve 套件(server.test.tsacp-http/transport.test.tsmulti-workspace-sessions.test.ts 等)保持全绿。workspace-agents/workspace-memory 里两个无关失败是本沙箱 root 身份下 fs.chmod 造成的既有现象,与本 PR 无关。

环境

Linux,Node 22,单 worktree A/B(fixed = PR head;pre-fix = bridge.ts @ merge-base)。daemon 内部逻辑与平台无关,macOS 行为等价。


🤖 Generated with Claude Code — Claude Opus 4.8 (1M context)

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Released in v0.20.1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

detachClient steals other attachers' attachCount, causing premature session kill in daemon serve mode

3 participants