diff --git a/error_recovery.go b/error_recovery.go index 9c18f03..fa53fd5 100644 --- a/error_recovery.go +++ b/error_recovery.go @@ -191,12 +191,16 @@ type CircuitBreaker struct { maxFailures int // timeout is how long to wait before allowing requests in half-open state timeout time.Duration - // resetTimeout is how long to wait before transitioning from open to half-open + // resetTimeout is how long to stay in half-open (after the open timeout) + // before a successful response fully closes the circuit resetTimeout time.Duration // state tracks the current circuit breaker state state CircuitBreakerState // failures counts consecutive failures failures int64 + // halfOpenSince is when the circuit entered half-open; used to enforce + // resetTimeout before a success can fully close it + halfOpenSince time.Time } // CircuitBreakerConfig holds configuration parameters for circuit breakers. @@ -275,6 +279,7 @@ func (cb *CircuitBreaker) allowRequest() bool { case CircuitBreakerOpen: if now.Sub(cb.lastFailureTime) > cb.timeout { cb.state = CircuitBreakerHalfOpen + cb.halfOpenSince = now cb.logger.Infof("Circuit breaker transitioning to half-open state") return true } @@ -317,6 +322,15 @@ func (cb *CircuitBreaker) recordSuccess() { switch cb.state { case CircuitBreakerHalfOpen: + // Honor resetTimeout: do not fully close until the circuit has been + // in half-open for at least resetTimeout. This provides the + // documented open -> half-open -> (reset cooldown) -> closed + // hysteresis instead of closing on the very first half-open + // success, which would let a stray success immediately reopen + // (bounce). While in half-open requests are still allowed as probes. + if time.Since(cb.halfOpenSince) < cb.resetTimeout { + return + } cb.failures = 0 cb.state = CircuitBreakerClosed cb.LogInfo("Circuit breaker closed after successful request in half-open state") diff --git a/error_recovery_test.go b/error_recovery_test.go index e9cd1e8..60c2855 100644 --- a/error_recovery_test.go +++ b/error_recovery_test.go @@ -89,18 +89,50 @@ func TestCircuitBreakerHalfOpenTransition(t *testing.T) { time.Sleep(150 * time.Millisecond) - allowed := false - _ = cb.Execute(func() error { - allowed = true - return nil - }) - - if !allowed { - t.Error("Request should be allowed in half-open state") + // First request after the open timeout probes half-open. A single success + // does NOT immediately close: the circuit stays half-open until + // resetTimeout has elapsed (reset-cooldown hysteresis). + if err := cb.Execute(func() error { return nil }); err != nil { + t.Errorf("Request should be allowed in half-open state, got: %v", err) + } + if cb.GetState() != CircuitBreakerHalfOpen { + t.Errorf("Circuit should stay half-open during reset cooldown, got %v", circuitBreakerStateToString(cb.GetState())) } + // After resetTimeout elapses, a successful request fully closes it. + time.Sleep(60 * time.Millisecond) + if err := cb.Execute(func() error { return nil }); err != nil { + t.Errorf("Request should be allowed, got: %v", err) + } if cb.GetState() != CircuitBreakerClosed { - t.Errorf("Circuit should be closed after successful half-open request, got %v", cb.GetState()) + t.Errorf("Circuit should be closed after successful request following reset cooldown, got %v", circuitBreakerStateToString(cb.GetState())) + } +} + +// TestCircuitBreakerResetTimeoutEnforced is a regression test for the dead +// resetTimeout config field: it must be consulted before a half-open success +// can fully close the circuit. This failed on the old code where resetTimeout +// was set but never read, so a single half-open success closed the circuit +// immediately regardless of the configured reset cooldown. +func TestCircuitBreakerResetTimeoutEnforced(t *testing.T) { + cb := NewCircuitBreaker(CircuitBreakerConfig{ + MaxFailures: 1, + Timeout: 50 * time.Millisecond, + ResetTimeout: time.Minute, + }, nil) + + _ = cb.Execute(func() error { return errors.New("fail") }) + if cb.GetState() != CircuitBreakerOpen { + t.Fatal("circuit should be open after failure") + } + + time.Sleep(70 * time.Millisecond) // past the open timeout -> half-open + + if err := cb.Execute(func() error { return nil }); err != nil { + t.Fatalf("request should be allowed in half-open state, got: %v", err) + } + if cb.GetState() != CircuitBreakerHalfOpen { + t.Fatalf("circuit must remain half-open until resetTimeout elapses, got %v", circuitBreakerStateToString(cb.GetState())) } }