Skip to content

Fix cache-miss diagnostic for undefined keys #863

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
chuckdries merged 2 commits into from
Nov 3, 2026
+32 −3
Merged

Fix cache-miss diagnostic for undefined keys #863

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d27c55f
Fix getState for undefined keys and drop the dead nullish fallback in…
LukasMod Oct 23, 2026
5907e38
Assert storage in the lost-value merge test and explain the parked read
LukasMod Oct 30, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions lib/Onyx.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -791,7 +791,7 @@ function exportState(): Promise<Record<OnyxKey, OnyxValue<OnyxKey>>> {
* @returns The current value, or `undefined` if the key has none.
*/
function get<TKey extends OnyxKey>(key: TKey): Promise<OnyxValue<TKey>> {
return OnyxUtils.afterInit(() => Promise.resolve((onyxSubscriptionManager.getState(key) ?? undefined) as OnyxValue<TKey>));
return OnyxUtils.afterInit(() => Promise.resolve(onyxSubscriptionManager.getState(key) as OnyxValue<TKey>));
}

/**
Expand All @@ -813,7 +813,7 @@ function get<TKey extends OnyxKey>(key: TKey): Promise<OnyxValue<TKey>> {
*/
function multiGet<const Keys extends readonly OnyxKey[]>(keys: Keys): Promise<{[Index in keyof Keys]: OnyxValue<Keys[Index]>}> {
// map() widens the key tuple to an array, so the per-slot value types survive only through the cast.
return OnyxUtils.afterInit(() => Promise.resolve(keys.map((key) => onyxSubscriptionManager.getState(key) ?? undefined) as {[Index in keyof Keys]: OnyxValue<Keys[Index]>}));
return OnyxUtils.afterInit(() => Promise.resolve(keys.map((key) => onyxSubscriptionManager.getState(key)) as {[Index in keyof Keys]: OnyxValue<Keys[Index]>}));
}

const Onyx = {
Expand Down
2 changes: 1 addition & 1 deletion lib/OnyxSubscriptionManager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ type GenericListener = (value: unknown, key: OnyxKey) => void;
const reportedCacheMisses = new Set<OnyxKey>();

function reportCacheMissIfStorageHasValue(key: OnyxKey): void {
if (OnyxKeys.isRamOnlyKey(key) || !cache.getAllKeys().has(key) || cache.hasNullishStorageKey(key)) {
if (!cache.getAllKeys().has(key) || OnyxKeys.isRamOnlyKey(key) || cache.hasNullishStorageKey(key)) {

@TaduJR TaduJR Oct 29, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This only works while undefined isn't in the key index. Onyx.set(undefined, 1) adds it before it throws, and after that getState(undefined) throws for every reader. It also returns the value of a key named 'undefined'. Checking the key where it enters Onyx, as update does, fixes both:

// OnyxKeys.ts
function isValidKey(key: unknown): key is string {
    return typeof key === 'string';
}

// Top of Onyx.set and Onyx.merge
if (!OnyxKeys.isValidKey(key)) {
    Logger.logInfo(`Invalid ${typeof key} key provided in Onyx set. Key must be of type string. Skipping this operation.`);
    return Promise.resolve();
}

// Top of getState, which every read goes through
if (!OnyxKeys.isValidKey(key)) {
    return undefined as OnyxValue<TKey>;
}

@LukasMod LukasMod Oct 30, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I reproduced it. set(undefined, 1) adds the key to the index before it throws, and after that every reader and clear throw.

I'd rather stay with TypeScript as the protection here. The type system already rejects every call that could trigger this, so reaching it takes a cast or a plain JS caller. Production logs show zero Onyx-frame lastIndexOf or startsWith errors in 30 days, and the existing guard of the same kind in update hasn't fired on the days I checked either.

Not sure we need an extra guard as a net for JS callers. I would skip it as its already guarded. WDYT?

@TaduJR TaduJR Nov 3, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree

return;
}

Expand Down
29 changes: 29 additions & 0 deletions tests/unit/OnyxSubscriptionManagerTest.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import cache from '../../lib/OnyxCache';
import * as Logger from '../../lib/Logger';
import OnyxKeys from '../../lib/OnyxKeys';
import StorageMock from '../../lib/storage';
import createDeferredTask from '../../lib/createDeferredTask';
import waitForPromisesToResolve from '../utils/waitForPromisesToResolve';

// We need access to some internal properties of `onyxSubscriptionManager` during the tests but they are private,
Expand Down Expand Up @@ -131,6 +132,10 @@ describe('OnyxSubscriptionManager', () => {

expect(onyxSubscriptionManager.getState(COLLECTION)).toBe(collectionData);
});

it('should return undefined without throwing for an undefined key', () => {
expect(onyxSubscriptionManager.getState(undefined as unknown as OnyxKey)).toBeUndefined();
});
});

describe('collection routing on notifyKey', () => {
Expand Down Expand Up @@ -397,6 +402,30 @@ describe('OnyxSubscriptionManager', () => {
expect(logHmmmSpy).not.toHaveBeenCalled();
});

it('should report when a merge lands on the lost value before storage answers', async () => {
await StorageMock.setItem(ONYXKEYS.TEST_KEY, {a: 1, b: 2});
forgetValueButKeepKey(ONYXKEYS.TEST_KEY);

// Park the diagnostic's storage read so the merge below lands on the empty cache before it answers.
const getItemMock = jest.mocked(StorageMock.getItem);
const readStorage = getItemMock.getMockImplementation();
const storageAnswer = createDeferredTask();
getItemMock.mockImplementationOnce((key) => {
const storedValue = readStorage?.(key);
return storageAnswer.promise.then(() => storedValue);
});

onyxSubscriptionManager.getState(ONYXKEYS.TEST_KEY);
await Onyx.merge(ONYXKEYS.TEST_KEY, {c: 3});
storageAnswer.resolve();
await waitForPromisesToResolve();

// The merge built on nothing, so {a, b} is gone from cache and storage. The log is the only trace.
expect(cache.get(ONYXKEYS.TEST_KEY)).toEqual({c: 3});
expect(await StorageMock.getItem(ONYXKEYS.TEST_KEY)).toEqual({c: 3});
expect(logHmmmSpy).toHaveBeenCalledTimes(1);
});

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);
Expand Down
Loading