Skip to content

fix: add retry logic for 429 http codes#125

Open
flz wants to merge 2 commits intomasterfrom
fix-429
Open

fix: add retry logic for 429 http codes#125
flz wants to merge 2 commits intomasterfrom
fix-429

Conversation

@flz
Copy link
Copy Markdown
Owner

@flz flz commented Mar 27, 2026

Retry with exponential backoff when the server returns 429 Too Many
Requests. Addresses throttling when multiple systems are polled
concurrently.

  • add AqualinkServiceThrottledException raised when retries exhausted
  • add retry constants (RETRY_MAX_ATTEMPTS, RETRY_BASE_DELAY, RETRY_MAX_DELAY)
  • retry loop in send_request() with Retry-After header support and
    exponential backoff with jitter; only 429 is retried
  • log all 429 response headers at DEBUG for rate-limit header discovery
  • fix mutation of module-level AQUALINK_HTTP_HEADERS dict
  • add unit tests for retry, Retry-After, exhaustion, and non-retry codes

@claude
Copy link
Copy Markdown

claude bot commented Mar 27, 2026

Code Review

Overall this is a solid, well-structured addition. The implementation is clean and the test coverage is good. A few observations:


Bug: Unnecessary sleep on final retry attempt

In send_request, the retry loop sleeps before continue on every 429 response, including the last attempt (attempt == RETRY_MAX_ATTEMPTS - 1). After that final sleep the loop exits and raises AqualinkServiceThrottledException immediately, meaning the caller waits up to RETRY_MAX_DELAY seconds for no reason.

# current behaviour – sleeps even when we are about to give up
for attempt in range(RETRY_MAX_ATTEMPTS):
    ...
    if r.status_code == httpx.codes.TOO_MANY_REQUESTS:
        delay = self._get_retry_delay(r, attempt)
        await asyncio.sleep(delay)   # pointless on the last iteration
        continue

raise AqualinkServiceThrottledException(...)

One idiomatic fix:

for attempt in range(RETRY_MAX_ATTEMPTS):
    ...
    if r.status_code == httpx.codes.TOO_MANY_REQUESTS:
        if attempt < RETRY_MAX_ATTEMPTS - 1:
            delay = self._get_retry_delay(r, attempt)
            LOGGER.warning(...)
            await asyncio.sleep(delay)
        continue

raise AqualinkServiceThrottledException(...)

The test test_429_retries_exhausted currently asserts mock_sleep.call_count == RETRY_MAX_ATTEMPTS (5 sleeps for 5 attempts), which confirms this extra sleep is happening today.


Minor: Retry-After as an HTTP-date is silently ignored

RFC 7231 allows Retry-After to be either a delay-in-seconds or an HTTP-date string (e.g. Retry-After: Wed, 21 Oct 2026 07:28:00 GMT). The current _get_retry_delay catches the ValueError from float() and falls back to exponential backoff, which is safe. A LOGGER.debug noting that the date format was not parsed would make debugging easier in the field:

except ValueError:
    LOGGER.debug("Could not parse Retry-After header as seconds: %s", retry_after)

Good fixes / things to keep

  • AQUALINK_HTTP_HEADERS.copy() — nice catch; the original code mutated the module-level dict on every request.
  • Exponential backoff with full-jitter (+ random.random()) is the right choice.
  • Retry-After header is honoured and capped at RETRY_MAX_DELAY.
  • New exception (AqualinkServiceThrottledException) extends AqualinkServiceException, so callers that catch the base class continue to work.
  • Constants extracted to const.py — easy to tune without touching logic.
  • Test suite covers the key cases (success after retry, Retry-After header, exhaustion, 500/401 not retried).

Nit: missing test case

A test for a non-numeric Retry-After value (e.g. an HTTP-date string) would ensure the fallback to exponential backoff is exercised explicitly.


Overall: approve with the sleep-on-last-attempt fix addressed.

flz added 2 commits March 27, 2026 07:55
Retry with exponential backoff when the server returns 429 Too Many
Requests. Addresses throttling when multiple systems are polled
concurrently.

- add AqualinkServiceThrottledException raised when retries exhausted
- add retry constants (RETRY_MAX_ATTEMPTS, RETRY_BASE_DELAY, RETRY_MAX_DELAY)
- retry loop in send_request() with Retry-After header support and
  exponential backoff with jitter; only 429 is retried
- log all 429 response headers at DEBUG for rate-limit header discovery
- fix mutation of module-level AQUALINK_HTTP_HEADERS dict
- add unit tests for retry, Retry-After, exhaustion, and non-retry codes
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.

1 participant