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 👍 / 👎.
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