From 80eb213a4580207cd45e53fd1a57030110dae30f Mon Sep 17 00:00:00 2001 From: keaixiaozhu Date: Tue, 14 Jul 2026 20:03:57 +0800 Subject: [PATCH 1/2] fix(cli): apply FETCH_TIMEOUT_MS to /update version check and log fetchInfo results (#6857) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 #6857 --- packages/cli/src/ui/utils/updateCheck.test.ts | 57 ++++++++++++++++++- packages/cli/src/ui/utils/updateCheck.ts | 53 ++++++++++++++++- 2 files changed, 106 insertions(+), 4 deletions(-) diff --git a/packages/cli/src/ui/utils/updateCheck.test.ts b/packages/cli/src/ui/utils/updateCheck.test.ts index 4ae6070eeae..9416a7045cb 100644 --- a/packages/cli/src/ui/utils/updateCheck.test.ts +++ b/packages/cli/src/ui/utils/updateCheck.test.ts @@ -5,7 +5,12 @@ */ import { vi, describe, it, expect, beforeEach } from 'vitest'; -import { checkForUpdates, checkForUpdatesDetailed } from './updateCheck.js'; +import { + checkForUpdates, + checkForUpdatesDetailed, + FETCH_TIMEOUT_MS, + UpdateCheckTimeoutError, +} from './updateCheck.js'; const getPackageJson = vi.hoisted(() => vi.fn()); vi.mock('../../utils/package.js', () => ({ @@ -254,4 +259,54 @@ describe('checkForUpdates', () => { expect(result?.update.latest).toBe('1.2.3-nightly.2'); }); }); + + describe('fetchInfo timeout (#6857)', () => { + it('returns a detailed error when fetchInfo does not resolve within FETCH_TIMEOUT_MS', async () => { + // update-notifier's fetchInfo() takes no timeout option, so an + // unreachable registry (proxy, offline, corporate mirror without + // scoped .npmrc auth) would hang the check. We race it against a + // bounded timer instead — this asserts the timer actually fires and + // surfaces a real error rather than silently reporting "up to date". + getPackageJson.mockResolvedValue({ + name: 'test-package', + version: '1.0.0', + }); + updateNotifier.mockReturnValue({ + // never resolves + fetchInfo: vi.fn().mockReturnValue(new Promise(() => {})), + }); + + const resultPromise = checkForUpdatesDetailed(); + await vi.advanceTimersByTimeAsync(FETCH_TIMEOUT_MS + 1); + const result = await resultPromise; + + expect(result.status).toBe('error'); + if (result.status === 'error') { + expect(result.error).toBeInstanceOf(UpdateCheckTimeoutError); + expect(result.error.message).toContain(`${FETCH_TIMEOUT_MS}ms`); + expect(result.currentVersion).toBe('1.0.0'); + } + }); + + it('still resolves the update path when fetchInfo returns before the timeout', async () => { + // Guards against the timer accidentally firing on a healthy fast fetch — + // if it did, every /update call would silently drop back to error. + getPackageJson.mockResolvedValue({ + name: 'test-package', + version: '1.0.0', + }); + updateNotifier.mockReturnValue({ + fetchInfo: vi + .fn() + .mockResolvedValue({ current: '1.0.0', latest: '1.1.0' }), + }); + + const result = await checkForUpdatesDetailed(); + + expect(result.status).toBe('update'); + if (result.status === 'update') { + expect(result.info.update.latest).toBe('1.1.0'); + } + }); + }); }); diff --git a/packages/cli/src/ui/utils/updateCheck.ts b/packages/cli/src/ui/utils/updateCheck.ts index 748e9fdb22d..5d8461ea722 100644 --- a/packages/cli/src/ui/utils/updateCheck.ts +++ b/packages/cli/src/ui/utils/updateCheck.ts @@ -15,6 +15,42 @@ const debugLogger = createDebugLogger('UPDATE_CHECK'); export const FETCH_TIMEOUT_MS = 2000; +/** + * Sentinel error thrown when `fetchInfo()` does not resolve within + * `FETCH_TIMEOUT_MS`. `update-notifier`'s `fetchInfo()` does not accept a + * timeout option, so slow / unreachable registries (corporate proxies, offline + * networks, DNS failures) would otherwise hang the check indefinitely or fall + * through to a stale configstore cache. Race the call against a bounded timer + * and surface a real error so `/update` can report "check failed" instead of + * silently returning "up to date". Related: #6857. + */ +export class UpdateCheckTimeoutError extends Error { + constructor(timeoutMs: number) { + super(`update-notifier fetchInfo timed out after ${timeoutMs}ms`); + this.name = 'UpdateCheckTimeoutError'; + } +} + +async function fetchInfoWithTimeout( + notifier: { fetchInfo(): UpdateInfo | Promise }, + timeoutMs: number, +): Promise { + let timer: ReturnType | undefined; + try { + return await Promise.race([ + Promise.resolve(notifier.fetchInfo()), + new Promise((_, reject) => { + timer = setTimeout( + () => reject(new UpdateCheckTimeoutError(timeoutMs)), + timeoutMs, + ); + }), + ]); + } finally { + if (timer !== undefined) clearTimeout(timer); + } +} + export interface UpdateObject { message: string; update: UpdateInfo; @@ -77,10 +113,14 @@ export async function checkForUpdatesDetailed(): Promise { if (isNightly) { const [nightlyUpdateInfo, latestUpdateInfo] = await Promise.all([ - createNotifier('nightly').fetchInfo(), - createNotifier('latest').fetchInfo(), + fetchInfoWithTimeout(createNotifier('nightly'), FETCH_TIMEOUT_MS), + fetchInfoWithTimeout(createNotifier('latest'), FETCH_TIMEOUT_MS), ]); + debugLogger.debug( + `fetchInfo returned nightly=${JSON.stringify(nightlyUpdateInfo)} latest=${JSON.stringify(latestUpdateInfo)} for current=${version}`, + ); + const bestUpdate = getBestAvailableUpdate( nightlyUpdateInfo, latestUpdateInfo, @@ -99,7 +139,14 @@ export async function checkForUpdatesDetailed(): Promise { }; } } else { - const updateInfo = await createNotifier('latest').fetchInfo(); + const updateInfo = await fetchInfoWithTimeout( + createNotifier('latest'), + FETCH_TIMEOUT_MS, + ); + + debugLogger.debug( + `fetchInfo returned ${JSON.stringify(updateInfo)} for current=${version}`, + ); if (updateInfo && semver.gt(updateInfo.latest, version)) { return { From 51578836af550d71b67798c66ed52a4a81c7470f Mon Sep 17 00:00:00 2001 From: keaixiaozhu Date: Tue, 14 Jul 2026 21:56:06 +0800 Subject: [PATCH 2/2] fix(cli): carry dist-tag on UpdateCheckTimeoutError and cover nightly timeout paths MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address bot review on #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`. --- packages/cli/src/ui/utils/updateCheck.test.ts | 60 +++++++++++++++++++ packages/cli/src/ui/utils/updateCheck.ts | 29 +++++++-- 2 files changed, 83 insertions(+), 6 deletions(-) diff --git a/packages/cli/src/ui/utils/updateCheck.test.ts b/packages/cli/src/ui/utils/updateCheck.test.ts index 9416a7045cb..a2b1d0d8589 100644 --- a/packages/cli/src/ui/utils/updateCheck.test.ts +++ b/packages/cli/src/ui/utils/updateCheck.test.ts @@ -284,6 +284,9 @@ describe('checkForUpdates', () => { if (result.status === 'error') { expect(result.error).toBeInstanceOf(UpdateCheckTimeoutError); expect(result.error.message).toContain(`${FETCH_TIMEOUT_MS}ms`); + // Non-nightly path only queries the `latest` dist-tag; the message + // must name it so oncall can tell which registry endpoint stalled. + expect(result.error.message).toContain('for latest'); expect(result.currentVersion).toBe('1.0.0'); } }); @@ -308,5 +311,62 @@ describe('checkForUpdates', () => { expect(result.info.update.latest).toBe('1.1.0'); } }); + + it('surfaces a timeout when only the nightly dist-tag stalls', async () => { + // The nightly path fires `latest` and `nightly` fetches concurrently via + // Promise.all — if the timer wiring is wrong (e.g. only the outer race + // has one, or the reject reaches Promise.all and Promise.all doesn't + // propagate), a single stalled fetch would let /update silently degrade. + // Assert Promise.all propagates the timeout AND names the exact dist-tag + // that stalled so oncall reading logs can point at the endpoint. + getPackageJson.mockResolvedValue({ + name: 'test-package', + version: '1.0.0-nightly.1', + }); + updateNotifier.mockImplementation(({ distTag }) => ({ + fetchInfo: () => + distTag === 'nightly' + ? new Promise(() => {}) // never resolves + : Promise.resolve({ + current: '1.0.0-nightly.1', + latest: '1.0.0', + }), + })); + + const resultPromise = checkForUpdatesDetailed(); + await vi.advanceTimersByTimeAsync(FETCH_TIMEOUT_MS + 1); + const result = await resultPromise; + + expect(result.status).toBe('error'); + if (result.status === 'error') { + expect(result.error).toBeInstanceOf(UpdateCheckTimeoutError); + expect(result.error.message).toContain('for nightly'); + expect(result.currentVersion).toBe('1.0.0-nightly.1'); + } + }); + + it('surfaces a timeout when both nightly dist-tags stall', async () => { + // Full outage / offline network — both fetches hang, both timers fire. + // The first rejection Promise.all sees wins; assert only that we get a + // typed UpdateCheckTimeoutError for one of the two dist-tags (either is + // a valid symptom of the same failure). + getPackageJson.mockResolvedValue({ + name: 'test-package', + version: '1.0.0-nightly.1', + }); + updateNotifier.mockImplementation(() => ({ + fetchInfo: () => new Promise(() => {}), + })); + + const resultPromise = checkForUpdatesDetailed(); + await vi.advanceTimersByTimeAsync(FETCH_TIMEOUT_MS + 1); + const result = await resultPromise; + + expect(result.status).toBe('error'); + if (result.status === 'error') { + expect(result.error).toBeInstanceOf(UpdateCheckTimeoutError); + expect(result.error.message).toMatch(/for (nightly|latest)/); + } + }); }); }); diff --git a/packages/cli/src/ui/utils/updateCheck.ts b/packages/cli/src/ui/utils/updateCheck.ts index 5d8461ea722..3cea93cc731 100644 --- a/packages/cli/src/ui/utils/updateCheck.ts +++ b/packages/cli/src/ui/utils/updateCheck.ts @@ -22,18 +22,26 @@ export const FETCH_TIMEOUT_MS = 2000; * networks, DNS failures) would otherwise hang the check indefinitely or fall * through to a stale configstore cache. Race the call against a bounded timer * and surface a real error so `/update` can report "check failed" instead of - * silently returning "up to date". Related: #6857. + * silently returning "up to date". The `distTag` is carried on the message so + * an oncall reading logs can tell which registry endpoint stalled — the + * nightly path fires two concurrent fetches, and only one of them may be + * blocked (e.g. a corporate proxy that lets `nightly` through but not + * `latest`). Related: #6857. */ export class UpdateCheckTimeoutError extends Error { - constructor(timeoutMs: number) { - super(`update-notifier fetchInfo timed out after ${timeoutMs}ms`); + readonly distTag?: string; + constructor(timeoutMs: number, distTag?: string) { + const suffix = distTag ? ` for ${distTag}` : ''; + super(`update-notifier fetchInfo timed out after ${timeoutMs}ms${suffix}`); this.name = 'UpdateCheckTimeoutError'; + this.distTag = distTag; } } async function fetchInfoWithTimeout( notifier: { fetchInfo(): UpdateInfo | Promise }, timeoutMs: number, + distTag?: string, ): Promise { let timer: ReturnType | undefined; try { @@ -41,7 +49,7 @@ async function fetchInfoWithTimeout( Promise.resolve(notifier.fetchInfo()), new Promise((_, reject) => { timer = setTimeout( - () => reject(new UpdateCheckTimeoutError(timeoutMs)), + () => reject(new UpdateCheckTimeoutError(timeoutMs, distTag)), timeoutMs, ); }), @@ -113,8 +121,16 @@ export async function checkForUpdatesDetailed(): Promise { if (isNightly) { const [nightlyUpdateInfo, latestUpdateInfo] = await Promise.all([ - fetchInfoWithTimeout(createNotifier('nightly'), FETCH_TIMEOUT_MS), - fetchInfoWithTimeout(createNotifier('latest'), FETCH_TIMEOUT_MS), + fetchInfoWithTimeout( + createNotifier('nightly'), + FETCH_TIMEOUT_MS, + 'nightly', + ), + fetchInfoWithTimeout( + createNotifier('latest'), + FETCH_TIMEOUT_MS, + 'latest', + ), ]); debugLogger.debug( @@ -142,6 +158,7 @@ export async function checkForUpdatesDetailed(): Promise { const updateInfo = await fetchInfoWithTimeout( createNotifier('latest'), FETCH_TIMEOUT_MS, + 'latest', ); debugLogger.debug(