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

[doudouOUC]

Summary

detachClient in packages/acp-bridge/src/bridge.ts unconditionally decrements entry.attachCount and then unregisters the client. The decrement is completely decoupled from whether the detach actually released a live attach reference, so several perfectly legal call patterns steal another attacher's count:

• Duplicate detach: the same client detaching twice (e.g. route handler + disconnect reaper both firing) decrements twice for a single attach.
• Unknown clientId: a stray DELETE with a bogus X-Qwen-Client-Id still decrements.
• Anonymous detach: a DELETE without X-Qwen-Client-Id still decrements.
• Spawn owner self-detach: the spawn owner never contributes to attachCount (it stays 0 for the owner), yet its own DELETE detach decrements the counter that belongs to other attachers.

Impact

Once the counter is stolen down to 0 while real attachers are still connected, two kill paths fire early:

1. spawnOwnerWantedKill (the deferred-reap tombstone set when the owner's killSession({requireZeroAttaches: true}) bails) completes the reap and kills a session that still has live clients.
2. The close-on-last-detach path (clientIds.size === 0) becomes reachable earlier than it should.

Either way a live client's session is torn down underneath it — every subsequent request from that client 404s.

Reproduction (unit level)

With client A spawning (owner) and client B attached (attachCount === 1):

await bridge.detachClient(b.sessionId, b.clientId); // 1 -> 0, fine
await bridge.detachClient(b.sessionId, b.clientId); // duplicate: decrements again on pre-fix code, stealing the next attacher's count

Or more directly: after killSession(a.sessionId, { requireZeroAttaches: true }) bails and sets the tombstone, a detach with an unknown/anonymous clientId completes the deferred reap even though B never left.

Expected behavior

Every attachCount decrement must correspond to the release of a real attach reference, keyed by clientId. Detaches that don't hold such a reference (duplicates, unknown ids, anonymous requests, owner-style registrations) must leave the counter untouched — while still removing the client registration itself, which is idempotent.

Environment

• Affects the ACP bridge daemon serve mode (packages/acp-bridge/src/bridge.ts), present on current main.

[qwen-code-ci-bot]

Triage

• Type: bug
• Labels: type/bug, category/core, scope/session-management, priority/P2, welcome-pr
• Next: confirmed in source — detachClient decrements attachCount without verifying the detaching clientId actually held an attach reference. Fix direction: track which clientIds contribute to attachCount and only decrement on their detach.

---

Source Analysis

Reproduced: yes (source-level confirmation; the unit-level reproduction in the issue is accurate).

Root cause (packages/acp-bridge/src/bridge.ts, line ~7586):

async detachClient(sessionId, clientId) {
  const entry = byId.get(sessionId);
  if (!entry) return;
  if (entry.attachCount > 0) entry.attachCount--;  // ← unconditional
  unregisterClient(entry, clientId);               // ← properly keyed
  // ... kill paths fire when attachCount hits 0
}

attachCount is a global counter incremented only on attach paths (lines 3817, 4050, 4525, 4587). The spawn owner registers via registerClient (line 2340) but never touches attachCount. However, detachClient decrements the counter for any call — duplicate detaches, unknown clientIds, anonymous requests, and spawn-owner self-detaches all steal a count that belongs to a real attacher.

Once the counter is stolen to 0 while real attachers remain connected, two kill paths fire early:

1. spawnOwnerWantedKill tombstone completes the deferred reap.
2. clientIds.size === 0 close-on-last-detach becomes reachable.

Affected area: detachClient in packages/acp-bridge/src/bridge.ts (daemon serve mode).

Fix direction:

1. Add an attachedClientIds: Set<string> (or similar) to SessionEntry to track which clientIds contributed to attachCount.
2. On each attach path that does attachCount++, also add the clientId to the set.
3. In detachClient, only decrement attachCount if clientId is in the set, then remove it. Anonymous (clientId === undefined), unknown, and spawn-owner detaches leave the counter untouched.
4. unregisterClient remains idempotent — it already handles duplicate/unknown clientIds correctly for the registration map.
5. Add regression tests for: duplicate detach, unknown clientId detach, anonymous detach, and spawn-owner self-detach — each must leave attachCount unchanged.

Community PRs welcome — the issue is well-specified with a clear reproduction and fix direction.

中文说明

• 类型: bug
• 标签: type/bug, category/core, scope/session-management, priority/P2, welcome-pr
• 下一步: 源码已确认 — detachClient 在递减 attachCount 时未验证发起 detach 的 clientId 是否实际持有 attach 引用。

---

已复现：是（源码级确认；issue 中的单元级复现准确）。

根因（packages/acp-bridge/src/bridge.ts，约第 7586 行）：

attachCount 是全局计数器，仅在 attach 路径递增（3817、4050、4525、4587 行）。spawn owner 通过 registerClient（2340 行）注册但从不触碰 attachCount。然而 detachClient 对任何调用都递减计数器——重复 detach、未知 clientId、匿名请求、spawn owner 自行 detach 都会偷走属于真实 attacher 的计数。

计数被偷到 0 后，两条 kill 路径提前触发：

1. spawnOwnerWantedKill 墓碑完成延迟回收。
2. clientIds.size === 0 的 last-detach 关闭路径变得可达。

影响范围：packages/acp-bridge/src/bridge.ts 中的 detachClient（daemon serve 模式）。

修复方向：

1. 在 SessionEntry 上添加 attachedClientIds: Set<string>，记录哪些 clientId 贡献了 attachCount。
2. 每个递增 attachCount 的 attach 路径同时将 clientId 加入集合。
3. detachClient 中仅在 clientId 存在于集合时递减，然后移除。匿名、未知、spawn owner 的 detach 不动计数器。
4. unregisterClient 保持幂等——它已正确处理注册表的重复/未知 clientId。
5. 添加回归测试：重复 detach、未知 clientId detach、匿名 detach、spawn owner 自行 detach——均不得改变 attachCount。

欢迎社区 PR——issue 规格清晰，有明确复现和修复方向。

--- Qwen Code