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                                                                                                                      | `8080`                |
+| `PORT`                                 | HTTP listen port; a port that cannot be bound, such as one already in use, stops startup                                              | `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,6 +21,10 @@ 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 ListenAndServe fails immediately and Run
-// returns after storing its http.Server.
+// Run an unbindable port, so that Run stores its http.Server and then
+// returns an error without listening.
 func SetListenPort(s *Server, port int) {
 	s.port = port
 }

internal/server/server.go

@@ -6,6 +6,7 @@ import (
 	"errors"
 	"fmt"
 	"log/slog"
+	"net"
 	"net/http"
 	"time"
 
@@ -112,7 +113,7 @@ func New(
 	}
 
 	lifecycle.Append(fx.Hook{
-		OnStart: func(_ context.Context) error {
+		OnStart: func(ctx context.Context) error {
 			srv.startupTime = time.Now()
 
 			err := srv.enableSentry()
@@ -120,9 +121,7 @@ func New(
 				return err
 			}
 
-			go srv.Run()
-
-			return nil
+			return srv.Run(ctx)
 		},
 		OnStop: func(ctx context.Context) error {
 			return srv.Shutdown(ctx)
@@ -149,8 +148,10 @@ func newHTTPServer(
 	}
 }
 
-// Run starts the HTTP server.
-func (s *Server) Run() {
+// 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 {
 	s.SetupRoutes()
 
 	listenAddr := fmt.Sprintf(":%d", s.port)
@@ -158,10 +159,21 @@ func (s *Server) Run() {
 
 	s.log.Info("http server starting", "addr", listenAddr)
 
-	err := s.httpServer.ListenAndServe()
-	if err != nil && !errors.Is(err, http.ErrServerClosed) {
-		s.log.Error("http server error", "error", err)
+	lc := &net.ListenConfig{}
+
+	listener, err := lc.Listen(ctx, "tcp", listenAddr)
+	if err != nil {
+		return fmt.Errorf("starting http server: %w", 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,6 +1,8 @@
 package server_test
 
 import (
+	"net"
+	"strings"
 	"testing"
 
 	"github.com/spf13/viper"
@@ -58,15 +60,13 @@ func buildServer(t *testing.T) *server.Server {
 }
 
 // TestRunWiresSocketTimeouts pins that the http.Server the running
-// server actually serves — the one Run builds and hands to
-// ListenAndServe — carries every socket-level timeout, plus the two
+// server actually serves — the one Run builds and serves on the port
+// it binds — carries every socket-level timeout, plus the two
 // relationships the values must satisfy.
 //
-// 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.
+// 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.
 //
 // ReadTimeout must be at least ReadHeaderTimeout. net/http reads the
 // headers under ReadHeaderTimeout, then sets the read deadline for the
@@ -84,7 +84,10 @@ func TestRunWiresSocketTimeouts(t *testing.T) {
 	srv := buildServer(t)
 	server.SetListenPort(srv, -1)
 
-	srv.Run()
+	err := srv.Run(t.Context())
+	if err == nil {
+		t.Fatal("Run bound port -1")
+	}
 
 	hs := server.HTTPServerOf(srv)
 	if hs == nil {
@@ -133,3 +136,38 @@ 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,6 +4,7 @@ package state
 import (
 	"context"
 	"encoding/json"
+	"errors"
 	"fmt"
 	"log/slog"
 	"maps"
@@ -253,10 +254,20 @@ func (s *State) Save() error {
 		return fmt.Errorf("creating data directory: %w", err)
 	}
 
-	// Atomic write: write to temp file, then rename
+	// 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.
 	tmpPath := path + ".tmp"
 
-	err = os.WriteFile(tmpPath, data, filePermissions)
+	//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())
+	}
+
 	if err != nil {
 		return fmt.Errorf("writing temp state file: %w", err)
 	}
@@ -266,6 +277,15 @@ 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,6 +618,53 @@ 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 nameservers stay those the saved records were asked at,
-	// as detectNSDisappearances needs.
+	// the saved delegation and the saved records come from the same
+	// check, 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 one is the delegation
-// prev was asked at.
+// only together with the records, so the saved delegation and the
+// records in prev come from the same check.
 func (w *Watcher) detectNSDisappearances(
 	ctx context.Context,
 	hostname string,