fix: close race in Avo.boot that corrupts the plugin registry

lib/avo.rb

@@ -55,6 +55,11 @@ class DeprecatedAPIError < StandardError; end
 
   class ViewTypeComponentNotFoundError < StandardError; end
 
+  # Serializes Avo.boot so concurrent code reloads (config.to_prepare fires on
+  # every reload, from every request thread) can't interleave the plugin
+  # registry reset with its repopulation. See PluginManager#begin_reload.
+  @boot_mutex = Mutex.new
+
   # Exception raised when a resource is missing
   class MissingResourceError < StandardError
     def initialize(model_class, field)
@@ -92,16 +97,19 @@ class << self
 
     # Runs when the app boots up
     def boot
-      Turbo::Streams::TagBuilder.prepend(Avo::TurboStreamActionsHelper)
-      @logger = Avo.configuration.logger
-      @field_manager = Avo::Fields::FieldManager.build
-      @view_type_manager = nil # force re-init with defaults on next access
-      @cache_store = Avo.configuration.cache_store
-      Avo.plugin_manager.reset
-      # Run load hooks for plugins to include them in the app.
-      # This is useful for plugins that need to include modules in the app that will be used on avo_boot hook.
-      ActiveSupport.run_load_hooks(:avo_plugin_include, self)
-      ActiveSupport.run_load_hooks(:avo_boot, self)
+      @boot_mutex.synchronize do
+        Turbo::Streams::TagBuilder.prepend(Avo::TurboStreamActionsHelper)
+        @logger = Avo.configuration.logger
+        @field_manager = Avo::Fields::FieldManager.build
+        @view_type_manager = nil # force re-init with defaults on next access
+        @cache_store = Avo.configuration.cache_store
+        Avo.plugin_manager.begin_reload
+        # Run load hooks for plugins to include them in the app.
+        # This is useful for plugins that need to include modules in the app that will be used on avo_boot hook.
+        ActiveSupport.run_load_hooks(:avo_plugin_include, self)
+        ActiveSupport.run_load_hooks(:avo_boot, self)
+        Avo.plugin_manager.commit_reload
+      end
       eager_load_actions
     end
 

lib/avo/plugin_manager.rb

@@ -1,18 +1,47 @@
 module Avo
   class PluginManager
     attr_reader :plugins
-    attr_accessor :engines
+    attr_reader :engines
 
     alias_method :all, :plugins
 
     def initialize
       @plugins = []
       @engines = []
+      @building_plugins = []
+      @building_engines = []
     end
 
-    def reset
-      @plugins = []
-      @engines = []
+    # Starts a re-registration cycle without touching the currently published
+    # @plugins/@engines, so concurrent readers (e.g. mount_avo's route-drawing
+    # loop) keep seeing the complete pre-reload list until #commit_reload
+    # publishes the new one. Called by Avo.boot inside its Mutex, so only one
+    # reload is ever building at a time.
+    def begin_reload
+      @building_plugins = []
+      @building_engines = []
+    end
+
+    # Publishes the registrations collected since #begin_reload. Each of the
+    # two reassignments below is individually an atomic pointer swap, so a
+    # reader of .engines alone (or .plugins alone) always sees the complete
+    # old list or the complete new one, never a partially rebuilt one. The
+    # two fields are not published jointly -- a reader combining both in one
+    # operation could observe them from different reload generations. No
+    # current reader does that (mount_avo reads only .engines; installed?
+    # reads only .plugins).
+    def commit_reload
+      @plugins = @building_plugins
+      @engines = @building_engines
+      # Reset to fresh arrays rather than nil: register/mount_engine must
+      # stay callable even outside a begin_reload/commit_reload window --
+      # e.g. avo-permissions calls Avo.plugin_manager.register at Rails
+      # initializer time, before Avo.boot ever runs. Such a call is simply
+      # discarded by the next #begin_reload rather than raising, matching
+      # the pre-atomic-publish behavior where register/mount_engine never
+      # crashed regardless of timing.
+      @building_plugins = []
+      @building_engines = []
     end
 
     def register(name, priority: 10)
@@ -21,7 +50,7 @@ def register(name, priority: 10)
       # (e.g. `:rhino` for `avo-rhino_field`), so the name alone isn't enough.
       registered_from = caller_locations(1, 1)&.first&.path
 
-      @plugins << Plugin.new(name:, priority: priority, registered_from: registered_from)
+      @building_plugins << Plugin.new(name:, priority: priority, registered_from: registered_from)
     end
 
     def register_view_type(name, component:, icon:, active_icon:, translation_key: nil)
@@ -80,7 +109,11 @@ def installed?(name)
     end
 
     def mount_engine(klass, **options)
