launchdarkly / android-client-sdk

LaunchDarkly Client-side SDK for Android
Other
44 stars 23 forks source link

Cached flags are not persisted correctly #150

Closed eric-klukovich closed 1 year ago

eric-klukovich commented 2 years ago

The LaunchDarkly library caches fetched feature flags locally so they are known if the servers cannot be reached. We are encountering an issue where the cached flags are "cleared" when we load up the app offline after a Google Play store update.

We were able to track down the issue in the SDK code, and it appears that the DefaultUserManager uses a hashed value of the LDUser json string, which includes all fields. We created a LDUser using the builder code below, where the "version_code" gets updated after the release and causes the DefaultUserManager to create a new cache file instead of using the original one.

Is there a work-around where we can specify the LDUser cache name specifically, so it will not be impacted by any properties that have a dynamic value? Ideally, having the userId as the cache name would work for our use case.

val builder = LDUser.Builder(userId)
            .email(userEmailAddress)
            .firstName(userFirstName)
            .lastName(userLastName)
            .custom("company_id", companyId)
            .custom("project_id", projectId)
            .custom("version_code", appVersionCode)

We believe this method in the DefaultUserManager class is causing our issue:

public static String sharedPrefs(final LDUser user) {
     return HASHER.hash(toJson(user));
}

To reproduce

  1. Create a LDUser that a property that can change
  2. Get the current flags
  3. Change the property to another value
  4. Flags will not longer be cached

SDK version 3.1.2

Language version, developer tools Kotlin 1.5.31

OS/platform Android - All OS versions

vorobyov-denis commented 2 years ago

Looks like the issue I posted half an hour ago 😄

maxkohne commented 2 years ago

We are experiencing the same thing. What a coincidence you posted about it around the same time!

vorobyov-denis commented 2 years ago

It's amazing! Given that this bug has been present since version 2.0.4

louis-launchdarkly commented 2 years ago

Hello @eric-klukovich, @vorobyov-denis, @maxkohne, thank you for reaching out.

Just to clarify, is there a reason that the version_code is part of the User construction? Because you can target user using custom attribute, when a custom value changes, the SDK will need to get the flags again because the actual flag value can change. And from the Android Client-side SDK perspective, the SDK cannot tell even if you have no rules referring to the version_code.

@vorobyov-denis, I will reply to your issue separately - because if your User object didn't change, that seems to be a different issue compared to this one.

eric-klukovich commented 2 years ago

Hi @louis-launchdarkly!

We have the version_code as a custom attribute because we use it to target a flag specific app versions. For instance we want to turn on a feature for apps after a specific version. We are using the custom attributes to target specific users:

Screen Shot 2022-02-11 at 4 13 55 PM

The issue that that we are facing is that the LDClient creates a user flag cache name from the hashed JSON string created from LDUser model, including custom properties. In our case project_id and company_id are also dynamic so we would encounter this issue with those as well.

Here is an example of the LDUser JSON string:

{
  "firstName": "test_first_name",
  "lastName": "test_last_name",
  "key": "123456",
  "email": "test_email@test.com",
  "custom": {
    "version_code": 1,
    "company_id": "123",
    "os": 30,
    "project_id": "456",
    "device": "SM-G973U1 beyond1qlteue"
    }
}

Any time one of the custom property values change, then the hash code for this LDUser will differ and causing a new cache file to be created for the same user.

louis-launchdarkly commented 2 years ago

@eric-klukovich Thank you for clarifying. As described in your case, we know that is the same person behind the android device. However, if we reuse the old cache file, wouldn't the user potentially get incorrect flags? Or are you asking for something that can selectively update some cache entries but reuse the same cache file key? Note, the SDK will still need to fetch all flags even if we could replace some entries in the cache, so this will not save any time.

If your exact concern is the number of cache files (i.e. the SDK is creating too many cache files and is taking up unnecessary space), you could use the maxCachedUsers setting in the Config to reduce the number of cache files. https://javadoc.io/doc/com.launchdarkly/launchdarkly-android-client-sdk/latest/com/launchdarkly/sdk/android/LDConfig.Builder.html - The default is 5.

