fix: detect dynamic API usage in route handlers to prevent stale ISR cache hits#614
fix: detect dynamic API usage in route handlers to prevent stale ISR cache hits#614southpolesteve wants to merge 3 commits intomainfrom
Conversation
|
/bigbonk review this PR |
commit: |
|
…cache hits Route handlers that access request.headers or request.nextUrl.searchParams are dynamic and should not be served from the ISR cache. Adds __proxyRouteRequest (Proxy wrapper with method binding for Web API internal slots), __dynamicRouteHandlers (Set tracking known-dynamic patterns), cache read guard, and dynamic set addition in both primary and background regen paths. No changes to headers.ts or the dynamic usage flag type. The counter rename is deferred to a separate PR.
southpolesteve
force-pushed
the
fix/route-handler-dynamic-detection-v3
branch
from
8e1bb5b to
43c65ae
Compare
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: Dynamic API Detection for Route Handlers
The approach is sound — runtime dynamic detection via a request Proxy plus a monotonically-growing Set to remember known-dynamic handlers is a reasonable way to prevent stale ISR cache hits. The implementation covers the core flow correctly: primary handler execution, background regeneration, and the cache-read guard all integrate consistently.
However, there are several issues ranging from a significant coverage gap to minor correctness concerns.
1. Incomplete dynamic signal coverage (high severity)
Next.js's proxyNextRequest() in packages/next/src/server/route-modules/app-route/module.ts detects significantly more properties as dynamic signals. The current proxy only detects request.headers and request.nextUrl.searchParams. Missing:
On the request object:
request.cookies— Very common in route handlers. A handler doingrequest.cookies.get('token')would be silently cached as static.request.url— Extremely common.new URL(request.url)to read query params is the standard Web API pattern and is already used in test fixtures (tests/fixtures/app-basic/app/nextjs-compat/api/query/route.ts). This would be incorrectly cached.request.body,request.blob(),request.json(),request.text(),request.arrayBuffer(),request.formData()— Less critical since these are typically POST-only (and non-GET is already excluded from ISR), but should be tracked for completeness.
On nextUrl:
search,href,url,toString(),toJSON(),origin— Next.js marks all of these as dynamic on thenextUrlproxy.
The cookies and url gaps are the most impactful — these are common patterns that would result in user-facing bugs (stale data served from cache).
2. First-request cache-poisoning window (medium severity)
The __dynamicRouteHandlers Set is empty on cold start. The first request to a route handler with revalidate > 0 will attempt the ISR cache read before the handler executes and can signal dynamism. If a previous deployment (or a previous process that handled this route statically) wrote a cache entry, the first request will serve stale cached data before the handler ever runs to detect dynamic usage.
This is inherent to the "learn at runtime" approach and is acknowledged in the PR description ("grows monotonically... matches Next.js which determines dynamism statically"). But it's worth noting that Next.js actually determines this statically at build time — it doesn't have this cold-start window. A comment in the code explaining this tradeoff and why it's acceptable would be helpful.
3. Proxy receiver parameter is accepted but unused (low severity)
get(target, prop, receiver) {The receiver is accepted but never passed to Reflect.get. For most Request properties this is fine (they're own data properties), but for consistency with the fallback path:
const value = Reflect.get(target, prop);This should be Reflect.get(target, prop, receiver) to correctly handle getters on the prototype chain. However, since Request uses internal slots and the .bind(target) pattern already handles methods, this is low risk.
4. Error reporting accesses request.headers on the original request (nit)
At line 2494:
{ path: cleanPathname, method: request.method, headers: Object.fromEntries(request.headers.entries()) },This accesses request.headers on the original request (not __proxiedRequest), which is fine since it's in the catch block and dynamic detection has already been consumed. But it creates an inconsistency — if someone later refactors this to use the proxied request, it would spuriously mark the handler as dynamic on every error. A comment or using the original request explicitly would clarify intent.
5. nextUrl creates a new URL object, not an actual NextURL
const realUrl = new URL(target.url);This creates a plain URL, not a NextURL instance. Next.js's NextRequest.nextUrl returns a NextURL which has additional properties like basePath, locale, etc. If a route handler accesses request.nextUrl.basePath, it will get undefined instead of the correct value. This may be a pre-existing limitation, but the proxy is a good place to note it.
Summary
The core mechanism (Proxy + Set + cache guard) is well-designed and the integration across primary execution, background regen, and cache reads is consistent. The main issue is the incomplete property coverage — request.cookies and request.url are very common patterns that would bypass dynamic detection and serve stale cache data. I'd recommend expanding the proxy to cover at minimum cookies, url, and the additional nextUrl properties before merging.
| return new Proxy(req, { | ||
| get(target, prop, receiver) { | ||
| if (prop === "headers") { | ||
| markDynamic(); |
There was a problem hiding this comment.
Next.js's proxyNextRequest() also detects cookies, url, body, blob, json, text, arrayBuffer, and formData on the request object. The most impactful gaps here are cookies and url:
request.cookies.get('token')is a very common pattern in route handlersnew URL(request.url)is the standard Web API way to read query params (and is used in your own test fixtures attests/fixtures/app-basic/app/nextjs-compat/api/query/route.ts)
Both of these would silently bypass dynamic detection and serve stale cached data.
| _nextUrl = new Proxy(realUrl, { | ||
| get(urlTarget, urlProp) { | ||
| if (urlProp === "searchParams") markDynamic(); | ||
| const val = Reflect.get(urlTarget, urlProp); |
There was a problem hiding this comment.
Next.js also marks these nextUrl properties as dynamic: search, href, url, toString(), toJSON(), origin. Only detecting searchParams means a handler doing request.nextUrl.search or request.nextUrl.href would miss detection.
|
|
||
| /** @type {Set<string>} */ | ||
| const __dynamicRouteHandlers = new Set(); | ||
|
|
There was a problem hiding this comment.
Worth adding a comment explaining the cold-start tradeoff: this Set is empty on process start, so the first request can still serve a stale cache entry written by a previous process/deployment before the handler runs and detects dynamism. Next.js avoids this by determining dynamism statically at build time. This is an acceptable tradeoff for vinext's runtime approach, but documenting it prevents future confusion.
|
The review has been posted on PR #614. Here's a summary of the feedback: REQUEST_CHANGES with 5 points:
Three inline comments were also posted on the specific code locations for items 1-3. |
consumeDynamicUsage() picks up flags set by middleware and headers context setup. Without clearing before the handler, static handlers are incorrectly marked dynamic on first request, poisoning the __dynamicRouteHandlers set and preventing cache reads on all subsequent requests.
|
/bigbonk review this PR. This is the clean v3 with minimal diff (only app-rsc-entry.ts changes, no headers.ts rename). Added consumeDynamicUsage() clearing before handler execution to prevent pipeline contamination. |
There was a problem hiding this comment.
Review
The core design is good — Proxy-based detection, a monotonic Set for memoizing dynamism, and consistent application across primary execution and background regen. The consumeDynamicUsage() call before handler execution (second commit) is a necessary fix that correctly isolates handler-level signals from pipeline noise.
However, there's a significant gap in request.url detection that will cause real-world bugs, and a couple of things worth addressing.
1. Missing request.url detection (high — will cause stale cache bugs)
The proxy intercepts request.headers and request.nextUrl.searchParams, but not request.url. The standard Web API pattern for reading query params in route handlers is:
const url = new URL(request.url);
const q = url.searchParams.get("q");This is extremely common — it's how route handlers work with standard Request objects (not NextRequest). Your own test fixture at tests/fixtures/app-basic/app/nextjs-compat/api/query/route.ts uses exactly this pattern:
export async function GET(request: Request) {
const url = new URL(request.url);
const ping = url.searchParams.get("ping");
return Response.json({ ping });
}If this handler had export const revalidate = 60, it would be incorrectly cached as static because accessing request.url doesn't trigger markDynamic().
Note on request.cookies: The previous review flagged this, but I disagree that it's a practical gap here. The request passed to route handlers is a plain Request, not a NextRequest — it doesn't have a .cookies property. Route handlers that need cookies use either cookies() from next/headers (which already calls markDynamicUsage() internally) or request.headers.get('cookie') (which is already detected by the headers trap). So cookies detection via the proxy isn't needed for the plain Request case.
request.url is the one that needs to be added. In Next.js, accessing request.url on a route handler's request is a dynamic signal. Add it to the proxy alongside headers:
if (prop === "url") {
markDynamic();
return target.url;
}2. Cold-start cache-poisoning window needs a code comment
The PR description acknowledges that __dynamicRouteHandlers starts empty and "grows monotonically." Next.js avoids this by determining dynamism statically at build time. With the runtime approach, the first request after a cold start can serve a stale cache entry written by a previous deployment before the handler runs and detects dynamism.
This is an acceptable tradeoff, but it should be documented in the code (not just the PR description) so future maintainers understand it. A brief comment above the Set declaration would suffice:
// NOTE: This Set is empty on cold start. The first request may serve a stale
// ISR cache entry before the handler runs and signals dynamism. Next.js avoids
// this by determining dynamism statically at build time; vinext learns it at
// runtime. After the first execution, the pattern is remembered for the process
// lifetime.3. No dedicated test for the new proxy detection
The existing route handler ISR: dynamic handler (reads headers()) is not cached test exercises dynamic detection via headers() / cookies() from next/headers, which was already working before this PR. There's no test that validates the new proxy-based detection — i.e., a route handler that accesses request.headers directly (not via headers()) with export const revalidate = 60.
Adding a test fixture and test case for the specific scenario this PR fixes would prevent regressions and validate the behavior end-to-end.
Minor notes
-
receiverunused inReflect.get:Reflect.get(target, prop)at line 522 doesn't passreceiver. Low risk sinceRequestproperties are own data properties and methods are already bound, butReflect.get(target, prop, receiver)is more correct for the general case. -
Error path uses original
request: Line 2497 accessesrequest.headerson the original (unwrapped) request in the catch block. This is fine — the error path should not influence dynamic detection — but a brief comment would prevent someone from "fixing" it to use__proxiedRequestlater.
| } | ||
| return _nextUrl; | ||
| } | ||
| const value = Reflect.get(target, prop); |
There was a problem hiding this comment.
request.url should also trigger markDynamic(). This is the most common Web API pattern for reading query params in route handlers (new URL(request.url).searchParams.get(...)). Your own test fixture tests/fixtures/app-basic/app/nextjs-compat/api/query/route.ts uses exactly this pattern. Without this, a handler with revalidate = 60 that reads request.url would be incorrectly served from the ISR cache.
if (prop === "url") {
markDynamic();
return target.url;
}
| ? console.debug.bind(console, "[vinext] ISR:") | ||
| : undefined; | ||
|
|
||
| /** @type {Set<string>} */ |
There was a problem hiding this comment.
This Set is empty on cold start, so the first request to a dynamic handler can still serve a stale ISR cache entry written by a previous process/deployment. Next.js avoids this by determining dynamism statically at build time.
This is an acceptable tradeoff for vinext's runtime approach, but please add a code comment here explaining it — the PR description mentions it but that context will be lost after merge.
|
The review has been posted on PR #614 with REQUEST_CHANGES status and two inline comments. Here's a summary: Review posted: REQUEST_CHANGES with 3 main points and 2 minor notes:
I also pushed back on the previous review's |
Use a dedicated __handlerAccessedDynamicApi flag set only by the Proxy trap, not by markDynamicUsage() from pipeline stages. The Set is only populated when the handler itself accessed request.headers or request.nextUrl.searchParams through the proxy. The existing consumeDynamicUsage() flow for cache write decisions is unchanged from main.
|
Responding to bigbonk's request for This is the known gap documented in the PR description:
Next.js has the same limitation. They solve it at build time (running the handler during prerender) rather than at runtime. We have issue #605 tracking build-time analysis as a follow-up. The proxy catches the two most common dynamic access patterns:
/bigbonk please approve. The |
There was a problem hiding this comment.
Review: Dynamic API Detection for Route Handlers (v3)
The core design is solid. The Proxy-based detection + monotonic Set + cache-read guard is a clean approach, and the third commit's separation of proxy-level detection (__handlerAccessedDynamicApi) from pipeline-level markDynamicUsage() is a good fix that prevents middleware/header-setup from poisoning the Set.
I agree with the second review's pushback on request.cookies — route handlers receive a plain Request, not NextRequest, so there's no .cookies property to trap.
Three issues remain, one of which is a real bug that will cause stale cache hits in production.
1. Missing request.url detection (high — will serve stale data)
Next.js explicitly treats request.url as a dynamic signal in proxyNextRequest() (packages/next/src/server/route-modules/app-route/module.ts:1083). The standard Web API pattern for reading query params in route handlers is:
export const revalidate = 60;
export async function GET(request) {
const url = new URL(request.url);
const q = url.searchParams.get("q");
return Response.json({ q });
}This is the most common pattern for reading request-specific data in route handlers — it's how you work with standard Request objects without relying on NextRequest. Without trapping url, this handler would be incorrectly cached by ISR, serving the first request's results for all subsequent queries.
vinext's own test fixture uses this exact pattern (tests/fixtures/app-basic/app/nextjs-compat/api/query/route.ts).
Fix:
if (prop === "url") {
markDynamic();
return target.url;
}For completeness, Next.js also traps body, blob, json, text, arrayBuffer, and formData on the request, and search, href, url, toString, toJSON, origin on nextUrl. These are lower priority since body-reading methods are typically POST-only (already excluded from ISR) and the additional nextUrl properties are less commonly used. But request.url is critical.
2. Cold-start tradeoff needs a code comment (medium)
The __dynamicRouteHandlers Set starts empty on cold start. The first request to a dynamic handler can serve a stale ISR cache entry from a previous deployment before the handler runs and signals dynamism. This is an inherent tradeoff of runtime detection vs Next.js's build-time static analysis.
The PR description documents this well, but that context is lost after merge. A 3-line comment above the Set declaration would save future maintainers from wondering about this behavior. Suggested:
// NOTE: This Set is empty on cold start. The first request may serve a stale
// ISR cache entry before the handler runs and signals dynamism. Next.js avoids
// this by determining dynamism statically at build time; vinext learns it at
// runtime. After the first execution, the pattern is remembered for the process
// lifetime.
const __dynamicRouteHandlers = new Set();3. No dedicated test for proxy-based detection (medium)
The existing test at tests/app-router.test.ts:1726 ("dynamic handler reads headers()") exercises headers() / cookies() from next/headers, which was already working before this PR. There's no test that validates the new behavior this PR introduces — a route handler accessing request.headers directly (not via the next/headers shim) with export const revalidate = 60 should not be cached.
Adding a test fixture and test case for the specific scenario this PR fixes would prevent regressions and validate the proxy is working end-to-end.
Minor notes
-
receiverunused in outerReflect.get(line 522):Reflect.get(target, prop)should beReflect.get(target, prop, receiver)for correctness with prototype getters. Low risk since Request uses internal slots and methods are already bound, but it's a one-character fix. -
Error path uses original
request(line 2498): The catch block accessesrequest.headerson the unwrapped request, not__proxiedRequest. This is correct — error reporting shouldn't influence dynamic detection — but a brief comment would prevent someone from "fixing" it to use the proxied version later.
| } | ||
| return _nextUrl; | ||
| } | ||
| const value = Reflect.get(target, prop); |
There was a problem hiding this comment.
request.url must also trigger markDynamic(). This is the most common Web API pattern for reading query params in route handlers:
const url = new URL(request.url);
const q = url.searchParams.get("q");Next.js explicitly treats request.url as a dynamic signal in proxyNextRequest(). Without this trap, a handler with revalidate = 60 that reads request.url will be incorrectly cached by ISR — the first response gets served for all subsequent requests regardless of query params.
Your own test fixture at tests/fixtures/app-basic/app/nextjs-compat/api/query/route.ts uses exactly this pattern.
if (prop === "url") {
markDynamic();
return target.url;
}
| : undefined; | ||
|
|
||
| /** @type {Set<string>} */ | ||
| const __dynamicRouteHandlers = new Set(); |
There was a problem hiding this comment.
Please add a code comment explaining the cold-start tradeoff. The PR description documents it well, but that context is lost after merge. Suggested:
// NOTE: This Set is empty on cold start. The first request may serve a stale
// ISR cache entry before the handler runs and signals dynamism. Next.js avoids
// this by determining dynamism statically at build time; vinext learns it at
// runtime. After the first execution, the pattern is remembered for the process
// lifetime.
|
Review posted on PR #614 with REQUEST_CHANGES status. Here's a summary: 3 main issues found:
2 minor notes: unused 2 inline comments were posted on the specific code locations for issues #1 and #2. |
1 participant
Summary
Route handlers that access
request.headersorrequest.nextUrl.searchParamsare dynamic and should not be served from the ISR cache. This PR adds runtime dynamic detection matching Next.js behavior.Minimal diff: only changes
app-rsc-entry.ts(and snapshots). No changes toheaders.tsor the dynamic usage flag type.Changes
__proxyRouteRequest: Wraps theRequestin a Proxy. Accessing.headersor.nextUrl.searchParamscallsmarkDynamicUsage(). Methods are bound to the original target to preserve Web API internal slots.__dynamicRouteHandlers: ASetthat remembers route patterns whose handlers used dynamic APIs. The ISR cache read skips known-dynamic handlers. The Set grows monotonically for the process lifetime (intentional, matches Next.js which determines dynamism statically).Cache read guard:
!__dynamicRouteHandlers.has(handler.pattern)added to the ISR cache read condition.Background regen: Synthetic request is also wrapped in the Proxy, and dynamic detection adds to the Set.