Only failsafe on transient database errors

Avoid rescuing all ActiveRecord errors in the failsafe block as we might be hiding configuration errors. Fixes: #182
rails · Sep 3, 2024 · e30afec · e30afec
1 parent 1074279
commit e30afec
Show file tree

Hide file tree

Showing 6 changed files with 44 additions and 34 deletions.
diff --git a/README.md b/README.md
@@ -170,7 +170,7 @@ end
 
 Solid Cache supports these options in addition to the standard `ActiveSupport::Cache::Store` options:
 
-- `error_handler` - a Proc to call to handle any `ActiveRecord::ActiveRecordError`s that are raises (default: log errors as warnings)
+- `error_handler` - a Proc to call to handle any transient database errors that are raised (default: log errors as warnings)
 - `expiry_batch_size` - the batch size to use when deleting old records (default: `100`)
 - `expiry_method` - what expiry method to use `thread` or `job` (default: `thread`)
 - `expiry_queue` - which queue to add expiry jobs to (default: `default`)

diff --git a/lib/solid_cache/store/failsafe.rb b/lib/solid_cache/store/failsafe.rb
@@ -3,6 +3,16 @@
 module SolidCache
   class Store
     module Failsafe
+      TRANSIENT_ACTIVE_RECORD_ERRORS = [
+        ActiveRecord::AdapterTimeout,
+        ActiveRecord::ConnectionNotEstablished,
+        ActiveRecord::ConnectionTimeoutError,
+        ActiveRecord::Deadlocked,
+        ActiveRecord::LockWaitTimeout,
+        ActiveRecord::QueryCanceled,
+        ActiveRecord::StatementTimeout
+      ]
+
       DEFAULT_ERROR_HANDLER = ->(method:, returning:, exception:) do
         if Store.logger
           Store.logger.error { "SolidCacheStore: #{method} failed, returned #{returning.inspect}: #{exception.class}: #{exception.message}" }
@@ -20,7 +30,7 @@ def initialize(options = {})
 
         def failsafe(method, returning: nil)
           yield
-        rescue ActiveRecord::ActiveRecordError => error
+        rescue *TRANSIENT_ACTIVE_RECORD_ERRORS => error
           ActiveSupport.error_reporter&.report(error, handled: true, severity: :warning)
           error_handler&.call(method: method, exception: error, returning: returning)
           returning

diff --git a/test/unit/behaviors/failure_raising_behavior.rb b/test/unit/behaviors/failure_raising_behavior.rb
@@ -5,7 +5,7 @@ def test_fetch_read_failure_raises
     key = SecureRandom.uuid
     @cache.write(key, SecureRandom.alphanumeric)
 
-    assert_raise ActiveRecord::ActiveRecordError do
+    assert_raise ActiveRecord::StatementTimeout do
       emulating_unavailability do |cache|
         cache.fetch(key)
       end
@@ -17,7 +17,7 @@ def test_fetch_with_block_read_failure_raises
     value = SecureRandom.alphanumeric
     @cache.write(key, value)
 
-    assert_raise ActiveRecord::ActiveRecordError do
+    assert_raise ActiveRecord::StatementTimeout do
       emulating_unavailability do |cache|
         cache.fetch(key) { SecureRandom.alphanumeric }
       end
@@ -30,7 +30,7 @@ def test_read_failure_raises
     key = SecureRandom.uuid
     @cache.write(key, SecureRandom.alphanumeric)
 
-    assert_raise ActiveRecord::ActiveRecordError do
+    assert_raise ActiveRecord::StatementTimeout do
       emulating_unavailability do |cache|
         cache.read(key)
       end
@@ -45,23 +45,23 @@ def test_read_multi_failure_raises
       other_key => SecureRandom.alphanumeric
     )
 
-    assert_raise ActiveRecord::ActiveRecordError do
+    assert_raise ActiveRecord::StatementTimeout do
       emulating_unavailability do |cache|
         cache.read_multi(key, other_key)
       end
     end
   end
 
   def test_write_failure_raises
-    assert_raise ActiveRecord::ActiveRecordError do
+    assert_raise ActiveRecord::StatementTimeout do
       emulating_unavailability do |cache|
         cache.write(SecureRandom.uuid, SecureRandom.alphanumeric)
       end
     end
   end
 
   def test_write_multi_failure_raises
-    assert_raise ActiveRecord::ActiveRecordError do
+    assert_raise ActiveRecord::StatementTimeout do
       emulating_unavailability do |cache|
         cache.write_multi(
           SecureRandom.uuid => SecureRandom.alphanumeric,
@@ -79,7 +79,7 @@ def test_fetch_multi_failure_raises
       other_key => SecureRandom.alphanumeric
     )
 
