fix: use seconds for retry backoff and fix refresh_secret_now datetime math

Author: SaiTejaKundety

src/aws_secretsmanager_caching/cache/items.py

@@ -20,8 +20,6 @@
 from datetime import datetime, timedelta, timezone
 from random import randint
 
-from botocore.exceptions import BotoCoreError, ClientError, NoCredentialsError, NoRegionError
-
 from .lru import LRUCache
 
 
@@ -53,50 +51,6 @@ def __init__(self, config, client, secret_id):
         self._refresh_needed = True
         self._next_retry_time = None
 
-    # Throttling is the only transient error that carries a 4xx status, so it
-    # must be matched by error code rather than by the status check below.
-    _THROTTLING_ERROR_CODES = frozenset({
-        "ThrottlingException",
-        "TooManyRequestsException",
-        "Throttling",
-    })
-
-    @staticmethod
-    def _is_transient_error(e):
-        """Determine whether a refresh exception is transient (worth retrying).
-
-        Only configuration errors and clear 4xx client errors (e.g.
-        ResourceNotFound, AccessDenied) are permanent. Everything else --
-        timeouts, throttling, 5xx, and any unrecognized error -- is retried,
-        preserving the previous behavior for errors we do not classify.
-
-        :type e: Exception
-        :param e: The exception raised during refresh.
-
-        :rtype: bool
-        :return: True if the error is transient and should be retried.
-        """
-        # Config errors are BotoCoreErrors but can never succeed on retry.
-        if isinstance(e, (NoCredentialsError, NoRegionError)):
-            return False
-
-        # Other BotoCoreErrors are transport failures (timeouts, connection).
-        if isinstance(e, BotoCoreError):
-            return True
-
-        # Service errors: permanent only for a clear 4xx (throttling excepted).
-        if isinstance(e, ClientError):
-            code = e.response.get("Error", {}).get("Code", "")
-            if code in SecretCacheObject._THROTTLING_ERROR_CODES:
-                return True
-            status = e.response.get("ResponseMetadata", {}).get("HTTPStatusCode")
-            if status is not None and 400 <= status < 500:
-                return False
-            return True
-
-        # Unrecognized error type: retry (preserves previous behavior).
-        return True
-
     def _is_refresh_needed(self):
         """Determine if the cached object should be refreshed.
 
@@ -145,17 +99,12 @@ def __refresh(self):
             self._exception_count = 0
         except Exception as e:  # pylint: disable=broad-except
             self._exception = e
-            if self._is_transient_error(e):
-                delay = self._config.exception_retry_delay_base * (
-                    self._config.exception_retry_growth_factor ** self._exception_count
-                )
-                self._exception_count += 1
-                delay = min(delay, self._config.exception_retry_delay_max)
-                self._next_retry_time = datetime.now(timezone.utc) + timedelta(milliseconds=delay)
-            else:
-                # Clear any retry time left over from a prior transient failure
-                # so a permanent error is not automatically retried.
-                self._next_retry_time = None
+            delay = self._config.exception_retry_delay_base * (
+                self._config.exception_retry_growth_factor ** self._exception_count
+            )
+            self._exception_count += 1
+            delay = min(delay, self._config.exception_retry_delay_max)
+            self._next_retry_time = datetime.now(timezone.utc) + timedelta(seconds=delay)
 
     def get_secret_value(self, version_stage=None):
         """Get the cached secret value for the given version stage.
@@ -186,8 +135,8 @@ def refresh_secret_now(self):
         sleep = randint(int(self.FORCE_REFRESH_JITTER_SLEEP / 2), self.FORCE_REFRESH_JITTER_SLEEP + 1)
 
         if self._exception is not None:
-            current_time_millis = int(datetime.now(timezone.utc).timestamp() * 1000)
-            exception_sleep = self._next_retry_time - current_time_millis
+            now = datetime.now(timezone.utc)
+            exception_sleep = max((self._next_retry_time - now).total_seconds() * 1000, 0)
             sleep = max(exception_sleep, sleep)
 
         # Divide by 1000 for millis

test/unit/test_items.py

