Presence fix for idle mode

changelog.d/15989.bugfix

@@ -0,0 +1 @@
+Allow presence to achieve idle state.

synapse/handlers/presence.py

@@ -19,6 +19,7 @@
 The methods that define policy are:
     - PresenceHandler._update_states
     - PresenceHandler._handle_timeouts
+    - PresenceHandler.set_state
     - should_notify
 """
 import abc
@@ -444,20 +445,18 @@ def send_user_sync(self, user_id: str, is_syncing: bool, last_sync_ms: int) -> N
             )
 
     def mark_as_coming_online(self, user_id: str) -> None:
-        """A user has started syncing. Send a UserSync to the presence writer,
-        unless they had recently stopped syncing.
-        """
-        going_offline = self.users_going_offline.pop(user_id, None)
-        if not going_offline:
-            # Safe to skip because we haven't yet told the presence writer they
-            # were offline
-            self.send_user_sync(user_id, True, self.clock.time_msec())
+        """A user has started syncing. Send a UserSync to the presence writer."""
+        # Need to remove the user_id from users_going_offline or the looping call will
+        # tell the presence writer that the user went offline.
+        self.users_going_offline.pop(user_id, None)
+        self.send_user_sync(user_id, True, self.clock.time_msec())
 
     def mark_as_going_offline(self, user_id: str) -> None:
-        """A user has stopped syncing. We wait before notifying the presence
-        writer as its likely they'll come back soon. This allows us to avoid
-        sending a stopped syncing immediately followed by a started syncing
-        notification to the presence writer
+        """A user has stopped syncing. We allow the timeout(UPDATE_SYNCING_USERS_MS)
+        to expire and the looping_call to handle notifying the presence writer as its
+        likely they'll come back soon. This allows us to avoid sending a stopped
+        syncing immediately followed by a started syncing notification to the
+        presence writer
         """
         self.users_going_offline[user_id] = self.clock.time_msec()
 
@@ -486,11 +485,12 @@ async def user_syncing(
         if prev_state.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.
+            # Note that this causes last_active_ts to be incremented only if changing
+            # state to 'online' from 'offline'.
             await self.set_state(
-                UserID.from_string(user_id), {"presence": presence_state}, True
+                UserID.from_string(user_id),
+                {"presence": presence_state},
+                ignore_status_msg=True,
             )
 
         curr_sync = self._user_to_num_current_syncs.get(user_id, 0)
@@ -1004,13 +1004,12 @@ async def user_syncing(
             # 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:
-                # 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.
+                # process modes.
                 await self.set_state(
-                    UserID.from_string(user_id), {"presence": presence_state}, True
+                    UserID.from_string(user_id),
+                    {"presence": presence_state},
+                    ignore_status_msg=True,
                 )
                 # Retrieve the new state for the logic below. This should come from the
                 # in-memory cache.
@@ -1254,9 +1253,12 @@ async def set_state(
         if not ignore_status_msg:
             new_fields["status_msg"] = status_msg
 
-        if presence == PresenceState.ONLINE or (
-            presence == PresenceState.BUSY and self._busy_presence_enabled
-        ):
+        if (
+            prev_state.state == PresenceState.OFFLINE
+            and presence == PresenceState.ONLINE
+        ) or (presence == PresenceState.BUSY and self._busy_presence_enabled):
+            # By updating last_active_ts in this way, currently_active will be triggered
+            # in handle_update()
             new_fields["last_active_ts"] = self.clock.time_msec()
 
         await self._update_states(
@@ -1954,6 +1956,9 @@ def handle_update(
             - persist_and_notify: whether to persist and notify people
             - federation_ping: whether we should send a ping over federation
     """
+    # Note: if the previous state was OFFLINE and we are now ONLINE, this is most likely
+    # caused by a call to the /sync endpoint. If that is the case, last_active_ts would
+    # have been set in PresenceHandler.set_state()
     user_id = new_state.user_id
 
     persist_and_notify = False
@@ -1977,6 +1982,14 @@ def handle_update(
                     obj=user_id,
                     then=new_state.last_active_ts + LAST_ACTIVE_GRANULARITY,
                 )
+            if prev_state.state == PresenceState.UNAVAILABLE:
+                # If we had idled out, but are still syncing, then new state will look
+                # like it's online. Verify with last_active_ts, as that is a sign of a
+                # pro-active event. Override that we are online when we probably aren't.
+                if now - new_state.last_active_ts > IDLE_TIMER:
+                    new_state = new_state.copy_and_replace(
+                        state=PresenceState.UNAVAILABLE
+                    )
 
         if new_state.state != PresenceState.OFFLINE:
             # User has stopped syncing

tests/handlers/test_presence.py

@@ -306,6 +306,37 @@ def test_online_to_idle(self) -> None:
             any_order=True,
         )
 
+    def test_idle_while_syncing_stays_idle(self) -> None:
+        """Test that syncing alone doesn't affect unavailable state"""
+        wheel_timer = Mock()
+        user_id = "@foo:bar"
+        now = 5000000
+        time_went_idle = now - IDLE_TIMER - 1
+
+        prev_state = UserPresenceState.default(user_id)
+        prev_state = prev_state.copy_and_replace(
+            state=PresenceState.UNAVAILABLE,
+            last_active_ts=time_went_idle,
+            currently_active=False,
+        )
+
+        # The key to the test here is that while syncing, we are being told we are
+        # 'online'. But if the last_active_ts hasn't moved, then we are actually still
+        # idle.
+        new_state = prev_state.copy_and_replace(state=PresenceState.ONLINE)
+
+        state, persist_and_notify, federation_ping = handle_update(
+            prev_state, new_state, is_mine=True, wheel_timer=wheel_timer, now=now
+        )
+
+        self.assertFalse(persist_and_notify)
+        self.assertEqual(prev_state.state, state.state)
+        self.assertEqual(state.last_federation_update_ts, now)
+        self.assertEqual(prev_state.state, state.state)
+        self.assertEqual(prev_state.status_msg, state.status_msg)
+
+        self.assertEqual(wheel_timer.insert.call_count, 2)
+
     def test_persisting_presence_updates(self) -> None:
         """Tests that the latest presence state for each user is persisted correctly"""
         # Create some test users and presence states for them
@@ -773,6 +804,32 @@ def test_set_presence_from_syncing_keeps_busy(
         # we should still be busy
         self.assertEqual(state.state, PresenceState.BUSY)
 
+    def test_syncing_appropriately_handles_last_active_ts(self) -> None:
+        """Test that syncing does not bump last_active_ts when already online"""
+
+        user_id = "@test:server"
+
+        # Start from a time that is not zero
+        self.reactor.advance(2)
+
+        self._set_presencestate_with_status_msg(user_id, PresenceState.ONLINE, None)
+
+        prev_state = self.get_success(
+            self.presence_handler.get_state(UserID.from_string(user_id))
+        )
+
+        # Poke the reactor so some time passes
+        self.reactor.advance(2)
+        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))
+        )
+        # last_active_ts should not have changed as no pro-active event has occurred
+        self.assertEqual(prev_state.last_active_ts, state.last_active_ts)
+
     def _set_presencestate_with_status_msg(
         self, user_id: str, state: str, status_msg: Optional[str]
     ) -> None: