fix: unstable_io() returns hanging promise during prerendering#979
Conversation
…flare#972) During prerendering (static export, --prerender-all, TPR), unstable_io() must return a hanging promise to suspend React past the IO boundary. Previously it always returned a resolved promise, matching only the browser/client implementation. Changes: - Add makeHangingPromise utility (never-resolving promise, rejects on abort) - Define typed WorkUnitStore discriminated union in workUnitAsyncStorage - unstable_io() branches on work unit store type per Next.js's io.ts - Set prerender work unit store in RSC entry when VINEXT_PRERENDER=1 - Add tests for prerender, request, cache, and abort scenarios
commit: |
|
/bigbonk review |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review
Good implementation — the core unstable_io() branching logic, makeHangingPromise, and the typed WorkUnitStore discriminated union are all faithful ports from Next.js and the tests cover the important paths. A few issues to address before merging:
Key concerns
-
AbortController in
prerender-work-unit-setup.tsis never aborted. The controller is created butabort()is never called, which means every hanging promise created during prerendering will never reject and the abort listeners will leak. Next.js aborts this signal when the prerender completes or is cancelled. This needs a plan — either abort the controller afterfn()resolves, or expose it so the caller can abort it. -
Memory leak in
makeHangingPromisewhen signal is never aborted. TheabortListenersBySignalWeakMap holds an ever-growing array of rejection callbacks per signal. If the signal is never aborted (see point 1), these callbacks accumulate for the lifetime of the AbortSignal. During prerendering with manyunstable_io()calls on one request, this could be meaningful. The WeakMap itself is module-scoped, so it persists across requests. -
Normal (non-prerender) requests don't set a work unit store. The
runWithPrerenderWorkUnithelper only sets a store whenVINEXT_PRERENDER=1. For normal requests,workUnitAsyncStorage.getStore()returnsundefined, sounstable_io()falls through to the "no store" path — which is correct today. But as more dynamic APIs start branching on the work unit store type (headers, cookies, etc.), you'll want a{ type: "request" }store set during normal request handling too. Not a blocker for this PR, but worth a TODO comment. -
Knip entries for
make-hanging-promise.tsmay be unnecessary. The comment says "knip sometimes loses the trace through barrel-free internal modules", butcache.tsdirectly importsmake-hanging-promise.ts— there's no barrel. If knip actually reports it as unused, fair enough, but this should be verified rather than preemptively added.
| renderSignal: controller.signal, | ||
| }, | ||
| fn, | ||
| ); |
There was a problem hiding this comment.
The AbortController created at line 14 is never aborted. This means:
- Hanging promises created via
makeHangingPromiseduring prerendering will never reject — they'll just be GC'd along with the signal when the request scope ends. - The abort listeners registered in
makeHangingPromiseaccumulate without cleanup.
In Next.js, the render signal is aborted when the prerender completes or is cancelled. Here, you should abort the controller after fn() resolves (or rejects):
| ); | |
| return workUnitAsyncStorage.run( | |
| { | |
| type: "prerender", | |
| renderSignal: controller.signal, | |
| }, | |
| fn, | |
| ).finally(() => controller.abort()); |
This ensures the abort listeners are cleaned up and hanging promises reject with the expected error (which is then caught by the suppressUnhandledRejection handler).
| } | ||
| } | ||
|
|
||
| const abortListenersBySignal = new WeakMap<AbortSignal, (() => void)[]>(); |
There was a problem hiding this comment.
The abortListenersBySignal WeakMap is module-scoped, so it persists across requests. The listener array per signal can grow unboundedly if many unstable_io() calls happen within a single prerender pass. This isn't a critical issue since the WeakMap entries get GC'd when the AbortSignal is collected, but it's worth noting that the cleanup depends on the signal's abort being fired (which currently doesn't happen — see the comment on prerender-work-unit-setup.ts).
With the .finally(() => controller.abort()) fix, this cleans itself up nicely.
| // The hanging promise suspends React's render indefinitely until | ||
| // the prerender is aborted or completed. | ||
| return makeHangingPromise( | ||
| workUnitStore.renderSignal, |
There was a problem hiding this comment.
The route is hardcoded to "unknown" here. In Next.js, the actual route path is passed so the error message is actionable ("Route /api/foo used unstable_io() during prerendering...").
Not a blocker, but consider adding a route field to PrerenderStore so this can provide a useful message. Even a TODO comment would help track this.
| case "generate-static-params": | ||
| case "prerender-legacy": | ||
| return _resolvedIOPromise; | ||
| default: |
There was a problem hiding this comment.
Nit: the default branch executes workUnitStore satisfies never (which is a no-op at runtime), then falls through to the return _resolvedIOPromise at line 503. This is technically correct but somewhat subtle — a reader might expect either a return or throw in the default branch. TypeScript will catch unexpected types at compile time, but for runtime defensiveness:
| default: | |
| default: | |
| workUnitStore satisfies never; | |
| return _resolvedIOPromise; |
| return _runWithUnifiedCtx(__uCtx, () => | ||
| __runWithPrerenderWorkUnit(async () => { |
There was a problem hiding this comment.
Per AGENTS.md's "dev/prod server parity" guidance: this wraps the App Router handler, but the Pages Router production server (server/prod-server.ts) and the Cloudflare worker entry (cloudflare/worker-entry.ts) are not wrapped. If prerendering also runs through those paths, they'll miss the work unit store setup.
Is prerendering App Router-only for now, or does it also use the Pages Router prod server? If the latter, the same wrapping is needed there.
| "src/server/prerender-work-unit-setup.ts", | ||
| // makeHangingPromise is imported by cache.ts, but knip sometimes loses | ||
| // the trace through barrel-free internal modules. | ||
| "src/shims/internal/make-hanging-promise.ts", |
There was a problem hiding this comment.
The comment says "knip sometimes loses the trace through barrel-free internal modules", but cache.ts imports make-hanging-promise.ts directly — there's no barrel indirection. Is this actually flagged by knip, or was it added preemptively? If the latter, it would be better to only add it if knip actually reports a false positive, to avoid masking real dead code in the future.
| expect((hanging as any).status).toBeUndefined(); | ||
|
|
||
| // Clean up by aborting the signal | ||
| controller.abort(); |
There was a problem hiding this comment.
Good cleanup, but this controller.abort() at the end is also testing the rejection path as a side effect. If the test is only about verifying the promise hangs, consider checking that the promise doesn't settle within a timeout instead:
const result = await Promise.race([
hanging.then(() => "resolved"),
new Promise(r => setTimeout(() => r("still-hanging"), 50)),
]);
expect(result).toBe("still-hanging");This would be a stronger assertion that the promise actually hangs. The current test only checks .status is undefined, which is a V8 internal property and not a guaranteed API.
|
Review posted successfully. Here's a summary of the feedback: Key findings
|
…store - AbortController now properly aborted in .finally() to prevent listener leaks - Add optional route field to PrerenderStore for better error messages - Pass pathname as route in app-rsc-entry handler - Use actual route instead of hardcoded 'unknown' in unstable_io() - Add explicit return in default branch of cache.ts for clarity - Remove premature knip entry for make-hanging-promise.ts - Replace V8-internal .status check with Promise.race pattern in tests
Move { route: __pathname } to __runWithPrerenderWorkUnit() call
instead of incorrectly appending to _handleRequest closing brace.
Fixes parse error introduced in 035a54f.
Refs cloudflare#979
|
/bigbonk review |
There was a problem hiding this comment.
Review
Good work addressing the prior review feedback — the .finally(() => controller.abort()) fix is in place, the hanging-promise test uses Promise.race instead of V8 internals, and the route field is on PrerenderStore. The core implementation is a faithful port from Next.js and the test coverage is solid.
A few remaining issues:
Must fix
- Missing unhandled-rejection suppression on early-return path —
makeHangingPromisesuppresses unhandled rejections for the normal path (line 59) but not thesignal.abortedearly-return path (line 35). If the caller doesn't synchronously.catch()the returned promise, Node will log an unhandled rejection warning. This is the exact scenario thesuppressUnhandledRejectionpattern is designed to prevent.
Should fix
-
new URL(request.url).pathnamecomputed on every request — The__pathnamevariable is extracted from the URL at line 982 on every request, but it's only used insiderunWithPrerenderWorkUnitfor the route field, which is only read whenVINEXT_PRERENDER=1. For non-prerender requests (the common case), this is wasted work. Consider moving it inside the prerender branch or lazily computing it. -
Listener array cleanup after abort — After the abort handler fires all rejection callbacks, the
listenersarray and its closures remain in the WeakMap until the signal is GC'd. This is not a hard leak (WeakMap semantics bound the lifetime), but addinglisteners.length = 0inside the abort handler would release the closure references eagerly. Minor hygiene improvement.
Observations (non-blocking)
-
Dev/prod server parity is fine here — I verified that
prod-server.ts(App Router) and the worker entry both delegate to the built RSC handler, so the wrapping inapp-rsc-entry.tsis the single chokepoint for all prerender requests. No parity issue. -
Knip entries are justified —
work-unit-async-storage.tstypes flow through ALS generics (invisible to knip), andprerender-work-unit-setup.tsis imported via template string. Both entries are legitimate. -
The
defaultbranch in the switch now has an explicitreturn(line 499) — good, this addresses the prior review's concern about the subtle fall-through.
| expression: string, | ||
| ): Promise<T> { | ||
| if (signal.aborted) { | ||
| return Promise.reject(new HangingPromiseRejectionError(route, expression)); |
There was a problem hiding this comment.
The early-return path creates a rejected promise without suppressing unhandled rejection, while the normal path (line 59) does suppress it. If the caller doesn't synchronously .catch() this promise (which is possible — React's use() may not handle it in the same microtick), Node will emit an unhandled rejection warning.
Apply the same pattern as the normal path:
| return Promise.reject(new HangingPromiseRejectionError(route, expression)); | |
| const rejected = Promise.reject(new HangingPromiseRejectionError(route, expression)); | |
| rejected.catch(suppressUnhandledRejection); | |
| return rejected; |
| }); | ||
| return _runWithUnifiedCtx(__uCtx, async () => { | ||
| // Extract pathname for prerender work unit store (used by unstable_io() for error messages) | ||
| const __pathname = new URL(request.url).pathname; |
There was a problem hiding this comment.
This new URL(request.url).pathname runs on every request, but it's only consumed by runWithPrerenderWorkUnit which only reads the route when VINEXT_PRERENDER=1. For normal (non-prerender) requests — the vast majority — this is wasted work.
Not a big deal since URL parsing is cheap, but you could avoid it by passing the URL lazily or moving the extraction inside the prerender branch. Low priority.
| "abort", | ||
| () => { | ||
| for (let i = 0; i < listeners.length; i++) { |
There was a problem hiding this comment.
Nit: after firing all rejection callbacks, the listeners array and its closures stay alive in the WeakMap until the signal is GC'd. Adding cleanup here would release the closure references eagerly:
| "abort", | |
| () => { | |
| for (let i = 0; i < listeners.length; i++) { | |
| () => { | |
| for (let i = 0; i < listeners.length; i++) { | |
| listeners[i](); | |
| } | |
| listeners.length = 0; |
Non-blocking since the signal lifetime is bounded by the prerender work unit, but it's good hygiene.
|
Review posted successfully. Here's a summary of the feedback: Key findings
Non-blocking observations
|
- Suppress unhandled rejection when signal already aborted in makeHangingPromise - Add lazy pathname extraction via getter function in runWithPrerenderWorkUnit - Add listener array cleanup after abort in makeHangingPromise - Add test for unhandled rejection suppression on early-return path - Update entry template snapshots for lazy route getter
|
/bigbonk review |
There was a problem hiding this comment.
Review
Solid implementation. The core porting from Next.js is faithful — the WorkUnitStore discriminated union, the unstable_io() branching logic, and the makeHangingPromise pattern all match the upstream implementation closely. The two prior reviews surfaced real issues (missing abort cleanup, missing unhandled-rejection suppression on the early-return path, listener array cleanup) and all of them have been addressed.
The test coverage is good: hanging promise during prerender, immediate resolution for request/cache stores, rejection on abort, pre-aborted signal handling, and the unhandled-rejection suppression test. The Promise.race approach is the right way to test the hanging behavior.
A few minor non-blocking observations below.
| } | ||
| return response; | ||
| }); | ||
| }, { route: () => new URL(request.url).pathname }) |
There was a problem hiding this comment.
Nit: the route thunk () => new URL(request.url).pathname is a nice lazy-evaluation pattern (only called when VINEXT_PRERENDER=1), but the indentation of this closing section is a bit hard to follow. The async () => { body at line 982 is indented 4 spaces, but the closing }, { route: ... }) here breaks the visual nesting.
Not worth a change — just something to keep in mind if this area gets refactored later. The generated code is already dense, so readability of the template matters more than usual.
| ): Promise<T> { | ||
| if (process.env.VINEXT_PRERENDER === "1") { | ||
| const controller = new AbortController(); | ||
| const route = typeof options?.route === "function" ? options.route() : options?.route; |
There was a problem hiding this comment.
Worth noting: when VINEXT_PRERENDER=1 and options?.route is a function, it's evaluated eagerly here (before fn() runs). This is fine because request.url is available before the handler executes, but it does mean the route value is captured at setup time, not at the point where unstable_io() is called. That's correct for the current use case — just documenting the timing for future readers.
| * Discriminated union of all known work unit types. | ||
| * Matches Next.js's WorkUnitStore: packages/next/src/server/app-render/work-unit-async-storage.external.ts | ||
| */ | ||
| export type WorkUnitStore = |
There was a problem hiding this comment.
Good forward-looking design. The discriminated union covers all Next.js work unit types even though only prerender (with renderSignal) is actively used today. This will make it straightforward to add headers()/cookies() branching later without touching the type definitions.
One thing to verify when that work happens: Next.js's RequestStore type carries much more state (headers, cookies, draft mode, etc.). This minimal { type: "request" } shape is fine for now but will need to grow.
| return rejected; | ||
| } | ||
| const hangingPromise = new Promise<T>((_, reject) => { | ||
| const boundRejection = reject.bind(null, new HangingPromiseRejectionError(route, expression)); |
There was a problem hiding this comment.
Minor: each call to makeHangingPromise for the same signal creates a new HangingPromiseRejectionError instance upfront (in the boundRejection). If a prerender has many unstable_io() calls (e.g., a page with dozens of IO boundaries), that's dozens of error objects allocated that may never be thrown (if the prerender completes successfully and the signal is aborted for cleanup rather than for an actual error).
This matches Next.js's implementation, so it's not a divergence — just noting that a possible future optimization would be to defer error creation to the abort handler. Not worth doing now.
| "src/shims/internal/utils.ts", | ||
| // Typed WorkUnitStore exports consumed by cache.ts via AsyncLocalStorage | ||
| // generic — knip cannot trace type-only dependencies through ALS. | ||
| "src/shims/internal/work-unit-async-storage.ts", |
There was a problem hiding this comment.
The comment says knip can't trace type-only dependencies through ALS. That's accurate — cache.ts imports workUnitAsyncStorage (a value), but knip may not follow the type exports (WorkUnitStore, PrerenderStore, etc.) since they're consumed via the generic parameter. The entry is justified.
|
Review submitted. Here's a summary: Approved — the implementation is a faithful port from Next.js and the prior review feedback has been addressed. What's good
Non-blocking observations left as inline comments
|
- Take main's refactored app-rsc-entry.ts as base (extracted runtime primitives) - Apply prerender-work-unit wrapping around handler function - Update tests to match refactored code structure (delegated helpers) - Update entry-templates snapshots for new generated code structure
- Keep main's tests/app-router.test.ts (already has correct refactored tests) - Update entry-templates snapshots to include prerender-work-unit generated code
|
/bigbonk review |
There was a problem hiding this comment.
Review
This PR is ready to merge. The core unstable_io() implementation is a faithful port from Next.js, all three rounds of prior review feedback have been addressed, and CI is fully green.
What's in this PR
The PR does two things:
-
unstable_io()prerender support (the fix for #972): Adds work unit type discrimination sounstable_io()returns a hanging promise during prerendering and resolves immediately in all other contexts. This is the stated goal of the PR. -
Entry-thinning refactoring in
app-rsc-entry.ts: Moves ~400 lines of inlined runtime helpers (ISR cache helpers, body readers, error sanitization, metadata route handling, public file serving,makeThenableParams) out of the generated entry template and into proper typed runtime modules. This is a welcome cleanup that aligns with the AGENTS.md guidance about keeping generated entries thin.
The large net deletion (-2941 lines) is almost entirely from #2 — the snapshot file shrinks dramatically as inlined code is replaced by imports.
Implementation quality
makeHangingPromiseis a correct port of Next.js'sdynamic-rendering-utils.ts. The WeakMap + batched listener pattern matches upstream. The two deviations (.catch(suppressUnhandledRejection)on the already-aborted path, andlisteners.length = 0cleanup) are both improvements over the Next.js version.WorkUnitStorediscriminated union covers all Next.js work unit types. Thesatisfies neverexhaustiveness check in thedefaultbranch is good practice.runWithPrerenderWorkUnitcorrectly gates onVINEXT_PRERENDER=1, aborts the controller in.finally()for cleanup, and supports lazy route evaluation via a thunk.- Test coverage is solid: 6 new tests covering hanging during prerender, immediate resolution for request/cache stores, rejection on abort, pre-aborted signal, and unhandled rejection suppression.
Prior review feedback — all addressed
- AbortController cleanup (
.finally(() => controller.abort())) ✓ - Unhandled rejection suppression on early-return path ✓
- Listener array cleanup (
listeners.length = 0) ✓ routefield onPrerenderStore✓Promise.racetest instead of V8.statusinternal ✓- Explicit
returnindefaultbranch ✓
| } | ||
| return response; | ||
| }); | ||
| }, { route: () => new URL(request.url).pathname }) |
There was a problem hiding this comment.
Nit: the indentation here is a bit jarring — the async () => { body at line 743 is indented 4 spaces, but this closing }, { route: ... }) breaks the visual nesting since it mixes the callback close, the options object, and the two wrapper function closes on two lines.
Not worth changing now — just something to keep in mind if this area gets touched again. The generated code is already dense.
| export const workUnitAsyncStorage = new AsyncLocalStorage<unknown>(); | ||
| // ── WorkUnitStore discriminated union ─────────────────────────────────── | ||
|
|
||
| export type RequestStore = { |
There was a problem hiding this comment.
Observation for future work: Next.js's RequestStore carries a lot more state (headers, cookies, draft mode, mutableCookies, etc.). When headers() / cookies() branching is added to vinext, this type will need to grow. The current minimal { type: "request" } shape is the right starting point.
|
Review submitted successfully. Here's a summary: Approved PR #979 — The PR does two things well:
All prior review feedback (3 rounds) has been addressed, CI is fully green across all 20+ checks, and test coverage is solid with 6 new tests plus substantive improvements to existing tests (ISR timing assertions, metadata route corrections, server action validation tightening). Left two non-blocking observations as inline comments about indentation readability and future |
2 participants
Fixes #972
Summary
What changed
Test plan