Skip to content

fix(core): allow transport stream retry during the thinking-only phase - #7938

Merged
wenshao merged 1 commit into
QwenLM:mainfrom
ComplexSimply:fix/thinking-phase-stream-retry-7832
Jul 30, 2026
Merged

fix(core): allow transport stream retry during the thinking-only phase#7938
wenshao merged 1 commit into
QwenLM:mainfrom
ComplexSimply:fix/thinking-phase-stream-retry-7832

Conversation

@ComplexSimply

@ComplexSimply ComplexSimply commented Jul 28, 2026

Copy link
Copy Markdown
Collaborator

What

Implements the thinking-phase retry refinement scoped by @doudouOUC in #7832 (comment) (the welcome-pr direction): track a separate streamYieldedContentChunk that is only set when a chunk carries a non-thought candidate part (text, functionCall, inlineData, …), and gate the transport stream replay on it instead of streamYieldedChunk.

  • New helper hasNonThoughtCandidateParts() next to isToolCallPreparationOnly() documents the invariant the gate relies on.
  • The not-taken log's retryDecision becomes skipped_after_content (was skipped_after_chunk; single producer, no other consumers — verified by grep), and the retry-scheduled log gains yieldedNonContentChunks so thinking-phase replays are visible in diagnostics.
  • Content-phase behavior is unchanged: once any non-thought part has flowed, the replay stays blocked and the error propagates.

Why

