Add rate limiting functionality to Cache Warmer#1
Conversation
- Introduced a rate limiter to handle HTTP 429 responses, adjusting concurrency based on server feedback. - Added configuration options for rate limit cooldown and recovery. - Updated database query logic to use constants for HTTP status codes. - Enhanced error handling in the GetLastFlush method. - Defined constants for HTTP status codes and display truncation limits for improved readability and maintainability.
- Introduced a new configuration option for maximum retries on 429 responses. - Updated database logic to handle error messages more effectively. - Enhanced rate limiter to manage concurrency during cooldown periods. - Improved error handling in the fetch and warm methods to accommodate new retry logic.
- Added a new feature description for adaptive concurrency reduction on HTTP 429 responses. - Included configuration options for rate limit cooldown, recovery, and maximum retries in the README. - Updated troubleshooting section to address handling of 429 errors and concurrency adjustments.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| }() | ||
|
|
||
| status, errMsg := c.warmOne(ctx, u) | ||
| status, errMsg, slotReleased := c.warmOne(ctx, u) |
There was a problem hiding this comment.
🔴 Variable shadowing of slotReleased causes double-release of rate limiter slot
At cache-warmer.go:1064, the short variable declaration := creates a new slotReleased variable that shadows the outer slotReleased declared at line 1057. The deferred function at lines 1058-1062 captures the outer variable, which always remains false. When warmOne returns slotReleased=true (indicating it already released the slot), the defer still sees false and calls c.rl.release() a second time.
Root Cause and Impact
The code at line 1057 declares var slotReleased bool and the defer at line 1058 captures it by reference. Then at line 1064:
status, errMsg, slotReleased := c.warmOne(ctx, u)The := operator creates a new slotReleased in the inner scope, leaving the outer one as false.
warmOne returns slotReleased=true in three paths (lines 965-966, 971, 976) — all after it has already called c.rl.release(). The defer then calls c.rl.release() again, decrementing activeWorkers below its correct value (potentially to negative). This corrupts the rate limiter state: activeWorkers becomes artificially low, allowing more goroutines to acquire slots than currentConcurrency permits, completely defeating the rate limiting mechanism.
| status, errMsg, slotReleased := c.warmOne(ctx, u) | |
| status, errMsg, sr := c.warmOne(ctx, u) | |
| slotReleased = sr |
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
Uh oh!
There was an error while loading. Please reload this page.