Skip to content

fix(qqbot): streaming idle-flush with tool-call and stale-callback protection - #6204

Merged
wenshao merged 14 commits into
QwenLM:mainfrom
Eric-GoodBoy-Tech:fix/qqbot-streaming-idle-flush
Jul 4, 2026
Merged

fix(qqbot): streaming idle-flush with tool-call and stale-callback protection#6204
wenshao merged 14 commits into
QwenLM:mainfrom
Eric-GoodBoy-Tech:fix/qqbot-streaming-idle-flush

Conversation

@Eric-GoodBoy-Tech

@Eric-GoodBoy-Tech Eric-GoodBoy-Tech commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

What this PR does

Adds streaming infrastructure to the QQ Bot channel adapter: idle-flush buffering, tool-call coordination, and stale-callback protection. Text chunks are buffered and flushed after 2 seconds of inactivity (idle-flush), or immediately before a tool call executes. Multiple race-condition guards prevent duplicate sends, stale callbacks from previous reconnects, and buffer loss during concurrent events. PR 3 of 4 splitting PR #5902.

Why it's needed

QQ Bot users currently receive AI responses as a single large message only after the entire generation completes. For long responses or agent tool-call loops, this creates a "blackout" period where nothing appears for 30+ seconds. Streaming with idle-flush delivers partial responses incrementally, giving users real-time feedback without overwhelming them (or hitting rate limits) with every individual token chunk. The 2s idle timer balances responsiveness against QQ Bot API rate limits.

Reviewer Test Plan

How to verify

  1. Configure a QQ Bot channel with streaming enabled (default behavior).
  2. Send a message that triggers a long AI response (e.g. "write a detailed analysis of...").
  3. Observe that partial text appears in ~2-second intervals while the response is still generating.
  4. Trigger a tool call (e.g. "search the web for X") and verify the preceding text is flushed before the tool executes.
  5. Disconnect and reconnect during an active streaming response — verify no duplicate or stale messages appear.
  6. Enable blockStreaming: 'on' in the QQ Bot config — verify messages are sent only on completion, matching the pre-streaming behavior.

Evidence (Before & After)

Before: User waits 30-60s with no feedback, then receives the full response as a single message.
After: User receives partial messages every 2s while the response generates, with tool-call results interleaved.

Tested on

OS Status
🍏 macOS ✅ tested
🪟 Windows N/A
🐧 Linux N/A

Risk & Scope

  • Main risk or tradeoff: The 2s idle timer adds up to 2s of latency to partial message delivery. If the QQ Bot API returns 429/500 during a streaming flush, the buffer is restored and a retry timer is armed (no data loss).
  • Not validated / out of scope: End-to-end streaming behavior with a live QQ Bot API instance; only unit-test coverage.
  • Breaking changes / migration notes: None. blockStreaming: 'on' restores the original send-on-complete behavior.

Linked Issues

Part of #5902

…otection

Add streaming infrastructure to QQ Bot channel:
- streamState Map with per-session buffer and 2s idle-flush timer
- onResponseChunk: accumulates text, resets timer on each chunk
- idleFlush: flushes accumulated buffer, coordinates with pendingStreamDelete
- onToolCall: flushes buffer before tool execution with double-send guard
- onResponseComplete: defers streamState cleanup during flush
- _reconnectId monotonic counter for stale async callback detection
- blockStreaming config-driven guard (skip streaming when enabled)
- All sendMessage calls chained with .catch() for Node 22+ safety

Add 25 stream tests covering:
- idle-flush accumulation and timer reset
- onToolCall immediate flush
- pendingStreamDelete coordination
- Stale reconnect callback guard
- blockStreaming guard
- Concurrent flush prevention

@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.

Hey @Eric-GoodBoy-Tech — thanks for the PR! The streaming infrastructure work looks interesting.

However, the PR body doesn't follow our PR template. The template has specific headings that help reviewers quickly assess changes — things like What this PR does, Why it's needed, Reviewer Test Plan, Risk & Scope, and Linked Issues. Your current description uses a different structure (Summary, Changes, Testing, Dependencies).

Could you update the PR body to follow the template? It makes a big difference for review turnaround — maintainers prioritize PRs with a clear reviewer test plan.

A few specific things that would help:

  • Why it's needed — explain the motivation: what user problem does streaming solve for QQ Bot users? Why idle-flush instead of sending every chunk?
  • Reviewer Test Plan — how can a reviewer verify this works end-to-end? What should they expect to see?
  • Risk & Scope — any known tradeoffs (e.g., the 2s idle timer adds latency to message delivery)?
  • Linked Issues — link to the parent issue or PR #5902 with the right keyword (Part of #5902)

