fix(core): record the delivered prefix when a transport cut is continued - #8624
Conversation
After a socket cut mid-response, the continuation attempt resumes from the text the user already saw. `prependTextToLastModelTurn` merges that prefix back into the trailing model turn, but it writes `this.history` and nothing else. The JSONL transcript keeps only the resumed remainder, so `--resume` and `--continue` rehydrate a turn that starts mid-sentence while the live session shows a coherent answer. Merge the prefix into the assistant record as it is built, reusing the same overlap dedup `prependTextToLastModelTurn` uses, so one turn goes in and one matching turn lands on disk. The merge belongs at the record build, not next to the history merge: the record is appended from inside `processStreamResponse`, before the outer send loop regains control, and `appendRecord` is append-only — a second record written afterwards would sit behind the remainder and resume would read the halves out of order. It is gated on success. When the attempt fails, the record has to keep matching the remainder-only partial that survives in history, and a fresh-restart retry discards the prefix from history via `resetTransportContinuation`. The prefix rides as a per-attempt argument rather than instance state, so no stash can dangle into a later turn. Refs: QwenLM#8094 Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
…timers The tool-call-cut test drained its stream with `collectStreamWithFakeTimers`, which returns the collecting promise only after advancing timers. That send rejects during the advance, so the rejection sat unhandled for a tick — vitest caught it as an unhandled error and exited 1 with every test still passing. Attach the assertion first, then advance, matching the shape `expectStreamExhaustion` in this file already uses for the same reason. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Not reviewed: reverse audit — stopped before round 5 by the review time budget.
— qwen3.8-max via Qwen Code /review (v0.21.6)
| // `pendingPartialAssistantRecord` path below) — the prefix belongs to an | ||
| // attempt that did not survive, and a fresh-restart retry discards it | ||
| // from history via `resetTransportContinuation`. | ||
| const recordedContentText = |
There was a problem hiding this comment.
[Critical] R1-1: The record-side prefix merge computes getRecoveryContinuationSuffix against contentText — the text parts joined and .trim()-ed (~line 4674) — while the history-side merge in prependTextToLastModelTurn (~line 4997) dedupes against the raw, untrimmed first plain-text part. The two layers this PR commits to holding identical compute the merged turn text from different operands whenever the continuation remainder has leading/trailing whitespace.
Failure scenario: Transport cut lands before a space byte (a normal token boundary); prefix "The result is", continuation remainder " 42.". History merges the raw part → "The result is 42."; the record trims the merge input → "The result is42." (fused words). Probe-observed on the unmodified PR, including a dedup-divergence variant: prefix "The grand total", remainder " total sum is 9." → record "The grand totaltotal sum is 9." (6-byte overlap " total" is significant only untrimmed) vs clean history. --resume rehydrates the corrupted turn; the four new tests put boundary spaces on the prefix side only, so none reaches this input.
Suggested fix: Dedup the record merge against the same untrimmed input the history merge uses (the raw join of consolidatedHistoryParts text parts), or route both merges through one shared helper so they cannot diverge; add a regression test whose continuation chunk starts with whitespace and asserts record == history byte-for-byte.
— qwen3.8-max via Qwen Code /review (v0.21.6)
| // `pendingPartialAssistantRecord` path below) — the prefix belongs to an | ||
| // attempt that did not survive, and a fresh-restart retry discards it | ||
| // from history via `resetTransportContinuation`. | ||
| const recordedContentText = |
There was a problem hiding this comment.
[Critical] R2-2: The merged record is written inside processStreamResponse before the outer send loop's prependTextToLastModelTurn runs. On a send that is both a tool-result continuation (deferredFinishReason synthetic chunk yielded after record-write + history-push, ~4952) and a successful transport continuation, there is a one-chunk suspension window between record write and history merge. Abandoning iteration at that chunk leaves the merged record persisted while history keeps the remainder-only turn. Pre-diff the record in that window was remainder-only and matched history — the diff introduces the window (verified against the base tree).
Failure scenario: Probe-observed on the unmodified PR: for-await broken at the CHUNK carrying the synthetic deferred finishReason — the exact model of Turn.run's abort-return (turn.ts:497-501 checks signal.aborted at the top of every iteration; Esc drives it via useGeminiStream.ts cancelOngoingRequest) — yields record "Analysis: the file contains the bug." vs history "contains the bug.". The divergence is persistent once hit (append-only JSONL, nothing reconciles) and defeats the PR's stated invariant in the opposite direction; trigger frequency (tool-result continuation ∧ successful transport continuation ∧ Esc inside the final-event window) is situational, but the mechanism is deterministic and high confidence.
Suggested fix: Compute the merged text where both layers are written from one merge that cannot be skipped independently — e.g. perform the history merge inside processStreamResponse right after history.push and before the synthetic-chunk yield (validated by probe: flips the abandon arm green with the control still green; the outer merge re-runs idempotently), or defer the record append until after the history merge like pendingPartialAssistantRecord.
— qwen3.8-max via Qwen Code /review (v0.21.6)
| // `pendingPartialAssistantRecord` path below) — the prefix belongs to an | ||
| // attempt that did not survive, and a fresh-restart retry discards it | ||
| // from history via `resetTransportContinuation`. | ||
| const recordedContentText = |
There was a problem hiding this comment.
[Suggestion] R1-2: The merge composition prefix + getRecoveryContinuationSuffix(prefix, remainder) is hand-written at three sites: foldTransportAttemptText (~2660), prependTextToLastModelTurn (~4997), and the new recordedContentText merge (~4827). Nothing forces the twin expressions to keep agreeing. The file already treats this drift risk as real one block down: the willPersistToHistory gates share a single binding (~4898-4904) specifically so the JSONL recording cannot silently desync from in-memory history.
Failure scenario: Any future change to the merge semantics (dedup rule, whitespace guard, floor adjustment) applied on one side only silently reintroduces the transcript/history desync this PR exists to remove — the synchronized-edit invariant is encoded nowhere structurally. This is no longer hypothetical: R1-1's probe shows the twin expressions already diverge on the same turn.
Suggested fix: Extract one shared helper, e.g. mergeDeliveredPrefix(prefix, remainder) = prefix + getRecoveryContinuationSuffix(prefix, remainder) (and, per R1-1, over the same untrimmed operand), called from both prependTextToLastModelTurn and the record path — mirroring the shared-binding pattern already used for the persistence gates.
— qwen3.8-max via Qwen Code /review (v0.21.6)
Addresses review findings R1-1, R2-2 and R1-2 on QwenLM#8624. The first version merged the prefix into the JSONL record only, at the point the record was built, and left history to the outer send loop's `prependTextToLastModelTurn`. Two expressions, two write times — and both diverged. R1-1: the record deduped against `contentText`, which is trimmed, while history deduped against the raw part. A cut landing on a token boundary recorded "The result is" + " 42." as "The result is42.", and a 6-byte overlap that is significant only untrimmed recorded "The grand totaltotal sum is 9.". R2-2: the record was appended before the history push, and a tool-result continuation yields a deferred finishReason chunk after it. A consumer abandoning iteration there — an abort inside `Turn.run` — left a merged record against a remainder-only history, permanently, since the JSONL is append-only. Fold the prefix into `consolidatedHistoryParts` instead, once, after stream validation and before either write. The record and the history push are then built from the same parts, so they cannot disagree about whitespace, dedup, or timing. The outer merge is gone, along with `prependTextToLastModelTurn`, and the merge itself is now one shared helper (R1-2) rather than a hand-written expression per site. Not re-run in the outer loop on purpose: the dedup helper only strips a replayed prefix that clears its significance floor, so a short prefix would survive a second pass and be doubled. Refs: QwenLM#8094 Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
The new helper was inserted between that JSDoc block and the function it documents, so the block silently reattached to the wrong function. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
|
Thanks — R1-1 and R2-2 were both real, and both reproduced. They turned out to be the same weakness seen twice, so I've reworked the fix rather than patching each one. Root cause of bothThe original diff merged the prefix into the JSONL record only, at the point the record was built, and left history to the outer send loop's So the prefix is now folded into R1-1 — confirmed
The second is the worse half: R2-2 — confirmedThe record was appended inside One correction on the suggested fix. Re-running the outer merge idempotently isn't safe. R1-2 — takenExtracted What changed
Three new tests, each red against the previous head:
These assert
The branch is also merged up to current Not addressedOn an unretryable failure after a continuation, the delivered text is in neither history nor the transcript. Both layers agree, so it isn't this bug — it's merge-base behavior and I've left it alone rather than widen the diff. |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
— qwen3.8-max via Qwen Code /review (v0.21.7)
| contentText = consolidatedHistoryParts | ||
| .filter((part) => part.text) | ||
| .map((part) => part.text) | ||
| .join('') | ||
| .trim(); |
There was a problem hiding this comment.
[Suggestion] R3-1: This adds a third copy of the contentText derivation expression inside processStreamResponse — identical copies already exist at ~line 4700 (initial binding) and ~line 4750 (XML-tool-call recompute). The comment block above this merge invokes exactly this drift hazard as load-bearing ("Computing the record's text and the history's text from separate expressions lets them drift"), while the diff adds a third copy of one of the drifting expressions.
Failure scenario: any future change to the derivation rule (switching the filter predicate, changing the join) applied at one or two of the three sites leaves the JSONL record's text diverging from the pushed parts — the record/history desync class this PR exists to remove.
Suggested fix — extract one helper used at all three sites:
const deriveContentText = (parts: Part[]) =>
parts.filter((part) => part.text).map((part) => part.text).join('').trim();— qwen3.8-max via Qwen Code /review (v0.21.7)
| function mergeDeliveredPrefix( | ||
| deliveredText: string, | ||
| continuationText: string, | ||
| ): string { |
There was a problem hiding this comment.
[Suggestion] R3-2: This helper's doc claims it is "The single definition of 'merged turn text' for the transport-continuation path", but foldTransportAttemptText (~2681-2687) still inlines the byte-identical merge arithmetic: transportContinuationText += getRecoveryContinuationSuffix(transportContinuationText, transportAttemptText) is exactly transportContinuationText = mergeDeliveredPrefix(transportContinuationText, transportAttemptText). The buffer that fold builds is assigned to transportContinuationPrefix at ~2956 — it is the very prefix this helper later merges into the persisted turn.
Failure scenario: if the merge/dedup rule ever changes (normalisation, the significance floor in getRecoveryContinuationSuffix), the accumulated prefix and the persisted merge diverge — doubled or dropped text on continuation.
Suggested fix in foldTransportAttemptText:
transportContinuationText = mergeDeliveredPrefix(
transportContinuationText,
transportAttemptText,
);— qwen3.8-max via Qwen Code /review (v0.21.7)
| const textIndex = consolidatedHistoryParts.findIndex(isPlainTextPart); | ||
| if (textIndex < 0) { | ||
| // Continuation returned no text of its own (e.g. only a functionCall). |
There was a problem hiding this comment.
[Suggestion] R3-3: The textIndex < 0 (unshift) branch of this merge has no test at the JSONL-record layer. All seven new recorder-based tests feed the continuation a text remainder, and the one pre-existing test reaching this branch ('inserts the delivered text when the continuation has no text part') uses a plain chat with no recorder and asserts only history. In this branch the record's text comes solely from the contentText recompute above the record build.
Failure scenario (probe-verified): moving that recompute inside the else — so it is skipped exactly when textIndex < 0 — leaves all 307 tests in this file green while the record silently drops the delivered prefix (the record gate still passes via thoughtContentPart/hasToolCall) even though history keeps it: the exact record/history desync this PR removes, surviving in the sibling branch. A probe test asserting the recorded prefix flips the mutant: expected undefined to be 'delivered prefix'.
Suggested fix: add one chatWithRecorder test whose continuation completes with only a functionCall (and/or only a thought part), asserting the recorded message's text part equals the delivered prefix and matches history.
— qwen3.8-max via Qwen Code /review (v0.21.7)
| if (streamError === null && transportContinuationPrefix) { | ||
| const textIndex = consolidatedHistoryParts.findIndex(isPlainTextPart); |
There was a problem hiding this comment.
[Suggestion] R3-4: The merge — including its heuristic dedup decision — runs silently: this block contains no debugLogger call, while the file codifies the investigator-anchor standard for comparable recovery machinery (the [PARTIAL_PUSH] comment: without the anchor line, an investigator "sees the downstream symptom but has no anchor for when/why"). The continuation path logs 'Transport stream continuation scheduled' (with deliveredChars) when the continuation is scheduled, but nothing records that the merge ran, which branch (inserted vs deduped) it took, or how many chars the dedup consumed.
Failure scenario: on a cut-prone gateway the dedup decides wrong (the contained-prefix path strips a legitimately repeated block, or a sub-floor replayed tail gets doubled); days later a user reports corrupted --resume output. The investigator has no log line proving the merge ran or which branch it took, and the JSONL is append-only, so the live state is gone.
Suggested fix — one trace line after the merge:
debugLogger.info(
'[TRANSPORT_CONTINUATION] Merged delivered prefix into continuation turn',
{
prefixChars: transportContinuationPrefix.length,
mergedChars: contentText.length,
mode: textIndex < 0 ? 'inserted' : 'deduped',
},
);— qwen3.8-max via Qwen Code /review (v0.21.7)
|
@qwen-code /triage |
|
Sandboxed verification: Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. 中文 — 判定：
|
|
Thanks for the PR! Template: present in substance, not in shape — the body uses its own headings ( Problem: observed, not theoretical. #8094 is open with real-gateway measurements (socket destroyed mid-SSE stream), the root cause is pinned to the exact spot where Direction: aligned — durable-transcript fidelity on Size: core path hit ( Approach: scope feels right. One merge point inside Risk: high-risk path matched ( Moving on to code review. 🔍 中文说明感谢贡献！ 模板：实质内容齐全，只是没用模板标题——正文用自己的标题（ 问题：已观测到的 bug，非理论问题。#8094 仍然 open，有真实网关（SSE 流中途断开 socket）的测量数据，根因定位到 方向：对齐—— 规模：触及核心路径（ 方案：范围合理。合并点收敛到 风险：命中高风险路径（ 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewIndependent baseline first: from the title and issue alone, I'd have merged the delivered prefix into the response parts inside How the two round-1 Criticals are settled in the current head:
I verified the load-bearing claims against the base tree, not just the diff: the removed Non-blocking nits carrying over from round 3 (none gate this): Testing — this PR's own CI, quoted via the API (I do not run PR code)All 72 check runs on the reviewed commit are completed, zero failures. The ubuntu job is the full profile (build, lint, typecheck, unit suite, required integration gate), so the green is substantive; the skipped jobs only run in the merge queue by design on
Not independently verified, and saying so plainly: the author's RED/GREEN and mutation-kill numbers are self-reported — CI independently runs the suite green, but nobody besides the author has run the mutants; and the tests stub the content generator, so no real gateway destroys a socket in this evidence (the PR's own Limits section says the same). Sandboxed verification would settle the residue: 中文说明代码审查先说独立基线：只看标题和 issue，我的方案也是——把已交付前缀在 round-1 两个 Critical 在当前 head 的解决方式：
关键论断均对照基线树验证过（而非只看 diff）：被删的 承接 round-3 的非阻塞建议（均不影响合并）： 测试——本 PR 自己的 CI，经 API 读取（审查不运行 PR 代码）被审 commit 上 72 个 check run 全部完成，零失败。ubuntu 任务为完整档（构建、lint、typecheck、单测、必需集成门禁），绿色是实质性的；被跳过的任务按 未独立验证的部分，直说：作者的 RED/GREEN 与变异杀灭数字为自述——CI 独立跑绿了测试套件，但变异体除作者外无人跑过；且测试用桩内容生成器，证据中没有真实网关断连（PR 的 Limits 一节自己也这么说）。沙箱验证可以补齐残留缺口： — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 4/5 — clean across every stage; both round-1 Criticals are structurally eliminated and pinned by regression tests, with only non-blocking nits left. Honest read: this is what a good fix in this file looks like. The problem is real and was measured on a live gateway; the fix is the minimal correct shape — one merge point before either durable write, one shared helper encoding the "record and history cannot drift" invariant, success-only, per-attempt lifetime, dead code deleted rather than left dormant. The comment block at the merge site documents both failure modes it exists to prevent, which is the kind of thing a maintainer six months from now thanks the author for. The round-1 review found two real desync bugs; the follow-up commit doesn't patch around them — it removes the class of bug by construction, and the new tests reproduce the exact probe scenarios byte-for-byte. CI is fully green on the reviewed commit (72/72 checks completed, zero failures; PENDING = 0). Reservations, named: the round-3 suggestions are still worth taking (recorder-layer test for the Process note for @wenshao: your round-1 changes-requested review is still standing on this PR. The fix commit ( Approving, pinned to the reviewed commit. 中文说明置信度：4/5 —— 各阶段均干净；round-1 的两个 Critical 已从结构上消除并由回归测试钉住，只剩非阻塞建议。 直说：这是这个文件里一个优秀修复的样子。问题真实存在，有真实网关上的测量；修复是最小的正确形态——两处持久写入之前的单一合并点、一个编码"记录与 history 不得漂移"不变量的共享 helper、仅成功路径、按次生命周期、死代码删除而非留置。合并点处的注释记录了它要防的两类失效，这是六个月后的维护者会感谢作者的东西。round-1 发现了两个真实的失同步 bug；后续 commit 没有绕着打补丁，而是从构造上消除了这一类 bug，新测试逐字节复现了探针场景。被审 commit 上 CI 全绿（72/72 check 完成，零失败；PENDING = 0）。 保留意见，明说：round-3 的建议仍然值得采纳（ 给 @wenshao 的流程提示：你 round-1 的 changes-requested 仍挂在 PR 上。修复 commit（ 批准，锚定到被审 commit。 — Qwen Code · qwen3.8-max Reviewed at |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
Maintainer deep verification (local run by @wenshao)Verdict: merge-ready — 72/72 scripted assertions passed, 0 failed. Verified head 中文摘要（点击展开）结论：可合并——72/72 条脚本化断言全部通过，0 失败。PR head 落后当前 main 30 个提交且 main 期间大改了同两个文件，因此额外验证了合并本身：试验性合并无冲突且结果恰为本 PR 的 diff，合并树上
Central claim (verified)After a socket cut mid-response, the continuation path (#7832) merged the delivered prefix into in-memory history only; the durable JSONL record kept just the resumed remainder, so A/B load-bearing proofSame PR test file against base source (control) vs head source; the control must go red on exactly the five new cross-layer tests — a scripted comparison, not an impression:
Wire oracle — real dist, real JSONL, real resume projection
Raw durable JSONL from the run (bottom of the capture): head
Mutation matrix (vacuity check)Five single-point mutants against head source, each driven against the test that should pin it — 5/5 killed, including two positive controls I added (m4 word-fusion via trimmed remainder; m5
Corrections to the PR description (drift, not code issues)
FindingsNone blocking. Two observations, both pre-existing and unchanged by this PR:
Not covered
MethodologymacOS, Node v22.22.2. No container runtime available, so instead of the sandboxed lane: full-diff audit first (2 files, pure string/record logic + tests; no manifests/scripts/network/fs/process execution), then isolated |
|
Released in v0.21.8. |
3 participants
Refs: #8094
The bug
After a socket cut mid-response, the transport-continuation path asks the model to resume from the text the user already saw. On success,
prependTextToLastModelTurnmerges that delivered prefix back into the trailing model turn — but it writesthis.historyand nothing else:https://github.com/QwenLM/qwen-code/blob/main/packages/core/src/core/geminiChat.ts#L2731-L2733
So
/compressand every later turn read a coherent answer, while the JSONL transcript that--resume/--continuereads keeps only the resumed remainder. The recovered turn starts mid-sentence on resume.Not a regression: before #7896 the cut turn recorded no assistant record at all.
The fix
Merge the prefix into the assistant record as it is built, reusing the same overlap dedup
prependTextToLastModelTurnalready uses. One turn in, one matching turn on disk.Why at the record build and not next to the history merge. The record is appended from inside
processStreamResponse, before the outer send loop regains control at:2731, andappendRecordis append-only. A second record written "alongsideprependTextToLastModelTurn" would land after the remainder, and resume maps records toContent[]in file order — so the transcript would read[remainder][prefix], halves reversed. That rules out the record-a-second-turn shape.Why success-only. On
streamError !== nullthe record must keep matching the remainder-only partial that survives in history (thependingPartialAssistantRecordpath), and a fresh-restart retry discards the prefix from history viaresetTransportContinuation. Merging unconditionally would put the delivered text in the transcript but not in history — the same desync, pointing the other way.Why a parameter, not a field. The prefix rides as a per-attempt argument captured by value, so it dies with the attempt. There is no stash that can dangle into a later turn — which matters, because
NO_TOOL_RESULT_PROGRESS_MAX_TOKENSbreaks out of the loop without reaching either the merge or the reset, and an instance-field stash flushed in the generator'sfinallywould write a spurious prefix record there.resetTransportContinuationneeds no change: it already clearstransportContinuationPrefix, so the next attempt is invoked withundefined.Proof
packages/core, Node 22.22.3, fully mocked (fake timers, no network).RED — new tests against unmodified
geminiChat.ts:The two guard tests (discard-on-reset, and the tool-call-cut case) pass on unmodified source — they are tripwires, not the red.
GREEN
Mutation checks — each mutant killed by exactly the test that should catch it:
streamError === null &&(merge on failure too)keeps the record remainder-only when the continuation itself is cut after a tool callprefix + contentText(naive concat, no overlap dedup)dedupes replayed overlap in the recorded turn tooundefined)Blast radius, by symbol not directory.
processStreamResponsehas one production caller plus a direct call ingoal-turn-integration.test.ts;makeApiCallAndProcessStreamhas three call sites (:2696changed,:3225and:3899pass nothing →undefined→ byte-identical behavior). Every file matching those symbols orrecordAssistantTurn:Full suite, against restored upstream sources vs this branch:
tsc --noEmitexit 0 on both.Demo
N/A — no user-facing change in the live session. The change is to the durable transcript, and the observable difference is the recorded turn text, asserted directly in the tests above.
Limits
sendMessageStream→processStreamResponsepath with a stubbed content generator; it does not run against a gateway that really destroys the TCP socket. The original measurements in Transport-continuation recovery leaves the resumed transcript starting mid-sentence #8094 are @wenshao's from that setup.AI-assisted: investigated, implemented and proven with Claude Opus 5.