Repository navigation
feat(serve): Propagate session list cancellation - #8954
Conversation
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
E2E test report
|
|
@qwen-code /triage |
wenshao
left a comment
There was a problem hiding this comment.
Not reviewed: cross-file tracing — the agent failed to return twice.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not reviewed: build-and-test — Test (macos-latest, Node 22.x) was skipped in CI and local verification used Node 24.18.1.
Not reviewed: build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and Windows was not tested locally.
Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 5.
中文说明
未审查:cross-file tracing — the agent failed to return twice。
未审查:build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未审查:build-and-test — Test (macos-latest, Node 22.x) was skipped in CI and local verification used Node 24.18.1。
未审查:build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and Windows was not tested locally。
未审查:反向审计——在 5 轮的反审轮数上限内未收敛。
— gpt-5.6-sol via Qwen Code /review (v0.21.10)
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
[codex] Review round 1 handled in
Verification: focused Core and CLI Vitest suites passed; repository build, typecheck, and lint passed. Two consecutive broad diff audits found no further actionable issue. Resolved 19/19 review threads after replying to each. |
|
Review round 2 triage on
No code change was made. Seven deferred threads were resolved; the two disputed threads remain open. The unrelated Linux |
yiliang114
left a comment
There was a problem hiding this comment.
Reviewed at 9e835e2 — cancellation propagation through the daemon session-list path (REST disconnect + ACP connection teardown → catalog waiter cancellation, per-load AbortController in the persisted catalog cache).
Correctness — no P0/P1 found:
- Clean waiter/load signal separation: caller signals attach only to their own waiter promise (
attachWaiter); the physical load owns its controller and is never directly combined with caller signals. Abort listeners are removed on settlement ({once:true}+ explicitremoveEventListener) — no leaked listeners, no timers added. - First-wins settlement (
load.settled+ per-waiter flag) prevents a late caller abort from replacing a completed scan; detached/stale loads cannot install snapshots (identity + generation + aborted guards) nor clear a replacement load — both race directions are test-covered. - Last-waiter cancellation aborts and synchronously detaches the load so a replacement scan starts immediately; the detached
managedrejection is still handled by the already-attached waiter handlers, so no unhandled-rejection path even if a loader ignores cancellation. - Every catch-swallow site re-checks
signal.throwIfAborted()before falling back (ENOENT→empty incl. an ENOENT-coded abort reason, sidecar→null, sessionExists→false, live-merge→liveMergeFailed, readLines/readRuntimeStatus/readWorktreeSession), so cancellation cannot be misreported as missing data. - The 128-entry setImmediate yield in
listSessionsonly triggers when a signal is passed; the signal-less CLI/LiveTask path is verified unchanged. - Route wiring removes both
req 'aborted'/res 'close'listeners in finally; the ACP dispatch silently returns when the connection signal is aborted, avoiding buffered error frames on destroyed connections (asserted viabufferedConnectionFrames).
Tests: ~950 new lines covering single/last-waiter cancel, detach+replace, pre-aborted lookup, null abort reason, cache-hit cancellation, REST/ACP disconnect integration, worktree enrichment, numeric pagination, and core signal plumbing.
CI at head sha: Test (ubuntu-latest, Node 22.x) ✅, web-shell E2E smoke ✅, Desktop Shell (ubuntu/windows) ✅; cancelled route runs are fork-routing no-ops; mac/win/integration skipped as expected for fork PRs.
Non-blocking nits (P3):
- The
signal ? sessionExists(id, {signal}) : sessionExists(id)ternary is repeated in three places; always passing the options object would be equivalent since it defaults to{}. req.aborted/'aborted'is legacy-leaning on newer Node, but fine for the current HTTP/1 serve stack.
LGTM.
|
@qwen-code /triage |
What this PR does
This PR propagates request cancellation through daemon Session List reads while preserving the persisted catalog cache introduced by #8892. Shared organized and metadata scans now track independent waiters: cancelling one REST or ACP caller leaves other REST, ACP, and LiveTask waiters running, while cancelling the last waiter aborts and detaches the physical scan so a replacement request can start immediately. Cancellation also reaches numeric pagination, JSONL reads, runtime-status and worktree-sidecar enrichment, persisted-existence checks, and the cooperative directory scan.
Why it's needed
Disconnected Session List clients previously left expensive persisted-session scans running to completion, which could waste daemon resources and delay later catalog work. Directly wiring a caller signal to the shared loader would instead let one client cancel every consumer, so the cache needs waiter-aware cancellation ownership and late-load isolation.
Reviewer Test Plan
How to verify
Start two identical organized Session List requests and disconnect one; the remaining request should return the complete catalog from one physical scan. Disconnect every cancellable waiter; the physical scan should abort, and the next request should start a fresh scan before the following request hits the existing two-second TTL. Verify numeric pagination, trusted-secondary persisted preflight, ACP connection destruction, and cancellation during JSONL, runtime-status, worktree-sidecar, and project-membership reads return no partial data, HTTP 500, ACP error frame, or cancellation error log.
Evidence (Before & After)
N/A — daemon-internal behavior with unchanged REST and ACP response schemas.
Tested on
Environment (optional)
macOS local Node.js 22-compatible workspace; isolated
qwen serveE2E with 1,200 persisted sessions and package-level Vitest coverage.Risk & Scope
Linked Issues
References #8892.
中文说明
此 PR 的作用
此 PR 在保留 #8892 引入的持久化目录缓存基础上,将请求取消传播到 daemon Session List 读取。共享的 organized 和 metadata 扫描现在跟踪独立 waiter:取消单个 REST 或 ACP 调用方不会影响其他 REST、ACP 或 LiveTask waiter;最后一个 waiter 取消时会中止并同步脱离物理扫描,使替代请求可以立即启动。取消还会传播到数字分页、JSONL 读取、runtime status 与 worktree sidecar 补充、持久化存在性检查以及协作式目录扫描。
为什么需要
Session List 客户端断开后,昂贵的持久化会话扫描此前仍会运行到结束,浪费 daemon 资源并可能延迟后续目录工作。若将调用方 signal 直接连接到共享 loader,单个客户端又会取消所有消费者,因此缓存需要 waiter 感知的取消所有权和晚到 load 隔离。
Reviewer 测试计划
如何验证
启动两个相同的 organized Session List 请求并断开其中一个;剩余请求应通过一次物理扫描返回完整目录。断开所有可取消 waiter 后,物理扫描应中止,下一请求应启动新的扫描,随后请求命中现有两秒 TTL。验证数字分页、可信 secondary 的 persisted preflight、ACP connection destroy,以及 JSONL、runtime status、worktree sidecar 和项目归属读取期间的取消都不会返回部分数据、HTTP 500、ACP error frame 或取消错误日志。
证据(Before & After)
N/A — daemon 内部行为,REST 和 ACP 响应 schema 未变化。
测试平台
环境(可选)
macOS 本地 Node.js 22 兼容工作区;使用 1,200 个持久化 session 的隔离
qwen serveE2E,并运行了包级 Vitest。风险与范围
关联 Issue
参考 #8892。