Skip to content

Batch collection member removals and coalesce cross-tab sync events #820

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

Open
elirangoshen wants to merge 3 commits into
base: main
Choose a base branch
from
+274 −24
Open

Batch collection member removals and coalesce cross-tab sync events #820

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
33dd460
Batch collection member removals and coalesce cross-tab sync events
elirangoshen Aug 10, 2026
9ecff96
Regenerate API docs for prepareKeyValuePairsForStorage change
elirangoshen Aug 10, 2026
a37d06b
Add regression test: cached-only member removal still notifies member…
elirangoshen Aug 10, 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
11 changes: 6 additions & 5 deletions API-INTERNAL.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,10 +111,11 @@ provider) once so it&#39;s visible, then bounded retry without eviction.</li>
<dt><a href="#broadcastUpdate">broadcastUpdate()</a></dt>
<dd><p>Notifies subscribers and writes current value to cache</p>
</dd>
<dt><a href="#prepareKeyValuePairsForStorage">prepareKeyValuePairsForStorage()</a> ⇒</dt>
<dt><a href="#prepareKeyValuePairsForStorage">prepareKeyValuePairsForStorage()</a></dt>
<dd><p>Storage expects array like: [[&quot;@MyApp_user&quot;, value_1], [&quot;@MyApp_key&quot;, value_2]]
This method transforms an object like {&#39;@MyApp_user&#39;: myUserValue, &#39;@MyApp_key&#39;: myKeyValue}
to an array of key-value pairs in the above format and removes key-value pairs that are being set to null</p>
to an array of key-value pairs in the above format, and collects the keys of null values into
<code>keysToRemove</code> for the caller to delete as one batch (cache drop + notification + batched storage removal).</p>
</dd>
<dt><a href="#mergeChanges">mergeChanges(changes, existingValue)</a></dt>
<dd><p>Merges an array of changes with an existing value or creates a single change.</p>
Expand Down Expand Up @@ -376,13 +377,13 @@ Notifies subscribers and writes current value to cache
**Kind**: global function
<a name="prepareKeyValuePairsForStorage"></a>

## prepareKeyValuePairsForStorage() ⇒
## prepareKeyValuePairsForStorage()
Storage expects array like: [["@MyApp_user", value_1], ["@MyApp_key", value_2]]
This method transforms an object like {'@MyApp_user': myUserValue, '@MyApp_key': myKeyValue}
to an array of key-value pairs in the above format and removes key-value pairs that are being set to null
to an array of key-value pairs in the above format, and collects the keys of null values into
`keysToRemove` for the caller to delete as one batch (cache drop + notification + batched storage removal).

**Kind**: global function
**Returns**: an array of key - value pairs <[key, value]>
<a name="mergeChanges"></a>

## mergeChanges(changes, existingValue)
Expand Down
110 changes: 92 additions & 18 deletions lib/OnyxUtils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,12 @@ function resetDiskPressureLogThrottle(): void {

type OnyxMethod = ValueOf<typeof METHOD>;

/** Result of `prepareKeyValuePairsForStorage`: pairs to write and keys whose `null` value marks them for removal. */
type PreparedKeyValuePairs = {
pairs: StorageKeyValuePair[];
keysToRemove: OnyxKey[];
};

// Key/value store of Onyx key and arrays of values to merge
let mergeQueue: Record<OnyxKey, Array<OnyxValue<OnyxKey>>> = {};
let mergeQueuePromise: Record<OnyxKey, Promise<void>> = {};
Expand Down Expand Up @@ -919,21 +925,20 @@ function hasPendingMergeForKey(key: OnyxKey): boolean {
/**
* Storage expects array like: [["@MyApp_user", value_1], ["@MyApp_key", value_2]]
* This method transforms an object like {'@MyApp_user': myUserValue, '@MyApp_key': myKeyValue}
* to an array of key-value pairs in the above format and removes key-value pairs that are being set to null
*
* @return an array of key - value pairs <[key, value]>
* to an array of key-value pairs in the above format, and collects the keys of null values into
* `keysToRemove` for the caller to delete as one batch (cache drop + notification + batched storage removal).
*/
function prepareKeyValuePairsForStorage(
data: Record<OnyxKey, OnyxInput<OnyxKey>>,
shouldRemoveNestedNulls?: boolean,
replaceNullPatches?: MultiMergeReplaceNullPatches,
isProcessingCollectionUpdate?: boolean,
): StorageKeyValuePair[] {
): PreparedKeyValuePairs {
const pairs: StorageKeyValuePair[] = [];
const keysToRemove: OnyxKey[] = [];

for (const [key, value] of Object.entries(data)) {
if (value === null) {
remove(key, isProcessingCollectionUpdate);
keysToRemove.push(key);
continue;
}

Expand All @@ -944,7 +949,7 @@ function prepareKeyValuePairsForStorage(
}
}

return pairs;
return {pairs, keysToRemove};
}

/**
Expand Down Expand Up @@ -1408,7 +1413,7 @@ function multiSetWithRetry(data: OnyxMultiSetInput, retryAttempt?: number): Prom
}, {});
}

const keyValuePairsToSet = OnyxUtils.prepareKeyValuePairsForStorage(newData, true);
const {pairs: keyValuePairsToSet, keysToRemove} = OnyxUtils.prepareKeyValuePairsForStorage(newData, true);

// Group collection members by their parent collection key so each collection can be notified
// via a single batched keysChanged() call instead of one keyChanged() per member. For each
Expand Down Expand Up @@ -1456,6 +1461,27 @@ function multiSetWithRetry(data: OnyxMultiSetInput, retryAttempt?: number): Prom
}
}

// Null keys join the same per-collection batches (as undefined) and are deleted from storage
// in one batched call below, so cross-tab sync raises a single event instead of one per key.
for (const key of keysToRemove) {
const previousValue = cache.get(key);
cache.drop(key);

const collectionKey = OnyxKeys.getCollectionKey(key);
if (collectionKey && OnyxKeys.isCollectionMemberKey(collectionKey, key)) {
let batch = collectionBatches.get(collectionKey);
if (!batch) {
batch = {partial: {}, previous: {}};
collectionBatches.set(collectionKey, batch);
}
batch.partial[key] = undefined;
batch.previous[key] = previousValue;
} else if (!retryAttempt) {
// Skip subscriber notification on retry — already notified on attempt 0.
keyChanged(key, undefined);
}
}

// One keysChanged() per collection — fires each collection-level subscriber once and lets
// keysChanged() internally decide which individual member subscribers need notification.
// Skip on retry — already notified on attempt 0 (see same-reason comment above).
Expand All @@ -1470,10 +1496,16 @@ function multiSetWithRetry(data: OnyxMultiSetInput, retryAttempt?: number): Prom
// Filter out the RAM-only key value pairs, as they should not be saved to storage
return !OnyxKeys.isRamOnlyKey(key);
});
const keysToRemoveFromStorage = keysToRemove.filter((key) => !OnyxKeys.isRamOnlyKey(key));

const inFlightKeys = new Set<OnyxKey>(keyValuePairsToSet.map(([key]) => key));

return Storage.multiSet(keyValuePairsToStore)
const storagePromises = [Storage.multiSet(keyValuePairsToStore)];
if (keysToRemoveFromStorage.length > 0) {
storagePromises.push(Storage.removeItems(keysToRemoveFromStorage));
}

return Promise.all(storagePromises)
.then(() => StorageCircuitBreaker.recordWriteSuccess())
.catch((error) => OnyxUtils.retryOperation(error, multiSetWithRetry, newData, retryAttempt, inFlightKeys))
.then(() => {
Expand Down Expand Up @@ -1534,10 +1566,14 @@ function setCollectionWithRetry<TKey extends CollectionKeyBase>({collectionKey,
mutableCollection[key] = null;
}

const keyValuePairs = OnyxUtils.prepareKeyValuePairsForStorage(mutableCollection, true, undefined, true);
const {pairs: keyValuePairs, keysToRemove: removalCandidates} = OnyxUtils.prepareKeyValuePairsForStorage(mutableCollection, true);
// Removals of keys that are neither cached nor persisted are no-ops and skipped.
const keysToRemove = removalCandidates.filter((key) => cache.get(key) !== undefined || persistedKeys.has(key));
// Snapshot before cache mutations so keysChanged() can diff removed members.
const previousCollection = OnyxUtils.getCachedCollection(collectionKey);

for (const [key, value] of keyValuePairs) cache.set(key, value);
for (const key of keysToRemove) cache.drop(key);

// Skip subscriber notification on retry — already notified on attempt 0.
// Collection-root subscribers re-fire on every keysChanged by contract.
Expand All @@ -1553,7 +1589,13 @@ function setCollectionWithRetry<TKey extends CollectionKeyBase>({collectionKey,

const inFlightKeys = new Set<OnyxKey>(keyValuePairs.map(([key]) => key));

return Storage.multiSet(keyValuePairs)
// One batched removal = one cross-tab sync event instead of one per key.
const storagePromises = [Storage.multiSet(keyValuePairs)];
if (keysToRemove.length > 0) {
storagePromises.push(Storage.removeItems(keysToRemove));
}
Comment on lines +1592 to +1596

@mountiny mountiny Aug 10, 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.

When removeItems fails, the retry redoes the write but never retries the deletion.

removeItems is now part of the same promise as multiSet, so if the delete fails, the whole setCollection is retried. But by then the keys are already gone from the cache, so the filter on line 1571 comes back empty and the delete is never attempted again. Only the multiSet runs a second time, and it had already succeeded.

I saw this with a storage error that Onyx classifies as UNKNOWN: multiSet ran twice, removeItems ran once, and the row was left behind in storage even though getAllKeys() says it is gone.

The leftover row is not new. On main a failed removeItem was dropped too, and this PR at least stops it from becoming an unhandled rejection. The pointless second write is the new part. partialSetCollection and multiSet do the same thing.

Would it be safer to give the removal its own .catch() so a failed delete does not re-run the whole write?


return Promise.all(storagePromises)
.then(() => StorageCircuitBreaker.recordWriteSuccess())
.catch((error) => OnyxUtils.retryOperation(error, setCollectionWithRetry, {collectionKey, collection}, retryAttempt, inFlightKeys))
.then(() => {
Expand Down Expand Up @@ -1612,10 +1654,14 @@ function mergeCollectionWithPatches<TKey extends CollectionKeyBase>(

return getAllKeys()
.then((persistedKeys) => {
// Split to keys that exist in storage and keys that don't
// Split to keys that exist in storage and keys that don't. Null members are collected
// for one batched removal below; nulls that are neither cached nor persisted are no-ops and skipped.
const keysToRemove: OnyxKey[] = [];
const keys = resultCollectionKeys.filter((key) => {
if (resultCollection[key] === null) {
remove(key, isProcessingCollectionUpdate);
if (cache.get(key) !== undefined || persistedKeys.has(key)) {
keysToRemove.push(key);
}
return false;
}
return true;
Expand Down Expand Up @@ -1658,11 +1704,11 @@ function mergeCollectionWithPatches<TKey extends CollectionKeyBase>(
// When (multi-)merging the values with the existing values in storage,
// we don't want to remove nested null values from the data that we pass to the storage layer,
// because the storage layer uses them to remove nested keys from storage natively.
const keyValuePairsForExistingCollection = prepareKeyValuePairsForStorage(existingKeyCollection, false, mergeReplaceNullPatches);
const {pairs: keyValuePairsForExistingCollection} = prepareKeyValuePairsForStorage(existingKeyCollection, false, mergeReplaceNullPatches);

// We can safely remove nested null values when using (multi-)set,
// because we will simply overwrite the existing values in storage.
const keyValuePairsForNewCollection = prepareKeyValuePairsForStorage(newCollection, true);
const {pairs: keyValuePairsForNewCollection} = prepareKeyValuePairsForStorage(newCollection, true);

// finalMergedCollection contains all the keys that were merged, without the keys of incompatible updates
const finalMergedCollection = {
Expand All @@ -1688,15 +1734,33 @@ function mergeCollectionWithPatches<TKey extends CollectionKeyBase>(
// ensuring subscribers still reflect the merged data even if the subsequent storage
// write fails.
const previousCollection = getCachedCollection(collectionKey, existingKeys);

// Removed members join the same keysChanged() batch (as undefined); previous values
// are snapshotted before the cache drop so keysChanged() can diff them.
const removedPreviousValues: OnyxInputKeyValueMapping = {};
for (const key of keysToRemove) {
removedPreviousValues[key] = cache.get(key);
cache.drop(key);
Comment on lines +1741 to +1743

@chatgpt-codex-connector chatgpt-codex-connector Bot Aug 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Preserve removal keys when retrying batched deletes

When Storage.removeItems(keysToRemove) rejects with a retryable error, retryOperation() re-enters this method, but this loop has already dropped the removed members from the cache. On the retry, keysToRemove is recomputed from cache.get(key) !== undefined || persistedKeys.has(key), and getAllKeys() normally returns the cache-backed key set, so the failed removal is filtered out and never retried; the stale member remains in persistent storage and can reappear after reload. Preserve the original removal list across retries, or avoid deriving retry deletions from the cache after it has been mutated.

Useful? React with 👍 / 👎.

}
Comment on lines +1738 to +1744

@mountiny mountiny Aug 10, 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.

A key that gets deleted here can wipe out a write that came after it.

On main, the deletion happened right away, early in the function. In this PR it moved later, into the block that waits for Storage.multiGet to finish. That wait is real whenever any member of the collection is not in the cache yet.

So if something writes to one of the deleted keys during that wait, the deletion runs afterwards and throws that write away.

Here is the repro. Put test_A and test_B in storage, clear them from the cache so the slow path runs, then fire both calls in the same tick:

const removal = Onyx.mergeCollection('test_', {test_A: null, test_B: {b: 2}});
const concurrent = Onyx.merge('test_A', {y: 2});   // this one is called second
await Promise.all([removal, concurrent]);
  • On main, test_A ends up as {a: 1, y: 2} in the cache and in storage.
  • On this branch, test_A ends up gone from both.

The second call should win, since it was made last. Could the cache.drop() move back up into the first .then(), and only the keysChanged() batch stay down here?


cache.merge(finalMergedCollection);
// Skip subscriber notification on retry — already notified on attempt 0.
// Collection-root subscribers re-fire on every keysChanged by contract.
if (!retryAttempt) {
keysChanged(collectionKey, finalMergedCollection, previousCollection);
const partialForNotify = keysToRemove.length > 0 ? {...finalMergedCollection, ...Object.fromEntries(keysToRemove.map((key) => [key, undefined]))} : finalMergedCollection;
const previousForNotify = keysToRemove.length > 0 ? {...previousCollection, ...removedPreviousValues} : previousCollection;
if (Object.keys(partialForNotify).length > 0) {
keysChanged(collectionKey, partialForNotify, previousForNotify);
}
}

const promises = [];

// One batched removal = one cross-tab sync event instead of one per key.
if (!OnyxKeys.isRamOnlyKey(collectionKey) && keysToRemove.length > 0) {
promises.push(Storage.removeItems(keysToRemove));
}

// 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).
Expand Down Expand Up @@ -1777,10 +1841,14 @@ function partialSetCollection<TKey extends CollectionKeyBase>({collectionKey, co
return getAllKeys().then((persistedKeys) => {
const mutableCollection: OnyxInputKeyValueMapping = {...resultCollection};
const existingKeys = resultCollectionKeys.filter((key) => persistedKeys.has(key));
const {pairs: keyValuePairs, keysToRemove: removalCandidates} = prepareKeyValuePairsForStorage(mutableCollection, true);
// Removals of keys that are neither cached nor persisted are no-ops and skipped.
const keysToRemove = removalCandidates.filter((key) => cache.get(key) !== undefined || persistedKeys.has(key));
// Snapshot before cache mutations so keysChanged() can diff removed members.
const previousCollection = getCachedCollection(collectionKey, existingKeys);
Comment thread
elirangoshen marked this conversation as resolved.
const keyValuePairs = prepareKeyValuePairsForStorage(mutableCollection, true, undefined, true);

for (const [key, value] of keyValuePairs) cache.set(key, value);
for (const key of keysToRemove) cache.drop(key);

// Skip subscriber notification on retry — already notified on attempt 0.
// Collection-root subscribers re-fire on every keysChanged by contract.
Expand All @@ -1795,7 +1863,13 @@ function partialSetCollection<TKey extends CollectionKeyBase>({collectionKey, co

const inFlightKeys = new Set<OnyxKey>(keyValuePairs.map(([key]) => key));

return Storage.multiSet(keyValuePairs)
// One batched removal = one cross-tab sync event instead of one per key.
const storagePromises = [Storage.multiSet(keyValuePairs)];
if (keysToRemove.length > 0) {
storagePromises.push(Storage.removeItems(keysToRemove));
}

return Promise.all(storagePromises)
.then(() => StorageCircuitBreaker.recordWriteSuccess())
.catch((error) => retryOperation(error, partialSetCollection, {collectionKey, collection}, retryAttempt, inFlightKeys))
.then(() => {
Expand Down
18 changes: 17 additions & 1 deletion lib/storage/InstanceSync/index.web.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,10 @@ const InstanceSync = {
init: (onStorageKeysChanged: OnStorageKeysChanged, store: StorageProvider<unknown>) => {
storage = store;

// Coalesce storage events into one dispatch per tick: a per-key sender would otherwise re-run
// the whole notification pipeline once per key and can flood the receiving tab into unresponsiveness.
let pendingSyncKeys: Set<OnyxKey> | null = null;

// This listener will only be triggered by events coming from other tabs
global.addEventListener('storage', (event) => {
// Ignore events that don't originate from the SYNC_ONYX logic
Expand All @@ -109,7 +113,19 @@ const InstanceSync = {

const onyxKeys = parseSyncOnyxStorageEventValue(event.newValue);

storage.multiGet(onyxKeys).then((pairs) => onStorageKeysChanged(pairs));
if (pendingSyncKeys) {
for (const onyxKey of onyxKeys) {
pendingSyncKeys.add(onyxKey);
}
return;
}

pendingSyncKeys = new Set(onyxKeys);
setTimeout(() => {
const keys = Array.from(pendingSyncKeys ?? []);
pendingSyncKeys = null;
storage.multiGet(keys).then((pairs) => onStorageKeysChanged(pairs));
}, 0);
});
},
setItem: raiseStorageSyncEvent,
Expand Down
Loading
Loading