Handle more notification edge cases (get_notification_item et al.)

[manuroe]

The get_notification_item fn is currently based on a simple /event call. It only manages the happy path where we already have data in the cache.

We are managing the case where we do not have the keys in cache yet but there are other use cases we must address. I am listing below all use cases I see even if they may have a common technical solution.

Room not yet loaded in the app

In this situation, the room state can miss. This creates those problems:

• We cannot display avatars and names for the room and the sender in the notification
• We cannot correctly apply push rules. We disabled push rules check because of that

Room cache not update

Display names and avatars can have been changed elsewhere

• get_notification_item must get the latest profile info to render the notification in an accurate way

Push rules update

A user can update their notification settings from another client. If they disabled one type of push elsewhere, they expect all clients to stop notifying.

• get_notification_item must check push rules on the latest push rules

Future

Coming features that worth considering for the implementation

• Feed app timeline cache from the push reception
• Timeline will be opened on last unread
• Push received for every e2ee message
• If we use /sync to get event content, an event could have been already fetched and cached by a previous call of get_notification_item

Related tasks

• push_actions of TimelineEvent do not work properly in a sliding sync context #2159

[manuroe]

I want the edge cases list to be as exhaustive as possible so that we can think about all of them. Update the original issue if you see more.

[jmartinesp]

matrix-org/synapse#15246 is affecting how we can display notifications for invites, since the get_notification_item method is actually calling this endpoint.

[bnjbvr]

Summarizing the content of our discussion with Manu. Running a /sync for a notification might turn out to be quite complicated, especially for the iOS Element X app, where it's a different process that takes care of decrypting a notification. In that case, we'd get concurrent access on the store backing the /sync response, and we'd need a global cross-process lock there too + proper cache reloading in the main app. That was already quite complicated to handle for the crypto store, but it would be really really tedious to implement for the state store, which has numerous caches all over the place: it's basically equivalent to a full audit of the client, with large rewrites everywhere, and that would take multiple months (4+) to implement properly.

As such, the following edge cases could not be handled:

• making sure that we always have the latest room name and avatar
• feeding the app timeline cache from the push reception
• open the timeline on the last unread message
• (avoiding re-fetch becomes a non bullet item too, since there's no caching in the first place)

Some ideas for technical workarounds:

• "copy on write" the state store: duplicate the sqlite storage, use it in the notification process, then we can run syncs safely (it's separate from the main app's store). The cached information won't be injected back into the main app, but at least we can make sure that we have the latest room name and avatar.
• apparently there's a MSC for improving more context in push notifications, and there we could request to include the room's name, etc. so that would remove the need for the sync altogether.
• request a new endpoint to fetch the room information separately from a sync.

[bnjbvr]

So @poljar suggested in chat that there was another way to get the latest room avatar / name, without resorting to using the same state store (so no shared caching possible, still):

• configure the client to use an in-memory state store (and the real crypto store)
• use a single sliding sync request to fetch the event, room member information, room state info, and push rules (via account data), with a small limit of events
• if that doesn't include the event from the notification, use other endpoints to get the full event and member info (and the rest should still be there)

Not only it would fill more information, but it'd also handle invites properly, and it would be more efficient than the current set of requests we're running.

[manuroe]

thanks @poljar for the idea.
This strategy can also help to achieve "Push rules update" section of the ticket.

From a design point of view, it could help to have the same behavior on iOS and android (or, more generally, multi-process and single process envs). We could have the same logic on both platforms with iOS using an in-memory state store and android, the "normal" state store.

[bnjbvr]

From a design point of view, it could help to have the same behavior on iOS and android (or, more generally, multi-process and single process envs). We could have the same logic on both platforms with iOS using an in-memory state store and android, the "normal" state store.

Interesting idea, but I wonder if that would be subpar for Android, where we could reuse the same state store without worrying about concurrent cross-process writes.

[bnjbvr]

Should be fixed by #2252 which introduced get_notification_with_sliding_sync (soon to be renamed just get_notification, when #2330 is done). Some part would require much much more work, namely Feed app timeline cache from the push reception: that would imply sharing the existing state store in a multi-process way (and basically audit the entire code base to look for cached data that would need invalidation in case another process wrote into the store).

We have some weird edge cases to handle by #2330, like receiving a notification while the app was in airplane mode; the sliding sync may not find the event (as it may have happened hours before), in which case we'll use a /context query and we may return a somewhat degraded notification context.

That being said, most of the work here is done, so closing.