Skip to content
Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
07387a0
Add some tests for push rule behavior on room upgrade
MadLittleMods Nov 17, 2025
3fdbd02
Make the test generic with multiple push rules types
MadLittleMods Nov 17, 2025
cc58ec9
Revert "Make the test generic with multiple push rules types"
MadLittleMods Nov 17, 2025
1167cee
Add `MustUpgradeRoom`
MadLittleMods Nov 17, 2025
d53a307
Test multiple users
MadLittleMods Nov 17, 2025
e00c1c0
Better remote tests
MadLittleMods Nov 17, 2025
fd90c90
Run tests in parallel
MadLittleMods Nov 17, 2025
ad2a7ea
More robust test (`MustAwaitPartialStateJoinCompletion`)
MadLittleMods Nov 17, 2025
5ebcc9f
Even better test
MadLittleMods Nov 17, 2025
e71b03e
Add manually upgraded variant and more robust
MadLittleMods Nov 18, 2025
4fb255c
Specify predecessor when manually upgrading room
MadLittleMods Nov 18, 2025
369250a
Skip test until Synapse is fixed
MadLittleMods Nov 18, 2025
3b4ca0b
Use `SetPushRule`
MadLittleMods Nov 18, 2025
6e2c6f2
Wait/sync until push rules show up
MadLittleMods Nov 18, 2025
10755c2
Fix waiting since the correct spot
MadLittleMods Nov 18, 2025
a7da686
Merge branch 'main' into madlittlemods/room-upgrade-push-rules
MadLittleMods Nov 18, 2025
7a601fa
Skip earlier
MadLittleMods Nov 18, 2025
201a93f
Skip on Dendrite
MadLittleMods Nov 18, 2025
126cc9b
This does sometimes pass but skip since not reliable
MadLittleMods Nov 18, 2025
32aff46
Accurate plural language
MadLittleMods Nov 20, 2025
bae2b74
`upgradeDescriptorPrefix` typo
MadLittleMods Nov 20, 2025
87d6ef0
Explain state of the spec
MadLittleMods Nov 20, 2025
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
40 changes: 40 additions & 0 deletions client/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -174,6 +174,31 @@ func (c *CSAPI) CreateRoom(t ct.TestLike, body map[string]interface{}) *http.Res
return c.Do(t, "POST", []string{"_matrix", "client", "v3", "createRoom"}, WithJSONBody(t, body))
}

// MustUpgradeRoom upgrades a room to the newVersion. Fails the test on error. Returns the new room ID.
func (c *CSAPI) MustUpgradeRoom(t ct.TestLike, roomID string, newVersion string) string {
t.Helper()
res := c.UpgradeRoom(t, roomID, newVersion)
mustRespond2xx(t, res)
resBody := ParseJSON(t, res)
return GetJSONFieldStr(t, resBody, "replacement_room")
}

// UpgradeRoom upgrades a room to the newVersion
func (c *CSAPI) UpgradeRoom(t ct.TestLike, roomID string, newVersion string) *http.Response {
t.Helper()
// Ensure we don't call create a room (upgrade creates a new room) from the same user
// in parallel, else we might try to make 2 rooms in the same millisecond (same
// `origin_server_ts`), causing v12 rooms to get the same room ID thus failing the
// test.
c.createRoomMutex.Lock()
defer c.createRoomMutex.Unlock()
return c.Do(t, "POST", []string{"_matrix", "client", "v3", "rooms", roomID, "upgrade"},
WithJSONBody(t, map[string]string{
"new_version": newVersion,
}),
)
}

// MustJoinRoom joins the room ID or alias given, else fails the test. Returns the room ID.
//
// Args:
Expand Down Expand Up @@ -214,6 +239,21 @@ func (c *CSAPI) JoinRoom(t ct.TestLike, roomIDOrAlias string, serverNames []spec
)
}

// MustAwaitPartialStateJoinCompletion waits until the joined room is no longer partial-stated
func (c *CSAPI) MustAwaitPartialStateJoinCompletion(t ct.TestLike, room_id string) {
t.Helper()

// Use a `/members` request to wait for the room to be un-partial stated.
// We avoid using `/sync`, as it only waits (or used to wait) for full state at
// particular events, rather than the whole room.
c.MustDo(
t,
"GET",
[]string{"_matrix", "client", "v3", "rooms", room_id, "members"},
)
t.Logf("%s's partial state join to %s completed.", c.UserID, room_id)
}

// MustLeaveRoom leaves the room ID, else fails the test.
func (c *CSAPI) MustLeaveRoom(t ct.TestLike, roomID string) {
res := c.LeaveRoom(t, roomID)
Expand Down
281 changes: 281 additions & 0 deletions tests/csapi/room_upgrade_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,281 @@
package csapi_tests

import (
"testing"

"github.com/matrix-org/complement"
"github.com/matrix-org/complement/client"
"github.com/matrix-org/complement/helpers"
"github.com/matrix-org/complement/match"
"github.com/matrix-org/complement/must"
"github.com/matrix-org/complement/runtime"
"github.com/matrix-org/gomatrixserverlib/spec"
"github.com/tidwall/gjson"
)

func TestPushRuleRoomUpgrade(t *testing.T) {
deployment := complement.Deploy(t, 2)
defer deployment.Destroy(t)

alice := deployment.Register(t, "hs1", helpers.RegistrationOpts{
LocalpartSuffix: "alice",
})
alice2 := deployment.Register(t, "hs1", helpers.RegistrationOpts{
LocalpartSuffix: "alice2",
})
bob := deployment.Register(t, "hs2", helpers.RegistrationOpts{
LocalpartSuffix: "bob",
})
bob2 := deployment.Register(t, "hs2", helpers.RegistrationOpts{
LocalpartSuffix: "bob2",
})

t.Run("parallel", func(t *testing.T) {
for _, useManualRoomUpgrade := range []bool{true, false} {
upgradeDescritorPrefix := ""
Comment thread
MadLittleMods marked this conversation as resolved.
Outdated
if useManualRoomUpgrade {
upgradeDescritorPrefix = "manually "
}

// When a homeserver becomes aware of a room upgrade (upgrade is done on local
// homeserver), it should copy over any existing push rules for all of its local users
// from the old room to the new room at the time of upgrade.
t.Run(upgradeDescritorPrefix+"upgrading a room carries over existing push rules for local users", func(t *testing.T) {
t.Parallel()

// FIXME: We have to skip this test on Synapse until
// https://github.com/element-hq/synapse/issues/19199 is resolved.
if useManualRoomUpgrade {
runtime.SkipIf(t, runtime.Synapse)
}
Comment on lines +57 to +61
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

We have to skip with Synapse until element-hq/synapse#19199 is resolved ⏩


// Create a room
roomID := alice.MustCreateRoom(t, map[string]interface{}{
"preset": "public_chat",
"room_version": "10",
})
// Have alice2 join the room
//
// We use two users to ensure that all users get taken care of (not just the person
// upgrading the room).
alice2.MustJoinRoom(t, roomID, nil)

// Add some push rules
alice.MustDo(t, "PUT", []string{"_matrix", "client", "v3", "pushrules", "global", "room", roomID},
client.WithJSONBody(t, map[string]interface{}{
"actions": []string{"dont_notify"},
}),
)
alice2.MustDo(t, "PUT", []string{"_matrix", "client", "v3", "pushrules", "global", "room", roomID},
client.WithJSONBody(t, map[string]interface{}{
"actions": []string{"dont_notify"},
}),
)

// Sanity check the push rules are in the expected state before the upgrade
for _, client := range []*client.CSAPI{alice, alice2} {
t.Logf("Checking push rules (before upgrade) for %s", client.UserID)
pushRulesBefore := client.GetAllPushRules(t)
must.MatchGJSON(t, pushRulesBefore,
match.JSONCheckOff("global.room",
[]interface{}{
roomID,
},
match.CheckOffAllowUnwanted(),
match.CheckOffMapper(func(r gjson.Result) interface{} { return r.Get("rule_id").Str }),
match.CheckOffForEach(func(roomIDFromPushRule interface{}, result gjson.Result) error {
return match.JSONKeyEqual("actions.0", "dont_notify")(result)
}),
),
)
}

// Upgrade the room
var newRoomID string
if useManualRoomUpgrade {
newRoomID = mustManualUpgradeRoom(t, alice, roomID, "11")
} else {
newRoomID = alice.MustUpgradeRoom(t, roomID, "11")
}
t.Logf("Upgraded room %s to %s", roomID, newRoomID)

// Alice2 joins the new room
alice2.MustJoinRoom(t, newRoomID, nil)

// Sanity check the push rules are in the expected state after the upgrade
for _, client := range []*client.CSAPI{alice, alice2} {
t.Logf("Checking push rules (after upgrade) for %s", client.UserID)
pushRulesAfter := client.GetAllPushRules(t)
must.MatchGJSON(t, pushRulesAfter,
match.JSONCheckOff("global.room",
[]interface{}{
roomID,
newRoomID,
},
match.CheckOffAllowUnwanted(),
match.CheckOffMapper(func(r gjson.Result) interface{} { return r.Get("rule_id").Str }),
match.CheckOffForEach(func(roomIDFromPushRule interface{}, result gjson.Result) error {
return match.JSONKeyEqual("actions.0", "dont_notify")(result)
}),
),
)
}
})

// When a homeserver becomes aware of a room upgrade (upgrade is done on remote
// homeserver), it should copy over any existing push rules for all of its local users
// from the old room to the new room at the time of upgrade.
t.Run("joining a remote "+upgradeDescritorPrefix+"upgraded room carries over existing push rules", func(t *testing.T) {
t.Parallel()

// Start a sync loop
_, bobSince := bob.MustSync(t, client.SyncReq{TimeoutMillis: "0"})

// Alice create a room
roomID := alice.MustCreateRoom(t, map[string]interface{}{
"preset": "public_chat",
"room_version": "10",
})
// Remote bob joins the room
bob.MustJoinRoom(t, roomID, []spec.ServerName{
deployment.GetFullyQualifiedHomeserverName(t, "hs1"),
})
// Wait until the homeserver is fully participating in the room so that we can
// double-check the subsequent joins also work (sanity check participating vs
// non-participating logic in the homeserver)
bob.MustAwaitPartialStateJoinCompletion(t, roomID)
// Wait until we know the first bob is joined for sure. We want to make sure bob2
// doesn't also race us to remotely join the room as bob2 should be able to
// locally join and then send a join over federation (because the first bob is
// already joined to the room).
bobSince = bob.MustSyncUntil(t, client.SyncReq{Since: bobSince}, client.SyncJoinedTo(bob.UserID, roomID))
// Remote bob2 joins the room
//
// We use two users to ensure that all users get taken care of (not just the first
// user on the homeserver).
bob2.MustJoinRoom(t, roomID,
// bob2 can do a local join since bob is already in the room. No need to specify
// via servers here.
//
// []spec.ServerName{
// deployment.GetFullyQualifiedHomeserverName(t, "hs1"),
// },
nil,
)

// Add some push rules
bob.MustDo(t, "PUT", []string{"_matrix", "client", "v3", "pushrules", "global", "room", roomID},
client.WithJSONBody(t, map[string]interface{}{
"actions": []string{"dont_notify"},
}),
)
bob2.MustDo(t, "PUT", []string{"_matrix", "client", "v3", "pushrules", "global", "room", roomID},
client.WithJSONBody(t, map[string]interface{}{
"actions": []string{"dont_notify"},
}),
)

// Sanity check the push rules are in the expected state before the upgrade
for _, client := range []*client.CSAPI{bob, bob2} {
t.Logf("Checking push rules (before upgrade) for %s", client.UserID)
pushRulesBefore := client.GetAllPushRules(t)
must.MatchGJSON(t, pushRulesBefore,
match.JSONCheckOff("global.room",
[]interface{}{
roomID,
},
match.CheckOffAllowUnwanted(),
match.CheckOffMapper(func(r gjson.Result) interface{} { return r.Get("rule_id").Str }),
match.CheckOffForEach(func(roomIDFromPushRule interface{}, result gjson.Result) error {
return match.JSONKeyEqual("actions.0", "dont_notify")(result)
}),
),
)
}

// Upgrade the room
var newRoomID string
if useManualRoomUpgrade {
newRoomID = mustManualUpgradeRoom(t, alice, roomID, "11")
} else {
newRoomID = alice.MustUpgradeRoom(t, roomID, "11")
}
t.Logf("Upgraded room %s to %s", roomID, newRoomID)

// Ensure that the remote server sees the tombstone in the old room before
// joining the new room (avoid races and the client woudn't know where to go
// without this hint anyway)
bobSince = bob.MustSyncUntil(t, client.SyncReq{Since: bobSince}, client.SyncTimelineHas(roomID, func(ev gjson.Result) bool {
return ev.Get("type").Str == "m.room.tombstone" && ev.Get("state_key").Str == ""
}))

// Remote bob joins the new room
bob.MustJoinRoom(t, newRoomID, []spec.ServerName{
deployment.GetFullyQualifiedHomeserverName(t, "hs1"),
})
// Wait until the homeserver is fully participating in the room so that we can
// double-check the subsequent joins also work (sanity check participating vs
// non-participating logic in the homeserver)
bob.MustAwaitPartialStateJoinCompletion(t, newRoomID)
// Wait until we know the first bob is joined for sure. We want to make sure bob2
// doesn't also race us to remotely join the room as bob2 should be able to
// locally join and then send a join over federation (because the first bob is
// already joined to the room).
bobSince = bob.MustSyncUntil(t, client.SyncReq{Since: bobSince}, client.SyncJoinedTo(bob.UserID, newRoomID))
// Remote bob2 joins the new room
bob2.MustJoinRoom(t, newRoomID,
// bob2 can do a local join since bob is already in the room. No need to specify
// via servers here.
//
// []spec.ServerName{
// deployment.GetFullyQualifiedHomeserverName(t, "hs1"),
// },
nil,
)

// Sanity check the push rules are in the expected state after the upgrade
for _, client := range []*client.CSAPI{bob, bob2} {
pushRulesAfter := client.GetAllPushRules(t)
t.Logf("Checking push rules (after upgrade) for %s", client.UserID)
must.MatchGJSON(t, pushRulesAfter,
Copy link
Copy Markdown
Collaborator Author

@MadLittleMods MadLittleMods Nov 18, 2025

Choose a reason for hiding this comment

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

These tests appear flaky with Dendrite so we may need to sync until the push rules appear.

Copy link
Copy Markdown
Collaborator Author

@MadLittleMods MadLittleMods Nov 18, 2025

Choose a reason for hiding this comment

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

Making the tests wait for push rules from /sync did not help Dendrite 😅

Seems like the push rules aren't being returned from /sync when handleRoomUpgrade -> copyPushrules is used.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Skipping tests on Dendrite for now ⏩

Copy link
Copy Markdown
Collaborator Author

@MadLittleMods MadLittleMods Nov 19, 2025

Choose a reason for hiding this comment

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

For the future: @S7evinK (former Dendrite dev) mentioned,

Looks like we don't let the syncapi know we changed the account data. Should be trivial to fix.

-- @S7evinK, #dendrite-dev:matrix.org

The relevant code is probably in syncapi/streams/stream_accountdata.go#L41-L108 but I don't immediatly see the problem. There is a skip condition which is a bit suspect but supposedly only applies when from == 0 (initial syncs) vs the incremental syncs that's happening here in the test (and the other conditions should make the skip only apply to room scoped account data).

Or I'm looking in the wrong place 🤷

Given that Dendrite doesn't have a convenient way to run Complement locally like we have with Synapse's complement.sh, this takes more effort than necessary to debug.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

https://github.com/element-hq/dendrite/blob/fbbdf84ac62699ee952e091b4a8cc9577bd4f6bb/userapi/consumers/roomserver.go#L209

Would be the right place, but I didn't have time to check if we need to/should s.syncProducer.SendAccountData after each function call.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

element-hq/dendrite#3668 may fix this.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Exploring this further in #822

match.JSONCheckOff("global.room",
[]interface{}{
roomID,
newRoomID,
},
match.CheckOffAllowUnwanted(),
match.CheckOffMapper(func(r gjson.Result) interface{} { return r.Get("rule_id").Str }),
match.CheckOffForEach(func(roomIDFromPushRule interface{}, result gjson.Result) error {
return match.JSONKeyEqual("actions.0", "dont_notify")(result)
}),
),
)
}
})
}
})
}

func mustManualUpgradeRoom(t *testing.T, c *client.CSAPI, oldRoomID string, newRoomVersion string) string {
t.Helper()

// Create a new room
newRoomID := c.MustCreateRoom(t, map[string]interface{}{
"preset": "public_chat",
"room_version": newRoomVersion,
"creation_content": map[string]interface{}{
// Specify the old room as the predecessor
"predecessor": map[string]interface{}{
"room_id": oldRoomID,
},
},
})

// Send the m.room.tombstone event to the old room
c.MustDo(t, "PUT", []string{"_matrix", "client", "v3", "rooms", oldRoomID, "state", "m.room.tombstone", ""}, client.WithJSONBody(t, map[string]interface{}{
"body": "This room has been replaced",
"replacement_room": newRoomID,
}))

return newRoomID
}
Loading