Wait for IRC listener cleanup and HTTP shutdown on stop (closes #159)

Stopping the server returned before the IRC listener's connections had
run their cleanup, and the database closed under them, so their
sessions, clients and channel memberships stayed and their nicks stayed
taken after a restart.

The IRC listener's stop now waits up to five seconds for every
connection's cleanup, which now runs on a context that stopping does
not cancel. The HTTP server's stop hook now runs Shutdown itself and
waits for it; httpServer is set before serving starts, and the server's
own signal watcher, which raced fx's stop hooks, is gone.

Model: opus-5-5

internal/ircserver/export_test.go

@@ -85,6 +85,10 @@ const PingInterval = pingInterval
 // for a line before it closes the connection.
 const PongDeadline = pongDeadline
 
+// StopTimeout is the longest Stop waits for the connections'
+// cleanup.
+const StopTimeout = stopTimeout
+
 // NewTestConn returns a Conn for nick on tcpConn, without a
 // server, database or service behind it.
 func NewTestConn(

internal/ircserver/server.go

@@ -16,8 +16,8 @@ import (
 	"sneak.berlin/go/neoirc/internal/service"
 )
 
-// stopTimeout is how long stop waits for the connections'
-// cleanup, which ends their sessions, before it returns.
+// stopTimeout is the longest stop waits for the connections'
+// cleanup, which ends their sessions.
 const stopTimeout = 5 * time.Second
 
 // Params defines the dependencies for creating an IRC

internal/ircserver/server_test.go

@@ -886,6 +886,44 @@ func TestStopEndsSessions(t *testing.T) {
 	t.Errorf("peer was not sent the QUIT of shutirc: %v", msgs)
 }
 
+// TestStopReturnsWhileCleanupIsStuck checks that Stop stops
+// waiting for a connection's cleanup after StopTimeout, so a
+// cleanup that cannot finish does not hold up stopping.
+func TestStopReturnsWhileCleanupIsStuck(t *testing.T) {
+	t.Parallel()
+
+	env := newTestEnv(t)
+	client := env.dial(t)
+
+	client.register("stuck")
+
+	// The test database has one connection. While the test
+	// holds it, the cleanup's first query waits for it.
+	held, err := env.database.GetDB().Conn(t.Context())
+	if err != nil {
+		t.Fatalf("take the database connection: %v", err)
+	}
+
+	stopped := make(chan struct{})
+
+	go func() {
+		env.srv.Stop()
+		close(stopped)
+	}()
+
+	select {
+	case <-stopped:
+	case <-time.After(ircserver.StopTimeout + testTimeout):
+		t.Error("Stop did not return while a cleanup was stuck")
+	}
+
+	// Lets the cleanup finish before the database is closed.
+	err = held.Close()
+	if err != nil {
+		t.Errorf("release the database connection: %v", err)
+	}
+}
+
 func TestTopicSetAndGet(t *testing.T) {
 	t.Parallel()
 

internal/server/export_test.go

@@ -1,13 +1,6 @@
 package server
 
-import (
-	"context"
-	"log/slog"
-	"net/http"
-
-	"github.com/go-chi/chi/v5"
-	"sneak.berlin/go/neoirc/internal/config"
-)
+import "github.com/go-chi/chi/v5"
 
 // NewTestSPAServer creates a Server whose router serves only
 // the embedded web client.
@@ -17,28 +10,3 @@ func NewTestSPAServer() *Server {
 
 	return srv
 }
-
-// NewTestHTTPServer creates a Server whose router serves
-// handler at / and whose HTTP server listens on port once
-// Start is called.
-func NewTestHTTPServer(port int, handler http.HandlerFunc) *Server {
-	router := chi.NewRouter()
-	router.Get("/", handler)
-
-	return &Server{
-		log:    slog.New(slog.DiscardHandler),
-		router: router,
-		params: Params{Config: &config.Config{Port: port}},
-	}
-}
-
-// Start starts the HTTP server, as the start hook does once
-// the router is built.
-func (srv *Server) Start() {
-	srv.startHTTP()
-}
-
-// Stop does what the stop hook does.
-func (srv *Server) Stop(ctx context.Context) {
-	srv.cleanShutdown(ctx)
-}

internal/server/server_test.go

@@ -1,86 +1,161 @@
 package server_test
 
 import (
+	"crypto/rand"
+	"fmt"
 	"net"
 	"net/http"
 	"strconv"
 	"testing"
 	"time"
 
+	"go.uber.org/fx"
+	"go.uber.org/fx/fxtest"
+	"sneak.berlin/go/neoirc/internal/broker"
+	"sneak.berlin/go/neoirc/internal/config"
+	"sneak.berlin/go/neoirc/internal/db"
+	"sneak.berlin/go/neoirc/internal/globals"
+	"sneak.berlin/go/neoirc/internal/handlers"
+	"sneak.berlin/go/neoirc/internal/healthcheck"
+	"sneak.berlin/go/neoirc/internal/logger"
+	"sneak.berlin/go/neoirc/internal/middleware"
 	"sneak.berlin/go/neoirc/internal/server"
+	"sneak.berlin/go/neoirc/internal/service"
+	"sneak.berlin/go/neoirc/internal/stats"
 )
 
 const (
 	testTimeout = 5 * time.Second
 
-	// listenCheckStep is how often waitUntilListening tries
-	// to connect.
-	listenCheckStep = 10 * time.Millisecond
+	// checkStep is how often waitUntil checks again.
+	checkStep = 10 * time.Millisecond
 
-	// stopHoldTime is how long a test waits to see that Stop
-	// has not returned.
+	// stopHoldTime is how long a test waits to see that the
+	// app has not stopped.
 	stopHoldTime = 200 * time.Millisecond
 )
 
 // TestStopWaitsForRequestInProgress checks that stopping the
-// server waits for a request it is serving to finish, and
-// that the request gets its response.
+// app, which runs the stop hook server.New registers, waits
+// for a long poll the HTTP server is serving to finish, and
+// that the poll gets its response.
 func TestStopWaitsForRequestInProgress(t *testing.T) {
 	t.Parallel()
 
-	started := make(chan struct{})
-	release := make(chan struct{})
-
 	port := freePort(t)
 	addr := net.JoinHostPort("127.0.0.1", strconv.Itoa(port))
 
-	srv := server.NewTestHTTPServer(
-		port,
-		func(writer http.ResponseWriter, _ *http.Request) {
-			close(started)
-			<-release
-			writer.WriteHeader(http.StatusNoContent)
-		},
-	)
-	srv.Start()
+	app, brk, database := startApp(t, port)
 
-	waitUntilListening(t, addr)
+	sessionID, _, token, err := database.CreateSession(
+		t.Context(), "poller", "poller", "", "",
+	)
+	if err != nil {
+		t.Fatalf("create session: %v", err)
+	}
+
+	waitUntil(t, "the HTTP server to listen", func() bool {
+		return canConnect(t, addr)
+	})
 
 	statuses := make(chan int, 1)
 
 	go func() {
-		statuses <- get(t, "http://"+addr+"/")
+		statuses <- getAuthenticated(
+			t, "http://"+addr+"/api/v1/messages?timeout=30", token,
+		)
 	}()
 
-	<-started
+	waitUntil(t, "the long poll to wait for a message", func() bool {
+		return brk.ListenerCount(sessionID) == 1
+	})
+
+	var stopErr error
 
 	stopped := make(chan struct{})
 
 	go func() {
-		srv.Stop(t.Context())
+		stopErr = app.Stop(t.Context())
+
 		close(stopped)
 	}()
 
 	select {
 	case <-stopped:
-		t.Error("Stop returned while a request was in progress")
+		t.Error("the app stopped while a request was in progress")
 	case <-time.After(stopHoldTime):
 	}
 
-	close(release)
+	brk.Notify(sessionID)
 
 	status := <-statuses
-	if status != http.StatusNoContent {
-		t.Errorf("status %d, want %d", status, http.StatusNoContent)
+	if status != http.StatusOK {
+		t.Errorf("status %d, want %d", status, http.StatusOK)
 	}
 
 	select {
 	case <-stopped:
+		if stopErr != nil {
+			t.Errorf("stop: %v", stopErr)
+		}
 	case <-time.After(testTimeout):
-		t.Fatal("Stop did not return after the request finished")
+		t.Fatal("the app did not stop after the request finished")
 	}
 }
 
+// startApp starts the server with the dependencies
+// cmd/neoircd gives it, on a fresh in-memory database, with
+// the HTTP server listening on port.
+func startApp(
+	t *testing.T,
+	port int,
+) (*fxtest.App, *broker.Broker, *db.Database) {
+	t.Helper()
+
+	var (
+		brk      *broker.Broker
+		database *db.Database
+	)
+
+	app := fxtest.New(t,
+		fx.Supply(&globals.Globals{
+			Appname:   "neoirc-test",
+			Version:   "test",
+			StartTime: time.Now(),
+		}),
+		fx.Provide(
+			broker.New,
+			config.New,
+			db.New,
+			handlers.New,
+			healthcheck.New,
+			logger.New,
+			middleware.New,
+			server.New,
+			service.New,
+			stats.New,
+		),
+		fx.Decorate(func(cfg *config.Config) *config.Config {
+			// A random name, so that no other test can share it.
+			cfg.DBURL = fmt.Sprintf(
+				"file:%s?mode=memory&cache=shared", rand.Text(),
+			)
+			cfg.Port = port
+
+			return cfg
+		}),
+		fx.Invoke(func(*server.Server) {}),
+		fx.Populate(&brk, &database),
+	)
+
+	app.RequireStart()
+
+	// Stops the app if the test ends before stopping it.
+	t.Cleanup(app.RequireStop)
+
+	return app, brk, database
+}
+
 // freePort returns a TCP port that nothing was listening on a
 // moment ago.
 func freePort(t *testing.T) int {
@@ -108,34 +183,42 @@ func freePort(t *testing.T) int {
 	return tcpAddr.Port
 }
 
-// waitUntilListening waits until addr accepts connections,
-// and fails the test if it does not within testTimeout.
-func waitUntilListening(t *testing.T, addr string) {
+// canConnect reports whether addr accepts a TCP connection.
+func canConnect(t *testing.T, addr string) bool {
 	t.Helper()
 
 	var dialer net.Dialer
 
+	conn, err := dialer.DialContext(t.Context(), "tcp", addr)
+	if err != nil {
+		return false
+	}
+
+	_ = conn.Close()
+
+	return true
+}
+
+// waitUntil calls done every checkStep until it returns true,
+// and fails the test if it does not within testTimeout.
+func waitUntil(t *testing.T, what string, done func() bool) {
+	t.Helper()
+
 	deadline := time.Now().Add(testTimeout)
 
-	for {
-		conn, err := dialer.DialContext(t.Context(), "tcp", addr)
-		if err == nil {
-			_ = conn.Close()
-
-			return
-		}
-
+	for !done() {
 		if time.Now().After(deadline) {
-			t.Fatalf("nothing listening on %s: %v", addr, err)
+			t.Fatalf("timed out waiting for %s", what)
 		}
 
-		time.Sleep(listenCheckStep)
+		time.Sleep(checkStep)
 	}
 }
 
-// get sends a GET request to url and returns the response
-// status, or 0 if there was no response.
-func get(t *testing.T, url string) int {
+// getAuthenticated sends a GET request to url as the session
+// whose auth token is token, and returns the response status,
+// or 0 if there was no response.
+func getAuthenticated(t *testing.T, url, token string) int {
 	t.Helper()
 
 	request, err := http.NewRequestWithContext(
@@ -147,6 +230,8 @@ func get(t *testing.T, url string) int {
 		return 0
 	}
 
+	request.Header.Set("Cookie", "neoirc_auth="+token)
+
 	client := http.Client{Timeout: testTimeout}
 
 	response, err := client.Do(request)