feat(serve): Expose active work state - #8588
Conversation
E2E test reportAutomated verification completed on macOS:
The full serve server test file completed with 865/867 passing. The two failures are pre-existing workspace tool-auth baseline failures unrelated to this diff: The manual daemon restart scenario has not been executed in this local environment. The prepared E2E plan covers the released-build baseline, |
🩺 serve daemon A/BBuilt the PR base vs this PR head
|
| field | PR base (before) | this PR (after) |
|---|---|---|
activeWork |
— | true |
activeWorkReporting |
— | "partial" |
activeWorkStaleMs |
— | 0 |
— Qwen Code · serve A/B
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Reworks the active-work signal after review. Three changes of substance. Drops the 45s heartbeat watchdog entirely. It inferred "this channel is dead" from "one Session stopped reporting" and killed the whole channel, taking every Session on that process with it — including on a suspend, a long event-loop stall, or a single dropped notification. Channel liveness is a transport concern and gets its own mechanism. Replaces the per-Session boolean with a channel-wide snapshot of named holds, derived on every report from the owners of the work (the registry's unfinalized set, the notification queue) rather than from a ledger kept alongside them. Full snapshots make a dropped report self-correcting in both directions, and a Session's absence from one is positive evidence the child released it. Agent holds now use hasUnfinalizedTasks()'s predicate, closing the cancel to finalizeCancelled() window where a cancelled agent looked idle and its terminal notification could be stranded. Leaves prompts out of the child's report: the daemon accepts, queues, dispatches, and settles them, so its own count is authoritative and covers the FIFO wait the child cannot see. A snapshot is flushed ahead of the prompt response so a hold the prompt left behind is on the wire before the daemon drops that count. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Completes the active-work rework with the two facts a restart controller was still missing and the one guarantee automatic cleanup was missing. Automatic cleanup no longer destroys a Session on the strength of a cached snapshot. It asks the child to close only if unheld, and the child answers under its own close gate — with the gate held no prompt is admitted and no automatic turn starts, so a hold cannot appear between the check and the teardown. A refusal hands back the current holds and the daemon adopts them. An unanswered request is neither retried nor assumed: the Session stays, and the next snapshot settles it, because a Session absent from one has provably been released. Every automatic path — detach, attach rollback, prompt settle, notification settle, a child reporting itself idle — now funnels through one decision point instead of four near-copies. Health gains activeWorkReporting and activeWorkStaleMs. Without them activeWork:false cannot be told apart from "no child told me anything", which is the one case where acting on it is unsafe. Freshness is graded by the daemon rather than the controller, since the cadence is negotiated per channel; a stale snapshot or a child omitting a category degrades the grade instead of silently narrowing what the boolean covers. Tests: acp-bridge 489/489, acpAgent 383/383, Session 534/534, serve suites 1188 with one pre-existing cross-file flake in the Live Appshot integration tests (reproduces on the unmodified tree, failing a different test each run). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
5d145ae to
612bcb7
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）为单个提交。 |
Force-pushed: the design changed, not just the codeRebased onto current Removed: the per-Session boolean heartbeat and the 45s channel recycle. Inferring channel death from one Session's silence kills every Session on that process, and a host suspend, a long event-loop stall, or a single dropped notification are all indistinguishable from a wedged child. Transport liveness is now its own layer (new PR 2 in #8586). Replaced with: channel-wide full snapshots of named holds, derived on every report from the owners of the work rather than kept in a parallel ledger; a three-state daemon cache that distinguishes "never negotiated" from "not yet heard from"; and a conditional close where the child confirms under its own close gate before anything is destroyed. Added: Concrete bug fixed along the way: agent holds key on Full reasoning: #8586 (comment) Please note the testing status section in the body: unit suites are green, but end-to-end has not been run and none of the seven reviewer test items were executed manually. |
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Reviewed the current force-pushed implementation at 612bcb7. The full-snapshot hold model and fail-closed reporting direction look sound, but the inline findings below still leave one unsafe automatic-close path, a health aggregation edge case, and a red test suite. Current failing CI: https://github.com/QwenLM/qwen-code/actions/runs/31071986719/job/92521662522
…ion mocks CI caught two things the local runs missed. The reporter's snapshot construction was unguarded. Only the send was wrapped, so a throw while collecting a Session's holds escaped through setInterval and queueMicrotask as an uncaught exception — capable of taking down the ACP child — and through flush() into the prompt path, turning a reporting problem into a failed prompt. Collection is now wrapped and a failed snapshot is abandoned whole rather than sent partially: a Session missing from a report reads as released, and one reported with no holds reads as safe to close, so publishing a partial snapshot would actively invite the daemon to destroy live work. Sending nothing lets the daemon's copy age instead, which its freshness grading already treats as untrustworthy and retains. flush() no longer rejects. Session.review-lease and Session.worktree mock the background-task registry without setStatusChangeCallback, so constructing a Session threw. That break arrived with the original commit, which verified only Session.test.ts; the sibling Session.*.test.ts files were never run. Both mocks now carry the methods the constructor and the hold collector need. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…lures The previous commit added the guard but could not have demonstrated it: the same commit also gave the acpAgent Session mock a collectActiveWorkHolds, removing the very condition that triggered the throw. The unhandled error disappearing was therefore explained by the mock alone, and active-work-reporter.ts had no tests at all. These cover the escape routes that matter — the interval timer, the coalescing microtask, and flush() on the prompt path — plus the choice to abandon a whole snapshot rather than send a partial one, since a session omitted from a report reads as released and one reported with no holds reads as safe to close. Verified by removing the guard: five of the nine fail with the collection error escaping, and pass again once it is restored. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Self-audit: three things to fix before this leaves draftI re-audited this adversarially. The read side — the part the title is about — holds up. The write side does not, and the three problems below share one root cause, so they should be fixed as one change rather than three patches. Since this is a draft I am not calling these merge blockers, but they are design defects rather than unfinished work: the functions involved are all written, and it is the guard model itself that does not hold. What is actually fineThe exposed surface is a whitelist projection of three scalars on The root causeThis PR promotes a cached child-reported snapshot from a hint into the authority that permits destroying a session. Three destruction paths now consult it, their guards are inconsistent with each other, and every one of them is weaker than what 1. Snapshot absence tears down sessions a user is actively watching. The absence loop checks only This also contradicts the PR description directly. I wrote that an unreported or unconfirmed Session is retained, never destroyed. 2. A ten-second TOCTOU window that 3. Arbitrarily stale cache authorizes reaping. Fix directionOne shared helper for "may this session be destroyed", with a single in-flight flag and a single freshness gate, used by all three paths. Concretely: add the in-flight gate before Also worth noting: Smaller items, not part of the above
|
Self-review of the previous revision found that this PR had promoted a cached child report from a hint into the authority that permits destroying a Session. Four teardown paths consulted it, their guards disagreed with each other, and each was weaker than what main had. The four are one defect with four exits, so they are fixed as one change. Absence from a snapshot no longer authorizes teardown. Because reports are complete, a Session the child omits holds nothing on the child side — so absence and reported-with-no-holds are the same fact and now take the same path. The separate absence loop is gone; it lacked the subscriber and client guards `maybeCloseIdleSession` applies, so one snapshot could destroy a Session with a live SSE subscriber and a registered client. That contradicted this PR's own claim that an unreported Session is retained, and the old test asserted the destruction. Both are corrected. A conditional close is now marked in flight across the whole confirm-then- teardown span, and attach, prompt, and rewind refuse a Session in that state exactly as they refuse one already closing. `closeSessionImpl` sets `closing` synchronously, but the round trip in front of it is an await of up to ten seconds; on main the guard sequence ran straight into teardown, so splitting it is what opened the window. A snapshot older than the freshness window stops counting as evidence. Staleness was already computed, but only to grade health, never to gate destruction — so a child that went quiet after one empty report left a cache that permitted reaping indefinitely. Never-reported and gone-quiet now land in the same retained bucket. Reclaiming a channel that has truly stopped answering belongs to transport liveness, not here. The idle reaper asks the child too. Its TTL says the client stopped caring, which is not the same as the child having nothing left to run. Health coverage is exposed as counts and graded once daemon-wide, because grades do not compose: a runtime with zero Sessions is vacuously `full`, and folding that in let an empty workspace vouch for another workspace's unreported Sessions. `activeWorkStaleMs` now measures only covered Sessions, so it can no longer report positive staleness beside a grade saying nothing is covered. Also: bound snapshot `sessions[]` and `holds[]` so a buggy child cannot make the daemon walk an unbounded structure per report, and retract the background-task status callback by identity rather than blanking a single-slot setter the TUI also uses. Tests: the absence test now asserts retention under a registered client and under a live subscriber; new regressions cover the recovered lost close response, the stale-snapshot gate, admission refusal during a conditional close, the reaper's confirmation, the oversized-snapshot discard, and the mixed empty/uncovered health aggregate.
|
Re-run over the new head
Moving on to code review. 🔍 Because of the core-size escalation above, final approval needs a maintainer's sign-off regardless of how the review lands — that's policy, not a judgment on the code. (One already exists at exactly this head — see Stage 3.) 中文说明本次 re-run 针对新的 head
进入代码审查。🔍 由于上述 core 规模升级，无论 review 结果如何，最终批准都需要维护者签字 —— 这是策略要求，不是对代码的否定。（事实上本 head 上已有一个维护者批准 —— 见 Stage 3。） — Qwen Code · qwen3.8-max Reviewed at |
Code reviewRe-reviewed at Critical 1 — a restore in flight looked exactly like an abandoned Session. Fixed at the right layer. Critical 2 — teardown re-resolved the target by id after the confirm round trip. Fixed with one identity re-check ( Critical 3 — the restore path wasn't upgraded to the new admission predicate. Fixed at all three guards in The nine round-1 Suggestions are explicitly deferred by the author — recorded thread-by-thread as "defer, not dispute", consistent with this repo's guidance to stop widening scope after ~5 review rounds. They remain valid follow-ups: the missing Everything said in the previous pass about the base of this PR still holds — the three-state model, the single-funnel retention rule, bounded snapshots, daemon-wide grading, and the conventions (no sequenceDiagram
participant D as Daemon bridge
participant C as ACP child
participant R as ActiveWorkReporter
participant S as Session
D->>C: initialize - proposes capability in _meta
C->>R: creates reporter with clamped cadence
C-->>D: answers with cadence and covered categories
R->>D: full channel-wide snapshot (interval, on change, on flush)
Note over D: absence and no-holds are the same fact
D->>D: candidate check - shared guards, restore-in-flight excluded
D->>C: conditional close ask, marked in-flight
Note over D: attach, prompt, rewind, restore refuse in-flight
D->>D: identity re-check before teardown
C->>S: checks holds under its close gate
S-->>C: no holds - or refusal handing back holds
C-->>D: closed true, or refusal, or silence (retain)
Files changed (20 of 20 shown)
TestingAll checks green on
The central claims are live-lifecycle behaviours — retention across a restore in flight, the conditional-close handshake including refusal and unanswered paths, admission refusal (now on the restore path too), the identity re-check across the confirm window — and the unit suite pins them only through mocked channels, with two of the three new fixes resting on code-path reading by the author's own account. Sandboxed verification is the lane that settles this: 中文说明代码审查已在 Critical 1 —— 进行中的 restore 看起来与被遗弃的 Session 完全一样。 在正确的层面修复。 Critical 2 —— 拆除在确认往返后按 id 重新解析目标。 以一处在 Critical 3 —— restore 路径未升级到新的准入谓词。 第一轮的九个 Suggestion 由作者明确推迟 —— 在每个线程中记录为"推迟，不反驳"，符合本仓库约 5 轮审查后不再扩大范围的指引。它们仍是有效的后续项：缺失的 上次审查对本 PR 基础部分的所有结论仍然成立 —— 三态模型、单漏斗保留规则、有界快照、daemon 级分级，以及约定（无 测试
核心声明是生命周期行为 —— restore 进行中保留、条件关闭握手（含拒绝与无应答）、准入拒绝（现在包括 restore 路径）、确认窗口上的身份复查 —— 单测只能通过 mock channel 固定，且三个新修复中有两个按作者自己的说法依赖代码路径阅读。沙箱验证是坐实它的通道： — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 3/5 — the delta review at the new head is clean: all three round-1 Criticals are verifiably fixed in the final tree, CI is green at exactly this commit, and a maintainer approval already stands on it; the cap is pure core-size policy, which keeps the bot out of the final sign-off on a 1298-line fork PR touching core paths. Stepping back: what this round demonstrates is the PR's best habit. Three race findings — the kind that invite a scatter of point patches — got one shared diagnosis ("the round trip turned a synchronous guard-then-teardown into an awaited span"), three minimal fixes at exactly the points the diagnosis names, and a commit message that tells you which two fixes the test harness cannot honestly pin and why. The restore-in-flight exclusion went into the shared candidacy predicate rather than the snapshot trigger, so the reaper's path inherits it for free — that is the difference between fixing a finding and fixing the class. The identity re-check sits exactly between the confirm await and the re-resolving teardown. The restore path now uses the same admission predicate as every other entry point, making the doc comment that previously overpromised actually true. Scope discipline holds: +39/−2 production lines, one regression test, nothing else touched; layers 2–5 of #8586 stay out. My independent baseline for this problem (additive deep-health fields, versioned What remains, plainly stated:
⏸️ Deferring to @wenshao @tanzhenxin @yiliang114 @LaZzyMan — 中文说明置信度：3/5 —— 新 head 上的增量审查干净：第一轮的三个 Critical 均已在最终代码中核实修复，CI 恰在本 commit 上全绿，且已有维护者批准落在同一 head；封顶纯粹来自核心模块规模策略 —— 1298 行生产逻辑、触及 core 路径的 fork PR，最终签字不归 bot。 退一步看：这一轮展示的是这个 PR 最好的习惯。三个竞态发现 —— 最容易引来一堆零散补丁的那类问题 —— 得到了一个共享诊断（"往返把同步的'守卫即拆除'变成了带 await 的区间"）、三处恰好落在诊断所指位置的最小修复，以及一条如实说明哪两个修复无法被测试装置诚实固定、为什么的 commit message。restore 进行中的排除被放进共享候选谓词而不是快照触发点，于是 reaper 路径免费继承了它 —— 这是"修复一类问题"与"修复一个发现"的区别。身份复查恰好位于确认 await 与重新解析目标的拆除之间。restore 路径现在与其他所有入口使用同一准入谓词，使此前过度承诺的文档注释真正成立。范围纪律保持：生产代码 +39/−2，一个回归测试，未触碰其他；#8586 的第 2–5 层仍在范围外。 我对这个问题的独立基线（深度健康接口增量字段、带版本的 尚未完成的，直说：
⏸️ 转交 @wenshao @tanzhenxin @yiliang114 @LaZzyMan（CODEOWNERS 中 — Qwen Code · qwen3.8-max Reviewed at |
Triage review found that the design doc, the PR description, and the comment on `entryHasActiveWork` all promised the daemon *asks* the child about a Session it has not heard about, while no code path ever did: `entryHasActiveWork` returns true when the child's side is unknown, and the cleanup path returned early on exactly that. The finding predates the guard rework and survived it unchanged. Skipping on unknown looks like the safe direction and is in fact the worse failure. Nothing resolves it — a Session on a channel that went quiet is retained forever, and the idle reaper skips it too, so there is no path out at all. Asking resolves it definitively: the child answers under its own close gate whether or not its snapshots are arriving, the round trip is bounded, and every non-answer still retains. So the predicate is split by what it actually knows. `childReportsHeldWork` is positive knowledge only; `childWorkIsUnknown` is the absence of a gradeable report. The health surface ORs both, because a controller must never read "nobody told me" as "nothing is running". Automatic cleanup blocks only on known work and lets unknown through to `confirmChildUnheld`. Also moves `parseActiveWorkSnapshot` out from between two import blocks (pure relocation, no logic change) and aligns the doc wording, including the shared-guard table, with what the code now does.
|
Thanks — both findings taken, and one correction to the record: this review is against Finding 1 — the "unknown" state promises an ask the code never made. Correct, and it survived the rework unchanged. Fixed by splitting the predicate on what it actually knows rather than by softening the wording: Worth stating why the wording was not the thing to change. Skipping on unknown looks like the safe direction and is in fact the worse failure — nothing resolves it, the reaper skips it too, and a Session on a channel that went quiet is retained forever with no path out. Asking resolves it definitively: the child answers under its own close gate whether or not its snapshots are arriving, the round trip is bounded, and every non-answer still retains. Two regression tests cover it: the ask happens for a never-reported Session, and a refusal resolves the unknown toward retention with the reason attached. Finding 2 — What the review could not have seen. The self-audit found that this PR had promoted a cached child report from a hint into the authority that permits destroying a Session, across four teardown paths whose guards disagreed with each other:
All four are the same defect with four exits, so they are fixed as one change: one shared candidacy predicate, one in-flight flag held across confirm-then-teardown, one freshness gate, and the reaper routed through the same ask. The absence loop is gone — because reports are complete, absence and reported-with-no-holds are the same fact and now take the same path. On the two deferred items: agreed that Re-review will need to happen at the new head rather than |
Self-audit + triage findings addressed — head is now
|
| # | Defect | Fix |
|---|---|---|
| 1 | Absence from a snapshot tore down a Session with a live SSE subscriber and a registered client — guards maybeCloseIdleSession treats as hard |
Separate absence loop deleted. Because reports are complete, absence and reported-with-no-holds are the same fact and take the same path, through every shared guard |
| 2 | confirmChildUnheld is a 10s round trip that never set closing, so attach / prompt / rewind could be admitted into a Session already authorized for teardown |
One in-flight flag held across the whole confirm-then-teardown span; all three admission paths refuse it exactly as they refuse closing |
| 3 | Staleness was computed but only ever graded health, never gated destruction — a child that went quiet after one empty report left a cache permitting reaping indefinitely | Freshness gate moved into the retention predicate; never-reported and gone-quiet are now the same state |
| 4 | The idle reaper never called confirmChildUnheld at all |
Routed through the same ask, keeping its own TTL and crash-path policy |
Defect 1 also contradicted this PR's own description — it claimed an unreported or unconfirmed Session is retained, never destroyed, while bridge.test.ts asserted the destruction as expected behaviour. Both are corrected; the description is updated, and the test now asserts retention under a registered client and under a live subscriber.
Triage findings
- "unknown promises an ask the code never made" — correct, and it survived the rework. Fixed by splitting the predicate on what it actually knows rather than by softening the wording:
childReportsHeldWork(positive knowledge) vschildWorkIsUnknown(absence of a gradeable report). Health ORs both; cleanup blocks only on known work and lets unknown through to the ask. Skipping on unknown looks safe and is the worse failure — nothing resolves it, so the Session is retained forever with no path out. parseActiveWorkSnapshotbetween two import blocks — fixed, pure relocation.
Also in scope
Snapshot sessions[] / holds[] are now bounded; health coverage is exposed as counts and graded once daemon-wide (an empty runtime is vacuously full and could vouch for another workspace's unreported Sessions); activeWorkStaleMs counts only covered Sessions; the background-task status callback is retracted by identity instead of blanking a single-slot setter the TUI shares; and activeWork's Session-scoped boundary vs channel-level work is documented rather than widened.
Verification
| Check | Result |
|---|---|
| acp-bridge suite | 1085/1085 |
| active-work subset | 17/17, incl. 8 new regressions |
Session.test.ts |
534/534 |
core background-tasks |
123/123 |
| serve suites | 1188/1189 — the one failure is a pre-existing cross-file flake (passes in isolation, fails a different test each run) |
| deep-health subset | 14/14 |
| tsc core / acp-bridge | clean |
| tsc cli | clean in every touched file |
| eslint + prettier | clean |
New regressions: retention under a registered client and under a live subscriber; recovery of a lost close response; the stale-snapshot gate; admission refusal during a conditional close; the reaper's confirmation; the oversized-snapshot discard; the mixed empty/uncovered health aggregate; and the ask-on-unknown path in both directions.
Still not done
No live end-to-end run. Agreed that @qwen-code /verify is the right instrument — the central claims are lifecycle behaviours the unit suite pins only through mocked channels, and note that none of the four defects above would have been caught by CI, which is the honest argument for the sandboxed lane before merge. macOS and Windows remain untested.
Syncs 46 commits of base drift. CI's 'Check voice guard mirror sync' step runs from main and invokes `npm run check:voice-guard-sync`, a script added alongside that step in 732f4d8 (QwenLM#8350) and absent from this branch — so the check failed on missing-script, not on anything in this diff. Merged rather than rebased so existing review comments stay anchored.
CI:
|
| Check | Result |
|---|---|
| acp-bridge suite | 1110/1110 (25 files) |
| Session + acpAgent + reporter | 929/929 |
| serve suites | 1216/1218 |
check:voice-guard-sync |
passes |
The two serve failures are Live conversation runtime lifecycle — a pre-existing cross-file flake in this file, unrelated to active work. All 6 tests in that describe block pass in isolation, and the same file fails a different test on different runs.
Two interactions worth naming explicitly, since both are the class of thing a clean textual merge hides:
mainchangedAcpSessionBridgeinbridgeTypes.tstoo —getChildResourceSnapshotgained an optionalageMs. Independent ofactiveWorkCoverage; no interaction.mainadded 184 lines toacpAgent.test.ts, the same file whose Session mock previously caused an uncaught exception in the reporter. Checked directly: none of the new tests construct a Session, and theSessionMock.prototype.collectActiveWorkHoldspatch survived the merge.
Also picked up cb3dc107f (#8604), which deflakes the GlobTool external-path test that had to be reran on an earlier revision of this PR.
The three route checks reported cancelled on separate workflow runs — CI runner routing pre-empted by the newer push, no action needed; the merge push re-triggers them.
|
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: 307 passed · 0 failed · 307 total 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：307 通过 · 0 失败 · 307 总计 Verification reportPR 8570 — fix(cli): report zero-height VP items so collapsed thoughts release reserved spaceVerdict: 中文摘要
Central claim + A/BCentral claim: in VP mode, collapsing an expanded thought group releases the reserved vertical space, because continuations that render nothing now report their measured zero height into the height cache ( Harness:
Witnesses: Secondary claims
Reading: zero-report alone fixes the reported gap but breaks re-expand coherence; the walk-back/clamp hunks heal exactly those. Every kill set matched its prediction; no off-target reds.
Findings (non-blocking)
Not covered
MethodologyEnvironment: CI Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention). — Qwen Code · sandboxed verification |
|
Triage re-run completed without a new review.
The stage comments above were updated with the latest result. View workflow run. 上方各阶段评论已更新为最新结果。查看工作流运行。 |
Runtime verification (maintainer review)I built a real end-to-end environment for this PR and executed all seven reviewer test-plan items, plus a genuine mixed-version run. All seven pass. Two non-blocking notes below. Setup. Both arms compiled from source into runnable
Reviewer test plan
The decisive A/B (item 3)Same script, same mock, two builds:
Extra checks beyond the plan
Note A — a prompt inside the confirm window is accepted (202) and lost
The caller gets no error and nothing on the event stream. This shape predates the PR ( Note B — the conditional close does not carry the daemon's drain budget
VerdictThe mechanism does what the description says, the failure direction is genuinely the safe one (unreported or unconfirmed ⇒ retained, never destroyed), and the mixed-version path leaves old children behaving exactly as before. The two notes are documentation/polish, not correctness blockers. LGTM — recommend merge. Note A's wording is worth a one-line fix before or after merge. 中文版运行时验证（维护者评审）我为本 PR 搭建了真实的端到端环境，执行了 Reviewer 测试计划的全部 7 条，另加一次真实的跨版本（新 daemon + 旧子进程）验证。7 条全部通过，另有 2 条不阻塞的说明。 环境。 两侧均从源码编译成可运行的
Reviewer 测试计划
决定性 A/B（第 3 条）同一脚本、同一 mock、两个构建：
计划之外的补充验证
说明 A —— confirm 窗口内的 prompt 会被接受（202）并丢失
调用方既拿不到错误，事件流上也什么都没有。这个形状早于本 PR（ 说明 B —— 条件关闭没有携带 daemon 的 drain 预算
结论机制与描述相符，失败方向确实落在安全侧（未上报或未确认 ⇒ 保留，绝不销毁），跨版本路径也让旧子进程保持原样。两条说明属于文档与打磨，不构成正确性阻塞。 LGTM —— 建议合并。 说明 A 的措辞值得在合并前后顺手改一行。 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not reviewed: build-and-test — 'Integration Tests (CLI, No Sandbox)' was skipped in CI and its suite did not run locally (the local run was unit-only).
中文说明
未审查：build-and-test — 'Integration Tests (CLI, No Sandbox)' was skipped in CI and its suite did not run locally (the local run was unit-only)。
— qwen3.8-max via Qwen Code /review (v0.21.7)
Review found three ways the conditional close can still destroy a live Session. All three share a cause: the round trip turned a synchronous guard-then-teardown into an awaited span, and three things that were previously impossible to observe mid-teardown now are. **A restore in flight looks exactly like an abandoned Session.** `session/load` registers the entry before awaiting `artifacts.restore()` and `seedSessionUpdates()`, and registers its first client only after — so for that whole window there are no clients, no subscribers, nothing held, and the child answers the conditional close truthfully. The snapshot trigger this PR added fires inside it. Excluded in `entryIsAutoCloseCandidate` rather than at the snapshot trigger, so the reaper's TTL elapsing inside a slow restore is covered too. `pendingRestoreIds` already existed but was read only by `hasNoChannelWork`, never by the close funnel. **Teardown re-resolved the target by id without re-checking identity.** `closeSessionImpl` does a fresh `byId.get`, and the id can be re-registered to a different entry during the round trip: an explicit kill removes this one (kill ignores the in-flight flag by design, keeping its force semantics) and a `session/load` for the same persisted id registers a fresh one. The stale continuation then tore down the newly restored Session under its just-attached client. One identity re-check after the await. **The restore path was not upgraded to the new admission predicate.** `sendPrompt`, `rewindSession`, and single-scope attach check `isClosingOrAuthorizingClose`; `restoreSession` still checked bare `closing` at both its guards, so a client could attach inside the window and lose the session under it. That directly contradicted the `closeIfChildUnheld` comment claiming every admission path checks the flag. Its `racedEntry` branch had no closing guard at all — a narrower pre-existing hole, same defect, same predicate. Regression test covers the restore-path admission refusal. The other two need a mid-restore snapshot and a kill-then-reload interleave that the mocked-channel harness cannot stage honestly; both are pinned by reading the code paths, which is weaker and worth saying.
Review round addressed — 3 Critical fixed in
|
| Finding | Verified how | Fix |
|---|---|---|
Snapshot auto-close can tear down a session/load mid-restore |
Entry is registered before await artifacts.restore() / seedSessionUpdates(), first client only after; pendingRestoreIds was read solely by hasNoChannelWork, never by the close funnel |
Excluded in entryIsAutoCloseCandidate — not at the snapshot trigger as the inline suggestion had it, because the reaper's TTL can elapse inside a slow restore too |
| Teardown re-resolves by id after the await without an identity re-check | closeSessionImpl does a fresh byId.get; kill deliberately ignores the in-flight flag, so the id can be re-registered mid-round-trip |
One identity re-check after confirmChildUnheld |
restoreSession guards on bare closing, not the new predicate |
Three admission paths upgraded, this one missed — contradicting this PR's own "every admission path checks this flag" comment | Both guards upgraded, plus the racedEntry branch which had no closing guard at all |
All three share one cause: turning a synchronous guard-then-teardown into an awaited span made three previously unobservable mid-teardown states reachable. That is the same root cause as the guard rework earlier in this PR, which is the honest reading — the confirm window is genuinely the risky part of this design, and this is the third round of finding things inside it.
Verification: acp-bridge 1111/1111, active-work subset 18/18 including a new regression for the restore-path admission refusal. The other two fixes are pinned only by reading the code paths — a mid-restore snapshot and a kill-then-reload interleave are not something the mocked-channel harness can stage honestly, and I'd rather say so than imply test coverage I don't have.
Deferred — 9 Suggestions, replied and resolved individually
Not disputed, and none silently dropped. This is round 6+ of automated review on a PR that already carries a maintainer approval, so autonomous changes are held to critical-level findings rather than accumulating commits on an approved diff.
Five are flagged to the author as worth promoting, because they are the same class of problem this PR has already had to correct twice — a claim broader than the implementation:
- The refusal-response hold set is adopted unbounded, while
parseActiveWorkSnapshotcaps the identical payload. Capping only one of two ingestion paths is an inconsistency introduced here. - A single huge-but-valid
seqpermanently latches the channel high-water mark, costing the self-healing property the design doc advertises (fails closed, so no destruction). - The reporter's eager constructor publish appears to be always discarded, which would make the comment justifying it false.
- Design doc drift: the close-response success shape, and
the child proposes, the daemon clampsis inverted. - Protocol doc drift: the
activeWorkenumeration omits the fail-closedunknown ⇒ activebehavior a restart controller depends on.
The remaining four are bounded churn or scope (enqueueBackgroundNotification, setSessionApprovalMode / setSessionModel, a dead readonly, and the test-coverage gap — partly exercised by the maintainer's live run, though not by a committed test, so the mutation described would still pass CI).
Threads: 12/12 replied and resolved. 0 unresolved.
Runtime re-verification at
|
| AFTER | PR head 12adf869 |
| BEFORE (fix-level) | previous head 80019acab7 — isolates the three fixes |
| BEFORE (PR-level) | merge-base 20b9504276 |
| Daemon | node packages/cli/dist/index.js serve --workspace … --token …, fresh daemon + fresh workspace per scenario |
| Child | real qwen --acp process |
| Fault injection | QWEN_CLI_ENTRY stdio tee: logs every JSON-RPC frame both ways, and can delay the response to qwen/control/session/close {onlyIfUnheld:true}, delay any close, drop snapshots, or replay the child's own snapshot at a chosen instant |
| Model | scripted OpenAI-compatible mock that launches a background Agent and then parks the sub-agent's completion on demand |
The three fixes
| # | Fix | Verdict |
|---|---|---|
| 1 | restoreSession guards on the new predicate, not bare closing |
✅ confirmed live — decisive A/B below |
| 2 | identity re-check after confirmChildUnheld |
|
| 3 | pendingRestoreIds excluded from entryIsAutoCloseCandidate |
Fix 1 — restore admission (confirmed)
Same driver, same stalled child (conditional-close response held 7 s), two daemons. POST /session/:id/load issued 1.2 s into the confirm window:
On 80019ac the caller is told 200 {"attached":true, clientId:…} and the teardown it raced then destroys the Session under it — no error ever reaches the client. On 12adf869 it is refused up front with 404 "The session is closing; retry after close completes". This is a real defect, really fixed.
The full admission matrix inside an 8 s window at the new head is in the screenshot: load 404, rewind 404, attach-by-id 409, prompt 202 then lost, DELETE 409.
Fix 2 — identity re-check (could not be staged, and that is informative)
To reach it I need the id removed and re-registered during the confirm round trip. Over REST that is not reachable at this head:
- there is no kill route (
killSessionis only called from sub-session/archive/rollback paths), and DELETE /session/:idinside the window returns409 session_archiving,Retry-After: 5— becausePOST /session/:id/detachruns underwithOwnerMutableSession, which holds a shared archive lock for the whole handler, and the handler now awaits the conditional close.
So the lock that blocks my repro is also what narrows this race's real-world exposure — the re-registration has to come from a non-REST path. The guard costs one map lookup and fails safe; I'd keep it, but it stays code-read-only.
Fix 3 — mid-restore exclusion (inconclusive, and I tried hard)
I could not reach the state it prevents:
- The window is ~1 ms. Polling
/health?deep=1at ~5 samples/ms across a 501 mssession/loadof a 1200-record transcript, exactly 1 of 2453 samples saw the entry registered while the load was still in flight. - Snapshot path: replaying the child's own
qwen/notify/channel/active-worksnapshot 700 times at 1 ms resolution across the whole restore did not tear the Session down on80019ac. - Reaper path: with
--session-reap-interval-ms 1 --session-idle-timeout-ms 1, both builds behave identically — the conditional close for the restoring id goes out 5–6 ms after the child's load response on80019acand on12adf869. I cannot separate "decided inside the guarded window" from "decided in the unguarded tail between the restore promise settling and the HTTP response being written", so the test does not discriminate.
Not evidence the fix is wrong — evidence that the live path is too narrow to pin. If you want it pinned, a unit test with a controllable await inside the restore is the only honest way.
What still holds at the new head
- Headline claim, re-proven against the merge base. Main prompt settled with a background Agent still running:
activePrompts: 0,activeWork: true,reporting: "full". Detach the last client → merge-base destroys the Session (404, channel dead, Agent killed with it); PR head retains it (200). Agent + terminal notification settle → clean close,sessions: 0,activeWork: false. - Negotiation and holds captured verbatim. Daemon sends
initialize._meta{"v":1,"intervalMs":15000}; child answers{"v":1,"intervalMs":15000,"categories":["agent","notification"]}; holds appear as{"category":"agent","id":"general-purpose-call_3"}whileactivePromptsis already 0. - Fail-closed is honest. A Session on a negotiated channel that has not been named by a snapshot yet reads
activeWork: true, reporting: "partial"until its first report — the documented "unknown ⇒ retained" state, observed live. - Unit suites at
12adf869: acp-bridge499/499, cli acp-integration929/929, cli serve1218/1218, core background-tasks123/123— 2769 tests, no failures. CI on this head is green on every job.
Notes (non-blocking)
A. A prompt inside the confirm window is still accepted (202) and lost. Unchanged from my last report and re-measured here: POST /session/:id/prompt → 202 {promptId}, and after the window GET /session/:id/pending-prompts → 404. load and rewind both throw synchronously and answer 404; sendPrompt returns a rejected promise the route's try/catch cannot see. Pre-existing shape, but the PR widens the window it applies to, and test-plan item 5 claims the prompt is "refused rather than accepted and lost", which is not what a REST caller observes.
B. The conditional close still omits the drain budget. On the wire: {"sessionId":…,"onlyIfUnheld":true} versus the real close's {"sessionId":…,"drainTimeoutMs":8000}. The child falls back to SESSION_DRAIN_TIMEOUT_MS = 30_000 while the daemon waits only 10 s. Self-correcting, safe direction, one wasted cycle.
C. Detach costs one extra bounded round trip. I checked whether this is new before reporting it: with every session/close response stalled 8 s, POST /session/:id/detach takes 8019 ms on the merge base, 16017 ms on 80019ac, 16016 ms on 12adf869. So blocking detach is pre-existing; the PR adds one bounded confirm in front of it (worst case 10 s confirm + 8 s drain). With a healthy child it is 15 ms. Worth knowing because the route holds its shared archive lock for that whole span, so DELETE/archive on that Session are refused with 409 meanwhile — "explicit close keeps force semantics" is true of the bridge, but not of what a REST caller sees.
Verdict
The one fix that is observable from outside the daemon is genuinely fixed, and I reproduced the bug it fixes on the previous head. The other two are in the safe direction and cheap; I could not stage either, which matches the author's own statement rather than contradicting it. Everything I verified last round still holds at 12adf869, and the failure direction remains "unreported or unconfirmed ⇒ retained, never destroyed".
LGTM — recommend merge, my previous approval stands at this head. Note A is worth one line of code or one line of prose before or after merge.
中文版
在 12adf869 上的运行时复验 —— 针对三个 Critical 修复
我上一份报告测的是 80019acab7。此后合入了三个 [Critical] 修复，作者明确说明其中两个只靠读代码确认，没有测试覆盖（restore 期间的快照、kill 后重新加载的交错）。我在新 head 上重建了环境，专门去打这两个点。
环境。 三个 arm，全部从源码编译成可运行的 dist，以真实进程驱动 —— bridge、子进程、health 路由都没有 mock：
| AFTER | PR head 12adf869 |
| BEFORE（修复级） | 上一个 head 80019acab7 —— 用于隔离这三个修复 |
| BEFORE（PR 级） | merge-base 20b9504276 |
| Daemon | node packages/cli/dist/index.js serve --workspace … --token …，每个场景都用全新 daemon + 全新 workspace |
| 子进程 | 真实 qwen --acp 进程 |
| 故障注入 | QWEN_CLI_ENTRY stdio tee：双向记录每一条 JSON-RPC 帧，并可延迟 qwen/control/session/close {onlyIfUnheld:true} 的应答、延迟任意 close、丢弃快照，或在指定时刻重放子进程自己的快照 |
| 模型 | 脚本化的 OpenAI 兼容 mock：先拉起后台 Agent，再按需挂起子 agent 的补全 |
三个修复
| # | 修复 | 结论 |
|---|---|---|
| 1 | restoreSession 改用新判据而非裸 closing |
✅ 实测确认 |
| 2 | confirmChildUnheld 之后补身份重校验 |
|
| 3 | entryIsAutoCloseCandidate 排除 pendingRestoreIds |
修复 1（已确认）。 同一脚本、同一被卡住的子进程（条件关闭应答延迟 7 秒），两个 daemon；在 confirm 窗口内 1.2 秒处发 POST /session/:id/load：80019ac 返回 200 {"attached":true, clientId:…}，随后它所竞争的拆除把这个 Session 在客户端脚下销毁 —— 调用方拿不到任何错误；12adf869 直接 404 "The session is closing; retry after close completes"。真实缺陷，真实修好。窗口内完整的准入矩阵见截图：load 404、rewind 404、按 id attach 409、prompt 202 然后丢失、DELETE 409。
修复 2（无法构造，但这件事本身有信息量）。 要触发它，必须在 confirm 往返期间把同一个 id 移除再重新注册。当前 head 上 REST 做不到：没有 kill 路由（killSession 只在子会话/归档/回滚路径里被调用）；而窗口内的 DELETE /session/:id 会返回 409 session_archiving（Retry-After: 5）—— 因为 POST /session/:id/detach 跑在 withOwnerMutableSession 里，整个 handler 期间持有 archive 的共享锁，而该 handler 现在要 await 条件关闭。也就是说，挡住我复现的那把锁，同时也压缩了这个竞态的现实暴露面 —— 重新注册只能来自非 REST 路径。这个守卫只值一次 map 查找且失败方向安全，我建议保留，但它仍然只是"读代码确认"。
修复 3（无法判定，且我确实尽力了）。
- 窗口只有约 1 毫秒：以约 5 次/毫秒的频率轮询
/health?deep=1，覆盖一次 1200 条记录、耗时 501 ms 的session/load，2453 个采样里只有 1 个看到"条目已注册但 load 仍在进行"。 - 快照路径：把子进程自己的
qwen/notify/channel/active-work快照以 1 ms 粒度重放 700 次贯穿整个 restore，80019ac上 Session 依然没有被拆除。 - reaper 路径：用
--session-reap-interval-ms 1 --session-idle-timeout-ms 1，两个构建表现完全一致 —— 针对正在 restore 的 id 的条件关闭，都在子进程 load 应答之后 5–6 ms 发出。我无法区分"在被守卫的窗口内做的决定"和"在 restore promise settle 之后、HTTP 响应写出之前那段无守卫尾巴里做的决定"，因此该测试不具备区分力。
这不是说修复错了，而是说这条活路径太窄、钉不住。若要钉死，只能写一个能控制 restore 内部 await 时机的单测。
新 head 上依然成立的部分。 主 Prompt 结束、后台 Agent 仍在跑时：activePrompts: 0、activeWork: true、reporting: "full"；断开最后一个客户端后，merge-base 销毁 Session（404，通道死亡，Agent 一并被杀），PR head 保留（200）；Agent 与终态通知 settle 后干净关闭，sessions: 0、activeWork: false。能力协商逐字抓取：daemon 发 {"v":1,"intervalMs":15000}，子进程回 {"v":1,"intervalMs":15000,"categories":["agent","notification"]}，hold 形如 {"category":"agent","id":"general-purpose-call_3"}。尚未被快照点名的 Session 读作 activeWork: true, reporting: "partial"，即文档中的"unknown ⇒ 保留"，实测如此。单测：acp-bridge 499/499、cli acp-integration 929/929、cli serve 1218/1218、core background-tasks 123/123，合计 2769 条全绿；该 head 的 CI 全部 job 通过。
说明（均不阻塞）。
- A. confirm 窗口内的 prompt 仍然被 202 接受然后丢失。 与上次一致并重新实测：
POST /session/:id/prompt→202 {promptId}，窗口结束后GET /session/:id/pending-prompts→ 404。load与rewind都是同步抛出并返回 404；sendPrompt返回的是 rejected promise，路由的try/catch看不见。形状早于本 PR，但本 PR 扩大了其适用窗口，而测试计划第 5 条写的是 prompt 会"被拒绝而非被接受后丢失"，与 REST 调用方观察到的不符。 - B. 条件关闭仍未携带 drain 预算。 线上为
{"sessionId":…,"onlyIfUnheld":true}，而真正的 close 是{"sessionId":…,"drainTimeoutMs":8000}。子进程回落到SESSION_DRAIN_TIMEOUT_MS = 30_000，daemon 却只等 10 秒。可自愈、方向安全，只是多一个循环。 - C. detach 多出一次有界往返。 我在报告前先做了对照：把所有
session/close应答都延迟 8 秒，POST /session/:id/detach在 merge-base 上耗时8019 ms、80019ac上16017 ms、12adf869上16016 ms。所以"detach 会阻塞"并非新增，本 PR 只是在它前面加了一次有界 confirm（最坏 10 秒 confirm + 8 秒 drain）；子进程健康时是 15 ms。之所以值得知道，是因为该路由在整段时间里持有 archive 共享锁，期间对该 Session 的DELETE/归档会被 409 拒绝 —— "显式 close 保持强制语义"对 bridge 成立，但对 REST 调用方看到的结果并不成立。
结论
唯一能从 daemon 外部观测到的那个修复确实修好了，我也在上一个 head 上复现了它所修的缺陷。另外两个方向安全、代价极小；我两个都没能构造出来，这与作者自己的说法一致，而非相反。上一轮验证过的内容在 12adf869 上全部依然成立，失败方向仍然落在安全侧（未上报或未确认 ⇒ 保留，绝不销毁）。
LGTM —— 建议合并，我此前的 approve 在本 head 上继续有效。说明 A 值得在合并前后顺手改一行代码或一行措辞。
|
@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: 3473 passed · 0 failed · 3473 total 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：3473 通过 · 0 失败 · 3473 总计 Verification report<!-- qwen-triage:verify --> Sandboxed verification: ✅ passed — merge-ready (agent verdict) 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: 3473 passed · 0 failed · 3473 total 中文 — 判定：✅ 通过 · 可合入（agent 判定）
Verification reportPR 8588 — feat(serve): expose active work stateVerdict: Note on Central claim + A/BCentral claim: Harness:
Witnesses: Secondary claims
Findings (non-blocking)
Not covered
MethodologyEnvironment: CI Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention). — Qwen Code · sandboxed verification |
|
Triage re-run completed without a new review.
The stage comments above were updated with the latest result. View workflow run. 上方各阶段评论已更新为最新结果。查看工作流运行。 |
|
@qwen-code /review |
| _Qwen Code review request accepted. Review is queued in [workflow run](https://github.com/QwenLM/qwen-code/actions/runs/31243191061)._ |
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
LGTM. The three round-1 Criticals are fixed at this head — verified by reading the current code, not just the author notes: snapshot auto-close now excludes pendingRestoreIds, the stale continuation is re-checked by identity immediately before closeSessionImpl, and all three admission paths (including the previously unguarded raced-entry branch) use isClosingOrAuthorizingClose.
Counter pairing on the reporting side holds on all paths (inc-before-try / dec-in-finally), holds are derived per report from the real work owners so nothing can outlive the work it names, unknown or stale state fails closed to busy with a downgraded reporting grade, and the new health fields are purely additive. CI is green on this head.
The remaining items are the ones already deferred with maintainer approval and tracked in the automated review thread — non-blocking: uncapped refusal-hold adoption, the seq high-water latch, the eager constructor publish that is deterministically discarded, the three admission paths still missing the close guard, and the doc drift. Nothing here blocks merge.
Merging main's active-work close protocol (QwenLM#8588) into this PR's abandoned restore bound produced a deadlock that neither side has on its own, and the conflict resolution was committed without running tests. `maybeCloseIdleSession` now routes through `confirmChildUnheld`, which asks the child whether it still holds work before closing a session nobody is attached to. That is right in general and wrong for a channel this PR has already condemned. `restoreSettlementOverdue` and quarantine exist precisely because the child stopped being answerable, and their whole premise is that visible work drains so the channel can be reaped — closing the transport is the only thing that can release a restore we cannot cancel. Making that drain depend on a round trip to the wedged child inverts it: a child stuck in a non-cancellable restore is exactly the one that cannot reply inside `ACTIVE_WORK_CLOSE_TIMEOUT_MS`, so the sessions never close, the channel never drains, the reap never fires, and the bound never takes effect. A channel condemned by the restore lifecycle now skips the round trip and proceeds to local teardown. Nothing is attached to the session by then — `maybeCloseIdleSession` gates on that — and the sibling-safety invariant is untouched: this closes sessions whose clients have already left, it does not force-kill a channel that still has live ones. The regression test drives an overdue channel whose child never answers the close-if-unheld probe and asserts the detach still reaps it. Reverting the guard reproduces the deadlock as a test timeout. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Released in v0.21.8. |
…#8691) * fix(serve): make session restore timeouts safe Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(cli): restore missing core mock exports in the ACP worktree suite The restore-tracing change added `extractDaemonTraceContext` and `withDaemonSpan` to `acpAgent.ts`, but `acpAgent.worktree.test.ts` replaces `@qwen-code/qwen-code-core` with a full mock factory that never listed them. `loadSession` then failed on an undefined export, taking all three cases down and producing teardown rejections from the half-built agent. The sibling suite was updated; this one was missed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(serve): bound and disambiguate the abandoned restore lifecycle Four follow-ups from review of the restore timeout work. A startup budget may now raise the restore budget but never lower it. Taking an explicitly configured `initializeTimeoutMs` as the restore fallback meant a deployment that tightened its child-initialize check still inherited a sub-default restore deadline — exactly the failure this change exists to remove. An explicit `sessionRestoreTimeoutMs` still wins outright, including below the default, for deployments that want restore to fail fast. Validation now names the field actually at fault. A restore fenced behind a timed-out predecessor is no longer reported as an ordinary in-flight restore. It carries `reason: awaiting_abandoned_cleanup` and a retry hint of one restore budget (capped at 120s) instead of the ordinary 5 seconds, because the fence cannot clear until the non-cancellable ACP request settles and a 5-second cadence just spins the caller against a 409 it cannot resolve. Whether a channel is condemned is now derived rather than sticky. A timeout recorded `emptyReapPending` permanently, so any channel that had ever seen one was guaranteed to be reaped once its remaining work drained, forcing a cold respawn even when the late restore had landed and closed cleanly. The reap condition is now computed from an outstanding `unsettledAbandonedRestores` set, quarantine, or an ordinary pending empty reap; real settlement clears the entry and hands the channel back to the configured idle policy. Abandonment no longer retains ownership without bound. One further restore budget after the deadline, a still-unsettled restore marks the channel `restoreSettlementOverdue`: existing sessions and workspace control keep working, but fresh session work is refused so the channel can drain, since closing the transport is the only lever that releases a permanently hung request. Releasing capacity while hidden work runs would allow unbounded oversubscription, and force-killing a channel with live siblings would reintroduce the failure this work removes, so neither is done. Fresh-admission blocking is now scanned across alive channels rather than tracked in a single reference, so a second condemned channel cannot silently displace the first. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(serve): keep the abandoned restore lifecycle off ids it no longer owns Two correctness gaps in the abandoned-restore machinery introduced by this PR, both reported by automated review and both confirmed by mutation testing (each new test fails when its fix is reverted). A caller-supplied `sessionId` is used verbatim by the agent, but `spawnOrAttach` never consulted `inFlightRestores`. A fresh spawn could therefore take an id that a restore still owns, in either lifecycle phase. The consequences were silent: `abandonedRestoreIds` suppresses session updates, guardrail events, and child notifications, so the new session would have registered successfully and then emitted nothing; and a late `settleAbandonedRestore` would have closed and tombstoned it out from under its owner. Such a spawn is now rejected with the same `RestoreInProgressError` and reason the restore path uses, so the caller gets the correct retry hint for whichever phase is holding the id. The cleanup path is guarded independently, because the request-level check only covers the id the caller asked for and a session registers under the id the child returns. An abandoned restore never reaches `createSessionEntry` — the deadline rejects before registration — so any live entry under that id belongs to someone else. Cleanup now detects that and returns without closing or tombstoning, releasing its own bookkeeping instead. The notification fence has no TTL and was only cleared by `markRestoreInFlight`, which covers a subsequent restore and nothing else. `createSessionEntry` now clears it for every registration route, so a legitimate owner of the id is never handed a session that silently drops everything the child sends it. Also tightens two tests that could not observe the values they pin. The SDK default restore timeout admitted any value in (30s, 70s]; it is now split at the exact boundary, so collapsing the default onto the 60s server budget — which would make the client abort race the daemon's own deadline and cost the caller its structured 504 — fails. And the advertised-budget propagation from capabilities through to the SDK call had no live-path assertion; dropping the capabilities argument at the real call site left every existing test green. The `as never` casts are replaced with typed `DaemonCapabilities` values so a field rename fails typecheck. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(serve): let a condemned channel drain without its wedged child Merging main's active-work close protocol (QwenLM#8588) into this PR's abandoned restore bound produced a deadlock that neither side has on its own, and the conflict resolution was committed without running tests. `maybeCloseIdleSession` now routes through `confirmChildUnheld`, which asks the child whether it still holds work before closing a session nobody is attached to. That is right in general and wrong for a channel this PR has already condemned. `restoreSettlementOverdue` and quarantine exist precisely because the child stopped being answerable, and their whole premise is that visible work drains so the channel can be reaped — closing the transport is the only thing that can release a restore we cannot cancel. Making that drain depend on a round trip to the wedged child inverts it: a child stuck in a non-cancellable restore is exactly the one that cannot reply inside `ACTIVE_WORK_CLOSE_TIMEOUT_MS`, so the sessions never close, the channel never drains, the reap never fires, and the bound never takes effect. A channel condemned by the restore lifecycle now skips the round trip and proceeds to local teardown. Nothing is attached to the session by then — `maybeCloseIdleSession` gates on that — and the sibling-safety invariant is untouched: this closes sessions whose clients have already left, it does not force-kill a channel that still has live ones. The regression test drives an overdue channel whose child never answers the close-if-unheld probe and asserts the detach still reaps it. Reverting the guard reproduces the deadlock as a test timeout. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(serve): pin the restore-timeout contract the review found unasserted Automated review identified eleven places where the restore-timeout work's behavior was correct but unpinned — each with a mutation that ships green. Every fix below was verified the same way: apply the mutation, watch the new assertion fail, revert, watch it pass. The timeout path's telemetry had no coverage at all, which is the sharpest gap given that observability is what this work exists to deliver. A shared recorder now asserts the public timeout result and its kill_empty-vs- fence_shared signal, the late arrival, and the cleanup outcome for both the closed and quarantined cases. The deadline timer's cancellation on a successful restore was likewise unpinned: deleting both `clearTimeout` calls kept the whole suite green, while in production the stale timer fires one budget after a successful restore and abandons a live session — fencing its frames, closing its event bus, and emitting a spurious timeout. A success-path test now advances past the deadline and asserts no second public result. Three more bridge assertions proved less than they claimed: the concurrent- restore case never checked that the abandoned restore settles, the workspace-control case never checked that the deferred reap eventually fires, and the resolver never pinned the accepting side of the MAX boundary (a `>` to `>=` mutation rejects the largest legal delay at boot). The workspace-control case also needed a positive channel idle budget, since with the default zero the idle-timer kill substitutes for the reap junction under test; its assertions are rewritten around the derived reap semantics rather than the sticky flag they predate. Outside the bridge: the scheduled-task timeout wiring had no test, so deleting the arguments silently fell back to the helpers' own defaults; the cold restore path never asserted that `live_restore_ms` is absent; the SDK's per-request validation and its over-ceiling clamp were untested; the WebUI watchdog test jumped straight to its own value, staying green for any watchdog at or below it, including the 30s attach value that would recreate the original symptom in the browser; and the two new known error types were unexercised, so dropping either would relabel every restore-timeout and quarantine error as unknown. Two review items are deliberately not taken here and are recorded in the design doc's non-goals instead: transcript materialization is still not separately attributable from `config_setup`, which needs instrumentation inside the core session loader that P1/P2 restructures anyway, and sibling event-loop latency during a large restore remains unmeasured. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(serve): bound the condemned-channel close and complete the fence contract Second automated review round, on the code the first round produced. One Critical and twelve suggestions; all verified by mutation before and after. **The Critical is a regression I introduced.** Letting a condemned channel skip the bounded hold probe routed it into `closeSessionImpl`, whose agent close is unbounded when it throws on failure — so the fix traded a bounded wait on a wedged child for an unbounded one. A settlement-overdue channel with an unresponsive child would hang `detachClient` forever, strand the session in `closing`, never drain, never reap, and 503 every new session until restart: strictly worse than before. `CloseSessionOpts` now carries an `agentCloseTimeoutMs` that the condemned path sets, so a hang lands in the existing unknown-outcome recovery, which kills the channel — the teardown the drain was waiting for. The earlier test missed this because its fake child still answered the plain close; it now answers nothing at all, and asserts the detach itself returns. **The fence was invisible on the transports clients actually use.** `toRpcError` had no `RestoreInProgressError` case, so over acp-http and acp-ws — which SDK negotiation prefers over REST — the fence degraded to an opaque internal 500 with no code, reason, or hint, and the backoff contract this work documents was impossible to honor. **Two retry hints still advertised five seconds for states that outlive a budget.** The restore 504 creates the fence, and quarantine lasts until the channel drains; a fresh-id caller never reaches the 409 that carries the real hint, so its header was the only signal it got. Both now derive from the budget through one shared clamp helper, which also replaces the formula that was inlined in the bridge and gives the documented 5-120s bounds a test. **A spawn collision reported an operation the caller never issued**, naming the restore owner's action as both the active and the requested one and telling the caller to retry an endpoint it never called. The rest: five places still described the initialize-timeout fallback as a plain chain rather than raise-only, contradicting sibling docs shipped in this same PR; the design doc omitted the retry-hint clamp; the protocol reference omitted the new spawn emission site; the error taxonomy omitted `restore_settlement_overdue`, which matters because its audience is monitoring. Test-only gaps: the dynamic 409 had no HTTP-layer coverage, the 120-second cap was unpinned, and the SDK's precedence of an explicit global timeout over the advertised budget was pinned only branch-by-branch. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(serve): preserve restore session ownership handoff Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> --------- Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: qwen-code-dev-bot <qwen-code-dev-bot@users.noreply.github.com>
4 participants
What this PR does
Adds three additive fields to
GET /health?deep=1—activeWork,activeWorkReporting, andactiveWorkStaleMs— and the reporting machinery behind them.activeWorkis true while any managed workspace has an accepted-but-unsettled prompt, a running background Agent, or an Agent terminal notification that is queued, awaiting acceptance, or being processed by its parent continuation. It does not cover background shells, Monitors, workflows, or cron; that exclusion is deliberate and documented, because a controller that readsactiveWork: falseas "nothing at all is running" will be wrong about those.activeWorkReporting(full/partial/none) says how much of that boolean is actually vouched for, andactiveWorkStaleMsis the age of the oldest snapshot it rests on (0when nothing is covered). Without the grade,activeWork: falsecannot be told apart from "no child told me anything", which is the one case where acting on it is unsafe.Reporting. The daemon and ACP child negotiate a private versioned capability through initialization
_meta; the child answers with the cadence it will use and the categories it covers, and each side clamps the other's value into an agreed range. A supported child then publishes channel-wide full snapshots of named holds:{ "v": 1, "seq": 12, "sessions": [ { "sessionId": "…", "holds": [ { "category": "agent", "id": "a1b2" } ] } ] }Holds are derived on every report from the owners of the work — the background-task registry's unfinalized set, the notification queue, the in-flight acceptance and continuation state. There is no acquire/release ledger, because a ledger can miss a release and a leaked hold would pin its Session forever while every snapshot faithfully republished the leak. Full snapshots make a dropped report self-correcting in both directions, and because a report is complete, a Session absent from a fresh snapshot holds nothing on the child side. Absence and reported-with-no-holds are therefore the same fact and take the same path — one that ends in asking the child, never in assuming.
Prompts are deliberately absent from the child's report: the daemon accepts, queues, dispatches, and settles them, so its own count is authoritative and strictly wider (it covers prompts still waiting in the FIFO, which the child cannot see). A snapshot is flushed ahead of the prompt response on the same stream, so a hold the prompt left behind is on the wire before the daemon drops that count.
Cleanup. Automatic cleanup no longer destroys a Session on the strength of a cached snapshot. It asks the child to close only if unheld, and the child answers under its own close gate — with the gate held no prompt is admitted and no automatic turn starts, so a hold cannot appear between the check and the teardown. A refusal hands back the current holds and the daemon adopts them. An unanswered request is neither retried nor assumed: the Session stays, and the next snapshot settles it. The child's gate makes the check atomic on the child side only; the daemon marks the Session in-flight across the whole confirm-then-teardown span, and attach, prompt, and rewind refuse it there exactly as they refuse one already closing. Detach, attach rollback, prompt settle, notification settle, a child reporting itself idle, and the idle reaper's TTL all funnel through one decision point — the reaper included, so a TTL that says the client stopped caring no longer destroys work the child is still running. Explicit close, kill, shutdown, and channel exit keep their force semantics.
Per Session the daemon tracks three states: unsupported (channel never negotiated — contributes nothing, pre-existing cleanup behavior unchanged), unknown (negotiated, not heard from recently enough — reads as retained, and prompts the daemon to ask), and known. Never-reported and gone-quiet are the same state deliberately: a snapshot older than three report intervals is not a report that the Session is idle, so it stops counting as evidence. Reclaiming a channel that has genuinely stopped answering belongs to transport liveness, not here.
Why it's needed
activePromptsreaches zero when a main prompt finishes, even if background Agents started by that prompt are still running. A restart controller that reads zero active prompts as idle can therefore restart the daemon before those Agents finish and before their terminal notifications reach the parent session.activeWorksupplies the missing fact without embedding restart policy in the daemon.Controllers should use:
Dropping the third term makes
activeWork === falseindistinguishable from an unreported channel.What this deliberately does not do
There is no heartbeat watchdog and no channel kill driven by work state. Inferring "this channel is dead" from "one Session stopped reporting" kills every Session on that process, and a host suspend, a long event-loop stall, or a single dropped notification all look identical to a stalled child. Transport/process liveness (channel ping-pong) and stalled-Agent detection (progress-based watchdog) are separate mechanisms tracked as follow-ups under the umbrella issue.
These fields are an observation cache, not a restart lease. Even a fresh, fully-graded, empty answer describes the moment it was sampled; work can start immediately afterwards. The rule above lowers the risk of a wrong restart substantially but does not eliminate it — strict safety needs a prepare-restart fence that stops new work admission, confirms the drain, and only then shuts down. That is out of scope here and stated as such in the docs.
Reviewer Test Plan
activePrompts: 0withactiveWork: trueuntil the Agent terminal notification and its parent continuation settle.cancel()→finalizeCancelled()window (up to the 5s grace timer) and is not reaped with the terminal notification still owed. This is the concrete bug thehasUnfinalizedTasks()predicate fixes.activeWorkReportingisnone, and the shallow health response remains exactly{ "status": "ok" }.activeWorktrue, and that an exception from a later workspace getter still returns503 aggregation_failed.Evidence (Before & After)
N/A — daemon protocol, lifecycle, and health-state change with no TUI presentation change. The
Serve A/Bjob's diff table is the check on the public response shape: it should now show three changed fields rather than one.Tested on
Testing status — please read
Unit suites run locally and green: ACP bridge
489/489, ACP agent383/383, Session534/534, core background-tasks123/123, and the three serve suites1187/1188. That one failure is a pre-existing cross-file flake in the Live Appshot integration tests — it reproduces on the unmodified tree and fails a different test each run.End-to-end has not been run. None of the seven items above were executed manually; they are for the reviewer (and for CI's
Real daemon E2E). Typecheck is clean forcoreandacp-bridge;clihas 22 residual errors, all from unbuilt workspace packages in the local sandbox and none in any file this PR touches.Risk & Scope
AcpSessionBridgegains three required readonly members (activeWork,activeWorkReporting,activeWorkOldestReportAt), which is breaking for any external implementer of that interface; all in-repo implementations are updated. The shallow health response and persisted formats are unchanged. Existing restart controllers may ignore the new fields;activePromptskeeps its exact previous meaning as an independent compatibility signal.Linked Issues
Refs #8586
中文说明
本 PR 做了什么
为
GET /health?deep=1增加三个向后兼容字段 ——activeWork、activeWorkReporting、activeWorkStaleMs—— 以及支撑它们的上报机制。只要任一受管 workspace 存在已接受但未 settle 的 Prompt、运行中的后台 Agent，或正在排队/等待接收/由父 continuation 处理的 Agent 终态通知，
activeWork即为 true。它不包含后台 shell、Monitor、workflow 和 cron；这是有意为之并写进了文档，因为把activeWork: false理解成"什么都没在跑"对这几类就是错的。activeWorkReporting（full/partial/none）说明这个布尔量有多少是真正被担保的，activeWorkStaleMs是它所依赖的最旧快照的年龄（无覆盖时为0）。没有这个分级，activeWork: false就无法与"没有任何子进程告诉过我"区分开，而后者恰恰是唯一不能据此行动的情形。上报机制。 daemon 与 ACP 子进程通过初始化
_meta协商一个私有、带版本的能力；子进程回复它实际采用的上报周期和覆盖的类别，两侧都会把对方给的值钳制到约定区间内。支持该能力的子进程随后发布 channel 级全量快照（见英文段的 JSON 示例）。Hold 每次上报时现算，来源是工作的真正持有者：后台任务注册表的 unfinalized 集合、通知队列、在途的 acceptance 与 continuation 状态。没有 acquire/release 账本 —— 账本可能漏掉一次 release，而泄漏的 hold 会永久钉住 Session，并被每一份快照忠实地重复上报。全量快照让丢失的报文在两个方向上都能自愈，且某个 Session 未出现在新快照中，就是子进程已释放它的正面证据。
Prompt 有意不由子进程上报：daemon 自己负责接受、排队、下发和 settle，它的计数既权威又严格更宽（覆盖了子进程看不到的 FIFO 等待）。快照会在 prompt 响应之前 flush 到同一条流上，确保该 prompt 留下的 hold 先于 daemon 清零计数抵达。
清理路径。 自动清理不再凭缓存快照销毁 Session，而是请求子进程"仅在无 hold 时关闭"，由子进程在自己的 close gate 下作答 —— gate 持有期间不接受新 Prompt、不启动新的自动 turn，因此 hold 不可能在检查与拆除之间出现。被拒绝时返回当前 hold 集合，daemon 予以采纳。请求无应答时既不重试也不假设：Session 保留，由下一份快照裁决。detach、attach 回滚、prompt settle、通知 settle、子进程自报空闲，现在全部汇入同一个决策点，取代原先四处近似重复的逻辑。显式 close、kill、shutdown 和 channel 退出保持强制语义。
daemon 按 Session 维护三态：unsupported（通道从未协商 —— 不贡献任何值，既有清理行为不变）、unknown（已协商但尚未收到上报 —— 视为保留，并促使 daemon 主动询问）、known。
为什么需要它
主 Prompt 结束后
activePrompts即归零，即使它拉起的后台 Agent 仍在运行。把"零活跃 Prompt"直接当作空闲的重启控制器，就可能在这些 Agent 完成、其终态通知抵达父 session 之前重启 daemon。activeWork补上这个缺失的事实，同时不把重启策略嵌进 daemon。控制器判据见英文段的代码块；去掉第三项会让activeWork === false与"未上报的通道"无法区分。明确不做的事
没有心跳看门狗，也没有由工作状态驱动的 channel kill。 从"某个 Session 停止上报"推断"整条通道已死"会连带杀死该进程上的所有 Session，而主机休眠、长时间 event loop 阻塞、单次报文丢失，在观测上与子进程卡死完全一样。传输/进程存活（通道 ping-pong）与 Agent 停滞检测（基于进度的 watchdog）是独立机制，作为后续 PR 由 umbrella issue 跟踪。
这些字段是观测缓存，不是重启租约。即使是新鲜、分级完整、且为空的回答，描述的也只是采样那一刻；工作可能紧随其后开始。上面的判据能显著降低误重启风险，但不能消除 —— 严格安全需要一个 prepare-restart 栅栏：先停止新工作准入，确认 drain，然后才停机。这不在本 PR 范围内，文档中已如实写明。
Reviewer 测试计划
见英文段的 7 条。其中第 2 条（在 detached session 中 cancel 一个后台 Agent，确认它能挺过
cancel()→finalizeCancelled()窗口）针对的是本次修复的一个具体缺陷。测试状态 —— 请务必阅读
本地单测全绿：ACP bridge
489/489、ACP agent383/383、Session534/534、core background-tasks123/123、serve 三套1187/1188。那 1 条失败是 Live Appshot 集成测试里既有的跨文件 flake —— 在未修改的代码树上同样复现，且每次失败的是不同的 test。端到端没有跑过。 上述 7 条没有任何一条被手动执行，它们留给评审者（以及 CI 的
Real daemon E2E）。typecheck 方面core与acp-bridge干净；cli残留 22 条，全部源自本地沙箱中未构建的 workspace 包，无一落在本 PR 改动的文件上。风险与范围
AcpSessionBridge新增三个必需只读成员（activeWork、activeWorkReporting、activeWorkOldestReportAt），对该接口的外部实现者构成破坏性变更；仓库内所有实现均已更新。浅层健康响应与持久化格式不变。现有重启控制器可以忽略新字段；activePrompts保持原有语义，作为独立兼容信号。