Silence the cache-miss diagnostic for keys removed in another tab (web)

lib/Onyx.ts

@@ -78,7 +78,11 @@ function init({
 
                 // Capture the previous cached value before cache.set() so notifyCollection() can diff old vs new per member.
                 const previousValue = isCollectionMember ? cache.get(key) : undefined;
+                // The key stays indexed because this answer can predate a write of ours still on its way to storage, and only indexed keys get deleted by Onyx.clear and collection sweeps.
                 cache.set(key, value);
+                if (value === null || value === undefined) {
+                    cache.addRemovedInOtherInstanceKey(key);
+                }
 
                 if (isCollectionMember && collectionKey) {
                     let batch = collectionBatches.get(collectionKey);

lib/OnyxCache.ts

@@ -48,6 +48,9 @@ class OnyxCache {
     /** A list of keys where a nullish value has been fetched from storage before, but the key still exists in cache */
     private nullishStorageKeys: Set<OnyxKey>;
 
+    /** Keys another instance removed, which explains why the cache has no value for them. They still count as uncached, unlike nullish keys. */
+    private removedInOtherInstanceKeys: Set<OnyxKey>;
+
     /** A map of cached values */
     private storageMap: Record<OnyxKey, OnyxValue<OnyxKey>>;
 
@@ -72,6 +75,7 @@ class OnyxCache {
     constructor() {
         this.storageKeys = new Set();
         this.nullishStorageKeys = new Set();
+        this.removedInOtherInstanceKeys = new Set();
         this.storageMap = {};
         this.pendingPromises = new Map();
         this.collectionSnapshots = new Map();
@@ -87,6 +91,8 @@ class OnyxCache {
             'addNullishStorageKey',
             'hasNullishStorageKey',
             'clearNullishStorageKeys',
+            'addRemovedInOtherInstanceKey',
+            'hasRemovedInOtherInstanceKey',
             'set',
             'drop',
             'merge',
@@ -153,6 +159,22 @@ class OnyxCache {
         this.nullishStorageKeys = new Set();
     }
 
+    /** Records that another instance removed this key */
+    addRemovedInOtherInstanceKey(key: OnyxKey): void {
+        this.removedInOtherInstanceKeys.add(key);
+    }
+
+    /** Whether another instance removed this key and it hasn't changed here since */
+    hasRemovedInOtherInstanceKey(key: OnyxKey): boolean {
+        return this.removedInOtherInstanceKeys.has(key);
+    }
+
+    /** A write here replaces what the cache knew about the key being empty, including another instance's removal */
+    private forgetEmptyMarks(key: OnyxKey): void {
+        this.nullishStorageKeys.delete(key);
+        this.removedInOtherInstanceKeys.delete(key);
+    }
+
     /** Check whether cache has data for the given key */
     hasCacheForKey(key: OnyxKey): boolean {
         return this.storageMap[key] !== undefined || this.hasNullishStorageKey(key);
@@ -172,7 +194,7 @@ class OnyxCache {
 
         // When a key is explicitly set in cache, we can remove it from the list of nullish keys,
         // since it will either be set to a non nullish value or removed from the cache completely.
-        this.nullishStorageKeys.delete(key);
+        this.forgetEmptyMarks(key);
 
         const collectionKey = OnyxKeys.getCollectionKey(key);
         const oldValue = this.storageMap[key];
@@ -210,6 +232,7 @@ class OnyxCache {
         }
 
         this.storageKeys.delete(key);
+        this.removedInOtherInstanceKeys.delete(key);
         OnyxKeys.deregisterMemberKey(key);
     }
 
@@ -248,7 +271,7 @@ class OnyxCache {
                     affectedCollections.add(collectionKey);
                 }
             } else {
-                this.nullishStorageKeys.delete(key);
+                this.forgetEmptyMarks(key);
 
                 const existing = this.storageMap[key];
 
@@ -319,7 +342,7 @@ class OnyxCache {
                     affectedCollections.add(collectionKey);
                 }
             } else {
-                this.nullishStorageKeys.delete(key);
+                this.forgetEmptyMarks(key);
 
                 // Per-key merge instead of spreading the entire storageMap
                 const existing = this.storageMap[key];

lib/OnyxSubscriptionManager.ts

@@ -19,7 +19,7 @@ type GenericListener = (value: unknown, key: OnyxKey) => void;
 const reportedCacheMisses = new Set<OnyxKey>();
 
 function reportCacheMissIfStorageHasValue(key: OnyxKey): void {
-    if (!cache.getAllKeys().has(key) || OnyxKeys.isRamOnlyKey(key) || cache.hasNullishStorageKey(key)) {
+    if (!cache.getAllKeys().has(key) || OnyxKeys.isRamOnlyKey(key) || cache.hasNullishStorageKey(key) || cache.hasRemovedInOtherInstanceKey(key)) {
         return;
     }
 

tests/unit/OnyxSubscriptionManagerTest.ts

@@ -28,6 +28,7 @@ const MEMBER_2 = `${COLLECTION}2`;
 
 Onyx.init({
     keys: ONYXKEYS,
+    shouldSyncMultipleInstances: true,
 });
 
 beforeEach(() => Onyx.clear());
@@ -354,6 +355,13 @@ describe('OnyxSubscriptionManager', () => {
             cache.setAllKeys([key]);
         };
 
+        // The handler Onyx.init registered for changes another tab wrote to storage.
+        const getSyncCallback = (): ((pairs: Array<[OnyxKey, unknown]>) => void) => {
+            const syncCallback = (StorageMock.keepInstancesSync as jest.Mock).mock.calls.at(-1)?.[0] as ((pairs: Array<[OnyxKey, unknown]>) => void) | undefined;
+            expect(syncCallback).toBeDefined();
+            return syncCallback as (pairs: Array<[OnyxKey, unknown]>) => void;
+        };
+
         it('should report once when the cache has no value but storage does', async () => {
             await StorageMock.setItem(ONYXKEYS.TEST_KEY, 'from-storage');
             forgetValueButKeepKey(ONYXKEYS.TEST_KEY);
@@ -426,6 +434,90 @@ describe('OnyxSubscriptionManager', () => {
             expect(logHmmmSpy).toHaveBeenCalledTimes(1);
         });
 
+        it('should not read storage or report for a key another tab removed', async () => {
+            const syncCallback = getSyncCallback();
+            const getItemMock = jest.mocked(StorageMock.getItem);
+
+            await Onyx.set(MEMBER_1, {id: 1});
+            await StorageMock.removeItem(MEMBER_1);
+            getItemMock.mockClear();
+            syncCallback([[MEMBER_1, undefined]]);
+
+            expect(onyxSubscriptionManager.getState(MEMBER_1)).toBeUndefined();
+            await waitForPromisesToResolve();
+
+            expect(getItemMock).not.toHaveBeenCalled();
+            expect(logHmmmSpy).not.toHaveBeenCalled();
+        });
+
+        it('should still report a real miss on a collection after another tab removed one of its members', async () => {
+            const syncCallback = getSyncCallback();
+
+            await Onyx.set(MEMBER_1, {id: 1});
+            await StorageMock.removeItem(MEMBER_1);
+            syncCallback([[MEMBER_1, undefined]]);
+            onyxSubscriptionManager.getState(MEMBER_1);
+            await waitForPromisesToResolve();
+
+            await StorageMock.setItem(MEMBER_2, {id: 2});
+            forgetValueButKeepKey(MEMBER_2);
+            onyxSubscriptionManager.getState(MEMBER_2);
+            await waitForPromisesToResolve();
+
+            expect(logHmmmSpy).toHaveBeenCalledTimes(1);
+            expect(logHmmmSpy).toHaveBeenCalledWith(expect.stringContaining(COLLECTION));
+        });
+
+        it('should not report when another tab removes a key and writes it back before this tab reads it', async () => {
+            // Given another tab removed a key and this tab applied that removal
+            const syncCallback = getSyncCallback();
+            await Onyx.set(ONYXKEYS.TEST_KEY, 'first');
+            await StorageMock.removeItem(ONYXKEYS.TEST_KEY);
+            syncCallback([[ONYXKEYS.TEST_KEY, undefined]]);
+
+            // When that tab writes the key back, and this tab reads it before the sync for that write arrives
+            await StorageMock.setItem(ONYXKEYS.TEST_KEY, 'second');
+            onyxSubscriptionManager.getState(ONYXKEYS.TEST_KEY);
+            await waitForPromisesToResolve();
+
+            // Then nothing is reported, because nothing was lost
+            expect(logHmmmSpy).not.toHaveBeenCalled();
+        });
+
+        it('should report a key lost after another tab removed it and it got a value again', async () => {
+            // Given a key another tab removed, which then got a value again
+            const syncCallback = getSyncCallback();
+            await Onyx.set(ONYXKEYS.TEST_KEY, 'first');
+            await StorageMock.removeItem(ONYXKEYS.TEST_KEY);
+            syncCallback([[ONYXKEYS.TEST_KEY, undefined]]);
+            await Onyx.set(ONYXKEYS.TEST_KEY, 'second');
+
+            // When the cache then has no value for it while storage still does
+            jest.spyOn(cache, 'get').mockReturnValueOnce(undefined);
+            expect(onyxSubscriptionManager.getState(ONYXKEYS.TEST_KEY)).toBeUndefined();
+            await waitForPromisesToResolve();
+
+            // Then this is a real miss, and the old removal doesn't hide it
+            expect(logHmmmSpy).toHaveBeenCalledTimes(1);
+        });
+
+        it('should not report a key whose removal and re-add by another tab straddle a clear that keeps it', async () => {
+            // Given a key another tab removed
+            const syncCallback = getSyncCallback();
+            await Onyx.set(ONYXKEYS.TEST_KEY, 'first');
+            await StorageMock.removeItem(ONYXKEYS.TEST_KEY);
+            syncCallback([[ONYXKEYS.TEST_KEY, undefined]]);
+
+            // When that tab writes it back, and this tab clears while keeping the key before that sync arrives
+            await StorageMock.setItem(ONYXKEYS.TEST_KEY, 'second');
+            await Onyx.clear([ONYXKEYS.TEST_KEY]);
+            onyxSubscriptionManager.getState(ONYXKEYS.TEST_KEY);
+            await waitForPromisesToResolve();
+
+            // Then the pending write-back isn't reported as a lost value
+            expect(logHmmmSpy).not.toHaveBeenCalled();
+        });
+
         it('should not report a key the index does not know, which means init has not loaded it', async () => {
             await StorageMock.setItem(ONYXKEYS.TEST_KEY, 'from-storage');
             cache.drop(ONYXKEYS.TEST_KEY);

tests/unit/onyxCacheTest.tsx

@@ -217,6 +217,33 @@ describe('Onyx', () => {
             });
         });
 
+        describe('keys removed in another instance', () => {
+            it('should not count as cached, unlike a nullish key', () => {
+                // Given a key another instance removed
+                cache.addRemovedInOtherInstanceKey('mockKey');
+
+                // Then it is marked, and reads still treat the key as having nothing cached
+                expect(cache.hasRemovedInOtherInstanceKey('mockKey')).toBe(true);
+                expect(cache.hasCacheForKey('mockKey')).toBe(false);
+            });
+
+            it.each<[string, (onyxCache: typeof OnyxCache) => void]>([
+                ['set', (onyxCache) => onyxCache.set('mockKey', 'value')],
+                ['merge', (onyxCache) => onyxCache.merge({mockKey: {a: 1}})],
+                ['hydrate', (onyxCache) => onyxCache.hydrate({mockKey: 'value'})],
+                ['drop', (onyxCache) => onyxCache.drop('mockKey')],
+            ])('should forget the mark on %s, because the removal no longer explains a missing value', (_name, change) => {
+                // Given a key another instance removed
+                cache.addRemovedInOtherInstanceKey('mockKey');
+
+                // When the key changes state here
+                change(cache);
+
+                // Then the old removal is forgotten
+                expect(cache.hasRemovedInOtherInstanceKey('mockKey')).toBe(false);
+            });
+        });
+
         describe('merge', () => {
             it('should create the value in cache when it does not exist', () => {
                 // Given empty cache

tests/unit/onyxTest.ts

@@ -4616,6 +4616,80 @@ describe('RAM-only keys should not read from storage', () => {
     });
 });
 
+// A sync read can start before this instance's write reaches storage and answer after it. The answer reports the key as removed, while storage ends up holding the write.
+describe('a stale removal synced from another instance', () => {
+    const COLLECTION = ONYX_KEYS.COLLECTION.TEST_KEY;
+    const MEMBER_1 = `${COLLECTION}1`;
+    const MEMBER_2 = `${COLLECTION}2`;
+
+    const writeThenReceiveStaleRemoval = async (key: OnyxKey, value: unknown) => {
+        await Onyx.set(key, value);
+        const syncCallback = (StorageMock.keepInstancesSync as jest.Mock).mock.calls.at(-1)?.[0];
+        syncCallback([[key, undefined]]);
+    };
+
+    beforeEach(async () => {
+        Object.assign(OnyxUtils.getDeferredInitTask(), createDeferredTask());
+        Onyx.init({keys: ONYX_KEYS, shouldSyncMultipleInstances: true});
+        await act(async () => waitForPromisesToResolve());
+
+        // Another key keeps the index non-empty, as in any running app, because getAllKeys lists storage when the index is empty and that would hide a missed key.
+        await Onyx.set(ONYX_KEYS.OTHER_TEST, 'other');
+    });
+
+    afterEach(async () => {
+        await Onyx.clear();
+        await StorageMock.clear();
+    });
+
+    it('should still delete the key on Onyx.clear', async () => {
+        // Given a stored value whose key this instance was told is removed
+        await writeThenReceiveStaleRemoval(ONYX_KEYS.TEST_KEY, 'mine');
+
+        // When this instance clears, as it does on sign-out
+        await Onyx.clear();
+
+        // Then the value doesn't survive into the next session
+        expect(await StorageMock.getItem(ONYX_KEYS.TEST_KEY)).toBeNull();
+    });
+
+    it('should merge into the stored value on mergeCollection instead of overwriting it', async () => {
+        // Given a stored member whose key this instance was told is removed
+        await writeThenReceiveStaleRemoval(MEMBER_1, {a: 1, b: 2});
+
+        // When a collection merge touches that member
+        await Onyx.mergeCollection(COLLECTION, {[MEMBER_1]: {c: 3}} as GenericCollection);
+
+        // Then the stored fields are kept
+        expect(await StorageMock.getItem(MEMBER_1)).toEqual({a: 1, b: 2, c: 3});
+    });
+
+    it.each<[string, () => Promise<void>]>([
+        ['multiSet', () => Onyx.multiSet({[MEMBER_1]: null})],
+        ['mergeCollection', () => Onyx.mergeCollection(COLLECTION, {[MEMBER_1]: null} as GenericCollection)],
+    ])('should delete the stored value when %s writes null', async (_name, writeNull) => {
+        // Given a stored member whose key this instance was told is removed
+        await writeThenReceiveStaleRemoval(MEMBER_1, {a: 1});
+
+        // When a later write removes it
+        await writeNull();
+
+        // Then the row doesn't come back on the next load
+        expect(await StorageMock.getItem(MEMBER_1)).toBeNull();
+    });
+
+    it('should delete a member that setCollection leaves out', async () => {
+        // Given a stored member whose key this instance was told is removed
+        await writeThenReceiveStaleRemoval(MEMBER_1, {a: 1});
+
+        // When setCollection replaces the collection without it
+        await Onyx.setCollection(COLLECTION, {[MEMBER_2]: {b: 2}} as GenericCollection);
+
+        // Then the row doesn't come back on the next load
+        expect(await StorageMock.getItem(MEMBER_1)).toBeNull();
+    });
+});
+
 describe('get() should prefer cache over stale storage', () => {
     let cache: typeof OnyxCache;
 

tests/unit/useOnyxTest.ts

@@ -5,6 +5,7 @@ import type {UseOnyxSelector} from '../../lib/useOnyx';
 import type GenericCollection from '../utils/GenericCollection';
 
 import Onyx, {useOnyx} from '../../lib';
+import cache from '../../lib/OnyxCache';
 import {resetDeferredInitTask} from '../../lib/OnyxUtils';
 import StorageMock from '../../lib/storage';
 import waitForPromisesToResolve from '../utils/waitForPromisesToResolve';
@@ -25,6 +26,7 @@ Onyx.init({
     keys: ONYXKEYS,
     skippableCollectionMemberIDs: ['skippable-id'],
     ramOnlyKeys: [ONYXKEYS.RAM_ONLY_KEY, ONYXKEYS.COLLECTION.RAM_ONLY_COLLECTION, ONYXKEYS.RAM_ONLY_WITH_INITIAL_VALUE],
+    shouldSyncMultipleInstances: true,
 });
 
 beforeEach(async () => {
@@ -219,6 +221,49 @@ describe('useOnyx', () => {
             expect(result.current[1].status).toEqual('loaded');
         });
 
+        it('should keep reporting loading for a key another tab removed while a merge to it is pending', async () => {
+            const syncCallback = (StorageMock.keepInstancesSync as jest.Mock).mock.calls.at(-1)?.[0];
+            expect(syncCallback).toBeDefined();
+
+            await Onyx.set(ONYXKEYS.TEST_KEY, 'before');
+            await StorageMock.removeItem(ONYXKEYS.TEST_KEY);
+            syncCallback([[ONYXKEYS.TEST_KEY, undefined]]);
+            Onyx.merge(ONYXKEYS.TEST_KEY, 'after');
+
+            const {result} = renderHook(() => useOnyx(ONYXKEYS.TEST_KEY));
+
+            // Same as after a local removal: the pending merge keeps the hook loading until it lands.
+            expect(result.current[0]).toBeUndefined();
+            expect(result.current[1].status).toEqual('loading');
+
+            await act(async () => waitForPromisesToResolve());
+
+            expect(result.current[0]).toEqual('after');
+            expect(result.current[1].status).toEqual('loaded');
+        });
+
+        it('should keep reporting loading for a known-empty key another tab removed while a merge to it is pending', async () => {
+            const syncCallback = (StorageMock.keepInstancesSync as jest.Mock).mock.calls.at(-1)?.[0];
+            expect(syncCallback).toBeDefined();
+
+            // Given a key this tab knows is empty, which another tab then removes
+            cache.addNullishStorageKey(ONYXKEYS.TEST_KEY);
+            syncCallback([[ONYXKEYS.TEST_KEY, undefined]]);
+
+            // When a merge to it is pending as a subscriber mounts
+            Onyx.merge(ONYXKEYS.TEST_KEY, 'after');
+            const {result} = renderHook(() => useOnyx(ONYXKEYS.TEST_KEY));
+
+            // Then the subscriber waits for the merge instead of seeing a loaded key with no value
+            expect(result.current[0]).toBeUndefined();
+            expect(result.current[1].status).toEqual('loading');
+
+            await act(async () => waitForPromisesToResolve());
+
+            expect(result.current[0]).toEqual('after');
+            expect(result.current[1].status).toEqual('loaded');
+        });
+
         it('should initially return undefined and then return cached value after multiple merge operations', async () => {
             Onyx.merge(ONYXKEYS.TEST_KEY, 'test1');
             Onyx.merge(ONYXKEYS.TEST_KEY, 'test2');