fix: restore metadata request timeout (#289)

Author: Perdolique

schema.json

@@ -182,6 +182,12 @@
       "minimum": 1,
       "description": "Number of concurrent requests when resolving dependencies.",
       "default": 10
+    },
+    "requestTimeout": {
+      "type": "integer",
+      "minimum": 1,
+      "description": "Request timeout in milliseconds when fetching package metadata.",
+      "default": 5000
     }
   }
 }

src/cli.ts

@@ -40,6 +40,7 @@ cli
   .option('--maturity-period [days]', 'wait period in days before upgrading to newly released packages (default: 7 when flag is used, 0 when not used)')
   .option('--maturity-period-exclude <deps>', 'dependencies to exclude from the maturity period filter')
   .option('--concurrency <requests>', 'number of concurrent requests when resolving dependencies', { default: 10 })
+  .option('--request-timeout <ms>', 'request timeout in milliseconds when fetching package metadata', { default: 5000 })
   .action(async (mode: RangeMode | undefined, options: Partial<CheckOptions>) => {
     if (mode) {
       if (!MODE_CHOICES.includes(mode)) {

src/constants.ts

@@ -38,6 +38,7 @@ export const DEFAULT_CHECK_OPTIONS: CheckOptions = {
   all: false,
   json: false,
   sort: 'diff-asc',
+  requestTimeout: 5000,
   group: true,
   includeLocked: false,
   nodecompat: true,

src/io/resolves.ts

@@ -21,6 +21,7 @@ const debug = {
 
 let cache: Record<string, { cacheTime: number, data: PackageData }> = {}
 let cacheChanged = false
+const inflightRequests = new Map<string, Promise<PackageData>>()
 
 const cacheDir = resolve(os.tmpdir(), 'taze')
 const cachePath = resolve(cacheDir, 'cache.json')
@@ -58,7 +59,7 @@ export async function dumpCache() {
   }
 }
 
-export async function getPackageData(name: string, protocol: Protocol = 'npm', cwd?: string): Promise<PackageData> {
+export async function getPackageData(name: string, protocol: Protocol = 'npm', cwd?: string, requestTimeout?: number): Promise<PackageData> {
   let error: any
   const cacheName = `${protocol}:${name}`
 
@@ -72,25 +73,45 @@ export async function getPackageData(name: string, protocol: Protocol = 'npm', c
     }
   }
 
-  try {
-    debug.resolve(`resolving ${cacheName}`)
-    const data = protocol === 'jsr' ? await fetchJsrPackageMeta(name) : await fetchPackage(name, false, cwd)
+  const inflightRequest = inflightRequests.get(cacheName)
 
-    if (data) {
-      cache[cacheName] = { data, cacheTime: now() }
-      cacheChanged = true
-      return data
-    }
-  }
-  catch (e) {
-    error = e
+  if (inflightRequest) {
+    debug.cache(`in-flight hit for ${cacheName}`)
+    return inflightRequest
   }
 
-  return {
-    tags: {},
-    versions: [],
-    error: error?.statusCode?.toString() || error,
-    deprecated: {},
+  const request = (async () => {
+    try {
+      debug.resolve(`resolving ${cacheName}`)
+      const data = protocol === 'jsr'
+        ? await fetchJsrPackageMeta(name, requestTimeout)
+        : await fetchPackage(name, false, cwd, requestTimeout)
+
+      if (data) {
+        cache[cacheName] = { data, cacheTime: now() }
+        cacheChanged = true
+        return data
+      }
+    }
+    catch (e) {
+      error = e
+    }
+
+    return {
+      tags: {},
+      versions: [],
+      error: error?.statusCode?.toString() || error,
+      deprecated: {},
+    }
+  })()
+
+  inflightRequests.set(cacheName, request)
+
+  try {
+    return await request
+  }
+  finally {
+    inflightRequests.delete(cacheName)
   }
 }
 
@@ -276,7 +297,7 @@ export async function resolveDependency(
     resolvedName = packages.pop() ?? dep.name
   }
 
-  const pkgData = await getPackageData(resolvedName, dep.protocol, options.cwd)
+  const pkgData = await getPackageData(resolvedName, dep.protocol, options.cwd, options.requestTimeout)
   const { error, deprecated } = pkgData
 
   dep.pkgData = pkgData

src/types.ts

@@ -149,6 +149,12 @@ export interface CheckOptions extends CommonOptions {
    * @default 10
    */
   concurrency?: number
+  /**
+   * Request timeout in milliseconds when fetching package metadata
+   *
+   * @default 5000
+   */
+  requestTimeout?: number
   /**
    * Group dependencies by source, e.g. dependencies, devDependencies, etc.
    *

src/utils/packument.ts

@@ -4,7 +4,7 @@ import process from 'node:process'
 import { getVersions } from 'get-npm-meta'
 import { fetch as ofetch } from 'ofetch'
 
-const TIMEOUT = 5000
+const DEFAULT_REQUEST_TIMEOUT = 5000
 const JSR_API_REGISTRY = 'https://jsr.io/'
 const USER_AGENT = `taze@npm node/${process.version}`
 
@@ -48,37 +48,54 @@ const fetchWithUserAgent: typeof fetch = (input, init) => {
   return ofetch(input, { ...init, headers })
 }
 
-export async function fetchPackage(spec: string, force: boolean = false, cwd?: string): Promise<PackageData> {
-  const data = await Promise.race([
-    getVersions(spec, {
-      cwd,
-      force,
-      fetch: fetchWithUserAgent,
-      metadata: true,
-      throw: false,
-    }),
-    new Promise<Packument>(
-      (_, reject) => setTimeout(() => reject(new Error(`Timeout requesting "${spec}"`)), TIMEOUT),
-    ),
-  ]) as PackageVersionsInfoWithMetadata
+function createTimeoutError(name: string) {
+  return new Error(`Timeout requesting "${name}"`)
+}
+
+async function withRequestTimeout<T>(name: string, timeout: number, run: (signal: AbortSignal) => Promise<T>) {
+  const controller = new AbortController()
+  const timeoutError = createTimeoutError(name)
+  let timeoutId: ReturnType<typeof setTimeout> | undefined
+  const timeoutPromise = new Promise<never>((_, reject) => {
+    timeoutId = setTimeout(() => {
+      reject(timeoutError)
+      controller.abort(timeoutError)
+    }, timeout)
+  })
+
+  try {
+    return await Promise.race([run(controller.signal), timeoutPromise])
+  }
+  finally {
+    if (timeoutId)
+      clearTimeout(timeoutId)
+
+    controller.abort(timeoutError)
+  }
+}
+
+export async function fetchPackage(spec: string, force: boolean = false, cwd?: string, requestTimeout: number = DEFAULT_REQUEST_TIMEOUT): Promise<PackageData> {
+  const data = await withRequestTimeout(spec, requestTimeout, signal => getVersions(spec, {
+    cwd,
+    force,
+    fetch: (input, init) => fetchWithUserAgent(input, { ...init, signal }),
+    metadata: true,
+    throw: false,
+  })) as PackageVersionsInfoWithMetadata
 
   if ('error' in data)
     throw new Error(`Failed to fetch package "${spec}": ${data.error}`)
 
   return toPackageData(data)
 }
 
-export async function fetchJsrPackageMeta(name: string): Promise<PackageData> {
-  const meta = await Promise.race([
-    fetchWithUserAgent(new URL(`${name}/meta.json`, JSR_API_REGISTRY), {
-      headers: {
-        accept: 'application/json',
-      },
-    }).then(r => r.json()),
-    new Promise<JsrPackageMeta>(
-      (_, reject) => setTimeout(() => reject(new Error(`Timeout requesting "${name}"`)), TIMEOUT),
-    ),
-  ]) as JsrPackageMeta
+export async function fetchJsrPackageMeta(name: string, requestTimeout: number = DEFAULT_REQUEST_TIMEOUT): Promise<PackageData> {
+  const meta = await withRequestTimeout(name, requestTimeout, signal => fetchWithUserAgent(new URL(`${name}/meta.json`, JSR_API_REGISTRY), {
+    signal,
+    headers: {
+      accept: 'application/json',
+    },
+  }).then(r => r.json())) as JsrPackageMeta
 
   return {
     versions: Object.keys(meta.versions),

test/cli.test.ts

@@ -19,3 +19,12 @@ it('taze cli should accept --concurrency option', async () => {
   expect(proc.stderr).toBe('')
   expect(proc.exitCode).toBe(0)
 })
+
+it('taze cli should accept --request-timeout option', async () => {
+  const binPath = resolve(__dirname, '../bin/taze.mjs')
+
+  const proc = await exec(process.execPath, [binPath, '--request-timeout', '15000'], { throwOnError: false })
+
+  expect(proc.stderr).toBe('')
+  expect(proc.exitCode).toBe(0)
+})

test/packument.test.ts

@@ -0,0 +1,94 @@
+import { afterEach, expect, it, vi } from 'vitest'
+import { fetchPackage } from '../src/utils/packument'
+
+const { getVersionsMock, ofetchMock } = vi.hoisted(() => ({
+  getVersionsMock: vi.fn(),
+  ofetchMock: vi.fn(),
+}))
+
+vi.mock('get-npm-meta', () => ({
+  getVersions: getVersionsMock,
+}))
+
+vi.mock('ofetch', () => ({
+  fetch: ofetchMock,
+}))
+
+afterEach(() => {
+  vi.restoreAllMocks()
+  vi.useRealTimers()
+  getVersionsMock.mockReset()
+  ofetchMock.mockReset()
+})
+
+it('aborts npm metadata requests when timeout is exceeded', async () => {
+  vi.useFakeTimers()
+
+  let aborted = false
+
+  ofetchMock.mockImplementation((_input, init) => new Promise((_, reject) => {
+    const signal = init?.signal as AbortSignal | undefined
+
+    signal?.addEventListener('abort', () => {
+      aborted = true
+      reject(new DOMException('Aborted', 'AbortError'))
+    })
+  }))
+
+  getVersionsMock.mockImplementation((_spec, options) => options.fetch('https://registry.npmjs.org/drizzle-orm'))
+
+  const promise = fetchPackage('drizzle-orm', false, undefined, 25)
+  const rejection = expect(promise).rejects.toThrow('Timeout requesting "drizzle-orm"')
+
+  await vi.advanceTimersByTimeAsync(25)
+
+  await rejection
+  expect(aborted).toBe(true)
+})
+
+it('times out promptly even when upstream ignores abort settlement', async () => {
+  vi.useFakeTimers()
+
+  let aborted = false
+
+  ofetchMock.mockImplementation((_input, init) => new Promise(() => {
+    const signal = init?.signal as AbortSignal | undefined
+
+    signal?.addEventListener('abort', () => {
+      aborted = true
+    })
+  }))
+
+  getVersionsMock.mockImplementation((_spec, options) => {
+    options.fetch('https://registry.npmjs.org/drizzle-orm')
+    return new Promise(() => {})
+  })
+
+  const promise = fetchPackage('drizzle-orm', false, undefined, 25)
+  const rejection = expect(promise).rejects.toThrow('Timeout requesting "drizzle-orm"')
+
+  await vi.advanceTimersByTimeAsync(25)
+
+  await rejection
+  expect(aborted).toBe(true)
+})
+
+it('clears the timeout after a successful request', async () => {
+  vi.useFakeTimers()
+
+  getVersionsMock.mockResolvedValue({
+    distTags: { latest: '1.0.0' },
+    timeCreated: '2024-01-01T00:00:00.000Z',
+    timeModified: '2024-01-01T00:00:00.000Z',
+    versionsMeta: {
+      '1.0.0': {},
+    },
+  })
+
+  await expect(fetchPackage('drizzle-orm', false, undefined, 25)).resolves.toMatchObject({
+    tags: { latest: '1.0.0' },
+    versions: ['1.0.0'],
+  })
+
+  expect(vi.getTimerCount()).toBe(0)
+})

test/resolves-cache.test.ts

@@ -0,0 +1,44 @@
+import type { PackageData } from '../src/types'
+import { afterEach, expect, it, vi } from 'vitest'
+
+const { fetchJsrPackageMetaMock, fetchPackageMock } = vi.hoisted(() => ({
+  fetchJsrPackageMetaMock: vi.fn(),
+  fetchPackageMock: vi.fn(),
+}))
+
+vi.mock('../src/utils/packument.ts', () => ({
+  fetchJsrPackageMeta: fetchJsrPackageMetaMock,
+  fetchPackage: fetchPackageMock,
+}))
+
+afterEach(() => {
+  vi.restoreAllMocks()
+  vi.resetModules()
+  fetchJsrPackageMetaMock.mockReset()
+  fetchPackageMock.mockReset()
+})
+
+it('dedupes concurrent requests for the same package', async () => {
+  const packageData: PackageData = {
+    tags: { latest: '5.8.3' },
+    versions: ['5.8.3'],
+  }
+
+  let resolveFetch: ((value: PackageData) => void) | undefined
+  const pendingFetch = new Promise<PackageData>((resolve) => {
+    resolveFetch = resolve
+  })
+
+  fetchPackageMock.mockReturnValue(pendingFetch)
+
+  const { getPackageData } = await import('../src/io/resolves')
+  const firstRequest = getPackageData('typescript', 'npm', '/tmp', 15000)
+  const secondRequest = getPackageData('typescript', 'npm', '/tmp', 15000)
+
+  expect(fetchPackageMock).toHaveBeenCalledTimes(1)
+  expect(fetchPackageMock).toHaveBeenCalledWith('typescript', false, '/tmp', 15000)
+
+  resolveFetch?.(packageData)
+
+  await expect(Promise.all([firstRequest, secondRequest])).resolves.toStrictEqual([packageData, packageData])
+})