-
Notifications
You must be signed in to change notification settings - Fork 258
fix: detect dynamic API usage in route handlers to prevent stale ISR cache hits #613
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -495,6 +495,48 @@ const __isrDebug = process.env.NEXT_PRIVATE_DEBUG_CACHE | |
| ? console.debug.bind(console, "[vinext] ISR:") | ||
| : undefined; | ||
|
|
||
| // Track route handler patterns that have used dynamic APIs (headers, cookies, | ||
| // searchParams). On first request (cache MISS), the handler runs and we detect | ||
| // 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(); | ||
|
|
||
| // Wrap the Request passed to route handlers in a Proxy that detects dynamic | ||
| // API access. Accessing request.headers or request.nextUrl.searchParams marks | ||
| // the handler as dynamic, preventing ISR caching. | ||
| function __proxyRouteRequest(req, markDynamic) { | ||
| let _nextUrl; | ||
| return new Proxy(req, { | ||
| get(target, prop, receiver) { | ||
| if (prop === "headers") { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: |
||
| markDynamic(); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Every let _marked = false;
const markOnce = () => { if (!_marked) { _marked = true; markDynamic(); } }; |
||
| return target.headers; | ||
| } | ||
| if (prop === "nextUrl") { | ||
| if (!_nextUrl) { | ||
| const realUrl = new URL(target.url); | ||
| _nextUrl = new Proxy(realUrl, { | ||
| get(urlTarget, urlProp) { | ||
| if (urlProp === "searchParams") markDynamic(); | ||
| const val = Reflect.get(urlTarget, urlProp); | ||
| return typeof val === "function" ? val.bind(urlTarget) : val; | ||
| }, | ||
| }); | ||
| } | ||
| return _nextUrl; | ||
| } | ||
| // Bind methods to the original Request to preserve internal slots. | ||
| // Web API objects like Request use internal slots for .json(), | ||
| // .text(), .arrayBuffer(), etc. that break when called through | ||
| // a Proxy (the Proxy becomes the receiver instead of the real Request). | ||
| const value = Reflect.get(target, prop); | ||
| if (typeof value === "function") return value.bind(target); | ||
| return value; | ||
| }, | ||
| }); | ||
| } | ||
|
|
||
| // Normalize null-prototype objects from matchPattern() into thenable objects | ||
| // that work both as Promises (for Next.js 15+ async params) and as plain | ||
| // objects with synchronous property access (for pre-15 code like params.id). | ||
|
|
@@ -2257,11 +2299,12 @@ async function _handleRequest(request, __reqCtx, _mwCtx) { | |
|
|
||
| // ISR cache read for route handlers (production only). | ||
| // Only GET/HEAD (auto-HEAD) with finite revalidate > 0 are ISR-eligible. | ||
| // This runs before handler execution so a cache HIT skips the handler entirely. | ||
| // Skip cache read if this handler was previously detected as dynamic. | ||
| if ( | ||
| process.env.NODE_ENV === "production" && | ||
| revalidateSeconds !== null && | ||
| handler.dynamic !== "force-dynamic" && | ||
| !__dynamicRouteHandlers.has(handler.pattern) && | ||
| (method === "GET" || isAutoHead) && | ||
| typeof handlerFn === "function" | ||
| ) { | ||
|
|
@@ -2299,11 +2342,15 @@ async function _handleRequest(request, __reqCtx, _mwCtx) { | |
| await _runWithUnifiedCtx(__revalUCtx, async () => { | ||
| _ensureFetchPatch(); | ||
| setNavigationContext({ pathname: cleanPathname, searchParams: __revalSearchParams, params: __revalParams }); | ||
| const __syntheticReq = new Request(__revalUrl, { method: "GET" }); | ||
| const __syntheticReq = __proxyRouteRequest( | ||
| new Request(__revalUrl, { method: "GET" }), | ||
| markDynamicUsage, | ||
| ); | ||
| const __revalResponse = await __revalHandlerFn(__syntheticReq, { params: __revalParams }); | ||
| const __regenDynamic = consumeDynamicUsage(); | ||
| setNavigationContext(null); | ||
| if (__regenDynamic) { | ||
| __dynamicRouteHandlers.add(handler.pattern); | ||
| __isrDebug?.("route regen skipped (dynamic usage)", cleanPathname); | ||
| return; | ||
| } | ||
|
|
@@ -2336,11 +2383,17 @@ async function _handleRequest(request, __reqCtx, _mwCtx) { | |
|
|
||
| if (typeof handlerFn === "function") { | ||
| const previousHeadersPhase = setHeadersAccessPhase("route-handler"); | ||
| const __proxiedRequest = __proxyRouteRequest(request, markDynamicUsage); | ||
| try { | ||
| const response = await handlerFn(request, { params }); | ||
| const dynamicUsedInHandler = consumeDynamicUsage(); | ||
| const handlerSetCacheControl = response.headers.has("cache-control"); | ||
|
|
||
| // Remember this handler as dynamic so future requests skip cache read. | ||
| if (dynamicUsedInHandler) { | ||
| __dynamicRouteHandlers.add(handler.pattern); | ||
| } | ||
|
|
||
| // Apply Cache-Control from route segment config (export const revalidate = N). | ||
| // Runtime request APIs like headers() / cookies() make GET handlers dynamic, | ||
| // so only attach ISR headers when the handler stayed static. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -34,7 +34,7 @@ export type HeadersAccessPhase = "render" | "action" | "route-handler"; | |
|
|
||
| export type VinextHeadersShimState = { | ||
| headersContext: HeadersContext | null; | ||
| dynamicUsageDetected: boolean; | ||
| dynamicUsageCount: number; | ||
| pendingSetCookies: string[]; | ||
| draftModeCookieHeader: string | null; | ||
| phase: HeadersAccessPhase; | ||
|
|
@@ -55,7 +55,7 @@ const _als = (_g[_ALS_KEY] ??= | |
|
|
||
| const _fallbackState = (_g[_FALLBACK_KEY] ??= { | ||
| headersContext: null, | ||
| dynamicUsageDetected: false, | ||
| dynamicUsageCount: 0, | ||
| pendingSetCookies: [], | ||
| draftModeCookieHeader: null, | ||
| phase: "render", | ||
|
|
@@ -81,7 +81,11 @@ function _getState(): VinextHeadersShimState { | |
| * Called by connection(), cookies(), headers(), and noStore(). | ||
| */ | ||
| export function markDynamicUsage(): void { | ||
| _getState().dynamicUsageDetected = true; | ||
| _getState().dynamicUsageCount++; | ||
| } | ||
|
|
||
| export function getDynamicUsageCount(): number { | ||
| return _getState().dynamicUsageCount; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| } | ||
|
|
||
| // --------------------------------------------------------------------------- | ||
|
|
@@ -137,8 +141,8 @@ export function throwIfInsideCacheScope(apiName: string): void { | |
| */ | ||
| export function consumeDynamicUsage(): boolean { | ||
| const state = _getState(); | ||
| const used = state.dynamicUsageDetected; | ||
| state.dynamicUsageDetected = false; | ||
| const used = state.dynamicUsageCount > 0; | ||
| state.dynamicUsageCount = 0; | ||
| return used; | ||
| } | ||
|
|
||
|
|
@@ -182,7 +186,7 @@ export function setHeadersContext(ctx: HeadersContext | null): void { | |
| const state = _getState(); | ||
| if (ctx !== null) { | ||
| state.headersContext = ctx; | ||
| state.dynamicUsageDetected = false; | ||
| state.dynamicUsageCount = 0; | ||
| state.pendingSetCookies = []; | ||
| state.draftModeCookieHeader = null; | ||
| state.phase = "render"; | ||
|
|
@@ -209,7 +213,7 @@ export function runWithHeadersContext<T>( | |
| if (isInsideUnifiedScope()) { | ||
| return runWithUnifiedStateMutation((uCtx) => { | ||
| uCtx.headersContext = ctx; | ||
| uCtx.dynamicUsageDetected = false; | ||
| uCtx.dynamicUsageCount = 0; | ||
| uCtx.pendingSetCookies = []; | ||
| uCtx.draftModeCookieHeader = null; | ||
| uCtx.phase = "render"; | ||
|
|
@@ -218,7 +222,7 @@ export function runWithHeadersContext<T>( | |
|
|
||
| const state: VinextHeadersShimState = { | ||
| headersContext: ctx, | ||
| dynamicUsageDetected: false, | ||
| dynamicUsageCount: 0, | ||
| pendingSetCookies: [], | ||
| draftModeCookieHeader: null, | ||
| phase: "render", | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This
Setis 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.