[CIVIS-11753] FIX retries: recursion error and wait strategy (#525)

* ENH verify user-provided retrying is a tenacity.Retrying instance

* FIX use a new tenacity.Retrying instance in each API call

* ENH update civis api spec

* SEC ignore GHSA-4xh5-x5gv-qwph for pip

* ENH wait strategy to add base on top of retry-after

* REF break out _utils.py into _retries.py and _get_api_key.py

* ENH apply retries to remaining HTTP requests

* ENH re-raise the original exception, not tenacity's RetryError

* REF use the defined retrying strategy in civis/io/_files.py

* REF drop unused constant

* TST no recursion at retry_request

* FIX retries in civis/io/_files.py

* TST placeholders for more tests needed

* TST retries on _single_upload and _multipart_upload

* ENH wait at least retry-after seconds

* TST fix file read behavior for windows test

* FIX import tenacity

* MAINT update changelog

Author: jacksonlee-civis

.circleci/config.yml

@@ -94,7 +94,8 @@ workflows:
             pip install --progress-bar off -r docs/requirements.txt && \
             pip install --progress-bar off -e ".[dev-core,dev-civisml]" && \
             pip-audit --version && \
-            pip-audit --skip-editable
+            pip-audit --skip-editable \
+              --ignore-vuln GHSA-4xh5-x5gv-qwph  # https://github.com/pypa/pip/issues/13607
       - pre-build:
           name: twine
           command-run: |

CHANGELOG.md

@@ -9,13 +9,17 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 ### Added
 
 ### Changed
+- Updated retries for Civis API calls under `civis.APIClient`
+  to wait at least the `Retry-After` duration from the response header,
+  instead of potentially less than that. (#525)
 
 ### Deprecated
 
 ### Removed
 
 ### Fixed
-
+- Fixed retries for Civis API calls under `civis.APIClient`
+  that would lead to a recursion error. (#525)
 - Updated documentation for the `preview_rows` argument of `civis.io.query_civis` to
   have the correct maximum of 1000. (#523)
 

src/civis/_get_api_key.py

@@ -0,0 +1,16 @@
+import os
+
+
+def get_api_key(api_key):
+    """Pass-through if `api_key` is not None otherwise tries the CIVIS_API_KEY
+    environment variable.
+    """
+    if api_key is not None:  # always prefer user given one
+        return api_key
+    api_key = os.environ.get("CIVIS_API_KEY", None)
+    if api_key is None:
+        raise EnvironmentError(
+            "No Civis API key found. Please store in "
+            "CIVIS_API_KEY environment variable"
+        )
+    return api_key

src/civis/_utils.py renamed to src/civis/_retries.py

@@ -1,5 +1,4 @@
 import logging
-import os
 
 import tenacity
 from tenacity.wait import wait_base
@@ -19,36 +18,32 @@
     wait=tenacity.wait_random_exponential(multiplier=2, max=60),
     stop=(tenacity.stop_after_delay(600) | tenacity.stop_after_attempt(10)),
     retry_error_callback=lambda retry_state: retry_state.outcome.result(),
+    reraise=True,
 )
 """
 
-# Explicitly set the available globals and locals
-# to mitigate risk of unwanted code execution
-DEFAULT_RETRYING = eval(  # nosec
-    DEFAULT_RETRYING_STR,
-    {"tenacity": tenacity, "__builtins__": {}},  # globals
-    {},  # locals
-)
-
 
-def get_api_key(api_key):
-    """Pass-through if `api_key` is not None otherwise tries the CIVIS_API_KEY
-    environment variable.
-    """
-    if api_key is not None:  # always prefer user given one
-        return api_key
-    api_key = os.environ.get("CIVIS_API_KEY", None)
-    if api_key is None:
-        raise EnvironmentError(
-            "No Civis API key found. Please store in "
-            "CIVIS_API_KEY environment variable"
-        )
-    return api_key
+def get_default_retrying():
+    """Return a new instance of the default tenacity.Retrying."""
+    # Explicitly set the available globals and locals
+    # to mitigate risk of unwanted code execution
+    return eval(  # nosec
+        DEFAULT_RETRYING_STR,
+        {"tenacity": tenacity, "__builtins__": {}},  # globals
+        {},  # locals
+    )
 
 
 def retry_request(method, prepared_req, session, retrying=None):
     retry_conditions = None
-    retrying = retrying if retrying else DEFAULT_RETRYING
+
+    # New tenacity.Retrying instance needed, whether it's a copy of the user-provided
+    # one or it's one based on civis-python's default settings.
+    retrying = retrying.copy() if retrying else get_default_retrying()
+
+    # If retries are exhausted,
+    # raise the last exception encountered, not tenacity's RetryError.
+    retrying.reraise = True
 
     def _make_request(req, sess):
         """send the prepared session request"""
@@ -66,33 +61,31 @@ def _make_request(req, sess):
 
     if retry_conditions:
         retrying.retry = retry_conditions
-        retrying.wait = wait_for_retry_after_header(fallback=retrying.wait)
+        retrying.wait = wait_at_least_retry_after_header(base=retrying.wait)
         response = retrying(_make_request, prepared_req, session)
         return response
 
     response = _make_request(prepared_req, session)
     return response
 
 
-class wait_for_retry_after_header(wait_base):
-    """Wait strategy that first looks for Retry-After header. If not
-    present it uses the fallback strategy as the wait param"""
+class wait_at_least_retry_after_header(wait_base):
+    """Wait strategy for at least `Retry-After` seconds (if present from header)"""
 
-    def __init__(self, fallback):
-        self.fallback = fallback
+    def __init__(self, base):
+        self.base = base
 
     def __call__(self, retry_state):
         # retry_state is an instance of tenacity.RetryCallState.
         # The .outcome property contains the result/exception
         # that came from the underlying function.
-        result_headers = retry_state.outcome._result.headers
-        retry_after = result_headers.get("Retry-After") or result_headers.get(
-            "retry-after"
-        )
+        headers = retry_state.outcome._result.headers
 
         try:
-            log.info("Retrying after {} seconds".format(retry_after))
-            return int(retry_after)
+            retry_after = float(
+                headers.get("Retry-After") or headers.get("retry-after") or "0.0"
+            )
         except (TypeError, ValueError):
-            pass
-        return self.fallback(retry_state)
+            retry_after = 0.0
+        # Wait at least retry_after seconds (compared to the user-specified wait).
+        return max(retry_after, self.base(retry_state))

src/civis/base.py

@@ -10,7 +10,7 @@
 
 import civis
 from civis.response import PaginatedResponse, convert_response_data_type
-from civis._utils import retry_request, DEFAULT_RETRYING
+from civis._retries import retry_request
 
 FINISHED = ["success", "succeeded"]
 FAILED = ["failed"]
@@ -159,16 +159,13 @@ def _make_request(self, method, path=None, params=None, data=None, **kwargs):
         params = self._handle_array_params(params)
 
         with self._lock:
-            if self._client._retrying is None:
-                retrying = self._session_kwargs.pop("retrying", None)
-                self._client._retrying = retrying if retrying else DEFAULT_RETRYING
             with open_session(self._session_kwargs["api_key"], self._headers) as sess:
                 request = requests.Request(
                     method, url, json=data, params=params, **kwargs
                 )
                 pre_request = sess.prepare_request(request)
                 response = retry_request(
-                    method, pre_request, sess, self._client._retrying
+                    method, pre_request, sess, self._session_kwargs["retrying"]
                 )
 
         if response.status_code == 401:

src/civis/cli/__main__.py

@@ -41,7 +41,7 @@
 from civis.base import get_headers, open_session
 from civis.resources import get_api_spec, CACHED_SPEC_PATH
 from civis.resources._resources import parse_method_name
-from civis._utils import retry_request
+from civis._retries import retry_request
 
 
 _REPLACEABLE_COMMAND_CHARS = re.compile(r"[^A-Za-z0-9]+")

src/civis/cli/_cli_commands.py

@@ -14,6 +14,7 @@
 import civis
 from civis.io import file_to_civis, civis_to_file
 from civis.utils import job_logs
+from civis._retries import get_default_retrying
 
 
 # From http://patorjk.com/software/taag/#p=display&f=3D%20Diagonal&t=CIVIS
@@ -243,8 +244,10 @@ def notebooks_download_cmd(notebook_id, path):
     """Download a notebook to a specified local path."""
     client = civis.APIClient()
     info = client.notebooks.get(notebook_id)
-    response = requests.get(info["notebook_url"], stream=True, timeout=60)
-    response.raise_for_status()
+    for attempt in get_default_retrying():
+        with attempt:
+            response = requests.get(info["notebook_url"], stream=True, timeout=60)
+            response.raise_for_status()
     chunk_size = 32 * 1024
     chunked = response.iter_content(chunk_size)
     with open(path, "wb") as f:

src/civis/client.py

@@ -1,20 +1,19 @@
 from __future__ import annotations
 
+import collections
 from functools import lru_cache
 import logging
 import textwrap
 import warnings
-from typing import TYPE_CHECKING
+
+import tenacity
 
 import civis
 from civis.resources import generate_classes_maybe_cached
-from civis._utils import get_api_key, DEFAULT_RETRYING_STR
+from civis._get_api_key import get_api_key
+from civis._retries import DEFAULT_RETRYING_STR
 from civis.response import _RETURN_TYPES, find, find_one
 
-if TYPE_CHECKING:
-    import collections
-    import tenacity
-
 
 _log = logging.getLogger(__name__)
 
@@ -59,6 +58,9 @@ class APIClient:
         please note that you should leave the ``retry`` attribute unspecified,
         because the conditions under which retries apply are pre-determined
         -- see :ref:`retries` for details.
+        In addition, the ``reraise`` attribute will be overridden to ``True``
+        to raise the last exception (rather than tenacity's `RetryError`)
+        if all retry attempts are exhausted.
     user_agent : str, optional
         A custom user agent string to use for requests made by this client.
         The user agent string will be appended with the Python version,
@@ -81,6 +83,11 @@ def __init__(
             )
         self._feature_flags = ()
         session_auth_key = get_api_key(api_key)
+        if retries is not None and not isinstance(retries, tenacity.Retrying):
+            raise TypeError(
+                "If provided, the `retries` parameter must be "
+                "a tenacity.Retrying instance."
+            )
         self._session_kwargs = {
             "api_key": session_auth_key,
             "retrying": retries,
@@ -102,11 +109,6 @@ def __init__(
                 cls(self._session_kwargs, client=self, return_type=return_type),
             )
 
-        # Don't create the `tenacity.Retrying` instance until we make the first
-        # API call with this `APIClient` instance.
-        # Once that happens, we keep re-using this `tenacity.Retrying` instance.
-        self._retrying = None
-
     @property
     def feature_flags(self):
         """The feature flags for the current user.