fix(runtime): restore js_node_system_error_value dropped by #5112 (unbreaks ALL auto-opt http/https/http2 compilation)#5122
Conversation
PR #5112's error.rs refactor deleted the #[no_mangle] js_node_system_error_value (added by #5078) and its #[used] retention static, but perry-ffi/src/error.rs still declares it extern and calls it from system_error_value(). Any build that links perry-ffi's system_error_value (libperry_ext_http.a) fails to link: undefined reference to `js_node_system_error_value' Error: Linking failed This breaks ALL auto-optimize (default) http/https/http2 compilation at HEAD. It was masked by warm auto-opt caches and never linked in CI, so it merged green; a fresh --no-cache http compile reproduces it. Restored verbatim (no #5112 replacement exists) next to the sibling keepalive anchors; all helper signatures it calls (js_error_value_with_code, js_string_from_bytes, js_nanbox_string, js_object_set_field_by_name) are unchanged post-#5112.
📝 WalkthroughWalkthroughA new ChangesNode system error FFI entrypoint
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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/error.rs`:
- Line 375: The variable assignment at line 375 in the error handling code
directly bitcasts the f64 value using to_bits() without properly decoding the
NaN-boxed pointer, which will produce an invalid object pointer before
js_object_set_field_by_name mutates it. Replace the direct bitcast with a call
to js_nanbox_get_pointer to properly decode the NaN-boxed value first, matching
the existing pattern used elsewhere in the codebase (such as at lines 648, 1090,
and 1111).
🪄 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: 2084dda8-bbfb-48d3-915b-34a26e75d57f
📒 Files selected for processing (1)
crates/perry-runtime/src/error.rs
| errno: f64, | ||
| ) -> f64 { | ||
| let err_val = js_error_value_with_code(msg_ptr, msg_len, code_ptr, code_len, 0); | ||
| let obj = err_val.to_bits() as *mut crate::object::ObjectHeader; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify direct NaN-box bitcasts to raw pointers in this file.
rg -nP 'to_bits\(\)\s+as\s+\*(mut|const)\s+' crates/perry-runtime/src/error.rs
rg -n 'js_nanbox_get_pointer\(' crates/perry-runtime/src/error.rsRepository: PerryTS/perry
Length of output: 383
🏁 Script executed:
# Find js_nanbox_get_pointer definition
rg -n 'fn\s+js_nanbox_get_pointer' crates/perry-runtime/Repository: PerryTS/perry
Length of output: 265
🏁 Script executed:
# Check context around line 375 in error.rs
sed -n '370,380p' crates/perry-runtime/src/error.rsRepository: PerryTS/perry
Length of output: 642
Decode the NaN-boxed pointer before object mutation.
Line 375 directly bitcasts f64 to *mut ObjectHeader using to_bits(), bypassing NaN-box decoding. This produces an invalid object pointer before js_object_set_field_by_name(...) mutates it. Use js_nanbox_get_pointer instead, which properly decodes the value—the file already uses this pattern at lines 648, 1090, and 1111.
Suggested fix
- let obj = err_val.to_bits() as *mut crate::object::ObjectHeader;
+ let obj = crate::value::js_nanbox_get_pointer(err_val) as *mut crate::object::ObjectHeader;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let obj = err_val.to_bits() as *mut crate::object::ObjectHeader; | |
| let obj = crate::value::js_nanbox_get_pointer(err_val) as *mut crate::object::ObjectHeader; |
🤖 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/error.rs` at line 375, The variable assignment at
line 375 in the error handling code directly bitcasts the f64 value using
to_bits() without properly decoding the NaN-boxed pointer, which will produce an
invalid object pointer before js_object_set_field_by_name mutates it. Replace
the direct bitcast with a call to js_nanbox_get_pointer to properly decode the
NaN-boxed value first, matching the existing pattern used elsewhere in the
codebase (such as at lines 648, 1090, and 1111).
Source: Coding guidelines
1 participant
Severity: HIGH — breaks ALL auto-optimize http/https/http2 compilation at HEAD
PR #5112 ("Next.js standalone walls 31-35") refactored `crates/perry-runtime/src/error.rs` and deleted the `#[no_mangle] pub unsafe extern "C" fn js_node_system_error_value` (originally added by #5078) along with its `#[used]` symbol-retention static. But `crates/perry-ffi/src/error.rs` still declares it `extern` and calls it from `system_error_value()`. So any build that links perry-ffi's `system_error_value` (which lives in `libperry_ext_http.a`) fails to link:
```
"_js_node_system_error_value", referenced from:
perry_ffi::error::system_error_value ... in libperry_ext_http.a
Error: Linking failed
```
Since auto-optimize is the default, this breaks compiling every http / https / http2 program from a cold cache. It merged green because CI never links that path and warm auto-opt caches masked it locally; a fresh `--no-cache` http compile reproduces it immediately.
This does not revert #5112 (a large, legitimate Next.js fix) — it only restores the one symbol #5112 dropped.
Fix
Re-added the deleted function verbatim (no #5112 replacement for system-error construction exists) next to the sibling `#[used]` keepalive anchors (`KEEP_JS_ERROR_VALUE_WITH_CODE` / `KEEP_JS_THROW_ERROR_WITH_CODE`), plus its own `KEEP_JS_NODE_SYSTEM_ERROR_VALUE` retention static. All helper signatures it calls are unchanged post-#5112:
The extern contract perry-ffi imports — `js_node_system_error_value(msg,len,code,len,syscall,len,errno)->f64` — is restored as a `#[no_mangle]` symbol with `#[used]` retention.
Verification
Regression confirmed on pristine origin/main (built clean):
```
"_js_node_system_error_value", referenced from:
Error: Linking failed
```
Fixed tree — fresh `--no-cache` auto-opt http link now succeeds:
```
Wrote executable: /tmp/v
Binary size: 14.6MB
```
and the binary runs (status-codes.ts prints expected output).
http/https/http2 sweep (all `.ts`, `--no-cache`, diffed vs node v26):
cargo test `-p perry-runtime -p perry-ffi -p perry-stdlib`: 1032 passed; the 3 perry-runtime failures (`dynamic_props` GC-scanner isolation flakes + `builtin_prototype_methods_reject_dynamic_new`) are pre-existing — verified identical on pristine origin/main.
Summary by CodeRabbit
Release Notes