[CACHE] Never hits cache with TagAwareAdapter about symfony

[Fahl-Design]

@nicolas-grekas here we go https://github.com/Fahl-Design/cache-issue-reproducer

*edit: inside redis: "tags" is always set as a key suffix NOT prefix

https://github.com/Fahl-Design/cache-issue-reproducer edit: updated to make it more visible and symfony v7 as well can be reproduces in v6.4 and v7.0

[AtCliffUnderline]

@nicolas-grekas Hello! Very sorry for bothering you. Could you have a look at this please? Could be cause of #53846

[nicolas-grekas]

Can you try to figure out what happens?

[AtCliffUnderline]

I'm trying, but tbh i'm a little bit confused with TagAwareAdapter code :)
Right now I understood that even invalidateTags is broken, it removes only tag itself from Redis, without removing KV pairs that belongs to it.

[stof]

I don't think invalidateTags is broken. See the phpdoc of the class which provides some explanation about its architecture.

[AtCliffUnderline]

Well, i compared with my current 5.4.34 and I agree that behaviour of invalidateTags is the same. But still something going wrong

[AtCliffUnderline]

Index: vendor/symfony/cache/Adapter/AbstractTagAwareAdapter.php
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/vendor/symfony/cache/Adapter/AbstractTagAwareAdapter.php b/vendor/symfony/cache/Adapter/AbstractTagAwareAdapter.php
--- a/vendor/symfony/cache/Adapter/AbstractTagAwareAdapter.php	
+++ b/vendor/symfony/cache/Adapter/AbstractTagAwareAdapter.php	(date 1709196146551)
@@ -56,7 +56,7 @@
                 $item->isHit = $isHit;
                 // Extract value, tags and meta data from the cache value
                 $item->value = $value['value'];
-                $item->metadata[CacheItem::METADATA_TAGS] = isset($value['tags']) ? array_combine($value['tags'], $value['tags']) : [];
+                $item->metadata[CacheItem::METADATA_TAGS] = isset($value['tags']) ? $value['tags'] : [];
                 if (isset($value['meta'])) {
                     // For compactness these values are packed, & expiry is offset to reduce size
                     $v = unpack('Ve/Nc', $value['meta']);
@@ -107,7 +107,6 @@
                     foreach (array_diff_key($oldTags, $value['tags']) as $removedTag) {
                         $value['tag-operations']['remove'][] = $getId($tagPrefix.$removedTag);
                     }
-                    $value['tags'] = array_keys($value['tags']);
 
                     $byLifetime[$ttl][$getId($key)] = $value;
                     $item->metadata = $item->newMetadata;

wdyt? Saving and retrieving tags as associative array . Comparing is correct after this because we not losing these 6 random_bytes which are version.

[AtCliffUnderline]

Can I up this? Still thinks this is pretty critical

[tecbird]

+1

[Fahl-Design]

+1

[nicolas-grekas]

I would need more insights to be of any help.

I tried your reproducer @AtCliffUnderline but I miss an observer to tell which behavior is wrong. If I dump($notHitItems), I don't see any difference between v6.4.3 and v6.4.4.

Note that your wiring is really strange: a RedisTagAwareAdapter inside a TagAwareAdapter?

[Fahl-Design]

@nicolas-grekas here is an other example, maybe even simpler to reproduce

I have the same issue with this config based on the docs of the chain adapter with redis tag aware i guess (should not matter at all, looks like a cache tag problem anyways)

services:
    redis.provider.one:
        class: Redis
        factory: [ 'Symfony\Component\Cache\Adapter\RedisTagAwareAdapter', 'createConnection' ]
        arguments:
            - '%env(CACHE_REDIS_DSN_DEFAULT)%/6'

    redis.provider.two:
        class: Redis
        factory: [ 'Symfony\Component\Cache\Adapter\RedisTagAwareAdapter', 'createConnection' ]
        arguments:
            - '%env(WEBSHOP_REDIS_DSN_FLATDATA)%'
            - { retry_interval: 1, timeout: 10 }

    'cache.adapter.redis.one':
        parent: 'cache.adapter.redis_tag_aware'
        arguments:
            $redis: '@redis.provider.one'
            $namespace: 'fancy_namespace'
      
    'cache.adapter.redis.two':
        parent: 'cache.adapter.redis_tag_aware'
        arguments:
            $redis: '@redis.provider.two'
            $namespace: 'other_fancy_namespace'

framework:
    cache:
        pools:
            fancy_cache_pool:
                default_lifetime: 31536000  # One year
                adapters:
                    - cache.adapter.redis.one
                    - cache.adapter.redis.two
                tags: true

If you run this twice, it is build twice (or more)

        $myCachedData = $this->cachePool->get(
            key: 'fancy_cache_key',
            callback: function (ItemInterface $item): string {
                dump($item->getKey() . ' was build ');
                $item->tag(['tag1', 'tag2']);

                return 'my cached value';
            });

you will see the dump "was build" every time, meaning no cache at all

• clear your cache
• now remove the tag
• repeat and see expected 1 time "build"

        $myCachedData = $this->cachePool->get(
            key: 'fancy_cache_key_2',
            callback: function (ItemInterface $item): string {
                dump($item->getKey() . ' was build ');
                // $item->tag(['tag1', 'tag2']); <-- no tags

                return 'my cached value';
            });

cache works as expected

possible cause:
the use of "TAGS_PREFIX" is inconsistent or just not named correct, sometimes it is used as prefix, sometime suffix. Redis gets data filled with "tags" as suffix and never has data with "tags" as prefix (getTagVersions can not work, because no data in redis with tags as prefix)

• prefix:
  https://github.com/symfony/symfony/blob/6.4/src/Symfony/Component/Cache/Adapter/TagAwareAdapter.php#L123
• suffix:
  https://github.com/symfony/symfony/blob/6.4/src/Symfony/Component/Cache/Adapter/TagAwareAdapter.php#L339

v6.4

I hope this may help to fix this

[nicolas-grekas]

Can you please put the reproducer in a small app I could clone ?

[Fahl-Design]

Of cause, give me a minute or 10 ;)

[nicolas-grekas]

Thanks for the reproducer app @Fahl-Design

Now I'm confused. Is this issue new? For some reason, I had the understanding that this was introduced in 6.4.4, but it's not, right? The reproducer behaves the same with 7.0.0 or 7.0.4, or I missed something.

Could any of you tell me why you're putting a RedisTagAwareAdapter in a chain that's then given to TagAwareAdapter? This doesn't make any sense to me and that'd the other thing I'd need to clarify.

[Fahl-Design]

You'r welcome, I found this issue mainly by accident ;) I use 6.4 in an app and this was my first time using a tagged cache pool.
There is a 6.4 version in the reproducer version as well, after that i updated to 7.0 and still reproduce it.

Could any of you tell me why you're putting a RedisTagAwareAdapter in a chain that's then given to TagAwareAdapter? This doesn't make any sense to me and that'd the other thing I'd need to clarify.

maybe the yaml don't make it clear ;)

...
    redis.provider.two:
        class: Redis
        factory: [ 'Symfony\Component\Cache\Adapter\RedisTagAwareAdapter', 'createConnection' ]
...

redis.provider.two is just the \Redis instance https://github.com/symfony/symfony/blob/6.4/src/Symfony/Component/Cache/Traits/RedisTrait.php#L87

I guess that's from my understanding of the docs
https://symfony.com/doc/current/components/cache/adapters/redis_adapter.html#working-with-tags
https://symfony.com/doc/current/cache.html#creating-a-cache-chain

isn't that how the pools should be set up to work?^^
I mean I don't get any autowiring issues and it doesn't look to bad

I attempt is to setup a cache pool with tags in a chain to fill a second redis instance for an other app.
It works fine and as expected as long as i don't use the tags

when you run bin/console app:cache-test --add-tags you can see the pool will not give you the cached value, and calls the get callback every time.

I had a look at the corresponding code but man, that's not easy to follow (and when tags are involved)
any ideas?

[nicolas-grekas]

OK, I think I get what we want to achieve. There are two aspects here:

Why does it not work as is?

My guess is that there is some bad interaction between RedisTagAwareAdapter and TagAwareAdapter. We could try to understand why and figure out a way around, but the wiring doesn't make sense in the first place to me. This leads to the second point;

What are you trying to achieve?

My guess is you're trying to have a cache that stores things locally (ArrayAdapter) on top of a remote cache storage (Redis), and you want that chain to be able to manage tags. You can achieve this immediately by not using RedisTagAwareAdapter in the chain. Just use a regular RedisAdapter.

This means the storage layout is going to use versioned tags instead of tag relationships (what RedisTagAwareAdapter manages). Tag relationships require more storage but save a few roundtrips. If you don't have an objective reason to use them, versioned tags are really good anyway.

Then remains the question of wiring an ArrayAdapter on top of a RedisTagAwareAdapter. I think we would need a TagAwareChainAdapter and TagAwareArrayAdapter for this. Or another better idea :)

[Fahl-Design]

@nicolas-grekas

I will open a new issue with a new title.
I'm not the author of this issue, so I cant edit this one.

In my case, and the reproducer code, there are no array adapters

[nicolas-grekas]

Yes, the array adapter is not the issue so it doesn't matter.