Skip to content

fix(breaker): let the sliding window recover after a successful half-open call - #36

Merged
costela merged 12 commits into
mainfrom
fix/sliding-window-half-open-recovery
Oct 9, 2026
Merged

costela merged 12 commits into
mainfrom
fix/sliding-window-half-open-recovery

Conversation

@costela

@costela costela commented Oct 7, 2026 •

Copy link
Copy Markdown
Member

Why

After a successful half-open probe, SlidingWindowBreaker closed the circuit but kept the failures that had opened it, so the next call reopened it. A recovered upstream stayed cut off until those failures aged out of the window, regardless of the half-open delay. EWMABreaker already starts fresh after a successful probe.

While reproducing this, window rotation turned out to have two more problems:

  • The rotating observation was weighed against the window from two rotations ago (the value returned by Swap), which made the reopening erratic.
  • Rotation restarted the window at the first observation after it expired, so the ended window always counted in full, however long ago it ended. After an idle period, outdated failures could still reopen the circuit, contrary to the documented expiry.

Reviewing both breakers on top of this turned up three more ways a healthy circuit opens, plus one masked result:

  • EWMABreaker updated its rate with a swap-then-store, so under concurrent load an observer could read another one's raw sample as the previous rate and wipe the history: a single failure opened the circuit. Its "unobserved" sentinel was also reachable by decay, after which the next failure counted as the very first sample.
  • A SlidingWindowBreaker observation racing with a window rotation could read the counts and start of different windows, e.g. judge a lone failure over the near-empty new window, where it is a rate of 1. The five separate atomics could only publish a rotation piece by piece.
  • The watchdog recorded an ignored context error (e.g. IgnoreContextCanceled) as a success, masking whatever the wrapped function then returned.

What

  • Clear the sliding window on a successful half-open call.
  • Rotate the window so the observation is weighed against the window that just ended, by its remaining overlap; drop both windows once more than two window sizes have passed.
  • Update the EWMA rate with a CompareAndSwap loop and move the sentinel out of the valid range.
  • Keep each sliding window together with its predecessor's counts and swap it in as a whole through one atomic pointer, so every call reads a consistent snapshot. Samples added to a window after its successor copied the counts are lost; that only affects calls racing a rotation. Not slower on one CPU, 48-69% faster on 8 and 24 (benchstat, n=10, interleaved runs, including a new benchmark with a 1ms window that rotates ~2400 times per run; command and table in the commit message).
  • Have the watchdog observe failures only; otherwise the wrapped function's return decides. Side effect: an ignored cancellation no longer releases the limiter permit and the in-flight gauge mid-call.
  • Skip storing an unchanged EWMA failure rate, which a healthy breaker settles on (benchstat, n=8: 81.7 → 3.3 ns/op at 8 CPUs, unchanged on one).
  • Share one immutable observer per admitted state, making the uncancellable-context hot path allocation-free (benchstat, n=6: allocs/op 1 → 0 for both breakers, EWMA sec/op -10.7% at p=0.026).
  • Drop a redundant selector flagged by staticcheck so golangci-lint passes.

Trade-off: like EWMA, the circuit now closes on an empty window, so the first failure after closing reopens it.

🤖 Generated with Claude Code

@costela
costela marked this pull request as ready for review October 7, 2026 10:10
@costela
costela requested a balanced review from Copilot October 7, 2026 10:11

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Delayed rotation can give arbitrarily old failures full weight and reopen the breaker after the window has expired.

Review effort: Balanced
Findings: 1 Medium severity

Open (1)
What changed in this PR

Fixes sliding-window recovery and rotation behavior after half-open probes.

Changes:

  • Clears sliding-window counters after a successful probe.
  • Uses the most recently ended window during rotation.
  • Simplifies failure-condition access and adds regression tests.
File Description
breaker.go Resets counters and updates window rotation.
breaker_test.go Adds recovery and rotation tests.
hoglet.go Removes redundant embedded-field selectors.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread breaker.go Outdated
The rotating observation used the value returned by Swap, i.e. the
window from two rotations ago, so it could close or reopen the circuit
based on stale counts.
@costela
costela force-pushed the fix/sliding-window-half-open-recovery branch from 21e2ef2 to ef7e8c0 Compare October 7, 2026 10:22
Rotation restarted the window at the first observation after it
expired, so the window that just ended always counted in full, however
long ago it ended. After an idle period, outdated failures could still
reopen the circuit, contrary to the documented expiry.
Failures from before the circuit opened stayed in the window, so the
call right after a successful half-open probe reopened the circuit.
Recovery took until those failures aged out of the window instead of
one half-open delay.
Fixes staticcheck QF1008 so golangci-lint passes.
@costela
costela force-pushed the fix/sliding-window-half-open-recovery branch from ef7e8c0 to 01a6767 Compare October 7, 2026 10:36
costela and others added 4 commits October 7, 2026 13:32
A swap-then-store let a concurrent observer read the raw sample another
one had just swapped in as the previous failure rate, wiping the whole
history with it: under load a single failure opened a healthy circuit
(4460 spurious opens in 400k observations at 1% failures and a 50%
threshold, see the new concurrent test).

