Skip to content

fix(daemon): distinguish draining channel worker diagnostics - #7932

Merged
wenshao merged 1 commit into
QwenLM:mainfrom
destire-mio:codex/fix-channel-draining-diagnostic
Jul 29, 2026
Merged

fix(daemon): distinguish draining channel worker diagnostics#7932
wenshao merged 1 commit into
QwenLM:mainfrom
destire-mio:codex/fix-channel-draining-diagnostic

Conversation

@destire-mio

Copy link
Copy Markdown
Contributor

What this PR does

This change gives channel delivery and webhook enqueue a distinct diagnostic when their owning workspace is draining. A genuinely missing worker keeps the existing No channel worker... diagnostic, while both conditions continue to use the stable channel_worker_unavailable error code.

Why it's needed

The routing layer previously collapsed a draining workspace and a missing channel worker into the same message. During workspace reload or removal, operators were told that no worker owned the channel even though the worker existed and was intentionally rejecting new work while draining. Distinguishing the messages makes the failure actionable without changing the delivery protocol.

Reviewer Test Plan

How to verify

  • Route delivery to a worker owned by a selected workspace, begin draining that workspace, and confirm delivery is rejected with a workspace-draining diagnostic.
  • Enqueue a webhook task while its owning workspace is draining and confirm it receives the same draining-specific diagnostic.
  • Remove the owning worker completely and confirm both missing-worker paths keep their existing diagnostics.
  • Confirm all failure paths retain channel_worker_unavailable, do not invoke the supervisor, and do not expose the workspace path.

Evidence (Before & After)

Before: missing workers and draining workspaces both reported No channel worker....

After: missing workers keep No channel worker..., while draining workspaces report Channel worker for channel "..." is unavailable while its workspace is draining.

Tested on

OS Status
🍏 macOS ✅ tested
🪟 Windows ⚠️ not tested
🐧 Linux ⚠️ not tested

Environment (optional)

Node.js v26.5.0. The focused channel worker group suite passed 39/39 tests. The full workspace build and typecheck passed, along with focused Prettier and ESLint checks.

Risk & Scope

  • Main risk or tradeoff: Consumers that match the exact draining error text will see the new, more accurate message; the stable error code and HTTP status are unchanged.
  • Not validated / out of scope: Platform-specific integration tests, delivery persistence, retries, replay, and the other follow-ups tracked by Harden channel delivery follow-ups after #7388 #7504.
  • Breaking changes / migration notes: None.

Linked Issues

Refs #7504

中文说明

本 PR 做了什么

当 Channel 投递或 webhook 入队对应的 workspace 正在 draining 时,本修改会返回独立、准确的诊断信息。真正缺少 worker 的情况继续使用原有的 No channel worker... 提示,两种情况仍统一保留稳定的 channel_worker_unavailable 错误码。

为什么需要

此前路由层把 workspace 正在 draining 和 Channel worker 不存在合并成同一条错误信息。在 workspace 重载或移除期间,worker 明明存在并且只是按设计拒绝新任务,运维人员却会看到“没有 worker 管理该 Channel”的错误提示。拆分两种提示后,错误更便于排查,同时不会改变投递协议。

Reviewer 测试计划

如何验证

  • 将投递路由到选定 workspace 所属的 worker,开始 draining 该 workspace,确认投递被拒绝并返回 workspace-draining 专属诊断。
  • 在 Channel 所属 workspace 正在 draining 时入队 webhook 任务,确认返回相同的 draining 专属诊断。
  • 完全移除所属 worker,确认缺失 worker 的两条路径仍保留原有诊断。
  • 确认所有失败路径继续返回 channel_worker_unavailable,不会调用 supervisor,也不会暴露 workspace 路径。

修改前后证据

修改前:worker 缺失和 workspace draining 都返回 No channel worker...

修改后:worker 缺失继续返回 No channel worker...,workspace draining 则返回 Channel worker for channel "..." is unavailable while its workspace is draining.

测试平台

操作系统 状态
🍏 macOS ✅ 已测试
🪟 Windows ⚠️ 未测试
🐧 Linux ⚠️ 未测试

环境(可选)

Node.js v26.5.0。Channel worker group 聚焦测试 39/39 通过;完整 workspace build 和 typecheck 通过,相关文件的 Prettier 与 ESLint 检查也通过。

风险与范围

  • 主要风险或取舍:依赖 draining 错误文本精确匹配的调用方会看到新的、更准确的信息;稳定错误码和 HTTP 状态不变。
  • 未验证 / 不在范围内:平台相关集成测试、投递持久化、重试、重放,以及 Harden channel delivery follow-ups after #7388 #7504 跟踪的其他后续项。
  • 破坏性变更 / 迁移说明:无。

关联 Issue

Refs #7504

@qwen-code-ci-bot

qwen-code-ci-bot commented Jul 28, 2026

Copy link
Copy Markdown
Collaborator

Qwen Triage finished — CI landed green on 975e50e and the deferred approval was posted. finalize run

Qwen Triage 已完成 —— 975e50e 的 CI 全绿,延迟审批已提交。查看 finalize 运行

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Thanks for the PR!

