fix: prevent Onyx.merge from clobbering concurrent Onyx.update writes - #817
Merged
mountiny merged 3 commits intoAug 7, 2026
Merged
Conversation
Contributor
Author
|
Reassure failure is flaky, ran locally and there are not significant perf regressions DetailsPerformance Comparison Report
Significant Changes To DurationShow entries
Show details
Meaningless Changes To DurationShow entries
Show details
Render Count ChangesThere are no entries Render IssuesThere are no entries Added EntriesThere are no entries Removed EntriesThere are no entries |
![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: 8f9a5d046b
ℹ️ 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".
| // Other writers (notably Onyx.update's mergeCollection path, which doesn't participate in mergeQueue) | ||
| // can land between get() resolving and this callback running. Applying the delta on top of the value | ||
| // captured back then and broadcasting it would overwrite those writes wholesale, so re-read the cache. | ||
| const existingValue = cache.hasCacheForKey(key) ? (cache.get(key) as OnyxInput<TKey> | undefined) : valueFromGet; |
There was a problem hiding this comment.
When the concurrent Onyx.update removes the same collection member while this merge's get() is outstanding, partialSetCollection/mergeCollectionWithPatches calls remove(), which only cache.drop()s the key, so hasCacheForKey(key) is false here. This then falls back to the stale valueFromGet and applyMerge() writes that old object back with the new delta, resurrecting a record that the update just deleted (the same class of race this change is trying to fix for deleted report actions).
Useful? React with 👍 / 👎.
Contributor
Author
There was a problem hiding this comment.
It's valid concern that Claude already raised to me, though it's a pre-existing behaviour – we are not introducing this issue here.
It can be further explored in a separate issue in my opinion
Contributor
There was a problem hiding this comment.
@fabioh8010 can you create an issue for that if you think its worth pursuing
Contributor
Author
There was a problem hiding this comment.
Gonals
reviewed
Aug 3, 2026
Gonals
previously approved these changes
Aug 3, 2026
mountiny
self-requested a review
August 3, 2026 15:23
mountiny
approved these changes
Aug 7, 2026
| // Other writers (notably Onyx.update's mergeCollection path, which doesn't participate in mergeQueue) | ||
| // can land between get() resolving and this callback running. Applying the delta on top of the value | ||
| // captured back then and broadcasting it would overwrite those writes wholesale, so re-read the cache. | ||
| const existingValue = cache.hasCacheForKey(key) ? (cache.get(key) as OnyxInput<TKey> | undefined) : valueFromGet; |
Contributor
There was a problem hiding this comment.
@fabioh8010 can you create an issue for that if you think its worth pursuing
Contributor
This was referenced Aug 7, 2026
Open
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
3 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.mergereads the key withOnyxUtils.get(), computes the whole merged value from whatever that read resolved with, and writes it back throughbroadcastUpdate->cache.set(), which is a full replace. The.then()callback does not run the instantget()resolves — microtasks already queued run first — so any write landing in that gap is silently discarded, andOnyx.update'smergeCollectionpath is exactly such a writer: it updates the same key viacache.merge()and never registers inmergeQueue, so the two never serialize. The practical effect is that a standaloneOnyx.mergefired from an Onyx subscriber while anOnyx.updatebatch is in flight reverts everything that batch wrote to the key; in E/App this dropped thependingAction: nullentries coming fromsuccessDataand brought back report actions the server had just deleted, leaving deleted expenses visible again offline (Expensify/App#97400).This PR re-reads the cache at apply time instead of trusting the value
get()resolved with, mirroring whatOnyxUtils.getalready does internally ("prefer cache over stale storage if a concurrent write populated it during the read"), and adds a deterministic regression test underOnyx > merge > concurrency with Onyx.update.Related Issues
Expensify/App#97400
Linked E/App PR
Expensify/App#97691
Automated Tests
Tests were added.
Manual Tests
Test steps of Expensify/App#97400:
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
Screen.Recording.2026-08-03.at.14.56.03.mov
iOS: mWeb Safari
MacOS: Chrome / Safari
Screen.Recording.2026-08-03.at.14.51.49.mov