[MM-69464] Fix data race in pluginapi.ConfigureLogrus (#37183)

* [MM-69464] Fix data race in pluginapi.ConfigureLogrus

ConfigureLogrus registered the hook via logger.Hooks.Add, which mutates
the hook map without holding the logger mutex. logrus copies that map
under the mutex when firing hooks, so configuring the logger while another
goroutine logs through it is a data race that can panic with "concurrent
map read and map write". This is reachable when a plugin re-runs its
activation while goroutines from a prior activation still log through
logrus.StandardLogger() (originally reported as MM-52096).

Use logger.AddHook, which acquires the logger mutex. Also configure the
passed-in logger instead of the global standard logger: SetReportCaller
and SetLevel previously called the package-level logrus functions, silently
mutating logrus.StandardLogger() regardless of the logger argument.

Adds a -race regression test that configures the logger concurrently with
logging.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* Use WaitGroup.Go in the regression test (golangci modernize)

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* Remove obvious comment on SetReportCaller

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

server/public/pluginapi/logrus.go

@@ -60,10 +60,14 @@ func (lh *LogrusHook) Fire(entry *logrus.Entry) error {
 // discarding the default output to avoid duplicating the events across the standard STDOUT proxy.
 func ConfigureLogrus(logger *logrus.Logger, client *Client) {
 	hook := NewLogrusHook(client.Log)
-	logger.Hooks.Add(hook)
+
+	// AddHook (not Hooks.Add) takes the logger mutex, so registering the hook is
+	// safe against goroutines logging through the same logger concurrently.
+	logger.AddHook(hook)
 	logger.SetOutput(io.Discard)
-	logrus.SetReportCaller(true)
+
+	logger.SetReportCaller(true)
 
 	// By default, log everything to the server, and let it decide what gets through.
-	logrus.SetLevel(logrus.TraceLevel)
+	logger.SetLevel(logrus.TraceLevel)
 }

server/public/pluginapi/logrus_test.go

@@ -1,6 +1,7 @@
 package pluginapi_test
 
 import (
+	"sync"
 	"testing"
 
 	"github.com/mattermost/mattermost/server/public/plugin/plugintest"
@@ -83,3 +84,35 @@ func TestLogrus(t *testing.T) {
 		})
 	}
 }
+
+// TestConfigureLogrusConcurrentWithLogging guards against a data race between
+// registering the hook and logging through the same logger. Run with -race.
+func TestConfigureLogrusConcurrentWithLogging(t *testing.T) {
+	api := &plugintest.API{}
+	api.On("LogDebug", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Maybe()
+	client := pluginapi.NewClient(api, &plugintest.Driver{})
+
+	logger := logrus.New()
+
+	var wg sync.WaitGroup
+	start := make(chan struct{})
+
+	for range 4 {
+		wg.Go(func() {
+			<-start
+			for range 500 {
+				logger.WithField("k", "v").Debug("message")
+			}
+		})
+	}
+
+	wg.Go(func() {
+		<-start
+		for range 50 {
+			pluginapi.ConfigureLogrus(logger, client)
+		}
+	})
+
+	close(start)
+	wg.Wait()
+}