fix(core): resume long streams cut by a socket-level close - #7896
Conversation
|
Thanks for the PR — this is a well-analysed fix for a real pain point. Template: the headings don't match the template exactly ( Problem: observed bug with strong evidence. Issue #7832 has a 5/5 reproduction table, the root cause was confirmed in source by a maintainer, and the code path ( Direction: aligned. Claude Code's CHANGELOG has two directly analogous fixes ("Fixed streaming responses being discarded when the API emits a mid-stream overloaded/server error after partial output" and "Fixed Size: 269 production lines (geminiChat.ts: +250 −19), 559 test lines (geminiChat.test.ts: +514 −45). Under the 500-line threshold — no maintainer escalation needed. Two files, one production, one test. Focused. Approach: the scope feels right. Two changes, both necessary: (1) widening the replay gate to ignore thoughts/blank text, and (2) adding continuation recovery for cuts after visible output. The continuation path reuses the existing Moving on to code review. 🔍 中文说明感谢贡献!这是一个分析透彻的修复。 模板:标题与模板不完全一致( 问题:已观测到的 bug,证据充分。Issue #7832 有 5/5 复现表,根因已由 maintainer 在源码中确认,代码路径(约第 2579 行的 方向:对齐。Claude Code 的 CHANGELOG 有两个直接类似的修复,确认这是编码代理的公认问题空间。从已交付文本恢复而非重放的方案是正确的。 规模:269 行生产代码(geminiChat.ts: +250 −19),559 行测试代码(geminiChat.test.ts: +514 −45)。低于 500 行阈值,无需维护者升级。两个文件,一个生产一个测试,聚焦。 方案:范围合理。两处改动都是必要的:(1) 放宽重放门控以忽略 thinking/空白文本,(2) 为可见输出后的断连添加续写恢复。续写路径复用了 MAX_TOKENS 路径已有的 进入代码审查 🔍 — Qwen Code · qwen3.8-max-preview Reviewed at |
Code ReviewIndependent proposal. Given the problem (socket close mid-stream, replay gate blocks recovery because thinking chunks trip Comparison with the diff. The PR matches this proposal closely. The two-part structure (widen the replay gate + add continuation recovery) is the right decomposition. Specific observations:
No critical blockers found. No AGENTS.md violations. The code follows existing conventions (ESM, strict TS, collocated tests, kebab-case files). Comments explain the "why" at the right level. One minor observation (non-blocking): the Testing
The main CI suite ("Qwen Code CI") has not executed — it is in The author reports Not verified: unit tests, lint, typecheck (CI has not run). Real-scenario testing is N/A for this run (unattended CI). The bug requires the DashScope gateway's 3–5 min SSE connection cap to reproduce, which cannot be simulated locally; a maintainer can check the PR out in a disposable environment to verify the behavioural claim if needed. 中文说明代码审查:PR 的方案与独立提案高度一致。两部分结构(放宽重放门控 + 添加续写恢复)是正确的分解。 测试:主 CI 套件("Qwen Code CI")尚未执行——处于 — Qwen Code · qwen3.8-max-preview Reviewed at |
|
Confidence: 4/5 — clean review, sound approach, comprehensive tests; the main CI suite hasn't executed yet (fork PR in Stepping back: this is a well-constructed fix for a P1 bug that makes long YOLO-mode generations unrecoverable on DashScope. The author correctly identified that the The implementation reuses the existing The only reservation is the CI gap: the "Qwen Code CI" workflow is in 中文说明置信度 4/5——代码审查干净,方案合理,测试全面;主 CI 套件尚未执行(fork PR 处于 这是一个针对 P1 bug 的高质量修复,解决了 DashScope 上 YOLO 模式长生成不可恢复的问题。作者正确识别了 唯一保留意见是 CI 缺口:主 CI 工作流处于 — Qwen Code · qwen3.8-max-preview Reviewed at |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
| if (suppressNextRetryEvent) { | ||
| suppressNextRetryEvent = false; |
There was a problem hiding this comment.
[Critical] suppressNextRetryEvent branch bypasses continuation state reset — Failure scenario: (1) Socket cut delivers text → continuation scheduled → continuation attempt fails with transport error before any chunks → transport replay fires (suppressNextRetryEvent=true) → next iteration enters the if (suppressNextRetryEvent) arm, skipping the else if that resets transportContinuationCount/Text/Prefix → buildAttemptContents() appends stale synthetic model+user continuation turns to a replay request → UI discarded the text (plain RETRY) but model receives a resume instruction → prependTextToLastModelTurn merges discarded prefix into history → permanent UI/history mismatch on /compress and --resume. (2) Same root cause also affects reactive compression: its success path sets suppressNextRetryEvent = true at ~line 2898, reaching the same bypass. The existing test "drops a pending continuation" uses InvalidStreamError, whose retry branch does not set suppressNextRetryEvent, so neither trigger is exercised.
Fix: reset continuation state in the non-continuation paths that set suppressNextRetryEvent — the transport replay branch (~line 2748) and the reactive compression branch (~line 2898) — before suppressNextRetryEvent = true. A blanket reset in the if (suppressNextRetryEvent) arm would also clear state after a continuation-in-progress yield, breaking ongoing continuations.
— qwen3.7-max via Qwen Code /review
|
Confirmed — the finding is correct, and both triggers you named are real. Fixed in 845f57d. I traced all three writers of On reachability, your ordering for the replay trigger holds — a continuation attempt can itself be cut before yielding visible output, which lands in the replay branch rather than the continuation branch, because replay is still legal with nothing visible delivered. The compression trigger is arguably the more likely of the two: a continuation request sends the delivered text plus the resume instruction on top of the original contents, so it is the attempt most likely to overflow the window in the first place. Fix. Extracted the reset into a On the test gap. You were right that the existing supersession test exercised neither trigger:
Both assert the third request carries neither the delivered text nor the resume instruction, and that history ends with only the clean answer. I verified each test actually catches its bug by reverting its own reset in isolation: each fails with Verification after the fix: One note on the buffer growth flagged as non-blocking in stage 1 and 2: I've left it as is. It's bounded by the 3-attempt budget, and the same text is already held in the caller's buffer, so capping it here would add a truncation path without removing a real allocation. Happy to bound it explicitly if a maintainer would rather have the invariant stated in code. The main CI suite is still in |
|
Pushed Fix: extracted The test-gap point was also right — the pre-existing supersession test used Per-fix control experiment: reverting the replay-branch reset in isolation fails its test ( Verification: /review |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
No issues found. LGTM! ✅
中文说明
未发现问题。LGTM!✅
— qwen3.7-max via Qwen Code /review
Maintainer verification: real local build + end-to-end run against a socket-cutting gatewayI built both sides of this PR locally and drove the real bundled CLI against a real HTTP server that destroys the TCP socket mid-SSE-stream. Nothing here is mocked: real Builds under test — merge-base The fix worksSame gateway, same prompt, same cut. On Most importantly, the mechanism is visible on the wire. Request #1 from the PR build is the continuation, and it carries the delivered turn verbatim: What I verified end-to-end
Row 9 is worth calling out because it validates the second commit specifically. I armed a continuation with a cut, then answered the continuation attempt with an empty stream (handled inside Tests, lint, typecheck (Linux, Node 22)
Two things I'd like your take on before merging1.
|
| # | 行为 | 结果 |
|---|---|---|
| 1 | 在 merge-base 上复现 bug(UND_ERR_SOCKET,1 次请求,整轮失败) |
✅ |
| 2 | PR 从同样的掐断中恢复,答案完成 | ✅ |
| 3 | 续写请求携带已交付文本 + 恢复指令 | ✅ 在网络层面观测到 |
| 4 | 会话内历史被正确缝合(prependTextToLastModelTurn) |
✅ 通过一次独立的后台调用验证 |
| 5 | 反复掐断(3 次)累积了每一个片段 | ✅ 4 次模型调用,全部片段齐全 |
| 6 | 预算上限为 3 次续写,之后向上抛出 | ✅ 恰好 4 次模型调用,然后报错 |
| 7 | 模型重放自己的尾部 → 历史去重 | ✅ 实时输出重复,历史中已去重 |
| 8 | 只交付思考内容的掐断现在会重放(放宽后的门控) | ✅ 3 次尝试,而 merge-base 上只有 1 次 |
| 9 | 完整重发型重试会丢弃待处理的续写 | ✅ 下一个请求不再携带续写状态 |
| 10 | 续写返回工具调用时文本合并正确 | ✅ 已交付文本被并入 functionCall turn |
| 11 | 合成 turn 绝不进入持久化转录 | ✅ 磁盘上两个标记均为 0 次出现 |
第 9 行值得特别指出,因为它专门验证了第二个 commit。我先用一次掐断把续写状态挂起,然后用一个空流(由 geminiChat 处理,而非 SDK)来回应续写尝试。下一个请求回到了 roles=system,user,不带任何合成 turn——待处理的续写被正确丢弃,原始请求被重放。
测试、lint、类型检查(Linux,Node 22)
geminiChat.test.ts—— 270 通过 / 0 失败- RED/GREEN:只把
geminiChat.ts回退到 merge-base、保留 PR 的测试文件,会让 10 个测试变红、260 个仍然通过。保持通过的正是那两个负向用例(functionCall中断、空白文本中断),这是正确的。 tsc --noEmit -p packages/core—— 干净 · 两个文件的eslint --max-warnings 0—— 干净- 更大范围扫描
packages/core/src/core/+retryErrorClassification.test.ts—— 54 个文件,2526 通过,0 失败。 - 说明:描述中提到的两个
session-start-profiler.test.ts失败在这里无法复现——该文件就在src/core/下,在两个分支上都是 19/19 通过。看起来是 Windows 特有的。
合并前有两点想听听你的意见
1. --resume 仍然会丢失已交付的前缀(与描述中的一项声明不符)
描述里说,之所以要把前缀并回去,是因为否则历史里会存下一个从句子中间开始的答案,「这在 /compress、--resume 和之后每一轮的上下文里都能看到」。第一项和第三项确实修好了。--resume 没有。
prependTextToLastModelTurn 修改的是 GeminiChat.history,但磁盘上的聊天记录在那之前就已经写完了。一次已恢复会话的磁盘转录里只有续写的后半段:
[assistant] [{"text":"Planning the five steps.","thought":true},
{"text":"STEP-FOUR: close the handle. STEP-FIVE: report the result. ANSWER-COMPLETE-7896"}]
我用 --continue 真的恢复了这个会话来确认后果。重建后发往上游的历史是:
[assistant] STEP-FOUR: close the handle. STEP-FIVE: report the result. ANSWER-COMPLETE-7896
STEP-ONE/STEP-TWO/STEP-THREE 永久丢失了。同样的探针作用在由既有 MAX_TOKENS 路径恢复的会话上,则完整保留:
[assistant] STEP-ONE: ... STEP-TWO: ... STEP-THREE: ... STEP-FOUR: ... ANSWER-COMPLETE-7896
关于严重程度需要公允地说明:这不是回归。在 merge-base 上,被掐断的这一轮根本不会留下任何 assistant 记录,所以本 PR 严格更好。它只是对描述中所声明目标的一次不完整修复。我建议要么把合并逻辑延伸到 recording service,要么把那句话改得与实际行为一致。
2. 续写尝试没有重新钳制 maxOutputTokens
buildAttemptContents() 会因为已交付文本而让 prompt 变长,但 params.config.maxOutputTokens 仍然是首次发送前钳制出来的值。在网络层面观测到——prompt 在增长,而 max_tokens 保持不变:
传输层续写(本 PR) 既有的 MAX_TOKENS 恢复
req#0 max_tokens=32000 req#0 max_tokens=32000
req#1 max_tokens=32000 req#1 max_tokens=64000 <- 提升
req#2 max_tokens=32000 req#2 max_tokens=64000 <- 每轮重新钳制
MAX_TOKENS 恢复路径是特意重新钳制的,并且写了注释解释原因:「prompt 已经因为上一段部分响应而变长,所以复用首次发送前钳制的值会超出窗口」。由于 clampOutputTokensToWindow 返回的是 window - promptTokens - margin,一旦这个钳制起作用,每次续写都会以「已交付文本的大小」为幅度超出窗口——而已交付文本恰恰在本 PR 所针对的长生成场景中最大。
我没有从中复现出实际失败(我的已交付文本只有约 100 字符,钳制从未起作用)。这里是作为「与本 PR 声称所模仿的路径之间的不对称」提出,而不是作为已确认的 bug。
另外两点较小的说明(都不是阻断项)
- headless
-o text只显示恢复之后的那一段。result字段取的是最后一条 assistant 消息,而重复的思考块会把消息切开。这是既有行为,并非本 PR 造成——在 merge-base 构建上通过 MAX_TOKENS 恢复路径可以完全一样地复现。两种情况下-o stream-json都携带完整内容。 !streamYieldedFunctionCall这个排除条件在 OpenAI/DashScope 的传输路径上似乎不可达。 我尝试了两次「先交付完整工具调用、再掐断 socket」,转换器都会把工具调用连同流一起丢弃,于是streamYieldedFunctionCall始终为 false,续写照常进行(这是无害的,因为什么都没交付出去)。这个保护是正确且低成本的——只是需要知道它目前唯一的覆盖来自单元测试。
结论
核心修复是可靠的、范围清晰的,在我能构造出的每一种失败形态(包括对抗性的那些)下都表现正确。除上述两点外,恢复机制在我测量的每一个维度上都与既有的 MAX_TOKENS 路径保持一致。就我这边而言可以合并;希望第 1 点能以某种方式解决——改代码或改描述都行——因为那是描述中明确作出的声明。
Verified locally with Claude Code (Opus 5, 1M context). Harness: hand-rolled OpenAI-compatible SSE server with real res.destroy() mid-stream; real bundled dist/cli.js on both merge-base and PR head; TUI screenshots via node-pty + xterm.js.
Follow-up on finding 1: I prototyped the obvious fix, and it trades one defect for anotherRather than leave the The prototype. At the point the continuation is scheduled, append the newly delivered delta to the recorder as its own assistant turn (the recorder is append-only — transportContinuationPrefix = transportContinuationText;
const deltaForRecord = transportContinuationText.slice(recordedContinuationChars);
if (deltaForRecord.length > 0) {
self.chatRecordingService?.recordAssistantTurn({ model, message: [{ text: deltaForRecord }] });
recordedContinuationChars = transportContinuationText.length;
}It does fix the reported problem. Rebuilt, re-ran the same socket-cut scenario, resumed with But it breaks the reset path. Same build, the scenario where a fresh-restart retry takes over and the delivered text is deliberately discarded (I drive it with an empty stream on the continuation attempt, which That is precisely the hazard the existing comment in So the correct fix is the stash-and-decide pattern this file already has. That is a real change, not a one-liner — which I think makes it a legitimate follow-up PR rather than something to bolt onto this one. If you'd rather keep this PR at its current scope, the other clean option is to drop Either way, none of this is a regression against All measurements above are from real runs; the prototype was reverted afterwards and the restored PR-head build reproduces the original gap exactly (negative control). 中文说明关于第 1 点的后续:我把「显而易见的修法」做了原型,结果是用一个缺陷换另一个缺陷为了不把 原型做法。 在调度续写的位置,把这次新交付的增量作为一条独立的 assistant turn 追加进 recorder(recorder 是只追加的—— transportContinuationPrefix = transportContinuationText;
const deltaForRecord = transportContinuationText.slice(recordedContinuationChars);
if (deltaForRecord.length > 0) {
self.chatRecordingService?.recordAssistantTurn({ model, message: [{ text: deltaForRecord }] });
recordedContinuationChars = transportContinuationText.length;
}它确实修好了所报告的问题。 重新构建后跑同样的掐断场景,再用 但它破坏了重置路径。 同一个构建,换成「有完整重发型重试接管、已交付文本被刻意丢弃」的场景(我用「对续写尝试返回空流」来触发,这是由 这正是 所以正确的修法应该用这个文件里已有的「暂存再决定」模式。 这是一个真正的改动,不是一行代码——我认为这使它更适合作为后续 PR,而不是硬塞进本 PR。如果你更希望本 PR 保持当前范围,另一个干净的选择是把描述里那句话中的 无论选哪种,这些都不构成相对 以上所有测量都来自真实运行;原型随后已回退,恢复后的 PR-head 构建能完全复现原本的缺口(负向对照)。 Verified locally with Claude Code (Opus 5, 1M context). |
|
Thank you for building the prototype rather than leaving it as an open question — that saved me from shipping the wrong fix, and the negative control (reverting to PR head and reproducing the gap) is what makes the result trustworthy. I verified your distinction statically before acting on it. Taking your recommendation to keep this PR at scope. Two changes, no code:
I filed #8094 for the follow-up, crediting your measurements and marking it as depending on this PR since One note on the template feedback from the earlier triage comment: I've kept the headings as they are in this update to avoid churning the description twice. If the exact template headings plus the 中文说明感谢你把原型真的做出来,而不是把它留成一个开放问题——这让我避免了提交一个错误的修法,而且负向对照(回退到 PR head 并复现出缺口)正是让这个结论可信的地方。 在采纳之前我静态核实了你的区分。 采纳你的建议,本 PR 保持当前范围。两处改动,不涉及代码:
我已提交 #8094 作为后续,注明了你的测量结果,并标注它依赖本 PR,因为 关于早先 triage 评论里的模板反馈:这次更新我保持了现有标题,以免把描述改动两次。如果合并前需要严格使用模板标题并补上 |
Long generations that are cut mid-response by a gateway SSE idle timeout
(DashScope closes at ~3-5 min) failed outright, however many retries were
configured. The stream retry loop could only recover by *replaying* the
request, and that path is gated on `!streamYieldedChunk` — by the time a
long answer is cut, chunks have obviously been delivered, so the gate was
shut exactly when recovery was needed.
Two changes:
- Widen the replay gate from "any chunk yielded" to "visible output
yielded" (non-blank answer text or a functionCall). Thinking models emit
reasoning within seconds, which made replay unreachable for the very
generations that need it. The UI's non-continuation RETRY handler clears
the thought buffer, and the rate-limit and invalid-stream branches
already re-send after thoughts have streamed, so this matches existing
behaviour rather than adding risk.
- Add continuation recovery for cuts that already delivered answer text,
where replaying would duplicate what the caller has on screen. Keep the
delivered text, ask the model to resume from it, and emit
`{ RETRY, isContinuation: true }` so the UI keeps its text buffer — the
same shape the MAX_TOKENS truncation path already uses. The synthetic
turns are built into the request only, never written to `history`, so
they cannot leak into the transcript or a later compression; on success
the delivered prefix is merged back into the trailing model turn, with
replayed overlap deduped by the existing recovery helper. Budget of 3,
since one generation can be cut repeatedly by the same timeout.
Cuts that delivered a `functionCall` are excluded: injecting a user turn
between a `functionCall` and its `functionResponse` yields a sequence
providers reject, and the scheduler's repair path already covers it.
Fixes QwenLM#7832
Review found that the `suppressNextRetryEvent` arm bypasses the continuation reset. Two branches emit a non-continuation RETRY *and* set `suppressNextRetryEvent`, so the next loop iteration takes the suppress arm and skips the `else if` that clears the staged continuation: - the transport replay branch, reachable when a continuation attempt is itself cut before yielding visible output, and - the reactive compression branch, which is if anything more likely on a continuation attempt, since that request carries the delivered text and the resume instruction on top of the original contents. Either way the UI has dropped the delivered text (plain RETRY) while the request still asks the model to resume from it, and the merge on success writes that discarded text back into history — a permanent UI/history mismatch visible on `/compress` and `--resume`. Extract the reset into `resetTransportContinuation()` and call it from both branches before they set the flag. Resetting inside the suppress arm instead would clear the state of a continuation that is legitimately in flight. The pre-existing supersession test used `InvalidStreamError`, whose retry branch does not set `suppressNextRetryEvent`, so it exercised the `else if` path and neither trigger. Add a test per branch; both fail if their reset is removed.
845f57d to
80aa4da
Compare
|
Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. 中文请勿对活跃的 PR 执行 rebase 或 force-push,因为这会使已有的评审评论失效。另外,供日后参考:作为集成流程的一部分,机器人始终会自动将所有改动压缩(squash)为单个提交。 |
|
Rebased onto
What remains is this PR's actual thesis, which So the two changes compose rather than overlap: One of main's tests had to change, and I want to be explicit about it rather than have it look like a silently relaxed assertion. const secondRequest = vi.mocked(
mockContentGenerator.generateContentStream,
).mock.calls[1]![0].contents as Content[];
expect(secondRequest.at(-2)).toEqual({
role: 'model',
parts: [{ text: 'Visible answer begins' }],
});
expect(
events.filter(
(event) =>
event.type === StreamEventType.RETRY && !event.isContinuation,
),
).toHaveLength(0);A replay resends the original contents unchanged; a continuation carries the delivered text plus a resume instruction. Asserting the second request's shape distinguishes them, so the test still fails if a replay sneaks back in — it just no longer fails merely because any second request was made. The prefix is asserted to be only One fix the rebase surfaced: Verification on the rebased head ( |
|
Noted on the force-push, and my apologies for tripping the reminder — I'll use a merge commit for conflict resolution here from now on rather than a rebase. For the record on what this particular push cost, since the concern is invalidated review comments: nothing was orphaned. The only inline comment on this PR is the CI bot's on The reason I reached for a rebase is that |
| const chunkParts = chunk.candidates?.[0]?.content?.parts; | ||
| transportContinuationText += getPlainTextFromParts(chunkParts); |
There was a problem hiding this comment.
[Critical] transportContinuationText accumulates every delivered chunk across all continuation attempts via raw += with no inter-attempt overlap dedup. When a model replays its own tail in an intermediate continuation attempt (which the PR explicitly acknowledges models do — the single-cut test drops replayed overlap when the model repeats its own tail covers this), the replayed text is baked into transportContinuationPrefix and propagated to every subsequent request and into durable history. — Failure scenario: attempt 1 delivers "part one " and is cut; attempt 2 replays the tail, delivering "part one part two ", and is cut again. transportContinuationText becomes "part one part one part two ". The merge-time dedup in prependTextToLastModelTurn (getRecoveryContinuationSuffix) only strips overlap replayed by the final attempt against the prefix — it never dedups the prefix against itself — so the final history turn contains duplicated text, corrupting /compress, --resume, and every later turn's context.
| const chunkParts = chunk.candidates?.[0]?.content?.parts; | |
| transportContinuationText += getPlainTextFromParts(chunkParts); | |
| const chunkParts = chunk.candidates?.[0]?.content?.parts; | |
| const chunkText = getPlainTextFromParts(chunkParts); | |
| if (transportContinuationPrefix.length > 0) { | |
| const dedupedSuffix = getRecoveryContinuationSuffix( | |
| transportContinuationText, | |
| chunkText, | |
| ); | |
| transportContinuationText += dedupedSuffix; | |
| } else { | |
| transportContinuationText += chunkText; | |
| } |
中文说明
[Critical] transportContinuationText 通过原始 += 在所有续写尝试间累积每个已交付的 chunk,没有尝试间重叠去重。当模型在中间续写尝试中重放自己的尾部时(PR 明确承认模型会这样做——单次截断测试 drops replayed overlap when the model repeats its own tail 覆盖了这种情况),重放的文本会被固化到 transportContinuationPrefix 中,并传播到每个后续请求和持久化历史中。— 故障场景:尝试 1 交付 "part one " 后被截断;尝试 2 重放尾部,交付 "part one part two ",再次被截断。transportContinuationText 变为 "part one part one part two "。prependTextToLastModelTurn 中的合并时去重(getRecoveryContinuationSuffix)只剥离最终尝试相对于前缀重放的重叠——它从不对前缀自身进行去重——因此最终的历史轮次包含重复文本,破坏 /compress、--resume 和每个后续轮次的上下文。
建议修复:在累积时对每个 chunk 的文本应用与前缀的重叠去重,而不是原始字符串拼接。
— qwen3.8-max-preview via Qwen Code /review
| const textIndex = parts.findIndex(isPlainTextPart); | ||
| if (textIndex < 0) { | ||
| const insertAt = parts.findIndex((part) => !part.thought); |
There was a problem hiding this comment.
[Suggestion] The textIndex < 0 branch (continuation turn has no plain-text part — e.g. a thinking model emits only thought chunks then completes with finishReason: 'STOP') and the textIndex > 0 case (a thought part precedes the text part) have no test coverage. All existing continuation tests use textChunk() which produces a single non-thought text part at index 0. — Concrete cost: a regression in the insertAt computation (e.g. always inserting at 0 instead of after thought parts) or in the findIndex call would silently misplace or drop the delivered text from durable history — visible on /compress, --resume, and every later turn's context — with no test to catch it.
| const textIndex = parts.findIndex(isPlainTextPart); | |
| if (textIndex < 0) { | |
| const insertAt = parts.findIndex((part) => !part.thought); | |
| const textIndex = parts.findIndex(isPlainTextPart); | |
| if (textIndex < 0) { | |
| const insertAt = parts.findIndex((part) => !part.thought); |
Consider adding a test where the continuation attempt yields a thought chunk followed by a text chunk (covers textIndex > 0), and one where it yields only a thought chunk with finishReason: 'STOP' (covers textIndex < 0), asserting chat.getHistory().at(-1) pins the merged text in the correct part position.
中文说明
[Suggestion] textIndex < 0 分支(续写轮次没有纯文本 part——例如思考模型只发出思考 chunk 然后以 finishReason: 'STOP' 完成)和 textIndex > 0 的情况(思考 part 在文本 part 之前)没有测试覆盖。所有现有的续写测试都使用 textChunk(),它产生一个位于索引 0 的单个非思考文本 part。— 具体代价:insertAt 计算中的回归(例如总是在 0 处插入而不是在思考 part 之后)或 findIndex 调用中的回归会静默地将已交付文本从持久化历史中错误放置或丢弃——在 /compress、--resume 和每个后续轮次的上下文中可见——没有测试可以捕获。
建议添加测试:续写尝试产出一个思考 chunk 后跟一个文本 chunk(覆盖 textIndex > 0),以及一个只产出思考 chunk 且 finishReason: 'STOP' 的情况(覆盖 textIndex < 0),断言 chat.getHistory().at(-1) 将合并文本固定在正确的 part 位置。
— qwen3.8-max-preview via Qwen Code /review
The continuation buffer accumulated each attempt's delivered text with a raw `+=`. Overlap dedup ran only at merge time, comparing the *final* attempt against the buffer, so an overlap replayed by an intermediate attempt was never stripped: it was baked into the buffer, sent in every later request, and merged into durable history. Reachable because maxContinuationRetries is 3, so two cuts fit in one send. Attempt 1 delivers "part one " and is cut; attempt 2 replays its tail as "part one part two " and is cut too. The buffer became "part one part one part two " — corrupting /compress, --resume, and the context of every later turn. The fix folds one attempt at a time. A per-attempt buffer collects the running attempt's text, and the catch path folds it into the accumulated buffer through getRecoveryContinuationSuffix, stripping whatever that attempt replayed. Deduping per chunk instead would be wrong: the overlap scan is suffix-anchored, so it would eat legitimately repeated text mid-stream. The attempt boundary is the only place a replay can occur. Also covers the two untested branches of the delivered-text merge, both of which already behaved correctly: a continuation leading with a thought part (the text merges into the text part, not ahead of the thinking) and one that finishes with only a thought part (the delivered text is inserted after it). Without coverage a regression in the insert position would silently misplace text in history. 276 tests pass in geminiChat.test.ts; tsc and eslint clean.
|
Both points addressed in The overlap bugReproduced exactly as described. A test with attempt 1 delivering The analysis is right on both counts: merge-time dedup compares only the final attempt against the buffer and never the buffer against itself, and the two existing tests miss it because they cover the pieces separately — I did not take the suggested diff, and the reason matters: applying So the fix folds one attempt at a time. A per-attempt buffer ( Same end state as your suggestion for the reported scenario, without the mid-stream false positive. The uncovered merge branchesAdded both, and both already behaved correctly — so these pin existing behavior rather than fixing anything:
You were right that 276 tests pass in |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
中文说明
已审查——无阻断问题。 建议见行内评论。
— qwen3.8-max-preview via Qwen Code /review
| const TRANSPORT_CONTINUATION_MESSAGE = | ||
| 'The connection dropped mid-response. Resume directly — no apology, no ' + | ||
| 'recap of what you were doing. Pick up mid-thought if that is where the ' + | ||
| 'cut happened. Break remaining work into smaller pieces.'; |
There was a problem hiding this comment.
[Suggestion] The resume-instruction prose is duplicated verbatim between TRANSPORT_CONTINUATION_MESSAGE and OUTPUT_RECOVERY_MESSAGE (line 524) — only the lead-in sentence differs. The doc comment on buildRecoveryMessageFromText promises the two paths "cannot drift," but that guarantee covers only the suffix-fencing block built inside that function, not this lead-in prose embedded in the constants themselves. — Concrete cost: if someone tunes the resume wording in one constant (e.g. to reduce model recap behavior), the other recovery path silently keeps the old wording and the two paths behave differently with no intended reason.
const RECOVERY_RESUME_BODY =
'Resume directly — no apology, no recap of what you were doing. Pick up ' +
'mid-thought if that is where the cut happened. Break remaining work into ' +
'smaller pieces.';
const OUTPUT_RECOVERY_MESSAGE = `Output token limit hit. ${RECOVERY_RESUME_BODY}`;
const TRANSPORT_CONTINUATION_MESSAGE = `The connection dropped mid-response. ${RECOVERY_RESUME_BODY}`;中文说明
[Suggestion] 续写指令文案在 TRANSPORT_CONTINUATION_MESSAGE 与 OUTPUT_RECOVERY_MESSAGE(524 行)之间逐字重复——仅引导句不同。buildRecoveryMessageFromText 的文档注释承诺两条路径「不会漂移」,但该保证只覆盖该函数内部构建的 suffix 围栏块,不覆盖嵌入常量本身的这段引导文案。— 具体代价:若有人调整其中一个常量的续写措辞(例如为减少模型复述行为),另一条恢复路径会静默保留旧措辞,两条路径无故行为不一致。建议抽取共享文案体(见上方代码块)。
— qwen3.8-max-preview via Qwen Code /review
| resetTransportContinuation(); | ||
| suppressNextRetryEvent = true; | ||
| await delay(delayMs, params.config?.abortSignal).promise; | ||
| continue; |
There was a problem hiding this comment.
[Suggestion] The replay branch is checked before the continuation branch and gates only on the per-attempt streamYieldedContentChunk. After one or more continuations have already delivered visible text (transportContinuationText non-empty, transportStreamRetryCount still 0), a subsequent thought-only cut sets streamYieldedContentChunk=false for that attempt, so this replay branch fires: it calls resetTransportContinuation() (clearing all accumulated text) and emits a plain RETRY (no isContinuation), telling the UI to discard the delivered output. — Failure scenario: attempt 1 delivers "part one " → continuation; attempt 2 delivers "part two " → continuation; attempt 3 is in a thinking phase when cut again → streamYieldedContentChunk is false → replay fires, discarding "part one part two " that the user was watching, and the model regenerates from scratch — losing output the continuation branch below would have preserved.
if (
isRetryableStreamTransportError &&
!streamYieldedContentChunk &&
transportContinuationText.trim().length === 0 &&
transportStreamRetryCount <
TRANSPORT_STREAM_RETRY_CONFIG.maxRetries
) {中文说明
[Suggestion] 重放分支在续写分支之前检查,且仅以单次尝试的 streamYieldedContentChunk 为门控。在一次或多次续写已交付可见文本后(transportContinuationText 非空、transportStreamRetryCount 仍为 0),随后一次仅思考内容的中断会使本次尝试 streamYieldedContentChunk=false,于是此重放分支触发:调用 resetTransportContinuation()(清空所有累积文本)并发出不带 isContinuation 的普通 RETRY,让 UI 丢弃已交付输出。— 故障场景:尝试 1 交付 "part one " → 续写;尝试 2 交付 "part two " → 续写;尝试 3 处于思考阶段再次被掐断 → streamYieldedContentChunk 为 false → 重放触发,丢弃用户正在观看的 "part one part two ",模型从头重新生成——丢失了下方的续写分支本可保留的输出。建议在重放分支条件中追加 transportContinuationText.trim().length === 0(见上方代码块)。
— qwen3.8-max-preview via Qwen Code /review
…vers
The replay gate tests `!streamYieldedContentChunk`, which is declared inside
the attempt loop and therefore per-attempt. That cannot distinguish "nothing
has been delivered to the caller" from "this attempt was cut before delivering
anything, after earlier attempts already put text on screen" — and only the
first is replayable. Because the replay branch is checked before the
continuation branch, the second case fell into it: `resetTransportContinuation()`
dropped the accumulated text and a plain RETRY (no `isContinuation`) told the UI
to discard output the user was watching, after which the model regenerated it
from scratch.
Reachable two ways once a continuation is in flight, both with
`transportStreamRetryCount` still 0 so the replay budget is intact:
- attempt 1 delivers "part one " -> continuation; attempt 2 is cut while
still in its thinking phase, so it yields only a thought part and
`streamYieldedContentChunk` is false;
- the same, with attempt 2 cut having yielded nothing at all.
Gate the replay branch on the accumulated buffer being empty as well. The two
gates are now exact complements (`=== 0` here, `> 0` on the continuation
branch), so a retryable transport cut after any delivered text always
continues and never replays. The `resetTransportContinuation()` call inside the
branch now has nothing to clear; it stays as an assertion of that invariant so
a future gate change cannot leak staged text into a restarted attempt.
`drops a pending continuation when a replay takes over` asserted the old
behaviour for the yielded-nothing case — it was written to pin the design's
internal consistency, not a user-visible requirement, and that premise is what
this fixes. Rewritten as `keeps continuing when a later attempt is cut with
nothing yielded`, asserting the delivered text survives into the third request
and into history. The sibling tests covering the *other* plain-RETRY paths
(fresh-restart retry, reactive compression) are unchanged and still pass:
those genuinely do discard the delivered text, and reaching them still resets.
Also extracts the resume instruction shared verbatim by
OUTPUT_RECOVERY_MESSAGE and TRANSPORT_CONTINUATION_MESSAGE into
RECOVERY_RESUME_INSTRUCTION. Only the lead-in sentence naming the cause
differed, so tuning the wording on one path silently left the other on the old
text. Both resulting strings are byte-identical to before.
277 tests pass in geminiChat.test.ts; both new tests verified failing without
the source change. tsc --noEmit and eslint clean.
|
Both addressed in The replay branch discarding a live continuationConfirmed, including the reachability argument. Two reachable shapes, both with One of the two RETRY events came through plain, which is the UI being told to throw away Took the suggested diff as-is — gating on Two consequences worth flagging rather than leaving to be discovered in review: The One of my own tests asserted the buggy behaviour. I checked the sibling tests rather than assuming they were the same case: Duplicated resume proseFair, and the doc-comment caveat is accurate — I used the name 277 tests pass in |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
中文说明
已审查——无阻断问题。 建议见行内评论。
— qwen3.8-max-preview via Qwen Code /review
| // stripping any overlap it replayed from the previous attempt's tail. | ||
| // Called on both exits from an attempt — success and cut — so the | ||
| // accumulated buffer never contains text twice. |
There was a problem hiding this comment.
[Suggestion] This comment says foldTransportAttemptText is "Called on both exits from an attempt — success and cut", but it is only called in the catch block below. The success path calls prependTextToLastModelTurn(transportContinuationPrefix) and breaks without folding, so transportContinuationText is left missing the final attempt's text. — Concrete cost: the stated invariant ("the accumulated buffer never contains text twice") is maintained by the code, but the factual claim about call sites is wrong; a future maintainer adding a post-loop read of transportContinuationText (telemetry, or a guard deciding whether to run MAX_TOKENS recovery) would silently get a value omitting the last successful attempt's output. No runtime effect today — the buffer is never read after a successful break.
| // stripping any overlap it replayed from the previous attempt's tail. | |
| // Called on both exits from an attempt — success and cut — so the | |
| // accumulated buffer never contains text twice. | |
| // stripping any overlap it replayed from the previous attempt's tail. | |
| // Called on the error exit only, so the next attempt's continuation | |
| // request carries everything delivered so far. On success the loop | |
| // breaks (after prependTextToLastModelTurn merges the prefix into | |
| // history) and the accumulated buffer is not read again, so no fold | |
| // is needed there. |
中文说明
[Suggestion] 此注释称 foldTransportAttemptText 「在 attempt 的两个出口——成功与截断——都会被调用」,但它实际上只在下方 catch 块中被调用。成功路径会调用 prependTextToLastModelTurn(transportContinuationPrefix) 后直接 break,并不执行 fold,因此 transportContinuationText 会缺少最后一次 attempt 的文本。— 具体代价:注释所述不变量(「累积缓冲区绝不会包含重复文本」)由代码保证成立,但关于调用位置的事实性描述是错误的;未来若有维护者在循环之后新增对 transportContinuationText 的读取(遥测,或判断是否进入 MAX_TOKENS 恢复的守卫),将会悄无声息地得到一个缺少最后一次成功 attempt 输出的值。今天没有运行时影响——成功 break 之后该缓冲区不会再被读取。
— qwen3.8-max-preview via Qwen Code /review
The comment claimed the fold runs on both exits from an attempt. It runs only in the catch block; the success exit merges the continuation prefix into history and breaks without folding. No behaviour change — every read of the accumulated buffer is inside the catch, after the fold. But the comment would mislead anyone adding a post-loop read, which would silently omit the last attempt's text, so the note now says which exit folds and what a new reader would have to do.
|
Fixed in
I also verified the "no runtime effect today" half, since that's what decides whether this is a comment fix or a code fix. Every read of
Nothing reads it after the loop, so folding on the success path would have no reader and adding a call there would be dead code. Comment corrected instead: That keeps your point about the future maintainer, which is the real value here — it names the condition under which the current arrangement stops being safe, rather than just describing what the code does. 277 tests still pass in |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
No issues found. LGTM! ✅
中文说明
未发现问题。LGTM!✅
— qwen3.8-max-preview via Qwen Code /review
Maintainer re-verification at
|
| build | what it is | why |
|---|---|---|
b64a6c4 |
merge-base | the "before" |
80aa4da |
PR minus its two newest fixes | RED control for a1fd8a6 and 8528cc0 |
6371637 |
PR head | the "after" |
The fix works
Same gateway, same prompt, same cut, real TUI. On the merge-base the turn dies and exactly one request is ever made — no retry is attempted, because the replay gate was already shut by the first chunk. On this PR the delivered text is kept, the model resumes, and the answer completes.
The mechanism is visible on the wire — request #1 is a continuation, not a replay:
The two new fixes, RED/GREEN against a real cut
Both of the substantive commits added since my last review reproduce as real end-to-end defects on 80aa4da and are gone at head. This is the part I most wanted to confirm, since both were found by review rather than by a failing run:
For 8528cc0 in particular, the consequence on 80aa4da is not subtle: PART-ONE delivered. is dropped from the request and from durable history, so the user's screen and the model's context both lose work that was already produced. At head both keep it.
What I verified end-to-end
| # | Behaviour | Result |
|---|---|---|
| 1 | Bug reproduces on merge-base (UND_ERR_SOCKET, 1 request, turn fails) |
✅ |
| 2 | PR recovers the same cut, answer completes | ✅ |
| 3 | Continuation request carries delivered text + resume instruction | ✅ observed on the wire |
| 4 | In-session history stitched (prependTextToLastModelTurn) |
✅ two independent probes |
| 5 | Repeated cuts accumulate every delivered fragment | ✅ prefix grows per attempt; final history holds every fragment |
| 6 | Budget capped at 3 continuations, then propagates | ✅ exactly 4 calls, then the error |
| 7 | Model replays its tail → history deduped | ✅ |
| 8 | Model ignores the instruction and restarts from scratch → history deduped | ✅ |
| 9 | Overlap across an intermediate cut deduped (a1fd8a6) |
✅ RED on 80aa4da, green at head |
| 10 | Later attempt cut during thinking still continues (8528cc0) |
✅ RED on 80aa4da, green at head |
| 11 | Every later attempt cut while thinking → bounded, no loop | ✅ 4 calls, prefix stable, then propagates |
| 12 | Thought-only first cut still replays (main's #7938) | ✅ replay, not continuation |
| 13 | Fresh-restart retry drops a pending continuation | ✅ next request carried no continuation state, no leak into history |
| 14 | Continuation returning a tool call: text merged, tool actually runs | ✅ marker file written |
| 15 | Legitimately repeated text inside one attempt is not eaten by the dedup | ✅ "REPEAT-ME. REPEAT-ME. " survives intact |
| 16 | Synthetic turns never reach the durable transcript | ✅ 0 occurrences of either marker on disk, across every run |
Row 15 is worth calling out because it validates the design choice you argued for when you rejected the per-chunk dedup: folding at the attempt boundary keeps a phrase the model legitimately repeats mid-stream, which a suffix-anchored per-chunk scan would have eaten.
Row 13 I drove by answering the continuation attempt with an empty stream (handled inside geminiChat, not the SDK): the next request came back as roles=system,user, and durable history held only the clean answer — the discarded prefix did not leak in. The real TUI shows only the clean answer too, so UI and history agree.
Tests, lint, typecheck (Linux, Node 22)
geminiChat.test.ts— 277 passed / 0 failed- RED/GREEN: reverting only
geminiChat.tsto the merge-base while keeping the PR's test file flips 14 red / 263 pass. The three that stay green are exactly the negative cases (functionCallcut, thought-only replay, blank-text replay), which is correct. - Per-fix RED: with the source at
80aa4daand the head's test file, exactly 3 fail —drops replayed overlap when an intermediate attempt is cut again,keeps continuing when a later attempt is cut during thinking,keeps continuing when a later attempt is cut with nothing yielded. One per finding, nothing incidental. tsc --noEmit -p packages/coreclean ·eslint --max-warnings 0on both files clean ·prettier --checkclean- Wider sweep
packages/core/src/core/— 55 files, 2542 passed, 0 failed. The twosession-start-profiler.test.tsfailures the description mentions still do not reproduce here; they look Windows-specific.
I also checked the comment corrected in 6371637: foldTransportAttemptText has exactly one call site (the catch), and every read of transportContinuationText (the replay gate, the continuation gate, the prefix assignment, the deliveredChars field) sits after it inside that same catch. The comment now matches the code.
Non-blocking notes
1. maxOutputTokens is still not re-clamped for continuation attempts. Unchanged since I raised it; re-measured on this head. buildAttemptContents() grows the prompt by the delivered text, but max_tokens stays flat while the pre-existing MAX_TOKENS path re-clamps per iteration:
transport continuation (this PR) pre-existing MAX_TOKENS recovery
req#0 max_tokens=32000 req#0 max_tokens=32000
req#1 max_tokens=32000 req#1 max_tokens=64000 <- escalated
req#2 max_tokens=32000 req#2 max_tokens=64000 <- re-clamped per iteration
req#3 max_tokens=32000
I still cannot make it fail (my delivered text is ~100 chars, so the clamp never binds), and it is an asymmetry with the path the PR says it mirrors rather than a confirmed bug — but it is the one item from my earlier review that is neither fixed nor written down. Worth a line in the description or a follow-up issue so it isn't rediscovered.
2. The !streamYieldedFunctionCall exclusion is still unreachable via the OpenAI/DashScope wire path. I delivered a complete tool call and then cut the socket; the converter drops the tool call along with the stream, so the flag stays false and the continuation proceeds — harmlessly, since the tool call never reached the caller and nothing can be duplicated. (Separately, row 14 covers the case that does occur: a continuation whose own response carries a tool call merges the delivered text correctly and the tool runs.) The guard is correct and cheap; just be aware its only coverage is the unit test.
3. A cut before any response byte is absorbed by the SDK's own connection retry, not by this code — I confirmed it by body-identical consecutive requests with a single error surfacing. That makes the thinking-phase cut the reachable shape of "a later attempt delivered nothing", which is exactly what 8528cc0 fixes, so the fix targets the case that actually occurs.
4. On-screen duplication when the model replays its tail is not removed (history is). At head the terminal still shows SEG-ALPHA. SEG-BRAVO. SEG-BRAVO. SEG-CHARLIE. while history holds each segment once. That is inherent — the bytes were already delivered and cannot be unsent — and the MAX_TOKENS path behaves the same way. Not a defect of this PR; noting it so nobody reads row 7/9 as "the user never sees a duplicate".
5. Headless -o stream-json keeps text that a plain RETRY discarded. Pre-existing, and I verified that on the merge-base build rather than assuming it: a thought-only cut that replays emits both the discarded and the kept reasoning into one assistant message. The interactive TUI discards correctly in the same scenario. Unrelated to this PR, but it means headless output is not a reliable witness for what the user saw.
6. --resume gap confirmed exactly as the description now documents it. The on-disk record for a recovered turn is [assistant] STEP-THREE… ANSWER-COMPLETE-7896, and resuming with --continue sends that same truncated turn upstream — STEP-ONE/STEP-TWO are gone. On the merge-base the cut turn records no assistant record at all, so this is strictly better and not a regression. The Known gap section and #8094 describe this accurately.
7. Description nit — the Reviewer Test Plan numbers are stale. It still says 268 passed, "nine cases", and "seven of the nine new tests fail". Current reality is 277 passed, fifteen cases in the transport stream continuation (#7832) block, and fourteen tests flip red on revert. Worth a one-pass refresh before merge since it is the section a reviewer runs against.
Verdict
The core fix is sound and behaves correctly under every failure shape I could construct, including the adversarial ones. The two review findings fixed since my last pass were both real, both reproduce end-to-end on the pre-fix commit, and both are pinned by their own test. From my side this is good to merge; items 1 and 7 are description-level and can be handled in a single edit.
中文说明
维护者在 6371637 上的复验:本地真实构建 + 掐断 socket 的真实网关
自我上次验证之后,本分支 rebase 到了 main(吸收了 #7938 中重放门控的那一半),又新增了三个提交,所以我从零重新构建,并针对当前 head 重跑了整个矩阵。这里没有任何 mock:真实的打包 CLI、真实的 undici、在 SSE 流中途销毁 TCP 连接的真实 HTTP 服务器、真实的错误分类、真实的恢复循环。在跑其它任何东西之前,客户端抛出的正是生产环境的那个错误:
error.name = TypeError
error.message = terminated
cause.code = UND_ERR_SOCKET <- 即 RETRYABLE_STREAM_TRANSPORT_CODES 里的那个 code
cause.message = other side closed
三个被测构建,各自执行 core build + esbuild bundle,在信任任何一次运行之前先在 bundle 层面验证 A/B 确实换掉了(Transport stream continuation scheduled / The connection dropped mid-response 只出现在预期的构建里):
| 构建 | 是什么 | 用途 |
|---|---|---|
b64a6c4 |
merge-base | 「改动前」 |
80aa4da |
本 PR 去掉最新两个修复 | a1fd8a6 与 8528cc0 的 RED 对照 |
6371637 |
PR head | 「改动后」 |
修复有效
同一个网关、同一个提示词、同一次掐断,真实 TUI。在 merge-base 上这一轮直接失败,并且总共只发出一次请求——完全没有尝试重试,因为重放门控已经被第一个 chunk 关闭了。在本 PR 上,已交付的文本被保留,模型继续,答案完成。
机制在网络请求层面是可见的——req#1 是续写请求,不是重放。
两个新修复:针对真实掐断的 RED/GREEN
自我上次审查以来新增的两个实质性提交,在 80aa4da 上都能复现出真实的端到端缺陷,在 head 上都已消失。这是我最想确认的部分,因为这两个问题都是靠代码审查发现的,而不是靠某次失败的运行。
其中 8528cc0 在 80aa4da 上的后果并不轻微:PART-ONE delivered. 会同时从请求和持久化历史中被丢弃,于是用户屏幕和模型上下文都会丢掉已经产出的成果。在 head 上两者都保留了。
我端到端验证了什么
| # | 行为 | 结果 |
|---|---|---|
| 1 | 在 merge-base 上复现 bug(UND_ERR_SOCKET,1 次请求,整轮失败) |
✅ |
| 2 | 本 PR 从同一次掐断中恢复,答案完成 | ✅ |
| 3 | 续写请求携带已交付文本 + 恢复指令 | ✅ 在网络请求层面观测到 |
| 4 | 会话内历史被缝合(prependTextToLastModelTurn) |
✅ 两条独立探针 |
| 5 | 反复掐断时累积每一个已交付片段 | ✅ 前缀逐次增长;最终历史包含全部片段 |
| 6 | 预算上限 3 次续写,随后向上抛出 | ✅ 恰好 4 次调用,然后报错 |
| 7 | 模型重复自己的尾部 → 历史去重 | ✅ |
| 8 | 模型忽略指令从头重来 → 历史去重 | ✅ |
| 9 | 跨「中间那次掐断」的重叠去重(a1fd8a6) |
✅ 80aa4da 上 RED,head 上通过 |
| 10 | 后续尝试在思考阶段被掐断时仍然续写(8528cc0) |
✅ 80aa4da 上 RED,head 上通过 |
| 11 | 所有后续尝试都在思考阶段被掐断 → 有界,无死循环 | ✅ 4 次调用,前缀稳定,随后抛出 |
| 12 | 首次就是思考阶段掐断时仍走重放(main 的 #7938) | ✅ 重放而非续写 |
| 13 | 完整重发型重试丢弃待处理的续写状态 | ✅ 下一次请求不带任何续写状态,历史无泄漏 |
| 14 | 续写返回工具调用:文本正确并入,工具真的执行 | ✅ marker 文件已写出 |
| 15 | 单次尝试内部合理重复的文本不会被去重逻辑吃掉 | ✅ "REPEAT-ME. REPEAT-ME. " 完整保留 |
| 16 | 合成 turn 从不进入持久化转录 | ✅ 所有运行中磁盘上两个标记均为 0 次 |
第 15 行值得专门指出,因为它验证了你在否决「按 chunk 去重」时所持的设计理由:在尝试边界折叠,可以保住模型在流中途合理重复的短语,而后缀锚定的按 chunk 扫描会把它吃掉。
第 13 行我是通过「对续写尝试返回空流」来驱动的(这由 geminiChat 自己处理,不是 SDK):下一次请求回到 roles=system,user,持久化历史里只有干净的答案——被丢弃的前缀没有漏进去。真实 TUI 同样只显示干净的答案,即 UI 与历史一致。
测试、lint、类型检查(Linux,Node 22)
geminiChat.test.ts—— 277 通过 / 0 失败- RED/GREEN:只把
geminiChat.ts回退到 merge-base、保留 PR 的测试文件,会翻红 14 个 / 263 通过。仍然通过的三个恰好是负向用例(functionCall掐断、仅思考走重放、空白文本走重放),这是正确的。 - 逐修复 RED:源码取
80aa4da、测试文件取 head,恰好 3 个失败——drops replayed overlap when an intermediate attempt is cut again、keeps continuing when a later attempt is cut during thinking、keeps continuing when a later attempt is cut with nothing yielded。每个发现对应一个,没有连带失败。 tsc --noEmit -p packages/core干净 · 两个文件的eslint --max-warnings 0干净 ·prettier --check干净- 更大范围扫描
packages/core/src/core/—— 55 个文件,2542 通过,0 失败。描述里提到的两个session-start-profiler.test.ts失败在这里依然无法复现,看起来是 Windows 特有的。
我也核对了 6371637 修正的那条注释:foldTransportAttemptText 确实只有一个调用点(catch),而 transportContinuationText 的每一次读取(重放门控、续写门控、前缀赋值、deliveredChars 字段)都位于同一个 catch 内、且在其之后。注释现在与代码一致。
非阻塞事项
1. 续写尝试的 maxOutputTokens 仍然没有重新 clamp。 自我提出以来未变,在本 head 上重新测量。buildAttemptContents() 会因已交付文本而增长 prompt,但 max_tokens 保持不变,而既有的 MAX_TOKENS 路径是逐轮重新 clamp 的:
本 PR 的传输层续写 既有的 MAX_TOKENS 恢复
req#0 max_tokens=32000 req#0 max_tokens=32000
req#1 max_tokens=32000 req#1 max_tokens=64000 <- 已提升
req#2 max_tokens=32000 req#2 max_tokens=64000 <- 逐轮重新 clamp
req#3 max_tokens=32000
我依然无法让它失败(我的已交付文本约 100 字符,clamp 从未生效),而且它更像是与「本 PR 声称对齐的那条路径」之间的不对称,而非已确认的 bug——但这是我上次审查中唯一既没修、也没被写下来的一条。建议在描述里补一句或开一个后续 issue,免得日后被重新发现。
2. !streamYieldedFunctionCall 这条排除在 OpenAI/DashScope 的网络路径上依然不可达。 我交付了一个完整的工具调用然后掐断 socket;转换器会连同这个流一起丢掉该工具调用,因此该标志保持为 false,续写照常进行——这是无害的,因为该工具调用根本没有到达调用方,也就不存在重复的可能。(另外,第 14 行覆盖的才是真正会发生的情况:续写自身的响应里携带工具调用时,已交付文本被正确并入,工具也确实执行了。)这条守卫正确且成本低;只是要知道它目前只有单测覆盖。
3. 在响应的任何一个字节之前发生的掐断,是被 SDK 自身的连接重试吸收的,而不是被这段代码处理——我通过「连续两次请求体完全相同、且只浮现一次错误」确认了这一点。这意味着「后续尝试什么都没交付」在现实中可达的形态就是思考阶段掐断,而这正是 8528cc0 修复的那个,因此该修复瞄准的是真实会发生的场景。
4. 模型重复自己尾部时,屏幕上的重复并没有被消除(历史被消除了)。在 head 上终端仍会显示 SEG-ALPHA. SEG-BRAVO. SEG-BRAVO. SEG-CHARLIE.,而历史中每个片段只有一份。这是固有的——字节已经交付出去,收不回来——而且 MAX_TOKENS 路径也是同样的表现。这不是本 PR 的缺陷;写在这里是为了避免有人把第 7/9 行读成「用户绝不会看到重复」。
5. Headless -o stream-json 会保留被普通 RETRY 丢弃的文本。 这是既有行为,而且我是在 merge-base 构建上验证的,而不是假设:一次仅思考的掐断在重放后,会把被丢弃的和被保留的推理内容一起写进同一条 assistant 消息。在同样的场景下,交互式 TUI 会正确丢弃。这与本 PR 无关,但意味着 headless 输出不能作为「用户看到了什么」的可靠证据。
6. --resume 的缺口与描述现在的写法完全一致。 恢复后那一轮在磁盘上的记录是 [assistant] STEP-THREE… ANSWER-COMPLETE-7896,用 --continue 恢复会把这同一条被截断的 turn 发往上游——STEP-ONE/STEP-TWO 已永久丢失。在 merge-base 上,被掐断的这一轮根本不会留下任何 assistant 记录,所以本 PR 严格更好,不构成回归。Known gap 小节与 #8094 的描述是准确的。
7. 描述的小问题——审查者测试计划中的数字已过期。 那里仍写着 268 passed、「九个用例」、「九个新测试中有七个失败」。当前实际是 277 通过、transport stream continuation (#7832) 块中有 15 个用例、回退后翻红 14 个。建议合并前一次性刷新,因为这一节正是审查者会照着跑的部分。
结论
核心修复是可靠的,在我能构造出的每一种失败形态(包括对抗性的)下都表现正确。上次审查之后修掉的两个问题都是真实的,都能在修复前的提交上端到端复现,并且各自都有专属测试钉住。从我这边看,可以合并;第 1 和第 7 条属于描述层面,一次编辑即可处理。
Verified locally with Claude Code (Opus 5, 1M context).
|
Thank you — that is the most thorough verification any of my PRs has had, and the two items you left open are both done. I re-measured everything rather than copying your numbers into the description. Item 7 — stale Reviewer Test Plan numbers. Fixed. Reproduced all three on my machine first: The three that stay green are the negative cases, as you said — Item 1 —
So the asymmetry is real and in the direction you described. I also could not make it bind, and I have stated in the description why it is hard to: the delivered text has to be large enough for On your non-blocking notes 2–6, I have nothing to add or correct — 4 and 5 in particular are worth having on the record, and note 3 is the more interesting one to me: it means the thinking-phase cut is the reachable shape of "a later attempt delivered nothing", so Head is unchanged at |
|
Released in v0.21.4. |
PR QwenLM#7896 (d7c0d4c) landed on main with a superset mechanism for the same mid-stream transport-cut recovery this PR implements. Resolve the semantic conflict in favor of main: - geminiChat.ts becomes identical to main: drop the pendingPartialTextParts stash and the history-based continuation block. Keeping both left a reachable duplicate continuation path that resumed from only the last attempt's partial while the accumulated prefix stayed unstitched. - geminiChat.test.ts keeps main's QwenLM#7832 suite and re-adds the PR tests that pass under the QwenLM#7896 mechanism; drop the two pins that contradict it (one-continuation-per-send, partial-turn-kept-on-failed-continuation). Main allows up to maxContinuationRetries chained continuations and deliberately persists no text-only partial on failure. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>





What this PR does
Makes a response that gets cut off by a socket-level close mid-stream recoverable, instead of failing the whole send. Two changes, both in
packages/core/src/core/geminiChat.ts.First, the existing replay path no longer treats reasoning as delivered output. Its guard used to be "has any chunk reached the caller", which a thinking model trips within seconds of starting, so replay was unavailable for precisely the long generations that need it. The guard now asks whether anything a replay could duplicate was delivered — non-blank answer text or a
functionCall. Thoughts and tool-call preparation metadata no longer count. This is consistent with the rate-limit and invalid-stream branches, which already re-send after reasoning has streamed, and with the UI's non-continuationRETRYhandler, which clears the thought buffer.Second, once answer text has been delivered, replaying is genuinely off the table — the user would see the first half of the answer twice — but failing the send is not the only remaining option. The response so far is already on screen, so the fix keeps it, asks the model to resume from it, and signals the UI with
{ type: StreamEventType.RETRY, isContinuation: true }so it appends to its text buffer rather than replacing it. This is the same shape theMAX_TOKENStruncation path already uses; the continuation reuses that machinery rather than introducing a parallel one. Budget is 3 attempts, since one long generation can be cut repeatedly by the same idle timeout.Three implementation points worth a reviewer's attention. The delivered text is mirrored into a local buffer as it is yielded rather than read back from history, because it cannot be read back:
processStreamResponsedeliberately does not persist a text-only partial turn when the stream throws, so at the catch site history holds nothing about what the user already saw. The synthetic turns that carry the delivered text and the resume instruction are built into the request only and never written toself.history, so they cannot leak into the JSONL transcript or a later/compress— unlike theMAX_TOKENSloop, which routes through history and cleans up afterwards withcoalesceRecoveryPairs. And on success the delivered prefix is merged back into the trailing model turn, because otherwise in-memory history would hold an answer that begins mid-sentence, which is visible on/compressand every later turn's context; replayed overlap is deduped with the existinggetRecoveryContinuationSuffix. This merge targetsthis.historyonly, not the recorder, so the JSONL transcript that--resumereads still starts the turn mid-sentence -- see the Known gap below.Cuts that delivered a
functionCallare excluded. Injecting a user turn between afunctionCalland itsfunctionResponseproduces a sequence providers reject — the same constraint theMAX_TOKENSrecovery loop enforces through itshasFunctionCallcheck — and the scheduler's repair path already covers that case.Why it's needed
Long generations behind a gateway that caps SSE connection lifetime die with
TypeError: terminated/UND_ERR_SOCKET, and no retry setting helps. DashScope closes at roughly 3–5 minutes, so in YOLO mode a long task is unrecoverable: the work already streamed to the screen is thrown away and the turn fails outright. Short requests never reach the timeout, so the failure is specific to exactly the requests that took the most time to produce.A note on the two fixes suggested in #7832, since the first one is misleading. Suggested fix 1 — classify
UND_ERR_SOCKETas retryable — is already implemented; the code is inRETRYABLE_STREAM_TRANSPORT_CODESinpackages/core/src/core/stream-transport-retry.ts. The second half of that suggestion, retry regardless of whether chunks were yielded, would actively introduce a bug, because the retry in question is a replay of the whole request and the user would see duplicated output. The error classification was never the blocker; thestreamYieldedChunkguard was. Suggested fix 2 — send the accumulated partial output as context and continue — is what this PR implements.Reviewer Test Plan
How to verify
The unit suite is the primary evidence, since the production trigger needs a real gateway. From the repo root:
Expected:
geminiChat.test.ts277 passed / 0 failed; typecheck and lint clean.The behaviour is covered by a new
describe('transport stream continuation (#7832)')block with fifteen cases: it continues from the delivered text instead of failing the send; stitches the delivered text into durable history; drops replayed overlap when the model repeats its own tail; survives repeated cuts and accumulates every delivered fragment; propagates once the continuation budget is exhausted (asserting exactly four calls — initial plus three); does not continue a cut that delivered afunctionCall; replays rather than continues when only a thought was delivered; replays rather than continues when the delivered text was blank; and drops a pending continuation when a fresh-restart retry takes over.Two things a reviewer may want to check specifically. One pre-existing test was changed by design:
does not retry retryable transport stream errors after yielding a chunkassertedtoHaveBeenCalledTimes(1)and zeroRETRYevents, which encoded the old policy. It is nowdoes not replay a transport stream error after yielding a chunkand asserts the new distinction — still no replay (zero non-continuationRETRYs, and the second request carries the delivered turn), but a continuation is expected. Separately, to confirm the tests exercise the new behaviour rather than restating it, revertgeminiChat.tswhile keeping the test file: fourteen tests fail, 263 pass. The three that stay green are the negative cases (functionCallcut, thought-only replay, blank-text replay), which is correct, since they assert that recovery does not happen.Reproducing the original bug against a live gateway requires a long generation behind DashScope and cannot be simulated locally; a maintainer with credentials can confirm the behavioural claim end-to-end if they want it.
Evidence (Before & After)
N/A — no TUI or user-visible surface changes. The user-facing effect is that an existing failure mode stops failing; there is no new UI. The
isContinuationrendering path this relies on is pre-existing and unchanged (packages/cli/src/ui/hooks/useGeminiStream.ts).Test output from the local run:
A wider sweep across
packages/core/src/coreplusretryErrorClassification.test.tsgives 2576 passed, 2 skipped, 2 failed. The two failures are insession-start-profiler.test.ts(writes bounded JSONL without sensitive fields,appends to an existing JSONL file) and are pre-existing and unrelated:git stash -uon cleanmainreproduces the identical pair.Tested on
Environment (optional)
Unit tests only, via
viteston Node 22, Windows 11. No sandbox or live provider involved.Risk & Scope
RETRYhandler inpackages/cli/src/ui/hooks/useGeminiStream.ts, which clears both the text and thought buffers, so a replay after a thought-only prefix renders correctly. On the continuation path the tradeoff is that the model is asked to resume from a suffix of its own output, so a model that ignores the instruction and restarts would produce a duplicated opening; the existinggetRecoveryContinuationSuffixdedup mitigates this, and it is the same tradeoff theMAX_TOKENSrecovery path already accepts.Known gap:
max_tokensis not re-clamped for continuation attemptsRaised by @wenshao and confirmed here by reading both paths.
buildAttemptContents()grows the prompt by the delivered text, butparams— which carriesmaxOutputTokens— is passed tomakeApiCallAndProcessStreamunchanged, so every continuation attempt reuses the value clamped before the first send. The pre-existingMAX_TOKENSrecovery loop re-clamps per iteration (geminiChat.ts, theRe-clamp maxOutputTokens for THIS iterationblock) precisely because its prompt grows the same way:Neither of us can make it fail: the delivered text has to be large enough for
prompt + max_tokensto cross the window, and the continuation budget caps the growth at three attempts. So this is a documented asymmetry with the path this PR mirrors, not a confirmed bug — recorded here rather than fixed so it is not rediscovered as a new finding. Fixing it means routing the continuation attempt through the sameclampOutputTokensToWindowcall the recovery loop uses.Known gap:
--resume/--continueNot fixed here, and confirmed by @wenshao with a real socket-cutting gateway.
prependTextToLastModelTurnwrites tothis.history, not tochatRecordingService, so a resumed session still sees the recovered turn starting mid-sentence./compressand later-turn context are fixed, because those read in-memory history.The naive fix -- recording the delivered delta as its own assistant turn when the continuation is scheduled -- was prototyped and measured, and it trades this defect for a worse one: on the fresh-restart-retry path the discarded prefix is already durable, so the resumed transcript shows the answer twice. That is the exact hazard the existing comment in
processStreamResponsewarns about.The correct fix is the stash-and-decide pattern this file already uses for
functionCallpartials (pendingPartialAssistantRecord): stash the delta, flush it on success alongsideprependTextToLastModelTurn, discard it inresetTransportContinuation. That is a real change rather than a one-liner, so it belongs in a follow-up rather than bolted onto this PR.Not a regression against
main: on merge-base the cut turn records no assistant record at all.中文说明
本 PR 未修复此项,且已由 @wenshao 用真实掐断 socket 的网关验证。
prependTextToLastModelTurn只写this.history,不写chatRecordingService,所以恢复的会话里该轮仍然从句子中间开始。/compress和后续轮次的上下文确实已修复,因为它们读的是内存中的历史。最直接的修法——在调度续写时把已交付增量作为独立 assistant turn 记录下来——已被做成原型并实测,结果是用一个缺陷换了个更糟的:在完整重发型重试路径上,被丢弃的前缀已经落盘,于是恢复后的转录会把答案显示两次。这正是
processStreamResponse中既有注释所警告的风险。正确的修法是采用该文件对
functionCall半截 turn 已经在用的「暂存再决定」模式(pendingPartialAssistantRecord):暂存增量,成功时与prependTextToLastModelTurn一并 flush,在resetTransportContinuation中丢弃。这是一个真正的改动而非一行代码,因此更适合作为后续 PR,而不是硬塞进本 PR。相对
main不构成回归:在 merge-base 上,被掐断的这一轮根本不会留下任何 assistant 记录。Also worth flagging, rather than leaving it to be discovered: this touches
packages/core/src/**, so it falls under the two-tier core gate inAGENTS.md. It is afixrather than arefactor, at 250 production lines in a single file, well under the 500-line hard block, so it should fall to Tier 2.Linked Issues
Fixes #7832
中文说明
这个 PR 做了什么
让流式响应在传输层中途被掐断时可以恢复,而不是整次发送直接失败。两处改动,都在
packages/core/src/core/geminiChat.ts。第一处,已有的「重放」路径不再把思考内容当作已交付的输出。它原本的门控条件是「是否已有任何 chunk 到达调用方」,而思考模型在开始后几秒内就会触发这个条件,于是重放对最需要它的长生成恰好不可用。现在门控问的是「是否交付了重放会重复的东西」——非空的正文,或者一个
functionCall。思考内容和工具调用的准备元数据不再计入。这与限流分支、无效流分支的行为一致(它们本来就在思考内容流完之后重发),也与 UI 的非续写RETRY处理器一致(它会清空思考缓冲)。第二处,一旦正文已经交付,重放就确实不能用了——用户会看到答案的前半段出现两次——但整次发送失败并非唯一剩下的选择。目前的响应已经在屏幕上,所以修复的做法是保留它、让模型从它继续,并向 UI 发出
{ type: StreamEventType.RETRY, isContinuation: true },使 UI 在已有文本后追加而不是替换。这与MAX_TOKENS截断路径已有的形态相同;续写复用了那套机制,而不是另起一套。预算为 3 次,因为同一次长生成可能被同一个空闲超时反复掐断。有三处实现细节值得审查者注意。已交付文本是在 yield 时镜像进一个本地缓冲区的,而不是从历史中读回——因为读不回来:
processStreamResponse在流抛错时故意不持久化纯文本的半截 turn,所以在 catch 处,历史里没有任何关于用户已看到内容的记录。承载已交付文本和续写指令的合成 turn 只构建进请求,绝不写入self.history,因此不会漏进 JSONL 转录或后续的/compress——这与MAX_TOKENS循环不同,后者要经由历史再用coalesceRecoveryPairs清理。成功后已交付前缀会被并回尾部的 model turn,否则内存中的历史里会存下一个从句子中间开始的答案,这在/compress和之后每一轮的上下文里都能看到;重叠部分用现有的getRecoveryContinuationSuffix去重。这次并回只作用于this.history,不涉及 recorder,因此--resume读取的 JSONL 转录中该轮仍然从句子中间开始——详见下方的 Known gap。交付了
functionCall的中断被排除在外。在functionCall和它的functionResponse之间插入用户 turn 会产生 provider 拒绝的序列——这与MAX_TOKENS恢复循环通过hasFunctionCall检查所遵守的约束相同——而且调度器的修复路径已经覆盖了这种情况。为什么需要
在限制 SSE 连接生命周期的网关后面,长生成会以
TypeError: terminated/UND_ERR_SOCKET失败,任何重试设置都无济于事。DashScope 大约在 3–5 分钟关闭连接,所以在 YOLO 模式下长任务不可恢复:已经流到屏幕上的成果被丢弃,整轮直接失败。短请求永远碰不到这个超时,所以这个故障恰好只发生在最耗时的请求上。关于 #7832 里提出的两个修复方案,需要说明一下,因为第一个有误导性。方案 1——把
UND_ERR_SOCKET归类为可重试——已经实现了,代码就在packages/core/src/core/stream-transport-retry.ts的RETRYABLE_STREAM_TRANSPORT_CODES里。而该方案的后半句「不论是否已 yield chunk 都重试」会主动引入 bug,因为这里的重试是整个请求的重放,用户会看到重复输出。错误分类从来不是堵点,streamYieldedChunk门控才是。方案 2——把累积的部分输出作为上下文发送并继续——正是本 PR 实现的内容。审查者测试计划
如何验证
单元测试是主要证据,因为生产环境的触发条件需要真实网关。在仓库根目录:
预期:
geminiChat.test.ts277 通过 / 0 失败;类型检查与 lint 干净。行为由新增的
describe('transport stream continuation (#7832)')块覆盖,共十五个用例:从已交付文本继续而非失败;将已交付文本缝合进持久化历史;模型重复自己尾部时丢弃重叠;在反复中断下存活并累积每一个已交付片段;续写预算耗尽后向上抛出(断言恰好四次调用——首次加三次续写);不对交付了functionCall的中断做续写;只交付了思考内容时走重放而非续写;已交付文本为空白时走重放而非续写;有新的完整重发型重试接管时丢弃待处理的续写状态。有两点审查者可能想专门确认。有一个既有测试是按设计修改的:
does not retry retryable transport stream errors after yielding a chunk原本断言toHaveBeenCalledTimes(1)和零个RETRY事件,编码的是旧策略。现在它是does not replay a transport stream error after yielding a chunk,断言新的区分——仍然不重放(零个非续写RETRY,且第二次请求携带已交付的 turn),但预期会有一次续写。另外,为确认这些测试是在检验新行为而不是复述新行为:保留测试文件、只回退geminiChat.ts,14 个失败、263 个通过。仍然通过的三个是负向用例(functionCall中断、仅思考内容时重放、空白文本时重放),这是正确的,因为它们断言的正是「不应发生恢复」。针对真实网关复现原始 bug 需要在 DashScope 后面跑一次长生成,本地无法模拟;有凭据的维护者如果需要,可以端到端确认行为主张。
证据(改动前后)
N/A——没有 TUI 或用户可见界面的改动。用户可感知的效果是一个既有的故障模式不再发生,没有新增 UI。本改动依赖的
isContinuation渲染路径是既有的且未被修改(packages/cli/src/ui/hooks/useGeminiStream.ts)。本地运行的测试输出:
在
packages/core/src/core加retryErrorClassification.test.ts的更大范围扫描结果为 2576 通过、2 跳过、2 失败。这 2 个失败在session-start-profiler.test.ts(writes bounded JSONL without sensitive fields、appends to an existing JSONL file),是预先存在且无关的:在干净的main上执行git stash -u可复现完全相同的这两个失败。测试环境
运行环境(可选)
仅单元测试,通过
vitest在 Node 22、Windows 11 上运行。不涉及沙箱或真实 provider。风险与范围
packages/cli/src/ui/hooks/useGeminiStream.ts中的非续写RETRY处理器,它会同时清空文本缓冲和思考缓冲,所以思考内容之后的重放渲染是正确的。在续写路径上,取舍在于模型被要求从自己输出的一个后缀继续,因此如果模型忽略指令重新开始,就会产生重复的开头;现有的getRecoveryContinuationSuffix去重可以缓解,而且这与MAX_TOKENS恢复路径已经接受的取舍相同。action_required,等待维护者批准 fork PR——所以上面的测试、lint 和类型检查结果仅为本地结果。针对真实 DashScope 网关的复现未覆盖,因为 3–5 分钟的连接上限无法在单元测试中模拟。macOS 和 Linux 未在本地测试。对以不同方式掐断长流的其他 provider 未做专门验证,不过本修复依据的是传输错误码而非 provider。stream-transport-retry.ts是故意放在包 barrel 之外的。新增的maxContinuationRetries是内部常量,不对用户开放配置。另外主动说明一点,而不是留着被发现:本改动触及
packages/core/src/**,因此落在AGENTS.md的核心模块两级门禁之内。它的类型是fix而非refactor,单文件 250 行生产代码,远低于 500 行硬阻断线,所以应当归入 Tier 2。关联 Issue
Fixes #7832