Add missing transaction rollbacks for SQL store (#9964)

* Add missing transaction rollbacks for SQL store

* Add `defer transaction.Rollback()` in SQL stores.

* abstract finalizeTransaction

store/sqlstore/channel_store.go

@@ -334,12 +334,7 @@ func (s SqlChannelStore) CreateIndexesIfNotExists() {
 // MigratePublicChannels initializes the PublicChannels table with data created before this version
 // of the Mattermost server kept it up-to-date.
 func (s SqlChannelStore) MigratePublicChannels() error {
-	transaction, err := s.GetMaster().Begin()
+	if _, err := s.GetMaster().Exec(`
-	if err != nil {
-		return err
-	}
-
-	if _, err := transaction.Exec(`
 		INSERT INTO PublicChannels
 		    (Id, DeleteAt, TeamId, DisplayName, Name, Header, Purpose)
 		SELECT
@@ -355,10 +350,6 @@ func (s SqlChannelStore) MigratePublicChannels() error {
 		return err
 	}
 
-	if err := transaction.Commit(); err != nil {
-		return err
-	}
-
 	return nil
 }
 
@@ -443,16 +434,15 @@ func (s SqlChannelStore) Save(channel *model.Channel, maxChannelsPerTeam int64)
 			result.Err = model.NewAppError("SqlChannelStore.Save", "store.sql_channel.save.open_transaction.app_error", nil, err.Error(), http.StatusInternalServerError)
 			return
 		}
+		defer finalizeTransaction(transaction)
 
 		*result = s.saveChannelT(transaction, channel, maxChannelsPerTeam)
 		if result.Err != nil {
-			transaction.Rollback()
 			return
 		}
 
 		// Additionally propagate the write to the PublicChannels table.
 		if err := s.upsertPublicChannelT(transaction, result.Data.(*model.Channel)); err != nil {
-			transaction.Rollback()
 			result.Err = model.NewAppError("SqlChannelStore.Save", "store.sql_channel.save.upsert_public_channel.app_error", nil, err.Error(), http.StatusInternalServerError)
 			return
 		}
@@ -504,12 +494,12 @@ func (s SqlChannelStore) SaveDirectChannel(directchannel *model.Channel, member1
 			result.Err = model.NewAppError("SqlChannelStore.SaveDirectChannel", "store.sql_channel.save_direct_channel.open_transaction.app_error", nil, err.Error(), http.StatusInternalServerError)
 			return
 		}
+		defer finalizeTransaction(transaction)
 
 		directchannel.TeamId = ""
 		channelResult := s.saveChannelT(transaction, directchannel, 0)
 
 		if channelResult.Err != nil {
-			transaction.Rollback()
 			result.Err = channelResult.Err
 			result.Data = channelResult.Data
 			return
@@ -527,7 +517,6 @@ func (s SqlChannelStore) SaveDirectChannel(directchannel *model.Channel, member1
 		}
 
 		if member1Result.Err != nil || member2Result.Err != nil {
-			transaction.Rollback()
 			details := ""
 			if member1Result.Err != nil {
 				details += "Member1Err: " + member1Result.Err.Message
@@ -599,16 +588,15 @@ func (s SqlChannelStore) Update(channel *model.Channel) store.StoreChannel {
 			result.Err = model.NewAppError("SqlChannelStore.Update", "store.sql_channel.update.open_transaction.app_error", nil, err.Error(), http.StatusInternalServerError)
 			return
 		}
+		defer finalizeTransaction(transaction)
 
 		*result = s.updateChannelT(transaction, channel)
 		if result.Err != nil {
-			transaction.Rollback()
 			return
 		}
 
 		// Additionally propagate the write to the PublicChannels table.
 		if err := s.upsertPublicChannelT(transaction, result.Data.(*model.Channel)); err != nil {
-			transaction.Rollback()
 			result.Err = model.NewAppError("SqlChannelStore.Update", "store.sql_channel.update.upsert_public_channel.app_error", nil, err.Error(), http.StatusInternalServerError)
 			return
 		}
@@ -786,10 +774,10 @@ func (s SqlChannelStore) SetDeleteAt(channelId string, deleteAt, updateAt int64)
 			result.Err = model.NewAppError("SqlChannelStore.SetDeleteAt", "store.sql_channel.set_delete_at.open_transaction.app_error", nil, err.Error(), http.StatusInternalServerError)
 			return
 		}
+		defer finalizeTransaction(transaction)
 
 		*result = s.setDeleteAtT(transaction, channelId, deleteAt, updateAt)
 		if result.Err != nil {
-			transaction.Rollback()
 			return
 		}
 
@@ -805,7 +793,6 @@ func (s SqlChannelStore) SetDeleteAt(channelId string, deleteAt, updateAt int64)
 			"DeleteAt":  deleteAt,
 			"ChannelId": channelId,
 		}); err != nil {
-			transaction.Rollback()
 			result.Err = model.NewAppError("SqlChannelStore.SetDeleteAt", "store.sql_channel.set_delete_at.update_public_channel.app_error", nil, "channel_id="+channelId+", "+err.Error(), http.StatusInternalServerError)
 			return
 		}
@@ -837,10 +824,10 @@ func (s SqlChannelStore) PermanentDeleteByTeam(teamId string) store.StoreChannel
 			result.Err = model.NewAppError("SqlChannelStore.PermanentDeleteByTeam", "store.sql_channel.permanent_delete_by_team.open_transaction.app_error", nil, err.Error(), http.StatusInternalServerError)
 			return
 		}
+		defer finalizeTransaction(transaction)
 
 		*result = s.permanentDeleteByTeamtT(transaction, teamId)
 		if result.Err != nil {
-			transaction.Rollback()
 			return
 		}
 
@@ -853,7 +840,6 @@ func (s SqlChannelStore) PermanentDeleteByTeam(teamId string) store.StoreChannel
 		`, map[string]interface{}{
 			"TeamId": teamId,
 		}); err != nil {
-			transaction.Rollback()
 			result.Err = model.NewAppError("SqlChannelStore.PermanentDeleteByTeamt", "store.sql_channel.permanent_delete_by_team.delete_public_channels.app_error", nil, "team_id="+teamId+", "+err.Error(), http.StatusInternalServerError)
 			return
 		}
@@ -884,10 +870,10 @@ func (s SqlChannelStore) PermanentDelete(channelId string) store.StoreChannel {
 			result.Err = model.NewAppError("SqlChannelStore.PermanentDelete", "store.sql_channel.permanent_delete.open_transaction.app_error", nil, err.Error(), http.StatusInternalServerError)
 			return
 		}
+		defer finalizeTransaction(transaction)
 
 		*result = s.permanentDeleteT(transaction, channelId)
 		if result.Err != nil {
-			transaction.Rollback()
 			return
 		}
 
@@ -900,7 +886,6 @@ func (s SqlChannelStore) PermanentDelete(channelId string) store.StoreChannel {
 		`, map[string]interface{}{
 			"ChannelId": channelId,
 		}); err != nil {
-			transaction.Rollback()
 			result.Err = model.NewAppError("SqlChannelStore.PermanentDelete", "store.sql_channel.permanent_delete.delete_public_channel.app_error", nil, "channel_id="+channelId+", "+err.Error(), http.StatusInternalServerError)
 			return
 		}
