From 942e45a720a089813b5d295ad0d920666511807f Mon Sep 17 00:00:00 2001 From: JJ Kasper Date: Mon, 10 Jun 2024 12:13:05 -0700 Subject: [PATCH] Fix inconsistency with 404 getStaticProps cache-control (#66674) While investigating unexpected stale responses when leveraging `getStaticProps` on the 404 page noticed we have an inconsistency between local and deployed due to `Cache-Control` being set to `no-store, must-revalidate` even though a revalidate period is provided. The inconsistency also differs between the HTML response and the data response `_next/data` which causes even more unexpected behavior. To avoid this behavior, this replaces the handling to ensure we honor the originally provided revalidate period during `notFound: true` for the `Cache-Control` header. Validated against provided reproduction here https://github.com/fusdev0/next-notfound-revalidate Deployment: https://vercel.live/link/next-notfound-revalidate-govzskknf-vtest314-ijjk-testing.vercel.app/fallback-blocking/fasdf Prior PR for prior context that introduced this https://github.com/vercel/next.js/pull/19165 x-ref: [slack thread](https://vercel.slack.com/archives/C0676QZBWKS/p1717492459342109) # Conflicts: # packages/next/src/server/base-server.ts --- packages/next/src/server/base-server.ts | 44 +++++++++++++------ packages/next/src/server/request-meta.ts | 5 +++ .../not-found-revalidate/pages/404.js | 2 +- .../not-found-revalidate/test/index.test.js | 12 ++--- 4 files changed, 43 insertions(+), 20 deletions(-) diff --git a/packages/next/src/server/base-server.ts b/packages/next/src/server/base-server.ts index 16d03b945da12..d4c806dddabdd 100644 --- a/packages/next/src/server/base-server.ts +++ b/packages/next/src/server/base-server.ts @@ -852,8 +852,8 @@ export default abstract class Server { ...(typeof val === 'string' ? [val] : Array.isArray(val) - ? val - : []), + ? val + : []), ]), ] } @@ -904,8 +904,8 @@ export default abstract class Server { req.headers['x-forwarded-port'] ??= this.port ? this.port.toString() : isHttps - ? '443' - : '80' + ? '443' + : '80' req.headers['x-forwarded-proto'] ??= isHttps ? 'https' : 'http' req.headers['x-forwarded-for'] ??= originalRequest.socket?.remoteAddress @@ -1674,8 +1674,8 @@ export default abstract class Server { typeof fallbackField === 'string' ? 'static' : fallbackField === null - ? 'blocking' - : fallbackField, + ? 'blocking' + : fallbackField, } } @@ -2647,10 +2647,10 @@ export default abstract class Server { isOnDemandRevalidate ? 'REVALIDATED' : cacheEntry.isMiss - ? 'MISS' - : cacheEntry.isStale - ? 'STALE' - : 'HIT' + ? 'MISS' + : cacheEntry.isStale + ? 'STALE' + : 'HIT' ) } @@ -2684,9 +2684,8 @@ export default abstract class Server { typeof cacheEntry.revalidate !== 'undefined' && (!this.renderOpts.dev || (hasServerProps && !isDataReq)) ) { - // If this is a preview mode request, we shouldn't cache it. We also don't - // cache 404 pages. - if (isPreviewMode || (is404Page && !isDataReq)) { + // If this is a preview mode request, we shouldn't cache it + if (isPreviewMode) { revalidate = 0 } @@ -2698,6 +2697,18 @@ export default abstract class Server { } } + // If we are rendering the 404 page we derive the cache-control + // revalidate period from the value that trigged the not found + // to be rendered. So if `getStaticProps` returns + // { notFound: true, revalidate 60 } the revalidate period should + // be 60 but if a static asset 404s directly it should have a revalidate + // period of 0 so that it doesn't get cached unexpectedly by a CDN + else if (is404Page) { + const notFoundRevalidate = getRequestMeta(req, 'notFoundRevalidate') + revalidate = + typeof notFoundRevalidate === 'undefined' ? 0 : notFoundRevalidate + } + // If the cache entry has a revalidate value that's a number, use it. else if (typeof cacheEntry.revalidate === 'number') { if (cacheEntry.revalidate < 1) { @@ -2731,6 +2742,12 @@ export default abstract class Server { } if (!cachedData) { + // add revalidate metadata before rendering 404 page + // so that we can use this as source of truth for the + // cache-control header instead of what the 404 page returns + // for the revalidate value + addRequestMeta(req, 'notFoundRevalidate', cacheEntry.revalidate) + if (cacheEntry.revalidate) { res.setHeader( 'Cache-Control', @@ -2749,7 +2766,6 @@ export default abstract class Server { if (this.renderOpts.dev) { query.__nextNotFoundSrcPage = pathname } - await this.render404(req, res, { pathname, query }, false) return null } else if (cachedData.kind === 'REDIRECT') { diff --git a/packages/next/src/server/request-meta.ts b/packages/next/src/server/request-meta.ts index 209f65c925afa..7a25f8b304549 100644 --- a/packages/next/src/server/request-meta.ts +++ b/packages/next/src/server/request-meta.ts @@ -94,6 +94,11 @@ export interface RequestMeta { cacheEntry: any, requestMeta: any ) => Promise | boolean | void + + /** + * The previous revalidate before rendering 404 page for notFound: true + */ + notFoundRevalidate?: number | false } /** diff --git a/test/integration/not-found-revalidate/pages/404.js b/test/integration/not-found-revalidate/pages/404.js index cb670e86b596c..6c4da3942ca80 100644 --- a/test/integration/not-found-revalidate/pages/404.js +++ b/test/integration/not-found-revalidate/pages/404.js @@ -14,6 +14,6 @@ export const getStaticProps = () => { notFound: true, random: Math.random(), }, - revalidate: 1, + revalidate: 6000, } } diff --git a/test/integration/not-found-revalidate/test/index.test.js b/test/integration/not-found-revalidate/test/index.test.js index 2f2928961429b..98ff9ea8be548 100644 --- a/test/integration/not-found-revalidate/test/index.test.js +++ b/test/integration/not-found-revalidate/test/index.test.js @@ -81,9 +81,9 @@ const runTests = () => { let res = await fetchViaHTTP(appPort, '/fallback-blocking/hello') let $ = cheerio.load(await res.text()) - const privateCache = - 'private, no-cache, no-store, max-age=0, must-revalidate' - expect(res.headers.get('cache-control')).toBe(privateCache) + expect(res.headers.get('cache-control')).toBe( + `s-maxage=1, stale-while-revalidate` + ) expect(res.status).toBe(404) expect(JSON.parse($('#props').text()).notFound).toBe(true) @@ -91,7 +91,9 @@ const runTests = () => { res = await fetchViaHTTP(appPort, '/fallback-blocking/hello') $ = cheerio.load(await res.text()) - expect(res.headers.get('cache-control')).toBe(privateCache) + expect(res.headers.get('cache-control')).toBe( + `s-maxage=1, stale-while-revalidate` + ) expect(res.status).toBe(404) expect(JSON.parse($('#props').text()).notFound).toBe(true) @@ -146,7 +148,7 @@ const runTests = () => { let $ = cheerio.load(await res.text()) expect(res.headers.get('cache-control')).toBe( - 'private, no-cache, no-store, max-age=0, must-revalidate' + `s-maxage=1, stale-while-revalidate` ) expect(res.status).toBe(404) expect(JSON.parse($('#props').text()).notFound).toBe(true)