Skip to content

fix(app-router): keep dynamic usage visible across nested request scopes - #3068

Open
NathanDrake2406 wants to merge 1 commit into
cloudflare:mainfrom
NathanDrake2406:nathan/fix-nested-scope-dynamic-usage
Open

fix(app-router): keep dynamic usage visible across nested request scopes#3068
NathanDrake2406 wants to merge 1 commit into
cloudflare:mainfrom
NathanDrake2406:nathan/fix-nested-scope-dynamic-usage

Conversation

@NathanDrake2406

Copy link
Copy Markdown
Contributor

Overview

Goal Keep dynamicUsageDetected a property of the request, not of whichever nested ALS scope happens to be active.
Core change runWithUnifiedStateMutation() aliases the flag to the parent context instead of copying it by value.
Key boundary The unified request context owns the flag. Scopes that measure a subtree opt out through the new isolateDynamicUsage().
Expected impact A cookies(), headers(), or noStore() 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 forces no-store and skips ISR writes when a render touches request data, so every markDynamicUsage() 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. dynamicUsageDetected is 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:

  1. Nested scopes wrap lazily consumed streams. handleSsr() runs inside runWithNavigationContext() and then inside runWithRootParamsScope(), but the HTTP layer pulls the HTML stream after both callbacks settle. app-page-render.ts documents this timing where it defers clearRequestContext() until the stream drains.
  2. The cache decision runs in the outer lifecycle. renderAppPageLifecycle() and finalizeAppPageHtmlCacheResponse() both call options.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 outer consumeDynamicUsage() returns false, 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.

Area Principle / invariant What this PR changes
unified-request-context.ts A nested scope shares request-level state and replaces only the slice it overrides. Aliases dynamicUsageDetected to the parent so nested scopes cannot fork it.
headers.ts runWithIsolatedDynamicUsage() and runWithHeadersContext() deliberately start a fresh dynamic-usage measurement. Both opt out through isolateDynamicUsage() and keep their current isolation.
app-layout-param-observation.ts The layout probe measures one subtree and folds the result into its own observation. Opts out through isolateDynamicUsage() and keeps its current isolation.

What changed

Scenario Before After
markDynamicUsage() inside any nested scope Marked the child copy only. Marks the request.
markDynamicUsage() from async work that outlives the nested scope callback Lost when the child scope was discarded. Marks the request.
runWithIsolatedDynamicUsage() Measured its subtree without marking the request. Unchanged.
runWithHeadersContext() Started a fresh flag and restored the outer value on exit. Unchanged.
Layout param probe Isolated its flag and folded the result into the observation. Unchanged.
runWithConnectionProbe() Copied the child flag to the parent when the probe finished. Unchanged in effect. The copy is now a no-op because both read the same flag.
setHeadersContext() Reset the flag on the current state at request entry. Unchanged. Every call site runs at the outer level.
Maintainer review path
  1. packages/vinext/src/shims/unified-request-context.ts for the ownership decision: aliasDynamicUsageToParent(), isolateDynamicUsage(), and the call added to runWithUnifiedStateMutation().
  2. packages/vinext/src/shims/headers.ts and packages/vinext/src/server/app-layout-param-observation.ts for the three deliberate opt-outs.
  3. tests/unified-request-context.test.ts for the owning-layer contract in both directions.
  4. tests/root-params.test.ts for the reported caller.
Validation
  • Added a regression test at the owning layer: late dynamic usage from work that outlives a nested scope callback reaches the request context.
  • Added a regression test for the opposite direction: runWithIsolatedDynamicUsage() still measures its subtree without marking the request.
  • Added a regression test for the reported caller: dynamic usage during work that outlives runWithRootParamsScope() reaches consumeDynamicUsage().
  • Confirmed the two new failure cases are red on the parent commit and green after the change.
  • Existing isolation contracts stay green without modification: the runWithHeadersContext() sub-state test in tests/unified-request-context.test.ts, and the layout probe test in tests/app-layout-param-observation.test.ts.
  • Ran 32 targeted suites covering request context, shims, App Router render, dispatch, probe, cache, ISR, prerender, route handlers, and draft mode. 2093 tests passed.
  • Ran vp check. No formatting, lint, or type errors in 1289 files.
