Fix RedisCacheStore INCR/DECR for Redis < v7.0.0

This commit fixes a discrepancy in the behavior of the `#increment` and
`#decrement` methods in `RedisCacheStore` when used with Redis versions less
than 7.0.0. The existing condition `count != amount` prevented setting the
Time-To-Live (TTL) for keys that were equal to the increment/decrement amount
after the `INCRBY`/`DECRBY` operation. This occurs when incrementing a
non-existent key by `1`, for example.

Using Redis pipelining, we minimize the network overhead incurred by checking
for existing TTLs. It decouples the TTL operations from the increment/decrement
operation, allowing the TTL to be set correctly regardless of the resulting
value from the `INCRBY`/`DECRBY`.

New tests have been added to verify the correct behavior of `#increment` and
`#decrement` methods, specifically when the `expires_in` option is not used.
Using a separate cache store instance (`@cache_no_ttl`), these tests ensure that
keys are correctly incremented or decremented and that their TTL remains unset.

Co-authored-by: Benjamin Quorning <benjamin@quorning.net>
Co-authored-by: Jury Razumau <jury.razumau@zendesk.com>
Co-authored-by: Edyta Rozczypała <edyta.rozczypala@zendesk.com>

activesupport/lib/active_support/cache/redis_cache_store.rb

@@ -438,18 +438,26 @@ def change_counter(key, amount, options)
           redis.then do |c|
             c = c.node_for(key) if c.is_a?(Redis::Distributed)
 
-            if options[:expires_in] && supports_expire_nx?
-              c.pipelined do |pipeline|
-                pipeline.incrby(key, amount)
-                pipeline.call(:expire, key, options[:expires_in].to_i, "NX")
-              end.first
+            expires_in = options[:expires_in]
+
+            if expires_in
+              if supports_expire_nx?
+                count, _ = c.pipelined do |pipeline|
+                  pipeline.incrby(key, amount)
+                  pipeline.call(:expire, key, expires_in.to_i, "NX")
+                end
+              else
+                count, ttl = c.pipelined do |pipeline|
+                  pipeline.incrby(key, amount)
+                  pipeline.ttl(key)
+                end
+                c.expire(key, expires_in.to_i) if ttl < 0
+              end
             else
               count = c.incrby(key, amount)
-              if count != amount && options[:expires_in] && c.ttl(key) < 0
-                c.expire(key, options[:expires_in].to_i)
-              end
-              count
             end
+
+            count
           end
         end
 

activesupport/test/cache/stores/redis_cache_store_test.rb

@@ -141,6 +141,8 @@ class StoreTest < ActiveSupport::TestCase
       @cache = lookup_store(expires_in: 60)
       # @cache.logger = Logger.new($stdout)  # For test debugging
 
+      @cache_no_ttl = lookup_store
+
       # For LocalCacheBehavior tests
       @peek = lookup_store(expires_in: 60)
     end
@@ -193,6 +195,23 @@ def test_write_expires_at
       end
     end
 
+    def test_increment_ttl
+      # existing key
+      redis_backend(@cache_no_ttl) { |r| r.set "#{@namespace}:jar", 10 }
+      @cache_no_ttl.increment "jar", 1
+      redis_backend(@cache_no_ttl) do |r|
+        assert r.get("#{@namespace}:jar").to_i == 11
+        assert r.ttl("#{@namespace}:jar") < 0
+      end
+
+      # new key
+      @cache_no_ttl.increment "kar", 1
+      redis_backend(@cache_no_ttl) do |r|
+        assert r.get("#{@namespace}:kar").to_i == 1
+        assert r.ttl("#{@namespace}:kar") < 0
+      end
+    end
+
     def test_increment_expires_in
       @cache.increment "foo", 1, expires_in: 60
       redis_backend do |r|
@@ -208,13 +227,30 @@ def test_increment_expires_in
       end
 
       # key exist but not have expire
-      redis_backend { |r| r.set "#{@namespace}:dar", 10 }
-      @cache.increment "dar", 1, expires_in: 60
-      redis_backend do |r|
+      redis_backend(@cache_no_ttl) { |r| r.set "#{@namespace}:dar", 10 }
+      @cache_no_ttl.increment "dar", 1, expires_in: 60
+      redis_backend(@cache_no_ttl) do |r|
         assert r.ttl("#{@namespace}:dar") > 0
       end
     end
 
+    def test_decrement_ttl
+      # existing key
+      redis_backend(@cache_no_ttl) { |r| r.set "#{@namespace}:jar", 10 }
+      @cache_no_ttl.decrement "jar", 1
+      redis_backend(@cache_no_ttl) do |r|
+        assert r.get("#{@namespace}:jar").to_i == 9
+        assert r.ttl("#{@namespace}:jar") < 0
+      end
+
+      # new key
+      @cache_no_ttl.decrement "kar", 1
+      redis_backend(@cache_no_ttl) do |r|
+        assert r.get("#{@namespace}:kar").to_i == -1
+        assert r.ttl("#{@namespace}:kar") < 0
+      end
+    end
+
     def test_decrement_expires_in
       @cache.decrement "foo", 1, expires_in: 60
       redis_backend do |r|
@@ -230,9 +266,9 @@ def test_decrement_expires_in
       end
 
       # key exist but not have expire
-      redis_backend { |r| r.set "#{@namespace}:dar", 10 }
-      @cache.decrement "dar", 1, expires_in: 60
-      redis_backend do |r|
+      redis_backend(@cache_no_ttl) { |r| r.set "#{@namespace}:dar", 10 }
+      @cache_no_ttl.decrement "dar", 1, expires_in: 60
+      redis_backend(@cache_no_ttl) do |r|
         assert r.ttl("#{@namespace}:dar") > 0
       end
     end
@@ -252,8 +288,8 @@ def test_decrement_expires_in
       end
     end
 
-    def redis_backend
-      @cache.redis.with do |r|
+    def redis_backend(cache = @cache)
+      cache.redis.with do |r|
         yield r if block_given?
         return r
       end