[$1000] Web - Onyx: out-of-order updates #28737

kbecciv · 2023-10-03T16:32:09Z

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

Action Performed:

This bug comes from discussion in #26592
TL;DR: Onyx processes the successData and API response in the wrong order on iOS and Android.
Dive deeper:

App/src/libs/actions/Policy.js

Line 408 in 3352cc8

 API.write('AddMembersToWorkspace', params, {optimisticData, successData, failureData}); 

When adding a member to workspace here, we send the following data:

"optimisticData": {
    "onyxMethod": "set",
    "key": "report_3008335418540078",
    "value": {
        "type": "chat",
        "chatType": "policyExpenseChat",
        "ownerAccountID": 899964195,
        "participantAccountIDs": [
            15664021,
            899964195
        ],
        "policyID": "41A8E180FAEB43DF",
        "reportID": "3008335418540078",
        "reportName": "Chat Report",
        "pendingFields": {
            "createChat": "add"
        },
        "isOptimisticReport": true
        ...
    }
},
"successData": {
    "onyxMethod": "merge",
    "key": "report_3008335418540078",
    "value": {
        "pendingFields": {
            "createChat": null
        },
        "errorFields": {
            "createChat": null
        },
        "isOptimisticReport": false
    }
}

And the API call returns the following onyx merge_collection action:

{
    "jsonCode": 200,
    "requestID": "806fecea0afd3512-WAW",
    "onyxData": [
         {
            "onyxMethod": "mergecollection",
            "key": "report_",
            "value": {
                ...
                "report_3008335418540078": {
                    "reportID": "3008335418540078",
                    "reportName": "",
                    "type": "chat",
                    "chatType": "policyExpenseChat",
                    "ownerEmail": "[pasha@test.co](mailto:pasha@test.co)",
                    "ownerAccountID": 15671904,
                    "policyID": "41A8E180FAEB43DF",
                    "participants": [
                        "[pasha@test.co](mailto:pasha@test.co)",
                        "[paultsimura+3@gmail.com](mailto:paultsimura+3@gmail.com)"
                    ],
                    "participantAccountIDs": [
                        15664021,
                        15671904
                    ],
                    "lastActorAccountID": 15671904,
                },   
            }
        },
    ]
}

It should replace the temporary accountID of the report.ownerAccountID with the real one, which comes from the server.
On the web, the onyx actions are executed in the following order:

persist optimisticData;
persist successData;
persist API response;
The issue is that on iOS, the onyx actions are executed in the following order:
persist optimisticData;
persist API response;
persist successData;
The successData holds merge operation of only several certain fields, which shouldn't affect the report much.
However, here's the tricky part:
This piece of code, which creates a promise for future merge operation, is executed before Onyx persists the API response:
https://github.com/Expensify/react-native-onyx/blob/616c4c5286b7a7fadac18d477cd75edd60865b12/lib/Onyx.js#L1136-L1147
Meaning the following execution chain:
persist optimisticData;
create persistSuccessData promise;
persist API response;
execute persistSuccessData promise;
This means, at the moment persistSuccessData is created, the existingValue is still not updated, so even though it should update only a couple of fields, the modifiedData here is saved in a state of pre-API-response.
Then the report is updated by the "persist API response" operation, but gets overwritten by the persistSuccessData operation with the outdated data.
If we debug step-by-step, you'll notice that the chat name is displayed correctly between the steps 3 and 4 from above.
As an outcome of this complete operation, the state of Onyx is the following:

personalDetailsList contains the new permanent ownerAccountID;
report has the temporary ownerAccountID, which is already removed from Onyx.

Expected Result:

Onyx updates are executed in correct order without data being overwritten

Actual Result:

Onyx updates face a race condition and some data is overwritten

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

Version Number: 1.3.76.0
Reproducible in staging?: n/a
Reproducible in production?: n/a
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation

n/a

Expensify/Expensify Issue URL:
Issue reported by: @@paultsimura
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1696287156456449

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~013373cff08a2575e9
Upwork Job ID: 1709846441891180544
Last Price Increase: 2023-10-05

The text was updated successfully, but these errors were encountered:

melvin-bot · 2023-10-03T16:32:15Z

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

melvin-bot · 2023-10-03T16:32:17Z

Bug0 Triage Checklist (Main S/O)

This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
This bug is not a duplicate report (check E/App issues and #expensify-bugs)
- If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
This bug is reproducible using the reproduction steps in the OP. S/O
- If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
- If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
This issue is filled out as thoroughly and clearly as possible
- Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

ospfranco · 2023-10-04T10:04:20Z

I believe I found the cause.

When the update function is called, all the operations are fired in an async manner. However, this causes a race condition where certain operations will be written into the DB before others. It's a built-in race condition.

Just for documentation purposes: I suggested we need to make the update operations sequentially await. @marcaaron @tgolen are however worried about the app taking a big performance hit. I will try another solution and report back if I manage to get the app to do reliable updates.

pradeepmdk · 2023-10-05T03:13:12Z

I believe I found the cause.

When the update function is called, all the operations are fired in an async manner. However, this causes a race condition where certain operations will be written into the DB before others. It's a built-in race condition.

Just for documentation purposes: I suggested we need to make the update operations sequentially await. @marcaaron @tgolen are however worried about the app taking a big performance hit. I will try another solution and report back if I manage to get the app to do reliable updates.

for note: that is what I already told in my proposal. #26592 (comment)
cc: @Julesssss @ospfranco

pradeepmdk · 2023-10-05T06:08:58Z

Another problem in the App side:

Proposal

Please re-state the problem that we are trying to solve in this issue.

Onyx updating on race condition

What is the root cause of that problem?

https://github.com/Expensify/App/assets/16595846/736946e5-183f-4b16-969a-c0ef118ad228

@marcaaron I saw recently it changed to onyx to memory here
#25614

we are flushed out to Onyx once all the api is completed in the queue (networkRequestQueue). I think this will create a performance issue right? ex: here in the queue 2035 is pending after coming online it gets a response object in the queuedOnyxUpdates to 46k object.
Save queued updates to memory only #25614 Due to this change incase the middle of the response I refresh the page means it will never update to onyx.

What changes do you think we should make in order to solve the problem?

App/src/libs/Network/SequentialQueue.js

Line 126 in 41ddd74

flushOnyxUpdatesQueue();

we need to flush out each api success or failure.

cc: @marcaaron @tgolen @ospfranco @mountiny

melvin-bot · 2023-10-05T08:22:16Z

Job added to Upwork: https://www.upwork.com/jobs/~013373cff08a2575e9

melvin-bot · 2023-10-05T08:22:24Z

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

melvin-bot · 2023-10-05T08:30:53Z

Upwork job price has been updated to $1000

Julesssss · 2023-10-05T08:31:04Z

@CortneyOfstad bumping the price as this one is pretty critical

CortneyOfstad · 2023-10-05T19:26:33Z

@jjcoffee any thoughts on the comments/proposals above?

jjcoffee · 2023-10-06T15:23:47Z

Unsure if this is still reproducible based on this, but also looks like @ospfranco has a PR in the works.

pradeepmdk · 2023-10-07T01:13:17Z

@jjcoffee #26592 (comment), is fixed . but still have some issues can you check my #28737 (comment)

CortneyOfstad · 2023-10-09T14:47:20Z

Bump @jjcoffee ^^^ Thanks!

jjcoffee · 2023-10-09T14:54:44Z

Sorry @pradeepmdk could you flesh out your proposal or try to explain it a bit better? I'm finding it hard to follow what new issue you've found.

ospfranco · 2023-10-10T06:21:11Z

From what I understand API requests responses are placed in a networkRequestQueue, which was before persisted to disk but now it is only kept in memory. If one refreshes the tab, then this queue will be lost (also due to its size it will probably take some time to process). Which could potentially lead to an inconsistent state. IMO this is another issue, not even sure if it is an issue if it doesn't cause incosistent data in the app, and should be in a separate ticket.

my PR is merged and should have already fixed the out-of-order database writes. Let me know if there is anything else.

Julesssss · 2023-10-10T08:18:45Z

Hi @ospfranco, the PR link you shared just links back to this issue.

ospfranco · 2023-10-10T08:19:40Z

Oops, sorry, this is the PR

Expensify/react-native-onyx#384

Julesssss · 2023-10-10T08:37:02Z

Ah, fantastic. So I think we can consider this issue finished once the Onyx lib is bumped.

Julesssss · 2023-12-06T13:53:42Z

@CortneyOfstad yeah 👍

CortneyOfstad · 2023-12-07T14:31:32Z

Sounds good @Julesssss!

@paultsimura I've adjusted the offer in Upwork, so please let me know once you accept!

@ospfranco I still need you to link me your Upwork profile so I can send you an offer. Thanks!

paultsimura · 2023-12-07T14:32:55Z

Thank you @CortneyOfstad, accepted the offer

CortneyOfstad · 2023-12-07T14:37:15Z

Thanks! Both @paultsimura and @Julesssss have been paid 👍

Will also ping @ospfranco in Slack 👍

CortneyOfstad · 2023-12-07T14:38:34Z

Actually, can't find them in Slack 🤔

shubham1206agra · 2023-12-08T07:33:45Z

@Julesssss Can you tell me if this issue is fixed or not? Cause I saw the linked PR and I am not able to understand how changes to withOnyx solved the race condition in Onyx.update.

Julesssss · 2023-12-08T08:58:05Z

@shubham1206agra there are a couple of linked PRs, can you clarify the question please?

shubham1206agra · 2023-12-08T09:37:13Z

I am talking about this PR #29169. My question is how did this PR fix the out-of-order updates?

shubham1206agra · 2023-12-11T06:52:43Z

@Julesssss Bump on the question ^

Julesssss · 2023-12-11T09:52:10Z

There are explanations in the original and follow up onyx repo PR

Julesssss · 2023-12-13T12:02:29Z

Actually, can't find them in Slack 🤔

I found them on Twitter, but I got blocked for typing a message that sounded like a scam 😆

lets see if they get my second message

ospfranco · 2023-12-13T20:36:25Z

Ah, so actually my PR was reverted, I don't know which steps the team took to fix the issue if any at all. As it had entailed doing a large refactoring of the Onyx internals. I cannot in good conscience accept any payment since I'm no longer helping Margelo with the project. You can forgo any payment you are planning for me, it's fine :)

Julesssss · 2023-12-18T09:42:45Z

@ospfranco thanks, I see. It's hard to tell but it looks like we would have reverted, then resubmitted with a small fix which would still be eligible for payment.

But I'll leave it up to you

CortneyOfstad · 2023-12-20T12:40:44Z

@ospfranco bump on the comment above ^^^ Thanks!

Julesssss · 2023-12-21T15:55:41Z

Sounds like he's happy not to. We can always reopen if that changes though!

melvin-bot · 2023-12-23T12:40:14Z

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

kbecciv added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Oct 3, 2023

kbecciv assigned Julesssss Oct 3, 2023

melvin-bot bot assigned CortneyOfstad Oct 3, 2023

ospfranco mentioned this issue Oct 4, 2023

Fix concurrent writes in the update operation Expensify/react-native-onyx#384

Merged

Julesssss added the External Added to denote the issue can be worked on by a contributor label Oct 5, 2023

melvin-bot bot changed the title ~~Web - Onyx: out-of-order updates~~ [$500] Web - Onyx: out-of-order updates Oct 5, 2023

melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Oct 5, 2023

melvin-bot bot assigned jjcoffee Oct 5, 2023

Julesssss changed the title ~~[$500] Web - Onyx: out-of-order updates~~ [$1000] Web - Onyx: out-of-order updates Oct 5, 2023

melvin-bot bot added the Overdue label Oct 9, 2023

melvin-bot bot removed the Overdue label Oct 9, 2023

melvin-bot bot added the Overdue label Dec 11, 2023

melvin-bot bot added Daily KSv2 Overdue and removed Overdue Daily KSv2 labels Dec 11, 2023

melvin-bot bot removed the Overdue label Dec 13, 2023

melvin-bot bot added the Overdue label Dec 18, 2023

melvin-bot bot added Overdue and removed Overdue labels Dec 18, 2023

melvin-bot bot removed the Overdue label Dec 20, 2023

Julesssss closed this as completed Dec 21, 2023

paultsimura mentioned this issue Dec 23, 2023

[$500] [MEDIUM] LHN - Green dot(IOU) disappears from WS conversation in LHN when going offline-online #33544

Closed

6 tasks

paultsimura mentioned this issue Feb 22, 2024

Fix waypoints removal #35268

Closed

2 tasks

blimpich mentioned this issue Feb 29, 2024

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

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[$1000] Web - Onyx: out-of-order updates #28737

[$1000] Web - Onyx: out-of-order updates #28737

kbecciv commented Oct 3, 2023 •

edited by melvin-bot bot

Loading

melvin-bot bot commented Oct 3, 2023

melvin-bot bot commented Oct 3, 2023 •

edited by CortneyOfstad

Loading

ospfranco commented Oct 4, 2023

pradeepmdk commented Oct 5, 2023 •

edited

Loading

pradeepmdk commented Oct 5, 2023 •

edited

Loading

melvin-bot bot commented Oct 5, 2023

melvin-bot bot commented Oct 5, 2023

melvin-bot bot commented Oct 5, 2023

Julesssss commented Oct 5, 2023

CortneyOfstad commented Oct 5, 2023

jjcoffee commented Oct 6, 2023 •

edited

Loading

pradeepmdk commented Oct 7, 2023

CortneyOfstad commented Oct 9, 2023

jjcoffee commented Oct 9, 2023

ospfranco commented Oct 10, 2023

Julesssss commented Oct 10, 2023

ospfranco commented Oct 10, 2023

Julesssss commented Oct 10, 2023

Julesssss commented Dec 6, 2023

CortneyOfstad commented Dec 7, 2023

paultsimura commented Dec 7, 2023

CortneyOfstad commented Dec 7, 2023

CortneyOfstad commented Dec 7, 2023

shubham1206agra commented Dec 8, 2023

Julesssss commented Dec 8, 2023

shubham1206agra commented Dec 8, 2023 •

edited

Loading

shubham1206agra commented Dec 11, 2023

Julesssss commented Dec 11, 2023

Julesssss commented Dec 13, 2023

ospfranco commented Dec 13, 2023

Julesssss commented Dec 18, 2023

CortneyOfstad commented Dec 20, 2023

Julesssss commented Dec 21, 2023

melvin-bot bot commented Dec 23, 2023

[$1000] Web - Onyx: out-of-order updates #28737

[$1000] Web - Onyx: out-of-order updates #28737

Comments

kbecciv commented Oct 3, 2023 • edited by melvin-bot bot Loading

Action Performed:

Expected Result:

Actual Result:

Workaround:

Platforms:

melvin-bot bot commented Oct 3, 2023

melvin-bot bot commented Oct 3, 2023 • edited by CortneyOfstad Loading

Bug0 Triage Checklist (Main S/O)

ospfranco commented Oct 4, 2023

pradeepmdk commented Oct 5, 2023 • edited Loading

pradeepmdk commented Oct 5, 2023 • edited Loading

Another problem in the App side:

Proposal

Please re-state the problem that we are trying to solve in this issue.

What is the root cause of that problem?

What changes do you think we should make in order to solve the problem?

melvin-bot bot commented Oct 5, 2023

melvin-bot bot commented Oct 5, 2023

melvin-bot bot commented Oct 5, 2023

Julesssss commented Oct 5, 2023

CortneyOfstad commented Oct 5, 2023

jjcoffee commented Oct 6, 2023 • edited Loading

pradeepmdk commented Oct 7, 2023

CortneyOfstad commented Oct 9, 2023

jjcoffee commented Oct 9, 2023

ospfranco commented Oct 10, 2023

Julesssss commented Oct 10, 2023

ospfranco commented Oct 10, 2023

Julesssss commented Oct 10, 2023

Julesssss commented Dec 6, 2023

CortneyOfstad commented Dec 7, 2023

paultsimura commented Dec 7, 2023

CortneyOfstad commented Dec 7, 2023

CortneyOfstad commented Dec 7, 2023

shubham1206agra commented Dec 8, 2023

Julesssss commented Dec 8, 2023

shubham1206agra commented Dec 8, 2023 • edited Loading

shubham1206agra commented Dec 11, 2023

Julesssss commented Dec 11, 2023

Julesssss commented Dec 13, 2023

ospfranco commented Dec 13, 2023

Julesssss commented Dec 18, 2023

CortneyOfstad commented Dec 20, 2023

Julesssss commented Dec 21, 2023

melvin-bot bot commented Dec 23, 2023

kbecciv commented Oct 3, 2023 •

edited by melvin-bot bot

Loading

melvin-bot bot commented Oct 3, 2023 •

edited by CortneyOfstad

Loading

pradeepmdk commented Oct 5, 2023 •

edited

Loading

pradeepmdk commented Oct 5, 2023 •

edited

Loading

jjcoffee commented Oct 6, 2023 •

edited

Loading

shubham1206agra commented Dec 8, 2023 •

edited

Loading