fix(runtime): share symbol-keyed metadata by reference across wrappers of one native handle#5715
Merged
Merged
Conversation
…wrappers (Next.js resolvedPathname)
A native handle (small-id NaN-boxed POINTER, e.g. a node:http IncomingMessage)
carries per-request metadata in the symbol side table keyed by its handle id.
Node shares ONE metadata object by reference across every NodeNextRequest
wrapper that re-news around the same IncomingMessage, so:
- a wrapper read `req[NEXT_REQUEST_META]` resolves to the shared object, and
- a write-back `this._req[SYM] = this[SYM]` is harmless when `this[SYM]` is
the shared object.
In Perry the sharing broke in two ways for a late SSR-bundled wrapper:
1. WIPE: the bundled wrapper reached the write-back with an *undefined*
`this[SYM]`, clobbering the handle's existing metadata to undefined and
losing `resolvedPathname` → Next's `resolvedPathname must be set` invariant.
2. UNSEEDED READ: the bundled wrapper's own `[SYM]` was never set, so the
render's `getRequestMeta` read undefined off it even though its `_req`
handle held the shared meta.
Fix (both gated tightly to native handle-band receivers):
- set_symbol_property: an `undefined` write onto a handle-band receiver that
already holds a non-undefined entry is a no-op (never adds information; the
by-reference object the handle still points at is what Node keeps).
- js_object_get_symbol_property: on a heap-wrapper side-table miss, fall
through to the wrapper's `_req` native handle's symbol meta.
Regression tests cover the share, the wipe-guard, and that a plain heap object
setting a symbol prop to undefined still clears it.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds ChangesShared handle symbol metadata
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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: 2
🤖 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/symbol/get.rs`:
- Around line 73-78: The `get` path in `symbol/get.rs` is incorrectly rejecting
low-address heap wrappers by treating every `is_small_handle(raw)` value as a
handle before checking whether it is a valid object pointer. Update the guard in
the `get` function so it only skips true handle receivers and still allows valid
heap-backed wrappers at low addresses, using
`crate::value::addr_class::is_small_handle` together with
`crate::object::is_valid_obj_ptr` in a way that preserves the `_req` fallback
for real `ObjectHeader` instances.
In `@crates/perry-runtime/src/symbol/properties.rs`:
- Around line 235-265: The no-op guard in the symbol property write path is too
broad and currently blocks clearing any symbol on small native handles, leaving
stale values behind. Narrow the special-case in the symbol property setter to
only the Next request metadata flow by checking the specific metadata symbol
and/or the IncomingMessage metadata path in properties.rs, while keeping
ordinary handle symbol writes (including setting undefined) unchanged.
🪄 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: 9f5bb7fe-f418-4ccf-a56b-828f35086f60
📒 Files selected for processing (2)
crates/perry-runtime/src/symbol/get.rscrates/perry-runtime/src/symbol/properties.rs
added 3 commits
June 27, 2026 08:17
…test flake) The two #5437 handle-meta tests compared a heap `meta` pointer that the GC-rooted SYMBOL_PROPERTIES side table rewrites on a move while the local stays stale — deterministic serially (how it was validated), but under parallel cargo-test a sibling allocation triggers a GC mid-test → spurious `got != meta`. Wrap each body in gc_suppress()/gc_unsuppress() so the objects can't move; unsuppress before the assert so a panic can't leak suppression. The symbol-meta fix itself is unchanged.
(A) Make the handle-meta-share tests deterministic under parallel cargo-test
without relying on gc_suppress (which only gates gc_check_trigger, not the
1MB block-alloc trigger). Use an immovable NaN-boxed NUMBER for the metadata
value: numbers are never pointers, so the SYMBOL_PROPERTIES side-table
scanner never rewrites them on a GC move -> got.to_bits() == meta.to_bits()
holds across any collection. wrapper_reads_share_underlying_handle_meta also
forces a full gc() up front to compact the arena so its few allocations
can't trip a mid-test block-alloc GC. The metadata tests now use the exact
Symbol.for("NextInternalRequestMeta") symbol (required by the narrowed
guard below).
(B1) req_handle_symbol_fallback (get.rs): drop the is_small_handle pre-check.
is_valid_obj_ptr already rejects handles and accepts a valid heap object even
at a low address; the pre-check wrongly rejected a real low-address wrapper.
(B2) set_symbol_property (properties.rs): narrow the undefined-write no-op to
ONLY Symbol.for("NextInternalRequestMeta") on a handle-band receiver
(resolved+cached once). Any other handle symbol — including clearing it with
undefined — now stores normally. Added a regression test
(undefined_write_clears_non_metadata_symbol_on_handle).
…bj_ptr HEAP_MIN) The handle-meta tests failed ONLY on x86_64-LINUX CI (pass on arm64 + x86_64 macOS — proven via Rosetta). Root: is_valid_obj_ptr uses HEAP_MIN=0x1000 on Linux but 0x200_0000_0000 on macOS, so synthetic handle ids 0x4321/0x5678/0x6789 (all >= 0x1000) were classified as valid heap objects on Linux, breaking the handle-band gate. On macOS they're far below HEAP_MIN so the gate worked. Fix: ids < 0x1000 (matching real tiny native handles), rejected as non-objects on every platform. Not a GC race (corrects the prior gc_suppress/number-meta theory).
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.
What
Per-request metadata (Next's
req[Symbol.for('NextInternalRequestMeta')]) was lost across multiple JS wrappers of one native IncomingMessage handle. Perry stores symbol-keyed properties for handle-band receivers in an address-keyed side table (SYMBOL_PROPERTIES). Two bugs broke Node's by-reference sharing:get originalRequest()runsthis._req[SYM] = this[SYM]withthis[SYM]undefined, clobbering the handle's already-good meta object.[SYM]was never seeded reads its meta and misses — it should fall through to the shared handle's meta.Found via Next.js (#5437):
resolvedPathnameset on oneNodeNextRequestwrapper never reached the wrapper the page-render reads →InvariantError: resolvedPathname must be set in request metadata→ 500 on dynamic page routes.Fix (gated tightly to native handle-band receivers)
set_symbol_property: anundefinedwrite onto a handle-band receiver that already holds a non-undefined entry is a no-op (preserve the by-reference object the handle points at — Node semantics).js_object_get_symbol_property: on a heap-wrapper side-table miss, fall through to the wrapper's_reqnative-handle symbol meta, so all wrappers of one IncomingMessage share one meta object.Validation
cargo test -p perry-runtime -p perry-hir -p perry-codegen -p perry-stdlib -p perry-transform: 0 failed; symbol/handle/http tests green. Gap suite exit-0.resolvedPathname must be setnow thrown 0×; the dynamic-page render advances past it.Refs #5437.
Summary by CodeRabbit
undefinedfor the Next.js internal request-metadata symbol from clobbering existing handle-backed metadata, while preserving the normal behavior for regular heap objects and other symbols.undefinedwrite protection behavior.