Bind history to the recipient session and channel, not the name (closes #148)

History was selected by msg_to, so whoever next held a released nick, or
created a deleted channel again under its old name, could read the
earlier holder's messages. Each message row now records the session a
DM or INVITE was sent to (to_session_id) and the channel a channel
message was sent to (to_channel_id), and GET /api/v1/history selects by
those ids. A session's DM history now also follows it across nick
changes.

Model: opus-5-5

README.md

@@ -2528,9 +2528,9 @@ Unique constraint on `(channel_id, session_id)`.
 
 Indexes on `(to_session_id, id)`, `(to_channel_id, id)` and `(created_at)`.
 `to_session_id` and `to_channel_id` are null on a message sent to neither, such
-as `QUIT`. `GET /api/v1/history` selects by one of these two columns rather than
+as `QUIT`. `GET /api/v1/history` selects by these two columns rather than by
-by `msg_to`, because nicks and channel names are reused once released, and
+`msg_to`, because nicks and channel names are reused once released, and session
-session and channel ids are not.
+and channel ids are not.
 
 #### `spent_hashcash`
 

internal/broker/broker.go

@@ -47,16 +47,6 @@ func (b *Broker) Notify(userID int64) {
 	}
 }
 
-// ListenerCount returns how many channels from Wait are still
-// registered for the user: those that neither Notify nor Remove
-// has taken out.
-func (b *Broker) ListenerCount(userID int64) int {
-	b.mu.Lock()
-	defer b.mu.Unlock()
-
-	return len(b.listeners[userID])
-}
-
 // Remove removes a specific wait channel (for cleanup on timeout).
 func (b *Broker) Remove(
 	userID int64,

internal/broker/broker_test.go

@@ -59,39 +59,6 @@ func TestRemove(t *testing.T) {
 	}
 }
 
