fix(isr): use getRequestExecutionContext() from ALS in background regeneration#429
fix(isr): use getRequestExecutionContext() from ALS in background regeneration#429james-elicx merged 1 commit intomainfrom
Conversation
…eneration Instead of relying on the caller to pass ctx to triggerBackgroundRegeneration, read it from AsyncLocalStorage via getRequestExecutionContext(). This ensures waitUntil() is always registered on the live Workers ExecutionContext, fixing stale responses that were not triggering revalidation on Cloudflare Workers.
|
/bigbonk review |
commit: |
|
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Clean, well-scoped fix. The approach is correct and the implementation is solid.
Analysis
The problem: __triggerBackgroundRegeneration in the generated RSC entry received ctx as a parameter forwarded from the outer _handleRequest(request, __reqCtx, _mwCtx, ctx) scope. On Cloudflare Workers, by the time the stale-while-revalidate branch fires the async regeneration, the original ctx reference could be stale (or absent in non-Worker environments), meaning waitUntil() never gets called and the isolate terminates before regeneration completes — silently dropping the background work.
The fix: Instead of threading ctx through function parameters, call _getRequestExecutionContext() (backed by AsyncLocalStorage stored on globalThis) inside the function body. This retrieves the live ExecutionContext for the current request from the ALS scope established by _runWithExecutionContext(ctx, _run) at the top of the request handler. The same fallback pattern is applied to isr-cache.ts's exported triggerBackgroundRegeneration for the dev-server code path.
Why this works: The ALS scope is established at the outermost wrapper in the request pipeline (confirmed in app-rsc-entry.ts line ~1440 and pages-server-entry.ts line ~679). The __triggerBackgroundRegeneration call happens synchronously within that scope (it's called inline during the stale-cache branch, before the response is returned), so _getRequestExecutionContext() will always see the correct store value. The regeneration promise is registered with waitUntil() before the ALS scope unwinds.
Consistency: This aligns with the existing pattern used elsewhere in the codebase — kv-cache-handler.ts already calls getRequestExecutionContext() for its _deleteInBackground and _putInBackground methods, and the Pages Router entry (pages-server-entry.ts) already uses the same pattern for both middleware drain and ISR regeneration.
A few observations (not blocking):
-
The
isr-cache.tschange (const execCtx = ctx ?? getRequestExecutionContext()) preserves backward compatibility for callers that still passctxexplicitly (the test atisr-cache.test.ts:250exercises this path), which is good defensive design. -
The snapshot diff is large but mechanical — it's the same 3-line change repeated across all 7 test fixture permutations, which is expected.
|
Approved PR #429. Here's a summary of the review: Verdict: Approved The fix correctly addresses a real bug where The solution replaces parameter-threading with an ALS-backed
The |
1 participant
Summary
__triggerBackgroundRegenerationinapp-rsc-entry.tswas receivingctxas a parameter from the outer request handler, but by the time the stale-while-revalidate branch executed, that reference could be stale or absent — causingwaitUntil()to never be called and the Workers isolate to terminate before the revalidation completedgetRequestExecutionContext()(AsyncLocalStorage-backed) inside the function to always retrieve the liveExecutionContextfor the current requestisr-cache.ts'striggerBackgroundRegenerationso the dev-server path (which passes noctx) also benefits