fix: apply caching headers to pages router 404 with getStaticProps (#2764)

* fix: apply caching headers to pages router 404 with getStaticProps

* chore: don't store revalidate on request context for app router pages to limit potentially unwanted impact of the change

* chore: don't apply new 404 caching header code path, if existing method would have set cacheable header

* test: add notFound cases for 404 with getStaticProps without revalidate

* test: add fixture for 404 with getStaticProps with revalidate

* tmp: outline 404 integration test cases for page router

* test: update 404 caching tests

* fix: relax the conditions for caching 404s

* test: remove only

* feat: hide 404 caching behind env var

* test: remove only and console logs

* test: update static 404 test

* feat: reduce 404 caching scope to just the 404 page itself

* test: remove only modifier

* test: fix tests for new custom 404 pate

---------

Co-authored-by: Rob Stanford <me@robstanford.com>

Author: pieh

src/build/content/prerendered.ts

@@ -57,6 +57,7 @@ const routeToFilePath = (path: string) => {
 
 const buildPagesCacheValue = async (
   path: string,
+  initialRevalidateSeconds: number | false | undefined,
   shouldUseEnumKind: boolean,
   shouldSkipJson = false,
 ): Promise<NetlifyCachedPageValue> => ({
@@ -65,6 +66,7 @@ const buildPagesCacheValue = async (
   pageData: shouldSkipJson ? {} : JSON.parse(await readFile(`${path}.json`, 'utf-8')),
   headers: undefined,
   status: undefined,
+  revalidate: initialRevalidateSeconds,
 })
 
 const buildAppCacheValue = async (
@@ -178,6 +180,7 @@ export const copyPrerenderedContent = async (ctx: PluginContext): Promise<void>
                   }
                   value = await buildPagesCacheValue(
                     join(ctx.publishDir, 'server/pages', key),
+                    meta.initialRevalidateSeconds,
                     shouldUseEnumKind,
                   )
                   break
@@ -210,6 +213,7 @@ export const copyPrerenderedContent = async (ctx: PluginContext): Promise<void>
           const key = routeToFilePath(route)
           const value = await buildPagesCacheValue(
             join(ctx.publishDir, 'server/pages', key),
+            undefined,
             shouldUseEnumKind,
             true, // there is no corresponding json file for fallback, so we are skipping it for this entry
           )

src/run/handlers/cache.cts

@@ -308,6 +308,11 @@ export class NetlifyCacheHandler implements CacheHandlerForMultipleVersions {
         case 'PAGES': {
           const { revalidate, ...restOfPageValue } = blob.value
 
+          const requestContext = getRequestContext()
+          if (requestContext) {
+            requestContext.pageHandlerRevalidate = revalidate
+          }
+
           span.addEvent(blob.value?.kind, { lastModified: blob.lastModified, revalidate, ttl })
 
           await this.injectEntryToPrerenderManifest(key, revalidate)

src/run/handlers/request-context.cts

@@ -25,6 +25,7 @@ export type RequestContext = {
   didPagesRouterOnDemandRevalidate?: boolean
   serverTiming?: string
   routeHandlerRevalidate?: NetlifyCachedRouteValue['revalidate']
+  pageHandlerRevalidate?: NetlifyCachedRouteValue['revalidate']
   /**
    * Track promise running in the background and need to be waited for.
    * Uses `context.waitUntil` if available, otherwise stores promises to

src/run/headers.ts

@@ -2,6 +2,7 @@ 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 { getLogger, RequestContext } from './handlers/request-context.cjs'
 import type { RuntimeTracer } from './handlers/tracer.cjs'
@@ -197,6 +198,19 @@ export const adjustDateHeader = async ({
   headers.set('date', lastModifiedDate.toUTCString())
 }
 
+function setCacheControlFromRequestContext(
+  headers: Headers,
+  revalidate: NetlifyCachedRouteValue['revalidate'],
+) {
+  const cdnCacheControl =
+    // if we are serving already stale response, instruct edge to not attempt to cache that response
+    headers.get('x-nextjs-cache') === 'STALE'
+      ? 'public, max-age=0, must-revalidate, durable'
+      : `s-maxage=${revalidate || 31536000}, stale-while-revalidate=31536000, durable`
+
+  headers.set('netlify-cdn-cache-control', cdnCacheControl)
+}
+
 /**
  * Ensure stale-while-revalidate and s-maxage don't leak to the client, but
  * assume the user knows what they are doing if CDN cache controls are set
@@ -214,13 +228,7 @@ export const setCacheControlHeaders = (
     !headers.has('netlify-cdn-cache-control')
   ) {
     // handle CDN Cache Control on Route Handler responses
-    const cdnCacheControl =
-      // if we are serving already stale response, instruct edge to not attempt to cache that response
-      headers.get('x-nextjs-cache') === 'STALE'
-        ? 'public, max-age=0, must-revalidate, durable'
-        : `s-maxage=${requestContext.routeHandlerRevalidate === false ? 31536000 : requestContext.routeHandlerRevalidate}, stale-while-revalidate=31536000, durable`
-
-    headers.set('netlify-cdn-cache-control', cdnCacheControl)
+    setCacheControlFromRequestContext(headers, requestContext.routeHandlerRevalidate)
     return
   }
 
@@ -231,14 +239,27 @@ export const setCacheControlHeaders = (
       .log('NetlifyHeadersHandler.trailingSlashRedirect')
   }
 
-  if (status === 404 && request.url.endsWith('.php')) {
-    // temporary CDN Cache Control handling for bot probes on PHP files
-    // https://linear.app/netlify/issue/FRB-1344/prevent-excessive-ssr-invocations-due-to-404-routes
-    headers.set('cache-control', 'public, max-age=0, must-revalidate')
-    headers.set('netlify-cdn-cache-control', `max-age=31536000, durable`)
+  const cacheControl = headers.get('cache-control')
+  if (status === 404) {
+    if (request.url.endsWith('.php')) {
+      // temporary CDN Cache Control handling for bot probes on PHP files
+      // https://linear.app/netlify/issue/FRB-1344/prevent-excessive-ssr-invocations-due-to-404-routes
+      headers.set('cache-control', 'public, max-age=0, must-revalidate')
+      headers.set('netlify-cdn-cache-control', `max-age=31536000, durable`)
+      return
+    }
+
+    if (
+      process.env.CACHE_404_PAGE &&
+      request.url.endsWith('/404') &&
+      ['GET', 'HEAD'].includes(request.method)
+    ) {
+      // handle CDN Cache Control on 404 Page responses
+      setCacheControlFromRequestContext(headers, requestContext.pageHandlerRevalidate)
+      return
+    }
   }
 
-  const cacheControl = headers.get('cache-control')
   if (
     cacheControl !== null &&
     ['GET', 'HEAD'].includes(request.method) &&

tests/e2e/page-router.test.ts

@@ -1,6 +1,6 @@
 import { expect } from '@playwright/test'
-import { test } from '../utils/playwright-helpers.js'
 import { nextVersionSatisfies } from '../utils/next-version-helpers.mjs'
+import { test } from '../utils/playwright-helpers.js'
 
 export function waitFor(millis: number) {
   return new Promise((resolve) => setTimeout(resolve, millis))
@@ -462,7 +462,7 @@ test.describe('Simple Page Router (no basePath, no i18n)', () => {
     const headers = response?.headers() || {}
     expect(response?.status()).toBe(404)
 
-    expect(await page.textContent('h1')).toBe('404')
+    expect(await page.textContent('p')).toBe('Custom 404 page')
 
     // https://github.com/vercel/next.js/pull/69802 made changes to returned cache-control header,
     // after that (14.2.10 and canary.147) 404 pages would have `private` directive, before that
@@ -486,7 +486,7 @@ test.describe('Simple Page Router (no basePath, no i18n)', () => {
     const headers = response?.headers() || {}
     expect(response?.status()).toBe(404)
 
-    expect(await page.textContent('h1')).toBe('404')
+    expect(await page.textContent('p')).toBe('Custom 404 page')
 
     expect(headers['netlify-cdn-cache-control']).toBe(
       nextVersionSatisfies('>=15.0.0-canary.187')
@@ -1326,7 +1326,7 @@ test.describe('Page Router with basePath and i18n', () => {
     const headers = response?.headers() || {}
     expect(response?.status()).toBe(404)
 
-    expect(await page.textContent('h1')).toBe('404')
+    expect(await page.textContent('p')).toBe('Custom 404 page')
 
     // https://github.com/vercel/next.js/pull/69802 made changes to returned cache-control header,
     // after that 404 pages would have `private` directive, before that it would not
@@ -1349,7 +1349,7 @@ test.describe('Page Router with basePath and i18n', () => {
     const headers = response?.headers() || {}
     expect(response?.status()).toBe(404)
 
-    expect(await page.textContent('h1')).toBe('404')
+    expect(await page.textContent('p')).toBe('Custom 404 page')
 
     expect(headers['netlify-cdn-cache-control']).toBe(
       nextVersionSatisfies('>=15.0.0-canary.187')

tests/fixtures/page-router-404-get-static-props-with-revalidate/next.config.js

@@ -0,0 +1,10 @@
+/** @type {import('next').NextConfig} */
+const nextConfig = {
+  output: 'standalone',
+  eslint: {
+    ignoreDuringBuilds: true,
+  },
+  generateBuildId: () => 'build-id',
+}
+
+module.exports = nextConfig

tests/fixtures/page-router-404-get-static-props-with-revalidate/package.json

@@ -0,0 +1,15 @@
+{
+  "name": "page-router-404-get-static-props-with-revalidate",
+  "version": "0.1.0",
+  "private": true,
+  "scripts": {
+    "postinstall": "next build",
+    "dev": "next dev",
+    "build": "next build"
+  },
+  "dependencies": {
+    "next": "latest",
+    "react": "18.2.0",
+    "react-dom": "18.2.0"
+  }
+}

tests/fixtures/page-router-404-get-static-props-with-revalidate/pages/404.js

@@ -0,0 +1,17 @@
+export default function NotFound({ timestamp }) {
+  return (
+    <p>
+      Custom 404 page with revalidate: <pre data-testid="timestamp">{timestamp}</pre>
+    </p>
+  )
+}
+
+/** @type {import('next').GetStaticProps} */
+export const getStaticProps = ({ locale }) => {
+  return {
+    props: {
+      timestamp: Date.now(),
+    },
+    revalidate: 300,
+  }
+}

tests/fixtures/page-router-404-get-static-props-with-revalidate/pages/products/[slug].js

@@ -0,0 +1,40 @@
+const Product = ({ time, slug }) => (
+  <div>
+    <h1>Product {slug}</h1>
+    <p>
+      This page uses getStaticProps() and getStaticPaths() to pre-fetch a Product
+      <span data-testid="date-now">{time}</span>
+    </p>
+  </div>
+)
+
+/** @type {import('next').GetStaticProps} */
+export async function getStaticProps({ params }) {
+  if (params.slug === 'not-found-no-revalidate') {
+    return {
+      notFound: true,
+    }
+  } else if (params.slug === 'not-found-with-revalidate') {
+    return {
+      notFound: true,
+      revalidate: 600,
+    }
+  }
+
+  return {
+    props: {
+      time: new Date().toISOString(),
+      slug: params.slug,
+    },
+  }
+}
+
+/** @type {import('next').GetStaticPaths} */
+export const getStaticPaths = () => {
+  return {
+    paths: [],
+    fallback: 'blocking', // false or "blocking"
+  }
+}
+
+export default Product

tests/fixtures/page-router-base-path-i18n/pages/products/[slug].js

@@ -8,7 +8,19 @@ const Product = ({ time, slug }) => (
   </div>
 )
 
+/** @type {import('next').GetStaticProps} */
 export async function getStaticProps({ params }) {
+  if (params.slug === 'not-found-no-revalidate') {
+    return {
+      notFound: true,
+    }
+  } else if (params.slug === 'not-found-with-revalidate') {
+    return {
+      notFound: true,
+      revalidate: 600,
+    }
+  }
+
   return {
     props: {
       time: new Date().toISOString(),

tests/fixtures/page-router/pages/404.js

@@ -0,0 +1,3 @@
+export default function NotFound() {
+  return <p>Custom 404 page</p>
+}

tests/fixtures/page-router/pages/products/[slug].js

@@ -9,6 +9,13 @@ const Product = ({ time, slug }) => (
 )
 
 export async function getStaticProps({ params }) {
+  if (params.slug === 'not-found-with-revalidate') {
+    return {
+      notFound: true,
+      revalidate: 600,
+    }
+  }
+
   return {
     props: {
       time: new Date().toISOString(),

tests/integration/page-router.test.ts

@@ -4,7 +4,7 @@ import { HttpResponse, http, passthrough } from 'msw'
 import { setupServer } from 'msw/node'
 import { platform } from 'node:process'
 import { v4 } from 'uuid'
-import { afterAll, afterEach, beforeAll, beforeEach, expect, test, vi } from 'vitest'
+import { afterAll, afterEach, beforeAll, beforeEach, describe, expect, test, vi } from 'vitest'
 import { type FixtureTestContext } from '../utils/contexts.js'
 import { createFixture, invokeFunction, runPlugin } from '../utils/fixture.js'
 import { encodeBlobKey, generateRandomObjectID, startMockBlobStore } from '../utils/helpers.js'
@@ -19,7 +19,6 @@ beforeAll(() => {
   // and passthrough everything else
   server = setupServer(
     http.post('https://api.netlify.com/api/v1/purge', () => {
-      console.log('intercepted purge api call')
       return HttpResponse.json({})
     }),
     http.all(/.*/, () => passthrough()),
@@ -91,7 +90,6 @@ test<FixtureTestContext>('Should revalidate path with On-demand Revalidation', a
   expect(staticPageRevalidated.headers?.['cache-status']).toMatch(/"Next.js"; hit/)
   const dateCacheRevalidated = load(staticPageRevalidated.body)('[data-testid="date-now"]').text()
 
-  console.log({ dateCacheInitial, dateCacheRevalidated })
   expect(dateCacheInitial).not.toBe(dateCacheRevalidated)
 })
 
@@ -153,3 +151,189 @@ test<FixtureTestContext>('Should serve correct locale-aware custom 404 pages', a
     'Served 404 page content should use non-default locale if non-default locale is explicitly used in pathname (after basePath)',
   ).toBe('fr')
 })
+
+// These tests describe how the 404 caching should work, but unfortunately it doesn't work like
+// this in v5 and a fix would represent a breaking change so we are skipping them for now, but
+// leaving them here for future reference when considering the next major version
+describe.skip('404 caching', () => {
+  describe('404 without getStaticProps', () => {
+    test<FixtureTestContext>('not matching dynamic paths should be cached permanently', async (ctx) => {
+      await createFixture('page-router', ctx)
+      await runPlugin(ctx)
+
+      const notExistingPage = await invokeFunction(ctx, {
+        url: '/not-existing-page',
+      })
+
+      expect(notExistingPage.statusCode).toBe(404)
+
+      expect(
+        notExistingPage.headers['netlify-cdn-cache-control'],
+        'should be cached permanently',
+      ).toBe('s-maxage=31536000, stale-while-revalidate=31536000, durable')
+    })
+    test<FixtureTestContext>('matching dynamic path with revalidate should be cached for revalidate time', async (ctx) => {
+      await createFixture('page-router', ctx)
+      await runPlugin(ctx)
+
+      const notExistingPage = await invokeFunction(ctx, {
+        url: '/products/not-found-with-revalidate',
+      })
+
+      expect(notExistingPage.statusCode).toBe(404)
+
+      expect(
+        notExistingPage.headers['netlify-cdn-cache-control'],
+        'should be cached for revalidate time',
+      ).toBe('s-maxage=600, stale-while-revalidate=31536000, durable')
+    })
+  })
+
+  describe('404 with getStaticProps without revalidate', () => {
+    test<FixtureTestContext>('not matching dynamic paths should be cached permanently', async (ctx) => {
+      await createFixture('page-router-base-path-i18n', ctx)
+      await runPlugin(ctx)
+
+      const notExistingPage = await invokeFunction(ctx, {
+        url: '/base/path/not-existing-page',
+      })
+
+      expect(notExistingPage.statusCode).toBe(404)
+
+      expect(
+        notExistingPage.headers['netlify-cdn-cache-control'],
+        'should be cached permanently',
+      ).toBe('s-maxage=31536000, stale-while-revalidate=31536000, durable')
+    })
+    test<FixtureTestContext>('matching dynamic path with revalidate should be cached for revalidate time', async (ctx) => {
+      await createFixture('page-router-base-path-i18n', ctx)
+      await runPlugin(ctx)
+
+      const notExistingPage = await invokeFunction(ctx, {
+        url: '/base/path/products/not-found-with-revalidate',
+      })
+
+      expect(notExistingPage.statusCode).toBe(404)
+
+      expect(
+        notExistingPage.headers['netlify-cdn-cache-control'],
+        'should be cached for revalidate time',
+      ).toBe('s-maxage=600, stale-while-revalidate=31536000, durable')
+    })
+  })
+
+  describe('404 with getStaticProps with revalidate', () => {
+    test<FixtureTestContext>('not matching dynamic paths should be cached for 404 page revalidate', async (ctx) => {
+      await createFixture('page-router-404-get-static-props-with-revalidate', ctx)
+      await runPlugin(ctx)
+
+      // ignoring initial stale case
+      await invokeFunction(ctx, {
+        url: 'not-existing-page',
+      })
+
+      await new Promise((res) => setTimeout(res, 100))
+
+      const notExistingPage = await invokeFunction(ctx, {
+        url: 'not-existing-page',
+      })
+
+      expect(notExistingPage.statusCode).toBe(404)
+
+      expect(
+        notExistingPage.headers['netlify-cdn-cache-control'],
+        'should be cached for 404 page revalidate',
+      ).toBe('s-maxage=300, stale-while-revalidate=31536000, durable')
+    })
+
+    test<FixtureTestContext>('matching dynamic path with revalidate should be cached for revalidate time', async (ctx) => {
+      await createFixture('page-router-404-get-static-props-with-revalidate', ctx)
+      await runPlugin(ctx)
+
+      // ignoring initial stale case
+      await invokeFunction(ctx, {
+        url: 'products/not-found-with-revalidate',
+      })
+      await new Promise((res) => setTimeout(res, 100))
+
+      const notExistingPage = await invokeFunction(ctx, {
+        url: 'products/not-found-with-revalidate',
+      })
+
+      expect(notExistingPage.statusCode).toBe(404)
+
+      expect(
+        notExistingPage.headers['netlify-cdn-cache-control'],
+        'should be cached for revalidate time',
+      ).toBe('s-maxage=600, stale-while-revalidate=31536000, durable')
+    })
+  })
+})
+
+// This is a temporary fix to ensure that the 404 page itself is cached correctly when requested
+// directly. This is a workaround for a specific customer and should be removed once the 404 caching
+// is fixed in the next major version.
+describe('404 page caching', () => {
+  beforeAll(() => {
+    process.env.CACHE_404_PAGE = 'true'
+  })
+
+  afterAll(() => {
+    delete process.env.CACHE_404_PAGE
+  })
+
+  test<FixtureTestContext>('404 without getStaticProps', async (ctx) => {
+    await createFixture('page-router', ctx)
+    await runPlugin(ctx)
+
+    const notExistingPage = await invokeFunction(ctx, {
+      url: '/404',
+    })
+
+    expect(notExistingPage.statusCode).toBe(404)
+
+    expect(
+      notExistingPage.headers['netlify-cdn-cache-control'],
+      'should be cached permanently',
+    ).toBe('s-maxage=31536000, stale-while-revalidate=31536000, durable')
+  })
+
+  test<FixtureTestContext>('404 with getStaticProps without revalidate', async (ctx) => {
+    await createFixture('page-router-base-path-i18n', ctx)
+    await runPlugin(ctx)
+
+    const notExistingPage = await invokeFunction(ctx, {
+      url: '/base/404',
+    })
+
+    expect(notExistingPage.statusCode).toBe(404)
+
+    expect(
+      notExistingPage.headers['netlify-cdn-cache-control'],
+      'should be cached permanently',
+    ).toBe('s-maxage=31536000, stale-while-revalidate=31536000, durable')
+  })
+
+  test<FixtureTestContext>('404 with getStaticProps with revalidate', async (ctx) => {
+    await createFixture('page-router-404-get-static-props-with-revalidate', ctx)
+    await runPlugin(ctx)
+
+    // ignoring initial stale case
+    await invokeFunction(ctx, {
+      url: '/404',
+    })
+
+    await new Promise((res) => setTimeout(res, 100))
+
+    const notExistingPage = await invokeFunction(ctx, {
+      url: '/404',
+    })
+
+    expect(notExistingPage.statusCode).toBe(404)
+
+    expect(
+      notExistingPage.headers['netlify-cdn-cache-control'],
+      'should be cached for 404 page revalidate',
+    ).toBe('s-maxage=300, stale-while-revalidate=31536000, durable')
+  })
+})

tests/integration/static.test.ts

@@ -54,7 +54,7 @@ test<FixtureTestContext>('requesting a non existing page route that needs to be
   // test that it should request the 404.html file
   const call1 = await invokeFunction(ctx, { url: 'static/revalidate-not-existing' })
   expect(call1.statusCode).toBe(404)
-  expect(load(call1.body)('h1').text()).toBe('404')
+  expect(load(call1.body)('p').text()).toBe('Custom 404 page')
 
   // https://github.com/vercel/next.js/pull/69802 made changes to returned cache-control header,
   // after that (14.2.10 and canary.147) 404 pages would have `private` directive, before that it