[HOLD for payment 2024-04-15] HIGH: [Reliability] Create a 1:1:1 GetMissingOnyxMessagesForPushNotification API command

[quinthar]

Problem:

Every time you get a push message, we send ReconnectApp, which sometimes does GetMissingOnyxMessages behind the scenes. This breaks 1:1:1 because every API call should generate at most one Auth call, and in this case it generates multiple.

Additionally, sometime we also send OptInToPushNotifications. This is violating our 1:1:1 principle, as every user action (in this case, receiving a push message) should generate a single API call -- and this is sometimes generating two.

Finally, it seems like we send this even if the incoming push notification shows no "gap" with the previous update.

Solution:

Fix this to follow 1:1:1 by:

1. Create a new ReceivedPushNotificationWithUpdateGap API command that accepts a fromUpdateID and toUpdateID range
2. When receiving a new push notification, check to see if the lastUpdateID of the notification matches the lastUpdateID of the client -- and only send ReceivedPushNotificationWithUpdateGap if there is a gap detected

[melvin-bot]

Auto-assigning issues to engineers is no longer supported. If you think this issue should receive engineering attention, please raise it in #whatsnext.

[iwiznia]

Every time you get a push message, we send ReconnectApp,

This is not correct.

which sometimes does GetMissingOnyxMessages behind the scenes. This breaks 1:1:1 because every API call should generate at most one Auth call, and in this case it generates multiple.

Huh? I don't get this at all, are you saying we are breaking the do not make 2 api calls from app for a single user action or the do not make 2 auth calls from a single php request?

But also, I don't think neither of those is happening in this flow.

[arosiclair]

Every time you get a push message, we send ReconnectApp, which sometimes does GetMissingOnyxMessages behind the scenes. This breaks 1:1:1 because every API call should generate at most one Auth call, and in this case it generates multiple.

ReconnectApp should be 1:1:1. It fetches the gap of onyx updates with GetOnyxUpdates in Auth. If it fails to do that, it does a "full" reconnect. The "full reconnect" includes many Auth calls, but they are all reads so it should not violate 1:1:1.

Additionally, sometime we also send OptInToPushNotifications. This is violating our 1:1:1 principle, as every user action (in this case, receiving a push message) should generate a single API call -- and this is sometimes generating two.

We should stop doing this 👍

Finally, it seems like we send this even if the incoming push notification shows no "gap" with the previous update.

This part is intentional. We're attempting to fetch additional updates to keep your app up to date. This does mean that receiving a push notification in the background may generate two ReconnectApp calls:

1. To fetch any gaps
2. To fetch additional updates after your local lastUpdateID

We could combine these though it'll be a little tricky.

[arosiclair]

Posted a PR for OptInToPushNotifications ^

[arosiclair]

Small clarification: I was wrong, the initial API command to fill a gap is actually GetMissingOnyxMessages. This is mostly just an alias for ReconnectApp on the backend which confused me.

So far we have

• Removed the OptInToPushNotifications call.

Next we need to

1. Remove the extra ReconnectApp call for fetching additional updates
2. Replace the GetMissingOnyxMessages call with a ReceivedPushNotificationWithUpdateGap call which should function identically

This is just a day's worth of work but will need to go through reviews and deploy so ETA would be end of next week.

[arosiclair]

After some discussion we decided not to add a new command since it would be identical to GetMissingOnyxMessages. So all we need left is the App PR to remove the extra ReconnectApp call.

However, one thing I'm noticing on Android is that GetMissingOnyxMessages is NOT being called when the app wakes in the background. What I'm seeing is:

1. Add yourself as a workspace member
2. Kill the app
3. Remove yourself from the workspace (this creates some missed onyx updates)
4. Send yourself a DM
5. The app wakes up in the background
6. A gap is detected and ONYX_UPDATES_FROM_SERVER gets set in Onyx to fill the gap
7. The callback for ONYX_UPDATES_FROM_SERVER does NOT run and thus GetMissingOnyxMessages is not called.

The next time the app is opened, the callback finally runs and fills the gap. So it seems there are issues running Onyx callbacks in the background process. I confirmed this by moving that code out of the callback and just running it synchronously and it worked.

This is a separate problem so I'll make a follow up issue to address it, but in the meantime, we should probably expect a bit of regression for background refresh.

Follow up here: #39544

[arosiclair]

This was CP'd to staging and should be on production soon

[melvin-bot]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.60-13 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• [No QA] Use one API call when receiving push notifications #39375

If no regressions arise, payment will be issued on 2024-04-15. 🎊

[arosiclair]

All done!