dns: distinguish "this name does not resolve" from "this resolver failed" - #18
Merged
Conversation
…led" Unblocks the re-selection follow-up recorded in spark's ADR 0014. The proxyless transport wants to drop a strategy whose resolver has died, but could not: every resolve failure arrived as `io::Error::other`, so NXDOMAIN for a mistyped host was indistinguishable from a blackholed resolver. Acting on that signal would have discarded a working strategy — and forced a full verified search — every time a user visited a domain that does not exist, which is why the earlier attempt at self-healing had to be reverted. The information was always there, just flattened. `codec::parse_response` already returns `Rcode(3)` for NXDOMAIN and `validate_answers` already separates `Empty` from `Poisoned`; `map_err(io::Error::other)` threw the distinction away at the last step. Now each failure maps onto a meaningful `io::ErrorKind`: - **NotFound** — NXDOMAIN, or an answer with no usable address. The resolver did its job. - **InvalidData** — SERVFAIL/REFUSED, truncated, not-a-response, mismatched transaction ID, or bogon-only records. The resolver answered and the answer cannot be believed. Poisoned belongs here rather than with NotFound: bogons mean something is lying, which is very much about the resolver. - **InvalidInput** — an unencodable name. Our bug; nothing was ever sent. - transport kinds pass through untouched, which is the clearest case of all. `indicts_resolver(&io::Error)` is the predicate callers actually want — "should I stop trusting this resolver?" — so they need not learn the mapping. No signatures changed, so existing callers are unaffected; they simply get better-kinded errors. `Kind::System` is documented as the exception: `getaddrinfo` reports "no such host" and "the network is down" through the same opaque failure, so it is reported as indicting. That errs toward re-selecting, the safe direction — moving off the OS resolver is cheap and is the right move if the network really did change. Gate: fmt, clippy --workspace --all-targets -D warnings, cargo test --workspace (flint-dns 33, three new covering both sides of the distinction), and cargo doc with zero warnings. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BufGsK81otiqihvrZdfoJX
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe DNS resolver now classifies failures by resolver attribution. NXDOMAIN and empty validated answers map to ChangesDNS resolver attribution
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant DNSResolver
participant DNSTransport
participant parse_response
participant ErrorClassifier
DNSResolver->>DNSTransport: send DNS query
DNSTransport->>parse_response: provide DNS response
parse_response-->>ErrorClassifier: return response or codec error
ErrorClassifier-->>DNSResolver: classify resolver attribution
Possibly related PRs
Suggested reviewers: 🚥 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: 2
🤖 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-dns/src/lib.rs`:
- Around line 96-108: Update codec::parse_response to inspect the DNS header
flags before parsing records, and return codec::DnsError::Truncated when flags &
0x0200 is nonzero. Add a test covering a syntactically valid response with the
TC flag set and verify it is rejected as truncation rather than producing
partial addresses; keep codec_err’s existing InvalidData mapping unchanged.
- Around line 92-94: Update indicts_resolver to return false for both
io::ErrorKind::NotFound and io::ErrorKind::InvalidInput, while preserving
indictment for other errors. In crates/flint-dns/src/lib.rs lines 375-383, add
an assertion verifying that an InvalidInput error does not indict the resolver.
🪄 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: 4df7b01c-1571-4441-8140-0212f338bfd5
📒 Files selected for processing (1)
crates/flint-dns/src/lib.rs
There was a problem hiding this comment.
Pull request overview
This PR improves flint-dns error classification so callers can reliably distinguish “the name doesn’t resolve” (e.g., NXDOMAIN / no usable records) from “the resolver failed/misbehaved” (e.g., SERVFAIL, malformed responses, poisoning), enabling higher-level strategy re-selection without false positives.
Changes:
- Adds
indicts_resolver(&io::Error) -> boolas a simple policy helper for callers. - Preserves resolver-vs-name attribution by mapping codec and validation failures into meaningful
io::ErrorKinds (instead of flattening toOther). - Adds unit tests covering the new kind mapping and the resolver-indictment decision boundary.
Suppressed comments (2)
crates/flint-dns/src/lib.rs:103
- This comment says an unencodable DNS name is “our bug”, but
codec::DnsError::BadNamecan be triggered by caller-supplied input (e.g., a label > 63 bytes). It’s more accurate to describe this as invalid input / nothing sent, rather than an internal bug.
// A name we could not even encode is our bug, not the resolver's.
codec::DnsError::BadName => io::ErrorKind::InvalidInput,
crates/flint-dns/src/lib.rs:380
- This test’s name/comment (“our bug” / “still indicts”) becomes misleading once
InvalidInputis treated as non-indicting. Consider asserting theindicts_resolverbehavior here so the contract stays covered.
fn an_unencodable_name_is_our_bug_not_a_resolver_failure() {
// Still "indicts" (it is not a NotFound), but the kind records who to blame: nothing was ever
// sent, so no resolver was involved.
assert_eq!(
codec_err(codec::DnsError::BadName).kind(),
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…uncated answers Review catches (CodeRabbit + Copilot, PR #18). Both reviewers independently found the first, which is a logic bug of mine. **`indicts_resolver` blamed the resolver for a caller error.** `DnsError::BadName` maps to `InvalidInput` precisely because *we* could not encode the name and nothing was ever sent — the doc and the PR's own mapping table both say so — yet the predicate returned `true` for everything except `NotFound`. A caller would have discarded a perfectly good resolver over a name that never left the process, which is the same class of mistake as blaming it for a typo'd domain: exactly what this predicate exists to prevent. My own test made it easy to miss: it asserted the *kind* was `InvalidInput` without ever asking what `indicts_resolver` said about it. Now both are asserted. **The TC flag was never checked.** `parse_response` validated QR and RCODE but not TC, so a syntactically complete response the server had cut short would parse as a success and return whatever records happened to fit. That mattered little before; it matters now, because if truncation removed every address record the result would be "no addresses" → `NotFound` → "this name does not resolve" — and this PR makes callers *act* on that. The verdict would be exactly backwards: the resolver is the thing that failed to serve us, so it should be indicted, not excused. TC is now checked after the RCODE, so a truncated NXDOMAIN still reports the more specific NXDOMAIN, and maps to `InvalidData` (indicting). We do no TCP retry, so a resolver that truncates genuinely cannot answer the query and should lose to one that can. Two tests: that a TC answer is rejected rather than partially parsed (with the flag cleared again to prove TC is what rejected it, not the fixture), and that NXDOMAIN wins over TC. Gate: fmt, clippy --workspace --all-targets -D warnings, cargo test --workspace (flint-dns 35, two more new), 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 59 minutes. |
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.
Unblocks the re-selection follow-up recorded in spark's ADR 0014 — the first of the three open items from the proxyless port.
The problem this removes
spark's proxyless transport wants to drop a strategy whose resolver has died, but couldn't: every resolve failure arrived as
io::Error::other, so NXDOMAIN for a mistyped host was indistinguishable from a blackholed resolver.That is not hypothetical — I shipped exactly that self-heal in spark#137 and had to revert it during review, because acting on the signal would have discarded a working strategy and forced a full verified search every time a user visited a domain that does not exist.
The information was always there
codec::parse_responsealready returnsRcode(3)for NXDOMAIN.validate_answersalready separatesEmptyfromPoisoned. It wasmap_err(io::Error::other)throwing the distinction away at the very last step.Each failure now maps onto a meaningful
io::ErrorKind:NotFoundInvalidDataInvalidInputPoisoneddeliberately sits with the resolver rather than withNotFound: bogons mean something is lying, which is very much about the resolver (or whoever is speaking for it).The API callers actually want
"Should I stop trusting this resolver?" — so callers needn't learn the mapping. No signatures changed, so existing callers are unaffected; they just get better-kinded errors.
One honest exception
Kind::Systemcannot be discriminated:getaddrinforeports "no such host" and "the network is down" through the same opaque failure, so a system-resolver error is reported as indicting. That errs toward re-selecting, which is the safe direction — moving off the OS resolver is cheap and is the right call if the network really did change. Documented on the function.Verification
cargo fmt --all -- --check— cleancargo clippy --workspace --all-targets -- -D warnings— cleancargo test --workspace— clean; flint-dns 33 tests, 3 new covering both sides of the distinction plus the caller-bug casecargo doc— zero warningsFollow-up
With this merged and the pin bumped, spark can reinstate the self-heal it had to revert — gated on
indicts_resolverso a typo'd domain no longer evicts a good strategy. That is the other half of the re-selection item; an explicit network-change control handle remains a separate, larger option.🤖 Generated with Claude Code
https://claude.ai/code/session_01BufGsK81otiqihvrZdfoJX
Summary by CodeRabbit