perf(cli): Cache persisted session catalogs - #8892
Conversation
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
E2E Test ReportEnvironment: macOS arm64, Node.js v22.22.3, npm 10.9.8, global qwen 0.21.8 for the baseline, source daemon for the implementation, and isolated temporary runtime/workspace directories. No user session directory was read or modified. The empty-runtime baseline sent five concurrent list variants to global qwen; every request returned HTTP 200 in approximately 1–8 ms.
After the 1,000-session run, daemon status reported event-loop mean 21.2 ms, p99 24.1 ms, and max 33.8 ms. Response envelopes retained only the existing Deterministic verification passed for the cache state machine (9 tests), focused REST/session-list coverage (80 tests), ACP list and mutation coverage (27 tests), LiveTask coverage (14 tests), and selected multi-workspace/runtime-isolation coverage (7 tests). The five concurrent query variants performed one complete catalog load and one sidecar pass; warm hits performed neither, while live and organization changes remained immediately visible. Prettier, ESLint, and |
🩺 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>
|
Re-run after two test-hardening commits ( Template looks good ✓ (all sections, bilingual).
Moving on to code review. 🔍 中文说明本轮 re-run 基于两个测试加固 commit（ 模板完整 ✓（各节齐全，中英双语）。
进入代码审查。🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewRe-run note: the two commits since the previous pass ( Independent baseline (unchanged from last pass): for "concurrent requests repeat the same catalog scan", the minimal design is a keyed map of in-flight promises (single-flight) with no TTL. Against the diff:
sequenceDiagram
participant P1 as List requester
participant P2 as Catalog cache
participant P3 as JSONL and sidecar scan
participant P4 as Mutation route
P1->>P2: lookup scope
alt fresh snapshot
P2-->>P1: cache hit
else scan in flight
P2-->>P1: join shared promise
else cold
P2->>P3: run scan once
P3-->>P2: install snapshot, 2s TTL
P2-->>P1: scan result
end
P4->>P2: invalidate after mutation
Files changed (12)
Test evidence (the PR's own CI, read via API — no PR code executed here)On the reviewed commit, all four What the suite proves: mechanism and wiring — coalescing, TTL, invalidation ordering, runtime isolation, telemetry, wire shape (Serve A/B). What it does not prove: the size of the perf win. The numbers in play come from the author's E2E report (macOS arm64, self-run): cold-concurrent p50 ~988 ms at 1,000 sessions vs ~7 ms warm, one catalog load for five concurrent variants, bounded event-loop lag. Plausible and consistent with the mechanism — but author-reported, not independently re-run here.
Sandboxed verification would settle the remaining claim: 中文说明代码审查（re-run）：自上一轮以来的两个 commit（ 独立基线不变：对"并发请求重复同一目录扫描"，最简方案是只做 single-flight、不做 TTL。对照 diff：生产代码（与上轮审查的 commit 逐字节一致）为 single-flight 加 2s TTL 快照缓存、代数失效、每个 REST/ACP mutation 路由的失效钩子、写时复制、淘汰上限与 span 遥测；并发核心经推演仍然正确（被失效的在途加载无法安装快照、过期拒绝不会清掉更新的加载、失败永不缓存也永不回退旧值、共享快照跨请求只读）。Runtime 隔离经 测试证据（来自 PR 自己的 CI，通过 API 读取，本审查未执行任何 PR 代码）：该 commit 上四个 pull_request 事件 workflow 全部绿色完成——Qwen Code CI（含 ubuntu 单元门禁：build + typecheck + 单测）、Serve A/B（与 base 相比 4 个场景无响应结构变化）、Qwen Live Host CI、SDK Java（含 Real daemon E2E）。Desktop Shell 双平台与 web-shell E2E Smoke 亦绿色。macOS/Windows 单测与 CLI 集成测试按 ci.yml 设计仅在 merge queue 运行，PR 阶段跳过属正常信号。该 SHA 上其余 check-run 为机器人编排任务（autofix 路由、review 管线），按设计跳过或取消，非 PR CI。无红色、无 pending。单测证明的是机制与接线（合并、TTL、失效时序、隔离、遥测、wire shape）；不能证明收益大小——目前的数字来自作者 E2E 报告（macOS arm64 自测）：1,000 sessions 冷并发 p50 约 988 ms 对热态约 7 ms、五并发一次目录加载、event-loop lag 有界。合理且与机制一致，但为作者自述，未在本审查中独立复跑。 沙箱验证可以解决剩余声明： — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 4/5 — careful, well-tested concurrency work with green CI and a maintainer approval on this head; the one standing reservation is that the perf win rests on author-reported measurements. The previous pass deferred with three questions for a human. All three now have answers:
What moved the verdict beyond those answers: the two follow-up commits are exactly the response you want to review feedback — they converted the Reservations kept on record, none blocking: "faster" is measured by the author on one platform — an independent Approving, pinned to the reviewed commit. 中文说明置信度：4/5 —— 仔细且测试充分的并发工作，CI 全绿，当前 head 已有维护者批准；唯一保留意见是性能收益基于作者自述的测量。 上一轮以三个问题转交人工裁决，现在三个都有了答案：
除答案之外真正改变结论的是：两个后续 commit 正是面对 review 反馈应有的回应——把上一轮 保留意见（均不阻塞）："更快"由作者在单一平台上测量——合入前如有人想要独立验证，对 base build 的 批准，钉在所审查的 commit 上。 — Qwen Code · qwen3.8-max Reviewed at |
|
[codex] Thanks — keeping the 2s TTL design. The E2E report above already records 300- and 1,000-session cold-concurrent and warm p50/p95 results, plus deterministic catalog/sidecar load counts; while that is not a same-dataset base-build A/B, it measures the burst path this PR targets. The TTL intentionally covers sequential reload bursts beyond exact promise overlap, and the listed internal maintenance removal paths deliberately rely on the documented two-second bound under the approved consistency boundary. No code change is needed for this review. |
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.
Not explored to full depth (tool budget reached): You are review agent reverse-audit — Reverse audit agen...: none — the walk completed within budget.; You are review agent reverse-audit — Reverse audit agen...: none — all checks above completed within budget.; This PR adds a process-local, two-second-TTL single-fligh...: none — all checks I started completed within budget..
Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 5.
中文说明
已审查。 建议见行内评论。
未探索到全部深度（达到工具调用预算）：You are review agent reverse-audit — Reverse audit agen...：none — the walk completed within budget.；You are review agent reverse-audit — Reverse audit agen...：none — all checks above completed within budget.；This PR adds a process-local, two-second-TTL single-fligh...：none — all checks I started completed within budget.。
未审查：反向审计——在 5 轮的反审轮数上限内未收敛。
— qwen3.8-max via Qwen Code /review (v0.21.9)
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
Review follow-up completed on
Verification: 25 cache/LiveTask tests passed; 3 targeted server mutation/telemetry tests passed; 101 multi-workspace tests passed; changed-file ESLint passed. The combined server + multi-workspace run had 1 unrelated flaky registry-source assertion among 1,016 tests, which passed immediately in isolation. Full build remains locally blocked by pre-existing Ink type/export mismatches in selection UI files; the prior PR SHA was fully green and the new test-only SHA has fresh CI running. |
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
LGTM. The cache core is correct and unusually well-tested: single-flight waiters receive failures (never cached, inFlight cleared on both outcomes), TTL is non-sliding from scan completion with an independent read-path age guard, generation counters keep invalidated/superseded loads from installing or deleting newer values, keys derive from the same runtime.sessionRuntimeBaseDir + workspaceCwd on read and invalidate sides so cross-workspace collision is impossible in every current call path, reads are pinned via resolved-runtime ALS with SessionService capturing the root at construction, and memory is bounded (50k-summary cap, oversized served-not-installed, 2s lifetime). Invalidation runs after the mutation settles and before responses are sent, including ACP over-the-wire archive/unarchive/delete; the 12 bot suggestions are all dispositioned (7 fixed with tests, 2 deferred, 3 rejected with verified rationale). CI green on head.
Two P2 hardening items for follow-up, not blockers: (1) runtimeBaseDir is optional on the read options (session-list.ts:88) — all three current daemon callers pass it, but a future caller omitting it silently reads the process-default root and its invalidations would never match; make it required or assert presence. (2) Pre-existing: the ACP _qwen/sessions/{delete,archive,unarchive} handlers build SessionService without the runtime root (dispatch.ts:4337/4362/4386) unlike removeOrphanSession and the REST equivalents — now that reads are pinned to sessionRuntimeBaseDir, a secondary-workspace dispatcher with a different ambient root mutates a different root than the catalog being listed; one-line-each fix. P3s (internal maintenance deletes TTL-only, span-attribute overwrite on multi-lookup, no over-the-wire close/update_metadata test, external writers TTL-covered) are all documented, bounded tradeoffs. Ship it.
|
@qwen-code /triage |
|
Sandboxed verification: ❌ not passed — findings reported (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: 41 passed · 1 failed · 42 total 中文 — 判定：❌ 不通过 · 报告了发现（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：41 通过 · 1 失败 · 42 总计 Verification reportPR 8892 Deep Verification —
|
| phase (requests) | base (no cache) | head (cached) | interpretation |
|---|---|---|---|
| cold — 5 concurrent variants (org-all ×2 [LiveTask shape], org-archived, sourceType, parentSessionId) | 2032 (4 full active scans ×505 + 1 archived ×12) | 517 (1 active + 1 archived scan) | single-flight + shared scope across query kinds |
| warm — same 5, ~0.4 s later (inside TTL) | 2032 | 0 | TTL reuse; base rescans everything |
| expire — 1 organized-all after >2.4 s | 505 | 505 | TTL lapse rescans (parity) |
| numeric default path, twice | 41 / 41 | 41 / 41 | cache bypass preserved (parity) |
session-info, twice |
256 / 256 | 256 / 256 | independent disk scan preserved (parity) |
| total catalog reads over the 10 list requests | 4064 | 517 (-87.3%) | load-bearing proof |
Wire shape: all 15 response bodies saved per arm are value-identical (order-insensitive deep compare, ab-assert.mjs); 10/15 are byte-identical, the 5 organized-view bodies differ only in JSON key order (Finding #3). 17/17 scripted A/B assertions passed. Raw cells: harness/run-head.result.txt, harness/run-base.result.txt. Witness: 01-ab-catalog-reads-head-vs-base.png.
Findings
1. The generation install-guard is load-bearing but unpinned by any test (Medium — test gap, code is correct)
The cache installs a completed scan only if current.generation === generation && snapshot.sessions.length <= maxRetainedSummaries (persisted-session-list-cache.ts). Deleting the generation clause (mutant M5) leaves the PR's own test does not let an invalidated load install or clear a newer load green (1 passed | 10 skipped): that test starts a replacement load immediately after invalidate(), so current.inFlight?.promise === managed already blocks the stale install and the generation clause never decides the outcome — the test passes for a different reason than its name claims.
The clause is not dead code: it is the only thing guarding the interleaving invalidate arrives mid-scan and no replacement request joins. A scratch probe (harness/zz-verify-generation.test.ts.probe; run log harness/mut-M5-probe.log) drives exactly that sequence — lookup → invalidate → resolve old load → next lookup must be scan:
- clean source: 1 passed (stale snapshot correctly not installed);
- M5 mutant: 1 failed —
expected 'cache_hit' to be 'scan': the pre-invalidation snapshot is installed and served to the next request for up to the full TTL — the exact stale-read class this PR exists to prevent.
Consequence: a future refactor could delete the generation check and every test in this PR would stay green. Suggested fix (measured, in scratch copy): add the probe test verbatim to persisted-session-list-cache.test.ts. It passes on the shipped code and dies cleanly under M5; the affected suite's other counts are unchanged (10 passed neighbors stay green). The reject path needs no analogous clause — it never installs a value — and the failure-not-retained property is pinned (M6 killed by does not retain loader failures).
2. Reviewer Test Plan step "Exercise metadata update … through REST" is unreachable for persisted-only sessions (Low — plan/code mismatch, pre-existing)
PATCH /session/:id/metadata is wrapped in withOwnerMutableSession, which requires a live session; persisted-only sessions get 401 on both arms (identical, so not a regression — the guard predates the PR). The plan's metadata-freshness step therefore cannot be performed against the fixture it describes ("workspace containing persisted sessions"); it only works for sessions with an attached client, which the harness cannot create without model credentials. The invalidation wiring itself is real and pinned: mutant M9 (removing the route's own finally { invalidateSessionLists(runtime, ['active']) }) is killed by invalidates a warm catalog on metadata update… with expected "listSessions" to be called 2 times, but got 1 times (harness/logs/mut-M9.log). Note this route does not use the shared runWithSessionListInvalidation helper, which is why M2 does not kill that test. Suggestion: reword the plan step to "attach a live session, then PATCH metadata".
3. Organized-view responses change JSON key order vs base (Informational)
For the five organized-view bodies, head emits worktree before groupId/color/isPinned, base emits it after — because enrichment moved inside the cached loader (loadAllPersistedSummaries) ahead of applyOrganization, while base enriched after. Values are identical (verified order-insensitively); JSON objects are unordered, so no REST/ACP consumer should observe this — but any byte-comparing snapshot/golden-file client would. No action needed unless such consumers exist.
Injection screening
PR text contained no steering instructions; no injection attempts observed.
Not covered
- Per-commit attribution. Checkout is depth 2 (shallow); only
HEAD^2of the 4 commits in the metadata snapshot is locally reachable (git rev-list HEAD^1..HEAD^2= 1 commit). The aggregateHEAD^1..HEADdiff is what was verified. Commit 2 ("Pin LiveTask rollback storage runtime") is nonetheless pinned behaviorally: mutant M8 (unpinningremoveSessionfromrunWithWorkspaceRuntimeStorage) is killed bylive-task-service.test.tsasserting the runtime base dir of the deletion. - Live-session mutation paths end-to-end (REST/ACP metadata on live sessions, session close): require an attached ACP client + model credentials this sandbox does not have. The close/close-invalidates wiring is covered by the PR's own
server.test.tstest (green under the gate) and shares the M2-killed helper. - Two different runtime roots in one daemon (E2E). Reachable in production (per-workspace
resolveSessionRuntimeBaseDirinrun-qwen-serve.ts), but standing up a managed secondary runtime with its own root was out of budget. Proven by construction instead: cache key includespath.resolve(runtimeBaseDir)and mutant M4 (dropping it from the key) is killed by the isolation unit test (expected "spy" to be called 4 times, but got 3 times). - Direct latency / event-loop-lag measurement. The mechanism (87.3% fewer catalog file opens; zero opens on warm repeats) is proven; actual ms savings under thousands of sessions on a loaded runner were not measured — a perf number here would measure the shared CI box, not the PR.
- 50,000-summary retention cap under real load — unit-tested by the PR (eviction + reclaim + oversized-serve, all killed by M7-family mutations), not load-tested.
- Windows (PR author tested macOS only). This run is Linux, the previously-untested lane; the A/B and invalidation cells above are Linux evidence.
previous-report.md: none present — first round.
Methodology
Environment: CI verify container (node:22-bookworm, user node, no root, no strace — hence a gcc-compiled LD_PRELOAD openat/openat64 shim logging <monotonic_ns> <pid> <R|W> <path> filtered by chats-dir prefix; sources tmp/fscount.c). Head build = CI's npm run build at the merge commit; base control = git worktree add tmp/base-tree HEAD^1 + scripts/build_package.js for packages/cli only (wired to the already-installed root node_modules; realpath of @qwen-code/qwen-code-core asserted → head tree core, which the PR does not touch; base dist asserted free of persistedSessionListCache symbols, head dist asserted to contain them). Harness harness/ab-harness.mjs boots one daemon per arm (serve --port <free> --workspace <scratch ws> with isolated QWEN_RUNTIME_DIR/QWEN_HOME), seeds deterministic fixtures, and runs cold/warm/expire/numeric/session-info phases with per-phase fs-log windows; harness/inv-harness.mjs drives delete/archive/unarchive/metadata/failure-recovery on both arms. Mutation matrix: 10 single-point mutants applied as exact-string edits (9 guards — harness/M*.json specs, raw vitest output in harness/logs/mut-M*.log — plus one positive control inverting the pre-existing pinned-first ordering to prove the suite is live in this area), each run against its intended target test and restored afterward. Targeted gates: npx vitest run on the five changed suites in packages/cli (1338/1338 green: server.test.ts 913, transport.test.ts 299, multi-workspace-sessions.test.ts 101, live-task-service.test.ts 14, persisted-session-list-cache.test.ts 11). Raw logs live under harness/ and harness/logs/; evidence images: 01-ab-catalog-reads-head-vs-base.png, 02-invalidation-head-vs-base.png, 03-mutation-matrix.png.
Assertion ledger (41 scripted checks → assertions.json 40 pass / 1 fail)
| group | checks | result |
|---|---|---|
A/B (ab-assert.mjs: head cache cells, base control validity, parity, reduction, wire shape) |
17 | 17 pass |
Mutation freshness (inv-harness.mjs, 15 distinct assertions executed on both arms, identical outcomes) |
15 | 15 pass |
| Mutation matrix pinning (M1–M9 guards + positive control; each "target test catches mutant") | 10 | 9 pass, 1 fail (M5 — Finding #1) |
Targeted-gate totals (1338/1338) are cited in Methodology and not double-counted in the ledger. The M5 row's fail is the Finding #1 test-pinning gap: the shipped behavior is correct (my probe passes on clean source), but the PR's own test does not isolate the guard.
Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention).
— Qwen Code · sandboxed verification
|
@qwen-code /triage |
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.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not explored to full depth (tool budget reached): You are review agent reverse-audit — Reverse audit agen...: none — no checks left unfinished.; You are review agent reverse-audit — Reverse audit agen...: none — all checks completed within budget.; You are review agent reverse-audit — Reverse audit agen...: none — all checks above completed within budget.; You are review agent reverse-audit — Reverse audit agen...: none — all checks above were completed within budget.; You are review agent reverse-audit — Reverse audit agen...: none — all checks above reached a conclusion within budget., and 1 more.
Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 5.
中文说明
已审查。 建议见行内评论。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未探索到全部深度（达到工具调用预算）：You are review agent reverse-audit — Reverse audit agen...：none — no checks left unfinished.；You are review agent reverse-audit — Reverse audit agen...：none — all checks completed within budget.；You are review agent reverse-audit — Reverse audit agen...：none — all checks above completed within budget.；You are review agent reverse-audit — Reverse audit agen...：none — all checks above were completed within budget.；You are review agent reverse-audit — Reverse audit agen...：none — all checks above reached a conclusion within budget.，另有 1 条。
未审查：反向审计——在 5 轮的反审轮数上限内未收敛。
— qwen3.8-max via Qwen Code /review (v0.21.9)
| expect(newLookup.status).toBe('scan'); | ||
| oldLoad.resolve(snapshot(1)); | ||
| await oldLookup.promise; |
There was a problem hiding this comment.
[Suggestion] The install-side generation guard (current.generation === generation in the install condition, persisted-session-list-cache.ts ~104) is pinned by no test: both superseded-load tests perform a second cache.lookup() immediately after invalidate(), which replaces slot.inFlight before the old load settles, so the promise-identity check fails first and the generation conjunct is never consulted. — Failure scenario: probe-verified — removing the conjunct leaves this file (11/11) and server.test.ts (915/915) green. Reachable in production: loadAllPersistedSummaries pages up to 50k sessions plus worktree-sidecar reads, so a mutation invalidation landing mid-scan with no list request before completion installs the pre-mutation snapshot, and the next list within the 2 s TTL serves stale data (archived session in the active list, deleted session still present). A discriminating probe (invalidate mid-scan, old load resolves with no intervening lookup) fails on the mutant with expected 'cache_hit' to be 'scan'. Suggested fix: add a superseded-load test with no intervening lookup — lookup (scan starts) → invalidate() → old load resolves → next lookup must be 'scan'.
中文说明
安装侧的 generation 守卫（安装条件中的 current.generation === generation，persisted-session-list-cache.ts ~104）没有任何测试固定：两个「被取代 load」测试都在 invalidate() 之后立即执行第二次 cache.lookup()，这会在旧 load settle 之前替换掉 slot.inFlight，因此 promise 身份检查会先失败，generation 条件分支根本不会被执行到。——失败场景：已用探针验证——删除该条件后本文件（11/11）与 server.test.ts（915/915）仍然全绿。生产可达：loadAllPersistedSummaries 分页扫描最多 5 万个会话并逐个读取 worktree sidecar，若 mutation 的失效落在扫描中途、且扫描完成前没有新的 list 请求，则会安装变更前的快照，随后 2 秒 TTL 内的 list 将返回过期数据（active 列表出现已归档会话、已删除会话仍然存在）。判别探针（扫描中途 invalidate、旧 load 在无中间 lookup 时 resolve）在突变体上以 expected 'cache_hit' to be 'scan' 失败。建议修复：新增一个无中间 lookup 的被取代 load 测试——lookup（开始扫描）→ invalidate() → 旧 load resolve → 下一次 lookup 必须为 'scan'。
— qwen3.8-max via Qwen Code /review (v0.21.9)
| const lookup = cache.lookup(SCOPE, () => reload.promise); | ||
| expect(lookup.status).toBe('scan'); | ||
|
|
||
| reload.resolve(snapshot()); | ||
| await lookup.promise; |
There was a problem hiding this comment.
[Suggestion] This read-path TTL test ('rejects an installed value when its read-path age reaches the TTL') pins only the expiry decision, not the teardown that accompanies it: dropping the read-path this.removeValue(slot) call and proceeding straight to rescan leaves all 11 tests green — the reload's installValue overwrites slot.value and increments retainedSummaries without the matching decrement, and the trailing cache.clear() masks the residual state. — Failure scenario: probe-verified — under the mutant a mechanism probe (cap 9, three scopes, one read-path expiry+reinstall) shows the leaked count persists (evictFor evicts a healthy in-TTL scope) while shipped code keeps it; on a long-lived daemon listing a hot workspace at >2 s intervals each expired→reinstall cycle leaks its session count permanently, and once drift exceeds the 50,000 cap every install evicts all other scopes' healthy snapshots, collapsing the cache into a full rescan per list request. Production code is correct; the suite cannot catch a teardown-removing regression. Suggested fix: use a small maxRetainedSummaries, install a second scope after the expiry/reinstall, and assert both scopes are still cache_hit — the inflated counter under the mutant would evict one of them.
中文说明
这个读路径 TTL 测试（'rejects an installed value when its read-path age reaches the TTL'）只固定了过期判定，没有固定伴随的清理动作：删除读路径上的 this.removeValue(slot) 调用并直接重新扫描，全部 11 个测试仍为绿——reload 的 installValue 会覆盖 slot.value 并在没有对应扣减的情况下增加 retainedSummaries，结尾的 cache.clear() 又掩盖了残留状态。——失败场景：已用探针验证——在该突变体下，机制探针（容量 9、三个 scope、一次读路径过期+重装）显示泄漏计数持续存在（evictFor 会逐出一个仍在 TTL 内的健康 scope），而现有代码不会；在长期运行、以超过 2 秒间隔重复列出同一热点 workspace 的守护进程中，每次过期→重装都会永久泄漏其会话计数，漂移超过 50,000 容量后每次安装都会逐出所有其他 scope 的健康快照，缓存退化为每次 list 都全量重扫。生产代码本身是正确的，问题在于测试套件无法捕获「删除清理逻辑」这类回归。建议修复：使用较小的 maxRetainedSummaries，在过期/重装后再安装第二个 scope，并断言两个 scope 仍为 cache_hit——突变体下膨胀的计数会逐出其中之一。
— qwen3.8-max via Qwen Code /review (v0.21.9)
| it('evicts the oldest retained snapshot to honor the global cap', async () => { | ||
| vi.useFakeTimers(); | ||
| const cache = new PersistedSessionListCache(10_000, 2); |
There was a problem hiding this comment.
[Suggestion] The eviction tests never hold more than one valued snapshot at the moment eviction runs, so neither the oldest-first victim-selection rule nor the multi-round while loop in evictFor is discriminated — any arbitrary single-eviction policy passes. — Failure scenario: probe-verified — inverting the selection comparison to newest-first leaves all 11 tests green, and replacing the while with an if (single-pass eviction) also leaves all 11 green (liveness check: disabling eviction entirely fails the cap test, so the file can fail — it just cannot discriminate these mutants). The production singleton runs cap 50,000, unreachable by integration tests; a newest-first or single-pass regression degrades hot workspaces to full disk scans per list request once retained summaries exceed the cap. Suggested fix: restructure so ≥2 valued snapshots coexist when eviction fires and assert which one survives; note a cap-3/three-install shape catches only the selection mutant — discriminating the while loop additionally requires forcing two eviction rounds in one install (e.g. cap 5: A(2), B(2), C(1), then D(4)).
中文说明
逐出（eviction）测试在触发逐出的时刻从未同时持有多于一个带值快照，因此「最旧优先」的选择规则和 evictFor 的多轮 while 循环都没有被区分——任意单轮逐出策略都能通过。——失败场景：已用探针验证——把选择比较反转为「最新优先」后 11 个测试仍全绿；把 while 换成 if（单轮逐出）同样全绿（活性检查：完全禁用逐出会让容量测试失败，说明该文件可以失败——只是无法区分这两个突变体）。生产环境的单例容量为 50,000，集成测试无法触及；一旦保留摘要数超过容量，「最新优先」或单轮逐出回归会使热点 workspace 退化为每次 list 都全量磁盘扫描。建议修复：重构测试使逐出触发时至少有 2 个带值快照并存，并断言哪一个存活；注意 cap=3/三次安装的形态只能捕获选择突变体——要区分 while 循环还需迫使一次安装触发两轮逐出（例如 cap 5：A(2)、B(2)、C(1)，再安装 D(4)）。
— qwen3.8-max via Qwen Code /review (v0.21.9)
| it('uses a non-sliding TTL measured from load completion', async () => { | ||
| vi.useFakeTimers(); | ||
| const cache = new PersistedSessionListCache(2_000, 50_000); |
There was a problem hiding this comment.
[Suggestion] This test is named 'uses a non-sliding TTL measured from load completion', but it cannot distinguish a scan-start TTL baseline from a load-completion baseline: every loader in this file resolves within microseconds of scan start, so both baselines produce identical cacheAgeMs values in every test. — Failure scenario: probe-verified — a mutant recording the TTL epoch at lookup/scan start instead of completion passes all 11 tests; a scan taking longer than the 2 s TTL would then install an already-expired snapshot (read-path age check fails immediately, expiry timer scheduled with delay 0), so every session-list request rescans and the cache silently never caches. The discriminating probe (5 s load vs 2 s TTL, then expect cache_hit) passes on real code and fails on the mutant. Suggested fix: use a deferred loader, advance time past half the TTL while the load is still in flight, then resolve it and assert cache_hit still holds afterward — under a scan-start baseline the remaining TTL would be exhausted and the assertion would fail.
中文说明
该测试名为 'uses a non-sliding TTL measured from load completion'（TTL 从加载完成起算、不滑动），但它无法区分「从扫描开始起算」与「从加载完成起算」两种 TTL 基准：文件中所有 loader 都在扫描开始后微秒级内 resolve，因此两种基准在每个测试中产生完全相同的 cacheAgeMs。——失败场景：已用探针验证——把 TTL 纪元改为在 lookup/扫描开始时记录的突变体可通过全部 11 个测试；此时一次耗时超过 2 秒 TTL 的扫描会安装一个已过期的快照（读路径年龄检查立即失败、过期定时器以 0 延迟调度），导致每次 session list 都重新扫描，缓存实际上永远不生效。判别探针（5 秒 load 对比 2 秒 TTL，随后期望 cache_hit）在真实代码上通过、在突变体上失败。建议修复：使用延迟 loader，在 load 仍在进行时推进时间超过 TTL 的一半，再 resolve 并断言其后仍为 cache_hit——若基准是扫描开始，则剩余 TTL 已耗尽，断言会失败。
— qwen3.8-max via Qwen Code /review (v0.21.9)
| try { | ||
| return await mutation(); | ||
| } finally { | ||
| invalidateSessionLists(runtime, archiveStates); | ||
| } |
There was a problem hiding this comment.
[Suggestion] The invalidate-even-when-the-mutation-rejects (try/finally) semantics of runWithSessionListInvalidation (mirrored in acp-http/dispatch.ts) are untested — no test makes a mutation actually reject; the closest tests throw SessionNotFoundError, which the flows treat as benign and still resolve 200. — Failure scenario: probe-verified — moving invalidation to the success path only leaves all 1315 tests green (server.test.ts 915 + multi-workspace + transport 299), while a comparator mutant removing invalidation entirely fails 3 of the 4 new invalidation tests, so the harness is alive. Real rejection paths exist (409 gate races, DaemonDrainingError, non-benign close failures on the single-session close route) and carry no invalidation assertion; a failed batch would return 500 and leave the warm catalog serving already-removed sessions for up to 2 s. Suggested fix: add a test where the wrapped mutation rejects with a non-benign error, then assert the subsequent organized list rescans instead of hitting the warm snapshot (e.g. via a SessionService.prototype.listSessions spy count).
中文说明
runWithSessionListInvalidation（acp-http/dispatch.ts 中有镜像实现）的「mutation 失败时也同样失效缓存」（try/finally）语义没有被测试——没有任何测试让 mutation 真正 reject；最接近的测试抛出 SessionNotFoundError，而流程把它视为良性错误，仍然返回 200。——失败场景：已用探针验证——把失效逻辑移到仅成功路径后，全部 1315 个测试仍为绿（server.test.ts 915 + multi-workspace + transport 299）；而完全移除失效逻辑的对照突变体会使 4 个新失效测试中的 3 个失败，说明测试装置是有效的。真实的 rejection 路径确实存在（409 门控竞态、DaemonDrainingError、单会话 close 路由上的非良性 close 失败），且都没有失效断言；失败的批量操作会返回 500，并使热缓存在最长 2 秒内继续返回已被移除的会话。建议修复：新增一个让被包裹 mutation 以非良性错误 reject 的测试，然后断言随后的 organized 列表是重新扫描而非命中热快照（例如通过 SessionService.prototype.listSessions 的 spy 计数）。
— qwen3.8-max via Qwen Code /review (v0.21.9)
| expect(setAttribute).toHaveBeenCalledWith( | ||
| 'qwen-code.daemon.session_list.scan_duration_ms', | ||
| expect.any(Number), | ||
| ); |
There was a problem hiding this comment.
[Suggestion] The leader/waiter telemetry test mocks trace.getSpan to return ONE shared {setAttribute} spy for all four requests, so only the union of attributes across requests is observable; consequently the if (lookup.status === 'scan') guard around scan_duration_ms in listAllPersistedSummaries (session-list.ts:499-504) is pinned by no test. — Failure scenario: probe-verified — deleting the status guard so scan_duration_ms is recorded on every lookup leaves this test green (the genuine rescan satisfies the shared spy), while an emission-count assertion fails on the mutant (length of 1 but got 2) — a cache_hit request emits the cached snapshot's original scan duration as its own, polluting scan-latency metrics with hit traffic. Distinct from R1-3 (attribute presence, fixed) and the rejected round-2 failure-phase attribution claim. Suggested fix: mock getSpan to return a fresh setAttribute spy per call (captured in order) and assert per-request attribute sets — the scan request's spy received scan_duration_ms while the cache-hit request's spy did not.
中文说明
leader/waiter 遥测测试把 trace.getSpan mock 为对全部四个请求返回同一个共享的 {setAttribute} spy，因此只能观察到跨请求的属性并集；于是 listAllPersistedSummaries 中围绕 scan_duration_ms 的 if (lookup.status === 'scan') 守卫（session-list.ts:499-504）没有任何测试固定。——失败场景：已用探针验证——删除该状态守卫使每次 lookup 都记录 scan_duration_ms 后，本测试仍为绿（真正的重新扫描满足了共享 spy）；而发射次数断言在突变体上失败（length of 1 but got 2）——cache_hit 请求会把缓存快照原始的扫描耗时当作自己的发出，用命中流量污染扫描延迟指标。与 R1-3（属性存在性，已修复）及第二轮被驳回的失败阶段归属问题均不相同。建议修复：把 getSpan mock 为每次调用返回一个新的 setAttribute spy（按顺序捕获），并按请求断言属性集合——scan 请求的 spy 收到 scan_duration_ms，而 cache_hit 请求的 spy 没有。
— qwen3.8-max via Qwen Code /review (v0.21.9)
| conn.closingSessions.delete(sessionId); | ||
| this.invalidateSessionLists(['active']); |
There was a problem hiding this comment.
[Suggestion] R1-1 (round-1 finding, re-verified this round, still standing): the ACP session/close and _qwen/session/update_metadata invalidation call sites have no over-the-wire test — the new transport test covers only _qwen/sessions/{archive,unarchive,delete}. The round-1 deferral is acknowledged; re-reporting because the gap is unchanged at this commit. — Failure scenario: re-verified by probe this round — deleting both invalidateSessionLists(['active']) call sites leaves all 346 ACP tests green (transport.test.ts + workspace-qualified-acp.test.ts). An ACP client that closes a session (or updates its metadata) and re-lists within two seconds would then be served the stale pre-mutation persisted snapshot — the regression the REST-side close/metadata tests were written to catch. Suggested fix: extend the transport test (or add a sibling) to warm an organized session/list, run session/close and _qwen/session/update_metadata, and assert the immediate re-list reflects the post-mutation state without asserting synchronous title durability.
中文说明
R1-1（第一轮发现，本轮重新验证后仍然存在）：ACP session/close 与 _qwen/session/update_metadata 的失效调用点没有 over-the-wire 测试——新的 transport 测试只覆盖了 _qwen/sessions/{archive,unarchive,delete}。已知悉第一轮的延期决定；因该缺口在本提交仍未变化，故再次报告。——失败场景：本轮探针重新验证——删除两处 invalidateSessionLists(['active']) 调用点后，全部 346 个 ACP 测试仍为绿（transport.test.ts + workspace-qualified-acp.test.ts）。ACP 客户端关闭会话（或更新其 metadata）后两秒内重新 list，将得到变更前的过期持久化快照——这正是 REST 侧 close/metadata 测试要防范的回归。建议修复：扩展 transport 测试（或新增同级测试），先预热 organized session/list，再执行 session/close 与 _qwen/session/update_metadata，断言紧随其后的重新 list 反映变更后状态，但不要求标题同步落盘。
— qwen3.8-max via Qwen Code /review (v0.21.9)
| addDaemonRequestAttribute( | ||
| 'qwen-code.daemon.session_list.cache_status', | ||
| lookup.status, | ||
| ); |
There was a problem hiding this comment.
[Suggestion] R1-11 (round-1 finding, still standing): the span attributes emitted from listAllPersistedSummaries use setAttribute (overwrite semantics via addDaemonRequestAttribute → trace.getSpan(...)?.setAttribute, daemon-tracing.ts:189), so a request performing multiple catalog lookups records only the last lookup's values. The round-1 deferral is acknowledged — correcting it requires an observability design (events or child spans with aggregation semantics) outside this cache PR; re-reporting only to keep the thread open for that follow-up. — Failure scenario: a daemon request that hits several catalog scopes (multiple archive states or workspaces in one handler) emits span attributes describing only the final lookup — metrics/traces attributed to the request misrepresent the earlier lookups' cache status, age, and scan statistics.
中文说明
R1-11（第一轮发现，仍然存在）：listAllPersistedSummaries 发出的 span 属性使用 setAttribute（经由 addDaemonRequestAttribute → trace.getSpan(...)?.setAttribute 的覆盖语义，daemon-tracing.ts:189），因此执行多次目录 lookup 的请求只会记录最后一次 lookup 的值。已知悉第一轮的延期决定——修正需要本缓存 PR 之外的可观测性设计（事件或带聚合语义的子 span）；再次报告仅为保留该后续事项的线程。——失败场景：命中多个目录 scope 的守护进程请求（单个处理器中涉及多个归档状态或 workspace）发出的 span 属性只描述最后一次 lookup——归属于该请求的指标/链路会错误呈现先前 lookup 的缓存状态、年龄与扫描统计。
— qwen3.8-max via Qwen Code /review (v0.21.9)
| } else if (current.value === undefined) { | ||
| this.slots.delete(key); | ||
| } |
There was a problem hiding this comment.
[Suggestion] R1-5 (round-1 finding, re-measured this round, still standing): test-efficacy probe (harness validated) — forcing the current.value === undefined guard to true leaves every test green; no test pins the case where the guard must NOT fire. The round-1 pushback was checked against the code this round and is architecturally sound: installs happen only through the identity-checked success handler, so a superseded load cannot reach this branch after a newer value installs — the guard is defensive cleanup, not a reachable public state transition. Re-reporting the probe fact so the thread can be closed deliberately. — Failure scenario: as filed, with the guard broken an old-generation or oversized load resolving while a newer value is installed would delete the slot and its valid snapshot, orphaning the expiry timer and skewing retained-summary accounting — with no failing test; the transition is unreachable today, so practical risk is limited to future refactors of the install path. Suggested fix: either add the round-1 suggested test (install a value, then resolve a superseded/oversized in-flight load against the same key and assert the lookup is still cache_hit), or accept the guard as defensive and close the thread.
中文说明
R1-5（第一轮发现，本轮重新测量后仍然存在）：测试有效性探针（harness 已验证）——把 current.value === undefined 守卫强制为 true 后所有测试仍全绿；没有测试固定该守卫「不应触发」的情形。本轮对照代码核查了第一轮的反馈，其架构论证成立：安装只发生在经过身份检查的成功处理路径中，因此在更新的值已安装后，被取代的 load 不可能进入该分支——该守卫是防御性清理，而非可达的公开状态转换。重新报告探针事实，以便有意识地关闭该线程。——失败场景：如原报告所述，守卫被破坏时，旧一代或超大 load 在更新值已安装时 resolve 会删除 slot 及其有效快照，使过期定时器成为孤儿、污染 retained-summary 计数——且没有测试失败；该转换目前不可达，实际风险仅限于未来对安装路径的重构。建议修复：要么新增第一轮建议的测试（先安装一个值，再让同 key 上被取代/超大的在飞 load resolve，断言 lookup 仍为 cache_hit），要么接受该守卫为防御性代码并关闭线程。
— qwen3.8-max via Qwen Code /review (v0.21.9)
| this.slots.delete(key); | ||
| } | ||
| } | ||
| return snapshot; |
There was a problem hiding this comment.
[Suggestion] R1-6 (round-1 finding, re-measured this round, still standing): test-efficacy probe (harness validated) — deleting the added this.slots.delete(key); cleanup leaves every test green; no test asserts that empty slots are removed from the slots map after a non-retained result. The round-1 pushback stands: a follow-up lookup is scan in both cases, so there is no public behavioral discriminator without exposing private Map state. — Failure scenario: with the delete removed, every scope whose load completed without installing (oversized snapshot, generation bumped mid-flight) leaves a permanently empty slot; across many distinct scopes the map grows without bound — a slow leak no test in this diff detects. Cost is bounded by the distinct-scope space (workspaces × 2 archive states) and each entry is tiny, so this is retention hygiene rather than a correctness risk. Suggested fix: either expose a minimal test seam for slot-map size, or accept the bounded hygiene cost and close the thread.
中文说明
R1-6（第一轮发现，本轮重新测量后仍然存在）：测试有效性探针（harness 已验证）——删除新增的 this.slots.delete(key); 清理语句后所有测试仍全绿；没有测试断言未保留结果的 load 完成后空 slot 会从 slots map 中移除。第一轮的反馈成立：后续 lookup 在两种情况下都是 scan，若不暴露私有 Map 状态就不存在公开的行为判别依据。——失败场景：若删除该语句，任何 load 完成但未安装值的 scope（超大快照、飞行中被提升 generation）都会留下永久为空的 slot；在大量不同 scope 下 map 无限增长——这是本 diff 中没有任何测试能检测的缓慢泄漏。成本受限于不同 scope 的数量空间（workspace 数 × 2 种归档状态）且每个条目极小，因此属于保留卫生问题而非正确性风险。建议修复：要么为 slot-map 大小暴露一个最小测试接缝，要么接受这一有界的卫生成本并关闭线程。
— qwen3.8-max via Qwen Code /review (v0.21.9)
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. ✅
|
Sandboxed verification: Skipped because the PR is not open for verification (state=MERGED, draft=false). 中文 — 判定：
|
|
Released in v0.21.10. |
3 participants
What this PR does
This PR adds a process-local, two-second single-flight cache for the persisted session catalogs used by organized and metadata-filtered daemon session lists. Catalogs are isolated by resolved runtime root, exact workspace identity, and archive state; persisted JSONL summaries and worktree sidecars are cached, while live bridge state, organization metadata, filtering, sorting, and pagination remain request-local.
Explicit REST and ACP metadata, close, delete, archive, and unarchive operations invalidate the affected catalog states before their responses are sent. The implementation also pins each read to the selected workspace runtime, adds cache status telemetry for scans, waiters, hits, and failures, and keeps default numeric pagination and the session-info disk scan on their existing paths.
Why it's needed
Organized, pinned, source-filtered, and LiveTask session lists currently repeat the same full JSONL and worktree-sidecar scan when several requests arrive together. On workspaces with hundreds or thousands of sessions, those duplicate synchronous filesystem reads increase route latency and event-loop lag without producing different persisted data.
Sharing only the short-lived persisted catalog removes redundant work while preserving current live and organization freshness, runtime isolation, cursor behavior, truncation semantics, and public REST/ACP response shapes.
Reviewer Test Plan
How to verify
Start a daemon with an isolated runtime directory and a workspace containing persisted sessions. Send five concurrent organized-all, organized-pinned, parent/source-filtered, and LiveTask-style requests for the same active workspace; confirm that one request performs the catalog scan, the other requests join it, and all responses retain the existing wire shape.
Repeat the requests within two seconds and confirm that no JSONL or worktree sidecar is read again. Change live client/prompt/title state and organization pin/group state between requests and confirm that the next response reflects those changes without waiting for cache expiry.
Exercise metadata update, close, delete, archive, and unarchive through REST and ACP, then list the affected active and archived catalogs before the two-second TTL expires. Confirm that each mutation exposes the new state immediately, failed loaders do not serve stale data, and identical workspace paths under different runtime roots never share results.
Confirm that default numeric cursor pagination still calls storage with the requested page size and that session-info remains an independent disk scan.
Evidence (Before & After)
N/A — this is a daemon performance and consistency change with no TUI or Web Shell UI change.
Tested on
Environment (optional)
macOS arm64, Node.js v22.22.3, npm 10.9.8, source daemon plus global qwen 0.21.8 baseline, temporary
QWEN_RUNTIME_DIRand temporary workspaces only.Risk & Scope
Linked Issues
N/A
中文说明
此 PR 的作用
此 PR 为 organized 和 metadata 过滤型 daemon session list 使用的持久化会话目录增加进程内、两秒 TTL 的 single-flight 缓存。目录按解析后的 runtime root、精确 workspace identity 和 archive state 隔离；持久化 JSONL 摘要与 worktree sidecar 会被缓存，而 live bridge 状态、organization metadata、过滤、排序和分页仍在每次请求中独立计算。
显式 REST 与 ACP metadata、close、delete、archive 和 unarchive 操作会在响应发送前失效受影响的目录状态。实现还将每次读取固定到选中的 workspace runtime，增加 scan、waiter、hit 和 failure 的缓存状态 telemetry，并让默认数字游标分页与 session-info 磁盘扫描继续使用现有路径。
为什么需要此改动
organized、pinned、source-filtered 和 LiveTask session list 在多个请求同时到达时，目前会重复执行相同的完整 JSONL 与 worktree sidecar 扫描。在包含数百或数千个 session 的 workspace 中，这些重复的同步文件系统读取会增加路由延迟和 event-loop lag，却不会产生不同的持久化数据。
仅共享短生命周期的持久化目录可以消除冗余工作，同时保持现有 live 与 organization 新鲜度、runtime 隔离、cursor 行为、truncation 语义以及公开 REST/ACP response shape 不变。
Reviewer 测试计划
验证方法
使用隔离的 runtime directory 启动 daemon，并准备一个包含持久化 session 的 workspace。针对同一个 active workspace 并发发送五个 organized-all、organized-pinned、parent/source-filtered 和 LiveTask 风格请求；确认只有一个请求执行目录扫描，其余请求加入该扫描，并且所有响应保持现有 wire shape。
在两秒内重复这些请求，确认不会再次读取 JSONL 或 worktree sidecar。在两次请求之间修改 live client/prompt/title 状态以及 organization pin/group 状态，确认下一个响应无需等待缓存过期即可反映这些变化。
通过 REST 和 ACP 执行 metadata update、close、delete、archive 和 unarchive，然后在两秒 TTL 到期前列出受影响的 active 与 archived 目录。确认每个 mutation 都立即暴露新状态，失败的 loader 不会提供 stale data，并且不同 runtime root 下相同 workspace path 永远不会共享结果。
确认默认数字游标分页仍使用请求的 page size 调用 storage，并且 session-info 仍是独立的磁盘扫描。
证据（改动前后）
N/A——这是 daemon 性能与一致性改动，不包含 TUI 或 Web Shell UI 变化。
测试平台
环境（可选）
macOS arm64、Node.js v22.22.3、npm 10.9.8、源码 daemon 与全局 qwen 0.21.8 基线；仅使用临时
QWEN_RUNTIME_DIR和临时 workspace。风险与范围
关联 Issue
N/A