[Segment Cache] Fix stale time unit conversion

Fixes a bug in the handling of stale times in the client Segment Cache, when prefetching a PPR-enabled route. The stale time sent by the server is in seconds, but I was handling it as if it were in milliseconds. This caused the entry to expire almost immediately. I didn't notice this sooner because the main test apps I've been working with doesn't have PPR enabled, and the non-PPR path was converting the values correctly. The existing e2e tests also didn't catch it because they run fast enough that they the entries don't expire.
vercel · Jan 10, 2025 · 4f388f4 · 4f388f4
1 parent 5bd025d
commit 4f388f4
Show file tree

Hide file tree

Showing 8 changed files with 202 additions and 11 deletions.
diff --git a/packages/next/src/client/components/segment-cache/cache.ts b/packages/next/src/client/components/segment-cache/cache.ts
@@ -847,12 +847,13 @@ export async function fetchRouteOnCacheMiss(
         return null
       }
 
+      const staleTimeMs = serverData.staleTime * 1000
       fulfillRouteCacheEntry(
         entry,
         convertRootTreePrefetchToRouteTree(serverData),
         serverData.head,
         serverData.isHeadPartial,
-        Date.now() + serverData.staleTime,
+        Date.now() + staleTimeMs,
         couldBeIntercepted,
         canonicalUrl,
         routeIsPPREnabled
@@ -1114,17 +1115,19 @@ function writeDynamicTreeResponseIntoCache(
 
   const flightRouterState = flightData.tree
   // TODO: Extract to function
-  const staleTimeHeader = response.headers.get(NEXT_ROUTER_STALE_TIME_HEADER)
-  const staleTime =
-    staleTimeHeader !== null
-      ? parseInt(staleTimeHeader, 10)
+  const staleTimeHeaderSeconds = response.headers.get(
+    NEXT_ROUTER_STALE_TIME_HEADER
+  )
+  const staleTimeMs =
+    staleTimeHeaderSeconds !== null
+      ? parseInt(staleTimeHeaderSeconds, 10) * 1000
       : STATIC_STALETIME_MS
   fulfillRouteCacheEntry(
     entry,
     convertRootFlightRouterStateToRouteTree(flightRouterState),
     flightData.head,
     flightData.isHeadPartial,
-    now + staleTime,
+    now + staleTimeMs,
     couldBeIntercepted,
     canonicalUrl,
     routeIsPPREnabled
@@ -1189,17 +1192,17 @@ function writeDynamicRenderResponseIntoCache(
           encodeSegment(segment)
         )
       }
-      const staleTimeHeader = response.headers.get(
+      const staleTimeHeaderSeconds = response.headers.get(
         NEXT_ROUTER_STALE_TIME_HEADER
       )
-      const staleTime =
-        staleTimeHeader !== null
-          ? parseInt(staleTimeHeader, 10)
+      const staleTimeMs =
+        staleTimeHeaderSeconds !== null
+          ? parseInt(staleTimeHeaderSeconds, 10) * 1000
           : STATIC_STALETIME_MS
       writeSeedDataIntoCache(
         now,
         route,
-        now + staleTime,
+        now + staleTimeMs,
         seedData,
         segmentKey,
         spawnedEntries

diff --git a/test/e2e/app-dir/segment-cache/staleness/app/layout.tsx b/test/e2e/app-dir/segment-cache/staleness/app/layout.tsx
@@ -0,0 +1,11 @@
+export default function RootLayout({
+  children,
+}: {
+  children: React.ReactNode
+}) {
+  return (
+    <html lang="en">
+      <body>{children}</body>
+    </html>
+  )
+}
diff --git a/test/e2e/app-dir/segment-cache/staleness/app/page.tsx b/test/e2e/app-dir/segment-cache/staleness/app/page.tsx
@@ -0,0 +1,26 @@
+import { LinkAccordion } from '../components/link-accordion'
+
+export default function Page() {
+  return (
+    <>
+      <p>
+        This page is used to test various scenarios related to prefetch cache
+        staleness. In the corresponding e2e test, the links below are prefetched
+        (by toggling their visibility), time is elapsed, and then prefetched
+        again to check whether a new network request is made.
+      </p>
+      <ul>
+        <li>
+          <LinkAccordion href="/stale-5-minutes">
+            Page with stale time of 5 minutes
+          </LinkAccordion>
+        </li>
+        <li>
+          <LinkAccordion href="/stale-10-minutes">
+            Page with stale time of 10 minutes
+          </LinkAccordion>
+        </li>
+      </ul>
+    </>
+  )
+}
diff --git a/test/e2e/app-dir/segment-cache/staleness/app/stale-10-minutes/page.tsx b/test/e2e/app-dir/segment-cache/staleness/app/stale-10-minutes/page.tsx
@@ -0,0 +1,17 @@
+import { Suspense } from 'react'
+import { unstable_cacheLife as cacheLife } from 'next/cache'
+
+async function Content() {
+  'use cache'
+  await new Promise((resolve) => setTimeout(resolve, 0))
+  cacheLife({ stale: 10 * 60 })
+  return 'Content with stale time of 10 minutes'
+}
+
+export default function Page() {
+  return (
+    <Suspense fallback="Loading...">
+      <Content />
+    </Suspense>
+  )
+}
diff --git a/test/e2e/app-dir/segment-cache/staleness/app/stale-5-minutes/page.tsx b/test/e2e/app-dir/segment-cache/staleness/app/stale-5-minutes/page.tsx
@@ -0,0 +1,17 @@
+import { Suspense } from 'react'
+import { unstable_cacheLife as cacheLife } from 'next/cache'
+
+async function Content() {
+  'use cache'
+  await new Promise((resolve) => setTimeout(resolve, 0))
+  cacheLife({ stale: 5 * 60 })
+  return 'Content with stale time of 5 minutes'
+}
+
+export default function Page() {
+  return (
+    <Suspense fallback="Loading...">
+      <Content />
+    </Suspense>
+  )
+}
diff --git a/test/e2e/app-dir/segment-cache/staleness/components/link-accordion.tsx b/test/e2e/app-dir/segment-cache/staleness/components/link-accordion.tsx
@@ -0,0 +1,23 @@
+'use client'
+
+import Link from 'next/link'
+import { useState } from 'react'
+
+export function LinkAccordion({ href, children }) {
+  const [isVisible, setIsVisible] = useState(false)
+  return (
+    <>
+      <input
+        type="checkbox"
+        checked={isVisible}
+        onChange={() => setIsVisible(!isVisible)}
+        data-link-accordion={href}
+      />
+      {isVisible ? (
+        <Link href={href}>{children}</Link>
+      ) : (
+        `${children} (link is hidden)`
+      )}
+    </>
+  )
+}
diff --git a/test/e2e/app-dir/segment-cache/staleness/next.config.js b/test/e2e/app-dir/segment-cache/staleness/next.config.js
@@ -0,0 +1,12 @@
+/**
+ * @type {import('next').NextConfig}
+ */
+const nextConfig = {
+  experimental: {
+    ppr: true,
+    dynamicIO: true,
+    clientSegmentCache: true,
+  },
+}
+
+module.exports = nextConfig
diff --git a/test/e2e/app-dir/segment-cache/staleness/segment-cache-stale-time.test.ts b/test/e2e/app-dir/segment-cache/staleness/segment-cache-stale-time.test.ts
@@ -0,0 +1,82 @@
+import { nextTestSetup } from 'e2e-utils'
+import type * as Playwright from 'playwright'
+import { createRouterAct } from '../router-act'
+
+describe('segment cache (staleness)', () => {
+  const { next, isNextDev, skipped } = nextTestSetup({
+    files: __dirname,
+    skipDeployment: true,
+  })
+  if (isNextDev || skipped) {
+    test('disabled in development / deployment', () => {})
+    return
+  }
+
+  it('entry expires when its stale time has elapsed', async () => {
+    let page: Playwright.Page
+    const browser = await next.browser('/', {
+      beforePageLoad(p: Playwright.Page) {
+        page = p
+      },
+    })
+    const act = createRouterAct(page)
+
+    await page.clock.install()
+
+    // Reveal the link to trigger a prefetch
+    const toggle5MinutesLink = await browser.elementByCss(
+      'input[data-link-accordion="/stale-5-minutes"]'
+    )
+    const toggle10MinutesLink = await browser.elementByCss(
+      'input[data-link-accordion="/stale-10-minutes"]'
+    )
+    await act(
+      async () => {
+        await toggle5MinutesLink.click()
+        await browser.elementByCss('a[href="/stale-5-minutes"]')
+      },
+      {
+        includes: 'Content with stale time of 5 minutes',
+      }
+    )
+    await act(
+      async () => {
+        await toggle10MinutesLink.click()
+        await browser.elementByCss('a[href="/stale-10-minutes"]')
+      },
+      {
+        includes: 'Content with stale time of 10 minutes',
+      }
+    )
+
+    // Hide the links
+    await toggle5MinutesLink.click()
+    await toggle10MinutesLink.click()
+
+    // Fast forward 5 minutes and 1 millisecond
+    await page.clock.fastForward(5 * 60 * 1000 + 1)
+
+    // Reveal the links again to trigger new prefetch tasks
+    await act(
+      async () => {
+        await toggle5MinutesLink.click()
+        await browser.elementByCss('a[href="/stale-5-minutes"]')
+      },
+      // The page with a stale time of 5 minutes is requested again
+      // because its stale time elapsed.
+      {
+        includes: 'Content with stale time of 5 minutes',
+      }
+    )
+
+    await act(
+      async () => {
+        await toggle10MinutesLink.click()
+        await browser.elementByCss('a[href="/stale-10-minutes"]')
+      },
+      // The page with a stale time of 10 minutes is *not* requested again
+      // because it's still fresh.
+      'no-requests'
+    )
+  })
+})