From 018af7e6fc624ef77a1c1622dc604e6293f8011c Mon Sep 17 00:00:00 2001 From: Sai Asish Y Date: Mon, 11 May 2026 16:19:11 -0700 Subject: [PATCH 1/2] fix(reftracker): return copy of refs map to avoid concurrent iteration panic AppsForRef and RefsForApp returned the internal map directly, so callers iterated outside the tracker lock while ReconcileRefs and RemoveAppFromAllRefs mutated the same maps under the lock. Under load this triggered fatal 'concurrent map iteration and map write' panics in the controller. Returning a copied set lets callers iterate safely; the map values are unused so the shallow copy is sufficient. Fixes #1812 Signed-off-by: Sai Asish Y --- pkg/reftracker/ref_tracker.go | 19 ++++++++-- pkg/reftracker/ref_tracker_test.go | 59 ++++++++++++++++++++++++++++++ 2 files changed, 75 insertions(+), 3 deletions(-) diff --git a/pkg/reftracker/ref_tracker.go b/pkg/reftracker/ref_tracker.go index 26a2c0101..0558741ed 100644 --- a/pkg/reftracker/ref_tracker.go +++ b/pkg/reftracker/ref_tracker.go @@ -28,18 +28,31 @@ 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 without holding the lock. + // Returning the internal map directly races with mutations from + // ReconcileRefs and RemoveAppFromAllRefs and triggers the Go runtime's + // "concurrent map iteration and map write" fatal error. + return copyRefKeySet(apps), nil } func (a *AppRefTracker) RefsForApp(appKey RefKey) (map[RefKey]struct{}, error) { a.lock.Lock() defer a.lock.Unlock() - if a.appsToRefs[appKey] == nil { + refs := a.appsToRefs[appKey] + if refs == nil { return nil, fmt.Errorf("could not find refs for App %s", appKey.RefName()) } - return a.appsToRefs[appKey], nil + return copyRefKeySet(refs), nil +} + +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_test.go b/pkg/reftracker/ref_tracker_test.go index 9ff9c9b6b..70c5f8ac2 100644 --- a/pkg/reftracker/ref_tracker_test.go +++ b/pkg/reftracker/ref_tracker_test.go @@ -4,6 +4,7 @@ package reftracker_test import ( + "sync" "testing" "carvel.dev/kapp-controller/pkg/reftracker" @@ -59,3 +60,61 @@ func Test_RemoveAppFromAllRefs_RemovesApp(t *testing.T) { t.Fatalf("expected app to be removed from appRefTracker after deletion") } } + +// Test_AppsForRef_SafeForConcurrentIteration verifies the returned map can be +// iterated by a caller while other goroutines mutate the tracker. Before the +// fix, AppsForRef returned the internal map directly and triggered Go's +// "concurrent map iteration and map write" fatal error. +func Test_AppsForRef_SafeForConcurrentIteration(t *testing.T) { + appRefTracker := reftracker.NewAppRefTracker() + + refKey := reftracker.NewSecretKey("secretName", "default") + refKeyMap := map[reftracker.RefKey]struct{}{refKey: {}} + + for i := 0; i < 20; i++ { + appKey := reftracker.NewAppKey("seed-app", "default") + appKey = reftracker.NewAppKey(appKey.RefName()+string(rune('a'+i)), "default") + appRefTracker.ReconcileRefs(refKeyMap, appKey) + } + + stop := make(chan struct{}) + var wg sync.WaitGroup + + // Mutator: continuously reconciles refs to mutate the internal map. + wg.Add(1) + go func() { + defer wg.Done() + i := 0 + for { + select { + case <-stop: + return + default: + appKey := reftracker.NewAppKey("mutator-app", "default") + appKey = reftracker.NewAppKey(appKey.RefName()+string(rune('a'+(i%26))), "default") + appRefTracker.ReconcileRefs(refKeyMap, appKey) + appRefTracker.RemoveAppFromAllRefs(appKey) + i++ + } + } + }() + + // Iterator: repeatedly fetches the apps map and iterates it. + iterDone := make(chan struct{}) + go func() { + defer close(iterDone) + for i := 0; i < 200; i++ { + apps, err := appRefTracker.AppsForRef(refKey) + if err != nil { + continue + } + for range apps { + // Force iteration; would fatal if apps aliased the internal map. + } + } + }() + + <-iterDone + close(stop) + wg.Wait() +} From 7215232bf11f23b8649141c48be4dd56c5f1d2f2 Mon Sep 17 00:00:00 2001 From: Sai Asish Y Date: Tue, 12 May 2026 00:18:19 -0700 Subject: [PATCH 2/2] trim verbose comments Signed-off-by: Sai Asish Y --- pkg/reftracker/ref_tracker.go | 4 ---- pkg/reftracker/ref_tracker_test.go | 7 ------- 2 files changed, 11 deletions(-) diff --git a/pkg/reftracker/ref_tracker.go b/pkg/reftracker/ref_tracker.go index 0558741ed..e75160553 100644 --- a/pkg/reftracker/ref_tracker.go +++ b/pkg/reftracker/ref_tracker.go @@ -28,10 +28,6 @@ func (a *AppRefTracker) AppsForRef(refKey RefKey) (map[RefKey]struct{}, error) { return nil, fmt.Errorf("could not find ref %s", refKey.Description()) } - // Return a copy so callers can iterate without holding the lock. - // Returning the internal map directly races with mutations from - // ReconcileRefs and RemoveAppFromAllRefs and triggers the Go runtime's - // "concurrent map iteration and map write" fatal error. return copyRefKeySet(apps), nil } diff --git a/pkg/reftracker/ref_tracker_test.go b/pkg/reftracker/ref_tracker_test.go index 70c5f8ac2..72bf8ec9f 100644 --- a/pkg/reftracker/ref_tracker_test.go +++ b/pkg/reftracker/ref_tracker_test.go @@ -61,10 +61,6 @@ func Test_RemoveAppFromAllRefs_RemovesApp(t *testing.T) { } } -// Test_AppsForRef_SafeForConcurrentIteration verifies the returned map can be -// iterated by a caller while other goroutines mutate the tracker. Before the -// fix, AppsForRef returned the internal map directly and triggered Go's -// "concurrent map iteration and map write" fatal error. func Test_AppsForRef_SafeForConcurrentIteration(t *testing.T) { appRefTracker := reftracker.NewAppRefTracker() @@ -80,7 +76,6 @@ func Test_AppsForRef_SafeForConcurrentIteration(t *testing.T) { stop := make(chan struct{}) var wg sync.WaitGroup - // Mutator: continuously reconciles refs to mutate the internal map. wg.Add(1) go func() { defer wg.Done() @@ -99,7 +94,6 @@ func Test_AppsForRef_SafeForConcurrentIteration(t *testing.T) { } }() - // Iterator: repeatedly fetches the apps map and iterates it. iterDone := make(chan struct{}) go func() { defer close(iterDone) @@ -109,7 +103,6 @@ func Test_AppsForRef_SafeForConcurrentIteration(t *testing.T) { continue } for range apps { - // Force iteration; would fatal if apps aliased the internal map. } } }()