From e6566989c2c8bc6b804c0d1a1d9537a9bc20135c Mon Sep 17 00:00:00 2001 From: JJ Kasper Date: Wed, 21 Aug 2024 15:27:06 -0700 Subject: [PATCH] Ensure fetch cache TTL is updated properly (#69164) To reduce the number of set requests we were sending upstream we added an optimization to skip sending the set request if the cached data didn't change after a revalidation. The problem with this optimization is the upstream service relies on this set request to know to update the cache entries TTL and consider it up to date. This causes unexpected revalidations while the cache value hasn't changed and the TTL is kept stale. x-ref: NEXT-3693 --- .../lib/incremental-cache/fetch-cache.ts | 20 ------- test/ppr-tests-manifest.json | 3 +- .../fetch-cache/app/not-changed/page.tsx | 33 ++++++++++++ .../app-dir/fetch-cache/fetch-cache.test.ts | 54 ++++++++++++++++++- test/turbopack-build-tests-manifest.json | 3 +- 5 files changed, 90 insertions(+), 23 deletions(-) create mode 100644 test/production/app-dir/fetch-cache/app/not-changed/page.tsx diff --git a/packages/next/src/server/lib/incremental-cache/fetch-cache.ts b/packages/next/src/server/lib/incremental-cache/fetch-cache.ts index 538d628692382..599ad0c4707bb 100644 --- a/packages/next/src/server/lib/incremental-cache/fetch-cache.ts +++ b/packages/next/src/server/lib/incremental-cache/fetch-cache.ts @@ -337,26 +337,6 @@ export default class FetchCache implements CacheHandler { public async set(...args: Parameters) { const [key, data, ctx] = args - const newValue = - data?.kind === CachedRouteKind.FETCH ? data.data : undefined - const existingCache = memoryCache?.get(key) - const existingValue = existingCache?.value - if ( - existingValue?.kind === CachedRouteKind.FETCH && - Object.keys(existingValue.data).every( - (field) => - JSON.stringify( - (existingValue.data as Record)[field] - ) === - JSON.stringify((newValue as Record)[field]) - ) - ) { - if (DEBUG) { - console.log(`skipping cache set for ${key} as not modified`) - } - return - } - const { fetchCache, fetchIdx, fetchUrl, tags } = ctx if (!fetchCache) return diff --git a/test/ppr-tests-manifest.json b/test/ppr-tests-manifest.json index b8fab0c850b44..42af2577f803e 100644 --- a/test/ppr-tests-manifest.json +++ b/test/ppr-tests-manifest.json @@ -5,8 +5,9 @@ "passed": [], "failed": [ "fetch-cache should have correct fetchUrl field for fetches and unstable_cache", + "fetch-cache should not retry for failed fetch-cache GET", "fetch-cache should retry 3 times when revalidate times out", - "fetch-cache should not retry for failed fetch-cache GET" + "fetch-cache should update cache TTL even if cache data does not change" ], "pending": [], "flakey": [], diff --git a/test/production/app-dir/fetch-cache/app/not-changed/page.tsx b/test/production/app-dir/fetch-cache/app/not-changed/page.tsx new file mode 100644 index 0000000000000..36db672079671 --- /dev/null +++ b/test/production/app-dir/fetch-cache/app/not-changed/page.tsx @@ -0,0 +1,33 @@ +import { unstable_cache } from 'next/cache' + +export const dynamic = 'force-dynamic' +export const fetchCache = 'default-cache' + +// this is ensuring we still update TTL even +// if the cache value didn't change during revalidate +const stableCacheItem = unstable_cache( + async () => { + return 'consistent value' + }, + [], + { + revalidate: 3, + tags: ['thankyounext'], + } +) + +export default async function Page() { + const data = await fetch('https://example.vercel.sh', { + next: { tags: ['thankyounext'], revalidate: 3 }, + }).then((res) => res.text()) + + const cachedValue = stableCacheItem() + + return ( + <> +

hello world

+

{data}

+

{cachedValue}

+ + ) +} diff --git a/test/production/app-dir/fetch-cache/fetch-cache.test.ts b/test/production/app-dir/fetch-cache/fetch-cache.test.ts index 972e8ef280076..6290bab790d75 100644 --- a/test/production/app-dir/fetch-cache/fetch-cache.test.ts +++ b/test/production/app-dir/fetch-cache/fetch-cache.test.ts @@ -2,6 +2,7 @@ import glob from 'glob' import http from 'http' import fs from 'fs-extra' import { join } from 'path' +import rawBody from 'next/dist/compiled/raw-body' import { FileRef, NextInstance, createNext } from 'e2e-utils' import { retry, @@ -25,6 +26,8 @@ describe('fetch-cache', () => { method: string headers: Record }> = [] + let storeCacheItems = false + const fetchCacheStore = new Map() let fetchCacheEnv: Record = { SUSPENSE_CACHE_PROTO: 'http', } @@ -107,8 +110,9 @@ describe('fetch-cache', () => { fetchGetReqIndex = 0 revalidateReqIndex = 0 fetchCacheRequests = [] + storeCacheItems = false fetchGetShouldError = false - fetchCacheServer = http.createServer((req, res) => { + fetchCacheServer = http.createServer(async (req, res) => { console.log(`fetch cache request ${req.url} ${req.method}`, req.headers) const parsedUrl = new URL(req.url || '/', 'http://n') @@ -148,6 +152,19 @@ describe('fetch-cache', () => { res.end('internal server error') return } + + if (storeCacheItems && fetchCacheStore.has(key)) { + console.log(`returned cache for ${key}`) + res.statusCode = 200 + res.end(JSON.stringify(fetchCacheStore.get(key))) + return + } + } + + if (type === 'post' && storeCacheItems) { + const body = await rawBody(req, { encoding: 'utf8' }) + fetchCacheStore.set(key, JSON.parse(body.toString())) + console.log(`set cache for ${key}`) } res.statusCode = type === 'post' ? 200 : 404 res.end(`${type} for ${key}`) @@ -237,4 +254,39 @@ describe('fetch-cache', () => { fetchGetShouldError = false } }) + + it('should update cache TTL even if cache data does not change', async () => { + storeCacheItems = true + const fetchCacheRequestsIndex = fetchCacheRequests.length + + try { + for (let i = 0; i < 3; i++) { + const res = await fetchViaHTTP(appPort, '/not-changed') + expect(res.status).toBe(200) + // give time for revalidate period to pass + await new Promise((resolve) => setTimeout(resolve, 3_000)) + } + + const newCacheGets = [] + const newCacheSets = [] + + for ( + let i = fetchCacheRequestsIndex - 1; + i < fetchCacheRequests.length; + i++ + ) { + const requestItem = fetchCacheRequests[i] + if (requestItem.method === 'get') { + newCacheGets.push(requestItem) + } + if (requestItem.method === 'post') { + newCacheSets.push(requestItem) + } + } + expect(newCacheGets.length).toBeGreaterThanOrEqual(2) + expect(newCacheSets.length).toBeGreaterThanOrEqual(2) + } finally { + storeCacheItems = false + } + }) }) diff --git a/test/turbopack-build-tests-manifest.json b/test/turbopack-build-tests-manifest.json index 8397f06f26b60..662d7e2ebd622 100644 --- a/test/turbopack-build-tests-manifest.json +++ b/test/turbopack-build-tests-manifest.json @@ -15637,7 +15637,8 @@ "failed": [ "fetch-cache should have correct fetchUrl field for fetches and unstable_cache", "fetch-cache should not retry for failed fetch-cache GET", - "fetch-cache should retry 3 times when revalidate times out" + "fetch-cache should retry 3 times when revalidate times out", + "fetch-cache should update cache TTL even if cache data does not change" ], "pending": [], "flakey": [],