fix: prevent sql.DB connectionCleaner race and harden flaky tests (#35891)

* fix: prevent sql.DB connectionCleaner race with testify mock diffing

The previous fix (#35881) moved store.Close()/th.Shutdown() before
AssertExpectations, but the race actually occurs earlier — during
mock.Called() when RegisterDBCollector fires inside initConnection.

Root cause: testify's Arguments.Diff() unconditionally calls
fmt.Sprintf("%v", *sql.DB) on every recorded argument (mock.go:976),
which uses reflect to read sql.DB internal fields. The connectionCleaner
goroutine concurrently writes to those same fields, triggering a DATA
RACE under Go 1.25's stricter race detector.

Fix: Set ConnMaxLifetimeMilliseconds=0 and ConnMaxIdleTimeMilliseconds=0
in test SqlSettings before initConnection. This prevents the
connectionCleaner goroutine from starting at all — no concurrent writer
means no race. Connection pool cleanup serves no purpose in tests.

For store_test.go: settings are set directly before SqlStore creation.
For platform tests: settings are set in setupTestHelper's config before
sqlstore.New() is called via platform.New().

* fix: harden flaky TestScheduleOnceSequential and TestGroupStore tests

TestScheduleOnceSequential: Replace fixed 300ms sleep with
require.Eventually polling (5s timeout, 50ms interval). Under the race
detector, execution is significantly slower and 25+ scheduled jobs may
not complete within a fixed 300ms window.

TestGroupStore/GetGroups: Use unique uid-prefixed display names for
test groups to avoid collisions with groups created by other parallel
subtests sharing the same database. Search queries and result checks
updated to use the uid prefix.

* fix: address CodeRabbit review feedback

- Restore mixed-case search coverage in TestGroupStore by using
  mixed-case query strings (uid + "-GrOuP-3") to verify
  case-insensitive matching behavior.
- Strengthen exactly-once validation in TestScheduleOnceSequential
  by checking callback count == 1 instead of > 0, ensuring
  duplicate executions are caught.

---------

Co-authored-by: Hanzo Dev <[email protected]>

server/channels/app/platform/helper_test.go

@@ -147,6 +147,13 @@ func setupTestHelper(dbStore store.Store, dbSettings *model.SqlSettings, enterpr
 
 	memoryConfig := configStore.Get()
 	memoryConfig.SqlSettings = *dbSettings
+	// Disable connection pool cleanup goroutines to prevent DATA RACE with
+	// testify mock argument diffing under the race detector. The sql.DB
+	// connectionCleaner goroutine writes to internal fields while testify's
+	// mock.Called() → Arguments.Diff() → fmt.Sprintf reads them via reflect.
+	// Setting lifetime/idle to 0 prevents the cleaner from starting.
+	memoryConfig.SqlSettings.ConnMaxLifetimeMilliseconds = model.NewPointer(0)
+	memoryConfig.SqlSettings.ConnMaxIdleTimeMilliseconds = model.NewPointer(0)
 	*memoryConfig.PluginSettings.Directory = filepath.Join(tempWorkspace, "plugins")
 	*memoryConfig.PluginSettings.ClientDirectory = filepath.Join(tempWorkspace, "webapp")
 	*memoryConfig.PluginSettings.AutomaticPrepackagedPlugins = false

server/channels/app/platform/service_test.go

@@ -195,17 +195,6 @@ func TestMetrics(t *testing.T) {
 
 		_ = th.CreateUserOrGuest(t, false)
 
-		// Shut down the test helper before asserting mock expectations.
-		// The underlying sql.DB connection pool spawns a background
-		// connectionCleaner goroutine that periodically locks the DB's
-		// internal mutex via atomic.CompareAndSwapInt32. When testify's
-		// AssertExpectations calls reflect to diff the recorded *sql.DB
-		// call arguments, it reads those same internal fields without
-		// synchronization. Under Go 1.25's stricter race detector this
-		// is flagged as a DATA RACE. Shutting down first stops all
-		// background goroutines (store, metrics server, connection
-		// cleaner) so the reflect-based comparison runs without a
-		// concurrent writer.
 		th.Shutdown(t)
 		mockMetricsImpl.AssertExpectations(t)
 	})

server/channels/store/sqlstore/store_test.go

@@ -817,6 +817,15 @@ func TestReplicaLagQuery(t *testing.T) {
 				QueryTimeLag:     model.NewPointer(query),
 			}}
 
+			// Disable connection pool cleanup goroutines to prevent DATA RACE
+			// with testify's reflect-based argument diffing. When sql.DB's
+			// connectionCleaner goroutine is active, it writes to internal
+			// fields while testify's mock.Called() → Arguments.Diff() reads
+			// them via fmt.Sprintf("%v", *sql.DB). Setting lifetime/idle to 0
+			// prevents the cleaner goroutine from starting at all.
+			settings.ConnMaxLifetimeMilliseconds = model.NewPointer(0)
+			settings.ConnMaxIdleTimeMilliseconds = model.NewPointer(0)
+
 			mockMetrics := &mocks.MetricsInterface{}
 			mockMetrics.On("SetReplicaLagAbsolute", tableName, float64(1))
 			mockMetrics.On("SetReplicaLagTime", tableName, float64(1))
@@ -842,15 +851,6 @@ func TestReplicaLagQuery(t *testing.T) {
 			err = store.ReplicaLagTime()
 			require.NoError(t, err)
 
-			// Close the store before asserting mock expectations. The sql.DB
-			// connection pool spawns a background connectionCleaner goroutine
-			// that mutates internal DB fields (e.g. the mutex in sql.DB via
-			// atomic.CompareAndSwapInt32). When testify's AssertExpectations
-			// uses reflect to diff the recorded *sql.DB call arguments, it
-			// reads those same fields without synchronization, triggering a
-			// DATA RACE under Go 1.25's stricter race detector. Closing the
-			// store first shuts down all background goroutines (including the
-			// connection cleaner), eliminating the concurrent writer.
 			store.Close()
 			mockMetrics.AssertExpectations(t)
 		})
