Prevent a sync request from removing a user's busy presence status

[dbkr]

In trying to use the MSC3026 busy presence status, the user's status
would be set back to 'online' next time they synced. The change makes
it so that sycning does not affect a user's presence status if it
is currently set to 'busy': it must be removed through the presence
API.

The MSC defers to implementations on the behaviour of busy presence,
so I think this remains compatible with the MSC.

I've replaced the explicit presence-set call from the sync code and moved
this logic into the presence handler, which feels like a better place for it. I've
added some tests to assert the behaviour remains what it's supposed to be.

I haven't added this code to the worker version of user_syncing as there
was no code there to set presence status before, but having removed the
set_state from sync, I'm not certain whether it needs to be there or not,
so I'd appreciate a check from someone who knows the worker setup better.

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.
• Pull request includes a sign off
• Code style is correct
  (run the linters)

[deepbluev7]

What happens if a user explicitly sets the presence to online in the sync request in one client and the other client had set the presence to busy, but didn't sync for a while? Shouldn't clients instead set their presence explicitly in /sync every time and then the max calculated from that? Because one of my clients will always be busy, when I have 2 open at the same time.

[dbkr]

Sytest added in matrix-org/sytest#1238

[squahtx]

Thanks for updating the PR!

The worker mode changes look like a step in the right direction. There's a slight discrepancy in whether last_active_ts gets updated between worker and non-worker mode. I'm not sure whether it's important to us. I guess it depends on how clients want to use it?

Before this PR

prev_state	set_presence	new presence set	last_active_ts updated	last_user_sync_ts updated
{offline,unavailable,online,busy}	offline	❌	❌	❌
{offline,unavailable,online,busy}	unavailable	✔️	❌	✔️
{offline,unavailable,online,busy}	{online,busy}	✔️	✔️	✔️

After this PR, single process

prev_state	set_presence	new presence set	last_active_ts updated	last_user_sync_ts updated
{offline,unavailable,online}	offline	❌	❌	❌
{offline,unavailable,online}	{unavailable,online,busy}	✔️	✔️	✔️
busy	offline	❌	❌	❌
busy	{unavailable,online,busy}	❌	✔️	✔️

After this PR, with workers

prev_state	set_presence	new presence set	last_active_ts updated	last_user_sync_ts updated
{offline,unavailable,online}	offline	❌	❌	❌
{offline,unavailable,online}	unavailable	✔️	❌ (!)	✔️
{offline,unavailable,online}	{online,busy}	✔️	✔️	✔️
busy	offline	❌	❌	❌
busy	unavailable	❌	❌ (!)	✔️
busy	{online,busy}	❌	❌ (!)	✔️

[dbkr]

Thanks for going through working out what all the various behaviours were. It's a good point that the last_active_ago would have been affected which was probably not the correct thing. I've updated this for the non-worker case. I was hoping to move this into a common method between the worker & non-worker cases to use the same logic, but the worker doesn't have _update_states so it's not as simple as that. I'll look at this again tomorrow - hopefully it can be done without adding a new replication endpoint.

[dbkr]

Having looked at this some more, I've realised that the previous behaviour always updated last_active_ts because set_state() updates last_active_ts, so it looks like all the logic in user_syncing to update or not update last_active_ts isn't really doing anything, as far as I can see. Testing this by writing a sytest seems to confirm this: performing a normal sync request (previously online, set_presence=online) caused last_active_ago to be updated. I'm guessing this is a bug, at this point I'm inclined to leave that bug for a different PR. I can leave it as it is and leave the behaviour 'correct' (I think) in single-process mode and keeping the old behaviour in worker mode, or use set_state everywhere and keep the existing behaviour in both modes - any preference?

[squahtx]

Hm, tricky. I'd prefer that we keep the single process and worker mode behaviour the same, so that it's easier to reason about, even if it's subtly wrong.

ie. make PresenceHandler.user_syncing and PresenceHandler.update_external_syncs_row do the same thing, and call set_state (somewhere) in both configurations.

[dbkr]

Yeah, agreed there's strong argument for keeping it consistent and fixing the bug everywhere separately. Done, and filed #12424 to track the incorrect behaviour.

[squahtx]

Did a final round of review of the overall diff. Just some picky tidy-ups left, otherwise I'm happy for us to merge

[squahtx]

Looks good to merge. Thanks for your patience!