dns: do not resolve with an empty value when worker.terminate() races a lookup completion

[robobun]

Reproduction

const { Worker } = require("node:worker_threads");
const src = [
  'const { parentPort } = require("node:worker_threads");',
  'const dns = require("node:dns"); const { promisify } = require("node:util");',
  'const lookup = promisify(dns.lookup);',
  'const lanes = (n, f) => { for (let i = 0; i < n; i++) (async () => { for (;;) { try { await f(i); } catch {} } })(); };',
  'lanes(20, () => lookup("localhost"));',
  'lanes(10, (i) => lookup("nx-" + i + "-" + Math.random().toString(36).slice(2) + ".invalid"));',
  'parentPort.postMessage("up");',
].join("\n");
for (let r = 0; r < 40; r++) {
  const w = new Worker(src, { eval: true });
  await new Promise((res) => w.once("message", res));
  await Bun.sleep((r % 3) * 5);
  await w.terminate();
}

Release build: panic: Segmentation fault at address 0x5 (3/3).
Debug build: ASSERTION FAILED: !target.isEmpty() in JSC__JSPromise__resolve (bindings.cpp:3564).

Cause

worker.terminate() from the parent sets the JSC termination flag on the worker's VM cross-thread. If the worker thread is mid-tick() dispatching a GetAddrInfoRequestTask when that lands, the native result-to-JSArray conversion (result_any_to_js / addr_info_to_js_array / to_js_response) throws, and every caller falls back to JSValue::ZERO (the existing // TODO: properly propagate exception upwards sites). The DNS on_complete sinks then passed that empty value straight into promise.resolve_task(global, result), which hits ASSERT(!target.isEmpty()) in JSC__JSPromise__resolve (near-null deref in release).

The dns.resolve* / c-ares path is already safe here because worker shutdown destroys the c-ares channel before the drain; the getaddrinfo work-pool path had no equivalent.

Fix

• Guard the four DNS on_complete sinks (DNSLookup::on_complete_with_array, CAresLookup::<T>::on_complete, CAresReverse::on_complete, CAresNameInfo::on_complete) so an empty result drops the promise Strong and cleans up instead of calling resolve_task.
• Replace the one .unwrap() in drain_pending_host_native's re-conversion loop with the same .unwrap_or(JSValue::ZERO) pattern so a mid-drain throw there feeds the guard above instead of panicking.
• Add a GetAddrInfoRequestTask arm to __bun_release_task_at_shutdown (and a GetAddrInfoRequest::release_for_shutdown helper) so queued-but-never-dispatched completions free their JSPromiseStrong handles and request boxes while the JSC VM is still live, instead of being re-queued past worker VM dealloc.

The separate cross-thread race where the work-pool thread posts into a freed EventLoop after the worker box is dealloc'd is the general class #34154 addresses and is not in scope here.

Test

test/js/web/workers/worker-terminate-lifetime.test.ts gains a regression test that queues 128 distinct IP-literal lookups with backend: "getaddrinfo" (so each is its own GetAddrInfoRequestTask on the work pool), parks the worker in Atomics.wait until they pile up, then wakes and terminates it mid-drain. Fails 8/8 before (ASSERTION FAILED: !target.isEmpty() / Segmentation fault at address 0x5), passes 8/8 after.

---

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/web/workers/worker-terminate-lifetime.test.ts

[robobun]

^{Updated 1:57 PM PT - Jul 22nd, 2026}

❌ @robobun, your commit ac2b7bc has 2 failures in Build #78011 (All Failures):

• test/js/node/test/parallel/test-gc-http-client-connaborted.js - timeout on 🐧 25.04 x64
• test/js/node/test/parallel/test-gc-http-client-connaborted.js - timeout on 🐧 13 x64
• test/js/node/test/parallel/test-net-connect-memleak.js - code 1 on 🐧 25.04 x64
• test/js/node/test/parallel/test-net-connect-memleak.js - code 1 on 🐧 13 x64

---