-    assert_raise ActiveRecord::ActiveRecordError do
+    assert_raise ActiveRecord::StatementTimeout do
       emulating_unavailability do |cache|
         cache.fetch_multi(key, other_key) { |k| "unavailable" }
       end
@@ -90,7 +90,7 @@ def test_delete_failure_raises
     key = SecureRandom.uuid
     @cache.write(key, SecureRandom.alphanumeric)
 
-    assert_raise ActiveRecord::ActiveRecordError do
+    assert_raise ActiveRecord::StatementTimeout do
       emulating_unavailability do |cache|
         cache.delete(key)
       end
@@ -101,7 +101,7 @@ def test_exist_failure_raises
     key = SecureRandom.uuid
     @cache.write(key, SecureRandom.alphanumeric)
 
-    assert_raise ActiveRecord::ActiveRecordError do
+    assert_raise ActiveRecord::StatementTimeout do
       emulating_unavailability do |cache|
         cache.exist?(key)
       end
@@ -112,7 +112,7 @@ def test_increment_failure_raises
     key = SecureRandom.uuid
     @cache.write(key, 1, raw: true)
 
-    assert_raise ActiveRecord::ActiveRecordError do
+    assert_raise ActiveRecord::StatementTimeout do
       emulating_unavailability do |cache|
         cache.increment(key)
       end
@@ -123,15 +123,15 @@ def test_decrement_failure_raises
     key = SecureRandom.uuid
     @cache.write(key, 1, raw: true)
 
-    assert_raise ActiveRecord::ActiveRecordError do
+    assert_raise ActiveRecord::StatementTimeout do
       emulating_unavailability do |cache|
         cache.decrement(key)
       end
     end
   end
 
   def test_clear_failure_returns_nil
-    assert_raise ActiveRecord::ActiveRecordError do
+    assert_raise ActiveRecord::StatementTimeout do
       emulating_unavailability do |cache|
         cache.clear
       end

diff --git a/test/unit/behaviors_rails_7_2/failure_raising_behavior.rb b/test/unit/behaviors_rails_7_2/failure_raising_behavior.rb
@@ -5,7 +5,7 @@ def test_fetch_read_failure_raises
     key = SecureRandom.uuid
     @cache.write(key, SecureRandom.alphanumeric)
 
-    assert_raise ActiveRecord::ActiveRecordError do
+    assert_raise ActiveRecord::StatementTimeout do
       emulating_unavailability do |cache|
         cache.fetch(key)
       end
@@ -17,7 +17,7 @@ def test_fetch_with_block_read_failure_raises
     value = SecureRandom.alphanumeric
     @cache.write(key, value)
 
-    assert_raise ActiveRecord::ActiveRecordError do
+    assert_raise ActiveRecord::StatementTimeout do
       emulating_unavailability do |cache|
         cache.fetch(key) { SecureRandom.alphanumeric }
       end
@@ -30,7 +30,7 @@ def test_read_failure_raises
     key = SecureRandom.uuid
     @cache.write(key, SecureRandom.alphanumeric)
 
-    assert_raise ActiveRecord::ActiveRecordError do
+    assert_raise ActiveRecord::StatementTimeout do
       emulating_unavailability do |cache|
         cache.read(key)
       end
@@ -45,23 +45,23 @@ def test_read_multi_failure_raises
       other_key => SecureRandom.alphanumeric
     )
 
-    assert_raise ActiveRecord::ActiveRecordError do
+    assert_raise ActiveRecord::StatementTimeout do
       emulating_unavailability do |cache|
         cache.read_multi(key, other_key)
       end
     end
   end
 
   def test_write_failure_raises
-    assert_raise ActiveRecord::ActiveRecordError do
+    assert_raise ActiveRecord::StatementTimeout do
       emulating_unavailability do |cache|
         cache.write(SecureRandom.uuid, SecureRandom.alphanumeric)
       end
     end
   end
 
   def test_write_multi_failure_raises
-    assert_raise ActiveRecord::ActiveRecordError do
+    assert_raise ActiveRecord::StatementTimeout do
       emulating_unavailability do |cache|
         cache.write_multi(
           SecureRandom.uuid => SecureRandom.alphanumeric,
@@ -79,7 +79,7 @@ def test_fetch_multi_failure_raises
       other_key => SecureRandom.alphanumeric
     )
 
-    assert_raise ActiveRecord::ActiveRecordError do
+    assert_raise ActiveRecord::StatementTimeout do
       emulating_unavailability do |cache|
         cache.fetch_multi(key, other_key) { |k| "unavailable" }
       end
@@ -90,7 +90,7 @@ def test_delete_failure_raises
     key = SecureRandom.uuid
     @cache.write(key, SecureRandom.alphanumeric)
 
