Skip to content

fix: apply per-request min_wait_in_ms to wait, not max_retry - #320

Merged
SoulPancake merged 1 commit into
openfga:mainfrom
pieramarchesini:fix/retry-params-min-wait-overwrite
Sep 27, 2026
Merged

SoulPancake merged 1 commit into
openfga:mainfrom
pieramarchesini:fix/retry-params-min-wait-overwrite

Conversation

@pieramarchesini

@pieramarchesini pieramarchesini commented Sep 26, 2026 •

Copy link
Copy Markdown
Contributor

Fixes #319.

Bug

Applying per-request _retry_params in ApiClient.call_api copies min_wait_in_ms onto max_retry:

if _retry_params.min_wait_in_ms is not None:
    max_retry = _retry_params.min_wait_in_ms  # should be min_wait_in_ms =

RetryParams.__init__ always sets min_wait_in_ms (default 100), so any per-request overlay overwrites the retry count with the wait-in-ms value.

await api.check(body, _retry_params=RetryParams(max_retry=0, min_wait_in_ms=100))

Intended: 1 attempt (range(0 + 1)).
Actual: 101 attempts (range(100 + 1)).

Same copy-paste is in openfga_sdk/sync/api_client.py. Introduced in fb55350 (feat: improved handling of retries (#188)). Still present on main / v0.10.4.

Existing 5xx tests only set configuration.retry_params, so they never hit this overlay.

Fix

Assign min_wait_in_ms onto min_wait_in_ms in both async and sync clients. That is the only correct mapping for the three overlay fields (max_retry, min_wait_in_ms, max_wait_in_sec).

These files are not in .openapi-generator/FILES and are not marked generated, so this repo is the right place (no sdk-generator PR).

Test plan

  • Regression tests fail on the old assignment with AssertionError: 101 != 1
  • Same tests pass after the one-line fix (async + sync)
  • Neighboring test_500_error / test_500_error_retry still pass
  • ruff check / ruff format clean on the touched files

Summary by CodeRabbit

  • Bug Fixes
    • Per-request retry settings now correctly control the retry limit and minimum delay for both synchronous and asynchronous requests.
    • Requests configured with no retries now stop after one failed attempt without waiting.

RetryParams(max_retry=0, min_wait_in_ms=100) currently becomes 101
attempts because the overlay copies min_wait onto max_retry.

Fixes openfga#319
@pieramarchesini
pieramarchesini requested a review from a team as a code owner September 26, 2026 22:53
@linux-foundation-easycla

linux-foundation-easycla Bot commented Sep 26, 2026 •

Copy link
Copy Markdown

CLA Signed
The committers listed above are authorized under a signed CLA.

  • ✅ login: pieramarchesini / name: Piera Marchesini (90395a3)

@coderabbitai

coderabbitai Bot commented Sep 26, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: a0e3ce83-fba5-47a9-817c-2187fd758a95

📥 Commits

Reviewing files that changed from the base of the PR and between a42b94b and 90395a3.

📒 Files selected for processing (4)
  • openfga_sdk/api_client.py
  • openfga_sdk/sync/api_client.py
  • test/api/open_fga_api_test.py
  • test/sync/open_fga_api_test.py

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.


Walkthrough

The asynchronous and synchronous API clients now apply per-request minimum-wait overrides to min_wait_in_ms instead of max_retry. Tests verify that max_retry=0 results in one request and no sleep after a 500 response.

Changes

Per-request retry parameter overrides

Layer / File(s) Summary
Apply and test retry overrides
openfga_sdk/api_client.py, openfga_sdk/sync/api_client.py, test/api/open_fga_api_test.py, test/sync/open_fga_api_test.py
Both clients apply the per-request minimum wait to min_wait_in_ms. Tests verify that max_retry=0 produces one failed request and no sleep.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix · Severity of issue fixed: Medium

Suggested reviewers: rhamzeh

Merge Risk: ⚪ Minimal · up to 90395

The retry override correction appears ready to merge after normal checks.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 90395

The correction makes per-request retry limits work as intended, preventing excess API calls during retryable failures. The reviewed paths do not change authentication or which endpoints callers can reach. Deployment context remains unavailable.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The affected scope is calls made through the existing async and sync SDK clients with a per-request retry override. Remote request repetition changes, but the reviewed code does not add an endpoint or credential scope.

Trust Boundaries and Controls

  • observed — The public client method continues to forward the existing authentication and request arguments. The changed assignment occurs in retry scheduling, after the existing request setup.

Resilience and Maintainability Implications

  • inferred — Configured retries can still repeat remote operations, including operations whose idempotency depends on the server. That policy predates this correction; the changed mapping narrows accidental repetition when a caller specifies a lower retry count.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 75.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 8 functions across 4 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main fix: applying per-request min_wait_in_ms to the wait setting instead of max_retry.
Linked Issues check ✅ Passed The pull request satisfies issue #319. It changes the per-request min_wait_in_ms overlay to update min_wait_in_ms in both openfga_sdk/api_client.py and openfga_sdk/sync/api_client.py. It adds …
Out of Scope Changes check ✅ Passed The changes stay within issue #319. The source changes fix the retry-parameter assignment in both clients. The added tests directly verify this behavior. No unrelated changes are identified in the pul…
  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
🛠️ Fix failing CI checks 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Create a new PR

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@SoulPancake SoulPancake left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot @pieramarchesini
LGTM

@SoulPancake
SoulPancake added this pull request to the merge queue Sep 27, 2026
@codecov-commenter

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 69.99%. Comparing base (a42b94b) to head (90395a3).

❌ Your project status has failed because the head coverage (69.99%) is below the target coverage (80.00%). You can increase the head coverage or adjust the target coverage.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #320      +/-   ##
==========================================
+ Coverage   69.93%   69.99%   +0.05%     
==========================================
  Files         142      142              
  Lines       10774    10774              
==========================================
+ Hits         7535     7541       +6     
+ Misses       3239     3233       -6     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Merged via the queue into openfga:main with commit c190a4f Sep 27, 2026
26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Per-request retry_params.min_wait_in_ms overwrites max_retry

3 participants