diff --git a/TODO.md b/TODO.md index e3fee17..c40735a 100644 --- a/TODO.md +++ b/TODO.md @@ -23,6 +23,10 @@ Rationale, Design, TODO, License, Author) if any are still missing. # Completed Steps +- 2026-09-21: server timeout test now drives `Run` and asserts the + served `http.Server` carries the timeouts; corrected the inverted + `ReadTimeout` rationale note (#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). @@ -30,6 +34,7 @@ Rationale, Design, TODO, License, Author) if any are still missing. `internal/healthcheck`, and `internal/logger` (closes #110). - 2026-09-21: `go mod tidy` dropped the redundant `golang.org/x/sync` `// indirect` line so `script/bootstrap` leaves a clean tree (#132) + - 2026-08-10: comment-only corrections to `script/bootstrap`, `script/cibuild`, and `Dockerfile.lint`. The `goimports` pin in `script/bootstrap` was justified by a claim that `script/fmt-check` diff --git a/internal/server/export_test.go b/internal/server/export_test.go index 8df05f2..5a1bec7 100644 --- a/internal/server/export_test.go +++ b/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 +} diff --git a/internal/server/server_test.go b/internal/server/server_test.go index 9909f7d..ba65432 100644 --- a/internal/server/server_test.go +++ b/internal/server/server_test.go @@ -1,112 +1,136 @@ 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. -func noopHandler() http.Handler { - return http.HandlerFunc( - func(w http.ResponseWriter, _ *http.Request) { - w.WriteHeader(http.StatusOK) - }, +// buildServer wires a *server.Server exactly as cmd/dnswatcher does, +// minus the watcher/resolver subtree that would touch live DNS. fx +// builds the object graph but the lifecycle is never started, so no +// OnStart hook runs and nothing listens or resolves. The caller must +// 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), ) -} -// TestHTTPServerTimeoutsAreSet asserts that every socket-level -// 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. -// -// The assertions are on the configured field values only; nothing -// here measures elapsed time, so the test cannot flake on timing. -func TestHTTPServerTimeoutsAreSet(t *testing.T) { - t.Parallel() - - srv := server.NewHTTPServer(":8080", noopHandler()) - - if srv.ReadTimeout <= 0 { - t.Errorf( - "ReadTimeout must be non-zero, got %v", - srv.ReadTimeout, - ) + err := app.Err() + if err != nil { + t.Fatalf("building server graph: %v", err) } - if srv.ReadHeaderTimeout <= 0 { + return srv +} + +// 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. Earlier tests asserted these +// on newHTTPServer directly, which left the call site unguarded: a Run +// that built its http.Server inline would drop every timeout with the +// suite still green (https://git.eeqj.de/sneak/dnswatcher/issues/120). +// +// 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. +// +// On the ReadTimeout >= ReadHeaderTimeout relationship: a smaller +// ReadTimeout does NOT make the header phase unreachable. net/http's +// (*Server).readHeaderTimeout applies ReadHeaderTimeout directly, so +// the header read keeps its full budget. What breaks is the +// whole-request deadline: once the headers are read, readRequest +// installs a read deadline of t0+ReadTimeout, which is already in the +// past when ReadTimeout is the smaller value, severing the request. +// Verified against the pinned go1.25 net/http (Dockerfile golang +// 1.25-alpine; go.mod go 1.25.5): src/net/http/server.go readRequest +// and (*Server).readHeaderTimeout. +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 := buildServer(t) + server.SetListenPort(srv, -1) + + srv.Run() + + hs := server.HTTPServerOf(srv) + if hs == nil { + t.Fatal("Run did not build an http.Server") + } + + 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 { - t.Errorf( - "WriteTimeout must be non-zero, got %v", - srv.WriteTimeout, - ) + if hs.WriteTimeout <= 0 { + t.Errorf("WriteTimeout must be non-zero, got %v", hs.WriteTimeout) } - if srv.IdleTimeout <= 0 { - t.Errorf( - "IdleTimeout must be non-zero, got %v", - srv.IdleTimeout, - ) + if hs.IdleTimeout <= 0 { + t.Errorf("IdleTimeout must be non-zero, got %v", hs.IdleTimeout) } -} -// TestWriteTimeoutExceedsHandlerBudget pins the one relationship the -// 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 { + if hs.WriteTimeout <= server.RequestTimeout { t.Errorf( "WriteTimeout (%v) must exceed handler budget (%v)", - srv.WriteTimeout, + hs.WriteTimeout, server.RequestTimeout, ) } -} -// TestReadTimeoutCoversHeaderTimeout asserts the read deadline for -// 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 { + if hs.ReadTimeout < hs.ReadHeaderTimeout { t.Errorf( "ReadTimeout (%v) must be >= ReadHeaderTimeout (%v)", - srv.ReadTimeout, - srv.ReadHeaderTimeout, + hs.ReadTimeout, + 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") - } -}