diff --git a/pkg/reftracker/ref_tracker.go b/pkg/reftracker/ref_tracker.go index 26a2c0101c..564d7b4e09 100644 --- a/pkg/reftracker/ref_tracker.go +++ b/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) { diff --git a/pkg/reftracker/ref_tracker_race_test.go b/pkg/reftracker/ref_tracker_race_test.go new file mode 100644 index 0000000000..56c2a8f9a2 --- /dev/null +++ b/pkg/reftracker/ref_tracker_race_test.go @@ -0,0 +1,127 @@ +// 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 this test crashes the process; after it (AppsForRef returns a +// copy) it passes. Run with -race for extra signal. +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{}) + + // Writer: continuously mutate the internal maps under the lock. + wg.Add(1) + go func() { + defer wg.Done() + for { + select { + case <-stop: + return + default: + } + appRefTracker.ReconcileRefs(seedRefs, appKey) + appRefTracker.RemoveAppFromAllRefs(appKey) + } + }() + + // 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 { + } + } + }() + + time.Sleep(200 * time.Millisecond) + close(stop) + wg.Wait() +} + +// 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{}) + + 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 { + } + } + }() + + time.Sleep(200 * time.Millisecond) + close(stop) + wg.Wait() +}