resolver, watcher: a record type whose query fails keeps its previous records (closes #231)

The resolver lists in FailedTypes each record type whose query to a
nameserver got no usable reply: none after two tries, an error reply, a
referral, or a truncated reply whose TCP retry failed. Records holds
nothing for such a type, never none or the part that fit. A nameserver
that answered no type has failed, as before. The watcher keeps the
previous check's records for a failed type; when that check did not know
them either (first check, new or failing nameserver), the type is saved
in failedTypes and left out of record and inconsistency comparisons.
ResolveIPAddresses no longer counts a nameserver whose A, AAAA or CNAME
query failed as an answer.

Model: opus-5-5

internal/watcher/export_test.go

@@ -79,7 +79,8 @@ func (w *Watcher) RunTLSChecks(ctx context.Context) {
 // BuildHostnameState exports buildHostnameState for testing.
 func BuildHostnameState(
 	results map[string]*resolver.NameserverResponse,
+	prev *state.HostnameState,
 	now time.Time,
 ) *state.HostnameState {
-	return buildHostnameState(results, now)
+	return buildHostnameState(results, prev, now)
 }

internal/watcher/failedtype_test.go

@@ -0,0 +1,213 @@
+package watcher_test
+
+import (
+	"maps"
+	"slices"
+	"testing"
+	"time"
+
+	"sneak.berlin/go/dnswatcher/internal/resolver"
+	"sneak.berlin/go/dnswatcher/internal/state"
+	"sneak.berlin/go/dnswatcher/internal/watcher"
+)
+
+const (
+	// txt is the record type whose query fails in these tests.
+	txt  = "TXT"
+	spf1 = "v=spf1 -all"
+	spf2 = "v=spf1 include:example.net -all"
+)
+
+// response is a nameserver's response with these records, whose queries
+// for failedTypes failed.
+func response(
+	records map[string][]string,
+	failedTypes ...string,
+) *resolver.NameserverResponse {
+	return &resolver.NameserverResponse{
+		Records:     records,
+		FailedTypes: failedTypes,
+		Status:      resolver.StatusOK,
+	}
+}
+
+// savedChecks saves the state of each check in turn from the
+// nameservers' responses, each from the state the check before saved.
+func savedChecks(
+	checks ...map[string]*resolver.NameserverResponse,
+) []*state.HostnameState {
+	states := make([]*state.HostnameState, 0, len(checks))
+
+	var prev *state.HostnameState
+
+	for _, results := range checks {
+		prev = watcher.BuildHostnameState(results, prev, time.Now())
+		states = append(states, prev)
+	}
+
+	return states
+}
+
+// TestFailedTypeKeepsPreviousRecords saves a check in which nsA's query
+// for TXT failed, after previous checks of several kinds.
+func TestFailedTypeKeepsPreviousRecords(t *testing.T) {
+	t.Parallel()
+
+	aOnly := map[string][]string{"A": {ip1}}
+	withTXT := map[string][]string{"A": {ip1}, txt: {spf1}}
+	txtNotKnown := &state.NameserverRecordState{
+		Records: aOnly, FailedTypes: []string{txt}, Status: "ok",
+	}
+
+	tests := []struct {
+		name        string
+		prev        *state.HostnameState
+		wantRecords map[string][]string
+		wantFailed  []string
+	}{
+		{
+			"previous TXT records are kept",
+			saved(map[string]*state.NameserverRecordState{
+				nsA: answered(withTXT),
+			}),
+			withTXT, nil,
+		},
+		{
+			"previous check had no TXT records",
+			saved(map[string]*state.NameserverRecordState{
+				nsA: answered(aOnly),
+			}),
+			aOnly, nil,
+		},
+		{"first check", nil, aOnly, []string{txt}},
+		{
+			"nameserver new on this check",
+			saved(map[string]*state.NameserverRecordState{
+				nsB: answered(withTXT),
+			}),
+			aOnly, []string{txt},
+		},
+		{
+			"nameserver failed on the previous check",
+			saved(map[string]*state.NameserverRecordState{nsA: failed()}),
+			aOnly, []string{txt},
+		},
+		{
+			"TXT failed on the previous check too",
+			saved(map[string]*state.NameserverRecordState{
+				nsA: txtNotKnown,
+			}),
+			aOnly, []string{txt},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+
+			hs := watcher.BuildHostnameState(
+				map[string]*resolver.NameserverResponse{
+					nsA: response(map[string][]string{"A": {ip1}}, txt),
+				},
+				tt.prev, time.Now(),
+			)
+
+			got := hs.RecordsByNameserver[nsA]
+			if got.Status != "ok" ||
+				!maps.EqualFunc(got.Records, tt.wantRecords, slices.Equal) ||
+				!slices.Equal(got.FailedTypes, tt.wantFailed) {
+				t.Errorf(
+					"saved status %q, records %v, failed types %v; "+
+						"want ok, %v, %v",
+					got.Status, got.Records, got.FailedTypes,
+					tt.wantRecords, tt.wantFailed,
+				)
+			}
+		})
+	}
+}
+
+// TestFailedTypeAlerts saves the checks of each case in turn from the
+// nameservers' responses, the first being the state loaded at startup,
+// and counts the alerts sent.
+func TestFailedTypeAlerts(t *testing.T) {
+	t.Parallel()
+
+	records := map[string][]string{"A": {ip1}, txt: {spf1}}
+	changed := map[string][]string{"A": {ip1}, txt: {spf2}}
+	aOnly := map[string][]string{"A": {ip1}}
+
+	bothAnswer := map[string]*resolver.NameserverResponse{
+		nsA: response(records), nsB: response(records),
+	}
+	bTXTFails := map[string]*resolver.NameserverResponse{
+		nsA: response(records), nsB: response(aOnly, txt),
+	}
+	onlyA := map[string]*resolver.NameserverResponse{
+		nsA: response(records),
+	}
+	bFails := map[string]*resolver.NameserverResponse{
+		nsA: response(records),
+		nsB: {
+			Records: map[string][]string{},
+			Status:  resolver.StatusTimeout,
+			Error:   "all queries timed out",
+		},
+	}
+	bothChange := map[string]*resolver.NameserverResponse{
+		nsA: response(changed), nsB: response(changed),
+	}
+
+	tests := []struct {
+		name   string
+		checks []map[string]*resolver.NameserverResponse
+		want   alertCounts
+	}{
+		{
+			"type failing at one nameserver alerts nothing, nor its next answer",
+			[]map[string]*resolver.NameserverResponse{
+				bothAnswer, bTXTFails, bothAnswer,
+			},
+			alertCounts{},
+		},
+		{
+			"type failing on the first check alerts nothing on the next",
+			[]map[string]*resolver.NameserverResponse{bTXTFails, bothAnswer},
+			alertCounts{},
+		},
+		{
+			"type failing at a nameserver new on that check alerts nothing",
+			[]map[string]*resolver.NameserverResponse{
+				onlyA, bTXTFails, bothAnswer,
+			},
+			alertCounts{},
+		},
+		{
+			"type failing at a recovering nameserver alerts the recovery",
+			[]map[string]*resolver.NameserverResponse{
+				bFails, bTXTFails, bothAnswer,
+			},
+			alertCounts{recoveries: 1},
+		},
+		{
+			"change made while a type failed is sent when it answers",
+			[]map[string]*resolver.NameserverResponse{
+				bothAnswer, bTXTFails, bothChange,
+			},
+			alertCounts{recordChanges: 2},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+
+			states := savedChecks(tt.checks...)
+
+			got := countAlerts(t, states[0], states[1:])
+			if got != tt.want {
+				t.Errorf("sent %+v, want %+v", got, tt.want)
+			}
+		})
+	}
+}

