TESTING.md

@@ -39,6 +39,42 @@ assertions and sensible timeouts, not from mocks.
   running at once the gates sum instead of holding, and the resolver's
   per-attempt deadlines start expiring
 
+### Transport failures: loopback nameservers, not mocks
+
+The resolver classifies a nameserver that stays silent as
+`StatusTimeout` and one that answers SERVFAIL as `StatusError`. The
+public network cannot be made to produce either on demand — a
+black-holed address is only black-holed on some networks, and build
+environments that transparently intercept UDP/53 answer it locally —
+so a test built on a chosen remote address asserts on the network it
+happens to run on rather than on the resolver.
+
+`internal/resolver/transport_test.go` binds a real nameserver on
+`127.0.0.1` instead and points the query at it.
+`nameserverAddr` dials an address that already carries a port as
+written, so no production behaviour is bypassed to arrange this.
+
+**This is permitted, and it is not a mock.** The rule above bans
+substituting `DNSClient` or any other DNS abstraction, which lets the
+code under test skip DNS and hands it a manufactured verdict. A
+loopback nameserver does the opposite: the resolver dials a real
+socket, writes a real query with the real `miekg/dns` client, and
+applies its real deadline and its real classification logic to what
+comes back. Choosing which nameserver a live query is sent to is not
+faking DNS — the resolver is aimed at a nameserver of the caller's
+choosing in production too.
+
+The distinction to hold on to: **substituting the client is banned;
+choosing the server is not.** A test that reaches for a fake
+`DNSClient` to force a classification is still forbidden, no matter
+how awkward the alternative looks.
+
+Such a test must stay cheap. The resolver asks for eight record
+types and retries each once, so a nameserver silent on every type
+costs sixteen query timeouts. `TestQueryNameserverIP_Timeout` is
+silent on `A` alone and answers the rest, which is all the resolver
+needs to classify the response and keeps the test to two.
+
 ### What NOT to do
 
 - **Do not mock `DNSClient`**, the watcher's `DNSResolver` interface,

TODO.md

@@ -26,6 +26,22 @@ Rationale, Design, TODO, License, Author) if any are still missing.
 
 # Completed Steps
 
+- 2026-09-03: restored the transport-failure classification coverage
+  the DNS-mock removal had dropped, and tightened two over-tolerant
+  watcher assertions. `internal/resolver/transport_test.go` covers
+  `StatusTimeout`, `StatusError` and the connection-refused path by
+  binding real nameservers on loopback rather than by mocking
+  `DNSClient` or by aiming a query at a remote address and hoping the
+  network black-holes it; `queryDNS` now honours a port already
+  present in a nameserver address, which is what lets a query be
+  aimed at one. The watcher's `assertStatePopulated` and
+  `TestDomainPortAndTLSChecks` now assert that port and certificate
+  state, and the arguments the port and TLS checkers were called
+  with, match the addresses live DNS returned — previously they
+  asserted only that those sets were non-empty, which would not have
+  caught resolving the wrong addresses. `TESTING.md` records why a
+  loopback nameserver is not a mock.
+
 - 2026-08-10: comment-only corrections to `script/bootstrap`,
   `script/cibuild`, and `Dockerfile.lint`. The `goimports` pin in
   `script/bootstrap` was justified by a claim that `script/fmt-check`

internal/resolver/iterative.go

@@ -20,6 +20,10 @@ const (
 	minDomainLabels      = 2
 )
 
+// defaultDNSPort is the port a nameserver is assumed to listen on
+// when its address does not carry one.
+const defaultDNSPort = "53"
+
 // ErrRefused is returned when a DNS server refuses a query.
 var ErrRefused = errors.New("dns query refused")
 
@@ -106,6 +110,20 @@ func (r *Resolver) retryTCP(
 	return resp
 }
 
+// nameserverAddr renders a nameserver address for dialling. A bare
+// address — the normal case, and what a delegation's glue records
+// carry — is given the default DNS port. An address that already
+// specifies a port is dialled as written, which is what makes a
+// nameserver listening somewhere other than 53 reachable.
+func nameserverAddr(nsIP string) string {
+	_, _, err := net.SplitHostPort(nsIP)
+	if err == nil {
+		return nsIP
+	}
+
+	return net.JoinHostPort(nsIP, defaultDNSPort)
+}
+
 // queryDNS sends a DNS query to a specific server IP.
 // Tries non-recursive first, falls back to recursive on
 // REFUSED (handles DNS interception environments).
