fix: use waitUntil in after() for Cloudflare Workers by NathanDrake2406 · Pull Request #543 · cloudflare/vinext

NathanDrake2406 · 2026-03-15T06:20:07Z

Summary

after() was scheduling tasks as fire-and-forget microtasks via Promise.resolve().then(task), which are silently dropped when Cloudflare Workers terminate after response completion
Now uses getRequestExecutionContext().waitUntil(promise) when an execution context exists, falling back to fire-and-forget for Node.js dev server
Added throwIfInsideCacheScope("after()") guard matching Next.js behavior (same pattern as connection(), headers(), cookies())
Kept after() synchronous (function after(): void) with static imports to ensure waitUntil is registered immediately, not on a deferred microtask

Test plan

Test: after() calls waitUntil on execution context when one exists
Test: after() falls back to fire-and-forget when no execution context
Test: after() throws inside "use cache" scope
All 688 tests pass
Typecheck and lint clean

pkg-pr-new · 2026-03-15T06:20:45Z

npm i https://pkg.pr.new/vinext@543

commit: cb85769

after() was scheduling tasks as fire-and-forget microtasks, which are silently dropped when the Cloudflare Workers runtime terminates after the response stream completes. Now uses getRequestExecutionContext()?.waitUntil() to keep tasks alive past the response, falling back to fire-and-forget when no execution context exists (e.g. Node.js dev server). Also adds throwIfInsideCacheScope() guard matching Next.js behavior — after() must not be called inside "use cache" scopes.

after() was async with dynamic imports, which broke the API contract: - Next.js after() returns void (fire-and-forget), so making it async causes unhandled rejections when throwIfInsideCacheScope throws - await import() deferred waitUntil registration to a later microtask, risking task loss if the Worker tears down in between Changed to static top-level imports and synchronous function signature. Updated tests to assert synchronous throw behavior and remove setTimeout hacks that were only needed due to the async implementation.

…mport The static `import { throwIfInsideCacheScope } from "./headers.js"` pulled headers.ts into the module graph for every consumer of next/server. headers.ts contains the literal string "use cache" which triggers Vite's cache-transform handler (`code.includes("use cache")`). Pages Router fixtures without @vitejs/plugin-rsc then fail at dev-server startup. Inline the cache-scope check using the same Symbol.for ALS keys, avoiding the problematic import chain. This mirrors how connection() already avoids eagerly importing headers.ts (via dynamic import), but keeps after() synchronous.

james-elicx · 2026-03-15T09:34:36Z

/bigbonk review

ask-bonk

Good fix — after() tasks being silently dropped on Workers was a real problem, and integrating with waitUntil via the existing request-context ALS is the right approach. The decision to keep after() synchronous and inline the cache-scope guard is well-reasoned. A few things worth addressing:

ask-bonk · 2026-03-15T09:36:08Z

+  const cacheAls = _g[_USE_CACHE_ALS_KEY] as { getStore(): unknown } | undefined;
+  if (cacheAls?.getStore() != null) {
+    throw new Error(
+      `\`${apiName}\` cannot be called inside a cached scope. ` +


The error message here says "a cached scope" but the canonical throwIfInsideCacheScope in headers.ts:119 says 'inside "use cache"'. Since these check the exact same ALS, the error messages should match — otherwise users get different wording depending on which API they call (after() vs headers() vs cookies()).

Suggested change

`\`${apiName}\` cannot be called inside a cached scope. ` +

`\`${apiName}\` cannot be called inside "use cache". ` +

ask-bonk · 2026-03-15T09:36:09Z

+
+const _USE_CACHE_ALS_KEY = Symbol.for("vinext.cacheRuntime.contextAls");
+const _UNSTABLE_CACHE_ALS_KEY = Symbol.for("vinext.unstableCache.als");
+const _g = globalThis as unknown as Record<PropertyKey, unknown>;


Minor: headers.ts names this _gHeaders to avoid confusion with other _g variables in the module graph. Using _g here is fine since it's the only globalThis alias in this file, but worth noting that both files are reaching into the same globalThis — if this inlined pattern spreads further, it might be worth extracting the cache-scope check into a tiny leaf module with no directive strings that both server.ts and headers.ts can import.

ask-bonk · 2026-03-15T09:36:09Z

+    cacheContextStorage.run({ tags: [], lifeConfigs: [], variant: "default" }, () => {
+      expect(() => after(() => {})).toThrow(/cannot be called inside a cached scope/);
+    });
+  });


This test covers the "use cache" scope but not the unstable_cache() scope. The inlined guard checks both ALS instances — would be good to add a test that after() also throws inside unstable_cache() to prevent regressions on that branch. Something like:

it('after() throws inside unstable_cache() scope', async () => { const { after } = await import("../packages/vinext/src/shims/server.js"); // The unstable_cache ALS is stored on globalThis via Symbol.for const key = Symbol.for("vinext.unstableCache.als"); const { AsyncLocalStorage } = await import("node:async_hooks"); const als = ((globalThis as any)[key] ??= new AsyncLocalStorage()); als.run(true, () => { expect(() => after(() => {})).toThrow(/unstable_cache/); }); });

ask-bonk · 2026-03-15T09:36:09Z

+    };
+
+    let called = false;
+    await runWithExecutionContext(mockCtx, async () => {


The async callback passed to runWithExecutionContext returns immediately after calling after(). Since after() wraps the task in Promise.resolve().then(task), the task callback hasn't executed yet when runWithExecutionContext resolves. The test then awaits waitUntilCalls[0] to verify the callback ran — this works, but it's worth noting that the test is relying on waitUntil receiving the full promise chain (including the .then(task) step). That's correct behavior, just making sure it's intentional and not coincidental.

ask-bonk · 2026-03-15T09:36:21Z

Review submitted. Here's a summary of the feedback I posted:

Overall: The fix is solid — after() tasks were silently dropped on Cloudflare Workers because fire-and-forget microtasks don't survive past response completion. Using getRequestExecutionContext()?.waitUntil() is the right integration point, and keeping after() synchronous (with the inlined cache-scope guard) ensures waitUntil is registered immediately rather than on a deferred microtask.

Comments posted (4):

Error message mismatch (server.ts:39): The inlined guard says "a cached scope" but the canonical throwIfInsideCacheScope in headers.ts says 'inside "use cache"'. These should match so users get consistent error messages regardless of which API they call. Suggested the fix.
_g naming note (server.ts:33): Minor observation that headers.ts uses _gHeaders to namespace its globalThis alias. If this inlined pattern spreads further, it might be worth extracting the cache-scope check into a tiny leaf module with no directive strings.
Missing unstable_cache scope test (shims.test.ts:1158): The test covers the "use cache" scope but not the unstable_cache() branch. Since the inlined guard checks both ALS instances, there should be a test for the second branch too. Included a code snippet.
Test timing note (shims.test.ts:1126): Noted that the waitUntil test correctly relies on the full promise chain (including .then(task)) being passed to waitUntil, which is the desired behavior — just flagging it as an intentional design detail.

github run

…test for after()

james-elicx · 2026-03-15T10:10:07Z

/bigbonk review

ask-bonk

Good fix — after() tasks being silently dropped on Workers is a real production issue and waitUntil via the execution context ALS is the right integration point. The inlined cache-scope guard is well-justified by the comment, the error messages now match headers.ts, and the tests cover the key paths.

A few things to consider:

ask-bonk · 2026-03-15T10:11:57Z

    console.error("[vinext] after() task failed:", err);
  });
+
+  getRequestExecutionContext()?.waitUntil(guarded);


Behavioral difference from Next.js worth documenting: In Next.js, after() throws when called outside a request scope (no workStore), and also throws if waitUntil is not available on the platform. Here, vinext silently falls back to fire-and-forget when there's no execution context.

The fire-and-forget fallback makes sense for the Node.js dev server (which has no waitUntil), but it also means that if someone forgets to wrap their Worker entry with runWithExecutionContext(), tasks will silently fall through instead of failing loudly.

Not necessarily something to change in this PR, but worth a // TODO: or a comment noting the intentional divergence. A console.warn on the fallback path (maybe once-only) could also help users catch misconfigured deployments.

ask-bonk · 2026-03-15T10:11:57Z

 export function after<T>(task: Promise<T> | (() => T | Promise<T>)): void {
+  _throwIfInsideCacheScope("after()");
+
  const promise = typeof task === "function" ? Promise.resolve().then(task) : task;


Function tasks vs promise tasks — different waitUntil behavior: When task is a function, it gets wrapped in Promise.resolve().then(task), meaning the actual work is deferred to a microtask. When task is a pre-existing promise, it's used directly. In both cases guarded is passed to waitUntil.

This is fine for correctness, but there's a subtle difference from Next.js: their AfterContext treats promise tasks and function tasks differently — promises go directly to waitUntil, while function callbacks are queued and only run after the response is sent (via onClose). The current vinext implementation runs function tasks immediately (on the next microtask), which means they execute concurrently with response streaming rather than after it.

For this PR, the current behavior is a reasonable simplification. Just noting it as a known divergence for future work.

ask-bonk · 2026-03-15T10:11:57Z

+    };
+
+    let called = false;
+    await runWithExecutionContext(mockCtx, async () => {


Nit: the runWithExecutionContext callback is async but doesn't await anything — it calls after() (which is synchronous) and returns. The async keyword means the callback returns a resolved promise, but runWithExecutionContext will still resolve synchronously since _als.run(ctx, fn) returns the promise from the async IIFE.

This is fine — just noting that the async is unnecessary here and could be dropped without changing behavior:

Suggested change

await runWithExecutionContext(mockCtx, async () => {

await runWithExecutionContext(mockCtx, () => {

ask-bonk · 2026-03-15T10:11:57Z

+    after(() => {
+      called = true;
+    });
+    await new Promise((r) => setTimeout(r, 10));


Minor: the 10ms setTimeout is a bit fragile for CI environments under load. Since after() wraps function tasks in Promise.resolve().then(task), a single await Promise.resolve() (or even two for the .then chain) would be sufficient and deterministic:

Suggested change

await new Promise((r) => setTimeout(r, 10));

await new Promise((r) => setTimeout(r, 50));

Or better yet, await the promise chain directly (though that would require capturing it, which changes the test structure). The current 10ms is probably fine in practice; just flagging it.

ask-bonk · 2026-03-15T10:12:09Z

Review submitted. Here's a summary of what I posted:

Overall: The fix is correct and well-motivated. after() tasks were silently dropped on Cloudflare Workers because fire-and-forget microtasks don't survive past response completion. Using getRequestExecutionContext()?.waitUntil() is the right approach, and keeping after() synchronous with an inlined cache-scope guard ensures waitUntil is registered immediately.

4 comments posted:

Silent fallback vs Next.js behavior (server.ts:631): Next.js throws when after() is called outside a request scope or when waitUntil is unavailable. vinext silently falls back to fire-and-forget. This is reasonable for the Node.js dev server, but means a misconfigured Worker entry (missing runWithExecutionContext) would silently drop tasks. Suggested a // TODO or a one-time console.warn on the fallback path.
Function vs promise task timing divergence (server.ts:626): Next.js queues function callbacks to run after the response is sent (via onClose), while vinext runs them immediately on the next microtask (concurrent with response streaming). Noted this as a known simplification for future work.
Unnecessary async in test (shims.test.ts:1126): The runWithExecutionContext callback is async but never awaits anything — the async can be dropped. Posted as a suggestion.
Timing in fallback test (shims.test.ts:1147): The 10ms setTimeout is slightly fragile for CI under load. Suggested bumping to 50ms as a small safety margin.

github run

…-cache transform The fast-path code.includes("use cache") check in the vinext:use-cache transform handler was firing on files that contain the string only in error messages or comments (after TS constant folding), not as actual directives. This caused Pages Router fixtures without @vitejs/plugin-rsc to throw at startup when server.ts (which uses "use cache" in its cache-scope error message) was transformed. Fix: parse the AST first and confirm an actual file-level or function-level "use cache" directive exists before throwing the missing-RSC error. Also address second-round review comments: - server.ts: add TODO about silent waitUntil fallback vs Next.js throw behavior - server.ts: document timing divergence (concurrent vs post-response) on task execution - shims.test.ts: drop unnecessary async from runWithExecutionContext callback - shims.test.ts: bump fire-and-forget fallback timeout 10ms -> 50ms for CI robustness

james-elicx · 2026-03-15T10:41:54Z

/bigbonk review

NathanDrake2406 · 2026-03-15T10:47:24Z

@james-elicx i don't get this bug :(

james-elicx · 2026-03-15T10:51:30Z

/bigbonk review

james-elicx · 2026-03-15T10:53:58Z

@james-elicx i don't get this bug :(

Because the comment was updated to be more informative (being explicit about the directive being used), there were false positives from the static analysis to determine if the directive is used in a file. So, the change walks the AST to check for real use of the directives rather than checking if the string exists in the code, which can throw up misleading matches.

james-elicx · 2026-03-15T11:06:27Z

/bigbonk review

NathanDrake2406 · 2026-03-15T11:10:51Z

@james-elicx i don't get this bug :(

Because the comment was updated to be more informative (being explicit about the directive being used), there were false positives from the static analysis to determine if the directive is used in a file. So, the change walks the AST to check for real use of the directives rather than checking if the string exists in the code, which can throw up misleading matches.

Ahh so instead of grep ctrlf we use ast, ic

ask-bonk · 2026-03-15T11:11:58Z

@james-elicx Bonk workflow was cancelled.

View workflow run · To retry, trigger Bonk again.

james-elicx · 2026-03-15T11:12:21Z

/bigbonk review

NathanDrake2406 · 2026-03-15T11:12:29Z

Btw could you pls explain more about the half baked static rendering implementation and what's ur approach going to be?
I wanna learn 😃 my claude is gonna take a beating tonight

ask-bonk

Good PR overall. The core fix — using waitUntil via the execution context ALS so after() tasks survive past response completion on Workers — is the right approach and well-implemented. The decision to keep after() synchronous and inline the cache-scope guard is well-justified by the comment explaining the import chain problem. The AST-based directive detection in index.ts is a solid improvement over the string-based check.

A few things worth addressing:

ask-bonk · 2026-03-15T11:14:02Z

+              if (
+                (node.type === "FunctionDeclaration" ||
+                  node.type === "FunctionExpression" ||
+                  node.type === "ArrowFunctionExpression") &&


The AST walker doesn't check for MethodDefinition nodes (class methods). If someone writes:

class Fetcher { async getData() { "use cache"; return fetch(...); } }

the walker will descend into the MethodDefinition via the generic key-walking loop, find the FunctionExpression inside .value, and match it — so this actually works by accident. But it's fragile: it relies on the generic traversal reaching the function node inside the method definition. Might be worth explicitly listing MethodDefinition (where the function is in .value) alongside the three function types for clarity and to make the intent obvious.

ask-bonk · 2026-03-15T11:14:02Z

+              for (const key of Object.keys(node)) {
+                if (key === "type" || key === "start" || key === "end" || key === "loc") continue;
+                const child = node[key];
+                if (Array.isArray(child) && child.length > 0 && typeof child[0] === "object") {


The child.length > 0 && typeof child[0] === "object" heuristic only checks the first array element to decide if the array contains AST nodes. If the first element is null (which can happen in sparse arrays in some AST representations, e.g. [null, FunctionDeclaration]), this would skip the entire array. In practice this probably never fires for rollup's parseAst, but it's a subtle assumption.

ask-bonk · 2026-03-15T11:14:03Z

+  // fire-and-forget here (safe for the Node.js dev server), but a misconfigured
+  // Cloudflare Worker entry that omits runWithExecutionContext would silently drop
+  // tasks. Consider a one-time console.warn on the fallback path.
+  getRequestExecutionContext()?.waitUntil(guarded);


The TODO comment is good. One concrete follow-up thought: since the after() shim is also imported during the dev server (where getRequestExecutionContext() always returns null), the warning would need to be gated on something like process.env.NODE_ENV === 'production' or a runtime check for the Cloudflare Workers runtime, otherwise every dev-server after() call would warn. Worth capturing in the TODO.

ask-bonk · 2026-03-15T11:14:03Z

 export function after<T>(task: Promise<T> | (() => T | Promise<T>)): void {
+  _throwIfInsideCacheScope("after()");
+
  const promise = typeof task === "function" ? Promise.resolve().then(task) : task;


Minor: when task is a pre-existing promise (not a function), the .catch() on line 631 attaches to it — but the original caller still holds a reference to the unwrapped task promise. If the caller awaits task elsewhere (unlikely but possible), the rejection would surface there too since .catch() creates a new chain, it doesn't suppress the original. This is fine for the fire-and-forget use case, just noting it.

ask-bonk · 2026-03-15T11:14:03Z

+          // Check for function-level "use cache" directives by walking function bodies
+          function nodeHasInlineCacheDirective(node: any): boolean {
+            if (!node || typeof node !== "object") return false;
+            const body: any[] = node.body ?? node.consequent ?? [];


Nit: node.consequent is used here for SwitchCase nodes (which have consequent: Statement[]), but a "use cache" directive in a switch case body wouldn't be a valid function-level directive — it would need to be the first statement in a function body. Is there a case where consequent is needed here?

For ArrowFunctionExpression with an expression body (e.g. () => "use cache"), node.body is an Expression, not a BlockStatement, so the Array.isArray(body) check correctly filters it out. That's fine.

ask-bonk · 2026-03-15T11:14:03Z

+    const { AsyncLocalStorage } = await import("node:async_hooks");
+    const key = Symbol.for("vinext.unstableCache.als");
+    const g = globalThis as unknown as Record<symbol, unknown>;
+    if (!g[key]) g[key] = new AsyncLocalStorage();


Good that this test was added (the earlier bonk review correctly flagged the missing unstable_cache branch coverage). One edge case: this test lazily creates the ALS on globalThis if it doesn't exist yet (if (!g[key]) g[key] = new AsyncLocalStorage()). If the cache.ts module has already been imported in a prior test and registered its own ALS instance on the same key, this if guard correctly reuses it. But if test ordering changes and this test runs first, it creates a standalone ALS that won't be the same instance cache.ts would later register. This is fine for this test since it only checks the guard in server.ts, but worth noting the test is isolated from the real cache.ts registration.

ask-bonk · 2026-03-15T11:14:17Z

Review submitted. Here's a summary of the feedback I posted:

Overall: The core fix is solid — after() tasks were silently dropped on Cloudflare Workers because fire-and-forget microtasks don't survive past response completion. Using getRequestExecutionContext()?.waitUntil() is the right integration point. Keeping after() synchronous with an inlined cache-scope guard is well-justified (the comment explaining why throwIfInsideCacheScope can't be statically imported is helpful). The AST-based "use cache" directive detection replacing the string-based check is a good defensive improvement.

6 comments posted:

AST walker missing MethodDefinition (index.ts:2866): The walker doesn't explicitly handle class method definitions. It works by accident via the generic key traversal, but should list MethodDefinition explicitly for clarity.
Array heuristic fragility (index.ts:2875): The child[0] check to decide if an array contains AST nodes could miss arrays starting with null.
TODO: gate dev-server warning (server.ts:640): The TODO about adding a console.warn on the fallback path needs to account for the dev server, where getRequestExecutionContext() always returns null — the warning should be production-only.
Promise .catch() doesn't suppress original (server.ts:626): When task is a pre-existing promise, .catch() creates a new chain but doesn't suppress rejections on the original reference. Fine for fire-and-forget, just noted.
node.consequent usage (index.ts:2845): consequent is a SwitchCase property — a "use cache" directive there wouldn't be a valid function-level directive. Questioned whether this fallback is needed.
Test isolation for unstable_cache ALS (shims.test.ts:1165): The test lazily creates the ALS, which works for the guard check but is isolated from the real cache.ts registration — noted as acceptable but worth being aware of.

github run

james-elicx · 2026-03-15T12:29:45Z

Btw could you pls explain more about the half baked static rendering implementation and what's ur approach going to be? I wanna learn 😃 my claude is gonna take a beating tonight

When I started looking through the diff manually, it was essentially only static routes in the pages directory that it would pre-render. Everything else fell back to how it was before. I tried running it locally to confirm what I was seeing.

It's one of those things that reviewing the code that is being created highlights rather than relying solely on an agent, as it doesn't necessarily always come up with the right solution, or the full solution.

Anyhow, my iteration started with asking it to draft a comprehensive plan for how prerendering should work, and then I went through and reviewed that before letting it have a crack at implementing it. And then I've spent the past few hours this morning iterating with it on the implementation it devised from the plan and reworking some parts (both manually and with the agent).

- Add MethodDefinition to the explicit node-type check in astHasInlineCache (was handled by generic key traversal but should be listed explicitly) - Fix array heuristic in astHasInlineCache to use .some() instead of checking child[0], which could miss arrays starting with null - Remove node.consequent fallback from nodeHasInlineCacheDirective — consequent is a SwitchCase property and a 'use cache' directive there would not be a valid function-level directive - Clarify the after() TODO comment: the dev server always returns null from getRequestExecutionContext(), so the fire-and-forget fallback is correct there; the warning concern is production-only (Workers with missing runWithExecutionContext)

james-elicx · 2026-03-15T13:29:21Z

/bigbonk review

ask-bonk

Good fix. The core problem — after() tasks silently dropped on Workers because fire-and-forget microtasks don't survive past response completion — is real and well-understood. Using getRequestExecutionContext()?.waitUntil() is the correct integration point.

The inlined cache-scope guard is well-justified (the comment explaining the headers.ts import chain problem is appreciated and correct), and the error messages are properly aligned with the canonical throwIfInsideCacheScope in headers.ts. The AST-based directive detection replacing the string-based check is a solid defensive improvement.

A few items below, none blocking.

ask-bonk · 2026-03-15T13:31:39Z

+                  node.type === "FunctionExpression" ||
+                  node.type === "ArrowFunctionExpression" ||
+                  node.type === "MethodDefinition") &&
+                nodeHasInlineCacheDirective(node.body ?? node.value)


Nit: the ?? node.value fallback is only needed for MethodDefinition (where the function lives in .value not .body), but it reads as if any of the four node types might need it. A brief inline comment like // MethodDefinition stores the function in .value would make the intent clearer.

Not a bug — body is always truthy for the three function types so ?? never fires for them.

ask-bonk · 2026-03-15T13:31:39Z

+  // getRequestExecutionContext() always returns null). On Workers, a misconfigured
+  // entry that omits runWithExecutionContext would silently drop tasks — consider
+  // a one-time console.warn on the fallback path, gated to non-dev environments.
+  getRequestExecutionContext()?.waitUntil(guarded);


The TODO is well-scoped. One concrete note for when it's implemented: getRequestExecutionContext() returns null on every after() call in the Node.js dev server, so a naive console.warn here would fire on every dev-mode after() call. It should be gated on production (e.g., process.env.NODE_ENV === 'production' or a Workers runtime check like typeof caches !== 'undefined'), and ideally once-only (module-level let _warned = false).

ask-bonk · 2026-03-15T13:31:39Z

+
+const _USE_CACHE_ALS_KEY = Symbol.for("vinext.cacheRuntime.contextAls");
+const _UNSTABLE_CACHE_ALS_KEY = Symbol.for("vinext.unstableCache.als");
+const _g = globalThis as unknown as Record<PropertyKey, unknown>;


There are now two files with nearly identical cache-scope guard logic: this inlined version in server.ts and the canonical one in headers.ts (lines 94–131). The duplication is justified for now (the comment explains why), but if a third consumer appears, it'd be worth extracting the check into a tiny leaf module (no "use cache" strings in its error messages — pass the message prefix as a parameter) that both can import without triggering the Vite transform.

Not actionable for this PR — just a heads-up for future maintenance.

ask-bonk · 2026-03-15T13:31:39Z

+    };
+
+    let called = false;
+    await runWithExecutionContext(mockCtx, () => {


Minor: this callback is synchronous (calls after() which is void, no await). The previous bonk review suggested dropping the async keyword here. Agree it's unnecessary — runWithExecutionContext works fine with a sync callback, and removing async makes it clearer that no async work happens inside the scope.

ask-bonk · 2026-03-15T13:31:39Z

+    after(() => {
+      called = true;
+    });
+    await new Promise((r) => setTimeout(r, 50));


Good that this was bumped from 10ms to 50ms (the pre-existing error-swallowing test at line 1108 still uses 10ms FWIW). Since after() wraps function tasks in Promise.resolve().then(task), the work completes after two microtask ticks — so await Promise.resolve(); await Promise.resolve(); would be deterministic. The setTimeout approach is fine in practice, just noting the alternative for robustness.

ask-bonk · 2026-03-15T13:31:51Z

Review submitted successfully. Here's a summary of my feedback:

Overall: The fix is solid and well-motivated. after() tasks were silently dropped on Cloudflare Workers because fire-and-forget microtasks don't survive past response completion. Using getRequestExecutionContext()?.waitUntil() is the right approach, and keeping after() synchronous with an inlined cache-scope guard is well-justified by the import chain constraint.

5 inline comments posted (none blocking):

index.ts:2868 — The node.body ?? node.value fallback only matters for MethodDefinition, but reads ambiguously. A brief inline comment would clarify intent.
server.ts:641 — The TODO for a console.warn on the fallback path needs to be gated on production, since getRequestExecutionContext() always returns null in the Node.js dev server.
server.ts:33 — Noted the duplication between the inlined guard here and the canonical one in headers.ts. Justified for now, but worth extracting into a shared leaf module if a third consumer appears.
shims.test.ts:1126 — The async on runWithExecutionContext's callback is unnecessary since no await happens inside.
shims.test.ts:1147 — The 50ms timeout works but await Promise.resolve(); await Promise.resolve() would be deterministic since after() uses Promise.resolve().then(task).

github run

james-elicx · 2026-03-15T15:41:28Z

oh god, i was trying out running opencode from my server and i guess the git identity wasnt set up properly and it set it to someone random

NathanDrake2406 · 2026-03-15T15:42:24Z

HAHAHAHA so it is absolutely random his name is NATHAN DRAKE? OMFG

- Add inline comment explaining why `?? node.value` is needed for MethodDefinition nodes in the AST walker - Expand TODO on the after() fire-and-forget fallback to note the warn should be gated to production (NODE_ENV or Workers runtime check) and use a module-level _warned flag - Replace 50ms setTimeout in the fire-and-forget test with two await Promise.resolve() ticks, which is deterministic since after() wraps tasks in Promise.resolve().then(task)

Replace 10ms setTimeout with two microtask awaits in the pre-existing error-swallowing test, matching the same pattern used in the fire-and-forget test. after() wraps function tasks in Promise.resolve().then(task) so two await Promise.resolve() ticks are sufficient and deterministic.

NathanDrake2406 · 2026-03-15T15:44:41Z

this is peak haahahaha holy

james-elicx · 2026-03-15T15:44:48Z

there we go

james-elicx · 2026-03-15T15:57:05Z

/bigbonk review

ask-bonk

Good PR. The core fix is correct and well-motivated — after() tasks silently dropping on Workers was a real production bug, and getRequestExecutionContext()?.waitUntil() is the right integration point. The AST-based directive detection in index.ts is a solid defensive fix for the false positive that motivated it.

The inlined cache-scope guard, the explanatory comments, and the test coverage are all well done. Previous bonk reviews have already flagged most of the nits (timing, async keyword, duplication extraction, etc.) and I can see the author iterated on the feedback — the error messages now match headers.ts, microtask-based timing replaced setTimeout, and both "use cache" and unstable_cache scope tests are present.

A few remaining observations below, none blocking.

One thing not covered by the inline comments: the pre-existing test at line 1084 (after() runs a callback asynchronously without throwing) still uses setTimeout(r, 10) for timing, while the newer tests correctly use await Promise.resolve(); await Promise.resolve(). Would be good to align it for consistency in a follow-up.

ask-bonk · 2026-03-15T15:59:22Z

+                nodeHasInlineCacheDirective(node.body ?? node.value) // MethodDefinition stores the function in .value
+              ) {
+                return true;
+              }


Nit: for MethodDefinition, node.body is undefined and node.value is the FunctionExpression. The ?? fallback works correctly, but nodeHasInlineCacheDirective then receives the FunctionExpression and checks node.body on that — which is the BlockStatement. This two-hop indirection (caller does node.body ?? node.value → callee does node.body ?? []) makes the flow non-obvious.

The inline comment helps. Just flagging that if someone later passes a MethodDefinition node directly to nodeHasInlineCacheDirective (not via astHasInlineCache), it would silently return false since MethodDefinition.body is undefined and the fallback [] would yield no statements. Not a bug today since the only call site is correct.

ask-bonk · 2026-03-15T15:59:22Z

+            const body: any[] = node.body ?? [];
+            if (Array.isArray(body)) {
+              for (const stmt of body) {
+                if (


Minor: when node.body is an expression (e.g., arrow function () => expr), this assigns a non-array to body and the Array.isArray(body) check below correctly skips it. The ?? [] fallback only fires for MethodDefinition where node.body is undefined.

This is fine — just noting that the default value [] is specific to the MethodDefinition case, not a general "missing body" fallback.

ask-bonk · 2026-03-15T15:59:22Z

 export function after<T>(task: Promise<T> | (() => T | Promise<T>)): void {
+  _throwIfInsideCacheScope("after()");
+
  const promise = typeof task === "function" ? Promise.resolve().then(task) : task;


Worth noting: when task is a pre-existing rejected promise, the .catch() on guarded (line 631) handles the rejection. But between lines 626 and 631, the original promise reference briefly has no rejection handler attached, which could trigger an unhandledrejection event on some runtimes if the microtask queue drains between these two synchronous lines.

In practice this doesn't happen because the .catch() is attached synchronously in the same tick. Just noting the pattern — if these lines were ever separated by an await, it would become a real issue.

ask-bonk · 2026-03-15T15:59:22Z

+    const { AsyncLocalStorage } = await import("node:async_hooks");
+    const key = Symbol.for("vinext.unstableCache.als");
+    const g = globalThis as unknown as Record<symbol, unknown>;
+    if (!g[key]) g[key] = new AsyncLocalStorage();


This lazily creates an ALS instance if none exists on globalThis. If cache.ts hasn't been imported yet in this test run, this creates an ALS that won't be the same instance cache.ts would later register. That's fine for this test since it only validates the guard in server.ts (which reads from the same Symbol key). But it does mean this test is isolated from the real cache.ts registration path — worth being explicit about in a comment.

ask-bonk · 2026-03-15T15:59:34Z

Review submitted successfully. Here's a summary of my review:

Approved with 4 non-blocking inline comments.

Overall assessment: The fix is correct and well-motivated. after() tasks were silently dropped on Cloudflare Workers because fire-and-forget microtasks don't survive past response completion. Using getRequestExecutionContext()?.waitUntil() is the right integration point. The AST-based directive detection replacing string matching in index.ts is a solid defensive improvement that prevents false positives from "use cache" appearing in comments or error messages.

Inline comments posted:

index.ts:2871 — The two-hop indirection for MethodDefinition handling (caller does node.body ?? node.value, callee does node.body ?? []) works but could silently return false if nodeHasInlineCacheDirective is called directly with a MethodDefinition node in the future.
index.ts:2848 — The ?? [] fallback is specific to the MethodDefinition case, not a general "missing body" fallback. Fine as-is, just noting for clarity.
server.ts:626 — When task is an already-rejected promise, the rejection handler is attached synchronously so there's no unhandledrejection risk. But if these lines were ever separated by an await, it would become a real issue.
shims.test.ts:1171 — The lazy ALS creation is isolated from the real cache.ts registration path, which is acceptable for this test but worth being explicit about.

Also noted in the review body that the pre-existing test at line 1084 still uses setTimeout(r, 10) and should be aligned with the microtask approach used in the newer tests.

github run

…jection safety note, and ALS test isolation comment

james-elicx · 2026-03-16T08:40:01Z

/bigbonk re-review

ask-bonk

Review

The after() + waitUntil fix in server.ts is correct and well-implemented — the inlined cache-scope guard, the explanatory comments, and the test coverage are all solid. However, the AST-based directive detection in index.ts has a correctness bug that will silently break function-level "use cache" transforms.

Blocking issue: `nodeHasInlineCacheDirective` never detects inline directives

For FunctionDeclaration/FunctionExpression/ArrowFunctionExpression (with block body), fn.body is a BlockStatement node (an object with type: "BlockStatement"), not a Statement[] array. The code checks Array.isArray(body) which is always false for a BlockStatement, so the function always returns false.

The fix should unwrap the BlockStatement:

const body = fn?.body;
const stmts = Array.isArray(body) ? body : body?.body;
if (Array.isArray(stmts)) { ... }

Or more directly: check body?.type === "BlockStatement" ? body.body : body then Array.isArray(...) on the result.

This means the use-cache-fn-test fixture (which has async function getData() { "use cache"; ... }) would silently stop getting the inline directive transform applied. The E2E test in e2e/app-router/use-cache.spec.ts should catch this — worth verifying it still passes on this branch.

Minor items (non-blocking)

shims.test.ts:1084 — The pre-existing test still uses setTimeout(r, 10) while the new/updated tests correctly use microtick awaits. Would be nice to align for consistency.
The previous bonk reviews covered the remaining nits well (unnecessary async on the runWithExecutionContext callback, production-gating the TODO console.warn, etc.).

ask-bonk · 2026-03-16T08:43:05Z

+            // MethodDefinition wraps its FunctionExpression in .value; unwrap to reach .body.
+            const fn = node.type === "MethodDefinition" ? node.value : node;
+            const body = fn?.body;
+            if (Array.isArray(body)) {


Bug: fn.body is a BlockStatement node (an object with { type: "BlockStatement", body: Statement[] }), not a Statement[] array. Array.isArray(body) is always false for function nodes with block bodies, so nodeHasInlineCacheDirective always returns false and no inline "use cache" directives are ever detected.

This silently regresses function-level "use cache" support — the use-cache-fn-test fixture (async function getData() { "use cache"; ... }) would no longer get the inline directive transform.

You need to unwrap the BlockStatement to get the actual statement array:

Suggested change

if (Array.isArray(body)) {

const body = fn?.body;

const stmts = body?.type === "BlockStatement" ? body.body : Array.isArray(body) ? body : null;

if (Array.isArray(stmts)) {

for (const stmt of stmts) {

ask-bonk · 2026-03-16T08:43:05Z

+            }
+            return false;
+          }
+          const hasInlineCache = !cacheDirective && astHasInlineCache(ast.body as any[]);


Because of the nodeHasInlineCacheDirective bug above, astHasInlineCache always returns false, so hasInlineCache is always false. This means the if (hasInlineCache) block at line 3493 is now dead code — files with only inline "use cache" directives (no file-level directive) will hit return null on line 3412 and skip the transform entirely.

The old code.includes("use cache") && !cacheDirective was imprecise (false positives from comments/strings) but at least correct for real directives. The new AST check is the right direction but needs the BlockStatement unwrap fix.

ask-bonk · 2026-03-16T08:43:05Z

+  // `process.env.NODE_ENV === 'production'` or `typeof caches !== 'undefined'` for
+  // a Workers runtime check) with a module-level `let _warned = false` guard so it
+  // fires at most once and doesn't spam the dev-server console.
+  getRequestExecutionContext()?.waitUntil(guarded);


The after() + waitUntil integration is clean. getRequestExecutionContext()?.waitUntil(guarded) is the right pattern — the optional chaining means it's a no-op on Node.js dev server, and the guarded promise (with .catch()) ensures waitUntil never sees an unhandled rejection.

The TODO comment is well-scoped. When implementing, note that the warning should be gated on production (process.env.NODE_ENV === 'production') since getRequestExecutionContext() always returns null in the dev server.

ask-bonk · 2026-03-16T08:43:17Z

Review submitted. Here's a summary:

Requested changes on PR #543 with 3 inline comments.

Blocking issue: `nodeHasInlineCacheDirective` never detects inline directives

The AST walker in index.ts has a correctness bug. For function nodes (FunctionDeclaration, FunctionExpression, ArrowFunctionExpression), fn.body is a BlockStatement node (an object), not a Statement[] array. The Array.isArray(body) check at index.ts:3371 is always false, so nodeHasInlineCacheDirective always returns false. This means:

hasInlineCache is always false (line 3410)
Files with only function-level "use cache" directives silently skip the transform (line 3412)
The use-cache-fn-test fixture (async function getData() { "use cache"; ... }) would be broken

The fix is to unwrap the BlockStatement to get at the actual statement array: body?.type === "BlockStatement" ? body.body : body.

Non-blocking items

The after() + waitUntil fix in server.ts is solid — correct integration, good comments, good test coverage.
Pre-existing test at line 1084 still uses setTimeout while newer tests use deterministic microtick awaits.

github run

…inline use-cache detection

james-elicx · 2026-03-16T09:30:47Z

/bigbonk re-review

ask-bonk

Review

The core fix is correct and well-motivated: after() tasks were silently dropped on Cloudflare Workers because fire-and-forget microtasks don't survive past response completion. Using getRequestExecutionContext()?.waitUntil(guarded) is the right integration point. The implementation is clean — after() stays synchronous so waitUntil is registered in the same tick, and the optional chaining gracefully degrades on the Node.js dev server.

The AST-based directive detection replacing the string-based code.includes("use cache") check is the right fix for the false positive that motivated it (the "use cache" string in error messages). The BlockStatement unwrap (fn.body.type === "BlockStatement" ? fn.body.body : null) is correct — it was flagged as a bug in an earlier bonk review but has since been fixed.

The inlined cache-scope guard is well-justified by the comment explaining the Vite transform import chain problem. Error messages match the canonical throwIfInsideCacheScope in headers.ts.

All CI checks pass (Vitest, Playwright E2E across all projects, typecheck, lint, format).

Two minor items (neither blocking):

Pre-existing test timing inconsistency (tests/shims.test.ts:1084): The first after() test still uses setTimeout(r, 10) while the updated/new tests correctly use deterministic await Promise.resolve(); await Promise.resolve(). Worth aligning in a follow-up.
Inline comment below on server.ts:33 about future duplication cleanup.

ask-bonk · 2026-03-16T09:32:34Z

+
+const _USE_CACHE_ALS_KEY = Symbol.for("vinext.cacheRuntime.contextAls");
+const _UNSTABLE_CACHE_ALS_KEY = Symbol.for("vinext.unstableCache.als");
+const _g = globalThis as unknown as Record<PropertyKey, unknown>;


Nit: headers.ts uses _gHeaders to namespace its globalThis alias. Using _g here is fine since it's the only one in this file, but if this inlined pattern ever grows a third consumer, extracting the cache-scope check into a tiny leaf module (parameterized by API name, no "use cache" string literals in its own source) would eliminate the duplication with headers.ts. Not actionable now — just a future maintenance note.

ask-bonk · 2026-03-16T09:32:45Z

Review submitted successfully. Here's a summary of what I posted:

Approved with two minor non-blocking observations:

after() + waitUntil fix — correct and well-implemented. The getRequestExecutionContext()?.waitUntil(guarded) integration keeps after() synchronous so waitUntil is registered in the same tick, with graceful degradation on Node.js dev server via optional chaining. The inlined cache-scope guard is justified (comment explains the Vite transform import chain problem), and error messages match headers.ts.
AST-based directive detection — the BlockStatement unwrap (fn.body.type === "BlockStatement" ? fn.body.body : null) is correct. This properly replaces the string-based code.includes("use cache") check that caused false positives from "use cache" in error messages.
Tests — good coverage of all paths: waitUntil integration, fire-and-forget fallback, "use cache" scope guard, and unstable_cache scope guard.

Minor items noted:

Pre-existing test at line 1084 still uses setTimeout(r, 10) while new tests use deterministic microtick awaits — worth aligning in a follow-up
Inline comment about extracting the duplicated cache-scope guard into a shared leaf module if a third consumer ever appears

github run

NathanDrake2406 added 2 commits March 15, 2026 17:21

NathanDrake2406 force-pushed the fix/after-waituntil branch from 2f58e2c to a518ff6 Compare March 15, 2026 06:21

ask-bonk Bot reviewed Mar 15, 2026

View reviewed changes

james-elicx added 2 commits March 15, 2026 10:06

Merge remote-tracking branch 'origin/main' into fix/after-waituntil

93a2773

address review: fix cache-scope error message and add unstable_cache …

be66398

…test for after()

ask-bonk Bot reviewed Mar 15, 2026

View reviewed changes

james-elicx added 2 commits March 15, 2026 15:44

james-elicx force-pushed the fix/after-waituntil branch from 9a816c6 to 9b37734 Compare March 15, 2026 15:44

ask-bonk Bot approved these changes Mar 15, 2026

View reviewed changes

james-elicx added 2 commits March 16, 2026 07:21

Merge remote-tracking branch 'origin/main' into pr-543

0912564

address bonk review: clarify MethodDefinition unwrapping, unhandledre…

c052b68

…jection safety note, and ALS test isolation comment

ask-bonk Bot requested changes Mar 16, 2026

View reviewed changes

fix: unwrap BlockStatement in nodeHasInlineCacheDirective to restore …

cb85769

…inline use-cache detection

ask-bonk Bot approved these changes Mar 16, 2026

View reviewed changes

james-elicx merged commit ad368d9 into cloudflare:main Mar 16, 2026
19 checks passed

NathanDrake2406 deleted the fix/after-waituntil branch March 18, 2026 09:50

	`\`${apiName}\` cannot be called inside a cached scope. ` +
	`\`${apiName}\` cannot be called inside "use cache". ` +

	await runWithExecutionContext(mockCtx, async () => {
	await runWithExecutionContext(mockCtx, () => {

	await new Promise((r) => setTimeout(r, 10));
	await new Promise((r) => setTimeout(r, 50));

Conversation

NathanDrake2406 commented Mar 15, 2026

Summary

Test plan

Uh oh!

pkg-pr-new Bot commented Mar 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

james-elicx commented Mar 15, 2026

Uh oh!

ask-bonk Bot left a comment

Choose a reason for hiding this comment

Uh oh!

ask-bonk Bot Mar 15, 2026

Choose a reason for hiding this comment

Uh oh!

ask-bonk Bot Mar 15, 2026

Choose a reason for hiding this comment

Uh oh!

ask-bonk Bot Mar 15, 2026

Choose a reason for hiding this comment

Uh oh!

ask-bonk Bot Mar 15, 2026

Choose a reason for hiding this comment

Uh oh!

ask-bonk Bot commented Mar 15, 2026

Uh oh!

james-elicx commented Mar 15, 2026

Uh oh!

ask-bonk Bot left a comment

Choose a reason for hiding this comment

Uh oh!

ask-bonk Bot Mar 15, 2026

Choose a reason for hiding this comment

Uh oh!

ask-bonk Bot Mar 15, 2026

Choose a reason for hiding this comment

Uh oh!

ask-bonk Bot Mar 15, 2026

Choose a reason for hiding this comment

Uh oh!

ask-bonk Bot Mar 15, 2026

Choose a reason for hiding this comment

Uh oh!

ask-bonk Bot commented Mar 15, 2026

Uh oh!

james-elicx commented Mar 15, 2026

Uh oh!

NathanDrake2406 commented Mar 15, 2026

Uh oh!

james-elicx commented Mar 15, 2026

Uh oh!

james-elicx commented Mar 15, 2026

Uh oh!

james-elicx commented Mar 15, 2026

Uh oh!

NathanDrake2406 commented Mar 15, 2026

Uh oh!

ask-bonk Bot commented Mar 15, 2026

Uh oh!

james-elicx commented Mar 15, 2026

Uh oh!

NathanDrake2406 commented Mar 15, 2026

Uh oh!

ask-bonk Bot left a comment

Choose a reason for hiding this comment

Uh oh!

ask-bonk Bot Mar 15, 2026

Choose a reason for hiding this comment

Uh oh!

ask-bonk Bot Mar 15, 2026

Choose a reason for hiding this comment

Uh oh!

ask-bonk Bot Mar 15, 2026

Choose a reason for hiding this comment

Uh oh!

ask-bonk Bot Mar 15, 2026

Choose a reason for hiding this comment

Uh oh!

ask-bonk Bot Mar 15, 2026

pkg-pr-new Bot commented Mar 15, 2026 •

edited

Loading