Improve handling of Jira's retry-after handling (#1825)

* Respect the "Retry-After" times requested by Jira

The time Jira sends in the Retry-After header is the minimum time Jira wants us to wait before retrying our request. However, the former implementation used this as a maximum waiting time for the next request. In result, there was a chance that we reached three retries without reaching the time that Jira expected us to wait and our request would fail.

This implementation does also affect the other retry cases, as while previously we jittered our backoff between 0 and the target backoff, we now only jitter between 50% and 100% of the target backoff. However, this should still protect us from thundering herds and safes us from introducing a new minimum backoff variable for the retry-after case.

* Also retry requests where Jira specifies a Retry-after of 0 seconds

When rejecting request with a 429 response, Jira sometimes sends a Retry-after header asking for a backoff of 0 seconds. With the existing retry logic this would mark the request as non-retryable and thus fail the request. With this change, such requests are treated as if Jira had send a retry-after value of 1 second.

Author: matthias-bach-by

jira/resilientsession.py

@@ -315,7 +315,9 @@ def __recoverable(
             if response.status_code in recoverable_error_codes:
                 retry_after = response.headers.get("Retry-After")
                 if retry_after:
-                    suggested_delay = int(retry_after)  # Do as told
+                    suggested_delay = 2 * max(
+                        int(retry_after), 1
+                    )  # Do as told but always wait at least a little
                 elif response.status_code == HTTPStatus.TOO_MANY_REQUESTS:
                     suggested_delay = 10 * 2**counter  # Exponential backoff
 
@@ -326,7 +328,9 @@ def __recoverable(
         is_recoverable = suggested_delay > 0
         if is_recoverable:
             # Apply jitter to prevent thundering herd
-            delay = min(self.max_retry_delay, suggested_delay) * random.random()
+            delay = min(self.max_retry_delay, suggested_delay) * random.uniform(
+                0.5, 1.0
+            )
             LOG.warning(
                 f"Got recoverable error from {request_method} {url}, will retry [{counter}/{self.max_retries}] in {delay}s. Err: {msg}"  # type: ignore[str-bytes-safe]
             )

tests/test_resilientsession.py

@@ -65,13 +65,17 @@ def tearDown(self):
 
 
 # Retry test data tuples: (status_code, with_rate_limit_header, with_retry_after_header, retry_expected)
-with_rate_limit = with_retry_after = True
-without_rate_limit = without_retry_after = False
+with_rate_limit = True
+with_retry_after = 1
+without_rate_limit = False
+without_retry_after = None
 status_codes_retries_test_data = [
     # Always retry 429 responses
     (HTTPStatus.TOO_MANY_REQUESTS, with_rate_limit, with_retry_after, True),
+    (HTTPStatus.TOO_MANY_REQUESTS, with_rate_limit, 0, True),
     (HTTPStatus.TOO_MANY_REQUESTS, with_rate_limit, without_retry_after, True),
     (HTTPStatus.TOO_MANY_REQUESTS, without_rate_limit, with_retry_after, True),
+    (HTTPStatus.TOO_MANY_REQUESTS, without_rate_limit, 0, True),
     (HTTPStatus.TOO_MANY_REQUESTS, without_rate_limit, without_retry_after, True),
     # Retry 503 responses only when 'Retry-After' in headers
     (HTTPStatus.SERVICE_UNAVAILABLE, with_rate_limit, with_retry_after, True),
@@ -103,10 +107,11 @@ def test_status_codes_retries(
     mocked_request_method: Mock,
     status_code: int,
     with_rate_limit_header: bool,
-    with_retry_after_header: bool,
+    with_retry_after_header: int | None,
     retry_expected: bool,
 ):
-    RETRY_AFTER_HEADER = {"Retry-After": "1"}
+    RETRY_AFTER_SECONDS = with_retry_after_header or 0
+    RETRY_AFTER_HEADER = {"Retry-After": f"{RETRY_AFTER_SECONDS}"}
     RATE_LIMIT_HEADERS = {
         "X-RateLimit-FillRate": "1",
         "X-RateLimit-Interval-Seconds": "1",
@@ -124,7 +129,7 @@ def test_status_codes_retries(
 
     mocked_response: Response = Response()
     mocked_response.status_code = status_code
-    if with_retry_after_header:
+    if with_retry_after_header is not None:
         mocked_response.headers.update(RETRY_AFTER_HEADER)
     if with_rate_limit_header:
         mocked_response.headers.update(RATE_LIMIT_HEADERS)
@@ -141,6 +146,11 @@ def test_status_codes_retries(
     assert mocked_request_method.call_count == expected_number_of_requests
     assert mocked_sleep_method.call_count == expected_number_of_sleep_invocations
 
+    for actual_sleep in (
+        call_args.args[0] for call_args in mocked_sleep_method.call_args_list
+    ):
+        assert actual_sleep >= RETRY_AFTER_SECONDS
+
 
 errors_parsing_test_data = [
     (403, {"x-authentication-denied-reason": "err1"}, "", ["err1"]),