Skip to content

fix(runtime): define js_node_system_error_value (unbreak cargo-test link)#5124

Merged
proggeramlug merged 1 commit into
mainfrom
claude/fix-js-node-system-error-value
Jun 14, 2026
Merged

fix(runtime): define js_node_system_error_value (unbreak cargo-test link)#5124
proggeramlug merged 1 commit into
mainfrom
claude/fix-js-node-system-error-value

Conversation

@proggeramlug

@proggeramlug proggeramlug commented Jun 14, 2026

Copy link
Copy Markdown
Contributor

Summary

main's cargo-test is failing with a linker error, unrelated to any one feature:

/usr/bin/ld: ... libperry_ext_http.a(perry_ffi...): in function `perry_ffi::error::system_error_value':
undefined reference to `js_node_system_error_value'
collect2: error: ld returned 1 exit status
Error: Linking failed

Root cause

PR #5078 (1c9e3c6, "node:http client transport errors → real coded Error") added an extern declaration for js_node_system_error_value in perry-ffi/src/error.rs and a caller (system_error_value()perry-ext-http's client_events.rs), but the runtime #[no_mangle] definition was never shipped. Any program/test that links perry-ext-http therefore fails at link time. This takes down cargo-test — e.g. issue_4903_listen_callback_deferred's late_listening_listener_fires and listen_callback_is_deferred_and_this_bound.

Fix

Define js_node_system_error_value in perry-runtime/src/error.rs, mirroring the sibling js_error_value_with_code:

  • Build the Error from the message.
  • Register .code / .syscall / .errno into the message-keyed side tables that the existing object::field_get_set getters already read back (register_error_code_pub / register_error_syscall / register_error_errno — the same mechanism fs/dns/process errors use).
  • code/syscall are interned to &'static str (reusing intern_error_code).
  • A #[used] anchor pins the symbol against the auto-optimize LTO dead-strip, matching the sibling KEEP_JS_ERROR_VALUE_WITH_CODE.

No behavior change beyond making the already-intended system_error_value path link and produce a Node-shaped system error (.message/.code/.syscall/.errno).

Verification

  • cargo build -p perry-runtime -p perry-stdlib -p perry — clean.
  • cargo test -p perry --test issue_4903_listen_callback_deferred2 passed; 0 failed (both previously failed at the link step).

Context

Surfaced while babysitting CI on #5118 (perry/ui toggleSetState); the failure was unrelated to that PR. This is the focused fix for the main regression.

https://claude.ai/code/session_01HKXzNHuniuu33SAUrLSN2f


Generated by Claude Code

Summary by CodeRabbit

  • New Features
    • Added support for Node.js-compatible system errors with detailed error context, including message, code, syscall identifier, and errno value for improved error handling and debugging.

…ink)

