Broadcast a session's QUIT once when its clients leave together (closes #154)

Logout deleted the client, counted the session's clients and ran
BroadcastQuit as separate statements, and BroadcastQuit read the peers,
parted, deleted empty channels and deleted the session separately too.
Two logouts or QUITs of one session at once both sent its QUIT, and a
JOIN landing between a part and the channel's deletion got into a +k
channel without the key.

db.QuitSession now does all of that in one transaction, parting each
channel as PartChannelAndDeleteIfEmpty does, and stores and queues the
QUIT in it only when it deleted the session, so the QUIT is queued
before the nick is free. db.LogoutClient deletes the client and counts
the rest in the same transaction.

Model: opus-5-5

internal/handlers/api_test.go

@@ -126,7 +126,6 @@ const (
 	privmsgCmd     = "PRIVMSG"
 	joinCmd        = "JOIN"
 	apiMessages    = "/api/v1/messages"
-	apiLogout      = "/api/v1/logout"
 	apiSession     = "/api/v1/session"
 	apiState       = "/api/v1/state"
 	authCookieName = "neoirc_auth"
@@ -137,8 +136,6 @@ type testServer struct {
 	httpServer *httptest.Server
 	t          *testing.T
 	fxApp      *fxtest.App
-	broker     *broker.Broker
-	database   *db.Database
 }
 
 func newTestServer(
@@ -165,11 +162,7 @@ func newTestServerWith(
 	)
 	copyTemplateDB(t, dbURL)
 
-	var (
+	var srv *server.Server
-		srv      *server.Server
-		brk      *broker.Broker
-		database *db.Database
-	)
 
 	app := fxtest.New(t,
 		fx.Provide(
@@ -202,14 +195,13 @@ func newTestServerWith(
 				return cfg, nil
 			},
 			newTestDB,
-			broker.New,
 			stats.New,
 			newTestHealthcheck,
 			newTestMiddleware,
 			newTestHandlers,
 			newTestServerFx,
 		),
-		fx.Populate(&srv, &brk, &database),
+		fx.Populate(&srv),
 	)
 
 	app.RequireStart()
@@ -225,8 +217,6 @@ func newTestServerWith(
 		httpServer: httpSrv,
 		t:          t,
 		fxApp:      app,
-		broker:     brk,
-		database:   database,
 	}
 }
 
@@ -301,8 +291,8 @@ func newTestHandlers(
 	database *db.Database,
 	hcheck *healthcheck.Healthcheck,
 	tracker *stats.Tracker,
-	brk *broker.Broker,
 ) (*handlers.Handlers, error) {
+	brk := broker.New()
 	svc := service.New(service.Params{ //nolint:exhaustruct
 		Logger:   log,
 		Config:   cfg,
@@ -335,7 +325,6 @@ func newTestServerFx(
 	cfg *config.Config,
 	mware *middleware.Middleware,
 	hdlr *handlers.Handlers,
-	shutdowner fx.Shutdowner,
 ) (*server.Server, error) {
 	srv, err := server.New(lifecycle, server.Params{ //nolint:exhaustruct
 		Logger:     log,
@@ -343,7 +332,6 @@ func newTestServerFx(
 		Config:     cfg,
 		Middleware: mware,
 		Handlers:   hdlr,
-		Shutdowner: shutdowner,
 	})
 	if err != nil {
 		return nil, fmt.Errorf("test server: %w", err)
@@ -1414,7 +1402,7 @@ const quitRaceTrials = 5
 // is sent once.
 func TestConcurrentLogoutsSendOneQuit(t *testing.T) {
 	checkQuitRace(t, func(tserver *testServer, tokens []string) {
-		tserver.postConcurrently(apiLogout, nil, tokens...)
+		tserver.postConcurrently("/api/v1/logout", nil, tokens...)
 	})
 }
 
@@ -1529,7 +1517,15 @@ func TestLogoutOfOneClientKeepsSession(t *testing.T) {
 		t.Fatalf("other client's state: got %d, want 200", status)
 	}
 
-	names := channelNames(state)
+	channels, _ := state["channels"].([]any)
+	names := make([]string, 0, len(channels))
+
+	for _, entry := range channels {
+		info, _ := entry.(map[string]any)
+		name, _ := info["name"].(string)
+		names = append(names, name)
+	}
+
 	if !slices.Equal(names, []string{channel}) {
 		t.Errorf("session's channels: got %v, want [%s]", names, channel)
 	}
@@ -1551,201 +1547,6 @@ func TestLogoutOfOneClientKeepsSession(t *testing.T) {
 	}
 }
 
-// TestFailedLogoutChangesNothing checks that a logout whose
-// session cannot be deleted answers 500 and changes
-// nothing: the client's token still works, the session is
-// still on its channel and its channel peer has no QUIT.
-func TestFailedLogoutChangesNothing(t *testing.T) {
-	const (
-		nick    = "stuckleaver"
-		channel = "#stuck"
-	)
-
-	tserver := newTestServer(t)
-	token := tserver.createSession(nick)
-	observerToken := tserver.createSession("stuckobserver")
-
-	for _, joiner := range []string{token, observerToken} {
-		tserver.sendCommand(joiner, map[string]any{
-			commandKey: joinCmd, toKey: channel,
-		})
-	}
-
-	_, lastID := tserver.pollMessages(observerToken, 0)
-
-	_, err := tserver.database.GetDB().ExecContext(t.Context(),
-		`CREATE TRIGGER keep_sessions
-		 BEFORE DELETE ON sessions
-		 BEGIN
-		   SELECT RAISE(ABORT, 'sessions are kept');
-		 END`)
-	if err != nil {
-		t.Fatal(err)
-	}
-
-	resp, err := doRequestAuth(
-		t, http.MethodPost, tserver.url(apiLogout), token, nil,
-	)
-	if err != nil {
-		t.Fatalf("logout: %v", err)
-	}
-
-	_ = resp.Body.Close()
-
-	if resp.StatusCode != http.StatusInternalServerError {
-		t.Errorf("logout: got %d, want 500", resp.StatusCode)
-	}
-
-	status, state := tserver.getState(token)
-	if status != http.StatusOK {
-		t.Fatalf("state after the failed logout: got %d, want 200", status)
-	}
-
-	names := channelNames(state)
-	if !slices.Equal(names, []string{channel}) {
-		t.Errorf("session's channels: got %v, want [%s]", names, channel)
-	}
-
-	msgs, _ := tserver.pollMessages(observerToken, lastID)
-
-	quits := countQuits(msgs, nick)
-	if quits != 0 {
-		t.Errorf("%d QUITs, want 0: %v", quits, msgs)
-	}
-}
-
-const (
-	// quitWakePollTimeout is the timeout, in seconds, of the
-	// long poll in checkQuitWakesPoll.
-	quitWakePollTimeout = 10
-
-	// quitWakeDeadline is how long checkQuitWakesPoll waits
-	// for that long poll to start waiting, and then to
-	// return once the session has left.
-	quitWakeDeadline = 5 * time.Second
-
-	// pollWaitStep is how often checkQuitWakesPoll checks
-	// whether the long poll is waiting yet.
-	pollWaitStep = 10 * time.Millisecond
-)
-
-// TestQuitWakesPeerLongPoll checks that a session's QUIT
-// wakes the long poll of a user who shares a channel with
-// it.
-func TestQuitWakesPeerLongPoll(t *testing.T) {
-	const reason = "gone fishing"
-
-	checkQuitWakesPoll(t, reason, func(tserver *testServer, token string) {
-		tserver.sendCommand(token, map[string]any{
-			commandKey: irc.CmdQuit, bodyKey: []string{reason},
-		})
-	})
-}
-
-// TestLastLogoutWakesPeerLongPoll checks that the logout of
-// a session's last client wakes the long poll of a user who
-// shares a channel with it.
-func TestLastLogoutWakesPeerLongPoll(t *testing.T) {
-	checkQuitWakesPoll(t, "Connection closed", func(tserver *testServer, token string) {
-		tserver.logout(token)
-	})
-}
-
-// checkQuitWakesPoll puts a session and a peer on a
-// channel, and calls leave with the session's token while
-// the peer long-polls with a timeout of quitWakePollTimeout
-// seconds. It checks that the poll returns within
-// quitWakeDeadline with the session's QUIT, whose body is
-// reason.
-func checkQuitWakesPoll(
-	t *testing.T,
-	reason string,
-	leave func(tserver *testServer, token string),
-) {
-	t.Helper()
-
-	const (
-		nick     = "wakequitter"
-		peerNick = "wakepeer"
-	)
-
-	tserver := newTestServer(t)
-	token := tserver.createSession(nick)
-	peerToken := tserver.createSession(peerNick)
-
-	for _, joiner := range []string{token, peerToken} {
-		tserver.sendCommand(joiner, map[string]any{
-			commandKey: joinCmd, toKey: "#wake",
-		})
-	}
-
-	_, lastID := tserver.pollMessages(peerToken, 0)
-
-	peerID, err := tserver.database.GetSessionByNick(t.Context(), peerNick)
-	if err != nil {
-		t.Fatalf("get session of %s: %v", peerNick, err)
-	}
-
-	var (
-		msgs    []map[string]any
-		pollErr error
-	)
-
-	polled := make(chan struct{})
-
-	go func() {
-		defer close(polled)
-
-		msgs, pollErr = tserver.longPoll(peerToken, lastID, quitWakePollTimeout)
-	}()
-
-	deadline := time.Now().Add(quitWakeDeadline)
-
-	for tserver.broker.ListenerCount(peerID) == 0 {
-		if time.Now().After(deadline) {
-			t.Fatal("the long poll did not start waiting")
-		}
-
-		time.Sleep(pollWaitStep)
-	}
-
-	leave(tserver, token)
-
-	select {
-	case <-polled:
-	case <-time.After(quitWakeDeadline):
-		t.Fatalf("the long poll did not return within %v", quitWakeDeadline)
-	}
-
-	if pollErr != nil {
-		t.Fatalf("long poll: %v", pollErr)
-	}
-
-	if len(msgs) != 1 || countQuits(msgs, nick) != 1 {
-		t.Fatalf("long poll: got %v, want one QUIT from %s", msgs, nick)
-	}
-
-	lines, _ := msgs[0][bodyKey].([]any)
-	if len(lines) != 1 || lines[0] != reason {
-		t.Errorf("QUIT body: got %v, want [%s]", lines, reason)
-	}
-}
-
-// channelNames returns the names of the channels in state,
-// a response to GET /api/v1/state.
-func channelNames(state map[string]any) []string {
-	channels, _ := state["channels"].([]any)
-	names := make([]string, 0, len(channels))
-
-	for _, entry := range channels {
-		info, _ := entry.(map[string]any)
-		name, _ := info["name"].(string)
-		names = append(names, name)
-	}
-
-	return names
-}
-
 // countQuits returns how many of msgs are a QUIT from nick.
 func countQuits(msgs []map[string]any, nick string) int {
 	quits := 0
@@ -1766,7 +1567,7 @@ func (tserver *testServer) logout(token string) {
 
 	resp, err := doRequestAuth(
 		tserver.t, http.MethodPost,
-		tserver.url(apiLogout), token, nil,
+		tserver.url("/api/v1/logout"), token, nil,
 	)
 	if err != nil {
 		tserver.t.Fatalf("logout: %v", err)
@@ -1779,38 +1580,6 @@ func (tserver *testServer) logout(token string) {
 	}
 }
 
-// longPoll returns the messages after afterID that the
-// client token has been sent, waiting up to timeout seconds
-// for one if there are none yet. It returns errors instead
-// of failing the test, so a goroutine can call it.
-func (tserver *testServer) longPoll(
-	token string, afterID int64, timeout int,
-) ([]map[string]any, error) {
-	resp, err := doRequestAuth(
-		tserver.t, http.MethodGet,
-		tserver.url(fmt.Sprintf(
-			"%s?timeout=%d&after=%d", apiMessages, timeout, afterID,
-		)),
-		token, nil,
-	)
-	if err != nil {
-		return nil, err
-	}
-
-	defer func() { _ = resp.Body.Close() }()
-
-	var result struct {
-		Messages []map[string]any `json:"messages"`
-	}
-
-	err = json.NewDecoder(resp.Body).Decode(&result)
-	if err != nil {
-		return nil, fmt.Errorf("decode poll: %w", err)
-	}
-
-	return result.Messages, nil
-}
-
 // login logs in to the session nick with testPassword and
 // returns the new client's token.
 func (tserver *testServer) login(nick string) string {

internal/server/export_test.go

@@ -1,10 +1,6 @@
 package server
 
-import (
+import "github.com/go-chi/chi/v5"
-	"net"
-
-	"github.com/go-chi/chi/v5"
-)
 
 // NewTestSPAServer creates a Server whose router serves only
 // the embedded web client.
@@ -14,9 +10,3 @@ func NewTestSPAServer() *Server {
 
 	return srv
 }
-
-// ServeUntilShutdown runs serveUntilShutdown, which serves the
-// HTTP server on listener, and returns when it does.
-func (srv *Server) ServeUntilShutdown(listener net.Listener) {
-	srv.serveUntilShutdown(listener)
-}

internal/server/server.go

@@ -6,7 +6,6 @@ import (
 	"errors"
 	"fmt"
 	"log/slog"
-	"net"
 	"net/http"
 	"os"
 	"os/signal"
@@ -40,7 +39,6 @@ type Params struct {
 	Config     *config.Config
 	Middleware *middleware.Middleware
 	Handlers   *handlers.Handlers
-	Shutdowner fx.Shutdowner
 }
 
 // Server is the main HTTP server.
@@ -68,7 +66,7 @@ func New(
 	}
 
 	lifecycle.Append(fx.Hook{
-		OnStart: func(ctx context.Context) error {
+		OnStart: func(_ context.Context) error {
 			srv.startupTime = time.Now()
 
 			// Build the router before OnStart returns, so srv can
@@ -80,7 +78,9 @@ func New(
 			// A closed stdout or stderr must not end the process.
 			signal.Ignore(syscall.SIGPIPE)
 
-			return srv.startHTTP(ctx)
+			srv.startHTTP()
+
+			return nil
 		},
 		OnStop: func(ctx context.Context) error {
 			srv.cleanShutdown(ctx)
@@ -162,23 +162,15 @@ func (srv *Server) configure() {
 	// Server configuration placeholder.
 }
 
-// startHTTP binds the HTTP listen address, so that a port
+// startHTTP creates the HTTP server and serves it from a new
-// that is taken fails startup, then serves the HTTP server
+// goroutine. srv.httpServer is set before that goroutine
-// from a new goroutine. srv.httpServer is set before that
+// starts and never changes, so it and cleanShutdown read it
-// goroutine starts and never changes, so it and cleanShutdown
+// without a lock.
-// read it without a lock.
+func (srv *Server) startHTTP() {
-func (srv *Server) startHTTP(ctx context.Context) error {
 	listenAddr := fmt.Sprintf(
 		":%d", srv.params.Config.Port,
 	)
 
-	var listenConfig net.ListenConfig
-
-	listener, err := listenConfig.Listen(ctx, "tcp", listenAddr)
-	if err != nil {
-		return fmt.Errorf("http listen: %w", err)
-	}
-
 	srv.httpServer = &http.Server{ //nolint:exhaustruct // optional fields
 		Addr:           listenAddr,
 		ReadTimeout:    httpReadTimeout,
@@ -187,30 +179,16 @@ func (srv *Server) startHTTP(ctx context.Context) error {
 		Handler:        srv,
 	}
 
-	go func() {
+	go srv.serveUntilShutdown()
-		srv.serveUntilShutdown(listener)
-	}()
-
-	return nil
 }
 
-// serveUntilShutdown serves the HTTP server on listener until
+func (srv *Server) serveUntilShutdown() {
-// cleanShutdown stops it. If serving fails before then, it
-// stops the app with exit code 1.
-func (srv *Server) serveUntilShutdown(listener net.Listener) {
 	srv.log.Info(
 		"http begin listen", "listenaddr", srv.httpServer.Addr,
 	)
 
-	err := srv.httpServer.Serve(listener)
+	err := srv.httpServer.ListenAndServe()
-	if errors.Is(err, http.ErrServerClosed) {
+	if err != nil && !errors.Is(err, http.ErrServerClosed) {
-		return
+		srv.log.Error("listen error", "error", err)
-	}
-
-	srv.log.Error("listen error", "error", err)
-
-	err = srv.params.Shutdowner.Shutdown(fx.ExitCode(1))
-	if err != nil {
-		srv.log.Error("shutdown failed", "error", err)
 	}
 }

internal/server/server_test.go

@@ -1,7 +1,3 @@
-// Tests use a global viper instance for configuration,
-// making parallel execution unsafe.
-//
-//nolint:paralleltest
 package server_test
 
 import (
@@ -10,7 +6,6 @@ import (
 	"net"
 	"net/http"
 	"strconv"
-	"strings"
 	"testing"
 	"time"
 
@@ -45,6 +40,8 @@ const (
 // 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()
+
 	port := freePort(t)
 	addr := net.JoinHostPort("127.0.0.1", strconv.Itoa(port))
 
@@ -106,114 +103,12 @@ func TestStopWaitsForRequestInProgress(t *testing.T) {
 	}
 }
 
-// TestStartFailsWhenHTTPPortIsTaken checks that starting the
+// startApp starts the server with the dependencies
-// app, which runs the start hook server.New registers,
+// cmd/neoircd gives it, on a fresh in-memory database, with
-// returns an error naming the HTTP listen address when
+// the HTTP server listening on port.
-// something else is already listening on its port.
-func TestStartFailsWhenHTTPPortIsTaken(t *testing.T) {
-	var listenConfig net.ListenConfig
-
-	taken, err := listenConfig.Listen(t.Context(), "tcp", ":0")
-	if err != nil {
-		t.Fatalf("listen: %v", err)
-	}
-
-	t.Cleanup(func() { _ = taken.Close() })
-
-	tcpAddr, ok := taken.Addr().(*net.TCPAddr)
-	if !ok {
-		t.Fatalf("listener address %v is not TCP", taken.Addr())
-	}
-
-	app, _, _ := newApp(t, tcpAddr.Port)
-
-	err = app.Start(t.Context())
-	if err == nil {
-		app.RequireStop()
-		t.Fatal("the app started while its HTTP port was taken")
-	}
-
-	listenAddr := fmt.Sprintf(":%d", tcpAddr.Port)
-	if !strings.Contains(err.Error(), listenAddr) {
-		t.Errorf("start error %q does not name %s", err, listenAddr)
-	}
-}
-
-// TestServeFailureStopsApp checks that when serving HTTP fails
-// after the start hook has bound the listen address, the
-// server stops the app with exit code 1.
-func TestServeFailureStopsApp(t *testing.T) {
-	var srv *server.Server
-
-	app, _, _ := startApp(t, freePort(t), fx.Populate(&srv))
-
-	shutdowns := app.Wait()
-
-	srv.ServeUntilShutdown(closedListener(t))
-
-	select {
-	case shutdown := <-shutdowns:
-		if shutdown.ExitCode != 1 {
-			t.Errorf("exit code %d, want 1", shutdown.ExitCode)
-		}
-	default:
-		t.Error("the server did not stop the app")
-	}
-}
-
-// TestServeAfterStopDoesNotStopApp checks that serving HTTP
-// after the app has stopped, which ends at once because the
-// stop hook has shut the HTTP server down, does not stop the
-// app. Serving ends the same way on every normal stop.
-func TestServeAfterStopDoesNotStopApp(t *testing.T) {
-	var srv *server.Server
-
-	app, _, _ := startApp(t, freePort(t), fx.Populate(&srv))
-
-	shutdowns := app.Wait()
-
-	app.RequireStop()
-
-	srv.ServeUntilShutdown(closedListener(t))
-
-	select {
-	case shutdown := <-shutdowns:
-		t.Errorf(
-			"the server stopped the app with exit code %d",
-			shutdown.ExitCode,
-		)
-	default:
-	}
-}
-
-// startApp starts the app newApp builds, with the HTTP server
-// listening on port.
 func startApp(
 	t *testing.T,
 	port int,
-	options ...fx.Option,
-) (*fxtest.App, *broker.Broker, *db.Database) {
-	t.Helper()
-
-	app, brk, database := newApp(t, port, options...)
-
-	app.RequireStart()
-
-	// Stops the app if the test ends before stopping it.
-	t.Cleanup(app.RequireStop)
-
-	return app, brk, database
-}
-
-// newApp builds, without starting it, an app that runs the
-// server with the dependencies cmd/neoircd gives it, on a
-// fresh in-memory database, with the HTTP port set to port.
-// options are added to the app, for a test that needs more
-// from it than the broker and the database.
-func newApp(
-	t *testing.T,
-	port int,
-	options ...fx.Option,
 ) (*fxtest.App, *broker.Broker, *db.Database) {
 	t.Helper()
 
@@ -251,9 +146,13 @@ func newApp(
 		}),
 		fx.Invoke(func(*server.Server) {}),
 		fx.Populate(&brk, &database),
-		fx.Options(options...),
 	)
 
+	app.RequireStart()
+
+	// Stops the app if the test ends before stopping it.
+	t.Cleanup(app.RequireStop)
+
 	return app, brk, database
 }
 
@@ -262,21 +161,6 @@ func newApp(
 func freePort(t *testing.T) int {
 	t.Helper()
 
-	listener := closedListener(t)
-
-	tcpAddr, ok := listener.Addr().(*net.TCPAddr)
-	if !ok {
-		t.Fatalf("listener address %v is not TCP", listener.Addr())
-	}
-
-	return tcpAddr.Port
-}
-
-// closedListener returns a TCP listener that has already been
-// closed, so that accepting a connection on it fails.
-func closedListener(t *testing.T) net.Listener {
-	t.Helper()
-
 	var listenConfig net.ListenConfig
 
 	listener, err := listenConfig.Listen(
@@ -286,12 +170,17 @@ func closedListener(t *testing.T) net.Listener {
 		t.Fatalf("listen: %v", err)
 	}
 
+	tcpAddr, ok := listener.Addr().(*net.TCPAddr)
+	if !ok {
+		t.Fatalf("listener address %v is not TCP", listener.Addr())
+	}
+
 	err = listener.Close()
 	if err != nil {
 		t.Fatalf("close listener: %v", err)
 	}
 
-	return listener
+	return tcpAddr.Port
 }
 
 // canConnect reports whether addr accepts a TCP connection.

internal/service/service.go

@@ -829,10 +829,9 @@ func (s *Service) ChangeNick(
 // BroadcastQuit parts the session from all its channels,
 // deletes it, and queues a QUIT to the users who shared a
 // channel with it, in one transaction, so that the QUIT is
-// queued before anyone else can take the nick, and then
+// queued before anyone else can take the nick. Of several
-// wakes those users' long polls. Of several calls for one
+// calls for one session at once, only the one that deletes
-// session at once, only the one that deletes it sends the
+// it sends the QUIT.
-// QUIT.
 func (s *Service) BroadcastQuit(
 	ctx context.Context,
 	sessionID int64,
@@ -854,8 +853,6 @@ func (s *Service) BroadcastQuit(
 
 // Logout deletes a client of the session and, if that was
 // the session's last client, does what BroadcastQuit does.
-// If that fails, nothing changes and Logout returns the
-// error.
 func (s *Service) Logout(
 	ctx context.Context,
 	sessionID, clientID int64,