notify: fix flaky drain timing assertions, drop false abandon warn (closes #106 )

The drain tests measured elapsed time from an instant captured after the clock they compared it against had already started, so the lower bounds were structurally unreachable and passed only when the gap between the two statements rounded to zero. TestDrainBoundedByContext- Deadline failed the Docker gate outright (49.9ms against its own 50ms deadline) and roughly 1 run in 12 locally. - TestDrainBoundedByContextDeadline: capture start before context.WithTimeout, so the measured interval is a superset of the deadline interval and only an early return can fail the lower bound. The upper bound moves to a watchdog around the drain, which turns an unbounded drain into a prompt failure instead of a package-timeout hang. - TestDrainWaitsForInFlightDelivery: same ordering fix, ahead of the timer that releases the held delivery. - TestDrainWithoutDeliveriesReturnsImmediately: its 50ms ceiling was under the observed cost of the goroutine hop through inFlight.Wait() on a loaded box (57ms), and failed once in 20 runs. It now bounds the idle drain at 500ms, still well under the 2s deadline a stalled drain would hit. - drain: an OnStop context already expired on entry with nothing outstanding logged a WARN about abandoning deliveries with abandoned=0 and closed the abandon channel for no reason. The timeout branch now reports at debug level when the outstanding count is zero, and warns only when deliveries genuinely are abandoned. TestDrainWithCancelledContextDoesNotWarn covers it. Verified: script/cibuild passes; 25 consecutive cache-bypassed make test runs under -race, all clean; make check green at 5.2s. Both corrected assertions were confirmed non-vacuous by temporarily breaking drain and watching them fail.
notify: drain in-flight deliveries at shutdown (closes #106 )
2026-09-03 17:08:21 +00:00 · 2026-09-03 17:08:21 +00:00
5 changed files with 7 additions and 224 deletions
@@ -182,27 +182,6 @@ dnswatcher exposes a lightweight HTTP API for operational visibility:
 | `GET /api/v1/status`                  | Current monitoring state       |
 | `GET /metrics`                        | Prometheus metrics (optional)  |
 #### Server timeouts
 The HTTP server sets all four socket-level timeouts. These are compile-time
 constants in `internal/server/server.go`, not configurable via environment
 variables.
 | Timeout             | Value | Purpose                                       |
 |---------------------|-------|-----------------------------------------------|
 | `ReadHeaderTimeout` | 10s   | Bounds the request header read (slowloris)    |
 | `ReadTimeout`       | 15s   | Bounds the whole request read, headers + body |
 | `WriteTimeout`      | 75s   | Bounds handler execution plus response flush  |
 | `IdleTimeout`       | 120s  | Reaps idle keep-alive connections             |
 These are distinct from the 60s per-request handler budget applied by
 `chimw.Timeout` in `internal/server/routes.go`, which cancels the request
 context but does not touch the socket. `WriteTimeout` is deliberately
 larger than that budget: the write deadline is armed once request headers
 are read, so a smaller value would sever the connection before a handler
 using its full budget could respond. `IdleTimeout` exceeds common
 Prometheus scrape intervals so the scraper reuses its connection.
 ---
 ## Architecture
@@ -101,14 +101,6 @@ Rationale, Design, TODO, License, Author) if any are still missing.
  so shutdown cannot be extended indefinitely; an `OnStop` context that
  is already expired on entry with nothing outstanding drains quietly
  rather than warning about deliveries that were never abandoned
 - 2026-08-09: `http.Server` now sets all four socket-level timeouts
  (`ReadTimeout` 15s, `ReadHeaderTimeout` 10s, `WriteTimeout` 75s,
  `IdleTimeout` 120s) as named constants in `internal/server/server.go`,
  closing the slowloris / unreaped-keep-alive exposure required by
  `REPO_POLICIES.md` before 1.0; `WriteTimeout` is deliberately greater
  than the 60s `chimw.Timeout` handler budget so that budget stays
  reachable, and tests in `internal/server` pin both the non-zero
  values and that relationship (#99)
 - 2026-08-07: golangci-lint bumped to v2.12.2 (commit-pinned installs
  in `Dockerfile` and `script/bootstrap`); `.golangci.yml` set to the
  org-standard v2-schema config used across the org's repos
@@ -1,19 +0,0 @@
 package server
 import (
 	"net/http"
 	"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
@@ -33,52 +33,8 @@ type Params struct {
 // shutdownTimeout is how long to wait for graceful shutdown.
 const shutdownTimeout = 30 * time.Second
-// Socket-level timeouts for the HTTP server.
+// readHeaderTimeout is the max duration for reading request headers.
-//
+const readHeaderTimeout = 10 * time.Second
 // These bound time spent on the connection itself and are a distinct
 // control from the per-request handler budget enforced by
 // chimw.Timeout(requestTimeout) in routes.go: that one cancels the
 // request context after requestTimeout but never touches the socket,
 // so without the values below a peer can hold a connection open
 // forever (slowloris, unreaped keep-alives).
 //
 // The one hard constraint between the two controls is
 // writeTimeout > requestTimeout. net/http arms the write deadline
 // once the request headers have been read, so on a plaintext
 // connection it covers handler execution AND the response flush. If
 // writeTimeout were <= requestTimeout the server would sever the
 // connection before a handler that legitimately consumed its full
 // budget could emit anything, making the 60s budget unreachable in
 // practice. The margin between them is the response-flush allowance.
 //
 // The only clients of this service are browsers loading the dashboard
 // and a Prometheus scraper; the values are sized for those.
 const (
 	// readHeaderTimeout is the max duration for reading request
 	// headers.
 	readHeaderTimeout = 10 * time.Second
 	// readTimeout bounds reading the entire request, headers plus
 	// body. Every route here is a GET with no body, so this only
 	// ever needs to cover headers; the extra 5s over
 	// readHeaderTimeout is slack, not a real allowance, and keeps a
 	// body dribbled one byte at a time from holding the read side
 	// open indefinitely.
 	readTimeout = 15 * time.Second
 	// writeTimeout must exceed the requestTimeout handler budget
 	// (60s) per the note above. The 15s difference is the allowance
 	// for flushing a completed response to a slow client.
 	writeTimeout = 75 * time.Second
 	// idleTimeout reaps keep-alive connections between requests. It
 	// is deliberately longer than the common Prometheus scrape
 	// intervals (15s/30s/60s) so the scraper reuses its connection
 	// rather than reconnecting every cycle, while a browser tab
 	// left open on the dashboard stops occupying a connection
 	// within two minutes of going quiet.
 	idleTimeout = 120 * time.Second
 )
 // Server is the HTTP server.
 type Server struct {
@@ -120,29 +76,16 @@ func New(
 	return srv, nil
 }
 // newHTTPServer builds the listening http.Server with every
 // socket-level timeout set. All four are set deliberately: a zero
 // value in net/http means "no limit", not "some default".
 func newHTTPServer(
 	listenAddr string,
 	handler http.Handler,
 ) *http.Server {
 	return &http.Server{
 		Addr:              listenAddr,
 		Handler:           handler,
 		ReadTimeout:       readTimeout,
 		ReadHeaderTimeout: readHeaderTimeout,
 		WriteTimeout:      writeTimeout,
 		IdleTimeout:       idleTimeout,
 	}
 }
 // Run starts the HTTP server.
 func (s *Server) Run() {
 	s.SetupRoutes()
 	listenAddr := fmt.Sprintf(":%d", s.port)
-	s.httpServer = newHTTPServer(listenAddr, s)
+	s.httpServer = &http.Server{
 		Addr:              listenAddr,
 		Handler:           s,
 		ReadHeaderTimeout: readHeaderTimeout,
 	}
 	s.log.Info("http server starting", "addr", listenAddr)
@@ -1,112 +0,0 @@
 package server_test
 import (
 	"net/http"
 	"testing"
 	"sneak.berlin/go/dnswatcher/internal/server"
 )
 // 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)
 		},
 	)
 }
 // 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,
 		)
 	}
 	if srv.ReadHeaderTimeout <= 0 {
 		t.Errorf(
 			"ReadHeaderTimeout must be non-zero, got %v",
 			srv.ReadHeaderTimeout,
 		)
 	}
 	if srv.WriteTimeout <= 0 {
 		t.Errorf(
 			"WriteTimeout must be non-zero, got %v",
 			srv.WriteTimeout,
 		)
 	}
 	if srv.IdleTimeout <= 0 {
 		t.Errorf(
 			"IdleTimeout must be non-zero, got %v",
 			srv.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 {
 		t.Errorf(
 			"WriteTimeout (%v) must exceed handler budget (%v)",
 			srv.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 {
 		t.Errorf(
 			"ReadTimeout (%v) must be >= ReadHeaderTimeout (%v)",
 			srv.ReadTimeout,
 			srv.ReadHeaderTimeout,
 		)
 	}
 }
 // 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")
 	}
 }