fix(app-router): keep dynamic usage visible across nested request scopes - #3068
Open
NathanDrake2406 wants to merge 1 commit into
Open
fix(app-router): keep dynamic usage visible across nested request scopes#3068NathanDrake2406 wants to merge 1 commit into
NathanDrake2406 wants to merge 1 commit into
Conversation
`runWithUnifiedStateMutation()` shallow-clones the unified request context, so `dynamicUsageDetected` forked per nested scope. Cacheability is decided once per request, and nested scopes wrap lazily consumed SSR/RSC streams, so a `cookies()`, `headers()`, or `noStore()` call that lands after the scope callback settles was invisible to `consumeDynamicUsage()`. Alias the flag to the parent context instead of copying it. Scopes that deliberately measure a subtree now opt out through `isolateDynamicUsage()`.
Contributor
Author
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
commit: |
NathanDrake2406
marked this pull request as ready for review
August 23, 2026 13:30
|
You have reached your Codex usage limits for security reviews. Please try again later. |
Contributor
Performance benchmarksCompared 0 improved · 0 regressed · 6 within ±1.5%
View detailed results and traces 🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Overview
dynamicUsageDetecteda property of the request, not of whichever nested ALS scope happens to be active.runWithUnifiedStateMutation()aliases the flag to the parent context instead of copying it by value.isolateDynamicUsage().cookies(),headers(), ornoStore()call that happens inside a nested scope, or in async work that scope spawned, now reaches the cache policy.Why
Cacheability is decided once per request.
consumeDynamicUsage()is the control that forcesno-storeand skips ISR writes when a render touches request data, so everymarkDynamicUsage()in that request must reach it.runWithUnifiedStateMutation()builds a child scope with{ ...parentCtx }. The file already documents the hazard for reference-typed fields, which leak into the parent.dynamicUsageDetectedis a primitive and has the mirror hazard: the child gets a private copy, and writes to that copy never reach the parent.Two properties make this observable rather than theoretical:
handleSsr()runs insiderunWithNavigationContext()and then insiderunWithRootParamsScope(), but the HTTP layer pulls the HTML stream after both callbacks settle.app-page-render.tsdocuments this timing where it defersclearRequestContext()until the stream drains.renderAppPageLifecycle()andfinalizeAppPageHtmlCacheResponse()both calloptions.consumeDynamicUsage()from outside those scopes.Concretely: a render enters a nested scope, returns a stream handle, and the scope callback settles. The HTTP layer then pulls the body, and a component that runs during that pull calls
cookies(). The flag is set on the discarded child copy. The outerconsumeDynamicUsage()returnsfalse, and the response is treated as cacheable.A fix in
runWithRootParamsScope()alone would be a symptom patch.runWithNavigationContext()wraps the same SSR render one level further out and reproduces the identical failure, and seven other wrappers build child scopes the same way.unified-request-context.tsdynamicUsageDetectedto the parent so nested scopes cannot fork it.headers.tsrunWithIsolatedDynamicUsage()andrunWithHeadersContext()deliberately start a fresh dynamic-usage measurement.isolateDynamicUsage()and keep their current isolation.app-layout-param-observation.tsisolateDynamicUsage()and keeps its current isolation.What changed
markDynamicUsage()inside any nested scopemarkDynamicUsage()from async work that outlives the nested scope callbackrunWithIsolatedDynamicUsage()runWithHeadersContext()runWithConnectionProbe()setHeadersContext()Maintainer review path
packages/vinext/src/shims/unified-request-context.tsfor the ownership decision:aliasDynamicUsageToParent(),isolateDynamicUsage(), and the call added torunWithUnifiedStateMutation().packages/vinext/src/shims/headers.tsandpackages/vinext/src/server/app-layout-param-observation.tsfor the three deliberate opt-outs.tests/unified-request-context.test.tsfor the owning-layer contract in both directions.tests/root-params.test.tsfor the reported caller.Validation
runWithIsolatedDynamicUsage()still measures its subtree without marking the request.runWithRootParamsScope()reachesconsumeDynamicUsage().runWithHeadersContext()sub-state test intests/unified-request-context.test.ts, and the layout probe test intests/app-layout-param-observation.test.ts.vp check. No formatting, lint, or type errors in 1289 files.Commands and extended results
Risk / compatibility
UnifiedRequestContextkeeps thedynamicUsageDetectedfield name andbooleantype, so no caller or existing test needed updating.isolateDynamicUsage()is a new internal export fromvinext/shims/unified-request-context.Object.definePropertywithenumerable: true, so a child context still spreads correctly into further nested scopes. Nested aliases chain up to the nearest isolating ancestor.Non-goals
dynamicUsageDetectedinto a shared holder object. That is the idiom this file already uses for reference-typed slices, but it renames a field used in about 20 places and in thecreateRequestContext()options, which mixes a mechanical refactor into a behavioural fix.UnifiedRequestContextfor the same copy-by-value question.phase,actionRevalidationKind, and thecurrentFetch*fields are genuinely scope-local today.