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

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
-			a.mu.Lock()
+		// connection.
-			a.lastQID = result.LastID
+		a.mu.Lock()
+
+		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()