@@ -15,22 +15,12 @@
 """
 import unittest
 from datetime import timezone, datetime, timedelta
-from unittest.mock import Mock
-
-from botocore.exceptions import ClientError, NoCredentialsError, ReadTimeoutError
+from unittest.mock import Mock, patch
 
 from aws_secretsmanager_caching.cache.items import SecretCacheObject, SecretCacheItem
 from aws_secretsmanager_caching.config import SecretCacheConfig
 
 
-def _client_error(code, status):
-    """Build a botocore ClientError with the given error code and HTTP status."""
-    return ClientError(
-        {"Error": {"Code": code}, "ResponseMetadata": {"HTTPStatusCode": status}},
-        "DescribeSecret",
-    )
-
-
 class TestSecretCacheObject(unittest.TestCase):
 
     def setUp(self):
@@ -109,72 +99,39 @@ def test_datetime_fix_refresh(self):
         t_after = datetime.now(tz=timezone.utc)
 
         t_before_delay = t_before + timedelta(
-            milliseconds=secret_cached_object._config.exception_retry_delay_base * (
+            seconds=secret_cached_object._config.exception_retry_delay_base * (
                 secret_cached_object._config.exception_retry_growth_factor ** exp_factor
             )
         )
         self.assertLessEqual(t_before_delay, secret_cached_object._next_retry_time)
 
         t_after_delay = t_after + timedelta(
-            milliseconds=secret_cached_object._config.exception_retry_delay_base * (
+            seconds=secret_cached_object._config.exception_retry_delay_base * (
                 secret_cached_object._config.exception_retry_growth_factor ** exp_factor
             )
         )
         self.assertGreaterEqual(t_after_delay, secret_cached_object._next_retry_time)
 
-    def test_is_transient_error_classification(self):
-        # Permanent service errors (4xx) -- must not be retried.
-        for code in ("ResourceNotFoundException", "AccessDeniedException",
-                     "InvalidParameterException", "DecryptionFailure",
-                     "ValidationException"):
-            self.assertFalse(
-                SecretCacheObject._is_transient_error(_client_error(code, 400)),
-                f"{code} should be permanent")
-
-        # Throttling is a 4xx but is transient -- must be retried.
-        self.assertTrue(
-            SecretCacheObject._is_transient_error(_client_error("ThrottlingException", 400)))
-
-        # 5xx server-side errors are transient.
-        for code, status in (("InternalServiceError", 500),
-                             ("InternalFailure", 500),
-                             ("ServiceUnavailable", 503)):
-            self.assertTrue(
-                SecretCacheObject._is_transient_error(_client_error(code, status)),
-                f"{code} should be transient")
-
-        # Transport-layer failures are transient; config errors are not.
-        self.assertTrue(
-            SecretCacheObject._is_transient_error(ReadTimeoutError(endpoint_url="https://x")))
-        self.assertFalse(SecretCacheObject._is_transient_error(NoCredentialsError()))
-
-        # Unknown error types default to transient, preserving the previous
-        # behavior of retrying any error we do not recognize as permanent.
-        self.assertTrue(SecretCacheObject._is_transient_error(KeyError("boom")))
-
-    def test_refresh_permanent_error_schedules_no_retry(self):
+    @patch("aws_secretsmanager_caching.cache.items.time.sleep")
+    def test_refresh_secret_now_with_pending_exception(self, mock_sleep):
+        # Regression test: when a prior refresh failed, _next_retry_time holds a
+        # datetime. The old code subtracted an int (current time in millis) from
+        # that datetime, raising TypeError. refresh_secret_now() must instead
+        # diff the two datetimes and sleep until the scheduled retry time.
         sco = SecretCacheObject(SecretCacheConfig(), None, None)
-        sco._set_result = Mock(side_effect=_client_error("ResourceNotFoundException", 400))
-        sco._refresh_needed = True
-
-        sco._SecretCacheObject__refresh()
-
-        self.assertIsNone(sco._next_retry_time)
-        self.assertFalse(sco._is_refresh_needed())
-        self.assertIsNotNone(sco._exception)
-
-    def test_refresh_permanent_error_clears_stale_retry_time(self):
-        # A permanent error following a transient one must clear the retry
-        # time left behind, otherwise the permanent error keeps being retried.
-        sco = SecretCacheObject(SecretCacheConfig(), None, None)
-        sco._next_retry_time = datetime.now(timezone.utc) - timedelta(seconds=1)
-        sco._set_result = Mock(side_effect=_client_error("ResourceNotFoundException", 400))
-        sco._refresh_needed = True
-
-        sco._SecretCacheObject__refresh()
-
-        self.assertIsNone(sco._next_retry_time)
-        self.assertFalse(sco._is_refresh_needed())
+        sco._exception = Exception("prior refresh failure")
+        sco._next_retry_time = datetime.now(timezone.utc) + timedelta(seconds=30)
+        sco._execute_refresh = Mock()
+
+        # Would have raised TypeError before the fix.
+        sco.refresh_secret_now()
+
+        # ~30s until retry -> ~30000ms; time.sleep() receives seconds (ms / 1000).
+        mock_sleep.assert_called_once()
+        slept_seconds = mock_sleep.call_args[0][0]
+        self.assertGreater(slept_seconds, 25)
+        self.assertLess(slept_seconds, 60)
+        sco._execute_refresh.assert_called_once()
 
 
 class TestSecretCacheItem(unittest.TestCase):