From 8f9a5d046b6eba2ce8e2164e84b398c88fabfa9c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?F=C3=A1bio=20Henriques?= Date: Mon, 3 Aug 2026 14:33:09 +0100 Subject: [PATCH 1/2] fix: prevent Onyx.merge from clobbering concurrent Onyx.update writes --- lib/Onyx.ts | 7 +++- tests/unit/onyxTest.ts | 81 ++++++++++++++++++++++++++++++++++++------ 2 files changed, 77 insertions(+), 11 deletions(-) diff --git a/lib/Onyx.ts b/lib/Onyx.ts index 16a0f6ec6..192df3c02 100644 --- a/lib/Onyx.ts +++ b/lib/Onyx.ts @@ -218,12 +218,17 @@ function merge(key: TKey, changes: OnyxMergeInput): } mergeQueue[key] = [changes]; - mergeQueuePromise[key] = OnyxUtils.get(key).then((existingValue) => { + mergeQueuePromise[key] = OnyxUtils.get(key).then((valueFromGet) => { // Calls to Onyx.set after a merge will terminate the current merge process and clear the merge queue if (mergeQueue[key] == null) { return Promise.resolve(); } + // Other writers (notably Onyx.update's mergeCollection path, which doesn't participate in mergeQueue) + // can land between get() resolving and this callback running. Applying the delta on top of the value + // captured back then and broadcasting it would overwrite those writes wholesale, so re-read the cache. + const existingValue = cache.hasCacheForKey(key) ? (cache.get(key) as OnyxInput | undefined) : valueFromGet; + try { const validChanges = mergeQueue[key].filter((change) => { const {isCompatible, existingValueType, newValueType, isEmptyArrayCoercion} = utils.checkCompatibilityWithExistingValue(change, existingValue); diff --git a/tests/unit/onyxTest.ts b/tests/unit/onyxTest.ts index 2ea8c6c6c..cec06e660 100644 --- a/tests/unit/onyxTest.ts +++ b/tests/unit/onyxTest.ts @@ -1,17 +1,20 @@ +import {act} from '@testing-library/react-native'; + import lodashClone from 'lodash/clone'; import lodashCloneDeep from 'lodash/cloneDeep'; -import {act} from '@testing-library/react-native'; -import Onyx from '../../lib'; -import * as Logger from '../../lib/Logger'; -import waitForPromisesToResolve from '../utils/waitForPromisesToResolve'; -import OnyxUtils from '../../lib/OnyxUtils'; + import type OnyxCache from '../../lib/OnyxCache'; -import StorageMock from '../../lib/storage'; +import type {Connection} from '../../lib/OnyxConnectionManager'; import type {OnyxCollection, OnyxKey, OnyxUpdate} from '../../lib/types'; import type {GenericDeepRecord} from '../types'; import type GenericCollection from '../utils/GenericCollection'; -import type {Connection} from '../../lib/OnyxConnectionManager'; + +import Onyx from '../../lib'; import createDeferredTask from '../../lib/createDeferredTask'; +import * as Logger from '../../lib/Logger'; +import OnyxUtils from '../../lib/OnyxUtils'; +import StorageMock from '../../lib/storage'; +import waitForPromisesToResolve from '../utils/waitForPromisesToResolve'; const ONYX_KEYS = { TEST_KEY: 'test', @@ -2629,6 +2632,59 @@ describe('Onyx', () => { expect(cache.get(collectionMemberKey)).toEqual({data: 'test'}); expect(await StorageMock.getItem(collectionMemberKey)).toBeNull(); }); + + describe('concurrency with Onyx.update', () => { + afterEach(() => { + jest.restoreAllMocks(); + }); + + // Onyx.merge reads the key, computes the whole merged value and writes it back with cache.set(), which is + // a full replace. Onyx.update's mergeCollection path writes the same key with cache.merge() and never + // registers in mergeQueue, so the two don't serialize. If merge() applies its delta to the value get() + // resolved with rather than to the current one, everything the update wrote meanwhile is silently dropped. + it('should the delta on top of an Onyx.update that landed after get() is resolved', async () => { + const member1 = `${ONYX_KEYS.COLLECTION.TEST_KEY}1`; + const member2 = `${ONYX_KEYS.COLLECTION.TEST_KEY}2`; + + await Onyx.merge(member1, {itemA: {pendingAction: 'add'}, itemB: {name: 'b'}}); + await Onyx.merge(member2, {itemC: {name: 'c'}}); + await waitForPromisesToResolve(); + + // The value merge()'s get() would have resolved with, before the update below runs. + const staleValue = lodashCloneDeep(cache.get(member1)); + + // Park merge()'s read so the update is guaranteed to land first. Doing this explicitly keeps the test + // deterministic instead of relying on the two operations interleaving by luck. + const deferredGet = createDeferredTask(); + const originalGet = OnyxUtils.get; + jest.spyOn(OnyxUtils, 'get').mockImplementation(((key: OnyxKey) => + key === member1 ? deferredGet.promise.then(() => staleValue) : originalGet(key)) as typeof OnyxUtils.get); + + const mergePromise = Onyx.merge(member1, {itemA: {childID: '1'}}); + + // Two keys of the same collection, so Onyx.update batches them into mergeCollectionWithPatches. + await Onyx.update([ + {onyxMethod: Onyx.METHOD.MERGE, key: member1, value: {itemA: {pendingAction: null}, itemB: null}}, + {onyxMethod: Onyx.METHOD.MERGE, key: member2, value: {itemC: {touched: true}}}, + ]); + await waitForPromisesToResolve(); + + // Release the read: merge() now applies its delta with a value that is already out of date. + deferredGet.resolve(); + await mergePromise; + await waitForPromisesToResolve(); + + const value = cache.get(member1) as GenericDeepRecord; + expect(value).toEqual({itemA: {childID: '1'}}); + // Spelled out so a failure says which half of the update was lost. + expect(value.itemA).not.toHaveProperty('pendingAction'); + expect(value).not.toHaveProperty('itemB'); + expect(value.itemA).toHaveProperty('childID', '1'); + + // Storage must agree with the cache, otherwise the divergence resurfaces on the next app start. + expect(await StorageMock.getItem(member1)).toEqual({itemA: {childID: '1'}}); + }); + }); }); describe('set', () => { @@ -3360,6 +3416,12 @@ describe('RAM-only keys should not read from storage', () => { describe('get() should prefer cache over stale storage', () => { let cache: typeof OnyxCache; + // StorageMock.getItem is a plain jest.fn(), not a spy, so jest.restoreAllMocks() will not undo a + // mockImplementation() set on it. Capture the default now and put it back after each test, otherwise the + // override below leaks into every suite that runs afterwards. + const getItemMock = StorageMock.getItem as jest.Mock; + const defaultGetItem = getItemMock.getMockImplementation() as (key: OnyxKey) => Promise; + beforeEach(() => { Object.assign(OnyxUtils.getDeferredInitTask(), createDeferredTask()); cache = require('../../lib/OnyxCache').default; @@ -3367,6 +3429,7 @@ describe('get() should prefer cache over stale storage', () => { }); afterEach(() => { + getItemMock.mockImplementation(defaultGetItem); jest.restoreAllMocks(); return Onyx.clear(); }); @@ -3376,15 +3439,13 @@ describe('get() should prefer cache over stale storage', () => { const member2 = `${ONYX_KEYS.COLLECTION.TEST_KEY}2`; // Delay getItem for member1 to simulate slow Native storage (returns null before the write lands) - const getItemMock = StorageMock.getItem as jest.Mock; - const originalGetItem = getItemMock.getMockImplementation()!; getItemMock.mockImplementation((key: OnyxKey) => { if (key === member1) { return new Promise((resolve) => { setTimeout(() => resolve(undefined), 50); }); } - return originalGetItem(key); + return defaultGetItem(key); }); // 2+ collection keys get batched into mergeCollectionWithPatches (deferred cache write) From cfa94f132d44976e2bbfa5e810477947ae4852e0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?F=C3=A1bio=20Henriques?= Date: Wed, 5 Aug 2026 14:54:36 +0100 Subject: [PATCH 2/2] Address review --- tests/unit/onyxTest.ts | 22 ++++------------------ 1 file changed, 4 insertions(+), 18 deletions(-) diff --git a/tests/unit/onyxTest.ts b/tests/unit/onyxTest.ts index b2536337b..938a64038 100644 --- a/tests/unit/onyxTest.ts +++ b/tests/unit/onyxTest.ts @@ -2638,11 +2638,7 @@ describe('Onyx', () => { jest.restoreAllMocks(); }); - // Onyx.merge reads the key, computes the whole merged value and writes it back with cache.set(), which is - // a full replace. Onyx.update's mergeCollection path writes the same key with cache.merge() and never - // registers in mergeQueue, so the two don't serialize. If merge() applies its delta to the value get() - // resolved with rather than to the current one, everything the update wrote meanwhile is silently dropped. - it('should the delta on top of an Onyx.update that landed after get() is resolved', async () => { + it('should apply the delta on top of an Onyx.update that landed after get() is resolved', async () => { const member1 = `${ONYX_KEYS.COLLECTION.TEST_KEY}1`; const member2 = `${ONYX_KEYS.COLLECTION.TEST_KEY}2`; @@ -2650,11 +2646,9 @@ describe('Onyx', () => { await Onyx.merge(member2, {itemC: {name: 'c'}}); await waitForPromisesToResolve(); - // The value merge()'s get() would have resolved with, before the update below runs. const staleValue = lodashCloneDeep(cache.get(member1)); - // Park merge()'s read so the update is guaranteed to land first. Doing this explicitly keeps the test - // deterministic instead of relying on the two operations interleaving by luck. + // Park merge()'s read so the update below is guaranteed to land first. const deferredGet = createDeferredTask(); const originalGet = OnyxUtils.get; jest.spyOn(OnyxUtils, 'get').mockImplementation(((key: OnyxKey) => @@ -2662,26 +2656,18 @@ describe('Onyx', () => { const mergePromise = Onyx.merge(member1, {itemA: {childID: '1'}}); - // Two keys of the same collection, so Onyx.update batches them into mergeCollectionWithPatches. + // Two keys of the same collection, so this goes through mergeCollectionWithPatches. await Onyx.update([ {onyxMethod: Onyx.METHOD.MERGE, key: member1, value: {itemA: {pendingAction: null}, itemB: null}}, {onyxMethod: Onyx.METHOD.MERGE, key: member2, value: {itemC: {touched: true}}}, ]); await waitForPromisesToResolve(); - // Release the read: merge() now applies its delta with a value that is already out of date. deferredGet.resolve(); await mergePromise; await waitForPromisesToResolve(); - const value = cache.get(member1) as GenericDeepRecord; - expect(value).toEqual({itemA: {childID: '1'}}); - // Spelled out so a failure says which half of the update was lost. - expect(value.itemA).not.toHaveProperty('pendingAction'); - expect(value).not.toHaveProperty('itemB'); - expect(value.itemA).toHaveProperty('childID', '1'); - - // Storage must agree with the cache, otherwise the divergence resurfaces on the next app start. + expect(cache.get(member1)).toEqual({itemA: {childID: '1'}}); expect(await StorageMock.getItem(member1)).toEqual({itemA: {childID: '1'}}); }); });