fix(core): Preserve ownership during session cleanup - #10286
Conversation
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
Thanks for the follow-up! Template looks good ✓ Problem: real, not theoretical. This is the deferred class from the #9626 review — rounds 10–11 recorded findings (several probe-verified) that a generation trip between the transcript unlink/rename and the companion cleanup (worktree / PR / prompt-ledger / file-history / organization) aborts an already-committed lifecycle operation halfway and strands sidecars. The runtime generation is exactly what closes when the transcript is deleted or moved, so this window is reachable in production; the mechanism shipped in v0.22.2. Direction: aligned. Core daemon lifecycle reliability, paying down review debt from a merged PR. The change is additive — new optional fence parameters, no public-contract change — and the protocol-doc clarification was itself a deferred item from #9626. Size: core paths touched ( Approach: right shape. Keeping the generation fence in front of the primary transcript mutation and switching post-mutation cleanup to a byte-exact writer-lease ownership check delivers both properties the review asked for: committed cleanup finishes after the runtime closes, and a replacement writer's same-session artifacts are never deleted by stale cleanup. Making the check synchronous and fd/inode-bound (O_NOFOLLOW / O_NONBLOCK, fstat vs lstat identity) closes the event-loop interleaving window between fence and destructive syscall. Nothing in the diff beyond the stated goal. Risk: no high-risk path signals. No elevated risk indicators. Moving on to code review. 🔍 中文说明感谢这个 follow-up！ 模板完整 ✓ 问题：真实存在，不是理论问题。这是 #9626 评审中遗留的一类问题：第 10–11 轮记录了（多项经探针验证的）发现——在 transcript unlink/rename 与伴随清理（worktree / PR / prompt-ledger / file-history / organization）之间发生 generation 跳变，会使已提交的生命周期操作中途停止、遗留孤儿 sidecar。而 runtime generation 恰恰会在 transcript 被删除或移动时关闭，因此该窗口在生产环境可达；相关机制已随 v0.22.2 发布。 方向：对齐。核心 daemon 生命周期可靠性问题，偿还在已合并 PR 上的评审债务。改动是增量式的——新增可选围栏参数、无公共契约变更——协议文档的澄清本身也是 #9626 的遗留项。 规模：触及核心路径（ 方案：形态正确。主 transcript 变更前保留 generation 围栏，变更后将清理切换为按字节精确校验的 writer-lease 所有权，同时满足评审要求的两个属性：已提交的清理在 runtime 关闭后仍能完成；替代 writer 的同 session 数据不会被旧清理误删。校验做成同步且绑定 fd/inode（O_NOFOLLOW / O_NONBLOCK、fstat 与 lstat 身份比对），关闭了围栏与破坏性系统调用之间的事件循环交错窗口。diff 中没有超出目标的内容。 风险：无高风险路径信号，无升级风险信号。 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewRe-run on the same head — I re-read the diff and re-verified the load-bearing claims rather than re-posting the earlier pass on faith. My independent take before reading the diff: once the transcript unlink/rename has committed, the generation fence is the wrong gate for the remaining cleanup, because the runtime generation closes precisely when the transcript is deleted or moved — but dropping the gate entirely would let stale cleanup delete a replacement writer's same-session artifacts. So: keep the generation fence for the primary mutation, then switch to a writer-lease ownership check for cleanup, fail closed when ownership is lost. That is exactly what this PR does, and I did not find a simpler path it missed. What I verified against the code:
One observation, non-blocking: a transient sequenceDiagram
participant P1 as Daemon route
participant P2 as runWithDaemonWriterLease
participant P3 as SessionWriterLease
participant P4 as SessionService
participant P5 as Companion storage
P1->>P2: delete, archive, or unarchive
P2->>P3: acquire writer lease
P2->>P4: mutate with two fences
Note over P4: generation fence guards the primary mutation
P4->>P5: unlink or rename transcript
Note over P4: runtime generation may now close, cleanup switches to ownership
P4->>P3: assertCleanupOwned before each cleanup step
alt still owned
P4->>P5: remove or move sidecars, ledgers, org data
else ownership lost
Note over P4: stop fail closed, spare replacement-generation artifacts
end
Testing evidence — the PR's own CI (fetched once via API, not re-run)Unattended run — PR code is never executed here. At this re-run's fetch time no check had failed: every completed check is green, including CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 Not verified: the real-daemon race behavior itself. The unit suite pins the fence wiring through mocked fences; the author's real filesystem/daemon harness results are a self-reported claim, and the author tested on macOS only (Windows/Linux not exercised locally). Sandboxed verification is now running — triggered from this thread in parallel with this re-run — and its report will post to the verification comment here; the claim it settles is that committed companion cleanup finishes after the runtime generation closes, and stops fail-closed when the writer lock is replaced — race semantics no static read of the diff can prove. 中文说明代码审查（同一 head 的 re-run：重新通读 diff 并逐项复核关键结论，而非直接复用上一轮）：在读 diff 之前我的独立方案是——主变更（transcript unlink/rename）提交之后，generation 围栏已不再适合作为后续清理的门槛，因为 runtime generation 恰恰在 transcript 被删除或移动时关闭；但完全去掉围栏又会让旧清理误删替代 writer 的同 session 数据。因此：主变更保留 generation 围栏，清理切换为 writer-lease 所有权校验、所有权丢失时 fail closed。这正是本 PR 的做法，我没有找到更简单的路径。 逐项核对：所有三个 daemon 面（REST 路由与 ACP-over-HTTP）都经由 一个非阻塞观察：清理期间重读锁遇到瞬时 测试证据：无人值守运行，此处从不执行 PR 代码。本次 re-run 抓取时无失败检查：所有已完成检查全部通过（含 未验证：真实 daemon 竞态行为本身。单测通过 mock 围栏固定接线；作者的真实文件系统/daemon harness 结果属自述，且仅在 macOS 上运行（Windows/Linux 未在本地执行）。沙箱验证已在运行——由本线程触发、与本次 re-run 并行——报告将发布在本帖的验证评论中；它要定论的是"已提交的伴随清理在 runtime generation 关闭后完成、在 writer 锁被替换时 fail-closed 停止"这类静态读 diff 无法证明的竞态语义。 — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 4/5 — clean static review with the mechanism pinned by tests in both directions; withholding the fifth until CI lands green and because the daemon-level race behavior rests on the author's self-reported harness (sandboxed verification of exactly that claim is now running). Re-run reflection, same head: I re-read the diff and re-audited every caller of the three lifecycle methods before re-posting this verdict, so this is a fresh pass, not a repost. My independent proposal — keep the generation fence for the primary transcript mutation, switch post-mutation cleanup to a writer-lease ownership check, fail closed on lost ownership — is exactly what the PR implements, and I never found a simpler path it missed. The problem is not framed into existence: it is the deferred, recorded finding class from #9626's review rounds, living in code that shipped in v0.22.2, and this PR pays down exactly that debt without carrying anything else. The diff is 156 production lines against 275 test lines, every new test pins a mechanism (both directions × all five lifecycle surfaces) rather than an implementation detail, and the two-fence contract is legible enough that maintaining it in six months reads as a thank-the-author change. The one semantic nuance I'd want a maintainer to have in mind — a transient lock-read failure now surfaces as an error after a committed mutation, fail-closed by design — is noted in my review comment, not a blocker. The remaining gap is honest evidence, not doubt about the code: the unit suite proves the fence wiring through mocks, and the real race semantics (cleanup finishing after the runtime closes; stopping cold on a replaced lock) are backed so far by the author's macOS-only harness claim. The sandboxed Approval remains deferred until CI lands green on 中文说明回顾全局（同一 head 的 re-run：重新通读 diff、重新审计三个生命周期方法的全部调用方后才再次给出结论，这是新一轮审查而非转发）：我在读 diff 之前写下了独立方案——主 transcript 变更保留 generation 围栏，变更后的清理切换为 writer-lease 所有权校验、所有权丢失即 fail closed——本 PR 恰好落在这个设计上，我没有找到它遗漏的更简路径。问题不是被框架出来的：它是 #9626 评审轮次中被记录、被遗留的发现类，存在于 v0.22.2 已发布的代码中，本 PR 正是偿还这笔债务，且没有夹带任何其他内容。生产逻辑 156 行对测试 275 行，每个新测试固定的是机制（两个方向 × 五个生命周期面）而非实现细节，双围栏契约足够清晰，六个月后维护它应当是感谢作者而非抱怨作者。唯一希望维护者心中有数的语义细节——锁的瞬时读取失败现在会在主变更已提交后以错误形式上报，这是设计内的 fail-closed——已写在审查评论中，不构成阻塞。 剩余的差距是证据层面的诚实问题，而非对代码的怀疑：单测通过 mock 围栏证明了接线，而真实竞态语义（runtime 关闭后清理完成；锁被替换时立即停止）目前仅有作者自述的、仅在 macOS 上运行的 harness 支撑。用于定论的沙箱 批准继续推迟到 — Qwen Code · qwen3.8-max Reviewed at |
|
@qwen-code /triage |
|
Sandboxed verification: ❌ not passed — non-deterministic tests (flakiness gate) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 885 passed · 0 failed · 885 total Flakiness gate: ❌ 1 of 4 changed test file(s) returned different results across identical re-runs (5 full round(s)) The deterministic flakiness gate re-ran the test files this PR changes and got different outcomes from identical runs (agent verdict: 中文 — 判定：❌ 不通过 · 测试结果不确定（抖动门）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：885 通过 · 0 失败 · 885 总计 抖动门：❌ 1 of 4 changed test file(s) returned different results across identical re-runs (5 full round(s)) 确定性抖动门将本 PR 改动的测试文件原样重跑了多轮，得到了不一致的结果（agent 判定： Verification reportPR #10286 Deep Verification —
|
| Cell | Oracle | base (HEAD^1) |
head (HEAD^2) |
|---|---|---|---|
| C1 delete, generation closes after commit | return + leftover artifacts | rejects at first cleanup fence; worktree/pr/ledger/file-history left behind | resolves true; all removed |
| C2 archive, generation closes after move | errors + sidecar location |
1 WorkspaceGenerationClosedError; sidecars stay in active |
errors=[]; sidecars moved to archived |
| C3 unarchive, generation closes after move | mirror of C2 | mirror (left in archived) | mirror (moved to active) |
| C4 archive conflict repair, generation closes | resolvedConflicts + leftovers |
1 error; active sidecars left behind | resolved; sidecars cleaned |
| S1 delete, replacement writer takes lock | error identity + replacement artifacts | resolves; replacement artifacts destroyed (hazard) | SessionWriterLostError; replacement artifacts preserved byte-for-byte |
Head arm: 40/40. Base arm: 32/32 (each base cell asserts the predicted broken behaviour, so the base failing to clean up counts as a passed assertion of intent). Witnesses: evidence/01-ab-head-cleanup-completes.png, evidence/02-ab-base-cleanup-stalls.png. Raw per-cell logs: logs/ab-head-results.json, logs/ab-base-results.json.
assertCleanupOwned boundary probe (02-boundary.mjs, real lease, 11/11): witness evidence/03-boundary-matrix.png. CLI end-to-end wiring (03-cli-wiring.mjs, real daemon entry points, 18/18): witness evidence/04-cli-wiring.png.
Targeted gates
| Gate | Result |
|---|---|
Core focused suites (sessionService, sessionService.corruption, session-writer-lease) |
364 passed, 2 skipped (366), 3 files green |
CLI daemon lifecycle (session-archive, scheduled-task-session-lifecycle) |
72 passed (65 + 7), 2 files green |
npm run typecheck (repo-wide, all workspaces + integration) |
exit 0 |
I ran the gates from a clean, unmutated tree; the mutation runs below are separate scratch edits, never the gate tree.
Vacuity / mutation matrix
All mutants are scratch edits in a throwaway worktree (tmp/mutant-tree); the gate tree was never mutated. Positive control: the unmutated baseline (M0) is green, so a red below is attributable to the mutant, and each red is quoted where it matters.
| # | Mutant | Suite(s) | Outcome | Read |
|---|---|---|---|---|
| M0 | none (baseline) | 3 core suites | 364 green | runner is live |
| M1 | sessionService.ts reverted to base (no assertCleanupOwned) |
2 service suites | 14 red | every new behavioral test flips; the PR's tests are not vacuous |
| M2 | assertCleanupOwned body → return; |
lease suite | 2 red (toThrow(SessionWriterLostError) not thrown) |
lease tests pin the check |
| M3a | remove O_NOFOLLOW only |
symlink test | green | redundant defence (lstat layer holds) |
| M3b | remove lstat identity block only | symlink test | green | redundant defence (O_NOFOLLOW/ELOOP holds) |
| M3c | remove both O_NOFOLLOW and lstat identity |
symlink test | 1 red ("expected function to throw, but it didn't") | the layered guard set is load-bearing |
| M4 | drop SessionWriterError re-throw in archive pr-sidecar cleanup |
pr-sidecar test | 1 red (expected "spy" to be called 2 times, but got 3) |
ownership-loss propagation is pinned |
M1 red sample (behavioral, expected-vs-actual): expected [ { …(2) } ] to deeply equal [] — received an errors entry { error: Error("generation changed"), sessionId }. M3c witness: evidence/06-mutation-m3c-symlink-combo.png. M1/M3 raw vitest logs: logs/mutation-m1-vitest.txt, logs/mutation-m3c-vitest.txt.
Vacuity probe on the CLI wiring (source of Finding 1): reverting only packages/cli/src/serve/server/session-archive.ts to base (so assertCleanupOwned is never threaded into removeSession/archiveSessions/unarchiveSessions) and re-running session-archive.test.ts gives 65/65 green — the existing CLI suite does not observe the cleanup fence at all. That is expected given the fence is a core-level concern, but it means the CLI-level regression is caught only by the core suites + the harness here, not by a CLI test that fails on the wiring. Recorded as Finding 1.
Findings (non-blocking)
- No CLI-level test pins the cleanup-ownership wiring (coverage gap, Suggestion). Reverting the CLI plumbing to base leaves all 65
session-archive.test.tstests green (see vacuity probe above). The behaviour itself is verified correct by the core A/B and the CLI harness in this round, so this is a coverage observation, not a defect: a future regression that dropsassertCleanupOwnedfromrunWithDaemonWriterLeasewould not be caught by the CLI suite. A small CLI test asserting that a replacement-generation lock leaves companion sidecars intact after a committed delete would close it.
Repro:git show HEAD^1:packages/cli/src/serve/server/session-archive.ts > <scratch>/packages/cli/src/serve/server/session-archive.ts && cd <scratch>/packages/cli && npx vitest run src/serve/server/session-archive.test.ts→ 65/65 green on base wiring + head core. - Symlink defence relies on
O_NOFOLLOW, with a?? 0fallback (informational).assertCleanupOwnedopens withO_RDONLY | (O_NOFOLLOW ?? 0) | (O_NONBLOCK ?? 0). On platforms whereO_NOFOLLOWis undefined the open falls through to the lstat identity block, which independently rejects symlinks (M3a shows the lstat layer alone still passes the symlink test), so the two layers are genuinely redundant rather than one being dead. The PR's own symlink test isrunIf !== win32; the Windows path therefore rests on the lstat/dev-ino layer rather thanELOOP. Calling it out so a Windows reviewer can confirmlstatSync().isSymbolicLink()/dev/inobehave as expected there; M3a on Linux demonstrates the lstat layer alone holds whenO_NOFOLLOWis absent.
Repro: apply M3a (delete the(nodeConstants.O_NOFOLLOW ?? 0) |line insession-writer-lease.ts) and runnpx vitest run src/services/session-writer-lease.test.ts -t "rejects a symlinked cleanup lock"→ green; apply M3c (also delete the lstat identityifblock) → red.
Neither finding changes the verdict: the first is a test-coverage gap (behaviour verified correct this round), the second documents an existing cross-platform design.
Not covered
- Real multi-process contention / crash recovery for the writer lease (the suite covers it via forked helpers; I did not add a live two-daemon race).
- Performance / very large transcripts — out of scope for a correctness PR; no size claim made by the PR.
- Windows and macOS — run on Linux (
node:22-bookworm). The symlinkrunIfguard andO_NOFOLLOW ?? 0fallback are the platform-sensitive spots (Finding 2). - Repo-wide
npm run testand bundle (npm run bundle) — only the affected workspaces' focused suites plus typecheck were run, per scope discipline.
Methodology
One Linux node:22 container, PR merge-ref checked out at depth 2 (HEAD = merge, HEAD^1 = base tip, HEAD^2 = PR head). npm ci + npm run build had completed at HEAD before this round. Base arm: git worktree add tmp/base-tree HEAD^1, rebuilt only packages/core with node_modules symlinked from the root (the PR changes no package.json/package-lock.json, so the dependency closure is identical; no internal workspace symlink confound for core). Harnesses 01-ab-core.mjs, 02-boundary.mjs, 03-cli-wiring.mjs import the compiled dist/ directly via file:// URLs and drive real SessionService/SessionWriterLease/daemon entry points against real temp-directory filesystems — no stub of the unit under test; the only injected pieces are the runtime-generation fence function (the scenario trigger) and, in CLI-B, a prototype-level assertCleanupOwned that reports lost ownership (standing in for the already-proven real check on a replaced lock). Each cell's expected outcome is encoded as an assertion, so a control arm failing as predicted counts as a pass and assertions.json fail counts only unexpected outcomes (zero here). Disclosure: the first run of 03-cli-wiring.mjs printed 16/18 because the harness itself never seeded the CLI-A sidecars (fixture bug, not a PR defect); the harness was fixed and re-run (18/18), and only the fixed run is counted. Mutation/vacuity work ran in tmp/mutant-tree via npx vitest run <files> with targeted -t filters; the gate tree stayed pristine. Evidence images were produced with scripts/verify-capture.mjs. Raw logs in logs/.
Flakiness gate log
rounds=5 files=4 skipped=0
file packages/cli/src/serve/server/session-archive.test.ts: (cd packages/cli) npx --no-install vitest run ./src/serve/server/session-archive.test.ts
file packages/core/src/services/session-writer-lease.test.ts: (cd packages/core) npx --no-install vitest run ./src/services/session-writer-lease.test.ts
file packages/core/src/services/sessionService.corruption.test.ts: (cd packages/core) npx --no-install vitest run ./src/services/sessionService.corruption.test.ts
file packages/core/src/services/sessionService.test.ts: (cd packages/core) npx --no-install vitest run ./src/services/sessionService.test.ts
per-file results (P=pass F=fail I=infra-exit, one letter per run):
packages/cli/src/serve/server/session-archive.test.ts: FPPFF
packages/core/src/services/session-writer-lease.test.ts: FFFFF
packages/core/src/services/sessionService.corruption.test.ts: FFFFF
packages/core/src/services/sessionService.test.ts: PPPPP
verdict: flaky
summary: 1 of 4 changed test file(s) returned different results across identical re-runs (5 full round(s))
--- per-invocation detail (full copy in the artifact) ---
round 1 · packages/cli/src/serve/server/session-archive.test.ts: F (exit 1)
--- output tail · round 1 · packages/cli/src/serve/server/session-archive.test.ts ---
rror": SessionTranscriptChangedError {�[39m
�[31m+ "message": "The session transcript changed outside its active writer.",�[39m
�[31m+ "name": "SessionTranscriptChangedError",�[39m
�[31m+ "rpcCode": -32022,�[39m
�[31m+ "errorKind": "session_transcript_changed",�[39m
�[31m+ "httpStatus": 409,�[39m
�[31m+ },�[39m
�[31m+ "sessionId": "550e8400-e29b-41d4-a716-446655440010",�[39m
�[31m+ },�[39m
�[2m ],�[22m
�[32m- "archived": [],�[39m
�[32m- "errors": [],�[39m
�[2m "notFound": [],�[22m
�[2m "resolvedConflicts": [],�[22m
�[2m }�[22m
�[36m �[2m❯�[22m src/serve/server/session-archive.test.ts:�[2m849:20�[22m�[39m
�[90m847| �[39m })�[33m;�[39m
�[90m848| �[39m
�[90m849| �[39m �[34mexpect�[39m(result)�[33m.�[39m�[34mtoEqual�[39m({
�[90m | �[39m �[31m^�[39m
�[90m850| �[39m archived�[33m:�[39m []�[33m,�[39m
�[90m851| �[39m alreadyArchived�[33m:�[39m [sessionId]�[33m,�[39m
�[31m�[2m⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯[2/6]⎯�[22m�[39m
�[41m�[1m FAIL �[22m�[49m src/serve/server/session-archive.test.ts�[2m > �[22marchiveDaemonSessions�[2m > �[22mrepairs an active/archive conflict by keeping the archived copy
�[31m�[1mAssertionError�[22m: expected { archived: [], …(4) } to match object { …(3) }
(2 matching properties omitted from actual)�[39m
�[32m- Expected�[39m
�[31m+ Received�[39m
�[2m {�[22m
�[32m- "archived": [�[39m
�[32m- "550e8400-e29b-41d4-a716-446655440116",�[39m
�[32m- ],�[39m
�[32m- "errors": [],�[39m
�[32m- "resolvedConflicts": [�[39m
�[32m- "550e8400-e29b-41d4-a716-446655440116",�[39m
�[31m+ "archived": [],�[39m
�[31m+ "errors": [�[39m
�[31m+ {�[39m
�[31m+ "error": SessionTranscriptChangedError {�[39m
�[31m+ "message": "The session transcript changed outside its active writer.",�[39m
�[31m+ "name": "SessionTranscriptChangedError",�[39m
�[31m+ "rpcCode": -32022,�[39m
�[31m+ "errorKind": "session_transcript_changed",�[39m
�[31m+ "httpStatus": 409,�[39m
�[31m+ },�[39m
�[31m+ "sessionId": "550e8400-e29b-41d4-a716-446655440116",�[39m
�[31m+ },�[39m
�[2m ],�[22m
�[31m+ "resolvedConflicts": [],�[39m
�[2m }�[22m
�[36m �[2m❯�[22m src/serve/server/session-archive.test.ts:�[2m897:20�[22m�[39m
�[90m895| �[39m })�[33m;�[39m
�[90m896| �[39m
�[90m897| �[39m �[34mexpect�[39m(result)�[33m.�[39m�[34mtoMatchObject�[39m({
�[90m | �[39m �[31m^�[39m
�[90m898| �[39m archived�[33m:�[39m [sessionId]�[33m,�[39m
�[90m899| �[39m resolvedConflicts�[33m:�[39m [sessionId]�[33m,�[39m
�[31m�[2m⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯[3/6]⎯�[22m�[39m
�[41m�[1m FAIL �[22m�[49m src/serve/server/session-archive.test.ts�[2m > �[22munarchiveDaemonSessions�[2m > �[22mrepairs an active/archive conflict by keeping the active copy
�[31m�[1mAssertionError�[22m: expected { unarchived: [], …(4) } to match object { …(3) }
(2 matching properties omitted from actual)�[39m
�[32m- Expected�[39m
�[31m+ Received�[39m
�[2m {�[22m
�[32m- "errors": [],�[39m
�[32m- "resolvedConflicts": [�[39m
�[32m- "550e8400-e29b-41d4-a716-446655440117",�[39m
�[32m- ],�[39m
�[32m- "unarchived": [�[39m
�[32m- "550e8400-e29b-41d4-a716-446655440117",�[39m
�[31m+ "errors": [�[39m
�[31m+ {�[39m
�[31m+ "error": SessionTranscriptChangedError {�[39m
�[31m+ "message": "The session transcript changed outside its active writer.",�[39m
�[31m+ "name": "SessionTranscriptChangedError",�[39m
�[31m+ "rpcCode": -32022,�[39m
�[31m+ "errorKind": "session_transcript_changed",�[39m
�[31m+ "httpStatus": 409,�[39m
�[31m+ },�[39m
�[31m+ "sessionId": "550e8400-e29b-41d4-a716-446655440117",�[39m
�[31m+ },�[39m
�[2m ],�[22m
�[31m+ "resolvedConflicts": [],�[39m
�[31m+ "unarchived": [],�[39m
�[2m }�[22m
�[36m �[2m❯�[22m src/serve/server/session-archive.test.ts:�[2m1150:20�[22m�[39m
�[90m1148| �[39m })�[33m;�[39m
�[90m1149| �[39m
�[90m1150| �[39m �[34mexpect�[39m(result)�[33m.�[39m�[34mtoMatchObject�[39m({
�[90m | �[39m �[31m^�[39m
�[90m1151| �[39m unarchived�[33m:�[39m [sessionId]�[33m,�[39m
�[90m1152| �[39m resolvedConflicts�[33m:�[39m [sessionId]�[33m,�[39m
�[31m�[2m⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯[4/6]⎯�[22m�[39m
�[41m�[1m FAIL �[22m�[49m src/serve/server/session-archive.test.ts�[2m > �[22munarchiveDaemonSessions�[2m > �[22mkeeps independent unarchive sessions moving when one classification fails
�[31m�[1mAssertionError�[22m: expected [] to deeply equal [ Array(1) ]�[39m
�[32m- Expected�[39m
�[31m+ Received�[39m
�[32m- [�[39m
�[32m- "550e8400-e29b-41d4-a716-446655440022",�[39m
�[32m- ]�[39m
�[31m+ []�[39m
�[36m �[2m❯�[22m src/serve/server/session-archive.test.ts:�[2m1231:31�[22m�[39m
�[90m1229| �[39m })�[33m;�[39m
�[90m1230| �[39m
�[90m1231| �[39m �[34mexpect�[39m(result�[33m.�[39munarchived)�[33m.�[39m�[34mtoEqual�[39m([availableId])�[33m;�[39m
�[90m | �[39m �[31m^�[39m
�[90m1232| �[39m expect(result.errors).toEqual([{ sessionId: failedId, error: failu…
�[90m1233| �[39m })�[33m;�[39m
�[31m�[2m⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯[5/6]⎯�[22m�[39m
�[41m�[1m FAIL �[22m�[49m src/serve/server/session-archive.test.ts�[2m > �[22mdeleteDaemonSessions�[2m > �[22mdeletes the transcript when killSession throws SessionNotFoundError
�[31m�[1mAssertionError�[22m: promise rejected "SessionTranscriptChangedError: The sessio… { …(3) }" instead of resolving�[39m
�[36m �[2m❯�[22m src/serve/server/session-archive.test.ts:�[2m1772:5�[22m�[39m
�[90m1770| �[39m coordinator�[33m:�[39m �[35mnew�[39m �[33mSessionArchiveCoordinator�[39m()�[33m,�[39m
�[90m1771| �[39m })�[33m,�[39m
�[90m1772| �[39m )�[33m.�[39mresolves�[33m.�[39m�[34mtoBe�[39m(�[35mtrue�[39m)�[33m;�[39m
�[90m | �[39m �[31m^�[39m
�[90m1773| �[39m expect(fs.existsSync(sessionPath(workspaceDir, sessionId, 'active'…
�[90m1774| �[39m �[35mfalse�[39m�[33m,�[39m
�[31m�[1mCaused by: SessionTranscriptChangedError�[22m: The session transcript changed outside its active writer.�[39m
�[36m �[2m❯�[22m SessionService.readMaintainableSessionIdentity ../core/src/services/sessionService.ts:�[2m1095:15�[22m�[39m
�[90m �[2m❯�[22m SessionService.resolveMaintainableSessionSnapshot ../core/src/services/sessionService.ts:�[2m984:26�[22m�[39m
�[90m �[2m❯�[22m SessionService.getMaintainableSessionLocation ../core/src/services/sessionService.ts:�[2m959:15�[22m�[39m
�[90m �[2m❯�[22m deletePersistedSessionWithLease src/serve/server/session-archive.ts:�[2m413:27�[22m�[39m
�[90m �[2m❯�[22m SessionArchiveCoordinator.runExclusiveMany src/serve/server/session-archive.ts:�[2m109:14�[22m�[39m
�[90m �[2m❯�[22m deleteDaemonSessionIfOrphan src/serve/server/session-archive.ts:�[2m635:18�[22m�[39m
�[90m �[2m❯�[22m src/serve/server/session-archive.test.ts:�[2m1760:5�[22m�[39m
�[31m�[2m⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯�[22m�[39m
�[31m�[1mSerialized Error:�[22m�[39m �[90m{ rpcCode: -32022, errorKind: 'session_transcript_changed', httpStatus: 409 }�[39m
�[31m�[2m⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯[6/6]⎯�[22m�[39m
�[2m Test Files �[22m �[1m�[31m1 failed�[39m�[22m�[90m (1)�[39m
�[2m Tests �[22m �[1m�[31m6 failed�[39m�[22m�[2m | �[22m�[1m�[32m59 passed�[39m�[22m�[90m (65)�[39m
�[2m Start at �[22m 12:53:01
�[2m Duration �[22m 14.96s�[2m (transform 4.02s, setup 95ms, collect 5.91s, tests 340ms, environment 345ms, prepare 112ms)�[22m
JUNIT report written to /__w/qwen-code/qwen-code/packages/cli/junit.xml
round 1 · packages/core/src/services/session-writer-lease.test.ts: F (exit 1)
--- output tail · round 1 · packages/core/src/services/session-writer-lease.test.ts ---
ionWriterLease�[2m > �[22mseals a transcript proof and permits only certified takeover�[32m 20�[2mms�[22m�[39m
�[31m �[31m�[31m SessionWriterLease�[2m > �[22mwaits for an accepted append before sealing the transcript�[39m�[32m 56�[2mms�[22m�[39m
�[31m → The session transcript changed outside its active writer.�[39m
�[32m✓�[39m SessionWriterLease�[2m > �[22mreconciles a sealing error reported after the sealed primary is installed�[32m 8�[2mms�[22m�[39m
�[32m✓�[39m Ses
...truncated -- full content in the run artifacts.
Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention).
— Qwen Code · sandboxed verification
🩺 serve daemon A/BBuilt the PR base vs this PR head ✅ No response changes against the PR base across 12 scenario(s). — Qwen Code · serve A/B |
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. ✅
|
Superseded by #10300, which rebases the required writer-lease cleanup ownership fix onto the lifecycle implementation merged in #10179. The replacement keeps the narrowed scope, adds byte-identical lock-replacement protection, and carries the updated verification evidence. Closing this PR to avoid duplicate review and CI. |
2 participants
What this PR does
This PR separates the pre-mutation runtime-generation fence from post-transcript cleanup ownership. After delete, archive, unarchive, or conflict repair commits the primary transcript mutation, companion cleanup now continues only while the exact daemon writer lease is still owned. The ownership check rejects missing, replaced, malformed, non-regular, or symlinked lock entries and protects worktree, pull-request, prompt-ledger, file-history, and organization cleanup without falling back to another workspace runtime.
It also clarifies that conflict repair remains limited to regular transcript copies that the selected workspace may maintain; foreign or ambiguous copies continue to fail closed without mutation.
Why it's needed
A workspace runtime can close immediately after the transcript is deleted or moved. Reusing that runtime-generation fence for subsequent companion cleanup causes a committed lifecycle operation to stop halfway, leaving stale sidecars and metadata behind. Removing the fence entirely would create the opposite risk: a replacement writer could create same-session artifacts that the old operation then deletes. The writer-lease cleanup fence preserves both properties by allowing committed cleanup to finish while refusing to touch artifacts once ownership is lost.
Reviewer Test Plan
How to verify
Run the focused Core lifecycle and writer-lease tests and the CLI daemon lifecycle tests. Confirm that delete, archive, unarchive, and both conflict-repair directions finish companion cleanup after the runtime generation closes, while missing or atomically replaced writer locks preserve replacement-generation artifacts and return a writer-ownership error. Confirm that selected-runtime routes remain scoped to their trusted workspace and that PR refresh/backfill operations stay serialized with lifecycle mutations.
Local verification completed: Core focused tests passed with 363 passed and 3 skipped; CLI lifecycle tests passed 65/65; selected-runtime and Live organization boundary tests passed; PR refresh/backfill coordinator tests passed; the real filesystem/daemon harness passed twice plus one independent verification run; build, typecheck, lint, Prettier, and diff checks passed.
Evidence (Before & After)
N/A — lifecycle persistence and documentation changes with no user-interface change.
Tested on
Environment (optional)
macOS local workspace, Node.js v22.22.3, npm 10.9.8, sandbox disabled for the filesystem harness.
Risk & Scope
Linked Issues
Follow-up to #9626.
中文说明
本 PR 的改动
本 PR 将主变更前的运行时 generation 栅栏与 transcript 变更后的清理所有权分离。删除、归档、取消归档或冲突修复提交主 transcript 变更后，只有在 daemon 仍持有完全相同的 writer lease 时才继续清理伴随数据。所有权检查会拒绝缺失、被替换、格式损坏、非普通文件或符号链接形式的锁，并在不回退到其他 workspace runtime 的前提下保护 worktree、PR、prompt ledger、file history 和 organization 清理。
同时补充说明：冲突修复仍然只适用于所选 workspace 可维护的普通 transcript 副本；foreign 或所有权不明确的副本继续以不修改数据的方式 fail closed。
为什么需要
workspace runtime 可能在 transcript 被删除或移动后立即关闭。如果后续伴随清理继续复用 runtime generation 栅栏，已经提交的生命周期操作会中途停止，遗留过期 sidecar 和元数据。如果完全移除栅栏，则存在相反风险：替代 writer 可能创建同 session 的新数据，而旧操作随后将其删除。writer lease cleanup 栅栏同时保证两点：仍持有所有权时完成已提交的清理，所有权丢失后拒绝触碰新数据。
Reviewer 测试计划
如何验证
运行 Core lifecycle、writer lease 聚焦测试和 CLI daemon lifecycle 测试。确认 delete、archive、unarchive 以及两个方向的冲突修复在 runtime generation 关闭后仍能完成伴随清理；同时确认 writer lock 缺失或被原子替换时会保留替代 generation 的数据，并返回 writer ownership 错误。确认 selected-runtime 路由仍限制在其可信 workspace 内，PR refresh/backfill 操作仍与生命周期变更串行化。
本地验证已完成：Core 聚焦测试 363 passed、3 skipped；CLI lifecycle 测试 65/65；selected-runtime 和 Live organization 边界测试通过；PR refresh/backfill coordinator 测试通过；真实 filesystem/daemon harness 主流程运行两次并由测试工程师独立复跑一次，全部通过；build、typecheck、lint、Prettier 和 diff 检查均通过。
证据（修改前与修改后）
N/A——这是 lifecycle persistence 和文档改动，没有用户界面变化。
测试平台
环境（可选）
macOS 本地 workspace，Node.js v22.22.3，npm 10.9.8；filesystem harness 在 sandbox 关闭的环境中运行。
风险与范围
关联 Issue
#9626 的 follow-up。