feat: Gate session writer lease behind opt-in - #7894
Conversation
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
Qwen Code review did not complete successfully. Run review failed. See workflow logs for details. A transient error is retried automatically; if you are seeing this, retry with |
|
✅ Qwen Triage finished — CI landed green on ✅ Qwen Triage 已完成 —— |
|
Thanks for the PR! Template looks good ✓ Problem: This is a real operational concern, not theoretical hardening. #7237 (merged) auto-enables the session writer lease for all ACP sessions, but the protocol is only safe when every concurrent writer participates. During a rolling deployment or rollback, mixed-version processes that don't understand the lease will corrupt session history. The PR provides a before/after explanation and links the originating PR as evidence. Direction: Aligned. Gating a safety-critical cross-process protocol behind an explicit opt-in is standard rollout practice — it lets operators drain old writers and enable the protocol consistently before restarting. This is squarely within qwen-code's session persistence mission. Size: 48 production logic lines (config plumbing + schema + ACP snapshot), 159 test lines, 11 docs lines, 5 generated schema lines. Well within bounds — no maintainer escalation needed. Approach: The scope is tight and every edit serves the stated goal. The strict Moving on to code review. 🔍 中文说明感谢贡献! 模板完整 ✓ 问题:这是一个真实的运维问题,不是理论性加固。#7237(已合并)对所有 ACP 会话自动启用写入租约,但该协议只有在所有并发写入方都参与时才安全。滚动部署或回滚期间,不理解租约的混合版本进程会损坏会话历史。PR 提供了 before/after 说明并关联了原始 PR 作为证据。 方向:对齐。将安全关键的跨进程协议放在显式开关后面是标准的发布实践——允许运维人员排空旧写入进程,统一启用协议后再重启。这完全在 qwen-code 会话持久化的核心使命内。 规模:48 行生产逻辑(配置管道 + schema + ACP 快照),159 行测试,11 行文档,5 行生成 schema。远在阈值内——无需维护者关注。 方案:范围紧凑,每处改动都服务于既定目标。严格的 进入代码审查 🔍 — Qwen Code · qwen3.8-max-preview Reviewed at |
Code ReviewIndependent proposal: Given the goal — gate the session writer lease behind an explicit opt-in — I would add a Comparison with the diff: The PR does exactly this. The implementation is clean and minimal:
No critical blockers. No AGENTS.md violations. No over-abstraction or duplication. CI Test EvidenceFinal CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行(取最新一次运行),省略 skipped,失败项排在最前。 Precheck (lint, format, typecheck) passed. Ubuntu unit tests are still running; macOS and Windows are skipped pending ubuntu completion. The Not verified: live ACP/daemon lease behavior (requires multi-process ACP setup not available in this review context). The author reports macOS testing; Windows and Linux were not tested by the author. 中文说明代码审查独立方案与 PR 实现完全一致。改动干净、最小化:
无关键阻塞项。无 AGENTS.md 违规。无过度抽象或重复。 CI 测试证据precheck(lint、格式化、类型检查)通过。ubuntu 单元测试仍在运行中;macOS 和 Windows 等待 ubuntu 完成后执行。 未验证:实际 ACP/daemon 租约行为(需要多进程 ACP 环境,本次审查上下文中不可用)。作者报告了 macOS 测试;Windows 和 Linux 未经作者测试。 — Qwen Code · qwen3.8-max-preview Reviewed at |
|
Confidence: 5/5 — Clean across every stage; would merge without hesitation once CI lands. This is a textbook feature gate. The problem is real (#7237 auto-enables a cross-process protocol that's only safe when all writers participate), the solution is 48 lines of config plumbing with a strict boolean check and a process-stability snapshot, and the test coverage hits every gate-negative case plus both reload directions. The design doc and user docs are updated in the same PR. Nothing extraneous, nothing missing. The one behavioral note: this changes the default from "lease active for all ACP sessions" to "lease off unless explicitly opted in." The PR calls this out clearly in the migration notes, and it's the correct tradeoff — a partially-participating lease is worse than no lease. Approval deferred until CI lands green on 中文说明置信度:5/5 — 每个阶段都很干净;CI 通过后即可合并。 这是一个教科书级的功能开关。问题真实存在(#7237 自动启用了一个只有所有写入方都参与才安全的跨进程协议),方案是 48 行配置管道加严格布尔检查和进程稳定性快照,测试覆盖了所有门控否定场景和两个重载方向。设计文档和用户文档在同一 PR 中更新。没有多余内容,也没有遗漏。 唯一的行为变化:默认值从"所有 ACP 会话启用租约"变为"除非显式启用否则关闭"。PR 在迁移说明中清楚说明了这一点,这是正确的权衡——部分参与的租约比没有租约更危险。 审批延迟至 CI 在 — Qwen Code · qwen3.8-max-preview Reviewed at |
|
@qwen-code /takeover |
ytahdn
left a comment
There was a problem hiding this comment.
Found one rollout-safety blocker. The restart-required lease gate is currently re-read for each ACP/daemon session, so a running process can contain both legacy and leased writers after the setting changes.
doudouOUC
left a comment
There was a problem hiding this comment.
整体 LGTM,改动本身正确,没有发现功能性 bug。以下是验证记录和几条建议。
已验证
| 项 | 结果 |
|---|---|
packages/core 全量 tsc --noEmit |
✅ |
| ESLint / Prettier(改动文件 + 两个 md) | ✅ |
scripts/generate-settings-schema.ts 后 git status |
✅ 无 drift |
core: config.test.ts + session-writer-lease.test.ts + chatRecordingService.test.ts |
✅ 528 passed / 1 skipped |
cli: settingsSchema.test.ts + settingsUtils.test.ts |
✅ 118 passed |
cli: acp-integration/acpAgent.test.ts |
✅ 323 passed |
cli: config/config.test.ts |
@qwen-code/channel-github 解析失败,在 main 上同样失败,属本地 workspace 未 install 的环境问题,与本 PR 无关 |
关键降级路径逐条走查,确认安全:
- 构造顺序:
sessionWriterLeaseEnabled在experimentalZedIntegration之后赋值;构造函数内没有任何地方提前触发getChatRecordingService()。 - 两条写入路径落在同一个文件:lease 路径
Config.getTranscriptPath()与 legacy 路径ensureConversationFile()都解析到<projectDir>/chats/<sessionId>.jsonl,所以开关来回切换不会把同一 session 的 transcript 劈成两份。 - 所有 lease 下游消费者都能正确退化:
hasSessionWriteOwnership()(shutdownSessionConfig不会误抛)、runWithWriteBarrier、enqueueRecordWrite(走jsonl.writeLine)、resetStoragePaths(ACP/cd不再抛错)、Config.assertCanStartTurn。 - 不存在漏传新参数的生产路径:全仓非测试
new Config(只有 4 处,唯一设置experimentalZedIntegration的是loadCliConfig;ACP 与 daemon(经acp-bridge/spawnChannel.ts的--acp)都走它,PR 描述中 "ACP or daemon" 的说法成立。
需要讨论的三点
ChatRecordingService构造函数的默认参数仍是旧 gate(见packages/core/src/config/config.ts:6706的行内评论)——生产无影响,但默认值现在与真实 gate 矛盾,失效模式是静默丢记录。showInDialog: true与同为 ACP/daemon-only 的todoStopGuard(false)不一致,且交互式会话永远不可能启用它。- gate 从 merged settings 读取,workspace scope 可覆盖 —— 而 lease 的安全性质是机器全局的。
关于「默认关闭」这个决策
理由成立,Risk 段也如实披露了。需要 reviewer 明确知道的权衡是:#7237 修的「并发 ACP 写入方静默损坏 transcript」事故,从此对所有默认用户重新暴露,且该路径默认死掉后容易腐化。值得一并讨论的替代方案是「默认开启 + 紧急关闭开关」——混合版本的风险在旧二进制上本来就存在(旧 writer 根本不认识锁),本设置并不能消除它,它真正提供的是混合配置下的可控性。属产品决策,不反对当前选择。
yiliang114
left a comment
There was a problem hiding this comment.
LGTM — gate logic is sound (=== true strict comparison, compound ACP+opt-in check, private readonly freeze). Strictly more restrictive than prior behavior, no bypass path.
Two non-blocking suggestions:
- Confirm daemon sessions set
experimentalZedIntegration(likely true via ACP transport) — a code comment would help future readers. - Consider adding negative test cases for
null,1, and explicitfalseto guard against a future truthy-check refactor.
Snapshot the effective restart-required lease gate from the bootstrap Config and reuse it for every session Config in the ACP process. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
ytahdn
left a comment
There was a problem hiding this comment.
The rollout blocker is fixed in b4bab7a. The effective lease gate is now snapshotted at ACP process startup and forced into every new/load/resume session Config, with regression coverage for both setting-transition directions. No remaining blockers from my review.
Use the effective session writer lease gate when ChatRecordingService is constructed without an explicit writer mode. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
@qwen-code /triage |
yiliang114
left a comment
There was a problem hiding this comment.
Re-review after b4bab7a + a789291: freeze semantics are sound (snapshot before first await, override per-session settings, constructor default). Recorder alignment maintains backward compat via ?? fallback chain. Previous P2s (daemon path confirmation, negative test cases) are resolved. 6 new test cases cover freeze both directions + recorder gate. No regressions to legacy/interactive paths. LGTM.
|
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
There was a problem hiding this comment.
中文说明
— qwen3.7-max via Qwen Code /review
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.1. |
|
|
What this PR does
This PR adds
experimental.sessionWriterLeaseas an explicit, restart-required opt-in for cross-process write fencing, with a default value offalse. The lease is enabled only when the setting is the boolean valuetrueand the session is running through ACP or the daemon; interactive and headless sessions continue to use the legacy recorder path. ACP snapshots the effective gate from its bootstrap configuration and reuses it for every session served by that process, so per-session settings reloads cannot mix leased and legacy writers before a restart. The settings UI, user documentation, design documentation, generated settings schema, and regression coverage are updated to describe and preserve the rollout behavior.Why it's needed
#7237 introduced session writer leases to prevent concurrent ACP processes from corrupting persisted session history, but the protocol is safe only when every concurrent writer participates. Enabling it automatically makes mixed-version or mixed-configuration deployments unsafe during rollout and rollback, while allowing a live ACP process to change modes on a settings reload could create the same unsafe mix inside one process lifetime. A default-off, process-stable opt-in lets operators drain existing writers, enable the protocol consistently across ACP and daemon processes, and restart them in a controlled order.
Reviewer Test Plan
How to verify
experimental.sessionWriterLease, or set it tofalse, and confirm recording continues through the legacy path without acquiring a writer lease.experimental.sessionWriterLeasetotrue, restart the ACP or daemon process, and confirm the existing lease contention, authoritative reload, write fencing, and release behavior remains active."true"and confirm it does not enable the lease.cd packages/cli && npx vitest run src/config/config.test.ts src/config/settingsSchema.test.ts src/utils/settingsUtils.test.ts; all 422 tests should pass.cd packages/core && npx vitest run src/services/session-writer-lease.test.ts; the gate tests should pass, and the lease suite should report 21 passed with one platform-specific skip.git diff --check; all should complete successfully.Evidence (Before & After)
Before: ACP sessions automatically entered the session writer lease protocol, and there was no rollout control in settings.
After: the Experimental settings expose “Enable ACP Session Writer Lease” as a restart-required boolean that defaults to off; only an explicit boolean
trueactivates the protocol for ACP and daemon sessions, and an ACP process keeps its startup value for every session until that process restarts.Tested on
Environment (optional)
macOS 26.4.1, Node.js v22.22.3, npm 10.9.8, local workspace without a sandbox.
Risk & Scope
experimental.sessionWriterLeasefor every concurrent ACP or daemon writer and restart those processes.Linked Issues
Follow-up to #7237.
中文说明
本 PR 的改动
本 PR 新增
experimental.sessionWriterLease,作为跨进程写入隔离的显式开关;该设置需要重启才能生效,默认值为false。只有设置值严格为布尔值true,并且会话通过 ACP 或 daemon 运行时,才会启用写入租约;交互式和 headless 会话继续使用旧记录器路径。ACP 会从启动配置中快照有效门控值,并将其复用于该进程服务的每个会话,因此按会话重新加载设置不会在重启前混用租约和旧写入方。同时更新设置界面、用户文档、设计文档、生成的设置 schema 和回归测试,以说明并固定这一发布行为。为什么需要
#7237 引入了会话写入租约,用于防止多个 ACP 进程并发写入时损坏持久化会话历史,但该协议只有在所有并发写入方都参与时才安全。自动启用会导致混合版本或混合配置部署在发布和回滚期间处于不安全状态,而允许存活的 ACP 进程在重新加载设置时切换模式,也可能在单个进程生命周期内产生同样的不安全混用。默认关闭且进程内稳定的显式开关允许运维人员先排空已有写入进程,再统一为 ACP 和 daemon 进程启用协议,并按可控顺序重启。
Reviewer 测试计划
如何验证
experimental.sessionWriterLease或将其设置为false的情况下启动 ACP 会话,确认记录功能继续走旧路径且不会获取写入租约。experimental.sessionWriterLease设置为true,重启 ACP 或 daemon 进程,确认现有的租约竞争、权威数据重载、写入隔离和释放行为仍然生效。"true"等非法 truthy 值,确认不会启用租约。cd packages/cli && npx vitest run src/config/config.test.ts src/config/settingsSchema.test.ts src/utils/settingsUtils.test.ts;预期 422 个测试全部通过。cd packages/core && npx vitest run src/services/session-writer-lease.test.ts;预期门控测试通过,租约测试为 21 个通过、1 个平台相关跳过。git diff --check;预期全部成功完成。证据(Before & After)
Before:ACP 会话会自动进入会话写入租约协议,设置中没有发布控制开关。
After:Experimental 设置中会显示“Enable ACP Session Writer Lease”,它是一个需要重启、默认关闭的布尔开关;只有显式设置为布尔值
true时,ACP 和 daemon 会话才会启用该协议,并且 ACP 进程会对其服务的每个会话保持启动值,直到该进程重启。测试平台
环境(可选)
macOS 26.4.1、Node.js v22.22.3、npm 10.9.8,本地工作区,未使用 sandbox。
风险与范围
experimental.sessionWriterLease并重启这些进程。关联问题
#7237 的后续改动。