sdk: notify the room list of a room info change due to late decryption #3018

timokoesters · 2024-01-15T13:02:13Z

This PR fixes element-hq/element-x-ios#1847 for delayed decryptions.
Wrong latest events can still occur when backfill is needed to find the events.

Public API changes documented in changelogs (optional)

bnjbvr

Looks like this is on the right track, congratulations 🥳 Let's brainstorm together how to test this properly.

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

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

crates/matrix-sdk-base/src/client.rs

crates/matrix-sdk-base/src/store/mod.rs

crates/matrix-sdk-ui/src/room_list_service/room_list.rs

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

crates/matrix-sdk-base/src/client.rs

crates/matrix-sdk-base/src/rooms/normal.rs

testing/matrix-sdk-integration-testing/src/tests/sliding_sync/room.rs

codecov · 2024-01-29T10:37:46Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (d84387d) 83.74% compared to head (aa17148) 83.85%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3018      +/-   ##
==========================================
+ Coverage   83.74%   83.85%   +0.10%     
==========================================
  Files         229      229              
  Lines       23647    23697      +50     
==========================================
+ Hits        19803    19870      +67     
+ Misses       3844     3827      -17

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

bnjbvr

One small logic bug, and then we need to improve a bit the code comments and documentation. And then we can merge it after the hopefully final round of review. Thanks, it's a bunch of work.

crates/matrix-sdk-base/src/rooms/normal.rs

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

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

crates/matrix-sdk-ui/src/room_list_service/room_list.rs

testing/matrix-sdk-integration-testing/src/tests/sliding_sync/room.rs

bnjbvr · 2024-01-29T15:44:58Z

testing/matrix-sdk-integration-testing/src/tests/sliding_sync/room.rs

+    assert!(timeout(Duration::from_millis(100), stream.next()).await.is_err());
+
+    // Latest event is not set yet
+    assert!(matches!(alice_room.latest_event(), None));


Suggested change

assert!(matches!(alice_room.latest_event(), None));

assert!(alice_room.latest_event().is_none());

I've taken over the review 👀

bnjbvr

Yay, let's address the last comments related to the test and merge this. Good job!

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

bnjbvr · 2024-02-09T15:11:11Z

testing/matrix-sdk-integration-testing/src/tests/sliding_sync/room.rs

+                    r = r.set_body_bytes(bytes);
+                    r


This should work:

Suggested change

r = r.set_body_bytes(bytes);

r

r.set_body_bytes(bytes)

bnjbvr · 2024-02-09T15:11:21Z

testing/matrix-sdk-integration-testing/src/tests/sliding_sync/room.rs

+                    let r = wiremock::ResponseTemplate::new(504);
+                    r


Suggested change

let r = wiremock::ResponseTemplate::new(504);

r

wiremock::ResponseTemplate::new(504)

bnjbvr · 2024-02-09T15:12:43Z

testing/matrix-sdk-integration-testing/src/tests/sliding_sync/room.rs

@@ -538,3 +551,354 @@ async fn test_room_notification_count() -> Result<()> {

    Ok(())
 }
+
+/// Boolean that decides if to_device messages should be dropped.
+static DROP_TODEVICE: StdMutex<bool> = StdMutex::new(true);


Instead of global state, could we store this bool in the CustomResponder struct?

bnjbvr · 2024-02-09T15:13:09Z

testing/matrix-sdk-integration-testing/src/tests/sliding_sync/room.rs

+    // Looks for a json payload containing "extensions" with a "to_device" part.
+    // This should only match the sliding sync response. In all other cases, it
+    // makes no changes.
+    let Ok(mut json) = serde_json::from_slice::<serde_json::Value>(response) else {
+        return;
+    };
+    let Some(extensions) = json.get_mut("extensions").and_then(|e| e.as_object_mut()) else {
+        return;
+    };
+    // Remove to_device field if it exists
+    let Some(to_device) = extensions.remove("to_device") else {
+        return;
+    };
+    if *DROP_TODEVICE.lock().unwrap() {
+        info!("Dropping to_device: {to_device}");
+        *response = serde_json::to_vec(&json).unwrap().into();
+        return;
+    }


I think it might be easier to understand by checking the boolean first, and if it's false, return early.

bnjbvr · 2024-02-09T15:13:59Z

testing/matrix-sdk-integration-testing/src/tests/sliding_sync/room.rs

+    entries.set_filter(new_filter_all());
+    pin_mut!(stream);
+
+    // Send a message, but the keys won't arrive


Suggested change

// Send a message, but the keys won't arrive

// Send a message, but the keys won't arrive because to-device events are stripped away from the server's response

bnjbvr · 2024-02-09T15:14:43Z

testing/matrix-sdk-integration-testing/src/tests/sliding_sync/room.rs

+    alice_room.latest_event().unwrap();
+
+    // The stream has a single update
+    timeout(Duration::from_millis(100), stream.next()).await.unwrap();


(This comment ^ still valid :P)

bnjbvr · 2024-02-09T15:15:50Z

testing/matrix-sdk-integration-testing/src/tests/sliding_sync/room.rs

+    // Stream has the room again, but no second event
+    // TODO: Synapse sometimes sends the same event two times. This is the
+    // workaround:
+    assert!(timeout(Duration::from_millis(100), stream.next()).await.unwrap().unwrap().len() > 0);


Could you use temporary variables here? It's not clear what's the final thing we're checking against...

bnjbvr

Nice, let's get this merged after a rebase 🥳 Thanks!

This fixes element-hq/element-x-ios#1847 Signed-off-by: Timo Kösters <timo@koesters.xyz>

Signed-off-by: Timo Kösters <timo@koesters.xyz>

I have also changed the priorities so that manual updates are preferred. This means that duplicate updates do not happen if the room was previously unknown. Signed-off-by: Timo Kösters <timo@koesters.xyz>

Signed-off-by: Timo Kösters <timo@koesters.xyz>

…happens Previously, there were duplicate updates. Signed-off-by: Timo Kösters <timo@koesters.xyz>

Signed-off-by: Timo Kösters <timo@koesters.xyz>

timokoesters requested a review from a team as a code owner January 15, 2024 13:02

timokoesters requested review from Hywan and removed request for a team January 15, 2024 13:02

timokoesters changed the title ~~sliding_sync: Trigger room list update when room info changes~~ Fix latest_event for delayed decryptions Jan 15, 2024

timokoesters force-pushed the timo/fix-latest-events branch 2 times, most recently from 0852947 to 25b01fa Compare January 15, 2024 13:09

bnjbvr requested changes Jan 15, 2024

View reviewed changes

timokoesters force-pushed the timo/fix-latest-events branch from 6929d50 to 28ebf3e Compare January 16, 2024 10:57

Hywan previously requested changes Jan 18, 2024

View reviewed changes

crates/matrix-sdk-base/src/client.rs Outdated Show resolved Hide resolved

crates/matrix-sdk-base/src/client.rs Outdated Show resolved Hide resolved

crates/matrix-sdk-base/src/rooms/normal.rs Outdated Show resolved Hide resolved

timokoesters force-pushed the timo/fix-latest-events branch from 079091c to 83957f6 Compare January 23, 2024 15:06

timokoesters requested review from Hywan and bnjbvr January 23, 2024 15:22

timokoesters commented Jan 23, 2024

View reviewed changes

testing/matrix-sdk-integration-testing/src/tests/sliding_sync/room.rs Outdated Show resolved Hide resolved

bnjbvr removed their request for review January 26, 2024 10:16

timokoesters force-pushed the timo/fix-latest-events branch from 509e794 to 9ea4cf2 Compare January 29, 2024 10:25

bnjbvr requested review from bnjbvr and removed request for Hywan January 29, 2024 11:02

bnjbvr requested changes Jan 29, 2024

View reviewed changes

bnjbvr mentioned this pull request Jan 30, 2024

Event Cache API 🕸 #3058

Open

10 tasks

timokoesters force-pushed the timo/fix-latest-events branch from e336783 to 3b87f36 Compare February 5, 2024 15:13

timokoesters requested a review from bnjbvr February 5, 2024 16:00

bnjbvr approved these changes Feb 9, 2024

View reviewed changes

timokoesters force-pushed the timo/fix-latest-events branch 2 times, most recently from cbb60f8 to e648a3e Compare February 12, 2024 09:26

bnjbvr approved these changes Feb 12, 2024

View reviewed changes

bnjbvr mentioned this pull request Feb 12, 2024

Room previews are blank after restoring from backup element-hq/element-x-ios#2398

Open

timokoesters added 2 commits February 12, 2024 11:37

sliding_sync: Trigger room list update when room info changes

a7b221d

This fixes element-hq/element-x-ios#1847 Signed-off-by: Timo Kösters <timo@koesters.xyz>

sliding_sync: Refactor roominfo update receiver

61018a4

Signed-off-by: Timo Kösters <timo@koesters.xyz>

timokoesters and others added 10 commits February 12, 2024 11:37

sliding_sync: Add test to make sure stream updates are in correct order

3a331e5

I have also changed the priorities so that manual updates are preferred. This means that duplicate updates do not happen if the room was previously unknown. Signed-off-by: Timo Kösters <timo@koesters.xyz>

sliding_sync: Add test for delayed decryption roominfo updates

ab43641

Signed-off-by: Timo Kösters <timo@koesters.xyz>

sliding_sync: Refactor roominfo sender/receiver code

ab40ee8

Signed-off-by: Timo Kösters <timo@koesters.xyz>

sliding_sync: Refactor delayed decryption test

802db39

Signed-off-by: Timo Kösters <timo@koesters.xyz>

sliding_sync: More documentation for roominfo sender/receiver

9e2f11e

Signed-off-by: Timo Kösters <timo@koesters.xyz>

sliding_sync: Only emit manual room list update when late decryption …

bafb203

…happens Previously, there were duplicate updates. Signed-off-by: Timo Kösters <timo@koesters.xyz>

sliding_sync: Documentation for RoomInfoUpdate and cargo fmt

35cc274

Signed-off-by: Timo Kösters <timo@koesters.xyz>

sliding_sync: Refactor for code review

b95a444

Signed-off-by: Timo Kösters <timo@koesters.xyz>

test: refactor integration test code to avoid Box::leak

283d30c

sliding_sync: Use assert_next_eq for tests

aa17148

Signed-off-by: Timo Kösters <timo@koesters.xyz>

timokoesters force-pushed the timo/fix-latest-events branch from 8d497e2 to aa17148 Compare February 12, 2024 11:44

bnjbvr changed the title ~~Fix latest_event for delayed decryptions~~ sdk: notify the room list of a room info change due to late decryption Feb 12, 2024

bnjbvr merged commit 5cb587a into main Feb 12, 2024
34 checks passed

bnjbvr deleted the timo/fix-latest-events branch February 12, 2024 13:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sdk: notify the room list of a room info change due to late decryption #3018

sdk: notify the room list of a room info change due to late decryption #3018

timokoesters commented Jan 15, 2024

bnjbvr left a comment

codecov bot commented Jan 29, 2024 •

edited

bnjbvr left a comment •

edited

bnjbvr Jan 29, 2024

bnjbvr left a comment

bnjbvr Feb 9, 2024

bnjbvr Feb 9, 2024

bnjbvr Feb 9, 2024

bnjbvr Feb 9, 2024

bnjbvr Feb 9, 2024

bnjbvr Feb 9, 2024

bnjbvr Feb 9, 2024

bnjbvr left a comment

	assert!(matches!(alice_room.latest_event(), None));
	assert!(alice_room.latest_event().is_none());

	let r = wiremock::ResponseTemplate::new(504);
	r
	wiremock::ResponseTemplate::new(504)

	// Send a message, but the keys won't arrive
	// Send a message, but the keys won't arrive because to-device events are stripped away from the server's response

sdk: notify the room list of a room info change due to late decryption #3018

sdk: notify the room list of a room info change due to late decryption #3018

Conversation

timokoesters commented Jan 15, 2024

bnjbvr left a comment

Choose a reason for hiding this comment

codecov bot commented Jan 29, 2024 • edited

Codecov Report

bnjbvr left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bnjbvr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bnjbvr left a comment

Choose a reason for hiding this comment

codecov bot commented Jan 29, 2024 •

edited

bnjbvr left a comment •

edited