Use fully-qualified PersistedEventPosition when returning RoomsForUser

[MadLittleMods]

Use fully-qualified PersistedEventPosition (instance_name and stream_ordering) when returning RoomsForUser to facilitate proper comparisons and RoomStreamToken generation.

Spawning from #17187 where we want to utilize this change

Pull Request Checklist

• Pull request is based on the develop branch
• Pull request includes a changelog file. The entry should:
  • Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from EventStore to EventWorkerStore.".
  • Use markdown where necessary, mostly for code blocks.
  • End with either a period (.) or an exclamation mark (!).
  • Start with a capital letter.
  • Feel free to credit yourself, by adding a sentence "Contributed by @github_username." or "Contributed by [Your Name]." to the end of the entry.
• Code style is correct
  (run the linters)

[MadLittleMods]

Moving ShutdownRoomParams and ShutdownRoomResponse in this PR is a random change but I wasn't able to run the tests because of the circular import here.

SYNAPSE_POSTGRES=1 SYNAPSE_POSTGRES_USER=postgres SYNAPSE_TEST_LOG_LEVEL=INFO poetry run trial tests.replication.storage.test_events
[...]
Traceback (most recent call last):
  File "pypoetry/virtualenvs/matrix-synapse-xCtC9ulO-py3.12/lib/python3.12/site-packages/twisted/trial/runner.py", line 711, in loadByName
    return self.suiteFactory([self.findByName(name, recurse=recurse)])
  File "pypoetry/virtualenvs/matrix-synapse-xCtC9ulO-py3.12/lib/python3.12/site-packages/twisted/trial/runner.py", line 474, in findByName
    obj = reflect.namedModule(searchName)
  File "pypoetry/virtualenvs/matrix-synapse-xCtC9ulO-py3.12/lib/python3.12/site-packages/twisted/python/reflect.py", line 156, in namedModule
    topLevel = __import__(name)
  File "synapse/tests/replication/storage/test_events.py", line 33, in <module>
    from synapse.handlers.room import RoomEventSource
  File "synapse/synapse/handlers/room.py", line 74, in <module>
    from synapse.rest.admin._base import assert_user_is_admin
  File "synapse/synapse/rest/__init__.py", line 24, in <module>
    from synapse.rest import admin
  File "synapse/synapse/rest/admin/__init__.py", line 41, in <module>
    from synapse.handlers.pagination import PURGE_HISTORY_ACTION_NAME
  File "synapse/synapse/handlers/pagination.py", line 30, in <module>
    from synapse.handlers.room import ShutdownRoomParams, ShutdownRoomResponse
builtins.ImportError: cannot import name 'ShutdownRoomParams' from partially initialized module 'synapse.handlers.room' (most likely due to a circular import) (synapse/synapse/handlers/room.py)

[MadLittleMods]

On matrix.org, my /sync token looks like s4983244229_757284974_13038521_2867177461_3105561071_197466693_1334422313_10818115877_0_293162. I assume we use multiple event persisters on matrix.org so I would expect it to see the m{min_pos}~{writer1}.{pos1}~{writer2}.{pos2}. ... variant of RoomStreamToken at the front? What's happening there?

Relevant docs:

synapse/synapse/types/__init__.py

Lines 585 to 605 in 8a32700

	There is also a third mode for live tokens where the token starts with "m",
	which is sometimes used when using sharded event persisters. In this case
	the events stream is considered to be a set of streams (one for each writer)
	and the token encodes the vector clock of positions of each writer in their
	respective streams.
	The format of the token in such case is an initial integer min position,
	followed by the mapping of instance ID to position separated by '.' and '~':
	m{min_pos}~{writer1}.{pos1}~{writer2}.{pos2}. ...
	The `min_pos` corresponds to the minimum position all writers have persisted
	up to, and then only writers that are ahead of that position need to be
	encoded. An example token is:
	m56~2.58~3.59
	Which corresponds to a set of three (or more writers) where instances 2 and
	3 (these are instance IDs that can be looked up in the DB to fetch the more
	commonly used instance names) are at positions 58 and 59 respectively, and
	all other instances are at position 56.

[MadLittleMods]

hmm, actually I see both used:

Ex.
m4982500302~37.4982500309~3.4982500309~2.4982500309_757284974_11788273_2866394521_3104790542_197391253_1334263147_10815910062_0_293003

Perhaps, this just depends on whether the now_token in sync v2 was updated in-place although I see rooms data in the response when we get either type of token 🤷

This isn't an initial vs incremental sync thing. It changes back-and-forth through the life-time of my sync loop.

[MadLittleMods]

Do we need to (should we) be more careful about crafting the RoomStreamToken in all of the places here? (do they need instance_map?)

I know in #17187, we talked about constructing the RoomStreamToken using the instance_map to be ultra correct.

I'm racking my brain on each of these locations. My hunch, is that we should be pay closer attention and add the instance_map where RoomStreamToken(stream=room.event_pos.stream, instance_map={ <max-stream-of-each-instance-in-get_rooms_for_local_user_where_membership_is-results> })

Is this something to push off to a more broad, holistic update to how we craft stream tokens everywhere? It's at least as good as it was before in any case.

[MadLittleMods]

I know there was something recent about changing all of the streams to MultiWriterIdGenerator for some guarantee (they don't go backwards, or something) (edit: see #17226). Just want to make sure whether it's ok to define RoomStreamToken(..., instance_map={...}) even if we're not using multiple event persisters. Any kind of check we need before adding instance_map? The closest thing I can find is in synapse/storage/databases/main/stream.py#L565 for example.

[erikjohnston]

🚀

[MadLittleMods]

Thanks for the review @erikjohnston 🦁