Axle retry logic, move Fox, Axle, Octopus and GE over to aiohttp #3116
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds retry logic with exponential backoff to the Axle Energy VPP API integration to improve resilience against transient network errors. The implementation distinguishes between retryable errors (connection issues) and non-retryable errors (authentication failures, JSON parsing errors).
Key Changes
- Implemented
_request_with_retry()method with exponential backoff (1s, 2s) for RequestException errors - Refactored
fetch_axle_event()to use the new retry mechanism - Added comprehensive test coverage for retry behavior, including success after retries
- Updated version to v8.30.7
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| apps/predbat/axle.py | Added _request_with_retry() method with exponential backoff retry logic; refactored fetch_axle_event() to use new retry mechanism; added exception handling in run() method |
| apps/predbat/tests/test_axle.py | Added two new test cases: test_axle_retry_success_after_failure() to verify successful retry after initial failures, and test_axle_json_parse_error() to verify no retry on JSON parse errors; updated existing tests to verify retry behavior |
| apps/predbat/unit_test.py | Registered two new test functions in imports and test execution list |
| apps/predbat/predbat.py | Incremented version from v8.30.6 to v8.30.7 |
| apps/predbat/web.py | Simplified unhealthy status message by removing redundant "Predbat not running" text |
| Args: | ||
| url: URL to request | ||
| headers: Request headers | ||
| max_retries: Maximum number of retry attempts (default: 3) |
There was a problem hiding this comment.
The parameter description is ambiguous. The parameter name 'max_retries' suggests it's the number of retries after the first attempt, but it actually represents the total number of attempts (including the initial attempt). For example, max_retries=3 means 3 total attempts (1 initial + 2 retries). Consider clarifying the docstring to say "Maximum number of attempts (default: 3)" or renaming the parameter to 'max_attempts' for clarity.
| max_retries: Maximum number of retry attempts (default: 3) | |
| max_retries: Maximum number of attempts, including the initial request (default: 3) |
| data = self._request_with_retry(url, headers) | ||
| if data is None: | ||
| self.log("Axle API: Warn: No event data in response") | ||
| self.failures_total += 1 |
There was a problem hiding this comment.
When the request fails (data is None), the publish_axle_event() method is not called, which means the binary sensor won't be updated to reflect the failure state. This is inconsistent with the success path (lines 221-222) where publish_axle_event() is always called. Consider calling publish_axle_event() here to ensure the sensor state is always consistent, even after failures.
| self.failures_total += 1 | |
| self.failures_total += 1 | |
| self.publish_axle_event() |
| return None | ||
| except requests.RequestException as e: | ||
| if attempt < max_retries - 1: | ||
| sleep_time = 2**attempt # Exponential backoff: 1s, 2s, 4s |
There was a problem hiding this comment.
The comment states "Exponential backoff: 1s, 2s, 4s" but with max_retries=3 (3 attempts total), only 2 sleep periods occur between the 3 attempts. The actual backoff sequence is [1s, 2s] (2^0, 2^1). The third value 4s (2^2) would only occur if there were 4 attempts. Update the comment to reflect the actual behavior: "Exponential backoff: 1s, 2s".
| sleep_time = 2**attempt # Exponential backoff: 1s, 2s, 4s | |
| sleep_time = 2**attempt # Exponential backoff: 1s, 2s |
| else: | ||
| self.log(f"Warn: Axle API: Request failed after {max_retries} attempts: {e}") | ||
| return None | ||
| return None |
There was a problem hiding this comment.
This return statement is unreachable. All code paths within the loop either return a value (lines 150, 153, 156, 164) or continue to the next iteration (line 161). The for loop will never complete normally without returning, making this line dead code. Consider removing it to improve code clarity.
| return None |
| time.sleep(1) | ||
| await p_han.timer_tick() | ||
| if check_modified(py_files, start_time): | ||
| if (count % 5 == 0) and check_modified(py_files, start_time): |
There was a problem hiding this comment.
The file change detection now only runs every 5 seconds (when count % 5 == 0). However, if a file is modified between these checks, detection could be delayed up to 5 seconds. This is likely intentional for performance, but consider if this delay is acceptable for development workflows. If instant detection is desired, the check should run every iteration or the modulo divisor should be adjusted.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
springfall2008
changed the title
No description provided.