diff --git a/lib/OnyxUtils.ts b/lib/OnyxUtils.ts index 06e56f8c6..917a3b4a1 100644 --- a/lib/OnyxUtils.ts +++ b/lib/OnyxUtils.ts @@ -769,9 +769,14 @@ function getCollectionDataAndSendAsObject(matchingKeys: Co /** * Remove a key from Onyx and update the subscribers */ -function remove(key: TKey, isProcessingCollectionUpdate?: boolean): Promise { +function remove(key: TKey, isProcessingCollectionUpdate?: boolean, skipNotify?: boolean): Promise { cache.drop(key); - keyChanged(key, undefined as OnyxValue, undefined, isProcessingCollectionUpdate); + // skipNotify is used by retryOperation's eviction branch — the imminent retry's cache.set + // will re-populate cache, so firing keyChanged(undefined) here would only strand subscribers + // in the "removed" state across the retry. + if (!skipNotify) { + keyChanged(key, undefined as OnyxValue, undefined, isProcessingCollectionUpdate); + } if (OnyxKeys.isRamOnlyKey(key)) { return Promise.resolve(); @@ -842,8 +847,10 @@ function retryOperation(error: Error, on Logger.logInfo(`Out of storage. Evicting least recently accessed key (${keyForRemoval}) and retrying. Error: ${error}`); reportStorageQuota(error); + // skipNotify=true: retry's orchestrator skips keysChanged on retryAttempt > 0, so we + // must not let remove() fire keyChanged(undefined) — cache.set on retry restores the value. // @ts-expect-error No overload matches this call. - return remove(keyForRemoval).then(() => onyxMethod(defaultParams, nextRetryAttempt)); + return remove(keyForRemoval, undefined, true).then(() => onyxMethod(defaultParams, nextRetryAttempt)); } /** @@ -1395,14 +1402,21 @@ function multiSetWithRetry(data: OnyxMultiSetInput, retryAttempt?: number): Prom // so re-entrant callbacks (e.g. Onyx.set inside a callback) see consistent cache // and subscriber state, matching the original per-key notification semantics. cache.set(key, value); - keyChanged(key, value); + // Skip subscriber notification on retry — already notified on attempt 0. + // waitForCollectionCallback subscribers re-fire on every keyChanged by contract. + if (!retryAttempt) { + keyChanged(key, value); + } } } // One keysChanged() per collection — fires each collection-level subscriber once and lets // keysChanged() internally decide which individual member subscribers need notification. - for (const [collectionKey, batch] of collectionBatches) { - keysChanged(collectionKey as CollectionKeyBase, batch.partial, batch.previous); + // Skip on retry — already notified on attempt 0 (see same-reason comment above). + if (!retryAttempt) { + for (const [collectionKey, batch] of collectionBatches) { + keysChanged(collectionKey as CollectionKeyBase, batch.partial, batch.previous); + } } const keyValuePairsToStore = keyValuePairsToSet.filter((keyValuePair) => { @@ -1476,7 +1490,11 @@ function setCollectionWithRetry({collectionKey, for (const [key, value] of keyValuePairs) cache.set(key, value); - keysChanged(collectionKey, mutableCollection, previousCollection); + // Skip subscriber notification on retry — already notified on attempt 0. + // waitForCollectionCallback subscribers re-fire on every keysChanged by contract. + if (!retryAttempt) { + keysChanged(collectionKey, mutableCollection, previousCollection); + } // RAM-only keys are not supposed to be saved to storage if (OnyxKeys.isRamOnlyKey(collectionKey)) { @@ -1611,7 +1629,11 @@ function mergeCollectionWithPatches( // write fails. const previousCollection = getCachedCollection(collectionKey, existingKeys); cache.merge(finalMergedCollection); - keysChanged(collectionKey, finalMergedCollection, previousCollection); + // Skip subscriber notification on retry — already notified on attempt 0. + // waitForCollectionCallback subscribers re-fire on every keysChanged by contract. + if (!retryAttempt) { + keysChanged(collectionKey, finalMergedCollection, previousCollection); + } const promises = []; @@ -1690,7 +1712,11 @@ function partialSetCollection({collectionKey, co for (const [key, value] of keyValuePairs) cache.set(key, value); - keysChanged(collectionKey, mutableCollection, previousCollection); + // Skip subscriber notification on retry — already notified on attempt 0. + // waitForCollectionCallback subscribers re-fire on every keysChanged by contract. + if (!retryAttempt) { + keysChanged(collectionKey, mutableCollection, previousCollection); + } if (OnyxKeys.isRamOnlyKey(collectionKey)) { sendActionToDevTools(METHOD.SET_COLLECTION, undefined, mutableCollection); diff --git a/tests/unit/onyxUtilsTest.ts b/tests/unit/onyxUtilsTest.ts index a545275c5..bdf85d134 100644 --- a/tests/unit/onyxUtilsTest.ts +++ b/tests/unit/onyxUtilsTest.ts @@ -920,6 +920,128 @@ describe('OnyxUtils', () => { }); }); + describe('retry side-effect idempotency', () => { + // Save originals so each test can replace StorageMock.multiMerge / StorageMock.multiSet + // with a one-shot rejecting mock that triggers retryOperation's transient-error path. + // Restoring keeps mocks from leaking into the storage-eviction describe block below. + const originalMultiMerge = StorageMock.multiMerge; + const originalMultiSet = StorageMock.multiSet; + + afterEach(() => { + StorageMock.multiMerge = originalMultiMerge; + StorageMock.multiSet = originalMultiSet; + }); + + // A retriable error: not in NON_RETRIABLE_ERRORS, not in STORAGE_ERRORS, so retryOperation + // re-enters the failing method on the next attempt. + const transientError = new Error('Transient storage error'); + + it('mergeCollection — waitForCollectionCallback subscriber fires once across retries', async () => { + const collectionKey = ONYXKEYS.COLLECTION.TEST_KEY; + const existingMemberKey = `${collectionKey}1`; + const newMemberKey = `${collectionKey}2`; + + await Onyx.set(existingMemberKey, {value: 'initial'}); + + const collectionCallback = jest.fn(); + Onyx.connect({ + key: collectionKey, + waitForCollectionCallback: true, + callback: collectionCallback, + }); + await waitForPromisesToResolve(); + collectionCallback.mockClear(); + + StorageMock.multiMerge = jest.fn(originalMultiMerge).mockRejectedValueOnce(transientError); + + await Onyx.mergeCollection(collectionKey, { + [existingMemberKey]: {value: 'merged'}, + [newMemberKey]: {value: 'new'}, + } as GenericCollection); + + // Before this fix, every retry attempt re-fired keysChanged() — and + // waitForCollectionCallback subscribers fire on every keysChanged() call by contract. + // After the fix, retries skip the keysChanged re-fire, so subscribers are notified + // exactly once per logical operation. + expect(collectionCallback).toHaveBeenCalledTimes(1); + }); + + it('Onyx.multiSet — collection subscriber fires once across retries', async () => { + const collectionKey = ONYXKEYS.COLLECTION.TEST_KEY; + const memberKey1 = `${collectionKey}1`; + const memberKey2 = `${collectionKey}2`; + + const collectionCallback = jest.fn(); + Onyx.connect({ + key: collectionKey, + waitForCollectionCallback: true, + callback: collectionCallback, + }); + await waitForPromisesToResolve(); + collectionCallback.mockClear(); + + StorageMock.multiSet = jest.fn(originalMultiSet).mockRejectedValueOnce(transientError); + + await Onyx.multiSet({ + [memberKey1]: {value: 'first'}, + [memberKey2]: {value: 'second'}, + }); + + expect(collectionCallback).toHaveBeenCalledTimes(1); + }); + + it('Onyx.setCollection — collection subscriber fires once across retries', async () => { + const collectionKey = ONYXKEYS.COLLECTION.TEST_KEY; + const memberKey1 = `${collectionKey}1`; + const memberKey2 = `${collectionKey}2`; + + const collectionCallback = jest.fn(); + Onyx.connect({ + key: collectionKey, + waitForCollectionCallback: true, + callback: collectionCallback, + }); + await waitForPromisesToResolve(); + collectionCallback.mockClear(); + + StorageMock.multiSet = jest.fn(originalMultiSet).mockRejectedValueOnce(transientError); + + await Onyx.setCollection(collectionKey, { + [memberKey1]: {value: 'first'}, + [memberKey2]: {value: 'second'}, + } as GenericCollection); + + expect(collectionCallback).toHaveBeenCalledTimes(1); + }); + + it('OnyxUtils.partialSetCollection — collection subscriber fires once across retries', async () => { + const collectionKey = ONYXKEYS.COLLECTION.TEST_KEY; + const memberKey1 = `${collectionKey}1`; + const memberKey2 = `${collectionKey}2`; + + const collectionCallback = jest.fn(); + Onyx.connect({ + key: collectionKey, + waitForCollectionCallback: true, + callback: collectionCallback, + }); + await waitForPromisesToResolve(); + collectionCallback.mockClear(); + + StorageMock.multiSet = jest.fn(originalMultiSet).mockRejectedValueOnce(transientError); + + await OnyxUtils.partialSetCollection({ + collectionKey, + collection: { + [memberKey1]: {value: 'first'}, + [memberKey2]: {value: 'second'}, + } as GenericCollection, + }); + + expect(collectionCallback).toHaveBeenCalledTimes(1); + }); + }); + describe('storage eviction', () => { const diskFullError = new Error('database or disk is full');