[Fix] Model store saving to and loading from prefs, and remove duplicates

[nan-li]

Description

One Line Summary

Don't overwrite operation model store cache before it as been loaded, and avoid duplicates when loading them.

Details

Bug Fix

• This is to remedy the Operation Model Store adding duplicate operations when the same ones that were previously added to the store and persisted, are re-read from cache
• Though the duplicate would not get enqueued to the Operation Repo, the duplicate was still persisted. When the operations are successful, only one of the duplicates will be removed from cache as the Model Store's removal logic utilizes a firstOrNull search for the operation. Then, on the next cold start, the remaining operation will load from cache and become enqueued to the OperationRepo
• Bug introduced in 5.1.12 with the Operation-loading changes to prevent ANRs
• See manual testing below for more information

Solution

• Fix model store to not overwrite cache. Don't persist models before load is called. This is safe because the time gap between any models being added and load being called should be very small.
• When load is called, the cached models are added to the front. Then the models get persisted altogether again if necessary.
• Add check for duplicate operations when loading model store

Motivation

Bug fix

Testing

Unit testing

Added test that saves duplicates to Prefs, enqueues a model, then loads from cache.

Manual testing

Android emulator on API 33
Reproduce Bug Before PR

1. New app install
2. A login-user and a create-subscription are quickly enqueued and persisted
3. The Operation Model Store then loads from cache and reads the 2 operations above
4. They get added to the store
5. The store now has 4 operations (2 of each)
6. The operations enqueue to the Operation Repo but it does prevents the duplicates from enqueueing
7. The Operation Repo flushes the login-user and create-subscription
8. On success, they are removed from the model store via firstOrNull
9. The Operation Model Store now has 2 operations left
10. On the next cold start, the login-user and the create-subscription are read from cache and added to the Operation Repo

Affected code checklist

• Notifications
  • Display
  • Open
  • Push Processing
  • Confirm Deliveries
• Outcomes
• Sessions
• In-App Messaging
• REST API requests
• Public API changes

Checklist

Overview

• I have filled out all REQUIRED sections above
• PR does one thing
• Any Public API changes are explained in the PR details and conform to existing APIs

Testing

• I have included test coverage for these changes, or explained why they are not needed
• All automated tests pass, or I explained why that is not possible
• I have personally tested this on my device, or explained why that is not possible

Final pass

• Code is as readable as possible.
• I have reviewed this PR myself, ensuring it meets each checklist item

---

This change is 

[jinliu9508]

Code looks good and the test works!

Should we consider removing the duplicates since they are most likely unnecessary in the event that a duplicate scenario occurs?

[nan-li]

Should we consider removing the duplicates since they are most likely unnecessary in the event that a duplicate scenario occurs?

Can you explain what you mean? This PR prevents a duplicate operation from being added. Are you talking about the model store's remove method?

[jinliu9508]

Should we consider removing the duplicates since they are most likely unnecessary in the event that a duplicate scenario occurs?

Can you explain what you mean? This PR prevents a duplicate operation from being added. Are you talking about the model store's remove method?

Sure. If the user already has duplicate operations in the cache from migrating from 5.1.12, the saved operations will not be added to the model store but will remain in the cache. I am thinking we can add an extra step to remove the duplicates if they are detected and ensure a cleaner migration.

[nan-li]

Sure. If the user already has duplicate operations in the cache from migrating from 5.1.12, the saved operations will not be added to the model store but will remain in the cache. I am thinking we can add an extra step to remove the duplicates if they are detected and ensure a cleaner migration.

I see, I don't think another check or removal from cache is necessary or fixes the issue because of the following reasons:

• Anytime persist() is called, the cache will be overwritten with the current models. And the current models we now prevent adding duplicates to, so models will not have duplicates anymore.
• In normal circumstances, you can't tell when an operation is actually a duplicate from the past. From the example above, 2 identical login-user operations were added to the operation model store and cached. The first one should be sent very quickly and removed once the request is successful, leaving one more operation in the store and cache.
• On the next cold start, that operation will be read from the cache and put in the store. This is actually a duplicate and we would not know.

[jkasten2]

@nan-li Nice catch here! Didn't think about the case where OperationRepo._operationModelStore would still contain the duplicate.

Another question comes to mind now, do we need to keep the duplicate check in internalEnqueue as well? I am thinking yes, since we still need to ensure it doesn't get put into OperationRepo.queue, where it would be processed during the currently runtime.

• This problem however does point to a duplicate state complexity we have in our system. Something that would be nice to clean up in the future, if possible.

Lastly, could we add a test before merging this in?

[jkasten2]

Thinking about this again the root of the problem is ModelStore.load doesn't do what you would think it does. It should take exactly what is on disk and use that for it state (AKA model list), instead it is appending it which I think is the root of the problem.

[nan-li]

Another question comes to mind now, do we need to keep the duplicate check in internalEnqueue as well?

Yes we still need the check in the Operation Repo. The operations are already enqueued to the Op Repo before load() is called.

Thinking about this again the root of the problem is ModelStore.load doesn't do what you would think it does. It should take exactly what is on disk and use that for it state (AKA model list), instead it is appending it which I think is the root of the problem.

This is a good point, I agree. I checked that the cache should always be updated and correct, so it is safe to do a full replacement. I updated the PR.

[jkasten2]

LGTM! Much simpler logic now!

[nan-li]

Thinking again about the model store reading from cache, I realized that it will almost always lose old cached operations...

1. There are some cached operations
2. If there is a onesignal ID, on next cold start, refresh-user enqueues and persists. This will overwrite the cache.
3. Then load is called, loading only the refresh-user operation.

[jkasten2]

I see, so this isn't just a problem with ModelStore.load(). We need to do something with OperationRepo so it doesn't overwrite the local data.

[nan-li]

Testing again, I think the refresh-user operation comes after Operation Repo loads operations, so the situations of overwriting cache are rare.

I have only been able to reproduce with the creation of the initial anonymous user, as those operations enqueue really early in initWithContext.
Or, by having a delay to the coroutine that loads operations.

[jkasten2]

This should be in the synchronized(models) {} below to prevent race conditions and concurrent modification expectations.