[Onyx] Normalize mergeCollectionWithPatches to cache-first write ordering

[fabioh8010]

Coming from #87862 (comment).

Issue

Onyx relies on a cache-first / storage-second invariant: every write method updates the in-memory cache synchronously before issuing the storage write. This invariant is what makes the session resilient to storage failures — when storage fails, the cache still has the correct value, subscribers fire with the right data, and the user's session is unaffected.

Every Onyx write method follows this pattern except mergeCollectionWithPatches. It is the only outlier where storage promises are pushed first and the cache update is bundled into an asynchronous chain that runs in parallel with storage.

Cache-vs-storage ordering across all Onyx write methods

Method	Cache write	Storage call	Order
setWithRetry	OnyxUtils.ts:1327 (broadcastUpdate → cache.set)	OnyxUtils.ts:1340 (Storage.setItem)	✅ Cache first
multiSetWithRetry	OnyxUtils.ts:1395 / 1408 (cache.set in loop)	OnyxUtils.ts:1425 (Storage.multiSet)	✅ Cache first
applyMerge (web) — used by Onyx.merge	OnyxMerge/index.ts:22 (broadcastUpdate → cache.set)	OnyxMerge/index.ts:33 (Storage.setItem)	✅ Cache first
applyMerge (native)	OnyxMerge/index.native.ts:30	OnyxMerge/index.native.ts:42 (Storage.mergeItem)	✅ Cache first
setCollectionWithRetry	OnyxUtils.ts:1488 (cache.set in loop)	OnyxUtils.ts:1498 (Storage.multiSet)	✅ Cache first
partialSetCollection	OnyxUtils.ts:1697	OnyxUtils.ts:1706 (Storage.multiSet)	✅ Cache first
clear	Onyx.ts:349 (cache.set) / Onyx.ts:385 (cache.drop)	Onyx.ts:386 / 388	✅ Cache first
mergeCollectionWithPatches	OnyxUtils.ts:1632 (cache.merge inside promiseUpdate)	OnyxUtils.ts:1618 / 1623 (Storage.multiMerge / multiSet)	⚠️ Inverted

Affected code (lib/OnyxUtils.ts:1519-1651)

// Async — depends on get(key) for each existing key, which is cache-first but falls back to storage on miss
const previousCollectionPromise = Promise.all(
    existingKeys.map((key) => get(key).then((value) => [key, value])),
).then(Object.fromEntries);

const promises = [];

// Storage writes pushed to a promises array — fire in parallel
if (!OnyxKeys.isRamOnlyKey(collectionKey) && keyValuePairsForExistingCollection.length > 0) {
    promises.push(Storage.multiMerge(keyValuePairsForExistingCollection));
}
if (!OnyxKeys.isRamOnlyKey(collectionKey) && keyValuePairsForNewCollection.length > 0) {
    promises.push(Storage.multiSet(keyValuePairsForNewCollection));
}

// Cache update bundled into a chain that depends on the async previousCollectionPromise
const promiseUpdate = previousCollectionPromise.then((previousCollection) => {
    cache.merge(finalMergedCollection);
    keysChanged(collectionKey, finalMergedCollection, previousCollection);
});

// Return-value chain reads "storage first, then cache"
return Promise.all(promises)
    .catch((error) => retryOperation(...))
    .then(() => {
        sendActionToDevTools(METHOD.MERGE_COLLECTION, undefined, resultCollection);
        return promiseUpdate;
    });

The structure is parallel rather than strictly sequential: the cache update is gated on previousCollectionPromise (which uses get(key) for each existing key — cache-first and usually fast), and Storage.multiMerge / Storage.multiSet fire independently via Promise.all(promises). In the common warm-cache case, the cache update wins the race.

But:

• The design intent is "storage first, then cache" — that's what the return-value chain expresses.
• If previousCollectionPromise has a cache miss for any key, get(key) falls back to storage. That storage read can take longer than the storage writes — the cache update can lose the race, leaving subscribers with stale data while storage already succeeded (or failed) silently.
• If storage retries exhaust and retryOperation returns Promise.resolve(), promiseUpdate still fires and the cache catches up eventually — but the timing is fragile, the semantics are inconsistent with every other write method, and reasoning about partial-failure scenarios becomes harder than necessary.

Why this matters

In the IDB-corruption population analyzed in #87862, storage writes fail. Every other write method's cache update has already happened by the time the failure surfaces, so the session is unaffected. For mergeCollectionWithPatches, the cache update can race the storage failure and lose, depending on cache-hit state and timing. This is the only Onyx write path where a storage error can theoretically miss the cache and leave subscribers unaware of the change.

Solution

Update the cache synchronously before issuing storage writes, the same way every other write method does.

Approach

1. Source previousCollection from getCachedCollection(collectionKey, existingKeys) — already called a few lines above at line 1567 for cachedCollectionForExistingKeys. This removes the dependency on previousCollectionPromise.
2. Fire cache.merge(finalMergedCollection) and keysChanged(collectionKey, finalMergedCollection, previousCollection) synchronously before pushing the storage promises.
3. Remove previousCollectionPromise and the promiseUpdate chain.
4. The remaining Promise.all(promises).catch(retryOperation).then(sendActionToDevTools) chain is kept as-is, just without the trailing return promiseUpdate.

Sketch

const previousCollection = getCachedCollection(collectionKey, existingKeys);

// Cache + subscribers synchronously, before any storage call
cache.merge(finalMergedCollection);
keysChanged(collectionKey, finalMergedCollection, previousCollection);

const promises = [];
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,
            mergeCollectionWithPatches,
            {collectionKey, collection: resultCollection, mergeReplaceNullPatches, isProcessingCollectionUpdate},
            retryAttempt,
        ),
    )
    .then(() => {
        sendActionToDevTools(METHOD.MERGE_COLLECTION, undefined, resultCollection);
    });

Test plan

• Unit test: confirm that for a mergeCollection call where Storage.multiMerge rejects, the cache and subscribers still reflect the merged collection (currently fragile, depends on race outcome).
• Unit test: regression — confirm the merged collection values match expectations for both new keys and existing keys.
• Confirm existing mergeCollection tests still pass.
• Verify subscriber notification ordering didn't shift in any user-visible way during integration testing.

[melvin-bot]

@Julesssss, @cristipaval Whoops! This issue is 2 days overdue. Let's get this updated quick!

[elirangoshen]

Hi, Im Eliran from Callstack and I can work on this issue @mountiny

[melvin-bot]

📣 @Ilya0527! 📣
Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork.
Please follow these steps:

1. Make sure you've read and understood the contributing guidelines.
2. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
3. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
4. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.
   
   Format:

Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>