watcher: a removed nameserver is an NS Change, not an NS Failure (closes #262)

A nameserver that left a domain's delegation was reported twice: as the
domain's NS Change, and by the check of the domain's own records as an
error-level NS Failure saying it disappeared. The check of a name's
records now sends one NS Change at warning level, starting Hostname: or
Domain:, listing the nameservers it was asked at before and not now.
For a domain it leaves out those of its saved delegation, which the
domain's own NS Change lists, so a domain's new delegation is now
notified and saved only after, and only together with, its records.
Nothing is sent for a nameserver that joins.

A nameserver that was failing before it left is now listed too; the
NS Failure skipped it.

Model: opus-5-5

README.md

@@ -78,8 +78,9 @@ notification endpoint set, changes show only on the dashboard; see
   answer NXDOMAIN, that it does not exist, has no nameservers and is shown as
   not existing (see Web Dashboard and HTTP API). A domain that exists but has no
   delegation of its own, such as `octocat.github.io`, has no nameservers either.
-  When the parent zone's servers do not answer, the check fails and the set from
+  When the parent zone's servers do not answer, or the lookup of the domain's
-  the previous check is kept.
+  own records (below) fails, the check fails and the set from the previous check
+  is kept.
 - Any change triggers a notification:
     - NS added to or removed from that set. A domain that had nameservers on the
       previous check and no longer exists gets one with all of them removed.

internal/watcher/watcher.go

@@ -322,22 +322,23 @@ func (w *Watcher) checkDomain(
 		ctx, nameservers, prevAddresses,
 	)
 
-	if hasPrev && !w.firstRun {
-		w.detectNSChanges(ctx, domain, prev.Nameservers, nameservers)
-		w.detectNSAddressChanges(ctx, domain, prevAddresses, addresses)
-	}
-
 	// A domain that does not exist has no records of its own: none are
 	// asked for, and those saved by an earlier check are removed.
 	// Otherwise the apex domain's records are also checked and saved as
 	// a hostname's, so that the port and TLS checks find its addresses.
-	// Notifications about them name it as a domain (see nameLine). They
+	// Notifications about them name it as a domain (see nameLine). When
-	// are checked before the domain's new nameservers are saved:
+	// their lookup fails, the check fails as when the NS lookup does, so
-	// detectNSDisappearances reads those of the previous check.
+	// the saved nameservers stay those the saved records were asked at,
+	// as detectNSDisappearances needs.
 	if nxdomain {
 		w.state.DeleteHostnameState(domain)
-	} else {
+	} else if !w.checkHostname(ctx, domain) {
-		w.checkHostname(ctx, domain)
+		return
+	}
+
+	if hasPrev && !w.firstRun {
+		w.detectNSChanges(ctx, domain, prev.Nameservers, nameservers)
+		w.detectNSAddressChanges(ctx, domain, prevAddresses, addresses)
 	}
 
 	w.state.SetDomainState(domain, &state.DomainState{
@@ -456,10 +457,12 @@ func (w *Watcher) detectNSAddressChanges(
 	}
 }
 
+// checkHostname checks hostname's records and reports whether it saved
+// them; it does not when their lookup fails.
 func (w *Watcher) checkHostname(
 	ctx context.Context,
 	hostname string,
-) {
+) bool {
 	results, err := w.resolver.LookupAllRecords(ctx, hostname)
 	if err != nil {
 		w.logFailedLookup(
@@ -469,7 +472,7 @@ func (w *Watcher) checkHostname(
 			"error", err,
 		)
 
-		return
+		return false
 	}
 
 	prev, _ := w.state.GetHostnameState(hostname)
@@ -477,6 +480,8 @@ func (w *Watcher) checkHostname(
 	w.updateHostnameState(
 		ctx, hostname, buildHostnameState(results, prev, time.Now().UTC()),
 	)
+
+	return true
 }
 
 // updateHostnameState finishes a check of hostname from newState, built
@@ -775,9 +780,10 @@ func (w *Watcher) detectRecordChanges(
 // detectNSDisappearances sends one NS Change listing the nameservers of
 // the previous check that are no longer those of the zone hostname is
 // in. For a configured apex domain it leaves out those of the domain's
-// delegation on the previous check, which detectNSChanges notifies when
+// saved delegation, which detectNSChanges notifies when they are removed
-// they are removed from it; checkDomain saves the new delegation only
+// from it. checkDomain saves a new delegation only after this runs, and
-// after this runs.
+// only together with the records, so the saved one is the delegation
+// prev was asked at.
 func (w *Watcher) detectNSDisappearances(
 	ctx context.Context,
 	hostname string,

internal/watcher/watcher_test.go

@@ -139,6 +139,22 @@ func (m *mockNotifier) getNotifications() []notification {
 	return result
 }
 
+// cancelOnNotify is a mockNotifier that calls cancel at each
+// notification, as if shutdown cut the check short there.
+type cancelOnNotify struct {
+	*mockNotifier
+
+	cancel context.CancelFunc
+}
+
+func (n cancelOnNotify) SendNotification(
+	ctx context.Context,
+	title, message, priority string,
+) {
+	n.mockNotifier.SendNotification(ctx, title, message, priority)
+	n.cancel()
+}
+
 // --- Helpers to build a Watcher and run its checks against live DNS ---
 
 type testDeps struct {
@@ -559,8 +575,10 @@ func TestDomainThatDoesNotExist(t *testing.T) {
 // codeberg.page servers, are saved. Those are testSmallDomain's two
 // nameservers; github.io, the zone of the README's example, has eight.
 // The previous check saved a delegation to oldNS1, now withdrawn, and
-// records asked at oldNS1: the NS Change of its delegation removes
+// records asked at oldNS1. A first check is cut short at its first
-// oldNS1, and no other NS Change lists it again.
+// notification, as shutdown would, and a second runs in full: across
+// them the NS Change of its delegation removes oldNS1, and no other NS
+// Change lists it again.
 func TestDomainWithNoDelegationOfItsOwn(t *testing.T) {
 	t.Parallel()
 
@@ -572,9 +590,19 @@ func TestDomainWithNoDelegationOfItsOwn(t *testing.T) {
 	var deps *testDeps
 
 	livednstest.Retry(t, "watcher checks", func(ctx context.Context) error {
-		var w *watcher.Watcher
+		deps = newTestDeps(t, cfg)
 
-		w, deps = newTestWatcher(t, cfg)
+		firstCtx, cancel := context.WithCancel(ctx)
+		defer cancel()
+
+		w := watcher.NewForTest(
+			cfg,
+			deps.state,
+			resolver.NewFromLogger(slog.Default()),
+			deps.portChecker,
+			deps.tlsChecker,
+			cancelOnNotify{mockNotifier: deps.notifier, cancel: cancel},
+		)
 
 		deps.state.SetDomainState(domain, &state.DomainState{
 			Nameservers: []string{oldNS1},
@@ -585,6 +613,8 @@ func TestDomainWithNoDelegationOfItsOwn(t *testing.T) {
 			},
 		))
 
+		w.RunOnce(firstCtx)
+
 		err := checkOnce(ctx, w, deps)
 
 		// A domain saved as not existing has no records to wait for;