Skip to content

Replace c-ares status transmute with a checked from_repr conversion#31989

Open
robobun wants to merge 3 commits into
mainfrom
farm/3562b7e2/cares-error-from-repr
Open

Replace c-ares status transmute with a checked from_repr conversion#31989
robobun wants to merge 3 commits into
mainfrom
farm/3562b7e2/cares-error-from-repr

Conversation

@robobun

@robobun robobun commented Jun 8, 2026

Copy link
Copy Markdown
Collaborator

What

Error::get in src/cares_sys/c_ares.rs converted a c-ares status code to the Error enum with a range assert! followed by mem::transmute::<i32, Error>. This PR derives strum::FromRepr on the enum and uses the generated Error::from_repr instead, removing the unsafe block. This is the existing in-tree pattern for int-to-enum conversion (see src/errno/windows_errno.rs, which documents that from_repr generates a checked match over every declared discriminant).

Why it is behavior-preserving

  • The enum discriminants are contiguous 1..=ARES_ENOSERVER (1..=26), exactly the range the old assert admitted, so from_repr returns Some for precisely the same inputs the transmute accepted.
  • An out-of-range status still panics through the same assert! with the same message.
  • from_repr is a generated const fn match; for a contiguous #[repr(i32)] enum this compiles to the same range check the assert already performed. This is a cold per-query error path in any case.

Verification

  • cargo clippy -p bun_cares_sys clean, bun bd builds.
  • New test in test/js/node/dns/node-dns.test.js: a local UDP DNS server (no external network) answers queries with NXDOMAIN and SERVFAIL, and the test asserts the resolver surfaces ENOTFOUND and ESERVFAIL with syscall: "queryA" and the hostname set. Real Node v24.3.0 returns the same values for the same fixture. Because the change is behavior-preserving by design, this test passes before and after; it locks the c-ares status to error code mapping that Error::get owns.
  • Hermetic c-ares error-path suites test-dns-channel-cancel.js, test-dns-channel-cancel-promise.js, and test-dns-channel-timeout.js (ECANCELLED and ETIMEOUT through the same conversion) pass with the debug build.
  • The rest of node-dns.test.js shows 40 pass / 30 fail both on this branch and on an unmodified build in my environment; the 30 failures are tests that require external DNS, which this environment blocks.

[daily-code-quality]

@robobun robobun enabled auto-merge (squash) June 8, 2026 16:26
@robobun

robobun commented Jun 8, 2026

Copy link
Copy Markdown
Collaborator Author
Updated 1:05 AM PT - Jun 10th, 2026

@robobun, your commit a1f84eb has 1 failures in Build #61689 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 31989

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

bun-31989 --bun

@github-actions github-actions Bot added the claude label Jun 8, 2026
@coderabbitai

coderabbitai Bot commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: ab027adb-67fb-4462-b3c2-48300acbc581

📥 Commits

Reviewing files that changed from the base of the PR and between a988615 and 506d9d9.

📒 Files selected for processing (2)
  • src/cares_sys/c_ares.rs
  • test/js/node/dns/node-dns.test.js

Walkthrough

This PR refactors DNS error code handling for safety and adds validation tests. The Error enum in c-ares now derives strum::FromRepr to enable checked conversion from numeric ARES_* codes, replacing an unsafe transmute-based approach. A new test suite validates that the resolver correctly surfaces Node.js-style error codes (ENOTFOUND, ESERVFAIL) when a local DNS server returns specific error responses.

Changes

DNS Error Code Handling

Layer / File(s) Summary
Error enum safety upgrade
src/cares_sys/c_ares.rs
Error derives strum::FromRepr for safe checked conversion from numeric codes. Error::get replaces unsafe transmute with Error::from_repr(n as i32) and asserts the conversion succeeds, panicking if the discriminant is out of range.
Error code mapping test
test/js/node/dns/node-dns.test.js
Imports UDP socket support and event utilities. New test suite spins up a local DNS server returning RCODE-determined responses, configures dns_promises.Resolver to query it, and asserts resolve4 converts NXDOMAIN and SERVFAIL error codes to expected Node-style errors.
🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main change: replacing an unsafe transmute with a checked from_repr conversion in the Error::get function.
Description check ✅ Passed The description comprehensively covers both template sections with clear explanations of what changed, why it's behavior-preserving, and detailed verification steps across multiple test suites.
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.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Comment @coderabbitai help to get the list of available commands and usage tips.

