fix(core): add stream idle watchdog for silent responses - #4256
fix(core): add stream idle watchdog for silent responses#4256Alexxigang wants to merge 4 commits into
Conversation
| * | ||
| * @example | ||
| * ```ts | ||
| * ``ts |
There was a problem hiding this comment.
[Critical] JSDoc @example code fence markers are damaged — double backticks (``ts / ``) instead of triple backticks (```ts / ```). This breaks JSDoc rendering for sendMessageStream.
| * ``ts | |
| * ```ts | |
| * const chat = ai.chats.create({model: 'gemini-2.0-flash'}); | |
| * const response = await chat.sendMessageStream({ | |
| * message: 'Why is the sky blue?' | |
| * }); | |
| * for await (const chunk of response) { | |
| * console.log(chunk.text); | |
| * } | |
| * ``` |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| }, timeoutMs); | ||
| }); | ||
|
|
||
| return Promise.race([nextPromise, timeoutPromise]).finally(clearTimers); |
There was a problem hiding this comment.
[Suggestion] Each stream chunk in the hot while(true) loop allocates a new Promise.race + 2 setTimeout/clearTimeout pairs + 3 Promise objects via streamWatchdog.next(). For a stream with 20-50 chunks this creates measurable overhead. Consider lifting the timeout timer outside the loop and using timeoutId.refresh() per chunk (Node.js timer refresh API) to eliminate per-chunk timer churn while preserving the idle-detection guarantee.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| }, | ||
| authType: this.config.getContentGeneratorConfig()?.authType, | ||
| persistentMode: isUnattendedMode(), | ||
| signal: params.config?.abortSignal, |
There was a problem hiding this comment.
[Suggestion] retryWithBackoff receives only params.config?.abortSignal (the user's signal), not the internal streamAbortController.signal. When the stream idle watchdog fires during a retry delay, retryWithBackoff won't notice the internal abort and will wait the full delay before calling apiCall — which will then immediately fail because streamAbortController.signal is already aborted. Consider passing streamAbortController.signal (or a combined signal) to retryWithBackoff so internal aborts skip unnecessary retry delays.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
DragonnZhang
left a comment
DragonnZhang
left a comment
There was a problem hiding this comment.
The stream idle watchdog logic is sound — correct lifecycle management, proper retry integration via InvalidStreamError, and adequate test coverage. However, the branch has a character-encoding issue that corrupts em-dashes and JSDoc fences throughout both files, including a runtime string sent to the model.
Encoding corruption (blocker):
UTF-8 em-dashes (—, bytes e2 80 94) have been corrupted to mojibake (e.g. e9 97 3f) in ~15 lines across geminiChat.ts and geminiChat.test.ts. The most impactful instance is OUTPUT_RECOVERY_MESSAGE at line 179 — the model will now receive Resume directly é—?no apology instead of Resume directly — no apology. This likely stems from a commit-encoding mismatch on the fork. A rebase with correct UTF-8 handling should fix all instances at once.
Other issues in this diff:
- The JSDoc
@examplecode fences forsendMessageStreamare damaged (triple backticks became double) — see existing inline comment at line 767. - The class-level JSDoc block for
GeminiChat(/** Chat session that enables sending messages... */) was deleted, likely unintentionally.
Once the encoding corruption is resolved, the watchdog feature itself looks good to merge.
— qwen-code via Qwen Code /review
| */ | ||
| const OUTPUT_RECOVERY_MESSAGE = | ||
| 'Output token limit hit. Resume directly — no apology, no recap of what ' + | ||
| 'Output token limit hit. Resume directly �?no apology, no recap of what ' + |
There was a problem hiding this comment.
[Bug] This runtime string has encoding corruption: the em-dash — (UTF-8 e2 80 94) became é—? (e9 97 3f). The model will receive garbled text: Resume directly é—?no apology. This is part of a broader encoding issue across the branch — approximately 15 lines in both files have corrupted em-dashes. Please rebase with correct UTF-8 encoding to fix all instances.
DragonnZhang
left a comment
DragonnZhang
left a comment
There was a problem hiding this comment.
The stream idle watchdog design is sound: the createStreamIdleWatchdog lifecycle (timer reset per chunk, finally cleanup in processStreamResponse, catch-path cleanup in makeApiCallAndProcessStream) is correct, and the InvalidStreamError('STREAM_IDLE_TIMEOUT') reuse of the existing transient retry budget is the right approach. Test coverage for timeout, timer reset on progress, and env-based disablement is adequate.
However, the branch-wide encoding corruption remains unresolved. At least 19 em-dash (—, U+2014) instances across both files are corrupted to \xe9\x97? at the byte level, and arrow characters (→) are similarly damaged. Critically, this affects the runtime string OUTPUT_RECOVERY_MESSAGE (line 179), which means the model will receive garbled text (Resume directly \xe9\x97?no apology) during output-limit recovery. The JSDoc @example code fences (triple backticks reduced to double backticks at line 767) also need restoration.
These encoding issues must be fixed before merge. A clean rebase from a UTF-8-safe editor or git checkout origin/main -- packages/core/src/core/geminiChat.ts packages/core/src/core/geminiChat.test.ts followed by reapplying only the watchdog logic would resolve this cleanly.
— qwen3-coder via Qwen Code /review
|
@Alexxigang heads up — this PR currently has merge conflicts with Conflicting files:
The rest merges cleanly. Thanks! 中文@Alexxigang 提个醒 —— 这个 PR 目前和 冲突文件:
其余文件可以自动合并。谢谢! |
|
@qwen-code /resolve |
Resolve merge conflicts in geminiChat.ts and geminiChat.test.ts between the stream idle watchdog feature and main's partial-tool-use repair, coerceUsageCount, normalizeModelToolCallIds, and deferred recording. The watchdog's while(true)+watchdog stream loop and abort-signal linking are integrated inside main's try/catch/streamError architecture, with watchdog cleanup in the finally block of processStreamResponse.
|
Qwen Code resolved the merge conflicts and pushed the branch update. PR #4256 Merge Resolution SummaryCommit: Conflicted Files
Resolution StrategyStarted from main's clean version of each file (which has the partial-tool-use repair subsystem, coerceUsageCount, normalizeModelToolCallIds, syncFunctionCallsField, deferred recording, etc.) and surgically applied the PR's watchdog changes on top using string-replacement scripts. Conflict Details —
|
DragonnZhang
left a comment
DragonnZhang
left a comment
There was a problem hiding this comment.
Code Review — Stream Idle Watchdog
Finding — cleanupAbortLink not called on success path → abort-listener leak
File: packages/core/src/core/geminiChat.ts, processStreamResponse()
The finally block in processStreamResponse calls streamWatchdog?.cleanup() but does not call cleanupAbortLink(). The cleanupAbortLink function removes the abort event listener that was registered on the parent AbortSignal by linkAbortSignal(). On the normal (non-error) completion path, this listener is never removed.
} finally {
streamWatchdog?.cleanup();
// cleanupAbortLink() is missing here
}Over a long session, one dangling abort listener accumulates on the caller's AbortSignal per successful stream call. If the signal is the same long-lived object (e.g., a session-level controller), this grows without bound.
Fix: Add cleanupAbortLink() to the finally block alongside streamWatchdog?.cleanup():
} finally {
streamWatchdog?.cleanup();
cleanupAbortLink?.();
}The cleanupAbortLink return value from linkAbortSignal() should be captured in makeApiCallAndProcessStream and threaded into processStreamResponse (or called directly in makeApiCallAndProcessStream's own finally block).
Everything else checked out: QWEN_CODE_STREAM_IDLE_TIMEOUT_MS parsing correctly guards against NaN and zero; linkAbortSignal handles an already-aborted signal immediately; STREAM_IDLE_TIMEOUT reaches the isTransientStreamError retry path and is retried like other InvalidStreamError types.
Generated by Claude Code
|
@qwen-code /triage |
|
@qwen-code /resolve |
|
@qwen-code /resolve |
|
Qwen Code resolved the merge conflicts and pushed the branch update. Merge Conflict Resolution — PR #4256PR: fix(core): add stream idle watchdog for silent responsesConflicted file
Non-conflicted (auto-merged)
Conflict 1:
|
|
Qwen Code did not run conflict resolution for this request. PR #4256 does not currently have merge conflicts with main. |
|
Re-run — maintainer requested fresh triage. Template looks good ✓ Problem: Real and well-documented. Issue #4177 describes the SSE silent-drop hang with detailed analysis, upstream reference (claude-code), and a clear proposed design. Users on flaky networks get a frozen spinner with no recovery path other than Ctrl+C. Direction: Aligned. A stream idle watchdog that aborts stalled streams and reuses the existing Size: 165 production lines (geminiChat.ts: +157/−8), 181 test lines (geminiChat.test.ts: +179/−2, excluded from count). Well under thresholds. Approach: Focused — 2 files, clean separation of watchdog logic from existing stream processing. The One consistency note: Moving on to code review. 🔍 中文说明重新审查 — 维护者要求重新 triage。 模板完整 ✓ 问题：真实且有充分文档。Issue #4177 详细描述了 SSE 静默断开的挂起问题，包含上游参考（claude-code）以及明确的设计方案。用户在弱网环境下只能看到冻结的 spinner，除了 Ctrl+C 无计可施。 方向：对齐。添加流空闲看门狗来中止停滞的流并复用现有 规模：165 行生产代码，181 行测试代码（已排除）。远低于阈值。 方案：聚焦——2 个文件，看门狗逻辑与现有流处理干净分离。 一致性备注： 进入代码审查 🔍 — Qwen Code · qwen3.7-max |
Code ReviewRe-run — reviewing the latest diff after maintainer's re-triage request. One confirmed bug, unchanged from the previous triage:
Note: the catch block in Fix is one line: } finally {
streamWatchdog?.cleanup();
cleanupAbortLink?.(); // ← add this
}Same issue flagged by @DragonnZhang and the prior triage run. Everything else checks out:
Non-blocking notes:
TestingUnit tests: could not re-run locally (npm install timed out in this environment). The previous triage confirmed 195/195 pass (3 new watchdog tests + 192 existing), and the diff has not changed since. The new tests cover all three critical paths: silent stream timeout → retry exhaustion, timer reset on chunk arrival, and env-var disablement. Tmux before/after: N/A for this PR. The fix targets provider-side silent stream stalls that cannot be reproduced locally without network manipulation. The unit tests comprehensively cover the watchdog behavior with fake timers and mock streams. 中文说明代码审查重新审查 — 维护者要求重新 triage 后的最新 diff 审查。 一个已确认的 bug，与上次相同：
其他部分均已验证正确： 看门狗生命周期、 非阻塞备注： 逐块 测试单元测试：本地环境无法重新运行（npm install 超时）。上次 triage 确认 195/195 通过，且 diff 自此未有变更。新测试覆盖了三个关键路径。 Tmux before/after：本 PR 不适用。 该修复针对 provider 侧的静默流停滞，无法在本地环境中复现。 — Qwen Code · qwen3.7-max |
|
Re-run verdict — same as before. The design is sound — a focused, 165-line watchdog that follows the upstream claude-code pattern and solves a real user pain point. The approach matches what I'd propose independently: One bug still blocks merge: the The maintainers' existing 中文说明重新审查结论——与之前相同。 设计合理——165 行的看门狗实现，遵循上游 claude-code 的模式，解决了真实的用户痛点。方案与独立提案一致：逐块 一个 bug 仍然阻塞合并： 维护者的现有 — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
— qwen3.7-max via Qwen Code /review
| } catch (e) { | ||
| streamError = e; | ||
| } finally { | ||
| streamWatchdog?.cleanup(); |
There was a problem hiding this comment.
[Suggestion] The for await (const chunk of streamResponse) loop was replaced with a manual while (true) + streamResponse.next() loop to interpose the watchdog's Promise.race. However, for await...of implicitly calls streamResponse.return() on early exit (exception thrown inside the loop body, or from yield chunk if the consumer throws), which triggers the underlying generator's internal finally blocks. The manual loop never calls streamResponse.return(), so the SDK's async generator is abandoned without explicit finalization on error paths.
This is a regression for all error paths through processStreamResponse, not just watchdog timeouts — any mid-iteration throw (from normalizeModelToolCallIds, syncFunctionCallsField, or the yield chunk itself) now skips the generator's cleanup.
| streamWatchdog?.cleanup(); | |
| streamWatchdog?.cleanup(); | |
| await streamResponse.return?.(); |
— qwen3.7-max via Qwen Code /review
|
@qwen-code /triage |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
No review findings. Downgraded from Approve to Comment: CI failing (Test (ubuntu-latest, Node 22.x)). Suggestion-level recommendations are in the Suggestion summary comment below.
— qwen3.7-max via Qwen Code /review
Suggestions — commit
|
| File | Issue | Suggested fix |
|---|---|---|
packages/core/src/core/geminiChat.ts:~1131 |
InvalidStreamError timeout message omits the model name — the warning log above includes it, but the error that propagates to the user does not |
Include model: `Model ${model} stream went idle for ${timeoutMs}ms without receiving a chunk.` |
packages/core/src/core/geminiChat.ts:~1128 |
abortController.abort() called with no reason — watchdog timeout is indistinguishable from user-initiated abort at the signal level |
Pass a reason: abortController.abort(new Error(\Stream idle timeout after ${timeoutMs}ms`))` |
packages/core/src/core/geminiChat.test.ts |
No test verifies the positive case of caller abort signal propagation through linkAbortSignal to the internal streamAbortController |
Add a test that calls abortController.abort() on a slow stream and verifies the internal signal is also aborted |
— qwen3.7-max via Qwen Code /review
| } catch (e) { | ||
| streamError = e; | ||
| } finally { | ||
| streamWatchdog?.cleanup(); |
There was a problem hiding this comment.
[Critical] cleanupAbortLink() is passed to processStreamResponse but never invoked. The finally block only calls streamWatchdog?.cleanup(), so the abort event listener attached by linkAbortSignal on the caller's AbortSignal is never removed on any path that enters processStreamResponse (success, stream error, or watchdog abort). In long sessions, listeners accumulate — Node.js emits MaxListenersExceededWarning after 10+ and the orphaned closures retain references preventing GC.
| streamWatchdog?.cleanup(); | |
| } finally { | |
| streamWatchdog?.cleanup(); | |
| cleanupAbortLink?.(); | |
| } |
— qwen3.7-max via Qwen Code /review
…7389) * fix(ci): stop /resolve reports from being guillotined mid-sentence Every substantive /resolve summary was hitting the 2000-byte cap exactly and stopping mid-word: #2993, #4256 and #6206 all ended at 2100 bytes total, cut inside a sentence, with nothing saying the report had been clipped rather than abandoned. Two causes, both fixed: - The contract asked for a file-by-file inventory, which duplicates the diff and grows without bound. It now asks for what only the resolver knows — the root cause on the base branch, whether the merge was semantic or merely textual, what the resolution's correctness rests on, and what it could not verify (this command runs no tests and may not touch non-conflicted files, so a merge that breaks an untouched test can only be reported). - The cap was silent and too low. It is now 6000, above the 4000 the prompt asks for, and a report that still exceeds it says so. Also adds the project's collapsed Chinese section to the contract; no /resolve report had one. * fix(ci): make the truncation test fatal-decode real and link the run in the notice (#7389) --------- Co-authored-by: wenshao <wenshao@example.com> Co-authored-by: qwen-code-dev-bot <qwen-code-dev@service.alibaba.com>
7 participants
Summary
next()calls inGeminiChatInvalidStreamError('STREAM_IDLE_TIMEOUT')and reuses existing transient retry handlingSTREAM_IDLE_TIMEOUTQWEN_CODE_STREAM_IDLE_TIMEOUT_MS,QWEN_CODE_DISABLE_STREAM_WATCHDOG)Validation
geminiChat.test.ts, lint, typecheck, and buildQWEN_CODE_STREAM_IDLE_TIMEOUT_MS=50stream idle watchdogtests insrc/core/geminiChat.test.tsmakeApiCallAndProcessStream()andprocessStreamResponse()for cleanup and retry flow91tests passed insrc/core/geminiChat.test.tsnpm run lint,npm run typecheck, andnpm run buildall completed successfullyScope / Risk
QWEN_CODE_STREAM_IDLE_TIMEOUT_MS,QWEN_CODE_DISABLE_STREAM_WATCHDOGTesting Matrix
Testing matrix notes:
packages/coreworkspace only.Linked Issues / Bugs
Fixes #4177