add max_sleep_seconds, fix tests

Author: ericphanson

README.md

@@ -73,6 +73,7 @@ These methods all accept keyword arguments which control how the request is made
 
 - `max_retries::Int=5`: how many retries to attempt in requesting the resources. Retries are only made for idempotent requests ("GET", "HEAD", "OPTIONS", "TRACE", "PUT", "DELETE") and delays respect GitHub [rate limit headers](https://docs.github.com/en/rest/using-the-rest-api/rate-limits-for-the-rest-api?apiVersion=2022-11-28#checking-the-status-of-your-rate-limit).
 - `verbose::Bool=true`: whether or not to log retries as Info level logs
+- `max_sleep_seconds::Real=60*20`: if GitHub.jl intends to sleep for longer than `max_sleep_seconds` before retrying, e.g. due to rate limit headers from GitHub, throws an `RetryDelayException` instead. 
 
 #### Users and Organizations
 

src/GitHub.jl

@@ -44,7 +44,7 @@ export # auth.jl
        authenticate
 
 export # requests.jl
-       rate_limit
+       rate_limit, RetryDelayException
 
 ##################################
 # Owners (organizations + users) #

src/utils/requests.jl

@@ -184,8 +184,13 @@ function github_retry_decision(method::String, resp::Union{HTTP.Response, Nothin
     return (true, delay_seconds)
 end
 
+struct RetryDelayException <: Exception
+    msg::String
+end
+Base.showerror(io::IO, e::RetryDelayException) = print(io, e.msg)
+
 """
-    with_retries(f; method::AbstractString="GET", max_retries::Int=5, verbose::Bool=true, sleep_fn=sleep) -> Any
+    with_retries(f; method::AbstractString="GET", max_retries::Int=5, verbose::Bool=true, sleep_fn=sleep, max_sleep_seconds::Real = 20*60)
 
 Generic retry wrapper that executes function `f()` with GitHub-specific retry logic.
 
@@ -195,6 +200,7 @@ Generic retry wrapper that executes function `f()` with GitHub-specific retry lo
 - `max_retries`: Maximum number of retry attempts (default: 5)
 - `verbose`: Whether to log retry decisions (default: true)
 - `sleep_fn`: Function to call for sleeping between retries (default: sleep). For testing, can be replaced with a custom function.
+- `max_sleep_seconds::Real`: maximum number of seconds to sleep when delaying before retrying. If the intended retry delay exceeds `max_sleep_seconds` an exception is thrown instead. This parameter defaults to 20*60 (20 minutes).
 
 # Returns
 Returns the result of `f()` if successful, or re-throws the final exception if all retries fail.
@@ -206,7 +212,7 @@ result = with_retries(method="GET", verbose=false) do
 end
 ```
 """
-function with_retries(f; method::AbstractString="GET", max_retries::Int=5, verbose::Bool=true, sleep_fn=sleep)
+function with_retries(f; method::AbstractString="GET", max_retries::Int=5, verbose::Bool=true, sleep_fn=sleep, max_sleep_seconds::Real = 60*20)
     backoff = Base.ExponentialBackOff(n = max_retries+1)
 
     for (attempt, exponential_delay) in enumerate(backoff)
@@ -227,7 +233,7 @@ function with_retries(f; method::AbstractString="GET", max_retries::Int=5, verbo
         end
 
         # Check if we should retry based on this attempt
-        should_retry, sleep_seconds = github_retry_decision(method, r, ex, exponential_delay; verbose=verbose)
+        should_retry, sleep_seconds = github_retry_decision(method, r, ex, exponential_delay; verbose)
 
         if !should_retry
             if ex !== nothing
@@ -236,7 +242,9 @@ function with_retries(f; method::AbstractString="GET", max_retries::Int=5, verbo
                 return r
             end
         end
-
+        if sleep_seconds > max_sleep_seconds
+            throw(RetryDelayException("Retry delay $(sleep_seconds) exceeds configured maximum ($(max_sleep_seconds) seconds)"))
+        end
         if sleep_seconds > 0
             sleep_fn(sleep_seconds)
         end
@@ -246,14 +254,14 @@ end
 function github_request(api::GitHubAPI, request_method::String, endpoint;
                         auth = AnonymousAuth(), handle_error = true,
                         headers = Dict(), params = Dict(), allowredirects = true,
-                        max_retries = 5, verbose = true)
+                        max_retries = 5, verbose = true, max_sleep_seconds = 20*60)
     authenticate_headers!(headers, auth)
     params = github2json(params)
     api_endpoint = api_uri(api, endpoint)
     _headers = convert(Dict{String, String}, headers)
     !haskey(_headers, "User-Agent") && (_headers["User-Agent"] = "GitHub-jl")
 
-    r = with_retries(; method = request_method, max_retries, verbose) do
+    r = with_retries(; method = request_method, max_retries, verbose, max_sleep_seconds) do
         if request_method == "GET"
             return HTTP.request(request_method, URIs.URI(api_endpoint, query = params), _headers;
                                redirect = allowredirects, status_exception = false,
@@ -307,20 +315,20 @@ end
 extract_page_url(link) = match(r"<.*?>", link).match[2:end-1]
 
 function github_paged_get(api, endpoint; page_limit = Inf, start_page = "", handle_error = true,
-                          auth = AnonymousAuth(), headers = Dict(), params = Dict(), max_retries = 5, verbose = true, options...)
+                          auth = AnonymousAuth(), headers = Dict(), params = Dict(), max_retries = 5, verbose = true,  max_sleep_seconds = 20*60, options...)
     authenticate_headers!(headers, auth)
     _headers = convert(Dict{String, String}, headers)
     !haskey(_headers, "User-Agent") && (_headers["User-Agent"] = "GitHub-jl")
 
     # Helper function to make a get request with retries
     function make_request_with_retries(url, headers)
-        return with_retries(; method = "GET", max_retries, verbose) do
+        return with_retries(; method = "GET", max_retries, verbose, max_sleep_seconds) do
             HTTP.request("GET", url, headers; status_exception = false, retry = false)
         end
     end
 
     if isempty(start_page)
-        r = gh_get(api, endpoint; handle_error, headers = _headers, params, auth, max_retries, verbose, options...)
+        r = gh_get(api, endpoint; handle_error, headers = _headers, params, auth, max_retries, verbose, max_sleep_seconds, options...)
     else
         @assert isempty(params) "`start_page` kwarg is incompatible with `params` kwarg"
         r = make_request_with_retries(start_page, _headers)

test/retries.jl

@@ -2,6 +2,9 @@ using Test
 using HTTP
 using GitHub
 
+primary_rate_limit_body = Vector{UInt8}("primary rate limit")
+secondary_rate_limit_body = Vector{UInt8}("secondary rate limit")
+
 @testset "github_retry_decision" begin
 
     @testset "HTTP.jl recoverable exceptions" begin
@@ -42,9 +45,9 @@ using GitHub
         resp = HTTP.Response(403, [
             "x-ratelimit-remaining" => "0",
             "x-ratelimit-reset" => future_time
-        ])
+        ], primary_rate_limit_body)
 
-        should_retry, sleep_seconds = GitHub.github_retry_decision("GET",resp, nothing, 2.0; verbose=false)
+        should_retry, sleep_seconds = GitHub.github_retry_decision("GET", resp, nothing, 2.0; verbose=false)
         @test should_retry == true
         @test sleep_seconds > 100000  # Should be a large delay since reset time is far in future
 
@@ -53,18 +56,17 @@ using GitHub
         resp2 = HTTP.Response(403, [
             "x-ratelimit-remaining" => "0",
             "x-ratelimit-reset" => past_time
-        ])
+        ], primary_rate_limit_body)
 
-        should_retry, sleep_seconds = GitHub.github_retry_decision("GET",resp2, nothing, 5.0; verbose=false)
+        should_retry, sleep_seconds = GitHub.github_retry_decision("GET", resp2, nothing, 5.0; verbose=false)
         @test should_retry == true
         @test sleep_seconds == 5.0  # Should use the exponential delay
     end
 
     @testset "Secondary rate limit - retry-after header" begin
 
         # Test secondary rate limit with retry-after
-        body = """{"message": "You have been rate limited due to a secondary rate limit", "documentation_url": "..."}"""
-        resp = HTTP.Response(429, ["retry-after" => "30"]; body = Vector{UInt8}(body))
+        resp = HTTP.Response(429, ["retry-after" => "30"]; body = secondary_rate_limit_body)
 
         should_retry, sleep_seconds = GitHub.github_retry_decision("GET",resp, nothing, 2.0; verbose=false)
         @test should_retry == true
@@ -77,11 +79,8 @@ using GitHub
         @test sleep_seconds == 15.0
     end
 
-    @testset "Secondary rate limit - message in body" begin
-
-        # Test secondary rate limit detected from body message
-        body = """{"message": "You have exceeded a secondary rate limit. Please wait one minute before trying again."}"""
-        resp = HTTP.Response(429; body = Vector{UInt8}(body))
+    @testset "Secondary rate limit - no headers" begin
+        resp = HTTP.Response(429; body = secondary_rate_limit_body)
 
         should_retry, sleep_seconds = GitHub.github_retry_decision("GET",resp, nothing, 2.0; verbose=false)
         @test should_retry == true
@@ -97,20 +96,18 @@ using GitHub
 
         # Test secondary rate limit with reset time - use fixed timestamp to avoid race conditions
         future_time = "1900000000"  # Fixed timestamp in the future (year 2030)
