`Semian::Adapter`'s re-entrancy guard silently disables the circuit when the adapter is mixed into a shared object

[serioushaircut]

Summary

Semian::Adapter implicitly requires that the object it is mixed into is used by exactly one thread at a time — i.e. a per-connection object, like all the adapters that ship with the gem. Nothing in the README says so, and the failure is silent when the requirement is not met.

Mixed into a shared object (a process-wide client singleton, a memoized service
object, a pool wrapper), the re-entrancy guard turns the circuit breaker off:

1. While any call is in flight anywhere in the process, every concurrent call
   skips acquire entirely — no fast-fail on an open circuit, no failure
   accounting, no :success / :circuit_open notifications.
2. The guard's save/restore is a lost-update race that can strand it at true,
   which disables circuit breaking permanently for that process.

Both modes report a healthy, closed circuit while every call goes to a dependency
that is timing out. We hit this in a threaded Rails app whose Semian adapter was
mixed into a process-wide client singleton; it took a deliberate concurrency test to
notice, because there is nothing in the logs or the metrics to see.

The trust_battery_client gem does this ⬆️ .

Versions: semian 0.28.2 (also 0.26.6), CRuby 3.4.10. The code below is unchanged
on main.

The guard

lib/semian/adapter.rb#L40-L45:

def acquire_semian_resource(scope:, adapter:, &block)
  return yield if resource_already_acquired?

  semian_resource.acquire(scope: scope, adapter: adapter, resource: self) do
    mark_resource_as_acquired(&block)
  end

#L77-L87:

def resource_already_acquired?
  @resource_acquired
end

def mark_resource_as_acquired
  previous = @resource_acquired
  @resource_acquired = true
  yield
ensure
  @resource_acquired = previous
end

This is a correct re-entrancy guard for the built-in adapters, because they are
per-connection objects: one Net::HTTP / Mysql2::Client / Redis instance is
only ever used by one thread at a time, so @resource_acquired is effectively
per-execution-context, and it only suppresses a genuinely nested acquisition of the
same resource.

On a shared object, the same instance variable is shared by every thread.

Failure mode 1 — a call in flight bypasses the circuit for every other thread

Thread A enters, sets @resource_acquired = true, and blocks on the network for the
duration of the timeout. Every other thread now reads a truthy guard and returns at
adapter.rb:41, so it never reaches semian_resource.acquire:

• an open circuit does not fast-fail those calls;
• their failures are never marked, so the circuit cannot open in the first place;
• Semian.notify is not called for them, so instrumentation under-reports.

The bypass probability rises with concurrency and with call duration, so protection
degrades exactly when it is needed — a slow dependency keeps a call in flight almost
all the time, which is the scenario Semian exists for.

Failure mode 2 — lost update strands the guard at true

mark_resource_as_acquired is a non-atomic read-modify-write of state shared
between threads. One interleaving:

	thread A	thread B	@resource_acquired
1	reads guard → nil, proceeds		nil
2		reads guard → nil, proceeds	nil
3	previous = nil; set true		true
4		previous = true; set true	true
5	ensure → restore nil		nil
6		ensure → restore true	true

From step 6 on, every call on that object short-circuits at adapter.rb:41 and
nothing ever writes the flag again. Circuit breaking is dead for the life of the
process. Nothing logs, nothing alerts.

Reaching step 2 needs a thread switch between the guard read and the previous
capture, and the window contains several ordinary yield points:

• Semian.notify(:success, …) runs inside the window
  (protected_resource.rb#L32-L38
  calls it before yielding back to acquire_semian_resource), so any
  subscriber that writes a metric to a socket releases the GVL there. That alone
  is enough — see the reproduction below.