@@ -1316,10 +1301,10 @@ func (s SqlChannelStore) SaveMember(member *model.ChannelMember) store.StoreChan
 			result.Err = model.NewAppError("SqlChannelStore.SaveMember", "store.sql_channel.save_member.open_transaction.app_error", nil, err.Error(), http.StatusInternalServerError)
 			return
 		}
+		defer finalizeTransaction(transaction)
 
 		*result = s.saveMemberT(transaction, member, channel)
 		if result.Err != nil {
-			transaction.Rollback()
 			return
 		}
 
@@ -2319,6 +2304,7 @@ func (s SqlChannelStore) MigrateChannelMembers(fromChannelId string, fromUserId
 			result.Err = model.NewAppError("SqlChannelStore.MigrateChannelMembers", "store.sql_channel.migrate_channel_members.open_transaction.app_error", nil, err.Error(), http.StatusInternalServerError)
 			return
 		}
+		defer finalizeTransaction(transaction)
 
 		var channelMembers []channelMember
 		if _, err := transaction.Select(&channelMembers, "SELECT * from ChannelMembers WHERE (ChannelId, UserId) > (:FromChannelId, :FromUserId) ORDER BY ChannelId, UserId LIMIT 100", map[string]interface{}{"FromChannelId": fromChannelId, "FromUserId": fromUserId}); err != nil {
@@ -2352,10 +2338,6 @@ func (s SqlChannelStore) MigrateChannelMembers(fromChannelId string, fromUserId
 			member.Roles = strings.Join(newRoles, " ")
 
 			if _, err := transaction.Update(&member); err != nil {
-				if err2 := transaction.Rollback(); err2 != nil {
-					result.Err = model.NewAppError("SqlChannelStore.MigrateChannelMembers", "store.sql_channel.migrate_channel_members.rollback_transaction.app_error", nil, err2.Error(), http.StatusInternalServerError)
-					return
-				}
 				result.Err = model.NewAppError("SqlChannelStore.MigrateChannelMembers", "store.sql_channel.migrate_channel_members.update.app_error", nil, err.Error(), http.StatusInternalServerError)
 				return
 			}
@@ -2363,10 +2345,6 @@ func (s SqlChannelStore) MigrateChannelMembers(fromChannelId string, fromUserId
 		}
 
 		if err := transaction.Commit(); err != nil {
-			if err2 := transaction.Rollback(); err2 != nil {
-				result.Err = model.NewAppError("SqlChannelStore.MigrateChannelMembers", "store.sql_channel.migrate_channel_members.rollback_transaction.app_error", nil, err2.Error(), http.StatusInternalServerError)
-				return
-			}
 			result.Err = model.NewAppError("SqlChannelStore.MigrateChannelMembers", "store.sql_channel.migrate_channel_members.commit_transaction.app_error", nil, err.Error(), http.StatusInternalServerError)
 			return
 		}
@@ -2385,10 +2363,10 @@ func (s SqlChannelStore) ResetAllChannelSchemes() store.StoreChannel {
 			result.Err = model.NewAppError("SqlChannelStore.ResetAllChannelSchemes", "store.sql_channel.reset_all_channel_schemes.open_transaction.app_error", nil, err.Error(), http.StatusInternalServerError)
 			return
 		}
+		defer finalizeTransaction(transaction)
 
 		*result = s.resetAllChannelSchemesT(transaction)
 		if result.Err != nil {
-			transaction.Rollback()
 			return
 		}
 
@@ -2427,15 +2405,13 @@ func (s SqlChannelStore) ClearAllCustomRoleAssignments() store.StoreChannel {
 
 			var channelMembers []*channelMember
 			if _, err := transaction.Select(&channelMembers, "SELECT * from ChannelMembers WHERE (ChannelId, UserId) > (:ChannelId, :UserId) ORDER BY ChannelId, UserId LIMIT 1000", map[string]interface{}{"ChannelId": lastChannelId, "UserId": lastUserId}); err != nil {
-				if err2 := transaction.Rollback(); err2 != nil {
+				finalizeTransaction(transaction)
-					result.Err = model.NewAppError("SqlChannelStore.ClearAllCustomRoleAssignments", "store.sql_channel.clear_all_custom_role_assignments.rollback_transaction.app_error", nil, err2.Error(), http.StatusInternalServerError)
-					return
-				}
 				result.Err = model.NewAppError("SqlChannelStore.ClearAllCustomRoleAssignments", "store.sql_channel.clear_all_custom_role_assignments.select.app_error", nil, err.Error(), http.StatusInternalServerError)
 				return
 			}
 
 			if len(channelMembers) == 0 {
+				finalizeTransaction(transaction)
 				break
 			}
 
@@ -2457,10 +2433,7 @@ func (s SqlChannelStore) ClearAllCustomRoleAssignments() store.StoreChannel {
 				newRolesString := strings.Join(newRoles, " ")
 				if newRolesString != member.Roles {
 					if _, err := transaction.Exec("UPDATE ChannelMembers SET Roles = :Roles WHERE UserId = :UserId AND ChannelId = :ChannelId", map[string]interface{}{"Roles": newRolesString, "ChannelId": member.ChannelId, "UserId": member.UserId}); err != nil {
-						if err2 := transaction.Rollback(); err2 != nil {
+						finalizeTransaction(transaction)
-							result.Err = model.NewAppError("SqlChannelStore.ClearAllCustomRoleAssignments", "store.sql_channel.clear_all_custom_role_assignments.rollback_transaction.app_error", nil, err2.Error(), http.StatusInternalServerError)
-							return
-						}
 						result.Err = model.NewAppError("SqlChannelStore.ClearAllCustomRoleAssignments", "store.sql_channel.clear_all_custom_role_assignments.update.app_error", nil, err.Error(), http.StatusInternalServerError)
 						return
 					}
@@ -2468,10 +2441,7 @@ func (s SqlChannelStore) ClearAllCustomRoleAssignments() store.StoreChannel {
 			}
 
 			if err := transaction.Commit(); err != nil {
-				if err2 := transaction.Rollback(); err2 != nil {
+				finalizeTransaction(transaction)
-					result.Err = model.NewAppError("SqlChannelStore.ClearAllCustomRoleAssignments", "store.sql_channel.clear_all_custom_role_assignments.rollback_transaction.app_error", nil, err2.Error(), http.StatusInternalServerError)
-					return
-				}
 				result.Err = model.NewAppError("SqlChannelStore.ClearAllCustomRoleAssignments", "store.sql_channel.clear_all_custom_role_assignments.commit_transaction.app_error", nil, err.Error(), http.StatusInternalServerError)
 				return
 			}
@@ -2486,10 +2456,10 @@ func (s SqlChannelStore) ResetLastPostAt() store.StoreChannel {
 			result.Err = model.NewAppError("SqlChannelStore.ResetLastPostAt", "store.sql_channel.reset_last_post_at.open_transaction.app_error", nil, err.Error(), http.StatusInternalServerError)
 			return
 		}
+		defer finalizeTransaction(transaction)
 
 		*result = s.resetLastPostAtT(transaction)
 		if result.Err != nil {
-			transaction.Rollback()
 			return
 		}
 

store/sqlstore/oauth_store.go

@@ -164,6 +164,7 @@ func (as SqlOAuthStore) DeleteApp(id string) store.StoreChannel {
 		if err != nil {
 			result.Err = model.NewAppError("SqlOAuthStore.DeleteApp", "store.sql_oauth.delete.open_transaction.app_error", nil, err.Error(), http.StatusInternalServerError)
 		} else {
+			defer finalizeTransaction(transaction)
 			if extrasResult := as.deleteApp(transaction, id); extrasResult.Err != nil {
 				*result = extrasResult
 			}
@@ -173,10 +174,6 @@ func (as SqlOAuthStore) DeleteApp(id string) store.StoreChannel {
 					// don't need to rollback here since the transaction is already closed
 					result.Err = model.NewAppError("SqlOAuthStore.DeleteApp", "store.sql_oauth.delete.commit_transaction.app_error", nil, err.Error(), http.StatusInternalServerError)
 				}
-			} else {
-				if err := transaction.Rollback(); err != nil {
-					result.Err = model.NewAppError("SqlOAuthStore.DeleteApp", "store.sql_oauth.delete.rollback_transaction.app_error", nil, err.Error(), http.StatusInternalServerError)
-				}
 			}
 		}
 	})

store/sqlstore/preference_store.go

@@ -61,6 +61,7 @@ func (s SqlPreferenceStore) Save(preferences *model.Preferences) store.StoreChan
 		if err != nil {
 			result.Err = model.NewAppError("SqlPreferenceStore.Save", "store.sql_preference.save.open_transaction.app_error", nil, err.Error(), http.StatusInternalServerError)
 		} else {
+			defer finalizeTransaction(transaction)
 			for _, preference := range *preferences {
 				if upsertResult := s.save(transaction, &preference); upsertResult.Err != nil {
 					*result = upsertResult
@@ -75,10 +76,6 @@ func (s SqlPreferenceStore) Save(preferences *model.Preferences) store.StoreChan
 				} else {
 					result.Data = len(*preferences)
 				}
-			} else {
-				if err := transaction.Rollback(); err != nil {
-					result.Err = model.NewAppError("SqlPreferenceStore.Save", "store.sql_preference.save.rollback_transaction.app_error", nil, err.Error(), http.StatusInternalServerError)
-				}
 			}
 		}
 	})

store/sqlstore/role_supplier.go

@@ -11,7 +11,6 @@ import (
 	"strings"
 
 	"github.com/mattermost/gorp"
-
 	"github.com/mattermost/mattermost-server/model"
 	"github.com/mattermost/mattermost-server/store"
 )
@@ -94,6 +93,7 @@ func (s *SqlSupplier) RoleSave(ctx context.Context, role *model.Role, hints ...s
 			result.Err = model.NewAppError("SqlRoleStore.RoleSave", "store.sql_role.save.open_transaction.app_error", nil, err.Error(), http.StatusInternalServerError)
 			return result
 		} else {
+			defer finalizeTransaction(transaction)
 			result = s.createRole(ctx, role, transaction, hints...)
 
 			if result.Err != nil {

store/sqlstore/scheme_supplier.go

@@ -38,12 +38,13 @@ func (s *SqlSupplier) SchemeSave(ctx context.Context, scheme *model.Scheme, hint
 		if transaction, err := s.GetMaster().Begin(); err != nil {
 			result.Err = model.NewAppError("SqlSchemeStore.SaveScheme", "store.sql_scheme.save.open_transaction.app_error", nil, err.Error(), http.StatusInternalServerError)
 		} else {
+			defer finalizeTransaction(transaction)
 			result = s.createScheme(ctx, scheme, transaction, hints...)
 
-			if result.Err != nil {
+			if result.Err == nil {
-				transaction.Rollback()
+				if err := transaction.Commit(); err != nil {
-			} else if err := transaction.Commit(); err != nil {
+					result.Err = model.NewAppError("SqlSchemeStore.SchemeSave", "store.sql_scheme.save_scheme.commit_transaction.app_error", nil, err.Error(), http.StatusInternalServerError)
-				result.Err = model.NewAppError("SqlSchemeStore.SchemeSave", "store.sql_scheme.save_scheme.commit_transaction.app_error", nil, err.Error(), http.StatusInternalServerError)
+				}
 			}
 		}
 	} else {

store/sqlstore/supplier_reactions.go

@@ -34,18 +34,16 @@ func (s *SqlSupplier) ReactionSave(ctx context.Context, reaction *model.Reaction
 	if transaction, err := s.GetMaster().Begin(); err != nil {
 		result.Err = model.NewAppError("SqlReactionStore.Save", "store.sql_reaction.save.begin.app_error", nil, err.Error(), http.StatusInternalServerError)
 	} else {
+		defer finalizeTransaction(transaction)
 		err := saveReactionAndUpdatePost(transaction, reaction)
 
 		if err != nil {
-			transaction.Rollback()
-
 			// We don't consider duplicated save calls as an error
 			if !IsUniqueConstraintError(err, []string{"reactions_pkey", "PRIMARY"}) {
 				result.Err = model.NewAppError("SqlPreferenceStore.Save", "store.sql_reaction.save.save.app_error", nil, err.Error(), http.StatusBadRequest)
 			}
 		} else {
 			if err := transaction.Commit(); err != nil {
-				// don't need to rollback here since the transaction is already closed
 				result.Err = model.NewAppError("SqlPreferenceStore.Save", "store.sql_reaction.save.commit.app_error", nil, err.Error(), http.StatusInternalServerError)
 			}
 		}
@@ -64,14 +62,12 @@ func (s *SqlSupplier) ReactionDelete(ctx context.Context, reaction *model.Reacti
 	if transaction, err := s.GetMaster().Begin(); err != nil {
 		result.Err = model.NewAppError("SqlReactionStore.Delete", "store.sql_reaction.delete.begin.app_error", nil, err.Error(), http.StatusInternalServerError)
 	} else {
+		defer finalizeTransaction(transaction)
 		err := deleteReactionAndUpdatePost(transaction, reaction)
 
 		if err != nil {
-			transaction.Rollback()
-
 			result.Err = model.NewAppError("SqlPreferenceStore.Delete", "store.sql_reaction.delete.app_error", nil, err.Error(), http.StatusInternalServerError)
 		} else if err := transaction.Commit(); err != nil {
-			// don't need to rollback here since the transaction is already closed
 			result.Err = model.NewAppError("SqlPreferenceStore.Delete", "store.sql_reaction.delete.commit.app_error", nil, err.Error(), http.StatusInternalServerError)
 		} else {
 			result.Data = reaction

store/sqlstore/team_store.go

@@ -792,6 +792,7 @@ func (s SqlTeamStore) MigrateTeamMembers(fromTeamId string, fromUserId string) s
 			result.Err = model.NewAppError("SqlTeamStore.MigrateTeamMembers", "store.sql_team.migrate_team_members.open_transaction.app_error", nil, err.Error(), http.StatusInternalServerError)
 			return
 		}
+		defer finalizeTransaction(transaction)
 
 		var teamMembers []teamMember
 		if _, err := transaction.Select(&teamMembers, "SELECT * from TeamMembers WHERE (TeamId, UserId) > (:FromTeamId, :FromUserId) ORDER BY TeamId, UserId LIMIT 100", map[string]interface{}{"FromTeamId": fromTeamId, "FromUserId": fromUserId}); err != nil {
@@ -825,10 +826,6 @@ func (s SqlTeamStore) MigrateTeamMembers(fromTeamId string, fromUserId string) s
 			member.Roles = strings.Join(newRoles, " ")
 
 			if _, err := transaction.Update(&member); err != nil {
-				if err2 := transaction.Rollback(); err2 != nil {
-					result.Err = model.NewAppError("SqlTeamStore.MigrateTeamMembers", "store.sql_team.migrate_team_members.rollback_transaction.app_error", nil, err2.Error(), http.StatusInternalServerError)
-					return
-				}
 				result.Err = model.NewAppError("SqlTeamStore.MigrateTeamMembers", "store.sql_team.migrate_team_members.update.app_error", nil, err.Error(), http.StatusInternalServerError)
 				return
 			}
@@ -836,10 +833,6 @@ func (s SqlTeamStore) MigrateTeamMembers(fromTeamId string, fromUserId string) s
 		}
 
 		if err := transaction.Commit(); err != nil {
-			if err2 := transaction.Rollback(); err2 != nil {
-				result.Err = model.NewAppError("SqlTeamStore.MigrateTeamMembers", "store.sql_team.migrate_team_members.rollback_transaction.app_error", nil, err2.Error(), http.StatusInternalServerError)
-				return
-			}
 			result.Err = model.NewAppError("SqlTeamStore.MigrateTeamMembers", "store.sql_team.migrate_team_members.commit_transaction.app_error", nil, err.Error(), http.StatusInternalServerError)
 			return
 		}
@@ -873,13 +866,10 @@ func (s SqlTeamStore) ClearAllCustomRoleAssignments() store.StoreChannel {
 				result.Err = model.NewAppError("SqlTeamStore.ClearAllCustomRoleAssignments", "store.sql_team.clear_all_custom_role_assignments.open_transaction.app_error", nil, err.Error(), http.StatusInternalServerError)
 				return
 			}
+			defer finalizeTransaction(transaction)
 
 			var teamMembers []*teamMember
 			if _, err := transaction.Select(&teamMembers, "SELECT * from TeamMembers WHERE (TeamId, UserId) > (:TeamId, :UserId) ORDER BY TeamId, UserId LIMIT 1000", map[string]interface{}{"TeamId": lastTeamId, "UserId": lastUserId}); err != nil {
-				if err2 := transaction.Rollback(); err2 != nil {
-					result.Err = model.NewAppError("SqlTeamStore.ClearAllCustomRoleAssignments", "store.sql_team.clear_all_custom_role_assignments.rollback_transaction.app_error", nil, err2.Error(), http.StatusInternalServerError)
-					return
-				}
 				result.Err = model.NewAppError("SqlTeamStore.ClearAllCustomRoleAssignments", "store.sql_team.clear_all_custom_role_assignments.select.app_error", nil, err.Error(), http.StatusInternalServerError)
 				return
 			}
@@ -906,10 +896,6 @@ func (s SqlTeamStore) ClearAllCustomRoleAssignments() store.StoreChannel {
 				newRolesString := strings.Join(newRoles, " ")
 				if newRolesString != member.Roles {
 					if _, err := transaction.Exec("UPDATE TeamMembers SET Roles = :Roles WHERE UserId = :UserId AND TeamId = :TeamId", map[string]interface{}{"Roles": newRolesString, "TeamId": member.TeamId, "UserId": member.UserId}); err != nil {
-						if err2 := transaction.Rollback(); err2 != nil {
-							result.Err = model.NewAppError("SqlTeamStore.ClearAllCustomRoleAssignments", "store.sql_team.clear_all_custom_role_assignments.rollback_transaction.app_error", nil, err2.Error(), http.StatusInternalServerError)
-							return
-						}
 						result.Err = model.NewAppError("SqlTeamStore.ClearAllCustomRoleAssignments", "store.sql_team.clear_all_custom_role_assignments.update.app_error", nil, err.Error(), http.StatusInternalServerError)
 						return
 					}
@@ -917,10 +903,6 @@ func (s SqlTeamStore) ClearAllCustomRoleAssignments() store.StoreChannel {
 			}
 
 			if err := transaction.Commit(); err != nil {
-				if err2 := transaction.Rollback(); err2 != nil {
-					result.Err = model.NewAppError("SqlTeamStore.ClearAllCustomRoleAssignments", "store.sql_team.clear_all_custom_role_assignments.rollback_transaction.app_error", nil, err2.Error(), http.StatusInternalServerError)
-					return
-				}
 				result.Err = model.NewAppError("SqlTeamStore.ClearAllCustomRoleAssignments", "store.sql_team.clear_all_custom_role_assignments.commit_transaction.app_error", nil, err.Error(), http.StatusInternalServerError)
 				return
 			}

store/sqlstore/upgrade.go

@@ -173,15 +173,18 @@ func UpgradeDatabaseToVersion33(sqlStore SqlStore) {
 			if err != nil {
 				themeMigrationFailed(err)
 			}
+			defer finalizeTransaction(transaction)
 
 			// increase size of Value column of Preferences table to match the size of the ThemeProps column
 			if sqlStore.DriverName() == model.DATABASE_DRIVER_POSTGRES {
 				if _, err := transaction.Exec("ALTER TABLE Preferences ALTER COLUMN Value TYPE varchar(2000)"); err != nil {
 					themeMigrationFailed(err)
+					return
 				}
 			} else if sqlStore.DriverName() == model.DATABASE_DRIVER_MYSQL {
 				if _, err := transaction.Exec("ALTER TABLE Preferences MODIFY Value text"); err != nil {
 					themeMigrationFailed(err)
+					return
 				}
 			}
 
@@ -196,15 +199,18 @@ func UpgradeDatabaseToVersion33(sqlStore SqlStore) {
 				WHERE
 					Users.ThemeProps != 'null'`, params); err != nil {
 				themeMigrationFailed(err)
+				return
 			}
 
 			// delete old data
 			if _, err := transaction.Exec("ALTER TABLE Users DROP COLUMN ThemeProps"); err != nil {
 				themeMigrationFailed(err)
+				return
 			}
 
 			if err := transaction.Commit(); err != nil {
 				themeMigrationFailed(err)
+				return
 			}
 
 			// rename solarized_* code themes to solarized-* to match client changes in 3.0

store/sqlstore/user_access_token_store.go

@@ -57,6 +57,7 @@ func (s SqlUserAccessTokenStore) Delete(tokenId string) store.StoreChannel {
 		if err != nil {
 			result.Err = model.NewAppError("SqlUserAccessTokenStore.Delete", "store.sql_user_access_token.delete.app_error", nil, err.Error(), http.StatusInternalServerError)
 		} else {
+			defer finalizeTransaction(transaction)
 			if extrasResult := s.deleteSessionsAndTokensById(transaction, tokenId); extrasResult.Err != nil {
 				*result = extrasResult
 			}
@@ -66,10 +67,6 @@ func (s SqlUserAccessTokenStore) Delete(tokenId string) store.StoreChannel {
 					// don't need to rollback here since the transaction is already closed
 					result.Err = model.NewAppError("SqlUserAccessTokenStore.Delete", "store.sql_user_access_token.delete.app_error", nil, err.Error(), http.StatusInternalServerError)
 				}
-			} else {
-				if err := transaction.Rollback(); err != nil {
-					result.Err = model.NewAppError("SqlUserAccessTokenStore.Delete", "store.sql_user_access_token.delete.app_error", nil, err.Error(), http.StatusInternalServerError)
-				}
 			}
 		}
 	})
@@ -109,6 +106,7 @@ func (s SqlUserAccessTokenStore) DeleteAllForUser(userId string) store.StoreChan
 		if err != nil {
 			result.Err = model.NewAppError("SqlUserAccessTokenStore.DeleteAllForUser", "store.sql_user_access_token.delete.app_error", nil, err.Error(), http.StatusInternalServerError)
 		} else {
+			defer finalizeTransaction(transaction)
 			if extrasResult := s.deleteSessionsandTokensByUser(transaction, userId); extrasResult.Err != nil {
 				*result = extrasResult
 			}
@@ -118,10 +116,6 @@ func (s SqlUserAccessTokenStore) DeleteAllForUser(userId string) store.StoreChan
 					// don't need to rollback here since the transaction is already closed
 					result.Err = model.NewAppError("SqlUserAccessTokenStore.DeleteAllForUser", "store.sql_user_access_token.delete.app_error", nil, err.Error(), http.StatusInternalServerError)
 				}
-			} else {
-				if err := transaction.Rollback(); err != nil {
-					result.Err = model.NewAppError("SqlUserAccessTokenStore.DeleteAllForUser", "store.sql_user_access_token.delete.app_error", nil, err.Error(), http.StatusInternalServerError)
-				}
 			}
 		}
 	})
@@ -247,6 +241,7 @@ func (s SqlUserAccessTokenStore) UpdateTokenDisable(tokenId string) store.StoreC
 		if err != nil {
 			result.Err = model.NewAppError("SqlUserAccessTokenStore.UpdateTokenDisable", "store.sql_user_access_token.update_token_disable.app_error", nil, err.Error(), http.StatusInternalServerError)
 		} else {
+			defer finalizeTransaction(transaction)
 			if extrasResult := s.deleteSessionsAndDisableToken(transaction, tokenId); extrasResult.Err != nil {
 				*result = extrasResult
 			}
@@ -256,10 +251,6 @@ func (s SqlUserAccessTokenStore) UpdateTokenDisable(tokenId string) store.StoreC
 					// don't need to rollback here since the transaction is already closed
 					result.Err = model.NewAppError("SqlUserAccessTokenStore.UpdateTokenDisable", "store.sql_user_access_token.update_token_disable.app_error", nil, err.Error(), http.StatusInternalServerError)
 				}
-			} else {
-				if err := transaction.Rollback(); err != nil {
-					result.Err = model.NewAppError("SqlUserAccessTokenStore.UpdateTokenDisable", "store.sql_user_access_token.update_token_disable.app_error", nil, err.Error(), http.StatusInternalServerError)
-				}
 			}
 		}
 	})

store/sqlstore/user_store.go

@@ -1382,13 +1382,10 @@ func (us SqlUserStore) ClearAllCustomRoleAssignments() store.StoreChannel {
 				result.Err = model.NewAppError("SqlUserStore.ClearAllCustomRoleAssignments", "store.sql_user.clear_all_custom_role_assignments.open_transaction.app_error", nil, err.Error(), http.StatusInternalServerError)
 				return
 			}
+			defer finalizeTransaction(transaction)
 
 			var users []*model.User
 			if _, err := transaction.Select(&users, "SELECT * from Users WHERE Id > :Id ORDER BY Id LIMIT 1000", map[string]interface{}{"Id": lastUserId}); err != nil {
-				if err2 := transaction.Rollback(); err2 != nil {
-					result.Err = model.NewAppError("SqlUserStore.ClearAllCustomRoleAssignments", "store.sql_user.clear_all_custom_role_assignments.rollback_transaction.app_error", nil, err2.Error(), http.StatusInternalServerError)
-					return
-				}
 				result.Err = model.NewAppError("SqlUserStore.ClearAllCustomRoleAssignments", "store.sql_user.clear_all_custom_role_assignments.select.app_error", nil, err.Error(), http.StatusInternalServerError)
 				return
 			}
@@ -1414,10 +1411,6 @@ func (us SqlUserStore) ClearAllCustomRoleAssignments() store.StoreChannel {
 				newRolesString := strings.Join(newRoles, " ")
 				if newRolesString != user.Roles {
 					if _, err := transaction.Exec("UPDATE Users SET Roles = :Roles WHERE Id = :Id", map[string]interface{}{"Roles": newRolesString, "Id": user.Id}); err != nil {
-						if err2 := transaction.Rollback(); err2 != nil {
-							result.Err = model.NewAppError("SqlUserStore.ClearAllCustomRoleAssignments", "store.sql_user.clear_all_custom_role_assignments.rollback_transaction.app_error", nil, err2.Error(), http.StatusInternalServerError)
-							return
-						}
 						result.Err = model.NewAppError("SqlUserStore.ClearAllCustomRoleAssignments", "store.sql_user.clear_all_custom_role_assignments.update.app_error", nil, err.Error(), http.StatusInternalServerError)
 						return
 					}
@@ -1425,10 +1418,6 @@ func (us SqlUserStore) ClearAllCustomRoleAssignments() store.StoreChannel {
 			}
 
 			if err := transaction.Commit(); err != nil {
-				if err2 := transaction.Rollback(); err2 != nil {
-					result.Err = model.NewAppError("SqlUserStore.ClearAllCustomRoleAssignments", "store.sql_user.clear_all_custom_role_assignments.rollback_transaction.app_error", nil, err2.Error(), http.StatusInternalServerError)
-					return
-				}
 				result.Err = model.NewAppError("SqlUserStore.ClearAllCustomRoleAssignments", "store.sql_user.clear_all_custom_role_assignments.commit_transaction.app_error", nil, err.Error(), http.StatusInternalServerError)
 				return
 			}

store/sqlstore/utils.go

@@ -5,8 +5,12 @@ package sqlstore
 
 import (
 	"bytes"
+	"database/sql"
 	"fmt"
 	"strconv"
+
+	"github.com/mattermost/gorp"
+	"github.com/mattermost/mattermost-server/mlog"
 )
 
 // Converts a list of strings into a list of query parameters and a named parameter map that can
@@ -26,3 +30,11 @@ func MapStringsToQueryParams(list []string, paramPrefix string) (string, map[str
 
 	return fmt.Sprintf("(%v)", keys.String()), params
 }
+
+// finalizeTransaction ensures a transaction is closed after use, rolling back if not already committed.
+func finalizeTransaction(transaction *gorp.Transaction) {
+	// Rollback returns sql.ErrTxDone if the transaction was already closed.
+	if err := transaction.Rollback(); err != nil && err != sql.ErrTxDone {
+		mlog.Error("Failed to rollback transaction", mlog.Err(err))
+	}
+}