Skip to content

fix: handle case of zero-length cacheable route handler responses #2819

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
89 changes: 65 additions & 24 deletions src/run/storage/request-scoped-in-memory-cache.cts
Original file line number Diff line number Diff line change
Expand Up @@ -25,44 +25,71 @@ export function setInMemoryCacheMaxSizeFromNextConfig(size: unknown) {
}
}

const estimateBlobSize = (valueToStore: BlobType | null | Promise<unknown>): number => {
type PositiveNumber = number & { __positive: true }
const isPositiveNumber = (value: unknown): value is PositiveNumber => {
return typeof value === 'number' && value > 0
}

const BASE_BLOB_SIZE = 25 as PositiveNumber

const estimateBlobKnownTypeSize = (
valueToStore: BlobType | null | Promise<unknown>,
): number | undefined => {
// 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
return BASE_BLOB_SIZE
}
if (isHtmlBlob(valueToStore)) {
return valueToStore.html.length
return BASE_BLOB_SIZE + valueToStore.html.length
}

if (valueToStore.value?.kind === 'FETCH') {
return BASE_BLOB_SIZE + valueToStore.value.data.body.length
}
if (valueToStore.value?.kind === 'APP_PAGE') {
return (
BASE_BLOB_SIZE + valueToStore.value.html.length + (valueToStore.value.rscData?.length ?? 0)
)
}
if (valueToStore.value?.kind === 'PAGE' || valueToStore.value?.kind === 'PAGES') {
return (
BASE_BLOB_SIZE +
valueToStore.value.html.length +
JSON.stringify(valueToStore.value.pageData).length
)
}
let knownKindFailed = false
if (valueToStore.value?.kind === 'ROUTE' || valueToStore.value?.kind === 'APP_ROUTE') {
return BASE_BLOB_SIZE + valueToStore.value.body.length
Copy link
Contributor Author

@pieh pieh Apr 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This in particular is the fix for added test case.

But using it as example, I did add more adjustments:

  • ensure we add "base" blob size to all known cases
  • ensure estimateBlobSize returns "PositiveNumber"
  • make using in-memory store non-fatal - this is optimization, but failures related to it should NOT cause crashes. I did add recording of failures there so we can adjust code if problems happen still

}
}

const estimateBlobSize = (valueToStore: BlobType | null | Promise<unknown>): PositiveNumber => {
let knownTypeFailed = false
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically, you could omit this var now that you're using estimateBlobKnownTypeSizeError. It's only used in the warning message below, which could be replaced by checking if estimateBlobKnownTypeSizeError is not undefined, but it's not a blocker, just something to consider next time you're touching the code here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll adjust now - it's unlikely this will be done as follow up

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

f1ab628 dropped that extra unneded variable. Also added calculated size to warning just for it to have a bit more information

let estimatedKnownTypeSize: number | undefined
let estimateBlobKnownTypeSizeError: unknown
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
estimatedKnownTypeSize = estimateBlobKnownTypeSize(valueToStore)
if (isPositiveNumber(estimatedKnownTypeSize)) {
return estimatedKnownTypeSize
}
} 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
} catch (error) {
knownTypeFailed = true
estimateBlobKnownTypeSizeError = error
}

// fallback for not known kinds or known kinds that did fail to calculate size
// fallback for not known kinds or known kinds that did fail to calculate positive 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'}`,
`Blob size calculation did fallback to JSON.stringify. KnownTypeFailed: ${knownTypeFailed}, EstimatedKnownTypeSize: ${estimatedKnownTypeSize}, ValueToStore: ${JSON.stringify(valueToStore)}`,
estimateBlobKnownTypeSizeError ? { cause: estimateBlobKnownTypeSizeError } : undefined,
),
)

return JSON.stringify(valueToStore).length
const calculatedSize = JSON.stringify(valueToStore).length
return isPositiveNumber(calculatedSize) ? calculatedSize : BASE_BLOB_SIZE
}

function getInMemoryLRUCache() {
Expand Down Expand Up @@ -98,12 +125,26 @@ export const getRequestScopedInMemoryCache = (): RequestScopedInMemoryCache => {
return {
get(key) {
if (!requestContext) return
const value = inMemoryLRUCache?.get(`${requestContext.requestID}:${key}`)
return value === NullValue ? null : value
try {
const value = inMemoryLRUCache?.get(`${requestContext.requestID}:${key}`)
return value === NullValue ? null : value
} catch (error) {
// using in-memory store is perf optimization not requirement
// trying to use optimization should NOT cause crashes
// so we just record warning and return undefined
recordWarning(new Error('Failed to get value from memory cache', { cause: error }))
}
},
set(key, value) {
if (!requestContext) return
inMemoryLRUCache?.set(`${requestContext?.requestID}:${key}`, value ?? NullValue)
try {
inMemoryLRUCache?.set(`${requestContext?.requestID}:${key}`, value ?? NullValue)
} catch (error) {
// using in-memory store is perf optimization not requirement
// trying to use optimization should NOT cause crashes
// so we just record warning and return undefined
recordWarning(new Error('Failed to store value in memory cache', { cause: error }))
}
},
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
export async function GET() {
return new Response('')
}

export const dynamic = 'force-static'
11 changes: 11 additions & 0 deletions tests/integration/cache-handler.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -367,6 +367,7 @@ describe('plugin', () => {
'/api/revalidate-handler',
'/api/static/first',
'/api/static/second',
'/api/zero-length-response',
'/index',
'/product/事前レンダリング,test',
'/revalidate-fetch',
Expand Down Expand Up @@ -508,4 +509,14 @@ describe('route', () => {

expect(call2.body).toBe('{"params":{"slug":"not-in-generateStaticParams"}}')
})

test<FixtureTestContext>('cacheable route handler response with 0 length response is served correctly', async (ctx) => {
await createFixture('server-components', ctx)
await runPlugin(ctx)

const call = await invokeFunction(ctx, { url: '/api/zero-length-response' })

expect(call.statusCode).toBe(200)
expect(call.body).toBe('')
})
})
Loading