From a7152348ace031d421d203bc6137ad893971550b Mon Sep 17 00:00:00 2001 From: Kegan Dougal <7190048+kegsay@users.noreply.github.com> Date: Thu, 11 Apr 2024 16:08:33 +0100 Subject: [PATCH 1/3] Add regression test for https://github.com/matrix-org/sliding-sync/issues/417 --- tests-e2e/account_data_test.go | 64 ++++++++++++++++++++++++++++++++++ 1 file changed, 64 insertions(+) diff --git a/tests-e2e/account_data_test.go b/tests-e2e/account_data_test.go index 917f95cb..91179444 100644 --- a/tests-e2e/account_data_test.go +++ b/tests-e2e/account_data_test.go @@ -5,6 +5,7 @@ import ( "testing" "time" + "github.com/matrix-org/complement/b" "github.com/matrix-org/sliding-sync/sync3" "github.com/matrix-org/sliding-sync/sync3/extensions" "github.com/matrix-org/sliding-sync/testutils" @@ -210,6 +211,69 @@ func TestAccountDataDoesntDupe(t *testing.T) { } } +// Regression test for https://github.com/matrix-org/sliding-sync/issues/417 +func TestAccountDataDoesntDupeWithUpdates(t *testing.T) { + alice := registerNewUser(t) + roomID := alice.MustCreateRoom(t, map[string]interface{}{ + "preset": "public_chat", + }) + roomAccountData1 := putRoomAccountData(t, alice, roomID, "foo", map[string]interface{}{"a": "b"}) + roomAccountData2 := putRoomAccountData(t, alice, roomID, "bar", map[string]interface{}{"a2": "b2"}) + + // all_rooms with timeline limit 1 initially + res := alice.SlidingSync(t, sync3.Request{ + Extensions: extensions.Request{ + AccountData: &extensions.AccountDataRequest{ + Core: extensions.Core{ + Enabled: &boolTrue, + }, + }, + }, + Lists: map[string]sync3.RequestList{ + "all_rooms": { + Ranges: sync3.SliceRanges{{0, 10}}, + RoomSubscription: sync3.RoomSubscription{ + TimelineLimit: 1, + }, + }, + }, + }) + m.MatchResponse(t, res, m.MatchList("all_rooms", m.MatchV3Count(1)), m.MatchAccountData(nil, map[string][]json.RawMessage{ + roomID: {roomAccountData1, roomAccountData2}, + })) + + // now send some events into the room + alice.SendTyping(t, roomID, true, 5000) + alice.SendEventSynced(t, roomID, b.Event{ + Type: "custom", + Content: map[string]interface{}{ + "foo": "bar", + }, + }) + + // now sync with a visible_rooms list, we should not see duplicate account data (though do expect 2 as we're changing the subscription) + res = alice.SlidingSync(t, sync3.Request{ + Lists: map[string]sync3.RequestList{ + "all_rooms": { + Ranges: sync3.SliceRanges{{0, 10}}, + RoomSubscription: sync3.RoomSubscription{ + TimelineLimit: 1, + }, + }, + "visible_rooms": { + Ranges: sync3.SliceRanges{{0, 10}}, + RoomSubscription: sync3.RoomSubscription{ + TimelineLimit: 10, + }, + }, + }, + }, WithPos(res.Pos)) + m.MatchResponse(t, res, m.MatchList("all_rooms", m.MatchV3Count(1)), m.MatchAccountData(nil, map[string][]json.RawMessage{ + roomID: {roomAccountData1, roomAccountData2}, + })) + +} + // putAccountData is a wrapper around SetGlobalAccountData. It returns the account data // event as a json.RawMessage, automatically including top-level `type` and `content` // fields. This is useful because it can be compared with account data events in a From 48b5a549b604d358ae51edec700d56e72b968ceb Mon Sep 17 00:00:00 2001 From: Kegan Dougal <7190048+kegsay@users.noreply.github.com> Date: Thu, 11 Apr 2024 17:16:26 +0100 Subject: [PATCH 2/3] Fix sending dupe room account data Fixes #417 --- sync3/extensions/account_data.go | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/sync3/extensions/account_data.go b/sync3/extensions/account_data.go index db63a4a2..b98f7a12 100644 --- a/sync3/extensions/account_data.go +++ b/sync3/extensions/account_data.go @@ -3,6 +3,7 @@ package extensions import ( "context" "encoding/json" + "github.com/matrix-org/sliding-sync/internal" "github.com/matrix-org/sliding-sync/state" "github.com/matrix-org/sliding-sync/sync3/caches" @@ -21,6 +22,8 @@ func (r *AccountDataRequest) Name() string { type AccountDataResponse struct { Global []json.RawMessage `json:"global,omitempty"` Rooms map[string][]json.RawMessage `json:"rooms,omitempty"` + // which rooms have had account data loaded from the DB in this response + loadedRooms map[string]bool } func (r *AccountDataResponse) HasData(isInitial bool) bool { @@ -54,6 +57,13 @@ func (r *AccountDataRequest) AppendLive(ctx context.Context, res *Response, extC } // if this is a room update which is included in the response, send account data for this room if _, exists := extCtx.RoomIDToTimeline[update.RoomID()]; exists { + // ..but only if we haven't before + if res.AccountData != nil && res.AccountData.loadedRooms[update.RoomID()] { + // we've loaded this room before, don't do it again + // this can happen when we consume lots of items in the buffer. If many of them are room updates + // for the same room, we could send dupe room account data if we didn't do this check. + break + } roomAccountData, err := extCtx.Store.AccountDatas(extCtx.UserID, update.RoomID()) if err != nil { logger.Err(err).Str("user", extCtx.UserID).Str("room", update.RoomID()).Msg("failed to fetch room account data") @@ -70,12 +80,14 @@ func (r *AccountDataRequest) AppendLive(ctx context.Context, res *Response, extC } if res.AccountData == nil { res.AccountData = &AccountDataResponse{ - Rooms: make(map[string][]json.RawMessage), + Rooms: make(map[string][]json.RawMessage), + loadedRooms: make(map[string]bool), } } res.AccountData.Global = append(res.AccountData.Global, globalMsgs...) for roomID, roomAccountData := range roomToMsgs { res.AccountData.Rooms[roomID] = append(res.AccountData.Rooms[roomID], roomAccountData...) + res.AccountData.loadedRooms[roomID] = true } } @@ -89,7 +101,8 @@ func (r *AccountDataRequest) ProcessInitial(ctx context.Context, res *Response, } } extRes := &AccountDataResponse{ - Rooms: make(map[string][]json.RawMessage), + Rooms: make(map[string][]json.RawMessage), + loadedRooms: make(map[string]bool), } // room account data needs to be sent every time the user scrolls the list to get new room IDs // TODO: remember which rooms the client has been told about @@ -102,6 +115,7 @@ func (r *AccountDataRequest) ProcessInitial(ctx context.Context, res *Response, extRes.Rooms = make(map[string][]json.RawMessage) for _, ad := range roomsAccountData { extRes.Rooms[ad.RoomID] = append(extRes.Rooms[ad.RoomID], ad.Data) + extRes.loadedRooms[ad.RoomID] = true } } } From 236f065953b2ca23117ac77c42777e8af6d60474 Mon Sep 17 00:00:00 2001 From: Kegan Dougal <7190048+kegsay@users.noreply.github.com> Date: Fri, 12 Apr 2024 08:34:37 +0100 Subject: [PATCH 3/3] Given we only process 1 update in this function, return not break --- sync3/extensions/account_data.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/sync3/extensions/account_data.go b/sync3/extensions/account_data.go index b98f7a12..a84d7f2a 100644 --- a/sync3/extensions/account_data.go +++ b/sync3/extensions/account_data.go @@ -53,7 +53,7 @@ func (r *AccountDataRequest) AppendLive(ctx context.Context, res *Response, extC } case caches.RoomUpdate: if !r.RoomInScope(update.RoomID(), extCtx) { - break + return } // if this is a room update which is included in the response, send account data for this room if _, exists := extCtx.RoomIDToTimeline[update.RoomID()]; exists { @@ -62,7 +62,7 @@ func (r *AccountDataRequest) AppendLive(ctx context.Context, res *Response, extC // we've loaded this room before, don't do it again // this can happen when we consume lots of items in the buffer. If many of them are room updates // for the same room, we could send dupe room account data if we didn't do this check. - break + return } roomAccountData, err := extCtx.Store.AccountDatas(extCtx.UserID, update.RoomID()) if err != nil {