From bfd6902e8bfbe3f5874cb39f802026e1d536a6da Mon Sep 17 00:00:00 2001 From: eliran goshen Date: Mon, 25 May 2026 15:30:08 +0200 Subject: [PATCH 1/4] Make retried Onyx writes idempotent on storage retry Follow-up to #787. Splits each retriable collection write into an orchestrator (cache + subscriber notify + storage prep) and a small file-private write helper (Storage.multiSet/multiMerge + retryOperation + devtools log). retryOperation re-enters the write helper, not the orchestrator, which fixes two pre-existing bugs flagged in #787 review: 1. New keys no longer get silently downgraded to multiMerge on retry. The existing/new key split was previously re-derived from getAllKeys against a cache that the first attempt had already mutated, routing brand-new keys through multiMerge instead of multiSet on retry. Benign on IDB but wrong semantics and a crash on backends that require multiSet for missing keys. 2. keysChanged/keyChanged subscribers (notably waitForCollectionCallback ones, which re-fire on every call by contract) are now notified exactly once per logical operation rather than once per retry attempt. Affected paths: mergeCollectionWithPatches, multiSetWithRetry, setCollectionWithRetry, partialSetCollection. setWithRetry was already safe via its hasValueChanged guard. partialSetCollection shares the new persistCollectionWrite helper with setCollectionWithRetry since their storage tails are identical. Adds a 'retry side-effect idempotency' test block covering all four paths. Tests confirmed to fail without the lib changes. Co-Authored-By: Claude Opus 4.7 (1M context) --- lib/OnyxUtils.ts | 134 ++++++++++++++++++++------------ lib/types.ts | 3 + tests/unit/onyxUtilsTest.ts | 150 ++++++++++++++++++++++++++++++++++++ 3 files changed, 237 insertions(+), 50 deletions(-) diff --git a/lib/OnyxUtils.ts b/lib/OnyxUtils.ts index 06e56f8c6..916d88501 100644 --- a/lib/OnyxUtils.ts +++ b/lib/OnyxUtils.ts @@ -1333,6 +1333,21 @@ function setWithRetry({key, value, options}: SetParams { + const {keyValuePairsToStore, newData} = params; + + return Storage.multiSet(keyValuePairsToStore) + .catch((error) => OnyxUtils.retryOperation(error, persistMultiSetWrite, params, retryAttempt)) + .then(() => { + OnyxUtils.sendActionToDevTools(OnyxUtils.METHOD.MULTI_SET, undefined, newData); + }); +} + /** * Sets multiple keys and values. * Serves as core implementation for `Onyx.multiSet()` public function, the difference being @@ -1411,10 +1426,34 @@ function multiSetWithRetry(data: OnyxMultiSetInput, retryAttempt?: number): Prom return !OnyxKeys.isRamOnlyKey(key); }); - return Storage.multiSet(keyValuePairsToStore) - .catch((error) => OnyxUtils.retryOperation(error, multiSetWithRetry, newData, retryAttempt)) + return persistMultiSetWrite({keyValuePairsToStore, newData}, retryAttempt); +} + +/** + * Storage-write tail of setCollectionWithRetry, isolated so that retryOperation re-enters only the + * storage step. Cache and subscriber notifications already happened in the orchestrator, so + * retries no longer re-fire `waitForCollectionCallback` subscribers with the same payload. + */ +function persistCollectionWrite( + params: { + collectionKey: TKey; + keyValuePairs: StorageKeyValuePair[]; + mutableCollection: OnyxInputKeyValueMapping; + }, + retryAttempt?: number, +): Promise { + const {collectionKey, keyValuePairs, mutableCollection} = params; + + // RAM-only keys never persist to storage. + if (OnyxKeys.isRamOnlyKey(collectionKey)) { + OnyxUtils.sendActionToDevTools(OnyxUtils.METHOD.SET_COLLECTION, undefined, mutableCollection); + return Promise.resolve(); + } + + return Storage.multiSet(keyValuePairs) + .catch((error) => OnyxUtils.retryOperation(error, persistCollectionWrite, params, retryAttempt)) .then(() => { - OnyxUtils.sendActionToDevTools(OnyxUtils.METHOD.MULTI_SET, undefined, newData); + OnyxUtils.sendActionToDevTools(OnyxUtils.METHOD.SET_COLLECTION, undefined, mutableCollection); }); } @@ -1478,20 +1517,46 @@ function setCollectionWithRetry({collectionKey, keysChanged(collectionKey, mutableCollection, previousCollection); - // RAM-only keys are not supposed to be saved to storage - if (OnyxKeys.isRamOnlyKey(collectionKey)) { - OnyxUtils.sendActionToDevTools(OnyxUtils.METHOD.SET_COLLECTION, undefined, mutableCollection); - return; - } - - return Storage.multiSet(keyValuePairs) - .catch((error) => OnyxUtils.retryOperation(error, setCollectionWithRetry, {collectionKey, collection}, retryAttempt)) - .then(() => { - OnyxUtils.sendActionToDevTools(OnyxUtils.METHOD.SET_COLLECTION, undefined, mutableCollection); - }); + return persistCollectionWrite({collectionKey, keyValuePairs, mutableCollection}, retryAttempt); }); } +/** + * Storage-write tail of mergeCollectionWithPatches, isolated so that retryOperation re-enters only + * the storage step. Cache and subscriber notifications already happened in the orchestrator, and + * the existing/new key split is captured in `params` — so retries don't re-fire subscribers and + * don't reclassify keys against a cache that was already mutated on the first attempt. + */ +function persistMergedCollectionWrite( + params: { + collectionKey: TKey; + keyValuePairsForExistingCollection: StorageKeyValuePair[]; + keyValuePairsForNewCollection: StorageKeyValuePair[]; + resultCollection: OnyxInputKeyValueMapping; + }, + retryAttempt?: number, +): Promise { + const {collectionKey, keyValuePairsForExistingCollection, keyValuePairsForNewCollection, resultCollection} = params; + + const promises = []; + + // New keys are added via multiSet while existing keys are updated using multiMerge; using + // multiMerge on a key that doesn't exist yet throws on some storage backends. RAM-only keys + // never persist to storage. + if (!OnyxKeys.isRamOnlyKey(collectionKey) && keyValuePairsForExistingCollection.length > 0) { + promises.push(Storage.multiMerge(keyValuePairsForExistingCollection)); + } + if (!OnyxKeys.isRamOnlyKey(collectionKey) && keyValuePairsForNewCollection.length > 0) { + promises.push(Storage.multiSet(keyValuePairsForNewCollection)); + } + + return Promise.all(promises) + .catch((error) => retryOperation(error, persistMergedCollectionWrite, params, retryAttempt)) + .then(() => { + sendActionToDevTools(METHOD.MERGE_COLLECTION, undefined, resultCollection); + }); +} + /** * Merges a collection based on their keys. * Serves as core implementation for `Onyx.mergeCollection()` public function, the difference being @@ -1613,32 +1678,7 @@ function mergeCollectionWithPatches( cache.merge(finalMergedCollection); keysChanged(collectionKey, finalMergedCollection, previousCollection); - 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); - }); + return persistMergedCollectionWrite({collectionKey, keyValuePairsForExistingCollection, keyValuePairsForNewCollection, resultCollection}, retryAttempt); }); }) .then(() => undefined); @@ -1692,16 +1732,7 @@ function partialSetCollection({collectionKey, co keysChanged(collectionKey, mutableCollection, previousCollection); - if (OnyxKeys.isRamOnlyKey(collectionKey)) { - sendActionToDevTools(METHOD.SET_COLLECTION, undefined, mutableCollection); - return; - } - - return Storage.multiSet(keyValuePairs) - .catch((error) => retryOperation(error, partialSetCollection, {collectionKey, collection}, retryAttempt)) - .then(() => { - sendActionToDevTools(METHOD.SET_COLLECTION, undefined, mutableCollection); - }); + return persistCollectionWrite({collectionKey, keyValuePairs, mutableCollection}, retryAttempt); }); } @@ -1766,12 +1797,15 @@ const OnyxUtils = { reduceCollectionWithSelector, updateSnapshots, mergeCollectionWithPatches, + persistMergedCollectionWrite, partialSetCollection, logKeyChanged, logKeyRemoved, setWithRetry, multiSetWithRetry, + persistMultiSetWrite, setCollectionWithRetry, + persistCollectionWrite, }; export type {OnyxMethod}; diff --git a/lib/types.ts b/lib/types.ts index 039130df9..7a549dc77 100644 --- a/lib/types.ts +++ b/lib/types.ts @@ -371,8 +371,11 @@ type MergeCollectionWithPatchesParams = { type RetriableOnyxOperation = | typeof OnyxUtils.setWithRetry | typeof OnyxUtils.multiSetWithRetry + | typeof OnyxUtils.persistMultiSetWrite | typeof OnyxUtils.setCollectionWithRetry + | typeof OnyxUtils.persistCollectionWrite | typeof OnyxUtils.mergeCollectionWithPatches + | typeof OnyxUtils.persistMergedCollectionWrite | typeof OnyxUtils.partialSetCollection; /** diff --git a/tests/unit/onyxUtilsTest.ts b/tests/unit/onyxUtilsTest.ts index a545275c5..6bc736530 100644 --- a/tests/unit/onyxUtilsTest.ts +++ b/tests/unit/onyxUtilsTest.ts @@ -920,6 +920,156 @@ describe('OnyxUtils', () => { }); }); + describe('retry side-effect idempotency', () => { + // Save originals so each test can replace StorageMock.multiMerge / StorageMock.multiSet + // with a mock that rejects once and then resolves, exercising the retryOperation path + // without burning the full retry budget. Restoring keeps mocks from leaking into the + // storage-eviction describe block below (which depends on these storage methods). + 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 — new keys still route through multiSet on retry, not downgraded to multiMerge', async () => { + const collectionKey = ONYXKEYS.COLLECTION.TEST_KEY; + const existingMemberKey = `${collectionKey}1`; + const newMemberKey = `${collectionKey}2`; + + // Seed an existing member via setItem so the multiMerge mock below doesn't intercept seeding. + await Onyx.set(existingMemberKey, {value: 'initial'}); + + const multiMergeSpy = jest.fn(originalMultiMerge).mockRejectedValueOnce(transientError); + StorageMock.multiMerge = multiMergeSpy; + + await Onyx.mergeCollection(collectionKey, { + [existingMemberKey]: {value: 'merged'}, + [newMemberKey]: {value: 'new'}, + } as GenericCollection); + + // Before this fix, the retry attempt re-derived the existing/new split against a cache + // already mutated by the first attempt, which silently routed `newMemberKey` through + // multiMerge. Benign on IDB (multiMerge on a missing key behaves like set) but wrong + // semantically and a crash on storage backends that require multiSet for new keys. + const allMultiMergeKeys = multiMergeSpy.mock.calls.flatMap((args) => (args[0] as Array<[string, unknown]>).map(([key]) => key)); + expect(allMultiMergeKeys).not.toContain(newMemberKey); + + // Sanity: the retry actually happened. + expect(multiMergeSpy).toHaveBeenCalledTimes(2); + }); + + 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 re-enter only the storage-write helper, 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'); From de62ae5eae69a4aaf0b1b734e408ba5db2a9c79b Mon Sep 17 00:00:00 2001 From: eliran goshen Date: Mon, 25 May 2026 15:35:02 +0200 Subject: [PATCH 2/4] Regenerate API-INTERNAL.md for new write helpers `npm run build:docs` output for the three new file-private helpers introduced in the previous commit: persistMultiSetWrite, persistCollectionWrite, persistMergedCollectionWrite. Co-Authored-By: Claude Opus 4.7 (1M context) --- API-INTERNAL.md | 41 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/API-INTERNAL.md b/API-INTERNAL.md index b3cfeb90a..7149a40cd 100644 --- a/API-INTERNAL.md +++ b/API-INTERNAL.md @@ -125,17 +125,33 @@ It will also mark deep nested objects that need to be entirely replaced during t Serves as core implementation for Onyx.set() public function, the difference being that this internal function allows passing an additional retryAttempt parameter to retry on failure.

