fix(sdk): Mark tracked users as dirty when the SS connection is reset. by Hywan · Pull Request #3965 · matrix-org/matrix-rust-sdk

Hywan · 2024-09-09T13:14:45Z

Closes #3959.

There is a non-negligible difference MSC3575 and MSC4186 in how the
e2ee extension works. When the client sends a request with no pos:

MSC3575 returns all device lists updates since the last request
from the device that asked for device lists (this works similarly to
to-device message handling),
MSC4186 returns no device lists updates, as it only returns changes
since the provided pos (which is null in this case); this is in
line with sync v2.

Therefore, with MSC4186, the device list cache must be marked as to be
re-downloaded if the since token is None, otherwise it's easy to
miss device lists updates that happened between the previous request and
the new “initial” request.

This patch first off implements OlmMachine::mark_all_tracked_users_as_dirty.
Next, this patch uses this method inside sliding sync when pos is None and
e2ee extension is enabled.

codecov · 2024-09-09T13:27:51Z

Codecov Report

Attention: Patch coverage is 97.67442% with 1 line in your changes missing coverage. Please review.

Project coverage is 84.31%. Comparing base (cb82586) to head (943c46f).
Report is 4 commits behind head on main.

Files with missing lines	Patch %	Lines
crates/matrix-sdk-crypto/src/machine/mod.rs	96.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3965      +/-   ##
==========================================
+ Coverage   84.27%   84.31%   +0.03%     
==========================================
  Files         267      267              
  Lines       28296    28323      +27     
==========================================
+ Hits        23846    23880      +34     
+ Misses       4450     4443       -7

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

poljar

Left some nits, I think it generally does what it should do.

crates/matrix-sdk-crypto/src/machine/mod.rs

poljar · 2024-09-09T13:27:43Z

crates/matrix-sdk-crypto/src/machine/tests/mod.rs

+    let account = vodozemac::olm::Account::new();
+
+    // Put some tracked users
+    let damir = user_id!("@damir:localhost");


crates/matrix-sdk/src/sliding_sync/error.rs

crates/matrix-sdk/src/sliding_sync/mod.rs

kegsay · 2024-09-10T07:36:52Z

It's worth emphasising here that this will negatively impact performance and bandwidth, as the rust SDK will be doing many more /keys/query requests now because the client itself resets the E2EE connection more frequently due to not remembering ?pos=. The SDK needs to remember the ?pos= in the same way it remembers to_device.since to not be hit with this performance penalty.

This patch adds the `OldMachine::mark_all_tracked_users_as_dirty`. This patch rewrites a bit `OlmMachine::new_helper` by extracting some piece of it inside `OlmMachine::new_helper_prelude`. With that, we can rewrite `OlmMachine::migration_post_verified_latch_support` to use `IdentityManager::mark_all_tracked_users_as_dirty`. This latter is the shared implementation with `OlmMachine::mark_all_tracked_users_as_dirty`. This patch adds a test for `OlmMachine:mark_all_tracked_users_as_dirty`.

There is a non-negligible difference MSC3575 and MSC4186 in how the `e2ee` extension works. When the client sends a request with no `pos`: * MSC3575 returns all device lists updates since the last request from the device that asked for device lists (this works similarly to to-device message handling), * MSC4186 returns no device lists updates, as it only returns changes since the provided `pos` (which is `null` in this case); this is in line with sync v2. Therefore, with MSC4186, the device list cache must be marked as to be re-downloaded if the `since` token is `None`, otherwise it's easy to miss device lists updates that happened between the previous request and the new “initial” request.

poljar

There are still some nits left, but they are trivial so gonna approve ahead of time.

Thanks for taking the time to write a test for this.

crates/matrix-sdk/src/sliding_sync/mod.rs

crates/matrix-sdk-crypto/src/machine/mod.rs

…ices

See matrix-org/matrix-rust-sdk#3965 for more information. Requires `Extension.onRequest` to be `async`.

* Switch sliding sync support to simplified sliding sync Experimental PR to test js-sdk with simlified sliding sync. This does not maintain support for regulaer sliding sync. * Remove txn_id handling, ensure we always resend when req params change * Fix some tests * Fix remaining tests * Mark TODOs on tests which need to die * Linting * Make comments lie less * void * Always sent full extension request * Fix test * Remove usage of deprecated field * Hopefully fix DM names * Refactor how heroes are handled in Room * Fix how heroes work * Linting * Ensure that when SSS omits heroes we don't forget we had heroes Otherwise when the room next appears the name/avatar reset to 'Empty Room' with no avatar. * Check the right flag when doing timeline trickling * Also change when the backpagination token is set * Remove list ops and server-provided sort positions SSS doesn't have them. * Linting * Add Room.bumpStamp * Update crypto wasm lib For new functions * Add performance logging * Fix breaking change in crypto wasm v8 * Update crypto wasm for breaking changes See https://github.com/matrix-org/matrix-rust-sdk-crypto-wasm/releases/tag/v8.0.0 for how this was mapped from the previous API. * Mark all tracked users as dirty on expired SSS connections See matrix-org/matrix-rust-sdk#3965 for more information. Requires `Extension.onRequest` to be `async`. * add ts extension * Fix typedoc ref * Add method to interface * Don't force membership to invite The membership was set correctly from the stripped state anyway so this was redundant and was breaking rooms where we'd knocked. * Missed merge * Type import * Make coverage happier * More test coverage * Grammar & formatting Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> * Remove markAllTrackedUsersAsDirty from crypto API Not sure why this was in there, seems like it just needed to be in crypto sync callbacks, which it already was. * Remove I from interface * API doc * Move Hero definition to room-summary * make comment more specific * Move internal details into room.ts and make the comment a proper tsdoc comment * Use terser arrow function syntax Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> * Move comment to where we do the lookup * Clarify comment also prettier says hi * Add comment Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> * Add tsdoc explaining that the summary event will be modified * more comment * Remove unrelated changes * Add docs & make fields optional * Type import * Clarify sync versions Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> * Make tsdoc comment & add info on when it's used. Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> * Rephrase comment Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> * Prettier * Only fetch member for hero in legacy sync mode * Split out a separate method to set SSS room summary Rather than trying to fudge up an object that looked enough like the old one that we could pass it in. * Type import * Make link work * Nope, linter treats it as an unused import * Add link the other way * Add more detail to doc Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> * Remove unnecessary cast Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> * Remove length > 0 check as it wasn't really necessary and may cause heroes not to be cleared? * Doc params * Remove unnecessary undefined comparison Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> * Put the comparison back as it's necessary to stop typescript complaining * Fix comment * Fix comment --------- Co-authored-by: Kegan Dougal <7190048+kegsay@users.noreply.github.com> Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>

Hywan requested review from a team as code owners September 9, 2024 13:14

Hywan requested review from andybalaam, jmartinesp and poljar and removed request for a team, andybalaam and jmartinesp September 9, 2024 13:14

poljar requested changes Sep 9, 2024

View reviewed changes

Hywan force-pushed the fix-sdk-sliding-sync-no-since-must-reset-devices branch from dc004b4 to 62cf64c Compare September 10, 2024 13:07

Hywan force-pushed the fix-sdk-sliding-sync-no-since-must-reset-devices branch from 62cf64c to c0bb278 Compare September 10, 2024 13:18

Hywan requested a review from poljar September 10, 2024 13:19

Hywan force-pushed the fix-sdk-sliding-sync-no-since-must-reset-devices branch from c0bb278 to 781d3d0 Compare September 10, 2024 13:36

poljar approved these changes Sep 10, 2024

View reviewed changes

crates/matrix-sdk/src/sliding_sync/mod.rs Show resolved Hide resolved

crates/matrix-sdk-crypto/src/machine/mod.rs Outdated Show resolved Hide resolved

Hywan added 2 commits September 11, 2024 09:44

doc(crypto): Improve documentation.

ef18c13

chore(sdk): Add an info log in sliding sync.

df8d8ac

Hywan enabled auto-merge (rebase) September 11, 2024 07:45

Merge branch 'main' into fix-sdk-sliding-sync-no-since-must-reset-dev…

943c46f

…ices

Hywan merged commit 2576042 into matrix-org:main Sep 11, 2024

Hywan mentioned this pull request Sep 11, 2024

Add regression test for SSS matrix-org/complement-crypto#131

Merged

kegsay mentioned this pull request Sep 13, 2024

Add mark_all_tracked_users_as_dirty matrix-org/matrix-sdk-crypto-wasm#143

Merged

kegsay added a commit to matrix-org/matrix-js-sdk that referenced this pull request Sep 18, 2024

Mark all tracked users as dirty on expired SSS connections

8616294

See matrix-org/matrix-rust-sdk#3965 for more information. Requires `Extension.onRequest` to be `async`.

BillCarsonFr mentioned this pull request Nov 13, 2024

Every push is marking all tracked users as dirty, thus triggering a keys/query for all users #4257

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(sdk): Mark tracked users as dirty when the SS connection is reset. #3965

fix(sdk): Mark tracked users as dirty when the SS connection is reset. #3965
Hywan merged 5 commits intomatrix-org:mainfrom
Hywan:fix-sdk-sliding-sync-no-since-must-reset-devices

Hywan commented Sep 9, 2024 •

edited

Loading

Uh oh!

codecov bot commented Sep 9, 2024 •

edited

Loading

Uh oh!

poljar left a comment

Uh oh!

Uh oh!

Uh oh!

poljar Sep 9, 2024

Uh oh!

Hywan Sep 10, 2024

Uh oh!

Uh oh!

Uh oh!

kegsay commented Sep 10, 2024

Uh oh!

poljar left a comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

Hywan commented Sep 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Sep 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

poljar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

poljar Sep 9, 2024

Choose a reason for hiding this comment

Uh oh!

Hywan Sep 10, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kegsay commented Sep 10, 2024

Uh oh!

poljar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Hywan commented Sep 9, 2024 •

edited

Loading

codecov bot commented Sep 9, 2024 •

edited

Loading