-func TestListenerCount(t *testing.T) {
-	t.Parallel()
-
-	brk := broker.New()
-
-	wantCount := func(step string, userID int64, want int) {
-		t.Helper()
-
-		got := brk.ListenerCount(userID)
-		if got != want {
-			t.Errorf("%s: user %d: got %d, want %d",
-				step, userID, got, want)
-		}
-	}
-
-	wantCount("before Wait", 1, 0)
-
-	waitCh := brk.Wait(1)
-	brk.Wait(1)
-	brk.Wait(2)
-
-	wantCount("after two Waits", 1, 2)
-
-	brk.Remove(1, waitCh)
-
-	wantCount("after Remove", 1, 1)
-
-	brk.Notify(1)
-
-	wantCount("after Notify", 1, 0)
-	wantCount("after Notify", 2, 1)
-}
-
 func TestMultipleWaiters(t *testing.T) {
 	t.Parallel()
 

internal/db/errors.go

@@ -17,14 +17,3 @@ func IsUniqueConstraintError(err error) bool {
 
 	return sqliteErr.Code() == sqlite3.SQLITE_CONSTRAINT_UNIQUE
 }
-
-// IsForeignKeyConstraintError reports whether err is a
-// SQLite foreign-key constraint violation.
-func IsForeignKeyConstraintError(err error) bool {
-	var sqliteErr *sqlite.Error
-	if !errors.As(err, &sqliteErr) {
-		return false
-	}
-
-	return sqliteErr.Code() == sqlite3.SQLITE_CONSTRAINT_FOREIGNKEY
-}

internal/db/queries.go

@@ -9,7 +9,6 @@ import (
 	"encoding/json"
 	"errors"
 	"fmt"
-	"math"
 	"strconv"
 	"strings"
 	"time"
@@ -1069,10 +1068,11 @@ func (database *Database) PollMessages(
 
 // GetHistory returns the PRIVMSG history of one recipient:
 // the DMs sent to session toSessionID, or the messages sent
-// to channel toChannelID. Pass 0 for the other. A non-empty
+// to channel toChannelID. Pass 0 for the other, which
-// before is the id of a PRIVMSG in that history, and only
+// matches no message. A non-empty before is the id of a
-// messages older than it are returned; an id with no such
+// message in that history, and only messages older than it
-// message gives an error wrapping sql.ErrNoRows.
+// are returned; an id with no such message gives an error
+// wrapping sql.ErrNoRows.
 func (database *Database) GetHistory(
 	ctx context.Context,
 	toSessionID, toChannelID int64,
@@ -1083,13 +1083,12 @@ func (database *Database) GetHistory(
 		limit = defaultHistLimit
 	}
 
-	// With no before, every message is older than this.
+	var beforeID int64
-	beforeID := int64(math.MaxInt64)
 
 	if before != "" {
 		err := database.conn.QueryRowContext(ctx,
 			`SELECT id FROM messages
-			 WHERE uuid = ? AND command = 'PRIVMSG'
+			 WHERE uuid = ?
 			   AND (to_session_id = ? OR to_channel_id = ?)`,
 			before, toSessionID, toChannelID,
 		).Scan(&beforeID)
@@ -1121,23 +1120,21 @@ func (database *Database) GetHistory(
 	return msgs, nil
 }
 
-// queryHistory selects by to_session_id when toSessionID is
-// set and by to_channel_id otherwise, so that the column's
-// (column, id) index serves the order and the limit.
 func (database *Database) queryHistory(
 	ctx context.Context,
 	toSessionID, toChannelID, beforeID int64,
 	limit int,
 ) (*sql.Rows, error) {
-	if toSessionID != 0 {
+	if beforeID > 0 {
 		rows, err := database.conn.QueryContext(ctx,
 			`SELECT id, uuid, command, msg_from,
 			   msg_to, params, body, meta, created_at
 			 FROM messages
-			 WHERE to_session_id = ? AND id < ?
+			 WHERE (to_session_id = ? OR to_channel_id = ?)
+			   AND id < ?
 			   AND command = 'PRIVMSG'
 			 ORDER BY id DESC LIMIT ?`,
-			toSessionID, beforeID, limit)
+			toSessionID, toChannelID, beforeID, limit)
 		if err != nil {
 			return nil, fmt.Errorf(
 				"query history: %w", err,
@@ -1151,10 +1148,10 @@ func (database *Database) queryHistory(
 		`SELECT id, uuid, command, msg_from,
 		   msg_to, params, body, meta, created_at
 		 FROM messages
-		 WHERE to_channel_id = ? AND id < ?
+		 WHERE (to_session_id = ? OR to_channel_id = ?)
 		   AND command = 'PRIVMSG'
 		 ORDER BY id DESC LIMIT ?`,
-		toChannelID, beforeID, limit)
+		toSessionID, toChannelID, limit)
 	if err != nil {
 		return nil, fmt.Errorf("query history: %w", err)
 	}

internal/handlers/api_test.go

@@ -1654,25 +1654,9 @@ func TestHistoryUnknownBefore(t *testing.T) {
 		)
 	}
 
-	// History holds only PRIVMSG, so the id of the JOIN to
-	// #here is not in it.
-	var joinID string
-
-	queued, _ := tserver.pollMessages(token, 0)
-	for _, msg := range queued {
-		if msg[commandKey] == joinCmd && msg[toKey] == "#here" {
-			joinID, _ = msg["id"].(string)
-		}
-	}
-
-	if joinID == "" {
-		t.Fatal("expected a JOIN to #here in the queue")
-	}
-
 	cases := []struct{ name, before string }{
 		{"made-up id", "no-such-message"},
 		{"id from another channel", elsewhere[0].ID},
-		{"id of a JOIN", joinID},
 	}
 
 	for _, tc := range cases {

internal/ircserver/export_test.go

@@ -60,10 +60,6 @@ func (s *Server) Listener() net.Listener {
 // to a killed victim.
 const KillWriteWindow = killWriteWindow
 
-// PollInterval is how often the relay checks the client
-// output queue when no broker notification arrives.
-const PollInterval = pollInterval
-
 // NewTestConn returns a Conn for nick on tcpConn, without a
 // server, database or service behind it.
 func NewTestConn(

internal/ircserver/relay.go

@@ -42,10 +42,7 @@ func (c *Conn) relayMessages(ctx context.Context) {
 		case <-waitCh:
 			// New message notification — loop back.
 		case <-ticker.C:
-			// Periodic check. Nothing used waitCh, so take it
+			// Periodic check.
-			// out of the broker before the next pass adds
-			// another.
-			c.brk.Remove(c.sessionID, waitCh)
 		case <-ctx.Done():
 			c.brk.Remove(c.sessionID, waitCh)
 

internal/ircserver/relay_test.go

@@ -5,9 +5,6 @@ import (
 	"slices"
 	"strings"
 	"testing"
-	"time"
-
-	"sneak.berlin/go/neoirc/internal/ircserver"
 )
 
 // queueMessage queues a message for the user with nick
@@ -47,51 +44,6 @@ func (env *testEnv) queueMessage(
 	env.brk.Notify(sessionID)
 }
 
-// TestRelayKeepsOneWaitChannel checks that an idle IRC
-// listener connection holds at most one broker wait channel
-// across several relay ticks, and none once it closes.
-func TestRelayKeepsOneWaitChannel(t *testing.T) {
-	t.Parallel()
-
-	const idleTicks = 5
-
-	env := newTestEnv(t)
-
-	client := env.dial(t)
-	client.register("idle")
-
-	sessionID, err := env.database.GetSessionByNick(t.Context(), "idle")
-	if err != nil {
-		t.Fatalf("get session of idle: %v", err)
-	}
-
-	for tick := range idleTicks {
-		time.Sleep(ircserver.PollInterval)
-
-		got := env.brk.ListenerCount(sessionID)
-		if got > 1 {
-			t.Fatalf("tick %d: got %d wait channels, want at most 1",
-				tick+1, got)
-		}
-	}
-
-	err = client.conn.Close()
-	if err != nil {
-		t.Fatalf("close: %v", err)
-	}
-
-	deadline := time.Now().Add(testTimeout)
-
-	for env.brk.ListenerCount(sessionID) != 0 {
-		if time.Now().After(deadline) {
-			t.Fatalf("after close: got %d wait channels, want 0",
-				env.brk.ListenerCount(sessionID))
-		}
-
-		time.Sleep(relayCheckStep)
-	}
-}
-
 // TestRelayMultiLineBody checks that each line of a PRIVMSG
 // or NOTICE body sent over the HTTP API reaches an IRC
 // listener user as its own line.

internal/service/service.go

@@ -400,13 +400,13 @@ func (s *Service) SendChannelMessage(
 	nick, command, channel string,
 	body, meta json.RawMessage,
 ) (int64, string, error) {
-	noSuchChannel := &IRCError{
-		irc.ErrNoSuchChannel, []string{channel}, msgNoSuchChannel,
-	}
-
 	chID, err := s.db.GetChannelByName(ctx, channel)
 	if err != nil {
-		return 0, "", noSuchChannel
+		return 0, "", &IRCError{
+			irc.ErrNoSuchChannel,
+			[]string{channel},
+			msgNoSuchChannel,
+		}
 	}
 
 	if command == irc.CmdPrivmsg {
@@ -466,11 +466,6 @@ func (s *Service) SendChannelMessage(
 		ctx, command, nick, channel, 0, chID,
 		nil, body, meta, recipients,
 	)
-	// The channel was deleted after the lookup above.
-	if db.IsForeignKeyConstraintError(fanErr) {
-		return 0, "", noSuchChannel
-	}
-
 	if fanErr != nil {
 		return 0, "", fanErr
 	}
@@ -487,13 +482,13 @@ func (s *Service) SendDirectMessage(
 	nick, command, target string,
 	body, meta json.RawMessage,
 ) (*DirectMsgResult, error) {
-	noSuchNick := &IRCError{
-		irc.ErrNoSuchNick, []string{target}, "No such nick",
-	}
-
 	targetSID, err := s.db.GetSessionByNick(ctx, target)
 	if err != nil {
-		return nil, noSuchNick
+		return nil, &IRCError{
+			irc.ErrNoSuchNick,
+			[]string{target},
+			"No such nick",
+		}
 	}
 
 	away, _ := s.db.GetAway(ctx, targetSID)
@@ -507,11 +502,6 @@ func (s *Service) SendDirectMessage(
 		ctx, command, nick, target, targetSID, 0,
 		nil, body, meta, recipients,
 	)
-	// The target's session was deleted after the lookup above.
-	if db.IsForeignKeyConstraintError(fanErr) {
-		return nil, noSuchNick
-	}
-
 	if fanErr != nil {
 		return nil, fanErr
 	}

internal/service/service_test.go

@@ -339,78 +339,6 @@ func TestBroadcastQuit(t *testing.T) {
 	}
 }
 
-// TestSendDirectMessageTargetQuits checks that a DM whose
-// target quits after its nick is looked up gets the same
-// error as a DM to an unknown nick. The trigger deletes the
-// target's session just before the DM is stored.
-func TestSendDirectMessageTargetQuits(t *testing.T) {
-	env := newTestEnv(t)
-	ctx := t.Context()
-
-	sid := createSession(ctx, t, env.db, "alice")
-	createSession(ctx, t, env.db, "bob")
-
-	_, err := env.db.GetDB().ExecContext(ctx,
-		`CREATE TRIGGER delete_recipient
-		 BEFORE INSERT ON messages
-		 BEGIN
-		   DELETE FROM sessions WHERE id = NEW.to_session_id;
-		 END`)
-	if err != nil {
-		t.Fatalf("create trigger: %v", err)
-	}
-
-	body, _ := json.Marshal([]string{"hi"}) //nolint:errchkjson
-
-	_, err = env.svc.SendDirectMessage(
-		ctx, sid, "alice", irc.CmdPrivmsg, "bob", body, nil,
-	)
-
-	var ircErr *service.IRCError
-	if !errors.As(err, &ircErr) || ircErr.Code != irc.ErrNoSuchNick {
-		t.Fatalf("want ERR_NOSUCHNICK, got %v", err)
-	}
-}
-
-// TestSendChannelMessageChannelDeleted checks that a channel
-// message whose channel is deleted after it is looked up gets
-// the same error as a message to an unknown channel. The
-// trigger deletes the channel just before the message is
-// stored.
-func TestSendChannelMessageChannelDeleted(t *testing.T) {
-	env := newTestEnv(t)
-	ctx := t.Context()
-
-	sid := createSession(ctx, t, env.db, "alice")
-
-	_, err := env.svc.JoinChannel(ctx, sid, "alice", "#gone", "")
-	if err != nil {
-		t.Fatalf("join alice: %v", err)
-	}
-
-	_, err = env.db.GetDB().ExecContext(ctx,
-		`CREATE TRIGGER delete_recipient
-		 BEFORE INSERT ON messages
-		 BEGIN
-		   DELETE FROM channels WHERE id = NEW.to_channel_id;
-		 END`)
-	if err != nil {
-		t.Fatalf("create trigger: %v", err)
-	}
-
-	body, _ := json.Marshal([]string{"hi"}) //nolint:errchkjson
-
-	_, _, err = env.svc.SendChannelMessage(
-		ctx, sid, "alice", irc.CmdPrivmsg, "#gone", body, nil,
-	)
-
-	var ircErr *service.IRCError
-	if !errors.As(err, &ircErr) ||
-		ircErr.Code != irc.ErrNoSuchChannel {
-		t.Fatalf("want ERR_NOSUCHCHANNEL, got %v", err)
-	}
-}
-
 func TestSendChannelMessage_Moderated(t *testing.T) {
 	env := newTestEnv(t)
 	ctx := t.Context()