Repository navigation
Conversation
c-ares turns TCP Fast Open on for macOS only when ares_config.h defines HAVE_CONNECTX (ares_set_socket_functions.c). The hand-written DARWIN list never had the name, so macOS builds since v1.3.14 compile Fast Open out. Upstream's CMake defines the name when connectx() exists. Add a config test that reads the generated header.
|
Updated 6:04 AM PT - Oct 21st, 2026
✅ @robobun, your commit 2c0d0099c6311b4e309c114203a5137f7767f1e0 passed in 🧪 To try this PR locally: bunx bun-pr 46246That installs a local version of the PR into your bun-46246 --bun |
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 19 seconds. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Status: waiting for CI on the head commit. Two new macOS test cases have their first run there.
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline finding, I also checked whether enabling HAVE_CONNECTX could leave connectx() undeclared on the darwin target: scripts/build/deps/cares.ts:82 already passes _DARWIN_C_SOURCE for darwin, which is what Apple's sys/socket.h gates the connectx prototype on, so no implicit-declaration risk. The runtime effect (c-ares now taking the connectx() TCP Fast Open path for DNS-over-TCP on macOS) could not be exercised from this host; the vendored c-ares source is not in this checkout and nothing here ran on macOS, so the macOS CI run of test/js/node/dns/dns-tcp-bidirectional-poll.test.ts is the first real check of that path.
Extended reasoning...
Two-file, +11-line change: adds HAVE_CONNECTX to the DARWIN define table in scripts/build/deps/cares.ts (so macOS ares_config.h enables c-ares' connectx-based TCP Fast Open path) and a host-side test asserting the generated darwin header contains the define. No security-sensitive surface; it is a build-config flag change whose only runtime effect is on macOS DNS-over-TCP connects. Deferring rather than approving because the behavioral consequence runs only on macOS, which neither the author nor this review could exercise, and an inline test-coverage finding was posted.
…e c-ares config case
There was a problem hiding this comment.
LGTM — a one-line addition to the darwin define list, and the follow-up commit covers what I asked for in the earlier inline comment (both darwin arches, plus absence on linux and freebsd).
What was reviewed:
configH()in scripts/build/deps/cares.ts splicesDARWINonly in thecfg.darwinbranch, so linux/android/freebsd/windows headers are unchanged;_DARWIN_C_SOURCEis already set for darwin targets, so theconnectx()prototype is visible to the compile.- The new test reads the literal
ares_config.hstring offcares.build(cfg)with akind === "direct"/typeof === "string"guard (headers are typedstring | HeaderSubst), and the linux/freebsd cases pass explicit/fakesysroots the same way the neighboring tests do, soresolveConfignever probes the filesystem. - Restores the
_connectximport that shipped in macOS binaries from v1.1.38 through v1.3.13, so this is not a first-time code path; the author notes nothing ran on macOS locally, so the macOS CI lane (dns-tcp-bidirectional-poll test) is the on-target check.
Extended reasoning...
The diff adds HAVE_CONNECTX to the darwin-only define list in scripts/build/deps/cares.ts and a config test in test/internal/macos-cross-config.test.ts that asserts the define is present for darwin aarch64 and x64 and absent for linux and freebsd; no vendored source, patch, or commit pin changes. It touches no security-sensitive surface. Neither file is covered by .github/CODEOWNERS, the change is 22 insertions with a test that would fail without the define, and the only open feedback on the PR (my own earlier inline comment on test coverage) was addressed by commit 8b78ef3. The c-ares sources are not vendored in this checkout, so the claim that c-ares gates its macOS TCP Fast Open path on HAVE_CONNECTX with no companion define rests on the PR's own object-file evidence and prior release binaries rather than a read of the pinned upstream source here.
Two macOS cases in dns-tcp-bidirectional-poll.test.ts: - A dylib inserted into the fixture prints the flags of each connectx() call. c-ares must call it once, with CONNECT_RESUME_ON_READ_WRITE | CONNECT_DATA_IDEMPOTENT. - A fixture blocks its event loop right after resolveTxt(). With Fast Open the query is already written, so the answer waits at the first poll, readable and writable in one batch.
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline findings, I checked that configH in scripts/build/deps/cares.ts splices DARWIN only under the cfg.darwin branch and that nothing else in the tree reads HAVE_CONNECTX, so the define cannot reach the Linux, FreeBSD, Android or Windows headers. The four-target matrix case added in the latest commit covers the x64 and negative-contract point raised on the earlier thread.
Extended reasoning...
The change adds one define (HAVE_CONNECTX) to the darwin list in scripts/build/deps/cares.ts, switching c-ares DNS-over-TCP on macOS from connect() to connectx() with TCP Fast Open flags, and adds a configure-time matrix test plus two macOS-only runtime tests with a new fixture. No auth, crypto or input-parsing surface is touched; the runtime effect is a networking behaviour change in release macOS binaries that the author says was not exercised locally on macOS. Three inline findings on the new tests are posted; the build-side define itself was confirmed darwin-only by reading configH, and the earlier inline thread about x64 and the negative contract is covered by the new matrix case.
| stderr: "pipe", | ||
| }); | ||
|
|
There was a problem hiding this comment.
🟡 (optional) A maintainer whose fixture dies before the query reaches the server gets a silent test timeout with no stderr, instead of a failing assertion. At test/js/node/dns/dns-tcp-bidirectional-poll.test.ts:138 await answered only resolves from the server's data handler; nothing rejects it when the spawned fixture exits early (bad argv[2] for setServers, a thrown resolveTxt, a crash), so the test hangs until the runner timeout and proc.stderr is never read or printed. Fix: race the readiness promise against fixture exit so any early exit rejects with the collected stderr and exit code (e.g. Promise.race([answered, proc.exited.then(async code => { throw new Error(...) })])), which also keeps the existing pass path unchanged.
Why this was flagged
The fixture test/js/node/dns/dns-tcp-answer-before-poll-fixture.ts is spawned at test/js/node/dns/dns-tcp-bidirectional-poll.test.ts:129-135 with stdin, stdout and stderr all piped. The test then awaits answered at line 138, a promise resolved only inside the Bun.listen data handler at lines 117-123 once a full TCP DNS frame arrives. If the fixture exits before sending that frame (for example resolver.setServers throwing on the argv value, resolveTxt failing synchronously, or a runtime crash in a debug/ASAN build), no code path rejects answered: proc.exited is not raced and stderr is only drained at line 142, after the await. The test therefore sits until bun test's per-test timeout and reports a timeout with none of the fixture's stderr, whereas a wired failure would fail immediately with the fixture's message. The base branch has no such test, so this only affects diagnosability of the new test, but the repository review rules explicitly require every failure event (process exit included) to reject the awaited promise.
Verification: Triggers whenever the spawned fixture exits before it sends a complete TCP DNS frame to the local server. In test/js/node/dns/dns-tcp-bidirectional-poll.test.ts, the test does await answered; at line 139 with nothing racing it against proc.exited. So a fixture that dies early leaves the test pending until the runner's per-test timeout, and its stderr is never surfaced.
| let calls = 0; | ||
| resolver.resolveTxt("example.test", (err, records) => { | ||
| calls++; | ||
| // Print one turn of the event loop later, so that a second call is counted. | ||
| setImmediate(() => { | ||
| console.log(JSON.stringify({ calls, code: err ? (err as NodeJS.ErrnoException).code : null, records })); | ||
| process.exit(0); | ||
| }); | ||
| }); | ||
|
|
There was a problem hiding this comment.
🟡 nit (optional): the "delivered once" half of the new macOS test cannot fail, so a double delivery would pass unnoticed. The fixture counts invocations of a node:dns callback, but Bun invokes that callback from a promise .then (src/js/node/dns.ts:647-656), so calls is 1 whenever anything is printed at all; the setImmediate deferral at dns-tcp-answer-before-poll-fixture.ts:18 adds nothing. Fix: drop the counter and the deferral (keep the records/exit assertion) and name the test for what it checks, an answer already readable at the first poll is delivered; or count at a layer that can observe a second completion.
Why this was flagged
The test at test/js/node/dns/dns-tcp-bidirectional-poll.test.ts:115-149 asserts calls: 1 from the fixture's JSON. The fixture increments calls inside the resolveTxt callback (dns-tcp-answer-before-poll-fixture.ts:15-16) and prints one event-loop turn later (line 18-21). Resolver.prototype.resolveTxt calls the user callback from the native promise's .then handlers (src/js/node/dns.ts:647-656), and a promise settles at most once; natively, the first c-ares completion consumes the ResolveInfoRequest (src/runtime/dns_jsc/dns.rs:5443-5447), so a second c-ares callback would be a use-after-free rather than a second JS call. Therefore calls is always exactly 1 whenever the line is printed, and 0 is never printed (the only print is inside the callback); the calls field and the setImmediate deferral can never turn the test red. The useful part of the test (answer already in the socket at the first poll is delivered, process exits 0 with [["hello"]]) still works. This is a test-quality nit; the base branch has no such test.
Verification: Resolver.prototype.resolveTxt in /home/claude/bun/src/js/node/dns.ts:647-656 invokes the user callback only from .then handlers on the native promise; a promise settles at most once, so exactly one of those handlers runs, once. Therefore in /home/claude/bun/test/js/node/dns/dns-tcp-answer-before-poll-fixture.ts:14-22 calls is always exactly 1 whenever the JSON line is printed at all.
| // when its config defines HAVE_CONNECTX. The answer is the same with connect(), | ||
| // so the call itself is observed: a dylib inserted into the fixture prints the | ||
| // flags of each connectx() and forwards it. | ||
| test.skipIf(!isMacOS || !cc)("c-ares opens its TCP DNS connection with connectx() on macOS", async () => { |
There was a problem hiding this comment.
🟡 nit (optional): the file now runs three independent subprocess tests plus a cc compile strictly in series, while test/CLAUDE.md asks that tests which each spawn processes use test.concurrent unless that is hard. Each new test owns its own tempDir, port 0 server and fixture process, so nothing is shared. Fix: declare the three tests with test.concurrent.skipIf(...) (or wrap them in describe.concurrent) so the macOS lane does not pay the compile and three Bun startups back to back, especially on debug/ASAN builds.
Why this was flagged
test/js/node/dns/dns-tcp-bidirectional-poll.test.ts:18, :37 and :110 declare test.skipIf(...) serial tests; the second one compiles a dylib with cc at lines 56-66 and then spawns bunExe(), the third starts a Bun.listen server and spawns another bunExe(). None of the three shares state: separate tempDir, ephemeral ports, separate fixture files. test/CLAUDE.md 'Prefer concurrent tests over sequential tests' applies when multiple tests in one file spawn processes. On the base branch the file had a single test, so the serial cost is introduced by this diff; on a debug+ASAN macOS lane three sequential Bun startups plus a compile add several seconds of wall time with no stated reason for staying serial.
Verification: Triggering condition: any macOS run of the file, where all three tests now execute. test/js/node/dns/dns-tcp-bidirectional-poll.test.ts declares three serial tests at lines 18, 37 and 110, none of which shares state. test/CLAUDE.md line 22 states: "Prefer concurrent tests over sequential tests". Nothing breaks functionally; the cost is serial wall time (a C compile plus three Bun startups).
Problem
llvm-nm -ufinds_connectxin the macOS binaries of v1.1.38 to v1.3.13, not in later ones.DARWINlist (scripts/build/deps/cares.ts:199) has noHAVE_CONNECTX, whichares_set_socket_functions.c:70needs on macOS.Fix
HAVE_CONNECTXto theDARWINlist. The macOSares_config.hgains one line. No other target's header changes.CMakeLists.txt:434), and bun's Linux build has Fast Open. Nobody asked, and no gain is measured.test/internal/macos-cross-config.test.tsfails without the name. Two new macOS cases intest/js/node/dns/dns-tcp-bidirectional-poll.test.tsrun first in CI.Background
connectx(2).node:dnsresolve*().DARWINlist is the only source of the name. Left out: a check of all hand-written names against upstream's probe, and zlib-ng'sARM_AUXV_HAS_CRC32on Linux arm64, which build: convert 12 vendored deps from nested-cmake to DirectBuild #29584 also dropped (scripts/build/deps/zlib.ts:201, no issue yet).Downsides
{ timeout: 1000, tries: 1 }then fails withETIMEOUT.tries: 1getsECONNREFUSED(source reading).Notes
Release binaries
llvm-nm -u <bun> | grep -x _connectx, on the GitHub release zips and the@oven/bun-darwin-*npm packages:connectx(call insrc/,packages/andvendor/is the c-ares one.connectx()since 10.11.How the name went missing
connectx. build: convert 12 vendored deps from nested-cmake to DirectBuild #29584 wrote the answers by hand. Its description says the darwin sections were "written from docs, not verified on-target".HAVE_ARC4RANDOM_BUF(glibc 2.36) and sixGETNAMEINFO_*/RECVFROM_QUAL_ARG5names that no compiled source reads.Why parity, and not speed
connectx, TCP Fast Open orHAVE_CONNECTX. Nobody reported the loss, and this PR has no measurement of a gain.1.1.1.1appears to support TFO, however8.8.8.8does not."What other builds do
bun-linux-x64v1.4.3 uses Fast Open for c-ares today. With a preload shim that logs socket calls,dns-tcp-bidirectional-poll-fixture.tsshowssetsockopt(IPPROTO_TCP, TCP_FASTOPEN_CONNECT, 1) = 0, thensendto()with the address on the TCP socket.ares_set_socket_functions.c:58-69keys them onTCP_FASTOPEN_CONNECTandTCP_FASTOPENfrom<netinet/tcp.h>._connectx. Homebrew's node links that library (--shared-cares)._connectx. Node's checked-indeps/cares/config/darwin/ares_config.his from c-ares 1.26.0, which is older than Fast Open in c-ares.Other names, and what this PR leaves out
HAVE_CONNECTXis the only name whose absence turns a feature off.HAVE_AVAILABILITYMACROS_HandHAVE_SYS_RANDOM_Honly add an#include.HAVE_MSG_NOSIGNALis redundant, because c-ares setsSO_NOSIGPIPEon macOS.HAVE_STRICMPand the related names, c-ares uses its own compare loop.HAVE_ARC4RANDOM_BUFis absent.getrandomis used.scripts/build/deps/zlib.ts:201definesARM_AUXV_HAS_NEONfor Linux arm64 and notARM_AUXV_HAS_CRC32.vendor/zlib/arch/arm/arm_features.c:29then reports no CRC32 support, andfunctable.c:250never selectscrc32_armv8. Upstream probes the name (CMakeLists.txt:748). The fix changes the Linux arm64 binaries and needs a CRC-32 number from an arm64 machine, so it is a change of its own._connectx, and a patch that makes c-ares ignore the macro.binary-expectations.tsstates the binary's contract with the machine it runs on (exports, libraries, symbol versions), and a required import would be a new kind of rule there. The patch diverges from upstream.Costs on a bad network path
Measured on Linux (bun 1.4.3, loopback, a network namespace with
iptables -A OUTPUT -p tcp --syn --tcp-option 34 -j DROP). Linux builds have Fast Open today, so these are costs that Linux users already have.dns-tcp-bidirectional-poll-fixture.ts(timeout: 1000, tries: 1)ETIMEOUTafter about 1.2 sares_conn.c:376-380).tcp_tfo_checkandtcp_tfo_write_cookieinbsd/netinet/tcp_output.c). A SYN retransmit callstcp_disable_tfo()for that socket (tcp_timer.c).tcp_cache_hash_srcintcp_cache.c), and needs two losses.ENODATAon a socket that sent SYN data when it decides that a middlebox dropped the data (tcp_timer.c,tcp_input.c). c-ares has no case for that errno:ares_socket.c:58-98maps it to a generic failure, andares_process.c:531-537fails the connection. This needs a resolver that granted a cookie earlier. No test reaches it.Tests
macos-cross-config.test.ts: the new case checks four targets. macOS arm64 and x64 define the name, Linux and FreeBSD do not. It fails when the name is removed fromDARWIN, and when it is added to the sharedPOSIXlist.dns-tcp-bidirectional-poll.test.ts, first new case (macOS with a C compiler): a dylib with a__DATA,__interposeentry forconnectxis inserted into the existing fixture withDYLD_INSERT_LIBRARIES. The case expects one call with flags 3 (CONNECT_RESUME_ON_READ_WRITE | CONNECT_DATA_IDEMPOTENT).test/js/bun/dns/resolve-dns.test.tsuses the same mechanism.resolveTxt()withRES_OPTIONS=use-vcand then blocks its event loop. The test answers the query and only then releases the fixture. With Fast Open, c-ares writes the query insideresolveTxt(), so the answer waits at the first poll, and kqueue reports readable and writable in one batch. Withconnect()the query is not sent before the first poll, and the case times out.net.ipv4.tcp_fastopen=0, which is theconnect()flow. The case is limited to macOS: Linux accepts the early write only when the loopback handshake is already complete, and macOS queues it in the kernel.Local checks
#define HAVE_CONNECTX 1. Linux (glibc, musl, Android), FreeBSD and Windows are byte-identical.ares_build.h, the-Ddefines and the source list are identical on every target.arpa/nameser.h,notify.horSystemConfiguration, which that header set does not have. None of them reads the name.ares_set_socket_functions.o.__textgoes from 1524 to 1656 bytes on arm64 and from 1702 to 1830 bytes on x86_64. Its undefined symbols gain_connectxand nothing else.bsd/netinet/tcp_usrreq.c: withCONNECT_RESUME_ON_READ_WRITE,connectx()sets the state toSYN_SENTand sends nothing. The firstsendto()then sends the SYN. The kernel ignores the address argument because the state is alreadySYN_SENT.tsc -p scripts/build/tsconfig.jsonandprettier --checkpass.macOS
bun-profile: 714 on macOS arm64 and 716 on x64 with this change, 713 and 715 for the merge base. The check prints no names.dns-tcp-bidirectional-poll.test.ts(the case that was there before) passes on macOS 15 for arm64 and x64. It passes onmaintoo.on_dns_socket_stateinsrc/runtime/dns_jsc/dns.rs). With and without Fast Open, c-ares asks for readable and writable, then readable only. FilePoll: support readable+writable on one poll (fixes c-ares TCP DNS assertion) #29955 reworked how Bun's poll code handles both directions on one descriptor.History, and c-ares 1.34.9
connectx()on macOS with c-ares up to 1.34.6 and with the poll code from before FilePoll: support readable+writable on one poll (fixes c-ares TCP DNS assertion) #29955. This PR is the first build that runs it with c-ares 1.34.8 and the current poll code. The old releases are not a record for that combination.connectx()with 1.34.9. Ifdns-tcp-bidirectional-poll.test.tsgoes red there, revert theHAVE_CONNECTXline and do not hold the bump.Self-review
Limits
llvm-nm -uon them is not done. The import count and the object-level compile are the evidence that the import returns.