dns: destroy the per-VM c-ares channel before JSC/RareData teardown on worker terminate - #34455
Merged
Jarred-Sumner merged 5 commits intoJul 18, 2026
Merged
Claude / Claude Code Review
completed
Jul 17, 2026 in 24m 46s
Code review found 1 important issue
Found 4 candidates, confirmed 3. See review comments for details.
Details
| Severity | Count |
|---|---|
| 🔴 Important | 1 |
| 🟡 Nit | 2 |
| 🟣 Pre-existing | 0 |
| Severity | File:Line | Issue |
|---|---|---|
| 🔴 Important | src/runtime/dns_jsc/dns.rs:2150-2159 |
EDESTRUCTION callbacks enqueue reject_later ManagedTasks whose ctx leaks at shutdown |
| 🟡 Nit | test/js/web/workers/worker-terminate-lifetime.test.ts:158-161 |
ASAN-only test asserts stderr is exactly empty |
| 🟡 Nit | src/runtime/jsc_hooks.rs:1627-1637 |
close_dns_for_terminate only closes the global resolver, not per-instance new dns.Resolver() channels |
Annotations
Check failure on line 2159 in src/runtime/dns_jsc/dns.rs
claude / Claude Code Review
EDESTRUCTION callbacks enqueue reject_later ManagedTasks whose ctx leaks at shutdown
Running `ares_destroy()` while the VM is live means each in-flight query's `ARES_EDESTRUCTION` callback now reaches `reject_later(global_this)`, which heap-allocates a `Box<Context>` and enqueues it via `ManagedTask::new` (`cleanup: None`). That task never runs — `release_queued_tasks_for_shutdown` re-queues `ManagedTask` entries and `EventLoop::deinit` only frees `ctx` when `cleanup` is `Some` — so the `Context` / `ErrorDeferred` / hostname / `JSPromiseStrong` slot are orphaned per in-flight qu
Check warning on line 161 in test/js/web/workers/worker-terminate-lifetime.test.ts
claude / Claude Code Review
ASAN-only test asserts stderr is exactly empty
nit: REVIEW.md's subprocess-tests rule says "Never assert stderr is exactly empty (ASAN/debug builds emit benign warnings); assert a combined `{ stdout, stderr, exitCode }` object" — and this test runs *only* under ASAN. The three sibling tests in this file use the same shape and don't flake today, so this is non-blocking, but `expect({ stdout, stderr, exitCode }).toEqual({ stdout: "ok\n", stderr: "", exitCode: 0 })` matches the repo rule and gives a much better failure diff (you see the ASAN re
Check warning on line 1637 in src/runtime/jsc_hooks.rs
claude / Claude Code Review
close_dns_for_terminate only closes the global resolver, not per-instance new dns.Resolver() channels
This hook only reaches `(*state).global_dns_data` — per-instance `new dns.Resolver()` channels (each with their own `c_ares::Channel`) are not touched, so a worker terminated with an in-flight `r.resolve4()` leaks the resolver, its channel/fds, the heap request, and its `JSPromiseStrong`. This is a pre-existing leak (not the UAF fixed here) and would need a per-VM registry of live resolvers to close, so a follow-up is fine — just noting the gap since the PR description cites Node's `CleanupHandl
Loading