From e30afecf544465a445635ec14f5b266c1844ddfe Mon Sep 17 00:00:00 2001 From: Donal McBreen Date: Tue, 3 Sep 2024 17:14:29 +0100 Subject: [PATCH] Only failsafe on transient database errors Avoid rescuing all ActiveRecord errors in the failsafe block as we might be hiding configuration errors. Fixes: https://github.com/rails/solid_cache/issues/182 --- README.md | 2 +- lib/solid_cache/store/failsafe.rb | 12 +++++++++- .../behaviors/failure_raising_behavior.rb | 24 +++++++++---------- .../failure_raising_behavior.rb | 24 +++++++++---------- test/unit/execution_test.rb | 4 ++-- test/unit/solid_cache_test.rb | 12 +++++----- 6 files changed, 44 insertions(+), 34 deletions(-) diff --git a/README.md b/README.md index 68732d4..7355c44 100644 --- 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 index 2f5c711..c75866f 100644 --- 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 index 2c28806..7afd425 100644 --- 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,7 +45,7 @@ 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 @@ -53,7 +53,7 @@ def test_read_multi_failure_raises 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 @@ -61,7 +61,7 @@ def test_write_failure_raises 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,7 +123,7 @@ 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 @@ -131,7 +131,7 @@ def test_decrement_failure_raises 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 index 2c28806..7afd425 100644 --- 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,7 +45,7 @@ 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 @@ -53,7 +53,7 @@ def test_read_multi_failure_raises 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 @@ -61,7 +61,7 @@ def test_write_failure_raises 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,7 +123,7 @@ 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 @@ -131,7 +131,7 @@ def test_decrement_failure_raises 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 index 590f2aa..ba84f89 100644 --- 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 index d3306f7..5129de0 100644 --- 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