The "unobserved" sentinel moves out of the valid range as well: a long
run of successes decays the rate down to the smallest subnormal float,
which was the sentinel, so the next failure counted as the first sample
ever and opened the circuit regardless of threshold.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…tation

An observer racing with another goroutine's window rotation measured the
elapsed time against the stale window start, so the last window weighed
nothing and its own sample was judged over the near-empty new window: a
single failure there is a failure rate of 1 and opened a healthy circuit
at window boundaries under load. Reading the last window's counts before
the rotating goroutine stores them has the same effect on a young
breaker, or right after a half-open clean slate.

The sample still counts; only the decision is left to the next
observation, which sees consistent windows again. The new test fails on
the old code in 10 of 10 runs.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…rror

The watchdog observed every context error as soon as it occurred, with
the failure condition applied: an ignored one (e.g. context.Canceled
with IgnoreContextCanceled) was thereby recorded as a success, and the
wrapped function's actual result deduplicated away. A real error
returned after a client cancellation never counted.

The watchdog now records failures only and otherwise leaves the
observation to the wrapped function's return. As a side effect, an
ignored cancellation no longer releases the concurrency limiter's permit
and the in-flight gauge while the call is still running.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
… call

A stateObserver is immutable, so handing out the same one to every call
admitted in the same state saves the only allocation left on the hot
path of calls with an uncancellable context.

benchstat over 6 runs each of
  go test -bench 'BenchmarkHoglet_Do' -benchmem -count=6
allocs/op 1 → 0 for both breakers; EWMA sec/op -10.7% (p=0.026), sliding
window unchanged within noise.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Comment thread breaker.go Outdated
costela and others added 4 commits October 8, 2026 22:09
A healthy EWMA settles on a fixed point (0, or a subnormal for larger
sample counts), after which every call stored the same bits again with
a CompareAndSwap, bouncing the cache line between cores for nothing.
Skipping it is equivalent to the store happening at the moment of the
load.

benchstat, n=8, before/after; the benchmark sits on the fixed point
from its first call, real traffic only after a long healthy stretch:
  go test -run '^$' -bench BenchmarkHoglet_Do_EWMA -benchmem -count=8 -cpu 1,8 .
  Hoglet_Do_EWMA      26.78n ± 0%   26.74n ± 4%       ~ (p=0.739)
  Hoglet_Do_EWMA-8    81.70n ± 9%    3.30n ± 1%  -95.96% (p=0.000)

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Fold the three single-rotation tests into one table, name the
concurrent rotation test after what it asserts, reuse the noop helper
in the watchdog tests and shorten the EWMA stress test, which still
caught the old swap-then-store update in 20 of 20 runs with and without
the race detector at a tenth of the iterations.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The existing benchmark's 10s window never rotates within a run, so it
cannot show what rotation costs. A 1ms window rotates on every elapsed
window; the reported "windows" metric shows how many a run spans.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The two windows lived in five separate atomics, so a rotation was
published piece by piece: a call that read the new window start before
the rotating goroutine had stored the last window's counts judged its
sample against stale ones. The straddle check could not catch that, and
the concurrent rotation test still failed 5 of 300 race-detector runs.
Stale counts hurt most right after a successful half-open call, when
they are the failures that opened the circuit.

A window now carries its start and the counts of the one before it, and
a rotation swaps it in whole through one atomic pointer, so every call
reads a consistent snapshot and the straddle check goes away. Samples
added to a window after its successor copied the counts are lost, which
only affects calls racing a rotation. The rotation test passed 1200 of
1200 runs with and without the race detector.

benchstat, n=10, old and new test binaries run interleaved, 2s each;
the rotating benchmark spans ~2400 windows per run (instrumented copies
counted one rotation per elapsed window):
  ./old.test|./new.test -test.run '^$' -test.bench BenchmarkHoglet_Do_ \
    -test.benchmem -test.benchtime 2s -test.cpu 1,8,24 -test.count 1
  Hoglet_Do_SlidingWindow              103.2n   103.1n        ~ (p=0.062)
  Hoglet_Do_SlidingWindow-8            105.9n    52.1n  -50.85% (p=0.000)
  Hoglet_Do_SlidingWindow-24            92.1n    28.5n  -69.10% (p=0.000)
  Hoglet_Do_SlidingWindow_rotating     103.4n   103.2n        ~ (p=0.642)
  Hoglet_Do_SlidingWindow_rotating-8   105.6n    54.6n  -48.26% (p=0.000)
  Hoglet_Do_SlidingWindow_rotating-24   94.1n    40.2n  -57.27% (p=0.000)
allocs/op stay 0. The unchanged EWMA benchmark, as a control, moved by
up to +1.89%, which marks the noise floor.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@costela
costela requested a review from vietuoso October 9, 2026 08:18
@costela
costela merged commit 3916a6c into main Oct 9, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants