From feaf8a3d46b62d0e0f23aaeec97f240b0237a0c4 Mon Sep 17 00:00:00 2001 From: Leo Antunes Date: Fri, 26 Jun 2026 15:57:31 +0200 Subject: [PATCH] fix: reject half-open delay exceeding the sliding window size SlidingWindowBreaker.apply silently clamped halfOpenDelay to windowSize whenever it was 0 OR greater than windowSize. Defaulting an unset (0) value is fine, but silently shrinking an explicit value (e.g. 30s -> 10s) discards the caller's configuration without any signal, violating least-astonishment. Keep defaulting the unset case, but return an error when an explicit halfOpenDelay exceeds windowSize (such a delay could never let the circuit go half-open, since the window expires and closes it first). This mirrors how EWMABreaker already rejects invalid half-open configuration loudly at construction. WithHalfOpenDelay and NewSlidingWindowBreaker docs are updated to describe the behavior. Co-Authored-By: Claude Opus 4.8 (1M context) --- breaker.go | 23 +++++++++++++++-------- options.go | 4 ++++ options_test.go | 13 +++++++++++++ 3 files changed, 32 insertions(+), 8 deletions(-) diff --git a/breaker.go b/breaker.go index 7800159..cbd9ee3 100644 --- a/breaker.go +++ b/breaker.go @@ -155,10 +155,11 @@ type SlidingWindowBreaker struct { // // This is a time-based breaker, which means it will revert back to closed after its window size has passed: if no // observations are made in the window, the failure rate is effectively zero. -// This also means: if the circuit has a halfOpenDelay and it is bigger than windowSize, the breaker will never enter -// half-open state and will directly close instead. -// Conversely, if halfOpenDelay is smaller than windowSize, the errors observed in the last window will still count -// proportionally in half-open state, which will lead to faster re-opening on errors. +// The half-open delay (see [WithHalfOpenDelay]) defaults to windowSize when unset and may not exceed it (a larger delay +// would never let the circuit go half-open, since the window expires and closes it first); an explicit larger value is +// rejected by [NewCircuit]. +// If halfOpenDelay is smaller than windowSize, the errors observed in the last window will still count proportionally in +// half-open state, which will lead to faster re-opening on errors. // // The windowSize is the time interval over which to calculate the failure rate. // @@ -224,14 +225,20 @@ func (s *SlidingWindowBreaker) observe(halfOpen, failure bool) stateChange { // apply implements Option. func (s *SlidingWindowBreaker) apply(o *options) error { - if o.halfOpenDelay == 0 || o.halfOpenDelay > s.windowSize { - o.halfOpenDelay = s.windowSize - } - if s.threshold < 0 || s.threshold > 1 { return fmt.Errorf("SlidingWindowBreaker threshold must be between 0 and 1") } + switch { + case o.halfOpenDelay == 0: + // Unset: default to the window size, after which the breaker self-heals anyway. + o.halfOpenDelay = s.windowSize + case o.halfOpenDelay > s.windowSize: + // An explicit delay larger than the window would never let the circuit go half-open (the window expires and + // closes it first), so reject it instead of silently discarding the caller's value. + return fmt.Errorf("SlidingWindowBreaker half-open delay (%s) cannot exceed window size (%s)", o.halfOpenDelay, s.windowSize) + } + return nil } diff --git a/options.go b/options.go index a530363..50fa629 100644 --- a/options.go +++ b/options.go @@ -19,6 +19,10 @@ func (f optionFunc) apply(o *options) error { // WithHalfOpenDelay sets the duration the circuit will stay open before switching to the half-open state, where a // limited (~1) amount of calls are allowed that - if successful - may re-close the breaker. +// +// Breakers may require or constrain this value: [EWMABreaker] requires a non-zero delay (it cannot recover without one), +// while [SlidingWindowBreaker] defaults it to its window size and rejects a value exceeding it. Such violations are +// reported as errors by [NewCircuit]. func WithHalfOpenDelay(delay time.Duration) Option { return optionFunc(func(o *options) error { o.halfOpenDelay = delay diff --git a/options_test.go b/options_test.go index 832cf7c..48d2de5 100644 --- a/options_test.go +++ b/options_test.go @@ -37,3 +37,16 @@ func TestWithHalfOpenDelay(t *testing.T) { }) } } + +func TestSlidingWindowBreaker_halfOpenDelay_exceeding_window_errors(t *testing.T) { + _, err := hoglet.NewCircuit( + hoglet.NewSlidingWindowBreaker(time.Second, 0.1), + hoglet.WithHalfOpenDelay(2*time.Second), + ) + require.Error(t, err, "expected error when half-open delay exceeds window size") +} + +func TestSlidingWindowBreaker_halfOpenDelay_unset_defaults_to_window(t *testing.T) { + _, err := hoglet.NewCircuit(hoglet.NewSlidingWindowBreaker(time.Second, 0.1)) + require.NoError(t, err, "unset half-open delay should default to the window size") +}