ProtonMail / proton-mail

React web application to manage ProtonMail
https://beta.protonmail.com
GNU General Public License v3.0
175 stars 25 forks source link

The "/api/v4/events/<EventID>" API possibly missing to reflect some actions, reflects some of them more than once and in a wrong order #53

Closed vladimiry closed 3 years ago

vladimiry commented 3 years ago

This is a backend related issue but I don't know where to place it so placing here.

The issue is that the /api/v4/events/<EventID> API presumably missing to reflect some actions at least in the case if iterating started from something like 20+ days old EventID value. And so the data loss happens since I iterate the events in order to incrementally pick-up missing updates and patch the local database then. I tested the case on 20 days old EventID value but the value might potentially be lower.

Observations:

The guess is that the issue lies in the events combining logic. Is there a way to switch to the /api/v4/events/<EventID> API version that doesn't do events combining?

Is the /api/v4/events/<EventID> API designed as allowed to incrementally reconstruct the full messages/folders/etc structure regardless from which EventID value the events iteration was started?

Is there an implemented at the company scenario when you iterate the events starting from old EventID value? I understand that normally it's not the case for the in-browser web client (in browser you poll events every 30 seconds).

@dhoko please assist with redirecting the issue to the appropriate team.

dhoko commented 3 years ago

@vladimiry :wave:

Thx for the issue :+1: , we have forwarded it to the API team, we should give you an update soon :crossed_fingers:

vladimiry commented 3 years ago

Another thing I've noticed is that if the sent to API event id is older than 19 days then the API responds with { Refresh: 255, EventID: <latest event id>, ... } props which I guess means that all the previously downloaded/cached data must be dropped and full/bootstrap sync/fetch started from scratch (maybe 19 days is not the actual threshold but API calculates threshold based on other criteria). Can someone confirm this guess?

This observation doesn't withdraw the originally asked questions.

bartbutler commented 3 years ago

That's correct. It sounds a little tight (I thought we kept 3 weeks of events) but yes, that's what that means.

vladimiry commented 3 years ago

but yes, that's what that means.

Was this behavior there initially or got enabled some time ago?

That's correct. It sounds a little tight (I thought we kept 3 weeks of events) but yes, that's what that means.

I have the backups of "event id" only for 23/27/19 days back values. So I didn't actually try the 21-days/3-weeks value which implies that it might actually be the 3 weeks threshold value like you posted.

bartbutler commented 3 years ago

It's been like this since the beginning, so about 4 years, including the combining. The combining is designed to recreate the exact same final state but skip irrelevant information if you are "fast forwarding" the local cache. This scenario (updating old caches) occurs fairly often for the "thick" clients--mobile and bridge.

That said, you should never get delete/update/delete unless there's a race condition or bug somewhere (which could certainly be the case). You can get delete/create/delete, or delete/create/update/delete, as there is an 'undelete' functionality that in particular is used by the bridge. I can look into the raw events if you want to give me the MessageID affected.

vladimiry commented 3 years ago

You can get delete/create/delete, or delete/create/update/delete, as there is an 'undelete' functionality that in particular is used by the bridge.

Thanks for highlighting this behavior. I was initially picking the last update from the updates list like const [update] = updates.slice().reverse(); and then was just removing the entity if update.action=delete and download+save it otherwise. But I didn't want to keep assuming that the update items properly sorted and so I then "simplified" the logic to just look up for any "delete" update in the updates list assuming that the delete is irrecoverable action. So I'm getting back the initial logic which should cover the "undelete" case you highlighted.

I can look into the raw events if you want to give me the MessageID affected.

Sent privately to your email.

bartbutler commented 3 years ago

Hmm, I didn't receive anything. Can you try bart AT pm.me? Thanks.

vladimiry commented 3 years ago

Can you try bart AT pm.me?

The original message got forwarded to the specified address.

bartbutler commented 3 years ago

Sorry, found it. I was looking for something from "Vladimir" :)

vladimiry commented 3 years ago

I've received the needed answers and related assistance. So closing the issue as resolved.