@@ -882,6 +882,10 @@ func TestInvalidReplicaLagDataSource(t *testing.T) {
 				QueryTimeLag:     model.NewPointer("SELECT 1"),
 			}}
 
+			// Disable connection pool cleanup goroutines (see TestReplicaLagQuery).
+			settings.ConnMaxLifetimeMilliseconds = model.NewPointer(0)
+			settings.ConnMaxIdleTimeMilliseconds = model.NewPointer(0)
+
 			mockMetrics := &mocks.MetricsInterface{}
 			mockMetrics.On("RegisterDBCollector", mock.AnythingOfType("*sql.DB"), "master")
 
@@ -900,12 +904,6 @@ func TestInvalidReplicaLagDataSource(t *testing.T) {
 			// Verify no replica lag handles were added despite having ReplicaLagSettings
 			assert.Equal(t, 0, len(store.replicaLagHandles))
 
-			// Close the store before the test ends. Same rationale as
-			// TestReplicaLagQuery: the sql.DB connection pool's background
-			// connectionCleaner goroutine races with testify's reflect-based
-			// argument diffing in AssertExpectations (and with the race
-			// detector's tracking of goroutine lifetimes). Explicit close
-			// ensures all background goroutines are stopped before cleanup.
 			store.Close()
 		})
 	}

server/channels/store/storetest/group_store.go

