dns: make the resolver pool dual-stack and race A/AAAA in the proxyless probe - #17
Merged
Conversation
…ss probe Closes the IPv6 gap CodeRabbit found reviewing spark#137. Two halves, and both had to move — fixing either alone leaves a v6-only network exactly as stranded. **The pool was 23 IPv4-only entries.** On a v6-only network that is not merely suboptimal, it is unreachable: no resolver answers, every proxyless candidate fails, and strategy selection reports "nothing works" on a network where proxyless would in fact work fine. Added v6 raw-IP entries for all four operators. Both families for *every* operator rather than one or two, because a partial v6 set silently costs v6-only clients the operator/jurisdiction diversity the pool exists to provide — they would fall back to whichever provider happened to get an entry. There is a test asserting the v4 and v6 host sets are equal, so a future operator cannot be added to one family only. Adding these costs a v4-only client almost nothing: the connect fails immediately with "no route to host" rather than timing out, so the entry loses its race and frees its window slot at once. **The probe resolved A only**, so even with v6 resolvers reachable, a v6-only host would have produced no address and failed every candidate. `resolve_all` now races A and AAAA (either may legitimately fail — a host with no AAAA is ordinary, so only the empty union is an error) and returns every address; `probe` and `dial` try them in order, because resolution succeeding does not make an address routable and on a single-stack network the wrong family simply is not. **The Quad9 v6 entry is `2620:fe::10`, not the hostname's AAAA.** `dns.quad9.net` resolves to `2620:fe::fe`, which is the *filtering* service; the v4 entry deliberately uses the no-blocking `9.9.9.10` so a flagged config host is never NXDOMAIN'd out from under us. Taking the AAAA record would have quietly given the two families different filtering behaviour for the same host. Tested. Addresses confirmed against live AAAA records (`dig AAAA <host>`, 2026-07-30). Being explicit about the limit of that: it is weaker than the "verified live" claim on the v4 entries, which were confirmed to *answer DoH*. The vantage point here has no IPv6 egress, so these were confirmed only to be the correct addresses — a v6-capable check should confirm they serve DoH with a valid certificate before they are relied on. Recorded in the code comment rather than left to look equally trusted. Until then they can only help: a non-answering entry loses its race exactly as a blocked one would. `tokio`'s `macros` feature is now on for flint-proxyless (`tokio::join!`). Gate: fmt, clippy --workspace --all-targets -D warnings, cargo test --workspace (flint-dns 30, two new), cargo doc with zero warnings on both crates, and cargo check --features boring. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BufGsK81otiqihvrZdfoJX
|
Caution Review failedAn error occurred during the review process. Please try again later. 📝 WalkthroughWalkthroughThe default resolver pool now includes IPv6 DoH endpoints, while proxyless probing and dialing resolve and try IPv4 and IPv6 addresses under a shared attempt deadline. Tests and design documentation cover dual-stack behavior and Quad9’s IPv6 endpoint selection. ChangesDual-stack resolution
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ProxylessProbe
participant resolve_all
participant DNS
participant dial_first
participant VerifiedTLS
ProxylessProbe->>resolve_all: resolve A and AAAA before deadline
resolve_all->>DNS: concurrently query hostname
DNS-->>resolve_all: return IPv4 and IPv6 addresses
resolve_all-->>ProxylessProbe: return socket addresses
ProxylessProbe->>dial_first: try addresses until shared deadline
dial_first->>VerifiedTLS: dial each address
VerifiedTLS-->>dial_first: return success or failure
dial_first-->>ProxylessProbe: return first success or last error
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@crates/flint-proxyless/src/lib.rs`:
- Around line 497-517: Update resolve_all to preserve DNS resolution errors
instead of converting both failed A and AAAA queries to empty results. Track the
errors from resolve_one_with, return the relevant underlying error when both
lookups fail, and retain the existing NotFound error only when resolution
succeeds but produces no addresses.
- Around line 450-466: Update probe so the outer bounded call no longer covers
resolve_all and the entire address loop. Apply the existing timeout
independently to address resolution and to each flint_dial::dial attempt,
allowing subsequent addresses to be tried when one hangs while preserving the
current last-error and no-address behavior.
In `@docs/design.md`:
- Around line 175-177: Update the discussion near “connect fails immediately” to
qualify that this applies only when the v4-only client has no IPv6 route;
partially configured or blackholed IPv6 paths may still wait for the dial
timeout. State that bounded connection timeouts remain necessary.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5e0b133c-4c40-47c2-a492-3b05be87cba3
📒 Files selected for processing (4)
crates/flint-dns/src/pool.rscrates/flint-proxyless/Cargo.tomlcrates/flint-proxyless/src/lib.rsdocs/design.md
There was a problem hiding this comment.
Pull request overview
This PR closes an IPv6 reachability gap in the proxyless path by (1) making the DNS resolver pool dual-stack and (2) updating proxyless probing/dialing to resolve and try both A and AAAA results, ensuring v6-only networks can still select and use proxyless strategies.
Changes:
- Added IPv6 raw-IP DoH resolvers to the default resolver pool, plus tests enforcing v4/v6 operator parity and pinning Quad9’s no-blocking IPv6 address.
- Updated proxyless
probe/dialresolution to query A and AAAA concurrently and attempt all returned addresses in order. - Documented the dual-stack requirement and Quad9 IPv6 address choice; enabled
tokiomacros fortokio::join!.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| docs/design.md | Documents why the resolver pool and probe must be dual-stack, and explains the Quad9 IPv6 choice. |
| crates/flint-proxyless/src/lib.rs | Resolves A+AAAA and iterates all addresses when probing/dialing to support v6-only networks. |
| crates/flint-proxyless/Cargo.toml | Enables tokio macros to support tokio::join! used for dual-family resolution. |
| crates/flint-dns/src/pool.rs | Adds IPv6 resolver entries and tests enforcing dual-stack/operator parity and Quad9 no-block pin. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…solution error Review catches (CodeRabbit + Copilot, PR #17). Both real, and the first is a regression this PR introduced. **One shared 5s budget defeated the fallback it was added for.** `probe` wrapped the whole resolve-and-dial loop in a single `bounded()`, so a first address that *hangs* could consume the entire `ATTEMPT_TIMEOUT` before any other was tried. A censor's usual move is to blackhole rather than refuse, so that is the likely case, not a corner one — the multi-address loop would have been decorative exactly when it mattered, silently reintroducing the false negative it exists to prevent. Now each address gets `ATTEMPT_TIMEOUT / addresses` in a shared `dial_first` helper. I did not take the suggested fix of bounding each dial at a full `ATTEMPT_TIMEOUT`: that multiplies a candidate's worst case by its address count, which would blow the budget arithmetic documented one PR ago (`5s + ceil(N / 4) × 5s`, the basis for recommending a cap of 4). Slicing fixes the starvation while keeping the per-candidate cost at one `ATTEMPT_TIMEOUT`, so the cap recommendation still holds. **`resolve_all` swallowed the real DNS failure.** Both reviewers flagged this independently. `unwrap_or_default()` on each query meant that if *both* failed — resolver TLS handshake rejected, timeout, malformed response — the cause was discarded and replaced with a synthetic `NotFound`. "The resolver is unreachable" and "this domain has no records" are very different diagnoses, and collapsing them hides the one thing that tells a broken strategy from a nonexistent host. Now keeps the error, preserves its `ErrorKind`, and reports it in the message; the synthetic error remains only for a genuinely empty answer. Gate: fmt, clippy --workspace --all-targets -D warnings, cargo test --workspace, and cargo doc with zero warnings. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BufGsK81otiqihvrZdfoJX
myleshorton
commented
Jul 30, 2026
Contributor
Author
|
@coderabbitai full review |
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 31 minutes. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
crates/flint-proxyless/src/lib.rs:490
dial_firstslices the total 5sATTEMPT_TIMEOUTacross all resolved addresses. For hosts that legitimately resolve to many A/AAAA records (common with CDNs), this can shrink each address’s timeout to a few tens of milliseconds, making TCP+TLS handshakes effectively impossible and turning healthy candidates into systematic false negatives. Consider capping how many addresses are tried within one attempt budget (or otherwise enforcing a reasonable per-address minimum) so additional addresses don’t reduce each try below a workable floor.
let slice = ATTEMPT_TIMEOUT / u32::try_from(addrs.len().max(1)).unwrap_or(1);
let mut last = None;
for &addr in addrs {
match tokio::time::timeout(slice, flint_dial::dial(&verified(addr, host, policy))).await {
Ok(Ok(stream)) => return Ok(stream),
crates/flint-proxyless/src/lib.rs:534
resolve_allcurrently overwriteserron each failure (err = Some(e)), so if A and AAAA fail differently the earlier (potentially more informative) error can be lost. Preserving the first error (at least) avoids a later "no records"-style failure masking an earlier transport failure.
let mut err = None;
for result in [a, aaaa] {
match result {
Ok(found) => ips.extend(found),
Err(e) => err = Some(e),
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@crates/flint-proxyless/src/lib.rs`:
- Around line 450-456: Separate resolution and dialing time budgets in probe:
bound resolve_all independently, then pass the remaining budget to dial_first
instead of wrapping both operations in one bounded call. Update dial_first’s
documentation to reflect the actual 5-second resolution plus per-address batch
budget. In dial(), apply a timeout to resolve_all so a hung resolver cannot
stall the public dialing path.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: fbaf841b-ae26-4092-8cfc-84b7a9c42588
📒 Files selected for processing (1)
crates/flint-proxyless/src/lib.rs
…blanket timeout Review catch (CodeRabbit, PR #17 round 2). Three claims, all three checked out. **A slow resolve could starve dialing.** `probe` wrapped resolution and the address loop in a single `bounded()`, but `dial_first` computed each address's share from the *full* `ATTEMPT_TIMEOUT` — so after resolution had eaten part of the budget it was handing out shares that no longer existed, and the outer bound cut the later addresses off unheard. The slicing was arithmetic against a budget it did not have. **`dial()` resolved with no timeout at all.** The public dial path could hang indefinitely on a hung resolver — the exact failure the per-attempt bound exists to prevent, left open on the path a caller actually uses. Both are fixed by passing a **deadline** rather than a duration: the phases of one attempt now share a single budget. Resolution is bounded by it, and `dial_first` slices the time genuinely remaining. That keeps a candidate's worst case at one `ATTEMPT_TIMEOUT`, which is what the callers' `5s + ceil(N / 4) × 5s` arithmetic assumes — two independent 5s timeouts would have doubled it and broken the cap recommendation, which is why I did not simply bound each phase separately. **The doc was misleading.** `dial_first` quoted the callers' budget formula as if it were its own. Rewritten to explain what it actually guarantees and why it takes a deadline. `bounded()` stays for `connect_cached`'s two call sites, where a single phase is being bounded and a deadline would add nothing. Gate: fmt, clippy --workspace --all-targets -D warnings, cargo test --workspace, and cargo doc with zero warnings. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BufGsK81otiqihvrZdfoJX
Contributor
Author
|
@coderabbitai full review |
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 18 minutes. |
There was a problem hiding this comment.
♻️ Duplicate comments (2)
docs/design.md (1)
172-183: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winQualify the "fails immediately" guarantee (duplicate of a prior, still-open comment).
Both locations claim that a v4-only client hitting a v6 entry "fails immediately with 'no route to host' rather than timing out." This only holds when there's genuinely no IPv6 route; a partially configured or blackholed IPv6 path can still hang until the dial timeout — the same censorship pattern this codebase's
dial_firstdesign (crates/flint-proxyless/src/lib.rs) is explicitly built around. This was already raised ondocs/design.mdin a prior review round and doesn't appear to have been addressed.
docs/design.md#L172-L183: qualify "fails immediately" to the no-route case only, and note that blackholed/filtered v6 paths still rely on the bounded per-attempt timeout.crates/flint-dns/src/pool.rs#L373-L421: apply the same qualification to the matching comment on the new IPv6 entries.🤖 Prompt for 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. In `@docs/design.md` around lines 172 - 183, Qualify the “fails immediately” claim for v6 connections: in docs/design.md lines 172-183 and the matching comment in crates/flint-dns/src/pool.rs lines 373-421, state that immediate failure applies only when no IPv6 route exists, while blackholed or filtered IPv6 paths may wait for the bounded per-attempt dial timeout.crates/flint-proxyless/src/lib.rs (1)
450-456: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
dial()still has no timeout onresolve_all;probe()still shares one budget across resolution and dialing.This mirrors a previously raised, apparently unresolved concern on these exact lines:
dial()callsresolve_all(strategy, host).await?with no bound at all, so a hung/blackholed resolver stalls the publicdial()path indefinitely. Separately,probe()'s singlebounded()wrapsresolve_all+dial_firsttogether, so slow resolution eats into the timedial_first's per-address slicing assumes is available for dialing.🛡️ Proposed fix: bound `resolve_all` in `dial()`
pub async fn dial(strategy: &Strategy, host: &str, port: u16) -> io::Result<BoxedTlsStream> { - let addrs: Vec<SocketAddr> = resolve_all(strategy, host) + let addrs: Vec<SocketAddr> = bounded(resolve_all(strategy, host)) .await? .into_iter() .map(|a| SocketAddr::new(a.ip(), port)) .collect(); dial_first(&addrs, host, &strategy.policy).await }Also applies to: 463-470
🤖 Prompt for 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. In `@crates/flint-proxyless/src/lib.rs` around lines 450 - 456, Update the public dial flow and probe function to bound DNS resolution separately from dialing: apply the established timeout mechanism directly around resolve_all in dial(), and restructure probe() so resolution receives its own budget before dial_first starts with its full per-address budget. Preserve existing error propagation and Strategy policy handling.
🤖 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.
Duplicate comments:
In `@crates/flint-proxyless/src/lib.rs`:
- Around line 450-456: Update the public dial flow and probe function to bound
DNS resolution separately from dialing: apply the established timeout mechanism
directly around resolve_all in dial(), and restructure probe() so resolution
receives its own budget before dial_first starts with its full per-address
budget. Preserve existing error propagation and Strategy policy handling.
In `@docs/design.md`:
- Around line 172-183: Qualify the “fails immediately” claim for v6 connections:
in docs/design.md lines 172-183 and the matching comment in
crates/flint-dns/src/pool.rs lines 373-421, state that immediate failure applies
only when no IPv6 route exists, while blackholed or filtered IPv6 paths may wait
for the bounded per-attempt dial timeout.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5e60a788-0e3b-4d21-bacc-59914d766a42
📒 Files selected for processing (4)
crates/flint-dns/src/pool.rscrates/flint-proxyless/Cargo.tomlcrates/flint-proxyless/src/lib.rsdocs/design.md
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
crates/flint-proxyless/src/lib.rs:514
dial_firstcan compute a zero-lengthslicewhen little time remains (e.g., most ofATTEMPT_TIMEOUTwas spent resolving andaddrs.len()is larger than the remaining duration). A zeroDurationcausestokio::time::timeout(slice, ...)to time out immediately, so all addresses can fail without any real dial attempt. Consider ensuring a non-zero per-address slice (or bailing out if the deadline is already exhausted).
let remaining = deadline.saturating_duration_since(tokio::time::Instant::now());
let slice = remaining / u32::try_from(addrs.len().max(1)).unwrap_or(1);
let mut last = None;
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes the IPv6 gap CodeRabbit found reviewing spark#137 — the one thread I left deliberately open there, because the fix lives here rather than in spark.
Two halves, and both had to move: fixing either alone leaves a v6-only network exactly as stranded.
1. The pool was 23 IPv4-only entries
On a v6-only network that is not "suboptimal", it is unreachable: no resolver answers, every proxyless candidate fails, and strategy selection reports "nothing works" on a network where proxyless would in fact work fine.
Added v6 raw-IP entries for all four operators — Cloudflare, Google, Quad9, Mullvad.
Both families for every operator, not one or two. A partial v6 set silently costs v6-only clients the operator/jurisdiction diversity the pool exists to provide: they would fall back to whichever provider happened to get an entry. A test asserts the v4 and v6 host sets are equal, so a future operator cannot be added to one family only.
Cost to a v4-only client is near zero — the connect fails immediately with "no route to host" rather than timing out, so the entry loses its race and frees its window slot at once.
2. The probe resolved A only
Even with v6 resolvers reachable, a v6-only host would produce no address and fail every candidate.
resolve_allnow races A and AAAA and returns every address;probeanddialtry them in order, because resolution succeeding does not make an address routable, and on a single-stack network the wrong family simply is not. Either query may legitimately fail (a host with no AAAA is ordinary), so only the empty union is an error.The Quad9 entry is
2620:fe::10, not the hostname's AAAAWorth a look in review.
dns.quad9.netresolves to2620:fe::fe— the filtering service. The v4 entry deliberately uses no-blocking9.9.9.10so "a flagged config host is neverNXDOMAIN'd out from under us".Taking the AAAA record would have quietly given the two families different filtering behaviour for the same host: a config domain reachable on v4 and NXDOMAIN on v6. There's a test pinning it.
What was and wasn't verified
Addresses confirmed against live AAAA records (
dig AAAA <host>, 2026-07-30).Being explicit about the limit: that is weaker than the "verified live" claim on the v4 entries, which were confirmed to answer DoH. The vantage point used here has no IPv6 egress, so these were confirmed only to be the correct addresses. A v6-capable check should confirm they serve DoH with a valid certificate before they are relied on. That caveat is in the code comment rather than left to look equally trusted.
Until then they can only help: a non-answering entry loses its race exactly as a blocked one would.
Verification
cargo fmt --all -- --check— cleancargo clippy --workspace --all-targets -- -D warnings— cleancargo test --workspace— clean; flint-dns 30 tests, 2 new (dual-stack/operator-parity, and the Quad9 no-block pin)cargo doc— zero warnings on both cratescargo check --features boring— cleantokio'smacrosfeature is now enabled for flint-proxyless (tokio::join!).Follow-up
Once this merges, spark should bump its flint pin to pick it up — and the still-open thread on spark#137 can be closed against this PR.
🤖 Generated with Claude Code
https://claude.ai/code/session_01BufGsK81otiqihvrZdfoJX
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests