fix: back off failed retention passes

The previous fix removed the timestamp that suppressed retries but left
the failure path waiting the configured interval, so a transient lock or
connection error still deferred retention for up to an hour. Greptile
caught that the reply overstated what the change did.

A failed pass now retries at monitor cadence and doubles the pause per
consecutive failure, capped by the retention interval, so recovery is
prompt without polling a database that stays down once a second forever.
The retry test now configures a 600 second interval, which the previous
behaviour would have waited out.

Also merge the duplicate Unreleased changelog heading left by the rebase.

Author: cardmagic

CHANGELOG.md

@@ -6,9 +6,9 @@
   unscheduled. Every actor call writes a durable message row, so a policy that
   nothing invokes let history grow without bound until an application scheduled
   its own job. `retention_interval` defaults to one hour, and zero disables it.
-
-## Unreleased
-
+  Retention runs on its own thread, so a slow pass cannot delay replacing a
+  crashed role, and a failed pass retries at monitor cadence with a doubling
+  backoff rather than deferring for the whole interval.
 - Add Ruby 4.0 to the compatibility matrix, which now covers Ruby 3.3, 3.4, and
   4.0 against Rails 8.0 and 8.1.
 

lib/solid_objects/supervisor.rb

@@ -2,6 +2,8 @@
 
 module SolidObjects
   class Supervisor
+    MAXIMUM_RETENTION_BACKOFF_DOUBLINGS = 16
+
     # @rbs @components: Array[Worker | EffectExecutor | ReminderScheduler | BroadcastExecutor]
     # @rbs @threads: Array[Thread]
     # @rbs @monitor: Thread?
@@ -139,29 +141,31 @@ def thread_error(thread)
 
     # Retention gets its own thread rather than sharing the monitor's. A large
     # backlog or a lock wait can make a pass slow, and role replacement must not
-    # wait behind housekeeping. A failed pass retries on the next tick instead of
-    # deferring for the whole interval.
+    # wait behind housekeeping.
     # @rbs () -> void
     def retention_loop
+      failures = 0
       while @started
         begin
           prune_expired_records
+          failures = 0
         rescue => error
+          failures += 1
           SolidObjects.instrument(
             :"supervisor.retention_failed",
             error_class: error.class.name,
             error_message: error.message
           )
         end
-        wait_for_next_retention
+        wait_for_next_retention(failures)
       end
     end
 
     # Sleeping the whole interval would make shutdown wait out an hour-long
     # nap, so the pause is taken in short steps that notice a stop request.
-    # @rbs () -> void
-    def wait_for_next_retention
-      deadline = monotonic_now + retention_pause
+    # @rbs (Integer) -> void
+    def wait_for_next_retention(failures)
+      deadline = monotonic_now + retention_pause(failures)
       step = SolidObjects.configuration.supervisor_monitor_interval
       while @started && monotonic_now < deadline
         sleep [ step, deadline - monotonic_now ].min
@@ -180,12 +184,19 @@ def prune_expired_records
       ProcessPruner.new.prune
     end
 
-    # @rbs () -> Float
-    def retention_pause
+    # A transient lock or connection error must not defer retention for the
+    # whole interval, so a failed pass retries at monitor cadence. The pause
+    # then doubles per consecutive failure, capped by the interval, so a
+    # database that stays down is not polled once a second forever.
+    # @rbs (Integer) -> Float
+    def retention_pause(failures)
       interval = SolidObjects.configuration.retention_interval
-      return interval if interval.positive?
+      interval = SolidObjects.configuration.supervisor_monitor_interval unless interval.positive?
+      return interval if failures.zero?
 
-      SolidObjects.configuration.supervisor_monitor_interval
+      backoff = SolidObjects.configuration.supervisor_monitor_interval *
+        (2**[ failures - 1, MAXIMUM_RETENTION_BACKOFF_DOUBLINGS ].min)
+      [ backoff, interval ].min
     end
 
     # @rbs () -> void

sig/generated/lib/solid_objects/supervisor.rbs

@@ -2,6 +2,8 @@
 
 module SolidObjects
   class Supervisor
+    MAXIMUM_RETENTION_BACKOFF_DOUBLINGS: ::Integer
+
     @lifecycle: Thread::Mutex
 
     @retention: Thread?
@@ -56,15 +58,14 @@ module SolidObjects
 
     # Retention gets its own thread rather than sharing the monitor's. A large
     # backlog or a lock wait can make a pass slow, and role replacement must not
