From 0957e18ea2347ab2bf3cf7dc0be78da306c90187 Mon Sep 17 00:00:00 2001 From: C0d3N1nja97342 Date: Wed, 15 Jul 2026 11:54:35 +0800 Subject: [PATCH] fix(cli): apply FETCH_TIMEOUT_MS to /update version check and log fetchInfo results (#6857) (#6887) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(cli): apply FETCH_TIMEOUT_MS to /update version check and log fetchInfo results (#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 #6857 * fix(cli): carry dist-tag on UpdateCheckTimeoutError and cover nightly timeout paths 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 | 117 +++++++++++++++++- packages/cli/src/ui/utils/updateCheck.ts | 70 ++++++++++- 2 files changed, 183 insertions(+), 4 deletions(-) diff --git a/packages/cli/src/ui/utils/updateCheck.test.ts b/packages/cli/src/ui/utils/updateCheck.test.ts index 4ae6070eea..a2b1d0d858 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,114 @@ 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`); + // 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'); + } + }); + + 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'); + } + }); + + 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 748e9fdb22..3cea93cc73 100644 --- a/packages/cli/src/ui/utils/updateCheck.ts +++ b/packages/cli/src/ui/utils/updateCheck.ts @@ -15,6 +15,50 @@ 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". 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 { + 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 { + return await Promise.race([ + Promise.resolve(notifier.fetchInfo()), + new Promise((_, reject) => { + timer = setTimeout( + () => reject(new UpdateCheckTimeoutError(timeoutMs, distTag)), + timeoutMs, + ); + }), + ]); + } finally { + if (timer !== undefined) clearTimeout(timer); + } +} + export interface UpdateObject { message: string; update: UpdateInfo; @@ -77,10 +121,22 @@ 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, + 'nightly', + ), + fetchInfoWithTimeout( + createNotifier('latest'), + FETCH_TIMEOUT_MS, + 'latest', + ), ]); + debugLogger.debug( + `fetchInfo returned nightly=${JSON.stringify(nightlyUpdateInfo)} latest=${JSON.stringify(latestUpdateInfo)} for current=${version}`, + ); + const bestUpdate = getBestAvailableUpdate( nightlyUpdateInfo, latestUpdateInfo, @@ -99,7 +155,15 @@ export async function checkForUpdatesDetailed(): Promise { }; } } else { - const updateInfo = await createNotifier('latest').fetchInfo(); + const updateInfo = await fetchInfoWithTimeout( + createNotifier('latest'), + FETCH_TIMEOUT_MS, + 'latest', + ); + + debugLogger.debug( + `fetchInfo returned ${JSON.stringify(updateInfo)} for current=${version}`, + ); if (updateInfo && semver.gt(updateInfo.latest, version)) { return {