feat: AsyncLocalStorage for ExecutionContext (ctx) propagation#410
feat: AsyncLocalStorage for ExecutionContext (ctx) propagation#410james-elicx merged 1 commit intomainfrom
Conversation
|
20b5bac to
8c88bdc
Compare
commit: |
|
/bigbonk review |
There was a problem hiding this comment.
Review: AsyncLocalStorage for ExecutionContext (ctx) propagation
Overall this is a clean, well-structured PR that follows the established ALS patterns in the codebase. The implementation is correct for the App Router path and provides a solid foundation for waitUntil-based background work. A few observations:
Looks Good
-
ALS pattern is consistent — The
Symbol.for("vinext.requestContext.als")+globalThisstorage pattern matchesheaders.ts,cache.ts, andnavigation-state.ts. The intentional omission of a fallback state (returningnullinstead) is the right design for ExecutionContext, since there's no meaningful default. -
Correct scope placement —
app-router-entry.tswraps at the outermost level (beforerscHandler(request)is called), so the ctx is available throughout the entire request lifecycle including RSC streaming. Thectx ? runWithExecutionContext(ctx, handleFn) : handleFn()ternary correctly handles the Node.js dev case where ctx is undefined. -
KV cache handler fallback —
getRequestExecutionContext() ?? this.ctxin_deleteInBackgroundand_putInBackgroundis the right precedence: prefer ALS (always correct per-request), fall back to constructor-injected ctx. -
Test coverage is thorough — The 8 tests cover the important cases: null outside scope, value inside scope, async propagation, concurrent request isolation, nested scope override/restore, and waitUntil callable. The concurrent request test with staggered timeouts is a good pattern.
Issue: Unused imports in generated RSC entry
app-rsc-entry.ts adds this import to the generated code:
import { runWithExecutionContext as _runWithExecutionContext, getRequestExecutionContext as _getRequestExecutionContext } from "...request-context.js";But neither _runWithExecutionContext nor _getRequestExecutionContext is actually called anywhere in the generated RSC entry module. The RSC entry doesn't need them because the ALS scope is established by app-router-entry.ts before calling rscHandler(request), and getRequestExecutionContext() is called by kv-cache-handler.ts directly (which imports from the shim itself).
These imports are dead code in the generated entry. This won't cause runtime issues, but it adds unnecessary bytes to the RSC bundle and may confuse future readers into thinking the generated entry is responsible for ctx propagation.
Suggestion: Either remove these imports, or if they're intended as a forward-looking hook for #405 (ISR background cache writes in the RSC entry), add a comment explaining that.
Note: Pages Router worker entry doesn't use ALS-based ctx
The generated Pages Router worker entry in deploy.ts threads ctx explicitly through function arguments (runMiddleware(request, ctx), renderPage(request, resolvedUrl, null, ctx)) but does not wrap in runWithExecutionContext(). This means getRequestExecutionContext() returns null in Pages Router Workers.
This is fine today because:
- The KV cache handler falls back to
this.ctxfrom the constructor - The Pages Router production server path already threads ctx explicitly
But it's worth noting that if future code relies on getRequestExecutionContext() without the constructor fallback, it won't work in Pages Router. This could be addressed in a follow-up (not blocking for this PR).
Minor Nits
-
ExecutionContextinterface duplication —app-router-entry.tsdefines its ownExecutionContextinterface (lines 19-22) despite the shim already exportingExecutionContextLike. Consider importing and reusingExecutionContextLikefrom the shim to reduce duplication. Though I understand the interface in app-router-entry.ts predates this PR and includespassThroughOnException()as required (vs optional in the shim), so this is a minor style point. -
passThroughOnExceptionis optional inExecutionContextLike— The Workers runtime always provides it, so marking it optional (passThroughOnException?()) is a deviation from the actual Cloudflare Workers type. This is fine for the minimal structural interface approach, but worth noting that callers who needpassThroughOnExceptionwould need a type guard.
|
Review posted on PR #410. Here's a summary of my findings: Overall: Clean, well-structured PR that correctly follows the established ALS patterns in the codebase. The implementation is sound for the App Router path. Key findings:
|
8c88bdc to
ded215f
Compare
Adds request-context.ts shim with runWithExecutionContext() and
getRequestExecutionContext() so Cloudflare Workers ctx is accessible
anywhere in the call stack without manual threading.
- New shims/request-context.ts with ALS keyed on Symbol.for('vinext.requestContext.als')
- app-router-entry.ts wraps each request in runWithExecutionContext(ctx, ...)
- app-rsc-entry.ts generated code imports the helpers for downstream use
- deploy.ts generated worker entry forwards ctx in fetch(request, env, ctx)
- kv-cache-handler.ts uses getRequestExecutionContext() ?? this.ctx
- 8 new tests in tests/request-context.test.ts
ded215f to
7a3d9a3
Compare
|
/bigbonk re-review |
There was a problem hiding this comment.
Review: AsyncLocalStorage for ExecutionContext propagation
Clean, well-structured PR. The implementation is correct and consistent with the existing ALS patterns in the codebase. Approving with a couple of observations.
What looks good
Consistent ALS pattern. request-context.ts follows the exact same Symbol.for() on globalThis pattern as headers.ts, cache.ts, and navigation-state.ts. The intentional choice to return null (no fallback state) is correct — there's no meaningful default for an ExecutionContext.
Correct scope placement. In app-router-entry.ts, the ALS scope wraps at the outermost level before rscHandler(request), so ctx is available throughout the entire request lifecycle including RSC streaming. The ctx ? runWithExecutionContext(ctx, handleFn) : handleFn() ternary correctly handles Node.js dev where ctx is undefined.
KV cache handler precedence. getRequestExecutionContext() ?? this.ctx is the right order — prefer the per-request ALS context (always correct), fall back to the constructor-injected ctx. This eliminates the need to thread ctx through every function call while maintaining backward compatibility.
Pages Router integration. The runMiddleware and renderPage wrapper functions in pages-server-entry.ts are well-structured — they establish the ALS scope before delegating to the actual implementation (_runMiddleware, _renderPage). This means triggerBackgroundRegeneration and middleware's fetchEvent.drainWaitUntil() can both call _getRequestExecutionContext() directly.
triggerBackgroundRegeneration simplification. Removing the explicit ctx parameter in favor of _getRequestExecutionContext() is a nice cleanup — callers no longer need to thread ctx through to the ISR layer.
Test coverage. The 8 tests cover the essential cases: null outside scope, value inside scope, async propagation, concurrent isolation, nested override/restore, and waitUntil callable. The concurrent test with staggered timeouts is a good pattern for verifying ALS isolation.
Observations (non-blocking)
-
Pages Router worker entry doesn't use
runWithExecutionContextat the top level. The generated Pages Router worker entry indeploy.tspassesctxexplicitly torunMiddleware(request, ctx)andrenderPage(request, resolvedUrl, null, ctx), which then internally wrap withrunWithExecutionContext. This works, but it means code that runs between those calls (e.g., the redirect/rewrite/header matching logic at lines 600-715 ofdeploy.ts) does not have access to the execution context via ALS. This is fine today since none of that code needswaitUntil, but differs from the App Router path where the entire request is wrapped. Worth noting for when future code might want to callgetRequestExecutionContext()from a shared utility that runs in both router contexts. -
passThroughOnExceptionoptionality.ExecutionContextLikemarkspassThroughOnExceptionas optional, while the Workers runtime always provides it. This is fine for the minimal structural interface, but callers wantingpassThroughOnExceptionwould need a type guard. Not an issue for the current usage (onlywaitUntilis called).
|
Approved PR #410. Here's a summary of my review: Verdict: Approve The implementation is clean and correct. It follows the established ALS pattern ( Key strengths:
Two non-blocking observations noted:
|
Summary
Extracts
ExecutionContext(ctx) propagation from #405 into a standalone feature. Adds anAsyncLocalStorage-based mechanism so Cloudflare Workers'ctx(used forwaitUntil) is accessible anywhere in the call stack without manual threading.What changed
shims/request-context.ts— New ALS module keyed onSymbol.for("vinext.requestContext.als"), following the same pattern asheaders.ts,cache.ts, andnavigation-state.ts. ExportsrunWithExecutionContext(ctx, fn)andgetRequestExecutionContext().server/app-router-entry.ts—fetch(request, _env?, ctx?)now wraps the RSC handler inrunWithExecutionContext(ctx, ...)whenctxis present.entries/app-rsc-entry.ts— Generated RSC entry importsrunWithExecutionContextandgetRequestExecutionContextfrom the shim for downstream use.deploy.ts— Generated Cloudflare worker entry updated tofetch(request, env, ctx)and forwardsctxtohandler.fetch(request, env, ctx).cloudflare/kv-cache-handler.ts— UsesgetRequestExecutionContext() ?? this.ctxso the ALS-provided ctx takes priority over the constructor-injected one.Design
getRequestExecutionContext()returnsnullwhen called outside a request scope (e.g. Node.js dev server), making it safe to call anywhere. The ALS scope is established byapp-router-entry.tsbefore any RSC/SSR/routing logic runs, so it's available throughout the entire request lifecycle.Tests
tests/request-context.test.tscovering: null outside scope, correct value inside scope, async propagation, concurrent request isolation, nested scope override/restore, andwaitUntilcallable from inside scope.tests/deploy.test.tsassertions for the new worker entry signature.tests/__snapshots__/entry-templates.test.ts.snap.Related
Extracted from #405 (ISR caching) — the ctx plumbing is a prerequisite for
waitUntil-based background cache writes but is generally useful on its own.