From 42b796d64f92b8122c1e545adb6aac587df7a787 Mon Sep 17 00:00:00 2001 From: eliran goshen Date: Tue, 19 May 2026 16:23:00 +0200 Subject: [PATCH 1/2] Make mergeCollectionWithPatches cache-first Update cache and notify subscribers synchronously before issuing storage writes, matching the cache-first / storage-second invariant followed by every other Onyx write method. Subscribers now still reflect the merged data when the underlying storage write fails (e.g. IDB corruption). `get()` is invoked on existingKeys first to pre-warm cache from storage for cache-miss keys; it is a no-op (sync-resolved) for cache hits. This preserves the previous cold-cache merge semantics while removing the prior race between the cache update and the storage write. Adds a regression test asserting cache + subscribers reflect the merged collection when Storage.multiMerge rejects. Fixes Expensify/App#90634 --- lib/OnyxUtils.ts | 75 ++++++++++++++++++++----------------- tests/unit/onyxUtilsTest.ts | 43 +++++++++++++++++++++ 2 files changed, 83 insertions(+), 35 deletions(-) diff --git a/lib/OnyxUtils.ts b/lib/OnyxUtils.ts index cd63e7d6b..1193dc311 100644 --- a/lib/OnyxUtils.ts +++ b/lib/OnyxUtils.ts @@ -1579,47 +1579,52 @@ function mergeCollectionWithPatches( // because we will simply overwrite the existing values in storage. const keyValuePairsForNewCollection = prepareKeyValuePairsForStorage(newCollection, true); - const promises = []; - - // We need to get the previously existing values so we can compare the new ones - // against them, to avoid unnecessary subscriber updates. - const previousCollectionPromise = Promise.all(existingKeys.map((key) => get(key).then((value) => [key, value]))).then(Object.fromEntries); - - // New keys will be added via multiSet while existing keys will be updated using multiMerge - // This is because setting a key that doesn't exist yet with multiMerge will throw errors - // We can skip this step for RAM-only keys as they should never be saved to storage - if (!OnyxKeys.isRamOnlyKey(collectionKey) && keyValuePairsForExistingCollection.length > 0) { - promises.push(Storage.multiMerge(keyValuePairsForExistingCollection)); - } - - // We can skip this step for RAM-only keys as they should never be saved to storage - if (!OnyxKeys.isRamOnlyKey(collectionKey) && keyValuePairsForNewCollection.length > 0) { - promises.push(Storage.multiSet(keyValuePairsForNewCollection)); - } - // finalMergedCollection contains all the keys that were merged, without the keys of incompatible updates const finalMergedCollection = {...existingKeyCollection, ...newCollection}; - // Prefill cache if necessary by calling get() on any existing keys and then merge original data to cache - // and update all subscribers - const promiseUpdate = previousCollectionPromise.then((previousCollection) => { + // Pre-warm cache for any existing storage keys that aren't yet in cache. get() is a no-op + // (sync-resolved) for cache hits, and on a cache miss it reads from storage and writes the + // value back to cache. This is required so the subsequent cache.merge() merges the new delta + // into the real previous storage value (rather than starting from `undefined` and dropping + // the existing keys). + return Promise.all(existingKeys.map((key) => get(key))).then(() => { + // Snapshot previous values from the (now-warm) cache for keysChanged's diff, then update + // cache and notify subscribers synchronously BEFORE issuing storage writes. This matches + // the cache-first / storage-second invariant followed by every other Onyx write method + // (setWithRetry, applyMerge, setCollectionWithRetry, partialSetCollection, clear), + // ensuring subscribers still reflect the merged data even if the subsequent storage + // write fails. + const previousCollection = getCachedCollection(collectionKey, existingKeys); cache.merge(finalMergedCollection); keysChanged(collectionKey, finalMergedCollection, previousCollection); - }); - return Promise.all(promises) - .catch((error) => - retryOperation( - error, - mergeCollectionWithPatches, - {collectionKey, collection: resultCollection as OnyxMergeCollectionInput, mergeReplaceNullPatches, isProcessingCollectionUpdate}, - retryAttempt, - ), - ) - .then(() => { - sendActionToDevTools(METHOD.MERGE_COLLECTION, undefined, resultCollection); - return promiseUpdate; - }); + const promises = []; + + // New keys will be added via multiSet while existing keys will be updated using multiMerge + // This is because setting a key that doesn't exist yet with multiMerge will throw errors + // We can skip this step for RAM-only keys as they should never be saved to storage + if (!OnyxKeys.isRamOnlyKey(collectionKey) && keyValuePairsForExistingCollection.length > 0) { + promises.push(Storage.multiMerge(keyValuePairsForExistingCollection)); + } + + // We can skip this step for RAM-only keys as they should never be saved to storage + if (!OnyxKeys.isRamOnlyKey(collectionKey) && keyValuePairsForNewCollection.length > 0) { + promises.push(Storage.multiSet(keyValuePairsForNewCollection)); + } + + return Promise.all(promises) + .catch((error) => + retryOperation( + error, + mergeCollectionWithPatches, + {collectionKey, collection: resultCollection as OnyxMergeCollectionInput, mergeReplaceNullPatches, isProcessingCollectionUpdate}, + retryAttempt, + ), + ) + .then(() => { + sendActionToDevTools(METHOD.MERGE_COLLECTION, undefined, resultCollection); + }); + }); }) .then(() => undefined); } diff --git a/tests/unit/onyxUtilsTest.ts b/tests/unit/onyxUtilsTest.ts index 2bdb9169e..cb5788632 100644 --- a/tests/unit/onyxUtilsTest.ts +++ b/tests/unit/onyxUtilsTest.ts @@ -804,6 +804,49 @@ describe('OnyxUtils', () => { }); }); + describe('mergeCollection cache-first ordering', () => { + it('updates cache and notifies subscribers even when Storage.multiMerge rejects', async () => { + const collectionKey = ONYXKEYS.COLLECTION.TEST_KEY; + const existingMemberKey = `${collectionKey}1`; + const newMemberKey = `${collectionKey}2`; + + // Seed an existing member so the merge path exercises multiMerge (existing) + multiSet (new) + await Onyx.set(existingMemberKey, {value: 'initial'}); + + const collectionCallback = jest.fn(); + Onyx.connect({ + key: collectionKey, + waitForCollectionCallback: true, + callback: collectionCallback, + }); + await waitForPromisesToResolve(); + collectionCallback.mockClear(); + + // Force Storage.multiMerge to reject with a non-retriable IDB error so the failure + // path is taken without burning the full retry budget and without rejecting the + // outer Onyx.mergeCollection promise. + const nonRetriableIdbError = Object.assign(new Error('Internal error opening backing store for indexedDB.open.'), {name: 'UnknownError'}); + StorageMock.multiMerge = jest.fn().mockRejectedValue(nonRetriableIdbError); + + await Onyx.mergeCollection(collectionKey, { + [existingMemberKey]: {value: 'merged'}, + [newMemberKey]: {value: 'new'}, + } as GenericCollection); + + // Cache must reflect the merge regardless of the multiMerge rejection. This is the + // cache-first / storage-second invariant that mergeCollectionWithPatches must honor. + const cachedCollection = OnyxCache.getCollectionData(collectionKey); + expect(cachedCollection?.[existingMemberKey]).toEqual({value: 'merged'}); + expect(cachedCollection?.[newMemberKey]).toEqual({value: 'new'}); + + // Subscribers must have been notified with the merged values. + expect(collectionCallback).toHaveBeenCalled(); + const lastBroadcast = collectionCallback.mock.calls.at(-1)?.[0] as Record | undefined; + expect(lastBroadcast?.[existingMemberKey]).toEqual({value: 'merged'}); + expect(lastBroadcast?.[newMemberKey]).toEqual({value: 'new'}); + }); + }); + describe('storage eviction', () => { const diskFullError = new Error('database or disk is full'); From 9445bbf295aa19eb1f64c17eb98efeb9bda9fcb4 Mon Sep 17 00:00:00 2001 From: eliran goshen Date: Thu, 21 May 2026 10:12:01 +0200 Subject: [PATCH 2/2] Add cache-first regression test for Storage.multiSet rejection Adds a parallel test inside the 'mergeCollection cache-first ordering' describe asserting that cache + subscribers reflect the merged data even when Storage.multiSet (the new-keys path) rejects. The existing test only exercises the Storage.multiMerge (existing-keys) path. Also adds an afterEach that restores StorageMock.multiMerge and StorageMock.multiSet to their originals after each test in this block, so the rejecting mocks no longer leak into later describe blocks (eviction, afterInit) whose setup relies on these storage methods working normally. Addresses review feedback on Expensify/react-native-onyx#787. --- tests/unit/onyxUtilsTest.ts | 52 +++++++++++++++++++++++++++++++++++++ 1 file changed, 52 insertions(+) diff --git a/tests/unit/onyxUtilsTest.ts b/tests/unit/onyxUtilsTest.ts index cb5788632..33857768d 100644 --- a/tests/unit/onyxUtilsTest.ts +++ b/tests/unit/onyxUtilsTest.ts @@ -805,6 +805,18 @@ describe('OnyxUtils', () => { }); describe('mergeCollection cache-first ordering', () => { + // Save originals so we can restore them after each test. The tests below replace + // StorageMock.multiMerge / StorageMock.multiSet with rejecting mocks; without + // restoring, the mock leaks into later describe blocks (e.g. eviction tests) whose + // setup relies on these storage methods working normally. + const originalMultiMerge = StorageMock.multiMerge; + const originalMultiSet = StorageMock.multiSet; + + afterEach(() => { + StorageMock.multiMerge = originalMultiMerge; + StorageMock.multiSet = originalMultiSet; + }); + it('updates cache and notifies subscribers even when Storage.multiMerge rejects', async () => { const collectionKey = ONYXKEYS.COLLECTION.TEST_KEY; const existingMemberKey = `${collectionKey}1`; @@ -845,6 +857,46 @@ describe('OnyxUtils', () => { expect(lastBroadcast?.[existingMemberKey]).toEqual({value: 'merged'}); expect(lastBroadcast?.[newMemberKey]).toEqual({value: 'new'}); }); + + it('updates cache and notifies subscribers even when Storage.multiSet rejects', async () => { + const collectionKey = ONYXKEYS.COLLECTION.TEST_KEY; + const newMemberKey1 = `${collectionKey}1`; + const newMemberKey2 = `${collectionKey}2`; + + // No keys are seeded, so every merged key is a "new" key. This forces the merge path + // to use Storage.multiSet (existing keys would go through Storage.multiMerge). + const collectionCallback = jest.fn(); + Onyx.connect({ + key: collectionKey, + waitForCollectionCallback: true, + callback: collectionCallback, + }); + await waitForPromisesToResolve(); + collectionCallback.mockClear(); + + // Force Storage.multiSet to reject with a non-retriable IDB error so the failure + // path is taken without burning the full retry budget and without rejecting the + // outer Onyx.mergeCollection promise. + const nonRetriableIdbError = Object.assign(new Error('Internal error opening backing store for indexedDB.open.'), {name: 'UnknownError'}); + StorageMock.multiSet = jest.fn().mockRejectedValue(nonRetriableIdbError); + + await Onyx.mergeCollection(collectionKey, { + [newMemberKey1]: {value: 'first'}, + [newMemberKey2]: {value: 'second'}, + } as GenericCollection); + + // Cache must reflect the merge regardless of the multiSet rejection. This is the + // cache-first / storage-second invariant that mergeCollectionWithPatches must honor. + const cachedCollection = OnyxCache.getCollectionData(collectionKey); + expect(cachedCollection?.[newMemberKey1]).toEqual({value: 'first'}); + expect(cachedCollection?.[newMemberKey2]).toEqual({value: 'second'}); + + // Subscribers must have been notified with the merged values. + expect(collectionCallback).toHaveBeenCalled(); + const lastBroadcast = collectionCallback.mock.calls.at(-1)?.[0] as Record | undefined; + expect(lastBroadcast?.[newMemberKey1]).toEqual({value: 'first'}); + expect(lastBroadcast?.[newMemberKey2]).toEqual({value: 'second'}); + }); }); describe('storage eviction', () => {