fix(acp): pass per-session settings explicitly instead of racing on this.settings - #6292
Merged
Conversation
…his.settings ACP session handlers run concurrently, and session creation awaits config load, MCP discovery, and auth refresh between "load settings" and "construct Session". Two reads of the shared mutable `this.settings` race across that window: - `createAndStoreSession` constructed `Session` with whatever instance the most recent handler loaded, so a slow session creation could bind another workspace's LoadedSettings — which Session persists model changes through, writing into the wrong workspace's settings.json. - `loadSession`/`unstable_resumeSession` ran their existence check under the previous handler's `advanced.runtimeOutputDir`, producing spurious "session not found" errors across workspaces. Each handler now loads its workspace's settings once at the top and threads that instance through `newSessionConfig` and `createAndStoreSession`; `this.settings` remains a "latest loaded" cache for agent-level readers. 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a concurrency race in ACP JSON-RPC session entry points by loading LoadedSettings once per request and explicitly threading that instance through session config creation and Session construction, instead of relying on the shared mutable this.settings cache across long awaits.
Changes:
- Load per-request
LoadedSettingsat the top ofnewSession,loadSession, andunstable_resumeSession, and pass it explicitly intonewSessionConfigandcreateAndStoreSession. - Update
newSessionConfigto accept a requiredLoadedSettingsinstance and ensure hook accessors + disabled-skill provider close over that same instance. - Add a regression test that interleaves two concurrent
newSessioncalls and asserts eachSessionis constructed with the correct workspace’sLoadedSettingsinstance.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| packages/cli/src/acp-integration/acpAgent.ts | Threads per-request LoadedSettings through session creation to eliminate races on this.settings during concurrent handlers. |
| packages/cli/src/acp-integration/acpAgent.test.ts | Adds an interleaving regression test to verify concurrent newSession calls don’t mix workspace settings instances. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+1438
to
+1460
| await vi.waitFor(() => | ||
| expect(vi.mocked(loadCliConfig)).toHaveBeenCalledTimes(1), | ||
| ); | ||
| await agent.newSession({ cwd: '/workspace-b', mcpServers: [] }); | ||
| releaseSessionA(); | ||
| await sessionAPromise; | ||
|
|
||
| expect(vi.mocked(loadSettings)).toHaveBeenCalledWith('/workspace-a'); | ||
| expect(vi.mocked(loadSettings)).toHaveBeenCalledWith('/workspace-b'); | ||
|
|
||
| const sessionCalls = vi.mocked(Session).mock.calls; | ||
| expect(sessionCalls).toHaveLength(2); | ||
| // Session B finished first while A was still mid-creation. | ||
| expect(sessionCalls[0]![0]).toBe('session-b'); | ||
| expect(sessionCalls[0]![3]).toBe(settingsB); | ||
| // Session A must still be constructed with workspace A's settings, not | ||
| // with the instance session B loaded in the meantime. | ||
| expect(sessionCalls[1]![0]).toBe('session-a'); | ||
| expect(sessionCalls[1]![3]).toBe(settingsA); | ||
|
|
||
| mockConnectionState.resolve(); | ||
| await agentPromise; | ||
| }); |
Collaborator
|
(Re-run triggered by @wenshao — PR updated with follow-up commit Thanks for the PR! Template looks good ✓ Problem: Observed concurrency bug with solid evidence. The interleaving regression test stalls session A at Direction: Aligned. Multi-workspace ACP daemon correctness is core to the session-management roadmap. The race window (config load + MCP discovery + auth refresh = hundreds of ms to seconds) is realistic. Related to #6263's shared-agent-state investigation. Approach: Scope is tight — two files, 127 additions / 23 deletions, purely the fix and its regression test. The explicit settings-threading pattern is minimal: load once at handler top, pass down. One observation from the initial run (not a blocker): Moving on to code review. 🔍 中文说明（由 @wenshao 触发的重新运行——PR 已更新，包含后续提交 感谢贡献！ 模板完整 ✓ 问题： 有充分证据的并发 bug。交错回归测试在 方向： 对齐。多工作区 ACP daemon 正确性是 session-management 路线图的核心。竞争窗口现实。与 #6263 相关。 方案： 范围紧凑——两个文件，127 增 / 23 删。后续提交 进入代码审查 🔍 — Qwen Code · qwen3.7-max |
Collaborator
|
(Re-run — PR updated with follow-up commit Code ReviewIndependent proposal (before reading the diff): Thread a per-request Diff comparison: The implementation matches the independent proposal exactly, including the follow-up commit's refinement. Key decisions verified in source:
All remaining TestingUnit TestsTypecheck ( Real-Scenario Test (tmux)This is an ACP-protocol-level concurrency race — not reproducible via CLI prompts. The tmux test verifies basic sanity (no regressions in normal usage): CLI builds and runs normally. No functional regression observed. 中文说明代码审查独立方案： 穿入请求级 Diff 对比： 实现与独立方案完全一致，包括后续提交的改进。所有关键决策已在源码中验证。未发现正确性 bug、安全漏洞或回归。 测试181/181 通过。Typecheck 清洁。CLI 正常运行。 — Qwen Code · qwen3.7-max |
Contributor
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
Collaborator
|
(Re-run — PR updated with follow-up commit This is a clean, well-understood concurrency fix with a tight regression test. The follow-up commit makes it even better. Reflection: The author clearly knows this codebase deeply. The My independent proposal was identical to what the PR does, including the post-check adoption. The interleaving test is genuinely good engineering: it stalls one session at the exact point where the race window opens and verifies instance identity via Scope is minimal — every line in the diff serves the fix. The Verdict: All three stages clean. Problem is real and reproduced, implementation matches the obvious correct approach, follow-up commit strengthens it, tests pass (181/181), typecheck/lint clean, CLI runs fine. Ship it. 中文说明这是一个干净、理解透彻的并发修复，配有紧凑的回归测试。后续提交使其更加完善。 反思： 作者对代码库有深入理解。后续提交展示了真正的纪律性——仅在存在性检查通过后才采纳缓存，意味着本 PR 相对于 判定： 三个阶段全部清洁。合并。 — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
previously approved these changes
Jul 4, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
A failed loadSession/unstable_resumeSession probe (stale id, different cwd) must not repoint the agent-level `this.settings` cache at the failed request's workspace — readers like `authenticate` and provider ext-methods would otherwise pick it up. The existence check itself only needs the local per-request instance, so move the cache adoption after the 404 throw, restoring the pre-fix cache timing exactly. 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)
Collaborator
Author
|
Follow-up 7cc2be9 (from a local Codex review pass): 🤖 Generated with Qwen Code |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Downgraded from Approve to Comment: CI still running.
The session-construction race is correctly fixed — the per-request LoadedSettings threading through newSessionConfig and createAndStoreSession is sound, and the interleaving test is well-designed.
Needs human review: No concurrency/interleaving tests were added for loadSession or unstable_resumeSession. The same per-request settings threading pattern is applied to all three handlers, but only newSession has a regression test. Consider adding analogous interleaving tests for the other two handlers to guard against future regressions.
— qwen3.7-max via Qwen Code /review
| // creation from picking up whichever workspace loaded last — Session | ||
| // persists model changes through this instance, so a mix-up writes to | ||
| // another workspace's settings.json. | ||
| const settings = loadSettings(cwd); |
Collaborator
There was a problem hiding this comment.
[Suggestion] The session-construction path is correctly fixed, but this.settings = settings still mutates the shared field here (and in loadSession / unstable_resumeSession). Several agent-level handlers that can run concurrently with session creation read this.settings directly — e.g. unstable_listSessions, unstable_deleteSession, qwen/providers/list, qwen/providers/connect, qwen/settings/getPath. Under concurrent sessions with different cwd values, these readers can observe another workspace's settings.
The providers/connect path is the most concerning: it writes credentials through this.settings, so a concurrent overwrite could redirect persistence to the wrong workspace's settings file.
The PR description acknowledges this as a known limitation, and the most damaging symptom (Session writing model changes to the wrong workspace) is fixed. Consider a follow-up that either removes this.settings writes entirely (letting each consumer load its own per-request instance) or documents the residual race at each remaining this.settings read site.
— qwen3.7-max via Qwen Code /review
wenshao
approved these changes
Jul 4, 2026
wenshao
left a comment
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
Fix correctly addresses the race condition where concurrent session creation calls overwrite this.settings. Per-request settings threading through newSessionConfig and createAndStoreSession is clean and well-tested. LGTM ✅
— qwen3.7-max via Qwen Code /review
doudouOUC
enabled auto-merge
July 4, 2026 03:33
Collaborator
|
@qwen-code /triage |
qwen-code-ci-bot
approved these changes
Jul 4, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
| @@ -1377,6 +1377,88 @@ describe('QwenAgent MCP SSE/HTTP support', () => { | |||
| await agentPromise; | |||
| }); | |||
|
|
|||
Collaborator
There was a problem hiding this comment.
[Suggestion] This regression test covers the newSession path, but loadSession and unstable_resumeSession have the same race vulnerability (two concurrent requests with different cwd values can pick up each other's settings via this.settings during the async runWithAcpRuntimeOutputDir existence check). Without tests for those paths, a future refactor could reintroduce the race silently.
Consider adding an analogous interleaving test for loadSession (or unstable_resumeSession) — two concurrent calls with distinct cwd values, gating the first on sessionExists, and asserting each Session constructor receives the correct per-workspace settings instance.
— qwen3.7-max via Qwen Code /review
| // agent-level readers at this request's workspace. | ||
| this.settings = settings; | ||
|
|
||
| const config = await this.newSessionConfig( |
Collaborator
There was a problem hiding this comment.
[Suggestion] The deferred this.settings = settings assignment (after the if (!exists) throw guard) is the most subtle part of the fix — the comment correctly warns that a failed probe must not repoint agent-level readers. However, the existing resourceNotFound tests only assert the error is thrown; they do not verify that this.settings retains its prior value. A future "cleanup" that moves this assignment above the existence check would pass all existing tests while reintroducing the cross-workspace leak.
Consider adding a test that: (1) establishes a known this.settings via a successful newSession, (2) attempts a loadSession from a different workspace that fails with resourceNotFound, and (3) asserts this.settings still reflects the original workspace.
— qwen3.7-max via Qwen Code /review
This was referenced Jul 4, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does
Makes each ACP session entry point (
newSession,loadSession,unstable_resumeSession) load its workspace'sLoadedSettingsonce at the top of the handler and thread that same instance explicitly throughnewSessionConfigandcreateAndStoreSession, instead of re-reading the shared mutablethis.settingsfield after long awaits.this.settingsstays in place as a "latest loaded" cache for agent-level readers (auth persistence, fastModel, folder trust, language, settings ext-methods), which keeps their behavior unchanged.Concretely:
newSession/loadSession/unstable_resumeSessionstart withconst settings = loadSettings(cwd); this.settings = settings;and passsettingsdown.newSessionConfig(cwd, mcpServers, settings, sessionId?, resume?)takes the instance as a required parameter and no longer callsloadSettingsitself; its merged snapshot, hook accessors, and the disabled-skills provider all use the passed instance.createAndStoreSession(config, settings, sessionData?, options?)constructsSessionwith the passed instance instead ofthis.settings.loadSession/unstable_resumeSessionrun their session-existence check (runWithAcpRuntimeOutputDir) with the settings just loaded for the request'scwd, not with whatever a previous handler left behind.Why it's needed
ACP JSON-RPC handlers run concurrently, and session creation awaits config load, MCP discovery, and an auth refresh between "load settings" and "construct Session" — a window of hundreds of ms to seconds. Two reads of
this.settingsrace across that window:createAndStoreSession→new Session(..., this.settings): if session B starts while session A is mid-creation, A'sSessionis constructed with B'sLoadedSettings.Sessionpersists model changes through that instance (setValue(persistScope, 'model.name' / 'model.baseUrl', ...)), so with multi-workspace clients a model switch in workspace A writes into workspace B's settings.json. It also breaks the instance-identity contract documented at thebuildDisabledSkillNamesProvidercall site (the provider must close over the same instance later reloaded viareloadScopeFromDisk).loadSession/unstable_resumeSessionexistence check: it ran underrunWithAcpRuntimeOutputDir(this.settings, ...)before this request's settings were loaded, i.e. with the previous handler's instance. With per-workspaceadvanced.runtimeOutputDirvalues, the check looks in the wrong directory and returns a spurious "session not found".Semantic notes:
this.settingscache keeps its pre-fix update timing:newSessionadopts the freshly loaded instance immediately, andloadSession/unstable_resumeSessionadopt it only after the session-existence check passes — a failed (404) probe leaves the cache untouched, exactly as before this PR. The existence check itself uses the local per-request instance.reloadScopeFromDiskstill only affects the settings instance of the session(s) that share it; sessions bound to other instances don't see the reload. That is exactly the single-cache behavior before this PR — this change neither widens nor narrows it.Reviewer Test Plan
How to verify
passes each concurrent newSession its own workspace settings instanceinpackages/cli/src/acp-integration/acpAgent.test.ts. It interleaves twonewSessioncalls (workspace A stalls inloadCliConfigwhile workspace B completes) and asserts eachSessionis constructed with its own workspace'sLoadedSettingsinstance.cd packages/cli && npx vitest run src/acp-integration/acpAgent.test.ts— passes on this branch.acpAgent.tshunks (keep the test) and re-run: the test fails with session A receiving workspace B's settings instance, reproducing the race.Evidence Before & After
Before (fix reverted via
git stash push -- packages/cli/src/acp-integration/acpAgent.ts, test kept):("no visual difference" is the point: the two fixture instances are structurally identical, and session A was constructed with workspace B's instance — an identity mix-up, which is exactly what makes
setValuepersist into the wrong workspace file in production.)After (this branch, full file suite):
tsc --noEmitandeslinton the two changed files are clean.Tested on
Environment
Risk & Scope
this.settingsreader inside the session-creation path that should now use the per-request instance. Mitigated by auditing everythis.settingsuse inacpAgent.ts: remaining reads are agent-level by design (authenticate persistence,loadPermissionSettings, fastModel/folderTrust/language, settings ext-methods) and keep the existing "latest loaded" cache semantics.session/newfor different cwds (covered by the unit-level interleaving test instead).newSessionConfig/createAndStoreSessionare private methods; ACP protocol surface is unchanged.Linked Issues
Related: #6263 (daemon multi-session performance; this PR fixes the correctness half of the shared-agent-state findings from the same investigation).
中文说明
本 PR 做了什么
让三个 ACP 会话入口(
newSession、loadSession、unstable_resumeSession)在 handler 开头各自加载一次工作区的LoadedSettings,并把同一个实例显式传递给newSessionConfig和createAndStoreSession,不再在长 await 之后重新读取共享可变字段this.settings。this.settings保留为"最近一次加载"的缓存,供 agent 级读者(认证持久化、fastModel、目录信任、语言、settings 扩展方法)使用,其行为不变。具体改动:
const settings = loadSettings(cwd); this.settings = settings;,后续显式传递settings;newSessionConfig(cwd, mcpServers, settings, sessionId?, resume?)增加必选参数,不再自己调用loadSettings;merged 快照、hooks 读取、disabled-skills provider 全部改用传入实例;createAndStoreSession(config, settings, sessionData?, options?)用传入实例构造Session;loadSession/unstable_resumeSession的会话存在性检查(runWithAcpRuntimeOutputDir)使用本次请求刚加载的 settings,而不是上一个 handler 留下的实例。为什么需要
ACP JSON-RPC handler 是并发执行的,而会话创建在"加载 settings"与"构造 Session"之间要经过配置加载、MCP 发现、认证刷新等多段 await,窗口达数百毫秒到秒级。这个窗口内有两处
this.settings读取会发生竞争:createAndStoreSession里new Session(..., this.settings):会话 A 创建过程中会话 B 开始,A 的Session会拿到 B 的LoadedSettings。Session通过该实例持久化模型切换(setValue(persistScope, 'model.name' / 'model.baseUrl', ...)),多工作区客户端下,A 工作区的模型切换会写进 B 工作区的 settings.json;同时破坏buildDisabledSkillNamesProvider调用点注释所要求的实例一致性约定。loadSession/unstable_resumeSession的存在性检查:原先在本次loadSettings之前执行,用的是上一个 handler 的实例;各工作区advanced.runtimeOutputDir不同时会在错误目录找会话文件,误报"会话不存在"(404)。语义说明:
this.settings缓存保持修复前的更新时点:newSession立即采纳新加载的实例;loadSession/unstable_resumeSession仅在存在性检查通过后才更新缓存——失败(404)的探测不触碰缓存,与修复前完全一致。存在性检查本身使用请求级局部实例。reloadScopeFromDisk仍只作用于与之共享实例的会话;绑定其他实例的会话感知不到。这与修复前单缓存的行为一致,本 PR 既不扩大也不缩小该限制。评审验证建议
passes each concurrent newSession its own workspace settings instance:两个newSession交错执行(A 在loadCliConfig挂起、B 先完成),断言每个Session拿到各自工作区的LoadedSettings实例。cd packages/cli && npx vitest run src/acp-integration/acpAgent.test.ts在本分支通过(181/181)。acpAgent.ts改动后重跑,测试失败(A 拿到 B 的实例),即复现竞争;失败信息中 "Compared values have no visual difference" 恰好说明这是实例身份错配而非内容差异。风险与范围
this.settings读点。已逐一审计acpAgent.ts中全部this.settings使用:剩余读点均为设计上的 agent 级语义(认证持久化、loadPermissionSettings、fastModel/目录信任/语言、settings 扩展方法),保持既有缓存语义。session/new的端到端场景(以单测层面的交错测试覆盖)。🤖 Generated with Qwen Code