From 55a8bc005923dac77f3b01696925914785fb8bc0 Mon Sep 17 00:00:00 2001 From: Pradeep Kintali Date: Wed, 23 Sep 2026 16:13:15 -0700 Subject: [PATCH 1/3] Require stable Freno recovery After a rejection, require consecutive healthy checks before releasing work and keep normal sustained throttling out of the circuit breaker failure path. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0fbc396f-32f9-449a-8537-6264e8665c31 --- README.md | 14 ++++++- lib/freno/throttler.rb | 54 +++++++++++++++++++++---- test/freno/throttler_test.rb | 78 ++++++++++++++++++++++++++++++++++-- 3 files changed, 133 insertions(+), 13 deletions(-) diff --git a/README.md b/README.md index c61b91f..73176eb 100644 --- a/README.md +++ b/README.md @@ -202,6 +202,7 @@ module Freno DEFAULT_WAIT_SECONDS = 0.5 DEFAULT_MAX_WAIT_SECONDS = 10 + DEFAULT_REQUIRED_CONSECUTIVE_SUCCESSES = 3 def initialize(client: nil, app: nil, @@ -209,7 +210,8 @@ module Freno instrumenter: Instrumenter::Noop, circuit_breaker: CircuitBreaker::Noop, wait_seconds: DEFAULT_WAIT_SECONDS, - max_wait_seconds: DEFAULT_MAX_WAIT_SECONDS) + max_wait_seconds: DEFAULT_MAX_WAIT_SECONDS, + required_consecutive_successes: DEFAULT_REQUIRED_CONSECUTIVE_SUCCESSES) @client = client @@ -238,6 +240,16 @@ You optionally provide the time you want the throttler to sleep in case the chec If replication lags badly, you can control until when you want to keep sleeping and retrying the check by setting `max_wait_seconds`. When that times out, the throttle will raise a `Freno::Throttler::WaitedTooLong` error. +An initially healthy check proceeds immediately. After any failed check, +`required_consecutive_successes` passing checks are required before the block +runs. Passing samples must be consecutive; another rejection resets the count. +This prevents an oscillating metric from releasing work on a single healthy +trough. + +`WaitedTooLong` represents normal sustained throttling and does not count as a +circuit-breaker failure. Freno transport and decision errors still fail the +circuit breaker. + #### Instrumenting the throttler You can also configure the throttler with an `instrumenter` collaborator to subscribe to events happening during the `throttle` call. diff --git a/lib/freno/throttler.rb b/lib/freno/throttler.rb index aec22bb..78d0fda 100644 --- a/lib/freno/throttler.rb +++ b/lib/freno/throttler.rb @@ -35,6 +35,7 @@ module Freno class Throttler DEFAULT_WAIT_SECONDS = 0.5 DEFAULT_MAX_WAIT_SECONDS = 10 + DEFAULT_REQUIRED_CONSECUTIVE_SUCCESSES = 3 REQUIRED_ARGS = %i[ client app @@ -43,6 +44,7 @@ class Throttler circuit_breaker wait_seconds max_wait_seconds + required_consecutive_successes ].freeze attr_accessor :client, @@ -51,7 +53,8 @@ class Throttler :instrumenter, :circuit_breaker, :wait_seconds, - :max_wait_seconds + :max_wait_seconds, + :required_consecutive_successes # Initializes a new instance of the throttler # @@ -99,6 +102,10 @@ class Throttler # seconds the throttler will wait in total for replicas to catch-up # before raising a `WaitedTooLong` error. # + # - `:required_consecutive_successes`: The number of consecutive passing + # checks required after any failed check. An initially passing check + # still proceeds immediately. + # def initialize( client: nil, app: nil, @@ -106,7 +113,8 @@ def initialize( instrumenter: Instrumenter::Noop, circuit_breaker: CircuitBreaker::Noop, wait_seconds: DEFAULT_WAIT_SECONDS, - max_wait_seconds: DEFAULT_MAX_WAIT_SECONDS + max_wait_seconds: DEFAULT_MAX_WAIT_SECONDS, + required_consecutive_successes: DEFAULT_REQUIRED_CONSECUTIVE_SUCCESSES ) @client = client @app = app @@ -115,6 +123,7 @@ def initialize( @circuit_breaker = circuit_breaker @wait_seconds = wait_seconds @max_wait_seconds = max_wait_seconds + @required_consecutive_successes = required_consecutive_successes yield self if block_given? @@ -165,6 +174,8 @@ def throttle(context = nil, **options) store_names = mapper.call(context) instrument(:called, store_names: store_names) waited = 0 + throttled = false + consecutive_successes = 0 while true unless circuit_breaker.allow_request? @@ -173,19 +184,43 @@ def throttle(context = nil, **options) end if all_stores_ok?(store_names, **options) - instrument(:succeeded, store_names: store_names, waited: waited) - circuit_breaker.success - break + consecutive_successes += 1 + if !throttled || consecutive_successes >= required_consecutive_successes + instrument( + :succeeded, + store_names: store_names, + waited: waited, + consecutive_successes: consecutive_successes + ) + circuit_breaker.success + break + end + else + throttled = true + consecutive_successes = 0 end if waited + wait_seconds > max_wait_seconds - instrument(:waited_too_long, store_names: store_names, waited: waited, max: max_wait_seconds) - circuit_breaker.failure + instrument( + :waited_too_long, + store_names: store_names, + waited: waited, + max: max_wait_seconds, + consecutive_successes: consecutive_successes, + required_consecutive_successes: required_consecutive_successes + ) raise WaitedTooLong.new(waited_seconds: waited, max_wait_seconds: max_wait_seconds) else wait waited += wait_seconds - instrument(:waited, store_names: store_names, waited: waited, max: max_wait_seconds) + instrument( + :waited, + store_names: store_names, + waited: waited, + max: max_wait_seconds, + consecutive_successes: consecutive_successes, + required_consecutive_successes: required_consecutive_successes + ) end end @@ -204,6 +239,9 @@ def validate_args unless max_wait_seconds > wait_seconds errors << "max_wait_seconds (#{max_wait_seconds}) has to be greather than wait_seconds (#{wait_seconds})" end + unless required_consecutive_successes.is_a?(Integer) && required_consecutive_successes.positive? + errors << "required_consecutive_successes (#{required_consecutive_successes}) must be a positive integer" + end raise ArgumentError, errors.join("\n") if errors.any? end diff --git a/test/freno/throttler_test.rb b/test/freno/throttler_test.rb index 7053983..01e0e08 100644 --- a/test/freno/throttler_test.rb +++ b/test/freno/throttler_test.rb @@ -10,6 +10,15 @@ def test_validations assert_includes ex.message, "app must be provided" assert_includes ex.message, "client must be provided" assert_includes ex.message, "max_wait_seconds (0.5) has to be greather than wait_seconds (1)" + + ex = assert_raises(ArgumentError) do + Freno::Throttler.new( + client: sample_client, + app: :github, + required_consecutive_successes: 0 + ) + end + assert_includes ex.message, "required_consecutive_successes (0) must be a positive integer" end def test_using_the_default_identity_mapper @@ -80,9 +89,9 @@ def test_sleeps_when_a_check_fails_and_then_calls_the_block block_called = false stub = sample_client - stub.expects(:check?).times(2) + stub.expects(:check?).times(4) .with(app: :github, store_name: :mysqla, options: {}) - .returns(false).then.returns(true) + .returns(false).then.returns(true).then.returns(true).then.returns(true) throttler = Freno::Throttler.new do |t| t.client = stub @@ -90,7 +99,7 @@ def test_sleeps_when_a_check_fails_and_then_calls_the_block t.mapper = ->(_context) { [:mysqla] } t.instrumenter = MemoryInstrumenter.new end - throttler.expects(:wait).once + throttler.expects(:wait).times(3) throttler.throttle do block_called = true @@ -105,10 +114,12 @@ def test_sleeps_when_a_check_fails_and_then_calls_the_block waited_events = throttler.instrumenter.events_for("throttler.waited") - assert_equal 1, waited_events.count + assert_equal 3, waited_events.count assert_equal [:mysqla], waited_events.first[:store_names] assert_in_delta 0.5, waited_events.first[:waited], 0.01 assert_equal 10, waited_events.first[:max] + assert_equal 3, waited_events.last[:required_consecutive_successes] + assert_equal 2, waited_events.last[:consecutive_successes] assert_equal 0, throttler.instrumenter.count("throttler.waited_too_long") assert_equal 0, throttler.instrumenter.count("throttler.freno_errored") @@ -158,6 +169,65 @@ def test_raises_waited_too_long_if_freno_checks_failed_consistenly assert_equal 0, throttler.instrumenter.count("throttler.circuit_open") end + def test_recovery_requires_consecutive_passing_checks + block_called = false + + stub = sample_client + stub.expects(:check?).times(7) + .with(app: :github, store_name: :mysqla, options: {}) + .returns(false) + .then.returns(true) + .then.returns(false) + .then.returns(true) + .then.returns(true) + .then.returns(false) + .then.returns(true) + + throttler = Freno::Throttler.new do |t| + t.client = stub + t.app = :github + t.mapper = ->(_context) { [:mysqla] } + t.instrumenter = MemoryInstrumenter.new + t.wait_seconds = 1 + t.max_wait_seconds = 6 + end + throttler.expects(:wait).times(6) + + assert_raises(Freno::Throttler::WaitedTooLong) do + throttler.throttle do + block_called = true + end + end + + refute block_called + + event = throttler.instrumenter.events_for("throttler.waited_too_long").first + + assert_equal 1, event[:consecutive_successes] + assert_equal 3, event[:required_consecutive_successes] + end + + def test_waited_too_long_does_not_fail_the_circuit_breaker + client = sample_client + client.stubs(:check?).returns(false) + circuit_breaker = mock + circuit_breaker.stubs(:allow_request?).returns(true) + circuit_breaker.expects(:failure).never + + throttler = Freno::Throttler.new( + client: client, + app: :github, + circuit_breaker: circuit_breaker, + wait_seconds: 1, + max_wait_seconds: 2 + ) + throttler.expects(:wait).times(2) + + assert_raises(Freno::Throttler::WaitedTooLong) do + throttler.throttle(:mysqla) { flunk "throttled block should not run" } + end + end + def test_raises_a_specific_error_in_case_freno_itself_errored block_called = false From 9b64e00ac9af25d91df11a717f838e3748a40fba Mon Sep 17 00:00:00 2001 From: Pradeep Kintali Date: Thu, 24 Sep 2026 10:50:41 -0700 Subject: [PATCH 2/3] Fix RuboCop violations Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0fbc396f-32f9-449a-8537-6264e8665c31 --- lib/freno/client.rb | 2 +- lib/freno/client/errors.rb | 3 ++- lib/freno/client/preconditions.rb | 3 ++- 3 files changed, 5 insertions(+), 3 deletions(-) diff --git a/lib/freno/client.rb b/lib/freno/client.rb index 36505e1..4674bb5 100644 --- a/lib/freno/client.rb +++ b/lib/freno/client.rb @@ -233,7 +233,7 @@ def decorated(request) outermost = to_decorate[0] current = outermost - (to_decorate[1..]).each do |decorator| + to_decorate[1..].each do |decorator| current.request = decorator current = current.request end diff --git a/lib/freno/client/errors.rb b/lib/freno/client/errors.rb index afbb591..fbbb7a1 100644 --- a/lib/freno/client/errors.rb +++ b/lib/freno/client/errors.rb @@ -1,5 +1,6 @@ # frozen_string_literal: true module Freno - Error = Class.new(StandardError) + class Error < StandardError + end end diff --git a/lib/freno/client/preconditions.rb b/lib/freno/client/preconditions.rb index a49174b..2c5ed55 100644 --- a/lib/freno/client/preconditions.rb +++ b/lib/freno/client/preconditions.rb @@ -5,7 +5,8 @@ class Client module Preconditions module_function - PreconditionNotMet = Class.new(ArgumentError) + class PreconditionNotMet < ArgumentError + end class Checker attr_reader :errors From 6c560ef266c68bf3d8f1358aa749f619b653f3fb Mon Sep 17 00:00:00 2001 From: Pradeep Kintali Date: Mon, 28 Sep 2026 14:49:24 -0700 Subject: [PATCH 3/3] Address stable recovery review feedback Correct the documented configuration and circuit-breaker behavior, clarify instrumentation semantics, and cover non-default recovery thresholds. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0fbc396f-32f9-449a-8537-6264e8665c31 --- README.md | 6 +++++- lib/freno/throttler.rb | 10 +++++----- test/freno/throttler_test.rb | 25 +++++++++++++++++++++++++ 3 files changed, 35 insertions(+), 6 deletions(-) diff --git a/README.md b/README.md index 73176eb..b3e7fbf 100644 --- a/README.md +++ b/README.md @@ -221,6 +221,7 @@ module Freno @circuit_breaker = circuit_breaker @wait_seconds = wait_seconds @max_wait_seconds = max_wait_seconds + @required_consecutive_successes = required_consecutive_successes yield self if block_given? @@ -299,7 +300,10 @@ The throttler can also receive a `circuit_breaker` object to implement resilienc With that information it receives, the circuit breaker determines whether or not to allow the next request. A circuit is said to be open when the next request is not allowed; and it's said to be closed when the next request is allowed -If the throttler waited too long, or an unexpected error happened; the circuit breaker will receive a `failure`. If in contrast it succeeded, the circuit breaker will receive a `success` message. +If an unexpected Freno transport or decision error happens, the circuit breaker +receives a `failure`. Sustained throttling that raises `WaitedTooLong` does not +fail the circuit breaker. When a check succeeds, the circuit breaker receives a +`success` message. Once the circuit is open, the throttler will not try to throttle calls, an instead throw a `Freno::Throttler::CircuitOpen` diff --git a/lib/freno/throttler.rb b/lib/freno/throttler.rb index 78d0fda..fe04826 100644 --- a/lib/freno/throttler.rb +++ b/lib/freno/throttler.rb @@ -160,11 +160,11 @@ def initialize( # # - "throttler.called" each time this method is called # - "throttler.succeeded" when the stores were ok, before yielding the block - # - "throttler.waited" when the stores were not ok, after waiting - # `wait_seconds` - # - "throttler.waited_too_long" when the stores were not ok, but the - # thottler already waited at least `max_wait_seconds`, right before - # raising `WaitedTooLong` + # - "throttler.waited" after waiting `wait_seconds` because stores were not + # ok or recovery had not yet reached the consecutive-success threshold + # - "throttler.waited_too_long" when stores remain unavailable or recovery + # stabilization exceeds `max_wait_seconds`, right before raising + # `WaitedTooLong` # - "throttler.freno_errored" when there was an error with freno, before # raising `ClientError`. # - "throttler.circuit_open" when the circuit breaker does not allow the diff --git a/test/freno/throttler_test.rb b/test/freno/throttler_test.rb index 01e0e08..9fb1d44 100644 --- a/test/freno/throttler_test.rb +++ b/test/freno/throttler_test.rb @@ -207,6 +207,31 @@ def test_recovery_requires_consecutive_passing_checks assert_equal 3, event[:required_consecutive_successes] end + def test_recovery_uses_configured_consecutive_success_threshold + block_called = false + client = sample_client + client.expects(:check?).times(3) + .with(app: :github, store_name: :mysqla, options: {}) + .returns(false).then.returns(true).then.returns(true) + + throttler = Freno::Throttler.new( + client: client, + app: :github, + instrumenter: MemoryInstrumenter.new, + required_consecutive_successes: 2 + ) + throttler.expects(:wait).times(2) + + throttler.throttle(:mysqla) do + block_called = true + end + + assert block_called + event = throttler.instrumenter.events_for("throttler.succeeded").first + + assert_equal 2, event[:consecutive_successes] + end + def test_waited_too_long_does_not_fail_the_circuit_breaker client = sample_client client.stubs(:check?).returns(false)