fix(cli): resolve session-management settings per request, not from the stale this.settings cache - #10095
fix(cli): resolve session-management settings per request, not from the stale this.settings cache#10095tomsen02 wants to merge 3 commits into
Conversation
…he stale this.settings cache In a multi-workspace daemon, unstable_listSessions, deleteSession and the dead-session renameSession path derived the runtime output root from this.settings — the agent-level "latest loaded" cache — so after another workspace's activity they operated on the wrong runtime root: listSessions returned an empty/foreign list, deleteSession silently no-oped (or could remove a stale same-id copy under the wrong root), and renameSession mis-targeted the same way. The misrouting persists until the requesting workspace's settings happen to be reloaded. Load settings per request with loadSettingsCached(cwd), mirroring the pattern the sessionTranscript and session-restore handlers in the same file already use. Follow-up to QwenLM#6292, which fixed this class on the session-creation entry points and deliberately left this.settings in place for workspace-neutral agent-level readers — these three handlers were misfiled in that bucket. Fixes QwenLM#10094. Verification: three regression tests assert each handler passes the per-request settings instance (mutation-verified: reverting a call site to this.settings fails its test); acpAgent.test.ts 486/486; typecheck, ESLint and Prettier clean. Real-filesystem probe in the issue. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Thanks for the PR, @tomsen02! The write-up is thorough — the stale-cache mechanism is clearly explained, and issue #10094 carries a real-filesystem reproduction (BUG arm vs CONTROL arm), which is exactly the kind of evidence this gate looks for.
One thing blocks it at the gate: the PR body doesn't follow the PR template. Two gaps:
- The
## Reviewer Test Plansection is missing, including its### How to verify,### Evidence (Before & After), and### Tested onsubsections. Your## Reproductionand## Verificationcontent is good — please move it into those subsections: the probe output from #10094 (thefound=undefined, removed=falsevsfound=<uuid>, removed=truearms) belongs under Evidence (Before & After), the regression-test/mutation-verification notes under How to verify, and please fill in the Tested on OS table. - The
## Linked Issuesheading is missing —Fixes #10094is already in the body, it just needs to live under that heading.
Everything else is present (## What this PR does, ## Why it's needed, ## Risk & Scope, and the bilingual summary). Once the template sections are in place, re-run the review and it will continue.
中文说明
感谢贡献,@tomsen02!PR 描述写得很扎实——过期缓存机制讲得很清楚,issue #10094 里还有真实文件系统的复现(BUG 臂 vs 对照臂),正是评审所需要的证据。
目前卡在模板检查这一步:PR 正文没有遵循 PR 模板,有两处缺口:
- 缺少
## Reviewer Test Plan部分,包括其### How to verify、### Evidence (Before & After)、### Tested on子节。现有的## Reproduction和## Verification内容很好,请把它们移入对应子节:#10094 中的探针输出(found=undefined, removed=false与found=<uuid>, removed=true两臂)放进 Evidence (Before & After),回归测试/变异验证说明放进 How to verify,并请填写 Tested on 的操作系统表格。 - 缺少
## Linked Issues标题——Fixes #10094已经写在正文里,只需移到该标题下。
其余部分都已具备(## What this PR does、## Why it's needed、## Risk & Scope 以及中文摘要)。补齐模板各节后重新触发评审即可继续。
— Qwen Code · qwen3.8-max
|
PR body restructured to the template: @qwen-code /triage |
The round-1 gate blocked solely on PR-body template structure; the body now carries Reviewer Test Plan (How to verify / Evidence / Tested on) and Linked Issues. No code changes.
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.0)
| expect(runWithAcpRuntimeOutputDir).toHaveBeenCalledTimes(1); | ||
| expect(vi.mocked(runWithAcpRuntimeOutputDir).mock.calls[0]![0]).toBe( | ||
| perRequestSettings, | ||
| ); |
There was a problem hiding this comment.
[Suggestion] The three new regression tests pin the settings object but not the per-request cwd: the mocked loadSettings (and the passthrough loadSettingsCached) return perRequestSettings for any argument, and runWithAcpRuntimeOutputDir's second argument is never checked — the deleteSession and unstable_listSessions tests have no SessionService-cwd assertion either. A future regression that resolves settings from the wrong directory (e.g. loadSettingsCached(this.config.getTargetDir())), or drifts the routed cwd to the boot-workspace value, would re-introduce exactly the multi-workspace misrouting this PR fixes — while all three tests stay green. Verified by mutation probe at the reviewed commit: reverting the settings key or the routing cwd to the boot dir keeps all three tests passing (Tests 3 passed), and adding the assertions below turns both mutants red (Tests 3 failed). Pin the key here, and add the same two assertions to the renameSession and unstable_listSessions tests:
| expect(runWithAcpRuntimeOutputDir).toHaveBeenCalledTimes(1); | |
| expect(vi.mocked(runWithAcpRuntimeOutputDir).mock.calls[0]![0]).toBe( | |
| perRequestSettings, | |
| ); | |
| expect(runWithAcpRuntimeOutputDir).toHaveBeenCalledTimes(1); | |
| expect(vi.mocked(runWithAcpRuntimeOutputDir).mock.calls[0]![0]).toBe( | |
| perRequestSettings, | |
| ); | |
| expect(loadSettings).toHaveBeenCalledWith('/tmp/workspace-a'); | |
| expect(vi.mocked(runWithAcpRuntimeOutputDir).mock.calls[0]![1]).toBe( | |
| '/tmp/workspace-a', | |
| ); |
中文说明
三个新回归测试只固定了 settings 对象,没有固定按请求的 cwd:mock 的 loadSettings(以及直通式的 loadSettingsCached)对任何参数都返回 perRequestSettings,且 runWithAcpRuntimeOutputDir 的第二个参数从未被断言——deleteSession 与 unstable_listSessions 测试也没有对 SessionService 的 cwd 断言。未来若有回归把 settings 解析到错误目录(例如 loadSettingsCached(this.config.getTargetDir())),或把路由的 cwd 漂移到启动时 workspace 的值,将重新引入本 PR 所修复的多 workspace 错误路由——而三个测试仍全部为绿。已在被审提交上用变异探针验证:把 settings 键或路由 cwd 改回启动目录,三个测试依然通过(Tests 3 passed);加入上述两条断言后,两种变异均变红(Tests 3 failed)。请在此处固定键值,并在 renameSession 与 unstable_listSessions 测试中加入相同的两条断言。
— qwen3.8-max via Qwen Code /review (v0.22.0)
… tests
Round-1 R1-1: the three regression tests pinned the settings object but
not the directory it was resolved from or routed to — a regression that
resolves settings from the boot workspace's dir (or drifts the routed
cwd) would pass all three. Each test now also asserts
loadSettings('/tmp/workspace-a') and runWithAcpRuntimeOutputDir's cwd
argument, per the review's suggestion. Mutation-verified: rerouting
deleteSession through this.config.getTargetDir() fails its test.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
R1-1 addressed in Mutation-verified locally: rerouting deleteSession's settings/cwd through |
|
Status note: review converged (round on |
What this PR does
In a multi-workspace daemon, three session-management handlers —
unstable_listSessions,deleteSession, and the dead-sessionrenameSessionpath — derived the runtime output root fromthis.settings, the agent-level "latest loaded" cache, instead of the request's own workspace settings. This PR makes all three resolve settings per request withloadSettingsCached(cwd), the same pattern thesessionTranscriptand session-restore handlers in the same file already use.this.settingsstays in place for the genuinely agent-level readers, per #6292's design.Why it's needed
runWithAcpRuntimeOutputDirderives the runtime base dir fromsettings.merged.advanced.runtimeOutputDir, andSessionServicecaptures it at construction, so the chats directory it operates on is<runtimeBaseDir>/projects/<cwd>/chats/. The cache is refreshed by any workspace's activity, so after workspace B's session activity, a request for workspace A (whoseadvanced.runtimeOutputDirdiffers) operated on B's runtime root:listSessionsreturned an empty/foreign list for A,deleteSessionsilently returnedsuccess:falsefor a session that exists (or could remove a stale same-id copy under the wrong root and report success while the live session survives), and dead-sessionrenameSessionmis-targeted the same way. Not a transient race — the misrouting persists until A's settings happen to be reloaded.#6292 fixed exactly this class on the session-creation entry points and deliberately kept
this.settingsfor "agent-level readers … which keeps their behavior unchanged"; these three handlers were misfiled in that bucket, but they are not workspace-neutral — they derive the runtime root. (#4709/#4715 fixed the auto-memory variant of the same runtime-dir misrouting.)Reviewer Test Plan
How to verify
cd packages/cli && npx vitest run src/acp-integration/acpAgent.test.ts→ 486/486. The three new tests (resolves deleteSession settings per request…,resolves dead-session renameSession settings per request…,resolves unstable_listSessions settings per request…) boot the agent with one settings object, re-pointloadSettingsto a different per-request object, invoke each handler, and assert the per-request instance (not the boot-time cache) reachesrunWithAcpRuntimeOutputDir.this.settings— its test fails ("expected … to be … // Object.is equality"); the shipped code passes all three.Storage/SessionService/runWithAcpRuntimeOutputDiron a real filesystem (no mocks): seed a session for workspace A under A's configuredadvanced.runtimeOutputDir, then run find/remove once with workspace B's settings (the stale-cache shape) and once with A's own.Evidence (Before & After)
Probe output (from #10094), real filesystem, production modules:
The control arm runs after the bug arm, proving the session file had survived the misrouted delete. No user-visible TUI change.
Tested on
Environment (optional)
Unit tests via vitest from
packages/cli; probe viatsxagainst the built@qwen-code/qwen-code-coreon macOS (Darwin 25.4.0, Node 22).Risk & Scope
advanced.runtimeOutputDir(same default root either way);loadSettingsCachedis the same per-cwd cache the neighboring handlers already use, so no extra settings I/O on repeated calls.this.settingsreaders that are genuinely agent-level (auth persistence, fastModel, folder trust, language) — deliberately untouched per fix(acp): pass per-session settings explicitly instead of racing on this.settings #6292's design; a full-daemon two-workspace E2E (the module-level probe above covers the mechanism).Linked Issues
Fixes #10094.
Follow-up to #6292; same misrouting family as #4709/#4715.
中文说明
本 PR 做了什么:多 workspace daemon 下,三个会话管理入口(
unstable_listSessions、deleteSession、dead-session 的renameSession)用 agent 级"最后加载"缓存this.settings解析 runtime 输出根目录。本 PR 让三者改为按请求loadSettingsCached(cwd)解析——与同文件sessionTranscript等入口已有的模式一致;真正 agent 级的this.settings读者按 #6292 的设计保持不变。为什么需要:
runWithAcpRuntimeOutputDir从settings.merged.advanced.runtimeOutputDir派生 runtime 根目录,SessionService在构造时捕获它。缓存会被任何 workspace 的活动刷新,因此 workspace B 活动后,对 workspace A 的请求会操作 B 的 runtime 根:列表对 A 返回空/异 workspace 结果;删除对存在的会话静默返回success:false(或删掉错误根下的同 id 陈旧副本并报告成功而真会话幸存);重命名同样错靶。这不是瞬时竞态——错误路由持续到 A 的设置恰好再次被加载。#6292 已在会话创建入口修复同类问题并刻意为"agent 级读者"保留this.settings;这三个入口被误归入该桶,但它们并非 workspace 无关。(#4709/#4715 修复过同家族的 auto-memory 变体。)验证方法:1)
cd packages/cli && npx vitest run src/acp-integration/acpAgent.test.ts→ 486/486,三个新回归测试分别断言各入口把按请求的 settings 实例(而非启动时缓存)传给runWithAcpRuntimeOutputDir;2) 变异检查:任一调用点改回this.settings,对应测试变红;3) 可选端到端复现见 #10094 的真实文件系统探针(生产模块、无 mock)。Before/After 证据:Before(B 的过期设置 + A 的 cwd)
found=undefined, removed=false(A 的会话不可见且不可删);After(按请求的 A 设置,同一 cwd)found=<uuid>, removed=true。对照臂在 bug 臂之后运行,证明会话文件在错误路由的删除下幸存。无用户可见 TUI 变化。测试平台:macOS 已测(Darwin 25.4.0,Node 22);Windows/Linux 未本地测试,交由 CI。
风险与范围:单 workspace daemon 与未配置
advanced.runtimeOutputDir的 workspace 行为不变;loadSettingsCached为按 cwd 缓存,无额外 I/O。未验证:真正 agent 级的其它this.settings读者(按 #6292 设计刻意不动);完整双 workspace daemon E2E(模块级探针已覆盖机制)。无破坏性变更。关联 Issue:Fixes #10094;#6292 的后续;与 #4709/#4715 同一错误路由家族。
🤖 Generated with Claude Code