Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Commit

Permalink
Time out busy presence status & test multi-device busy (#16174)
Browse files Browse the repository at this point in the history 
Add a (long) timeout to when a "busy" device is considered not online.
This does *not* match MSC3026, but is a reasonable thing for an
implementation to do.

Expands tests for the (unstable) busy presence with multiple devices.
  • Loading branch information
@clokep
clokep authored Sep 5, 2023
1 parent ea75346 commit 8b5013d
Show file tree
Hide file tree
Showing 3 changed files with 120 additions and 4 deletions.
1 change: 1 addition & 0 deletions changelog.d/16174.bugfix
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix a long-standing bug where multi-device accounts could cause high load due to presence.
19 changes: 18 additions & 1 deletion synapse/handlers/presence.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,8 @@
# How long to wait until a new /events or /sync request before assuming
# the client has gone.
SYNC_ONLINE_TIMEOUT = 30 * 1000
# Busy status waits longer, but does eventually go offline.
BUSY_ONLINE_TIMEOUT = 60 * 60 * 1000

# How long to wait before marking the user as idle. Compared against last active
IDLE_TIMER = 5 * 60 * 1000
Expand Down Expand Up @@ -2066,7 +2068,15 @@ def handle_timeout(
device_state.last_sync_ts, device_state.last_active_ts
)

if now - sync_or_active > SYNC_ONLINE_TIMEOUT:
# Implementations aren't meant to timeout a device with a busy
# state, but it needs to timeout *eventually* or else the user
# will be stuck in that state.
online_timeout = (
BUSY_ONLINE_TIMEOUT
if device_state.state == PresenceState.BUSY
else SYNC_ONLINE_TIMEOUT
)
if now - sync_or_active > online_timeout:
# Mark the device as going offline.
offline_devices.append(device_id)
device_changed = True
Expand Down Expand Up @@ -2166,6 +2176,13 @@ def handle_update(
new_state = new_state.copy_and_replace(last_federation_update_ts=now)
federation_ping = True

if new_state.state == PresenceState.BUSY:
wheel_timer.insert(
now=now,
obj=user_id,
then=new_state.last_user_sync_ts + BUSY_ONLINE_TIMEOUT,
)

else:
wheel_timer.insert(
now=now,
Expand Down
104 changes: 101 additions & 3 deletions tests/handlers/test_presence.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
from synapse.events.builder import EventBuilder
from synapse.federation.sender import FederationSender
from synapse.handlers.presence import (
BUSY_ONLINE_TIMEOUT,
EXTERNAL_PROCESS_EXPIRY,
FEDERATION_PING_INTERVAL,
FEDERATION_TIMEOUT,
Expand Down Expand Up @@ -912,6 +913,13 @@ def test_set_presence_from_syncing_is_set(self) -> None:
for cases in [
# If both devices have the same state, online should eventually idle.
# Otherwise, the state doesn't change.
(
PresenceState.BUSY,
PresenceState.BUSY,
PresenceState.BUSY,
PresenceState.BUSY,
PresenceState.BUSY,
),
(
PresenceState.ONLINE,
PresenceState.ONLINE,
Expand All @@ -933,7 +941,29 @@ def test_set_presence_from_syncing_is_set(self) -> None:
PresenceState.OFFLINE,
PresenceState.OFFLINE,
),
# If the second device has a "lower" state it should fallback to it.
# If the second device has a "lower" state it should fallback to it,
# except for "busy" which overrides.
(
PresenceState.BUSY,
PresenceState.ONLINE,
PresenceState.BUSY,
PresenceState.BUSY,
PresenceState.BUSY,
),
(
PresenceState.BUSY,
PresenceState.UNAVAILABLE,
PresenceState.BUSY,
PresenceState.BUSY,
PresenceState.BUSY,
),
(
PresenceState.BUSY,
PresenceState.OFFLINE,
PresenceState.BUSY,
PresenceState.BUSY,
PresenceState.BUSY,
),
(
PresenceState.ONLINE,
PresenceState.UNAVAILABLE,
Expand All @@ -956,6 +986,27 @@ def test_set_presence_from_syncing_is_set(self) -> None:
PresenceState.UNAVAILABLE,
),
# If the second device has a "higher" state it should override.
(
PresenceState.ONLINE,
PresenceState.BUSY,
PresenceState.BUSY,
PresenceState.BUSY,
PresenceState.BUSY,
),
(
PresenceState.UNAVAILABLE,
PresenceState.BUSY,
PresenceState.BUSY,
PresenceState.BUSY,
PresenceState.BUSY,
),
(
PresenceState.OFFLINE,
PresenceState.BUSY,
PresenceState.BUSY,
PresenceState.BUSY,
PresenceState.BUSY,
),
(
PresenceState.UNAVAILABLE,
PresenceState.ONLINE,
Expand Down Expand Up @@ -1114,6 +1165,12 @@ def test_set_presence_from_syncing_multi_device(
for workers in (False, True)
for cases in [
# If both devices have the same state, nothing exciting should happen.
(
PresenceState.BUSY,
PresenceState.BUSY,
PresenceState.BUSY,
PresenceState.BUSY,
),
(
PresenceState.ONLINE,
PresenceState.ONLINE,
Expand All @@ -1132,7 +1189,26 @@ def test_set_presence_from_syncing_multi_device(
PresenceState.OFFLINE,
PresenceState.OFFLINE,
),
# If the second device has a "lower" state it should fallback to it.
# If the second device has a "lower" state it should fallback to it,
# except for "busy" which overrides.
(
PresenceState.BUSY,
PresenceState.ONLINE,
PresenceState.BUSY,
PresenceState.BUSY,
),
(
PresenceState.BUSY,
PresenceState.UNAVAILABLE,
PresenceState.BUSY,
PresenceState.BUSY,
),
(
PresenceState.BUSY,
PresenceState.OFFLINE,
PresenceState.BUSY,
PresenceState.BUSY,
),
(
PresenceState.ONLINE,
PresenceState.UNAVAILABLE,
Expand All @@ -1152,6 +1228,24 @@ def test_set_presence_from_syncing_multi_device(
PresenceState.OFFLINE,
),
# If the second device has a "higher" state it should override.
(
PresenceState.ONLINE,
PresenceState.BUSY,
PresenceState.BUSY,
PresenceState.BUSY,
),
(
PresenceState.UNAVAILABLE,
PresenceState.BUSY,
PresenceState.BUSY,
PresenceState.BUSY,
),
(
PresenceState.OFFLINE,
PresenceState.BUSY,
PresenceState.BUSY,
PresenceState.BUSY,
),
(
PresenceState.UNAVAILABLE,
PresenceState.ONLINE,
Expand Down Expand Up @@ -1266,7 +1360,11 @@ def test_set_presence_from_non_syncing_multi_device(

# 8. Advance such that the second device should be discarded (the sync timeout),
# then pump so _handle_timeouts function to called.
self.reactor.advance(SYNC_ONLINE_TIMEOUT / 1000)
if dev_1_state == PresenceState.BUSY or dev_2_state == PresenceState.BUSY:
timeout = BUSY_ONLINE_TIMEOUT
else:
timeout = SYNC_ONLINE_TIMEOUT
self.reactor.advance(timeout / 1000)
self.reactor.pump([5])

# 9. There are no more devices, should be offline.
Expand Down

0 comments on commit 8b5013d

Please sign in to comment.