diff --git a/internal/web/service/tgbot/tgbot.go b/internal/web/service/tgbot/tgbot.go index d7514c733..54f2e9cc0 100644 --- a/internal/web/service/tgbot/tgbot.go +++ b/internal/web/service/tgbot/tgbot.go @@ -440,6 +440,14 @@ func (t *Tgbot) IsRunning() bool { return isRunning } +// adminSnapshot returns the admin chat list under the mutex Start and Stop +// replace it under: a torn slice header is not a harmless race. +func adminSnapshot() []int64 { + tgBotMutex.Lock() + defer tgBotMutex.Unlock() + return slices.Clone(adminIds) +} + // SetHostname sets the hostname for the bot. func (t *Tgbot) SetHostname() { host, err := os.Hostname() diff --git a/internal/web/service/tgbot/tgbot_admin_list_race_test.go b/internal/web/service/tgbot/tgbot_admin_list_race_test.go new file mode 100644 index 000000000..5f775e68d --- /dev/null +++ b/internal/web/service/tgbot/tgbot_admin_list_race_test.go @@ -0,0 +1,73 @@ +package tgbot + +import ( + "sync" + "testing" +) + +// Regression test: writers replace adminIds and isRunning under tgBotMutex, so +// a bare read of either is reported by -race (CI's `race` job). +func TestAdminListReadersShareTheWriterLock(t *testing.T) { + mock, _ := staleButtonServer(t, map[string]any{ + "sendMessage": map[string]any{"ok": true, "result": map[string]any{ + "message_id": 1, + "date": 0, + "chat": map[string]any{"id": 1, "type": "private"}, + }}, + }) + swapTestBot(t, mock.URL) + defer mock.Close() + + tgBotMutex.Lock() + origAdmins := adminIds + origRunning := isRunning + tgBotMutex.Unlock() + t.Cleanup(func() { + tgBotMutex.Lock() + adminIds = origAdmins + isRunning = origRunning + tgBotMutex.Unlock() + }) + + stop := make(chan struct{}) + var writer sync.WaitGroup + writer.Add(1) + go func() { + defer writer.Done() + for i := 0; ; i++ { + select { + case <-stop: + return + default: + } + // The same lock order Start and Stop write with. + tgBotMutex.Lock() + if i%2 == 0 { + adminIds = []int64{111, 222, 333} + } else { + adminIds = nil + } + isRunning = i%2 == 0 + tgBotMutex.Unlock() + } + }() + + tb := &Tgbot{} + var readers sync.WaitGroup + for range 4 { + readers.Add(1) + go func() { + defer readers.Done() + for range 300 { + checkAdmin(111) + _ = tb.IsRunning() + // SendMsgToTgbot reads isRunning itself; the mock bot keeps + // the live path cheap enough to run under -race. + tb.SendMsgToTgbot(1, "tick") + } + }() + } + readers.Wait() + close(stop) + writer.Wait() +} diff --git a/internal/web/service/tgbot/tgbot_report.go b/internal/web/service/tgbot/tgbot_report.go index b3237335a..e9ed58dff 100644 --- a/internal/web/service/tgbot/tgbot_report.go +++ b/internal/web/service/tgbot/tgbot_report.go @@ -53,10 +53,11 @@ func (t *Tgbot) SendBackupToAdmins() { logger.Error("Error in getting db backup: ", err) } dbFilename := t.serverService.BackupFilename("") - for i, adminId := range adminIds { + admins := adminSnapshot() + for i, adminId := range admins { t.sendBackupData(adminId, dbData, dbFilename) // Add delay between sends to avoid Telegram rate limits - if i < len(adminIds)-1 { + if i < len(admins)-1 { time.Sleep(1 * time.Second) } } @@ -67,7 +68,7 @@ func (t *Tgbot) sendExhaustedToAdmins() { if !t.IsRunning() { return } - for _, adminId := range adminIds { + for _, adminId := range adminSnapshot() { t.getExhausted(adminId) } } diff --git a/internal/web/service/tgbot/tgbot_router.go b/internal/web/service/tgbot/tgbot_router.go index 161e8ea61..73e4d1a30 100644 --- a/internal/web/service/tgbot/tgbot_router.go +++ b/internal/web/service/tgbot/tgbot_router.go @@ -1332,7 +1332,7 @@ func (t *Tgbot) answerCallback(callbackQuery *telego.CallbackQuery, isAdmin bool // checkAdmin checks if the given Telegram ID is an admin. func checkAdmin(tgId int64) bool { - return slices.Contains(adminIds, tgId) + return slices.Contains(adminSnapshot(), tgId) } // isClientSelfCallback reports whether a callback is per-user rather than diff --git a/internal/web/service/tgbot/tgbot_send.go b/internal/web/service/tgbot/tgbot_send.go index fd0e87b20..6765bbc0f 100644 --- a/internal/web/service/tgbot/tgbot_send.go +++ b/internal/web/service/tgbot/tgbot_send.go @@ -121,7 +121,7 @@ func splitMessageLines(block string, limit int) []string { // SendMsgToTgbot sends a message to the Telegram bot with optional reply markup. func (t *Tgbot) SendMsgToTgbot(chatId int64, msg string, replyMarkup ...telego.ReplyMarkup) { - if !isRunning { + if !t.IsRunning() { return } @@ -180,12 +180,13 @@ func (t *Tgbot) SendMsgToTgbot(chatId int64, msg string, replyMarkup ...telego.R // SendMsgToTgbotAdmins sends a message to all admin Telegram chats. func (t *Tgbot) SendMsgToTgbotAdmins(msg string, replyMarkup ...telego.ReplyMarkup) { + admins := adminSnapshot() if len(replyMarkup) > 0 { - for _, adminId := range adminIds { + for _, adminId := range admins { t.SendMsgToTgbot(adminId, msg, replyMarkup[0]) } } else { - for _, adminId := range adminIds { + for _, adminId := range admins { t.SendMsgToTgbot(adminId, msg) } }