Skip to content

fix(runtime): node:http client transport errors → real coded Error (code/syscall/errno)#5078

Merged
proggeramlug merged 1 commit into
mainfrom
node-http-fix-parity
Jun 13, 2026
Merged

fix(runtime): node:http client transport errors → real coded Error (code/syscall/errno)#5078
proggeramlug merged 1 commit into
mainfrom
node-http-fix-parity

Conversation

@proggeramlug

@proggeramlug proggeramlug commented Jun 13, 2026

Copy link
Copy Markdown
Contributor

What

A failed http.request / http.get handed request.on('error') listeners a bare string instead of Node's real system Error. Every library that branches on err.code === 'ECONNREFUSED' (most HTTP clients do) saw undefined.

# before (Perry)              # node v26
typeof: string                typeof: object (Error)
code:    undefined            code:    ECONNREFUSED
syscall: undefined            syscall: connect
errno:   undefined            errno:   -61
message: undefined            message: connect ECONNREFUSED 127.0.0.1:1

Root causes

  1. reqwest::Error::to_string() carries only the top-level "error sending request for url (...)" text — the OS reason (Connection refused) lives in the source() chain, so client_events::error_event_arg's substring match never fired and fell through to the plain-string branch.
  2. Even the recognized path only set .code; Node also sets .syscall, .errno, and a ${syscall} ${CODE} ${detail} message.

Fix

  • js_node_system_error_value (runtime FFI, perry-runtime/src/error.rs): builds an Error with .message + .code (the message→code side table the .code getter reads) and .syscall / .errno own properties (the Error expando store), exposed as perry_ffi::system_error_value. Purely additive — no existing error path changed.
  • perry-ext-http::transport_error: classifies a reqwest::Error (+ request URL) into (message, code, syscall, errno) by walking the source() chain for the underlying std::io::Error (raw_os_error() → exact errno, ErrorKind → code), plus a text trail for DNS failures (getaddrinfo ENOTFOUND). Recognized failures push a new PendingHttpEvent::TransportError that the drain turns into the coded Error; anything unrecognized keeps the legacy string path, so the existing socket hang up / ECONNRESET surface is untouched.

Handles ECONNREFUSED, ENOTFOUND (getaddrinfo), ETIMEDOUT, ECONNABORTED, EADDRNOTAVAIL, EHOSTUNREACH, ENETUNREACH.

Verification (node v26 vs Perry, byte-for-byte, this host)

New node-suite test http/client-request/transport-error.ts matches node exactly:

refused:  object|Error|connect ECONNREFUSED 127.0.0.1:1|ECONNREFUSED|connect|-61|true
notfound: object|Error|getaddrinfo ENOTFOUND does-not-exist.invalid|ENOTFOUND|getaddrinfo|-3008|true

node-suite http: 18/18 → 19/19 (0 regressions).

Note: the mandate's "node:http 38.9% (7/18), 11 crashes" baseline was stale — all 18 prior http node-suite tests already passed on main; this PR adds genuine client error-path correctness on top and pins it with a 19th test.

Regression check (re-ran modules not touched)

  • https 5/5 — shares the client-dispatch path; the change only alters the request-send failure branch, so success paths are unaffected.
  • net 12/13, dns 5/6 — the 2 failures (net/server/connection-state-limits connection-event ordering; dns/imports/default-export resolver-rejection stack formatting + env-dependent ESERVFAIL localhost) are pre-existing, in perry-ext-net / perry-runtime/dns.rs — neither file is touched by this PR.

Per contributor rules, no Cargo.toml version / CLAUDE.md / CHANGELOG.md edits (maintainer folds metadata at merge).

Summary by CodeRabbit

  • Bug Fixes

    • Enhanced HTTP request error handling to provide structured error details (code, syscall, errno) for transport failures, enabling better diagnostics for connection and resolution issues.
  • Tests

    • Added HTTP transport error test coverage for connection refusal and DNS resolution failure scenarios.

…ode/syscall/errno)

