From df76dcf477045ab343f25793a08973b93563b98e Mon Sep 17 00:00:00 2001 From: Bastian Schon Date: Mon, 31 Aug 2026 10:42:25 +0200 Subject: [PATCH] Clear the re-entrancy guard instead of restoring its previous value acquire_semian_resource is the only caller of mark_resource_as_acquired and it returns early when the guard is already set, so in supported use the value captured as `previous` can only ever be falsey. Capturing and restoring it is what turns a violation of that contract into a permanent one: if two callers interleave, one captures true, the other restores false, and the one holding true restores it after both have left. From then on every call on that instance returns early from acquire_semian_resource, so the circuit breaker is disabled for the lifetime of the process, silently and with the circuit still reporting closed. Assign false on the way out instead. In supported use this is the value `previous` always held, so behaviour is unchanged; a shared instance now degrades transiently rather than permanently. Two tests, since the guard had none: the interleaving above, and nesting still short-circuiting so a nested call neither re-enters the circuit breaker nor takes a second bulkhead ticket. Refs #1045 Co-authored-by: Claude Opus 5 Orchestrated-by: ae Assisted-By: devx/39c7b308-c333-4ffe-8cd7-f642a14a4b7c --- lib/semian/adapter.rb | 3 +-- test/adapter_test.rb | 52 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 53 insertions(+), 2 deletions(-) diff --git a/lib/semian/adapter.rb b/lib/semian/adapter.rb index 404f9507d..69326e005 100644 --- a/lib/semian/adapter.rb +++ b/lib/semian/adapter.rb @@ -79,11 +79,10 @@ def resource_already_acquired? end def mark_resource_as_acquired - previous = @resource_acquired @resource_acquired = true yield ensure - @resource_acquired = previous + @resource_acquired = false end end end diff --git a/test/adapter_test.rb b/test/adapter_test.rb index 8178d5b64..28f605a7f 100644 --- a/test/adapter_test.rb +++ b/test/adapter_test.rb @@ -106,6 +106,58 @@ def test_dynamic_adapter_not_registered_as_consumer assert_empty(Semian.consumers) end + def test_reentrancy_guard_short_circuits_nested_acquires + client = Semian::AdapterTestClient.new(bulkhead: false) + acquisitions = 0 + Semian.subscribe(:test_reentrancy_guard) do |event, resource, _scope, _adapter, _payload| + acquisitions += 1 if event == :success && resource.name == client.semian_identifier + end + + client.send(:acquire_semian_resource, scope: :query, adapter: :test) do + assert(client.send(:resource_already_acquired?)) + + client.send(:acquire_semian_resource, scope: :query, adapter: :test) { :nested } + end + + assert_equal(1, acquisitions) + refute(client.send(:resource_already_acquired?)) + ensure + Semian.unsubscribe(:test_reentrancy_guard) + end + + # An adapter instance is meant to belong to one session at a time, but when that is + # not the case two callers can interleave inside `mark_resource_as_acquired`. The + # guard must not outlive them: one left set disables the circuit breaker for that + # object for the life of the process. + def test_reentrancy_guard_is_cleared_when_marks_interleave + client = Semian::AdapterTestClient.new(bulkhead: false) + first_holding = Queue.new + first_restored = Queue.new + second_marked = Queue.new + + first = Thread.new do + client.send(:mark_resource_as_acquired) do + first_holding << true + second_marked.pop + end + first_restored << true + end + first_holding.pop + + second = Thread.new do + client.send(:mark_resource_as_acquired) do + second_marked << true + first_restored.pop + end + end + + [first, second].each do |thread| + assert(thread.join(5), "thread did not finish") + end + + refute(client.send(:resource_already_acquired?), "re-entrancy guard was left set") + end + class MyAdapterError < StandardError include Semian::AdapterError end