fix(tests): race condition creating the sync id (#23481)

Signed-off-by: Alexandre Gaudreault <[email protected]>

Author: agaudreault

controller/sync.go

@@ -7,7 +7,6 @@ import (
 	"os"
 	"strconv"
 	"strings"
-	"sync/atomic"
 	"time"
 
 	"k8s.io/apimachinery/pkg/util/strategicpatch"
@@ -29,6 +28,7 @@ import (
 	"k8s.io/kubectl/pkg/util/openapi"
 
 	"github.com/argoproj/argo-cd/v3/controller/metrics"
+	"github.com/argoproj/argo-cd/v3/controller/syncid"
 	"github.com/argoproj/argo-cd/v3/pkg/apis/application/v1alpha1"
 	listersv1alpha1 "github.com/argoproj/argo-cd/v3/pkg/client/listers/application/v1alpha1"
 	applog "github.com/argoproj/argo-cd/v3/util/app/log"
@@ -38,11 +38,8 @@ import (
 	kubeutil "github.com/argoproj/argo-cd/v3/util/kube"
 	logutils "github.com/argoproj/argo-cd/v3/util/log"
 	"github.com/argoproj/argo-cd/v3/util/lua"
-	"github.com/argoproj/argo-cd/v3/util/rand"
 )
 
-var syncIdPrefix uint64
-
 const (
 	// EnvVarSyncWaveDelay is an environment variable which controls the delay in seconds between
 	// each sync-wave
@@ -96,6 +93,7 @@ func (m *appStateManager) SyncAppState(app *v1alpha1.Application, state *v1alpha
 	// concrete git commit SHA, the SHA is remembered in the status.operationState.syncResult field.
 	// This ensures that when resuming an operation, we sync to the same revision that we initially
 	// started with.
+
 	var revision string
 	var syncOp v1alpha1.SyncOperation
 	var syncRes *v1alpha1.SyncOperationResult
@@ -248,15 +246,12 @@ func (m *appStateManager) SyncAppState(app *v1alpha1.Application, state *v1alpha
 		return
 	}
 
-	atomic.AddUint64(&syncIdPrefix, 1)
-	randSuffix, err := rand.String(5)
+	syncId, err := syncid.Generate()
 	if err != nil {
 		state.Phase = common.OperationError
-		state.Message = fmt.Sprintf("Failed generate random sync ID: %v", err)
+		state.Message = fmt.Sprintf("Failed to generate sync ID: %v", err)
 		return
 	}
-	syncId := fmt.Sprintf("%05d-%s", syncIdPrefix, randSuffix)
-
 	logEntry := log.WithFields(applog.GetAppLogFields(app)).WithField("syncId", syncId)
 	initialResourcesRes := make([]common.ResourceSyncResult, len(syncRes.Resources))
 	for i, res := range syncRes.Resources {

controller/syncid/id.go

@@ -0,0 +1,20 @@
+package syncid
+
+import (
+	"fmt"
+	"sync/atomic"
+
+	"github.com/argoproj/argo-cd/v3/util/rand"
+)
+
+var globalCount = &atomic.Uint64{}
+
+// Generate generates a new ID
+func Generate() (string, error) {
+	randSuffix, err := rand.String(5)
+	if err != nil {
+		return "", fmt.Errorf("failed to generate random suffix: %w", err)
+	}
+	prefix := globalCount.Add(1)
+	return fmt.Sprintf("%05d-%s", prefix, randSuffix), nil
+}

controller/syncid/id_test.go

@@ -0,0 +1,46 @@
+package syncid
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func TestGenerate(t *testing.T) {
+	t.Parallel()
+	const goroutines = 10
+	const idsPerGoroutine = 50
+	idsCh := make(chan string, goroutines*idsPerGoroutine)
+	errCh := make(chan error, goroutines*idsPerGoroutine)
+
+	// Reset globalCount for deterministic test (not strictly necessary, but can help in CI)
+	globalCount.Store(0)
+
+	// Run goroutines in parallel to test for race conditions
+	for g := 0; g < goroutines; g++ {
+		go func() {
+			for i := 0; i < idsPerGoroutine; i++ {
+				id, err := Generate()
+				if err != nil {
+					errCh <- err
+					continue
+				}
+				idsCh <- id
+			}
+		}()
+	}
+
+	ids := make(map[string]any)
+	for i := 0; i < goroutines*idsPerGoroutine; i++ {
+		select {
+		case err := <-errCh:
+			require.NoError(t, err)
+		case id := <-idsCh:
+			assert.Regexp(t, `^\d{5}-[a-zA-Z0-9]{5}$`, id, "ID should match the expected format")
+			_, exists := ids[id]
+			assert.False(t, exists, "ID should be unique")
+			ids[id] = id
+		}
+	}
+}