A failed `http.request`/`http.get` handed `request.on('error')` listeners a
bare string instead of Node's real system `Error`, so every library that
branches on `err.code === 'ECONNREFUSED'` (which is most of them) saw
`undefined`.

Two root causes:
  * `reqwest::Error::to_string()` carries only the top-level "error sending
    request for url (...)" text — the OS reason ("Connection refused") lives
    in the `source()` chain, so `client_events::error_event_arg`'s substring
    match never fired and fell through to the plain-string branch.
  * even the recognized path only set `.code`; Node also sets `.syscall`,
    `.errno` and a `${syscall} ${CODE} ${detail}` message.

Fix:
  * new runtime FFI `js_node_system_error_value` builds an `Error` with
    `.message` + `.code` (message→code side table) and `.syscall`/`.errno`
    own properties (Error expando store), exposed via
    `perry_ffi::system_error_value`.
  * `perry-ext-http::transport_error` classifies a `reqwest::Error` (+ request
    URL) into `(message, code, syscall, errno)` by walking the `source()`
    chain for the underlying `io::Error` (`raw_os_error()` → exact errno,
    `ErrorKind` → code) and a text trail for DNS failures (`getaddrinfo
    ENOTFOUND`). Recognized failures push a new `PendingHttpEvent::TransportError`
    that the drain turns into the coded Error; anything unrecognized keeps the
    legacy string path (so the existing `socket hang up`/ECONNRESET surface is
    untouched).

Now byte-identical to node v26:
  connect ECONNREFUSED 127.0.0.1:1 | ECONNREFUSED | connect | -61
  getaddrinfo ENOTFOUND host       | ENOTFOUND    | getaddrinfo | -3008

node-suite http: 18/18 → 19/19 (adds client-request/transport-error.ts).
https 5/5 unaffected (the change only alters the request-send *failure*
branch); runtime/ffi changes are purely additive.
@coderabbitai

coderabbitai Bot commented Jun 13, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

This PR adds structured transport error reporting for HTTP requests. Reqwest errors are now classified into Node-style error objects with code, syscall, and errno fields instead of generic strings. A new TransportError event carries this information through the HTTP event system to error handlers and the JavaScript layer.

Changes

HTTP Transport Error Classification

Layer / File(s) Summary
Transport error classifier
crates/perry-ext-http/src/transport_error.rs
New classify_reqwest function parses reqwest error chains and URL details to produce Node-shaped (message, code, syscall, errno) tuples for known patterns (connect, DNS, connection refused) or None for unrecognized failures; includes unit tests for parsing, message formatting, and error-kind mapping.
PendingHttpEvent model extension
crates/perry-ext-http/src/lib.rs
New transport_error module imported; PendingHttpEvent enum gains TransportError { request_handle, message, code, syscall, errno } variant to carry structured error information.
Request dispatch classification
crates/perry-ext-http/src/client_dispatch.rs
In dispatch_request, non-timeout reqwest errors are classified via classify_reqwest; classified errors emit PendingHttpEvent::TransportError with structured fields, others fall back to generic PendingHttpEvent::Error.
Transport error event handler
crates/perry-ext-http/src/client_events.rs
New handle_transport_error_event function marks request complete, converts transport fields into a Node-coded Error via perry_ffi::system_error_value, fires 'error' listeners, and emits 'close'.
Event loop drain dispatch
crates/perry-ext-http/src/lib.rs
Main pending-event drain matches PendingHttpEvent::TransportError and routes to handle_transport_error_event with request handle and error fields.
FFI Node error construction
crates/perry-ffi/src/error.rs, crates/perry-runtime/src/error.rs
FFI declaration and Rust helper system_error_value enable building Node-style Error objects with .code, .syscall, and .errno properties; runtime implementation creates base Error then sets properties via js_object_set_field_by_name; keepalive anchor prevents LTO dead-stripping.
FFI re-export formatting
crates/perry-ffi/src/lib.rs
Re-export block reformatted to multi-line; system_error_value added to exported error helpers.
Transport error integration test
test-parity/node-suite/http/client-request/transport-error.ts
Test verifies HTTP error listeners receive real Error objects with code, syscall, and errno properties; tests connection refusal and DNS lookup failure scenarios.

