feat(daemon): Add ACP channel transport liveness - #9976
Conversation
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
E2E test reportTested the locally built bundle on macOS 26.4.1 / Node.js 24.12.0 at Scenario
Result
Supporting checks
|
|
✅ Qwen Triage finished — CI landed green on ✅ Qwen Triage 已完成 —— |
|
Thanks for the PR! Template looks good ✓ — all required sections present, bilingual, and the reviewer test plan is concrete. Problem: real, not theoretical. The daemon today detects child exit and explicit transport failure, but a child that stays alive with its pipe open while its event loop or ACP transport is wedged holds every multiplexed Session resident indefinitely — no signal reaches the existing teardown path. The E2E report comment in this thread shows a real SIGSTOP reproduction of exactly that gap (author-reported, not independently re-run at this stage). This is Layer 2 of #8586, whose design explicitly rules out using Session active-work silence as the transport-health signal. Direction: aligned. This is the next PR in the #8586 stack, following four already-merged PRs (#8588, #9042, #9134, #9820), and the scope is exactly Layer 2 (transport and process liveness) with Layers 3–5 left to their own PRs. No public config, persistence, or status-schema changes. The claude-code CHANGELOG has no direct reference to channel-level liveness, but the area (long-lived background agents, daemon health) is clearly relevant. Size: 263 production lines (channel-liveness.ts 168, bridge.ts 61, acpAgent.ts 31, bridgeTypes.ts 2, status.ts 1) + 500 test lines + 89 lines of design doc. Below the 500-line maintainer-awareness threshold. The change spans two packages (acp-bridge + cli), so the core-infrastructure confidence bar applies — noting the author is a Approach: the scope feels right and minimal — every file serves the feature (the monitor, capability negotiation on both sides, teardown wiring, tests, design doc). The pieces that go beyond a naive "ping and kill" — capability negotiation, monotonic late-timer classification, exact nonce echo, reusing the existing transport-failure path — each answers a real risk of the naive version rather than speculative flexibility. I don't see a materially simpler path that keeps the failure semantics correct for multiplexed Sessions. Risk: Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ —— 所有必填部分齐全，中英双语，reviewer 测试计划具体可执行。 问题：真实存在，不是理论性的。目前 daemon 能检测子进程退出和显式传输失败，但子进程存活、pipe 打开、事件循环或 ACP 传输卡死时，所有复用该通道的 Session 会无限期驻留——没有任何信号进入已有的 teardown 路径。本线程的 E2E 报告评论展示了对该缺口的真实 SIGSTOP 复现（作者自述，本阶段未独立复现）。这是 #8586 的第 2 层，其设计已明确排除用 Session active-work 沉默作为传输健康信号。 方向：对齐。这是 #8586 叠加 PR 栈中的下一个，前四个（#8588、#9042、#9134、#9820）已合并；范围正好是第 2 层（传输与进程活性），第 3–5 层留给后续 PR。无公开配置、持久化或状态 schema 变更。claude-code CHANGELOG 没有直接提到 channel 级 liveness，但该领域（长时后台 agent、daemon 健康）明显相关。 规模：生产代码 263 行（channel-liveness.ts 168、bridge.ts 61、acpAgent.ts 31、bridgeTypes.ts 2、status.ts 1）+ 测试 500 行 + 设计文档 89 行，低于 500 行维护者关注阈值。变更跨两个包（acp-bridge + cli），适用核心基础设施的信心标准——同时注明作者是 方案：范围合理且最小——每个文件都服务于该特性（monitor、双端能力协商、teardown 接线、测试、设计文档）。超出朴素 "ping 后杀进程" 的部分——能力协商、单调时钟的延迟定时器分类、精确 nonce 回显、复用既有传输失败路径——各自对应朴素方案的真实风险，而非过度设计。没有看到能在保持复用 Session 正确失败语义的前提下更简的路径。 风险： 进入代码审查。🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewThe implementation matches what I would have proposed for this problem: an opt-in, negotiated channel-level ping owned by
No blockers, no convention violations, no drive-by changes. Downstream consumers are the daemon serve bridge and the QwenAgent child handshake; the sequenceDiagram
participant P1 as Daemon bridge
participant P2 as Liveness monitor
participant P3 as ACP child
participant P4 as Sessions on the channel
P1->>P3: initialize advertising channelLiveness v1
P3-->>P1: echo v1, capability negotiated
P1->>P2: start monitor after handshake, 15s cadence
P2->>P3: ping nonce 0
P3-->>P2: echo v1 nonce 0
Note over P3: child wedges, event loop frozen
P2->>P3: ping nonce 1, no answer within 10s
P2->>P3: immediate retry nonce 2, no answer within 10s
P2->>P1: onFailure acp_channel_liveness_timeout
P1->>P1: markTransportFailed, block new admission
P1->>P3: transportGuard.fail, terminate child
P3-->>P1: channel.exited
P1-->>P4: session_died channel_closed, every Session torn down
Files changed (9 of 9)
Test evidence (PR's own CI — unattended run, no PR code executed here)Nothing red on the reviewed commit at review time. The ubuntu Node suite is still running — that is the PR-time Node signal; per ci.yml the macOS/Windows Node suites and the CLI integration suite run only in the merge queue, so their skips below are standing repo policy, not caused by this PR. SDK Java is fully green (ubuntu 11/17/21, macOS 21, Windows 21, and the Real daemon E2E job), as are Security Checks and the Desktop Shell builds. Bot orchestration checks (triage, review-pr) are not PR CI and are left out of the table. The finalize workflow updates the table below once CI settles. Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 Not verified: runtime behavior on Windows and Linux — the author tested macOS only (author's claim). The SIGSTOP end-to-end run reported in this thread is likewise the author's claim, not evidence re-run here; unattended CI never executes PR code. Sandboxed verification would settle this: 中文说明代码审查实现与我针对该问题会提出的方案一致：由
无阻塞项、无规范违反、无夹带改动。下游消费方是 daemon serve bridge 与 QwenAgent 子进程握手； 时序图（英文主评论中）：daemon 在 initialize 中声明能力并等待回显；协商成功后每 15 秒发一次 ping；子进程卡死后，第一次按时超时立即重试，第二次连续按时超时触发 onFailure；daemon 先同步标记传输失败、阻止新接入，再经 transportGuard 终止子进程，最后由既有 exited 路径清理该通道上的全部 Session 并发送 session_died。 测试证据引用的是本 PR 自己的 CI（无人值守运行，不在此执行 PR 代码）。审查时该提交没有红色检查：ubuntu Node 套件仍在运行（PR 阶段的 Node 信号）；按 ci.yml 配置，macOS/Windows Node 套件与 CLI 集成套件仅在 merge queue 运行，其跳过是既有仓库策略而非本 PR 导致。SDK Java 全绿（含 Real daemon E2E），安全检查与 Desktop Shell 构建通过。机器人编排检查（triage、review-pr）不计入。CI 落定后 finalize 工作流会更新表格。 未验证：Windows 与 Linux 上的运行时行为——作者仅在 macOS 上测试（作者自述）。线程中的 SIGSTOP 端到端运行同样是作者自述，非此处复跑的证据。 沙箱验证可以定论： — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 4/5 — clean review with nothing to fix; it's not a 5 because the end-to-end behavior (a genuinely SIGSTOP'd child getting condemned and recycled) rests on the author's macOS report and the fake-timer suite so far, and CI hasn't fully landed. Stepping back: this is a strong, disciplined layer of the #8586 stack. The approach matches what I would have chosen independently — channel-owned rather than session-owned monitoring, explicit capability negotiation, two on-time timeouts before touching multiplexed Sessions, and a terminal path that reuses the existing transport-failure lifecycle instead of forking a new one. What I verified in this review is that each of those claims holds against the base code, not just that the diff says so: stop coverage on every kill path, the guard's idempotent fail path, the bounded failure code accepted by The remaining reservation is not about the code. Windows/Linux runtime behavior is unexercised by the author, and the one claim a green suite cannot prove — that a truly wedged transport is detected inside the negotiated 35-second bound and replacement admission still works afterwards — is what the Verdict: approve — CI is still running, so the approval is deferred until CI lands green on 中文说明信心：4/5 —— 审查干净，没有需要修改的项；未到 5 分是因为端到端行为（真正被 SIGSTOP 的子进程被判定失效并回收）目前只有作者的 macOS 报告与假定时器测试套件支撑，且 CI 尚未全部落定。 整体看：这是 #8586 栈中一层高质量、有纪律的递进。方案与我独立会选择的一致——monitor 归 channel 而非 Session 所有、显式能力协商、两次按时超时才动用复用的 Session、终止路径复用既有传输失败生命周期而不是另起一套。本次审查逐一验证了这些主张在基础代码中成立，而不只是 diff 自述：每条 kill 路径的 stop 覆盖、guard fail 路径的幂等、有界失败码被 保留意见不在代码本身：Windows/Linux 运行时行为作者未验证；绿套件无法证明的那一条——真正卡死的传输在协商的 35 秒上限内被检测、且替换接入随后仍可用——正是上面点名的 结论：批准 —— CI 仍在运行，待其在被审查的提交上全绿后，由 finalize 工作流自动执行批准。 — Qwen Code · qwen3.8-max Reviewed at |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship — CI landed green after the review. ✅
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. Suggestions are inline.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not explored to full depth (tool budget reached): "agent 4": none — no check was cut short by the tool budget..
中文说明
仅完成部分审查，审查缺口已披露。 建议见行内评论。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未探索到全部深度（达到工具调用预算）："agent 4"：none — no check was cut short by the tool budget.。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| ) | ||
| : ACTIVE_WORK_LEGACY_HOLD_CATEGORIES | ||
| : undefined; | ||
| const requestedChannelLiveness = args._meta?.[CHANNEL_LIVENESS_META_KEY]; |
There was a problem hiding this comment.
[Suggestion] The added channel-liveness negotiation is the third copy of the same versioned _meta advertise/parse/echo boilerplate (after CHANNEL_STARTUP_PROFILE and ACTIVE_WORK_HEARTBEAT), and the inline record-shape check duplicates the existing isObjectRecord helper defined earlier in this file (line 791), whose semantics match it exactly. The record-shape validation now has three spellings across two files (bridge.ts isRecord, this inline triple condition, isObjectRecord), so the checks can drift when one copy is edited and the others aren't: a version bump or meta-key change needs coordinated edits at 4+ sites, and missing one silently disables the capability with no error.
The locally available helper expresses the added check today without the cast:
const channelLivenessRequested =
isObjectRecord(requestedChannelLiveness) &&
requestedChannelLiveness['v'] === CHANNEL_LIVENESS_VERSION;Since the new block deliberately matches its two siblings' style, consider converting all three inline checks (profile, active-work, channel-liveness) together.
中文说明
新增的 channel-liveness 协商是同一套带版本号的 _meta advertise/parse/echo 模板的第三份拷贝（此前已有 CHANNEL_STARTUP_PROFILE 和 ACTIVE_WORK_HEARTBEAT），并且内联的 record 形状检查与本文件前文（第 791 行）已定义的 isObjectRecord helper 重复，两者语义完全一致。record 形状校验现在在两个文件中有三种写法（bridge.ts 的 isRecord、此处的内联三重条件、isObjectRecord），当其中一份被修改而其他未同步时，这些检查就可能发生漂移：版本升级或 meta-key 变更需要在 4 处以上协同修改，漏掉任何一处都会悄无声息地禁用该能力且不会有任何报错。
现有的本地 helper 无需类型断言即可表达新增的检查：
const channelLivenessRequested =
isObjectRecord(requestedChannelLiveness) &&
requestedChannelLiveness['v'] === CHANNEL_LIVENESS_VERSION;由于新代码块刻意与其两个兄弟代码块保持同款风格，建议将三处内联检查（profile、active-work、channel-liveness）一并转换。
— qwen3.8-max via Qwen Code /review (v0.22.0)
There was a problem hiding this comment.
Thanks — agreed, will fix. Will switch all three _meta negotiation checks (startup profile, active-work, channel-liveness) to the isObjectRecord helper so the three copies cannot drift.
| ci.emptyReapPending = false; | ||
| ci.isDying = true; | ||
| ci.channelLiveness?.stop(); |
There was a problem hiding this comment.
[Suggestion] This PR pairs channelLiveness?.stop() with every channel-dying site it touches — but the two restore-flow empty-reap markers that also set isDying = true were left without it: line ~7317 (if (ci.emptyReapPending) { ci.isDying = true; }) and line ~7546 (ci.emptyReapPending = true; ci.isDying = true;). The impact today is bounded — the monitor's isActive() gate (!isDying) suppresses any further probe or condemnation, and the unref'd interval timer self-terminates on its next firing (≤15s) — but the invariant this PR establishes, "every path that marks a channel dying stops its liveness monitor", holds at 7 sites and silently not at these 2. A restore failure that marks the channel dying while reapPendingEmptyChannel (anchored here) returns early at its channelShouldReapWhenIdle/hasNoChannelWork guard leaves the monitor running until the next isActive re-check happens to fire — and any future edit that delays or skips that reap inherits a monitor whose cleanup depends on the re-check instead of the deterministic stop every sibling gets. Add ci.channelLiveness?.stop(); next to ci.isDying = true; at both restore-flow markers to match the sibling paths.
中文说明
本 PR 在它触及的每个 channel 死亡路径上都将 channelLiveness?.stop() 与 isDying = true 配对 —— 但同样会设置 isDying = true 的两个 restore 流程 empty-reap 标记点却没有加上：约第 7317 行（if (ci.emptyReapPending) { ci.isDying = true; }）和约第 7546 行（ci.emptyReapPending = true; ci.isDying = true;）。目前的影响是有界的 —— monitor 的 isActive() 门（!isDying）会阻止后续的探测或失效判定，且未 ref 的 interval timer 会在下次触发时自行终止（≤15 秒）—— 但本 PR 建立的不变量"每条将 channel 标记为死亡的路径都要停止其 liveness monitor"在 7 处成立，在这 2 处却悄然不成立。当 restore 失败将 channel 标记为死亡、而 reapPendingEmptyChannel（锚点处）在其 channelShouldReapWhenIdle/hasNoChannelWork 守卫处提前返回时，monitor 会继续运行，直到下一次 isActive 复查恰好触发为止 —— 而未来任何推迟或跳过该 reap 的改动，都会继承一个清理依赖于复查、而非每个兄弟路径都有的确定性 stop 的 monitor。请在这两个 restore 流程标记点的 ci.isDying = true; 旁添加 ci.channelLiveness?.stop();，与兄弟路径保持一致。
— qwen3.8-max via Qwen Code /review (v0.22.0)
There was a problem hiding this comment.
Thanks — agreed, will fix. Will add ci.channelLiveness?.stop() next to isDying = true at both restore-flow empty-reap markers so the invariant holds at all sites instead of relying on the isActive re-check.
| await expect( | ||
| agent.extMethod(SERVE_STATUS_EXT_METHODS.channelPing, { | ||
| v: CHANNEL_LIVENESS_VERSION, | ||
| nonce: -1, |
There was a problem hiding this comment.
[Suggestion] The child-side channelPing validation is only exercised with nonces 42 and −1. The acceptance boundary nonce: 0 — the daemon's first probe on every negotiated channel (let nextNonce = 0) — plus the wrong-v, missing/non-number nonce, and non-safe-integer branches have no test. Verified by mutation probe at the reviewed commit: the one-character mutant nonce < 0 → nonce <= 0 passes the entire shipped suite (2 passed | 463 skipped), yet it makes the child reject the daemon's very first ping; a rejected probe after the v1 ack is a definite protocol failure with no retry budget, so every newly spawned channel would be condemned and killed ~15s after startup — infinite channel churn that no test catches. The same gap hides mutants dropping the params['v'] or Number.isSafeInteger checks. Adding a nonce: 0 acceptance assertion turns the mutant red (promise rejected "Error: Invalid channel liveness ping" instead of resolving). Add cases: nonce: 0 resolves to { v: 1, nonce: 0 }; v: 2, missing nonce, and nonce: 1.5 each reject with 'Invalid channel liveness ping'.
中文说明
子进程侧的 channelPing 校验目前只用 nonce 42 和 −1 验证过。接受边界 nonce: 0 —— daemon 对每条协商成功 channel 的首个探测（let nextNonce = 0）—— 以及错误的 v、缺失/非数字 nonce、非安全整数分支都没有测试。已在被审提交上通过变异探针验证：单字符变异 nonce < 0 → nonce <= 0 能通过整个现有测试套件（2 passed | 463 skipped），但它会让子进程拒绝 daemon 的第一个 ping；而 v1 ack 之后被拒绝的探测属于确定性协议失败且不消耗重试预算，因此每个新启动的 channel 都会在启动约 15 秒后被判定失效并杀掉 —— 这是没有任何测试能捕获的无限 channel 翻转。同样的缺口也会放走删除 params['v'] 或 Number.isSafeInteger 检查的变异。添加 nonce: 0 的接受断言后该变异变红（promise rejected "Error: Invalid channel liveness ping" instead of resolving）。建议补充用例：nonce: 0 应解析为 { v: 1, nonce: 0 }；v: 2、缺失 nonce、nonce: 1.5 均应以 'Invalid channel liveness ping' 拒绝。
— qwen3.8-max via Qwen Code /review (v0.22.0)
There was a problem hiding this comment.
Thanks — agreed, will fix. Will add nonce: 0 acceptance plus v: 2 / missing nonce / nonce: 1.5 rejection cases. The <= 0 mutant turning first-probe rejection into per-channel protocol failure is a real gap worth pinning.
| vi.useFakeTimers(); | ||
| const probe = vi.fn(async () => ({ | ||
| v: CHANNEL_LIVENESS_VERSION, | ||
| nonce: 99, |
There was a problem hiding this comment.
[Suggestion] The only malformed-response test varies the nonce while keeping v correct, so isValidResponse's ['v'] === CHANNEL_LIVENESS_VERSION clause — and the null / array / non-object guards — is verified by no test. Verified by mutation probe at the reviewed commit: deleting the v clause keeps all 9 tests green; adding a wrong-version case turns that mutant red (1 failed | 9 passed). A child that acknowledged v1 but answers probes with { v: 2, nonce: 0 } (buggy or hostile) must be a definite protocol failure per the wire contract — with the clause deleted, every test still passes and the daemon silently accepts wrong-version responses as healthy probes, so the strictness the contract promises disappears and nothing turns red. Parameterize the malformed-response test to also cover { v: 2, nonce: <expected> }, null, and a non-object response, each expecting one onFailure with CHANNEL_LIVENESS_PROTOCOL_ERROR_CODE.
中文说明
唯一的畸形响应测试只改变 nonce 而保持 v 正确，因此 isValidResponse 的 ['v'] === CHANNEL_LIVENESS_VERSION 子句 —— 以及 null / 数组 / 非对象守卫 —— 没有任何测试验证。已在被审提交上通过变异探针验证：删除 v 子句后全部 9 个测试仍然为绿；新增一个错误版本用例后该变异变红（1 failed | 9 passed）。一个已确认 v1、却以 { v: 2, nonce: 0 } 应答探测的（有缺陷或恶意的）子进程，按 wire contract 必须被视为确定性协议失败 —— 删除该子句后所有测试依旧通过，daemon 会悄无声息地把错误版本的响应当作健康探测接受，契约承诺的严格性随之消失且没有任何测试变红。请将畸形响应测试参数化，同时覆盖 { v: 2, nonce: <expected> }、null 和非对象响应，每种都期望恰好一次 onFailure 且错误码为 CHANNEL_LIVENESS_PROTOCOL_ERROR_CODE。
— qwen3.8-max via Qwen Code /review (v0.22.0)
There was a problem hiding this comment.
Thanks — agreed, will fix. Will parameterize the malformed-response test to also cover wrong v, null, and non-object responses, each expecting one onFailure with the protocol-error code.
| if (info.channel.transportGuard) { | ||
| info.channel.transportGuard.fail(error); | ||
| } else { | ||
| void killChannelWithLog(info, 'channel liveness failure'); |
There was a problem hiding this comment.
[Suggestion] The liveness-failure kill fallback for channels without a transportGuard (injected channels) has no test: the only bridge-level liveness-failure test installs a guard on its makeChannel handle, and makeChannel provides none by default. Verified by mutation probe at the reviewed commit: deleting this else branch passes the whole acp-bridge suite (33 files / 1702 tests); adding a guard-less liveness-failure test turns that mutant red — it times out awaiting channel.exited while stderr prints the kill line but no kill ever occurs. That is exactly the leak mode this layer exists to close: on a guard-less channel, markTransportFailed only sets flags (it never kills), so without this fallback a condemned child would never be killed, channel.exited would never fire, the multiplexed Sessions would get no session_died, and the child would leak. Add a bridge-level test mirroring the two-timeout test with the default guard-less makeChannel: assert the channel is killed via this fallback, sessions are torn down, and the channel.exited telemetry still carries transport_failed=true with the liveness timeout code.
中文说明
针对没有 transportGuard 的 channel（注入式 channel）的 liveness 失败兜底 kill 没有任何测试：唯一的 bridge 级 liveness 失败测试在其 makeChannel 句柄上安装了 guard，而 makeChannel 默认不提供。已在被审提交上通过变异探针验证：删除这个 else 分支，整个 acp-bridge 测试套件（33 个文件 / 1702 个测试）全部通过；新增一个无 guard 的 liveness 失败测试后该变异变红 —— 等待 channel.exited 超时，stderr 打印了 kill 日志但实际上从未执行任何 kill。这正是本层要消除的泄漏模式：在无 guard 的 channel 上，markTransportFailed 只设置标志（它从不 kill），因此没有这个兜底，被判失效的子进程永远不会被杀掉，channel.exited 永远不会触发，复用的 Session 收不到 session_died，子进程就此泄漏。请参照两连超时测试、使用默认无 guard 的 makeChannel 增加一个 bridge 级测试：断言 channel 通过该兜底被杀掉、Session 被清理，且 channel.exited 遥测仍携带 transport_failed=true 与 liveness 超时错误码。
— qwen3.8-max via Qwen Code /review (v0.22.0)
There was a problem hiding this comment.
Thanks — agreed, will fix. Will add a guard-less liveness-failure test mirroring the two-timeout test: assert the killChannelWithLog fallback kills the channel, sessions get session_died, and channel.exited still carries transport_failed=true with the liveness timeout code.
| 'qwen-code.daemon.channel.transport_error_code': error.code, | ||
| }); | ||
| writeStderrLine( | ||
| `qwen serve: channel liveness failed (${error.code}); killing channel`, |
There was a problem hiding this comment.
[Suggestion] This escalation stderr line carries no channel id, workspace, or session count, so the daemon's most consequential new self-initiated kill is not attributable from its always-on log surface. The channel.liveness_failed telemetry event does include acp_channel.id and session_count, but daemon telemetry defaults off (telemetry.enabled ?? false in run-qwen-serve.ts), and the paired channel.exited event also lacks a channel id — so even with telemetry on, pairing the two events is order-based guesswork. A single qwen serve process hosts multiple bridges concurrently (the main bridge, a secondary bridge, and per-workspace bridge instances), so interleaved unattributed kill lines are a realistic operator scenario. There is precedent for embedding the id in stderr (on channel ${ci.id} at line ~3424). Include bounded identifying fields, e.g. qwen serve: channel liveness failed (id=${info.id}, code=${error.code}, sessions=${info.sessionIds.size}); killing channel.
中文说明
这条升级路径的 stderr 日志不携带 channel id、workspace 或 session 数量，因此 daemon 最具影响力的新增自主 kill 无法从其常开的日志面上归因。channel.liveness_failed 遥测事件确实包含 acp_channel.id 和 session_count，但 daemon 遥测默认关闭（run-qwen-serve.ts 中 telemetry.enabled ?? false），且配对的 channel.exited 事件同样缺少 channel id —— 因此即使开启遥测，把两个事件配对也只能靠顺序猜测。单个 qwen serve 进程会同时承载多个 bridge（主 bridge、次级 bridge 以及每个 workspace 的 bridge 实例），所以相互交错、无法归因的 kill 日志是真实存在的运维场景。在 stderr 中嵌入 id 已有先例（约第 3424 行的 on channel ${ci.id}）。建议加入有界的标识字段，例如 qwen serve: channel liveness failed (id=${info.id}, code=${error.code}, sessions=${info.sessionIds.size}); killing channel。
— qwen3.8-max via Qwen Code /review (v0.22.0)
There was a problem hiding this comment.
Thanks — agreed, will fix. Will add bounded identifying fields (channel id, error code, session count) to the liveness-failure stderr line, matching the existing on channel ${ci.id} precedent.
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. Suggestions are inline.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not reviewed: verification and reverse audit — each ran and opened its brief, but neither was launched with the prompt the CLI built — the launches were written by hand, so the posted findings cannot be counted as verified, and what the agents were actually asked is not what this skill certifies.
中文说明
仅完成部分审查，审查缺口已披露。 建议见行内评论。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未审查：验证与反向审计——两者都运行并打开了各自的 brief，但都不是用 CLI 构建的 prompt 启动的——启动 prompt 是手写的，发布的发现不能算作已验证，agent 实际被要求做的也不是本 skill 所认证的内容。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| const requestedChannelLiveness = args._meta?.[CHANNEL_LIVENESS_META_KEY]; | ||
| const channelLivenessRequested = | ||
| requestedChannelLiveness !== null && |
There was a problem hiding this comment.
[Suggestion] This adds a third copy of the same inline _meta capability-request parse in one initialize handler — channelLivenessRequested joins the pre-existing profileRequested and activeWorkRequested blocks (lines 4374 and 4381), all five lines identical apart from the meta key and version constant. Each future _meta-negotiated capability adds another copy, and the copies can drift independently: if the version-match or record-shape rule ever needs a fix, it must be applied to each copy by hand, and missing one silently disables that capability's negotiation for children that do acknowledge it. Consider extracting one small local helper and calling it for all three capabilities:
function metaCapabilityRequested(
meta: Record<string, unknown> | undefined,
key: string,
version: number,
): boolean {
const requested = meta?.[key];
return (
requested !== null &&
typeof requested === 'object' &&
!Array.isArray(requested) &&
(requested as Record<string, unknown>)['v'] === version
);
}中文说明
这里在同一个 initialize handler 中新增了第三个相同的内联 _meta 能力请求解析副本——channelLivenessRequested 与已有的 profileRequested、activeWorkRequested（第 4374、4381 行）并列，除 meta key 和版本常量外五行完全相同。未来每新增一个 _meta 协商能力就会再多一个副本，而这些副本可能各自漂移：一旦版本匹配或 record 形状规则需要修复，必须手动应用到每个副本，漏掉一个就会让确实支持该能力的 child 静默失去协商。建议抽取一个本地小工具函数，三个能力都调用它：
function metaCapabilityRequested(
meta: Record<string, unknown> | undefined,
key: string,
version: number,
): boolean {
const requested = meta?.[key];
return (
requested !== null &&
typeof requested === 'object' &&
!Array.isArray(requested) &&
(requested as Record<string, unknown>)['v'] === version
);
}— qwen3.8-max via Qwen Code /review (v0.22.0)
There was a problem hiding this comment.
Thanks — same concern as the earlier review pass (already acknowledged there). Will extract one helper and route all three capability checks through it in the fix commit.
| channelLivenessNegotiated = | ||
| isRecord(channelLivenessCapability) && | ||
| channelLivenessCapability['v'] === CHANNEL_LIVENESS_VERSION; |
There was a problem hiding this comment.
[Suggestion] The ['v'] === CHANNEL_LIVENESS_VERSION clause here is unpinned by any test. The two bridge tests only cover ack-with-v:1 and no-meta-at-all, so deleting this clause leaves both green — yet under that mutation a child acking { v: 2 } would get a v1 monitor, its {v: 2, nonce} echo would trip the strict-echo check, and a healthy channel would be condemned with acp_channel_liveness_protocol_error. Verified by mutation: with the clause deleted the suite stayed green (Tests 2 passed | 779 skipped), and a wrong-version ack test failed under the mutant (a v1 ping was sent to a v2-acking child) while passing against this commit. Adding a bridge case where initialize acks { v: CHANNEL_LIVENESS_VERSION + 1 } — asserting no channelPing and no kill after advancing CHANNEL_LIVENESS_INTERVAL_MS + 2 * CHANNEL_LIVENESS_PROBE_TIMEOUT_MS — would pin the clause.
中文说明
这里的 ['v'] === CHANNEL_LIVENESS_VERSION 子句没有任何测试锁定。两个 bridge 测试只覆盖“ack 携带 v:1”和“完全没有 _meta”两种情况，因此删除该子句后两个测试依然全绿——但在该变异下，ack { v: 2 } 的 child 会被挂上 v1 monitor，其 {v: 2, nonce} 响应会触发严格回显校验，导致健康 channel 被以 acp_channel_liveness_protocol_error 判死。已通过变异验证：删除该子句后测试仍为绿色（Tests 2 passed | 779 skipped），而新增的错误版本 ack 测试在变异体下失败（v1 ping 被发给了 ack v2 的 child），在当前提交上通过。建议新增一个 bridge 用例：initialize 应答 { v: CHANNEL_LIVENESS_VERSION + 1 }，并在推进 CHANNEL_LIVENESS_INTERVAL_MS + 2 * CHANNEL_LIVENESS_PROBE_TIMEOUT_MS 后断言没有 channelPing 调用且没有 kill。
— qwen3.8-max via Qwen Code /review (v0.22.0)
There was a problem hiding this comment.
Thanks — agreed, will fix. Will add a bridge case where initialize acks { v: CHANNEL_LIVENESS_VERSION + 1 } and assert no channelPing is sent and the channel is never killed after advancing past the probe window. New finding vs the earlier pass, verified against the negotiation code.
| writeStderrLine( | ||
| `qwen serve: channel liveness failed (${error.code}); killing channel`, | ||
| ); |
There was a problem hiding this comment.
[Suggestion] This escalation breadcrumb names neither the channel nor the workspace, but one daemon process can host several bridges — the primary, the secondary, and per-workspace bridges in run-qwen-serve.ts — each with its own channel and liveness monitor, all writing to the same daemon stderr. When liveness condemns a channel, the operator sees this line followed by the equally unidentifying channel exited line and cannot tell which workspace's child was killed without correlating per-client session_died frames or querying telemetry. The identifying data is already one statement above — the channel.liveness_failed event carries qwen-code.daemon.acp_channel.id.
| writeStderrLine( | |
| `qwen serve: channel liveness failed (${error.code}); killing channel`, | |
| ); | |
| writeStderrLine( | |
| `qwen serve: channel liveness failed (${error.code}); killing channel ${info.id}`, | |
| ); |
中文说明
这条升级开始的 stderr 日志既没有 channel 也没有 workspace 标识，但一个 daemon 进程可以同时承载多个 bridge——run-qwen-serve.ts 中的主 bridge、secondary bridge 和按 workspace 创建的 bridge——每个都有自己的 channel 和活性 monitor，全部写入同一个 daemon stderr。当活性判定终止某个 channel 时，操作者看到这条日志后紧接着同样是无法定位的 channel exited 日志，不关联各客户端的 session_died 帧或查询遥测就无法判断是哪个 workspace 的 child 被 kill 了。而标识数据就在上一条语句里——channel.liveness_failed 事件已经携带 qwen-code.daemon.acp_channel.id。
— qwen3.8-max via Qwen Code /review (v0.22.0)
There was a problem hiding this comment.
Thanks — already acknowledged in the earlier review pass; the stderr line will carry the channel id (plus error code and session count) in the fix commit.
| } else { | ||
| void killChannelWithLog(info, 'channel liveness failure'); | ||
| } |
There was a problem hiding this comment.
[Suggestion] This killChannelWithLog fallback — the terminal branch for channels without a transportGuard (injected/factory channels) — has no test. The only bridge-level failure test injects a guard, and the harness's default makeChannel() builds no guard, so this branch is never exercised. If a future change collapses the conditional ("real channels always have a guard"), the suite stays green and a wedged injected channel is never killed on liveness failure — its multiplexed Sessions hang indefinitely on a dead transport, precisely the gap this PR closes. Verified by mutation: a no-guard variant of the two-timeout test passes against this commit, and deleting this else branch makes it fail with a timeout (channel never killed, sessions hang). Adding that variant — asserting handle.killed === true and session_died on both sessions — would pin the branch.
中文说明
这个 killChannelWithLog 回退分支——针对没有 transportGuard 的 channel（注入/工厂 channel）的终止分支——没有测试。唯一的 bridge 级失败测试注入了 guard，而测试工具的默认 makeChannel() 不构造 guard，因此该分支从未被执行。如果未来有人合并这个条件分支（“真实 channel 总是有 guard”），测试套件依然全绿，而卡死的注入 channel 在活性失败时永远不会被 kill——其上复用的 Session 会在死传输上无限挂起，这正是本 PR 要解决的缺口。已通过变异验证：不带 guard 的两超时测试变体在当前提交上通过；删除该 else 分支后测试因超时失败（channel 未被 kill，session 挂起）。建议补充该变体——断言 handle.killed === true 且两个 session 都收到 session_died。
— qwen3.8-max via Qwen Code /review (v0.22.0)
There was a problem hiding this comment.
Thanks — already acknowledged in the earlier review pass; a guard-less liveness-failure test asserting the killChannelWithLog fallback kills the channel and tears down sessions is part of the planned fix batch.
| function isValidResponse(response: unknown, nonce: number): boolean { | ||
| return ( | ||
| typeof response === 'object' && | ||
| response !== null && | ||
| !Array.isArray(response) && |
There was a problem hiding this comment.
[Suggestion] Of this function's five guard clauses, only the nonce echo is pinned by a test — the only malformed-response test in channel-liveness.test.ts varies nonce ({ v: VERSION, nonce: 99 }). Verified by mutation: deleting the ['v'] === CHANNEL_LIVENESS_VERSION clause ships green (Tests 11 passed | 779 skipped), which would grade a child that acked v1 but echoes {v: 2, nonce} healthy indefinitely; deleting the response !== null guard also ships green, and null is a reachable ping result — the ACP SDK maps a handler returning undefined to result: null. Under that mutant the validator throws TypeError: Cannot read properties of null (reading 'v') inside the floated void runProbe(): the monitor dies silently (no further timers, so liveness protection lapses for the channel's lifetime), and in an embedded createAcpSessionBridge host without the CLI's unhandledRejection handler the floating rejection crashes the host process. The committed code handles all these shapes correctly — nothing pins that. Extending the malformed-response coverage with wrong-v, null, array, and primitive responses (each asserting CHANNEL_LIVENESS_PROTOCOL_ERROR_CODE without spending the retry budget) kills both mutants; optionally attach .catch() to the floated void runProbe() calls so any future throw inside the probe loop degrades to at worst a liveness failure.
中文说明
这个函数的五个守卫子句中，只有 nonce 回显一项被测试锁定——channel-liveness.test.ts 中唯一的畸形响应测试只变化 nonce（{ v: VERSION, nonce: 99 }）。已通过变异验证：删除 ['v'] === CHANNEL_LIVENESS_VERSION 子句后测试全绿（Tests 11 passed | 779 skipped），此时 ack 了 v1 却回显 {v: 2, nonce} 的 child 会被永久判定为健康；删除 response !== null 守卫同样全绿，而 null 是可达的 ping 结果——ACP SDK 会把返回 undefined 的 handler 映射为 result: null。在该变异体下，校验器会在被浮动的 void runProbe() 内部抛出 TypeError: Cannot read properties of null (reading 'v')：monitor 会静默死亡（不再有任何 timer，该 channel 的活性保护就此失效）；在没有 CLI 的 unhandledRejection handler 的嵌入式 createAcpSessionBridge 宿主中，这个浮动 rejection 还会直接使宿主进程崩溃。当前提交的代码对所有这些形状的处理都是正确的（协议错误判死）——只是没有任何测试锁定这一点。建议扩展畸形响应覆盖：错误 v、null、数组、原始类型各一例（每例断言 CHANNEL_LIVENESS_PROTOCOL_ERROR_CODE 且不消耗重试预算），即可杀死这两个变异体；也可以给浮动的 void runProbe() 调用附加 .catch()，使未来探测循环内的任何抛错至多降级为一次活性失败。
— qwen3.8-max via Qwen Code /review (v0.22.0)
There was a problem hiding this comment.
Thanks — the guard-clause coverage was already acknowledged in the earlier review pass (wrong-v, null, array, and non-object response cases are in the planned fix batch). Not taking the optional .catch() hardening on the floated runProbe() calls: with the committed guards the probe loop cannot throw, and the floated-call pattern matches the rest of this file — keeping this PR scoped to the test coverage.
| if ( | ||
| params['v'] !== CHANNEL_LIVENESS_VERSION || | ||
| typeof nonce !== 'number' || | ||
| !Number.isSafeInteger(nonce) || | ||
| nonce < 0 | ||
| ) { |
There was a problem hiding this comment.
[Suggestion] This validator has four rejection branches, but the tests exercise only one (nonce: -1 hits nonce < 0); wrong v, non-number nonce, and non-safe-integer nonce are untested. Deleting params['v'] !== CHANNEL_LIVENESS_VERSION || or !Number.isSafeInteger(nonce) || ships green (verified by mutation — both mutants left the suite at Tests 2 passed), so the rejection contract is only certified for negative nonces. Blast radius is bounded because the daemon re-validates every response in isValidResponse, but these are exactly the branches a future refactor of this switch is most likely to disturb. Three added rejection cases, one per untested branch, each rejects.toThrow('Invalid channel liveness ping') — { v: CHANNEL_LIVENESS_VERSION + 1, nonce: 0 } (wrong version), { v: CHANNEL_LIVENESS_VERSION, nonce: '0' } (non-number nonce), and { v: CHANNEL_LIVENESS_VERSION, nonce: Number.MAX_SAFE_INTEGER + 1 } (non-safe-integer nonce) — catch all three mutants.
中文说明
这个校验器有四个拒绝分支，但测试只覆盖了其中一个（nonce: -1 命中 nonce < 0）；错误的 v、非 number 的 nonce、非安全整数的 nonce 都没有测试。删除 params['v'] !== CHANNEL_LIVENESS_VERSION || 或 !Number.isSafeInteger(nonce) || 后测试依然全绿（已通过变异验证——两个变异体的套件结果都是 Tests 2 passed），也就是说拒绝契约只为负数 nonce 提供了保证。影响范围有限，因为 daemon 端会在 isValidResponse 中重新校验每个响应，但这些分支恰恰是未来重构这个 switch 时最容易被破坏的地方。为每个未测试分支各补充一个拒绝用例，均断言 rejects.toThrow('Invalid channel liveness ping')——{ v: CHANNEL_LIVENESS_VERSION + 1, nonce: 0 }（错误版本）、{ v: CHANNEL_LIVENESS_VERSION, nonce: '0' }（非 number 的 nonce）、{ v: CHANNEL_LIVENESS_VERSION, nonce: Number.MAX_SAFE_INTEGER + 1 }（非安全整数）——即可捕获这三个变异。
— qwen3.8-max via Qwen Code /review (v0.22.0)
There was a problem hiding this comment.
Thanks — already acknowledged in the earlier review pass; the three rejection cases (wrong v, non-number nonce, non-safe-integer nonce) plus the nonce: 0 acceptance case are in the planned fix batch.
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
Approved. Opt-in negotiation via initialize _meta (only negotiated channels get a monitor), stateless nonce-echo ping with MAX_SAFE_INTEGER wraparound, 15s interval / 10s timeout / immediate retry after first on-time timeout / condemn on second, local-delay tolerance to avoid false kills under event-loop pressure, unref'd timers, and isActive guards covering isDying/shutdown/reattach; onFailure routes through transportGuard or killChannelWithLog with telemetry. Open threads are Suggestion-level (boilerplate dedup, nonce=0/validator-branch coverage, breadcrumb attribution), none blocking.
|
Released in v0.22.2. |
|
The non-blocking review suggestions that were acknowledged but not landed before merge have been deduplicated and recorded in #10182. The follow-up covers capability-parse helper reuse, deterministic monitor stop at the two restore-flow dying markers, attributable stderr fields, wrong-version negotiation coverage, the guard-less channel kill fallback, and daemon/child validation boundary tests. It does not represent a known production correctness regression in #9976 and does not block Layer 3 of #8586. |
3 participants
What this PR does
This PR adds an opt-in ACP channel transport-liveness contract for daemon-owned child processes. A supporting child acknowledges the capability during initialization and answers one stateless ping per shared channel. The daemon waits 15 seconds between healthy probes, retries immediately after the first on-time 10-second timeout, and condemns the channel only after a second consecutive on-time timeout. Invalid responses and rejected requests are treated as protocol failures after negotiation.
The monitor uses a monotonic clock and clears its timeout streak when the parent timer itself fires late, so host suspend or a blocked daemon event loop is not charged to the child. A terminal failure enters the existing transport-failure path, which blocks new admission, records a bounded failure code, terminates the child, and tears down every Session multiplexed on that transport. Children that do not acknowledge the capability keep the previous behavior.
The change also documents the wire contract, fixed timing policy, lifecycle ownership, failure semantics, compatibility boundary, and the remaining separation from logical Agent watchdogs and runtime recovery.
Why it's needed
The daemon currently detects process exit and explicit transport failure, but a child can remain alive with its pipe open while its event loop or ACP transport stops answering. In that state, every multiplexed Session can remain resident indefinitely because no transport-liveness signal reaches the existing teardown path.
Session active-work reporting cannot safely solve this problem: silence from one Session is not proof that the shared channel is dead, and using it as such could destroy unrelated Sessions on the same child. A negotiated channel-level ping keeps transport health independent from work state and provides a bounded, backward-compatible failure path for Layer 2 of #8586.
Reviewer Test Plan
How to verify
session_diedto both Session event streams.Local verification passed the full ACP bridge test files (790 tests), the ACP child integration test file (465 tests), build, bundle, typecheck, lint, and diff checks. A real local SIGSTOP run reproduced two live Sessions on one child, observed both Sessions torn down after the negotiated timeout sequence, then completed an
OKprompt through a newly spawned child.Evidence (Before & After)
N/A — this is daemon transport behavior with no TUI change; the separate E2E report comment contains the runtime evidence.
Tested on
Environment (optional)
macOS 26.4.1, Node.js 24.12.0, npm 10.9.8, locally built bundle,
qwen serve --no-web, and a real ACP child frozen with SIGSTOP.Risk & Scope
Linked Issues
Part of #8586
中文说明
本 PR 做了什么
本 PR 为 daemon 管理的 ACP 子进程增加了一个选择性启用的 Channel 传输活性协议。支持该协议的 child 会在初始化阶段确认能力，并对每条共享 Channel 回答一个无状态 ping。daemon 在健康探测之间等待 15 秒；第一次按时触发的 10 秒超时后立即重试；只有第二次连续且按时触发的超时才会判定 Channel 失效。能力协商完成后，无效响应和请求拒绝会被视为协议失败。
monitor 使用单调时钟；如果 parent 自身的 timer 延迟触发，它会清空超时连续计数，因此宿主机休眠或 daemon 事件循环阻塞不会被错误归因于 child。终止性失败会进入已有的传输失败路径：阻止新任务进入、记录有界失败码、终止 child，并清理该传输上复用的全部 Session。未确认该能力的 child 保持原有行为。
本变更同时记录了 wire contract、固定时间策略、生命周期归属、失败语义、兼容边界，以及它与逻辑 Agent watchdog 和运行时恢复之间仍然保持的职责分离。
为什么需要
daemon 目前可以检测进程退出和显式传输失败，但 child 可能仍保持存活、pipe 仍然打开，同时其事件循环或 ACP 传输已经停止响应。在这种状态下，由于没有传输活性信号进入已有 teardown 路径，所有复用的 Session 都可能无限期驻留。
Session 的 active-work 上报无法安全解决这个问题：一个 Session 沉默并不能证明共享 Channel 已失效，如果据此处理，可能会误毁同一 child 上无关的 Session。协商后的 Channel 级 ping 将传输健康与工作状态分离，并为 #8586 的第 2 层提供一个有界、向后兼容的失败路径。
Reviewer 测试计划
如何验证
session_died。本地验证已通过完整 ACP bridge 测试文件（790 个测试）、ACP child 集成测试文件（465 个测试）、build、bundle、typecheck、lint 和 diff 检查。一次真实本地 SIGSTOP 运行先在同一个 child 上复现两个存活 Session，随后观察到两个 Session 在协商的超时序列后一起被清理，最后通过新启动的 child 完成了一个返回
OK的 prompt。证据（Before & After）
N/A — 这是 daemon 传输行为变更，没有 TUI 变化；独立的 E2E 报告评论包含运行时证据。
测试平台
环境（可选）
macOS 26.4.1、Node.js 24.12.0、npm 10.9.8、本地构建 bundle、
qwen serve --no-web，并用 SIGSTOP 冻结真实 ACP child。风险与范围
关联 Issue
属于 #8586 的一部分