diff --git a/lib/npm-registry.js b/lib/npm-registry.js index 8bcb8b5..8f70901 100644 --- a/lib/npm-registry.js +++ b/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) { diff --git a/lib/pkgdata.js b/lib/pkgdata.js index cfde236..3708928 100644 --- a/lib/pkgdata.js +++ b/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 { diff --git a/lib/pkginfo.js b/lib/pkginfo.js index 739cb34..1f8a723 100644 --- a/lib/pkginfo.js +++ b/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 diff --git a/nodeico-fastify.js b/nodeico-fastify.js index acb7470..fa67c19 100644 --- a/nodeico-fastify.js +++ b/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' diff --git a/test/npm-registry-cache-test.js b/test/npm-registry-cache-test.js new file mode 100644 index 0000000..0272e13 --- /dev/null +++ b/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') +})