Report DNS lookup failures from fetch() and Bun.connect as ENOTFOUND - #32990
Conversation
fetch() to a hostname that does not resolve rejected with
ConnectionRefused ("Unable to connect. Is the computer able to access
the url?") and Bun.connect with ECONNREFUSED / syscall "connect", with
no hostname on either, so callers could not tell a DNS failure from a
refused connection. Bun's own node:dns already reports ENOTFOUND for
the same lookup.
The getaddrinfo return code was discarded in uSockets:
us_internal_socket_after_resolve dropped result->error, so
us_connecting_socket_close fabricated ECONNABORTED, and the
cached-failure short-circuit in us_socket_group_connect stuffed the EAI
code into errno and returned NULL. The same bad hostname fetched twice
produced two different wrong errors (ConnectionRefused, then
FailedToOpenSocket).
Preserve the code in a new dns_error field on us_connecting_socket_t
(kept separate from `error`: the EAI_* and errno constant sets
overlap), route the cached-failure case through the same
connecting-socket path as the uncached one, and have the connect-error
handlers report a nonzero dns_error as the same
"getaddrinfo <CODE> <hostname>" SystemError that node:dns produces,
via the existing init_eai mapping.
|
Warning Review limit reached
More reviews will be available in 10 minutes and 36 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (16)
Comment |
|
Updated 5:25 AM PT - Jun 28th, 2026
❌ @robobun, your commit d7e7b51 has 1 failures in 🧪 To try this PR locally: bunx bun-pr 32990That installs a local version of the PR into your bun-32990 --bun |
|
Found 2 issues this PR may fix:
🤖 Generated with Claude Code |
|
Verified both suggestions against the issues and added I did not add |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…resolve With an HTTP proxy configured, the HTTP thread resolves the proxy hostname, not the origin's, so the ENOTFOUND error must name the proxy. Cover it in the fetch test with an explicit unresolvable proxy option, and include stderr in the subprocess assertion so a child crash surfaces the actual output instead of an opaque JSON parse error.
…cting it After a redirect, `AsyncHTTP.url` still holds the original request URL (only `client.url` is updated), so the resolver error named a host that resolved fine instead of the redirect target. Reading the post-redirect `client.url.hostname` from the JS thread is a use-after-free under ASAN: that URL slices into the buffer `handle_response_metadata` allocates, which the HTTP thread frees right after the result callback returns. Capture the hostname once, on the HTTP thread, at the failure: `connected_url.hostname` is the exact name `HTTPContext::connect` was told to resolve (the proxy's when one is configured, the post-redirect target otherwise), so one owned copy on `HTTPClientResult` covers every case and the JS side never dereferences the client's borrowed URL buffers. Cover it with a fifth case in the fetch test: a local 302 to an unresolvable host must name the redirect target.
Jarred-Sumner
left a comment
Jarred-Sumner
left a comment
There was a problem hiding this comment.
Why both error and dns_error instead of a 1 bit marker to say the error is a dns error?
The getaddrinfo return code and the connect errno are mutually exclusive, so store whichever one happened in the existing `error` field and tag its namespace with a bit in the existing bitfield (the two constant sets overlap numerically). This drops the second int and the fabricated ECONNABORTED that `error` carried on the DNS path. `us_connecting_socket_get_dns_error` derives its value from the pair, so nothing past that accessor changes.
|
Done in d7e7b51. The reason I originally used a second |
There was a problem hiding this comment.
All my earlier feedback has been addressed and the current revision looks correct to me, but this threads error state through uSockets C internals, the cross-thread HTTP client result path, and per-platform errno mapping (with an acknowledged Windows follow-up), so it warrants a human pass before merge.
Extended reasoning...
Overview
This PR plumbs the raw getaddrinfo(3) return code from uSockets up through Bun.connect and fetch() so DNS resolution failures surface as ENOTFOUND/getaddrinfo/hostname instead of being collapsed into ECONNREFUSED. It touches 16 files across three layers: the C uSockets library (context.c, socket.c, internal.h, libusockets.h — adds an error_is_dns tag bit, removes the cached-failure short-circuit, changes DNS-cache invalidation), the Rust FFI/HTTP/socket layer (uws_sys, HTTPContext, InternalState, lib.rs, socket_body.rs, FetchTasklet, cares_jsc), and tests. New owned dns_error/dns_hostname fields flow through HTTPClientResult from the HTTP thread to the JS thread.
Security risks
None identified. This is error-reporting plumbing on the failure path; no new parsing of untrusted input, no auth/crypto changes. The hostname copied into the error is already user-/server-supplied and was previously displayed in other error messages.
Level of scrutiny
High. Every fetch() and Bun.connect() traverses these code paths. The change spans a C struct bitfield layout, a behavioral change to DNS-cache invalidation in uSockets, cross-thread ownership of a hostname buffer (the author found and fixed a use-after-free under ASan during review when trying my first suggested fix), and per-platform errno-namespace handling. Two correctness bugs (proxy hostname, redirect hostname) were found and fixed during review, and a third (Windows WSA→UV_EAI namespace mismatch in init_eai) is deliberately deferred to a follow-up with the rationale documented in the PR description.
Other factors
All four of my prior inline comments are resolved and the fixes look right — in particular the final approach (capturing connected_url.hostname as an owned Box<[u8]> on the HTTP thread) is cleaner and safer than the borrowed-slice alternatives. Test coverage is good (uncached/cached, proxy, redirect, both connectError and promise-rejection paths). The bug-hunting pass on the latest revision found nothing. That said, the combination of C-side control-flow changes, the deferred Windows mapping gap, and the number of HTTPClientResult plumbing sites makes this worth a maintainer's eyes rather than a bot approval.
## Problem `docs/runtime/networking/dns.mdx` says: > If any connections to a host fail, we remove the entry from the cache. This has not worked since #13249. The eviction in `us_connecting_socket_close` is gated on `c->error == ECONNREFUSED`, but nothing assigns the connect outcome to `c->error` anymore. The field only ever receives the `ECONNABORTED` default (meaning "caller aborted"), so the gate is always false and a dead host keeps serving cache hits for its full TTL (30s by default). ```js const stat = () => { const s = Bun.dns.getCacheStats(); return `hits=${s.cacheHitsCompleted} size=${s.size} errors=${s.errors}`; }; await Bun.connect({ hostname: "localhost", port: 1, socket: { data() {}, open() {} } }).catch(() => {}); console.log(stat()); // hits=0 size=1 errors=0 await Bun.connect({ hostname: "localhost", port: 1, socket: { data() {}, open() {} } }).catch(() => {}); console.log(stat()); // hits=1 size=1 errors=0 <- dead host served from the cache, never evicted ``` ## Cause Before #13249, `us_internal_socket_after_open` and `us_internal_socket_after_resolve` set `c->error = ECONNREFUSED` themselves once every resolved address had been tried and failed, then evicted the entry. That PR consolidated the teardown into `us_connecting_socket_close` but dropped those assignments, leaving the consolidated eviction check unreachable. ## Fix Restore the two assignments. `us_connecting_socket_close` already distinguishes a real connect failure (`ECONNREFUSED`, evict) from a caller abort (the `ECONNABORTED` default, do not evict); the two paths that mean "every address failed" just need to say so again. No user-visible error code changes: the socket layer normalizes unrecognized connect errnos (including the previous `ECONNABORTED`) to `ECONNREFUSED` before it reaches JS, and the HTTP client ignores the code entirely. ## Intentionally not changed - A completed cache hit that resolves to a single address takes a shortcut in `us_socket_group_connect` (`entries->info.ai_next == NULL`) that returns a plain `us_socket_t` and releases the DNS request before the connect outcome is known, so a failure on that path still cannot evict. Wiring eviction into it means giving raw connect sockets a connecting-state backpointer and reconciling two different close and dispatch contracts, which is a larger change than this fix should carry. - Failed resolutions are still cached. The pre-#13249 code deliberately did not evict those, and the next synchronous lookup that consumes a cached failure already evicts it (`us_socket_group_connect`). #32990 handles that side (stop caching resolver failures and report them as `ENOTFOUND`); it does not overlap with this change. ## Verification `test/js/bun/dns/dns-prefetch.test.ts`: a subprocess fails three connects to a localhost port with no listener and asserts, after each one, that the entry was evicted (`size` back to 0, `errors` incremented) and that `cacheHitsCompleted` stays 0. Without the fix it fails with `size: 1, errors: 0, cacheHitsCompleted: 2`. The existing `dns.prefetch > should prefetch` test in that file resolved and fetched `example.com`, so it could not pass in an environment without internet access. It now runs against a local `Bun.serve` on `localhost` with the same assertions; `localhost` is still a real DNS lookup, so the prefetch and the fetch still share one cache entry.
2 participants
Fixes NXDOMAIN (and every other resolver failure) from
fetch()andBun.connect()being reported as a refused connection.Repro
"name does not resolve" and "host refused the connection" drive opposite reactions (fix the hostname vs. retry and back off), and HTTP client libraries and retry wrappers branch on exactly this distinction, so under Bun they all misdiagnose NXDOMAIN.
Cause
The
getaddrinfo(3)return code was discarded twice in uSockets:us_internal_socket_after_resolvedroppedresult->error, sous_connecting_socket_closefabricatedECONNABORTED(c->errorwas still 0). The Rust handlers then mapped that toConnectionRefused(fetch) /ECONNREFUSED(Bun.connect).us_socket_group_connectstuffed the EAI code intoerrnoand returnedNULL, which surfaced asFailedToOpenSocket. So the same bad hostname produced two different wrong errors on consecutive lookups.Fix
us_connecting_socket_t.errorand tag its namespace with a new 1-biterror_is_dnsfield in the existing bitfield (getaddrinfo return codes and errnos overlap numerically, so the tag is what disambiguates them). This also removes the fabricatedECONNABORTEDthaterrorcarried on the resolver-failure path.HTTPContext/NewSocketreaddns_errorfrom the connecting socket and, when it is nonzero, build the error with the sameinit_eai+getaddrinfo <CODE> <hostname>machinery Bun'snode:dnsalready uses (factored intosystem_error_with_syscall_and_hostname).hostnameis captured as an owned copy on the HTTP thread at the failure, fromconnected_url.hostname: the exact nameHTTPContext::connectwas told to resolve (the proxy's when one is configured, the post-redirect target after a redirect). The JS side never reconstructs it from the client's borrowed URL buffers, which the HTTP thread frees after the result callback returns.After:
and the second fetch to the same host now reports the same error as the first.
On Windows,
init_eaionly matchesUV_EAI_*constants in its#[cfg(windows)]arm (it already carries aTODO: revisit this), so the rawws2_32::getaddrinfocode falls through to theENOTFOUNDcatch-all. NXDOMAIN (WSAHOST_NOT_FOUND) lands on the right answer, and thesyscall/hostnameare correct, but a transientWSATRY_AGAINis also reported asENOTFOUND. The POSIX arm has the same class of imprecision (EAI_AGAINfalls through toENOTIMP, whichtest/js/bun/dns/resolve-dns.test.tsalready pins). Both halves ofinit_eai's non-NXDOMAIN mapping deserve one follow-up with platform test coverage rather than a partial, untested patch here; every case is still a strict improvement over the unconditionalECONNREFUSEDit replaces.Intentionally not covered here: the HTTP/3 (QUIC) connect path has its own DNS registration (
Bun__addrinfo_registerQuic) and still maps every connect failure toConnectionRefused.Tests
test/js/bun/net/socket-dns-error.test.ts(new):Bun.connectreportsENOTFOUND/getaddrinfo/hostnamethrough bothconnectErrorand the rejected promise, exactly once, and consistently across consecutive (cached) attempts. Uses a >63-byte DNS label (illegal per RFC 1035 section 2.3.4), which getaddrinfo rejects locally with no network, the same fixture as Node's owntest-net-dns-error.js.test/js/web/fetch/client-fetch.test.ts: the existing"invalid url"test asserted the buggy"Unable to connect. Is the computer able to access the url?"message from inside acatchblock that never fires when an HTTP proxy is configured. Rewritten to a subprocess with the proxy env cleared, asserting the full error shape across five cases: three consecutive direct fetches (the uncached and the cached path), one through an explicit unresolvableproxy:option (must name the proxy, not the origin), and one to a local server that 302s to an unresolvable host (must name the redirect target, not the original origin).Every other consumer of
HTTPClientResult.fail(bun install, S3, the sync CLI requests) formats the error name generically, so the newDNSResolveFailedvariant falls through to, for example,error: DNSResolveFailed downloading package manifest ...instead ofConnectionRefused.Fixes #11345
This also addresses Case 1 (NXDOMAIN reported as
ConnectionRefusedinstead ofENOTFOUND) of #20486. The other cases in that issue (wrapping the rejection inTypeError: fetch failedwith acause,ERR_INVALID_URL, and the invalid-protocol code) are about the overall fetch error shape and are not covered here.