Skip to content

fix(core): stream chat-compression side-query to survive gateway timeout - #5865

Merged
wenshao merged 7 commits into
QwenLM:mainfrom
doudouOUC:claude/compassionate-morse-c16cc1
Jun 25, 2026
Merged

fix(core): stream chat-compression side-query to survive gateway timeout#5865
wenshao merged 7 commits into
QwenLM:mainfrom
doudouOUC:claude/compassionate-morse-c16cc1

Conversation

@doudouOUC

@doudouOUC doudouOUC commented Jun 25, 2026

Copy link
Copy Markdown
Collaborator

What this PR does

Adds an opt-in streaming path to the text side-query so the context-compression summary request can keep its HTTP connection alive while the model is still thinking. A new stream?: boolean option flows from ChatCompressionService.compress through runSideQuery down to BaseLlmClient.generateText; when set, the call routes through the already-existing generateContentStream and the streamed deltas are collected back into the same { text, usage } result the non-streaming path returns. Only the compression caller opts in — every other side-query keeps the unchanged non-streaming behaviour.

Why it's needed

When the context window approaches capacity, compression fires a summarization side-query that today runs non-streaming, so the entire LLM inference must finish before the first HTTP byte arrives. Behind a BFF gateway (e.g. DataWorks ACP / DQ_AGENT) whose proxy_read_timeout is ~60s, a long compression inference — a real production trace measured ~1.9 minutes — is killed by the gateway at 60s with a 504 Gateway Timeout, surfaced to qwen-code as UnprocessableEntityError: 422. Compression then fails, the turn errors out, and the session breaks even though the model itself would have succeeded. Streaming fixes this because the server returns HTTP 200 and an initial delta within seconds, which resets the gateway's read timeout on every chunk, so a slow inference can no longer trip the 60s ceiling.

Reviewer Test Plan

How to verify

Unit: cd packages/core && npx vitest run src/core/baseLlmClient.test.ts src/utils/sideQuery.test.ts src/services/chatCompressionService.test.ts — expect all green. New coverage asserts that stream: true routes through generateContentStream, concatenates multi-chunk deltas, trims, drops thought parts, and captures usage from the final chunk; that omitting stream still uses the non-streaming generateContent; that runSideQuery forwards the flag only when set; and that compression calls generateText with stream: true.

End-to-end (optional, needs a gateway): drive a long session against an OpenAI-compatible endpoint behind a ≤60s proxy_read_timeout proxy, force compression, and confirm the compression request now streams and completes where the non-streaming path previously failed with a 504/422.

Evidence (Before & After)

Non-user-visible change (core inference plumbing), so no UI before/after. Local unit run: Test Files 3 passed (3) · Tests 132 passed (132). Repo gates pass: npm run typecheck (all workspaces) and npm run lint both exit 0; the pre-commit hook (prettier + eslint --max-warnings 0) passed on the six changed files.

Tested on

OS Status
🍏 macOS
🪟 Windows ⚠️
🐧 Linux ⚠️

Environment (optional)

Unit tests only (vitest), plus npm run typecheck / npm run lint. No runtime/sandbox needed.

