log: Work around race when replacing gRPC logger

The `log.Configure()` function is expected to be called exactly once,
either in the `main()` function or in the respective subcommand. This is
because we set up global resources, namely our normal loggers as well as
the gRPC logger. Consequentially, when multiple tests call that function
in parallel, we run into a data race that gets detected by Go's race
detector.

While our normal loggers will eventually be converted to be non-global,
the gRPC logger cannot ever be non-global because the gRPC library does
not provide a way to do so. We thus introduce an ugly workaround in the
form of a new variable that asks the `log` package to skip replacing the
gRPC logger, which is only set in our `testhelper` package.

While not pretty, it at least addresses the data race.

2 files changed, 24 insertions, 6 deletions

diff --git a/internal/log/logger.go b/internal/log/logger.go
index ef8604b3f..7aacc409f 100644
--- a/internal/log/logger.go
+++ b/internal/log/logger.go
@@ -50,6 +50,17 @@ var defaultLogger = func() *logrus.Logger {
 	return logger
 }()
 
+// SkipReplacingGlobalLoggers will cause `Configure()` to skip replacing global loggers. This is mostly a hack: command
+// line applications are expected to call `log.Configure()` in their subcommand actions, and that should indeed always
+// replace global loggers, as well. But when running tests, we invoke the subcommand actions multiple times, which is
+// thus re-configuring the logger repeatedly. Because global logger are per definition a global shared resource, the
+// consequence is that we might end up replacing the global loggers while tests are using them, and this race rightfully
+// gets detected by Go's race detector.
+//
+// This variable should thus only be set in the testhelper's setup routines such that we configure the global logger a
+// single time for all of our tests, only.
+var SkipReplacingGlobalLoggers bool
+
 // Config contains logging configuration values
 type Config struct {
 	Dir    string `toml:"dir,omitempty" json:"dir"`
@@ -64,14 +75,16 @@ func Configure(out io.Writer, format string, level string, hooks ...logrus.Hook)
 		return nil, err
 	}
 
-	// We replace the gRPC logger with a custom one because the default one is too chatty.
-	grpcLogger := logrus.New() //nolint:forbidigo
+	if !SkipReplacingGlobalLoggers {
+		// We replace the gRPC logger with a custom one because the default one is too chatty.
+		grpcLogger := logrus.New() //nolint:forbidigo
 
-	if err := configure(grpcLogger, out, format, mapGRPCLogLevel(level), hooks...); err != nil {
-		return nil, err
-	}
+		if err := configure(grpcLogger, out, format, mapGRPCLogLevel(level), hooks...); err != nil {
+			return nil, err
+		}
 
-	grpcmwlogrus.ReplaceGrpcLogger(grpcLogger.WithField("pid", os.Getpid()))
+		grpcmwlogrus.ReplaceGrpcLogger(grpcLogger.WithField("pid", os.Getpid()))
+	}
 
 	return Default(), nil
 }
diff --git a/internal/testhelper/configure.go b/internal/testhelper/configure.go
index 537485239..7788788a3 100644
--- a/internal/testhelper/configure.go
+++ b/internal/testhelper/configure.go
@@ -86,6 +86,11 @@ func configure() (_ func(), returnedErr error) {
 		return nil, fmt.Errorf("configuring logger failed: %w", err)
 	}
 
+	// From now on we will refrain from replacing the gRPC logger. The gRPC logger is a global shared resource, so
+	// any tests that run in parallel and end up calling `log.Configure()` would potentially try to replace the
+	// logger while it is in use.
+	log.SkipReplacingGlobalLoggers = true
+
 	for key, value := range map[string]string{
 		// We inject the following two variables, which instruct Git to search its
 		// configuration in non-default locations in the global and system scope. This is