Run JOIN, PART and KICK as one transaction each (closes #153)

JoinChannel made the channel, counted its members, ran the join
checks and inserted the member as separate statements, so two
requests could each decide on a count the other was about to change:
+l was exceeded, two first joiners both became operator, and a JOIN
racing the last PART failed its foreign key and answered 500.

db.CreateOrJoinChannel now does all of that, and uses up the invite,
in one transaction; the join checks moved into it, and the service
turns its refusals into the same IRC errors as before. PART and KICK
remove the member and delete an empty channel in one transaction
(db.PartChannelAndDeleteIfEmpty). The db functions this leaves with
no caller are removed.

Model: opus-5-5

internal/service/service.go

@@ -536,41 +536,13 @@ func (s *Service) JoinChannel(
 		}
 	}
 
-	chID, err := s.db.GetOrCreateChannel(ctx, channel)
-	if err != nil {
-		return nil, fmt.Errorf("get/create channel: %w", err)
-	}
-
-	memberCount, countErr := s.db.CountChannelMembers(
-		ctx, chID,
+	chID, isCreator, err := s.db.CreateOrJoinChannel(
+		ctx, channel, sessionID, suppliedKey,
 	)
-	isCreator := countErr == nil && memberCount == 0
-
-	if !isCreator {
-		joinErr := checkJoinRestrictions(
-			ctx, s.db, chID, sessionID,
-			channel, suppliedKey, memberCount,
-		)
-		if joinErr != nil {
-			return nil, joinErr
-		}
-	}
-
-	if isCreator {
-		err = s.db.JoinChannelAsOperator(
-			ctx, chID, sessionID,
-		)
-	} else {
-		err = s.db.JoinChannel(ctx, chID, sessionID)
-	}
-
 	if err != nil {
-		return nil, fmt.Errorf("join channel: %w", err)
+		return nil, joinError(channel, err)
 	}
 
-	// Clear invite after successful join.
-	_ = s.db.ClearChannelInvite(ctx, chID, sessionID)
-
 	memberIDs, _ := s.db.GetChannelMemberIDs(ctx, chID)
 	body, _ := json.Marshal([]string{channel}) //nolint:errchkjson
 
@@ -622,8 +594,7 @@ func (s *Service) PartChannel(
 		nil, body, nil, recipients,
 	)
 
-	s.db.PartChannel(ctx, chID, sessionID) //nolint:errcheck,gosec
-	s.db.DeleteChannelIfEmpty(ctx, chID)   //nolint:errcheck,gosec
+	s.db.PartChannelAndDeleteIfEmpty(ctx, chID, sessionID) //nolint:errcheck,gosec
 
 	return nil
 }
@@ -748,8 +719,7 @@ func (s *Service) KickUser(
 		params, body, nil, memberIDs,
 	)
 
-	s.db.PartChannel(ctx, chID, targetSID) //nolint:errcheck,gosec
-	s.db.DeleteChannelIfEmpty(ctx, chID)   //nolint:errcheck,gosec
+	s.db.PartChannelAndDeleteIfEmpty(ctx, chID, targetSID) //nolint:errcheck,gosec
 
 	return nil
 }
@@ -1392,54 +1362,30 @@ func (s *Service) broadcastNickChange(
 	}
 }
 
