fix(server): evaluate lazy route modules outside the request context - #2740
Conversation
Page and route-handler modules moved from eager RSC-entry evaluation to lazy `() => import()` thunks resolved on the first request that matches the route. A dynamic import propagates AsyncLocalStorage into the imported module's top-level evaluation, and `ensureAppRouteModulesLoaded` is awaited inside `runWithRequestContext`, so module-scope `headers()`/`cookies()` bound to whichever request happened to reach the route first. ESM evaluates a module once per isolate and the namespace is cached on the route with `__loaded = true`, so that first visitor's session data was then reused for every later request. Invoke every lazy thunk through a new `runOutsideRequestContext` (`AsyncLocalStorage.exit()`, which workerd implements) so module scope sees no request. This restores the eager-evaluation contract the lazy loader replaced and matches Next.js, which loads components in `base-server` before `app-render` enters the request store.
|
@codex review |
commit: |
`resolveGlobalNotFoundModule` imports the user's `app/global-not-found.tsx` on the first route-miss 404 and caches the promise for the worker's lifetime. That import runs inside the 404-triggering request's context, so module-scope `headers()`/`cookies()` in global-not-found.tsx bound to that first visitor and were then served to everyone after them — the same mechanism as the lazy route-module thunks, on a sibling call path. Route it through `runOutsideRequestContext` too.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e2f32ff402
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Performance benchmarksCompared 0 improved · 0 regressed · 6 within ±1.5%
View detailed results and traces 🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head |
Exiting only the unified request context was not enough. The Cloudflare entry (`app-router-entry.ts:173`) enters the standalone execution-context ALS *outside* it, and prerendering enters the work-unit store *inside* it, so both stayed visible to module scope. Worse, `after()` takes its `getRequestExecutionContext()` fallback precisely when the unified store is absent — so the partial exit enabled that path instead of closing it, letting top-level `after()` attach to the first request's `waitUntil`. `runOutsideRequestScopes` now exits every ALS handed out by `getOrCreateAls`, which is future-proof by construction: new shims are enrolled as they are created, with no enumeration to keep in sync. `workUnitAsyncStorage` registers itself via `registerAlsForScopeExit` because it must stay module-local (Sentry resolves it by specifier), and registering from that side keeps `als-registry` importing nothing but `node:async_hooks` — it stays evaluable in client bundles where that resolves to a constructor-less stub. Reported by Codex on cloudflare#2740.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d5335ccc6c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The intercepting-route page and not-found modules were imported inline at their call sites — `resolveAppPageInterceptState` in app-page-request.ts and the generated `probePage` path — so they missed the isolation every other route module gets. They are user modules cached on the intercept for the isolate's lifetime, so module-scope `cookies()`/`headers()` could still capture the first visitor to an intercepted route. Move both onto the shared loader as `loadAppInterceptPage` / `loadAppInterceptNotFound`, beside the `loadAppInterceptLayouts` that already lives there. Both call sites now route through one guarded implementation instead of two hand-rolled copies of the same dedup block. Reported by Codex on cloudflare#2740.
|
@codex review |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
/bigbonk review for issues |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: lazy route modules outside request context
Solid, well-reasoned fix. The core insight is correct and important: a dynamic import() propagates AsyncLocalStorage into the imported module's top-level evaluation, so hydrating lazy route modules inside runWithRequestContext(...) lets module-scope cookies()/headers()/after() capture the first request and serve it to every subsequent request in the isolate. I verified this against the described mechanism and the code.
What I checked and confirmed
- Registry-wide exit is the right call.
runOutsideRequestScopescomposingals.exit()over every ALS fromgetOrCreateAls(plus the self-registeredworkUnitAsyncStorage) is correct. Exiting only the unified store would leave the standalone execution-context ALS (entered outside the unified scope in the Cloudflare entry) and the work-unit store (entered inside it during prerender) visible. The observation thatafter()'sgetRequestExecutionContext()fallback fires precisely when the unified store is absent — so a partial exit would enable rather than close that path — is subtle and correct. Good catch, and the nesting-depth test locks it in. - Self-registration direction is correct.
work-unit-async-storage.tsimportingregisterAlsForScopeExit(rather than the registry importing it) keepsals-registry.tsimporting nothing butnode:async_hooks, so it stays client-bundle-safe where that resolves to a constructor-less stub.tests/als-registry.test.tsguards this. - Only-call-site claim holds.
pushFieldLoad/pushArrayLoadsare the wrapped seams, and consolidating the intercept page/not-found loads intoloadAppInterceptPage/loadAppInterceptNotFoundcloses the three previously-inline invocations (app-page-request.ts, generatedprobePage). I swept the remaining() => import()call sites in the generated entry (ssrLoader, route-handler/action/metadata dispatch) — all resolve vinext's own server modules, not user code.app-rsc-route-matching.ts:317reads a cached__loadState.page(no loader invocation), andapp-rsc-handler.ts:931is a truthiness gate, not an invocation. No missed leak paths found. .thenstill runs in request context. Only the loader thunk is wrapped; the assignment continuation runs in the original scope, preserving downstream behavior. Confirmed by theliveCookieassertions in the tests.- Concurrent dedup improved.
hydrateInterceptModulenow assignsintercept[field] = await loadingfor the shared-clone case, fixing a latent gap where a concurrent caller observing the in-flight promise wouldn't get the namespace published onto its own request-local clone. Tested.
Verification
Ran the targeted suites locally — all green:
tests/app-route-module-loader.test.ts (18)
tests/app-page-request.test.ts (38)
tests/als-registry.test.ts (2)
tests/app-fallback-renderer.test.ts (20)
vp check clean on all changed source files. The regression tests are well-designed: reading the live cookie inside the same runWithRequestContext scope before asserting isolation proves the context is genuinely active at the call site (not merely absent), which is the right way to test this.
No blocking issues. One minor note inline.
| * the unified store is absent, so a partial exit would enable that path rather | ||
| * than close it. | ||
| */ | ||
| export function runOutsideRequestScopes<T>(fn: () => T): T { |
There was a problem hiding this comment.
Non-blocking: runOutsideRequestScopes iterates _registry synchronously to build the closure chain, so concurrent getOrCreateAls calls can't mutate the Set mid-iteration (single-threaded, no await between). That's safe today. Worth a one-line comment noting the invariant, since a future refactor that made this async or interleaved an await inside the loop would silently reintroduce a mutation-during-iteration hazard.
Also worth confirming (I believe it holds): any request-scoped ALS must already be registered by the time the loader runs, because entering the request scope necessarily imported the module that called getOrCreateAls. An ALS created for the first time after a request scope is open — if that ever happens — would not be exited. Not a concern for the current stores, but a fragile implicit ordering dependency worth a note near the registry comment.
|
Review posted successfully. Here's a summary of my findings. Review summary for PR #2740Verdict: Approve — no blocking issues. This is a well-reasoned, thorough security fix. The bug (confirmed correct)Making route modules lazy quietly broke an important invariant: a route module's top-level scope must never observe a request. A dynamic What I verified
One minor inline note (non-blocking)Flagged the implicit invariants in |
2 participants
The decision boundary
A route module's top-level scope must not be able to observe a request. It is evaluated exactly once per isolate and its namespace is cached forever, so anything it captures from a request becomes shared state for every user that follows.
That contract held for free while route modules were eagerly imported at RSC-entry evaluation — there was no request in scope to capture. Making them lazy quietly removed it.
The bug
app-rsc-manifest.tsemits page/route-handler modules as() => import(...)thunks, andensureAppRouteModulesLoadedresolves them fromapp-rsc-handler.ts:1167— which is inside therunWithRequestContext(...)scope opened atapp-rsc-handler.ts:1438.A dynamic
import()propagates AsyncLocalStorage into the imported module's top-level evaluation, not just into the calling frame:So for an app with module-scope request-API use:
…the first request to reach
/dashboardbindssessionto its own cookie.ensureAppRouteModulesLoadedthen caches the namespace onroute.pageand sets__loaded = true, so every subsequent request in that isolate skips the import and reads the first visitor's session.Before the lazy change this same code threw
cookies() can only be called from a Server Component, Route Handler, or Server Actionat startup — loud, and correct. Next.js behaves the same way:findPageComponents/loadComponentsruns inbase-server.tsbeforeapp-render.tsxenters the request store viaworkUnitAsyncStorage.run, so module scope never sees a request there either.The fix
runOutsideRequestScopescomposesAsyncLocalStorage.exit()over every ALS handed out bygetOrCreateAls.exit()is one of the methods workerd implements (unlikeenterWith()/disable()), and the registry'sNoopAsyncLocalStoragealready provides it for runtimes withoutnode:async_hooks.Exiting one store is not enough, and that mistake is not fail-safe. vinext's request stores nest at different depths: the Cloudflare entry (
app-router-entry.ts:173) enters the standalone execution-context ALS outside the unified request context, and prerendering enters the work-unit store inside it. Worse,after()takes itsgetRequestExecutionContext()fallback precisely when the unified store is absent — so a unified-only exit would have enabled top-levelafter()to attach to the first request'swaitUntilrather than closing it. Registry-wide exit is also future-proof by construction: new shims enrol themselves as they are created, with no enumeration to keep in sync.Commit 1 — route modules. One guard at the single place every lazy thunk is invoked.
pushFieldLoadandpushArrayLoadsinapp-route-module-loader.tsare the only call sites, so all nineensureRouteLoadedcallers (RSC handler, page dispatch, route-handler dispatch, server actions, intercept layouts) are covered by two lines:Commit 2 — global-not-found.
resolveGlobalNotFoundModuleinapp-fallback-renderer.tsis a sibling call path with the identical mechanism: it imports the user'sapp/global-not-found.tsxon the first route-miss 404, from inside that request's context, and memoizes the promise for the worker's lifetime. Not introduced by the lazy-route change, but the same class of bug on user-authored code, so it is fixed here rather than left behind.In both cases the guard wraps only the import — the
.thenthat assigns the resolved module still runs in the request's context, so nothing downstream changes.Scenario-level behavior
maincookies()/headers()at module scopeafter()at module scopewaitUntilgetRequestExecutionContext()at module scopenullnull, as beforeOnly apps in rows 2–3 change, and they change back to the pre-lazy behavior.
Validation
tests/app-route-module-loader.test.ts— new case driven through a real dynamicimport()oftests/fixtures/module-scope-request-capture.ts, which readscookies()at module scope exactly as a vulnerable app would. The test reads the live cookie inside the samerunWithRequestContextscope first, so a pass proves isolation rather than a merely absent context:Confirmed failing before the fix, passing after — pre-fix:
expected 'read:victim-secret' to be 'rejected-no-request-context'.tests/app-fallback-renderer.test.ts— the same shape for global-not-found, driven throughrenderer.renderNotFound(null, ...)(the route-miss path) inside a live request context. Also confirmed failing before, passing after, with the same pre-fix message.tests/app-route-module-loader.test.ts— a second case for the nesting, built through the realrunWithExecutionContext→runWithRequestContext→ hydration chain: module scope must seegetRequestExecutionContext() === null,after()must throwafter() was called outside a request scope, andwaitUntilmust never be called. Confirmed failing against a unified-only exit.tests/als-registry.test.ts(pre-existing) earned its keep here: an earlier draft importedwork-unit-async-storageinto the registry, and this test caught that it would crash client bundles wherenode:async_hooksresolves to a constructor-less stub. Hence the self-registration direction.Also green:
app-rsc-handler,app-page-route-wiring,app-page-dispatch,app-route-handler-dispatch,app-page-request,app-server-action-execution,app-rsc-route-matching(516 tests), plus the pre-commit fullvp checkand knip on both commits.Review path
packages/vinext/src/shims/internal/als-registry.ts— therunOutsideRequestScopesprimitive and the registry that feeds it.packages/vinext/src/server/app-route-module-loader.ts— the two call sites it wraps; worth confirming these are still the only places a thunk is invoked.packages/vinext/src/server/app-fallback-renderer.ts— the sibling global-not-found path.tests/app-route-module-loader.test.ts+ fixture,tests/app-fallback-renderer.test.ts— the regression tests.