connor4312 / cockatiel

🐦 A resilience and fault-handling library. Supports Backoffs, Retries, Circuit Breakers, Timeouts, Bulkhead Isolation, and Fallbacks.
MIT License
1.59k stars 50 forks source link

Issue With The Sampling Breaker #64

Closed aayush-agrawal closed 2 years ago

aayush-agrawal commented 2 years ago

consider the following sampling breaker const breaker = new SamplingBreaker({ threshold: 0.2, duration: 5000, minimumRps: 1 });

If I make 4 continuous failure calls and goes to grab a cup of coffee, after 5 min I make another 2 failure calls, then in that case the circuit should not be opened. But currently the circuit moves to open state.

const breaker = circuitBreaker( handleAll, { halfOpenAfter: 10 * 1000, breaker: new SamplingBreaker( { threshold: 0.2, duration: 5000, minimumRps: 1 } ), } );

for( let i= 0; i < 5; i++ ) { await expectAsync( breaker.execute( () => Promise.reject( "" ) ) ).toBeRejectedWith( "" ); } jasmine.clock().tick( 20_000 ); await expectAsync( breaker.execute( () => Promise.reject( "" ) ) ).toBeRejectedWith( new BrokenCircuitError('Execution prevented because the circuit breaker is open') );

The test case should not pass.

connor4312 commented 2 years ago

The circuit should be in an open state after making another 2 failure calls. The logic of the breaker when the circuit it open is irrelevant: a call must succeed when the circuit is in its half-open state in order for it to close. See the Polly docs for a nice diagram on this.


When I put that test case into our test suite, it does indeed fail (I also tried replacing the fake clock with a real setTimeout and observed the same behavior)

it('fixes #64', async () => {
  const breaker = circuitBreaker(handleAll, {
    halfOpenAfter: 10 * 1000,
    breaker: new SamplingBreaker({ threshold: 0.2, duration: 5000, minimumRps: 1 }),
  });

  breaker.onStateChange(s => console.log('state =', s));

  for (let i = 0; i < 5; i++) {
    await expect(breaker.execute(() => Promise.reject(''))).to.be.rejectedWith('');
  }
  clock.tick(20_000);
  await expect(breaker.execute(() => Promise.reject(''))).to.be.rejectedWith(
    new BrokenCircuitError('Execution prevented because the circuit breaker is open'),
  );
});

With the console log, I observe what I would expect to see:

  1. The failing calls cause the breaker to open
  2. Time passes. The circuit breaker is allowed to half-open...
  3. The next call comes in. It starts a test call into the inner function...
  4. The inner call fails again, and the circuit opens

Let me know if you have any other questions or confusion.