element-hq / element-hq/synapse

`@cachedList` can insert `None` instead of absence of a key

Open
#17,726 5 comments 0 reactions 0 assignees View on GitHub
T-Task Z-Dev-Wishlist
Dominant language
Python
Stars
4.6k
Forks
600
Avg merge
5d 22h
Merged PRs (30d)
51

Description

The docs for this state:

> Note that any missing values are cached as None.

This is not semantically the same as absence of a key, and creates very subtle failure modes. I just spent an hour or two trying to figure out how a function could possibly set None and well, it can't. It turns out the cache is doing it because some rooms don't have entries in the DB. The function in question is https://github.com/element-hq/synapse/blob/51dd4df0a317330d0679e48d7a6dcd5abb054ec7/synapse/storage/databases/main/stream.py#L1495

Unfortunately, the caller of this function never did a None check causing runtime failures: see https://github.com/element-hq/element-x-ios/issues/3300#issuecomment-2358222041

It is absolutely not clear that adding a cache decorator would change the semantics like this. I suggest we double check every caller of these functions to make sure they `None` check before usage.
```
synapse/storage/databases/main/end_to_end_keys.py:855: @cachedList(
synapse/storage/databases/main/end_to_end_keys.py-856- cached_method_name="_get_bare_e2e_cross_signing_keys",
--
synapse/storage/databases/main/receipts.py:441: @cachedList(
synapse/storage/databases/main/receipts.py-442- cached_method_name="_get_linearized_receipts_for_room",
--
synapse/storage/databases/main/roommember.py:199: @cachedList(
synapse/storage/databases/main/roommember.py-200- cached_method_name="get_user_in_room_with_profile", list_name="user_ids"
--
synapse/storage/databases/main/roommember.py:734: @cachedList(
synapse/storage/databases/main/roommember.py-735- cached_method_name="get_rooms_for_user",
--
synapse/storage/databases/main/roommember.py:793: @cachedList(
synapse/storage/databases/main/roommember.py-794- cached_method_name="does_pair_of_users_share_a_room", list_name="other_user_ids"
--
synapse/storage/databases/main/roommember.py:942: @cachedList(
synapse/storage/databases/main/roommember.py-943- cached_method_name="_get_user_id_from_membership_event_id",
--
synapse/storage/databases/main/roommember.py:1284: @cachedList(
synapse/storage/databases/main/roommember.py-1285- cached_method_name="_get_membership_from_event_id", list_name="member_event_ids"
--
synapse/storage/databases/main/transactions.py:211: @cachedList(
synapse/storage/databases/main/transactions.py-212- cached_method_name="get_destination_retry_timings", list_name="destinations"
--
synapse/storage/databases/main/relations.py:477: @cachedList(cached_method_name="get_references_for_event", list_name="event_ids")
synapse/storage/databases/main/relations.py-478- async def get_references_for_events(
--
synapse/storage/databases/main/relations.py:532: @cachedList(cached_method_name="get_applicable_edit", list_name="event_ids") # type: ignore[synapse-@cached-mutable]
synapse/storage/databases/main/relations.py-533- async def get_applicable_edits(
--
synapse/storage/databases/main/relations.py:619: @cachedList(cached_method_name="get_thread_summary", list_name="event_ids") # type: ignore[synapse-@cached-mutable]
synapse/storage/databases/main/relations.py-620- async def get_thread_summaries(
--
synapse/storage/databases/main/relations.py:793: @cachedList(cached_method_name="get_thread_participated", list_name="event_ids")
synapse/storage/databases/main/relations.py-794- async def get_threads_participated(
--
synapse/storage/databases/main/keys.py:137: @cachedList(
synapse/storage/databases/main/keys.py-138- cached_method_name="_get_server_keys_json", list_name="server_name_and_key_ids"
--
synapse/storage/databases/main/keys.py:207: @cachedList(
synapse/storage/databases/main/keys.py-208- cached_method_name="get_server_key_json_for_remote", list_name="key_ids"
--
synapse/storage/databases/main/presence.py:253: @cachedList(
synapse/storage/databases/main/presence.py-254- cached_method_name="_get_presence_for_user",
--
synapse/storage/databases/main/events_worker.py:1624: # @cachedList chomps lots of memory if you call it with a big list, so
synapse/storage/databases/main/events_worker.py-1625- # we break it down. However, each batch requires its own index scan, so we make
--
synapse/storage/databases/main/events_worker.py:1639: @cachedList(cached_method_name="have_seen_event", list_name="event_ids")
synapse/storage/databases/main/events_worker.py-1640- async def _have_seen_events_dict(
--
synapse/storage/databases/main/events_worker.py:2329: @cachedList(cached_method_name="is_partial_state_event", list_name="event_ids")
synapse/storage/databases/main/events_worker.py-2330- async def get_partial_state_events(
--
synapse/storage/databases/main/events_worker.py:2352: # convert the result to a dict, to make @cachedList work
synapse/storage/databases/main/events_worker.py-2353- partial = {r[0] for r in result}
--
synapse/storage/databases/main/stream.py:1495: @cachedList(cached_method_name="_get_max_event_pos", list_name="room_ids")
synapse/storage/databases/main/stream.py-1496- async def _bulk_get_max_event_pos(
--
synapse/storage/databases/main/signatures.py:41: @cachedList(
synapse/storage/databases/main/signatures.py-42- cached_method_name="get_event_reference_hash", list_name="event_ids", num_args=1
--
synapse/storage/databases/main/push_rule.py:262: @cachedList(cached_method_name="get_push_rules_for_user", list_name="user_ids")
synapse/storage/databases/main/push_rule.py-263- async def bulk_get_push_rules(
--
synapse/storage/databases/main/devices.py:1076: @cachedList(
synapse/storage/databases/main/devices.py-1077- cached_method_name="get_device_list_last_stream_id_for_remote",
--
synapse/storage/databases/main/room.py:1361: @cachedList(cached_method_name="is_partial_state_room", list_name="room_ids")
synapse/storage/databases/main/room.py-1362- async def is_partial_state_room_batched(
--
synapse/storage/databases/main/state.py:314: @cachedList(cached_method_name="get_room_type", list_name="room_ids")
synapse/storage/databases/main/state.py-315- async def bulk_get_room_type(
--
synapse/storage/databases/main/state.py:393: @cachedList(cached_method_name="get_room_encryption", list_name="room_ids")
synapse/storage/databases/main/state.py-394- async def bulk_get_room_encryption(
--
synapse/storage/databases/main/state.py:605: @cachedList(
synapse/storage/databases/main/state.py-606- cached_method_name="_get_state_group_for_event",
--
synapse/storage/databases/main/user_erasure_store.py:48: @cachedList(cached_method_name="is_user_erased", list_name="user_ids")
synapse/storage/databases/main/user_erasure_store.py-49- async def are_users_erased(self, user_ids: Iterable[str]) -> Mapping[str, bool]:
```

Contributor guide

Open the contributing guide

Research direction

Start with the @cachedList implementation and synapse/storage/databases/main/stream.py:_bulk_get_max_event_pos, then inspect the callers listed in the issue for their handling of missing values. Check the relevant storage tests and add coverage for absent database rows; done means cachedList semantics are explicit and every affected caller safely handles missing entries.

Written by the indexing model from the issue text.

Assessment

Tech stack
python
Domain
backend, databases
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.