fix(cache): resume cached fallback shells - #2853
Conversation
|
/bigbonk review for issues |
commit: |
|
|
/bigbonk review for issues |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: fix(cache): resume cached fallback shells
Reviewed the full 41-file diff (commits 3357bd2..HEAD). Ran vp check on the core files (clean) and the affected focused test suites. Most of the change is coherent, but I found two concrete issues — one blocking (a flaky test caused by a real ordering regression in the refactored cachedFn), and one behavioral regression worth confirming.
🔴 Blocking: cache-handler-resume.test.ts is flaky — refactor of cachedFn deferred the cache-task registration behind awaits
The new test resume data cache handler > leaves explicit revalidate zero use-cache results as dynamic fallback holes (tests/cache-handler-resume.test.ts:205, the revalidate zero parametrization) fails intermittently. In isolation it always passes, but when run in the same process as sibling cache tests it fails ~40% of the time:
# these all pass alone, but together:
npx vitest run tests/prerender-data-cache.test.ts tests/cache-handler-resume.test.ts tests/app-page-cache.test.ts
# run 1: FAIL run 2: pass run 3: pass run 4: FAIL run 5: FAIL (repeat of identical command)
AssertionError: expected false to be true // tests/cache-handler-resume.test.ts:224
expect(state.hasDynamicBoundary).toBe(true);
Since the full Vitest suite runs all files serially in one process, this will cause intermittent red CI on main.
Root cause — the refactor in cache-runtime.ts changed the wrapper from
// before: task is registered synchronously at call time
const cachedFn = (...args) =>
trackPprFallbackShellCacheTask(async () => { ... }, cacheVariant);to
// after: several awaits (getRscModule, decryptCaptures, encodeReply...) run
// BEFORE trackPprFallbackShellCacheTask is ever called
const cachedFn = async (...args) => {
const rsc = await getRscModule();
...
const captures = options.decryptCaptures ? await options.decryptCaptures(...) : undefined;
...
const encoded = await encodeInput(); // encode the key
...
return trackPprFallbackShellCacheTask(async () => { ... }, cacheVariant); // increments pendingCacheTasks
};trackPprFallbackShellCacheTask is what increments state.pendingCacheTasks. The test does:
const pending = runWithPprFallbackShellState(state, () => cached());
await waitForPprFallbackShellCacheReady(state);
expect(state.hasDynamicBoundary).toBe(true);waitForPprFallbackShellCacheReady resolves once pendingCacheTasks === 0 (via a scheduleAfterTask = 2×setTimeout(0)). Because the task is now only registered after a variable number of await turns, there's a window where pendingCacheTasks is still 0 when the waiter's timers fire, so it resolves before the dynamic boundary is marked. The number of pre-registration turns depends on whether getRscModule()'s memoized _rscModule (module-level, cache-runtime.ts:265) has resolved — which leaks across test files — hence the order/timing dependence.
Fix options:
- Register the cache task synchronously at call entry again (keep the
trackPprFallbackShellCacheTaskwrapper around the whole async body, including key encoding), sopendingCacheTasksis incremented before anyawaityields; or - Have
waitForPprFallbackShellCacheReadyaccount for an in-flight-but-not-yet-registered task for the current scope.
The first option restores the pre-refactor invariant and is the safer fix. Either way this needs to be deterministic before merge — please also run the trio above a few times to confirm.
🟠 Behavioral regression: custom CacheHandlers without a seed() method no longer receive persisted resume data
seedPrerenderDataCache (prerender-data-cache.ts:396) dropped the else branch that the stacked base PR (#2849, 3357bd2) added specifically for handlers predating the seed() hook:
// removed in this PR:
} else {
// Existing custom handlers predate the timestamp-preserving seed hook.
await handler.set(entry.key, entry.value, { ...entry.context, lastModified: entry.lastModified });
seeded++;
}Now, since seed? is optional on CacheHandler, any custom handler without a seed() method silently gets zero persisted FETCH entries seeded at startup — the loop skips it entirely. The base PR's test "falls back to set for custom handlers without timestamp-preserving seed support" (which asserted resolves.toBe(1) and set called) was also removed, and replaced by "does not publish cache entries from a discarded speculative render" (:168) whose set-not-called assertion only exercises the speculative-skip path (the sole entry is speculative), not the general no-seed case.
Note ResumeDataCacheHandler.seed (cache-handler.ts:682) does fall back to delegate.set(...) for delegates lacking seed, so the omission in seedPrerenderDataCache looks unintentional rather than a deliberate policy change. Please either restore the set fallback (and its test) or, if this is intentional, document why no-seed custom handlers are no longer seeded and confirm the default/Cloudflare handlers always implement seed.
✅ Looked fine
app-ssr-entry.tsdev/prod renderer loading (resume()+prerender(),isReactDevelopmentRuntimeviacaptureOwnerStack), postponed-state versioning with legacy fallback — reasonable.NEXT_PHASE = "phase-production-build"during prerender matches Next.js; restore logic in thefinallyis correct.prerender-data-cache.tsspeculative/normal isolation (separatespeculativeMemory,persistEntryprecedence,buildPrerenderKVPairs/seedPrerenderDataCacheskipping speculative entries) is consistent.- App-layout cache-key handling (
app-layout-props-cache-key.ts, temporary-reference decode/encode for slot holes, private-cache + non-RSC direct-execution guards) is well-guarded with fallbacks. otel-tracer-extension.tsnow importingisUseCacheFunctionfrom the new lightweightuse-cache-function.tspreserves the "don't pull in heavy cache-runtime" intent.decorateTextStreamprefix/suffix + reader-release handling is correct.- Other affected focused suites (
app-ppr-fallback-shell,ppr-fallback-shell,thenable-params,app-page-render,app-bfcache-resume-identity,app-page-element-builder,use-cache-transform,prerender,prerender-kv-populate) — 322/322 passed.
Performance benchmarksCompared 0 improved · 2 regressed · 4 within ±1.5%
View detailed results and traces 🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head |
|
Review complete and posted to PR #2853 with CHANGES_REQUESTED. Summary of findings: 🔴 Blocking — flaky test from a real ordering regression
🟠 Behavioral regression — worth confirming
✅ Verified soundDev/prod renderer parity in The push/PR handling is automatic per the GitHub Action context — no manual steps needed. |
|
Mapping note for the cache-components backlog from Actions run |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Re-review: fix(cache): resume cached fallback shells
Re-reviewed after the two follow-up commits (8336aa6 "warm fallback params before postponing", 719486b "preserve fallback cache readiness"). Both prior findings are now addressed. Ran vp check on all 27 changed source files (clean) and re-ran the previously-flaky test trio plus the affected focused suites.
✅ Blocking flaky test — resolved
The cachedFn refactor no longer defers cache-task registration behind awaits. 719486b reintroduces a synchronous reservation via trackPprFallbackShellCacheTaskUntil (ppr-fallback-shell.ts:236): pendingCacheTasks is now incremented at call entry, and an explicit release() handoff completes the setup reservation only after module load + capture decryption — with the execution-phase task registered by the nested trackPprFallbackShellCacheTask. Deferring the readiness release across the key-encoding phase is deliberate and matches Next.js (tasky args may never settle), and completeCacheTask is idempotent (ppr-fallback-shell.ts:131 guards on isPending), so the release() + .finally() double-call is safe.
Verified deterministic — ran the exact trio from the prior review 5× consecutively:
npx vitest run tests/prerender-data-cache.test.ts tests/cache-handler-resume.test.ts tests/app-page-cache.test.ts
# 5/5: Test Files 3 passed, Tests 85 passed
No intermittent failure reproduced (was ~40% before).
✅ Custom-handler seeding regression — resolved (better than a plain set restore)
Rather than restoring the naive set() fallback (which — as the prior review noted — could resurrect stale build data across processes for shared handlers), 719486b introduces PrerenderDataCacheRuntimeOverlay (prerender-data-cache.ts:406), a process-local read-through view for handlers lacking seed():
- Build snapshots are held in a private
MemoryCacheHandlerand are never written into the delegate — no cross-process stale resurrection. - Runtime data stays authoritative:
getreturns the delegate value first, and once a runtimesetbumpsruntimeVersionsfor a key (> 0) the snapshot is permanently shadowed (:440). - The write-during-read race is handled with a version counter +
pendingWritesretry loop (:437,:443,:449), and a snapshot hit releases any delegate single-flight claim viareleasePendingSet(:448). - Adapters exposing timestamp-aware
seed()keep the direct-seed path (:491).
New tests cover read-through exposure without writes, runtime-freshness precedence, the concurrent write race, and tag invalidation (prerender-data-cache.test.ts:494+). The ResumeDataCacheHandler.seed no-seed-delegate branch now returns false instead of blind set (cache-handler.ts:690), consistent with the compare-and-install contract documented on the seed? hook (cache-handler.ts:105).
Ordering was also hardened: __registerConfiguredCacheAdapters() now runs before seeding in the generated entry (app-rsc-entry.ts:679), guarded by a codegen ordering assertion (app-router-isr-codegen.test.ts:204), so a configured data adapter is installed before the overlay wraps getDataCacheHandler().
Dev/prod & runtime parity checked:
- The
pprResume-as-dynamic fix (app-page-render.ts:1168— a resumed fallback shell is treated as inherently dynamic so the completed HTML/RSC pair is never admitted to ISR) lives inrenderAppPageLifecycle, the single path reached fromapp-page-dispatch.ts:1102in both dev and prod. No divergent prod copy. - The overlay path is Node/generic only:
seedMemoryCacheFromPrerenderis disk-based (fs) and does not run on Workers. Cloudflare KV is populated at deploy time viabuildPrerenderKVPairs, which this PR also fixes to skip speculative entries and carrypostponed(prerender-kv-populate.ts:113,:177). No Workers regression.
✅ Also verified
beginPprFallbackShellCacheWarmup/isCacheWarmupActivegating inthenable-params.ts:225,285correctly resolves fallback params during the warmup pass (so nesteduse cachework can start and warm) while keeping them hanging in the real fallback shell render.hasReactPostponedReplayNodes→ sharedhasAppPprPostponedReplayNodes(app-ppr-fallback-shell.ts:48) is a clean extraction;app-ssr-entry.ts:733uses it identically.prerender.tsskip-artifact comment/logic now correctly allows replay-node-only shells (prerender.ts:1550).- Affected focused suites:
app-page-render,app-ppr-fallback-shell,ppr-fallback-shell,thenable-params,prerender,prerender-kv-populate,app-router-isr-codegen,app-page-cache,app-bfcache-resume-identity— all green (locally re-run). vp checkon all 27 changed source files: format, lint, and types clean.
Minor note (non-blocking)
On Node/generic deployments using a custom handler without seed(), the overlay loads all persisted FETCH entries into a process-local MemoryCacheHandler at seed time. For apps with a very large prerender data cache this adds resident memory proportional to the snapshot. It's a fallback-only path (adapters with seed(), incl. any timestamp-aware backend, avoid it) and is correct — just worth a comment or a follow-up if large-snapshot custom handlers become common. No action required for this PR.
Prior blocking + behavioral findings are both resolved. Looks good to merge once CI is green.
|
Review posted to PR #2853 with APPROVE. SummaryThe prior review's two findings are both resolved by the two follow-up commits ( ✅ Blocking flaky test — resolvedThe ✅ Custom-handler seeding regression — resolved (better than a plain
|
|
/bigbonk review for issues |
There was a problem hiding this comment.
Re-review: fix(cache): resume cached fallback shells
Reviewed the one new commit since the prior APPROVE — fc58990 "coordinate fallback cache fills" (the only delta over 719486b). Verified the two prior findings stay resolved, ran the previously-flaky trio 5×, ran the affected focused suites, and ran vp check on the two changed source files.
Scope of the new commit
app-ppr-fallback-shell.ts—parseAppPprPostponedStatenow throws on a malformed resume envelope instead of silently treating the raw value aspostponed.cache-handler.ts—PprFallbackShellDataCacheHandlergains apendingLocalFillssingle-flight gate acrossget/set/seed/releasePendingSet.- Tests added in
cache-handler-resume.test.ts,app-page-cache.test.ts,app-page-dispatch.test.ts.
✅ parseAppPprPostponedState throw-on-malformed — sound
Both callers (readAppPageFallbackShellCache app-page-cache.ts:645, readAppPageFallbackShellCacheResume :700) already wrap the call in try/catch that logs console.error and return null (a cache miss). So a corrupt/truncated cache entry now degrades to a MISS instead of serving corrupt shell HTML — a strict improvement.
Confirmed the throw can't fire on legitimate production data:
- The stored value is always produced by
serializeAppPprPostponedState(app-page-render.ts:1119), which isPREFIX + JSON.stringify({postponed, resumeDataCache})— always prefixed and parseable. parsed.postponedis itselfserializeSsrPostponedState(...)=JSON.stringify(...)(app-ssr-entry.ts:138), so the innerJSON.parse(parsed.postponed)(:100) always succeeds.- The non-enveloped branch's new
JSON.parse(value)(:89) only guards legacy/unenveloped values, which are also JSON. Empty-string postponed never reaches parse — both callers guard!cachedValue.postponedfirst.
New parametrized test (app-page-cache.test.ts:1008) covers all four malformed shapes → asserts null + the logged error. Good.
✅ PprFallbackShellDataCacheHandler single-flight gate — correct
The for(;;) in get is the right shape: local hit → return; pending fill → await then retry (re-checks local.get with the waiter's own context/softTags, so tag invalidation during the wait is honored); else claim via beginLocalFill and query the delegate.
Verified the tricky invariants:
- No double-claim race: there is no
awaitbetweenpendingLocalFills.get(key)(:566) andbeginLocalFill(:572), so within a single microtask turn only onegetcan claim per key.beginLocalFill's overwrite can't orphan a live gate. - Read-error ownership retention (
:573-575): a throwingdelegate.getpropagates while leaving the gate owned; the owner recovers via a latersetthat resolves siblings. Covered by "keeps fill ownership after a delegate read error". nulldelegate result keeps the single-flight claim (:576-579) — matches the build-only-handler claim-in-getcontract; released byset/seed/releasePendingSet.ownsDelegateClaimcapture inset/seed(:602,:624) correctly scopes the delegatereleasePendingSetto the owner, and the nestedfinallyalways runscompleteLocalFill.releasePendingSetno-ops when it doesn't own the key (:642).
New tests exercise: concurrent single-flight fill, read-error ownership handoff, atomic seed completion without releasing a direct write, and waiter-context re-check on tag invalidation.
✅ Prior findings still resolved
- Flaky
revalidate zerotest: re-ran the exact prior trioprerender-data-cache + cache-handler-resume + app-page-cache5× — 93/93 every time, no intermittency (was ~40% fail). The synchronoustrackPprFallbackShellCacheTaskUntilreservation from719486bis unaffected by this commit. - Custom-handler seeding overlay (
PrerenderDataCacheRuntimeOverlay) — unchanged here, still correct.
Verification
vp checkonapp-ppr-fallback-shell.ts+cache-handler.ts: format/lint/types clean.- Focused suites green:
cache-handler-resume(17),app-page-cache(49),app-page-dispatch(69),app-ppr-fallback-shell(11),ppr-fallback-shell(18),prerender-data-cache(27),prerender-kv-populate(10).
Minor note (non-blocking, carried over)
The prior review's note about the overlay loading all persisted FETCH entries into process memory for custom no-seed() handlers still applies — worth a follow-up comment for very large snapshots. No action needed for this PR.
No new findings. LGTM once CI is green.
|
Review posted to PR #2853 with APPROVE. SummaryThe only change since the prior APPROVE ( New commit reviewed — both changes sound1. 2. Prior findings still resolved
Verification
One non-blocking note carried over (overlay memory footprint for large custom-handler snapshots). No new findings. |
…o codex/fix-fallback-shells-cached-io # Conflicts: # packages/vinext/src/build/run-prerender.ts
1 participant
Summary
generateStaticParamsfallback generationThis is stacked on #2849 because fallback replay consumes the prerender resume-data persistence introduced there.
Next.js parity
Source run 31290819291 had 7 failures in:
test/e2e/app-dir/fallback-shells/fallback-shells.test.tsfallback-shells > with cached IOExact targeted wrapper result on this candidate:
Command:
Validation
git diff --check: clean