fix: detect dynamic API usage in route handlers to prevent stale ISR cache hits#610
fix: detect dynamic API usage in route handlers to prevent stale ISR cache hits#610southpolesteve wants to merge 10 commits intomainfrom
Conversation
…cache hits Route handlers that access request.headers or request.nextUrl.searchParams are dynamic and should not be served from the ISR cache. Previously, the cache key was pathname-only and there was no mechanism to skip the cache read for handlers that use dynamic APIs. Add three mechanisms matching Next.js behavior: 1. __proxyRouteRequest: wraps the Request passed to route handlers in a Proxy. Accessing .headers or .nextUrl.searchParams calls markDynamicUsage(), which the existing consumeDynamicUsage() check on the write path already respects. 2. __dynamicRouteHandlers: a module-level Set<string> that remembers route patterns whose handlers used dynamic APIs. On subsequent requests, the cache read path checks this set and skips the cache for known-dynamic handlers. 3. Cache read guard: the ISR cache read condition now includes !__dynamicRouteHandlers.has(handler.pattern), preventing stale cache hits for dynamic handlers. This replaces the query-string-in-cache-key approach (PR #603) with proper dynamic detection that matches how Next.js handles route handler caching: dynamic handlers are never cached, static handlers use pathname-only keys.
|
/bigbonk review this PR |
commit: |
![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
Good approach overall — runtime detection via Proxy matches how Next.js detects dynamic usage, and using the existing markDynamicUsage()/consumeDynamicUsage() plumbing is clean. The __dynamicRouteHandlers Set to remember past dynamic detections avoids the stale-first-request problem elegantly.
Dev/prod parity is fine — the prod server delegates App Router requests entirely to the RSC entry (rscHandler(request) at prod-server.ts:737), so this fix applies to both dev and prod without needing changes elsewhere. Worker entry uses the same generated RSC entry.
I have two issues — one is a real bug, the other is a correctness concern worth discussing.
Bug: Background regeneration passes raw Request, bypassing the Proxy
At line 2342-2343, the background regeneration path creates a raw Request and passes it directly to the handler:
const __syntheticReq = new Request(__revalUrl, { method: "GET" });
const __revalResponse = await __revalHandlerFn(__syntheticReq, { params: __revalParams });This means the Proxy detection doesn't run during background regen. Right now it doesn't matter because:
- The regen path already calls
consumeDynamicUsage()on line 2344 and adds to__dynamicRouteHandlerson line 2347 if the handler callsheaders()/cookies()directly (via the shims). - If a handler only reads
request.headers(not theheaders()function), the regen path would miss it, cache the response, and keep serving it.
However, this scenario is mitigated because the first request's handler execution (line 2383-2391) would have already added the pattern to __dynamicRouteHandlers, so the STALE path wouldn't be reached in practice.
But there's an edge case: If the handler conditionally accesses request.headers (e.g., only when a specific query param is present), the first request might not trigger detection, leading to a cached MISS response. A subsequent request with the param would be served from cache (stale), trigger background regen with a raw Request, and the regen would also miss the detection — silently caching a stale response.
I'd suggest wrapping the synthetic request in the proxy too, for defense in depth:
const __syntheticReq = __proxyRouteRequest(
new Request(__revalUrl, { method: "GET" }),
markDynamicUsage
);Question: First-request race condition (concurrent MISSes)
When N concurrent requests arrive for a not-yet-cached dynamic route handler:
- All N check
__dynamicRouteHandlers.has(pattern)→ false (not yet known) - All N check the ISR cache → MISS
- All N execute the handler, all N detect dynamic usage
- All N add to
__dynamicRouteHandlersand skip cache write
This is actually fine — all requests get fresh responses, and subsequent requests skip the cache. The Set is idempotent. The only cost is N redundant handler executions, which is unavoidable without a lock. No action needed, just confirming this is benign.
Minor: Tests are code-gen string assertions only
The new tests verify the generated code contains the right string snippets, which is consistent with the existing test style in this file. For completeness, an integration test that actually exercises the proxy behavior (sends a request, verifies no caching when request.headers is accessed) would be valuable, but that could be a follow-up.
Summary: Please wrap the synthetic request in __proxyRouteRequest in the background regeneration path. The rest looks good.
|
Review posted on PR #610 with a "request changes" verdict. Here's the summary: One actionable issue found:
Confirmed non-issues:
|
Address bigbonk review: the background regen path was passing a raw Request to the handler, bypassing the dynamic detection Proxy. If a handler conditionally accesses request.headers (e.g. only for certain query params), the regen path would miss the detection and silently cache a stale response. Now wraps the synthetic request in __proxyRouteRequest for defense in depth, matching the primary handler execution path.
|
/bigbonk Addressed: wrapped the synthetic request in |
There was a problem hiding this comment.
Review: Dynamic API detection for route handlers
Good PR. The approach is correct — runtime Proxy detection of dynamic API access on the Request object matches how Next.js detects dynamic usage, and the three-piece design (__proxyRouteRequest, __dynamicRouteHandlers Set, cache read guard) is clean and well-integrated with the existing markDynamicUsage()/consumeDynamicUsage() plumbing.
What I verified
Correctness of the Proxy implementation (app-rsc-entry.ts:509-534): The Proxy correctly intercepts .headers and .nextUrl.searchParams while passing through all other properties. The _nextUrl memoization avoids creating a new URL/Proxy on every .nextUrl access, which is good. The Reflect.get(target, prop, receiver) usage is correct throughout — maintains proper this binding.
Background regeneration path is covered (app-rsc-entry.ts:2342-2344): The previous review flagged that the synthetic request in the STALE/regen path wasn't wrapped. This has been fixed — __proxyRouteRequest(new Request(...), markDynamicUsage) is used, and the regen path now also adds to __dynamicRouteHandlers when dynamic usage is detected (line 2350). This closes the conditional-dynamic-access edge case.
Concurrency is safe: markDynamicUsage() and consumeDynamicUsage() operate on per-request state via AsyncLocalStorage (confirmed in headers.ts:48-54,83-84,138-142), so concurrent requests don't interfere with each other's dynamic detection. The __dynamicRouteHandlers Set is module-level but only written to with Set.add() which is idempotent — concurrent MISS requests all detect dynamic usage independently and all add the same pattern.
Dev/prod parity: The App Router prod server (prod-server.ts:728-747) delegates entirely to rscHandler(request), which is the handler generated by app-rsc-entry.ts. The worker entry does the same. No parallel implementation to keep in sync. This is different from Pages Router where dev-server.ts and prod-server.ts have independent pipelines.
Cache read guard is correct (app-rsc-entry.ts:2300-2306): The !__dynamicRouteHandlers.has(handler.pattern) check is in the right position — after the revalidateSeconds !== null and force-dynamic checks, before the cache read. Once a handler is known-dynamic, all future requests skip straight to handler execution.
Cache write guard is correct (app-rsc-entry.ts:2412-2418): The write path checks !dynamicUsedInHandler (the per-request flag), so even the first request that discovers dynamic usage skips the cache write.
One minor observation (non-blocking)
The Proxy's nextUrl implementation creates a new URL(target.url) to back the proxied object. This means request.nextUrl.pathname returns the full URL pathname (which includes basePath if present). In Next.js, NextRequest.nextUrl is a NextURL instance that has basePath-aware behavior. For ISR route handlers this is fine since nobody caches based on nextUrl.pathname from the proxy — it's only used for dynamic detection. But if someone later extends the proxy to cover more NextURL properties, this could diverge. Not a concern for this PR.
Tests
The new tests (app-router.test.ts:3830-3845) are code-gen string assertions, which is consistent with the existing test style in this describe block. They verify the generated code contains the right structural pieces. An integration test exercising the actual Proxy behavior end-to-end would be a nice follow-up but isn't blocking.
LGTM — clean implementation that correctly matches Next.js semantics.
|
Review posted on PR #610 with an approve verdict. Here's the summary: The PR correctly implements runtime dynamic API detection for route handlers via a Key findings:
One non-blocking observation: the proxy's |
The generated RSC entry is JavaScript, not TypeScript. Rolldown's parser rejected `new Set<string>()` and typed function parameters as invalid JS. Use JSDoc annotations and untyped parameters instead.
|
Web API objects like Request use internal slots for methods like .json(), .text(), .arrayBuffer(). When accessed through a Proxy, the method's 'this' becomes the Proxy instead of the original Request, causing internal slot checks to fail with a TypeError. Bind all function properties to the original target, and access .headers directly from the target instead of via Reflect to avoid the same internal slot issue with the Headers object.
The consumeDynamicUsage() call after the handler was picking up dynamic usage flags set by earlier pipeline stages (middleware, headers context setup), causing static handlers to be incorrectly marked as dynamic and preventing ISR caching. Clear the flag immediately before handler execution so only the handler's own dynamic API access is detected.
The background regeneration pipeline (setNavigationContext, fetch patching, etc.) was setting dynamic usage flags before the handler ran, causing static handlers to be incorrectly marked as dynamic during regen. This added the pattern to __dynamicRouteHandlers, blocking all subsequent cache reads. Clear the flag before handler execution in both the primary handler path and the background regen path.
…dynamic detection Two improvements that make route handler ISR easier to reason about: 1. Dynamic usage counter: replace the boolean dynamicUsageDetected flag with a dynamicUsageCount counter and getDynamicUsageCount() reader. Handler execution is bracketed with before/after counter reads, so pipeline stages that call headers()/cookies() don't contaminate the handler's dynamic detection. No more fragile clear-before/ read-after pattern. 2. __handleRouteWithIsrCache wrapper: extract all ISR cache logic (cache read, dynamic detection, cache write, stale-while-revalidate background regen) into a single function. Previously this was ~150 lines of inlined code with the same dynamic detection pattern repeated in the primary path and regen path. Now there is one place for the cache logic and one place for dynamic scoping.
Tests still referenced the old dynamicUsageDetected boolean field. Updated to use dynamicUsageCount (number): false->0, true->1.
|
/bigbonk review |
There was a problem hiding this comment.
Review: Dynamic API detection for route handlers
Clean implementation. The boolean-to-counter migration and the extracted __handleRouteWithIsrCache wrapper are both well-motivated and correctly implemented.
What I verified
Counter-based bracketing is correct and non-disruptive. The getDynamicUsageCount() before/after pattern (app-rsc-entry.ts:640-642, 603-605) correctly isolates dynamic detection to a specific handler execution without resetting the counter — unlike the old consumeDynamicUsage() which is destructive. The page-rendering path at app-rsc-entry.ts:3254 still uses consumeDynamicUsage() (reset-on-read), which remains correct there since it runs once at the end of the render. The two approaches don't interfere because they operate on separate request scopes via AsyncLocalStorage.
Proxy implementation is sound. The __proxyRouteRequest function (app-rsc-entry.ts:510-542) correctly:
- Uses
Reflect.get(target, prop)for non-intercepted properties (preserves prototype chain) - Binds methods to
target(avoids internal slot violations onRequest.json(),.text(), etc.) - Memoizes
_nextUrlto avoid per-access URL/Proxy allocation - Only marks
.headersand.nextUrl.searchParamsas dynamic —.method,.url,.nextUrl.pathnamepass through without triggering detection
Background regen is covered. The STALE path (app-rsc-entry.ts:598-601) wraps the synthetic request in __proxyRouteRequest and brackets with getDynamicUsageCount() before/after (603-605), matching the primary handler execution path. The previous review's bug has been addressed.
__dynamicRouteHandlers Set growth is bounded. Entries are keyed by route pattern (e.g., /api/users/[id]), not by request URL. The set size is bounded by the number of route files in the app.
Dev/prod parity is fine. The App Router prod server and worker entry both delegate to the generated RSC entry handler, so this fix applies everywhere without parallel implementations to sync.
One minor note (non-blocking)
The __handleRouteWithIsrCache function parameter opts is untyped (plain JS in generated code), which is expected for generated entry code. The JSDoc comment above it is sufficient documentation.
LGTM.
|
Review posted on PR #610 with an approve verdict. Summary of my reviewThe PR correctly implements runtime dynamic API detection for route handlers, matching Next.js semantics where handlers that access Key findings:
|
…rtions Use consumeDynamicUsage() to reset the counter to 0 before handler execution, then check getDynamicUsageCount() > 0 after. This is simpler and more reliable than the before/after comparison since it guarantees a clean baseline. Also fix the last boolean assertion in the concurrent isolation test (expected 1 to be true -> expected 1 to be 1).
The counter-based approach (getDynamicUsageCount() > 0) was causing ISR cache write failures in production tests. Revert to the proven consumeDynamicUsage() boolean pattern that the old inline code used successfully. The counter infrastructure remains available via getDynamicUsageCount() for future use. Both the primary handler path and regen path now use: consumeDynamicUsage(); // reset before handler // ... run handler ... const dynamicUsed = consumeDynamicUsage(); // read after handler
1 participant
Summary
Route handlers that access
request.headersorrequest.nextUrl.searchParamsare dynamic and should not be served from the ISR cache. This PR adds proper dynamic detection matching Next.js behavior, replacing the query-string-in-cache-key approach from PR #603.How it works
Three new mechanisms:
__proxyRouteRequest: Wraps theRequestpassed to route handlers in a Proxy. Accessing.headersor.nextUrl.searchParamscallsmarkDynamicUsage(), which the existingconsumeDynamicUsage()check on the write path already respects.__dynamicRouteHandlers: A module-levelSet<string>that remembers route patterns whose handlers used dynamic APIs. On the first request (cache MISS), the handler runs and dynamic usage is detected. On subsequent requests, the cache read path skips the cache for known-dynamic handlers.Cache read guard: The ISR cache read condition now includes
!__dynamicRouteHandlers.has(handler.pattern), preventing stale cache hits for dynamic handlers.Why not query-string-in-cache-key?
PR #603 took a different approach: including the query string in the ISR cache key. James pointed out that this diverges from Next.js, which uses pathname-only cache keys (source). In Next.js, handlers that read searchParams are detected as dynamic and never cached at all.
This PR matches that behavior: dynamic handlers are never cached, static handlers use pathname-only keys.
Granularity
The Proxy is granular about what triggers dynamic detection:
request.methodrequest.url(string)request.nextUrl.pathnamerequest.nextUrl.searchParamsrequest.headersThe one gap (same as Next.js):
new URL(request.url).searchParamsbypasses the Proxy since it operates on the raw URL string. Developers should userequest.nextUrl.searchParamsinstead.