MM-63923/MM-63924/MM-63925 Prevent deadlocks and constraint errors in UpdateSidebarCategories (#30965)

* MM-63925 Remove most nested transactions from channel_store_categories.go

There's one place which still has a nested transaction in
CreateInitialSidebarCategories, but that's because it's calling out to a
different part of the store. The only way to avoid that would be to
break the extraction like UpdateSidebarCategories does to update
preferences, but I chose not to follow that pattern here and leave it
as-is.

* MM-63923 Prevent deadlocks caused by updating multiple categories in a different order

* MM-63923 Prevent deadlocks while deleting from SidebarChannels

This could also have been resolved by sorting the categories, but
combining the queries seems a bit more elegant.

* MM-63924 Ensure adding SidebarChannels rows is idempotent

* Add additional test to cause deadlocks

* Prevent channels from appearing in a single category multiple times

* Other review feedback
Этот коммит содержится в:
Harrison Healey
2025-05-20 16:02:32 -04:00
коммит произвёл GitHub
родитель fa40a8c5d4
Коммит b70f1d859d
4 изменённых файлов: 356 добавлений и 70 удалений

Просмотреть файл

@@ -257,6 +257,8 @@ func validateSidebarCategories(c *Context, teamId, userId string, categories []*
return nil
}
// validateSidebarCategoryChannels returns a normalized slice of channel IDs by removing duplicates from it and
// ensuring that it only contains IDs of channels in the given ChannelList.
func validateSidebarCategoryChannels(c *Context, userId string, channelIds []string, channels model.ChannelList) []string {
var filtered []string
@@ -276,6 +278,8 @@ func validateSidebarCategoryChannels(c *Context, userId string, channelIds []str
}
}
filtered = model.RemoveDuplicateStringsNonSort(filtered)
return filtered
}

Просмотреть файл

