From 4ff5f853200e87741a1aad7f380e7d3384d7314a Mon Sep 17 00:00:00 2001
From: sneak <sneak@sneak.berlin>
Date: Thu, 1 Oct 2026 22:29:23 +0000
Subject: [PATCH] resolver: try servers in a random order on each resolution
 (closes #138)

Every resolution walked the root servers in a fixed order, so
a.root-servers.net got every first query and its timeouts were paid on
every lookup. Each list of servers the resolver walks is now walked in a
random order from rand.Shuffle, chosen anew each time; a server that does
not reply, refuses, or gives an error reply or a referral that leads no
closer is still passed over for the next. When a referral names a zone's
nameservers without their addresses, all of them are now looked up, not
only the first that resolves, so the zone is not given up because one
randomly picked nameserver failed. No test fails if the walk stops
shuffling: which server a live query reached is not observable.

Model: opus-5-5
---
 README.md                           |  7 ++++++
 TODO.md                             |  3 ++-
 internal/resolver/export_test.go    | 21 ++++++++++++++++++
 internal/resolver/iterative.go      | 34 ++++++++++++++++++++++-------
 internal/resolver/iterative_test.go | 29 ++++++++++++++++++++++++
 internal/resolver/livedns_test.go   | 34 +++++++++++++++++++++++++++++
 internal/resolver/resolver_test.go  | 22 +++++++++++++++++++
 7 files changed, 141 insertions(+), 9 deletions(-)

diff --git a/README.md b/README.md
index d1ec4db..1850c65 100644
--- a/README.md
+++ b/README.md
@@ -382,6 +382,13 @@ performs full iterative resolution:
 4. **Authoritative query**: Queries all discovered authoritative nameservers
    directly for the requested records.
 
+In steps 2 and 3 the servers are asked one at a time in a random order, chosen
+anew each time, so no one root server gets every first query. A server that does
+not reply, refuses the query, or gives an error reply such as SERVFAIL or a
+referral that leads no closer to the name is passed over for the next one. When
+a referral names a zone's nameservers without their addresses, the addresses of
+all of them are looked up, so that each can be asked.
+
 This approach ensures:
 
 - Independence from any upstream resolver's cache or filtering.
diff --git a/TODO.md b/TODO.md
index 6b45c81..69d8e06 100644
--- a/TODO.md
+++ b/TODO.md
@@ -19,6 +19,8 @@ trial run of the finished image: https://git.eeqj.de/sneak/dnswatcher/issues/149
 
 # Completed Steps
 
+- 2026-10-02: the resolver tries root servers, and every other server list it
+  walks, in a random order each time, not always from the top (closes #138).
 - 2026-10-01: a certificate within the expiry warning period is warned about on
   every TLS check, where some checks used to skip it at random (closes #204).
 - 2026-10-01: a domain's NS set is its delegation from the parent zone's
@@ -128,5 +130,4 @@ trial run of the finished image: https://git.eeqj.de/sneak/dnswatcher/issues/149
 - 1.0 readiness: run it with a real config and read the logs:
   https://git.eeqj.de/sneak/dnswatcher/issues/66
 - README accuracy sweep: https://git.eeqj.de/sneak/dnswatcher/issues/108
-- fixed root server order: https://git.eeqj.de/sneak/dnswatcher/issues/138
 - review toward 1.0: https://git.eeqj.de/sneak/dnswatcher/issues/144
diff --git a/internal/resolver/export_test.go b/internal/resolver/export_test.go
index 5c59228..868174c 100644
--- a/internal/resolver/export_test.go
+++ b/internal/resolver/export_test.go
@@ -36,3 +36,24 @@ func (r *Resolver) QueryEachNS(
 ) (map[string]*NameserverResponse, error) {
 	return r.queryEachNS(ctx, nameservers, hostname)
 }
+
+// ResolveNSIPs exports resolveNSIPs for testing.
+func (r *Resolver) ResolveNSIPs(
+	ctx context.Context,
+	nsNames []string,
+) []string {
+	return r.resolveNSIPs(ctx, nsNames)
+}
+
+// RootServerList exports rootServerList for testing.
+func RootServerList() []string {
+	return rootServerList()
+}
+
+// Shuffled exports shuffled for testing.
+func Shuffled(
+	servers []string,
+	shuffle func(n int, swap func(i, j int)),
+) []string {
+	return shuffled(servers, shuffle)
+}
diff --git a/internal/resolver/iterative.go b/internal/resolver/iterative.go
index d495193..0c526e5 100644
--- a/internal/resolver/iterative.go
+++ b/internal/resolver/iterative.go
@@ -4,7 +4,9 @@ import (
 	"context"
 	"errors"
 	"fmt"
+	"math/rand/v2"
 	"net"
+	"slices"
 	"sort"
 	"strings"
 	"time"
@@ -259,9 +261,25 @@ func (r *Resolver) followDelegation(
 	return nil, ErrNoNameservers
 }
 
-// queryServers asks servers, the servers of zone, about name until one
-// gives a usable reply. A server that times out, refuses or gives a
-// reply that is not usable is passed over for the next.
+// shuffled returns a copy of servers in the order shuffle puts them
+// in. The resolver passes rand.Shuffle, so each time it walks a list of
+// servers it starts at a random one, and no one server gets every
+// first query.
+func shuffled(
+	servers []string,
+	shuffle func(n int, swap func(i, j int)),
+) []string {
+	order := slices.Clone(servers)
+	shuffle(len(order), func(i, j int) {
+		order[i], order[j] = order[j], order[i]
+	})
+
+	return order
+}
+
+// queryServers asks servers, the servers of zone, about name in a random
+// order until one gives a usable reply. A server that times out, refuses
+// or gives a reply that is not usable is passed over for the next.
 func (r *Resolver) queryServers(
 	ctx context.Context,
 	servers []string,
@@ -271,7 +289,7 @@ func (r *Resolver) queryServers(
 ) (*dns.Msg, error) {
 	var lastErr error
 
-	for _, ip := range servers {
+	for _, ip := range shuffled(servers, rand.Shuffle) {
 		if checkCtx(ctx) != nil {
 			return nil, ErrContextCanceled
 		}
@@ -340,6 +358,10 @@ func nsSetFrom(resp *dns.Msg, domain string) []string {
 	return extractNSSet(resp.Answer)
 }
 
+// resolveNSIPs returns the addresses of every nameserver in nsNames
+// whose name resolves, for a referral that carries none. The walk can
+// then go on to the zone's other nameservers when one gives no usable
+// reply.
 func (r *Resolver) resolveNSIPs(
 	ctx context.Context,
 	nsNames []string,
@@ -351,10 +373,6 @@ func (r *Resolver) resolveNSIPs(
 		if err == nil {
 			ips = append(ips, resolved...)
 		}
-
-		if len(ips) > 0 {
-			break
-		}
 	}
 
 	return ips
diff --git a/internal/resolver/iterative_test.go b/internal/resolver/iterative_test.go
index 413cc71..4f539ae 100644
--- a/internal/resolver/iterative_test.go
+++ b/internal/resolver/iterative_test.go
@@ -1,6 +1,8 @@
 package resolver_test
 
 import (
+	"math/rand/v2"
+	"slices"
 	"testing"
 
 	"github.com/miekg/dns"
@@ -235,3 +237,30 @@ func TestExtractRecordValue_LetterCase(t *testing.T) {
 		})
 	}
 }
+
+// TestShuffled shuffles the root servers with many seeds. Every order
+// must hold each root server once, so each is tried before a
+// resolution fails; each root server must come first for some seed, so
+// no one root server gets every first query; and the list passed in
+// must be left as it was.
+func TestShuffled(t *testing.T) {
+	t.Parallel()
+
+	const seeds = 1000
+
+	roots := resolver.RootServerList()
+	before := slices.Clone(roots)
+	first := make(map[string]bool)
+
+	for seed := range uint64(seeds) {
+		rng := rand.New(rand.NewPCG(seed, 0)) //nolint:gosec // seeded on purpose
+		order := resolver.Shuffled(roots, rng.Shuffle)
+
+		assert.ElementsMatch(t, roots, order)
+
+		first[order[0]] = true
+	}
+
+	assert.Len(t, first, len(roots))
+	assert.Equal(t, before, roots)
+}
diff --git a/internal/resolver/livedns_test.go b/internal/resolver/livedns_test.go
index fb425a8..0bbccb4 100644
--- a/internal/resolver/livedns_test.go
+++ b/internal/resolver/livedns_test.go
@@ -383,3 +383,37 @@ func liveResolveIPsAllowingEmpty(
 
 	return out
 }
+
+// liveResolveNSIPs looks up the addresses of the nameservers named
+// names, retrying until there are at least atLeast of them: a name
+// whose lookup got no reply is left out of the result, not an error.
+func liveResolveNSIPs(
+	t *testing.T,
+	r *resolver.Resolver,
+	names []string,
+	atLeast int,
+) []string {
+	t.Helper()
+
+	var out []string
+
+	livednstest.Retry(
+		t,
+		"ResolveNSIPs("+strings.Join(names, ", ")+")",
+		func(ctx context.Context) error {
+			ips := r.ResolveNSIPs(ctx, names)
+			if len(ips) < atLeast {
+				return fmt.Errorf(
+					"%w: %d addresses, expected at least %d",
+					livednstest.ErrNoAnswer, len(ips), atLeast,
+				)
+			}
+
+			out = ips
+
+			return nil
+		},
+	)
+
+	return out
+}
diff --git a/internal/resolver/resolver_test.go b/internal/resolver/resolver_test.go
index dd585c8..abb6b92 100644
--- a/internal/resolver/resolver_test.go
+++ b/internal/resolver/resolver_test.go
@@ -139,6 +139,28 @@ func TestFindAuthoritativeNameservers_CloudflareDomain(
 	}
 }
 
+// TestResolveNSIPs_EveryNameserver looks up the addresses of two of
+// google.com's nameservers together, as the walk does when a referral
+// names a zone's nameservers without their addresses, and compares them
+// with each looked up alone. Together they must give the addresses of
+// both, not only of the first that resolves, so that when one gives no
+// usable reply the walk goes on to the other.
+func TestResolveNSIPs_EveryNameserver(t *testing.T) {
+	t.Parallel()
+
+	r := newTestResolver(t)
+	names := []string{"ns3.google.com.", "ns4.google.com."}
+	want := make([]string, 0, len(names))
+
+	for _, name := range names {
+		want = append(want, liveResolveNSIPs(t, r, []string{name}, 1)...)
+	}
+
+	got := liveResolveNSIPs(t, r, names, len(want))
+
+	assert.ElementsMatch(t, want, got)
+}
+
 // ----------------------------------------------------------------
 // QueryNameserver tests
 // ----------------------------------------------------------------