Risk & Scope

  • Main risk or tradeoff: The motivating deployment (DataWorks ACP / DQ_AGENT is OpenAI-compatible, served by OpenAIContentGenerator / QwenContentGenerator) routes through the OpenAI pipeline, which already wraps the stream in a per-chunk inactivity watchdog (withStreamInactivityTimeout, tunable via QWEN_STREAM_IDLE_TIMEOUT_MS / streamIdleTimeoutMs, 0 disables) — so a stream that returns 200 then goes silent is bounded there, not the old all-or-nothing non-streaming wait. Native USE_GEMINI / USE_ANTHROPIC streams have no per-chunk idle timeout at the provider layer, but that is the exact contract the main chat loop already consumes for every turn (geminiChat.ts iterates the same raw generateContentStream), bounded by the SDK transport timeout plus abortSignal; this side-query reuses that existing invariant rather than introducing a new exposure. The whole stream is consumed inside retryWithBackoff, so a mid-stream failure retries the entire idempotent request, matching the non-streaming retry semantics. A provider-agnostic idle timeout for Gemini/Anthropic would belong at the ContentGenerator layer (so the main loop benefits too) and is out of scope here.
  • Not validated / out of scope: The optional secondary improvements from the issue are intentionally not bundled here, to keep the change focused on the 504 root cause. "Disable thinking for compression" is already satisfied in current code — the compression call passes thinkingConfig: { includeThoughts: false } and applyThinkingDefault defaults every side-query to it (the issue's thoughts_token_count=1 was from an older preview build). "Use the fast model for compression" is a summarization-quality / model-selection change unrelated to the timeout and is left as a possible follow-up (see the related Disable reasoning on all fastModel side queries (follow-up to #3759) #3760 for the broader fastModel-reasoning topic). The end-to-end gateway repro above was not run locally.
  • Breaking changes / migration notes: None. stream?: boolean is an additive optional field defaulting to falsy; all existing text-mode runSideQuery callers omit it and keep the non-streaming path, and JSON-mode side-queries are untouched.

Linked Issues

Closes #5861

中文说明

这个 PR 做了什么

为文本 side-query 增加一个可选的流式路径,使上下文压缩的摘要请求在模型仍在推理时也能保持 HTTP 连接存活。新增的 stream?: boolean 选项从 ChatCompressionService.compressrunSideQuery 一直传到 BaseLlmClient.generateText;开启后请求改走已存在的 generateContentStream,流式增量再被收集回与非流式路径相同的 { text, usage } 结果。只有压缩这一个调用方 opt-in,其余 side-query 保持原有的非流式行为不变。

为什么需要

上下文窗口接近上限时,压缩会发起一个摘要 side-query,而它当前是非流式的,因此整个 LLM 推理必须全部完成后第一个 HTTP 字节才会返回。在 BFF 网关(如 DataWorks ACP / DQ_AGENT,proxy_read_timeout 约 60s)后面,较长的压缩推理(生产 trace 实测约 1.9 分钟)会在 60s 被网关以 504 Gateway Timeout 杀掉,到 qwen-code 侧表现为 UnprocessableEntityError: 422。于是压缩失败、本轮 turn 报错、会话中断——尽管模型本身其实会成功。流式能解决:服务端会在数秒内返回 HTTP 200 和首个增量 chunk,每个 chunk 都会重置网关的读超时,慢推理因此不再触发 60s 上限。

Reviewer Test Plan

如何验证

单测:cd packages/core && npx vitest run src/core/baseLlmClient.test.ts src/utils/sideQuery.test.ts src/services/chatCompressionService.test.ts,预期全绿。新增用例断言:stream: truegenerateContentStream、拼接多 chunk 增量、trim、过滤 thought part、并从最后一个 chunk 取 usage;省略 stream 时仍走非流式 generateContentrunSideQuery 仅在设置时转发该 flag;压缩以 stream: true 调用 generateText

端到端(可选,需要网关):用 OpenAI 兼容端点 + 一个 proxy_read_timeout ≤60s 的代理跑一个长会话,强制触发压缩,确认压缩请求现在以流式完成,而此前非流式路径会以 504/422 失败。

证据(Before & After)

非用户可见改动(核心推理链路),无 UI before/after。本地单测:Test Files 3 passed (3) · Tests 132 passed (132)。仓库门禁通过:npm run typecheck(全 workspace)与 npm run lint 均 exit 0;pre-commit(prettier + eslint --max-warnings 0)在 6 个改动文件上通过。

测试平台

仅 macOS 本地验证;Windows / Linux 未本地测试,交由 CI 覆盖。

运行环境(可选)

仅单测(vitest)+ npm run typecheck / npm run lint,无需运行时/沙箱。

风险与范围

  • 主要风险 / 取舍:本 PR 的目标部署(DataWorks ACP / DQ_AGENT 是 OpenAI 兼容协议,由 OpenAIContentGenerator / QwenContentGenerator 承接)走的是 OpenAI pipeline,该 pipeline 已把流包进按 chunk 计的空闲看门狗(withStreamInactivityTimeout,可经 QWEN_STREAM_IDLE_TIMEOUT_MS / streamIdleTimeoutMs 调节,0 关闭)——所以「返回 200 后转入静默」的流在那里是有界的,而非旧的「全有或全无」非流式等待。原生 USE_GEMINI / USE_ANTHROPIC 流在 provider 层没有按 chunk 的空闲超时,但这正是主对话循环每一轮已经在消费的同一契约(geminiChat.ts 迭代的是同一个原始 generateContentStream),由 SDK 传输超时 + abortSignal 兜底;本 side-query 复用了这个既有不变量,并未引入新的暴露面。整段流在 retryWithBackoff 内消费,中途失败会重试整个幂等请求,与非流式的重试语义一致。若要为 Gemini/Anthropic 加一个 provider 无关的空闲超时,应放在 ContentGenerator 层(这样主循环也受益),不在本 PR 范围内。
  • 未验证 / 范围外:issue 中可选的次要改进刻意不在本 PR 捆绑,以聚焦 504 根因。「为压缩禁用 thinking」当前代码其实已满足——压缩调用已传 thinkingConfig: { includeThoughts: false },且 applyThinkingDefault 把每个 side-query 默认设为该值(issue 里的 thoughts_token_count=1 来自较旧的 preview 构建)。「为压缩使用 fast model」是与超时无关的摘要质量 / 选模行为变更,留作可能的 follow-up(fastModel-reasoning 的更大话题见关联的 Disable reasoning on all fastModel side queries (follow-up to #3759) #3760)。上面的端到端网关复现未在本地执行。
  • 破坏性变更 / 迁移说明:无。stream?: boolean 是默认 falsy 的新增可选字段;所有现有文本模式 runSideQuery 调用方都不传它、保持非流式路径,JSON 模式 side-query 不受影响。

关联 Issue

Closes #5861

🤖 Generated with Qwen Code

The context-compression side-query runs non-streaming, so the whole LLM
inference must finish before the first HTTP byte arrives. Behind a BFF
gateway whose `proxy_read_timeout` is ~60s, a long compression inference
(observed ~1.9 min) is killed at 60s with a 504 — surfaced to qwen-code
as a 422 — which fails compression and breaks the session.

Add an opt-in `stream?: boolean` to the text side-query path
(`BaseLlmClient.generateText` -> `runSideQuery`) and have only
`ChatCompressionService.compress` opt in. Streaming keeps the connection
alive (the first delta arrives within seconds, resetting the gateway
timeout), so a slow inference no longer trips the gateway. Streamed
deltas are collected into the same `{ text, usage }` result, so every
other side-query caller keeps the unchanged non-streaming path.

Refs QwenLM#5861

🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)
Copilot AI review requested due to automatic review settings June 25, 2026 11:22

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@qwen-code-ci-bot

qwen-code-ci-bot commented Jun 25, 2026

Copy link
Copy Markdown
Collaborator

Thanks for the PR!

Template looks good ✓ — all required sections present with substantive content.

On direction: this solves a real production problem — 504 gateway timeouts killing compression inference behind BFF proxies (DataWorks ACP / DQ_AGENT). Sessions break mid-conversation despite the model itself succeeding. Directly aligned with reliability. Closes #5861, which has clear user impact.

On approach: scope is tight and well-bounded. Only the compression caller opts in to streaming; every other side-query keeps the unchanged non-streaming path. Reuses the existing generateContentStream pipeline that the main chat loop already exercises — no new transport layer. The stream?: boolean flag threads cleanly through three layers (chatCompressionServicerunSideQuerygenerateText) with no scope creep. +327/-11 is dominated by tests, which is the right ratio.

Moving on to code review. 🔍

中文说明

感谢贡献!

模板完整 ✓

方向:解决真实的生产问题——BFF 网关(DataWorks ACP / DQ_AGENT)的 504 超时导致压缩推理中断会话。与可靠性目标直接对齐,关联 #5861

方案:范围紧凑,只有压缩调用方 opt-in 流式;其余 side-query 保持非流式。复用已有的 generateContentStream 管线,无新增传输层。stream?: boolean 干净地穿过三层,无范围蔓延。+327/-11 以测试为主,比例合理。

进入代码审查 🔍

Qwen Code · qwen3.7-max

@qwen-code-ci-bot

qwen-code-ci-bot commented Jun 25, 2026

Copy link
Copy Markdown
Collaborator

Code review: no blockers. The streaming branch in baseLlmClient.generateText is clean — both paths (stream / non-stream) are wrapped by the same retryWithBackoff, so a mid-stream failure retries the whole idempotent call, matching existing retry semantics. getResponseText (which already drops thought parts) is reused per-chunk. Trim happens once at the end. The sideQuery.ts conditional spread (...(options.stream !== undefined && { stream: options.stream })) correctly keeps the key absent when not opted in. chatCompressionService.ts opts in with stream: true and a clear comment linking back to #5861.

One minor note (non-blocking): the error catch block logs 'Error generating text content via API.' with no indication whether it was streaming or not. A small differentiator in the log message could help debugging in production, but not worth blocking on.

Tests are thorough: 7 new streaming tests covering delta concatenation, thought-part filtering, usage capture (separate chunk and same-chunk), mid-stream error, mid-stream abort, empty stream, and per-model generator streaming. Plus 2 runSideQuery tests and 1 chatCompressionService assertion. CI green on all 3 platforms (ubuntu verified; macOS/Windows skipped this run but ubuntu is the gate).

Tmux smoke test: this is internal plumbing — the streaming path only activates during context compression behind a BFF gateway with proxy_read_timeout, which can't be reproduced in a local tmux session. Both installed and dev builds respond correctly to a simple query:

Installed build

$ qwen -p 'say hello in one word' 2>&1 | tee tmp/triage-test-064802/installed.log
Hello!

Dev build (main branch, without PR)

$ npm run dev -- -p 'say hello in one word' 2>&1 | tee tmp/triage-dev-065208/dev.log
> @qwen-code/qwen-code@0.19.2 dev
> node scripts/dev.js -p say hello in one word

DEV is set to true, but the React DevTools server is not running. Start it with:
$ npx react-devtools

Hello

No regression. The streaming behavior requires a gateway repro that the PR's E2E section explicitly marks as optional. Unit tests (127 on main, 132 on PR per CI) and CI gates are the appropriate verification here.

中文说明

**代码审查:**无阻塞问题。baseLlmClient.generateText 的流式分支干净——两条路径都由同一个 retryWithBackoff 包裹,中途失败会重试整个幂等调用,与现有重试语义一致。每个 chunk 复用已有的 getResponseText(已过滤 thought parts)。trim 在最终执行一次。sideQuery.ts 的条件展开正确。chatCompressionService.tsstream: true opt-in 并附有清晰注释。

一个小建议(非阻塞):错误 catch 块的日志 'Error generating text content via API.' 没有区分流式/非流式,生产排查时可能不太方便。

测试覆盖充分:7 个新流式测试 + 2 个 runSideQuery 测试 + 1 个 chatCompressionService 断言。CI 全绿。

**Tmux 冒烟测试:**这是内部链路改动,流式路径只在 BFF 网关后触发,本地 tmux 无法复现。已验证安装版和开发版对简单查询响应正常。

Qwen Code · qwen3.7-max

@doudouOUC doudouOUC self-assigned this Jun 25, 2026
@qwen-code-ci-bot

qwen-code-ci-bot commented Jun 25, 2026

Copy link
Copy Markdown
Collaborator

This PR does exactly what it says: adds a minimal streaming opt-in to keep the HTTP connection alive during slow compression inference behind BFF gateways. The implementation matches my independent proposal — extend GenerateTextOptions with stream?: boolean, route through the already-existing generateContentStream, collect deltas into the same { text, usage } shape, and wrap in the same retryWithBackoff. No new abstractions, no scope creep, no drive-by refactors.

The code is straightforward — one conditional in generateText, one line in chatCompressionService, one conditional spread in sideQuery. Everything else is tests (which are thorough and cover the edge cases that matter: mid-stream failure, abort, empty stream, thought-part filtering, per-model generator). The risk analysis in the PR body is honest and well-reasoned — the OpenAI pipeline already has withStreamInactivityTimeout, and Gemini/Anthropic streams reuse the same contract the main chat loop already depends on.

CI is green (ubuntu tests, typecheck, lint). The existing CHANGES_REQUESTED reviews appear to be from earlier triage runs that identified test gaps — those gaps have since been filled (mid-stream error, abort, per-model streaming tests are all present now).

Approving. ✅

中文说明

这个 PR 完全兑现了承诺:以最小改动为压缩推理增加流式 opt-in,在 BFF 网关后保持 HTTP 连接存活。实现与我的独立方案一致——扩展 GenerateTextOptions,复用已有的 generateContentStream,收集到相同的 { text, usage } 结构,包裹在同一个 retryWithBackoff 中。无新抽象,无范围蔓延。

代码简洁直接,测试充分覆盖了重要的边界情况。CI 全绿。此前的 CHANGES_REQUESTED 来自早期 triage 指出的测试缺口——这些缺口现已填补。

批准。✅

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

Comment thread packages/core/src/core/baseLlmClient.ts
Keep the full gateway-timeout rationale canonical on
`GenerateTextOptions.stream` and slim `SideQueryTextOptions.stream` to a
brief opt-in note that links to it, instead of repeating the same ~7-line
block in both. Also harmonize "Defaults to falsy" -> "Defaults to `false`"
for the typed boolean. Addresses review feedback on QwenLM#5865.

🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)
chiga0
chiga0 previously approved these changes Jun 25, 2026

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

Overview

Final Verdict: Approve — Clean, focused bug fix that solves a real production issue (504 gateway timeout killing slow compression inference). The streaming opt-in is minimally invasive — only the compression caller opts in, all other side-queries keep the unchanged non-streaming path. Both branches resolve to the same { text, usage } shape under a single retryWithBackoff, preserving retry semantics.

Findings Summary

  • Critical/Major: 0 items
  • Minor: 0 items
  • Nit: 0 items

Key Observations

The design is correct: streaming keeps the HTTP connection alive by returning the first delta within seconds, which resets the gateway's proxy_read_timeout on every chunk. The entire stream is consumed inside retryWithBackoff, so a mid-stream failure retries the whole idempotent request — matching non-streaming retry semantics.

Cross-Validation

Finding Other Reviewer My Assessment
JSDoc duplication on stream across layers qwen-code-ci-bot Fixed at HEAD (commit 7de4967b) — canonical rationale on GenerateTextOptions.stream, slim {@link} note on SideQueryTextOptions.stream

Additional Audit Coverage

  • Trim behavior change: Previously getResponseText(result).trim() on raw response; now result.text.trim() after stream concatenation. Correct — trim is applied once on the full concatenated text, not per-delta. Test confirms: ' Hello' + ', ' + 'world ''Hello, world'.
  • Usage "last one wins": if (chunk.usageMetadata) { usage = chunk.usageMetadata } — overwrites on each chunk with usage, so final chunk's usage is captured. Matches streaming pipeline convention where usage rides the trailing chunk.
  • Abort signal propagation: for await on the response stream respects abort — if abortSignal fires mid-stream, the async iterator throws, which propagates to retryWithBackoff's catch. The existing if (abortSignal.aborted) check in the outer catch handles it correctly.
  • Stream inactivity timeout: The existing withStreamInactivityTimeout wraps generateContentStream internally, providing a per-chunk 120s watchdog. This is strictly better than the non-streaming all-or-nothing wait.
  • Conditional spread in sideQuery.ts: ...(options.stream !== undefined && { stream: options.stream }) correctly omits the field when not set. Test confirms: expect(callArg).not.toHaveProperty('stream').
  • maxAttempts: 1 for compression: Compression is best-effort (next turn re-triggers), so the existing single-attempt limit is preserved. Streaming doesn't change this — a stream failure just means compression returns NOOP faster.
  • Backward compatibility: stream defaults to falsy. All existing runSideQuery callers omit it. JSON-mode side-queries are untouched (the stream option is only on SideQueryTextOptions).

This review was generated by QoderWork AI

@doudouOUC doudouOUC left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

No review findings. Downgraded from Approve to Comment: self-PR; CI still running.

— qwen3.7-max via Qwen Code /review

@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 review findings. LGTM. Downgraded from Approve to Comment: CI still running (30 checks pending).

The change is clean and well-scoped: the stream?: boolean opt-in threads correctly through ChatCompressionServicerunSideQueryBaseLlmClient.generateText, the streaming closure routes through generateContentStream and accumulates deltas into the same { text, usage } shape, and retryWithBackoff wraps the whole call so mid-stream failures retry cleanly. Abort propagation through the SDK transport is intact (no need for an explicit in-loop check — matches the established pattern in geminiChat.ts). Tests cover the new paths thoroughly (multi-chunk concatenation + trim, thought-part filtering, usage capture, forwarding + backward compat).

— qwen3.7-max via Qwen Code /review

// the final chunk (last one wins), matching the non-streaming read.
let text = '';
let usage: GenerateContentResponseUsageMetadata | undefined;
for await (const chunk of responseStream) {

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.

[Critical] The for await loop has no inter-chunk idle timeout. The PR description claims "Streaming arms a per-chunk inactivity watchdog (withStreamInactivityTimeout, default 120s between chunks)" — but that wrapper only exists for the OpenAI provider (openaiContentGenerator/pipeline.ts). The Gemini (geminiContentGenerator.ts) and Anthropic (anthropicContentGenerator.ts) content generators return raw streams with no inactivity timeout.

For the chat compression use case (the PR's sole motivation, which runs on Gemini via the BFF gateway), a stalled model that stops emitting chunks but keeps the TCP connection open would cause this loop to block indefinitely — worse than the pre-PR behavior where the gateway's 60s timeout at least produced a catchable error that triggered the NOOP fallback.

Consider wrapping responseStream with withStreamInactivityTimeout (or an equivalent) at this level so all providers benefit uniformly, regardless of which content generator is resolved by resolveForModel.

— qwen3.7-max via Qwen Code /review

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Thanks — I checked this carefully. Two parts of the observation are correct, but the severity premise isn't, so I've corrected the PR description rather than adding the wrapper.

Correct: withStreamInactivityTimeout lives only in the OpenAI pipeline (openaiContentGenerator/pipeline.ts:120, applied at :265); GeminiContentGenerator.generateContentStream returns the raw googleGenAI stream and AnthropicContentGenerator returns a raw Anthropic stream — neither has a per-chunk idle timeout.

But the motivating path is not Gemini. The DataWorks ACP / DQ_AGENT BFF is OpenAI-compatible — issue #5861's own trace shows openai.chat.completions.create. That resolves to OpenAIContentGenerator (or QwenContentGenerator, whose generateContentStream override just wraps super.generateContentStream in credential management — qwenContentGenerator.ts:167), so compression behind the BFF does route through the pipeline and is covered by the watchdog. The "blocks indefinitely / worse than pre-PR" scenario doesn't apply where this PR actually runs, and the 60s gateway timeout you cite only exists in that BFF deployment.

For native USE_GEMINI / USE_ANTHROPIC: the raw-stream point is fair, but that's the exact contract the main chat loop already consumes on every turn (geminiChat.ts iterates the same generateContentStream, e.g. :2013, :2994), bounded by the SDK transport timeout + abortSignal. This side-query reuses that existing invariant; it isn't a new exposure introduced here.

Changed: corrected the PR description's risk note, which overclaimed a universal 120s watchdog — it's now scoped to the OpenAI/Qwen pipeline, with the Gemini/Anthropic contract spelled out.

Not changed: I didn't wrap responseStream at the baseLlmClient level. A provider-agnostic idle timeout belongs at the ContentGenerator layer so the main loop benefits too; adding it only to this side-query would double-wrap OpenAI/Qwen and give the side-query a guarantee the main turn doesn't have. Happy to do that as a separate, broader change if maintainers want it — leaving this thread open for a maintainer to weigh in.

🤖 Addressed by Qwen Code

Comment thread packages/core/src/core/baseLlmClient.test.ts
wenshao
wenshao previously approved these changes Jun 25, 2026

@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 new findings. The streaming error-path test gap (mid-stream failure, abort during iteration) was already flagged in a prior review. Production code is clean and correct — the stream?: boolean opt-in threads properly through the three layers, retryWithBackoff wraps both branches uniformly, and all 132 tests pass. LGTM. ✅

— qwen3.7-max via Qwen Code /review

Add two regression tests for the streaming branch of generateText: a
mid-stream throw rejects the whole call (partial text is never returned
as success) and is reported, and an abort firing mid-stream surfaces the
original error unwrapped while skipping reportError via the
`abortSignal.aborted` guard. The first is the gateway-timeout-mid-
inference scenario this PR targets. Addresses review feedback on QwenLM#5865.

🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)
@doudouOUC
doudouOUC dismissed stale reviews from wenshao and chiga0 via 91e364a June 25, 2026 15:05

@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 high-confidence findings. Downgraded from Approve to Comment: CI still running.

Needs Human Review (low-confidence, not blockers):

  • baseLlmClient.ts:365-382 — No partial text length or chunk count in error context when streaming fails mid-stream. The accumulated partial text is silently discarded.
  • baseLlmClient.test.ts — No test for retry-then-success path for streaming. The comment claims retries work but this path is untested.
  • baseLlmClient.test.ts — No test for empty stream (zero chunks). Handled correctly by construction but untested boundary case.

— qwen3.7-max via Qwen Code /review

Add a boundary test for a stream that yields zero chunks: generateText
must resolve to an empty string with undefined usage rather than throw.
Addresses review feedback on QwenLM#5865.

🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)
@doudouOUC

Copy link
Copy Markdown
Collaborator Author

Thanks for the low-confidence notes. Triaged the three:

  • Empty stream (zero chunks) — added in d7a4ca1: a stream that closes immediately resolves to '' / undefined usage rather than throwing.
  • Partial text length / chunk count in the error context — not adding. The accumulator is local to the streaming apiCall closure; surfacing it would mean hoisting mutable partial state out across the retryWithBackoff boundary into the shared catch block, and partial text from a failed attempt that's about to be retried is misleading to log. The cost/complexity outweighs the marginal telemetry value, so I'd rather keep the two-branch structure clean.
  • Retry-then-success test for streaming — not adding. Retry is retryWithBackoff's contract, which this suite deliberately mocks to a single pass (it's covered in retry.test.ts); a test here would exercise the mock, not real retry. The "no cross-attempt accumulation" property it implies is guaranteed by construction — let text = '' is re-initialized inside the apiCall closure on every invocation.

