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.go

@@ -3395,15 +3395,11 @@ func (hdlr *Handlers) HandleLogout() http.HandlerFunc {
 			return
 		}
 
-		ctx := request.Context()
-
-		err := hdlr.params.Database.DeleteClient(
-			ctx, clientID,
+		err := hdlr.svc.Logout(
+			request.Context(), sessionID, clientID, nick,
 		)
 		if err != nil {
-			hdlr.log.Error(
-				"delete client failed", "error", err,
-			)
+			hdlr.log.Error("logout failed", "error", err)
 			hdlr.respondError(
 				writer, request,
 				"internal error",
@@ -3413,23 +3409,6 @@ func (hdlr *Handlers) HandleLogout() http.HandlerFunc {
 			return
 		}
 
-		// If no clients remain, clean up the user fully:
-		// part all channels (notifying members) and
-		// delete the session.
-		remaining, err := hdlr.params.Database.
-			ClientCountForSession(ctx, sessionID)
-		if err != nil {
-			hdlr.log.Error(
-				"client count check failed", "error", err,
-			)
-		}
-
-		if remaining == 0 {
-			hdlr.cleanupUser(
-				ctx, sessionID, nick,
-			)
-		}
-
 		hdlr.clearAuthCookie(writer)
 
 		hdlr.respondJSON(writer, request,

internal/handlers/api_test.go

@@ -126,6 +126,7 @@ const (
 	privmsgCmd     = "PRIVMSG"
 	joinCmd        = "JOIN"
 	apiMessages    = "/api/v1/messages"
+	apiLogout      = "/api/v1/logout"
 	apiSession     = "/api/v1/session"
 	apiState       = "/api/v1/state"
 	authCookieName = "neoirc_auth"
@@ -136,6 +137,8 @@ type testServer struct {
 	httpServer *httptest.Server
 	t          *testing.T
 	fxApp      *fxtest.App
+	broker     *broker.Broker
+	database   *db.Database
 }
 
 func newTestServer(
@@ -162,7 +165,11 @@ func newTestServerWith(
 	)
 	copyTemplateDB(t, dbURL)
 
-	var srv *server.Server
+	var (
+		srv      *server.Server
+		brk      *broker.Broker
+		database *db.Database
+	)
 
 	app := fxtest.New(t,
 		fx.Provide(
@@ -195,13 +202,14 @@ func newTestServerWith(
 				return cfg, nil
 			},
 			newTestDB,
+			broker.New,
 			stats.New,
 			newTestHealthcheck,
 			newTestMiddleware,
 			newTestHandlers,
 			newTestServerFx,
 		),
-		fx.Populate(&srv),
+		fx.Populate(&srv, &brk, &database),
 	)
 
 	app.RequireStart()
@@ -217,6 +225,8 @@ func newTestServerWith(
 		httpServer: httpSrv,
 		t:          t,
 		fxApp:      app,
+		broker:     brk,
+		database:   database,
 	}
 }
 
@@ -291,8 +301,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,
@@ -1395,6 +1405,486 @@ func TestQuit(t *testing.T) {
 	}
 }
 
+// quitRaceTrials is how many times checkQuitRace repeats
+// its race.
+const quitRaceTrials = 5
+
+// TestConcurrentLogoutsSendOneQuit checks that when both
+// clients of a session log out at once, the session's QUIT
+// is sent once.
+func TestConcurrentLogoutsSendOneQuit(t *testing.T) {
+	checkQuitRace(t, func(tserver *testServer, tokens []string) {
+		tserver.postConcurrently(apiLogout, nil, tokens...)
+	})
+}
+
+// TestConcurrentQuitsSendOneQuit checks that when both
+// clients of a session send QUIT at once, the session's
+// QUIT is sent once.
+func TestConcurrentQuitsSendOneQuit(t *testing.T) {
+	quit, err := json.Marshal(map[string]any{commandKey: irc.CmdQuit})
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	checkQuitRace(t, func(tserver *testServer, tokens []string) {
+		tserver.postConcurrently(apiMessages, quit, tokens...)
+	})
+}
+
+// checkQuitRace gives a session a password and two
+// clients, puts it on a channel with a second session, and
+// calls leave with the tokens of the two clients. It checks
+// that the second session then holds one QUIT from the
+// first, and that the first session is deleted: its tokens
+// are refused and its nick is free. It does all this
+// quitRaceTrials times, each on a new server, because each
+// trial logs in and logins are rate limited.
+func checkQuitRace(
+	t *testing.T,
+	leave func(tserver *testServer, tokens []string),
+) {
+	t.Helper()
+
+	const nick = "racingquitter"
+
+	for trial := range quitRaceTrials {
+		tserver := newTestServer(t)
+		firstToken := tserver.createSession(nick)
+		observerToken := tserver.createSession("raceobserver")
+
+		tserver.sendCommand(firstToken, map[string]any{
+			commandKey: irc.CmdPass, bodyKey: []string{testPassword},
+		})
+
+		tokens := []string{firstToken, tserver.login(nick)}
+
+		for _, token := range []string{firstToken, observerToken} {
+			tserver.sendCommand(token, map[string]any{
+				commandKey: joinCmd, toKey: "#quitrace",
+			})
+		}
+
+		_, lastID := tserver.pollMessages(observerToken, 0)
+
+		leave(tserver, tokens)
+
+		msgs, _ := tserver.pollMessages(observerToken, lastID)
+
+		quits := countQuits(msgs, nick)
+		if quits != 1 {
+			t.Errorf("trial %d: %d QUITs, want 1: %v", trial, quits, msgs)
+		}
+
+		for _, token := range tokens {
+			status, _ := tserver.getState(token)
+			if status != http.StatusUnauthorized {
+				t.Errorf("trial %d: state got %d, want 401", trial, status)
+			}
+		}
+
+		// createSession fails the test if the nick is taken.
+		tserver.createSession(nick)
+	}
+}
+
+// TestLogoutOfOneClientKeepsSession checks that when one of
+// a session's two clients logs out, the other client's
+// token still works, the session stays on its channel and
+// its channel peer gets no QUIT, and that the other
+// client's logout then sends one QUIT.
+func TestLogoutOfOneClientKeepsSession(t *testing.T) {
+	const (
+		nick    = "twoclients"
+		channel = "#logouts"
+	)
+
+	tserver := newTestServer(t)
+	firstToken := tserver.createSession(nick)
+	observerToken := tserver.createSession("logoutobserver")
+
+	tserver.sendCommand(firstToken, map[string]any{
+		commandKey: irc.CmdPass, bodyKey: []string{testPassword},
+	})
+
+	secondToken := tserver.login(nick)
+
+	for _, token := range []string{firstToken, observerToken} {
+		tserver.sendCommand(token, map[string]any{
+			commandKey: joinCmd, toKey: channel,
+		})
+	}
+
+	_, lastID := tserver.pollMessages(observerToken, 0)
+
+	tserver.logout(firstToken)
+
+	status, _ := tserver.getState(firstToken)
+	if status != http.StatusUnauthorized {
+		t.Errorf("logged-out client's state: got %d, want 401", status)
+	}
+
+	status, state := tserver.getState(secondToken)
+	if status != http.StatusOK {
+		t.Fatalf("other client's state: 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, lastID := tserver.pollMessages(observerToken, lastID)
+
+	quits := countQuits(msgs, nick)
+	if quits != 0 {
+		t.Errorf("after the first logout: %d QUITs, want 0: %v", quits, msgs)
+	}
+
+	tserver.logout(secondToken)
+
+	msgs, _ = tserver.pollMessages(observerToken, lastID)
+
+	quits = countQuits(msgs, nick)
+	if quits != 1 {
+		t.Errorf("after the second logout: %d QUITs, want 1: %v", quits, msgs)
+	}
+}
+
+// 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
+
+	for _, msg := range msgs {
+		if msg[commandKey] == irc.CmdQuit && msg["from"] == nick {
+			quits++
+		}
+	}
+
+	return quits
+}
+
+// logout logs out the client token and fails the test
+// unless the server answers 200.
+func (tserver *testServer) logout(token string) {
+	tserver.t.Helper()
+
+	resp, err := doRequestAuth(
+		tserver.t, http.MethodPost,
+		tserver.url(apiLogout), token, nil,
+	)
+	if err != nil {
+		tserver.t.Fatalf("logout: %v", err)
+	}
+
+	defer func() { _ = resp.Body.Close() }()
+
+	if resp.StatusCode != http.StatusOK {
+		tserver.t.Fatalf("logout: status %d", resp.StatusCode)
+	}
+}
+
+// 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 {
+	tserver.t.Helper()
+
+	body, err := json.Marshal(map[string]string{
+		nickKey: nick, passwordKey: testPassword,
+	})
+	if err != nil {
+		tserver.t.Fatalf("marshal login: %v", err)
+	}
+
+	resp, err := doRequest(
+		tserver.t, http.MethodPost,
+		tserver.url("/api/v1/login"), bytes.NewReader(body),
+	)
+	if err != nil {
+		tserver.t.Fatalf("login: %v", err)
+	}
+
+	defer func() { _ = resp.Body.Close() }()
+
+	if resp.StatusCode != http.StatusOK {
+		tserver.t.Fatalf("login: status %d", resp.StatusCode)
+	}
+
+	for _, cookie := range resp.Cookies() {
+		if cookie.Name == authCookieName {
+			return cookie.Value
+		}
+	}
+
+	tserver.t.Fatal("no auth cookie in login response")
+
+	return ""
+}
+
+// postConcurrently POSTs body to path once with each of
+// tokens, all at once.
+func (tserver *testServer) postConcurrently(
+	path string, body []byte, tokens ...string,
+) {
+	tserver.t.Helper()
+
+	start := make(chan struct{})
+
+	var waitGroup sync.WaitGroup
+
+	for _, token := range tokens {
+		waitGroup.Add(1)
+
+		go func() {
+			defer waitGroup.Done()
+
+			<-start
+
+			resp, err := doRequestAuth(
+				tserver.t, http.MethodPost, tserver.url(path),
+				token, bytes.NewReader(body),
+			)
+			if err != nil {
+				tserver.t.Error(err)
+
+				return
+			}
+
+			_ = resp.Body.Close()
+		}()
+	}
+
+	close(start)
+	waitGroup.Wait()
+}
+
 func TestUnknownCommand(t *testing.T) {
 	tserver := newTestServer(t)
 	token := tserver.createSession("cmdtest")