fix: use seconds for retry backoff and fix refresh_secret_now datetime math by SaiTejaKundety · Pull Request #72 · aws/aws-secretsmanager-caching-python

SaiTejaKundety · 2026-08-06T21:43:34Z

Description

Why is this change being made?

The exception-retry path in SecretCacheObject had several bugs that made the refresh backoff behave incorrectly:

Retry backoff used the wrong time unit (off by 1000×). __refresh built the next retry time with timedelta(milliseconds=delay), but the retry-delay config values are in seconds. Treating seconds as milliseconds made every backoff 1000× too short, the max delay capped at 3.6 seconds (3600 ms) instead of the intended 1 hour (3600 s). persistently failing refresh hammered Secrets Manager every ~3.6s, wasting calls and adding to throttling.
refresh_secret_now() raised TypeError when a retry was pending. _next_retry_time is a datetime, but the code
subtracted an int (current time in millis) from it: exception_sleep = self._next_retry_time - current_time_millis.
Subtracting an int from a datetime raises TypeError, so any forced refresh while an exception retry was scheduled
crashed instead of sleeping until the retry time.
refresh_secret_now() never updated the cache. It called _execute_refresh() directly and
discarded the result, unlike __refresh() which stores it via _set_result(). The fetched
secret was thrown away, leaving the stale value cached.
Exception state was not reset on a successful refresh. _next_retry_time stayed stale even
after success.
refresh_secret_now() held no lock. It read/wrote shared state (_refresh_needed, _exception,
_next_retry_time, cached result) without self._lock, unlike get_secret_value() — a race
under concurrent access.

What is changing?

__refresh now uses timedelta(seconds=delay), restoring the intended 1-hour cap (a 1000× correction from the broken 3.6s).
refresh_secret_now now guards on _next_retry_time (not just _exception) before computing the wait, then diffs two
datetimes and takes the larger of the retry wait and the jitter sleep:

  if self._exception is not None and self._next_retry_time is not None:
      now = datetime.now(timezone.utc)
      exception_sleep = (self._next_retry_time - now).total_seconds() * 1000
      sleep = max(exception_sleep, sleep)

The jitter floors the wait, so a stale or negative delta can never shorten it below the normal jitter.

refresh_secret_now now delegates to __refresh(), so the fetched value is stored in the
cache instead of discarded.
The refresh runs under self._lock, matching get_secret_value().
__refresh clears _next_retry_time (alongside _exception and _exception_count) on success.

What has changed since the last revision

Addressed review feedback on the forced-refresh path:

refresh_secret_now() now delegates to __refresh(), so the fetched value is stored in the cache via _set_result() instead of being discarded.
The refresh now runs under self._lock, matching get_secret_value(), removing the race on shared state.
__refresh() now clears _next_retry_time (alongside _exception and _exception_count) on a successful refresh, so stale backoff state is no longer carried forward.
Strengthened test_force_refresh_with_retry_pending to also assert the cache is updated and the exception/backoff state is reset on success.
Updated the description and breaking-changes note to cover the new behavior (a failed refresh_secret_now() no longer raises; the error surfaces on the next get_secret_value()).

Testing

How was this tested?

Updated the existing backoff test in test/unit/test_items.py to assert _next_retry_time is in seconds, not
milliseconds.
test_force_refresh_with_retry_pending sets up a pending retry by driving a real failure through __refresh(), then confirms that refresh_secret_now() completes without raising TypeError, refreshes the cache, and resets the exception and backoff state on success.
Verified both fixes by reintroducing each bug and confirming the corresponding test fails.
Ran the full unit suite with coverage + doctest gates, plus flake8 and pylint.

When testing locally, provide testing artifact(s):

$ pytest test/unit
43 passed
Required test coverage of 90% reached. Total coverage: 99.26%

$ flake8
(clean)

$ pylint --rcfile=.pylintrc src/aws_secretsmanager_caching
Your code has been rated at 10.00/10

Reviewee Checklist

Update the checklist after submitting the PR

Reviewer Checklist

All reviewers please ensure the following are true before reviewing:

Reviewee checklist has been accurately filled out
Code changes align with stated purpose in description
Test coverage adequately validates the changes

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

derik01

I'm pretty sure this should error out too: https://github.com/aws/aws-secretsmanager-caching-python/blob/master/src/aws_secretsmanager_caching/cache/items.py#L139

derik01 · 2026-08-07T21:19:14Z

+                )
+                self._exception_count += 1
+                delay = min(delay, self._config.exception_retry_delay_max)
+                self._next_retry_time = datetime.now(timezone.utc) + timedelta(milliseconds=delay)


I think a real bug here (not caused by you) is that the README calls out secret_refresh_interval - The number of seconds to wait between refreshing cached secret information. The default value is 3600.0.. However, we can see that we instead use milliseconds. So at a maximum, we retry at 3.6 seconds.

…e math

codecov · 2026-08-11T20:10:55Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.26%. Comparing base (34480e7) to head (af33a1d).

Additional details and impacted files

@@            Coverage Diff             @@
##           master      #72      +/-   ##
==========================================
+ Coverage   98.14%   99.26%   +1.11%     
==========================================
  Files           8        8              
  Lines         270      272       +2     
==========================================
+ Hits          265      270       +5     
+ Misses          5        2       -3

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Drive a real failure through __refresh() so _next_retry_time is set by production code, then confirm the forced refresh walks that path without raising TypeError. Drops the brittle sleep-duration assertions; the seconds-based backoff is already covered by test_datetime_fix_refresh.

derik01 · 2026-08-11T22:30:26Z

Required test coverage of 90% reached. Total coverage: 99.26%

Since we missed the TypeError when we first implemented it, I'm a little curious what the remaining lines are.

derik01 · 2026-08-11T22:31:29Z

-            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)


Why do we need the max function?

EDIT: also, should we gate the if statement on if self._next_retry_time is not None?

in case (self._next_retry_time - now) is ever negative, this will just pick 0. however, in the next line it has a max between exception_sleep & sleep and sleep can never be negative so we can remove this portion (its redundant).

For your gating question: I think we can gate on both exception & next_retry time and it would be the safest option

derik01 · 2026-08-17T23:51:08Z

I see that we are not clearing the exception if a refresh of the secret is successful. I feel like if a refresh is successful we can also reset the exception counter. I think the Java caching library does something similar. I also noticed that we have a __refresh function. Maybe we can re-use that?

Let me know what you think about that.

+1 to Derik. Additionally:
1/ Since _execute_refresh is invoked here instead of __refresh, I believe we are never actually updating the cache with the updated value.
2/ I feel this method reads/writes shared state like _refresh_needed, _exception, _next_retry_time but never acquires a lock (ref get_secret_value).

I'd like your thoughts on this. I also understand this seems to be an existing bug so I am ok fixing this in a separate PR too.

Yup, I think both of you are correct. This is something that should be changed

i-am-SR · 2026-08-19T17:06:50Z

-            exception_sleep = self._next_retry_time - current_time_millis
+        if self._exception is not None and self._next_retry_time is not None:
+            now = datetime.now(timezone.utc)
+            exception_sleep = (self._next_retry_time - now).total_seconds() * 1000


A minor issue here - exception sleep can be negative which Ideally should be 0 if next_retry_time hasn't been reached yet. The PR description captures the change as

exception_sleep = max((self._next_retry_time - now).total_seconds() * 1000, 0)

Which doesn't match the implementation.

I think this is okay since we have sleep = max(exception_sleep, sleep) which will always be at a minimum of line 135.

+1. For the minimum number it will choose the random number. I had that line in the code before, but then saw sleep = max(exception_sleep, sleep) and then removed it.

i-am-SR · 2026-08-19T17:31:23Z

+1 to Derik. Additionally:
1/ Since _execute_refresh is invoked here instead of __refresh, I believe we are never actually updating the cache with the updated value.
2/ I feel this method reads/writes shared state like _refresh_needed, _exception, _next_retry_time but never acquires a lock (ref get_secret_value).

I'd like your thoughts on this. I also understand this seems to be an existing bug so I am ok fixing this in a separate PR too.

derik01

Customers can wait up to an 1 hour for their secret to refresh now.

derik01 · 2026-09-01T00:11:27Z

+        # Refresh under the lock: __refresh stores the result and resets exception/backoff
+        # state on success, or records the exception and schedules a retry on failure.
+        with self._lock:
+            self.__refresh()


Since we opted for the __refresh() method here, we're changing the customer behavior. __refresh() has broad exception handling, meaning a failure in refreshing the secret now is swallowed.

If we pull inspiration from the Java caching library, it uses a bool to signal to the customer whether a refresh successful or not. I think this is a decent approach, what do you think?

derik01 reviewed Aug 7, 2026

View reviewed changes

SaiTejaKundety added 2 commits August 11, 2026 13:03

fix: only retry on transient errors for secret refresh

e1294b9

fix: use seconds for retry backoff and fix refresh_secret_now datetim…

c87f6be

…e math

SaiTejaKundety force-pushed the classify-transient-refresh-errors branch from b696d3c to c87f6be Compare August 11, 2026 20:04

SaiTejaKundety changed the title ~~fix: only retry on transient errors for secret refresh~~ fix: use seconds for retry backoff and fix refresh_secret_now datetime math Aug 11, 2026

derik01 reviewed Aug 11, 2026

View reviewed changes

SaiTejaKundety added 2 commits August 11, 2026 15:42

refactor: remove redundant max() clamp on exception_sleep

a9140a0

fix: guard on _next_retry_time as well in refresh_secret_now

d097c7e

derik01 reviewed Aug 17, 2026

View reviewed changes

i-am-SR reviewed Aug 19, 2026

View reviewed changes

SaiTejaKundety marked this pull request as ready for review August 19, 2026 22:13

SaiTejaKundety requested a review from a team as a code owner August 19, 2026 22:13

derik01 previously approved these changes Aug 21, 2026

View reviewed changes

fix: refresh_secret_now updates cache under lock

2d1f198

SaiTejaKundety dismissed derik01’s stale review via 2d1f198 August 25, 2026 17:43

test: drop issue-number labels from refresh test

af33a1d

derik01 reviewed Sep 1, 2026

View reviewed changes

Uh oh!

Conversation

SaiTejaKundety commented Aug 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Why is this change being made?

What is changing?

What has changed since the last revision

Related Links

Testing

How was this tested?

When testing locally, provide testing artifact(s):

Reviewee Checklist

Reviewer Checklist

Uh oh!

derik01 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented Aug 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

derik01 commented Aug 11, 2026

Uh oh!

derik01 Aug 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SaiTejaKundety Aug 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

derik01 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

SaiTejaKundety commented Aug 6, 2026 •

edited

Loading

codecov Bot commented Aug 11, 2026 •

edited

Loading

derik01 Aug 11, 2026 •

edited

Loading

SaiTejaKundety Aug 19, 2026 •

edited

Loading