Skip to content

fix(runtime): node:dns — correct resolve/reverse error shape (syscall + hostname); 6/6 node-suite#5077

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

fix(runtime): node:dns — correct resolve/reverse error shape (syscall + hostname); 6/6 node-suite#5077
proggeramlug merged 1 commit into
mainfrom
node-dns-fix-parity

Conversation

@proggeramlug

@proggeramlug proggeramlug commented Jun 13, 2026

Copy link
Copy Markdown
Contributor

Summary

Investigated the report that node:dns was crashing on all 6 node-suite tests. The "0/6 crash" premise was stale for this environment — on a pristine origin/main worktree the dns node-suite already measured 5/6 with no panics or segfaults (the #5040 hosts-file reverse logic is correct here — this machine's /etc/hosts produces exactly node's ["proxyman.dev","enceladus.local","local","kubernetes.docker.internal"]).

Root-causing the single real failure surfaced one genuine product bug (error shape) and one non-portable test. Both fixed. node-suite dns: 5/6 → 6/6, zero regressions.

Root causes fixed

1. dns.resolve* / dns.reverse rejections missing .syscall and .hostname (real bug)

A caught dns.resolve4 error exposed only .code ("ESERVFAIL"). Node's c-ares errors also carry e.syscall === "queryA" and e.hostname === "localhost" (with errno left undefined); dns.reverse errors carry syscall === "getHostByAddr".

Before / after (this machine SERVFAILs localhost A-queries):

node : resolve4 threw: Error ESERVFAIL queryA localhost
perry: resolve4 threw: Error ESERVFAIL undefined undefined   ← before
perry: resolve4 threw: Error ESERVFAIL queryA localhost      ← after
  • dns.rs: new dns_query_error_value registers code + syscall + hostname on the message StringHeader (mirrors the existing .code side-table path); resolve_error_value/reverse_error_value route through it.
  • node_submodules/diagnostics.rs: new ERROR_MESSAGE_HOSTNAMES table + register_error_hostname/error_hostname_for_message, paralleling the syscall/path/dest tables.
  • object/field_get_set.rs: new .hostname Error getter arm (mirrors .path/.syscall). The user-assigned-prop path is consulted before this arm, so a user-set err.hostname still wins (verified against node).

2. imports/default-export.ts asserted an unportable result (test bug)

The test did const r = await resolver.resolve4("localhost") and asserted an array. resolve4 queries the configured nameserver directly — it does not consult /etc/hosts — so localhost yields an A record on some resolvers and ESERVFAIL/ENOTFOUND on others. On a SERVFAIL machine node itself aborts on the unhandled rejection, so the assertion can't pass regardless of Perry. Rewrote it to print array-or-error-code — the identical environment-robust record-or-error approach #5040 already applied to the sibling dns/resolve/localhost.ts. No product behavior was papered over: Perry already threw the correct ESERVFAIL; the fix above just completes that error's shape.

node-suite dns: before → after

before (origin/main):  5/6   (imports/default-export.ts FAIL — unhandled ESERVFAIL rejection)
after:                 6/6

All six: constants/error-aliases, imports/default-export, lookup/loopback, resolve/localhost, settings/default-result-order, settings/servers — PASS.

Regression check (zero regressions)

  • error-shape modules assert/net/dgram: clean. (Two net cases initially showed FAIL under heavy concurrent-worktree CPU load — one literally Killed: 9 — re-running net/socket/type-of-service cleanly gives byte-identical node/perry output. Both read only err.code/.name/.message, never .hostname/.syscall, so they're untouched by this change.)
  • regression probe: user-set err.hostname/err.path and a plain error's .hostname/.syscall all match node exactly.
  • perry-runtime dns_resolver unit tests: 3/3 pass.

Files

  • crates/perry-runtime/src/dns.rs
  • crates/perry-runtime/src/node_submodules/diagnostics.rs
  • crates/perry-runtime/src/object/field_get_set.rs
  • test-parity/node-suite/dns/imports/default-export.ts

Per repo policy, no Cargo.toml/CLAUDE.md/CHANGELOG.md edits (maintainer folds version + changelog at merge).

Summary by CodeRabbit

New Features

  • DNS lookup errors now include detailed error information (error code, system call details, and hostname) for improved error diagnostics.

Tests

  • Enhanced DNS test reliability across different resolver configurations.

… + hostname)

node:dns node-suite: 5/6 -> 6/6 locally (zero regressions in
assert/net/dgram + dns_resolver unit tests + error-shape probes).

Root causes:

1. resolve*/reverse rejections were missing Node's `.syscall` and
   `.hostname` own properties. A caught `dns.resolve4` error exposed only
   `.code` ("ESERVFAIL"); Node's c-ares errors also carry
   `e.syscall === "queryA"` and `e.hostname === "localhost"` (with
   `errno` left undefined). `dns.reverse` errors likewise carry
   `syscall === "getHostByAddr"`.
   - dns.rs: new `dns_query_error_value` builds the error and registers
     code + syscall + hostname on the message StringHeader (mirrors the
     existing `.code` side-table path); resolve_error_value /
     reverse_error_value route through it.
   - node_submodules/diagnostics.rs: new ERROR_MESSAGE_HOSTNAMES side
     table + register_error_hostname / error_hostname_for_message,
     paralleling the syscall/path/dest tables.
   - object/field_get_set.rs: new `.hostname` Error getter arm (mirrors
     `.path`/`.syscall`). The user-assigned-prop path is consulted first,
     so a user-set `err.hostname` still wins — verified against Node.

2. test imports/default-export.ts asserted that
   `resolver.resolve4("localhost")` returns an array. resolve4 queries the
   configured nameserver directly (it does NOT consult /etc/hosts), so
   "localhost" yields an A record on some resolvers and
   ESERVFAIL/ENOTFOUND on others — on a SERVFAIL machine Node itself
   aborts on the unhandled rejection, making the assertion unportable.
   Rewrote it to print array-or-error-code, the same environment-robust
   record-or-error approach #5040 already applied to the sibling
   dns/resolve/localhost.ts test. (No product behavior was papered over:
   Perry already threw the correct ESERVFAIL there — the fix above just
   completes that error's shape.)
@coderabbitai

coderabbitai Bot commented Jun 13, 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: c8182ffd-f141-406c-bf16-14e4264a67ef

📥 Commits

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

📒 Files selected for processing (4)
  • crates/perry-runtime/src/dns.rs
  • crates/perry-runtime/src/node_submodules/diagnostics.rs
  • crates/perry-runtime/src/object/field_get_set.rs
  • test-parity/node-suite/dns/imports/default-export.ts

📝 Walkthrough

Walkthrough

This PR adds hostname property support to DNS error objects in Perry's Node.js runtime. It introduces a message-keyed metadata side table to store hostnames, wires hostname property access into Error object field dispatch, updates DNS error construction to register and expose hostnames, and adjusts test logging for cross-platform stability.

Changes

DNS Error Hostname Support

Layer / File(s) Summary
Error hostname metadata infrastructure
crates/perry-runtime/src/node_submodules/diagnostics.rs, crates/perry-runtime/src/object/field_get_set.rs
A per-thread side table ERROR_MESSAGE_HOSTNAMES keyed by error message pointers is introduced with public registration and lookup helpers. Error object field dispatch gains a hostname property read arm that uses the lookup helper to return the hostname string as a JS value or undefined.
DNS error construction with hostname
crates/perry-runtime/src/dns.rs
A shared dns_query_error_value helper is added to construct DNS error objects with registered .code, .syscall, and .hostname properties. resolve_error_value and reverse_error_value are refactored to use this helper, replacing prior plain error construction.
Test parity adjustment
test-parity/node-suite/dns/imports/default-export.ts
DNS resolution test wraps resolve4("localhost") in try/catch and logs a string summary ("array", typeof, or "err:" + code) for consistent cross-machine results instead of logging raw values.

🎯 3 (Moderate) | ⏱️ ~25 minutes

🐰 A DNS error hops in with hostname in tow,
Through side tables keyed, the rabbits now know,
Property access and error shapes align,
Test summaries stabilize—cross-platform by design! ✨

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main product bug fix (DNS error shape) and indicates the test outcome (6/6 node-suite pass), clearly reflecting the primary changes.
Description check ✅ Passed The description thoroughly documents root causes, fixes, test results, and regression checks; it follows the template structure with Summary, Changes, and Test plan sections completed; policy compliance confirmed (no version/CLAUDE.md/CHANGELOG.md edits).
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-dns-fix-parity

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

@proggeramlug proggeramlug merged commit 77177e2 into main Jun 13, 2026
14 checks passed
@proggeramlug proggeramlug deleted the node-dns-fix-parity branch June 13, 2026 09:58
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