fix(core): time out stalled background agents - #11270
Conversation
|
Re-run at the current head. This PR has moved a long way since the last triage pass at Template looks good ✓ — every required heading is present and filled in. Problem: observed, not theoretical. Direction: aligned — a planned layer of #8586, authored by a repo admin. The stack is now retargeted at Size: core paths across four packages. Production logic 944 lines — Approach: both questions I raised last time are answered. Reusing Risk: Stage 1e matches — Moving on to code review. 🔍 中文说明在当前 head 上重跑。自上次 模板完整 ✓ —— 所有必填小节都在且都写了内容。 问题： 已观测到的，不是理论性的。 方向： 对齐——这是 #8586 计划中的一层，作者是仓库 admin。这个栈现在已经把 base 改回 规模： 跨四个包触及核心路径。生产逻辑 944 行—— 方案： 上次我提的两个问题都有了答复。复用 风险： Stage 1e 命中—— 进入代码审查 🔍 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
Code reviewMy independent proposal before reading the diff was the same one I wrote last pass — one idle timer per turn, armed at turn start, reset by any observable progress event, aborting through the existing controller and settling once as The five asks from the last pass are genuinely closed, and I verified the wiring rather than taking the commit messages for it.
I also traced the newest commit ( One finding blocks my approval — the recovery exemption is documented but not implemented
const workOwningGenerations = [...aliveChannels].filter((info) => info.state !== 'dying');
if (workOwningGenerations.length >= 2) {
writeStderrLine(`qwen serve: runtime recycling blocked ${admission} work; generations=…`);
throw new BridgeRuntimeRecyclingError();
}The ceiling therefore blocks That matters because owner.state = 'draining';
if (channelInfo === owner) cancelIdleTimer();
await retireChannelAfterSessionsDrain(owner, …);
if (!owner.isDying) await ensureChannel('recovery');So if a previous recycle's generation is still draining — non-dying, sessions still pinned — the count is already 2 when the second recycle asks for its replacement, and the spawn throws. The recycle is left half-applied: the owner is condemned, no active replacement exists, and because And I could not find anything that bounds the wait. A draining generation is reaped only through I want to be precise about what I am and am not claiming. I read every line cited above and I am confident about the code facts. What I cannot settle statically is reachability: it needs a second unresponsive Agent inside the replacement generation while the first generation has not exited. That may be rare, or there may be a bound I have not found. I am not permitted to execute this PR's code to find out. sequenceDiagram
participant P1 as ProgressWatchdog
participant P2 as TaskRegistry
participant P3 as Session child
participant P4 as BridgeClient daemon
participant P5 as AcpSessionBridge
P1->>P1: deadline expires, abort turn
Note over P1: five second cooperative grace
P1->>P2: failUnresponsive, retains physical slot
P2->>P3: terminal notification marked recordOnly
P3->>P3: persist and display, no model turn
P3->>P4: ext method session runtime recycle
P4->>P5: requestRuntimeRecycle, owner scoped
P5->>P5: mark owner generation draining
P5->>P5: ensureChannel recovery
Note over P5: ceiling counts non dying generations
P5--xP3: throws 503 when two are draining
Note over P5: owner condemned, no active replacement, fresh work 503s
Non-blocking
Files changed (28 of 28 shown)
Test evidenceThis is the PR's own CI at the reviewed commit, fetched through the API. I did not build, run or test any of this PR's code — the review is static, and executed code in this environment could read the agent's write token.
Every Not verified: whether the watchdog fires exactly once end-to-end at the real 15/10-minute constants; whether Sandboxed verification would settle this, and the author has write access so both lanes are open. A run is already in flight on this head and will report into the 中文说明代码审查在读 diff 之前，我的独立方案和上次一样——每个 turn 一个空闲定时器，turn 开始时启动，任何可观察进度事件都重置它，到期后通过已有 controller 中止并只结算一次 上次提的五点都真正关闭了，而且我核对的是接线本身，不是采信 commit message。
我也追踪了最新那个 commit（ 有一个发现让我无法批准——文档承诺的 recovery 豁免并没有实现
这一点之所以要紧，是因为 而且我找不到任何能给这个等待设上限的东西。draining generation 只能通过 我想精确说明我在声称什么、不在声称什么。上面引用的每一行我都读过，代码事实我有信心。我静态无法定论的是可达性：它需要在替代 generation 内出现第二个无响应 Agent，同时第一个 generation 还没有退出。这可能很罕见，也可能存在我没找到的上限。我不被允许执行这个 PR 的代码去验证。 不阻塞合并
测试证据上面是被审查 commit 上本 PR 自己的 CI，通过 API 获取。我没有 build、运行或测试这个 PR 的任何代码——审查是静态的，而且在这个环境里执行代码可能读到 agent 的 write token。 这个 head 上所有 未验证的部分： watchdog 在真实的 15/10 分钟常量下是否端到端只触发一次； 沙箱验证可以定这件事，而且作者有 write 权限，两条通道都可用。当前 head 上已经有一个验证任务在跑，结果会发布到 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
|
Confidence: 3/5 — the watchdog itself is now in good shape and every prior ask is verifiably closed, but I found one daemon-admission path I cannot finish reasoning about statically, and a verification run on this exact head is still in flight. Stepping back. Last pass I scored this 2/5, and the honest summary of what changed is that the author did the work rather than arguing about it. Both correctness paths I named are closed at the wiring level, not just in intent — I checked the So the watchdog — the thing the title names — I would merge. My reservation is entirely in the second mechanism this PR carries, and it is narrow enough that I want to be careful not to overstate it.
Two reasons I am deferring rather than requesting changes. First, I am confident about the code and not about the reachability — it needs a second unresponsive Agent in the replacement generation while the first has not exited, and I am not permitted to run this PR's code to find out whether that is a real shape or a paper one. Second, this PR is at round six, and the project's own rule at this point is to land Critical fixes and defer the rest; stacking a sixth What would settle it, in ascending order of cost: a line in the design doc stating whether recovery is meant to be exempt from the ceiling, and if it is, gating the throw on One last thing, said once and then dropped: at 944 production lines across four packages this PR ships two mechanisms, and the finding above is in the one that is not in the title. R1-7 raised the bundling and the author deferred it under the round cap, which was a legitimate call. I am not reopening it — I am noting that the cost of bundling showed up exactly where it usually does, in the half of the diff that had accumulated the least review attention. @wenshao — deferring to you. You have the harness and the runtime history on this PR, and the open question is a runtime one: can a second recycle land while a previous generation is still draining with pinned sessions, and if it does, does the workspace recover? Note for the record that the deterministic maintainer resolver could not pick a name here — this PR carries no labels, so the label-driven area match finds nothing, and the last-resort "most recent human reviewer" resolves to the author. You are the first-listed 中文说明Confidence: 3/5 —— watchdog 本身现在状态不错，之前提的每一点都已可核实地关闭了，但我发现了一条 daemon 准入路径，静态推理走不完，而且当前这个 head 上的验证任务还在跑。 退一步看整体。上一轮我给的是 2/5，而这次变化的诚实总结是：作者是把活干了，而不是来争论的。我点名的两条正确性路径都在接线层面关闭了，不只是意图层面——我把 所以 watchdog——也就是标题点名的那个东西——我是愿意合的。我的保留意见完全在本 PR 携带的第二套机制里，而且足够狭窄，所以我想小心不要夸大它。
我选择 defer 而不是 request changes，有两个原因。第一，我对代码有信心，对可达性没有——它需要在替代 generation 里出现第二个无响应 Agent，同时第一个还没退出，而我不被允许运行这个 PR 的代码去判断这是真实形态还是纸面形态。第二，这个 PR 已经在第六轮，项目自己的规则是此时只落 Critical、其余延后；在一个已经被 gate 住的 PR 上再叠第六个 按成本从低到高，能定这件事的做法是：在设计文档里写一句 recovery 是否应当豁免于这个上限，如果应当，就把抛错限定在 最后一点，说一次就不再提：944 行生产代码跨四个包，这个 PR 装了两套机制，而上面那个发现位于不在标题里的那一套。R1-7 提过捆绑问题，作者在轮次上限下将其延后，那是合理的处理。我不重开这个话题——我只是指出，捆绑的代价恰好出现在它通常出现的地方：diff 里累积 review 注意力最少的那一半。 @wenshao —— 转交给你。你有 harness，也有这个 PR 的运行时历史，而这个未决问题是一个运行时问题：第二次 recycle 是否可能在上一个 generation 仍带着固定 session 处于 draining 时落地，如果落地了，工作区能否恢复？为了留下记录说明一下：确定性的维护者 resolver 在这里没能选出名字——这个 PR 没有任何 label，所以基于 label 的 area 匹配什么都没匹配到，而最后兜底的"最近一位人类 reviewer"解析出来的是作者本人。你是 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Needs some rethinking before this goes further — see my notes above. 🙏
Two paths abort a healthy Agent: the 15-minute model deadline fires during transport rate-limit backoff (invisible to the watchdog, and llm-chat's delay() honours the abort, so it interrupts a retry that was working as designed), and a tool that executes without streaming output stays queued and gets charged to that same model deadline. Since the contract is settle-once with no retry, a false positive doesn't degrade a background Agent, it destroys it and reports a misleading reason.
Nothing has executed this code either: no tests were added for the new 202-line timing module, and because ci.yml gates pull_request on base main or release/**, no CI ran on a feature-branch base. #11273 is already stacked on this head branch, so both findings propagate upward.
The design is right — I'd rather see it land with those closed than see it rewritten.
— Qwen Code · qwen3.8-max-2026-09-02
Runtime verification of PR #11270 (maintainer review)I built a real local environment and ran this PR end-to-end against a real background Agent, rather than reviewing the diff. Reviewed at Verdict: the mechanism works and is wired correctly, but one scenario is a regression that I think blocks merge — a healthy background Agent sitting in ordinary HTTP 429 transport backoff is killed and settled as
HarnessReal The merge-base arm is the same bundle with A separate instrumented arm subscribes a logger to all eleven events the watchdog listens for, so the state machine's inputs are captured directly rather than inferred. What I verified works
The whole chain holds: watchdog →
The Blocking: a healthy Agent in transport backoff is killedScripted the subagent's provider to answer
One correction to the static review's mechanism. It states that Either emit an event from the Refuted: Finding 2 (silently-executing tools charged to the model deadline)The static review argues that only tools streaming live output ever reach
Worth noting as a fragility rather than a bug: the tool deadline is armed by an event whose stated purpose is UI, and neither the code nor Other measured resultsThe rearm heuristic roughly doubles the deadline per event-loop stall, unbounded. Driving the compiled module with the constants replaced by 9s (drift guard untouched):
Prettier. Confirmed: 4 files fail Typecheck / tests. Confirmed as designed, but undocumented. Both pauses are unbounded — I drove a parked approval for a simulated 60 minutes with no timer running at all. That is probably right (an outer wall-clock limit covers it), but the design doc should say so rather than leave it looking like the original symptom relocated. RecommendationThe problem is real, the architecture is sound, and the plumbing is genuinely careful — the typed abort reason, the
Harness details & limitations
中文版PR #11270 真机验证报告（维护者复核）我没有只读 diff，而是在本地搭了真实环境，用真实后台 Agent 端到端跑了这个 PR。验证提交： 结论：机制本身可用、接线正确，但有一个场景是回归，我认为阻塞合并 —— 一个健康的后台 Agent 只要处在普通的 HTTP 429 传输退避中，就会被杀掉并永久结算为 验证环境用 PR head 构建的真实 merge-base 对照臂是同一个 bundle，把 另有一个插桩臂，把日志订阅到 watchdog 监听的全部 11 个事件上，直接捕获状态机的输入，而不是靠推断。 验证通过的部分
整条链路成立：watchdog → 在 turn controller 上抛
阻塞项：健康的 Agent 在传输退避中被杀让子 agent 的 provider 返回
对静态审查机制描述的一处订正。 该审查称 修法二选一：在 已推翻：Finding 2（静默执行中的工具被算到模型期限）静态审查认为只有会流式输出的工具才会进入
值得作为脆弱点（而非 bug）记录：工具期限是被一个用途为 UI 的事件启动的，代码和 其他实测结果rearm 启发式每遇一次事件循环卡顿就大致翻倍期限，且无上界。 用常量替换为 9s 的编译模块驱动（drift 判据保持原样）：
Prettier。 已确认：PR head 上有 4 个文件 Typecheck / 测试。 确认为设计如此，但缺文档。 两个暂停都是无界的——我让一个停在审批上的工具跑了模拟 60 分钟，期间没有任何定时器在运行。这大概率是对的（外层墙钟限制兜底），但设计文档应当写明，否则读起来像是原来的症状换了个地方继续存在。 建议问题真实，架构合理，管线处理确实细致——带类型的 abort reason、fork 与非 fork 两条分支上的
验证环境的边界： 期限在构建产物里缩短为 12s/8s（仅常量）；rearm 阶梯用的是常量替换为 9s、drift 判据未动的编译模块副本；merge-base 臂是把 |
|
Addressed the two current-head correctness findings in |
… into codex/issue-8586-runtime-generations
…ions' into codex/issue-8586-unresponsive-agent
… into codex/issue-8586-runtime-generations
…ions' into codex/issue-8586-unresponsive-agent
🩺 serve daemon A/BBuilt the PR base vs this PR head ✅ No response changes against the PR base across 12 scenario(s). — Qwen Code · serve A/B |
|
@qwen-code /triage |
|
Sandboxed verification: ❌ not passed — findings reported (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: 52 passed · 0 failed · 52 total Flakiness gate: ✅ 3 changed test file(s) x 5 identical rounds, no divergence 中文 — 判定：❌ 不通过 · 报告了发现（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：52 通过 · 0 失败 · 52 总计 抖动门：✅ 3 changed test file(s) x 5 identical rounds, no divergence Verification reportPR #11270 — deep verificationVerdict: Verified head OID: 中文摘要结论：
Scope chosenCentral claim — an ordinary background Agent turn whose model/control path stops producing events now aborts once at a fixed deadline and settles as Secondary claims — (S1) each executing tool owns an independent 10-minute deadline; queued tools own none; approval and nested external-input waits suspend it. (S2) a run that ignores the cooperative abort escalates after a 5-second grace to Out of scope by choice (listed under Not covered): the bridge-side runtime-generation machinery, the resume/resident paths, typecheck and lint. Central claim and A/BBoth arms load the real compiled Cells printed as they ran:
Timer fidelity: the harness's own scenario clock started at +654 ms (first probe scheduled for 1,000 ms recorded 1,654 ms), so the true lateness of the 15-minute timer was ≈ 27 ms and of the 10-minute timer ≈ 35 ms — well inside the watchdog's 1-second drift-rearm tolerance. The rearm path therefore never fired; Assertion totals per run: head full 31/31, base full 6/6, head smoke 12/12, base smoke 3/3 → 52 pass, 0 fail ( Escalation chain, driven through the real Corrections to the PR descriptionThese are corrections to the description, not requests to change code.
FindingsF1 — a nested external-input park suspends every deadline without the Monitor cross-check the top-level path applies (Suggestion)
Measured ( node tmp/pr11270-verify-20260909-191050/harness-watchdog-ab.mjs \
--tree /__w/qwen-code/qwen-code --arm head --mode full --only nested-input-parkWhat this is not. I did not demonstrate an unbounded end-to-end wedge, and I do not believe this is an oversight. Applying the obvious candidate fix — gating the nested park on What remains for the author to decide is the asymmetry itself: the nested path trusts a flag forwarded from a child's display state, while the top-level path verifies it against the Monitor registry. A nested chain in which each level parks on the next, or a legitimately long-running Monitor, has no deadline at any level. Since closing it means rewriting two of this PR's tests, I am reporting it as a design question rather than shipping a measured fix. F2 — the interactive TUI drops the unresponsive-Agent notification entirely (Suggestion)
registry.setNotificationCallback((displayText, modelText, meta) => {
if (meta?.recordOnly) return;No display, no history entry. The other two consumers do record it: The consequence compounds, because the same PR makes the retained slot block user commands: F3 — the escalation half of the PR is unpinned; two user-visible guards are deletable with everything green (Suggestion)Census over all Mutation matrix (all rows run against
One row needs a caveat: M6c first reported SURVIVED while its own component M6a was KILLED, which is internally inconsistent. I re-ran M6a/M6b/M6c with the mutation verified on disk by sha256 before each run ( G1 and G2 are the rows I would act on: they are the only tests standing between this PR's user-visible blocking behaviour and a silent regression, and both are deletable today with a fully green suite. F4 —
|
| gate | command | result |
|---|---|---|
| core (changed files) | cd packages/core && npx vitest run src/agents/runtime/agent-progress-watchdog.test.ts src/agents/background-tasks.test.ts src/agents/runtime/agent-core.test.ts src/agents/runtime/agent-headless.test.ts src/agents/background-agent-resume.test.ts src/tools/agent/agent.test.ts |
6 files, 605 passed |
| acp-bridge | cd packages/acp-bridge && npx vitest run src/bridge.test.ts src/bridgeClient.test.ts |
2 files, 1052 passed |
| cli (changed files) | cd packages/cli && npx vitest run src/ui/utils/backgroundWorkUtils.test.ts src/nonInteractiveCli.test.ts |
2 files, 188 passed, 1 skipped |
Gate liveness: proven for agent-progress-watchdog.test.ts (M8 killed 1 test) and background-tasks.test.ts (V1 killed 1 test). For backgroundWorkUtils.test.ts, G2 is itself the liveness probe and it survived — the file is green but does not pin the branch this PR added. No liveness proof for the bridge gate.
Methodology
CI merge-ref checkout of refs/pull/11270/merge in node:22-bookworm, 64 cores, load ≈ 35 at start; npm ci and npm run build were already complete at HEAD. The base arm is a scratch worktree at HEAD^1 under tmp/base-tree, whose packages/{core,cli,acp-bridge,web-templates,sdk-typescript}/node_modules were symlinked to the head tree's — a clean control because git diff --name-only HEAD^1..HEAD shows this PR touches no package.json, package-lock.json or pnpm-lock.yaml. Base core rebuilt in place; the build exited 1 on a single pre-existing TS7016 for @lydell/node-pty in src/services/shellExecutionService.ts — a file this PR does not touch, failing on a third-party package's type layout — and still emitted the JS the A/B consumed (dist/src/agents/background-tasks.js), with zero errors in any agents/ file.
Because node_modules/@qwen-code/qwen-code-core is a symlink into the head tree (readlink -f → /__w/qwen-code/qwen-code/packages/core), a naive base harness would have loaded head code and passed both cells. The harness therefore imports each arm's compiled modules by absolute path inside that arm's own worktree, and asserts arm identity from the loaded code itself: base reports ERR_MODULE_NOT_FOUND for agent-progress-watchdog.js and has no failUnresponsive on its registry, head has both. packages/core has no workspace dependencies, so nothing else crosses the boundary.
Every cell ran the real compiled watchdog, a real AgentEventEmitter, a real AbortController and a real BackgroundTaskRegistry (capacity pinned to 1 so slot occupancy is observable through the public canStartBackgroundAgent()), with the watchdog wired exactly as packages/core/src/tools/agent/agent.ts wires it. Timers were real: the six head cells and the base control ran concurrently in two processes over a shared 30 m 40 s window, each with a ref'd 30 s heartbeat because the watchdog's own timers are unref()'d. Mutations were applied to source, run, and restored byte-identically; git status --porcelain is clean apart from tmp/ after every batch. Raw logs, harness scripts and per-run JSON are in this directory; harness-watchdog-ab.mjs, mutation-matrix.mjs, mutation-rerun.mjs, guard-mutations.mjs and probe-unref-exit.mjs are rerunnable as-is.
Flakiness gate log
rounds=5 files=3 skipped=0
file packages/acp-bridge/src/bridge.test.ts: (cd packages/acp-bridge) npx --no-install vitest run ./src/bridge.test.ts
file packages/core/src/agents/background-tasks.test.ts: (cd packages/core) npx --no-install vitest run ./src/agents/background-tasks.test.ts
file packages/core/src/agents/runtime/agent-progress-watchdog.test.ts: (cd packages/core) npx --no-install vitest run ./src/agents/runtime/agent-progress-watchdog.test.ts
per-file results (P=pass F=fail I=infra-exit, one letter per run):
packages/acp-bridge/src/bridge.test.ts: PPPPP
packages/core/src/agents/background-tasks.test.ts: PPPPP
packages/core/src/agents/runtime/agent-progress-watchdog.test.ts: PPPPP
verdict: pass
summary: 3 changed test file(s) x 5 identical rounds, no divergence
--- per-invocation detail (full copy in the artifact) ---
round 1 · packages/acp-bridge/src/bridge.test.ts: P (exit 0)
round 1 · packages/core/src/agents/background-tasks.test.ts: P (exit 0)
round 1 · packages/core/src/agents/runtime/agent-progress-watchdog.test.ts: P (exit 0)
round 2 · packages/acp-bridge/src/bridge.test.ts: P (exit 0)
round 2 · packages/core/src/agents/background-tasks.test.ts: P (exit 0)
round 2 · packages/core/src/agents/runtime/agent-progress-watchdog.test.ts: P (exit 0)
round 3 · packages/acp-bridge/src/bridge.test.ts: P (exit 0)
round 3 · packages/core/src/agents/background-tasks.test.ts: P (exit 0)
round 3 · packages/core/src/agents/runtime/agent-progress-watchdog.test.ts: P (exit 0)
round 4 · packages/acp-bridge/src/bridge.test.ts: P (exit 0)
round 4 · packages/core/src/agents/background-tasks.test.ts: P (exit 0)
round 4 · packages/core/src/agents/runtime/agent-progress-watchdog.test.ts: P (exit 0)
round 5 · packages/acp-bridge/src/bridge.test.ts: P (exit 0)
round 5 · packages/core/src/agents/background-tasks.test.ts: P (exit 0)
round 5 · packages/core/src/agents/runtime/agent-progress-watchdog.test.ts: P (exit 0)
Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention).
— Qwen Code · sandboxed verification
|
⏸️ Deferring to @wenshao — not approving and not requesting changes. The watchdog half of this PR is in good shape: both correctness paths from the last triage pass are closed at the wiring level, the round-2 The one thing I cannot settle statically is in the runtime-generation half: The open question is a runtime one, and I am not permitted to execute this PR's code: can a second recycle land while a previous generation is still draining with pinned sessions, and if it does, does the workspace recover? Full detail and the three non-blocking notes are in the Stage 2 comment above. The Assigned to you so this does not sit invisible in the thread. 中文说明⏸️ 转交给 @wenshao —— 不批准，也不请求修改。 这个 PR 的 watchdog 那一半状态不错：上次 triage 提的两条正确性路径都在接线层面关闭了，第二轮的 我静态无法定论的一点在 runtime generation 那一半： 未决的问题是一个运行时问题，而我不被允许执行这个 PR 的代码：第二次 recycle 是否可能在上一个 generation 仍带着固定 session 处于 draining 时落地，如果落地了，工作区能否恢复？完整细节和三条不阻塞的说明在上面的 Stage 2 评论里。当前 head 上已经在跑的 已经把你设为 assignee，以免这件事在讨论串里无人看见。 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
|
Triage re-run completed without a new review.
The stage comments above were updated with the latest result. View workflow run. 上方各阶段评论已更新为最新结果。查看工作流运行。 |
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.
7 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- D6-10 isChannelLive() doc promises admission, impl reports… — packages/acp-bridge/src/bridgeTypes.ts:2500 — already reported (round-5 deferral list (bridgeTypes.ts:2497))
- D6-11 New public requestRuntimeRecycle member has no caller — packages/acp-bridge/src/bridge.ts:9665 — already reported (comment 3950282825 (R1-11) and the round-5 deferral list)
- D6-12 notified early-return skips the physical slot retention — packages/core/src/agents/background-tasks.ts:920 — already reported (round-5 deferral list (background-tasks.ts:921))
- D6-14 New error class absent from the metrics allowlist and the… — packages/cli/src/serve/acp-http/dispatch.ts:914 — already reported (round-5 deferral list (bridgeErrors.ts:676))
- D6-15 settled TOOL_PROGRESS re-fires with no transition guard — packages/core/src/agents/runtime/agent-core.ts:2069 — already reported (round-5 deferral list (agent-core.ts:2058))
- D6-23 Headless recordOnly filter is untested — packages/cli/src/nonInteractiveCli.ts:2802 — already reported (comment 3958277731 (R3-13))
- P9 untested producer half of the nested-wait bridge — packages/core/src/tools/agent/agent.ts:1573 — already reported (qwen-code-ci-bot stage-2 comment 2026-09-07 and the round-5 deferral list)
Unresolved, please confirm:
- [Critical] R1-25 packages/core/src/agents/background-tasks.ts:1496 — could not be ruled on: its full body sits past the 8,000-char render cap in the recovered context and the remaining budget went to the fan-out and the round-3/4 verification tail
- [Critical] R3-1 packages/core/src/agents/runtime/agent-progress-watchdog.ts:141 — same reason: body not read in full, so the mechanism could not be traced at HEAD
- [Critical] R3-4 packages/core/src/agents/runtime/agent-progress-watchdog.ts:172 — same reason; note R6-1 names this entry as the counter-authority for the approval-park deadline and traces the git history of the suppression list at :104-106
- [Critical] R1-4 packages/core/src/agents/runtime/agent-progress-watchdog.ts:104 — same reason; round 5 already marked this entry fix-induced, and R6-1 reports a new defect at the same suppression list
- [Critical] 4 entries — same reason: body not read in full:
- R4-1 packages/core/src/tools/agent/agent.ts:3791
- R4-6 packages/acp-bridge/src/bridge.ts:3761
- R4-2 packages/acp-bridge/src/bridge.ts:8652
- R4-5 packages/core/src/tools/agent/agent.ts:3559
- [Critical] R4-4 packages/acp-bridge/src/bridge.ts:5649 — same reason; note R6-4 is a new Critical in the same doSpawn guard pair (pre-check at :5478 tightened, post-check at :5654 left two-state)
- [Critical] R5-1 packages/cli/src/acp-integration/session/Session.ts:9954 — same reason; D6-20 and D6-21 are new findings on the same record-only branch but claim different mechanisms
- [Critical] R5-2 packages/core/src/tools/agent/agent.ts:3737 — same reason; N11 (rejected this round) and D6-17 (confirmed) both examined this clause but on different claims
- [Critical] R5-3 packages/core/src/agents/background-agent-resume.ts:1367 — same reason: second location of R5-2, body not read in full
- [Critical] R1-33 packages/cli/src/nonInteractiveCli.ts — round 5 recorded this as unresolved and it stays unresolved: the file is byte-identical between round 5's reviewed commit 85c1c2a and HEAD (git diff --numstat -> 0), so nothing changed that wou…
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally; Test (macos-latest, Node 22.x) and Test (windows-latest, Node 22.x) were also skipped and the same unit suites ran on Linux only, so just the Linux unit ground is covered.
Not reviewed: test-efficacy — the probe harness could not be validated (harnessValidated: null; the probe tree is a fresh detached worktree that borrows node_modules but has no built dist/, so this repo's scripts/vitest-global-setup.js guard refused every run) and no revert, mutant or hunk verdict exists for this diff.
Not explored to full depth (tool budget reached): "agent reverse-audit (round 2)": bridgeClient.ts — I read the new handleExtMethod recycle arm but did not verify ownsSession / resolveEntry / RequestError.invalidParams internals, nor whe…; "agent reverse-audit (round 2)": nonInteractiveCli.ts — I read the localQueue push and the modelBatch filter, but did not trace emitNotificationToSdk to determine whether the SDK frame …; "agent reverse-audit (round 2)": Session.ts — I did not verify whether a recordOnly item with continuesTodoStopGuardWorkChain === false can be starved indefinitely by #nextNotificationQueu…; "agent reverse-audit (round 3)": I did not verify empirically that any real provider returns a Retry-After beyond 6 h — finding 1's trigger is established from retryPolicy.ts:106 's Date.pa…; "agent reverse-audit (round 3)": I did not determine whether this.eventEmitter (used by the background path's setupEventListeners at agent.ts:2765 ) is the same object as bgEmitter = bgSu….
Not reviewed: reverse audit — stopped before round 5 by the review time budget.
Deferred under the convergence posture (round 6, not a blocker) — recorded, not requested in this round; 7 Critical(s) among them are deferred by their axes — fails-closed on new surface, where no wrong result is certified and the merge base had neither the surface nor the defect — and remain follow-up work recorded in the findings artifact:
packages/acp-bridge/src/bridge.ts:3780 (+1 locations) — [probe] Critical [fails-closed] [new-surface] Recycle condemns the owner before checking admissibilitypackages/acp-bridge/src/bridge.ts:3820 (+1 locations) — [probe] Critical [fails-closed] [new-surface] Emptied draining generation is never reapedpackages/acp-bridge/src/bridge.ts:14435 (+1 locations) — [probe] Critical [fails-closed] [new-surface] Transient draining state reported as a non-retryable 500packages/cli/src/ui/hooks/use-llm-stream.ts:6298 (+1 locations) — [probe] Critical [fails-closed] [new-surface] Interactive TUI discards the escalated terminal notificationpackages/core/src/agents/background-tasks.ts:943 (+1 locations) — [probe] Critical [fails-closed] [new-surface] Retained slot is releasable only by the turn that never…packages/core/src/agents/runtime/agent-core.ts:1306 (+1 locations) — [probe] Critical [fails-closed] [new-surface] External-input latch leaves the next model call with no…packages/core/src/tools/agent/agent.ts:1570 (+1 locations) — [review] Critical [fails-closed] [new-surface] Sticky nested park flags disarm the parent watchdogdocs/design/background-agent-progress-watchdog.md:22 (+1 locations) — [review] Doc discloses the uncovered retry case but does not size itdocs/design/background-agent-runtime-generations.md:13 (+1 locations) — [probe] 'until one exits' has no mechanism behind itpackages/acp-bridge/src/bridge.ts:1092 (+1 locations) — [review] state field inserted between the isDying contract and fieldpackages/acp-bridge/src/bridge.ts:4603 (+3 locations) — [probe] New generation machinery and its 503 ship with zero testspackages/cli/src/acp-integration/session/Session.ts:9778 (+1 locations) — [review] A rejected persist loses the kill from the transcript…packages/cli/src/acp-integration/session/Session.ts:9958 (+1 locations) — [probe] Daemon-side recordOnly invariant is unpinnedpackages/cli/src/acp-integration/session/Session.ts:9958 (+1 locations) — [review] recordOnly display rides the automatic-turn admission gatespackages/cli/src/ui/utils/backgroundWorkUtils.ts:100 (+1 locations) — [probe] Retained-slot branch of the blocking-work list untestedpackages/cli/src/ui/utils/backgroundWorkUtils.ts:108 (+1 locations) — [probe] 'still stopping' marker is clipped by the width clamppackages/core/src/agents/background-agent-resume.ts:1287 (+1 locations) — [probe] Model deadline stays armed across silent hook phasespackages/core/src/agents/background-tasks.ts:929 (+1 locations) — [probe] Escalation sidecar patch drops the terminal summarypackages/core/src/agents/background-tasks.ts:943 (+1 locations) — [probe] Retained-slot release and accounting sites are untestedpackages/core/src/agents/background-tasks.ts:1873 (+1 locations) — [review] pruneTerminalEntries contract docstring is now false- …and 7 more (see the run report)
Convergence: round 6 posted 6 inline comment(s), 5 of them reported for the first time; the previous round posted 13 (3 new). Findings keep coming back to the same files: packages/core/src/agents/runtime/agent-progress-watchdog.ts (findings in rounds 1, 3; 2 more now); packages/core/src/tools/agent/agent.ts (findings in rounds 4, 5; 1 more now); packages/acp-bridge/src/bridge.ts (findings in round 4; 1 more now). A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. (Observation only — nothing was withheld from this review because of this observation.)
Mechanism health: this round did not close cleanly, so it withholds the incremental anchor — and the round it recovered had no anchor this round could use either — none at all, one with no certifier, one certified by an identity other than the one this round runs under, or one this round's fetch refused or resolved to the head — so the next review re-reads the whole diff unless recovery grafts an earlier own anchor that the round running it can use onto the complete work list this round leaves behind, and keeps doing so until a round's marker carries an anchor again or a graft lands that the round running it can use. (Stated, not acted on — this changes nothing about what the round posts.)
[Critical] R6-5 [certifies-falsely] [new-surface] packages/core/src/agents/background-tasks.ts:1489 — The whole retainsPhysicalSlot protection is unreachable whenever anything other than the watchdog aborts the turn first — the escalation is armed only inside abort(), whose first statement early-returns on an already-aborted signal, and a user cancel aborts the same controller. retainsPhysicalSlot has exactly one write site (failUnresponsive, background-tasks.ts:928), whose only callers are the two watchdog escalation callbacks; the escalation is armed only inside abort(), whose first statement is if (disposed || controller.signal.aborted) return; (agent-progress-watchdog.ts:68). A background agent's tool ignores AbortSignal — the exact premise the field documents. The user stops it via task_stop//tasks -> cancel() (task-stop.ts:76) -> entry.abortController.abort() + status cancelled + a CANCEL_GRACE_MS timer, and CANCEL_GRACE_MS = 5000 against a 10 min tool / 15 min model deadline, so cancel-first is the NORMAL ordering — a human stops a stuck agent long before 15 minutes of silence. Five seconds later finalizeCancellationIfPending -> emitNotification sets notified, so getRunningBackgroundCount's (status === 'cancelled' && !entry.notified) goes false with retainsPhysicalSlot never set, and its trailing drainWaitQueue() EAGERLY ADMITS A QUEUED LAUNCH ON TOP OF THE STILL-EXECUTING RUN, past maxConcurrentBackgroundAgents; hasRunningTasks() goes false, so /clear, /new, /resume and /branch pass hasBlockingBackgroundWork() and reset() erases the only remaining owner of a live run. Every later watchdog deadline hits abort()'s early-return, so failUnresponsive never runs: no recordOnly notification, no sidecar patch, and no runtime recycle request for the wedged generation — the PR's headline remedy. The added test encodes only the reverse ordering, which is why it is green. This blocker could not be posted inline: its anchor line collides with an existing comment at the same location (3958277693, qwen-code-ci-bot, [Suggestion] R3-9) that reports a different claim, so the overlap rule dropped the inline copy; it is carried here so the blocker is not lost. Fix constraint: hasRunningTasks() must keep excluding a cancelled-and-notified entry that has NOT retained a slot — packages/cli/src/ui/utils/backgroundWorkUtils.ts:18-23: "Gating on the unfinalized set made /new silently no-op when typed in the window between cancel and finalize (issue #5949)." So the fix must set the flag on the escalation path, not widen the hasRunningTasks()/getRunningBackgroundCount predicates to status === 'cancelled'. Fix witness: packages/core/src/agents/background-tasks.test.ts — register a backgrounded agent, call registry.cancel(id), advance past CANCEL_GRACE_MS (so notified is true and the slot is released), then invoke the escalation the way the watchdog would and assert entry.retainsPhysicalSlot === true, registry.hasRunningTasks() === true and getRunningBackgroundCount unchanged; paired with an agent-progress-watchdog.test.ts case that aborts the controller externally, advances past the model deadline + 5 s, and asserts onUnresponsive was called. Both go red with if (disposed || controller.signal.aborted) return; restored as the whole-body early return.
中文说明
仅完成部分审查，审查缺口已披露。
本轮确认的 7 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未决，请确认：共 13 条（原文未翻译，列表见上方英文部分）。
未审查（原文为英文）：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally; Test (macos-latest, Node 22.x) and Test (windows-latest, Node 22.x) were also skipped and the same unit suites ran on Linux only, so just the Linux unit ground is covered.
未审查（原文为英文）：test-efficacy — the probe harness could not be validated (harnessValidated: null; the probe tree is a fresh detached worktree that borrows node_modules but has no built dist/, so this repo's scripts/vitest-global-setup.js guard refused every run) and no revert, mutant or hunk verdict exists for this diff.
未探索到全部深度（达到工具调用预算）："agent reverse-audit (round 2)"：bridgeClient.ts — I read the new handleExtMethod recycle arm but did not verify ownsSession / resolveEntry / RequestError.invalidParams internals, nor whe…；"agent reverse-audit (round 2)"：nonInteractiveCli.ts — I read the localQueue push and the modelBatch filter, but did not trace emitNotificationToSdk to determine whether the SDK frame …；"agent reverse-audit (round 2)"：Session.ts — I did not verify whether a recordOnly item with continuesTodoStopGuardWorkChain === false can be starved indefinitely by #nextNotificationQueu…；"agent reverse-audit (round 3)"：I did not verify empirically that any real provider returns a Retry-After beyond 6 h — finding 1's trigger is established from retryPolicy.ts:106 's Date.pa…；"agent reverse-audit (round 3)"：I did not determine whether this.eventEmitter (used by the background path's setupEventListeners at agent.ts:2765 ) is the same object as bgEmitter = bgSu…。
未审查：反向审计——评审时间预算不足，未能开始第 5 轮。
收敛姿态下延后（第 6 轮，非阻断）——已记录，本轮不要求修改；其中 7 条 Critical 按其失败方向与对照基线延后——fails-closed 且 new-surface：未认证任何错误结果，且 merge base 既无该功能面也无该缺陷——作为后续工作记录在 findings 工件中：共 27 条（原文未翻译，列表见上方英文部分）。
收敛情况：第 6 轮发布了 6 条行内评论，其中 5 条是首次提出；上一轮发布了 13 条（其中 3 条首次提出）。发现反复回到同一批文件：packages/core/src/agents/runtime/agent-progress-watchdog.ts（第 1、3 轮已出过发现，本轮又有 2 条）；packages/core/src/tools/agent/agent.ts（第 4、5 轮已出过发现，本轮又有 1 条）；packages/acp-bridge/src/bridge.ts（第 4 轮已出过发现，本轮又有 1 条）。一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。（仅为观察——本轮评审未因此扣留任何内容。）
机制健康：本轮未能干净收尾，因而扣留了增量锚点，而它恢复到的那一轮也没有留下本轮可用的锚点——要么完全没有、要么没有认证者、要么由本轮运行身份之外的身份认证、要么被本轮的获取拒绝或解析为头提交——因此下一次评审将重读整个 diff，除非恢复流程把本轮能使用的更早自有锚点嫁接到本轮留下的完整工作清单上；并会一直如此，直到某一轮的标记重新带上锚点，或落地的嫁接能被运行该轮的评审使用。（仅陈述，不据此行动——这不改变本轮发布的任何内容。）
[Critical] R6-5 [certifies-falsely] [new-surface] packages/core/src/agents/background-tasks.ts:1489 — The whole retainsPhysicalSlot protection is unreachable whenever anything other than the watchdog aborts the turn first — the escalation is armed only inside abort(), whose first statement early-returns on an already-aborted signal, and a user cancel aborts the same controller. retainsPhysicalSlot has exactly one write site (failUnresponsive, background-tasks.ts:928), whose only callers are the two watchdog escalation callbacks; the escalation is armed only inside abort(), whose first statement is if (disposed || controller.signal.aborted) return; (agent-progress-watchdog.ts:68). A background agent's tool ignores AbortSignal — the exact premise the field documents. The user stops it via task_stop//tasks -> cancel() (task-stop.ts:76) -> entry.abortController.abort() + status cancelled + a CANCEL_GRACE_MS timer, and CANCEL_GRACE_MS = 5000 against a 10 min tool / 15 min model deadline, so cancel-first is the NORMAL ordering — a human stops a stuck agent long before 15 minutes of silence. Five seconds later finalizeCancellationIfPending -> emitNotification sets notified, so getRunningBackgroundCount's (status === 'cancelled' && !entry.notified) goes false with retainsPhysicalSlot never set, and its trailing drainWaitQueue() EAGERLY ADMITS A QUEUED LAUNCH ON TOP OF THE STILL-EXECUTING RUN, past maxConcurrentBackgroundAgents; hasRunningTasks() goes false, so /clear, /new, /resume and /branch pass hasBlockingBackgroundWork() and reset() erases the only remaining owner of a live run. Every later watchdog deadline hits abort()'s early-return, so failUnresponsive never runs: no recordOnly notification, no sidecar patch, and no runtime recycle request for the wedged generation — the PR's headline remedy. The added test encodes only the reverse ordering, which is why it is green. This blocker could not be posted inline: its anchor line collides with an existing comment at the same location (3958277693, qwen-code-ci-bot, [Suggestion] R3-9) that reports a different claim, so the overlap rule dropped the inline copy; it is carried here so the blocker is not lost. Fix constraint: hasRunningTasks() must keep excluding a cancelled-and-notified entry that has NOT retained a slot — packages/cli/src/ui/utils/backgroundWorkUtils.ts:18-23: "Gating on the unfinalized set made /new silently no-op when typed in the window between cancel and finalize (issue #5949)." So the fix must set the flag on the escalation path, not widen the hasRunningTasks()/getRunningBackgroundCount predicates to status === 'cancelled'. Fix witness: packages/core/src/agents/background-tasks.test.ts — register a backgrounded agent, call registry.cancel(id), advance past CANCEL_GRACE_MS (so notified is true and the slot is released), then invoke the escalation the way the watchdog would and assert entry.retainsPhysicalSlot === true, registry.hasRunningTasks() === true and getRunningBackgroundCount unchanged; paired with an agent-progress-watchdog.test.ts case that aborts the controller externally, advances past the model deadline + 5 s, and asserts onUnresponsive was called. Both go red with if (disposed || controller.signal.aborted) return; restored as the whole-body early return.
— qwen3.8-max via Qwen Code /review (v0.23.2)
|
|
||
| const MODEL_CONTROL_PROGRESS_TIMEOUT_MS = 15 * 60_000; | ||
| const TOOL_PROGRESS_TIMEOUT_MS = 10 * 60_000; | ||
| const UNRESPONSIVE_ABORT_GRACE_MS = 5_000; |
There was a problem hiding this comment.
[Critical] R6-2: [certifies-falsely] [new-surface] The 5-second escalation grace is measured against the entire cooperative unwind, so an agent that obeys the abort but takes longer than 5 s to settle is force-failed, its terminal payload is discarded, and a full runtime-generation recycle is spent on an agent that was cooperating.
abort() calls controller.abort(error) and arms the escalation. The abort then has to propagate out through the model stream, the CoreToolScheduler cancellation of the very tool that has been unresponsive for ten minutes, await cleanupWorktreeIsolation() (a git worktree removal), patchAgentMeta and span recording before bgBody resolves and its .finally() reaches disposeWatchdog(). The population that triggers a tool-phase watchdog is strongly correlated with the population that cannot unwind in 5 s — a hung shell child, an MCP call awaiting a dead peer, a fetch with no timeout.
When the unwind lands at, say, T+8 s: failUnresponsive has already set status='failed', notified=true, retainsPhysicalSlot=true, patched the sidecar lastError with the bare timeout string and emitted a recordOnly notification; Session.#recordUnresponsiveAgentNotification has already fired qwen/control/session/runtime/recycle, so the daemon marks the generation draining and spawns a replacement ACP child. agent.ts:3765-3768 then computes errorMsg = baseErrorMsg + wtSuffix and returns, discarding it; agent.ts:3654 computes finalText (with wtSuffix) and breaks, discarding it. registry.complete/finalizeCancelled/fail all short-circuit on entry.notified, so nothing re-publishes. The user's failure text and the persisted lastError say only "made no progress for 600000ms" and never name the preserved worktree path/branch — precisely the outcome the surviving comment at agent.ts:3752-3757 warns against ("an agent that crashed mid-edit would have its worktree preserved on disk but the user would never see its location in the failure notification — they would assume nothing was left behind").
Witness: the conditional itself ("obeys the abort but takes >5 s") is measured by probe against the real watchdog and registry; the trigger realism is corroborated externally by the maintainer's own harness at anchor 25df3993, which measured a settle 33 s after the watchdog abort for a run that was not ignoring the signal but sleeping in a delay that does not observe it (+0.21s <429 Retry-After: 45> … +45.40s settled: failed, 12 s deadline; "the sleep is not interrupted … The retry loop only observes the abort after the full sleep elapses"). baseline: new-surface settled against the merge base: attachAgentProgressWatchdog / failUnresponsive appear in 0 files at 558d7f2290. Not measured: how often a real worktree-cleanup unwind exceeds 5 s on production disks — that frequency is the residual uncertainty.
Suggested fix: do not let the escalation be irreversible on a cooperative settlement. Either widen the grace well past the cooperative-unwind budget (worktree cleanup + scheduler cancellation), or have the terminal paths re-publish the real payload when the entry is retainsPhysicalSlot but the run did settle — in agent.ts / background-agent-resume.ts, when retainsPhysicalSlot is set, still write finalText/errorMsg (including wtSuffix) into entry.error and patchAgentMeta({ lastError }) before break/return, and skip the recycle request when the turn settled rather than hung.
Any fix must respect this: const CANCEL_GRACE_MS = 5000; (packages/core/src/agents/background-tasks.ts:157), used at :1011 — the registry already treats "5 s after an abort with no natural settlement" as the normal case needing only a deferred finalizeCancellationIfPending() fallback, and its comment states the fallback exists for "the rare case where a tool ignores AbortSignal and bgBody never settles". A new bound must not reuse that same 5 s budget for a strictly harsher consequence (terminal failed + retained slot + runtime recycle) on the same population.
Please add the test that pins this: in packages/core/src/tools/agent/agent.test.ts, a background-agent case that stalls a tool past the tool deadline, lets the turn body settle cooperatively after the grace window, and asserts (1) the registry entry's error still contains the preserved-worktree suffix and (2) no qwen/control/session/runtime/recycle was requested. Removing the fix must turn both red. The existing background-tasks.test.ts case calls failUnresponsive directly and so cannot see this path.
中文说明
[Critical] R6-2：5 秒升级宽限期是按整个协作式收尾过程来计量的，因此一个服从了中止、只是收尾耗时超过 5 秒的 Agent 会被强制判为失败，它的终态内容被丢弃，而且一次完整的 runtime generation 回收被花在了一个本来在配合的 Agent 上。
abort() 会调用 controller.abort(error) 并武装升级定时器。随后这个中止必须依次穿过：模型流、CoreToolScheduler 对那个已经十分钟无响应的工具本身的取消、await cleanupWorktreeIsolation()（一次 git worktree 删除）、patchAgentMeta 以及 span 记录，bgBody 才会 resolve，其 .finally() 才能执行到 disposeWatchdog()。会触发工具阶段 watchdog 的那类运行，与无法在 5 秒内完成收尾的那类运行高度重合 —— 卡死的 shell 子进程、等待已死对端的 MCP 调用、没有超时的 fetch。
假设收尾在 T+8 秒才落地：此时 failUnresponsive 已经设置 status='failed'、notified=true、retainsPhysicalSlot=true，已用光秃秃的超时字符串写入 sidecar 的 lastError，并发出了 recordOnly 通知；Session.#recordUnresponsiveAgentNotification 也已发出 qwen/control/session/runtime/recycle，于是 daemon 把该 generation 标记为 draining 并派生出替代的 ACP 子进程。接着 agent.ts:3765-3768 算出 errorMsg = baseErrorMsg + wtSuffix 却直接 return 把它丢弃；agent.ts:3654 算出带 wtSuffix 的 finalText 却break 把它丢弃。registry.complete/finalizeCancelled/fail 都因 entry.notified 短路，所以没有任何路径会重新发布。用户看到的失败文本与持久化的 lastError 只有“made no progress for 600000ms”，永远不会提到被保留的 worktree 路径与分支 —— 这正是 agent.ts:3752-3757 那段留存注释所警告的结果（“一个在编辑中途崩溃的 agent，其 worktree 会被保留在磁盘上，但用户永远看不到它在失败通知中的位置 —— 他们会以为什么都没留下”）。
证据：“服从中止但收尾超过 5 秒”这一条件本身已由探针在真实 watchdog 与 registry 上实测；触发的现实性另有外部佐证 —— 维护者在 anchor 25df3993 的实测环境中，测得一个并未忽略信号、只是睡在一个不观察该信号的 delay 里的运行，在 watchdog 中止后 33 秒才结算（+0.21s <429 Retry-After: 45> … +45.40s settled: failed，期限 12 秒；“该 sleep 不会被打断……重试循环只有在整段 sleep 结束后才会观察到中止”）。baseline: new-surface 已对合并基线核实：attachAgentProgressWatchdog / failUnresponsive 在 558d7f2290 中出现于 0 个文件。未实测部分：真实磁盘上 worktree 清理收尾超过 5 秒的频率 —— 这是残余不确定性所在。
修复建议：不要让升级在“协作式结算”的情形下不可逆。要么把宽限期放宽到明显超过协作收尾所需预算（worktree 清理 + 调度器取消），要么在终态路径上，当条目处于 retainsPhysicalSlot 但运行确实结算了时重新发布真实内容 —— 即在 agent.ts / background-agent-resume.ts 中，当 retainsPhysicalSlot 已设置时，在 break/return 之前仍把 finalText/errorMsg（含 wtSuffix）写入 entry.error 并 patchAgentMeta({ lastError })；并在“turn 是结算而非挂死”的情况下跳过回收请求。
修复必须遵守：const CANCEL_GRACE_MS = 5000;（packages/core/src/agents/background-tasks.ts:157，在 :1011 使用）—— registry 已经把“中止后 5 秒仍未自然结算”视为正常情况，只需要一个延后的 finalizeCancellationIfPending() 兜底，其注释也写明该兜底是为“工具忽略 AbortSignal 且 bgBody 永不结算的罕见情况”准备的。新的上界不得把同一个 5 秒预算，用于对同一类运行施加严格更重的后果（终态 failed + 保留槽位 + runtime 回收）。
请补上能钉住这一点的测试：在 packages/core/src/tools/agent/agent.test.ts 中增加一个后台 agent 用例 —— 让某个工具停滞超过工具期限，让 turn 主体在宽限期之后才协作式结算，并断言（1）registry 条目的 error 仍包含被保留 worktree 的后缀，（2）没有发出 qwen/control/session/runtime/recycle。移除修复时两者都必须变红。现有的 background-tasks.test.ts 用例是直接调用 failUnresponsive 的，因此看不到这条路径。
— qwen3.8-max via Qwen Code /review (v0.23.2)
There was a problem hiding this comment.
Not fixing — escalating. This is one of three findings (R6-1, R6-2, R6-3) that all change watchdog/abort semantics, which is the human-gated class on a PR whose entire subject is timeout semantics. Verified at head:
UNRESPONSIVE_ABORT_GRACE_MS = 5_000(agent-progress-watchdog.ts:19) is a single constant, and it is measured against the whole cooperative unwind — which includesCoreToolSchedulercancelling the very tool that has already been unresponsive for ten minutes, pluscleanupWorktreeIsolation()(agit worktreeremoval),patchAgentMetaand span recording beforebgBody's.finally()reachesdisposeWatchdog().
The population that triggers a tool-phase watchdog is correlated with the population that cannot unwind in 5 s, so the premise is sound rather than hypothetical.
The gate condition: making the escalation reversible, or lengthening the grace, is a decision about who owns the terminal record. Today failUnresponsive sets notified = true and every settle path (complete / finalizeCancelled / fail) short-circuits on it, so a late cooperative unwind computes errorMsg and finalText and then discards them. Fixing that means choosing whether the watchdog or the agent body owns the terminal payload — an ownership change in the settle path — and it interacts with the runtime recycle that Session.#recordUnresponsiveAgentNotification has already fired by then, so the answer also decides whether a spent generation is recoverable.
That is concurrency/settle semantics on the exact mechanism this PR introduces, and the report itself flags the residual uncertainty honestly (how often a real worktree cleanup exceeds 5 s on production disks was not measured). Not a closeout-pass decision. Leaving unresolved.
There was a problem hiding this comment.
[Critical] R6-2: [certifies-falsely] [new-surface] The 5-second escalation grace is measured against the entire cooperative unwind, so an agent that obeys the abort but takes longer than 5 s to settle is force-failed, its terminal payload is discarded, and a full runtime-generation recycle is spent on an agent that was cooperating.
Failure scenario: abort() calls controller.abort(error) and immediately arms a 5 s escalation. The unwind it is racing includes CoreToolScheduler cancelling a tool that has already been unresponsive for ten minutes, plus cleanupWorktreeIsolation and the registry settle path. An agent that obeys the abort but needs longer than 5 s to settle is recorded as failed/unresponsive, its real terminal payload is discarded, and Session.ts requests a runtime-generation recycle for a runtime that was cooperating.
Witness:
Read at HEAD 8b23578fc9: `const UNRESPONSIVE_ABORT_GRACE_MS = 5_000;` (packages/core/src/agents/runtime/agent-progress-watchdog.ts:19) is still a single constant, and abort() (:67-78) calls controller.abort(error) then armEscalation(), which schedules UNRESPONSIVE_ABORT_GRACE_MS with armEscalation itself as the re-arm callback — so the grace spans the whole cooperative unwind and re-fires. The author's own reply on this thread verified the same anchor at head and answered 'Not fixing — escalating'; a reply does not retire a blocker, and no commit changed it.
Suggested fix: Give the tool deadline the same extension the model deadline has. Carry the nested retry delay out on the task_execution display (set it in the MODEL_RETRY handler next to forwardProgress, clear it on ROUND_START/STREAM_TEXT), surface it as an optional retryDelayMs on AgentToolProgressEvent in agent-core.ts's outputUpdateHandler, and in armTool schedule TOOL_PROGRESS_TIMEOUT_MS + Math.min(event.retryDelayMs ?? 0, MAX_RETRY_DEADLINE_EXTENSION_MS), re-arming through a closure that preserves the granted delay exactly as armModel does.
The fix must not violate this existing fact: The tool-side extension must stay within the same bound the model side uses — const MAX_RETRY_DEADLINE_EXTENSION_MS = 6 * 60 * 60_000; (packages/core/src/agents/runtime/agent-progress-watchdog.ts:19) — which is deliberately equal to const PERSISTENT_CAP_MS = 6 * 60 * 60 * 1000; // 6 hours — absolute single wait cap (packages/core/src/utils/retry.ts:25). A smaller cap re-int
Acceptance criterion: packages/core/src/agents/runtime/agent-progress-watchdog.test.ts — a case mirroring the existing 'keeps a provider-backoff deadline extension across a clock-drift re-arm': toolCall('t1'); toolProgress('t1'); toolProgress('t1', { retryDelayMs: 30 * 60_000 }), then assert abortPhase() is still undefined at TOOL_TIMEOUT_MS + 1 and becomes 'tool' at `TOOL_TIMEOUT_MS + Please confirm the mutation that proves it (remove the guard, run that test, confirm it goes red).
中文说明
UNRESPONSIVE_ABORT_GRACE_MS = 5_000 仍是单一常量，且 abort() 在 controller.abort(error) 之后立即装配升级定时器，并以 armEscalation 自身作为重新装配回调。因此这 5 秒宽限覆盖的是整个协作式收尾过程（包括调度器取消那个已无响应十分钟的工具、worktree 清理、registry 落定）。一个服从中止但收尾超过 5 秒的 Agent 会被判定为 unresponsive、真实终止负载被丢弃，并为一个本来在协作的运行时花费一次完整的 generation 回收。
— qwen3.8-max via Qwen Code /review (v0.23.3)
There was a problem hiding this comment.
Real bug, but the fix is a design decision I did not want to settle unilaterally in this pass. The escalation grace only resets on dispose() (agent settle). Re-arming on settle signals (TOOL_RESULT / TOOL_PROGRESS(settled)) does not cover the motivating case — a single tool whose cancellation takes >5s emits its settle signal after the grace has already fired. Re-arming on all activity (e.g. STREAM_TEXT) defeats the escalation for an abort-ignoring agent that keeps streaming. Which signals reset the grace, and whether to add a bounded total grace, needs a decision. Left unresolved.
There was a problem hiding this comment.
[Critical] R6-2: [certifies-falsely] [new-surface] Still stands — the 5-second escalation grace is measured against the entire cooperative unwind, so an agent that obeys the abort but takes longer than 5 s to settle is force-failed, its terminal payload is discarded, and a full runtime-generation recycle is spent on an agent that was cooperating
abort() calls controller.abort(error) and immediately arms the 5 s escalation with armEscalation itself as the re-arm callback, so the grace spans the whole unwind — CoreToolScheduler cancelling a tool already unresponsive for ten minutes, cleanupWorktreeIsolation, and the registry settle path. An agent that obeys but needs longer than 5 s is recorded failed/unresponsive, its real terminal payload is discarded, and Session.ts requests a runtime-generation recycle for a runtime that was cooperating.
Witness:
not run — scratch-tree refused (available: false, repo-local git include chain); settled by quoted lines at HEAD 90e2d6f7ae: `const UNRESPONSIVE_ABORT_GRACE_MS = 5_000;` (agent-progress-watchdog.ts:19) and abort() at :67-78 calling controller.abort(error) then armEscalation(), which schedules UNRESPONSIVE_ABORT_GRACE_MS with armEscalation as its own re-arm callback. The author replied "Not fixing — escalating"; a reply does not retire a blocker and no commit changed the constant.
Suggested fix:
Give the escalation a settle signal to stand down on (re-arm on TOOL_RESULT / TOOL_PROGRESS(settled) / dispose) or make the grace measurable against the unwind rather than a fixed 5 s, and record which signals count.
The fix must not violate this existing fact: UNRESPONSIVE_ABORT_GRACE_MS = 5_000 (agent-progress-watchdog.ts:19) and CANCEL_GRACE_MS = 5000 (background-tasks.ts:157) are the two 5 s windows this PR adds; a stand-down must not let an abort-ignoring run escape escalation entirely, which is the case the constant exists for.
Please confirm the mutation that proves it — packages/core/src/agents/runtime/agent-progress-watchdog.test.ts — a case that aborts, emits a settle signal after more than UNRESPONSIVE_ABORT_GRACE_MS, and asserts onUnresponsive was never called. It goes red if the stand-down is removed.
中文说明
UNRESPONSIVE_ABORT_GRACE_MS = 5_000 仍是单一常量，abort() 在 controller.abort(error) 之后立刻装配升级定时器，并以 armEscalation 自身作为重新装配回调。因此这 5 秒宽限覆盖的是整个协作式收尾过程——包括调度器取消那个已无响应十分钟的工具、cleanupWorktreeIsolation，以及注册表落定路径。
一个服从中止、但收尾超过 5 秒的 Agent 会被判定为 unresponsive：真实终止负载被丢弃，并为一个本来在协作的运行时花费一次完整的 generation 回收。
作者已回复“不修复——上报维护者裁决”。回复本身不能撤销阻塞项，且没有提交改动这段代码。
— qwen3.8-max via Qwen Code /review (v0.23.3)
There was a problem hiding this comment.
Design-level — left open deliberately, not silently resolved. The escalation grace is measured against the entire cooperative unwind, and narrowing that means re-specifying watchdog semantics: whether observed progress restarts the grace, what counts as "settling", and how the terminal payload survives a force-fail. That is a semantics change to agent-progress-watchdog.ts, beyond a mechanical guard fix, so it is not being landed inside this fix round. Evidence anchor: packages/core/src/agents/runtime/agent-progress-watchdog.ts:19 (UNRESPONSIVE_ABORT_GRACE_MS) plus the drift-guarded re-arm at :85-92.
There was a problem hiding this comment.
Re-verified at the post-merge head 93eef769a7 (this thread was last answered against 90e2d6f7ae). The finding is real, and the fix is still a policy decision rather than a guard.
Anchors at 93eef769a7: UNRESPONSIVE_ABORT_GRACE_MS = 5_000 (packages/core/src/agents/runtime/agent-progress-watchdog.ts:19); abort() at :67-78 arms the escalation with armEscalation itself as the re-arm callback; the only clear is the watchdog detach, which runs in the turn promise's .finally (packages/core/src/tools/agent/agent.ts:3881, :3916). cleanupWorktreeIsolation(), the registry settle and patchAgentMeta all run inside bgBody, i.e. inside that same promise — so the whole unwind is inside the 5 s window.
Damage confirmed: failUnresponsive sets status='failed' + notified + retainsPhysicalSlot (packages/core/src/agents/background-tasks.ts:935, :943); complete() and fail() bail on status/notified (:900ff) and cancel() requires running, so the settling turn's payload and its worktree suffix are dropped; Session.ts:9933 -> #recordUnresponsiveAgentNotification (:10142) requests the recycle with reason unresponsive_agent (:10171).
Why this is not a guard fix: the round-8 ask ("give the escalation a settle signal to stand down on") is a semantics spec, and the settle-signal variant does not cover the motivating case — the TOOL_RESULT/TOOL_PROGRESS(settled) of the very tool whose cancellation is the slow part arrive after the grace fires, while re-arming on STREAM_TEXT lets an abort-ignoring streamer live forever.
Question for the maintainer: pick the policy — (a) a bounded total grace that covers the cooperative unwind, keeping the hard force-fail, or (b) keep 5 s but let a late cooperative settle win (when retainsPhysicalSlot is set and the turn does settle, still publish the real payload incl. wtSuffix into entry.error/patchAgentMeta, and skip the recycle). Option (b) crosses agent.ts + background-tasks.ts + Session.ts and inverts terminal-outcome precedence. Leaving unresolved pending that decision.
There was a problem hiding this comment.
Round jmtz4tb6u1d independent re-verification at head 93eef769a7. Re-derived from source rather than inherited from the previous round. REAL, and human-gated. One anchor the earlier replies did not spell out:
The escalation has exactly one clear point, and it is coupled to the very thing it is racing.
agent-progress-watchdog.ts:19UNRESPONSIVE_ABORT_GRACE_MS = 5_000;abort()at:67-78arms it witharmEscalationas its own drift re-arm callback.- The only clear is
:239(if (escalationTimer) clearTimeout(escalationTimer)) inside the detach closure, and that closure runs atagent.ts:3915in the turn promise's.finally-- on the same line-pair asregistry.releaseRetainedPhysicalSlot(hookOpts.agentId)(agent.ts:3916). - Repo-wide grep, non-test:
releaseRetainedPhysicalSlothas exactly two call sites --agent.ts:3916andbackground-agent-resume.ts:1437. Both are inside that same.finally.
So the unwind the 5 s grace is measured against (cleanupWorktreeIsolation, scheduler cancellation of the already-hung tool, registry settle, patchAgentMeta, span recording) runs inside the only promise whose .finally can stop the escalation. There is no observation point outside it. That is also why the round-8 "give the escalation a settle signal to stand down on" cannot cover the motivating case: the settle signal of the slow tool arrives after the grace has fired, while re-arming on STREAM_TEXT lets an abort-ignoring streamer live forever.
Gate condition hit: architecture/design decision -- changing concurrency-primitive semantics. The decision underneath is who owns the terminal record once failUnresponsive has run (background-tasks.ts:935-956: status='failed' at :940, retainsPhysicalSlot at :943, meta patched, emitNotification(entry, true)), and whether a late cooperative settle may win over it. Maintainer options, unchanged: (a) a bounded total grace that actually covers the cooperative unwind, keeping the hard force-fail; or (b) keep 5 s and let a late cooperative settle re-publish the real payload (including the worktree suffix) and skip the recycle. (b) crosses agent.ts + background-tasks.ts + Session.ts and inverts terminal-outcome precedence.
No code change this round. Leaving unresolved rather than silently resolving a Critical I declined to fix.
There was a problem hiding this comment.
[Critical] R6-2: [certifies-falsely] [new-surface] Still stands — the 5-second escalation grace is measured against the entire cooperative unwind, so an agent that obeys the abort but takes longer than 5 s to settle is force-failed with a message asserting it made no progress, its terminal payload is discarded, and a runtime-generation recycle is spent on an agent that was cooperating.
abort() calls controller.abort(error) and immediately arms the escalation. Nothing but disposeWatchdog() clears that timer, and the disposer runs only from the run body's .finally. What the 5 s has to cover is the whole unwind: CoreToolScheduler awaits Promise.all(executing) with each invocation.execute(...) awaited directly and no abort race, and the run body then awaits worktree teardown before it reaches the payload guard — cleanupWorktreeIsolation spawns up to three git subprocesses. Once the escalation wins, failUnresponsive publishes failed and agent.ts's if (registry.get(agentId)?.retainsPhysicalSlot) break; discards the terminal payload the cooperating run had already produced.
The drift guard does not tighten this bound: if (performance.now() - expectedAt > 1_000) { rearm(); return; } can only push the escalation later, by another full 5 s per stall. The nested case is the unargued sibling — the parent's 5 s must cover the child's entire unwind. The design doc fixes the window deliberately ("a fixed five-second cooperative exit window") but offers no margin argument for what the window measures.
Confidence is qualified, and stated plainly so it is not overread: the mechanism is settled by run, but whether a real cooperative unwind exceeds 5 s was not measured — that needs a live model-backed background run, mid-task with a nested agent or worktree-isolated, at the moment a tool deadline expires.
Witness:
probe on the PR's own watchdog under a scaled clock:
ARM A (dispose inside the grace) {"abortedAfterMs":6003,"phase":"tool","onUnresponsiveCalls":0}
ARM B (still unwinding 200 ms after the abort, disposed later)
{"onUnresponsiveBeforeDispose":1,
"messages":["Background agent tool \"run_shell_command\" made no progress for 600000ms."]}
structural half: escalationTimer is written only at agent-progress-watchdog.ts:71 and cleared only inside the returned dispose closure
Measure the grace from the point the run stops making progress rather than from the abort, or have the escalation consult whether the turn is still unwinding (a flag the run body sets on cooperative exit) before force-failing. At minimum, do not discard a terminal payload the cooperating run already produced.
Please add the test that pins this: a watchdog test that aborts, keeps the turn unwinding past the grace, and asserts onUnresponsive does NOT fire while a cooperative-unwind flag is set — red if the flag is ignored.
中文说明
[Critical] R6-2：依然存在——5 秒升级宽限是针对整个协作式退出过程计时的，因此一个服从 abort、但需要超过 5 秒才能结算的 Agent 会被强制判为失败，并被告知"没有任何进展"，其终态载荷被丢弃，同时为这个本来在配合的 Agent 白白消耗一次 runtime generation 回收。
abort() 调用 controller.abort(error) 后立即启动升级定时器。除 disposeWatchdog() 之外没有任何东西会清除它，而该 disposer 只从 run body 的 .finally 调用。这 5 秒需要覆盖的是整个退出过程：CoreToolScheduler 会 await Promise.all(executing)，其中每个 invocation.execute(...) 都被直接 await 且没有 abort 竞争；随后 run body 在到达载荷判断之前还要 await worktree 拆除——cleanupWorktreeIsolation 最多会派生三个 git 子进程。一旦升级获胜，failUnresponsive 会发布 failed，而 agent.ts 的 if (registry.get(agentId)?.retainsPhysicalSlot) break; 会丢弃这个配合退出的 run 已经产出的终态载荷。
漂移保护并不会收紧这个上界：if (performance.now() - expectedAt > 1_000) { rearm(); return; } 只能把升级往后推，每次阻塞再推整整 5 秒。嵌套场景是未被论证的同类问题——父级的 5 秒必须覆盖子级的整个退出过程。设计文档有意固定了这个窗口（"a fixed five-second cooperative exit window"），但对窗口所度量的内容没有给出任何余量论证。
置信度有所限定，此处明确说明以免被过度解读：机制已由运行验证，但真实的协作式退出是否会超过 5 秒未被测量——这需要一个真实的、由模型驱动的后台 run，在其处于 task 嵌套或 worktree 隔离状态、且工具期限到期的那一刻进行观测。
修复方向：从 run 停止产生进展的时刻开始计时，而不是从 abort 开始；或让升级在强制判失败之前先判断该轮是否仍在退出过程中（由 run body 在协作退出时置位的标志）。至少不要丢弃配合退出的 run 已经产出的终态载荷。
请补充测试：一个 watchdog 用例，先 abort，再让该轮在宽限期之后仍处于退出过程，并断言在协作退出标志置位期间 onUnresponsive 不会触发；若该标志被忽略，测试必须为红。
（证据见上方 Witness 代码块；witness 为程序输出，未翻译。）
— qwen3.8-max via Qwen Code /review (v0.23.3)
| this.updateDisplay({}, updateOutput); | ||
| }; | ||
| eventEmitter.on(AgentEventType.STREAM_TEXT, forwardProgress); | ||
| eventEmitter.on(AgentEventType.MODEL_RETRY, forwardProgress); |
There was a problem hiding this comment.
[Critical] R6-3: [certifies-falsely] [new-surface] The new retry-backoff deadline extension cannot cross a nesting boundary — a nested run's MODEL_RETRY is downgraded to a flagless forwardProgress heartbeat, so the watched background agent's tool deadline stays at the fixed 10 minutes while the nested run sleeps out a provider-directed backoff.
Background agent B (watchdog on bgEmitter, agent.ts:3824) spawns nested agent C in the foreground, so setupEventListeners(updateOutput, sessionWorkflowAgent, runtimeEventEmitter) (agent.ts:3123) is C's only progress channel into B's scheduler. C's model call hits a 429 whose Retry-After is 3600 s. retryWithBackoff deliberately preserves it unclamped (actualDelayMs = retryAfterMs, packages/core/src/utils/retry.ts:496-500) and fires onRetry once before the sleep. C's own emitter gets one MODEL_RETRY — which agent-core.ts:1000 would turn into a 15 min + delay extension if C were the watched agent, but here it only reaches forwardProgress, which calls updateDisplay({}, updateOutput). B's outputUpdateHandler (agent-core.ts:1977-1992) then emits TOOL_PROGRESS for C's callId, and AgentToolProgressEvent (agent-events.ts:181-194) has no delay field, so B's watchdog takes the executing branch and re-arms exactly TOOL_PROGRESS_TIMEOUT_MS = 10 * 60_000. Nothing else emits during the sleep (persistent-mode heartbeatFn writes to stderr, not the emitter), so at 10 minutes into a legitimate rate-limit wait B is aborted with Background agent tool "agent" made no progress for 600000ms, C is killed through the child abort controller, and the registry settles B as failed — a false terminal record for a run that was honouring a provider-directed wait, on the exact retry path the onRetry plumbing was added to cover. Aggravating: forwardProgress throttles to 1/s, so a MODEL_RETRY arriving within 1 s of any other forwarded event is dropped entirely and the parent's clock keeps running from the previous heartbeat.
Witness (probe, pristine source, 6/6 pass):
P5 extends the model deadline by the reported retry delay -> PASS
(MODEL_RETRY retryDelayMs=3 600 000 => no abort at 15 m + 1 h - 1 s, abort 'model/control' at +1 s)
P6 a flagless nested heartbeat gives a long retry backoff only 10 minutes -> PASS
(one flagless TOOL_PROGRESS => abortPhase() === 'tool' at exactly TOOL_PROGRESS_TIMEOUT_MS)
P5/P6 together are the asymmetry: the same one-hour provider wait buys 75 minutes when the watched agent reports it and 10 minutes when a nested run reports it — and the flagless heartbeat also runs clearModel() (:198), so it removes any model deadline the parent had. Trigger realism: the repo's own suite pins a 10-minute provider wait as a case that must be honoured — retry.test.ts:1344 "should respect oversized Retry-After values for normal retries" asserts a setTimeout delay of 600_000 for retry-after: '600' — which is exactly TOOL_PROGRESS_TIMEOUT_MS. Any Retry-After above that in a nested run aborts the background agent. baseline: new-surface: the watchdog, MODEL_RETRY and the whole abort path are added by this commit.
Suggested fix: give the propagation channel a delay. Add retryDelayMs?: number to AgentToolProgressEvent and to the nested task_execution display, have C's MODEL_RETRY listener record it via updateDisplay({ retryDelayMs }, updateOutput) instead of the empty patch, forward it in outputUpdateHandler alongside waitingForExternalInput/awaitingApproval, and in onToolHeartbeat's executing branch arm the tool deadline as TOOL_PROGRESS_TIMEOUT_MS + Math.min(event.retryDelayMs ?? 0, MAX_RETRY_DEADLINE_EXTENSION_MS) (clearing it from the display on the next non-retry progress event so it cannot stick).
Any fix must respect this: MAX_RETRY_DEADLINE_EXTENSION_MS = 6 * 60 * 60_000 (agent-progress-watchdog.ts:20) already bounds the model-side extension and keeps the resulting setTimeout under Node's 2^31-1 overflow clamp, so any propagated tool-side extension must use the same clamp — retryWithBackoff reports an unclamped provider Retry-After (retry.ts:496-500), and 10 * 60_000 plus an unclamped value can exceed 2147483647 ms, which Node fires immediately instead of late.
Please add the test that pins this: in agent-progress-watchdog.test.ts, a case that emits TOOL_CALL + TOOL_PROGRESS{retryDelayMs: 20 * 60_000}, advances 11 minutes and asserts the signal is not aborted, then advances past the extended deadline and asserts phase === 'tool'; removing the propagation must make the 11-minute assertion fail. Plus an agent.test.ts case asserting a child MODEL_RETRY produces an updateOutput call whose display carries the delay (today it produces {}).
中文说明
[Critical] R6-3：新增的“重试退避延长期限”机制无法跨越嵌套边界 —— 嵌套运行的 MODEL_RETRY 被降级成一次不带标记的 forwardProgress 心跳，于是被监控的后台 Agent 的工具期限仍固定在 10 分钟，而嵌套运行正在按 provider 指定的退避时长休眠。
后台 Agent B（watchdog 挂在 bgEmitter 上，agent.ts:3824）以前台方式派生嵌套 Agent C，因此 setupEventListeners(updateOutput, sessionWorkflowAgent, runtimeEventEmitter)（agent.ts:3123）是 C 向 B 的调度器汇报进度的唯一通道。C 的模型请求遇到 429，其 Retry-After 为 3600 秒。retryWithBackoff 有意不做钳制地保留它（actualDelayMs = retryAfterMs，packages/core/src/utils/retry.ts:496-500），并在休眠之前触发一次 onRetry。C 自己的 emitter 收到一次 MODEL_RETRY —— 如果 C 是被监控方，agent-core.ts:1000 会把它变成 15 分钟 + delay 的延长；但在这里它只到达 forwardProgress，后者调用 updateDisplay({}, updateOutput)。B 的 outputUpdateHandler（agent-core.ts:1977-1992）随后为 C 的 callId 发出 TOOL_PROGRESS，而 AgentToolProgressEvent（agent-events.ts:181-194）没有 delay 字段，所以 B 的 watchdog 走 executing 分支，重新武装的正好是 TOOL_PROGRESS_TIMEOUT_MS = 10 * 60_000。休眠期间没有别的事件发出（persistent 模式的 heartbeatFn 写的是 stderr，不是 emitter），于是在一次正当限流等待进行到 10 分钟时，B 被以 Background agent tool "agent" made no progress for 600000ms 中止，C 通过子 abort controller 被杀掉，registry 把 B 结算为 failed —— 对一个正在遵守 provider 指定等待的运行写下了错误的终态记录，而这恰恰是 onRetry 这条管线被加进来要覆盖的重试路径。加重情节：forwardProgress 有 1 秒节流，因此若 MODEL_RETRY 在任何其他被转发事件的 1 秒内到达，它会被完全丢弃，父级的计时会继续从上一次心跳算起。
证据（探针，源码未修改，6/6 通过）见上方英文代码块：P5 与 P6 合起来正是这个不对称 —— 同样一小时的 provider 等待，由被监控 Agent 上报时可换来 75 分钟，由嵌套运行上报时只有 10 分钟；而且不带标记的心跳还会执行 clearModel()（:198），即移除父级原本已有的模型期限。触发现实性：仓库自己的测试就把 10 分钟的 provider 等待钉为必须尊重的场景 —— retry.test.ts:1344“should respect oversized Retry-After values for normal retries”对 retry-after: '600' 断言 setTimeout 延迟为 600_000 —— 而这正好等于 TOOL_PROGRESS_TIMEOUT_MS。嵌套运行中任何高于该值的 Retry-After 都会中止后台 Agent。baseline: new-surface：watchdog、MODEL_RETRY 与整条中止路径都由本次提交新增。
修复建议：给这条传播通道带上 delay。为 AgentToolProgressEvent 和嵌套的 task_execution display 增加 retryDelayMs?: number；让 C 的 MODEL_RETRY 监听器通过 updateDisplay({ retryDelayMs }, updateOutput) 记录它，而不是发一个空 patch；在 outputUpdateHandler 中与 waitingForExternalInput/awaitingApproval 一起转发；并在 onToolHeartbeat 的 executing 分支中把工具期限武装为 TOOL_PROGRESS_TIMEOUT_MS + Math.min(event.retryDelayMs ?? 0, MAX_RETRY_DEADLINE_EXTENSION_MS)（并在下一个非重试进度事件时从 display 中清除，避免它粘住）。
修复必须遵守：MAX_RETRY_DEADLINE_EXTENSION_MS = 6 * 60 * 60_000（agent-progress-watchdog.ts:20）已经在约束模型侧的延长，并使最终的 setTimeout 保持在 Node 的 2^31-1 溢出钳位之下，因此任何传播到工具侧的延长都必须使用同一个钳位 —— retryWithBackoff 上报的是未钳制的 provider Retry-After（retry.ts:496-500），而 10 * 60_000 加上一个未钳制的值可能超过 2147483647 毫秒，此时 Node 会立即触发而不是延后触发。
请补上能钉住这一点的测试：在 agent-progress-watchdog.test.ts 中增加一个用例，发出 TOOL_CALL + TOOL_PROGRESS{retryDelayMs: 20 * 60_000}，推进 11 分钟并断言信号未被中止，再推进超过延长后的期限并断言 phase === 'tool'；移除该传播机制时，11 分钟处的断言必须失败。另在 agent.test.ts 中增加一个用例，断言子级 MODEL_RETRY 会产生一次 display 携带该 delay 的 updateOutput 调用（目前它产生的是 {}）。
— qwen3.8-max via Qwen Code /review (v0.23.2)
There was a problem hiding this comment.
Not fixing — escalating. This is one of three findings (R6-1, R6-2, R6-3) that all change watchdog/abort semantics, which is the human-gated class on a PR whose entire subject is timeout semantics. Verified at head:
agent.ts:1483—eventEmitter.on(AgentEventType.MODEL_RETRY, forwardProgress), so a nested run's retry event reaches the parent through the same handler asSTREAM_TEXTandTOOL_PROGRESS.forwardProgressthrottles to 1/s (agent.ts:1478) and callsthis.updateDisplay({}, updateOutput)(:1480) with an empty object, so any delay the event carried is dropped.AgentToolProgressEvent(agent-events.ts:181) carriessettled?(:186),waitingForExternalInput?(:189) andawaitingApproval?(:192) — and no delay or retry field. So the parent watchdog has nothing to extend from and re-armsTOOL_PROGRESS_TIMEOUT_MS = 10 * 60_000(agent-progress-watchdog.ts:18).
The asymmetry the report describes is therefore structural, not speculative: the same provider-directed wait buys 15 min + delay (capped at MAX_RETRY_DEADLINE_EXTENSION_MS, :20) when the watched agent reports it via agent-core.ts:1000, and a flat 10 minutes when a nested run reports it.
The gate condition: crossing the nesting boundary needs a new field on a shared event type — exactly how awaitingApproval? was added for the approval case — which is a change to the agent event contract, plus a change to what forwardProgress forwards past its 1/s throttle. The decision underneath it is whether a nested run's provider-directed backoff should extend the parent's tool deadline at all, and if so whether the throttle may swallow it. Both are timeout-semantics and data-structure decisions on the mechanism this PR introduces.
Leaving unresolved.
There was a problem hiding this comment.
[Critical] R6-3: [certifies-falsely] [new-surface] The tool deadline has no provider-backoff extension, so a nested subagent honouring a long Retry-After gets the whole background turn aborted at 10 minutes — the exact false kill that armModel(retryDelayMs) was added to prevent one level up.
Failure scenario: A background agent spawns a foreground subagent, so the nested run is one tool call under the parent's watchdog. The provider answers the nested model call with 429 and Retry-After: 3600. retry.ts:433 sets delayMs = Math.min(3600000, capMs) = 3600000 (and the non-persistent path at retry.ts:496, actualDelayMs = retryAfterMs, is uncapped); onRetry fires exactly once before the sleep (retry.ts:462). That produces one nested MODEL_RETRY, which forwardProgress (agent.ts:1494) turns into one parent TOOL_PROGRESS heartbeat — resetting the parent's tool deadline to a flat 10 minutes and discarding the delay, because armTool takes no extension argument. Nothing else is emitted during the hour-long sleep, so at 10 minutes abort(new AgentProgressTimeoutError('tool', 600000, 'Agent')) fires on the turn controller and 5 s later registry.failUnresponsive(agentId, ...). The subagent is killed mid-legitimate-backoff and the background agent is marked failed, while the identical wait at the top level survives because armModel grants 15min + min(3600000, MAX_RETRY_DEADLINE_EXTENSION_MS). The author aligned that cap to PERSISTENT_CAP_MS (both 6 h), so a wait of up to 6 hours is explicitly considered legitimate — the tool path just never learns about it.
Witness:
sweep of the real source — `retryDelayMs` occurs in `packages/core/src/agents/runtime/` at exactly 4 watchdog/producer sites on the **model** path (`agent-progress-watchdog.ts:99, 111, 122, 158`) and **0** on the tool path (`armTool` `:125-141`, `onToolHeartbeat` `:168-206`); `AgentToolProgressEvent` declares no such field. Supporting run (non-discriminating — it does not exercise the nested case): `npx vitest run src/agents/runtime/agent-progress-watchdog.test.ts` → `Tests 7 passed (7)`, i.e. the suite simultaneously proves the model side survives a 1 h retry delay (`keeps a provider-backoff
Suggested fix: Give the tool deadline the same extension the model deadline has. Carry the nested retry delay out on the task_execution display (set it in the MODEL_RETRY handler next to forwardProgress, clear it on ROUND_START/STREAM_TEXT), surface it as an optional retryDelayMs on AgentToolProgressEvent in agent-core.ts's outputUpdateHandler, and in armTool schedule TOOL_PROGRESS_TIMEOUT_MS + Math.min(event.retryDelayMs ?? 0, MAX_RETRY_DEADLINE_EXTENSION_MS), re-arming through a closure that preserves the granted delay exactly as armModel does.
The fix must not violate this existing fact: The tool-side extension must stay within the same bound the model side uses — const MAX_RETRY_DEADLINE_EXTENSION_MS = 6 * 60 * 60_000; (packages/core/src/agents/runtime/agent-progress-watchdog.ts:19) — which is deliberately equal to const PERSISTENT_CAP_MS = 6 * 60 * 60 * 1000; // 6 hours — absolute single wait cap (packages/core/src/utils/retry.ts:25). A smaller cap re-int
Acceptance criterion: packages/core/src/agents/runtime/agent-progress-watchdog.test.ts — a case mirroring the existing 'keeps a provider-backoff deadline extension across a clock-drift re-arm': toolCall('t1'); toolProgress('t1'); toolProgress('t1', { retryDelayMs: 30 * 60_000 }), then assert abortPhase() is still undefined at TOOL_TIMEOUT_MS + 1 and becomes 'tool' at `TOOL_TIMEOUT_MS + Please confirm the mutation that proves it (remove the guard, run that test, confirm it goes red).
中文说明
工具截止时间没有 provider 退避延期，而模型截止时间有。后台 Agent 派生的前台子 Agent 收到 429 Retry-After: 3600 时，只会向父级发出一次无标记的 TOOL_PROGRESS 心跳，把父级工具截止时间重置为固定 10 分钟并丢弃该延迟；随后一小时内再无事件，10 分钟时整个后台轮次被中止。作者已把模型侧上限对齐到 PERSISTENT_CAP_MS（同为 6 小时），说明长达 6 小时的等待被视为合法——工具侧只是从未得知这一点。
— qwen3.8-max via Qwen Code /review (v0.23.3)
There was a problem hiding this comment.
Real bug: line 1494 forwards MODEL_RETRY via forwardProgress, which drops retryDelayMs, so the top-level watchdog never sees a nested run backoff. Two blockers: (1) there is no transport from the nested AgentCore retry delay to the top-level watchdog — the existing outputChunk channel only carries waitingForExternalInput/awaitingApproval; (2) armModel(retryDelayMs) extends the model deadline, which is suppressed while the nested agent tool is executing, so the active deadline is the 10-minute tool deadline, which armModel does not touch. Extending the tool deadline on a nested retry is a cross-layer design change. Left unresolved.
There was a problem hiding this comment.
[Critical] R6-3: [certifies-falsely] [new-surface] Still stands — the retry-backoff deadline extension cannot cross a nesting boundary: a nested run's MODEL_RETRY is downgraded to a flagless forwardProgress heartbeat, so the watched background agent's tool deadline stays at the fixed 10 minutes while the nested run sleeps out a provider-directed backoff
A background agent spawns a foreground subagent, so the nested run is one tool call with a 10-minute tool deadline. The nested model call gets a 429/503 whose Retry-After is honoured uncapped by retryWithBackoff; onRetry fires MODEL_RETRY{retryDelayMs}, but agent.ts forwards it through forwardProgress → updateDisplay({}) which carries no delay, and AgentToolProgressEvent has no retryDelayMs field, so armTool() can only re-arm the fixed TOOL_PROGRESS_TIMEOUT_MS. Ten minutes into a legitimate 30-minute provider wait the parent watchdog aborts the whole background turn and reports it failed — the exact false positive MAX_RETRY_DEADLINE_EXTENSION_MS exists to prevent one level up.
Witness:
not run — scratch-tree refused (available: false); settled by quoted lines at HEAD 90e2d6f7ae: armTool(callId) takes no delay parameter and schedules the bare TOOL_PROGRESS_TIMEOUT_MS (agent-progress-watchdog.ts:122-141), onToolHeartbeat's plain branch is the only caller (agent-progress-watchdog.ts:201-203), and agent.ts:1494-1503 forwards MODEL_RETRY through the 1/s-throttled forwardProgress, which calls updateDisplay({}, updateOutput) with no delay field. Re-derived independently this round by two reverse-audit agents.
Suggested fix:
Carry the delay across the boundary: add retryDelayMs?: number to AgentToolProgressEvent, latch the last MODEL_RETRY delay on the child display, forward it in agent-core's outputUpdateHandler, and give armTool the same Math.min(retryDelayMs, MAX_RETRY_DEADLINE_EXTENSION_MS) extension armModel applies.
The fix must not violate this existing fact: MAX_RETRY_DEADLINE_EXTENSION_MS = 6 * 60 * 60_000 (agent-progress-watchdog.ts:20) is deliberately equal to PERSISTENT_CAP_MS = 6 * 60 * 60 * 1000 (utils/retry.ts:25); the tool-side extension must clamp to the same bound and must be re-passed through the drift re-arm closure exactly as armModel does, or a clock-drift re-arm collapses the deadline back to the base timeout.
Please confirm the mutation that proves it — packages/core/src/agents/runtime/agent-progress-watchdog.test.ts — mirroring the existing MODEL_RETRY-extension case: toolCall, toolProgress carrying retryDelayMs, assert no abort at TOOL_PROGRESS_TIMEOUT_MS + 1 and an abort at TOOL_PROGRESS_TIMEOUT_MS + retryDelayMs. Red if the tool-side extension is removed.
中文说明
重试退避的截止时间延长无法跨越嵌套边界。子运行的 MODEL_RETRY 经 forwardProgress 降级为一次不带标志位的显示心跳（agent.ts:1494-1503，1 秒节流后调用 updateDisplay({}, updateOutput)），而 AgentToolProgressEvent 没有 retryDelayMs 字段，于是 armTool() 只能重新装配固定的 TOOL_PROGRESS_TIMEOUT_MS。
后果：后台 Agent 派生一个前台子 Agent（嵌套运行因此只是一个 10 分钟工具截止时间的工具调用）。当子运行的模型调用收到 429/503 且 Retry-After 被 retryWithBackoff 不设上限地遵守时，父看门狗会在这次合法的 30 分钟等待进行到第 10 分钟时中止整个后台轮次并把它报为 failed——正是 MAX_RETRY_DEADLINE_EXTENSION_MS 在上一层要防止的误杀。
修复方向：把延迟带过边界——为 AgentToolProgressEvent 增加 retryDelayMs?: number，在子显示上锁存最近一次 MODEL_RETRY 延迟，经 agent-core 的 outputUpdateHandler 转发，并让 armTool 采用与 armModel 相同的 Math.min(retryDelayMs, MAX_RETRY_DEADLINE_EXTENSION_MS) 延长。
— qwen3.8-max via Qwen Code /review (v0.23.3)
There was a problem hiding this comment.
Design-level — left open deliberately. The only fix is a new retryDelayMs field on the cross-package AgentToolProgressEvent transport so a nested run's provider-directed backoff can extend the watched agent's tool deadline across the nesting boundary. Adding new cross-package transport to close one thread is out of scope for this round; it needs its own change with the transport contract reviewed on its own merits. Evidence anchor: packages/core/src/tools/agent/agent.ts:1502.
There was a problem hiding this comment.
Re-verified at the post-merge head 93eef769a7 (last answered against 90e2d6f7ae). The nesting boundary holds at this head:
packages/core/src/tools/agent/agent.ts:1484—eventEmitter.on(AgentEventType.MODEL_RETRY, forwardProgress), andforwardProgress(:1477-1481) ignores the event payload and 1/s-throttlesupdateDisplay({}, ...).packages/core/src/agents/runtime/agent-events.ts:181-194—AgentToolProgressEventcarries onlysettled/waitingForExternalInput/awaitingApproval.packages/core/src/agents/runtime/agent-core.ts:2094-2109—outputUpdateHandlerforwards only those two flags intoTOOL_PROGRESS.agent-progress-watchdog.ts:128—armTool(callId)arms the bareTOOL_PROGRESS_TIMEOUT_MS(10 min); onlyarmModel(:114) accepts a delay.
So the fix is a new field on the runtime event transport, plus a latch/clear rule (a nested MODEL_RETRY delay must be cleared by the next non-retry progress event or it sticks), plus reuse of the MAX_RETRY_DEADLINE_EXTENSION_MS clamp so the resulting setTimeout stays under the 2^31-1 overflow clamp: agent-events.ts + agent.ts + agent-core.ts + agent-progress-watchdog.ts, two layers, one new cross-layer contract.
That is a transport-contract change, not the guard correction this round is scoped to. Leaving unresolved; it needs its own review on the transport contract's merits.
There was a problem hiding this comment.
Round jmtz4tb6u1d independent re-verification at head 93eef769a7. REAL, and human-gated. One correction that makes the loss stronger than previously stated on this thread:
forwardProgress is declared zero-arity -- const forwardProgress = () => { (agent.ts:1477) -- and is registered for three event types (:1483-1485, MODEL_RETRY among them). The nested run's retryDelayMs is therefore dropped by signature, before the 1/s throttle at :1479 is ever reached; the throttle is a second, independent loss stacked on top of it. No payload-preserving variant of the existing handler can carry the delay, so this is not a "pass the event through" fix.
The rest confirms at this head:
agent-events.ts:181-194--AgentToolProgressEventcarries onlysettled(:186),waitingForExternalInput(:189) andawaitingApproval(:192). No delay field.agent-progress-watchdog.ts:128--armTool(callId)takes no delay and schedules the bareTOOL_PROGRESS_TIMEOUT_MS; onlyarmModel(:99) accepts one.agent-progress-watchdog.ts:142--armTool's drift re-arm re-passes the barearmTool(callId), so any tool-side extension must be threaded through a closure exactly asarmModeldoes at:125, or a clock-drift re-arm collapses the deadline back to the base timeout.
Gate condition hit: the fix adds a field to the runtime event transport (the same way awaitingApproval? was added for the approval case), plus a latch/clear rule so a nested retry delay cannot stick past the next non-retry progress event, plus reuse of the MAX_RETRY_DEADLINE_EXTENSION_MS clamp -- agent-events.ts + agent.ts + agent-core.ts + agent-progress-watchdog.ts, two layers, one new cross-layer contract. That is outside this round's scope ceiling (no new cross-package field/transport to close a single thread) and needs review on the transport contract's own merits.
No code change this round. Leaving unresolved.
There was a problem hiding this comment.
[Critical] R6-3: [certifies-falsely] [new-surface] Still stands, and this round traced a second consequence of the same root — because forwardProgress republishes the merged display and contributes nothing from the event it is bound to, neither the retry-backoff extension nor the nested park flags cross a nesting boundary. The watched ancestor keeps a flat 10-minute tool deadline over a nested run that is legitimately sleeping out a provider-directed backoff or waiting on a human.
Consequence 1, retry: background agent A launches nested B; B's provider returns 429 with a Retry-After above 10 minutes. retry.ts fires onRetry once before sleeping in sleepWithHeartbeat, whose heartbeatFn only writes to stderr and emits no agent event — so B's own watchdog calls armModel(retryDelayMs) and extends, but the parent sees a flagless TOOL_PROGRESS and arms a flat TOOL_PROGRESS_TIMEOUT_MS, then aborts A's turn at 10 minutes. The coverage boundary is asymmetric: a single server-directed wait is capped only at PERSISTENT_CAP_MS (6 h) on the persistent branch and is uncapped on the normal branch, so the direct case is covered up to MAX_RETRY_DEADLINE_EXTENSION_MS while everything above 600 s across a nesting boundary is uncovered.
Consequence 2, park: the parent watchdog's only nested-approval suspension path is onToolHeartbeat's if (event.awaitingApproval) branch — onApproval cannot fire, because a nested approval is never emitted on the watched agent's own emitter for a callId in its tools map. Since the forwarded event contributes nothing, a park two or more levels below a watched background agent never suspends anything and armTool keeps a 10-minute deadline running over a human being asked. DEFAULT_MAX_SUBAGENT_DEPTH = 5 makes that depth reachable with default configuration. This contradicts the design doc's "The relevant tool deadline is suspended while user approval is pending" and the watchdog's own comment that approval waits must not cause false watchdog failures.
R3-12 already reports the code fact (forwardProgress merges zero fields) as a Suggestion. It is filed here separately because the consequence changes its weight: those dropped fields are the ancestor watchdog's only signal for a nested park or a nested backoff, so the same line produces a false kill of a healthy run rather than a stale display.
Confidence is qualified: both mechanisms are settled by run, but the retry trigger needs a provider-directed Retry-After above 600 s on a nested run inside a watched background agent, which was not produced.
Witness:
probe in the PR's own createInvocationWithEventDrivenAgent harness:
intact -> {"statuses":["c1:executing"]} (no park flag on any heartbeat)
one-line candidate fix merging the event's flags into forwardProgress
-> {"awaitingApproval":true,"statuses":["c1:executing"]} and {"waitingForExternalInput":true,...} <- the flip
retry arm (parent watching a nested run in backoff, MODEL_RETRY carrying retryDelayMs)
-> {"aborted":true,"phase":"tool","unresponsive":["Background agent tool \"task\" made no progress for 600000ms."]}
structural: AgentToolProgressEvent has no delay field (agent-events.ts:181-195), so armTool can only ever schedule 10 * 60_000
Give forwardProgress the event it is bound to and merge the fields the ancestor watchdog needs — awaitingApproval, waitingForExternalInput, and a retry-delay field on AgentToolProgressEvent that armTool can extend by, mirroring armModel(retryDelayMs) under the same bounded-extension policy. Propagate recursively so a park or a backoff at any depth reaches the watched ancestor.
Any fix must respect utils/retry.ts:501 — actualDelayMs = retryAfterMs, under the comment that normal HTTP retries intentionally preserve provider-directed Retry-After waits instead of clamping to the exponential maxDelayMs: a new bound must keep treating that wait as legitimate backoff, and must respect retry.ts:25 PERSISTENT_CAP_MS = 6 * 60 * 60 * 1000 plus the existing Math.min(..., 2_147_483_647) clamp so a 6-hour Retry-After cannot become a setTimeout overflow.
Please add the tests that pin this: one driving a nested run two levels below a watched background agent into an approval wait and asserting the ancestor's tool deadline is suspended, and one asserting a nested MODEL_RETRY carrying retryDelayMs extends the ancestor's tool deadline. Both red while forwardProgress merges zero fields.
中文说明
[Critical] R6-3：依然存在，并且本轮追溯到同一根因的第二个后果——由于 forwardProgress 只是重新发布合并后的 display，完全不贡献它所绑定的事件内容，重试退避的期限延长与嵌套的 park 标志都无法跨越嵌套边界。被监视的祖先 Agent 会对一个正在合理等待 provider 退避、或正在等待人工批准的嵌套 run，维持固定的 10 分钟工具期限。
后果一（重试）：后台 Agent A 启动嵌套的 B；B 的 provider 返回带 Retry-After（超过 10 分钟）的 429。retry.ts 会在 sleepWithHeartbeat 睡眠之前触发一次 onRetry，而其 heartbeatFn 只向 stderr 写入、不发出任何 agent 事件——因此 B 自己的 watchdog 会调用 armModel(retryDelayMs) 并延长期限，但父级只看到一个无标志的 TOOL_PROGRESS，于是启动固定的 TOOL_PROGRESS_TIMEOUT_MS，并在 10 分钟时中止 A 的这一轮。覆盖边界是不对称的：单次服务端指定的等待在 persistent 分支上只受 PERSISTENT_CAP_MS（6 小时）限制，在 normal 分支上则完全不设上限，因此直连场景可覆盖到 MAX_RETRY_DEADLINE_EXTENSION_MS，而跨嵌套边界超过 600 秒的部分完全无覆盖。
后果二（park）：父级 watchdog 唯一的嵌套批准挂起路径是 onToolHeartbeat 的 if (event.awaitingApproval) 分支——onApproval 不可能触发，因为嵌套的批准不会以属于其 tools 映射的 callId 在被监视 Agent 自己的 emitter 上发出。由于转发的事件不贡献任何内容，位于被监视后台 Agent 下方两层或更深的 park 永远不会挂起任何期限，armTool 会在有人正在被征询时继续跑满 10 分钟期限。DEFAULT_MAX_SUBAGENT_DEPTH = 5 使该深度在默认配置下即可达到。这与设计文档的 "The relevant tool deadline is suspended while user approval is pending" 以及 watchdog 自身"批准等待不得导致误报"的注释相矛盾。
R3-12 已经把这一代码事实（forwardProgress 合并零字段）作为 Suggestion 报告过。此处单独立项是因为后果改变了它的分量：这些被丢弃的字段是祖先 watchdog 关于嵌套 park 或嵌套退避的唯一信号，因此同一行代码造成的是对健康 run 的误杀，而不只是 display 陈旧。
置信度有所限定：两个机制均已由运行验证，但重试触发需要在被监视后台 Agent 内部的嵌套 run 上出现超过 600 秒的服务端 Retry-After，该条件未被实际构造。
修复方向：把 forwardProgress 所绑定的事件交给它，并合并祖先 watchdog 需要的字段——awaitingApproval、waitingForExternalInput，以及在 AgentToolProgressEvent 上增加一个重试延迟字段供 armTool 据此延长，与 armModel(retryDelayMs) 采用相同的有界延长策略。并递归传播，使任意深度的 park 或退避都能抵达被监视的祖先。
修复约束：必须遵守 utils/retry.ts:501 的 actualDelayMs = retryAfterMs，其注释说明普通 HTTP 重试有意保留 provider 指定的 Retry-After 而不收敛到指数退避的 maxDelayMs：新的上界必须继续把该等待视为合法退避，并遵守 retry.ts:25 的 PERSISTENT_CAP_MS = 6 * 60 * 60 * 1000 以及现有的 Math.min(..., 2_147_483_647) 收敛，使 6 小时的 Retry-After 不会造成 setTimeout 溢出。
请补充测试：一个驱动位于被监视后台 Agent 下方两层的嵌套 run 进入批准等待，并断言祖先的工具期限被挂起；另一个断言携带 retryDelayMs 的嵌套 MODEL_RETRY 会延长祖先的工具期限。当 forwardProgress 仍合并零字段时，两者都必须为红。
（证据见上方 Witness 代码块；witness 为程序输出，未翻译。）
— qwen3.8-max via Qwen Code /review (v0.23.3)
| /** Reports retry backoff so background-agent liveness can extend its deadline. */ | ||
| onRetry?: (delayMs: number) => void; |
There was a problem hiding this comment.
[Critical] R6-6: [certifies-falsely] [regression] The new retry-delay reporting stops at makeApiCallAndProcessStream, so the context-compression side query that runs inside the same awaited sendMessageStream retries invisibly — and the 15-minute model deadline kills a background agent that is legitimately waiting for provider capacity in the very mode documented for background automation.
With QWEN_CODE_UNATTENDED_RETRY=1 (retry.ts:154-157) — the mode docs/users/configuration/settings.md:815 describes as "Designed for CI/CD pipelines and background automation where long-running tasks should survive temporary API outages" — a long-running background agent's round crosses the compaction threshold, so sendMessageStream awaits tryCompress (llm-chat.ts:2864, or the reactive call at :3664) → ChatCompressionService → config.getBaseLlmClient().generateText(...) → retryWithBackoff with persistentMode: isUnattendedMode() (baseLlmClient.ts:300) whose onRetry is wired only to logApiRetry (baseLlmClient.ts:307-320, :462). GenerateTextOptions exposes no onRetry field, so no caller could surface the delay even if it wanted to. The compression call's maxAttempts: 1 does not bound that wait: shouldPersist = persistent && isTransient && callerAllowsRetry && !isFailFast (retry.ts:412-413) ignores maxAttempts, and the persistent branch clamps attempt so "the while-loop never exits" (retry.ts:485-487), sleeping in heartbeat chunks on a 429/529. Meanwhile the watchdog's model timer sees nothing at all — the compression phase emits no AgentEvent, and StreamEventType.COMPRESSED is only logged (agent-core.ts:1084-1092, continue) and is not in the listener set. At 15 minutes abort(new AgentProgressTimeoutError('model/control', 900000)) fires, registry.failUnresponsive settles the agent permanently as failed with "made no model/control progress for 900000ms", the user gets a failure notification for an agent doing exactly what unattended mode promises, and this diff's recycle path condemns the runtime generation. The identical wait on the round's own model call would have been extended by up to 6 h. This falsifies the diff's own contract in docs/design/background-agent-progress-watchdog.md: "Retry delays surfaced by qwen-code extend the model deadline by at most six hours" — a compression retry is a qwen-code retry, not provider-internal; it is simply never surfaced.
Tighter than it looks: sendMessageStream returns its generator only at llm-chat.ts:3111, so the proactive tryCompress at :2864 runs in the eager prologue — during await chat.sendMessageStream(...), i.e. before agent-core emits ROUND_START (:1009). The budget charged to the compression wait is therefore the timer armed by the previous round's last event, or the attach-time arm on round 1 — not a freshly re-armed 15 minutes.
Witness — not run: the nearest capability was mock-provider + drive (429 the compression side query with a long Retry-After under QWEN_CODE_UNATTENDED_RETRY=1 and observe the abort). It cannot reach the outcome here: MODEL_CONTROL_PROGRESS_TIMEOUT_MS = 15 * 60_000 is a hardcoded module constant with no env or config knob (the design doc states the change "adds no public timeout configuration"), so a faithful drive needs 15 minutes of real wall time, and faking the clock would require an integration harness spanning AgentCore + LlmChat + ChatCompressionService that does not exist in the repo (no test attaches the watchdog to a real run). The chain was verified link by link at HEAD instead, and the reporting sweep below is run-produced:
grep "onRetry:" packages/core/src (non-test) -> 5 sites
agent-core.ts:1000 <- the only producer that reaches the watchdog
llm-chat.ts:4654 <- the new forwarder added by this diff
baseLlmClient.ts:307 <- logApiRetry only
baseLlmClient.ts:462 <- logApiRetry only
client.ts:5055 <- logApiRetry only
baseline: regression — agent-progress-watchdog.ts is a new file in this diff and is the only source of the 15-minute abort; at the merge base nothing aborted a background turn for lack of model progress, so an unattended compression capacity-wait survived (bounded only by the caller's own wall-clock limit). Precondition, stated plainly: the long wait needs QWEN_CODE_UNATTENDED_RETRY=1 and a 429/529 on the compression side query whose cumulative wait exceeds the remaining model budget (one Retry-After > 15 min suffices; otherwise ~3 backoff retries at the 5-min cap). Without that env var, maxAttempts: 1 fails the compression immediately to NOOP and there is no long wait.
Suggested fix: thread the send option down the compression path rather than widening its retry budget — add onRetry?: (delayMs: number) => void to TryCompressOptions (llm-chat.ts:481) and pass options?.onRetry at both tryCompress call sites (:2864, :3664); carry it in ChatCompressionService's opts next to the existing signal (chatCompressionService.ts:284) and forward it into both generateText calls, having BaseLlmClient invoke a caller-supplied onRetry alongside logApiRetry (baseLlmClient.ts:307, :462). Then state in both design docs which retry loops report.
Any fix must respect this: chatCompressionService.ts:749-751 — "Best-effort: failures fall back to NOOP and the next turn re-triggers compression anyway, so don't burn 7 retries blocking the user mid-turn." with maxAttempts: 1; a fix must report the delay, not raise compression's retry budget.
Please add the test that pins this: in packages/core/src/core/llm-chat.test.ts, a case that forces compression inside sendMessageStream with a stubbed Config.getBaseLlmClient().generateText whose retryWithBackoff reports one retry, and asserts the onRetry supplied in LlmChatSendOptions was invoked with that delay. Deleting the new forwarding must turn it red; today nothing in the repo observes compression retry reporting.
中文说明
[Critical] R6-6：新增的重试延迟上报止步于 makeApiCallAndProcessStream，因此在同一个 await 的 sendMessageStream 内部运行的上下文压缩旁路查询，其重试是不可见的 —— 而 15 分钟模型期限会杀掉一个正当等待 provider 容量的后台 Agent，且发生在那个专为后台自动化而记录的运行模式下。
在 QWEN_CODE_UNATTENDED_RETRY=1（retry.ts:154-157）下 —— docs/users/configuration/settings.md:815 把该模式描述为“为 CI/CD 流水线与后台自动化设计，让长时间运行的任务能够挺过临时性 API 中断” —— 一个长时间运行的后台 Agent 的某轮越过压缩阈值，于是 sendMessageStream 会 await tryCompress（llm-chat.ts:2864，或 :3664 的反应式调用）→ ChatCompressionService → config.getBaseLlmClient().generateText(...) → retryWithBackoff，其 persistentMode: isUnattendedMode()（baseLlmClient.ts:300），而它的 onRetry 只接到了 logApiRetry（baseLlmClient.ts:307-320、:462）。GenerateTextOptions 不暴露任何 onRetry 字段，因此即便调用方想上报也无从下手。压缩调用的 maxAttempts: 1 并不能约束这段等待：shouldPersist = persistent && isTransient && callerAllowsRetry && !isFailFast（retry.ts:412-413）完全忽略 maxAttempts，且 persistent 分支会钳制 attempt 以致“while 循环永不退出”（retry.ts:485-487），在 429/529 上以心跳分片方式持续休眠。与此同时 watchdog 的模型定时器什么都收不到 —— 压缩阶段不发出任何 AgentEvent，而 StreamEventType.COMPRESSED 只被记日志（agent-core.ts:1084-1092，随后 continue），并不在监听集合内。到 15 分钟时 abort(new AgentProgressTimeoutError('model/control', 900000)) 触发，registry.failUnresponsive 把该 Agent 永久结算为 failed，理由是“made no model/control progress for 900000ms”，用户收到一个“Agent 失败”的通知，而这个 Agent 做的恰恰是 unattended 模式所承诺的事；本 diff 的回收路径还会判该 runtime generation 死刑。同样这段等待如果发生在本轮自己的模型调用上，是会被延长最多 6 小时的。这与 diff 自己在 docs/design/background-agent-progress-watchdog.md 写下的契约相矛盾：“由 qwen-code 上报的重试延迟最多把模型期限延长六小时” —— 压缩重试是 qwen-code 的重试，不是 provider 内部的；它只是从未被上报。
比看起来更紧：sendMessageStream 直到 llm-chat.ts:3111 才返回其 generator，因此 :2864 处主动触发的 tryCompress 运行在** eagerly 前置段**中 —— 也就是在 await chat.sendMessageStream(...) 期间、在 agent-core 发出 ROUND_START（:1009）之前。所以计入压缩等待的预算，是上一轮最后一个事件武装的定时器，或者第 1 轮时挂载即武装的那个 —— 而不是一个刚刚重新武装的 15 分钟。
证据 —— not run：最接近的手段是 mock-provider + drive（在 QWEN_CODE_UNATTENDED_RETRY=1 下用一个较长的 Retry-After 让压缩旁路查询返回 429，然后观察中止）。但它无法达到此处要证明的结果：MODEL_CONTROL_PROGRESS_TIMEOUT_MS = 15 * 60_000 是硬编码的模块常量，没有任何 env 或配置开关（设计文档明说本次变更“不新增公开的 timeout 配置”），因此一次忠实的驱动需要 15 分钟真实墙钟时间；而伪造时钟需要一个横跨 AgentCore + LlmChat + ChatCompressionService 的集成测试装置，仓库中并不存在（没有任何测试把 watchdog 挂到真实运行上）。因此改为在 HEAD 上逐环节核实链路，而下面这份上报点清点是由运行产出的：grep "onRetry:" packages/core/src（非测试）→ 5 处，其中只有 agent-core.ts:1000 能到达 watchdog，llm-chat.ts:4654 是本 diff 新增的转发点，其余三处（baseLlmClient.ts:307、:462、client.ts:5055）只调用 logApiRetry。
baseline: regression —— agent-progress-watchdog.ts 在本 diff 中是新文件，也是这个 15 分钟中止的唯一来源；在合并基线上，没有任何逻辑会因为“模型无进展”而中止一个后台 turn，因此一次 unattended 的压缩容量等待是可以挺过去的（只受调用方自己的墙钟上限约束）。前提条件明确写出：这段长等待需要 QWEN_CODE_UNATTENDED_RETRY=1，并且压缩旁路查询上出现 429/529 且其累计等待超过剩余的模型预算（一个大于 15 分钟的 Retry-After 就够了；否则约为 5 分钟上限下的 3 次退避重试）。若没有该环境变量，maxAttempts: 1 会让压缩立刻失败并回退为 NOOP，也就没有长等待。
修复建议：把这个 send option 沿压缩路径传下去，而不是放宽它的重试预算 —— 为 TryCompressOptions（llm-chat.ts:481）增加 onRetry?: (delayMs: number) => void，并在两个 tryCompress 调用点（:2864、:3664）传入 options?.onRetry；把它与既有的 signal 一起放进 ChatCompressionService 的 opts（chatCompressionService.ts:284）并转发进两个 generateText 调用，让 BaseLlmClient 在 logApiRetry 之外也调用调用方提供的 onRetry（baseLlmClient.ts:307、:462）。随后在两份设计文档中写明哪些重试循环会上报。
修复必须遵守：chatCompressionService.ts:749-751 —— “尽力而为：失败会回退为 NOOP，下一轮反正会重新触发压缩，所以不要为了它烧掉 7 次重试、把用户卡在 turn 中间。”并配有 maxAttempts: 1；修复必须上报该延迟，而不是提高压缩的重试预算。
请补上能钉住这一点的测试：在 packages/core/src/core/llm-chat.test.ts 中增加一个用例，在 sendMessageStream 内部强制触发压缩，并用一个桩替换 Config.getBaseLlmClient().generateText，让其 retryWithBackoff 上报一次重试，然后断言 LlmChatSendOptions 中提供的 onRetry 被以该延迟调用。删除新增的转发时该测试必须变红；目前仓库中没有任何东西在观察压缩重试的上报。
— qwen3.8-max via Qwen Code /review (v0.23.2)
There was a problem hiding this comment.
Not fixing this one — it needs a maintainer decision. Escalating rather than leaving it silently.
I verified the structural claim at head instead of taking the summary's word for it, and it holds:
LlmChatSendOptions.onRetryis the new field (llm-chat.ts:453-458), documented as "Reports retry backoff so background-agent liveness can extend its deadline".- It reaches only the round's own model call:
makeApiCallAndProcessStream's param (:4593,:4691), invoked at:4655, fed from:3335,:3996,:4435. - Neither
tryCompresscall site passes it (:2864,:3664), andTryCompressOptions(:481onward) has noonRetryfield, so no caller could surface it. baseLlmClient.ts:307and:462wireonRetrytologApiRetryonly, alongsidepersistentMode: isUnattendedMode()(:300);GenerateTextOptionsexposes noonRetry.agent-core.ts:1000is the only producer that reaches the watchdog.
So the compression side query's capacity wait really is invisible to the model deadline, while the round's own identical wait extends it by up to MAX_RETRY_DEADLINE_EXTENSION_MS.
Why I am escalating rather than fixing:
- The fix changes watchdog deadline semantics, not reporting plumbing. Wiring compression retries into
onRetrymeans a compression 429 now extends the model deadline — it changes when a background agent is aborted and settledfailed. On a PR whose entire subject is timeout semantics, that is the human-gated class, not a mechanical defect like the twin guard I did fix. - Blast radius. Three source files plus two design docs (
llm-chat.ts,chatCompressionService.ts,baseLlmClient.ts), and it adds a caller-supplied callback toBaseLlmClient.generateText's options — a new field on a widely used core entry point. - The underlying decision is not mine. Should an unattended compression capacity-wait buy the same six-hour extension as the round's own model call, or should compression stay
maxAttempts: 1and fail to NOOP fast? The file argues for the latter atchatCompressionService.ts:749-751("don't burn 7 retries blocking the user mid-turn"), and the design doc's contract argues for the former. Those pull in opposite directions, and whichever way it goes changes behaviour for every unattended run.
One correction to offer so the decision is made on accurate footing: the witness here is marked not run, and MODEL_CONTROL_PROGRESS_TIMEOUT_MS is a hardcoded constant with no knob, so nobody has observed this end to end. The chain reads sound, but the trigger needs QWEN_CODE_UNATTENDED_RETRY=1 and a 429/529 on the compression side query whose cumulative wait outlasts the remaining model budget. Worth knowing that this is a read-verified rather than observed failure before deciding how urgently to reshape the compression path.
There was a problem hiding this comment.
[Critical] R6-6: [certifies-falsely] [new-surface] retainsPhysicalSlot has exactly two release sites — this .finally and its twin at background-agent-resume.ts:1452 — and both require the turn promise to settle, so a watchdog kill of a run that never physically settles retains the slot permanently, and the watchdog has already flipped the entry to failed, which removes the task_stop remedy that previously unblocked the session-switch gate.
Failure scenario: A background agent's tool ignores the abort signal (the case cancel()'s own comment at background-tasks.ts:957-960 names: "the rare case where a tool ignores AbortSignal and bgBody never settles"). The watchdog aborts at the 10 min tool deadline, and 5 s later failUnresponsive sets entry.status = 'failed', entry.notified = true, entry.retainsPhysicalSlot = true (background-tasks.ts:920-931). bgBody never returns, so runBackgroundTurn's .finally never runs and the flag is never deleted. From then on, for the life of the process: (1) hasRunningTasks() returns true via the clause this diff added (background-tasks.ts:1497, diff line 1330), so hasBlockingBackgroundWork() is true and /clear (clearCommand.ts:47), /resume (useResumeCommand.ts:107), /branch (useBranchCommand.ts:112) and the opentui session-switch (session-switch.ts:101,267) all refuse permanently; (2) the refusal message names the blocker as "— still stopping" and tells the user "Use /tasks to inspect them, then retry", but task_stop on that entry hits if (agentEntry.status !== 'running') return notRunningError('agent', taskId, agentEntry.status) (task-stop.ts:74-75) and cancel()/the dialog's cancelSelected hit if (!entry || entry.status !== 'running') return; (background-tasks.ts:969) — both no-ops, so there is no user-reachable clear; (3) pruneTerminalEntries
Witness:
[probe] A1 after failUnresponsive {"status":"failed","retainsPhysicalSlot":true,"notified":true,"inMap":true,"hasRunningTasks":true} notification emitted, meta.recordOnly = true status = failed A2 after cancel() [task_stop / dialog stop] {…retainsPhysicalSlot:true, hasRunningTasks:true} A3 after abandon() {…retainsPhysicalSlot:true, hasRunningTasks:true} A4 after abortAll({notify:false}) {…retainsPhysicalSlot:true, hasRunningTasks:true} A5 after late fail()/complete() {…retainsPhysicalSlot:true, hasRunningTasks:true} A6 af
Suggested fix: Give the retained slot a bounded exit. Either mirror cancel()'s deferred fallback inside failUnresponsive — an unref'd setTimeout that calls releaseRetainedPhysicalSlot(agentId) after a hard grace, so the flag can never outlive a bounded window — or (better, because it restores the user's remedy) let cancel() accept a failed-with-retainsPhysicalSlot entry and force-release: if (entry.retainsPhysicalSlot) { this.releaseRetainedPhysicalSlot(agentId); return; } ahead of the status !== 'running' early return, and drop the matching status !== 'running' refusal in `task-stop.ts
The fix must not violate this existing fact: background-tasks.ts:1489-1491 — "A watchdog-terminal run still counts while its underlying execution holds a physical slot, so session reset cannot erase the only remaining owner." A forced release re-opens the /clear gate, and reset() (background-tasks.ts:1512-1535) clears the map without aborting entry.abortController (unlike the workflow registry, which aborts
Acceptance criterion: packages/core/src/agents/background-tasks.test.ts — a new case that calls failUnresponsive(agentId, …), leaves the turn promise unsettled, then (fake timers) advances past the fallback grace and asserts entry.retainsPhysicalSlot is undefined and registry.hasRunningTasks() is false; equivalently, a task-stop test asserting a force-clear on a failed+retained entry Please confirm the mutation that proves it (remove the guard, run that test, confirm it goes red).
中文说明
retainsPhysicalSlot 只有两个释放点（此处与 background-agent-resume.ts:1452），且都要求轮次 Promise 落定。因此当被看门狗杀死的运行在物理上永不结束时，该槽位永久保留：hasRunningTasks() 持续为真，/clear、/new、/resume、/branch 与会话切换被无限期拒绝，而用户侧没有任何可达的解除手段。
— qwen3.8-max via Qwen Code /review (v0.23.3)
There was a problem hiding this comment.
Real bug, but the compression side query does not go through makeApiCallAndProcessStream: tryCompress -> ChatCompressionService.compress calls config.getBaseLlmClient().generateText(...) directly (chatCompressionService.ts ~line 855), so the onRetry callback threaded only into makeApiCallAndProcessStream never reaches it. Reporting compression backoff requires either adding an onRetry channel to compress() or changing generateText/the base LLM client, which is cross-cutting. Deferring to a focused pass. Left unresolved.
There was a problem hiding this comment.
[Critical] R6-6: [certifies-falsely] [regression] Still stands — retainsPhysicalSlot has exactly two release sites and both require the turn promise to settle, so a watchdog kill of a run that never physically settles retains the slot permanently, and the watchdog has already flipped the entry to failed, which removes the task_stop escape
A background agent's tool ignores AbortSignal. The watchdog aborts; 5 s later failUnresponsive sets status=failed, notified=true and retainsPhysicalSlot=true. Because the tool never returns, runBody never settles, so neither .finally release site runs. Every escape is closed: cancel() bails on status !== running, abandon() requires paused, pruneTerminalEntries() explicitly exempts retained entries, and reset() sits behind the very gate that is stuck. For the rest of the process lifetime hasRunningTasks() stays true, so /clear, /resume, /branch and session switch are refused forever with a "— still stopping" row the user cannot act on, and getRunningBackgroundCount() keeps counting the entry, so each hang permanently costs one of the 10 background slots. The daemon path escapes only because Session.ts requests a runtime recycle that kills the child; the interactive TUI has no equivalent.
Witness:
[probe] mutation over the real population: releaseRetainedPhysicalSlot has 3 hits repo-wide (definition background-tasks.ts:943, call sites agent.ts:3934 and background-agent-resume.ts:1452) and 0 in any test; deleting all three retained-slot terms (background-tasks.ts:1332/1497/1876) leaves background-tasks.test.ts green at 151 passed, while the positive control (entry.retainsPhysicalSlot = undefined in failUnresponsive) fails 1 — "retains the physical slot when the watchdog escalates a cancelled agent" — proving the suite reaches the module and pins only the flag, never its consequences.
Suggested fix:
Bound the retention the way the cancel path bounds its slot-holding state: in failUnresponsive arm an unref'd fallback timer that calls releaseRetainedPhysicalSlot(agentId) after a grace window, so a run that never settles cannot hold the gate and a concurrency slot for the process lifetime.
The fix must not violate this existing fact: hasRunningTasks()'s documented contract — "A watchdog-terminal run still counts while its underlying execution holds a physical slot, so session reset cannot erase the only remaining owner." (background-tasks.ts:1489-1493) — so the fix must be a delayed release, not removing entry.retainsPhysicalSlot from that predicate.
Please confirm the mutation that proves it — packages/core/src/agents/background-tasks.test.ts, beside the existing failUnresponsive/retainsPhysicalSlot case: with fake timers, advance past the new grace window while the turn promise stays unsettled and assert hasRunningTasks() === false and that a fresh admission at the cap succeeds. Red if the timer is deleted.
中文说明
retainsPhysicalSlot 只有两个释放点——启动路径的 .finally 与恢复路径的这一个——两者都要求轮次 Promise 落定；而这恰恰是该标志位存在的前提所不成立的情形。看门狗已经先把条目置为 failed，这又移除了 task_stop 这条逃生路径。
后果（交互式 TUI 没有运行时回收可依赖）：hasRunningTasks() 永远为真，/clear、/resume、/branch 与会话切换被永久拒绝，并显示一行用户无法操作的 “— still stopping”；getRunningBackgroundCount() 持续计数，每次挂起都永久消耗 10 个后台并发槽中的一个，直到重启 CLI。cancel() 因 status !== 'running' 提前返回，abandon() 要求 paused，pruneTerminalEntries() 明确豁免保留条目，reset() 又位于被卡住的那道门之后。
修复方向：像取消路径用 CANCEL_GRACE_MS 约束其占槽状态那样约束保留——在 failUnresponsive 中装配一个 unref 的兜底定时器，在宽限窗口后调用 releaseRetainedPhysicalSlot(agentId)。
— qwen3.8-max via Qwen Code /review (v0.23.3)
There was a problem hiding this comment.
Design-level — left open deliberately. Making the compression side-query retry visible to the watchdog requires a retry hook on the shared GenerateTextOptions interface (there is no onRetry today), so the fix is a new option on a cross-cutting interface rather than a guard correction. Out of scope here; it needs to be designed with the other GenerateTextOptions consumers. Evidence anchor: packages/core/src/core/llm-chat.ts:564.
There was a problem hiding this comment.
Re-verified at the post-merge head 93eef769a7. The chain holds end to end:
LlmChatSendOptions.onRetry(packages/core/src/core/llm-chat.ts:564) reaches only the round's own model call;TryCompressOptions(:588) has no such field, so bothtryCompresscall sites drop it.- The compression path calls
config.getBaseLlmClient().generateText(...)directly (packages/core/src/services/chatCompressionService.ts:844, withmaxAttempts: 1at:884), andGenerateTextOptions(packages/core/src/core/baseLlmClient.ts:78) has noonRetry— the hook at:307islogApiRetryonly.
The minimal change set is 3 files (llm-chat.ts, chatCompressionService.ts, baseLlmClient.ts), but it adds a caller-supplied callback to the exported GenerateTextOptions entry point, which other consumers share (packages/core/src/utils/sideQuery.ts:254), and it settles a liveness-policy question that should not be decided in a closeout round: should a compression side query's provider backoff extend a background agent's model deadline at all, or can that mask a genuinely wedged agent? No guard-level fix is available without that decision. Leaving unresolved.
Ledger note: this thread's round-8 body describes a different finding — retainsPhysicalSlot release sites (packages/core/src/agents/background-tasks.ts:935, :943), released only from agent.ts:3916 and background-agent-resume.ts:1437. If that one is the live entry under this id, it has the same shape: a bounded exit for a run that never physically settles is a decision about what the retained concurrency slot means, so it is not a guard fix either.
There was a problem hiding this comment.
Round jmtz4tb6u1d independent re-verification at head 93eef769a7. This thread carries two different findings -- the round-6 body is the compression side query, the round-8 body is retainsPhysicalSlot. Both re-derived from source. Both REAL, both human-gated.
(a) Compression retry invisible to the model deadline. Chain confirmed link by link:
llm-chat.ts:564--onRetry?: (delayMs: number) => voidonLlmChatSendOptions(:560).llm-chat.ts:588--TryCompressOptionsdeclares no such field, and neithertryCompresscall site passes one (:3102,:3965).chatCompressionService.ts:844-- compression reaches the provider viaconfig.getBaseLlmClient().generateText({...})withmaxAttempts: 1at:884.baseLlmClient.ts:78--GenerateTextOptionsexposes noonRetry; the hooks at:307and:462arelogApiRetryonly, sitting next topersistentMode: isUnattendedMode()(:300,:455).
Gate: the minimal change adds a caller-supplied callback to the exported GenerateTextOptions, which other consumers share (utils/sideQuery.ts:254), and it settles a liveness-policy question -- may a compression side query's provider backoff extend a background agent's model deadline, or does that mask a genuinely wedged agent? chatCompressionService.ts:749-751 argues the latter; this PR's design doc argues the former. Not a closeout-round decision.
(b) retainsPhysicalSlot has no bounded exit -- new evidence, and it cuts against the suggested fix. The structural half confirms: failUnresponsive sets the flag at background-tasks.ts:943; the only two release sites are the .finally pair (agent.ts:3916, background-agent-resume.ts:1437); hasRunningTasks() counts it at :1512; pruneTerminalEntries() exempts it at :1890; and cancel() bails on status !== 'running' at :984, which failUnresponsive already flipped to failed at :940. So there is genuinely no user-reachable remedy in the interactive TUI, which has no runtime recycle to fall back on.
But the proposed remediation (an unref'd fallback timer inside failUnresponsive calling releaseRetainedPhysicalSlot) contradicts this PR's own documented invariant at background-tasks.ts:920-934: "Reaching this method at all proves the execution is still alive (the watchdog is detached once it settles), so the physical slot must still be retained ... otherwise getRunningBackgroundCount and hasRunningTasks() free a concurrency slot that is still occupied, and /clear, /resume, /branch and session switches all proceed over live work." A delayed auto-release does exactly that once it fires, and reset() clears the map without aborting entry.abortController, so a still-running orphan loses its only owner. The force-release-in-cancel() variant is the same trade with a human in the loop, and additionally needs the matching status !== 'running' refusal dropped in task-stop.ts.
Gate: choosing between an unrecoverable stuck gate and freeing a slot a live orphan still occupies is concurrency-primitive semantics with a data-loss edge. Needs the maintainer to pick a side, plus the grace bound if the automatic release is chosen.
No code change this round. Leaving unresolved.
| const info = admissibleChannelInfo(); | ||
| if (!info) { | ||
| throw Object.assign( | ||
| new Error(`No live ACP channel for runtime MCP remove: ${name}`), |
There was a problem hiding this comment.
[Critical] R1-36: [certifies-falsely] [regression] Still stands — the workspace-scoped runtime-MCP routes resolve their target through admissibleChannelInfo(), so during a drain they act on the replacement generation while every live session stays on the draining one. removeRuntimeMcpServer silently no-ops with HTTP 200, and addRuntimeMcpServer broadcasts a workspace-wide event that no existing session can observe.
Carried forward under its original id from the round-1 ledger; re-traced and re-measured at HEAD 6bc80c0df4. Commit d7f26da1b5 ("stop routing workspace mutations to draining generations") moved generateWorkspaceAgent (:14435), addRuntimeMcpServer (:14558) and removeRuntimeMcpServer (:14613) from liveChannelInfo() to admissibleChannelInfo(). But after a recycle marks generation A draining and publishes active B, admissibleChannelInfo() returns B — the generation that owns no sessions — so the incident's step is unchanged.
A background Agent in session S1 ignores abort; Session.ts:9788 requests the recycle → requestRuntimeRecycleForSession sets A to draining, defers its retirement (retireWhenSessionsDrain = true, :3765, because S1/S2 are still attached) and ensureChannel('recovery') spawns B and assigns channelInfo = B (:5220). DELETE /workspaces/:w/mcp/servers/internal-db then resolves to B; B never had internal-db, so the child answers {name, skipped: true, reason: 'not_present'} (the RemoveSkip shape at :14626), no mcp_server_removed event is broadcast, and the route returns HTTP 200 with that skip body (workspace-mcp-control.ts:634-641). internal-db stays connected in generation A — which still owns every live session — and its tools remain callable from S1/S2 for as long as A lives (arbitrarily long: A is retired only when its sessions drain). The symmetric add writes the server into B only, yet broadcasts mcp_server_added to all workspace subscribers, so the Web Shell lists a server that sessions on A cannot call. The session-scoped twins in the same object were not retargeted — addSessionRuntimeMcpServer/removeSessionRuntimeMcpServer route through requestSessionStatus(sessionId, …) (:14658-14678), which resolves the entry's owning generation — so the two scopes now disagree about which child is "the" runtime. This violates issue #8586's Layer-4 acceptance criterion "Runtime draining preserves owner-generation routing".
Witness (probe against the real bridge):
ROW A (remove, after recycle):
add result = {"name":"internal-db","transport":"stdio",...,"toolCount":1} -> gen1
generations after recycle = 2
workspace remove result = {"name":"internal-db","skipped":true,"reason":"not_present"}
gen1: calls ["workspace/mcp/runtime-add","session/mcp/runtime-remove"] workspaceServers ["internal-db"] killed false
gen2: calls ["workspace/mcp/runtime-remove"] workspaceServers [] killed true
ROW F (add, after recycle; subscriber attached to the session on the DRAINED generation):
s1 still on gen1 (gen1 alive) = true
add result = {"name":"internal-db",...,"toolCount":3}
gen1 (owns s1) servers = [] calls = []
gen2 (replacement) servers = ["internal-db"] calls = ["qwen/control/workspace/mcp/runtime-add"]
event seen by the session on the DRAINED generation = {"type":"mcp_server_added","data":{"name":"internal-db","toolCount":3}}
Correction to the causal framing, which does not change the verdict: the liveChannelInfo()→admissibleChannelInfo() swap is not what retargets these routes during a drain — liveChannelInfo() also returns the replacement, because channelInfo is the replacement and is not dying. The retargeting comes from the recycle repointing channelInfo; the swap's own effect is to refuse (acp_channel_unavailable) when channelInfo itself is draining with no replacement spawned.
Suggested fix: route workspace-scoped MCP mutations to every work-owning generation rather than to the admission-eligible one — fan out over Array.from(aliveChannels).filter((c) => c.state !== 'dying'), aggregate the per-generation results, and report not_present only when no generation had the server (suppressing the broadcast in that case). If fan-out is rejected as too broad, refuse instead of silently skipping — throw the retryable BridgeRuntimeRecyclingError this diff already added (bridgeErrors.ts:676) when liveChannelInfo() !== admissibleChannelInfo(), so the caller retries after the drain instead of getting a false success.
Any fix must respect this: packages/cli/src/serve/acp-http/client-mcp-sender-registry.ts:265-270 — "the workspace-scoped add fans out to every active session AND is copied onto every session created later (acp-integration/acpAgent.ts)"; that fan-out is within one child process (Config.runtimeMcpServers, packages/core/src/config/config.ts:2287), so a fix must fan out across generations at the bridge layer, not rely on the child.
Please add the test that pins this: in packages/acp-bridge/src/bridge.test.ts, mark a generation draining while it owns a session, spawn the replacement, then issue a workspace-scoped MCP mutation and assert it was dispatched to the owner generation's connection (or that the call rejected with BridgeRuntimeRecyclingError) rather than resolving to a not_present skip on the replacement. Routing it through admissibleChannelInfo() must turn it red. The generation machine currently has zero test references, so no existing case covers this.
中文说明
[Critical] R1-36：依然存在 —— workspace 作用域的 runtime MCP 路由通过 admissibleChannelInfo() 解析目标，因此在排空期间它们作用在替代 generation 上，而所有存活会话仍留在正在 draining 的那一个上。removeRuntimeMcpServer 会以 HTTP 200 静默空转，addRuntimeMcpServer 则会广播一个没有任何现存会话能观察到的 workspace 级事件。
本条以原始 id 从第 1 轮的账本中延续下来，并在 HEAD 6bc80c0df4 上重新追踪、重新实测。提交 d7f26da1b5（“stop routing workspace mutations to draining generations”）把 generateWorkspaceAgent（:14435）、addRuntimeMcpServer（:14558）和 removeRuntimeMcpServer（:14613）从 liveChannelInfo() 改为 admissibleChannelInfo()。但当一次回收把 generation A 标记为 draining 并发布了处于 active 的 B 之后，admissibleChannelInfo() 返回的是 B —— 那个不拥有任何会话的 generation —— 因此事故中的这一步没有改变。
会话 S1 中的一个后台 Agent 忽略中止；Session.ts:9788 请求回收 → requestRuntimeRecycleForSession 把 A 设为 draining，延迟其退役（retireWhenSessionsDrain = true，:3765，因为 S1/S2 仍挂在其上），并且 ensureChannel('recovery') 派生出 B 并把 channelInfo = B（:5220）。随后 DELETE /workspaces/:w/mcp/servers/internal-db 解析到 B；B 从来没有过 internal-db，于是子进程回答 {name, skipped: true, reason: 'not_present'}（:14626 处的 RemoveSkip 形状），不广播任何 mcp_server_removed 事件，路由则以该 skip 响应体返回 HTTP 200（workspace-mcp-control.ts:634-641）。internal-db 仍连接在 generation A 上 —— 而 A 仍然拥有每一个存活会话 —— 只要 A 还活着（可能无限久：A 只在其会话排空后才退役），它的工具就仍可从 S1/S2 调用。对称的 add 只把服务器写入 B，却向所有 workspace 订阅者广播 mcp_server_added，于是 Web Shell 会列出一个 A 上的会话无法调用的服务器。同一对象中会话作用域的孪生方法没有被改向 —— addSessionRuntimeMcpServer/removeSessionRuntimeMcpServer 走 requestSessionStatus(sessionId, …)（:14658-14678），会解析到条目所属的 generation —— 因此这两种作用域现在对“哪个子进程才是那个 runtime”给出了互相矛盾的答案。这违反了 issue #8586 第 4 层的验收条件“Runtime draining preserves owner-generation routing”。
证据（对真实 bridge 的探针）见上方英文代码块 ROW A / ROW F：workspace 作用域的 remove 落到了不拥有任何会话的 generation 上，并以成功形状的 200 skip 返回，而拥有存活会话的 generation 仍保留着 internal-db；workspace 作用域的 add 只落在替代 generation 上，却把 mcp_server_added 广播给了一个其会话 runtime 并不拥有该服务器的订阅者。两种作用域已被实测证明不一致（ROW A：workspace→gen2，session→gen1）。
对因果表述的一处更正，不改变结论：liveChannelInfo()→admissibleChannelInfo() 的替换并不是排空期间让这些路由改向的原因 —— liveChannelInfo() 同样会返回替代者，因为 channelInfo 就是替代者且并未 dying。改向来自回收把 channelInfo 重新指向；这次替换本身的效果是：当 channelInfo 自身处于 draining 且尚未派生替代者时予以拒绝（acp_channel_unavailable）。
修复建议：把 workspace 作用域的 MCP 变更路由到每一个拥有工作的 generation，而不是路由到可准入的那一个 —— 对 Array.from(aliveChannels).filter((c) => c.state !== 'dying') 做扇出，聚合各 generation 的结果，并且只有在没有任何 generation 持有该服务器时才报告 not_present（此时同时抑制广播）。如果认为扇出过宽，那就拒绝而不是静默跳过 —— 当 liveChannelInfo() !== admissibleChannelInfo() 时抛出本 diff 已经加入的可重试 BridgeRuntimeRecyclingError（bridgeErrors.ts:676），让调用方在排空后重试，而不是拿到一个虚假的成功。
修复必须遵守：packages/cli/src/serve/acp-http/client-mcp-sender-registry.ts:265-270 —— “workspace 作用域的 add 会扇出到每一个活跃会话，并且会被复制到之后创建的每一个会话（acp-integration/acpAgent.ts）”；该扇出发生在单个子进程内部（Config.runtimeMcpServers，packages/core/src/config/config.ts:2287），因此修复必须在 bridge 层跨 generation 扇出，而不能依赖子进程。
请补上能钉住这一点的测试：在 packages/acp-bridge/src/bridge.test.ts 中，让某个 generation 在拥有会话的情况下被标记为 draining，派生出替代者，然后发起一次 workspace 作用域的 MCP 变更，并断言它被派发到了所有者 generation 的连接上（或者该调用以 BridgeRuntimeRecyclingError 拒绝），而不是在替代者上解析成 not_present 跳过。让它经由 admissibleChannelInfo() 时该测试必须变红。目前整个 generation 机制的测试引用数为零，因此没有任何既有用例覆盖此点。
— qwen3.8-max via Qwen Code /review (v0.23.2)
There was a problem hiding this comment.
Leaving this unresolved. It is a routing decision, not a mechanical defect, and I could not find a fix that stays inside the guardrails this PR needs.
Verified at head, and the report's own correction to the causal framing is the load-bearing part:
liveChannelInfo()=channelInfo && !channelInfo.isDying(bridge.ts:6248-6249)admissibleChannelInfo()=channelInfo?.state === 'active'(:6251-6252)
After a recycle repoints channelInfo at the replacement, both return the replacement. So the liveChannelInfo() → admissibleChannelInfo() swap is not what retargets these routes during a drain, and reverting it would not fix the incident. I confirmed the swap's scope with git show d7f26da1b5: exactly three -/+ pairs, and all three workspace routes now read admissibleChannelInfo() at :14435, :14557, :14613.
Why I am not picking a fix. The repo already has an owner-generation resolver — channelInfoForEntry(entry) (:6254-6261) walks aliveChannels for the generation that owns a session, and that is what the session-scoped twins use (:14658-14678). But it is keyed by session. A workspace-scoped mutation has no single owning entry: during a drain there can be N sessions on the draining generation and zero on the replacement. Expressing "every work-owning generation" requires a new selection semantic, which is precisely the design question raised here.
Both proposed options change a contract I should not change unilaterally in a closeout pass:
- Fan-out over
aliveChannels.filter(c => c.state !== 'dying')with aggregated results makes a workspace mutation multi-target, and needs a new rule for partial success (gen1 removed, gen2not_present) plus a decision on when the broadcast is legitimate. - Refusing with
BridgeRuntimeRecyclingError(bridgeErrors.ts:676) turns an HTTP 200 skip into a retryable error on a public route (workspace-mcp-control.ts:634-641), and to stay coherent it would have to apply togenerateWorkspaceAgentas well — a behaviour change for every caller of three endpoints.
The question for the maintainer, stated plainly: after a generation hand-off, which child is "the" runtime for a workspace-scoped MCP mutation, given that every live session sits on the draining generation and the replacement owns none? Whichever way that goes, the two scopes need to agree — today the session-scoped routes resolve the owner generation and the workspace-scoped ones resolve the admissible generation, which is the actual inconsistency behind the not_present skip and the unobservable mcp_server_added broadcast.
There was a problem hiding this comment.
[Critical] R1-36: [certifies-falsely] [regression] Still stands — the workspace-scoped runtime-MCP routes resolve their target through the newest admissible generation rather than the generation that owns the workspace's live sessions, so during a drain they mutate an empty replacement child while every live session stays on the draining one that never receives the change.
Failure scenario: A recycle marks generation A draining and publishes an empty replacement B. addRuntimeMcpServer resolves one channel via admissibleChannelInfo() and round-trips workspaceMcpRuntimeAdd only over that connection, then broadcasts mcp_server_added to every client. A is never told and cannot be: runtime-added servers live in a private per-Config map that loadCliConfig does not re-read, and the child-side handler fans out only to its own active sessions. A model-driven tools/call for that server from the live session then fails 'not found in registry' while the UI shows it as added. removeRuntimeMcpServer silently no-ops with HTTP 200.
Witness:
Read at HEAD 8b23578fc9 (Agent 0, issue-fidelity): commit d7f26da1b5 switched generateWorkspaceAgent (bridge.ts:14664), addRuntimeMcpServer (:14786) and removeRuntimeMcpServer (:14842) from liveChannelInfo() to admissibleChannelInfo(); addRuntimeMcpServer still resolves a single channel — the newest active one — and round-trips workspaceMcpRuntimeAdd only over info.connection, then broadcasts mcp_server_added to every client. queryWorkspaceStatus (:12601) and setUserLanguage (:13358) still take liveChannelInfo(). No workspace-mutation route fans out across aliveChannels or resolves to the session-owning generation.
Suggested fix: Give the retained slot a bounded exit. Either mirror cancel()'s deferred fallback inside failUnresponsive — an unref'd setTimeout that calls releaseRetainedPhysicalSlot(agentId) after a hard grace, so the flag can never outlive a bounded window — or (better, because it restores the user's remedy) let cancel() accept a failed-with-retainsPhysicalSlot entry and force-release: if (entry.retainsPhysicalSlot) { this.releaseRetainedPhysicalSlot(agentId); return; } ahead of the status !== 'running' early return, and drop the matching status !== 'running' refusal in `task-stop.ts
The fix must not violate this existing fact: background-tasks.ts:1489-1491 — "A watchdog-terminal run still counts while its underlying execution holds a physical slot, so session reset cannot erase the only remaining owner." A forced release re-opens the /clear gate, and reset() (background-tasks.ts:1512-1535) clears the map without aborting entry.abortController (unlike the workflow registry, which aborts
Acceptance criterion: packages/core/src/agents/background-tasks.test.ts — a new case that calls failUnresponsive(agentId, …), leaves the turn promise unsettled, then (fake timers) advances past the fallback grace and asserts entry.retainsPhysicalSlot is undefined and registry.hasRunningTasks() is false; equivalently, a task-stop test asserting a force-clear on a failed+retained entry Please confirm the mutation that proves it (remove the guard, run that test, confirm it goes red).
中文说明
工作区级的运行时 MCP 路由仍把目标解析为“最新的可接纳 generation”，而不是“拥有该工作区活跃会话的 generation”。回收把 A 标记为 draining 并发布空的替代 B 后，addRuntimeMcpServer 只通过 B 的连接往返 workspaceMcpRuntimeAdd，却向所有客户端广播 mcp_server_added；A 永远收不到，也无法收到（运行时添加的服务器存放在 loadCliConfig 不会重读的 per-Config 私有映射中，子进程侧只向自己的活跃会话扇出）。于是来自活跃会话的 tools/call 报 “not found in registry”，而 UI 显示已添加；removeRuntimeMcpServer 则静默 no-op 并返回 HTTP 200。
— qwen3.8-max via Qwen Code /review (v0.23.3)
There was a problem hiding this comment.
[Critical] R1-36: [certifies-falsely] [regression] Still stands — the workspace-scoped runtime-MCP routes resolve their target through admissibleChannelInfo(), so during a drain they act on the replacement generation while every live session stays on the draining one that never receives the change
A recycle repoints channelInfo at the replacement generation while the draining one still owns the workspace's live sessions. addRuntimeMcpServer and removeRuntimeMcpServer both resolve through admissibleChannelInfo(), so the ext-method round trip goes to the empty replacement child: removeRuntimeMcpServer silently no-ops with HTTP 200 while the sessions that still need the server keep it, and addRuntimeMcpServer broadcasts a workspace-wide mcp_server_added event for a server the live sessions never got.
Witness:
not run — scratch-tree refused (available: false); settled by quoted lines at HEAD 90e2d6f7ae: `const info = admissibleChannelInfo();` at bridge.ts:14795 (add) and :14851 (remove), against liveChannelInfo() = channelInfo && !channelInfo.isDying (:6316-6317) and admissibleChannelInfo() = channelInfo?.state === 'active' (:6319-6320). The author replied "Leaving this unresolved. It is a routing decision, not a mechanical defect" and verified the same two predicates at head; a reply does not retire a blocker.
Suggested fix:
Resolve workspace-scoped runtime-MCP mutations against the generation that owns the workspace's live sessions (fan out to every non-dying generation, or route by session ownership) rather than the newest admissible one, or return the retryable 503 so the caller can wait for the drain to finish.
The fix must not violate this existing fact: bridgeTypes.ts:2531-2534 states a draining generation "still owns existing sessions but cannot accept new work", so a fix must not route fresh session admission back through liveChannelInfo(); the admissible predicate is correct for admission and wrong for mutating owned sessions.
Please confirm the mutation that proves it — packages/acp-bridge/src/bridge.test.ts — a case that puts a session on gen1, recycles, and asserts a runtime MCP remove reaches gen1 (the generation owning the session) rather than no-oping on gen2. Red if the route is switched back to admissibleChannelInfo().
中文说明
工作区级的 runtime-MCP 路由通过 admissibleChannelInfo() 解析目标，因此在排空期间它们操作的是替代代，而所有活跃会话仍留在 draining 代上，永远收不到这次变更。
后果：removeRuntimeMcpServer 会静默空操作并返回 HTTP 200，而仍需该服务器的会话照样持有它；addRuntimeMcpServer 会广播一个工作区级的 mcp_server_added 事件，而活跃会话从未拿到该服务器。
作者已回复“保持未解决——这是路由决策而非机械缺陷”，并在 head 上核实了同样的两个判定式。回复本身不能撤销阻塞项。
— qwen3.8-max via Qwen Code /review (v0.23.3)
There was a problem hiding this comment.
Design-level — left open deliberately. Fixing this means giving the workspace-scoped runtime-MCP routes a draining-aware, generation-anchored resolution instead of admissibleChannelInfo(), which changes the routing contract for every one of those routes during a drain. That is a lifecycle/routing semantics change, not a guard fix, so it is not landable inside this round. Evidence anchor: packages/acp-bridge/src/bridge.ts:14854.
There was a problem hiding this comment.
Re-verified at the post-merge head 93eef769a7. Still stands: generateWorkspaceAgent (packages/acp-bridge/src/bridge.ts:14699), addRuntimeMcpServer (:14821) and removeRuntimeMcpServer (:14877) all resolve const info = admissibleChannelInfo(), against liveChannelInfo() = channelInfo && !channelInfo.isDying (:6316-6317) and admissibleChannelInfo() = channelInfo?.state === 'active' (:6319-6320). The session-scoped twins resolve by owning entry instead, so the two scopes disagree.
New evidence against the cheaper remediation proposed on this thread: refusing when liveChannelInfo() !== admissibleChannelInfo() does not fire in the reported scenario. After the recycle repoints channelInfo at the replacement, that replacement is alive and active, so both predicates return it — the guard only trips when channelInfo itself is draining with no replacement spawned, which is not the incident (the round-8 comment already noted this correction). Routing by session ownership is not expressible here either: a workspace-scoped mutation can face N sessions on the draining generation and zero on the replacement.
So the only fix that addresses the reported failure is generation-owning fan-out with new partial-success semantics — gen1 removed / gen2 not_present has to define a result, and the workspace-wide mcp_server_added broadcast has to become conditional. That is a routing-contract change, beyond this round. Leaving unresolved with the decision for the maintainer: fan out to every non-dying generation and define partial success, or reject workspace-scoped runtime-MCP mutations whenever a work-owning generation is not the active one.
There was a problem hiding this comment.
Round jmtz4tb6u1d independent re-verification at head 93eef769a7. REAL, and human-gated. Adding the structural proof for why the cheaper remediation cannot work -- the earlier reply asserted this, it is now derived:
Both predicates read the same single channelInfo binding:
liveChannelInfo()=channelInfo && !channelInfo.isDying ? channelInfo : undefined(bridge.ts:6318-6319)admissibleChannelInfo()=channelInfo?.state === 'active' ? channelInfo : undefined(bridge.ts:6321-6322)
They can therefore only disagree when channelInfo itself is non-dying and not active -- i.e. draining with no replacement spawned yet. Once a recycle repoints channelInfo at the replacement, both return the reference-identical object, so a liveChannelInfo() !== admissibleChannelInfo() guard is unreachable in the reported scenario. Rejecting on that predicate cannot fix the incident.
The scope disagreement confirms at this head: the workspace routes resolve const info = admissibleChannelInfo() at :14699 (generateWorkspaceAgent), :14821 (addRuntimeMcpServer) and :14877 (removeRuntimeMcpServer), while the session-scoped twins resolve by owning entry through requestSessionStatus(sessionId, ...) at :14927 and :14936 -- the same channelInfoForEntry(entry) resolution the rest of the session surface uses (:3167, :3258, :3535, :3803).
A generation-owning fan-out is expressible here (aliveChannels is iterated at :2821 and membership-tested at :4035), so the blocker is not feasibility -- it is the contract. Fanning out forces a partial-success result on the workspace HTTP route (gen1 removed / gen2 not_present has to resolve to something, and workspace-mcp-control.ts currently maps that skip shape to HTTP 200), and makes the workspace-wide mcp_server_added broadcast conditional.
Gate: routing-contract plus public API change, applied to every workspace-scoped runtime-MCP route during a drain. Decision for the maintainer, unchanged: fan out to every non-dying generation and define partial success, or reject workspace-scoped runtime-MCP mutations whenever a work-owning generation is not the active one.
No code change this round. Leaving unresolved.
There was a problem hiding this comment.
[Critical] R1-36: [certifies-falsely] [regression] Still stands — and this round measured a second, worse consequence of the same root: under the default idle policy the misrouted mutation destroys the replacement generation and leaves the workspace with no admissible generation at all.
generateWorkspaceAgent, addRuntimeMcpServer and removeRuntimeMcpServer all resolve their target through admissibleChannelInfo(), which returns the newest active generation. During a drain that is the empty replacement, not the generation that owns the workspace's live sessions. So removeRuntimeMcpServer returns { skipped: true, reason: 'not_present' } — which workspace-mcp-control.ts answers with HTTP 200 — and a revocation silently no-ops while the draining owner's sessions keep the server. addRuntimeMcpServer succeeds against the replacement and then broadcasts mcp_server_added workspace-wide, although the owner never received it. The session-scoped twins do keep owner routing, so the diff already contains the correct pattern.
The new consequence: the mutation's withWorkspaceControl finally calls startIdleTimer(replacement, 'workspace control'), and channelIdleTimeoutMs defaults to 0 in production (run-qwen-serve.ts: "0 = immediate kill"), so that kills the replacement outright. Its exited handler clears channelInfo, after which admissibleChannelInfo() returns undefined and startIdleTimer bails on its own non-primary guard — and none of the three routes calls ensureChannel, so every later workspace mutation fails acp_channel_unavailable until unrelated fresh-session work happens to spawn a new generation. The draining owner stays alive serving every session the whole time.
Issue #8586's Layer-4 acceptance criterion is explicit — "Runtime draining preserves owner-generation routing" — and these three routes do not preserve it. The failure is silent (HTTP 200) for a mutation whose whole purpose is revocation.
Witness:
probe driving the real createAcpSessionBridge (production default, channelIdleTimeoutMs unset):
removeRuntimeMcpServer during drain returned: {"name":"srv","skipped":true,"reason":"not_present"} -> HTTP route does res.status(200).json(result)
STATE after remove | gen1(owner of the live session): [] | gen2(replacement): ["runtime-remove"] | gen1.killed= false | gen2.killed= true
addRuntimeMcpServer during drain THREW: No live ACP channel for runtime MCP add: srv-new {"errorKind":"acp_channel_unavailable"}
control arm (channelIdleTimeoutMs: 300_000) | gen2.killed= false, gen2ext=["runtime-remove"] <- wrong-generation mutation reproduces without the kill
control arm (no recycle) | gen1ext=["runtime-remove"] <- owner routing is correct when nothing is draining
Route every workspace-scoped mutation through the generation that owns the workspace's live sessions — channelInfoForEntry, or an explicit generation parameter — rather than the newest admissible one, and give partial failure defined semantics instead of a 200 that means nothing happened. Separately, bracket the recycle's ensureChannel with the reservation/settle machinery so a session-less replacement is not immediately reaped.
Any fix must keep the session-scoped twins' owner routing intact — bridge.ts:14921-14941 already route through requestSessionStatus(sessionId, …), and the design doc's contract for Sessions ("Existing Session entries continue to route through their recorded channel while it drains") must not regress.
Please add the test that pins this: in packages/acp-bridge/src/bridge.test.ts, mark a generation draining while it owns a session, spawn the replacement, issue a workspace-scoped mutation and assert it was dispatched to the OWNER generation's connection — routing it through admissibleChannelInfo() must turn it red.
中文说明
[Critical] R1-36：依然存在——并且本轮实测到同一根因的第二个、更严重的后果：在默认 idle 策略下，被错误路由的变更会销毁替代 generation，使整个 workspace 不再有任何可受理的 generation。
generateWorkspaceAgent、addRuntimeMcpServer 与 removeRuntimeMcpServer 都通过 admissibleChannelInfo() 解析目标，而它返回的是最新的 active generation。在 drain 期间那是空的替代 generation，而不是拥有该 workspace 活跃会话的那个。于是 removeRuntimeMcpServer 返回 { skipped: true, reason: 'not_present' }——workspace-mcp-control.ts 会以 HTTP 200 回应——撤销操作静默失效，而正在 drain 的 owner 上的会话仍保留着该 server。addRuntimeMcpServer 在替代 generation 上成功，随后向整个 workspace 广播 mcp_server_added，尽管 owner 从未收到。会话级的同类方法确实保留了 owner 路由，因此本 diff 内部已经有正确的写法。
新发现的后果：变更结束时 withWorkspaceControl 的 finally 会调用 startIdleTimer(replacement, 'workspace control')，而生产环境中 channelIdleTimeoutMs 默认为 0（run-qwen-serve.ts："0 = immediate kill"），因此这会直接杀掉替代 generation。其 exited 处理器清空 channelInfo，此后 admissibleChannelInfo() 返回 undefined，startIdleTimer 又因自身的非 primary 判断而直接退出——而这三条路由都不调用 ensureChannel，于是之后每一次 workspace 变更都会以 acp_channel_unavailable 失败，直到某个无关的新会话工作恰好派生出新的 generation。整个过程中正在 drain 的 owner 一直存活并服务着所有会话。
issue #8586 的 Layer-4 验收标准写得很明确——"Runtime draining preserves owner-generation routing"——而这三条路由并未做到。对于一个以撤销为目的的变更，其失败还是静默的（HTTP 200）。
修复方向：让每一次 workspace 级变更都路由到拥有该 workspace 活跃会话的 generation——channelInfoForEntry，或显式的 generation 参数——而不是最新的可受理 generation；并为部分失败定义明确语义，而不是返回一个什么也没发生的 200。另外，请用 reservation/settle 机制包裹回收路径的 ensureChannel，使没有会话的替代 generation 不会被立即回收。
修复约束：必须保持会话级同类方法的 owner 路由不变——bridge.ts:14921-14941 已经通过 requestSessionStatus(sessionId, …) 路由，且设计文档对 Session 的约定（"Existing Session entries continue to route through their recorded channel while it drains"）不得回退。
请补充测试：在 packages/acp-bridge/src/bridge.test.ts 中，让某个 generation 在持有会话时被标记为 draining，派生替代 generation，发起一次 workspace 级变更，并断言它被派发到 owner generation 的连接上；若仍经 admissibleChannelInfo() 路由，该测试必须为红。
（证据见上方 Witness 代码块；witness 为程序输出，未翻译。）
— qwen3.8-max via Qwen Code /review (v0.23.3)
doSpawn's pre-`newSession` guard was tightened to the three-state model (`ci.state !== 'active'`), but the paired post-`newSession` re-check was left at the old two-state test `if (ci.isDying)`. `isDying` is a getter for `state === 'dying'`, so a `draining` channel sailed through it. A recycle landing inside the `newSession` round-trip takes exactly that path: `requestRuntimeRecycleForSession` sets `owner.state = 'draining'`, and because `hasNoSessionWork` counts the in-flight spawn (`inFlightSpawnCount = ci.sessionSpawnsInFlight`) it takes the defer branch — `retireWhenSessionsDrain = true`, no kill, `isDying` stays false. The pending `newSession` then resolved past a re-check that only tested `isDying`, registering a brand-new session on the condemned generation (and claiming `defaultEntry` under `single` scope). The fresh session shared the runtime the daemon had just judged unsafe for fresh work, and the generation could not drain until that session closed. Mirror the twin guard. The throw stays inside the existing `try` so the `finally` still decrements `sessionSpawnsInFlight`; leaking that counter would leave the drained generation unreapable, since it is what makes `reapPendingEmptyChannel` refuse to reap. For a draining channel the late child-side session is closed by the existing bounded `sessionClose` round-trip rather than the `await ci.channel.exited` short-circuit, and `reapPendingEmptyChannel` still declines to kill while a live session holds the generation. Test drives the real bridge: two sessions multiplexed on gen1, the second held inside `connection.newSession`, recycle fired for the first mid-flight, then released. Before the fix the spawn resolved with `sess-drain-b` installed on the draining generation; after it, the spawn rejects with `BridgeChannelClosedError` and gen1 is left alive for its surviving session. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Patrol-Run: qwen-pr-closeout/jmtusclhbul
|
Qwen Code review timed out. Qwen review timed out after 21600 seconds (of the 360-minute budget). This run already used the maximum 360 minute timeout. See workflow logs. |
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Review — head b19532e
Reviewed by reading the diff and the affected files at this SHA. No local test run.
Verdict: ESCALATE to maintainer. 28 files across packages/core/src/agents/**, packages/acp-bridge, packages/cli/src/serve/** and the TUI — cross-package, and it introduces a new abort authority over running agents. One Critical below. Bilingual design docs are present and paired, which I appreciated.
Critical — a clock-drift re-arm silently discards the retry deadline extension
packages/core/src/agents/runtime/agent-progress-watchdog.ts. schedule re-arms rather than firing when the timer came back early or late (:79-93):
const timer = setTimeout(() => {
if (performance.now() - expectedAt > 1_000) {
rearm();
return;
}
callback();
}, timeoutMs);and armModel passes itself as that rearm callback (:99-119):
const armModel = (retryDelayMs = 0) => {
...
modelTimer = schedule(
MODEL_CONTROL_PROGRESS_TIMEOUT_MS +
Math.min(retryDelayMs, MAX_RETRY_DEADLINE_EXTENSION_MS),
() => abort(...),
armModel,
);rearm is the bare function reference, so the re-arm call receives no argument and retryDelayMs falls back to its = 0 default. Any extension granted for a long provider backoff — up to MAX_RETRY_DEADLINE_EXTENSION_MS, i.e. six hours — is discarded the first time the drift branch fires, and the deadline collapses to the base MODEL_CONTROL_PROGRESS_TIMEOUT_MS of 15 minutes. The drift branch is precisely what triggers on laptop sleep/resume and on a loaded host, which is also when a long retry is most likely to be in flight, so an agent that is legitimately waiting gets aborted with AgentProgressTimeoutError('model/control', ...). Capturing the current retryDelayMs in the closure passed as rearm (() => armModel(retryDelayMs)) fixes it; a test that arms with a large retryDelayMs, forces the drift branch, and asserts the deadline is still extended would keep it fixed.
Retracting a concern I checked and could not sustain
I want to be explicit about this rather than leave it implied. ensureChannel(admission: 'fresh' | 'recovery' = 'fresh') at packages/acp-bridge/src/bridge.ts:4603-4604 reads admission only inside a stderr diagnostic at :4622:
`qwen serve: runtime recycling blocked ${admission} work; generations=${...}`That looks like an unpopulated parameter at a glance, but it is not: 'recovery' is passed at bridge.ts:3788, :8502 and :11366, with the default covering :4090, :14514 and :15106. A parameter that exists purely to make a log line say which kind of work was blocked is a legitimate diagnostic, and the distinction it records is genuinely useful when triaging a recycling stall. No change wanted here.
Suggestion — the model watchdog is fully disarmed while any tool is executing
:101-107:
if (
disposed ||
waitingForExternalInput ||
[...tools.values()].some(
(tool) => tool.state === 'executing' || tool.parkedOnInput === true,
)
)
return;That is the right call for the model deadline, and TOOL_PROGRESS_TIMEOUT_MS covers the tool itself, so there is no gap for a tool that is merely slow. The case I could not convince myself is covered is a tool that completes its own deadline bookkeeping but never transitions out of executing in this map — then neither timer is armed. Worth a test that drops a tool entry into a terminal-but-unreported state and asserts something still fires, if only to prove the state machine cannot reach it.
The clock-drift branch of `schedule` called `rearm()` with no argument, and `armModel` was passed as that bare reference, so the first drift re-arm dropped the granted provider-backoff extension and collapsed the model deadline back to the 15-minute base timeout. Re-arm through a closure capturing `retryDelayMs`. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Patrol-Run: qwen-pr-closeout/jmtvk7oj0vo
|
Thanks @doudouOUC — review 5167462288 confirmed at b19532e and fixed in 89cb7d6. Clock-drift re-arm. Regression test Suggestion — model watchdog disarmed while a tool executes: declined a test; the state is unreachable through the public API.
The 5 bot Criticals on this PR stay open as escalated maintainer decisions on watchdog/abort semantics, and are untouched here. Targeted verification: |
…t-watchdog Refresh the branch onto main so the Lint & Static lane passes the `Check lint gate freshness` gate (main changed .github/workflows/ci.yml in 17990c3). Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Patrol-Run: qwen-pr-conflict/jmtvkxeirvp
|
Qwen Code review did not complete successfully. The review pipeline failed before a review could be posted. A transient error is retried automatically; if you are seeing this, retry with |
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.
3 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- bridge.ts:5701 orphaned child-side session after the widened post-newSession rejection — already reported (R4-3 / R4-4, and answered by the author in the R6-4 thread)
- agent-progress-watchdog.ts:204 approval-park deadline — already reported (R6-1); mechanism re-verified true this round, not re-filed
- background-agent-resume.ts:1449 retained physical slot unbounded off-daemon — already reported (R1-21)
Unresolved, please confirm:
- [Critical] R6-5 packages/core/src/core/llm-chat.ts:457 — could not determine whether the new retry-delay reporting now reaches the context-compression side query: at HEAD that line sits inside getHardRescueFailureMessage (a message builder), and the c…
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally (Agent 7 ran per-workspace vitest only).
Not reviewed: test-efficacy — all three probes returned inconclusive (runner produced no parseable JSON), so no coverage-gap claim was measured either way and the positive control never ran (harnessValidated: null).
Not reviewed: reverse audit — stopped at the 5-round cap without two consecutive dry rounds; rounds 3, 4 and 5 all reported findings (chunks 5 and 6 retired on two-dry certificates).
Not explored to full depth (tool budget reached): "agent reverse-audit (round 4)": whether updateOutput is actually wired for a depth-2 nested background agent — i.e. whether the task_execution chunk that carries the new park flags reaches…; "agent reverse-audit (round 4)": none — I also examined, and deliberately did not file, the unguarded sessionRuntimeRecycle ext-method call in the finally when this.closing / this.disposed…; "agent reverse-audit (round 4)": did not finish tracing whether a workflow-dispatched agent can reach runBackgroundTurn (packages/core/src/tools/agent/agent.ts:3861-3866), i.e. whether the ne….
Deferred under the convergence posture (round 7, not a blocker) — recorded, not requested in this round; 3 Critical(s) among them are deferred by their axes — fails-closed on new surface, where no wrong result is certified and the merge base had neither the surface nor the defect — and remain follow-up work recorded in the findings artifact:
packages/acp-bridge/src/bridge.ts:3811 — [review] Critical [fails-closed] [new-surface] Recycle recovery spawn never reaches the idle/reap policypackages/acp-bridge/src/bridge.ts:5699 — [probe] Critical [fails-closed] [new-surface] R6-4: (fix-induced) widened post-newSession rejection failspackages/core/src/agents/runtime/agent-core.ts:2019 — [review] Critical [fails-closed] [new-surface] A sibling’s execution voids the model deadline bounding an adocs/design/background-agent-runtime-generations.md:1 — [review] This PR — which #8586's thread records as *"Layer 3 is nowdocs/design/background-agent-runtime-generations.md:13 — [review] The design doc this PR ships states the opposite of what tpackages/acp-bridge/src/bridge.test.ts:29181 — [probe] This added rationale states the opposite of the shipped adpackages/acp-bridge/src/bridge.ts:4639 — [review] ensureChannel still runs cancelIdleTimer() before thispackages/acp-bridge/src/bridge.ts:4642 — [review] A recycle refused by the two-generation cap is never re-atpackages/acp-bridge/src/bridge.ts:9763 — [review] requestRuntimeRecycle is added to the exported AcpSessipackages/acp-bridge/src/bridge.ts:14664 — [review] Switching the three workspace-scoped routes to admissiblepackages/acp-bridge/src/bridgeTypes.ts:2531 — [review] The replaced doc for isChannelLive() now promises admisspackages/cli/src/acp-integration/session/Session.ts:9876 — [review] The record-only terminal notification is routed into the spackages/cli/src/acp-integration/session/Session.ts:10091 — [review] Nothing in the suite covers the record-only notification ppackages/cli/src/nonInteractiveCli.ts:2802 — [review] Nothing in the suite pins the headless half of the record-packages/cli/src/serve/acp-http/dispatch.ts:918 — [review] The new retryable 503 carries no retryAfterSeconds and tpackages/cli/src/serve/server/error-response.ts:383 — [probe] The new fence is routed into the daemon's bridge-error metpackages/cli/src/ui/hooks/use-llm-stream.ts:6314 — [review] The interactive TUI's background-agent notification callbapackages/core/src/agents/background-agent-resume.ts:1315 — [review] The resume path's new watchdog integration — the retainsPpackages/core/src/agents/background-tasks.ts:1497 — [review] Making hasRunningTasks() count a watchdog-terminal entrypackages/core/src/agents/background-tasks.ts:1733 — [review] The <remaining> / <all-terminal> count inside emitNot- …and 15 more (see the run report)
Convergence: round 7 posted 6 inline comment(s), 1 of them reported for the first time; the previous round posted 6 (5 new). Findings keep coming back to the same files: packages/acp-bridge/src/bridge.ts (findings in rounds 1, 6; 1 more now). A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. (Observation only — nothing was withheld from this review because of this observation.)
Mechanism health: this round did not close cleanly, so it withholds the incremental anchor — and the round it recovered had no anchor this round could use either — none at all, one with no certifier, one certified by an identity other than the one this round runs under, or one this round's fetch refused or resolved to the head — so the next review re-reads the whole diff unless recovery grafts an earlier own anchor that the round running it can use onto the complete work list this round leaves behind, and keeps doing so until a round's marker carries an anchor again or a graft lands that the round running it can use. (Stated, not acted on — this changes nothing about what the round posts.)
中文说明
仅完成部分审查，审查缺口已披露。
本轮确认的 3 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未决，请确认：共 1 条（原文未翻译，列表见上方英文部分）。
未审查（原文为英文）：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally (Agent 7 ran per-workspace vitest only).
未审查（原文为英文）：test-efficacy — all three probes returned inconclusive (runner produced no parseable JSON), so no coverage-gap claim was measured either way and the positive control never ran (harnessValidated: null).
未审查（原文为英文）：reverse audit — stopped at the 5-round cap without two consecutive dry rounds; rounds 3, 4 and 5 all reported findings (chunks 5 and 6 retired on two-dry certificates).
未探索到全部深度（达到工具调用预算）："agent reverse-audit (round 4)"：whether updateOutput is actually wired for a depth-2 nested background agent — i.e. whether the task_execution chunk that carries the new park flags reaches…；"agent reverse-audit (round 4)"：none — I also examined, and deliberately did not file, the unguarded sessionRuntimeRecycle ext-method call in the finally when this.closing / this.disposed…；"agent reverse-audit (round 4)"：did not finish tracing whether a workflow-dispatched agent can reach runBackgroundTurn (packages/core/src/tools/agent/agent.ts:3861-3866), i.e. whether the ne…。
收敛姿态下延后（第 7 轮，非阻断）——已记录，本轮不要求修改；其中 3 条 Critical 按其失败方向与对照基线延后——fails-closed 且 new-surface：未认证任何错误结果，且 merge base 既无该功能面也无该缺陷——作为后续工作记录在 findings 工件中：共 35 条（原文未翻译，列表见上方英文部分）。
收敛情况：第 7 轮发布了 6 条行内评论，其中 1 条是首次提出；上一轮发布了 6 条（其中 5 条首次提出）。发现反复回到同一批文件：packages/acp-bridge/src/bridge.ts（第 1、6 轮已出过发现，本轮又有 1 条）。一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。（仅为观察——本轮评审未因此扣留任何内容。）
机制健康：本轮未能干净收尾，因而扣留了增量锚点，而它恢复到的那一轮也没有留下本轮可用的锚点——要么完全没有、要么没有认证者、要么由本轮运行身份之外的身份认证、要么被本轮的获取拒绝或解析为头提交——因此下一次评审将重读整个 diff，除非恢复流程把本轮能使用的更早自有锚点嫁接到本轮留下的完整工作清单上；并会一直如此，直到某一轮的标记重新带上锚点，或落地的嫁接能被运行该轮的评审使用。（仅陈述，不据此行动——这不改变本轮发布的任何内容。）
— qwen3.8-max via Qwen Code /review (v0.23.3)
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Agent-assisted review at 8b23578fc905b0516c2818bf3916d79059632d30 — Partial review — coverage gaps. Four confirmed correctness findings (three historical, one newly identified below). Comment only; no approval implied.
Critical findings
- The generation-cap rollback re-admits the runtime just judged unresponsive.
packages/acp-bridge/src/bridge.ts:3796-3824marks the session owner draining, but onBridgeRuntimeRecyclingErrorrestoresowner.state = 'active'and clears retirement. Recycle A with attached sessions to create B; then an abort-ignoring agent on B requests recycle. Both generations count toward the cap (:4642-4654), so the catch reactivates B. The next fresh session is sent straight back to B (:4639-4640,:6312-6313), sharing the unresponsive execution and retained slots. The new bridge test explicitly asserts this fallback, rather than detecting it. It contradicts the design's requirement not to route fresh work back to the condemned generation. Preserve fail-closed admission when replacement capacity is exhausted, or introduce an explicit recovery decision based on actual health; do not treat failure to allocate a replacement as evidence that its owner is safe. This is distinct from the earlier post-newSession guard fix. - R1-36 still stands: workspace MCP mutations miss live owner generations.
bridge.ts:14780-14883targets onlyadmissibleChannelInfo(). After A drains and B replaces it, removal executes on B and can returnnot_present/HTTP 200 while A's sessions retain the server; addition broadcasts success to workspace subscribers although A never received it. Verified real consumers:packages/cli/src/serve/routes/workspace-mcp-control.ts:552-645resolves the selected trusted workspace and calls this bridge;acp-integration/acpAgent.ts:12649-12720fans out only within the receiving child. Session-scoped twins (bridge.ts:14886-14904) retain owner routing. Choosing a newer generation does not implement workspace-wide mutation. Coordinate all work-owning generations with defined partial-failure semantics, or reject the mutation while that contract cannot be met. - R6-3 still stands: nested retry delays disappear before the parent watchdog.
packages/core/src/tools/agent/agent.ts:1486-1495forwards nestedMODEL_RETRYas an empty, throttled display update; the actual foreground listener is installed at:3137-3144.agents/runtime/agent-core.ts:2009-2026converts it toTOOL_PROGRESSwithout a delay, soagent-progress-watchdog.ts:125-140,198-201grants only ten minutes. A nested call honoring a one-hour Retry-After is killed while the same direct call receives the explicit extension (agent-core.ts:1025-1041;utils/retry.ts:496-501,528-539). Preserve the expected backoff through the nesting boundary with the existing bounded-extension policy, rather than treating it as generic activity. - R6-5 still stands: cancel-first bypasses physical-slot protection and recycle.
agent-progress-watchdog.ts:67-78never arms escalation if another caller already aborted the signal. A user cancellation does exactly that (background-tasks.ts:967-1013); its five-second fallback emits the notification and drains queued launches (:1068-1080). An abort-ignoring run then stops counting at:1326-1333, with no retained slot and no later watchdog recycle.failUnresponsivealso refuses an already-notified entry (:920-928). The added registry test calls escalation directly before cancellation finalizes, so it does not cover this real ordering. Keep physical execution ownership until settlement on the cancel-first path as well; a synthetic terminal notification must not free a still-executing slot.
Previous-review reassessment
- Our prior clock-drift Critical is fixed: the retry extension is captured by
() => armModel(retryDelayMs)at watchdog:122; the new test exercises the re-arm. - R6-4's post-newSession admission defect is fixed at bridge:5701-5702. This does not certify all abandoned-session cleanup (see gaps).
- Fixed in the inspected mechanisms: R1-2 optional notification metadata; R1-3 ordinary nested activity forwarding (not retry semantics); R1-4 indefinite retry suspension/overflow now has a bounded timer; R1-23 per-tool settlement is emitted before whole-batch completion; R1-25 original retained-slot gate and its blocking-label mirror; R1-24/R1-33 direct notification emission now uses queues; R5-1 display rejection is caught and end-turn helper catches transport failure (
Session.ts:10281-10293,10768-10781); R4-6 soft timeout no longer itself marks a generation draining (bridge.ts:3781-3793,3829-3836). R3-1's overbroad ROUND_END suspension now requires the explicit wait marker, not merely a running Monitor. - R6-1 approval timeout and R6-2 whole-unwind five-second grace remain as implemented, not fixed. The documented bounded approval behavior conflicts with older review guidance/test-plan text; terminal-record ownership on late cleanup also remains a maintainer decision. R1-29's cancel-versus-watchdog terminal semantics likewise must not be called resolved merely because a later test chooses the opposite policy. Existing escalation/deferral discussions remain open; I am not inventing another timeout policy here.
Coverage and limits
Read the complete current 28-file diff, new tests and paired design docs; inspected watchdog state machine, registry/cancel/slot consumers, event producers, notification queues, recycle handler and bridge admission/MCP owner routing. The private recycle route is live-session-owner scoped (BridgeClient validates its channel's sessionIds, existing entry and reason); its channel factory retains the bound workspace/environment. MCP add/remove and agent generation are selected-runtime/workspace operations, not process-global or primary-runtime fallback operations. HTTP/RPC recycling error mappings remain 503/retryable. Approximately 835 non-comment source lines changed (970 non-test source lines including comments); verified admin/maintainer author, so no external-refactor policy block.
Coverage gaps: this is not exhaustive validation of the large bridge's restore/branch/abandoned-child cleanup, all workspace-control/liveness consumers, hook unwinds, multi-level parked-state propagation, or compression/provider-internal retries. Historical R1-1/R1-5/R1-17, R3-3, R4-2/3 and terminal-hook R4 variants, R5-2/3, and R6-6 are cannot tell comprehensively in this pass, not cleared; repeated IDs refer to their described mechanisms/anchors. Other old resolved flags were not treated as proof. No repository tests/build, daemon/provider execution, or timing experiments ran for this PR. Existing Suggestions remain deferred; no new Suggestions. A posted partial review is not full review completion.
An approval-pending background agent was still charged the 15-minute model deadline, so a user who parked an approval for longer than that got a false watchdog failure. Issue #8586's acceptance criteria forbid approval waits from causing watchdog failures, so exempt the approval state from the model deadline just like external-input waits. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Patrol-Run: qwen-pr-closeout/jmtwznj86xz
|
Qwen Code review timed out. Qwen review timed out after 21600 seconds (of the 360-minute budget). This run already used the maximum 360 minute timeout. See workflow logs. |
Only clear retireWhenSessionsDrain when this recycle itself set it; a rollback must not erase a reap-after-drain condemnation a different condemnor (timeout, MCP discovery/auth) had already flagged on the channel. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Patrol-Run: qwen-pr-closeout/jmtxl34z1yy
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.
22 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- recordOnly terminal notification dropped in the interactive TUI (packages/cli/src/ui/hooks/use-llm-stream.ts:6314) — already reported as R1-9 (comment 3950282815)
- isChannelLive() doc describes admission while the implementation reports liveness (packages/acp-bridge/src/bridgeTypes.ts:2532) — already reported (round-3 deferral record)
- the recycle's replacement generation is never handed to the channel idle-reap policy (packages/acp-bridge/src/bridge.ts:3816) — already reported as R3-10 (round-3 review 5142185428)
- the public AcpSessionBridge.requestRuntimeRecycle member has no caller and the shipped child-to-daemon route is untested (packages/acp-bridge/src/bridgeTypes.ts:2549) — already reported as R1-11 (comment 3950282825)
- the daemon's record-only path has no test (packages/cli/src/acp-integration/session/Session.ts:10298) — already reported in the missing-test aggregate (round-3 review 5142185428)
- neither new runtime_recycling 503 mapping is tested (packages/cli/src/serve/acp-http/dispatch.ts:918) — already reported in the missing-test aggregate (round-3 review 5142185428)
- BridgeRuntimeRecyclingError missing from the daemon metrics known-error allowlist (packages/cli/src/serve/server/error-response.ts:383) — already reported as R1-10 (comment 3950282821), author confirmed still standing at head
- the escalation callback onUnresponsive is asserted nowhere (packages/core/src/agents/runtime/agent-progress-watchdog.test.ts:65) — already reported in the missing-test aggregate (round-3 review 5142185428)
- the new settled flag is never emitted by any test (packages/core/src/agents/runtime/agent-progress-watchdog.test.ts:48) — already reported in the missing-test aggregate (round-3 review 5142185428)
- the agent-headless progress-timeout mapping has no test (packages/core/src/agents/runtime/agent-headless.ts:402) — already reported in the missing-test aggregate over the abort-reason TIMEOUT mappings (round-3 review 5142185428)
- agent-core's producer-side watchdog emissions are untested (packages/core/src/agents/runtime/agent-core.ts:1399) — already reported in the missing-test aggregate (round-3 review 5142185428)
- releaseRetainedPhysicalSlot and the retained-slot accounting have no test (packages/core/src/agents/background-tasks.ts:943) — already reported in the missing-test aggregate (round-3 review 5142185428)
- the headless recordOnly consumer is untested (packages/cli/src/nonInteractiveCli.ts:2800) — already reported in the missing-test aggregate over recordOnly routing (round-3 review 5142185428)
- the admissibleChannelInfo() swap and per-generation identity fixes are untested for two coexisting generations (packages/acp-bridge/src/bridge.ts:14673) — already reported in the missing-test aggregate over the two-generation admission cap …
- describeBlockingBackgroundWork's widened predicate and 'still stopping' label have no test (packages/cli/src/ui/utils/backgroundWorkUtils.ts:108) — already reported in the missing-test aggregate over retainsPhysicalSlot accounting (round-3 …
- no case runs an executing tool past MODEL_CONTROL_PROGRESS_TIMEOUT_MS (packages/core/src/agents/runtime/agent-progress-watchdog.test.ts:74) — already reported in the missing-test aggregate over the watchdog module (round-3 review 5142185428…
- the retainsPhysicalSlot early exits in agent.ts and background-agent-resume.ts have no test (packages/core/src/tools/agent/agent.ts:3725) — already reported in the missing-test aggregate (round-3 review 5142185428)
- the suite never emits START/STREAM_TEXT/USAGE_METADATA so onActivity is unexercised (packages/core/src/agents/runtime/agent-progress-watchdog.test.ts:30) — already reported in the missing-test aggregate over the watchdog module (round-3 rev…
- armTool's drift re-arm closure is unpinned (packages/core/src/agents/runtime/agent-progress-watchdog.ts:141) — already reported in the missing-test aggregate over the watchdog module (round-3 review 5142185428)
- the new retryable 503 carries no Retry-After / retryAfterSeconds backoff hint (packages/cli/src/serve/server/error-response.ts:354) — already reported (round-3 deferral record)
- …and 2 more (see the run report)
Not reviewed: reverse audit — reached the 5-round cap for a large diff without two consecutive dry rounds; round 5 still reported findings, so what it surfaced is verified but the loop never converged.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally; Test (macos-latest, Node 22.x) and Test (windows-latest, Node 22.x) were also skipped and the same unit suites ran on Linux, so only the Linux unit ground is covered.
Not explored to full depth (tool budget reached): "agent reverse-audit (round 2)": whether registerOwnedMonitorNotifications 's wake fires when a Monitor task reaches a terminal status without delivering a message — the orphaned-waiter prem…; "agent reverse-audit (round 3)": whether the modified fake's exited resolving with undefined (instead of a real {exitCode, signalCode} ) drives the channel.exited handler down a differen….
Deferred under the convergence posture (round 8, not a blocker) — recorded, not requested in this round; 2 Critical(s) among them are deferred by their axes — fails-closed on new surface, where no wrong result is certified and the merge base had neither the surface nor the defect — and remain follow-up work recorded in the findings artifact:
packages/core/src/agents/runtime/agent-progress-watchdog.ts:154 — [review] Critical [fails-closed] [new-surface] The external-input latch is cleared only by ROUND_START …packages/core/src/agents/runtime/agent-core.ts:2104 — [review] Critical [fails-closed] [new-surface] The new TOOL_PROGRESS emit is placed *above* the…packages/acp-bridge/src/bridge.ts:3805 — [review] A draining generation is an OS-live ACP child that still…packages/acp-bridge/src/bridge.ts:3805 — [review] A recycle that lands while a spawn is inside…packages/acp-bridge/src/bridge.ts:3827 — [review] The rollback's clearing of retireWhenSessionsDrain is…packages/acp-bridge/src/bridge.ts:1098 — [review] Inserting state between the load-bearing JSDoc block and…packages/core/src/agents/runtime/agent-progress-watchdog.ts:106 — [review] ToolDeadline.parkedOnInput is a dead switch — its only…docs/design/background-agent-runtime-generations.md:13 — [review] Both language versions state the only recovery from the…packages/acp-bridge/src/bridge.ts:14673 — [review] generateWorkspaceAgent now reports a *draining* runtime…packages/core/src/agents/runtime/agent-headless.ts:402 — [review] The new catch arm classifies *any* exception as a progress…packages/core/src/agents/runtime/agent-progress-watchdog.ts:112 — [review] The model deadline armModel actually enforces is…packages/acp-bridge/src/bridge.test.ts:32729 — [review] The new test pins the drain-race refusal to…packages/cli/src/ui/utils/backgroundWorkUtils.ts:108 — [review] The — still stopping qualifier is appended to the label…packages/core/src/agents/runtime/agent-progress-watchdog.test.ts:181 — [review] The clock-drift branch in agent-progress-watchdog.ts:86 …packages/acp-bridge/src/bridge.ts:4661 — [review] The two-generation cap counts the still-draining…packages/core/src/agents/background-tasks.ts:930 — [review] The watchdog-escalation terminal transition persists a…packages/core/src/agents/runtime/agent-core.ts:2188 — [review] The settled transition is emitted unconditionally for…packages/core/src/tools/agent/agent.ts:1589 — [review] waitingForApproval() is derived from currentToolCalls …packages/acp-bridge/src/bridge.test.ts:29181 — [review] This new comment states an admission invariant the code…packages/core/src/agents/background-tasks.ts:1497 — [review] The widened hasRunningTasks() predicate has a second…
Convergence: round 8 posted 6 inline comment(s), 2 of them reported for the first time; the previous round posted 6 (1 new). Findings keep coming back to the same files: packages/acp-bridge/src/bridge.ts (findings in rounds 1, 7; 1 more now). The rate of new findings is not falling. A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. Batching the remaining fixes and verifying them before the next push keeps the loop from re-deriving the same set; this PR's reviews already resolve to a critical posting floor. (Observation only — nothing was withheld from this review because of this observation.)
Mechanism health: this round did not close cleanly, so it withholds the incremental anchor — and the round it recovered had no anchor this round could use either — none at all, one with no certifier, one certified by an identity other than the one this round runs under, or one this round's fetch refused or resolved to the head — so the next review re-reads the whole diff unless recovery grafts an earlier own anchor that the round running it can use onto the complete work list this round leaves behind, and keeps doing so until a round's marker carries an anchor again or a graft lands that the round running it can use. (Stated, not acted on — this changes nothing about what the round posts.)
Residual risk: this loop is persistently critical — Criticals stood in the previous round's work-list and stand again this round (6 Critical(s)), the rate of first-time findings is not falling (this round 2, previous 1), and the standing Critical backlog is not shrinking. The severity floor will not converge it. Recommendation: land-with-residual-risk — the exit is a maintainer risk-acceptance decision (merge, carrying the residual risk), not another review round. Residual-risk inventory for that decision (maintainer to complete):
| standing Critical | attack surface | attacker-dependency | blast radius |
|---|---|---|---|
| (each standing Critical) | … | … | … |
Advisory only — it does not block this review.
中文说明
仅完成部分审查，审查缺口已披露。
本轮确认的 22 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未审查（原文为英文）：reverse audit — reached the 5-round cap for a large diff without two consecutive dry rounds; round 5 still reported findings, so what it surfaced is verified but the loop never converged.
未审查（原文为英文）：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally; Test (macos-latest, Node 22.x) and Test (windows-latest, Node 22.x) were also skipped and the same unit suites ran on Linux, so only the Linux unit ground is covered.
未探索到全部深度（达到工具调用预算）："agent reverse-audit (round 2)"：whether registerOwnedMonitorNotifications 's wake fires when a Monitor task reaches a terminal status without delivering a message — the orphaned-waiter prem…；"agent reverse-audit (round 3)"：whether the modified fake's exited resolving with undefined (instead of a real {exitCode, signalCode} ) drives the channel.exited handler down a differen…。
收敛姿态下延后（第 8 轮，非阻断）——已记录，本轮不要求修改；其中 2 条 Critical 按其失败方向与对照基线延后——fails-closed 且 new-surface：未认证任何错误结果，且 merge base 既无该功能面也无该缺陷——作为后续工作记录在 findings 工件中：共 20 条（原文未翻译，列表见上方英文部分）。
收敛情况：第 8 轮发布了 6 条行内评论，其中 2 条是首次提出；上一轮发布了 6 条（其中 1 条首次提出）。发现反复回到同一批文件：packages/acp-bridge/src/bridge.ts（第 1、7 轮已出过发现，本轮又有 1 条）。新发现的产出速度没有下降。一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。把剩余修复攒成一批、验证后再推送，可以避免循环反复推导同一组发现；本 PR 的评审已解析为 critical 发布下限。（仅为观察——本轮评审未因此扣留任何内容。）
机制健康：本轮未能干净收尾，因而扣留了增量锚点，而它恢复到的那一轮也没有留下本轮可用的锚点——要么完全没有、要么没有认证者、要么由本轮运行身份之外的身份认证、要么被本轮的获取拒绝或解析为头提交——因此下一次评审将重读整个 diff，除非恢复流程把本轮能使用的更早自有锚点嫁接到本轮留下的完整工作清单上；并会一直如此，直到某一轮的标记重新带上锚点，或落地的嫁接能被运行该轮的评审使用。（仅陈述，不据此行动——这不改变本轮发布的任何内容。）
残余风险：本循环处于 persistently-critical 形态——上一轮工作清单中的 Critical 本轮依然存在（本轮 6 条 Critical），首次发现的速率没有下降（本轮 2，上一轮 1），且未决 Critical 积压没有减少。severity floor 无法使其收敛。建议：land-with-residual-risk——出口是 maintainer 的风险接受决定（合入并承担残余风险），而非再开一轮评审。供该决定使用的残余风险清单（maintainer 填写）：按每条未决 Critical 列出「攻击面 · 攻击者依赖性 · 影响范围」三栏。仅为建议——不阻断本次评审。
— qwen3.8-max via Qwen Code /review (v0.23.3)
…tores on drained generations Two round-8 review findings: - acp-bridge: `loadSession`/`unstable_resumeSession` only re-tested `ci.isDying` after the restore round-trip, so a runtime recycle landing mid-restore installed a brand-new session on the condemned generation. The recycle leaves the channel `draining` with `isDying === false`, because retirement is deferred while that very restore is in flight (`hasNoSessionWork` counts `pendingRestoreCount`), so the guard passed and `createSessionEntry` pinned the drained generation open until the new session closed. Mirror the post-`newSession` twin's three-state test: `ci.state !== 'active'`. - core: `failUnresponsive` bailed out on `entry.notified`, so when the cancel-grace timer (`CANCEL_GRACE_MS`) finalized the cancellation — emitting the terminal notification and setting `notified` — before the drift-guarded watchdog escalation landed, the escalation was dropped entirely and `retainsPhysicalSlot` was never set, even though `onUnresponsive` can only fire while the execution is provably still alive. Reaching the escalation is itself proof that the execution still holds a physical slot, so settle the entry and retain the slot regardless of the already-delivered notification. `emitNotification` stays idempotent, so the terminal notification is not re-fired. Regression tests sit beside the existing twins and both fail when their guard is reverted: packages/core/src/agents/background-tasks.test.ts and packages/acp-bridge/src/bridge.test.ts. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Patrol-Run: qwen-pr-closeout/jmtyno0zh0n
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Patrol-Run: qwen-pr-closeout/jmtypt6si0q
|
Integrity audit of the resolved Verified this round, highest-severity first — all four judge the cited hazard obsolete at this head, so all stay resolved:
Nothing was re-opened. Two notes for the reviewer: the per-call Remaining 32 silent-resolved Criticals: "resolve justified, not audited this round". No code was changed. |
|
Qwen Code review did not complete successfully. The review pipeline failed before a review could be posted. A transient error is retried automatically; if you are seeing this, retry with |
Closeout round
|
| Finding | Anchor verified at 93eef769a7 |
Gate hit |
|---|---|---|
| R6-2 escalation grace vs. cooperative unwind | agent-progress-watchdog.ts:19, abort() :67-78, only clear at :239 inside the detach closure that runs at agent.ts:3915 — same .finally as both releaseRetainedPhysicalSlot call sites (agent.ts:3916, background-agent-resume.ts:1437) |
concurrency / settle semantics |
| R6-3 retry extension cannot cross nesting | agent.ts:1477-1485 (forwardProgress is zero-arity, so the delay is dropped by signature before the 1/s throttle), agent-events.ts:181-194, agent-progress-watchdog.ts:128 + drift re-arm :142 |
new cross-layer event field |
| R6-6a compression retry invisible | llm-chat.ts:564 vs TryCompressOptions :588; call sites :3102/:3965; chatCompressionService.ts:844/:884; baseLlmClient.ts:78/:307/:462 |
new callback on exported GenerateTextOptions |
R6-6b retainsPhysicalSlot unbounded |
background-tasks.ts:943, :1512, :1890, cancel() bail :984; PR's own invariant at :920-934 |
semantics + data-loss edge |
| R1-36 workspace MCP routes during drain | bridge.ts:6318-6322, :14699/:14821/:14877 vs session twins :14927/:14936 |
routing contract + public API |
Two of these produced new evidence this round that changes the shape of the decision:
- R6-6b: the suggested remediation (an unref'd fallback timer in
failUnresponsivecallingreleaseRetainedPhysicalSlot) contradicts this PR's own documented invariant atbackground-tasks.ts:920-934— "the physical slot must still be retained … otherwisegetRunningBackgroundCountandhasRunningTasks()free a concurrency slot that is still occupied, and/clear,/resume,/branchand session switches all proceed over live work." A delayed auto-release does exactly that once it fires, andreset()clears the map without abortingentry.abortController, so a still-running orphan loses its only owner. The stuck gate is real (cancel()bails at:984on thefailedstatusfailUnresponsiveset at:940;pruneTerminalEntries()exempts retained entries at:1890) — but both exits cost something, so the choice is not mechanical. - R1-36: the cheaper "refuse when
liveChannelInfo() !== admissibleChannelInfo()" remediation is provably unreachable in the reported scenario, not merely ineffective. Both predicates read the same singlechannelInfobinding (:6318-6319and:6321-6322), so they can only disagree whenchannelInfois non-dying and notactive— draining with no replacement spawned. After a recycle repointschannelInfoat the replacement both return the reference-identical object. A fan-out is feasible (aliveChannelsis iterated at:2821, membership-tested at:4035); what blocks it is the partial-success contract on the workspace HTTP result and the conditionalmcp_server_addedbroadcast.
Decisions requested
- R6-2 — bounded total grace covering the cooperative unwind (keeping the hard force-fail), or let a late cooperative settle re-publish its real payload (incl. the worktree suffix) and skip the recycle? The second crosses
agent.ts+background-tasks.ts+Session.tsand inverts terminal-outcome precedence. - R6-3 — should a nested run's provider-directed backoff extend the parent's tool deadline at all? If yes,
retryDelayMsgoes onAgentToolProgressEventwith a latch/clear rule and theMAX_RETRY_DEADLINE_EXTENSION_MSclamp re-passed through the drift re-arm closure. - R6-6a — may a compression side query's backoff extend a background agent's model deadline, or must compression stay
maxAttempts: 1and fail to NOOP fast?chatCompressionService.ts:749-751argues the latter; this PR's design doc argues the former. - R6-6b — bounded automatic release (contradicts
:920-934, can free a slot a live orphan still occupies), or user-initiated force-release incancel()plus dropping the matching refusal intask-stop.ts? - R1-36 — fan out to every non-dying generation and define partial success, or reject workspace-scoped runtime-MCP mutations whenever a work-owning generation is not the active one?
Resolution-integrity audit — 32 flagged, 0 re-opened
32 resolved threads whose first comment is [Critical] end with the bot's own finding and carry no author reply. Grouped by finding id that is 20 ids, and every one of them was minted on or before 2026-09-09T11:37:45Z. The bot's two later full-diff scans (2026-09-11T09:32, 2026-09-12T04:52) re-minted only R1-36, R6-2, R6-3 and R6-6, each with explicit "Still stands at <sha>" wording — i.e. the reviewer demonstrably re-mints anything that survives a scan. Non-re-mint across two later scans is therefore the retirement evidence, and none of the 20 is a silent resolve of a still-live Critical. The resolved threads that do come from those two later rounds all carry author replies with fix SHAs (1487c0d5b6 on background-tasks.ts and bridge.ts:3808; 90e2d6f7ae on retireWhenSessionsDrain).
CI — clean, nothing PR-caused to fix
gh pr checks 11270 at this head: 22 pass / 26 skipping / 1 pending. All required suites pass — Test (ubuntu-latest, Node 22.x) 22m23s, Lint & Static 12m38s, Integration Tests (no-AK, No Sandbox) 8m49s, Serve A/B 21m30s, web-shell E2E Smoke 10m13s, Real daemon E2E, TUI parity snapshots, OpenTUI no-flicker gate, Desktop Shell (ubuntu + windows). The single pending job is review-pr, an informational review bot that does not gate. No DFSAMPLE load failures and no checkout-verification race in any log, so no infra-vs-PR attribution was needed. The skipping rows are conditional jobs (fork-PR signalling, windows/macos test shards, takeover/retry command handlers) that this run did not select.
Why /triage was not re-triggered
The standing CHANGES_REQUESTED is qwen-code-ci-bot's (2026-09-12T04:52:05Z), which is normally the cue for a standalone @qwen-code /triage. It was deliberately skipped: the head is unchanged since that scan, so a re-run would re-mint the same four findings rather than re-judge anything, and the CR is not stale — the four Criticals are live and unresolved. It clears on a code change answering one of the five decisions above, or on a maintainer override.
No push this round; head remains 93eef769a7.
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.
22 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- isChannelLive() doc describes admission while the implementation reports liveness (packages/acp-bridge/src/bridgeTypes.ts:2531) — already reported (round-3 deferral record)
- recordOnly terminal notification dropped in the interactive TUI (packages/cli/src/ui/hooks/use-llm-stream.ts:6314) — already reported as R1-9 (comment 3950282815)
- BridgeRuntimeRecyclingError missing from the daemon metrics known-error allowlist (packages/acp-bridge/src/bridgeErrors.ts:676) — already reported as R1-10 (comment 3950282821)
- the public AcpSessionBridge.requestRuntimeRecycle member has no production caller (packages/acp-bridge/src/bridge.ts:9783) — already reported as R1-11 (comment 3950282825)
- the shipped child-to-daemon sessionRuntimeRecycle ext-method validation is untested (packages/acp-bridge/src/bridgeClient.ts:1340) — already reported as R1-11 (comment 3950282825)
- wasReapPending is captured before two awaits so the rollback can erase an independent condemnation (packages/acp-bridge/src/bridge.ts:3813) — already reported (round-8 deferral, bridge.ts:3827)
- isDying is declared a plain boolean but implemented as a write-only-true alias over state (packages/acp-bridge/src/bridge.ts:1098) — already reported (round-8 deferral, bridge.ts:1098)
- the recycle's replacement generation is never handed to the channel idle-reap policy (packages/acp-bridge/src/bridge.ts:3816) — already reported as R3-10 (round-3 review 5142185428)
- settled:true TOOL_PROGRESS is re-emitted for every terminal call on every scheduler notify (packages/core/src/agents/runtime/agent-core.ts:2188) — already reported (round-8 deferral, agent-core.ts:2188)
- ToolDeadline.parkedOnInput is a dead switch (packages/core/src/agents/runtime/agent-progress-watchdog.ts:106) — already reported (round-8 deferral)
- the escalation callback onUnresponsive is asserted nowhere (packages/core/src/agents/runtime/agent-progress-watchdog.test.ts:66) — already reported in the missing-test aggregate (round-3 review 5142185428)
- the suite never emits START/STREAM_TEXT/USAGE_METADATA so onActivity is unexercised (packages/core/src/agents/runtime/agent-progress-watchdog.test.ts:25) — already reported in the missing-test aggregate (round-3 review 5142185428)
- agent-core's producer-side watchdog emissions are untested (packages/core/src/agents/runtime/agent-core.ts:2104) — already reported in the missing-test aggregate (round-3 review 5142185428)
- the daemon's record-only drain branch has no test (packages/cli/src/acp-integration/session/Session.ts:10337) — already reported in the missing-test aggregate (round-3 review 5142185428)
- releaseRetainedPhysicalSlot and the retained-slot accounting have no test (packages/core/src/agents/background-tasks.ts:1348) — already reported in the missing-test aggregate (round-3 review 5142185428)
- the resume-path retainsPhysicalSlot early exits have no test (packages/core/src/agents/background-agent-resume.ts:1300) — already reported in the missing-test aggregate (round-3 review 5142185428)
- neither new runtime_recycling 503 mapping is tested (packages/cli/src/serve/acp-http/dispatch.ts:918) — already reported in the missing-test aggregate (round-3 review 5142185428)
- describeBlockingBackgroundWork's widened predicate and 'still stopping' label have no test (packages/cli/src/ui/utils/backgroundWorkUtils.ts:101) — already reported in the missing-test aggregate (round-3 review 5142185428)
- all agent.ts watchdog wiring is untested (packages/core/src/tools/agent/agent.ts:3881) — already reported in the missing-test aggregate (round-3 review 5142185428)
- the new retryable 503 carries no Retry-After backoff hint and is absent from the error-taxonomy docs (packages/cli/src/serve/server/error-response.ts:383) — already reported (round-3 deferral record)
- …and 2 more (see the run report)
Unresolved, please confirm:
- [Critical] R6-1 (agent-progress-watchdog approval timeout) — @doudouOUC's review 5177234081 records it as still implemented and not fixed, while round 8's machine ledger does not carry it; this round did not re-read that mechanism, so it cannot be rul…
- [Critical] R1-29 (cancel-versus-watchdog terminal semantics) — @doudouOUC's review 5177234081 states it must not be called resolved merely because a later test chooses the opposite policy, and round 8's ledger does not carry it; not re-read this round…
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally; Test (macos-latest, Node 22.x) and Test (windows-latest, Node 22.x) were also skipped and the same unit suites ran on Linux, so only the Linux unit ground is covered.
Not reviewed: reverse audit — stopped after round 4 of the plan's 5-round cap; rounds 3 and 4 both reported findings, so two consecutive dry rounds were unreachable and the final round was not run.
Not reviewed: test-efficacy probe — Agent 7's mutation probe came back inconclusive because its scratch tree could not resolve the workspace packages' dist/, so no coverage gap was measured in either direction.
Not reviewed: reverse-audit rounds 3 and 4 Suggestion-level candidates — the loop was stopped before they could be put through a verifier, so they are disclosed as unverified rather than confirmed.
Not reviewed: reverse-audit round 4 Critical at packages/acp-bridge/src/bridge.ts:8836 — reported but never put through a verifier; carried in the findings artifact at low confidence.
Not explored to full depth (tool budget reached): chunk 1: did not verify whether any SDK/client-side layer retries an errorKind: 'internal' frame (finding 1's impact hinges on it), and did not read the restore re-che…; "agent reverse-audit (round 3)": the filed finding was traced by reading only — I did not execute a probe of the two-generation recycle sequence, which is why it carries Confidence: low .; "agent reverse-audit (round 1)": did not verify the child-side cost of the orphaned restored session in finding 1 — whether the agent-owned writer lease it holds ( packages/cli/src/acp-integrat….
Deferred under the convergence posture (round 9, not a blocker) — recorded, not requested in this round; 3 Critical(s) among them are deferred by their axes — fails-closed on new surface, where no wrong result is certified and the merge base had neither the surface nor the defect — and remain follow-up work recorded in the findings artifact:
packages/core/src/tools/agent/agent.ts:3915 — [probe] Critical [fails-closed] [new-surface] R6-6: a retained physical slot has no terminal remedy once the watchdog has failed the entrypackages/acp-bridge/src/bridge.ts:5711 — [probe] Critical [fails-closed] [new-surface] a recycle-condemned spawn rejection is classified as a non-retryable internal error while this PR adds a retryable 503 siblingpackages/acp-bridge/src/bridge.ts:4648 — [probe] Critical [fails-closed] [new-surface] the recycle cancels the current primary's idle reaper and never re-arms itpackages/acp-bridge/src/bridge.ts:5711 — [probe] a spawn rejected by the draining re-check leaves the child-side session unsettledpackages/core/src/agents/runtime/agent-progress-watchdog.ts:114 — [probe] the six-hour retry-extension cap is untested and an above-cap delay is reachable by defaultpackages/core/src/agents/runtime/agent-progress-watchdog.ts:204 — [probe] nothing pins that a heartbeating executing tool is exempt from the model deadline
Convergence: round 9 posted 4 inline comment(s), 1 of them reported for the first time; the previous round posted 6 (2 new). Findings keep coming back to the same files: packages/core/src/agents/background-tasks.ts (findings in round 8; 1 more now). A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. (Observation only — nothing was withheld from this review because of this observation.)
Mechanism health: this round did not close cleanly, so it withholds the incremental anchor — and the round it recovered had no anchor this round could use either — none at all, one with no certifier, one certified by an identity other than the one this round runs under, or one this round's fetch refused or resolved to the head — so the next review re-reads the whole diff unless recovery grafts an earlier own anchor that the round running it can use onto the complete work list this round leaves behind, and keeps doing so until a round's marker carries an anchor again or a graft lands that the round running it can use. (Stated, not acted on — this changes nothing about what the round posts.)
中文说明
仅完成部分审查，审查缺口已披露。
本轮确认的 22 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未决，请确认：共 2 条（原文未翻译，列表见上方英文部分）。
未审查（原文为英文）：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally; Test (macos-latest, Node 22.x) and Test (windows-latest, Node 22.x) were also skipped and the same unit suites ran on Linux, so only the Linux unit ground is covered.
未审查（原文为英文）：reverse audit — stopped after round 4 of the plan's 5-round cap; rounds 3 and 4 both reported findings, so two consecutive dry rounds were unreachable and the final round was not run.
未审查（原文为英文）：test-efficacy probe — Agent 7's mutation probe came back inconclusive because its scratch tree could not resolve the workspace packages' dist/, so no coverage gap was measured in either direction.
未审查（原文为英文）：reverse-audit rounds 3 and 4 Suggestion-level candidates — the loop was stopped before they could be put through a verifier, so they are disclosed as unverified rather than confirmed.
未审查（原文为英文）：reverse-audit round 4 Critical at packages/acp-bridge/src/bridge.ts:8836 — reported but never put through a verifier; carried in the findings artifact at low confidence.
未探索到全部深度（达到工具调用预算）：chunk 1：did not verify whether any SDK/client-side layer retries an errorKind: 'internal' frame (finding 1's impact hinges on it), and did not read the restore re-che…；"agent reverse-audit (round 3)"：the filed finding was traced by reading only — I did not execute a probe of the two-generation recycle sequence, which is why it carries Confidence: low .；"agent reverse-audit (round 1)"：did not verify the child-side cost of the orphaned restored session in finding 1 — whether the agent-owned writer lease it holds ( packages/cli/src/acp-integrat…。
收敛姿态下延后（第 9 轮，非阻断）——已记录，本轮不要求修改；其中 3 条 Critical 按其失败方向与对照基线延后——fails-closed 且 new-surface：未认证任何错误结果，且 merge base 既无该功能面也无该缺陷——作为后续工作记录在 findings 工件中：共 6 条（原文未翻译，列表见上方英文部分）。
收敛情况：第 9 轮发布了 4 条行内评论，其中 1 条是首次提出；上一轮发布了 6 条（其中 2 条首次提出）。发现反复回到同一批文件：packages/core/src/agents/background-tasks.ts（第 8 轮已出过发现，本轮又有 1 条）。一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。（仅为观察——本轮评审未因此扣留任何内容。）
机制健康：本轮未能干净收尾，因而扣留了增量锚点，而它恢复到的那一轮也没有留下本轮可用的锚点——要么完全没有、要么没有认证者、要么由本轮运行身份之外的身份认证、要么被本轮的获取拒绝或解析为头提交——因此下一次评审将重读整个 diff，除非恢复流程把本轮能使用的更早自有锚点嫁接到本轮留下的完整工作清单上；并会一直如此，直到某一轮的标记重新带上锚点，或落地的嫁接能被运行该轮的评审使用。（仅陈述，不据此行动——这不改变本轮发布的任何内容。）
— qwen3.8-max via Qwen Code /review (v0.23.3)
| } | ||
| this.releaseFinishingWaiters(agentId, true); | ||
| this.rejectPendingApprovals(entry); | ||
| this.emitNotification(entry, true); |
There was a problem hiding this comment.
[Critical] R9-1: [certifies-falsely] [new-surface] The runtime recycle is hung off the record-only notification, so whenever the cancel-grace timer wins the race the owner generation is never condemned — while the cost half of the escalation still lands.
failUnresponsive is deliberately not gated on entry.notified, and it latches retainsPhysicalSlot = true before calling emitNotification(entry, true). But emitNotification still returns at if (entry.notified) return;, and the recycle has exactly one trigger in the whole repository: Session.ts's if (meta.recordOnly) branch calling #recordUnresponsiveAgentNotification, whose finally sends qwen/control/session/runtime/recycle. Suppress the notification and the recycle goes with it.
Both timers are 5 s, and the escalation is drift-guarded and re-arms while the cancel grace is a bare setTimeout, so one event-loop stall is enough for the cancel side to win — the comment above this method says exactly that. Any cancel({ notify: false }) inside the window does the same (/clear, a session switch, ACP session teardown). When it happens the owner generation stays state: 'active', so admissibleChannelInfo() keeps handing fresh sessions to the child the design doc calls unsafe for fresh work, and no replacement spawns; meanwhile retainsPhysicalSlot keeps hasRunningTasks() true, so /clear, /resume, /branch and session switches refuse permanently — the only release is the run body's .finally, which by hypothesis never runs. The design doc promises the opposite order ("recorded and displayed … then the trusted child-to-daemon route requests recycle"), and issue #8586's Layer 5 requires both halves.
The in-diff rationale argues "Only the notification is suppressed". That is true of the user-visible line and does not account for the recycle bolted to the same call.
Witness:
probe against the real registry at HEAD:
ARM B (cancel grace finalized first) {"newCallsFromEscalation":0,"recordOnlyCalls":0,"status":"failed","retainsPhysicalSlot":true,"hasRunningTasks":true}
ARM C (notified cleared, re-entered) the recordOnly call appears — so `if (entry.notified) return;` is the sole suppressor
grep sessionRuntimeRecycle packages/: 3 hits — the constant, the child-side handler, and the single call site in #recordUnresponsiveAgentNotification's finally
Do not hang the recycle off the notification: request it from failUnresponsive directly, or from a path that does not consult entry.notified, so the generation is condemned whenever the escalation lands. If it must stay behind the notification, let the record-only path still invoke the recycle callback when entry.notified is already set.
Any fix must keep the user-visible notification idempotent — the comment at background-tasks.ts:921-935 states that emitNotification is itself idempotent (if (entry.notified) return), "so the already-delivered terminal notification is never re-fired", and a fix must not re-deliver a duplicate terminal line while adding the recycle.
Please extend background-tasks.test.ts's "retains the physical slot when the cancel grace timer finalizes before the escalation" case to assert the recycle is still requested in that ordering; it must go red while emitNotification short-circuits on entry.notified.
中文说明
[Critical] R9-1：运行时回收（recycle）挂在 record-only 通知上，因此只要 cancel-grace 定时器赢得竞争，owner generation 就永远不会被判定回收——而升级处理的代价那一半却照常发生。
failUnresponsive 有意不以 entry.notified 为门槛，并在调用 emitNotification(entry, true) 之前置上 retainsPhysicalSlot = true。但 emitNotification 仍然会在 if (entry.notified) return; 处返回，而整个仓库中回收只有一个触发点：Session.ts 的 if (meta.recordOnly) 分支调用 #recordUnresponsiveAgentNotification，由其 finally 发出 qwen/control/session/runtime/recycle。通知被抑制，回收就一并被抑制。
两个定时器都是 5 秒，而升级定时器带漂移保护、会重新计时，cancel grace 却是裸 setTimeout，因此一次事件循环阻塞就足以让 cancel 一侧获胜——本方法上方的注释正是这么写的。窗口内任何 cancel({ notify: false }) 效果相同（/clear、切换会话、ACP 会话销毁）。一旦发生，owner generation 仍保持 state: 'active'，于是 admissibleChannelInfo() 会继续把新会话交给设计文档称为"不适合承接新工作"的那个子进程，且不会派生替代 generation；同时 retainsPhysicalSlot 让 hasRunningTasks() 恒为真，/clear、/resume、/branch 与会话切换将被永久拒绝——唯一的释放点是 run body 的 .finally，而按前提它永远不会执行。设计文档承诺的是相反的顺序（"先记录并展示……然后由可信的 child-to-daemon 路由请求回收"），issue #8586 的 Layer 5 也要求两半都成立。
diff 内的理由写的是"只有通知被抑制"。这对用户可见的那一行成立，但没有考虑到挂在同一次调用上的回收。
修复方向：不要把回收挂在通知上——直接从 failUnresponsive 请求回收，或从一个不读取 entry.notified 的路径请求，使升级一旦落地 generation 就被判定回收。若必须留在通知之后，则让 record-only 路径在 entry.notified 已置位时仍然调用回收回调。
修复约束：必须保持用户可见通知的幂等性——background-tasks.ts:921-935 的注释写明 emitNotification 自身是幂等的（if (entry.notified) return），"因此已送达的终态通知不会被再次触发"；补上回收的同时不得重复投递终态通知。
请补充测试：扩展 background-tasks.test.ts 中 "retains the physical slot when the cancel grace timer finalizes before the escalation" 用例，断言在该顺序下回收请求仍然发出；当 emitNotification 仍在 entry.notified 处短路时，该断言必须为红。
（证据见上方 Witness 代码块；witness 为程序输出，未翻译。）
— qwen3.8-max via Qwen Code /review (v0.23.3)
5 participants
What this PR does
Adds fixed progress watchdogs to ordinary background Agent turns, including fresh launches, restored runs, and resident continuations. Model/control work times out after 15 minutes without observable progress; each executing tool has its own 10-minute progress deadline. Tool output and silent-shell liveness updates renew only that tool's deadline.
A watchdog expiry cooperatively aborts the turn and is reported as
TIMEOUT, then persisted and notified once asfailed. It never enters the workflow retry loop. If the turn ignores that abort, the registry retains its physical slot while the daemon drains and replaces the owning Session runtime generation; existing Sessions stay pinned to their owner generation and fresh work uses the active replacement.Why it's needed
A background Agent can remain registered as running while its model, control flow, or a tool has stopped making progress. The existing workflow watchdog cannot be reused because it retries and deliberately suspends timing for every running tool. This leaves ordinary background Agents able to wedge indefinitely and keep their Session active without a terminal result.
Approval waits pause the affected tool deadline. A no-tool round waiting for Monitor-owned external input pauses the model deadline until input arrives. Queued tools do not start their deadline before execution, and a timer delayed by host suspend or a local event-loop gap is rearmed instead of being charged to the Agent.
Reviewer Test Plan
How to verify
TIMEOUTfinish, persistsfailed, and is not retried.Evidence (Before & After)
Before: an ordinary background Agent has no logical progress deadline; the workflow watchdog is retrying and treats all running tools as unbounded.
After: ordinary background turns have independent model/control and per-tool deadlines and settle cooperative stalls once as
TIMEOUT/failed.Tested on
Environment (optional)
Static diff review and formatting only. No local test, build, typecheck, or CI command was run.
Risk & Scope
Linked Issues
Part of #8586.
Depends on #11265.
中文说明
本 PR 做了什么
为普通后台 Agent 的每个 turn 增加固定进度 watchdog，覆盖首次启动、恢复运行和驻留 Agent 的后续继续。模型/控制流程连续 15 分钟没有可观察进度时超时；每个正在执行的工具各自拥有 10 分钟进度期限。工具输出和静默 shell 存活更新只续期对应工具的期限。
watchdog 到期后会协作式中止该 turn，并上报为
TIMEOUT，随后只持久化和通知一次failed。它不会进入 workflow 的重试循环。如果该 turn 忽略中止，registry 会保留其物理槽位，同时 daemon 排空并替换该 Session 所属的 runtime generation；已有 Session 继续固定在原 owner generation，新任务使用 active replacement。为什么需要
后台 Agent 的模型、控制流程或某个工具停止推进时，registry 仍可能一直把它记录为 running。现有 workflow watchdog 不能直接复用，因为它会重试，而且会对所有运行中的工具暂停计时。结果是普通后台 Agent 可能无限卡住，并持续占用 Session，且永远没有终态结果。
审批等待会暂停对应工具的期限。无工具 round 在等待 Monitor 所属外部输入时，会暂停模型期限，直到输入到达。排队中的工具不会在真正执行前开始计时；由主机休眠或本地事件循环卡顿导致的延迟定时器会重新计时，而不是归咎于 Agent。
Reviewer Test Plan
如何验证
TIMEOUTfinish，持久化为failed，并且不重试。证据（Before & After）
Before：普通后台 Agent 没有逻辑进度期限；workflow watchdog 会重试，并把所有运行工具视为无界等待。
After：普通后台 turn 拥有独立的模型/控制和逐工具期限，协作式停滞只会结算一次为
TIMEOUT/failed。已测试平台
环境（可选）
只做了静态 diff 复核和格式化。未运行本地测试、build、typecheck 或 CI 命令。
风险与范围
关联 Issue
属于 #8586 的一部分。
依赖 #11265。