Skip to content

Return copies from AppRefTracker AppsForRef and RefsForApp - #1890

Open
himsngh wants to merge 1 commit into
carvel-dev:developfrom
himsngh:himsngh/fix-reftracker-map-race
Open

himsngh wants to merge 1 commit into
carvel-dev:developfrom
himsngh:himsngh/fix-reftracker-map-race

Conversation

@himsngh

@himsngh himsngh commented Oct 27, 2026

Copy link
Copy Markdown
Contributor

AppRefTracker.AppsForRef and RefsForApp take the tracker lock, but return the tracker's internal maps. The Secret and ConfigMap event handlers range over the map returned by AppsForRef after the lock is released, while App and PackageRepository reconcilers call ReconcileRefs and RemoveAppFromAllRefs from other goroutines, which modify the same maps. Concurrent iteration and modification of a map is a data race, and the Go runtime can terminate the controller with "fatal error: concurrent map iteration and map write", which is not recoverable.

Return a copy of the map from both accessors while the lock is held, so callers iterate a snapshot that is not modified by later tracker updates. Return types are unchanged, so callers do not need changes.

Changes:

  1. pkg/reftracker/ref_tracker.go

    • Return maps.Clone of the internal map from AppsForRef and RefsForApp
  2. pkg/reftracker/ref_tracker_test.go

    • Add Test_AppsForRef_RefsForApp_ReturnSnapshot verifying returned maps are not affected by later tracker changes and vice versa
    • Add Test_AppsForRef_ConcurrentWithReconcileRefs which iterates returned maps while another goroutine updates the tracker. Without this change it fails under -race, and without -race it usually crashes with "concurrent map iteration and map write".

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #

Does this PR introduce a user-facing change?


Additional Notes for your reviewer:

Review Checklist:
  • Follows the developer guidelines
  • Relevant tests are added or updated
  • Relevant docs in this repo added or updated
  • Relevant carvel.dev docs added or updated in a separate PR and there's
    a link to that PR
  • Code is at least as readable and maintainable as it was before this
    change

Additional documentation e.g., Proposal, usage docs, etc.:


AppRefTracker.AppsForRef and RefsForApp take the tracker lock, but
return the tracker's internal maps. The Secret and ConfigMap event
handlers range over the map returned by AppsForRef after the lock is
released, while App and PackageRepository reconcilers call
ReconcileRefs and RemoveAppFromAllRefs from other goroutines, which
modify the same maps. Concurrent iteration and modification of a map
is a data race, and the Go runtime can terminate the controller with
"fatal error: concurrent map iteration and map write", which is not
recoverable.

Return a copy of the map from both accessors while the lock is held,
so callers iterate a snapshot that is not modified by later tracker
updates. Return types are unchanged, so callers do not need changes.

Changes:

1. pkg/reftracker/ref_tracker.go
   - Return maps.Clone of the internal map from AppsForRef and
     RefsForApp

2. pkg/reftracker/ref_tracker_test.go
   - Add Test_AppsForRef_RefsForApp_ReturnSnapshot verifying returned
     maps are not affected by later tracker changes and vice versa
   - Add Test_AppsForRef_ConcurrentWithReconcileRefs which iterates
     returned maps while another goroutine updates the tracker. Without
     this change it fails under -race, and without -race it usually
     crashes with "concurrent map iteration and map write".

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Himanshu Singh <himansh.singh3@gmail.com>

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

1 participant