eric-klukovich commented 2 years ago

I'm asking for something to reuse the same cache file key, but the flags continue to be updated properly.

We are trying to solve an edge-case where the user has updated the app (version code changes) but the device is offline when they open the app. Since they are offline, no flags will be fetched from LD and they go to the default state. It would be better if the flags can use the last cached version of the flags instead.

louis-launchdarkly commented 2 years ago

Hello @eric-klukovich, thank you for being patient with me. Now I understand your case exactly - the cache key changed but there is no way to fetch new information (the app is offline) so the code gets the default. I will need to check how the caching refresh logic works and discuss it with the team before we can decide what is the next step. I will give an update once we looked into this.

Filed Internally as 142147.

eric-klukovich commented 2 years ago

Great! Thank you for the quick response! 😃

eric-klukovich commented 2 years ago

Hi @louis-launchdarkly, are there any updates on this?

maxkohne commented 2 years ago

@louis-launchdarkly Can we get a status update on this issue please?

louis-launchdarkly commented 2 years ago

Hello @eric-klukovich and @maxkohne, sorry for the wait - we were discussing this issue within the squad. What the Android SDK is doing doesn't line up with our typical approach (using the user key as the key to the cache to avoid this exact issue you have encountered).

As a heads up, because of the way we have hashed the user before, we don't think we can migrate the cache when we provide a fix for this issue. The end-user will need to populate the cache once more from scratch. The work is on our backlog and we will give another update when we have a change ready for review.

denis-bezrukov commented 1 year ago

@louis-launchdarkly I see LD bot mentioned the issue in 3.1.6 release, but I don't see anything related 🤔. Were there any updates on this?

As a heads up, because of the way we have hashed the user before, we don't think we can migrate the cache when we provide a fix for this issue. The end-user will need to populate the cache once more from scratch.

Can't you simply check if there is key using hash logic? It should work, if LDUser remains unchanged (e.g. if some user don't use version_code). The case when some LDUser's field was changed (e.g. version_code), it will cause a new cache entry creation, but we're not concerned with this, since it's already happen with the current hashing logic.

louis-launchdarkly commented 1 year ago

Hello @denis-bezrukov, unfortunately, the LD bot is a bit buggy and the changelog is the source of truth.

We can go into more detail when the new major version with a fix for this is up. You are on point - there will be some changes to the LDUser in the upcoming major version, so there will be a one-time reinitialization to get the cache ready again.

louis-launchdarkly commented 1 year ago

Hello @eric-klukovich, @vorobyov-denis, @maxkohne, @denis-bezrukov, Sorry for the wait. We have released the major release 4.x for Android, which contains the fix for this behavior - now the cache key should only be based on the context key.

Please feel free to open a new issue if there is any issue with the cache logic in 4.x.

maxkohne commented 1 year ago

@louis-launchdarkly thank you! Just so I fully understand, even if I upgrade to 4.x with the new Context system, it won't work unless we sign up for the Early Access Program, correct?

If that is correct, is there an estimated timeline that it will be fully released without the early access?

https://docs.launchdarkly.com/sdk/client-side/android/migration-3-to-4

louis-launchdarkly commented 1 year ago

@maxkohne Sorry to miss your follow-up question - The Context system was released on 27 Feb.

mruth1022 commented 1 year ago

We have released the major release 4.x for Android, which contains the fix for this behavior - now the cache key should only be based on the context key.

Please feel free to open a new issue if there is any issue with the cache logic in 4.x.

@louis-launchdarkly Can you explain how the cache key is derived from the LDContext, especially with multi-contexts? Or, can you provide some links to documentation which defines this behavior?

How would I ensure the cache key is unchanged (and the single cache is maintained) while updating custom attributes and nested contexts within a multi-context? For example, if my LDContext a multi-context made up of a User LDContext and a Location LDContext, how do I maintain a single cache while updating from nil Location to Location A, or from Location A to Location B, or from Location B to nil Location?