-
Notifications
You must be signed in to change notification settings - Fork 3k
fix(cli): apply FETCH_TIMEOUT_MS to /update version check and log fetchInfo results (#6857) #6887
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
80eb213
c1670a5
5157883
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -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; | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
Comment on lines
+31
to
+39
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] The timeout error message ("fetchInfo timed out after 2000ms") carries no indication of which dist-tag (
Suggested change
Then pass the dist-tag from — qwen3.7-max via Qwen Code /review
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch — applied in |
||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| async function fetchInfoWithTimeout( | ||||||||||||||||||||||||||||
| notifier: { fetchInfo(): UpdateInfo | Promise<UpdateInfo> }, | ||||||||||||||||||||||||||||
| timeoutMs: number, | ||||||||||||||||||||||||||||
| distTag?: string, | ||||||||||||||||||||||||||||
| ): Promise<UpdateInfo> { | ||||||||||||||||||||||||||||
| let timer: ReturnType<typeof setTimeout> | undefined; | ||||||||||||||||||||||||||||
| try { | ||||||||||||||||||||||||||||
| return await Promise.race([ | ||||||||||||||||||||||||||||
| Promise.resolve(notifier.fetchInfo()), | ||||||||||||||||||||||||||||
| new Promise<never>((_, 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<UpdateCheckResult> { | |||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| 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<UpdateCheckResult> { | |||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
| } 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 { | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Suggestion] The timeout tests only exercise the non-nightly (single-fetch) path. The nightly branch uses
Promise.allwith two concurrentfetchInfoWithTimeoutcalls — a more complex interaction that has no timeout test. If a bug existed in the nightly path'sPromise.all+ timeout interaction, it would go undetected. — Concrete cost: the nightlyPromise.allpath 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 bothfetchInfomocks return a never-resolving promise, advance timers pastFETCH_TIMEOUT_MS, and assertstatus: 'error'withUpdateCheckTimeoutError.— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch — added in
51578836a. Two new tests cover the nightlyPromise.alltimeout 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).