From abe22c8649c103720a0630edfc4a140b82b1119b Mon Sep 17 00:00:00 2001 From: Sojan Jose Date: Wed, 21 May 2025 20:00:11 -0700 Subject: [PATCH] fix: Rack-attack disable double Redis pooling (#11545) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Rails 7.1 ships with connection-pooling enabled by default for `RedisCacheStore` (see rails/rails#45235). Because we already wrap our Redis clients in our own `ConnectionPool` ($alfred / $velma), the upgrade resulted in a double-wrapped object and runtime errors such as: NoMethodError: undefined method `get` for an instance of ConnectionPool This patch: * Passes `pool: false` when instantiating `RedisCacheStore` in `config/initializers/rack_attack.rb`, telling Rails to use the pool we supply instead of building its own. * Adds an inline comment explaining the rationale. * Adds a TODO in `config/initializers/01_redis.rb` suggesting a future simplification: switch to plain Redis clients and let Rails manage the pool. Reference docs: * rails/rails#45235 – “Enable connection pooling by default for MemCacheStore and RedisCacheStore” - https://github.com/rails/rails/pull/45235 * Rails 7.1 Caching Guide – 2.1.1 “Connection Pool Options” (use `pool: false`) [Ruby on Rails Guides](https://guides.rubyonrails.org/v7.1/caching_with_rails.html) --- config/initializers/01_redis.rb | 4 ++++ config/initializers/rack_attack.rb | 7 ++++++- 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/config/initializers/01_redis.rb b/config/initializers/01_redis.rb index 664dcd435..93c12fce7 100644 --- a/config/initializers/01_redis.rb +++ b/config/initializers/01_redis.rb @@ -1,3 +1,7 @@ +# TODO: Phase out the custom ConnectionPool wrappers ($alfred / $velma), +# switch to plain Redis clients here and let Rails 7.1+ handle pooling +# via `pool:` in RedisCacheStore (see rack_attack initializer). + # Alfred # Add here as you use it for more features # Used for Round Robin, Conversation Emails & Online Presence diff --git a/config/initializers/rack_attack.rb b/config/initializers/rack_attack.rb index fdbd47008..fe3f6c554 100644 --- a/config/initializers/rack_attack.rb +++ b/config/initializers/rack_attack.rb @@ -11,7 +11,12 @@ class Rack::Attack # Rack::Attack.cache.store = ActiveSupport::Cache::MemoryStore.new # https://github.com/rack/rack-attack/issues/102 - Rack::Attack.cache.store = ActiveSupport::Cache::RedisCacheStore.new(redis: $velma) + # Rails 7.1 automatically adds its own ConnectionPool around RedisCacheStore. + # Because `$velma` is *already* a ConnectionPool, double-wrapping causes + # Redis calls like `get` to hit the outer wrapper and explode. + # `pool: false` tells Rails to skip its internal pool and use ours directly. + # TODO: We can use build in connection pool in future upgrade + Rack::Attack.cache.store = ActiveSupport::Cache::RedisCacheStore.new(redis: $velma, pool: false) class Request < ::Rack::Request # You many need to specify a method to fetch the correct remote IP address