Skip to content

fix(daemon): drain recycled runtime generations - #11273

Merged
yiliang114 merged 7 commits into
codex/issue-8586-agent-watchdogfrom
codex/issue-8586-runtime-generations
Sep 7, 2026
Merged

fix(daemon): drain recycled runtime generations#11273
yiliang114 merged 7 commits into
codex/issue-8586-agent-watchdogfrom
codex/issue-8586-runtime-generations

Conversation

@yiliang114

Copy link
Copy Markdown
Collaborator

What this PR does

This PR adds the reusable runtime-generation layer for background-Agent recovery. ACP child generations now move explicitly through active, draining, and dying states. A recycle request drains the generation that owns the requesting Session, keeps every existing Session pinned to that owner, and prepares a fresh active generation for new work. The bridge permits at most two OS-live generations; when both slots are occupied and no active generation is available, REST and ACP callers receive a retryable 503 runtime_recycling response.

Why it is needed

A child can remain physically alive after it becomes unsafe for fresh work. The previous bridge had enough pieces to retire a channel and remember Session ownership, but it represented only an implicit alive/dying overlap and merely warned if a third child appeared. That was insufficient for the non-cooperative Agent escalation planned in the next layer.

Reviewer Test Plan

How to verify

Review the three lifecycle transitions: fresh work uses the active generation; a recycle request leaves existing Sessions on their recorded generation while a replacement becomes active; and a second recycle while an older generation is still alive cannot create a third child and returns runtime_recycling. Also confirm that a draining generation remains covered by channel liveness and active-work reporting until it exits.

Evidence (Before & After)

N/A — daemon lifecycle behavior with no UI change.

Tested on

OS Status
🍏 macOS N/A
🪟 Windows N/A
🐧 Linux N/A

No local test, build, typecheck, or CI command was run. The changed files were formatted, git diff --check passed, and the complete diff received two clean static review passes.

Risk & Scope

  • Main risk or tradeoff: a draining generation can keep one of the two physical slots until its final Session exits; this is deliberate fail-closed behavior.
  • Not validated / out of scope: detecting an Agent that ignores cooperative abort and sending the protected record-only terminal notification remain in the next stacked PR.
  • Breaking changes / migration notes: none; Session persistence and public timeout configuration are unchanged.

Linked Issues

Part of #8586.

Depends on #11270.

中文说明

这个 PR 做了什么

这个 PR 为后台 Agent 恢复增加了可复用的 runtime generation 层。ACP 子进程 generation 现在显式经过 active、draining 和 dying 三个状态。recycle 请求只 drain 发起请求的 Session 所属 generation,现有 Session 继续固定路由到原 owner,新工作则使用新的 active generation。bridge 最多允许两个 OS 层仍存活的 generation;如果两个槽位都被占用且没有 active generation,REST 和 ACP 调用会收到可重试的 503 runtime_recycling

为什么需要

子进程可能仍然物理存活,但已经不适合接收新工作。之前 bridge 已经具备延迟退役 channel 和记录 Session owner 的零件,但只有隐式的 alive/dying 重叠,而且第三个子进程出现时只报警,无法支撑下一层的非协作 Agent 升级恢复。

Reviewer Test Plan

如何验证

检查三个生命周期转换:新工作只使用 active generation;recycle 后现有 Session 仍走原 generation,同时新的 generation 成为 active;当旧 generation 尚未退出又触发第二次 recycle 时,不会创建第三个子进程,而是返回 runtime_recycling。同时确认 draining generation 在退出前仍受 channel liveness 和 active-work reporting 覆盖。

前后证据

N/A——这是 daemon 生命周期行为变更,没有 UI 变化。

验证环境

三个系统均为 N/A。没有运行本地 test、build、typecheck 或 CI 命令;仅格式化改动文件、通过 git diff --check,并对完整 diff 完成两遍干净的静态审查。

风险与范围

  • 主要风险或取舍:draining generation 会一直占用两个物理槽位中的一个,直到其最后一个 Session 退出;这是刻意的 fail-closed 行为。
  • 未验证 / 范围外:识别不响应协作 abort 的 Agent,以及发送受保护的 record-only 终态通知,留在下一个堆叠 PR。
  • 破坏性变更 / 迁移说明:无;Session 持久化和公开超时配置均未变化。

关联 Issue

属于 #8586 的一部分。

依赖 #11270

@qwen-code-ci-bot

qwen-code-ci-bot commented Sep 7, 2026

Copy link
Copy Markdown
Collaborator

Qwen Triage finishedview run. See the stage comments in this thread for the result.

Qwen Triage 已完成 —— 查看运行。结果见本线程中的各阶段评论。

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Thanks — this is layer 4 of #8586, and it lines up with what that tracker asks for.

Template looks good ✓ (every required section is there plus the Chinese half; only nit is the heading reads "Why it is needed" where the template says "Why it's needed" — not worth a re-push).

Problem: planned rather than observed. #8586 is open on the background-automation roadmap and prescribes this layer almost word for word — explicit active/draining/dying generations, Session ownership preserved, no third live generation, 503 runtime_recycling while capacity is unavailable. So the design isn't invented here, it's implemented to spec. What's missing is any evidence of the failure being hit: the PR says plainly that no local test, build, typecheck or CI command was run, and there's no before/after. That's acceptable for a maintainer-authored roadmap layer, but it does mean nothing in this PR demonstrates the draining path working.

Direction: aligned. Capping OS-live generations at two and failing closed on admission is the right shape for the layer-5 escalation that follows, and it's what the tracker's acceptance criteria demand ("never creates more than two live generations").

