Fix flawed membership checks and align membership checking patterns by MadLittleMods · Pull Request #813 · matrix-org/complement

MadLittleMods · 2025-10-27T20:08:51Z

Spawning from #808 per my suggestion on #808 (comment),

We have other spots that have flawed membership checks that need to be fixed. For example, when our goal is to wait for the user's membership to be leave, we should keep checking until it is leave. Currently, there are some spots that wait until any membership exists for the user and then asserts leave on that which is flawed because that user may have previous membership events that may be picked up first instead of waiting for the leave.

This PR fixes those flawed checks and aligns our membership checks so we don't cargo cult this bad pattern elsewhere.

Generalize our client.SyncXXX helpers to use syncMembershipIn utility
- More robust
- Standardize extra checks on event (previously, only available with client.SyncJoinedTo)
Introduce client.SyncBannedFrom so we can differentiate ban/leave

Dev notes

Pull Request Checklist

Pull request includes a sign off

Signed-off-by: Eric Eastwood erice@element.io

Same fix as applied in #808

MadLittleMods · 2025-10-27T20:14:32Z

-	charlie.MustSyncUntil(t, client.SyncReq{}, client.SyncTimelineHas(
-		room,
-		func(ev gjson.Result) bool {
-			if ev.Get("type").Str != "m.room.member" || ev.Get("state_key").Str != bob.UserID {
-				return false
-			}
-			must.Equal(t, ev.Get("sender").Str, bob.UserID, "Bob should have joined by himself")
-			must.Equal(t, ev.Get("content").Get("membership").Str, "join", "Bob failed to join the room")
-
-			return true
-		},
-	))


Previous flawed check.

Our goal is to wait until bob is joined. Previously, this waited for any bob membership (which could have been the invite instead of the join which we're waiting for) and then asserted that it must be a join.

MadLittleMods · 2025-10-27T20:23:52Z

-	bob.MustSyncUntil(t, client.SyncReq{}, client.SyncTimelineHas(
-		room,
-		func(ev gjson.Result) bool {
-			if ev.Get("type").Str != "m.room.member" || ev.Get("state_key").Str != charlie.UserID {
-				return false
-			}
-			must.MatchGJSON(t, ev,
-				match.JSONKeyEqual("content.membership", "join"),
-				match.JSONKeyEqual("content.join_authorised_via_users_server", alice.UserID),
-			)
-			return true
-		},
-	))


Previous flawed check.

Our goal is to wait until charlie is joined. Previously, this waited for any charlie membership (which could have been the leave instead of the join which we're waiting for) and then asserted that it must be a join.

MadLittleMods · 2025-10-27T20:24:36Z

+		// We assume the passively observing client user is joined to the room
+		roomTypeKey := "join"
+		// Otherwise, if the client is the user whose membership we are checking, we need to
+		// pick the correct room type JSON key based on the membership being checked.
+		if clientUserID == userID {
+			if membership == "join" {
+				roomTypeKey = "join"
+			} else if membership == "leave" || membership == "ban" {
+				roomTypeKey = "leave"
+			} else if membership == "invite" {
+				roomTypeKey = "invite"
+			} else if membership == "knock" {
+				roomTypeKey = "knock"
+			} else {
+				return fmt.Errorf("syncMembershipIn(%s, %s): unknown membership: %s", roomID, membership, membership)
+			}
+		}


Spec: https://spec.matrix.org/v1.16/client-server-api/#get_matrixclientv3sync_response-200_rooms

MadLittleMods · 2025-10-27T20:39:40Z

-					return true
-				},
-			))
+			bob.MustSyncUntil(t, client.SyncReq{}, client.SyncJoinedTo(bob.UserID, roomID))


I've double-checked the translation is correct for all of these updates:

The client syncing

The user we're checking

The membership being checked

MadLittleMods · 2025-10-27T20:42:54Z

-			bob.MustSyncUntil(t, client.SyncReq{}, client.SyncTimelineHas(
-				roomID,
-				func(ev gjson.Result) bool {
-					if ev.Get("type").Str != "m.room.member" || ev.Get("state_key").Str != bob.UserID {
-						return false
-					}
-					must.Equal(t, ev.Get("content").Get("membership").Str, "join", "Bob failed to join the room")
-					return true
-				},
-			))


Any of these client.SyncTimelineHas(...) checks where we check until we see a certain event type and then use an assert for the membership are bad form.

