Cleanup stale device lists for users we don't share a room with anymore (#2857)

The stale device lists table might contain entries for users we don't
share a room with anymore. This now asks the roomserver about left users
and removes those entries from the table.

Co-authored-by: Neil Alexander <neilalexander@users.noreply.github.com>

roomserver/api/api.go

@@ -17,6 +17,7 @@ type RoomserverInternalAPI interface {
 	ClientRoomserverAPI
 	UserRoomserverAPI
 	FederationRoomserverAPI
+	KeyserverRoomserverAPI
 
 	// needed to avoid chicken and egg scenario when setting up the
 	// interdependencies between the roomserver and other input APIs
@@ -199,3 +200,7 @@ type FederationRoomserverAPI interface {
 	// Query a given amount (or less) of events prior to a given set of events.
 	PerformBackfill(ctx context.Context, req *PerformBackfillRequest, res *PerformBackfillResponse) error
 }
+
+type KeyserverRoomserverAPI interface {
+	QueryLeftUsers(ctx context.Context, req *QueryLeftUsersRequest, res *QueryLeftUsersResponse) error
+}

roomserver/api/api_trace.go

@@ -19,6 +19,12 @@ type RoomserverInternalAPITrace struct {
 	Impl RoomserverInternalAPI
 }
 
+func (t *RoomserverInternalAPITrace) QueryLeftUsers(ctx context.Context, req *QueryLeftUsersRequest, res *QueryLeftUsersResponse) error {
+	err := t.Impl.QueryLeftUsers(ctx, req, res)
+	util.GetLogger(ctx).WithError(err).Infof("QueryLeftUsers req=%+v res=%+v", js(req), js(res))
+	return err
+}
+
 func (t *RoomserverInternalAPITrace) SetFederationAPI(fsAPI fsAPI.RoomserverFederationAPI, keyRing *gomatrixserverlib.KeyRing) {
 	t.Impl.SetFederationAPI(fsAPI, keyRing)
 }

roomserver/api/query.go

@@ -447,3 +447,15 @@ type QueryMembershipAtEventResponse struct {
 	// do not have known state will return an empty array here.
 	Memberships map[string][]*gomatrixserverlib.HeaderedEvent `json:"memberships"`
 }
+
+// QueryLeftUsersRequest is a request to calculate users that we (the server) don't share a
+// a room with anymore. This is used to cleanup stale device list entries, where we would
+// otherwise keep on trying to get device lists.
+type QueryLeftUsersRequest struct {
+	StaleDeviceListUsers []string `json:"user_ids"`
+}
+
+// QueryLeftUsersResponse is the response to QueryLeftUsersRequest.
+type QueryLeftUsersResponse struct {
+	LeftUsers []string `json:"user_ids"`
+}

roomserver/internal/query/query.go

@@ -805,6 +805,12 @@ func (r *Queryer) QueryBulkStateContent(ctx context.Context, req *api.QueryBulkS
 	return nil
 }
 
+func (r *Queryer) QueryLeftUsers(ctx context.Context, req *api.QueryLeftUsersRequest, res *api.QueryLeftUsersResponse) error {
+	var err error
+	res.LeftUsers, err = r.DB.GetLeftUsers(ctx, req.StaleDeviceListUsers)
+	return err
+}
+
 func (r *Queryer) QuerySharedUsers(ctx context.Context, req *api.QuerySharedUsersRequest, res *api.QuerySharedUsersResponse) error {
 	roomIDs, err := r.DB.GetRoomsByMembership(ctx, req.UserID, "join")
 	if err != nil {

roomserver/inthttp/client.go

@@ -63,6 +63,7 @@ const (
 	RoomserverQueryAuthChainPath               = "/roomserver/queryAuthChain"
 	RoomserverQueryRestrictedJoinAllowed       = "/roomserver/queryRestrictedJoinAllowed"
 	RoomserverQueryMembershipAtEventPath       = "/roomserver/queryMembershipAtEvent"
+	RoomserverQueryLeftMembersPath             = "/roomserver/queryLeftMembers"
 )
 
 type httpRoomserverInternalAPI struct {
@@ -553,3 +554,10 @@ func (h *httpRoomserverInternalAPI) QueryMembershipAtEvent(ctx context.Context,
 		h.httpClient, ctx, request, response,
 	)
 }
+
+func (h *httpRoomserverInternalAPI) QueryLeftUsers(ctx context.Context, request *api.QueryLeftUsersRequest, response *api.QueryLeftUsersResponse) error {
+	return httputil.CallInternalRPCAPI(
+		"RoomserverQueryLeftMembers", h.roomserverURL+RoomserverQueryLeftMembersPath,
+		h.httpClient, ctx, request, response,
+	)
+}

roomserver/inthttp/server.go

@@ -203,4 +203,9 @@ func AddRoutes(r api.RoomserverInternalAPI, internalAPIMux *mux.Router, enableMe
 		RoomserverQueryMembershipAtEventPath,
 		httputil.MakeInternalRPCAPI("RoomserverQueryMembershipAtEventPath", enableMetrics, r.QueryMembershipAtEvent),
 	)
+
+	internalAPIMux.Handle(
+		RoomserverQueryLeftMembersPath,
+		httputil.MakeInternalRPCAPI("RoomserverQueryLeftMembersPath", enableMetrics, r.QueryLeftUsers),
+	)
 }

roomserver/roomserver_test.go

@@ -2,20 +2,27 @@ package roomserver_test
 
 import (
 	"context"
+	"net/http"
 	"testing"
+	"time"
 
+	"github.com/gorilla/mux"
+	"github.com/matrix-org/gomatrixserverlib"
+
+	"github.com/matrix-org/dendrite/internal/httputil"
 	"github.com/matrix-org/dendrite/roomserver"
 	"github.com/matrix-org/dendrite/roomserver/api"
+	"github.com/matrix-org/dendrite/roomserver/inthttp"
 	"github.com/matrix-org/dendrite/roomserver/storage"
 	"github.com/matrix-org/dendrite/setup/base"
 	"github.com/matrix-org/dendrite/test"
 	"github.com/matrix-org/dendrite/test/testrig"
-	"github.com/matrix-org/gomatrixserverlib"
 )
 
 func mustCreateDatabase(t *testing.T, dbType test.DBType) (*base.BaseDendrite, storage.Database, func()) {
+	t.Helper()
 	base, close := testrig.CreateBaseDendrite(t, dbType)
-	db, err := storage.Open(base, &base.Cfg.KeyServer.Database, base.Caches)
+	db, err := storage.Open(base, &base.Cfg.RoomServer.Database, base.Caches)
 	if err != nil {
 		t.Fatalf("failed to create Database: %v", err)
 	}
@@ -67,3 +74,69 @@ func Test_SharedUsers(t *testing.T) {
 		}
 	})
 }
+
+func Test_QueryLeftUsers(t *testing.T) {
+	alice := test.NewUser(t)
+	bob := test.NewUser(t)
+	room := test.NewRoom(t, alice, test.RoomPreset(test.PresetTrustedPrivateChat))
+
+	// Invite and join Bob
+	room.CreateAndInsert(t, alice, gomatrixserverlib.MRoomMember, map[string]interface{}{
+		"membership": "invite",
+	}, test.WithStateKey(bob.ID))
+	room.CreateAndInsert(t, bob, gomatrixserverlib.MRoomMember, map[string]interface{}{
+		"membership": "join",
+	}, test.WithStateKey(bob.ID))
+
+	ctx := context.Background()
+	test.WithAllDatabases(t, func(t *testing.T, dbType test.DBType) {
+		base, _, close := mustCreateDatabase(t, dbType)
+		defer close()
+
+		rsAPI := roomserver.NewInternalAPI(base)
+		// SetFederationAPI starts the room event input consumer
+		rsAPI.SetFederationAPI(nil, nil)
+		// Create the room
+		if err := api.SendEvents(ctx, rsAPI, api.KindNew, room.Events(), "test", "test", "test", nil, false); err != nil {
+			t.Fatalf("failed to send events: %v", err)
+		}
+
+		// Query the left users, there should only be "@idontexist:test",
+		// as Alice and Bob are still joined.
+		res := &api.QueryLeftUsersResponse{}
+		leftUserID := "@idontexist:test"
+		getLeftUsersList := []string{alice.ID, bob.ID, leftUserID}
+
+		testCase := func(rsAPI api.RoomserverInternalAPI) {
+			if err := rsAPI.QueryLeftUsers(ctx, &api.QueryLeftUsersRequest{StaleDeviceListUsers: getLeftUsersList}, res); err != nil {
+				t.Fatalf("unable to query left users: %v", err)
+			}
+			wantCount := 1
+			if count := len(res.LeftUsers); count > wantCount {
+				t.Fatalf("unexpected left users count: want %d, got %d", wantCount, count)
+			}
+			if res.LeftUsers[0] != leftUserID {
+				t.Fatalf("unexpected left users : want %s, got %s", leftUserID, res.LeftUsers[0])
+			}
+		}
+
+		t.Run("HTTP API", func(t *testing.T) {
+			router := mux.NewRouter().PathPrefix(httputil.InternalPathPrefix).Subrouter()
+			roomserver.AddInternalRoutes(router, rsAPI, false)
+			apiURL, cancel := test.ListenAndServe(t, router, false)
+			defer cancel()
+			httpAPI, err := inthttp.NewRoomserverClient(apiURL, &http.Client{Timeout: time.Second * 5}, nil)
+			if err != nil {
+				t.Fatalf("failed to create HTTP client")
+			}
+			testCase(httpAPI)
+		})
+		t.Run("Monolith", func(t *testing.T) {
+			testCase(rsAPI)
+			// also test tracing
+			traceAPI := &api.RoomserverInternalAPITrace{Impl: rsAPI}
+			testCase(traceAPI)
+		})
+
+	})
+}

