From ccfbfdaafa9120e661243b45d1586390b739226a Mon Sep 17 00:00:00 2001 From: snowkide Date: Wed, 24 Jun 2026 18:49:08 +0200 Subject: [PATCH] fix(failsafe): release half-open permit on ignored outcome (breaker wedge) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Root cause of the internal-eRPC "probe/selection wedge" (incident 2026-06-24). Breaker.Record returned early for OutcomeIgnore BEFORE the HalfOpen branch that releases the trial permit (halfOpenInflight--). A HalfOpen trial reserves a permit in TryAcquirePermit; if that trial resolves as ignorable (timeout, cancellation, soft error — precisely what a transient redis/upstream blip produces), the permit was never released. After enough such trials halfOpenInflight saturates the trial capacity, every subsequent TryAcquirePermit in HalfOpen is denied, and the breaker wedges open indefinitely — failing real traffic AND the selection-recovery probes (which are breaker-eligible). The upstream can never re-admit, so eRPC serves no healthy upstream for the chain until the pods are rollout-restarted (which resets in-memory breaker state). Evidence: during a wedge the selection-probe error RATIO sits at ~1.0 (every probe denied) sustained for tens of minutes across multiple chains at once, recovering within minutes of a restart; onset correlates with redis-haproxy churn. Fix: on OutcomeIgnore, still release a reserved HalfOpen trial permit (without counting it as success/failure — the trial was inconclusive). Minimal, behaviour- preserving for Closed/Open. Test: breaker_test.go reproduces the leak — without the fix the breaker "wedges after 0 ignored trials" (halfOpenInflight leaks to 1 and TryAcquirePermit denies); with the fix, repeated ignored trials never wedge and a later success still closes. Follow-ups (separate): mark selection-recovery probes breaker-ineligible so a genuinely-open breaker can't blind its own recovery probe; bounded HalfOpen dwell / cordon TTL as defence-in-depth. Co-Authored-By: Claude Opus 4.8 (1M context) --- failsafe/breaker.go | 21 ++++++++++- failsafe/breaker_test.go | 80 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 99 insertions(+), 2 deletions(-) create mode 100644 failsafe/breaker_test.go diff --git a/failsafe/breaker.go b/failsafe/breaker.go index e17b182ff..0e802bfa9 100644 --- a/failsafe/breaker.go +++ b/failsafe/breaker.go @@ -176,9 +176,26 @@ func (b *Breaker) TryAcquirePermit() bool { } // Record applies the given outcome to the breaker's state machine. -// OutcomeIgnore is a no-op. +// OutcomeIgnore does not move the success/failure counters, but it MUST still +// release a HalfOpen trial permit that TryAcquirePermit reserved — otherwise +// halfOpenInflight leaks. Once it saturates the trial capacity, every +// TryAcquirePermit in HalfOpen is denied and the breaker wedges open +// indefinitely, failing real traffic AND the selection-recovery probes until +// the process is restarted. Ignorable outcomes (timeouts, cancellations, soft +// errors) are exactly what a transient dependency blip produces during a trial, +// so this leak is the production "probe/selection wedge" failure mode. func (b *Breaker) Record(o Outcome) { - if b == nil || o == OutcomeIgnore { + if b == nil { + return + } + if o == OutcomeIgnore { + b.mu.Lock() + if State(b.state.Load()) == StateHalfOpen && b.halfOpenInflight > 0 { + // Release the reserved trial permit without counting the outcome as + // a success or failure — the trial was inconclusive, not passed. + b.halfOpenInflight-- + } + b.mu.Unlock() return } b.mu.Lock() diff --git a/failsafe/breaker_test.go b/failsafe/breaker_test.go new file mode 100644 index 000000000..e1020300a --- /dev/null +++ b/failsafe/breaker_test.go @@ -0,0 +1,80 @@ +package failsafe + +import ( + "testing" + "time" + + "github.com/erpc/erpc/common" + "github.com/rs/zerolog" + "github.com/stretchr/testify/require" +) + +func newWedgeTestBreaker() *Breaker { + cfg := &common.CircuitBreakerPolicyConfig{ + FailureThresholdCount: 1, + FailureThresholdCapacity: 1, + SuccessThresholdCount: 1, + SuccessThresholdCapacity: 1, + HalfOpenAfter: common.Duration(1 * time.Millisecond), + } + lg := zerolog.Nop() + return NewBreaker(cfg, &lg) +} + +func openThenHalfOpen(t *testing.T, b *Breaker) { + t.Helper() + require.True(t, b.TryAcquirePermit()) + b.Record(OutcomeFailure) // one failure in Closed opens (threshold 1) + require.Equal(t, StateOpen, State(b.state.Load()), "breaker should be open after a failure") + time.Sleep(5 * time.Millisecond) // exceed HalfOpenAfter + require.True(t, b.TryAcquirePermit(), "first half-open trial permit must be granted") + require.Equal(t, StateHalfOpen, State(b.state.Load())) +} + +// TestBreaker_HalfOpenPermitReleasedOnIgnoredOutcome is the regression for the +// production probe/selection wedge (incident 2026-06-24): a HalfOpen trial that +// returns an IGNORABLE outcome (timeout / cancellation / soft error) must +// release the trial permit it reserved in TryAcquirePermit. If it doesn't, +// halfOpenInflight leaks, saturates the trial capacity, and TryAcquirePermit +// denies every subsequent caller — wedging the breaker open and failing both +// real traffic AND the selection-recovery probes until a process restart. +func TestBreaker_HalfOpenPermitReleasedOnIgnoredOutcome(t *testing.T) { + b := newWedgeTestBreaker() + openThenHalfOpen(t, b) + + // The trial is inconclusive (e.g. the request was canceled or timed out — + // exactly what a transient redis/upstream blip produces). + b.Record(OutcomeIgnore) + + b.mu.Lock() + inflight, hoSucc, hoFail := b.halfOpenInflight, b.halfOpenSuccess, b.halfOpenFailure + b.mu.Unlock() + + require.Equal(t, 0, inflight, "ignored half-open outcome must release the trial permit (no leak)") + require.Equal(t, 0, hoSucc, "ignored outcome must not count as a half-open success") + require.Equal(t, 0, hoFail, "ignored outcome must not count as a half-open failure") + require.True(t, b.TryAcquirePermit(), + "breaker must not be wedged: a fresh trial permit must be grantable after an ignored outcome") +} + +// TestBreaker_RepeatedIgnoredTrialsDoNotWedge proves the breaker keeps offering +// trial permits across many inconclusive trials, and a real success still closes +// it. Before the fix this Fatals on the first iteration (permit leaked → denied). +func TestBreaker_RepeatedIgnoredTrialsDoNotWedge(t *testing.T) { + b := newWedgeTestBreaker() + openThenHalfOpen(t, b) + // First reserved permit (from openThenHalfOpen) resolves as ignored. + b.Record(OutcomeIgnore) + + for i := 0; i < 50; i++ { + if !b.TryAcquirePermit() { + t.Fatalf("breaker wedged after %d ignored trials: half-open permit leaked", i) + } + b.Record(OutcomeIgnore) + } + + require.True(t, b.TryAcquirePermit(), "trial permit still grantable") + b.Record(OutcomeSuccess) + require.Equal(t, StateClosed, State(b.state.Load()), + "a successful trial must still close the breaker after a run of ignored trials") +}