streamYieldedChunk trips on the first thinking chunk — within seconds for thinking models — while the model can then spend minutes reasoning. That is exactly when gateways close long-lived SSE connections (UND_ERR_SOCKET, reporter's 5/5 repro; same server-side cause as #7831). The retry was already classified as safe (UND_ERR_SOCKETRETRYABLE_STREAM_TRANSPORT_CODES) but locked out even though zero user-visible content existed.

Replaying after thinking-only output is safe because thought parts are ephemeral — they are never recorded as the assistant's final response in history (stripThoughtPartsFromContent and the model-parts filter drop part.thought before history is recorded), so a replay cannot duplicate visible output. The UI does re-render the new attempt's thinking, which is the same behavior as the existing pre-chunk retry path.

Reviewer Test Plan

  • vitest run src/core/geminiChat.test.ts → 262 passing. Full src/core/ slice: 53 files / 2492 passing; src/utils/ slice: 117 files / 4117 passing. tsc --noEmit, eslint --max-warnings 0, prettier --check clean.
  • Existing test renamed for precision: does not retry … after yielding a content chunk (fixture already yields visible text).
  • New: retries a transport stream error after yielding only thinking chunks — first attempt yields a { thought: true } part then throws UND_ERR_SOCKET; asserts 2 API calls, exactly 1 RETRY event, and the recovered content arrives.
  • New: does not retry when visible content followed the thinking chunks — pins that the flag accumulates across the attempt; thinking-then-content still blocks (1 API call, 0 RETRY).
  • Mutation check: reverting the gate to !streamYieldedChunk fails exactly the thinking-retry test (1/262); neutering the content check fails exactly the two blocking tests (2/262).

Not reproduced against a live DashScope gateway: the failure needs a server-side idle close minutes into a real SSE stream. The deterministic unit tests above pin the decision logic itself; the reporter's repro matrix covers the environment side.

Risk & Scope

One flag + one gate condition + log fields in sendMessageStream's retry loop; no API or event-shape changes. The riskiest edge — a chunk with no parts (usage-only / finishReason-only) — now allows replay where it previously blocked; both are pre-content states where replay is safe for the same history-based reason.

Linked Issues

Fixes #7832. Related: #7831 (same gateway idle-close, higher-context variant). The issue's suggestion (2) — partial-output continuation for content-phase failures — is explicitly out of scope per the triage discussion; it needs API-level continuation semantics and a design doc.

The transport stream retry was gated on !streamYieldedChunk, which any
thinking/reasoning chunk trips within seconds for thinking models. A
gateway closing the long-lived SSE connection minutes into the thinking
phase (UND_ERR_SOCKET) then propagated as a fatal error even though
zero user-visible content had been produced — making large generations
in YOLO/headless mode effectively impossible (QwenLM#7832).

Track a separate streamYieldedContentChunk that is only set when a
chunk carries a non-thought candidate part (text, functionCall, …) and
gate the replay on it instead. Thought parts are ephemeral — never
recorded as the assistant's response in history — so replaying after
thinking-only output cannot duplicate visible content. Content-phase
failures keep the existing behavior: once a non-thought part has
flowed, the retry stays blocked and the error propagates.

The not-taken log now reports skipped_after_content, and the retry log
gains yieldedNonContentChunks so thinking-phase replays are visible in
diagnostics.

Fixes QwenLM#7832
@ComplexSimply

Copy link
Copy Markdown
Collaborator Author

@qwen-code /takeover

1 similar comment
@ComplexSimply

Copy link
Copy Markdown
Collaborator Author

@qwen-code /takeover

@qwen-code-ci-bot

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

Copy link
Copy Markdown
Collaborator

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

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

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Thanks for the PR!

Template looks good ✓ — all substantive sections are present and thorough.

Problem: Observed bug with strong evidence. Issue #7832 is a P1 with a 5/5 reproduction matrix — YOLO mode large code generation consistently fails with UND_ERR_SOCKET when the DashScope gateway closes idle SSE connections during the thinking phase. Maintainer @doudouOUC confirmed the root cause and scoped this as a welcome-pr. This is not theoretical.

Direction: Aligned. Transport stream retry is core reliability infrastructure, and thinking models are the primary use case for qwen-code. The retry was already classified as safe (UND_ERR_SOCKETRETRYABLE_STREAM_TRANSPORT_CODES) but locked out by an overly conservative gate. CHANGELOG: no direct reference to this specific fix, but stream retry reliability is clearly within scope.

Size: Core paths touched (packages/core/src/core/geminiChat.ts). Production logic: 47 lines (39 additions + 8 deletions). Test: 128 lines (127 additions + 1 deletion). Well under any threshold.

Approach: The scope is tight — one new flag, one helper, one gate condition change, plus log field updates. No unrelated changes, no drive-by refactors. The PR explicitly defers the issue's suggestion (2) (partial-output continuation) as out of scope, which is the right call. I don't see a simpler path: distinguishing thought-only output from visible content is exactly the right granularity for the retry gate.

Risk: ⚠️ geminiChat.ts matches the high-risk path list (10 of 31 historically reverted PRs touched these paths). This doesn't block the PR, but it means full CI evidence is required before approval, and a maintainer should pay extra attention to the retry semantics.

Moving on to code review. 🔍

中文说明

感谢贡献!

模板完整 ✓ —— 所有实质性章节齐全且详尽。

问题: 已观测到的 bug,证据充分。Issue #7832 是 P1 级别,有 5/5 的复现矩阵 —— YOLO 模式下大型代码生成在思考阶段因 DashScope 网关关闭空闲 SSE 连接而持续失败(UND_ERR_SOCKET)。维护者 @doudouOUC 已确认根因并将此标记为 welcome-pr。这不是理论性问题。

方向: 对齐。传输流重试是核心可靠性基础设施,思考模型是 qwen-code 的主要使用场景。重试已被分类为安全(UND_ERR_SOCKETRETRYABLE_STREAM_TRANSPORT_CODES),但被过于保守的门控阻止。

规模: 触及核心路径(packages/core/src/core/geminiChat.ts)。生产逻辑:47 行(39 增 + 8 删)。测试:128 行(127 增 + 1 删)。远低于任何阈值。

方案: 范围紧凑 —— 一个新标志、一个辅助函数、一个门控条件变更,加上日志字段更新。无无关改动,无顺手重构。PR 明确将 issue 的建议 (2)(部分输出续传)标记为超出范围,这是正确的决定。

风险: ⚠️ geminiChat.ts 匹配高风险路径列表(历史上 31 个被回滚的 PR 中有 10 个触及这些路径)。这不会阻止 PR,但意味着批准前需要完整的 CI 证据,维护者应额外关注重试语义。

进入代码审查 🔍

Qwen Code · qwen3.8-max-preview

Reviewed at 90fc58c12ced01b1743890e6abcb6af28618642c · re-run with @qwen-code /triage

@qwen-code-ci-bot

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

Copy link
Copy Markdown
Collaborator

Code Review

Independent proposal: Given the problem (retry gate trips on thinking chunks), I would add a second boolean that only tracks non-thought candidate parts, swap the gate condition to use it, and add a small helper to keep the check readable. The thought/visible-content distinction already exists in the codebase (stripThoughtPartsFromContent, the model-parts filter), so the helper just formalizes what's already an invariant.

Comparison with the diff: The PR does exactly this, and does it cleanly.

  • hasNonThoughtCandidateParts() checks !part.thought across all candidate parts — consistent with the existing stripThoughtPartsFromContent filter (line 1144) and the content-recording logic (lines 4062–4067). The JSDoc documents the invariant the gate relies on, which is the right place for it.
  • The new flag is set outside the isToolCallPreparationOnly guard, which is correct: tool-prep-only chunks have no candidate parts, so hasNonThoughtCandidateParts returns false for them anyway. No behavioral difference, but the placement avoids a misleading nesting.
  • The gate change (!streamYieldedChunk!streamYieldedContentChunk) is the one-line fix. The old flag is preserved for streamYieldedAnyChunk and the new yieldedNonContentChunks diagnostic field, so no existing behavior is disturbed.
  • Edge case — usage-only / finishReason-only chunks (no parts): previously blocked retry via streamYieldedChunk, now allows it via streamYieldedContentChunk staying false. The PR calls this out explicitly and the reasoning holds: these are pre-content states where replay is safe for the same history-based reason.
  • Log renames (skipped_after_chunkskipped_after_content) are accurate and the PR verified no other consumers via grep.

No correctness bugs, no security concerns, no convention violations. The tests are well-structured: the thinking-retry test uses fake timers and asserts the full recovery path (2 API calls, 1 RETRY event, recovered content), and the thinking-then-content test pins the accumulation invariant (1 API call, 0 RETRY). The mutation testing described in the PR body (reverting the gate fails exactly 1/262; neutering the content check fails 2/262) is a good signal that the tests are load-bearing.

No findings. ✅

Testing

CI evidence (fetched via API for 90fc58c1):

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

Check Conclusion
Classify PR ✅ success
route ✅ success
Test (ubuntu-latest, Node 22.x) ✅ success
web-shell E2E Smoke (ubuntu-latest, Node 22.x) ✅ success

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

The ubuntu unit test suite is still running. macOS/Windows tests and integration tests were skipped (likely gated on the ubuntu result). The precheck-pr and authorize checks passed. Approval is deferred until CI lands green — see Stage 3.

Real-scenario testing: N/A — this is a CI run (pull_request_target). The change is in stream retry logic (not TUI surface); a maintainer can trigger the isolated @qwen-code /tmux job if behavioral verification is desired, but note the author lacks write access so the sandboxed lanes (/tmux, /verify) are unavailable for this PR. A maintainer could check the PR out in a disposable container to reproduce the specific retry claim, though the failure requires a real DashScope gateway idle-close (minutes into an SSE stream) which is hard to simulate deterministically — the unit tests pin the decision logic itself.

中文说明

代码审查

独立方案: 给定问题(重试门控在思考 chunk 上误触发),我会添加第二个布尔值仅跟踪非思考候选部分,将门控条件切换为使用它,并添加一个小辅助函数保持检查的可读性。思考/可见内容的区分在代码库中已经存在(stripThoughtPartsFromContent、模型部分过滤器),所以辅助函数只是将已有不变量形式化。

与 diff 的比较: PR 完全这样做了,而且做得很干净。

  • hasNonThoughtCandidateParts() 检查所有候选部分的 !part.thought —— 与现有的 stripThoughtPartsFromContent 过滤器(第 1144 行)和内容记录逻辑(第 4062–4067 行)一致。
  • 新标志设在 isToolCallPreparationOnly 保护之外,这是正确的:纯工具准备 chunk 没有候选部分,所以 hasNonThoughtCandidateParts 对它们返回 false。
  • 门控变更(!streamYieldedChunk!streamYieldedContentChunk)是一行修复。旧标志保留用于 streamYieldedAnyChunk 和新的 yieldedNonContentChunks 诊断字段。
  • 边界情况 —— 仅含 usage/finishReason 的 chunk(无部分):之前通过 streamYieldedChunk 阻止重试,现在允许。PR 明确指出了这一点,推理成立。
  • 日志重命名准确,PR 通过 grep 验证了无其他消费者。

无正确性 bug,无安全问题,无规范违反。测试结构良好,mutation testing 信号良好。

无发现 ✅

测试

CI 证据:ubuntu 单元测试仍在运行中。macOS/Windows 测试和集成测试被跳过。precheck-prauthorize 检查通过。批准推迟到 CI 全绿。

真实场景测试:不适用 —— 这是 CI 运行。作者无写入权限,沙箱通道不可用。

Qwen Code · qwen3.8-max-preview

Reviewed at 90fc58c12ced01b1743890e6abcb6af28618642c · re-run with @qwen-code /triage

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Confidence: 5/5 — Clean across every stage; would merge without hesitation once CI lands.

This is a textbook minimal fix for a real, well-evidenced problem. The PR does one thing — relaxes the transport retry gate from "no chunks yielded" to "no user-visible content yielded" — and does it with a single flag, a small helper that formalizes an existing invariant, and two well-targeted tests. The thought/visible-content distinction was already load-bearing in the history-recording path (stripThoughtPartsFromContent), so the retry gate was the outlier, not the fix.

The independent proposal I wrote before reading the diff was essentially identical to what the PR does. I didn't find a simpler path. Every line in the diff earns its place — no drive-by changes, no scope creep, and the explicit deferral of partial-output continuation (suggestion 2 from the issue) is the right scoping call.

The high-risk path flag on geminiChat.ts is warranted as a general signal, but this specific change is low-risk: it widens an existing retry window by one well-defined category (thinking-only output) without touching the retry mechanism itself, the error classification, or the content pipeline. The mutation testing (reverting the gate fails exactly 1/262 tests) confirms the new test is load-bearing.

Approval deferred until CI lands green on 90fc58c12ced01b1743890e6abcb6af28618642c — the ubuntu unit suite is still running.

中文说明

置信度:5/5 —— 每个阶段都很干净;CI 通过后会毫不犹豫地合并。

这是一个针对真实的、有充分证据的问题的教科书式最小修复。PR 只做一件事 —— 将传输重试门控从"未产生任何 chunk"放宽为"未产生用户可见内容" —— 用一个标志、一个将现有不变量形式化的小辅助函数,以及两个精准测试来完成。思考/可见内容的区分在历史记录路径中已经是承重的(stripThoughtPartsFromContent),所以重试门控才是异常值,而不是这个修复。

diff 中的每一行都有其存在价值 —— 无顺手改动,无范围蔓延。高风险路径标记是合理的一般性信号,但这个具体变更风险很低:它在一个明确定义的类别(仅思考输出)上扩展了现有的重试窗口,没有触及重试机制本身、错误分类或内容管道。

批准推迟到 CI 在 90fc58c12ced01b1743890e6abcb6af28618642c 上全绿。

Qwen Code · qwen3.8-max-preview

Reviewed at 90fc58c12ced01b1743890e6abcb6af28618642c · re-run with @qwen-code /triage

@qwen-code-ci-bot qwen-code-ci-bot added category/core Core engine and logic type/bug Something isn't working as expected labels Jul 28, 2026
@qwen-code-dev-bot qwen-code-dev-bot added the autofix/takeover Summon the autofix loop to manage this PR (remove to release; needs triage+) label Jul 28, 2026

@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 — CI landed green after the 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 issues found. LGTM! ✅

— qwen3.7-max via Qwen Code /review

@qwen-code-dev-bot

Copy link
Copy Markdown
Collaborator

🤝 Takeover engaged: the autofix loop now manages this PR — it will address new review feedback and resolve base conflicts until the label is removed or the round cap is reached. Remove the autofix/takeover label (or comment @qwen-code /takeover stop) to release.

中文说明

🤝 已接管:autofix 循环现在管理此 PR —— 将持续处理新的评审反馈与 base 冲突,直到移除标签或达到轮次上限。移除 autofix/takeover 标签(或评论 @qwen-code /takeover stop)即可释放。

@qwen-code-dev-bot

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

Copy link
Copy Markdown
Collaborator

AutoFix round 1 finishedview run. See this round's report below.

中文说明

AutoFix 第 1 轮已完成 —— 查看运行。本轮报告见下方。

@qwen-code-dev-bot

Copy link
Copy Markdown
Collaborator

🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下:

No action taken

This review round contained no actionable feedback:

  • Issue-level comment (@yiliang114): A bot error message ("⚠️ Failed to process this request. Please re-mention the bot to retry.") — not a code review finding.
  • Failed check (label: CANCELLED): A transient CI cancellation unrelated to the code change.
  • Reviews / Inline comments: None.

No code changes were made. The PR is ready for human review.

中文说明

未采取任何操作

本轮审查没有包含可操作的反馈:

  • Issue 级别评论@yiliang114):一条机器人错误消息("⚠️ Failed to process this request. Please re-mention the bot to retry.")——不是代码审查意见。
  • 失败的检查label: CANCELLED):与代码变更无关的 CI 临时取消。
  • 审查 / 行内评论:无。

