fix: replace roles that crash after self-cleanup

Worker and ReminderScheduler run their shutdown cleanup in an ensure, so a crashed role reports itself stopped exactly like one that was asked to stop. Skipping stopped components therefore skipped every real crash and left the process permanently below its role count, which is the case the feature exists for. Key replacement on the supervisor still running, and build a fresh instance because the crashed one has already released its process record. Reject a non-positive monitor interval during validation, and merge the duplicated unreleased changelog section.

Author: cardmagic

CHANGELOG.md

@@ -8,9 +8,6 @@
   records on an interval as part of the same monitor. Both intervals are
   configurable through `supervisor_monitor_interval` and
   `dead_process_cleanup_interval`.
-
-## Unreleased
-
 - Add `SolidObjects::WakeUpAdapters::Postgresql`, an optional cross-process
   wake-up using PostgreSQL notifications. In-process signalling cannot reach a
   worker process, so reactive delivery waited out `polling_interval`. With the

lib/solid_objects/configuration.rb

@@ -144,6 +144,10 @@ def validate!
         raise ArgumentError, "table_name_prefix must contain lowercase letters, digits, and underscores"
       end
 
+      unless supervisor_monitor_interval.positive?
+        raise ArgumentError, "supervisor_monitor_interval must be positive"
+      end
+
       unless lease_duration > lease_renewal_interval
         raise ArgumentError, "lease_duration must be greater than lease_renewal_interval"
       end

lib/solid_objects/supervisor.rb

@@ -82,17 +82,25 @@ def monitor_loop
       retry if @started
     end
 
+    # A role that raises runs its own shutdown cleanup on the way out, so a
+    # crashed component reports itself stopped exactly like one that was asked
+    # to stop. While the supervisor is still running, a dead thread can only
+    # mean a crash, so replacement keys on the supervisor rather than on the
+    # component. The crashed instance has already released its process record,
+    # so a fresh one takes its place.
     # @rbs () -> void
     def replace_dead_roles
       components.each_with_index do |component, index|
         thread = threads[index]
         next if thread&.alive?
-        next if component.stopped?
+        break unless @started
 
-        threads[index] = supervise(component)
+        replacement = component.class.new
+        components[index] = replacement
+        threads[index] = supervise(replacement)
         SolidObjects.instrument(
           :"supervisor.role_replaced",
-          role: component.class.name,
+          role: replacement.class.name,
           error_class: thread_error(thread)
         )
       end

sig/generated/lib/solid_objects/supervisor.rbs

@@ -36,6 +36,12 @@ module SolidObjects
     # @rbs () -> void
     def monitor_loop: () -> void
 
+    # A role that raises runs its own shutdown cleanup on the way out, so a
+    # crashed component reports itself stopped exactly like one that was asked
+    # to stop. While the supervisor is still running, a dead thread can only
+    # mean a crash, so replacement keys on the supervisor rather than on the
+    # component. The crashed instance has already released its process record,
+    # so a fresh one takes its place.
     # @rbs () -> void
     def replace_dead_roles: () -> void
 

test/integration/supervisor_replacement_test.rb

@@ -6,20 +6,29 @@
 class SupervisorReplacementTest < ActiveSupport::TestCase
   # A component that crashes on its first run and records every run after.
   class CrashingRole
+    class << self
+      attr_accessor :shared_runs, :shared_crashes
+    end
+
     attr_reader :runs
 
-    def initialize(crashes: 1)
+    def initialize(crashes: self.class.shared_crashes || 1)
       @crashes = crashes
-      @runs = Queue.new
+      self.class.shared_runs ||= Queue.new
+      @runs = self.class.shared_runs
       @stopped = false
       @shutdown = false
     end
 
+    # Mirrors Worker and ReminderScheduler, which run their shutdown cleanup in
+    # an ensure and therefore report themselves stopped after a crash too.
     def run
       @runs << monotonic_now
       raise "role crashed" if @runs.size <= @crashes
 
       sleep 0.01 until @shutdown
+    ensure
+      stop
     end
 
     def request_shutdown = @shutdown = true
@@ -34,10 +43,14 @@ def monotonic_now = ::Process.clock_gettime(::Process::CLOCK_MONOTONIC)
   end
 
   class HealthyRole < CrashingRole
-    def initialize = super(crashes: 0)
+    def initialize(**) = super(crashes: 0)
   end
 
   setup do
+    [ CrashingRole, HealthyRole ].each do |role|
+      role.shared_runs = nil
+      role.shared_crashes = nil
+    end
     SolidObjects.configuration.supervisor_monitor_interval = 0.02
     SolidObjects.configuration.dead_process_cleanup_interval = 0.05
   end
@@ -56,7 +69,8 @@ def initialize = super(crashes: 0)
   end
 
   test "keeps replacing a role that keeps crashing" do
-    role = CrashingRole.new(crashes: 3)
+    CrashingRole.shared_crashes = 3
+    role = CrashingRole.new
     @supervisor = supervisor_for(role)
 
     @supervisor.start
@@ -135,6 +149,8 @@ def initialize = super(crashes: 0)
 
   private
 
+  # Every instance of a role shares one run log, so replacement instances are
+  # observable the way the supervisor creates them.
   def supervisor_for(*roles)
     supervisor = SolidObjects::Supervisor.new(
       worker_count: 0,