feat(app-router): gate SSR useSearchParams() in cache-candidate renders - #3455
james-elicx wants to merge 6 commits into
Conversation
|
/bigbonk review for issues |
@vinext/cloudflare
create-vinext-app
@vinext/types
vinext
@cloudflare/workers-response-store
commit: |
Performance benchmarksCompared 0 improved · 0 regressed · 6 within ±1.5%
View detailed results and traces 🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
| * server component can mark the render dynamic any more. | ||
| */ | ||
| export function startCandidateSearchParamsGate(): CandidateSearchParamsGate { | ||
| const controller = createSearchParamsGate({ onOpen: markDynamicUsage }); |
There was a problem hiding this comment.
[P1] Preserve the cache veto in the parent request scope
When isRenderDynamicLatched() is already true because a layout probe called a dynamic API inside runWithIsolatedDynamicUsage, this callback runs inside handleSsr’s nested runWithNavigationContext scope. markDynamicUsage() therefore sets only that shallow-cloned child’s boolean; the parent flag consumed by finalizeAppPageHtmlCacheResponse remains false. The gate is nevertheless opened, so useSearchParams() can render the real query and, if the final render does not repeat the dynamic read (for example because the probed work is memoized), that HTML is stored under the query-free key. Propagate the gate-open cache veto to the owning request scope, or make finalization consult shared gate/latch state.
There was a problem hiding this comment.
Fixed in fb51d5a. A candidate HTML render now also treats the request's shared dynamic latch as dynamic usage, so usage in a child scope (layout probe or SSR) vetoes both core's write and adapter admission, and sets the response policy when it happens before headers.
| (options?.isStaticGeneration === true ? options.isForceStatic !== true : undefined); | ||
| return buildHeadInjectionHtml( | ||
| ssrNavigationContext, | ||
| searchParamsGate |
There was a problem hiding this comment.
[P1] Keep the effective query for gates that open as dynamic
This always serializes an empty query with searchParamsFromBrowser: true for every candidate, including gates decided as real. A rewrite can add query parameters that are intentionally visible through useSearchParams() (the middleware fixture documents this contract), while window.location.search still contains only the public URL. In a render that becomes dynamic via headers()/cookies(), SSR now emits the rewrite query but hydration restores the browser query, causing a mismatch and then returning different values client-side. Once the gate opens, serialize ssrNavigationContext.searchParams instead of forcing the browser-sourced payload (including the case where the decision occurs after the first head insertion).
There was a problem hiding this comment.
Fixed in fb51d5a for gates that open before the head is written: the payload then serializes the effective query without the browser flag. That covers dynamic APIs in the shell, such as the rewrite fixture's headers() page. It's pinned in the Response Store e2e.
Gates that open after the head was flushed are left as they are. By then the browser-sourced payload is already on the wire, and the client reads it once, before hydration starts. Fixing that needs client runtime work beyond this PR. It only matters when a rewrite changes the query, the dynamic API runs after the shell, and a client component reads useSearchParams(). React's hydration then recovers with the browser query.
| `${error.reason} should be wrapped in a suspense boundary at page "${options.routePattern}". Read more: https://nextjs.org/docs/messages/missing-suspense-with-csr-bailout`, | ||
| ); | ||
| options.clearRequestContext(); | ||
| return Promise.resolve(internalServerErrorResponse()); |
There was a problem hiding this comment.
[P1] Preserve middleware response headers on this 500
This new early response bypasses buildAppPageHtmlResponse, so middleware-provided response headers are dropped only for the missing-Suspense failure path. That loses Set-Cookie, CSP/security headers, and other custom headers that all normal and recovered App-page responses merge from options.middlewareContext.headers. Build/merge the error response through the same middleware-header path while keeping status 500 and a non-cacheable policy.
There was a problem hiding this comment.
Fixed in fb51d5a. The 500 now merges middleware headers and is sent with Next.js's never-cache Cache-Control plus the CDN headers, matching the recovered shell-error path.
|
Submitted 3 actionable inline findings. Focused tests and checks passed. |
|
/bigbonk review for issues Please review exact head |
| if (settled) return; | ||
| settled = true; | ||
| unsubscribe(); | ||
| controller.settle(); |
There was a problem hiding this comment.
[P1] Open the gate for fetches that make the output fresh
settle() only checks the dynamic-API latch indirectly through the listener, but several fetch paths deliberately call recordDynamicFetchObservation() without markDynamicUsage() (notably the default uncached fetch, auth-keyed safety bypass, and cache-key-generation fallback). Those observations make the page output ineligible for shared reuse, yet this gate settles to "bailout", so a Suspense-wrapped useSearchParams() renders its fallback instead of the real query on an otherwise dynamic/fresh response. Include the completed render/fetch observations in the settle decision, or signal the gate when one of these cache-vetoing fetches is observed.
There was a problem hiding this comment.
Declining this one, because it would diverge from Next.js. These fetch paths are Next.js's auto no cache cases: the default fetch with no cache config, and the auth or cookie header bypass. Next.js deliberately doesn't let autoNoCache make a render dynamic: // we don't consider autoNoCache to switch to dynamic for ISR in packages/next/src/server/lib/patch-fetch.ts. Only revalidate: 0 and no-store fetches call markCurrentScopeAsDynamic. vinext's markUncachedFetchForPageOutput mirrors that.
So in Next.js a static page with one of these fetches is still prerendered. Its Suspense-wrapped useSearchParams() bails out to the fallback, and the page is stored with revalidate = false. dynamicFetches doesn't veto the ISR write in vinext either; it only feeds client reuse metadata and layout classification. Opening the gate here would call markDynamicUsage and stop the page being stored, which is less than Next.js stores. The key-generation fallback is the same: Next.js would cache that fetch, so the page stays static there too.
| const headers = new Headers(); | ||
| mergeMiddlewareResponseHeaders(headers, options.middlewareContext.headers); | ||
| headers.set("Cache-Control", NEVER_CACHE_CONTROL); | ||
| const response = internalServerErrorResponse(undefined, { headers }); |
There was a problem hiding this comment.
[P2] Preserve the edge-runtime marker on the bailout response
This direct response bypasses buildAppPageHtmlResponse, so an otherwise cache-candidate page configured with runtime = "edge" loses the x-edge-runtime: 1 marker only on this missing-Suspense 500. Other App-page response paths consistently call applyEdgeRuntimeHeader; apply it here as well using options.isEdgeRuntime.
There was a problem hiding this comment.
Fixed in f9ec96e. The bail-out 500 now applies x-edge-runtime from options.isEdgeRuntime, like the other App-page response paths. The unit test asserts it.
|
Submitted 2 actionable inline findings. Targeted tests passed; no files modified. |
fb51d5a to
f9ec96e
Compare
|
/bigbonk review for issues Please review exact head |
|
| // A render that may be stored, and so must not let the request's query reach | ||
| // its output unless it turns out dynamic. PPR fallback shells follow | ||
| // cacheComponents' model instead. | ||
| const isCacheCandidate = shouldReadCache && options.pprRuntime === undefined; |
There was a problem hiding this comment.
[P1] Gate normal cacheComponents renders too
pprRuntime is supplied on every request whenever cacheComponents is enabled; only pprRuntime.getState()/pprFallbackShell identifies an active fallback-shell render. This condition therefore disables candidate mode for ordinary HTML misses in every cacheComponents app. Those renders can still be cacheable (for example revalidate = 60 or a generateStaticParams route with the Infinity default), so a client component's useSearchParams() reads the real request query during SSR and the resulting HTML is written under the query-free page key, preserving the poisoning bug this PR fixes. Exempt only an active fallback-shell render rather than every build that has a PPR runtime.
|
Submitted 1 actionable inline finding. Targeted tests and checks passed; no files modified. |
f9ec96e to
3fadbd1
Compare
|
/bigbonk review for issues Please review exact head |
3fadbd1 to
edf4f2b
Compare
| try { | ||
| result = await reader.read(); | ||
| } catch (error) { | ||
| settle(); |
There was a problem hiding this comment.
[P2] Do not turn Flight failures into a static bailout
This error path calls settle(), which fulfills every suspended useSearchParams() with the BailoutToCSRError before propagating the actual Flight error. If the root model was decoded before a later stream failure, Fizz can retry the waiting client component and the new bailout can replace the real render failure with the missing-Suspense 500 (or a Suspense fallback). Cancellation at line 60 similarly is not the normal EOF that proves a static render. Abnormal termination should unsubscribe and reject/open the gate while preserving the original failure, rather than deciding "bailout".
There was a problem hiding this comment.
Fixed in 307bbf1. Only a normal end of the Flight stream settles the gate as a bailout now. A failed or cancelled stream opens it instead: that marks the render dynamic, so it's never stored, and waiting useSearchParams() calls read real values rather than throwing a bailout that could replace the real failure. The Flight error still reaches SSR unchanged. Tests: cancellation opens the gate and marks the render dynamic, and a Flight failure rejects the read with the original error and opens the gate.
| handlerStart: options.handlerStart, | ||
| hasLoadingBoundary: hasActiveLoadingBoundary, | ||
| omitPendingDynamicCacheState: hasRequestSearchParams, | ||
| omitPendingDynamicCacheState: hasRequestSearchParams && !isCacheCandidate, |
There was a problem hiding this comment.
[P2] Keep query-bearing RSC cache state provisional
isCacheCandidate is true for RSC cache misses too, but only HTML installs the new search-params gate (isCacheCandidateHtmlRender explicitly excludes RSC). A loading-boundary RSC render can therefore read searchParams after headers are built, skip the cache write, and still expose X-Vinext-Cache: MISS; before this change query-bearing RSC responses deliberately omitted that uncorrectable provisional state. Restrict this exception to HTML candidates (or keep omitPendingDynamicCacheState true for isRscRequest); the existing loading-boundary test only asserts not HIT, so it misses this regression.
There was a problem hiding this comment.
Fixed in 0a8897d. The provisional-state exception now covers only candidate HTML renders (hasRequestSearchParams && !(isCacheCandidate && !isRscRequest)), since only HTML installs the gate. Query-bearing RSC responses again send no X-Vinext-Cache. The loading-boundary RSC test now asserts the header is absent. It got MISS before the fix.
|
Submitted 2 actionable inline findings. Targeted tests passed ( |
edf4f2b to
0a8897d
Compare
|
/bigbonk review for issues Please review exact head |
0a8897d to
2495c69
Compare
|
No additional findings beyond the existing unresolved inline finding. Targeted tests passed ( |
3ff3648 to
d9efbaa
Compare
|
/bigbonk review for issues Please review exact head |
No new findings. Exact head verified; 489 targeted tests passed. Worktree unchanged. |
d9efbaa to
a6fc690
Compare
|
/bigbonk review for issues Please review exact head |
1 similar comment
|
/bigbonk review for issues Please review exact head |
No new findings. Exact head verified; 490 targeted tests passed. Worktree unchanged. |
a6fc690 to
d69107d
Compare
|
/bigbonk review for issues Please review exact head |
No new findings. Exact head verified; 492 targeted tests passed. Worktree unchanged. |
841b2cc to
f0c09f9
Compare
f0c09f9 to
a5388ea
Compare
a5388ea to
a4d2a5a
Compare
A production render that may be stored no longer lets the request's query reach its HTML unless the render turns dynamic: - SSR useSearchParams() waits on a per-request gate. It opens with real values once the render uses a dynamic API (the sticky latch), and otherwise bails out to client rendering once the RSC stream has ended and SSR has read the whole Flight response. - An unwrapped call fails the render with a 500 and Next.js's missing-suspense message instead of rendering error.tsx. - The navigation payload leaves the query for the browser to read. - Candidate renders report x-vinext-cache: MISS with or without a query.
- Treat a candidate HTML render as dynamic once its request latches, since usage in child scopes (the layout probe, SSR) never reaches the render's own flag but still opens the gate with the real query. - Keep the effective query in the navigation payload when the gate has opened before the head is written, so rewrite queries hydrate. - Merge middleware headers into the missing-Suspense 500 and mark it never-cacheable.
…bleError A cache-candidate render that settles static bails useSearchParams() out to its Suspense boundary, which React reports on hydration as a recoverable error carrying the BAILOUT_TO_CLIENT_SIDE_RENDERING digest. Like Next.js's onRecoverableError, skip it before reporting it globally or failing the initial visited-response cache publication.
a4d2a5a to
fe36ba1
Compare
1 participant
Stacked on #3454. Second part of plan PR 1 (cache-candidate SSR mode). This fixes the poisoning bug for SSR
useSearchParams(), where a query-bearing miss wrote the first visitor's query into the shared ISR entry.Problem
In production, SSR
useSearchParams()always returned the request's query, untracked, and the navigation payload serialized it into the HTML. On a static page the render was then stored under the query-free key, so every later visitor got the first visitor's query. Next.js never puts the query into static HTML: a call inside Suspense client-renders that boundary, and a call outside Suspense fails the build (a 500 at runtime for on-demand paths).Change
A render is a cache candidate when core would read the cache for it: production, a route Next.js classifies static or SSG (#3451), no draft mode, no nonce on HTML, not a server action, not an unverified interception or a cacheability probe. PPR fallback shells keep cacheComponents' model. Regeneration and prerender already render with
isStaticGeneration, so they bail out at once, as before.For candidate HTML renders:
useSearchParams()waits on a per-request gate (shims/search-params-gate.ts, started inhandleSsrbyserver/app-ssr-search-params-gate.ts). It's decided once, at whichever comes first:BailoutToCSRError, and React client-renders the nearest Suspense boundary, as in Next.js's static HTML. The page is stored.useSearchParams() should be wrapped in a suspense boundary at page "<route>", even when the route has anerror.tsx. Next.js rethrows the bail-out there too (app-render.tsx:3477-3488). The 500 keeps middleware headers and is never cacheable.searchParamsFromBrowserand serializes an emptysearchParams, so the browser reads its own URL. When the gate has already opened by the time the head is written, the payload keeps the effective query (a rewrite may have changed it) instead.force-statickeeps its current empty payload and never gates.x-vinext-cache: MISSwith or without a query.Dev is unchanged: it still renders real values.
Behaviour change for the release notes: an App page that calls
useSearchParams()outside Suspense on a static route rendered before (and cached the first visitor's query). It now returns a 500 in production with Next.js's message in the log, as Next.js does for an on-demand path. Wrapping the call in<Suspense>fixes it on both.Tests
tests/candidate-search-params-gate.test.tsrenders with Fizz and covers the plan's gate unit tests: wrapped (fallback, nothing dynamic), unwrapped (shell error), dynamic before the gate, dynamic while waiting, dynamic usage consumed at the shell, dynamic usage in an isolated scope, opening marks the render dynamic, dynamic after the settle point (fallback kept), a call after the decision, nested boundaries, and a cancelled Flight stream.tests/app-page-render.test.ts: an unwrapped bail-out in a candidate render is a 500 with Next.js's message and middleware headers, skips the error boundary and writes nothing; outside candidate mode the boundary still renders. A candidate render that latched dynamic only in an isolated scope is sent no-store and never stored.tests/app-page-dispatch.test.ts: the query-bearing store test now asserts the candidate flag reaches SSR and the response reportsMISS; draft mode and dev render outside candidate mode.examples/response-store-demo, new/search-params/*routes):?q=<uuid>on a static page with a wrapped call is stored and re-served with the fallback, a browser-sourced payload and no trace of the uuid; an unwrapped call on an on-demand path is a 500 and never stored; a page that readsheaders()renders the real query, serializes it in the payload, and isn't stored.Known residual
A gate that opens after the head has been flushed can't change the browser-sourced payload that's already been sent. This only matters when a rewrite changes the query and a client
useSearchParams()reads it. React's hydration then recovers with the browser query.