fix(daemon): Preserve sessions when active-work close is refused - #9134
Conversation
|
Re-run after rounds 2–3 were addressed in Template: complete ✓ Problem: real — verified in code across three review rounds, not taken on faith. Both #9042 lifecycle races exist on Direction: unchanged and aligned — stabilizes the lifecycle #9042 merged, refs roadmap issue #8586. No CHANGELOG reference; the area is in scope regardless. Size: 119 production lines (bridge.ts 47, bridgeTypes.ts 12, acpAgent.ts 60) vs 427 test lines, spanning two packages with an unchanged wire surface. Approach: still minimal. The two new commits are exactly the round-2 and round-3 asks: the drain budget now keys to the outer wait actually applied (all four call sites), and killSession tolerates a definitive child-side refusal instead of channel-killing siblings — each with a test that goes red on base. No drive-by changes. Risk: The gate passes. 🔍 中文说明在 **模板：**完整 ✓ **问题：**真实存在——三轮 review 均已在代码中核实，非采信自述。#9042 的两个生命周期竞态存在于 main 上：conditional close 在最终 hold 复查前中止在途 turn；延迟 spawn-owner kill 被挡在 legacy 类别 child 永远无法满足的 auto-close candidacy 之后。第二、三轮又各自发现修复自身的一处真实升级（drain 预算配错外层等待键；killSession 把 child 的明确拒绝升级为 channel kill）。四处缺陷现均有代码与锁定测试。 **方向：**不变且对齐——稳定 #9042 合并的生命周期，关联 roadmap issue #8586。 **规模：**生产代码 119 行（bridge.ts 47、bridgeTypes.ts 12、acpAgent.ts 60），测试 427 行，跨两个包且 wire 接口不变。 **方案：**仍然最小。两个新 commit 恰为第二、三轮的要求：drain 预算改为以实际应用的外层等待为键（四处调用点全部覆盖）；killSession 容忍 child 侧明确拒绝而非 channel-kill 连坐——各有一个在 base 上转红的测试。无夹带改动。 风险： 门禁通过。🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewIndependent proposal carried from round 1, restated before touching the diff: refuse on known holds, let the running turn settle naturally under the close gate, re-check holds, only then cancel queued work and tear down — both drain phases under one deadline that the daemon keys strictly below its own outer wait; on the daemon side, move the deferred kill ahead of the auto-close candidacy gate behind identity and close-state guards, and treat a definitive child refusal as "the child is alive and kept the session" rather than escalating. The diff matches that proposal, including the round-3 killSession branch I had not yet seen; I did not find a simpler path it missed. Round-2 findings — all addressed, each re-verified against this commit:
Round-3 findings — all addressed:
Non-blocking, no action needed this PR: the history-mutation body itself stays untimed, so a pathological mutation could still push the round trip past the daemon's outer wait — the new comment scopes the guarantee honestly as approximate, and the shape is pre-existing and shared with unconditional close. The conditional-close ordering (unchanged from round 1, still correct at this head): sequenceDiagram
participant P1 as Daemon bridge
participant P2 as Child close handler
participant P3 as Session
P1->>P2: sessionClose - onlyIfUnheld, drain budget 8000ms
P2->>P2: take close gate (blocks new turns)
P2->>P3: collectActiveWorkHolds (early read)
P3-->>P2: no holds
P2->>P3: waitForActiveTurnsToSettle (natural settle)
P3-->>P2: settled
P2->>P3: collectActiveWorkHolds (re-check)
alt holds appeared while settling
P2-->>P1: closed false + holds (session untouched)
else still unheld
P2->>P2: abort controllers + cancel queued work (remaining budget)
P2->>P3: finalize recorder, dispose session
P2-->>P1: closed true
end
CI test evidenceThe PR's own CI at the reviewed commit is settled — green, no failures. Real check names and conclusions:
The one cancelled What green does not prove: the suite pins the new ordering and budgets with mocks, and the two round-2/round-3 Criticals pass identically with the old code absent from the suite's assertions anyway — they are pinned by the new tests, which the maintainer's mutation check confirms go red on base source. The behavioural evidence for this head is the maintainer's independent real-stack verification (real Sandboxed verification is already in flight for exactly this gap: a sponsored 中文说明**代码审查：**沿用第一轮的独立方案（先于 diff 写下）：已知 hold 立即拒绝、close gate 下让运行中 turn 自然结束、复查 hold、之后才取消排队工作并拆除——两个 drain 阶段共用一个截止时间，且 daemon 告知 child 的预算严格小于自身外层等待；daemon 侧把延迟 kill 提到 auto-close candidacy 门之前，以身份与关闭状态守卫保护，并把 child 的明确拒绝视为"child 存活且保留了会话"而非升级。diff 与方案一致（包括我此前未见过的第三轮 killSession 分支），未找到更简路径。 第二轮发现全部处理并逐项复核：R2-1【Critical】drain 预算错配已修—— 第三轮发现全部处理：R3-5【Critical】killSession 不再把明确拒绝升级为 channel kill——catch 按 非阻塞、本 PR 无需动作：history-mutation 本体仍不计时间，极端情况下往返仍可能超出外层等待——新注释已诚实标注保证为近似，且该形态先于本 PR、与无条件关闭共有。 **CI 证据：**被审 commit 的 CI 已落定为绿、无失败（真实 check 名与结论见上表；表外说明：唯一 cancelled 的 **沙箱验证通道：**针对此缺口的赞助 — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 4/5 — all three rounds of findings are addressed with pinning tests, both Criticals re-verified independently against this commit, CI settled green, and the maintainer's real-stack A/B verification reproduces both races on base and confirms them gone here; the remaining items are named non-blocking gaps, not reservations about the change itself. Reflection, honestly:
中文说明置信度：4/5 —— 三轮发现全部以锁定测试处理，两个 Critical 已在本 commit 上独立复核，CI 落定为绿，maintainer 的真实栈 A/B 验证在 base 上复现两个竞态并确认本 PR 上消失；剩余项为已点名的非阻塞缺口，而非对本变更本身的保留。 反思：这是生命周期安全 PR 应有的落地方式——四轮 review，每一轮都有代码或有理有据的反驳（包括两处经得起推敲的 dispute），门禁发现的两处 Critical（第二轮预算错配、第三轮 killSession 升级）作者每次都以近单行修复加测试解决。两个 Critical 修复均在 — Qwen Code · qwen3.8-max Reviewed at |
qwen-code-ci-bot
left a comment
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
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not explored to full depth (tool budget reached): "agent 4": none — no checks were cut short..
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未探索到全部深度（达到工具调用预算）："agent 4"：none — no checks were cut short.。
— qwen3.8-max via Qwen Code /review (v0.21.12)
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
The reaper can hold a conditional-close probe on a tombstoned entry for up to ACTIVE_WORK_CLOSE_TIMEOUT_MS; a deferred kill fired in that window bounces off the close gate of the child and killSession escalates the error to a channel kill, taking every sibling session down with it. Re-add the activeWorkCloseInFlight exclusion to the branch: the probe resolves the entry one way or the other, and a refusal leaves the tombstone to complete on the next settle event. Also extract sessionCloseDrainBudgetMs so the child drain budget lives in one place, report the shared drain budget instead of the phase-2 residue in the close timeout message, and pin the guards with tests (in-flight probe, in-flight force close, no abort on refusal, phase-1 settle timeout, close-gate release on the timeout path). Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
1cb5b06 to
618717f
Compare
|
Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. 中文请勿对活跃的 PR 执行 rebase 或 force-push，因为这会使已有的评审评论失效。另外，供日后参考：作为集成流程的一部分，机器人始终会自动将所有改动压缩（squash）为单个提交。 |
|
Review round 1 addressed in
Verification: acp-bridge 1463/1463, acpAgent 429/429, |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
— qwen3.8-max via Qwen Code /review (v0.21.12)
|
@qwen-code /triage |
|
Sandboxed verification: ✅ passed — merge-ready (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 17 passed · 0 failed · 17 total 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：17 通过 · 0 失败 · 17 总计 Verification reportVerification — PR #9134
|
| Scenario | base (HEAD^1) |
head (HEAD^2) |
oracle |
|---|---|---|---|
| deferred kill completes for legacy-category child | red (sessionCount stays 1) |
green (0) | vitest honors a deferred spawn-owner kill for an incomplete child |
conditional close sends drainTimeoutMs=8000 |
red (param absent) | green | vitest asks the child about a session it has never reported on |
| refusal leaves running turn un-aborted, un-cancelled | red (cancelPendingPrompt called) |
green | vitest rechecks local holds after active turns drain... |
| one shared drain budget across both phases | red (close resolved, no shared timeout) | green | vitest shares one timeout budget... |
| settle phase capped by shared budget (no hang) | red (cancelPendingPrompt called) |
green | vitest rejects a conditional close when the settle phase exceeds... |
| pre-category baseline refusal (guard) | green | green | vitest uses the pre-category v1 baseline... (pre-existing, both arms) |
Base cells were realized by restoring the HEAD^1 copy of the single changed unit (bridge.ts or acpAgent.ts) while keeping the rest of the tree at head — valid because the depth-2 checkout means base and head differ only by this PR, and bridgeTypes.ts (which the test imports sessionCloseDrainBudgetMs from) stays at head.
Mutation matrix (deferred-kill guards) — 03-mutation-matrix.png
| Mutation (at head) | failing focused test | status |
|---|---|---|
drop !entry.activeWorkCloseInFlight |
defers a tombstoned kill while a conditional close probe is in flight |
KILLED |
drop !entry.closing |
does not escalate a deferred kill while a force close is in flight |
KILLED |
drop byId.get(...)===entry |
(none) | SURVIVED → coverage gap |
Positive control: unmutated head passes all focused tests; mutations A and B each turn exactly one test red, proving the harness is live.
Findings (ordered by severity)
-
Suggestion (non-blocking) — identity guard not pinned by a test. Removing
byId.get(entry.sessionId) === entryfrom the deferred-kill branch leaves all focused tests green (mutation C). This is a coverage gap, not dead code: without it,bridgeApi.killSession(entry.sessionId)re-resolves by raw id and could kill a re-registered entry (thesession/loadre-registration race documented incloseIfChildUnheld's identity comment). The guard preserves the identity protection base provided implicitly via theentryIsAutoCloseCandidategate, so behavior is correct at head — the gap is only that no focused test constructs the re-registration race. Recommend adding one; not merge-blocking. -
Note (expected, not a defect) — the two escalation-guard tests also pass on base.
defers a tombstoned kill...anddoes not escalate...pass on base because base's candidacy gate coincidentally excludedclosing/activeWorkCloseInFlightentries. They are regression guards for the reorder; their load-bearing nature is proven by mutations A and B (dropping the now-explicit guards turns each red). This is the intended shape, not a vacuity problem. -
Correction to the description (minor). The body says "the complete ACP bridge suite passes with 642 tests"; the measured count at head is 671 (
bridge.test.ts). Not a code defect — the description's number is stale.
Not covered
- Per-commit attribution. Depth-2 shallow checkout: only the head commit (
618717f7) is reachable; the first PR commit (b316e9e1) is not. Verified the aggregateHEAD^1..HEADdiff; per-commit attribution was out of reach. - Real end-to-end daemon↔child over sockets. Harnesses are the PR's own vitest harnesses: bridge tests use an in-memory channel (real bridge), acpAgent tests use a mocked
Sessioncollaborator (realcloseStoredSession). The A/B proves the changed logic, not the live wire. - Full-repo build/typecheck. The PR states it is blocked by a pre-existing shared-dependency resolution issue; I ran the package-level vitest gates instead.
- Windows/Linux runtime behavior and channel liveness / Agent watchdog / shell-stall detection / retry — out of scope per the PR.
Methodology
Environment: node:22-bookworm CI verify container; npm ci + npm run build already done at head. Each A/B cell ran the PR's own focused vitest test against (a) head and (b) a base cell built by restoring the HEAD^1 copy of the single changed unit; the oracle is the vitest pass/fail of that test, so "base red" is itself an encoded-passing assertion that the bug exists pre-fix. Mutation cells edited one guard out of head's bridge.ts and re-ran the focused tests. Gates: full packages/acp-bridge/src/bridge.test.ts (671/671) and packages/cli/src/acp-integration/acpAgent.test.ts (429/429), exit 0. Raw logs in logs/, harnesses in harness/, captures in evidence/. All source files restored to head after each cell (working tree left clean).
Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention).
— Qwen Code · sandboxed verification
|
Superseded: the re-run at 已被取代：针对 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not explored to full depth (tool budget reached): "agent 4": none — completed all planned checks (used about 9 tool calls, well under 42). No Budget gap: lines..
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未探索到全部深度（达到工具调用预算）："agent 4"：none — completed all planned checks (used about 9 tool calls, well under 42). No Budget gap: lines.。
— qwen3.8-max via Qwen Code /review (v0.21.12)
Round-2 review follow-ups: - notifyAgentSessionClose derived the child drain budget from the initTimeoutMs default even when the caller applied a shorter outer wait (opts.timeoutMs): a condemned-channel close with an operator --initialize-timeout-ms of 30s told the child to drain for 24s while the daemon stopped listening after 10s, and the unknown outcome escalates to a channel kill. The budget now keys off the wait actually applied. - branchSession's partial-restore cleanup was the last sessionClose sender without a drainTimeoutMs; it now uses the shared helper. - The deferred spawn-owner kill branch uses the canonical isClosingOrAuthorizingClose predicate instead of an inline copy. - The conditional close's history-mutation wait now receives the shared-budget residue instead of a fresh full budget, keeping the round trip under the daemon's outer wait (the body stays untimed, so the guarantee is approximate); its timeout message reports the shared budget. - Tests: pin the conditional-close success path (abort ordered after the first settle via invocationCallOrder) and deduplicate the close-gate tracking mock into trackCloseGateHeld. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Round-3 review follow-ups: - killSession escalated ANY sessionClose error to a channel kill, but a child holding its own close gate (changeSessionCwd, restore — child-side state the daemon cannot observe) answers with a definitive RequestError. On that answer the kill now resets entry.closing and returns false, leaving the deferred tombstone to complete on the next settle event, instead of SIGTERMing every sibling session on the channel. Pinned by a test that flips the child from definitive refusal to acceptance and asserts the kill completes without a channel kill. - The deferred spawn-owner kill logs one stderr line before firing; its success path was previously unattributable in daemon logs. - sessionCloseDrainBudgetMs gains a literal unit test pinning the ratio strictly under the outer wait and the >=1ms clamp, so the documented invariants cannot drift with the implementation. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
Review rounds 2 and 3 addressed in
Verification: acp-bridge 673/673, acpAgent 430/430, |
Independent local verification (real stack) — head
|
| # | Claim | Base (merge-base) | PR head | Result |
|---|---|---|---|---|
| 1 | Conditional close lets a running turn settle; refusal is non-destructive | closed:true in 34ms — in-flight turn aborted, tool call never ran, session destroyed |
closed:false, holds:[shell/background-shells] after 1712ms — turn settled to end_turn, generation never aborted, bg shell alive, session usable |
✅ fixed |
| 2 | Both drain phases share one budget; timeout reports the budget, releases the gate, touches nothing | closed:true in 12ms (turn aborted, session gone) |
error Session close timed out after 1500ms at 1505ms; turn finished afterwards; session retained & usable |
✅ fixed |
| 3 | Deferred spawn-owner kill completes on final detach even with an incomplete (v1) child | session leaks forever — sessions=1 for the full 20s observation, no close frame ever sent |
sessions=1 → 0 within 1s of the final detach; forced qwen/control/session/close {drainTimeoutMs:8000} on the wire |
✅ fixed |
Leg 1 — conditional close vs a real --acp child
Driver speaks raw ACP over stdio to the built dist/cli.js --acp (fake OpenAI backend from integration-tests/fake-openai-server.ts), mirroring the daemon's initialize meta (qwen.daemon.activeWorkHeartbeat with all three categories). Scenario: session/prompt starts a turn; while the model response is still streaming (hold set empty), the driver sends qwen/control/session/close {onlyIfUnheld:true}; the turn then settles into a run_shell_command {is_background:true} hold.
- Base takes the destructive path this PR removes: it aborts the in-flight generation immediately (
stopReason:cancelled), the tool call never executes, the re-check sees no holds, and the "conditional" close destroys the session (closed:truein 34ms; follow-up prompt →Session not found). This is the feat(daemon): Track background shells in activeWork #9042 post-merge race, observable on a real child. - PR waits for the natural settle under the close gate (model request completes, background shell spawns and registers the aggregate
shell/background-shellshold, continuation runs), then refuses withclosed:false+ the hold set. The backgroundsleepPID is alive, the retained session accepts a follow-up prompt, and the fake-server ledger shows the full turn (initial + continuation) with zero aborted requests. - Budget arm (
drainTimeoutMs:1500, settle takes ~4000ms): PR rejects at 1505ms withSession close timed out after 1500ms— the shared budget in the message, not a phase-2 residue — releases the close gate, and provably leaves the turn running (it completes withend_turn~3.7s later; session stays usable). Base again just kills the turn and closes.
Leg 2 — deferred spawn-owner kill, real daemon × real v1 child
qwen serve (PR/base bundle) with QWEN_CLI_ENTRY pointed at a wire-tee wrapper around the npm 0.21.11 CLI — an authentic pre-#9042 child whose initialize response negotiates activeWork with categories:['agent','notification'] only (captured on the wire; /health?deep=1 grades it activeWorkReporting:"partial"). To plant the tombstone deterministically through a real surface, client A restores a persisted session over /acp WS and drops the connection mid-restore (the wrapper delays the child's session/load response by 3s), so the dispatch's teardown-race guard runs killSession({requireZeroAttaches:true}) while client B's coalesced restore holds an attach — exactly the deferred-kill tombstone. B then detaches.
- Base: after B (the final attacher) detaches,
entryIsAutoCloseCandidatebails on the incomplete category report before the tombstone check — the deferred kill never fires.sessions=1for the entire 20s observation window and no close frame is ever sent to the child: the Session and its child-side state leak indefinitely. - PR: the tombstone check now runs first with the
byId-identity /closing/activeWorkCloseInFlightguards. Within 1s of B's detach the daemon logscompleting deferred kill of session … (last_client_detached), sends a forcedqwen/control/session/close {drainTimeoutMs:8000}(=sessionCloseDrainBudgetMs(10000), verified on the wire), and/health?deep=1drops tosessions=0.
Unit suites & mutation checks
packages/acp-bridge:vitest run src/bridge.test.ts→ 673/673 passed on PR head.packages/cli: the focused conditional-close tests → 4/4 passed.- Mutation check (do the new tests actually pin the fix?): restoring the base
acpAgent.tsunder the PR's test file turns all 4 new child-side tests red (settle-first re-check, shared budget, settle-phase timeout, no-holds completion); restoring the basebridge.tsturns bothhonors a deferred spawn-owner kill for an incomplete childandspares the channel when a kill meets a definitive close refusalred. Files restored to PR state afterwards. The escalation-guard test (does not escalate a deferred kill while a force close is in flight) passes on base too — expected: on base the candidacy gate shadowed that path, so it pins a regression the reordering could otherwise have introduced.
Review notes (non-blocking)
211d0cba07closed the one gap I had flagged while reading618717f7: the exclusive-history-mutation wait now shares the conditional close's deadline instead of getting a fresh full budget, keeping the child round trip under the daemon's 10s outer wait (base's probe sent nodrainTimeoutMs, so the child defaulted toSESSION_DRAIN_TIMEOUT_MS = 30s— an inversion where the daemon always timed out first; the 8s budget fixes that inversion at the source).70cc7b2dca(kill meeting a definitive close refusal no longer channel-kills siblings) is the right fail-closed direction: a structured JSON-RPC error proves the child is alive and kept the session, while daemon-side timeouts still escalate. Its test goes red on base bridge.ts (see mutation check); I did not build a separate e2e for this path.- Side observation, pre-existing and unrelated to this PR: I could not trigger the
POST /sessiondisconnect-reaper (!res.writableafter spawn) on Node 24.18.1/macOS — after a client-side TCP reset mid-spawn,ServerResponse.destroyedflips totruebutres.writablestaystrue(verified with a minimalnode:httpprobe), and the daemon never logssession reaped (client disconnected before response). That's why Leg 2 plants the tombstone through the/acprestore-race guard instead. Might be worth a follow-up issue to re-check that signal on current Node.
Not covered
Windows/Linux (macOS only, matching the PR's tested matrix), channel liveness/watchdog behavior (out of scope per the PR), and the probe-in-flight tombstone deferral, which I verified at unit level only.
Screenshots are rendered from the recorded run artifacts (verdict JSONs, wire logs, vitest output) via the repo's terminal-capture harness; all timings and values are from the actual runs. Evidence branch: pr-assets/9134-verify (added alongside the CI run's assets, not replacing them).
中文版本（Chinese version）
独立本地验证（真实栈）— head 70cc7b2dca
我在本地构建了本 PR，并用真实进程端到端验证了两个生命周期修复——不 mock Session、不 mock bridge：child 侧修复用真实 --acp 子进程 + 原始 NDJSON JSON-RPC 驱动 + 脚本化 OpenAI 兼容后端；daemon 侧修复用真实 qwen serve 搭配真实 v1 child（npm @qwen-code/qwen-code@0.21.11，发布于 #9042 合入之前）。所有场景均做 A/B 对照：base bundle 在 merge-base（3186d4ea67，仅还原 5 个 PR 文件）构建，PR bundle 在 70cc7b2dca 构建。环境：macOS（Darwin 25.6.0）、Node.js v24.18.1。
结论：两个修复与 PR 声明一致，所声称的竞态在 base 上可复现、在 PR 构建上消失，新增测试确实钉住了行为（在 base 源码上会失败）。我这边 LGTM——证据见下。
| # | 声明 | Base（merge-base） | PR head | 结果 |
|---|---|---|---|---|
| 1 | 条件关闭让运行中的 turn 自然结束；拒绝时零破坏 | 34ms 内 closed:true——在途 turn 被中止，工具调用未执行，会话被销毁 |
1712ms 后 closed:false, holds:[shell/background-shells]——turn 自然 end_turn，生成从未被中止，后台 shell 存活，会话可用 |
✅ 已修复 |
| 2 | 两个 drain 阶段共享预算；超时报共享预算、释放 gate、不碰任何东西 | 12ms 内 closed:true（turn 被中止，会话消失） |
1505ms 时报错 Session close timed out after 1500ms；turn 之后照常完成；会话保留且可用 |
✅ 已修复 |
| 3 | 最后一个 attacher 离开时，即使 child 上报类别不完整（v1），延迟的 spawn-owner kill 也会完成 | 会话永久泄漏——20 秒观察窗内 sessions=1，从未向 child 发送 close 帧 |
B detach 后 1 秒内 sessions=1 → 0；线上可见强制 qwen/control/session/close {drainTimeoutMs:8000} |
✅ 已修复 |
Leg 1 — 条件关闭 vs 真实 --acp child
驱动脚本通过 stdio 原始 ACP 协议直连构建产物 dist/cli.js --acp（模型后端用仓库自带 integration-tests/fake-openai-server.ts），initialize meta 完整复刻 daemon（qwen.daemon.activeWorkHeartbeat 三类别）。场景：session/prompt 启动 turn；模型响应仍在途、hold 集合为空时，发送 qwen/control/session/close {onlyIfUnheld:true}；随后 turn 在结算中通过 run_shell_command {is_background:true} 登记 shell hold。
- Base 走的正是本 PR 移除的破坏性路径：立即中止在途生成（
stopReason:cancelled），工具调用未执行，复查无 hold，"条件"关闭销毁了会话（34ms 内closed:true；后续 prompt 报Session not found）。这就是 feat(daemon): Track background shells in activeWork #9042 合并后审查发现的竞态，在真实 child 上可观测。 - PR 在 close gate 下等待自然结算（模型请求完成、后台 shell spawn 并登记聚合 hold
shell/background-shells、continuation 正常执行），然后以closed:false+ hold 集合拒绝。后台sleep进程存活，保留的会话可继续接受 prompt，fake-server 台账显示完整 turn（initial + continuation）且零中止请求。 - 预算臂（
drainTimeoutMs:1500，settle 需约 4000ms）：PR 在 1505ms 时报Session close timed out after 1500ms——错误信息报的是共享预算而非 phase-2 残量——释放 close gate，且 turn 确实未被打扰（约 3.7 秒后以end_turn完成；会话仍可用）。Base 则依旧直接杀掉 turn 并关闭。
Leg 2 — 延迟 spawn-owner kill，真实 daemon × 真实 v1 child
qwen serve（PR/base bundle）通过 QWEN_CLI_ENTRY 指向包裹 npm 0.21.11 CLI 的 wire-tee 包装器——货真价实的 pre-#9042 child，其 initialize 响应只协商 categories:['agent','notification']（线上抓包证实；/health?deep=1 相应显示 activeWorkReporting:"partial"）。为了在真实面上确定性地种下 tombstone：客户端 A 经 /acp WS 恢复一个持久化会话并在恢复中途断开连接（包装器把 child 的 session/load 响应延迟 3 秒），dispatch 的 teardown-race 守卫随即执行 killSession({requireZeroAttaches:true})，而客户端 B 的合流恢复正持有一个 attach——正是延迟 kill 的 tombstone。随后 B detach。
- Base：最后一个 attacher（B）离开后，
entryIsAutoCloseCandidate在 tombstone 检查之前就因类别上报不完整而返回——延迟 kill 永远不会执行。整个 20 秒观察窗内sessions=1，且从未向 child 发送任何 close 帧：会话与 child 侧状态无限期泄漏。 - PR：tombstone 检查现在先行，并带
byId身份 /closing/activeWorkCloseInFlight守卫。B detach 后 1 秒内 daemon 打出completing deferred kill of session … (last_client_detached)，发送强制qwen/control/session/close {drainTimeoutMs:8000}（=sessionCloseDrainBudgetMs(10000)，线上抓包证实），/health?deep=1降为sessions=0。
单测与变异验证
packages/acp-bridge：vitest run src/bridge.test.ts→ PR head 上 673/673 全过。packages/cli：条件关闭焦点测试 → 4/4 全过。- 变异验证（新测试是否真的钉住修复？）：在 PR 测试文件下还原 base 版
acpAgent.ts，4 个新增 child 侧测试全部转红（settle 后复查、共享预算、settle 阶段超时、无 hold 时正常完成）；还原 base 版bridge.ts，honors a deferred spawn-owner kill for an incomplete child与spares the channel when a kill meets a definitive close refusal双双转红。之后均已还原为 PR 状态。护栏测试does not escalate a deferred kill while a force close is in flight在 base 上也通过——符合预期：base 上候选判定门挡住了该路径，它钉住的是本次重排可能引入的回归。
审查备注（不阻塞）
211d0cba07补上了我读618717f7时标记的缺口：exclusive-history-mutation 等待现在共享条件关闭的截止时间，而不是再拿一份完整预算，保证 child 往返压在 daemon 10 秒外层等待之内（base 的探测根本不带drainTimeoutMs，child 默认SESSION_DRAIN_TIMEOUT_MS = 30s——daemon 必先超时的倒挂；8 秒预算从源头修正了这一倒挂）。70cc7b2dca（kill 遇到 definitive 拒绝不再 channel-kill 连坐兄弟会话）方向正确：结构化 JSON-RPC 错误证明 child 存活且保留了会话，而 daemon 侧超时仍会升级。其测试在 base 版 bridge.ts 上转红（见变异验证）；此路径我未单独做 e2e。- 旁路观察（先于本 PR 存在、与本 PR 无关）：在 Node 24.18.1/macOS 上无法触发
POST /session的断连收割（spawn 后的!res.writable）——客户端在 spawn 中途 TCP reset 后，ServerResponse.destroyed变为true但res.writable保持true（用最小node:http探针证实），daemon 从不打印session reaped (client disconnected before response)。这也是 Leg 2 改走/acprestore-race 守卫种 tombstone 的原因。或许值得开个 follow-up issue 在当前 Node 上复核该信号。
未覆盖
Windows/Linux（仅 macOS，与 PR 的测试矩阵一致）、channel 存活性/watchdog 行为（PR 声明范围外）、以及 probe-in-flight 的 tombstone 延迟——后者仅在单测层面验证。
截图由仓库自带 terminal-capture 基建从录制的运行工件（verdict JSON、wire 日志、vitest 输出）渲染而来；所有时间与数值均来自真实运行。证据分支：pr-assets/9134-verify（追加在 CI run 资产之后，未覆盖）。
|
@qwen-code /triage |
|
Sandboxed verification: ✅ passed — merge-ready (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 22 passed · 0 failed · 22 total 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：22 通过 · 0 失败 · 22 总计 Verification report<!-- qwen-triage:verify --> Sandboxed verification: ✅ passed — merge-ready (agent verdict) - follow-up round at head Ran the PR in an isolated, token-free container: A/B against the base build (re-measured at the new head), mock-free harness assertions, mutation matrix, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 22 passed · 0 failed · 22 total 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证容器中执行（与 base 构建 A/B 对照、无 mock harness、变异矩阵、定向门禁），本轮为 follow-up：所有上轮测量均在新 head 脚本断言：22 通过 · 0 失败 · 22 总计 Verification reportVerification — PR #9134
|
| # | finding (round 1) | severity | status at new head |
|---|---|---|---|
| 1 | identity guard byId.get(...)===entry in the deferred-kill branch not pinned by any test (mutation C survived) |
Suggestion (non-blocking) | stands — re-ran as M2: dropping the guard leaves all 5 focused bridge tests green. Still a coverage gap, not dead code (the guard protects the session/load re-registration race documented in closeIfChildUnheld's identity comment; no focused test constructs that race). I agree with the original non-blocking assessment. |
| 2 | the two escalation-guard tests also pass on base (candidacy gate coincidentally covered them) | Note (expected shape) | stands — re-measured: both green on both arms (control rows in 01-ab-cells-base-vs-head.png). Load-bearing nature now proven by the consolidated predicate mutation M1 (dropping !isClosingOrAuthorizingClose(entry) turns exactly those two tests red). |
| 3 | description's "642 tests" stale vs measured 671 | Correction (minor) | stands, still stale — measured at new head: bridge bridge.test.ts 673/673, cli acpAgent.test.ts 430/430 (+2/+1 vs the old head, matching the two commits' new tests). The body still says 642. |
Central claims & A/B (re-measured at the new head)
Claim 1 (child, closeStoredSession reorder): a refused conditional close leaves the retained Session untouched (no generation abort, no cancelPendingPrompt, no dispose, gate released), with one shared deadline across the settle phase, the destructive drain, and the history-mutation wait.
Claim 2 (daemon, maybeCloseIdleSession reorder): a deferred spawn-owner kill completes for an incomplete (legacy-category) child once the last attacher detaches, guarded against in-flight probes and closes.
Claim 3 (delta, 211d0cba): drain budgets key off the outer wait actually applied (opts?.timeoutMs ?? initTimeoutMs), in one place (sessionCloseDrainBudgetMs).
Claim 4 (delta, 70cc7b2d): killSession tolerates a definitive close refusal (child holding its own gate) — resets entry.closing, returns false, no channel kill.
| Scenario | base | head | oracle | kind |
|---|---|---|---|---|
| deferred kill completes for legacy-category child | red (sessionCount stays 1) | green | honors a deferred spawn-owner kill for an incomplete child |
flip |
| kill meets definitive refusal → channel spared, kill retryable | red (channel killed) | green | spares the channel when a kill meets a definitive close refusal |
flip (claim 4) |
conditional probe carries drainTimeoutMs=8000 |
red (param absent) | green | asks the child about a session it has never reported on |
flip |
| refusal leaves turn un-aborted, un-cancelled, gate released | red | green | rechecks local holds after active turns drain during conditional close |
flip |
| one shared budget: 1000 ms = 600 ms settle + 400 ms destructive | red (resolved instead of timing out) | green | shares one timeout budget across conditional close drain phases |
flip |
| settle phase capped by shared budget, gate released | red (cancelPendingPrompt called) |
green | rejects a conditional close when the settle phase exceeds the shared budget |
flip |
| abort ordered after first settle on the success path | red (abort before settle) | green | completes a conditional close when no holds appear at any re-check |
flip |
| tombstoned kill deferred while probe in flight | green | green | defers a tombstoned kill while a conditional close probe is in flight |
control (load-bearing via M1) |
| deferred kill deferred while force close in flight | green | green | does not escalate a deferred kill while a force close is in flight |
control (load-bearing via M1) |
| restore-cleanup + sibling close budgets (helper refactor, value unchanged) | green | green | quarantines only fresh work... / preserves bridge state... |
control |
| helper unit pin | n/a (head-only) | green | stays strictly under the outer wait and clamps to >=1ms |
control (load-bearing via M3/M4) |
| pre-category v1 baseline refusal | green | green | uses the pre-category v1 baseline when initialize omits categories |
control |
Witness: 01-ab-cells-base-vs-head.png (7/7 flips, 6/6 controls).
Delta keying probe (02-keying-probe-ab.png): with initializeTimeoutMs: 30_000 and a condemned channel (applied wait 10 s), the sibling close at head sends drainTimeoutMs: 8000; at base it sent 24000 (Math.floor(30000*0.8)) — the exact 24 s-vs-10 s mismatch the round-2 commit describes. No repo test pins this (see Findings #2); the probe is a scratch test kept in harness/probe-test.txt, not part of the PR.
Mutation matrix at head — 03-mutation-matrix.png
| Mutation | outcome | failing oracle(s) |
|---|---|---|
M1 drop !isClosingOrAuthorizingClose(entry) from deferred-kill branch |
KILLED | both escalation-guard tests (in-flight probe → forcedCloseCalls 1; in-flight force close → handle.killed true) |
M2 drop byId.get(...)===entry identity guard |
SURVIVED (all 5 focused green) | — (carry-over finding #1) |
| M3 helper ratio 0.8 → 1.0 | KILLED | helper unit test |
| M4 drop ≥1 ms clamp | KILLED | helper unit test |
M5 drop isDefinitiveAcpRequestError tolerance in killSession |
KILLED | spares the channel... (expected true to be false on handle.killed) |
| M6 drop onlyIfUnheld phase-1 settle + re-check block | KILLED | all three conditional-close tests (abort called early; close resolved instead of rejecting; abort order 190 < settle 192) |
Positive controls: unmutated head 13/13 green (head arm); every red above is a behavioral assertion failure, quoted from the JSON logs.
Findings (ordered by severity)
- Suggestion (non-blocking, carry-over) — identity guard unpinned. Re-measured as M2 at the new head: survives. Coverage gap, not dead code (see status table). The round-2 commit swapped the two inline guards for the canonical
isClosingOrAuthorizingClosebut did not add a re-registration-race test; the gap is unchanged. - Suggestion (non-blocking, new) — the drain-budget keying fix has no repo test. The
opts?.timeoutMs ?? initTimeoutMskeying (round-2's headline fix) is only observable wheninitTimeoutMsdiffers from the applied wait; every existing assertion runs with both at 10 s (or both at 20 ms), where base and head compute the same value — which is exactly whyquarantines only fresh work...passes on both arms. My probe (harness/probe-test.txt) withinitializeTimeoutMs: 30_000is the fixture that would pin it (base red / head green). Recommend landing it. Related minor: the conditional path's history-mutation wait now receives the shared-budget residue, but the only mutation-gate test (times out close while a branch holds the history mutation gate) exercises the unconditional path, so the residue on that branch is also unpinned. Neither is a defect — both are completeness notes.
Not covered
- Per-commit attribution. Depth-2 shallow checkout: only the merge, base tip, and PR head are reachable; the four commits in the snapshot's
commitsarray are not individually exercisable. Verified the aggregateHEAD^1..HEADdiff. - Real end-to-end daemon↔child over sockets. Harnesses are the PR's own vitest harnesses (in-memory channel for the bridge; mocked
Sessioncollaborator for the child). The A/B proves the changed logic, not the live wire. - Full-repo build/typecheck. Blocked per the PR by a pre-existing shared-dependency resolution issue; package-level gates ran instead.
- Windows/Linux, channel liveness / Agent watchdog / shell-stall / retry — out of scope per the PR.
- The history-mutation wait body stays untimed by design (author states the guarantee is approximate).
Methodology
Environment: node:22-bookworm CI verify container; npm ci + npm run build already done at head. A/B cells ran the PR's focused vitest tests against (a) head and (b) a base cell built by restoring the HEAD^1 copy of the single changed unit (bridge.ts or acpAgent.ts) while the rest of the tree — including the test files and bridgeTypes.ts — stayed at head; valid because the depth-2 checkout means base and head differ only by this PR. The keying probe appended a scratch test to a copy of bridge.test.ts and ran both arms; the file was restored afterwards. Mutations applied exact string replacements, ran focused oracles, and git checkout-restored (verified clean after each). Gates: full bridge.test.ts 673/673 and acpAgent.test.ts 430/430, exit 0. Working tree left clean. Raw logs in logs/, harnesses in harness/, captures in evidence/.
Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention).
— Qwen Code · sandboxed verification
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| if (isDefinitiveAcpRequestError(error)) { | ||
| entry.closing = false; | ||
| return false; | ||
| } |
There was a problem hiding this comment.
[Critical] R4-1: The refusal branch resets entry.closing and returns false but arms no tombstone — and on the direct path none exists: spawnOwnerWantedKill is only written in the requireZeroAttaches && attachCount > 0 bail above (bridge.ts:11167), and spawn-owner registrations carry no attach ref, so a direct kill at attachCount === 0 (the common spawn-owner disconnect / orphan-cleanup shape) that meets a definitive refusal returns false to callers that are all one-shot and best-effort — nothing retries. maybeCloseIdleSession's deferred branch requires spawnOwnerWantedKill; the reaper/auto-close paths are vetoed by entryIsAutoCloseCandidate for held-work and incomplete-reporting children; the channel idle timer needs an empty session set. The comment above — "the deferred tombstone completes it on the next settle event" — describes a mechanism that does not exist on this path. — Failure scenario: a cleanup kill arrives while the child holds its own close gate (cd or restore in flight — the routine state this tolerance was written for) → the child answers 'Session close is already in progress' → killSession returns false and nothing retries → on a version-skewed or held-work child the session entry, its child process, and the persisted archive copy linger until daemon restart. Pre-PR this window escalated to a whole-channel kill (worse), so the tolerant direction is right — the missing tombstone leaves it incomplete.
Witness (bridge-harness probe: incomplete-reporting child, direct killSession({requireZeroAttaches: true}) refused, then gate released + idle snapshot):
PR side: sessionCount stays 1 — nothing completes the kill
with entry.spawnOwnerWantedKill = true added to this branch:
sessionCount 0 on the next settle event; existing
refusal / deferred-kill tests still pass
| if (isDefinitiveAcpRequestError(error)) { | |
| entry.closing = false; | |
| return false; | |
| } | |
| if (isDefinitiveAcpRequestError(error)) { | |
| if (opts?.requireZeroAttaches) entry.spawnOwnerWantedKill = true; | |
| entry.closing = false; | |
| return false; | |
| } |
(If dropping direct kills on refusal is intended instead, scope the comment to the tombstone case.)
中文说明
该拒绝分支重置 entry.closing 并返回 false，但没有立 tombstone——而直接路径上本来就没有 tombstone：spawnOwnerWantedKill 只在上方 requireZeroAttaches && attachCount > 0 的 bail 分支（bridge.ts:11167）中写入，且 spawn-owner 注册不携带 attach 引用，因此 attachCount === 0 的直接 kill（常见的 spawn-owner 断开/孤儿清理形态）遇到明确拒绝时，只会把 false 返回给所有一次性、尽力而为的调用方——没有任何路径会重试。maybeCloseIdleSession 的延迟分支要求 spawnOwnerWantedKill；对上报 hold 或类别不完整的 child，reaper/自动关闭路径被 entryIsAutoCloseCandidate 否决；channel 空闲定时器要求 session 集合为空。上方注释“延迟 tombstone 会在下一个 settle 事件完成 kill”描述的机制在这条路径上并不存在。失败场景：cleanup kill 到达时 child 正持有自己的 close gate（cd 或 restore 进行中——正是该容忍机制要处理的常态）→ child 回复 'Session close is already in progress' → killSession 返回 false 且无人重试 → 对版本错位或仍上报 hold 的 child，session 条目、child 进程与持久化存档会滞留到 daemon 重启。PR 之前该窗口会升级为整 channel kill（更糟），因此容忍方向是对的——缺的是 tombstone，使修复不完整。
证据（bridge 测试架探针：不完整上报 child，直接 killSession({requireZeroAttaches: true}) 被拒绝后释放 gate 并发送空闲快照）：PR 侧 sessionCount 保持 1（无人完成 kill）；在本分支加入 entry.spawnOwnerWantedKill = true 后，下一个 settle 事件 sessionCount 归 0，且现有拒绝/延迟 kill 测试仍通过。
建议修复：在拒绝分支中（至少当 opts?.requireZeroAttaches 时）同时置 entry.spawnOwnerWantedKill = true，让下一个 settle 事件经由既有延迟分支完成 kill；若直接 kill 被拒绝后丢弃是有意语义，则把注释限定到 tombstone 场景。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| timeoutMs: initTimeoutMs, | ||
| }); | ||
| } catch (error) { | ||
| // A definitive refusal means the child is alive and kept the session: |
There was a problem hiding this comment.
[Critical] R4-2: This comment's premise is wider than the predicate below it. isDefinitiveAcpRequestError (bridge.ts:300) accepts any record with an integer code + string message — but the ACP SDK flattens every child-thrown error into that same shape, so this branch tolerates not only the gate-held refusal it was written for but also child drain timeouts and close-handler crashes. Verified over the real wire: a child drain timeout (a plain Error from waitForSessionDrain) is wrapped by the SDK as {code: -32603, message: 'Internal error', data: {details: 'Session close timed out after 8000ms'}}, and the predicate accepts it. A drain timeout is a failed close with an unknown outcome — not a refusal — and the budget coupling makes this the norm: sessionCloseDrainBudgetMs guarantees the child deadline fires before the daemon's outer wait, so every child-side close failure arrives as a "definitive" record; the unknown-outcome escalation the helper docstring describes is unreachable for exactly the failures the budget exists to surface. Compounds R4-1: a misclassified timeout on the direct path arms no tombstone either. — Failure scenario: spawn-owner disconnect (attachCount === 0) → reaper kill → the turn is cancelled but slow to settle, so the child's close consumes its full drain budget and times out → the timeout is tolerated as a refusal → nothing retries → on an incomplete-category reporter the session persists until daemon restart (pre-PR: channel kill).
Witness (probe over the real bridge + real AgentSideConnection wire):
child drain timeout (plain Error) → observed record
{"code":-32603,"message":"Internal error",
"data":{"details":"Session close timed out after 8000ms"}} predicate=true
PR arm: killSession=false channelKilled=false sessionCount=1
FLIP A (branch reverted = pre-PR): channelKilled=true sessionCount=0
FLIP B (predicate excludes -32603): timeout escalates (channelKilled=true)
while the production refusal still tolerates
(killSession=false, sessionCount=1)
Suggested fix: classify the actual refusal, not the record shape — e.g. have Session.beginClose's refusal carry a marker matched in data (RequestError.invalidParams({errorKind: 'close_in_progress'}, …)), or restrict this call site to the -32602 refusal and let -32603/internal errors take the unknown-outcome escalation. Combined with R4-1's tombstone, a tolerated failure still completes later instead of leaking.
中文说明
该注释的前提比下方谓词更宽。isDefinitiveAcpRequestError（bridge.ts:300）接受任何带整数 code + 字符串 message 的记录——但 ACP SDK 会把 child 抛出的所有错误压成同一种记录形状，因此这个分支容忍的不仅是它要处理的 gate 持有拒绝，还包括 child drain 超时与 close 处理器崩溃。已在真实链路上验证：child drain 超时（waitForSessionDrain 抛出的普通 Error）被 SDK 包成 {code: -32603, message: 'Internal error', data: {details: 'Session close timed out after 8000ms'}}，谓词同样接受。drain 超时是一次结果未知的失败关闭，不是拒绝；而且预算耦合使这成为常态：sessionCloseDrainBudgetMs 保证 child 截止先于 daemon 外层等待触发，因此 child 侧每一次关闭失败都会以“明确”记录的形态到达——helper 文档所述的“结果未知→升级”路径对预算机制本来要呈现的失败恰好不可达。与 R4-1 叠加：被误分类的超时在直接路径上同样不会立 tombstone。失败场景：spawn-owner 断开（attachCount === 0）→ reaper kill → turn 已取消但 settle 缓慢，child 关闭耗尽全部 drain 预算后超时 → 超时被当作拒绝容忍 → 无人重试 → 对类别上报不完整的 child，session 滞留至 daemon 重启（PR 之前：channel kill）。
证据（真实 bridge + 真实 AgentSideConnection 链路探针）：child drain 超时到达的记录为 {code:-32603, 'Internal error', details:'Session close timed out after 8000ms'}，predicate=true；PR 侧 killSession=false channelKilled=false sessionCount=1；回退本分支（=PR 前）channelKilled=true；谓词排除 -32603 后超时升级（channelKilled=true）且生产拒绝仍被容忍（killSession=false，sessionCount=1）。
建议修复：按真实拒绝分类而非记录形状——例如让 Session.beginClose 的拒绝携带可在 data 中匹配的标记（RequestError.invalidParams({errorKind: 'close_in_progress'}, …)），或将本调用点限定为 -32602 拒绝、让 -32603/内部错误走结果未知升级；与 R4-1 的 tombstone 结合后，被容忍的失败仍能稍后完成而不是泄漏。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| throw new RequestError( | ||
| -32603, | ||
| 'Session close is already in progress', | ||
| ); |
There was a problem hiding this comment.
[Suggestion] R4-3: This fake throws new RequestError(-32603, 'Session close is already in progress'), but a production refusal is RequestError.invalidParams → over the wire {code: -32602, message: 'Invalid params: Session close is already in progress'} (Session.ts:3017, observed by probe). The predicate is code-agnostic today, so both shapes pass — but this test's oracle is wired to bytes production never sends: a natural R4-2 fix that narrows the predicate to the shape this test pins ships green while production refusals fall through to killChannelWithLog — the exact sibling-session cascade this PR exists to prevent. — Failure scenario: the classifier is narrowed to match this test's bytes → production refusals escalate to a whole-channel kill while the suite stays green. Probe reproduced exactly this: predicate narrowed to the test's shape → production-refusal arm killSession=true channelKilled=true sessionCount=0 while this test still passes (1 passed, 672 skipped).
Witness:
production refusal observed on the wire:
{"code":-32602,"message":"Invalid params: Session close is already in progress"}
narrowed to this test's shape: production refusal escalates to
channel kill; this test stays green
| throw new RequestError( | |
| -32603, | |
| 'Session close is already in progress', | |
| ); | |
| throw RequestError.invalidParams( | |
| undefined, | |
| 'Session close is already in progress', | |
| ); |
(and add one -32603/internal-error case asserting the escalation side — the R4-2 boundary)
中文说明
该 fake 抛出 new RequestError(-32603, 'Session close is already in progress')，但生产环境的拒绝是 RequestError.invalidParams → 线上字节为 {code: -32602, message: 'Invalid params: Session close is already in progress'}（Session.ts:3017，探针实测）。当前谓词与 code 无关，两种形状都能通过——但本测试的 oracle 绑定的是生产从不发送的字节：一个自然的 R4-2 修复若把谓词收窄到本测试钉住的形状，会绿灯放行，而生产拒绝将落入 killChannelWithLog——正是本 PR 要消除的 sibling session 连带 kill。失败场景：分类器按本测试的字节收窄 → 生产拒绝升级为整 channel kill，测试套件仍全绿。探针已精确复现：谓词收窄到测试形状后，生产拒绝分支 killSession=true channelKilled=true sessionCount=0，而本测试仍通过（1 passed, 672 skipped）。
建议修复：让 fake 按线上真实形态拒绝——抛出 Session.beginClose 实际抛出的 RequestError.invalidParams(undefined, 'Session close is already in progress')，并新增一个 -32603/内部错误用例断言升级侧（即 R4-2 的边界）。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| refuseClose = false; | ||
| await expect(bridge.killSession(owner.sessionId)).resolves.toBe(true); | ||
| expect(bridge.sessionCount).toBe(0); |
There was a problem hiding this comment.
[Suggestion] R4-4: The retry phase here does not pin the entry.closing = false reset it claims to verify: resolves.toBe(true) + sessionCount 0 are identical on the intended clean-retry path and on killSession's closing-branch channel-kill path (which also kills the channel and returns true), and handle.killed is never asserted after the retry. — Failure scenario: delete entry.closing = false; from the refusal branch → the second kill enters the closing branch and channel-kills — resolves.toBe(true) and sessionCount 0 still hold, so the mutant's detection depends on microtask timing of the channel-exit reap rather than a deliberate assertion. A regression latching refused entries closing-true turns every later kill of that session into a whole-channel kill (siblings included) with this test green on timing-favorable runs.
Witness (mutation run, entry.closing = false deleted — test still passes in both arms):
baseline stderr: channel exited (..., 0 session(s) torn down) — clean retry
mutant stderr: channel exited (..., 1 session(s) torn down) — retry was a channel kill
| refuseClose = false; | |
| await expect(bridge.killSession(owner.sessionId)).resolves.toBe(true); | |
| expect(bridge.sessionCount).toBe(0); | |
| refuseClose = false; | |
| await expect(bridge.killSession(owner.sessionId)).resolves.toBe(true); | |
| expect(handle.killed).toBe(false); | |
| expect(bridge.sessionCount).toBe(0); |
中文说明
此处的重试阶段并未钉住它声称要验证的 entry.closing = false 复位：resolves.toBe(true) + sessionCount 0 在预期的干净重试路径与 killSession 的 closing 分支 channel-kill 路径上完全相同（后者同样杀掉 channel 并返回 true），且重试之后从未断言 handle.killed。失败场景：从拒绝分支删除 entry.closing = false; → 第二次 kill 进入 closing 分支并杀掉整个 channel——resolves.toBe(true) 与 sessionCount 0 依然成立，突变体是否被发现取决于 channel 退出回收的微任务时序而非刻意断言。若回归使被拒绝的 entry 锁死在 closing=true，该 session 之后每次 kill 都会变成整 channel kill（连带 sibling），而本测试在时序有利的运行中仍是绿的。
证据（突变运行：删除 entry.closing = false 后测试仍通过）：基线 stderr 'channel exited (..., 0 session(s) torn down)'（干净重试）；突变体 stderr 'channel exited (..., 1 session(s) torn down)'（重试实为 channel kill）。
建议修复：在第二次 killSession 后补 expect(handle.killed).toBe(false)（可选再断言记录了第二次 sessionClose ext 调用），证明成功重试走的是干净关闭路径。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| drainTimeoutMs: sessionCloseDrainBudgetMs( | ||
| opts?.timeoutMs ?? initTimeoutMs, | ||
| ), |
There was a problem hiding this comment.
[Suggestion] R4-5: This keying fix (round-2 R2-1) has no discriminating test: every close-path test runs a bridge whose initTimeoutMs is the 10_000 default — equal to the only opts.timeoutMs ever applied (ACTIVE_WORK_CLOSE_TIMEOUT_MS on the condemned-channel path) — so the two arms of ?? are observationally identical across the suite; no test passes agentCloseTimeoutMs and none varies initializeTimeoutMs. — Failure scenario: a future edit reverting the keying to sessionCloseDrainBudgetMs(initTimeoutMs) ships green and reintroduces the R2-1 escalation — with --initialize-timeout-ms 30000 and a quarantined/restore-overdue channel, auto-close tells the child to drain for 24_000ms against a 10_000ms outer wait → the daemon deadline fires first → the unknown-outcome recovery kills the whole channel mid-drain.
Witness (mutation run: keying reverted to sessionCloseDrainBudgetMs(initTimeoutMs); full bridge.test.ts suite): Tests 673 passed (673) — the mutant survives.
Suggested fix: add a bridge test reaching closeIfChildUnheld's condemned path (quarantined channel or restoreSettlementOverdue) with makeBridge({ initializeTimeoutMs: 30_000 }), asserting the sent drainTimeoutMs is sessionCloseDrainBudgetMs(ACTIVE_WORK_CLOSE_TIMEOUT_MS) (8_000), not 24_000.
中文说明
该键控修复（第二轮 R2-1）没有判别测试：所有关闭路径测试使用的 bridge initTimeoutMs 都是默认 10_000——恰好等于唯一会被传入的 opts.timeoutMs（condemned channel 路径上的 ACTIVE_WORK_CLOSE_TIMEOUT_MS）——因此 ?? 的两个分支在整个测试套件中观测上完全相同；没有测试传入 agentCloseTimeoutMs，也没有测试改变 initializeTimeoutMs。失败场景：未来把键控回退为 sessionCloseDrainBudgetMs(initTimeoutMs) 的改动会绿灯放行，并重新引入 R2-1 的升级——--initialize-timeout-ms 30000 且 channel 处于 quarantined/restore 超时状态时，自动关闭会告诉 child 有 24_000ms drain 预算，而外层等待只有 10_000ms → daemon 截止先触发 → 结果未知恢复路径在 drain 中途杀掉整个 channel。
证据（突变运行：键控回退为 sessionCloseDrainBudgetMs(initTimeoutMs) 后，bridge.test.ts 全套件 'Tests 673 passed (673)'——突变存活）。
建议修复：新增 bridge 测试，以 makeBridge({ initializeTimeoutMs: 30_000 }) 到达 closeIfChildUnheld 的 condemned 路径（quarantined channel 或 restoreSettlementOverdue），断言发送的 drainTimeoutMs 为 sessionCloseDrainBudgetMs(ACTIVE_WORK_CLOSE_TIMEOUT_MS)（8_000）而非 24_000。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| } | ||
| }); | ||
|
|
||
| it('spares the channel when a kill meets a definitive close refusal', async () => { |
There was a problem hiding this comment.
[Suggestion] R4-6: Only the true side of isDefinitiveAcpRequestError is tested at the killSession site: no test drives a non-definitive close failure — a plain Error from the child's sessionClose handler, or a wedged child hitting withTimeout's BridgeTimeoutError (no integer code) — through killSession, so the escalation the classifier guards (killChannelWithLog) is unpinned at this call site. — Failure scenario: mutate the predicate to always-true (tolerate every close failure) → killSession against a wedged child returns false and leaves the kill pending instead of killing the channel; for a child that can never settle the session lingers indefinitely and the channel is never reaped — defeating kill's force semantics. Verified: the mutant ships green (Tests 673 passed (673)).
Witness (mutation run: if (isDefinitiveAcpRequestError(error)) → always-true; full bridge.test.ts suite): Tests 673 passed (673) — the mutant survives.
Suggested fix: add a bridge test where the channel answers sessionClose with throw new Error('close failed') (or never resolves under a short initializeTimeoutMs), asserting killSession resolves true and handle.killed is true — pinning the escalation side of the classifier.
中文说明
killSession 处只测试了 isDefinitiveAcpRequestError 的 true 侧：没有测试把非明确关闭失败——child sessionClose 处理器抛出的普通 Error，或卡死 child 触发 withTimeout 的 BridgeTimeoutError（无整数 code）——送入 killSession，因此分类器所守护的升级（killChannelWithLog）在该调用点没有被钉住。失败场景：把谓词突变为恒真（容忍一切关闭失败）→ 对卡死 child 的 killSession 返回 false 并把 kill 挂起，而不是杀掉 channel；对永远无法 settle 的 child，session 无限期滞留、channel 永不被回收——kill 的强制语义失效。已验证：该突变全套件绿灯（'Tests 673 passed (673)'）。
建议修复：新增 bridge 测试，让 channel 对 sessionClose 抛 new Error('close failed')（或在较短 initializeTimeoutMs 下永不响应），断言 killSession 解析为 true 且 handle.killed 为 true——钉住分类器的升级侧。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| conditionalDrainDeadline === undefined | ||
| ? drainTimeoutMs | ||
| : Math.max(1, conditionalDrainDeadline - Date.now()), |
There was a problem hiding this comment.
[Suggestion] R4-7: The identical remaining-budget expression appears twice in closeStoredSession — as the phase-2 waitForSessionDrain timeout (~4630) and as this runExclusiveHistoryMutation wait timeout — with no shared helper. The new tests pin the phase-2 site, but none exercises this site under timeout pressure (the timeout test rejects before reaching it; the success test resolves instantly), so a mutation at this second site ships green and the two copies can drift exactly where the comment above says the coupling must hold ("or the whole round trip can outlast the daemon's outer wait"). — Failure scenario: a future change to the clamp or fallback (a different minimum than 1, or a fresh budget instead of the residue) is applied to one site only → the mutation wait silently diverges from the phase-2 budget → the mutation wait outlasts the daemon's outer wait on the conditional path — the regression this PR's comments warn about.
Suggested fix:
const remainingDrainBudgetMs = () =>
conditionalDrainDeadline === undefined
? drainTimeoutMs
: Math.max(1, conditionalDrainDeadline - Date.now());declared next to conditionalDrainDeadline and called at both sites — it must stay a function re-evaluated per call, since wall-clock time elapses between the phases.
中文说明
同一个“剩余预算”表达式在 closeStoredSession 中出现两次——phase-2 的 waitForSessionDrain 超时（约 4630 行）与此处 runExclusiveHistoryMutation 的等待超时——且没有共享 helper。新测试钉住了 phase-2 一侧，但没有任何测试在超时压力下验证这一侧（超时测试在到达之前就拒绝了；成功测试立即解析），因此对第二处的突变会绿灯放行，两份副本恰好会在上方注释声明必须保持耦合的位置（“否则整个往返可能超过 daemon 的外层等待”）发生漂移。失败场景：未来对 clamp 或兜底的改动（最小值不再是 1，或用全新预算代替剩余预算）只应用于一处 → mutation 等待悄悄偏离 phase-2 预算 → conditional 路径上 mutation 等待超出 daemon 外层等待——正是本 PR 注释警告的回归。
建议修复：在 conditionalDrainDeadline 旁声明 const remainingDrainBudgetMs = () => conditionalDrainDeadline === undefined ? drainTimeoutMs : Math.max(1, conditionalDrainDeadline - Date.now()); 并在两处调用——它必须是每次调用重新求值的函数，因为两个阶段之间墙钟时间在流逝。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| export function sessionCloseDrainBudgetMs(outerWaitMs: number): number { | ||
| return Math.max(1, Math.floor(outerWaitMs * 0.8)); | ||
| } |
There was a problem hiding this comment.
[Suggestion] R4-8: The docstring invariant — "strictly under the daemon's outer wait so the child deadline always fires first" — does not hold for outerWaitMs === 1: the clamp yields a budget equal to the outer wait (the new unit test even pins sessionCloseDrainBudgetMs(1) === 1 while its strictly-under loop starts at 2). initializeTimeoutMs is validated only as > 0 (bridge.ts:2220), so a 1ms outer wait is constructible and the two deadlines race — the failure mode the docstring says this function exists to prevent. Practical impact is negligible (a 1ms outer wait makes every close round trip an unknown outcome regardless of the budget), so scoping the doc is an adequate fix. — Failure scenario: initializeTimeoutMs: 1 → drainTimeoutMs: 1 against a 1ms outer wait → the outer fires first → close outcome unknown → the close path recovers by killing the whole channel.
Suggested fix: state the ≥2ms precondition in the doc comment (keeping the pinned test honest) — or, for outerWaitMs >= 2 where both invariants are satisfiable, return Math.max(1, Math.min(outerWaitMs - 1, Math.floor(outerWaitMs * 0.8))).
中文说明
docstring 断言的不变量——“严格小于 daemon 的外层等待，使 child 截止总是先触发”——在 outerWaitMs === 1 时不成立：clamp 使预算恰好等于外层等待（新的单元测试甚至钉住 sessionCloseDrainBudgetMs(1) === 1，而其“严格小于”的循环从 2 开始）。initializeTimeoutMs 只校验 > 0（bridge.ts:2220），因此 1ms 外层等待是可构造的，两个截止时间会竞速——正是 docstring 声称本函数要防止的失效形态。实际影响可忽略（1ms 外层等待下，无论预算多少，每次关闭往返都是结果未知），因此限定文档范围即为充分修复。失败场景：initializeTimeoutMs: 1 → 1ms 外层等待下发送 drainTimeoutMs: 1 → 外层先触发 → 关闭结果未知 → 关闭恢复路径杀掉整个 channel。
建议修复：在文档注释中注明 ≥2ms 前提（使被钉住的测试保持诚实）——或在 outerWaitMs >= 2（两个不变量可同时满足）时返回 Math.max(1, Math.min(outerWaitMs - 1, Math.floor(outerWaitMs * 0.8)))。
— qwen3.8-max via Qwen Code /review (v0.21.11)
|
Released in v0.21.13. |
3 participants
What this PR does
This follow-up makes automatic active-work close authorization non-destructive. The child rejects known holds immediately, lets an already-running turn settle naturally under the close gate, checks holds again, and only then cancels queued work and tears the Session down. Both drain phases share one child-side deadline, and the daemon supplies an 8-second drain budget inside its existing 10-second round-trip deadline.
It also completes a previously deferred spawn-owner kill as soon as the final attacher leaves, before ordinary cleanup evaluates active-work reporting completeness. The deferred request therefore keeps the same force semantics as an immediate explicit kill, including with an older v1 child that reports only the legacy categories.
Why it's needed
Post-merge review of #9042 found two lifecycle races. First, a conditional close could cancel cron, goal, and notification queues before an already-running turn registered a shell hold; the close would then be refused, retaining a Session whose queued work and scheduler had already been destroyed. Second, a spawn-owner kill deferred while another client was attached could remain pending forever when the child negotiated active-work reporting without the newer
shellcategory.The new ordering preserves retained Sessions exactly as they were when authorization is refused, while still keeping explicit kill forceful and bounded.
Reviewer Test Plan
How to verify
Exercise a conditional close whose initial hold set is empty and whose running turn registers a shell while naturally settling. Confirm the close returns
closed: false, does not cancel pending work, does not dispose the Session, and releases the close gate. Also exercise a 1,000 ms close budget with a 600 ms natural-settlement phase and confirm the destructive phase receives only the remaining 400 ms.Pair the daemon with a child that negotiates only the legacy
agentandnotificationcategories. Create a spawn owner and an attacher, request a zero-attacher owner kill, detach the final attacher, and confirm the Session is removed. Confirm separately that quarantined channels still reap detached Sessions despite incomplete category reporting, while healthy incomplete children remain protected from ordinary cleanup.The complete ACP bridge suite passes with 642 tests. The three focused ACP child tests pass, and independent pre-fix/post-fix harnesses confirm zero destructive cancellations on refusal,
sessionCount=0after the deferred kill, and the shared 600 ms + 400 ms drain budget.Evidence (Before & After)
N/A — daemon lifecycle behavior with no UI change.
Tested on
Environment (optional)
macOS, Node.js 26.0.0, package-level Vitest without sandboxing.
Risk & Scope
ajv/dist/2020.js, missingfdirandmime/lite, and leaving core declaration outputs unavailable; formatting, lint, the full ACP bridge suite, and focused child lifecycle tests passed.Linked Issues
Refs #8586
Follow-up to #9042
中文说明
本 PR 的变更
这个 follow-up 将 active-work 自动关闭授权调整为非破坏性流程。child 会立即拒绝已有 hold；如果没有已有 hold，则在 close gate 下让已经运行的 turn 自然结束，再次检查 hold，只有两次检查都为空时才取消排队工作并拆除 Session。两个 drain 阶段共用一个 child 侧总截止时间，daemon 在现有 10 秒往返超时内提供 8 秒 drain 预算。
本 PR 还会在最后一个 attacher 离开后、普通清理检查 active-work 上报完整性之前，完成此前延迟的 spawn-owner kill。因此，即使旧 v1 child 只上报 legacy 类别，延迟请求仍与立即执行的显式 kill 保持相同的强制语义。
为什么需要
#9042 合并后的审查发现了两个生命周期竞态。第一，conditional close 可能先清空 cron、goal 和 notification 队列，而已经运行的 turn 随后登记 shell hold；关闭因此被拒绝，但保留下来的 Session 已经丢失排队工作，scheduler 也已被停止。第二，当 child 已协商 active-work 但不包含新的
shell类别时，spawn owner 在其他 client attached 期间延迟的 kill 可能永远无法完成。新的顺序保证授权被拒绝时 Session 保持原样，同时维持显式 kill 的强制和有界语义。
Reviewer 测试计划
验证方式
构造初始 hold 集合为空、但正在运行的 turn 在自然结束时登记 shell 的 conditional close。确认返回
closed: false，不取消排队工作、不 dispose Session，并释放 close gate。同时以 1,000 ms 关闭预算验证：自然结束阶段使用 600 ms 后，破坏性阶段只能使用剩余 400 ms。让 daemon 与仅协商 legacy
agent和notification类别的 child 配对。创建一个 spawn owner 和一个 attacher，请求仅在零 attacher 时执行 owner kill，随后 detach 最后一个 attacher，并确认 Session 被移除。另行确认 quarantined channel 即使类别上报不完整仍能回收 detached Session，而健康但类别不完整的 child 继续受到普通清理保护。ACP bridge 完整测试 642 项全部通过。3 项 ACP child 定向测试全部通过；独立的修复前/修复后 harness 还确认：拒绝前破坏性取消次数为 0、延迟 kill 后
sessionCount=0，以及 600 ms + 400 ms 共用同一个 drain 预算。证据（Before & After）
N/A —— daemon 生命周期行为变更，不涉及 UI。
测试平台
环境（可选）
macOS、Node.js 26.0.0，未启用 sandbox，运行 package 级 Vitest。
风险与范围
ajv/dist/2020.js，同时缺少fdir和mime/lite，core declaration 输出也不可用；格式检查、lint、ACP bridge 完整测试和 child 生命周期定向测试均已通过。关联 Issue
Refs #8586
#9042 的 follow-up