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

@@ -435,7 +435,7 @@ following precedence (highest to lowest):
 
 | Variable                               | Description                                                                                                                           | Default               |
 | -------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------- | --------------------- |
-| `PORT`                                 | HTTP listen port; a port that cannot be bound, such as one already in use, stops startup                                              | `8080`                |
+| `PORT`                                 | HTTP listen port                                                                                                                      | `8080`                |
 | `DNSWATCHER_DEBUG`                     | Enable debug logging                                                                                                                  | `false`               |
 | `DNSWATCHER_DATA_DIR`                  | Directory for state file                                                                                                              | `/var/lib/dnswatcher` |
 | `DNSWATCHER_TARGETS`                   | Comma-separated DNS names (auto-classified via PSL)                                                                                   | `""`                  |

TODO.md

@@ -21,10 +21,6 @@ trial run of the finished image: https://git.eeqj.de/sneak/dnswatcher/issues/149
 
 - 2026-10-13: a nameserver leaving a domain's delegation sends only NS Change;
   one leaving a hostname's zone sends NS Change, not NS Failure (closes #262).
-- 2026-10-13: a save syncs the state file to disk before renaming it into place,
-  so a crash cannot leave it empty and stop the next start (closes #264).
-- 2026-10-13: a `PORT` that cannot be bound stops startup with an error naming
-  the address, instead of running without the HTTP server (closes #266).
 - 2026-10-13: a bad `PORT`, `DNSWATCHER_TLS_EXPIRY_WARNING` or true/false
   setting stops startup instead of being read as 0 or false (closes #269).
 - 2026-10-13: a check saving the state while the dashboard or `/api/v1/status`

internal/server/export_test.go

@@ -13,8 +13,8 @@ import (
 const RequestTimeout time.Duration = requestTimeout
 
 // SetListenPort overrides the port Run binds. A test uses it to hand
-// Run an unbindable port, so that Run stores its http.Server and then
-// returns an error without listening.
+// Run an unbindable port so ListenAndServe fails immediately and Run
+// returns after storing its http.Server.
 func SetListenPort(s *Server, port int) {
 	s.port = port
 }

internal/server/server.go

@@ -6,7 +6,6 @@ import (
 	"errors"
 	"fmt"
 	"log/slog"
-	"net"
 	"net/http"
 	"time"
 
@@ -113,7 +112,7 @@ func New(
 	}
 
 	lifecycle.Append(fx.Hook{
-		OnStart: func(ctx context.Context) error {
+		OnStart: func(_ context.Context) error {
 			srv.startupTime = time.Now()
 
 			err := srv.enableSentry()
@@ -121,7 +120,9 @@ func New(
 				return err
 			}
 
-			return srv.Run(ctx)
+			go srv.Run()
+
+			return nil
 		},
 		OnStop: func(ctx context.Context) error {
 			return srv.Shutdown(ctx)
@@ -148,10 +149,8 @@ func newHTTPServer(
 	}
 }
 
-// Run binds the listen port, then serves HTTP on it in a new goroutine
-// and returns. A port that cannot be bound is returned as an error, so
-// that startup stops instead of running without the HTTP server.
-func (s *Server) Run(ctx context.Context) error {
+// Run starts the HTTP server.
+func (s *Server) Run() {
 	s.SetupRoutes()
 
 	listenAddr := fmt.Sprintf(":%d", s.port)
@@ -159,21 +158,10 @@ func (s *Server) Run(ctx context.Context) error {
 
 	s.log.Info("http server starting", "addr", listenAddr)
 
-	lc := &net.ListenConfig{}
-
-	listener, err := lc.Listen(ctx, "tcp", listenAddr)
-	if err != nil {
-		return fmt.Errorf("starting http server: %w", err)
+	err := s.httpServer.ListenAndServe()
+	if err != nil && !errors.Is(err, http.ErrServerClosed) {
+		s.log.Error("http server error", "error", err)
 	}
-
-	go func() {
-		err := s.httpServer.Serve(listener)
-		if err != nil && !errors.Is(err, http.ErrServerClosed) {
-			s.log.Error("http server error", "error", err)
-		}
-	}()
-
-	return nil
 }
 
 // Shutdown gracefully shuts down the server, then sends the error

internal/server/server_test.go

@@ -1,8 +1,6 @@
 package server_test
 
 import (
-	"net"
-	"strings"
 	"testing"
 
 	"github.com/spf13/viper"
@@ -60,13 +58,15 @@ func buildServer(t *testing.T) *server.Server {
 }
 
 // TestRunWiresSocketTimeouts pins that the http.Server the running
-// server actually serves — the one Run builds and serves on the port
-// it binds — carries every socket-level timeout, plus the two
+// server actually serves — the one Run builds and hands to
+// ListenAndServe — carries every socket-level timeout, plus the two
 // relationships the values must satisfy.
 //
-// Run is given an unbindable port: it builds and stores s.httpServer,
-// then the bind fails and Run returns the error without ever
-// listening. Nothing here measures elapsed time.
+// Run is driven to completion with an unbindable port: it builds and
+// stores s.httpServer, then ListenAndServe fails at once and Run
+// returns without ever listening. The assertions run in the same
+// goroutine after Run returns, so reading s.httpServer is free of any
+// data race. Nothing here measures elapsed time.
 //
 // ReadTimeout must be at least ReadHeaderTimeout. net/http reads the
 // headers under ReadHeaderTimeout, then sets the read deadline for the
@@ -84,10 +84,7 @@ func TestRunWiresSocketTimeouts(t *testing.T) {
 	srv := buildServer(t)
 	server.SetListenPort(srv, -1)
 
-	err := srv.Run(t.Context())
-	if err == nil {
-		t.Fatal("Run bound port -1")
-	}
+	srv.Run()
 
 	hs := server.HTTPServerOf(srv)
 	if hs == nil {
@@ -136,38 +133,3 @@ func TestRunWiresSocketTimeouts(t *testing.T) {
 		)
 	}
 }
-
-func TestPortInUseStopsStartup(t *testing.T) {
-	// Bound on every address, as Run binds, so that Run's bind fails.
-	lc := &net.ListenConfig{}
-
-	taken, err := lc.Listen(t.Context(), "tcp", ":0")
-	if err != nil {
-		t.Fatalf("binding a port for the test: %v", err)
-	}
-
-	defer func() { _ = taken.Close() }()
-
-	_, port, err := net.SplitHostPort(taken.Addr().String())
-	if err != nil {
-		t.Fatalf("reading the bound port: %v", err)
-	}
-
-	viper.Reset()
-	t.Setenv("DNSWATCHER_TARGETS", "example.com")
-	t.Setenv("DNSWATCHER_DATA_DIR", t.TempDir())
-	t.Setenv("PORT", port)
-
-	app := newServerApp(fx.Invoke(func(*server.Server) {}))
-
-	err = app.Start(t.Context())
-	if err == nil {
-		_ = app.Stop(t.Context())
-
-		t.Fatal("startup succeeded on a port that is already bound")
-	}
-
-	if !strings.Contains(err.Error(), ":"+port) {
-		t.Errorf("startup error does not name port %s: %v", port, err)
-	}
-}

internal/state/state.go

@@ -4,7 +4,6 @@ package state
 import (
 	"context"
 	"encoding/json"
-	"errors"
 	"fmt"
 	"log/slog"
 	"maps"
@@ -254,20 +253,10 @@ func (s *State) Save() error {
 		return fmt.Errorf("creating data directory: %w", err)
 	}
 
-	// Atomic write: write to temp file, then rename. The temp file is
-	// synced before the rename and the directory after it, or a crash
-	// can leave the renamed state file empty.
+	// Atomic write: write to temp file, then rename
 	tmpPath := path + ".tmp"
 
-	//nolint:gosec // path is from trusted config
-	tmpFile, err := os.OpenFile(
-		tmpPath, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, filePermissions,
-	)
-	if err == nil {
-		_, err = tmpFile.Write(data)
-		err = errors.Join(err, tmpFile.Sync(), tmpFile.Close())
-	}
-
+	err = os.WriteFile(tmpPath, data, filePermissions)
 	if err != nil {
 		return fmt.Errorf("writing temp state file: %w", err)
 	}
@@ -277,15 +266,6 @@ func (s *State) Save() error {
 		return fmt.Errorf("renaming state file: %w", err)
 	}
 
-	dir, err := os.Open(filepath.Dir(path))
-	if err == nil {
-		err = errors.Join(dir.Sync(), dir.Close())
-	}
-
-	if err != nil {
-		return fmt.Errorf("syncing data directory: %w", err)
-	}
-
 	s.log.Debug("state saved to disk", "path", path)
 
 	return nil

internal/state/state_test.go

@@ -618,53 +618,6 @@ func TestSaveAtomicWrite(t *testing.T) {
 	}
 }
 
-// TestSaveOverLeftoverTempFile verifies that a temp file left by a
-// crash, longer than the state Save writes, leaves nothing of itself in
-// the state file, and that no temp file remains after Save.
-func TestSaveOverLeftoverTempFile(t *testing.T) {
-	t.Parallel()
-
-	dir := t.TempDir()
-	statePath := filepath.Join(dir, "state.json")
-	tmpPath := statePath + ".tmp"
-
-	err := os.WriteFile(tmpPath, []byte(strings.Repeat("x", 64*1024)), 0o600)
-	if err != nil {
-		t.Fatalf("writing leftover temp file: %v", err)
-	}
-
-	s := state.NewForTestWithDataDir(dir)
-
-	populateState(t, s)
-
-	err = s.Save()
-	if err != nil {
-		t.Fatalf("Save() error: %v", err)
-	}
-
-	//nolint:gosec // test file with known path
-	data, err := os.ReadFile(statePath)
-	if err != nil {
-		t.Fatalf("reading state file: %v", err)
-	}
-
-	var snap state.Snapshot
-
-	err = json.Unmarshal(data, &snap)
-	if err != nil {
-		t.Fatalf("state file is not complete JSON: %v", err)
-	}
-
-	if len(snap.Domains) != 2 {
-		t.Errorf("state file has %d domains, want 2", len(snap.Domains))
-	}
-
-	_, err = os.Stat(tmpPath)
-	if !os.IsNotExist(err) {
-		t.Error("temp file should not exist after save")
-	}
-}
-
 // TestLoadMissingFile verifies that loading a nonexistent file is not an error.
 func TestLoadMissingFile(t *testing.T) {
 	t.Parallel()

internal/watcher/watcher.go

@@ -328,8 +328,8 @@ func (w *Watcher) checkDomain(
 	// a hostname's, so that the port and TLS checks find its addresses.
 	// Notifications about them name it as a domain (see nameLine). When
 	// their lookup fails, the check fails as when the NS lookup does, so
-	// the saved delegation and the saved records come from the same
-	// check, as detectNSDisappearances needs.
+	// the saved nameservers stay those the saved records were asked at,
+	// as detectNSDisappearances needs.
 	if nxdomain {
 		w.state.DeleteHostnameState(domain)
 	} else if !w.checkHostname(ctx, domain) {
@@ -782,8 +782,8 @@ func (w *Watcher) detectRecordChanges(
 // in. For a configured apex domain it leaves out those of the domain's
 // saved delegation, which detectNSChanges notifies when they are removed
 // from it. checkDomain saves a new delegation only after this runs, and
-// only together with the records, so the saved delegation and the
-// records in prev come from the same check.
+// 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,