• With bulkheads enabled (the default), Semian::Resource#acquire blocks in
  WITHOUT_GVL when tickets are exhausted
  (ext/semian/sysv_semaphores.c#L193-L200),
  which is inside the window too.
• For dynamic: true resources, raw_semian_options is called on every acquire,
  also inside the window, and it is arbitrary user code.

With no subscriber attached and a block that does no I/O we could not trigger it in
~10M calls, so it is latent rather than constant — but with a metrics subscriber it
reproduced on the first round of 8 threads × 200 calls, in every one of 75 rounds we
ran.

Reproduction

Self-contained; only requires semian. No monkey-patching, no rendezvous hooks: the
only ingredients are a shared adapter instance, threads, a metrics subscriber, and a
block that does I/O.

repro.rb

# frozen_string_literal: true
#
# Semian::Adapter's re-entrancy guard (@resource_acquired) is unsafe when the
# adapter is mixed into an object that more than one thread uses.
#
#   gem install semian -v 0.28.2 && ruby repro.rb
#
# Nothing here is application-specific. `SharedClient` is a plain
# `Semian::Adapter` includer; the only unusual thing is that several threads
# call the *same instance* -- the normal shape for a process-wide client
# singleton or a memoized service object.

require "logger"
require "socket"
require "semian"
require "semian/adapter"

OPTIONS = {
  bulkhead: false, circuit_breaker: true,
  success_threshold: 1, error_threshold: 3, error_timeout: 60,
}.freeze

class SharedClient
  Error = Class.new(StandardError)
  class SemianError < Error
    def initialize(semian_identifier, *args)
      super(*args)
      @semian_identifier = semian_identifier
    end
  end
  ResourceBusyError = Class.new(SemianError)
  CircuitOpenError = Class.new(SemianError)

  include Semian::Adapter

  def initialize(identifier)
    @identifier = identifier
  end

  attr_reader :identifier
  alias_method :semian_identifier, :identifier

  def raw_semian_options = OPTIONS
  def resource_exceptions = [Error]

  # The wrapper an adapter puts around a driver call.
  def call(&block)
    acquire_semian_resource(scope: :query, adapter: :shared_client) { block.call }
  end

  def circuit_state = semian_resource.circuit_breaker.state.value
  def guard_flag = instance_variable_get(:@resource_acquired)
end

def outcome(client)
  client.call { yield }
  :ok
rescue SharedClient::CircuitOpenError
  :circuit_open  # fast-failed by Semian -- the whole point of the library
rescue SharedClient::Error
  :failed        # the call was really attempted, and failed
end

def failing(client) = outcome(client) { raise SharedClient::Error, "boom" }

Semian.logger = Logger.new(File::NULL)
puts "semian #{Semian::VERSION} / ruby #{RUBY_VERSION} / #{RUBY_PLATFORM}\n\n"

# ---------------------------------------------------------------------------
# 1) Baseline: serial failures open the circuit, later calls fast-fail. Good.
# ---------------------------------------------------------------------------
serial = SharedClient.new(:demo_serial)
puts "1) serial failures ......... #{5.times.map { failing(serial) }.inspect}"
puts "   circuit state ........... #{serial.circuit_state}  (want open)\n\n"

# ---------------------------------------------------------------------------
# 2) One call in flight anywhere in the process makes the guard truthy for
#    every thread, so concurrent calls return from `adapter.rb:41` and never
#    reach `semian_resource.acquire`.
# ---------------------------------------------------------------------------
shared = SharedClient.new(:demo_shared)
in_flight = Queue.new
finish = Queue.new
holder = Thread.new { shared.call { in_flight << :ready; finish.pop } }
in_flight.pop

# 2a) Failures are not counted, so the circuit cannot open.
puts "2a) concurrent failures .... #{5.times.map { failing(shared) }.inspect}"
puts "    circuit state .......... #{shared.circuit_state}  (want open)\n\n"

# 2b) An already-open circuit is bypassed too. A second adapter instance --
#     per-connection, the way Semian's own adapters are built -- shares the
#     same resource and opens it while the shared client's call is in flight.
per_connection = SharedClient.new(:demo_shared)
3.times { failing(per_connection) }

puts "2b) circuit state .......... #{shared.circuit_state}"
puts "    via per-connection ..... #{failing(per_connection).inspect}  (fast-failed, correct)"
puts "    via shared client ...... #{5.times.map { failing(shared) }.inspect}"
puts "                             (want 5x :circuit_open)\n\n"

finish << :go
holder.join

# ---------------------------------------------------------------------------
# 3) `mark_resource_as_acquired` is a non-atomic save/set/restore of state
#    shared between threads. If B captures `previous` while A holds the flag
#    set, and A unwinds first, B's `ensure` restores `true` -- for good.
#
#    No hook or monkey-patch below: the only additions are a metrics
#    subscriber (Semian.notify runs it inside the window) and a block that
#    does I/O (every real adapter does). Both release the GVL, which is all
#    the interleaving needs.
# ---------------------------------------------------------------------------
sink = UDPSocket.new.tap { |s| s.bind("127.0.0.1", 0) }
statsd = UDPSocket.new
Semian.subscribe(:repro_metrics) do |event, resource, _scope, _adapter, _payload|
  statsd.send("semian.#{resource.name}.#{event}:1|c", 0, "127.0.0.1", sink.addr[1])
end

leaked_at = nil
racer = nil
1.upto(20) do |round|
  racer = SharedClient.new(:"demo_race_#{round}")
  8.times.map { Thread.new { 200.times { racer.call { sleep(0.0005) } } } }.each(&:join)
  if racer.guard_flag
    leaked_at = round
    break
  end
end

puts "3) guard flag after #{leaked_at ? "round #{leaked_at}" : "20 rounds"} .. #{racer.guard_flag.inspect}  (want nil)"
puts "   serial failures now ..... #{5.times.map { failing(racer) }.inspect}"
puts "   circuit state ........... #{racer.circuit_state}  (want open)"
puts "   -> circuit breaking is dead for the rest of the process's life, silently."

semian 0.28.2 / ruby 3.4.10 / arm64-darwin24

1) serial failures ......... [:failed, :failed, :failed, :circuit_open, :circuit_open]
   circuit state ........... open  (want open)

