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

jacksonlee-civis · 2025-10-30T15:02:34Z

This pull request fixes a few issues related to retries:

There have been recent reports of a recursion error due to nesting "wait" strategies, when making Civis API calls under civis.APIClient. As a solution, instead of a single tenacity.Retrying instance for all API calls for a given civis.APIClient instance, each API call now has its own tenacity.Retrying instance. Since tenacity.Retrying is stateful (for attempt counts, etc.), this solution would also resolve yet-to-be-reported issues where retries would only work correctly for the first API calls but not subsequent ones for a given civis.APIClient instance.
Before this PR, the wait strategy was "use the retry-after seconds if known, else use the user-specified amount". When the amount of retry-after seconds is less than that from the user-specified strategy, the wait time would be less than what the user (or our default retryer) would require, which is not ideal. I've modified the code for "between retry-after and the user-specified one, wait at least the retry-after amount".
civis/io/_files.py had its own retrying strategy for a few helper functions. I've cleaned things up to use the same default retryer over these functions instead, and have written new unit tests for their retrying behavior.
Elsewhere in this codebase, there were other standalone HTTP calls via requests not yet covered by any retries. I've wrapped them up with the default retryer.

(For Civis employees only) Reference to a relevant ticket in the pull request title
Changelog entry added to CHANGELOG.md at the repo's root level
Description of change in the pull request description
If applicable, unit tests have been added and/or updated
The CircleCI builds have all passed

jacksonlee-civis · 2025-10-30T15:04:31Z

src/civis/io/_files.py

    # Store the current buffer position in case we need to retry below.
    buf_orig_position = buf.tell()

-    @_retry(RETRY_EXCEPTIONS)


Switching to the default retryer. New unit test written as test_file_single_upload_retries below for this helper function.

jacksonlee-civis · 2025-10-30T15:04:54Z

src/civis/io/_files.py

        raise ValueError(f"There are {num_parts} file parts but only {len(urls)} urls")

-    # upload function wrapped with a retry decorator
-    @_retry(RETRY_EXCEPTIONS)


Switching to the default retryer. New unit test written astest_file_multipart_upload_retries below for this helper function.

jacksonlee-civis · 2025-10-30T15:06:04Z

src/civis/io/_files.py

    # Store the current buffer position in case we need to retry below.
    buf_orig_position = buf.tell()

-    @_retry(RETRY_EXCEPTIONS)


Switching to the default retryer. The test suite has already had the unit test test_civis_to_file_retries for this helper function.

jacksonlee-civis · 2025-10-30T15:07:43Z

src/civis/_get_api_key.py

+import os
+
+
+def get_api_key(api_key):


This isn't new code. Moved this function here from _utils.py (now renamed _retries.py for clarity and not having a cluttered "utilities" module around).

jacksonlee-civis · 2025-10-30T15:08:56Z

src/civis/_retries.py

+
+    # If retries are exhausted,
+    # raise the last exception encountered, not tenacity's RetryError.
+    retrying.reraise = True


The docstring for civis.APIClient has also been updated to alert the end user about reraise being true if they provide their own retryer.

jacksonlee-civis · 2025-10-30T15:10:01Z

src/civis/_retries.py

    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)


The "fallback" idea doesn't seem right. I've modified code to "wait at least rety-after seconds, compared to the specified wait strategy" instead.

jacksonlee-civis · 2025-10-30T15:11:42Z

src/civis/client.pyi

This client.pyi is for type hints and is auto-generated/modified when civis_api_spec.json is updated (we run python tools/update_civis_api_spec.py locally to update these files together).

jacksonlee-civis · 2025-10-30T15:13:19Z

tests/test_retries.py

+@pytest.mark.parametrize(
+    "retrying, max_calls",
+    [
+        # Simulate a user-provided tenacity.Retrying with limited max attempts.
+        (_get_retrying(3), 3),
+        # No user-provided tenacity.Retrying. Use civis-python's default retrying.
+        (None, 10),
+    ],
+)
+@mock.patch("civis.futures.time.sleep", side_effect=lambda x: None)
+def test_no_recursion_in_retry_request(m_sleep, retrying, max_calls):


New unit test based on #524. I've modified the test code to test for both cases of a user-specified retryer and the default retryer.

Copilot

Pull Request Overview

This PR fixes several issues related to retries in the Civis Python client by refactoring how retry mechanisms work and standardizing retry usage across the codebase.

Fixed recursion error by creating new tenacity.Retrying instances for each API call instead of reusing a single stateful instance
Improved wait strategy to wait at least the Retry-After header duration rather than potentially less
Consolidated retry logic by removing custom retry implementation in _files.py and using the default retryer throughout

Reviewed Changes

Copilot reviewed 14 out of 15 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
tests/test_retries.py	Added test to verify recursion is avoided with new retry instances
tests/test_io.py	Added tests for retry behavior in file upload functions
src/civis/resources/_resources.py	Updated imports to use new retry module structure
src/civis/resources/_client_pyi.py	Added type hints for tenacity import and retries parameter
src/civis/io/_tables.py	Added retry wrapper for HTTP requests
src/civis/io/_files.py	Removed custom retry decorator and used default retryer
src/civis/client.pyi	Updated type annotations and API documentation
src/civis/client.py	Added type validation and documentation for retries parameter
src/civis/cli/_cli_commands.py	Added retry wrapper for notebook download requests
src/civis/cli/main.py	Updated import path for retry functions
src/civis/base.py	Updated to use new retry instance per request
src/civis/_retries.py	Major refactor of retry logic with new instance creation and improved wait strategy
src/civis/_get_api_key.py	Extracted API key functionality into separate module
.circleci/config.yml	Added security vulnerability ignore for pip-audit

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

src/civis/client.py

src/civis/_retries.py

tests/test_retries.py

mattyb

LGTM! Thanks for getting this fix in

jacksonlee-civis added 16 commits October 24, 2025 13:06

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

fbe7d65

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

d73feb9

ENH update civis api spec

ca8baf3

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

f985f3a

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

e142749

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

0c24763

ENH apply retries to remaining HTTP requests

2df86ff

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

832ccba

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

6a7c6d8

REF drop unused constant

bb82032

TST no recursion at retry_request

7ab3b1b

FIX retries in civis/io/_files.py

3515397

TST placeholders for more tests needed

f9ca80c

TST retries on _single_upload and _multipart_upload

4b2f4e1

ENH wait at least retry-after seconds

1267387

TST fix file read behavior for windows test

a7c89e0

jacksonlee-civis commented Oct 30, 2025

View reviewed changes

jacksonlee-civis requested a review from Copilot October 30, 2025 15:14

Copilot AI reviewed Oct 30, 2025

View reviewed changes

src/civis/client.py Show resolved Hide resolved

src/civis/_retries.py Show resolved Hide resolved

tests/test_retries.py Show resolved Hide resolved

FIX import tenacity

cc3f0b7

jacksonlee-civis requested a review from mattyb October 30, 2025 15:25

MAINT update changelog

ac5e82a

mattyb approved these changes Oct 31, 2025

View reviewed changes

jacksonlee-civis merged commit d829708 into main Oct 31, 2025
2 checks passed

jacksonlee-civis deleted the civis-11753-new-retrying-instance-for-each-api-call branch October 31, 2025 17:22

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

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

Uh oh!

Conversation

jacksonlee-civis commented Oct 30, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mattyb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants