Beta: clean up servers that opted out while the bot was offline

The bot remembers where it registered beta commands (beta_registered) and
removes them from servers that are no longer in the beta at the next start.
Failed cleanups are retried; a server the bot has left is forgotten.

Co-Authored-By: Claude Sonnet 5.5 <[email protected]>

internal/bot/beta_sync_test.go

@@ -0,0 +1,221 @@
+package bot
+
+import (
+	"context"
+	"encoding/json"
+	"io"
+	"net/http"
+	"regexp"
+	"sort"
+	"strconv"
+	"strings"
+	"sync"
+	"testing"
+
+	"velox-bot/internal/beta"
+	"velox-bot/internal/commands"
+
+	"github.com/bwmarrin/discordgo"
+)
+
+const (
+	appID      = "100"
+	betaServer = "1170934850157547600"
+	optedIn    = "222"
+	leftServer = "333"
+)
+
+// fakeDiscord answers the application-command calls the beta sync makes, from
+// memory, and records what it was asked to change.
+var registerOnce sync.Once
+
+type fakeDiscord struct {
+	mu       sync.Mutex
+	cmds     map[string][]*discordgo.ApplicationCommand // guild -> commands
+	failures map[string]int                             // guild -> status for every call
+	nextID   int
+	created  []string // "guild/name"
+	deleted  []string // "guild/name"
+}
+
+var cmdPath = regexp.MustCompile(`/applications/` + appID + `/guilds/(\d+)/commands(?:/(\d+))?$`)
+
+func (f *fakeDiscord) RoundTrip(r *http.Request) (*http.Response, error) {
+	f.mu.Lock()
+	defer f.mu.Unlock()
+	reply := func(code int, v any) (*http.Response, error) {
+		b, _ := json.Marshal(v)
+		return &http.Response{StatusCode: code, Body: io.NopCloser(strings.NewReader(string(b))), Header: http.Header{"Content-Type": {"application/json"}}, Request: r}, nil
+	}
+	m := cmdPath.FindStringSubmatch(r.URL.Path)
+	if m == nil {
+		return reply(404, map[string]any{"message": "unknown route"})
+	}
+	guild, id := m[1], m[2]
+	if code := f.failures[guild]; code != 0 {
+		return reply(code, map[string]any{"message": "failure", "code": 0})
+	}
+	switch r.Method {
+	case http.MethodGet:
+		return reply(200, f.cmds[guild])
+	case http.MethodPost:
+		var c discordgo.ApplicationCommand
+		_ = json.NewDecoder(r.Body).Decode(&c)
+		f.nextID++
+		c.ID = strconv.Itoa(1000 + f.nextID)
+		f.cmds[guild] = append(f.cmds[guild], &c)
+		f.created = append(f.created, guild+"/"+c.Name)
+		return reply(200, c)
+	case http.MethodDelete:
+		kept := f.cmds[guild][:0]
+		for _, c := range f.cmds[guild] {
+			if c.ID == id {
+				f.deleted = append(f.deleted, guild+"/"+c.Name)
+				continue
+			}
+			kept = append(kept, c)
+		}
+		f.cmds[guild] = kept
+		return reply(204, nil)
+	}
+	return reply(405, nil)
+}
+
+func (f *fakeDiscord) names(guild string) []string {
+	f.mu.Lock()
+	defer f.mu.Unlock()
+	var out []string
+	for _, c := range f.cmds[guild] {
+		out = append(out, c.Name)
+	}
+	sort.Strings(out)
+	return out
+}
+
+type fakeStore struct {
+	registered map[string]bool
+	servers    []string
+	failWrite  bool
+}
+
+func (s *fakeStore) GetBetaSettings(context.Context) (string, string, error) {
+	return betaServer, "555", nil
+}
+func (s *fakeStore) ListBetaGuilds(context.Context) ([]string, error) { return s.servers, nil }
+func (s *fakeStore) ListBetaRegistered(context.Context) ([]string, error) {
+	var out []string
+	for id := range s.registered {
+		out = append(out, id)
+	}
+	return out, nil
+}
+func (s *fakeStore) SetBetaRegistered(_ context.Context, id string, on bool) error {
+	if s.failWrite {
+		return io.ErrUnexpectedEOF
+	}
+	if on {
+		s.registered[id] = true
+	} else {
+		delete(s.registered, id)
+	}
+	return nil
+}
+
+func newTestBot(t *testing.T, d *fakeDiscord) *Bot {
+	t.Helper()
+	sess, err := discordgo.New("Bot x")
+	if err != nil {
+		t.Fatal(err)
+	}
+	sess.Client = &http.Client{Transport: d}
+	sess.MaxRestRetries = 0
+	return &Bot{
+		Session:        sess,
+		AppID:          appID,
+		betaRegistered: map[string]bool{},
+		betaKnown:      map[string]bool{},
+	}
+}
+
+func contains(list []string, v string) bool {
+	for _, x := range list {
+		if x == v {
+			return true
+		}
+	}
+	return false
+}
+
+func TestBetaCommandsFollowTheServers(t *testing.T) {
+	registerOnce.Do(func() {
+		commands.RegisterBeta(&discordgo.ApplicationCommand{Name: "betatest", Description: "test"}, func(*discordgo.Session, *discordgo.InteractionCreate) {})
+	})
+	d := &fakeDiscord{cmds: map[string][]*discordgo.ApplicationCommand{
+		// A server that opted out while the bot was offline still has them,
+		// next to a command that isn't a beta one.
+		leftServer: {{ID: "1", Name: "betatest"}, {ID: "2", Name: "ping"}},
+		// A stale command in the beta server, which the bot cleans up.
+		betaServer: {{ID: "3", Name: "oldthing"}},
+	}, failures: map[string]int{}}
+	store := &fakeStore{registered: map[string]bool{leftServer: true}, servers: []string{optedIn}}
+	b := newTestBot(t, d)
+	ctx := context.Background()
+
+	beta.Configure(betaServer, "555", store.servers)
+	b.syncBetaCommands(ctx, store)
+
+	if got := d.names(leftServer); len(got) != 1 || got[0] != "ping" {
+		t.Errorf("the beta command is removed from the server that left, nothing else: %v", got)
+	}
+	if got := d.names(optedIn); len(got) != 1 || got[0] != "betatest" {
+		t.Errorf("an opted-in server gets the beta commands but not /beta: %v", got)
+	}
+	got := d.names(betaServer)
+	if !contains(got, "beta") || !contains(got, "betatest") || contains(got, "oldthing") {
+		t.Errorf("the beta server gets /beta and the beta commands, and loses stale ones: %v", got)
+	}
+	if store.registered[leftServer] || !store.registered[optedIn] || !store.registered[betaServer] {
+		t.Errorf("the database remembers where the commands are now: %v", store.registered)
+	}
+
+	// The server opts out.
+	store.servers = nil
+	beta.Configure(betaServer, "555", store.servers)
+	b.syncBetaCommands(ctx, store)
+	if got := d.names(optedIn); len(got) != 0 {
+		t.Errorf("an opted-out server loses them within one round: %v", got)
+	}
+	if store.registered[optedIn] || !store.registered[betaServer] {
+		t.Errorf("only the beta server stays remembered: %v", store.registered)
+	}
+}
+
+func TestBetaCleanupIsRetriedAndGivesUpWhenTheBotIsGone(t *testing.T) {
+	d := &fakeDiscord{cmds: map[string][]*discordgo.ApplicationCommand{
+		leftServer: {{ID: "1", Name: "betatest"}},
+	}, failures: map[string]int{leftServer: 500}}
+	store := &fakeStore{registered: map[string]bool{leftServer: true}}
+	b := newTestBot(t, d)
+	ctx := context.Background()
+	beta.Configure(betaServer, "555", nil)
+
+	b.syncBetaCommands(ctx, store)
+	if !store.registered[leftServer] {
+		t.Fatal("a failed cleanup is remembered so it is tried again")
+	}
+
+	d.failures[leftServer] = 0
+	b.syncBetaCommands(ctx, store)
+	if store.registered[leftServer] || len(d.names(leftServer)) != 0 {
+		t.Errorf("the retry cleans up: %v, %v", store.registered, d.names(leftServer))
+	}
+
+	// The bot was removed from a server: nothing to clean, don't try forever.
+	store.registered["444"] = true
+	b.betaKnown["444"] = true
+	d.failures["444"] = 403
+	b.syncBetaCommands(ctx, store)
+	if store.registered["444"] {
+		t.Error("a server the bot can't reach any more is forgotten")
+	}
+}