2a) concurrent failures .... [:failed, :failed, :failed, :failed, :failed]
    circuit state .......... closed  (want open)

2b) circuit state .......... open
    via per-connection ..... :circuit_open  (fast-failed, correct)
    via shared client ...... [:failed, :failed, :failed, :failed, :failed]
                             (want 5x :circuit_open)

3) guard flag after round 1 .. true  (want nil)
   serial failures now ..... [:failed, :failed, :failed, :failed, :failed]
   circuit state ........... closed  (want open)
   -> circuit breaking is dead for the rest of the process's life, silently.

Why we think this belongs in semian

• The contract is not stated anywhere. The README's Creating
  Adapters section says
  include Semian::Adapter "takes care of situations such as monitoring, nested
  resources, unsupported platforms, …", which reads as nesting is handled for you,
  not as your includer must never be shared between threads.
• The Thread Safety section says
  the circuit breaker implementation is thread-safe by default. That is true of the
  breaker's own state, but the adapter mixin in front of it is not, and the
  distinction is invisible from the outside.
• The consequence is not a crash or a wrong value, it is "your circuit breaker is
  off" — undetectable without a concurrency test written specifically to look for
  it.

What we'd like

Anything that closes the gap; in rough order of cost:

1. Docs — one sentence in Creating Adapters ("the object you include this into
   must be per-execution-context; a shared or singleton includer will share the
   re-entrancy flag between threads and bypass the circuit"), plus a note in Thread
   Safety. That would have prevented this for us.
2. Document the escape hatch — that resource_already_acquired? and
   mark_resource_as_acquired can be overridden by an includer that is shared.
   That is what we did (fiber-local state, below).
3. Fix it in the gem — make the guard fiber-local instead of an instance
   variable. Note this is a behaviour change: circuits that never opened will start
   opening, so it probably wants a minor/major release with a release note rather
   than a patch. Keying the fiber-local state per adapter object (or per
   semian_identifier) preserves today's nesting semantics; a single fiber-local
   boolean would make one adapter's acquisition suppress an unrelated adapter's
   nested acquisition on the same fiber.

Our local workaround, for reference — it overrides only our own module, so the
built-in adapters nested inside our calls are untouched:

RESOURCE_ACQUIRED_KEY = :my_adapter_resource_acquired

def resource_already_acquired?
  Thread.current[RESOURCE_ACQUIRED_KEY] || false
end

def mark_resource_as_acquired(&block)
  previous = Thread.current[RESOURCE_ACQUIRED_KEY]
  Thread.current[RESOURCE_ACQUIRED_KEY] = true
  block.call
ensure
  Thread.current[RESOURCE_ACQUIRED_KEY] = previous
end

Happy to open a PR for whichever of the three you'd prefer — docs, docs + escape
hatch, or the fiber-local guard with tests.

_{🤖 AE · ✅ approved by @serioushaircut}