fix: detect dynamic API usage in route handlers to prevent stale ISR cache hits#613
fix: detect dynamic API usage in route handlers to prevent stale ISR cache hits#613southpolesteve wants to merge 2 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 ISR cache had no mechanism to skip the cache read for handlers that use dynamic APIs, which could serve one user's cached response to another. Changes: 1. __proxyRouteRequest: wraps the Request passed to route handlers in a Proxy. Accessing .headers or .nextUrl.searchParams calls markDynamicUsage(). Methods are bound to the original target to preserve Web API internal slots (.json(), .text(), etc.). 2. __dynamicRouteHandlers: a module-level Set that remembers route patterns whose handlers used dynamic APIs. The ISR cache read condition checks this set and skips the cache for known-dynamic handlers. 3. dynamicUsageCount: replaces the boolean dynamicUsageDetected flag with a counter, enabling future counter-based bracketing of handler execution. consumeDynamicUsage() now checks count > 0 and resets to 0. This matches Next.js behavior where dynamic route handlers are never cached. Static handlers continue to use pathname-only cache 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.
Good approach overall. The proxy-based dynamic detection matches Next.js semantics well, and the internal-slots-aware method binding is a nice touch. A few observations:
Unused export: getDynamicUsageCount()
getDynamicUsageCount() is exported from headers.ts but nothing in this PR (or the codebase) calls it. The PR description mentions "enabling future counter-based bracketing" — if there's no immediate consumer, consider removing it to avoid dead code. It's trivial to add back when needed.
__dynamicRouteHandlers is never cleared
The Set grows monotonically for the process lifetime. Once a handler is marked dynamic, it stays dynamic forever. This is correct for handlers that always read headers/searchParams, but consider an edge case: a handler that conditionally reads request.headers only for certain request patterns. After one dynamic hit, it's permanently evicted from cache.
In practice this matches Next.js (which determines dynamism statically at build time, so conditional access doesn't matter). But it's worth a comment on the Set noting this is intentional.
Missing test for proxy-based detection
The existing test at tests/app-router.test.ts:1726 covers headers() (the shim import) preventing caching, but there's no test that verifies a route handler reading request.headers directly (without importing from next/headers) is detected as dynamic. This is the core new behavior — a dedicated test fixture with a handler like export async function GET(request) { const auth = request.headers.get('authorization'); ... } would give confidence the proxy works end-to-end.
Similarly, no test covers request.nextUrl.searchParams triggering dynamic detection.
request.cookies is not trapped
The PR description mentions tracking "headers, cookies, searchParams" but the proxy only intercepts .headers and .nextUrl.searchParams. Standard Request objects don't have .cookies, so this is fine as long as vinext passes a standard Request (not NextRequest) to route handlers — which it does (line 2386 passes the original request through the proxy). The cookies() shim from next/headers already calls markDynamicUsage() separately. No action needed, just noting for completeness.
| } | ||
|
|
||
| export function getDynamicUsageCount(): number { | ||
| return _getState().dynamicUsageCount; |
There was a problem hiding this comment.
This function is exported but has no callers in the codebase. If the counter-based bracketing pattern isn't landing in this PR, consider deferring this export to avoid dead code.
| // dynamic usage. On subsequent requests, we skip the cache read for handlers | ||
| // in this set, matching Next.js behavior where dynamic handlers are never cached. | ||
| /** @type {Set<string>} */ | ||
| const __dynamicRouteHandlers = new Set(); |
There was a problem hiding this comment.
This Set is append-only and never cleared. Worth adding a brief comment noting this is intentional (matches Next.js static analysis behavior) — if a handler ever accesses dynamic APIs, it's permanently marked dynamic for the process lifetime.
Edge case to consider: a handler that conditionally reads request.headers (e.g., only for authenticated requests) will be permanently evicted from cache after its first dynamic hit, even if subsequent requests wouldn't access headers. This matches Next.js but is a subtle behavioral contract.
| let _nextUrl; | ||
| return new Proxy(req, { | ||
| get(target, prop, receiver) { | ||
| if (prop === "headers") { |
There was a problem hiding this comment.
Nit: receiver is declared but never used. Could simplify to get(target, prop) since Reflect.get(target, prop) on line 533 doesn't pass receiver either (intentionally, to avoid the Proxy-as-receiver problem with internal slots).
| return new Proxy(req, { | ||
| get(target, prop, receiver) { | ||
| if (prop === "headers") { | ||
| markDynamic(); |
There was a problem hiding this comment.
Every .headers access increments the counter. In a handler that reads headers in a loop or accesses it multiple times (common pattern: request.headers.get('auth') + request.headers.get('content-type')), this fires multiple times. Functionally harmless since consumeDynamicUsage() checks > 0, but if the counter is intended for future bracketing purposes, the multiple increments from a single "conceptual" dynamic access could be surprising. A markDynamic-once pattern (boolean guard in the closure) would be more precise:
let _marked = false;
const markOnce = () => { if (!_marked) { _marked = true; markDynamic(); } };
|
Review posted successfully on PR #613. Here's a summary of the feedback: Overall: The proxy-based dynamic detection is a solid approach that correctly matches Next.js semantics. The internal-slots-aware method binding on the Request proxy is well thought out. Key feedback points:
|
|
Superseded by v3 branch with minimal diff (no counter rename). |
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.Changes
__proxyRouteRequest: Wraps theRequestpassed to route handlers in a Proxy. Accessing.headersor.nextUrl.searchParamscallsmarkDynamicUsage(). Methods are bound to the original target to preserve Web API internal slots (.json(),.text(), etc.).__dynamicRouteHandlers: A module-levelSetthat remembers route patterns whose handlers used dynamic APIs. The ISR cache read condition checks this set and skips the cache for known-dynamic handlers.dynamicUsageCount: Replaces the booleandynamicUsageDetectedflag with a counter, enabling future counter-based bracketing of handler execution.Why not query-string-in-cache-key?
James pointed out that including query strings in the cache key diverges from Next.js, which uses pathname-only keys (source). In Next.js, handlers that read searchParams are detected as dynamic and never cached at all. This PR matches that behavior.
Proxy granularity
request.methodrequest.url(string)request.nextUrl.pathnamerequest.nextUrl.searchParamsrequest.headersThe one gap (same as Next.js):
new URL(request.url).searchParamsbypasses the Proxy.