fix(serve): Release managed session writer locks on shutdown - #7812
Conversation
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
🩺 serve daemon A/BBuilt the PR base vs this PR head ✅ No response changes against the PR base across 4 scenario(s). — Qwen Code · serve A/B |
|
Thanks for the PR! Template looks good ✓ Problem: observed bug with clear evidence. Linked to P0 issue #7752 — a managed daemon replacement on a different hostname cannot prove the previous ACP child's foreign-host writer lock is stale, so it returns Direction: aligned. This is the correct P0 fix — making lock release an acknowledged part of managed child shutdown, while preserving fail-closed behavior when evidence is uncertain. The design doc ( Size: this is a cross-package change touching core paths ( Approach: the scope feels right for the problem. The atomic rename ( Risk: Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 问题：已观测到的 bug，有明确证据。关联 P0 issue #7752——托管 daemon 替代实例在不同 hostname 启动时，无法证明前一个 ACP 子进程的外部 hostname writer 锁已过期，因此无限期返回 方向：对齐。这是正确的 P0 修复——将锁释放变成托管子进程关闭确认的一部分，同时在证据不确定时保持 fail-closed 行为。设计文档清楚地界定了范围内和范围外的内容。 规模：跨包变更，触及核心路径。生产逻辑约 1304 行，测试约 1235 行，文档/配置约 141 行。超过 1000 行大 PR 建议适用——但范围合理：协作式关闭必然跨越进程生命周期、writer 锁、会话准入和资源清理。 方案：范围合理。原子 rename（ 风险： 进入代码审查 🔍 — Qwen Code · qwen3.8-max-preview Reviewed at |
Code ReviewIndependent proposal: for this problem I would have (1) made the ACP child release writer locks in its SIGTERM handler, (2) tracked all spawned children in the daemon parent with a bounded TERM→KILL→deadline timeline, (3) made lock release atomic so a successor can't be confused, (4) closed session admission once shutdown starts, and (5) reported unclean shutdown when release fails. Comparison: the PR matches and exceeds this baseline. The atomic rename ( No critical blockers found. No AGENTS.md violations. The new One observation (non-blocking): the multiple TestingCI evidence (from the PR's own checks on the reviewed commit):
All substantive checks green. The macOS/Windows/Integration skips are conditional (not failures). The Real-scenario testing: N/A — daemon lifecycle and persistence behavior only; no TUI or Web UI changes. The author's test plan describes a real managed-ACP harness (3 writer locks released 3/3, clean exit ~266ms after SIGTERM), but that is the author's claim, not independently verified here. A maintainer can trigger Not verified: Windows and Linux behavior (author tested macOS only). Kubernetes rolling replacement was not exercised. SIGKILL-before-handler and event-loop stall scenarios remain theoretical (by design — they produce unclean shutdown, which is the safe outcome). 中文说明代码审查独立方案： 我会 (1) 让 ACP 子进程在 SIGTERM handler 中释放 writer 锁，(2) 在 daemon 父进程中跟踪所有子进程并使用有界的 TERM→KILL→截止时间线，(3) 让锁释放原子化以免后继者混淆，(4) 关闭开始后拒绝新会话，(5) 释放失败时报告不干净关闭。 对比： PR 匹配并超越了基线方案。原子 rename（ 未发现关键阻塞问题。未发现 AGENTS.md 违规。 测试CI 证据：所有实质性检查通过。 真实场景测试：N/A——仅涉及 daemon 生命周期与持久化行为，没有 TUI 或 Web UI 变化。维护者可触发 未验证：Windows 和 Linux 行为。Kubernetes 滚动替换未执行。 — Qwen Code · qwen3.8-max-preview Reviewed at |
|
Confidence: 4/5 — solid P0 fix with sound design, thorough tests, and green CI; the high-risk path match and untested Windows/Linux paths keep it from a perfect score, but nothing blocks. This is a well-executed fix for a real operational problem. The design doc scopes the work honestly — what's in, what's deferred, and why automatic lock stealing is unsafe. The implementation follows through: the atomic rename as release commit point, the fail-closed behavior on any uncertainty, the bounded process timeline, and the initialization-race handling all show careful thinking about the failure modes that matter in production. The test coverage is extensive (1235 lines) and covers the right edge cases: flush failure, rename error after effect, successor acquisition, shutdown during initialization, concurrent shutdown calls, and the stable-terminal guarantee on release failure. CI is fully green, including the daemon-specific Non-blocking nits: the Ships the feature cleanly. The before/after is clear: managed daemon replacement goes from "indefinite 中文说明置信度：4/5 —— 可靠的 P0 修复，设计合理，测试充分，CI 全绿；高风险路径匹配和未测试的 Windows/Linux 路径使其未达满分，但没有阻塞项。 这是一个针对真实运维问题的精心修复。设计文档诚实地界定了工作范围——范围内、范围外、以及为什么自动锁抢占不安全。实现跟进了：原子 rename 作为释放提交点、任何不确定性时的 fail-closed 行为、有界的进程时间线、以及初始化竞态处理，都展示了对生产环境中重要失败模式的仔细思考。 测试覆盖广泛（1235 行），覆盖了正确的边缘情况。CI 全绿，包括验证本 PR 修改的生命周期路径的 daemon 特定检查。 非阻塞建议： 干净地交付了功能。前后对比清晰：托管 daemon 替换从"无限期 — Qwen Code · qwen3.8-max-preview Reviewed at |
|
⏸️ Deferring to @doudouOUC — the review is clean and CI is green, but the core-path size escalation (1198 production lines touching packages/core config and services) requires a maintainer explicit sign-off per the gate policy. No blocking code concerns found — this needs a human approval decision on the scale, not a code fix. |
Review —
|
| step | budget |
|---|---|
fireSessionEndOnce (:2857) |
DEFAULT_HOOK_TIMEOUT = 60000, sequential per Config |
finishManagedShutdown (:2862) |
unbounded — joins in-flight Config.initialize() by design |
drainPoolBeforeExit(label, true) (:2868) → shutdownMcpPool(8_000) (:2774) |
8s |
Any one of those crossing 5s ⇒ SIGKILL ⇒ signalCode !== null ⇒ exited uncleanly during shutdown (process-registry.ts:156) ⇒ registry rejects ⇒ bridgeShutdownError ⇒ daemon shutdown incomplete and close() rejects.
The locks are released (writer terminal runs first), so the P0 still holds — but a user with one SessionEnd hook or one slow MCP server gets a nonzero daemon exit on every graceful restart. Worth either raising TERM_GRACE_MS above the child's own worst case, or bounding the child's post-writer phase to comfortably under 5s so the escalation stays exceptional.
3. Consequence of #2: qwen serve can now refuse to exit on the first SIGTERM.
close()'s rejection reaches onSignal's catch, which hits the pre-existing branch:
if (channelWorkerManager?.state().enabled) {
daemonLog.error('refusing to exit while a channel worker or service lease remains; signal again to retry…');
return; // no process.exit
}Before this PR that branch was unreachable from an ACP child, because bridge.shutdown() swallowed every kill error (channels.map((ci) => ci.channel.kill().catch(() => {}))). Now a child that exits by signal or nonzero — likely per #2 — propagates all the way to bridgeShutdownError (packages/cli/src/serve/run-qwen-serve.ts:6565) and can hang the daemon until a second signal. Under Kubernetes there is no second SIGTERM: the pod sits until terminationGracePeriodSeconds and is SIGKILLed, which is a worse restart story than the one being fixed.
Recommend keeping the process-registry/child-exit failure distinct from the errors that gate that "refuse to exit" path (that branch is about an un-reaped channel worker, not an ACP child the registry has already SIGKILLed).
Medium
4. shutdownMcpPool throws on a very low bar. if (result.forced > 0 || result.errors.length > 0) throw (acpAgent.ts:3609-3621). forced is computed by subtraction — it counts entries that merely hadn't settled when the deadline fired (mcp-transport-pool.ts:814), not entries that misbehaved. One MCP transport that rejects on forceShutdown (already-closed transport is a plausible case) makes the managed child exit 1 and the whole daemon shutdown unclean. Consider treating errors as unclean but forced as a warning, or logging which servers were responsible so the operator can act.
5. Config.shutdown() can never reject, so the resource-cleanup terminal is not actually a terminal. shutdownResourcesOnce wraps its entire body in try { … } catch { debugLogger.error('Error during Config shutdown:', error) } (packages/core/src/config/config.ts:4585-4621); closeWriter() is .catch-wrapped; telemetry is disabled on the managed path. Therefore the rejection-collecting loop in finishManagedShutdown:
const results = await Promise.allSettled(configs.map((c) => c.shutdown({ shutdownTelemetry: false, skipSessionWriter: true })));
for (const result of results) if (result.status === 'rejected') failures.push(result.reason);is dead code in production, and the design doc's "Writer shutdown and Config resource shutdown are separate terminals" only holds for the writer half. The test that appears to cover it (keeps managed EOF unclean after writer failure while finishing resources) mocks innerConfig.shutdown to throw, so it passes without touching the real method — it proves the ordering assertion (['writer','resources']) but nothing about resource-failure propagation.
Low / nits
-
releaseOnce()callsreadOwnedLock()outside the try (session-writer-lease.ts:854-856). A pre-renameSessionWriterLostError(lock already gone) therefore leavesreleased === falsewhilereleasePromisememoizes the rejection forever. Old code setthis.released = trueon ENOENT. Downstream,Config.closeSessionWriterOncepushes that error intofailuresunconditionally, so "the lock we were going to release is already gone" reports the whole managed shutdown as unclean. Settingreleased = truewhenreadOwnedLock()throwsSessionWriterLostErrorwould keepisReleasedtruthful. -
ProcessRegistry.shutdown()ignores outstanding reservations. It snapshotsthis.childrenonly; a token reserved beforedrainingbut attached after the snapshot is terminated out-of-band viaif (this.draining) void tracked.terminate().catch(() => {})(process-registry.ts:47), soshutdown()can resolve while that child is still being reaped. Unreachable today —spawn()is synchronous betweenreserve()andattach()— butProcessRegistryis now public API (index re-export + new./processRegistrysubpath), so a third-party factory awaiting in that window would break the seal the design doc promises. Cheap fix: join outstanding reservations inshutdown(). -
Missing vitest alias for the new subpath.
run-qwen-serve.ts:1063adds a runtimeimport('@qwen-code/acp-bridge/processRegistry'), butpackages/cli/vitest.config.tsisn't updated — its sibling@qwen-code/acp-bridge/spawnChannelon the very next line is aliased, with a comment saying the alias block exists because "dist may be stale or absent during development". As written, the CLI suite resolvesProcessRegistryfrompackages/acp-bridge/dist, soruntime.ProcessRegistryin tests is a different class object from the onespawnChannel.tsuses internally, and the suite silently requires a fresh acp-bridge build. -
Orphan
.released.*artifacts. Theunlink(R)after the rename is best-effort and unlogged on crash; nothing ever GCssession-writer-locks/*.released.*. One stray file per affected session accumulates under the runtime base dir with no cleanup path. -
Silent mid-turn transcript truncation.
beginClose()flipsacceptingWrites = falsesynchronously andrecordRecordreturns early with no error (chatRecordingService.ts:904), whilecancelPendingPrompt()is fire-and-forget (acpAgent.ts:3262). On the managed fast path there's also no finalize record, so after a daemon restart a transcript just ends mid-turn with no marker. Intentional per the design doc, but worth surfacing in user-facing docs. -
cannot remove a successor lock after release commitsis weaker than its name. The secondfirst.release()returns the memoized already-resolved promise, so the rename/inspect logic never runs; only thesuccessor.appendJsonLine(...)assertion carries signal. A version that clearsreleasePromise(or drivesreleaseOncedirectly) would actually test the guard. -
Doc filename convention. Every other file in
docs/design/is date-prefixed (2026-06-24-…); this one isn't.
What's good
- The rename-to-owner-unique-path release commit, with the old owner only ever cleaning its exact
Rand never retrying the primary path — that's the right primitive, andreadOwnedLock's new raw-bytes comparison againstlockRecordRawis a correct tightening (verified: the record is written as exactlyJSON.stringify(record)atsession-writer-lease.ts:394and never rewritten). reclaimPolicy: 'never'scoped to managed acquisition only, with standalone ACP keeping local stale-owner recovery — correctly conservative given container PID namespaces.- Distinguishing pre-spawn
error(no process) from post-spawnerror(rawexitis the only proof of reaping) — this fixes a real correctness bug in the oldexitedpromise, andspawnChannel.test.tscovers both directions. - One shared registry across primary/secondary/dynamic workspace factories, with the timeline started before the slower worker/bridge phases.
bridge.shutdown()waiting for a retiring channel from an overlappingkillSessionbefore reporting — the new test for that is a good one.- Single-flight terminals with stable results throughout (
terminate,close,closeSessionWriter,release,shutdown) — consistent and easy to reason about.
Verdict
The lock-release mechanism itself looks correct and well-tested. I'd want #1 resolved before merge (it can permanently wedge a lock in the same way the PR is fixing, and it's asserted by a new test as intended behavior), and #2/#3 addressed or explicitly accepted — as written, a normal restart with a SessionEnd hook or a slow MCP server ends in a nonzero daemon exit, or a daemon that won't exit at all on the first SIGTERM.
中文说明
概述
本 PR 把 writer 锁释放变成托管 ACP 子进程关闭确认的一部分，包含四块：lease 用同目录 rename(P → R) 提交释放并让失败成为稳定终态、新增 reclaimPolicy: 'never'；recorder/Config 拆出同步的写入准入关闭与单次 writer 终态；agent 在第一个 await 之前快照所有 writer-capable Config 并启动全部 writer 终态；父进程新增 daemon 作用域的 ProcessRegistry（SIGTERM → 5s SIGKILL → 10s 回收）。整体设计合理，"先 writer、再 hook、最后资源"的顺序对 P0 是正确的。我的意见集中在哪些失败被归类为"保留锁"/"不干净"，以及 bridge.shutdown() 变成会 reject 之后的影响面。
严重
- 单次瞬时写失败会永久卡死锁 ——
closeOnce()在 flush 失败时直接抛出、不再release()；而flush()抛的是writeFailure，该字段在 recorder 生命周期内永不清除。所以保留锁的触发条件是"本会话内任意时刻发生过一次 append 失败"，而不是 PR 描述里的"所有权不确定"。会话第 3 分钟一次EIO，两小时后关闭时锁被保留；由于托管模式reclaimPolicy: 'never'，任何后继实例都无法再获取，只能人工删锁 —— 正是本 PR 想消除的故障模式。改动前该场景是会释放锁的。建议改为以integrityFailure为准，或至少输出面向运维、包含锁路径的可见告警（debugLogger.error在未开调试日志时不可见）。 - 子进程自身的清理预算超过父进程的 5s 宽限期 —— SessionEnd hook 默认 60s、MCP drain 8s、
initialize()join 无上限，任一超过 5s 都会被 SIGKILL，进而exited uncleanly→daemon shutdown incomplete。锁确实已释放（writer 终态最先执行），但只要用户有一个 hook 或一个较慢的 MCP server，每次正常重启都会得到非零退出。 - 由 Where is the config saved? #2 引出：
qwen serve可能在第一次 SIGTERM 时拒绝退出 ——close()的 reject 会走到onSignal的 catch，命中既有的refusing to exit while a channel worker…分支并直接return。改动前 ACP 子进程的 kill 错误被.catch(() => {})吞掉，永远到不了这里。K8s 不会发第二次 SIGTERM，结果是等到terminationGracePeriodSeconds再被 SIGKILL。
中等
shutdownMcpPool的抛出门槛过低：forced是用减法算出的"截止时刻尚未 settle"的条目数，并非行为异常；一个 transport 在关闭时 reject 就会让整个 daemon 关闭不干净。Config.shutdown()实际上永远不会 reject（shutdownResourcesOnce全量 try/catch 吞异常、closeWriter()已.catch、托管路径关闭 telemetry），因此finishManagedShutdown里收集config.shutdown()拒绝的循环在生产中是死代码；对应测试因为直接 mock 了innerConfig.shutdown，只验证了顺序断言，没有覆盖真实方法。
次要
releaseOnce()把readOwnedLock()放在 try 之外：锁已消失时released仍为 false，isReleased长期不真实，且会让"锁本来就不在了"被记为不干净关闭。ProcessRegistry.shutdown()未纳入未完成的 reservation（当前同步spawn下不可达，但它已是对外 API）。packages/cli/vitest.config.ts缺少@qwen-code/acp-bridge/processRegistry别名（相邻的spawnChannel有），测试会从 dist 加载到与源码不同的类对象。.released.*残留文件无任何回收路径。- 托管快速关闭会静默截断进行中的 turn，且不写 finalize 记录，建议在用户文档中说明。
cannot remove a successor lock after release commits实际只等到了被 memo 的已完成 promise，没有真正跑到 rename/inspect 逻辑。docs/design/下其它文件均带日期前缀，本文件没有。
结论
锁释放机制本身正确且测试充分。建议合并前处理 #1（会以与本 PR 所修问题相同的方式永久卡死锁，且被新测试当作预期行为固化），并处理或明确接受 #2/#3。
|
Thanks @wenshao for the detailed review. I independently re-checked all 12 points against the reviewed head and pushed
Validation on the pushed commit:
There were no inline review threads to resolve; this top-level comment records the disposition of every review item. |
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. 1 Suggestion-level finding(s) could not be anchored to a changed line and were dropped; nothing further to act on here.
中文说明
已审查——无阻断问题。 建议见行内评论。 1 条建议级发现无法锚定到改动行，已丢弃；此处无需进一步处理。
— qwen3.7-max via Qwen Code /review
Local verification on head
|
| Claim | Result |
|---|---|
| core writer/config tests | 521 passed, 1 skipped (3 files, all collected) |
| ACP bridge/process tests | 487 passed |
| ACP agent + serve tests | 525 passed |
| build / typecheck / lint | all pass |
| Managed harness releases every lock and exits clean | reproduced — 3/3 locks released, ACP child reaped, daemon stopped, 101 ms after SIGTERM |
The core mechanism works. On a healthy filesystem this PR does exactly what it says.
1. Blocking — writer-lock retention is a regression against the base, not just a conservative choice
Your reply to review item 1 kept fail-closed retention on the grounds that "a generic filesystem write failure can be partial, so successful ownership alone does not prove transcript integrity." That reasoning is sound in isolation, but it was made against the wrong baseline: the base does not retain the lock in this situation.
chatRecordingService.ts on 1f9a1a90a captures the flush failure and still releases the lease, then rethrows:
let flushFailure: unknown;
try { await this.flush(); } catch (error) { flushFailure = error; }
try {
await this.binding?.lease.release(); // <-- always runs
...
}
if (flushFailure !== undefined) throw flushFailure; // caller still learns about itThe PR replaces this with closeOnce() (chatRecordingService.ts:1114), which await this.flush() at :1116 — and flush() rethrows the sticky writeFailure (:1044). enterWriteFailure sets that flag on any append error and never clears it (:847), so lease?.release() at :1123 is never reached. Independently, reclaimPolicy: 'never' (new in this PR — the base has no such concept) is applied to every managed session at acpAgent.ts:10685, which removes the stale-owner reclaim that used to recover exactly this state.
Neither change alone is fatal. Together they turn a recoverable state into a permanent one.
A/B, identical scenario, two real bundled builds — one metadata touch on the transcript, filesystem then fully healed (content byte-identical), then an ordinary SIGTERM:
Follow-through on the PR side: a replacement daemon calling session/load gets session_writer_conflict — permanently. The owner PID is dead and the hostname matches this host, and it still never recovers:
reclaimPolicy='never' -> session_writer_conflict # what this PR sets for managed sessions
reclaimPolicy='local' -> ACQUIRED — self-heals
Only manual rm of the lock file clears it.
How reachable is this? I triggered it with chmod, which changes the file's metadata and trips the fingerprint check in appendJsonLine → SessionTranscriptChangedError. Any backup agent, antivirus scanner, container-layer copy, or chown does the same. Separately — by code reading, not executed — appendJsonLine's catch normalises every non-SessionWriterError into SessionWriterUnavailableError, so a plain transient EACCES/ENOSPC/EIO on one append reaches the identical sticky path. A single recovered I/O blip permanently wedges that session.
Worth being clear about scope: the motivating k8s case is not regressed. The base returns {kind:'live'} for any foreign hostname (session-writer-lease.ts:265), so cross-host locks never self-healed before either, and this PR's cooperative release genuinely fixes them. The regression is on the same-host restart path, which is the common case for a local or single-node daemon.
Suggestion — the fail-closed intent and same-host recovery aren't actually in conflict. Either restore the base's ordering (release, then report the flush failure — the owner still holds provable exact ownership), or keep retention but scope reclaimPolicy: 'never' to the case it was written for, letting a same-host provably-dead owner still be reclaimed.
2. The regression test added for review item 3 is vacuous
Item 3's fix looks correct by inspection — retryableChannelWorkerShutdownErrors only tags an error when channelWorkerShutdownError !== undefined, so an ACP-only failure falls through to process.exit(1). But the test added for it does not pin it.
Keeping the round-2 tests and overlaying the round-1 source (reverting only the feedback commit's code):
acpAgent.test.ts→ 2 tests fail. ThestrictResourceCleanupfix (item 5) is properly pinned. 👍run-qwen-serve.test.ts→ all 204 pass. Nothing in the file pins the item-3 fix.
Why: probing the guard at signal time shows channelWorkerManager.state().enabled = false. enabled is committedSelection !== undefined || group !== undefined || leaseReserved (channel-worker-manager.ts:258), and the fixture never gets the manager there — passing channelSelection: {names:['telegram']} with a stubbed supervisor isn't sufficient. So the old if (channelWorkerManager?.state().enabled) guard and the new WeakSet guard are both skipped, and the test passes on either version. Swapping in makeReadyWorkerFactory doesn't fix it either — I tried.
Cheap to fix, and worth fixing: this is the one failure mode that is fatal under Kubernetes (no second SIGTERM before SIGKILL), so it should not be able to regress silently.
3. Item 2 (hook budget) — your assessment is correct, and I was over-worried
I raised the 60 s DEFAULT_HOOK_TIMEOUT vs 5 s TERM_GRACE_MS gap; you replied that slow post-writer cleanup "does not restore the released lock." Executed with a 20 s SessionEnd hook, that is exactly right:
The writer phase runs before hooks, so all locks were still released — the PR's core guarantee holds under a slow hook. What remains is narrower than I implied: the hook is silently truncated at ~5 s despite its documented 60 s budget, and the restart reports daemon shutdown incomplete. Worth a documentation note about the effective SessionEnd budget under serve; not a merge blocker.
4. Nit — the new retention warning over-reports
The stderr message lists a candidate lock path per config in terminal.configs, including the daemon's own primary config, which holds no lock. My run printed 2 candidate paths when exactly 1 lock existed. Filtering to paths that exist would make it directly actionable for an operator.
Recommendation
Items 3–5 are handled well and item 5 is genuinely test-pinned. Finding 1 is the one I'd want resolved before merge — not because fail-closed is wrong, but because the combination with reclaimPolicy:'never' makes a previously self-healing state permanent on the same host, and that trade wasn't visible when the call was made. Finding 2 is a small test change that protects the most operationally severe path.
Happy to re-run this harness against a new head.
中文版本
在 head 4f8991bf0 上的本地验证 —— 真实打包 daemon
这是对我此前 review 和 @doudouOUC 的逐条回复 的后续。本轮是实际执行，而非阅读代码：在 PR head 上使用独立 worktree 并单独 npm ci，真实打包的 qwen serve、真实的托管 ACP 子进程、真实的磁盘 writer 锁；针对核心结论，还完整构建了 base 提交做 A/B 对比。
你报告的数据全部复现
| 声明 | 结果 |
|---|---|
| core writer/config 测试 | 521 通过，1 跳过（3 个文件全部收集） |
| ACP bridge/process 测试 | 487 通过 |
| ACP agent + serve 测试 | 525 通过 |
| build / typecheck / lint | 全部通过 |
| 托管 harness 释放全部锁并干净退出 | 已复现 —— 3/3 锁释放，ACP 子进程回收，daemon stopped，SIGTERM 后 101 ms |
核心机制是有效的。在文件系统健康的情况下，本 PR 的行为与描述完全一致。
1. 阻塞项 —— writer 锁保留相对 base 是功能回退，而不仅仅是保守选择
你在 review 第 1 条的回复中保留了 fail-closed 保留策略，理由是"通用文件系统写入失败可能是部分完成的，因此仅有所有权成功并不能证明 transcript 完整性"。这个理由本身成立，但它对照的基线不对：base 在这种情况下并不会保留锁。
1f9a1a90a 上的 chatRecordingService.ts 会捕获 flush 失败，仍然释放 lease，然后再抛出：
let flushFailure: unknown;
try { await this.flush(); } catch (error) { flushFailure = error; }
try {
await this.binding?.lease.release(); // <-- 始终执行
...
}
if (flushFailure !== undefined) throw flushFailure; // 调用方仍然会知道失败本 PR 用 closeOnce()（chatRecordingService.ts:1114）取代了它，其中 :1116 处 await this.flush()，而 flush() 会重新抛出粘滞的 writeFailure（:1044）。enterWriteFailure 在任何追加错误时设置该标志且永不清除（:847），因此 :1123 的 lease?.release() 永远不会执行。另外，reclaimPolicy: 'never'（本 PR 新增，base 中不存在该概念）在 acpAgent.ts:10685 被应用到每一个托管会话，这移除了原本能恢复该状态的 stale owner 回收机制。
单独看任一改动都不致命。两者叠加，就把一个可恢复状态变成了永久状态。
A/B：相同场景，两个真实打包构建 —— 对 transcript 做一次元数据触碰，随后文件系统完全恢复（内容逐字节一致），再执行一次普通 SIGTERM：base 锁已释放、会话可恢复；PR 锁被保留、会话卡死。
后续验证：PR 侧替代 daemon 调用 session/load 会得到 session_writer_conflict，且是永久性的。owner PID 已死亡、hostname 与本机相同，依然无法恢复：
reclaimPolicy='never' -> session_writer_conflict # 本 PR 为托管会话设置的值
reclaimPolicy='local' -> ACQUIRED —— 可自愈
只有手动 rm 锁文件才能清除。
触发难度如何？ 我用 chmod 触发，它改变文件元数据，从而触发 appendJsonLine 中的指纹校验 → SessionTranscriptChangedError。任何备份代理、杀毒扫描、容器层复制或 chown 都会产生同样效果。另外（这一条来自代码阅读而非实际执行）：appendJsonLine 的 catch 会把所有非 SessionWriterError 归一化为 SessionWriterUnavailableError，因此单次瞬时 EACCES/ENOSPC/EIO 也会进入完全相同的粘滞路径。一次已恢复的 I/O 抖动就会永久卡死该会话。
需要明确范围：k8s 这一动机场景没有回退。base 对任何外部 hostname 都返回 {kind:'live'}（session-writer-lease.ts:265），所以跨主机锁此前也从不自愈，本 PR 的协作式释放确实修复了它。回退发生在同主机重启路径上，而这正是本地或单节点 daemon 的常见情况。
建议 —— fail-closed 的意图与同主机恢复其实并不冲突。要么恢复 base 的顺序（先释放、再上报 flush 失败 —— 此时 owner 仍持有可证明的精确所有权），要么保留该策略但把 reclaimPolicy: 'never' 限定在它真正针对的场景，允许同主机上可证明已死亡的 owner 仍被回收。
2. 为 review 第 3 条新增的回归测试是空转的
第 3 条的修复从代码上看是正确的 —— retryableChannelWorkerShutdownErrors 只在 channelWorkerShutdownError !== undefined 时标记错误，因此纯 ACP 失败会落到 process.exit(1)。但为它新增的测试并没有锁定这个行为。
保留 round-2 的测试、覆盖回 round-1 的源码（只回退 feedback 提交的代码）：
acpAgent.test.ts→ 2 个测试失败。第 5 条的strictResourceCleanup修复确实被锁定了 👍run-qwen-serve.test.ts→ 204 个全部通过。文件中没有任何测试锁定第 3 条的修复。
原因：在信号触发时刻探测该 guard，得到 channelWorkerManager.state().enabled = false。enabled 的定义是 committedSelection !== undefined || group !== undefined || leaseReserved（channel-worker-manager.ts:258），而 fixture 从未让 manager 进入该状态 —— 仅传入 channelSelection: {names:['telegram']} 加一个 stub supervisor 并不够。因此旧的 if (channelWorkerManager?.state().enabled) 分支与新的 WeakSet 分支都被跳过，测试在两个版本下都通过。改用 makeReadyWorkerFactory 也不行，我试过了。
修复成本很低，也值得修：这是在 Kubernetes 下唯一致命的失败模式（SIGKILL 之前不会有第二次 SIGTERM），不应该能够无声回退。
3. 第 2 条（hook 预算）—— 你的判断是对的，是我此前担心过度了
我此前提出 60 s 的 DEFAULT_HOOK_TIMEOUT 与 5 s 的 TERM_GRACE_MS 之间的落差；你回复说 writer 之后的缓慢清理"不会让已释放的锁复活"。用一个 20 s 的 SessionEnd hook 实测，结论完全正确：
writer 阶段在 hooks 之前执行，因此所有锁仍然被正确释放 —— 在慢 hook 下本 PR 的核心保证依然成立。剩下的问题比我此前描述的要小：hook 在约 5 s 时被静默截断（尽管其文档预算是 60 s），并且该次重启会被报告为 daemon shutdown incomplete。建议在文档中说明 serve 下 SessionEnd 的实际预算；不构成合并阻塞。
4. 小问题 —— 新增的保留警告存在过度上报
该 stderr 信息会为 terminal.configs 中的每个 config 列出一个候选锁路径，其中包含 daemon 自身的 primary config —— 而它并不持有锁。我这次运行打印了 2 个候选路径，而实际只存在 1 个锁。过滤为实际存在的路径会让它对运维人员更直接可用。
结论
第 3–5 条处理得很好，第 5 条确实有测试锁定。第 1 条是我希望在合并前解决的 —— 不是因为 fail-closed 本身有问题，而是因为它与 reclaimPolicy:'never' 叠加后，把同主机上原本可自愈的状态变成了永久状态，而做该决定时这个代价并不可见。第 2 条只是一个小的测试改动，用于保护运维影响最严重的那条路径。
如果推送新的 head，我很乐意再用这套 harness 重跑一遍。
|
@wenshao Thanks for the execution-level A/B. I independently reproduced both blocking points and addressed them in
Validation: core writer/config tests |
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.7-max via Qwen Code /review
|
@qwen-code /takeover |
qwen-code-dev-bot
added
the
autofix/takeover
Review: release managed session writer locks on shutdownReviewed the full diff (20 files). The core design holds up well:
Three points below, in descending order of how much I'd want them addressed. 1. A wedged child now turns a clean daemon exit into exit 1, and the operator-facing zombie warning is goneThe old
Failing loudly instead of reporting a false "graceful" is the right call and I'm not arguing against it. Two consequences worth handling:
2.
|
621e085 to
8e583a0
Compare
|
Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. 中文请勿对活跃的 PR 执行 rebase 或 force-push，因为这会使已有的评审评论失效。另外，供日后参考：作为集成流程的一部分，机器人始终会自动将所有改动压缩（squash）为单个提交。 |
|
Rebased onto the latest Verification after the rebase:
No new actionable review feedback appeared. Existing six suggestion-only threads already have author replies and remain intentionally unresolved under the repository's review-round cutoff. Resolved this round: 0. 已 rebase 到最新 Rebase 后验证：
本轮没有新增可执行 review feedback。现有 6 个仅建议级 thread 均已有作者回复，并依据仓库 review 轮次截止规则有意保持未解决。本轮 resolved：0。 |
|
🤝 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. Remove the 中文说明🤝 已接管：autofix 循环现在管理此 PR —— 将持续处理新的评审反馈与 base 冲突，直到移除标签或达到轮次上限。移除 |
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! ✅
中文说明
未发现问题。LGTM！✅
— qwen3.7-max via Qwen Code /review
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
1f7203a to
da86eb0
Compare
Local verification report — real build, real daemon, real child processesI built this PR from source and verified it end-to-end on Linux against a real bundled Verdict: the behavior this PR claims is real, measurable, and does not regress the existing fail-closed paths. One blocker before merge: the branch no longer merges cleanly with Environment: Linux 6.12.63 (Debian 13) · Node v22.22.2 · 1. The decisive A/B — what happens when the lock cannot be releasedI made the lock directory immutable (
This is the core of the P0. On 2. Managed acquisition never reclaims — and standalone still doesI hand-planted a writer lock and asked a replacement daemon to
The second row is the regression check that matters: the pre-existing fail-closed behavior is preserved exactly. A separate real-filesystem run against the built
3. Child-process timeline and end-to-end shutdown
Bundled
For transparency: in the happy path and the mid-turn path, base 4. Static checks and test suites (PR head)
† Both failures are Blocker: rebase needed
Everything else auto-merges. The conflict is against #7894 "Gate session writer lease behind opt-in", which landed on
Worth stating in the PR body afterwards: after #7894 the whole writer-lease protocol is behind Minor, non-blockingThe retained-lock warning derives candidate paths from every RecommendationApprove after rebase. The correctness story holds up under real execution, the fail-loud behavior is a genuine improvement over silently-clean, and the no-reclaim policy is correctly scoped to managed children only. Full harness sources (E2E shutdown, mid-turn, multi-workspace, fail-injection, reclaim A/B, real-process registry, real-fs lease policy) are reproducible from the descriptions above; happy to attach them if useful. 中文说明本地验证报告 —— 真实构建、真实 daemon、真实子进程我从源码构建了本 PR，并在 Linux 上针对真实打包的 结论：本 PR 声称的行为是真实、可测量的，并且没有破坏现有的 fail-closed 路径。合并前有一个阻塞项：该分支已经无法与 环境：Linux 6.12.63（Debian 13）· Node v22.22.2 · 在两个隔离的 worktree 中执行 1. 决定性的 A/B —— 当锁无法被释放时会发生什么我把锁目录设为不可变（
这就是这个 P0 的核心。在今天的 2. 托管模式下永不抢锁 —— 而独立模式仍然会回收我手工植入一个 writer 锁，然后让一个替代 daemon 执行
第二行才是真正重要的回归检查：既有的 fail-closed 行为被完整保留。另外一轮针对已构建
3. 子进程时间线与端到端关闭
打包后的
为了透明说明：在正常路径和turn 进行中这两个场景下，base 4. 静态检查与测试套件（PR head）
† 两个失败都来自 阻塞项：需要 rebase
其余文件都能自动合并。冲突来自今天刚合入
rebase 之后值得在 PR 描述里补一句：在 #7894 之后，整套 writer lease 协议都在 次要问题（不阻塞）保留锁的告警是从终态中的每一个 建议rebase 后 Approve。 正确性在真实执行下站得住脚，「失败即大声报错」相比「静默地看起来干净」是实打实的改进，而「不抢锁」策略也正确地只限定在托管子进程上。 完整的 harness 源码（端到端关闭、turn 进行中、多 workspace、失败注入、抢锁 A/B、真实进程 registry、真实文件系统 lease policy）都可以按上文描述复现；如果有需要我可以附上。 🤖 Generated with Claude Code — Claude Opus 5 (1M context) |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Not reviewed: invariant-a — the coverage harness recorded the first launch's transcript (which did not open the brief); the substantive relaunch examined state, timers, and collections and found nothing, but the transcript gap persists. Not reviewed: the invariant check (state, timers, collections) on packages/acp-bridge/src/spawnChannel.ts — never opened its brief, so it reviewed without the instructions it was launched to follow.
中文说明
已审查。 未审查：invariant-a — the coverage harness recorded the first launch's transcript (which did not open the brief); the substantive relaunch examined state, timers, and collections and found nothing, but the transcript gap persists。 未审查：不变量检查（状态、定时器、集合）（packages/acp-bridge/src/spawnChannel.ts）——从未打开自己的 brief，审查时缺失了它本应遵循的指令。
— qwen3.7-max via Qwen Code /review
|
@wenshao Thanks for the thorough real-process A/B report. The reported blocker is resolved on |
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： Autofix review round — no code changesThis round triaged the new feedback (newer than the last evaluation). No code @wenshao — local verification reportBlocker "rebase needed" — verified resolved (no action required). The
No merge was performed this round (the workflow signaled no conflict); the Minor, non-blocking — candidate lock paths include configs that never held a @qwen-code-ci-bot — automated reviewThe note discloses that two of the reviewer's own sub-agents did not load their Prior inline findings — unchangedThe earlier review's inline findings remain in their prior state after the 中文说明Autofix 审查轮次 —— 无代码改动本轮对新增反馈（晚于上次评估的部分）进行了分类处理。未做任何代码改动：唯一的阻塞项已经在分支上解决，剩余的建议是一个非关键的观感项、按仓库的审查轮次截止规则延期处理，而自动审查器的说明只是对其自身审查覆盖缺口的信息披露，并非缺陷。 @wenshao —— 本地验证报告阻塞项「需要 rebase」—— 已验证解决（无需处理）。 分支已经把 #7894 作为其 merge base：
本轮没有执行 merge（工作流给出的信号是无冲突）；以上验证全部是只读的。 次要、不阻塞 —— 候选锁路径包含了从未持有过 lease 的 Config：延期处理。 要把这条告警过滤成只保留真正获取过 lease 的 Config，需要从 @qwen-code-ci-bot —— 自动审查该说明披露了审查器自身的两个子代理没有加载它们的审查 brief（ 此前的内联发现 —— 状态不变上一轮审查的内联发现在 rebase 之后保持其先前状态：各项正确性修复（错误标记、 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧠 Handled by Qwen Code · model/模型 |
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： Autofix review round — no action takenNo code change was made this round. The only new feedback is an issue-level comment from maintainer @doudouOUC. It is a status confirmation rather than a change request:
The single remaining item mentioned — "extra candidate-path filtering" — is explicitly described by the maintainer as "valid but cosmetic" and "deferred under the repository review-round cutoff." Per the repository's review policy, once a PR has been through roughly five review rounds only Critical fixes (correctness, security, data loss, regressions) are landed and remaining Suggestions are deferred to a follow-up. This item is a deferred Suggestion, not a Critical, so implementing it now would contradict the maintainer's stated decision and grow the diff without cause. There were no reviews, no inline comments, and no failed or still-red checks to address. 中文说明Autofix 审查轮次 —— 本轮未做任何改动本轮未做任何代码改动。 唯一的新反馈是维护者 @doudouOUC 的一条 issue 级评论。它是状态确认，而非改动请求：
评论中提到的唯一剩余项 —— “额外的候选路径过滤（extra candidate-path filtering）” —— 被维护者明确描述为“有效但属于表面性（cosmetic）”，并“依据仓库的审查轮次截止规则予以推迟（deferred）”。根据仓库的审查政策，当一个 PR 经历大约五轮审查后，只落地 Critical 级修复（正确性、安全性、数据丢失、回归），其余 Suggestion 推迟到后续处理。该项是一个被推迟的 Suggestion，而非 Critical，因此现在实现它会与维护者已明确表达的决定相矛盾，并会毫无理由地扩大 diff。 本轮没有需要处理的 review、内联评论，也没有失败或持续飘红的检查项。 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧠 Handled by Qwen Code · model/模型 |
Local verification, round 2 — real build, real daemon, real child processes, at head
|
| my point | current head |
|---|---|
activateChatRecording() must keep #7894's gate |
✅ if (!this.chatRecordingEnabled || !this.sessionWriterLeaseEnabled) return; |
ChatRecordingService default writerLeaseRequired |
✅ config.isSessionWriterLeaseEnabled?.() ?? … |
| this PR's additions layer on top, not instead | ✅ the shutdown checkpoints and reclaimPolicy sit inside the gated body; config.ts is no longer part of the lease-gate diff at all |
That last row is the strongest form of the check: the gate expression is now identical on both sides of the diff, so #7894 cannot have been undone.
1. The decisive A/B — what happens when the lock cannot be released
Lock directory made immutable (chattr +i, so the exact-owner rename/unlink fails with EPERM even for root), two persisted sessions holding writer locks, then SIGTERM.
base 2db663bec |
PR #7812 | |
|---|---|---|
| daemon exit code | 0 | 1 |
| writer locks | 2 → 2 (retained) | 2 → 2 (retained by design) |
| operator warning | none | qwen --acp: managed session writer shutdown failed; a writer lock may be retained for safety… + candidate lock paths |
| daemon log | daemon stopped |
daemon shutdown incomplete / ACP child pid=… exited uncleanly during shutdown (code=1, signal=none) |
Unchanged from the previous round, which is the point: this is still the whole value of the P0. On main a leaked writer lock is indistinguishable from a clean shutdown; under a rolling replacement that lock carries a foreign hostname, the replacement can never prove it stale, and nothing in the departing Pod's logs explains why.
2. Managed acquisition never reclaims — and standalone still does
| planted lock owner | base 2db663bec |
PR #7812 |
|---|---|---|
| dead local pid, same hostname | 200 OK — lock stolen, planted owner replaced |
409 session_writer_conflict, planted lock untouched |
| foreign hostname (the k8s rollout shape) | 409 session_writer_conflict |
409 session_writer_conflict (unchanged) |
The second row is the regression check that matters — fail-closed behaviour is preserved exactly. A separate real-filesystem run against the built core dist confirms the split is per-policy, 3/3: default policy still reclaims a provably-dead local owner; reclaimPolicy: 'never' refuses the same lock and leaves it byte-identical; release() rejects with SessionWriterLostError rather than deleting a successor lock it does not own.
3. Child-process timeline and end-to-end shutdown
ProcessRegistry driven with real Node children and a wall clock — 5/5: cooperative children clean in 5 ms; a SIGTERM-ignoring child SIGKILLed at 5002 ms and reported unclean; an unreapable child hitting the 10005 ms not-reaped deadline; a repeated shutdown() returning the same terminal in 0 ms (the deadline is not restarted); reserve() refused while draining.
Bundled qwen serve under SIGTERM, PR head: single workspace / 3 sessions — locks 3 → 0, exit 0 in 43 ms, 0 orphans, replacement daemon resumed 3× 200 and appended 3× 202. Mid-turn (3 sessions parked in a never-ending model stream) — the three prompt turn completed lines land after received SIGTERM, draining, locks 3 → 0, exit 0 in 51 ms. Primary + secondary workspace — 2 ACP children, both reaped, locks 4 → 0, exit 0 in 38 ms.
As before, and worth repeating: in the happy path and the mid-turn path the base also releases every lock and exits 0. Those are non-regression evidence, not a reproduction. §1 and §2 are the discriminating cases.
4. New this round — blast radius with the shipping default
Since #7894, experimental.sessionWriterLease defaults to false, so I re-ran the whole thing with the flag simply absent — including the same chattr +i injection, which is now a no-op because there is nothing to release. This is the configuration almost every operator will actually run.
Base and PR are indistinguishable: 0 locks created, exit 0, daemon stopped, no "incomplete", 0 orphans, replacement daemon resumed 2× 200 and appended 2× 202, and the appended record is on disk in both (8 transcript records per session, append marker present). The fail-loud path only arms for operators who opted in — a genuinely small blast radius, and it doubles as a negative control for §1: the same injection, the same build, exit 0 when there is no lease to release.
I also checked the restart-required freeze, since experimental.sessionWriterLease can be flipped on disk and re-read by a live ACP child via POST /workspace/reload. Flipping it in both directions and then creating a new session in that same child: OFF→ON the new session still takes no lock, ON→OFF it still takes one — on both builds. So the freeze is main's behaviour (it arrived with #7894, not with this PR) and this PR leaves it intact; no leased/legacy writer mixing in either direction.
5. New this round — a wedged MCP server at shutdown
This PR makes shutdownMcpPool() throw where it used to only warn, and the managed path drains it strictly. I probed that with a stdio MCP server that answers initialize/tools/list normally and then ignores SIGTERM and stdin EOF forever.
base 2db663bec |
PR #7812 | |
|---|---|---|
| daemon exit | 0 in 39 ms |
0 in 4040 ms |
| writer locks | 2 → 0 | 2 → 0 |
| wedged MCP process | survives — still alive 19 s after the daemon is gone; my harness had to SIGKILL it |
reaped — gone by the first sample after exit |
Two things worth stating plainly. First, the strict drain does not produce a false "unclean": exit stays 0, the log still says daemon stopped. Second, this is an unadvertised improvement — on main the daemon exits before the ACP child's pool drain can finish, so the wedged MCP process is simply left behind; here the shutdown waits for the child to be reaped and the drain completes. Reproduced twice per side.
The cost is shutdown latency in that shape: 0.04 s → 4.0 s. It is bounded by the 5 s TERM→KILL escalation, so it cannot run away, but operators with slow-closing MCP servers will notice their pod termination getting slower. Worth a line in the PR description rather than a change.
6. Static checks and test suites (PR head)
| check | result |
|---|---|
tsc --noEmit — core, acp-bridge, cli |
✅ 0 errors in all three |
eslint --max-warnings 0 on all 18 changed TS files |
✅ clean |
npm run check:serve-fast-path-bundle |
✅ Startup bundle closure checks passed. |
packages/core full suite |
18050 passed, 13 skipped, 2 failed† |
packages/acp-bridge full suite (20 files) |
979 passed |
cli acpAgent.test.ts + run-qwen-serve.test.ts |
535 passed |
† Both failures are the session-writer-lease.test.ts cases that depend on chmod restricting access (classifies an unreadable owned lock as unavailable, exposes the owned lease when transcript inspection cleanup must be retried). My shell runs as root, where chmod 000/500 restricts nothing. I re-ran that file on the merge base 2db663bec this round: the same 2 fail, 19 pass, 1 skipped — pre-existing environment artefact, not a regression.
Minor, non-blocking
Unchanged from last round, and I still would not hold the merge for it: the retained-lock warning derives candidate paths from every Config in the terminal, including ones that never acquired a lock — this run printed 3 candidate paths for 2 real locks. The wording already says "Candidate lock paths", so it is cosmetic; filtering to configs that actually held a lease would tighten the operator message.
Recommendation
Approve. The correctness story holds under real execution at the current head, the rebase preserved #7894's opt-in gate in the way that matters, the default configuration is unchanged from main, and the fail-loud behaviour is a real improvement over silently-clean. Please consider adding the MCP-drain observation from §5 to the PR description — it is a user-visible timing change and an unclaimed win.
中文说明
本地验证第 2 轮 —— 真实构建、真实 daemon、真实子进程，基于 head 7396e3e40
这是对我在 1f7203aa1 上那份报告的后续。当时提出的阻塞项 —— 分支已无法与 main 干净合并 —— 已经解决，而且解决方式在语义上是正确的，不是单纯的文本合并。我重新从源码构建了两侧，在当前 head 上重跑了全部测量，并针对这期间的变化新增了三项探测。
结论：LGTM，无阻塞项。这次 rebase 完整保留了 #7894，所有具备区分度的 A/B 依然可复现，并且默认配置下与 main 的行为完全一致。
环境：Linux 6.12.63（Debian 13）· Node v22.22.2 · 在两个隔离 worktree 中执行 npm run build -- --cli-only && npm run bundle —— PR head 7396e3e40 对比其 merge base 2db663bec。以下所有证据没有任何 mock，也没有任何 fake timer。
0. 自我上次审阅以来，实际改动了什么
我对比了两份 net patch —— 923e5ab42…1f7203aa1（我审阅过的）与 2db663bec…7396e3e40（现在提交的）。二者完全一致，只多了一行，而且是测试 fixture：
it('treats managed shutdown during writer acquisition as a clean terminal', async () => {
const config = new Config({
...baseParams,
chatRecording: true,
experimentalZedIntegration: true,
+ sessionWriterLeaseEnabled: true,也就是说，这一轮的全部增量就是与 main 的合并。我当时点名"合并时最容易做错"的三处，现在都处理正确：
| 我当时提的点 | 当前 head |
|---|---|
activateChatRecording() 必须保留 #7894 的开关 |
✅ if (!this.chatRecordingEnabled || !this.sessionWriterLeaseEnabled) return; |
ChatRecordingService 的 writerLeaseRequired 默认值 |
✅ config.isSessionWriterLeaseEnabled?.() ?? … |
| 本 PR 的新增应当叠加在上面，而不是替换 | ✅ 关闭检查点与 reclaimPolicy 都位于被 gate 的函数体内；config.ts 已完全不在 lease-gate 的 diff 里 |
最后一行是最强形式的验证：这个 gate 表达式现在在 diff 两侧完全相同，因此 #7894 不可能被撤销。
1. 决定性的 A/B —— 当锁无法被释放时
把锁目录设为不可变（chattr +i，这样即使 root，精确 owner 的 rename/unlink 也会以 EPERM 失败），创建两个持有 writer 锁的持久化会话，然后发送 SIGTERM。
base 2db663bec |
PR #7812 | |
|---|---|---|
| daemon 退出码 | 0 | 1 |
| writer 锁 | 2 → 2（保留） | 2 → 2（按设计保留） |
| 运维告警 | 无 | qwen --acp: managed session writer shutdown failed; a writer lock may be retained for safety… + 候选锁路径 |
| daemon 日志 | daemon stopped |
daemon shutdown incomplete / ACP child pid=… exited uncleanly during shutdown (code=1, signal=none) |
与上一轮完全一致 —— 这正是重点：这依然是这个 P0 的全部价值所在。在 main 上，泄漏一个 writer 锁与干净关闭无法区分；在滚动替换场景下，这个锁带着外部 hostname，替代实例永远无法证明它已失效，而退出的那个 Pod 的日志里没有任何线索。
2. 托管模式下永不抢锁 —— 独立模式仍会回收
| 植入的锁 owner | base 2db663bec |
PR #7812 |
|---|---|---|
| 已死的本机 pid，相同 hostname | 200 OK —— 锁被抢走，植入的 owner 被替换 |
409 session_writer_conflict，植入的锁原封不动 |
| 外部 hostname（k8s 滚动替换形态） | 409 session_writer_conflict |
409 session_writer_conflict（无变化） |
第二行才是真正重要的回归检查 —— fail-closed 行为被完整保留。另有一轮针对已构建 core dist 的真实文件系统测试确认这是按 policy 区分而非一刀切，3/3 通过：默认 policy 仍会回收可证明已死的本机 owner；reclaimPolicy: 'never' 拒绝同一个锁并让其逐字节不变；release() 以 SessionWriterLostError 拒绝，而不会删除不属于自己的后继锁。
3. 子进程时间线与端到端关闭
用真实 Node 子进程 + 真实墙上时钟驱动 ProcessRegistry —— 5/5：协作式子进程 5 ms 内干净退出；忽略 SIGTERM 的子进程在 5002 ms 被 SIGKILL 并报告为不干净；无法回收的子进程触发 10005 ms 未回收截止；重复调用 shutdown() 在 0 ms 返回同一终态（10 秒截止不会被重启）；draining 期间 reserve() 被拒绝。
打包后的 qwen serve 在 SIGTERM 下（PR head）：单 workspace / 3 会话 —— 锁 3 → 0，退出码 0、耗时 43 ms、0 孤儿，替代 daemon 成功 resume 3× 200 并追加 3× 202。turn 进行中（3 个会话停在永不结束的模型流里）—— 三条 prompt turn completed 出现在 received SIGTERM, draining 之后，锁 3 → 0，退出码 0、耗时 51 ms。primary + secondary workspace —— 2 个 ACP 子进程全部回收，锁 4 → 0，退出码 0、耗时 38 ms。
需要再次说明：正常路径和 turn 进行中这两个场景，base 同样释放全部锁并以 0 退出。它们属于无回归证据，不是对 bug 的复现。§1 与 §2 才是具有区分度的用例。
4. 本轮新增 —— 默认配置下的影响范围
自 #7894 起，experimental.sessionWriterLease 默认为 false，所以我在完全不写这个开关的情况下重跑了全套流程 —— 包括同样的 chattr +i 注入（此时它是空操作，因为根本没有锁需要释放）。这是几乎所有运维实际会跑的配置。
base 与 PR 无法区分：创建 0 个锁、退出码 0、daemon stopped、无 "incomplete"、0 孤儿，替代 daemon resume 2× 200 并追加 2× 202，并且追加的记录在两侧都确实落盘（每个会话 8 条 transcript 记录，追加标记存在）。fail-loud 路径只对显式开启的运维生效 —— 影响范围确实很小；同时它也是 §1 的负对照：同样的注入、同样的构建，在没有 lease 需要释放时退出码为 0。
我还验证了这个 requiresRestart 设置的"冻结"行为，因为 experimental.sessionWriterLease 可以在磁盘上被改写、并被存活的 ACP 子进程通过 POST /workspace/reload 重新读取。在两个方向上翻转后于同一子进程内新建会话：OFF→ON 新会话依然不取锁，ON→OFF 新会话依然取锁 —— 两侧构建行为一致。因此这个冻结是 main 的行为（随 #7894 引入，不是本 PR 带来的），本 PR 完整保留了它；两个方向都不会出现 leased/legacy writer 混用。
5. 本轮新增 —— 关闭时卡死的 MCP server
本 PR 把 shutdownMcpPool() 从"仅告警"改成了抛异常，并且托管路径会严格 drain。我用一个 stdio MCP server 来探测：它正常响应 initialize/tools/list，随后永远忽略 SIGTERM 和 stdin EOF。
base 2db663bec |
PR #7812 | |
|---|---|---|
| daemon 退出 | 0，耗时 39 ms |
0，耗时 4040 ms |
| writer 锁 | 2 → 0 | 2 → 0 |
| 卡死的 MCP 进程 | 存活 —— daemon 退出 19 秒后仍在，最后由我的 harness SIGKILL |
已回收 —— 退出后第一次采样即已消失 |
有两点需要明确说明。第一，严格 drain 不会产生误报的"不干净"：退出码仍是 0，日志仍写 daemon stopped。第二，这是一个 PR 描述里没有提到的改进 —— 在 main 上，daemon 在 ACP 子进程的 pool drain 完成之前就退出了，卡死的 MCP 进程被直接遗留；而这里关闭流程会等待子进程被回收，drain 得以完成。两侧各复现两次。
代价是这种形态下的关闭延迟：0.04 秒 → 4.0 秒。它受 5 秒 TERM→KILL 升级的上界约束，不会失控，但使用关闭较慢的 MCP server 的运维会感觉到 Pod 终止变慢。这一点建议写进 PR 描述，而不是改代码。
6. 静态检查与测试套件（PR head）
| 检查项 | 结果 |
|---|---|
tsc --noEmit —— core、acp-bridge、cli |
✅ 三者均 0 错误 |
对全部 18 个改动 TS 文件执行 eslint --max-warnings 0 |
✅ 干净 |
npm run check:serve-fast-path-bundle |
✅ Startup bundle closure checks passed. |
packages/core 全量套件 |
18050 通过，13 跳过，2 失败† |
packages/acp-bridge 全量套件（20 个文件） |
979 通过 |
cli 的 acpAgent.test.ts + run-qwen-serve.test.ts |
535 通过 |
† 两个失败都来自 session-writer-lease.test.ts 中依赖 chmod 限制访问的用例（classifies an unreadable owned lock as unavailable、exposes the owned lease when transcript inspection cleanup must be retried）。我的 shell 以 root 运行，chmod 000/500 对 root 不产生任何限制。本轮我在 merge base 2db663bec 上重跑了这个文件：同样 2 失败、19 通过、1 跳过 —— 属于既有环境问题，不是本 PR 引入的回归。
次要问题（不阻塞）
与上一轮相同，我也不会因此拖住合并：保留锁的告警会从终态里的每一个 Config 推导候选路径，包括那些从未获取过锁的 —— 本次运行为 2 个真实锁打印了 3 条候选路径。措辞本身已经是 "Candidate lock paths"，所以只是观感问题；过滤为真正持有过 lease 的 config 会让运维信息更精确。
建议
同意合并。 在当前 head 上，正确性在真实执行下成立；rebase 以正确方式保留了 #7894 的 opt-in gate；默认配置下与 main 行为一致；fail-loud 相比"静默干净"是实打实的改进。建议把 §5 的 MCP drain 观察补进 PR 描述 —— 那是一个用户可见的耗时变化，也是一个尚未被认领的收益。
Verified locally with Claude Code (model: claude-opus-5[1m]). Harnesses: fail-injection, reclaim A/B, real-process registry, real-fs lease policy, E2E / mid-turn / multi-workspace shutdown, default-off blast radius, writer-protocol freeze, wedged-MCP drain.
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： Autofix review round — no action takenThe only new feedback on this PR is an issue-level verification report from The report carries two suggestions, both explicitly labelled "Minor, 1. Filter the retained-lock warning's candidate paths to configs that 2. Add the §5 MCP-drain shutdown-latency observation to the PR description — No commit was made and no source files were modified, so no build/typecheck/ 中文说明Autofix 审阅轮次 —— 未采取任何行动本 PR 上唯一的新反馈是来自 @wenshao 的一条 issue 级验证报告。这是一份同意合并的结论："LGTM。无阻塞项。" 该报告在当前 head 该报告附带两条建议，均被明确标注为"次要、不阻塞"。本轮二者均不产生代码改动： 1. 将保留锁告警的候选路径过滤为真正持有过 lease 的 config —— 拒绝（不值得增加 diff）。 2. 将 §5 中 MCP drain 关闭延迟的观察补进 PR 描述 —— 本轮范围之外；已转交 maintainer。 本轮未创建任何提交，也未修改任何源文件，因此本轮无需执行 build/typecheck/lint/test 验证。 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧠 Handled by Qwen Code · model/模型 |
|
Reviewed QwenLM/qwen-code#7812 at The GitHub posting action was cancelled, so no comment was published. ✅ completed |
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: reverse audit — an auditor ran and opened its brief, but no agent was launched with the prompt the CLI built — the launch was written by hand, and what the agent was actually asked is not what this skill certifies.
中文说明
已审查。 建议见行内评论。 未审查：反向审计——有审计 agent 运行并打开了自己的 brief，但没有 agent 是用 CLI 构建的 prompt 启动的——启动 prompt 是手写的，agent 实际被要求做的并不是本 skill 所认证的内容。
— qwen3.7-max via Qwen Code /review
| throw new Error( | ||
| `MCP pool drain incomplete: ${result.forced} forced, ${result.errors.length} errors`, | ||
| ); |
There was a problem hiding this comment.
[Suggestion] shutdownMcpPool now throws when drainAll resolves with forced > 0 || errors.length > 0 (line 3794), but only the rejection path is tested — the existing test "reports managed MCP pool drain failure" uses mockRejectedValueOnce, exercising only the catch (err) → throw err branch. — Failure scenario: MCP pool drainAll resolves with { drained: 5, forced: 1, errors: [] } (slow MCP subprocess). The new throw fires, managed shutdown re-throws, and the ACP child exits code 1 — a path with no test coverage.
Suggested fix: add a test with mockMcpPoolDrainAll.mockResolvedValueOnce({ drained: 0, forced: 1, errors: [] }) that verifies managed shutdown reports unclean exit.
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Thanks — deferring. The coverage observation is valid, but it is a non-blocking missing-test suggestion and this PR is past the roughly five-review-round cutoff; the resolved-forced drain behavior has also been independently exercised with a real wedged MCP process.
| if (teardownFailures.length > 1) { | ||
| throw new AggregateError( | ||
| teardownFailures, | ||
| 'ACP bridge shutdown failed', | ||
| ); | ||
| } |
There was a problem hiding this comment.
[Suggestion] The multi-failure AggregateError branch (teardownFailures.length > 1) is untested — all new shutdown-failure tests produce exactly one failure, exercising only the length === 1 path. — Failure scenario: two alive channels both reject kill() during shutdown; the AggregateError wrapping is never validated by any test, so a future refactor that drops or reorders errors would go undetected.
Suggested fix: add a test where both channels' kill() rejects during bridge.shutdown() and assert the rejection is an AggregateError containing both failures (similar to the construction-path test that asserts AggregateError shape).
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Thanks — deferring. The current code preserves every rejected reason via Promise.allSettled and wraps multiple failures in AggregateError; this is a non-blocking coverage-only suggestion, so under the roughly five-review-round cutoff it is not expanding the PR.
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： No action takenBoth inline suggestions from the automated reviewer were already explicitly deferred by maintainer @doudouOUC in the PR thread:
No other findings require action. Reviewer @yiliang114 confirmed no blocking correctness, security, data-loss, coverage, or quality issues, and CI is green. 中文说明未采取任何操作自动化审查器的两条行内建议均已被维护者 @doudouOUC 在 PR 线程中明确推迟：
无其他需要处理的发现。审查者 @yiliang114 确认无阻塞性的正确性、安全性、数据丢失、覆盖率或质量问题，CI 为绿色。 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧠 Handled by Qwen Code · model/模型 |
|
@qwen-code /triage |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline. 2 Suggestion-level finding(s) could not be anchored to a changed line and were dropped; nothing further to act on here.
中文说明
已审查——无阻断问题。 建议见行内评论。 2 条建议级发现无法锚定到改动行，已丢弃；此处无需进一步处理。
— qwen3.8-max-preview via Qwen Code /review
| throw new Error( | ||
| `MCP pool drain incomplete: ${result.forced} forced, ${result.errors.length} errors`, | ||
| ); | ||
| } | ||
| } catch (err) { | ||
| debugLogger.error( | ||
| `MCP pool drainAll failed: ${err instanceof Error ? err.message : String(err)}`, | ||
| ); | ||
| throw err; |
There was a problem hiding this comment.
[Suggestion] shutdownMcpPool 中新增的 throw new Error(...) 位于 try 块内部，会被紧邻的 catch 自行捕获，导致一条误导性的 "MCP pool drainAll failed" 错误日志——实际上 drainAll() 成功返回了，只是报告了不完整的 drain。— 失败场景：当 drainAll() 成功但报告 forced > 0 时，catch 记录 "MCP pool drainAll failed: MCP pool drain incomplete: Y forced, Z errors"，暗示 drainAll() 本身抛了异常，而实际原因是 MCP server 超时。事故排查时会将调查方向引向 drainAll 异常（网络错误、池损坏），而非真正的原因。
| throw new Error( | |
| `MCP pool drain incomplete: ${result.forced} forced, ${result.errors.length} errors`, | |
| ); | |
| } | |
| } catch (err) { | |
| debugLogger.error( | |
| `MCP pool drainAll failed: ${err instanceof Error ? err.message : String(err)}`, | |
| ); | |
| throw err; | |
| async shutdownMcpPool(timeoutMs = 10_000): Promise<void> { | |
| if (!this.mcpPool) return; | |
| let result: Awaited<ReturnType<typeof this.mcpPool.drainAll>>; | |
| try { | |
| result = await this.mcpPool.drainAll({ force: true, timeoutMs }); | |
| } catch (err) { | |
| debugLogger.error( | |
| `MCP pool drainAll failed: ${err instanceof Error ? err.message : String(err)}`, | |
| ); | |
| throw err; | |
| } | |
| if (result.forced > 0 || result.errors.length > 0) { | |
| debugLogger.warn( | |
| `MCP pool drain: ${result.drained} clean, ${result.forced} timed out, ` + | |
| `${result.errors.length} errors`, | |
| ); | |
| throw new Error( | |
| `MCP pool drain incomplete: ${result.forced} forced, ${result.errors.length} errors`, | |
| ); | |
| } | |
| } |
中文说明
shutdownMcpPool 中新增的 throw new Error(...) 位于 try 块内部，会被紧邻的 catch 自行捕获，产生一条误导性的 "MCP pool drainAll failed" 错误日志。实际上 drainAll() 成功返回了，只是报告了不完整的 drain（有连接超时）。建议将不完整 drain 的 throw 移到 try/catch 之外，使 catch 只处理真正的 drainAll() 异常。
— qwen3.8-max-preview via Qwen Code /review
| } catch (error) { | ||
| if ( | ||
| error instanceof SessionWriterLostError || | ||
| pendingLease.isReleased | ||
| ) { | ||
| this.pendingSessionWriterLease = undefined; | ||
| } | ||
| failures.push(error); | ||
| } |
There was a problem hiding this comment.
[Suggestion] closeSessionWriterOnce 无条件地将 lease 释放错误推入 failures——包括良性的 SessionWriterLostError / 已释放的情况——而 activateChatRecording 中相同的释放清理 catch 块（约第 3006–3019 行）会刻意吞掉该情况，仅升级其他错误。— 失败场景：在 ACP 托管关闭期间（beginManagedShutdown → closeSessionWriter），如果并发进程在 closeSessionWriterOnce 释放 pendingSessionWriterLease 之前回收了 writer 锁，release() 抛出 SessionWriterLostError（lease 实际已消失——releaseOnce 在抛出前设置 released = true）。if 分支正确清除了 pendingSessionWriterLease，但 failures.push(error) 无条件执行，导致 closeSessionWriterOnce 抛出异常。ACP agent 的 writerShutdown promise 被拒绝，产生一个虚假的 "Managed session writer shutdown failed" AggregateError 和一条误导性 stderr 警告（"a writer lock may be retained for safety"），尽管实际上没有锁被保留。
| } catch (error) { | |
| if ( | |
| error instanceof SessionWriterLostError || | |
| pendingLease.isReleased | |
| ) { | |
| this.pendingSessionWriterLease = undefined; | |
| } | |
| failures.push(error); | |
| } | |
| } catch (error) { | |
| if ( | |
| error instanceof SessionWriterLostError || | |
| pendingLease.isReleased | |
| ) { | |
| this.pendingSessionWriterLease = undefined; | |
| } else { | |
| failures.push(error); | |
| } | |
| } |
中文说明
closeSessionWriterOnce 无条件地将 lease 释放错误推入 failures（包括良性的 SessionWriterLostError），而 activateChatRecording 中相同的 catch 块会吞掉该情况。这种不对称会在锁已被并发进程回收时产生虚假的关闭失败报告。建议将 failures.push(error) 移入 else 分支，与 activateChatRecording 保持一致。
— qwen3.8-max-preview via Qwen Code /review
|
Released in v0.21.1. |
What this PR does
This PR adds cooperative shutdown for daemon-managed ACP children. On the first shutdown signal, session and turn admission close synchronously, all transcript work accepted before that cutoff drains, exact-owned writer locks are retired atomically, and SessionEnd hooks and remaining resources are cleaned up only after the writer phase has settled.
It also tracks every ACP child spawned for primary, secondary, and dynamically added workspace runtimes on one daemon-scoped shutdown timeline. Shutdown sends SIGTERM immediately, escalates to SIGKILL after 5 seconds, requires raw process reaping within 10 seconds, and remains unclean if a child exits nonzero, exits by signal, or cannot be confirmed reaped. Partial channel construction and overlapping live-channel teardown now join those same terminal outcomes before returning.
Managed acquisition intentionally never reclaims an existing writer lock from hostname, age, or container-visible PID evidence. Standalone ACP keeps its existing local stale-owner recovery behavior.
Why it's needed
A managed daemon replacement can start on a different hostname while the previous ACP child still owns a v1 session writer lock. If that child is terminated without cooperative cleanup, the replacement cannot safely prove that the foreign-host owner is dead and returns
session_writer_conflictindefinitely.Automatically stealing by lock age or hostname would be unsafe when two Pods overlap on the same workspace. This P0 fixes the normal termination path by making lock release an acknowledged part of managed child shutdown while preserving the existing fail-closed behavior whenever flush, integrity, ownership, or process-reaping evidence is uncertain.
Reviewer Test Plan
How to verify
qwen serve, create persisted sessions that hold writer locks, and send SIGTERM while at least one session remains live. Confirm that new session/turn admission closes, accepted transcript writes finish, every primary writer lock disappears, the ACP child exits with code 0, and a replacement daemon can load and append to the sessions withoutsession_writer_conflict.Evidence (Before & After)
N/A — daemon lifecycle and persistence behavior only; no TUI or Web UI changes.
Tested on
Environment (optional)
macOS (Darwin), Node.js v22.22.3, npm 10.9.8, local bundled
qwen serve, Vitest, and a real managed ACP child-process harness.Risk & Scope
Linked Issues
Part of #7752
中文说明
本 PR 做了什么
本 PR 为 daemon 托管的 ACP 子进程增加协作式关闭。收到第一次关闭信号时，会同步关闭新会话和新轮次准入，排空截止点之前已接受的所有 transcript 工作，以原子方式退役精确属于当前 owner 的 writer 锁；只有 writer 阶段终态确定后，才执行 SessionEnd hook 和其余资源清理。
同时，primary、secondary 和动态新增 workspace runtime 所派生的全部 ACP 子进程会由同一个 daemon 作用域的关闭时间线管理。关闭时立即发送 SIGTERM，5 秒后升级为 SIGKILL，10 秒内必须观察到原始进程回收；如果子进程非零退出、因信号退出或无法确认已回收，关闭结果会稳定保持为不干净。部分 channel 构造失败和重叠存活 channel 的清理也必须等待相同的终态后才能返回。
托管模式下的锁获取明确不会根据 hostname、锁年龄或容器内可见的 PID 证据回收已有 writer 锁。独立 ACP 仍保留现有的本机 stale owner 回收行为。
为什么需要
托管 daemon 的替代实例可能使用不同 hostname 启动，而上一个 ACP 子进程仍持有 v1 session writer 锁。如果旧子进程在未协作清理的情况下被终止，替代实例无法安全证明这个外部 hostname 的 owner 已死亡，因此会无限期返回
session_writer_conflict。当两个 Pod 可能同时挂载同一个 workspace 时，按锁年龄或 hostname 自动抢锁是不安全的。本 P0 把锁释放变成托管子进程关闭确认的一部分，从而修复正常终止路径；只要 flush、完整性、所有权或进程回收证据存在不确定性，仍保持现有的 fail-closed 行为。
Reviewer 测试计划
如何验证
qwen serve，创建持有 writer 锁的持久化会话，并在至少一个会话仍存活时发送 SIGTERM。确认新会话/轮次准入关闭、已接受的 transcript 写入完成、所有主 writer 锁消失、ACP 子进程以退出码 0 结束，并且替代 daemon 能加载会话并继续追加，且不出现session_writer_conflict。证据（修改前与修改后）
N/A——仅涉及 daemon 生命周期与持久化行为，没有 TUI 或 Web UI 变化。
测试平台
环境（可选）
macOS（Darwin）、Node.js v22.22.3、npm 10.9.8、本地打包的
qwen serve、Vitest，以及真实的 managed ACP 子进程 harness。风险与范围
关联 Issue
属于 #7752 的一部分