Retry retryable 403 (rate limit) #2387

EnricoMi · 2022-12-21T07:33:08Z

GitHub response with rate limit errors (403 Forbidden) when request occur too frequently. While they can be mitigated by throttling request rate (see #1989), they can still occur.

Not all 403 errors are retry-able (e.g. access forbidden if not authenticated), so retrying based on status code (403) only would be a bad idea. The urllib3.util.retry.Retry interface does not allow for checking the response message or body content, only response code: urllib3/urllib3#2500

This PR introduces a bespoke PyGithub retry class that is able to determine whether to retry based on the response content. With that, 403 responses that refer to RateLimitExceeded are retried, other 403 responses are not retried. The base Retry implementation respects Retry-After headers if they exist.

Primary rate limit errors are retried after the primary rate reset occurs, secondary rate limit errors are retried after 60 seconds (configurable).

Log for a primary error rate limit:

2022-12-16 13:56:47 +0000 - urllib3.connectionpool - DEBUG - https://api.github.com:443 "GET /repositories/290227969/commits/71ca27c4a9d74fa7ec2898304ee75909e4a57124/check-runs?per_page=1&page=150 HTTP/1.1" 403 None
2022-12-16 13:56:47 +0000 - github.GithubRetry -  INFO - Request GET /repositories/290227969/commits/71ca27c4a9d74fa7ec2898304ee75909e4a57124/check-runs?per_page=1&page=150 failed with 403: Forbidden
2022-12-16 13:56:47 +0000 - github.GithubRetry - DEBUG - Response body indicates retry-able primary rate limit error: API rate limit exceeded for installation ID 5425010.
2022-12-16 13:56:47 +0000 - urllib3.util.retry - DEBUG - Incremented Retry for (url='/repositories/290227969/commits/71ca27c4a9d74fa7ec2898304ee75909e4a57124/check-runs?per_page=1&page=150'): GithubRetry(total=9, connect=None, read=None, redirect=None, status=None)
2022-12-16 13:56:47 +0000 - github.GithubRetry - DEBUG - Reset occurs in 0:24:24.940607 (1671200472 / 2022-12-16 14:21:12)
2022-12-16 13:56:47 +0000 - github.GithubRetry - INFO - Setting next backoff to 1465.940607s
2022-12-16 14:21:13 +0000 - urllib3.connectionpool - DEBUG - Retry: /repositories/290227969/commits/71ca27c4a9d74fa7ec2898304ee75909e4a57124/check-runs?per_page=1&page=150
2022-12-16 14:21:13 +0000 - urllib3.connectionpool - DEBUG - Resetting dropped connection: api.github.com
2022-12-16 14:21:13 +0000 - urllib3.connectionpool - DEBUG - https://api.github.com:443 "GET /repositories/290227969/commits/71ca27c4a9d74fa7ec2898304ee75909e4a57124/check-runs?per_page=1&page=150 HTTP/1.1" 200 None
2022-12-16 14:21:13 +0000 - github.Requester - DEBUG - GET https://api.github.com/repositories/290227969/commits/71ca27c4a9d74fa7ec2898304ee75909e4a57124/check-runs?per_page=1&page=150 {'Accept': 'application/vnd.github.v3+json', 'Authorization': 'token (oauth token removed)', 'User-Agent': 'PyGithub/Python'} None ==> 200

Log for a secondary error rate limit:

2022-12-17 09:48:56 +0000 - urllib3.connectionpool - DEBUG - https://api.github.com:443 "GET /search/issues?q=type%3Apr+repo%3A%22EnricoMi%2Fpublish-unit-test-result-action%22+df23eea893bc760a02ff52d63d9ce4c6a06c381b&per_page=10 HTTP/1.1" 403 None
2022-12-17 09:48:56 +0000 - github.GithubRetry -  INFO - Request GET /search/issues?q=type%3Apr+repo%3A%22EnricoMi%2Fpublish-unit-test-result-action%22+df23eea893bc760a02ff52d63d9ce4c6a06c381b&per_page=10 failed with 403: Forbidden
2022-12-17 09:48:56 +0000 - github.GithubRetry - DEBUG - Response body indicates retry-able secondary rate limit error: You have exceeded a secondary rate limit. Please wait a few minutes before you try again.
2022-12-17 09:48:56 +0000 - urllib3.util.retry - DEBUG - Incremented Retry for (url='/search/issues?q=type%3Apr+repo%3A%22EnricoMi%2Fpublish-unit-test-result-action%22+df23eea893bc760a02ff52d63d9ce4c6a06c381b&per_page=10'): GithubRetry(total=8, connect=None, read=None, redirect=None, status=None)
2022-12-17 09:48:56 +0000 - github.GithubRetry - DEBUG - Setting next backoff to 60s
2022-12-17 09:49:56 +0000 - urllib3.connectionpool - DEBUG - Retry: /search/issues?q=type%3Apr+repo%3A%22EnricoMi%2Fpublish-unit-test-result-action%22+df23eea893bc760a02ff52d63d9ce4c6a06c381b&per_page=10
2022-12-17 09:49:56 +0000 - urllib3.connectionpool - DEBUG - https://api.github.com:443 "GET /search/issues?q=type%3Apr+repo%3A%22EnricoMi%2Fpublish-unit-test-result-action%22+df23eea893bc760a02ff52d63d9ce4c6a06c381b&per_page=10 HTTP/1.1" 200 None

codecov-commenter · 2022-12-21T07:50:21Z

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.01 🎉

Comparison is base (094538e) 98.35% compared to head (9735c1d) 98.36%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2387      +/-   ##
==========================================
+ Coverage   98.35%   98.36%   +0.01%     
==========================================
  Files         131      132       +1     
  Lines       13178    13280     +102     
==========================================
+ Hits        12961    13063     +102     
  Misses        217      217

Impacted Files	Coverage Δ
github/GithubRetry.py	`100.00% <100.00%> (ø)`
github/MainClass.py	`99.30% <100.00%> (+<0.01%)`	⬆️
github/Requester.py	`97.43% <100.00%> (+0.10%)`	⬆️
github/__init__.py	`94.11% <100.00%> (+0.36%)`	⬆️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

hellkite500 · 2023-01-05T16:08:22Z

I have browsed through this PR and it looks practical to me, but I'm not fully up to speed with the code review requirements of this project to offer an official review. I'm about to pull this branch though and do some testing to see if this works for my use case.

Update:
Pulled this PR and ran a large workload against the API. Worked seamlessly! Thanks for your effort on this!

dkujawski · 2023-01-09T18:02:51Z

This works great in my testing. Implementation is clean and contained. Very similar to what I have implemented for my internal tools. I have a framework that iterates over many repositories(hundreds) using concurrent.futures to create branches, perform change operations and create PRs. I swapped out my implementation with a build from this branch and it performed as expected. In my tests, reducing the secondaryRateWait to 5s demonstrated the retries and eventual exceptions. For my use case secondaryRateWait=15 seems to be ideal.

Happy to help and/or provide any additional feedback where needed.

Thanks for the work on this @EnricoMi !

EnricoMi · 2023-01-09T18:36:08Z

@dkujawski thanks for testing this!

mikeNG · 2023-01-13T11:13:39Z

I can confirm this also works fine in my scripts that work with the entire https://github.com/LineageOS org (around 2500 repositories)

EnricoMi · 2023-02-17T10:19:32Z

@s-t-e-v-e-n-k @sfdye rebased with latest master, what do you think?

JosephTLyons · 2023-03-21T15:57:29Z

This seems so practical - would love to see this land.

lawndoc · 2023-04-11T17:54:34Z

github/GithubRetry.py

+                            )
+
+                            # we backoff primary rate limit at least until X-RateLimit-Reset,
+                            # we backoff secondary rate limit at for secondaryRateWait seconds