Template looks good ✓

Problem: observed operational issue, confirmed by the linked issue #7504 which explicitly lists "Distinguish the workspace-draining diagnostic from the missing-worker diagnostic" as a follow-up item from the #7388 review. During workspace drain, operators see a misleading "No channel worker…" message even though the worker exists and is intentionally rejecting new work. The before/after in the PR description is clear.

Direction: aligned — this is a small, well-scoped diagnostic improvement to the daemon's channel delivery path, directly tracked in #7504. CHANGELOG has no direct reference but the area is relevant (channel delivery hardening is active work).

Size: not applicable — files are in packages/cli/src/serve/, not core module paths. 16 production lines (+14 −2), 14 test lines (+12 −2).

Approach: the scope feels right. The change does exactly one thing — splits a combined condition into two so each failure path gets its own message — and nothing else. No unrelated edits, no drive-by refactors. The error code stays channel_worker_unavailable in all paths, so downstream consumers (which match on code, not message text) are unaffected.

Risk: no elevated risk signals — no high-risk paths matched.

Moving on to code review. 🔍

中文说明

感谢贡献!

模板完整 ✓

问题:已观测到的运维问题,关联 issue #7504 明确将"区分 workspace draining 诊断和 worker 缺失诊断"列为 #7388 review 的后续项。在 workspace drain 期间,运维人员会看到误导性的"No channel worker…"提示,尽管 worker 存在且只是按设计拒绝新任务。PR 描述中的 before/after 清晰明了。

方向:对齐——这是一个小而精确的 daemon channel 投递路径诊断改进,直接在 #7504 中跟踪。CHANGELOG 无直接引用,但该领域相关(channel delivery 加固是活跃工作)。

规模:不适用——文件位于 packages/cli/src/serve/,非核心模块路径。生产代码 16 行(+14 −2),测试代码 14 行(+12 −2)。

方案:范围合理。修改只做一件事——将合并的条件拆分为两个,使每条失败路径获得独立的消息——没有其他改动。无无关编辑,无顺手重构。所有路径的错误码保持 channel_worker_unavailable,下游消费者(按 code 匹配,非 message 文本)不受影响。

风险:无升级风险信号——未匹配高风险路径。

进入代码审查 🔍

Qwen Code · qwen3.8-max-preview

Reviewed at 975e50e1ee2205c31504f8546f1b6ba0052948fc · re-run with @qwen-code /triage

@qwen-code-ci-bot

qwen-code-ci-bot commented Jul 28, 2026

Copy link
Copy Markdown
Collaborator

Code review

Independent proposal: given "distinguish draining workspace from missing worker", I would split the combined if (!entry || drainingWorkspaces.has(…) || !deliver) guard into two sequential checks — draining first (requires entry to exist to read workspaceCwd), then missing-worker — each throwing with its own message but the same channel_worker_unavailable code. Same for enqueueWebhookTask. Update the existing tests to assert the specific messages.

The PR does exactly this. The implementation matches the independent proposal one-to-one:

  • deliverChannelMessage: draining check (entry && drainingWorkspaces.has(entry.workspaceCwd)) throws first with the new draining-specific message; the remaining !entry || !deliver path keeps the original missing-worker hint (workspace-scoped vs global). Correct — the entry && guard is necessary because entry.workspaceCwd would throw on a null entry.
  • enqueueWebhookTask: same pattern, draining check first, then !entry. The original code didn't check !deliver here (it calls entry.supervisor.enqueueWebhookTask directly), and the PR correctly preserves that.
  • Error code channel_worker_unavailable preserved in all four paths. HTTP status mapping (503) is driven by the code, not the message — verified that all downstream consumers (channel-webhooks.ts, channel-notify.ts, workspace-channel-management.ts, run-qwen-serve.ts, SDK DaemonClient) match on error.code, never on message text.
  • Tests updated to assert exact messages in all four scenarios (missing worker with/without workspace scope, draining delivery, draining webhook). The existing not.toContain(SECONDARY) assertion (no workspace path leakage) is preserved.

No correctness issues, no convention violations, no unnecessary code. Clean.

CI test evidence

CI is still running on the reviewed commit — the Ubuntu unit test suite is in progress. All completed checks passed. Quoting real check results:

Final CI results for 975e50e (auto-updated by the triage finalize job after CI completed):

Check Conclusion
Classify PR ✅ success
macos-latest / Java 21 ✅ success
Real daemon E2E / Java 11 ✅ success
Serve A/B (ubuntu-latest, Node 22.x) ✅ success
Test (ubuntu-latest, Node 22.x) ✅ success
ubuntu-latest / Java 11 ✅ success
ubuntu-latest / Java 17 ✅ success
ubuntu-latest / Java 21 ✅ success
web-shell E2E Smoke (ubuntu-latest, Node 22.x) ✅ success
windows-latest / Java 21 ✅ success

One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行(取最新一次运行),省略 skipped,失败项排在最前。

