fix(npm-registry): cache failed lookups, dedupe error logging

lib/npm-registry.js

@@ -7,8 +7,10 @@ const registryUrl = 'https://registry.npmjs.org/'
 const downloadsUrl = 'https://api.npmjs.org/downloads/point/'
 
 // Native fetch wrapper with error handling
+const fetchTimeoutMs = parseInt(process.env.FETCH_TIMEOUT_MS) || 10000
+
 async function fetchJson (url) {
-  const response = await fetch(url)
+  const response = await fetch(url, { signal: AbortSignal.timeout(fetchTimeoutMs) })
   if (!response.ok) {
     throw new Error(`HTTP ${response.status}: ${response.statusText}`)
   }
@@ -19,18 +21,30 @@ async function fetchJson (url) {
 const cacheMaxSize = parseInt(process.env.CACHE_MAX_SIZE) || 1000
 const cacheDocsTTLMinutes = parseInt(process.env.CACHE_DOCS_TTL_MINUTES) || parseInt(process.env.CACHE_TTL_MINUTES) || 5
 const cacheDownloadsTTLMinutes = parseInt(process.env.CACHE_DOWNLOADS_TTL_MINUTES) || 60
+// Short TTL for caching lookup failures (bad/missing packages), so a client hammering
+// the same non-existent package doesn't force a fresh registry round-trip every time
+const cacheErrorTTLMinutes = parseInt(process.env.CACHE_ERROR_TTL_MINUTES) || 1
 
 // LRU cache with TTL
 const cache = new LRUCache({
   max: cacheMaxSize, // Maximum number of items
-  ttl: 1000 * 60 * Math.max(cacheDocsTTLMinutes, cacheDownloadsTTLMinutes), // Use longest TTL for cache
+  ttl: 1000 * 60 * Math.max(cacheDocsTTLMinutes, cacheDownloadsTTLMinutes, cacheErrorTTLMinutes), // Use longest TTL for cache
   updateAgeOnGet: false,
   updateAgeOnHas: false
 })
 
 const CACHE_TTL = {
   doc: 1000 * 60 * cacheDocsTTLMinutes, // Cache TTL for package docs
-  downloads: 1000 * 60 * cacheDownloadsTTLMinutes // Cache TTL for download counts
+  downloads: 1000 * 60 * cacheDownloadsTTLMinutes, // Cache TTL for download counts
+  error: 1000 * 60 * cacheErrorTTLMinutes // Cache TTL for lookup failures
+}
+
+// Marks a cached value as a failed lookup, so a cache hit can rethrow instead of
+// being mistaken for a successful (if falsy-looking) result
+class CachedError {
+  constructor (err) {
+    this.err = err
+  }
 }
 
 // Log configuration on first use
@@ -45,13 +59,20 @@ function logCacheConfig () {
 async function getCached (key, ttl, loader) {
   const cached = cache.get(key)
   if (cached !== undefined) {
+    if (cached instanceof CachedError) {
+      throw cached.err
+    }
     return cached
   }
 
-  const value = await loader()
-  cache.set(key, value, { ttl })
-
-  return value
+  try {
+    const value = await loader()
+    cache.set(key, value, { ttl })
+    return value
+  } catch (err) {
+    cache.set(key, new CachedError(err), { ttl: CACHE_TTL.error })
+    throw err
+  }
 }
 
 async function loadDoc (pkg) {
@@ -103,13 +124,7 @@ async function getPackageInfo (pkg, options = {}) {
   logCacheConfig() // Log on first use
   const cacheKey = `pkg:${pkg}`
 
-  try {
-    const data = await getCached(cacheKey, CACHE_TTL.doc, () => loadDoc(pkg))
-    return data
-  } catch (err) {
-    log.error(err)
-    throw err
-  }
+  return getCached(cacheKey, CACHE_TTL.doc, () => loadDoc(pkg))
 }
 
 async function getUserPackages (username) {

lib/pkgdata.js

@@ -1,33 +1,24 @@
-import bole from 'bole'
 import npmRegistry from './npm-registry.js'
 
-const log = bole('pkgdata')
-
 async function pkgInfo (pkg, options = {}) {
-  try {
-    const needsDownloads = options.downloads || (options.dataTypes && options.dataTypes.includes('downloads'))
-
-    // Fetch package info and downloads in parallel if both are needed
-    if (needsDownloads) {
-      const [data, downloads] = await Promise.all([
-        npmRegistry.getPackageInfo(pkg, options),
-        npmRegistry.getDownloadCount(pkg)
-      ])
+  const needsDownloads = options.downloads || (options.dataTypes && options.dataTypes.includes('downloads'))
 
-      if (downloads !== null) {
-        data.downloads = downloads
-      }
+  // Fetch package info and downloads in parallel if both are needed
+  if (needsDownloads) {
+    const [data, downloads] = await Promise.all([
+      npmRegistry.getPackageInfo(pkg, options),
+      npmRegistry.getDownloadCount(pkg)
+    ])
 
-      return data
-    } else {
-      // If downloads not needed, just fetch package info
-      const data = await npmRegistry.getPackageInfo(pkg, options)
-      return data
+    if (downloads !== null) {
+      data.downloads = downloads
     }
-  } catch (err) {
-    log.error(err)
-    throw err
+
+    return data
   }
+
+  // If downloads not needed, just fetch package info
+  return npmRegistry.getPackageInfo(pkg, options)
 }
 
 export default {

lib/pkginfo.js

@@ -1,21 +1,13 @@
 import pkgdata from './pkgdata.js'
 
-async function pkginfo (log, pkg, options) {
-  let info = { name: pkg }
+async function pkginfo (pkg, options) {
+  const data = await pkgdata.pkgInfo(pkg, options)
 
-  try {
-    const data = await pkgdata.pkgInfo(pkg, options)
-
-    if (!data) {
-      throw new Error(`Package not found: ${pkg}`)
-    }
-
-    info = { ...info, ...data }
-    return info
-  } catch (err) {
-    log.error(err)
-    throw err
+  if (!data) {
+    throw new Error(`Package not found: ${pkg}`)
   }
+
+  return { name: pkg, ...data }
 }
 
 export default pkginfo

nodeico-fastify.js

@@ -329,7 +329,7 @@ export default function createServer () {
     }
 
     try {
-      const packageInfo = await pkginfo(request.reqLog, pkg, options)
+      const packageInfo = await pkginfo(pkg, options)
 
       // For shields/flat styles, we need to handle dimensions differently
       const styleName = options.style || 'standard'

test/npm-registry-cache-test.js

@@ -0,0 +1,73 @@
+import { test } from 'node:test'
+import assert from 'node:assert/strict'
+import npmRegistry from '../lib/npm-registry.js'
+
+test('caches failed package lookups to avoid repeat network calls', async (t) => {
+  npmRegistry.clearCache()
+
+  let calls = 0
+  t.mock.method(globalThis, 'fetch', async () => {
+    calls++
+    return new Response(JSON.stringify({ error: 'not_found', reason: 'document not found' }), {
+      status: 404,
+      statusText: 'Not Found'
+    })
+  })
+
+  const pkg = 'this-package-definitely-does-not-exist-xyz123'
+
+  await assert.rejects(() => npmRegistry.getPackageInfo(pkg), /HTTP 404/)
+  await assert.rejects(() => npmRegistry.getPackageInfo(pkg), /HTTP 404/)
+  await assert.rejects(() => npmRegistry.getPackageInfo(pkg), /HTTP 404/)
+
+  assert.equal(calls, 1, 'only the first failed lookup hits the network, the rest are served from cache')
+})
+
+test('successful lookups are cached and are not mistaken for failures', async (t) => {
+  npmRegistry.clearCache()
+
+  let calls = 0
+  t.mock.method(globalThis, 'fetch', async () => {
+    calls++
+    return new Response(JSON.stringify({
+      name: 'good-package',
+      'dist-tags': { latest: '1.0.0' },
+      time: { '1.0.0': '2024-01-01T00:00:00.000Z' },
+      versions: { '1.0.0': { dependencies: {} } }
+    }), { status: 200, statusText: 'OK' })
+  })
+
+  const pkg = 'good-package'
+  const first = await npmRegistry.getPackageInfo(pkg)
+  const second = await npmRegistry.getPackageInfo(pkg)
+
+  assert.equal(first.name, 'good-package')
+  assert.equal(second.name, 'good-package')
+  assert.equal(calls, 1, 'second call is served from cache, not a fresh fetch')
+})
+
+test('a later successful lookup clears out a previously cached failure', async (t) => {
+  npmRegistry.clearCache()
+
+  let shouldFail = true
+  t.mock.method(globalThis, 'fetch', async () => {
+    if (shouldFail) {
+      return new Response(JSON.stringify({ error: 'not_found' }), { status: 404, statusText: 'Not Found' })
+    }
+    return new Response(JSON.stringify({
+      name: 'flaky-package',
+      'dist-tags': { latest: '1.0.0' },
+      time: { '1.0.0': '2024-01-01T00:00:00.000Z' },
+      versions: { '1.0.0': { dependencies: {} } }
+    }), { status: 200, statusText: 'OK' })
+  })
+
+  const pkg = 'flaky-package'
+  await assert.rejects(() => npmRegistry.getPackageInfo(pkg), /HTTP 404/)
+
+  npmRegistry.clearCache()
+  shouldFail = false
+
+  const info = await npmRegistry.getPackageInfo(pkg)
+  assert.equal(info.name, 'flaky-package')
+})