Flag caching's use of traits in the keys creates some odd side effects

[mjwills-k]

Note that when you read this at first glance you may think this is the same issue as #123 . I am reasonably certain it is not.

using Flagsmith;

namespace TestsForFlagSmith
{
    public class PoorCaching
    {
        protected static readonly Lazy<FlagsmithClient> _flagsmithClient = new Lazy<FlagsmithClient>(() => new FlagsmithClient(keyhere, cacheConfig: new CacheConfig(true) { DurationInMinutes = 1 }));

        [Test]
        public async Task CachingOfValuesIncludesTraitsInTheKey()
        {
            var identity = "someidentityhere";

            List<ITrait> traitList = [CreateTrait("name", null), CreateTrait("name1", "Matthew1"), CreateTrait("name2", "Matthew2")];

            var flagsWithoutTraits = await _flagsmithClient.Value.GetIdentityFlags(identity);

            Thread.Sleep(15000);

            var flagsWithTraits = await _flagsmithClient.Value.GetIdentityFlags(identity, traitList);

            Thread.Sleep(15000);

            var moreFlagsWithoutTraits = await _flagsmithClient.Value.GetIdentityFlags(identity);

            Thread.Sleep(15000);

            // So this one is fine
            var moreFlagsWithTraits = await _flagsmithClient.Value.GetIdentityFlags(identity, traitList);

            Assert.AreEqual(flagsWithoutTraits, moreFlagsWithoutTraits);
            Assert.AreEqual(flagsWithTraits, moreFlagsWithTraits);
            Assert.AreNotEqual(flagsWithTraits, flagsWithoutTraits);
        }

        private static ITrait CreateTrait(string traitName, string? traitValue)
        {
            return new Trait(traitName, traitValue);
        }
    }
}

Note that this test currently passes - but that doesn't mean I think it is optimal behaviour. It is just trying to represent what happens now.

The surprise I ran into here was that the traits are part of the cache key. This effectively makes the cache quite problematic unless you pass the full set of traits on every call.

Why is it problematic?

Request 1 comes in with no traits and gets a set of flags back.
Wait 15 seconds (so we aren't impacted by #123)
Request 2 comes in with some traits. These traits may impact the flags returned (e.g. due to entering a segment)
Wait 15 seconds (so we aren't impacted by #123)
Request 3 comes in with no traits. Now what you might expect is to get the same flags back as request 2. But no - you get the same as request 1. This is because the cache key includes the traits. Which is fine as long as you keep passing the traits every time. But if you don't, you may well be getting a stale set of flags.
Wait 15 seconds (so we aren't impacted by #123)
Request 4 comes in with some traits. This returns the same flags payload as request 2.

Do you have any advice here? The obvious solution is for me to stop relying on CacheConfig and build my own caching layer external to FlagsmithClient. But I'd rather avoid that if possible.

[rolodato]

Request 1 comes in with no traits and gets a set of flags back.

Request 2 comes in with some traits. These traits may impact the flags returned (e.g. due to entering a segment)

Request 3 comes in with no traits. Now what you might expect is to get the same flags back as request 2. But no - you get the same as request 1.

I believe this is the expected behaviour, since there's no guarantee that requests 2 and 3 will have the same result, so we can't cache them under the same entry. Why would you expect adding traits to create a new cache entry, but not removing traits?

A better solution might be to allow implementing your own cache, similar to what we already support for other SDKs: https://docs.flagsmith.com/clients/server-side?language=nodejs#caching. This is on our radar, just hasn't been prioritised.

[mjwills-k]

Why would you expect adding traits to create a new cache entry, but not removing traits?

a) I never removed any traits in requests 2 and 4. I just failed to send traits on those requests.
b) I would expect a cache to return to me the most up to date set of flags for a given identity. Which isn't what it does (it can't - since the cache key incorporates the traits).

I am not saying I know how to solve the issue, to be clear. But it definitely presents some real world problems if you don't send the full set of traits every time.

https://docs.flagsmith.com/basic-features/managing-identities#identity-and-trait-storage states:

Identities are persisted within the Flagsmith platform, along with any Traits that have been assigned to them. When Flags are evaluated for an Identity, the full complement of Traits stored within the platform are used, even if they were not all sent as part of the request.

(emphasis mine)
But once caching is used in the .NET client that doesn't happen.

[rolodato]

I see, thanks for the clarification. All server-side Flagsmith clients have caching implemented this way. If I understand correctly, to support the behaviour you describe, you would need to share trait state for your identities across your application's threads and/or processes, and possibly even other applications. This is essentially what the Flagsmith API's trait database is doing to support this behaviour.

By having a stateless caching strategy, we don't introduce external dependencies and avoid making assumptions about the concurrency model of client applications.

For contrast, the JS client does have the caching behaviour you describe, because it stores the current traits and flags in local storage.

If you're using .NET on the client side, that's a whole different story - this SDK might be able to run on the client but it was mainly built for the server.

[rolodato]

To add another data point, this is also how OpenFeature works; specifically, the dynamic context paradigm for server-side SDKs: https://openfeature.dev/docs/reference/concepts/evaluation-context/#dynamic-context-implementations-server-side-sdks

The evaluation context (i.e. identity and traits in Flagsmith terms) are always made explicit at evaluation time, and previous evaluations don't affect future evaluations. I think that having stateless evaluation makes reasoning about flags much easier, and I assume this is why OpenFeature chose this approach.

I'll close this issue for now, but feel free to comment if there's something else you wanted to add.