From dce7bf81a006502de950654fbbd0b57d2a589605 Mon Sep 17 00:00:00 2001 From: David Baker Date: Fri, 11 Mar 2022 18:37:36 +0000 Subject: [PATCH 01/19] Prevent a sync request from removing a user's busy presence status 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. --- changelog.d/12210.bugfix | 1 + synapse/handlers/events.py | 6 ++- synapse/handlers/presence.py | 29 +++++----- synapse/replication/http/presence.py | 1 - synapse/rest/client/sync.py | 9 ++-- tests/handlers/test_presence.py | 79 ++++++++++++++++++++++++++++ 6 files changed, 101 insertions(+), 24 deletions(-) create mode 100644 changelog.d/12210.bugfix diff --git a/changelog.d/12210.bugfix b/changelog.d/12210.bugfix new file mode 100644 index 000000000000..9278e3a9c163 --- /dev/null +++ b/changelog.d/12210.bugfix @@ -0,0 +1 @@ +Prevent a sync request from removing a user's busy presence status. diff --git a/synapse/handlers/events.py b/synapse/handlers/events.py index d2ccb5c5d311..e89c4df31466 100644 --- a/synapse/handlers/events.py +++ b/synapse/handlers/events.py @@ -16,7 +16,7 @@ import random from typing import TYPE_CHECKING, Iterable, List, Optional -from synapse.api.constants import EduTypes, EventTypes, Membership +from synapse.api.constants import EduTypes, EventTypes, Membership, PresenceState from synapse.api.errors import AuthError, SynapseError from synapse.events import EventBase from synapse.events.utils import SerializeEventConfig @@ -67,7 +67,9 @@ async def get_stream( presence_handler = self.hs.get_presence_handler() context = await presence_handler.user_syncing( - auth_user_id, affect_presence=affect_presence + auth_user_id, + affect_presence=affect_presence, + presence_state=PresenceState.ONLINE, ) with context: if timeout: diff --git a/synapse/handlers/presence.py b/synapse/handlers/presence.py index 9927a30e6ed5..ef2917a4483c 100644 --- a/synapse/handlers/presence.py +++ b/synapse/handlers/presence.py @@ -151,7 +151,7 @@ def __init__(self, hs: "HomeServer"): @abc.abstractmethod async def user_syncing( - self, user_id: str, affect_presence: bool + self, user_id: str, affect_presence: bool, presence_state: str ) -> ContextManager[None]: """Returns a context manager that should surround any stream requests from the user. @@ -233,7 +233,6 @@ async def set_state( self, target_user: UserID, state: JsonDict, - ignore_status_msg: bool = False, force_notify: bool = False, ) -> None: """Set the presence state of the user. @@ -463,7 +462,7 @@ def send_stop_syncing(self) -> None: self.send_user_sync(user_id, False, last_sync_ms) async def user_syncing( - self, user_id: str, affect_presence: bool + self, user_id: str, affect_presence: bool, presence_state: str ) -> ContextManager[None]: """Record that a user is syncing. @@ -567,7 +566,6 @@ async def set_state( self, target_user: UserID, state: JsonDict, - ignore_status_msg: bool = False, force_notify: bool = False, ) -> None: """Set the presence state of the user. @@ -604,7 +602,6 @@ async def set_state( instance_name=self._presence_writer_instance, user_id=user_id, state=state, - ignore_status_msg=ignore_status_msg, force_notify=force_notify, ) @@ -944,7 +941,10 @@ async def bump_presence_active_time(self, user: UserID) -> None: await self._update_states([prev_state.copy_and_replace(**new_fields)]) async def user_syncing( - self, user_id: str, affect_presence: bool = True + self, + user_id: str, + affect_presence: bool = True, + presence_state: str = PresenceState.ONLINE, ) -> ContextManager[None]: """Returns a context manager that should surround any stream requests from the user. @@ -969,13 +969,14 @@ async def user_syncing( self.user_to_num_current_syncs[user_id] = curr_sync + 1 prev_state = await self.current_state_for_user(user_id) - if prev_state.state == PresenceState.OFFLINE: - # If they're currently offline then bring them online, otherwise - # just update the last sync times. + if prev_state.state != PresenceState.BUSY: + # If they're busy then they don't stop being busy just by syncing, + # so just update the last sync & last active times. Otherwise, set the + # new presence value await self._update_states( [ prev_state.copy_and_replace( - state=PresenceState.ONLINE, + state=presence_state, last_active_ts=self.clock.time_msec(), last_user_sync_ts=self.clock.time_msec(), ) @@ -985,7 +986,8 @@ async def user_syncing( await self._update_states( [ prev_state.copy_and_replace( - last_user_sync_ts=self.clock.time_msec() + last_active_ts=self.clock.time_msec(), + last_user_sync_ts=self.clock.time_msec(), ) ] ) @@ -1168,7 +1170,6 @@ async def set_state( self, target_user: UserID, state: JsonDict, - ignore_status_msg: bool = False, force_notify: bool = False, ) -> None: """Set the presence state of the user. @@ -1200,9 +1201,7 @@ async def set_state( prev_state = await self.current_state_for_user(user_id) new_fields = {"state": presence} - - if not ignore_status_msg: - new_fields["status_msg"] = status_msg + new_fields["status_msg"] = status_msg if presence == PresenceState.ONLINE or ( presence == PresenceState.BUSY and self._busy_presence_enabled diff --git a/synapse/replication/http/presence.py b/synapse/replication/http/presence.py index 4a5b08f56f73..994cf4e71ee3 100644 --- a/synapse/replication/http/presence.py +++ b/synapse/replication/http/presence.py @@ -114,7 +114,6 @@ async def _handle_request( # type: ignore[override] await self._presence_handler.set_state( UserID.from_string(user_id), content["state"], - content["ignore_status_msg"], content["force_notify"], ) diff --git a/synapse/rest/client/sync.py b/synapse/rest/client/sync.py index 53c385a86cc1..efbb167af5b4 100644 --- a/synapse/rest/client/sync.py +++ b/synapse/rest/client/sync.py @@ -179,13 +179,10 @@ async def on_GET(self, request: SynapseRequest) -> Tuple[int, JsonDict]: affect_presence = set_presence != PresenceState.OFFLINE - if affect_presence: - await self.presence_handler.set_state( - user, {"presence": set_presence}, True - ) - context = await self.presence_handler.user_syncing( - user.to_string(), affect_presence=affect_presence + user.to_string(), + affect_presence=affect_presence, + presence_state=set_presence, ) with context: sync_result = await self.sync_handler.wait_for_sync_for_user( diff --git a/tests/handlers/test_presence.py b/tests/handlers/test_presence.py index 6ddec9ecf1fe..4db76b0d976c 100644 --- a/tests/handlers/test_presence.py +++ b/tests/handlers/test_presence.py @@ -652,6 +652,85 @@ def test_set_presence_with_status_msg_none(self): # Mark user as online and `status_msg = None` self._set_presencestate_with_status_msg(user_id, PresenceState.ONLINE, None) + def test_set_presence_from_syncing_not_set(self): + """Test that presence is not set by syncing if affect_presence is false""" + user_id = "@test:server" + status_msg = "I'm here!" + + self._set_presencestate_with_status_msg( + user_id, PresenceState.UNAVAILABLE, status_msg + ) + + self.get_success( + self.presence_handler.user_syncing(user_id, False, PresenceState.ONLINE) + ) + + state = self.get_success( + self.presence_handler.get_state(UserID.from_string(user_id)) + ) + # we should still be unavailable + self.assertEqual(state.state, PresenceState.UNAVAILABLE) + # and status message should still be the same + self.assertEqual(state.status_msg, status_msg) + + def test_set_presence_from_syncing_is_set(self): + """Test that presence is set by syncing if affect_presence is true""" + user_id = "@test:server" + status_msg = "I'm here!" + + self._set_presencestate_with_status_msg( + user_id, PresenceState.UNAVAILABLE, status_msg + ) + + self.get_success( + self.presence_handler.user_syncing(user_id, True, PresenceState.ONLINE) + ) + + state = self.get_success( + self.presence_handler.get_state(UserID.from_string(user_id)) + ) + # we should now be online + self.assertEqual(state.state, PresenceState.ONLINE) + + def test_set_presence_from_syncing_keeps_status(self): + """Test that presence set by syncing retains status message""" + user_id = "@test:server" + status_msg = "I'm here!" + + self._set_presencestate_with_status_msg( + user_id, PresenceState.UNAVAILABLE, status_msg + ) + + self.get_success( + self.presence_handler.user_syncing(user_id, True, PresenceState.ONLINE) + ) + + state = self.get_success( + self.presence_handler.get_state(UserID.from_string(user_id)) + ) + # our status message should be the same as it was before + self.assertEqual(state.status_msg, status_msg) + + def test_set_presence_from_syncing_keeps_busy(self): + """Test that presence set by syncing doesn't affect busy status""" + # while this isn't the default + self.presence_handler._busy_presence_enabled = True + + user_id = "@test:server" + status_msg = "I'm busy!" + + self._set_presencestate_with_status_msg(user_id, PresenceState.BUSY, status_msg) + + self.get_success( + self.presence_handler.user_syncing(user_id, True, PresenceState.ONLINE) + ) + + state = self.get_success( + self.presence_handler.get_state(UserID.from_string(user_id)) + ) + # we should still be busy + self.assertEqual(state.state, PresenceState.BUSY) + def _set_presencestate_with_status_msg( self, user_id: str, state: PresenceState, status_msg: Optional[str] ): From e0292847ba49636aeeb8c381d836f4762f673e20 Mon Sep 17 00:00:00 2001 From: David Baker Date: Fri, 11 Mar 2022 18:48:50 +0000 Subject: [PATCH 02/19] I guessed wrong --- changelog.d/{12210.bugfix => 12213.bugfix} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename changelog.d/{12210.bugfix => 12213.bugfix} (100%) diff --git a/changelog.d/12210.bugfix b/changelog.d/12213.bugfix similarity index 100% rename from changelog.d/12210.bugfix rename to changelog.d/12213.bugfix From 68dfb7eac7299ea04b9bdd69933ebfbb3bc444f1 Mon Sep 17 00:00:00 2001 From: David Baker Date: Tue, 5 Apr 2022 11:58:59 +0100 Subject: [PATCH 03/19] Add new arg to docstring --- synapse/handlers/presence.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/synapse/handlers/presence.py b/synapse/handlers/presence.py index ef2917a4483c..cb1acd4e322c 100644 --- a/synapse/handlers/presence.py +++ b/synapse/handlers/presence.py @@ -165,6 +165,7 @@ async def user_syncing( affect_presence: If false this function will be a no-op. Useful for streams that are not associated with an actual client that is being used by a user. + presence_state: The presence state indicated in the sync request """ @abc.abstractmethod @@ -958,6 +959,7 @@ async def user_syncing( affect_presence: If false this function will be a no-op. Useful for streams that are not associated with an actual client that is being used by a user. + presence_state: The presence state indicated in the sync request """ # Override if it should affect the user's presence, if presence is # disabled. From 0fb0e4df35f06af3410f4aec30a578587a389115 Mon Sep 17 00:00:00 2001 From: David Baker Date: Tue, 5 Apr 2022 12:04:51 +0100 Subject: [PATCH 04/19] Remove ignore_status_msg from _serialize_payload too --- synapse/replication/http/presence.py | 1 - 1 file changed, 1 deletion(-) diff --git a/synapse/replication/http/presence.py b/synapse/replication/http/presence.py index 994cf4e71ee3..b61e48a986e1 100644 --- a/synapse/replication/http/presence.py +++ b/synapse/replication/http/presence.py @@ -102,7 +102,6 @@ async def _serialize_payload( # type: ignore[override] ) -> JsonDict: return { "state": state, - "ignore_status_msg": ignore_status_msg, "force_notify": force_notify, } From 9beae074371cccc5a22a00a8ec5bbd5c7a04f08f Mon Sep 17 00:00:00 2001 From: David Baker Date: Wed, 6 Apr 2022 12:43:16 +0100 Subject: [PATCH 05/19] Set presence in worker user_syncing --- synapse/handlers/presence.py | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/synapse/handlers/presence.py b/synapse/handlers/presence.py index cb1acd4e322c..5c834ce179eb 100644 --- a/synapse/handlers/presence.py +++ b/synapse/handlers/presence.py @@ -229,6 +229,11 @@ async def current_state_for_users( return states + async def current_state_for_user(self, user_id: str) -> UserPresenceState: + """Get the current presence state for a user.""" + res = await self.current_state_for_users([user_id]) + return res[user_id] + @abc.abstractmethod async def set_state( self, @@ -473,6 +478,16 @@ async def user_syncing( if not affect_presence or not self._presence_enabled: return _NullContextManager() + if affect_presence: + prev_state = await self.current_state_for_user(user_id) + if prev_state != PresenceState.BUSY: + # XXX: Why does this pass force_notify = True? This is copied + # from when the sync rest handler set the presence itself, but + # I don't really understand why this would be necessary. + self.set_state( + UserID.from_string(user_id), {"presence": presence_state}, True + ) + curr_sync = self._user_to_num_current_syncs.get(user_id, 0) self._user_to_num_current_syncs[user_id] = curr_sync + 1 @@ -1092,11 +1107,6 @@ async def update_external_syncs_clear(self, process_id: str) -> None: ) self.external_process_last_updated_ms.pop(process_id, None) - async def current_state_for_user(self, user_id: str) -> UserPresenceState: - """Get the current presence state for a user.""" - res = await self.current_state_for_users([user_id]) - return res[user_id] - async def _persist_and_notify(self, states: List[UserPresenceState]) -> None: """Persist states in the database, poke the notifier and send to interested remote servers From 9098c9da77465ad532229e34279aa4772d55bc96 Mon Sep 17 00:00:00 2001 From: David Baker Date: Wed, 6 Apr 2022 15:48:41 +0100 Subject: [PATCH 06/19] Await on set_state --- synapse/handlers/presence.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/handlers/presence.py b/synapse/handlers/presence.py index ef55614ef15b..e71ecf43a376 100644 --- a/synapse/handlers/presence.py +++ b/synapse/handlers/presence.py @@ -482,7 +482,7 @@ async def user_syncing( # XXX: Why does this pass force_notify = True? This is copied # from when the sync rest handler set the presence itself, but # I don't really understand why this would be necessary. - self.set_state( + await self.set_state( UserID.from_string(user_id), {"presence": presence_state}, True ) From 53d2fa466502a61a532c9a99d199308c49ebabb8 Mon Sep 17 00:00:00 2001 From: David Baker Date: Wed, 6 Apr 2022 17:23:15 +0100 Subject: [PATCH 07/19] Remove redundant check --- synapse/handlers/presence.py | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/synapse/handlers/presence.py b/synapse/handlers/presence.py index e71ecf43a376..6a96cac5ceb1 100644 --- a/synapse/handlers/presence.py +++ b/synapse/handlers/presence.py @@ -476,15 +476,14 @@ async def user_syncing( if not affect_presence or not self._presence_enabled: return _NullContextManager() - if affect_presence: - prev_state = await self.current_state_for_user(user_id) - if prev_state != PresenceState.BUSY: - # XXX: Why does this pass force_notify = True? This is copied - # from when the sync rest handler set the presence itself, but - # I don't really understand why this would be necessary. - await self.set_state( - UserID.from_string(user_id), {"presence": presence_state}, True - ) + prev_state = await self.current_state_for_user(user_id) + if prev_state != PresenceState.BUSY: + # XXX: Why does this pass force_notify = True? This is copied + # from when the sync rest handler set the presence itself, but + # I don't really understand why this would be necessary. + await self.set_state( + UserID.from_string(user_id), {"presence": presence_state}, True + ) curr_sync = self._user_to_num_current_syncs.get(user_id, 0) self._user_to_num_current_syncs[user_id] = curr_sync + 1 From b2a44ac41edf82538e93c37efddc16041cb3ab49 Mon Sep 17 00:00:00 2001 From: David Baker Date: Wed, 6 Apr 2022 17:42:44 +0100 Subject: [PATCH 08/19] Put ignore_status_msg back and use it in the worker user_syncing method --- synapse/handlers/presence.py | 13 +++++++++---- synapse/replication/http/presence.py | 2 ++ 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/synapse/handlers/presence.py b/synapse/handlers/presence.py index 6a96cac5ceb1..b32dc3bcb3e9 100644 --- a/synapse/handlers/presence.py +++ b/synapse/handlers/presence.py @@ -239,6 +239,7 @@ async def set_state( self, target_user: UserID, state: JsonDict, + ignore_status_msg: bool = False, force_notify: bool = False, ) -> None: """Set the presence state of the user. @@ -478,9 +479,8 @@ async def user_syncing( prev_state = await self.current_state_for_user(user_id) if prev_state != PresenceState.BUSY: - # XXX: Why does this pass force_notify = True? This is copied - # from when the sync rest handler set the presence itself, but - # I don't really understand why this would be necessary. + # We set state here but pass ignore_status_msg = True as we don't want to + # cause the status message to be cleared. await self.set_state( UserID.from_string(user_id), {"presence": presence_state}, True ) @@ -579,6 +579,7 @@ async def set_state( self, target_user: UserID, state: JsonDict, + ignore_status_msg: bool = False, force_notify: bool = False, ) -> None: """Set the presence state of the user. @@ -615,6 +616,7 @@ async def set_state( instance_name=self._presence_writer_instance, user_id=user_id, state=state, + ignore_status_msg=ignore_status_msg, force_notify=force_notify, ) @@ -1179,6 +1181,7 @@ async def set_state( self, target_user: UserID, state: JsonDict, + ignore_status_msg: bool = False, force_notify: bool = False, ) -> None: """Set the presence state of the user. @@ -1210,7 +1213,9 @@ async def set_state( prev_state = await self.current_state_for_user(user_id) new_fields = {"state": presence} - new_fields["status_msg"] = status_msg + + if not ignore_status_msg: + new_fields["status_msg"] = status_msg if presence == PresenceState.ONLINE or ( presence == PresenceState.BUSY and self._busy_presence_enabled diff --git a/synapse/replication/http/presence.py b/synapse/replication/http/presence.py index b61e48a986e1..4a5b08f56f73 100644 --- a/synapse/replication/http/presence.py +++ b/synapse/replication/http/presence.py @@ -102,6 +102,7 @@ async def _serialize_payload( # type: ignore[override] ) -> JsonDict: return { "state": state, + "ignore_status_msg": ignore_status_msg, "force_notify": force_notify, } @@ -113,6 +114,7 @@ async def _handle_request( # type: ignore[override] await self._presence_handler.set_state( UserID.from_string(user_id), content["state"], + content["ignore_status_msg"], content["force_notify"], ) From 920fddf6cff36c9800f227377bc4dd8f6999edc7 Mon Sep 17 00:00:00 2001 From: David Baker Date: Wed, 6 Apr 2022 18:41:34 +0100 Subject: [PATCH 09/19] Make non-worker behaviour match old behaviour re-adding previous case where it didn't set last_active_ts unless the previous state was offline. I have made it set the presence state in other cases as that looked like a bug. --- synapse/handlers/presence.py | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-) diff --git a/synapse/handlers/presence.py b/synapse/handlers/presence.py index b32dc3bcb3e9..11fef663b10a 100644 --- a/synapse/handlers/presence.py +++ b/synapse/handlers/presence.py @@ -985,10 +985,19 @@ async def user_syncing( self.user_to_num_current_syncs[user_id] = curr_sync + 1 prev_state = await self.current_state_for_user(user_id) - if prev_state.state != PresenceState.BUSY: - # If they're busy then they don't stop being busy just by syncing, - # so just update the last sync & last active times. Otherwise, set the - # new presence value + # If they're busy then they don't stop being busy just by syncing, + # so just update the last sync time. + if prev_state.state == PresenceState.BUSY: + await self._update_states( + [ + prev_state.copy_and_replace( + last_user_sync_ts=self.clock.time_msec(), + ) + ] + ) + # If they were offline, then bring them online (update the presence + # state to the new state and update the last_active_ts) + elif prev_state.state == PresenceState.OFFLINE: await self._update_states( [ prev_state.copy_and_replace( @@ -998,11 +1007,14 @@ async def user_syncing( ) ] ) + # otherwise, set the new presence state & update the last sync time, + # but don't update last_active_ts as this isn't an indication that + # they've been active. else: await self._update_states( [ prev_state.copy_and_replace( - last_active_ts=self.clock.time_msec(), + state=presence_state, last_user_sync_ts=self.clock.time_msec(), ) ] From f5119b40210281bbe769042d06d493ca1cb5545e Mon Sep 17 00:00:00 2001 From: David Baker Date: Fri, 8 Apr 2022 18:08:20 +0100 Subject: [PATCH 10/19] Use ste_state in both places so behaviour is consistent --- synapse/handlers/presence.py | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-) diff --git a/synapse/handlers/presence.py b/synapse/handlers/presence.py index 11fef663b10a..0b17aa26cdfe 100644 --- a/synapse/handlers/presence.py +++ b/synapse/handlers/presence.py @@ -481,6 +481,9 @@ async def user_syncing( if prev_state != PresenceState.BUSY: # We set state here but pass ignore_status_msg = True as we don't want to # cause the status message to be cleared. + # Note that this causes last_active_ts to be incremented which is not + # what the spec wants: see comment in the BasePresenceHandler version + # of this function. await self.set_state( UserID.from_string(user_id), {"presence": presence_state}, True ) @@ -1001,7 +1004,9 @@ async def user_syncing( await self._update_states( [ prev_state.copy_and_replace( - state=presence_state, + # We do want to update state here but currently do so with separate + # set_state below. + # state=presence_state, last_active_ts=self.clock.time_msec(), last_user_sync_ts=self.clock.time_msec(), ) @@ -1014,12 +1019,24 @@ async def user_syncing( await self._update_states( [ prev_state.copy_and_replace( - state=presence_state, + # We do want to update state here but currently do so with separate + # set_state below. + # state=presence_state, last_user_sync_ts=self.clock.time_msec(), ) ] ) + if prev_state != PresenceState.BUSY: + # XXX: We set_state separately here and just update the last_active_ts above + # This keeps the logic as similar as possible between the worker and single + # process modes. Using set_state will actually cause last_active_ts to be + # updated always, which is not what the spec calls for, but synapse has done + # this for... forever, I think. + await self.set_state( + UserID.from_string(user_id), {"presence": presence_state}, True + ) + async def _end() -> None: try: self.user_to_num_current_syncs[user_id] -= 1 From 2d2f774a76d3285a626c6b34ef5aebcad5ea911f Mon Sep 17 00:00:00 2001 From: David Baker Date: Fri, 8 Apr 2022 18:31:21 +0100 Subject: [PATCH 11/19] Forgot to get the state member --- synapse/handlers/presence.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/handlers/presence.py b/synapse/handlers/presence.py index 0b17aa26cdfe..9ecc045f9a9f 100644 --- a/synapse/handlers/presence.py +++ b/synapse/handlers/presence.py @@ -1027,7 +1027,7 @@ async def user_syncing( ] ) - if prev_state != PresenceState.BUSY: + if prev_state.state != PresenceState.BUSY: # XXX: We set_state separately here and just update the last_active_ts above # This keeps the logic as similar as possible between the worker and single # process modes. Using set_state will actually cause last_active_ts to be From e7093fab1f7691a20199f3dea3af1ed1988803b3 Mon Sep 17 00:00:00 2001 From: David Baker Date: Mon, 11 Apr 2022 14:33:55 +0100 Subject: [PATCH 12/19] Rearrange to hopefully be easier to understand --- synapse/handlers/presence.py | 39 ++++++++++++------------------------ 1 file changed, 13 insertions(+), 26 deletions(-) diff --git a/synapse/handlers/presence.py b/synapse/handlers/presence.py index 9ecc045f9a9f..b6e56af529ba 100644 --- a/synapse/handlers/presence.py +++ b/synapse/handlers/presence.py @@ -988,25 +988,25 @@ async def user_syncing( self.user_to_num_current_syncs[user_id] = curr_sync + 1 prev_state = await self.current_state_for_user(user_id) + # If they're busy then they don't stop being busy just by syncing, # so just update the last sync time. - if prev_state.state == PresenceState.BUSY: - await self._update_states( - [ - prev_state.copy_and_replace( - last_user_sync_ts=self.clock.time_msec(), - ) - ] + if prev_state.state != PresenceState.BUSY: + # XXX: We set_state separately here and just update the last_active_ts above + # This keeps the logic as similar as possible between the worker and single + # process modes. Using set_state will actually cause last_active_ts to be + # updated always, which is not what the spec calls for, but synapse has done + # this for... forever, I think. + await self.set_state( + UserID.from_string(user_id), {"presence": presence_state}, True ) - # If they were offline, then bring them online (update the presence - # state to the new state and update the last_active_ts) - elif prev_state.state == PresenceState.OFFLINE: + prev_state = prev_state.copy_and_replace(state=presence_state) + + if prev_state.state == PresenceState.OFFLINE: await self._update_states( [ prev_state.copy_and_replace( - # We do want to update state here but currently do so with separate - # set_state below. - # state=presence_state, + state=presence_state, last_active_ts=self.clock.time_msec(), last_user_sync_ts=self.clock.time_msec(), ) @@ -1019,24 +1019,11 @@ async def user_syncing( await self._update_states( [ prev_state.copy_and_replace( - # We do want to update state here but currently do so with separate - # set_state below. - # state=presence_state, last_user_sync_ts=self.clock.time_msec(), ) ] ) - if prev_state.state != PresenceState.BUSY: - # XXX: We set_state separately here and just update the last_active_ts above - # This keeps the logic as similar as possible between the worker and single - # process modes. Using set_state will actually cause last_active_ts to be - # updated always, which is not what the spec calls for, but synapse has done - # this for... forever, I think. - await self.set_state( - UserID.from_string(user_id), {"presence": presence_state}, True - ) - async def _end() -> None: try: self.user_to_num_current_syncs[user_id] -= 1 From 0a7b3f878a3ff5c5ee880b6b29b1647014f2e167 Mon Sep 17 00:00:00 2001 From: David Baker Date: Mon, 11 Apr 2022 15:27:58 +0100 Subject: [PATCH 13/19] Take a copy of the previous state before we start messing with it --- synapse/handlers/presence.py | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/synapse/handlers/presence.py b/synapse/handlers/presence.py index b6e56af529ba..e767443afb56 100644 --- a/synapse/handlers/presence.py +++ b/synapse/handlers/presence.py @@ -988,6 +988,8 @@ async def user_syncing( self.user_to_num_current_syncs[user_id] = curr_sync + 1 prev_state = await self.current_state_for_user(user_id) + # take a copy so we can start modifying it + new_state = prev_state.copy_and_replace() # If they're busy then they don't stop being busy just by syncing, # so just update the last sync time. @@ -1000,12 +1002,12 @@ async def user_syncing( await self.set_state( UserID.from_string(user_id), {"presence": presence_state}, True ) - prev_state = prev_state.copy_and_replace(state=presence_state) + new_state = new_state.copy_and_replace(state=presence_state) if prev_state.state == PresenceState.OFFLINE: await self._update_states( [ - prev_state.copy_and_replace( + new_state.copy_and_replace( state=presence_state, last_active_ts=self.clock.time_msec(), last_user_sync_ts=self.clock.time_msec(), @@ -1018,7 +1020,7 @@ async def user_syncing( else: await self._update_states( [ - prev_state.copy_and_replace( + new_state.copy_and_replace( last_user_sync_ts=self.clock.time_msec(), ) ] From bd932fc9972a15a2d1455138cc63570a4d89f0c8 Mon Sep 17 00:00:00 2001 From: David Baker Date: Wed, 13 Apr 2022 13:21:53 +0100 Subject: [PATCH 14/19] Update prev_state using the proper function --- synapse/handlers/presence.py | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/synapse/handlers/presence.py b/synapse/handlers/presence.py index e767443afb56..634a179c4515 100644 --- a/synapse/handlers/presence.py +++ b/synapse/handlers/presence.py @@ -988,8 +988,6 @@ async def user_syncing( self.user_to_num_current_syncs[user_id] = curr_sync + 1 prev_state = await self.current_state_for_user(user_id) - # take a copy so we can start modifying it - new_state = prev_state.copy_and_replace() # If they're busy then they don't stop being busy just by syncing, # so just update the last sync time. @@ -1002,12 +1000,12 @@ async def user_syncing( await self.set_state( UserID.from_string(user_id), {"presence": presence_state}, True ) - new_state = new_state.copy_and_replace(state=presence_state) + prev_state = await self.current_state_for_user(user_id) if prev_state.state == PresenceState.OFFLINE: await self._update_states( [ - new_state.copy_and_replace( + prev_state.copy_and_replace( state=presence_state, last_active_ts=self.clock.time_msec(), last_user_sync_ts=self.clock.time_msec(), @@ -1020,7 +1018,7 @@ async def user_syncing( else: await self._update_states( [ - new_state.copy_and_replace( + prev_state.copy_and_replace( last_user_sync_ts=self.clock.time_msec(), ) ] From 6d6b88ef34ab16eb28f592834d9288045d97dc14 Mon Sep 17 00:00:00 2001 From: David Baker Date: Wed, 13 Apr 2022 14:37:38 +0100 Subject: [PATCH 15/19] Add comment --- synapse/handlers/presence.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/synapse/handlers/presence.py b/synapse/handlers/presence.py index 634a179c4515..dac7b5665dcb 100644 --- a/synapse/handlers/presence.py +++ b/synapse/handlers/presence.py @@ -1000,6 +1000,9 @@ async def user_syncing( await self.set_state( UserID.from_string(user_id), {"presence": presence_state}, True ) + # To keep the single process behaviour consistent with worker mode, run the + # same logic as `update_external_syncs_row`, even though it looks weird + # (this should come from the in-memory cache) prev_state = await self.current_state_for_user(user_id) if prev_state.state == PresenceState.OFFLINE: From e50a0a13b7613b2fac88a95145eafa154041e93c Mon Sep 17 00:00:00 2001 From: David Baker Date: Wed, 13 Apr 2022 14:39:00 +0100 Subject: [PATCH 16/19] Revert presence_state setting We can only get here if presence_state is OFFLINE, which is never, given the current implementations of SyncRestServlet.on_GET and EventStreamHandler.get_stream Co-authored-by: Sean Quah <8349537+squahtx@users.noreply.github.com> --- synapse/handlers/presence.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/handlers/presence.py b/synapse/handlers/presence.py index dac7b5665dcb..a90824b31599 100644 --- a/synapse/handlers/presence.py +++ b/synapse/handlers/presence.py @@ -1009,7 +1009,7 @@ async def user_syncing( await self._update_states( [ prev_state.copy_and_replace( - state=presence_state, + state=PresenceState.ONLINE, last_active_ts=self.clock.time_msec(), last_user_sync_ts=self.clock.time_msec(), ) From edcdfed378f14103b003e1e6907ae7f8f777a2d3 Mon Sep 17 00:00:00 2001 From: David Baker Date: Wed, 13 Apr 2022 14:40:15 +0100 Subject: [PATCH 17/19] Revert unnecessary comma change Co-authored-by: Sean Quah <8349537+squahtx@users.noreply.github.com> --- synapse/handlers/presence.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/handlers/presence.py b/synapse/handlers/presence.py index a90824b31599..0df2510a5ca6 100644 --- a/synapse/handlers/presence.py +++ b/synapse/handlers/presence.py @@ -1022,7 +1022,7 @@ async def user_syncing( await self._update_states( [ prev_state.copy_and_replace( - last_user_sync_ts=self.clock.time_msec(), + last_user_sync_ts=self.clock.time_msec() ) ] ) From 31d3060008c8b0fb8b45263f102d777512f7f426 Mon Sep 17 00:00:00 2001 From: David Baker Date: Wed, 13 Apr 2022 14:41:30 +0100 Subject: [PATCH 18/19] Add comment on set_state's updating of last_active_ts --- synapse/handlers/presence.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/synapse/handlers/presence.py b/synapse/handlers/presence.py index 0df2510a5ca6..37103bffa9a9 100644 --- a/synapse/handlers/presence.py +++ b/synapse/handlers/presence.py @@ -1017,7 +1017,8 @@ async def user_syncing( ) # otherwise, set the new presence state & update the last sync time, # but don't update last_active_ts as this isn't an indication that - # they've been active. + # they've been active (even though it's probably been updated by + # set_state above) else: await self._update_states( [ From 9e8ef0fce8ca14f4d5b27488993bef59dc078558 Mon Sep 17 00:00:00 2001 From: David Baker Date: Wed, 13 Apr 2022 15:06:03 +0100 Subject: [PATCH 19/19] Put comment in the right place Co-authored-by: Sean Quah <8349537+squahtx@users.noreply.github.com> --- synapse/handlers/presence.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/synapse/handlers/presence.py b/synapse/handlers/presence.py index 37103bffa9a9..d078162c2938 100644 --- a/synapse/handlers/presence.py +++ b/synapse/handlers/presence.py @@ -1000,11 +1000,12 @@ async def user_syncing( await self.set_state( UserID.from_string(user_id), {"presence": presence_state}, True ) - # To keep the single process behaviour consistent with worker mode, run the - # same logic as `update_external_syncs_row`, even though it looks weird - # (this should come from the in-memory cache) + # Retrieve the new state for the logic below. This should come from the + # in-memory cache. prev_state = await self.current_state_for_user(user_id) + # To keep the single process behaviour consistent with worker mode, run the + # same logic as `update_external_syncs_row`, even though it looks weird. if prev_state.state == PresenceState.OFFLINE: await self._update_states( [