+
persistMultiSetWrite()
+

Storage-write tail of multiSetWithRetry, isolated so that retryOperation re-enters only the +storage step. Cache and subscriber notifications already happened in the orchestrator, so +retries no longer re-fire waitForCollectionCallback subscribers with the same payload.

+
multiSetWithRetry(data, retryAttempt)

Sets multiple keys and values. Serves as core implementation for Onyx.multiSet() public function, the difference being that this internal function allows passing an additional retryAttempt parameter to retry on failure.

+
persistCollectionWrite()
+

Storage-write tail of setCollectionWithRetry, isolated so that retryOperation re-enters only the +storage step. Cache and subscriber notifications already happened in the orchestrator, so +retries no longer re-fire waitForCollectionCallback subscribers with the same payload.

+
setCollectionWithRetry(params, retryAttempt)

Sets a collection by replacing all existing collection members with new values. Any existing collection members not included in the new data will be removed. Serves as core implementation for Onyx.setCollection() public function, the difference being that this internal function allows passing an additional retryAttempt parameter to retry on failure.

+
persistMergedCollectionWrite()
+

Storage-write tail of mergeCollectionWithPatches, isolated so that retryOperation re-enters only +the storage step. Cache and subscriber notifications already happened in the orchestrator, and +the existing/new key split is captured in params — so retries don't re-fire subscribers and +don't reclassify keys against a cache that was already mutated on the first attempt.

