Skip to content

Conversation

@Sukuna0007Abhi
Copy link
Contributor

@Sukuna0007Abhi Sukuna0007Abhi commented Jan 17, 2026

The issue is exhausted from a timing mismatch where the local system assumed reset was imminent (within ~2 minutes), but GitHub hadn't actually refreshed the key yet, leading to rapid cycling in a tight loop. To fix this, I've implemented a configurable 120-second buffer in the __handle_github_ratelimit_response method of github_data_access.py, ensuring keys stay expired long enough for GitHub to catch up.

Enhancement:

The buffer uses max(epoch_when_key_resets, time.time() + GITHUB_KEY_RESET_BUFFER) to only delay when necessary, preventing unnecessary waits while avoiding the race. This resolves the depletion issue without impacting normal operation, and the buffer is adjustable via the GITHUB_KEY_RESET_BUFFER environment variable for flexibility.So it does not stick to hardcoded the 120 seconds. It allows the buffer value to be configured via an environment variable...

How the buffer fixes it:

By setting expire_time = max(epoch_when_key_resets, time.time() + 120), the key stays expired for at least 120 seconds from now.

This ensures:

If reset is imminent, the key waits extra time for GitHub to catch up.
The loop is broken because the key isn't available for reuse too soon.

Why 120 seconds:

Matches the "within 2 minutes" window from the issue where the race occurs. It's conservative but not excessive.

Changes

  • Added a configurable 120-second buffer in __handle_github_ratelimit_response.
  • Keys now stay expired for at least 2 minutes when reset is imminent, preventing premature reuse.
  • Buffer is set via GITHUB_KEY_RESET_BUFFER env var (default 120s).

Credits

I used contributor @guptapratyksh's approach, thanks to him, but a little enhancement has done more to it.... As I fixed the hardcoded 120 seconds part..
& Thanks to @MoralCode and @cdolfi for flagging this issue..

I enhanced the contributor's 120-second buffer approach by making it conditional (only applies when reset is imminent via max()), configurable via env var, and defined as a maintainable constant—keeping it efficient and flexible without over-delaying keys.

This PR fixes #3598

Signed commits

  • Yes, I signed my commits.

Added a configurable 120-second buffer in __handle_github_ratelimit_response
to prevent premature key reuse when GitHub reset is imminent.

- GITHUB_KEY_RESET_BUFFER constant (default 120s, env configurable)
- Uses max(expire_time, now + buffer) for conditional delay

Signed-off-by: Sukuna0007Abhi <appsonly310@gmail.com>
@guptapratykshh
Copy link
Contributor

seems specific changes, as mentioned in the issue thread(#3598 (comment)), nothing is missed

@sgoggins sgoggins self-assigned this Jan 20, 2026
@sgoggins sgoggins requested a review from ABrain7710 January 20, 2026 17:24
Copy link
Member

@sgoggins sgoggins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM: Approving with the caveat that we need to have a plan for populating config items that are new.

from keyman.KeyClient import KeyClient

GITHUB_RATELIMIT_REMAINING_CAP = 50
GITHUB_KEY_RESET_BUFFER = int(os.getenv("GITHUB_KEY_RESET_BUFFER", 120))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using a default solves for there not being an entry in the config. I think we still need PRs that add config items to provide a pathway for those config items to make it into either the augur.json or the table.

@sgoggins sgoggins added release Related to releasing a new version of Augur ready Items tested and seeking additional approvals or a merge. Usually for items under active development labels Jan 20, 2026
@sgoggins sgoggins added this to the v0.92.0 Release milestone Jan 20, 2026
Copy link
Contributor

@MoralCode MoralCode left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Im a little concerned about whether this is the right solution and want some time to think about it in more depth after CHAOSScon

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready Items tested and seeking additional approvals or a merge. Usually for items under active development release Related to releasing a new version of Augur

Projects

None yet

Development

Successfully merging this pull request may close these issues.

first api key gets run down to 0 shortly before the keys reset

4 participants