fix(daemon): bind QWEN_CODE_SESSION_ID to the current session via AsyncLocalStorage - #4998
Conversation
…est.ts
tsc --build fails on daemon_mode_b_main because sdk.test.ts references
createSessionRootContext (mocked via vi.mock('./tracer.js')) without
importing the symbol. Test-only change; unblocks the package build.
…ncLocalStorage In daemon mode one process hosts many sessions, but the shell context env session ID was read from process.env — a single process-global slot that only the FIRST Config ever claims (sessionEnvClaimed guard in config.ts). Every later session (new or resumed) spawned shells that reported the first session's ID, mismatching the actual session. - add sessionIdContext (AsyncLocalStorage), mirroring promptIdContext - getShellContextEnvVars(): prefer sessionIdContext over process.env; fall back to process.env so single-session CLI behavior is unchanged - ACP Session: wrap #executePrompt / #executeCronPrompt / #executeBackgroundNotificationPrompt in sessionIdContext.run(...) - tests: ALS precedence, env fallback, concurrent-session isolation
|
Thanks for the PR! Template looks good ✓ On direction: this is a real correctness bug — daemon mode spawning shells with the wrong session ID breaks audit logging and trace correlation. Clearly in-scope and worth fixing. No direct CHANGELOG reference, but the area (daemon/session management) is core infrastructure. On approach: the scope is tight and the pattern is exactly right. One observation on the wrapper pattern in Moving on to code review. 🔍 中文说明感谢贡献! 模板完整 ✓ 方向:这是一个真实的正确性问题——daemon 模式下 shell 子进程拿到错误的 session ID,破坏了审计日志和链路追踪。明确在项目范围内,值得修复。CHANGELOG 无直接引用,但该领域(daemon/session 管理)是核心基础设施。 方案:范围紧凑,模式完全正确。 关于 进入代码审查 🔍 — Qwen Code · qwen3.7-max |
There was a problem hiding this comment.
Pull request overview
This PR fixes session-ID propagation for shell subprocesses in daemon (multi-session) mode by binding QWEN_CODE_SESSION_ID to the current async execution context rather than relying on the process-global process.env slot (which is intentionally “claimed” only once by the first Config).
Changes:
- Added
sessionIdContext(AsyncLocalStorage) and updatedgetShellContextEnvVars()to prefer the async-context session ID overprocess.env(with aprocess.envfallback to preserve single-session CLI behavior). - Wrapped ACP session execution entry points in the CLI (
#executePrompt,#executeCronPrompt,#executeBackgroundNotificationPrompt) withsessionIdContext.run(...)to ensure correct session binding during tool/shell execution. - Expanded unit tests for
getShellContextEnvVars()to cover ALS precedence, env fallback, and concurrent session isolation; fixed a missing import in telemetry tests to keeptsc --buildpassing.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| packages/core/src/utils/shellContextEnv.ts | Prefer per-async-context session ID when generating env vars for shell subprocesses, with env fallback. |
| packages/core/src/utils/shellContextEnv.test.ts | Adds regression tests for ALS session ID precedence, fallback behavior, and concurrent isolation. |
| packages/core/src/utils/sessionIdContext.ts | Introduces AsyncLocalStorage-backed session ID context to support multi-session hosts. |
| packages/core/src/telemetry/sdk.test.ts | Adds missing import used by session-context refresh tests to satisfy TS build. |
| packages/core/src/index.ts | Exposes sessionIdContext from the core package barrel exports. |
| packages/cli/src/acp-integration/session/Session.ts | Binds ACP execution entry points to the current session via sessionIdContext.run(...). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Code ReviewClean implementation. The three wrapper splits in No correctness bugs, no security concerns, no regressions. The TypeScript: Unit TestsReal-Process VerificationReal CLI Smoke Test (tmux)Single-session CLI unchanged — Note: Full daemon double-session test (step 3 in the PR's test plan — ACP agent + model call) could not be run here due to model gateway unavailability in this CI environment. Same limitation noted by the author. 中文说明代码审查实现干净。 无正确性 bug、无安全问题、无回归。 TypeScript: 单元测试12/12 shellContextEnv 测试通过(含 3 条新回归用例),59/59 sdk 测试通过。 真实进程验证使用编译后 CLI 冒烟测试单 session CLI 行为不变—— 注意: 完整 daemon 双 session 测试(PR 测试计划第 3 步——ACP agent + 模型调用)因当前 CI 环境模型网关不可达未能运行。作者同样注明了此限制。 — Qwen Code · qwen3.7-max |
chiga0
left a comment
There was a problem hiding this comment.
Code Review Overview (AI Generated)
PR: #4998 fix(daemon): bind QWEN_CODE_SESSION_ID to the current session via AsyncLocalStorage
Author: @yiliang114
Type: Bug Fix
Change size: +117/-7 across 6 files, 2 commits
HEAD: ee411d1
Findings Summary
- Critical/Major: 0
- Minor: 0
- Nit: 0
Key Observations
This is a clean, well-scoped bug fix. In daemon mode, process.env['QWEN_CODE_SESSION_ID'] is set once by the first Config (sessionEnvClaimed guard), so every subsequent session's shell subprocesses read a stale session ID. The fix introduces sessionIdContext (AsyncLocalStorage) — exactly mirroring the existing promptIdContext pattern — and wraps all three session execution entry points (#executePrompt, #executeCronPrompt, #executeBackgroundNotificationPrompt) in sessionIdContext.run(sessionId, ...).
getShellContextEnvVars() now does sessionIdContext.getStore() ?? process.env['QWEN_CODE_SESSION_ID'], correctly preferring ALS in daemon mode while falling back to env for single-session CLI.
Verified:
- All 3 execution paths wrapped (user prompts, cron, notifications) ✓
- Fallback to
process.envpreserves CLI behavior ✓ promptIdContextandagentIdalready used ALS — session ID was the last gap ✓- Tests cover: ALS preference, env fallback, concurrent session isolation ✓
- Export from
packages/core/index.tsfollows convention ✓ - No package boundary violations ✓
Final Verdict — APPROVE
Textbook AsyncLocalStorage fix following the project's existing promptIdContext pattern. Complete coverage of all execution entry points. Good test coverage including concurrent isolation. LGTM.
This review was generated by QoderWork AI
|
This is what a good bug fix looks like. The PR identified a real correctness gap in daemon mode (process-global env slot can't track per-session identity), picked the exact pattern the codebase already uses for the analogous problem ( The real-process verification (real One minor note: the full daemon double-session test (ACP agent + live model call) remains unverified by both the author and this review, due to model gateway unavailability. This is a known gap but doesn't block — the unit and real-process tests cover the core mechanism, and the daemon path is a thin wrapper over the same Approving. ✅ 中文说明这就是一个好的 bug 修复应有的样子。PR 发现了 daemon 模式中一个真实的正确性缺口(进程级 env 槽位无法追踪 per-session 身份),选择了代码库中已有对应问题的完全相同模式( 真实进程验证(真实 一个小注:完整 daemon 双 session 测试(ACP agent + 实际模型调用)因模型网关不可达,作者和本审查均未验证。这是已知缺口但不阻塞——单元和真实进程测试覆盖了核心机制,daemon 路径只是对同一 批准。✅ — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
No issues found. LGTM! ✅ — qwen3.7-max via Qwen Code /review
doudouOUC
left a comment
There was a problem hiding this comment.
[Critical] server.ts:2571 和 dispatch.ts:1064 — daemon 的显式 shell 端点(POST /session/:id/shell 和 ACP session/shell)调用 bridge.executeShellCommand() 时没有包 sessionIdContext.run(),导致 getShellContextEnvVars() 回退到过期的 process.env 值。这正是本 PR 已为 prompt/cron/background 三条路径修复的同类 bug,但这两个直接 shell 路径被遗漏了。
影响: 除第一个 session 外,通过 IDE 集成或 ACP shell 命令启动的 shell 子进程都会读到错误的 QWEN_CODE_SESSION_ID,导致审计日志和链路追踪静默错位。
修复建议:
// server.ts:2571 — 包裹 sessionIdContext.run()
const result = await sessionIdContext.run(sessionId, () =>
bridge.executeShellCommand(
sessionId, command.trim(), abort.signal,
clientId !== undefined ? { clientId } : undefined,
),
);
// dispatch.ts:1064 — 同样模式
const result = await sessionIdContext.run(sessionId, () =>
this.bridge.executeShellCommand(
sessionId, rawCmd, undefined,
this.sessionCtx(conn, sessionId, loopback),
),
);两个文件都需加上 import { sessionIdContext } from '@qwen-code/qwen-code-core';。
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| // like the daemon) over the process-global env slot, which only ever | ||
| // reflects the first session created in this process. | ||
| const sessionId = | ||
| sessionIdContext.getStore() ?? process.env['QWEN_CODE_SESSION_ID']; |
There was a problem hiding this comment.
[Suggestion] 当 sessionIdContext.getStore() 为 undefined 时,代码静默回退到 process.env — 零诊断输出。如果有任何未来代码路径遗漏了 ALS 包裹,错误 session ID 会毫无预警地传播,生产环境极难排查。建议在 daemon 模式下,ALS 为空时加一条 debug 级别日志。
— DeepSeek/deepseek-v4-pro via Qwen Code /review
What this PR does
Makes
QWEN_CODE_SESSION_IDin shell subprocess environments always reflect the session that actually spawned the shell. It adds asessionIdContext(AsyncLocalStorage, same pattern aspromptIdContext), makesgetShellContextEnvVars()prefer that context overprocess.env(falling back toprocess.envso the single-session CLI is unchanged), and wraps the three ACP session execution entry points (#executePrompt,#executeCronPrompt,#executeBackgroundNotificationPrompt) insessionIdContext.run(...). A separate test-only commit adds a missing import insdk.test.tsthat was breakingtsc --buildon this branch.Why it's needed
In daemon mode one process hosts many sessions, but
process.env['QWEN_CODE_SESSION_ID']is a single process-global slot that only the FIRSTConfigever writes (thesessionEnvClaimedguard in config.ts), and the ACP path never callsstartNewSession()— that only exists in the interactive TUI. So every session created or resumed after the first one spawned shells that reported the first session's ID:/statusshowed the right session ID whileecho $QWEN_CODE_SESSION_IDinside a tool call showed a different one, breaking audit logging and trace correlation for downstream SQL/Python scripts.Reviewer Test Plan
How to verify
cd packages/core && npx vitest run src/utils/shellContextEnv.test.ts— 12/12, including three new regression cases: ALS takes precedence over a stale env value, env fallback keeps the single-session CLI behavior, and two concurrent sessions in one process each see their own ID.Configinstances in one Node process, then spawn a realsh -c 'echo $QWEN_CODE_SESSION_ID'with{...process.env, ...getShellContextEnvVars()}— exactly howshellExecutionServiceinjects env. Before the fix the second session's shell prints the first session's ID; after the fix it prints its own.node dist/cli.js --acp), create session A, prompt it to runecho "SID=$QWEN_CODE_SESSION_ID", then create session B in the same process and repeat — the SID should follow the active session instead of staying at A.Evidence (Before & After)
Output of the real-process check (real
Configclass + real shell subprocess):Live daemon run (step 3) — real ACP agent process, two sessions in ONE process, model-driven Shell tool call, auto-approved permissions:
Tested on
Environment (optional)
Unit tests via vitest; real-process check via a standalone Node script against the built
packages/core/dist. No sandbox.Risk & Scope
process.env— i.e. pre-fix behavior, never worse. Audited the branch: no such path exists today.Session.test.tscases and 2 failing suites on this branch are pre-existing (verified identical with and without this change — 404 passed both ways).process.envfallback.Linked Issues
Relates to #4649 (the PR that introduced shell context env injection).
中文说明
本 PR 做了什么
让 shell 子进程环境变量里的
QWEN_CODE_SESSION_ID始终反映真正发起这次 spawn 的 session。新增与promptIdContext同模式的sessionIdContext(AsyncLocalStorage),getShellContextEnvVars()优先从该上下文读 session id、读不到再回退process.env(普通单 session CLI 行为完全不变);ACPSession的三个执行入口(#executePrompt、#executeCronPrompt、#executeBackgroundNotificationPrompt)都包进sessionIdContext.run(...)。另含一个独立的纯测试 commit:补sdk.test.ts缺失的 import,修复本分支tsc --build编译不过的问题。为什么需要
daemon 模式下一个进程承载多个 session,但
process.env['QWEN_CODE_SESSION_ID']是单一进程级槽位,只有进程里第一个Config会写入(config.ts 的sessionEnvClaimed守卫);ACP 路径又从不调用startNewSession()(它只挂在交互式 TUI 上)。结果是第一个 session 之后新建/恢复的任何 session,其 bash/SQL/Python 子进程读到的都是第一个 session 的 id——/status显示的会话 ID 和脚本里echo $QWEN_CODE_SESSION_ID对不上,下游审计日志和链路追踪全部错位。如何验证
packages/core下跑npx vitest run src/utils/shellContextEnv.test.ts——12/12 通过,含 3 条新回归用例:ALS 优先于过期 env 值、无上下文时回退 env、同进程两个并发 session 各自拿到自己的 id。Config,再以{...process.env, ...getShellContextEnvVars()}起真实 shell 读$QWEN_CODE_SESSION_ID(与shellExecutionService注入方式一致)。修复前第二个 session 的 shell 打出第一个 session 的 id;修复后打出自己的。证据输出见上方英文区。echo "SID=$QWEN_CODE_SESSION_ID",同进程再建 session B 重复——两个 session 的 shell 各自读到自己的 session id(A/B 均 match=true,LIVE VERIFY: PASS,证据见上方英文区)。风险与范围
process.env,即修复前行为,不会更糟(已排查,当前不存在这样的路径)。Session.test.ts的 7 个失败用例和 2 个失败套件为预先存在(带/不带本改动对照一致,均为 404 passed)。关联
关联 #4649(引入 shell context env 注入的 PR)。