-        body = """{"message": "secondary rate limit exceeded"}"""
         resp = HTTP.Response(403, [
             "x-ratelimit-remaining" => "0",
             "x-ratelimit-reset" => future_time
-        ]; body = Vector{UInt8}(body))
+        ], secondary_rate_limit_body)
 
         should_retry, sleep_seconds = GitHub.github_retry_decision("GET",resp, nothing, 5.0; verbose=false)
         @test should_retry == true
         @test sleep_seconds > 100000  # Should be a large delay since reset time is far in future
     end
 
-    @testset "Primary rate limit - exponential backoff" begin
-
-        # Primary rate limit without specific headers
+    @testset "429 - exponential backoff" begin
+        # 429 without specific headers or body
         resp = HTTP.Response(429, [])
 
         should_retry, sleep_seconds = GitHub.github_retry_decision("GET",resp, nothing, 4.0; verbose=false)
@@ -123,7 +120,6 @@ using GitHub
     end
 
     @testset "Other HTTP errors" begin
-
         for status in [408, 409, 500, 502, 503, 504, 599]
             resp = HTTP.Response(status, [])
 
@@ -134,8 +130,7 @@ using GitHub
     end
 
     @testset "Non-retryable client errors" begin
-
-        for status in [400, 401, 404, 422]
+        for status in [400, 401, 403, 404, 422]
             resp = HTTP.Response(status, [])
             should_retry, sleep_seconds = GitHub.github_retry_decision("GET",resp, nothing, 1.0; verbose=false)
             @test should_retry == false
@@ -144,25 +139,23 @@ using GitHub
     end
 
     @testset "Invalid header values" begin
-
-        # Test with invalid retry-after header (should fall back to secondary rate limit minimum)
-        resp1 = HTTP.Response(429, ["retry-after" => "invalid"], Vector{UInt8}("secondary rate limit"))
+        # Test with invalid retry-after header (should use secondary rate limit minimum)
+        resp1 = HTTP.Response(429, ["retry-after" => "invalid"], secondary_rate_limit_body)
         should_retry, sleep_seconds = GitHub.github_retry_decision("GET",resp1, nothing, 2.0; verbose=false)
         @test should_retry == true
         @test sleep_seconds == 60.0  # Falls back to secondary rate limit minimum (1 minute)
 
-        # Test with invalid reset time (should fall back to exponential backoff)
+        # Test with invalid reset time (should fall back to secondary min)
         resp2 = HTTP.Response(403, [
             "x-ratelimit-remaining" => "0",
             "x-ratelimit-reset" => "invalid"
-        ])
+        ], secondary_rate_limit_body)
         should_retry, sleep_seconds = GitHub.github_retry_decision("GET",resp2, nothing, 3.0; verbose=false)
         @test should_retry == true
-        @test sleep_seconds == 3.0  # Falls back to exponential backoff
+        @test sleep_seconds == 60.0  # minimum for secondary rate limit
     end
 
     @testset "Rate limit header precedence" begin
-
         # retry-after should take precedence over x-ratelimit-reset
         future_time = "1900000000"  # Fixed timestamp (doesn't matter since retry-after takes precedence)
         resp = HTTP.Response(429, [
@@ -183,7 +176,7 @@ using GitHub
         resp = HTTP.Response(403, [
             "x-ratelimit-remaining" => "5",
             "x-ratelimit-reset" => future_time
-        ])
+        ], primary_rate_limit_body)
 
         should_retry, sleep_seconds = GitHub.github_retry_decision("GET",resp, nothing, 3.0; verbose=false)
         @test should_retry == true
@@ -332,13 +325,13 @@ end
         current_time = time()
         reset_time = string(Int(round(current_time)) + 500000000)  # 500000000 seconds from now
 
-        result = GitHub.with_retries(method="GET", max_retries=1, verbose=false, sleep_fn=test_sleep) do
+        result = GitHub.with_retries(method="GET", max_retries=1, verbose=false, sleep_fn=test_sleep, max_sleep_seconds=2*500000000) do
             call_count[] += 1
             if call_count[] == 1
                 return HTTP.Response(403, [
                     "x-ratelimit-remaining" => "0",
                     "x-ratelimit-reset" => reset_time
-                ])
+                ], primary_rate_limit_body)
             else
                 return HTTP.Response(200)
             end
@@ -347,7 +340,15 @@ end
         @test result.status == 200
         @test call_count[] ==2
         @test length(sleep_calls) == 1
-        @test sleep_calls[1] >= 5.0  # Should wait at least until reset time
+        @test sleep_calls[1] >= 500000000  # Should wait at least until reset time
+
+
+        @test_throws RetryDelayException GitHub.with_retries(method="GET", max_retries=1, verbose=false, sleep_fn=test_sleep) do
+            return HTTP.Response(403, [
+                "x-ratelimit-remaining" => "0",
+                "x-ratelimit-reset" => reset_time
+            ], primary_rate_limit_body)
+        end
     end
 
     @testset "Secondary rate limit with retry-after" begin