Skip to content

feat(native): resolve A and AAAA in parallel for Happy Eyeballs dials - #2749

Open
kixelated wants to merge 1 commit into
mainfrom
claude/github-issue-2595-6dfb92
Open

feat(native): resolve A and AAAA in parallel for Happy Eyeballs dials#2749
kixelated wants to merge 1 commit into
mainfrom
claude/github-issue-2595-6dfb92

Conversation

@kixelated

Copy link
Copy Markdown
Collaborator

Summary

  • feat(native): race resolved addresses Happy Eyeballs style when dialing #2594 races the resolved addresses, but the DNS phase in front of it was still one blocking getaddrinfo per dial, and that call doesn't return until both the A and AAAA answers are in. A slow or silently dropped AAAA query therefore still delayed the whole connect, which is exactly the network Happy Eyeballs exists for. Each family now gets its own family-restricted getaddrinfo, the two run in parallel, and failover::race pulls candidates as they land instead of from a completed Vec (RFC 8305 section 3).
  • New private moq-native::resolve module owns the DNS phase; failover keeps the connection phase. resolve::Candidates yields addresses in dial order (IPv6 first, families alternating, deduped by value, adapted to the local socket) and absorbs the old failover::interleave + failover::match_local, which only existed to order a fully-resolved list. When only the A answer has landed it holds the IPv4 address back for the Resolution Delay, so a resolver that is merely quicker with IPv4 doesn't decide the family.
  • Both queries go through the system resolver, one per family, so /etc/hosts, nsswitch (mDNS and friends), search domains and split-DNS resolve exactly as they did. That is why this uses family-restricted getaddrinfo (via dns-lookup, a thin safe binding) rather than hickory-resolver as DNS-phase Happy Eyeballs: parallel A/AAAA resolution via hickory-resolver #2595 proposed: an in-process resolver reads resolv.conf and nothing else, so .local names and other NSS sources would silently stop resolving. The new dependency adds no new transitive crates (cfg-if, libc, socket2, windows-sys are all already in the tree).
  • IP literals skip the resolver entirely, including moqt://192.0.2.1, where the URL parser hands back a Host::Domain because the scheme isn't special. That path previously went to getaddrinfo, and moqt://[::1]:443 went there with the brackets still attached.
  • New knob: --client-resolution-delay / MOQ_CLIENT_RESOLUTION_DELAY / resolution_delay in TOML, defaulting to RFC 8305's 50ms. 0s dials whichever family answers first.

The WebSocket fallback still resolves inside tokio_tungstenite, unchanged from #2594's note: racing it per-address needs a qmux API that takes an already-connected stream. So a ws:///wss:// dial, and the fallback arm of an https:// dial, still use one dual-family lookup.

Public API changes

Additive only, hence main:

  • moq_native::ClientConfig::resolution_delay and ClientConfig::resolved_resolution_delay().
  • resolution_delay: Duration on quinn::QuinnClient, noq::NoqClient, quiche::QuicheClient. Each already has a private field, so no struct literal breaks.
  • libmoq: moq_client_set_resolution_delay / moq_client_get_resolution_delay.
  • failover::interleave, failover::match_local and Aggregate were pub(crate); Aggregate gained a resolve method. No published item changed shape.

Test plan

  • cargo nextest run -p moq-native: 232 passed, including the new resolve tests (family alternation, IPv6-first, local-socket adaptation and its dedup/fallback cases migrated from match_local, IP literals, the limit quiche needs) and the timing ones under paused time: IPv4 waits out the Resolution Delay for a slow AAAA answer, proceeds when it expires, does not wait when the AAAA query already failed, and only the first candidate ever waits.
  • New failover tests for the streaming source: the first attempt goes out on the first answer while the other query is still outstanding, a candidate that resolves after the stagger has elapsed starts the moment it lands, and a resolution that yields nothing reports why instead of an attempt.
  • resolve::tests::resolves_localhost covers the real getaddrinfo path end to end via /etc/hosts; a_rejected_host_reports_a_failure covers the error path without depending on a resolver that answers NXDOMAIN honestly.
  • Config-merge regressions for the new flag (TOML survives a CLI re-apply, CLI parse, default).
  • just check.

Cross-package sync

  • doc/bin/relay/config.md documents the [client] resolution_delay knob; doc/lib/c/index.md lists the two new setters.
  • No wire change, so no drafts/ update. js/ is unaffected: the browser resolves for us.
  • rs/moq-ffi is untouched, so no wrapper regeneration. libmoq gained two additive C functions, which cpp/obs doesn't call.

Closes #2595

(Written by Opus 5)

The DNS phase in front of #2594's address race was still one blocking
getaddrinfo per dial, and that call doesn't return until both the A and
AAAA answers are in, so a slow or dropped AAAA query still delayed the
whole connect. Each family now gets its own family-restricted
getaddrinfo, the two run in parallel, and failover::race pulls candidates
as they land instead of from a completed Vec (RFC 8305 section 3).

Both queries go through the system resolver, so /etc/hosts, nsswitch,
search domains and split-DNS resolve exactly as before.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for security reviews. Please try again later.

@coderabbitai

coderabbitai Bot commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

@kixelated, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 6 minutes

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: a279a74d-57cb-421d-9dc6-3fb809cda5a5

📥 Commits

Reviewing files that changed from the base of the PR and between b8fa47f and d5b2d47.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (14)
  • doc/bin/relay/config.md
  • doc/lib/c/index.md
  • rs/libmoq/src/api.rs
  • rs/libmoq/src/test.rs
  • rs/moq-native/Cargo.toml
  • rs/moq-native/src/bind.rs
  • rs/moq-native/src/client.rs
  • rs/moq-native/src/failover.rs
  • rs/moq-native/src/lib.rs
  • rs/moq-native/src/noq.rs
  • rs/moq-native/src/quiche.rs
  • rs/moq-native/src/quinn.rs
  • rs/moq-native/src/resolve.rs
  • rs/moq-native/src/tcp.rs

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.

❤️ Share

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

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d5b2d47b7c

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

// stamped on here instead.
let answers = dns_lookup::getaddrinfo(Some(host), None, Some(hints)).map_err(io::Error::from)?;
answers
.map(|answer| Ok(SocketAddr::new(answer?.sockaddr.ip(), port)))

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Preserve IPv6 scope IDs from getaddrinfo

When NSS or mDNS resolves an IPv6 link-local target, getaddrinfo returns the required interface scope in the socket address's sin6_scope_id. Extracting only sockaddr.ip() and rebuilding it with SocketAddr::new clears that scope, so a valid address such as fe80::1%eth0 is dialed as fe80::1%0 and typically fails as unreachable. Preserve the returned SocketAddr metadata and replace only its port. (Written by GPT-5.6 Sol)

Useful? React with 👍 / 👎.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

DNS-phase Happy Eyeballs: parallel A/AAAA resolution via hickory-resolver

1 participant