In my experience we can use x-ratelimit-reset for secondary wait as well, we don't have to guess

We could still potentially use secondaryRateWait as the max time we are willing to wait

RomainC-lab · 2023-04-19T15:24:24Z

Hello,
I’m interested in this feature and I appreciate your work on it.
Do you have an estimate of when it will be ready to merge?
Thank you.

EnricoMi · 2023-06-26T06:54:01Z

@JLLeitschuh can I get a review here as well, please?

JLLeitschuh · 2023-06-26T14:51:56Z

github/GithubRetry.py

+    # used to mock datetime, mock.patch("github.GithubRetry.date") does not work as this
+    # references the class, not the module (due to re-exporting in github/__init__.py)
+    __datetime = datetime


I see you mention mocking here, but the changes don't seem to actually do any mocking. Am I missing something?

The mocking is done here:
https://github.com/PyGithub/PyGithub/pull/2387/files#diff-223d619ef9fda22ba4275f05a88a10b6403d6735aa31b8985be6062a910bd288R124-R138

…setters into BasicTestCase" This reverts commit 01ae50a.

JLLeitschuh

LGTM

EnricoMi force-pushed the branch-retry-retryable-403 branch from 1588ec3 to 3265cd1 Compare December 21, 2022 07:54

EnricoMi changed the title ~~Retry retryable 403 errors (rate limit errors)~~ Retry retryable 403 (rate limit) errors Dec 21, 2022

EnricoMi force-pushed the branch-retry-retryable-403 branch 2 times, most recently from 5225698 to 8b9d62d Compare December 21, 2022 14:00

hellkite500 mentioned this pull request Jan 5, 2023

API rate limit exceeded clockfort/GitHub-Backup#42

Closed

EnricoMi changed the title ~~Retry retryable 403 (rate limit) errors~~ Retry retryable 403 (rate limit) and 5xx errors Jan 21, 2023

EnricoMi changed the title ~~Retry retryable 403 (rate limit) and 5xx errors~~ Retry retryable 403 (rate limit) Jan 21, 2023

EnricoMi force-pushed the branch-retry-retryable-403 branch from 8b9d62d to b9575ba Compare February 17, 2023 09:50

EnricoMi mentioned this pull request Mar 20, 2023