-      @engines << {klass:, options:}
+      # Dedup by class so a plugin hook that (by bug) calls mount_engine
+      # twice for the same engine within one boot can't leave a duplicate
+      # entry for #commit_reload to publish.
+      @building_engines.delete_if { |engine| engine[:klass] == klass }
+      @building_engines << {klass:, options:}
     end
   end
 

spec/features/avo/lib/mount_avo_spec.rb

@@ -8,12 +8,6 @@
     Avo.configuration.mount_lookbook = original
   end
 
-  def draw_mount_avo(**options)
-    routes = ActionDispatch::Routing::RouteSet.new
-    routes.draw { mount_avo(**options) }
-    routes
-  end
-
   it "defaults mount_lookbook to false" do
     draw_mount_avo
 

spec/lib/avo/boot_thread_safety_spec.rb

@@ -0,0 +1,130 @@
+require "rails_helper"
+
+RSpec.describe "Avo.boot thread safety" do
+  include_context "with isolated plugin manager"
+
+  it "populates plugin_manager as before for a normal single-threaded boot" do
+    fake_engine = Class.new(Rails::Engine)
+    allow(ActiveSupport).to receive(:run_load_hooks).and_call_original
+    allow(ActiveSupport).to receive(:run_load_hooks).with(:avo_boot, Avo) do
+      Avo.plugin_manager.register(:fake_plugin, priority: 5)
+      Avo.plugin_manager.mount_engine(fake_engine, at: "/fake")
+    end
+
+    Avo.boot
+
+    expect(Avo.plugin_manager.engines).to eq([{klass: fake_engine, options: {at: "/fake"}}])
+    expect(Avo.plugin_manager.plugins.map(&:name)).to eq([:fake_plugin])
+  end
+
+  it "leaves no duplicate engines or plugins when multiple threads call Avo.boot concurrently" do
+    fake_engine = Class.new(Rails::Engine)
+    allow(ActiveSupport).to receive(:run_load_hooks).and_call_original
+    allow(ActiveSupport).to receive(:run_load_hooks).with(:avo_boot, Avo) do
+      # Widen the interleaving window so, without the fix, concurrent boots
+      # reliably interleave their reset-then-repopulate sequence instead of
+      # relying on timing luck.
+      Avo.plugin_manager.register(:fake_plugin, priority: 5)
+      Thread.pass
+      Avo.plugin_manager.mount_engine(fake_engine, at: "/fake")
+      Thread.pass
+    end
+
+    threads = 8.times.map { Thread.new { Avo.boot } }
+    threads.each(&:join)
+
+    expect(Avo.plugin_manager.engines).to eq([{klass: fake_engine, options: {at: "/fake"}}])
+    expect(Avo.plugin_manager.plugins.map(&:name)).to eq([:fake_plugin])
+  end
+
+  it "never exposes a partially rebuilt engines list to a concurrent reader" do
+    fake_engine_1 = Class.new(Rails::Engine)
+    fake_engine_2 = Class.new(Rails::Engine)
+    reached_midpoint = Queue.new
+    release_midpoint = Queue.new
+
+    allow(ActiveSupport).to receive(:run_load_hooks).and_call_original
+    allow(ActiveSupport).to receive(:run_load_hooks).with(:avo_boot, Avo) do
+      Avo.plugin_manager.mount_engine(fake_engine_1, at: "/one")
+      reached_midpoint << true
+      release_midpoint.pop
+      Avo.plugin_manager.mount_engine(fake_engine_2, at: "/two")
+    end
+
+    pre_boot_engines = Avo.plugin_manager.engines
+
+    boot_thread = Thread.new { Avo.boot }
+    # Bounded wait: if a future regression makes the background boot never
+    # reach the rendezvous (e.g. it raises before mount_engine runs), fail
+    # with a clear message instead of hanging this example (and, since
+    # boot_thread would then hold the mutex forever, every later spec too).
+    raise "background boot never reached the rendezvous point" if reached_midpoint.pop(timeout: 5).nil?
+
+    begin
+      # Mid-boot, a concurrent reader (standing in for mount_avo's route-drawing
+      # loop) must see the complete pre-boot list, never a partial one that
+      # contains only fake_engine_1.
+      expect(Avo.plugin_manager.engines).to eq(pre_boot_engines)
+    ensure
+      # Always release the paused background thread, even if the assertion
+      # above fails -- otherwise boot_thread stays parked mid-Avo.boot,
+      # permanently holding the boot mutex and hanging every later spec in
+      # this process that calls Avo.boot.
+      release_midpoint << true
+      boot_thread.join
+    end
+
+    expect(Avo.plugin_manager.engines).to eq([
+      {klass: fake_engine_1, options: {at: "/one"}},
+      {klass: fake_engine_2, options: {at: "/two"}}
+    ])
+  end
+
+  it "does not raise when Avo.boot is called sequentially, matching the two real (non-nested) call sites" do
+    allow(ActiveSupport).to receive(:run_load_hooks).and_call_original
+
+    expect {
+      Avo.boot
+      Avo.boot
+    }.not_to raise_error
+  end
+
+  it "raises ThreadError instead of deadlocking on genuine same-thread reentrancy" do
+    # Ruby's Mutex is not reentrant: locking it twice on the same thread
+    # raises immediately rather than hanging. Neither real call site
+    # (config.after_initialize, config.to_prepare) nests a call to Avo.boot,
+    # but this documents what happens if a future hook ever does.
+    allow(ActiveSupport).to receive(:run_load_hooks).and_call_original
+    allow(ActiveSupport).to receive(:run_load_hooks).with(:avo_boot, Avo) do
+      Avo.boot
+    end
+
+    expect { Avo.boot }.to raise_error(ThreadError, /deadlock/)
+  end
+
+  it "leaves the previously published registry intact and releases the mutex when a hook raises mid-boot" do
+    fake_engine = Class.new(Rails::Engine)
+    allow(ActiveSupport).to receive(:run_load_hooks).and_call_original
+    allow(ActiveSupport).to receive(:run_load_hooks).with(:avo_boot, Avo) do
+      Avo.plugin_manager.mount_engine(fake_engine, at: "/fake")
+    end
+    Avo.boot
+    published_engines = Avo.plugin_manager.engines
+
+    allow(ActiveSupport).to receive(:run_load_hooks).with(:avo_boot, Avo) do
+      Avo.plugin_manager.mount_engine(Class.new(Rails::Engine), at: "/never-published")
+      raise "boom"
+    end
+
+    expect { Avo.boot }.to raise_error("boom")
+    # commit_reload never ran, so the last successful boot's registry stands.
+    expect(Avo.plugin_manager.engines).to eq(published_engines)
+
+    # Mutex#synchronize releases the lock via ensure even on exception, so
+    # the next boot proceeds normally rather than deadlocking.
+    allow(ActiveSupport).to receive(:run_load_hooks).with(:avo_boot, Avo) do
+      Avo.plugin_manager.mount_engine(fake_engine, at: "/fake")
+    end
+    expect { Avo.boot }.not_to raise_error
+  end
+end

