fix(core): Fence concurrent ACP session writers - #7237
Conversation
P0a verification reportThe cross-process regression recreates the incident boundary without production identifiers: process A owns a transcript whose physical tail is a tool result, process B is rejected before it can load as a writer, process A appends the final answer and releases, and process B then acquires, authoritatively reloads that final answer, and appends the next user record with the final-answer UUID as its parent. Reloading the transcript preserves one active chain. Local verification on macOS 26.4.1 (Node.js v26.0.0, npm 11.12.1, Bun 1.3.13):
Authenticated manual model traffic and Windows/Linux runtime behavior were not exercised locally. Desktop |
🩺 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 |
Self-review notes (full pass: design + code)Posted five inline notes; consolidated summary here so reviewers can triage quickly. Verified during review (beyond the PR description's claims)
Known tradeoffs / follow-ups (details in the inline notes)
CI statusThe ubuntu Test job failed at the |
|
Thanks for the PR! Template looks good ✓ Problem: Observed production incident with clear evidence — linked issue #7164 describes the exact timing where process A recorded a tool result, the daemon fresh-loaded the same session, and both writers produced unmarked sibling branches. The design doc and PR body reconstruct the incident sequence convincingly. This is not theoretical hardening. Direction: Directly aligned — session data integrity is core to the product. The P0a scoping (extracted from #7166) is well-bounded: it protects the ACP/daemon append path that produced the incident without taking on the broader interactive/headless transition. CHANGELOG has no direct reference to writer fencing, but the area (session persistence integrity) is unambiguously relevant. Size: This PR touches core paths across 4 packages (core, cli, acp-bridge, desktop). Breakdown:
Approach: The lease protocol is well-designed — atomic hard-link acquisition, dead-process recovery via process-start identity, transcript fingerprinting, and fail-closed semantics throughout. The error taxonomy (4 stable RPC kinds mapped to sanitized HTTP 409/503) is clean and the design doc's invariants are clear. One observation on scope: the Desktop text-element persistence change (from direct JSONL rewriting to an ACP ext-method) adds ~475 production lines. I initially considered this scope creep, but it's actually a necessary consequence of the lease — once the transcript is fenced, direct rewriting by Desktop would trip The Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 问题： 已观测到的生产事故，有明确证据——关联 issue #7164 描述了进程 A 记录 tool result 后 daemon 重新加载同一会话、两个 writer 产生未标记 sibling branches 的完整时序。设计文档和 PR 正文对事故时序的重构令人信服。这不是理论性加固。 方向： 直接对齐——会话数据完整性是产品核心。P0a 的范围界定（从 #7166 中拆出）很合理：只保护产生事故的 ACP/daemon 追加路径，不引入更广泛的 interactive/headless 切换。CHANGELOG 没有直接提及 writer fencing，但会话持久化完整性领域明确相关。 规模： 本 PR 触及 4 个包（core、cli、acp-bridge、desktop）的核心路径。分解如下：
方案： 租约协议设计良好——原子 hard-link 获取、通过进程启动身份实现的死进程回收、transcript 指纹校验、以及全程 fail-closed 语义。错误分类（4 种稳定 RPC kind 映射到脱敏 HTTP 409/503）清晰，设计文档的不变量明确。 关于范围的一点观察：Desktop 文本元素持久化变更（从直接重写 JSONL 改为 ACP ext-method）增加了约 475 行生产代码。最初我认为是范围溢出，但实际上这是租约的必要后果——一旦 transcript 被围栏保护，Desktop 直接重写会触发
进入代码审查 🔍 — Qwen Code · qwen3.7-max Reviewed at |
Code ReviewIndependent proposal: For this problem, I would have designed a simpler file-based lock using Findings: No critical blockers. The implementation is thorough and follows project conventions (ESM, TypeScript strict, kebab-case
sequenceDiagram
participant P1 as Process A (ACP)
participant P2 as Process B (ACP)
participant L as Lock File
participant T as Transcript JSONL
P1->>L: acquire (atomic hard-link)
L-->>P1: owner token + transcript fingerprint
P1->>T: reload authoritative tail
P1->>P1: activate recorder
P2->>L: acquire (same session)
L-->>P2: session_writer_conflict (409)
P1->>T: appendJsonLine (verify owner + fingerprint + byteLength)
T-->>P1: advance expected state
P1->>L: release (unlink)
P2->>L: acquire (after release)
L-->>P2: owner token + reloaded fingerprint
P2->>T: reload authoritative tail (includes P1 final answer)
Files changed (30 of 36 shown)
Real-Scenario TestingThis is a non-UI persistence and lifecycle change (PR states "Evidence: N/A"). The session writer lease operates in the ACP/daemon path, not the interactive CLI. Testing approach: build verification, focused unit tests, and daemon smoke test. Build: Focused unit tests (all passing): Daemon smoke test (tmux capture): Session writer lock verification: After creating a session via ACP, the lock file was created at {
"schema_version": 1,
"session_id": "5580ee60-2e63-4ef7-b0d3-623833139b31",
"owner_id": "fe3cb963-791c-4ec3-9670-45832f10a666",
"pid": 184535,
"process_start_identity": "linux:a593921c-...:27769821",
"hostname": "...",
"process_kind": "acp",
"acquired_at": "2026-07-20T16:24:37.207Z",
"qwen_version": "0.20.0"
}Headless CLI verification: The interactive/headless CLI path (which does not acquire a P0a lease) continues to work correctly. 中文说明代码审查独立方案： 对于这个问题，我会设计一个更简单的基于文件的锁，使用 发现： 无关键阻塞项。实现彻底，遵循项目规范（ESM、TypeScript strict、kebab-case
真实场景测试这是非 UI 的持久化与生命周期变更。测试方法：构建验证、聚焦单元测试、守护进程冒烟测试。 构建成功。所有聚焦测试通过（共 1,849 项）。守护进程成功启动并创建会话，租约锁文件以预期格式创建。无头 CLI 路径继续正常工作。 — Qwen Code · qwen3.7-max Reviewed at |
|
Confidence: 3/5 — clean review across every stage, but the core-infrastructure gate (3,179 production lines across 4 packages from a fork author) requires a maintainer's sign-off before merge. This is a well-executed PR that addresses a real production incident with a sound design. The lease protocol is robust — atomic hard-link acquisition, process-start identity for PID reuse detection, full transcript fingerprinting, and consistent fail-closed semantics. The error taxonomy (4 stable RPC kinds mapped to sanitized HTTP 409/503) is clean, and the design doc's invariants are clear and verifiable. Going back to my independent proposal: I would have built a simpler The test coverage is extensive — 1,849 focused tests passing across the lease module, recorder, config, agent, session, bridge, transport, and error-response layers. The daemon smoke test confirms the lease mechanism works end-to-end: lock files are created with the expected record format, and the headless CLI path (which intentionally does not acquire a P0a lease) continues to work. Reservations for the maintainer's attention:
⏸️ Deferring to the maintainer — the core-infrastructure gate (3,179 production lines, cross-package, fork author) requires a human sign-off. The review found no blocking issues; this is a policy escalation, not a quality concern. 中文说明置信度：3/5 —— 各阶段审查均干净，但核心基础设施门禁（来自 fork 作者的跨 4 个包 3,179 行生产代码）需要维护者签字后才能合入。 这是一个执行良好的 PR，以可靠的设计解决了真实的生产事故。租约协议健壮——原子 hard-link 获取、进程启动身份用于 PID 复用检测、完整的 transcript 指纹、以及一致的 fail-closed 语义。错误分类清晰，设计文档的不变量明确且可验证。 回到我的独立方案：我会构建一个更简单的基于 测试覆盖广泛——1,849 项聚焦测试通过。守护进程冒烟测试确认租约机制端到端工作。 供维护者关注的保留意见：
⏸️ 转交维护者——核心基础设施门禁需要人工签字。审查未发现阻塞问题；这是策略性上报，而非质量顾虑。 — Qwen Code · qwen3.7-max Reviewed at |
|
⏸️ Deferring to @wenshao — this PR touches core infrastructure at scale (3,179 production lines across 中文说明⏸️ 转交 @wenshao —— 本 PR 大规模触及核心基础设施（跨 — Qwen Code · qwen3.7-max |
|
Addressed the remaining review threads in
Verification: targeted core, CLI, and ACP bridge regressions passed; lint, build, typecheck, and diff checks passed. An independent verification pass also covered the HTTP sanitization paths and confirmed a clean worktree. |
|
Addressed the latest review batch in
Verification: core config and recording tests 447/447; CLI ACP and worktree tests 300/300; ACP bridge full suite 413/413; build, lint, typecheck, diff checks, two clean self-audit passes, and independent post-fix verification all passed. The unrelated repository-baseline |
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. 1 Suggestion-level finding(s) could not be anchored to a changed line and were dropped; nothing further to act on here.
— qwen3.8-max-preview via Qwen Code /review
|
Review follow-up is pushed in 8a55da2 and all four threads are resolved. This update creates new transcript directories with mode 0700, adds focused Config.startNewSession ownership-guard coverage, preserves primary ACP request failures across new/load/resume and shared initialization cleanup, force-removes failed bulk-load Sessions while retaining ownership-aware deferred cleanup, and retains both activation/release causes while intentionally keeping the external session_writer_unavailable fail-closed contract. Verification: clean full build, full typecheck, full lint, core affected suites 425 passed / 1 skipped, ACP suite 294 passed, git diff check clean, and independent post-fix audit CLEAN. The existing vscode-ide-companion NOTICES check failure remains an unrelated deterministic base-branch issue. |
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
Accepted the legacy-recorder regression-coverage follow-up in
Verification: 72/72 focused recorder tests passed; Prettier and ESLint passed for both files; full build and full typecheck passed. |
Review:
|
Follow-up: lease lifecycle in
|
Add an ACP-only cross-process writer lease, authoritative reload, append fencing, turn admission, and acknowledged close semantics for persisted sessions. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Review: fix(core) — Fence concurrent ACP session writersThanks for the detailed writeup and for splitting P0a out of #7166. The incident analysis is right, and a per- What I confirmed works — worth stating up front, because the core is sound:
Overall: requesting changes. Not on direction. My concern is (a) the test suite has essentially no teeth on the concurrency core, and (b) too many paths end in a permanently dead session with no recovery. BlockersB1. Every race-critical step can be deleted without failing a single testThis is the one I'd fix first. Five mutations, each passing the entire suite unchanged:
The test literally named Similarly The good news: all the missing tests are cheap and deterministic in-process, no forking needed — plant a stale record with a dead pid and Also untested: B2. Failing to delete the reclaim guard destroys the just-acquired primary lock
Reachable without the test hook, because the guard lives under B3.
|
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Reviewed. 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. Not reviewed: chunk 1, chunk 2, chunk 3, chunk 4, chunk 5, chunk 6, chunk 7, chunk 8, chunk 9, chunk 10, chunk 11, chunk 12, chunk 13, chunk 14, chunk 15, chunk 16, chunk 17, chunk 18, chunk 19, chunk 20, chunk 21, chunk 22, chunk 23, chunk 24, chunk 25, chunk 26 — no agent reported covering these; nobody read them. Not reviewed: every dimension — none of the 34 required agents is on record as launched with a prompt this skill built, so this diff was reviewed, if at all, from prompts the run wrote for itself: no record shows the severity bar, the finding format or this project's own rules reaching an agent. Not reviewed: reverse audit — no auditor was launched with a prompt this skill builds — the pass that hunts what the rest of the review missed ran, if at all, without the method its brief carries. Not reviewed: verification — the review posts findings, but no verifier was launched with a prompt this skill builds — they were ruled on, if at all, without the verdict bar its brief carries.
— bailian/glm-5.2 via Qwen Code /review
| } catch { | ||
| throw new SessionWriterUnavailableError(); | ||
| } | ||
| throw error; |
There was a problem hiding this comment.
[Suggestion] finishAcquisition swallows the original acquisition error when lock cleanup also fails — throws cause-less SessionWriterUnavailableError(). Every other construction site in this file passes { cause: error }. — Failure scenario: transcript corruption triggers SessionTranscriptChangedError, removeOwnedLock fails with EIO → operator sees only "ownership could not be verified" with no root cause.
| } catch { | |
| throw new SessionWriterUnavailableError(); | |
| } | |
| throw error; | |
| throw new SessionWriterUnavailableError({ cause: error }); |
— bailian/glm-5.2 via Qwen Code /review
There was a problem hiding this comment.
Deferred under the post-five-round Critical-only cutoff. Preserving the original diagnostic cause when both acquisition and cleanup fail would improve operability, but it does not change lock ownership, fail-closed behavior, or data safety.
| if (error instanceof SessionWriterError) throw error; | ||
| throw new SessionWriterUnavailableError(); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
[Suggestion] releaseOnce throws cause-less SessionWriterUnavailableError() for unexpected removeOwnedLock errors — same inconsistency as finishAcquisition above and readOwnedLock (line 728). inspectExistingLock and appendJsonLine both pass { cause: error } for the same class of fs errors.
| if (error instanceof SessionWriterError) throw error; | |
| throw new SessionWriterUnavailableError(); | |
| } | |
| } | |
| } | |
| throw new SessionWriterUnavailableError({ cause: error instanceof Error ? error : undefined }); |
— bailian/glm-5.2 via Qwen Code /review
There was a problem hiding this comment.
Deferred under the post-five-round Critical-only cutoff. This is diagnostic-cause preservation for an already failed release; it does not alter whether ownership is retained, lost, or released.
| if (isDefinitiveAcpRequestError(error)) { | ||
| entry.closing = false; |
There was a problem hiding this comment.
[Suggestion] No bridge-level test for the definitive-close-refusal path (isDefinitiveAcpRequestError → reset entry.closing). The three new bridge tests cover timeout and kill paths but not the definitive-refusal path. If isDefinitiveAcpRequestError regresses (e.g., always returns false), retryable close refusals become channel-wide kills with no test to catch it. — Failure scenario: a RequestError refusal to session/close should preserve the channel and reset closing; a regression would fall through to killChannelWithLog, terminating all sessions on the channel.
— bailian/glm-5.2 via Qwen Code /review
There was a problem hiding this comment.
Not accepted: this path is already covered by preserves bridge state when required agent close fails so retry can flush. The fake child failure crosses the ACP connection as a definitive request refusal; the test asserts the first close rejects while the same session remains usable, then a retry succeeds. That would fail if the refusal fell through to channel-wide kill.
| const result = await operation(); | ||
| await lease?.assertOwnedAndUnchanged(); | ||
| return result; |
There was a problem hiding this comment.
[Suggestion] runWithWriteBarrier failure path (when assertOwnedAndUnchanged rejects) is untested. If the catch path regresses, the recorder stays active after an integrity violation — the exact stale-branch bug this PR prevents. — Failure scenario: external process modifies the transcript while the owner is inside runWithWriteBarrier → assertOwnedAndUnchanged throws SessionTranscriptChangedError → should set integrityFailure and transition to integrity_failed; a regression would allow subsequent appendJsonLine calls to append to a corrupted transcript.
— bailian/glm-5.2 via Qwen Code /review
There was a problem hiding this comment.
Deferred under the post-five-round Critical-only cutoff. This is additional regression coverage, not an uncovered production safety path: the catch already routes ownership and transcript-integrity failures through enterWriteFailure, and subsequent writes fail closed.
| if (this.state === 'active') this.state = 'closing'; | ||
| let flushFailure: unknown; | ||
| try { | ||
| await this.flush(); |
There was a problem hiding this comment.
[Suggestion] ChatRecordingService.close() has three untested failure paths (flush-fail+release-ok, release-fail+Lost, release-fail+generic). A regression could leak the lease lock file or leave the recorder in closing state indefinitely. — Failure scenario: flush() rejects (disk I/O) and lease.release() also rejects with a non-SessionWriterLostError → recorder should end in integrity_failed; a regression could leave it stuck in closing or leak the lock.
— bailian/glm-5.2 via Qwen Code /review
There was a problem hiding this comment.
Deferred under the post-five-round Critical-only cutoff. Expanding the close failure matrix is useful coverage, but the suggestion does not identify a current incorrect ownership transition or a Critical data-loss path.
| const rawDrainTimeoutMs = params['drainTimeoutMs']; | ||
| if ( | ||
| rawDrainTimeoutMs !== undefined && |
There was a problem hiding this comment.
[Suggestion] drainTimeoutMs parameter validation is untested — the two existing tests use valid values (5 and 50 ms); no test passes an invalid value (0, -1, 3.5, NaN, Infinity) to verify RequestError.invalidParams. — Failure scenario: a regression that removes the guard would pass the raw value to setTimeout, which coerces 0/negative to fire immediately, silently turning a close-drain timeout into a no-op.
— bailian/glm-5.2 via Qwen Code /review
There was a problem hiding this comment.
Deferred under the post-five-round Critical-only cutoff. The validation is present and this asks for additional invalid-input coverage; repository guidance classifies missing tests for otherwise correct behavior as a Suggestion.
| const sessionData = recording | ||
| ? await recording.runWithWriteBarrier(loadAuthoritative) | ||
| : await loadAuthoritative(); |
There was a problem hiding this comment.
[Suggestion] Rewind path's catch (line 9125) swallows ALL errors from runWithWriteBarrier (including permanent SessionWriterError subclasses) into soft artifactSnapshotUnavailable, returning success: true. The sibling loadUpdates path propagates writer errors correctly. Before this PR, the rewind path used runWithAcpRuntimeOutputDir (no writer barrier), so writer errors could not originate here. — Failure scenario: Process A holds the lease, Process B takes it, Process A calls rewind → runWithWriteBarrier throws SessionWriterUnavailableError → catch swallows it → response says success: true → next operation fails confusingly.
| const sessionData = recording | |
| ? await recording.runWithWriteBarrier(loadAuthoritative) | |
| : await loadAuthoritative(); | |
| const writerError = getSessionWriterError(err); | |
| if (writerError) throw new RequestError(writerError.rpcCode, writerError.message, { errorKind: writerError.errorKind }); |
— bailian/glm-5.2 via Qwen Code /review
There was a problem hiding this comment.
Deferred under the post-five-round Critical-only cutoff. The logical rewind has already completed before artifact snapshot reconstruction; on writer-ownership failure, runWithWriteBarrier still transitions the recorder to integrity-failed and prevents subsequent writes, while this response reports artifactSnapshotUnavailable. Propagating the typed writer error would improve error presentation, but it does not permit stale writes or data loss.
Keep reclaim cleanup failures from rolling back an acquired writer lock, and let a slow per-session close finish without reaping multiplexed sibling sessions. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
@wenshao Thanks for the current-SHA review. I independently reproduced the two close/lock failure paths and pushed 4892001. Accepted and fixed
Not accepted in this roundThis PR is beyond five review-fix rounds, so the remaining items are being judged under the Critical-only cutoff.
Verification
The branch is clean after commit and GitHub CI is running on 4892001. |
Maintainer verification — built and tested locally on Linux 🐧I built this PR from source at head Environment: Linux 6.12.63 (Debian 13), Node v22.22.2, npm 10.9.7, ext4. Model traffic served by a local mock OpenAI-compatible server so real turn machinery (and therefore real transcript writes) executes without network auth. 1. The headline: this actually fixes the incident, and base actually reproduces itI ran the same two-process scenario against the merge-base ( BASE — second writer is not fenced, and the transcript branches:
Process B PR — B is rejected before model work, and the chain stays linear:
17/17 checks against real
2. Cross-process lease invariants — 32/32A multi-process harness driving the compiled
Covered: live-owner conflict; 12 concurrent acquirers → exactly 1 winner, 11 clean conflicts (the hard-link atomicity claim); SIGKILL'd owner reclaimed with transcript preserved; PID-reuse fencing (a forged I also verified the documented hard-link-hostile filesystem risk: when 3. HTTP mapping through a live
|
| Suite | Result |
|---|---|
packages/core (8 changed files) |
860 pass, 1 skip, 2 fail — root-only artifact, see F1 |
packages/acp-bridge bridge.test.ts |
413 pass |
packages/cli acp-integration + serve (6 files) |
967 pass |
packages/desktop shared (bun) |
39 pass |
npm run typecheck / npm run lint / prettier |
clean |
Repo CI on this head is green (no failing checks; BLOCKED is only the pending review gate).
Non-blocking findings
F1 — Two lease tests fail when the suite runs as root (test portability, not a product bug)
session-writer-lease.test.ts has two cases that depend on chmod producing EACCES:
classifies an unreadable owned lock as unavailable(chmod(lockPath, 0o000), ~line 563)exposes the owned lease when transcript inspection cleanup must be retried(chmodSync(lockDir, 0o500), ~line 410)
Root holds CAP_DAC_OVERRIDE, so the chmod is a no-op and both assertions fail. The guard is platform-only:
it.runIf(process.platform !== 'win32')(I confirmed this is purely an euid artifact by exercising the identical code paths against the compiled module twice:
########## AS ROOT ##########
euid=0 (root)
FAIL unreadable owned lock -> unavailable observed=no-throw
FAIL cleanup retry exposes owned lease observed=... + lockSurvived=false
########## AS NOBODY (unprivileged) ##########
euid=65534 (unprivileged)
PASS unreadable owned lock -> unavailable observed=SessionWriterUnavailableError
PASS cleanup retry exposes owned lease observed=SessionWriterUnavailableError + lockSurvived=true
GitHub-hosted runners are non-root so CI stays green, but rootful Docker/devcontainer runs will see two red tests. Cheap fix if you want it:
const canDropPrivileges = process.platform !== 'win32' && process.geteuid?.() !== 0;
it.runIf(canDropPrivileges)(...)F2 — Interactive/headless writers still bypass the lease, and the wedged ACP session only recovers via a new process
This is in-scope per the PR body (interactive/headless is explicitly P0b), so I'm recording it as confirmation plus one recovery detail worth knowing before rollout.
Acquisition is gated at packages/cli/src/config/config.ts:2102:
experimentalZedIntegration: argv.acp || argv.experimentalAcp || false,and Config.activateChatRecording() returns early unless that flag is set. So a plain qwen -p ... --resume <acp-owned-session> takes no lease. I verified live: while an ACP writer held the lease, a headless run wrote 5 records into that same transcript and exited 0.
The good news — no branch was created, because the ACP owner's byte/identity fence then refuses to advance. The nuance is what happens next:
step 3: live ACP owner next turn -> session_transcript_changed
step 4: same live session, retry -> session_transcript_changed (sticky)
step 5: session/load in same process -> session_transcript_changed
step 7: fresh process session/load -> SUCCEEDED
step 8: fresh process turn -> SUCCEEDED
The recorder's integrity_failed state is permanent for that live session, and session/load in the same process cannot clear it (activate() requires state === 'inactive'). Only a brand-new process recovers. That is consistent with the fail-closed design and I'd not block on it — but for a daemon serving many sessions it means an affected session stays unusable until the client reconnects through a fresh agent process, so it may be worth an explicit client-facing recovery hint (or a P0b note) rather than leaving it to look like a hang.
Bottom line: the mechanism is sound, the atomicity and fencing claims hold under real concurrency, the incident reproduces on base and is prevented here, and Linux can now be marked ✅. I'd merge it.
中文版本（点击展开）
Maintainer 本地构建与真实测试验证 🐧
我在 head 4892001da 上从源码构建并在 Linux 上做了一次真实验证（PR 中 Linux 标记为
环境：Linux 6.12.63（Debian 13）、Node v22.22.2、npm 10.9.7、ext4。模型流量由本地 mock 的 OpenAI 兼容服务提供，因此真实的 turn 机制（以及真实的 transcript 写入）会实际执行，而无需网络认证。
1. 核心结论：这确实修复了事故，且 base 确实能复现该事故
我用同一套双进程场景分别跑了 merge-base（102c69217）和本 PR。
BASE —— 第二个 writer 未被拦截，transcript 发生分叉：
进程 B session/load 同一个 live session 完全没有被拒绝，两个进程同时写入，结果产生了未标记的 sibling branch：parent 2bc296e7（A 第一轮的回答）有两个子节点 —— a13cbf83（idx 4，A 的后续）和 4813a2a4（idx 8，B 的 user message）。重启会沿物理尾部进入 4813a2a4 这条链，因此 idx 7 处 A 已完成的回答不在恢复后的 active chain 中。这正是 PR 描述的事故时序。
PR —— B 在模型调用前被拒绝，链保持线性：
针对真实 qwen --experimental-acp 子进程的 17/17 项检查。其中值得注意的：
- B 被
session_writer_conflict/-32020拒绝，并且我确认没有发生任何模型调用（B 尝试前后 mock 的请求计数不变）—— 拦截确实发生在模型调用之前，符合测试计划第 1 步。 - A 追加 final answer 并退出后，后继进程取得租约、重载并追加。最终链中后继进程的 user record（idx 8）的 parent 正是 A 的 final answer（idx 7）—— 事故中出错的那一点现在是正确的。
- 零分叉、零孤儿记录。
- 同进程内重复 load 会复用 owner，不会自我冲突（测试计划第 3 步）。
2. 跨进程租约不变量 —— 32/32
一个多进程 harness，从真正独立的操作系统进程、在真实文件系统上驱动已编译的 packages/core/dist/.../session-writer-lease.js：
覆盖内容：live owner 冲突；12 个并发申请者 → 恰好 1 个成功、11 个干净冲突（即 hard-link 原子性这一claim）；被 SIGKILL 的 owner 可回收且 transcript 完整保留；PID 复用防护（在存活的 PID 上伪造 process_start_identity 会被正确判定为 stale，而真正存活的 owner 绝不会被误回收 —— 这正是朴素的 kill(pid,0) 检查会出错的情形）；外部追加会被读屏障和写入围栏同时检测到；锁被替换或被删除 → session_writer_lost 且不写入任何内容；干净交接后保持 4 条记录的线性链；截断（结尾无换行）的尾部被拒绝；符号链接 transcript 被拒绝；跨 session 隔离；以及 6 次进程交接共 18 条记录顺序完全正确。
我还验证了文档中提到的「文件系统不支持 hard link」风险：当 link() 失败时，acquisition 返回 session_writer_unavailable（-32023/503）且不会残留锁文件 —— 是 fail-closed，而不是 fail-open。
3. 通过真实 qwen serve 验证 HTTP 映射 —— 8/8
不只是单元测试：真实 daemon、真实 REST 调用，并由一个外部进程持有租约。
POST /session/:id/load → 409 session_writer_conflict、409 session_transcript_changed（截断尾部）、503 session_writer_unavailable（畸形锁），干扰移除后又能干净地返回 200（说明错误在 acquisition 层不是粘滞的）。响应体已正确脱敏 —— 没有泄漏锁路径、owner id、errno 或堆栈。我也确认了 DELETE /session/:id → 204 会释放租约（测试计划第 5 步的成功路径）。
4. 测试与静态检查
| 套件 | 结果 |
|---|---|
packages/core（8 个改动文件） |
860 通过、1 跳过、2 失败 —— 仅 root 环境产物，见 F1 |
packages/acp-bridge bridge.test.ts |
413 通过 |
packages/cli acp-integration + serve（6 个文件） |
967 通过 |
packages/desktop shared（bun） |
39 通过 |
npm run typecheck / npm run lint / prettier |
干净 |
该 head 上仓库 CI 为绿（无失败项；BLOCKED 仅是待评审的门禁）。
非阻塞发现
F1 —— 以 root 运行测试时有两个租约测试会失败（测试可移植性问题，非产品缺陷）
session-writer-lease.test.ts 中有两个用例依赖 chmod 产生 EACCES：
classifies an unreadable owned lock as unavailable（chmod(lockPath, 0o000)，约 563 行）exposes the owned lease when transcript inspection cleanup must be retried（chmodSync(lockDir, 0o500)，约 410 行）
root 拥有 CAP_DAC_OVERRIDE，因此 chmod 实际不生效，两处断言都会失败。而当前的 guard 只判断平台：it.runIf(process.platform !== 'win32')。
我通过对已编译模块执行完全相同的代码路径两次，确认这纯粹是 euid 造成的：root 下两项 FAIL，切换到非特权用户（nobody，euid=65534）后两项均 PASS。
GitHub 托管的 runner 是非 root，所以 CI 仍然是绿的；但在 rootful 的 Docker / devcontainer 环境下会看到两个红色测试。如果想修，代价很小：
const canDropPrivileges = process.platform !== 'win32' && process.geteuid?.() !== 0;
it.runIf(canDropPrivileges)(...)F2 —— 交互式/headless writer 仍然绕过租约；被卡住的 ACP session 只能通过新进程恢复
这一点在 PR 描述的范围之内（交互式/headless 明确属于 P0b），因此我把它记录为确认信息，外加一个上线前值得知道的恢复细节。
租约获取的开关在 packages/cli/src/config/config.ts:2102：experimentalZedIntegration: argv.acp || argv.experimentalAcp || false，且 Config.activateChatRecording() 在该 flag 未设置时会直接 return。所以普通的 qwen -p ... --resume <ACP 持有的 session> 不会取得租约。我做了实测：在 ACP writer 持有租约期间，一次 headless 运行向同一个 transcript 写入了 5 条记录并以 0 退出。
好消息是 —— 并没有产生分叉，因为 ACP owner 的字节/身份围栏随后拒绝继续推进。需要注意的是接下来的行为：
step 3: live ACP owner 下一轮 -> session_transcript_changed
step 4: 同一 live session 重试 -> session_transcript_changed（粘滞）
step 5: 同进程内 session/load -> session_transcript_changed
step 7: 全新进程 session/load -> 成功
step 8: 全新进程发起 turn -> 成功
recorder 的 integrity_failed 状态对该 live session 是永久的，且同一进程内的 session/load 无法清除它（activate() 要求 state === 'inactive'）。只有全新进程才能恢复。这与 fail-closed 的设计是一致的，我不会因此阻塞合入；但对于同时服务多个 session 的 daemon 来说，这意味着受影响的 session 在客户端通过新的 agent 进程重连之前会一直不可用 —— 因此也许值得给客户端一个明确的恢复提示（或在 P0b 中记一笔），以免它表现得像是卡死。
结论： 机制是可靠的，原子性与围栏在真实并发下均成立，事故在 base 上可复现、在本 PR 上被阻止，Linux 现在可以标记为 ✅。我认为可以合入。
|
@wenshao Thank you for the full real-process Linux verification and the base/PR differential. I am recording this as independent confirmation that the P0a fence prevents the incident and that Linux validation is green on 4892001. No code change is needed from the two non-blocking notes in this round:
The current SHA remains unchanged while the automatic review gate finishes. |
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.
— qwen3.7-max via Qwen Code /review
| const persistedTitleInfo = authoritative | ||
| ? this.getSessionService().getSessionTitleInfo(this.sessionId) | ||
| : undefined; |
There was a problem hiding this comment.
[Suggestion] getSessionTitleInfo is a synchronous tail-read that can throw on I/O errors between the async loadSession() and this sync call, causing the entire activateChatRecording() to fail and the lease to be released — even though the transcript was successfully loaded moments earlier. The legacy path in readPersistedTitleInfo() (chatRecordingService.ts:~599) wraps the same call in a try-catch as best-effort. — Failure scenario: transcript file is unlinked or corrupted between async load and sync title read → activateChatRecording fails → session activation blocked on a title cache that should be best-effort.
| const persistedTitleInfo = authoritative | |
| ? this.getSessionService().getSessionTitleInfo(this.sessionId) | |
| : undefined; | |
| let persistedTitleInfo: { title?: string; source?: TitleSource } | undefined; | |
| if (authoritative) { | |
| try { | |
| persistedTitleInfo = this.getSessionService().getSessionTitleInfo(this.sessionId); | |
| } catch { | |
| // Best-effort — don't block activation on a title read | |
| } | |
| } |
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Deferred under the post-five-round Critical-only cutoff. A filesystem change between loadSession() and the title tail-read is still rejected by the subsequent lease integrity assertion, so swallowing this read would not make that race recoverable; other read failures affect activation availability/diagnostics, not writer exclusivity or data safety.
| } else { | ||
| this.state = 'integrity_failed'; | ||
| } | ||
| throw error; |
There was a problem hiding this comment.
[Suggestion] When close() captures a flushFailure but lease.release() then throws a non-SessionWriterLostError, the flush failure is silently discarded — only the release error reaches the caller. The flushFailure often carries the more actionable diagnostic (e.g. SessionTranscriptChangedError — "the transcript was modified externally"). — Concrete cost: operator debugging close failures sees only the less-specific release error, losing the transcript-modification signal.
| } else { | |
| this.state = 'integrity_failed'; | |
| } | |
| throw error; | |
| } else { | |
| this.state = 'integrity_failed'; | |
| } | |
| if (flushFailure !== undefined) { | |
| throw new AggregateError([flushFailure, error], 'Session writer close failed'); | |
| } | |
| throw error; |
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Deferred under the post-five-round Critical-only cutoff. Aggregating flush and release failures would preserve more diagnostic context, but the current path already transitions to integrity_failed and propagates the release failure; it does not alter ownership, stale-write prevention, or data safety.
|
@qwen-code /triage |
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Reviewed. Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally. Not reviewed: chunk 24, PR #7237 fences concurrent ACP session writers using an a..., chunk 21, chunk 23, chunk 15, chunk 25, chunk 14, chunk 11, chunk 10, chunk 20, chunk 5, chunk 2, chunk 9, chunk 8, chunk 17, chunk 6, chunk 7, chunk 22, chunk 18, chunk 3, chunk 19, chunk 12, chunk 16, chunk 13, chunk 26, chunk 1, chunk 4 — pointed at diff lines it never opened: it made tool calls, but none of them read the diff. Not reviewed: reverse audit — no auditor was launched with a prompt this skill builds — the pass that hunts what the rest of the review missed ran, if at all, without the method its brief carries.
— qwen3.8-max-preview via Qwen Code /review
|
@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 review timed out. Qwen review timed out after 300 seconds (of the 180-minute budget). For large PRs, retry with a longer timeout by commenting: |
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: Invariant agent A: state, timers, collections — packages/cli/src/acp-integration/acpAgent.ts, Invariant agent B: counters, return values, error taxonomies — packages/cli/src/acp-integration/acpAgent.ts, Invariant agent C: config fields, early returns — packages/cli/src/acp-integration/acpAgent.ts — its prompt was built, but no agent on record was launched with it.
— qwen3.7-max via Qwen Code /review
| handle = await fs.open(temporaryPath, 'wx', 0o600); | ||
| await handle.writeFile(JSON.stringify(record), 'utf8'); | ||
| await handle.sync(); |
There was a problem hiding this comment.
[Suggestion] installLockRecord does not handle EEXIST from fs.open on the temp file. A stale temp file from a failed cleanup permanently kills the enclosing acquisition instead of just failing that retry iteration. — Failure scenario: within acquireInternal's retry loop, fs.link returns EEXIST → installLockRecord returns false → finally's fs.unlink fails transiently → next iteration's fs.open(temporaryPath, 'wx') hits EEXIST from the stale temp file → falls through to outer catch → wrapped as SessionWriterUnavailableError → acquisition fails even though the lock file itself may now be available.
| handle = await fs.open(temporaryPath, 'wx', 0o600); | |
| await handle.writeFile(JSON.stringify(record), 'utf8'); | |
| await handle.sync(); | |
| let handle: fs.FileHandle; | |
| try { | |
| handle = await fs.open(temporaryPath, 'wx', 0o600); | |
| } catch (openError) { | |
| if ((openError as NodeJS.ErrnoException).code === 'EEXIST') { | |
| await fs.unlink(temporaryPath).catch(() => {}); | |
| handle = await fs.open(temporaryPath, 'wx', 0o600); | |
| } else { | |
| throw openError; | |
| } | |
| } |
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Deferred under the post-five-round Critical-only cutoff. The stale temporary path can make this acquisition attempt fail closed, but it cannot grant ownership, append without a lease, or fork the transcript; a fresh activation uses a new random owner ID. Handling the transient cleanup failure would improve availability, not data safety.
| try { | ||
| await this.assertCanStartTurn(); | ||
| } catch (error) { | ||
| debugLogger.warn( | ||
| `Cron turn rejected [session ${this.sessionId}]: ${error instanceof Error ? error.message : String(error)}`, | ||
| ); | ||
| return; | ||
| } |
There was a problem hiding this comment.
[Suggestion] Cron and notification turn rejection by writer admission at the Session level has no direct test. client.test.ts covers admission at the client level, and Session.test.ts covers user prompts — but no test verifies that #drainCronQueue or #drainNotificationQueue skip a turn when assertCanStartTurn() rejects. A regression that bypasses the admission check in the drain path would allow a cron/notification turn to start after writer ownership is lost, producing the unmarked branch this PR prevents. — Concrete cost: the exact incident shape (process A writes while process B's cron fires against a lost lease) is silently possible at this layer without test coverage.
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Deferred under the post-five-round Critical-only cutoff. Both queue drains currently call assertCanStartTurn() before setting their processing flags or executing a queued turn, and existing client/session tests cover the admission primitive. This asks for additional regression coverage rather than identifying a current stale-write path.
Review: fix(core): Fence concurrent ACP session writersOverviewThis PR introduces a cross-process writer lease for ACP/daemon sessions: an atomic hard-link lock per I read the full diff (36 files) and cross-checked integration points against the head branch. Verdict up front: this is careful, well-tested work and I found no correctness bug in the lease protocol or its integration. The findings below are tradeoffs, sharp edges, and small improvements — none blocking on their own. Strengths
Findings1. 2. Windows lease acquisition spawns PowerShell on every session create/load (medium, perf). 3. Per-append verification cost (low, worth measuring). 4. Maintenance ops aren't lease-aware yet (known P0b gap — worth a doc note). 5. Shutdown latency (low). 6. Nits.
Risk assessment
ConclusionSolid engineering on a hard problem, with the failure interleavings actually enumerated and tested rather than hand-waved. Items 1 and 2 are the ones I'd most like addressed (or explicitly acknowledged as deliberate) before merge; everything else can be follow-up. |
|
@wenshao Thanks for the thorough review. I rechecked these findings against 4892001. Given that this PR has passed more than five review-fix rounds, I am keeping the current Critical-only freeze and will not expand this PR for these non-blocking items:
No Critical issue was identified, so the branch and SHA remain unchanged. |
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
LGTM. Lease protocol is well-designed — atomic hard-link acquisition, multi-layer transcript fencing, and sanitized error taxonomy all look solid. Critical findings from prior rounds are resolved. CI green.
|
Released in v0.20.1. |
|
Agent run timed out after 1800000ms ❌ failed |
4 participants
What this PR does
This PR extracts the independently landable P0a from #7166 and protects the ACP/daemon path that produced the incident. Each
(runtime base, session ID)gets one cross-process writer through an atomic hard-link lease; the owner reloads the authoritative transcript after acquisition, and every append verifies the owner token, transcript file identity, metadata, and UTF-8 byte length before advancing the in-memory tail.The daemon reuses an already-live owner, gates user, cron, notification, and teammate turns on verified ownership, serves live transcript replay through the owner's write barrier and pinned session storage, and removes a live entry only after an acknowledged close has drained and released the lease. Runtime and persistence roots remain pinned across logical working-directory changes. Desktop metadata is appended through the owning ACP session instead of rewriting JSONL, and a writer conflict is surfaced to the user instead of silently replacing the requested persisted session with a fresh session. ACP and HTTP expose stable, sanitized conflict and availability errors.
Why it's needed
In the incident, process A had already recorded a tool result when a daemon fresh-loaded the same persisted session from that tail. Process A then recorded the remaining tool work and final answer, while the daemon later appended another user message using its stale parent. Both writes looked valid in isolation, but they created unmarked sibling branches. Restart followed the physical tail, so the complete answer that the user had already seen was absent from the active restored chain.
P0a prevents that ordering without taking on the broader interactive/headless transition and maintenance protocol from #7166. A second cooperating ACP writer is rejected before model work, and a replacement can proceed only after it owns the lease and reloads the final physical tail.
Reviewer Test Plan
How to verify
session_writer_conflictbefore model invocation and should not change the transcript.session_writer_conflict,session_writer_lost,session_transcript_changed, andsession_writer_unavailablemap to sanitized HTTP 409/503 responses.Evidence (Before & After)
N/A (non-UI persistence and lifecycle change).
Tested on
Environment (optional)
macOS 26.4.1; Node.js v26.0.0; npm 11.12.1; Bun 1.3.13; local filesystem.
npm run build, rootnpm run lint, rootnpm run typecheck, and Desktop typecheck passed. Focused results include 843 Core tests passed with 1 skipped, 413 ACP bridge tests passed, 290 ACP agent tests passed, 371 ACP Session tests passed, 275 HTTP transport tests passed, 39 Desktop integration tests passed, 18 cross-process lease tests passed with 1 platform-conditional skip, and 5 error-response tests passed. Desktoplint:sharedis still blocked by existing repository ESLint configuration errors for unavailableimport/no-internal-modulesrules and an unrelated source-auth test violation.Risk & Scope
session_writer_unavailable. No transcript schema migration is required.Linked Issues
Related to #7164. Extracted from #7166.
中文说明
本 PR 做了什么
本 PR 从 #7166 中拆出可独立合入的 P0a，只保护本次事故涉及的 ACP/daemon 路径。每个
(runtime base, session ID)通过原子 hard-link 租约只允许一个跨进程 writer；owner 在取得租约后权威重载 transcript，并在每次追加前校验 owner token、transcript 文件身份、元数据和 UTF-8 字节长度，然后才推进内存中的尾节点。daemon 会复用已经 live 的 owner，在 user、cron、notification 和 teammate 轮次开始前验证所有权，通过 owner 的写屏障和固定的 session storage 提供 live transcript 回放，并且只在已确认的 close 完成 drain 和 lease release 后移除 live entry。逻辑工作目录变化不会改变固定的 runtime 与 persistence root。Desktop 元数据改为通过持有 owner 的 ACP session 追加，不再重写 JSONL；writer 冲突会直接返回给用户，而不是静默把请求恢复的持久化会话替换为 fresh session。ACP 与 HTTP 对外提供稳定且脱敏的冲突/不可用错误。
为什么需要
事故时序中，进程 A 已经记录了一个 tool result，此时 daemon 从该尾节点 fresh-load 了同一个持久化会话。之后进程 A 继续记录剩余工具工作和 final answer，而 daemon 又使用过期 parent 追加了下一条 user message。两次写入各自看起来都合法，但形成了未标记的 sibling branches。重启按物理文件尾恢复，因此用户已经看到的完整回答不在恢复后的 active chain 中。
P0a 在不引入 #7166 中更广泛的 interactive/headless 切换和维护协议的前提下阻止该时序。第二个遵守协议的 ACP writer 会在模型工作开始前被拒绝；替代进程只有在取得租约并重新加载最终物理尾后才能继续。
Reviewer 测试计划
如何验证
session_writer_conflict，且不得修改 transcript。session_writer_conflict、session_writer_lost、session_transcript_changed和session_writer_unavailable分别映射为脱敏的 HTTP 409/503 响应。证据（Before & After）
N/A（非 UI 的持久化与生命周期变更）。
测试平台
环境（可选）
macOS 26.4.1；Node.js v26.0.0；npm 11.12.1；Bun 1.3.13；本地文件系统。
npm run build、根目录npm run lint、根目录npm run typecheck和 Desktop typecheck 均通过。聚焦测试结果包括：Core 843 项通过、1 项跳过，ACP bridge 413 项通过，ACP agent 290 项通过，ACP Session 371 项通过，HTTP transport 275 项通过，Desktop 集成测试 39 项通过，跨进程 lease 测试 18 项通过、1 项平台条件跳过，以及错误响应测试 5 项通过。Desktoplint:shared仍受仓库现有 ESLint 配置问题阻塞，包括缺少import/no-internal-modules规则，以及一个与本改动无关的 source-auth 测试违规。风险与范围
session_writer_unavailable失败关闭。不需要进行 transcript schema migration。关联 Issue
关联 #7164；从 #7166 中拆出。