chore(serve): Log session continuation admissions - #8932
Conversation
Refs QwenLM#8923 Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Refs QwenLM#8923 Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
Thanks for the PR!
Moving on to code review. 🔍 中文说明感谢贡献！
进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
🩺 serve daemon A/BBuilt the PR base vs this PR head ✅ No response changes against the PR base across 4 scenario(s). — Qwen Code · serve A/B |
Code reviewThe implementation is exactly the minimal change this needs: capture the bridge result, and when Things I checked specifically:
Reuse is good: existing Test evidence (PR's own CI, read via API — no PR code executed)
The one red check is not caused by this PR — it's a heap OOM in an unrelated suite:
Not verified: live daemon output against a real bridge (unattended run — PR code is never executed here). The tests cover the real route handler with only the bridge faked, which is the layer this PR does not change, so the CI signal above substantiates the claim. The finalize pass will update the table once CI settles on this commit. 中文说明代码审查实现恰好是所需的最小改动：接住 bridge 返回值，当 具体核对过：
复用良好：沿用既有 测试证据（来自 PR 自身 CI，经 API 读取——未执行任何 PR 代码）CI 结果见上表。唯一的红色检查并非本 PR 导致——是一个无关测试套件的堆内存 OOM：
未验证：真实 bridge 下的 daemon 实际输出（无人值守运行——此处从不执行 PR 代码）。测试覆盖了真实路由处理器、仅 bridge 为假，而 bridge 正是本 PR 未改动的层，因此上述 CI 信号即为该结论的证据。finalize 流程会在该提交的 CI 落定后更新表格。 — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 4/5 — clean, minimal, well-tested observability change; the only reservations are flaky-CI aftermath and one open design question, neither blocking. Thinking it through before deciding:
Non-blocking notes for the maintainer:
LGTM, approving pinned to the reviewed commit. ✅ 中文说明置信度：4/5 —— 干净、最小化、测试充分的可观测性改动；仅有的保留意见来自 flaky CI 的后续影响和一个未决设计问题，均不阻塞。 决定前的思考：
给维护者的非阻塞建议：
LGTM，批准已固定在所审查的提交上。✅ — Qwen Code · qwen3.8-max Reviewed at |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not explored to full depth (tool budget reached): Change summary: This PR adds a structured continuation e...: none — I completed every check I wanted. Tool calls ~12.; Change summary: This PR adds a structured continuation e...: none — everything I planned to check, I checked. What I did not do: run the test suite or build (not required by my methodology; static verification only). This….
中文说明
已审查。 建议见行内评论。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未探索到全部深度（达到工具调用预算）：Change summary: This PR adds a structured continuation e...：none — I completed every check I wanted. Tool calls ~12.；Change summary: This PR adds a structured continuation e...：none — everything I planned to check, I checked. What I did not do: run the test suite or build (not required by my methodology; static verification only). This…。
— qwen3.8-max via Qwen Code /review (v0.21.9)
| if (daemonLog && result.accepted) { | ||
| daemonLog.info('continuation enqueued', { |
There was a problem hiding this comment.
[Suggestion] Rejected continuations are silently unlogged. When the bridge returns accepted: false, this route responds 200 and emits no lifecycle record — the log is gated on result.accepted and the bridge returns the rejection without logging — so the interruption reason exists only in the HTTP response body. The sibling prompt route already logs admission rejections as WARN records (prompt admission rejected: queue full / invalid client id), while the continuation route leaves rejections invisible in the lifecycle family this PR documents (the per-request access log records the 200, but carries no reason and is subject to burst suppression). — Failure scenario: a controller keeps POSTing /session/:id/continue while a turn is in flight or after a turn ended cleanly; every attempt returns 200 with the reason only in the body, and an oncall investigating "continuations never start" finds an empty lifecycle log and no server-side trace of the rejection reason.
Suggested fix — emit a record on the not-accepted branch, mirroring the prompt route's admission-rejection records:
if (daemonLog && result.accepted) {
daemonLog.info('continuation enqueued', { sessionId, promptId, clientId });
} else if (daemonLog) {
daemonLog.info('continuation rejected', {
sessionId,
promptId,
clientId,
interruption: result.interruption,
});
}中文说明
建议：被拒绝的 continuation 不会被记录。当 bridge 返回 accepted: false 时，该路由返回 200 但不产生任何生命周期记录——日志以 result.accepted 为条件，而 bridge 返回拒绝时也不会记录——因此 interruption 原因只存在于 HTTP 响应体中。同文件中的 prompt 路由已经将准入拒绝记录为 WARN（prompt admission rejected: queue full / invalid client id），而 continuation 路由让拒绝在本 PR 所文档化的生命周期日志族中完全不可见（按请求的 access log 会记录该 200，但不包含原因，且受突发抑制）。— 失败场景：控制器在 turn 进行中或 turn 正常结束后反复 POST /session/:id/continue；每次尝试都返回 200，原因仅在响应体中。排查"continuation 一直不启动"的值班人员只会看到空的生命周期日志，服务端没有拒绝原因的任何痕迹。
建议修复——在未接受分支也输出一条记录，与 prompt 路由的准入拒绝记录保持一致（见上方代码示例）。
— qwen3.8-max via Qwen Code /review (v0.21.9)
| promptId, | ||
| }); | ||
| if (daemonLog && result.accepted) { | ||
| daemonLog.info('continuation enqueued', { |
There was a problem hiding this comment.
[Suggestion] continuation enqueued has no structured terminal counterpart. An accepted continuation that fails mid-turn leaves this record permanently unterminated in the structured log family: the async turn's only failure path is .catch → teeServeDebugLine — unstructured, gated on QWEN_SERVE_DEBUG, and even with the flag on it reaches the daemon log only as a raw line, never a structured record. Prompt turns, by contrast, get structured prompt turn completed / prompt turn failed records. This diff creates the first half of that unterminated pair, and the new observability bullet enrolls it in the lifecycle-log family beside prompts. — Failure scenario: an accepted continuation fails after admission (model/tool error, ACP child dies); with QWEN_SERVE_DEBUG off (typical production) the daemon file logs show continuation enqueued and nothing after it, so the oncall cannot tell from the structured family whether the turn completed or died — clients are covered by SSE terminal events, but the file-based audit trail is not.
Suggested fix — add a structured terminal record for continuation turns (mirroring prompt turn completed / prompt turn failed, keyed by sessionId + promptId). If that is out of scope here, extend the new 19-observability.md bullet to state that continuation-turn outcomes are only visible via SSE terminal events and QWEN_SERVE_DEBUG output, not the structured file logs.
中文说明
建议：continuation enqueued 没有对应的结构化终态记录。被接受后在 turn 中途失败的 continuation 会让这条记录在结构化日志族中永远没有终态：异步 turn 唯一的失败路径是 .catch → teeServeDebugLine——非结构化、受 QWEN_SERVE_DEBUG 开关控制，即使开关打开也只会以原始文本行进入 daemon 日志，永远不会成为结构化记录。相比之下，prompt turn 有结构化的 prompt turn completed / prompt turn failed 记录。本 diff 创建了这对未闭合记录的前一半，且新增的可观测性条目又把它与 prompt 一起归入生命周期日志族。— 失败场景：被接受的 continuation 在准入后失败（模型/工具错误、ACP 子进程退出）；在 QWEN_SERVE_DEBUG 关闭（典型生产环境）时，daemon 文件日志只显示 continuation enqueued，其后什么都没有，值班人员无法从结构化日志族判断 turn 是完成了还是失败了——客户端有 SSE 终态事件覆盖，但基于文件的审计轨迹没有。
建议修复——为 continuation turn 增加结构化终态记录（与 prompt turn completed / prompt turn failed 对应，以 sessionId + promptId 为键）。若超出本 PR 范围，可在新增的 19-observability.md 条目中说明：continuation turn 的结果仅可通过 SSE 终态事件和 QWEN_SERVE_DEBUG 输出可见，结构化文件日志中不可见。
— qwen3.8-max via Qwen Code /review (v0.21.9)
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
LGTM. Verified at head: the continuation enqueued record is emitted exactly once per accepted continuation (the bridge calls sendPrompt synchronously before returning accepted:true, so truthy accepted genuinely means enqueued; queue-full/pre-abort throw synchronously), never on reject/fail, carries only sessionId/promptId/clientId (the route never reads the body; a test asserts the body string never reaches any log call), clientId absent renders as dropped per the logger's undefined-drop, and the charset+128-cap bound cardinality and log injection. The client-identity doc matches code (webui_ prefix verified in clientLifecycle.ts). Tests cover accept/reject/error branches plus the leak guard and passed in CI (cli 798/798 files).
Non-blocking: the two outstanding bot suggestions (a rejection record mirroring the prompt route's WARN, and a structured terminal counterpart for accepted continuations) are legitimate observability follow-ups the PR explicitly scopes out. CI on head is red only on the pre-existing web-shell App.test.tsx worker heap OOM — this PR touches zero web-shell files and everything relevant passed; a re-run should clear it. Ship it.
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
No blockers. The change is minimal and well-tested: the continuation enqueued record mirrors the existing prompt enqueued pattern (gated on result.accepted, identifiers-only, no content leakage). Tests cover accept/reject/error branches plus a leak guard. Documentation is accurate. LGTM.
|
Released in v0.21.11. |
3 participants
What this PR does
This PR adds a structured
continuation enqueuedlifecycle record after the daemon has accepted a session continuation. The record contains onlysessionId, the generatedpromptId, andclientIdwhen the request supplies one; rejected and failed continuation attempts are not reported as enqueued, and prompt content is never included.It also documents the client identity contract for multi-controller integrations: each independent controller should use a distinct stable ID, WebUI-generated IDs use the
webui_prefix, and a host should share an ID with an embedded WebShell only when both intentionally act as one logical controller.Why it's needed
Prompt submission and cancellation already have admission lifecycle records, but continuation did not. That gap made it difficult to determine whether a continuation was actually admitted and which controller initiated it while investigating session-navigation behavior under #8923. The new record closes that observability gap without changing the REST, ACP, SDK, cancellation, prompt queue, continuation, or detach contracts.
Reviewer Test Plan
How to verify
Send a continuation that the bridge accepts and confirm the daemon emits exactly one
continuation enqueuedrecord containing the session ID, generated prompt ID, and client ID, with no request content. Then returnaccepted: falseand throw a mapped bridge error; neither path should emit the admission record. Run the focused serve route tests and confirm all four continuation cases pass, then run the repository build, typecheck, lint, and bundle checks.Evidence (Before & After)
N/A — this is a daemon observability and documentation change with no user-interface output.
Tested on
Environment (optional)
macOS 26.4.1, Node.js 22.22.3, npm 10.9.8, sandbox disabled for local repository checks.
Risk & Scope
Linked Issues
Refs #8923
中文说明
本 PR 做了什么
本 PR 在 daemon 接受会话 continuation 后新增结构化的
continuation enqueued生命周期记录。记录只包含sessionId、生成的promptId，以及请求提供时的clientId；被拒绝或失败的 continuation 不会被记录为 enqueued，也绝不会记录 prompt 内容。同时补充多控制器集成的客户端身份约定：每个独立控制器应使用不同且稳定的 ID，WebUI 生成的 ID 使用
webui_前缀；只有宿主与内嵌 WebShell 有意作为同一个逻辑控制器时，二者才应共享 ID。为什么需要
Prompt 提交和取消已经有准入生命周期记录，但 continuation 没有。排查 #8923 下的会话导航行为时，这一缺口导致难以判断 continuation 是否真正被准入，以及由哪个控制器发起。新增记录在不改变 REST、ACP、SDK、取消、prompt 队列、continuation 或 detach 契约的前提下补齐了可观测性。
Reviewer 测试计划
如何验证
发送一个被 bridge 接受的 continuation，确认 daemon 只产生一条
continuation enqueued记录，包含会话 ID、生成的 prompt ID 和客户端 ID，且不包含请求内容。随后让 bridge 返回accepted: false，再抛出一个会被映射的错误；这两条路径都不应产生准入记录。运行定向 serve 路由测试并确认四个 continuation 场景全部通过，再运行仓库 build、typecheck、lint 和 bundle 检查。证据（修复前后）
N/A——这是 daemon 可观测性和文档变更，没有用户界面输出。
测试平台
环境（可选）
macOS 26.4.1、Node.js 22.22.3、npm 10.9.8；本地仓库检查未启用 sandbox。
风险与范围
关联 Issue
Refs #8923