From bd0b1e98192756a42d4204e38356dd31bc8c5b17 Mon Sep 17 00:00:00 2001 From: eliran goshen Date: Thu, 28 May 2026 15:42:15 +0200 Subject: [PATCH 1/4] fix: avoid subscriber re-fire when storage writes retry Co-Authored-By: Claude Opus 4.7 (1M context) --- lib/OnyxUtils.ts | 44 ++++++++++--- tests/unit/onyxUtilsTest.ts | 122 ++++++++++++++++++++++++++++++++++++ 2 files changed, 157 insertions(+), 9 deletions(-) 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'); From 8b18535cbc408e1352d890e83129c94f8d4450ce Mon Sep 17 00:00:00 2001 From: eliran goshen Date: Fri, 29 May 2026 10:44:52 +0200 Subject: [PATCH 2/4] fix: notify subscribers when evicting unrelated keys during write retry Co-Authored-By: Claude Opus 4.7 (1M context) --- lib/OnyxUtils.ts | 31 +++++++++++++++----- tests/unit/onyxUtilsTest.ts | 57 +++++++++++++++++++++++++++++++++++++ 2 files changed, 81 insertions(+), 7 deletions(-) diff --git a/lib/OnyxUtils.ts b/lib/OnyxUtils.ts index 917a3b4a1..87a4a9370 100644 --- a/lib/OnyxUtils.ts +++ b/lib/OnyxUtils.ts @@ -802,7 +802,13 @@ function reportStorageQuota(error?: Error): Promise { * - Non-retriable errors: logs an alert and resolves without retrying * - Other errors: retries the operation */ -function retryOperation(error: Error, onyxMethod: TMethod, defaultParams: Parameters[0], retryAttempt: number | undefined): Promise { +function retryOperation( + error: Error, + onyxMethod: TMethod, + defaultParams: Parameters[0], + retryAttempt: number | undefined, + inFlightKeys?: Set, +): Promise { const currentRetryAttempt = retryAttempt ?? 0; const nextRetryAttempt = currentRetryAttempt + 1; @@ -847,10 +853,12 @@ 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. + // Only suppress keyChanged(undefined) when the evicted key is part of the in-flight + // write — then cache.set on retry will restore it. For unrelated keys, eviction is a + // genuine loss and subscribers must see the removed state. + const willBeRestored = inFlightKeys?.has(keyForRemoval) ?? false; // @ts-expect-error No overload matches this call. - return remove(keyForRemoval, undefined, true).then(() => onyxMethod(defaultParams, nextRetryAttempt)); + return remove(keyForRemoval, undefined, willBeRestored).then(() => onyxMethod(defaultParams, nextRetryAttempt)); } /** @@ -1425,8 +1433,10 @@ function multiSetWithRetry(data: OnyxMultiSetInput, retryAttempt?: number): Prom return !OnyxKeys.isRamOnlyKey(key); }); + const inFlightKeys = new Set(keyValuePairsToSet.map(([key]) => key)); + return Storage.multiSet(keyValuePairsToStore) - .catch((error) => OnyxUtils.retryOperation(error, multiSetWithRetry, newData, retryAttempt)) + .catch((error) => OnyxUtils.retryOperation(error, multiSetWithRetry, newData, retryAttempt, inFlightKeys)) .then(() => { OnyxUtils.sendActionToDevTools(OnyxUtils.METHOD.MULTI_SET, undefined, newData); }); @@ -1502,8 +1512,10 @@ function setCollectionWithRetry({collectionKey, return; } + const inFlightKeys = new Set(keyValuePairs.map(([key]) => key)); + return Storage.multiSet(keyValuePairs) - .catch((error) => OnyxUtils.retryOperation(error, setCollectionWithRetry, {collectionKey, collection}, retryAttempt)) + .catch((error) => OnyxUtils.retryOperation(error, setCollectionWithRetry, {collectionKey, collection}, retryAttempt, inFlightKeys)) .then(() => { OnyxUtils.sendActionToDevTools(OnyxUtils.METHOD.SET_COLLECTION, undefined, mutableCollection); }); @@ -1649,6 +1661,8 @@ function mergeCollectionWithPatches( promises.push(Storage.multiSet(keyValuePairsForNewCollection)); } + const inFlightKeys = new Set(Object.keys(finalMergedCollection)); + return Promise.all(promises) .catch((error) => retryOperation( @@ -1656,6 +1670,7 @@ function mergeCollectionWithPatches( mergeCollectionWithPatches, {collectionKey, collection: resultCollection as OnyxMergeCollectionInput, mergeReplaceNullPatches, isProcessingCollectionUpdate}, retryAttempt, + inFlightKeys, ), ) .then(() => { @@ -1723,8 +1738,10 @@ function partialSetCollection({collectionKey, co return; } + const inFlightKeys = new Set(keyValuePairs.map(([key]) => key)); + return Storage.multiSet(keyValuePairs) - .catch((error) => retryOperation(error, partialSetCollection, {collectionKey, collection}, retryAttempt)) + .catch((error) => retryOperation(error, partialSetCollection, {collectionKey, collection}, retryAttempt, inFlightKeys)) .then(() => { sendActionToDevTools(METHOD.SET_COLLECTION, undefined, mutableCollection); }); diff --git a/tests/unit/onyxUtilsTest.ts b/tests/unit/onyxUtilsTest.ts index bdf85d134..564104326 100644 --- a/tests/unit/onyxUtilsTest.ts +++ b/tests/unit/onyxUtilsTest.ts @@ -1181,6 +1181,63 @@ 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}`); }); + + it('multiSet — eviction of an UNRELATED key still notifies its subscribers (codex regression guard)', async () => { + const evictableKey = `${ONYXKEYS.COLLECTION.TEST_KEY}1`; + const writeKey = `${ONYXKEYS.COLLECTION.TEST_KEY}2`; + + // Seed the evictable key first so it becomes the LRU evictable. The subsequent multiSet + // writes a DIFFERENT key, so the evicted key is unrelated to the in-flight write. + await LocalOnyx.set(evictableKey, {value: 'will-be-evicted'}); + expect(LocalOnyxCache.getKeyForEviction()).toBe(evictableKey); + + const subscriberCalls: unknown[] = []; + LocalOnyx.connect({ + key: evictableKey, + 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({[writeKey]: {value: 'new'}}); + + // evictableKey was the LRU evictable, so retryOperation evicted it. It's not in the + // in-flight write's keys, so the retry's cache.set won't restore it — subscribers MUST + // see keyChanged(undefined) so they reflect the genuine removal (not stale value). + expect(LocalOnyxCache.hasCacheForKey(evictableKey)).toBe(false); + expect(subscriberCalls.at(-1)).toBeUndefined(); + }); + + it('multiSet — eviction of an IN-FLIGHT key does not strand its subscriber', async () => { + const memberKey = `${ONYXKEYS.COLLECTION.TEST_KEY}1`; + + // Seed memberKey so it becomes the LRU evictable. The multiSet below writes to the SAME + // key, so eviction picks an in-flight key. + await LocalOnyx.set(memberKey, {value: 'original'}); + expect(LocalOnyxCache.getKeyForEviction()).toBe(memberKey); + + const subscriberCalls: unknown[] = []; + LocalOnyx.connect({ + key: memberKey, + callback: (value) => subscriberCalls.push(value), + }); + await waitForPromisesToResolve(); + subscriberCalls.length = 0; + + LocalStorageMock.multiSet = jest.fn(LocalStorageMock.multiSet).mockRejectedValueOnce(diskFullError).mockImplementation(LocalStorageMock.multiSet); + + await LocalOnyx.multiSet({[memberKey]: {value: 'updated'}}); + + // The in-flight key was evicted then restored by the retry's cache.set. Subscriber's + // last value must be the new value, never a transient undefined from the eviction. + expect(LocalOnyxCache.get(memberKey)).toEqual({value: 'updated'}); + expect(subscriberCalls.at(-1)).toEqual({value: 'updated'}); + // Subscriber should never have seen undefined in the middle of the eviction-retry cycle. + expect(subscriberCalls).not.toContain(undefined); + }); }); describe('afterInit', () => { From b001767d783c016403d892d27c6fb099927471e7 Mon Sep 17 00:00:00 2001 From: eliran goshen Date: Wed, 3 Jun 2026 09:28:32 +0200 Subject: [PATCH 3/4] fix: don't evict in-flight keys on retry; correct stale multiMerge-new-key comment Co-Authored-By: Claude Opus 4.8 (1M context) --- lib/OnyxCache.ts | 13 ++++- lib/OnyxUtils.ts | 28 ++++----- tests/unit/onyxUtilsTest.ts | 57 ++++++++++++++++++- .../providers/IDBKeyvalProviderTest.ts | 5 ++ 4 files changed, 82 insertions(+), 21 deletions(-) diff --git a/lib/OnyxCache.ts b/lib/OnyxCache.ts index 725dbfd77..23fae2130 100644 --- a/lib/OnyxCache.ts +++ b/lib/OnyxCache.ts @@ -362,11 +362,18 @@ class OnyxCache { /** * Finds the least recently accessed key that can be safely evicted from storage. + * `excludeKeys` skips keys that must not be evicted (e.g. the in-flight write's own keys, + * whose cache value is the merge base the retry depends on). */ - getKeyForEviction(): OnyxKey | undefined { + getKeyForEviction(excludeKeys?: Set): OnyxKey | undefined { // recentlyAccessedKeys is ordered from least to most recently accessed, - // so the first element is the best candidate for eviction. - return this.recentlyAccessedKeys.values().next().value; + // so the first non-excluded key is the best candidate for eviction. + for (const key of this.recentlyAccessedKeys) { + if (!excludeKeys?.has(key)) { + return key; + } + } + return undefined; } /** diff --git a/lib/OnyxUtils.ts b/lib/OnyxUtils.ts index 6713d749f..6c180b983 100644 --- a/lib/OnyxUtils.ts +++ b/lib/OnyxUtils.ts @@ -777,14 +777,9 @@ function getCollectionDataAndSendAsObject(matchingKeys: Co /** * Remove a key from Onyx and update the subscribers */ -function remove(key: TKey, isProcessingCollectionUpdate?: boolean, skipNotify?: boolean): Promise { +function remove(key: TKey, isProcessingCollectionUpdate?: boolean): Promise { cache.drop(key); - // 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); - } + keyChanged(key, undefined as OnyxValue, undefined, isProcessingCollectionUpdate); if (OnyxKeys.isRamOnlyKey(key)) { return Promise.resolve(); @@ -851,8 +846,10 @@ function retryOperation( return onyxMethod(defaultParams, nextRetryAttempt); } - // Find the least recently accessed evictable key that we can remove - const keyForRemoval = cache.getKeyForEviction(); + // Find the least recently accessed evictable key that we can remove. Never evict an in-flight + // key — its cache value is the merge base this retry depends on, so dropping it would truncate + // the write to just the delta and diverge cache from storage. + const keyForRemoval = cache.getKeyForEviction(inFlightKeys); if (!keyForRemoval) { // If we have no acceptable keys to remove then we are possibly trying to save mission critical data. If this is the case, // then we should stop retrying as there is not much the user can do to fix this. Instead of getting them stuck in an infinite loop we @@ -865,12 +862,10 @@ function retryOperation( Logger.logInfo(`Out of storage. Evicting least recently accessed key (${keyForRemoval}) and retrying. Error: ${error}`); reportStorageQuota(error); - // Only suppress keyChanged(undefined) when the evicted key is part of the in-flight - // write — then cache.set on retry will restore it. For unrelated keys, eviction is a - // genuine loss and subscribers must see the removed state. - const willBeRestored = inFlightKeys?.has(keyForRemoval) ?? false; + // The evicted key is never in-flight (excluded above), so this is a genuine removal — + // subscribers must see the removed state. // @ts-expect-error No overload matches this call. - return remove(keyForRemoval, undefined, willBeRestored).then(() => onyxMethod(defaultParams, nextRetryAttempt)); + return remove(keyForRemoval).then(() => onyxMethod(defaultParams, nextRetryAttempt)); } /** @@ -1666,8 +1661,9 @@ function mergeCollectionWithPatches( 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 + // New keys go through multiSet and existing keys through multiMerge. multiMerge on a + // missing key stores the value just like multiSet across all backends; splitting them lets + // multiSet strip nested nulls (the merge layer keeps them to delete nested storage keys). // 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)); diff --git a/tests/unit/onyxUtilsTest.ts b/tests/unit/onyxUtilsTest.ts index 3a7c8d222..8767dca82 100644 --- a/tests/unit/onyxUtilsTest.ts +++ b/tests/unit/onyxUtilsTest.ts @@ -1566,13 +1566,66 @@ describe('OnyxUtils', () => { await LocalOnyx.multiSet({[memberKey]: {value: 'updated'}}); - // The in-flight key was evicted then restored by the retry's cache.set. Subscriber's - // last value must be the new value, never a transient undefined from the eviction. + // The in-flight key is excluded from eviction, so its cache value (the merge base) is + // never dropped. Subscriber's last value is the new value, never a transient undefined. expect(LocalOnyxCache.get(memberKey)).toEqual({value: 'updated'}); expect(subscriberCalls.at(-1)).toEqual({value: 'updated'}); // Subscriber should never have seen undefined in the middle of the eviction-retry cycle. expect(subscriberCalls).not.toContain(undefined); }); + + it('mergeCollection — evicts an unrelated key, not the in-flight key, so its fields survive', async () => { + const collectionKey = ONYXKEYS.COLLECTION.TEST_KEY; + const memberKey = `${collectionKey}1`; + const unrelatedKey = `${collectionKey}2`; + + // Seed the in-flight member with extra fields, plus a separate evictable key. The merge + // only touches memberKey; the unrelated key is the genuine eviction target. + await LocalOnyx.set(memberKey, {id: 1, value: 'orig'}); + await LocalOnyx.set(unrelatedKey, {value: 'evict-me'}); + + const memberCalls: unknown[] = []; + LocalOnyx.connect({key: memberKey, callback: (value) => memberCalls.push(value)}); + await waitForPromisesToResolve(); + memberCalls.length = 0; + + // Storage.multiMerge rejects once with disk-full, then succeeds on retry. + LocalStorageMock.multiMerge = jest.fn(LocalStorageMock.multiMerge).mockRejectedValueOnce(diskFullError).mockImplementation(LocalStorageMock.multiMerge); + + await LocalOnyx.mergeCollection(collectionKey, {[memberKey]: {value: 'merged'}} as GenericCollection); + + // The old code evicted the in-flight key and re-ran the merge against an empty cache, + // collapsing {id: 1, value: 'orig'} + {value: 'merged'} to just {value: 'merged'}. Now + // the in-flight key is protected, so its pre-existing {id: 1} survives. + expect(LocalOnyxCache.get(memberKey)).toEqual({id: 1, value: 'merged'}); + expect(memberCalls.at(-1)).toEqual({id: 1, value: 'merged'}); + expect(memberCalls).not.toContain(undefined); + // The unrelated key was the genuine eviction target. + expect(LocalOnyxCache.hasCacheForKey(unrelatedKey)).toBe(false); + }); + + it('mergeCollection — does not truncate the in-flight key when it is the only evictable key', async () => { + const collectionKey = ONYXKEYS.COLLECTION.TEST_KEY; + const memberKey = `${collectionKey}1`; + + await LocalOnyx.set(memberKey, {id: 1, value: 'orig'}); + expect(LocalOnyxCache.getKeyForEviction()).toBe(memberKey); + + const memberCalls: unknown[] = []; + LocalOnyx.connect({key: memberKey, callback: (value) => memberCalls.push(value)}); + await waitForPromisesToResolve(); + memberCalls.length = 0; + + // The only evictable key is the in-flight one, which is now excluded — so retryOperation + // finds no acceptable key and reports the quota instead of dropping (and truncating) it. + LocalStorageMock.multiMerge = jest.fn(LocalStorageMock.multiMerge).mockRejectedValue(diskFullError); + + await LocalOnyx.mergeCollection(collectionKey, {[memberKey]: {value: 'merged'}} as GenericCollection); + + expect(LocalOnyxCache.get(memberKey)).toEqual({id: 1, value: 'merged'}); + expect(memberCalls.at(-1)).toEqual({id: 1, value: 'merged'}); + expect(memberCalls).not.toContain(undefined); + }); }); describe('afterInit', () => { diff --git a/tests/unit/storage/providers/IDBKeyvalProviderTest.ts b/tests/unit/storage/providers/IDBKeyvalProviderTest.ts index ca34611f9..66e2d2b0d 100644 --- a/tests/unit/storage/providers/IDBKeyvalProviderTest.ts +++ b/tests/unit/storage/providers/IDBKeyvalProviderTest.ts @@ -172,6 +172,11 @@ describe('IDBKeyValProvider', () => { ), ).toEqual(expectedEntries.map((e) => (e[1] === null ? undefined : e[1]))); }); + + it('should insert a new record when key does not exist', async () => { + await IDBKeyValProvider.multiMerge([[ONYXKEYS.TEST_KEY_2, {fresh: true}]]); + expect(await IDBKeyValProvider.getItem(ONYXKEYS.TEST_KEY_2)).toEqual({fresh: true}); + }); }); describe('mergeItem', () => { From 32b493a197f1bc55136e8747081cfa54af7ec48f Mon Sep 17 00:00:00 2001 From: eliran goshen Date: Fri, 5 Jun 2026 16:26:48 +0200 Subject: [PATCH 4/4] Remove redundant comment in retryOperation --- lib/OnyxUtils.ts | 2 -- 1 file changed, 2 deletions(-) diff --git a/lib/OnyxUtils.ts b/lib/OnyxUtils.ts index 6c180b983..c88b2e170 100644 --- a/lib/OnyxUtils.ts +++ b/lib/OnyxUtils.ts @@ -862,8 +862,6 @@ function retryOperation( Logger.logInfo(`Out of storage. Evicting least recently accessed key (${keyForRemoval}) and retrying. Error: ${error}`); reportStorageQuota(error); - // The evicted key is never in-flight (excluded above), so this is a genuine removal — - // subscribers must see the removed state. // @ts-expect-error No overload matches this call. return remove(keyForRemoval).then(() => onyxMethod(defaultParams, nextRetryAttempt)); }