@@ -1063,7 +1063,7 @@ func TestValidateSidebarCategoryChannels(t *testing.T) {
require.Empty(t, filtered)
})
t.Run("should preserve duplicate channel IDs", func(t *testing.T) {
t.Run("should prevent duplicate channel IDs", func(t *testing.T) {
channels := model.ChannelList{
th.BasicChannel,
}
@@ -1075,8 +1075,8 @@ func TestValidateSidebarCategoryChannels(t *testing.T) {
}
filtered := validateSidebarCategoryChannels(c, th.BasicUser.Id, channelIds, channels)
require.Len(t, filtered, 2) // Function preserves duplicates as per implementation
require.Equal(t, []string{th.BasicChannel.Id, th.BasicChannel.Id}, filtered)
require.Len(t, filtered, 1)
require.Equal(t, []string{th.BasicChannel.Id}, filtered)
})
}

Просмотреть файл

@@ -5,6 +5,8 @@ package sqlstore
import (
"fmt"
"slices"
"strings"
sq "github.com/mattermost/squirrel"
"github.com/pkg/errors"
@@ -418,25 +420,6 @@ func (s SqlChannelStore) CreateSidebarCategory(userId, teamId string, newCategor
return result, nil
}
func (s SqlChannelStore) completePopulatingCategoryChannels(category *model.SidebarCategoryWithChannels) (_ *model.SidebarCategoryWithChannels, err error) {
transaction, err := s.GetMaster().Beginx()
if err != nil {
return nil, errors.Wrap(err, "begin_transaction")
}
defer finalizeTransactionX(transaction, &err)
result, err := s.completePopulatingCategoryChannelsT(transaction, category)
if err != nil {
return nil, err
}
if err = transaction.Commit(); err != nil {
return nil, errors.Wrap(err, "commit_transaction")
}
return result, nil
}
func (s SqlChannelStore) completePopulatingCategoryChannelsT(db dbSelecter, category *model.SidebarCategoryWithChannels) (*model.SidebarCategoryWithChannels, error) {
if category.Type == model.SidebarCategoryCustom || category.Type == model.SidebarCategoryFavorites {
return category, nil
@@ -515,6 +498,10 @@ func (s SqlChannelStore) completePopulatingCategoryChannelsT(db dbSelecter, cate
}
func (s SqlChannelStore) GetSidebarCategory(categoryId string) (*model.SidebarCategoryWithChannels, error) {
return s.getSidebarCategoryT(s.GetReplica(), categoryId)
}
func (s SqlChannelStore) getSidebarCategoryT(db dbSelecter, categoryId string) (*model.SidebarCategoryWithChannels, error) {
query := s.sidebarCategorySelectQuery.
Columns("SidebarChannels.ChannelId").
LeftJoin("SidebarChannels ON SidebarChannels.CategoryId=SidebarCategories.Id").
@@ -527,7 +514,7 @@ func (s SqlChannelStore) GetSidebarCategory(categoryId string) (*model.SidebarCa
}
categories := []*sidebarCategoryForJoin{}
if err = s.GetReplica().Select(&categories, sql, args...); err != nil {
if err = db.Select(&categories, sql, args...); err != nil {
return nil, errors.Wrap(err, fmt.Sprintf("failed to get category with id=%s", categoryId))
}
@@ -544,7 +531,7 @@ func (s SqlChannelStore) GetSidebarCategory(categoryId string) (*model.SidebarCa
result.Channels = append(result.Channels, *category.ChannelId)
}
}
return s.completePopulatingCategoryChannels(result)
return s.completePopulatingCategoryChannelsT(db, result)
}
func (s SqlChannelStore) getSidebarCategoriesT(db dbSelecter, userId string, opts *store.SidebarCategorySearchOpts) (*model.OrderedSidebarCategories, error) {
@@ -630,6 +617,10 @@ func (s SqlChannelStore) GetSidebarCategories(userID string, opts *store.Sidebar
}
func (s SqlChannelStore) GetSidebarCategoryOrder(userId, teamId string) ([]string, error) {
return s.getSidebarCategoryOrderT(s.GetReplica(), userId, teamId)
}
func (s SqlChannelStore) getSidebarCategoryOrderT(db dbSelecter, userId, teamId string) ([]string, error) {
ids := []string{}
sql, args, err := s.getQueryBuilder().
@@ -645,7 +636,7 @@ func (s SqlChannelStore) GetSidebarCategoryOrder(userId, teamId string) ([]strin
return nil, errors.Wrap(err, "sidebar_category_tosql")
}
if err := s.GetReplica().Select(&ids, sql, args...); err != nil {
if err := db.Select(&ids, sql, args...); err != nil {
return nil, errors.Wrap(err, fmt.Sprintf("failed to get category order for userId=%s, teamId=%s", userId, teamId))
}
@@ -680,7 +671,7 @@ func (s SqlChannelStore) UpdateSidebarCategoryOrder(userId, teamId string, categ
defer finalizeTransactionX(transaction, &err)
// Ensure no invalid categories are included and that no categories are left out
existingOrder, err := s.GetSidebarCategoryOrder(userId, teamId)
existingOrder, err := s.getSidebarCategoryOrderT(transaction, userId, teamId)
if err != nil {
return err
}
@@ -724,7 +715,7 @@ func (s SqlChannelStore) UpdateSidebarCategories(userId, teamId string, categori
updatedCategories := []*model.SidebarCategoryWithChannels{}
originalCategories := []*model.SidebarCategoryWithChannels{}
for _, category := range categories {
srcCategory, err2 := s.GetSidebarCategory(category.Id)
srcCategory, err2 := s.getSidebarCategoryT(transaction, category.Id)
if err2 != nil {
return nil, nil, errors.Wrap(err2, "failed to find SidebarCategories")
}
@@ -752,11 +743,24 @@ func (s SqlChannelStore) UpdateSidebarCategories(userId, teamId string, categori
destCategory.Muted = category.Muted
}
// The order in which the queries are executed in the transaction is important.
// SidebarCategories need to be update first, and then SidebarChannels should be deleted.
// The net effect remains the same, but it prevents deadlocks from other transactions
// operating on the tables in reverse order.
updatedCategories = append(updatedCategories, destCategory)
originalCategories = append(originalCategories, srcCategory)
}
// The order in which the queries are executed in the transaction is important.
// SidebarCategories need to be update first, and then SidebarChannels should be deleted.
// The net effect remains the same, but it prevents deadlocks from other transactions
// operating on the tables in reverse order.
// Similarly, sort the categories when updating SidebarCategories to prevent deadlocks that would occur
// if multiple transactions were to update the table in reverse order.
sortedUpdatedCategories := slices.Clone(updatedCategories)
slices.SortFunc(sortedUpdatedCategories, func(a *model.SidebarCategoryWithChannels, b *model.SidebarCategoryWithChannels) int {
return strings.Compare(a.Id, b.Id)
})
// First, update the categories themselves
for _, destCategory := range sortedUpdatedCategories {
updateQuery, updateParams, err2 := s.getQueryBuilder().
Update("SidebarCategories").
Set("DisplayName", destCategory.DisplayName).
@@ -770,49 +774,67 @@ func (s SqlChannelStore) UpdateSidebarCategories(userId, teamId string, categori
if _, err = transaction.Exec(updateQuery, updateParams...); err != nil {
return nil, nil, errors.Wrap(err, "failed to update SidebarCategories")
}
}
// if we are updating DM category, it's order can't channel order cannot be changed.
if category.Type != model.SidebarCategoryDirectMessages {
// Remove any SidebarChannels entries that were either:
// - previously in this category (and any ones that are still in the category will be recreated below)
// - in another category and are being added to this category
query, args, err2 := s.getQueryBuilder().
Delete("SidebarChannels").
Where(
sq.And{
sq.Eq{"ChannelId": srcCategory.Channels},
sq.Eq{"CategoryId": category.Id},
},
).ToSql()
// Second, update the channels in those categories
categoryIds := make([]string, len(categories))
for i, category := range categories {
categoryIds[i] = category.Id
}
// Remove any SidebarChannels entries that were previously in this category. This needs to be done for all
// categories at once to prevent deadlocks.
//
// Note that this means that moving channels between categories requires updating both the source and
// destination categories.
query, args, err2 := s.getQueryBuilder().
Delete("SidebarChannels").
Where(sq.Eq{"CategoryId": categoryIds}).ToSql()
if err2 != nil {
return nil, nil, errors.Wrap(err2, "update_sidebar_categories_tosql2")
}
if _, err = transaction.Exec(query, args...); err != nil {
return nil, nil, errors.Wrap(err, "failed to delete SidebarChannels")
}
for _, category := range categories {
if category.Type == model.SidebarCategoryDirectMessages {
// The order of the DM category isn't stored explicitly, so there's nothing to do here
continue
}
runningOrder := 0
insertQuery := s.getQueryBuilder().
Insert("SidebarChannels").
Columns("ChannelId", "UserId", "CategoryId", "SortOrder")
if s.DriverName() == model.DatabaseDriverMysql {
insertQuery = insertQuery.Suffix("ON DUPLICATE KEY UPDATE SortOrder = VALUES(SortOrder)")
} else {
insertQuery = insertQuery.Suffix("ON CONFLICT (ChannelId, UserId, CategoryId) DO UPDATE SET SortOrder = excluded.SortOrder")
}
for _, channelID := range category.Channels {
insertQuery = insertQuery.Values(channelID, userId, category.Id, int64(runningOrder))
runningOrder += model.MinimalSidebarSortDistance
}
if len(category.Channels) > 0 {
sql, args, err2 := insertQuery.ToSql()
if err2 != nil {
return nil, nil, errors.Wrap(err2, "update_sidebar_categories_tosql2")
return nil, nil, errors.Wrap(err2, "InsertSidebarChannels_Tosql")
}
if _, err = transaction.Exec(query, args...); err != nil {
return nil, nil, errors.Wrap(err, "failed to delete SidebarChannels")
}
runningOrder := 0
insertQuery := s.getQueryBuilder().
Insert("SidebarChannels").
Columns("ChannelId", "UserId", "CategoryId", "SortOrder")
for _, channelID := range category.Channels {
insertQuery = insertQuery.Values(channelID, userId, category.Id, int64(runningOrder))
runningOrder += model.MinimalSidebarSortDistance
}
if len(category.Channels) > 0 {
sql, args, err2 := insertQuery.ToSql()
if err2 != nil {
return nil, nil, errors.Wrap(err2, "InsertSidebarChannels_Tosql")
}
if _, err2 := transaction.Exec(sql, args...); err2 != nil {
return nil, nil, errors.Wrap(err2, "failed to save SidebarChannels")
}
if _, err2 := transaction.Exec(sql, args...); err2 != nil {
return nil, nil, errors.Wrap(err2, "failed to save SidebarChannels")
}
}
}
// Finally, update preferences for Favorites
for i, category := range categories {
srcCategory := originalCategories[i]
// Update the favorites preferences based on channels moving into or out of the Favorites category for compatibility
if category.Type == model.SidebarCategoryFavorites {
@@ -862,9 +884,6 @@ func (s SqlChannelStore) UpdateSidebarCategories(userId, teamId string, categori
return nil, nil, errors.Wrap(nErr, "failed to delete Preferences")
}
}
updatedCategories = append(updatedCategories, destCategory)
originalCategories = append(originalCategories, srcCategory)
}
// Ensure Channels are populated for Channels/Direct Messages category if they change

Просмотреть файл

@@ -6,8 +6,11 @@ package storetest
import (
"database/sql"
"errors"
"fmt"
"math/rand"
"sync"
"testing"
"time"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
@@ -27,6 +30,7 @@ func TestChannelStoreCategories(t *testing.T, rctx request.CTX, ss store.Store,
t.Run("DeleteSidebarCategory", func(t *testing.T) { testDeleteSidebarCategory(t, rctx, ss, s) })
t.Run("UpdateSidebarChannelsByPreferences", func(t *testing.T) { testUpdateSidebarChannelsByPreferences(t, rctx, ss) })
t.Run("SidebarCategoryDeadlock", func(t *testing.T) { testSidebarCategoryDeadlock(t, rctx, ss) })
t.Run("SidebarCategoryConcurrentAccess", func(t *testing.T) { testSidebarCategoryConcurrentAccess(t, rctx, ss, s) })
}
func setupTeam(t *testing.T, rctx request.CTX, ss store.Store, userIds ...string) *model.Team {
@@ -2413,3 +2417,262 @@ func testSidebarCategoryDeadlock(t *testing.T, rctx request.CTX, ss store.Store)
wg.Wait()
}
func testSidebarCategoryConcurrentAccess(t *testing.T, rctx request.CTX, ss store.Store, s SqlStore) {
if s.DriverName() == model.DatabaseDriverMysql {
t.Skip("This is known to fail on MySQL")
}
for i := 0; i < 2; i++ {
i := i
t.Run(fmt.Sprint(i), func(t *testing.T) {
t.Parallel()
doTestSidebarCategoryConcurrentAccess(t, rctx, ss)
})
}
}
func doTestSidebarCategoryConcurrentAccess(t *testing.T, rctx request.CTX, ss store.Store) {
userID := model.NewId()
team := setupTeam(t, rctx, ss, userID)
numGoroutines := 20
// Create regular channels
channels := make([]*model.Channel, 5)
for i := 0; i < len(channels); i++ {
channel, nErr := ss.Channel().Save(rctx, &model.Channel{
Name: fmt.Sprintf("channel-%d", i),
DisplayName: fmt.Sprintf("Channel %d", i),
Type: model.ChannelTypeOpen,
TeamId: team.Id,
}, 10)
require.NoError(t, nErr)
_, err := ss.Channel().SaveMember(rctx, &model.ChannelMember{
UserId: userID,
ChannelId: channel.Id,
NotifyProps: model.GetDefaultChannelNotifyProps(),
})
require.NoError(t, err)
channels[i] = channel
}
// Create DM channels to exercise different code paths
dmChannels := make([]*model.Channel, 3)
for i := 0; i < len(dmChannels); i++ {
otherUserID := model.NewId()
dmChannel, nErr := ss.Channel().CreateDirectChannel(rctx, &model.User{
Id: userID,
}, &model.User{
Id: otherUserID,
})
require.NoError(t, nErr)
dmChannels[i] = dmChannel
}
// Put them into a channel so that we can ensure they're evenly used across goroutines, and put 4x the number of
// goroutines in since that should ensure we have enough
channelChan := make(chan string, 4*numGoroutines)
for i := 0; i < cap(channelChan); i++ {
channelChan <- channels[i%len(channels)].Id
}
dmChannelChan := make(chan string, 4*numGoroutines)
for i := 0; i < cap(dmChannelChan); i++ {
dmChannelChan <- dmChannels[i%len(dmChannels)].Id
}
// Create the initial categories
opts := &store.SidebarCategorySearchOpts{
TeamID: team.Id,
ExcludeTeam: false,
}
res, nErr := ss.Channel().CreateInitialSidebarCategories(rctx, userID, opts)
require.NoError(t, nErr)
require.NotEmpty(t, res)
initialCategories, err := ss.Channel().GetSidebarCategoriesForTeamForUser(userID, team.Id)
require.NoError(t, err)
// Create custom categories
customCategories := make([]*model.SidebarCategoryWithChannels, 2)
for i := 0; i < 2; i++ {
customCategory, createErr := ss.Channel().CreateSidebarCategory(userID, team.Id, &model.SidebarCategoryWithChannels{
SidebarCategory: model.SidebarCategory{
DisplayName: fmt.Sprintf("Custom Category %d", i),
},
})
require.NoError(t, createErr)
customCategories[i] = customCategory
}
// Run concurrent operations
var wg sync.WaitGroup
for i := 0; i < numGoroutines; i++ {
wg.Add(1)
// Run GetSidebarCategoriesForTeamForUser
go func() {
defer wg.Done()
categories, getErr := ss.Channel().GetSidebarCategoriesForTeamForUser(userID, team.Id)
require.NoError(t, getErr)
require.NotEmpty(t, categories.Categories)
}()
// Run UpdateSidebarCategories with different update patterns
wg.Add(1)
go func(iteration int) {
defer wg.Done()
var updatedCategories []*model.SidebarCategoryWithChannels
switch iteration % 8 {
case 0:
// Move a regular channel to a custom category
updatedCategories = []*model.SidebarCategoryWithChannels{
{
SidebarCategory: initialCategories.Categories[1].SidebarCategory, // Channels category
Channels: []string{},
},
{
SidebarCategory: customCategories[0].SidebarCategory,
Channels: []string{<-channelChan, <-channelChan, <-channelChan},
},
}
case 1:
// Move a regular channel back from a custom category
updatedCategories = []*model.SidebarCategoryWithChannels{
{
SidebarCategory: initialCategories.Categories[1].SidebarCategory, // Channels category
Channels: []string{<-channelChan, <-channelChan, <-channelChan},
},
{
SidebarCategory: customCategories[0].SidebarCategory,
Channels: []string{},
},
}
case 2:
// Move a DM channel to a custom category
updatedCategories = []*model.SidebarCategoryWithChannels{
{
SidebarCategory: initialCategories.Categories[2].SidebarCategory, // DMs category
Channels: []string{},
},
{
SidebarCategory: customCategories[1].SidebarCategory,
Channels: []string{<-dmChannelChan, <-channelChan, <-channelChan},
},
}
case 3:
// Move a DM channel back from to a custom category
updatedCategories = []*model.SidebarCategoryWithChannels{
{
SidebarCategory: initialCategories.Categories[2].SidebarCategory, // DMs category
Channels: []string{<-dmChannelChan},
},
{
SidebarCategory: customCategories[1].SidebarCategory,
Channels: []string{<-channelChan, <-channelChan},
},
}
case 4:
// Move a channel between custom categories
updatedCategories = []*model.SidebarCategoryWithChannels{
{
SidebarCategory: customCategories[0].SidebarCategory,
Channels: []string{<-channelChan, <-channelChan, <-channelChan},
},
{
SidebarCategory: customCategories[1].SidebarCategory,
Channels: []string{<-channelChan},
},
}
case 5:
// Move a channel back between custom categories
updatedCategories = []*model.SidebarCategoryWithChannels{
{
SidebarCategory: customCategories[0].SidebarCategory,
Channels: []string{<-channelChan},
},
{
SidebarCategory: customCategories[1].SidebarCategory,
Channels: []string{<-channelChan, <-channelChan, <-channelChan},
},
}
case 6:
// Add to favorites category (triggers preference updates)
updatedCategories = []*model.SidebarCategoryWithChannels{
{
SidebarCategory: initialCategories.Categories[0].SidebarCategory, // Favorites category
Channels: []string{<-channelChan, <-dmChannelChan},
},
}
case 7:
// Update category properties
customCatIndex := rand.Intn(2)
updatedCategories = []*model.SidebarCategoryWithChannels{
{
SidebarCategory: model.SidebarCategory{
Id: customCategories[customCatIndex].Id,
DisplayName: fmt.Sprintf("Updated Name %d", iteration),
Sorting: model.SidebarCategorySortRecent,
Muted: iteration%2 == 0,
},
Channels: customCategories[customCatIndex].Channels,
},
}
}
// Remove duplicates to prevent database errors when updating the SidebarChannels table
for i, category := range updatedCategories {
updatedCategories[i].Channels = model.RemoveDuplicateStringsNonSort(category.Channels)
}
_, _, updateErr := ss.Channel().UpdateSidebarCategories(userID, team.Id, updatedCategories)
if err != nil {
require.NoError(t, fmt.Errorf("[iteration %d]: %v", iteration, updateErr))
}
}(i)
// Small sleep to vary timing between iterations
time.Sleep(time.Millisecond * time.Duration(rand.Intn(3)))
}
// Wait with timeout to catch any deadlocks
done := make(chan struct{})
go func() {
wg.Wait()
close(done)
}()
select {
case <-done:
// All goroutines completed successfully
case <-time.After(30 * time.Second):
t.Log("Test timed out, likely deadlock")
t.FailNow()
}
// Verify the final state is valid
finalCategories, err := ss.Channel().GetSidebarCategoriesForTeamForUser(userID, team.Id)
require.NoError(t, err)
require.GreaterOrEqual(t, len(finalCategories.Categories), 5, "Should have at least 5 categories (3 default + 2 custom)")
// Verify each channel is assigned to a category
channelFound := make(map[string]bool)
for _, channel := range channels {
channelFound[channel.Id] = false
}
for _, category := range finalCategories.Categories {
for _, channelId := range category.Channels {
if _, exists := channelFound[channelId]; exists {
channelFound[channelId] = true
}
}
}
// Every channel should be found in at least one category
for channelId, found := range channelFound {
require.True(t, found, "Channel %s should be assigned to at least one category", channelId)
}
}