fix(cache): tag ISR/TPR-seeded entries with the live-render tag set#2061
fix(cache): tag ISR/TPR-seeded entries with the live-render tag set#2061Divkix wants to merge 4 commits into
Conversation
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.
Reviewed the #1984 fix. The core mechanism is sound: persisting routeSegments into the prerender manifest and routing seed-cache through buildAppPageTags(pathname, [], routeSegments) correctly produces the bracketed pattern tags the live render emits, and the round-trip encoding for the TPR header (encodeCacheTag -> encodeURIComponent/comma-join -> decodeURIComponent -> encodeCacheTag) is lossless and comma-safe. Tag-order assertions in the new tests match the builder output. Nice work on the test coverage.
Three things worth addressing before merge, plus a couple of nits:
-
(Moderate) Info disclosure: the
x-vinext-implicit-tagsgate is a spoofable User-Agent with no outbound strip.dispatchAppPageis the shared App Router dispatch used in production (incl. Cloudflare Workers viaapp-rsc-entry), andfilterInternalHeadersonly strips inbound request headers — there is no outbound strip for this response header. So a public client that sendsUser-Agent: vinext-tpr/1.0to a deployed Worker will receivex-vinext-implicit-tags, leaking internal route topology (route-group names like(main), dynamic segment patterns, full layout hierarchy). This is information that didn't leak before. Consider gating on a per-build secret token header instead of a guessable UA, and/or stripping response-only internal headers on the public response path. -
(Minor) Root-route fallback re-introduces the exact divergence this PR removes. For
/the manifest omitsrouteSegments(empty), so seed-cache takes thebuildAppPageCacheTags('/', [])fallback, which yields["/", "_N_T_/", "_N_T_/layout", "_N_T_/page"]. But the live render passesrouteSegments: []tobuildAppPageTags('/', fetchTags, []), which additionally emits_N_T_/index. So the seeded root entry's tag set is not byte-identical to the live render's — the_N_T_/indextag is missing.revalidatePath('/')still works (both carry_N_T_/), so impact is narrow, but the comment's 'byte-identical for static routes' claim is inaccurate for the root route specifically. -
(Minor/edge) Fresh-render TPR header is computed before the stream drains.
decorateInternalImplicitTagscallsgetCollectedFetchTags()synchronously whenrenderAppPageLifecyclereturns, but the live ISR cache write computesgetPageTags()inside the post-draincachePromisecontinuation (seeapp-page-cache.ts). Any fetch tags collected while the HTML/RSC stream drains will be in the stored ISR entry but missing from the TPR header, so the 'EXACT set live ISR stores' claim doesn't hold on the fresh-render path for pages that collect tags during streaming. The HIT path is correct (no render runs). Likely rare for TPR (mostly HITs), but worth a comment acknowledging the gap or capturing tags post-drain.
Nits: the instrumentation.test.ts cleanup-retry change is unrelated to #1984 and would be cleaner as its own commit/PR (it's already a separate commit, so just noting the scope). Otherwise looks good.
| // fetches run so getCollectedFetchTags() is empty, matching the seeded entry's | ||
| // own path/route tag set. | ||
| const decorateInternalImplicitTags = (response: Response): Response => { | ||
| if (options.request.headers.get("user-agent") !== VINEXT_TPR_USER_AGENT) { |
There was a problem hiding this comment.
Spoofable gate + no outbound strip. This UA check is the only thing preventing x-vinext-implicit-tags from reaching real clients, but User-Agent: vinext-tpr/1.0 is trivially forgeable, and filterInternalHeaders only strips inbound request headers — nothing strips this header from the outbound public response. Since dispatchAppPage serves production traffic (incl. Cloudflare Workers), a public request with this UA receives the page's full implicit-tag set, leaking internal route topology (route-group names, dynamic segment patterns, layout hierarchy). Prefer a per-build secret token over a guessable UA, and/or strip response-only internal headers on the public path.
| } | ||
| const tags = buildAppPageTags( | ||
| options.cleanPathname, | ||
| getCollectedFetchTags(), |
There was a problem hiding this comment.
Header is fixed before the stream drains. getCollectedFetchTags() here runs synchronously when the lifecycle returns, but the live ISR write computes getPageTags() inside the post-drain cachePromise continuation. Fetch/cacheTag() tags collected while the stream drains will land in the stored ISR entry but not in this header, so on the fresh-render path the header is not always the 'EXACT set live ISR stores' (the comment above). The HIT path is fine. Consider capturing tags after the body drains, or softening the comment to note the fresh-render gap.
| // (non-crashing) degrade for dynamic ones. | ||
| const tags = route.routeSegments | ||
| ? buildAppPageTags(cachePathname, [], route.routeSegments) | ||
| : buildAppPageCacheTags(cachePathname, []); |
There was a problem hiding this comment.
Fallback is not byte-identical for the root route. The comment says the concrete-path fallback is 'byte-identical for static routes', but / (which always hits this branch since its routeSegments are empty) diverges: the live render does buildAppPageTags('/', fetchTags, []), which emits _N_T_/index, whereas buildAppPageCacheTags('/', []) does not. So the seeded root entry is missing the _N_T_/index tag that the live render stores. Basic revalidatePath('/') still works (both carry _N_T_/), so impact is narrow, but the claim is inaccurate for the root route — either special-case / to use buildAppPageTags(cachePathname, [], []), or adjust the comment.
|
All comments posted successfully. Here's a summary of my review of PR #2061. Review SummaryThe core fix is sound: persisting
Issues raised (1 review + 3 inline comments)
No blocking correctness bugs in the happy path; the security item (#1) is the one I'd most want addressed before merge. |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Second-pass review. The core fix is correct — I traced the seed/TPR tag derivation and confirm persisting routeSegments into the manifest and routing seed-cache through buildAppPageTags(pathname, [], routeSegments) produces the bracketed pattern tags (_N_T_/posts/[slug]/page) that the live render emits, so typed revalidatePath('/posts/[slug]', 'layout') now reaches seeded entries. The TPR header round-trip (encodeCacheTag → encodeURIComponent/comma-join → decodeURIComponent → encodeCacheTag) is lossless and comma-safe (commas encode to %2C, so the split(',') is safe; literal %2C double-encodes to %252C and survives).
I independently confirm the three issues @ask-bonk raised; rather than duplicate the inline anchors, summarizing with priority + a concrete direction for the blocker:
1. (Should fix before merge) Info disclosure via spoofable UA + no outbound strip. Verified: __dispatchAppPage in entries/app-rsc-entry.ts:676 is the production Worker request handler, not just the local TPR subprocess. decorateInternalImplicitTags (app-page-dispatch.ts:674) gates only on User-Agent: vinext-tpr/1.0, and filterInternalHeaders (request-pipeline.ts:632) only strips inbound request headers — it is never applied to outbound responses. So any public client that sends User-Agent: vinext-tpr/1.0 to a deployed Worker receives x-vinext-implicit-tags, leaking route-group names ((main)), dynamic segment patterns, and the full layout hierarchy. This is data that did not leak before this PR. The UA gate is also brittle in another way: a real browser/bot that happens to send that UA would get the header on every page. Recommended fix: gate on a per-build secret token header (generated at build, known only to the TPR client) rather than a guessable UA. The token approach also removes the need for an outbound strip, since no public client can produce a valid token. If you keep the UA gate, you additionally need to strip x-vinext-implicit-tags from the outbound public response path.
2. (Minor) Root-route fallback is not byte-identical, contradicting the comment. Verified by tracing both builders for /: buildAppPageCacheTags('/', []) → ['/', '_N_T_/', '_N_T_/layout', '_N_T_/page'], whereas the live render's buildAppPageTags('/', fetchTags, []) additionally emits _N_T_/index (implicit-tags.ts:63). Since / always has empty routeSegments it always takes the fallback branch (seed-cache.ts:140-142), so the seeded root entry is missing _N_T_/index. revalidatePath('/') still works (both carry _N_T_/), so impact is narrow — but the 'byte-identical for static routes' claim in the comment is inaccurate for the root specifically. Either special-case / to buildAppPageTags(cachePathname, [], []), or soften the comment.
3. (Minor/edge) Fresh-render TPR header is captured before the stream drains. Verified: decorateInternalImplicitTags calls getCollectedFetchTags() synchronously when renderAppPageLifecycle returns (app-page-dispatch.ts:680), but the live ISR write computes getPageTags() (→ getCollectedFetchTags()) lazily inside the post-drain cachePromise continuation (app-page-cache.ts:678 and :799). Fetch/cacheTag() tags collected while the HTML/RSC stream drains land in the stored ISR entry but not in the TPR header, so the 'EXACT set live ISR stores' claim does not hold on the fresh-render path for pages that collect tags during streaming. The HIT path is fine (no render). Likely rare for TPR (mostly HITs), but worth either capturing post-drain or softening the comment.
Nit: the instrumentation.test.ts maxRetries change is a reasonable flake fix but unrelated to #1984 scope (already a separate commit, just noting).
No correctness bugs in the happy path. Issue #1 is the one I'd want resolved before merge.
|
Review posted. Summary of my findings: Review of PR #2061The core fix is correct. I independently traced the tag-derivation paths and verified:
I confirmed all three issues the prior reviewer raised hold up under tracing, and posted a corroborating review prioritizing them:
Plus a nit on the unrelated |
Performance benchmarksCompared 0 improved · 0 regressed · 6 within ±1.5%
View detailed results and traces 🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head |
- Security: replace spoofable User-Agent gate for x-vinext-implicit-tags with a per-build random UUID token (VINEXT_TPR_SECRET / x-vinext-tpr-secret). Token is generated in prerenderRoutes, passed to the subprocess via env, sent as a request header, and stripped from inbound requests via VINEXT_INTERNAL_HEADERS. Deployed Workers never have the env var set so the header can never be emitted to public clients. - Root route: prerender.ts now always writes routeSegments (even []) into the manifest so seed-cache uses buildAppPageTags for root, emitting the _N_T_/index tag that the concrete-path fallback missed. - Fresh-render comment: clarify that streaming-collected fetch tags may be missing from the TPR header on the fresh-render path (rare; mostly HITs). Tests: +1 root-route seed test (_N_T_/index), updated request-pipeline assertion for VINEXT_INTERNAL_HEADERS to include new secret header.
|
Addressed all three items:
|
Prerender-seeded entries for dynamic routes were tagged from the concrete
pathname (e.g. `_N_T_/posts/hello/page`), while runtime re-renders tag from
the bracketed route pattern (`_N_T_/posts/[slug]/page`). A typed
`revalidatePath('/posts/[slug]','layout')` emits the bracketed tag, so it
could never reach seeded entries — stale prerendered HTML/RSC survived
content updates until natural regeneration. Next.js derives layout/page tags
from the bracketed page for both build-prerender and live-render, so this was
a vinext-only divergence.
Make seeded and TPR-uploaded entries carry the IDENTICAL tag set the live
render produces:
- Build prerender now records each app route's `routeSegments` (the raw
filesystem segments, including route groups and brackets — the same field
the live render uses) in `vinext-prerender.json`.
- seed-cache builds tags via `buildAppPageTags(pathname, [], routeSegments)`
instead of the concrete-path `buildAppPageCacheTags`, falling back to the
concrete builder when `routeSegments` is absent (legacy manifest / root
route) — byte-identical for static routes.
- TPR has only analytics URLs with no route pattern, so the prod server now
emits the live-computed implicit tags as an internal-only
`x-vinext-implicit-tags` response header (gated strictly to the TPR
User-Agent, on both cache-hit and fresh-render responses, and stripped from
inbound requests). `buildTprKVPairs` reads it verbatim, falling back to the
concrete builder when absent.
The exact-path tag (`_N_T_/posts/hello`) is still emitted, so concrete
`revalidatePath('/posts/hello')` keeps working. `revalidatePath(concrete,'page')`
for a dynamic route no longer matches seeded entries — aligning with Next.js,
where dynamic-route page/layout revalidation requires the route pattern.
Closes cloudflare#1984
`withInjectClientServer` spins up a real Vite dev server; its esbuild/rollup workers and file watchers can still be flushing files into the temp dir while the recursive `fs.rmSync` walks it, racing to ENOTEMPTY even with `force:true`. Add `maxRetries`/`retryDelay` so teardown retries on ENOTEMPTY/EBUSY instead of failing the unit shard. Unrelated to the cloudflare#1984 cache change; surfaced as a flaky CI failure on this branch.
- Security: replace spoofable User-Agent gate for x-vinext-implicit-tags with a per-build random UUID token (VINEXT_TPR_SECRET / x-vinext-tpr-secret). Token is generated in prerenderRoutes, passed to the subprocess via env, sent as a request header, and stripped from inbound requests via VINEXT_INTERNAL_HEADERS. Deployed Workers never have the env var set so the header can never be emitted to public clients. - Root route: prerender.ts now always writes routeSegments (even []) into the manifest so seed-cache uses buildAppPageTags for root, emitting the _N_T_/index tag that the concrete-path fallback missed. - Fresh-render comment: clarify that streaming-collected fetch tags may be missing from the TPR header on the fresh-render path (rare; mostly HITs). Tests: +1 root-route seed test (_N_T_/index), updated request-pipeline assertion for VINEXT_INTERNAL_HEADERS to include new secret header.
Divkix
force-pushed
the
fix/issue-1984-isr-seeded-tags
branch
from
b53d196 to
7743f62
Compare
2 participants
Problem
Prerender-seeded entries for dynamic routes are tagged from the concrete pathname (e.g.
_N_T_/posts/hello/page), while runtime re-renders tag from the bracketed route pattern (_N_T_/posts/[slug]/page). A typedrevalidatePath('/posts/[slug]','layout')emits_N_T_/posts/[slug]/layout, which can never reach a seeded entry — so stale prerendered HTML/RSC survives content updates until natural regeneration. Concrete-pathrevalidatePath('/posts/hello')still works; the gap is the typed/segment-pattern invalidation.Next.js derives layout/page tags from the bracketed
pagefor both build-time prerender and runtime re-render (singlegetImplicitTags(page, pathname)path), so seeded and rendered entries share an identical tag set. vinext's seed/TPR path used the concrete-path builder while its live render used arouteSegments-aware builder — a vinext-only divergence.Closes #1984.
Fix
Make seeded and TPR-uploaded entries carry the identical tag set the live render produces.
prerender.ts) now records each app route'srouteSegments— the raw filesystem segments (incl. route groups(main)and brackets[slug]), the same field the live render reads — intovinext-prerender.json.buildAppPageTags(pathname, [], routeSegments)instead of the concrete-pathbuildAppPageCacheTags. Falls back to the concrete builder whenrouteSegmentsis absent (legacy manifest / root route) — byte-identical for static routes. (field-absent, not empty-array, is the fallback trigger:[]is valid for/).x-vinext-implicit-tagsresponse header — gated strictly to the TPRUser-Agent, on both the cache-hit and fresh-render paths, and stripped from inbound requests.buildTprKVPairsreads it verbatim, falling back to the concrete builder when absent.Behavior notes
_N_T_/posts/hello) is still emitted, so concreterevalidatePath('/posts/hello')(no type) keeps working.revalidatePath(concrete, 'page'|'layout')for a dynamic route no longer matches seeded entries — aligning with Next.js, where dynamic-route page/layout revalidation requires the route pattern (/posts/[slug]), not a concrete URL.@slots, and i18n are correct by construction (samerouteSegmentssource as the live render). Pages Router is untouched (router === "app"only).Tests
tests/seed-cache.test.ts(+5): dynamic-route bracketed tags; typedrevalidatePath('/posts/[slug]','layout')invalidation; concreterevalidatePath('/posts/hello')still invalidates; route-group(main)case; legacy-manifest concrete fallback.tests/tpr-kv-keys.test.ts(+3):buildTprKVPairsuses thex-vinext-implicit-tagsheader verbatim; route-group round-trip; header-absent fallback.tests/request-pipeline.test.ts: updated for the new internal header (also asserts a forged inbound value is stripped).Full unit suite green except a pre-existing
better-sqlite3native-build issue and appr-fallback-shelltiming flake (passes in isolation), both unrelated. Typecheck/format clean for all changed files.