Batch collection member removals and coalesce cross-tab sync events - #820
Batch collection member removals and coalesce cross-tab sync events#820elirangoshen wants to merge 3 commits into
Conversation
Collection writes fanned out null members as one remove() per key, raising one cross-tab SYNC_ONYX event per removed key. On heavy accounts OpenApp nulls ~2k transactionViolations members on every boot, so each refresh flooded the other tab with thousands of single-key events, each triggering a derived-value recompute in its own macrotask until the tab ran out of memory. - prepareKeyValuePairsForStorage now returns the null keys instead of removing them; mergeCollection/multiSet/setCollection/partialSetCollection drop no-op removals (neither cached nor persisted), fold real ones into the single keysChanged batch, and delete them with one Storage.removeItems call. - InstanceSync (web) coalesces incoming SYNC_ONYX storage events into one multiGet + one dispatch per tick, so per-key senders (e.g. tabs on an older bundle during a deploy) can no longer flood the receiving tab.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9ecff96823
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
… subscribers Covers the partialSetCollection path for RAM-only collection members. getAllKeys() returns the cache-augmented key set, so cached-only members are part of existingKeys and their previous values are present in the keysChanged() diff snapshot.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a37d06b5f9
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| for (const key of keysToRemove) { | ||
| removedPreviousValues[key] = cache.get(key); | ||
| cache.drop(key); |
There was a problem hiding this comment.
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 👍 / 👎.
mountiny
left a comment
mountiny
left a comment
There was a problem hiding this comment.
Reviewed this locally on the branch. The core fix looks right and the win is real, so nothing here blocks it.
What I checked and found clean:
npx jest tests/unitpasses (567 tests). The only failing suite isSQLiteProviderTest, which is abetter-sqlite3native binding problem in my environment and has nothing to do with this diff.eslintis clean on all four changed files.- RAM-only keys are guarded on all four write paths.
SQLiteProvider.removeItemschunks by the SQLite variable limit, and the IndexedDB provider does one transaction, so batching a few thousand keys into one call is safe on every platform.keysChanged()does deliverundefinedto member subscribers for a batched removal.- The coalescing in
InstanceSyncuses a fixed one-tick window rather than a debounce, so a steady stream of events cannot delay a dispatch indefinitely. Storage.removeItemsis wired toraiseStorageSyncManyKeysEvent, so the batch really is one cross-tab event.
Two things I could not check locally: the two-tab measurements on a heavy account, and the native platforms.
I left two comments below. The first is a behaviour change I was able to reproduce and I think it is worth fixing. The second is smaller.
A few nits that need no action, just flagging:
multiSetdid not get the same "skip removals that are no-ops" filter the other three paths got.isProcessingCollectionUpdateis now dead. Nothing passes the second argument toremove()any more, so theif (isProcessingCollectionUpdate)branch inkeyChanged()can no longer be reached.setCollectionandpartialSetCollectionnotify withnullfor removed members, whilemergeCollectionandmultiSetnotify withundefined. Same result either way, but it would read better if all four matched.- A
mergeCollectionmade up entirely of nulls that are all no-ops now notifies collection subscribers zero times, wheremainnotified them once. That is where most of the win comes from, so this is intended, but it is worth calling out to the App reviewers in case anything treats a collection callback as a signal that data arrived.
| // 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); | ||
| } |
There was a problem hiding this comment.
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_Aends up as{a: 1, y: 2}in the cache and in storage. - On this branch,
test_Aends 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?
| // 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)); | ||
| } |
There was a problem hiding this comment.
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?
2 participants
Details
Fixes the two tabs with a heavy account crash both tabs
Root cause. A collection write (
mergeCollection/multiSet/setCollection/partialSetCollection) persists its non-null members in one batchedmultiMerge/multiSetcall (one cross-tabSYNC_ONYXevent), but everynullmember went throughremove(key)individually — one storage call and one cross-tab event per removed key, with no check that the key even exists. On a heavy account, OpenApp'stransactionViolations_mergeCollection contains ~2,000nullmembers on every boot (keys that were never stored, so the removals are pure no-ops). Each refresh therefore flooded the other tab with ~2,000 single-key events; each event is its own macrotask, so the receiving tab ran onekeysChanged→ one derived-value recompute (reportAttributes,reportTransactionsAndViolations) → oneOnyx.setof an MB-sized derived value per event, saturating the main thread for ~1 minute and broadcasting ~4,000 derived-key events back to the first tab, which then performed ~4,000 IndexedDB reads of those MB-sized values. On Applause-sized accounts both tabs run out of memory and crash.Fix (two layers):
prepareKeyValuePairsForStorageno longer removes null keys as a side effect; it returns them askeysToRemove. All four collection-write paths now:keysChanged()batch (asundefined, with previous values snapshotted for the diff);Storage.removeItems()call, which raises one cross-tab event for the whole batch.InstanceSync(web) now buffers incomingSYNC_ONYXstorage events and flushes once per tick (singlemultiGet+ single dispatch). A tab still running an older bundle during a deploy emits one event per key; the flush timer is scheduled on the first event of the burst, so every event already in the task queue joins one batch instead of re-running the notification pipeline per key.Measured on a heavy (Applause) account, two tabs, per boot: cross-tab events 1,971 → 41 (zero per-key
transactionViolations_events); derived-value writes in the other tab 3,891 → 8; no crash; pin/draft/read-unread/message sync, a 19.5 MB state import, and Clear cache and restart all work across tabs without refresh.Related Issues
Expensify/App#94839
Linked E/App PR
Expensify/App#98121
Automated Tests
tests/unit/onyxTest.ts— newdescribe('batched collection member removals'):mergeCollectiondeletes null members from cache and storage via one batchedremoveItemscall (and never per-keyremoveItem);undefineddelivered);setCollectiondeletes missing members via one batchedremoveItemscall;multiSetdeletes null keys via one batchedremoveItemscall.tests/unit/storage/instanceSyncWebTest.ts— new coalescing tests:multiGetand one dispatch (with key dedup);Manual Tests
With this branch pinned in E/App (web dev build), on a heavy account:
Author Checklist
### Related Issuessection above### Linked E/App PRsection above, and verified this change against it (E/App CI passed and manual testing completed)TestssectiontoggleReportand notonIconClick)myBool && <MyComponent />.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)Avataris modified, I verified thatAvataris working as expected in all cases)mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
Screen.Recording.2026-08-10.at.11.23.39.mov
Screen.Recording.2026-08-10.at.11.29.33.mov