fix(channel): recover ACP bridge after wake - #8211
Conversation
|
Thanks for the PR! Template: the substance is all here, though the headings deviate from the template ( Problem: observed, not theoretical. This is a follow-up to #6329 ("Recover DingTalk channel when ACP bridge stalls but bot process stays alive"), a real closed bug. The new angle is concrete too: after a long host sleep, the event-loop-lag monitor sees a huge scheduling gap and the ACP child gets killed on wake even though it was healthy. The mechanism is real — Direction: aligned. Scoped recovery of just the ACP bridge (keeping the messaging adapter connection alive) is the right call — reconnecting the adapter would duplicate platform connections and drop adapter-owned state. The suspension-vs-stall split (long gap + low CPU = sleep; long gap + high CPU = real stall) is a clean way to stop misclassifying wake as a stall. Size: cross-package (channels/base, cli, core/telemetry) but only ~316 production lines (ChannelBase.ts 24, start.ts 208, event-loop-lag.ts 84) vs ~565 test lines — under the 500-line awareness threshold. The core touch is additive (new options with safe defaults) and I traced its consumers: the ACP agent's stall log (→ the child kill this fixes), and the daemon's stall warning + metrics gauge. No escalation needed. Approach: scope feels right — host-suspension detection, bridge-only recovery with chained-disconnect coalescing, and a readiness gate held across inbound/loop/webhook boundaries are each needed for the stated goal, and I don't see a materially simpler path. One thing to consider: Risk: no elevated risk signals — none of the changed files match the revert-correlated path list. Moving on to code review. 🔍 中文说明感谢贡献！ 模板： 内容齐全，但小标题与模板不一致（用 问题： 已观测到的真实问题，非理论性加固。这是 #6329（"ACP bridge 卡死但进程存活时恢复钉钉渠道"，已关闭的真实 bug）的后续。新增的角度也很具体：长时间休眠后，event-loop-lag 监控看到一个巨大的调度间隙，ACP 子进程在唤醒时被误杀，尽管它是健康的。机制真实存在—— 方向： 对齐。把恢复范围限定在 ACP bridge（保持消息适配器连接存活）是正确的——重连适配器会复制平台连接并丢失适配器持有的状态。用"长间隙 + 低 CPU = 休眠；长间隙 + 高 CPU = 真实卡顿"来区分休眠与卡顿，是避免把唤醒误判为卡顿的干净做法。 规模： 跨包（channels/base、cli、core/telemetry），但生产代码仅约 316 行（ChannelBase.ts 24、start.ts 208、event-loop-lag.ts 84），测试约 565 行——低于 500 行的关注阈值。对 core 的改动是增量式的（带安全默认值的新选项），我已追溯其消费方：ACP agent 的卡顿日志（→ 本 PR 要修复的子进程误杀）、以及 daemon 的卡顿告警 + 指标 gauge。无需升级。 方案： 范围合理——休眠检测、仅替换 bridge 并合并链式断连、以及在 inbound/loop/webhook 边界保持就绪门控，这三者对所述目标都是必需的，我没有看到明显更简的路径。一点建议： 风险： 无升级风险信号——改动的文件均未命中与 revert 相关的高风险路径列表。 进入代码审查 🔍 — Qwen Code · qwen3.8-max-preview Reviewed at |
Code reviewI worked through the diff against my own take on how this should be built, and the implementation lands where I'd expect. No correctness blockers — the concurrency handling is the part most likely to be wrong, and it's the part that's right:
Two non-blocking notes:
Recovery flowsequenceDiagram
participant P1 as AcpBridge
participant P2 as start.ts recovery
participant P3 as Readiness gate
participant P4 as ChannelBase
P1->>P2: disconnected
P2->>P3: block
P4->>P3: inbound, loop, webhook wait
P2->>P1: new bridge, start, rewire router and channels
P2->>P3: release
P3-->>P4: proceed on new bridge
Files changed (6)
TestingThis is an unattended CI run, so I have not built or executed any PR code — the signal below is the PR's own CI on the reviewed commit, read through the API. The Linux unit suite is still running at the time of writing; the finalize job updates the table once it settles. No failures on any completed check so far. Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 The unit tests pin each layer of the change individually — suspension-vs-stall classification (including the CPU-unavailable fail-open), recovery coalescing, the gate staying blocked across chained disconnects, and the gate holding each prompt boundary. What they don't exercise is the full chain end to end under real conditions. Sandboxed verification would settle this: 中文说明代码审查我对照自己的实现思路通读了 diff，结论是 PR 的做法与我的预期一致。没有正确性阻塞项——最容易出错的并发处理恰恰是做对的部分：
两点非阻塞建议：
测试这是无人值守的 CI 运行，因此我没有构建或执行任何 PR 代码——以下信号是 PR 自身在被审 commit 上的 CI，通过 API 读取。撰写时 Linux 单元测试套件仍在运行；finalize 任务会在其结束后更新表格。目前已完成的检查中没有任何失败。 单元测试分别钉住了改动的每一层——休眠与卡顿的分类（含 CPU 不可用时的失败即上报）、恢复合并、门控在链式断连中保持阻塞、以及门控对每个提示词边界的拦截。未被覆盖的是真实条件下端到端的完整链路。 沙箱验证可以补齐这一点： — Qwen Code · qwen3.8-max-preview Reviewed at |
|
Confidence: 4/5 — a well-constructed fix for a real, observed failure; only non-blocking nits (the duplicated recovery code, and an end-to-end chain that's assembled from separately-pinned unit tests rather than one integration test). Stepping back: this is the work of someone who clearly knows the channel recovery paths. The design decision to replace only the ACP bridge and keep the messaging adapter alive is the right call, and the suspension-vs-stall split is a clean way to stop killing a healthy child on host wake. My independent take on how to build this converged on the same three pieces — CPU-ratio suspension detection, bridge-only recovery with coalescing, and a readiness gate at the prompt boundaries — so I'm endorsing the approach, not just failing to find a better one. The concurrency is where a PR like this lives or dies, and it holds up: the gate releases only after the replacement is wired in, there's no await between the gate check and the bridge capture, and the coalescing distinguishes current-bridge disconnects (re-loop) from superseded-bridge disconnects (ignore). The CPU heuristic fails open when CPU accounting is unavailable, which is the safe direction. What keeps this from a clean 5/5 is just that the full chain — real wake → suppressed kill → recovered channel — is what Approval is deferred until CI lands green on 中文说明置信度：4/5 —— 针对一个真实、已观测到的故障的良好修复；只有非阻塞的小问题（重复的恢复代码，以及一条由分别钉住的单元测试拼装、而非单一集成测试覆盖的端到端链路）。 退一步看：这显然出自熟悉渠道恢复路径的人之手。只替换 ACP bridge、保持消息适配器存活的设计决策是正确的，而用休眠/卡顿的区分来避免在主机唤醒时误杀健康子进程，是一个干净的做法。我对如何实现的独立判断与这三部分一致——基于 CPU 比例的休眠检测、带合并的仅替换 bridge、以及在提示词边界设置就绪门控——因此我是认可这个方案，而不仅仅是没找到更好的。 并发处理是这类 PR 成败的关键，而它站得住脚：门控只在替换 bridge 接入后才释放，门控检查与 bridge 捕获之间没有 await，合并逻辑能区分当前 bridge 的断连（重新循环）与已被替换 bridge 的断连（忽略）。CPU 启发式在 CPU 统计不可用时失败即上报，这是安全的方向。 之所以不是干净的 5/5，仅因为完整链路——真实唤醒 → 抑制杀进程 → 渠道恢复——正是 批准将推迟到 CI 在 — Qwen Code · qwen3.8-max-preview Reviewed at |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
— qwen3.8-max-preview via Qwen Code /review
|
@qwen-code /takeover |
qwen-code-dev-bot
added
the
autofix/takeover
|
🤝 Takeover engaged: the autofix loop now manages this PR — it will address new review feedback and resolve base conflicts until the label is removed or the round cap is reached. This is a fork PR, so the first round comes from the next scheduled scan (usually within minutes). Remove the 中文说明🤝 已接管：autofix 循环现在管理此 PR —— 将持续处理新的评审反馈与 base 冲突，直到移除标签或达到轮次上限。本 PR 来自 fork，首轮处理将由下一次定时扫描执行（通常几分钟内）。移除 |
ReviewReviewed the diff against a local checkout of Findings below, most significant first. All of them were verified by running code, not by reading alone. 1. Most of the new
|
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
— qwen3.8-max-preview via Qwen Code /review
…upe recovery - Lower the default event-loop suspend threshold to 5 minutes so any low-CPU sleep gap is filtered before it reaches the AcpBridge stall-kill threshold; pin the invariant with a cross-package test. - Extract the duplicated startSingle/startAll bridge recovery into a shared createBridgeRecovery helper.
The mock provided a record method but ChannelBaseOptions.observedContacts expects observe, so the slow-contact-recording path was never exercised.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
— qwen3.8-max-preview via Qwen Code /review
|
Addressed both threshold-test findings in
No production logic or public API changed. No screenshot is included because this is unit-test-only evidence. 中文说明已在 |
|
🤖 Addressed the latest review feedback (round 1/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 1/100 轮）。改动内容与我反驳保留之处如下： Autofix review round summary — PR #8211This round addresses the two findings the maintainer review explicitly named as Changes made this round#2 — Bridge crash no longer records a failure against in-flight loop jobs (production fix)Recovery no longer stops/starts the scheduler, so the generation bump that used
#1 — Gate tests now actually test the gates (test fix)The recheck/entry gate tests asserted Inline suggestions
Already resolved by earlier commits (verified against current code)
Open question for the maintainer (not resolved this round)#7 — unbounded readiness gate.
Recommendation: if desired, add a user-visible "reconnecting…" notice once the Conflict notes
Verification
中文说明Autofix 审查轮次总结 — PR #8211本轮处理维护者审查中明确要求在合并前解决的两条意见（#1 门控测试形同虚设、#2 被恢复中断的循环任务被记为失败），以及可操作的行内建议。其余若干审查点已由本分支更早的提交解决；下面逐条列出其当前代码证据。 本轮改动#2 — 桥接崩溃不再把被中断的进行中循环任务记为失败（生产修复）恢复路径不再 stop/start 调度器，因此过去用于保护进行中任务的 generation 自增副作用消失了；当 ACP 子进程被杀死时，某个正处于
#1 — 门控测试现在真正测试了门控（测试修复）这些 recheck/entry 门控测试在单次 行内建议
已由更早提交解决（已对当前代码核验）
留给维护者的开放问题（本轮不解决）#7 — 无界的就绪门。
建议：如需要，可在等待超过阈值时增加一个对用户可见的 “reconnecting…” 提示（不改变行为），同时保持门本身无界，使消息永远不会被路由到已死的桥接。若维护者倾向某种界限，乐于实现。（#8 的范围说明： 冲突说明
验证
Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. · 有空请复审；第 100 轮后本 bot 停止并将 PR 交给人工。 🧠 Handled by Qwen Code · model/模型 |
There was a problem hiding this comment.
Pull request overview
Improve channel resilience by rebuilding only the ACP bridge when it wedges (e.g., after long host sleep or prolonged event-loop stalls), while keeping channel adapters connected and ensuring inbound/loop/webhook prompt boundaries wait until bridge recovery completes.
Changes:
- Add host-suspension-aware event-loop lag monitoring (CPU-ratio-based) and export the default suspend threshold.
- Refactor channel start crash-recovery to rebuild ACP bridges without reconnecting adapters, and add a readiness gate to hold session routing/prompt capture until recovery completes.
- Add ACP bridge startup timeout handling, plus scheduler logic to avoid counting recovery-aborted loop prompts as agent failures, with expanded test coverage.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/core/src/telemetry/index.ts | Re-export the default event-loop suspend threshold constant. |
| packages/core/src/telemetry/event-loop-lag.ts | Add suspension filtering using CPU usage ratio; always run a background histogram check loop. |
| packages/core/src/telemetry/event-loop-lag.test.ts | Add tests covering suspension filtering, snapshot purity, and CPU-unavailable behavior. |
| packages/cli/src/commands/channel/start.ts | Introduce a bridge readiness gate and unified ACP bridge recovery that preserves adapter connectivity. |
| packages/cli/src/commands/channel/start.test.ts | Add tests ensuring bridge recovery coalescing, readiness gating, and failure cleanup without adapter reconnects. |
| packages/cli/src/acp-integration/acpAgent.ts | Configure ACP agent lag monitor to treat long low-CPU gaps as suspension. |
| packages/cli/src/acp-integration/acpAgent.test.ts | Assert ACP agent lag monitor wiring includes the suspend threshold setting. |
| packages/channels/base/src/index.ts | Re-export ACP_EVENT_LOOP_STALL_RESTART_MS from AcpBridge. |
| packages/channels/base/src/ChannelLoopScheduler.ts | Add recovery epoch tracking to avoid counting recovery-aborted prompts as failures. |
| packages/channels/base/src/ChannelLoopScheduler.test.ts | Add test for recovery-aborted in-flight loop prompt handling. |
| packages/channels/base/src/ChannelBase.ts | Add bridgeRecovery option and gate session routing / prompt bridge capture on recovery completion. |
| packages/channels/base/src/ChannelBase.test.ts | Add tests asserting inbound/webhook/loop paths wait for recovery and re-check after preprocessing. |
| packages/channels/base/src/AcpBridge.ts | Add ACP initialization timeout and ensure child is stopped on startup failure. |
| packages/channels/base/src/AcpBridge.test.ts | Add test that initialization timeout rejects and kills the ACP child process. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Resolved the remaining suggestion without a code change. The duplicated value is already protected by the ACP agent test, which compares the monitor option directly against ACP_EVENT_LOOP_STALL_RESTART_MS from channel-base. A future drift makes that test fail. Importing the production constant here would therefore be a deduplication refactor, not a current correctness fix, so I am not expanding this closeout. 中文说明剩余建议已 resolve，未改代码。现有 ACP agent 测试已经直接用 channel-base 的 ACP_EVENT_LOOP_STALL_RESTART_MS 校验 monitor 参数，未来数值漂移会使测试失败；改为生产代码跨模块导入只属于去重重构，不是当前缺陷。 |
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Reviewed at b8baca2. The highest-risk part of this change — the readiness gate's await-before-capture ordering — is correct at every site, which I checked individually rather than in aggregate. All 8 waitForBridgeRecovery() call sites gate strictly before the bridge reference is taken, including the two inside the queued prompt continuations where a stale capture would be invisible:
await this.waitForBridgeRecovery();
const promptBridge = this.bridge;That holds for the loop path (ChannelBase.ts:1623) and the webhook path (:1920), and the outer entries (:1472, :1791) gate before router.resolve, so no consumer can route into or prompt a dead bridge. The gate itself re-loops on a newly installed barrier (bridgeRecovery === completedRecovery is the exit condition, not first-await), so a recovery that starts while a consumer is already waiting is also awaited.
Recovery coalescing is sound: a disconnect arriving mid-recovery from the replacement bridge sets recoveryRequested instead of spawning a second task (failedBridge !== recoverySourceBridge), and the do…while (recoveryRequested && !isShuttingDown()) loop chains it into the same task, so the gate stays blocked across the whole chain. Release is in .finally guarded on recoveryTask === task; the two paths that skip release both process.exit(1), and the crash-window trim can't spin (the just-pushed timestamp always terminates the shift loop), so I found no path where the gate never settles. Adapter preservation checks out — the recovery body only constructs the bridge, re-points router.setBridge / channel.setBridge, and re-registers the four relays; nothing calls the adapter's connect/disconnect, and tests assert connect stays at one call.
The suspend-vs-stall discriminator's units are right (process.cpuUsage() is µs, elapsedMs * 1_000 converts the window to µs, so the ratio is dimensionless), and the reasoning holds where it matters: an active stall blocks the interval callback too, so elapsedMs and newMaxMs both cross the threshold in both scenarios — CPU ratio is the only real discriminator, and a CPU-spinning wedge lands far above 1% and is still reported and killed.
Two non-blocking notes:
-
The discriminator has one hole worth knowing about: it keys on wall-clock
Date.now()deltas plus CPU idleness, so a ≥5-minute event-loop block that is not CPU-bound — a blocking syscall on a hung network/FUSE mount, an attached debugger, a SIGSTOP/resume — is indistinguishable from host suspension and gets filtered, leaving that wedge un-killed. That is the #6329 class this PR follows up on, narrowed rather than reopened (the common spin-wedge is still caught), and the trade is deliberate per the PR body. If you want it closed later, comparing the wall-clock delta against a monotonic (process.hrtime.bigint()) delta separates the two cleanly: only real suspension advances wall clock without advancing CLOCK_MONOTONIC. -
The open copilot thread on acpAgent.ts:350 still stands at this HEAD —
ACP_EVENT_LOOP_SUSPEND_THRESHOLD_MS = 5 * 60 * 1000is still re-declared while the comment says "Match the parent channel bridge's restart threshold". I confirmed the suggested fix is feasible: channel-base exportsACP_EVENT_LOOP_STALL_RESTART_MSfrom its package entry at this HEAD, and packages/cli already depends on@qwen-code/channel-baseand imports from it in start.ts, so a package-entry import is available and house-style-clean. Worth taking, because the test asserts only numeric equality against the imported constant — it would stay green if the two drifted in the same direction, and would only catch a one-sided edit.
Ran the reviewer test plan locally at this HEAD: event-loop-lag 15/15, channels/base (ChannelBase + AcpBridge + ChannelLoopScheduler) 619/619, start 31/31, acpAgent 353/353. Note for anyone reproducing: acpAgent.test.ts fails with suspendThresholdMs: undefined against a stale packages/channels/base/dist — this PR adds ACP_EVENT_LOOP_STALL_RESTART_MS to the package entry and the test imports it through the built module, so rebuild channel-base first. Not a defect; CI builds before testing. CI green apart from review-pr still running.
| if (this.generation !== generation || !currentJob?.enabled) { | ||
| if ( | ||
| this.generation !== generation || | ||
| recoveryEpoch !== this.recoveryEpoch || |
There was a problem hiding this comment.
[P1] Scope recovery suppression to the prompt it actually aborted
recoveryEpoch is captured before await this.store.update(...) and before runLoopPrompt(). If bridge recovery starts while that write is pending, runLoopPrompt() begins afterward, waits for the recovery gate, and can then fail for a genuine reason on the replacement bridge. This epoch mismatch still clears only runningSince and returns, even though recovery did not abort that prompt. Because lastFiredAt was already advanced, the failure becomes invisible: no error status or failure count is recorded, a recurring occurrence is lost, and a one-shot job remains eligible instead of recording its failed attempt.
I reproduced this deterministically by blocking the initial store update, calling markBridgeRecovery(), releasing the write, and then rejecting the runner: the only follow-up patch was { runningSince: undefined }; no failure patch was emitted. Please scope the marker to runs actually aborted by that recovery rather than every run spanning an epoch change. The new test marks recovery from inside an already-running prompt before throwing, so it does not cover this post-recovery failure case.
qqqys
left a comment
qqqys
left a comment
There was a problem hiding this comment.
Reviewed exact head b8baca2a4b. I found one new correctness issue and left it inline: the scheduler-wide recovery epoch can also suppress a genuine failure from a prompt that starts after recovery and runs on the replacement bridge. The reproduction advances lastFiredAt but emits only a runningSince clear, with no error/failure accounting. I recommend fixing that before merge.
Outside that finding, I traced the added options and recovery consumers, rechecked the current review threads, and found the bridge-only replacement, readiness gate, chained-disconnect coalescing, startup/restore bounds, and suspension filter internally consistent at this head.
Verification: ChannelLoopScheduler.test.ts + ChannelBase.test.ts (583 passed), AcpBridge.test.ts (36 passed), event-loop-lag.test.ts (15 passed), and start.test.ts (31 passed). The changed acpAgent threshold assertion also passes when its package alias is resolved to this PR worktree; the full local file otherwise picked up the main checkout build through the shared dependency symlink, so I did not treat that environment-only mismatch as a PR failure.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
— qwen3.8-max-preview via Qwen Code /review
| it('times out bridge initialization and stops the child', async () => { | ||
| vi.useFakeTimers(); | ||
| child.setInitializeImplementation(() => new Promise(() => {})); |
There was a problem hiding this comment.
[Suggestion] Fake timers are enabled here with vi.useFakeTimers() but restored only by a trailing vi.useRealTimers() at the end of the test body, not in a finally. — Concrete cost: if any assertion before that line fails (e.g. the rejection message or the kill assertion regresses), vi.useRealTimers() is never reached and fake timers leak into the subsequent tests in this file. Later tests call bridge.start(), which awaits a real setTimeout(resolve, 1000) (AcpBridge.ts:148); under leaked fake timers that never fires, so the next test hangs until the vitest timeout and masks the real failure. The other fake-timer test in this same file already uses the safe try { … } finally { vi.useRealTimers(); } pattern.
Wrap the body in try { … } finally { vi.useRealTimers(); }:
it('times out bridge initialization and stops the child', async () => {
vi.useFakeTimers();
try {
// …existing body…
} finally {
vi.useRealTimers();
}
});— qwen3.8-max-preview via Qwen Code /review
Review (round 2 —
|
| Prior finding | Status |
|---|---|
| 1. Gate tests were vacuous | Fixed — mutation now kills 8/9 (was 1/8) |
| 2. Bridge crash recorded a failure against in-flight loops | Fixed — markBridgeRecovery() / recoveryEpoch |
| 3. Suspend threshold (10 min) above kill threshold (5 min) | Fixed — both now 5 min |
| 4. CPU ratio measured over the wrong window | Fixed — elapsedMs >= suspendThresholdMs added |
5. snapshot() had side effects |
Fixed — snapshot is read-only again, with tests |
6. failedBridge === bridge mis-attribution |
Fixed — recoverySourceBridge |
| 7. Readiness gate unbounded | Fixed — ACP_START_TIMEOUT_MS + BRIDGE_SESSION_RESTORE_TIMEOUT_MS |
| 8. ~70 duplicated lines across the two start paths | Fixed — createBridgeRecovery |
Good round. The remaining items are below, most significant first.
1. The fix for #4 made suspension classification nondeterministic
Adding elapsedMs >= suspendThresholdMs (event-loop-lag.ts:86-87) does fix the stale-max problem, but it now requires both pieces of evidence — the starved interval (elapsedMs) and the new histogram max — to be visible on the same tick. Nothing orders monitorEventLoopDelay's internal sample against our setInterval callback, so which tick the max lands on is a coin flip.
I drove the PR's and main's event-loop-lag.ts directly from a standalone harness with scaled thresholds (resolutionMs: 20, stallThresholdMs: 500, suspendThresholdMs: 2000, 3 s block — a 5-minute threshold isn't practical to exercise; the race is threshold-independent). 10 runs each:
| Block type | main |
this PR |
|---|---|---|
SIGSTOP (process frozen — host-suspend proxy, 0.08 % CPU) |
reports 10/10 | filtered 8/10, reports 2/10 |
Atomics.wait (event loop blocked, 0.05 % CPU) |
reports 10/10 | reports 7/10, filtered 3/10 |
| busy loop (100 % CPU) | reports 10/10 | reports 10/10 ✓ |
So on ~20 % of wakes the healthy child is still reported as a 5-minute-plus stall and SIGKILLed — the exact outcome the PR exists to prevent.
Instrumenting the decision points shows the two tick patterns:
filtered (correct): [{elapsedMs:3020, maxMs:3020, newMaxMs:3020, ratio:0.00008}]
reported (wrong): [{elapsedMs:3017, maxMs: 20, newMaxMs: 0, ratio:0.00049}, <- interval ran before
{elapsedMs: 20, maxMs:3003, newMaxMs:3003, ratio:0.00430}] <- the histogram published
On tick 1 the histogram hasn't published yet, so newMaxMs is 0 and nothing is classified — but lastCheckTimeMs is consumed. On tick 2 the max is there and the CPU ratio is still 0.4 %, yet elapsedMs is back to 20 ms, so the suspend branch is skipped and the gap is reported as an active stall.
The unit tests can't catch this because they assign histogram.max directly under fake timers, so the gap and the max are always visible together.
Minimal fix — keep the verdict sticky instead of requiring same-tick agreement:
let suspendGraceUntilMs = 0;
// ...
if (
elapsedMs >= suspendThresholdMs &&
cpuRatio !== undefined &&
cpuRatio <= suspendCpuRatio
) {
// The starved interval is the evidence. The histogram may not publish the
// matching sample until a later tick, so keep the verdict for a moment
// instead of requiring both signals on the same tick.
suspendGraceUntilMs = nowMs + Math.max(resolutionMs * 5, 1_000);
}
if (newMaxMs >= suspendThresholdMs && nowMs <= suspendGraceUntilMs) {
histogram.reset();
lastObservedMaxMs = 0;
lastReportedMaxMs = 0;
suspendGraceUntilMs = 0;
return;
}I applied this and re-ran: all 15 existing event-loop-lag.test.ts tests still pass (including does not suppress an old histogram max after a short idle check, whose tick-1 ratio is 3.3 % so no grace window opens), and the probe becomes deterministic — SIGSTOP filtered 10/10, busy loop reported 10/10.
Worth adding a regression test that raises histogram.max on the tick after the clock jump; that is the case the current suite cannot express.
2. …which exposes that the CPU ratio alone cannot identify a wedge
Making the classification deterministic also makes the blind spot deterministic. Measured ratios over a 3 s gap:
SIGSTOP (frozen) 0.00077
Atomics.wait (blocked loop) 0.00047
blocking read() on a FIFO 0.00202
busy loop 1.00027
The first three are indistinguishable, so "host suspended" and "event loop blocked in a low-CPU syscall" are the same observation. Today's coin flip eventually kills a low-CPU wedge; with the sticky fix such a child would be filtered indefinitely. That matters because #6329's symptom — process alive, sessions frozen, stall maxima climbing 917 s → 925 s → 952 s — is fully compatible with a low-CPU wedge.
So the sticky window and a positive liveness signal are both needed; neither alone is sufficient. The cheapest addition: when the parent filters a gap as suspension, have AcpBridge follow up with a lightweight ACP round-trip on a short timeout and kill the child if it doesn't answer. That only runs after a filtered gap, and it turns the heuristic from "guess" into "guess, then verify" — which is what #6329's acceptance criterion ("detection or recovery for a stuck bridge where the process remains alive") actually asks for. Wall-clock-vs-monotonic divergence is a cleaner signal for true S3 sleep, but I confirmed it does not help here: across SIGSTOP, Date.now() and process.hrtime.bigint() both advanced 4503 ms.
3. One gate test is still vacuous
Re-running both mutations from last round against the current tests:
- Mutation A (
await this.waitForBridgeRecovery()→void …, 7 sites): kills 8/9 (was 1/8) - Mutation B (delete all 7 call sites): kills 8/9 (was 5/8)
The single survivor under both is rechecks bridge recovery after inbound preprocessing has started (ChannelBase.test.ts:16197). Line 16215 is the problem:
await vi.waitFor(() => expect(bridge.newSession).not.toHaveBeenCalled());vi.waitFor on a negative assertion succeeds on its first poll, so it synchronises nothing. The later await Promise.resolve() is a single microtask, which is not enough for the ungated flow to reach bridge.prompt either — so both assertions hold whether or not the gate exists. The pattern the other eight tests now use works here:
await expect(
vi.waitFor(() => expect(bridge.prompt).toHaveBeenCalled(), { timeout: 500, interval: 25 }),
).rejects.toThrow();4. A genuine loop failure during recovery is now recorded nowhere
ChannelLoopScheduler.ts:230 — if any recovery started during the run, the catch path calls clearRunningSince and returns. Not bumping consecutiveFailures is right, but a real failure (model error, prompt timeout) that merely coincides with a recovery now leaves no lastStatus: 'error', no lastError, and no stderr line. That is a debuggability regression in exactly the scenario #6329 was filed about. Suggest persisting lastStatus/lastError while skipping the consecutiveFailures increment.
Minor: markBridgeRecovery() fires once per recoverBridge() call, not once per do…while iteration, so a chained recovery shares one epoch bump. Harmless today because the readiness gate spans the chain, but worth a comment.
5. start() bounds initialize but not registerChannelLoopMcpServer
AcpBridge.ts:191 — registerChannelLoopMcpServer() awaits connection.extMethod(...), whose .catch handles rejection but not a promise that never settles against a wedged child. It can't hang today (a freshly constructed bridge has no channelLoopMcpServer until registerChannelLoopToolHandler runs, which is after start()), but it is one reordering away from an unbounded await bridge.start() holding bridgeReadiness blocked forever with every inbound message queued behind it. Cheap to fold into the same withTimeout.
6. Three copies of the 5-minute threshold, and the new export has no consumers
ACP_EVENT_LOOP_STALL_RESTART_MS—AcpBridge.ts:45, the kill thresholdDEFAULT_EVENT_LOOP_SUSPEND_THRESHOLD_MS— newly exported fromcore/telemetry/index.ts, zero importers anywhere insrc/ACP_EVENT_LOOP_SUSPEND_THRESHOLD_MS—acpAgent.ts:350, a hardcoded literal whose comment says "Match the parent channel bridge's restart threshold"
The PR also newly exports ACP_EVENT_LOOP_STALL_RESTART_MS from @qwen-code/channel-base's index, and the only consumer is acpAgent.test.ts:21. packages/cli already depends on @qwen-code/channel-base (package.json:49), so acpAgent.ts can import the real constant instead of duplicating the literal — the invariant would then be enforced by the compiler rather than by a test asserting one constant equals another. Either do that, or drop the unused core export.
7. The daemon's event-loop telemetry is now silently reset on wake
run-qwen-serve.ts:3235 starts a monitor with no suspendThresholdMs, so it inherits the 5-minute default, and its snapshot() feeds registerDaemonEventLoopLagGauge plus the Status metrics ring. When a suspension is detected, histogram.reset() zeroes mean/p50/p99/max — confirmed in the probe (snapshotMaxMs drops to 20 after filtering). That is probably desirable, but it is an unannounced behaviour change for daemon telemetry that the daemon opted into by omission. Worth an explicit value at the call site or a line in the PR body.
Related, still open from last round: the interval is now created unconditionally (event-loop-lag.ts:109), where before it existed only when onNewMaxStall was supplied. Both current callers pass one, so there is no live regression, but any future snapshot()-only caller now pays Date.now() + process.cpuUsage() + a histogram read 50×/s.
8. The invariant comment overstates the code
ChannelBase.ts:354-355 says "every path that resolves a session or calls the bridge must await waitForBridgeRecovery() immediately before that operation", but respondToPermission (506, 547, 732, 2754, 2798), cancelSession (2130, 2143, 4541, 5258) and availableCommands (3092) have no gate. Most are either downstream of the processInbound entry gate or are responses to events originating from the current bridge, so I don't think any of them is a bug — but as written the comment will mislead the next reader in both directions. Narrowing it to the session-routing and prompt-capture paths it actually describes would be more useful.
Things I checked that are fine
- The chained-recovery coalescing is sound. I looked specifically for a window where a
disconnectedarriving between thedo…whilere-check and the.finallyrelease would be dropped: there isn't one, because those are separated only by microtasks whiledisconnectedoriginates from a childexitmacrotask. - The crash-window rewrite (
while (now - crashTimestamps[0] >= CRASH_WINDOW_MS) shift()) preserves the old<semantics and can neither spin nor derefundefined, sincenowis pushed first. - Both
withTimeoutandrestoreBridgeSessionsusePromise.race, so the loser's rejection is still handled — no unhandled-rejection risk. - Not reconnecting the adapter is the right call and the tests pin it (
mockChannelConnect2 → 1).
Verdict
The design is right and this round fixed nearly everything from the last one. I'd want #1 addressed before merge — a 20 % chance of killing a healthy child on every wake defeats the PR's headline goal, and the fix is a few lines that leave the existing suite green. #3 is small and closes the last hole in the regression protection. #2 is the more interesting one and could reasonably be a follow-up, but it should be written down somewhere rather than left implicit, because it decides whether #6329 is actually closed.
|
🤖 Addressed the latest review feedback (round 2/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 2/100 轮）。改动内容与我反驳保留之处如下： Round summaryAddressed both inline findings. [P1] Scope recovery suppression to the prompt it actually aborted (rc:3691879065)Decision: Fixed. The Fix: Re-read
Added a regression test that blocks the store update, calls [Suggestion] Fake timer cleanup in AcpBridge.test.ts (rc:3691926626)Decision: Fixed. Wrapped the test body in Verification
中文说明本轮总结处理了两条行内反馈。 [P1] 将恢复抑制限定到实际被中止的 prompt（rc:3691879065）决定： 已修复。
修复： 在 store update 完成且 generation 检查通过后、
新增回归测试：阻塞 store update，调用 [建议] AcpBridge.test.ts 中的假定时器清理（rc:3691926626）决定： 已修复。 将测试体包裹在 验证
Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. · 有空请复审；第 100 轮后本 bot 停止并将 PR 交给人工。 🧠 Handled by Qwen Code · model/模型 |
Review:
|
| Suite | Result |
|---|---|
packages/core telemetry/event-loop-lag.test.ts |
15/15 pass |
packages/channels/base AcpBridge + ChannelLoopScheduler |
61/61 pass |
packages/channels/base ChannelBase.test.ts |
559/559 pass |
packages/cli commands/channel/start.test.ts |
31/31 pass |
prettier --check, eslint --max-warnings 0 on the touched files |
clean |
🔴 Blocker — the wake-suppression path is a two-timer race and loses about half the time
checkHistogram() (packages/core/src/telemetry/event-loop-lag.ts:85-95) requires newMaxMs >= suspendThresholdMs and elapsedMs >= suspendThresholdMs on the same tick. Those two numbers come from two independent libuv timers: elapsedMs from this module's own setInterval (line 109), histogram.max from the timer inside monitorEventLoopDelay. After a resume both are overdue and the order they fire in is not fixed. When our interval wins, the gap lands in the histogram one tick later:
tick N : elapsedMs=2020 histogram.max=20 -> newMaxMs=0 -> suppression skipped
tick N+1 : elapsedMs=21 histogram.max=2017 -> newMaxMs=2017 -> elapsedMs check fails
The two conditions never co-occur, the wake gap is reported as a stall, and AcpBridge.maybeKillOnEventLoopStall SIGKILLs the healthy child anyway — the exact outcome the PR sets out to prevent.
Measured, driving the real startEventLoopLagMonitor from this branch in a child process frozen with SIGSTOP for 2000 ms (suspendThresholdMs: 1000, resolutionMs: 20 — production's default resolution):
| build | runs that reported a stall (two independent batches) |
|---|---|
| PR head | 7 / 12, then 9 / 12 |
| head + one-tick carry (below) | 0 / 12, then 0 / 12 |
The rate varies because the failure is the race; what's stable is that head loses it most of the time and the patched build never does.
Negative controls on the patched build, which must still report:
| scenario | result |
|---|---|
| active stall 2000 ms @ 100% CPU | reported 2009 ms ✅ |
| active stall 700 ms (below suspend threshold) | reported 710 ms ✅ |
Repro (elag.mjs = esbuild bundle of the PR's event-loop-lag.ts):
// victim.mjs — freeze me from outside with SIGSTOP
const reported = [];
const m = startEventLoopLagMonitor({ resolutionMs: 20, stallThresholdMs: 500,
suspendThresholdMs: 1000, onNewMaxStall: (x) => reported.push(Math.round(x)) });
setTimeout(() => { m.dispose(); console.log(JSON.stringify({ reported })); }, 4000);
// driver.mjs
const c = spawn(process.execPath, ['victim.mjs'], { stdio: ['ignore','pipe','inherit'] });
await sleep(500); process.kill(c.pid, 'SIGSTOP');
await sleep(2000); process.kill(c.pid, 'SIGCONT'); // reported=[] is the passSuggested fix — latch the low-CPU gap so it stays eligible for one more tick. This is the build measured at 0/12 above:
let lastCpuUsage = safeCpuUsage();
+ let pendingSuspendGapMs = 0;
@@
- if (
- newMaxMs >= suspendThresholdMs &&
- elapsedMs >= suspendThresholdMs &&
- cpuRatio !== undefined &&
- cpuRatio <= suspendCpuRatio
- ) {
+ const isLowCpuGap =
+ elapsedMs >= suspendThresholdMs &&
+ cpuRatio !== undefined &&
+ cpuRatio <= suspendCpuRatio;
+ // The histogram's own libuv timer can land the gap one tick after ours, so
+ // a low-CPU gap stays eligible for one further check.
+ const suspendGapMs = isLowCpuGap ? elapsedMs : pendingSuspendGapMs;
+ pendingSuspendGapMs = isLowCpuGap ? elapsedMs : 0;
+ if (
+ newMaxMs >= suspendThresholdMs &&
+ suspendGapMs >= suspendThresholdMs &&
+ newMaxMs <= suspendGapMs * 1.5
+ ) {
histogram.reset();
lastObservedMaxMs = 0;
lastReportedMaxMs = 0;
+ pendingSuspendGapMs = 0;
return;
}Note that simply dropping the elapsedMs condition is not a sufficient repair: on tick N+1 the denominator is only ~20 ms, so post-wake CPU very easily clears 1% of it. In my trace tick N+1's ratio came out at 0.0060 — it passed, but with almost no margin. The gap value has to be carried, not the conditions relaxed.
Why the 15 unit tests don't catch it
I applied the patch above to event-loop-lag.ts and re-ran event-loop-lag.test.ts: 15/15 pass, unchanged. The suite cannot tell the broken and fixed implementations apart, because every test assigns histogram.max = … before advanceTimersByTimeAsync, which hard-codes the one timer ordering that happens roughly 40% of the time in production. Worth adding a case that mutates histogram.max on the tick after the setSystemTime jump — it fails on head and passes with the carry.
Medium
1. ACP_START_TIMEOUT_MS (30 s) bites hardest in exactly the window it's meant to cover. A fresh qwen --acp boot right after a host wake competes with every other process resuming. If initialize misses 30 s, bridge.start() rejects, and start.ts:272 treats any rejection anywhere in the loop as terminal — straight to process.exit(1), bypassing the MAX_CRASH_RESTARTS budget entirely. (Not a regression; the old handler did the same. But the new timeout makes the path much easier to reach.) Consider letting a start failure count as a crash and re-enter the loop rather than hard-exiting on the first one.
2. Replacement-bridge coalescing only covers the post-start() window. If the replacement dies during await bridge.start() (line 256), the listener does set recoveryRequested = true (line 217) — but start() rejects, the .catch runs, and the while at line 270 is never evaluated. The PR description's "including disconnects from a replacement bridge while recovery is in progress" holds only when start() resolves; the keeps the readiness gate blocked and coalesces replacement disconnects test exercises only that path.
3. The other side of the CPU-ratio heuristic. A child genuinely wedged on a blocking syscall — hung sync I/O, deadlocked native addon, contended lock — burns ~0% CPU and is indistinguishable from suspension. Measured: in-process Atomics.wait for 2000 ms at 0% CPU is suppressed on both head and the patched build. Since #6329 is specifically about wedged-but-alive children, it's worth stating explicitly that this is accepted. A wall-vs-monotonic delta (Date.now() vs performance.now()) would separate the two on Linux/macOS, where CLOCK_MONOTONIC is frozen across an S3 suspend but keeps running through a blocking syscall — I could not verify that here, since SIGSTOP does not freeze CLOCK_MONOTONIC.
Minor
- The stated invariant is broader than the code.
ChannelBase.ts:353-356says "every path that resolves a session or calls the bridge must awaitwaitForBridgeRecovery()".cancelSession(~2130, ~2143, ~4541, ~5258) andrespondToPermission(~506, ~547, ~727, ~2798) are not gated. They're benign — a crash already resolves pending permissions as cancelled, and there's nothing left to cancel — so I'd narrow the comment to session-resolution and prompt-capture paths rather than add gates. waitForBridgeRecovery'scompletedRecoveryguard is unreachable with this gate:release()clearspendingbefore resolving, so a waiter that resumes always readsundefinedon the next loop. Harmless, but it reads as if it's handling a case that can't occur. (Message ordering is fine, by the way — waiters resume in arrival order and all drain before any newblock()can land.)- Three copies of the 5-minute constant:
ACP_EVENT_LOOP_STALL_RESTART_MS,DEFAULT_EVENT_LOOP_SUSPEND_THRESHOLD_MS, and the localACP_EVENT_LOOP_SUSPEND_THRESHOLD_MSinacpAgent.ts. The test already imports the first from@qwen-code/channel-baseto assert they match, so the dependency exists — importing it in the source too would make the invariant structural instead of test-enforced. startEventLoopLagMonitornow always creates the interval, not just whenonNewMaxStallis set. Both in-repo consumers pass the callback so nothing regresses today, but it's exported frompackages/core, and a snapshot-only caller now pays a 20 ms timer plus aprocess.cpuUsage()syscall per tick and gets its histogram silently reset on suspension. Worth a JSDoc line.
What's good
- Collapsing the two duplicated disconnect handlers into
createBridgeRecovery()is a real win — that duplication was a standing hazard, and −170/+200 for strictly more behaviour is a good trade. - Not reconnecting the adapters is the right call, and it checks out: the adapters carry their own reconnect (
WeComAdapter.startKickReconnect,DingtalkConnectionManager), so nothing is left unrepaired.start.tsis also the onlyAcpBridgeowner in the tree, so the fix has full coverage of that surface. - The
recoveryEpochdesign is subtle and correct: re-reading it after thestore.updateawait, so a bump during the write doesn't swallow a genuine failure. Therecords a genuine failure when recovery starts before the prompt runstest pins exactly that, which is the case that would have been easy to get wrong. - The loop timeout is applied after the gate wait, so a slow recovery can't eat a scheduled loop's budget.
cleanupStartedChannelsnow alsobridge.stop()s — the oldstartSinglegive-up path left the child alive.crashTimestampsis pruned now instead of growing for the process lifetime.- Asserting absence via
await expect(vi.waitFor(...)).rejects.toThrow()is a genuinely good pattern for "this must not happen within N ms", and the inline comments explaining why the window is long enough are the right thing to leave behind.
Security
Nothing concerning. No new network, filesystem, or credential surface; the readiness gate only delays existing work, and sanitizeLogText still fronts the stderr path that feeds stall detection.
Verdict: hold for the blocker. Parts 2 and 3 are ready; part 1 needs the carry (or an equivalent) plus a test that varies the tick on which histogram.max moves, otherwise the wake scenario in the title is still a coin flip.
🤖 Generated with Claude Code — Claude Opus 5 (1M context)
The primary attempt took run-agent.mjs's 50-minute default while its step capped at 80, so a third of that step was unreachable and every "ran out of time" round ended for a reason this file never named. Raising it surfaced the larger problem: the budget was never checked as a whole. Measured on one scan (run 30646547838): setup 5-7m in earlier steps, so it never competes with the agent step; Triage and address 50m03s on #8005 round 9 (its own timer) and 12m45s on #8211; the Verification gate 22m48s on #8211 — the largest consumer in the job, and unbounded; push, report and finalize 3-4s. The old arithmetic ("80 + 20 leaves ~50 for setup, two verification passes, and reporting") predates that measurement: the real worst case was 7 + 80 + 23 + 20 + 23 = 153 against a 150-minute job, and a JOB timeout cancels the always() reporters, which is the silent round the design exists to prevent. Sized for the PRs that actually exhaust the budget (47 and 35 changed files): agent 120m under a 130m step cap, each verification gate bounded at 60m — 2.6x the measured pass, and a graceful degrade because both gates already carry continue-on-error, so a bound turns a job kill into the ordinary verification-failure path that reports. Worst case 7 + 130 + 60 + 20 + 60 + 3 = 280 against a 300-minute job, itself under the 360-minute ceiling ubuntu-latest imposes regardless. Raising the budget does not make a doomed round cheaper — each exhausted budget still pushes nothing, which is why TIMEOUT_WINDOW_CAP stops a PR after three in one window. It buys rounds that were close to finishing. The test now asserts the SUM against the job cap and the count of bounded steps, because asserting the numbers individually is exactly what let 153-against-150 pass. Mutations checked: budget over its step cap, either verification bound removed, job cap back to 150, job cap over the runner ceiling, and continue-on-error dropped are each caught.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
— qwen3.8-max-preview via Qwen Code /review
| const mockChannelLoopSchedulerStop = vi.hoisted(() => vi.fn()); | ||
| const mockChannelLoopSchedulerMarkRecovery = vi.hoisted(() => vi.fn()); | ||
| const mockChannelLoopScheduler = vi.hoisted(() => |
There was a problem hiding this comment.
[Suggestion] mockChannelLoopSchedulerMarkRecovery is declared here and wired into the scheduler mock below (line 91), but it is never asserted in any recovery test. This leaves the scheduler?.markBridgeRecovery() call in createBridgeRecovery (start.ts:226) unverified at the integration level. — Concrete cost: if a future refactor removes that call, no test fails; recovery-aborted in-flight loop prompts would again be recorded as agent failures (incrementing consecutiveFailures, auto-disabling loops at maxConsecutiveFailures) — the exact bug the recovery epoch was added to prevent. The unit tests in ChannelLoopScheduler.test.ts verify markBridgeRecovery() in isolation, but nothing verifies start.ts invokes it during recovery.
Add an assertion to at least one recovery test (e.g. 'recovers a standalone bridge without reconnecting the channel adapter'), alongside the existing expect(mockChannelLoopSchedulerStop).not.toHaveBeenCalled():
expect(mockChannelLoopSchedulerMarkRecovery).toHaveBeenCalled();— qwen3.8-max-preview via Qwen Code /review
|
🤖 Addressed the latest review feedback (round 3/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 3/100 轮）。改动内容与我反驳保留之处如下： Review feedback addressedBlocker: two-timer race in suspension classification (round 2 #1, round 1 Blocker)Decision: Fixed.
Change: the low-CPU gap is now carried forward one tick via Files: Vacuous gate test (round 2 #3)Decision: Fixed.
Change: replaced with the Files: Assert
|
| Finding | Reason |
|---|---|
| Round 2 #2 — CPU ratio blind spot / liveness probe | Significant design addition (ACP round-trip liveness check after a filtered gap). The reviewer explicitly says it "could reasonably be a follow-up." Should be tracked as a separate issue. |
Round 2 #5 — registerChannelLoopMcpServer timeout |
The reviewer confirms "it can't hang today" because channelLoopMcpServer is null until registerChannelLoopToolHandler runs, which is after start(). Defense for an impossible case per Simplicity First. |
| Round 2 #7 — daemon telemetry reset on wake | Observation about a behaviour change the daemon opted into by omission. The reviewer says "probably desirable." Not worth the diff growth; can be documented in the PR body if the maintainer wants. |
Round 1 Medium #1 — ACP_START_TIMEOUT_MS terminal exit |
"Not a regression" per the reviewer — the old handler did the same. Pre-existing design issue, out of scope. |
Round 1 Medium #2 — coalescing during start() |
Edge case where the replacement dies during await bridge.start(). Not flagged as required; the .catch → process.exit(1) path is pre-existing. |
Round 1 Minor — completedRecovery guard unreachable |
Harmless loop-termination guard. Removing it saves nothing and the defensive check protects against a future caller that doesn't clear the promise. |
| Round 1 Minor — interval always created / JSDoc | Both in-repo consumers pass onNewMaxStall. No live regression. Not worth the diff. |
Verification
npm run build— passednpm run typecheck— passednpm run lint— passedvitest packages/core event-loop-lag.test.ts— 16 passed (15 existing + 1 new regression test)vitest packages/channels/base ChannelLoopScheduler.test.ts— 25 passedvitest packages/channels/base AcpBridge.test.ts— 36 passedvitest packages/channels/base ChannelBase.test.ts— 559 passedvitest packages/cli commands/channel/start.test.ts— 31 passedvitest packages/cli acp-integration/acpAgent.test.ts— 353 passed
中文说明
已处理的审查反馈
阻塞项：挂起分类中的双定时器竞态（第二轮 #1，第一轮阻塞项）
决定： 已修复。
checkHistogram() 要求在同一个 tick 上同时满足 elapsedMs >= suspendThresholdMs 和 newMaxMs >= suspendThresholdMs，但这两个值来自两个独立的 libuv 定时器，其触发顺序不固定。主机唤醒后，间隙在约 50–80% 的情况下晚一个 tick 才进入直方图，导致唤醒间隙被报告为卡顿，健康子进程被 SIGKILL——这正是本 PR 要防止的结果。
修改： 通过 pendingSuspendGapMs 将低 CPU 间隙向前携带一个 tick。当 elapsedMs >= suspendThresholdMs 且 CPU 使用率低时，锁定间隙值；下一个 tick 用携带的值来匹配迟到的直方图最大值。newMaxMs <= suspendGapMs * 1.5 的合理性检查防止抑制远大于携带间隙的真实卡顿。新增了一个回归测试，在时钟跳变之后的 tick 上提升 histogram.max——在旧代码上失败，在修复后通过。
文件：event-loop-lag.ts、event-loop-lag.test.ts
空门测试（第二轮 #3）
决定： 已修复。
rechecks bridge recovery after inbound preprocessing has started 使用了否定断言的 vi.waitFor（首次轮询即成功，不起同步作用）和 await Promise.resolve()（单个微任务，不足以让无门控流程到达 bridge.prompt）。无论门控是否存在，两个断言都通过。
修改： 替换为其他八个恢复门控测试使用的 await expect(vi.waitFor(...)).rejects.toThrow() 模式。
文件：ChannelBase.test.ts
在 start.test.ts 中断言 markBridgeRecovery（行内评论 rc:3692617995）
决定： 已修复。
mockChannelLoopSchedulerMarkRecovery 已连接到调度器 mock 但从未被断言，导致 createBridgeRecovery 中的 scheduler?.markBridgeRecovery() 调用在集成层面未被验证。
修改： 在 recovers a standalone bridge without reconnecting the channel adapter 测试中添加了 expect(mockChannelLoopSchedulerMarkRecovery).toHaveBeenCalled()。
文件：start.test.ts
恢复期间中止的故障无处记录（第二轮 #4）
决定： 已修复。
与恢复同时发生的真实故障（模型错误、提示超时）不会留下 lastStatus、lastError 或 stderr 痕迹——这正是 #6329 所针对场景中的可调试性回退。
修改： 恢复纪元的 catch 路径现在会持久化 lastStatus: 'error'、lastError 和 lastFinishedAt，同时仍跳过 consecutiveFailures 递增。更新了现有测试以验证错误被记录但不写入失败计数。
文件：ChannelLoopScheduler.ts、ChannelLoopScheduler.test.ts
5 分钟阈值的三份拷贝（第二轮 #6，第一轮次要项）
决定： 已修复。
acpAgent.ts 重复了 5 分钟字面量并注释"匹配父通道桥的重启阈值"，而测试从 @qwen-code/channel-base 导入真实常量来断言它们匹配。
修改： acpAgent.ts 现在从 @qwen-code/channel-base 导入 ACP_EVENT_LOOP_STALL_RESTART_MS（依赖已存在于 package.json）。移除了本地常量及其注释。同时移除了 core/telemetry/index.ts 中无消费者的 DEFAULT_EVENT_LOOP_SUSPEND_THRESHOLD_MS 再导出；常量本身保留在 event-loop-lag.ts 中作为默认值。
文件：acpAgent.ts、core/telemetry/index.ts
不变量注释夸大了代码（第二轮 #8，第一轮次要项）
决定： 已修复。
注释说"每个解析会话或调用桥的路径"，但 respondToPermission、cancelSession 和 availableCommands 没有门控（它们是良性的——位于入口门控下游或响应来自当前桥的事件）。
修改： 缩小为"会话解析和提示捕获路径"。
文件：ChannelBase.ts
已拒绝
| 发现 | 原因 |
|---|---|
| 第二轮 #2 — CPU 比率盲点 / 存活性探测 | 重大设计添加（过滤间隙后的 ACP 往返存活性检查）。审查者明确表示"可以作为后续工作"。应作为单独 issue 跟踪。 |
第二轮 #5 — registerChannelLoopMcpServer 超时 |
审查者确认"目前不会挂起"，因为 channelLoopMcpServer 在 registerChannelLoopToolHandler 运行前为 null，而后者在 start() 之后。按简洁优先原则，这是对不可能情况的防御。 |
| 第二轮 #7 — 守护进程遥测在唤醒时重置 | 关于守护进程因省略而选择的行为变更的观察。审查者说"可能是期望的"。不值得增加 diff；如维护者需要可在 PR 正文中说明。 |
第一轮中等 #1 — ACP_START_TIMEOUT_MS 终端退出 |
审查者说"不是回退"——旧处理器也是如此。预先存在的设计问题，超出范围。 |
第一轮中等 #2 — start() 期间的合并 |
替换桥在 await bridge.start() 期间死亡的边缘情况。未被标记为必需；.catch → process.exit(1) 路径是预先存在的。 |
第一轮次要 — completedRecovery 守卫不可达 |
无害的循环终止守卫。移除它不会节省任何东西，防御性检查保护了未来不清除 promise 的调用者。 |
| 第一轮次要 — 间隔始终创建 / JSDoc | 两个仓库内消费者都传递了 onNewMaxStall。无实际回退。不值得增加 diff。 |
验证
npm run build— 通过npm run typecheck— 通过npm run lint— 通过vitest packages/core event-loop-lag.test.ts— 16 通过（15 个现有 + 1 个新回归测试）vitest packages/channels/base ChannelLoopScheduler.test.ts— 25 通过vitest packages/channels/base AcpBridge.test.ts— 36 通过vitest packages/channels/base ChannelBase.test.ts— 559 通过vitest packages/cli commands/channel/start.test.ts— 31 通过vitest packages/cli acp-integration/acpAgent.test.ts— 353 通过
Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。
Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. · 有空请复审；第 100 轮后本 bot 停止并将 PR 交给人工。
🧠 Handled by Qwen Code · model/模型 qwen3.8-max-preview
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
No issues found. LGTM! ✅
— qwen3.8-max-preview via Qwen Code /review
Local verification — real channel runtime, before vs afterI built both revisions locally and ran the same failure scenarios against a real
1. Host suspend — the failure this PR is about
Baseline emitted 2. ACP bridge death with traffic in the recovery window
With this PR the message was held and answered ( 3. An active stall must still be killable310 s of blocked-but-burning event loop against the built monitor, with the ACP agent's own options ( The suspension filter does not swallow the case it must not swallow. A 2 s stall on a warm loop is still reported (2001.7 ms here vs 2013.3 ms on the baseline), so ordinary stall telemetry is unchanged. 4. Scheduled loop in flight when the bridge diesA cron loop was 1.5 s into its prompt when the ACP child was killed.
Neither revision auto-disables the loop for this abort shape — the prompt resolves empty rather than throwing, so the new 5. Focused tests and static checks
Notes for the merge decision
Verdict: the headline failure reproduces on the baseline and is gone on this PR, the recovery path behaves as described, and I found no regression in the paths I exercised. 中文版本（点击展开）本地真实环境验证 —— 改动前后对比我在本地分别构建了 PR 版本与基线版本，用同一组故障场景跑真实的
场景 1：基线打印 场景 2：本 PR 中窗口内的消息被挂起并在替换桥就绪后回答（ 场景 3：以 ACP agent 相同参数（ 场景 4：两个版本都不会因这次中止而自动禁用循环——该场景下 prompt 是"返回空结果"而非抛错，所以真正生效的不是新增的 场景 5：覆盖改动代码的 7 个测试文件共 1117 条用例全部通过； 合并前值得注意的几点
结论：目标缺陷在基线上可稳定复现，在本 PR 上消失；恢复路径的行为与描述一致；在我覆盖到的路径上没有发现回归。 |
…oom (QwenLM#8257) * fix(autofix): budget the whole round, not just the agent step The primary attempt took run-agent.mjs's 50-minute default while its step capped at 80, so a third of that step was unreachable and every "ran out of time" round ended for a reason this file never named. Raising it surfaced the larger problem: the budget was never checked as a whole. Measured on one scan (run 30646547838): setup 5-7m in earlier steps, so it never competes with the agent step; Triage and address 50m03s on QwenLM#8005 round 9 (its own timer) and 12m45s on QwenLM#8211; the Verification gate 22m48s on QwenLM#8211 — the largest consumer in the job, and unbounded; push, report and finalize 3-4s. The old arithmetic ("80 + 20 leaves ~50 for setup, two verification passes, and reporting") predates that measurement: the real worst case was 7 + 80 + 23 + 20 + 23 = 153 against a 150-minute job, and a JOB timeout cancels the always() reporters, which is the silent round the design exists to prevent. Sized for the PRs that actually exhaust the budget (47 and 35 changed files): agent 120m under a 130m step cap, each verification gate bounded at 60m — 2.6x the measured pass, and a graceful degrade because both gates already carry continue-on-error, so a bound turns a job kill into the ordinary verification-failure path that reports. Worst case 7 + 130 + 60 + 20 + 60 + 3 = 280 against a 300-minute job, itself under the 360-minute ceiling ubuntu-latest imposes regardless. Raising the budget does not make a doomed round cheaper — each exhausted budget still pushes nothing, which is why TIMEOUT_WINDOW_CAP stops a PR after three in one window. It buys rounds that were close to finishing. The test now asserts the SUM against the job cap and the count of bounded steps, because asserting the numbers individually is exactly what let 153-against-150 pass. Mutations checked: budget over its step cap, either verification bound removed, job cap back to 150, job cap over the runner ceiling, and continue-on-error dropped are each caught. * fix(autofix): bound every long step and cap the timeout override (QwenLM#8257) * fix(autofix): enforce the timeout ceiling and trim review feedback (QwenLM#8257) * fix(autofix): force base-10 clamp and align stale bound with job cap (QwenLM#8257) * fix(autofix): close the clamp int64 escape and pin it with a bash replay (QwenLM#8257) * fix(autofix): give the timeout clamp a floor, not only a ceiling The review's Finding 1: the guard clamped only the ceiling, and the uncovered side is the likelier typo. Every comment in this file, the PR body and the operator message speak in MINUTES; this one variable wants MILLISECONDS. A maintainer told to "raise the agent time budget" who sets QWEN_AUTOFIX_TIMEOUT_MS=120 armed a 120 ms timer — every round SIGTERMs instantly, writes agent-timeout, and reports "ran out of time (timeout (120ms))" until TIMEOUT_WINDOW_CAP trips and AutoFix stops on the PR, advising the human to raise the budget they just raised. No warning anywhere in that loop, which is the exact misreport the clamp exists to prevent, reached from the other direction. A 60000 ms floor rejects every minutes-shaped value, and it also closes the `0`/`000` hole the review noted alongside it — those passed the bare regex while the message asserted the value had to be positive. The message now names the units, because a units confusion is the whole failure mode. Replayed the review's own table against the extracted block, stdout and stderr separated: 7200000, 3600000 and the floor itself pass untouched; 120, 60, 0, 000 and 59999 all clamp with a warning, alongside the over-cap, malformed, octal and int64 cases the previous round closed. The test pins both boundaries from each side (59999 clamps, 60001 does not) and asserts the warning names MILLISECONDS. --------- Co-authored-by: verify <verify@local> Co-authored-by: qwen-code-dev-bot <qwen-code-dev@service.alibaba.com> Co-authored-by: Qwen Autofix <qwen-autofix@users.noreply.github.com>
|
Released in v0.21.3. |
TLDR
Recover channel ACP bridges that become unusable after a long host sleep or an active event-loop stall, without reconnecting the messaging adapter.
Before this change, the channel process could stay online while its ACP child was wedged or disconnected, so inbound messages and scheduled work could remain queued against a dead bridge. After this change, the channel start paths rebuild only the ACP bridge, preserve channel connectivity, and hold inbound, loop, and webhook prompt boundaries until the replacement bridge is ready.
What this changes
Design Consideration
Recovery is scoped to the ACP bridge. Reconnecting the channel adapter would duplicate platform connections and can lose adapter-owned state; the 99% path is that the messaging connection remains healthy while only the local ACP child needs replacement.
The readiness gate stays blocked across chained recovery attempts and is released only after a replacement bridge initializes successfully or recovery stops after a terminal failure.
Reviewer Test Plan
Focused result: 729 tests passed. Static checks and full build passed.
Linked issues / bugs
Follow-up to #6329. That issue captured the wedged-but-alive ACP bridge failure; this PR also handles host wake without misclassifying suspension as an active stall.