Repository navigation
feat(cli): reap owned worktrees when daemon sessions are deleted - #11309
Conversation
POST /sessions/delete removed the session record and sidecar but never the owned checkout, marker, git registration, or branch, so deletes of worktree-owning sessions leaked them permanently. deleteDaemonSessions now classifies ownership before the record deletion (strict sidecar + marker + containment + cross-sidecar sharing scan, executed under the worktree ownership lock moved to a shared path-keyed module) and, after a confirmed removal, safely deletes the checkout and branch — never forcing the branch delete, preserving dirty checkouts, superseded predecessors, tombstone-shared checkouts, and anything doubtful with a named log line. Refs QwenLM#11024
…gate The cleanliness gate now runs a full git status --porcelain (untracked files included, marker file exempted) so an agent-written draft that was never committed preserves the checkout instead of being silently destroyed. Also covers the invalid-marker branch and fixes the design doc's logger reference.
|
@qwen-code /triage |
chiga0
left a comment
chiga0
left a comment
There was a problem hiding this comment.
PR #11309 Review — feat(cli): reap owned worktrees when daemon sessions are deleted
Tier: Deep（不可逆文件系统删除 + 并发生命周期，涉及锁序和 TOCTOU 安全合约）
审查范围
| 文件 | 关注维度 |
|---|---|
worktree-orphan-cleanup.ts |
三阶段流程（preclassify→verify→execute），ownership 验证逻辑 |
worktree-ownership-op.ts |
promise-chain mutex，map 清理，release 幂等性 |
session-archive.ts（deleteDaemonSessions） |
runWithWorktreeCleanup 集成，coordinatorLockHeld 分支 |
session.ts |
旧桥接作用域锁 → 共享模块迁移，调用点签名变更 |
| design doc | 安全合约、锁序、失败语义 |
结果：无阻塞问题
锁序（Class 7 / 并发）：CLEAN
设计文档要求 worktree lock → coordinator lock 的一致顺序。经核查：
- Part 4A restore（
session.ts行 ~3925）：acquireWorktreeOwnershipOp→ coordinator - Part 4B reset（
session.ts行 ~4673）：acquireWorktreeOwnershipOp→ coordinator - 删除路径（
session-archive.ts）：acquireWorktreeCleanupLock→coordinator.runExclusiveMany
三条路径均遵循同一顺序，不存在 AB-BA 死锁风险。 ✓
分类在删除前，执行在确认删除后（Class 8 / 时序）：CLEAN
acquireCleanupLock
verifyOwnership (under lock, before sidecar is destroyed)
coordinator.runExclusiveMany → delete record + sidecar
if (ownership.ok && result.kind === 'removed') → executeCleanup
releaseCleanupLock
sidecar 在 record 删除时被销毁，verifyWorktreeCleanupOwnership 在此之前完成读取；executeWorktreeCleanup 仅在 kind === 'removed' 后触发。 ✓
marker 在执行阶段二次验证（Class 1 / 合约对称）：CLEAN
executeWorktreeCleanup 进入后立即再次读取 marker（readWorktreeSessionMarkerStrict），任何外带修改均会被检测到并保留 checkout。 ✓
coordinatorLockHeld: true 路径完全跳过清理（Class 7）：CLEAN
return await (coordinatorLockHeld ? mutateSession() : runWithWorktreeCleanup(sessionId, mutateSession));内部 runtime 直接调用 mutateSession()，不进入 cleanup 路径，不会产生 worktree→coordinator 的逆序。 ✓
checkoutHasWork 安全合约：CLEAN
git status --porcelain 包含 untracked 文件，仅豁免 .qwen-session（line.slice(3) !== WORKTREE_SESSION_FILE）；任何 read error 均返回 true（fail closed）。 ✓
并发批量删除（Class 7）：CLEAN
两个并发删除同一 session：第一个在 worktree lock 下验证并删除；第二个等锁后重新读取 sidecar，发现 missing，verifyOwnership 返回 { ok: false } → 跳过清理；coordinator.runExclusiveMany 返回 notFound。 ✓
supersededBy / supersedes 分类语义：CLEAN
pre-classification 检测 supersededBy 则静默跳过（checkout 已转让）；supersedes 不触发跳过，本 session 仍是当前 owner。与设计文档的第一次 A/B 验证结论一致。 ✓
旧桥接作用域锁迁移：CLEAN
session.ts 移除了 WeakMap 作用域的本地 acquireWorktreeOwnershipOp 闭包（47 行删除），统一改用 worktree-ownership-op.ts 的模块级版本，仅去掉 bridge 参数，功能语义等价。 ✓
非阻塞观察
-
current.originalCwd无显式 undefined 检查：verifyWorktreeCleanupOwnership将current.originalCwd传入worktreeAllowedRoots(workspaceCwd, originalCwd)（参数类型为string），但 guard 只检查了current.workspaceCwd === undefined。若WorktreeSession.originalCwd在类型上可以为string | undefined，则当 sidecar 是某种缺originalCwd的历史格式时，这里会抛出 TypeError，异常会被外层try/catch捕获为phase: 'delete'错误，阻止 session 删除（而非仅跳过清理）。若originalCwd在类型定义中为string（非 optional），TypeScript 会保证此处安全。建议确认核心类型定义。不阻塞合并。 -
模块级 Map 替代 WeakMap：旧版以
WeakMap<bridge, Map<path, tail>>在 bridge 被 GC 时自动清理 key。新版以模块级Map存储，清理完全依赖 release 回调。release 函数的releasedflag 防止重复调用，逻辑正确；但进程 kill 会导致 key 在本次进程生命周期内永久 pending（等同于任何 in-memory mutex 的行为，fail closed）。
覆盖率声明
已覆盖 5 个核心文件的静态分析（锁序、时序合约、并发场景）。未进行本地执行（工作区不可用）——单测（474 行新增覆盖多个保留/删除场景）和 CI 结果依赖 CI bot（已报绿）。
Reviewed with AI assistance.
Independent verification — PR #11309 at head
|
| # | site | originalCwd |
sets workspaceCwd? |
|---|---|---|---|
| 1 | packages/core/src/tools/enter-worktree.ts:195 |
projectRoot (:203) |
no |
| 2 | packages/cli/src/startup/worktreeStartup.ts:446 |
context.repoRoot (:450) |
no |
| 3 | packages/cli/src/serve/routes/session.ts:3496 |
workspaceCwd (:3504) |
yes |
| 4 | packages/cli/src/serve/routes/session.ts:4822 |
sidecar read back from disk, verbatim | inherited |
| 5 | packages/cli/src/serve/routes/session.ts:4946 |
...effectiveOldSidecar + supersededBy |
inherited |
| 6 | packages/cli/src/serve/routes/session.ts:4960 |
...oldSidecarCore + supersedes |
inherited |
| 7 | packages/cli/src/serve/routes/session.ts:5138 |
effectiveOldSidecar, verbatim |
inherited |
Finding 2 (verify A / remove B) — the invariant is enforced by a gate, not by convention. preclassifyWorktreeCleanup returns undefined at worktree-orphan-cleanup.ts:130 whenever session.workspaceCwd === undefined, so sites 1 and 2 never reach the cleanup path at all. Those are precisely the two writers whose originalCwd is a repo top-level rather than the registered workspace root — the WorktreeSession.originalCwd doc comment (worktreeSessionService.ts:38-48) states both populations explicitly ("Tool and startup flows store the Git repo top-level here, while legacy daemon-created worktrees may store the registered workspace root"), so the divergent population is exactly the excluded one.
Of the writers that do set workspaceCwd: site 3 pairs worktreePath: worktreeMeta.path — from wtService.createUserWorktree(slug, baseBranch) at session.ts:3165, where wtService = new GitWorktreeService(workspaceCwd) (:3003, :3126) — with originalCwd: workspaceCwd. Both create and remove derive from the same field: createUserWorktree builds path.join(getUserWorktreesDir(), slug) (gitWorktreeService.ts:2108-2110) and removeUserWorktree calls getUserWorktreePath(slug) (:2636 → :1581 → :1575), both rooted at this.sourceRepoPath = path.resolve(sourceRepoPath) (:684). One service instance therefore cannot create at one path and remove at another. Sites 4 and 7 rewrite a sidecar read from disk; site 6 spreads oldSidecarCore, carrying worktreePath/originalCwd/workspaceCwd together; site 5 adds supersededBy, which makes preclassify skip it at worktree-orphan-cleanup.ts:131.
So the divergent state needs a sidecar that no production writer produces. High, not Critical — but the bot's conclusion stands unchanged and is worth repeating: nothing asserts the equality at the one place that calls delete, and its M7/M8 mutation survivors show the suite cannot tell head from head-plus-fix on either axis. A fix here must ship its own fixture.
Finding 1 (sharing scan reads one chats directory) — blocked structurally by the marker, not by the scan. The marker is a single file inside the checkout naming exactly one session, and verifyWorktreeCleanupOwnership requires marker.sessionId === sessionId (worktree-orphan-cleanup.ts:196-204). For a target to pass that check while an archived sibling also names the checkout, something must have moved the marker off the sibling — and every production path that does so either sets supersededBy on it (site 5, which the scan already discounts and preclassify already skips) or writes a sidecar with no workspaceCwd (site 2, whose re-attach adoption at worktreeStartup.ts:422-443 is itself gated on the previous owner not being a live runtime). I could not construct a reachable sequence either.
The part I would fix regardless is the silence. On this path a checkout is destroyed and deregistered with zero log lines, while every neighbouring preserve path emits a named reason via logWorktreeCleanupPreserve. The preserve-and-log design is the operator's only window into these decisions; the one branch that skips the scan skips the window too.
§2 — The open question in the approving review, settled
The approval at 16:47:58Z records one non-blocking observation it could not resolve: whether WorktreeSession.originalCwd is optional, because if it were, worktreeAllowedRoots(current.workspaceCwd, current.originalCwd) (worktree-orphan-cleanup.ts:206-209) would throw a TypeError inside the ownership check, which the outer catch reports as phase: 'delete' and thereby blocks the session deletion rather than merely skipping cleanup.
It is not optional. worktreeSessionService.ts:49 declares originalCwd: string;, while workspaceCwd?: string; sits at :51 — the one optional member is exactly the one the guard tests (worktree-orphan-cleanup.ts:130 and :186). There is no TypeError path. Independently: Lint & Static (ubuntu-latest, Node 22.x) is SUCCESS at this head, and that lane would have failed on the assignment if the field were optional.
§3 — 🔴 The determinism gate failed at this head, the failure is PR-caused, and neither approval mentions it
This is the item I would most want a human to read before merging.
The sandboxed verification reports session-archive.test.ts: PFPPP over five identical rounds and therefore "not passed ... regardless of the agent verdict". The failing test is pre-existing, not one of the 17 new worktree-cleanup tests — all 17 passed in the failing round. It is deleteDaemonSessions > reports a gate race per session after another batch item was deleted (session-archive.test.ts:1763).
Its reported stack maps frame-for-frame onto this PR's own change, and I re-read every frame at head:
session-archive.ts:630 the ternary this PR rewrote
runWithWorktreeCleanup session-archive.ts:541
runExclusiveMany session-archive.ts:116 (return await fn())
mutateSession session-archive.ts:611 (await bridge.closeSession(sessionId))
test:1782 competingMaintenance = coordinator.runExclusiveMany([blockedId], …)
runExclusiveMany session-archive.ts:106
assertNotTransitioning session-archive.ts:91 → throw SessionArchivingError
+ Unhandled Rejection of the same error
Mechanism. The entire diff at that call site is coordinator.runExclusiveMany([sessionId], mutateSession) → runWithWorktreeCleanup(sessionId, mutateSession) (+49/−1), and runWithWorktreeCleanup awaits preclassifyWorktreeCleanup at :539 before it reaches the coordinator. Promise.all(ids.map(async …)) runs each task's synchronous prefix in array order, so before this PR removedId's task reached closeSession(removedId) — which arms the competing registration on blockedId — before blockedId's task got its first turn, and the expected per-session gate race was guaranteed. With one await inserted ahead of the coordinator call, both tasks suspend on a filesystem read first, and whichever read completes second enters runExclusiveMany second. When blockedId wins, it registers and deletes itself; removedId's later closeSession then finds blockedId already transitioning and rejects a promise the test does not await until its finally — the observed unhandled rejection and inverted assertions.
Qualification added after posting — the interleaving above is an INFERENCE FROM THE STACK, NOT A REPRODUCTION, and my attempt to reproduce it failed. What the evidence does establish: the throw site is the test's own competing coordinator.runExclusiveMany([blockedId], …), and assertNotTransitioning throws only when exclusive.has(blockedId) is already true (session-archive.ts:90-91), so blockedId was registered at that moment. The coordinator in this test is freshly constructed and the bridge is a mock, and exclusive has only two add sites — :112 inside runExclusiveMany and :138 inside runExclusiveAfterShared, whose seven callers are all in standalone-session-service.ts and none of which this test reaches — so the registrant can only have been blockedId's own task entering runExclusiveMany ahead of closeSession(removedId). That much is sound.
What is not demonstrated is that the inserted await is what permits the flip. A 60-run fixture mirroring the verified shapes (Promise.all([A, B].map(task)); task = optional await preclassify then runExclusiveMany([id], mutateSession); preclassify = two failing readFiles on the sidecar and archive-sidecar paths; closeSession(A) arming a never-resolving runExclusiveMany([B])) gave identical, deterministic results in both arms — 60/60 [removed, ERR:SessionArchivingError:bbbb], with B never registered before closeSession(A): the two reads resolved in issue order every time. A 1-in-5 rate points at latency variance this fixture does not have (threadpool contention from concurrent vitest workers, and the real deletePersistedSessionWithLease awaits the fixture omits), but that is a reading of the rate, not a measurement of the cause, and it leaves room for a second contributing factor.
So the ordering claim should be confirmed by re-running the file under instrumentation rather than taken from this report. Neither the classification (test-determinism regression, not a production Critical) nor the two fix directions depends on it — both follow from the verified one-line diff and the verified stack alone.
Why I am not filing this as a Critical. The reordering is real and PR-introduced, but I could not make it yield a wrong production result: Promise.all preserves input order for its result array, so removed[] / errors[] in the delete response stay request-ordered whatever the completion order, and the coordinator-registration race the test stages has no production analogue I could find (nothing in bridge.closeSession registers other sessions with the coordinator). It is a test-determinism regression that will surface as intermittent red on unrelated PRs, not a defect in the feature — so under a merge-blocking-Criticals-only bar it is not something I file request-changes for.
It is still worth fixing before merge, and either direction is small: keep the coordinator call on the synchronous prefix (classify after acquiring, or give the no-sidecar case a fast path that does not await), or make the test order-independent. What I would not do is merge it as environmental noise — the gate's own reasoning ("a test that can fail with no code changing lands as intermittent red on unrelated PRs") is correct here, and the cause is in this diff.
§4 — What I re-confirmed independently
- Release pairing / no wedge.
acquireWorktreeCleanupLockatsession-archive.ts:543→tryat:544with no intervening statement →finally { releaseCleanupLock(); }at:564-566. Sinceworktree-ownership-op.tsis now a process-wideMap, a missed release would hang every later restore/reset/delete for that checkout; this path cannot miss it. - Release identity guard.
if (worktreeOwnershipOpTails.get(worktreeKey) === tail)compares against the closure's owntail, so a release can never delete a successor's entry, and thereleasedflag makes it idempotent. - No chain poisoning.
releaseGateisgate'sresolve;gatenever rejects, the base case isPromise.resolve(), so notailin the map can reject. routes/session.ts(+3/−47) is a pure scope migration. The deleted per-bridgeWeakMap<AcpSessionBridge, Map<string, Promise<void>>>closure and the new module-levelMaphave identical chain logic; the two call sites (:3925,:4673) only drop thebridgeargument. Widening per-bridge → process-wide adds serialization, it does not remove any.
§5 — Not covered by this report
No execution of any kind, so: no A/B artifact-leak measurement of my own (the 9/9 → 0/9 result is the sandboxed verification's, and I cite rather than reproduce it), no M1–M8 mutation runs, no fail-closed matrix, no real-daemon HTTP drive, no instrumentation of the flaky test itself (the fixture described in §3's qualification models the scheduling shapes but is not the repo's test and does not reproduce the failure), no macOS/Windows check — and the last one matters here, because path-separator handling and macOS /tmp → /private/tmp realpath behaviour interact with both canonicalPath (worktree-orphan-cleanup.ts:41-47) and the deliberately un-realpath'd path.resolve(originalCwd) in Finding 2. Nothing in §1–§4 should be read as covering those axes.
Conclusion — and why there is no approval from me
No Critical findings at head 5c9593c5e5eb, and I agree the two High findings are correctly classified as non-blocking. But this review only approves behind its own executed end-to-end run, and I have executed nothing here — so the approval precondition is not met on my side and I am not supplying one.
The human-judgement item is §3. Two approvals landed at this head at 16:41:03Z and 16:47:58Z, both after the 16:16:53Z verification that reports this head as "not passed" on a determinism failure whose cause is in this diff, and neither approval addresses it. Whether a PR-introduced flaky test blocks the merge is a maintainer call, not a reviewer's — but it should be made deliberately rather than by reading the green CI lanes only.
中文摘要
仅静态读码验证，未构建、未执行任何测试（无 bundle、无 vitest、无变异、无 POST /sessions/delete 实驱、无 tmux 交互）。所有行号均在 head 5c9593c5e5eb 的干净检出中逐条复核。
结论：该 head 无 Critical。 同意沙箱验证对两项 High finding 的定级（High 而非 Critical），并补上它自己声明未覆盖的「生产可达性」：穷举 head 上全部 7 处非测试 writeWorktreeSession 调用点（该报告只列了 4 处）后，Finding 2 的不可达是由 workspaceCwd 这道门保证的——preclassifyWorktreeCleanup 在 worktree-orphan-cleanup.ts:130 对没有 workspaceCwd 的 sidecar 直接返回 undefined，而被排除的恰好就是 originalCwd 存 repo top-level 的那两个写入点（enter-worktree.ts:195、worktreeStartup.ts:446）；其余写入点要么成对写入（:3496 的 wtService = new GitWorktreeService(workspaceCwd) 与 originalCwd: workspaceCwd，且 create/remove 同源于 this.sourceRepoPath），要么整体继承。Finding 1 则由 marker 单文件单会话身份检查（:196-204）结构性拦住。但「无任何断言保证校验路径 == 删除路径」这一点不变，M7/M8 变异全绿说明测试也没有钉住，修复必须自带 fixture。
同时确定性地回答了 approve 评审中悬置的那个问题：WorktreeSession.originalCwd 是 string，非可选（worktreeSessionService.ts:49；唯一可选的是 :51 的 workspaceCwd?，也正是守卫检查的那个），故不存在 TypeError 阻断删除的路径。
🔴 最需要人工判断的一条：抖动门在本 head 判「不通过」，且抖动是本 PR 引入的，两次 approve 都没有提及。 失败的是既有测试 reports a gate race per session after another batch item was deleted（session-archive.test.ts:1763，17 个新测试当轮全绿）。本 PR 在该调用点唯一的改动是把 coordinator.runExclusiveMany(...) 换成 runWithWorktreeCleanup(...)，而后者在进入 coordinator 之前先 await preclassifyWorktreeCleanup（:539）。Promise.all(ids.map(async …)) 原本按数组顺序同步执行每个 task 的前缀，因此 removedId 必定先进入 closeSession；插入一个 await 后两个 task 都先挂起在文件系统读取上，谁后完成谁后进 coordinator，blockedId 抢先时就会自我注册并删除自己，随后 removedId 的 closeSession 发现它已在 transitioning 而 reject 一个测试直到 finally 才 await 的 promise —— 即日志中的 Unhandled Rejection 与断言反转。
我没有把它作为 Critical 提交：Promise.all 的结果数组保持入参顺序，故删除响应的 removed[] / errors[] 仍是请求序；我在生产路径上也找不到能让 bridge.closeSession 去注册其他 session 的对应场景。所以这是测试确定性回归（会以间歇红灯落到无关 PR 上），不是功能缺陷。但它是抖动门判「不通过」的真实原因，且原因在本 diff 内，不建议当作环境噪声合入；修法两个方向都很小：让 coordinator 调用回到同步前缀（先取锁再分类，或给无 sidecar 情况一条不 await 的快路径），或让该测试与顺序无关。
（发布后补充限定）上面那段交错叙述是「由调用栈推断」，不是复现结果，而且我事后尝试复现失败了。 证据能确立的是：抛出点正是测试自己的 coordinator.runExclusiveMany([blockedId], …)，而 assertNotTransitioning 只在 exclusive.has(blockedId) 已为真时抛出（session-archive.ts:90-91），故那一刻 blockedId 确实已被注册；该测试的 coordinator 是新建的、bridge 是 mock，而 exclusive 只有两个 add 点（:112 在 runExclusiveMany、:138 在 runExclusiveAfterShared，后者 7 个调用点全在 standalone-session-service.ts，本测试都到不了），所以注册者只能是 blockedId 自己的 task 先于 closeSession(removedId) 进入了 runExclusiveMany。未能确立的是「插入的那个 await 就是允许翻转的原因」：一个按已核实结构搭的 60 轮 fixture（两侧唯一差别就是有没有那个 await）两臂结果完全一致且确定——60/60 [removed, ERR:SessionArchivingError:bbbb]，B 从未在 closeSession(A) 之前被注册，两次 readFile 每次都按发起顺序完成。1/5 的频率指向该 fixture 不具备的延迟抖动（vitest 并发 worker 的线程池竞争、以及 fixture 省略的 deletePersistedSessionWithLease 真实 await），但这是对频率的解读、不是对成因的测量，因此不排除还有第二个成因。结论：该顺序主张应当用带插桩的重跑来确认，不要直接采信本报告。 定级（测试确定性回归，非生产 Critical）与两个修复方向都不依赖它——它们只依赖已核实的那一行 diff 和已核实的调用栈。
本轮不给 approve：本巡检只在自己跑过端到端之后才 approve，而我这次没有执行任何东西。
Correction — PR #11309 at head
|
…ified - assert the removal target (originalCwd + slug) realpaths to the checkout every guard inspected, and consume the locked sidecar - pin git status flags and scrub the environment in checkoutHasWork, exempting only disposable build output from the ignored tier - refuse to fold the record on a session_closing refusal from a bridge-internal auto-close - re-assert the workspace generation guard before the destructive step - re-observe the checkout before logging preservation on removal failure, naming partial deletion when the shell is empty - reject non-absolute or foreign sidecar bases and thread the runtime workspace cwd into the containment check - keep no-sidecar deletes on the synchronous prefix so batch scheduling stays deterministic (gate-race flake)
Review round 1 addressed —
|
| Finding | Disposition |
|---|---|
| R1-1 removal targets an unverified path | Fixed — equality guard (canonicalized both sides) + locked sidecar carried onto the plan |
R1-2 checkoutHasWork inherits ambient git answer |
Fixed — -u normal + --ignored=matching + gitEnv(), disposable roots exempted |
R1-3 session_closing folds the record |
Fixed — fold requires code !== 'session_closing'; closing-in-flight errors out retryably |
R1-4 cleanup skips assertCanMutate |
Fixed — re-assert immediately before the destructive step |
| R1-5 failure logs certify preservation | Fixed — re-observe the checkout; empty shell logs "may be partially deleted" instead |
| R1-6 containment derived from untrusted bases | Fixed — non-absolute bases rejected; runtime workspace threaded and realpath-compared |
| Flakiness gate (gate-race test) | Fixed — no-sidecar deletes no longer await before the coordinator, restoring the synchronous-prefix scheduling the test stages; 8/8 deterministic |
Suggestions R1-7 – R1-22 — accepted, deferred to a follow-up round
All sixteen are defensible and none are declined; they cluster into docs corrections (R1-7, R1-16, R1-17, R1-20), observability wording (R1-8, R1-9, R1-15), test-hardening (R1-10, R1-11, R1-12, R1-18, R1-21, R1-22), and two design edges that deserve their own round rather than riding this one (R1-13 mutationApplied:true error arm skipping both cleanup and warning; R1-14 cleanup running outside the coordinator's maintenance accounting so shutdown can truncate a removal; R1-19 tombstones outliving the checkout and blocking slug reuse). Keeping this commit reviewable — follow-up round next.
|
@qwen-code /triage |
chiga0
left a comment
chiga0
left a comment
There was a problem hiding this comment.
PR #11309 Review — feat(cli): reap owned worktrees when daemon sessions are deleted
Tier: Deep (不可逆文件系统操作 + 并发生命周期 + 锁序安全合约)
审查结果：无阻塞问题，批准 ✅
已检查
锁序（worktree → coordinator）：CLEAN — 三条调用路径（Part 4A restore / Part 4B reset / delete path）均遵循 worktree lock → coordinator lock。coordinatorLockHeld: true 路径完全跳过 runWithWorktreeCleanup，不存在 AB-BA 死锁。
分类前置 / 执行后置（sidecar 生命周期合约）：CLEAN — verifyWorktreeCleanupOwnership 在 record 删除前 worktree lock 下完成；executeWorktreeCleanup 仅在 result.kind === 'removed' 后触发。
Marker 二次验证（TOCTOU）：CLEAN — executeWorktreeCleanup 入口重读 marker，关闭 verify 与 execute 之间的带外修改窗口。
session_closing guard：CLEAN — 正确区分"会话正在自动关闭中（child 可能仍持有 cwd）"和"会话真正不存在"，仅后者折叠 record。测试 #b5 / #b6 覆盖两分支。
Slug 重推导等值守卫：CLEAN — verifyWorktreeCleanupOwnership 末尾将 originalCwd + slug 重推导为 removalTarget 并断言等于 plan.lockKey，防止 slug 注入指向另一 checkout。测试 #b0 / #b1 覆盖。
checkoutHasWork 安全合约：CLEAN — --untracked-files=normal pin 防 ambient config 干扰；--ignored=matching + DISPOSABLE_IGNORED_ROOTS 仅豁免可再生产物；read error fail-closed；唯一单文件豁免为 .qwen-session。测试 #b2 / #b3 / #b4 覆盖。
acquireWorktreeOwnershipOp 重构：CLEAN — 从 WeakMap<bridge, …> 改为模块级 Map<path, tail>，release callback 删除条目，map 仅在操作 in-flight 期间增长。
批量并发删除：CLEAN — 两个并发 delete 同一 session，后得锁者在 verify 中发现 sidecar missing → ok: false → 跳过清理，无竞态。
Cross-check（与 R1 轮次对比）
R1-1 ～ R1-6（先前 CHANGES_REQUESTED 的 6 条 Critical）均在当前头 f77d9435 已修复并有对应测试 pin。与 CI bot 第二轮 APPROVED 结论一致。
未审查维度
- 执行层：无本地工作树，未运行测试套件。
- Inline comments 列表：API 获取失败，无法确认是否有未解决线程；review 正文确认无开放阻塞项。
Reviewed with AI assistance.
4 participants
What this PR does
Deleting a daemon session that owns a worktree now also removes the owned checkout and its branch — safely and only when ownership is unambiguous. Before the record is deleted, the delete path classifies the session's ownership chain under the worktree ownership lock (strict sidecar read, in-worktree marker, path containment, and a cross-session sharing scan); after the deletion is confirmed, it removes the checkout only when the tree is clean, and deletes the branch without ever forcing it. Any doubt — unreadable sidecar, marker mismatch, shared checkout, dirty tree, containment failure — preserves the checkout and logs a named reason. A superseded predecessor is the expected post-reset state and is skipped silently, while a post-reset replacement (the current owner) is cleaned normally with its tombstone predecessor discounted from the sharing scan. The design is documented in
docs/design/channel-worktree-orphan-reap.md.Why it's needed
Until now, deleting a worktree-owning session removed its record and sidecar but never the checkout, the in-worktree ownership marker, the git worktree registration, or the branch — a real-daemon leak map confirmed these artifacts accumulate permanently on every delete. The existing stale-worktree sweep deliberately never touches named worktrees, so Part 4A channel worktree sessions had no cleanup path at all. This was flagged during the Part 4A review (#10643) and is tracked as one of the two items in #11024.
Reviewer Test Plan
How to verify
Against a real daemon, create a channel worktree session and delete it: the record, sidecar, checkout, marker, registration, and branch should all be gone (previously only the record and sidecar were removed). Variants a reviewer should confirm behave conservatively: a checkout with tracked modifications is preserved with a
checkout has tracked changeslog line; a checkout whose branch carries unmerged commits is removed while the branch is kept and logged; a superseded session deletes its record but leaves the replacement's checkout untouched with no warning; a post-reset replacement deletes cleanly even while its tombstone predecessor's sidecar still names the same checkout. Unit coverage: 15 new tests in the session-archive suite exercise every classification and execution branch against real git repositories (85 tests in the file pass, plus the worktree-reset route suite and the core worktree suites).Evidence (Before & After)
N/A — daemon-internal behavior verified by real-daemon A/B: before,
POST /sessions/deleteon a worktree session left the checkout, marker, registration, and branch behind every time; after, the same delete removes the full artifact set when ownership verifies and preserves it with a named log line otherwise. The full leak map and both verification rounds are in the issue thread for #11024.Tested on
Environment (optional)
Real daemon (
node dist/cli.js serve) driven over the HTTP API; unit tests use real git repositories in temp dirs.Risk & Scope
Linked Issues
Refs #11024 (item 1 — orphan-reap worktree cleanup, flagged by yiliang114 on #10643)
中文说明
本 PR 的内容
删除拥有 worktree 的 daemon 会话时，现在会一并移除其拥有的 checkout 与分支——以安全方式、且仅在所有权无歧义时执行。删除记录之前，删除路径会在 worktree 所有权锁下对该会话的所有权链进行分类（严格 sidecar 读取、worktree 内 marker、路径包含性、跨会话共享扫描）；删除确认之后，仅在工作区干净时移除 checkout，且删除分支绝不使用强制。任何疑点——sidecar 不可读、marker 不匹配、checkout 被共享、工作区脏、包含性失败——都会保留 checkout 并记录具名原因。被取代的前任是 reset 后的预期状态，静默跳过；而 reset 后的替换会话（当前所有者）可正常清理，其前任墓碑 sidecar 在共享扫描中被豁免。设计文档见
docs/design/channel-worktree-orphan-reap.md。为什么需要
此前删除拥有 worktree 的会话只移除其记录与 sidecar，从不移除 checkout、worktree 内所有权 marker、git worktree 注册与分支——真 daemon 泄漏测绘确认这些产物在每次删除后永久累积。现有的 stale worktree 清扫有意不触碰命名 worktree，因此 Part 4A 的 channel worktree 会话完全没有任何清理路径。该问题在 Part 4A 评审（#10643）中被指出，作为 #11024 的两个事项之一被跟踪。
评审者测试计划
如何验证
对真实 daemon 创建一个 channel worktree 会话并删除：记录、sidecar、checkout、marker、注册与分支应全部消失（此前只有记录与 sidecar 被移除）。建议评审者确认以下变体的保守行为：有跟踪文件修改的 checkout 被保留并记录
checkout has tracked changes日志；分支携带未合并提交的 checkout 被移除但分支保留并记录日志；被取代的会话删除其记录但不触碰替换会话的 checkout 且无告警；reset 后的替换会话即使其前任墓碑 sidecar 仍指向同一 checkout 也能正常清理。单元覆盖：session-archive 套件新增 15 个测试，基于真实 git 仓库覆盖全部分类与执行分支（该文件 85 个测试通过，另有 worktree-reset 路由套件与 core worktree 套件）。证据（前后对比）
N/A——daemon 内部行为，已通过真 daemon A/B 验证：修复前
POST /sessions/delete删除 worktree 会话后每次都残留 checkout、marker、注册与分支；修复后相同删除在所有权验证通过时移除全部产物，否则保留并记录具名日志。完整泄漏矩阵与两轮验证记录见 #11024 的 issue 讨论。测试平台
Linux 已测试；macOS / Windows 未测试（CI 覆盖）。
环境（可选）
真实 daemon（
node dist/cli.js serve）通过 HTTP API 驱动；单元测试使用临时目录中的真实 git 仓库。风险与范围
关联 Issue
Refs #11024（事项 1——孤儿 worktree 回收，由 yiliang114 在 #10643 指出）