internal/watcher/nsfailure_test.go

@@ -201,7 +201,7 @@ func TestNameserverThatNeverAnswers(t *testing.T) {
 	}
 
 	hs := watcher.BuildHostnameState(
-		map[string]*resolver.NameserverResponse{nsA: resp}, time.Now(),
+		map[string]*resolver.NameserverResponse{nsA: resp}, nil, time.Now(),
 	)
 
 	got := hs.RecordsByNameserver[nsA]
@@ -256,7 +256,7 @@ func TestNameserverThatAnswersNXDOMAIN(t *testing.T) {
 	}
 
 	hs := watcher.BuildHostnameState(
-		map[string]*resolver.NameserverResponse{ns: resp}, time.Now(),
+		map[string]*resolver.NameserverResponse{ns: resp}, nil, time.Now(),
 	)
 
 	got := hs.RecordsByNameserver[ns]
@@ -320,7 +320,7 @@ func TestNameserverThatRefuses(t *testing.T) {
 	}
 
 	hs := watcher.BuildHostnameState(
-		map[string]*resolver.NameserverResponse{ns: resp}, time.Now(),
+		map[string]*resolver.NameserverResponse{ns: resp}, nil, time.Now(),
 	)
 
 	got := hs.RecordsByNameserver[ns]

internal/watcher/watcher.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"fmt"
 	"log/slog"
+	"maps"
 	"slices"
 	"sort"
 	"strings"
@@ -266,9 +267,9 @@ func (w *Watcher) checkDomain(
 		return
 	}
 
-	newState := buildHostnameState(results, now)
-
 	prevHS, hasPrevHS := w.state.GetHostnameState(domain)
+	newState := buildHostnameState(results, prevHS, now)
+
 	if hasPrevHS && !w.firstRun {
 		w.detectHostnameChanges(ctx, domain, prevHS, newState)
 	}
