fix(core): retry mid-stream transport failures as continuations - #7876
fix(core): retry mid-stream transport failures as continuations#7876he-yufeng wants to merge 12 commits into
Conversation
makeMergedSessionUpdateEvent and mergeToolCallEvent rebuilt the event envelope from scratch and dropped top-level promptId and originatorClientId, even though the source chunks carried them. Live subscribers see those fields on every chunk, but a client resuming from a compacted replay lost them: prompt correlation broke and echo suppression (event.originatorClientId === clientId) silently stopped applying to merged events. Track the latest promptId / originatorClientId on text and thought slots (mirroring lastMeta / lastEnvelopeMeta) and copy them onto merged events; folded tool calls take them from the incoming event with the existing event as fallback, matching the id merge. Events whose sources carried no attribution still emit none. Signed-off-by: Yufeng He <40085740+he-yufeng@users.noreply.github.com>
The stream transport retry only fires before the first chunk reaches callers. For thinking models the whole think -> tool loop runs as one SSE stream that can outlive the gateway's idle window, so a socket close mid-stream (UND_ERR_SOCKET after minutes of output) propagated and discarded everything. In headless mode the partial text is already printed and cannot be re-rendered either, which is the QwenLM#7832 report. When chunks have already flowed and the failed attempt left a text or thought partial (no functionCall, which cannot take an injected user message), the retry now runs as a continuation instead: the partial turn stays in history and on the transcript, a recovery user message shows the model its own suffix, and the pair coalesces on success, the same flow the MAX_TOKENS output recovery already uses. The UI keeps its buffer through the isContinuation retry event, so the continuation appends where the stream stopped. One continuation per send; if it fails the partial turn survives and the error propagates, the same end state as an unretryable mid-stream break. Signed-off-by: Yufeng He <40085740+he-yufeng@users.noreply.github.com>
|
Qwen precheck requires maintainer approval before automated triage/review. Head SHA: Reason:
A maintainer with write access can inspect the PR and manually request a run with |
qqqys
left a comment
qqqys
left a comment
There was a problem hiding this comment.
A second transport failure inside the new continuation path corrupts the persisted conversation history. The inline trace below shows the exact state transition and a bounded fix direction.
| self.coalesceRecoveryPairs(1); | ||
| lastError = null; | ||
| break; | ||
| } catch (continuationError) { |
There was a problem hiding this comment.
[Critical] A continuation that itself drops after yielding text leaves consecutive model turns in history.
Trigger: the initial stream yields text A then a retryable socket error; the first continuation yields text B then the same error; the second continuation succeeds. processStreamResponse stashes B in pendingPartialTextParts. This catch removes only recoveryUserContent, then falls through to the outer transport-retry condition. That condition still uses the original retryable classification and streamYieldedChunk === true, sees the new B stash, and takes the remaining second retry.
The second retry pushes B as a new model turn after the already-persisted A. On success, coalesceRecoveryPairs(1) merges only B + C, leaving history as [... user, model(A), model(B+C)]. A later request therefore receives two adjacent model contents instead of the required alternating conversation, causing malformed context / provider rejection after exactly the recovery path this PR adds.
Do not fall through into another standalone continuation after continuationError; either bound continuation recovery to one attempt, or merge the new partial into the existing preceding model turn before retrying and coalesce the whole chain. Add a regression where both the initial stream and first continuation yield text before UND_ERR_SOCKET, then assert history has one combined model turn and no recovery user turn.
There was a problem hiding this comment.
Thanks for the careful trace. I walked the control flow again on a6e7450, and the second continuation cannot fire there: the continuation attempt is wrapped in its own try/catch, so a transport error inside it is caught inline (that is where the recovery user message gets popped), lastError is replaced, and control moves on to the log-only "not taken" branch and then the catch tail, which ends with throw lastError. The continuation branch sits before the attempt in program order and is never re-entered within the same send, so the B stash never gets consumed by a second retry.
Pinned the exact chain you describe as a regression test in 064efac: attempt 1 yields "text A" then UND_ERR_SOCKET, continuation 1 yields "text B" then UND_ERR_SOCKET, and a third stream stands ready to succeed. The test asserts generateContentStream is called exactly twice, exactly one RETRY event (isContinuation) is emitted, the error propagates, and history is ['user', 'model'] holding only "text A ". No model(B), no adjacent model turns, no recovery user turn survives.
The end state after a failed continuation (partial A kept in history and on the transcript) is deliberate: it matches the existing "kept if break is unretryable" behavior of tool-call partials, and roles still alternate on the next send. Happy to adjust if you see a path I missed, but the [user, model(A), model(B+C)] shape is not reachable in the committed flow.
…ures Signed-off-by: Yufeng He <40085740+he-yufeng@users.noreply.github.com>
# Conflicts: # packages/acp-bridge/src/compactionEngine.ts # packages/core/src/core/geminiChat.test.ts
|
|
|
@qwen-code /resolve |
…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.
|
Qwen Code resolved the merge conflicts and pushed the branch update. Merge summary: PR #7876 ← mainRoot causeMain's #7938 (commit Textual or semanticSemantic. In // 1. #7938: thinking-only -> replay from zero
if (isRetryableStreamTransportError && !streamYieldedContentChunk && budget) { …; continue; }
// 2. this PR: text/thought partial -> continuation
if (isRetryableStreamTransportError && streamYieldedChunk && pendingPartialTextParts !== null && budget) { … }
// 3. otherwise propagate (keeps main's discriminator)
retryDecision: streamYieldedContentChunk ? 'skipped_after_content' : 'exhausted'In the test file both sides rewrote the same pre-existing test. I kept the PR's What is load-bearing
What I could not verify (needs follow-up)No build/tests run here. Main's auto-merged, NON-conflicted test 中文说明根因： main 的 #7938（ 语义冲突： 关键依赖： 从零重放块必须排在 continuation 块之前（纯思考失败同时满足两个门，靠前者 需后续： 未跑构建/测试。main 未冲突测试 |
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.
|
@qwen-code /resolve |
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>
|
Qwen Code resolved the merge conflicts and pushed the branch update. Root cause
Semantic, not textualThe auto-merged tree kept main's local-buffer continuation ( What is load-bearing
Could not verifyNo build/tests run (this command resolves conflicts only). The three re-added tests were hand-traced against main's mechanism and should pass; CI is the oracle. Note for reviewers: after this merge the PR's net contribution over main is tests only (+244 test lines plus the auto-merged acp-bridge test) — the runtime fix is now provided entirely by #7896; maintainers may prefer closing it. 中文说明根因: 语义冲突:自动合并同时保留了 main 的局部缓冲续写(独立预算 关键点: 未能验证:本流程不跑构建/测试;三个加回的测试经手工推演应能通过,以 CI 为准。合并后本 PR 相对 main 只剩测试增量,运行时修复完全由 #7896 提供,维护者可考虑直接关闭本 PR。 |
|
Closing: #7896 landed the same feature (mid-stream transport continuation) while this was in review, and after the conflict merge the runtime diff here was reduced to zero. I also checked the remaining test-only delta against main before closing: the isContinuation retry flag and the merged single model turn are pinned by the #7832 suite, and the functionCall cut case has its own test ('does not continue a cut that delivered a functionCall'). Nothing left that main does not already have. Thanks for the reviews along the way. |
6 participants
What this PR does
Fixes #7832. The stream transport retry used to fire only before the first chunk reached callers, so a socket close anywhere into a long thinking stream (UND_ERR_SOCKET minutes in) propagated and threw away everything already generated. With this change, when chunks have already flowed and the failed attempt left a text or thought partial, the retry runs as a continuation instead: the partial turn stays in history, a recovery user message shows the model its own suffix, and the model picks up where the stream died. One continuation per send; if it fails, the partial turn survives in history and on the transcript, and the error propagates as before.
Why it's needed
YOLO/headless generation of large outputs fails consistently for thinking models today: the whole think -> tool loop runs as a single SSE stream that outlives the gateway's idle window, the gateway closes the socket, and the session dies with
[API Error: terminated]. Restarting from zero would also re-print the partial output in headless mode, which is why a plain after-chunk retry was never enabled. The continuation shape reuses the same machinery as the MAX_TOKENS output recovery (recovery message,isContinuationretry event, coalescing), so the UI keeps its buffer and the continuation appends seamlessly in both interactive and headless modes. FunctionCall partials still propagate immediately, since a user message cannot be injected between a functionCall and its response.Reviewer Test Plan
How to verify
npx vitest run src/core/geminiChat.test.tsinpackages/core: 242/242 pass. The new tests cover the three relevant shapes: mid-stream socket close with a text partial retries as a continuation (RETRYevent withisContinuation: true, history coalesced to a single model turn holding partial + continuation, no synthetic user turn left behind); a functionCall partial still propagates with no retry attempt; a failed continuation keeps the partial turn and rethrows. The previously existing test that pinned "never retry after a chunk" was updated to the new contract, since that pinned behavior is exactly what #7832 reports as the bug.Evidence (Before & After)
N/A (core retry semantics, no UI surface change; the TUI's existing
isContinuationhandling keeps the text buffer, verified by readinguseGeminiStream.ts's Retry case).Tested on