🤖 Addressed by Qwen Code

Comment thread packages/core/src/core/baseLlmClient.test.ts
Comment thread packages/core/src/core/baseLlmClient.test.ts
Two streaming-branch gaps surfaced in review:

- The per-model `fastContentGenerator` mock had no `generateContentStream`,
  so no test exercised streaming against a `resolveForModel`-selected
  generator — the exact `model` + `stream: true` combination compression
  uses. Add that mock method and a per-model streaming test asserting the
  resolved generator's stream is consumed (not the injected default).
- `mockTextStream` only emitted usage on a separate trailing chunk. Add a
  test where the final content-bearing chunk carries both a text delta and
  usageMetadata (the realistic Gemini/OpenAI shape), guarding against a
  refactor that skips text extraction on usage-bearing chunks.

Addresses review feedback on QwenLM#5865.

🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)
Comment thread packages/core/src/core/baseLlmClient.test.ts

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

[Suggestion] baseLlmClient.ts:430 — The error catch block is identical for streaming and non-streaming paths. reportError logs 'Error generating text content via API.' with no indication that streaming was active, no partial text length, and no chunk count. The stream flag is destructured at line 335 and in scope here but unused. An oncall engineer debugging a stalled compression at 3 AM cannot distinguish a streaming mid-stream failure from the original non-streaming 504 this PR fixes.

Consider adding streaming context to the error message:

const streamContext = stream ? ` [streaming]` : '';
await reportError(
  error,
  `Error generating text content via API${streamContext}.`,
  contents,
  'generateText-api',
);

— qwen3.7-max via Qwen Code /review

@wenshao

wenshao commented Jun 25, 2026

Copy link
Copy Markdown
Collaborator

✅ Verification report — stream chat-compression side-query

Verdict: works as designed. I reproduced the #5861 gateway timeout on main, and this branch fixes it. Unit tests have teeth, no regressions seen. Recommend merge.

I built a real end-to-end reproduction of the BFF gateway timeout and ran a BASE-vs-PR A/B against it (real qwen TUI over tmux, deterministic mock provider).

1. Unit tests (PR head) — all green

src/utils/sideQuery.test.ts ................. 23 passed
src/core/baseLlmClient.test.ts .............. 43 passed
src/services/chatCompressionService.test.ts . 71 passed
Test Files  3 passed (3) · Tests  137 passed (137)

Reverse-audit — do the new tests actually pin the new behaviour? Reverting only the 3 source files to the merge-base while keeping the PR's test files makes 9 tests fail (1 × runSideQuery forwards stream:true, 7 × generateText - streaming, 1 × compression stream:true assertion). They are not vacuous.

2. End-to-end — gateway-timeout reproduction (the core of #5861)

Harness. A zero-dep OpenAI-compatible mock that also models a BFF gateway: proxy_read_timeout = 3s, and a compression inference that takes 9s (3× the gateway timeout).

  • stream:false → no bytes during inference → gateway returns 504 at 3s.
  • stream:true → HTTP 200 immediately + an SSE chunk every 600 ms → read timeout never trips → full summary delivered.

A/B. Dist-level swap of only the 3 changed core files (baseLlmClient.js, sideQuery.js, chatCompressionService.js) between the merge-base (BASE) and PR-head (PR) compilations — everything else identical, same esbuild toolchain, so the only variable is this PR's diff. Driver: real TUI → one turn (hiOK., so history ≥ 2) → /compress.

Result.

BASE (main) PR #5865
compression request on the wire no stream:true (non-streaming) stream:true + stream_options:{include_usage:true}
requests sent 4 (SDK retries the 504s) 1
gateway behaviour 504 at 3s × 4 200, streamed for 9.06 s
/compress outcome ✕ Failed to compress chat history … Request timeout after 15s ✦ Chat history compressed from 1200 to 1060 tokens

The PR flips the compression side-query to stream:true exactly as intended; a slow inference that the gateway kills when non-streaming now completes when streamed — the summary streams back, is accepted, and the context is compressed (1200 → 1060, usage captured from the final stream chunk).

Raw evidence (mock request log + TUI)
// PR arm — one streamed request, survives the 9s inference
{"requestIndex":2,"stream":true,"hasStreamKey":true,"stream_options":{"include_usage":true},"isCompression":true,"nMessages":4}
STREAM_START_200  →  STREAM_DONE_200 elapsedMs=9059
// TUI:  ✦ Chat history compressed from 1200 to 1060 tokens.

// BASE arm — non-streaming, killed 4× by the gateway
{"requestIndex":2,"stream":"(absent)","hasStreamKey":false,"stream_options":null,"isCompression":true,"nMessages":4}
{"requestIndex":3,"stream":"(absent)","hasStreamKey":false,...}
{"requestIndex":4,"stream":"(absent)","hasStreamKey":false,...}
{"requestIndex":5,"stream":"(absent)","hasStreamKey":false,...}
GATEWAY_TIMEOUT_504 × 4
// TUI:  ✕ Failed to compress chat history: ... Request timeout after 15s.

Notes (non-blocking)

  • The timeline is scaled for test speed (3s gateway / 9s inference vs the production 60s / 1.9min); the dynamic is identical.
  • On the wire BASE sends no stream field — the OpenAI SDK drops the explicit stream:false as a default — but the server still runs it non-streaming, so it behaves as stream:false.
  • Bonus: streaming also removes the 4 wasted non-streaming retry attempts BASE makes before failing.
  • Scope is correctly narrow — only the compression caller opts in; the normal chat turn and other text side-queries were unchanged in the run. Verified against the OpenAI/Qwen pipeline (the motivating DataWorks ACP / DQ_AGENT deployment); the native Gemini/Anthropic idle-timeout caveat called out in the PR description was not separately exercised and is reasonably out of scope.
中文版(点击展开)

✅ 验证报告 —— 压缩 side-query 流式化

结论:行为符合预期。我在 main 上复现了 #5861 的网关超时,本分支修复了它。新增单测有效、未见回归。建议合并。

我用真实 qwen TUI(tmux 驱动)+ 确定性 mock provider,搭了一个 BFF 网关超时的端到端复现,并做了 BASE 与 PR 的 A/B 对比。

1. 单元测试(PR head)—— 全绿

sideQuery.test.ts 23 passed · baseLlmClient.test.ts 43 passed · chatCompressionService.test.ts 71 passed
Test Files 3 passed (3) · Tests 137 passed (137)

逆向审计(这些新测试是否真的钉住了新行为?):把 3 个源文件回退到 merge-base、保留 PR 的测试文件后,有 9 个测试失败(1 个 runSideQuery 转发 stream:true、7 个 generateText - streaming、1 个压缩 stream:true 断言)。说明测试非空转、确实有效。

2. 端到端 —— 网关超时复现(#5861 的核心)

测试装置: 一个零依赖的 OpenAI 兼容 mock,同时模拟 BFF 网关:proxy_read_timeout = 3s,压缩推理耗时 9s(网关超时的 3 倍)。

  • stream:false → 推理期间无字节 → 网关在 3s 返回 504
  • stream:true → 立即 200 + 每 600ms 一个 SSE chunk → 读超时永不触发 → 完整摘要返回。

A/B: 仅在 dist 层替换 3 个改动文件(baseLlmClient.js / sideQuery.js / chatCompressionService.js)的 merge-base(BASE)与 PR-head(PR)编译产物,其余完全一致、同一 esbuild 工具链,唯一变量就是本 PR 的 diff。驱动:真实 TUI → 先发一轮(hiOK.,使历史 ≥ 2)→ /compress

结果:

BASE(main PR #5865
线上压缩请求 stream:true(非流式) stream:true + stream_options:{include_usage:true}
发出的请求数 4(SDK 对 504 重试) 1
网关表现 3s × 4 次 504 200,流式持续 9.06s
/compress 结果 ✕ 压缩失败 … Request timeout after 15s ✦ 上下文从 1200 压缩到 1060 tokens

PR 正如设计把压缩 side-query 切到 stream:true;非流式下会被网关杀掉的慢推理,在流式下顺利完成——摘要逐 token 流回、被接受、上下文成功压缩(1200 → 1060,usage 从最后一个流 chunk 捕获)。

备注(不阻塞)

  • 时间线为加速测试做了缩放(3s 网关 / 9s 推理 vs 生产 60s / 1.9 分钟),动力学一致。
  • 线上 BASE 实际未发 stream 字段——OpenAI SDK 把默认值 stream:false 省略了——但服务端仍按非流式处理,效果等同 stream:false
  • 附带收益:流式还省掉了 BASE 失败前那 4 次无谓的非流式重试。
  • 改动范围恰当收敛——只有压缩调用方 opt-in,本轮普通对话与其它文本 side-query 未受影响。已针对 OpenAI/Qwen pipeline(即目标部署 DataWorks ACP / DQ_AGENT)验证;PR 描述里提到的原生 Gemini/Anthropic 无按-chunk 空闲超时这一点未单独验证,合理地属于范围外。

wenshao
wenshao previously approved these changes Jun 25, 2026
@wenshao

wenshao commented Jun 25, 2026

Copy link
Copy Markdown
Collaborator

@qwen-code /triage

Surface a `[streaming]` marker in the generateText error report when the
streaming branch was active, so an oncall can tell a mid-stream failure
apart from the original non-streaming gateway timeout this PR fixes. The
already-destructured `stream` flag drives it; partial-text length / chunk
count are intentionally left out (they would require hoisting mutable
per-attempt state across the retryWithBackoff boundary).

Also assert the request shape (model, contents, abortSignal-bearing
config, promptId) in the main and per-model streaming tests, matching the
non-streaming tests so a request-construction regression is caught.

Addresses review feedback on QwenLM#5865.

🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)
@doudouOUC

Copy link
Copy Markdown
Collaborator Author

@wenshao thanks for the thorough end-to-end verification — the dist-level swap isolating only the 3 changed files, and the BASE (504 ×4) vs PR (streamed 9.06s, 1200→1060) contrast, is exactly the gateway-timeout evidence this PR needed. The reverse-audit (reverting only the source → 9 tests fail) is a nice touch too.

On your baseLlmClient.ts:430 note — implemented in 4ef0402: the generateText error report now appends [streaming] when the streaming branch was active, so an oncall can tell a mid-stream failure apart from the original non-streaming 504 in telemetry. I deliberately left partial-text-length / chunk-count out: those live in the streaming apiCall closure, and surfacing them in the shared catch would mean hoisting mutable per-attempt state across the retryWithBackoff boundary — where partial text from a soon-to-be-retried attempt is misleading to log. The [streaming] marker gives the streaming-vs-not signal without that complexity.

🤖 Addressed by Qwen Code

@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 previously approved these changes Jun 25, 2026
@doudouOUC
doudouOUC added this pull request to the merge queue Jun 25, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Jun 25, 2026
Comment thread packages/core/src/core/baseLlmClient.test.ts Outdated
@wenshao
wenshao added this pull request to the merge queue Jun 25, 2026
The mid-stream-error test verified reportError was called but not with
what message. Assert the `[streaming]` marker so a regression dropping it
(the signal that distinguishes a mid-stream failure from the original 504)
is caught. Addresses review feedback on QwenLM#5865.

🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)
@doudouOUC
doudouOUC dismissed stale reviews from wenshao and qwen-code-ci-bot via eccb533 June 25, 2026 23:31
Merged via the queue into QwenLM:main with commit 775a203 Jun 25, 2026
5 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.

Context compression request should use stream=true to avoid gateway timeout

5 participants