[MeshSync] Fix data race on informer stores map#592
Conversation
startDiscovery replaces h.stores wholesale on the discovery goroutine while handleInformerStoreRequest ranges it on the request-listener goroutine, an unsynchronized read/write of the map field. Guard it with storesMu (RWMutex) and route both accesses through replaceStores and snapshotStores, mirroring the sessions-map fix. snapshotStores copies the store slice under the read lock and releases it before any store List(), so a store read never blocks the next discovery swap. Signed-off-by: Harsh Singh <[email protected]>
There was a problem hiding this comment.
Code Review
This pull request introduces thread-safe access to the 'stores' map in the 'Handler' struct by adding a 'sync.RWMutex' ('storesMu') and implementing 'replaceStores' and 'snapshotStores' helper methods. These changes prevent race conditions between the discovery goroutine, which updates the stores, and the request listener goroutine, which reads them. Additionally, a new test file 'meshsync/stores_test.go' has been added to verify concurrent access and correct snapshot behavior. I have no further feedback to provide.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe handler now protects discovery store replacement with an RWMutex, snapshots stores before listing objects, and adds tests for concurrent access, complete snapshots, and nil store maps. ChangesStore concurrency
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Motivation
Handler.stores(the per-GVR informer store set) is replaced wholesale bystartDiscoveryon the discovery goroutine on every (re)discovery, and read byhandleInformerStoreRequest→listStoreObjectson the request-listener goroutine when Meshery Server issues aninformer-storerequest. These run concurrently with no synchronization, so the map field is read and written without a happens-before relationship — a data race (go test -raceflags it). This is the same class of bug already fixed for the exec/log-streamsessionsmap.Change
storesMu sync.RWMutexguarding thestoresfield.replaceStoresperforms the guarded wholesale swap;snapshotStorescopies the current store values under the read lock and returns them, so the lock is never held acrosscache.Store.List().listStoreObjectsiteratessnapshotStores()instead of the raw map.No exported API, wire format, CRD, or config changes. Access is read-mostly, hence
RWMutex; the set of objects returned is unchanged.Testing
meshsync/stores_test.go:TestStoresConcurrentAccess(16 goroutines racing replace/snapshot) passes under-raceand reports a data race when the guard is removed; plus completeness and nil-map cases.go test --short ./... -raceis green across all packages;go vetandgofmtclean.Roadmap fit
Prerequisite for the periodic-reconciliation blueprint (
docs/design/fd5-periodic-reconciliation.md), which adds a timer-driven reconcile loop that becomes the first concurrent reader ofh.storesalongside the existing informer-store path. Submitted as a standalone fix so the reconcile feature builds on a race-free store, keeping the bug fix separate from the feature.Summary by CodeRabbit
Bug Fixes
Tests