feat(daemon): Add SSE stream and client observability - #8572
Conversation
🖼️ web-shell visual previewRendered against a mock daemon (no real backend): the PR base vs this PR head Screenshots · before / afterℹ️ No screenshot changed against the PR base — but this PR edits 1 render-shaping file:
Either the change has no visual effect (logic, plumbing, a state the scenarios never reach), or no scenario renders this UI — in which case the preview cannot see it, and an empty result is a coverage gap rather than a clean bill of health. To make it visible, add a scenario to Full-resolution recordings (.webm) are attached to the workflow run. — Qwen Code · web-shell visuals |
🩺 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 |
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Review summary
Read every changed file at 9dc33831d and ran the affected suites locally. This is careful, well-scoped work — a design doc, a protocol-doc update, a clean onSubscriberDiagnostic seam that keeps EventBus free of session/client/stream identity, and 16 new focused tests. No correctness defect found in the SSE hot path. Findings below are one dead field, one attribute-naming deviation, one design gap, and nits.
Local verification
| Suite | Result |
|---|---|
acp-bridge/src/eventBus.test.ts |
58/58 pass |
cli/src/serve/auth.test.ts |
29/29 pass |
cli/src/serve/server.test.ts |
859/861 pass — the 2 failures are a 50k-item /workspace/:id/sessions timeout and an ENOTEMPTY tmpdir race in POST /workspace/reload, both unrelated to SSE. All 10 new SSE tests pass. |
sdk-typescript (RestSseTransport + DaemonSessionClient + AcpHttpTransport) |
150/150 pass |
webui/DaemonSessionProvider.test.tsx |
174 pass / 20 fail — the identical 20 also fail at the base commit 67d128715 (transcript pagination / notices routing), so pre-existing environmental, not this PR. The new sseConnectReason assertions all pass. |
prettier --check on all changed source files |
clean |
Things done right (worth keeping)
res.prependOnceListener('finish'|'close', finalize)— the telemetry middleware registers its ownres.once('finish'|'close', finish)beforenext()(serve/server/telemetry.ts:776), so prepending is what guarantees the close attributes land before the request span ends. Non-obvious and correct.liveTimingEnabled = lastEventId === undefinedis genuinely right:EventBus.subscribegates the whole replay block onlastEventId !== undefined, and only that path force-pushesreplay_complete. A fresh stream cannot accidentally measure replayed frames as live lag.subscriberDiagnosticHandled's try/catch preservespublish()'s never-throws contract, and capturing the telemetry context at handler entry means a warning emitted from a publisher's stack still parents to the right long-lived span.Access-Control-Expose-Headersupdated for the new header — easy to forget, and the WebUI would silently getundefinedlineage without it.
Also checked
Rate limiting is unaffected by the newly-sent X-Qwen-Client-Id on the SSE subscription — GET /session/*/events is explicitly exempt in rate-limit.ts resolveTier, so the header does not newly re-bucket anything through createKeyExtractor.
Two things that belong in Risk & Scope
The body says stream control behavior is unchanged, but two changes go slightly beyond observability (both are improvements — they just deserve a line): the terminal stream_error frame is now skipped on a socket-error close, and the write loop now breaks when a write settles as closed. Inline comments on both.
Also worth one line: for REST SSE subscribers the EventBus stderr fallback is now suppressed, so qwen serve: EventBus subscriber evicted {...} becomes qwen serve: SSE client evicted {...} and EventBus slow_client_warning becomes SSE slow client warning. Strictly richer, but anything grepping daemon stderr for the old strings breaks.
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
9dc3383 to
a7dee45
Compare
|
Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. 中文请勿对活跃的 PR 执行 rebase 或 force-push，因为这会使已有的评审评论失效。另外，供日后参考：作为集成流程的一部分，机器人始终会自动将所有改动压缩（squash）为单个提交。 |
github-actions
Bot
added
the
review/self-reported
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Review feedback addressed in
|
| Feedback | Resolution |
|---|---|
Dead restartReason field |
Fixed: removed the field and assign prompt_restart directly on both restart paths. |
| Bare close OTel attributes | Fixed: all close event and request-span attributes now use the qwen-code.daemon.sse.* namespace, with regression coverage for the former bare keys. |
| State Resync lineage | Not taking: the approved design intentionally limits lineage to adjacent accepted REST streams on the same DaemonSessionClient and permits a State Resync client rebuild to break that best-effort chain. |
| Socket/write control-flow disclosure | Addressed: Risk & Scope now documents immediate exit after a closed write and skipping the doomed terminal stream_error write after socket_error. |
| Diagnostic sanitization and bounding | Fixed: reason and session identity use the same bounded single-line sanitizer, and code-point materialization is bounded before allocation. |
| UUID validator and malformed client ID rationale | Fixed: added reciprocal validator cross-references and documented why a malformed diagnostic client ID does not reject the SSE handshake. |
Lifecycle handled boundary |
No change: the telemetry helper is async and its rejection is already caught, so it cannot synchronously escape after the human-visible log is written. |
Validation completed:
- Focused CLI SSE route tests passed, including sanitization and namespaced close attributes.
- Full WebUI provider tests passed (194 tests).
- SDK
RestSseTransporttests passed (47 tests). - WebUI and SDK typechecks passed; formatting, ESLint, and diff checks passed.
- Two consecutive open-ended diff audits and an independent read-only verification found no additional actionable issues.
- The repository build/typecheck gate remains blocked by pre-existing CLI Ink selection typing errors outside this PR's diff; the build reached the CLI package before failing on those known files.
OverviewThis adds per-connection identity and lifecycle observability to the REST SSE surface, without touching stream behavior:
What's done well
I also verified the two places this could have silently no-op'd: Suggestions1. 2. 3. 4. 5. Truncation is invisible. 6. Risks / behavior changesThe PR's Risk section is accurate. For reviewers, the non-observability changes are:
One inherent caveat worth knowing: Performance: the per-frame hot path adds one boolean read, one Security: no payloads, tokens, or auth data reach diagnostics; every client-supplied field is allowlisted or regex-validated before it reaches a log line or span attribute; diagnostics never feed auth, replay, eviction, dedup, or supersession. The newly exposed CORS header is a server-generated UUID. Overall this is well-scoped and carefully built — the suggestions above are all minor and none of them block. 中文小结结论：整体质量很高，建议均为小改动，不阻塞合入。 做得好的地方：EventBus 只拿回调、不感知身份，分层干净； 主要建议：
另外提醒 reviewer：本 PR 除可观测性外还有几处行为变更（写入 |
|
@qwen-code /triage |
|
Sandboxed verification: ✅ passed — merge-ready (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 61 passed · 0 failed · 61 total 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：61 通过 · 0 失败 · 61 总计 Verification reportPR 8572 Deep Verification — feat(daemon): Add SSE stream and client observabilityVerdict: 中文摘要结论：
Central claim and A/BCentral claim: each accepted REST SSE stream gets a stable UUID ( Secondary claims: (1) the SDK carries client identity, connect reason, and predecessor lineage while tolerating older daemons and stripped/mangled headers without inventing lineage; (2) reconnect/replay/queue/backpressure/eviction behavior is unchanged. The A/B drove the real route module from each tree's
32/32 assertions pass ( The first eviction attempt returned Back-compat and lineage (SDK, head build)Real
Diagnostic-context sanitizer boundary ( Reviewer Test Plan, walked step by step
No step was unreachable. Mutation matrix (vacuity check on the central new tests)Filtered to the four central tests in
Every mutation failed the intended behavioral assertion (expected-vs-actual, not import/compile breakage); no survivors in the central set, and the green control is the positive control for the harness. Worktree restored ( Targeted gates
Gate liveness proven before citation: a planted Corrections
FindingsNo blocking findings.
Not covered
MethodologyRan in the CI verify container ( Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention). — Qwen Code · sandboxed verification |
|
Thanks for the PR — the design doc and the staged rollout plan make this a straightforward gate to run. Template looks good ✓ Problem: real and well-stated. The linked issue (#8571, self-reported by the author) describes an attribution hole in diagnostics this repo already emits: a session ID identifies the logical session but not the physical SSE connection behind it, so slow-client warnings, evictions, replay gaps, and backpressure episodes can't be joined into one timeline. That's an observed operational gap, not a theoretical concern — no before/after reproduction is expected for an observability feature, and none is claimed. Direction: aligned. Daemon/SSE reliability is an actively worked surface in recent releases (no direct CHANGELOG mention of stream observability, but adjacent daemon/Web Shell work lands steadily — #7896, #8335 and friends). One note for the maintainer: the PR consumes the telemetry surface and extends the SDK↔daemon wire contract (a new response header, two new query params, new exported SDK types). Everything is optional and diagnostic-only by design, but flagging it per the sensitive-area rule. Size: cross-package change (acp-bridge, cli, sdk-typescript, webui; core itself is test-only). 859 production-logic lines (605 of them in the SSE route), 1,116 test lines, 158 docs lines. It's a Approach: the scope feels right for a "first stage". The design doc lists explicit non-goals (metric labels, active-stream status, ACP streams, automatic supersession), all wire fields are optional, and EventBus stays identity-free via a callback seam — the right layering. No materially simpler path jumps out: cutting the close-record statistics would remove exactly the data needed to tell a reconnect storm from a slow read or a large frame, which is the point of the change. Heads-up for reviewers: the diff carries four disclosed defensive behavior changes in the write path (immediate break on a closed write, skipping the doomed Risk: no match against the revert-history high-risk path panel. Moving on to code review. 🔍 中文说明感谢贡献——设计文档和分阶段上线计划让这个门禁审查很顺畅。 模板完整 ✓ 问题：真实且描述清晰。关联 issue（#8571，作者自报）指出的是本仓库已有诊断的一个归因缺口：session ID 只能标识逻辑会话，无法标识其背后的物理 SSE 连接，因此慢客户端告警、淘汰、replay 缺口和背压事件无法被拼接成一条时间线。这是已观测到的运维缺口，不是理论性顾虑——可观测性类 feature 不要求也不声称提供 before/after 复现。 方向：对齐。Daemon/SSE 可靠性是近期持续活跃的方向（CHANGELOG 没有直接提及 stream 可观测性，但相邻的 daemon/Web Shell 工作持续落地——#7896、#8335 等）。提醒 maintainer：此 PR 使用了 telemetry 面并扩展了 SDK↔daemon 的线上契约（一个新的响应头、两个新的查询参数、新的 SDK 导出类型）。按设计全部为可选且仅用于诊断，但按敏感领域规则予以标记。 规模：跨包变更（acp-bridge、cli、sdk-typescript、webui；core 本身只有测试改动）。859 行生产逻辑（其中 605 行在 SSE 路由），1,116 行测试，158 行文档。属于 方案：作为"第一阶段"范围合适。设计文档列出了明确的非目标（指标标签、活跃流状态、ACP 流、自动取代），所有线上字段均为可选，EventBus 通过回调接缝保持身份无感知——分层正确。没有明显更简的路径：砍掉 close 记录的统计信息，恰好会丢掉区分重连风暴、慢读取与大帧所需的数据，而那正是本变更的目的。提醒 reviewer：diff 夹带了四处已披露的写路径防御性行为变更（写入已关闭时立即退出循环、socket error 后跳过注定失败的 风险：未命中 revert 历史高风险路径面板。 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewMy independent proposal for this problem — daemon-minted UUID per accepted stream exposed via a response header, optional format-validated predecessor/reason query params that never feed auth or replay, lifecycle records enriched in place with the telemetry context captured at route entry (EventBus warnings fire under the publisher's async context, so they'd otherwise parent to the wrong span), an identity-free EventBus diagnostic callback, and lineage owned by the SDK session client — is essentially what this PR does. The implementation goes further than the baseline in good ways: close-record statistics (settled frames, backpressure counts, drain waits, live publish-to-write lag), and a bounded sanitizer for every client-controlled string. I didn't find a simpler path it missed. What I verified reading the diff at
No critical blockers and no convention violations from my side. The six suggestions in @wenshao's review were posted after this commit, and his approval explicitly treats them as non-blocking. The author has since recorded a disposition for each one (no further scope expansion, per the repo's review-round churn guard): none are taken in this PR — the connect-reason absent/invalid conflation (1) and the writer-idle sequenceDiagram
participant P1 as WebUI provider
participant P2 as DaemonSessionClient
participant P3 as RestSseTransport
participant P4 as SSE route
participant P5 as EventBus
P1->>P2: events() with provable reason
P2->>P3: subscribe(reason, predecessor)
P3->>P4: GET events (connectReason, previousStreamId)
P4->>P4: validate inputs, mint stream UUID
P4-->>P3: 200 + X-Qwen-SSE-Stream-Id
P3->>P2: onSseStreamAccepted(streamId)
P5-->>P4: slow-client / eviction diagnostic callback
P4->>P4: opened, warning, closed records under captured context
Files changed (23 of 23 shown)
Testing — the PR's own CI (this is a CI run; no PR code was executed here)All PR-event CI runs are now green on the reviewed commit, including Serve A/B, which was still in flight at the previous pass. The macOS/Windows test jobs and the CLI integration job are skipped by workflow configuration (the normal PR-CI shape in this repo — not failures). The
Sandboxed verification of the one claim static review and unit tests can't fully settle — that the four disclosed write-path changes (immediate break on a closed write, skipping the doomed 中文说明代码审查：我对这个问题的独立方案（daemon 侧为每条接受的流生成 UUID 并通过响应头暴露、可选且格式校验的前驱/原因查询参数且不参与鉴权与 replay、在路由入口捕获遥测上下文后就地丰富生命周期记录、EventBus 通过身份无感知的诊断回调解耦、lineage 由 SDK 会话客户端持有）与本 PR 基本一致；实现还更进一步（close 统计、背压/排空/实时滞后测量、对所有客户端可控字符串的有界清洗）。没有发现被遗漏的更简路径。已核验：未新增 core 面（core 生产代码零改动）； 测试：本次为 CI 运行，未执行任何 PR 代码。被审 commit 上所有 PR-event CI 运行现已全绿，包括上一轮审查时仍在运行的 Serve A/B；macOS/Windows 测试与 CLI 集成任务按工作流配置跳过（本仓库 PR CI 的常规形态，非失败）。关于"四处已披露的写路径变更在真实 socket 下保持 reconnect/replay 语义、close 归因成立"这一静态审查与单测无法完全定论的声明：Serve A/B（base 与 head 对真实 daemon 的对比）已在本 commit 上通过；本线程触发的 /verify 沙箱运行仍在进行——报告落地后应以审视 fork CI 日志的同样怀疑态度阅读。本地测试仅 macOS（作者自述），此处未独立复跑任何结果。 — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 3/5 — clean review, but the score is capped by policy, not by doubt: a cross-package The commit is unchanged since the previous pass — this re-run re-reviewed
One honest correction to the previous pass: it said a maintainer word on the size escalation plus green sandbox lanes "would move this to a bot approval". That overstates what this gate can do — per the core-module rule, a 500+-line core-touching ⏸️ Deferring to @wenshao — no blockers found, so no request-changes either; the PR simply sits in the bucket this gate does not approve (core-module size escalation + telemetry/wire-contract surface). Main requires two approvals and one (yours) stands on this commit; the second is a maintainer's call. If you want the 中文说明置信度：3/5 —— 审查本身干净，但分数由政策封顶而非疑虑：跨包 commit 与上一轮相同——本次重跑复审了
对上一轮评论的一处诚实更正：上轮称"maintainer 确认规模升级、沙箱通道转绿后，本 PR 可转为机器人批准"——这高估了本门禁的权限：按核心模块规则，500+ 行触及核心的 ⏸️ 转交 @wenshao —— 未发现阻塞项，因此也不发起 request-changes；本 PR 只是落在本门禁不予批准的区间（核心模块规模升级 + telemetry/线上契约面）。main 需要两个 approve，目前一个（你的）在此 commit 上有效；第二个 approve 由 maintainer 决定。若希望拿到 — Qwen Code · qwen3.8-max Reviewed at |
|
Triage re-run completed without a new review.
The stage comments above were updated with the latest result. View workflow run. 上方各阶段评论已更新为最新结果。查看工作流运行。 |
|
Thanks @wenshao — I rechecked all six suggestions against
No code change or new commit was made. Suggestions 1 and 2 remain reasonable follow-up candidates if rollout-adoption or terminal-frame query requirements call for different semantics. |
|
@qwen-code /triage |
|
Triage re-run completed without a new review.
The stage comments above were updated with the latest result. View workflow run. 上方各阶段评论已更新为最新结果。查看工作流运行。 |
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
LGTM, no blockers. Disciplined observability: diagnostics carry only event type + serialized bytes (never payloads); client inputs strictly validated (connectReason enum, previousStreamId UUID regex, X-Qwen-Client-Id charset — no CRLF/log injection, invalid normalizes); no new metric maps (cardinality safe); connection accounting leak-free (increment after accepted subscription, idempotent finalize decrements once, 400/404/429 never touch counter); pure instrumentation (behavior deltas benign). P2 (one-line, take before merge): sse-events.ts:180 puts raw sessionId into telemetryBaseAttributes/span attrs, skipping boundedDiagnosticString — use the already-computed diagnosticSessionId. P3s: first-observer-wins close reason, cosmetic terminal_event_type mislabel on dead socket, stderr format change (note in changelog), surrogate-pair slice, minor test gaps.
|
Released in v0.21.7. |
4 participants
What this PR does
Adds first-stage observability for REST SSE connections. Each accepted stream receives a stable UUID and emits correlated lifecycle telemetry and daemon logs for opening, slow-client warnings, eviction, state resync, and closing. Close records include duration, settled frame count, the last written event ID, backpressure and live-lag statistics, terminal event attribution, and an explicit close reason.
The TypeScript SDK carries the existing client identity plus optional connection reason and adjacent accepted-stream lineage while remaining compatible with older daemons and response headers stripped by gateways. The WebUI supplies a reason only when it can distinguish prompt restart, normal stream end, transport error, or state resync. Subscriber diagnostics contain queue and trigger metadata but no event payload, and session identity remains owned by the SSE route.
Why it's needed
A session ID identifies a logical session but cannot distinguish the physical SSE connections created by reconnects. Existing slow-client warnings therefore cannot be reliably tied to a particular stream, predecessor, queue state, write-backpressure episode, replay gap, or close outcome. This change makes a session-level query sufficient to reconstruct the REST SSE timeline and distinguish client restart storms, slow network or proxy reads, large frames, many small queued frames, and replay resynchronization without changing reconnect, replay, queue, backpressure, or eviction behavior.
Reviewer Test Plan
How to verify
cd packages/acp-bridge && npx vitest run src/eventBus.test.ts,cd packages/core && npx vitest run src/telemetry/daemon-tracing.test.ts,cd packages/sdk-typescript && npx vitest run test/unit/AcpHttpTransport.test.ts test/unit/DaemonSessionClient.test.ts test/unit/RestSseTransport.test.ts,cd packages/webui && npx vitest run src/daemon/session/DaemonSessionProvider.test.tsx, andcd packages/cli && npx vitest run src/serve/auth.test.ts src/serve/server.test.ts. The diagnostics, telemetry context, REST handshake and lineage, WebUI reason transitions, and daemon lifecycle tests should pass.Targeted validation passed locally: 58 EventBus tests, 13 daemon telemetry tests, 169 SDK tests, 181 WebUI tests, and the 40 focused daemon SSE tests after rebasing onto the current main branch. Relevant package builds, typechecks, lint, formatting, and diff checks also passed. The repository-wide CLI build/typecheck remains blocked by pre-existing Ink selection typing errors unrelated to this change.
Evidence (Before & After)
N/A — protocol diagnostics and observability only; no TUI change.
Tested on
Environment (optional)
macOS, Node.js v22.22.3, npm 10.9.8, npm workspaces.
Risk & Scope
stream_errorattempt after a socket error; these are defensive cleanup changes, while reconnect, replay, queue, backpressure, and eviction semantics remain unchanged.Linked Issues
Closes #8571
中文说明
本 PR 做了什么
为 REST SSE 连接增加一期可观测性。每条成功接受的流都会获得稳定的 UUID，并针对打开、慢客户端告警、淘汰、状态重同步和关闭发出可关联的生命周期遥测与 Daemon 日志。关闭记录包含持续时间、已完成写入的帧数、最后写入的事件 ID、背压与实时延迟统计、终态事件归因以及明确的关闭原因。
TypeScript SDK 会携带现有客户端身份，以及可选的连接原因和相邻成功流的 lineage，同时兼容旧版 Daemon 和被网关剥离响应头的情况。WebUI 只在能够区分 Prompt 主动重启、正常流结束、传输错误或状态重同步时提供原因。订阅者诊断包含队列与触发事件元数据，但不包含事件 payload；会话身份仍由 SSE 路由负责。
为什么需要它
session ID 能标识逻辑会话，却无法区分重连产生的多条物理 SSE 连接。因此，现有慢客户端告警无法可靠关联到具体 stream、前驱流、队列状态、写背压、Replay Gap 或关闭结果。此变更使仅凭会话维度查询即可重建 REST SSE 时间线，并区分客户端重启风暴、网络或代理读取缓慢、大帧、大量小帧积压和 Replay 重同步，同时不改变重连、Replay、队列、背压或淘汰行为。
Reviewer 测试计划
如何验证
cd packages/acp-bridge && npx vitest run src/eventBus.test.ts、cd packages/core && npx vitest run src/telemetry/daemon-tracing.test.ts、cd packages/sdk-typescript && npx vitest run test/unit/AcpHttpTransport.test.ts test/unit/DaemonSessionClient.test.ts test/unit/RestSseTransport.test.ts、cd packages/webui && npx vitest run src/daemon/session/DaemonSessionProvider.test.tsx以及cd packages/cli && npx vitest run src/serve/auth.test.ts src/serve/server.test.ts。诊断、遥测上下文、REST 握手与 lineage、WebUI 原因转换和 Daemon 生命周期测试应全部通过。本地定向验证已通过：58 个 EventBus 测试、13 个 Daemon 遥测测试、169 个 SDK 测试、181 个 WebUI 测试，以及 rebase 到当前 main 后的 40 个定向 Daemon SSE 测试。相关 package 的 build、typecheck、lint、格式检查和 diff 检查也已通过。全仓 CLI build/typecheck 仍被与本变更无关的既有 Ink selection 类型错误阻塞。
证据（变更前后）
N/A——仅涉及协议诊断与可观测性，没有 TUI 变化。
已测试平台
环境（可选）
macOS、Node.js v22.22.3、npm 10.9.8、npm workspaces。
风险与范围
stream_error写入；这些属于防御性清理改动，重连、Replay、队列、背压和淘汰语义保持不变。关联 Issue
Closes #8571