-    assert_raise ActiveRecord::ActiveRecordError do
+    assert_raise ActiveRecord::StatementTimeout do
       emulating_unavailability do |cache|
         cache.delete(key)
       end
@@ -101,7 +101,7 @@ def test_exist_failure_raises
     key = SecureRandom.uuid
     @cache.write(key, SecureRandom.alphanumeric)
 
-    assert_raise ActiveRecord::ActiveRecordError do
+    assert_raise ActiveRecord::StatementTimeout do
       emulating_unavailability do |cache|
         cache.exist?(key)
       end
@@ -112,7 +112,7 @@ def test_increment_failure_raises
     key = SecureRandom.uuid
     @cache.write(key, 1, raw: true)
 
-    assert_raise ActiveRecord::ActiveRecordError do
+    assert_raise ActiveRecord::StatementTimeout do
       emulating_unavailability do |cache|
         cache.increment(key)
       end
@@ -123,15 +123,15 @@ def test_decrement_failure_raises
     key = SecureRandom.uuid
     @cache.write(key, 1, raw: true)
 
-    assert_raise ActiveRecord::ActiveRecordError do
+    assert_raise ActiveRecord::StatementTimeout do
       emulating_unavailability do |cache|
         cache.decrement(key)
       end
     end
   end
 
   def test_clear_failure_returns_nil
-    assert_raise ActiveRecord::ActiveRecordError do
+    assert_raise ActiveRecord::StatementTimeout do
       emulating_unavailability do |cache|
         cache.clear
       end

diff --git a/test/unit/execution_test.rb b/test/unit/execution_test.rb
@@ -106,7 +106,7 @@ def test_active_record_instrumention_expiry
   end
 
   def test_no_connections_uninstrumented
-    ActiveRecord::ConnectionAdapters::ConnectionPool.any_instance.stubs(connection).raises(ActiveRecord::StatementInvalid)
+    ActiveRecord::ConnectionAdapters::ConnectionPool.any_instance.stubs(connection).raises(ActiveRecord::StatementTimeout)
 
     cache = lookup_store(expires_in: 60, active_record_instrumentation: false)
 
@@ -120,7 +120,7 @@ def test_no_connections_uninstrumented
   end
 
   def test_no_connections_instrumented
-    ActiveRecord::ConnectionAdapters::ConnectionPool.any_instance.stubs(connection).raises(ActiveRecord::StatementInvalid)
+    ActiveRecord::ConnectionAdapters::ConnectionPool.any_instance.stubs(connection).raises(ActiveRecord::StatementTimeout)
 
     cache = lookup_store(expires_in: 60)
 

diff --git a/test/unit/solid_cache_test.rb b/test/unit/solid_cache_test.rb
@@ -84,9 +84,9 @@ class SolidCacheFailsafeTest < ActiveSupport::TestCase
   def emulating_unavailability
     wait_for_background_tasks(@cache)
     stub_matcher = ActiveRecord::Base.connection.class.any_instance
-    stub_matcher.stubs(:exec_query).raises(ActiveRecord::StatementInvalid)
-    stub_matcher.stubs(:internal_exec_query).raises(ActiveRecord::StatementInvalid)
-    stub_matcher.stubs(:exec_delete).raises(ActiveRecord::StatementInvalid)
+    stub_matcher.stubs(:exec_query).raises(ActiveRecord::StatementTimeout)
+    stub_matcher.stubs(:internal_exec_query).raises(ActiveRecord::StatementTimeout)
+    stub_matcher.stubs(:exec_delete).raises(ActiveRecord::StatementTimeout)
     yield lookup_store(namespace: @namespace)
   ensure
     stub_matcher.unstub(:exec_query)
@@ -112,9 +112,9 @@ class SolidCacheRaisingTest < ActiveSupport::TestCase
   def emulating_unavailability
     wait_for_background_tasks(@cache)
     stub_matcher = ActiveRecord::Base.connection.class.any_instance
-    stub_matcher.stubs(:exec_query).raises(ActiveRecord::StatementInvalid)
-    stub_matcher.stubs(:internal_exec_query).raises(ActiveRecord::StatementInvalid)
-    stub_matcher.stubs(:exec_delete).raises(ActiveRecord::StatementInvalid)
+    stub_matcher.stubs(:exec_query).raises(ActiveRecord::StatementTimeout)
+    stub_matcher.stubs(:internal_exec_query).raises(ActiveRecord::StatementTimeout)
+    stub_matcher.stubs(:exec_delete).raises(ActiveRecord::StatementTimeout)
     yield lookup_store(namespace: @namespace,
       error_handler: ->(method:, returning:, exception:) { raise exception })
   ensure