PyGithub should provide means to throttle API requests #1989

Closed

EnricoMi force-pushed the branch-retry-retryable-403 branch from b9575ba to 16af8e8 Compare March 20, 2023 16:05

lawndoc reviewed Apr 11, 2023

View reviewed changes

EnricoMi modified the milestones: Version 1.59.0, Version 1.60.0 Jun 13, 2023

EnricoMi mentioned this pull request Jun 19, 2023

Raise a RateLimitExceededException when a secondary rate limit is hit #2127

Closed

EnricoMi force-pushed the branch-retry-retryable-403 branch from 16af8e8 to eb5aecb Compare June 19, 2023 10:56

EnricoMi requested a review from JLLeitschuh June 19, 2023 13:33

EnricoMi force-pushed the branch-retry-retryable-403 branch from a1bb64b to 47920ac Compare June 26, 2023 06:58

JLLeitschuh reviewed Jun 26, 2023

View reviewed changes

EnricoMi added 2 commits June 28, 2023 09:24

Add PyGithub specific Retry class that retries retryable 403 responses

92a1368

Add logic to wait for X-RateLimit-Reset (primary rate limit)

db7e66f

EnricoMi added 26 commits June 28, 2023 09:24

Revert "Allow for setting per_page in tests, mov retry and pool size …

af66c06

…setters into BasicTestCase" This reverts commit 01ae50a.

Reuse DEFAULT_SECONDARY_RATE_WAIT in tests

9bbd8e2

Improve typing

e679afe

Log 403 error as info

b62ed70

Test and fix error on non-retryable 403

b050f97

Test more messages, add : to info logging

5080217

Check early that we are at all retrying

9bcbd3c

Reworking logging, simplifying retry logic

fdaa4eb

Include another secondary rate limit error

6226c1e

Minor logging rework

ef9e493

Fix import

72f16fb

Import mock from unittest package

54239fb

Linting

88551b6

Rework __utc_now and __log mocking to work with Python 3.11

d6e8729

Make accessing mock.call.args and .kwargs work with Python 3.7

37fe675

Add doc string to GithubRetry, retry 500...599, GET and Post

892c09e

Add retry example to README.md and introduction.rst

c7b4e55

Remove Retry.RETRY_AFTER_STATUS_CODES from default status_forcelist

89b0865

Use GithubRetry by default

ade34a3

fixup tests

56e7d5e

Replace __utc_now with __datetime, add typing

c3dc610

Fix typing issues

04cf079

Ignore some typings

16a55cc

Make secondary_rate_wait parameter float

0cfff7e

Make log messages stabled across Python versions

84d711a

Patch datetime differently in Python 3.11

9735c1d

EnricoMi force-pushed the branch-retry-retryable-403 branch from 47920ac to 9735c1d Compare June 28, 2023 07:24

JLLeitschuh approved these changes Jun 28, 2023

View reviewed changes

JLLeitschuh merged commit 0bb72ca into PyGithub:main Jun 28, 2023
10 checks passed

TimidRobot mentioned this pull request Aug 7, 2023

[Bug] Implement GitHub retries for 5xx status codes creativecommons/ccos-scripts#179

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Retry retryable 403 (rate limit) #2387

Retry retryable 403 (rate limit) #2387

EnricoMi commented Dec 21, 2022 •

edited

codecov-commenter commented Dec 21, 2022 •

edited

hellkite500 commented Jan 5, 2023 •

edited

dkujawski commented Jan 9, 2023

EnricoMi commented Jan 9, 2023

mikeNG commented Jan 13, 2023

EnricoMi commented Feb 17, 2023

JosephTLyons commented Mar 21, 2023

lawndoc Apr 11, 2023

lawndoc Apr 11, 2023

RomainC-lab commented Apr 19, 2023

EnricoMi commented Jun 26, 2023

JLLeitschuh Jun 26, 2023

EnricoMi Jun 27, 2023

JLLeitschuh left a comment

Retry retryable 403 (rate limit) #2387

Retry retryable 403 (rate limit) #2387

Conversation

EnricoMi commented Dec 21, 2022 • edited

codecov-commenter commented Dec 21, 2022 • edited

Codecov Report

hellkite500 commented Jan 5, 2023 • edited

dkujawski commented Jan 9, 2023

EnricoMi commented Jan 9, 2023

mikeNG commented Jan 13, 2023

EnricoMi commented Feb 17, 2023

JosephTLyons commented Mar 21, 2023

lawndoc Apr 11, 2023

Choose a reason for hiding this comment

lawndoc Apr 11, 2023

Choose a reason for hiding this comment

RomainC-lab commented Apr 19, 2023

EnricoMi commented Jun 26, 2023

JLLeitschuh Jun 26, 2023

Choose a reason for hiding this comment

EnricoMi Jun 27, 2023

Choose a reason for hiding this comment

JLLeitschuh left a comment

Choose a reason for hiding this comment

EnricoMi commented Dec 21, 2022 •

edited

codecov-commenter commented Dec 21, 2022 •

edited

hellkite500 commented Jan 5, 2023 •

edited