Abort on critical error during server startup (#8204)

Only a handful of critical errors are present in the codebase.
They all occur during server startup (in `app.StartServer()`).

Currently, when one of these critical error occurs, it is simpled
mentionned in the logs – then the error is discarded, and the app
attempts to continue the execution (and probably fails pretty quickly in
a weird way).

Rather than continuing operations in an unknow state, these errors should
trigger a clean exit.

This commit rewrites critical startup errors to be correctly
propagated, logged, and then terminate the command execution.
Additionnaly, it makes the server return a proper error code to the
shell.

api/apitestlib.go

@@ -105,7 +105,11 @@ func setupTestHelper(enterprise bool) *TestHelper {
 	if testStore != nil {
 		th.App.UpdateConfig(func(cfg *model.Config) { *cfg.ServiceSettings.ListenAddress = ":0" })
 	}
-	th.App.StartServer()
+	serverErr := th.App.StartServer()
+	if serverErr != nil {
+		panic(serverErr)
+	}
+
 	th.App.UpdateConfig(func(cfg *model.Config) { *cfg.ServiceSettings.ListenAddress = prevListenAddress })
 	api4.Init(th.App, th.App.Srv.Router, false)
 	Init(th.App, th.App.Srv.Router)

api4/apitestlib.go

@@ -113,7 +113,11 @@ func setupTestHelper(enterprise bool) *TestHelper {
 	if testStore != nil {
 		th.App.UpdateConfig(func(cfg *model.Config) { *cfg.ServiceSettings.ListenAddress = ":0" })
 	}
-	th.App.StartServer()
+	serverErr := th.App.StartServer()
+	if serverErr != nil {
+		panic(serverErr)
+	}
+
 	th.App.UpdateConfig(func(cfg *model.Config) { *cfg.ServiceSettings.ListenAddress = prevListenAddress })
 	Init(th.App, th.App.Srv.Router, true)
 	wsapi.Init(th.App, th.App.Srv.WebSocketRouter)

app/app_test.go

@@ -51,7 +51,8 @@ func TestAppRace(t *testing.T) {
 		a, err := New()
 		require.NoError(t, err)
 		a.UpdateConfig(func(cfg *model.Config) { *cfg.ServiceSettings.ListenAddress = ":0" })
-		a.StartServer()
+		serverErr := a.StartServer()
+		require.NoError(t, serverErr)
 		a.Shutdown()
 	}
 }

app/apptestlib.go

@@ -96,7 +96,11 @@ func setupTestHelper(enterprise bool) *TestHelper {
 	if testStore != nil {
 		th.App.UpdateConfig(func(cfg *model.Config) { *cfg.ServiceSettings.ListenAddress = ":0" })
 	}
-	th.App.StartServer()
+	serverErr := th.App.StartServer()
+	if serverErr != nil {
+		panic(serverErr)
+	}
+
 	th.App.UpdateConfig(func(cfg *model.Config) { *cfg.ServiceSettings.ListenAddress = prevListenAddress })
 	th.App.Srv.Store.MarkSystemRanUnitTests()
 

app/server.go

@@ -17,6 +17,7 @@ import (
 	l4g "github.com/alecthomas/log4go"
 	"github.com/gorilla/handlers"
 	"github.com/gorilla/mux"
+	"github.com/pkg/errors"
 	"golang.org/x/crypto/acme/autocert"
 
 	"github.com/mattermost/mattermost-server/model"
@@ -116,7 +117,7 @@ func redirectHTTPToHTTPS(w http.ResponseWriter, r *http.Request) {
 	http.Redirect(w, r, url.String(), http.StatusFound)
 }
 
-func (a *App) StartServer() {
+func (a *App) StartServer() error {
 	l4g.Info(utils.T("api.server.start_server.starting.info"))
 
 	var handler http.Handler = &CorsWrapper{a.Config, a.Srv.Router}
@@ -126,8 +127,7 @@ func (a *App) StartServer() {
 
 		rateLimiter, err := NewRateLimiter(&a.Config().RateLimitSettings)
 		if err != nil {
-			l4g.Critical(err.Error())
-			return
+			return err
 		}
 
 		a.Srv.RateLimiter = rateLimiter
@@ -151,8 +151,8 @@ func (a *App) StartServer() {
 
 	listener, err := net.Listen("tcp", addr)
 	if err != nil {
-		l4g.Critical(utils.T("api.server.start_server.starting.critical"), err)
-		return
+		errors.Wrapf(err, utils.T("api.server.start_server.starting.critical"), err)
+		return err
 	}
 	a.Srv.ListenAddr = listener.Addr().(*net.TCPAddr)
 
@@ -214,6 +214,8 @@ func (a *App) StartServer() {
 		}
 		close(a.Srv.didFinishListen)
 	}()
+
+	return nil
 }
 
 type tcpKeepAliveListener struct {

app/server_test.go

@@ -0,0 +1,50 @@
+// Copyright (c) 2017-present Mattermost, Inc. All Rights Reserved.
+// See License.txt for license information.
+
+package app
+
+import (
+	"testing"
+
+	"github.com/mattermost/mattermost-server/model"
+	"github.com/stretchr/testify/require"
+)
+
+func TestStartServerSuccess(t *testing.T) {
+	a, err := New()
+	require.NoError(t, err)
+
+	a.UpdateConfig(func(cfg *model.Config) { *cfg.ServiceSettings.ListenAddress = ":0" })
+	serverErr := a.StartServer()
+	a.Shutdown()
+	require.NoError(t, serverErr)
+}
+
+func TestStartServerRateLimiterCriticalError(t *testing.T) {
+	a, err := New()
+	require.NoError(t, err)
+
+	// Attempt to use Rate Limiter with an invalid config
+	a.UpdateConfig(func(cfg *model.Config) {
+		*cfg.RateLimitSettings.Enable   = true
+		*cfg.RateLimitSettings.MaxBurst = -100
+	})
+
+	serverErr := a.StartServer()
+	a.Shutdown()
+	require.Error(t, serverErr)
+}
+
+func TestStartServerPortUnavailable(t *testing.T) {
+	a, err := New()
+	require.NoError(t, err)
+
+	// Attempt to listen on a system-reserved port
+	a.UpdateConfig(func(cfg *model.Config) {
+		*cfg.ServiceSettings.ListenAddress = ":21"
+	})
+
+	serverErr := a.StartServer()
+	a.Shutdown()
+	require.Error(t, serverErr)
+}

cmd/platform/server.go

@@ -53,7 +53,7 @@ func runServer(configFileLocation string, disableConfigWatch bool) error {
 
 	a, err := app.New(options...)
 	if err != nil {
-		l4g.Error(err.Error())
+		l4g.Critical(err.Error())
 		return err
 	}
 	defer a.Shutdown()
@@ -87,7 +87,12 @@ func runServer(configFileLocation string, disableConfigWatch bool) error {
 		}
 	})
 
-	a.StartServer()
+	serverErr := a.StartServer()
+	if serverErr != nil {
+		l4g.Critical(serverErr.Error())
+		return serverErr
+	}
+
 	api4.Init(a, a.Srv.Router, false)
 	api3 := api.Init(a, a.Srv.Router)
 	wsapi.Init(a, a.Srv.WebSocketRouter)

cmd/platform/test.go

@@ -53,7 +53,11 @@ func webClientTestsCmdF(cmd *cobra.Command, args []string) error {
 	defer a.Shutdown()
 
 	utils.InitTranslations(a.Config().LocalizationSettings)
-	a.StartServer()
+	serverErr := a.StartServer()
+	if serverErr != nil {
+		return serverErr
+	}
+
 	api4.Init(a, a.Srv.Router, false)
 	api.Init(a, a.Srv.Router)
 	wsapi.Init(a, a.Srv.WebSocketRouter)
@@ -71,7 +75,11 @@ func serverForWebClientTestsCmdF(cmd *cobra.Command, args []string) error {
 	defer a.Shutdown()
 
 	utils.InitTranslations(a.Config().LocalizationSettings)
-	a.StartServer()
+	serverErr := a.StartServer()
+	if serverErr != nil {
+		return serverErr
+	}
+
 	api4.Init(a, a.Srv.Router, false)
 	api.Init(a, a.Srv.Router)
 	wsapi.Init(a, a.Srv.WebSocketRouter)

web/web_test.go

@@ -44,7 +44,10 @@ func Setup() *app.App {
 	}
 	prevListenAddress := *a.Config().ServiceSettings.ListenAddress
 	a.UpdateConfig(func(cfg *model.Config) { *cfg.ServiceSettings.ListenAddress = ":0" })
-	a.StartServer()
+	serverErr := a.StartServer()
+	if serverErr != nil {
+		panic(serverErr)
+	}
 	a.UpdateConfig(func(cfg *model.Config) { *cfg.ServiceSettings.ListenAddress = prevListenAddress })
 	api4.Init(a, a.Srv.Router, false)
 	api3 := api.Init(a, a.Srv.Router)