Fix concurrent map iteration and write panic in AppRefTracker

pkg/reftracker/ref_tracker.go

@@ -28,7 +28,10 @@ func (a *AppRefTracker) AppsForRef(refKey RefKey) (map[RefKey]struct{}, error) {
 		return nil, fmt.Errorf("could not find ref %s", refKey.Description())
 	}
 
-	return apps, nil
+	// Return a copy so callers can iterate the result after the lock is
+	// released without racing map writers, which would otherwise crash with
+	// "concurrent map iteration and map write".
+	return copyRefKeySet(apps), nil
 }
 
 func (a *AppRefTracker) RefsForApp(appKey RefKey) (map[RefKey]struct{}, error) {
@@ -39,7 +42,18 @@ func (a *AppRefTracker) RefsForApp(appKey RefKey) (map[RefKey]struct{}, error) {
 		return nil, fmt.Errorf("could not find refs for App %s", appKey.RefName())
 	}
 
-	return a.appsToRefs[appKey], nil
+	// Return a copy for the same reason as AppsForRef above.
+	return copyRefKeySet(a.appsToRefs[appKey]), nil
+}
+
+// copyRefKeySet returns a shallow copy of a RefKey set so callers can safely
+// iterate it without holding the tracker lock.
+func copyRefKeySet(in map[RefKey]struct{}) map[RefKey]struct{} {
+	out := make(map[RefKey]struct{}, len(in))
+	for k := range in {
+		out[k] = struct{}{}
+	}
+	return out
 }
 
 func (a *AppRefTracker) RemoveRef(refKey RefKey) {

pkg/reftracker/ref_tracker_race_test.go

@@ -0,0 +1,141 @@
+// Copyright 2024 The Carvel Authors.
+// SPDX-License-Identifier: Apache-2.0
+
+package reftracker_test
+
+import (
+	"fmt"
+	"sync"
+	"testing"
+	"time"
+
+	"carvel.dev/kapp-controller/pkg/reftracker"
+)
+
+// Test_AppsForRef_ConcurrentIterationAndWrite_DoesNotPanic reproduces the race
+// reported in #1812/#1843: AppsForRef returned the internal map by reference, so
+// a caller iterating the result (e.g. SecretHandler.enqueueAppsForUpdate) could
+// run concurrently with another goroutine mutating the same map via
+// ReconcileRefs/RemoveAppFromAllRefs, causing a
+// "fatal error: concurrent map iteration and map write" crash.
+//
+// Before the fix, which makes AppsForRef return a copy, `go test -race` reports
+// the race on every run, while a plain run only sometimes crashes.
+func Test_AppsForRef_ConcurrentIterationAndWrite_DoesNotPanic(t *testing.T) {
+	appRefTracker := reftracker.NewAppRefTracker()
+	appKey := reftracker.NewAppKey("app", "default")
+
+	seedRefs := map[reftracker.RefKey]struct{}{}
+	for i := 0; i < 50; i++ {
+		seedRefs[reftracker.NewSecretKey(fmt.Sprintf("secret-%d", i), "default")] = struct{}{}
+	}
+	appRefTracker.ReconcileRefs(seedRefs, appKey)
+
+	var wg sync.WaitGroup
+	stop := make(chan struct{})
+	iterated := 0
+
+	// Writer: add and remove a second App, so each ref's app set keeps being
+	// written to but never becomes empty for the reader.
+	otherAppKey := reftracker.NewAppKey("other-app", "default")
+	wg.Add(1)
+	go func() {
+		defer wg.Done()
+		for {
+			select {
+			case <-stop:
+				return
+			default:
+			}
+			appRefTracker.ReconcileRefs(seedRefs, otherAppKey)
+			appRefTracker.RemoveAppFromAllRefs(otherAppKey)
+		}
+	}()
+
+	// Reader: read a ref's app set and iterate it outside the lock, mimicking
+	// SecretHandler.enqueueAppsForUpdate.
+	wg.Add(1)
+	go func() {
+		defer wg.Done()
+		refKey := reftracker.NewSecretKey("secret-0", "default")
+		for {
+			select {
+			case <-stop:
+				return
+			default:
+			}
+			apps, err := appRefTracker.AppsForRef(refKey)
+			if err != nil {
+				continue
+			}
+			for range apps {
+				iterated++
+			}
+		}
+	}()
+
+	time.Sleep(200 * time.Millisecond)
+	close(stop)
+	wg.Wait()
+
+	if iterated == 0 {
+		t.Fatal("the reader never iterated a result, so the race was not exercised")
+	}
+}
+
+// Test_RefsForApp_ConcurrentIterationAndWrite_DoesNotPanic covers the symmetric
+// path: RefsForApp also returned its internal map by reference.
+func Test_RefsForApp_ConcurrentIterationAndWrite_DoesNotPanic(t *testing.T) {
+	appRefTracker := reftracker.NewAppRefTracker()
+	appKey := reftracker.NewAppKey("app", "default")
+
+	seedRefs := map[reftracker.RefKey]struct{}{}
+	for i := 0; i < 50; i++ {
+		seedRefs[reftracker.NewSecretKey(fmt.Sprintf("secret-%d", i), "default")] = struct{}{}
+	}
+	appRefTracker.ReconcileRefs(seedRefs, appKey)
+
+	var wg sync.WaitGroup
+	stop := make(chan struct{})
+	iterated := 0
+
+	wg.Add(1)
+	go func() {
+		defer wg.Done()
+		for {
+			select {
+			case <-stop:
+				return
+			default:
+			}
+			appRefTracker.ReconcileRefs(seedRefs, appKey)
+		}
+	}()
+
+	wg.Add(1)
+	go func() {
+		defer wg.Done()
+		for {
+			select {
+			case <-stop:
+				return
+			default:
+			}
+			refs, err := appRefTracker.RefsForApp(appKey)
+			if err != nil {
+				continue
+			}
+			for range refs {
+				iterated++
+			}
+		}
+	}()
+
+	time.Sleep(200 * time.Millisecond)
+	close(stop)
+	wg.Wait()
+
+	if iterated == 0 {
+		t.Fatal("the reader never iterated a result, so the race was not exercised")
+	}
+}