Conversation
Draft
40 tasks
Contributor
|
@codex review |
Contributor
Author
|
@codex review |
Contributor
Author
|
@MelvinBot review |
grgia
reviewed
Sep 21, 2026
Comment on lines
1527
to
1544
| 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); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
NAB. should multiSetWithRetry also call cancelPendingMergesForKey for keysToRemove?
Data removed with Onyx.multiSet could come back if a merge on the same key was still in progress.
Contributor
|
@mkhutornyi can you please test the chnages in App? thanks |
grgia
previously approved these changes
Sep 22, 2026
Contributor
|
@mkhutornyi how is it looking? |
|
Almost done |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppios.moviOS: mWeb SafariMacOS: Chrome / Safariweb.mov |
mkhutornyi
approved these changes
Sep 22, 2026
| expect(await StorageMock.getItem(member1)).toEqual({itemA: {id: 'a', childID: '1'}}); | ||
| }); | ||
|
|
||
| it('should not cancel a merge that was queued after Onyx.clear() was called', async () => { |
There was a problem hiding this comment.
clear() still cancels merges queued after it when a merge for that key is already in flight
cancelPendingMerges guards on array reference identity, but merge() appends to the existing array instead of creating a new one when a merge for that key is already pending:
if (mergeQueue[key]) {
mergeQueue[key].push(changes); // same array -> identity still matches
return mergeQueuePromise[key];
}So the guard in cancelPendingMerges still matches and the post-clear() delta is dropped, while its promise resolves successfully and nothing is logged.
This is the new test case with the second merge moved before the first one drains:
- Park
member1'sOnyxUtils.getandOnyxUtils.getAllKeys(same mocks as that test). const firstMergePromise = Onyx.merge(member1, {itemA: {id: 'a'}});— creates arrayqc1.const clearPromise = Onyx.clear();thenawait waitForPromisesToResolve()— captures[[member1, qc1]]and parks ongetAllKeys().const secondMergePromise = Onyx.merge(member1, {itemB: {id: 'b'}});— without resolvingfirstGetfirst, so{itemB}is pushed ontoqc1.deferredGetAllKeys.resolve(); await clearPromise;—mergeQueue[member1] === qc1, so the whole array is cancelled.firstGet.resolve(); await Promise.all([firstMergePromise, secondMergePromise]);
Expected: cache.get(member1) → {itemB: {id: 'b'}}
Actual: cache.get(member1) → undefined, storage null. Both merges dropped, both promises resolved.
The existing test passes only because it does firstGet.resolve(); await firstMergePromise; before issuing the second merge, which drains the queue and hands the second merge a fresh array.
Btw I found no user visible bug for this.
Contributor
Author
There was a problem hiding this comment.
Added new test case and fixed this in 0eed0fd
Fix:
- lib/OnyxUtils.ts: getPendingMergeEntries now also records each queue's length when clear() captures it. In cancelPendingMerges:
- If the queue didn't grow, it's cancelled as before.
- If it grew, only the pre-clear() changes are removed, and the queue is flagged in a WeakSet (hasStaleMergeRead).
- lib/Onyx.ts: in the merge callback, a flagged queue uses undefined as its base instead of the stale valueFromGet. The cache still takes priority, as before.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Details
Onyx.mergecould resurrect a key that a concurrent deletion had removed, bringing back its pre-deletion contents.Onyx.mergeapplies its delta to a value captured by an asyncOnyxUtils.get(key).Onyx.setandOnyx.multiSetcancel merges queued before them so the parked callback aborts.setCollection,mergeCollectionandpartialSetCollectionnever did, so an in-flight merge wrote its stale snapshot back with the delta on top. Aftercache.drop()thehasCacheForKeyre-read from #817 cannot help, because the key is no longer cached.Two fixes:
await, so they discard only merges issued before the deleting call. Cancelling in thecache.drop()loop instead would also kill merges issued after the removal, which must win.Related Issues
Expensify/App#98073
Linked E/App PR
Expensify/App#101171
Automated Tests
Five tests in
tests/unit/onyxTest.ts, added to the existingmerge > concurrency with Onyx.updateblock. Each one parksOnyxUtils.getwithcreateDeferredTaskso the deletion lands first, and checks both the cache and storage.should keep the key deleted when a pending merge read resolves after Onyx.update deleted itpartialSetCollectionshould keep the key deleted when a pending merge read resolves after mergeCollection deleted itmergeCollectionWithPatchesshould keep the key deleted when a pending merge read resolves after setCollection dropped itsetCollectionWithRetryshould not resurrect pre-deletion data when a merge is queued after the key was removedshould keep the key deleted when a single-key Onyx.update removal lands during a pending mergesetManual Tests
The race is not manually reproducible without the instrumentation, so there are no direct reproduction steps to verify if the fix is working - we rely on unit testing. I propose these steps as regression verification:
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