[bug] - Fix race condition

[ahrav]

Description:

The function uses a global mutex rateLimitMu to synchronize access to the rateLimitResumeTime variable. However, when reading the rateLimitResumeTime variable. The code first reads the value using rateLimitMu.RLock() and then later locks the mutex using rateLimitMu.Lock() to update the value. If another goroutine updates the rateLimitResumeTime between the read and write operations.

Example:
Thread 1:

• Acquires rateLimitMu.RLock().
• Reads the value of rateLimitResumeTime.
• Releases rateLimitMu.RUnlock().
  Thread 2:
• Acquires rateLimitMu.Lock().
• Modifies rateLimitResumeTime.
• Releases rateLimitMu.Unlock().
  Thread 1 (cont.):
• Proceeds based on the old value of rateLimitResumeTime that it read earlier.

Checklist:

• Tests passing (make test-community)?
• Lint passing (make lint this requires golangci-lint)?

[rgmz]

Interesting, I didn't realize that such a race was possible. The underlying go-github repository implements its own rate limit handling, so it seemed highly unlikely that rateLimitResumeTime would be updated while an existing rate limit was still valid.

[rosecodym]

This is changing code introduced by #2379, right?

[rgmz]

I think it's entirely from #2041.

[ahrav]

This solution is actually not ideal. After some discussion w/ @mcastorina the fact we are holding the lock while sleeping is raising some alarm 🔔s. Additionally this approach potentially results in a stale error state once a waiting goroutine resumes processing after the retry after period. Converting this to a Draft for now.

[rgmz]

After some discussion w/ @mcastorina the fact we are holding the lock while sleeping is raising some alarm 🔔s

Yeah, that's why I used the (admittedly awkward) explicit unlocks instead of defer.

#2041 (comment)