Return some room data in Sliding Sync /sync

[MadLittleMods]

Return some room data in Sliding Sync /sync:

• Timeline events
• Stripped invite_state

Based on MSC3575: Sliding Sync

Dev notes

SYNAPSE_POSTGRES=1 SYNAPSE_POSTGRES_USER=postgres SYNAPSE_TEST_LOG_LEVEL=INFO poetry run trial tests.handlers.test_sliding_sync
SYNAPSE_TEST_LOG_LEVEL=INFO poetry run trial tests.handlers.test_sliding_sync

SYNAPSE_TEST_LOG_LEVEL=INFO poetry run trial tests.rest.client.test_sync.SlidingSyncTestCase

SYNAPSE_TEST_LOG_LEVEL=INFO poetry run trial tests.storage.test_stream

Handle state resets

See #17320 (comment)

Commit: 81c06be

SELECT
    COALESCE(e.event_id, e_by_stream.event_id) AS event_id,
    s.prev_event_id,
    s.room_id,
    s.instance_name,
    COALESCE(e.stream_ordering, e_by_stream.stream_ordering, s.stream_id) AS stream_ordering,
    COALESCE(m.membership, m_by_stream.membership) AS membership,
    COALESCE(e.sender, e_by_stream.sender) AS sender,
    m_prev.membership AS prev_membership
FROM current_state_delta_stream AS s
    LEFT JOIN events AS e ON e.event_id = s.event_id
    LEFT JOIN room_memberships AS m ON m.event_id = s.event_id
    LEFT JOIN room_memberships AS m_prev ON s.prev_event_id = m_prev.event_id
    LEFT JOIN events AS e_by_stream ON e_by_stream.stream_ordering = s.stream_id
        AND e_by_stream.type = ?
        AND e_by_stream.state_key = ?
    LEFT JOIN room_memberships AS m_by_stream ON m_by_stream.event_stream_ordering = s.stream_id
        AND m_by_stream.user_id = ?
WHERE s.stream_id > ? AND s.stream_id <= ?
    AND s.type = ?
    AND s.state_key = ?
ORDER BY s.stream_id ASC

Other references

Dumping database tables during tests:

events_db_dump = self.get_success(
    self.store.db_pool.simple_select_list(
        table="events",
        keyvalues={},
        retcols=["*"],
        desc="debug dump events",
    )
)

logger.info("events_db_dump: %s", events_db_dump)

current_state_delta_stream_db_dump = self.get_success(
    self.store.db_pool.simple_select_list(
        table="current_state_delta_stream",
        keyvalues={},
        retcols=["*"],
        desc="debug dump current_state_delta_stream",
    )
)

logger.info(
    "current_state_delta_stream_db_dump: %s", current_state_delta_stream_db_dump
)

Token comparison:

is_before_or_eq
persisted_after
wait_for_stream_token

---

Membership:

get_rooms_for_local_user_where_membership_is(...)
get_membership_changes_for_user(...)

---

State:

get_state_at(...)

Potential name: get_current_state_at(...)
Potential name: get_membership_in_rooms_at_token_for_user(...)

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]

Removing include_old_rooms because we will opt to always include old rooms (easiest option).

[MadLittleMods]

Just updating this field to match the MSC3575. next_pos sounds better to me but I assume pos was originally chosen to save on bytes and this can throw this in the bikeshed for later.

[MadLittleMods]

Does newly_joined affect limited? It does in sync v2 but I fail to understand why. The logic there is also messy and redundant.

limited also plays into bundled_aggregations which mentions "This is only calculated if limited is true." but the logic to decide to bundle aggregations vs limited is not the same although I think it may be equivalent because of the redundant/messy nature of the logic there.

[erikjohnston]

I think we also need to set limited to true if newly_joined. We need to handle the case where we've done a remote join (and so paginate events will only return the one join event), and we want limited to be true so clients not to paginate (and so trigger backfill).

We also need to handle the timeline_gaps logic from sync v2 as well. But that can wait.

[MadLittleMods]

I think the newly_joined remote room case would be handled by checking for timeline gaps which I'll be tackling in a follow-up (already has a TODO).

I can't think of a reason why newly_joined would affect limited specifically though.

[MadLittleMods]

Reviewer, please double-check the limited and prev_batch_token logic.

It's unclear how these fields should be set in cases where the user's room membership is invite with shared/joined history visibility where they can't see any timeline events until they join.

