fix(serve): Isolate daemon session maintenance writers - #7975
Conversation
|
@qwen-code /takeover |
qwen-code-dev-bot
added
the
autofix/takeover
|
🤝 Takeover engaged: the autofix loop now manages this PR — it will address new review feedback and resolve base conflicts until the label is removed or the round cap is reached. Remove the 中文说明🤝 已接管：autofix 循环现在管理此 PR —— 将持续处理新的评审反馈与 base 冲突，直到移除标签或达到轮次上限。移除 |
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
d91708c to
3a6659a
Compare
|
Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. 中文请勿对活跃的 PR 执行 rebase 或 force-push，因为这会使已有的评审评论失效。另外，供日后参考：作为集成流程的一部分，机器人始终会自动将所有改动压缩（squash）为单个提交。 |
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： No action takenNo review feedback, inline comments, failed checks, or still-red checks were present in this evaluation round. The PR branch ( 中文说明未采取任何操作本轮评估中没有审查反馈、行内评论、失败的检查或持续红色的检查。PR 分支（ Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧠 Handled by Qwen Code · model/模型 |
🩺 serve daemon A/BBuilt the PR base vs this PR head ✅ No response changes against the PR base across 4 scenario(s). — Qwen Code · serve A/B |
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
CI fix summary
Commit: |
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： No action required this roundNo inline comments or review feedback were raised on this PR. The failed CI check Test (ubuntu-latest Node 22.x) is a pre-existing failure on the base branch ( All tests touched or added by this PR pass locally (553 tests across 12 test files), and 中文说明本轮无需操作本 PR 没有收到任何行内评论或审查反馈。 CI 检查 Test (ubuntu-latest Node 22.x) 的失败是基础分支（ 本 PR 涉及或新增的所有测试均在本地通过（12 个测试文件共 553 个测试）， Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧠 Handled by Qwen Code · model/模型 |
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： Autofix review round — no action neededThis round triaged the feedback newer than the last evaluation and found nothing actionable:
The only new item is an issue-level "CI fix summary" comment. It is an informational recap of the work already committed at head ( No code changes were made this round; the branch is left as-is. 中文说明Autofix 审查轮次 —— 无需处理本轮对上次评估之后新增的反馈进行了分类，未发现任何需要处理的内容：
唯一的新增条目是一条 issue 级别的“CI 修复总结”评论。它是对头提交（ 本轮未做任何代码改动；分支保持原样。 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧠 Handled by Qwen Code · model/模型 |
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Review: fix(serve): Isolate daemon session maintenance writers
Reviewed the full diff (31 files, +2667/−951) at 987bdd4.
Overview
Three coupled changes:
- Runtime pinning —
WorkspaceRuntimegainssessionRuntimeBaseDir, resolved once at creation and injected asQWEN_RUNTIME_DIRinto managed children.Storage.runWithResolvedRuntimeBaseDir()adds apinned: truecontext that beats bothprocess.envand nestedrunWithRuntimeBaseDir()calls. - Per-session writer leases —
SessionService.acquireSessionWriterLease(), with daemon delete/archive/unarchive/orphan-cleanup rewritten to: gate → close → classify → lease → re-classify → assert-owned → mutate → reconcile task → release. - Shutdown seal —
SessionArchiveCoordinator.sealMaintenanceAndWait(), surfaced as503 daemon_draining(REST) anderrorKind: daemon_draining(ACP).
The direction is sound: fail-closed, no hostname/PID/age-based lock stealing, and the storage pinning closes a real cross-workspace mutation hazard. Test coverage is strong — 16 targeted new tests on real temp runtime roots, including state-change-between-classifications, release failure, and shutdown ordering.
Findings below; inline comments on the specific lines.
Blocking
Three /workspace/:id/session-groups mutating routes hang the request. session.ts:3700, :3725, :3761 all added if (!runtime) return; with no response written and no next(). getByWorkspaceCwd returns undefined whenever the entry's state isn't 'active' (workspace-registry.ts:293 — reachable via blockReplacement after a failed runtime replacement), so the connection stays open until a socket timeout. The GET sibling at :3676 already uses the correct helper, resolveRuntimeFromWorkspaceParam (:615), which sends sendWorkspaceMismatch. See inline.
Correctness / behavior risks
Batch archive/unarchive lost all-or-nothing conflict semantics. Previously runExclusiveMany(activeIds) acquired the whole batch before any mutation, so a SessionArchivingError meant nothing had been written. Now each id takes its own gate concurrently and a mid-batch conflict is rethrown out of Promise.all, failing the whole request after other sessions were already archived and their leases released — the client gets a 409 with no record of what changed. Given the design doc says "every session is processed independently", converting a per-session SessionArchivingError into an errors[] entry (as delete already does for non-exclusive errors) would be more consistent.
Archive now enters the gate before classifying. Every id — including not-found and already-archived — now gets closeSession(…, { requireAgentClose: true }) and takes the exclusive gate. The renamed test in transport.test.ts records the visible consequence: a request that previously returned notFound: [id] now returns errorKind: 'session_archiving'. That contradicts the PR body's "local session_archiving conflicts … remain unchanged". Either restore a pre-gate classification for the cheap outcomes, or correct the compatibility section — a client-visible change shouldn't be documented only by a test rename.
Unbounded shutdown drain, now ahead of child teardown. await initialSessionMaintenanceWait; lands before startProcessRegistryShutdown(), and the new test asserts bridge.shutdown waits on it. Shared reads are excluded from the drain, but an admitted archive blocks on closeSession(requireAgentClose: true) — an unresponsive agent then blocks SIGTERM shutdown indefinitely with managed children never terminated. SHUTDOWN_FORCE_CLOSE_MS only covers listener drain. See inline.
workspaceRegistry.primary is a throwing getter, now read outside try. In /sessions/archive and /sessions/unarchive it sits above the try; in /session/:id/export and PATCH /session/:id/organization it's evaluated in the argument list. It throws WorkspaceGenerationClosedError when the primary isn't active, which Express 5 turns into a generic 500 rather than the workspace_runtime_unavailable 503 these surfaces use elsewhere. The delete route already reads it inside the try — match that.
Ambient/pinned split in updateScheduledTaskForMaintenance. The transcript comes from the service's pinned Storage, but the cron path comes from getCronFilePath(service.getProjectRoot()), which resolves the runtime root ambiently. Correct at every call site in this PR, but a future caller that forgets the runWithWorkspaceRuntimeStorage wrapper silently reconciles the wrong workspace's cron file — exactly the class of bug this PR exists to eliminate. Deriving the path from the runtime/service explicitly would make it structurally impossible.
API surface
- Writer conflicts flatten to strings in batch
errors[]. ASessionWriterConflictErrorlands as{ sessionId, error: "<message>" }in a 200 response, so clients can't distinguish "another writer holds it, retry" from a permanent failure. Since making conflicts actionable is the point, consider carryingerrorKind/codeon the per-session entries. - Mutation-applied-but-release-failed reports as an error. Documented and deliberate, and both operations are idempotent on retry — worth confirming the Web Shell treats archive/delete errors as "unknown, re-fetch" rather than "unchanged".
Performance
Per-session afterMutationApplied replaces one batched removeTasksForSessions(root, removed) with N calls. updateCronTasks serializes on a per-path mutex plus a file lock (cronTasksFile.ts:368), so it stays correct — but a 200-session batch delete now performs 200 sequential lock + full-file read + full-file rewrite cycles instead of one. Batching the reconciliation after the leases are released keeps both properties.
Style / smaller items
run-qwen-serve.ts:3111resolveSessionRuntimeBaseDirreimplementsStorage.resolvePath+getGlobalQwenDir, including a hardcoded'.qwen'instead of the exportedQWEN_DIR. Behaviorally equivalent today, drift-prone; belongs in core.replaceRuntimeEffectiveEnvsilently overwrites a reloadedQWEN_RUNTIME_DIR. Correct per the design, but a warning when the reloaded value differs from the pin would save an operator a confusing debug session.Storage.runWithRuntimeBaseDirbecoming a silent no-op inside a pinned context is a footgun — at minimum adebugLogger.debugon the ignored redirect.storage.ts— in the pinned branch,contextualDir.dir ?? Storage.getGlobalQwenDir()is unreachable;runWithResolvedRuntimeBaseDiralways sets a non-nulldir.runWithResolvedRuntimeBaseDirusespath.resolve(dir), so a relative argument resolves againstprocess.cwd()while the contract says "absolute". Assert it, or document the base.session-archive.ts—let lease;is an evolvingany;classifySessionLocationis a pass-through wrapper aroundservice.getSessionLocation.acp-http/index.ts—opts.workspaceRegistry?.primaryguards the registry with?., butprimaryitself throws; the?? Storage.getRuntimeBaseDir()fallback doesn't cover that.
Documentation
The first two paragraphs of the PR description — managed ACP children stopping writer admission, draining accepted recording work, releasing ownership before the parent reaps them, and the parent tracking child terminal state across every bridge — have no corresponding code in this diff. No acp-bridge or agent-package files are touched. Either that work belongs here, or the description should be trimmed to what's present (maintenance leases, runtime pinning, shutdown seal). As written it makes the diff much harder to review against its stated intent.
Security
No concerns found. Session IDs are validated against SESSION_FILE_PATTERN before any lock directory is touched (test covers ../invalid), and the pattern is non-global so there's no lastIndex statefulness. Log lines carry workspace/session/action/errorKind only — no owner tokens or lock paths — and go through safeLogValue. Lease acquisition fails fast with no retry loop, so it isn't a DoS amplifier.
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
Addressed the latest review against commit
Validation:
|
|
Thanks for the PR! Template looks good ✓ Problem: observed architectural issue, not theoretical. Linked issue #7752 describes daemon-side archive, delete, and cleanup paths racing external writers on the same transcript. The problem is concrete: two daemon processes (or a daemon and a managed ACP child) can mutate the same persisted session because the in-process archive coordinator alone doesn't fence cross-process access. Multi-workspace operations can also resolve storage through the ambient primary runtime instead of the selected workspace. Direction: aligned. Daemon data integrity is core to the serve functionality. The writer-lease protocol already exists in the codebase — this PR extends it to cover the maintenance paths that were missing it. CHANGELOG has no direct reference, but the daemon serve surface is actively developed and this is a natural hardening of the existing protocol. Size: this PR touches core paths ( Approach: the scope feels right for the stated goal. Every maintenance path (delete, archive, unarchive, disconnect rollback, scheduled-task rollback, keepalive cleanup, ACP orphan cleanup) needs the writer-lease wrapper, and the runtime storage pinning is a prerequisite for correct lock-path resolution. The Risk: no elevated risk signals — none of the changed files match the high-risk path patterns from the revert-history analysis. Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 问题：已观测到的架构问题，非理论性加固。关联 issue #7752 描述了 daemon 侧归档、删除和清理路径与外部 writer 竞争同一 transcript 的问题。问题是具体的：两个 daemon 进程（或 daemon 与受管 ACP 子进程）可能修改同一持久化 session，因为进程内 archive coordinator 无法隔离跨进程访问。多 workspace 操作也可能通过环境中的 primary runtime 解析存储，而非使用实际选中的 workspace。 方向：对齐。Daemon 数据完整性是 serve 功能的核心。Writer-lease 协议已存在于代码库中——本 PR 将其扩展到缺失的维护路径。CHANGELOG 无直接引用，但 daemon serve 表面正在积极开发中，这是对现有协议的自然加固。 规模：本 PR 触及核心路径（ 方案：范围与目标匹配。每条维护路径都需要 writer-lease 包装，runtime storage 固定是正确锁路径解析的前提。 风险：无升级风险信号——变更文件均未命中 revert 历史分析中的高风险路径模式。 进入代码审查 🔍 — Qwen Code · qwen3.8-max-preview Reviewed at |
Code ReviewIndependent proposal: given the problem (daemon maintenance racing external writers), I would (1) pin each workspace runtime's storage root at creation, (2) add a Comparison: the PR matches this proposal almost exactly. The implementation is well-structured:
No correctness bugs, security holes, or regressions found. The code follows project conventions (ESM, strict TypeScript, collocated tests, kebab-case files). One observation (non-blocking): the Sandboxed verification would settle the remaining behavioural gap: sequenceDiagram
participant P1 as REST or ACP request
participant P2 as ArchiveCoordinator
participant P3 as SessionService
participant P4 as WriterLease
participant P5 as Transcript FS
P1->>P2: runExclusiveMany(sessionId)
P2->>P2: check maintenanceSealed
P2->>P3: closeSession (local owner)
P2->>P3: classifySessionLocation
P3-->>P2: active or archived or notFound
P2->>P4: acquireSessionWriterLease
P4-->>P2: lease (or conflict error)
P2->>P3: reclassify + assertOwnedAndUnchanged
P2->>P5: mutate transcript
P5-->>P2: result
P2->>P4: release (owner-token verified)
P2->>P3: updateScheduledTask (best-effort)
P2-->>P1: per-session result
Files changed (31 of 31 shown)
TestingCI is fully green on
Not verified: macOS and Windows test runs were skipped in CI. The author reports testing on macOS only. The writer-lease contention behavior under real multi-process conditions is covered by unit tests with real temporary runtime roots but not by a live multi-daemon E2E scenario. Real-scenario testing: N/A — this is daemon lifecycle, storage isolation, and concurrency behavior with no user-interface change. 中文说明代码审查独立方案： 针对此问题（daemon 维护与外部 writer 竞争），我会 (1) 在创建时固定每个 workspace runtime 的存储根目录，(2) 添加忽略后续 env 重载的 对比： PR 与此方案几乎完全一致。实现结构良好：
未发现正确性 bug、安全漏洞或回归。代码遵循项目约定。 一个观察（非阻塞）： 沙箱验证可以解决剩余行为差距： 测试CI 在 未验证：macOS 和 Windows 测试在 CI 中被跳过。作者仅在 macOS 上测试。Writer-lease 竞争行为在真实多进程条件下由使用真实临时 runtime root 的单元测试覆盖，但未有实时多 daemon E2E 场景。 真实场景测试：N/A——这是 daemon 生命周期、存储隔离和并发行为变更，没有用户界面变化。 — Qwen Code · qwen3.8-max-preview Reviewed at |
|
Confidence: 3/5 — clean review, but the Stage 0 maintainer-awareness escalation (2,367 production lines touching core across two packages) needs a maintainer's sign-off. Stepping back: this is a well-designed change that solves a real problem. The writer-lease protocol already existed — the PR extends it to the maintenance paths that were missing it, and pins workspace storage roots so multi-workspace operations can't accidentally resolve through the primary runtime. The implementation matches my independent proposal almost exactly, the code is straightforward, and the test coverage is thorough (692 lines of session-archive tests alone, plus coverage for the storage pinning, keepalive cleanup, and error mapping). The reason this caps at 3/5 is purely the Stage 0 policy: a fork PR with 2,367 production logic lines touching No blocking issues found. The non-blocking observation about @doudouOUC — nice work. The design doc, the per-session error isolation, and the shutdown drain protocol are all well thought out. 中文说明置信度：3/5 —— 审查干净，但 Stage 0 维护者关注升级（2,367 行生产逻辑触及核心，跨两个包）需要维护者签字。 退后一步看：这是一个设计良好的变更，解决了真实问题。Writer-lease 协议已经存在——PR 将其扩展到缺失的维护路径，并固定 workspace 存储根目录，使多 workspace 操作不会意外通过 primary runtime 解析。实现与我的独立方案几乎完全一致，代码直接，测试覆盖全面（仅 session-archive 测试就有 692 行，加上存储固定、keepalive 清理和错误映射的覆盖）。 封顶 3/5 的原因纯粹是 Stage 0 策略：一个 fork PR 有 2,367 行生产逻辑触及 未发现阻塞问题。关于 @doudouOUC —— 做得好。设计文档、逐 session 错误隔离和 shutdown drain 协议都考虑周全。 — Qwen Code · qwen3.8-max-preview Reviewed at |
|
⏸️ Deferring to @wenshao @tanzhenxin @yiliang114 @LaZzyMan — Stage 0 maintainer-awareness escalation: a Update on re-run ( |
|
@qwen-code /verify |
|
Sandboxed verification: ✅ passed — merge-ready (agent verdict) - 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: 40 passed · 0 failed · 40 total 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：40 通过 · 0 失败 · 40 总计 Verification report (report.md)Harness scripts and raw logs are in the workflow run artifacts (7-day retention). — Qwen Code · sandboxed verification |
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： No actionable feedback this round. The only new comment is an automated CI status update (sandboxed verification running) — no reviews, inline comments, failed checks, or maintainer requests to address. 中文说明本轮没有可操作的反馈。唯一的新评论是自动 CI 状态更新（沙箱验证正在运行）——没有审查意见、行内评论、失败的检查或维护者请求需要处理。 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧠 Handled by Qwen Code · model/模型 |
|
@qwen-code /verify |
|
Sandboxed verification: ✅ passed — merge-ready (agent verdict) - 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: 64 passed · 0 failed · 64 total 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：64 通过 · 0 失败 · 64 总计 Verification report (report.md)Harness scripts and raw logs are in the workflow run artifacts (7-day retention). — Qwen Code · sandboxed verification |
Post-merge measurement of QwenLM#8016, on the first two live runs after it landed (QwenLM#7975 and QwenLM#8066): the browser installed successfully — "Install evidence browser: success" in both — and both reports carried **zero images**. QwenLM#7975 has 31 tables, QwenLM#8066 has 407 assertions. Neither has a witness. The cause is the third instance of one structural bug, and it is the same one twice already fixed: QwenLM#7917 the /verify recommendation sat inside a section headed "local invocation ONLY" -> 1/16 reach QwenLM#8016 captures were "Optionally ... when text cannot carry the oracle" -> 0/14 now captures are a required deliverable in the ARTIFACT CONTRACT, but the plan the agent executes is the Scope-selection budget list, and that list had zero mentions of evidence, png, capture or screenshot An agent that budgets by that list never allocates time for a capture, however firmly a later section says to produce one. So captures are now budget item 4 alongside the A/B, the harnesses and the gates, with the time reserved (~5 min), the gate named (QWEN_VERIFY_CHROMIUM=1), the count bounded (normally two, at most a handful), and the measured failure stated so the rule carries its own reason. The report structure gets the matching half: the A/B table item now says to reference the capture of those cells by filename. A produced image with nowhere to go is as dead as an unproduced one. Mutation-verified 4/4: dropping the budget item, the time allowance, the count bound, or the report-side reference each turns the test red. Not changed, and worth stating rather than implying: the seven techniques from QwenLM#8010 do not appear in either report. I do not read that as failure — every one of them is conditional ("when one fix bundles two changes", "when the oracle is an instrument", "when a PR adds a writer to a shared store") and neither PR met those conditions. QwenLM#8010 is not yet falsifiable from these runs. What both reports DO show is older skill rules working: a naive-matcher differential in QwenLM#8066, and QwenLM#7975's realpath check finding node_modules/@qwen-code/qwen-code-core resolving into the HEAD tree and then reasoning about whether that contaminates the control. 107/107 tests; prettier and eslint clean. Co-authored-by: wenshao <wenshao@example.com>
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： No action this roundThere is no actionable feedback to address on PR #7975.
No code change was made and no verification commands were run, since there is nothing to fix or re-verify. The branch is left as-is at 中文说明本轮无需处理PR #7975 没有需要处理的可执行反馈。
本轮未做任何代码改动，也未运行任何验证命令，因为没有需要修复或重新验证的内容。分支保持在 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧠 Handled by Qwen Code · model/模型 |
|
@qwen-code /triage |
|
Triage re-run completed without a new review.
The stage comments above were updated with the latest result. View workflow run. |
|
@qwen-code /triage |
|
⏸️ Deferring to @wenshao — this PR touches core infrastructure ( |
|
Triage re-run completed without a new review.
The stage comments above were updated with the latest result. View workflow run. |
ytahdn
left a comment
ytahdn
left a comment
There was a problem hiding this comment.
LGTM 👍
Solid approach to isolating daemon session maintenance with the existing writer-lease protocol. The fail-closed semantics for conflicts and the daemon_draining response during shutdown are well-designed. The multi-workspace runtime root pinning and the partial-result preservation in batch maintenance are both important correctness guarantees that are well-implemented here.
1,600+ passing assertions covering Core, CLI, ACP bridge, and ACP agent suites gives good confidence. The risk/scope section is thorough and honest about what remains out of scope.
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
LGTM — writer-lease lifecycle is correctly implemented with release in finally-equivalent paths. Shutdown drain seals synchronously and waits for admitted work. Batch isolation preserves per-session results on conflict. Workspace runtime isolation prevents cross-contamination. Test coverage is thorough (lease lifecycle, drain, batch conflicts, workspace isolation all covered).
Non-blocking suggestions:
- Separate
alreadyActive+errorsdual-listing into awarnings[]array for non-fatal reconciliation failures - Make keepalive
cleanupSessionrequired (or have the fallback acquire the lease) to close the latent bypass - Extract the repeated batch orchestration skeleton into a generic
runPerSessionMaintenance<T>helper - Add a comment documenting why the pre-check + synchronous set-addition is race-free in single-threaded JS
- Add edge-case tests:
deleteDaemonSessionIfOrphannon-SessionNotFoundError propagation, unarchive dual-listing behavior
Local verification round — real daemon, A/B against baseVerdict: verified — 76 executed scripted assertions passed, 0 failed. No blocking finding. That is 38 distinct checks run twice: once at the PR head This ran on a maintainer machine (macOS, Darwin 25.6.0, Node The PR merged as 中文版报告结论：已验证 —— 实际执行的 76 条脚本化断言全部通过，0 条失败，无阻断性问题。 即 38 条不同的检查各跑两遍：一遍在 PR head 本轮在维护者本机（macOS，Darwin 25.6.0，Node 本 PR 已于 14:02Z 以 三个 A/B 场景的结论
发现的问题
未覆盖范围 ACP / JSON-RPC 侧的 Central claim: daemon maintenance now fails closed when another writer owns the sessionEach arm: one daemon, one workspace, six persisted transcripts. A separate Node process acquires a real
A/B table 1 — maintenance against a live foreign lease
3/3 operations flip from "silently mutates another writer's transcript" to "refuses per session, mutates nothing". Request status stays Secondary claim: workspace-qualified maintenance stays inside the selected runtime
A/B table 2 — two workspaces, two runtime roots
The base row is the sharper half of this: base does not merely read the wrong root, it deletes a file the caller never asked about and reports success. 9/9 assertions. Secondary claim: shutdown seals new maintenance and waits for admitted maintenanceThe natural duration of a 100-session archive batch was measured first (head 537 ms, base 149 ms) so
A/B table 3 —
7/7 assertions. Vacuity check on the new testsFive single-point mutations of the PR's own guards, each run against the suite that should catch it, plus an unmutated control.
Each killed mutant fails the intended behavioural assertion by name (e.g. "does not archive while another writer holds the lease", "seals new maintenance and waits only for admitted exclusive work"), not a compile or import error. Targeted gates at head: cli Re-run on the merged tree. Because the PR landed mid-round, all three harnesses were re-run against current Findings1 — # In a built tree, replace the body of runWithWorkspaceRuntimeStorage with
# void Storage; // keep the import used under noUnusedLocals
# return fn();
npm run build -w packages/cli
(cd packages/cli && npx vitest run src/serve/multi-workspace-sessions.test.ts \
src/serve/workspace-qualified-rest.test.ts) # 118/118 green
node harness/ab-runtime-isolation.mjs head <tree> /tmp/iso-m2.json # listing shows the decoy2 — Per-session lease cost, measured and accounted (informational). A 100-session archive batch goes 149 ms → 537 ms (+388 ms, ≈3.9 ms/session). Measured in isolation, one 3 — The PR head built clean; the merged result did not (already fixed). A side-task route added to Not covered
MethodologyTwo worktrees at |
|
Released in v0.21.2. |
6 participants
What this PR does
This PR isolates daemon transcript maintenance with the existing writer-lease protocol. Each workspace runtime now pins one absolute session runtime root for its lifetime and passes that root to managed children.
Session deletion, archive, unarchive, disconnect rollback, scheduled-task rollback, keepalive late-spawn cleanup, and ACP orphan cleanup operate inside the selected runtime and acquire one daemon writer lease per session before mutating persisted transcripts. Batch maintenance preserves completed per-session results if another session encounters a conflict after processing begins. Shutdown seals new maintenance admission, waits for admitted maintenance leases, and exposes a typed
daemon_drainingresponse without waiting for shared transcript exports.Why it's needed
Daemon-side archive, delete, and cleanup paths could mutate a transcript while another daemon or managed writer still owned it, and multi-workspace operations could resolve storage through an ambient primary runtime instead of the selected workspace.
The change makes daemon maintenance fail closed when another writer owns the session and keeps every selected workspace bound to its own transcript, lock, organization, and scheduled-task state. It deliberately does not infer safety from hostname, PID visibility, or lock age.
Reviewer Test Plan
How to verify
Hold a writer lease for a persisted session and attempt daemon delete, archive, and unarchive operations from another service instance. Confirm that each operation reports the per-session writer conflict without changing the transcript, then succeeds after the first lease is released.
Configure primary and secondary workspaces with different runtime roots and use workspace-qualified session maintenance. Confirm that transcript classification, writer locks, organization state, scheduled tasks, reads, and exports stay within the selected runtime, including after an environment reload.
Begin maintenance and daemon shutdown concurrently. Confirm that admitted maintenance completes and releases its lease before shutdown finishes, newly admitted maintenance receives
daemon_draining, and a shared export does not block maintenance drain. Confirm that managed child termination starts immediately when shutdown begins, so it can unblock an admitted close.Race one session in a multi-session archive or unarchive request with another local maintenance operation. Confirm that completed sessions remain reported in their normal result arrays while the racing session is reported in
errors[].Automated verification completed with the focused Core, CLI, ACP bridge, and ACP agent suites, including 1,600+ passing assertions, followed by
npm run build,npm run lint, andnpm run typecheck.Evidence (Before & After)
N/A — this is daemon lifecycle, storage isolation, and concurrency behavior with no user-interface change.
Tested on
Environment (optional)
macOS development checkout, Node.js 22, temporary local runtime roots, managed ACP child-process tests, and loopback daemon HTTP tests.
Risk & Scope
SIGKILL, hostname- or time-based lock stealing, TTL or heartbeat recovery, force unlock, sealed takeover, non-managed standalone writers, and mixed-version writers sharing one workspace remain out of scope.Linked Issues
Related to #7752.
中文说明
本 PR 做了什么
本 PR 使用现有 writer lease 协议隔离 daemon transcript 维护。每个 workspace runtime 现在会在整个生命周期内固定一个绝对 session runtime root，并将该根目录传给受管子进程。
会话删除、归档、取消归档、断连回滚、scheduled-task 回滚、keepalive late-spawn 清理和 ACP orphan 清理都会在选中的 runtime 内运行，并在修改持久化 transcript 前为每个 session 单独获取 daemon writer lease。当批量维护开始处理后某个 session 遇到冲突时，已完成的逐 session 结果会被保留。Shutdown 会封闭新的维护准入，等待已进入的 maintenance lease，并返回类型化的
daemon_draining响应，同时不等待 shared transcript export。为什么需要
daemon 侧的归档、删除和清理路径可能在另一个 daemon 或受管 writer 仍持有会话时修改 transcript；多 workspace 操作也可能通过环境中的 primary runtime 解析存储，而不是使用实际选中的 workspace。
本变更让 daemon maintenance 在其他 writer 持有会话时安全地 fail closed，并确保选中的每个 workspace 始终绑定到自己的 transcript、lock、organization 和 scheduled-task 状态。它明确不会根据 hostname、PID 可见性或锁时间推断可回收性。
Reviewer 测试计划
如何验证
为一个持久化 session 持有 writer lease，并从另一个 service 实例尝试 daemon delete、archive 和 unarchive。确认每个操作都会返回对应 session 的 writer conflict 且不修改 transcript；第一个 lease 释放后重试能够成功。
为 primary 和 secondary workspace 配置不同 runtime root，并使用 workspace-qualified session maintenance。确认 transcript 分类、writer lock、organization 状态、scheduled task、读取和导出始终位于选中的 runtime，包括环境 reload 之后。
让 maintenance 与 daemon shutdown 并发开始。确认已进入的 maintenance 完成并释放 lease 后 shutdown 才结束，新进入的 maintenance 收到
daemon_draining，并且 shared export 不会阻塞 maintenance drain。确认 shutdown 开始时会立即启动受管子进程终止流程，从而能够解除已进入 close 操作的阻塞。让一个多 session archive 或 unarchive 请求中的某个 session 与另一项本地 maintenance 操作发生竞争。确认已完成的 session 仍出现在正常结果数组中，而发生竞争的 session 会记录到
errors[]。自动验证已覆盖相关 Core、CLI、ACP bridge 和 ACP agent 测试套件，共通过 1,600 多项断言，随后
npm run build、npm run lint和npm run typecheck也全部通过。证据（修改前与修改后）
N/A——这是 daemon 生命周期、存储隔离和并发行为变更，没有用户界面变化。
已测试平台
环境（可选）
macOS 开发检出、Node.js 22、临时本地 runtime root、受管 ACP 子进程测试和 loopback daemon HTTP 测试。
风险与范围
SIGKILL后自动恢复、基于 hostname 或时间的锁抢占、TTL 或 heartbeat 恢复、force unlock、sealed takeover、非受管 standalone writer，以及 mixed-version writer 共享同一 workspace，均不在本 PR 范围内。关联 Issue
关联 #7752。