Do not retry the same name server on a negative response

crates/proto/src/xfer/retry_dns_handle.rs

@@ -16,7 +16,16 @@ use crate::error::{ProtoError, ProtoErrorKind};
 use crate::xfer::{DnsRequest, DnsResponse};
 use crate::DnsHandle;
 
-/// Can be used to reattempt a queries if they fail
+/// Can be used to reattempt queries if they fail
+///
+/// Note: this does not reattempt queries that fail with a negative response.
+/// For example, if a query gets a `NODATA` response from a name server, the
+/// query will not be retried. It only reattempts queries that effectively
+/// failed to get a response, such as queries that resulted in IO or timeout
+/// errors.
+///
+/// Whether an error is retryable by the [`RetryDnsHandle`] is determined by the
+/// [`RetryableError`] trait.
 ///
 /// *note* Current value of this is not clear, it may be removed
 #[derive(Clone)]

crates/resolver/src/config.rs

@@ -385,14 +385,16 @@ pub struct NameServerConfig {
     /// SPKI name, only relevant for TLS connections
     #[cfg_attr(feature = "serde-config", serde(default))]
     pub tls_dns_name: Option<String>,
-    /// Default to not trust negative responses from upstream nameservers
+    /// Whether to trust `NXDOMAIN` responses from upstream nameservers.
     ///
-    /// When this is `false`, and an empty `NXDOMAIN` response is received, the
-    /// query will be retried against other configured name servers.
+    /// When this is `true`, and an empty `NXDOMAIN` response is received, the
+    /// query will not be retried against other configured name servers.
     ///
     /// (On an empty `NoError` response, or a response with any other error
-    /// response code, the query will be retried regardless of this
+    /// response code, the query will still be retried regardless of this
     /// configuration setting.)
+    ///
+    /// Defaults to false.
     #[cfg_attr(feature = "serde-config", serde(default))]
     pub trust_nx_responses: bool,
     #[cfg(feature = "dns-over-rustls")]

crates/resolver/src/error.rs

@@ -252,7 +252,14 @@ impl ResolveError {
 
 impl RetryableError for ResolveError {
     fn should_retry(&self) -> bool {
-        !matches!(self.kind(), ResolveErrorKind::NoRecordsFound { trusted, .. } if *trusted)
+        match self.kind() {
+            ResolveErrorKind::Message(_)
+            | ResolveErrorKind::Msg(_)
+            | ResolveErrorKind::NoRecordsFound { .. } => false,
+            ResolveErrorKind::Io(_) | ResolveErrorKind::Proto(_) | ResolveErrorKind::Timeout => {
+                true
+            }
+        }
     }
 
     fn attempted(&self) -> bool {

tests/integration-tests/tests/retry_dns_handle_tests.rs

@@ -0,0 +1,77 @@
+use std::sync::{
+    atomic::{AtomicU16, Ordering},
+    Arc,
+};
+
+use futures::{executor::block_on, future, stream, Stream};
+
+use trust_dns_proto::{
+    op::{Message, MessageType, OpCode, ResponseCode},
+    xfer::{DnsRequest, DnsResponse, FirstAnswer},
+    DnsHandle, RetryDnsHandle,
+};
+use trust_dns_resolver::error::ResolveError;
+
+#[derive(Clone)]
+struct TestClient {
+    retries: u16,
+    error_response: ResolveError,
+    attempts: Arc<AtomicU16>,
+}
+
+impl DnsHandle for TestClient {
+    type Response = Box<dyn Stream<Item = Result<DnsResponse, Self::Error>> + Send + Unpin>;
+    type Error = ResolveError;
+
+    fn send<R: Into<DnsRequest>>(&mut self, _: R) -> Self::Response {
+        let i = self.attempts.load(Ordering::SeqCst);
+
+        if i > self.retries || self.retries - i == 0 {
+            let mut message = Message::new();
+            message.set_id(i);
+            return Box::new(stream::once(future::ok(message.into())));
+        }
+
+        self.attempts.fetch_add(1, Ordering::SeqCst);
+        Box::new(stream::once(future::err(self.error_response.clone())))
+    }
+}
+
+// The RetryDnsHandle should retry the same nameserver on IO errors, e.g. timeouts.
+#[test]
+fn retry_on_retryable_error() {
+    let mut handle = RetryDnsHandle::new(
+        TestClient {
+            retries: 1,
+            error_response: ResolveError::from(std::io::Error::from(std::io::ErrorKind::TimedOut)),
+            attempts: Arc::new(AtomicU16::new(0)),
+        },
+        2,
+    );
+    let test1 = Message::new();
+    let result = block_on(handle.send(test1).first_answer()).expect("should have succeeded");
+    assert_eq!(result.id(), 1); // this is checking the number of iterations the TestClient ran
+}
+
+// The RetryDnsHandle should not retry the same name server(s) on a negative response, such as
+// `NODATA`.
+#[test]
+fn dont_retry_on_negative_response() {
+    let mut response = Message::new();
+    response
+        .set_message_type(MessageType::Response)
+        .set_op_code(OpCode::Update)
+        .set_response_code(ResponseCode::NoError);
+    let error =
+        ResolveError::from_response(response.into(), false).expect_err("NODATA should be an error");
+    let mut client = RetryDnsHandle::new(
+        TestClient {
+            retries: 1,
+            error_response: error,
+            attempts: Arc::new(AtomicU16::new(0)),
+        },
+        2,
+    );
+    let test1 = Message::new();
+    assert!(block_on(client.send(test1).first_answer()).is_err());
+}