Fix getAllKeys when keys are removed from cache

[janicduplessis]

Details

When testing an account with a very large amount of chats we hit an issue where onyx will return undefined for an item that actually exists in storage. This is because that item was evicted from Onyx cache.

When items are evicted from cache we should not delete the key in storageKeys since this is used to determine if the item exists in storage.

Onyx uses that value to get all keys as soon as it has some items (https://github.com/Expensify/react-native-onyx/blob/main/lib/Onyx.js#L115). This means getAllKeys from cache needs to return all keys in Storage, not keys currently cached.

Related Issues

N/A

Automated Tests

Updated a test to add a check for getAllKeys after keys are evicted from cache.

Linked PRs

[arosiclair]

Is this change coming from slack or an issue somewhere? I don't have any context for this

[janicduplessis]

https://expensify.slack.com/archives/C035J5C9FAP/p1695378712978879 Has a little bit of context.

The issue is that when the cache is full keys are removed from the array returned by getAllKeys and then some items will start to return null even if they still exist in the db and could be fetched. This is because we return the keys from cache as soon as we have one, so it needs to be all keys that are in the db and not all the keys that we have cached values for.

If you have an account with very large amount of chats, or by reducing cache size to a small value you should be able to reproduce a crash by scrolling in the chats list.

[arosiclair]

Thanks for the context

If you have an account with very large amount of chats, or by reducing cache size to a small value you should be able to reproduce a crash by scrolling in the chats list.

Can you write specific testing steps for this? I'll verify that it's also working on my end

[janicduplessis]

@arosiclair The easiest way to repro without an account with a very large amount of data is to change the max cache size here to a small value like 30. Then when loading the app you should see the following error.

After this patch everything should work.

Edit: Actually setting a small cache size doesn't seem to work as the cache is too small to hold all keys displayed and will crash more. Currently the only way I have to repro this is with an account with very large amount of data (more than regular high traffic).

I think this fix is still useful and can prevent some crashes but we might want to investigate a way to retain keys that are currently rendered so they are not evicted from cache as this will cause crashes if it happens.

[mountiny]

@arosiclair are you happy to merge this? I think this can be tested with the App and the automated test added is handy here

[mountiny]

conflicts

[arosiclair]

Alright let's just rely on unit tests for now.

I do feel that we should probably increase the required testing for these Onyx core changes. I've seen regressions slip through from these (example) and they are naturally very hard to trace when they happen. Anyway that's a discussion for another day

[mountiny]

I do feel that we should probably increase the required testing for these Onyx core changes. I've seen regressions slip through from these (Expensify/App#26190) and they are naturally very hard to trace when they happen. Anyway that's a discussion for another day

I agree the current set up makes it hard to test the changes before actaully merging Onyx PR hard and it would be great to streamline this

[mountiny]

merged as part of #363