fix(core): Preserve ownership during session cleanup - #10300
Conversation
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
E2E and verification reportBaseline verification on the exact current Post-fix real-filesystem verification confirmed that all five scenarios converge when the original writer lease remains owned. Missing, malformed, non-regular, symlinked, replaced, and byte-identical atomically replaced writer locks all fail closed; the byte-identical replacement case preserves the new generation's worktree, PR, prompt-ledger, file-history, and organization state. Local automated verification:
Deliberately deferred because they are not required for this ownership fix: scheduled-task updates, attachment cleanup, final route-response semantics, mixed-case lookup, transcript/list merging, parent-child cascading deletion, and the existing standalone exact-retry result bucket behavior. None of these deferrals broadens or weakens the writer-lease ownership boundary implemented here. |
|
Re-run after the author pushed Template looks good ✓ Problem: real, and the mechanism is confirmed in code. On current Direction: aligned. A correctness fix for recently merged daemon session lifecycle code; the writer lease — which the daemon still holds across a generation closure — is a strictly more precise post-commit ownership signal than the generation counter. No external CHANGELOG signal applies (qwen-serve-internal), but the area is squarely in scope. Size: core paths touched ( Approach: the scope feels right. One new ownership primitive ( Risk: no elevated risk signals — none of the changed files match the repo's revert-correlated high-risk paths. Moving on to code review. 🔍 中文说明作者在原提交之上推送了 模板完整 ✓ 问题：真实存在，且机制已在代码中确认。当前 方向：对齐。这是对近期合入的 daemon session 生命周期代码的正确性修复；daemon 在 generation 关闭后仍然持有的 writer lease 是比 generation 计数更精确的提交后所有权信号。外部 CHANGELOG 无参考信号（qwen-serve 内部机制），但该领域完全在范围内。 规模：触及核心路径（ 方案：范围合理。一个新的所有权原语（ 风险：无升级风险信号——变更文件均未命中与 revert 相关的高风险路径。 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewBefore reading the diff I wrote down my own solution: after the primary transcript mutation commits, gate auxiliary cleanup on the writer lease instead of the runtime generation — the lease is the one ownership token that survives a generation closure. It would need to capture the lock file's identity at acquisition (so a byte-identical replacement is detectable), re-assert cheaply between cleanup steps, and fail closed on any ambiguous lock state. The PR does exactly this, and the implementation matches the approach I'd have taken. What I verified at the current head (
Three non-blocking observations:
The flow after the change: sequenceDiagram
participant D as Daemon lifecycle
participant L as Writer lease
participant S as SessionService
D->>L: acquire lease, capture lock dev-ino and bytes
D->>S: primary transcript mutation (generation fence still guards)
S-->>D: committed
loop each auxiliary cleanup step
D->>L: assertCleanupOwned, lock identity only
L-->>D: owned, continue
end
D->>L: release
CI test evidenceUnattended CI run — evidence below is the PR's own CI read through the API, plus the repo's serve A/B job. No PR code was built or executed here. The previously red
The suite pins the changed behavior at the unit level — the renamed tests previously asserted that a closed generation stops post-commit cleanup and now assert cleanup finishes, so they fail without the diff — and the nasty lease cases (byte-identical replacement, symlink) run against the real filesystem. Not verified here: the live-daemon interleaving itself (the unit suite models it with mocked fence callbacks) and Windows/Linux runtime behavior beyond the legs above (the author tested macOS only). Sandboxed verification is already in flight on this head — 中文说明代码审查在读 diff 之前我先写下了自己的方案：主转录操作提交之后，把辅助清理改为由 writer lease 而非 runtime generation 把关——lease 是唯一能在 generation 关闭后存活的所有权凭证。它需要在获取时捕获锁文件身份（以便识别字节一致的替换）、在清理步骤之间低成本地重新断言、并对任何模糊锁状态失败关闭。本 PR 正是这样做的，实现与我独立提出的方案一致。 在当前 head（
三点非阻塞观察：
变更后的流程（见上方时序图）：获取租约并捕获锁的 dev/ino 与字节内容 → 主转录操作（仍由 generation fence 保护）→ 提交后每一步辅助清理前只做锁身份断言（ CI 测试证据无人值守 CI 运行——以下证据是通过 API 读取的 PR 自身 CI 结果，以及仓库的 serve A/B 任务；此处未构建或执行任何 PR 代码。此前变红的 CI 表（上方，自动更新区域）显示当前 head 上所有完成的检查均为绿色。 测试套件在单元测试层面锁定了变更行为——被重命名的测试此前断言 generation 关闭会停止提交后清理，现在断言清理会完成，因此去掉 diff 测试即失败——棘手的租约场景（字节一致替换、符号链接）在真实文件系统上执行。此处未验证：真实 daemon 的交错本身（单元测试用 mock 的 fence 回调建模）以及上述分支之外的 Windows/Linux 运行时行为（作者仅在 macOS 上测试）。 沙箱验证已在当前 head 上运行—— — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 4/5 — a careful, well-tested fix with fail-closed semantics throughout; CI is fully green on the current head, and only non-blocking observations remain. Stepping back: I wrote down my own solution before reading the diff — gate post-commit cleanup on the writer lease, capture the lock's identity so a same-content replacement is detectable, fail closed on ambiguity — and this PR matches that approach, then improves on it: the dev/ino capture is exactly the piece a naive byte comparison misses. I didn't find a simpler path it missed. The defect is real: the code on Every change in the diff earns its place — production edits are exactly the fence plumbing across the two owners, the CI-fix commit is a minimal 8-line test repair, and the rest is tests and a matching doc update. If I had to maintain this in six months I'd thank the author: one well-named primitive, composed where needed, covered by tests that pin both directions of the behavior. CI on the reviewed head is fully green (the previously red ubuntu unit leg fixed test-only, root cause understood), and the serve A/B job found no response drift across 12 scenarios. What keeps this at 4 rather than 5: the three non-blocking observations in the review above (the sync assertion living outside Approving now, pinned to the exact commit reviewed: 中文说明置信度：4/5 —— 谨慎、测试充分、全程保持 fail-closed 的修复；当前 head 上 CI 全绿，只剩非阻塞观察项。 回顾：我在读 diff 之前写下了自己的方案——提交后的清理由 writer lease 把关、捕获锁身份以便识别相同内容的替换、模糊状态一律失败关闭——本 PR 与该方案一致，并且更进一步：dev/ino 捕获恰恰是纯字节比较会遗漏的部分。我没有找到更简的路径被它错过。 缺陷是真实的：当前 diff 中每一处改动都有其价值——生产代码改动恰好是两个所有者的 fence 接线，CI 修复提交是最小的 8 行测试修补，其余是测试和一致的文档更新。六个月后维护这段代码我会感谢作者：一个命名良好的原语、按需组合、测试锁定了行为的双向语义。被审 head 上 CI 全绿（此前变红的 ubuntu 单元测试分支以仅测试方式修复，根因清楚），serve A/B 任务在 12 个场景下未发现任何响应差异。 之所以是 4 而不是 5：上面审查中的三点非阻塞观察（同步断言位于 现在批准，并固定到所审查的确切提交： — Qwen Code · qwen3.8-max Reviewed at |
🩺 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 |
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
CI failure diagnosed and fixed in The failed Ubuntu unit job was branch-related: the new lock-identity verification correctly reads the owned lock through a numeric file descriptor, while the existing Config test globally mocked every synchronous file read to return The fix is test-only. The real-filesystem test now delegates numeric-descriptor reads to the actual Verification on the pushed commit:
No review thread required a reply or resolution (resolved 0/0). The bot's two observations remain non-blocking and were not expanded into this CI-only fix; positive/status comments were ignored. No decision is needed from maintainers at this point; CI is re-running on the new head. |
chiga0
left a comment
chiga0
left a comment
There was a problem hiding this comment.
Deep-tier review of the writer-lease ownership fence. No blocking findings.
Core mechanism — readVerifiedLockIdentity(): Opens the lock file with O_RDONLY | O_NOFOLLOW | O_NONBLOCK, calls fstatSync on the file descriptor, then lstatSync on the path and compares {dev, ino} pairs. This is the standard TOCTOU mitigation: the fd-open prevents symlink following at open time; the subsequent lstat verifies the path still resolves to the same inode after open. Atomic replacement is caught because rename changes the inode under the path. The initial identity is captured once at acquisition time and stored in lockFileIdentity.
assertCleanupOwned() vs assertOwnedAndUnchanged(): Correctly separates two invariants. After the primary transcript move/unlink, the snapshot has changed by design so assertOwnedAndUnchanged would always fail. Switching cleanup steps to assertCleanupOwned lets worktree, PR sidecar, prompt-ledger, file-history, and organization cleanup complete under the owner that began the transaction, fixing the stale-sidecar indefinite leak.
SessionWriterError propagation in catch blocks: Updated catch paths correctly re-throw instanceof SessionWriterError immediately (stops cleanup) while warning on other filesystem errors (cleanup continues). A writer-lease error should abort; a sidecar-specific error should not prevent the next sidecar from proceeding.
Backwards compat: assertCleanupOwned ?? assertCanMutate fallback ensures callers that don't pass the new option are unaffected.
Platform handling: O_NOFOLLOW ?? 0 gracefully degrades on Windows. Symlink and atomic-replacement tests correctly gated runIf(process.platform !== 'win32').
config.test.ts mock fix: The fd-based readFileSync delegation to actualFs is necessary because readVerifiedLockIdentity() calls nodeFs.readFileSync(descriptor, 'utf8') with a numeric fd, which would otherwise hit the blanket mock.
CI: Full green — unit tests, Desktop Shell (ubuntu+windows), web-shell E2E, Serve A/B, Real daemon E2E, and Live Host (macos) all pass.
Reviewed with AI assistance.
|
@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: 105 passed · 0 failed · 105 total Flakiness gate: ❌ 2 of 6 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 检查。 脚本断言：105 通过 · 0 失败 · 105 总计 抖动门：❌ 2 of 6 changed test file(s) returned different results across identical re-runs (5 full round(s)) 确定性抖动门将本 PR 改动的测试文件原样重跑了多轮，得到了不一致的结果（agent 判定： Verification reportPR #10300 Deep Verification —
|
| cell | scenario | base @ HEAD^1 |
head @ HEAD^2 |
|---|---|---|---|
| D-nominal | delete, generation open, lock intact | resolved; residue 0 | resolved; residue 0 (identical) |
| D-genclose | delete, generation closes after unlink | rejected runtime generation closed; 3 sidecars + file-history left |
resolved; residue 0 |
| D-lockswap | delete, lock replaced (new inode) after unlink | resolved; cleanup proceeded under foreign lock | rejected SessionWriterLostError; residue preserved |
| D-lockcontent | delete, lock rewritten in place (foreign owner bytes) | resolved; cleanup proceeded under foreign lock | rejected SessionWriterLostError; residue preserved |
| A-nominal | archive, generation open, lock intact | archived; 3 sidecars moved | identical |
| A-genclose | archive, generation closes after rename | rejected; transcript archived, 3 active sidecars never moved | resolved; 3 sidecars moved to archived |
| A-lockswap | archive, lock replaced after rename | resolved; sidecars moved under foreign lock | rejected SessionWriterLostError; sidecars untouched |
Result: 105/105 scripted assertions pass (driver-ab.mjs, expectation table per arm/cell), identical across two runs. Witness: evidence/01-ab-matrix-base-vs-head.png. All five non-nominal cells flip between arms: two (D/A-genclose) from "residue forever" to "cleanup completes" (the fix), three (D-lockswap, D-lockcontent, A-lockswap) from "mutates sidecars with no ownership proof" to "fail closed" (the safety fence). The two nominal cells are byte-equivalent across arms (no regression). Base-cell failures are the expected control outcomes and count as passing assertions.
Reviewer Test Plan, per step: step 1 (generation closes after primary mutation → everything completes) — measured for delete and archive on head (D/A-genclose); unarchive and the two conflict-repair paths share the identical assertCleanupOwned call sites and were verified by code walk, not separate cells. Step 2 (replace/invalidate lock → cleanup stops with per-session writer error) — measured (D-lockswap, D-lockcontent, A-lockswap: SessionWriterLostError, sidecars preserved). Step 3 (standalone through selected workspace runtime checks both runtime identity and lease) — code walk confirms the composition at all four call sites; end-to-end execution not done (see Not covered), and the mutation matrix shows the runtime half of that composition is unpinned (finding below). Step 4 (ordinary behavior unchanged) — D/A-nominal byte-equivalent outcomes on both arms plus the full changed-file gates below.
Findings
S1 (Suggestion, completeness): the runtime-identity half of the standalone composed cleanup check is asserted by no test
The standalone service composes assertCleanupOwned: () => { assertRuntimeCurrent(runtime); lease.assertCleanupOwned(); } at four sites. Mutation VM4b deleted the assertRuntimeCurrent line from one of them and the whole standalone suite stayed green (81/81), while the sibling mutation VM4a (delete the lease.assertCleanupOwned() line) killed exactly one test. So the lease half is pinned; the fail-closed routing half that the PR description's step 3 rests on is not. The runtime behavior itself is correct in code and assertRuntimeCurrent is pre-existing, tested machinery — this is a completeness gap, not a defect: the fixture that would pin it is one that calls the composed callback and asserts the runtime assertion fired. (Mutation-matrix convention: completeness reporting, not a merge condition.)
No higher-severity findings. Specifically checked and not found: callers of archiveSessions/unarchiveSessions/removeSessions that lost a fence (the archive/unarchive cleanup sites call options.assertCleanupOwned?.() without an assertCanMutate fallback — every production caller is routed through session-archive.ts / standalone-session-service.ts, which both pass assertCleanupOwned; the remove path keeps the ?? assertCanMutate fallback for plain callers, which is behavior-identical to base for them); divergent issue/accept ends (the fence is checked where the cleanup executes); lock-tamper shapes beyond swap/rewrite (symlink and byte-identical-rename shapes are pinned by the new lease tests, confirmed non-vacuous via VM1/VM5).
Mutation matrix (vacuity of the new tests)
Unmutated controls green first (core 385 passed | 2 skipped; cli 146 passed). Witness: evidence/02-mutation-matrix.png.
| mutation | suite | result | classification |
|---|---|---|---|
VM1: assertCleanupOwned() → no-op |
lease suite | killed 3 (expected function to throw an error, but it didn't) |
pinned |
VM2: all options.assertCleanupOwned → options.assertCanMutate |
sessionService + corruption suites | killed 14 (promise rejected "Error: generation changed" instead of resolving) |
pinned |
VM3 (control): delete the pre-existing pre-mutation assertCanMutate |
sessionService suite | killed 2 (expected "spy" to not be called at all, but actually been called 1 times) |
positive control — the existing guard test plus one whose fence-call sequence shifted; same file, proves the harness turns tests red |
| VM4a: composed standalone callback, drop lease half | standalone suite | killed 1 (expected "spy" to be called once, but got 0 times) |
pinned |
| VM4b: composed standalone callback, drop runtime half | standalone suite | survived (81/81) | coverage gap (S1 above) |
VM5: readOwnedLock minus the new dev/ino clause |
lease suite | killed 2 (promise resolved "SessionWriterLease{…}" instead of rejecting) |
pinned |
Layered-guard check: VM1 and VM5 each kill tests the other cannot (cleanup-phase assertion vs acquisition/release-phase identity clause) — the two identity checks defend different phases; neither is redundant, and no survivor was misread as vacuity of a guarded hunk. All quoted failures are expected-vs-actual behavioral assertions, not import/compile breakage.
Targeted gates
| gate | result |
|---|---|
vitest run core: sessionService.test.ts, sessionService.corruption.test.ts, session-writer-lease.test.ts |
385 passed | 2 skipped (387) — 3 files green |
vitest run cli: standalone-session-service.test.ts, session-archive.test.ts |
146 passed (146) — 2 files green, matching the PR body's CLI count |
Not covered
- Per-commit attribution: the checkout is depth 2; only the second commit (
429da3ca, "codex: fix CI failure on PR fix(core): Preserve ownership during session cleanup #10300") is reachable — the first (3171d115, the fix itself) sits behind the shallow boundary (git rev-list HEAD^1..HEAD^2returns 1 vs 2 in the metadata snapshot). The aggregateHEAD^1..HEADdiff is what was verified; which test adjustments belonged to which commit could not be split. - Trial merge: the workflow's merge commit already places the PR on the current base tip (
40c77fce, ahead of the metadatabaseRefOida3ec41a2), conflict-free — so the A/B verifies what actually lands. No separate re-merge was needed;main's delta between the two bases was not itself re-examined. - Standalone-session lifecycle was not executed end to end (step 3): verified by code walk + mutation characterization only.
- Unarchive-genclose and the two conflict-repair-genclose cells were not run as separate A/B cells; their cleanup paths call the same
assertCleanupOwnedsites measured for delete/archive. - Windows behavior:
readVerifiedLockIdentityrequires a verifiable inode (hasVerifiableInode= ino ≠ 0) for the lock file; this run proves the Linux path end to end. The PR marks Windows/Linux as locally untested (⚠️ ); on a filesystem returning inode 0 the lease now fails closed at acquisition — consistent with the PR's stated fail-closed intent, but not exercised here. - Repo-wide test/lint gates were not run (scope: changed files only).
- Replay calibration: N/A — not a workflow/CI PR.
Methodology
Environment: CI verify container (node:22-bookworm), working tree at refs/pull/10300/merge (depth 2), npm ci + npm run build pre-run at HEAD. Base control: git worktree at HEAD^1 with only packages/core rebuilt (scripts/build_package.js, root node_modules reused — PR changes no package.json/lockfile, so the control is dependency-clean; core's nested node_modules symlinked since it is dependency-identical). Harnesses import each arm's compiled dist/ by absolute file URL, so no workspace symlink can cross-contaminate arms; a symbol control confirmed the base dist contains zero assertCleanupOwned/lockFileIdentity occurrences while head contains them. One pre-existing oddity: worktree builds report a single TS7016 (@lydell/node-pty declaration resolution, shellExecutionService.ts — untouched by this PR) yet emit complete JS; it reproduces identically when building base sources, so it is an artifact of building a worktree against the root install, not a PR effect (A/A-equivalent observation). Mutation runs used a scratch worktree at HEAD, rebuilding the mutated package before each run so both relative and package-entry imports saw the mutant; every mutant was restored afterward (tree verified clean). Raw per-cell logs (logs/base-*.log, logs/head-*.log), the driver run (logs/driver-ab-run.txt), mutation logs (logs/vm*.log), build logs, and both harness scripts live in this artifact directory alongside evidence/01-ab-matrix-base-vs-head.png and evidence/02-mutation-matrix.png.
Flakiness gate log
rounds=5 files=6 skipped=0
file packages/cli/src/serve/conversations/standalone-session-service.test.ts: (cd packages/cli) npx --no-install vitest run ./src/serve/conversations/standalone-session-service.test.ts
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/config/config.test.ts: (cd packages/core) npx --no-install vitest run ./src/config/config.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/conversations/standalone-session-service.test.ts: PPPPP
packages/cli/src/serve/server/session-archive.test.ts: FPFFF
packages/core/src/config/config.test.ts: PPPPP
packages/core/src/services/session-writer-lease.test.ts: FFFFF
packages/core/src/services/sessionService.corruption.test.ts: FPFFF
packages/core/src/services/sessionService.test.ts: PPPPP
verdict: flaky
summary: 2 of 6 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/conversations/standalone-session-service.test.ts: P (exit 0)
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 ---
n one batch to a single archive�[32m 8�[2mms�[22m�[39m
�[32m✓�[39m archiveDaemonSessions�[2m > �[22mdisables a scheduled task bound to the archived session�[32m 16�[2mms�[22m�[39m
�[32m✓�[39m archiveDaemonSessions�[2m > �[22mreports task maintenance failure after archiving the transcript�[32m 15�[2mms�[22m�[39m
�[32m✓�[39m archiveDaemonSessions�[2m > �[22mdoes not acquire writer leases for ids already archived or missing�[32m 3�[2mms�[22m�[39m
�[32m✓�[39m archiveDaemonSessions�[2m > �[22mdoes not archive while another writer holds the lease�[32m 27�[2mms�[22m�[39m
�[32m✓�[39m archiveDaemonSessions�[2m > �[22mtakes over a sealed empty transcript before maintenance�[32m 30�[2mms�[22m�[39m
�[32m✓�[39m archiveDaemonSessions�[2m > �[22mrejects a transcript FIFO without waiting for a writer�[32m 7�[2mms�[22m�[39m
�[32m✓�[39m archiveDaemonSessions�[2m > �[22mkeeps independent batch sessions moving when one writer conflicts�[32m 18�[2mms�[22m�[39m
�[32m✓�[39m archiveDaemonSessions�[2m > �[22mreports a gate race per session after another batch item was archived�[32m 9�[2mms�[22m�[39m
�[32m✓�[39m archiveDaemonSessions�[2m > �[22mkeeps independent batch sessions moving when one classification fails�[32m 14�[2mms�[22m�[39m
�[32m✓�[39m archiveDaemonSessions�[2m > �[22mdoes not acquire a lease or mutate when closing the owner fails�[32m 2�[2mms�[22m�[39m
�[32m✓�[39m archiveDaemonSessions�[2m > �[22muses the classification made after acquiring the lease�[32m 12�[2mms�[22m�[39m
�[32m✓�[39m archiveDaemonSessions�[2m > �[22mdoes not lock an active/archive conflict�[32m 3�[2mms�[22m�[39m
�[32m✓�[39m archiveDaemonSessions�[2m > �[22mrepairs an active/archive conflict by keeping the archived copy�[32m 17�[2mms�[22m�[39m
�[32m✓�[39m archiveDaemonSessions�[2m > �[22mdoes not report success after release fails but reconciles the task to the applied archive�[32m 8�[2mms�[22m�[39m
�[32m✓�[39m archiveDaemonSessions�[2m > �[22mreleases the lease when scheduled-task reconciliation fails�[32m 19�[2mms�[22m�[39m
�[32m✓�[39m archiveDaemonSessions�[2m > �[22mchecks only the selected runtime root for transcripts and locks�[32m 21�[2mms�[22m�[39m
�[32m✓�[39m archiveDaemonSessions�[2m > �[22mrejects with DaemonDrainingError after the coordinator is sealed�[32m 2�[2mms�[22m�[39m
�[32m✓�[39m archiveDaemonSessions�[2m > �[22mrecovers an enabled task whose session is already archived�[32m 8�[2mms�[22m�[39m
�[32m✓�[39m unarchiveDaemonSessions�[2m > �[22mdeduplicates ids and does not lock already active or missing ids�[32m 17�[2mms�[22m�[39m
�[32m✓�[39m unarchiveDaemonSessions�[2m > �[22mcollapses case-variant spellings in one batch to a single unarchive�[32m 11�[2mms�[22m�[39m
�[31m �[31m�[31m unarchiveDaemonSessions�[2m > �[22mrepairs an active/archive conflict by keeping the active copy�[39m�[32m 19�[2mms�[22m�[39m
�[31m → expected { unarchived: [], …(4) } to match object { …(3) }
(2 matching properties omitted from actual)�[39m
�[32m✓�[39m unarchiveDaemonSessions�[2m > �[22mdoes not unarchive while another writer holds the lease�[32m 14�[2mms�[22m�[39m
�[32m✓�[39m unarchiveDaemonSessions�[2m > �[22mreports a single error per archived id when unarchive batch fails�[32m 12�[2mms�[22m�[39m
�[32m✓�[39m unarchiveDaemonSessions�[2m > �[22mkeeps independent unarchive sessions moving when one classification fails�[32m 11�[2mms�[22m�[39m
�[32m✓�[39m unarchiveDaemonSessions�[2m > �[22mreports a gate race per session after another batch item was unarchived�[32m 13�[2mms�[22m�[39m
�[32m✓�[39m unarchiveDaemonSessions�[2m > �[22mre-enables an archive-disabled task bound to the unarchived session�[32m 187�[2mms�[22m�[39m
�[32m✓�[39m unarchiveDaemonSessions�[2m > �[22mrecovers a stranded task on an ALREADY-active session�[32m 15�[2mms�[22m�[39m
�[32m✓�[39m unarchiveDaemonSessions�[2m > �[22mrejects with DaemonDrainingError after the coordinator is sealed�[32m 1�[2mms�[22m�[39m
�[32m✓�[39m deleteDaemonSessions�[2m > �[22mdeletes both copies of an exact active/archive conflict�[32m 22�[2mms�[22m�[39m
�[32m✓�[39m deleteDaemonSessions�[2m > �[22mremoves a scheduled task bound to the deleted session�[32m 18�[2mms�[22m�[39m
�[32m✓�[39m deleteDaemonSessions�[2m > �[22mrepairs task maintenance on retry after deleting the transcript�[32m 17�[2mms�[22m�[39m
�[32m✓�[39m deleteDaemonSessions�[2m > �[22mcollapses case-variant spellings in one batch to a single delete�[32m 12�[2mms�[22m�[39m
�[32m✓�[39m deleteDaemonSessions�[2m > �[22mdoes not delete while another writer holds the lease�[32m 8�[2mms�[22m�[39m
�[32m✓�[39m deleteDaemonSessions�[2m > �[22mreports attachment cleanup failures and allows an idempotent retry�[32m 9�[2mms�[22m�[39m
�[32m✓�[39m deleteDaemonSessions�[2m > �[22mreports a gate race per session after another batch item was deleted�[32m 8�[2mms�[22m�[39m
�[32m✓�[39m deleteDaemonSessions�[2m > �[22mskips orphan deletion when a new owner attached�[32m 2�[2mms�[22m�[39m
�[32m✓�[39m deleteDaemonSessions�[2m > �[22mrejects with DaemonDrainingError after the coordinator is sealed�[32m 1�[2mms�[22m�[39m
�[32m✓�[39m deleteDaemonSessions�[2m > �[22mdeletes the transcript when killSession resolves true�[32m 8�[2mms�[22m�[39m
�[32m✓�[39m deleteDaemonSessions�[2m > �[22mreturns true when task maintenance fails after orphan deletion�[32m 15�[2mms�[22m�[39m
�[32m✓�[39m deleteDaemonSessions�[2m > �[22mdeletes the transcript when killSession throws SessionNotFoundError�[32m 14�[2mms�[22m�[39m
�[32m✓�[39m deleteDaemonSessions�[2m > �[22mthrows when the lease is held by another writer�[32m 9�[2mms�[22m�[39m
�[31m⎯⎯⎯⎯⎯⎯⎯�[39m�[1m�[41m Failed Tests 1 �[49m�[22m�[31m⎯⎯⎯⎯⎯⎯⎯�[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⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯[1/1]⎯�[22m�[39m
�[2m Test Files �[22m �[1m�[31m1 failed�[39m�[22m�[90m (1)�[39m
�[2m Tests �[22m �[1m�[31m1 failed�[39m�[22m�[2m | �[22m�[1m�[32m64 passed�[39m�[22m�[90m (65)�[39m
�[2m Start at �[22m 16:37:29
�[2m Duration �[22m 19.38s�[2m (transform 5.58s, setup 141ms, collect 8.43s, tests 767ms, environment 417ms, prepare 138ms)�[22m
JUNIT report written to /__w/qwen-code/qwen-code/packages/cli/junit.xml
round 1 · packages/core/src/conf
...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
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. ✅
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
中文说明
已审查——无阻断问题。 建议见行内评论。
— qwen3.8-max via Qwen Code /review (v0.22.2)
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
e773f6c
Review follow-up for
|
| Item | Decision | Action |
|---|---|---|
| R1-8 terminal lost lease remains pending forever | Agree — correctness regression | Fixed. SessionWriterLostError now evicts only the matching pending lease and still returns failure for the current cleanup; the next recovery performs a fresh authoritative acquisition. |
| R1-1 extract ownership assertion closure | Defer | Non-blocking maintainability extraction; the explicit scoped call sites are correct and an abstraction would widen this critical-fix round. |
| R1-5 add daemon forwarding tests | Defer | Test hardening without a demonstrated exact-head defect; current forwarding is verified and this round is limited to the reproduced regression. |
| R1-6 add ownership-loss diagnostics | Defer | Useful observability hardening, but not a correctness fix; it also expands identity logging beyond this round. |
| R1-3 unify partial-options fallback policy | Defer | No production caller uses the partial-options shape; changing the policy is a separate API decision. |
| R1-4 rename the sidecar callback parameter | Defer | Non-behavioral naming cleanup. |
| R1-2 centralize catch guards | Defer | Maintainability refactor; the explicit guards are currently correct. |
Verification: npm run build, npm run typecheck, npm run lint, changed-file Prettier check, and standalone-session-service.test.ts (82/82) all pass. An independent reproduction also confirmed the stale pending entry is gone while retryable durability-pending release behavior remains unchanged.
Resolved review threads: 7/7 after the individual replies. Decisions needed: none.
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) this review confirmed are already reported on this PR and are not repeated:
- R1-1 four-way duplicated assertCleanupOwned closure in standalone-session-service.ts — already reported in round 1 (inline comment 3874017847), author explicitly deferred; re-derived this round on unchanged code and not re-posted
Not explored to full depth (tool budget reached): "agent 1a": none — but note I could not run npm run typecheck /lint on the changed files (install's prepare fails only on a missing husky binary; I built core and ran …; "agent 6b": executing packages/cli/src/serve/conversations/standalone-session-service.test.ts to confirm the new tests pass (worktree has no node_modules or built core d….
中文说明
已审查。 建议见行内评论。
本轮确认的 1 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未探索到全部深度（达到工具调用预算）："agent 1a"：none — but note I could not run npm run typecheck /lint on the changed files (install's prepare fails only on a missing husky binary; I built core and ran …；"agent 6b"：executing packages/cli/src/serve/conversations/standalone-session-service.test.ts to confirm the new tests pass (worktree has no node_modules or built core d…。
— qwen3.8-max via Qwen Code /review (v0.22.2)
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
Addressed the current review feedback in commit 79f5c29:
Verification: focused CLI tests 85/85, focused core tests 97 passed with 3 platform skips, independent focused verification 8/8, repository build, typecheck, lint, scoped Prettier check, and clean diff audits. Resolved 4/4 handled review threads. |
chiga0
left a comment
chiga0
left a comment
There was a problem hiding this comment.
Tier: Deep（并发安全 + 文件身份校验）— 增量 429da3ca → 79f5c29f
已核验内容
R2-1（TOCTOU：path identity check 与 content read 之间的替换窗口）
readVerifiedLockIdentity 现在对 assertPathMatchesDescriptor() 做了两次调用：
- 在
readFileSync(descriptor, 'utf8')之前：确认 path 指向已打开的 fd（inode 匹配） - 在 content 验证（
parseLockRecord+owner_id/lockRecordRaw校验）之后：再次确认 path 仍指向同一 inode
第二次检查关闭了这个窗口：攻击者在 fstat+内容读取之间原子替换 lockPath 后，重新 lstat 会发现 inode 不匹配并抛 SessionWriterLostError。配套测试通过 descriptorReadHook.afterRead 在 readFileSync 返回后同步执行 renameSync 来重现此时序，并验证 assertCleanupOwned() 会抛出。✓
R2-2（清理失败后 catalog/cache 未更新）
archive 和 unarchive 的错误路径现在都调用 reconcileCatalogAfterLifecycleError（即 markSessionCatalogChanged + invalidateSessionListCache）。两个新测试分别验证：move 完成但 cleanup 失败时，catalog 和 cache 均被 invalidate。✓
CI
1 success，1 in_progress（review-pr），28 skipped。核心测试无失败。
无阻塞项。
E2E verification report — head
|
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
1 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- test-matrix daemon-layer assertCleanupOwned-forwarding finding (session-archive.ts removeSession/archiveSessions/unarchiveSessions option wiring) — already reported as R1-5 (comment 3874017852), author explicitly deferred
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Deferred under the convergence posture (round 3, not a blocker) — recorded, not requested in this round:
packages/core/src/services/sessionService.ts:2608 — [probe] new cleanup fence in cleanupRemovedSessionStateForLifecycle is pinned by no test (deferred — code-age rule: anchored on code unchanged since the previous round)
Convergence: round 3 posted 2 inline comment(s), 2 of them reported for the first time; the previous round posted 2 (2 new). Findings keep coming back to the same files: packages/cli/src/serve/conversations/standalone-session-service.ts (findings in rounds 1, 2; 1 more now). The rate of new findings is not falling. (Evidence: the previous round was recovered from a marker this account did not post and merged over this account's own entries, so some of those rounds and its counts may not be this account's own.) A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. Batching the remaining fixes and verifying them before the next push, or dropping this PR's reviews to --severity-floor critical, keeps the loop from re-deriving the same set. (Observation only — nothing was withheld from this review because of this observation.)
中文说明
仅完成部分审查，审查缺口已披露。
本轮确认的 1 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
收敛姿态下延后（第 3 轮，非阻断）——已记录，本轮不要求修改：共 1 条（原文未翻译，列表见上方英文部分）。
收敛情况：第 3 轮发布了 2 条行内评论，其中 2 条是首次提出；上一轮发布了 2 条（其中 2 条首次提出）。发现反复回到同一批文件：packages/cli/src/serve/conversations/standalone-session-service.ts（第 1、2 轮已出过发现，本轮又有 1 条）。新发现的产出速度没有下降。（证据说明：上一轮的数据来自并非本账号发布的标记，并与本账号自己的条目合并，上述轮次与其计数中的部分可能不属于本账号。）一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。把剩余修复攒成一批、验证后再推送，或将本 PR 的评审降到 --severity-floor critical，可以避免循环反复推导同一组发现。（仅为观察——本轮评审未因此扣留任何内容。）
— qwen3.8-max via Qwen Code /review (v0.22.2)
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
Addressed the new Critical review feedback in
Verification: full build, typecheck, lint, targeted Prettier check, and all 87 standalone-session-service tests passed. Independent frozen-head probes reproduced both findings and confirmed the fixed deletion call ordering. |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline.
1 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- R4-2 four-way duplicated assertCleanupOwned fence closure plus duplicated cleanupOwnershipLost catch classification in standalone-session-service.ts — already reported as R1-1 (comment 3874017847); author explicitly deferred
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Convergence: round 4 posted 1 inline comment(s), 1 of them reported for the first time; the previous round posted 2 (2 new). Findings keep coming back to the same files: packages/cli/src/serve/conversations/standalone-session-service.ts (findings in round 3; 1 more now). A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. No Critical finding is open on this round, so merging and moving the remaining Suggestion threads to a follow-up issue is available as an ending — a merged pull request cannot diverge further. (Observation only — nothing was withheld from this review because of this observation.)
中文说明
仅完成部分审查，审查缺口已披露。 建议见行内评论。
本轮确认的 1 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
收敛情况：第 4 轮发布了 1 条行内评论，其中 1 条是首次提出；上一轮发布了 2 条（其中 2 条首次提出）。发现反复回到同一批文件：packages/cli/src/serve/conversations/standalone-session-service.ts（第 3 轮已出过发现，本轮又有 1 条）。一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。本轮没有未决的 Critical，因此"合入后把剩余 Suggestion 线程转到后续 issue"是一个可选的结束方式——已合入的 PR 不会继续发散。（仅为观察——本轮评审未因此扣留任何内容。）
— qwen3.8-max via Qwen Code /review (v0.22.2)
| if ( | ||
| durable.kind !== 'standalone' || | ||
| durable.location !== expectedLocation | ||
| ) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
[Suggestion] R4-1: The early-return guard in the new reconcileCatalogAfterLifecycleError — invalidate the catalog and session-list cache only when the session actually moved to the expected location — has no test pinning its negative case. The two "invalidates the catalog…" tests cover only the positive path, and the existing "does not misreport an unexpected batch failure as a session conflict" test already exercises this guard branch (the archive fails while the durable location is still active, not the expected archived) but asserts nothing about catalog invalidation. As it stands, deleting the guard is a mutation no assertion catches: both invalidation tests would still observe markSessionCatalogChanged called once through the same reconcile call, so a future refactor could ship unconditional invalidation — every failed archive/unarchive where nothing moved (including standalone_session_not_found and plain storage failures) would call markSessionCatalogChanged() and drop the session-list cache, churning catalog-change markers and forcing needless client re-lists, with the suite green.
Verified by a mutation probe in a scratch tree:
control (unmodified): 87/87 pass
mutant (guard deleted): 87/87 pass — the mutation survives
mutant + negative assertion: fails — expected "spy" to not be called at all, but actually been called 1 times
guard restored + assertion: passes
Suggested fix: add the negative assertions where the error path is already exercised — in "does not misreport an unexpected batch failure as a session conflict":
expect(harness.bridge.markSessionCatalogChanged).not.toHaveBeenCalled();
expect(harness.invalidateSessionListCache).not.toHaveBeenCalled();(an unarchive-side variant would also cover the 'active' expectation). Fix witness: those assertions must go red when the guard is removed — removing the durable.location !== expectedLocation early return and running that test must fail.
中文说明
新的 reconcileCatalogAfterLifecycleError 中的提前返回守卫（仅当会话确实移动到预期位置时才失效目录和会话列表缓存）没有针对其反向场景的测试固定。两个 "invalidates the catalog…" 测试只覆盖了正向路径；已有的 "does not misreport an unexpected batch failure as a session conflict" 测试实际已经进入了该守卫分支（归档失败时持久化位置仍为 active，不等于预期的 archived），但没有对目录失效做任何断言。目前删除该守卫是一个没有任何断言能捕获的突变：两个失效测试仍会观察到 markSessionCatalogChanged 经由同一次 reconcile 调用被执行一次，因此未来的重构可能会带上无条件的失效逻辑——所有未发生移动的失败归档/取消归档（包括 standalone_session_not_found 和普通存储失败）都会调用 markSessionCatalogChanged() 并清空会话列表缓存，导致目录变更标记被反复触发、客户端被迫无谓地重新拉取列表，而测试套件仍然全绿。
已在隔离 scratch tree 中通过突变探针验证：对照组（未修改）87/87 通过；突变体（删除守卫）87/87 通过——突变存活；突变体加上负向断言后失败（expected "spy" to not be called at all, but actually been called 1 times）；恢复守卫后同一断言通过。
建议修复：在已经覆盖该错误路径的测试中补充负向断言——在 "does not misreport an unexpected batch failure as a session conflict" 中加入 expect(harness.bridge.markSessionCatalogChanged).not.toHaveBeenCalled(); 和 expect(harness.invalidateSessionListCache).not.toHaveBeenCalled();（取消归档侧的变体可同时覆盖 'active' 预期）。修复验证：移除该守卫后这些断言必须变红——删除 durable.location !== expectedLocation 提前返回并运行该测试必须失败。
— qwen3.8-max via Qwen Code /review (v0.22.2)
There was a problem hiding this comment.
Thanks — deferring. This is a valid Suggestion-level negative-assertion gap, not a current correctness defect. After four review rounds, we are limiting this PR to Critical correctness fixes and keeping the verified guard unchanged.
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Main synchronization updateMerged the latest Verification on the exact pushed commit:
Review disposition: no new Critical issue was discovered or fixed during this synchronization. The remaining unresolved R4-1 item already has an operator reply and remains deferred because it is a Suggestion-level negative-test gap rather than an exact-head correctness defect. No maintainer decision is needed. GitHub checks have restarted for the new head. |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
3 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- R1-3 archive/unarchive assertCleanupOwned fallback asymmetry — already reported (comment 3874017866), author explicitly deferred
- R1-5 daemon-layer assertCleanupOwned forwarding unpinned — already reported (comment 3874017852), author explicitly deferred
- R4-1 reconcileCatalogAfterLifecycleError negative-branch assertion gap — already reported (comment 3878373091), author explicitly deferred this round
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 5.
Deferred under the convergence posture (round 5, not a blocker) — recorded, not requested in this round:
packages/core/src/services/sessionService.ts:1554 (+5 locations) — [probe] six of seven new SessionWriterError rethrow guards unpinned by tests (code-age rule: unchanged since the previous round)packages/core/src/services/session-writer-lease.ts:2103 — [probe] zero-inode lock-identity gate unreachable by fault-injection tests (code-age rule: unchanged since the previous round)
中文说明
仅完成部分审查，审查缺口已披露。
本轮确认的 3 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未审查：反向审计——在 5 轮的反审轮数上限内未收敛。
收敛姿态下延后（第 5 轮，非阻断）——已记录，本轮不要求修改：共 2 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.22.2)
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
Independent pass at 86f45c7. One P2 inline: the archive/unarchive fences were swapped to assertCleanupOwned without the assertCanMutate fallback the cleanup paths keep, and the production callers only pass assertCanMutate — so those fences currently never fire on the runtime-generation check. The lease/identity direction otherwise looks right (readVerifiedLockIdentity at acquire time narrows the check-then-act window); two residuals worth a look noted below. Not re-reporting the open R4-1 thread.
| this.removeFileIfExists(active.filePath); | ||
| try { | ||
| options.assertCanMutate?.(); | ||
| options.assertCleanupOwned?.(); |
There was a problem hiding this comment.
[P2] The mid-mutation fences in archiveSessions/unarchiveSessions (here and ~2831/2836/2840/2876/2894/2900/2906, plus the unarchive twins) were changed from options.assertCanMutate?.() to bare options.assertCleanupOwned?.() — unlike the cleanup paths in this same PR, which use (options.assertCleanupOwned ?? options.assertCanMutate)?.() (2608, 2751-2752, 2765-2766). The public options still advertise assertCanMutate, and routes/session.ts passes ONLY assertCanMutate to its archive/unarchive wrappers (~2500-2553, endpoints 5569/5597/5658/5690; acp-http/dispatch.ts too), so on those production paths the fences are silent no-ops: a runtime going stale mid-archive is no longer aborted between the session-file / pr-sidecar / prompt-ledger moves. Suggestion: resolve once at the top — const assertCleanupOwned = options.assertCleanupOwned ?? options.assertCanMutate; — matching the cleanup-path pattern, and add a regression test that passes only assertCanMutate.
There was a problem hiding this comment.
Not taking — the route and ACP callers pass assertCanMutate into archiveDaemonSessions/unarchiveDaemonSessions, which acquire the writer lease and then pass both assertCanMutate and the lease-backed assertCleanupOwned into SessionService (session-archive.ts:923-927 and 1121-1125). The standalone callers also pass both. Thus these post-commit fences are active for every production caller; adding the fallback would only harden the optional direct-call API shape already deferred as Suggestion R1-3, not fix a current production regression.
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not complete locally.
Not reviewed: build-and-test — required Node 22 configuration was not verified because the local run used Node 24 and macOS/Windows Node 22 checks were skipped.
Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 5.
Deferred under the convergence posture (round 6, not a blocker) — recorded, not requested in this round:
docs/developers/qwen-serve-protocol.md:2503 — [review] D6-1 conflict-repair partial-success semantics are not pinned by tests
Convergence: round 6 posted 3 inline comment(s), 3 of them reported for the first time. Findings keep coming back to the same files: packages/core/src/services/session-writer-lease.ts (findings in round 2; 1 more now). (Evidence: the previous round was recovered from a marker this account did not post and merged over this account's own entries, so some of those rounds may not be this account's own.) A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. (Observation only — nothing was withheld from this review because of this observation.)
[Critical] R6-1 [certifies-falsely] [new-surface] packages/cli/src/serve/conversations/standalone-session-service.ts:2023 — Runtime ownership loss still permits unfenced staged-directory cleanup. If assertRuntimeCurrent(runtime) throws ConversationRuntimeOwnershipError, cleanupOwnershipLost remains false; attachment cleanup fails and is swallowed, then fresh deletion can call removeStagedStandaloneDirectory without either ownership fence. Witness: not run — the code trace shows this error fails the instanceof SessionWriterError classification and enters the destructive branch. Fix constraint: conversation-runtime-manager.ts:61-66 throws ConversationRuntimeOwnershipError, not SessionWriterError. Add fresh-deletion and recovery tests that lose runtime ownership during cleanup and require every destructive cleanup to be skipped.
[Critical] R6-2 [certifies-falsely] [new-surface] packages/core/src/services/session-writer-lease.ts:2051 — Async lock ownership checks accept a byte-identical replacement. readOwnedLock() checks pathname identity before a pathname-based fs.readFile() but never re-checks it afterward, so an atomic byte-identical replacement can pass owner/raw checks and be falsely certified for append, sealing, or release. Witness: not run — the unchecked interval is directly present and the adjacent descriptor implementation demonstrates the missing post-read identity check. Add a coordinated replacement-during-read regression; release must not remove the replacement.
[Critical] R6-4 [certifies-falsely] [new-surface] packages/core/src/services/sessionService.ts:2805 — Retries after post-commit ownership loss leave source-state sidecars stranded. After the transcript move commits and ownership loss aborts sidecar cleanup, a retry returns alreadyArchived/alreadyActive before moving PR, worktree, or ledger sidecars. Probe: archive retry kept {source:true,target:false}; unarchive retry did the same, while temporary retry reconciliation flipped both to {source:false,target:true}. Add archive/unarchive regressions that retry under fresh ownership and require every stranded sidecar to reconcile before returning already-target.
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not complete locally。
未审查：build-and-test — required Node 22 configuration was not verified because the local run used Node 24 and macOS/Windows Node 22 checks were skipped。
未审查：反向审计——在 5 轮的反审轮数上限内未收敛。
收敛姿态下延后（第 6 轮，非阻断）——已记录，本轮不要求修改：共 1 条（原文未翻译，列表见上方英文部分）。
收敛情况：第 6 轮发布了 3 条行内评论，其中 3 条是首次提出。发现反复回到同一批文件：packages/core/src/services/session-writer-lease.ts（第 2 轮已出过发现，本轮又有 1 条）。（证据说明：上一轮的数据来自并非本账号发布的标记，并与本账号自己的条目合并，上述轮次中的部分可能不属于本账号。）一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。（仅为观察——本轮评审未因此扣留任何内容。）
[Critical] R6-1 [certifies-falsely] [new-surface] packages/cli/src/serve/conversations/standalone-session-service.ts:2023 — Runtime ownership loss still permits unfenced staged-directory cleanup. If assertRuntimeCurrent(runtime) throws ConversationRuntimeOwnershipError, cleanupOwnershipLost remains false; attachment cleanup fails and is swallowed, then fresh deletion can call removeStagedStandaloneDirectory without either ownership fence. Witness: not run — the code trace shows this error fails the instanceof SessionWriterError classification and enters the destructive branch. Fix constraint: conversation-runtime-manager.ts:61-66 throws ConversationRuntimeOwnershipError, not SessionWriterError. Add fresh-deletion and recovery tests that lose runtime ownership during cleanup and require every destructive cleanup to be skipped.
[Critical] R6-2 [certifies-falsely] [new-surface] packages/core/src/services/session-writer-lease.ts:2051 — Async lock ownership checks accept a byte-identical replacement. readOwnedLock() checks pathname identity before a pathname-based fs.readFile() but never re-checks it afterward, so an atomic byte-identical replacement can pass owner/raw checks and be falsely certified for append, sealing, or release. Witness: not run — the unchecked interval is directly present and the adjacent descriptor implementation demonstrates the missing post-read identity check. Add a coordinated replacement-during-read regression; release must not remove the replacement.
[Critical] R6-4 [certifies-falsely] [new-surface] packages/core/src/services/sessionService.ts:2805 — Retries after post-commit ownership loss leave source-state sidecars stranded. After the transcript move commits and ownership loss aborts sidecar cleanup, a retry returns alreadyArchived/alreadyActive before moving PR, worktree, or ledger sidecars. Probe: archive retry kept {source:true,target:false}; unarchive retry did the same, while temporary retry reconciliation flipped both to {source:false,target:true}. Add archive/unarchive regressions that retry under fresh ownership and require every stranded sidecar to reconcile before returning already-target.
— gpt-5.6-sol@954e5164 via Qwen Code /review (v0.22.2)
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
Round 6 follow-up is complete on head 4a42ea1. Discovered issues and actions:
Deferred/ignored:
Verification:
Resolved review threads: 3. Decisions needed: none. |
6 participants
What this PR does
This PR preserves exact daemon writer-lease ownership across the post-transcript cleanup phase of delete, archive, unarchive, and conflict repair. The primary transcript mutation remains guarded by the existing transcript snapshot and runtime-generation checks; once that mutation commits, auxiliary worktree, PR, prompt-ledger, file-history, and organization cleanup may continue only while the same active writer lock is still present and byte-for-byte owned by the lease holder.
The same ownership fence is composed with the selected-runtime check for standalone-session archive, unarchive, deletion, and deletion-journal recovery, so workspace routing remains fail closed. Missing, replaced, malformed, non-regular, or symlinked locks stop auxiliary cleanup. The protocol documentation also clarifies that conflict repair is limited to regular workspace-maintainable transcript copies and never bypasses ownership checks.
This replaces #10286 with a version based on the lifecycle implementation merged in #10179.
Why it's needed
On current
main, a daemon runtime generation may close immediately after the transcript has already been unlinked, moved, or chosen as the losing conflict copy. The old generation fence then rejects auxiliary cleanup even though the daemon still owns the exact writer lease. Retrying the same lifecycle request sees the primary operation as already complete and does not revisit those sidecars, leaving stale worktree, PR, prompt-ledger, file-history, or organization state indefinitely.Using the writer lease as the post-commit ownership boundary lets the transaction finish under the owner that began it without allowing a stale or foreign runtime to mutate another owner's state.
Reviewer Test Plan
How to verify
Local verification passed 384 core assertions (3 skipped), 146 CLI assertions, the repository build, typecheck, lint, and exact changed-file formatting checks. An isolated real-filesystem baseline on current
mainreproduced permanent auxiliary residue in all five ordinary daemon lifecycle scenarios; the same matrix is covered by the post-fix verification.Evidence (Before & After)
N/A — non-UI lifecycle correctness change.
Tested on
Environment (optional)
macOS 26.4.1, Node.js 22.22.3, npm 10.9.8, sandbox disabled.
Risk & Scope
Linked Issues
Supersedes #10286. Builds on #10179.
中文说明
本 PR 做了什么
本 PR 在删除、归档、取消归档和冲突修复的转录主操作之后，继续保留精确的 daemon writer lease 所有权校验。转录主操作仍由已有的转录快照与 runtime generation 校验保护；一旦主操作提交，只有在同一个活跃 writer lock 仍存在且其原始内容与 lease 持有者完全一致时，才允许继续清理辅助 worktree、PR、prompt ledger、file history 和 organization 状态。
对于 standalone session 的归档、取消归档、删除和删除日志恢复，同一个所有权 fence 会与 selected runtime 校验组合，因此 workspace 路由仍然保持 fail closed。writer lock 缺失、被替换、格式损坏、不是普通文件或为符号链接时，辅助清理都会停止。协议文档同时明确：冲突修复仅适用于当前 workspace 可维护的普通转录文件，并且不会绕过所有权校验。
本 PR 基于 #10179 已合入的生命周期实现，替代 #10286。
为什么需要
当前
main上，daemon runtime generation 可能在转录已经被删除、移动或作为冲突中的失败副本移除后立即关闭。旧的 generation fence 随后会拒绝辅助清理，即使 daemon 仍然持有精确的 writer lease。重试相同生命周期请求时，主操作已被识别为完成，不会再次清理这些 sidecar，从而使陈旧的 worktree、PR、prompt ledger、file history 或 organization 状态永久残留。将 writer lease 作为提交后的所有权边界，可以让发起事务的所有者安全完成清理，同时不会允许陈旧或外部 runtime 修改其他所有者的状态。
Reviewer 测试计划
如何验证
本地验证已通过 384 个 core 断言（3 个跳过）、146 个 CLI 断言、仓库 build、typecheck、lint 以及精确变更文件的格式检查。在当前
main的隔离真实文件系统基线中，五个普通 daemon 生命周期场景都能稳定复现永久辅助残留；修复后验证覆盖同一矩阵。证据（变更前后）
N/A — 非 UI 的生命周期正确性变更。
测试平台
环境（可选）
macOS 26.4.1、Node.js 22.22.3、npm 10.9.8、禁用 sandbox。
风险与范围
关联 Issue
替代 #10286，基于 #10179。