[Onyx] Onyx.merge can resurrect a key that a concurrent Onyx.update deleted

[fabioh8010]

Note: this bug lives in react-native-onyx, not in App — file paths below are relative to that repo. Filing here for visibility since it was found while investigating #97400.

Superseded by Expensify/react-native-onyx#773 (Make OnyxUtils get functions synchronous, WIP) — verified by running the repro below against that branch. Keeping this open to track the gap until that PR lands. Measurements are in the "Does the synchronous get PR fix this?" section.

Problem

A key deleted by Onyx.update can be resurrected — with its pre-deletion contents — by an Onyx.merge whose read was already outstanding when the deletion landed.

This is the residual half of the race fixed in Expensify/react-native-onyx#817. That PR made Onyx.merge re-read the cache at apply time so it stops clobbering concurrent Onyx.update writes, but the re-read only helps when the key is still in the cache. When the concurrent update removes the key, remove() calls cache.drop(), which deletes the entry without marking it nullish, so hasCacheForKey() is false and the merge falls back to the stale valueFromGet it captured before the deletion.

Flagged in review: Expensify/react-native-onyx#817 (comment)

Not a regression from that PR — verified byte-identical behaviour with and without the change. It is the same class of bug, left open.

Reproduction

Verified against react-native-onyx main @ cabb7a67 (v3.0.98).

await Onyx.merge(KEY_A, {item: {id: 'a', stale: 'SHOULD BE GONE'}});
await Onyx.merge(KEY_B, {item: {id: 'b'}});
await waitForPromisesToResolve();

const staleValue = lodashCloneDeep(cache.get(KEY_A));

// Park merge()'s read so the removal below is guaranteed to land first.
const deferredGet = createDeferredTask();
const originalGet = OnyxUtils.get;
jest.spyOn(OnyxUtils, 'get').mockImplementation(((key: OnyxKey) =>
    key === KEY_A ? deferredGet.promise.then(() => staleValue) : originalGet(key)) as typeof OnyxUtils.get);

const mergePromise = Onyx.merge(KEY_A, {item: {childID: '1'}});

// Two keys of the same collection, so this goes through mergeCollectionWithPatches -> remove().
await Onyx.update([
    {onyxMethod: Onyx.METHOD.MERGE, key: KEY_A, value: null},
    {onyxMethod: Onyx.METHOD.MERGE, key: KEY_B, value: {item: {touched: true}}},
]);
await waitForPromisesToResolve();

deferredGet.resolve();
await mergePromise;

Expected: KEY_A stays deleted, or at worst is recreated holding only the merge's own delta.

Actual: both cache and storage come back carrying the data the update deleted.

cache  : {"item":{"id":"a","stale":"SHOULD BE GONE","childID":"1"}}
storage: {"item":{"id":"a","stale":"SHOULD BE GONE","childID":"1"}}

A microtask-depth sweep of the same scenario without the spy shows depth 0 reproducing the full resurrection; at depths 1+ the read happens after the drop, so the key is recreated with the delta alone ({"item":{"childID":"1"}}) — still resurrected, but without the stale fields.

Root cause

Onyx.merge serializes writes to a key through mergeQueue, but only two write paths invalidate it:

Path	Clears mergeQueue[key]?
setWithRetry (lib/OnyxUtils.ts:1306)	yes
multiSetWithRetry (lib/OnyxUtils.ts:1428)	yes
remove (lib/OnyxUtils.ts:758)	no
partialSetCollection (lib/OnyxUtils.ts:1749)	no
mergeCollectionWithPatches (lib/OnyxUtils.ts:1578)	no

Onyx.set explicitly cancels queued merges — the comment at setWithRetry says so: "If Onyx.merge is currently reading the old value from storage, it will then not apply the changes." A deletion coming through Onyx.update should behave the same way, and does not.

cache.drop() (lib/OnyxCache.ts:185) compounds it by deleting from storageMap without calling addNullishStorageKey, so hasCacheForKey() (lib/OnyxCache.ts:143) reports false and the re-read added at lib/Onyx.ts:267 can't help.

Does the synchronous get PR fix this?

Yes. Ran the repro against Expensify/react-native-onyx#773 (feature/onyxutils-get-synchronous @ 1e87b15a):

Scenario	main (v3.0.98)	PR 773
delete first, merge after (microtask depths 0–4)	depth 0: {"item":{"id":"a","stale":"SHOULD BE GONE","childID":"1"}}	{"item":{"childID":"1"}} at every depth
merge first, delete after	—	cache=undefined storage=null
deterministic parked read (above)	{"item":{"id":"a","stale":"SHOULD BE GONE","childID":"1"}}	n/a — cannot be constructed

Stale data never returns on that branch, and cache and storage agree. Both orderings end up correct: whichever call happened last wins. The remaining {"item":{"childID":"1"}} is not a defect — Onyx.update (delete) was called first and Onyx.merge second, so the merge should win and recreate the key.

It works because the PR removes the stale snapshot entirely rather than patching around it. Onyx.merge reads at apply time instead of queue time:

mergeQueuePromise[key] = Promise.resolve().then(() => {
    if (mergeQueue[key] == null) return Promise.resolve();

    // Read the existing value at merge application time (not at queue time) so that
    // any intervening synchronous cache updates (e.g. from mergeCollection) are picked up.
    const existingValue = OnyxUtils.get(key);

with get reduced to a pure cache read:

function get<TKey extends OnyxKey, TValue extends OnyxValue<TKey>>(key: TKey): TValue {
    return cache.get(key) as TValue;
}

There is no captured snapshot left to go stale, so the deletion case is covered even though the hasCacheForKey re-read could not cover it. The PR also drains mergeQueue/mergeQueuePromise inside clear(), closing the same race there.

Two follow-ups for whoever picks this up:

• The ternary at lib/Onyx.ts:267 should collapse into the synchronous read as part of that PR — the two changes touch the same lines and will conflict on rebase.
• The PR drops the storage fallback in get, so correctness depends on the cache holding every key. Only clear() and remove() call cache.drop() on that branch (no LRU eviction path), so the premise holds today, but it is worth an explicit check — if a key ever isn't cached, merge would read undefined and treat a populated key as new.

Proposed fix (only if the synchronous get PR stalls)

Option A (preferred) — invalidate the merge queue in remove(), mirroring setWithRetry:

function remove<TKey extends OnyxKey>(key: TKey, isProcessingCollectionUpdate?: boolean): Promise<void> {
    if (OnyxUtils.hasPendingMergeForKey(key)) {
        delete OnyxUtils.getMergeQueue()[key];
    }
    cache.drop(key);
    ...
}

The in-flight merge then aborts at the existing if (mergeQueue[key] == null) return Promise.resolve() guard in Onyx.merge. Safe with respect to Onyx.merge's own null-merge path, which already deletes mergeQueue[key] (lib/Onyx.ts:285) before calling remove().

Option B — have cache.drop() mark the key nullish. hasCacheForKey() would then return true, cache.get() undefined, and the merge would recreate the key from its delta alone. Only two callers of drop (OnyxUtils.remove and Onyx.clear), both genuine deletions, so it is viable — but it changes get() semantics more broadly, since a dropped key would stop falling through to storage.

Option A is narrower and matches the existing set contract. Either way partialSetCollection and mergeCollectionWithPatches inherit the fix, since both delete members via remove().

Context

• Original bug: Expense - Deleted unreported split reappears when user turns to offline mode. #97400 — deleted split expenses reappearing offline, caused by the sibling race fixed in fix: prevent Onyx.merge from clobbering concurrent Onyx.update writes react-native-onyx#817.
• Fix PR: fix: prevent Onyx.merge from clobbering concurrent Onyx.update writes react-native-onyx#817 (shipped in react-native-onyx v3.0.98).
• Superseding PR: [WIP] Make OnyxUtils get functions synchronous react-native-onyx#773.

[fabioh8010]

@mountiny This probably should be on hold for 1-2 weeks since we expect to post the Onyx.get proposal very soon.