@@ -398,9 +399,9 @@ func (w *Watcher) checkHostname(
 		return
 	}
 
-	newState := buildHostnameState(results, time.Now().UTC())
-
 	prev, hasPrev := w.state.GetHostnameState(hostname)
+	newState := buildHostnameState(results, prev, time.Now().UTC())
+
 	if hasPrev && !w.firstRun {
 		w.detectHostnameChanges(ctx, hostname, prev, newState)
 	}
@@ -411,9 +412,11 @@ func (w *Watcher) checkHostname(
 // buildHostnameState saves each nameserver's response. A nameserver
 // that answered, even with NXDOMAIN or no records, is saved as ok; one
 // that timed out or failed is saved as error with the reason, and its
-// empty record set is not an answer.
+// empty record set is not an answer. prev is the hostname's state from
+// the previous check, or nil.
 func buildHostnameState(
 	results map[string]*resolver.NameserverResponse,
+	prev *state.HostnameState,
 	now time.Time,
 ) *state.HostnameState {
 	hs := &state.HostnameState{
@@ -425,7 +428,7 @@ func buildHostnameState(
 
 	for ns, resp := range results {
 		nsState := &state.NameserverRecordState{
-			Records:     resp.Records,
+			Records:     maps.Clone(resp.Records),
 			Status:      statusOK,
 			LastChecked: now,
 		}
@@ -434,6 +437,13 @@ func buildHostnameState(
 			resp.Status == resolver.StatusError {
 			nsState.Status = statusError
 			nsState.Error = resp.Error
+		} else {
+			var prevNS *state.NameserverRecordState
+			if prev != nil {
+				prevNS = prev.RecordsByNameserver[ns]
+			}
+
+			keepFailedTypes(nsState, prevNS, resp.FailedTypes)
 		}
 
 		hs.RecordsByNameserver[ns] = nsState
@@ -442,6 +452,30 @@ func buildHostnameState(
 	return hs
 }
 
+// keepFailedTypes copies into nsState, for each record type in
+// failedTypes, whose query to the nameserver failed, the records prevNS,
+// the nameserver's state from the previous check, holds for that type,
+// which may be none. When prevNS does not know them either, because the
+// nameserver was new or failing then or the type was in its
+// FailedTypes, the type goes in nsState.FailedTypes instead.
+func keepFailedTypes(
+	nsState, prevNS *state.NameserverRecordState,
+	failedTypes []string,
+) {
+	for _, rtype := range failedTypes {
+		if prevNS == nil || prevNS.Status != statusOK ||
+			slices.Contains(prevNS.FailedTypes, rtype) {
+			nsState.FailedTypes = append(nsState.FailedTypes, rtype)
+
+			continue
+		}
+
+		if records, ok := prevNS.Records[rtype]; ok {
+			nsState.Records[rtype] = records
+		}
+	}
+}
+
 func (w *Watcher) detectHostnameChanges(
 	ctx context.Context,
 	hostname string,
@@ -467,7 +501,7 @@ func (w *Watcher) detectRecordChanges(
 			continue
 		}
 
-		if recordsEqual(prevNS.Records, cur.Records) {
+		if sameRecords(prevNS, cur) {
 			continue
 		}
 
@@ -600,9 +634,9 @@ func newlyDisagreeingPairs(
 
 	for i, ns1 := range nameservers {
 		for _, ns2 := range nameservers[i+1:] {
-			if recordsEqual(
-				current.RecordsByNameserver[ns1].Records,
-				current.RecordsByNameserver[ns2].Records,
+			if sameRecords(
+				current.RecordsByNameserver[ns1],
+				current.RecordsByNameserver[ns2],
 			) {
 				continue
 			}
@@ -612,7 +646,7 @@ func newlyDisagreeingPairs(
 
 			if ok1 && ok2 &&
 				prev1.Status == statusOK && prev2.Status == statusOK &&
-				!recordsEqual(prev1.Records, prev2.Records) {
+				!sameRecords(prev1, prev2) {
 				continue
 			}
 
@@ -1090,6 +1124,21 @@ func toSet(items []string) map[string]bool {
 	return set
 }
 
+// sameRecords reports whether two nameserver states hold the same
+// records, leaving out the record types either lists in FailedTypes:
+// their records are not known.
+func sameRecords(a, b *state.NameserverRecordState) bool {
+	aRecords := maps.Clone(a.Records)
+	bRecords := maps.Clone(b.Records)
+
+	for _, rtype := range slices.Concat(a.FailedTypes, b.FailedTypes) {
+		delete(aRecords, rtype)
+		delete(bRecords, rtype)
+	}
+
+	return recordsEqual(aRecords, bRecords)
+}
+
 func recordsEqual(
 	a, b map[string][]string,
 ) bool {