fix(cli): apply FETCH_TIMEOUT_MS to /update version check and log fetchInfo results (#6857) - #6887
Conversation
…chInfo results (QwenLM#6857) The FETCH_TIMEOUT_MS = 2000 constant in updateCheck.ts was defined but never wired up. update-notifier's fetchInfo() takes no timeout option, so slow/unreachable registries (corporate proxies, offline networks, scoped .npmrc mirrors without auth) would either hang the check or fall back to whatever update-notifier internally decides — sometimes a stale configstore cache, reported by users as '/update reports up-to-date on 0.19.9 when 0.19.10 is available.' Race fetchInfo() against a bounded timer via Promise.race and surface a new UpdateCheckTimeoutError when it fires, so '/update' returns the existing 'error' status instead of silently reporting 'up to date.' Also log the fetchInfo return value under the UPDATE_CHECK debug tag so the next round of reports can distinguish 'registry returned the wrong version' from 'we compared incorrectly' without adding more speculation. Refs QwenLM#6857
|
Thanks for the PR! (Re-run after new commits) Template looks good ✓ Problem: Observed bug with evidence. Users report Direction: Aligned. Making Size: Not applicable — Approach: The scope is tight and well-scoped. Since the last review, the author added 中文说明感谢贡献！(新提交后重新审查) 模板完整 ✓ 问题：已观测到的 bug，有证据。用户报告 方向：对齐。让 规模：不适用—— 方案：范围紧凑且聚焦。自上次审查以来，作者在 — Qwen Code · qwen3.7-max Reviewed at |
Code Review (re-run — new commit
|
|
Confidence: 5/5 — Clean across every stage. Real problem, focused fix, proper tests, clean code. This PR does exactly what it says: wires a dead constant into the actual code path and adds the debug logging needed to diagnose future reports. The The scope remains disciplined — two files, 67 production lines, no drive-by refactors, no scope creep. The author explicitly deferred the larger If there's a risk, it's that 2 seconds might be too tight for some corporate proxy environments. But that's a one-line follow-up ( LGTM, looks ready to ship. ✅ 中文说明置信度：5/5 — 每个阶段都干净。真实问题、聚焦修复、完善测试、干净代码。 这个 PR 完全做到了它承诺的：将一个死常量接入实际代码路径，并添加诊断未来报告所需的调试日志。 范围依然严格自律——2 个文件、67 行生产代码、无顺手重构、无范围蔓延。作者明确将更大的 如果有风险，就是 2 秒对某些企业代理环境可能太紧。但那只是一个一行的后续修复（ 可以合入 ✅ — Qwen Code · qwen3.7-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. ✅
|
Heads up on the failing PR #6872 added a second identical import next to a new call site without noticing the same specifier was already imported earlier in the file. This blocks CI on every open PR against Filed the one-line fix separately in #6890. Once that merges I'll merge |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. Not reviewed: chunk 1 — no agent reported covering these; nobody read them.
— qwen3.7-max via Qwen Code /review
| getPackageJson.mockResolvedValue({ | ||
| name: 'test-package', | ||
| version: '1.0.0', | ||
| }); |
There was a problem hiding this comment.
[Suggestion] The timeout tests only exercise the non-nightly (single-fetch) path. The nightly branch uses Promise.all with two concurrent fetchInfoWithTimeout calls — a more complex interaction that has no timeout test. If a bug existed in the nightly path's Promise.all + timeout interaction, it would go undetected. — Concrete cost: the nightly Promise.all path is untested for timeout behavior despite being the more complex of the two code paths.
Add a test with version: '1.0.0-nightly.1' where one or both fetchInfo mocks return a never-resolving promise, advance timers past FETCH_TIMEOUT_MS, and assert status: 'error' with UpdateCheckTimeoutError.
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Good catch — added in 51578836a. Two new tests cover the nightly Promise.all timeout path:
surfaces a timeout when only the nightly dist-tag stalls— mocksnightlyas a never-resolving promise andlatestas a fast success, advances timers pastFETCH_TIMEOUT_MS, and assertsPromise.allpropagates the timeout AND the error message namesfor nightly. This catches both directions of the wiring: that the timer actually reaches insidePromise.all, and that the correct dist-tag gets tagged.surfaces a timeout when both nightly dist-tags stall— full outage; asserts a typedUpdateCheckTimeoutErrorwith either dist-tag on the message (whichever rejectionPromise.allsees first is a valid symptom of the same failure).
| export class UpdateCheckTimeoutError extends Error { | ||
| constructor(timeoutMs: number) { | ||
| super(`update-notifier fetchInfo timed out after ${timeoutMs}ms`); | ||
| this.name = 'UpdateCheckTimeoutError'; | ||
| } | ||
| } |
There was a problem hiding this comment.
[Suggestion] The timeout error message ("fetchInfo timed out after 2000ms") carries no indication of which dist-tag (nightly vs latest) failed. In the nightly path where two fetches run concurrently via Promise.all, a timeout from either produces an identical error. — Concrete cost: when investigating a timeout from logs alone (e.g., corporate proxy blocking only one dist-tag), the oncall engineer cannot tell which registry endpoint was unreachable.
| export class UpdateCheckTimeoutError extends Error { | |
| constructor(timeoutMs: number) { | |
| super(`update-notifier fetchInfo timed out after ${timeoutMs}ms`); | |
| this.name = 'UpdateCheckTimeoutError'; | |
| } | |
| } | |
| export class UpdateCheckTimeoutError extends Error { | |
| constructor(timeoutMs: number, distTag?: string) { | |
| const tag = distTag ? ` for ${distTag}` : ''; | |
| super(`update-notifier fetchInfo timed out after ${timeoutMs}ms${tag}`); | |
| this.name = 'UpdateCheckTimeoutError'; | |
| } | |
| } |
Then pass the dist-tag from fetchInfoWithTimeout through to the error constructor.
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Good catch — applied in 51578836a. UpdateCheckTimeoutError now takes an optional distTag (also exposed as a public field), fetchInfoWithTimeout threads it through, and both call sites pass the tag. Non-nightly path is latest; nightly path passes nightly and latest to the two Promise.all fetches respectively. Error messages now read update-notifier fetchInfo timed out after 2000ms for latest / ... for nightly, so a log reader can tell which endpoint stalled.
…meout-and-logging
… timeout paths Address bot review on QwenLM#6887: - `UpdateCheckTimeoutError` now takes an optional `distTag` argument that is threaded through by `fetchInfoWithTimeout` and appended to the message. The nightly path fires `nightly` and `latest` fetches concurrently via `Promise.all`; without a dist-tag on the error, an oncall reading logs cannot tell which registry endpoint stalled (e.g. a corporate proxy that lets `nightly` through but blocks `latest`). The tag also lands on the error instance as a public `distTag` field so callers can branch on it programmatically. - Add two regression tests for the nightly `Promise.all` timeout path: a single stalled dist-tag (asserts Promise.all propagates the timeout and names the exact tag) and both stalled (full outage — asserts we still surface a typed error with a valid tag). The non-nightly test now also asserts the message contains `for latest`.
|
@qwen-code /review |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Not reviewed: chunk 1 — no agent reported covering these; nobody read them.
— 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.
LGTM, looks ready to ship. ✅
✅ Local verification — real tests on macOS (build & run)Verified Verdict: verified — good to merge. Both halves of the PR do what they claim; the new tests are load-bearing; formatting/lint/types are clean. 1 · All affected suites green — 89/89Ran the file the PR changes plus the four other suites named in its test plan:
2 · The new tests are load-bearing (controlled experiment)Reverted only the timeout wiring (
3 · Real-runtime before/after + the new debug lineThe unit tests use fake timers, so I drove the real
Second change confirmed too: with a bound debug session I read the on-disk log and saw the new line —
4 · Hygiene
Notes for the merge decision
Environment: macOS (darwin 24.6.0) · Node 22.23.1 · vitest 3.2.4 · isolated worktree @ 🇨🇳 中文说明（点击展开）✅ 本地验证 —— macOS 上真实构建并运行测试在隔离的 git worktree 中对 结论：验证通过，可以合并。 PR 的两部分改动都名副其实；新增测试确实是"承重"的；格式 / lint / 类型均干净。 1 · 所有相关测试集全绿 —— 89/89跑了 PR 改动的文件，以及它测试计划里点名的另外四个测试集：
2 · 新增测试是"承重"的（对照实验）只回退超时接线（把 3 · 真实运行时 before/after + 新增 debug 日志单元测试用的是 fake timer，所以我用真实事件循环驱动了真实的
第二处改动也已确认：绑定一个 debug session 后，我从磁盘上的日志读到了新增的那一行—— 4 · 规范检查
合并决策备注
环境： macOS（darwin 24.6.0）· Node 22.23.1 · vitest 3.2.4 · 隔离 worktree @ Verification performed locally by the maintainer; screenshots are real terminal output rendered for readability. |
…raise timeout to 5s (QwenLM#7409) * fix(cli): soften update-check failure UX and raise timeout budget (QwenLM#7049) The startup update check now surfaces failures as a soft warning with the concrete failure reason, instead of an alarming red error: - Raise FETCH_TIMEOUT_MS from 2s to 5s, matching comparable CLIs, so slow mirrors and corporate proxies stop tripping the check. - Emit startup check failures on 'update-failed' with severity 'warning'; setUpdateHandler renders them as a yellow warning. Actual update install failures keep error severity. - Add classifyUpdateCheckError/describeUpdateCheckFailure so messages say what happened - timeout, unreachable registry, or registry error, e.g. "Update check skipped (registry unreachable) - run /update to retry." - /update keeps loud error styling but now includes the failure reason. - Update en/zh/zh-TW locales; drop the now-unused generic failure key. This is not a revert of QwenLM#6857/QwenLM#6887 - the loud-failure direction stays. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(cli): translate new update-check failure keys for remaining locales Review follow-up: the previous commit removed the old generic failure key from ca/de/fr/ja/pt/ru but only added the new parameterized keys to en/zh/zh-TW, leaving those six locales falling back to English. Add the translations, reusing each locale's existing registry/update terminology. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: ComplexSimply <rudy.arrowsong@gmail.com> Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
3 participants
What this PR does
Wires the existing
FETCH_TIMEOUT_MS = 2000constant inpackages/cli/src/ui/utils/updateCheck.tsup toupdate-notifier'sfetchInfo()— the option didn't exist upstream, so the constant was dead code and/updatehad no bound. Race the fetch against the timer and surface a newUpdateCheckTimeoutErrorwhen it fires, so/updatereturns the existing'error'status instead of silently reporting'up to date'. Also log thefetchInforeturn value under theUPDATE_CHECKdebug tag so the next round of reports can distinguish "registry returned the wrong version" from "we compared it incorrectly" without more speculation.Why it's needed
Users report
/updatesaying "Qwen Code 0.19.9 is up to date!" whilenpm view @qwen-code/qwen-code@latest versionon the same machine returns0.19.10. The triage on #6857 confirmedcheckForUpdatesDetailed()reachesreturn { status: 'up-to-date' }— meaningfetchInfo()either returned the current version or resolved late enough that a stale configstore cache was used, and the failure was swallowed silently.Two things make the failure invisible today:
FETCH_TIMEOUT_MSwas never applied.update-notifier@7.3.x'sfetchInfo()takes no timeout option, and the exportedFETCH_TIMEOUT_MSwas never passed anywhere. On slow / unreachable registries (corporate proxies, offline networks, scoped.npmrcmirrors without auth), the check hangs long enough for the caller to drop it — but the'error'code path never fires, so the user only sees the fallback message.debugLogger.warnruns only when an exception escapes, so a return of{ latest: '0.19.9' }in the exact "up to date but shouldn't be" case leaves nothing to bisect.This PR does the two smallest things that make future reports diagnosable and cut off the silent-hang failure mode. It intentionally does not replace
update-notifier'sfetchInfo()with a directpackage-json/npm viewcall — that swap is a larger design change that the triage flagged as a separate option, and this PR keeps the fix scoped to plumbing an already-declared constant.Reviewer Test Plan
How to verify
Unit coverage lives in
packages/cli/src/ui/utils/updateCheck.test.tsand asserts both directions of the race:returns a detailed error when fetchInfo does not resolve within FETCH_TIMEOUT_MS— mocksfetchInfoas a promise that never resolves, advances fake timers past 2000 ms, expects the result to be{ status: 'error', error: UpdateCheckTimeoutError, currentVersion }.still resolves the update path when fetchInfo returns before the timeout— guards against the timer accidentally firing on a healthy fast fetch (which would silently drop every real/updateto error).Full suites confirmed locally with
npx vitest run:packages/cli/src/ui/utils/updateCheck.test.ts— 17/17 pass (15 pre-existing + 2 new)packages/cli/src/commands/update.test.ts— 10/10 passpackages/cli/src/ui/commands/update-command.test.ts— 15/15 passpackages/cli/src/startup/startup-prefetch.test.ts— 21/21 passpackages/cli/src/utils/handleAutoUpdate.test.ts— 24/24 passFor a manual smoke: set
QWEN_DEBUG=UPDATE_CHECK, run/updateon any current version, and confirm aUPDATE_CHECK DEBUGline reporting thefetchInforeturn value now appears in the debug log; if run on a network that can't reachregistry.npmjs.orgwithin 2 s (e.g. block outbound toregistry.npmjs.org), the result should now beerrorwith anUpdateCheckTimeoutError, notup-to-date.Evidence (Before & After)
N/A — non-user-visible behavior change (debug log line + failure-path routing).
Tested on
Environment (optional)
npx vitest runfor the affected suites. No live-registry probe.Risk & Scope
fetchInfo()that resolves in more than 2 s on a slow-but-reachable network will now be reported aserrorinstead of the previous hang / fallback. That is the intended behavior — the user gets a real error message instead of an invisible failure — but if the timeout turns out to be too tight in practice, tuningFETCH_TIMEOUT_MSis a one-line follow-up. Kept the existing 2 s value to avoid mixing the wiring fix with a value bump.fetchInfo()sometimes returns a stalelatestin some environments (proxy, corporate.npmrc,package-jsonscoped-registry handling). The new debug log is the tool for that investigation; the swap to a direct registry call the triage sketched is a separate change.UpdateCheckTimeoutError); theUpdateCheckResultunion already had'error'.Linked Issues
Refs #6857
中文说明
这个 PR 做了什么
把
packages/cli/src/ui/utils/updateCheck.ts里已有但从未被使用的FETCH_TIMEOUT_MS = 2000常量真正接上update-notifier的fetchInfo()——因为该库上游没有 timeout 选项，这个常量此前是死代码，/update也就没有任何上限。用Promise.race与定时器竞速，超时时抛出新增的UpdateCheckTimeoutError，让/update走已有的'error'状态而非静默报"up to date"。同时把fetchInfo的返回值记录到UPDATE_CHECKdebug tag 下，方便下次收到报告时区分"registry 返回错了版本"与"比较逻辑错了"，而不必继续凭猜。为什么需要
用户报告
/update在 v0.19.9 上说"Qwen Code 0.19.9 is up to date!"，但同一台机器npm view @qwen-code/qwen-code@latest version返回0.19.10。#6857 的 triage 确认代码走到了return { status: 'up-to-date' }——要么fetchInfo()返回了当前版本号，要么响应太慢被上游用了陈旧的 configstore 缓存，且失败被静默吞掉。现在有两点让这类失败不可见：
FETCH_TIMEOUT_MS从没被真正应用。update-notifier@7.3.x的fetchInfo()不接受 timeout 选项，导出的FETCH_TIMEOUT_MS也没传给任何地方。在慢的 / 不可达的 registry（企业代理、离线网络、.npmrcscoped mirror 未配置 auth）上，请求悬挂到调用方放弃——但'error'分支从未触发，用户只看到 fallback 提示。debugLogger.warn只在异常逃出时打印，遇到fetchInfo()返回{ latest: '0.19.9' }这种真正 "看似正常却错误" 的情况时无从 bisect。本 PR 做两件把未来报告可诊断化 + 切断静默 hang 失败路径的最小改动。刻意不把
update-notifier的fetchInfo()换成对package-json/npm view的直接调用——那是 triage 里另一个更大的设计变更，本 PR 把修复限定在把已声明的常量接上。复审测试计划
如何验证
单元覆盖位于
packages/cli/src/ui/utils/updateCheck.test.ts，同时验证 race 的两个方向：returns a detailed error when fetchInfo does not resolve within FETCH_TIMEOUT_MS—— mockfetchInfo永远不 resolve，用 fake timer 推进 2000ms+，期望结果为{ status: 'error', error: UpdateCheckTimeoutError, currentVersion }。still resolves the update path when fetchInfo returns before the timeout—— 防止定时器在健康的快速请求上误触（那样每次真实/update都会被静默降级为 error）。npx vitest run本地全绿：packages/cli/src/ui/utils/updateCheck.test.ts17/17（原 15 + 新增 2）packages/cli/src/commands/update.test.ts10/10packages/cli/src/ui/commands/update-command.test.ts15/15packages/cli/src/startup/startup-prefetch.test.ts21/21packages/cli/src/utils/handleAutoUpdate.test.ts24/24手工冒烟：设置
QWEN_DEBUG=UPDATE_CHECK，运行/update于任意版本，确认 debug log 里多了一条UPDATE_CHECK DEBUG行报告fetchInfo的返回值；如果限制 2s 内不可达registry.npmjs.org，结果应变成error携带UpdateCheckTimeoutError而不是up-to-date。证据（Before & After）
N/A——非用户可见的行为变更（debug 日志 + 失败路径路由）。
测试环境
环境（可选）
npx vitest run相关测试集。未做真实 registry 探测。风险与范围
fetchInfo()现在会被报告为error，而不是此前那种悬挂 / fallback。这就是刻意的行为改变——用户会看到真实错误提示而非隐形失败——如果实际使用中发现 2s 太紧，调整FETCH_TIMEOUT_MS只是一行 follow-up。本 PR 保留 2s 原值，避免把常量接线的修复与调值混在一起。fetchInfo()在某些环境下返回陈旧的latest这个更深层问题（代理、企业.npmrc、package-json的 scoped registry 处理）。新增的 debug 日志正是为此提供工具；triage 里草拟的直接调用 registry 的方案是独立的改动。UpdateCheckTimeoutError）；UpdateCheckResultunion 早已含'error'。关联 Issue
Refs #6857