Once the body is updated, happy to re-run the review. 🙏

中文说明

@Eric-GoodBoy-Tech 你好!感谢提交 PR,流式传输的基础设施看起来很有价值。

不过 PR 描述没有按照我们的 PR 模板 来填写。模板有固定的标题结构(What this PR doesWhy it's neededReviewer Test PlanRisk & ScopeLinked Issues 等),方便维护者快速评估。当前的描述用了不同的结构(SummaryChangesTestingDependencies)。

请按照模板更新 PR 描述,这会显著加快审核速度。特别需要补充:

  • Why it's needed — 为什么 QQ Bot 需要流式传输?idle-flush 方案解决了什么用户痛点?
  • Reviewer Test Plan — 审核者如何端到端验证?预期行为是什么?
  • Risk & Scope — 有没有已知的取舍(比如 2 秒 idle 计时器会增加消息延迟)?
  • Linked Issues — 用正确的关键词关联到父 issue 或 PR #5902

更新后可以重新触发审核。

Qwen Code · qwen3.7-max

Comment thread packages/channels/qqbot/src/QQChannel.ts
Comment thread packages/channels/qqbot/src/QQChannel.ts Outdated
Comment thread packages/channels/qqbot/src/QQChannel.ts
Comment thread packages/channels/qqbot/src/QQChannel.ts
Comment thread packages/channels/qqbot/src/QQChannel.ts Outdated
@Eric-GoodBoy-Tech

Copy link
Copy Markdown
Contributor Author

Thanks for the review! All feedback addressed in 480ab05:

C1 — Duplicate message on onResponseComplete: Added flushedSessions tracking set. onResponseComplete now checks whether any flush already delivered content before falling back to fullText. Cleaned up on both session completion and disconnect.

C2 — Chunks dropped in idleFlush else-branch: Added !state.buffer guard to the else-branch, matching the pendingStreamDelete branch's existing guard.

C3 — Unreachable .catch() handlers: sendMessage now throws on HTTP errors when propagateError=true. idleFlush passes propagateError=true so its .catch() is reachable on both HTTP errors (429/500) and network exceptions.

C4 — No retry timer after flush failure: Both idleFlush and onToolCall .catch() handlers now re-arm the idle timer after restoring the buffer, using a fresh _reconnectId.

C5 — \\n literal escape: Fixed to \n in the idleFlush error message (the onToolCall message already had the correct \n).

PR body: Updated to follow the template with What/Why/Test Plan/Risk/Linked Issues sections.

All 97 tests pass, typecheck clean.

Comment thread packages/channels/qqbot/src/QQChannel.ts
Comment thread packages/channels/qqbot/src/QQChannel.ts Outdated
Comment thread packages/channels/qqbot/src/QQChannel.ts
Comment thread packages/channels/qqbot/src/QQChannel.ts
Comment thread packages/channels/qqbot/src/QQChannel.ts Outdated
Comment thread packages/channels/qqbot/src/QQChannel.ts Outdated
@wenshao

wenshao commented Jul 3, 2026

Copy link
Copy Markdown
Collaborator

@qwen-code /resolve

…or conflict

Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
@qwen-code-dev-bot

Copy link
Copy Markdown
Collaborator

Qwen Code resolved the merge conflicts and pushed the branch update.

Merge Conflict Resolution Summary

PR: #6204 - fix(qqbot): streaming idle-flush with tool-call and stale-callback protection

Conflict Location

  • File: packages/channels/qqbot/src/QQChannel.ts (lines 334-341)

What Conflicted

PR branch (HEAD) had:

process.stderr.write(`[QQ:${this.name}] Send error: ${e}\n`);
if (propagateError) throw e;

origin/main had:

process.stderr.write(
  `[QQ:${this.name}] Send error: ${sanitizeLogText(e instanceof Error ? e.message : String(e), 200)}\n`,
);

Resolution

Combined both improvements:

  • From main: Use sanitizeLogText() to safely sanitize error messages (prevents log injection and limits message length to 200 chars)
  • From PR: Keep the if (propagateError) throw e; conditional error propagation

Resolved code:

process.stderr.write(
  `[QQ:${this.name}] Send error: ${sanitizeLogText(e instanceof Error ? e.message : String(e), 200)}\n`,
);
if (propagateError) throw e;

Rationale

Both changes serve different purposes:

  1. sanitizeLogText (main) is a security/stability improvement that prevents log injection attacks and unbounded log lines
  2. propagateError (PR) is a feature that allows callers to receive errors when needed for proper error handling

The resolution preserves both improvements without conflict.

Commit

  • Message: fix(qqbot): merge origin/main, resolve sanitizeLogText + propagateError conflict
  • SHA: 5c72b4e33

Comment thread packages/channels/qqbot/src/stream.test.ts
Comment thread packages/channels/qqbot/src/QQChannel.ts Outdated
Comment thread packages/channels/qqbot/src/QQChannel.ts
Comment thread packages/channels/qqbot/src/QQChannel.ts
Comment thread packages/channels/qqbot/src/QQChannel.ts
Comment thread packages/channels/qqbot/src/QQChannel.ts
Comment thread packages/channels/qqbot/src/QQChannel.ts Outdated
Comment thread packages/channels/qqbot/src/QQChannel.ts Outdated
Comment thread packages/channels/qqbot/src/QQChannel.ts Outdated
Comment thread packages/channels/qqbot/src/QQChannel.ts
Comment thread packages/channels/qqbot/vitest.config.ts Outdated
@wenshao

wenshao commented Jul 3, 2026

Copy link
Copy Markdown
Collaborator

@qwen-code /resolve

@qwen-code-dev-bot

Copy link
Copy Markdown
Collaborator

Qwen Code resolved the merge conflicts and pushed the branch update.

Merge Conflict Resolution Summary — PR #6204

PR: fix(qqbot): streaming idle-flush with tool-call and stale-callback protection
Branch: qwen-resolve/pr-6204origin/main
Commit: merge: resolve sendMessage propagateError + plain-text fallback conflicts with origin/main

Conflicted File

packages/channels/qqbot/src/QQChannel.ts (only conflicted file)

Conflict 1: sendMessage signature + <noreply> suppression

Location: ~line 218 (sendMessage method declaration)

Side Code
HEAD (PR) async sendMessage(chatId: string, text: string, propagateError = false): Promise<void> — 3-param signature, no <noreply> handling
main async sendMessage(chatId: string, text: string): Promise<void> — 2-param signature + <noreply> suppression block at top of method

Resolution: Took main's version (2-param + <noreply> suppression). The propagateError parameter is redundant because main's outer catch already does throw e; // Re-throw for .catch() callers, which achieves the same effect — callers with .catch() handlers receive the error regardless.

Post-conflict fix: Removed , true from two call sites:

  • this.sendMessage(state.chatId, buffer, true)this.sendMessage(state.chatId, buffer) (idleFlush, ~line 493)
  • this.sendMessage(state.chatId, buffer, true)this.sendMessage(state.chatId, buffer) (onToolCall, ~line 542)

Conflict 2: Error handling after active retry fails

Location: ~line 339-389 (inside sendMessage, after markdown send fails and active retry also fails)

Side Code
HEAD (PR) Old loop-based if (!resp.ok) with propagateError check and break to stop chunk iteration
main Plain-text fallback path: when markdown fails and there's no reply context (no msg_id), sends as msg_type: 0 with comprehensive 429 and error handling

Resolution: Took main's plain-text fallback entirely. HEAD's code was fro

Comment thread packages/channels/qqbot/src/QQChannel.ts
Comment thread packages/channels/qqbot/src/stream.test.ts
Comment thread packages/channels/qqbot/src/QQChannel.ts
Comment thread packages/channels/qqbot/src/QQChannel.ts Outdated
- Extract IDLE_FLUSH_MS and MAX_FLUSH_RETRIES constants
- Add JSDoc to state machine transitions
- Implement onSessionDied lifecycle handler
- Track retryCount per session for bounded retries
- Sanitize log text via sanitizeLogText utility
- Guard zombie timer with !current.timer check
- Clean up flushedSessions and pendingStreamDelete entries
- Add 8 error-recovery test cases (retry, max retries, pendingStreamDelete,
  onToolCall retry, stale closure, disconnect cleanup, onSessionDied,
  flushingSessions guard)
- Fix vitest.config.ts server.deps.inline placement
Comment thread packages/channels/qqbot/src/QQChannel.ts
Comment thread packages/channels/qqbot/src/QQChannel.ts
Comment thread packages/channels/qqbot/src/QQChannel.ts
Comment thread packages/channels/qqbot/src/QQChannel.ts
Comment thread packages/channels/qqbot/src/QQChannel.ts Outdated
Comment thread packages/channels/qqbot/src/stream.test.ts
Comment thread packages/channels/qqbot/src/QQChannel.ts Outdated

@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.

Qwen Code Review Summary

PR: #6204 — fix(qqbot): streaming idle-flush with tool-call and stale-callback protection
Agents: 9 parallel reviewers (correctness, security, code quality, performance, test coverage, attacker/oncall/maintainer mindsets, build verification)
Build/Tests: ✅ 36 tests passed, typecheck clean

Findings

# Severity File Summary
1 🔴 Critical QQChannel.ts:562 flushedSessions.delete() in .catch() erases ALL prior delivery tracking → duplicate messages
2 🔴 Critical QQChannel.ts:546 Orphaned .then() repopulates tracking sets after session death → silent data loss on session-ID reuse
3 🟡 Suggestion QQChannel.ts:552 .then() accesses s.buffer without null guard — TypeError when streamState deleted in-flight
4 🟡 Suggestion QQChannel.ts:501 Unbounded buffer accumulation — defeats streaming for fast LLM generation
5 🟡 Suggestion stream.test.ts Missing test for .then() preserving streamState when chunks arrive during successful flush
6 🟡 Suggestion stream.test.ts:772 onSessionDied test doesn't assert clearTimeout was called

Note on existing issues

This PR already has ~38 inline comments from prior reviews. The findings above are new issues not covered by existing comments. The most impactful pre-existing issue remains the sendMessage resolves-on-HTTP-errors behavior (comment #31), which continues to affect the streaming layer's error recovery.

Comment thread packages/channels/qqbot/src/QQChannel.ts
Comment thread packages/channels/qqbot/src/QQChannel.ts
Comment thread packages/channels/qqbot/src/QQChannel.ts
Comment thread packages/channels/qqbot/src/QQChannel.ts
Comment thread packages/channels/qqbot/src/stream.test.ts
Comment thread packages/channels/qqbot/src/stream.test.ts
Comment thread packages/channels/qqbot/src/QQChannel.ts
Comment thread packages/channels/qqbot/src/QQChannel.ts
Comment thread packages/channels/qqbot/src/QQChannel.ts
Comment thread packages/channels/qqbot/src/QQChannel.ts Outdated
Comment thread packages/channels/qqbot/src/QQChannel.ts Outdated
Comment thread packages/channels/qqbot/src/QQChannel.ts
Comment thread packages/channels/qqbot/src/QQChannel.ts
Comment thread packages/channels/qqbot/src/QQChannel.ts
Comment thread packages/channels/qqbot/src/stream.test.ts
Comment thread packages/channels/qqbot/src/stream.test.ts
Comment thread packages/channels/qqbot/src/stream.test.ts Outdated
- Restore readyTimeout (30s READY guard) in dialGateway()
- Restore heartbeatTimer.unref() in startHeartbeat()
- Restore seenMessages.clear() in disconnect()
- Restore event.author defensive check in handleC2C()
Comment thread packages/channels/qqbot/src/QQChannel.ts Outdated
Comment thread packages/channels/qqbot/src/QQChannel.ts Outdated
Comment thread packages/channels/qqbot/src/QQChannel.ts
Comment thread packages/channels/qqbot/src/QQChannel.ts Outdated
Comment thread packages/channels/qqbot/src/QQChannel.ts Outdated

@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.

Two bugs found in the streaming infrastructure:

  1. Recursive flushAndTrack defeats concurrency guard + skips retry counter (Critical) — the .catch() handler's recursive call at line 636 passes the same state reference, causing the outer .finally() to clear flushingSessions while the inner send is in-flight. Combined with return bypassing retryCount++, this creates both a concurrency guard defeat and a potential infinite async retry loop.

  2. readyTimeout timer leaks on disconnect() (Suggestion) — disconnect() calls this.ws.close() and nulls this.ws, which triggers the stale-close guard in the close handler (line 1122), skipping readyTimeout cleanup at line 1268. The 30-second timer keeps the Node.js process alive. Fix: add clearTimeout(this.readyTimeout); this.readyTimeout = null; before the streaming cleanup block.

Comment thread packages/channels/qqbot/src/QQChannel.ts Outdated
Comment thread packages/channels/qqbot/src/QQChannel.ts

@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.

Review Summary

Verdict: Comment (3 suggestions, no blockers)

This PR adds well-structured streaming infrastructure to the QQ Bot channel adapter. The idle-flush buffering, tool-call coordination, and stale-callback protection are thoughtfully designed, and the test coverage (41 tests, 1102 lines) is thorough.

Three minor suggestions below:

  1. Missing \n in .catch() stderr writes (lines 595, 621, 656) — These three process.stderr.write calls in flushAndTrack's .catch() handler don't end with \n, unlike every other stderr write in this file. Output will run into the next log line.

  2. handleGroup missing !event.author guardhandleC2C was patched with if (!event.author) return; (line 1444) but handleGroup accesses event.author.username (line 1481) without a null check, risking a TypeError on malformed events.

  3. readyTimeout missing .unref() (line 1099) — Every other timer in this class calls .unref() to avoid keeping the process alive. This 30-second timer is the exception.

Comment thread packages/channels/qqbot/src/QQChannel.ts Outdated
Comment thread packages/channels/qqbot/src/QQChannel.ts

@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.

⚠️ Downgraded from Request Changes to Comment: CI still running.

— qwen3.7-max via Qwen Code /review

Comment thread packages/channels/qqbot/src/QQChannel.ts
Comment thread packages/channels/qqbot/src/QQChannel.ts
Comment thread packages/channels/qqbot/src/QQChannel.ts
…stderr newlines, readyTimeout unref, reconnect log

@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 new review findings beyond the 50+ existing inline comments from prior rounds. Downgraded from Approve to Comment: CI still running.

This PR has been reviewed to exceptional depth across multiple prior review rounds. The streaming state machine is well-structured and all 41 tests pass. Deterministic analysis (tsc, eslint) is clean. The extensive prior feedback covers race conditions, identity guards, retry logic, timer lifecycle, and test coverage gaps comprehensively.

— qwen3.7-max via Qwen Code /review

@wenshao

wenshao commented Jul 4, 2026

Copy link
Copy Markdown
Collaborator

@qwen-code /triage

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Thanks for the PR, @Eric-GoodBoy-Tech!

Template: ✓ All required headings present.

Problem: Real and observable. QQ Bot currently delivers responses as a single message only after full generation completes. For long responses or tool-call loops, users face 30+ seconds of silence. This is an inherent limitation of the current implementation.

Direction: Aligned with the project. Feishu already implements progressive display via the same onResponseChunk/onResponseComplete override pattern. Splitting from the larger PR #5902 (3 of 4) is good practice — keeps each change reviewable.

Solution: Scope is reasonable for streaming infrastructure. The ~1400 lines are mostly tests (1102 lines), with the streaming logic itself around 300 lines. Nothing here looks unnecessary for the stated goal of idle-flush buffering with tool-call coordination and stale-callback protection.

Minor note: the PR title uses fix: but this adds new streaming infrastructure — feat: would be more accurate. Not a blocker.

Moving to code review. 🔍

中文说明

感谢贡献!

模板完整 ✓

问题:真实且可观测的问题。QQ Bot 目前仅在完整生成后才发送单条消息。对于长回复或工具调用循环,用户会面临 30 秒以上的无反馈等待。这是当前实现的固有局限。

方向:与项目对齐。飞书已通过相同的 onResponseChunk/onResponseComplete 覆盖模式实现了渐进式显示。从较大的 PR #5902(第 3/4 部分)中拆分出来是好的做法——保持每个变更可审查。

方案:对于流式基础设施来说范围合理。约 1400 行主要是测试(1102 行),流式逻辑本身约 300 行。对于 idle-flush 缓冲加工具调用协调和过期回调保护的目标,没有看到不必要的内容。

小备注:PR 标题用了 fix:,但这实际是新增流式基础设施——feat: 更准确。不阻塞。

进入代码审查 🔍

Qwen Code · qwen3.7-max

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Code review:

The streaming state machine is well-documented with a clear comment block showing states (active → flushing → idle), transitions, and guards. All four override methods (onResponseChunk, onToolCall, onResponseComplete, onSessionDied) correctly follow the ChannelBase override pattern — proper override keyword, correct signatures, super calls where needed.

flushAndTrack() is the core async coordination point and handles concurrent sends, timer-based flushing, error recovery with re-buffering, and identity guards comprehensively. The identity guards (current === state reference comparison) correctly prevent stale-state mutation after session death/recreation. The readyTimeout addition (30s fail-fast for gateway READY phase) is a solid improvement with proper cleanup across READY, RESUMED, disconnect, and WebSocket close paths.

One minor cosmetic issue: a stray blank line was added between the qqStatePath comment and its declaration.

No critical bugs found.

Testing:

 RUN  v3.2.4 packages/channels/qqbot

 ✓ src/stream.test.ts (41 tests) 22ms

 Test Files  1 passed (1)
      Tests  41 passed (41)
   Duration  351ms

41 tests covering: idle-flush timer, tool-call flush, send failure re-buffer and retry, race conditions (stale reconnectId, flushingSessions guard), onResponseComplete coordination with in-flight sends, blockStreaming opt-out, disconnect/sessionDied cleanup, buffer limit flush, identity guards, and pendingStreamDelete lifecycle. Tests use vitest fake timers and properly mock sendQQMessage. The drain() microtask helper handles async promise chain timing.

Real-scenario tmux testing: Not feasible — QQ Bot streaming requires a live QQ Gateway API WebSocket connection, which is unavailable in this CI environment. Unit tests are the primary verification for this channel-adapter change.

Build: ✓ Full project build succeeds.

The vitest.config.ts correctly adds @qwen-code/channel-base to server.deps.inline for proper test module resolution.

中文说明

代码审查:流式状态机有清晰的文档注释,展示了状态(active → flushing → idle)、转换和守卫。所有四个覆盖方法(onResponseChunkonToolCallonResponseCompleteonSessionDied)正确遵循 ChannelBase 覆盖模式。

flushAndTrack() 是核心异步协调点,全面处理了并发发送、基于定时器的刷新、带重新缓冲的错误恢复和身份守卫。身份守卫(current === state 引用比较)正确防止了会话死亡/重建后的过期状态变更。readyTimeout 的添加(网关 READY 阶段的 30 秒快速失败)是一个扎实的改进。

一个小的格式问题:qqStatePath 注释和声明之间多了一个空行。

未发现关键 bug。

测试:41 个测试全部通过,覆盖 idle-flush 定时器、工具调用刷新、发送失败重新缓冲和重试、竞态条件、onResponseComplete 协调、blockStreaming、断开连接/sessionDied 清理、缓冲区限制刷新、身份守卫和 pendingStreamDelete 生命周期。

真实场景 tmux 测试:不可行——QQ Bot 流式传输需要实时 QQ Gateway API WebSocket 连接,在当前 CI 环境中不可用。单元测试是此频道适配器变更的主要验证手段。

构建:✓ 完整项目构建成功。

Qwen Code · qwen3.7-max

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Stepping back: this is solid engineering. The streaming coordination problem — concurrent sends, timer-based flushes, error recovery, session lifecycle — is genuinely complex, and it's handled cleanly. The state machine has clear documentation, the guards are disciplined (identity checks, flushing guards, reconnectId stale-callback protection), and the test suite covers state transitions and edge cases thoroughly (41 tests).

The code follows project conventions. The override pattern matches what Feishu, Telegram, and other channel adapters already use. Every change in the diff is necessary for the stated goal — no drive-by refactors or scope creep.

Approved. ✅

中文说明

退一步看:这是扎实的工程工作。流式协调问题——并发发送、基于定时器的刷新、错误恢复、会话生命周期——确实很复杂,而且处理得很干净。状态机有清晰的文档,守卫严谨(身份检查、刷新守卫、reconnectId 过期回调保护),测试套件全面覆盖了状态转换和边缘情况(41 个测试)。

代码遵循项目规范。覆盖模式与飞书、Telegram 和其他频道适配器已使用的模式一致。diff 中的每个更改都是实现目标所必需的——没有顺手重构或范围蔓延。

已批准 ✅

Qwen Code · qwen3.7-max

@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.

LGTM, looks ready to ship. ✅

@wenshao

wenshao commented Jul 4, 2026

Copy link
Copy Markdown
Collaborator

✅ Local verification report — real binary, no live QQ transport

Verified PR head 2de6f7199d with the real compiled channel (only the QQ HTTP transport / sendMessage is faked). Verdict: the streaming feature is correct end-to-end and mergeable. One low-severity, non-blocking edge case is documented below (with a one-line fix).

How it was verified

Tier What Result
① Unit PR's own stream.test.ts (channel-base mocked) 41/41 pass
② E2E New harness driving the real ChannelBase.handleInbound dispatch (channel-base NOT mocked) 4/4 pass
③ Mutation Break each load-bearing guard, confirm tests flip red 6/6 killed
④ Build tsc --build (production, excludes tests) EXIT 0
⑤ Live Real-timer streaming demo (idle-flush / tool-flush / completion) 3 triggers observed

① + ⑤ Live streaming behavior (real base, real timers)

The IDLE_FLUSH_MS=2000 idle-flush, the immediate tool-call flush, and the completion delivery all fire correctly in wall-clock time:

live streaming

② Why a real-base E2E was needed

The PR's own suite does vi.mock('@qwen-code/channel-base', …), so it proves the state-machine in isolation but never exercises the real base dispatch that calls the overrides in production. I added an E2E that does not mock the base and drives the real handleInbound → bridge.prompt → emit('textChunk'/'toolCall') → onResponseChunk/onToolCall/onResponseComplete chain. It confirms the contracts a mock can hide:

  • A — argument order onResponseChunk(chatId, chunk, sessionId) is honored; incremental idle-flush delivers partials and never re-sends already-flushed text.
  • B — with blockStreaming: 'on', the base's BlockStreamer delivers and QQ's own idle-flush stays silent (the if (this.blockStreaming) return; guard prevents a double-send — the base calls onResponseChunk even in block mode, so this guard is load-bearing).
  • C — a tool_call flushes the buffered pre-tool text immediately via the real dispatchToolCall, before the tool runs.
  • D — a real bridge sessionDied event clears the per-session buffer.

I also confirmed the invariant the completion path relies on: onResponseComplete(fullText) always receives chunks.join('') — verified in both AcpBridge.ts:198 and DaemonChannelBridge.ts:331 (return chunks.join('')). That is exactly what makes remaining = state?.buffer ?? (wasFlushed ? '' : fullText) correct: when streaming happened, fullText was already delivered chunk-by-chunk, so only the buffered tail is re-sent.

test evidence

③ Mutation testing (tests are non-vacuous)

Each guard was broken in QQChannel.ts; every mutation was killed by the suite (M1/M3/M5 are caught by both the unit suite and the real-base E2E — i.e. the E2E proves the base actually wires those hooks):

mutation matrix

🔎 Reverse audit

  • Loop-mode double-send — REFUTED. The base loop path also calls onResponseChunk (ChannelBase.ts:599) but delivers via pushProactive, not onResponseComplete, which would double-send streamed text. However QQ inherits supportsProactiveSend() === false, so runLoopPrompt throws before any loop streaming — unreachable for QQ.

  • ⚠️ Finding (low severity, non-blocking): the MAX_BUFFER_LENGTH immediate-flush path skips the flushingSessions guard. In onResponseChunk the buffer.length >= MAX_BUFFER_LENGTH branch (QQChannel.ts:516) calls flushAndTrack directly — unlike idleFlush (:539) and onToolCall (:684), which both bail with if (this.flushingSessions.has(...)) return;. So if the buffer crosses 4096 chars while a previous flush is still in-flight, a second concurrent sendMessage starts for the same session — violating the PR's own documented invariant "flushingSessions prevents concurrent sends per session."

    • Reproduced: with a never-settling first send, feeding a 5000-char chunk yields 2 concurrent in-flight sends; a control (small chunk) stays at 1.
    • Reachability (low but non-zero): streaming continues during a slow / rate-limited send, or a large held-chunk batch is replayed after a failed cancel. msgSeqMap is incremented synchronously before the await, so the two sends get distinct seqs (no duplicate-seq rejection), but they can be delivered out of order, and the two flushAndTrack closures share one state object — their .then/.catch can interleave and corrupt retryCount/buffer on mixed success/failure.
    • One-line fix (verified: flips the repro 2→1, keeps all 41 unit + 4 E2E green):
      // onResponseChunk, MAX_BUFFER branch
      if (
        state.buffer.length >= QQChannel.MAX_BUFFER_LENGTH &&
        !this.flushingSessions.has(sessionId)   // ← align with idleFlush / onToolCall
      ) {
        const buf = state.buffer;
        state.buffer = '';
        this.flushAndTrack(sessionId, buf, state, 'idleFlush');
        return;
      }

Recommendation

Mergeable as-is. The concurrent-send edge case above is optional hardening — it only triggers under a narrow rate-limit/replay window and does not lose data. Also independently sanity-checked: readyTimeout (30s) cleanup, heartbeatTimer.unref(), and the if (!event.author) return; guard are all consistent with the surrounding code.

🇨🇳 中文版(完整对应)

✅ 本地验证报告 —— 真实二进制,无实时 QQ 连接

在 PR head 2de6f7199d 上,用真实编译后的 channel 验证(仅伪造 QQ 的 HTTP 传输 / sendMessage)。结论:流式功能端到端正确,可合并。 下面记录一处低严重度、非阻塞的边界情况(附一行修复)。

验证方式

层级 内容 结果
① 单测 PR 自带 stream.test.ts(channel-base 被 mock 41/41 通过
② E2E 新 harness 驱动真实 ChannelBase.handleInbound 分发(channel-base 不 mock 4/4 通过
③ 变异 逐个破坏承重守卫,确认测试翻红 6/6 被杀
④ 构建 tsc --build(生产,排除测试) EXIT 0
⑤ 实时 真实计时器流式演示(idle-flush / 工具 flush / 完成) 观察到 3 种触发

① + ⑤ 实时流式行为(真实 base + 真实计时器)

IDLE_FLUSH_MS=2000 的 idle-flush、工具调用的立即 flush、以及完成投递,都在真实时钟下正确触发(见上方第一张图)。

② 为什么需要真实 base 的 E2E

PR 自带套件 vi.mock('@qwen-code/channel-base', …),因此它只验证了孤立的状态机,从未跑过生产环境中真正调用这些 override 的 base 分发。我补了一个不 mock base 的 E2E,驱动真实的 handleInbound → bridge.prompt → emit('textChunk'/'toolCall') → onResponseChunk/onToolCall/onResponseComplete 链路,验证了 mock 会掩盖的契约:

  • A —— 参数顺序 onResponseChunk(chatId, chunk, sessionId) 正确;增量 idle-flush 投递分段且不重发已发送内容。
  • B —— blockStreaming: 'on' 时,base 的 BlockStreamer 负责投递, QQ 自己的 idle-flush 保持沉默(if (this.blockStreaming) return; 守卫防止双发 —— base 在 block 模式下仍会onResponseChunk,所以这条守卫是承重的)。
  • C —— tool_call 通过真实 dispatchToolCall 在工具执行前立即 flush 已缓冲的文本。
  • D —— 真实 bridge 的 sessionDied 事件清理该 session 的缓冲。

我还确认了完成路径依赖的不变量:onResponseComplete(fullText) 收到的永远是 chunks.join('') —— 在 AcpBridge.ts:198DaemonChannelBridge.ts:331return chunks.join(''))双双验证。这正是 remaining = state?.buffer ?? (wasFlushed ? '' : fullText) 成立的原因:只要发生过流式,fullText 已被逐块投递,因此只重发缓冲的尾部(见上方第二张图)。

③ 变异测试(证明测试非空过)

逐个破坏 QQChannel.ts 的守卫,全部被套件杀掉(M1/M3/M5 被单测真实-base E2E 同时杀 —— 即 E2E 证明了 base 确实接线了这些钩子,见上方第三张图)。

🔎 反向审计

  • Loop 模式双发 —— 已证伪。 base 的 loop 路径也调 onResponseChunkChannelBase.ts:599),但通过 pushProactive 而非 onResponseComplete 投递,理论上会把流式文本发两遍。但 QQ 继承了 supportsProactiveSend() === falserunLoopPrompt 在任何 loop 流式前就抛错 —— 对 QQ 不可达。

  • ⚠️ 发现(低严重度、非阻塞):MAX_BUFFER_LENGTH 立即 flush 路径漏掉了 flushingSessions 守卫。 onResponseChunkbuffer.length >= MAX_BUFFER_LENGTH 分支(QQChannel.ts:516)直接调 flushAndTrack —— 而 idleFlush:539)和 onToolCall:684)都用 if (this.flushingSessions.has(...)) return; 提前返回。因此当缓冲在前一次 flush 仍在途时越过 4096 字符,会对同一 session 发起第二个并发 sendMessage,违反 PR 自己文档写的不变量*"flushingSessions prevents concurrent sends per session"*。

    • 已复现: 让第一次发送永不 settle,再喂一个 5000 字符的 chunk,得到两个并发在途发送;对照组(小 chunk)保持 1
    • 可达性(低但非零): 流式在一次慢速/被限流的发送期间继续,或失败取消后回放大批 held chunk。msgSeqMapawait 前同步自增,所以两次发送拿到不同 seq(不会因重复 seq 被拒),但可能乱序投递,且两个 flushAndTrack 闭包共享同一个 state 对象 —— 在成功/失败混合时它们的 .then/.catch 会交错并污染 retryCount/buffer
    • 一行修复(已验证:把复现从 2 翻回 1,且 41 单测 + 4 E2E 全绿):
      // onResponseChunk, MAX_BUFFER 分支
      if (
        state.buffer.length >= QQChannel.MAX_BUFFER_LENGTH &&
        !this.flushingSessions.has(sessionId)   // ← 与 idleFlush / onToolCall 对齐
      ) {
        const buf = state.buffer;
        state.buffer = '';
        this.flushAndTrack(sessionId, buf, state, 'idleFlush');
        return;
      }

建议

可直接合并。 上述并发发送边界情况属于可选加固 —— 只在很窄的限流/回放窗口触发,且不丢数据。另外也独立核对了:readyTimeout(30s)清理、heartbeatTimer.unref()、以及 if (!event.author) return; 守卫都与周边代码一致。

Verified locally with a worktree at PR head; real compiled ChannelBase + SessionRouter + BlockStreamer, fake bridge, isolated temp HOME. No live QQ Bot credentials used.

@wenshao
wenshao added this pull request to the merge queue Jul 4, 2026
Merged via the queue into QwenLM:main with commit abec702 Jul 4, 2026
35 checks passed
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.

4 participants