spec/lib/avo/plugin_manager_boot_race_spec.rb

@@ -0,0 +1,46 @@
+require "rails_helper"
+
+RSpec.describe "Avo plugin manager boot race - end-to-end regression" do
+  include_context "with isolated plugin manager"
+
+  def stub_pro_engine
+    stub_const("Avo::PluginManagerBootRaceSpec", Module.new)
+    stub_const("Avo::PluginManagerBootRaceSpec::Engine", Class.new(Rails::Engine) do
+      isolate_namespace Avo::PluginManagerBootRaceSpec
+
+      def self.name
+        "Avo::PluginManagerBootRaceSpec::Engine"
+      end
+    end)
+    Avo::PluginManagerBootRaceSpec::Engine
+  end
+
+  it "does not raise ArgumentError after concurrent Avo.boot calls followed by a route redraw" do
+    fake_engine = stub_pro_engine
+
+    allow(ActiveSupport).to receive(:run_load_hooks).and_call_original
+    allow(ActiveSupport).to receive(:run_load_hooks).with(:avo_boot, Avo) do
+      Avo.plugin_manager.mount_engine(fake_engine, at: "/plugin_manager_boot_race_spec")
+    end
+
+    threads = 8.times.map { Thread.new { Avo.boot } }
+    threads.each(&:join)
+
+    expect(Avo.plugin_manager.engines).to eq([{klass: fake_engine, options: {at: "/plugin_manager_boot_race_spec"}}])
+    expect { draw_mount_avo }.not_to raise_error
+  end
+
+  it "sanity check: mount_avo does raise ArgumentError when the registry actually holds a duplicate" do
+    # Proves the assertion above is a real regression guard, not vacuously
+    # green -- a corrupted registry (the pre-fix failure mode) still trips
+    # mount_avo's route-drawing loop.
+    fake_engine = stub_pro_engine
+
+    Avo.plugin_manager.begin_reload
+    Avo.plugin_manager.instance_variable_get(:@building_engines) << {klass: fake_engine, options: {at: "/one"}}
+    Avo.plugin_manager.instance_variable_get(:@building_engines) << {klass: fake_engine, options: {at: "/two"}}
+    Avo.plugin_manager.commit_reload
+
+    expect { draw_mount_avo }.to raise_error(ArgumentError, /Invalid route name, already in use/)
+  end
+end