feat(cli): Harden daemon-managed channel worker - #6098
Merged
Conversation
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Hardens the daemon-managed channel worker lifecycle used by qwen serve --channel by adding post-ready supervision (bounded restarts + heartbeat stale detection), forwarding redacted worker logs into the daemon log, and exposing richer worker operational status fields—while keeping the public CLI surface unchanged.
Changes:
- Add bounded post-ready restart supervision, heartbeat monitoring, and stdout/stderr redacted log forwarding to the channel worker supervisor.
- Update
runQwenServeto write serve-owned pidfile metadata on worker ready/exit and forward worker log lines into daemon logs. - Extend
/daemon/statusruntime issue details and document the new worker lifecycle/status behavior.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/cli/src/serve/run-qwen-serve.ts | Wires supervisor callbacks for pidfile updates and daemon log forwarding. |
| packages/cli/src/serve/run-qwen-serve.test.ts | Adds coverage for pidfile updates on restart-ready and worker log forwarding. |
| packages/cli/src/serve/daemon-status.ts | Includes restart/heartbeat fields in channel worker warning details. |
| packages/cli/src/serve/daemon-status.test.ts | Updates expectations and adds a “restarted successfully” running-worker case. |
| packages/cli/src/serve/channel-worker-supervisor.ts | Implements restart policy, heartbeat stale detection, and redacted worker log piping. |
| packages/cli/src/serve/channel-worker-supervisor.test.ts | Adds extensive tests for restarts, heartbeats, spawn failures, and log redaction/forwarding. |
| packages/cli/src/commands/channel/status.test.ts | Ensures status output omits worker PID when serve-owned metadata has no live worker PID. |
| packages/cli/src/commands/channel/daemon-worker.ts | Adds IPC heartbeat messages from the worker process and clears them on shutdown/disconnect. |
| packages/cli/src/commands/channel/daemon-worker.test.ts | Adds tests validating heartbeat emission and cleanup behavior. |
| docs/developers/qwen-serve-protocol.md | Documents restart/heartbeat policy, new status fields, and pidfile/worker log behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Collaborator
|
Thanks for the PR! Template looks good ✓ On direction: this is solid operational hardening for the daemon-managed channel worker path. Bounded restart supervision, heartbeat monitoring, and log forwarding with redaction are exactly the kind of reliability work a long-running daemon needs. Clearly aligned with the On approach: the scope is large (2541 additions, 15 files) but coherent — restart supervision, heartbeat monitoring, log forwarding/redaction, and pidfile management are all tightly coupled to the "make the worker lifecycle observable and recoverable" goal. The Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 方向：对 daemon-managed channel worker 做运维硬化——有界重启监督、心跳监测、带脱敏的日志转发——正是长运行 daemon 需要的可靠性工作。与 方案：范围较大（2541 行新增，15 个文件）但内聚——重启监督、心跳监测、日志转发/脱敏和 pidfile 管理都紧密耦合到"让 worker 生命周期可观测且可恢复"的目标。 进入代码审查 🔍 — 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 ReviewIndependently, I'd approach this the same way: extract the launch logic into a parameterized function for initial vs restart, add bounded restart with escalating backoff, pipe-based log forwarding with redaction, and heartbeat monitoring via IPC messages. The PR's implementation matches this approach well. The supervisor refactor is clean — The pidfile change to preserve No correctness bugs or security issues found. The Test ResultsUnit tests: 257/257 passed across 7 test files:
Typecheck: clean. Real-Scenario TestingCLI surface smoke test (built from PR branch): CLI surface is unchanged as claimed — 中文说明代码审查独立来看，我会采取相同方案：将 launch 逻辑提取为参数化函数（initial vs restart），添加有界重启和递增退避、基于 pipe 的日志转发和脱敏，以及通过 IPC 消息的心跳监测。PR 的实现与这个方案吻合。 supervisor 重构干净—— pidfile 变更保留 未发现正确性 bug 或安全问题。 测试结果单测： 257/257 通过，覆盖 7 个测试文件。类型检查： 通过。 实际场景测试CLI 表面烟雾测试： — Qwen Code · qwen3.7-max |
Collaborator
|
This is a well-executed operational hardening PR. The motivation is real — daemon-managed channel workers that crash or hang after ready were not recoverable or observable enough for production use. The implementation delivers exactly what the description promises: bounded restart supervision, heartbeat monitoring, log forwarding with redaction, and richer status reporting. The code is straightforward for what it does. The My independent proposal would have been simpler on the supervisor side (probably a basic retry loop without the sliding window), but the PR's sliding-window restart budget is the right call for a long-running daemon — it prevents restart storms without permanently disabling recovery. 257 tests pass, typecheck clean, CLI surface unchanged. The unit tests use fake timers to cover timing-sensitive scenarios (restart delays, heartbeat timeouts, shutdown races) that would be hard to test with real processes. The Approving. ✅ 中文说明这是一个执行良好的运维硬化 PR。动机真实——post-ready 崩溃或挂起的 daemon-managed channel worker 在生产使用中不够可恢复和可观测。实现精确交付了描述中承诺的内容：有界重启监督、心跳监测、带脱敏的日志转发、更丰富的状态报告。 代码对所做之事保持简洁。 257 测试通过，类型检查通过，CLI 表面未变。考虑到范围， 批准 ✅ — Qwen Code · qwen3.7-max |
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. ✅
qwen-code-ci-bot
requested changes
Jul 1, 2026
wenshao
requested changes
Jul 1, 2026
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
wenshao
requested changes
Jul 1, 2026
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
wenshao
requested changes
Jul 1, 2026
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
DragonnZhang
reviewed
Jul 1, 2026
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
wenshao
reviewed
Jul 1, 2026
wenshao
reviewed
Jul 1, 2026
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
wenshao
requested changes
Jul 1, 2026
wenshao
requested changes
Jul 1, 2026
Collaborator
✅ Local real-binary verification report (maintainer)I built the PR head into a real Environment: macOS (Darwin 25.5.0), Node v22.22.2, isolated 1. Build & unit tests
2. E2E baseline (
|
| Assertion | Evidence |
|---|---|
| Startup stays fail-fast / ordered | listening on http://127.0.0.1:61709 is printed before [Channel] Connecting "my-plugin-test"… / connected (worker started only in completeRuntimeStartup) |
| Out-of-process isolation (3 PIDs) | serve 7655 · --acp child 8221 · channel daemon-worker child 8226 (all PPID=7655) |
| Pidfile | {"owner":"serve","pid":7655,"servePid":7655,"workerPid":8226,"channels":["my-plugin-test"]} |
| Worker connected to platform | worker 8226 → TCP [::1]:61721->[::1]:9201 (ESTABLISHED) |
/daemon/status |
state:"running", restartCount:0, pid:8226, lastHeartbeatAt populated |
| Ownership commands | channel status → managed by qwen serve (PID 7655) + Worker PID: 8226 + uptime; channel stop & channel start both refuse with the serve-managed message |
3. Worker log forwarding + redaction (NEW)
Worker output is now piped and forwarded into the daemon log (previously inherit):
[INFO] [DAEMON] channel worker stdout: [Channel] Connecting "my-plugin-test"...
[INFO] [DAEMON] channel worker stdout: [Channel] "my-plugin-test" connected.
[WARN] [DAEMON] channel worker stderr: (node:8226) [DEP0040] DeprecationWarning: ...
stdout → daemonLog.info, stderr → daemonLog.warn, each prefixed channel worker <stream>: ✅
Redaction — driving the compiled dist supervisor with a fake child emitting secrets (real redaction code, not a mock):
stderr: failed with <redacted> # daemon bearer token
stderr: split <redacted> # zero-width-split token (secret-token)
stdout: adapter token <redacted> # TELEGRAM_BOT_TOKEN value
stdout: redis <redacted> # REDIS_PASSWORD value
stdout: auth <redacted> # BASIC_AUTH value
stderr: proxy http://<redacted>@proxy.example:8080/path # URL credentials (host preserved)
verdict: leaked plaintext secrets = NONE, ssword gone. ✅
4. Bounded post-ready restart supervision (NEW — headline)
Single crash → recovered: kill -9 the ready worker 8226 →
- daemon survives; new worker 28374 spawned (different PID) and re-
ESTABLISHEDto WS :9201 - pidfile
workerPidcleared → restored: tight sampling caughtworkerPid=CLEARED(owner staysserve) while no live/ready worker, thenworkerPid=28374once the restarted worker reported ready.channel statusshows noWorker PID:line during the gap (guarded byinfo.workerPid !== undefined). /daemon/status:restartCount:1,lastExitAt:…30.280Z,lastRestartAt:…31.281Z— exactly 1001 ms apart =delaysMs[0](1 s) ✅- daemon log:
channel worker exited (state=exited, pid=8226, code=null, signal=SIGKILL, restartCount=0, nextRestartAt=…31.280Z, staleHeartbeatAt=none)then new-worker connect lines.
Budget is bounded (maxRestarts=3): kill the worker repeatedly →
| kill | result |
|---|---|
| #1–#3 | restarted, restartCount 1 → 2 → 3, new PIDs 53782 → 56767 |
| #4 | state:"failed", error:"Channel worker restart budget exhausted.", no live worker, serve daemon 7655 still alive |
Failed-state /daemon/status warning carries all additive fields: channel_worker_exited … (…, restarts=3, lastExitAt=…, lastRestartAt=…, lastHeartbeatAt=…): Channel worker restart budget exhausted. (severity warning, a running restarted worker produces no warning since the warn guard only fires for exited/failed).
5. Heartbeat (NEW)
Worker emits {type:'heartbeat'} every 15 s → supervisor records lastHeartbeatAt, surfaced live in /daemon/status (observed on both the initial and the restarted worker). Stale-heartbeat SIGKILL path is covered by unit + mutation (below).
6. Graceful shutdown
SIGINT the daemon → serve + --acp + daemon-worker all exit, no orphan daemon-worker, pidfile removed. ✅
7. Mutation proofs (tests are non-vacuous)
| Mutation (source) | Expected | Observed |
|---|---|---|
A redactWorkerLogLine → return raw line |
redaction test red | ✅ FAIL — failed with secret-token leaked |
B canScheduleRestart → return true |
budget test red | ✅ FAIL — spy called 12× instead of 2× (infinite restart) |
C drop kill('SIGKILL') on stale heartbeat |
stale-HB tests red | ✅ FAIL — 2 stale-heartbeat tests red; unrelated HB tests stay green (targeted) |
All mutations reverted; source restored clean.
Verdict
LGTM from local verification. Bounded post-ready restart, IPC heartbeat, redacted worker-log forwarding, additive /daemon/status fields, and serve-owned pidfile workerPid clear/restore all behave exactly as described on the real binary; pre-ready startup stays fail-fast and the public command surface is unchanged. The only local hiccup (vscode-ide-companion type-check) is an environment artifact outside this PR.
Verified against PR head 2dfc83bbd · real compiled binary + tmux + mock WS platform · unit (213) + compiled-dist redaction harness + 3 mutation proofs.
中文版（完整对应）
✅ 本地真实二进制验证报告（维护者）
我把 PR head 构建成真实 qwen 二进制，用 tmux + 免凭据的 plugin-example channel + mock WebSocket 平台（HTTP 9200 / WS 9201）做端到端验证，并对时序敏感路径（脱敏、心跳、重启预算）用编译产物 harness + 定向变异测试兜底。PR 声称的行为全部成立。
环境：macOS（Darwin 25.5.0）、Node v22.22.2、隔离 HOME、PR head 2dfc83bbd 的独立 worktree（git diff --check 干净，head 与 pull/6098/head 一致）。
1. 构建与单元测试
- 全量构建 → 可运行的
packages/cli/dist/index.js✅（--version正常） - 6 个改动测试文件
vitest→ ✅ 213 passed check:serve-fast-path-bundle（本 PR 改了 fast-path 根run-qwen-serve.ts）→ ✅ 通过；git diff --check干净- 注：
packages/vscode-ide-companion在我沙箱里 type-check 失败（缺@types/vscode），该包不在本 PR diff 内、与改动无关；PR 涉及的core/acp-bridge/channels/*/cli均构建干净。
2. E2E 基线（qwen serve --port 0 --channel my-plugin-test）
- 启动顺序/仍 fail-fast：
listening on …:61709打印在[Channel] Connecting/connected之前（worker 只在 runtime ready 后于completeRuntimeStartup启动） - 进程隔离（3 PID）：
serve7655 ·--acp8221 ·channel daemon-worker8226（PPID 均 7655） - pidfile：
{"owner":"serve","pid":7655,"servePid":7655,"workerPid":8226,"channels":["my-plugin-test"]} - worker 连上平台：8226 →
[::1]:61721->[::1]:9201 (ESTABLISHED) /daemon/status：state:"running"、restartCount:0、pid:8226、lastHeartbeatAt已填充- 归属命令：
channel status→managed by qwen serve (PID 7655)+Worker PID: 8226+ uptime；channel stop/channel start均拒绝
3. worker 日志转发 + 脱敏（新增）
worker 输出现改为 pipe 并转发进 daemon log（原为 inherit）：stdout→daemonLog.info、stderr→daemonLog.warn，前缀 channel worker <stream>: ✅。
脱敏——用假 child 向编译后的 dist supervisor喂密文（真实脱敏代码，非 mock）：daemon token、零宽字符分割 token、敏感环境变量值、URL 凭据（http://<redacted>@host 保留主机）全部 <redacted>；无明文泄漏（ssword 消失）✅。
4. 有界 post-ready 重启监督（新增——核心）
单次崩溃→恢复：kill -9 ready 的 8226 → daemon 幸存、新 worker 28374（PID 不同）重连 WS :9201；pidfile workerPid 先清后恢复（紧密采样捕捉到 CLEARED（owner 仍 serve）→ ready 后 28374，间隙内 channel status 不打印 Worker PID:）；/daemon/status：restartCount:1、lastExitAt…30.280Z、lastRestartAt…31.281Z——恰好相差 1001ms = delaysMs[0]（1s） ✅；daemon log 记 channel worker exited (…signal=SIGKILL…nextRestartAt=…)。
预算有界（maxRestarts=3）：#1–#3 次 kill 都在预算内重启（restartCount 1→2→3）；第 4 次 → state:"failed"、error:"Channel worker restart budget exhausted."，无 live worker、serve daemon 7655 仍存活。failed 态告警消息带全部新增字段，且已恢复运行的 worker 不产生告警（告警只对 exited/failed 触发）。
5. 心跳（新增）
worker 每 15s 发 {type:'heartbeat'} → supervisor 记录 lastHeartbeatAt，在 /daemon/status 实时可见（初始与重启后 worker 均观测到）。stale-heartbeat 的 SIGKILL 路径由单测 + 变异兜底。
6. 优雅关闭
SIGINT daemon → serve+--acp+daemon-worker 全部退出、无孤儿 daemon-worker、pidfile 被移除 ✅。
7. 变异证明（测试非空过）
- A 脱敏改 no-op → 脱敏测试翻红（
secret-token泄漏）✅ - B
canScheduleRestart恒 true → 预算测试翻红（spy 被调 12 次 而非 2 次）✅ - C stale heartbeat 去掉
kill('SIGKILL')→ 2 个 stale-heartbeat 测试翻红，不相关心跳测试仍绿（定向）✅ - 所有变异已还原，源码干净。
结论
本地验证通过，LGTM。 有界 post-ready 重启、IPC 心跳、脱敏 worker 日志转发、/daemon/status 新增字段、serve-owned pidfile workerPid 清除/恢复，在真实二进制上均与描述一致；ready 前启动仍 fail-fast，公开命令面不变。唯一本地异常（vscode-ide-companion type-check）是环境问题，与本 PR 无关。
验证基于 PR head 2dfc83bbd · 真实编译二进制 + tmux + mock WS 平台 · 单测（213）+ 编译 dist 脱敏 harness + 3 项变异证明。
qwen-code-ci-bot
requested 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.
Code Review Summary
Overall this is solid operational hardening work — the restart supervision, heartbeat monitoring, and log forwarding are well-structured, and the test coverage is thorough (210 tests passing). One critical security gap and a few minor suggestions below.
Critical
sanitizeWorkerErrordoesn't redact secrets —redactWorkerLogLinecorrectly strips bearer tokens, env secrets, and proxy credentials from worker stdout/stderr, butsanitizeWorkerError(which setssnapshot.error) only callssanitizeLogText. Error messages containing secrets flow throughhandleRestartFailure,settleError, andfailBeforeReadyintosnapshot.error, which is then served by/daemon/statusunredacted.
Suggestions
- Restart budget exhausted message masked by
??operator whenhandleRestartFailurealready seterror. - Heartbeat PID mismatch guard (
message.pid !== currentPid) has no test coverage. notifyReadythrowing silently is untested.
Verdict: Request Changes (1 critical finding needs addressing before merge).
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
DragonnZhang
reviewed
Jul 1, 2026
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Collaborator
|
Qwen Code review timed out. Qwen review timed out after 120 minutes. For large PRs, retry with a longer timeout by commenting: |
Collaborator
Author
|
@qwen-code /review --timeout=180 |
Contributor
| _Qwen Code review request accepted. Review is queued in [workflow run](https://github.com/QwenLM/qwen-code/actions/runs/28508064954)._ |
DragonnZhang
reviewed
Jul 1, 2026
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
wenshao
reviewed
Jul 1, 2026
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
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. ✅
yiliang114
approved these changes
Jul 1, 2026
wenshao
requested changes
Jul 1, 2026
| } | ||
| snapshot = { | ||
| ...snapshot, | ||
| lastHeartbeatAt: message.at ?? new Date().toISOString(), |
Collaborator
There was a problem hiding this comment.
[Critical] This trusts the heartbeat timestamp supplied by the worker and later exposes it through /daemon/status. Channel adapters and plugins run inside that worker process, so a compromised or buggy adapter can send process.send({ type: 'heartbeat', pid: process.pid, at: process.env.TELEGRAM_BOT_TOKEN }) and bypass the new log/error redaction paths entirely. Please record the daemon's receive time here instead of reflecting worker-controlled data.
Suggested change
| lastHeartbeatAt: message.at ?? new Date().toISOString(), | |
| lastHeartbeatAt: new Date().toISOString(), |
— GPT-5 via Qwen Code /review
Collaborator
Author
| .map((secret) => new RegExp(escapeRegExp(secret), 'g')); | ||
|
|
||
| return (line: string): string => { | ||
| let redacted = line.replace( |
Collaborator
There was a problem hiding this comment.
[Critical] The redactor only removes the exact daemon token or sensitive env values, but the PR description promises bearer tokens are redacted. If a worker logs an HTTP header or SDK trace like Authorization: Bearer live-token, that token is not in secretPatterns, so it is forwarded into the daemon log. Please add a generic bearer/authorization-token redaction pass before emitting worker log lines, with a regression test.
— GPT-5 via Qwen Code /review
Collaborator
Author
There was a problem hiding this comment.
Fixed in PR #6146 (commit 87eab8f). Added redactLogCredentials as a pattern-based redaction pass in createWorkerLogRedactor, covering Bearer/QQBot tokens, Authorization headers, API key prefixes (sk-, ghp_, glpat-, xoxb-), AWS key IDs, JSON secret fields, and DingTalk headers. Includes 34 unit tests.
| ...(opts.daemonToken && opts.daemonToken.length >= 4 | ||
| ? [opts.daemonToken] | ||
| : []), | ||
| ...sensitiveEnvValues(opts.workerEnv), |
Collaborator
There was a problem hiding this comment.
[Critical] Sensitive env values are added to the redaction set only as complete strings, but worker output is split and redacted one line at a time. Common secrets such as PEM private keys span multiple lines, so none of the individual logged lines match the full env value and the key material can still be written to the daemon log. Please normalize/split sensitive env values and redact non-trivial line fragments as well as the full value.
— GPT-5 via Qwen Code /review
Collaborator
Author
Collaborator
✅ Local real-binary verification (tmux E2E) — ready to mergeVerified as maintainer by building the real
Environment
1. Static + unit + mutation
Mutations reverted; suite re-confirmed green afterward. 2. Real-binary tmux E2E (all observed with the shipped
|
| # | Behavior | Evidence |
|---|---|---|
| 1 | Startup fail-open ordering | daemon listening on …:57446 → worker GET /capabilities (13:30:39.314) → [Channel] "my-plugin-test" connected (13:30:39.409) |
| 2 | stdio pipe → worker logs forwarded to daemon.log |
channel worker stdout: [Channel] Connecting/connected (INFO) and channel worker stderr: … DeprecationWarning (WARN) land in daemon.log, not raw stdout |
| 3 | Out-of-process isolation (3 PIDs) | children of serve 66112: --acp child 66193 + channel daemon-worker child 66194 |
| 4 | Worker → platform | worker holds ESTABLISHED 127.0.0.1:57450 -> 127.0.0.1:9201 (live WS to mock server) |
| 5 | Pidfile ownership | {owner:"serve", pid:66112, servePid:66112, workerPid:66194, channels:["my-plugin-test"]} |
| 6 | channel status surface unchanged |
managed by qwen serve (PID 66112) + Worker PID: 66194 + uptime |
| 7 | Heartbeat auto-populated | /daemon/status runtime.channelWorker.lastHeartbeatAt advances on the worker's 15s IPC heartbeat; healthy worker ⇒ status:ok, issues:[] (no warning) |
| 8 | Crash resilience + restart | kill -9 worker ⇒ daemon survives; new worker PID 75243; restartCount 0→1; pidfile keeps owner:serve but clears workerPid for ~1s then restores it; lastExitAt → lastRestartAt gap = 1002 ms (delaysMs[0]) |
| 9 | Escalating restart delays | restartCount 1→2→3 with delays ≈ 1s → 5s → 15s |
| 10 | Restart budget exhaustion → ESCALATION (new) | 4th kill -9 ⇒ state:failed, restartCount:3, nextRestartAt:none, error:"Channel worker restart budget exhausted."; /daemon/status rolls up to status:error with issue severity:error (was warning before the review-feedback delta); daemon stays alive, no live worker; pidfile workerPid cleared; channel status omits the Worker PID: line |
| 11 | Stale-heartbeat detection (hung-but-alive worker) | SIGSTOP the worker (IPC stays open, no exit event) ⇒ after ~45s supervisor sets staleHeartbeatAt, error:"Channel worker heartbeat timed out.", SIGKILLs the frozen worker and restarts it (new PID 13485) — the only code path that detects this hang |
| 12 | Secret redaction (compiled dist) |
fed env secrets / daemonToken / scheme://user:pass@host / a zero-width-split token through the shipped supervisor → all emitted as <redacted>, zero plaintext leaked |
| 13 | Graceful shutdown | SIGINT ⇒ daemon + live worker both gone in ~0.5s, no orphan daemon-worker, pidfile removed (verified for both live-worker and terminal-failure states) |
Verdict
The worker lifecycle is observable and recoverable end-to-end: bounded restart with escalating backoff, stale-heartbeat kill/restart, redacted log forwarding, additive /daemon/status fields, and the new permanent-failure error escalation all behave exactly as described — while the daemon process itself never crashes and the public CLI surface is unchanged. Tests are non-vacuous (3/3 mutations load-bearing). No blocking issues found; LGTM to merge.
🇨🇳 中文版
✅ 本地真实二进制验证（tmux E2E）——可以合并
作为维护者，我在当前 PR head 构建了真实的 qwen 二进制，用 tmux 托管真实的 qwen serve --channel daemon，对接免凭据的 plugin-example channel + mock WebSocket server 做端到端验证。PR 描述里的每一条硬化声明都用真实进程、真实 IPC、真实信号、真实 daemon 日志复现了——不是 mock。
⚠️ 说明：分支自上一次验证（2dfc83bbd）以来又推进了 7 个提交到8fe4b9c15，所以这是一次全新复验。review 反馈的增量给 PR 加了真实新行为（永久失败升级为 error、收紧的敏感环境变量正则、预编译脱敏器、有界的超长行尾丢弃、输入校验守卫、Unicode 属性不可见字符剥离），下面都重新核验过。
环境
| Head | 8fe4b9c15476f98e49066370fcd525828d855439 |
| 净 diff | 15 个文件（packages/cli/**、packages/channels/base/**、docs） |
| 平台 | macOS (Darwin 25.5)、Node v22.22.2 |
| 方法 | worktree 构建真实 dist/index.js + tmux 托管 daemon（nohup）+ mock WS 9201 + plugin-example channel |
1. 静态 + 单测 + 变异测试
| 检查 | 结果 |
|---|---|
tsc --build cli / core / channels-base |
✅ 干净（dist 已产出）——唯一的构建报错是 vscode-ide-companion 缺 @types/vscode（我沙箱里没克隆它的嵌套 node_modules），与本 PR 无关（PR 未改任何 vscode 文件；真实 CI 跑完整 npm ci） |
git diff --check |
✅ 干净 |
| 单测——6 个 cli PR 文件 | ✅ 226 通过 |
单测——channels/base sanitize.test.ts |
✅ 31 通过 |
变异 A——绕过 createWorkerLogRedactor（返回原始行） |
✅ forwards … secrets redacted 变红（测试承重） |
变异 B——canScheduleRestart 恒 true |
✅ stops restarting after restart budget is exhausted 变红 |
变异 C——daemon-status severity 恒 warning |
✅ reports permanently failed … as errors 变红 |
变异已还原，之后重跑套件重新变绿。
2. 真实二进制 tmux E2E（全部用发布 dist 观测）
| # | 行为 | 证据 |
|---|---|---|
| 1 | 启动顺序 fail-open | daemon listening on …:57446 → worker GET /capabilities（13:30:39.314）→ [Channel] "my-plugin-test" connected（13:30:39.409） |
| 2 | stdio 改 pipe → worker 日志转发到 daemon.log |
channel worker stdout: [Channel] Connecting/connected（INFO）、channel worker stderr: … DeprecationWarning（WARN）落到 daemon.log，不再是裸 stdout |
| 3 | 进程外隔离（3 个 PID） | serve 66112 的子进程：--acp 子进程 66193 + channel daemon-worker 子进程 66194 |
| 4 | worker → 平台 | worker 持有 ESTABLISHED 127.0.0.1:57450 -> 127.0.0.1:9201（连 mock server 的真实 WS） |
| 5 | pidfile 归属 | {owner:"serve", pid:66112, servePid:66112, workerPid:66194, channels:["my-plugin-test"]} |
| 6 | channel status 命令面不变 |
managed by qwen serve (PID 66112) + Worker PID: 66194 + uptime |
| 7 | 心跳自动填充 | /daemon/status 的 lastHeartbeatAt 随 worker 15s IPC 心跳推进；健康 worker ⇒ status:ok, issues:[]（无 warning） |
| 8 | 崩溃恢复 + 重启 | kill -9 worker ⇒ daemon 存活；新 worker PID 75243；restartCount 0→1；pidfile 保留 owner:serve 但 workerPid 清空约 1s 后恢复；lastExitAt → lastRestartAt 间隔 = 1002 ms（delaysMs[0]） |
| 9 | 递增重启延迟 | restartCount 1→2→3，延迟 ≈ 1s → 5s → 15s |
| 10 | 重启预算耗尽 → 升级（新） | 第 4 次 kill -9 ⇒ state:failed、restartCount:3、nextRestartAt:none、error:"Channel worker restart budget exhausted."；/daemon/status 汇总为 status:error，issue severity:error（review 增量前是 warning）；daemon 仍存活、无 live worker；pidfile workerPid 已清；channel status 不再打印 Worker PID: 行 |
| 11 | 心跳僵死检测（活着但卡死的 worker） | SIGSTOP worker（IPC 不断、无 exit 事件）⇒ 约 45s 后 supervisor 记 staleHeartbeatAt、error:"Channel worker heartbeat timed out."、SIGKILL 冻结的 worker 并重启（新 PID 13485）——这是唯一能发现此类卡死的路径 |
| 12 | 密钥脱敏（编译 dist） |
把环境密钥 / daemonToken / scheme://user:pass@host / 一个零宽字符拆分的 token 喂给发布版 supervisor → 全部输出 <redacted>，零明文泄漏 |
| 13 | 优雅关闭 | SIGINT ⇒ daemon + live worker 都在 ~0.5s 内退出，无孤儿 daemon-worker，pidfile 被删（live-worker 和终态失败两种情况都验了） |
结论
worker 生命周期端到端可观测、可恢复：有界重启 + 递增退避、心跳僵死 kill/重启、脱敏日志转发、/daemon/status additive 字段、以及新增的永久失败 error 升级，全部与描述一致；同时 daemon 进程本身从不崩溃、公开 CLI 面不变。测试非空过（3/3 变异承重）。未发现阻断性问题，LGTM 可合并。
| restartTimer = undefined; | ||
| void launch('restart').catch((err: unknown) => { | ||
| handleRestartFailure(err instanceof Error ? err.message : String(err)); | ||
| }); |
Collaborator
There was a problem hiding this comment.
[Suggestion] The .catch handler calls handleRestartFailure without passing redaction options. The redaction variable is scoped to launch() and inaccessible here. If launch rejects during a restart (e.g., createWorkerEnv throws with env values in the error message), the error is sanitized (ANSI/control chars stripped) but worker env secrets are NOT redacted before being stored in snapshot.error and surfaced via /daemon/status or onExit.
Consider computing redaction at supervisor scope or reconstructing it in the catch:
Suggested change
| }); | |
| void launch('restart').catch((err: unknown) => { | |
| handleRestartFailure( | |
| err instanceof Error ? err.message : String(err), | |
| workerLogRedactionOptions(opts.daemonToken, process.env), | |
| ); | |
| }); |
— qwen3.7-max via Qwen Code /review
| lastHeartbeatAt: message.at ?? new Date().toISOString(), | ||
| }; | ||
| armStaleHeartbeatTimer(startedChild); | ||
| }; |
Collaborator
There was a problem hiding this comment.
[Suggestion] isReadyMessage only checks message.type === 'ready' without validating that channels or requestedChannels are actually arrays. In completeReady, the code does [...message.channels] — if a buggy or compromised worker sends channels: { length: 5 } (non-iterable with .length > 0), the spread throws a TypeError. Since handleMessage has no try/catch wrapper, this uncaught exception would crash the entire daemon process.
Consider adding Array.isArray validation:
Suggested change
| }; | |
| return ( | |
| typeof message === 'object' && | |
| message !== null && | |
| (message as { type?: unknown }).type === 'ready' && | |
| ((message as { channels?: unknown }).channels === undefined || | |
| Array.isArray((message as { channels?: unknown }).channels)) | |
| ); |
— qwen3.7-max via Qwen Code /review
| const redaction = workerLogRedactionOptions(opts.daemonToken, env); | ||
| const requestedChannels = requestedChannelNames(opts.selection); | ||
| const startedAt = new Date().toISOString(); | ||
| snapshot = { |
Collaborator
There was a problem hiding this comment.
[Nice to have] launch() replaces the entire snapshot with a manual whitelist of carried fields (restartCount, lastExitAt, lastHeartbeatAt, staleHeartbeatAt). Any future field added to ChannelWorkerSnapshot will silently vanish after the first restart unless it's also added here. Since all snapshot fields are optional, TypeScript won't flag a missing spread.
Consider using a delete-based approach instead (spread the previous snapshot, then delete fields that should be reset) to make the default behavior "carry forward" rather than "silently drop."
— qwen3.7-max via Qwen Code /review
| return restartAttemptTimes.length < restartPolicy.maxRestarts; | ||
| }; | ||
|
|
||
| const nextRestartDelayMs = (): number => { |
Collaborator
There was a problem hiding this comment.
[Nice to have] nextRestartDelayMs() has an undocumented ordering dependency on pruneRestartAttempts() (called as a side effect inside canScheduleRestart()). It uses restartAttemptTimes.length as the index into delaysMs, which is only correct after pruning. A future refactor that reorders the calls in scheduleRestart — or moves the restartAttemptTimes.push() from launch('restart') into scheduleRestart — would silently corrupt the backoff schedule.
Consider passing the pruned count as an explicit parameter instead of reading the shared mutable array.
— qwen3.7-max via Qwen Code /review
| } | ||
|
|
||
| function sensitiveEnvValues(env: NodeJS.ProcessEnv): string[] { | ||
| const sensitiveKey = |
Collaborator
There was a problem hiding this comment.
[Nice to have] sensitiveEnvValues scans the full inherited process.env with a broad key pattern. Common env vars like SESSION_SECRET=true, AUTH_TOKEN=none, or API_KEY=1234 (length >= 4) become global substring redaction patterns applied to ALL worker log output. A worker logging "tls verification: true" would produce "tls verification: <redacted>" if any matching env var has value "true".
The minimum length filter (>= 4) catches "true", "none", "1234". Consider either raising the minimum length threshold, scoping the scan to daemon-controlled env vars only (rather than the full inherited process.env), or requiring the value to be non-dictionary-word-like.
— qwen3.7-max via Qwen Code /review
doudouOUC
added a commit
that referenced
this pull request
Jul 2, 2026
…v secrets Two fixes for unresolved review threads from #6098: - Heartbeat: use daemon's own `new Date().toISOString()` instead of reflecting the worker-supplied `message.at` value. Prevents a compromised adapter from injecting arbitrary data into `/daemon/status`. - PEM multiline: split multi-line sensitive env values (e.g. PEM keys) into per-line redaction patterns so individual logged lines match. The full value is kept as a pattern too for single-line matches.
yiliang114
added a commit
that referenced
this pull request
Jul 2, 2026
* feat(cli): add credential redaction for worker stderr forwarding Add a `redactLogCredentials` function that strips credentials from worker log lines before they reach the daemon's stderr and log file. Covers Bearer/QQBot tokens, Authorization headers, common API key prefixes, env-var secret assignments, URL-embedded credentials, JSON secret fields, and platform-specific headers (DingTalk). Integrate the redaction into both stderr forwarding paths: - ACP children: `createStderrForwarder` now applies redaction in both the normal flush and 64 KiB forced-truncation code paths. - Daemon channel worker: change supervisor stdio from `'inherit'` to `'pipe'` for stderr, add a line-buffered forwarder with redaction, 64 KiB buffer cap, and try-catch to prevent daemon crashes. Wire `onDiagnosticLine` so worker stderr also reaches the daemon log file (previously it only went to the daemon's terminal). Issue: #5976 (V1.5 follow-up) * fix(cli): align redaction marker and bound URL scheme length - Change redaction marker from ***REDACTED*** to <redacted> to match the supervisor's existing convention and avoid double-redaction output mismatches in tests. - Bound URL credential regex scheme to {0,31} chars (matching the supervisor's pattern) to prevent O(n²) backtracking on long strings of scheme-like characters. * fix(cli): use daemon-local heartbeat timestamp and split multiline env secrets Two fixes for unresolved review threads from #6098: - Heartbeat: use daemon's own `new Date().toISOString()` instead of reflecting the worker-supplied `message.at` value. Prevents a compromised adapter from injecting arbitrary data into `/daemon/status`. - PEM multiline: split multi-line sensitive env values (e.g. PEM keys) into per-line redaction patterns so individual logged lines match. The full value is kept as a pattern too for single-line matches. * fix(cli): address review feedback on credential redaction - Fix sensitiveEnvValues: change `lines.length > 1` to `> 0` so single matching lines from multi-line env values are also added as patterns. - Add hyphens to sk- charset for compound prefixes (sk-proj-, sk-ant-). - Add github_pat_ (fine-grained PATs) and ghu_ (app user tokens). - Add ASIA prefix for AWS STS temporary credentials alongside AKIA. - Update Authorization catch-all comment to accurately describe the 2-token limitation. * fix(cli): add heartbeat rationale comment Add comment explaining why heartbeat uses daemon clock instead of worker-supplied message.at (security: compromised adapter injection). * test(acp-bridge): add onEnd redaction test and fix lint Add test verifying credential redaction on partial lines flushed via forwarder.onEnd(). Suppress pre-existing vitest/no-conditional-expect lint errors in getAcpMemoryArgs tests (system-dependent heapArg). * test(acp-bridge): add onEnd redaction test and revert spawnChannel lint Move the onEnd credential redaction test to logRedaction.test.ts to avoid triggering pre-existing vitest/no-conditional-expect lint errors in spawnChannel.test.ts. Revert the spawnChannel test file to its upstream state. * fix(test): remove unused eslint-disable directives vitest/no-conditional-expect is not enabled in this repo's eslint config. The directives cause CI failure via reportUnusedDisableDirectives warn + --max-warnings 0. --------- Co-authored-by: Shaojin Wen <shaojin.wensj@alibaba-inc.com> Co-authored-by: 易良 <1204183885@qq.com>
Collaborator
Author
Post-merge comprehensive reviewReviewed the merged state at The earlier Critical findings were all genuinely fixed before merge: bounded URL scheme regex against ReDoS, greedy userinfo Findings present at merge — since resolved by #6146Recording these for traceability; all three were unresolved redaction/trust threads from this PR that #6146 closed:
Minor items still present on
|
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 hardens daemon-managed channel workers for
qwen serve --channelby adding bounded post-ready restart supervision, IPC heartbeat monitoring, worker stdout/stderr forwarding into the daemon log with redaction, and richer channel worker status fields. Startup remains fail-fast before the worker reaches ready, and the public command surface remains unchanged.It also keeps serve-owned channel pidfile metadata reserved during worker failures while clearing stale worker PIDs, so
qwen channel statusonly prints a Worker PID when the current worker is known to be live.Why it's needed
The previous daemon-managed worker path made
serve --channelusable, but post-ready worker crashes, stale worker hangs, and worker logs were not operationally visible enough for a long-running daemon. This change makes the worker lifecycle observable and recoverable without moving channel adapters into the daemon process or expanding the public channel CLI contract.Reviewer Test Plan
How to verify
Confirm that a worker that exits after ready is restarted within the bounded restart policy, while a worker that fails before ready still fails serve startup. Confirm that missing heartbeats mark
staleHeartbeatAt, kill the worker, and use the same restart path. Confirm worker output reaches the daemon log with bearer tokens, sensitive environment values, and proxy credentials redacted. Confirm/daemon/statusexposes the additive restart and heartbeat fields, does not warn for a successfully restarted running worker, and still warns for partial channel connection. Confirm serve-owned pidfile metadata keeps serve ownership but clearsworkerPidafter unexpected worker exit until a restarted worker reports ready.Commands used locally:
(cd packages/cli && npx vitest run src/serve/channel-worker-supervisor.test.ts src/commands/channel/daemon-worker.test.ts src/serve/run-qwen-serve.test.ts src/serve/daemon-status.test.ts src/commands/channel/status.test.ts) npm run build -- --cli-only npm run check:serve-fast-path-bundle npm run typecheck git diff --checkEvidence (Before & After)
Before: a serve-managed channel worker that crashed or became stale after ready was not automatically recovered with observable restart and heartbeat metadata, and pidfile metadata could leave clients with stale worker PID information.
After: post-ready unexpected exits and stale heartbeat timeouts are restarted within policy, worker logs are redacted and forwarded to the daemon log,
/daemon/statusreports restart and heartbeat fields, andqwen channel statusomits Worker PID whenever serve owns the channel service but no live worker PID is known.Tested on
Environment (optional)
macOS 26.4.1, Node.js v22.22.3, npm 10.9.8.
Risk & Scope
qwen channel start --daemon-url, dynamic channel hot reload, in-process adapters, multi-workspace daemon coordination, or independent worker log files. Windows and Linux were not locally tested./daemon/statusfields are additive.Linked Issues
Refs #5976
中文说明
What this PR does
本 PR 对
qwen serve --channel的 daemon-managed channel worker 做运维硬化：增加有界的 post-ready 重启监督、IPC 心跳监测、带脱敏的 worker stdout/stderr 转发到 daemon log，以及更完整的 channel worker 状态字段。worker ready 前启动失败仍保持 fail-fast，公开命令面不变。它还会在 worker 失败期间保留 serve-owned channel pidfile 元数据，但清除过期的 worker PID，因此
qwen channel status只有在当前 worker 确认为 live 时才打印 Worker PID。Why it's needed
之前的 daemon-managed worker 路径已经让
serve --channel可用，但 post-ready worker 崩溃、worker stale hang 和 worker 日志在长运行 daemon 场景下还不够可恢复、可观测。本改动让 worker 生命周期可观测且可恢复，同时不把 channel adapter 放进 daemon 进程，也不扩展公开 channel CLI 合约。Reviewer Test Plan
How to verify
确认 ready 后退出的 worker 会按有界 restart policy 重启，而 ready 前失败仍会让 serve startup 失败。确认 heartbeat 缺失会记录
staleHeartbeatAt、kill worker，并走同一 restart 路径。确认 worker 输出会进入 daemon log，且 bearer token、敏感环境变量值和 proxy 凭据会被脱敏。确认/daemon/status暴露 additive restart/heartbeat 字段，对已成功重启且 running 的 worker 不产生 warning，并且 partial channel connection 仍然 warning。确认 unexpected worker exit 后 serve-owned pidfile 仍保留 serve ownership，但清除workerPid，直到重启后的 worker 再次 ready 并报告新 PID。本地使用的命令：
(cd packages/cli && npx vitest run src/serve/channel-worker-supervisor.test.ts src/commands/channel/daemon-worker.test.ts src/serve/run-qwen-serve.test.ts src/serve/daemon-status.test.ts src/commands/channel/status.test.ts) npm run build -- --cli-only npm run check:serve-fast-path-bundle npm run typecheck git diff --checkEvidence (Before & After)
Before：serve-managed channel worker 在 ready 后崩溃或 heartbeat stale 时，没有带可观测 restart/heartbeat 元数据的自动恢复路径，pidfile 元数据也可能让客户端看到过期 worker PID。
After：post-ready 非预期退出和 stale heartbeat timeout 会按策略重启，worker 日志会脱敏并转发到 daemon log，
/daemon/status报告 restart/heartbeat 字段，且当 serve 拥有 channel service 但未知 live worker PID 时，qwen channel status不显示 Worker PID。Tested on
Environment (optional)
macOS 26.4.1，Node.js v22.22.3，npm 10.9.8。
Risk & Scope
qwen channel start --daemon-url、动态 channel hot reload、in-process adapters、多 workspace daemon 协调或独立 worker log 文件。本地未测试 Windows 和 Linux。/daemon/status字段均为 additive。Linked Issues
Refs #5976