fix: let next-server handle SWR behavior (#206)

* chore: add local repro of failing test

* fix: return stale values for cache entries

* fix: give blob a couple seconds to propagate back

* fix: ensure lambda doesn't die before the revalidate is done

* chore: turn test page into js to prevent it from installing typescript

* chore: remove .only

* fix: also adjust date header for stale cache serves

* fix: set cache-status header for SWR

* chore: update route handler test to capture SWR

* similar fixes for page router with static revalidate

* aaand another test

* fix: another test

* remove .only

* fix first test

* fix: route handlers are also cacheable!

* test: cache-handler test for page router updated for SWR behavior

* chore: remove no longer used CacheHandler helper method

* chore remove unused imports and variables used in previously removed method

* test: cache-handler test for route handler updated for SWR behavior

* test: invoke functions in integration tests in sandboxed child process

* test: move request mocking to lambda runner and mock just purge API

* Revert "test: move request mocking to lambda runner and mock just purge API"

This reverts commit c773508bd1fc1398711fe242e791123642cc5688.

* Revert "test: invoke functions in integration tests in sandboxed child process"

This reverts commit a834f622b3fd6829032c53654d3600ceeb9492e3.

* chore: update msw/node setup to get rid of bunch of warnings

* test: split cache-handler tests into 3 separate test suites, one for each fixture

* adjust sequencer to run all 3 cache-handler tests in same shard

* adjust tests

* upgrade vitest

* run test suites sequentially

* maybe fix lock file

* add delay to app router cachehandler tests

* test: update fetch-handdler integration tests

* Revert "add delay to app router cachehandler tests"

This reverts commit b4112c4a53bf2d34086ca349944dfe3a588abbdd.

* Revert "maybe fix lock file"

This reverts commit db186688fd3f035176176fd76b4b7710ec46436b.

* Revert "run test suites sequentially"

This reverts commit 548f81d222eb5729e963a494d6a5ebf09dae68e9.

* Revert "upgrade vitest"

This reverts commit d65ed1a64f16dca2e328fd2fc2d9b79b9ae48448.

* Revert "adjust tests"

This reverts commit bc620eee8455336e98feeaea83dda5dbe250ddaf.

* Revert "adjust sequencer to run all 3 cache-handler tests in same shard"

This reverts commit 23f65d3797c69bff58fa24fd6b3ffd9430a31099.

* Revert "test: split cache-handler tests into 3 separate test suites, one for each fixture"

This reverts commit 6cb1422e5883964a7cf4bfd04b76fb0dbbb902d9.

* test: readd delay after reverting setup changes

* test: add some debug logs

* Revert "test: add some debug logs"

This reverts commit 35e92a5b3b0198de9a951aa97f7543839a72a62c.

* fix: fix bad merge conflict resolution

---------

Co-authored-by: Matt Kane <[email protected]>
Co-authored-by: Michal Piechowiak <[email protected]>

Author: 3 people

src/run/handlers/cache.cts

@@ -2,13 +2,10 @@
 // (CJS format because Next.js doesn't support ESM yet)
 //
 import { Buffer } from 'node:buffer'
-import { readFileSync } from 'node:fs'
-import { join } from 'node:path/posix'
 
 import { getDeployStore, Store } from '@netlify/blobs'
 import { purgeCache } from '@netlify/functions'
 import { trace } from '@opentelemetry/api'
-import type { PrerenderManifest } from 'next/dist/build/index.js'
 import { NEXT_CACHE_TAGS_HEADER } from 'next/dist/lib/constants.js'
 import type {
   CacheHandler,
@@ -18,16 +15,6 @@ import type {
 
 type TagManifest = { revalidatedAt: number }
 
-// load the prerender manifest
-const prerenderManifest: PrerenderManifest = JSON.parse(
-  readFileSync(join(process.cwd(), '.next/prerender-manifest.json'), 'utf-8'),
-)
-
-/** Strips trailing slashes and normalizes the index root */
-function toRoute(cacheKey: string): string {
-  return cacheKey.replace(/\/$/, '').replace(/\/index$/, '') || '/'
-}
-
 const fetchBeforeNextPatchedIt = globalThis.fetch
 
 export class NetlifyCacheHandler implements CacheHandler {
@@ -65,12 +52,10 @@ export class NetlifyCacheHandler implements CacheHandler {
         return null
       }
 
-      const revalidateAfter = this.calculateRevalidate(key, blob.lastModified, ctx)
-      const isStale = revalidateAfter !== false && revalidateAfter < Date.now()
       const staleByTags = await this.checkCacheEntryStaleByTags(blob, ctx.softTags)
 
-      if (staleByTags || isStale) {
-        span.addEvent('Stale', { staleByTags, isStale })
+      if (staleByTags) {
+        span.addEvent('Stale', { staleByTags })
         span.end()
         return null
       }
@@ -194,36 +179,6 @@ export class NetlifyCacheHandler implements CacheHandler {
 
     return isStale
   }
-
-  /**
-   * Retrieves the milliseconds since midnight, January 1, 1970 when it should revalidate for a path.
-   */
-  private calculateRevalidate(
-    cacheKey: string,
-    fromTime: number,
-    ctx: Parameters<CacheHandler['get']>[1],
-    dev?: boolean,
-  ): number | false {
-    // in development we don't have a prerender-manifest
-    // and default to always revalidating to allow easier debugging
-    if (dev) return Date.now() - 1_000
-
-    if (ctx?.revalidate && typeof ctx.revalidate === 'number') {
-      return fromTime + ctx.revalidate * 1_000
-    }
-
-    // if an entry isn't present in routes we fallback to a default
-    const { initialRevalidateSeconds } = prerenderManifest.routes[toRoute(cacheKey)] || {
-      initialRevalidateSeconds: 0,
-    }
-    // the initialRevalidate can be either set to false or to a number (representing the seconds)
-    const revalidateAfter: number | false =
-      typeof initialRevalidateSeconds === 'number'
-        ? initialRevalidateSeconds * 1_000 + fromTime
-        : initialRevalidateSeconds
-
-    return revalidateAfter
-  }
 }
 
 export default NetlifyCacheHandler

src/run/handlers/server.ts

@@ -53,15 +53,13 @@ export default async (request: Request) => {
   // temporary workaround for https://linear.app/netlify/issue/ADN-111/
   delete req.headers['accept-encoding']
 
-  try {
-    // console.log('Next server request:', req.url)
-    await nextHandler(req, resProxy)
-  } catch (error) {
+  // We don't await this here, because it won't resolve until the response is finished.
+  const nextHandlerPromise = nextHandler(req, resProxy).catch((error) => {
     logger.withError(error).error('next handler error')
     console.error(error)
     resProxy.statusCode = 500
     resProxy.end('Internal Server Error')
-  }
+  })
 
   // Contrary to the docs, this resolves when the headers are available, not when the stream closes.
   // See https://github.com/fastly/http-compute-js/blob/main/src/http-compute-js/http-server.ts#L168-L173
@@ -85,5 +83,13 @@ export default async (request: Request) => {
     return new Response(body || null, response)
   }
 
-  return response
+  const keepOpenUntilNextFullyRendered = new TransformStream({
+    flush() {
+      // it's important to keep the stream open until the next handler has finished,
+      // or otherwise the cache revalidates might not go through
+      return nextHandlerPromise
+    },
+  })
+
+  return new Response(response.body?.pipeThrough(keepOpenUntilNextFullyRendered), response)
 }

src/run/headers.ts

@@ -82,7 +82,9 @@ export const setVaryHeaders = (
  * from the cache, meaning that the CDN will cache it for 10 seconds, which is incorrect.
  */
 export const adjustDateHeader = async (headers: Headers, request: Request) => {
-  if (headers.get('x-nextjs-cache') !== 'HIT') {
+  const cacheState = headers.get('x-nextjs-cache')
+  const isServedFromCache = cacheState === 'HIT' || cacheState === 'STALE'
+  if (!isServedFromCache) {
     return
   }
   const path = new URL(request.url).pathname
@@ -135,12 +137,13 @@ export const setCacheTagsHeaders = (headers: Headers, request: Request, manifest
 /**
  * https://httpwg.org/specs/rfc9211.html
  *
- * We only should get HIT and MISS statuses from Next cache.
+ * We get HIT, MISS, STALE statuses from Next cache.
  * We will ignore other statuses and will not set Cache-Status header in those cases.
  */
 const NEXT_CACHE_TO_CACHE_STATUS: Record<string, string> = {
   HIT: `hit`,
   MISS: `miss,`,
+  STALE: `hit; fwd=stale`,
 }
 
 /**

tests/e2e/simple-app.test.ts

@@ -54,3 +54,51 @@ test('next/image is using Netlify Image CDN', async ({ page }) => {
 
   await expectImageWasLoaded(page.locator('img'))
 })
+
+const waitFor = (ms: number) => new Promise((resolve) => setTimeout(resolve, ms))
+
+// adaptation of https://github.com/vercel/next.js/blob/canary/test/e2e/app-dir/app-static/app-static.test.ts#L1716-L1755
+test('should stream properly', async ({ page }) => {
+  // Prime the cache.
+  const path = `${ctx.url}/stale-cache-serving/app-page`
+  const res = await fetch(path)
+  expect(res.status).toBe(200)
+
+  // Consume the cache, the revalidations are completed on the end of the
+  // stream so we need to wait for that to complete.
+  await res.text()
+
+  // different from next.js test:
+  // we need to wait another 10secs for the blob to propagate back
+  // can be removed once we have a local cache for blobs
+  await waitFor(10000)
+
+  for (let i = 0; i < 6; i++) {
+    await waitFor(1000)
+
+    const timings = {
+      start: Date.now(),
+      startedStreaming: 0,
+    }
+
+    const res = await fetch(path)
+
+    // eslint-disable-next-line no-loop-func
+    await new Promise<void>((resolve) => {
+      res.body?.pipeTo(
+        new WritableStream({
+          write() {
+            if (!timings.startedStreaming) {
+              timings.startedStreaming = Date.now()
+            }
+          },
+          close() {
+            resolve()
+          },
+        }),
+      )
+    })
+
+    expect(timings.startedStreaming - timings.start, `streams in less than 3s, run #${i}/6`).toBeLessThan(3000)
+  }
+})

tests/fixtures/revalidate-fetch/app/dynamic-posts/[id]/page.js

@@ -0,0 +1,33 @@
+const revalidateSeconds = +process.env.REVALIDATE_SECONDS || 5
+const API_BASE = process.env.API_BASE || 'https://api.tvmaze.com/shows/'
+
+async function getData(params) {
+  const res = await fetch(new URL(params.id, API_BASE).href, {
+    next: { revalidate: revalidateSeconds },
+  })
+  return res.json()
+}
+
+export default async function Page({ params }) {
+  const data = await getData(params)
+
+  return (
+    <>
+      <h1>Revalidate Fetch (on dynamic page)</h1>
+      <p>Revalidating used fetch every {revalidateSeconds} seconds</p>
+      <dl>
+        <dt>Show</dt>
+        <dd data-testid="name">{data.name}</dd>
+        <dt>Param</dt>
+        <dd data-testid="id">{params.id}</dd>
+        <dt>Time</dt>
+        <dd data-testid="date-now">{Date.now()}</dd>
+        <dt>Time from fetch response</dt>
+        <dd data-testid="date-from-response">{data.date ?? 'no-date-in-response'}</dd>
+      </dl>
+    </>
+  )
+}
+
+// make page dynamic, but still use fetch cache
+export const dynamic = 'force-dynamic'

tests/fixtures/revalidate-fetch/app/posts/[id]/page.js

@@ -27,6 +27,8 @@ export default async function Page({ params }) {
         <dd data-testid="id">{params.id}</dd>
         <dt>Time</dt>
         <dd data-testid="date-now">{Date.now()}</dd>
+        <dt>Time from fetch response</dt>
+        <dd data-testid="date-from-response">{data.date ?? 'no-date-in-response'}</dd>
       </dl>
     </>
   )

tests/fixtures/simple-next-app/app/stale-cache-serving/app-page/page.js

@@ -0,0 +1,20 @@
+export const dynamic = 'force-dynamic'
+
+const delay = 3000
+
+export default async function Page(props) {
+  const start = Date.now()
+  const data = await fetch(
+    `https://next-data-api-endpoint.vercel.app/api/delay?delay=${delay}`,
+    { next: { revalidate: 3 } }
+  ).then((res) => res.json())
+  const fetchDuration = Date.now() - start
+
+  return (
+    <>
+      <p id="data">
+        {JSON.stringify({ fetchDuration, data, now: Date.now() })}
+      </p>
+    </>
+  )
+}