Profile updates stream further improvements

[jaywink]

PR #19556 will add support for profile updates over legacy sync MSC4429, and an incoming pr will add MSC4262 for Sliding Sync. During the work on this feature, several points of concern have been raised on scalability, reliability and future proofing, especially regarding edge cases. In the interest of getting something usable ready, which also gives us data on usage, we're choosing to document and iterate on these points as follow-up.

This issue tracks solving those, in addition to tracking several items that were scoped out during the work on the initial profile updates delivery.

Concerns

While the implementation of #19556 has tried to ensure the data we push to clients is reliable, there are edge cases identified that we believe might a substantial rework of the Synapse implementation. This includes for example:

• The lazy loading cache to avoid sending too many profiles to clients (https://github.com/element-hq/synapse/pull/19556/files/edc8624a64591bf014729f09189f4e2d8b1a6b8f#diff-70c84622edff898486bdc02d7a579db4e52a3e0b26c14020bed0ca1fb8ea2018)
• Various edge cases where a user would be removed from a room that we may miss Introduce support for MSC4429: Profile Updates for Legacy Sync #19556 (comment), Introduce support for MSC4429: Profile Updates for Legacy Sync #19556 (comment))
• We probably want to stop piggybacking on the sliding_sync_table_changes calculated changes for figuring out if memberships have changed or not (https://github.com/element-hq/synapse/pull/19556/changes#r3603767711)
• We shouldn't be generating joined_room profile update rows for display name changes, which currently happens as we're piggybacking on the sliding_sync_table_changes data.
• Performance related to the amount of write-heavy operations needed to write to the per user stream tables (which needs to pull in room memberships), especially regarding the future profile updates over federation.

Scoped out items

• We will need to add purging of profiles updates stream rows from the database, to avoid endless growth there. This was initially in the pull request but later extracted out (see Introduce support for MSC4429: Profile Updates for Legacy Sync #19556 (comment)) to iron out complexity there
• We will need to adjust to match MSC4429 regarding deleted profile fields, see MSC4429: Profile Updates for Legacy Sync matrix-org/matrix-spec-proposals#4429 (review)
  • A branch has some wip code regarding this
• We may want to add sync filtering by user ID, which was scoped out of the initial pull request
• We may want to include profiles of rooms the user joins in the sync responses, as per MSC4429:
  • To bridge this gap, homeservers MAY communicate fields from a user's profiles to a client using the users.<user_id>.profile_updates /sync entry; even if those fields have not been updated recently. This may be useful if, for instance, a user has just joined a room with users that it hasn't seen before. The client should be able to display appropriate profile field information immediately without having to query the homeserver for each new user's profile.
• Connection tracking of sent fields over a sliding sync connection was pulled out in the initial sliding sync pr due to performance concerns. The old branch is here, specifically from this commit onwards.

Other gotchas

Currently only local users get profile updates pushed into the profile updates stream. Federated users are tracked in MSC4259. We may want to consider not having support for federated users a blocker for enabling profile updates over sync on Matrix.org, for example.

[reivilibre]

Just dropping this here, but hope to refine it a bit later on: I started thinking about how we would change the stream a little bit
to:

• avoid the '1 row per room-sharing user' scattering pattern (instead scatter to rooms, like device lists);
• avoid some race conditions/fundamental issues, specifically around relying on event persistence to have a global view of 'do user X and Y share room or not'. Because the events stream is sharded, there's no causal ordering between joins and leaves in different rooms, it's not possible (or not easy?) to correctly track this information there;
• support compaction/pruning without losing updates (forgetting to send data to clients);
• (possibly) make things a little easier for federation.

The data model feels OK, but the sync part of the draft is currently half-baked at best; it feels/seems feasible,
but it will take more thinking to chase down the exact machinery involved and to carefully think through all the cases, including lazy-loading.

It also depends on some changes to the MSC, which have been left as comments. So we'll see how this goes.

WIP

Profile Updates: data model suggestion

Schema

-- Stream of profile deltas
--
-- Each row represents the fact that a user updated
-- (or we became aware of an update to) one or more
-- profile fields.
CREATE TABLE profile_updates_stream (
    stream_id INTEGER NOT NULL PRIMARY KEY, -- or BIGINT?
    instance_name TEXT NOT NULL,

    -- User ID of the user whose profile changed
    user_id TEXT NOT NULL,
    
    -- Set of field names that got added, changed
    -- or removed
    affected_fields TEXT[] NOT NULL, -- not sure if we need to use JSON for SQLite compat or how that works here
    
    -- When this update was persisted.
    -- Used for compaction (and debugging).
    update_ts BIGINT NOT NULL -- or TIMESTAMPTZ DEFAULT NOW()?
);

-- 
CREATE INDEX ON profile_updates_stream (user_id);

-- ? could be a brin
CREATE INDEX ON profile_updates_stream (update_ts);

NOTES:

• multiple field changes: I think it's worth allowing multiple field changes at once. Because when we sync remote profiles into our local store, we will probably do that. I also see us accepting batch-writes in the future because some MSCs already are creating multiple fields that ought to be updated atomically.
  It doesn't seem to make anything more complex to just accept like this.
• concurrent writes to a user: this probably ought to be sharded by user_id so that two writers don't write the same user at the same time.
  Either that or acquire a worker lock?
  Or is it enough to just lock on the profiles table row? Hmm

ALTER TABLE profiles

    -- the `profile_updates` stream ID of the most recent
    -- change to this profile
    -- TODO: Unsure we need this -- actually we likely do, for federation backfill
    ADD COLUMN last_updated_stream_id -- or some better name
    
    -- the `profile_updates` stream ID of the
    -- most recently-compacted change to this profile
    ADD COLUMN last_compacted_stream_id -- or some better name
    ;

NOTES:

• tracking compaction: This lets us know if a reader missed a profile update, even after it gets removed. In that case, we send down a full profile. This is essentially what allows compaction to be safe. Relies on this MSC thread to be sensible.

-- Scatter of profile updates into the rooms from which
-- they are visible.
--
-- This is a 'meet in the middle' approach between
-- the two extremes of:
-- - (read-heavy) all readers gathering all interesting users
-- - (write-heavy) all writers scattering updates to all
--   interested users
CREATE TABLE profile_updates_by_room (
    profile_update_stream_id INTEGER/BIGINT NOT NULL
        REFERENCES profile_updates_stream(stream_id)
            ON DELETE CASCADE,
    
    room_id TEXT NOT NULL
        REFERENCES rooms(room_id)
            ON DELETE CASCADE,
    
    PRIMARY KEY (profile_update_stream_id, room_id)
);

-- For performance when querying what updates happened
-- since a given ID in a given room.
CREATE INDEX ON profile_updates_by_room (room_id, profile_update_stream_id);

NOTES:

• would be nicer if we had room_nids... but that can be fixed later.

Operations

Write a profile update

• Txn:
  • Update profiles
  • Insert into profile_updates_stream with the list of changed fields
  • Insert into profile_updates_by_room to scatter the update
    to all rooms we are a member of

Other than the regular stream notification machinery, I think that's it...?

Compaction (cleanup)

• Find $X = stream_id corresponding to <30 days ago>
• Rows with stream_id < $X get deleted
• Get user_id, MAX(stream_id) GROUP BY user_id amongst the rows to be deleted -> update profiles.last_compacted_stream_id with that info

Oldschool Sync

Similar to the existing lazy-loading cache, for lazy-loading purposes we have a cache that tracks the latest profile_updates stream position we sent down to the client.

Cache:

ExpiringCache[(user_id, device_id)] -> LruCache[user_id -> latest `profile_updates` stream position]

I think this has an idempotence bug (as does the existing one, #19978). Wouldn't be surprised if this will probably survive until oldschool sync goes away.

• For all rooms we're in, read profile_updates_by_room between old and new profile_update_stream_pos
  • → These ALWAYS get emitted as profile deltas over /sync
• Calculate the delta of interesting users
  1. Get rooms we have just joined (newly_joined_rooms)
  2. Get rooms we have just left (newly_left_rooms)
  3. Get users that have just joined rooms we are in, within our sync window (newly_seen_users)
  4. Get users that have just left rooms that we are in or have just left (newly_left_users)
  5. TODO lost my train of thought.
• (non-lazy-loading) For newly-interesting users: Fetch profiles for the relevant users, send them down, no questions asked.
• (lazy-loading) I don't think we actually have to do anything for those newly-interesting users, because we're lazy!
• (lazy-loading) For the heroes + senders in timeline: fetch profiles (as a sequence of deltas...?) and, based on LL cache, emit final delta. If not include_redundant_members(MAYBE?), Update cache.
  • Literally: from cache, get old profile_updates stream_position.
    • Not in cache OR include_redundant_members(?): send full profile
    • In cache: get profile_updates within range. All affected fields = union(affected_fields for all updates). Emit those fields as a delta.
• For no-longer-interesting users: emit profile-unsetting nulls

Sliding Sync

Still mid-thought but:

Let's track what profile position we sent for every lazy member in SS:

ALTER TABLE sliding_sync_connection_lazy_members
    ADD COLUMN sent_profile_updates_position BIGINT NOT NULL DEFAULT 0;

Logic then looks a bit like:

• For all rooms we're in, read profile_updates_by_room between old and new profile_update_stream_pos
  • → These ALWAYS get emitted as profile deltas over SS
  • TODO: how to do pagination?
• Calculate the delta of interesting users
  1. Get rooms we have just joined (newly_joined_rooms)
  2. Get rooms we have just left (newly_left_rooms)
  3. Get users that have just joined rooms we are in, within our sync window (newly_seen_users)
  4. Get users that have just left rooms that we are in or have just left (newly_left_users)
  5. TODO lost my train of thought.
• TODO: below is not fleshed out
• (non-lazy-loading) For newly-interesting users: Fetch profiles for the relevant users, send them down, no questions asked.
• (lazy-loading) I don't think we actually have to do anything for those newly-interesting users, because we're lazy!
• (lazy-loading) For the heroes + senders in timeline: fetch profiles (as a sequence of deltas...?) and, based on LL cache, emit final delta. If not include_redundant_members(MAYBE?), Update cache.
  • Literally: from cache, get old profile_updates stream_position.
    • Not in cache OR include_redundant_members(?): send full profile
    • In cache: get profile_updates within range. All affected fields = union(affected_fields for all updates). Emit those fields as a delta.
• For no-longer-interesting users: emit profile-unsetting nulls

[jaywink]

While working on sliding sync (#20003), I wanted to stop clients receiving users avatar_url: null values. This would happen at least in the following cases:

• initial sync
• incremental sync if a user joins a room
• incremental lazy loading for users with timeline events

Especially due to the latter we're wasting some amount of bandwidth for each user without an avatar, as Synapse defaults to avatar_url being None.

There are various things we could do;

• decide that we don't deliver None values in the profile update streams
  • since EW clears a custom profile field with PUT None (somewhat against MSC4133), we would need to translate those to a delete_profile_field internally.
  • maybe Synapse shouldn't anyway accept None values, as made possible by MSC4133?
• clean out just avatar_url: null before the response.

[jaywink]

Something pulled out of the legacy sync PR, and scoped out of the sliding sync PR, is including users full profile when in lazy loading mode a user knocks a room or gets invited.

For the legacy sync MSC, knocking/inviting is not mentioned. For the sliding sync MSC, those are explicitly mentioned. We should probably want these to have unified behaviour as the knock/invite would need to be recorded similarly for the profile updates stream.

[jaywink]

For the sliding sync pr a guard was introduced to ensure different ProfileUpdateActions don't overwrite each other. As per #20003 (comment) we already do this on the storage layer for LEFT_ROOM, but we don't do it for JOINED_ROOM. We should do both on the storage layer and then simplify the duplicated checks out of the sliding sync sync handler.

[jaywink]

Post-review of #20003 indicated that when a client connection has both lazy and non-lazy room configurations (something EX currently does not, it's always lazy), for incremental sync, we will be doing unnecessary database work at

synapse/synapse/handlers/sliding_sync/extensions.py

Line 1162 in b4a23dd

if non_lazy_rooms:

, due to the fact that incremental code will never utilize the user IDs found via "non-lazy rooms".

This should be fixed by ensuring we don't do room member lookups at all for incremental sync.

[reivilibre]

There appears to be a bug with the lazy-loading cache in oldschool sync, where it will fail to inform the client of certain updates in some circumstances. We would also like some more tests covering the 'deletion' logic for oldschool sync.

Here is a branch with some vibed tests https://github.com/element-hq/synapse/compare/rei%2Fvibe%2Fpr20144-tests?expand=1

LLM output:

  Results

  ┌────────────────────────────────────────────────────────────────────────────┬──────────────────┬─────────────────┬───────────────────────────────────────────────────────────────────────────┐
  │                                    test                                    │ develop (no fix) │      20147      │                                   20144                                   │
  ├────────────────────────────────────────────────────────────────────────────┼──────────────────┼─────────────────┼───────────────────────────────────────────────────────────────────────────┤
  │ 1. test_fields_a_user_never_had_are_not_sent_as_null                       │ PASS             │ PASS            │ FAIL — sends m.status: null                                               │
  ├────────────────────────────────────────────────────────────────────────────┼──────────────────┼─────────────────┼───────────────────────────────────────────────────────────────────────────┤
  │ 2. test_deleted_field_is_communicated_after_the_deletion_leaves_the_window │ FAIL             │ FAIL — sends {} │ PASS                                                                      │
  ├────────────────────────────────────────────────────────────────────────────┼──────────────────┼─────────────────┼───────────────────────────────────────────────────────────────────────────┤
  │ 3. test_initial_and_incremental_sync_agree_on_never_set_fields             │ PASS             │ PASS            │ FAIL — incremental {displayname, m.status: null} vs initial {displayname} │
  ├────────────────────────────────────────────────────────────────────────────┼──────────────────┼─────────────────┼───────────────────────────────────────────────────────────────────────────┤
  │ 4. test_field_change_is_not_suppressed_by_a_stale_lazy_loading_cache       │ FAIL             │ FAIL            │ FAIL — both send {}, client stuck on the old value                        │
  └────────────────────────────────────────────────────────────────────────────┴──────────────────┴─────────────────┴───────────────────────────────────────────────────────────────────────────┘

  Full class: 20147 → 41 pass / 2 of mine fail; 20144 → 39 pass / 3 of mine fail. Both PRs' own tests stay green on their own branch, and (from the earlier cross-run) each PR's new deletion test passes under both handlers — so their tests genuinely don't discriminate; these four do.

  What the tests do, as request sequences

  1. Never-set field isn't null — @other has a displayname and has never had m.status.
  GET /sync ?fields=[displayname,m.status] &lazy                 → T0
  PUT /rooms/!r/send/m.room.message            (as @other)       
  GET /sync ?since=T0 &lazy       → expect {@other: {displayname}}      ← no m.status key
  POST /rooms/!r/join                          (as @third, no m.status ever) 
  GET /sync ?since=T1 (non-lazy)  → expect {@third: {displayname}}      ← no m.status key

  2. Deletion after it leaves the window — the one scenario that justifies 20144.
  PUT    /profile/@other/m.status  {"text":"X"}
  GET    /sync ?fields=[m.status] &lazy     → {@other:{m.status:X}}   T0   (server caches X)
  DELETE /profile/@other/m.status 
  GET    /sync ?since=T0 ?fields=[displayname] &lazy   → {}           T1   (token now past the deletion)
  PUT    /rooms/!r/send                     (as @other)
  GET    /sync ?since=T1 ?fields=[m.status] &lazy  → expect {@other:{m.status:null}}

  3. Initial vs incremental agreement
  GET  /sync ?fields=[displayname,m.status]     → T0
  POST /rooms/!r/join                           (as @third, displayname only)
  GET  /sync ?since=T0                          → incremental view of @third
  GET  /sync (fresh initial)                    → initial view of @third
  assert the two @third dicts are identical

  4. Stale lazy cache suppresses a real change — broken everywhere.
  PUT /profile/@other/m.status {"text":"X"}
  GET /sync ?fields=[m.status] &lazy                     → T0
  PUT /rooms/!r/send  (as @other)
  GET /sync ?since=T0 &lazy   → {m.status:X}   T1   ← lazy path caches X
  PUT /profile/@other/m.status {"text":"Y"}
  GET /sync ?since=T1 &lazy   → {m.status:Y}   T2   ← diff path, cache still says X
  PUT /profile/@other/m.status {"text":"X"}
  PUT /rooms/!r/send  (as @other)
  GET /sync ?since=T2 &lazy   → expect {m.status:X}, got {} — client keeps Y

I suggest ignoring (2) as it relies on changing the fields query. I think that makes it invalid.

The other tests are probably worth adding though. The summary shows a request-by-request sequence, or the linked branch apparently implements them. (unvetted by me, will require polish)

[jaywink]

EDIT: Extracted to #20197

---

There is a caveat in the current implementations regarding lazy loading and non-DM rooms.

Both Element Web and Element X operate in lazy loading mode, which means they will get profiles in sync based on activity. To populate the room membership list, mobile (and web?) when lazy loading call /members to populate members list to fetch users who are not known from timeline or state events. Currently, /members does not return profile information, and at least EX will not do a /profile lookup separate for each user to produce a room membership list.

What this means from the end user perspective: after joining a new room, a user may not see all the statuses of users in the room who previously did not share rooms with them. Users will see new status changes and get old statuses for users who interact with the room.

A possible fix would be to Synapse either (optionally, when asked) add profiles to /members, or a completely new endpoint /profiles to fetch a number of profiles in one go (which clients could call after /members, for the profiles they are missing). Alternatively could be somehow handling this in the sync handlers, though unsure how this could be optimized as the room joined could be very large.

[jaywink]

During #20203 (comment), it was identified that we should be sending profiles down sync when our user joins a room, to avoid clients ending up with stale data, as we in that pr started to properly send down null profiles when we leave a room.

This brings Synapse implementation more in line with MSC4429 which specifically mentions:

To bridge this gap, homeservers MAY communicate fields from a user's profiles to a client using the users.<user_id>.profile_updates /sync entry; even if those fields have not been updated recently. This may be useful if, for instance, a user has just joined a room with users that it hasn't seen before. The client should be able to display appropriate profile field information immediately without having to query the homeserver for each new user's profile.

Also technically one could read this as bringing things more in line with MSC4262, even though we're not reacting to the sliding sync rooms subset changing, so this is more of a happy accident.

When a room enters this subset in this connection for the first time, all requested fields from profiles of users in that room MAY be sent down. This gives the client a base set of information for which future field updates can be applied on top of. The homeserver MAY omit some fields and profiles if it believes that the client has already received them, likewise repeat profiles MAY be sent down based on homeserver implementation.

However, this introduces an issue with the sync payload potentially increasing in size, especially in the lazy loading case, as we now include all room members full profiles in the sync response when our user joins a new room. The problem is somewhat also for when our user leaves a room, but since there we only send a user_id: null for each member, the amount of payload increase is drastically smaller.

We should figure out a potential improvement for this, potentially through MSC changes.