Cache `_get_e2e_cross_signing_signatures_for_devices` #18899

anoadragon453 · 2025-09-08T16:18:58Z

This PR adds a cache to _get_e2e_cross_signing_signatures_for_devices to put less pressure on the database.

Recommended to review commit-by-commit.

Requires matrix-org/sytest#1409 for Sytest CI to pass.

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)

anoadragon453 · 2025-09-08T16:54:04Z

This needs a small change. I had missed that functions decorated with @cachedList must return a dict with keys matching the original collection items.

This test started to fail after implementing element-hq/synapse#18899. The failure appeared to be due to the test not waiting for signatures to propagate over federation. The loop would exit as soon as it saw a 'signatures' key. But the value was an empty dict. A few moments later, the dict would be populated with the key we were waiting for. But while that was being sent over federation, the test would fail and exit abruptly. This commit changes the loop to actually check for the signature we're waiting for, instead of exiting upon seeing the 'signatures' key. This seems like it would be less flaky in general, and prevents the test from failing in this case.

The use of `@cachedList` requires having a cached, equivalent "single item" method that represents the key and cached value. Calls to methods with @cachedList are then processed and items that are already cached are removed before the @cachedList-decorated method is called. The single item method should not be directly used. The return type needed to be changed to a "Mapping" containing a "Sequence" to satisfy the requirement that cached results must be immutable (lest you risk mutating the cache). `@cachedList` also requires returning an dict where the keys matches that of the items in the cached input list. Our keys are tuples (and you'll later see why this is a pain).

The return type has changed, and we now need to handle a map of user_id/device_id tuples to lists of key signatures.

This is needed to then call `_invalidate_cache_and_stream_bulk with the `txn` object in the next commit.

Invalidate and stream whenever we update `e2e_cross_signing_signatures`.

As the "keys" in an incoming cache invalidation stream object appear to always be a string, we have to do some special case handling to convert the string into a tuple of (user_id, device_id). This works, but is awful.

synapse/storage/databases/main/cache.py

reivilibre

Seems overall OK though; are there any tests we should add (I guess cache invalidation would be on my mind)?

synapse/storage/databases/main/cache.py

synapse/storage/databases/main/end_to_end_keys.py

Instead of encoding them using a brittle, custom scheme. Avoids errors where the device ID or user ID contains a comma.

Required defining a three-column overload for the method to pass mypy. Additionally the type-ignore was no longer required according to mypy.

Since `_get_e2e_cross_signing_signatures_for_devices` always return a key for every entry in `device_query`, there will never be a case where the function can return `None`, even when decorated with `@cacheList`.

reivilibre · 2025-09-18T10:35:51Z

synapse/storage/databases/main/end_to_end_keys.py

+                    (json.dumps([user_id, item.target_device_id]),)
+                    for item in signatures


not possible to json.dumps the entries of to_invalidate?

You can; it looks like this:

to_send = [ (json.dumps(item[0]),) for item in to_invalidate ]

which seemed less clear as to what we're actually sending than what's there now.

anoadragon453 · 2025-09-18T11:07:25Z

are there any tests we should add (I guess cache invalidation would be on my mind)?

Ah, I missed this. Sytest does fail (specifically uploading signed devices gets propagated over federation) if the cache invalidation does not work. This is what allowed me to catch the lack of being able to send nested data structures over replication in the first place.

anoadragon453 marked this pull request as ready for review September 8, 2025 16:20

anoadragon453 requested a review from a team as a code owner September 8, 2025 16:20

anoadragon453 marked this pull request as draft September 8, 2025 16:52

reivilibre removed the request for review from a team September 9, 2025 10:32

anoadragon453 force-pushed the anoa/cache_device_key_signatures branch 4 times, most recently from 7ba742b to 8811a86 Compare September 11, 2025 12:55

anoadragon453 mentioned this pull request Sep 12, 2025

Deflake 'uploading signed devices gets propagated over federation' matrix-org/sytest#1409

Merged

anoadragon453 force-pushed the anoa/cache_device_key_signatures branch from 2df4e22 to 834662d Compare September 16, 2025 17:13

anoadragon453 added 5 commits September 17, 2025 11:07

Update the calling site

325632f

The return type has changed, and we now need to handle a map of user_id/device_id tuples to lists of key signatures.

Make store_e2e_cross_signing_signatures use a custom txn func

6ffd8b9

This is needed to then call `_invalidate_cache_and_stream_bulk with the `txn` object in the next commit.

Invalidate cross signing signatures cache

1ea7cf0

Invalidate and stream whenever we update `e2e_cross_signing_signatures`.

Special-case handling of cache invalidation stream

21c2c04

As the "keys" in an incoming cache invalidation stream object appear to always be a string, we have to do some special case handling to convert the string into a tuple of (user_id, device_id). This works, but is awful.

anoadragon453 force-pushed the anoa/cache_device_key_signatures branch from 834662d to 983f850 Compare September 17, 2025 10:49

anoadragon453 marked this pull request as ready for review September 17, 2025 10:49

anoadragon453 added 3 commits September 17, 2025 11:52

Clarify @cachedList None-caching behaviour

b4ff74c

newsfile

3ec9f16

lint

a096967

anoadragon453 force-pushed the anoa/cache_device_key_signatures branch from 983f850 to a096967 Compare September 17, 2025 10:52

anoadragon453 requested a review from a team September 17, 2025 11:08

anoadragon453 commented Sep 17, 2025

View reviewed changes

synapse/storage/databases/main/cache.py Outdated Show resolved Hide resolved

reivilibre requested changes Sep 17, 2025

View reviewed changes

synapse/storage/databases/main/cache.py Outdated Show resolved Hide resolved

synapse/storage/databases/main/end_to_end_keys.py Outdated Show resolved Hide resolved

synapse/storage/databases/main/end_to_end_keys.py Show resolved Hide resolved

JSON encode replication keys

c6534e6

Instead of encoding them using a brittle, custom scheme. Avoids errors where the device ID or user ID contains a comma.

anoadragon453 added 2 commits September 17, 2025 18:44

Use make_tuple_in_list_sql_clause

90387cc

Required defining a three-column overload for the method to pass mypy. Additionally the type-ignore was no longer required according to mypy.

Remove None handling

e7ca69d

Since `_get_e2e_cross_signing_signatures_for_devices` always return a key for every entry in `device_query`, there will never be a case where the function can return `None`, even when decorated with `@cacheList`.

anoadragon453 force-pushed the anoa/cache_device_key_signatures branch from b037d41 to e7ca69d Compare September 17, 2025 17:50

anoadragon453 requested a review from reivilibre September 17, 2025 17:50

reivilibre approved these changes Sep 18, 2025

View reviewed changes

anoadragon453 merged commit b596faa into develop Sep 18, 2025
44 checks passed

anoadragon453 deleted the anoa/cache_device_key_signatures branch September 18, 2025 11:06

MadLittleMods mentioned this pull request Sep 19, 2025

Bump batch size from 50 to 1000 for _get_e2e_cross_signing_signatures_for_devices query #18939

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Cache `_get_e2e_cross_signing_signatures_for_devices` #18899

Cache `_get_e2e_cross_signing_signatures_for_devices` #18899

anoadragon453 commented Sep 8, 2025 •

edited

Loading

Uh oh!

anoadragon453 commented Sep 8, 2025 •

edited

Loading

Uh oh!

Uh oh!

reivilibre left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

reivilibre Sep 18, 2025

Uh oh!

anoadragon453 Sep 18, 2025

Uh oh!

Uh oh!

anoadragon453 commented Sep 18, 2025

Uh oh!

Uh oh!

		(json.dumps([user_id, item.target_device_id]),)
		for item in signatures

Cache _get_e2e_cross_signing_signatures_for_devices #18899

Cache _get_e2e_cross_signing_signatures_for_devices #18899

Conversation

anoadragon453 commented Sep 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Checklist

Uh oh!

anoadragon453 commented Sep 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

reivilibre left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

reivilibre Sep 18, 2025

Choose a reason for hiding this comment

Uh oh!

anoadragon453 Sep 18, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

anoadragon453 commented Sep 18, 2025

Uh oh!

Uh oh!

Cache `_get_e2e_cross_signing_signatures_for_devices` #18899

Cache `_get_e2e_cross_signing_signatures_for_devices` #18899

anoadragon453 commented Sep 8, 2025 •

edited

Loading

anoadragon453 commented Sep 8, 2025 •

edited

Loading