fix(serve): Add certified session writer handoff - #7976
Conversation
Seal managed writer ownership after a durable recorder drain and allow trusted replacements to take over only when the transcript proof still matches. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Keep the fixed transition claim unless the exact predecessor primary is restored, and avoid rollback after claim ownership changes or unlink completes with a reported error. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Wait for claim-aware primary candidates to back out of transition gaps, and release losing claims when no primary transition started. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Fail closed when a claim-aware primary candidate is abandoned during a handoff transition, preserving the transition fence for authoritative recovery. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Reject dangling transcript paths and reconcile ambiguous lock-record link outcomes by exact bytes before continuing a certified handoff. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
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. |
|
@qwen-code /takeover |
qwen-code-dev-bot
added
the
autofix/takeover
|
🤝 Takeover engaged: the autofix loop now manages this PR — it will address new review feedback and resolve base conflicts until the label is removed or the round cap is reached. Remove the 中文说明🤝 已接管：autofix 循环现在管理此 PR —— 将持续处理新的评审反馈与 base 冲突，直到移除标签或达到轮次上限。移除 |
|
✅ Qwen Triage finished — CI landed green on ✅ Qwen Triage 已完成 —— |
|
Thanks for the PR! Template looks good ✓ Problem: observed and well-evidenced. This continues the writer-lease work from #7812 (cooperative shutdown) toward #7752. The gap is concrete: after managed shutdown, a foreign-host active lock cannot be safely distinguished from a killed/stalled writer when another Pod may share the workspace. Stealing by hostname or timeout risks two concurrent transcript writers. The sealed-handoff protocol gives a successor positive evidence that the previous writer stopped cleanly. Direction: aligned. Daemon-managed ACP writer safety is core infrastructure for multi-Pod deployments. The protocol stays behind the existing Size: 1136 production logic lines (3 in Approach: the scope is disciplined. The design doc explicitly fences out TTL/heartbeat reclaim, hostname stealing, force-steal APIs, and mixed-version operation — all of which would multiply complexity without solving the stated problem. Every edit in the diff serves the sealed-handoff protocol. The Risk: Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 问题：已观测且有充分证据。此 PR 继续 #7812（协作式关闭）的 writer-lease 工作，面向 #7752。差距是具体的：托管关闭后，当另一个 Pod 可能共享 workspace 时，外部 hostname 的 active 锁无法与被强杀/卡住的 writer 安全区分。按 hostname 或超时抢锁可能产生两个并发 transcript writer。sealed-handoff 协议为替代实例提供前任 writer 干净停止的确定证据。 方向：对齐。Daemon 托管 ACP writer 安全性是多 Pod 部署的核心基础设施。协议保持在现有 规模：1136 生产逻辑行（ 方案：范围有纪律。设计文档明确排除 TTL/heartbeat 回收、hostname 抢锁、force-steal API 和混合版本运行——这些都会成倍增加复杂度而不解决所述问题。diff 中的每个编辑都服务于 sealed-handoff 协议。 风险： 进入代码审查 🔍 — Qwen Code · qwen3.8-max-preview Reviewed at |
Code reviewIndependent proposal: for safe writer handoff after managed shutdown, I'd add a sealed lock state carrying a transcript integrity proof (SHA-256 + byte length) that the outgoing writer publishes after draining accepted records. The successor validates the proof against the actual transcript before taking over, using a claim file to serialize the transition and failing closed on any ambiguity. The protocol stays behind the existing experimental gate. Comparison with the diff: the PR matches and exceeds this baseline in three ways I'd want but might not have designed upfront:
The cleanup/rollback logic in No critical blockers found. No AGENTS.md violations — ESM throughout, strict TS, collocated tests, design doc committed under One observation, not blocking: Files changed (9 of 9)
CI test evidenceNo failures on the reviewed commit. The ubuntu unit suite passed; Windows and macOS jobs were skipped (not caused by this PR — appears to be a CI configuration choice). The web-shell E2E smoke test is still in progress but is unrelated to this change (no web-shell files touched). Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 Sandboxed verification would settle the cross-platform gap: 中文说明代码审查独立方案： 为实现托管关闭后的安全 writer 交接，我会添加一个携带 transcript 完整性证明（SHA-256 + 字节长度）的 sealed 锁状态，由离任 writer 在排空已接受记录后发布。替代实例在接管前验证证明与实际 transcript 的一致性，使用 claim 文件序列化转换，并在任何含糊情况下 fail-closed。协议保持在现有实验性开关后面。 与 diff 的比较： PR 在三个方面匹配并超越了此基线：
清理/回滚逻辑详尽—— 未发现关键阻塞问题。无 AGENTS.md 违规。 CI 测试证据审查提交上无失败。Ubuntu 单元测试套件通过；Windows 和 macOS 任务被跳过（非此 PR 导致）。Web-shell E2E 冒烟测试仍在进行中但与此更改无关。 沙箱验证可以弥合跨平台差距： — Qwen Code · qwen3.8-max-preview Reviewed at |
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Full review of the sealed-handoff protocol against 4aa1abe. The core transition machinery is solid: the proof is computed outside the claim, both transitions are serialized by the fixed claim, every error-after-effect is reconciled from exact record bytes, the claim-aware-candidate wait closes the gap-window race, and nothing ever overwrites or deletes an unknown successor. Two racing takeovers electing one winner and the rollback paths both check out on a read. The design doc is genuinely good.
My findings are about what happens to a sealed record after the happy path, not about the transition itself. The protocol is airtight for "old daemon seals, new daemon takes over"; it has no story for "old daemon seals, nobody ever takes over", which is the common case for most sessions on any given shutdown.
Main points (inline):
- A sealed record is strictly less recoverable than the stale active record it replaces —
reclaimPolicy: 'local'never applies to it, and it's terminal 409 for any non-certified acquirer even when the sealing owner is a dead PID on the same host. The evidence is inverted. - Managed shutdown seals every config, and only a certified takeover of that exact session ever removes a sealed record. The lock directory stops being self-cleaning, with no TTL or sweep.
- A residual
.claimis permanently 503 with no bounded retry on the acquire side and no operator breadcrumb naming the file to remove. - The handoff flush-failure path silently diverges from the existing close contract and strands the active lock, with no comment explaining why.
handoffRequestedis sticky and read post-flush, so managed shutdown can retroactively convert an in-flight ordinary close into a seal.
Nits:
!stat.isFile() || stat.isSymbolicLink()—lstatnever reportsisFile() === truefor a symlink, so the second disjunct is dead in all ~8 occurrences (inspectExactRecord,inspectClaimedPrimary,inspectReleasePath,inspectExistingLock,getTranscriptState,openTranscriptProof,validateOpenTranscriptProof).takeOverSealedL1339: when the observed sealed primary is simply gone (current.kind === 'missing', i.e. a racing winner already released), this returns 503 instead ofcontinue-ing the acquire loop, where ordinary acquisition would now succeed. The claim is correctly released on that path, so a retry is safe.- Title/scope:
fix(serve)— as the triage bot noted this is afeat, and the scope iscore, notserve.
Test gaps (Suggestion, not blocking): no case for a sealed record left by a provably dead local owner under reclaimPolicy: 'local', and none for the close/handoff ordering race in point 5.
I did not re-run the suite; findings are from reading the code at the head commit.
中文小结
针对 4aa1abe 的完整审查。转换机制本身是扎实的：proof 在 claim 之外计算、两个转换都由固定 claim 串行化、所有 "操作生效但报错" 的情况都按精确记录字节协调、claim-aware candidate 等待补上了 gap 窗口竞争、并且从不覆盖或删除未知后继。两个 takeover 竞争只产生一个赢家、以及各条回滚路径，通读下来都成立。设计文档质量很高。
我的问题都集中在 sealed 记录进入幸福路径之后会怎样，而不是转换本身。协议对"旧 daemon 封印、新 daemon 接管"是严密的；但对"旧 daemon 封印、之后没有人来接管"没有交代——而在任何一次关闭中，这才是大多数会话的常态。
主要问题（见行内评论）：
- sealed 记录比它所取代的 stale active 记录更难恢复——
reclaimPolicy: 'local'对它完全不生效，即使封印者是本机上已死的 PID，对任何非 certified 的 acquirer 都是永久 409。证据强弱关系被反转了。 - 托管关闭会封印每一个 config，而 sealed 记录只有该会话的 certified takeover 才会清除。锁目录不再自清理，且没有 TTL 或清扫。
- 残留的
.claim会导致永久 503，acquire 侧没有有界重试，也没有告诉运维要删哪个文件的日志线索。 - handoff 的 flush 失败路径悄悄偏离了既有的 close 契约，会遗留 active 锁，且没有注释说明原因。
handoffRequested是粘性的且在 flush 之后才读取，托管关闭可能把一个进行中的普通 close 追溯性地变成 seal。
另有若干 nit 与测试缺口，见上文英文部分。本次未重跑测试套件，结论来自对 head commit 的代码通读。
| if (state.kind === 'sealed') { | ||
| if (normalizedOptions.takeoverPolicy !== 'certified') { | ||
| throw new SessionWriterConflictError(); | ||
| } | ||
| return SessionWriterLease.takeOverSealed( | ||
| lockPath, | ||
| state, | ||
| lockRecord, | ||
| normalizedOptions, | ||
| ); | ||
| } |
There was a problem hiding this comment.
A sealed record is strictly less recoverable than the stale active record it replaces.
This branch runs before the reclaimPolicy check on L1230, and inspectExistingLock (L1513-1515) classifies a record as sealed without any liveness check at all — no hostname comparison, no isProcessAlive, no start-identity check. So for any acquirer that is not takeoverPolicy: 'certified', a sealed record is terminal: session_writer_conflict / 409 forever, regardless of reclaimPolicy: 'local', and regardless of whether the sealing owner is a dead PID on this very host.
That inverts the evidence the protocol is built on. A stale active record — owner died mid-write, transcript tail unknown — is reclaimable on L1234-1288. A sealed record — positive proof that the previous writer closed admission, drained accepted work, and committed an exact digest — is not. And removing a sealed record is exactly as safe as the release path: releaseOnce also just unlinks the primary, and either way the next acquirer reloads the authoritative tail from disk and re-fences it.
Concrete consequences:
- a standalone ACP process with
experimental.sessionWriterLeaseenabled can never open a session that a managed daemon sealed; - if
isTrustedManagedParent()returns false after a restart, or the gate is rolled back and then forward per the doc's rollout section, every sealed session is bricked with no in-product recovery; - the user-visible message is factually wrong.
SessionWriterConflictErrorsays "This session is already open in another Qwen process" for a session that provably has no writer, which sends an operator hunting for a live process that does not exist.
Suggestion: when takeover isn't certified, apply the same liveness check lockStateForRecord already performs and let a provably-dead local owner fall into the existing stale-reclaim path. If you'd rather keep it fail-closed, at minimum return a distinct error kind so the state is diagnosable from the API response instead of being indistinguishable from a genuinely live writer.
| const claimPath = `${lockPath}.claim`; | ||
|
|
||
| for (let attempt = 0; attempt < ACQUIRE_ATTEMPTS; attempt++) { | ||
| await assertPathMissing(claimPath); |
There was a problem hiding this comment.
A transient claim fails the whole acquisition with no retry, and a residual one is unrecoverable and unnamed.
assertPathMissing throws straight out of the ACQUIRE_ATTEMPTS loop, so an acquire that lands inside a transition's claim window returns 503 immediately — even though that window is milliseconds and the symmetric case already has a bounded wait (linkClaimedPrimary → waitForClaimedPrimaryCandidate, up to CLAIMED_PRIMARY_WAIT_ATTEMPTS * MALFORMED_RETRY_DELAY_MS). During a rolling daemon replacement the new daemon's acquire overlaps the old one's seal by construction, so this is precisely when it will fire. Retrying the precheck with the existing MALFORMED_RETRY_COUNT / MALFORMED_RETRY_DELAY_MS backoff before failing would make the two directions symmetric.
Separately, on the unrecoverable case: a SIGKILL between claim install and release leaves the session at permanent 503, and the only breadcrumb is the debugLogger.debug line in acquire() — which logs lockPath and transcriptPath but not the claim path. The doc's recovery story is "manual cleanup only after an authoritative external writer fence", so the failure should at least name the file the operator is expected to remove, at a level they'll actually see.
| for (const config of configList) { | ||
| try { | ||
| writerTerminals.push(config.closeSessionWriter()); | ||
| writerTerminals.push(config.closeSessionWriter({ handoff: true })); |
There was a problem hiding this comment.
The lock directory stops being self-cleaning.
This seals every config the agent knows about, and a sealed record is removed only by a certified takeover of that exact session (session-writer-lease.ts L1367) — there is no TTL and no sweep anywhere. Before this PR a graceful managed shutdown released every lock and left <runtime>/tmp/session-writer-locks/ empty. Now it retains one file per session that was live at shutdown, permanently, for every session that is never reopened — which for a long-running daemon is most of them.
Failed transitions add residue that nothing collects either: .claim, .sealed-candidate.*, .handoff.*, .sealed.*.*, .reclaim.*, .stale.*, and .tmp files are only removed on paths that complete, and several fail-closed branches deliberately leave them in place.
Worth a startup sweep for sealed records and orphaned transition artifacts past some age. Deleting a sealed record is safe for the same reason the release path is safe — it just means the next acquirer does ordinary acquisition and reloads the authoritative tail. Not blocking, but this is unbounded growth in a directory the daemon writes to on every session.
| close(options?: { handoff?: boolean }): Promise<void> { | ||
| if (options?.handoff) { | ||
| this.handoffRequested = true; | ||
| } | ||
| if (this.closePromise) return this.closePromise; |
There was a problem hiding this comment.
The handoff flag is sticky and read after the drain, so it can retroactively convert an in-flight ordinary close into a seal.
this.handoffRequested = true is set before the if (this.closePromise) return this.closePromise early return, and closeOnce reads this.handoffRequested only after await this.flush(). So if a client closes session S normally and managed shutdown fires while that flush is still draining, the already-running closeOnce seals instead of releasing. acpAgent.ts L3438-3456 calls closeSessionWriter({ handoff: true }) on every config it can reach, including ones already being torn down, so the two do overlap in practice.
Not a safety bug — the seal is still correct — but the outcome is a sealed lock left behind for a session the client explicitly closed and that will almost certainly never be reopened, which is exactly the input to the lock-directory accumulation problem. Latching the decision at closeOnce entry, or ignoring options.handoff once closePromise exists, would make it deterministic and match the intent ("normal per-session close still releases its lock").
|
Confidence: 4/5 — solid protocol implementation with thorough edge-case handling; maintainer has approved, deferring only for CI to land green. Going back to my independent proposal: the PR exceeds it. The kept-open FD as TOCTOU-proof identity evidence, exact-record reconciliation for error-after-effect filesystem outcomes, and the The scope is disciplined. The design doc explicitly fences out TTL/heartbeat reclaim, hostname stealing, force-steal APIs, and mixed-version operation. Every edit in the diff serves the sealed-handoff protocol. The test coverage (1146 new lines with fault injection for rename/link/unlink/write/readFile) exercises the ugly paths — racing replacements, corrupted proofs, residual claims, ambiguous filesystem errors — not just the happy path. The previous run deferred for maintainer sign-off on the core-module gate (1136 production lines). @wenshao has since approved the current HEAD, resolving that escalation. Non-blocking observations:
Approval deferred until CI lands green on 中文说明置信度：4/5 —— 扎实的协议实现，边界情况处理详尽；维护者已批准，仅等待 CI 全绿。 回到我的独立方案：PR 超越之。保留打开的 FD 作为防 TOCTOU 身份证据、操作生效但返回错误的精确记录协调、以及 范围有纪律。设计文档明确排除 TTL/heartbeat 回收、hostname 抢锁、force-steal API 和混合版本运行。测试覆盖（1146 新行，含 rename/link/unlink/write/readFile 故障注入）覆盖了棘手路径——竞争替代、损坏证明、残留 claim、含糊文件系统错误——而非仅覆盖正常路径。 上一次运行因核心模块门控（1136 生产行）转交维护者签核。@wenshao 已批准当前 HEAD，解决了该升级。 非阻塞观察：
批准推迟至 CI 在 — Qwen Code · qwen3.8-max-preview Reviewed at |
|
🔀 Base updated: red check(s) [review-pr] pass on current main — merged current main via update-branch; CI will re-run. 中文说明🔀 已更新 base：红色检查 [review-pr] 在当前 main 上通过 —— 已通过 update-branch 合入当前 main，CI 将重新运行。 |
|
🤖 AutoFix ran out of time before finishing (timeout (3000000ms)) (attempt 1/100) — it will retry on the next scan. What I found before stopping: See the Qwen Autofix agent step logs for model/tool output. Run log: https://github.com/QwenLM/qwen-code/actions/runs/30469249592 🧠 Handled by Qwen Code · model/模型 |
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. Unresolved, please confirm: [Critical] chatRecordingService.ts:1185-1188 — existing blocker (comment 3675076197): the handoff flush-failure path (if (this.handoffRequested && flushFailure !== undefined) { this.state = 'integrity_failed'; throw flushFailure; }) returns without releasing the lease, diverging from the adjacent non-handoff close path that releases first; the reviewer asked for an inline comment explaining why. The behavior is intentional per docs/design/certified-session-writer-handoff.md, but no inline comment was added at the divergence, so the thread cannot be certified resolved from the code alone.
— qwen3.8-max-preview via Qwen Code /review
| sealForHandoff(): Promise<void> { | ||
| this.terminalPromise ??= this.runExclusive(() => this.sealForHandoffOnce()); | ||
| return this.terminalPromise; | ||
| } |
There was a problem hiding this comment.
[Suggestion] release() and sealForHandoff() share a single terminalPromise via ??=. If sealForHandoffOnce() commits the transition but then fails (e.g. the post-transition validateOpenTranscriptProof throws) and the rollback restores the active primary, released stays false while terminalPromise is left holding the rejected seal promise. A later release() on the same lease then hits the ??= no-op and returns that rejected promise instead of running releaseOnce() — Concrete cost: the active lock can no longer be released programmatically on that lease object, and recovery falls back to stale-lock detection after the process dies. I verified this is currently latent: no production caller releases after a failed seal (chatRecordingService.closeOnce re-throws without releasing, and config's pendingLease.release() fallback only targets pendingSessionWriterLease, which is cleared once the recorder activates). Still, the shared field couples two semantically distinct terminal operations, so a future caller that releases after a failed seal would be silently blocked. Consider giving sealForHandoff() its own promise field (or resetting terminalPromise when the seal fails and the primary is restored) so a failed seal cannot poison a subsequent release.
— qwen3.8-max-preview via Qwen Code /review
There was a problem hiding this comment.
Thanks — deferring. The shared terminal promise is a real but currently latent coupling: there is no production caller that releases after a failed seal, and failed handoff intentionally retains the active lock. This PR has already exceeded roughly five review rounds, so we are limiting further changes to Critical fixes; splitting terminal-operation state belongs in follow-up work.
Local verification report — real build, real daemons, real lock filesVerified head Verdict: the protocol does what the PR claims. All 12 multi-process scenarios and all 4 real-daemon scenarios behave exactly as the design doc specifies, and the safety-critical guards are provably load-bearing. I found one real behavioural regression (below) that is not a merge blocker — the flag is default-off — but should be documented before anyone enables it. 1. The PR's own suites, re-run on the final head
The 1 skip is 2. Multi-process handoff against real lock filesTwelve scenarios, each writer a separate OS process against a real lock + transcript — covering every bullet in your design doc's Verification section.
Worth calling out C6: I swapped transcript content while keeping the byte length exactly identical. 3. Real
|
| 测试 | 结果 |
|---|---|
session-writer-lease.test.ts |
57 通过，1 跳过 |
chatRecordingService.test.ts |
65 通过 |
config.test.ts |
468 通过 |
| core 合计 | 590 通过，1 跳过 —— 与你报告的数字一致 |
acpAgent.test.ts (cli) |
340 通过（你报告 335，head 之后有推进） |
core tsc --noEmit、eslint、prettier（全部改动文件） |
均干净 |
那 1 个跳过是 it.runIf(process.platform === 'linux')，即 Linux 专属分支在此未执行，与你表格中 Linux/Windows 的
2. 针对真实锁文件的多进程 handoff
12 个场景，每个 writer 都是独立操作系统进程，操作真实锁与真实 transcript，覆盖设计文档 Verification 一节的每一条。
其中 C6 值得强调：我在保持字节长度完全不变的前提下替换了 transcript 内容。byte_length 仍然匹配，只有 SHA-256 捕获了改动 —— 说明摘要校验是真正起作用的，而不是长度检查的装饰。
3. 真实 qwen serve daemon —— 你的 Reviewer 测试计划第 1–2 步
两个真实 daemon、真实托管 ACP 子进程、真实托管关闭。磁盘上的 sealed 记录所含摘要与实际 transcript 逐字节一致，替代 daemon 成功接管并继续写入，未出现 session_writer_conflict。
4. 对照实验 —— 这些测试是否真的有效？
绿色测试本身不能说明问题，因此我用「被破坏的协议」来度量测试的检出能力。
- 基线源码 A/B：覆盖回 PR 前源码但保留你的测试 —— 36 个测试失败（core 35 + CLI 1），确属有区分度的测试，而非凑数的回归测试。
- M1（去掉
sha256判断项）与 M2（去掉 certified 接管守卫）均被你的测试与我的 harness 杀死。 - M3（在
takeOverSealed中忽略已存在的.claim）存活。我做了追踪而非直接判定为覆盖缺口：transitionExactPrimary→assertExactTransitionClaim会在下游重新校验精确 claim 字节，残留 claim 仍然 fail-closed 返回 503 且被保留。该守卫属于冗余的纵深防御，不是测试缺失。 - 协议带来的收益（基线实测）：当 transcript 被外部篡改后进行托管替换，基线以
ok=true, errorKind=none直接获取并在被篡改文件上继续追加；本 PR 返回session_transcript_changed/409，且 sealed 记录逐字节不变。
5. 发现 —— 普通 qwen --resume 会使 seal 失效并永久围栏住 daemon
在两个分支上用真实 daemon 与真实构建 CLI 执行同一序列（开关开启）：
qwen servedaemon 记录会话 → 2. 托管关闭 → 3. 用户在普通终端恢复该会话（qwen -r <id> -p "…"）→ 4. daemon 重新启动。
- 基线：daemon 正常获取锁并继续记录，可自愈。
- 本 PR：daemon 被
-32022 session_transcript_changed拒绝，且是永久性的。锁保持sealed，之后每个 daemon 都会算出同样的不匹配，必须由运维手动删除锁文件才能恢复。
根因：sealed 凭证假定 transcript 只会被持有 lease 的 writer 写入，但交互式/headless recorder 会在完全不获取 lease 的情况下写同一个文件 —— 我确认开关开启时普通 headless 运行创建的锁文件数量为 0。因此第 3 步会静默破坏凭证。
这是 fail-closed，符合你设计的方向（不会出现双写、不会损坏数据）。但它可由普通用户操作触达（任何同时使用 daemon 与终端 CLI 的 workspace），并把一个原本可自愈的状态变成需要人工介入的状态。该情况未出现在设计文档的失败契约表或 Risk & Scope 中。
对本 PR 的最小建议：把该情况补入失败契约表，并在 Risk & Scope 中说明。更彻底的修法 —— 让 seal 对非 lease recorder 也具备权威性，即交互路径拒绝写入已 sealed 的会话 —— 看起来适合作为后续工作；我不建议放宽接管逻辑去接受「仅追加增长」的 transcript，因为那恰恰就是双 Pod 威胁模型的表现形式。
合并参考结论
本次证据较充分地支持协议本身的正确性，失败路径均按设计 fail-closed，测试也确实锁定了这些行为。上述发现属于默认关闭的实验性开关的发布/文档事项，不构成阻断。另外附议 triage bot 的意见：fix(serve): 低估了改动性质，应为 feat。
|
🔀 Base updated: red check(s) [Test (ubuntu-latest, Node 22.x)] pass on current main — merged current main via update-branch; CI will re-run. 中文说明🔀 已更新 base：红色检查 [Test (ubuntu-latest, Node 22.x)] 在当前 main 上通过 —— 已通过 update-branch 合入当前 main，CI 将重新运行。 |
|
Review triage for head
No code changes in this round. The protocol verification evidence is otherwise positive. |
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： No code changes this roundAll feedback on head
No failed CI checks require attention. 中文说明本轮无代码变更针对 head
无需关注的 CI 失败检查。 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧠 Handled by Qwen Code · model/模型 |
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. Unresolved, please confirm: [Critical] chatRecordingService.ts:1188 (existing blocker, comment 3675076197): the handoff flush-failure path retains the active lock and diverges from the adjacent release-then-throw close path. The behavior is documented as intentional fail-closed in docs/design/certified-session-writer-handoff.md and the maintainer's verification report confirms the protocol fails closed as designed, so it could not be ruled a defect; but the inline comment explaining the divergence that the reviewer asked for is absent (file unchanged since the comment), so the thread cannot be certified resolved from the code alone. [Critical] Issue-level comment 5121142364 (@wenshao): a plain qwen --resume writes the transcript without a lease, invalidating a seal and permanently fencing the daemon with session_transcript_changed. The finder explicitly rules this NOT a merge blocker (a documentation/rollout item on a default-off experimental flag); the resume-fencing case does not appear in the design doc's failure-contract table or the PR's Risk & Scope, so documentation sufficiency could not be certified. Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
中文说明
已审查。 建议见行内评论。 未决，请确认：[Critical] chatRecordingService.ts:1188 (existing blocker, comment 3675076197): the handoff flush-failure path retains the active lock and diverges from the adjacent release-then-throw close path. The behavior is documented as intentional fail-closed in docs/design/certified-session-writer-handoff.md and the maintainer's verification report confirms the protocol fails closed as designed, so it could not be ruled a defect; but the inline comment explaining the divergence that the reviewer asked for is absent (file unchanged since the comment), so the thread cannot be certified resolved from the code alone. [Critical] Issue-level comment 5121142364 (@wenshao): a plain qwen --resume writes the transcript without a lease, invalidating a seal and permanently fencing the daemon with session_transcript_changed. The finder explicitly rules this NOT a merge blocker (a documentation/rollout item on a default-off experimental flag); the resume-fencing case does not appear in the design doc's failure-contract table or the PR's Risk & Scope, so documentation sufficiency could not be certified. 未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
— qwen3.8-max-preview via Qwen Code /review
|
🤖 Addressed the latest review feedback (round 2/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 2/100 轮）。改动内容与我反驳保留之处如下： Autofix review round summary — PR #7976All three feedback points are addressed. No conflict ( Feedback points1. [Critical] chatRecordingService.ts:1188 (comment 3675076197) — fail-closed divergence lacks an explanatory inline comment
2. [Critical] Issue-level comment 5121142364 (@wenshao) — non-lease
3. [Suggestion] rc:3677135152 session-writer-lease.ts:953 — swallowed I/O error discarded as
Conflict notesNone — Verification
中文说明Autofix 审查轮次总结 — PR #7976三个反馈点均已处理。无冲突（ 反馈点1. [Critical] chatRecordingService.ts:1188（评论 3675076197）— fail-closed 分歧缺少解释性行内注释
2. [Critical] Issue 级评论 5121142364（@wenshao）— 未文档化的非租约
3. [Suggestion] rc:3677135152 session-writer-lease.ts:953 — 被吞掉的 I/O 错误未作为
冲突说明无 — 验证
Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. · 有空请复审；第 100 轮后本 bot 停止并将 PR 交给人工。 🧠 Handled by Qwen Code · model/模型 |
|
Qwen Code review timed out. Qwen review timed out after 10800 seconds (of the 180-minute budget). For large PRs, retry with a longer timeout by commenting: |
Round-2 local verification — head
|
| arm | variant=lock (removeClaimedPrimary:983) |
variant=retired (rollbackExactTransition:1089) |
|---|---|---|
pre-round-2 580203927a |
discarded | discarded |
| this commit | EACCES | EACCES |
minus { cause } at :983 |
discarded | EACCES |
minus { cause: retired.cause } at :1089 |
EACCES | discarded |
Orthogonal, so each probe binds to exactly one new site and both new sites are individually load-bearing. The surfaced chain is SessionWriterUnavailableError → AggregateError → SessionWriterUnavailableError → Error(EACCES, syscall=open) — an operator now gets the path and the syscall instead of "ownership could not be verified".
One detail worth noting: the copy-chmod-rename in my probe is deliberate. The primary lock and its .claim are hard links to one inode, so a plain chmod blinds the claim check first and short-circuits before rollback — which is exactly how I found note (b) below.
4. My round-1 finding, re-verified here, and the new doc paragraph
Re-run end to end on this head with a freshly rebuilt dist/cli.js (I confirmed the bundle carries the round-2 code shape): daemon session → managed shutdown seals → user runs qwen -r <id> -p … in a plain terminal → the transcript grows from 1705 to 3448 bytes, the lock stays sealed and untouched (the plain CLI takes no lease), and the returning daemon is refused with session_transcript_changed, gen-2 unrecorded. Unchanged from round 1, as expected for a comment-and-doc commit.
I also measured the two things the new paragraph asserts:
- Permanence — 3 consecutive certified restarts all get
-32022, and the residual record stayssealedand byte-identical each time. - No policy self-heals it —
reclaimPolicy:'local'still getssession_transcript_changed; atakeoverPolicy:'never'writer getssession_writer_conflict. - What recovers it — removing the residual
.lockout of band, after which acquire succeeds and re-seals with a digest matching the grown transcript.
So the paragraph is accurate, and "authoritative external writer fence" is this doc family's existing term for that out-of-band action (already used at lines 32, 82, 137 and in managed-session-writer-shutdown.md), not a new promise. This closes my round-1 doc item — the fail-closed contract, the ordinary-user path into it, and the rollout constraint are all now written down.
5. Two non-blocking notes
(a) The observability fix is partial. Counting the places an inspect result can come back 'unknown' and produce an error: seven. Two now carry the real errno (:983, :1089 — this commit), one already carried the triggering operation's errno (transitionExactPrimary:1029), and four still substitute a synthesised message and drop it (:829, :907, :1415, :1831). From the same probe run, an EACCES on the .claim record still vanishes: the deepest cause is Error('Session writer transition claim ownership is unreadable') synthesised at session-writer-lease.ts:1419. Visible in the variant=lock run too — the injected failure was replaced by a bare SessionWriterUnavailableError at finishAcquisition:1498, dropping removeOwnedLock's own errno. Same diagnosability gap the reviewer flagged, one layer over. Cheap follow-up, no behaviour change.
(b) Nothing tests this commit. If someone later reverts either { cause }, the suite stays green. If you want one guard, the onOwnershipAcquired + chmod(0000) lever above reaches the rollback path deterministically in a plain temp dir — happy to contribute that test separately rather than hold this PR.
Environment
macOS 24.6 (arm64), Node v22, isolated worktree at 5fa5c355ca, dist/cli.js rebuilt from this head, isolated HOME per daemon run, model endpoint pointed at a dead port so transcripts are written without any network. Not verified: Linux and Windows behaviour (the runIf skip), and the multi-Pod K8s topology the design doc motivates — my foreign-hostname coverage is synthetic (C10).
中文版本（点击展开）
第二轮本地验证 —— head 5fa5c355ca，真实构建、真实守护进程、真实锁文件
这是对我第一轮报告（head dd21ab46c7）的后续。同一个隔离 worktree，从本 head 重新构建（macOS 24.6、Node v22、APFS）。第二轮改了三处：chatRecordingService.ts 中的解释性注释、session-writer-lease.ts 中两个 inspect 辅助函数的 { state, cause } 重构、以及回应我第一轮发现的设计文档段落。
结论：可以合并。 重构被证明不改变任何可观测行为；新增的 error cause 确实可达且各自起作用；新增文档段落与实测一致。文末两条均为非阻塞说明，都不是对本 PR 的要求。
1. 在新 head 上全部重跑
PR 描述引用的三个 core 套件：590 通过 / 1 跳过，与你的数字一致；acpAgent.test.ts：340 通过（你描述里写的是 335 —— head 之后有变动）；改动文件的 tsc --noEmit、eslint、prettier --check 全部干净。那 1 个跳过是 it.runIf(platform === 'linux')，所以 Linux 分支在此仍未执行 —— 与你标注的 .lock 文件）依然 12/12。
2. { state, cause } 重构是否行为中立？
本次提交改了 inspectExactRecord / inspectClaimedPrimary 的返回结构并更新了 13 处调用点，所以值得回答的问题不是"测试是否通过"，而是"有没有任何可观测行为发生位移"。
同一套 harness、同一台机器，仅把本次提交涉及的两个文件换回 580203927a：两侧都是 12/12，归一化后差异为 0，覆盖封印、接管、竞争、篡改检测、残留 claim、跨主机等结果。忘记解构的调用点会被 TypeScript 抓住（对象与字符串字面量比较），我也确认 13 处都显式读取了 state —— 但真正排除语义偏移的是这个差分实验。
同一面板中的对照：你的套件在第二轮之前的源码上同样完全通过（122 通过 / 1 跳过），也就是说没有任何测试能区分本次提交。对行为中立的重构而言这是预期结果；但同时说明新的 cause 管路没有测试守护，这正是第 3 节的内容。
3. 新的 cause 是否真的带上了 errno？
评审者的意见是：被吞进 'unknown' 的 EACCES/EIO 不可见。所以我把一个真实的 EACCES 打进那条路径 —— 不用 mock，不用 fs 桩。切入点是调用方自己的 onOwnershipAcquired 钩子：它在主记录迁移提交之后、lease 返回之前触发，恰好就是回滚路径运行的窗口。在钩子内 chmod(0000)，权限错误就落在 inspect 辅助函数读取的位置。
| 实验组 | variant=lock（removeClaimedPrimary:983） |
variant=retired（rollbackExactTransition:1089） |
|---|---|---|
第二轮之前 580203927a |
丢弃 | 丢弃 |
| 本次提交 | EACCES | EACCES |
去掉 :983 的 { cause } |
丢弃 | EACCES |
去掉 :1089 的 { cause: retired.cause } |
EACCES | 丢弃 |
结果正交：每个探针只绑定一个新站点，且两个新站点各自都是承重的。最终暴露的链是 SessionWriterUnavailableError → AggregateError → SessionWriterUnavailableError → Error(EACCES, syscall=open) —— 运维拿到的是路径和系统调用，而不是一句"ownership could not be verified"。
一个细节：探针里的 copy-chmod-rename 是刻意的。主锁与它的 .claim 是同一 inode 的硬链接，直接 chmod 会先让 claim 检查失明、在回滚之前短路 —— 下面的说明 (b) 正是这样发现的。
4. 第一轮发现在本 head 的复验，以及新增文档段落
在本 head 上用重新构建的 dist/cli.js 端到端重跑（我确认 bundle 携带了第二轮的代码形状）：守护进程会话 → 受管关闭封印 → 用户在普通终端执行 qwen -r <id> -p … → transcript 从 1705 字节增长到 3448 字节，锁仍为 sealed 且未被触碰（普通 CLI 根本不取 lease），回来的守护进程被 session_transcript_changed 拒绝，第二代记录未落盘。与第一轮一致 —— 对一个只含注释与文档的提交来说正是预期。
我还实测了新段落断言的两件事：
- 永久性 —— 连续 3 次认证重启全部得到
-32022，残留记录每次都保持sealed且逐字节相同。 - 没有任何策略能自愈 ——
reclaimPolicy:'local'仍得到session_transcript_changed；takeoverPolicy:'never'的 writer 得到session_writer_conflict。 - 什么能恢复 —— 带外删除残留
.lock，之后 acquire 成功，并按增长后的 transcript 重新封印，摘要匹配。
所以该段落是准确的；而 "authoritative external writer fence" 是这一系列设计文档既有的术语（第 32、82、137 行以及 managed-session-writer-shutdown.md 中已在使用），并非新承诺。我第一轮提的文档项到此闭环 —— 失败契约、普通用户如何撞上它、以及发布约束都已写入。
5. 两条非阻塞说明
(a) 可观测性修复是局部的。 统计 inspect 结果可能返回 'unknown' 并据此抛错的位置共 7 处：2 处现在携带了真实 errno（:983、:1089，即本次提交），1 处原本就携带触发操作自身的 errno（transitionExactPrimary:1029），其余 4 处仍以合成消息替代、把 errno 丢掉（:829、:907、:1415、:1831）。同一次探针运行中，.claim 记录上的 EACCES 依然消失：最深层 cause 是 session-writer-lease.ts:1419 合成的 Error('Session writer transition claim ownership is unreadable')，真实 errno 已丢。在 variant=lock 那次运行里也能看到 —— 注入的失败在 finishAcquisition:1498 被替换成裸的 SessionWriterUnavailableError，removeOwnedLock 自己的 errno 被丢弃。与评审者指出的是同一类诊断缺口，只是上移了一层。后续补一刀即可，无行为变化。
(b) 没有任何测试覆盖本次提交。 如果日后有人回退任一处 { cause }，套件依然全绿。若你想加一道守护，上面 onOwnershipAcquired + chmod(0000) 的手法可以在普通临时目录里确定性地到达回滚路径 —— 我可以另开 PR 贡献这个测试，不必卡住本 PR。
环境
macOS 24.6（arm64）、Node v22、5fa5c355ca 的隔离 worktree、从本 head 重建的 dist/cli.js、每次守护进程运行使用隔离 HOME、模型端点指向死端口以便在完全无网络的情况下写入 transcript。未验证：Linux 与 Windows 行为（即那个 runIf 跳过），以及设计文档所针对的多 Pod K8s 拓扑 —— 我的跨主机覆盖是合成的（C10）。
|
🔀 Base updated: red check(s) [review-pr] pass on current main — merged current main via update-branch; CI will re-run. 中文说明🔀 已更新 base：红色检查 [review-pr] 在当前 main 上通过 —— 已通过 update-branch 合入当前 main，CI 将重新运行。 |
|
@qwen-code /triage |
|
Triage re-run completed without a new review.
The stage comments above were updated with the latest result. View workflow run. |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship — CI landed green after the review. ✅
|
Released in v0.21.2. |
4 participants
What this PR does
This PR adds an integrity-protected handoff protocol for daemon-managed session writers. After managed shutdown closes admission and durably drains accepted recorder work, the current writer publishes a schema-v2 sealed lock containing the runtime-relative transcript key, existence, byte length, SHA-256 digest, sealing time, and previous-owner diagnostics.
A trusted managed replacement can take ownership only from that sealed state. It opens and hashes the expected transcript, keeps the file descriptor as an identity proof, acquires a fixed exclusive claim, revalidates the exact sealed record and transcript, and installs its active record without overwriting an unknown successor. Exact-record reconciliation covers ambiguous filesystem error-after-effect cases, and racing replacements elect at most one winner.
The protocol remains behind the existing
experimental.sessionWriterLeasegate, which is disabled by default. Normal per-session close still releases the lock, standalone ACP behavior is unchanged, schema-v1 records remain active-only compatibility records, and unsealed active locks are never reclaimed from hostname, PID visibility, age, or apparent process death.Why it's needed
The cooperative shutdown added in #7812 fixes the ordinary graceful replacement path, but a foreign-host active lock still cannot safely be distinguished from a writer that was killed or stalled while another Pod may share the workspace. Stealing such a lock by hostname or timeout could create two concurrent transcript writers.
An explicit sealed state gives a successor positive evidence that the previous managed writer stopped admission, flushed accepted records, and committed an exact transcript proof. This allows automatic recovery after future graceful daemon replacements while preserving fail-closed behavior for SIGKILL, crashes, storage uncertainty, proof mismatches, and residual transition claims.
Reviewer Test Plan
How to verify
experimental.sessionWriterLease, start a trusted managed ACP session, append transcript records, and perform managed shutdown. Confirm that accepted writes drain and the active lock becomes a sealed schema-v2 record with the expected runtime-relative transcript identity, byte length, and SHA-256.session_writer_conflict.Evidence (Before & After)
N/A — daemon persistence and lock-protocol behavior only; no TUI or Web UI changes.
Tested on
Environment (optional)
macOS (Darwin), Node.js v22.22.3, npm 10.9.8, Vitest, and the repository build/typecheck toolchain.
Risk & Scope
Linked Issues
Continues #7812.
Part of #7752.
中文说明
本 PR 做了什么
本 PR 为 daemon 托管的 session writer 增加受完整性保护的 handoff 协议。托管关闭停止准入并持久化排空已接受的 recorder 工作后，当前 writer 会发布 schema-v2 sealed 锁，其中包含 runtime 相对 transcript key、是否存在、字节长度、SHA-256 摘要、封印时间和前任 owner 诊断信息。
受信任的托管替代实例只能从这个 sealed 状态接管。它会打开并计算预期 transcript 的摘要，保留文件描述符作为身份凭证，获取固定排他 claim，重新校验精确的 sealed 记录和 transcript，并在不覆盖未知后继记录的前提下安装 active 记录。协议会按精确记录协调文件系统“操作实际生效但返回错误”的含糊结果，多个替代实例竞争时最多只能产生一个赢家。
该协议仍受现有
experimental.sessionWriterLease开关控制，默认关闭。普通单会话关闭仍会释放锁，独立 ACP 行为保持不变，schema-v1 记录仍只是兼容性的 active 记录，未封印的 active 锁绝不会根据 hostname、PID 可见性、锁年龄或表面上的进程死亡自动回收。为什么需要
#7812 增加的协作式关闭修复了普通优雅替换路径，但当多个 Pod 可能共享 workspace 时，外部 hostname 的 active 锁仍无法与被强杀或卡住的 writer 安全地区分。按 hostname 或超时抢锁可能产生两个并发 transcript writer。
显式 sealed 状态为替代实例提供确定证据，证明前一个托管 writer 已停止准入、flush 已接受记录并提交精确 transcript 凭证。这样可以在未来的 daemon 优雅替换后自动恢复，同时对 SIGKILL、崩溃、存储不确定、凭证不匹配和残留 transition claim 继续保持 fail-closed。
Reviewer 测试计划
如何验证
experimental.sessionWriterLease，启动受信任的托管 ACP 会话，追加 transcript 记录并执行托管关闭。确认已接受写入被排空，active 锁变成 sealed schema-v2 记录，且 runtime 相对 transcript 身份、字节长度和 SHA-256 与预期一致。session_writer_conflict。证据（修改前与修改后）
N/A——仅涉及 daemon 持久化与锁协议行为，没有 TUI 或 Web UI 变化。
测试平台
环境（可选）
macOS（Darwin）、Node.js v22.22.3、npm 10.9.8、Vitest，以及仓库 build/typecheck 工具链。
风险与范围
关联 Issue
继续 #7812。
属于 #7752 的一部分。