RF: centralize retry-after parsing logic, add to other places where R…

…ETRY_CODES are used

It also add additional checks/protection against odd retry-after results (too long negatives or positives),
which should address some concerns raised in prior code review

dandi/dandiapi.py

@@ -43,6 +43,7 @@
     chunked,
     ensure_datetime,
     get_instance,
+    get_retry_after,
     is_interactive,
     is_page2_url,
     joinurl,
@@ -236,6 +237,13 @@ def request(
                             )
                             if data is not None and hasattr(data, "seek"):
                                 data.seek(0)
+                        if retry_after := get_retry_after(result):
+                            lgr.debug(
+                                "Sleeping for %d seconds as instructed in response "
+                                "(in addition to tenacity imposed)",
+                                retry_after,
+                            )
+                            sleep(retry_after)
                         result.raise_for_status()
         except Exception as e:
             if isinstance(e, requests.HTTPError):

dandi/dandiarchive.py

@@ -51,7 +51,7 @@
 )
 from .dandiapi import BaseRemoteAsset, DandiAPIClient, RemoteDandiset
 from .exceptions import FailedToConnectError, NotFoundError, UnknownURLError
-from .utils import get_instance
+from .utils import get_instance, get_retry_after
 
 lgr = get_logger()
 
@@ -893,14 +893,26 @@ def follow_redirect(url: str) -> str:
         while True:
             r = requests.head(url, allow_redirects=True)
             if r.status_code in RETRY_STATUSES and i < 4:
-                delay = 0.1 * 10**i
-                lgr.warning(
-                    "HEAD request to %s returned %d; sleeping for %f seconds and then retrying...",
-                    url,
-                    r.status_code,
-                    delay,
-                )
-                sleep(delay)
+                retry_after = get_retry_after(r)
+                if retry_after is not None:
+                    delay = retry_after
+                else:
+                    delay = 0.1 * 10**i
+                if delay:
+                    lgr.warning(
+                        "HEAD request to %s returned %d; "
+                        "sleeping for %f seconds and then retrying...",
+                        url,
+                        r.status_code,
+                        delay,
+                    )
+                    sleep(delay)
+                else:
+                    lgr.warning(
+                        "HEAD request to %s returned %d; retrying...",
+                        url,
+                        r.status_code,
+                    )
                 i += 1
                 continue
             elif r.status_code == 404:

dandi/download.py

@@ -3,8 +3,7 @@
 from collections import Counter, deque
 from collections.abc import Callable, Iterable, Iterator, Sequence
 from dataclasses import InitVar, dataclass, field
-from datetime import datetime, timezone
-from email.utils import parsedate_to_datetime
+from datetime import datetime
 from enum import Enum
 from functools import partial
 import hashlib
@@ -50,6 +49,7 @@
     ensure_datetime,
     exclude_from_zarr,
     flattened,
+    get_retry_after,
     is_same_time,
     path_is_subpath,
     pluralize,
@@ -1121,44 +1121,17 @@ def _check_if_more_attempts_allowed(
                 exc,
             )
             return None
-        elif retry_after := exc.response.headers.get("Retry-After"):
-            if retry_after.isdigit():
-                sleep_amount = int(retry_after)
-            else:
-                # else if it is a datestamp like "Wed, 21 Oct 2015 07:28:00 GMT"
-                # we could parse it and calculate how long to sleep
-                try:
-                    retry_after_date = parsedate_to_datetime(retry_after)
-                    current_date = datetime.now(timezone.utc)
-                    difference = retry_after_date - current_date
-                    sleep_amount = int(difference.total_seconds())
-                    if sleep_amount < 0:
-                        lgr.warning(
-                            "%s - date in Retry-After=%r is in the past (current is %r)",
-                            path,
-                            retry_after,
-                            current_date,
-                        )
-                except ValueError as exc_ve:
-                    # our code or response is wrong, do not crash but issue warning
-                    # and continue with "default" sleep logic
-                    lgr.warning(
-                        "%s - download failed due to response %d with non-integer or future date"
-                        " Retry-After=%r: %s with %s upon converting assuming it is a date",
-                        path,
-                        exc.response.status_code,
-                        retry_after,
-                        exc,
-                        exc_ve,
-                    )
-            lgr.debug(
-                "%s - download failed due to response %d with "
-                "Retry-After=%d: %s, will sleep and retry",
-                path,
-                exc.response.status_code,
-                sleep_amount,
-                exc,
-            )
+        elif sleep_amount_ := get_retry_after(exc.response):
+            if sleep_amount_ is not None:  # could be 0
+                sleep_amount = sleep_amount_
+                lgr.debug(
+                    "%s - download failed due to response %d with "
+                    "Retry-After=%d: %s, will sleep and retry",
+                    path,
+                    exc.response.status_code,
+                    sleep_amount,
+                    exc,
+                )
     if sleep_amount < 0:
         # it was not Retry-after set, so we come up with random duration to sleep
         sleep_amount = random.random() * 5 * attempt

dandi/utils.py

@@ -3,6 +3,7 @@
 from bisect import bisect
 from collections.abc import Iterable, Iterator
 import datetime
+from email.utils import parsedate_to_datetime
 from functools import lru_cache
 from importlib.metadata import version as importlib_version
 import inspect
@@ -890,3 +891,65 @@ def joinurl(base: str, path: str) -> str:
         return path
     else:
         return base.rstrip("/") + "/" + path.lstrip("/")
+
+
+def get_retry_after(response: requests.Response) -> Optional[int]:
+    """If provided and parsed ok, returns duration in seconds to sleep before retry.
+
+    If not provided in the response header `Retry-After`, would
+    return None.
+    If parsing fails, or provided date/sleep does not make sense
+    since either too far in the past (over 2 seconds) or in the future
+    (over a week), would return None.
+    """
+    if_unparsable = None
+    retry_after = response.headers.get("Retry-After")
+    if retry_after is None:
+        return None
+    sleep_amount: int | None
+    if retry_after.isdecimal():
+        sleep_amount = int(retry_after)
+    else:
+        # else if it is a datestamp like "Wed, 21 Oct 2015 07:28:00 GMT"
+        # we could parse it and calculate how long to sleep
+        current_date = datetime.datetime.now(datetime.timezone.utc)
+        try:
+            retry_after_date = parsedate_to_datetime(retry_after)
+        except (ValueError, TypeError) as exc_ve:
+            # our code or response is wrong, do not crash but issue warning
+            # and continue with "if_unparsable" sleep logic
+            lgr.warning(
+                "response %d has incorrect date in Retry-After=%r: %s. " "Returning %r",
+                response.status_code,
+                retry_after,
+                exc_ve,
+            )
+            sleep_amount = if_unparsable
+        else:
+            difference = retry_after_date - current_date
+            sleep_amount = int(difference.total_seconds())
+
+        if sleep_amount:
+            if -2 < sleep_amount < 0:
+                # allow for up to a few seconds delay in us receiving/parsing etc
+                # but otherwise assume abnormality and just return if_unparsable
+                sleep_amount = 0
+            elif sleep_amount < 0:
+                lgr.warning(
+                    "date in Retry-After=%r is in the past (current is %r). "
+                    "Returning %r",
+                    retry_after,
+                    current_date,
+                    if_unparsable,
+                )
+                sleep_amount = if_unparsable
+            elif sleep_amount > 7 * 24 * 60 * 60:  # week
+                lgr.warning(
+                    "date in Retry-After=%r is over a week in the future (current is %r). "
+                    "Returning %r",
+                    retry_after,
+                    current_date,
+                    if_unparsable,
+                )
+                sleep_amount = if_unparsable
+    return sleep_amount