+
mergeCollectionWithPatches(params, retryAttempt)

Merges a collection based on their keys. Serves as core implementation for Onyx.mergeCollection() public function, the difference being @@ -435,6 +451,14 @@ that this internal function allows passing an additional `retryAttempt` paramete | params.options | optional configuration object | | retryAttempt | retry attempt | + + +## persistMultiSetWrite() +Storage-write tail of multiSetWithRetry, isolated so that retryOperation re-enters only the +storage step. Cache and subscriber notifications already happened in the orchestrator, so +retries no longer re-fire `waitForCollectionCallback` subscribers with the same payload. + +**Kind**: global function ## multiSetWithRetry(data, retryAttempt) @@ -449,6 +473,14 @@ that this internal function allows passing an additional `retryAttempt` paramete | data | object keyed by ONYXKEYS and the values to set | | retryAttempt | retry attempt | + + +## persistCollectionWrite() +Storage-write tail of setCollectionWithRetry, isolated so that retryOperation re-enters only the +storage step. Cache and subscriber notifications already happened in the orchestrator, so +retries no longer re-fire `waitForCollectionCallback` subscribers with the same payload. + +**Kind**: global function ## setCollectionWithRetry(params, retryAttempt) @@ -466,6 +498,15 @@ that this internal function allows passing an additional `retryAttempt` paramete | params.collection | Object collection keyed by individual collection member keys and values | | retryAttempt | retry attempt | + + +## persistMergedCollectionWrite() +Storage-write tail of mergeCollectionWithPatches, isolated so that retryOperation re-enters only +the storage step. Cache and subscriber notifications already happened in the orchestrator, and +the existing/new key split is captured in `params` — so retries don't re-fire subscribers and +don't reclassify keys against a cache that was already mutated on the first attempt. + +**Kind**: global function ## mergeCollectionWithPatches(params, retryAttempt) From 11bc0a626fa1cbd632fa39c1f7b3b56e7b910c5c Mon Sep 17 00:00:00 2001 From: eliran goshen Date: Tue, 26 May 2026 16:03:33 +0200 Subject: [PATCH 3/4] Restore cache state for evicted in-flight keys on retry Addresses Codex P1 on #792 review: https://github.com/Expensify/react-native-onyx/pull/792#discussion_r3303754190 When `retryOperation()` evicts an LRU key via `remove()` on a storage capacity error, `remove()` immediately drops the key from cache and fires `keyChanged(undefined)`. With the orchestrator/write-helper split introduced in this PR, the retry only re-attempts the storage write -- it does not re-apply cache state. So if the evicted key is part of the in-flight write, cache + subscribers stay in the "removed" state even though the storage retry eventually succeeds, leaving the App's view of the world inconsistent with persisted data. Fix: each orchestrator now constructs a `restoreEvictedKey(key)` closure that re-applies its cache write + subscriber notification for a single in-flight key, and passes it via the write helper's params. `retryOperation` invokes the closure after `remove(keyForRemoval)`, before re-entering the write helper. The closure is a no-op when the evicted key isn't part of this write (the typical case where some unrelated evictable key gets selected). For mergeCollection specifically, the closure snapshots the cache's post-merge state for in-flight keys (rather than just the merge delta) so that previously-merged-in fields are preserved when the eviction's cache.drop forces us to re-populate cache from scratch. Adds 3 regression tests under `storage eviction`: - `multiSet` -- LRU evictable is the in-flight key (persistMultiSetWrite path) - `setCollection` -- same scenario via persistCollectionWrite - `mergeCollection` -- same scenario via persistMergedCollectionWrite, asserts the previously-existing `id` field survives the eviction All 3 tests fail with `Received: undefined` / missing fields if the lib changes are reverted; pass with them in place. All 459 unit tests pass (456 existing + 3 new). --- lib/OnyxUtils.ts | 107 +++++++++++++++++++++++++++++++++--- tests/unit/onyxUtilsTest.ts | 87 +++++++++++++++++++++++++++++ 2 files changed, 187 insertions(+), 7 deletions(-) diff --git a/lib/OnyxUtils.ts b/lib/OnyxUtils.ts index 916d88501..9493f6d6b 100644 --- a/lib/OnyxUtils.ts +++ b/lib/OnyxUtils.ts @@ -842,8 +842,20 @@ function retryOperation(error: Error, on Logger.logInfo(`Out of storage. Evicting least recently accessed key (${keyForRemoval}) and retrying. Error: ${error}`); reportStorageQuota(error); - // @ts-expect-error No overload matches this call. - return remove(keyForRemoval).then(() => onyxMethod(defaultParams, nextRetryAttempt)); + return remove(keyForRemoval).then(() => { + // remove() drops `keyForRemoval` from cache and fires keyChanged(undefined). If that key is + // part of this in-flight write, the upcoming retry would land the value in storage but + // cache + subscribers would stay in the "removed" state. Each orchestrator passes a + // restoreEvictedKey closure via params that re-applies the orchestrator's cache write + + // subscriber notification for the evicted key only — preserving the cache-first invariant + // across eviction-driven retries. + const restore = (defaultParams as {restoreEvictedKey?: (key: OnyxKey) => void}).restoreEvictedKey; + if (typeof restore === 'function') { + restore(keyForRemoval); + } + // @ts-expect-error No overload matches this call. + return onyxMethod(defaultParams, nextRetryAttempt); + }); } /** @@ -1338,7 +1350,10 @@ function setWithRetry({key, value, options}: SetParams { +function persistMultiSetWrite( + params: {keyValuePairsToStore: StorageKeyValuePair[]; newData: OnyxMultiSetInput; restoreEvictedKey?: (evictedKey: OnyxKey) => void}, + retryAttempt?: number, +): Promise { const {keyValuePairsToStore, newData} = params; return Storage.multiSet(keyValuePairsToStore) @@ -1426,7 +1441,31 @@ function multiSetWithRetry(data: OnyxMultiSetInput, retryAttempt?: number): Prom return !OnyxKeys.isRamOnlyKey(key); }); - return persistMultiSetWrite({keyValuePairsToStore, newData}, retryAttempt); + // Capture the in-flight key/value map so retryOperation can re-apply cache state for any + // evicted-then-retried key (see retryOperation's eviction branch for context). + const inFlightValueByKey = new Map>(); + for (const [inFlightKey, inFlightValue] of keyValuePairsToSet) { + inFlightValueByKey.set(inFlightKey, inFlightValue); + } + const restoreEvictedKey = (evictedKey: OnyxKey): void => { + if (!inFlightValueByKey.has(evictedKey)) { + return; + } + const value = inFlightValueByKey.get(evictedKey) as OnyxValue; + const evictedCollectionKey = OnyxKeys.getCollectionKey(evictedKey); + cache.set(evictedKey, value); + if (evictedCollectionKey && OnyxKeys.isCollectionMemberKey(evictedCollectionKey, evictedKey)) { + keysChanged( + evictedCollectionKey as CollectionKeyBase, + {[evictedKey]: value} as Record>, + {[evictedKey]: undefined} as Record>, + ); + } else { + keyChanged(evictedKey, value); + } + }; + + return persistMultiSetWrite({keyValuePairsToStore, newData, restoreEvictedKey}, retryAttempt); } /** @@ -1439,6 +1478,7 @@ function persistCollectionWrite( collectionKey: TKey; keyValuePairs: StorageKeyValuePair[]; mutableCollection: OnyxInputKeyValueMapping; + restoreEvictedKey?: (evictedKey: OnyxKey) => void; }, retryAttempt?: number, ): Promise { @@ -1517,7 +1557,22 @@ function setCollectionWithRetry({collectionKey, keysChanged(collectionKey, mutableCollection, previousCollection); - return persistCollectionWrite({collectionKey, keyValuePairs, mutableCollection}, retryAttempt); + // Capture the in-flight key/value map so retryOperation can re-apply cache state for any + // evicted-then-retried key (see retryOperation's eviction branch for context). + const inFlightValueByKey = new Map>(); + for (const [inFlightKey, inFlightValue] of keyValuePairs) { + inFlightValueByKey.set(inFlightKey, inFlightValue); + } + const restoreEvictedKey = (evictedKey: OnyxKey): void => { + if (!inFlightValueByKey.has(evictedKey)) { + return; + } + const value = inFlightValueByKey.get(evictedKey) as OnyxValue; + cache.set(evictedKey, value); + keysChanged(collectionKey, {[evictedKey]: value} as Record>, {[evictedKey]: undefined} as Record>); + }; + + return persistCollectionWrite({collectionKey, keyValuePairs, mutableCollection, restoreEvictedKey}, retryAttempt); }); } @@ -1533,6 +1588,7 @@ function persistMergedCollectionWrite( keyValuePairsForExistingCollection: StorageKeyValuePair[]; keyValuePairsForNewCollection: StorageKeyValuePair[]; resultCollection: OnyxInputKeyValueMapping; + restoreEvictedKey?: (evictedKey: OnyxKey) => void; }, retryAttempt?: number, ): Promise { @@ -1678,7 +1734,29 @@ function mergeCollectionWithPatches( cache.merge(finalMergedCollection); keysChanged(collectionKey, finalMergedCollection, previousCollection); - return persistMergedCollectionWrite({collectionKey, keyValuePairsForExistingCollection, keyValuePairsForNewCollection, resultCollection}, retryAttempt); + // Snapshot the post-merge cache values for the in-flight keys. The orchestrator's + // cache.merge already merged the deltas into the previous storage values, so the cache + // now holds the *full* merged value for each key. Using the snapshot (rather than the + // delta in `finalMergedCollection`) preserves previously-merged-in fields when the + // eviction's cache.drop forces us to re-populate cache from scratch on retry. + const inFlightMergedSnapshot = new Map>(); + for (const inFlightKey of Object.keys(finalMergedCollection)) { + inFlightMergedSnapshot.set(inFlightKey, cache.get(inFlightKey)); + } + + // Closure invoked by retryOperation after an eviction picks a key that's part of this + // merge. We re-apply the full merged value + a keysChanged notification for that one + // key so subscribers don't stay in the "removed" state across the imminent storage retry. + const restoreEvictedKey = (evictedKey: OnyxKey): void => { + if (!inFlightMergedSnapshot.has(evictedKey)) { + return; + } + const value = inFlightMergedSnapshot.get(evictedKey) as OnyxValue; + cache.set(evictedKey, value); + keysChanged(collectionKey, {[evictedKey]: value} as Record>, {[evictedKey]: undefined} as Record>); + }; + + return persistMergedCollectionWrite({collectionKey, keyValuePairsForExistingCollection, keyValuePairsForNewCollection, resultCollection, restoreEvictedKey}, retryAttempt); }); }) .then(() => undefined); @@ -1732,7 +1810,22 @@ function partialSetCollection({collectionKey, co keysChanged(collectionKey, mutableCollection, previousCollection); - return persistCollectionWrite({collectionKey, keyValuePairs, mutableCollection}, retryAttempt); + // Capture the in-flight key/value map so retryOperation can re-apply cache state for any + // evicted-then-retried key (see retryOperation's eviction branch for context). + const inFlightValueByKey = new Map>(); + for (const [inFlightKey, inFlightValue] of keyValuePairs) { + inFlightValueByKey.set(inFlightKey, inFlightValue); + } + const restoreEvictedKey = (evictedKey: OnyxKey): void => { + if (!inFlightValueByKey.has(evictedKey)) { + return; + } + const value = inFlightValueByKey.get(evictedKey) as OnyxValue; + cache.set(evictedKey, value); + keysChanged(collectionKey, {[evictedKey]: value} as Record>, {[evictedKey]: undefined} as Record>); + }; + + return persistCollectionWrite({collectionKey, keyValuePairs, mutableCollection, restoreEvictedKey}, retryAttempt); }); } diff --git a/tests/unit/onyxUtilsTest.ts b/tests/unit/onyxUtilsTest.ts index 6bc736530..3a8405680 100644 --- a/tests/unit/onyxUtilsTest.ts +++ b/tests/unit/onyxUtilsTest.ts @@ -1209,6 +1209,93 @@ describe('OnyxUtils', () => { expect(logInfoSpy).toHaveBeenCalledWith(`Out of storage. Evicting least recently accessed key (${key1}) and retrying. Error: ${diskFullError}`); expect(logInfoSpy).toHaveBeenCalledWith(`Storage Quota Check -- bytesUsed: 0 bytesRemaining: Infinity. Original error: ${diskFullError}`); }); + + // Eviction-restoration tests: when retryOperation evicts a key via remove() that happens to be + // part of the in-flight write, the orchestrator's restoreEvictedKey closure must re-apply + // cache state for that key. Otherwise cache + subscribers stay in the "removed" state from + // the eviction's keyChanged(undefined) even after the retry succeeds in storage. + it('multiSet — restores cache + subscribers when the in-flight key is the LRU evictable', async () => { + const memberKey = `${ONYXKEYS.COLLECTION.TEST_KEY}1`; + + // Seed memberKey so it enters the recentlyAccessedKeys set. It's the only evictable key, + // so getKeyForEviction() will return memberKey at the time of the multiSet retry. + await LocalOnyx.set(memberKey, {id: 1, value: 'original'}); + expect(LocalOnyxCache.getKeyForEviction()).toBe(memberKey); + + const subscriberCalls: Array = []; + LocalOnyx.connect({ + key: memberKey, + callback: (value) => subscriberCalls.push(value), + }); + await waitForPromisesToResolve(); + subscriberCalls.length = 0; + + // Storage.multiSet rejects once with disk-full, then succeeds on retry. + LocalStorageMock.multiSet = jest.fn(LocalStorageMock.multiSet).mockRejectedValueOnce(diskFullError).mockImplementation(LocalStorageMock.multiSet); + + await LocalOnyx.multiSet({[memberKey]: {id: 1, value: 'updated'}}); + + // Cache must reflect the new value, not undefined from the eviction's remove(). + expect(LocalOnyxCache.get(memberKey)).toEqual({id: 1, value: 'updated'}); + + // Subscriber's last value must be the new value, not undefined. + expect(subscriberCalls.at(-1)).toEqual({id: 1, value: 'updated'}); + }); + + it('setCollection — restores cache + subscribers when the in-flight key is the LRU evictable', async () => { + const memberKey = `${ONYXKEYS.COLLECTION.TEST_KEY}1`; + + await LocalOnyx.set(memberKey, {id: 1, value: 'original'}); + expect(LocalOnyxCache.getKeyForEviction()).toBe(memberKey); + + const collectionCalls: Array = []; + LocalOnyx.connect({ + key: ONYXKEYS.COLLECTION.TEST_KEY, + waitForCollectionCallback: true, + callback: (value) => collectionCalls.push(value), + }); + await waitForPromisesToResolve(); + collectionCalls.length = 0; + + LocalStorageMock.multiSet = jest.fn(LocalStorageMock.multiSet).mockRejectedValueOnce(diskFullError).mockImplementation(LocalStorageMock.multiSet); + + await LocalOnyx.setCollection(ONYXKEYS.COLLECTION.TEST_KEY, { + [memberKey]: {id: 1, value: 'updated'}, + } as GenericCollection); + + expect(LocalOnyxCache.get(memberKey)).toEqual({id: 1, value: 'updated'}); + // Last collection callback must include the restored member with its new value. + const lastBroadcast = collectionCalls.at(-1) as Record | undefined; + expect(lastBroadcast?.[memberKey]).toEqual({id: 1, value: 'updated'}); + }); + + it('mergeCollection — restores cache + subscribers when the in-flight key is the LRU evictable', async () => { + const memberKey = `${ONYXKEYS.COLLECTION.TEST_KEY}1`; + + await LocalOnyx.set(memberKey, {id: 1, value: 'original'}); + expect(LocalOnyxCache.getKeyForEviction()).toBe(memberKey); + + const collectionCalls: Array = []; + LocalOnyx.connect({ + key: ONYXKEYS.COLLECTION.TEST_KEY, + waitForCollectionCallback: true, + callback: (value) => collectionCalls.push(value), + }); + await waitForPromisesToResolve(); + collectionCalls.length = 0; + + // mergeCollection routes existing keys through multiMerge — fail it once then succeed. + LocalStorageMock.multiMerge = jest.fn(LocalStorageMock.multiMerge).mockRejectedValueOnce(diskFullError).mockImplementation(LocalStorageMock.multiMerge); + + await LocalOnyx.mergeCollection(ONYXKEYS.COLLECTION.TEST_KEY, { + [memberKey]: {value: 'merged'}, + } as GenericCollection); + + // Cache must reflect the merged value, not undefined from the eviction's remove(). + expect(LocalOnyxCache.get(memberKey)).toEqual({id: 1, value: 'merged'}); + const lastBroadcast = collectionCalls.at(-1) as Record | undefined; + expect(lastBroadcast?.[memberKey]).toEqual({id: 1, value: 'merged'}); + }); }); describe('afterInit', () => { From e70af5e9c1869049d5b7fc14925427025196b15a Mon Sep 17 00:00:00 2001 From: eliran goshen Date: Tue, 26 May 2026 16:06:56 +0200 Subject: [PATCH 4/4] Fix lint: use `unknown[]` instead of `Array` CI flagged 3 occurrences in the eviction-restoration regression tests added in the previous commit. The repo's eslint config prefers the shorthand for simple types via @typescript-eslint/array-type. --- tests/unit/onyxUtilsTest.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/unit/onyxUtilsTest.ts b/tests/unit/onyxUtilsTest.ts index 3a8405680..c76c342d6 100644 --- a/tests/unit/onyxUtilsTest.ts +++ b/tests/unit/onyxUtilsTest.ts @@ -1222,7 +1222,7 @@ describe('OnyxUtils', () => { await LocalOnyx.set(memberKey, {id: 1, value: 'original'}); expect(LocalOnyxCache.getKeyForEviction()).toBe(memberKey); - const subscriberCalls: Array = []; + const subscriberCalls: unknown[] = []; LocalOnyx.connect({ key: memberKey, callback: (value) => subscriberCalls.push(value), @@ -1248,7 +1248,7 @@ describe('OnyxUtils', () => { await LocalOnyx.set(memberKey, {id: 1, value: 'original'}); expect(LocalOnyxCache.getKeyForEviction()).toBe(memberKey); - const collectionCalls: Array = []; + const collectionCalls: unknown[] = []; LocalOnyx.connect({ key: ONYXKEYS.COLLECTION.TEST_KEY, waitForCollectionCallback: true, @@ -1275,7 +1275,7 @@ describe('OnyxUtils', () => { await LocalOnyx.set(memberKey, {id: 1, value: 'original'}); expect(LocalOnyxCache.getKeyForEviction()).toBe(memberKey); - const collectionCalls: Array = []; + const collectionCalls: unknown[] = []; LocalOnyx.connect({ key: ONYXKEYS.COLLECTION.TEST_KEY, waitForCollectionCallback: true,