Skip to content

Commit 4fe0d6d

Browse files
fix(circuit_breaker): start a fresh probe success streak on each probe election (#8646)
_LOCAL_SUCCESSES is module-level and was only reset when a probe failed in the same environment or when this environment closed the circuit. A streak left from an earlier recovery window (this environment probed, lost its lease, and another environment reopened the circuit) counted towards the next window, so the circuit could close after fewer than success_threshold probes. Reset the streak right after this worker wins try_acquire_half_open, on both the OPEN path and the expired-lease takeover path. Add regression tests for both paths. Closes #8645 Co-authored-by: Leandro Damascena <lcdama@amazon.pt>
1 parent d82a695 commit 4fe0d6d

2 files changed

Lines changed: 68 additions & 0 deletions

File tree

‎aws_lambda_powertools/utilities/circuit_breaker/base.py‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -151,6 +151,7 @@ def handle(self) -> Any:
151151
if self._now() >= opened_at + self.config.recovery_timeout:
152152
# Recovery window elapsed: try to become the single prober.
153153
if self.persistence_store.try_acquire_half_open(self.name, _probe_owner_id(), opened_at):
154+
self._reset_probe_successes()
154155
self._notify(CircuitState.OPEN, CircuitState.HALF_OPEN, opened_at=opened_at)
155156
return self._call_probe()
156157
return self._open_response(record.to_circuit_info())
@@ -165,10 +166,23 @@ def handle(self) -> Any:
165166
if record.probe_lease_expiry is not None and self._now() >= record.probe_lease_expiry:
166167
logger.debug("Circuit '%s' probe lease expired; attempting takeover.", self.name)
167168
if self.persistence_store.try_acquire_half_open(self.name, _probe_owner_id(), record.opened_at or 0):
169+
self._reset_probe_successes()
168170
return self._call_probe()
169171

170172
return self._open_response(record.to_circuit_info())
171173

174+
def _reset_probe_successes(self) -> None:
175+
"""
176+
Start a fresh probe success streak after this worker wins a probe election.
177+
178+
``_LOCAL_SUCCESSES`` lives at module level, so a streak left over from an earlier
179+
recovery window (this environment probed, then lost the lease, or another
180+
environment finished that cycle) would otherwise count towards the new one and
181+
close the circuit after fewer than ``success_threshold`` probes.
182+
"""
183+
with _COUNTERS_LOCK:
184+
_LOCAL_SUCCESSES[self.name] = 0
185+
172186
def _call_closed(self) -> Any:
173187
"""Run the protected call while the circuit is closed, tracking failures."""
174188
try:

‎tests/functional/circuit_breaker/test_circuit_breaker.py‎

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -442,6 +442,60 @@ def call():
442442
assert base_module._LOCAL_FAILURES["c"] == 1 # fresh count, not 3
443443

444444

445+
def test_new_probe_election_does_not_reuse_successes_from_an_earlier_recovery_window(store, now):
446+
"""A probe success streak left from an earlier recovery window must not close a new one early."""
447+
config = CircuitBreakerConfig(recovery_timeout=30, success_threshold=3, local_cache_max_age=0)
448+
store.db["c"] = CircuitStateRecord(name="c", state=CircuitState.OPEN, failure_count=5, opened_at=now - 60)
449+
450+
@circuit_breaker(name="c", persistence_store=store, config=config)
451+
def call():
452+
return "ok"
453+
454+
# This environment wins the probe election and gets 2 of the 3 successes it needs.
455+
call()
456+
call()
457+
assert store.db["c"].state == CircuitState.HALF_OPEN
458+
assert base_module._LOCAL_SUCCESSES["c"] == 2
459+
460+
# Simulate: this environment goes idle, its probe lease lapses, another environment
461+
# takes over, its probe fails and the circuit reopens. That environment cannot reset
462+
# this one's module-level counter.
463+
store.db["c"] = CircuitStateRecord(name="c", state=CircuitState.OPEN, failure_count=0, opened_at=now - 60)
464+
465+
# The next recovery window is won by this environment again.
466+
call()
467+
468+
# One success must not close the circuit: the new window needs its own 3 in a row.
469+
assert store.db["c"].state == CircuitState.HALF_OPEN
470+
assert base_module._LOCAL_SUCCESSES["c"] == 1
471+
call()
472+
call()
473+
assert store.db["c"].state == CircuitState.CLOSED
474+
475+
476+
def test_probe_lease_takeover_starts_a_fresh_success_streak(store, now):
477+
"""Taking over an expired probe lease must not inherit the previous owner's streak."""
478+
config = CircuitBreakerConfig(recovery_timeout=30, success_threshold=3, local_cache_max_age=0)
479+
store.db["c"] = CircuitStateRecord(
480+
name="c",
481+
state=CircuitState.HALF_OPEN,
482+
opened_at=now - 200,
483+
half_open_owner="recycled-owner",
484+
probe_lease_expiry=now - 10, # expired
485+
)
486+
# The previous owner shared this environment and left 2 successes behind.
487+
base_module._LOCAL_SUCCESSES["c"] = 2
488+
489+
@circuit_breaker(name="c", persistence_store=store, config=config)
490+
def call():
491+
return "ok"
492+
493+
call()
494+
495+
assert store.db["c"].state == CircuitState.HALF_OPEN
496+
assert base_module._LOCAL_SUCCESSES["c"] == 1
497+
498+
445499
def test_store_write_failure_does_not_mask_downstream_result(store, now):
446500
"""Bug 2: if save_closed() fails, the successful probe result must still be returned."""
447501
config = CircuitBreakerConfig(failure_threshold=3, recovery_timeout=30, success_threshold=1, local_cache_max_age=0)

0 commit comments

Comments
 (0)