fix: Guard factory input and keep the next delay positive after a success

Author: jsonbailey

ldclient/impl/retry.py

@@ -18,6 +18,7 @@
 
 # currently excluded from documentation - see docs/README.md
 
+import math
 import random
 import time
 from enum import Enum
@@ -34,10 +35,10 @@
 # delay is configurable as ``initial_reconnect_delay``.
 STREAMING_MAX_DELAY = 30
 
-# The documented default for ``initial_reconnect_delay``, in seconds. It stands
-# in for a configured value of zero or less, which would reconnect with no wait
-# at all.
+# The documented defaults, in seconds. Each stands in for a configured value
+# that is not a positive, finite number.
 DEFAULT_INITIAL_RECONNECT_DELAY = 1
+DEFAULT_POLL_INTERVAL = 30
 
 # How long streaming must operate without a failure before its retry state
 # resets, in seconds.
@@ -195,7 +196,7 @@ def __init__(
         self._max_delay = max(normal_ceiling, initial_delay)
         self._attempts = 0
         # Read before any outcome is recorded, this is the ordinary interval.
-        self._next_delay = operating_cadence if operating_cadence > 0 else initial_delay
+        self._next_delay = self._wait_between_operations()
 
     @property
     def next_delay(self) -> float:
@@ -262,13 +263,18 @@ def record_success(self) -> None:
         Records a successful operation, and resets the retry state if that is
         now enough.
 
-        The wait before the next operation becomes the operating cadence, even
-        when the retry state is still raised, because a backoff wait applies to
-        a retry and not to every operation.
+        The wait before the next operation goes back to the ordinary interval,
+        even when the retry state is still raised, because a backoff wait
+        applies to a retry and not to every operation.
         """
         self._reset_policy.note_healthy()
         self._reset_if_due()
-        self._next_delay = self._operating_cadence
+        self._next_delay = self._wait_between_operations()
+
+    def _wait_between_operations(self) -> float:
+        """The wait when nothing is being retried: the operating cadence, or
+        the initial delay for a component that has no cadence."""
+        return self._operating_cadence if self._operating_cadence > 0 else self._initial_delay
 
     def _reset_if_due(self) -> None:
         """Clears the retry state when the reset policy is satisfied, returning
@@ -288,6 +294,19 @@ def _compute_wait(self) -> float:
         return max(delay - jitter, self._operating_cadence)
 
 
+def _positive_finite(value: float, default: float, name: str) -> float:
+    """Returns ``value`` if it is a positive, finite number of seconds, and the
+    default otherwise. A non-finite value would make the jitter arithmetic
+    produce a NaN delay, and a non-positive one would retry with no wait."""
+    if value > 0 and math.isfinite(value):
+        return value
+    log.warning(
+        "%s must be a positive, finite number of seconds; using the default of %ss"
+        % (name, default)
+    )
+    return default
+
+
 def for_streaming(initial_reconnect_delay: float) -> RetryState:
     """
     Builds the retry state for a streaming data source.
@@ -296,18 +315,14 @@ def for_streaming(initial_reconnect_delay: float) -> RetryState:
     is healthy from the first message of a fresh stream, and resets after a
     minute of that.
 
-    A configured delay of zero or less would reconnect with no wait, so the
-    documented default stands in for it. ``Config`` does not check this value,
-    though it does clamp ``poll_interval``.
+    ``Config`` does not check the configured delay, so the documented default
+    stands in for anything that is not a positive, finite number.
 
     The extended regime never starts below the configured delay.
     """
-    if initial_reconnect_delay <= 0:
-        log.warning(
-            "initial_reconnect_delay must be greater than zero; using the default of %ss"
-            % DEFAULT_INITIAL_RECONNECT_DELAY
-        )
-        initial_reconnect_delay = DEFAULT_INITIAL_RECONNECT_DELAY
+    initial_reconnect_delay = _positive_finite(
+        initial_reconnect_delay, DEFAULT_INITIAL_RECONNECT_DELAY, 'initial_reconnect_delay'
+    )
     return RetryState(
         initial_delay=initial_reconnect_delay,
         normal_ceiling=STREAMING_MAX_DELAY,
@@ -326,7 +341,11 @@ def for_polling(poll_interval: float) -> RetryState:
     interval itself, which means a normal failure simply polls again on
     schedule. Polling is healthy on any successful poll, and resets after two
     in a row.
+
+    ``Config`` clamps the poll interval, but the documented default stands in
+    for anything that reaches here and is not a positive, finite number.
     """
+    poll_interval = _positive_finite(poll_interval, DEFAULT_POLL_INTERVAL, 'poll_interval')
     return RetryState(
         initial_delay=poll_interval,
         normal_ceiling=poll_interval,

ldclient/testing/impl/test_retry.py

@@ -8,6 +8,7 @@
 """
 
 import logging
+import math
 import random
 from contextlib import contextmanager
 from unittest import mock
@@ -17,6 +18,7 @@
 from ldclient.impl import retry
 from ldclient.impl.retry import (
     DEFAULT_INITIAL_RECONNECT_DELAY,
+    DEFAULT_POLL_INTERVAL,
     EXTENDED_INITIAL_DELAY,
     EXTENDED_MAX_DELAY,
     POLLING_RESET_SUCCESSES,
@@ -107,24 +109,51 @@ def test_non_error_statuses_are_normal(self, status):
         assert classify_http_status(status) is NORMAL
 
 
-class TestStreamingInitialDelayGuard:
-    """``Config`` does not check ``initial_reconnect_delay``, and a value of
-    zero would reconnect with no wait at all."""
+class TestFactoryInputGuards:
+    """``Config`` does not check ``initial_reconnect_delay`` at all, and only
+    clamps ``poll_interval``. A non-positive value would retry with no wait; a
+    non-finite one makes the jitter arithmetic produce NaN."""
 
-    @pytest.mark.parametrize("configured", [0, -1, -0.5])
-    def test_a_non_positive_delay_falls_back_to_the_default(self, configured, caplog):
+    @pytest.mark.parametrize(
+        "configured",
+        [0, -1, -0.5, float('inf'), float('-inf'), float('nan')],
+        ids=["zero", "negative", "negative-fraction", "inf", "-inf", "nan"],
+    )
+    def test_streaming_falls_back_to_the_default(self, configured, caplog):
         caplog.set_level(logging.WARNING)
 
         state = for_streaming(configured)
+        delay = failure_delay(state, NORMAL)
 
         assert state.min_delay == DEFAULT_INITIAL_RECONNECT_DELAY
-        assert failure_delay(state, NORMAL) == DEFAULT_INITIAL_RECONNECT_DELAY
+        assert delay == DEFAULT_INITIAL_RECONNECT_DELAY
+        assert math.isfinite(delay) and delay > 0
         assert caplog.records[0].getMessage() == (
-            "initial_reconnect_delay must be greater than zero; using the default of 1s"
+            "initial_reconnect_delay must be a positive, finite number of seconds; "
+            "using the default of 1s"
+        )
+
+    @pytest.mark.parametrize(
+        "configured",
+        [0, -5, float('inf'), float('-inf'), float('nan')],
+        ids=["zero", "negative", "inf", "-inf", "nan"],
+    )
+    def test_polling_falls_back_to_the_default(self, configured, caplog):
+        caplog.set_level(logging.WARNING)
+
+        state = for_polling(configured)
+        delay = failure_delay(state, NORMAL)
+
+        assert state.operating_cadence == DEFAULT_POLL_INTERVAL
+        assert delay == DEFAULT_POLL_INTERVAL
+        assert math.isfinite(delay) and delay > 0
+        assert caplog.records[0].getMessage() == (
+            "poll_interval must be a positive, finite number of seconds; "
+            "using the default of 30s"
         )
 
     @pytest.mark.parametrize("configured", [0.001, 0.5, 1, 5, 45])
-    def test_a_positive_delay_is_left_alone(self, configured, caplog):
+    def test_a_positive_streaming_delay_is_left_alone(self, configured, caplog):
         caplog.set_level(logging.WARNING)
 
         state = for_streaming(configured)
@@ -133,6 +162,16 @@ def test_a_positive_delay_is_left_alone(self, configured, caplog):
         assert failure_delay(state, NORMAL) == configured
         assert caplog.records == []
 
+    @pytest.mark.parametrize("configured", [0.001, 1, 30, 300, 2 * 60 * 60])
+    def test_a_positive_poll_interval_is_left_alone(self, configured, caplog):
+        caplog.set_level(logging.WARNING)
+
+        state = for_polling(configured)
+
+        assert state.operating_cadence == configured
+        assert failure_delay(state, NORMAL) == configured
+        assert caplog.records == []
+
 
 class TestStreamingExtendedDelayFloor:
     """A delay that applies after an unexpected failure must not be below the
@@ -233,6 +272,32 @@ def test_every_delay_stays_within_the_jitter_bounds(self):
                 assert base / 2 <= delay <= base
 
 
+class TestWaitBetweenOperations:
+    def test_a_streaming_success_does_not_schedule_a_zero_wait(self):
+        """Streaming has no cadence, so a success falls back to the initial
+        delay. Zero would tell a scheduler to run again immediately."""
+        state = for_streaming(1)
+        state.record_failure(NORMAL)
+        state.record_success()
+
+        assert state.next_delay == 1
+
+    def test_a_polling_success_schedules_the_cadence(self):
+        state = for_polling(30)
+        state.record_failure(NORMAL)
+        state.record_success()
+
+        assert state.next_delay == 30
+
+    def test_a_fresh_state_and_a_success_agree(self):
+        """The constructor and record_success share one expression, so the two
+        cannot drift apart."""
+        for state in (for_streaming(5), for_polling(45)):
+            fresh = state.next_delay
+            state.record_success()
+            assert state.next_delay == fresh
+
+
 class TestStreamingReset:
     def test_a_minute_of_healthy_operation_resets_the_state(self):
         # The whole minute passes instantly.