fix(core): clarify stream idle timeout guidance - #9896
Conversation
|
Thanks for the PR! Clean follow-up to #9795. Template looks good ✓ Problem: real and observed. Since #9795 (merged today), an explicit Direction: aligned — this is about remediation advice actually working for every configuration source. The reference CHANGELOG shows several recent stream-idle-timeout fixes, so the area is actively maintained and correct guidance matters. Size: core paths touched. Production logic: 8 lines (5+/3- in Approach: scope is right — the message rewrite (R1-1) and the zero-value regression test (R1-2) map one-to-one to the two review findings, and nothing else is in the diff. The new wording matches Risk: Moving on to code review. 🔍 中文说明感谢贡献!这是对 #9795 的一次干净的后续修复。 模板完整 ✓ 问题:真实且已观测。#9795(今日已合并)之后,模型或 provider 设置中显式的 方向:对齐——目的是让修复建议在所有配置来源下都真正有效。参考 CHANGELOG 近期有多条 stream-idle-timeout 相关修复,说明该领域在活跃维护,正确的引导文案是有价值的。 规模:触及核心路径。生产逻辑 8 行( 方案:范围合理——错误信息重写(R1-1)和零值回归测试(R1-2)与两条审查意见一一对应,diff 中没有其他内容。新文案与 风险: 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewIndependent baseline first: from the title and motivation alone, I would rewrite the No blockers found. Details worth noting:
Skipping the sequence diagram and the changed-files table — 3 files and 8 production lines leave nothing for them to add. Testing evidence (this PR's own CI via API — no PR code executed here)
Both red checks are pre-existing main breakage, not caused by this PR. They fail at Verified from git history rather than taken on faith: Consequence: this PR's updated assertions have never executed in CI on this commit — the 181/181 pass figure in the description is the author's claim, not independently observed evidence. The macOS/Windows unit legs and the integration tests were skipped because the Linux leg failed first. Sandboxed verification would settle the remaining gap after a rebase: once the branch picks up Live-behavior testing is not driven on unattended CI runs — on this path the signal comes from the PR's CI and the lane above. 中文说明代码审查独立基线:仅凭标题和动机,我的方案就是重写 未发现阻塞问题。要点:
3 个文件、8 行生产改动,时序图和文件表没有可补充的信息,故省略。 测试证据(通过 API 读取本 PR 自身 CI——未在此执行任何 PR 代码)两个红色检查均为 main 上已存在的构建破坏,并非本 PR 引起:它们在 后果:本 PR 更新的断言在该提交上从未被 CI 执行过——描述中 181/181 通过是作者自述,不是独立证据。rebase 到包含 无人值守 CI 不进行实机行为测试。 — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 3/5 — clean review that would be a 4/5 on green CI; approval is held only because this head's CI is red on inherited main breakage and the PR's own tests have never executed here. Stepping back: this is what a good follow-up PR looks like. The problem is real and verified in code (the old message recommended a knob that #9795 deliberately made second-best), the fix is the minimum that makes the advice correct for every configuration source, and the wording matches the docs character for character. The zero-value regression test is the kind that earns its keep — it pins a strict Why no approval yet: both unit-leg checks fail at @yu-xin-c one mechanical step unblocks this: rebase onto current No deferred-approval marker on this run: CI already settled (red) on this head rather than still running, and the rebase moves the head — the green signal has to attach to the rebased commit. 中文说明置信度:3/5 —— 审查本身干净,若 CI 为绿可给 4/5;暂不批准仅因为当前提交的 CI 因继承的 main 构建破坏而红,本 PR 自己的测试从未在此被执行。 整体来看:这是一个高质量后续 PR 的范本。问题真实且已在代码中核实(旧提示建议的环境变量在 #9795 之后已沦为次优配置来源),修复是让建议在所有配置来源下都正确的最小改动,文案与文档逐字一致。零值回归测试值得保留——它钉住了严格的 为何暂不批准:两个单元检查都在 @yu-xin-c 只差一步机械操作:rebase 到当前 本次不附延迟批准标记:该提交的 CI 已结束(红)而非仍在运行,且 rebase 会移动头部——绿色信号必须落在 rebase 后的提交上。 — Qwen Code · qwen3.8-max Reviewed at |
378d03b to
b75746e
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 current |
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.22.0)
|
Addressed the follow-up precedence suggestion in |
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.22.0)
|
Addressed the built-in Qwen OAuth configuration gap from round 2 in |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not explored to full depth (tool budget reached): "agent 6c": end-to-end runtime verification that syncAfterAuthRefresh actually routes the startup OAuth flow through applyResolvedModelDefaults (its body was unread whe…; "agent 1c": could not read PR #9896's existing comment thread ( gh token lacks read:org scope) — could not determine whether the OAuth phrasing was already discussed/res….
中文说明
未探索到全部深度(达到工具调用预算):"agent 6c":end-to-end runtime verification that syncAfterAuthRefresh actually routes the startup OAuth flow through applyResolvedModelDefaults (its body was unread whe…;"agent 1c":could not read PR #9896's existing comment thread ( gh token lacks read:org scope) — could not determine whether the OAuth phrasing was already discussed/res…。
— qwen3.8-max via Qwen Code /review (v0.22.0)
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
No issues found. LGTM! ✅
中文说明
未发现问题。LGTM!✅
— qwen3.8-max via Qwen Code /review (v0.22.0)
Local verification on a real build — every remediation in the new message actually worksI built this PR locally and drove the real CLI against a live endpoint that answers a streaming request with SSE headers and then goes silent, so nothing but the client-side idle watchdog can end the turn. Verdict: all four claims in the new message reproduce, timeout behaviour is identical to the merge base, and the added test is a real regression guard. LGTM for merge. Four non-blocking observations at the end. Setup
1. Does each location the message names actually change the effective timeout?
2. Built-in Qwen OAuth modelsEnd-to-end isn’t reachable (see observation 1), so I probed the real built core instead — seed the settings value, then select the model the way the CLI does:
3. Before / after, in the real TUISame settings ( Before — merge base After — this PR: the user is pointed at the location that is actually live for them. 4. Behaviour is unchangedThe full 7-arm matrix was re-run on the before-bundle: identical reported idle values and per-attempt server-side timings (A 3010/3003/3003, B 8009/8002/8003, C 3006/3002/3001, D 3008/3002/3002, E 7008/7003/7001, F0 3012/3003/3004, F 12002/12001). This confirms the PR-body claim that runtime timeout selection and retry behaviour are untouched — only the string differs. 5. The added test has teethMutating
And the mirror check: merge-base 6. Checks run
Non-blocking observations
For context on observation 1: for the provider-vs-settings case, the CLI already emits a targeted startup warning naming the exact fix (quoted in arm E). The timeout message grew from 149 to 529 characters; the parts that earn their keep in my runs are the provider-vs-runtime split and the “explicit value beats the env var” correction — both of which arm B and arm D show were genuinely wrong before. 中文版本本地真实构建验证 —— 新错误信息里给出的每条修复建议都确实有效我在本地构建了这个 PR,并让真实 CLI 去访问一个「先回 SSE 响应头、然后一直沉默」的本地端点,这样只有客户端的空闲看门狗能结束这一轮。结论:新信息中的四条论断全部复现,超时行为与合并基线完全一致,新增的测试也确实是一道有效的回归护栏。建议合入。 文末有四条非阻断性观察。 环境
1. 信息里点名的每个配置位置,真的能改变生效的超时吗?
2. 内置 Qwen OAuth 模型端到端跑不通(见观察 1),因此改为直接探测真实构建出的 core:先把 settings 的值播种进去,再按 CLI 的方式选中该模型:
3. 真实 TUI 的前后对比相同配置( 修改前 —— 合并基线 修改后 —— 本 PR: 用户被指向对自己真正生效的那个位置。 (截图见英文版,两张图对应同一次运行。) 4. 行为没有变化同一套 7 组矩阵在「修改前」bundle 上重跑,得到完全一致的实报超时值和逐次服务端耗时(A 3010/3003/3003、B 8009/8002/8003、C 3006/3002/3001、D 3008/3002/3002、E 7008/7003/7001、F0 3012/3003/3004、F 12002/12001)。这印证了 PR 描述中「超时选择与重试行为均未改变」的说法 —— 变的只有文案。 5. 新增测试确有辨别力把
反向检查同样成立:基线 6. 已跑的检查
非阻断性观察
关于观察 1 的补充背景:对于 provider 与 settings 冲突这一情形,CLI 本来就会打印一条点名了确切修法的启动告警(见 E 组引用)。这条超时信息从 149 字符增长到了 529 字符;在我的实测中真正物有所值的部分是 provider / runtime 的区分,以及「显式值压过环境变量」这一纠正 —— B 组和 D 组恰恰证明了这两点在改动前确实是错的。 |
|
Released in v0.22.3. |



What this PR does
Updates stream inactivity timeout errors to point users to both supported settings locations before the environment fallback, and adds a regression test proving that a registry-resolved
streamIdleTimeoutMs: 0remains intact.Why it's needed
PR #9795 made explicit model and provider settings take precedence over
QWEN_STREAM_IDLE_TIMEOUT_MS, but the timeout error still recommended only the environment variable. That advice can be ineffective when an explicit value is active. The missing zero-value case also left the registry merge vulnerable to a future truthiness regression.Reviewer Test Plan
How to verify
cd packages/core && npx vitest run src/core/openaiContentGenerator/pipeline.test.ts src/models/content-generator-config.test.ts --maxWorkers=1.ETIMEDOUT, while the message namesmodel.generationConfig.streamIdleTimeoutMs,modelProviders[providerId][].generationConfig.streamIdleTimeoutMs, and usesQWEN_STREAM_IDLE_TIMEOUT_MSonly as the fallback.streamIdleTimeoutMs: 0rather than falling back to the parent value.Evidence (Before & After)
Before: with an explicit
streamIdleTimeoutMsactive, the error said onlySet QWEN_STREAM_IDLE_TIMEOUT_MS to increase this window, even though that environment value would be overridden.After: the error names both settings locations, explains when the environment fallback applies, and tells users to set the active value to
0to disable the guard. The two affected test files pass with181/181tests.Tested on
Environment (optional)
Node.js 22.22.2 on macOS. Focused ESLint, workspace typecheck, and full build passed on the #9795 merge base. Repository preflight completed format, lint, build, and typecheck; its parallel test stage reported 29 CLI and 4 core timing/resource failures in unrelated files, and every originally failing test passed when rerun serially. After rebasing onto current
main(including the #9888 build fix), the 181 affected tests and the full workspace typecheck pass again.Risk & Scope
streamMaxLifetimeMs, Windows/Linux runtime behavior, and a published-CLI E2E run (no globalqwenexecutable is installed).Linked Issues
Follow-up to #9795 and related to #5975. Addresses the two non-blocking review findings in R1-1 and R1-2.
中文说明
本 PR 的改动
更新流式响应无活动超时的错误提示:先指出两个受支持的设置位置,再说明环境变量回退;同时新增回归测试,证明从模型注册表解析出的
streamIdleTimeoutMs: 0会被完整保留。为什么需要
PR #9795 让显式模型设置和 provider 设置优先于
QWEN_STREAM_IDLE_TIMEOUT_MS,但超时错误仍只建议修改环境变量。当显式值生效时,这条建议可能完全不起作用。缺少零值用例也使注册表合并逻辑可能在未来被 truthiness 判断意外破坏。审查测试计划
验证方法
cd packages/core && npx vitest run src/core/openaiContentGenerator/pipeline.test.ts src/models/content-generator-config.test.ts --maxWorkers=1。ETIMEDOUT,同时错误信息会列出model.generationConfig.streamIdleTimeoutMs、modelProviders[providerId][].generationConfig.streamIdleTimeoutMs,并仅把QWEN_STREAM_IDLE_TIMEOUT_MS作为回退项。streamIdleTimeoutMs: 0,而不是退回父配置的值。前后证据
修改前:即使显式
streamIdleTimeoutMs已生效,错误仍只显示Set QWEN_STREAM_IDLE_TIMEOUT_MS to increase this window,但该环境变量实际上会被覆盖。修改后:错误信息会列出两个设置位置,说明环境变量何时作为回退生效,并提示将当前生效值设为
0可禁用保护。两个相关测试文件共181/181条通过。测试平台
环境(可选)
macOS,Node.js 22.22.2。在 #9795 合并基线上,定向 ESLint、全工作区 typecheck 和完整 build 均通过。仓库 preflight 完成了 format、lint、build 和 typecheck;其并行测试阶段在无关文件中出现 29 条 CLI 与 4 条 core 时序/资源失败,所有原失败用例串行重跑后均通过。rebase 到包含 #9888 构建修复的当前
main后,相关 181 条测试与全工作区 typecheck 再次全部通过。风险与范围
streamMaxLifetimeMs、Windows/Linux 运行时行为,以及已发布 CLI 的 E2E(本机未安装全局qwen命令)。关联问题
这是 #9795 的后续,并与 #5975 相关;处理了两个非阻塞审查意见:R1-1 与 R1-2。