roomserver/storage/interface.go

@@ -172,5 +172,6 @@ type Database interface {
 	ForgetRoom(ctx context.Context, userID, roomID string, forget bool) error
 
 	GetHistoryVisibilityState(ctx context.Context, roomInfo *types.RoomInfo, eventID string, domain string) ([]*gomatrixserverlib.Event, error)
+	GetLeftUsers(ctx context.Context, userIDs []string) ([]string, error)
 	UpgradeRoom(ctx context.Context, oldRoomID, newRoomID, eventSender string) error
 }

roomserver/storage/postgres/membership_table.go

@@ -21,12 +21,13 @@ import (
 	"fmt"
 
 	"github.com/lib/pq"
+	"github.com/matrix-org/gomatrixserverlib"
+
 	"github.com/matrix-org/dendrite/internal"
 	"github.com/matrix-org/dendrite/internal/sqlutil"
 	"github.com/matrix-org/dendrite/roomserver/storage/postgres/deltas"
 	"github.com/matrix-org/dendrite/roomserver/storage/tables"
 	"github.com/matrix-org/dendrite/roomserver/types"
-	"github.com/matrix-org/gomatrixserverlib"
 )
 
 const membershipSchema = `
@@ -157,6 +158,12 @@ const selectServerInRoomSQL = "" +
 	" JOIN roomserver_event_state_keys ON roomserver_membership.target_nid = roomserver_event_state_keys.event_state_key_nid" +
 	" WHERE membership_nid = $1 AND room_nid = $2 AND event_state_key LIKE '%:' || $3 LIMIT 1"
 
+const selectJoinedUsersSQL = `
+SELECT DISTINCT target_nid
+FROM roomserver_membership m
+WHERE membership_nid > $1 AND target_nid = ANY($2)
+`
+
 type membershipStatements struct {
 	insertMembershipStmt                            *sql.Stmt
 	selectMembershipForUpdateStmt                   *sql.Stmt
@@ -174,6 +181,7 @@ type membershipStatements struct {
 	selectLocalServerInRoomStmt                     *sql.Stmt
 	selectServerInRoomStmt                          *sql.Stmt
 	deleteMembershipStmt                            *sql.Stmt
+	selectJoinedUsersStmt                           *sql.Stmt
 }
 
 func CreateMembershipTable(db *sql.DB) error {
@@ -209,9 +217,33 @@ func PrepareMembershipTable(db *sql.DB) (tables.Membership, error) {
 		{&s.selectLocalServerInRoomStmt, selectLocalServerInRoomSQL},
 		{&s.selectServerInRoomStmt, selectServerInRoomSQL},
 		{&s.deleteMembershipStmt, deleteMembershipSQL},
+		{&s.selectJoinedUsersStmt, selectJoinedUsersSQL},
 	}.Prepare(db)
 }
 
+func (s *membershipStatements) SelectJoinedUsers(
+	ctx context.Context, txn *sql.Tx,
+	targetUserNIDs []types.EventStateKeyNID,
+) ([]types.EventStateKeyNID, error) {
+	result := make([]types.EventStateKeyNID, 0, len(targetUserNIDs))
+
+	stmt := sqlutil.TxStmt(txn, s.selectJoinedUsersStmt)
+	rows, err := stmt.QueryContext(ctx, tables.MembershipStateLeaveOrBan, pq.Array(targetUserNIDs))
+	if err != nil {
+		return nil, err
+	}
+	defer internal.CloseAndLogIfError(ctx, rows, "SelectJoinedUsers: rows.close() failed")
+	var targetNID types.EventStateKeyNID
+	for rows.Next() {
+		if err = rows.Scan(&targetNID); err != nil {
+			return nil, err
+		}
+		result = append(result, targetNID)
+	}
+
+	return result, rows.Err()
+}
+
 func (s *membershipStatements) InsertMembership(
 	ctx context.Context, txn *sql.Tx,
 	roomNID types.RoomNID, targetUserNID types.EventStateKeyNID,

roomserver/storage/shared/storage.go

@@ -1365,6 +1365,43 @@ func (d *Database) JoinedUsersSetInRooms(ctx context.Context, roomIDs, userIDs [
 	return result, nil
 }
 
+// GetLeftUsers calculates users we (the server) don't share a room with anymore.
+func (d *Database) GetLeftUsers(ctx context.Context, userIDs []string) ([]string, error) {
+	// Get the userNID for all users with a stale device list
+	stateKeyNIDMap, err := d.EventStateKeyNIDs(ctx, userIDs)
+	if err != nil {
+		return nil, err
+	}
+
+	userNIDs := make([]types.EventStateKeyNID, 0, len(stateKeyNIDMap))
+	userNIDtoUserID := make(map[types.EventStateKeyNID]string, len(stateKeyNIDMap))
+	// Create a map from userNID -> userID
+	for userID, nid := range stateKeyNIDMap {
+		userNIDs = append(userNIDs, nid)
+		userNIDtoUserID[nid] = userID
+	}
+
+	// Get all users whose membership is still join, knock or invite.
+	stillJoinedUsersNIDs, err := d.MembershipTable.SelectJoinedUsers(ctx, nil, userNIDs)
+	if err != nil {
+		return nil, err
+	}
+
+	// Remove joined users from the "user with stale devices" list, which contains left AND joined users
+	for _, joinedUser := range stillJoinedUsersNIDs {
+		delete(userNIDtoUserID, joinedUser)
+	}
+
+	// The users still in our userNIDtoUserID map are the users we don't share a room with anymore,
+	// and the return value we are looking for.
+	leftUsers := make([]string, 0, len(userNIDtoUserID))
+	for _, userID := range userNIDtoUserID {
+		leftUsers = append(leftUsers, userID)
+	}
+
+	return leftUsers, nil
+}
+
 // GetLocalServerInRoom returns true if we think we're in a given room or false otherwise.
 func (d *Database) GetLocalServerInRoom(ctx context.Context, roomNID types.RoomNID) (bool, error) {
 	return d.MembershipTable.SelectLocalServerInRoom(ctx, nil, roomNID)

roomserver/storage/shared/storage_test.go

@@ -0,0 +1,96 @@
+package shared_test
+
+import (
+	"context"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+
+	"github.com/matrix-org/dendrite/internal/sqlutil"
+	"github.com/matrix-org/dendrite/roomserver/storage/postgres"
+	"github.com/matrix-org/dendrite/roomserver/storage/shared"
+	"github.com/matrix-org/dendrite/roomserver/storage/sqlite3"
+	"github.com/matrix-org/dendrite/roomserver/storage/tables"
+	"github.com/matrix-org/dendrite/setup/config"
+	"github.com/matrix-org/dendrite/test"
+	"github.com/matrix-org/dendrite/test/testrig"
+)
+
+func mustCreateRoomserverDatabase(t *testing.T, dbType test.DBType) (*shared.Database, func()) {
+	t.Helper()
+
+	connStr, clearDB := test.PrepareDBConnectionString(t, dbType)
+	base, _, _ := testrig.Base(nil)
+	dbOpts := &config.DatabaseOptions{ConnectionString: config.DataSource(connStr)}
+
+	db, err := sqlutil.Open(dbOpts, sqlutil.NewExclusiveWriter())
+	assert.NoError(t, err)
+
+	var membershipTable tables.Membership
+	var stateKeyTable tables.EventStateKeys
+	switch dbType {
+	case test.DBTypePostgres:
+		err = postgres.CreateEventStateKeysTable(db)
+		assert.NoError(t, err)
+		err = postgres.CreateMembershipTable(db)
+		assert.NoError(t, err)
+		membershipTable, err = postgres.PrepareMembershipTable(db)
+		assert.NoError(t, err)
+		stateKeyTable, err = postgres.PrepareEventStateKeysTable(db)
+	case test.DBTypeSQLite:
+		err = sqlite3.CreateEventStateKeysTable(db)
+		assert.NoError(t, err)
+		err = sqlite3.CreateMembershipTable(db)
+		assert.NoError(t, err)
+		membershipTable, err = sqlite3.PrepareMembershipTable(db)
+		assert.NoError(t, err)
+		stateKeyTable, err = sqlite3.PrepareEventStateKeysTable(db)
+	}
+	assert.NoError(t, err)
+
+	return &shared.Database{
+			DB:                  db,
+			EventStateKeysTable: stateKeyTable,
+			MembershipTable:     membershipTable,
+			Writer:              sqlutil.NewExclusiveWriter(),
+		}, func() {
+			err := base.Close()
+			assert.NoError(t, err)
+			clearDB()
+			err = db.Close()
+			assert.NoError(t, err)
+		}
+}
+
+func Test_GetLeftUsers(t *testing.T) {
+	alice := test.NewUser(t)
+	bob := test.NewUser(t)
+	charlie := test.NewUser(t)
+
+	ctx := context.Background()
+	test.WithAllDatabases(t, func(t *testing.T, dbType test.DBType) {
+		db, close := mustCreateRoomserverDatabase(t, dbType)
+		defer close()
+
+		// Create dummy entries
+		for _, user := range []*test.User{alice, bob, charlie} {
+			nid, err := db.EventStateKeysTable.InsertEventStateKeyNID(ctx, nil, user.ID)
+			assert.NoError(t, err)
+			err = db.MembershipTable.InsertMembership(ctx, nil, 1, nid, true)
+			assert.NoError(t, err)
+			// We must update the membership with a non-zero event NID or it will get filtered out in later queries
+			membershipNID := tables.MembershipStateLeaveOrBan
+			if user == alice {
+				membershipNID = tables.MembershipStateJoin
+			}
+			_, err = db.MembershipTable.UpdateMembership(ctx, nil, 1, nid, nid, membershipNID, 1, false)
+			assert.NoError(t, err)
+		}
+
+		// Now try to get the left users, this should be Bob and Charlie, since they have a "leave" membership
+		expectedUserIDs := []string{bob.ID, charlie.ID}
+		leftUsers, err := db.GetLeftUsers(context.Background(), []string{alice.ID, bob.ID, charlie.ID})
+		assert.NoError(t, err)
+		assert.ElementsMatch(t, expectedUserIDs, leftUsers)
+	})
+}

roomserver/storage/sqlite3/membership_table.go

@@ -21,12 +21,13 @@ import (
 	"fmt"
 	"strings"
 
+	"github.com/matrix-org/gomatrixserverlib"
+
 	"github.com/matrix-org/dendrite/internal"
 	"github.com/matrix-org/dendrite/internal/sqlutil"
 	"github.com/matrix-org/dendrite/roomserver/storage/sqlite3/deltas"
 	"github.com/matrix-org/dendrite/roomserver/storage/tables"
 	"github.com/matrix-org/dendrite/roomserver/types"
-	"github.com/matrix-org/gomatrixserverlib"
 )
 
 const membershipSchema = `
@@ -133,6 +134,12 @@ const selectServerInRoomSQL = "" +
 const deleteMembershipSQL = "" +
 	"DELETE FROM roomserver_membership WHERE room_nid = $1 AND target_nid = $2"
 
+const selectJoinedUsersSQL = `
+SELECT DISTINCT target_nid
+FROM roomserver_membership m
+WHERE membership_nid > $1 AND target_nid IN ($2)
+`
+
 type membershipStatements struct {
 	db                                              *sql.DB
 	insertMembershipStmt                            *sql.Stmt
@@ -149,6 +156,7 @@ type membershipStatements struct {
 	selectLocalServerInRoomStmt                     *sql.Stmt
 	selectServerInRoomStmt                          *sql.Stmt
 	deleteMembershipStmt                            *sql.Stmt
+	// selectJoinedUsersStmt                           *sql.Stmt // Prepared at runtime
 }
 
 func CreateMembershipTable(db *sql.DB) error {
@@ -412,3 +420,40 @@ func (s *membershipStatements) DeleteMembership(
 	)
 	return err
 }
+
+func (s *membershipStatements) SelectJoinedUsers(
+	ctx context.Context, txn *sql.Tx,
+	targetUserNIDs []types.EventStateKeyNID,
+) ([]types.EventStateKeyNID, error) {
+	result := make([]types.EventStateKeyNID, 0, len(targetUserNIDs))
+
+	qry := strings.Replace(selectJoinedUsersSQL, "($2)", sqlutil.QueryVariadicOffset(len(targetUserNIDs), 1), 1)
+
+	stmt, err := s.db.Prepare(qry)
+	if err != nil {
+		return nil, err
+	}
+	defer internal.CloseAndLogIfError(ctx, stmt, "SelectJoinedUsers: stmt.Close failed")
+
+	params := make([]any, len(targetUserNIDs)+1)
+	params[0] = tables.MembershipStateLeaveOrBan
+	for i := range targetUserNIDs {
+		params[i+1] = targetUserNIDs[i]
+	}
+
+	stmt = sqlutil.TxStmt(txn, stmt)
+	rows, err := stmt.QueryContext(ctx, params...)
+	if err != nil {
+		return nil, err
+	}
+	defer internal.CloseAndLogIfError(ctx, rows, "SelectJoinedUsers: rows.close() failed")
+	var targetNID types.EventStateKeyNID
+	for rows.Next() {
+		if err = rows.Scan(&targetNID); err != nil {
+			return nil, err
+		}
+		result = append(result, targetNID)
+	}
+
+	return result, rows.Err()
+}

roomserver/storage/tables/interface.go

@@ -144,6 +144,7 @@ type Membership interface {
 	SelectLocalServerInRoom(ctx context.Context, txn *sql.Tx, roomNID types.RoomNID) (bool, error)
 	SelectServerInRoom(ctx context.Context, txn *sql.Tx, roomNID types.RoomNID, serverName gomatrixserverlib.ServerName) (bool, error)
 	DeleteMembership(ctx context.Context, txn *sql.Tx, roomNID types.RoomNID, targetUserNID types.EventStateKeyNID) error
+	SelectJoinedUsers(ctx context.Context, txn *sql.Tx, targetUserNIDs []types.EventStateKeyNID) ([]types.EventStateKeyNID, error)
 }
 
 type Published interface {

roomserver/storage/tables/membership_table_test.go

@@ -129,5 +129,11 @@ func TestMembershipTable(t *testing.T) {
 		knownUsers, err := tab.SelectKnownUsers(ctx, nil, userNIDs[0], "localhost", 2)
 		assert.NoError(t, err)
 		assert.Equal(t, 1, len(knownUsers))
+
+		// get users we share a room with, given their userNID
+		joinedUsers, err := tab.SelectJoinedUsers(ctx, nil, userNIDs)
+		assert.NoError(t, err)
+		// Only userNIDs[0] is actually joined, so we only expect this userNID
+		assert.Equal(t, userNIDs[:1], joinedUsers)
 	})
 }