[MM-60307] Fix racy use of session in NewWebConn (#28195)

server/channels/app/platform/session.go

@@ -17,6 +17,11 @@ import (
 func (ps *PlatformService) ReturnSessionToPool(session *model.Session) {
 	if session != nil {
 		session.Id = ""
+		// Once the session is retrieved from the pool, all existing prop fields are cleared.
+		// To avoid a race between clearing the props and accessing it, clear the props maps before returning it to the pool.
+		clear(session.Props)
+		// Also clear the team members slice to avoid a similar race condition.
+		clear(session.TeamMembers)
 		ps.sessionPool.Put(session)
 	}
 }

server/channels/app/platform/web_conn.go

@@ -194,15 +194,6 @@ func (ps *PlatformService) PopulateWebConnConfig(s *model.Session, cfg *WebConnC
 
 // NewWebConn returns a new WebConn instance.
 func (ps *PlatformService) NewWebConn(cfg *WebConnConfig, suite SuiteIFace, runner HookRunner) *WebConn {
-	userID := cfg.Session.UserId
-	session := cfg.Session
-	if cfg.Session.UserId != "" {
-		ps.Go(func() {
-			ps.SetStatusOnline(userID, false)
-			ps.UpdateLastActivityAtIfNeeded(session)
-		})
-	}
-
 	// Disable TCP_NO_DELAY for higher throughput
 	var tcpConn *net.TCPConn
 	switch conn := cfg.WebSocket.UnderlyingConn().(type) {
@@ -254,9 +245,20 @@ func (ps *PlatformService) NewWebConn(cfg *WebConnConfig, suite SuiteIFace, runn
 		remoteAddress:      cfg.RemoteAddress,
 		xForwardedFor:      cfg.XForwardedFor,
 	}
-	wc.Active.Store(cfg.Active)
 
 	wc.SetSession(&cfg.Session)
+	userID := cfg.Session.UserId
+	if userID != "" {
+		// UpdateLastActivityAtIfNeeded might block if the Hub is busy.
+		// Create a goroutine to avoid blocking the creation of the websocket connection.
+		ps.Go(func() {
+			ps.SetStatusOnline(userID, false)
+			ps.UpdateLastActivityAtIfNeeded(*wc.GetSession())
+		})
+	}
+
+	wc.Active.Store(cfg.Active)
+
 	wc.SetSessionToken(cfg.Session.Token)
 	wc.SetSessionExpiresAt(cfg.Session.ExpiresAt)
 	wc.SetConnectionID(cfg.ConnectionID)
@@ -388,6 +390,8 @@ func (wc *WebConn) GetSession() *model.Session {
 
 // SetSession sets the session of the connection.
 func (wc *WebConn) SetSession(v *model.Session) {
+	// Clone the session first as WebConn takes ownership of the object
+	// and the web.Hub will return it to the [sync.Pool] once the WebConn gets removed.
 	if v != nil {
 		v = v.DeepCopy()
 	}

server/channels/app/platform/web_conn_test.go

@@ -10,6 +10,7 @@ import (
 	"net/http"
 	"net/http/httptest"
 	"testing"
+	"time"
 
 	"github.com/gorilla/websocket"
 	"github.com/stretchr/testify/assert"
@@ -17,6 +18,7 @@ import (
 
 	"github.com/mattermost/mattermost/server/public/model"
 	"github.com/mattermost/mattermost/server/public/plugin"
+	"github.com/mattermost/mattermost/server/public/shared/i18n"
 )
 
 type hookRunner struct {
@@ -241,3 +243,38 @@ func TestWebConnDrainDeadQueue(t *testing.T) {
 		t.Run("Overwritten First", func(t *testing.T) { run(int64(128), deadQueueSize+10) })
 	})
 }
+
+// TestWebConnSessionRace guards against https://mattermost.atlassian.net/browse/MM-60307. It need to be run with the -race flag.
+func TestWebConnSessionRace(t *testing.T) {
+	th := Setup(t).InitBasic()
+	t.Cleanup(th.TearDown)
+
+	s := httptest.NewServer(dummyWebsocketHandler(t))
+	t.Cleanup(s.Close)
+	d := websocket.Dialer{}
+	c, _, err := d.Dial("ws://"+s.Listener.Addr().String()+"/ws", nil)
+	require.NoError(t, err)
+
+	err = th.Service.Start(nil)
+	require.NoError(t, err)
+
+	session, err := th.Service.CreateSession(th.Context, &model.Session{
+		UserId: th.BasicUser.Id,
+	})
+	require.NoError(t, err)
+	// Ensure LastActivityAt needs to get updated in the session store
+	session.LastActivityAt = session.LastActivityAt - model.SessionActivityTimeout - 1
+
+	cfg := &WebConnConfig{
+		WebSocket: c,
+		Session:   *session,
+		TFunc:     i18n.IdentityTfunc(),
+		Locale:    "en",
+	}
+	_ = th.Service.NewWebConn(cfg, th.Suite, &hookRunner{})
+
+	session.AddProp(model.SessionPropPlatform, "chrome")
+
+	// Wait a bit of the race checker to catch any
+	time.Sleep(100 * time.Millisecond)
+}