-    # wait behind housekeeping. A failed pass retries on the next tick instead of
-    # deferring for the whole interval.
+    # wait behind housekeeping.
     # @rbs () -> void
     def retention_loop: () -> void
 
     # Sleeping the whole interval would make shutdown wait out an hour-long
     # nap, so the pause is taken in short steps that notice a stop request.
-    # @rbs () -> void
-    def wait_for_next_retention: () -> void
+    # @rbs (Integer) -> void
+    def wait_for_next_retention: (Integer) -> void
 
     # Every actor call writes a durable message row, so retention that is only
     # configured and never run leaves those rows to grow without bound. The
@@ -73,8 +74,12 @@ module SolidObjects
     # @rbs () -> void
     def prune_expired_records: () -> void
 
-    # @rbs () -> Float
-    def retention_pause: () -> Float
+    # A transient lock or connection error must not defer retention for the
+    # whole interval, so a failed pass retries at monitor cadence. The pause
+    # then doubles per consecutive failure, capped by the interval, so a
+    # database that stays down is not polled once a second forever.
+    # @rbs (Integer) -> Float
+    def retention_pause: (Integer) -> Float
 
     # @rbs () -> void
     def stop_retention: () -> void

test/integration/scheduled_retention_test.rb

@@ -32,6 +32,8 @@ def runs = self.class.shared_runs
     def run
       self.class.shared_runs += 1
       if self.class.shared_runs == 1
+        # The crash is the point of the test, so its backtrace is not news.
+        Thread.current.report_on_exception = false
         sleep 0.1
         raise "role crashed"
       end
@@ -46,12 +48,17 @@ def run
     SolidObjects.configuration.dead_process_cleanup_interval = 0
     SolidObjects.configuration.retention_interval = 0.05
     SolidObjects.configuration.message_retention = 0
+    # A test that stalls a retention pass on purpose should not pay the
+    # production join before the thread is killed.
+    SolidObjects.configuration.shutdown_timeout = 0.2
   end
 
   teardown do
     @supervisor&.stop
     SolidObjects.configuration.retention_interval = 3600.0
     SolidObjects.configuration.dead_process_cleanup_interval = 60.0
+    SolidObjects.configuration.supervisor_monitor_interval = 1.0
+    SolidObjects.configuration.shutdown_timeout = 15.0
   end
 
   test "the supervisor prunes expired messages without being asked" do
@@ -108,7 +115,11 @@ def run
     ActiveSupport::Notifications.unsubscribe(subscription) if subscription
   end
 
+  # The interval here is far longer than the assertion window, so a retry that
+  # waited it out would fail. A transient lock or connection error must not
+  # defer retention for the whole hour.
   test "a failing retention pass retries rather than deferring for the interval" do
+    SolidObjects.configuration.retention_interval = 600
     failures = []
     subscription = ActiveSupport::Notifications.subscribe("solid_objects.supervisor.retention_failed") do
       failures << true
@@ -118,16 +129,37 @@ def run
     @supervisor.define_singleton_method(:prune_expired_records) { raise "boom" }
 
     @supervisor.start
-    Timeout.timeout(10) { sleep 0.02 until failures.length >= 2 }
+    Timeout.timeout(5) { sleep 0.02 until failures.length >= 3 }
 
     assert @supervisor.instance_variable_get(:@monitor).alive?,
       "the monitor should survive a failing retention pass"
-    assert_operator failures.length, :>=, 2,
-      "a failed pass should retry on the next tick, not wait out the interval"
+    assert_operator failures.length, :>=, 3
   ensure
     ActiveSupport::Notifications.unsubscribe(subscription) if subscription
   end
 
+  # Retrying at monitor cadence forever would hammer a database that stays
+  # down, so the pause grows and is capped by the configured interval.
+  test "repeated retention failures back off" do
+    SolidObjects.configuration.retention_interval = 600
+    SolidObjects.configuration.supervisor_monitor_interval = 0.05
+    @supervisor = supervisor
+    @supervisor.define_singleton_method(:prune_expired_records) { raise "boom" }
+
+    pauses = (1..5).map { |failures| @supervisor.send(:retention_pause, failures) }
+
+    assert_equal pauses.sort, pauses, "each failure should wait at least as long"
+    assert_operator pauses.last, :>, pauses.first
+    assert_operator pauses.max, :<=, 600
+  end
+
+  test "a recovered retention pass returns to its interval" do
+    SolidObjects.configuration.retention_interval = 600
+    @supervisor = supervisor
+
+    assert_equal 600, @supervisor.send(:retention_pause, 0)
+  end
+
   # Role replacement must not wait behind housekeeping.
   test "a slow retention pass does not block role replacement" do
     SolidObjects.configuration.retention_interval = 0.02