@@ -120,7 +138,7 @@ func (r *Resolver) queryDNS(
 	}
 
 	name = dns.Fqdn(name)
-	addr := net.JoinHostPort(serverIP, "53")
+	addr := nameserverAddr(serverIP)
 
 	msg := new(dns.Msg)
 	msg.SetQuestion(name, qtype)

internal/resolver/resolver_test.go

@@ -517,15 +517,9 @@ func TestQueryAllNameservers_ContextCanceled(t *testing.T) {
 	assert.Error(t, err)
 }
 
-// The resolver's transport-failure classification (StatusTimeout /
-// StatusError for a nameserver that does not answer) is deliberately
-// not covered here. Forcing it required the mock DNSClient this
-// change removes, and a live substitute is not available: the build
-// environment transparently intercepts all UDP/53 traffic and answers
-// it locally, so a query to a black-holed address such as an RFC 5737
-// documentation address comes back StatusOK with real records. See
-// TESTING.md; restoring this coverage needs a mechanism that is
-// neither a mock nor dependent on the sandbox's network behaviour.
+// Transport-failure classification (StatusTimeout / StatusError)
+// is covered in transport_test.go, against real nameservers bound on
+// loopback.
 
 func TestResolveIPAddresses_ContextCanceled(t *testing.T) {
 	t.Parallel()

internal/resolver/transport_test.go

@@ -0,0 +1,259 @@
+package resolver_test
+
+// Transport-failure classification tests.
+//
+// These are live tests, not mocks. Nothing here substitutes the
+// resolver's DNSClient: the resolver dials a real UDP socket, writes
+// a real DNS query with the real miekg/dns client, and applies its
+// real deadline and its real classification logic to what comes
+// back. The only thing under test control is which address the query
+// is sent to, and what — if anything — is listening there.
+//
+// That distinction is what the no-DNS-mocks rule in TESTING.md is
+// about. A fake DNSClient lets the code under test skip DNS entirely
+// and hands it a manufactured verdict; a nameserver bound on
+// loopback makes it speak DNS for real and earn one. Pointing a live
+// query at a nameserver of the test's choosing is no more a mock
+// than pointing it at a.root-servers.net.
+//
+// The public network cannot produce these outcomes on demand. A
+// black-holed address is not black-holed everywhere — build
+// environments that intercept UDP/53 answer it locally — so a test
+// built on one asserts on the network it happens to run on rather
+// than on the resolver. A loopback nameserver is deterministic
+// everywhere, and it is fast, because the test picks the deadline.
+
+import (
+	"context"
+	"net"
+	"testing"
+	"time"
+
+	"github.com/miekg/dns"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+
+	"sneak.berlin/go/dnswatcher/internal/resolver"
+)
+
+const (
+	// transportBudget is the wall time the timeout test must stay
+	// under. The resolver asks a nameserver for eight record types
+	// in turn and retries each one once, so a nameserver silent on
+	// every type would cost sixteen query timeouts. The test's
+	// nameserver is silent on exactly one type, which costs two,
+	// and this budget fails loudly if that ever stops being true.
+	transportBudget = 8 * time.Second
+
+	// transportDeadline is the caller deadline the tests run
+	// under. It is generous on purpose: these tests are about the
+	// resolver classifying a nameserver's behaviour, so the
+	// caller's deadline must never be the thing that expires.
+	transportDeadline = 30 * time.Second
+
+	// silentNS and failingNS are the nameserver names reported
+	// back in NameserverResponse.Nameserver. They are .test names
+	// (RFC 6761) and are never resolved: the tests address the
+	// nameserver by its socket address.
+	silentNS  = "silent.ns.test."
+	failingNS = "servfail.ns.test."
+
+	// transportHostname is the name queried. Nothing resolves it;
+	// the point is entirely how the nameserver behaves.
+	transportHostname = "example.com"
+)
+
+// startNameserver binds a real UDP nameserver on loopback and serves
+// every datagram it receives with handle, which returns the reply to
+// send or nil to stay silent. It returns the "host:port" address to
+// aim a query at, and stops the server when the test ends.
+func startNameserver(
+	t *testing.T,
+	handle func(query *dns.Msg) *dns.Msg,
+) string {
+	t.Helper()
+
+	var lc net.ListenConfig
+
+	conn, err := lc.ListenPacket(t.Context(), "udp", "127.0.0.1:0")
+	require.NoError(t, err, "binding loopback nameserver")
+
+	stopped := make(chan struct{})
+
+	t.Cleanup(func() {
+		_ = conn.Close()
+
+		<-stopped
+	})
+
+	go serveNameserver(conn, handle, stopped)
+
+	return conn.LocalAddr().String()
+}
+
+// serveNameserver reads queries until conn is closed, replying with
+// whatever handle produces.
+func serveNameserver(
+	conn net.PacketConn,
+	handle func(query *dns.Msg) *dns.Msg,
+	stopped chan<- struct{},
+) {
+	defer close(stopped)
+
+	buf := make([]byte, dns.MaxMsgSize)
+
+	for {
+		n, from, err := conn.ReadFrom(buf)
+		if err != nil {
+			return
+		}
+
+		query := new(dns.Msg)
+		if query.Unpack(buf[:n]) != nil {
+			continue
+		}
+
+		reply := handle(query)
+		if reply == nil {
+			continue
+		}
+
+		wire, err := reply.Pack()
+		if err != nil {
+			continue
+		}
+
+		_, err = conn.WriteTo(wire, from)
+		if err != nil {
+			return
+		}
+	}
+}
+
+// unservedAddr returns a loopback address with nothing listening on
+// it, by binding a port and releasing it again.
+func unservedAddr(t *testing.T) string {
+	t.Helper()
+
+	var lc net.ListenConfig
+
+	conn, err := lc.ListenPacket(t.Context(), "udp", "127.0.0.1:0")
+	require.NoError(t, err, "binding loopback port")
+
+	addr := conn.LocalAddr().String()
+	require.NoError(t, conn.Close(), "releasing loopback port")
+
+	return addr
+}
+
+// TestQueryNameserverIP_Timeout covers the StatusTimeout branch: a
+// nameserver that takes the query and never answers.
+func TestQueryNameserverIP_Timeout(t *testing.T) {
+	t.Parallel()
+
+	// A real nameserver that drops A queries and answers every
+	// other type. Silence on one type is all the resolver needs to
+	// classify the response as a timeout, and it keeps the test
+	// two query timeouts long instead of sixteen.
+	addr := startNameserver(t, func(query *dns.Msg) *dns.Msg {
+		if len(query.Question) > 0 &&
+			query.Question[0].Qtype == dns.TypeA {
+			return nil
+		}
+
+		reply := new(dns.Msg)
+		reply.SetReply(query)
+
+		return reply
+	})
+
+	r := newTestResolver(t)
+
+	ctx, cancel := context.WithTimeout(
+		context.Background(), transportDeadline,
+	)
+	defer cancel()
+
+	start := time.Now()
+
+	resp, err := r.QueryNameserverIP(
+		ctx, silentNS, addr, transportHostname,
+	)
+
+	elapsed := time.Since(start)
+
+	require.NoError(t, err)
+	require.NotNil(t, resp)
+
+	assert.Equal(t, resolver.StatusTimeout, resp.Status)
+	assert.Equal(t, "all queries timed out", resp.Error)
+	assert.Empty(t, resp.Records)
+	assert.Equal(t, silentNS, resp.Nameserver)
+
+	assert.Less(
+		t, elapsed, transportBudget,
+		"one silent record type must cost one query's retries, "+
+			"not every record type's",
+	)
+}
+
+// TestQueryNameserverIP_ServFail covers the StatusError branch: a
+// nameserver that answers, and answers SERVFAIL.
+func TestQueryNameserverIP_ServFail(t *testing.T) {
+	t.Parallel()
+
+	addr := startNameserver(t, func(query *dns.Msg) *dns.Msg {
+		reply := new(dns.Msg)
+		reply.SetRcode(query, dns.RcodeServerFailure)
+
+		return reply
+	})
+
+	r := newTestResolver(t)
+
+	ctx, cancel := context.WithTimeout(
+		context.Background(), transportDeadline,
+	)
+	defer cancel()
+
+	resp, err := r.QueryNameserverIP(
+		ctx, failingNS, addr, transportHostname,
+	)
+
+	require.NoError(t, err)
+	require.NotNil(t, resp)
+
+	assert.Equal(t, resolver.StatusError, resp.Status)
+	assert.Equal(t, "server returned SERVFAIL", resp.Error)
+	assert.Empty(t, resp.Records)
+	assert.Equal(t, failingNS, resp.Nameserver)
+}
+
+// TestQueryNameserverIP_NoListener pins the third transport outcome:
+// a refused datagram is not a timeout. The socket fails immediately
+// with ECONNREFUSED rather than going quiet, so isTimeout is false,
+// no failure flag is set, and the response classifies as NoData.
+// Asserting it here is what stops that path being mistaken for the
+// timeout path, in either direction.
+func TestQueryNameserverIP_NoListener(t *testing.T) {
+	t.Parallel()
+
+	addr := unservedAddr(t)
+
+	r := newTestResolver(t)
+
+	ctx, cancel := context.WithTimeout(
+		context.Background(), transportDeadline,
+	)
+	defer cancel()
+
+	resp, err := r.QueryNameserverIP(
+		ctx, silentNS, addr, transportHostname,
+	)
+
+	require.NoError(t, err)
+	require.NotNil(t, resp)
+
+	assert.Equal(t, resolver.StatusNoData, resp.Status)
+	assert.Empty(t, resp.Records)
+}

internal/watcher/watcher_test.go

@@ -6,6 +6,7 @@ import (
 	"fmt"
 	"log/slog"
 	"os"
+	"slices"
 	"sort"
 	"strings"
 	"sync"
@@ -53,6 +54,12 @@ const (
 	longLife  = 90 * 24 * time.Hour
 	shortLife = 3 * 24 * time.Hour
 
+	// monitoredTestPorts and tlsCheckPort mirror the ports the
+	// watcher checks. They are duplicated here deliberately: a
+	// test that imported the production values would agree with
+	// the watcher no matter which ports it drifted to.
+	tlsCheckPort = 443
+
 	pollInterval = 100 * time.Millisecond
 	scanTimeout  = 25 * time.Second
 	notifyGrace  = 500 * time.Millisecond
@@ -64,22 +71,31 @@ var errNotFound = errors.New("not found")
 
 // --- Test doubles for non-DNS collaborators ---
 
+// portCall records one CheckPort invocation. The arguments matter,
+// not just the count: a watcher that dutifully checks ports on the
+// wrong addresses produces exactly the same call count as one that
+// resolved correctly, so the tests assert what was checked.
+type portCall struct {
+	address string
+	port    int
+}
+
 type mockPortChecker struct {
 	mu      sync.Mutex
 	openAll bool
 	err     error
-	calls   int
+	seen    []portCall
 }
 
 func (m *mockPortChecker) CheckPort(
 	_ context.Context,
-	_ string,
-	_ int,
+	address string,
+	port int,
 ) (*portcheck.PortResult, error) {
 	m.mu.Lock()
 	defer m.mu.Unlock()
 
-	m.calls++
+	m.seen = append(m.seen, portCall{address: address, port: port})
 
 	if m.err != nil {
 		return nil, m.err
@@ -95,29 +111,48 @@ func (m *mockPortChecker) setOpenAll(open bool) {
 	m.openAll = open
 }
 
-func (m *mockPortChecker) callCount() int {
+// checkedKeys returns the distinct "address:port" pairs the checker
+// was asked about, sorted, in the same form as the state store's
+// port keys so the two can be compared directly.
+func (m *mockPortChecker) checkedKeys() []string {
 	m.mu.Lock()
 	defer m.mu.Unlock()
 
-	return m.calls
+	keys := make([]string, 0, len(m.seen))
+	for _, c := range m.seen {
+		keys = append(
+			keys, fmt.Sprintf("%s:%d", c.address, c.port),
+		)
+	}
+
+	return sortedSet(keys)
+}
+
+// tlsCall records one CheckCertificate invocation, for the same
+// reason portCall records CheckPort's arguments.
+type tlsCall struct {
+	address  string
+	hostname string
 }
 
 type mockTLSChecker struct {
 	mu   sync.Mutex
 	cert *tlscheck.CertificateInfo
 	err  error
-	calls int
+	seen []tlsCall
 }
 
 func (m *mockTLSChecker) CheckCertificate(
 	_ context.Context,
-	_ string,
+	address string,
 	hostname string,
 ) (*tlscheck.CertificateInfo, error) {
 	m.mu.Lock()
 	defer m.mu.Unlock()
 
-	m.calls++
+	m.seen = append(
+		m.seen, tlsCall{address: address, hostname: hostname},
+	)
 
 	if m.err != nil {
 		return nil, m.err
@@ -139,11 +174,21 @@ func (m *mockTLSChecker) setCert(cert *tlscheck.CertificateInfo) {
 	m.cert = cert
 }
 
-func (m *mockTLSChecker) callCount() int {
+// checkedKeys returns the distinct certificate keys the checker was
+// asked about, sorted, in the state store's "address:port:hostname"
+// form so the two can be compared directly.
+func (m *mockTLSChecker) checkedKeys() []string {
 	m.mu.Lock()
 	defer m.mu.Unlock()
 
-	return m.calls
+	keys := make([]string, 0, len(m.seen))
+	for _, c := range m.seen {
+		keys = append(keys, fmt.Sprintf(
+			"%s:%d:%s", c.address, tlsCheckPort, c.hostname,
+		))
+	}
+
+	return sortedSet(keys)
 }
 
 type notification struct {
@@ -367,6 +412,116 @@ func liveIPs(snap state.Snapshot, hostname string) []string {
 	return ips
 }
 
+// monitoredTestPorts is the set of ports the watcher is expected to
+// check on every resolved address.
+func monitoredTestPorts() []int {
+	return []int{80, tlsCheckPort}
+}
+
+// sortedSet deduplicates and sorts, so that call records and state
+// keys can be compared as sets.
+func sortedSet(values []string) []string {
+	seen := make(map[string]bool, len(values))
+
+	out := make([]string, 0, len(values))
+
+	for _, v := range values {
+		if seen[v] {
+			continue
+		}
+
+		seen[v] = true
+
+		out = append(out, v)
+	}
+
+	sort.Strings(out)
+
+	return out
+}
+
+// watchedNames is every name the config puts under observation.
+// Domains get hostname state too, for their port and TLS checks.
+func watchedNames(cfg *config.Config) []string {
+	names := make([]string, 0, len(cfg.Hostnames)+len(cfg.Domains))
+	names = append(names, cfg.Hostnames...)
+	names = append(names, cfg.Domains...)
+
+	return sortedSet(names)
+}
+
+// stateHostnames is the set of names the run actually recorded.
+func stateHostnames(snap state.Snapshot) []string {
+	names := make([]string, 0, len(snap.Hostnames))
+	for name := range snap.Hostnames {
+		names = append(names, name)
+	}
+
+	return sortedSet(names)
+}
+
+// expectedPortKeys is every "address:port" the watcher should have
+// touched: the monitored ports on every address live DNS resolved
+// for every watched name, and nothing else.
+func expectedPortKeys(
+	snap state.Snapshot,
+	names []string,
+) []string {
+	var keys []string
+
+	for _, name := range names {
+		for _, ip := range liveIPs(snap, name) {
+			for _, port := range monitoredTestPorts() {
+				keys = append(
+					keys, fmt.Sprintf("%s:%d", ip, port),
+				)
+			}
+		}
+	}
+
+	return sortedSet(keys)
+}
+
+// expectedCertKeys is every "address:port:hostname" the watcher
+// should have a certificate for, given that every port reports open.
+func expectedCertKeys(
+	snap state.Snapshot,
+	names []string,
+) []string {
+	var keys []string
+
+	for _, name := range names {
+		for _, ip := range liveIPs(snap, name) {
+			keys = append(keys, fmt.Sprintf(
+				"%s:%d:%s", ip, tlsCheckPort, name,
+			))
+		}
+	}
+
+	return sortedSet(keys)
+}
+
+// stateKeys sorts a state map's keys for set comparison.
+func stateKeys[V any](m map[string]V) []string {
+	keys := make([]string, 0, len(m))
+	for k := range m {
+		keys = append(keys, k)
+	}
+
+	return sortedSet(keys)
+}
+
+// assertSameSet fails with both sides named when they differ.
+func assertSameSet(t *testing.T, what string, want, got []string) {
+	t.Helper()
+
+	if !slices.Equal(want, got) {
+		t.Errorf(
+			"%s: expected %v, got %v", what, want, got,
+		)
+	}
+}
+
 // --- Tests ---
 
 func TestFirstRunBaseline(t *testing.T) {
@@ -415,16 +570,33 @@ func assertStatePopulated(
 		)
 	}
 
-	// Hostnames includes both explicit hostnames and domains
-	// (domains now also get hostname state for port/TLS checks).
-	if len(snap.Hostnames) < 1 {
-		t.Errorf(
-			"expected at least 1 hostname in state, got %d",
-			len(snap.Hostnames),
+	// Hostname state covers both explicit hostnames and domains
+	// (domains also get hostname state for port/TLS checks), and
+	// covers exactly those: a name in state that nothing asked
+	// for, or a configured name missing from it, is a bug.
+	names := watchedNames(deps.config)
+	assertSameSet(
+		t, "hostnames in state", names, stateHostnames(snap),
 	)
+
+	// Every watched name must have resolved to something, or the
+	// key comparisons below would pass vacuously on empty sets.
+	for _, name := range names {
+		if len(liveIPs(snap, name)) == 0 {
+			t.Errorf("no addresses resolved for %s", name)
 		}
 	}
 
+	// The addresses the port checker was aimed at must be the
+	// addresses live DNS returned. Counting calls would not
+	// notice the watcher checking the wrong hosts.
+	assertSameSet(
+		t, "ports checked",
+		expectedPortKeys(snap, names),
+		deps.portChecker.checkedKeys(),
+	)
+}
+
 func TestDomainPortAndTLSChecks(t *testing.T) {
 	t.Parallel()
 
@@ -438,24 +610,34 @@ func TestDomainPortAndTLSChecks(t *testing.T) {
 
 	snap := deps.state.GetSnapshot()
 
-	// The domain resolved via live DNS should have port state
-	// populated for its real addresses.
-	if len(snap.Ports) == 0 {
-		t.Error("expected port state for domain, got none")
+	names := watchedNames(cfg)
+
+	ips := liveIPs(snap, testDomain)
+	if len(ips) == 0 {
+		t.Fatal("live DNS resolved no addresses for " + testDomain)
 	}
 
-	// Domain should have certificate state populated.
-	if len(snap.Certificates) == 0 {
-		t.Error("expected certificate state for domain, got none")
-	}
+	// Port and certificate state must be keyed by the addresses
+	// live DNS actually returned — every one of them, and no
+	// others. Asserting only that the maps are non-empty would
+	// hold just as well if the watcher had resolved the wrong
+	// name or dropped all but one of its addresses.
+	wantPorts := expectedPortKeys(snap, names)
+	assertSameSet(t, "port state", wantPorts, stateKeys(snap.Ports))
+	assertSameSet(
+		t, "ports checked", wantPorts,
+		deps.portChecker.checkedKeys(),
+	)
 
-	if deps.portChecker.callCount() == 0 {
-		t.Error("expected port checker to be called for domain")
-	}
-
-	if deps.tlsChecker.callCount() == 0 {
-		t.Error("expected TLS checker to be called for domain")
-	}
+	wantCerts := expectedCertKeys(snap, names)
+	assertSameSet(
+		t, "certificate state", wantCerts,
+		stateKeys(snap.Certificates),
+	)
+	assertSameSet(
+		t, "certificates checked", wantCerts,
+		deps.tlsChecker.checkedKeys(),
+	)
 }
 
 func TestNSChangeDetection(t *testing.T) {