fix: don't set SWR cdn cache control on stale responses (#259)

* chore: make header helper function name consistent with others

* fix: don't set SWR cdn cache control on stale responses

* fix: use must-revalidate for cdn when nextjs returns stale

* test: update test assertions

---------

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

Author: pieh

src/build/content/prerendered.ts

@@ -74,7 +74,7 @@ export const copyPrerenderedContent = async (ctx: PluginContext): Promise<void>
 
     await Promise.all(
       Object.entries(manifest.routes).map(async ([route, meta]): Promise<void> => {
-        const lastModified = meta.initialRevalidateSeconds ? 1 : Date.now()
+        const lastModified = meta.initialRevalidateSeconds ? Date.now() - 31536000000 : Date.now()
         const key = routeToFilePath(route)
         let value: IncrementalCacheValue
         switch (true) {
@@ -124,7 +124,7 @@ export const copyFetchContent = async (ctx: PluginContext): Promise<void> => {
 
     await Promise.all(
       paths.map(async (key): Promise<void> => {
-        const lastModified = 1
+        const lastModified = Date.now() - 31536000000
         const path = join(ctx.publishDir, 'cache/fetch-cache', key)
         const value = await buildFetchCacheValue(path)
         await writeCacheEntry(key, value, lastModified, ctx)

src/run/handlers/server.ts

@@ -6,7 +6,7 @@ import type { WorkerRequestHandler } from 'next/dist/server/lib/types.js'
 import { TagsManifest, getTagsManifest } from '../config.js'
 import {
   adjustDateHeader,
-  handleNextCacheHeader,
+  setCacheStatusHeader,
   setCacheControlHeaders,
   setCacheTagsHeaders,
   setVaryHeaders,
@@ -85,7 +85,7 @@ export default async (request: Request) => {
     setCacheControlHeaders(response.headers, request)
     setCacheTagsHeaders(response.headers, request, tagsManifest)
     setVaryHeaders(response.headers, request, nextConfig)
-    handleNextCacheHeader(response.headers)
+    setCacheStatusHeader(response.headers)
 
     // Temporary workaround for an issue where sending a response with an empty
     // body causes an unhandled error. This doesn't catch everything, but redirects are the

src/run/headers.ts

@@ -163,19 +163,6 @@ export const adjustDateHeader = async ({
     return
   }
 
-  if (lastModified === 1) {
-    // epoch 1 time seems to not work well with the CDN
-    // it causes to produce "Thu, 01 Jan 1970 00:00:00 GMT" date
-    // header correctly but CDN calculates then AGE to be 0
-    // causing response to be cached for entire max-age period
-    // since the first hit
-    // so instead of using 1 we use 1 year ago
-    // this is done here instead at build time as it's unclear
-    // what exactly is edge case in CDN and setting 1 year ago
-    // from request time seems to work consistently
-    lastModified = Date.now() - 31536000000
-  }
-
   const lastModifiedDate = new Date(lastModified)
   // Show actual date of the function call in the date header
   headers.set('x-nextjs-date', headers.get('date') ?? lastModifiedDate.toUTCString())
@@ -195,16 +182,20 @@ export const setCacheControlHeaders = (headers: Headers, request: Request) => {
     !headers.has('cdn-cache-control') &&
     !headers.has('netlify-cdn-cache-control')
   ) {
-    const privateCacheControl = omitHeaderValues(cacheControl, [
+    const browserCacheControl = omitHeaderValues(cacheControl, [
       's-maxage',
       'stale-while-revalidate',
     ])
-    const sharedCacheControl = mapHeaderValues(cacheControl, (value) =>
-      value === 'stale-while-revalidate' ? 'stale-while-revalidate=31536000' : value,
-    )
-
-    headers.set('cache-control', privateCacheControl || 'public, max-age=0, must-revalidate')
-    headers.set('netlify-cdn-cache-control', sharedCacheControl)
+    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'
+        : mapHeaderValues(cacheControl, (value) =>
+            value === 'stale-while-revalidate' ? 'stale-while-revalidate=31536000' : value,
+          )
+
+    headers.set('cache-control', browserCacheControl || 'public, max-age=0, must-revalidate')
+    headers.set('netlify-cdn-cache-control', cdnCacheControl)
   }
 }
 
@@ -232,7 +223,7 @@ const NEXT_CACHE_TO_CACHE_STATUS: Record<string, string> = {
  * a Cache-Status header for Next cache so users inspect that together with CDN cache status
  * and not on its own.
  */
-export const handleNextCacheHeader = (headers: Headers) => {
+export const setCacheStatusHeader = (headers: Headers) => {
   const nextCache = headers.get('x-nextjs-cache')
   if (typeof nextCache === 'string') {
     if (nextCache in NEXT_CACHE_TO_CACHE_STATUS) {

tests/integration/cache-handler.test.ts

@@ -61,7 +61,7 @@ describe('page router', () => {
     expect(call1.headers, 'a stale page served with swr').toEqual(
       expect.objectContaining({
         'cache-status': '"Next.js"; hit; fwd=stale',
-        'netlify-cdn-cache-control': 's-maxage=5, stale-while-revalidate=31536000',
+        'netlify-cdn-cache-control': 'public, max-age=0, must-revalidate',
       }),
     )
 
@@ -92,7 +92,8 @@ describe('page router', () => {
       'the rendered date in regenerated page is newer than initial stale page',
     ).toBeGreaterThan(0)
     expect(
-      call2.headers['date'].toString().localeCompare(call1.headers['date'].toString()),
+      new Date(call2.headers['date'].toString()).getTime() -
+        new Date(call1.headers['date'].toString()).getTime(),
       'the date header of regenerated page is newer than initial stale page',
     ).toBeGreaterThan(0)
 
@@ -171,7 +172,8 @@ describe('page router', () => {
     const call4Date = load(call4.body)('[data-testid="date-now"]').text()
     expect(call4Date, 'the date was not cached').not.toBe(call3Date)
     expect(
-      call4.headers['date'].toString().localeCompare(call3.headers['date'].toString()),
+      new Date(call4.headers['date'].toString()).getTime() -
+        new Date(call3.headers['date'].toString()).getTime(),
       'the date header of regenerated page is newer than initial stale page',
     ).toBeGreaterThan(0)
     expect(call4.statusCode).toBe(200)
@@ -217,7 +219,7 @@ describe('app router', () => {
       // It will be stale instead of hit
       expect.objectContaining({
         'cache-status': '"Next.js"; hit; fwd=stale',
-        'netlify-cdn-cache-control': 's-maxage=5, stale-while-revalidate=31536000',
+        'netlify-cdn-cache-control': 'public, max-age=0, must-revalidate',
       }),
     )
     expect(
@@ -284,7 +286,8 @@ describe('app router', () => {
     expect(cached.statusCode).toBe(200)
     expect(cachedDate, 'the date is not stale').not.toBe(staleDate)
     expect(
-      cached.headers['date'].toString().localeCompare(post1.headers['date'].toString()),
+      new Date(cached.headers['date'].toString()).getTime() -
+        new Date(post1.headers['date'].toString()).getTime(),
       'the date header of regenerated page is newer than initial stale page',
     ).toBeGreaterThan(0)
     expect(cached.headers, 'a cache hit after dynamically regenerating the stale page').toEqual(
@@ -378,7 +381,8 @@ describe('route', () => {
     )
     expect(call2Time.localeCompare(call1Time), 'the rendered date is new').toBeGreaterThan(0)
     expect(
-      call2.headers['date'].toString().localeCompare(call1.headers['date'].toString()),
+      new Date(call2.headers['date'].toString()).getTime() -
+        new Date(call1.headers['date'].toString()).getTime(),
       'the date header of regenerated route is newer than initial stale route',
     ).toBeGreaterThan(0)
     expect(
@@ -429,7 +433,8 @@ describe('route', () => {
     )
     expect(call4Time.localeCompare(call3Time), 'the rendered date is new').toBeGreaterThan(0)
     expect(
-      call4.headers['date'].toString().localeCompare(call3.headers['date'].toString()),
+      new Date(call4.headers['date'].toString()).getTime() -
+        new Date(call3.headers['date'].toString()).getTime(),
       'the date header of regenerated route is newer than previously stale route',
     ).toBeGreaterThan(0)
     expect(

tests/integration/fetch-handler.test.ts

@@ -213,7 +213,7 @@ test<FixtureTestContext>('if the fetch call is cached correctly (cached page res
   expect(post1.headers, 'a stale page served with swr').toEqual(
     expect.objectContaining({
       'cache-status': '"Next.js"; hit; fwd=stale',
-      'netlify-cdn-cache-control': 's-maxage=5, stale-while-revalidate=31536000',
+      'netlify-cdn-cache-control': 'public, max-age=0, must-revalidate',
     }),
   )
 
@@ -290,7 +290,7 @@ test<FixtureTestContext>('if the fetch call is cached correctly (cached page res
   expect(post3.headers, 'a stale page served with swr').toEqual(
     expect.objectContaining({
       'cache-status': '"Next.js"; hit; fwd=stale',
-      'netlify-cdn-cache-control': 's-maxage=5, stale-while-revalidate=31536000',
+      'netlify-cdn-cache-control': 'public, max-age=0, must-revalidate',
     }),
   )