Skip to content

Commit ac55fd9

Browse files
unstubbableunstubbable
authored
Fix duplicate background revalidation for 'use cache' (#98446)
An early `cacheHandler.set()` call previously made subsequent same-key `get()` calls wait for generation and persistence. This prevented another background revalidation, but those requests waited instead of receiving the existing stale data. The work in #94694 added private-cache persistence in dev. Its helper refactor made cross-request invocations wait for cache writes before retrying root-param-specific lookups. It also moved the coarse-key `set()` call after entry collection. That unintentionally allowed subsequent requests to receive stale data and start another background revalidation while one was already running. This change tracks background work separately for each lookup key within a runtime instance. Subsequent requests can receive the existing stale entry without waiting for generation or starting duplicate work. The task remains registered until collection and its cache writes settle. Each lookup records existing work before calling `get()`, so a slow lookup does not start another revalidation after the task it observed has completed. The handler still receives `set()` after collection, including asynchronous rendering of the cached result. Subsequent `get()` calls can still block while the handler persists the entry. We accept that remaining wait to preserve the cache-handler contract without bypassing handler reads. **Alternatives Considered** - Restore early `set()` calls and rely on the handler's pending-set promises. This would prevent duplicate regeneration, but would also make subsequent requests wait for generation again instead of receiving stale data. - Extend the existing cross-request sharing mechanism through background revalidation. We prototyped retaining stale and regenerated results, but that required additional stream ownership, invalidation checks, and root-param matching. The separate background-task map avoids those changes and leaves cold and foreground generation unchanged. Fixes #98326
1 parent c115657 commit ac55fd9

9 files changed

Lines changed: 461 additions & 98 deletions

File tree

‎packages/next/src/server/use-cache/use-cache-wrapper.ts‎

Lines changed: 103 additions & 56 deletions
Original file line numberDiff line numberDiff line change
@@ -350,6 +350,10 @@ const crossRequestPendingCacheInvocations = new Map<
350350
Promise<SharedCacheResult>
351351
>()
352352

353+
// Prevent duplicate background revalidations for the same key until generation
354+
// and cache writes finish.
355+
const backgroundRevalidations = new Map<string, Promise<void>>()
356+
353357
// The first argument at each call site is the full directive that produced
354358
// the invocation, e.g. "'use cache'" or "'use cache: remote'".
355359
const debug = process.env.NEXT_PRIVATE_DEBUG_CACHE
@@ -610,17 +614,16 @@ function serveJoinedCacheEntry(
610614

611615
function saveToCacheHandler(
612616
cacheHandler: CacheReadWriteHandler,
613-
workStore: WorkStore,
614617
id: string,
615618
cacheHandlerKeyBase: string,
616619
savedCacheResult: Promise<CollectedCacheResult>,
617620
rootParams: Params | undefined
618-
): Promise<CollectedCacheResult> {
621+
): Promise<void> {
619622
// Write the entry to the cache handler. With root params, this is a redirect
620623
// entry at the coarse key plus the actual entry at the specific key;
621624
// otherwise just the entry at the coarse key. Both set calls are fired
622625
// together and awaited in parallel.
623-
const combinedSetPromise = savedCacheResult.then(async (collectedResult) => {
626+
return savedCacheResult.then(async (collectedResult) => {
624627
const { entry: fullEntry, readRootParamNames } = collectedResult
625628

626629
// Use the combined set (union of all historically observed reads) for both
@@ -644,7 +647,10 @@ function saveToCacheHandler(
644647
computeRootParamsCacheKeySuffix(rootParams, rootParamNames)
645648

646649
setPromises.push(
647-
cacheHandler.set(specificKey, Promise.resolve(fullEntry))
650+
// Capture synchronous `set()` throws as rejected write promises.
651+
new Promise<void>((resolve) => {
652+
resolve(cacheHandler.set(specificKey, Promise.resolve(fullEntry)))
653+
})
648654
)
649655

650656
// The coarse key gets a redirect entry instead. On a cold server (empty
@@ -671,25 +677,22 @@ function saveToCacheHandler(
671677
}
672678

673679
setPromises.push(
674-
cacheHandler.set(cacheHandlerKeyBase, Promise.resolve(coarseEntry))
680+
// Capture synchronous `set()` throws as rejected write promises.
681+
new Promise<void>((resolve) => {
682+
resolve(
683+
cacheHandler.set(cacheHandlerKeyBase, Promise.resolve(coarseEntry))
684+
)
685+
})
675686
)
676687

677-
await Promise.all(setPromises)
688+
// A failed write must not finish the operation while another write is
689+
// pending.
690+
for (const result of await Promise.allSettled(setPromises)) {
691+
if (result.status === 'rejected') {
692+
throw result.reason
693+
}
694+
}
678695
})
679-
680-
workStore.pendingRevalidateWrites ??= []
681-
workStore.pendingRevalidateWrites.push(combinedSetPromise)
682-
683-
// A cross-request joiner reads its recomputed specific key only after it has
684-
// awaited this entry's metadata, so gate the metadata on the writes landing:
685-
// that guarantees the entry is present when the joiner re-reads. A failed
686-
// write shouldn't reject the metadata (the joiner just misses and
687-
// regenerates), so settle either way; a collection failure still propagates
688-
// through `savedCacheResult`.
689-
return combinedSetPromise.then(
690-
() => savedCacheResult,
691-
() => savedCacheResult
692-
)
693696
}
694697

695698
function generateCacheEntry(
@@ -3064,9 +3067,14 @@ export async function cache(
30643067
}
30653068

30663069
let entry: CacheEntry | undefined
3070+
let observedRevalidation: Promise<void> | undefined
30673071

30683072
// We ignore existing cache entries when force revalidating.
30693073
if (cacheHandler && !shouldForceRevalidate(workStore, workUnitStore)) {
3074+
// Record an existing revalidation before `get()` so a stale result
3075+
// does not start another revalidation. The existing revalidation may
3076+
// finish and remove its map entry before the lookup returns.
3077+
observedRevalidation = backgroundRevalidations.get(cacheHandlerKey)
30703078
entry = await cacheHandler.get(cacheHandlerKey, implicitTags)
30713079

30723080
// Check if this is a redirect entry (coarse key → specific key).
@@ -3084,6 +3092,8 @@ export async function cache(
30843092
cacheHandlerKey =
30853093
cacheHandlerKeyBase +
30863094
computeRootParamsCacheKeySuffix(rootParams, paramNames)
3095+
observedRevalidation =
3096+
backgroundRevalidations.get(cacheHandlerKey)
30873097
entry = await cacheHandler.get(cacheHandlerKey, implicitTags)
30883098
}
30893099
}
@@ -3386,14 +3396,24 @@ export async function cache(
33863396
)
33873397

33883398
if (cacheHandler) {
3389-
metadataSource = saveToCacheHandler(
3399+
const pendingWrite = saveToCacheHandler(
33903400
cacheHandler,
3391-
workStore,
33923401
id,
33933402
cacheHandlerKeyBase,
33943403
savedCacheResult,
33953404
rootParams
33963405
)
3406+
workStore.pendingRevalidateWrites ??= []
3407+
workStore.pendingRevalidateWrites.push(pendingWrite)
3408+
// Wait for cache writes before exposing metadata. Cross-request
3409+
// joiners may recompute their root-param-specific key from it,
3410+
// then read the handler again. A write failure does not reject
3411+
// the metadata: joiners can regenerate if no entry was stored.
3412+
// Collection failures still propagate through `savedCacheResult`.
3413+
metadataSource = pendingWrite.then(
3414+
() => savedCacheResult,
3415+
() => savedCacheResult
3416+
)
33973417
}
33983418
}
33993419

@@ -3536,23 +3556,32 @@ export async function cache(
35363556
}
35373557
}
35383558

3539-
if (shouldTriggerBackgroundRevalidation) {
3559+
if (
3560+
shouldTriggerBackgroundRevalidation &&
3561+
// The revalidation observed before the lookup may have finished.
3562+
observedRevalidation === undefined &&
3563+
// A new revalidation may have started since the lookup began.
3564+
!backgroundRevalidations.has(cacheHandlerKey)
3565+
) {
35403566
const revalidateCacheHandlerKey = cacheHandlerKey
3541-
const revalidatePromise = generateCacheEntry(
3542-
workStore,
3543-
// The background revalidation preserves the outer store for
3544-
// reading (e.g. implicitTags) but skips propagation of cache life
3545-
// and tags back to the outer scope.
3546-
{
3547-
...cacheContext,
3548-
skipPropagation: true,
3549-
},
3550-
clientReferenceManifest,
3551-
encodedCacheKeyParts,
3552-
fn,
3553-
timeoutError,
3554-
deadlockError
3555-
)
3567+
// Defer the call so synchronous setup errors reject the background
3568+
// task instead of failing the stale response. This also lets us
3569+
// register the task before generation starts.
3570+
const revalidatePromise = Promise.resolve()
3571+
.then(() =>
3572+
generateCacheEntry(
3573+
workStore,
3574+
// The background revalidation preserves the outer store for
3575+
// reading (e.g. implicitTags) but skips propagation of cache
3576+
// life and tags back to the outer scope.
3577+
{ ...cacheContext, skipPropagation: true },
3578+
clientReferenceManifest,
3579+
encodedCacheKeyParts,
3580+
fn,
3581+
timeoutError,
3582+
deadlockError
3583+
)
3584+
)
35563585
.then(async (result) => {
35573586
if (result.type === 'cached') {
35583587
const { stream: ignoredStream, pendingCacheResult } = result
@@ -3564,28 +3593,46 @@ export async function cache(
35643593
logPrefix
35653594
)
35663595

3567-
if (cacheHandler) {
3568-
saveToCacheHandler(
3569-
cacheHandler,
3570-
workStore,
3571-
id,
3572-
cacheHandlerKeyBase,
3573-
savedCacheResult,
3574-
rootParams
3575-
)
3596+
const pendingWrite = cacheHandler
3597+
? saveToCacheHandler(
3598+
cacheHandler,
3599+
id,
3600+
cacheHandlerKeyBase,
3601+
savedCacheResult,
3602+
rootParams
3603+
)
3604+
: savedCacheResult.then(() => {})
3605+
3606+
for (const completion of await Promise.allSettled([
3607+
ignoredStream.cancel(),
3608+
pendingWrite,
3609+
])) {
3610+
if (completion.status === 'rejected') {
3611+
throw completion.reason
3612+
}
35763613
}
3577-
3578-
await ignoredStream.cancel()
35793614
}
35803615
})
3581-
.catch((error) => {
3582-
debug?.(
3583-
logPrefix,
3584-
'background cache revalidation failed for',
3585-
revalidateCacheHandlerKey,
3586-
error
3587-
)
3616+
.finally(() => {
3617+
if (
3618+
backgroundRevalidations.get(revalidateCacheHandlerKey) ===
3619+
revalidatePromise
3620+
) {
3621+
backgroundRevalidations.delete(revalidateCacheHandlerKey)
3622+
}
35883623
})
3624+
backgroundRevalidations.set(
3625+
revalidateCacheHandlerKey,
3626+
revalidatePromise
3627+
)
3628+
revalidatePromise.catch((error) => {
3629+
debug?.(
3630+
logPrefix,
3631+
'background cache revalidation failed for',
3632+
revalidateCacheHandlerKey,
3633+
error
3634+
)
3635+
})
35893636
workStore.pendingRevalidateWrites ??= []
35903637
workStore.pendingRevalidateWrites.push(revalidatePromise)
35913638
}

‎test/e2e/app-dir/app-root-params-getters/fixtures/use-cache-runtime/app/[lang]/[countryCode]/swr/page.tsx‎

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,43 @@
1+
import { cacheLife, cacheTag } from 'next/cache'
2+
import { lang, countryCode } from 'next/root-params'
3+
import { connection } from 'next/server'
4+
import { Suspense } from 'react'
5+
import { setTimeout } from 'timers/promises'
6+
7+
type Props = { searchParams: Promise<{ key?: string | string[] }> }
8+
9+
export default function Page(props: Props) {
10+
return (
11+
<Suspense fallback="Loading...">
12+
<Runtime {...props} />
13+
</Suspense>
14+
)
15+
}
16+
17+
async function Runtime({ searchParams }: Props) {
18+
await connection()
19+
const { key } = await searchParams
20+
if (typeof key !== 'string' || !key) {
21+
throw new Error('A query key is required')
22+
}
23+
const result = await getCachedData(key)
24+
return (
25+
<p>
26+
<span id="lang">{result.lang}</span>
27+
<span id="country-code">{result.countryCode}</span>
28+
<span id="value">{result.value}</span>
29+
</p>
30+
)
31+
}
32+
33+
async function getCachedData(key: string) {
34+
'use cache: remote'
35+
36+
cacheLife('days')
37+
const roots = { lang: await lang(), countryCode: await countryCode() }
38+
cacheTag(`swr:${key}:${roots.lang}:${roots.countryCode}`)
39+
console.log(`swr start ${key} ${roots.lang}/${roots.countryCode}`)
40+
await setTimeout(1000)
41+
console.log(`swr finish ${key} ${roots.lang}/${roots.countryCode}`)
42+
return { ...roots, value: new Date().toISOString() }
43+
}

‎test/e2e/app-dir/app-root-params-getters/fixtures/use-cache-runtime/app/[lang]/[countryCode]/swr/revalidate/route.ts‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
import { revalidateTag } from 'next/cache'
2+
import { NextRequest } from 'next/server'
3+
4+
export async function POST(
5+
request: NextRequest,
6+
{ params }: { params: Promise<{ lang: string; countryCode: string }> }
7+
) {
8+
const key = request.nextUrl.searchParams.get('key')
9+
if (!key) {
10+
return new Response('A query key is required', { status: 400 })
11+
}
12+
const { lang, countryCode } = await params
13+
revalidateTag(`swr:${key}:${lang}:${countryCode}`, 'minutes')
14+
return new Response(null, { status: 204 })
15+
}

0 commit comments

Comments
 (0)