feat(cli): Add daemon-managed channel worker for serve --channel - #6031
Merged
Conversation
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Collaborator
|
Thanks for the PR! Template looks good ✓ — all required sections present (What/Why/Reviewer Test Plan/Risk & Scope/Linked Issues/中文说明). On direction: this is a natural follow-up to the channel adapter system (#5976 roadmap). Running channels as daemon-managed workers under On approach: the scope is large (49 files, +5636/-413 lines) but the bulk is new code — two new modules ( The design is sound: fork-based supervisor with IPC readiness signal, SIGTERM→SIGKILL escalation, pidfile ownership to prevent conflicts between standalone and daemon-managed workers. This matches what I'd independently propose. Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ — 所有必填章节齐全（What/Why/Reviewer Test Plan/Risk & Scope/Linked Issues/中文说明）。 方向：这是 channel adapter 系统（#5976 路线图）的自然延续。让 channel 作为 方案：范围较大（49 文件，+5636/-413 行），但主体是新代码——两个新模块（ 设计合理：基于 fork 的 supervisor + IPC 就绪信号、SIGTERM→SIGKILL 升级、pidfile 所有权防止独立/托管 worker 冲突。与我的独立方案一致。 进入代码审查 🔍 — Qwen Code · qwen3.7-max |
Collaborator
Code ReviewIndependent proposal: to add daemon-managed channel workers, I'd create a worker entry point that connects to an existing daemon via the SDK, a supervisor that forks/monitors the worker with IPC, and integrate lifecycle into Comparison: the PR matches the independent baseline and goes further — env scrubbing for security, loopback URL validation, AbortController for startup cancellation, and comprehensive test coverage (508 tests). No critical blockers found. Findings:
No critical blockers. Code is well-structured with proper error handling and security measures. Test ResultsUnit tests: 508/508 passing ✅
Build: ✅ clean ( Typecheck: ✅ clean ( Real-Scenario Testing (tmux)
|
Collaborator
ReflectionStepping back: this PR solves a real operational pain point — having to run The PR's approach matches my independent proposal exactly: fork-based supervisor with IPC readiness, pidfile ownership metadata, lifecycle integration into serve's startup/shutdown sequence. Where it exceeds my baseline is in the defensive details — env scrubbing before fork, loopback URL validation, AbortController for cancellable startup, and the TOCTOU fix in ChannelBase. All 508 tests pass. Build and typecheck are clean. The The only minor concern is that the PR bundles PR1.1 cleanup changes (SessionRouter fix, TOCTOU fix, log sanitization) with the main feature. These are related but could have been a separate PR for easier review. Not a blocker — they're small, adjacent, and well-tested. Overall: the code is straightforward, well-tested, and solves the stated problem cleanly. If I had to maintain this in six months, the fork/IPC/signal-escalation pattern is well-documented and the test suite tells the story. Verdict: Approve ✅ 中文说明总结退一步看：这个 PR 解决了一个实际的运维痛点——需要在 PR 的方案与我的独立提案完全一致：基于 fork 的 supervisor + IPC 就绪信号、pidfile 所有权元数据、集成到 serve 的启动/关闭生命周期。超出我基准的部分在于防御性细节——fork 前的环境变量脱敏、loopback URL 验证、可取消的 AbortController 启动、ChannelBase 的 TOCTOU 修复。 508 个测试全部通过。构建和类型检查均无报错。 唯一的轻微顾虑是 PR 将 PR1.1 修复（SessionRouter 修复、TOCTOU 修复、日志脱敏）与主要功能合在一起。这些是相关的但本可以拆成单独 PR。不是阻塞项——体量小、相邻、且测试充分。 总体：代码简洁、测试充分、干净地解决了所陈述的问题。如果六个月后需要维护，fork/IPC/信号升级模式有文档支持，测试套件也说明了设计意图。 结论：批准 ✅ — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
requested changes
Jun 30, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
4 failing unit tests in pidfile.test.ts — test mock path mismatch between homedir()/.qwen/ and Storage.getGlobalQwenDir(). Production code is correct; test setup needs to align path resolution. See Stage 2/3 triage comments for full analysis and fix suggestion.
wenshao
reviewed
Jun 30, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Code Review Summary
Build/Test: CLI package compiles. 198/202 tests pass — 4 failures are pre-existing env issues (QWEN_HOME path mismatch), not from this PR.
Critical (3)
once('message')drops ready signal on non-ready IPC- Missing pidfile cleanup on force-exit (double Ctrl+C)
- No startup timeout in supervisor
start()
Suggestions (5)
- Credential leakage via child environment
killAllSyncmissing'failed'guard- TOCTOU race on pidfile uniqueness
- No auto-restart on worker crash
- Runtime startup timeout coupling
Test Coverage Gaps (4)
- Daemon-worker yargs handler untested
runtime.tsextracted module has zero test coverage- SIGTERM-to-SIGKILL escalation untested
- Pidfile conflict check untested in run-qwen-serve
Detailed inline comments below.
wenshao
requested changes
Jun 30, 2026
Collaborator
CI failure analysis —
|
wenshao
requested changes
Jun 30, 2026
wenshao
left a comment
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
The failing Test (ubuntu) check is not the pidfile.test.ts unit tests — it's the bundle guard (see the inline comment on run-qwen-serve.ts). The job fails at the "Check serve fast-path bundle closure" step, which runs before the test step, so the unit suite is skipped and never executes in CI. Repro: npm run check:serve-fast-path-bundle.
Separately, on the pidfile.test.ts failures mentioned earlier in this thread: those reproduce only when QWEN_HOME is set (the test helper getPidFilePath() hardcodes homedir()/.qwen, while the implementation honors QWEN_HOME via Storage.getGlobalQwenDir()). 3 of the 4 (corrupt / pid 0 / malformed) already exist on main with the same helper, so that coupling is pre-existing rather than introduced here — the new "legacy pidfiles" test just inherits it. To make them robust under QWEN_HOME, change the helper to join(Storage.getGlobalQwenDir(), 'channels', 'service.pid').
中文
失败的 Test (ubuntu) 检查不是 pidfile.test.ts 单测，而是打包守卫（见 run-qwen-serve.ts 的行内评论）。该 job 在 "Check serve fast-path bundle closure" 步骤就失败了，这一步在测试步骤之前，所以单测被跳过、在 CI 里根本没跑。复现：npm run check:serve-fast-path-bundle。
另外，关于本线程早先提到的 pidfile.test.ts 失败：只有设置了 QWEN_HOME 时才复现（测试 helper getPidFilePath() 硬编码 homedir()/.qwen，而实现通过 Storage.getGlobalQwenDir() 尊重 QWEN_HOME）。其中 3 个（corrupt/pid 0/malformed）在 main 上就已存在、用同一个 helper，所以这是既有的耦合、并非本 PR 引入——新增的 "legacy pidfiles" 测试只是继承了它。若要在 QWEN_HOME 下也稳定，把 helper 改成 join(Storage.getGlobalQwenDir(), 'channels', 'service.pid')。
— claude-opus-4-8 via Claude Code /qreview
wenshao
requested changes
Jun 30, 2026
wenshao
left a comment
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
Test coverage gaps (pattern): Multiple critical code paths lack test coverage: stop() graceful shutdown (SIGTERM→SIGKILL escalation), error event during startup (settleError), "No channels connected" error path, partial channel connect failure, pidfile conflict check, resolveOnListen: false with channelSelection, handle.close() cleanup verification (only bridge.stop() asserted, not router.clearAll() or channel.disconnect()), daemonWorkerCommand handler (sentinel, env reading, signal handlers), killAllSync() guard conditions for 'exited'/'stopped', createDisabledChannelWorkerSupervisor, loadChannelsFromExtensions branching logic, parseServiceInfo validation branches for invalid servePid/workerPid/owner, and channel-selection.ts has no dedicated unit tests.
— glm-5.2 via Qwen Code /review
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
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. |
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Collaborator
|
@qwen-code /triage |
qwen-code-ci-bot
requested changes
Jun 30, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Needs Human Review
The following were flagged with lower confidence and should be reviewed manually:
- No worker restart mechanism: The supervisor has no automatic restart after a post-startup worker crash. Channels stay offline until the daemon is restarted. This is listed as deferred in the PR description but should be documented in the supervisor's interface.
- Implicit pidfile semantics: For
owner: 'serve'pidfiles,pidtracks the daemon, not the worker. A daemon crash orphans the worker with no pidfile trail. Consider documenting this invariant. daemonWorkerCommand.handleruntested: The sentinel guard, env var validation, token scrubbing, and signal handlers in the handler have no direct test coverage.- Competing exit listeners in
stop(): Theonce('exit', settleExit)listener fromstart()persists after the promise resolves, potentially overwriting'stopped'state with'exited'during shutdown. - Fallback pidfile path uses
'w'instead of'wx': The fallback inreserveChannelServicePidfilebypasses atomic exclusive-create whenreserveServeServiceInfois unavailable.
qwen-code-ci-bot
approved these changes
Jun 30, 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. ✅
wenshao
requested changes
Jun 30, 2026
wenshao
reviewed
Jun 30, 2026
DragonnZhang
left a comment
DragonnZhang
left a comment
Collaborator
There was a problem hiding this comment.
Incremental Review: c144a1f (fix: track channel worker exit explicitly)
Verdict: LGTM - Clean, correct bug fix.
Summary
This commit fixes incorrect error-handling logic in the channel worker supervisor's settleError function. The old code used child?.pid === undefined as a proxy for "exit observed," which conflated spawn failures with actual process exits. The fix introduces an explicit exitObserved boolean set by the exit handler, making the state machine unambiguous.
What was wrong before
When fork() failed to spawn (pid undefined), settleError treated this as "exit already observed" and cleared the child reference. This meant stop() would skip sending SIGTERM, leaking the child process handle in spawn-failure scenarios.
What the fix does correctly
exitObservedflag: Set only insettleExit, sosettleErrorcan distinguish "exit handler already ran" from "spawn never produced a pid." These are semantically different conditions.settleErrorno longer clearschild: The exit handler owns child lifecycle cleanup. Error handler only records state and rejects the startup promise.- Guard
exitObserved || (settled && child === undefined): Correctly handles all event orderings - error-before-exit, exit-before-error, and late errors after settlement. - Test update: The changed test now correctly asserts SIGTERM is sent even when pid is undefined, matching real Node.js behavior where a ChildProcess object exists even after spawn failure.
No issues found. The state machine is now correct for all event orderings.
qwen-code-ci-bot
requested changes
Jun 30, 2026
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
qqqys
previously requested changes
Jun 30, 2026
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
wenshao
requested changes
Jun 30, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Qwen Code Review Summary
Verdict: COMMENT (Suggestions only, no blockers)
PR: Daemon-managed channel worker for qwen serve --channel (+5100/-253 across 48 files)
Overall this is a well-structured PR. The supervisor lifecycle, IPC protocol, and pidfile ownership tracking are solid. Below are 8 suggestions covering edge-case cleanup, test gaps, and minor maintainability improvements. None are blockers.
| # | File | Severity | Title |
|---|---|---|---|
| F1 | channel-worker-supervisor.ts:343 |
Suggestion | SIGKILL path leaks child ref and stopping flag |
| F2 | run-qwen-serve.ts:2613 |
Suggestion | Missing test for onUncaughtExceptionMonitor path |
| F3 | daemon-worker.ts:182 |
Suggestion | Duplicate firstModel() logic from start.ts |
| F4 | run-qwen-serve.ts:335 |
Nice to have | formatChannelWorkerDaemonUrl may reject 127.0.0.0/8 hostnames |
| F6 | daemon-status.ts:521 |
Suggestion | channel_worker_exited severity always 'warning' |
| F8 | run-qwen-serve.ts:2767 |
Suggestion | Shutdown ordering undocumented |
| F9 | pidfile.ts:182 |
Suggestion | Corrupt-pidfile handling untested |
| F10 | run-qwen-serve.ts:2890 |
Suggestion | server.listen error handler test gap |
— qwen3.7-max via Qwen Code /review
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
wenshao
reviewed
Jun 30, 2026
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Collaborator
✅ Maintainer verification report — real
|
| Area | Result |
|---|---|
Build (npm ci + full build) |
✅ clean |
npm run typecheck (whole monorepo) |
✅ exit 0, 0 TS errors |
Unit — channels/base (Router/ChannelBase/DaemonChannelBridge) |
✅ 235 passed |
Unit — cli serve/channel suites (12 files) |
✅ 296 passed |
| Unit — changed files re-run at merge head | ✅ 176 passed (run-qwen-serve 101, +3 new) |
| Mutation tests (non-vacuous proof) | ✅ guards load-bearing (see below) |
| Real E2E lifecycle (12 checks) | ✅ all pass |
🔎 The red CI check is unrelated flakiness, not this PR
Test (ubuntu-latest) fails on src/ui/voice/voice-keyterms-race.test.ts (one Test timed out in 5000ms, one race assertion). That file:
- is not in this PR's diff (PR 6031 touches only
channels/*+cli/src/serve|commands/channel+ docs); - already lives on
origin/main; - passes 3/3 locally in isolation — a timing-sensitive race test starving under the full ~10k-test parallel CI run.
So the merge block is not a defect here. reviewDecision = CHANGES_REQUESTED is a sticky state from earlier bot reviews (all 100 inline threads are resolved; the latest automated review is COMMENT — "Suggestions only, no blockers"). Recommend: re-run the ubuntu leg + a maintainer dismiss/re-review.
🧪 Real E2E evidence (real binary, isolated HOME, mock WS platform)
| # | Behavior | Evidence |
|---|---|---|
| 1 | channel daemon-worker rejects direct invocation |
daemon-worker is an internal qwen serve command. |
| 2 | Worker starts after HTTP listener is ready | listening on …:65163 (log line 2) → Connecting "my-plugin-test" (line 12) → connected (line 15) |
| 3 | Out-of-process worker (crash isolation) | 3 distinct PIDs: daemon, --acp child, channel daemon-worker child |
| 4 | Worker connects back to daemon + to platform | worker does GET /capabilities; lsof: worker holds ESTABLISHED …->[::1]:9201 WS to mock server |
| 5 | Pidfile ownership | {"owner":"serve","pid":<serve>,"servePid":<serve>,"workerPid":<worker>,"channels":["my-plugin-test"]} |
| 6 | qwen channel status |
Channel service: managed by qwen serve (PID …) + Worker PID + uptime |
| 7 | qwen channel stop refuses serve-owned |
managed by qwen serve (PID …). Stop qwen serve to stop channels. |
| 8 | qwen channel start <name> refuses conflict |
Error: Channel service is managed by qwen serve (PID …). Stop the qwen serve process… |
| 9 | /daemon/status running snapshot |
runtime.channelWorker.state:"running", channels, requestedChannels, pid |
| 10 | --channel all |
resolves to configured channel, single worker owns all |
| 11 | Worker crash (SIGKILL) → daemon survives | daemon stays up; status warning, issue channel_worker_exited, state:"exited", signal:"SIGKILL"; onExit removes pidfile + daemon.log WARN |
| 12 | Graceful daemon shutdown (SIGINT) | worker torn down, no orphan, pidfile removed |
Representative raw captures
# (3) process isolation — daemon + ACP child + separate worker
63995 node …/index.js serve --port 0 … --channel my-plugin-test
64101 node …/index.js --acp
64102 node …/index.js channel daemon-worker --channel my-plugin-test
# (4) worker's live WebSocket to the mock platform
node 64102 … TCP [::1]:62552->[::1]:9201 (ESTABLISHED)
# (11) after `kill -9 <worker>` — daemon still serving /daemon/status
overall status: warning
issues: [{ "code": "channel_worker_exited", "severity": "warning",
"message": "Channel worker is exited (pid=64102, code=null, signal=SIGKILL)." }]
runtime.channelWorker: { "state": "exited", "signal": "SIGKILL", … }
# daemon.log: [WARN] [DAEMON] channel worker exited (state=exited, pid=64102, code=null, signal=SIGKILL, error=none)
Mutation tests (prove the tests aren't vacuous):
channel-worker-supervisor.tsready ? 'exited' : 'failed'→ force'exited'⇒ 4 tests fail.daemon-status.tschannel_worker_exitedguard → force off ⇒ 1 test fails (reports failed channel worker snapshots).
Not covered (out of scope for this pass)
- Full message round-trip through a model (I verified the worker↔daemon↔platform transport/lifecycle, not a model reply — no creds in the isolated home).
- The token-auth worker path (used the loopback no-token default).
- macOS only (Windows/Linux not exercised locally, matching the PR's
⚠️matrix).
Recommendation
From hands-on testing the feature is correct and robust — startup ordering, process isolation, pidfile ownership, status reporting, crash resilience, and graceful teardown all work as claimed, and the review-feedback delta is behavior-preserving. The remaining merge blockers are process, not code: (1) the unrelated flaky voice CI test → re-run ubuntu; (2) sticky CHANGES_REQUESTED → maintainer dismiss/re-review.
🇨🇳 中文版（完整对应）
✅ 维护者验证报告 — qwen serve --channel 的真实 tmux 端到端测试
我用本 PR 构建了真实二进制，并用仓库内的 plugin-example channel + 它自带的 mock WebSocket server（无需真实平台凭证），通过 tmux 端到端驱动了 daemon 托管的 channel worker。行为与 PR 描述完全一致。
验证基于合并 head 5e3ed1b18。 验证过程中 head 发生了移动（1790a7a20 → 5e3ed1b18，"address serve channel review feedback"）。我针对当前 head 重新验证；该增量是一次行为保持的重构（firstModel → 共享的 selectFirstModel）、loopback 范围扩展（127.0.0.0/8）以及一条关闭顺序的注释——即解决了评审建议 F3 / F4 / F8 并补了测试。核心运行时行为没有变化。
结论：功能正确，从代码角度可以合并
| 项目 | 结果 |
|---|---|
构建（npm ci + 完整 build） |
✅ 干净 |
npm run typecheck（整个 monorepo） |
✅ exit 0，0 个 TS 错误 |
单测 — channels/base（Router/ChannelBase/DaemonChannelBridge） |
✅ 235 通过 |
单测 — cli serve/channel 套件（12 个文件） |
✅ 296 通过 |
| 单测 — 合并 head 上重跑改动文件 | ✅ 176 通过（run-qwen-serve 101，+3 新增） |
| 变异测试（证明测试非空过） | ✅ 守卫承重（见下） |
| 真实端到端生命周期（12 项） | ✅ 全部通过 |
🔎 CI 红叉与本 PR 无关，是既有 flaky 测试
Test (ubuntu-latest) 挂在 src/ui/voice/voice-keyterms-race.test.ts（一个 Test timed out in 5000ms，一个 race 断言）。该文件：
- 不在本 PR diff 内（PR 6031 只动
channels/*+cli/src/serve|commands/channel+ 文档）； - 本就存在于
origin/main； - 本地隔离运行 3/3 通过——是一个时序敏感的 race 测试，在满载并行的 ~10k 测试 CI 下被饿死。
所以合并受阻并非本 PR 的缺陷。reviewDecision = CHANGES_REQUESTED 是早期 bot review 留下的粘滞状态（100 条 inline thread 全部已 resolve；最新一次自动 review 是 COMMENT — "Suggestions only, no blockers"）。建议：重跑 ubuntu leg + 维护者 dismiss/re-review。
🧪 真实端到端证据（真实二进制、隔离 HOME、mock WS 平台）
| # | 行为 | 证据 |
|---|---|---|
| 1 | channel daemon-worker 拒绝直接调用 |
daemon-worker is an internal qwen serve command. |
| 2 | worker 在 HTTP listener ready 之后才启动 | listening on …:65163（日志第 2 行）→ Connecting "my-plugin-test"（第 12 行）→ connected（第 15 行） |
| 3 | 独立进程 worker（崩溃隔离） | 3 个不同 PID：daemon、--acp 子进程、channel daemon-worker 子进程 |
| 4 | worker 连回 daemon + 连到平台 | worker 发 GET /capabilities；lsof 显示 worker 持有到 mock server 的 ESTABLISHED …->[::1]:9201 WS |
| 5 | pidfile 归属 | {"owner":"serve","pid":<serve>,"servePid":<serve>,"workerPid":<worker>,"channels":["my-plugin-test"]} |
| 6 | qwen channel status |
Channel service: managed by qwen serve (PID …) + Worker PID + uptime |
| 7 | qwen channel stop 拒绝 serve-owned |
managed by qwen serve (PID …). Stop qwen serve to stop channels. |
| 8 | qwen channel start <name> 拒绝冲突 |
Error: Channel service is managed by qwen serve (PID …). Stop the qwen serve process… |
| 9 | /daemon/status running 快照 |
runtime.channelWorker.state:"running"、channels、requestedChannels、pid |
| 10 | --channel all |
解析到已配置 channel，单个 worker 托管全部 |
| 11 | worker 崩溃（SIGKILL）→ daemon 存活 | daemon 不倒；status warning，issue channel_worker_exited，state:"exited"，signal:"SIGKILL"；onExit 删除 pidfile + daemon.log WARN |
| 12 | daemon 优雅关闭（SIGINT） | worker 一并回收，无孤儿进程，pidfile 删除 |
代表性原始输出
# (3) 进程隔离 — daemon + ACP 子进程 + 独立 worker
63995 node …/index.js serve --port 0 … --channel my-plugin-test
64101 node …/index.js --acp
64102 node …/index.js channel daemon-worker --channel my-plugin-test
# (4) worker 到 mock 平台的实时 WebSocket
node 64102 … TCP [::1]:62552->[::1]:9201 (ESTABLISHED)
# (11) kill -9 <worker> 之后 — daemon 仍在服务 /daemon/status
overall status: warning
issues: [{ "code": "channel_worker_exited", "severity": "warning",
"message": "Channel worker is exited (pid=64102, code=null, signal=SIGKILL)." }]
runtime.channelWorker: { "state": "exited", "signal": "SIGKILL", … }
# daemon.log: [WARN] [DAEMON] channel worker exited (state=exited, pid=64102, code=null, signal=SIGKILL, error=none)
变异测试（证明测试非空过）：
channel-worker-supervisor.ts的ready ? 'exited' : 'failed'→ 强制'exited'⇒ 4 个测试挂。daemon-status.ts的channel_worker_exited守卫 → 关掉 ⇒ 1 个测试挂（reports failed channel worker snapshots）。
未覆盖（本轮范围外）
- 经过模型的完整消息往返（我验证的是 worker↔daemon↔平台的传输/生命周期，不是模型回复——隔离 home 无凭证）。
- token 鉴权的 worker 路径（用的是 loopback 默认无 token）。
- 仅 macOS（本地未跑 Windows/Linux，与 PR 的
⚠️矩阵一致）。
建议
从实测看，该功能正确且健壮——启动顺序、进程隔离、pidfile 归属、状态上报、崩溃韧性、优雅回收都符合描述，评审反馈的增量也是行为保持的。剩下的合并阻塞是流程而非代码：(1) 无关的 flaky voice CI 测试 → 重跑 ubuntu；(2) 粘滞的 CHANGES_REQUESTED → 维护者 dismiss/re-review。
Verified locally on macOS with a real build at 5e3ed1b18; isolated HOME, plugin-example channel + mock WS server, tmux panes.
wenshao
requested changes
Jul 1, 2026
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
doudouOUC
requested review from
DragonnZhang,
qqqys,
qwen-code-ci-bot and
wenshao
July 1, 2026 00:38
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Code Review Summary (4 Suggestions)
| # | Severity | File | Line | Finding |
|---|---|---|---|---|
| 1 | Suggestion | daemon-worker.ts | 478 | onEarlyShutdown does not abort startup |
| 2 | Suggestion | run-qwen-serve.ts | 2476 | Shared runtime startup timer budget |
| 3 | Suggestion | pidfile.ts | 161 | writeServeServiceInfo resets startedAt |
| 4 | Suggestion | channel-worker-supervisor.ts | 376 | stop() retains stale error field |
Deterministic checks: 0 findings in changed files (tsc, eslint).
Test suite: 516/516 tests pass across 12 test files.
Review by qwen3.7-max · /review workflow
| const completeRuntimeStartup = async (): Promise<void> => { | ||
| if (runtimeStartupSettled) return; | ||
| if (opts.channelSelection) { | ||
| await channelWorker.start(); |
Collaborator
There was a problem hiding this comment.
[Suggestion] armRuntimeStartupTimer() at L2462 starts a 120 s countdown that covers both bridge initialization and channelWorker.start(). The channel worker itself has a 30 s startup timeout (DEFAULT_CHANNEL_WORKER_STARTUP_TIMEOUT_MS), so in the worst case the combined budget is 120 s + 30 s = 150 s — but the runtime timer fires at 120 s regardless, producing a misleading "Runtime did not start within 120 s" error when the real bottleneck is the channel worker.
Consider either:
- Starting the runtime timer only after
channelWorker.start()resolves/rejects, or - Using a separate timer for channel-worker startup so the two budgets are independent.
— via Qwen Code /review
| const info: ServiceInfo = { | ||
| owner: 'serve', | ||
| pid: servePid, | ||
| startedAt: new Date().toISOString(), |
Collaborator
There was a problem hiding this comment.
[Suggestion] writeServeServiceInfo unconditionally sets startedAt: new Date().toISOString() every time it is called. The daemon calls reserveServeServiceInfo (which also sets startedAt) early in startup, then later calls writeServeServiceInfo once the channel worker becomes ready — at which point startedAt is reset to the worker-ready time, losing the original daemon start time.
This makes uptime reporting inaccurate. Consider preserving the original timestamp:
const existing = parseServiceInfo(JSON.parse(readFileSync(fd, 'utf-8')));
// ...
const info: ServiceInfo = {
// ...
startedAt: existing?.startedAt ?? new Date().toISOString(),
// ...
};— via Qwen Code /review
| } | ||
| child = undefined; | ||
| stopping = false; | ||
| snapshot = { ...snapshot, state: 'stopped' }; |
Collaborator
There was a problem hiding this comment.
[Suggestion] When stop() transitions to state: 'stopped', it spreads the previous snapshot ({ ...snapshot, state: 'stopped' }) but does not clear a stale error field. If the worker previously had a transient error that was resolved before shutdown (or if the snapshot was set to failed during startup and then stop() was called), the resulting snapshot retains the old error string.
Downstream consumers like daemon-status.ts check channelWorker.state for 'exited' or 'failed', so state: 'stopped' itself won't trigger false alarms — but any code that also inspects the error field would see a phantom error on a cleanly-stopped worker.
snapshot = { ...snapshot, state: 'stopped', error: undefined };
// or: delete snapshot.error before spreading— via Qwen Code /review
wenshao
approved these changes
Jul 1, 2026
Collaborator
|
@qwen-code /triage |
qwen-code-ci-bot
approved these changes
Jul 1, 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. ✅
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
This PR implements the PR2 daemon-managed channel worker path for #5976.
qwen servenow accepts repeatable--channel <name>and--channel all, starts one serve-owned out-of-process channel worker after the daemon runtime is ready, and connects that worker back to the current daemon through the TypeScript SDK andDaemonChannelBridge. The existing standaloneqwen channel startpath remains ACP-backed.The channel worker uses one shared
DaemonChannelBridgeand one sharedSessionRouterfor all selected channels, forces daemon session creation and load throughsessionScope: 'thread', gates optional shell command support on the daemon capability set, and cleans up adapters/router/bridge on worker shutdown.This also adds service ownership metadata for channel pidfiles, serve-owned conflict handling in
qwen channel start/stop/status, and aruntime.channelWorkersnapshot pluschannel_worker_exitedwarning in/daemon/status. Documentation now covers the experimental daemon-managed mode, the standalone ACP-backed mode, and the adapter-facingChannelAgentBridgeboundary.This PR also carries forward the PR1.1 cleanup from
683b982ed: session restore and channel cleanup logs are sanitized,shellCommanddispatch avoids a bridge capability TOCTOU read, and targeted router/start tests cover those fixes so PR2 does not reintroduce already-reviewed issues.Why it's needed
The daemon multi-channel roadmap needs a v1 path where
qwen serve --channel xxxactually hosts existing channel adapters through the daemon without moving adapter SDKs in-process. This keeps channel/platform SDK crashes isolated from the daemon, avoids wrapping the standalone ACP-backed channel service, and prevents multiple channel workers from racing on the sharedsessions.jsonrouter persistence file.Reviewer Test Plan
How to verify
Run
qwen serve --channel <configured-channel>and confirm the daemon starts, then the channel worker starts after the HTTP listener is ready. Runqwen serve --channel allwith multiple channel configs and confirm one worker owns all selected channels. Confirmqwen channel statusreports serve ownership while the daemon-managed worker is live, andqwen channel stoprefuses to signal it directly. If a ready worker exits, confirm/daemon/statusreportsruntime.channelWorker.state: "exited"and warning issue codechannel_worker_exitedwhile the daemon remains up.Evidence (Before & After)
N/A for screenshots. Local automated checks passed:
Tested on
Environment (optional)
Local macOS development checkout, Node/npm workspace commands.
npm run build -- --cli-onlyemits the existing Browserslist freshness and Nodemodule.register()deprecation warnings, but completed successfully.Risk & Scope
qwen channel start --daemon-url, and in-process adapter hosting are intentionally deferred.qwen channel startbehavior remains ACP-backed. Channel pidfiles now include anownerfield, and legacy pidfiles withoutownerare still treated as standalone channel services.Linked Issues
Part of #5976.
中文说明
What this PR does
这个 PR 实现 #5976 的 PR2 daemon-managed channel worker 路径。
qwen serve现在支持重复指定--channel <name>和--channel all，在 daemon runtime ready 之后启动一个由 serve 托管的独立 channel worker，并让 worker 通过 TypeScript SDK 和DaemonChannelBridge连接回当前 daemon。现有 standaloneqwen channel start仍然保持 ACP-backed 行为。channel worker 会为所有选中的 channel 共享一个
DaemonChannelBridge和一个SessionRouter，daemon session create/load 统一使用sessionScope: 'thread'，可选 shell command 能力按 daemon capability 暴露，并在 worker shutdown 时清理 adapter/router/bridge。这个 PR 还加入了 channel pidfile 的 service ownership metadata，处理
qwen channel start/stop/status遇到 serve-owned 服务时的冲突和提示，并在/daemon/status中新增runtime.channelWorker快照和channel_worker_exitedwarning。文档也同步说明 experimental daemon-managed mode、standalone ACP-backed mode，以及 adapter-facingChannelAgentBridge边界。这个 PR 也带上了
683b982ed里的 PR1.1 cleanup：session restore 和 channel cleanup 日志会先做清洗，shellCommanddispatch 避免 bridge capability 的 TOCTOU 读取，并补充 targeted router/start 测试，避免 PR2 重新踩到已 review 过的问题。Why it's needed
daemon 多 channel 路线需要一个 v1 闭环，让
qwen serve --channel xxx真正通过 daemon 托管现有 channel adapters，同时不把 adapter SDK 放进 daemon 进程内。这样可以隔离 channel/platform SDK 崩溃，避免只是包装 standalone ACP-backed channel service，也避免多个 worker 同时写共享sessions.jsonrouter 持久化文件导致互相覆盖。Reviewer Test Plan
How to verify
运行
qwen serve --channel <configured-channel>，确认 daemon 先启动，HTTP listener ready 后 channel worker 再启动。用多个 channel config 运行qwen serve --channel all，确认一个 worker 托管所有选中 channel。daemon-managed worker 存活时，确认qwen channel status展示 serve ownership，qwen channel stop不会直接 signal worker。如果 ready 后 worker 异常退出，确认/daemon/status仍可访问，且返回runtime.channelWorker.state: "exited"和 warning issue codechannel_worker_exited。Evidence (Before & After)
无截图。已通过以下本地自动化验证：
Tested on
Environment (optional)
本地 macOS 开发 checkout，使用 Node/npm workspace 命令。
npm run build -- --cli-only仍会打印现有 Browserslist 数据过期和 Nodemodule.register()deprecation warning，但构建成功。Risk & Scope
qwen channel start --daemon-url、in-process adapter hosting 都刻意留到后续。qwen channel start仍然是 ACP-backed。channel pidfile 新增owner字段；没有owner的 legacy pidfile 仍按 standalone channel service 处理。Linked Issues
Part of #5976.