[Flight] Fix visitAsyncNode infinite recursion with circular async nodes #35246
Conversation
The visitAsyncNode function used null as both an "in progress" marker and a valid cached result. When a node was revisited during its own traversal (circular reference), the function returned null as if it were a cached result, which could lead to infinite recursion. This change: - Introduces an IN_PROGRESS sentinel Symbol to distinguish nodes currently being evaluated from cached results - Returns null on cycle detection to indicate "no I/O found on this cyclic path" (not undefined, which would signal abort semantics and skip emitting I/O info for other non-cyclic branches) - Always caches the computed result after visitAsyncNodeImpl returns, including null/undefined values This fixes RangeError: Maximum call stack size exceeded that occurs in Next.js 15.5.0+ when using database clients like Gel/EdgeDB that create circular promise chains in their async sequences.
|
Hi @jere-co! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks! |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
|
@jere-co, you savior, i hadn't seen the patch in the repo, seems to work perfectly! This allows me to use a Next.js version without the infamous CVE, otherwise i had to stay on a vulnerable version (bug appears since v16.0.2-canary.2 exactly in my case, isn't present in the previous canary). I'm sure Seb is mighty busy so i woudn't tag him for a few more days at least, but without your patch i was lost. |
|
Well maybe now we could bother them with an @? 😅 |
|
Comparing: 6a0ab4d...2224817 Critical size changesIncludes critical production bundles, as well as any change greater than 2%:
Significant size changesIncludes any change greater than 0.2%: (No significant changes) |
|
@jere-co There are some type issues that need to be resolved. |
| // Return null to indicate no I/O was found on this cyclic path. We don't return | ||
| // undefined here because that signals "abort" semantics which would skip emitting | ||
| // I/O info for other non-cyclic branches of this node. | ||
| return null; |
There was a problem hiding this comment.
In the PR description you write:
Returns null on cycle detection to indicate "no I/O found on this cyclic path" (not undefined, which would signal abort semantics and skip emitting I/O info for other non-cyclic branches)
However, this doesn't actually fix the reported issue. I've synced your fix to the repro app and it still runs into a stack overflow. The fix just appears to work because it's not applied correctly here:
That patch returns undefined instead of null. If we did that, we'd drop I/O info for valid cases, which you can see with the failing unit tests that this change would trigger.
Also, I think the IN_PROGRESS sentinel as implemented here doesn't actually change the cycle detection behavior. The original code already returns null for a cycle (via visited.get(node) returning the null that was set before recursing). The sentinel would only matter if we wanted to return something different on cycle detection.
It seems like the DB library is producing very long linear chains of async nodes via previous pointers, and the stack overflow comes from the recursive traversal of these chains, not from undetected cycles. A proper fix might require converting the previous chain traversal from recursive to iterative, though this gets complicated because the traversal also needs to handle awaited branches and return values need to propagate correctly.
5 participants
The
visitAsyncNodefunction usednullas both an "in progress" marker and a valid cached result. When a node was revisited during its own traversal (circular reference), the function returnednullas if it were a cached result, which could lead to infinite recursion.This bug was introduced in #35005 when
visitedwas changed from aSetto aMapfor caching return values.This change:
IN_PROGRESSsentinel Symbol to distinguish nodes currently being evaluated from cached resultsnullon cycle detection to indicate "no I/O found on this cyclic path" (notundefined, which would signal abort semantics and skip emitting I/O info for other non-cyclic branches)visitAsyncNodeImplreturns, includingnull/undefinedvalues, so revisits get the real computed value instead of the sentinelThis fixes
RangeError: Maximum call stack size exceededthat occurs in Next.js 15.5.0+ when using database clients like Gel/EdgeDB that create circular promise chains in their async sequences.How did you test this change?
Validated manually in Next.js apps (15.5, 16.0.5, 16.1.0-canary) using the Gel/EdgeDB repro that previously hit the stack overflow. No React unit test added; the internal module system made mocking
getAsyncSequenceFromPromiseimpractical from the test suite. Fix verified via integration runs only.Reproduction Repository
https://github.com/jere-co/next-debug
This repository contains:
/docs