Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7f7a47b30f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
|
||
| def _exponential(base: float, failures: int) -> float: | ||
| """Return ``base * 2**failures`` without overflowing for large ``failures``.""" | ||
| return base * 2 ** min(failures, _MAX_DOUBLINGS) |
There was a problem hiding this comment.
Derive the clamp from the configured base
When callers configure a very small positive base, 1023 doublings do not necessarily reach the cap, so this fixed clamp permanently stops the exponential growth. For example, ExponentialBackoff(cap=0.512, base=1e-310) returns about 0.00899 for every failure count from 1023 onward, although the intended sequence reaches 0.512 around failure 1029. Use overflow-safe scaling or clamp only after determining that the configured base has saturated the cap.
AGENTS.md reference: AGENTS.md:L159-L163
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 6e012e7. The exponent is no longer clamped: _exponential now uses math.ldexp(base, failures), which only overflows when the result itself does, and returns infinity then so every strategy caps it. ExponentialBackoff(cap=0.512, base=1e-310) keeps growing past failure 1023 and reaches the cap at 1029, with a test covering it.
|
Hey @sachhg, thank you for your contribution! I'll have a look at it soon. |
The problem
Retryis documented to retry forever when given a negative retry count:and the loop honors that by skipping the bound entirely:
So
failuresis unbounded, andcomputeis eventually asked for a delay after a very large number of consecutive failures. Every exponential strategy computes the delay as2**failuresis an arbitrary precision int, and multiplying one by a float converts it first, which raisesOverflowError: int too large to convert to floatonce the value leaves the float range. The cap is applied to the product, so it never gets the chance to prevent this.The threshold is exactly 1024 failures, and it affects
ExponentialBackoff,FullJitterBackoff,EqualJitterBackoffandExponentialWithJitterBackoff.Why it matters
A client set up to retry forever is the configuration most likely to reach it, and the delay is capped long before then, so the failure count grows at a steady rate. With the default cap of 0.512s that is roughly nine minutes of a server being unreachable; a restart, a failover, or a network partition of that length is ordinary.
At that point the client does not keep retrying and it does not surface a Redis error either.
OverflowErroris not aRedisError, so it passes straight throughexcept redis.RedisErrorhandlers and reaches the caller as something that looks unrelated to Redis.The change
The exponent is clamped before it is used. A separate
_exponentialhelper makes the reason explicit and keeps each strategy reading the way it did before.Clamping cannot change any delay a caller can observe. The smallest base that survives construction still produces a value far above any cap once it has been doubled a thousand times, so past the clamp every strategy was already returning the cap.
Testing
test_backoff_survives_a_long_outageruns all four strategies at 1023, 1024, 5000 and 10**6 failures and asserts the delay stays finite and within the cap. It fails on the current code withOverflowError: int too large to convert to float.test_exponential_backoff_is_unchanged_below_the_clamppins the existing arithmetic for every failure count below the clamp, so the fix cannot quietly alter a real delay.Running
tests/test_backoff.py,tests/test_retry.py,tests/test_utils.pyandtests/test_connection.pybefore and after gives the same set of failures apart from the four this fixes; the rest need a live server.ruff checkandruff format --checkare clean.Note
Low Risk
Localized retry-timing math with tests preserving observable delays below the overflow threshold; no auth, data, or API surface changes.
Overview
Fixes exponential backoff blowing up with
OverflowErroroncefailuresreaches 1024, which breaks infinite retry (retries < 0) after a long outage even though delays were already capped.Adds a shared
_exponentialhelper that computesbase * 2**failuresviamath.ldexp, returningmath.infon overflow somin(cap, …)still yields the cap.ExponentialBackoff,FullJitterBackoff,EqualJitterBackoff, andExponentialWithJitterBackoffnow call it instead ofbase * 2**failures.Tests cover all four strategies at very large failure counts, parity with the old formula for normal counts, and continued growth toward the cap past the old float overflow cliff.
Reviewed by Cursor Bugbot for commit 6e012e7. Bugbot is set up for automated code reviews on this repo. Configure here.