Fix #36 and #73 (v4.4.5) - #74
Conversation
📝 WalkthroughWalkthroughChangesThe retry APIs now propagate original positional and keyword arguments into retry states and handler details. Synchronous timeout executors now shut down on success, timeout, and exception paths. Regression tests cover retry variants, generators, handlers, and executor cleanup. Retry corrections
Estimated code review effort: 3 (Moderate) | ~25 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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. Comment |
9ed3c33 to
ce78cee
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
ce78cee to
72231e2
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
pyproject.toml (1)
84-84: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep type checking aligned with the minimum supported Python version.
The package declares
requires-python = ">=3.9", but Line 84 makes mypy model Python 3.13. This can allow standard-library APIs that fail on Python 3.9.Set the target to Python 3.9, or add a separate Python 3.9 mypy check in CI. Verify locally with
mypy --python-version 3.9 backon/.Proposed change
-python_version = "3.13" +python_version = "3.9"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pyproject.toml` at line 84, Update the mypy configuration’s python_version setting from 3.13 to the package’s minimum supported version, 3.9, so type checking rejects APIs unavailable on Python 3.9. Verify the result with mypy --python-version 3.9 against the backon package.tests/test_issues_36_73.py (1)
28-33: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the thread-count polling loop into a helper.
The same six-line polling block appears four times in this class, at lines 28-33, 52-57, 75-80, and 101-106. Extract it into one module-level helper and call it from each test.
Note also that
threading.active_count()counts threads for the whole process. If other tests start threads concurrently, the<= baselineassertion can fail for unrelated reasons. The polling window reduces this risk but does not remove it.♻️ Proposed helper
def _wait_for_thread_baseline(baseline: int, timeout: float = 1.0) -> None: deadline = time.monotonic() + timeout while time.monotonic() < deadline: if threading.active_count() <= baseline: return time.sleep(0.01)Then each test ends with:
- deadline = time.monotonic() + 1.0 - while time.monotonic() < deadline: - if threading.active_count() <= baseline: - break - time.sleep(0.01) - assert threading.active_count() <= baseline + _wait_for_thread_baseline(baseline) + assert threading.active_count() <= baseline🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_issues_36_73.py` around lines 28 - 33, Extract the repeated thread-count polling logic from the tests in this class into a module-level _wait_for_thread_baseline helper accepting baseline and optional timeout parameters. Have it return once threading.active_count() reaches the baseline or the timeout expires, then replace all four duplicated loops with helper calls and retain each test’s final baseline assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backon/_typing.py`:
- Around line 9-11: Remove the inline “# pragma: no cover” comment from the
ImportError branch of the ParamSpec import fallback in _typing.py, leaving the
existing import and fallback behavior unchanged.
In `@tests/test_coverage_v4.py`:
- Around line 536-609: Add a per-test execution counter to each empty
async-generator fixture and assert the expected invocation count for all enabled
and disabled on_predicate and on_exception paths, alongside the existing
empty-result assertions. Ensure the enabled on_predicate case reflects whether
empty output should retry; if it must execute only once, adjust the
implementation or predicate configuration and keep the regression assertion
aligned with that behavior.
In `@tests/test_issues_36_73.py`:
- Around line 65-73: Update the test’s RetryingCaller setup to assign the result
of caller.on(ValueError) back to caller, preserving the configured exception
retry condition, and pass interval=0 to backon.constant so all four attempts run
without sleeping. Keep the existing cleanup-path assertions and test structure
unchanged.
---
Nitpick comments:
In `@pyproject.toml`:
- Line 84: Update the mypy configuration’s python_version setting from 3.13 to
the package’s minimum supported version, 3.9, so type checking rejects APIs
unavailable on Python 3.9. Verify the result with mypy --python-version 3.9
against the backon package.
In `@tests/test_issues_36_73.py`:
- Around line 28-33: Extract the repeated thread-count polling logic from the
tests in this class into a module-level _wait_for_thread_baseline helper
accepting baseline and optional timeout parameters. Have it return once
threading.active_count() reaches the baseline or the timeout expires, then
replace all four duplicated loops with helper calls and retain each test’s final
baseline assertion.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5488524f-8cb1-4b42-b85f-b3b2ea74ec97
📒 Files selected for processing (12)
CHANGELOG.mdbackon/_decorator.pybackon/_hedging.pybackon/_retry/_api.pybackon/_retry/_classes.pybackon/_retry/_fast.pybackon/_retry/_inner.pybackon/_retry/_loops.pybackon/_typing.pypyproject.tomltests/test_coverage_v4.pytests/test_issues_36_73.py
| try: | ||
| from typing import ParamSpec | ||
| else: | ||
| except ImportError: # pragma: no cover |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove the new inline comment.
Line 11 adds # pragma: no cover. The backon/**/*.py rules require zero comments.
As per coding guidelines, “Use zero comments.”
Proposed change
-except ImportError: # pragma: no cover
+except ImportError:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try: | |
| from typing import ParamSpec | |
| else: | |
| except ImportError: # pragma: no cover | |
| try: | |
| from typing import ParamSpec | |
| except ImportError: |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backon/_typing.py` around lines 9 - 11, Remove the inline “# pragma: no
cover” comment from the ImportError branch of the ParamSpec import fallback in
_typing.py, leaving the existing import and fallback behavior unchanged.
Source: Coding guidelines
…ndler details args/kwargs #36: ThreadPoolExecutor leaked worker threads whenever the target raised an exception (not a timeout) under attempt_timeout, because shutdown() was only called on the timeout path. _retry_loop_sync now wraps the executor in try/finally so shutdown(wait=False) runs on every path. #73: Handler details dicts always had empty args/kwargs because the real call arguments were never threaded into RetryState/RetryCallState. Args and kwargs are now plumbed from every entry point (on_exception/ on_predicate sync+async, sync/async generators, Retrying.call/ async_call, RetryingCaller/AsyncRetryingCaller, hedge) into the retry loop, so handlers receive the actual call arguments. Version 4.4.5.
72231e2 to
41e02e9
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_issues_36_73.py`:
- Around line 10-16: Update _wait_for_thread_baseline to enforce the 100ms
test-time budget by reducing its default timeout below 100ms and using a
correspondingly smaller polling interval; preserve the existing baseline check
and boolean return behavior.
- Around line 46-54: Extend the tests around the existing retry case to cover
both the backon.retry entry point and the hedging entry point with target
positional and keyword arguments. Assert in the resulting callback details that
details["args"] and details["kwargs"] preserve exactly the arguments supplied to
the target.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a1c50527-38ec-4ff0-b790-8f00b1ac61b2
📒 Files selected for processing (12)
CHANGELOG.mdbackon/_decorator.pybackon/_hedging.pybackon/_retry/_api.pybackon/_retry/_classes.pybackon/_retry/_fast.pybackon/_retry/_inner.pybackon/_retry/_loops.pybackon/_typing.pypyproject.tomltests/test_coverage_v4.pytests/test_issues_36_73.py
🚧 Files skipped from review as they are similar to previous changes (11)
- backon/_typing.py
- backon/_decorator.py
- backon/_hedging.py
- pyproject.toml
- backon/_retry/_inner.py
- backon/_retry/_fast.py
- backon/_retry/_loops.py
- backon/_retry/_api.py
- CHANGELOG.md
- tests/test_coverage_v4.py
- backon/_retry/_classes.py
| def _wait_for_thread_baseline(baseline, timeout=1.0): | ||
| deadline = time.monotonic() + timeout | ||
| while time.monotonic() < deadline: | ||
| if threading.active_count() <= baseline: | ||
| break | ||
| time.sleep(0.01) | ||
| return threading.active_count() <= baseline |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Keep the cleanup regression within the test-time budget.
At Line 10, timeout=1.0 permits a failing test to wait for one second. This exceeds the required 100ms test budget. Reduce the deadline and polling interval, or observe executor shutdown directly without waiting for thread termination.
As per coding guidelines: “Keep tests under 100ms, use jitter=None and small intervals.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_issues_36_73.py` around lines 10 - 16, Update
_wait_for_thread_baseline to enforce the 100ms test-time budget by reducing its
default timeout below 100ms and using a correspondingly smaller polling
interval; preserve the existing baseline check and boolean return behavior.
Source: Coding guidelines
| backon.retry( | ||
| fn, | ||
| backon.constant, | ||
| exception=ValueError, | ||
| max_tries=4, | ||
| interval=0, | ||
| jitter=None, | ||
| attempt_timeout=5.0, | ||
| ) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline backon/_retry/_api.py --items all
ast-grep outline backon/_hedging.py --items all
rg -n -C 5 --type py '\b(backon\.)?retry\s*\(|\bhedg\w*\s*\(' tests
rg -n -C 5 --type py 'details\["(args|kwargs)"\]|call_state\.(args|kwargs)' testsRepository: Llucs/backon
Length of output: 49242
Add functional/hedging regression for target argument propagation.
The decorator coverage here does not exercise backon.retry or the hedging entry point with target positional/keyword arguments, but the argument-propagation change affects backon/_retry/_api.py and backon/_hedging.py. Add coverage that calls those entry points with positional and keyword arguments and asserts details["args"] and details["kwargs"].
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_issues_36_73.py` around lines 46 - 54, Extend the tests around the
existing retry case to cover both the backon.retry entry point and the hedging
entry point with target positional and keyword arguments. Assert in the
resulting callback details that details["args"] and details["kwargs"] preserve
exactly the arguments supplied to the target.
Source: Coding guidelines
1 participant
Fixes #36 — ThreadPoolExecutor leak on the exception path when
attempt_timeoutis set (try/finally around the executor).Fixes #73 — handler
detailsdict always had emptyargs/kwargs; real call arguments are now plumbed from every entry point intoRetryState/RetryCallStateso all handlers receive them.Version bumped to 4.4.5; CHANGELOG updated. ruff + mypy pass locally.
Summary by CodeRabbit