Stop the earlier poll loop on a second /connect (closes #228)

A second /connect started a new poll loop without stopping the first.
The first loop then polled with the new client too, so each message
could be shown twice. It also read stopPoll without a.mu while
cmdConnect wrote it.

cmdConnect now closes the earlier loop's channel in the same locked
step that replaces the client and the cursor, and hands the new
channel to the new loop. The loop checks its channel under a.mu
before each poll and again before it moves the cursor, so a stopped
loop never polls with the new client or moves the new cursor.

Model: opus-5-5

README.md

@@ -1236,7 +1236,7 @@ IRC listener sends the usual 3-digit code. Over the HTTP API, 005 arrives named
 | `001` | RPL_WELCOME          | After session creation or login, on MOTD | `{"command":"RPL_WELCOME","code":1,"to":"alice","body":["Welcome to the network, alice"]}`                                                                                                                   |
 | `002` | RPL_YOURHOST         | After session creation or login, on MOTD | `{"command":"RPL_YOURHOST","code":2,"to":"alice","body":["Your host is neoirc, running version 0.1"]}`                                                                                                       |
 | `003` | RPL_CREATED          | After session creation or login, on MOTD | `{"command":"RPL_CREATED","code":3,"to":"alice","body":["This server was created 2026-02-10"]}`                                                                                                              |
-| `004` | RPL_MYINFO           | After session creation or login, on MOTD | `{"command":"RPL_MYINFO","code":4,"to":"alice","params":["neoirc","0.1","ow","ikmnostl"]}`                                                                                                                   |
+| `004` | RPL_MYINFO           | After session creation or login, on MOTD | `{"command":"RPL_MYINFO","code":4,"to":"alice","params":["neoirc","0.1","ow","Hbiklmnostv"]}`                                                                                                                |
 | `005` | RPL_ISUPPORT         | After session creation or login, on MOTD | `{"command":"RPL_BOUNCE","code":5,"to":"alice","params":["CHANTYPES=#","NICKLEN=32","PREFIX=(ov)@+","CHANMODES=b,k,Hl,imnst","NETWORK=neoirc","CASEMAPPING=ascii"],"body":["are supported by this server"]}` |
 | `221` | RPL_UMODEIS          | In response to user MODE                 | `{"command":"RPL_UMODEIS","code":221,"to":"alice","body":["+"]}`                                                                                                                                             |
 | `251` | RPL_LUSERCLIENT      | On connect, MOTD or LUSERS               | `{"command":"RPL_LUSERCLIENT","code":251,"to":"alice","body":["There are 5 users and 0 invisible on 1 servers"]}`                                                                                            |

internal/cli/app.go

@@ -199,11 +199,21 @@ func (a *App) cmdConnect(serverURL string) {
 		return
 	}
 
+	// The poll loop of an earlier connection is stopped in the same
+	// step that replaces the client, so it never polls with the new
+	// one.
 	a.mu.Lock()
 	a.client = client
 	a.nick = resp.Nick
 	a.connected = true
 	a.lastQID = 0
+
+	if a.stopPoll != nil {
+		close(a.stopPoll)
+	}
+
+	stop := make(chan struct{})
+	a.stopPoll = stop
 	a.mu.Unlock()
 
 	a.ui.AddStatus(fmt.Sprintf(
@@ -212,9 +222,7 @@ func (a *App) cmdConnect(serverURL string) {
 	))
 	a.ui.SetStatus(resp.Nick, "", "connected")
 
-	a.stopPoll = make(chan struct{})
+	go a.pollLoop(stop)
-
-	go a.pollLoop()
 }
 
 func (a *App) cmdNick(nick string) {
@@ -702,16 +710,23 @@ func (a *App) cmdHelp() {
 	}
 }
 
-// pollLoop long-polls for messages in the background.
+// pollLoop long-polls for messages in the background until stop is
-func (a *App) pollLoop() {
+// closed. A later /connect closes stop under a.mu as it replaces the
+// client and the cursor, so the loop checks stop under a.mu too. A
+// stopped loop then neither polls with the new client nor moves the
+// new cursor.
+func (a *App) pollLoop(stop <-chan struct{}) {
 	for {
+		a.mu.Lock()
+
 		select {
-		case <-a.stopPoll:
+		case <-stop:
+			a.mu.Unlock()
+
 			return
 		default:
 		}
 
-		a.mu.Lock()
 		client := a.client
 		lastQID := a.lastQID
 		a.mu.Unlock()
@@ -729,12 +744,24 @@ func (a *App) pollLoop() {
 			continue
 		}
 
-		if result.LastID > 0 {
+		// A poll sent before stop was closed answers for the earlier
+		// connection.
 		a.mu.Lock()
-			a.lastQID = result.LastID
+
+		select {
+		case <-stop:
 			a.mu.Unlock()
+
+			return
+		default:
 		}
 
+		if result.LastID > 0 {
+			a.lastQID = result.LastID
+		}
+
+		a.mu.Unlock()
+
 		for i := range result.Messages {
 			a.handleServerMessage(&result.Messages[i])
 		}

internal/cli/app_internal_test.go

@@ -24,12 +24,13 @@ const (
 )
 
 // eventLoopTimeout is how long a test waits for a typed line to
-// be handled, for the welcome lines, or for the UI to stop, before
+// be handled, for the welcome lines, for the UI to stop, for a poll
-// it fails.
+// or for a poll loop to end, before it fails.
 const eventLoopTimeout = 5 * time.Second
 
-// holdTime is how long TestLinesKeepOrder holds the first line,
+// holdTime is how long a test waits to see that something does not
-// which gives the second line time to start too early.
+// happen: the second line handled while the first is held, or a
+// poll from a poll loop that was stopped.
 const holdTime = 200 * time.Millisecond
 
 // welcomeLines is the number of lines run adds to the status
@@ -422,6 +423,191 @@ func TestLinesKeepOrder(t *testing.T) {
 	waitHandled(t, handled, secondLine)
 }
 
+// TestSecondConnectStopsFirstPollLoop connects to one server, then
+// to a second that holds every poll. Only the second connection's
+// poll loop may poll the second server.
+func TestSecondConnectStopsFirstPollLoop(t *testing.T) {
+	t.Parallel()
+
+	const session = `{"id": 1, "nick": "erin"}`
+
+	// The first server answers every request at once, so the first
+	// poll loop polls it until the second /connect.
+	first := newServer(t, session)
+	second, polled := newHoldingServer(t, session)
+
+	app := &App{ui: NewUI(), nick: testNick}
+	runUI(t, app.ui)
+
+	app.cmdConnect(first.URL)
+	app.cmdConnect(second.URL)
+	waitPoll(t, polled, "the second server")
+
+	// The second connection's poll loop waits on its held poll, so
+	// another poll comes from the first loop.
+	select {
+	case <-polled:
+		t.Fatal("the first poll loop polled again after the second /connect")
+	case <-time.After(holdTime):
+	}
+
+	// Stop the second poll loop as /quit does. /quit itself also
+	// stops the UI, and runUI's cleanup stopping it again is a data
+	// race inside tview.
+	app.mu.Lock()
+	close(app.stopPoll)
+	app.mu.Unlock()
+}
+
+// TestSecondConnectIgnoresHeldPoll holds a poll of the earlier
+// connection at its server while a second /connect runs, then
+// answers it with a cursor and a message. It fails if that answer
+// moves the cursor or shows the message.
+func TestSecondConnectIgnoresHeldPoll(t *testing.T) {
+	t.Parallel()
+
+	const session = `{"id": 1, "nick": "erin"}`
+
+	const answer = `{"messages": [{"command": "PRIVMSG", "from": "bob",
+		"to": "#chan", "body": ["hello"]}], "last_id": 42}`
+
+	// The first server sends on polled when the poll arrives, and
+	// answers it once release is closed.
+	polled := make(chan struct{})
+	release := make(chan struct{})
+	first := httptest.NewServer(http.HandlerFunc(
+		func(writer http.ResponseWriter, request *http.Request) {
+			select {
+			case polled <- struct{}{}:
+			case <-request.Context().Done():
+				return
+			}
+
+			select {
+			case <-release:
+				_, _ = io.WriteString(writer, answer)
+			case <-request.Context().Done():
+			}
+		},
+	))
+	t.Cleanup(first.Close)
+	t.Cleanup(first.CloseClientConnections)
+
+	// The second server answers every request at once with the
+	// session. A poll answered with it has no cursor and no messages,
+	// so the second poll loop never moves the cursor.
+	second := newServer(t, session)
+
+	// The test runs the earlier connection's poll loop itself, so
+	// that it can wait for the loop to end.
+	stop := make(chan struct{})
+	app := &App{
+		ui:        NewUI(),
+		client:    api.NewClient(first.URL),
+		nick:      testNick,
+		connected: true,
+		stopPoll:  stop,
+	}
+	runUI(t, app.ui)
+
+	done := make(chan struct{})
+
+	go func() {
+		app.pollLoop(stop)
+		close(done)
+	}()
+
+	select {
+	case <-polled:
+	case <-time.After(eventLoopTimeout):
+		t.Fatalf("the first server got no poll within %v", eventLoopTimeout)
+	}
+
+	app.cmdConnect(second.URL)
+	close(release)
+
+	select {
+	case <-done:
+	case <-time.After(eventLoopTimeout):
+		t.Fatalf("the earlier poll loop did not end within %v", eventLoopTimeout)
+	}
+
+	app.mu.Lock()
+	cursor := app.lastQID
+	app.mu.Unlock()
+
+	if cursor != 0 {
+		t.Errorf("the earlier connection's answer moved the cursor to %d", cursor)
+	}
+
+	lines, _ := bufferState(app.ui, testChannel)
+	if lines != 0 {
+		t.Errorf("the earlier connection's message was shown in %s", testChannel)
+	}
+
+	// Stop the second poll loop as /quit does, without stopping the UI.
+	app.mu.Lock()
+	close(app.stopPoll)
+	app.mu.Unlock()
+}
+
+// TestSecondConnectAfterFailedPoll holds a poll of the earlier
+// connection at its server while a second /connect runs, then
+// fails it. A poll loop waits pollRetry after a failed poll, then
+// polls again. The test fails if the earlier poll loop polls the
+// second server.
+func TestSecondConnectAfterFailedPoll(t *testing.T) {
+	t.Parallel()
+
+	const session = `{"id": 1, "nick": "erin"}`
+
+	first, firstPolled := newHoldingServer(t, session)
+
+	// The second server holds the one poll of the second
+	// connection's poll loop, so any other poll comes from the
+	// earlier loop.
+	second, secondPolled := newHoldingServer(t, session)
+
+	// The test runs the earlier connection's poll loop itself, so
+	// that it can wait for the loop to end.
+	stop := make(chan struct{})
+	app := &App{
+		ui:        NewUI(),
+		client:    api.NewClient(first.URL),
+		nick:      testNick,
+		connected: true,
+		stopPoll:  stop,
+	}
+	runUI(t, app.ui)
+
+	done := make(chan struct{})
+
+	go func() {
+		app.pollLoop(stop)
+		close(done)
+	}()
+
+	waitPoll(t, firstPolled, "the first server")
+	app.cmdConnect(second.URL)
+	waitPoll(t, secondPolled, "the second server")
+
+	// Closing the held poll's connection makes that poll fail.
+	first.CloseClientConnections()
+
+	select {
+	case <-done:
+	case <-secondPolled:
+		t.Fatal("the earlier poll loop polled the second server")
+	case <-time.After(eventLoopTimeout):
+		t.Fatalf("the earlier poll loop did not end within %v", eventLoopTimeout)
+	}
+
+	// Stop the second poll loop as /quit does, without stopping the UI.
+	app.mu.Lock()
+	close(app.stopPoll)
+	app.mu.Unlock()
+}
+
 // TestStartUp runs the client's start-up path on a simulation
 // screen. It fails unless the event loop starts and the status
 // window gets two lines: first the one that holds "Welcome to
@@ -706,6 +892,53 @@ func newServer(t *testing.T, body string) *httptest.Server {
 	return server
 }
 
+// newHoldingServer starts a server that sends on the returned
+// channel for each poll and holds the poll until its connection
+// closes. It answers any other request with body.
+func newHoldingServer(
+	t *testing.T, body string,
+) (*httptest.Server, <-chan struct{}) {
+	t.Helper()
+
+	polled := make(chan struct{})
+	server := httptest.NewServer(http.HandlerFunc(
+		func(writer http.ResponseWriter, request *http.Request) {
+			if request.Method != http.MethodGet ||
+				request.URL.Path != "/api/v1/messages" {
+				_, _ = io.WriteString(writer, body)
+
+				return
+			}
+
+			select {
+			case polled <- struct{}{}:
+			case <-request.Context().Done():
+			}
+
+			<-request.Context().Done()
+		},
+	))
+	// Close waits for the held polls, which end only once their
+	// connections close. Cleanups run last first, so the connections
+	// close before Close runs.
+	t.Cleanup(server.Close)
+	t.Cleanup(server.CloseClientConnections)
+
+	return server, polled
+}
+
+// waitPoll fails the test unless server gets a poll, sent on
+// polled, within eventLoopTimeout.
+func waitPoll(t *testing.T, polled <-chan struct{}, server string) {
+	t.Helper()
+
+	select {
+	case <-polled:
+	case <-time.After(eventLoopTimeout):
+		t.Fatalf("%s got no poll within %v", server, eventLoopTimeout)
+	}
+}
+
 // runUI runs ui on a simulated screen until the test ends.
 func runUI(t *testing.T, ui *UI) {
 	t.Helper()

internal/handlers/api.go

@@ -412,7 +412,7 @@ func (hdlr *Handlers) deliverWelcome(
 	// 004 RPL_MYINFO
 	hdlr.enqueueNumeric(
 		ctx, clientID, irc.RplMyInfo, nick,
-		[]string{srvName, version, service.UserModes, "ikmnostl"},
+		[]string{srvName, version, service.UserModes, "Hbiklmnostv"},
 		"",
 	)
 

internal/handlers/api_test.go

@@ -6797,9 +6797,10 @@ func TestISUPPORT(t *testing.T) {
 	}
 }
 
-// TestMyInfoListsUserModes verifies the 004 numeric lists
+// TestMyInfoListsModes verifies the 004 numeric lists the
-// the user modes o and w as its third parameter.
+// user modes o and w as its third parameter, and the HTTP
-func TestMyInfoListsUserModes(t *testing.T) {
+// API's channel modes Hbiklmnostv as its fourth.
+func TestMyInfoListsModes(t *testing.T) {
 	t.Parallel()
 
 	tserver := newTestServer(t)
@@ -6813,9 +6814,10 @@ func TestMyInfoListsUserModes(t *testing.T) {
 	}
 
 	params := getNumericParams(myInfo)
-	if len(params) != 4 || params[2] != "ow" {
+	if len(params) != 4 || params[2] != "ow" ||
+		params[3] != "Hbiklmnostv" {
 		t.Fatalf(
-			"004 params %v, want four with user modes ow third",
+			"004 params %v, want four ending ow Hbiklmnostv",
 			params,
 		)
 	}

internal/ircserver/commands.go

@@ -371,6 +371,7 @@ func (c *Conn) handleQuit(msg *Message) {
 
 	c.mu.Lock()
 	c.closed = true
+	c.quitReason = reason
 	c.mu.Unlock()
 }
 

internal/ircserver/conn.go

@@ -69,6 +69,11 @@ type Conn struct {
 	lastQueueID int64
 	closed      bool
 	killed      bool
+
+	// quitReason is the reason in the QUIT that peers are
+	// sent when the connection ends. The client's QUIT
+	// replaces it.
+	quitReason string
 }
 
 func newConn(
@@ -95,6 +100,7 @@ func newConn(
 		svc:        svc,
 		serverSfx:  srvName,
 		remoteIP:   host,
+		quitReason: "Connection closed",
 	}
 
 	conn.commands = conn.buildCommandMap()
@@ -278,6 +284,7 @@ func (c *Conn) cleanup(ctx context.Context) {
 	wasKilled := c.killed
 	sessID := c.sessionID
 	nick := c.nick
+	quitReason := c.quitReason
 	c.closed = true
 	c.mu.Unlock()
 
@@ -287,9 +294,7 @@ func (c *Conn) cleanup(ctx context.Context) {
 		// KILL has already sent the QUIT and deleted the
 		// session.
 		if !wasKilled {
-			c.svc.BroadcastQuit(
+			c.svc.BroadcastQuit(ctx, sessID, nick, quitReason)
-				ctx, sessID, nick, "Connection closed",
-			)
 		}
 	}
 
@@ -605,7 +610,7 @@ func (c *Conn) deliverWelcome() {
 	)
 	c.sendNumeric(
 		irc.RplMyInfo,
-		c.serverSfx, "neoirc", service.UserModes, "mnst",
+		c.serverSfx, "neoirc", service.UserModes, "Himnostv",
 	)
 	c.sendNumeric(
 		irc.RplIsupport,

internal/ircserver/relay_test.go

@@ -316,7 +316,7 @@ func TestRelayNickDiffersOnlyInCase(t *testing.T) {
 		{"JOIN #case", ":Alice!Alice@* JOIN #case"},
 		{"NICK Alicia", ":Alice!Alice@* NICK Alicia"},
 		{"NICK Alice", ":Alicia!Alicia@* NICK Alice"}, // back for QUIT
-		{"QUIT", ":Alice!Alice@* QUIT :Quit: Connection closed"},
+		{"QUIT", ":Alice!Alice@* QUIT :Quit: Client quit"},
 	} {
 		capitalAlice.send(step.line)
 

internal/ircserver/server_test.go

@@ -4,12 +4,14 @@ import (
 	"bufio"
 	"context"
 	"database/sql"
+	"encoding/json"
 	"errors"
 	"fmt"
 	"log/slog"
 	"net"
 	"os"
 	"runtime"
+	"slices"
 	"strings"
 	"sync/atomic"
 	"testing"
@@ -23,6 +25,7 @@ import (
 	"sneak.berlin/go/neoirc/internal/globals"
 	"sneak.berlin/go/neoirc/internal/ircserver"
 	"sneak.berlin/go/neoirc/internal/logger"
+	"sneak.berlin/go/neoirc/pkg/irc"
 
 	_ "modernc.org/sqlite"
 )
@@ -486,20 +489,20 @@ func TestWelcomeContainsNick(t *testing.T) {
 	}
 }
 
-// TestMyInfoListsUserModes checks that RPL_MYINFO lists the
+// TestMyInfoListsModes checks that RPL_MYINFO lists the user
-// user modes o and w, with no empty parameter before the
+// modes o and w, then the channel modes MODE accepts on the
-// channel modes.
+// IRC listener. It compares the whole line, so a missing or
-func TestMyInfoListsUserModes(t *testing.T) {
+// extra mode fails it.
+func TestMyInfoListsModes(t *testing.T) {
 	t.Parallel()
 
 	env := newTestEnv(t)
 	lines := env.dial(t).register("carol")
 
-	assertContains(
+	want := ":test.irc 004 carol test.irc neoirc ow Himnostv"
-		t, lines,
+	if !slices.Contains(lines, want) {
-		":test.irc 004 carol test.irc neoirc ow mnst",
+		t.Errorf("no line %q in %v", want, lines)
-		"RPL_MYINFO with user modes",
+	}
-	)
 }
 
 func TestPingPong(t *testing.T) {
@@ -872,6 +875,112 @@ func TestQuit(t *testing.T) {
 	assertContains(t, lines, "goodbye", "QUIT reason")
 }
 
+// TestQuitReasonReachesPeers checks the reason that an IRC
+// listener user's channel peers see in its QUIT, on both the
+// IRC listener and the HTTP API. It is the reason the user
+// gave, Client quit for a QUIT without one, or Connection
+// closed when the connection ends without a QUIT.
+func TestQuitReasonReachesPeers(t *testing.T) {
+	t.Parallel()
+
+	for _, step := range []struct {
+		name, quit, want string
+	}{
+		{"QUIT with a reason", "QUIT :gone home", "gone home"},
+		{"QUIT without a reason", irc.CmdQuit, "Client quit"},
+		{"connection closed without QUIT", "", "Connection closed"},
+	} {
+		t.Run(step.name, func(t *testing.T) {
+			t.Parallel()
+
+			ircLine, apiBody := quitSeenByPeers(t, step.quit)
+
+			want := ":quitter!quitter@* QUIT :Quit: " + step.want
+			if ircLine != want {
+				t.Errorf("IRC peer got %q, want %q", ircLine, want)
+			}
+
+			if len(apiBody) != 1 || apiBody[0] != step.want {
+				t.Errorf("HTTP API peer got %q, want [%q]", apiBody, step.want)
+			}
+		})
+	}
+}
+
+// quitSeenByPeers starts a server on which quitter, ircpeer on
+// the IRC listener and apipeer on the HTTP API share a channel.
+// quitter sends the line quit, or closes its connection when
+// quit is empty. It returns the QUIT line ircpeer is sent and
+// the body of the QUIT apipeer is sent.
+func quitSeenByPeers(t *testing.T, quit string) (string, []string) {
+	t.Helper()
+
+	env := newTestEnv(t)
+	ctx := t.Context()
+
+	ircPeer := env.dial(t)
+	ircPeer.register("ircpeer")
+	ircPeer.joinAndDrain("#bye")
+
+	apiPeerID, apiPeerClientID, _, err := env.database.CreateSession(
+		ctx, "apipeer", "apipeer", "", "",
+	)
+	if err != nil {
+		t.Fatalf("create peer session: %v", err)
+	}
+
+	channelID, err := env.database.GetChannelByName(ctx, "#bye")
+	if err != nil {
+		t.Fatalf("get channel: %v", err)
+	}
+
+	err = env.database.JoinChannel(ctx, channelID, apiPeerID)
+	if err != nil {
+		t.Fatalf("join peer: %v", err)
+	}
+
+	quitter := env.dial(t)
+	quitter.register("quitter")
+	quitter.joinAndDrain("#bye")
+
+	if quit == "" {
+		err = quitter.conn.Close()
+		if err != nil {
+			t.Fatalf("close: %v", err)
+		}
+	} else {
+		quitter.send(quit)
+	}
+
+	lines := ircPeer.readUntil(func(l string) bool {
+		return strings.Contains(l, " QUIT ")
+	})
+
+	// The QUIT is queued to both peers at once, so apipeer has
+	// it by now.
+	msgs, _, err := env.database.PollMessages(ctx, apiPeerClientID, 0, 0)
+	if err != nil {
+		t.Fatalf("poll peer: %v", err)
+	}
+
+	for _, msg := range msgs {
+		if msg.Command == irc.CmdQuit && msg.From == "quitter" {
+			var body []string
+
+			err = json.Unmarshal(msg.Body, &body)
+			if err != nil {
+				t.Fatalf("decode QUIT body %s: %v", msg.Body, err)
+			}
+
+			return lines[len(lines)-1], body
+		}
+	}
+
+	t.Fatalf("apipeer was not sent the QUIT: %v", msgs)
+
+	return "", nil
+}
+
 // TestStopEndsSessions checks that stopping the server ends
 // a registered connection's session before Stop returns, as
 // closing the connection does: the session, its client and

schema/README.md

@@ -145,7 +145,7 @@ Where a schema file and the HTTP API disagree, the HTTP API is right:
 - `003.json` describes the server's creation date. The HTTP API sends the date
   the server last started.
 - `004.json` has example mode lists. The HTTP API sends `ow` as the user modes
-  and `ikmnostl` as the channel modes.
+  and `Hbiklmnostv` as the channel modes.
 - `322.json` says `LIST` gets one reply per channel. The server leaves out a
   secret channel, one with mode `+s`, unless the user is on it.
 - `332.json` says `RPL_TOPIC` answers a `TOPIC` query. The HTTP API has no such