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
#19165

x-ref: [slack
thread](https://vercel.slack.com/archives/C0676QZBWKS/p1717492459342109)
# Conflicts:
#	packages/next/src/server/base-server.ts

Author: ijjk

packages/next/src/server/base-server.ts

@@ -852,8 +852,8 @@ export default abstract class Server<ServerOptions extends Options = Options> {
                 ...(typeof val === 'string'
                   ? [val]
                   : Array.isArray(val)
-                  ? val
-                  : []),
+                    ? val
+                    : []),
               ]),
             ]
           }
@@ -904,8 +904,8 @@ export default abstract class Server<ServerOptions extends Options = Options> {
       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<ServerOptions extends Options = Options> {
         typeof fallbackField === 'string'
           ? 'static'
           : fallbackField === null
-          ? 'blocking'
-          : fallbackField,
+            ? 'blocking'
+            : fallbackField,
     }
   }
 
@@ -2647,10 +2647,10 @@ export default abstract class Server<ServerOptions extends Options = Options> {
         isOnDemandRevalidate
           ? 'REVALIDATED'
           : cacheEntry.isMiss
-          ? 'MISS'
-          : cacheEntry.isStale
-          ? 'STALE'
-          : 'HIT'
+            ? 'MISS'
+            : cacheEntry.isStale
+              ? 'STALE'
+              : 'HIT'
       )
     }
 
@@ -2684,9 +2684,8 @@ export default abstract class Server<ServerOptions extends Options = Options> {
       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<ServerOptions extends Options = Options> {
         }
       }
 
+      // 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<ServerOptions extends Options = Options> {
     }
 
     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<ServerOptions extends Options = Options> {
       if (this.renderOpts.dev) {
         query.__nextNotFoundSrcPage = pathname
       }
-
       await this.render404(req, res, { pathname, query }, false)
       return null
     } else if (cachedData.kind === 'REDIRECT') {

packages/next/src/server/request-meta.ts

@@ -94,6 +94,11 @@ export interface RequestMeta {
     cacheEntry: any,
     requestMeta: any
   ) => Promise<boolean | void> | boolean | void
+
+  /**
+   * The previous revalidate before rendering 404 page for notFound: true
+   */
+  notFoundRevalidate?: number | false
 }
 
 /**

test/integration/not-found-revalidate/pages/404.js

@@ -14,6 +14,6 @@ export const getStaticProps = () => {
       notFound: true,
       random: Math.random(),
     },
-    revalidate: 1,
+    revalidate: 6000,
   }
 }

test/integration/not-found-revalidate/test/index.test.js

@@ -81,17 +81,19 @@ 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)
 
     await waitFor(1000)
     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)