Fail vs. fatal on store startup (#24170)

If the store fails to initialize (e.g. run a migration), it would `log.Fatal` and then `os.Exit`. Unfortunately, this trips up `TestMain`, which happily keeps running tests, now guaranteed to fail.

Avoid this by instead returning an error from the store initialization, handling appropriately at the layer above.

server/channels/app/platform/service.go

@@ -204,7 +204,11 @@ func New(sc ServiceConfig, options ...Option) (*PlatformService, error) {
 			// Timer layer
 			// |
 			// Cache layer
-			ps.sqlStore = sqlstore.New(ps.Config().SqlSettings, ps.metricsIFace)
+			var err error
+			ps.sqlStore, err = sqlstore.New(ps.Config().SqlSettings, ps.metricsIFace)
+			if err != nil {
+				return nil, err
+			}
 
 			searchStore := searchlayer.NewSearchLayer(
 				retrylayer.New(ps.sqlStore),

server/channels/app/plugin_api_tests/test_db_driver/main.go

@@ -31,7 +31,10 @@ func (p *MyPlugin) OnConfigurationChange() error {
 func (p *MyPlugin) MessageWillBePosted(_ *plugin.Context, _ *model.Post) (*model.Post, string) {
 	settings := p.API.GetUnsanitizedConfig().SqlSettings
 	settings.Trace = model.NewBool(false)
-	store := sqlstore.New(settings, nil)
+	store, err := sqlstore.New(settings, nil)
+	if err != nil {
+		panic(err)
+	}
 	store.GetMasterX().Close()
 
 	for _, isMaster := range []bool{true, false} {

server/channels/store/localcachelayer/layer_test.go

@@ -12,6 +12,7 @@ import (
 	"github.com/mattermost/mattermost/server/v8/channels/store"
 	"github.com/mattermost/mattermost/server/v8/channels/store/sqlstore"
 	"github.com/mattermost/mattermost/server/v8/channels/store/storetest"
+	"golang.org/x/sync/errgroup"
 )
 
 type storeType struct {
@@ -91,23 +92,29 @@ func initStores() {
 			panic(err)
 		}
 	}()
-	var wg sync.WaitGroup
+	var eg errgroup.Group
 	for _, st := range storeTypes {
 		st := st
-		wg.Add(1)
+		eg.Go(func() error {
-		go func() {
 			var err error
-			defer wg.Done()
+
-			st.SqlStore = sqlstore.New(*st.SqlSettings, nil)
+			st.SqlStore, err = sqlstore.New(*st.SqlSettings, nil)
+			if err != nil {
+				return err
+			}
 			st.Store, err = NewLocalCacheLayer(st.SqlStore, nil, nil, getMockCacheProvider())
 			if err != nil {
-				panic(err)
+				return err
 			}
 			st.Store.DropAllTables()
 			st.Store.MarkSystemRanUnitTests()
-		}()
+
+			return nil
+		})
+	}
+	if err := eg.Wait(); err != nil {
+		panic(err)
 	}
-	wg.Wait()
 }
 
 var tearDownStoresOnce sync.Once

server/channels/store/searchlayer/layer_test.go

@@ -14,6 +14,7 @@ import (
 	"github.com/mattermost/mattermost/server/v8/channels/store/storetest"
 	"github.com/mattermost/mattermost/server/v8/channels/testlib"
 	"github.com/mattermost/mattermost/server/v8/platform/services/searchengine"
+	"github.com/stretchr/testify/require"
 )
 
 // Test to verify race condition on UpdateConfig. The test must run with -race flag in order to verify
@@ -24,7 +25,8 @@ func TestUpdateConfigRace(t *testing.T) {
 		driverName = model.DatabaseDriverPostgres
 	}
 	settings := storetest.MakeSqlSettings(driverName, false)
-	store := sqlstore.New(*settings, nil)
+	store, err := sqlstore.New(*settings, nil)
+	require.NoError(t, err)
 
 	cfg := &model.Config{}
 	cfg.SetDefaults()

server/channels/store/sqlstore/migrate_test.go

@@ -8,6 +8,7 @@ import (
 
 	"github.com/mattermost/mattermost/server/public/model"
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
 func TestUpAndDownMigrations(t *testing.T) {
@@ -23,7 +24,8 @@ func TestUpAndDownMigrations(t *testing.T) {
 				t.Skip(err)
 			}
 
-			store := New(*settings, nil)
+			store, err := New(*settings, nil)
+			require.NoError(t, err)
 			defer store.Close()
 
 			err = store.migrate(migrationsDirectionDown, false)

server/channels/store/sqlstore/store.go

@@ -135,7 +135,7 @@ type SqlStore struct {
 	wgMonitor   *sync.WaitGroup
 }
 
-func New(settings model.SqlSettings, metrics einterfaces.MetricsInterface) *SqlStore {
+func New(settings model.SqlSettings, metrics einterfaces.MetricsInterface) (*SqlStore, error) {
 	store := &SqlStore{
 		rrCounter:   0,
 		srCounter:   0,
@@ -147,7 +147,7 @@ func New(settings model.SqlSettings, metrics einterfaces.MetricsInterface) *SqlS
 
 	err := store.initConnection()
 	if err != nil {
-		mlog.Fatal("Error setting up connections", mlog.Err(err))
+		return nil, errors.Wrap(err, "error setting up connections")
 	}
 
 	store.wgMonitor.Add(1)
@@ -155,32 +155,32 @@ func New(settings model.SqlSettings, metrics einterfaces.MetricsInterface) *SqlS
 
 	ver, err := store.GetDbVersion(true)
 	if err != nil {
-		mlog.Fatal("Error while getting DB version.", mlog.Err(err))
+		return nil, errors.Wrap(err, "error while getting DB version")
 	}
 
 	ok, err := store.ensureMinimumDBVersion(ver)
 	if !ok {
-		mlog.Fatal("Error while checking DB version.", mlog.Err(err))
+		return nil, errors.Wrap(err, "error while checking DB version")
 	}
 
 	err = store.ensureDatabaseCollation()
 	if err != nil {
-		mlog.Fatal("Error while checking DB collation.", mlog.Err(err))
+		return nil, errors.Wrap(err, "error while checking DB collation")
 	}
 
 	err = store.migrate(migrationsDirectionUp, false)
 	if err != nil {
-		mlog.Fatal("Failed to apply database migrations.", mlog.Err(err))
+		return nil, errors.Wrap(err, "failed to apply database migrations")
 	}
 
 	store.isBinaryParam, err = store.computeBinaryParam()
 	if err != nil {
-		mlog.Fatal("Failed to compute binary param", mlog.Err(err))
+		return nil, errors.Wrap(err, "failed to compute binary param")
 	}
 
 	store.pgDefaultTextSearchConfig, err = store.computeDefaultTextSearchConfig()
 	if err != nil {
-		mlog.Fatal("Failed to compute default text search config", mlog.Err(err))
+		return nil, errors.Wrap(err, "failed to compute default text search config")
 	}
 
 	store.stores.team = newSqlTeamStore(store)
@@ -229,7 +229,7 @@ func New(settings model.SqlSettings, metrics einterfaces.MetricsInterface) *SqlS
 
 	store.stores.preference.(*SqlPreferenceStore).deleteUnusedFeatures()
 
-	return store
+	return store, nil
 }
 
 // SetupConnection sets up the connection to the database and pings it to make sure it's alive.

server/channels/store/sqlstore/store_test.go

@@ -20,6 +20,7 @@ import (
 	"github.com/pkg/errors"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
+	"golang.org/x/sync/errgroup"
 
 	"github.com/mattermost/mattermost/server/public/model"
 	"github.com/mattermost/mattermost/server/public/plugin/plugintest/mock"
@@ -126,19 +127,26 @@ func initStores() {
 			panic(err)
 		}
 	}()
-	var wg sync.WaitGroup
+
+	var eg errgroup.Group
 	for _, st := range storeTypes {
 		st := st
-		wg.Add(1)
+		eg.Go(func() error {
-		go func() {
+			var err error
-			defer wg.Done()
+			st.SqlStore, err = New(*st.SqlSettings, nil)
-			st.SqlStore = New(*st.SqlSettings, nil)
+			if err != nil {
+				return err
+			}
 			st.Store = st.SqlStore
 			st.Store.DropAllTables()
 			st.Store.MarkSystemRanUnitTests()
-		}()
+
+			return nil
+		})
+	}
+	if err := eg.Wait(); err != nil {
+		panic(err)
 	}
-	wg.Wait()
 }
 
 var tearDownStoresOnce sync.Once
@@ -175,7 +183,8 @@ func TestStoreLicenseRace(t *testing.T) {
 		t.Skip(err)
 	}
 
-	store := New(*settings, nil)
+	store, err := New(*settings, nil)
+	require.NoError(t, err)
 	defer func() {
 		store.Close()
 		storetest.CleanupSqlSettings(settings)
@@ -275,7 +284,8 @@ func TestGetReplica(t *testing.T) {
 
 			settings.DataSourceReplicas = dataSourceReplicas
 			settings.DataSourceSearchReplicas = dataSourceSearchReplicas
-			store := New(*settings, nil)
+			store, err := New(*settings, nil)
+			require.NoError(t, err)
 			defer func() {
 				store.Close()
 				storetest.CleanupSqlSettings(settings)
@@ -348,7 +358,8 @@ func TestGetReplica(t *testing.T) {
 
 			settings.DataSourceReplicas = dataSourceReplicas
 			settings.DataSourceSearchReplicas = dataSourceSearchReplicas
-			store := New(*settings, nil)
+			store, err := New(*settings, nil)
+			require.NoError(t, err)
 			defer func() {
 				store.Close()
 				storetest.CleanupSqlSettings(settings)
@@ -417,7 +428,8 @@ func TestGetDbVersion(t *testing.T) {
 				t.Skip(err)
 			}
 
-			store := New(*settings, nil)
+			store, err := New(*settings, nil)
+			require.NoError(t, err)
 
 			version, err := store.GetDbVersion(false)
 			require.NoError(t, err)
@@ -641,7 +653,8 @@ func TestGetAllConns(t *testing.T) {
 
 			settings.DataSourceReplicas = dataSourceReplicas
 			settings.DataSourceSearchReplicas = dataSourceSearchReplicas
-			store := New(*settings, nil)
+			store, err := New(*settings, nil)
+			require.NoError(t, err)
 			defer func() {
 				store.Close()
 				storetest.CleanupSqlSettings(settings)
@@ -853,7 +866,8 @@ func TestGetDBSchemaVersion(t *testing.T) {
 			if err != nil {
 				t.Skip(err)
 			}
-			store := New(*settings, nil)
+			store, err := New(*settings, nil)
+			require.NoError(t, err)
 
 			assetsList, err := assets.ReadDir(filepath.Join("migrations", driver))
 			require.NoError(t, err)
@@ -891,7 +905,8 @@ func TestGetAppliedMigrations(t *testing.T) {
 			if err != nil {
 				t.Skip(err)
 			}
-			store := New(*settings, nil)
+			store, err := New(*settings, nil)
+			require.NoError(t, err)
 
 			assetsList, err := assets.ReadDir(filepath.Join("migrations", driver))
 			require.NoError(t, err)

server/channels/testlib/helper.go

@@ -118,7 +118,12 @@ func (h *MainHelper) setupStore(withReadReplica bool) {
 
 	h.SearchEngine = searchengine.NewBroker(config)
 	h.ClusterInterface = &FakeClusterInterface{}
-	h.SQLStore = sqlstore.New(*h.Settings, nil)
+
+	var err error
+	h.SQLStore, err = sqlstore.New(*h.Settings, nil)
+	if err != nil {
+		panic(err)
+	}
 	h.Store = searchlayer.NewSearchLayer(&TestStore{
 		h.SQLStore,
 	}, h.SearchEngine, config)
@@ -130,7 +135,12 @@ func (h *MainHelper) ToggleReplicasOff() {
 	}
 	h.Settings.DataSourceReplicas = []string{}
 	lic := h.SQLStore.GetLicense()
-	h.SQLStore = sqlstore.New(*h.Settings, nil)
+
+	var err error
+	h.SQLStore, err = sqlstore.New(*h.Settings, nil)
+	if err != nil {
+		panic(err)
+	}
 	h.SQLStore.UpdateLicense(lic)
 }
 
@@ -140,7 +150,13 @@ func (h *MainHelper) ToggleReplicasOn() {
 	}
 	h.Settings.DataSourceReplicas = h.replicas
 	lic := h.SQLStore.GetLicense()
-	h.SQLStore = sqlstore.New(*h.Settings, nil)
+
+	var err error
+	h.SQLStore, err = sqlstore.New(*h.Settings, nil)
+	if err != nil {
+		panic(err)
+	}
+
 	h.SQLStore.UpdateLicense(lic)
 }
 

server/cmd/mattermost/commands/db.go

@@ -113,7 +113,10 @@ func initDbCmdF(command *cobra.Command, _ []string) error {
 	}
 	defer configStore.Close()
 
-	sqlStore := sqlstore.New(configStore.Get().SqlSettings, nil)
+	sqlStore, err := sqlstore.New(configStore.Get().SqlSettings, nil)
+	if err != nil {
+		return errors.Wrap(err, "failed to initialize store")
+	}
 	defer sqlStore.Close()
 
 	CommandPrettyPrintln("Database store correctly initialised")

server/cmd/mattermost/commands/init.go

@@ -64,5 +64,5 @@ func initStoreCommandContextCobra(command *cobra.Command) (store.Store, error) {
 	}
 
 	config := cfgStore.Get()
-	return sqlstore.New(config.SqlSettings, nil), nil
+	return sqlstore.New(config.SqlSettings, nil)
 }

server/platform/services/searchengine/bleveengine/bleve_test.go

@@ -49,7 +49,12 @@ func (s *BleveEngineTestSuite) setupStore() {
 		driverName = model.DatabaseDriverPostgres
 	}
 	s.SQLSettings = storetest.MakeSqlSettings(driverName, false)
-	s.SQLStore = sqlstore.New(*s.SQLSettings, nil)
+
+	var err error
+	s.SQLStore, err = sqlstore.New(*s.SQLSettings, nil)
+	if err != nil {
+		s.Require().FailNow("Cannot initialize store: %s", err.Error())
+	}
 
 	cfg := &model.Config{}
 	cfg.SetDefaults()