dns: match Node's getaddrinfo error shape (errno/hostname/message/name)#30987
dns: match Node's getaddrinfo error shape (errno/hostname/message/name)#30987alii wants to merge 1 commit into
Conversation
|
Updated 5:35 AM PT - May 30th, 2026
✅ @robobun, your commit 6f481629c91a65ffb2d0723e99f549396dfcf351 passed in 🧪 To try this PR locally: bunx bun-pr 30987That installs a local version of the PR into your bun-30987 --bun |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThis PR extends DNS error handling to include the queried hostname, maps c-ares getaddrinfo errors to libuv-style negative EAI_* errno values, and converts DNS error instances’ name to "Error" so JS APIs emit Node-shaped getaddrinfo ENOTFOUND errors including errno and hostname. ChangesDNS error shape alignment
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
![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 `@test/js/node/dns/lookup-error.test.ts`:
- Around line 47-50: The test "dns.lookup({ all: true }) failure has a
Node-shaped error" calls lookupError(HOST, { all: true }) and asserts shape via
assertNodeShape(err, HOST) but omits checking err.message; add an assertion that
the error message equals the Node-like string (e.g., `getaddrinfo ENOTFOUND
${HOST}`) after obtaining err to mirror the other tests—use
assert.strictEqual(err.message, `getaddrinfo ENOTFOUND ${HOST}`) alongside the
existing assertNodeShape call.
- Around line 41-58: These three independent tests ("dns.lookup failure has a
Node-shaped error", "dns.lookup({ all: true }) failure has a Node-shaped error",
and "dnsPromises.lookup failure has a Node-shaped error") can be run in
parallel; change their declarations from test(...) to test.concurrent(...) so
Jest runs them concurrently, leaving the test bodies (calls to lookupError,
assertNodeShape, assert.rejects, and assertions) 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a99a3724-08e6-4a50-ba49-d8822375eeee
📒 Files selected for processing (3)
src/runtime/dns_jsc/cares_jsc.rssrc/runtime/dns_jsc/dns.rstest/js/node/dns/lookup-error.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
src/runtime/dns_jsc/dns.rs:2084-2099— nit: the.clone()here (and at lines 2053 and 2094) heap-allocates a freshBox<[u8]>only to reborrow it as&[u8]—error_to_deferredalready copies the bytes viabstr::String::clone_utf8, so the intermediate alloc is redundant. The six existing call sites in this file (e.g. dns.rs:983/997, 1773/1784, 1914/1925) passSome(&(*this).name)directly alongside&mut (*this).promise, so you can drop the clone for consistency.Extended reasoning...
What
At
dns.rs:2053,2085, and2094the new error paths do:let host = (*this).name.clone(); error_to_deferred(err, b"getaddrinfo", Some(&host[..]), &mut (*this).promise)
(*this).nameis aBox<[u8]>, so.clone()heap-allocates a new boxed slice and copies the hostname bytes into it. The very next line reborrows that fresh allocation as&[u8]and hands it toerror_to_deferred, which (cares_jsc.rs:798–806) doeshostname.map(bstr::String::clone_utf8)— copying the bytes a second time into the ownedbstr::Stringit actually keeps. The intermediateBox<[u8]>is then dropped at scope exit, having served only as a staging buffer.Why it is unnecessary
error_to_deferredtakesOption<&[u8]>; it never needs an owned buffer from the caller. Borrowing(*this).nameas&[u8]while simultaneously taking&mut (*this).promiseis fine: they are disjoint fields accessed through a raw-pointer place expression, which the borrow checker accepts. This is not hypothetical — the same file already does exactly that at six call sites:dns.rs:983/dns.rs:997—Some((*this).name.as_ref())with&mut (*this).promisedns.rs:1773/dns.rs:1784—Some(&(*this).name)with&mut (*this).promisedns.rs:1914/dns.rs:1925—Some(&(*this).name)with&mut (*this).promise
So the clone is not required for borrow-check reasons, and it is inconsistent with the established pattern everywhere else in the file.
Step-by-step
dns.lookup("foo.invalid")fails;process_get_addr_info_nativeis called with an error status.(*this).nameholdsb"foo.invalid"(11 bytes, one heap alloc).let host = (*this).name.clone();→ second heap alloc, 11 bytes copied.error_to_deferred(.., Some(&host[..]), ..)→bstr::String::clone_utf8makes a third copy into thebstr::Stringstored onErrorDeferred.hostdrops; the second alloc is freed having contributed nothing the original(*this).namecould not have provided directly.
Impact
Negligible in practice — error path only, hostnames are ≤253 bytes, one extra small alloc/free per failed lookup. There is no correctness issue. This is purely a consistency/cleanliness nit.
Fix
Drop the clone and pass the field directly, matching the other call sites:
error_to_deferred(err, b"getaddrinfo", Some(&(*this).name), &mut (*this).promise) .reject_later((*this).global_this());
Apply the same change at all three locations (2053, 2085, 2094).
|
@robobun fix review comments |
There was a problem hiding this comment.
Thanks — fe0b987 addresses both remaining nits (EBADNAME mapping and fail() errno/hostname). No further issues from me; deferring to a human given the cross-platform Rust DNS scope and the build failures CI flagged on b8e6dcb.
Extended reasoning...
Overview
This PR aligns dns.lookup (getaddrinfo) error objects with Node's shape across four fields (errno, hostname, message, name). It threads the queried hostname onto DNSLookup (new name: Box<[u8]> field + updated init signature) across all four backends (macOS libinfo, POSIX libc, Windows uv, c-ares), adds a getaddrinfo_uv_errno() c-ares→libuv errno mapping table, flips three b"DNSException" literals to b"Error" in cares_jsc.rs, reshapes the JS-side empty-result paths in dns.ts (throwIfEmpty) and _http_client.ts (fail()), updates resolve-dns.test.ts expectations, and adds a dual-runtime lookup-error.test.ts. There's also an unrelated one-line drive-by in wtf-bindings.cpp (assert → ASSERT).
Prior feedback addressed
Across three review rounds I flagged: (1) leftover DNSException literals in the sync-throw helpers, (2) resolve-dns.test.ts would break, (3) throwIfEmpty() still emitted the old shape, (4) getaddrinfo_uv_errno() missing the EBADNAME arm, and (5) _http_client.ts fail() not setting errno/hostname. All five are now fixed in the current diff (commits f7c4ee8 and fe0b987). No new issues found this round.
Security risks
None identified. The change only reshapes error metadata on already-failing DNS lookups; no new untrusted-input parsing, no auth/crypto/permissions surface.
Level of scrutiny
Moderate. DNS resolution is a hot, cross-platform runtime path; the DNSLookup struct gains a per-lookup heap allocation and its init signature changes across four platform-gated call sites that I can't all exercise locally. The err.name change from "DNSException" → "Error" is a user-visible behavior change (correct per Node, but could affect Bun-specific code that pattern-matches on the old name). Buildkite flagged failures on b8e6dcb (per my earlier read, build-infra on aarch64-android / x64-asan / freebsd rather than test failures), and CI status on the latest fe0b987 isn't yet reflected in the robobun comment.
Other factors
Good test coverage (new dual-runtime test plus updated Bun-API test); the author has been responsive to every round of feedback. The drive-by wtf-bindings.cpp change is trivially correct. Overall the change looks right to me, but the combination of cross-platform Rust runtime edits, a user-visible error-shape change, and unresolved CI status makes this one a human should sign off on rather than a bot.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@src/js/node/dns.ts`:
- Around line 672-682: Introduce a shared constant UV_EAI_NONAME = -3008 (near
other UV_* constants) and replace the hardcoded -3008 assignment in throwIfEmpty
(the err.errno = -3008 line inside function throwIfEmpty) and the corresponding
errno usage in the other file (the assignment at the location previously using
-3008 in _http_client.ts) to use UV_EAI_NONAME instead; ensure imports/exports
are added so throwIfEmpty and the other module reference the new constant and
keep the rest of the Error properties (code, syscall, hostname) unchanged.
In `@src/runtime/dns_jsc/cares_jsc.rs`:
- Around line 661-699: In getaddrinfo_uv_errno, change the mapping for
c_ares::Error::ECONNREFUSED (currently mapped to UV_EAI_SOCKTYPE) to use the
generic UV_EAI_FAIL constant instead so transport-level connection-refused
errors do not misreport as unsupported socket type; update the match arm for
c_ares::Error::ECONNREFUSED inside the getaddrinfo_uv_errno function
accordingly.
In `@src/runtime/dns_jsc/dns.rs`:
- Around line 224-228: The code currently clones query.name and passes the
normalized backend name into DNSLookup::init (and similar allocation sites),
causing err.hostname/message to show the rewritten name; instead, preserve the
caller-supplied hostname by threading both the original input and the normalized
lookup name separately: change DNSLookup::init (and other constructors/usages
around do_lookup and places that build GetAddrInfo) to accept two strings/byte
slices (e.g., original_hostname and lookup_name), pass Box::from(&*query.name)
as original_hostname and the normalized name as lookup_name, and make
err.hostname/message use original_hostname while lookup uses lookup_name for the
backend lookup; update related constructors and downstream fields (err.hostname,
message, any cache key logic) to retain this separation so inflight callers keep
their original hostnames.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: aa5ca73b-32ea-4f1f-8d75-6d80c196b16f
📒 Files selected for processing (7)
src/js/node/_http_client.tssrc/js/node/dns.tssrc/jsc/bindings/wtf-bindings.cppsrc/runtime/dns_jsc/cares_jsc.rssrc/runtime/dns_jsc/dns.rstest/js/bun/dns/resolve-dns.test.tstest/js/node/dns/lookup-error.test.ts
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/runtime/dns_jsc/cares_jsc.rs (1)
693-693:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
ECONNREFUSEDis mapped to the wrong libuv EAI errno.
c_ares::Error::ECONNREFUSEDshould not map toUV_EAI_SOCKTYPE; this misreports the failure category. UseUV_EAI_FAILas the generic fallback for this case.Proposed fix
- c_ares::Error::ECONNREFUSED => UV_EAI_SOCKTYPE, + c_ares::Error::ECONNREFUSED => UV_EAI_FAIL,In libuv errno definitions, what does UV_EAI_SOCKTYPE mean, and is UV_EAI_FAIL the generic getaddrinfo failure code for non-specific resolver failures?🤖 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 `@src/runtime/dns_jsc/cares_jsc.rs` at line 693, The match arm mapping c_ares::Error::ECONNREFUSED currently returns UV_EAI_SOCKTYPE, which is incorrect; update the mapping in cares_jsc.rs so that the c_ares::Error::ECONNREFUSED arm returns UV_EAI_FAIL instead (replace UV_EAI_SOCKTYPE with UV_EAI_FAIL) to use the generic getaddrinfo failure code; ensure you only change the enum/value used in the match for c_ares::Error::ECONNREFUSED and leave other mappings untouched.
🤖 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.
Duplicate comments:
In `@src/runtime/dns_jsc/cares_jsc.rs`:
- Line 693: The match arm mapping c_ares::Error::ECONNREFUSED currently returns
UV_EAI_SOCKTYPE, which is incorrect; update the mapping in cares_jsc.rs so that
the c_ares::Error::ECONNREFUSED arm returns UV_EAI_FAIL instead (replace
UV_EAI_SOCKTYPE with UV_EAI_FAIL) to use the generic getaddrinfo failure code;
ensure you only change the enum/value used in the match for
c_ares::Error::ECONNREFUSED and leave other mappings untouched.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 56f6f8cd-a5d9-42bb-8778-db52700fa840
📒 Files selected for processing (1)
src/runtime/dns_jsc/cares_jsc.rs
|
@robobun rebase and get this mergable |
robobun
force-pushed
the
ali/dns-lookup-error-node-parity
branch
from
493cdcf to
0c447de
Compare
dns.lookup (getaddrinfo) failures produced an error object that diverged from Node on four fields: - errno was the positive c-ares enum value (4) instead of libuv's negative UV_EAI_NONAME (-3008) - hostname was undefined instead of the queried name - message was "getaddrinfo ENOTFOUND" (no hostname suffix) - name was "DNSException" instead of "Error" Thread the queried name onto the DNSLookup node so every getaddrinfo failure path can build the Node-shaped error. Map the c-ares error to libuv's negative EAI_* errno for getaddrinfo/getnameinfo syscalls, and let err.name inherit from Error.prototype like Node (dropping the old DNSException put, which also corrects resolve*/reverse). Reshape the JS-side empty-result paths in dns.ts (throwIfEmpty) and _http_client.ts to the same Node shape. Test lives in the dns module: it is node:test/node:assert and runs under both bun test and node --experimental-strip-types --test.
robobun
force-pushed
the
ali/dns-lookup-error-node-parity
branch
from
0c447de to
6f48162
Compare
2 participants
dns.lookup(getaddrinfo) failures produced an error object that diverged from Node on four fields:errnowas the positive c-ares enum value (4) instead of libuv's negativeUV_EAI_NONAME(-3008)hostnamewasundefinedinstead of the queried namemessagewas"getaddrinfo ENOTFOUND"(no hostname suffix)namewas"DNSException"instead of"Error"Thread the queried name onto the
DNSLookupnode so every getaddrinfo failure path can build the Node-shaped error (this also fixesmessage, same root cause). Map the c-ares error to libuv's negativeEAI_*errno for the getaddrinfo syscall, and usename: "Error"like Node (also corrects resolve*/reverse, which Node likewise reports asError).Pre-existing (Zig 1.3.14 has the same bug), so the test lives in the dns module. It's
node:test/node:assertand runs under bothbun testandnode --experimental-strip-types --test: green under Node, fails under released Bun, green with this fix.dns.lookupoutput is now byte-identical to Node;node-dns.test.js(66) and vendoredtest-dns-lookup.js/test-dns.jsstill pass.