From 03f168daf6e13b68e0757afd57639691ef877305 Mon Sep 17 00:00:00 2001 From: Michal Piechowiak <misiek.piechowiak@gmail.com> Date: Wed, 19 Mar 2025 20:17:01 +0100 Subject: [PATCH 01/10] test: add test case with multiple fetches for same resource and test asserting we only check blobs once per request for same key --- .../same-fetch-multiple-times/[id]/page.js | 47 ++++++++++ tests/integration/fetch-handler.test.ts | 94 ++++++++++++++++++- 2 files changed, 140 insertions(+), 1 deletion(-) create mode 100644 tests/fixtures/revalidate-fetch/app/same-fetch-multiple-times/[id]/page.js diff --git a/tests/fixtures/revalidate-fetch/app/same-fetch-multiple-times/[id]/page.js b/tests/fixtures/revalidate-fetch/app/same-fetch-multiple-times/[id]/page.js new file mode 100644 index 0000000000..4f6d48e133 --- /dev/null +++ b/tests/fixtures/revalidate-fetch/app/same-fetch-multiple-times/[id]/page.js @@ -0,0 +1,47 @@ +const API_BASE = process.env.API_BASE || 'https://api.tvmaze.com/shows/' + +async function doFetch(params) { + const res = await fetch(new URL(params.id, API_BASE).href, { + next: { revalidate: false }, + }) + return res.json() +} + +async function getData(params) { + // trigger fetches in parallel to ensure we only do one cache lookup for ongoing fetches + const [data1, data2] = await Promise.all([doFetch(params), doFetch(params)]) + // also trigger fetch after to make sure we reuse already fetched cache + const data3 = await doFetch(params) + + if (data1?.name !== data2?.name || data1?.name !== data3?.name) { + throw new Error( + `Should have 3 names that are the same, instead got [${data1?.name}, ${data2?.name}, ${data3?.name}]`, + ) + } + + return data1 +} + +export default async function Page({ params }) { + const data = await getData(params) + + return ( + <> + <h1>Using same fetch multiple times</h1> + <dl> + <dt>Show</dt> + <dd data-testid="name">{data.name}</dd> + <dt>Param</dt> + <dd data-testid="id">{params.id}</dd> + <dt>Time</dt> + <dd data-testid="date-now">{Date.now()}</dd> + <dt>Time from fetch response</dt> + <dd data-testid="date-from-response">{data.date ?? 'no-date-in-response'}</dd> + </dl> + </> + ) +} + +// make page dynamic, but still use fetch cache +export const fetchCache = 'force-cache' +export const dynamic = 'force-dynamic' diff --git a/tests/integration/fetch-handler.test.ts b/tests/integration/fetch-handler.test.ts index b525dfd16e..c90bf44af3 100644 --- a/tests/integration/fetch-handler.test.ts +++ b/tests/integration/fetch-handler.test.ts @@ -6,9 +6,33 @@ import { v4 } from 'uuid' import { afterAll, beforeAll, beforeEach, expect, test, vi } from 'vitest' import { type FixtureTestContext } from '../utils/contexts.js' import { createFixture, invokeFunction, runPlugin, runPluginStep } from '../utils/fixture.js' -import { generateRandomObjectID, startMockBlobStore } from '../utils/helpers.js' +import { generateRandomObjectID, getBlobServerGets, startMockBlobStore } from '../utils/helpers.js' import { nextVersionSatisfies } from '../utils/next-version-helpers.mjs' +function isFetch(key: string) { + // exclude tag manifests (starting with `_N_T_`), pages (starting with `/`) and static html files (keys including `.html`) + return !key.startsWith('_N_T_') && !key.startsWith('/') && !key.includes('.html') +} + +expect.extend({ + toBeDistinct(received: string[]) { + const { isNot } = this + const pass = new Set(received).size === received.length + return { + pass, + message: () => `${received} is${isNot ? ' not' : ''} array with distinct values`, + } + }, +}) + +interface CustomMatchers<R = unknown> { + toBeDistinct(): R +} + +declare module 'vitest' { + interface Assertion<T = any> extends CustomMatchers<T> {} +} + // Disable the verbose logging of the lambda-local runtime getLogger().level = 'alert' @@ -304,3 +328,71 @@ test<FixtureTestContext>('if the fetch call is cached correctly (cached page res }), ) }) + +test<FixtureTestContext>('does not fetch same cached fetch data from blobs twice for the same request', async (ctx) => { + await createFixture('revalidate-fetch', ctx) + await runPluginStep(ctx, 'onPreBuild') + await runPlugin(ctx) + + handlerCalled = 0 + const request1 = await invokeFunction(ctx, { + url: 'same-fetch-multiple-times/99', + }) + + const request1FetchDate = load(request1.body)('[data-testid="date-from-response"]').text() + const request1Name = load(request1.body)('[data-testid="name"]').text() + + expect(request1.statusCode, 'Tested page should work').toBe(200) + expect(request1Name, 'Test setup should use test API mock').toBe('Fake response') + + expect( + handlerCalled, + 'Cache should be empty, and we should hit mock endpoint once to warm up the cache', + ).toBe(1) + + const request1FetchCacheKeys = getBlobServerGets(ctx, isFetch) + + expect( + request1FetchCacheKeys.length, + 'tested page should be doing 3 fetch calls to render single page - we should only try to get cached fetch data from blobs once', + ).toBe(1) + + const request1AllCacheKeys = getBlobServerGets(ctx) + expect( + request1AllCacheKeys, + 'expected blobs for all types of values to be retrieved at most once per key (including fetch data, tag manifests, static files)', + ).toBeDistinct() + + ctx.blobServerGetSpy.mockClear() + handlerCalled = 0 + const request2 = await invokeFunction(ctx, { + url: 'same-fetch-multiple-times/99', + }) + + const request2FetchDate = load(request2.body)('[data-testid="date-from-response"]').text() + const request2Name = load(request2.body)('[data-testid="name"]').text() + + expect(request2.statusCode, 'Tested page should work').toBe(200) + expect(request2Name, 'Test setup should use test API mock').toBe('Fake response') + expect(request2FetchDate, 'Cached fetch data should be used for second request').toBe( + request1FetchDate, + ) + + expect(handlerCalled, 'Cache should be warm, and we should not hit mock endpoint').toBe(0) + + const request2FetchCacheKeys = getBlobServerGets(ctx, isFetch) + + expect( + request2FetchCacheKeys.length, + 'We should not reuse in-memory cache from first request and have one fetch blob call for second request', + ).toBe(1) + expect(request2FetchCacheKeys, 'Same fetch keys should be used in both requests').toEqual( + request1FetchCacheKeys, + ) + + const request2AllCacheKeys = getBlobServerGets(ctx) + expect( + request2AllCacheKeys, + 'expected blobs for all types of values to be retrieved at most once per key (including fetch data, tag manifests, static files)', + ).toBeDistinct() +}) From 2e7a9517f5cf2f2c6c424e4c5c6dd9807fd5069e Mon Sep 17 00:00:00 2001 From: Michal Piechowiak <misiek.piechowiak@gmail.com> Date: Wed, 19 Mar 2025 11:12:16 +0100 Subject: [PATCH 02/10] refactor: create simplified key-value store interface to interact with blobs with common tracing pattern --- src/build/content/static.ts | 2 +- src/run/handlers/cache.cts | 59 ++++++++++++--------------------- src/run/handlers/server.ts | 1 - src/run/headers.ts | 27 ++++----------- src/run/next.cts | 17 +++------- src/run/regional-blob-store.cts | 48 ++++++++++++++++++++++++++- src/shared/cache-types.cts | 9 +++++ 7 files changed, 89 insertions(+), 74 deletions(-) diff --git a/src/build/content/static.ts b/src/build/content/static.ts index f972e8b8bd..0d80425bdb 100644 --- a/src/build/content/static.ts +++ b/src/build/content/static.ts @@ -6,8 +6,8 @@ import { trace } from '@opentelemetry/api' import { wrapTracer } from '@opentelemetry/api/experimental' import glob from 'fast-glob' -import type { HtmlBlob } from '../../run/next.cjs' import { encodeBlobKey } from '../../shared/blobkey.js' +import type { HtmlBlob } from '../../shared/cache-types.cjs' import { PluginContext } from '../plugin-context.js' import { verifyNetlifyForms } from '../verification.js' diff --git a/src/run/handlers/cache.cts b/src/run/handlers/cache.cts index fc40ec4bdf..9178cf84a1 100644 --- a/src/run/handlers/cache.cts +++ b/src/run/handlers/cache.cts @@ -5,7 +5,6 @@ import { Buffer } from 'node:buffer' import { join } from 'node:path' import { join as posixJoin } from 'node:path/posix' -import { Store } from '@netlify/blobs' import { purgeCache } from '@netlify/functions' import { type Span } from '@opentelemetry/api' import type { PrerenderManifest } from 'next/dist/build/index.js' @@ -21,37 +20,34 @@ import { type NetlifyCachedRouteValue, type NetlifyCacheHandlerValue, type NetlifyIncrementalCacheValue, + type TagManifest, } from '../../shared/cache-types.cjs' -import { getRegionalBlobStore } from '../regional-blob-store.cjs' +import { + getMemoizedKeyValueStoreBackedByRegionalBlobStore, + MemoizedKeyValueStoreBackedByRegionalBlobStore, +} from '../regional-blob-store.cjs' import { getLogger, getRequestContext } from './request-context.cjs' import { getTracer } from './tracer.cjs' -type TagManifest = { revalidatedAt: number } - -type TagManifestBlobCache = Record<string, Promise<TagManifest>> +type TagManifestBlobCache = Record<string, Promise<TagManifest | null>> const purgeCacheUserAgent = `${nextRuntimePkgName}@${nextRuntimePkgVersion}` export class NetlifyCacheHandler implements CacheHandlerForMultipleVersions { options: CacheHandlerContext revalidatedTags: string[] - blobStore: Store + cacheStore: MemoizedKeyValueStoreBackedByRegionalBlobStore tracer = getTracer() tagManifestsFetchedFromBlobStoreInCurrentRequest: TagManifestBlobCache constructor(options: CacheHandlerContext) { this.options = options this.revalidatedTags = options.revalidatedTags - this.blobStore = getRegionalBlobStore({ consistency: 'strong' }) + this.cacheStore = getMemoizedKeyValueStoreBackedByRegionalBlobStore({ consistency: 'strong' }) this.tagManifestsFetchedFromBlobStoreInCurrentRequest = {} } - private async encodeBlobKey(key: string) { - const { encodeBlobKey } = await import('../../shared/blobkey.js') - return await encodeBlobKey(key) - } - private getTTL(blob: NetlifyCacheHandlerValue) { if ( blob.value?.kind === 'FETCH' || @@ -245,19 +241,13 @@ export class NetlifyCacheHandler implements CacheHandlerForMultipleVersions { const [key, ctx = {}] = args getLogger().debug(`[NetlifyCacheHandler.get]: ${key}`) - const blobKey = await this.encodeBlobKey(key) - span.setAttributes({ key, blobKey }) + span.setAttributes({ key }) - const blob = (await this.tracer.withActiveSpan('blobStore.get', async (blobGetSpan) => { - blobGetSpan.setAttributes({ key, blobKey }) - return await this.blobStore.get(blobKey, { - type: 'json', - }) - })) as NetlifyCacheHandlerValue | null + const blob = await this.cacheStore.get<NetlifyCacheHandlerValue>(key, 'blobStore.get') // if blob is null then we don't have a cache entry if (!blob) { - span.addEvent('Cache miss', { key, blobKey }) + span.addEvent('Cache miss', { key }) return null } @@ -268,7 +258,6 @@ export class NetlifyCacheHandler implements CacheHandlerForMultipleVersions { // but opt to discard STALE data, so that Next.js generate fresh response span.addEvent('Discarding stale entry due to SWR background revalidation request', { key, - blobKey, ttl, }) getLogger() @@ -285,7 +274,7 @@ export class NetlifyCacheHandler implements CacheHandlerForMultipleVersions { const staleByTags = await this.checkCacheEntryStaleByTags(blob, ctx.tags, ctx.softTags) if (staleByTags) { - span.addEvent('Stale', { staleByTags, key, blobKey, ttl }) + span.addEvent('Stale', { staleByTags, key, ttl }) return null } @@ -403,9 +392,8 @@ export class NetlifyCacheHandler implements CacheHandlerForMultipleVersions { async set(...args: Parameters<CacheHandlerForMultipleVersions['set']>) { return this.tracer.withActiveSpan('set cache key', async (span) => { const [key, data, context] = args - const blobKey = await this.encodeBlobKey(key) const lastModified = Date.now() - span.setAttributes({ key, lastModified, blobKey }) + span.setAttributes({ key, lastModified }) getLogger().debug(`[NetlifyCacheHandler.set]: ${key}`) @@ -415,10 +403,7 @@ export class NetlifyCacheHandler implements CacheHandlerForMultipleVersions { // and we didn't yet capture cache tags, we try to get cache tags from freshly produced cache value this.captureCacheTags(value, key) - await this.blobStore.setJSON(blobKey, { - lastModified, - value, - }) + await this.cacheStore.set(key, { lastModified, value }, 'blobStore.set') if (data?.kind === 'PAGE' || data?.kind === 'PAGES') { const requestContext = getRequestContext() @@ -476,7 +461,7 @@ export class NetlifyCacheHandler implements CacheHandlerForMultipleVersions { await Promise.all( tags.map(async (tag) => { try { - await this.blobStore.setJSON(await this.encodeBlobKey(tag), data) + await this.cacheStore.set(tag, data, 'tagManifest.set') } catch (error) { getLogger().withError(error).log(`Failed to update tag manifest for ${tag}`) } @@ -544,23 +529,21 @@ export class NetlifyCacheHandler implements CacheHandlerForMultipleVersions { const tagManifestPromises: Promise<boolean>[] = [] for (const tag of cacheTags) { - let tagManifestPromise: Promise<TagManifest> = + let tagManifestPromise: Promise<TagManifest | null> = this.tagManifestsFetchedFromBlobStoreInCurrentRequest[tag] if (!tagManifestPromise) { - tagManifestPromise = this.encodeBlobKey(tag).then((blobKey) => { - return this.tracer.withActiveSpan(`get tag manifest`, async (span) => { - span.setAttributes({ tag, blobKey }) - return this.blobStore.get(blobKey, { type: 'json' }) - }) - }) + tagManifestPromise = this.cacheStore.get<TagManifest>(tag, 'tagManifest.get') this.tagManifestsFetchedFromBlobStoreInCurrentRequest[tag] = tagManifestPromise } tagManifestPromises.push( tagManifestPromise.then((tagManifest) => { - const isStale = tagManifest?.revalidatedAt >= (cacheEntry.lastModified || Date.now()) + if (!tagManifest) { + return false + } + const isStale = tagManifest.revalidatedAt >= (cacheEntry.lastModified || Date.now()) if (isStale) { resolve(true) return true diff --git a/src/run/handlers/server.ts b/src/run/handlers/server.ts index b5a9d0f800..dff1b6bd60 100644 --- a/src/run/handlers/server.ts +++ b/src/run/handlers/server.ts @@ -127,7 +127,6 @@ export default async ( headers: response.headers, request, span, - tracer, requestContext, }) } diff --git a/src/run/headers.ts b/src/run/headers.ts index 50d82d4c16..f9b1bcb5ba 100644 --- a/src/run/headers.ts +++ b/src/run/headers.ts @@ -1,12 +1,10 @@ import type { Span } from '@opentelemetry/api' import type { NextConfigComplete } from 'next/dist/server/config-shared.js' -import { encodeBlobKey } from '../shared/blobkey.js' -import type { NetlifyCachedRouteValue } from '../shared/cache-types.cjs' +import type { NetlifyCachedRouteValue, NetlifyCacheHandlerValue } from '../shared/cache-types.cjs' import { getLogger, RequestContext } from './handlers/request-context.cjs' -import type { RuntimeTracer } from './handlers/tracer.cjs' -import { getRegionalBlobStore } from './regional-blob-store.cjs' +import { getMemoizedKeyValueStoreBackedByRegionalBlobStore } from './regional-blob-store.cjs' const ALL_VARIATIONS = Symbol.for('ALL_VARIATIONS') interface NetlifyVaryValues { @@ -129,13 +127,11 @@ export const adjustDateHeader = async ({ headers, request, span, - tracer, requestContext, }: { headers: Headers request: Request span: Span - tracer: RuntimeTracer requestContext: RequestContext }) => { const key = new URL(request.url).pathname @@ -157,23 +153,12 @@ export const adjustDateHeader = async ({ warning: true, }) - const blobStore = getRegionalBlobStore({ consistency: 'strong' }) - const blobKey = await encodeBlobKey(key) - - // TODO: use metadata for this - lastModified = await tracer.withActiveSpan( + const cacheStore = getMemoizedKeyValueStoreBackedByRegionalBlobStore({ consistency: 'strong' }) + const cacheEntry = await cacheStore.get<NetlifyCacheHandlerValue>( + key, 'get cache to calculate date header', - async (getBlobForDateSpan) => { - getBlobForDateSpan.setAttributes({ - key, - blobKey, - }) - const blob = (await blobStore.get(blobKey, { type: 'json' })) ?? {} - - getBlobForDateSpan.addEvent(blob ? 'Cache hit' : 'Cache miss') - return blob.lastModified - }, ) + lastModified = cacheEntry?.lastModified } if (!lastModified) { diff --git a/src/run/next.cts b/src/run/next.cts index 17859c4835..af48b1fbb6 100644 --- a/src/run/next.cts +++ b/src/run/next.cts @@ -4,9 +4,11 @@ import { relative, resolve } from 'path' // @ts-expect-error no types installed import { patchFs } from 'fs-monkey' +import { HtmlBlob } from '../shared/cache-types.cjs' + import { getRequestContext } from './handlers/request-context.cjs' import { getTracer } from './handlers/tracer.cjs' -import { getRegionalBlobStore } from './regional-blob-store.cjs' +import { getMemoizedKeyValueStoreBackedByRegionalBlobStore } from './regional-blob-store.cjs' // https://github.com/vercel/next.js/pull/68193/files#diff-37243d614f1f5d3f7ea50bbf2af263f6b1a9a4f70e84427977781e07b02f57f1R49 // This import resulted in importing unbundled React which depending if NODE_ENV is `production` or not would use @@ -76,18 +78,11 @@ ResponseCache.prototype.get = function get(...getArgs: unknown[]) { type FS = typeof import('fs') -export type HtmlBlob = { - html: string - isFallback: boolean -} - export async function getMockedRequestHandler(...args: Parameters<typeof getRequestHandlers>) { const tracer = getTracer() return tracer.withActiveSpan('mocked request handler', async () => { const ofs = { ...fs } - const { encodeBlobKey } = await import('../shared/blobkey.js') - async function readFileFallbackBlobStore(...fsargs: Parameters<FS['promises']['readFile']>) { const [path, options] = fsargs try { @@ -97,11 +92,9 @@ export async function getMockedRequestHandler(...args: Parameters<typeof getRequ } catch (error) { // only try to get .html files from the blob store if (typeof path === 'string' && path.endsWith('.html')) { - const store = getRegionalBlobStore() + const cacheStore = getMemoizedKeyValueStoreBackedByRegionalBlobStore() const relPath = relative(resolve('.next/server/pages'), path) - const file = (await store.get(await encodeBlobKey(relPath), { - type: 'json', - })) as HtmlBlob | null + const file = await cacheStore.get<HtmlBlob>(relPath, 'staticHtml.get') if (file !== null) { if (!file.isFallback) { const requestContext = getRequestContext() diff --git a/src/run/regional-blob-store.cts b/src/run/regional-blob-store.cts index 62aec1db4b..39e4598e6d 100644 --- a/src/run/regional-blob-store.cts +++ b/src/run/regional-blob-store.cts @@ -1,5 +1,9 @@ import { getDeployStore, GetWithMetadataOptions, Store } from '@netlify/blobs' +import type { BlobType } from '../shared/cache-types.cjs' + +import { getTracer } from './handlers/tracer.cjs' + const FETCH_BEFORE_NEXT_PATCHED_IT = Symbol.for('nf-not-patched-fetch') const extendedGlobalThis = globalThis as typeof globalThis & { [FETCH_BEFORE_NEXT_PATCHED_IT]?: typeof globalThis.fetch @@ -53,10 +57,52 @@ const fetchBeforeNextPatchedItFallback = forceOptOutOfUsingDataCache( const getFetchBeforeNextPatchedIt = () => extendedGlobalThis[FETCH_BEFORE_NEXT_PATCHED_IT] ?? fetchBeforeNextPatchedItFallback -export const getRegionalBlobStore = (args: GetWithMetadataOptions = {}): Store => { +const getRegionalBlobStore = (args: GetWithMetadataOptions = {}): Store => { return getDeployStore({ ...args, fetch: getFetchBeforeNextPatchedIt(), region: process.env.USE_REGIONAL_BLOBS?.toUpperCase() === 'TRUE' ? undefined : 'us-east-2', }) } + +const encodeBlobKey = async (key: string) => { + const { encodeBlobKey: encodeBlobKeyImpl } = await import('../shared/blobkey.js') + return await encodeBlobKeyImpl(key) +} + +export const getMemoizedKeyValueStoreBackedByRegionalBlobStore = ( + args: GetWithMetadataOptions = {}, +) => { + const store = getRegionalBlobStore(args) + const tracer = getTracer() + + return { + async get<T extends BlobType>(key: string, otelSpanTitle: string): Promise<T | null> { + const blobKey = await encodeBlobKey(key) + + return tracer.withActiveSpan(otelSpanTitle, async (span) => { + span.setAttributes({ key, blobKey }) + const blob = (await store.get(blobKey, { type: 'json' })) as T | null + span.addEvent(blob ? 'Hit' : 'Miss') + return blob + }) + }, + async set(key: string, value: BlobType, otelSpanTitle: string) { + const blobKey = await encodeBlobKey(key) + + return tracer.withActiveSpan(otelSpanTitle, async (span) => { + span.setAttributes({ key, blobKey }) + return await store.setJSON(blobKey, value) + }) + }, + } +} + +/** + * Wrapper around Blobs Store that memoizes the cache entries within context of a request + * to avoid duplicate requests to the same key and also allowing to read its own writes from + * memory. + */ +export type MemoizedKeyValueStoreBackedByRegionalBlobStore = ReturnType< + typeof getMemoizedKeyValueStoreBackedByRegionalBlobStore +> diff --git a/src/shared/cache-types.cts b/src/shared/cache-types.cts index defba8e6fa..811860335c 100644 --- a/src/shared/cache-types.cts +++ b/src/shared/cache-types.cts @@ -154,3 +154,12 @@ export type CacheHandlerForMultipleVersions = BaseCacheHandlerForMultipleVersion context: CacheHandlerSetContextForMultipleVersions, ) => ReturnType<BaseCacheHandlerForMultipleVersions['set']> } + +export type TagManifest = { revalidatedAt: number } + +export type HtmlBlob = { + html: string + isFallback: boolean +} + +export type BlobType = NetlifyCacheHandlerValue | TagManifest | HtmlBlob From 2f7dee1bdc2ab01fef32807cea9a481357540a32 Mon Sep 17 00:00:00 2001 From: Michal Piechowiak <misiek.piechowiak@gmail.com> Date: Thu, 20 Mar 2025 13:59:44 +0100 Subject: [PATCH 03/10] refactor: create reusable helper to record warnings --- src/run/handlers/cache.cts | 18 +++++------------- src/run/handlers/tracer.cts | 13 +++++++++++++ src/run/headers.ts | 16 ++++++---------- 3 files changed, 24 insertions(+), 23 deletions(-) diff --git a/src/run/handlers/cache.cts b/src/run/handlers/cache.cts index 9178cf84a1..3a1e4aeb88 100644 --- a/src/run/handlers/cache.cts +++ b/src/run/handlers/cache.cts @@ -28,7 +28,7 @@ import { } from '../regional-blob-store.cjs' import { getLogger, getRequestContext } from './request-context.cjs' -import { getTracer } from './tracer.cjs' +import { getTracer, recordWarning } from './tracer.cjs' type TagManifestBlobCache = Record<string, Promise<TagManifest | null>> @@ -85,13 +85,7 @@ export class NetlifyCacheHandler implements CacheHandlerForMultipleVersions { if (!requestContext) { // we will not be able to use request context for date header calculation // we will fallback to using blobs - getCacheKeySpan.recordException( - new Error('CacheHandler was called without a request context'), - ) - getCacheKeySpan.setAttributes({ - severity: 'alert', - warning: true, - }) + recordWarning(new Error('CacheHandler was called without a request context'), getCacheKeySpan) return } @@ -100,15 +94,13 @@ export class NetlifyCacheHandler implements CacheHandlerForMultipleVersions { // so as a safety measure we will not use any of them and let blobs be used // to calculate the date header requestContext.responseCacheGetLastModified = undefined - getCacheKeySpan.recordException( + recordWarning( new Error( `Multiple response cache keys used in single request: ["${requestContext.responseCacheKey}, "${key}"]`, ), + getCacheKeySpan, ) - getCacheKeySpan.setAttributes({ - severity: 'alert', - warning: true, - }) + return } diff --git a/src/run/handlers/tracer.cts b/src/run/handlers/tracer.cts index b27ec88b22..85562e7376 100644 --- a/src/run/handlers/tracer.cts +++ b/src/run/handlers/tracer.cts @@ -89,3 +89,16 @@ export function getTracer(): RuntimeTracer { return tracer } + +export function recordWarning(warning: Error, span?: Span) { + const spanToRecordWarningOn = span ?? trace.getActiveSpan() + if (!spanToRecordWarningOn) { + return + } + + spanToRecordWarningOn.recordException(warning) + spanToRecordWarningOn.setAttributes({ + severity: 'alert', + warning: true, + }) +} diff --git a/src/run/headers.ts b/src/run/headers.ts index f9b1bcb5ba..967ecc7ada 100644 --- a/src/run/headers.ts +++ b/src/run/headers.ts @@ -4,6 +4,7 @@ import type { NextConfigComplete } from 'next/dist/server/config-shared.js' import type { NetlifyCachedRouteValue, NetlifyCacheHandlerValue } from '../shared/cache-types.cjs' import { getLogger, RequestContext } from './handlers/request-context.cjs' +import { recordWarning } from './handlers/tracer.cjs' import { getMemoizedKeyValueStoreBackedByRegionalBlobStore } from './regional-blob-store.cjs' const ALL_VARIATIONS = Symbol.for('ALL_VARIATIONS') @@ -145,13 +146,10 @@ export const adjustDateHeader = async ({ // request context would contain lastModified value // this is not fatal as we have fallback, // but we want to know about it happening - span.recordException( + recordWarning( new Error('lastModified not found in requestContext, falling back to trying blobs'), + span, ) - span.setAttributes({ - severity: 'alert', - warning: true, - }) const cacheStore = getMemoizedKeyValueStoreBackedByRegionalBlobStore({ consistency: 'strong' }) const cacheEntry = await cacheStore.get<NetlifyCacheHandlerValue>( @@ -164,15 +162,13 @@ export const adjustDateHeader = async ({ if (!lastModified) { // this should never happen as we only execute this code path for cached responses // and those should always have lastModified value - span.recordException( + recordWarning( new Error( 'lastModified not found in either requestContext or blobs, date header for cached response is not set', ), + span, ) - span.setAttributes({ - severity: 'alert', - warning: true, - }) + return } From 85d4eb91adef00ddccb802e10210f014c41cf357 Mon Sep 17 00:00:00 2001 From: Michal Piechowiak <misiek.piechowiak@gmail.com> Date: Wed, 19 Mar 2025 20:33:08 +0100 Subject: [PATCH 04/10] perf: memoize blobs requests in the request scope --- package-lock.json | 76 ++++------ package.json | 1 + src/run/config.ts | 32 ++++- src/run/handlers/cache.cts | 26 ++-- src/run/handlers/request-context.cts | 18 ++- src/run/regional-blob-store.cts | 98 ++++++++++++- src/run/regional-blob-store.test.ts | 205 +++++++++++++++++++++++++++ src/shared/cache-types.cts | 52 +++++++ 8 files changed, 431 insertions(+), 77 deletions(-) create mode 100644 src/run/regional-blob-store.test.ts diff --git a/package-lock.json b/package-lock.json index be232a5e5e..dd6be1e826 100644 --- a/package-lock.json +++ b/package-lock.json @@ -37,6 +37,7 @@ "fs-monkey": "^1.0.6", "get-port": "^7.1.0", "lambda-local": "^2.2.0", + "lru-cache": "^10.4.3", "memfs": "^4.9.2", "mock-require": "^3.0.3", "msw": "^2.0.7", @@ -243,6 +244,16 @@ "node": ">=6.9.0" } }, + "node_modules/@babel/helper-compilation-targets/node_modules/lru-cache": { + "version": "5.1.1", + "resolved": "https://registry.npmjs.org/lru-cache/-/lru-cache-5.1.1.tgz", + "integrity": "sha512-KpNARQA3Iwv+jTA0utUVVbrh+Jlrr1Fv0e56GGzAFOXN7dk/FviaDW8LHmK52DlcH4WP2n6gI8vN1aesBFgo9w==", + "dev": true, + "license": "ISC", + "dependencies": { + "yallist": "^3.0.2" + } + }, "node_modules/@babel/helper-compilation-targets/node_modules/semver": { "version": "6.3.1", "resolved": "https://registry.npmjs.org/semver/-/semver-6.3.1.tgz", @@ -14352,13 +14363,11 @@ } }, "node_modules/lru-cache": { - "version": "5.1.1", - "resolved": "https://registry.npmjs.org/lru-cache/-/lru-cache-5.1.1.tgz", - "integrity": "sha512-KpNARQA3Iwv+jTA0utUVVbrh+Jlrr1Fv0e56GGzAFOXN7dk/FviaDW8LHmK52DlcH4WP2n6gI8vN1aesBFgo9w==", + "version": "10.4.3", + "resolved": "https://registry.npmjs.org/lru-cache/-/lru-cache-10.4.3.tgz", + "integrity": "sha512-JNAzZcXrCt42VGLuYz0zfAzDfAvJWW6AfYlDBQyDV5DClI2m5sAmK+OIO7s59XfsRsWHp02jAJrRadPRGTt6SQ==", "dev": true, - "dependencies": { - "yallist": "^3.0.2" - } + "license": "ISC" }, "node_modules/luxon": { "version": "3.4.4", @@ -32368,15 +32377,6 @@ "url": "https://github.com/sponsors/isaacs" } }, - "node_modules/path-scurry/node_modules/lru-cache": { - "version": "10.2.2", - "resolved": "https://registry.npmjs.org/lru-cache/-/lru-cache-10.2.2.tgz", - "integrity": "sha512-9hp3Vp2/hFQUiIwKo8XCeFVnrg8Pk3TYNPIR7tJADKi5YfcF7vEaK7avFHTlSy3kOKYaJQaalfEo6YuXdceBOQ==", - "dev": true, - "engines": { - "node": "14 || >=16.14" - } - }, "node_modules/path-to-regexp": { "version": "6.3.0", "resolved": "https://registry.npmjs.org/path-to-regexp/-/path-to-regexp-6.3.0.tgz", @@ -32891,15 +32891,6 @@ "node": "^16.14.0 || >=18.0.0" } }, - "node_modules/read-package-up/node_modules/lru-cache": { - "version": "10.2.2", - "resolved": "https://registry.npmjs.org/lru-cache/-/lru-cache-10.2.2.tgz", - "integrity": "sha512-9hp3Vp2/hFQUiIwKo8XCeFVnrg8Pk3TYNPIR7tJADKi5YfcF7vEaK7avFHTlSy3kOKYaJQaalfEo6YuXdceBOQ==", - "dev": true, - "engines": { - "node": "14 || >=16.14" - } - }, "node_modules/read-package-up/node_modules/normalize-package-data": { "version": "6.0.1", "resolved": "https://registry.npmjs.org/normalize-package-data/-/normalize-package-data-6.0.1.tgz", @@ -36301,7 +36292,8 @@ "version": "3.1.1", "resolved": "https://registry.npmjs.org/yallist/-/yallist-3.1.1.tgz", "integrity": "sha512-a4UGQaWPH59mOXUYnAG2ewncQS4i4F43Tv3JoAM+s2VDAmS9NsK8GpDMLrCHPksFT7h3K6TOoUNn2pb7RoXx4g==", - "dev": true + "dev": true, + "license": "ISC" }, "node_modules/yargs": { "version": "17.7.2", @@ -36594,6 +36586,15 @@ "semver": "^6.3.1" }, "dependencies": { + "lru-cache": { + "version": "5.1.1", + "resolved": "https://registry.npmjs.org/lru-cache/-/lru-cache-5.1.1.tgz", + "integrity": "sha512-KpNARQA3Iwv+jTA0utUVVbrh+Jlrr1Fv0e56GGzAFOXN7dk/FviaDW8LHmK52DlcH4WP2n6gI8vN1aesBFgo9w==", + "dev": true, + "requires": { + "yallist": "^3.0.2" + } + }, "semver": { "version": "6.3.1", "resolved": "https://registry.npmjs.org/semver/-/semver-6.3.1.tgz", @@ -46273,13 +46274,10 @@ "dev": true }, "lru-cache": { - "version": "5.1.1", - "resolved": "https://registry.npmjs.org/lru-cache/-/lru-cache-5.1.1.tgz", - "integrity": "sha512-KpNARQA3Iwv+jTA0utUVVbrh+Jlrr1Fv0e56GGzAFOXN7dk/FviaDW8LHmK52DlcH4WP2n6gI8vN1aesBFgo9w==", - "dev": true, - "requires": { - "yallist": "^3.0.2" - } + "version": "10.4.3", + "resolved": "https://registry.npmjs.org/lru-cache/-/lru-cache-10.4.3.tgz", + "integrity": "sha512-JNAzZcXrCt42VGLuYz0zfAzDfAvJWW6AfYlDBQyDV5DClI2m5sAmK+OIO7s59XfsRsWHp02jAJrRadPRGTt6SQ==", + "dev": true }, "luxon": { "version": "3.4.4", @@ -59047,14 +59045,6 @@ "requires": { "lru-cache": "^10.2.0", "minipass": "^5.0.0 || ^6.0.2 || ^7.0.0" - }, - "dependencies": { - "lru-cache": { - "version": "10.2.2", - "resolved": "https://registry.npmjs.org/lru-cache/-/lru-cache-10.2.2.tgz", - "integrity": "sha512-9hp3Vp2/hFQUiIwKo8XCeFVnrg8Pk3TYNPIR7tJADKi5YfcF7vEaK7avFHTlSy3kOKYaJQaalfEo6YuXdceBOQ==", - "dev": true - } } }, "path-to-regexp": { @@ -59414,12 +59404,6 @@ "lru-cache": "^10.0.1" } }, - "lru-cache": { - "version": "10.2.2", - "resolved": "https://registry.npmjs.org/lru-cache/-/lru-cache-10.2.2.tgz", - "integrity": "sha512-9hp3Vp2/hFQUiIwKo8XCeFVnrg8Pk3TYNPIR7tJADKi5YfcF7vEaK7avFHTlSy3kOKYaJQaalfEo6YuXdceBOQ==", - "dev": true - }, "normalize-package-data": { "version": "6.0.1", "resolved": "https://registry.npmjs.org/normalize-package-data/-/normalize-package-data-6.0.1.tgz", diff --git a/package.json b/package.json index 0249bd843d..eb19b91430 100644 --- a/package.json +++ b/package.json @@ -76,6 +76,7 @@ "fs-monkey": "^1.0.6", "get-port": "^7.1.0", "lambda-local": "^2.2.0", + "lru-cache": "^10.4.3", "memfs": "^4.9.2", "mock-require": "^3.0.3", "msw": "^2.0.7", diff --git a/src/run/config.ts b/src/run/config.ts index 9ca3f3dc5b..4b858b125e 100644 --- a/src/run/config.ts +++ b/src/run/config.ts @@ -5,6 +5,7 @@ import { join, resolve } from 'node:path' import type { NextConfigComplete } from 'next/dist/server/config-shared.js' import { PLUGIN_DIR, RUN_CONFIG } from './constants.js' +import { setInMemoryCacheMaxSizeFromNextConfig } from './regional-blob-store.cjs' /** * Get Next.js config from the build output @@ -13,10 +14,27 @@ export const getRunConfig = async () => { return JSON.parse(await readFile(resolve(PLUGIN_DIR, RUN_CONFIG), 'utf-8')) } +type NextConfigForMultipleVersions = NextConfigComplete & { + experimental: NextConfigComplete['experimental'] & { + // those are pre 14.1.0 options that were moved out of experimental in // https://github.com/vercel/next.js/pull/57953/files#diff-c49c4767e6ed8627e6e1b8f96b141ee13246153f5e9142e1da03450c8e81e96fL311 + + // https://github.com/vercel/next.js/blob/v14.0.4/packages/next/src/server/config-shared.ts#L182-L183 + // custom path to a cache handler to use + incrementalCacheHandlerPath?: string + // https://github.com/vercel/next.js/blob/v14.0.4/packages/next/src/server/config-shared.ts#L207-L212 + /** + * In-memory cache size in bytes. + * + * If `isrMemoryCacheSize: 0` disables in-memory caching. + */ + isrMemoryCacheSize?: number + } +} + /** * Configure the custom cache handler at request time */ -export const setRunConfig = (config: NextConfigComplete) => { +export const setRunConfig = (config: NextConfigForMultipleVersions) => { const cacheHandler = join(PLUGIN_DIR, '.netlify/dist/run/handlers/cache.cjs') if (!existsSync(cacheHandler)) { throw new Error(`Cache handler not found at ${cacheHandler}`) @@ -25,15 +43,17 @@ export const setRunConfig = (config: NextConfigComplete) => { // set the path to the cache handler config.experimental = { ...config.experimental, - // @ts-expect-error incrementalCacheHandlerPath was removed from config type - // but we still need to set it for older Next.js versions + // Before Next.js 14.1.0 path to the cache handler was in experimental section, see NextConfigForMultipleVersions type incrementalCacheHandlerPath: cacheHandler, } - // Next.js 14.1.0 moved the cache handler from experimental to stable - // https://github.com/vercel/next.js/pull/57953/files#diff-c49c4767e6ed8627e6e1b8f96b141ee13246153f5e9142e1da03450c8e81e96fL311 + // Next.js 14.1.0 moved the cache handler from experimental to stable, see NextConfigForMultipleVersions type config.cacheHandler = cacheHandler - config.cacheMaxMemorySize = 0 + + // honor the in-memory cache size from next.config (either one set by user or Next.js default) + setInMemoryCacheMaxSizeFromNextConfig( + config.cacheMaxMemorySize ?? config.experimental?.isrMemoryCacheSize, + ) // set config process.env.__NEXT_PRIVATE_STANDALONE_CONFIG = JSON.stringify(config) diff --git a/src/run/handlers/cache.cts b/src/run/handlers/cache.cts index 3a1e4aeb88..4652cfc6ce 100644 --- a/src/run/handlers/cache.cts +++ b/src/run/handlers/cache.cts @@ -30,8 +30,6 @@ import { import { getLogger, getRequestContext } from './request-context.cjs' import { getTracer, recordWarning } from './tracer.cjs' -type TagManifestBlobCache = Record<string, Promise<TagManifest | null>> - const purgeCacheUserAgent = `${nextRuntimePkgName}@${nextRuntimePkgVersion}` export class NetlifyCacheHandler implements CacheHandlerForMultipleVersions { @@ -39,13 +37,11 @@ export class NetlifyCacheHandler implements CacheHandlerForMultipleVersions { revalidatedTags: string[] cacheStore: MemoizedKeyValueStoreBackedByRegionalBlobStore tracer = getTracer() - tagManifestsFetchedFromBlobStoreInCurrentRequest: TagManifestBlobCache constructor(options: CacheHandlerContext) { this.options = options this.revalidatedTags = options.revalidatedTags this.cacheStore = getMemoizedKeyValueStoreBackedByRegionalBlobStore({ consistency: 'strong' }) - this.tagManifestsFetchedFromBlobStoreInCurrentRequest = {} } private getTTL(blob: NetlifyCacheHandlerValue) { @@ -469,7 +465,8 @@ export class NetlifyCacheHandler implements CacheHandlerForMultipleVersions { } resetRequestCache() { - this.tagManifestsFetchedFromBlobStoreInCurrentRequest = {} + // no-op because in-memory cache is scoped to requests and not global + // see getRequestSpecificInMemoryCache } /** @@ -508,10 +505,9 @@ export class NetlifyCacheHandler implements CacheHandlerForMultipleVersions { } // 2. If any in-memory tags don't indicate that any of tags was invalidated - // we will check blob store, but memoize results for duration of current request - // so that we only check blob store once per tag within a single request - // full-route cache and fetch caches share a lot of tags so this might save - // some roundtrips to the blob store. + // we will check blob store. Full-route cache and fetch caches share a lot of tags + // but we will only do actual blob read once withing a single request due to cacheStore + // memoization. // Additionally, we will resolve the promise as soon as we find first // stale tag, so that we don't wait for all of them to resolve (but keep all // running in case future `CacheHandler.get` calls would be able to use results). @@ -521,14 +517,10 @@ export class NetlifyCacheHandler implements CacheHandlerForMultipleVersions { const tagManifestPromises: Promise<boolean>[] = [] for (const tag of cacheTags) { - let tagManifestPromise: Promise<TagManifest | null> = - this.tagManifestsFetchedFromBlobStoreInCurrentRequest[tag] - - if (!tagManifestPromise) { - tagManifestPromise = this.cacheStore.get<TagManifest>(tag, 'tagManifest.get') - - this.tagManifestsFetchedFromBlobStoreInCurrentRequest[tag] = tagManifestPromise - } + const tagManifestPromise: Promise<TagManifest | null> = this.cacheStore.get<TagManifest>( + tag, + 'tagManifest.get', + ) tagManifestPromises.push( tagManifestPromise.then((tagManifest) => { diff --git a/src/run/handlers/request-context.cts b/src/run/handlers/request-context.cts index 36e27e35a9..1e46dab4c5 100644 --- a/src/run/handlers/request-context.cts +++ b/src/run/handlers/request-context.cts @@ -39,9 +39,22 @@ export type RequestContext = { */ backgroundWorkPromise: Promise<unknown> logger: SystemLogger + requestID: string } type RequestContextAsyncLocalStorage = AsyncLocalStorage<RequestContext> +const REQUEST_CONTEXT_GLOBAL_KEY = Symbol.for('nf-request-context-async-local-storage') +const REQUEST_COUNTER_KEY = Symbol.for('nf-request-counter') +const extendedGlobalThis = globalThis as typeof globalThis & { + [REQUEST_CONTEXT_GLOBAL_KEY]?: RequestContextAsyncLocalStorage + [REQUEST_COUNTER_KEY]?: number +} + +function getFallbackRequestID() { + const requestNumber = extendedGlobalThis[REQUEST_COUNTER_KEY] ?? 0 + extendedGlobalThis[REQUEST_COUNTER_KEY] = requestNumber + 1 + return `#${requestNumber}` +} export function createRequestContext(request?: Request, context?: FutureContext): RequestContext { const backgroundWorkPromises: Promise<unknown>[] = [] @@ -72,10 +85,10 @@ export function createRequestContext(request?: Request, context?: FutureContext) return Promise.allSettled(backgroundWorkPromises) }, logger, + requestID: request?.headers.get('x-nf-request-id') ?? getFallbackRequestID(), } } -const REQUEST_CONTEXT_GLOBAL_KEY = Symbol.for('nf-request-context-async-local-storage') let requestContextAsyncLocalStorage: RequestContextAsyncLocalStorage | undefined function getRequestContextAsyncLocalStorage() { if (requestContextAsyncLocalStorage) { @@ -85,9 +98,6 @@ function getRequestContextAsyncLocalStorage() { // AsyncLocalStorage in the module scope, because it will be different for each // copy - so first time an instance of this module is used, we store AsyncLocalStorage // in global scope and reuse it for all subsequent calls - const extendedGlobalThis = globalThis as typeof globalThis & { - [REQUEST_CONTEXT_GLOBAL_KEY]?: RequestContextAsyncLocalStorage - } if (extendedGlobalThis[REQUEST_CONTEXT_GLOBAL_KEY]) { return extendedGlobalThis[REQUEST_CONTEXT_GLOBAL_KEY] } diff --git a/src/run/regional-blob-store.cts b/src/run/regional-blob-store.cts index 39e4598e6d..1c60bc8210 100644 --- a/src/run/regional-blob-store.cts +++ b/src/run/regional-blob-store.cts @@ -1,12 +1,23 @@ import { getDeployStore, GetWithMetadataOptions, Store } from '@netlify/blobs' +import { LRUCache } from 'lru-cache' -import type { BlobType } from '../shared/cache-types.cjs' +import { type BlobType, estimateBlobSize } from '../shared/cache-types.cjs' +import { getRequestContext } from './handlers/request-context.cjs' import { getTracer } from './handlers/tracer.cjs' +// lru-cache types don't like using `null` for values, so we use a symbol to represent it and do conversion +// so it doesn't leak outside +const NullValue = Symbol.for('null-value') +type BlobLRUCache = LRUCache<string, BlobType | typeof NullValue | Promise<BlobType | null>> + const FETCH_BEFORE_NEXT_PATCHED_IT = Symbol.for('nf-not-patched-fetch') +const IN_MEMORY_CACHE_MAX_SIZE = Symbol.for('nf-in-memory-cache-max-size') +const IN_MEMORY_LRU_CACHE = Symbol.for('nf-in-memory-lru-cache') const extendedGlobalThis = globalThis as typeof globalThis & { [FETCH_BEFORE_NEXT_PATCHED_IT]?: typeof globalThis.fetch + [IN_MEMORY_CACHE_MAX_SIZE]?: number + [IN_MEMORY_LRU_CACHE]?: BlobLRUCache | null } /** @@ -64,6 +75,73 @@ const getRegionalBlobStore = (args: GetWithMetadataOptions = {}): Store => { region: process.env.USE_REGIONAL_BLOBS?.toUpperCase() === 'TRUE' ? undefined : 'us-east-2', }) } +const DEFAULT_FALLBACK_MAX_SIZE = 50 * 1024 * 1024 // 50MB, same as default Next.js config +export function setInMemoryCacheMaxSizeFromNextConfig(size: unknown) { + if (typeof size === 'number') { + extendedGlobalThis[IN_MEMORY_CACHE_MAX_SIZE] = size + } +} + +function getInMemoryLRUCache() { + if (typeof extendedGlobalThis[IN_MEMORY_LRU_CACHE] === 'undefined') { + const maxSize = + typeof extendedGlobalThis[IN_MEMORY_CACHE_MAX_SIZE] === 'number' + ? extendedGlobalThis[IN_MEMORY_CACHE_MAX_SIZE] + : DEFAULT_FALLBACK_MAX_SIZE + + extendedGlobalThis[IN_MEMORY_LRU_CACHE] = + maxSize === 0 + ? null // if user sets 0 in their config, we should honor that and not use in-memory cache + : new LRUCache<string, BlobType | typeof NullValue | Promise<BlobType | null>>({ + max: 1000, + maxSize, + sizeCalculation: (valueToStore) => { + return estimateBlobSize(valueToStore === NullValue ? null : valueToStore) + }, + }) + } + return extendedGlobalThis[IN_MEMORY_LRU_CACHE] +} + +interface RequestSpecificInMemoryCache { + get(key: string): BlobType | null | Promise<BlobType | null> | undefined + set(key: string, value: BlobType | null | Promise<BlobType | null>): void +} + +const noOpInMemoryCache: RequestSpecificInMemoryCache = { + get(): undefined { + // no-op + }, + set() { + // no-op + }, +} + +const getRequestSpecificInMemoryCache = (): RequestSpecificInMemoryCache => { + const requestContext = getRequestContext() + if (!requestContext) { + // Fallback to a no-op store if we can't find request context + return noOpInMemoryCache + } + + const inMemoryLRUCache = getInMemoryLRUCache() + if (inMemoryLRUCache === null) { + return noOpInMemoryCache + } + + return { + get(key) { + const inMemoryValue = inMemoryLRUCache.get(`${requestContext.requestID}:${key}`) + if (inMemoryValue === NullValue) { + return null + } + return inMemoryValue + }, + set(key, value) { + inMemoryLRUCache.set(`${requestContext.requestID}:${key}`, value ?? NullValue) + }, + } +} const encodeBlobKey = async (key: string) => { const { encodeBlobKey: encodeBlobKeyImpl } = await import('../shared/blobkey.js') @@ -78,18 +156,30 @@ export const getMemoizedKeyValueStoreBackedByRegionalBlobStore = ( return { async get<T extends BlobType>(key: string, otelSpanTitle: string): Promise<T | null> { - const blobKey = await encodeBlobKey(key) + const inMemoryCache = getRequestSpecificInMemoryCache() - return tracer.withActiveSpan(otelSpanTitle, async (span) => { + const memoizedValue = inMemoryCache.get(key) + if (typeof memoizedValue !== 'undefined') { + return memoizedValue as T | null | Promise<T | null> + } + + const blobKey = await encodeBlobKey(key) + const getPromise = tracer.withActiveSpan(otelSpanTitle, async (span) => { span.setAttributes({ key, blobKey }) const blob = (await store.get(blobKey, { type: 'json' })) as T | null + inMemoryCache.set(key, blob) span.addEvent(blob ? 'Hit' : 'Miss') return blob }) + inMemoryCache.set(key, getPromise) + return getPromise }, async set(key: string, value: BlobType, otelSpanTitle: string) { - const blobKey = await encodeBlobKey(key) + const inMemoryCache = getRequestSpecificInMemoryCache() + + inMemoryCache.set(key, value) + const blobKey = await encodeBlobKey(key) return tracer.withActiveSpan(otelSpanTitle, async (span) => { span.setAttributes({ key, blobKey }) return await store.setJSON(blobKey, value) diff --git a/src/run/regional-blob-store.test.ts b/src/run/regional-blob-store.test.ts new file mode 100644 index 0000000000..bfed4ff75b --- /dev/null +++ b/src/run/regional-blob-store.test.ts @@ -0,0 +1,205 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' + +import { decodeBlobKey } from '../../tests/utils/helpers.ts' +import { BlobType } from '../shared/cache-types.cts' + +import { createRequestContext, runWithRequestContext } from './handlers/request-context.cts' +import { getMemoizedKeyValueStoreBackedByRegionalBlobStore } from './regional-blob-store.cts' + +let mockBlobValues: Record<string, unknown> = {} +const mockedStore = { + get: vi.fn((blobKey) => { + const key = decodeBlobKey(blobKey) + return Promise.resolve(mockBlobValues[key]) + }), + setJSON: vi.fn(async (blobKey, value) => { + const key = decodeBlobKey(blobKey) + mockBlobValues[key] = value + }), +} + +vi.mock('@netlify/blobs', () => { + return { + getDeployStore: vi.fn(() => mockedStore), + } +}) + +const OTEL_SPAN_TITLE = 'test' +const TEST_KEY = 'foo' +const TEST_DEFAULT_VALUE = { + revalidatedAt: 123, +} satisfies BlobType + +function generate30MBBlobTypeValue(id: string): BlobType { + return { + lastModified: Date.now(), + value: { + kind: 'ROUTE', + status: 200, + headers: {}, + body: `${id}:${'a'.repeat(30 * 1024 * 1024 - id.length - 1)}`, + }, + } +} + +beforeEach(() => { + mockBlobValues = { + [TEST_KEY]: TEST_DEFAULT_VALUE, + } +}) +describe('getMemoizedKeyValueStoreBackedByRegionalBlobStore', () => { + it('is not using in-memory lookups if not running in request context', async () => { + const store = getMemoizedKeyValueStoreBackedByRegionalBlobStore() + const get1 = await store.get(TEST_KEY, OTEL_SPAN_TITLE) + + expect(mockedStore.get, 'Blobs should be requested').toHaveBeenCalledTimes(1) + expect(get1, 'Expected blob should be returned').toBe(TEST_DEFAULT_VALUE) + + const get2 = await store.get(TEST_KEY, OTEL_SPAN_TITLE) + expect(mockedStore.get, 'Blobs should be requested twice').toHaveBeenCalledTimes(2) + expect(get2, 'Expected second .get to return the same as first one').toBe(get1) + }) + + it('is using in-memory cache when running in request context', async () => { + await runWithRequestContext(createRequestContext(), async () => { + const store = getMemoizedKeyValueStoreBackedByRegionalBlobStore() + + const get1 = await store.get(TEST_KEY, OTEL_SPAN_TITLE) + expect(mockedStore.get, 'Blobs should be requested').toHaveBeenCalledTimes(1) + expect(get1, 'Expected blob should be returned').toBe(TEST_DEFAULT_VALUE) + + const get2 = await store.get(TEST_KEY, OTEL_SPAN_TITLE) + expect(mockedStore.get, 'Blobs should be requested just once').toHaveBeenCalledTimes(1) + expect(get2, 'Expected second .get to return the same as first one').toBe(get1) + }) + }) + + it('can read their own writes without checking blobs', async () => { + await runWithRequestContext(createRequestContext(), async () => { + const store = getMemoizedKeyValueStoreBackedByRegionalBlobStore() + + const writeValue = { + revalidatedAt: 456, + } satisfies BlobType + + await store.set(TEST_KEY, writeValue, OTEL_SPAN_TITLE) + + expect(mockedStore.setJSON, 'Blobs should be posted').toHaveBeenCalledTimes(1) + + const get = await store.get(TEST_KEY, OTEL_SPAN_TITLE) + expect(mockedStore.get, 'Value should be read from memory').toHaveBeenCalledTimes(0) + expect(get, 'Value from memory should be correct').toBe(writeValue) + }) + }) + + it('is using separate in-memory caches when running in request contexts', async () => { + const store = getMemoizedKeyValueStoreBackedByRegionalBlobStore() + await runWithRequestContext(createRequestContext(), async () => { + await store.get(TEST_KEY, OTEL_SPAN_TITLE) + }) + + await runWithRequestContext(createRequestContext(), async () => { + await store.get(TEST_KEY, OTEL_SPAN_TITLE) + }) + + expect( + mockedStore.get, + 'Blobs should be requested separately for each request context', + ).toHaveBeenCalledTimes(2) + }) + + it('writing in one request context should not affect in-memory value in another request context', async () => { + const store = getMemoizedKeyValueStoreBackedByRegionalBlobStore() + + const requestContext1 = createRequestContext() + const requestContext2 = createRequestContext() + + const writeValue = { + revalidatedAt: 456, + } satisfies BlobType + + await runWithRequestContext(requestContext1, async () => { + const get = await store.get(TEST_KEY, OTEL_SPAN_TITLE) + expect(get, 'Value from memory should be the same as before').toBe(TEST_DEFAULT_VALUE) + expect(mockedStore.get, 'Blobs should be requested').toHaveBeenCalledTimes(1) + }) + + await runWithRequestContext(requestContext2, async () => { + mockedStore.get.mockClear() + await store.set(TEST_KEY, writeValue, OTEL_SPAN_TITLE) + const get = await store.get(TEST_KEY, OTEL_SPAN_TITLE) + expect(mockedStore.get, 'Value should be read from memory').toHaveBeenCalledTimes(0) + expect(get, 'Value from memory should be correct').toBe(writeValue) + }) + + await runWithRequestContext(requestContext1, async () => { + mockedStore.get.mockClear() + const get = await store.get(TEST_KEY, OTEL_SPAN_TITLE) + expect( + get, + 'Value from memory should be the same as before and not affected by other request context', + ).toBe(TEST_DEFAULT_VALUE) + expect(mockedStore.get, 'Value should be read from memory').toHaveBeenCalledTimes(0) + }) + }) + + it('in-memory caches share memory limit (~50MB)', async () => { + const store = getMemoizedKeyValueStoreBackedByRegionalBlobStore() + + const requestContext1 = createRequestContext() + const requestContext2 = createRequestContext() + + mockBlobValues = { + // very heavy values that in-memory caches can only hold one value at a time + 'heavy-route-1': generate30MBBlobTypeValue('1'), + 'heavy-route-2': generate30MBBlobTypeValue('2'), + } + + await runWithRequestContext(requestContext1, async () => { + await store.get('heavy-route-1', OTEL_SPAN_TITLE) + expect(mockedStore.get, 'Value should be read from blobs').toHaveBeenCalledTimes(1) + mockedStore.get.mockClear() + + await store.get('heavy-route-1', OTEL_SPAN_TITLE) + expect(mockedStore.get, 'Value should be read from memory').toHaveBeenCalledTimes(0) + mockedStore.get.mockClear() + + await store.get('heavy-route-2', OTEL_SPAN_TITLE) + expect(mockedStore.get, 'Value should be read from blobs').toHaveBeenCalledTimes(1) + mockedStore.get.mockClear() + + // at this point we should exceed the memory limit and least recently used value should be evicted + await store.get('heavy-route-1', OTEL_SPAN_TITLE) + expect( + mockedStore.get, + 'Previously stored in-memory value should be evicted and fresh value should be read from blobs', + ).toHaveBeenCalledTimes(1) + mockedStore.get.mockClear() + + await store.get('heavy-route-1', OTEL_SPAN_TITLE) + expect(mockedStore.get, 'Value should be read from memory again').toHaveBeenCalledTimes(0) + mockedStore.get.mockClear() + }) + + await runWithRequestContext(requestContext2, async () => { + await store.get('heavy-route-1', OTEL_SPAN_TITLE) + expect(mockedStore.get, 'Value should be read from blobs').toHaveBeenCalledTimes(1) + mockedStore.get.mockClear() + + await store.get('heavy-route-1', OTEL_SPAN_TITLE) + expect(mockedStore.get, 'Value should be read from memory').toHaveBeenCalledTimes(0) + mockedStore.get.mockClear() + }) + + await runWithRequestContext(requestContext1, async () => { + await store.get('heavy-route-1', OTEL_SPAN_TITLE) + // operations in requestContext2 should result in evicting value for requestContext1 + expect(mockedStore.get, 'Value should be read from blobs').toHaveBeenCalledTimes(1) + mockedStore.get.mockClear() + + await store.get('heavy-route-1', OTEL_SPAN_TITLE) + expect(mockedStore.get, 'Value should be read from memory').toHaveBeenCalledTimes(0) + mockedStore.get.mockClear() + }) + }) +}) diff --git a/src/shared/cache-types.cts b/src/shared/cache-types.cts index 811860335c..2a3b660666 100644 --- a/src/shared/cache-types.cts +++ b/src/shared/cache-types.cts @@ -1,3 +1,5 @@ +import { isPromise } from 'node:util/types' + import type { CacheHandler, CacheHandlerValue, @@ -10,6 +12,8 @@ import type { IncrementalCacheValue, } from 'next/dist/server/response-cache/types.js' +import { recordWarning } from '../run/handlers/tracer.cjs' + export type { CacheHandlerContext } from 'next/dist/server/lib/incremental-cache/index.js' type CacheControl = { @@ -163,3 +167,51 @@ export type HtmlBlob = { } export type BlobType = NetlifyCacheHandlerValue | TagManifest | HtmlBlob + +const isTagManifest = (value: BlobType): value is TagManifest => { + return false +} + +const isHtmlBlob = (value: BlobType): value is HtmlBlob => { + return false +} + +export const estimateBlobSize = (valueToStore: BlobType | null | Promise<unknown>): number => { + // very approximate size calculation to avoid expensive exact size calculation + // inspired by https://github.com/vercel/next.js/blob/ed10f7ed0246fcc763194197eb9beebcbd063162/packages/next/src/server/lib/incremental-cache/file-system-cache.ts#L60-L79 + if (valueToStore === null || isPromise(valueToStore) || isTagManifest(valueToStore)) { + return 25 + } + if (isHtmlBlob(valueToStore)) { + return valueToStore.html.length + } + let knownKindFailed = false + try { + if (valueToStore.value?.kind === 'FETCH') { + return valueToStore.value.data.body.length + } + if (valueToStore.value?.kind === 'APP_PAGE') { + return valueToStore.value.html.length + (valueToStore.value.rscData?.length ?? 0) + } + if (valueToStore.value?.kind === 'PAGE' || valueToStore.value?.kind === 'PAGES') { + return valueToStore.value.html.length + JSON.stringify(valueToStore.value.pageData).length + } + if (valueToStore.value?.kind === 'ROUTE' || valueToStore.value?.kind === 'APP_ROUTE') { + return valueToStore.value.body.length + } + } catch { + // size calculation rely on the shape of the value, so if it's not what we expect, we fallback to JSON.stringify + knownKindFailed = true + } + + // fallback for not known kinds or known kinds that did fail to calculate size + // we should also monitor cases when fallback is used because it's not the most efficient way to calculate/estimate size + // and might indicate need to make adjustments or additions to the size calculation + recordWarning( + new Error( + `Blob size calculation did fallback to JSON.stringify. Kind: KnownKindFailed: ${knownKindFailed}, ${valueToStore.value?.kind ?? 'undefined'}`, + ), + ) + + return JSON.stringify(valueToStore).length +} From 42977edfb101276ccb4dcd30144000e4f5aac210 Mon Sep 17 00:00:00 2001 From: Michal Piechowiak <misiek.piechowiak@gmail.com> Date: Thu, 27 Mar 2025 08:52:14 +0100 Subject: [PATCH 05/10] refactor: rename RequestSpecificInMemoryCache to RequestScopedInMemoryCache --- src/run/regional-blob-store.cts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/run/regional-blob-store.cts b/src/run/regional-blob-store.cts index 1c60bc8210..7dae336d45 100644 --- a/src/run/regional-blob-store.cts +++ b/src/run/regional-blob-store.cts @@ -103,12 +103,12 @@ function getInMemoryLRUCache() { return extendedGlobalThis[IN_MEMORY_LRU_CACHE] } -interface RequestSpecificInMemoryCache { +interface RequestScopedInMemoryCache { get(key: string): BlobType | null | Promise<BlobType | null> | undefined set(key: string, value: BlobType | null | Promise<BlobType | null>): void } -const noOpInMemoryCache: RequestSpecificInMemoryCache = { +const noOpInMemoryCache: RequestScopedInMemoryCache = { get(): undefined { // no-op }, @@ -117,7 +117,7 @@ const noOpInMemoryCache: RequestSpecificInMemoryCache = { }, } -const getRequestSpecificInMemoryCache = (): RequestSpecificInMemoryCache => { +const getRequestSpecificInMemoryCache = (): RequestScopedInMemoryCache => { const requestContext = getRequestContext() if (!requestContext) { // Fallback to a no-op store if we can't find request context From cff9f73dfefce8feb047e3177251c9b48d6b8f4c Mon Sep 17 00:00:00 2001 From: Michal Piechowiak <misiek.piechowiak@gmail.com> Date: Thu, 27 Mar 2025 09:49:34 +0100 Subject: [PATCH 06/10] refactor: modulurize blobs storage abstractions --- .eslintrc.cjs | 8 + src/run/config.ts | 2 +- src/run/handlers/cache.cts | 2 +- src/run/handlers/server.ts | 2 +- src/run/headers.ts | 2 +- src/run/next.cts | 2 +- src/run/regional-blob-store.cts | 198 ------------------ src/run/storage/regional-blob-store.cts | 62 ++++++ .../request-scoped-in-memory-cache.cts | 84 ++++++++ src/run/storage/storage.cts | 68 ++++++ .../storage.test.ts} | 8 +- tools/build.js | 7 +- 12 files changed, 236 insertions(+), 209 deletions(-) delete mode 100644 src/run/regional-blob-store.cts create mode 100644 src/run/storage/regional-blob-store.cts create mode 100644 src/run/storage/request-scoped-in-memory-cache.cts create mode 100644 src/run/storage/storage.cts rename src/run/{regional-blob-store.test.ts => storage/storage.test.ts} (97%) diff --git a/.eslintrc.cjs b/.eslintrc.cjs index dbcc8975f8..602a670ef1 100644 --- a/.eslintrc.cjs +++ b/.eslintrc.cjs @@ -72,6 +72,14 @@ module.exports = { message: 'Please use `getTracer()` from `./handlers/tracer.cjs` instead', }, ], + patterns: [ + { + // only */storage/storage.cjs is allowed to be imported + // rest are implementation details that should not be used directly + group: ['*/storage/*', '!*/storage/storage.cjs'], + message: 'Import public `[...]/storage/storage.cjs` module instead.', + }, + ], }, ], }, diff --git a/src/run/config.ts b/src/run/config.ts index 4b858b125e..9791c728a9 100644 --- a/src/run/config.ts +++ b/src/run/config.ts @@ -5,7 +5,7 @@ import { join, resolve } from 'node:path' import type { NextConfigComplete } from 'next/dist/server/config-shared.js' import { PLUGIN_DIR, RUN_CONFIG } from './constants.js' -import { setInMemoryCacheMaxSizeFromNextConfig } from './regional-blob-store.cjs' +import { setInMemoryCacheMaxSizeFromNextConfig } from './storage/storage.cjs' /** * Get Next.js config from the build output diff --git a/src/run/handlers/cache.cts b/src/run/handlers/cache.cts index 4652cfc6ce..dced60546a 100644 --- a/src/run/handlers/cache.cts +++ b/src/run/handlers/cache.cts @@ -25,7 +25,7 @@ import { import { getMemoizedKeyValueStoreBackedByRegionalBlobStore, MemoizedKeyValueStoreBackedByRegionalBlobStore, -} from '../regional-blob-store.cjs' +} from '../storage/storage.cjs' import { getLogger, getRequestContext } from './request-context.cjs' import { getTracer, recordWarning } from './tracer.cjs' diff --git a/src/run/handlers/server.ts b/src/run/handlers/server.ts index dff1b6bd60..72666dd5b3 100644 --- a/src/run/handlers/server.ts +++ b/src/run/handlers/server.ts @@ -13,8 +13,8 @@ import { setCacheTagsHeaders, setVaryHeaders, } from '../headers.js' -import { setFetchBeforeNextPatchedIt } from '../regional-blob-store.cjs' import { nextResponseProxy } from '../revalidate.js' +import { setFetchBeforeNextPatchedIt } from '../storage/storage.cjs' import { getLogger, type RequestContext } from './request-context.cjs' import { getTracer } from './tracer.cjs' diff --git a/src/run/headers.ts b/src/run/headers.ts index 967ecc7ada..bfc386506c 100644 --- a/src/run/headers.ts +++ b/src/run/headers.ts @@ -5,7 +5,7 @@ import type { NetlifyCachedRouteValue, NetlifyCacheHandlerValue } from '../share import { getLogger, RequestContext } from './handlers/request-context.cjs' import { recordWarning } from './handlers/tracer.cjs' -import { getMemoizedKeyValueStoreBackedByRegionalBlobStore } from './regional-blob-store.cjs' +import { getMemoizedKeyValueStoreBackedByRegionalBlobStore } from './storage/storage.cjs' const ALL_VARIATIONS = Symbol.for('ALL_VARIATIONS') interface NetlifyVaryValues { diff --git a/src/run/next.cts b/src/run/next.cts index af48b1fbb6..aa2ff5c26f 100644 --- a/src/run/next.cts +++ b/src/run/next.cts @@ -8,7 +8,7 @@ import { HtmlBlob } from '../shared/cache-types.cjs' import { getRequestContext } from './handlers/request-context.cjs' import { getTracer } from './handlers/tracer.cjs' -import { getMemoizedKeyValueStoreBackedByRegionalBlobStore } from './regional-blob-store.cjs' +import { getMemoizedKeyValueStoreBackedByRegionalBlobStore } from './storage/storage.cjs' // https://github.com/vercel/next.js/pull/68193/files#diff-37243d614f1f5d3f7ea50bbf2af263f6b1a9a4f70e84427977781e07b02f57f1R49 // This import resulted in importing unbundled React which depending if NODE_ENV is `production` or not would use diff --git a/src/run/regional-blob-store.cts b/src/run/regional-blob-store.cts deleted file mode 100644 index 7dae336d45..0000000000 --- a/src/run/regional-blob-store.cts +++ /dev/null @@ -1,198 +0,0 @@ -import { getDeployStore, GetWithMetadataOptions, Store } from '@netlify/blobs' -import { LRUCache } from 'lru-cache' - -import { type BlobType, estimateBlobSize } from '../shared/cache-types.cjs' - -import { getRequestContext } from './handlers/request-context.cjs' -import { getTracer } from './handlers/tracer.cjs' - -// lru-cache types don't like using `null` for values, so we use a symbol to represent it and do conversion -// so it doesn't leak outside -const NullValue = Symbol.for('null-value') -type BlobLRUCache = LRUCache<string, BlobType | typeof NullValue | Promise<BlobType | null>> - -const FETCH_BEFORE_NEXT_PATCHED_IT = Symbol.for('nf-not-patched-fetch') -const IN_MEMORY_CACHE_MAX_SIZE = Symbol.for('nf-in-memory-cache-max-size') -const IN_MEMORY_LRU_CACHE = Symbol.for('nf-in-memory-lru-cache') -const extendedGlobalThis = globalThis as typeof globalThis & { - [FETCH_BEFORE_NEXT_PATCHED_IT]?: typeof globalThis.fetch - [IN_MEMORY_CACHE_MAX_SIZE]?: number - [IN_MEMORY_LRU_CACHE]?: BlobLRUCache | null -} - -/** - * Attempt to extract original fetch in case it was patched by Next.js already - * - * @see github.com/vercel/next.js/blob/fa214c74c1d8023098c0e94e57f917ef9f1afd1a/packages/next/src/server/lib/patch-fetch.ts#L986 - */ -function attemptToGetOriginalFetch( - fetch: typeof globalThis.fetch & { - _nextOriginalFetch?: typeof globalThis.fetch - }, -) { - return fetch._nextOriginalFetch ?? fetch -} - -function forceOptOutOfUsingDataCache(fetch: typeof globalThis.fetch): typeof globalThis.fetch { - return (input, init) => { - return fetch(input, { - ...init, - next: { - ...init?.next, - // setting next.internal = true should prevent from trying to use data cache - // https://github.com/vercel/next.js/blob/fa214c74c1d8023098c0e94e57f917ef9f1afd1a/packages/next/src/server/lib/patch-fetch.ts#L174 - // https://github.com/vercel/next.js/blob/fa214c74c1d8023098c0e94e57f917ef9f1afd1a/packages/next/src/server/lib/patch-fetch.ts#L210-L213 - // this is last line of defense in case we didn't manage to get unpatched fetch that will not affect - // fetch if it's unpatched so it should be safe to apply always if we aren't sure if we use patched fetch - - // @ts-expect-error - this is an internal field that Next.js doesn't add to its global - // type overrides for RequestInit type (like `next.revalidate` or `next.tags`) - internal: true, - }, - }) - } -} - -export const setFetchBeforeNextPatchedIt = (fetch: typeof globalThis.fetch) => { - // we store in globalThis in case we have multiple copies of this module - // just as precaution - - extendedGlobalThis[FETCH_BEFORE_NEXT_PATCHED_IT] = forceOptOutOfUsingDataCache( - attemptToGetOriginalFetch(fetch), - ) -} - -const fetchBeforeNextPatchedItFallback = forceOptOutOfUsingDataCache( - attemptToGetOriginalFetch(globalThis.fetch), -) -const getFetchBeforeNextPatchedIt = () => - extendedGlobalThis[FETCH_BEFORE_NEXT_PATCHED_IT] ?? fetchBeforeNextPatchedItFallback - -const getRegionalBlobStore = (args: GetWithMetadataOptions = {}): Store => { - return getDeployStore({ - ...args, - fetch: getFetchBeforeNextPatchedIt(), - region: process.env.USE_REGIONAL_BLOBS?.toUpperCase() === 'TRUE' ? undefined : 'us-east-2', - }) -} -const DEFAULT_FALLBACK_MAX_SIZE = 50 * 1024 * 1024 // 50MB, same as default Next.js config -export function setInMemoryCacheMaxSizeFromNextConfig(size: unknown) { - if (typeof size === 'number') { - extendedGlobalThis[IN_MEMORY_CACHE_MAX_SIZE] = size - } -} - -function getInMemoryLRUCache() { - if (typeof extendedGlobalThis[IN_MEMORY_LRU_CACHE] === 'undefined') { - const maxSize = - typeof extendedGlobalThis[IN_MEMORY_CACHE_MAX_SIZE] === 'number' - ? extendedGlobalThis[IN_MEMORY_CACHE_MAX_SIZE] - : DEFAULT_FALLBACK_MAX_SIZE - - extendedGlobalThis[IN_MEMORY_LRU_CACHE] = - maxSize === 0 - ? null // if user sets 0 in their config, we should honor that and not use in-memory cache - : new LRUCache<string, BlobType | typeof NullValue | Promise<BlobType | null>>({ - max: 1000, - maxSize, - sizeCalculation: (valueToStore) => { - return estimateBlobSize(valueToStore === NullValue ? null : valueToStore) - }, - }) - } - return extendedGlobalThis[IN_MEMORY_LRU_CACHE] -} - -interface RequestScopedInMemoryCache { - get(key: string): BlobType | null | Promise<BlobType | null> | undefined - set(key: string, value: BlobType | null | Promise<BlobType | null>): void -} - -const noOpInMemoryCache: RequestScopedInMemoryCache = { - get(): undefined { - // no-op - }, - set() { - // no-op - }, -} - -const getRequestSpecificInMemoryCache = (): RequestScopedInMemoryCache => { - const requestContext = getRequestContext() - if (!requestContext) { - // Fallback to a no-op store if we can't find request context - return noOpInMemoryCache - } - - const inMemoryLRUCache = getInMemoryLRUCache() - if (inMemoryLRUCache === null) { - return noOpInMemoryCache - } - - return { - get(key) { - const inMemoryValue = inMemoryLRUCache.get(`${requestContext.requestID}:${key}`) - if (inMemoryValue === NullValue) { - return null - } - return inMemoryValue - }, - set(key, value) { - inMemoryLRUCache.set(`${requestContext.requestID}:${key}`, value ?? NullValue) - }, - } -} - -const encodeBlobKey = async (key: string) => { - const { encodeBlobKey: encodeBlobKeyImpl } = await import('../shared/blobkey.js') - return await encodeBlobKeyImpl(key) -} - -export const getMemoizedKeyValueStoreBackedByRegionalBlobStore = ( - args: GetWithMetadataOptions = {}, -) => { - const store = getRegionalBlobStore(args) - const tracer = getTracer() - - return { - async get<T extends BlobType>(key: string, otelSpanTitle: string): Promise<T | null> { - const inMemoryCache = getRequestSpecificInMemoryCache() - - const memoizedValue = inMemoryCache.get(key) - if (typeof memoizedValue !== 'undefined') { - return memoizedValue as T | null | Promise<T | null> - } - - const blobKey = await encodeBlobKey(key) - const getPromise = tracer.withActiveSpan(otelSpanTitle, async (span) => { - span.setAttributes({ key, blobKey }) - const blob = (await store.get(blobKey, { type: 'json' })) as T | null - inMemoryCache.set(key, blob) - span.addEvent(blob ? 'Hit' : 'Miss') - return blob - }) - inMemoryCache.set(key, getPromise) - return getPromise - }, - async set(key: string, value: BlobType, otelSpanTitle: string) { - const inMemoryCache = getRequestSpecificInMemoryCache() - - inMemoryCache.set(key, value) - - const blobKey = await encodeBlobKey(key) - return tracer.withActiveSpan(otelSpanTitle, async (span) => { - span.setAttributes({ key, blobKey }) - return await store.setJSON(blobKey, value) - }) - }, - } -} - -/** - * Wrapper around Blobs Store that memoizes the cache entries within context of a request - * to avoid duplicate requests to the same key and also allowing to read its own writes from - * memory. - */ -export type MemoizedKeyValueStoreBackedByRegionalBlobStore = ReturnType< - typeof getMemoizedKeyValueStoreBackedByRegionalBlobStore -> diff --git a/src/run/storage/regional-blob-store.cts b/src/run/storage/regional-blob-store.cts new file mode 100644 index 0000000000..62aec1db4b --- /dev/null +++ b/src/run/storage/regional-blob-store.cts @@ -0,0 +1,62 @@ +import { getDeployStore, GetWithMetadataOptions, Store } from '@netlify/blobs' + +const FETCH_BEFORE_NEXT_PATCHED_IT = Symbol.for('nf-not-patched-fetch') +const extendedGlobalThis = globalThis as typeof globalThis & { + [FETCH_BEFORE_NEXT_PATCHED_IT]?: typeof globalThis.fetch +} + +/** + * Attempt to extract original fetch in case it was patched by Next.js already + * + * @see github.com/vercel/next.js/blob/fa214c74c1d8023098c0e94e57f917ef9f1afd1a/packages/next/src/server/lib/patch-fetch.ts#L986 + */ +function attemptToGetOriginalFetch( + fetch: typeof globalThis.fetch & { + _nextOriginalFetch?: typeof globalThis.fetch + }, +) { + return fetch._nextOriginalFetch ?? fetch +} + +function forceOptOutOfUsingDataCache(fetch: typeof globalThis.fetch): typeof globalThis.fetch { + return (input, init) => { + return fetch(input, { + ...init, + next: { + ...init?.next, + // setting next.internal = true should prevent from trying to use data cache + // https://github.com/vercel/next.js/blob/fa214c74c1d8023098c0e94e57f917ef9f1afd1a/packages/next/src/server/lib/patch-fetch.ts#L174 + // https://github.com/vercel/next.js/blob/fa214c74c1d8023098c0e94e57f917ef9f1afd1a/packages/next/src/server/lib/patch-fetch.ts#L210-L213 + // this is last line of defense in case we didn't manage to get unpatched fetch that will not affect + // fetch if it's unpatched so it should be safe to apply always if we aren't sure if we use patched fetch + + // @ts-expect-error - this is an internal field that Next.js doesn't add to its global + // type overrides for RequestInit type (like `next.revalidate` or `next.tags`) + internal: true, + }, + }) + } +} + +export const setFetchBeforeNextPatchedIt = (fetch: typeof globalThis.fetch) => { + // we store in globalThis in case we have multiple copies of this module + // just as precaution + + extendedGlobalThis[FETCH_BEFORE_NEXT_PATCHED_IT] = forceOptOutOfUsingDataCache( + attemptToGetOriginalFetch(fetch), + ) +} + +const fetchBeforeNextPatchedItFallback = forceOptOutOfUsingDataCache( + attemptToGetOriginalFetch(globalThis.fetch), +) +const getFetchBeforeNextPatchedIt = () => + extendedGlobalThis[FETCH_BEFORE_NEXT_PATCHED_IT] ?? fetchBeforeNextPatchedItFallback + +export const getRegionalBlobStore = (args: GetWithMetadataOptions = {}): Store => { + return getDeployStore({ + ...args, + fetch: getFetchBeforeNextPatchedIt(), + region: process.env.USE_REGIONAL_BLOBS?.toUpperCase() === 'TRUE' ? undefined : 'us-east-2', + }) +} diff --git a/src/run/storage/request-scoped-in-memory-cache.cts b/src/run/storage/request-scoped-in-memory-cache.cts new file mode 100644 index 0000000000..801a43a7fe --- /dev/null +++ b/src/run/storage/request-scoped-in-memory-cache.cts @@ -0,0 +1,84 @@ +import { LRUCache } from 'lru-cache' + +import { type BlobType, estimateBlobSize } from '../../shared/cache-types.cjs' +import { getRequestContext } from '../handlers/request-context.cjs' + +// lru-cache types don't like using `null` for values, so we use a symbol to represent it and do conversion +// so it doesn't leak outside +const NullValue = Symbol.for('null-value') +type BlobLRUCache = LRUCache<string, BlobType | typeof NullValue | Promise<BlobType | null>> + +const IN_MEMORY_CACHE_MAX_SIZE = Symbol.for('nf-in-memory-cache-max-size') +const IN_MEMORY_LRU_CACHE = Symbol.for('nf-in-memory-lru-cache') +const extendedGlobalThis = globalThis as typeof globalThis & { + [IN_MEMORY_CACHE_MAX_SIZE]?: number + [IN_MEMORY_LRU_CACHE]?: BlobLRUCache | null +} + +const DEFAULT_FALLBACK_MAX_SIZE = 50 * 1024 * 1024 // 50MB, same as default Next.js config +export function setInMemoryCacheMaxSizeFromNextConfig(size: unknown) { + if (typeof size === 'number') { + extendedGlobalThis[IN_MEMORY_CACHE_MAX_SIZE] = size + } +} + +function getInMemoryLRUCache() { + if (typeof extendedGlobalThis[IN_MEMORY_LRU_CACHE] === 'undefined') { + const maxSize = + typeof extendedGlobalThis[IN_MEMORY_CACHE_MAX_SIZE] === 'number' + ? extendedGlobalThis[IN_MEMORY_CACHE_MAX_SIZE] + : DEFAULT_FALLBACK_MAX_SIZE + + extendedGlobalThis[IN_MEMORY_LRU_CACHE] = + maxSize === 0 + ? null // if user sets 0 in their config, we should honor that and not use in-memory cache + : new LRUCache<string, BlobType | typeof NullValue | Promise<BlobType | null>>({ + max: 1000, + maxSize, + sizeCalculation: (valueToStore) => { + return estimateBlobSize(valueToStore === NullValue ? null : valueToStore) + }, + }) + } + return extendedGlobalThis[IN_MEMORY_LRU_CACHE] +} + +interface RequestScopedInMemoryCache { + get(key: string): BlobType | null | Promise<BlobType | null> | undefined + set(key: string, value: BlobType | null | Promise<BlobType | null>): void +} + +const noOpInMemoryCache: RequestScopedInMemoryCache = { + get(): undefined { + // no-op + }, + set() { + // no-op + }, +} + +export const getRequestSpecificInMemoryCache = (): RequestScopedInMemoryCache => { + const requestContext = getRequestContext() + if (!requestContext) { + // Fallback to a no-op store if we can't find request context + return noOpInMemoryCache + } + + const inMemoryLRUCache = getInMemoryLRUCache() + if (inMemoryLRUCache === null) { + return noOpInMemoryCache + } + + return { + get(key) { + const inMemoryValue = inMemoryLRUCache.get(`${requestContext.requestID}:${key}`) + if (inMemoryValue === NullValue) { + return null + } + return inMemoryValue + }, + set(key, value) { + inMemoryLRUCache.set(`${requestContext.requestID}:${key}`, value ?? NullValue) + }, + } +} diff --git a/src/run/storage/storage.cts b/src/run/storage/storage.cts new file mode 100644 index 0000000000..bd7b65b292 --- /dev/null +++ b/src/run/storage/storage.cts @@ -0,0 +1,68 @@ +// This is storage module that rest of modules should interact with. +// Remaining modules in storage directory are implementation details +// and should not be used directly outside of this directory. +// There is eslint `no-restricted-imports` rule to enforce this. + +import { type BlobType } from '../../shared/cache-types.cjs' +import { getTracer } from '../handlers/tracer.cjs' + +import { getRequestSpecificInMemoryCache } from './request-scoped-in-memory-cache.cjs' +import { getRegionalBlobStore } from './regional-blob-store.cjs' + +const encodeBlobKey = async (key: string) => { + const { encodeBlobKey: encodeBlobKeyImpl } = await import('../../shared/blobkey.js') + return await encodeBlobKeyImpl(key) +} + +export const getMemoizedKeyValueStoreBackedByRegionalBlobStore = ( + ...args: Parameters<typeof getRegionalBlobStore> +) => { + const store = getRegionalBlobStore(...args) + const tracer = getTracer() + + return { + async get<T extends BlobType>(key: string, otelSpanTitle: string): Promise<T | null> { + const inMemoryCache = getRequestSpecificInMemoryCache() + + const memoizedValue = inMemoryCache.get(key) + if (typeof memoizedValue !== 'undefined') { + return memoizedValue as T | null | Promise<T | null> + } + + const blobKey = await encodeBlobKey(key) + const getPromise = tracer.withActiveSpan(otelSpanTitle, async (span) => { + span.setAttributes({ key, blobKey }) + const blob = (await store.get(blobKey, { type: 'json' })) as T | null + inMemoryCache.set(key, blob) + span.addEvent(blob ? 'Hit' : 'Miss') + return blob + }) + inMemoryCache.set(key, getPromise) + return getPromise + }, + async set(key: string, value: BlobType, otelSpanTitle: string) { + const inMemoryCache = getRequestSpecificInMemoryCache() + + inMemoryCache.set(key, value) + + const blobKey = await encodeBlobKey(key) + return tracer.withActiveSpan(otelSpanTitle, async (span) => { + span.setAttributes({ key, blobKey }) + return await store.setJSON(blobKey, value) + }) + }, + } +} + +/** + * Wrapper around Blobs Store that memoizes the cache entries within context of a request + * to avoid duplicate requests to the same key and also allowing to read its own writes from + * memory. + */ +export type MemoizedKeyValueStoreBackedByRegionalBlobStore = ReturnType< + typeof getMemoizedKeyValueStoreBackedByRegionalBlobStore +> + +// make select methods public +export { setInMemoryCacheMaxSizeFromNextConfig } from './request-scoped-in-memory-cache.cjs' +export { setFetchBeforeNextPatchedIt } from './regional-blob-store.cjs' diff --git a/src/run/regional-blob-store.test.ts b/src/run/storage/storage.test.ts similarity index 97% rename from src/run/regional-blob-store.test.ts rename to src/run/storage/storage.test.ts index bfed4ff75b..e36e8e3f38 100644 --- a/src/run/regional-blob-store.test.ts +++ b/src/run/storage/storage.test.ts @@ -1,10 +1,10 @@ import { beforeEach, describe, expect, it, vi } from 'vitest' -import { decodeBlobKey } from '../../tests/utils/helpers.ts' -import { BlobType } from '../shared/cache-types.cts' +import { decodeBlobKey } from '../../../tests/utils/helpers.ts' +import { BlobType } from '../../shared/cache-types.cts' +import { createRequestContext, runWithRequestContext } from '../handlers/request-context.cts' -import { createRequestContext, runWithRequestContext } from './handlers/request-context.cts' -import { getMemoizedKeyValueStoreBackedByRegionalBlobStore } from './regional-blob-store.cts' +import { getMemoizedKeyValueStoreBackedByRegionalBlobStore } from './storage.cts' let mockBlobValues: Record<string, unknown> = {} const mockedStore = { diff --git a/tools/build.js b/tools/build.js index c622a26d67..2c7f9fdecb 100644 --- a/tools/build.js +++ b/tools/build.js @@ -131,14 +131,17 @@ await Promise.all([ async function ensureNoRegionalBlobsModuleDuplicates() { const REGIONAL_BLOB_STORE_CONTENT_TO_FIND = 'fetchBeforeNextPatchedIt' + const EXPECTED_MODULE_TO_CONTAIN_FETCH_BEFORE_NEXT_PATCHED_IT = + 'run/storage/regional-blob-store.cjs' const filesToTest = await glob(`${OUT_DIR}/**/*.{js,cjs}`) const unexpectedModulesContainingFetchBeforeNextPatchedIt = [] let foundInExpectedModule = false + for (const fileToTest of filesToTest) { const content = await readFile(fileToTest, 'utf-8') if (content.includes(REGIONAL_BLOB_STORE_CONTENT_TO_FIND)) { - if (fileToTest.endsWith('run/regional-blob-store.cjs')) { + if (fileToTest.endsWith(EXPECTED_MODULE_TO_CONTAIN_FETCH_BEFORE_NEXT_PATCHED_IT)) { foundInExpectedModule = true } else { unexpectedModulesContainingFetchBeforeNextPatchedIt.push(fileToTest) @@ -147,7 +150,7 @@ async function ensureNoRegionalBlobsModuleDuplicates() { } if (!foundInExpectedModule) { throw new Error( - 'Expected to find "fetchBeforeNextPatchedIt" variable in "run/regional-blob-store.cjs", but it was not found. This might indicate a setup change that requires the bundling validation in "tools/build.js" to be adjusted.', + `Expected to find "fetchBeforeNextPatchedIt" variable in "${EXPECTED_MODULE_TO_CONTAIN_FETCH_BEFORE_NEXT_PATCHED_IT}", but it was not found. This might indicate a setup change that requires the bundling validation in "tools/build.js" to be adjusted.`, ) } if (unexpectedModulesContainingFetchBeforeNextPatchedIt.length !== 0) { From d2a7fac0a2a2674416ab60aecd84b5f710d00637 Mon Sep 17 00:00:00 2001 From: Michal Piechowiak <misiek.piechowiak@gmail.com> Date: Thu, 27 Mar 2025 10:23:52 +0100 Subject: [PATCH 07/10] chore: fix lint --- src/run/storage/storage.cts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/run/storage/storage.cts b/src/run/storage/storage.cts index bd7b65b292..f08702b529 100644 --- a/src/run/storage/storage.cts +++ b/src/run/storage/storage.cts @@ -6,8 +6,8 @@ import { type BlobType } from '../../shared/cache-types.cjs' import { getTracer } from '../handlers/tracer.cjs' -import { getRequestSpecificInMemoryCache } from './request-scoped-in-memory-cache.cjs' import { getRegionalBlobStore } from './regional-blob-store.cjs' +import { getRequestSpecificInMemoryCache } from './request-scoped-in-memory-cache.cjs' const encodeBlobKey = async (key: string) => { const { encodeBlobKey: encodeBlobKeyImpl } = await import('../../shared/blobkey.js') From ca233efee8b35879bfeeaf491138f380bbe3a642 Mon Sep 17 00:00:00 2001 From: Michal Piechowiak <misiek.piechowiak@gmail.com> Date: Thu, 27 Mar 2025 11:57:28 +0100 Subject: [PATCH 08/10] refactor: move non-CacheHandler related types and utils from cache-types to blob-types module --- src/build/content/static.ts | 2 +- src/run/handlers/cache.cts | 2 +- src/run/next.cts | 2 +- .../request-scoped-in-memory-cache.cts | 45 +++++++++++++- src/run/storage/storage.cts | 2 +- src/shared/blob-types.cts | 18 ++++++ src/shared/cache-types.cts | 61 ------------------- 7 files changed, 66 insertions(+), 66 deletions(-) create mode 100644 src/shared/blob-types.cts diff --git a/src/build/content/static.ts b/src/build/content/static.ts index 0d80425bdb..d251a17d97 100644 --- a/src/build/content/static.ts +++ b/src/build/content/static.ts @@ -6,8 +6,8 @@ import { trace } from '@opentelemetry/api' import { wrapTracer } from '@opentelemetry/api/experimental' import glob from 'fast-glob' +import type { HtmlBlob } from '../../shared/blob-types.cjs' import { encodeBlobKey } from '../../shared/blobkey.js' -import type { HtmlBlob } from '../../shared/cache-types.cjs' import { PluginContext } from '../plugin-context.js' import { verifyNetlifyForms } from '../verification.js' diff --git a/src/run/handlers/cache.cts b/src/run/handlers/cache.cts index dced60546a..294700901c 100644 --- a/src/run/handlers/cache.cts +++ b/src/run/handlers/cache.cts @@ -11,6 +11,7 @@ import type { PrerenderManifest } from 'next/dist/build/index.js' import { NEXT_CACHE_TAGS_HEADER } from 'next/dist/lib/constants.js' import { name as nextRuntimePkgName, version as nextRuntimePkgVersion } from '../../../package.json' +import { type TagManifest } from '../../shared/blob-types.cjs' import { type CacheHandlerContext, type CacheHandlerForMultipleVersions, @@ -20,7 +21,6 @@ import { type NetlifyCachedRouteValue, type NetlifyCacheHandlerValue, type NetlifyIncrementalCacheValue, - type TagManifest, } from '../../shared/cache-types.cjs' import { getMemoizedKeyValueStoreBackedByRegionalBlobStore, diff --git a/src/run/next.cts b/src/run/next.cts index aa2ff5c26f..fc6b3fcbef 100644 --- a/src/run/next.cts +++ b/src/run/next.cts @@ -4,7 +4,7 @@ import { relative, resolve } from 'path' // @ts-expect-error no types installed import { patchFs } from 'fs-monkey' -import { HtmlBlob } from '../shared/cache-types.cjs' +import { HtmlBlob } from '../shared/blob-types.cjs' import { getRequestContext } from './handlers/request-context.cjs' import { getTracer } from './handlers/tracer.cjs' diff --git a/src/run/storage/request-scoped-in-memory-cache.cts b/src/run/storage/request-scoped-in-memory-cache.cts index 801a43a7fe..d16b37dc7e 100644 --- a/src/run/storage/request-scoped-in-memory-cache.cts +++ b/src/run/storage/request-scoped-in-memory-cache.cts @@ -1,7 +1,10 @@ +import { isPromise } from 'node:util/types' + import { LRUCache } from 'lru-cache' -import { type BlobType, estimateBlobSize } from '../../shared/cache-types.cjs' +import { type BlobType, isHtmlBlob, isTagManifest } from '../../shared/blob-types.cjs' import { getRequestContext } from '../handlers/request-context.cjs' +import { recordWarning } from '../handlers/tracer.cjs' // lru-cache types don't like using `null` for values, so we use a symbol to represent it and do conversion // so it doesn't leak outside @@ -22,6 +25,46 @@ export function setInMemoryCacheMaxSizeFromNextConfig(size: unknown) { } } +const estimateBlobSize = (valueToStore: BlobType | null | Promise<unknown>): number => { + // very approximate size calculation to avoid expensive exact size calculation + // inspired by https://github.com/vercel/next.js/blob/ed10f7ed0246fcc763194197eb9beebcbd063162/packages/next/src/server/lib/incremental-cache/file-system-cache.ts#L60-L79 + if (valueToStore === null || isPromise(valueToStore) || isTagManifest(valueToStore)) { + return 25 + } + if (isHtmlBlob(valueToStore)) { + return valueToStore.html.length + } + let knownKindFailed = false + try { + if (valueToStore.value?.kind === 'FETCH') { + return valueToStore.value.data.body.length + } + if (valueToStore.value?.kind === 'APP_PAGE') { + return valueToStore.value.html.length + (valueToStore.value.rscData?.length ?? 0) + } + if (valueToStore.value?.kind === 'PAGE' || valueToStore.value?.kind === 'PAGES') { + return valueToStore.value.html.length + JSON.stringify(valueToStore.value.pageData).length + } + if (valueToStore.value?.kind === 'ROUTE' || valueToStore.value?.kind === 'APP_ROUTE') { + return valueToStore.value.body.length + } + } catch { + // size calculation rely on the shape of the value, so if it's not what we expect, we fallback to JSON.stringify + knownKindFailed = true + } + + // fallback for not known kinds or known kinds that did fail to calculate size + // we should also monitor cases when fallback is used because it's not the most efficient way to calculate/estimate size + // and might indicate need to make adjustments or additions to the size calculation + recordWarning( + new Error( + `Blob size calculation did fallback to JSON.stringify. Kind: KnownKindFailed: ${knownKindFailed}, ${valueToStore.value?.kind ?? 'undefined'}`, + ), + ) + + return JSON.stringify(valueToStore).length +} + function getInMemoryLRUCache() { if (typeof extendedGlobalThis[IN_MEMORY_LRU_CACHE] === 'undefined') { const maxSize = diff --git a/src/run/storage/storage.cts b/src/run/storage/storage.cts index f08702b529..937a6ccc45 100644 --- a/src/run/storage/storage.cts +++ b/src/run/storage/storage.cts @@ -3,7 +3,7 @@ // and should not be used directly outside of this directory. // There is eslint `no-restricted-imports` rule to enforce this. -import { type BlobType } from '../../shared/cache-types.cjs' +import { type BlobType } from '../../shared/blob-types.cjs' import { getTracer } from '../handlers/tracer.cjs' import { getRegionalBlobStore } from './regional-blob-store.cjs' diff --git a/src/shared/blob-types.cts b/src/shared/blob-types.cts new file mode 100644 index 0000000000..6e0ada28d3 --- /dev/null +++ b/src/shared/blob-types.cts @@ -0,0 +1,18 @@ +import { type NetlifyCacheHandlerValue } from './cache-types.cjs' + +export type TagManifest = { revalidatedAt: number } + +export type HtmlBlob = { + html: string + isFallback: boolean +} + +export type BlobType = NetlifyCacheHandlerValue | TagManifest | HtmlBlob + +export const isTagManifest = (value: BlobType): value is TagManifest => { + return false +} + +export const isHtmlBlob = (value: BlobType): value is HtmlBlob => { + return false +} diff --git a/src/shared/cache-types.cts b/src/shared/cache-types.cts index 2a3b660666..defba8e6fa 100644 --- a/src/shared/cache-types.cts +++ b/src/shared/cache-types.cts @@ -1,5 +1,3 @@ -import { isPromise } from 'node:util/types' - import type { CacheHandler, CacheHandlerValue, @@ -12,8 +10,6 @@ import type { IncrementalCacheValue, } from 'next/dist/server/response-cache/types.js' -import { recordWarning } from '../run/handlers/tracer.cjs' - export type { CacheHandlerContext } from 'next/dist/server/lib/incremental-cache/index.js' type CacheControl = { @@ -158,60 +154,3 @@ export type CacheHandlerForMultipleVersions = BaseCacheHandlerForMultipleVersion context: CacheHandlerSetContextForMultipleVersions, ) => ReturnType<BaseCacheHandlerForMultipleVersions['set']> } - -export type TagManifest = { revalidatedAt: number } - -export type HtmlBlob = { - html: string - isFallback: boolean -} - -export type BlobType = NetlifyCacheHandlerValue | TagManifest | HtmlBlob - -const isTagManifest = (value: BlobType): value is TagManifest => { - return false -} - -const isHtmlBlob = (value: BlobType): value is HtmlBlob => { - return false -} - -export const estimateBlobSize = (valueToStore: BlobType | null | Promise<unknown>): number => { - // very approximate size calculation to avoid expensive exact size calculation - // inspired by https://github.com/vercel/next.js/blob/ed10f7ed0246fcc763194197eb9beebcbd063162/packages/next/src/server/lib/incremental-cache/file-system-cache.ts#L60-L79 - if (valueToStore === null || isPromise(valueToStore) || isTagManifest(valueToStore)) { - return 25 - } - if (isHtmlBlob(valueToStore)) { - return valueToStore.html.length - } - let knownKindFailed = false - try { - if (valueToStore.value?.kind === 'FETCH') { - return valueToStore.value.data.body.length - } - if (valueToStore.value?.kind === 'APP_PAGE') { - return valueToStore.value.html.length + (valueToStore.value.rscData?.length ?? 0) - } - if (valueToStore.value?.kind === 'PAGE' || valueToStore.value?.kind === 'PAGES') { - return valueToStore.value.html.length + JSON.stringify(valueToStore.value.pageData).length - } - if (valueToStore.value?.kind === 'ROUTE' || valueToStore.value?.kind === 'APP_ROUTE') { - return valueToStore.value.body.length - } - } catch { - // size calculation rely on the shape of the value, so if it's not what we expect, we fallback to JSON.stringify - knownKindFailed = true - } - - // fallback for not known kinds or known kinds that did fail to calculate size - // we should also monitor cases when fallback is used because it's not the most efficient way to calculate/estimate size - // and might indicate need to make adjustments or additions to the size calculation - recordWarning( - new Error( - `Blob size calculation did fallback to JSON.stringify. Kind: KnownKindFailed: ${knownKindFailed}, ${valueToStore.value?.kind ?? 'undefined'}`, - ), - ) - - return JSON.stringify(valueToStore).length -} From 578fbc54fb33f29febea2bf770af713e817c19b9 Mon Sep 17 00:00:00 2001 From: Michal Piechowiak <misiek.piechowiak@gmail.com> Date: Thu, 27 Mar 2025 12:23:03 +0100 Subject: [PATCH 09/10] fix: add missing implementation for isTagManifest and isHtmlBlob type guards --- src/shared/blob-types.cts | 18 ++++++++++++++++-- src/shared/blob-types.test.ts | 27 +++++++++++++++++++++++++++ 2 files changed, 43 insertions(+), 2 deletions(-) create mode 100644 src/shared/blob-types.test.ts diff --git a/src/shared/blob-types.cts b/src/shared/blob-types.cts index 6e0ada28d3..976ffdb79c 100644 --- a/src/shared/blob-types.cts +++ b/src/shared/blob-types.cts @@ -10,9 +10,23 @@ export type HtmlBlob = { export type BlobType = NetlifyCacheHandlerValue | TagManifest | HtmlBlob export const isTagManifest = (value: BlobType): value is TagManifest => { - return false + return ( + typeof value === 'object' && + value !== null && + 'revalidatedAt' in value && + typeof value.revalidatedAt === 'number' && + Object.keys(value).length === 1 + ) } export const isHtmlBlob = (value: BlobType): value is HtmlBlob => { - return false + return ( + typeof value === 'object' && + value !== null && + 'html' in value && + 'isFallback' in value && + typeof value.html === 'string' && + typeof value.isFallback === 'boolean' && + Object.keys(value).length === 2 + ) } diff --git a/src/shared/blob-types.test.ts b/src/shared/blob-types.test.ts new file mode 100644 index 0000000000..e41a992045 --- /dev/null +++ b/src/shared/blob-types.test.ts @@ -0,0 +1,27 @@ +import { describe, expect, it } from 'vitest' + +import { BlobType, HtmlBlob, isHtmlBlob, isTagManifest, TagManifest } from './blob-types.cjs' + +describe('isTagManifest', () => { + it(`returns true for TagManifest instance`, () => { + const value: TagManifest = { revalidatedAt: 0 } + expect(isTagManifest(value)).toBe(true) + }) + + it(`returns false for non-TagManifest instance`, () => { + const value: BlobType = { html: '', isFallback: false } + expect(isTagManifest(value)).toBe(false) + }) +}) + +describe('isHtmlBlob', () => { + it(`returns true for HtmlBlob instance`, () => { + const value: HtmlBlob = { html: '', isFallback: false } + expect(isHtmlBlob(value)).toBe(true) + }) + + it(`returns false for non-HtmlBlob instance`, () => { + const value: BlobType = { revalidatedAt: 0 } + expect(isHtmlBlob(value)).toBe(false) + }) +}) From 7daeeca6604ec8a874cabf4b1d71aabf776bd963 Mon Sep 17 00:00:00 2001 From: Mateusz Bocian <mrstork@users.noreply.github.com> Date: Thu, 27 Mar 2025 15:38:18 -0400 Subject: [PATCH 10/10] chore: drop use of noOpInMemoryCache --- .../request-scoped-in-memory-cache.cts | 30 ++++--------------- src/run/storage/storage.cts | 6 ++-- 2 files changed, 9 insertions(+), 27 deletions(-) diff --git a/src/run/storage/request-scoped-in-memory-cache.cts b/src/run/storage/request-scoped-in-memory-cache.cts index d16b37dc7e..8a046c3fe1 100644 --- a/src/run/storage/request-scoped-in-memory-cache.cts +++ b/src/run/storage/request-scoped-in-memory-cache.cts @@ -91,37 +91,19 @@ interface RequestScopedInMemoryCache { set(key: string, value: BlobType | null | Promise<BlobType | null>): void } -const noOpInMemoryCache: RequestScopedInMemoryCache = { - get(): undefined { - // no-op - }, - set() { - // no-op - }, -} - -export const getRequestSpecificInMemoryCache = (): RequestScopedInMemoryCache => { +export const getRequestScopedInMemoryCache = (): RequestScopedInMemoryCache => { const requestContext = getRequestContext() - if (!requestContext) { - // Fallback to a no-op store if we can't find request context - return noOpInMemoryCache - } - const inMemoryLRUCache = getInMemoryLRUCache() - if (inMemoryLRUCache === null) { - return noOpInMemoryCache - } return { get(key) { - const inMemoryValue = inMemoryLRUCache.get(`${requestContext.requestID}:${key}`) - if (inMemoryValue === NullValue) { - return null - } - return inMemoryValue + if (!requestContext) return + const value = inMemoryLRUCache?.get(`${requestContext.requestID}:${key}`) + return value === NullValue ? null : value }, set(key, value) { - inMemoryLRUCache.set(`${requestContext.requestID}:${key}`, value ?? NullValue) + if (!requestContext) return + inMemoryLRUCache?.set(`${requestContext?.requestID}:${key}`, value ?? NullValue) }, } } diff --git a/src/run/storage/storage.cts b/src/run/storage/storage.cts index 937a6ccc45..273c8822de 100644 --- a/src/run/storage/storage.cts +++ b/src/run/storage/storage.cts @@ -7,7 +7,7 @@ import { type BlobType } from '../../shared/blob-types.cjs' import { getTracer } from '../handlers/tracer.cjs' import { getRegionalBlobStore } from './regional-blob-store.cjs' -import { getRequestSpecificInMemoryCache } from './request-scoped-in-memory-cache.cjs' +import { getRequestScopedInMemoryCache } from './request-scoped-in-memory-cache.cjs' const encodeBlobKey = async (key: string) => { const { encodeBlobKey: encodeBlobKeyImpl } = await import('../../shared/blobkey.js') @@ -22,7 +22,7 @@ export const getMemoizedKeyValueStoreBackedByRegionalBlobStore = ( return { async get<T extends BlobType>(key: string, otelSpanTitle: string): Promise<T | null> { - const inMemoryCache = getRequestSpecificInMemoryCache() + const inMemoryCache = getRequestScopedInMemoryCache() const memoizedValue = inMemoryCache.get(key) if (typeof memoizedValue !== 'undefined') { @@ -41,7 +41,7 @@ export const getMemoizedKeyValueStoreBackedByRegionalBlobStore = ( return getPromise }, async set(key: string, value: BlobType, otelSpanTitle: string) { - const inMemoryCache = getRequestSpecificInMemoryCache() + const inMemoryCache = getRequestScopedInMemoryCache() inMemoryCache.set(key, value)