Where should the prev_batch token be pointing to? If we do it just like other rooms, we paginate up to the timeline_limit, then all of those events are filtered out (because we're not joined yet) and the prev_batch token points to a place before those events that we never saw. And then potentially when we join, we will miss those events depending on what the client does.

If we change the scenario to a room with world_readable or invited history visibility where timeline events would be returned when you are invited; The answer is timeline events are returned, limited is set respectively, and prev_batch points to the first event to continue paginating (no problems there).

For ban/leave, we can just query for timeline events up to the point where they were banned/left (no problem here either).

---

To compare, Sync v2 doesn't have to deal with this invite memberhip problem because rooms are separated to their own fields (invite, join, knock, leave) in the sync response and invite rooms don't include timeline events or limited, etc (only stripped state).

Perhaps I'm making a leap too far in the Sliding Sync response and invite membership rooms should only have invite_state set (no timeline or limited). This may be the dumb-simple approach which I've only thought about now after writing all of this and isn't spelled out in the MSC. It feels like we could make rooms in the Sliding Sync response more uniform if we decide on some semantics here.

[erikjohnston]

Yeah, ermh, I'd ask what the proxy does. My hunch is that we don't return any timeline events (bar maybe the invite?), and just set prev_batch to the just before the invite. I'd guess clients don't actually use it, as the api will send down a new room entry for when we joined the room?

[MadLittleMods]

I asked what the Sliding Sync proxy does for invite rooms: it only returns invite_state (no timeline, limited, or prev_batch).

Because the proxy is just a Matrix client, it can basically only pass along events from Sync v2 so it's not able to pull out extra events and have some filtering logic or history visibility checks on them. I also asked whether a full server implementation of Sliding Sync should do something more/better in the invite situation but I only got one response back about preferring to keep the already complex sync endpoint to just be for syncing live traffic, and defer loading history to a separate endpoint.

That sentiment makes some sense. Especially, if we were to return strictly live traffic only but the sync endpoint is already loading history.

I assume the history is useful for room previews and being able to show something immediately. Ultimately, we're leaving that decision is also up to the client depending on the timeline_limit and filters they want to use anyway.

Deciding on how we treat invite rooms, doesn't really change how sync works in this sense though. Returning live traffic for invite rooms where you can see the messages also could make sense. And history is returned during an initial sync like any other room.

⏩ Since nothing was decided and there wasn't traction either way, I'll just copy the proxy for now and only have invite_state. I've also brought up this point on the MSC for wider discussion and to be clarified -> matrix-org/matrix-spec-proposals#3575 (comment)

[MadLittleMods]

We're deciding to make name optional because the m.room.name state event is not required and it doesn't make sense to calculate room names that need to be localized on the client anyway.

[MadLittleMods]

CI keeps failing on building debs. It's unclear what's even going wrong or a change to cause this and I'm unable to merge with the failing status. It was building fine on the develop merge.

https://github.com/element-hq/synapse/actions/runs/9754628604/job/26922590117

...

Building wheels for collected packages: cffi, jaeger-client, markupsafe, opentracing, psycopg2, pyicu, systemd-python, threadloop, thrift, zope-interface
  Building wheel for cffi (pyproject.toml): started
  error: subprocess-exited-with-error
  
  × Building wheel for cffi (pyproject.toml) did not run successfully.
  │ exit code: 1
  ╰─> See above for output.
  
  note: This error originates from a subprocess, and is likely not a problem with pip.
  Building wheel for cffi (pyproject.toml): finished with status 'error'
  ERROR: Failed building wheel for cffi

....

Failed to build cffi
ERROR: ERROR: Failed to build installable wheels for some pyproject.toml based projects (cffi)
Traceback (most recent call last):
  File "/usr/bin/dh_virtualenv", line 111, in <module>
    sys.exit(main() or 0)
             ^^^^^^
  File "/usr/bin/dh_virtualenv", line 91, in main
    deploy.install_dependencies()
  File "/usr/lib/python3/dist-packages/dh_virtualenv/deployment.py", line 202, in install_dependencies
    subprocess.check_call(self.pip('-r', requirements_path))
  File "/usr/lib/python3.12/subprocess.py", line 413, in check_call
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['/synapse/build/debian/matrix-synapse-py3/opt/venvs/matrix-synapse/bin/python', '/synapse/build/debian/matrix-synapse-py3/opt/venvs/matrix-synapse/bin/pip', 'install', '--log=/tmp/tmp1je1k3qf', '--no-deps', '--no-cache-dir', '--compile', '-r', './exported_requirements.txt']' returned non-zero exit status 1.
make[1]: Leaving directory '/synapse/build'
make[1]: *** [debian/rules:57: override_dh_virtualenv] Error 1
make: *** [debian/rules:66: binary] Error 2
dpkg-buildpackage: error: debian/rules binary subprocess returned exit status 2
build of debian:sid failed: Command '['docker', 'run', '--rm', '--name', 'synapse_build_sid', '--volume=/home/runner/work/synapse/synapse/src:/synapse/source:ro', '--volume=/home/runner/work/synapse/synapse/src/../debs:/debs', '-e', 'TARGET_USERID=1001', '-e', 'TARGET_GROUPID=127', '-e', 'DEB_BUILD_OPTIONS=', 'dh-venv-builder:sid']' returned non-zero exit status 2.
Traceback (most recent call last):
  File "/home/runner/work/synapse/synapse/./src/scripts-dev/build_debian_packages.py", line 225, in <module>
    run_builds(
  File "/home/runner/work/synapse/synapse/./src/scripts-dev/build_debian_packages.py", line 182, in run_builds
    for _ in res:
  File "/opt/hostedtoolcache/Python/3.12.4/x64/lib/python3.12/concurrent/futures/_base.py", line 619, in result_iterator
    yield _result_or_cancel(fs.pop())
          ^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/opt/hostedtoolcache/Python/3.12.4/x64/lib/python3.12/concurrent/futures/_base.py", line 317, in _result_or_cancel
    return fut.result(timeout)
           ^^^^^^^^^^^^^^^^^^^
  File "/opt/hostedtoolcache/Python/3.12.4/x64/lib/python3.12/concurrent/futures/_base.py", line 449, in result
    return self.__get_result()
           ^^^^^^^^^^^^^^^^^^^
  File "/opt/hostedtoolcache/Python/3.12.4/x64/lib/python3.12/concurrent/futures/_base.py", line 401, in __get_result
    raise self._exception
  File "/opt/hostedtoolcache/Python/3.12.4/x64/lib/python3.12/concurrent/futures/thread.py", line 58, in run
    result = self.fn(*self.args, **self.kwargs)
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/runner/work/synapse/synapse/./src/scripts-dev/build_debian_packages.py", line 179, in <lambda>
    res = e.map(lambda dist: builder.run_build(dist, skip_tests), dists)
                             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/runner/work/synapse/synapse/./src/scripts-dev/build_debian_packages.py", line 70, in run_build
    self._inner_build(dist, skip_tests)
  File "/home/runner/work/synapse/synapse/./src/scripts-dev/build_debian_packages.py", line 123, in _inner_build
    subprocess.check_call(
  File "/opt/hostedtoolcache/Python/3.12.4/x64/lib/python3.12/subprocess.py", line 413, in check_call
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['docker', 'run', '--rm', '--name', 'synapse_build_sid', '--volume=/home/runner/work/synapse/synapse/src:/synapse/source:ro', '--volume=/home/runner/work/synapse/synapse/src/../debs:/debs', '-e', 'TARGET_USERID=1001', '-e', 'TARGET_GROUPID=127', '-e', 'DEB_BUILD_OPTIONS=', 'dh-venv-builder:sid']' returned non-zero exit status 2.

If I run scripts-dev/build_debian_packages.py locally, I see a different error (even on develop) which is a bit confusing because I'd assume it should behaving about the same given it's all running in Docker.

Local build output

...
Successfully installed matrix-synapse-1.110.0rc2
/ /synapse/build
Running 0 tests.
E
===============================================================================
[ERROR]
Traceback (most recent call last):
  File "/synapse/build/debian/matrix-synapse-py3/opt/venvs/matrix-synapse/lib/python3.9/site-packages/twisted/trial/runner.py", line 711, in loadByName
    return self.suiteFactory([self.findByName(name, recurse=recurse)])
  File "/synapse/build/debian/matrix-synapse-py3/opt/venvs/matrix-synapse/lib/python3.9/site-packages/twisted/trial/runner.py", line 474, in findByName
    obj = reflect.namedModule(searchName)
  File "/synapse/build/debian/matrix-synapse-py3/opt/venvs/matrix-synapse/lib/python3.9/site-packages/twisted/python/reflect.py", line 156, in namedModule
    topLevel = __import__(name)
  File "/tmp/tmp.DNj5v3i4RS/tests/__init__.py", line 24, in <module>
    from synapse.util.patch_inline_callbacks import do_patch
  File "/synapse/build/debian/matrix-synapse-py3/opt/venvs/matrix-synapse/lib/python3.9/site-packages/synapse/__init__.py", line 32, in <module>
    from synapse.util.rust import check_rust_lib_up_to_date
  File "/synapse/build/debian/matrix-synapse-py3/opt/venvs/matrix-synapse/lib/python3.9/site-packages/synapse/util/rust.py", line 27, in <module>
    from synapse.synapse_rust import get_rust_file_digest
builtins.ImportError: /lib/x86_64-linux-gnu/libc.so.6: version `GLIBC_2.33' not found (required by /synapse/build/debian/matrix-synapse-py3/opt/venvs/matrix-synapse/lib/python3.9/site-packages/synapse/synapse_rust.abi3.so)

tests
-------------------------------------------------------------------------------
Ran 1 tests in 0.000s

FAILED (errors=1)
make[1]: *** [debian/rules:57: override_dh_virtualenv] Error 1
make[1]: Leaving directory '/synapse/build'
make: *** [debian/rules:66: binary] Error 2
dpkg-buildpackage: error: debian/rules binary subprocess returned exit status 2
build of debian:bullseye failed: Command '['docker', 'run', '--rm', '--name', 'synapse_build_bullseye', '--volume=/home/eric/Documents/github/element/synapse:/synapse/source:ro', '--volume=/home/eric/Documents/github/element/synapse/../debs:/debs', '-e', 'TARGET_USERID=1000', '-e', 'TARGET_GROUPID=1000', '-e', 'DEB_BUILD_OPTIONS=', 'dh-venv-builder:bullseye']' returned non-zero exit status 2.
not building debian:bookworm due to earlier failure
not building debian:sid due to earlier failure
not building ubuntu:focal due to earlier failure
not building ubuntu:jammy due to earlier failure
not building ubuntu:lunar due to earlier failure
not building ubuntu:mantic due to earlier failure
not building debian:trixie due to earlier failure
Traceback (most recent call last):
  File "/home/eric/Documents/github/element/synapse/scripts-dev/build_debian_packages.py", line 225, in <module>
    run_builds(
  File "/home/eric/Documents/github/element/synapse/scripts-dev/build_debian_packages.py", line 182, in run_builds
    for _ in res:
  File "/usr/lib/python3.12/concurrent/futures/_base.py", line 619, in result_iterator
    yield _result_or_cancel(fs.pop())
          ^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.12/concurrent/futures/_base.py", line 317, in _result_or_cancel
    return fut.result(timeout)
           ^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.12/concurrent/futures/_base.py", line 449, in result
    return self.__get_result()
           ^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.12/concurrent/futures/_base.py", line 401, in __get_result
    raise self._exception
  File "/usr/lib/python3.12/concurrent/futures/thread.py", line 58, in run
    result = self.fn(*self.args, **self.kwargs)
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/eric/Documents/github/element/synapse/scripts-dev/build_debian_packages.py", line 179, in <lambda>
    res = e.map(lambda dist: builder.run_build(dist, skip_tests), dists)
                             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/eric/Documents/github/element/synapse/scripts-dev/build_debian_packages.py", line 70, in run_build
    self._inner_build(dist, skip_tests)
  File "/home/eric/Documents/github/element/synapse/scripts-dev/build_debian_packages.py", line 123, in _inner_build
    subprocess.check_call(
  File "/usr/lib/python3.12/subprocess.py", line 413, in check_call
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['docker', 'run', '--rm', '--name', 'synapse_build_bullseye', '--volume=/home/eric/Documents/github/element/synapse:/synapse/source:ro', '--volume=/home/eric/Documents/github/element/synapse/../debs:/debs', '-e', 'TARGET_USERID=1000', '-e', 'TARGET_GROUPID=1000', '-e', 'DEB_BUILD_OPTIONS=', 'dh-venv-builder:bullseye']' returned non-zero exit status 2.

[erikjohnston]

I've just rerun this on develop and it looks like its broken there too

[erikjohnston]

#17389 was the issue in CI

#17390 was the issue running locally

[MadLittleMods]

Thanks for fixing up the deb build issues @erikjohnston!

For the first one, the fact that it was building a wheel at all is the hint as it should be downloading one normally.

For the second one, it makes more sense but if we expect a clean checkout for the Docker builds, it would be nice to remove things according to the .gitignore.

[MadLittleMods]

Thanks for the great review and CI unblocks @erikjohnston 🦜