Commands and extended results
vp test run tests/root-params.test.ts tests/unified-request-context.test.ts \
  tests/app-layout-param-observation.test.ts tests/cache-for-request.test.ts
  Test Files  4 passed (4)
       Tests  52 passed (52)

vp test run tests/shims.test.ts tests/app-page-render.test.ts tests/app-page-cache.test.ts \
  tests/app-page-cache-render.test.ts tests/app-page-dispatch.test.ts tests/app-page-probe.test.ts \
  tests/app-page-execution.test.ts tests/app-route-handler-execution.test.ts \
  tests/app-route-handler-cache.test.ts tests/fetch-cache.test.ts tests/app-request-context.test.ts \
  tests/app-prerender-static-params.test.ts tests/cloudflare-cdn-cache.test.ts tests/als-registry.test.ts
  Test Files  14 passed (14)
       Tests  1771 passed (1771)

vp test run tests/isr-cache.test.ts tests/isr-decision.test.ts tests/cache-proof.test.ts \
  tests/cache-control.test.ts tests/app-static-generation.test.ts tests/app-ppr-fallback-shell.test.ts \
  tests/ppr-fallback-shell.test.ts tests/app-visited-response-cache.test.ts tests/page-cache-tags.test.ts \
  tests/prerender-phase.test.ts tests/prerender-route-params.test.ts \
  tests/app-hydration-cache-publication.test.ts tests/nextjs-compat/draft-mode.test.ts \
  tests/app-page-boundary-render.test.ts
  Test Files  14 passed (14)
       Tests  270 passed (270)

vp check
  pass: Found no warnings, lint errors, or type errors in 1289 files
Risk / compatibility
  • Public API: unchanged. UnifiedRequestContext keeps the dynamicUsageDetected field name and boolean type, so no caller or existing test needed updating. isolateDynamicUsage() is a new internal export from vinext/shims/unified-request-context.
  • Behavioural direction: the change can only make more renders classify as dynamic. It cannot cause a render that previously bypassed the cache to start being cached.
  • Property descriptor: the alias uses Object.defineProperty with enumerable: true, so a child context still spreads correctly into further nested scopes. Nested aliases chain up to the nearest isolating ancestor.
  • Known limit: this change corrects the scope semantics. It does not add an end-to-end fixture that drives a production route through the previously affected path, so the practical exposure of any specific route configuration is not measured here.
Non-goals
  • Converting dynamicUsageDetected into 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 the createRequestContext() options, which mixes a mechanical refactor into a behavioural fix.
  • Auditing the other primitive fields on UnifiedRequestContext for the same copy-by-value question. phase, actionRevalidationKind, and the currentFetch* fields are genuinely scope-local today.

`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()`.
@NathanDrake2406

Copy link
Copy Markdown
Contributor Author

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

@pkg-pr-new

pkg-pr-new Bot commented Aug 23, 2026

Copy link
Copy Markdown

Open in StackBlitz

npm i https://pkg.pr.new/@vinext/cloudflare@3068
npm i https://pkg.pr.new/create-vinext-app@3068
npm i https://pkg.pr.new/@vinext/types@3068
npm i https://pkg.pr.new/vinext@3068

commit: 2924106

@NathanDrake2406
NathanDrake2406 marked this pull request as ready for review August 23, 2026 13:30
@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for security reviews. Please try again later.

@github-actions

Copy link
Copy Markdown
Contributor

Performance benchmarks

Compared 2924106 against base 20fdac4 using alternating same-runner rounds. Next.js was unchanged and skipped.

0 improved · 0 regressed · 6 within ±1.5%

Scenario Framework Baseline Current Change
Client bundle size (gzip) vinext 141.4 KB 141.4 KB ⚫ +0.0%
Client entry size (gzip) vinext 128.8 KB 128.8 KB ⚫ +0.0%
Dev server cold start vinext 3.15 s 3.15 s ⚫ -0.1%
Production build time vinext 3.43 s 3.41 s ⚫ -0.6%
RSC entry closure size (gzip) vinext 115.6 KB 115.7 KB ⚫ +0.1%
Server bundle size (gzip) vinext 196.8 KB 197.0 KB ⚫ +0.1%

View detailed results and traces

🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant