[HOLD for payment 2024-08-07] [$500] Onyx updates are not being queued in the correct order

[blimpich]

Problem

When batching update operations (e.g. when coming online), Onyx executes the merge and mergeCollection operations in an incorrect order which leads to data inconsistency. See this comment for context.

Why do we care?

This has caused multiple issues: this, this, and is currently holding this.

What's expected?

The updates should happen in the same exact order as they've been enqueued to guarantee the eventual consistency of the data.

Note: this will probably involve working with the react-native-onyx repository

Issue reported by: @paultsimura Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1709229991663269?thread_ts=1705484273.717249&cid=C01GTK53T8Q

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01fec4289917eb6d71
• Upwork Job ID: 1763289951731056640
• Last Price Increase: 2024-02-29
• Automatic offers:
• jjcoffee | Reviewer | 0
• paultsimura | Contributor | 0

Issue Owner

Current Issue Owner: @Christinadobrzyn

[melvin-bot[bot]]

Triggered auto assignment to @Christinadobrzyn (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

[melvin-bot[bot]]

Job added to Upwork: https://www.upwork.com/jobs/~01fec4289917eb6d71

[melvin-bot[bot]]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @jjcoffee (External)

[melvin-bot[bot]]

📣 @jjcoffee 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link Upwork job

[melvin-bot[bot]]

📣 @paultsimura 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link Upwork job Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻 Keep in mind: Code of Conduct | Contributing 📖

[blimpich]

@jjcoffee I assigned @paultsimura without proposal based off this conversation. @paultsimura is going to work on this with help from @tgolen.

[paultsimura]

Hey @tgolen, I'd like to discuss potential ways of fixing this (actually, only one of them worked for me).

This simple change fixes the issue:

- return clearPromise.then(() => Promise.all(_.map(promises, (p) => p())));
+ return clearPromise.then(() => _.reduce(promises, (p, fn) => p.then(fn), Promise.resolve()));

https://github.com/Expensify/react-native-onyx/blob/8cca556b08bf858c17a7d4f5ddd2879ac3f456d4/lib/Onyx.js#L1518

However, it makes all the batched changes execute in sequence, not in parallel, and I guess it's not something we would like, right?

https://github.com/Expensify/App/assets/12595293/45ba5b74-d4cb-4cbd-8494-7345c5722990

[paultsimura]

Okay, so I've spent a ton of time trying different approaches: using the SyncQueue, using the idb-keyval's update method, as well as merging each collection key separately instead of bulk. Unfortunately, nothing worked as expected, and I'm running out of ideas and just wasting a lot of time testing different approaches. I think we should make it external and see if somebody can suggest a proper solution here @tgolen. I will be happy to provide the steps to consistently reproduce the bug as well as the code of the solutions I've tried

[melvin-bot[bot]]

@tgolen, @paultsimura, @jjcoffee, @Christinadobrzyn Whoops! This issue is 2 days overdue. Let's get this updated quick!

[paultsimura]

Melvin, I'm doing my best with the PR 🙂

[Christinadobrzyn]

PR is in the works!

[Christinadobrzyn]

PR is in the works!

[Christinadobrzyn]

PR in the works!

[Christinadobrzyn]

The PR looks like it's being reviewed so I'm going to add the reviewing/weekly label.

[Christinadobrzyn]

monitoring PR https://github.com/Expensify/react-native-onyx/pull/490

[paultsimura]

I feel like that PR is not going to be merged soon - the more I test, the more Onyx bugs I discover 🫠 Moving this issue to weekly was a good thing

[Christinadobrzyn]

monitoring PR https://github.com/Expensify/react-native-onyx/pull/490

[Christinadobrzyn]

monitoring PR https://github.com/Expensify/react-native-onyx/pull/490

[Christinadobrzyn]

keeping an eye on the PR

[paultsimura]

Currently holding for https://github.com/Expensify/App/pull/39488

[chrispader]

@paultsimura i think you referenced the wrong PR. i think you're holding for https://github.com/Expensify/App/pull/38114, which has just been merged :)

[paultsimura]

@paultsimura i think you referenced the wrong PR. i think you're holding for https://github.com/Expensify/App/pull/38114, which has just been merged :)

You're right, thank you🫡

[Christinadobrzyn]

I think https://github.com/Expensify/App/pull/38114 is merged.

@paultsimura what is next with this? Anything you'd like me to retest?

[paultsimura]

I'll proceed with the PR in the next few days, so far no retest is needed, thanks.

[paultsimura]

Made some progress on the PR, it's close to being RFT. Requested some assistance with reviewing TypeScript-wise. Holding for https://github.com/Expensify/react-native-onyx/pull/523

[shahinyan11]

HI there. I recently worked on a problem with the same root cause and came up with a solution that differs from the current implementation in PR. So I took to share it with you, maybe you will like it.

The main logic is - Add new mergeUpdates function which will iterate on updates array and merge all updates with the same key, giving priority to the object whose index is greater. You can see example of mergeUpdates function here

[Christinadobrzyn]

Hi @paultsimura @jjcoffee can you provide an update for us? Thank you!

[paultsimura]

Hi, sorry I've reported on the PR, but not here: I'm OOO until May 13. Will handle the PR in the couple days after I'm back.

[Christinadobrzyn]

looks like we're monitoring - https://github.com/Expensify/react-native-onyx/pull/523

[Christinadobrzyn]

monitoring - https://github.com/Expensify/react-native-onyx/pull/523

[paultsimura]

The PR is ready for review.

[Christinadobrzyn]

monitoring - https://github.com/Expensify/react-native-onyx/pull/523

[paultsimura]

Status update: the PR's pending review from @tgolen who's currently OOO.

[mallenexpensify]

Posted on the PR for 👀 from Tim (if it's a priority)

[Christinadobrzyn]

Monitoring - https://github.com/Expensify/react-native-onyx/pull/490#issuecomment-2145716489

[Christinadobrzyn]

Monitoring - https://github.com/Expensify/react-native-onyx/pull/490#issuecomment-2145716489

Just a heads up - I'm going to be ooo until June 24th. I'm not going to assign anyone new but if you need a new BZ teammate for any reason please feel free to ask for one to be assigned here.

[tgolen]

Weekly Update

• The App PR is finally off HOLD (we were waiting for another PR that bumps the version of Onyx) and it almost ready to merge.
• I suggested waiting a couple of days before merging in order to make sure the new Onyx version doesn't get reverted.

Next Steps

• @paultsimura Fix some more conflicts in the PR
• @tgolen Merge the PR next week

ETA

• Merged by Wednesday, June 19

[tgolen]

Weekly Update

• There was a discussion about how to do regression testing with this change here
• @paultsimura closed https://github.com/Expensify/App/pull/44010 but I am not sure why. @paultsimura what is the plan with that now?

Next Steps

• @paultsimura Get an App PR with an ad-hoc build that points to the react-native-onyx code and have Applause do some regression testing against that

ETA

• Merged by Wednesday, July 3 (plenty of time for regression testing and any additional bug fixes)

[paultsimura]

@tgolen unfortunately, during more extensive testing I noticed quite a major bug with my changes and decided to close the testing PR to avoid spending the QA team's resources in vain:

You seem to update the renderers prop(s) of the "RenderHTML" component in short periods of time, causing costly tree rerenders (last update was 2.60ms ago)

It seems like while the PR was on hold, some significant changes were made to Onyx that started breaking the flow. Unfortunately, my capacity is too low at the moment to dig deep into Onyx again as this PR is quite time-consuming, so please feel free to reassign this task if there's anybody willing to pick it up.

[tgolen]

Ah, that's a real bummer. I'm sorry to hear that you weren't able to get any further on this. For now, I will close this issue and maybe someone can reopen it if they are able to bring it back to life.

[paultsimura]

@tgolen could you please reopen this issue as well?

[Christinadobrzyn]

just checking in on this - @paultsimura @jjcoffee do you have an update for us?

[paultsimura]

Yes @Christinadobrzyn, we are waiting for the QA team to complete the full regression test on the Onyx PR. As @tgolen is OOO until next week, I don't think we'll get any more movement here until then.

[Christinadobrzyn]

Thanks for the update @paultsimura!

[Christinadobrzyn]

Just checking in on this since Melvin will be asking soon - @paultsimura @tgolen do we have an update on this. TY!

[tgolen]

All the regression testing was finished and no issues were found. The onyx PR was merged an included in version 2.0.57 of Onyx. The current version of Onyx that App is using is 2.0.56, so @paultsimura needs to update App to use this new version of Onyx. Once that's done, then this issue can be closed.

[paultsimura]

@tgolen should I open the PR, or are there some kind of regular centralized Onyx bumps?

[tgolen]

I think you need to open a PR. There are no regular bumps for that. @mountiny might know if there are any currently open PRs that bump the version, but I am not aware of any.

[mountiny]

We have just merged a PR from @hannojg but I dont think he started a bump PR yet. So we should make sure we will test for all the merged PRs in onyx compared to the one used in App now

[paultsimura]

Ok @tgolen @mountiny - I will open a bump PR today. Will we look for C+ volunteer like usual for bump PRs, or should @jjcoffee review since he's the C+ for this issue?