-// checkJoinRestrictions validates Tier 2 join conditions:
-// bans, invite-only, channel key, and user limit.
-func checkJoinRestrictions(
-	ctx context.Context,
-	database *db.Database,
-	chID, sessionID int64,
-	channel, suppliedKey string,
-	memberCount int64,
-) error {
-	isBanned, banErr := database.IsSessionBanned(
-		ctx, chID, sessionID,
-	)
-	if banErr == nil && isBanned {
+// joinError returns the IRC error for a JOIN of channel
+// that db.CreateOrJoinChannel refused with err, or err
+// wrapped if it is not a refusal.
+func joinError(channel string, err error) error {
+	switch {
+	case errors.Is(err, db.ErrBannedFromChannel):
 		return &IRCError{
 			Code:    irc.ErrBannedFromChan,
 			Params:  []string{channel},
 			Message: "Cannot join channel (+b)",
 		}
-	}
-
-	isInviteOnly, ioErr := database.IsChannelInviteOnly(
-		ctx, chID,
-	)
-	if ioErr == nil && isInviteOnly {
-		hasInvite, invErr := database.HasChannelInvite(
-			ctx, chID, sessionID,
-		)
-		if invErr != nil || !hasInvite {
-			return &IRCError{
-				Code:    irc.ErrInviteOnlyChan,
-				Params:  []string{channel},
-				Message: "Cannot join channel (+i)",
-			}
+	case errors.Is(err, db.ErrInviteOnlyChannel):
+		return &IRCError{
+			Code:    irc.ErrInviteOnlyChan,
+			Params:  []string{channel},
+			Message: "Cannot join channel (+i)",
 		}
-	}
-
-	key, keyErr := database.GetChannelKey(ctx, chID)
-	if keyErr == nil && key != "" && suppliedKey != key {
+	case errors.Is(err, db.ErrBadChannelKey):
 		return &IRCError{
 			Code:    irc.ErrBadChannelKey,
 			Params:  []string{channel},
 			Message: "Cannot join channel (+k)",
 		}
-	}
-
-	limit, limErr := database.GetChannelUserLimit(ctx, chID)
-	if limErr == nil && limit > 0 &&
-		memberCount >= int64(limit) {
+	case errors.Is(err, db.ErrChannelIsFull):
 		return &IRCError{
 			Code:    irc.ErrChannelIsFull,
 			Params:  []string{channel},
@@ -1447,7 +1393,7 @@ func checkJoinRestrictions(
 		}
 	}
 
-	return nil
+	return fmt.Errorf("join channel: %w", err)
 }
 
 // extractHashcashFromMeta parses the meta JSON and

internal/service/service_test.go

@@ -13,6 +13,7 @@ import (
 	"os"
 	"slices"
 	"strings"
+	"sync"
 	"testing"
 	"time"
 
@@ -255,6 +256,402 @@ func TestPartChannel(t *testing.T) {
 	}
 }
 
+// concurrentJoinTrials is how many times each test of
+// concurrent JOINs repeats its race. Before a JOIN ran as
+// one transaction, ten trials lost each race every time.
+const concurrentJoinTrials = 10
+
+// runConcurrently runs each of funcs in its own goroutine,
+// releasing them all at once, and returns their errors in
+// the order of funcs.
+func runConcurrently(funcs ...func() error) []error {
+	start := make(chan struct{})
+	errs := make([]error, len(funcs))
+
+	var wg sync.WaitGroup
+
+	for i, run := range funcs {
+		wg.Add(1)
+
+		go func() {
+			defer wg.Done()
+
+			<-start
+
+			errs[i] = run()
+		}()
+	}
+
+	close(start)
+	wg.Wait()
+
+	return errs
+}
+
+// joinConcurrently creates a session for each nick and has
+// them all JOIN channel at once. It returns the result and
+// error of each JOIN, in the order of nicks.
+func joinConcurrently(
+	ctx context.Context,
+	t *testing.T,
+	env *testEnv,
+	channel string,
+	nicks ...string,
+) ([]*service.JoinResult, []error) {
+	t.Helper()
+
+	results := make([]*service.JoinResult, len(nicks))
+	joins := make([]func() error, len(nicks))
+
+	for i, nick := range nicks {
+		sessionID := createSession(ctx, t, env.db, nick)
+
+		joins[i] = func() error {
+			result, err := env.svc.JoinChannel(
+				ctx, sessionID, nick, channel, "",
+			)
+			results[i] = result
+
+			return err
+		}
+	}
+
+	return results, runConcurrently(joins...)
+}
+
+// TestConcurrentJoinsRespectUserLimit checks that of two
+// concurrent JOINs of a +l 2 channel with one member, one
+// gets in and the other gets ERR_CHANNELISFULL.
+func TestConcurrentJoinsRespectUserLimit(t *testing.T) {
+	const userLimit = 2
+
+	env := newTestEnv(t)
+	ctx := t.Context()
+
+	for trial := range concurrentJoinTrials {
+		channel := fmt.Sprintf("#full%d", trial)
+		opNick := fmt.Sprintf("op%d", trial)
+		opSID := createSession(ctx, t, env.db, opNick)
+
+		created, err := env.svc.JoinChannel(
+			ctx, opSID, opNick, channel, "",
+		)
+		if err != nil {
+			t.Fatalf("join %s: %v", opNick, err)
+		}
+
+		err = env.db.SetChannelUserLimit(
+			ctx, created.ChannelID, userLimit,
+		)
+		if err != nil {
+			t.Fatalf("set +l: %v", err)
+		}
+
+		_, errs := joinConcurrently(
+			ctx, t, env, channel,
+			fmt.Sprintf("a%d", trial), fmt.Sprintf("b%d", trial),
+		)
+
+		refused := 0
+
+		for _, joinErr := range errs {
+			var ircErr *service.IRCError
+
+			switch {
+			case joinErr == nil:
+			case errors.As(joinErr, &ircErr) &&
+				ircErr.Code == irc.ErrChannelIsFull:
+				refused++
+			default:
+				t.Errorf("trial %d: JOIN: %v", trial, joinErr)
+			}
+		}
+
+		members, err := env.db.ChannelMembers(ctx, created.ChannelID)
+		if err != nil {
+			t.Fatalf("channel members: %v", err)
+		}
+
+		if len(members) != userLimit || refused != 1 {
+			t.Errorf(
+				"trial %d: %d members and %d refused, want %d and 1",
+				trial, len(members), refused, userLimit,
+			)
+		}
+	}
+}
+
+// TestConcurrentFirstJoinsMakeOneOperator checks that of
+// two concurrent JOINs that create a channel, exactly one
+// makes its session the operator.
+func TestConcurrentFirstJoinsMakeOneOperator(t *testing.T) {
+	env := newTestEnv(t)
+	ctx := t.Context()
+
+	for trial := range concurrentJoinTrials {
+		results, errs := joinConcurrently(
+			ctx, t, env, fmt.Sprintf("#new%d", trial),
+			fmt.Sprintf("first%d", trial),
+			fmt.Sprintf("second%d", trial),
+		)
+
+		creators := 0
+
+		for i, err := range errs {
+			if err != nil {
+				t.Fatalf("trial %d: JOIN: %v", trial, err)
+			}
+
+			if results[i].IsCreator {
+				creators++
+			}
+		}
+
+		members, err := env.db.ChannelMembers(
+			ctx, results[0].ChannelID,
+		)
+		if err != nil {
+			t.Fatalf("channel members: %v", err)
+		}
+
+		operators := 0
+
+		for _, member := range members {
+			if member.IsOperator {
+				operators++
+			}
+		}
+
+		if creators != 1 || operators != 1 {
+			t.Errorf(
+				"trial %d: %d creators and %d operators, want 1 each",
+				trial, creators, operators,
+			)
+		}
+	}
+}
+
+// TestJoinConcurrentWithLastPart checks that a JOIN
+// concurrent with the PART of a channel's only member
+// either joins a channel that still exists or gets
+// ERR_NOSUCHCHANNEL, and never fails otherwise.
+func TestJoinConcurrentWithLastPart(t *testing.T) {
+	env := newTestEnv(t)
+	ctx := t.Context()
+
+	for trial := range concurrentJoinTrials {
+		channel := fmt.Sprintf("#last%d", trial)
+		leaverNick := fmt.Sprintf("leaver%d", trial)
+		leaverSID := createSession(ctx, t, env.db, leaverNick)
+		joinerNick := fmt.Sprintf("joiner%d", trial)
+		joinerSID := createSession(ctx, t, env.db, joinerNick)
+
+		_, err := env.svc.JoinChannel(
+			ctx, leaverSID, leaverNick, channel, "",
+		)
+		if err != nil {
+			t.Fatalf("join %s: %v", leaverNick, err)
+		}
+
+		errs := runConcurrently(
+			func() error {
+				return env.svc.PartChannel(
+					ctx, leaverSID, leaverNick, channel, "",
+				)
+			},
+			func() error {
+				_, joinErr := env.svc.JoinChannel(
+					ctx, joinerSID, joinerNick, channel, "",
+				)
+
+				return joinErr
+			},
+		)
+		if errs[0] != nil {
+			t.Fatalf("trial %d: PART: %v", trial, errs[0])
+		}
+
+		var ircErr *service.IRCError
+
+		switch {
+		case errs[1] == nil:
+			chID, lookupErr := env.db.GetChannelByName(ctx, channel)
+			isMember, _ := env.db.IsChannelMember(ctx, chID, joinerSID)
+
+			if lookupErr != nil || !isMember {
+				t.Errorf(
+					"trial %d: JOIN succeeded, but %s is not on %s",
+					trial, joinerNick, channel,
+				)
+			}
+		case errors.As(errs[1], &ircErr) &&
+			ircErr.Code == irc.ErrNoSuchChannel:
+		default:
+			t.Errorf(
+				"trial %d: JOIN got %v, want success or 403",
+				trial, errs[1],
+			)
+		}
+	}
+}
+
+// TestJoinUsesUpInvite checks that the JOIN an invite let
+// into a +i channel uses the invite up, so that a JOIN
+// after leaving gets ERR_INVITEONLYCHAN.
+func TestJoinUsesUpInvite(t *testing.T) {
+	const channel = "#club"
+
+	env := newTestEnv(t)
+	ctx := t.Context()
+
+	opSID := createSession(ctx, t, env.db, "op")
+	guestSID := createSession(ctx, t, env.db, "guest")
+
+	created, err := env.svc.JoinChannel(ctx, opSID, "op", channel, "")
+	if err != nil {
+		t.Fatalf("join op: %v", err)
+	}
+
+	err = env.db.SetChannelInviteOnly(ctx, created.ChannelID, true)
+	if err != nil {
+		t.Fatalf("set +i: %v", err)
+	}
+
+	_, err = env.svc.InviteUser(ctx, opSID, "op", "guest", channel)
+	if err != nil {
+		t.Fatalf("invite guest: %v", err)
+	}
+
+	_, err = env.svc.JoinChannel(ctx, guestSID, "guest", channel, "")
+	if err != nil {
+		t.Fatalf("invited join: %v", err)
+	}
+
+	err = env.svc.PartChannel(ctx, guestSID, "guest", channel, "")
+	if err != nil {
+		t.Fatalf("part guest: %v", err)
+	}
+
+	_, err = env.svc.JoinChannel(ctx, guestSID, "guest", channel, "")
+
+	var ircErr *service.IRCError
+	if !errors.As(err, &ircErr) || ircErr.Code != irc.ErrInviteOnlyChan {
+		t.Fatalf("second join: want ERR_INVITEONLYCHAN, got %v", err)
+	}
+}
+
+// TestJoinRefusedByHostmaskBan checks that a ban naming a
+// session's nick, username and hostname refuses its JOIN
+// with ERR_BANNEDFROMCHAN.
+func TestJoinRefusedByHostmaskBan(t *testing.T) {
+	const channel = "#club"
+
+	env := newTestEnv(t)
+	ctx := t.Context()
+
+	opSID := createSession(ctx, t, env.db, "op")
+
+	created, err := env.svc.JoinChannel(ctx, opSID, "op", channel, "")
+	if err != nil {
+		t.Fatalf("join op: %v", err)
+	}
+
+	err = env.db.AddChannelBan(
+		ctx, created.ChannelID, "guest!user@host.example", "op",
+	)
+	if err != nil {
+		t.Fatalf("add ban: %v", err)
+	}
+
+	guestSID, _, _, err := env.db.CreateSession(
+		ctx, "guest", "user", "host.example", "127.0.0.1",
+	)
+	if err != nil {
+		t.Fatalf("create session guest: %v", err)
+	}
+
+	_, err = env.svc.JoinChannel(ctx, guestSID, "guest", channel, "")
+
+	var ircErr *service.IRCError
+	if !errors.As(err, &ircErr) || ircErr.Code != irc.ErrBannedFromChan {
+		t.Fatalf("want ERR_BANNEDFROMCHAN, got %v", err)
+	}
+}
+
+// TestPartDeletesChannelOnlyWhenEmpty checks that a PART
+// keeps a channel other members are still on and deletes
+// the channel when its last member parts.
+func TestPartDeletesChannelOnlyWhenEmpty(t *testing.T) {
+	const channel = "#room"
+
+	env := newTestEnv(t)
+	ctx := t.Context()
+
+	aliceSID := createSession(ctx, t, env.db, "alice")
+	bobSID := createSession(ctx, t, env.db, "bob")
+
+	_, err := env.svc.JoinChannel(ctx, aliceSID, "alice", channel, "")
+	if err != nil {
+		t.Fatalf("join alice: %v", err)
+	}
+
+	_, err = env.svc.JoinChannel(ctx, bobSID, "bob", channel, "")
+	if err != nil {
+		t.Fatalf("join bob: %v", err)
+	}
+
+	err = env.svc.PartChannel(ctx, aliceSID, "alice", channel, "")
+	if err != nil {
+		t.Fatalf("part alice: %v", err)
+	}
+
+	chID, err := env.db.GetChannelByName(ctx, channel)
+	if err != nil {
+		t.Fatalf("channel gone while bob is on it: %v", err)
+	}
+
+	isMember, _ := env.db.IsChannelMember(ctx, chID, bobSID)
+	if !isMember {
+		t.Fatal("bob is not on the channel after alice parted")
+	}
+
+	err = env.svc.PartChannel(ctx, bobSID, "bob", channel, "")
+	if err != nil {
+		t.Fatalf("part bob: %v", err)
+	}
+
+	_, err = env.db.GetChannelByName(ctx, channel)
+	if err == nil {
+		t.Fatal("channel still exists after its last member parted")
+	}
+}
+
+// TestKickOfLastMemberDeletesChannel checks that a KICK
+// that leaves a channel with no members deletes it.
+func TestKickOfLastMemberDeletesChannel(t *testing.T) {
+	const channel = "#room"
+
+	env := newTestEnv(t)
+	ctx := t.Context()
+
+	sid := createSession(ctx, t, env.db, "alice")
+
+	_, err := env.svc.JoinChannel(ctx, sid, "alice", channel, "")
+	if err != nil {
+		t.Fatalf("join alice: %v", err)
+	}
+
+	err = env.svc.KickUser(ctx, sid, "alice", channel, "alice", "")
+	if err != nil {
+		t.Fatalf("kick alice: %v", err)
+	}
+
+	_, err = env.db.GetChannelByName(ctx, channel)
+	if err == nil {
+		t.Fatal("channel still exists after its last member was kicked")
+	}
+}
+
 func TestSendChannelMessage(t *testing.T) {
 	env := newTestEnv(t)
 	ctx := t.Context()