dns: re-read system nameservers when network config changes - #34781
dns: re-read system nameservers when network config changes#34781robobun wants to merge 16 commits into
Conversation
The c-ares channel backing dns.resolve*/dns.reverse/Bun.dns reads system DNS config once at creation and never again. After a VPN connect, Wi-Fi switch, or DHCP renew the resolver keeps querying the boot-time servers for the life of the process. Two layers: - OS config watcher (process-wide, lazy, best-effort): inotify on /etc (Linux), notify(3) on SystemConfiguration's dns key (macOS), NotifyIpInterfaceChange (Windows). On fire it bumps a generation counter and drops the connect-path getaddrinfo cache. Each Resolver checks the generation in get_channel() and recreates its channel when stale, unless the user pinned servers with setServers(). Mirrors c-ares' own ares_event_configchg.c, which we can't use because it's tied to c-ares' private event thread. - Node-parity ensure_servers() fallback (nodejs/node#13076, #61453) for platforms where the watcher can't register: if the previous query hit ECONNREFUSED, servers are still default, and the only server is loopback (c-ares' empty-resolv.conf fallback), recreate the channel. ares_reinit() would be the natural primitive but with CARES_THREADS it spawns a background thread that calls sock_state_cb off the JS thread, which our poll bookkeeping can't handle, so we destroy + lazy-recreate instead (pending queries see ECANCELLED, same as Resolver.cancel()). New is_servers_default / query_last_ok / config_generation state on Resolver, a GlobalCache::invalidate_all(), a new PollTag::DnsConfig, and bun:internal-for-testing hooks for the generation counter so tests can exercise the mechanism without touching /etc.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughChangesThe DNS runtime now watches operating-system configuration changes, invalidates shared DNS cache state, and reinitializes eligible resolver channels when generations change. Test-only bindings and coverage were added for forced changes, pinned servers, resolver recreation, and Linux file watching. DNS configuration changes
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 4:57 AM PT - Jul 20th, 2026
✅ @dylan-conway, your commit f950419c4023a7c03d64c8cfd7642798c32accba passed in 🧪 To try this PR locally: bunx bun-pr 34781That installs a local version of the PR into your bun-34781 --bun |
|
Found 5 issues this PR may fix:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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 `@src/runtime/dns_jsc/dns.rs`:
- Around line 4818-4831: Update ensure_servers so detecting a non-loopback
channel does not mutate is_servers_default; return without changing the
user-pinned server state, or track fallback recovery with a separate state flag.
Preserve watcher-driven reinitialization for later VPN, DHCP, or configuration
changes when setServers() was never used.
🪄 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: c90c66e9-8dcf-46ac-888b-cc061e4fa010
📒 Files selected for processing (9)
src/io/posix_event_loop.rssrc/js/internal-for-testing.tssrc/jsc/bindings/InternalForTesting.cppsrc/jsc/bindings/InternalForTesting.hsrc/runtime/dispatch.rssrc/runtime/dns_jsc/config_watcher.rssrc/runtime/dns_jsc/dns.rssrc/runtime/dns_jsc/mod.rstest/js/bun/dns/dns-config-change.test.ts
…n ECONNREFUSED Store the POSIX watcher's FilePoll in a per-VM RareData slot rather than gating on a process-global flag, so a Worker that creates the first c-ares channel doesn't permanently mask the main thread's watcher when it terminates. NotifyIpInterfaceChange stays process-global on Windows since the callback itself is process-scoped. Drop the is_servers_default.set(false) in ensure_servers() when the channel's servers aren't loopback: in Node it's a harmless memoization, but here the same flag gates check_config_change(), so one transient ECONNREFUSED against real system servers would disable the config watcher for the life of the resolver.
…n_sys::linux inotify
setLocalAddress() writes directly onto the c-ares channel via
ares_set_local_ip4/ip6 and was stored nowhere on the Resolver, so a
config-change recreate silently dropped the binding. Stash the last
IPv4/IPv6 values on Resolver and replay them after Channel::init.
Also switch the Linux inotify calls to bun_sys::linux::{inotify_init1,
inotify_add_watch, IN::*} and the macOS O_NONBLOCK set to
bun_sys::set_nonblocking, matching INotifyWatcher.rs and path_watcher.rs
rather than hand-rolling the FFI.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
test/js/bun/dns/dns-config-change.test.ts (1)
137-143: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winReplace the fixed 50 ms sleep with an event-loop yield.
The deadline is useful, but
setTimeout(..., 50)introduces an arbitrary wall-clock delay and violates test conventions. Yield one macrotask/event-loop turn instead, such asawait Bun.sleep(0), while retaining the deadline.As per coding guidelines, tests must avoid fixed
setTimeoutwaits. Based on learnings,Bun.sleep(0)is an acceptable macrotask barrier.🤖 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/js/bun/dns/dns-config-change.test.ts` around lines 137 - 143, Replace the fixed 50 ms setTimeout wait in the dnsConfigGeneration polling loop with an event-loop yield such as Bun.sleep(0). Preserve the existing deadline check and error behavior while allowing the loop to retry on the next macrotask.Sources: Coding guidelines, Learnings
src/runtime/dns_jsc/config_watcher.rs (1)
85-112: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winWatching only
/etcmisses some systemd-resolved updates.
When/etc/resolv.confpoints at/run/systemd/resolve/resolv.conf, the changes happen outside/etc, so this watch never fires for upstream-server changes. Add a fallback watch for the resolved target or document the limitation.🤖 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/config_watcher.rs` around lines 85 - 112, Update open_watch_fd to also watch the systemd-resolved target directory/file when /etc/resolv.conf points to /run/systemd/resolve/resolv.conf, while retaining the existing configurable BUN_DNS_CONFIG_WATCH_DIR behavior. Ensure changes to the resolved target trigger the same watcher and close the inotify descriptor if any required watch setup fails.
🤖 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/bun/dns/dns-config-change.test.ts`:
- Around line 51-54: Condense the comment above the
setLocalAddress/config-change test to three or fewer `//` lines, preserving both
the c-ares channel replay invariant and the reason the test verifies behavior
indirectly because no public getter exists.
- Around line 56-57: Replace the require calls in the spawned child sources,
including the scripts around dnsConfigChanged and dnsConfigGeneration, with
module-scope import statements. Keep the imported modules and bindings
unchanged; only use dynamic loading if that specific behavior is under test.
---
Outside diff comments:
In `@src/runtime/dns_jsc/config_watcher.rs`:
- Around line 85-112: Update open_watch_fd to also watch the systemd-resolved
target directory/file when /etc/resolv.conf points to
/run/systemd/resolve/resolv.conf, while retaining the existing configurable
BUN_DNS_CONFIG_WATCH_DIR behavior. Ensure changes to the resolved target trigger
the same watcher and close the inotify descriptor if any required watch setup
fails.
In `@test/js/bun/dns/dns-config-change.test.ts`:
- Around line 137-143: Replace the fixed 50 ms setTimeout wait in the
dnsConfigGeneration polling loop with an event-loop yield such as Bun.sleep(0).
Preserve the existing deadline check and error behavior while allowing the loop
to retry on the next macrotask.
🪄 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: af87be0f-b1fc-45b0-b463-d34c7e763c3c
📒 Files selected for processing (4)
src/jsc/rare_data.rssrc/runtime/dns_jsc/config_watcher.rssrc/runtime/dns_jsc/dns.rstest/js/bun/dns/dns-config-change.test.ts
…t, test nits
Add config_watcher::uninstall() that unregisters the FilePoll and closes
the inotify fd (Linux) / calls notify_cancel (macOS), and call it from
close_dns_for_terminate() so a terminating Worker doesn't leak an inotify
instance per lifetime. The watcher is now a boxed struct in the RareData
slot so macOS can stash the notify token alongside the poll.
Snapshot the config-generation before Channel::init rather than after so
a concurrent NotifyIpInterfaceChange bump on Windows can't be absorbed
between init's config read and the snapshot store.
Tests: fold the three subprocess assertions into one {stdout, exitCode}
combined check (stderr is still surfaced in the failure message but not
asserted empty), replace the 50ms poll sleep with Bun.sleep(0) + deadline,
and trim the setLocalAddress comment to three lines.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/runtime/dns_jsc/config_watcher.rs (1)
95-179: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider a
Dropguard forWatcherinstead of manualclose_watch_fdpairing.
Watcherowns a native fd (and, on macOS, anotify(3)token) but has noDropimpl; both cleanup call sites (install()'s register-failure path anduninstall()) must remember to callclose_watch_fdmanually. A future refactor that constructs/returns aWatcherthrough a new path could easily leak the fd/token.As per coding guidelines, "Pair every native resource acquisition with a release at the acquisition site; arm RAII or Drop guards before fallible calls."
🤖 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/config_watcher.rs` around lines 95 - 179, Add a Drop implementation for Watcher that calls close_watch_fd using its owned poll fd and macOS token, then remove the manual close_watch_fd calls from install() failure handling and uninstall(). Ensure Watcher is fully initialized before any fallible operation so its Drop cleanup covers every acquired native resource.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@src/runtime/dns_jsc/config_watcher.rs`:
- Around line 95-179: Add a Drop implementation for Watcher that calls
close_watch_fd using its owned poll fd and macOS token, then remove the manual
close_watch_fd calls from install() failure handling and uninstall(). Ensure
Watcher is fully initialized before any fallible operation so its Drop cleanup
covers every acquired native resource.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: dc2b2c62-aed3-4817-93e0-bdeabd991810
📒 Files selected for processing (4)
src/runtime/dns_jsc/config_watcher.rssrc/runtime/dns_jsc/dns.rssrc/runtime/jsc_hooks.rstest/js/bun/dns/dns-config-change.test.ts
…etLocalAddress test check_timeouts() now reads self.channel.get() instead of routing through get_channel_or_error(): get_channel() can reset + re-init the channel since this PR, and on init failure get_channel_or_error() throws a JS exception that the timer dispatch path has no way to propagate. The timer's only job is to time out queries on the existing channel; if it was reset, those queries were already ECANCELLED. Drop the setLocalAddress-survives-recreate test: ares_set_local_ip4 is a void C setter and there is no public getter, so the test cannot observe whether replay_local_address() ran. A test that passes with the fix reverted advertises coverage that doesn't exist; the underlying replay code remains in place.
…LocalAddress get_channel() can now destroy the existing channel (check_config_change / ensure_servers call reset_channel), so a raw *mut Channel captured before a JS coercion that re-enters get_channel() is a UAF. Refactor both set_local_address() and set_channel_servers() to do all JS coercions (to_slice, get_index, coerce_to_i32, to_bun_string) before calling get_channel_or_error(), per the 'coerce first while holding no raw pointers' rule. set_channel_servers now takes &self + is_global instead of a raw channel pointer, eliminating the redundant get_channel_from_vm() re-entry that could also invalidate the held pointer on Windows. Tests switched to test.concurrent since each spawns an independent child process.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/runtime/dns_jsc/dns.rs (1)
4090-4091: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRepresent unset local addresses explicitly.
0.0.0.0and::are valid bindings, but these zero sentinels makereplay_local_address()skip them. UseCell<Option<u32>>andCell<Option<[u8; 16]>>, then replaySome(...)values; add regressions for both unspecified addresses across a channel reset.🤖 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/dns.rs` around lines 4090 - 4091, Update the state containing local_ip4 and local_ip6 to use Cell<Option<u32>> and Cell<Option<[u8; 16]>>, representing unset addresses as None rather than zero values. Adjust all initialization, assignment, and replay_local_address() logic to replay only Some(...) values while preserving valid 0.0.0.0 and :: bindings, and add regression coverage for both unspecified addresses across a channel reset.Source: Coding guidelines
🤖 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/runtime/dns_jsc/dns.rs`:
- Around line 5837-5840: Condense the safety comment at
src/runtime/dns_jsc/dns.rs lines 5837-5840 to no more than three lines while
preserving the channel-reset, coercion re-entry, and UAF rationale; apply the
same three-line limit and equivalent rationale to the server-coercion comment at
lines 5927-5930.
- Around line 5841-5855: Update setLocalAddress around the stash_local_address
calls to parse and validate both address arguments without mutating resolver
state, including duplicate-family rejection, before committing either value.
Only after the complete argument set succeeds should both local-address cells be
updated and the existing replay/reset behavior proceed; failed calls must leave
the prior binding unchanged.
In `@test/js/bun/dns/dns-config-change.test.ts`:
- Line 20: Update the test “dns resolver re-initializes after a config-change
signal” so it observes channel initialization or recreation rather than only
comparing system server lists. Add a test-only channel-init signal or controlled
DNS configuration that changes after dnsConfigChanged(), ensuring the test fails
when get_channel() ignores the generation or check_config_change() is removed.
---
Outside diff comments:
In `@src/runtime/dns_jsc/dns.rs`:
- Around line 4090-4091: Update the state containing local_ip4 and local_ip6 to
use Cell<Option<u32>> and Cell<Option<[u8; 16]>>, representing unset addresses
as None rather than zero values. Adjust all initialization, assignment, and
replay_local_address() logic to replay only Some(...) values while preserving
valid 0.0.0.0 and :: bindings, and add regression coverage for both unspecified
addresses across a channel reset.
🪄 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: 34095eb2-6410-4380-b971-d668dfca4cb0
📒 Files selected for processing (2)
src/runtime/dns_jsc/dns.rstest/js/bun/dns/dns-config-change.test.ts
…ments Switch local_ip4/ip6 to Cell<Option<..>> so 0.0.0.0 / :: (valid 'any' bindings) aren't conflated with unset. set_local_address now parses and validates all arguments into locals before committing either cell, so a throwing call leaves the prior stash unchanged. Trim the two coercion comments to three lines.
There was a problem hiding this comment.
No new issues found in this revision — the coerce-before-fetch reordering in set_local_address/set_channel_servers, the parse-then-commit split for setLocalAddress, and the direct self.channel.get() in check_timeouts all look correct. Deferring to a human because this is a new ~350-line cross-platform native subsystem (inotify / notify(3) / NotifyIpInterfaceChange FFI, FilePoll lifecycle, off-thread callback on Windows touching GlobalCache) that changes get_channel() from lazy-init-only to potentially destructive.
What was reviewed:
reset_channel()ordering vs.sock_state_cbre-entry and pending-queryECANCELLEDdelivery- Windows
on_changeruns off-thread →invalidate_global_cache(): confirmed it only takes theGuardedlock and heap-frees refcount-0Requests (no JS-heap access) - inotify event parsing: kernel guarantees no partial events per
read(), soname_off + name_lenstays in-bounds setServers([])now setsis_servers_default = falseand passes the active-queries check — matches the pre-PR ordering
Extended reasoning...
Overview
Adds a per-OS DNS-config-change watcher (src/runtime/dns_jsc/config_watcher.rs, ~350 lines new) that bumps a process-global generation counter when /etc/resolv.conf (Linux inotify), the SystemConfiguration DNS key (macOS notify(3)), or IP interface state (Windows NotifyIpInterfaceChange) changes. Resolver::get_channel() now checks the generation and, if stale and servers are still system-default, destroys and lazily recreates the c-ares channel. Also adds a Node-parity ensure_servers() loopback-fallback recovery, replays setLocalAddress() bindings across recreates, and invalidates the connect-path getaddrinfo cache on config change. Touches 11 files: the new watcher module, dns.rs (channel lifecycle + setServers/setLocalAddress reordering), rare_data.rs / posix_event_loop.rs / dispatch.rs (FilePoll plumbing), jsc_hooks.rs (uninstall on VM teardown), and test-only hooks in InternalForTesting.{cpp,h} / internal-for-testing.ts.
Security risks
Low. The only externally-influenced parsing is the Linux inotify event stream (kernel-originated, header-length-driven slice into a stack buffer — kernel guarantees whole events per read so no partial-record OOB). No new user-controlled input paths; BUN_DNS_CONFIG_WATCH_DIR is a test-only env override that just changes the inotify watch directory.
Level of scrutiny
High. This is new native code with per-platform FFI (notify_register_file_descriptor, NotifyIpInterfaceChange), a new FilePoll owner tag with install/uninstall lifecycle tied to VM teardown, and a semantic change to get_channel() that makes it destructive. The Windows backend fires bump_generation() on a system threadpool thread, which then locks GlobalCache and heap-frees entries — I verified Request::deinit only drops owned heap boxes (no JS-heap or FilePoll access), but cross-thread mimalloc frees and the interaction with any other GlobalCache reader deserve a maintainer's eyes. Five prior review rounds on this PR already caught a UAF (raw *mut Channel held across re-entrant get_channel()), a leaked pending exception from check_timeouts, a partial-commit-on-throw in setLocalAddress, and two vacuous tests — the fix pattern is sound now, but the density of subtle issues found so far argues for human sign-off.
Other factors
All prior inline findings are resolved. The remaining CodeRabbit note (test doesn't directly observe channel recreation) was acknowledged as a follow-up. Tests are subprocess-isolated and test.concurrent; the Linux inotify test uses a temp-dir override so it doesn't touch /etc. rust:check-all reportedly passes on all 10 targets but CI hasn't run on the final commit yet.
|
CI build #76174 failures are all
The new |
There was a problem hiding this comment.
No new findings on the latest revision — the parse-then-commit refactor for setLocalAddress/setServers and the Option<> local-address stash look correct. Deferring to a human for sign-off given the scope: new per-platform OS watcher subsystem (inotify / notify(3) / NotifyIpInterfaceChange) plus c-ares channel destroy/recreate lifecycle changes.
What was reviewed:
get_channel()re-entrancy after it became destructive —set_local_address/set_channel_serversnow coerce all JS args before fetching the channel, andcheck_timeoutsreadsself.channel.get()directly.- Windows off-thread
bump_generation()→invalidate_global_cache()—GlobalCacheisGuarded+Send,Request::deinitis a plain Box drop with no thread-affine data. - Per-VM watcher lifecycle —
uninstall()wired intoclose_dns_for_terminate(); FilePoll/fd/notify-token released together. - Earlier rounds:
setServers([])short-circuit, ECONNREFUSED loopback fallback,is_servers_defaultgating, vacuous test dropped,test.concurrent.
Extended reasoning...
Overview
This PR adds a process-wide DNS-config-change watcher so the c-ares channel backing dns.resolve* re-reads system nameservers after VPN/Wi-Fi/DHCP changes. New file config_watcher.rs (~356 lines) implements per-OS backends (Linux inotify on /etc, macOS notify(3) on the SystemConfiguration DNS key, Windows NotifyIpInterfaceChange), bumps an AtomicU64 generation, and invalidates the connect-path getaddrinfo cache. dns.rs gains check_config_change()/ensure_servers()/reset_channel()/replay_local_address(), five new Resolver fields, note_query_result() at four callback sites, and a substantial refactor of set_local_address()/set_channel_servers() to parse-then-commit. Plumbing: new PollTag::DnsConfig, RareData slot, dispatch arm, jsc_hooks uninstall, and two bun:internal-for-testing hooks.
Security risks
None identified. The BUN_DNS_CONFIG_WATCH_DIR env override (Linux only) redirects the inotify watch to a test directory but is bounded to a 512-byte stack buffer and only affects which directory is watched — it doesn't influence what nameservers c-ares reads. No user-controlled data reaches the FFI paths.
Level of scrutiny
High. This is native code with per-platform raw FFI, a new FilePoll owner tag, and a change that makes get_channel() destructive (it can now ares_destroy an existing channel). The PR has been through five prior review rounds that surfaced and fixed real issues: a UAF from re-entrant get_channel() while holding a raw *mut Channel across JS coercions, a pending-exception leak from check_timeouts, partial-state commit on setLocalAddress error, per-VM vs process-global watcher lifecycle for Workers, and a vacuous test. All are resolved in the current revision. The Windows callback runs on a system threadpool thread; I verified invalidate_global_cache() only touches the Guarded<GlobalCache> mutex and Request::deinit is a plain Box drop, matching the existing unsafe impl Send contract on that type.
Other factors
The PR description notes tests are platform-gated and defer to CI ("no test proof · iteration 0"). No human reviewer has weighed in yet. Given the cross-platform surface, the number of memory-safety issues iterated through to reach this state, and the design decision to destroy+recreate rather than ares_reinit() (well-justified in the description but non-obvious), a maintainer should sign off on the overall approach and confirm CI is green across all three OS backends before merge.
There was a problem hiding this comment.
No new issues found in this pass, but this adds ~350 lines of platform-specific FFI (inotify, notify(3), NotifyIpInterfaceChange) and changes get_channel() from lazy-init-only to potentially-destructive — worth a maintainer's eyes on the three OS backends and the Windows off-thread callback path.
What was reviewed:
invalidate_global_cache()from the Windows threadpool callback —GlobalCacheisGuarded<>-locked andRequesthas no JS-thread-affine fields on Windows, soRequest::deinitoff-thread looks sound.- inotify event parsing —
name_lenis kernel-supplied andbufis zero-initialized; a short read reads zeros, not OOB. - The
set_channel_serversreorder (arg coercion now precedes the active-queries check and channel fetch) — no user-JS runs afterget_channel_or_error(), and the empty-array path folds intohead = nullwith the same c-ares semantics. - Earlier findings (leaked exception in
check_timeouts, re-entrantget_channelUAF inset_local_address/set_channel_servers, partial-commit insetLocalAddress) are all confirmed addressed in the current diff.
Extended reasoning...
Overview
Adds a per-process DNS config-change watcher (src/runtime/dns_jsc/config_watcher.rs, ~350 lines new) with three OS backends: Linux/Android inotify on /etc, macOS notify_register_file_descriptor on the SystemConfiguration DNS key, and Windows NotifyIpInterfaceChange. Bumps a global AtomicU64 generation on fire; each Resolver::get_channel() compares its snapshot and, if stale and servers weren't user-pinned, destroys and recreates the c-ares channel. Also adds a Node-parity ensure_servers() loopback-recovery fallback, invalidates the connect-path GlobalCache on config change, and refactors set_local_address() / set_channel_servers() to do all JS coercions before touching the (now-destructible) channel pointer. Plumbing: new PollTag::DnsConfig + dispatch arm, per-VM RareData slot, bun:internal-for-testing hooks, and a 3-test file.
Security risks
Low. The inotify parser reads kernel-supplied inotify_event records into a fixed 4 KB stack buffer; name_len is used to slice within that zero-initialized buffer, so a malformed record would panic (bounds-checked) or read zeros, not OOB. The BUN_DNS_CONFIG_WATCH_DIR override is copied into a 512-byte stack buffer with an explicit length guard. No user-controlled data reaches the FFI declarations. The Windows callback only touches an atomic and a mutex-guarded cache.
Level of scrutiny
High — this warrants a human maintainer. It introduces raw FFI across three platforms (two of which can't be exercised on a Linux CI runner beyond compile), changes get_channel() from monotonic to destructive (which already surfaced one UAF and one leaked-exception bug during earlier review rounds), runs a callback on a Windows system threadpool thread that frees heap allocations, and hand-parses kernel inotify records. The design decisions (why not ares_reinit(), why per-VM watcher on POSIX vs process-global on Windows, why cancel+destroy instead of reinit) are well-argued in the PR description but are exactly the kind of architectural calls a maintainer should sign off on.
Other factors
The PR has already been through three automated review rounds that found and fixed real bugs (leaked JS exception from check_timeouts, re-entrant-get_channel UAF in setLocalAddress/setServers, partial state commit on setLocalAddress failure, a vacuous test). All prior inline comments are resolved. CI is green on DNS-related tests across all lanes. Test coverage is reasonable for the generation-counter and Linux inotify paths but the macOS/Windows backends are compile-checked only (author noted this). The set_channel_servers refactor moves the pending-queries check to after argument coercion — a minor observable ordering change (a throwing toString on an array element now fires before ERR_DNS_SET_SERVERS_FAILED), which seems acceptable but is a behavior delta a maintainer should be aware of.
2 participants
What
The c-ares channel backing
dns.resolve*/dns.reverse/Bun.dnsreads the system nameserver list once when it's created (src/runtime/dns_jsc/dns.rsget_channel()) and never again. After a VPN connect, Wi-Fi switch, or DHCP renew the resolver keeps querying the boot-time servers for the life of the process, so lookups time out or return stale even though the OS resolver has moved on.dns.lookupandfetchgo through the OS resolver and are unaffected, but their connect-path cache (internal::GlobalCache, default 30 s TTL) also serves stale addresses for the rest of the window.How
Two layers, both landing here:
OS config-change watcher (new
src/runtime/dns_jsc/config_watcher.rs): one process-wide watch, armed lazily on the first c-ares channel creation, does not keep the event loop alive.inotifyon/etcfilteringresolv.conf/nsswitch.confnotify_register_file_descriptoron SystemConfiguration's DNS-config keyNotifyIpInterfaceChange(callback only touches atomics / theGuardedcache, so no JS-thread marshaling)On fire it bumps a process-global
AtomicU64generation and drops the connect-pathgetaddrinfocache. EachResolver::get_channel()compares its snapshot against the current generation and, if stale and the user hasn't calledsetServers(), cancels+destroys the channel so the next query re-reads fresh system config. This mirrors c-ares' ownares_event_configchg.c, which we can't call directly because it's bound to c-ares' private event thread.Node-parity
ensure_servers()fallback for platforms/containers where the watcher can't register (readonly/etc, seccomp, Android, other UNIX): reproducesChannelWrap::EnsureServersfrom nodejs/node#13076 / nodejs/node#61453. If the previous query hitECONNREFUSED, servers are still the system default, and the channel's only server is loopback (c-ares' empty-resolv.conf fallback), recreate the channel.Why not
ares_reinit()ares_reinit()is the natural primitive (it re-reads system config, preserves user-set servers, and requeues in-flight queries), but withCARES_THREADSenabled (we build with it) it spawns a background thread that calls oursock_state_cboff the JS thread when servers change (traced:ares_sysconfig_apply→ares_servers_remove_stale→ares_close_connection→ cb). Ouron_dns_socket_statetouchesJsCell<PollsMap>and FilePoll/uv_poll, which aren't thread-safe. So we destroy + lazy-recreate on the JS thread instead; pending queries seeECANCELLED(same asResolver.cancel()), which is strictly better than the 5-20 s timeout they'd hit against the now-unreachable server.Why not just match Node
Node's
EnsureServersonly covers "started offline → loopback fallback". The VPN/Wi-Fi-switch case is nodejs/node#49485, closed wontfix ("too complex, too platform-specific"). The per-OS watchers here are straight ports of what c-ares already ships internally.Testing
test/js/bun/dns/dns-config-change.test.ts:getServers()round-trips the same system listsetServers()survives a generation bumpBUN_DNS_CONFIG_WATCH_DIRand touchingresolv.confthere bumps the generation through the real inotify pathbun:internal-for-testinggainsdnsConfigGeneration()/dnsConfigChanged()so the tests don't need to touch/etc.rust:check-allpasses on all 10 targets.no test proof · iteration 2 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/bun/dns/dns-config-change.test.ts