@robobun

robobun commented Jun 8, 2026

Copy link
Copy Markdown
Collaborator Author

CI triage for build 61388: the two failures on the debian-13-x64-asan lane are unrelated to this diff.

The new DNS test passes locally on this branch (debug ASAN build and release). Not pushing a retrigger while #31797 is still failing on main; once that lands, a rerun here should go green. Auto-merge is already armed.

@robobun

robobun commented Jun 8, 2026

Copy link
Copy Markdown
Collaborator Author

Build 61388 finished. Final triage: every failure is accounted for and none involves this diff.

  • bunx.test.ts "should handle package that requires node 24" failed on 6 lanes (alpine x64/x64-baseline/aarch64, darwin-14-x64, debian-13-x64-asan, windows-11-aarch64; darwin-26-aarch64's failure sits in the truncated head of its log, and its shard places bunx.test.ts there). This is CI: bunx.test.ts fails on all platforms — @angular/cli@latest now requires Node >= 24.15.0, bun reports 24.3.0 #31797, open and main-wide; it reproduces on an unmodified build in my environment. Fix pending in test: pin @angular/cli version in bunx node-version test #31820, still unmerged.
  • test/regression/issue/30205.test.ts (napi isolate, debian-asan only): passes locally on this branch under the ASAN debug build; no code path from the c-ares conversion to napi finalizers.
  • darwin-14-aarch64 only: sql-mysql.test.ts "process should exit when idle" and the Squid proxy WebSocket tests failed in 1-25ms (docker-backed services unavailable on that host), plus the fetch-leak.test.ts RSS-threshold test. None of these touch DNS or c-ares.

The new DNS test passed on every lane whose shard included it (alpine x64 and x64-baseline in this build), and locally on both the debug ASAN build and release.

I am not pushing a retrigger: #31797 fails deterministically on all platforms, so a rerun cannot go green until #31820 lands. Auto-merge is armed, so once main is fixed, updating the branch or rerunning CI should merge this automatically.

@robobun

robobun commented Jun 10, 2026

Copy link
Copy Markdown
Collaborator Author

Update after the branch was brought up to date with main (which now skips bunx.test.ts, the #31797 blocker):

Build 61658's only hard failure was test/regression/issue/30205.test.ts on the debian-13-x64-asan lane (subprocess passes all 8 tests, then exits 134 at teardown). That failure is not specific to this branch: the same test fails the same way on the same lane across unrelated branches (error-level on builds 61493 and 61415, retry-then-pass on 61676, 61461, and 61446). No DNS or c-ares code runs in that test.

Pushed one empty retrigger commit to re-roll. Auto-merge is armed, so a green run merges this automatically.

@robobun

robobun commented Jun 10, 2026

Copy link
Copy Markdown
Collaborator Author

CI status on build 61689: the single hard failure is test/js/web/streams/streams-leak.test.ts ("native ReadableStream reuses the pull buffer across small reads", chunk-count assertion at line 39) on the debian-13-x64 lane. This test is currently flaky repo-wide: it appears in the failure annotations of 76 of the last 149 branch builds, usually recovering on retry, and has exhausted retries on several unrelated branches (61411, 61426, 61471, 61611). Nothing in it touches DNS or c-ares.

Everything that failed in earlier builds is now green on this branch, including the napi isolate test and the asan lane; the new DNS test passes on every lane that ran it.

I have already used my one retrigger, so I will not push again. A job-level retry of the failed debian-13-x64 job in Buildkite (or waiting for the next streams-leak-tolerant run) should go green; auto-merge is armed and will merge on a green run.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant