Ah this was a silly question, the usage is in D7254 but basically we need to set it after decrypting every time

yeah, makes sense

basically we need to set it after decrypting every time

exactly - this is for saving items to cache

Yeah, I had a similar scenario in mind while putting these comments. My first idea was to get rid of the memory cache, but I decided to give it "limited trust" and rebuild if needed

We could do simple promise-deduping here but it won't save us completely.
The "filtering instead of replacing" solution I proposed below (L86-89) would work here. Let me know if you think it's worth doing

I might be missing something, but I don't understand how the "filtering instead of replacing" solution would solve the issue.

My concern goes like this:

1. Two images are displayed on screen at the same time, triggering calls to set (and consequently to cleanupCacheIfNecessary) at around the same time.
2. The first set call has to temporarily clear the uriCache. We can't avoid this with "filtering", since we won't know what to filter until after the persistence.cleanupOldFiles call.
3. The second set call now backs up the in-memory cache, but it is an empty cache because the first cache cleared it.
4. Both set calls proceed. At the end of the second call, if any files were deleted, it will reset uriCache to backupMemoryCache, which is an empty set.

I think the "filtering instead of replacing" solution won't help because it still needs the in-memory cache to be temporarily cleared while the cleanup is happening. (Correct me if I'm wrong!)

I can see two solutions to this:

1. Make sure the files are removed from the in-memory cache before they are removed from disk
   • You could imagine passing a callback into persistence.cleanupOldFiles to achieve this
2. Make sure only one cleanup can occur at-a-time. Two sub-options:
   • If a cleanup is already in progress, then simply skip cleanup. The second call will miss the cache, but won't wipe the cache accidentally
   • The second call could still do a cleanup, but it could be forced to wait for the first cleanup to conclude before proceeding

I think it's okay either way, but I would probably pass a callback to persistence.cleanupOldFiles that just does a single Map.delete, which cleanupOldFiles can call before removing the file on disk.

Is there a risk that a currently-used cache file will be deleted by the cleanup call? I guess we can assume that the currently-used cache file will be in memory by the time the file is deleted, but we'll need to be careful to make sure we don't eg. get the file path, await something, and THEN display the file, as the file could've been deleted in the interim

I wonder if it would be better / more simple to just do cleanup when the app starts instead of having it done on every set. I don't think it's very important that the cache stays strictly below the limit... if it goes above it for some time until the next app start, it's probably okay

What's the best place to do this?

We probably want to block the app render on cleanup, right?

Hmmm... my first though was in NavigationHandler. We can block it from returning`LogInHandler`, which will keep the UI from "logging in" (we won't dismiss LoggedOutModal).

However, the app is rendered underneath LoggedOutModal... I'm not sure if there is a risk of an image being loaded in that scenario.

We could do it in SQLiteDataHandler instead – that way, we could make sure cleanup occurs before the upload data is in Redux, which would be a "stronger" guarantee. It feels strange to call image cache code from a component meant to handle SQLite data, but it might be the best place.

Do you know how long the call might take?