feat(sdk): Skip Sliding Sync Response if it's been received already

[Hywan]

A Sliding Sync v4::Response contains a pos value. It helps to identify progress during several requests/responses. If two requests with the same pos are sent, the server must reply with the same response, as defined in the specification.

The corollary is: If a response contains a pos that has already been received, the client must ignore it, otherwise it can create duplications. For example, let a response containing sync operations, like DELETE or INSERT, with indexes: if this pair of operations are repeated twice with the same index, it creates a broken state.

To avoid this behaviour, this patch stores the last 20 position markers received from the server. If a response contains a pos that has already been received, a (new) error is returned. As with all the other errors, the sync-loop is stopped, and must restarted. The past positions survive to this restart, as for the rest of the state.

When the server replies with a M_UNKNOWN_POS, the pos and the past_positions are all cleared.

⚠️ Note to reviewer:

• What I am sure about. Checking that a pos has not been received must be done at the beginning of the handle_response method.
• What I am not sure about. Right now, the pos and past_positions are saved when handle_response starts to execute. The rest of the code in this method can fail. If an error is raised after the pos has been saved, the next request will pretend everything was alright, and the server will reply with a different response. It can be a problem. For example, if a list has not been updated because an error happened before (e.g. with encryption handling, or with room response handling), the next list update will be broken because the previous one didn't happen.
  I was thinking of saving pos at the end of the method, so that we are sure that everything was done correctly, however it creates new problem: if the error was raised by the update of the list, the next request will use the same pos, the server will reply with the same response, and list updates is likely to be broken if it has partly been updated.
  👉 Actually, the place where pos and past_positions must be placed is unclear to me yet:
  • Should we consider that errors emitted during handle_response (apart from the pos check) should lead to closing the sliding sync session?
  • Should we make list.update infallible? Making handle_encryption, handle_room_response, and process_and_take_response the only fallible places (with pos and past_positions to be saved after those calls)?

[codecov]

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.04% 🎉

Comparison is base (80c9464) 77.78% compared to head (2b938e3) 77.82%.
Report is 23 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2395      +/-   ##
==========================================
+ Coverage   77.78%   77.82%   +0.04%     
==========================================
  Files         175      175              
  Lines       18619    18631      +12     
==========================================
+ Hits        14482    14500      +18     
+ Misses       4137     4131       -6     

Files Changed	Coverage Δ
crates/matrix-sdk/src/sliding_sync/builder.rs	66.07% <100.00%> (+0.30%)	⬆️
crates/matrix-sdk/src/sliding_sync/mod.rs	90.94% <100.00%> (+0.40%)	⬆️

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[Hywan]

After a long discussion with @bnjbvr, we've decided to save the positions (position and past_positions) at the end of handle_response.

[bnjbvr]

LGTM, thanks! (and thanks for adding the expire_session method that I'll be using in #2362)
Only minor comments below, so feel free to address and merge 🚀

[bnjbvr]

Any reason to store the SlidingSyncPositionMarkers overall, rather than just the plain pos as Option<String>? delta_token isn't returned by the server (or it might even be the same value, which should semantically cause an error too but won't), and we're only comparing against the pos fields anyways in the iteration loop below.

[Hywan]

I thought it was easier. We manipulate a single type. The API looks consistent. It also allows to rollback to some pos and delta_token if required, or to debug, if required. I don't have a strong opinion here.

[bnjbvr]

There's just potential to shoot ourselves in the feet later: comparing two pairs of SlidingSyncPositionMarkers isn't the same as independently comparing its fields against two pairs of position markers. Again, delta_token might not be even provided (or it should be a constant), but since your code only reads the pos: Option<String> field, it seems simpler to store only that, until there's need for more?