fix(mock): guard MockLBRepo and MockGlobalLBRepo with RWMutex (#652)#698
Conversation
|
Warning Review limit reached
More reviews will be available in 25 minutes and 44 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR makes the in-memory mock Load Balancer repositories safer to use from concurrent goroutines by adding read/write locking around map and slice access.
Changes:
- Added
sync.RWMutexto mock repositories. - Wrapped CRUD and list operations with
RLock/RUnlockandLock/Unlock. - Applied the same locking approach to endpoint operations in the global LB mock repo.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| internal/repositories/mock/lb_repo.go | Adds an RWMutex and locks around map reads/writes for mock LB storage. |
| internal/repositories/mock/global_lb_repo.go | Adds an RWMutex and locks around GLB map and endpoints slice/map operations. |
Comments suppressed due to low confidence (1)
internal/repositories/mock/global_lb_repo.go:41
- The comment says "return copy" but the function returns the stored pointer directly. Either update the implementation to actually return a copy (recommended, especially with the new concurrency intent) or adjust/remove the comment to avoid misleading future readers.
func (m *MockGlobalLBRepo) GetByID(ctx context.Context, id uuid.UUID) (*domain.GlobalLoadBalancer, error) {
m.mu.RLock()
defer m.mu.RUnlock()
if glb, ok := m.GLBs[id]; ok {
// return copy
return glb, nil
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func (m *MockLBRepo) GetByID(ctx context.Context, id uuid.UUID) (*domain.LoadBalancer, error) { | ||
| m.mu.RLock() | ||
| defer m.mu.RUnlock() | ||
| if lb, ok := m.LBs[id]; ok { | ||
| return lb, nil | ||
| } |
| func (m *MockGlobalLBRepo) ListEndpoints(ctx context.Context, glbID uuid.UUID) ([]*domain.GlobalEndpoint, error) { | ||
| m.mu.RLock() | ||
| defer m.mu.RUnlock() | ||
| return m.Endpoints[glbID], nil | ||
| } |
github-actions
Bot
added
area/repositories
database
bug
poyrazK
force-pushed
the
fix/mock-lb-repo-race-652
branch
from
ca5edc8 to
7047908
Compare
…tions Methods were returning direct pointers/slices into internal map state after the lock was released. Callers could mutate that state, racing with concurrent writes. Return shallow copies instead. Addresses Copilot review findings on PR poyrazK#698 (closes poyrazK#652)
…urn defensive copies - Adds sync.RWMutex to MockLBRepo and MockGlobalLBRepo, wrapping all map accesses with appropriate read/write locks - Returns shallow copies from all getters to prevent callers mutating internal state after the lock is released (fixes Copilot race findings) - Closes poyrazK#652
poyrazK
force-pushed
the
fix/mock-lb-repo-race-652
branch
from
7047908 to
26b1923
Compare
Both mock repositories used plain maps with no synchronization, which races under concurrent tests and can panic on simultaneous map writes. Adds sync.RWMutex to each and wraps every map access in the matching read or write lock.
Closes #652.