🧪   To try this PR locally:

bunx bun-pr 35161

That installs a local version of the PR into your bun-35161 executable, so you can run:

bun-35161 --bun

[github-actions]

Found 2 issues this PR may fix:

1. [CRASH] Worker termination races in-flight fetch(), corrupting the event loop's concurrent task queue (two crash signatures) #33911 - Partial fix: the DNS completion guards and drain_pending_host_native unwrap change address the DNS-specific sub-path of the broader worker-terminate-races-in-flight-fetch crash
2. ASAN CI: ExceptionScope::assertNoException during worker terminate (worker-transfer-terminate-stress, separate from #34095) #34690 - Plausible partial fix: skipping resolve_task when the result is empty eliminates one source of pending exceptions that trigger assertNoException during worker terminate

If this is helpful, copy the block below into the PR description to auto-close these issues on merge.

Fixes #33911
Fixes #34690

🤖 Generated with Claude Code

[robobun]

Neither issue is in scope here: #33911 is the cross-thread fetch/HTTP-thread race (this PR only touches the same-thread DNS completion path; #34154 is the tracker for the cross-thread class), and #34690's assertNoException in worker-transfer-terminate-stress is a different stack with no DNS involvement. Leaving both open.

[github-actions]

This PR may be a duplicate of:

1. Fix use-after-free when worker.terminate() races in-flight fetch/work-pool completions #34154 - Fixes the same worker.terminate() race with DNS lookup completions: adds identical if result.is_empty() guards to the same four DNS on_complete sinks in dns.rs, the same GetAddrInfoRequestTask shutdown arm in dispatch.rs, and the same regression test

🤖 Generated with Claude Code

[robobun]

Overlap with #34154 is real and intentional: that PR carries the full ShutdownGate model across 47 files for the whole worker-terminate class (including the cross-thread race). This PR carves out just the same-thread DNS on_complete crash (Segmentation fault at address 0x5 / ASSERTION FAILED: !target.isEmpty()) so it can land independently while #34154 is reviewed. Whichever merges first, the other rebases over it (the four is_empty() guards and the GetAddrInfoRequestTask release arm are the only shared hunks).

[coderabbitai]

Walkthrough

Changes

The DNS runtime skips empty promise resolutions, reclaims queued native getaddrinfo requests during shutdown, adjusts pending-host conversion fallback behavior, and adds a worker-termination regression test with platform-aware stress tuning.

DNS shutdown lifecycle

Layer / File(s)	Summary
DNS completion result handling src/runtime/dns_jsc/dns.rs	Completion handlers conditionally resolve promises only for non-empty JS results, while native pending-host draining falls back to JSValue::ZERO.
Queued getaddrinfo reclamation src/runtime/dns_jsc/dns.rs, src/runtime/dispatch.rs	Shutdown cleanup releases pending native cache entries, destroys the DNSLookup chain, frees the request, and destroys the queued task payload.
Worker termination regression coverage test/js/web/workers/worker-terminate-lifetime.test.ts	Adds platform-aware stress settings and verifies worker termination during concurrent getaddrinfo lookups on non-Windows systems.

Possibly related PRs

• oven-sh/bun#34455: Both PRs address worker/VM shutdown ordering for in-flight c-ares/DNS lookups.

Suggested reviewers: jarred-sumner

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
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.
Title check	✅ Passed	The title clearly summarizes the main DNS worker-termination race fix and is concise and specific.
Description check	✅ Passed	The description covers the issue, root cause, fix, and verification, though it does not use the template headings verbatim.

---

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

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 `@src/runtime/dns_jsc/dns.rs`:
- Around line 1569-1599: Update GetAddrInfoRequest::release_for_shutdown to call
resolver.request_completed() after removing any pending cache entry and before
freeing the request chain. Preserve the existing conditional cache cleanup, and
do not alter the separate global-resolver close_channel_for_terminate() path.

In `@test/js/web/workers/worker-terminate-lifetime.test.ts`:
- Line 217: Update the grace-period sleep in the worker termination test to
scale with the existing slow condition (isDebug || isASAN), matching the scaling
approach used for rounds, perRound, and timeout. Preserve the normal-build delay
while increasing it for slow/ASAN builds so getaddrinfo completions can
accumulate before terminate().

🪄 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: 96fe4d98-d2fb-47dd-a3f1-55d531f74007

📥 Commits

Reviewing files that changed from the base of the PR and between 47597ab and 5517531.

📒 Files selected for processing (3)

• src/runtime/dispatch.rs
• src/runtime/dns_jsc/dns.rs
• test/js/web/workers/worker-terminate-lifetime.test.ts

[robobun]

CI on build #78011 is green for this change: the new worker-terminate-lifetime.test.ts regression passes on every lane, and the flaky suites from the previous run cleared on retry. The remaining red is two FinalizationRegistry/GC-timing Node parallel tests (test-net-connect-memleak.js, test-gc-http-client-connaborted.js) on ubuntu-25.04 x64 and debian-13 x64; this diff does not touch net, http, GC, or FinalizationRegistry, and both have been reported as main breaks. The other lanes showing red are marked-flaky tests (fetch.stream, in-process-cron, require-cache, bun-serve-routes) that cleared on at least one retry. Ready for review.

[claude]

No bugs found, but this touches unsafe worker-VM shutdown ordering and JSC handle lifetime — worth a human pass, especially given the overlap with #34154's broader ShutdownGate model.

Checked that release_for_shutdown mirrors the normal then() reclaim (pending-native-cache slot pop, head.next chain destroy, box drop) and only sees Backend::Libc on this task path, so no file_poll to release.
Checked the four is_empty() guards leave promise to drop its Strong on scope exit, and the drain_pending_host_native .unwrap_or(ZERO) feeds directly into the new guard.
Checked DNSLookup::Drop (poll_ref unref, resolver deref) is safe at this shutdown stage — same precondition as the existing AsyncFSTask arm.

Extended reasoning...

Overview

This PR fixes a crash (ASSERT(!target.isEmpty()) / segfault at 0x5) when worker.terminate() races an in-flight dns.lookup() completion. Three files: four identical is_empty() guards in DNS on_complete sinks in src/runtime/dns_jsc/dns.rs, a new GetAddrInfoRequest::release_for_shutdown reclaim helper, an .unwrap() → .unwrap_or(JSValue::ZERO) in drain_pending_host_native, a GetAddrInfoRequestTask arm in __bun_release_task_at_shutdown (src/runtime/dispatch.rs), and a regression test in worker-terminate-lifetime.test.ts.

Security risks

None. This is a crash/UAF fix on the shutdown path; no user-controlled input reaches new parsing or allocation logic.

Level of scrutiny

High. This is native unsafe Rust in the memory-safety category REVIEW.md flags as most-blocked: JSC Strong handle lifetime across worker VM teardown, raw-pointer linked-list walking in release_for_shutdown, and shutdown ordering invariants (must run after close_dns_for_terminate but before JSC destructOnExit). The reasoning about why request_completed() is deliberately not called (would re-arm the timer mid-shutdown) is subtle enough that CodeRabbit flagged it and the author had to justify it in-thread.

Other factors

The change is small, well-commented, and follows the exact pattern of the existing AsyncFSTask/FetchTasklet/JSCDeferredWorkTask arms in __bun_release_task_at_shutdown. The regression test is hermetic (IP-literal lookups, Atomics.wait handshake), fails 8/8 before and passes 8/8 after per the PR description, and CI is green on the new test. Both CodeRabbit findings were resolved with sound reasoning. However: this is carved out of #34154 (47-file ShutdownGate PR) and someone familiar with that larger design should confirm the shared hunks land cleanly and that release_for_shutdown's reclaim-only shape (no request_completed(), no backend file_poll handling) is complete for every state a queued GetAddrInfoRequestTask can be in at shutdown. That is not something I can auto-approve.