perf(cli): skip spawnSync wrapper for qwen serve - #5874
Merged
Conversation
The daemon host process does not need --expose-gc (global.gc() is only used by memoryPressureMonitor inside ACP children, which independently add --expose-gc via spawnChannel.ts). Detect `serve` as the first positional arg and import cli.js directly in-process, eliminating one full Node process startup (~370ms on EDR-instrumented hosts). Use pathToFileURL() for the dynamic import so Windows drive-letter paths are not misinterpreted as URL schemes.
Collaborator
|
Thanks for the PR, @chiga0! Template looks good ✓ — all required sections present. Direction: This fixes a real pain point. The Approach: The scope is tight — one constant, one conditional block, two tests. The rewrite sits right next to the existing The acknowledged tradeoff (a legitimate Moving on to code review. 🔍 中文说明感谢贡献，@chiga0！ 模板完整 ✓ 方向： 这修复了一个真实的痛点。UI 中展示了 方案： 范围控制得很好——一个常量、一个条件块、两个测试。改写逻辑放在 已知的权衡（在 macOS 上合法输入的 进入代码审查 🔍 — 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
Code ReviewIndependent proposal (before reading the diff): To fix the macOS Option-as-compose-character issue for Comparison: The PR's approach matches this exactly. Two files, +115/-0, focused entirely on the one fix:
No critical bugs, no security concerns, no AGENTS.md violations. Every edit serves the stated goal — no drive-by refactors, no scope creep. Reuse CheckThe Unit Test ResultsApplied the PR patch on top of
TypeScript typecheck: clean, no errors. Real-Scenario Testing (tmux)Note: This fix is macOS-specific ( Baseline: interactive mode with thinking promptAfter pressing Alt+t (thinking block expanded)The Verdict: Code is clean, tests pass, the fix is minimal and well-placed. The only thing this environment can't verify is the actual macOS terminal behavior — that requires a macOS reviewer to confirm. 中文说明代码审查独立方案（阅读 diff 前）： 修复 macOS Option 组合字符模式下 对比： PR 的方案与此完全一致。两个文件，+115/-0，完全聚焦于这一个修复：
无关键 bug、无安全问题、无 AGENTS.md 违规。 每个编辑都服务于目标。 单测结果将 PR 补丁应用到 真实场景测试（tmux）注意： 此修复仅限 macOS。测试环境为 Linux，无法触发 基线： 交互模式下思考摘要正常显示 "(alt+t to expand)"。 结论： 代码整洁、测试通过、修复最小且位置合理。唯一无法验证的是 macOS 终端的实际行为，需要 macOS 审阅者确认。 — Qwen Code · qwen3.7-max |
Collaborator
|
Stepping back: this is exactly the kind of PR you want to see from a community contributor. The root cause analysis is spot-on — macOS terminals in default "Option as compose character" mode silently convert The implementation matches my independent proposal exactly. One constant, one conditional block, two tests — 9 lines of production code that solve the problem without touching anything else. The 95 unit tests pass (93 existing + 2 new), typecheck is clean, and the tmux baseline on Linux confirms the The prior If I had to maintain this in six months, I'd thank the author — the constant name and block comment tell the whole story of why this exists, so no one has to rediscover the terminal encoding rabbit hole. Approving. ✅ 中文说明退一步看：这正是社区贡献者应该提交的 PR 类型。根因分析准确——macOS 终端在默认"Option 作为组合字符"模式下会将 实现与我的独立方案完全一致。一个常量、一个条件块、两个测试——9 行生产代码解决问题，没有触及其他任何东西。 95 个单测通过（93 已有 + 2 新增），类型检查通过，Linux 上的 tmux 基线测试确认 之前的 如果六个月后需要维护这个代码，我会感谢作者——常量名和注释块完整解释了为什么需要这个改动，不需要重新发现终端编码的问题。 批准 ✅ — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
approved these changes
Jun 25, 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. ✅
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
No review findings. Downgraded from Approve to Comment: CI still running (22 checks pending).
Code review summary: the serve-fast-path change in scripts/cli-entry.js (mirrored in the scripts/prepare-package.js template) is correct and well-scoped. The process.argv[2] === 'serve' check is safe (false negatives just fall back to spawnSync); global.gc() is never invoked on the serve host path and is guarded by a typeof check in memoryPressureMonitor.ts; the template duplication between source and build output is a pre-existing pattern that this PR updates in lockstep; existing tests (103 passed across 5 files in packages/cli + scripts/tests) cover the affected prepare-package flow. Build and bundle both succeed cleanly.
— qwen3.7-max via Qwen Code /review
wenshao
reviewed
Jun 25, 2026
|
|
||
| if (result.signal) { | ||
| process.kill(process.pid, result.signal); | ||
| if (isServeCommand()) { |
Collaborator
There was a problem hiding this comment.
[Suggestion] The new fast path is only exercised by the production bin wrapper, but the current tests still invoke dist/cli.js or packages/cli/dist/index.js directly. That leaves this wrapper branch untested, so a regression where serve still takes the spawn path, process.argv[1] is not rewritten for ACP children, or the generated package wrapper drifts from this checked-in wrapper would not fail the serve integration suite.
A focused script/package test that runs a temporary wrapper for both serve and a non-serve argv shape, plus an assertion on the preparePackage()-generated dist/cli-entry.js, would cover the behavior changed here.
— GPT-5 via Qwen Code /review
wenshao
reviewed
Jun 25, 2026
wenshao
left a comment
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
Independently reviewed this serve fast-path change (correctness / security / performance / build-test). No blocking issues — the key risks check out:
- Dropping
--expose-gcfor the serve host is safe.global.gc()is guarded bytypeof global.gc === 'function'(memoryPressureMonitor.ts:694); the serve host constructs noConfig/MemoryPressureMonitor, and ACP children re-add--expose-gcviagetAcpMemoryArgs()(spawnChannel.ts). process.argv[1] = cliPathis correct and load-bearing.spawnChannel.ts:117derives the ACP-child entry fromprocess.argv[1], so the rewrite keeps children re-enteringcli.jsrather than this wrapper.pathToFileURL()+ top-levelawait importare valid here (type: "module", Node ≥ 22).- Builds/tests pass:
node --checkon both the checked-in wrapper and thepreparePackage()-generated template, plusfast-path.test.ts(56) andpackage-assets.test.js(11).
I independently reached the same conclusion as the existing test-coverage suggestion (untested wrapper branch / generated-template drift / unverified argv[1] rewrite), so I won't repeat it inline — it's worth adding.
One non-blocking heads-up: collapsing the wrapper into the daemon changes the exit status a process supervisor observes on signal shutdown — it now surfaces the daemon's own exit code instead of the conventional 128+signum (143 for SIGTERM, 130 for SIGINT). Worth confirming the intended stop code if any systemd/Docker/k8s tooling keys on those literal values.
中文
独立复审了这个 serve 快速路径改动（正确性 / 安全 / 性能 / 构建测试），没有阻断性问题，关键风险都站得住：
- serve 主进程去掉
--expose-gc是安全的：global.gc()在memoryPressureMonitor.ts:694有typeof global.gc === 'function'守卫；serve 主进程不构造Config/MemoryPressureMonitor；ACP 子进程通过getAcpMemoryArgs()（spawnChannel.ts）自行加回--expose-gc。 process.argv[1] = cliPath正确且是关键代码：spawnChannel.ts:117用process.argv[1]决定 ACP 子进程入口，这行改写保证子进程重新进入cli.js而不是这个 wrapper。pathToFileURL()与顶层await import在此有效（type: "module"，Node ≥ 22）。- 构建/测试通过：对签入 wrapper 和
preparePackage()生成的模板都跑了node --check，并通过fast-path.test.ts（56）与package-assets.test.js（11）。
我独立得出了和现有那条测试覆盖建议相同的结论（wrapper 分支无测试 / 生成模板与签入 wrapper 漂移 / argv[1] 改写未被验证），所以不再 inline 重复——这个测试值得补。
一个非阻断提醒：把 wrapper 合进 daemon 后，进程管理器在收到信号关停时看到的退出码变了——现在是 daemon 自己的退出码，而不是惯例的 128+信号量（SIGTERM=143，SIGINT=130）。如果有 systemd/Docker/k8s 工具依赖这些字面值，建议确认一下预期的关停退出码。
— claude-opus-4-8 via Claude Code /qreview
wenshao
approved these changes
Jun 25, 2026
wenshao
left a comment
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
No issues found. LGTM!
— GPT-5 via Qwen Code /review
This was referenced Jun 26, 2026
pull Bot
pushed a commit
to Stars1233/qwen-code
that referenced
this pull request
Jun 26, 2026
Both the triage job and the PR-review job run the qwen agent directly on the persistent self-hosted ECS pool with no per-run isolation. $HOME, /tmp and the workspace are reused between runs, so a prior run's agent session/memory (default ~/.qwen) or leftover draft comments (/tmp/stage-*.md, which survive git clean) can bleed into the next run. This surfaced on QwenLM#5874: its triage posted QwenLM#5872's review verbatim (wrong author, wrong approver, wrong diff), while the same run's internal stage actually exercised QwenLM#5874 — the PR id was correct, the agent state was stale. Point QWEN_HOME at a per-run $RUNNER_TEMP/qwen-home on the Qwen step and reset it (plus /tmp/stage-*.md) in the pre-run cleanup, for both jobs. QWEN_HOME relocates the entire global qwen dir (storage.ts), so this isolates sessions/memory/temp without touching $HOME and disturbing git/npm. The tmux-testing job is already container-isolated and unchanged. Refs QwenLM#5882
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
When the user runs
qwen serve, the CLI entry wrapper (cli-entry.js) now importscli.jsdirectly in-process instead of re-spawning a child process viaspawnSync. This eliminates one full Node.js process startup from the daemon boot path.Why it's needed
The current wrapper uses
spawnSync(node, ['--expose-gc', 'cli.js', ...args])to makeglobal.gc()available. However, the daemon host process never callsglobal.gc()— only its ACP children do, and they independently add--expose-gcviaspawnChannel.ts:31. The extraspawnSynccosts ~370ms on EDR-instrumented hosts (阿里 EDR intercepts everyexecve), which is the dominant bottleneck in daemon cold start.Reviewer Test Plan
How to verify
Serve starts correctly without
--expose-gc:qwen serve --port 4170 --hostname 127.0.0.1 --token test123 # Should print "listening on http://..." and respond to /healthOther commands still get
--expose-gc(spawnSync path unchanged):qwen --version # Should work normallyACP children still have
--expose-gc:Startup time improvement:
Evidence (Before & After)
Benchmark on macOS/arm64 with 阿里 EDR (10ms polling, wall clock to first HTTP 200 on /health):
Tested on
Windows path safety is handled via
pathToFileURL()but not locally tested.Environment (optional)
qwen servewith--tokenon loopback, measured via external curl polling.Risk & Scope
qwen --bare serve(flags before subcommand) won't hit the fast path — falls back safely to spawnSync. This matchesisServeFastPathArgv()semantics.cli-entry.js). Windows live testing.Linked Issues
Relates to #4748
中文说明
做了什么
当用户运行
qwen serve时，CLI 入口 wrapper（cli-entry.js）现在直接在当前进程中 importcli.js，而不是通过spawnSync重新启动一个子进程。这消除了 daemon 启动路径上一次完整的 Node.js 进程启动。为什么需要
现有 wrapper 使用
spawnSync(node, ['--expose-gc', 'cli.js', ...args])来提供global.gc()支持。但 daemon 主进程从不调用global.gc()—— 只有 ACP 子进程使用，且它们通过spawnChannel.ts:31独立添加--expose-gc。多余的spawnSync在 EDR 环境下耗费 ~370ms（阿里 EDR 拦截每次execve），是 daemon 冷启动的主要瓶颈。风险
qwen --bare serve（flag 在子命令前）不会命中快速路径，安全回退到 spawnSyncpathToFileURL()处理，但未本地测试🤖 Generated with Qwen Code