internal/bot/bot.go

@@ -2,7 +2,9 @@ package bot
 
 import (
 	"context"
+	"errors"
 	"log"
+	"net/http"
 	"sync"
 	"time"
 	"velox-bot/internal/beta"
@@ -29,8 +31,10 @@ type Bot struct {
 	DBHost             string
 	LavalinkHost       string
 	LavalinkPass       string
-	betaRegistered     map[string]bool // servers where the beta commands are registered right now
-	betaRoles          sync.Map        // "guild:user" -> cachedRoles
+	betaRegistered     map[string]bool // servers where this run registered the beta commands
+	betaKnown          map[string]bool // servers where they were registered, now or in an earlier run
+	betaLoaded         bool
+	betaRoles          sync.Map // "guild:user" -> cachedRoles
 }
 
 func NewBot(token, appID, guildID, lavalinkHost, lavalinkPass string, cmds []*discordgo.ApplicationCommand, services *services.Services, musicRepo *musicrepo.Repo, dbHost string) (*Bot, error) {
@@ -54,6 +58,7 @@ func NewBot(token, appID, guildID, lavalinkHost, lavalinkPass string, cmds []*di
 
 	return &Bot{
 		betaRegistered: map[string]bool{},
+		betaKnown:      map[string]bool{},
 		Session:        session,
 		AppID:          appID,
 		GuildID:        guildID,
@@ -210,21 +215,37 @@ func (b *Bot) Start() error {
 // made on the dashboard.
 const betaPollEvery = 30 * time.Second
 
+// BetaStore is where the bot reads the beta settings the dashboard writes,
+// and remembers which servers it registered beta commands in.
+type BetaStore interface {
+	GetBetaSettings(ctx context.Context) (guildID, roleID string, err error)
+	ListBetaGuilds(ctx context.Context) ([]string, error)
+	ListBetaRegistered(ctx context.Context) ([]string, error)
+	SetBetaRegistered(ctx context.Context, guildID string, registered bool) error
+}
+
 // WatchBeta keeps the bot's idea of who the beta testers are, and which
 // servers opted in, in step with the dashboard. When the list of servers
 // changes, the beta commands follow: they appear in servers that opted in and
-// are removed from servers that opted out. If the settings can't be read, the
-// last known ones stay in force (and before the first read, beta stays off).
-func (b *Bot) WatchBeta(ctx context.Context, load func(context.Context) (guildID, roleID string, servers []string, err error)) {
+// are removed from servers that opted out, also if that happened while the bot
+// was offline (it remembers where it registered them). If the settings can't
+// be read, the last known ones stay in force (and before the first read, beta
+// stays off).
+func (b *Bot) WatchBeta(ctx context.Context, store BetaStore) {
 	beta.SetRolesLookup(b.betaMemberRoles)
 	sync := func() {
-		guildID, roleID, servers, err := load(ctx)
+		guildID, roleID, err := store.GetBetaSettings(ctx)
 		if err != nil {
 			log.Printf("beta: cannot read settings, keeping the last ones: %v", err)
 			return
 		}
+		servers, err := store.ListBetaGuilds(ctx)
+		if err != nil {
+			log.Printf("beta: cannot read the opted-in servers, keeping the last ones: %v", err)
+			return
+		}
 		beta.Configure(guildID, roleID, servers)
-		b.syncBetaCommands()
+		b.syncBetaCommands(ctx, store)
 	}
 	sync()
 	go func() {
@@ -274,8 +295,22 @@ func (b *Bot) betaMemberRoles(guildID, userID string) ([]string, error) {
 
 // syncBetaCommands makes the beta commands exist in exactly the servers where
 // beta features are on (the beta server and the ones that opted in), and
-// nowhere else. A server whose registration failed is retried next time.
-func (b *Bot) syncBetaCommands() {
+// nowhere else. The servers it registered them in are remembered in the
+// database, so a server that opted out while the bot was offline is cleaned up
+// at the next start. A step that failed is retried on the next round.
+func (b *Bot) syncBetaCommands(ctx context.Context, store BetaStore) {
+	if !b.betaLoaded {
+		known, err := store.ListBetaRegistered(ctx)
+		if err != nil {
+			log.Printf("beta: cannot read where beta commands were registered: %v", err)
+			return
+		}
+		for _, id := range known {
+			b.betaKnown[id] = true
+		}
+		b.betaLoaded = true
+	}
+
 	want := map[string]bool{}
 	if g := beta.GuildID(); g != "" {
 		want[g] = true
@@ -283,16 +318,30 @@ func (b *Bot) syncBetaCommands() {
 	for _, id := range beta.Servers() {
 		want[id] = true
 	}
-	for id := range b.betaRegistered {
-		if !want[id] {
-			b.removeBetaCommands(id)
-			delete(b.betaRegistered, id)
+	for id := range b.betaKnown {
+		if want[id] {
+			continue
 		}
+		if !b.removeBetaCommands(id) {
+			continue
+		}
+		if err := store.SetBetaRegistered(ctx, id, false); err != nil {
+			log.Printf("beta: cannot forget server %s: %v", id, err)
+			continue
+		}
+		delete(b.betaKnown, id)
+		delete(b.betaRegistered, id)
 	}
 	for id := range want {
-		if !b.betaRegistered[id] && b.addBetaCommands(id) {
-			b.betaRegistered[id] = true
+		if b.betaRegistered[id] || !b.addBetaCommands(id) {
+			continue
 		}
+		b.betaRegistered[id] = true
+		if err := store.SetBetaRegistered(ctx, id, true); err != nil {
+			log.Printf("beta: cannot remember server %s: %v", id, err)
+			continue
+		}
+		b.betaKnown[id] = true
 	}
 }
 
@@ -339,26 +388,44 @@ func (b *Bot) addBetaCommands(guildID string) bool {
 	return ok
 }
 
+// goneFromServer reports whether an error from Discord means the bot isn't in
+// that server any more (or it no longer exists), so there is nothing left to
+// clean up there.
+func goneFromServer(err error) bool {
+	var rest *discordgo.RESTError
+	if errors.As(err, &rest) && rest.Response != nil {
+		return rest.Response.StatusCode == http.StatusForbidden || rest.Response.StatusCode == http.StatusNotFound
+	}
+	return false
+}
+
 // removeBetaCommands takes the beta commands out of a server that is no longer
-// part of the beta, and leaves everything else there alone.
-func (b *Bot) removeBetaCommands(guildID string) {
+// part of the beta, and leaves everything else there alone. It reports whether
+// the server is clean (or out of reach for good).
+func (b *Bot) removeBetaCommands(guildID string) bool {
 	isBeta := map[string]bool{}
 	for _, cmd := range commands.BetaCommands {
 		isBeta[cmd.Name] = true
 	}
 	existing, err := b.Session.ApplicationCommands(b.AppID, guildID)
 	if err != nil {
+		if goneFromServer(err) {
+			return true
+		}
 		log.Printf("beta: cannot list commands in %s: %v", guildID, err)
-		return
+		return false
 	}
+	clean := true
 	for _, cmd := range existing {
 		if !isBeta[cmd.Name] {
 			continue
 		}
-		if err := b.Session.ApplicationCommandDelete(b.AppID, guildID, cmd.ID); err != nil {
+		if err := b.Session.ApplicationCommandDelete(b.AppID, guildID, cmd.ID); err != nil && !goneFromServer(err) {
 			log.Printf("beta: cannot remove '%v' from %s: %v", cmd.Name, guildID, err)
+			clean = false
 		}
 	}
+	return clean
 }
 
 func (b *Bot) Close() error {