server: assert timeouts on the served http.Server, not just the constructor (closes #120)

The timeout tests called newHTTPServer directly, so a Run that built
its http.Server inline would drop every timeout with the suite still
green. TestRunWiresSocketTimeouts wires a Server as cmd/dnswatcher
does, minus the watcher and resolver so no live DNS is touched, drives
Run with an unbindable port so it stores its http.Server and returns
without listening, and checks that server carries all four timeouts
and both required relationships. The addr/handler test, which could
not fail, is dropped.

The ReadTimeout note now says what net/http does: a request whose
headers arrive after ReadTimeout but within ReadHeaderTimeout gets a
read deadline that has already passed, so reading its body fails at
once.

Model: opus-4-8 (implementation); opus-5-5 (rework)

TODO.md

@@ -23,6 +23,9 @@ Rationale, Design, TODO, License, Author) if any are still missing.
 
 # Completed Steps
 
+- 2026-09-28: the server timeout test now drives `Run` and checks the
+  `http.Server` it serves carries the timeouts; corrected the `ReadTimeout`
+  note in that test (closes #120).
 - 2026-09-28: upaas deploy readiness — runtime image runs as unprivileged
   `dnswatcher`, Docker `HEALTHCHECK`, startup fails when the data directory is
   not writable, README "Running under upaas" (closes #147).

internal/server/export_test.go

@@ -5,15 +5,20 @@ import (
 	"time"
 )
 
-// NewHTTPServer exports newHTTPServer for testing.
-func NewHTTPServer(
-	listenAddr string,
-	handler http.Handler,
-) *http.Server {
-	return newHTTPServer(listenAddr, handler)
-}
-
 // RequestTimeout exports the handler execution budget applied by
 // chimw.Timeout in SetupRoutes, so tests can assert the relationship
 // between it and the server's WriteTimeout.
 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.
+func SetListenPort(s *Server, port int) {
+	s.port = port
+}
+
+// HTTPServerOf returns the http.Server that Run built and stored, so a
+// test can inspect the timeouts the running server actually carries.
+func HTTPServerOf(s *Server) *http.Server {
+	return s.httpServer
+}

internal/server/server_test.go

@@ -1,112 +1,130 @@
 package server_test
 
 import (
-	"net/http"
 	"testing"
 
+	"github.com/spf13/viper"
+	"go.uber.org/fx"
+
+	"sneak.berlin/go/dnswatcher/internal/config"
+	"sneak.berlin/go/dnswatcher/internal/globals"
+	"sneak.berlin/go/dnswatcher/internal/handlers"
+	"sneak.berlin/go/dnswatcher/internal/healthcheck"
+	"sneak.berlin/go/dnswatcher/internal/logger"
+	"sneak.berlin/go/dnswatcher/internal/middleware"
+	"sneak.berlin/go/dnswatcher/internal/notify"
 	"sneak.berlin/go/dnswatcher/internal/server"
+	"sneak.berlin/go/dnswatcher/internal/state"
 )
 
-// noopHandler stands in for the router; newHTTPServer only stores it.
+// buildServer wires a *server.Server exactly as cmd/dnswatcher does,
-func noopHandler() http.Handler {
+// minus the watcher/resolver subtree that would touch live DNS. fx
-	return http.HandlerFunc(
+// builds the object graph but the lifecycle is never started, so no
-		func(w http.ResponseWriter, _ *http.Request) {
+// OnStart hook runs and nothing listens or resolves. The caller must
-			w.WriteHeader(http.StatusOK)
+// first configure viper (config.New reads it), which is also why the
-		},
+// caller cannot run in parallel.
+func buildServer(t *testing.T) *server.Server {
+	t.Helper()
+
+	var srv *server.Server
+
+	app := fx.New(
+		fx.NopLogger,
+		fx.Provide(
+			globals.New,
+			logger.New,
+			config.New,
+			state.New,
+			healthcheck.New,
+			notify.New,
+			middleware.New,
+			handlers.New,
+			server.New,
+		),
+		fx.Populate(&srv),
 	)
+
+	err := app.Err()
+	if err != nil {
+		t.Fatalf("building server graph: %v", err)
 	}
 
-// TestHTTPServerTimeoutsAreSet asserts that every socket-level
+	return srv
-// timeout is configured. A zero value in net/http means "no limit",
+}
-// so a refactor that silently drops one of these reintroduces the
+
-// slowloris / unreaped-keep-alive exposure this guards against.
+// 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
+// relationships the values must satisfy.
 //
-// The assertions are on the configured field values only; nothing
+// Run is driven to completion with an unbindable port: it builds and
-// here measures elapsed time, so the test cannot flake on timing.
+// stores s.httpServer, then ListenAndServe fails at once and Run
-func TestHTTPServerTimeoutsAreSet(t *testing.T) {
+// returns without ever listening. The assertions run in the same
-	t.Parallel()
+// 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
+// rest of the request to ReadTimeout, counted from when it started
+// reading the request. If ReadTimeout were smaller, a request whose
+// headers arrived after ReadTimeout but within ReadHeaderTimeout would
+// get a read deadline that had already passed, so reading its body
+// would fail at once.
+func TestRunWiresSocketTimeouts(t *testing.T) {
+	// Sets an env var and touches viper global state, so like the
+	// config tests it cannot use t.Parallel.
+	viper.Reset()
+	t.Setenv("DNSWATCHER_TARGETS", "example.com")
 
-	srv := server.NewHTTPServer(":8080", noopHandler())
+	srv := buildServer(t)
+	server.SetListenPort(srv, -1)
 
-	if srv.ReadTimeout <= 0 {
+	srv.Run()
-		t.Errorf(
+
-			"ReadTimeout must be non-zero, got %v",
+	hs := server.HTTPServerOf(srv)
-			srv.ReadTimeout,
+	if hs == nil {
-		)
+		t.Fatal("Run did not build an http.Server")
 	}
 
-	if srv.ReadHeaderTimeout <= 0 {
+	if hs.ReadTimeout <= 0 {
+		t.Errorf("ReadTimeout must be non-zero, got %v", hs.ReadTimeout)
+	}
+
+	if hs.ReadHeaderTimeout <= 0 {
 		t.Errorf(
 			"ReadHeaderTimeout must be non-zero, got %v",
-			srv.ReadHeaderTimeout,
+			hs.ReadHeaderTimeout,
 		)
 	}
 
-	if srv.WriteTimeout <= 0 {
+	if hs.WriteTimeout <= 0 {
-		t.Errorf(
+		t.Errorf("WriteTimeout must be non-zero, got %v", hs.WriteTimeout)
-			"WriteTimeout must be non-zero, got %v",
-			srv.WriteTimeout,
-		)
 	}
 
-	if srv.IdleTimeout <= 0 {
+	if hs.IdleTimeout <= 0 {
-		t.Errorf(
+		t.Errorf("IdleTimeout must be non-zero, got %v", hs.IdleTimeout)
-			"IdleTimeout must be non-zero, got %v",
-			srv.IdleTimeout,
-		)
-	}
 	}
 
-// TestWriteTimeoutExceedsHandlerBudget pins the one relationship the
+	if hs.WriteTimeout <= server.RequestTimeout {
-// values must satisfy. net/http arms the write deadline once request
-// headers are read, so it covers handler execution plus the response
-// flush. If WriteTimeout were not greater than the chimw.Timeout
-// handler budget, the connection would be severed before a handler
-// that used its full budget could respond, making that budget
-// unreachable.
-func TestWriteTimeoutExceedsHandlerBudget(t *testing.T) {
-	t.Parallel()
-
-	srv := server.NewHTTPServer(":8080", noopHandler())
-
-	if srv.WriteTimeout <= server.RequestTimeout {
 		t.Errorf(
 			"WriteTimeout (%v) must exceed handler budget (%v)",
-			srv.WriteTimeout,
+			hs.WriteTimeout,
 			server.RequestTimeout,
 		)
 	}
-}
 
-// TestReadTimeoutCoversHeaderTimeout asserts the read deadline for
+	if hs.ReadTimeout < hs.ReadHeaderTimeout {
-// the whole request is at least as long as the header-only deadline;
-// a smaller ReadTimeout would make ReadHeaderTimeout unreachable.
-func TestReadTimeoutCoversHeaderTimeout(t *testing.T) {
-	t.Parallel()
-
-	srv := server.NewHTTPServer(":8080", noopHandler())
-
-	if srv.ReadTimeout < srv.ReadHeaderTimeout {
 		t.Errorf(
 			"ReadTimeout (%v) must be >= ReadHeaderTimeout (%v)",
-			srv.ReadTimeout,
+			hs.ReadTimeout,
-			srv.ReadHeaderTimeout,
+			hs.ReadHeaderTimeout,
+		)
+	}
+
+	if hs.Handler != srv {
+		t.Errorf(
+			"Run wired handler %T, want the *server.Server",
+			hs.Handler,
 		)
 	}
 }
-
-// TestHTTPServerAddrAndHandler covers the rest of the constructor so
-// a future edit cannot drop the listen address or the handler.
-func TestHTTPServerAddrAndHandler(t *testing.T) {
-	t.Parallel()
-
-	srv := server.NewHTTPServer(":9999", noopHandler())
-
-	if srv.Addr != ":9999" {
-		t.Errorf("Addr = %q, want %q", srv.Addr, ":9999")
-	}
-
-	if srv.Handler == nil {
-		t.Error("Handler must not be nil")
-	}
-}