fix: unstable_cache should perform blocking revalidation during ISR revalidation

ztanner · ztanner · commit 9647a8294e5c · 2025-09-15T15:28:25.000-07:00
diff --git a/packages/next/src/server/web/spec-extension/unstable-cache.ts b/packages/next/src/server/web/spec-extension/unstable-cache.ts
@@ -238,18 +238,19 @@ export function unstable_cache<T extends Callback>(
                 cacheEntry.value.data.body !== undefined
                   ? 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
+                // Check if there's already a pending revalidation to avoid duplicate work
+                if (!workStore.pendingRevalidates[invocationKey]) {
+                  // Create the revalidation promise
+                  const revalidationPromise = workUnitAsyncStorage
                     .run(innerCacheStore, cb, ...args)
-                    .then((result) => {
-                      return cacheNewResult(
+                    .then(async (result) => {
+                      await cacheNewResult(
                         result,
                         incrementalCache,
                         cacheKey,
@@ -258,15 +259,37 @@ export function unstable_cache<T extends Callback>(
                         fetchIdx,
                         fetchUrl
                       )
+                      return result
                     })
-                    // @TODO This error handling seems wrong. We swallow the error?
-                    .catch((err) =>
+                    .catch((err) => {
+                      // @TODO This error handling seems wrong. We swallow the error?
                       console.error(
                         `revalidating cache with key: ${invocationKey}`,
                         err
                       )
-                    )
+                      // Return the stale value on error for foreground revalidation
+                      return cachedResponse
+                    })
+
+                  // Attach the empty catch here so we don't get a "unhandled promise
+                  // rejection" warning. (Behavior is matched with patch-fetch)
+                  if (workStore.isRevalidate) {
+                    revalidationPromise.catch(() => {})
+                  }
+
+                  workStore.pendingRevalidates[invocationKey] =
+                    revalidationPromise
+                }
+
+                // Check if we need to do foreground revalidation
+                if (workStore.isRevalidate) {
+                  // When the page is revalidating and the cache entry is stale,
+                  // we need to wait for fresh data (blocking revalidate)
+                  return workStore.pendingRevalidates[invocationKey]
+                }
+                // Otherwise, we're doing background revalidation - return stale immediately
               }
+
               // We had a valid cache entry so we return it here
               return cachedResponse
             }
diff --git a/test/experimental-tests-manifest.json b/test/experimental-tests-manifest.json
@@ -354,7 +354,8 @@
       "test/production/app-dir/typed-routes-with-webpack-worker/typed-routes-with-webpack-worker.test.ts",
       "test/production/app-dir/unexpected-error/unexpected-error.test.ts",
       "test/production/app-dir/worker-restart/worker-restart.test.ts",
-      "test/production/app-dir/resume-data-cache/resume-data-cache.test.ts"
+      "test/production/app-dir/resume-data-cache/resume-data-cache.test.ts",
+      "test/production/app-dir/unstable-cache-foreground-revalidate/unstable-cache-foreground-revalidate.test.ts"
     ]
   }
 }
diff --git a/test/production/app-dir/unstable-cache-foreground-revalidate/app/isr-10/page.js b/test/production/app-dir/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>
+  )
+}
diff --git a/test/production/app-dir/unstable-cache-foreground-revalidate/app/layout.js b/test/production/app-dir/unstable-cache-foreground-revalidate/app/layout.js
@@ -0,0 +1,7 @@
+export default function RootLayout({ children }) {
+  return (
+    <html>
+      <body>{children}</body>
+    </html>
+  )
+}
diff --git a/test/production/app-dir/unstable-cache-foreground-revalidate/next.config.js b/test/production/app-dir/unstable-cache-foreground-revalidate/next.config.js
@@ -0,0 +1 @@
+module.exports = {}
diff --git a/test/production/app-dir/unstable-cache-foreground-revalidate/unstable-cache-foreground-revalidate.test.ts b/test/production/app-dir/unstable-cache-foreground-revalidate/unstable-cache-foreground-revalidate.test.ts
@@ -0,0 +1,74 @@
+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)
+
+    const cacheExecutions = [
+      ...logs.matchAll(/\[TEST\] unstable_cache callback executed at: (\d+)/g),
+    ]
+    const completions = [
+      ...logs.matchAll(
+        /\[TEST\] Page render completed with cache data from: (\d+)/g
+      ),
+    ]
+
+    if (completions.length === 0) {
+      throw new Error('No page completions found in logs')
+    }
+
+    const lastCompletion = completions[completions.length - 1]
+    const lastCacheExecution =
+      cacheExecutions.length > 0
+        ? cacheExecutions[cacheExecutions.length - 1]
+        : null
+
+    if (!lastCacheExecution) {
+      throw new Error(
+        `Expected cache execution during ISR revalidation but found none. ` +
+          `Cache executions: ${cacheExecutions.length}, Page completions: ${completions.length}`
+      )
+    }
+
+    const cacheExecutedAt = parseInt(lastCacheExecution[1])
+    const cacheDataFrom = parseInt(lastCompletion[1])
+    const timeDiff = Math.abs(cacheExecutedAt - cacheDataFrom)
+
+    console.log('ISR revalidation timing:')
+    console.log('- Cache executed at:', cacheExecutedAt)
+    console.log('- ISR 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)
+  })
+})
