From 26d246ff976e2a9ed48afa72b1e293ae3f3f2ed1 Mon Sep 17 00:00:00 2001 From: Leo Antunes Date: Thu, 26 Mar 2026 12:19:00 +0100 Subject: [PATCH 1/3] fix: guard isFailure calls with nil check so failure conditions don't receive nil errors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Nil errors are now always treated as successes before calling the user-provided failure condition function. This simplifies failure condition implementations (e.g. IgnoreContextCanceled) by removing the need to handle the nil case. Also applies minor Go style cleanups: rename sentinel → errSentinel, use `any` instead of `interface{}`, and remove unnecessary loop variable copies (Go 1.22+). Co-Authored-By: Claude Opus 4.6 (1M context) --- breaker_test.go | 2 -- hoglet.go | 4 ++-- hoglet_test.go | 18 +++++++++--------- options.go | 4 ++-- 4 files changed, 13 insertions(+), 15 deletions(-) diff --git a/breaker_test.go b/breaker_test.go index b8192d6..3a4f9c0 100644 --- a/breaker_test.go +++ b/breaker_test.go @@ -167,9 +167,7 @@ func TestBreaker_Observe_State(t *testing.T) { }, } for _, tt := range tests { - tt := tt for bName, b := range tt.breakers { - b := b t.Run(bName+": "+tt.name, func(t *testing.T) { t.Parallel() diff --git a/hoglet.go b/hoglet.go index 4aa0d8f..1c41742 100644 --- a/hoglet.go +++ b/hoglet.go @@ -236,7 +236,7 @@ func Wrap[IN, OUT any](c *Circuit, f WrappableFunc[IN, OUT]) WrappableFunc[IN, O obs.Observe(true) panic(err) // let the caller deal with panics } - obs.Observe(c.options.isFailure(err)) + obs.Observe(err != nil && c.options.isFailure(err)) }() return f(ctx, in) @@ -257,7 +257,7 @@ func (c *Circuit) observeCtx(obs Observer, ctx context.Context) { if context.Cause(ctx) == errWrappedFunctionDone { err = nil // ignore internal cancellations; the wrapped function returned already } - obs.Observe(c.options.isFailure(err)) + obs.Observe(err != nil && c.options.isFailure(err)) } // State represents the state of a circuit. diff --git a/hoglet_test.go b/hoglet_test.go index e445a72..9b9cb70 100644 --- a/hoglet_test.go +++ b/hoglet_test.go @@ -10,7 +10,7 @@ import ( "github.com/stretchr/testify/require" ) -var sentinel = errors.New("sentinel error") +var errSentinel = errors.New("sentinel error") type noopIn int @@ -26,7 +26,7 @@ func noop(ctx context.Context, in noopIn) (struct{}, error) { case noopInSuccess: return struct{}{}, nil case noopInFailure: - return struct{}{}, sentinel + return struct{}{}, errSentinel default: // noopInPanic panic("boom") } @@ -80,9 +80,9 @@ func TestBreaker_nil_breaker_does_not_open(t *testing.T) { b, err := NewCircuit(nil) require.NoError(t, err) _, err = Wrap(b, noop)(t.Context(), noopInFailure) - assert.Equal(t, sentinel, err) + assert.Equal(t, errSentinel, err) _, err = Wrap(b, noop)(t.Context(), noopInFailure) - assert.Equal(t, sentinel, err) + assert.Equal(t, errSentinel, err) } func TestBreaker_ctx_parameter_not_cancelled(t *testing.T) { @@ -152,7 +152,7 @@ func TestHoglet_Do(t *testing.T) { name: "error opens", calls: []calls{ {arg: noopInSuccess, wantErr: nil}, - {arg: noopInFailure, wantErr: sentinel}, + {arg: noopInFailure, wantErr: errSentinel}, {arg: noopInSuccess, wantErr: ErrCircuitOpen}, }, }, @@ -168,7 +168,7 @@ func TestHoglet_Do(t *testing.T) { name: "success on half-open closes", calls: []calls{ {arg: noopInSuccess, wantErr: nil}, - {arg: noopInFailure, wantErr: sentinel}, + {arg: noopInFailure, wantErr: errSentinel}, {arg: noopInSuccess, wantErr: nil, halfOpen: true}, {arg: noopInSuccess, wantErr: nil}, }, @@ -177,8 +177,8 @@ func TestHoglet_Do(t *testing.T) { name: "failure on half-open keeps open", calls: []calls{ {arg: noopInSuccess, wantErr: nil}, - {arg: noopInFailure, wantErr: sentinel}, - {arg: noopInFailure, wantErr: sentinel, halfOpen: true}, + {arg: noopInFailure, wantErr: errSentinel}, + {arg: noopInFailure, wantErr: errSentinel, halfOpen: true}, {arg: noopInSuccess, wantErr: ErrCircuitOpen}, }, }, @@ -211,7 +211,7 @@ func TestHoglet_Do(t *testing.T) { func maybeAssertPanic(t *testing.T, f func(), wantPanic any) { wrapped := assert.NotPanics if wantPanic != nil { - wrapped = func(t assert.TestingT, f assert.PanicTestFunc, msgAndArgs ...interface{}) bool { + wrapped = func(t assert.TestingT, f assert.PanicTestFunc, msgAndArgs ...any) bool { return assert.PanicsWithValue(t, wantPanic, f, msgAndArgs...) } } diff --git a/options.go b/options.go index d8cba1d..9cf5220 100644 --- a/options.go +++ b/options.go @@ -29,7 +29,7 @@ func WithHalfOpenDelay(delay time.Duration) Option { // WithFailureCondition allows specifying a filter function that determines whether an error should open the breaker. // If the provided function returns true, the error is considered a failure and the breaker may open (depending on the // breaker logic). -// The default filter considers all non-nil errors as failures (err != nil). +// Nil errors are always considered successes. The provided function is only called in the non-nil error case. // // This does not modify the error returned by [Circuit.Call]. It only affects the circuit itself. func WithFailureCondition(condition func(error) bool) Option { @@ -41,7 +41,7 @@ func WithFailureCondition(condition func(error) bool) Option { // IgnoreContextCanceled is a helper function for [WithFailureCondition] that ignores [context.Canceled] errors. func IgnoreContextCanceled(err error) bool { - return err != nil && !errors.Is(err, context.Canceled) + return !errors.Is(err, context.Canceled) } // WithBreakerMiddleware allows wrapping the [Breaker] via a [BreakerMiddleware]. From eb07da3a80942cd417812bcaf85d6072a713fcb5 Mon Sep 17 00:00:00 2001 From: Leo Antunes Date: Thu, 26 Mar 2026 12:27:14 +0100 Subject: [PATCH 2/3] doc: avoid mention of non-existant Circuit.Call Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- options.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/options.go b/options.go index 9cf5220..a530363 100644 --- a/options.go +++ b/options.go @@ -31,7 +31,7 @@ func WithHalfOpenDelay(delay time.Duration) Option { // breaker logic). // Nil errors are always considered successes. The provided function is only called in the non-nil error case. // -// This does not modify the error returned by [Circuit.Call]. It only affects the circuit itself. +// This does not modify the error returned by the wrapped function call. It only affects the circuit itself. func WithFailureCondition(condition func(error) bool) Option { return optionFunc(func(o *options) error { o.isFailure = condition From b4a25961b2a5324e4b103df74708e989a2cbb8be Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 26 Mar 2026 11:30:11 +0000 Subject: [PATCH 3/3] test: add regression test that failure condition is never called with nil error Co-authored-by: costela <94699+costela@users.noreply.github.com> Agent-Logs-Url: https://github.com/exaring/hoglet/sessions/62208c78-9047-4dc5-9ff2-e590c8dd3af3 --- hoglet_test.go | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/hoglet_test.go b/hoglet_test.go index 9b9cb70..315c5e6 100644 --- a/hoglet_test.go +++ b/hoglet_test.go @@ -207,6 +207,22 @@ func TestHoglet_Do(t *testing.T) { } } +func TestCircuit_failure_condition_never_called_with_nil_error(t *testing.T) { + conditionCalled := false + condition := func(err error) bool { + conditionCalled = true + require.NotNil(t, err, "failure condition must not be called with nil error") + return true + } + + b, err := NewCircuit(nil, WithFailureCondition(condition)) + require.NoError(t, err) + + _, err = Wrap(b, noop)(t.Context(), noopInSuccess) + require.NoError(t, err) + assert.False(t, conditionCalled, "failure condition should not have been called for a successful call") +} + // maybeAssertPanic is a test-table helper to assert that a function panics or not, depending on the value of wantPanic. func maybeAssertPanic(t *testing.T, f func(), wantPanic any) { wrapped := assert.NotPanics