Fix RedisCacheStore INCR/DECR for Redis < v7.0.0

[Thomascountz]

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.

Motivation / Background

We noticed the test failures on the main branch for Redis versions <7.0.0. The culprit was a change introduced in #45711, where a condition was added that prevents the TTL from being set. This happens when a key without TTL is incremented or decremented and the resulting value equals the amount it is incremented/decremented by.

This breaks the expected behavior of setting the TTL on the first call to increment when :expires_in is provided, particularly when the operation results in the key's value set to 1. This happens often in rate limiting scenarios when the key is either initialized to 0 by the application, or by Redis, before being incremented to 1.

Detail

This PR modifies the change_counter method in RedisCacheStore, removing the count != amount check.

With this fix, TTL is set upon the first increment operation, irrespective of the counter's value, aligning the behavior outlined in the specs.

For example:

rails/activesupport/test/cache/stores/redis_cache_store_test.rb

Lines 196 to 201 in f11c651

	def test_increment_expires_in
	@cache.increment "foo", 1, expires_in: 60
	redis_backend do |r|
	assert r.exists?("#{@namespace}:foo")
	assert r.ttl("#{@namespace}:foo") > 0
	end

Because the foo key is not defined, ...it is set to 0 before performing the operation... by Redis. When it performs increment, the value will be 1. Once the value is 1, #change_counter will fail to issue EXPIRE.

rails/activesupport/lib/active_support/cache/redis_cache_store.rb

Lines 447 to 450 in f11c651

	count = c.incrby(key, amount)
	if count != amount && options[:expires_in] && c.ttl(key) < 0
	c.expire(key, options[:expires_in].to_i)
	end

Here, count is 1 and amount is 1, making the count != amount condition false.

In the original code before the patch, the TTL was set in the first call to #increment, given that the :expires_in option was provided.

rails/activesupport/lib/active_support/cache/redis_cache_store.rb

Lines 239 to 242 in 6bfc637

	redis.with do |c|
	c.incrby(key, amount).tap do
	write_key_expiry(c, key, options)
	end

Here, the TTL was set within a tap block that executes after the increment operation, irrespective of the counter's value.

Additional Details

Test Failures

$ bin/test test/cache/stores/redis_cache_store_test.rb --defer-output

Running 626 tests in parallel using 6 processes
Run options: --defer-output --seed 53989

# Running:

...................................................................................................................................................................S..........................................................................................................F........................................F........................................................................................................................................S.........................................................................................................F.......................................F...............................

Finished in 4.126056s, 151.7187 runs/s, 579.9728 assertions/s.

  1) Failure:
ActiveSupport::Cache::RedisCacheStoreTests::RedisCacheStoreCommonBehaviorTest#test_decrement_expires_in [/workspaces/rails/activesupport/test/cache/stores/redis_cache_store_test.rb:222]:
Expected false to be truthy.

  2) Failure:
ActiveSupport::Cache::RedisCacheStoreTests::RedisCacheStoreCommonBehaviorTest#test_increment_expires_in [/workspaces/rails/activesupport/test/cache/stores/redis_cache_store_test.rb:200]:
Expected false to be truthy.

  3) Failure:
ActiveSupport::Cache::RedisCacheStoreTests::RedisCacheStoreWithDistributedRedisTest#test_decrement_expires_in [/workspaces/rails/activesupport/test/cache/stores/redis_cache_store_test.rb:222]:
Expected false to be truthy.

  4) Failure:
ActiveSupport::Cache::RedisCacheStoreTests::RedisCacheStoreWithDistributedRedisTest#test_increment_expires_in [/workspaces/rails/activesupport/test/cache/stores/redis_cache_store_test.rb:200]:
Expected false to be truthy.

626 runs, 2393 assertions, 4 failures, 0 errors, 2 skips

You have skipped tests. Run with --verbose for details.

Failed tests:

bin/test test/cache/stores/redis_cache_store_test.rb:218
bin/test test/cache/stores/redis_cache_store_test.rb:196
bin/test test/cache/stores/redis_cache_store_test.rb:218
bin/test test/cache/stores/redis_cache_store_test.rb:196

Test Infrastructure Setup via Docker Compose

version: '3'

services:
  rails:
    build:
      context: ..
      dockerfile: .devcontainer/Dockerfile

    volumes:
      - ../..:/workspaces:cached

    # Overrides default command so things don't shut down after the process ends.
    command: sleep infinity

    # Runs app on the same network as the database container, allows "forwardPorts" in devcontainer.json function.
    networks:
      - default

    depends_on:
      - postgres
      - mariadb
      - redis
      - memcached

    # Use "forwardPorts" in **devcontainer.json** to forward an app port locally.
    # (Adding the "ports" property to this file will not forward from a Codespace.)

  postgres:
    image: postgres:latest
    restart: unless-stopped
    networks:
      - default
    volumes:
      - postgres-data:/var/lib/postgresql/data
    environment:
      POSTGRES_USER: postgres
      POSTGRES_DB: postgres
      POSTGRES_PASSWORD: postgres

  mariadb:
    image: mariadb:latest
    restart: unless-stopped
    networks:
      - default
    volumes:
      - mariadb-data:/var/lib/mysql
    environment:
      MARIADB_ROOT_PASSWORD: root

  redis:
-    image: redis:latest
+    image: redis:6.2
    restart: unless-stopped
    networks:
      - default
    volumes:
      - redis-data:/data

  memcached:
    image: memcached:latest
    restart: unless-stopped
    command: ["-m", "1024"]
    networks:
      - default

networks:
  default:

volumes:
  postgres-data:
  mariadb-data:
  redis-data:

Checklist

Before submitting the PR make sure the following are checked:

• This Pull Request is related to one change. Changes that are unrelated should be opened in separate PRs.
• Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
• Tests are added or updated if you fix a bug or add a feature.
• CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.

[Thomascountz]

@fatkodima, given your expertise in this area, I'd love if you took a look! I might be missing some context around why the conditional was added. Thanks! 😄

[byroot]

I might be missing some context around why the conditional was added.

The goal was to avoid calling ttl, which is an extra roundtrip to the server.

[Thomascountz]

Thank you for all the guidance and feedback, @byroot! I've pushed an update and added a few tests. Please let me know if there's anything else you'd like addressed.

[byroot]

Some small stuff, but looks good to me.

[byroot]

Backported to 7-1-stable as c3117b5

[byroot]

Thank you!