Skip to content

Remove messy WithWaitForLeave test clean-up in TestPartialStateJoin #894

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
MadLittleMods wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Remove messy WithWaitForLeave test clean-up in TestPartialStateJoin #894

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2d624cc
Remove messy clean-up code in `TestPartialStateJoin`
MadLittleMods Jul 13, 2026
7904085
Revert "Remove messy clean-up code in `TestPartialStateJoin`"
MadLittleMods Jul 13, 2026
ebda944
Remove `WithWaitForLeave`
MadLittleMods Jul 13, 2026
2b82460
Remove `awaitPartialStateJoinCompletion` from `Destroy`
MadLittleMods Jul 13, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
119 changes: 9 additions & 110 deletions tests/msc3902/federation_room_join_partial_state_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,74 +123,6 @@ func (s *server) AddEDUHandler(eduHandler func(gomatrixserverlib.EDU) bool) func
}
}

// WithWaitForLeave runs the given action and, when the resulting leave is
// expected to reach this server, waits for it. `leaveAction` is always run; the
// wait is skipped when `user` had already left the room (per their own
// homeserver, so the action produces no new leave) or when this server isn't in
// the room (so the leave won't be federated to us).
func (s *server) WithWaitForLeave(
t *testing.T, room *federation.ServerRoom, user *client.CSAPI, leaveAction func(),
) {
userID := user.UserID
leaveChannel := make(chan gomatrixserverlib.PDU, 10)
removePDUHandler := s.AddPDUHandler(
func(e gomatrixserverlib.PDU) bool {
if membership, _ := e.Membership(); e.Type() == "m.room.member" &&
*e.StateKey() == userID &&
membership == "leave" {
leaveChannel <- e
return true
}
return false
},
)
defer removePDUHandler()

// We need to check if the user (on their homeserver) thinks they're in the
// room, before performing the `leaveAction` (to avoid races).
//
// If they are not in the room, then the `leaveAction` will not produce a
// new leave event and we should not wait for one.
//
// If they are in the room then the `leaveAction` will produce a new leave
// event. We then need to check if we expect this server receive the leave
// event by checking if this server is in the room. If they are, we wait, if
// not we can return immediately after the `leaveAction`.
userInRoom := userIsJoinedTo(t, user, room.RoomID)

leaveAction()

if !userInRoom {
// The user had already left, so the action produced no new leave and
// none is coming: don't wait.
t.Logf("%s is not joined to test room %s; not waiting for them to leave.", userID, room.RoomID)
return
}

if !s.isInRoom(room) {
// The homeserver only federates the leave to servers that are in the
// room. If we aren't, no leave PDU is coming to us, so don't block until
// the timeout.
t.Logf("%s is not in test room %s; not waiting for %s to leave.", s.ServerName(), room.RoomID, userID)
return
}

// Otherwise the action triggered the leave, which arrives as a PDU our
// handler matches. Wait on its channel rather than polling
// `room.CurrentState`: the room's current state is updated (by
// `room.AddEvent`) *before* the PDU callback runs, so returning on a
// `CurrentState` check could deregister our handler in the window before the
// callback fires, making the (expected) leave look unexpected to
// `HandleTransactionRequests`. This returns as soon as the leave arrives; the
// timeout is only a ceiling for declaring failure.
select {
case <-leaveChannel:
t.Logf("%s saw %s leave test room %s.", s.ServerName(), userID, room.RoomID)
case <-time.After(1 * time.Second):
t.Errorf("%s timed out waiting for %s to leave test room %s.", s.ServerName(), userID, room.RoomID)
}
}

// isInRoom reports whether this Complement server has a joined user in the room,
// according to its own `ServerRoom` view. The server reliably tracks its own
// users' membership (it created their join/leave events), so this answers "will
Expand Down Expand Up @@ -2295,7 +2227,7 @@ func TestPartialStateJoin(t *testing.T) {

// The room starts with @charlie:server1 and @derek:server1 in it.
// @elsie:server2 joins the room before @t23alice:hs1.
server2Room := server2.MustJoinRoom(
server2.MustJoinRoom(
t,
deployment,
server1.ServerName(),
Expand All @@ -2306,7 +2238,6 @@ func TestPartialStateJoin(t *testing.T) {

// @t23alice:hs1 joins the room.
psjResult := beginPartialStateJoin(t, server1, room, alice)
defer server2.WithWaitForLeave(t, server2Room, alice, func() { psjResult.Destroy(t) })

// Both homeservers should receive device list updates.
renameDevice(t, alice, "A new device name 1")
Expand Down Expand Up @@ -2343,17 +2274,14 @@ func TestPartialStateJoin(t *testing.T) {
t.Log("@charlie and @derek received device list update.")

// @elsie:server2 joins the room.
server2Room := server2.MustJoinRoom(
server2.MustJoinRoom(
t,
deployment,
server1.ServerName(),
room.RoomID,
server2.UserID("elsie"),
federation.WithPartialState(),
)
// NB: We register the `psjResult.Destroy()` cleanup twice. This is alright because it
// is idempotent. Here we wait for server 2 to observe the leave too.
defer server2.WithWaitForLeave(t, server2Room, alice, func() { psjResult.Destroy(t) })
joinEvent := room.CurrentState("m.room.member", server2.UserID("elsie"))
server1.MustSendTransaction(t, deployment, deployment.GetFullyQualifiedHomeserverName(t, "hs1"), []json.RawMessage{joinEvent.JSON()}, nil)
awaitEventViaSync(t, alice, room.RoomID, joinEvent.EventID(), "")
Expand Down Expand Up @@ -2505,14 +2433,14 @@ func TestPartialStateJoin(t *testing.T) {
t *testing.T, deployment complement.Deployment, alice *client.CSAPI,
server1 *server, server2 *server,
partialStateRoom *federation.ServerRoom, syncToken string,
) (nextSyncToken string, leaveSharedRoom func()) {
) (nextSyncToken string) {
elsie := server2.UserID("elsie")

// @alice:hs1 creates a public room.
roomID := alice.MustCreateRoom(t, map[string]interface{}{"preset": "public_chat"})

// @elsie:server2 joins the room.
server2Room := server2.MustJoinRoom(t, deployment, deployment.GetFullyQualifiedHomeserverName(t, "hs1"), roomID, elsie)
server2.MustJoinRoom(t, deployment, deployment.GetFullyQualifiedHomeserverName(t, "hs1"), roomID, elsie)
alice.MustSyncUntil(t,
client.SyncReq{
Since: syncToken,
Expand All @@ -2529,13 +2457,7 @@ func TestPartialStateJoin(t *testing.T) {
server1.MustSendTransaction(t, deployment, deployment.GetFullyQualifiedHomeserverName(t, "hs1"), []json.RawMessage{leaveEvent.JSON()}, nil)
syncToken = awaitEventViaSync(t, alice, partialStateRoom.RoomID, leaveEvent.EventID(), syncToken)

leaveSharedRoom = func() {
server2.WithWaitForLeave(t, server2Room, alice, func() {
alice.MustLeaveRoom(t, roomID)
})
}
Comment on lines -2532 to -2536

@MadLittleMods MadLittleMods Jul 13, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This in the function called leaveSharedRoom but the leave already happens above.

For context, this was added in #627 which was addressing the flakes caused by homeservers still participating in rooms after each test.

Which as the PR description explains, we have a better solution for now.


return syncToken, leaveSharedRoom
return syncToken
}

// testMissedDeviceListUpdateSentOncePartialJoinCompletes takes a room where hs1 incorrectly
Expand All @@ -2561,8 +2483,7 @@ func TestPartialStateJoin(t *testing.T) {
// The homeserver under test cannot simply use the current state of the room to
// determine which device list updates it must send out once the partial state join
// completes.
_, leaveSharedRoom := setupAnotherSharedRoomThenLeave(t, deployment, alice, server1, server2, room, syncToken)
defer leaveSharedRoom()
setupAnotherSharedRoomThenLeave(t, deployment, alice, server1, server2, room, syncToken)
}

// Finish the partial state join.
Expand All @@ -2589,8 +2510,7 @@ func TestPartialStateJoin(t *testing.T) {
// The room starts with @charlie:server1 and @derek:server1 in it.
// @t26alice:hs1 joins the room, followed by @elsie:server2.
// @elsie:server2 is kicked with an invalid event.
syncToken, server2Room, psjResult := setupIncorrectlyAcceptedKick(t, deployment, alice, server1, server2, deviceListUpdateChannel1, deviceListUpdateChannel2, room)
defer server2.WithWaitForLeave(t, server2Room, alice, func() { psjResult.Destroy(t) })
syncToken, _, psjResult := setupIncorrectlyAcceptedKick(t, deployment, alice, server1, server2, deviceListUpdateChannel1, deviceListUpdateChannel2, room)

// @t26alice:hs1 sends out a device list update which is missed by @elsie:server2.
// @elsie:server2 must receive missed device list updates once the partial state join finishes.
Expand Down Expand Up @@ -2641,7 +2561,7 @@ func TestPartialStateJoin(t *testing.T) {
// The room starts with @charlie:server1 and @derek:server1 in it.
// @elsie:server2 joins the room, followed by @t28alice:hs1.
// server1 does not tell hs1 that server2 is in the room.
server2Room := server2.MustJoinRoom(
server2.MustJoinRoom(
t,
deployment,
server1.ServerName(),
Expand All @@ -2650,7 +2570,6 @@ func TestPartialStateJoin(t *testing.T) {
federation.WithPartialState(),
)
psjResult := beginPartialStateJoin(t, server1, room, alice)
defer server2.WithWaitForLeave(t, server2Room, alice, func() { psjResult.Destroy(t) })

// @t28alice:hs1 sends out a device list update which is missed by @elsie:server2.
// @elsie:server2 must receive missed device list updates once the partial state join finishes.
Expand Down Expand Up @@ -3045,15 +2964,14 @@ func TestPartialStateJoin(t *testing.T) {

// @charlie joins the room.
// Now @charlie's device list is definitely being tracked.
otherRoom := server.MustJoinRoom(t, deployment, deployment.GetFullyQualifiedHomeserverName(t, "hs1"), otherRoomID, server.UserID("charlie"))
server.MustJoinRoom(t, deployment, deployment.GetFullyQualifiedHomeserverName(t, "hs1"), otherRoomID, server.UserID("charlie"))
alice.MustSyncUntil(t,
client.SyncReq{
Since: syncToken,
Filter: buildLazyLoadingSyncFilter(nil),
},
client.SyncJoinedTo(server.UserID("charlie"), otherRoomID),
)
defer server.WithWaitForLeave(t, otherRoom, alice, func() { alice.MustLeaveRoom(t, otherRoomID) })

// Depending on the homeserver implementation, @t31alice:hs1 must have been told that either:
// * charlie updated their device list, or
Expand Down Expand Up @@ -4455,8 +4373,6 @@ func beginPartialStateJoin(t *testing.T, server *server, serverRoom *federation.

// Destroy cleans up the resources associated with the join attempt.
// It is idempotent and must be called once the test is finished.
// Specifically, it ensures that the partial state join completes and makes the joining user leave
// the room.
func (psj *partialStateJoinResult) Destroy(t *testing.T) {

@MadLittleMods MadLittleMods Jul 13, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a follow-up, we're removing this entire need for this function in #895

if psj.fedStateIdsSendResponseWaiter != nil {
psj.fedStateIdsSendResponseWaiter.Finish()
Expand All @@ -4465,23 +4381,6 @@ func (psj *partialStateJoinResult) Destroy(t *testing.T) {
if psj.fedStateIdsRequestReceivedWaiter != nil {
psj.fedStateIdsRequestReceivedWaiter.Finish()
}

// Since the same deployment is being used across multiple tests, ensure that it
// has finished all federation activity before tearing down the Complement server.
// Otherwise the homeserver at the Complement's hostname:port combination may be
// considered offline and interfere with subsequent tests.
t.Log("Cleaning up after test...")

awaitPartialStateJoinCompletion(t, psj.ServerRoom, psj.User)

@MadLittleMods MadLittleMods Jul 13, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LLM review likes to call out that awaitPartialStateJoinCompletion(...) is necessary in order for the server handlers in handleStateIdsRequests(...)/handleStateRequests(...) to finish before the test does but I don't think it really gives any guarantee of anything.

What it's trying to say is that calling psj.fedStateIdsSendResponseWaiter.Finish() wakes up the waiter so the handler goroutine can keep running but we don't wait for that handler to finish here in Destroy(...). The LLM claims that awaitPartialStateJoinCompletion(...) (requests /members on the homeserver) makes sure that the /state_ids or /state goroutine handlers finish because those requests are necessary to resync the room (partial -> full state) which is necessary in order to serve /members.

That seems like a very tenuous connection. I see the race condition it's explaining but using this is the wrong mechanism.

And the only thing it's trying to protect from is t being used after the test finishes which would panic.

In any case, all of this isn't necessary with the second step of this cleanup in a follow-up PR: #895

And the tests still pass in the mean-time (theoretical flake)


// The caller is about to tear down the Complement homeserver. Leave the room, so
// that the homeserver under test stops sending it presence updates.
psj.Server.WithWaitForLeave(
t,
psj.ServerRoom,
psj.User,
func() { psj.User.MustLeaveRoom(t, psj.ServerRoom.RoomID) },
)
}

// send a message into the room without letting the homeserver under test know about it.
Expand Down
Loading