fix(classes): value-super chain-walk skipped intermediate user ctors above a fetch builtin — Next.js middleware 500s#6547
Open
proggeramlug wants to merge 3 commits into
Conversation
added 2 commits
July 17, 2026 23:39
…e base/derived id collision
Class captures are stashed on each instance as `this.__perry_cap_<id>`
fields and rebound in member prologues. Local ids restart per module, and
`super()` runs the PARENT constructor's stashes on the SHARED instance —
so a derived-class method rebinding its own capture id K silently read
the PARENT MODULE's captured local K whenever the parent chain crossed
modules and a parent capture shared the id.
This is how a Next.js 16 standalone server died at boot: base-server.js
captures `_path = _interop_require_wildcard(require("path"))` and
next-server.js captures `_fs = _interop_require_default(require("fs"))`
under the same local id 13. `Server`'s ctor calls the virtual
`this.getBuildId()` before NextNodeServer's stashes exist; the override's
`_fs` rebind found base-server's `_path` wildcard (`_fs.default ===
path-namespace`), `readFileSync` dispatched on `path`, returned
undefined, and `.trim()` threw — 100% deterministic, before Ready.
Fix: the field name embeds a stable per-module salt
(`__perry_cap_<id>m<salt>`, new `perry_hir::cap_fields`). Same-module
inheritance keeps sharing parent stashes (equal salt ⇒ equal names);
cross-module chains are isolated. Only `synthesize_class_captures`
constructs names now; every consumer that needs the outer id back
(ctor-arg alignment, factory specialization, capture writeback, the
shared-mutable passes) parses the leading digits via
`cap_field_outer_id` instead of assuming the whole suffix is numeric —
the shared-mutable passes match by parsed id, so they need no salt.
Test: test_gap_cross_module_class_capture_ids.ts — base/derived in
separate modules with colliding capture ids and a virtual call from the
base ctor (the exact Next.js shape); byte-identical to node
--experimental-strip-types. Pre-existing (unrelated) failure noted while
sweeping: test_gap_2159_defineproperty_class_prototype fails identically
on pristine main.
Claude-Session: https://claude.ai/code/session_01KD4GTmiwZjRrxShzQydJpF
…er ctor above a fetch builtin
`js_fetch_or_value_super`'s stale-alias recovery walked the instance's
WHOLE parent chain for a recorded Request/Response parent. For
`class Hint extends NextRequest extends Request` (parent imported from
another module), the Hint's `super(input, init)` was classified as a
DIRECT native-Request super: the runtime constructed the native handle,
attached it, and returned — never invoking NextRequest's constructor
body. Its symbol-keyed internal state (`this[INTERNALS] = {...}`) never
existed, so every getter on it read `undefined.<prop>`.
This is Next.js middleware's exact shape (NextRequestHint extends
NextRequest extends Request in next/dist/server/web): with the server
finally booting, every request 500'd with "Cannot read properties of
undefined (reading 'nextUrl')" plus an unhandled-rejection pair per
request.
Fix: a parent value that resolves to a USER constructor (ClassRef,
per-evaluation class object, or closure) always takes the value-super
dispatch below — its own `super()` leg attaches the native handle when
it reaches the builtin. The chain-walk fallback keeps covering its
original case only: a stale/unresolvable alias of the builtin itself
(`class X extends GlobalRequest` with the alias var dead at super time).
Test: test_gap_cross_module_fetch_subclass_super.ts — the two-level
cross-module chain constructed from both a native Request instance and a
string URL, plus the direct-subclass control; byte-identical to
node --experimental-strip-types. Pre-fix the hint's getters threw.
Claude-Session: https://claude.ai/code/session_01KD4GTmiwZjRrxShzQydJpF
📝 WalkthroughWalkthroughChangesSalted class capture naming
Fetch super dispatch
GC-safe JSON replacers
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/perry-runtime/src/object/global_this/fetch_globals.rs`:
- Around line 666-686: Move the `parent_is_user_ctor` computation into the
`identify_global_builtin_constructor(parent_val).or_else` closure so pointer
checks run only after builtin identification fails. Extract the shared
tag-and-pointer classification into a local `is_user_ctor` helper, use it for
the deferred check, and replace the duplicated `usable` logic later in the
function with that helper.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 585a042b-c82a-483e-a6bd-4e28d7a703e8
📒 Files selected for processing (13)
crates/perry-codegen/src/lower_call/capture_writeback.rscrates/perry-codegen/src/lower_call/new_ctor_args.rscrates/perry-hir/src/cap_fields.rscrates/perry-hir/src/lib.rscrates/perry-hir/src/lower/context.rscrates/perry-hir/src/lower/shared_mutable_capture.rscrates/perry-hir/src/lower_decl/class_captures.rscrates/perry-runtime/src/object/global_this/fetch_globals.rscrates/perry-transform/src/inline/factory_specialize.rstest-files/_helpers/cross_module_cap_ids_base.tstest-files/_helpers/cross_module_fetch_super_base.tstest-files/test_gap_cross_module_class_capture_ids.tstest-files/test_gap_cross_module_fetch_subclass_super.ts
Comment on lines
+666
to
+686
| let parent_is_user_ctor = { | ||
| let bits = parent_val.to_bits(); | ||
| const TAG_MASK: u64 = 0xFFFF_0000_0000_0000; | ||
| const POINTER_TAG: u64 = 0x7FFD_0000_0000_0000; | ||
| const INT32_TAG: u64 = 0x7FFE_0000_0000_0000; | ||
| const PTR_MASK: u64 = 0x0000_FFFF_FFFF_FFFF; | ||
| match bits & TAG_MASK { | ||
| INT32_TAG => true, // ClassRef | ||
| POINTER_TAG => { | ||
| let p = (bits & PTR_MASK) as usize; | ||
| crate::closure::is_closure_ptr(p) | ||
| || super::super::class_registry::is_class_object_ptr(p as *const u8) | ||
| } | ||
| _ => false, | ||
| } | ||
| }; | ||
| let kind = super::super::class_registry::identify_global_builtin_constructor(parent_val) | ||
| .or_else(|| { | ||
| if parent_is_user_ctor { | ||
| return None; | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Defer the parent_is_user_ctor check to avoid memory reads on the hot path.
Computing parent_is_user_ctor unconditionally performs pointer dereferences (via is_closure_ptr and is_class_object_ptr) on every value-super call. Move this logic inside the .or_else closure so it only executes when identify_global_builtin_constructor fails.
Additionally, this exact logic duplicates the usable check further down in this function (around line 797). Consider extracting it into a shared helper closure to DRY the code.
⚡ Proposed fix
+ let is_user_ctor = |val: f64| -> bool {
+ let bits = val.to_bits();
+ const TAG_MASK: u64 = 0xFFFF_0000_0000_0000;
+ const POINTER_TAG: u64 = 0x7FFD_0000_0000_0000;
+ const INT32_TAG: u64 = 0x7FFE_0000_0000_0000;
+ const PTR_MASK: u64 = 0x0000_FFFF_FFFF_FFFF;
+ match bits & TAG_MASK {
+ INT32_TAG => true, // ClassRef
+ POINTER_TAG => {
+ let p = (bits & PTR_MASK) as usize;
+ crate::closure::is_closure_ptr(p)
+ || super::super::class_registry::is_class_object_ptr(p as *const u8)
+ }
+ _ => false,
+ }
+ };
- let parent_is_user_ctor = {
- let bits = parent_val.to_bits();
- const TAG_MASK: u64 = 0xFFFF_0000_0000_0000;
- const POINTER_TAG: u64 = 0x7FFD_0000_0000_0000;
- const INT32_TAG: u64 = 0x7FFE_0000_0000_0000;
- const PTR_MASK: u64 = 0x0000_FFFF_FFFF_FFFF;
- match bits & TAG_MASK {
- INT32_TAG => true, // ClassRef
- POINTER_TAG => {
- let p = (bits & PTR_MASK) as usize;
- crate::closure::is_closure_ptr(p)
- || super::super::class_registry::is_class_object_ptr(p as *const u8)
- }
- _ => false,
- }
- };
let kind = super::super::class_registry::identify_global_builtin_constructor(parent_val)
.or_else(|| {
- if parent_is_user_ctor {
+ if is_user_ctor(parent_val) {
return None;
}You can then replace the duplicated logic around line 797 with the same closure:
let usable = is_user_ctor(parent_val);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/perry-runtime/src/object/global_this/fetch_globals.rs` around lines
666 - 686, Move the `parent_is_user_ctor` computation into the
`identify_global_builtin_constructor(parent_val).or_else` closure so pointer
checks run only after builtin identification fails. Extract the shared
tag-and-pointer classification into a local `is_user_ctor` helper, use it for
the deferred check, and replace the duplicated `usable` logic later in the
function with that helper.
`stringify_array_with_replacer_pretty` / `stringify_object_with_replacer_ pretty` held raw pointers (the array/object being walked, its keys array, the element/field base pointers, and the replacer ClosureHeader) across `call_replacer` / `apply_to_json_keyed` — arbitrary JS that allocates. A minor GC mid-loop can PROMOTE (move) any of those objects; the stale `elements` base then yields garbage f64s whose NaN-boxed "pointers" faulted in whatever shape-probe touched them first. This was the first of the gscmaster ~10-render crash sites: React Flight serializes the RSC payload via JSON.stringify(model, resolveModelToJSON) deep-recursively; the crash fired the first time a GC landed mid- serialization (deterministically request 12 at -O0), with varying victims (temporal::dispatch::get_property, url::search_params …) depending on which probe consumed the garbage pointer. Fix: each recursion frame roots the container and the replacer in a RuntimeHandleScope (rewritten on evacuation/promotion) and re-derives every raw pointer from the roots on each loop iteration, after the callbacks have run. Verified: at -O0 the crash stack moved off json::replacer entirely (the next unrooted site in the async-iterator path is tracked separately) — pre-fix the segfault backtrace ran through stringify_array_with_replacer_pretty × 14 frames. Claude-Session: https://claude.ai/code/session_01KD4GTmiwZjRrxShzQydJpF
Contributor
Author
|
Pushed an additional commit on this branch: |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/perry-runtime/src/json/replacer.rs (1)
450-454: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winRe-derive
replaceracross JS callbacks to prevent dangling pointers.While
obj_rootandarr_rootare correctly re-derived for the holder, thereplacerpointer derived at the top of the loop survivesapply_to_json_keyed(which executes arbitrary user JS viatoJSON). As noted in the codebase comments, a minor GC mid-callback can promote the closure, leavingreplacerdangling when passed tocall_replacer.Both stringifier loops must re-derive
replacerimmediately before use. (Note: it must also be re-derived before the recursivedispatch_pointer_with_replacercalls on lines 488 and 594, ascall_replaceritself executes JS).
crates/perry-runtime/src/json/replacer.rs#L450-L454: Re-derivereplacer = replacer_root.get_raw_const_ptr::<crate::ClosureHeader>()before passing it tocall_replacer.crates/perry-runtime/src/json/replacer.rs#L578-L583: Re-derivereplacer = replacer_root.get_raw_const_ptr::<crate::ClosureHeader>()before passing it tocall_replacerin the array path.(As an additional note for follow-up: the unchanged
stringify_array_with_array_replacerfunction at line 1252 suffers from a similar hazard. It deriveselementsoutside its loop and callstoJSONinside, which will dangleelementson the next iteration. It needs the sameRuntimeHandleScoperooting applied here.)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/perry-runtime/src/json/replacer.rs` around lines 450 - 454, Re-derive the rooted replacer pointer immediately before each call to call_replacer in both stringifier loops: crates/perry-runtime/src/json/replacer.rs lines 450-454 and 578-583. In each site, refresh replacer from replacer_root after callbacks and before use, and ensure the same refreshed pointer is available before recursive dispatch_pointer_with_replacer calls. Do not modify stringify_array_with_array_replacer; it is follow-up context only.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@crates/perry-runtime/src/json/replacer.rs`:
- Around line 450-454: Re-derive the rooted replacer pointer immediately before
each call to call_replacer in both stringifier loops:
crates/perry-runtime/src/json/replacer.rs lines 450-454 and 578-583. In each
site, refresh replacer from replacer_root after callbacks and before use, and
ensure the same refreshed pointer is available before recursive
dispatch_pointer_with_replacer calls. Do not modify
stringify_array_with_array_replacer; it is follow-up context only.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3582fb12-cf13-4e92-bf64-6c133a617657
📒 Files selected for processing (1)
crates/perry-runtime/src/json/replacer.rs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #6545 (first commit here is that PR's commit; review the second commit only —
fetch_globals.rs+ tests).Problem
js_fetch_or_value_super's stale-alias recovery walked the instance's whole parent chain for a recorded Request/Response parent. Forclass Hint extends NextRequest extends Request— with the parent imported from another module — the Hint'ssuper(input, init)was classified as a direct native-Request super: the runtime constructed the native handle, attached it tothis, and returned, never invokingNextRequest's constructor body. Its symbol-keyed internal state (this[INTERNALS] = {...}) never existed, so every getter on it readundefined.<prop>.Impact
Next.js middleware's exact shape (
NextRequestHint extends NextRequest extends Requestinnext/dist/server/web): with the server finally booting (#6522 / #6527 / #6537 / #6545), every request 500'd withCannot read properties of undefined (reading 'nextUrl')plus an unhandled-rejection pair per request. With this fix the error is gone (the remaining render blocker is #6546, an unrelated Flight/typeof miscompile).Runtime probes that pinned it: the failing receiver was the correct Hint instance (
sourcePageset by its own ctor body,__perry_fetch_handle__attached) but the parent'sPRB-CTORprobe never printed andglobalThismarkers set by the parent ctor stayed unset — the intermediate constructor genuinely never ran.Fix
A parent value that resolves to a user constructor (ClassRef, per-evaluation class object, or closure) always takes the value-super dispatch — its own
super()leg attaches the native handle when it reaches the builtin. The chain-walk fallback keeps covering only its original case: a stale/unresolvable alias of the builtin itself (class X extends GlobalRequestwith the alias var dead at super time, the @hono/node-server shape).Test
test-files/test_gap_cross_module_fetch_subclass_super.ts(+ helper module) — the two-level cross-module chain constructed from both a nativeRequestinstance and a string URL, plus the direct-subclass control. Byte-identical tonode --experimental-strip-types; pre-fix the hint's getters threw.https://claude.ai/code/session_01KD4GTmiwZjRrxShzQydJpF
Summary by CodeRabbit
super()dispatch for cross-moduleRequestsubclasses, including user-constructor handling.replacerpretty-printing to keep pointers valid across callbacks.fetch/Requestsubclasssuper()construction.