fix(cache): preserve prerendered page cache tags - #709
Conversation
…ed model - Add TagRevalidationDurations interface and update CacheHandler interface - Rewrite MemoryCacheHandler to use stale/expired TagManifestEntry model - Rewrite KVCacheHandler with KVTagEntry JSON format (backward-compat with legacy plain-timestamp) - Add deprecation warning to public revalidateTag() when called without profile - SWR semantics when profile with expire>0: mark stale immediately, hard-expire after window - Hard invalidation when no profile or expire=0: set expired=now, next get() is a miss - Fix >= comparisons for same-millisecond set()+revalidateTag() correctness - Add tests for deprecation warning, SWR stale return, expire=0 hard miss, JSON KV format
commit: |
|
|
/bigbonk review |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: feat(cache): implement Next.js 16 revalidateTag two-phase stale/expired model
Thorough review of the two-phase stale/expired revalidateTag implementation. The overall architecture is sound — the TagManifestEntry shape matches Next.js exactly, the layering between MemoryCacheHandler, KVCacheHandler, and the ISR layer is clean, and the backward-compatible parseKVTagEntry() fallback for legacy plain-timestamp KV entries is well-designed.
Issues
1. >= vs > divergence from Next.js (deliberate but should be documented more prominently)
The code comments explain the rationale for >= (handle same-millisecond set()+revalidateTag()), and I confirmed that Next.js uses strict > in areTagsExpired and areTagsStale (expiredAt > timestamp, staleAt > timestamp). The vinext choice is defensively correct — it prevents a race where invalidation in the same ms as a write would be silently ignored. However, this is a behavioral divergence from Next.js that could cause subtle differences in edge cases (e.g., a set() immediately followed by revalidateTag() in the same ms will be invalidated in vinext but not in Next.js).
Per AGENTS.md: "If Next.js and vinext should behave differently (defense-in-depth, Cloudflare-specific requirements), that is OK, but it must be a deliberate, documented decision, not an accidental divergence." The inline comments explain the choice, which is good, but consider adding a brief note in the TagManifestEntry doc comment or the checkTagInvalidation JSDoc that this is an intentional divergence from Next.js's strict >.
2. revalidatePath doesn't pass durations — this is correct, but now it triggers the deprecation warning
revalidatePath calls _getActiveHandler().revalidateTag(tag) at line 507. This is correct — Next.js's revalidatePath always does hard invalidation without a profile. However, the new deprecation warning in the revalidateTag() public API function fires for any call without a profile argument. Since revalidatePath calls the handler directly (not the public revalidateTag function), this is fine — no warning is emitted. Good.
But updateTag() at line 529 also calls _getActiveHandler().revalidateTag(tag) directly, which also avoids the deprecation warning. This is also correct — updateTag is the recommended replacement for the deprecated single-arg revalidateTag. Just confirming the design is intentional. ✓
3. Missing TagRevalidationDurations export from next-shims.d.ts
The next-shims.d.ts file (which provides ambient module declarations for "next/cache") still uses the inline { expire?: number } type for CacheHandler.revalidateTag() (lines 432, 481) instead of the newly-created TagRevalidationDurations interface. While structurally equivalent, this means consumers who implement CacheHandler from the ambient types won't see the TagRevalidationDurations type name or its JSDoc. Consider exporting TagRevalidationDurations from the ambient declarations module too, and using it in those signatures for consistency.
4. MemoryCacheHandler.revalidateTag spreads existing — potential for stale stale field
In cache.ts line 367:
this.tagManifest.set(tag, {
...existing,
stale: now,
expired: now + durations.expire * 1000,
});And line 376:
this.tagManifest.set(tag, {
...existing,
stale: undefined,
expired: now,
});The spread of existing is mostly harmless since the explicit properties override it, but consider whether this is needed at all. What scenario would require preserving fields from the previous TagManifestEntry? Both branches set all fields explicitly (stale and expired), so ...existing is effectively dead code here. If a future TagManifestEntry gains new fields, spreading existing could accidentally preserve stale values from a previous invalidation cycle. I'd suggest dropping the spread entirely — just pass the literal object.
5. Test: deprecation warning test may be fragile
In shims.test.ts line 1466:
expect(warnSpy.mock.calls[0][0]).toMatch(/deprecated|second argument|max/i);This regex matches if any of those words appear. The actual warning message contains all three, so it passes, but this could match unrelated warnings. Consider tightening to check for the specific message or at least require two of the three terms. Minor nit.
6. The expired <= now check in MemoryCacheHandler.get() is always true at hard-invalidation time
In cache.ts line 274:
if (typeof expired === "number" && expired >= entry.lastModified && expired <= now) {For the no-profile case, expired is set to Date.now() in revalidateTag(). When get() runs next, expired <= now will always be true (unless the clock goes backward, which is vanishingly rare). For the SWR case, expired = now + expire * 1000, so expired <= now will only be true after the expire window elapses. This is correct behavior, but the expired <= now check is doing meaningful work only in the SWR case. The comment could be clearer about this — it currently says "the SWR window has itself elapsed" but could note that for hard-invalidation this condition is trivially satisfied.
Positives
- Clean extraction of
checkTagInvalidation()in KV handler — shared logic between the local-cache and KV-fetch paths, eliminates duplication. parseKVTagEntry()backward compat — handles both JSON{ stale?, expired? }and legacy plain-timestamp strings gracefully. The fallback to{ expired: ts }for old format correctly treats legacy entries as hard-invalidated.- Correct priority ordering — hard-expiry is checked before stale for both handlers. The KV handler does a full hard-expiry sweep across all tags before checking stale, preventing a scenario where an earlier tag's stale masks a later tag's hard-expiry.
- ISR layer agnosticism — confirmed that
isrGet()treatscacheState: "stale"uniformly regardless of source (time-based vs tag-based). No ISR changes needed. - All
revalidateTagcallers accounted for —revalidatePathandupdateTagcorrectly do hard invalidation (no durations), only the publicrevalidateTag()resolves and passes durations. - Good test coverage — deprecation warning, SWR stale, expire=0 hard miss, no-profile hard miss, KV JSON format, KV SWR lifecycle, legacy backward compat all tested.
|
Review posted on PR #709. Here's a summary of the findings: Overall: The implementation is architecturally sound. The Key issues raised:
Positives: Clean |
- Document >= vs > as deliberate divergence from Next.js with rationale (same-millisecond set+revalidateTag must invalidate; strict > would allow stale serves when both events share a timestamp) - Export TagRevalidationDurations interface from next-shims.d.ts and use it in CacheHandler, MemoryCacheHandler, and revalidateTag signatures - Remove dead ...existing spread in MemoryCacheHandler.revalidateTag — both branches fully overwrite the TagManifestEntry, the spread could accidentally preserve stale fields if the interface grows - Tighten deprecation warning test regex to match the exact emitted message
|
/bigbonk review |
There was a problem hiding this comment.
Review: feat(cache): implement Next.js 16 revalidateTag two-phase stale/expired model
Good implementation overall. The architecture cleanly mirrors the Next.js TagManifestEntry shape, the two-phase model is correct, backward compat is handled, and the test coverage is solid. A few issues follow, one of which is a correctness divergence from Next.js.
Divergence: MemoryCacheHandler.revalidateTag when durations is provided without expire
In Next.js's default.ts handler (createDefaultCacheHandler), the updateTags method with durations always sets stale = now, regardless of whether expire is defined:
// Next.js packages/next/src/server/lib/cache-handlers/default.ts:187-198
if (durations) {
const updates: TagManifestEntry = { ...existingEntry }
updates.stale = now
if (durations.expire !== undefined) {
updates.expired = now + durations.expire * 1000
}
tagsManifest.set(tag, updates)
}But vinext's MemoryCacheHandler.revalidateTag only enters the SWR branch when durations.expire > 0:
if (durations && durations.expire !== undefined && durations.expire > 0) {
// SWR branch — sets stale + expired
} else {
// Hard invalidation — only sets expired
}This means if someone calls handler.revalidateTag("tag", {}) (durations present but no expire field), Next.js would set stale = now (making the entry stale), while vinext would hard-expire it. Same for { expire: undefined }.
The fix is straightforward — match Next.js's logic: if durations is truthy, always set stale = now. The expired field only gets set when expire is defined on the durations object. The expire: 0 case also differs: Next.js treats { expire: 0 } as setting expired = now + 0 = now AND stale = now, while vinext routes it to the hard-invalidation branch. The functional outcome is the same (entry is both stale and hard-expired, so the hard-expiry check fires immediately), but the stored manifest entry differs.
The same divergence exists in KVCacheHandler.revalidateTag.
This may be intentional (the current behavior is arguably more correct for the API surface vinext actually exposes), but per AGENTS.md, behavioral divergences from Next.js should be deliberate and documented. Worth confirming whether revalidateTag("tag", {}) is a realistic call path or purely theoretical.
areTagsExpired comparison logic check
The >= vs > divergence is well-documented in comments. I verified against the Next.js source:
Next.js (tags-manifest.external.ts):
const isImmediatelyExpired = expiredAt <= now && expiredAt > timestampvinext:
if (typeof expired === "number" && expired >= entry.lastModified && expired <= now)These are logically equivalent with the >= being the deliberate divergence (handling same-millisecond writes). The documentation in both cache.ts and kv-cache-handler.ts clearly explains the rationale. This is fine.
Positive observations
checkTagInvalidation()in KV handler — clean extraction, correct priority (hard-expiry before stale sweep across ALL tags), eliminates duplication between local-cache and KV-fetch paths.parseKVTagEntry()backward compat — graceful handling of both JSON{ stale?, expired? }and legacy plain-timestamp strings. The fallback mapping to{ expired: ts }for old format correctly treats legacy entries as hard-invalidated.- Two-loop tag-fetch pattern in KV handler — populating local cache for ALL fetched tags before checking invalidation avoids wasted KV reads on early returns. Well designed.
- ISR layer requires zero changes — the
cacheState: "stale"abstraction cleanly slots into the existing ISR SWR logic regardless of whether staleness comes from time-based or tag-based invalidation. - Comprehensive test coverage — deprecation warning, SWR stale, expire=0 hard miss, no-profile hard miss, KV JSON format, KV SWR lifecycle, legacy backward compat all tested.
- Deprecation warning test — despite ask-bonk's flag, the actual test uses a specific substring match (
/"revalidateTag" without the second argument is now deprecated/), not the loose regex described. The test is fine.
Summary
The main item worth discussing is the durations handling divergence: when durations is truthy but expire is undefined or zero, the branching logic differs from Next.js. Whether this matters depends on whether that call path is realistic. Everything else is well-implemented with good documentation of deliberate divergences.
|
|
||
| for (const tag of tagList) { | ||
| this.tagRevalidatedAt.set(tag, now); | ||
| if (durations && durations.expire !== undefined && durations.expire > 0) { |
There was a problem hiding this comment.
Divergence from Next.js: In Next.js default.ts:187-198, when durations is truthy, stale = now is always set regardless of expire. The expired field is only conditionally set when durations.expire !== undefined. Here, the condition durations.expire !== undefined && durations.expire > 0 means:
revalidateTag("tag", {})→ hard invalidation in vinext, stale-only in Next.jsrevalidateTag("tag", { expire: 0 })→ hard invalidation in vinext, stale+expired in Next.js
The functional outcome for expire: 0 is the same (hard miss), but the stored manifest differs. For {} (no expire), the behavior genuinely diverges. Worth a documenting comment if intentional.
| // - Profile with expire (SWR): { stale: now, expired: now + expire * 1000 } | ||
| // Entries are served stale until `expired` is reached, then become a hard miss. | ||
| let tagEntry: KVTagEntry; | ||
| if (durations && durations.expire !== undefined && durations.expire > 0) { |
There was a problem hiding this comment.
Same divergence as MemoryCacheHandler: Next.js's updateTags always sets stale = now when durations is truthy, then conditionally sets expired only when durations.expire !== undefined. The expire > 0 guard here routes { expire: 0 } and {} to the hard-invalidation branch instead.
|
Review posted on PR #709. Here's a summary of what I found: Overall: The implementation is well-structured — the Key finding — divergence from Next.js's The main issue I identified (with inline comments on both
For Confirmed from ask-bonk's review:
|
…ut expire
When revalidateTag is called with a truthy durations object, stale is now
always written to the tag manifest regardless of whether expire is set.
The expired field is only set when durations.expire !== undefined.
This fixes two divergences from Next.js default.ts updateTags:
- revalidateTag('tag', {}) → { stale: now } (stale-only SWR), was { expired: now }
- revalidateTag('tag', { expire: 0 }) → { stale: now, expired: now }, was { expired: now }
Applies the same fix to both MemoryCacheHandler and KVCacheHandler.
Adds tests covering the new stale-only ({}) and expire=0 shapes.
|
/bigbonk review |
There was a problem hiding this comment.
Review: feat(cache): implement Next.js 16 revalidateTag two-phase stale/expired model
All tests pass (822/822). The implementation is correct and well-structured.
Verification of prior review findings
ask-bonk's durations branching divergence (the main issue) — fixed in cc43b17. The final commit correctly matches Next.js's updateTags logic: when durations is truthy, stale = now is always set, and expired is only conditionally set when durations.expire !== undefined. Both MemoryCacheHandler and KVCacheHandler are consistent. The edge cases are:
revalidateTag("tag", {})→{ stale: now }— SWR with no hard expiry ✓revalidateTag("tag", { expire: N })→{ stale: now, expired: now + N*1000 }✓revalidateTag("tag", { expire: 0 })→{ stale: now, expired: now }— immediate hard miss ✓revalidateTag("tag")(no durations) →{ expired: now }— hard invalidation ✓
New tests cover all four cases in both handlers.
>= vs > divergence — acceptable and well-documented. The inline comments in both cache.ts:264-270 and kv-cache-handler.ts:583-590 clearly explain the rationale. This is a defense-in-depth divergence that prevents same-millisecond stale-serve bugs. No action needed.
next-shims.d.ts — addressed. The ambient "next/cache" module declaration now exports TagRevalidationDurations as a named interface (line 432) and uses it in the CacheHandler.revalidateTag signature (line 444) and the revalidateTag function signature (line 501). Consumers implementing CacheHandler from ambient types will see the named type.
Code quality observations
-
checkTagInvalidation()extraction in KV handler — clean shared logic between local-cache and KV-fetch paths. Correct priority ordering: hard-expiry swept across ALL tags before stale is checked. -
Two-loop tag-fetch pattern (kv-cache-handler.ts:263-277) — populates local cache for ALL fetched tags before checking invalidation, avoiding wasted KV reads on early returns. Well designed.
-
parseKVTagEntry()backward compat — handles JSON{ stale?, expired? }and legacy plain-timestamp strings gracefully. The fallback mapping to{ expired: ts }for old format correctly treats legacy entries as hard-invalidated. Test at line 572 covers this. -
revalidatePathandupdateTagcorrectly bypass deprecation warning — both call_getActiveHandler().revalidateTag(tag)directly (not the publicrevalidateTagfunction), so no spurious warnings. -
ISR layer requires zero changes —
cacheState: "stale"from tag-based SWR slots cleanly into existing ISR SWR logic.
One minor observation (non-blocking)
In MemoryCacheHandler.get() at line 306, there's a second Date.now() call for time-based expiry that could reuse the now variable from line 271. This is cosmetic — the two timestamps will differ by microseconds at most — but reusing now would be slightly more consistent. Not worth a fixup commit.
LGTM. Clean implementation with comprehensive test coverage.
|
Review posted on PR #709 with approval. Summary:
|
|
Worker E note for deploy-suite run 28478866791 / job 84413308650: I think this PR covers the tag-invalidation/updateTag side of the cache/revalidation failures, including the |
|
Run 28478866791 / job 84413308650 mapping for the remaining
This PR is the exact existing fix candidate for that row: it implements the Next.js 16 stale/expired tag manifest model and, most importantly for this assertion, maps no-profile Validation done for this mapping: inspected the original deploy-suite report artifact and the failing assertion text, and checked this PR diff touches the relevant Memory/KV cache handler tag invalidation paths plus focused cache tests. Validation still needed before counting the row closed: refresh/rebase this PR onto current REPO="$(pwd)" \
NEXTJS_DIR="/Users/jamesanderson/Developer/vinext/.nextjs-ref" \
NEXT_TEST_CONCURRENCY=1 \
./scripts/run-targeted-nextjs-e2e.sh test/e2e/app-dir/app-static/app-static.test.ts |
|
Run 29871622126 / job 88775791401 still fails |
# Conflicts: # packages/cloudflare/src/cache/kv-data-adapter.runtime.ts # packages/vinext/src/shims/cache.ts # packages/vinext/src/shims/next-shims.d.ts # tests/shims.test.ts
|
Refreshed this PR onto current The remaining Exact Next.js v16.2.6 proof at this head: CI=true REPO="$(pwd)" \
NEXTJS_DIR="/private/tmp/nextjs-metadata-streaming-ref" \
./scripts/run-targeted-nextjs-e2e.sh \
test/e2e/app-dir/app-static/app-static.test.tsResult:
Local regression proof:
The stale auto-merged tests for the old KV JSON tag-manifest architecture were removed during the main refresh; the net PR diff now contains only the prerender tag propagation and its regression coverage. |
Performance benchmarksCompared 0 improved · 0 regressed · 6 within ±1.5%
View detailed results and traces 🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: fix(cache): preserve prerendered page cache tags
Note: the PR title/description describe the older KV JSON tag-manifest work, but the current diff (after the refresh onto main, head eca828a) is entirely different — it propagates user cache tags collected during App Router prerender through to the seeded page cache artifact. Reviewed against the actual diff.
Summary
The change is well-scoped and correct. It closes a real gap: prerendered App Router pages previously lost the user tags collected by unstable_cache/cacheTag before vinext-prerender.json was written, so revalidateTag("some-user-tag") could invalidate nested data-cache entries but never the prerendered page artifact itself. The fix carries collected tags through the same x-next-cache-tags static-generation side channel Next.js uses, persists them in the prerender manifest, and attaches them to both Node memory-cache and Cloudflare KV prerender seeds.
I verified the full flow end-to-end and it holds together:
getPageTags()→buildAppPageTags(cleanPathname, getCollectedFetchTags(), routeSegments)(app-page-dispatch.ts:1097) returns ASCII-encoded tags (both implicit_N_T_/…path tags and user tags).- Header emit is correctly gated on
options.isPrerender === true, andisPrerenderisprocess.env.VINEXT_PRERENDER === "1"(app-page-dispatch.ts:632) — build-time only. Sox-next-cache-tagsis never sent to real clients at runtime; it's purely the build-time capture channel, matching Next.js. readPrerenderCacheTagsHeader(prerender.ts:1731) reads/dedups → manifesttags(prerender-manifest.ts) →seedMemoryCacheFromPrerenderandbuildPrerenderKVPairsboth passroute.tags ?? []intobuildAppPageCacheTags, which dedups against the implicit tags it regenerates (app-page-cache.ts:147-151), so the redundant implicit tags in the manifest are harmless.
Correctness checks
Header name matches Next.js. x-next-cache-tags matches NEXT_CACHE_TAGS_HEADER in packages/types/next/upstream/dist/lib/constants.d.ts:20. Good.
Tag timing during prerender. The x-next-cache-tags header is set at response-build time, before the returned safeHtmlStream body is drained. This would be a bug if fetch/unstable_cache tags accumulated only during stream consumption. It's safe here because the prerender path awaits htmlRender.metadataReady (app-page-render.ts:1011-1013) and settleCapturedRscRenderForCacheMetadata(htmlRender.capturedRscData) (shouldReadRequestCacheLifeForPrerender is isPrerender === true, line 758) before building the response, so getCollectedFetchTags() is fully populated. The real App Router build+prerender fixture test (tests/prerender.test.ts "records collected App Router cache tags") passes and confirms unstable-data reaches the manifest, which is the authoritative proof.
Comma delimiting. Tags are joined/split on , with no escaping of commas within a tag. This matches Next.js (it uses the same comma-delimited x-next-cache-tags header), so it's consistent, not a divergence — worth being aware of but not a blocker.
One parity observation (non-blocking, likely out of scope)
packages/cloudflare/src/tpr.ts:935 still uses buildAppPageCacheTags(routePath, []) with the exact "Path-derived implicit tags … #1486" comment that the other two seed sites just moved past. TPR seeds from runtime responses (isPrerender false), so x-next-cache-tags isn't present there and user tags genuinely aren't available on that path — so this isn't a regression from this PR. But it does mean TPR-seeded entries still can't be invalidated by user tags, so the #1486 gap this PR closes for build prerender remains open for TPR. Worth a follow-up issue or a note that the divergence is intentional.
Tests
Good coverage across the stack:
tests/app-page-render.test.ts— asserts thex-next-cache-tagsheader is emitted with both implicit and user tags on the prerender path.tests/prerender.test.ts— real build/prerender fixture provesunstable_cachetags reach the route result andvinext-prerender.json.tests/prerender-kv-populate.test.ts— manifesttagsflow into KV metadata.tests/seed-cache.test.ts— full loop: manifesttags→ seeded entry →revalidateTag("test-update-tag")invalidates both html and rsc keys.
All 89 targeted unit tests + the prerender fixture test pass locally.
Verdict
LGTM. Correct, minimal, well-tested fix with proper dev/build parity. The only follow-up worth tracking is the TPR (tpr.ts:935) tag gap, which is out of scope for this PR.
|
Review posted on PR #709. SummaryThe PR title/body describe the old KV JSON tag-manifest work, but after the refresh onto Verdict: LGTM. The fix is correct, minimal, and well-tested. What it does: Closes a real gap — prerendered pages lost the user tags from Correctness verified:
One non-blocking observation: All 89 targeted unit tests plus the real prerender fixture test pass locally. |
james-elicx
changed the title
1 participant
Summary
vinext-prerender.jsonupdateTag/revalidateTagfrom server actions refreshes the pageRoot cause
Prerendered page artifacts were seeded without the user cache tags collected during render. Later tag invalidation therefore had no tag-to-page association to expire. The fix carries Next-compatible
x-next-cache-tagsmetadata through prerendering and both seed paths.Validation
app-static.test.ts: ownedupdateTagandrevalidateTag(..., "max")rows passThe remaining
useSearchParamsapp-static failure belongs to #2243. Cache Components /use cachecoverage remains intentionally deferred.