While, it works well in some tests like this specific one, that's only because the user doesn't have any other previous membership in the room to be confused with. If there was any other previous membership for this user, this is flawed (see #813 (comment) as an example).

Instead of leaving these around to be copy-pasted and cargo-culted around, I've updated all of them to use the more proper assertion check.

MadLittleMods · 2025-10-27T20:44:14Z

-			bob.MustSyncUntil(t, client.SyncReq{}, client.SyncTimelineHas(
-				roomID,
-				func(ev gjson.Result) bool {
-					return ev.Get("type").Str == "m.room.member" &&
-						ev.Get("content.membership").Str == "leave" &&
-						ev.Get("state_key").Str == alice.UserID
-				},
-			))


For reference, this kind of check was just fine. We return false until we find a leave membership for the user. While fine, I've updated it to use the more proper utility we have for this.

(notice the difference to the above bad form assertion)

anoadragon453

Thanks for taking this on! I wonder if it fixes any flaky tests in Synapse.

anoadragon453 · 2025-11-10T17:34:01Z

+		// FIXME: Ideally, we'd use something like `state_after` to get the actual current
+		// state in the room instead of us assuming that no state resets/conflicts happen
+		// when we apply state from the `timeline` on top of the `state`. But `state_after`
+		// is gated behind a sync request parameter which we can't control here.


Any reason not to enable state_after for all /sync calls in Complement?

Could work although it's only available in new Matrix versions (added in v1.16) and I don't think we can assume all homeservers support that version yet.

Something to revisit in the future ⏩

anoadragon453 · 2025-11-10T17:35:01Z

+		if clientUserID != userID ||
+			// Otherwise, if the client is the user whose membership we are checking,


This comment and if statement don't align.

I think it does align although more subtle than the other similar comments as it's using negative logic.

In any case, I've updated the logic layout and comments to make this more clear:

complement/client/sync.go

Lines 347 to 357 in e1ac188

// Check the timeline

//

// This is also important to differentiate between leave/ban because those both

// appear in the `leave` `roomTypeKey` and we need to specifically check the

// timeline for the membership event to differentiate them.

var secondErr error

// The `timeline` is only available for join/leave/ban memberships.

if slices.Contains([]string{"join", "leave", "ban"}, membership) ||

// We assume the passively observing client user is joined to the room (therefore

// has `timeline`).

clientUserID != userID {

MadLittleMods · 2025-11-10T20:42:19Z

Thanks for the review @anoadragon453 🦘

MadLittleMods added 9 commits October 27, 2025 12:01

Properly wait for users to leave down sync

558850d

Same fix as applied in #808

Better wait for correct membership event

8656464

Introduce SyncBannedFrom

8788c3c

Tighten up syncMembershipIn

a820988

Replace more SyncTimelineHas usage

cca4368

Replace SyncTimelineHas in TestFederationRoomsInvite

2ec2fed

Replace SyncTimelineHas in knocking tests

3c93edb

Align docstrings

ab61fd3

Use correct assertion now that we differentiate leave/ban

9547994

MadLittleMods commented Oct 27, 2025

View reviewed changes

Comment thread tests/restricted_rooms_test.go

MadLittleMods commented Oct 27, 2025

View reviewed changes

MadLittleMods mentioned this pull request Oct 27, 2025

Use SyncLeftFrom to check charlie leave event synced to hs1 #808

Merged

1 task

MadLittleMods marked this pull request as ready for review October 27, 2025 20:47

MadLittleMods requested review from a team as code owners October 27, 2025 20:47

anoadragon453 approved these changes Nov 10, 2025

View reviewed changes

MadLittleMods requested a review from anoadragon453 November 10, 2025 18:34

MadLittleMods added 3 commits November 10, 2025 14:24

Make it more clear that this is an additional use case

9cc6af9

Make the timeline logic more straight-forward

680e22a

Merge branch 'main' into madlittlemods/better-wait-for-membershipin-sync

e1ac188

MadLittleMods merged commit 1de6412 into main Nov 10, 2025
4 checks passed

MadLittleMods deleted the madlittlemods/better-wait-for-membershipin-sync branch November 10, 2025 20:41

MadLittleMods removed the request for review from anoadragon453 November 10, 2025 20:41

		if clientUserID != userID \|\|
		// Otherwise, if the client is the user whose membership we are checking,

	// Check the timeline
	//
	// This is also important to differentiate between leave/ban because those both
	// appear in the `leave` `roomTypeKey` and we need to specifically check the
	// timeline for the membership event to differentiate them.
	var secondErr error
	// The `timeline` is only available for join/leave/ban memberships.
	if slices.Contains([]string{"join", "leave", "ban"}, membership) \|\|
	// We assume the passively observing client user is joined to the room (therefore
	// has `timeline`).
	clientUserID != userID {

Conversation

MadLittleMods commented Oct 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Dev notes

Pull Request Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anoadragon453 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

MadLittleMods commented Nov 10, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

MadLittleMods commented Oct 27, 2025 •

edited

Loading