The macOS/Windows tests and integration tests are skipped (likely gated on the Ubuntu suite passing first). No failures so far. The finalize workflow will update this table when CI settles.

Not verified: real-scenario tmux testing — this is an unattended CI run, and the change is a daemon-internal diagnostic (API error message), not a TUI surface. A maintainer can trigger @qwen-code /tmux if desired, though the change is unlikely to warrant it.

中文说明

代码审查

独立方案:给定"区分 draining workspace 和 worker 缺失"的目标,我会将合并的 if (!entry || drainingWorkspaces.has(…) || !deliver) 守卫拆分为两个顺序检查——先检查 draining(需要 entry 存在以读取 workspaceCwd),再检查 worker 缺失——各自抛出独立消息但保持相同的 channel_worker_unavailable 错误码。enqueueWebhookTask 同理。更新现有测试以断言具体消息。

PR 的实现与独立方案完全一致:

  • deliverChannelMessage:draining 检查(entry && drainingWorkspaces.has(entry.workspaceCwd))优先抛出新的 draining 专属消息;剩余的 !entry || !deliver 路径保留原有的 worker 缺失提示(区分 workspace 作用域和全局)。正确——entry && 守卫是必要的,因为 entry.workspaceCwd 在 null entry 上会抛异常。
  • enqueueWebhookTask:相同模式。原代码此处不检查 !deliver(直接调用 entry.supervisor.enqueueWebhookTask),PR 正确保留了这一点。
  • 所有四条路径的错误码 channel_worker_unavailable 均保留。HTTP 状态映射(503)由 code 驱动,非 message——已验证所有下游消费者(channel-webhooks.tschannel-notify.tsworkspace-channel-management.tsrun-qwen-serve.ts、SDK DaemonClient)均按 error.code 匹配,从不匹配 message 文本。
  • 测试已更新,在所有四个场景中断言精确消息。现有的 not.toContain(SECONDARY) 断言(不泄露 workspace 路径)已保留。

无正确性问题,无规范违反,无多余代码。干净。

CI 测试证据

CI 仍在运行——Ubuntu 单元测试套件进行中。所有已完成的检查均通过。引用真实检查结果(见上表)。macOS/Windows 测试和集成测试已跳过(可能依赖于 Ubuntu 套件先通过)。目前无失败。finalize workflow 将在 CI 结束后更新表格。

未验证:真实场景 tmux 测试——这是无人值守的 CI 运行,且修改是 daemon 内部诊断(API 错误消息),非 TUI 界面。维护者可按需触发 @qwen-code /tmux

Qwen Code · qwen3.8-max-preview

Reviewed at 975e50e1ee2205c31504f8546f1b6ba0052948fc · re-run with @qwen-code /triage

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Confidence: 5/5 — clean across every stage; would merge without hesitation.

This is exactly the kind of PR the gate should wave through quickly. The problem is real and tracked (#7504), the fix is the minimal possible change (split one condition into two, give each its own message), the error code contract is preserved, and every downstream consumer matches on code — not message text — so nothing breaks. The tests assert the new messages in all four scenarios. No unrelated edits, no over-engineering, nothing to maintain that isn't immediately obvious.

My independent proposal and the PR's implementation are identical. I have no reservations.

Approval deferred until CI lands green on 975e50e1ee2205c31504f8546f1b6ba0052948fc — the Ubuntu unit test suite is still running. The finalize workflow will post the commit-pinned approval once all checks complete.

中文说明

置信度:5/5 — 每个阶段都干净;毫不犹豫地合并。

这正是 gate 应该快速放行的 PR。问题真实且已跟踪(#7504),修复是最小可能的改动(将一个条件拆为两个,各自给独立消息),错误码契约保留,所有下游消费者按 code 匹配而非 message 文本——不会有任何东西坏掉。测试在所有四个场景中断言了新消息。无无关编辑,无过度工程,没有不直观的需要维护的东西。

我的独立方案和 PR 实现完全一致。没有任何保留意见。

审批延迟至 CI 在 975e50e1ee2205c31504f8546f1b6ba0052948fc 上全部通过后执行——Ubuntu 单元测试套件仍在运行。finalize workflow 将在所有检查完成后发布绑定到该 commit 的审批。

Qwen Code · qwen3.8-max-preview

Reviewed at 975e50e1ee2205c31504f8546f1b6ba0052948fc · re-run with @qwen-code /triage

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

🩺 serve daemon A/B

Built the PR base vs this PR head 975e50e, drove a fixed endpoint set against each, and diffed the JSON responses. Only fields that changed are shown.

No response changes against the PR base across 4 scenario(s).

Qwen Code · serve A/B

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found. LGTM! ✅

中文说明

未发现问题。LGTM!✅

— qwen3.7-max via Qwen Code /review

@wenshao wenshao left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found. LGTM! ✅

中文说明

未发现问题。LGTM!✅

— qwen3.8-max-preview via Qwen Code /review

@wenshao
wenshao added this pull request to the merge queue Jul 29, 2026
Merged via the queue into QwenLM:main with commit cc617e6 Jul 29, 2026
59 checks passed
@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Released in v0.21.2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants