From 0d5354bedc1d06eee167053d28603d47c4749550 Mon Sep 17 00:00:00 2001 From: Lukasz Raczylo Date: Thu, 13 Aug 2026 05:50:33 +0100 Subject: [PATCH] Fix dead CircuitBreaker.ResetTimeout config (round 39) error_recovery.go: ResetTimeout was copied into the CircuitBreaker but never consulted by the state machine (used only for a metric), so the documented "how long to wait before fully closing the circuit" (reset_timeout, default 30s) had no effect: Open->HalfOpen used Timeout (60s), and a single half-open success closed the circuit immediately, losing the intended open -> half-open -> (reset cooldown) -> closed hysteresis and letting a stray success immediately reopen (bounce). Now the circuit remains half-open after entering it until resetTimeout has elapsed; only then can a successful request fully close it. Half-open requests are still allowed as probes. Default behavior is more conservative (up to Timeout+ResetTimeout before full close). Tests: updated TestCircuitBreakerHalfOpenTransition to the corrected semantics; added TestCircuitBreakerResetTimeoutEnforced (fails on the old dead-field code, where resetTimeout was ignored). Verified: full source-order suite, full -race -count=1 -shuffle=on ./... (all packages), golangci-lint run ./... (0 issues). --- error_recovery.go | 16 +++++++++++++- error_recovery_test.go | 50 ++++++++++++++++++++++++++++++++++-------- 2 files changed, 56 insertions(+), 10 deletions(-) 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())) } }