Skip to content

misc cleanup of dual circuit breaker (Dec 19) #947

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
adriangudas wants to merge 2 commits into
base: pid-take-2
Choose a base branch
from
+39 −18
Draft

misc cleanup of dual circuit breaker (Dec 19) #947

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ee006e8
Fix DRY in dual circuit breaker child classes
adriangudas Dec 19, 2025
1408498
add test to ensure dual circuit breaker isn't used when not specified
adriangudas Dec 23, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions examples/dual_circuit_breaker_demo.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
# simultaneously, switching between them at runtime based on a callable.

# Simulate a feature flag that can be toggled
module ExperimentFlags
class ExperimentFlags
@enabled = false

def enable_adaptive!
Expand Down Expand Up @@ -82,7 +82,7 @@ def print_semian_state
)

experiment_flags = ExperimentFlags.new
Semian::DualCircuitBreaker.adaptive_circuit_breaker_selector(->(_resource) { experiment_flags.use_adaptive_circuit_breaker? })
Semian::DualCircuitBreaker.adaptive_circuit_breaker_selector = ->(_resource) { experiment_flags.use_adaptive_circuit_breaker? }

puts "=== Dual Circuit Breaker Demo ===\n\n"

Expand Down
21 changes: 8 additions & 13 deletions lib/semian/dual_circuit_breaker.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,8 @@ module Semian
class DualCircuitBreaker
include CircuitBreakerBehaviour

class ChildClassicCircuitBreaker < CircuitBreaker
# Module to synchronize mark_success and mark_failed calls between sibling circuit breakers
module SiblingSync
attr_writer :sibling

def mark_success
Expand All @@ -20,18 +21,12 @@ def mark_failed(error)
end
end

class ChildAdaptiveCircuitBreaker < AdaptiveCircuitBreaker
attr_writer :sibling

def mark_success
super
@sibling.method(:mark_success).super_method.call
end
class ChildClassicCircuitBreaker < CircuitBreaker
include SiblingSync
end

def mark_failed(error)
super
@sibling.method(:mark_failed).super_method.call(error)
end
class ChildAdaptiveCircuitBreaker < AdaptiveCircuitBreaker
include SiblingSync
end

attr_reader :classic_circuit_breaker, :adaptive_circuit_breaker, :active_circuit_breaker
Expand All @@ -50,7 +45,7 @@ def initialize(name:, classic_circuit_breaker:, adaptive_circuit_breaker:)
@active_circuit_breaker = @classic_circuit_breaker
end

def self.adaptive_circuit_breaker_selector(selector) # rubocop:disable Style/ClassMethodsDefinitions
def self.adaptive_circuit_breaker_selector=(selector) # rubocop:disable Style/ClassMethodsDefinitions
Copy link
Author

@adriangudas adriangudas Dec 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Maybe this doesn't have to change. Or I can at least add the old method side-by-side so there's backwards compatibility.

@@adaptive_circuit_breaker_selector = selector # rubocop:disable Style/ClassVars
end

Expand Down
32 changes: 29 additions & 3 deletions test/dual_circuit_breaker_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,9 @@ class TestDualCircuitBreaker < Minitest::Test
def setup
Semian.reset!
@use_adaptive_flag = true
Semian::DualCircuitBreaker.adaptive_circuit_breaker_selector(->(_resource) {
Semian::DualCircuitBreaker.adaptive_circuit_breaker_selector = ->(_resource) {
@use_adaptive_flag
})
}
end

def teardown
Expand Down Expand Up @@ -70,6 +70,15 @@ def test_both_breakers_record_requests
classic_cb = resource.circuit_breaker.classic_circuit_breaker
adaptive_cb = resource.circuit_breaker.adaptive_circuit_breaker

# In the classic circuit breaker, mark_success doesn't result in any state change on a closed circuit,
# and just returns. So we have to use mocks to ensure it's being called.
#
# Here, we selectively mock only the mark_success method on the superclass.
# (If we mock mark_success on the ChildClassicCircuitBreaker class itself,
# the sibling circuit breaker's mark_success will never be called,
# which means adaptive_metrics will never contain any metrics, and that expectation will fail)
classic_cb.class.superclass.any_instance.expects(:mark_success).times(2)

2.times { resource.acquire { "success" } }

adaptive_metrics = adaptive_cb.pid_controller.metrics
Expand Down Expand Up @@ -109,7 +118,7 @@ def test_handles_use_adaptive_check_errors_gracefully
exceptions: [TestError],
)

Semian::DualCircuitBreaker.adaptive_circuit_breaker_selector(->(_resource) { raise StandardError, "check failed" })
Semian::DualCircuitBreaker.adaptive_circuit_breaker_selector = ->(_resource) { raise StandardError, "check failed" }

success_count = 0
resource.acquire { success_count += 1 }
Expand Down Expand Up @@ -207,6 +216,23 @@ def test_notifies_on_mode_change
Semian.unsubscribe(subscription)
end

def test_dual_circuit_breaker_is_not_used_when_configuration_is_not_specified
resource = Semian.register(
:test_classic_circuit_breaker,
circuit_breaker: true,
success_threshold: 2,
error_threshold: 3,
error_timeout: 5,
tickets: 5,
timeout: 0.5,
exceptions: [TestError],
)

assert_instance_of(Semian::CircuitBreaker, resource.circuit_breaker)
refute_instance_of(Semian::DualCircuitBreaker, resource.circuit_breaker)
refute_instance_of(Semian::AdaptiveCircuitBreaker, resource.circuit_breaker)
end

private

def create_dual_resource
Expand Down
Loading