未进行任何代码变更。该 PR 已准备好接受人工审查。

Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。


🧠 Handled by Qwen Code · model/模型 qwen3.8-max-preview

@wenshao

wenshao commented Jul 30, 2026

Copy link
Copy Markdown
Collaborator

Maintainer verification — built and tested locally

I re-verified this PR at head 90fc58c against merge-base c7de1d7 in two isolated worktrees (APFS-cloned node_modules, both confirmed contamination-free: every @qwen-code/* resolves in-tree, and the PR marker hasNonThoughtCandidateParts is present in one arm and absent in the other). Everything below is measured output, not a re-reading of the diff.

Verdict: merge-ready. Two non-blocking nits at the end.


1. The gap I wanted to close: are the fixtures real?

The PR's own tests feed hand-written { text: '…', thought: true } parts to a fully mocked ContentGenerator. That proves the gate logic, but it cannot prove that the real DashScope path actually produces thought-only chunks for reasoning_content deltas. If it didn't, the fix would be correct in the abstract and vacuous for #7832.

So I wrote an independent harness that removes the mock entirely: a local HTTP server speaking OpenAI-compatible SSE with genuine reasoning_content deltas, which then destroys the TCP socket mid-stream — real undici, real TypeError: terminated / UND_ERR_SOCKET. It runs through the real OpenAIContentGenerator → real converter → real GeminiChat.sendMessageStream.

The path holds up: reasoning_contentcreateOpenAIReasoningThoughtPart(){ text, thought: true } (utils/thoughtUtils.ts:22), which is exactly what the new gate keys on.

real-wire differential

Scenario (real wire) BASE c7de1d7 PR 90fc58c
Areasoning_content only, then socket kill 1 request · 0 retries · throws terminated · model turn lost (history = [user]) 2 requests · 1 RETRY · no throw · answer delivered
B — thinking then visible content, then kill 1 request · 0 retries · throws 1 request · 0 retries · throws — identical
C — usage-only chunk (choices: []), then kill 1 request · 0 retries · throws · 0 answers 2 requests · 1 RETRY · answer ×1

Scenario A is the reporter's bug, reproduced and then fixed end-to-end on the real transport. Scenario B is the important one for confidence: content-phase behavior is proven unchanged rather than merely asserted.

On the safety argument specifically — I checked the recorded history after the replay rather than trusting the rationale:

streamed to caller:            recorded history (role=model):
  THOUGHT 'Let me plan the refactor. '     ← attempt 1     THOUGHT 'Resuming my reasoning.'
  THOUGHT 'First I will read the files… '  ← attempt 1     VISIBLE 'RECOVERED_ANSWER_7938'
  THOUGHT 'Resuming my reasoning. '        ← attempt 2
  VISIBLE 'RECOVERED_ANSWER_7938'

Attempt 1's thinking is absent from history, and the answer occurs exactly once. No duplication, in the UI stream or in persisted history.

Scenario C is worth calling out because the PR flags it as its own riskiest edge. It behaves safely: replay is now permitted after a usage-only chunk, and the recovered output is emitted exactly once with no stale thinking carried over.


2. Are the new tests load-bearing?

Seven mutations, each proven to actually land (no-op edits rejected — a silent no-op would masquerade as a survivor), suite re-run, file restored.

mutation matrix

Mutation Result
M1 gate reverted to !streamYieldedChunk (pre-PR gate) KILLED 1/262
M2 helper neutered → always false KILLED 2/262
M3 !part.thought!!part.thought KILLED 2/262
M4 flag moved inside the isToolCallPreparationOnly guard SURVIVED — provably equivalent
M5 only the first candidate inspected KILLED 3/262
M6 yieldedNonContentChunks hardcoded false SURVIVED — no coverage
M7 retryDecision reverted to skipped_after_chunk SURVIVED — no coverage

M1 and M2 reproduce the PR's own stated mutation numbers exactly (1/262 and 2/262) — the self-report checks out.

M4 is not a test gap: isToolCallPreparationOnly returns true only when no candidate has parts.length > 0, which is precisely the condition under which hasNonThoughtCandidateParts must return false. The two placements are logically equivalent, so no test can distinguish them.

Independent of the mutation matrix, the single-variable A/B — the PR's test file run against BASE source, the PR touching only these two files — fails exactly 1 of 262, and it fails with TypeError: terminated / UND_ERR_SOCKET: the discriminating test reproduces the real bug signature rather than tautologically restating the new code.


3. Regression and gates (PR arm)

Check Result
geminiChat.test.ts 262 passed
full src/core/ slice 2190 passed, 0 failed (52 files)
prettier --check clean
eslint --max-warnings 0 clean
tsc --noEmit — changed file 0 errors
merge into main clean

Two local-environment items, both A/A-controlled so you can discount them: tsc --noEmit reports 31 pre-existing errors (ide-client.ts, sessionService*, client-mcp-registrar.test.ts) whose error sets are byte-identical in both arms with zero in geminiChat.ts; and client.test.ts fails to collect in my tree with the same vitest mock-hoisting error in both arms. That collection failure is why my slice count is 2190 rather than the 2492 in your test plan — the difference is that one uncollectable file, not a regression.


4. Two non-blocking findings

(a) The stated rationale is slightly imprecise. The PR says thought parts "are never recorded as the assistant's final response in history." Empirically they are recorded — the successful attempt's THOUGHT 'Resuming my reasoning.' is persisted (correctly; that's pre-existing behavior). The property that actually makes the replay safe is different and stronger: the failed attempt's accumulated parts are discarded wholesale on retry. The conclusion is right, the reasoning as written is not quite. Worth a one-line correction in the comment at geminiChat.ts so the next reader doesn't rely on a false invariant.

(b) The two diagnostic changes are untested (M6, M7). The PR body presents yieldedNonContentChunks and the skipped_after_content rename as part of the deliverable, but both mutate freely with the suite green. They're debugLogger-only, so severity is low — flagging for completeness, not asking for a change.

Neither blocks. The fix is minimal, load-bearing on the real transport path, and the content-phase guard is genuinely intact. Good PR.

中文说明

维护者验证 —— 本地真实构建与测试

我在两个隔离的 worktree 中对 head 90fc58c 与 merge-base c7de1d7 进行了复核(APFS 克隆 node_modules,两侧均确认无污染:所有 @qwen-code/* 都解析到树内,且 PR 标记 hasNonThoughtCandidateParts 在一侧存在、另一侧不存在)。以下均为实测输出,不是对 diff 的复读。

结论:可以合并。 文末有两个不阻塞的小问题。

1. 我想堵住的缺口:fixture 是真实的吗?

PR 自带的测试是把手写的 { text: '…', thought: true } part 喂给一个完全 mock 的 ContentGenerator。这能证明门控逻辑,但无法证明真实 DashScope 路径确实会为 reasoning_content delta 产生 thought-only chunk。如果不会,这个修复在抽象层面正确,但对 #7832 就是空转的。

因此我写了一个独立的测试装置,彻底去掉 mock:一个本地 HTTP 服务器用 OpenAI 兼容 SSE 发送真实的 reasoning_content delta,然后在流中途销毁 TCP socket —— 真实的 undici、真实的 TypeError: terminated / UND_ERR_SOCKET。全程走真实的 OpenAIContentGenerator → 真实 converter → 真实 GeminiChat.sendMessageStream

这条路径成立:reasoning_contentcreateOpenAIReasoningThoughtPart(){ text, thought: true }utils/thoughtUtils.ts:22),正是新门控所依赖的。

场景(真实链路) BASE c7de1d7 PR 90fc58c
A —— 仅 reasoning_content,随后断连 1 次请求 · 0 重试 · 抛 terminated · model 轮次丢失(history = [user] 2 次请求 · 1 次 RETRY · 未抛错 · 答案送达
B —— 思考后已有可见内容,随后断连 1 次请求 · 0 重试 · 抛错 1 次请求 · 0 重试 · 抛错 —— 完全一致
C —— 仅 usage 的 chunk(choices: []),随后断连 1 次请求 · 0 重试 · 抛错 · 0 条答案 2 次请求 · 1 次 RETRY · 答案 ×1

场景 A 就是报告者的 bug,在真实传输层被复现并修复。场景 B 对信心更关键:内容阶段的行为是被证明未变,而非仅仅被断言未变。

关于安全性论证,我没有采信说法,而是检查了 replay 之后真实记录的 history:attempt 1 的思考内容不在 history 中,答案恰好出现一次。无论 UI 流还是持久化 history 都没有重复。

场景 C 值得单独说明,因为 PR 自己把它标为最大风险点。实测是安全的:usage-only chunk 之后现在允许 replay,恢复的输出恰好发出一次,且没有残留旧的思考内容。

2. 新增测试是否承重?

七个变异,每个都验证过确实生效(拒绝空改动 —— 一个静默的 no-op 会伪装成"存活"),重跑测试后还原文件。

变异 结果
M1 门控回退为 !streamYieldedChunk(PR 前的门控) 杀死 1/262
M2 helper 置为恒 false 杀死 2/262
M3 !part.thought!!part.thought 杀死 2/262
M4 标志移入 isToolCallPreparationOnly 的 guard 内 存活 —— 可证明等价
M5 只检查第一个 candidate 杀死 3/262
M6 yieldedNonContentChunks 硬编码为 false 存活 —— 无覆盖
M7 retryDecision 回退为 skipped_after_chunk 存活 —— 无覆盖

M1 与 M2 精确复现了 PR 自述的变异数据(1/262 与 2/262),自我报告可信。

M4 不是测试缺口:isToolCallPreparationOnly 仅在没有任何 candidate 满足 parts.length > 0 时返回 true,而这正是 hasNonThoughtCandidateParts 必然返回 false 的条件。两种位置逻辑等价,任何测试都无法区分。

此外,单变量 A/B(用 PR 的测试文件跑 BASE 源码;PR 只改了这两个文件)恰好失败 1/262,且失败信息为 TypeError: terminated / UND_ERR_SOCKET —— 这个判别性测试复现的是真实 bug 特征,而不是对新代码的同义反复。

3. 回归与门禁(PR 侧)

geminiChat.test.ts 262 通过;完整 src/core/ 切片 2190 通过、0 失败(52 个文件);prettier 干净;eslint --max-warnings 0 干净;改动文件 tsc --noEmit 0 错误;合入 main 无冲突。

两处本地环境问题,均已做 A/A 对照可排除:tsc --noEmit 报 31 个既有错误,两侧错误集合逐字节一致geminiChat.ts 中为 0;client.test.ts 在我的环境两侧都因同一个 vitest mock hoisting 错误无法收集。后者正是我的切片数是 2190 而非你测试计划中 2492 的原因 —— 差异来自这一个无法收集的文件,不是回归。

4. 两个不阻塞的问题

(a) 论证表述略有偏差。 PR 说 thought part「从不作为 assistant 的最终响应记录进 history」。实测它们确实被记录 —— 成功那次的 THOUGHT 'Resuming my reasoning.' 被持久化了(这是正确的既有行为)。真正让 replay 安全的性质不同且更强:失败那次积累的 part 会被整体丢弃。结论正确,但写下来的理由不够准确。建议在 geminiChat.ts 的注释里改一行,以免后来的读者依赖一个不成立的不变量。

(b) 两处诊断改动无测试覆盖(M6、M7)。PR 正文把 yieldedNonContentChunksskipped_after_content 重命名列为交付内容的一部分,但两者都能自由变异而测试全绿。它们仅走 debugLogger,严重性低 —— 出于完整性提出,不要求修改。

两点都不阻塞。修复足够小、在真实传输路径上确实承重、内容阶段的保护完好。是个不错的 PR。

@wenshao
wenshao added this pull request to the merge queue Jul 30, 2026
Merged via the queue into QwenLM:main with commit d2ab8a5 Jul 30, 2026
95 of 96 checks passed
@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Released in v0.21.2.

qwen-code-dev-bot added a commit to he-yufeng/qwen-code that referenced this pull request Jul 31, 2026
…e replay

Merge main into fix/mid-stream-transport-continuation. Main's QwenLM#7938 split the transport retry gate into a from-zero replay for thinking-only output (!streamYieldedContentChunk); this PR adds a continuation retry for text/thought partials (streamYieldedChunk + pendingPartialTextParts). The replay block runs first and continues, so thinking-only failures replay from zero while content-phase failures continue. The not-taken log keeps main's skipped_after_content discriminator.
he-yufeng added a commit to he-yufeng/qwen-code that referenced this pull request Aug 3, 2026
After the merge with QwenLM#7938, a transport failure after thinking chunks
plus visible content takes the continuation path (blocked from replaying
from zero), so the old no-retry assertion contradicts the merged
behavior. Assert the continuation instead: one retry marked
isContinuation and the coalesced visible text preserved.
@ComplexSimply

Copy link
Copy Markdown
Collaborator Author

@wenshao Thank you for the verification — the real-wire differential (a genuine OpenAI-compatible SSE server emitting real reasoning_content deltas, with a real TCP socket destruction driving undici's UND_ERR_SOCKET into the real OpenAIContentGenerator) proves the fix at a level the PR's own mocked tests could not, and the history check caught a real imprecision in my comments.

Both findings are now addressed in follow-up #8861 (the fixes were ready the same day, but lost the race to the merge by about an hour):

(a) Both comment sites now state the actual invariant: the successful attempt's thoughts are recorded in history; the replay is safe because the failed attempt's accumulated partial turn is discarded wholesale (popPendingPartialAssistantTurn) and thought parts are never user-visible content — so nothing the caller saw from the failed attempt can appear twice.

(b) Both diagnostics are pinned: yieldedNonContentChunks is asserted in the thinking-only replay test, and skipped_after_content is covered by a new function-call-cut test — after #7896 added continuation recovery, a text cut after visible content recovers via continuation instead, so the functionCall cut (replay gate and continuation gate both closed) is the one path on current main that still reaches the not-taken log. Re-running your M6/M7 mutants: each now fails exactly one test (1 failed / 300 passed).

On M4: your equivalence proof matches my reading — isToolCallPreparationOnly can only be true when no candidate carries a non-empty parts, so the mutant is logically unreachable. No action needed there.

中文

@wenshao 感谢验证——真实链路差分(真正的 OpenAI 兼容 SSE 服务器发送真实 reasoning_content delta,真实 TCP socket 销毁驱动 undici 的 UND_ERR_SOCKET 进入真实 OpenAIContentGenerator)把修复证明到了 PR 自带 mock 测试达不到的层级,history 检查也抓到了我注释中的一处真实不精确。

两个 finding 已在后续 PR #8861 落地(修复当天就已完成,但比合并晚了约一小时):

(a) 两处注释现在陈述真正的不变量:成功 attempt 的 thought 进入 history;重放之所以安全,是因为失败 attempt 累积的 partial turn 会被整体丢弃(popPendingPartialAssistantTurn),且 thought part 永远不是用户可见内容——调用方从失败 attempt 看到的任何东西都不可能出现两次。

(b) 两个诊断字段均已钉住:yieldedNonContentChunks 在 thinking-only 重放测试中断言;skipped_after_content 由新增的 function-call 切断测试覆盖——#7896 引入续传恢复后,可见文本切断已改走续传,functionCall 切断(重放与续传门控同时关闭)是当前 main 上唯一仍到达 not-taken 日志的路径。复跑你的 M6/M7 变异体:各恰好导致一个测试失败(1 失败 / 300 通过)。

关于 M4:你的等价性证明与我的理解一致——isToolCallPreparationOnly 只在没有任何 candidate 携带非空 parts 时才为真,该变异体逻辑上不可达。无需处理。

TianYuan1024 pushed a commit to TianYuan1024/qwen-code that referenced this pull request Aug 25, 2026
…fety comment (QwenLM#8861)

* test(core): pin transport retry diagnostics and correct the replay-safety comment

Follow-up to the QwenLM#7938 maintainer verification, addressing both
non-blocking findings.

The comments justifying the thinking-phase replay claimed thought parts
are never recorded in history. That is not the invariant: the successful
attempt's thoughts are recorded. What makes the replay safe is that the
failed attempt's accumulated partial turn is discarded wholesale before
the retry (popPendingPartialAssistantTurn) and thought parts are never
user-visible content. Both comment sites now state that.

The two retry diagnostics were unpinned: hardcoding
yieldedNonContentChunks on the scheduled log or relabeling the
skipped_after_content decision on the not-taken log survived the suite.
The scheduled-log field is now asserted in the thinking-only replay
test, and a new test covers the path on current main that still emits
skipped_after_content — a cut after a delivered functionCall, where the
replay gate and the continuation gate are both closed. Each mutant now
fails exactly one test.

* test(core): close the review's four gaps on the retry diagnostics

Address the inline review on the follow-up:

- Correct the last surviving copy of the stale replay-safety rationale
  (the thinking-phase test's header comment still claimed thoughts never
  enter history).
- Hoist socketCut/cutAfter out of the continuation suite and reuse them
  in the function-call cut test instead of a character-for-character
  inline copy, so the transport error shape has a single producer.
- Pin the false side of yieldedNonContentChunks in the tool-preparation
  retry test, and the 'exhausted' arm of retryDecision in the budget
  exhaustion test. Hardcoding the diagnostic true or collapsing the
  ternary now each fail exactly one test.

* test(core): unify socket-cut fixtures and pin retry exhaustion with thinking (QwenLM#8861)

* docs(core): ground thinking-replay safety in the partial-turn discard (QwenLM#8861)

* fix(core): ground replay rationale and pin retry-decision ternary (QwenLM#8861)

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

---------

Co-authored-by: ComplexSimply <rudy.arrowsong@gmail.com>
Co-authored-by: qwen-code-dev-bot <qwen-code-dev@service.alibaba.com>
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

autofix/takeover Summon the autofix loop to manage this PR (remove to release; needs triage+) category/core Core engine and logic type/bug Something isn't working as expected

Projects

None yet

Development

Successfully merging this pull request may close these issues.

YOLO mode: mid-stream socket close is not retried, making large code generation impossible

4 participants