🎯 3 (Moderate) | ⏱️ ~25 minutes

🐰 Hops along the error path with glee,
Where transport woes now Error be,
With code and syscall, errno too—
Node-style errors shine right through!
A classified cascade of truth so clear,

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title directly summarizes the main change: converting http client transport errors from bare strings to real Node-coded Error objects with code, syscall, and errno properties.
Description check ✅ Passed The description comprehensively covers the problem, root causes, solution, verification steps, and regression checks, following the template structure with clear sections.
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 node-http-fix-parity

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

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 `@test-parity/node-suite/http/client-request/transport-error.ts`:
- Line 40: Replace the fixed setTimeout keepalive (setTimeout(() => {}, 2000))
with deterministic completion gating: create a Promise (or promises) that
resolve when the test's DNS/error callbacks are invoked (e.g., resolve inside
the request/error callback handlers used in this test) and await Promise.all (or
Promise.race with a reasonable per-request timeout to avoid hanging) so the test
only finishes after those callbacks run (or the timeout elapses); remove the
hardcoded 2s sleep and use the promise-based completion guard instead.
🪄 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: db6efd75-e1fe-4ec2-bdd5-e1b9658d94bd

📥 Commits

Reviewing files that changed from the base of the PR and between ab5b7e8 and ebf2b3e.

📒 Files selected for processing (8)
  • crates/perry-ext-http/src/client_dispatch.rs
  • crates/perry-ext-http/src/client_events.rs
  • crates/perry-ext-http/src/lib.rs
  • crates/perry-ext-http/src/transport_error.rs
  • crates/perry-ffi/src/error.rs
  • crates/perry-ffi/src/lib.rs
  • crates/perry-runtime/src/error.rs
  • test-parity/node-suite/http/client-request/transport-error.ts

});
});

setTimeout(() => {}, 2000);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Replace fixed sleep with deterministic completion gating.

Line 40 uses a hardcoded 2s keepalive, so the test can exit before the DNS error callback fires on slower resolvers/networks, causing flaky/missing parity output. Track completion from callbacks (and optionally add per-request timeout) instead of relying on wall-clock sleep.

Suggested fix
-function describe(label: string, e: any) {
+function describe(label: string, e: any) {
   console.log(
     label,
     [
       typeof e,
@@
-// Port 1 is reserved/unused — a connect there is refused immediately.
+let doneCount = 0;
+const done = () => {
+  doneCount += 1;
+  if (doneCount === 2) {
+    // allow event loop to drain naturally once both errors are observed
+  }
+};
+
+// Port 1 is reserved/unused — a connect there is refused immediately.
 const refused = http.get({ host: "127.0.0.1", port: 1, path: "/" }, () => {});
 refused.on("error", (e: any) => {
   describe("refused:", e);
+  done();
@@
   notfound.on("error", (e2: any) => {
     describe("notfound:", e2);
+    done();
   });
 });
-
-setTimeout(() => {}, 2000);
🤖 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 `@test-parity/node-suite/http/client-request/transport-error.ts` at line 40,
Replace the fixed setTimeout keepalive (setTimeout(() => {}, 2000)) with
deterministic completion gating: create a Promise (or promises) that resolve
when the test's DNS/error callbacks are invoked (e.g., resolve inside the
request/error callback handlers used in this test) and await Promise.all (or
Promise.race with a reasonable per-request timeout to avoid hanging) so the test
only finishes after those callbacks run (or the timeout elapses); remove the
hardcoded 2s sleep and use the promise-based completion guard instead.

@proggeramlug proggeramlug merged commit 1c9e3c6 into main Jun 13, 2026
13 of 14 checks passed
@proggeramlug proggeramlug deleted the node-http-fix-parity branch June 13, 2026 09:58
proggeramlug added a commit that referenced this pull request Jun 14, 2026
…ink) (#5124)

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

Co-authored-by: Claude <noreply@anthropic.com>
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>
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.

1 participant