fix: unstable_cache should perform blocking revalidation during ISR revalidation

Author: ztanner

packages/next/src/server/web/spec-extension/unstable-cache.ts

@@ -239,33 +239,72 @@ export function unstable_cache<T extends Callback>(
                   ? JSON.parse(cacheEntry.value.data.body)
                   : undefined
               if (cacheEntry.isStale) {
-                // In App Router we return the stale result and revalidate in the background
                 if (!workStore.pendingRevalidates) {
                   workStore.pendingRevalidates = {}
                 }
 
-                // We run the cache function asynchronously and save the result when it completes
-                workStore.pendingRevalidates[invocationKey] =
-                  workUnitAsyncStorage
-                    .run(innerCacheStore, cb, ...args)
-                    .then((result) => {
-                      return cacheNewResult(
-                        result,
-                        incrementalCache,
-                        cacheKey,
-                        tags,
-                        options.revalidate,
-                        fetchIdx,
-                        fetchUrl
-                      )
-                    })
-                    // @TODO This error handling seems wrong. We swallow the error?
-                    .catch((err) =>
-                      console.error(
-                        `revalidating cache with key: ${invocationKey}`,
-                        err
-                      )
-                    )
+                if (workStore.isRevalidate) {
+                  // When the page is revalidating and the cache entry is stale,
+                  // we need to wait for fresh data (blocking revalidate)
+                  let pendingRevalidate =
+                    workStore.pendingRevalidates[invocationKey]
+
+                  if (!pendingRevalidate) {
+                    // Create the revalidation promise
+                    pendingRevalidate = workUnitAsyncStorage
+                      .run(innerCacheStore, cb, ...args)
+                      .then(async (result) => {
+                        await cacheNewResult(
+                          result,
+                          incrementalCache,
+                          cacheKey,
+                          tags,
+                          options.revalidate,
+                          fetchIdx,
+                          fetchUrl
+                        )
+                        return result
+                      })
+
+                    // Attach empty catch to prevent unhandled rejection warnings
+                    // This mirrors handling in patch-fetch
+                    pendingRevalidate.catch(() => {})
+
+                    workStore.pendingRevalidates[invocationKey] =
+                      pendingRevalidate
+                  }
+
+                  // Wait for and return the revalidated result
+                  return pendingRevalidate
+                } else if (cacheEntry.isStale) {
+                  // In App Router we return the stale result and revalidate in the background
+
+                  // Check if there's already a pending revalidation to avoid duplicate work
+                  if (!workStore.pendingRevalidates[invocationKey]) {
+                    // We run the cache function asynchronously and save the result when it completes
+                    workStore.pendingRevalidates[invocationKey] =
+                      workUnitAsyncStorage
+                        .run(innerCacheStore, cb, ...args)
+                        .then((result) => {
+                          return cacheNewResult(
+                            result,
+                            incrementalCache,
+                            cacheKey,
+                            tags,
+                            options.revalidate,
+                            fetchIdx,
+                            fetchUrl
+                          )
+                        })
+                        // @TODO This error handling seems wrong. We swallow the error?
+                        .catch((err) =>
+                          console.error(
+                            `revalidating cache with key: ${invocationKey}`,
+                            err
+                          )
+                        )
+                  }
+                }
               }
               // We had a valid cache entry so we return it here
               return cachedResponse

test/production/unstable-cache-foreground-revalidate/app/isr-10/page.js

@@ -0,0 +1,44 @@
+import { unstable_cache } from 'next/cache'
+
+export const revalidate = 10
+
+const getCachedData = unstable_cache(
+  async () => {
+    const generatedAt = Date.now()
+
+    // Log when this function is actually executed
+    console.log('[TEST] unstable_cache callback executed at:', generatedAt)
+
+    // Add a delay to simulate expensive operation
+    await new Promise((resolve) => setTimeout(resolve, 100))
+
+    return {
+      generatedAt,
+      random: Math.random(),
+    }
+  },
+  ['cached-data'],
+  {
+    revalidate: 5,
+  }
+)
+
+export default async function Page() {
+  const pageRenderStart = Date.now()
+  console.log('[TEST] Page render started at:', pageRenderStart)
+
+  const cachedData = await getCachedData()
+
+  console.log(
+    '[TEST] Page render completed with cache data from:',
+    cachedData.generatedAt
+  )
+
+  return (
+    <div>
+      <div id="page-time">{pageRenderStart}</div>
+      <div id="cache-generated-at">{cachedData.generatedAt}</div>
+      <div id="random">{cachedData.random}</div>
+    </div>
+  )
+}

test/production/unstable-cache-foreground-revalidate/app/layout.js

@@ -0,0 +1,7 @@
+export default function RootLayout({ children }) {
+  return (
+    <html>
+      <body>{children}</body>
+    </html>
+  )
+}

test/production/unstable-cache-foreground-revalidate/next.config.js

@@ -0,0 +1 @@
+module.exports = {}

test/production/unstable-cache-foreground-revalidate/unstable-cache-foreground-revalidate.test.ts

@@ -0,0 +1,58 @@
+import { nextTestSetup } from 'e2e-utils'
+
+describe('unstable-cache-foreground-revalidate', () => {
+  const { next, isNextDev } = nextTestSetup({
+    files: __dirname,
+    skipDeployment: true,
+  })
+
+  if (isNextDev) {
+    it.skip('should not run in dev mode', () => {})
+    return
+  }
+
+  it('should block and wait for fresh data when ISR page revalidate time is greater than unstable_cache TTL', async () => {
+    // Initial render to warm up cache
+    await next.render('/isr-10')
+
+    // Record initial log position
+    const initialLogLength = next.cliOutput.length
+
+    // Wait for both ISR and unstable_cache to become stale
+    await new Promise((resolve) => setTimeout(resolve, 11000))
+
+    // This request triggers ISR background revalidation
+    await next.render('/isr-10')
+
+    // Wait for ISR background revalidation to complete
+    await new Promise((resolve) => setTimeout(resolve, 2000))
+
+    // Get logs since the initial render
+    const logs = next.cliOutput.substring(initialLogLength)
+
+    // Extract timestamps from logs
+    const cacheExecutionMatch = logs.match(
+      /\[TEST\] unstable_cache callback executed at: (\d+)/
+    )
+    const completionMatch = logs.match(
+      /\[TEST\] Page render completed with cache data from: (\d+)/
+    )
+
+    if (cacheExecutionMatch && completionMatch) {
+      const cacheExecutedAt = parseInt(cacheExecutionMatch[1])
+      const cacheDataFrom = parseInt(completionMatch[1])
+      const timeDiff = Math.abs(cacheExecutedAt - cacheDataFrom)
+
+      console.log('Cache execution timing:')
+      console.log('- Cache executed at:', cacheExecutedAt)
+      console.log('- Page used cache data from:', cacheDataFrom)
+      console.log('- Time difference:', timeDiff, 'ms')
+
+      // With foreground revalidation:
+      // - ISR waits for fresh data, so timestamps should match (< 1000ms difference)
+      // Without foreground revalidation:
+      // - ISR uses stale data, so timestamps will be far apart (> 10000ms)
+      expect(timeDiff).toBeLessThan(1000)
+    }
+  })
+})