Update error handling in Onyx to prevent unnecessary data eviction #694

VickyStash · 2025-10-28T12:50:28Z

Details

This PR updates error handling by:

adding operation retries limitation. Before, failed operations could retry endlessly, but with this PR, the number of retries is limited to 5.
preventing data eviction if error is not related to the storage capacity.

Related Issues

Expensify/App#73779

Automated Tests

Added to tests/unit/onyxUtilsTest.ts

Manual Tests

Launch the iOS/Android app and open the logs.
Send an expense
See the errors about some failing operations. But since the errors are not related to storage capacity, the data isn't evicted from the storage.

Before

After

Author Checklist

Screenshots/Videos

Android: Native

Android: mWeb Chrome

iOS: Native

iOS: mWeb Safari

MacOS: Chrome / Safari

MacOS: Desktop

…age related

lib/OnyxUtils.ts

# Conflicts: # lib/OnyxUtils.ts

lib/OnyxUtils.ts

chrispader

This looks good to me, i just have a few minor comments 🙌🏼

lib/OnyxUtils.ts

chrispader · 2025-10-29T20:43:49Z

lib/OnyxUtils.ts

+ * - Other errors: retries the operation
 */
-function evictStorageAndRetry<TMethod extends typeof Onyx.set | typeof Onyx.multiSet | typeof Onyx.mergeCollection | typeof Onyx.setCollection>(
+function retryOperation<TMethod extends typeof Onyx.set | typeof Onyx.multiSet | typeof Onyx.mergeCollection | typeof Onyx.setCollection>(


NAB: Do you think it would be feasible to add a type backing TMethod that requires an onyx method to have an optional retryAttempt parameter on the last position? 👀

chrispader · 2025-10-29T20:44:47Z

lib/OnyxUtils.ts

+    const isStorageCapacityError = STORAGE_ERRORS.some((storageError) => storageError === error?.name?.toLowerCase() || errorMessage?.includes(storageError));
+
+    if (!isStorageCapacityError) {
+        // @ts-expect-error No overload matches this call.


My above comment was to prevent the need for this // @ts-expect-error comment

This // @ts-expect-error comment isn't coming due to retryAttempt parameter, it something that exists on main as well, just cause onyx methods are different and can't be overloaded.

react-native-onyx/lib/OnyxUtils.ts

Lines 898 to 899 in 6a24680

// @ts-expect-error No overload matches this call.

return remove(keyForRemoval).then(() => onyxMethod(...args));

I've tried to go with different approaches (add retryOperation overloads, adding assertion worked by didn' look much better)

lib/OnyxUtils.ts

…tils

VickyStash · 2025-10-30T16:43:32Z

lib/OnyxUtils.ts

+    const updatePromise = OnyxUtils.broadcastUpdate(key, valueWithoutNestedNullValues, hasChanged);
+
+    // If the value has not changed or the key got removed, calling Storage.setItem() would be redundant and a waste of performance, so return early instead.
+    if (!hasChanged && !retryAttempt) {


I've updated the condition to not skip Storage.setItem operation if the retryAttempt is defined.
I haven't added any cache-clearing logic, as I'm not sure about it, since:

currently app doesn't rollback the cache in case of a failed operation (for any operation)

the subscribers (useOnyx for example) are also notified with the updated values no matter if storage operation succeed or not

I think it makes sense. Could we change the comment above to reflect the new logic?

fabioh8010

Minor things!

@VickyStash is possible to add some unit tests to cover these retry mechanisms?

lib/OnyxUtils.ts

fabioh8010 · 2025-10-30T17:50:17Z

lib/OnyxUtils.ts

+    const updatePromise = OnyxUtils.broadcastUpdate(key, valueWithoutNestedNullValues, hasChanged);
+
+    // If the value has not changed or the key got removed, calling Storage.setItem() would be redundant and a waste of performance, so return early instead.
+    if (!hasChanged && !retryAttempt) {


I think it makes sense. Could we change the comment above to reflect the new logic?

VickyStash · 2025-10-30T18:05:26Z

@VickyStash is possible to add some unit tests to cover these retry mechanisms?

Sure, it's planned! I've mentioned it here.

VickyStash · 2025-10-31T11:04:22Z

While writing the tests, I've found some flaws in the eviction mechanism.

We get keyForEviction by checking recentlyAccessedKeys:

react-native-onyx/lib/OnyxCache.ts

Lines 419 to 429 in 6a24680

    
               /** 
        
                * Finds a key that can be safely evicted 
        
                */ 
        
               getKeyForEviction(): OnyxKey | undefined { 
        
                   for (const key of this.recentlyAccessedKeys) { 
        
                       if (!this.evictionBlocklist[key]) { 
        
                           return key; 
        
                       } 
        
                   } 
        
                   return undefined; 
        
               }

In the recentlyAccessedKeys the key can be related to the data that was removed.

So if we remove value:

react-native-onyx/lib/OnyxUtils.ts

Line 899 in 6a24680

return remove(keyForRemoval).then(() => onyxMethod(...args));

We notify subscribers about that => the key is added to the recentlyAccessedKeys.

It means:

We can end up trying to evict the key that already has no data inside.
On retries, we always try to remove the key that was just removed before, cause it's the latest one in the recentlyAccessedKeys

fabioh8010

🚀

tgolen

@VickyStash Did you do anything about those eviction problems you found while writing the tests? Is that something we need to fix here in this PR or is that going to be something we look at separately?

tgolen · 2025-10-31T15:12:50Z

API-INTERNAL.md

-<dd><p>If we fail to set or merge we must handle this by
-evicting some data from Onyx and then retrying to do
-whatever it is we attempted to do.</p>
+<dt><a href="#retryOperation">retryOperation()</a></dt>


Did you regenerate these docs after your recent changes? I don't think retryOperation() is exposed anywhere publicly, right?

@tgolen I've re-generated it one more time.
Yeah, it's not public! This file is also API-INTERNAL (describes internal methods)

VickyStash · 2025-10-31T15:56:56Z

@VickyStash Did you do anything about those eviction problems you found while writing the tests? Is that something we need to fix here in this PR or is that going to be something we look at separately?

I haven't added any changes for it.

I've the idea of the fix in mind: updating this check to check not only for null, but for undefined as well.

react-native-onyx/lib/OnyxUtils.ts

Lines 692 to 697 in 6a24680

    
           // Add or remove this key from the recentlyAccessedKeys lists 
        
           if (value !== null) { 
        
               cache.addLastAccessedKey(key, isCollectionKey(key)); 
        
           } else { 
        
               cache.removeLastAccessedKey(key); 
        
           }

This way we at least won't try to evict what we know is removed.
But I'm not sure if I should complicate this PR even more. Eager to hear what you think!

tgolen · 2025-10-31T16:18:26Z

I think as long as that issue doesn't prevent this PR from working, then I think we should tackle it separately. I think the best step would be to make a proposal in the #quality channel in Slack.

lib/OnyxUtils.ts

abzokhattab · 2025-10-31T20:00:52Z

lib/OnyxUtils.ts

+        const updatePromise = OnyxUtils.scheduleNotifyCollectionSubscribers(collectionKey, mutableCollection, previousCollection);
+
+        return Storage.multiSet(keyValuePairs)
+            .catch((error) => OnyxUtils.retryOperation(error, setCollectionWithRetry, {collectionKey, collection}, retryAttempt))


same as #694 (comment)

@abzokhattab Onyx.setCollection/setCollectionWithRetry doesn't have any options param, right?
So there is nothing extra to pass.

chrispader · 2025-11-03T10:39:21Z

lib/types.ts

+    isProcessingCollectionUpdate?: boolean;
+};
+
+type OnyxRetryOperation =


NAB: Minor request, but OnyxRetryOperation sounds to me as if these operations would mainly retry and if that was their main purpose. I would name this type something like RetriableOperation or RetriableOnyxOperation

chrispader · 2025-11-03T10:41:33Z

tests/unit/onyxUtilsTest.ts

+
+    describe('retryOperation', () => {
+        it('should retry only one time if the operation is firstly failed and then passed', async () => {
+            const retryOperationSpy = jest.spyOn(OnyxUtils, 'retryOperation');


Can we move this line out of the individual tests and store globally?

chrispader · 2025-11-03T10:41:46Z

tests/unit/onyxUtilsTest.ts

+    describe('retryOperation', () => {
+        it('should retry only one time if the operation is firstly failed and then passed', async () => {
+            const retryOperationSpy = jest.spyOn(OnyxUtils, 'retryOperation');
+            const genericError = new Error('Generic storage error');


Same for this actually

chrispader · 2025-11-03T10:42:10Z

tests/unit/onyxUtilsTest.ts

+
+        it('should stop retrying after MAX_STORAGE_OPERATION_RETRY_ATTEMPTS retries for failing operation', async () => {
+            const retryOperationSpy = jest.spyOn(OnyxUtils, 'retryOperation');
+            const genericError = new Error('Generic storage error');


chrispader · 2025-11-03T10:42:22Z

tests/unit/onyxUtilsTest.ts

+
+        it('should not retry in case of storage capacity error and no keys to evict', async () => {
+            const retryOperationSpy = jest.spyOn(OnyxUtils, 'retryOperation');
+            const quotaError = new Error('out of memory');


And here :)

chrispader

LGTM, i have minor requests but these are not blockers. Thanks for your work @VickyStash 🙌🏼

Update evictStorageAndRetry to not evict the data if error isn't stor…

df729c3

…age related

melvin-bot bot assigned VickyStash Oct 28, 2025

Minor improvement and test fix

6fc70ed

fabioh8010 reviewed Oct 28, 2025

View reviewed changes

lib/OnyxUtils.ts Outdated Show resolved Hide resolved

Applied feedback

c4d29dc

fabioh8010 reviewed Oct 28, 2025

View reviewed changes

lib/OnyxUtils.ts Show resolved Hide resolved

tgolen requested changes Oct 28, 2025

View reviewed changes

lib/OnyxUtils.ts Outdated Show resolved Hide resolved

lib/OnyxUtils.ts Outdated Show resolved Hide resolved

lib/OnyxUtils.ts Show resolved Hide resolved

VickyStash added 4 commits October 29, 2025 11:57

Add comments

67bd9d4

Add a simple limitation of operations retries

a8fbd85

Merge branch 'main' into VickyStash/bugfix/update-eviction-rules

42f44fb

# Conflicts: # lib/OnyxUtils.ts

re-run checks

3bfb0e5

mountiny reviewed Oct 29, 2025

View reviewed changes

lib/OnyxUtils.ts Outdated Show resolved Hide resolved

tgolen requested changes Oct 29, 2025

View reviewed changes

lib/OnyxUtils.ts Outdated Show resolved Hide resolved

lib/OnyxUtils.ts Show resolved Hide resolved

chrispader suggested changes Oct 29, 2025

View reviewed changes

VickyStash added 7 commits October 30, 2025 12:19

Add internal functions for retries

9db02f2

Update mergeCollectionWithPatches

c146abf

Update partialSetCollection

5a6d696

Rename const

23e996e

Minor clean-up

1bd718a

Rename const

b57b22a

Move setWithRetry, multiSetWithRetry, setCollectionWithRetry to OnyxU…

5e0c4ad

…tils

VickyStash changed the title ~~Update evictStorageAndRetry to not evict the data if error isn't storage related~~ Update error handling in Onyx to prevent unnecessary data eviction Oct 30, 2025

VickyStash commented Oct 30, 2025

View reviewed changes

VickyStash mentioned this pull request Oct 30, 2025

[$250] Update error handling in Onyx to prevent unnecessary data eviction Expensify/App#73779

Open

fabioh8010 reviewed Oct 30, 2025

View reviewed changes

VickyStash added 2 commits October 31, 2025 09:15

Minor code improvements

7f0bf74

Add unit tests

ca326b9

VickyStash marked this pull request as ready for review October 31, 2025 11:07

VickyStash requested a review from a team as a code owner October 31, 2025 11:07

VickyStash requested review from chrispader, fabioh8010, mountiny and tgolen October 31, 2025 11:07

melvin-bot bot requested review from lakchote and removed request for a team, chrispader and fabioh8010 October 31, 2025 11:08

fabioh8010 approved these changes Oct 31, 2025

View reviewed changes

tgolen reviewed Oct 31, 2025

View reviewed changes

Update the docs

c55cc64

abzokhattab suggested changes Oct 31, 2025

View reviewed changes

lib/OnyxUtils.ts Outdated Show resolved Hide resolved

lib/OnyxUtils.ts Outdated Show resolved Hide resolved

abzokhattab reviewed Oct 31, 2025

View reviewed changes

Adjust params passed to the retry function

7da26ae

VickyStash requested a review from abzokhattab November 3, 2025 09:01

chrispader reviewed Nov 3, 2025

View reviewed changes

chrispader approved these changes Nov 3, 2025

View reviewed changes

Apply feedback

f5e8b4b

	// @ts-expect-error No overload matches this call.
	return remove(keyForRemoval).then(() => onyxMethod(...args));

Update error handling in Onyx to prevent unnecessary data eviction #694

Are you sure you want to change the base?

Update error handling in Onyx to prevent unnecessary data eviction #694

Conversation

VickyStash commented Oct 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Details

Related Issues

Automated Tests

Manual Tests

Author Checklist

Screenshots/Videos

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

chrispader left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fabioh8010 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

VickyStash commented Oct 30, 2025

Uh oh!

VickyStash commented Oct 31, 2025

Uh oh!

fabioh8010 left a comment

Choose a reason for hiding this comment

Uh oh!

tgolen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

VickyStash commented Oct 31, 2025

Uh oh!

tgolen commented Oct 31, 2025

Uh oh!

Uh oh!

Uh oh!

abzokhattab Oct 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

VickyStash commented Oct 28, 2025 •

edited

Loading

abzokhattab Oct 31, 2025 •

edited

Loading