From b70f1d859d8854deb746dda5fb0e53cb203fac6f Mon Sep 17 00:00:00 2001 From: Harrison Healey Date: Tue, 20 May 2025 16:02:32 -0400 Subject: [PATCH] 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 --- server/channels/api4/channel_category.go | 4 + server/channels/api4/channel_category_test.go | 6 +- .../sqlstore/channel_store_categories.go | 153 +++++----- .../storetest/channel_store_categories.go | 263 ++++++++++++++++++ 4 files changed, 356 insertions(+), 70 deletions(-) diff --git a/server/channels/api4/channel_category.go b/server/channels/api4/channel_category.go index c6f07e1091..500a516b4c 100644 --- a/server/channels/api4/channel_category.go +++ b/server/channels/api4/channel_category.go @@ -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 } diff --git a/server/channels/api4/channel_category_test.go b/server/channels/api4/channel_category_test.go index dbd5c2cef8..da8bcb2723 100644 --- a/server/channels/api4/channel_category_test.go +++ b/server/channels/api4/channel_category_test.go @@ -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) }) } diff --git a/server/channels/store/sqlstore/channel_store_categories.go b/server/channels/store/sqlstore/channel_store_categories.go index 95502e2ab6..6c4c528a27 100644 --- a/server/channels/store/sqlstore/channel_store_categories.go +++ b/server/channels/store/sqlstore/channel_store_categories.go @@ -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 diff --git a/server/channels/store/storetest/channel_store_categories.go b/server/channels/store/storetest/channel_store_categories.go index df7a51b56c..23a2db61dd 100644 --- a/server/channels/store/storetest/channel_store_categories.go +++ b/server/channels/store/storetest/channel_store_categories.go @@ -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) + } +}