@@ -3627,6 +3627,10 @@ func testGetGroupsByTeam(t *testing.T, rctx request.CTX, ss store.Store) {
 }
 
 func testGetGroups(t *testing.T, rctx request.CTX, ss store.Store) {
+	// Use a unique prefix for display names to avoid collisions with groups
+	// created by other parallel subtests sharing the same database.
+	uid := model.NewId()[:8]
+
 	// Create Team1
 	team1 := &model.Team{
 		DisplayName:      "Team1",
@@ -3657,7 +3661,7 @@ func testGetGroups(t *testing.T, rctx request.CTX, ss store.Store) {
 	// Create Groups 1 and 2
 	group1, err := ss.Group().Create(&model.Group{
 		Name:           model.NewPointer(model.NewId()),
-		DisplayName:    "group-1",
+		DisplayName:    uid + "-group-1",
 		RemoteId:       model.NewPointer(model.NewId()),
 		Source:         model.GroupSourceLdap,
 		AllowReference: true,
@@ -3665,8 +3669,8 @@ func testGetGroups(t *testing.T, rctx request.CTX, ss store.Store) {
 	require.NoError(t, err)
 
 	group2, err := ss.Group().Create(&model.Group{
-		Name:           model.NewPointer(model.NewId() + "-group-2"),
-		DisplayName:    "group-2",
+		Name:           model.NewPointer(model.NewId() + "-" + uid + "-group-2"),
+		DisplayName:    uid + "-group-2",
 		RemoteId:       model.NewPointer(model.NewId()),
 		Source:         model.GroupSourceLdap,
 		AllowReference: false,
@@ -3674,8 +3678,8 @@ func testGetGroups(t *testing.T, rctx request.CTX, ss store.Store) {
 	require.NoError(t, err)
 
 	deletedGroup, err := ss.Group().Create(&model.Group{
-		Name:           model.NewPointer(model.NewId() + "-group-deleted"),
-		DisplayName:    "group-deleted",
+		Name:           model.NewPointer(model.NewId() + "-" + uid + "-group-deleted"),
+		DisplayName:    uid + "-group-deleted",
 		RemoteId:       model.NewPointer(model.NewId()),
 		Source:         model.GroupSourceLdap,
 		AllowReference: false,
@@ -3730,8 +3734,8 @@ func testGetGroups(t *testing.T, rctx request.CTX, ss store.Store) {
 
 	// Create Group3
 	group3, err := ss.Group().Create(&model.Group{
-		Name:           model.NewPointer(model.NewId() + "-group-3"),
-		DisplayName:    "group-3",
+		Name:           model.NewPointer(model.NewId() + "-" + uid + "-group-3"),
+		DisplayName:    uid + "-group-3",
 		RemoteId:       model.NewPointer(model.NewId()),
 		Source:         model.GroupSourceLdap,
 		AllowReference: true,
@@ -3843,7 +3847,7 @@ func testGetGroups(t *testing.T, rctx request.CTX, ss store.Store) {
 	user2.DeleteAt = 1
 	u2Update, _ := ss.User().Update(rctx, user2, true)
 
-	group2NameSubstring := "group-2"
+	group2NameSubstring := uid + "-group-2"
 
 	endCreateTime := u2Update.New.UpdateAt + 1
 
@@ -3927,12 +3931,15 @@ func testGetGroups(t *testing.T, rctx request.CTX, ss store.Store) {
 		},
 		{
 			Name:    "Get group matching display name",
-			Opts:    model.GroupSearchOpts{Q: "rouP-3"},
+			Opts:    model.GroupSearchOpts{Q: uid + "-GrOuP-3"},
 			Page:    0,
 			PerPage: 100,
 			Resultf: func(groups []*model.Group) bool {
+				if len(groups) == 0 {
+					return false
+				}
 				for _, g := range groups {
-					if !strings.Contains(strings.ToLower(g.DisplayName), "roup-3") {
+					if !strings.Contains(strings.ToLower(g.DisplayName), uid+"-group-3") {
 						return false
 					}
 				}
@@ -3942,12 +3949,15 @@ func testGetGroups(t *testing.T, rctx request.CTX, ss store.Store) {
 		},
 		{
 			Name:    "Get group matching multiple display names",
-			Opts:    model.GroupSearchOpts{Q: "groUp"},
+			Opts:    model.GroupSearchOpts{Q: uid + "-GrOuP"},
 			Page:    0,
 			PerPage: 100,
 			Resultf: func(groups []*model.Group) bool {
+				if len(groups) < 2 {
+					return false
+				}
 				for _, g := range groups {
-					if !strings.Contains(strings.ToLower(g.DisplayName), "group") {
+					if !strings.Contains(strings.ToLower(g.DisplayName), uid+"-group") {
 						return false
 					}
 				}
@@ -4193,7 +4203,7 @@ func testGetGroups(t *testing.T, rctx request.CTX, ss store.Store) {
 		},
 		{
 			Name:    "Include archived groups",
-			Opts:    model.GroupSearchOpts{IncludeArchived: true, Q: "group-deleted"},
+			Opts:    model.GroupSearchOpts{IncludeArchived: true, Q: uid + "-group-deleted"},
 			Page:    0,
 			PerPage: 1,
 			Resultf: func(groups []*model.Group) bool {
@@ -4203,7 +4213,7 @@ func testGetGroups(t *testing.T, rctx request.CTX, ss store.Store) {
 		},
 		{
 			Name:    "Only return archived groups",
-			Opts:    model.GroupSearchOpts{FilterArchived: true, Q: "group-1"},
+			Opts:    model.GroupSearchOpts{FilterArchived: true, Q: uid + "-group-1"},
 			Page:    0,
 			PerPage: 1,
 			Resultf: func(groups []*model.Group) bool {

server/public/pluginapi/cluster/job_once_test.go

@@ -343,29 +343,26 @@ func TestScheduleOnceSequential(t *testing.T) {
 		err = s.scheduleNewJobsFromDB()
 		require.NoError(t, err)
 
-		// wait for the testPagingJobs created in the setup to finish
-		time.Sleep(300 * time.Millisecond)
-
-		numInDB := 0
-		numActive := 0
-		numCountsAtZero := 0
-		for k, v := range testPagingJobs {
-			if getVal(oncePrefix+k) != nil {
-				numInDB++
+		// Wait for all paging jobs to complete. Use polling instead of a
+		// fixed sleep because the race detector slows execution significantly,
+		// making a 300ms window insufficient for 25+ scheduled jobs.
+		require.Eventually(t, func() bool {
+			for k, v := range testPagingJobs {
+				if getVal(oncePrefix+k) != nil {
+					return false
+				}
+				s.activeJobs.mu.RLock()
+				active := s.activeJobs.jobs[k] != nil
+				s.activeJobs.mu.RUnlock()
+				if active {
+					return false
+				}
+				if atomic.LoadInt32(v) != int32(1) {
+					return false
+				}
 			}
-			s.activeJobs.mu.RLock()
-			if s.activeJobs.jobs[k] != nil {
-				numActive++
-			}
-			s.activeJobs.mu.RUnlock()
-			if atomic.LoadInt32(v) == int32(0) {
-				numCountsAtZero++
-			}
-		}
-
-		assert.Equal(t, 0, numInDB)
-		assert.Equal(t, 0, numActive)
-		assert.Equal(t, 0, numCountsAtZero)
+			return true
+		}, 5*time.Second, 50*time.Millisecond, "timed out waiting for paging jobs to complete")
 	})
 
 	t.Run("failed at the db", func(t *testing.T) {