PR #5078 added an extern declaration + caller for js_node_system_error_value
(perry-ffi: system_error_value → perry-ext-http client_events) but never
shipped the runtime #[no_mangle] definition, so any program/test linking
perry-ext-http failed at link time:

  undefined reference to `js_node_system_error_value'

This broke cargo-test on main (e.g. issue_4903_listen_callback_deferred's
late_listening_listener_fires and listen_callback_is_deferred_and_this_bound).

Define the symbol in perry-runtime mirroring js_error_value_with_code: build
the Error from the message, then register .code/.syscall/.errno into the
message-keyed side tables that the existing object::field_get_set getters
read back (register_error_code_pub / register_error_syscall /
register_error_errno). code/syscall are interned to &'static str. A #[used]
anchor pins the symbol against the auto-optimize LTO dead-strip, matching the
sibling KEEP_JS_ERROR_VALUE_WITH_CODE.

Verified: the two listener tests now pass (2 passed; 0 failed).

https://claude.ai/code/session_01HKXzNHuniuu33SAUrLSN2f
@coderabbitai

coderabbitai Bot commented Jun 14, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 784df9bb-95a1-454e-a083-f91eaf6086b4

📥 Commits

Reviewing files that changed from the base of the PR and between c970fc5 and cf6b805.

📒 Files selected for processing (1)
  • crates/perry-runtime/src/error.rs

📝 Walkthrough

Walkthrough

Adds js_node_system_error_value, a #[no_mangle] FFI function in crates/perry-runtime/src/error.rs that constructs a Node-style system Error with .message, .code, .syscall, and .errno fields from raw UTF-8 byte slices, then returns a NaN-boxed pointer. A #[used] keepalive static prevents the symbol from being dead-stripped by LTO.

Changes

Node System Error FFI Export

Layer / File(s) Summary
js_node_system_error_value implementation and LTO keepalive
crates/perry-runtime/src/error.rs
Adds js_node_system_error_value which decodes UTF-8 byte slices for message, code, and syscall, interns the strings, registers .code, .syscall, and .errno into Perry's Node error side tables, allocates the error via js_error_new_with_message, and returns a NaN-boxed f64. Also adds KEEP_JS_NODE_SYSTEM_ERROR_VALUE, a #[used] static pointing to the new symbol to prevent LTO dead-stripping.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

  • PerryTS/perry#5112: Modifies the same error.rs module, removing js_node_system_error_value in favor of new stack/prepareStackTrace CallSite logic — directly overlapping with this addition.
  • PerryTS/perry#5078: Consumes the js_node_system_error_value FFI entry point added here via perry_ffi::system_error_value to build coded system Error objects in the HTTP transport layer.

Poem

🐇 A syscall went awry, a code was assigned,
With .errno and .message perfectly aligned.
I boxed it in NaN, snug and secure,
Then anchored the symbol — LTO won't lure!
Hop hop, the error is ready to throw~ 🎉

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically describes the main change: defining the missing FFI function js_node_system_error_value to fix a linker error in cargo-test.
Description check ✅ Passed The description is comprehensive and well-structured, covering summary, root cause analysis, detailed fix explanation, verification steps, and context. It aligns well with the template requirements.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch claude/fix-js-node-system-error-value

Comment @coderabbitai help to get the list of available commands and usage tips.

@proggeramlug proggeramlug merged commit 58d5da0 into main Jun 14, 2026
15 checks passed
@proggeramlug proggeramlug deleted the claude/fix-js-node-system-error-value branch June 14, 2026 10:54
proggeramlug pushed a commit that referenced this pull request Jun 14, 2026
Bumps to 0.5.1167 to ship the js_node_system_error_value link-regression
fix (#5124) that the broken v0.5.1166 tag predates. See CHANGELOG.
proggeramlug pushed a commit that referenced this pull request Jun 14, 2026
… main merge

The merge of main (#5124, which independently defined js_node_system_error_value)
left two definitions in error.rs and lib.rs over the 2000-line cap. Drop this
PR's duplicate runtime definition (keep main's registry-based one) and move
arm_expect_continue from lib.rs into continue_client.rs.
proggeramlug pushed a commit that referenced this pull request Jun 14, 2026
…168)

Re-applies PR #5125's wall 36-44 code cleanly onto current main (which now
has #5124's js_node_system_error_value link fix, unblocking http
auto-optimize). Consolidated commit; per-wall detail in CHANGELOG.
proggeramlug added a commit that referenced this pull request Jun 14, 2026
* fix(http): node:http 100-continue handshake end-to-end (#5080)

Wires the `Expect: 100-continue` flow on both the client and the server so
the interim `100 Continue` exchange completes (previously produced no
output where Node completes the request).

Client (perry-ext-http):
- New `continue_client` raw-socket path. reqwest swallows interim 1xx
  responses, so a request carrying `Expect: 100-continue` (plain http)
  flushes its head up front (body withheld), reads the interim
  `100 Continue`, and emits a new `PendingHttpEvent::Continue` that drives
  the request's `'continue'` listeners. `req.end(body)` then hands the
  withheld body to the in-flight exchange over a oneshot, which sends it
  (chunked) and parses the final response. Delivery reuses the normal
  streaming path (`ResponseHead`/`ResponseChunk`/`ResponseEnd`).

Server (perry-ext-http-server):
- `Expect: 100-continue` with a `'checkContinue'` listener now dispatches
  that listener instead of `'request'`/the handler (Node's semantics).
  hyper auto-emits the interim `100 Continue` when the body is first
  polled, so `res.writeContinue()` is a confirmation no-op. Wired for both
  http and https; http2 defaults to the normal path.

Runtime (perry-runtime):
- Define `js_node_system_error_value`, the runtime entry the #5078
  transport-error path declared in perry-ffi but never implemented (a
  fresh ext-http link otherwise fails with an undefined reference). Builds
  a Node-style system Error carrying `.code`/`.syscall`/`.errno`.

Verified byte-for-byte against Node v22 (continue event / body
continue:payload / closed), both directions (perry client ↔ node server),
and the transport-error path now reports ECONNREFUSED/connect.

* fix(http): rustfmt + deadline-bound continue-client writes (#5080 review)

- cargo fmt (lint check was cargo fmt --check).
- Wrap the continue-exchange head/body `write_all`s in `tokio::time::timeout`
  so a stalled peer can't hang the request when `timeout` is set (the reads
  were already bounded). CodeRabbit review on #5123.

* fix(http): resolve js_node_system_error_value dup + lib.rs size after main merge

The merge of main (#5124, which independently defined js_node_system_error_value)
left two definitions in error.rs and lib.rs over the 2000-line cap. Drop this
PR's duplicate runtime definition (keep main's registry-based one) and move
arm_expect_continue from lib.rs into continue_client.rs.

* fix(http): defer 100-continue head flush to next tick (#5080 review)

CodeRabbit review: arming the Expect:100-continue raw-socket exchange at
construction froze the header snapshot too early, so a post-construction
`req.setHeader('Expect','100-continue')` never armed the continue path and
other late setHeader() calls were dropped from the on-wire head.

Match Node's nextTick head flush:
- `http.request`/overload now queue a `DeferredArmContinue` event instead of
  arming synchronously; it drains on the next event-loop tick, after the
  synchronous setHeader()/`.on('continue')` setup has run.
- `end()` and `flushHeaders()` arm too (synchronous send boundaries), so a
  request ended synchronously still engages the handshake. `arm_expect_continue`
  is idempotent, so the first boundary wins.

Verified: original repro still matches Node, and a late
`setHeader('Expect','100-continue')` now fires the continue handshake
(continue event / body continue:payload / closed).

* fix(http): add test_async_shims so perry-ext-http lib test links (cargo-test)

cargo-test (debug) failed to link the perry-ext-http lib test:
  undefined symbol: perry_ffi_spawn_blocking_with_reactor

The crate uses perry-ffi's spawn_blocking_with_reactor, whose real impl lives
in perry-stdlib (only linked into the final user program, not the lib test).
perry-ext-net and perry-ext-http-server already carry a #[cfg(test)]
test_async_shims.rs supplying synchronous shims for the perry_ffi_* async
bridge; perry-ext-http was missing one (release builds dead-stripped the
reference, hiding it locally — debug retains it). Add the same shim.

---------

Co-authored-by: Claude <noreply@anthropic.com>
proggeramlug added a commit that referenced this pull request Jun 14, 2026
…168) (#5125)

Re-applies PR #5125's wall 36-44 code cleanly onto current main (which now
has #5124's js_node_system_error_value link fix, unblocking http
auto-optimize). Consolidated commit; per-wall detail in CHANGELOG.

Co-authored-by: Ralph Küpper <ralph2@skelpo.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants