Alerting: Fix data races and improve testing (#81994)

* Alerting: fix race condition in (*ngalert/sender.ExternalAlertmanager).Run

* Chore: Fix data races when accessing members of *ngalert/state.FakeInstanceStore

* Chore: Fix data races in tests in ngalert/schedule and enable some parallel tests

* Chore: fix linters

* Chore: add TODO comment to remove loopvar once we move to Go 1.22

pkg/services/ngalert/state/manager_test.go

@@ -1380,7 +1380,7 @@ func TestProcessEvalResults(t *testing.T) {
 		require.NotEmpty(t, states)
 
 		savedStates := make(map[string]models.AlertInstance)
-		for _, op := range instanceStore.RecordedOps {
+		for _, op := range instanceStore.RecordedOps() {
 			switch q := op.(type) {
 			case models.AlertInstance:
 				cacheId, err := q.Labels.StringKey()
@@ -1669,7 +1669,7 @@ func TestStaleResults(t *testing.T) {
 	})
 
 	t.Run("should delete stale states from the database", func(t *testing.T) {
-		for _, op := range store.RecordedOps {
+		for _, op := range store.RecordedOps() {
 			switch q := op.(type) {
 			case state.FakeInstanceStoreOp:
 				keys, ok := q.Args[1].([]models.AlertInstanceKey)

pkg/services/ngalert/state/persister_async_test.go

@@ -42,7 +42,7 @@ func TestAsyncStatePersister_Async(t *testing.T) {
 
 		// Check if the state was saved
 		require.Eventually(t, func() bool {
-			return len(store.RecordedOps) == 1
+			return len(store.RecordedOps()) == 1
 		}, time.Second*5, time.Second)
 	})
 	t.Run("It should save on context done", func(t *testing.T) {
@@ -71,7 +71,7 @@ func TestAsyncStatePersister_Async(t *testing.T) {
 
 		// Check if the context cancellation was handled correctly
 		require.Eventually(t, func() bool {
-			return len(store.RecordedOps) == 1
+			return len(store.RecordedOps()) == 1
 		}, time.Second*5, time.Second)
 	})
 }

pkg/services/ngalert/state/persister_sync_test.go

@@ -67,7 +67,7 @@ func TestSyncPersister_saveAlertStates(t *testing.T) {
 		})
 		syncStatePersister.Sync(context.Background(), span, transitions, nil)
 		savedKeys := map[ngmodels.AlertInstanceKey]ngmodels.AlertInstance{}
-		for _, op := range st.RecordedOps {
+		for _, op := range st.RecordedOps() {
 			saved := op.(ngmodels.AlertInstance)
 			savedKeys[saved.AlertInstanceKey] = saved
 		}
@@ -89,7 +89,7 @@ func TestSyncPersister_saveAlertStates(t *testing.T) {
 		syncStatePersister.Sync(context.Background(), span, transitions, nil)
 
 		savedKeys := map[ngmodels.AlertInstanceKey]ngmodels.AlertInstance{}
-		for _, op := range st.RecordedOps {
+		for _, op := range st.RecordedOps() {
 			saved := op.(ngmodels.AlertInstance)
 			savedKeys[saved.AlertInstanceKey] = saved
 		}

pkg/services/ngalert/state/testing.go

@@ -2,6 +2,7 @@ package state
 
 import (
 	"context"
+	"slices"
 	"sync"
 
 	"github.com/grafana/grafana/pkg/services/ngalert/models"
@@ -13,7 +14,7 @@ var _ InstanceStore = &FakeInstanceStore{}
 
 type FakeInstanceStore struct {
 	mtx         sync.Mutex
-	RecordedOps []any
+	recordedOps []any
 }
 
 type FakeInstanceStoreOp struct {
@@ -21,17 +22,23 @@ type FakeInstanceStoreOp struct {
 	Args []any
 }
 
+func (f *FakeInstanceStore) RecordedOps() []any {
+	f.mtx.Lock()
+	defer f.mtx.Unlock()
+	return slices.Clone(f.recordedOps)
+}
+
 func (f *FakeInstanceStore) ListAlertInstances(_ context.Context, q *models.ListAlertInstancesQuery) ([]*models.AlertInstance, error) {
 	f.mtx.Lock()
 	defer f.mtx.Unlock()
-	f.RecordedOps = append(f.RecordedOps, *q)
+	f.recordedOps = append(f.recordedOps, *q)
 	return nil, nil
 }
 
 func (f *FakeInstanceStore) SaveAlertInstance(_ context.Context, q models.AlertInstance) error {
 	f.mtx.Lock()
 	defer f.mtx.Unlock()
-	f.RecordedOps = append(f.RecordedOps, q)
+	f.recordedOps = append(f.recordedOps, q)
 	return nil
 }
 
@@ -40,7 +47,7 @@ func (f *FakeInstanceStore) FetchOrgIds(_ context.Context) ([]int64, error) { re
 func (f *FakeInstanceStore) DeleteAlertInstances(ctx context.Context, q ...models.AlertInstanceKey) error {
 	f.mtx.Lock()
 	defer f.mtx.Unlock()
-	f.RecordedOps = append(f.RecordedOps, FakeInstanceStoreOp{
+	f.recordedOps = append(f.recordedOps, FakeInstanceStoreOp{
 		Name: "DeleteAlertInstances", Args: []any{
 			ctx,
 			q,
@@ -56,9 +63,9 @@ func (f *FakeInstanceStore) DeleteAlertInstancesByRule(ctx context.Context, key
 func (f *FakeInstanceStore) FullSync(ctx context.Context, instances []models.AlertInstance) error {
 	f.mtx.Lock()
 	defer f.mtx.Unlock()
-	f.RecordedOps = []any{}
+	f.recordedOps = []any{}
 	for _, instance := range instances {
-		f.RecordedOps = append(f.RecordedOps, instance)
+		f.recordedOps = append(f.recordedOps, instance)
 	}
 	return nil
 }