Introduce support for MSC4429: Profile Updates for Legacy Sync#19556
Introduce support for MSC4429: Profile Updates for Legacy Sync#19556anoadragon453 wants to merge 115 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Implements experimental MSC4429 support by introducing a new profile_updates stream (persisted + replicated) and surfacing profile field changes in legacy /sync when enabled.
Changes:
- Add
profile_updatesstream tracking (DB schema, stream token field, notifier + replication stream plumbing). - Emit MSC4429 profile updates in
/syncresponses and add filter support for selecting profile fields. - Record profile field updates on profile mutations (set/unset displayname/avatar/custom fields, deactivation cleanup).
Reviewed changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/rest/client/test_rooms.py | Updates hardcoded stream tokens to match the new StreamToken shape. |
| tests/rest/admin/test_room.py | Updates hardcoded stream tokens to match the new StreamToken shape. |
| synapse/types/init.py | Adds PROFILE_UPDATES stream key and extends StreamToken serialization/parsing. |
| synapse/streams/events.py | Includes profile_updates_key in current/bounded stream tokens. |
| synapse/storage/schema/main/delta/94/01_profile_updates_seq.sql.postgres | Adds Postgres sequence for the new stream. |
| synapse/storage/schema/main/delta/94/01_profile_updates.sql | Adds profile_updates table + indexes. |
| synapse/storage/schema/init.py | Bumps schema version to 94 and documents the change. |
| synapse/storage/databases/main/profile.py | Adds profile update persistence/query APIs and stream ID generator wiring. |
| synapse/rest/client/versions.py | Advertises org.matrix.msc4429 in unstable_features. |
| synapse/rest/client/sync.py | Encodes MSC4429 profile update payload into /sync response. |
| synapse/replication/tcp/streams/_base.py | Adds ProfileUpdatesStream replication stream. |
| synapse/replication/tcp/streams/init.py | Registers ProfileUpdatesStream for replication. |
| synapse/replication/tcp/handler.py | Replicates ProfileUpdatesStream from configured writers. |
| synapse/replication/tcp/client.py | Handles incoming profile update replication and notifies listeners. |
| synapse/notifier.py | Allows StreamKeyType.PROFILE_UPDATES to wake /sync waiters. |
| synapse/handlers/sync.py | Generates initial + incremental MSC4429 profile update sections in /sync. |
| synapse/handlers/profile.py | Records profile update markers on profile mutations and notifies. |
| synapse/config/experimental.py | Adds msc4429_enabled config flag. |
| synapse/api/filtering.py | Adds /sync filter support for selecting profile fields (MSC4429-gated). |
| docker/complement/conf/workers-shared-extra.yaml.j2 | Enables MSC4429 in complement worker test config. |
| changelog.d/19556.feature | Adds changelog entry for MSC4429 experimental support. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
anoadragon453
force-pushed
the
anoa/msc4429
branch
2 times, most recently
from
83e699f to
a846945
Compare
anoadragon453
force-pushed
the
anoa/msc4429
branch
from
dc8e9f8 to
a309372
Compare
We create a stream table dedicated to tracking profile updates. The sync machinary can this stream to understand whether a profile update needs to be included in a client's sync response.
Add a new stream type for profile updates. This allows sync processes to determine which profile updates a given user has or hasn't seen yet.
Replicate changes to the profile updates stream so that sync workers know there's a new profile update, and wake up sync waiters accordingly. Updates are keyed by `room_id` - sync waiters should only wake up if a user they share a room with updates their profile.
Based on the provided since token and filter.
And update relevant documentation/integration test config.
Prevent the `ProfileRestServlet` from handling requests intended for the `ProfileFieldRestServlet`. If a requester tried to call PUT `.../profile/@user:domain/(key_id?)` a worker that did not mount `ProfileFieldRestServlet`, they would then receive a `405 Method Not Allowed` instead of the expected `404 Not Found`.
anoadragon453
force-pushed
the
anoa/msc4429
branch
from
a309372 to
f27ca70
Compare
|
I believe this is ready for a new look. Thanks for all the reviews so far! 🙏 |
jaywink
changed the title
… updates Otherwise if the user has multiple clients doing incremental sync, they wont get their own profile updates to their other clients. For initial because it should probably match the incremental behaviour + it avoids clients needing to look up the profile separately potentially.
Co-authored-by: Olivier 'reivilibre' <oliverw@element.io>
reivilibre
left a comment
reivilibre
left a comment
There was a problem hiding this comment.
This broadly seems on the right track, but I'm left wondering if there isn't some light strengthening we can make to the data model and the worker architecture (if you will) to make some of the more painful corners go away.
The sync logic is also maybe a little gnarly (to me), but that could be par for the course with sync.
I think if I was doing this again, the sync portion would probably be better as its own PR separate from the introduction of a new stream (as tricky as that could be) as there is a LOT to digest in this one PR.
| await self.store.track_profile_updates_per_user( | ||
| stream_id=stream_id, | ||
| user_ids=users_who_share_rooms, | ||
| ) |
There was a problem hiding this comment.
it feels like this should be done in the same transaction as adding the profile update, as it again feels like the update can be lost half-way?
(Maybe I'm missing something as to why it doesn't matter)
| membership=Membership.LEAVE, | ||
| ) | ||
|
|
||
| elif self._msc4429_enabled and event.membership == Membership.JOIN: |
There was a problem hiding this comment.
I think this might trigger for name changes. I haven't looked carefully if it's safe to do so, but I feel like this probably needs to consider the previous membership.
|
|
||
| def get_lazy_loaded_profile_fields_cache( | ||
| self, cache_key: tuple[str, str | None, str, str] | ||
| ) -> LruCache[str, str]: |
There was a problem hiding this comment.
needs docstring, especially with a difficult signature like this
| user_id: {"profile_updates": updates} | ||
| for user_id, updates in sync_result.profile_updates.items() | ||
| } | ||
|
|
There was a problem hiding this comment.
(no good place to anchor this comment)
I think we are missing some tests for the actual wiring of this logic into sync itself.
In general I think we would benefit from a few API-level ('rest') sync tests against the profiles functionality.
Cases that seem relatively untested atm:
- filters (that they actually exclude data)
- (I guess, based on the review) returning falsy-valued data — though maybe that starts getting painful
And in general, non-sync cases that I think would benefit from coverage; I guess this is more profile-update related, but doing it at the 'rest' level might make sense:
- exercising join → leave → re-join → leave to try and rule out bugs in that aspect
- (from the review?) test that a displayname-only change doesn't write JOINED entries to all rooms
| ) | ||
| cache = self.get_lazy_loaded_profile_fields_cache(cache_key) | ||
| # Only send the field if we haven't recently sent it | ||
| if cache.get(other_user_id) is None: |
There was a problem hiding this comment.
I'm struggling to wrap my head around it as the interactions are quite complex, but doesn't this mean 'did we send this field (for any value) recently as a lazy-loaded user that sent a message in the timeline'?
If so, what happens if you update your field; does this (wrongly?) quash the update?
Maybe this points to a missing test along the lines of:
- syncing user syncs (initial sync, lazy loading)
- user sets profile field, sends event in timeline
- syncing user syncs (lazy loading), gets the profile value, this adds the user to their lazy-loading 'profile has been sent' cache
- user updates profile field, sends another event in timeline (?)
- syncing user syncs (lazy loading), check they get the NEW profile value [I think currently they won't]
Co-authored-by: Olivier 'reivilibre' <oliverw@element.io>
…fferent enum values mean.
…ting profile updates from timeline events
This PR implements support for MSC4429: Profile Updates for Legacy Sync.
Recommended to review commit-by-commit.Commit history has become messy, suggest reviewing as a single item.Paired with matrix-org/complement#849 and branch https://github.com/matrix-org/sytest/tree/anoa/msc4429
Tracking issue for removing unstable identifiers in Synapse: #19891
Pull Request Checklist
EventStoretoEventWorkerStore.".code blocks.