Size: cross-package (packages/acp-bridge + packages/cli), so Stage 0 applies. 106 production lines — 63 in the bridge, 14 in the RPC dispatcher, 11 each in the error class and the REST error mapper, 6 in the bridge interface, 1 in the re-export — plus 13 lines of design doc, and 0 test lines. Well under any threshold, and you're an admin on this repo so the two-tier gate doesn't apply to you anyway. Calling the test count out because it matters more than the size here.

Approach: focused, and I like that it resists adding public timeout config or a persistence migration, which the tracker explicitly rules out. Two observations rather than objections. The title says fix(daemon) but this adds a new lifecycle state, a new bridge method, a new error class and a new 503 — that's feat-shaped, and the type is what the core-module gate keys off. And requestRuntimeRecycle has no caller anywhere in the tree yet; the design doc says layer 5 wires it, which is fine for a stack, just noting the entry point is unreachable at this merge point.

Risk: no high-risk-path matches. The real risk is the CI hole — because the base is codex/issue-8586-agent-watchdog and ci.yml only triggers pull_request for main and release/**, the test / lint_and_static jobs never run on this PR. The only PR CI is tui-parity. So this lands with no automated signal at all behind a behavioural daemon-lifecycle change. Worth knowing before it retargets at main, where the suite will finally see it.

Moving on to code review. 🔍

中文说明

感谢贡献——这是 #8586 的第 4 层,和 tracker 的要求基本一致。

模板完整 ✓(所有必需章节加中文部分都在;唯一的小问题是标题写成 "Why it is needed",模板是 "Why it's needed"——不值得为此重推一次)。

问题:属于"计划中"而非"已观测"。#8586 仍是 open 状态,在 background-automation roadmap 上,几乎逐字规定了这一层的内容——显式的 active/draining/dying generation、保留 Session 归属、不允许第三个 live generation、容量不可用时返回 503 runtime_recycling。所以设计不是这里发明的,而是按规格实现。缺的是任何命中该故障的证据:PR 明确写了本地没有运行 test、build、typecheck 或 CI 命令,也没有 before/after。对于 maintainer 自己推进的 roadmap 分层这是可以接受的,但确实意味着本 PR 里没有任何东西证明 draining 路径可用。

方向:一致。把 OS 层存活的 generation 上限设为 2、并在准入时 fail closed,对紧随其后的第 5 层升级来说是正确的形态,也正是 tracker 验收标准要求的("never creates more than two live generations")。

规模:跨包改动(packages/acp-bridge + packages/cli),因此触发 Stage 0。生产代码 106 行——bridge 63 行、RPC dispatcher 14 行、error 类与 REST error 映射各 11 行、bridge 接口 6 行、re-export 1 行——外加 13 行设计文档,以及 0 行测试。远低于任何阈值;而且你是本仓库 admin,两级门禁对你本就不适用。之所以强调测试行数为 0,是因为它比规模更关键。

方案:聚焦,也很克制地没有引入公开超时配置或持久化迁移(tracker 明确排除这两项)。两点观察而非反对意见:标题写 fix(daemon),但本 PR 新增了生命周期状态、新的 bridge 方法、新的 error 类和新的 503——形态更像 feat,而核心模块门禁正是按类型判断的。另外 requestRuntimeRecycle 目前在整个代码树里没有任何调用方;设计文档说第 5 层会接上,对于堆叠 PR 没问题,只是说明在这个合并点上该入口不可达。

风险:没有命中高风险路径。真正的风险是 CI 空洞——因为 base 是 codex/issue-8586-agent-watchdog,而 ci.ymlpull_request 只对 mainrelease/** 触发,所以 test / lint_and_static 这两个 job 在本 PR 上根本不会运行,唯一的 PR CI 是 tui-parity。也就是说这个行为性的 daemon 生命周期改动落地时完全没有自动化信号。等它重新指向 main 时测试套件才会第一次看到它,这点值得提前知道。

进入代码审查 🔍

Qwen Code · qwen3.8-max-2026-09-02

Reviewed at 6e6639d4f7f1263106dff1e2a5321bbf00e78947 · re-run with @qwen-code /triage

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Read against 6e6639d with the base branch's source pulled for context. The core of this is sound — the two same-generation binding fixes are the part I'd have missed, and they're right.

What I'd want answered before merge: isChannelLive() now means something weaker than its consumers assume.

It went from !!liveChannelInfo() (the single attach-available generation, i.e. active) to "any OS-alive generation that isn't dying" — which includes a draining generation that by definition cannot accept fresh work. Meanwhile liveChannelInfo() still means active-only, and getWorkspaceRuntimeLifecycleSnapshot() still derives runtimeLive from it. So the bridge now carries two contradictory notions of "live" at once.

They diverge in a state this PR makes reachable: generation A draining with sessions still attached, generation B dying (transport failure or a kill of the replacement). In that state isChannelLive() returns true, the snapshot returns runtimeLive: false / state: 'stopping', and ensureChannel() throws BridgeRuntimeRecyclingError because both slots are held.

The consumer that turns this into a wrong answer is preheat. bridge.preheat() awaits ensureChannel(), so it rejects with runtime_recycling; preheatAcpChild catches it, and its catch branch re-reads channelLive() — which is now true because of the draining generation — and returns { ready: true, channelLive: true } (packages/cli/src/serve/workspace-service/index.ts, ~line 519-521). Preheat reports ready while new-session admission is 503ing. To be fair about the before-state: pre-diff this situation produced no 503 at all, because ensureChannel() went ahead and spawned a third child — precisely what the two-slot cap exists to prevent. So the new failure mode isn't the 503, it's that the 503 gets swallowed into a ready: true on its way out. The same loosened flag also drives getWorkspaceAcpStatus, the env/preflight status cells, and the skill-toggle activation: 'applied' | 'deferred' decision — a skills refresh during a draining-only window claims 'applied' and routes at whatever invokeWorkspaceCommand picks, not necessarily at a generation that will serve the next session.

So: is "live" meant to be "a non-dying generation exists" or "a generation can admit fresh work"? Both readings are defensible, and the design doc argues for the first one for liveness/active-work reporting. But the admission-side consumers need the second. Either split the two notions or narrow isChannelLive() back to active-only and give the liveness reporting its own predicate.

Two smaller things.

requestRuntimeRecycle drains the owner and then awaits ensureChannel() — which can throw the 503. So a recycle can fail after its side effect has landed, leaving the owner draining with no replacement and the caller holding a runtime_recycling rejection. Layer 5 will call this from a child-to-daemon request; is throwing the intended contract there, or should the caller be told "drained, replacement deferred"?

The isDying accessor shim keeps the diff small in a 15k-line file, which I appreciate, but set isDying(value) silently swallows false — only true moves state. All eleven set-sites assign true today so it's correct as written; it's just that a future ci.isDying = false is a no-op with no error. Getter-only plus explicit state = 'dying' assignments would remove the trap.

What's good, specifically: replacing channelInfo?.sessionIds === sessionIds with infoRef.current?.sessionSpawnsInFlight and channelInfo.sessionIds.size > 1 with sessionIds.size > 1 fixes two latent cross-generation bugs — both compared against the global newest channel, which stops being the channel under construction the moment two generations coexist. Dropping channelInfo === info from the liveness isActive predicate is consistent with keeping a draining generation covered until it exits. The error plumbing is complete end to end: class → value re-export → RPC mapping and REST mapper, both 503 with retryable: true and errorKind set, which is what a caller needs to back off correctly.

sequenceDiagram
    participant P1 as Caller
    participant P2 as AcpSessionBridge
    participant P3 as Generation A draining
    participant P4 as Generation B active
    P1->>P2: requestRuntimeRecycle sessionId
    P2->>P3: state draining, session owners kept
    P2->>P4: ensureChannel spawns replacement
    P1->>P2: new session work
    P2->>P4: routed to the active generation
    P3-->>P2: existing sessions still route here
    P1->>P2: recycle again while A and B are both alive
    P2-->>P1: 503 runtime_recycling, retryable
Loading

Test evidence

Stating plainly what this section carries: the PR's own CI, read through the API — and there is almost none of it. I ran no build, test or typecheck myself (this is an unattended CI run, and triage never executes PR-derived code).

ci.yml triggers pull_request only for base main and release/**. This PR's base is codex/issue-8586-agent-watchdog, so test, lint_and_static and classify_pr never ran — they are absent from the head commit's check-runs entirely, not failing, not pending. The only PR CI is tui-parity, still in progress at review time. The remaining checks are bot orchestration. Per the workflow rule I fetched this once and did not poll.

Check Conclusion
TUI parity snapshots (ink vs opentui) in_progress
OpenTUI no-flicker gate success
authorize success
label / assign / delay-automatic-review success
verify skipped
tmux-testing skipped
precheck-pr / resolve-pr / review-config skipped
publish-verify / publish-tmux / publish-resolution skipped
ack-review-request skipped
triage / review-pr in_progress (bot orchestration)
test, lint_and_static, classify_pr not triggered — base branch excluded from ci.yml

Not verified, and the reasons: the draining → replacement → admission sequence (no test in the diff — 0 test lines — and no CI lane that would run one); the 503 path and its retryable contract at the REST and RPC edges; the isChannelLive() divergence above. The only existing isChannelLive() assertion in bridge.test.ts (~line 34803) covers the preheat-active case and is unaffected either way, so nothing in the suite pins the old or the new semantics. The author's own report — no local test, build or typecheck run — is their statement, not evidence I re-ran.

Sandboxed verification would settle this: @qwen-code /verify — that a draining generation keeps routing its existing Sessions while fresh work lands on the replacement, and that a second recycle with both slots held returns 503 runtime_recycling instead of spawning a third child, is not observable from the diff, and no test in this PR pins any of it. You have write access, so @qwen-code /tmux is available too, though the surface here is the daemon REST edge rather than the TUI, so /verify is the one that would actually pay off. A preheat call made while a generation drains is the specific probe worth adding.

中文说明

基于 6e6639d 审查,并拉取了 base 分支源码作为上下文。核心实现是可靠的——两处"绑定到本 generation"的修正是我自己可能会漏掉的,而且改得对。

合并前我希望先有答案的问题:isChannelLive() 的含义现在比它的调用方所假设的更弱。

它从 !!liveChannelInfo()(唯一可 attach 的 generation,即 active)变成了"任何 OS 层存活且非 dying 的 generation"——这就把 draining 的 generation 也算了进去,而 draining 按定义不能接收新工作。同时 liveChannelInfo() 仍然只表示 active,getWorkspaceRuntimeLifecycleSnapshot()runtimeLive 也仍由它推导。于是 bridge 内部同时存在两种互相矛盾的 "live" 定义。

在本 PR 使其可达的状态下两者会分叉:generation A 处于 draining 且仍挂着 session,generation B 处于 dying(替代 generation 发生 transport failure 或被 kill)。此时 isChannelLive() 返回 true,snapshot 返回 runtimeLive: false / state: 'stopping',而 ensureChannel() 因为两个槽位都被占用而抛出 BridgeRuntimeRecyclingError

把这个矛盾变成错误答案的调用方是 preheat。bridge.preheat() 会 await ensureChannel(),因此以 runtime_recycling 拒绝;preheatAcpChild 捕获它,而它的 catch 分支重新读取 channelLive()——由于那个 draining generation 而返回 true——于是返回 { ready: true, channelLive: true }packages/cli/src/serve/workspace-service/index.ts 约 519-521 行)。也就是说新 session 准入正在返回 503,preheat 却报告 ready。关于改动前的状态需要说清楚:改动前这种情况根本不会产生 503,因为 ensureChannel() 会直接 spawn 第三个子进程——而这正是两槽位上限要防止的。所以新的故障模式不是 503 本身,而是这个 503 在向外传递时被吞掉、变成了 ready: true。这个被放宽的标志同样驱动 getWorkspaceAcpStatus、env/preflight 状态单元,以及技能开关的 activation: 'applied' | 'deferred' 判断——在只有 draining generation 的窗口里做技能刷新会声称 'applied',并路由到 invokeWorkspaceCommand 选中的任意对象,而不一定是会服务下一个 session 的那个 generation。

所以想确认:"live" 的含义是"存在一个非 dying 的 generation",还是"存在一个可以接收新工作的 generation"?两种解读都说得通,设计文档也主张在 liveness / active-work 上报上采用第一种。但准入侧的调用方需要第二种。要么把两个概念拆开,要么把 isChannelLive() 收回为仅 active,并给 liveness 上报单独一个判定函数。

另外两个较小的问题。

requestRuntimeRecycle 先 drain owner,然后才 await ensureChannel()——而后者可能抛出 503。因此一次 recycle 可能在副作用已经生效之后失败:owner 停在 draining 且没有替代 generation,调用方拿到一个 runtime_recycling 拒绝。第 5 层会从 child→daemon 的请求调用它;那里预期的契约是抛错,还是应告知调用方"已 drain,替代 generation 延后"?

isDying 访问器垫片在一个 15k 行的文件里确实让 diff 变小,这点我很认可,但 set isDying(value) 会静默吞掉 false——只有 true 会改变 state。目前全部 11 处赋值都是 true,所以按现状是正确的;只是将来某处写 ci.isDying = false 会变成无报错的空操作。改成只读 getter 加显式的 state = 'dying' 赋值可以消除这个陷阱。

明确说好的部分:channelInfo?.sessionIds === sessionIds 换成 infoRef.current?.sessionSpawnsInFlight、把 channelInfo.sessionIds.size > 1 换成 sessionIds.size > 1,修掉了两个潜在的跨 generation bug——两处原本都拿全局最新的 channel 作比较,而一旦两个 generation 并存,它就不再是正在构造的那个 channel。从 liveness 的 isActive 判定中去掉 channelInfo === info,与"draining generation 在退出前仍被覆盖"是一致的。错误链路端到端完整:error 类 → 值 re-export → RPC 映射与 REST 映射,两者都返回 503 并设置 retryable: trueerrorKind,这正是调用方正确退避所需要的信息。

测试证据

明确说明本节携带的内容:PR 自身的 CI,通过 API 读取——而且几乎没有。 我没有自己运行任何 build、test 或 typecheck(这是无人值守的 CI 运行,且 triage 从不执行 PR 派生的代码)。

ci.ymlpull_request 只对 base 为 mainrelease/** 触发。本 PR 的 base 是 codex/issue-8586-agent-watchdog,因此 testlint_and_staticclassify_pr 根本没有运行——它们在 head commit 的 check-runs 里完全不存在,不是失败也不是待定。唯一的 PR CI 是 tui-parity,审查时仍在进行中。其余都是 bot 编排 job。按流程规则我只取了一次结果,没有轮询等待。

未验证项及原因:draining → 替代 generation → 准入 这一整条链路(diff 中 0 行测试,也没有会运行测试的 CI 通道);503 路径及其在 REST 与 RPC 边界的 retryable 契约;以及上面提到的 isChannelLive() 分叉。bridge.test.ts 中唯一现存的 isChannelLive() 断言(约 34803 行)覆盖的是 preheat-active 场景,两种语义下都不受影响,因此测试套件里没有任何东西钉住旧语义或新语义。作者自述未运行本地 test、build、typecheck——那是作者的声明,不是我复跑的证据。

沙箱验证可以定论:@qwen-code /verify——"draining generation 仍继续路由其既有 Session,同时新工作落到替代 generation 上",以及"两个槽位都被占用时第二次 recycle 返回 503 runtime_recycling 而不是 spawn 第三个子进程",这两点无法从 diff 观察,本 PR 也没有任何测试钉住它们。你有写权限,因此 @qwen-code /tmux 也可用,但这里的界面是 daemon REST 边界而非 TUI,所以真正有价值的是 /verify。值得专门加的一个探针是:在某个 generation 正在 drain 时发起一次 preheat 调用。

Qwen Code · qwen3.8-max-2026-09-02

Reviewed at 6e6639d4f7f1263106dff1e2a5321bbf00e78947 · re-run with @qwen-code /triage

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Confidence: 3/5 — the structure is right and matches the tracker's spec, but I found one reachable inconsistency in the readiness signal and there is no runtime evidence of any kind behind it, so this needs your eyes rather than my approval.

My independent proposal before comparing: explicit three-state generation, owner-pinned routing for existing Sessions, a hard cap on OS-live generations, retryable 503 on admission. That's what you built, so on structure we agree and I don't have a simpler path to offer — cutting 80% of this wouldn't solve the problem, since the cap and the drain are the point. Where I'd have written it differently is the two things in the review above: I'd have added a new predicate for "a non-dying generation exists" and left isChannelLive() meaning "can admit fresh work", rather than widening the existing one that six workspace-service call sites already read as an admission proxy; and I'd have migrated the eleven isDying = true sites to state = 'dying' instead of a write-only-true accessor. Neither is a rewrite, both are contained.

Did I verify the problem exists? Honestly — no, and neither did you. #8586 describes the failure mode in detail and prescribes this layer verbatim, which is stronger than a drive-by hardening PR, but it's a specification, not an observation. That's a legitimate way to build a roadmap stack and I'm not going to gate a maintainer's own tracked layer on a reproduction. I am noting that nothing here — not the PR, not CI, not a test — demonstrates the draining path actually working end to end.

The part that decides the verdict is the evidence gap. Zero test lines on a lifecycle change whose whole value is behavioural, and ci.yml structurally cannot run against this base branch, so test and lint_and_static never executed. When this retargets at main the suite will see it for the first time. I'm not going to approve a change I can only read; the preheat divergence in particular is exactly the kind of thing that shows up in one assertion and not in a diff read.

Six months from now: the state machine will read fine and the design doc earns its place. The isDying/state dual representation is the bit that will cost someone an afternoon, and the two-contradictory-"live"-definitions is the bit that will cost someone an incident — a controller that preheats, sees ready: true, and then eats a 503 on the session it was told it could create.

On volume: this is one of five stacked PRs (#11270 is still open beneath it). I judged this one on its own diff, and my hesitation is specific to isChannelLive(), not to the stack.

Not approving, not requesting changes — deferring to you as the roadmap owner. Two calls are yours: whether "live" should mean admit-capable or merely non-dying (and if the former, the preheat/skill-toggle consumers need the narrower one), and whether requestRuntimeRecycle should be able to fail with runtime_recycling after it has already drained its owner. If you'd rather settle either in layer 5, say so here so the deferral is on the record instead of silently dropped. @qwen-code /verify would close the behavioural gap in the meantime — a preheat issued while a generation drains is the probe I'd want.

The maintainer resolver found no area label on this PR and no prior human reviewer, so there's nobody to hand this to but you; I've not guessed a login or assigned it.

中文说明

Confidence: 3/5——整体结构是对的,也符合 tracker 的规格,但我发现了一处在可达状态下的就绪信号不一致,而且背后完全没有任何运行时证据,因此这需要你亲自过目,而不是由我批准。

我在对比之前的独立方案是:显式的三态 generation、既有 Session 按 owner 固定路由、对 OS 层存活 generation 数设硬上限、准入时返回可重试的 503。你实现的正是这个,所以结构上我们一致,我也没有更简的路径可提——砍掉其中 80% 并不能解决问题,因为上限和 drain 本身就是重点。我会写得不一样的地方就是上面审查里的那两点:我会新增一个"存在非 dying generation"的判定函数,而让 isChannelLive() 继续表示"可以接收新工作",而不是放宽这个已被 workspace-service 六处调用点当作准入代理来读的函数;我也会把 11 处 isDying = true 直接迁移成 state = 'dying',而不是用一个只对 true 生效的访问器。两者都不是重写,改动范围可控。

我是否验证了问题确实存在?老实说——没有,你也没有。#8586 详细描述了故障模式并逐字规定了这一层,这比随手加固的 PR 强得多,但它是一份规格说明,不是一次观测。对于推进自己的 roadmap 分层,这是合理的做法,我不会因为缺少复现就拦下 maintainer 自己跟踪中的分层。我要指出的是:这里没有任何东西——PR 本身、CI、测试——证明 draining 路径端到端确实可用。

决定结论的是证据缺口。一个价值完全在行为上的生命周期改动,测试行数为 0;而 ci.yml 在结构上无法对这个 base 分支运行,所以 testlint_and_static 从未执行。等它重新指向 main 时,测试套件才会第一次看到它。我不会批准一个我只能靠阅读判断的改动;尤其是 preheat 那处分叉,正是那种靠一条断言就能暴露、而读 diff 读不出来的问题。

六个月之后:状态机本身读起来没问题,设计文档也有其价值。isDying/state 双重表示会花掉某人一个下午,而"两种互相矛盾的 live 定义"会花掉某人一次故障——一个控制器 preheat 后看到 ready: true,然后在它被告知可以创建的 session 上吃到一个 503。

关于数量:这是五个堆叠 PR 之一(#11270 仍在它下面未合并)。我是按本 PR 自身的 diff 判断的,我的犹豫针对的是 isChannelLive(),不是整个 stack。

不批准,也不 request changes——转交给你这位 roadmap owner 决定。 两个判断由你来做:"live" 应该表示"可接收新工作"还是"仅非 dying"(如果是前者,preheat / 技能开关这些调用方需要更窄的那个);以及 requestRuntimeRecycle 是否应该在已经 drain 掉 owner 之后仍可能以 runtime_recycling 失败。如果你更倾向把任一项放到第 5 层解决,请在这里说明,让这个延后决定留痕,而不是被静默丢弃。同时 @qwen-code /verify 可以补上行为证据——我最想要的探针是:在某个 generation 正在 drain 时发起一次 preheat。

maintainer 解析器在本 PR 上找不到 area 标签,也没有此前的人工 reviewer,所以除了你之外无人可交;我没有猜测任何 login,也没有指派。

Qwen Code · qwen3.8-max-2026-09-02

Reviewed at 6e6639d4f7f1263106dff1e2a5321bbf00e78947 · re-run with @qwen-code /triage

@yiliang114

Copy link
Copy Markdown
Collaborator Author

Addressed the reachable readiness inconsistency in f25f29235b19: isChannelLive() again means an active generation that can accept fresh workspace/session work. Draining generations remain tracked internally for owner routing, active-work reporting, teardown, and the two-OS-live cap, but no longer make preheat or skill activation report ready.

The post-drain runtime_recycling rejection is intentional when both OS-live slots are occupied: draining the owner has taken effect, but the requested replacement has not been prepared, so the operation is incomplete and remains retryable rather than returning a false success. I left the isDying compatibility accessor unchanged because every current write assigns true; replacing all eleven sites would be non-functional churn in this PR.

The updated #11270 base was merged into this branch. No tests were added and no local test, build, typecheck, or CI command was run.

@yiliang114
yiliang114 merged commit 90244a6 into codex/issue-8586-agent-watchdog Sep 7, 2026
9 of 12 checks passed
@wenshao

wenshao commented Sep 7, 2026

Copy link
Copy Markdown
Collaborator

Local verification report — real daemon E2E

I built a real verification environment for this PR (I'm a maintainer; this is a merge-readiness check, not a drive-by review). Everything below comes from a real qwen serve daemon with real spawned qwen --acp children, driven over real HTTP/WebSocket — no mocked bridge. A/B arms are the PR head 6e6639d4f7 and its stated base codex/issue-8586-agent-watchdog @ 25df3993ab, each npm ci'd and bundled independently.

To reach the three lifecycle transitions the Reviewer Test Plan asks for, I froze an ACP child with SIGSTOP so a workspace-control call blows its deadline. That is a genuine, unmocked path into retireChannelAfterSessionsDrain — the same BridgeTimeoutError path the existing timeout retirement uses, and a faithful stand-in for the stalled runtime this stack is about.

Bottom line: the design works exactly as described, but the PR is currently red and I'd hold merge on one blocker.


✅ What reproduces (the PR's own test plan, all three transitions)

Claim Result
Fresh work uses the active generation
A recycle leaves existing Sessions on their recorded generation while a replacement becomes active ✅ old Session keeps prompting 202 on child A while child B serves new work
A second recycle cannot create a third child, and returns runtime_recycling ✅ exactly 2 children, 503 on the third
A draining generation stays covered by channel liveness and active-work reporting until it exits channelAlive: true; the active-work coverage grade does not degrade when a second generation appears (identical grade on both arms)
The draining generation is actually reaped ✅ exits ~250 ms after its last Session closes; admission recovers immediately, no third child

Both wire mappings are correct:

The base/head contrast in fig 1 is the real value of this PR: on base, the generation marked for retirement keeps absorbing fresh Sessions, so its retirement is deferred again for as long as new work arrives. On head it drains properly. That is a genuine fix.

Also green on head: npm run typecheck (exit 0) and npm run lint (exit 0). packages/cli src/serve is unaffected — 15 failures, identical on both arms (pre-existing local-environment noise, not this PR).


🔴 Blocker — packages/acp-bridge is red on head, green on base

base   Test Files  35 passed (35)          Tests  1959 passed (1959)
head   Test Files   1 failed | 34 passed   Tests     1 failed | 1958 passed
FAIL src/bridge.test.ts > extNotification — MCP budget events (PR 14b)
     > purges buffered guardrail events when restore fails so retry-success
       does not replay stale frames (codex round 7 fix)

BridgeRuntimeRecyclingError: The ACP runtime is recycling; retry after an older generation exits
 ❯ ensureChannel src/bridge.ts:4503:40
 ❯ restoreSession src/bridge.ts:8828:5
 ❯ Object.loadSession src/bridge.ts:9511:14

I instrumented ensureChannel to dump aliveChannels at the refusal:

[{"state":"dying","sessions":0}, {"state":"dying","sessions":0}]   channelInfo.state=dying

Both slots hold terminal children with zero work. Neither will ever accept a Session again, so refusing gains nothing — while main/base happily spawns the replacement. The scenario is not exotic: a failed session/load tears its channel down, the retry needs a fresh one, and the cap counts the corpses. The >= 2 guard is doing the right thing for draining generations and the wrong thing for dying ones.

Candidate fix — I applied it, rebuilt, and re-ran everything:

-    if (aliveChannels.size >= 2) throw new BridgeRuntimeRecyclingError();
+    if (
+      aliveChannels.size >= 2 &&
+      Array.from(aliveChannels).some((c) => c.state !== 'dying')
+    ) {
+      throw new BridgeRuntimeRecyclingError();
+    }

Result: 1959/1959 green, and every behaviour in fig 1 and fig 2 still reproduces unchanged on the real daemon — the cap still holds at two, the third POST /session still gets 503 runtime_recycling, existing Sessions still stay pinned. Take it or pick your own shape; the point is that the cap should not count generations that are already terminal.


🟠 Important — the refusal is mislabelled and over-logged on POST /workspace/runtime/ensure

ensureChannel is also reached through preheat(), and WorkspaceRuntimeCoordinator.ensure() wraps anything that is not WorkspaceRuntimeStillStartingError into WorkspaceRuntimeInitializationError. So the same condition reports two different things:

POST /session                   -> 503 {"code":"runtime_recycling","errorKind":"runtime_recycling","retryable":true}
POST /workspace/runtime/ensure  -> 503 {"code":"runtime_initialization_failed"}          <- no errorKind, no retryable

and in the daemon log, the same expected state is an ERROR with a full stack trace on one route and a clean WARN on the other, because WorkspaceRuntimeInitializationError goes through reportBridgeError while BridgeRuntimeRecyclingError goes through recordExpectedBridgeError:

[ERROR] route=POST /workspace/runtime/ensure The ACP runtime is recycling; retry after ...
  BridgeRuntimeRecyclingError: ...
      at ensureChannel (...)
      at Object.preheat (...)
      at WorkspaceRuntimeCoordinator.ensure (...)
[WARN]  route=POST /session errorType=BridgeRuntimeRecyclingError The ACP runtime is recycling ...

Suggest letting BridgeRuntimeRecyclingError through ensure() the way WorkspaceRuntimeStillStartingError already is.


🟠 Important — /workspace/runtime/status now says the runtime is stopping after one ordinary timeout

This is the most common path, not the two-generation corner. One workspace-control timeout, Session still open and still perfectly usable:

state runtimeLive children POST /session/<id>/prompt /health?deep channelAlive
base active true 1 202 true
head stopping false 1 202 true

liveChannelInfo() now returns undefined for a draining generation, so runtimeLive flips false while isChannelLive() (which the PR widened) keeps /health saying channelAlive: true. The two status surfaces disagree about the same runtime.

That matters beyond cosmetics: scheduleSkillsReconciliation / scheduleMcpReconciliation both bail with 'deferred' when !snapshot.runtimeLive, so config reconciliation silently no-ops between a drain and the next generation actually becoming active. Live, I saw the capability state go {state: "stale", runtimeEpoch: 1} against runtimeEpoch: 2 after the recycle.

And the refusal window is not transient — it is bounded only by how long the two Sessions live:

I understand the fail-closed tradeoff is deliberate and I'm not asking you to change it. I am asking that the state be reported honestly while it holds, since runtimeLive: false on a runtime that is serving two Sessions is a signal controllers will act on.


🟡 Minor

  • No Retry-After on the new 503. It is the only 503 in error-response.ts that declares retryable: true without one — init_timeout and acp_channel_unavailable both set it, and runtime_still_starting sets it without even claiming retryable. Confirmed on the wire (Retry-After present? false). Clients that honour the header will fall back to their default cadence.
  • runtime_recycling is undocumented. Every sibling code appears in docs/developers/qwen-serve-protocol.md and docs/users/qwen-serve.md; the natural home is the POST /session error list alongside the 503 acp_channel_unavailable bullet. Right now the code exists only in the new design note.
  • No tests. The diff adds a three-state machine, a new error class, a new public interface method and two new wire paths, and touches zero test files. grep finds no test referencing BridgeRuntimeRecyclingError or runtime_recycling. Given the blocker above was caught by an unrelated pre-existing test, the state machine itself is currently unguarded.
  • requestRuntimeRecycle is dead code in this PR — declaration plus implementation, zero callers until fix(daemon): recycle unresponsive agent runtimes #11275. That is a reasonable way to stack, but it means the recycle entry point ships unexercised.
  • The isDying accessor pair silently drops false. set isDying(v) { if (v) this.state = 'dying'; } makes ci.isDying = false a no-op. All 10 current assignments are = true, so there is no live bug — but it is a trap for the next person, and worth a comment at minimum.

Verdict

Recommend changes requested: fix the aliveChannels cap so the suite goes green, then this is good to merge. The other items are worth folding in while you are in here, but none of them alone would block me.

How to reproduce

Harness at /root/git/h11273, arms at /root/git/pr11273 (head) and /root/git/pr11273-base (25df3993ab).

bash run-arm.sh base p10-status-ab.mjs     # runtime status A/B
bash run-pr-full.sh                        # lifecycle + 503 refusal on head
bash run-arm.sh pr   p4-drain-completes.mjs  # drained generation is reaped, slot recovers
bash run-arm.sh pr   p6-acp-ws.mjs           # ACP /acp WebSocket error mapping
bash run-arm.sh pr   p8-ensure.mjs           # runtime/ensure mislabel + ERROR log
bash run-arm.sh pr   p9-persistence.mjs      # refusal window persistence

The daemon runs with --initialize-timeout-ms 4000 purely to shorten the wait; nothing else is tuned. Freezing a child is kill -STOP / kill -CONT on the real qwen --acp pid.

中文版

本地验证报告 — 真实 daemon 端到端

我为这个 PR 搭建了真实的验证环境(我是维护者,这是合并前的检查)。下面所有结论都来自真实的 qwen serve daemon 和真实拉起的 qwen --acp 子进程,通过真实的 HTTP / WebSocket 驱动,没有 mock bridge。A/B 两侧分别是 PR head 6e6639d4f7 和它声明的 base codex/issue-8586-agent-watchdog @ 25df3993ab,各自独立 npm ci 并打包。

为了走到 Reviewer Test Plan 要求的三个生命周期转换,我用 SIGSTOP 冻结 ACP 子进程,让一次 workspace-control 调用超时。这是真实、未 mock 的 retireChannelAfterSessionsDrain 入口——和现有超时退役走的是同一条 BridgeTimeoutError 路径,也很贴近这个 stack 想解决的"卡住的 runtime"。

结论:设计完全按描述工作,但 PR 目前是红的,我建议先卡住这一个 blocker。


✅ 复现成功的部分(PR 自己的测试计划,三个转换全过)

声明 结果
新工作只使用 active generation
recycle 后现有 Session 仍走原 generation,同时新 generation 成为 active ✅ 老 Session 在子进程 A 上继续 202,子进程 B 服务新工作
旧 generation 未退出时第二次 recycle 不会创建第三个子进程,返回 runtime_recycling ✅ 恰好 2 个子进程,第三次请求 503
draining generation 在退出前仍受 channel liveness 和 active-work reporting 覆盖 channelAlive: true;第二个 generation 出现后 active-work 覆盖率评级没有退化(两侧评级一致)
draining generation 最终确实被回收 ✅ 最后一个 Session 关闭后约 250 ms 退出,准入立刻恢复,不会产生第三个子进程

两条线路的错误映射都正确(REST 503 + ACP JSON-RPC -32603httpStatus: 503)。

fig 1 的 base/head 对比才是这个 PR 的真正价值:在 base 上,被标记退役的 generation 会继续吸收新 Session,只要新工作不断到来,它的退役就会被一再推迟;head 上它能正常 drain。这是实打实的修复。

head 上同样通过:npm run typecheck(exit 0)、npm run lint(exit 0)。packages/clisrc/serve 未受影响——15 个失败两侧完全一致(本地环境的既有噪声,与本 PR 无关)。


🔴 Blocker — packages/acp-bridge 在 head 上是红的,base 是绿的

base   Test Files  35 passed (35)          Tests  1959 passed (1959)
head   Test Files   1 failed | 34 passed   Tests     1 failed | 1958 passed

失败用例是 bridge.test.ts 里那条 "purges buffered guardrail events when restore fails...",抛出 BridgeRuntimeRecyclingErrorsrc/bridge.ts:4503)。

我给 ensureChannel 加了埋点,打印拒绝时刻的 aliveChannels

[{"state":"dying","sessions":0}, {"state":"dying","sessions":0}]   channelInfo.state=dying

两个槽位里都是零工作的终态子进程。 它们永远不可能再接受 Session,所以拒绝准入毫无收益——而 base 会正常拉起替代者。这个场景并不刁钻:一次失败的 session/load 会拆掉自己的 channel,重试需要新的 channel,而这个上限把"尸体"也算进去了。>= 2 这个判断对 draining generation 是对的,对 dying generation 是错的。

候选修复(我已应用、重新构建并全量复跑):

-    if (aliveChannels.size >= 2) throw new BridgeRuntimeRecyclingError();
+    if (
+      aliveChannels.size >= 2 &&
+      Array.from(aliveChannels).some((c) => c.state !== 'dying')
+    ) {
+      throw new BridgeRuntimeRecyclingError();
+    }

结果:1959/1959 全绿,并且 fig 1、fig 2 里的所有行为在真实 daemon 上原样复现——上限仍是 2,第三次 POST /session 仍然 503 runtime_recycling,现有 Session 仍然固定在原 generation。方案可以换,关键是这个上限不应该把已经终态的 generation 算进去。


🟠 重要 — POST /workspace/runtime/ensure 上这个拒绝被贴错标签、日志级别也过高

ensureChannel 也会经由 preheat() 到达,而 WorkspaceRuntimeCoordinator.ensure() 会把除 WorkspaceRuntimeStillStartingError 之外的所有错误包成 WorkspaceRuntimeInitializationError。于是同一个状态报出两种结果:

POST /session                   -> 503 {"code":"runtime_recycling","errorKind":"runtime_recycling","retryable":true}
POST /workspace/runtime/ensure  -> 503 {"code":"runtime_initialization_failed"}          <- 没有 errorKind,没有 retryable

在 daemon 日志里,同一个预期内的状态在一条路由上是带完整堆栈的 ERROR,在另一条路由上是干净的 WARN——因为 WorkspaceRuntimeInitializationErrorreportBridgeError,而 BridgeRuntimeRecyclingErrorrecordExpectedBridgeError

建议像现在放行 WorkspaceRuntimeStillStartingError 那样,让 BridgeRuntimeRecyclingError 直接穿过 ensure()


🟠 重要 — 一次普通超时之后 /workspace/runtime/status 就说 runtime 正在 stopping

这是最常见的路径,不是双 generation 的角落场景。一次 workspace-control 超时,Session 仍然打开且完全可用:

state runtimeLive 子进程数 POST /session/<id>/prompt /health?deep channelAlive
base active true 1 202 true
head stopping false 1 202 true

liveChannelInfo() 现在对 draining generation 返回 undefined,所以 runtimeLive 翻成 false;而被本 PR 放宽的 isChannelLive()/health 继续报 channelAlive: true。两个状态面对同一个 runtime 给出了互相矛盾的答案。

这不只是观感问题:scheduleSkillsReconciliation / scheduleMcpReconciliation!snapshot.runtimeLive 时都会直接 'deferred',所以从 drain 到新 generation 真正 active 之间,配置协调会静默变成 no-op。实测中我看到 recycle 之后能力状态变成 {state: "stale", runtimeEpoch: 1},而 runtimeEpoch 已经是 2。

而且这个拒绝窗口不是瞬时的——它的长度只取决于那两个 Session 活多久(实测 70 秒内持续 503,关掉其中一个 Session 后立刻恢复 200)。

我理解 fail-closed 是刻意取舍,不要求你改这个策略。我要求的是:这个状态持续期间要如实上报——一个正在服务两个 Session 的 runtime 报 runtimeLive: false,控制面是会据此动作的。


🟡 次要

  • 新的 503 没有 Retry-After 它是 error-response.ts 里唯一一个声明 retryable: true 却不带该 header 的 503——init_timeoutacp_channel_unavailable 都带,runtime_still_starting 甚至没声明 retryable 也带。已在线上确认(Retry-After present? false)。
  • runtime_recycling 没有进文档。 所有同类错误码都在 docs/developers/qwen-serve-protocol.mddocs/users/qwen-serve.md 里;最自然的位置是 POST /session 错误列表中 503 acp_channel_unavailable 那一条旁边。目前这个码只存在于新增的设计说明里。
  • 没有测试。 这个 diff 新增了一个三态状态机、一个新错误类、一个新的公开接口方法和两条新的线路映射,却没有碰任何测试文件。grep 找不到任何测试引用 BridgeRuntimeRecyclingErrorruntime_recycling。考虑到上面那个 blocker 是被一个无关的既有测试逮到的,这个状态机目前是完全没有守卫的。
  • requestRuntimeRecycle 在本 PR 中是死代码 — 只有声明和实现,在 fix(daemon): recycle unresponsive agent runtimes #11275 之前没有任何调用方。作为 stack 的拆分方式没问题,但意味着这个 recycle 入口是未经验证就合入的。
  • isDying 的 setter 会静默吞掉 false set isDying(v) { if (v) this.state = 'dying'; }ci.isDying = false 变成 no-op。目前 10 处赋值全是 = true,所以没有实际 bug——但这是给后来人挖的坑,至少值得加一句注释。

结论

建议 changes requested:把 aliveChannels 上限修好让测试转绿,之后这个 PR 就可以合。其余几条建议顺手一起处理,但单独拿出来都不足以卡住合并。

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants