From c1b37248bba6bc4ab2dca51856dbc38abb6be8a0 Mon Sep 17 00:00:00 2001 From: coder6955 <25655284+coder6955@users.noreply.github.com> Date: Wed, 30 Sep 2026 18:10:55 +0530 Subject: [PATCH] Fix time-based circuit statistics after idle window reset --- .../failsafe/internal/TimedCircuitStats.java | 7 +--- .../failsafe/internal/ClosedStateTest.java | 22 ++++++++++ .../internal/TimedCircuitStatsTest.java | 41 +++++++++++++++++++ 3 files changed, 65 insertions(+), 5 deletions(-) diff --git a/core/src/main/java/dev/failsafe/internal/TimedCircuitStats.java b/core/src/main/java/dev/failsafe/internal/TimedCircuitStats.java index 1caac112..7c9b47b6 100644 --- a/core/src/main/java/dev/failsafe/internal/TimedCircuitStats.java +++ b/core/src/main/java/dev/failsafe/internal/TimedCircuitStats.java @@ -27,7 +27,6 @@ class TimedCircuitStats implements CircuitStats { private final Clock clock; private final long bucketSizeMillis; - private final long windowSizeMillis; // Mutable state final Bucket[] buckets; @@ -37,7 +36,6 @@ class TimedCircuitStats implements CircuitStats { public TimedCircuitStats(int bucketCount, Duration thresholdingPeriod, Clock clock, CircuitStats oldStats) { this.clock = clock; this.buckets = new Bucket[bucketCount]; - windowSizeMillis = thresholdingPeriod.toMillis(); bucketSizeMillis = thresholdingPeriod.toMillis() / buckets.length; for (int i = 0; i < buckets.length; i++) buckets[i] = new Bucket(); @@ -197,9 +195,7 @@ synchronized Bucket getCurrentBucket() { currentIndex = nextIndex(); previousBucket = currentBucket; currentBucket = buckets[currentIndex]; - long bucketStartTime = currentBucket.startTimeMillis == -1 ? - previousBucket.startTimeMillis + bucketSizeMillis : - currentBucket.startTimeMillis + windowSizeMillis; + long bucketStartTime = previousBucket.startTimeMillis + bucketSizeMillis; summary.remove(currentBucket); currentBucket.reset(bucketStartTime); bucketsToMove--; @@ -207,6 +203,7 @@ synchronized Bucket getCurrentBucket() { } else { // Reset all buckets reset(); + currentBucket = buckets[currentIndex]; } } diff --git a/core/src/test/java/dev/failsafe/internal/ClosedStateTest.java b/core/src/test/java/dev/failsafe/internal/ClosedStateTest.java index 8de41523..bf106e33 100644 --- a/core/src/test/java/dev/failsafe/internal/ClosedStateTest.java +++ b/core/src/test/java/dev/failsafe/internal/ClosedStateTest.java @@ -16,8 +16,11 @@ package dev.failsafe.internal; import dev.failsafe.CircuitBreaker; +import dev.failsafe.internal.TimedCircuitStatsTest.TestClock; import org.testng.annotations.Test; +import java.time.Duration; + import static org.testng.Assert.assertFalse; import static org.testng.Assert.assertTrue; @@ -84,6 +87,25 @@ public void testFailureWithFailureThreshold() { assertTrue(breaker.isOpen()); } + public void testFailureThresholdAfterIdlePeriod() { + CircuitBreakerImpl breaker = (CircuitBreakerImpl) CircuitBreaker.builder() + .withFailureThreshold(2, Duration.ofSeconds(4)) + .build(); + TestClock clock = new TestClock(); + breaker.state.get().stats = new TimedCircuitStats(4, Duration.ofSeconds(4), clock, null); + + clock.set(1000); + breaker.recordSuccess(); + + // The failures after an idle period must both count towards the threshold. + clock.set(10000); + breaker.recordFailure(); + assertTrue(breaker.isClosed()); + clock.set(11000); + breaker.recordFailure(); + assertTrue(breaker.isOpen()); + } + /** * Asserts that the circuit is still closed after a single success. */ diff --git a/core/src/test/java/dev/failsafe/internal/TimedCircuitStatsTest.java b/core/src/test/java/dev/failsafe/internal/TimedCircuitStatsTest.java index 6f9b6c29..f2490dad 100644 --- a/core/src/test/java/dev/failsafe/internal/TimedCircuitStatsTest.java +++ b/core/src/test/java/dev/failsafe/internal/TimedCircuitStatsTest.java @@ -18,6 +18,7 @@ import dev.failsafe.internal.TimedCircuitStats.Bucket; import dev.failsafe.internal.TimedCircuitStats.Clock; import org.testng.annotations.BeforeMethod; +import org.testng.annotations.DataProvider; import org.testng.annotations.Test; import java.time.Duration; @@ -152,6 +153,46 @@ public void testMetrics() { assertEquals(stats.getExecutionCount(), 0); } + @DataProvider + public Object[][] idlePeriods() { + return new Object[][] { + { 0, true }, { 1, true }, { 2, true }, { 3, true }, + { 0, false }, { 1, false }, { 2, false }, { 3, false } + }; + } + + @Test(dataProvider = "idlePeriods") + public void testExecutionExpiresAfterIdlePeriod(int previousIndex, boolean failure) { + stats = new TimedCircuitStats(4, Duration.ofSeconds(4), clock, null); + clock.set(previousIndex * 1000); + stats.recordSuccess(); + + // The first execution after a full window of inactivity must belong to the new current bucket. + clock.set(10000); + if (failure) + stats.recordFailure(); + else + stats.recordSuccess(); + + // Keep the execution for the new window, including when the previously current bucket is visited again. + for (int seconds = 0; seconds < 4; seconds++) { + clock.set(10000 + seconds * 1000); + stats.getCurrentBucket(); + assertEquals(stats.getExecutionCount(), 1); + assertEquals(stats.getFailureCount(), failure ? 1 : 0); + assertEquals(stats.getSuccessCount(), failure ? 0 : 1); + assertEquals(stats.getFailureRate(), failure ? 100 : 0); + assertEquals(stats.getSuccessRate(), failure ? 0 : 100); + } + + // Expire it exactly when its own window ends. + clock.set(14000); + stats.getCurrentBucket(); + assertEquals(stats.getExecutionCount(), 0); + assertEquals(stats.getFailureCount(), 0); + assertEquals(stats.getSuccessCount(), 0); + } + public void testCopyToEqualSizedStats() { stats = new TimedCircuitStats(4, Duration.ofSeconds(4), clock, null); assertValues(stats, b(0, 0, 0), b(-1, 0, 0), b(-1, 0, 0), b(-1, 0, 0));