Summary
Client.rateLimiter is a single *RateLimiter shared across all 6 concurrent
goroutines in GetAllRecommendations. Each goroutine calls
fetchRIPageWithRetry (or fetchSPPageWithRetry) which calls
c.rateLimiter.Reset() at the start of every CE call. With 6 goroutines
running simultaneously, two goroutines race on the same Reset() write path.
Discovered during -race run while reviewing PR #842. The race is pre-existing
on main (confirmed: the same failure reproduces before the PR's changes).
Reproduction
cd providers/aws
go test -race ./recommendations/... -run TestGetAllRecommendations
Expected: PASS
Actual: DATA RACE -- RateLimiter.Reset() concurrent write at ratelimiter.go:138
Root cause
feedback_rate_limiter_per_call.md documents this exact pattern:
instantiate a fresh rate limiter per sweep; a shared mutable limiter
causes data races under go test -race
Client.rateLimiter is initialized once in NewClient/NewClientWithAPI
and then shared. The fix is to instantiate a NewRateLimiter() locally
inside fetchRIPageWithRetry and fetchSPPageWithRetry (and any analogous
sweep functions), not in the Client struct, so each concurrent call has its
own limiter state.
Fix sketch
Remove rateLimiter *RateLimiter from Client. In fetchRIPageWithRetry
and fetchSPPageWithRetry, create rl := NewRateLimiter() as a local
variable and use it for the retry loop.
Summary
Client.rateLimiteris a single*RateLimitershared across all 6 concurrentgoroutines in
GetAllRecommendations. Each goroutine callsfetchRIPageWithRetry(orfetchSPPageWithRetry) which callsc.rateLimiter.Reset()at the start of every CE call. With 6 goroutinesrunning simultaneously, two goroutines race on the same
Reset()write path.Discovered during
-racerun while reviewing PR #842. The race is pre-existingon
main(confirmed: the same failure reproduces before the PR's changes).Reproduction
Expected: PASS
Actual: DATA RACE --
RateLimiter.Reset()concurrent write atratelimiter.go:138Root cause
feedback_rate_limiter_per_call.mddocuments this exact pattern:Client.rateLimiteris initialized once inNewClient/NewClientWithAPIand then shared. The fix is to instantiate a
NewRateLimiter()locallyinside
fetchRIPageWithRetryandfetchSPPageWithRetry(and any analogoussweep functions), not in the
Clientstruct, so each concurrent call has itsown limiter state.
Fix sketch
Remove
rateLimiter *RateLimiterfromClient. InfetchRIPageWithRetryand
fetchSPPageWithRetry, createrl := NewRateLimiter()as a localvariable and use it for the retry loop.