From aa548a15f92c4d0c0a45bc2590c03aaccdd5edf4 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Wed, 8 Jan 2025 18:40:13 +0900 Subject: [PATCH 1/4] Add test coverage of disconnection sending `null` presence --- osu.Server.Spectator.Tests/MetadataHubTest.cs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/osu.Server.Spectator.Tests/MetadataHubTest.cs b/osu.Server.Spectator.Tests/MetadataHubTest.cs index 33c309ab..6b870a16 100644 --- a/osu.Server.Spectator.Tests/MetadataHubTest.cs +++ b/osu.Server.Spectator.Tests/MetadataHubTest.cs @@ -105,6 +105,8 @@ public async Task UserStatusIsTrackedAndCleanedUp() using (var usage = await userStates.GetForUse(user_id, true)) Assert.Null(usage.Item); + + mockWatchersGroup.Verify(client => client.UserPresenceUpdated(user_id, null), Times.Once); } [Fact] From a7db6b8f4f9eb3e4a6c7e69bd36367a6a48cbb7c Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Wed, 8 Jan 2025 18:55:50 +0900 Subject: [PATCH 2/4] Add test coverage ensuring appear-offline users don't send state --- osu.Server.Spectator.Tests/MetadataHubTest.cs | 43 +++++++++++++++++-- 1 file changed, 40 insertions(+), 3 deletions(-) diff --git a/osu.Server.Spectator.Tests/MetadataHubTest.cs b/osu.Server.Spectator.Tests/MetadataHubTest.cs index 6b870a16..0d07a6a0 100644 --- a/osu.Server.Spectator.Tests/MetadataHubTest.cs +++ b/osu.Server.Spectator.Tests/MetadataHubTest.cs @@ -79,8 +79,9 @@ public async Task UserStatusIsTrackedAndCleanedUp() using (var usage = await userStates.GetForUse(user_id)) Assert.NotNull(usage.Item); - mockWatchersGroup.Verify(client => client.UserPresenceUpdated(user_id, It.IsAny()), Times.Once); + mockWatchersGroup.Verify(client => client.UserPresenceUpdated(user_id, It.IsAny()), Times.Never); + await hub.UpdateStatus(UserStatus.Online); await hub.UpdateActivity(new UserActivity.ChoosingBeatmap()); using (var usage = await userStates.GetForUse(user_id)) @@ -110,13 +111,19 @@ public async Task UserStatusIsTrackedAndCleanedUp() } [Fact] - public async Task OfflineUserUpdatesAreNotBroadcast() + public async Task UserSwitchingToAppearOfflineHandledCorrectly() { await hub.OnConnectedAsync(); + mockWatchersGroup.Verify(client => client.UserPresenceUpdated(user_id, null), Times.Never); + mockWatchersGroup.Verify(client => client.UserPresenceUpdated(user_id, It.IsAny()), Times.Never); + await hub.UpdateStatus(UserStatus.Online); + mockWatchersGroup.Verify(client => client.UserPresenceUpdated(user_id, null), Times.Never); mockWatchersGroup.Verify(client => client.UserPresenceUpdated(user_id, It.IsAny()), Times.Once); await hub.UpdateStatus(UserStatus.Offline); + mockWatchersGroup.Verify(client => client.UserPresenceUpdated(user_id, null), Times.Once); + mockWatchersGroup.Verify(client => client.UserPresenceUpdated(user_id, It.IsAny()), Times.Once); using (var usage = await userStates.GetForUse(user_id)) { @@ -124,9 +131,38 @@ public async Task OfflineUserUpdatesAreNotBroadcast() Assert.Equal(UserStatus.Offline, usage.Item!.UserStatus); } + await hub.UpdateActivity(new UserActivity.ChoosingBeatmap()); mockWatchersGroup.Verify(client => client.UserPresenceUpdated(user_id, null), Times.Once); + mockWatchersGroup.Verify(client => client.UserPresenceUpdated(user_id, It.IsAny()), Times.Once); + + using (var usage = await userStates.GetForUse(user_id)) + { + Assert.NotNull(usage.Item!.UserActivity); + Assert.IsType(usage.Item!.UserActivity); + } + + await hub.OnDisconnectedAsync(null); + mockWatchersGroup.Verify(client => client.UserPresenceUpdated(user_id, null), Times.Once); + mockWatchersGroup.Verify(client => client.UserPresenceUpdated(user_id, It.IsAny()), Times.Once); + } + + [Fact] + public async Task AppearOfflineUserUpdatesAreNeverBroadcast() + { + await hub.OnConnectedAsync(); + mockWatchersGroup.Verify(client => client.UserPresenceUpdated(user_id, It.IsAny()), Times.Never); + + await hub.UpdateStatus(UserStatus.Offline); + mockWatchersGroup.Verify(client => client.UserPresenceUpdated(user_id, It.IsAny()), Times.Never); + + using (var usage = await userStates.GetForUse(user_id)) + { + Assert.NotNull(usage.Item!.UserStatus); + Assert.Equal(UserStatus.Offline, usage.Item!.UserStatus); + } await hub.UpdateActivity(new UserActivity.ChoosingBeatmap()); + mockWatchersGroup.Verify(client => client.UserPresenceUpdated(user_id, It.IsAny()), Times.Never); using (var usage = await userStates.GetForUse(user_id)) { @@ -134,7 +170,8 @@ public async Task OfflineUserUpdatesAreNotBroadcast() Assert.IsType(usage.Item!.UserActivity); } - mockWatchersGroup.Verify(client => client.UserPresenceUpdated(user_id, null), Times.Exactly(2)); + await hub.OnDisconnectedAsync(null); + mockWatchersGroup.Verify(client => client.UserPresenceUpdated(user_id, It.IsAny()), Times.Never); } [Fact] From 1634db54724a420ed9520c81d0cc65f8d93c700d Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Wed, 8 Jan 2025 18:56:02 +0900 Subject: [PATCH 3/4] Ensure user state is never conveyed for "appear offline" users --- .../Hubs/Metadata/MetadataHub.cs | 39 ++++++++++++++++--- 1 file changed, 33 insertions(+), 6 deletions(-) diff --git a/osu.Server.Spectator/Hubs/Metadata/MetadataHub.cs b/osu.Server.Spectator/Hubs/Metadata/MetadataHub.cs index b8613577..b2b36fc9 100644 --- a/osu.Server.Spectator/Hubs/Metadata/MetadataHub.cs +++ b/osu.Server.Spectator/Hubs/Metadata/MetadataHub.cs @@ -68,7 +68,6 @@ public override async Task OnConnectedAsync() usage.Item = new MetadataClientState(Context.ConnectionId, Context.GetUserId(), versionHash); await logLogin(usage); - await broadcastUserPresenceUpdate(usage.Item.UserId, usage.Item.ToUserPresence()); await Clients.Caller.DailyChallengeUpdated(dailyChallengeUpdater.Current); } } @@ -116,7 +115,8 @@ public async Task UpdateActivity(UserActivity? activity) usage.Item.UserActivity = activity; - await broadcastUserPresenceUpdate(usage.Item.UserId, usage.Item.ToUserPresence()); + if (shouldBroadcastPresentToOtherUsers(usage.Item)) + await broadcastUserPresenceUpdate(usage.Item.UserId, usage.Item.ToUserPresence()); } } @@ -129,7 +129,14 @@ public async Task UpdateStatus(UserStatus? status) if (usage.Item.UserStatus != status) { usage.Item.UserStatus = status; - await broadcastUserPresenceUpdate(usage.Item.UserId, usage.Item.ToUserPresence()); + + if (status == UserStatus.Offline) + { + // special case of users that already broadcast that they are online switching to "appear offline". + await broadcastUserPresenceUpdate(usage.Item.UserId, null); + } + else + await broadcastUserPresenceUpdate(usage.Item.UserId, usage.Item.ToUserPresence()); } } } @@ -205,16 +212,36 @@ public async Task EndWatchingMultiplayerRoom(long id) protected override async Task CleanUpState(MetadataClientState state) { await base.CleanUpState(state); - await broadcastUserPresenceUpdate(state.UserId, null); + if (shouldBroadcastPresentToOtherUsers(state)) + await broadcastUserPresenceUpdate(state.UserId, null); await scoreProcessedSubscriber.UnregisterFromAllMultiplayerRoomsAsync(state.UserId); } private Task broadcastUserPresenceUpdate(int userId, UserPresence? userPresence) { - if (userPresence?.Status == UserStatus.Offline) - userPresence = null; + // we never want appearing offline users to have their status broadcast to other clients. + Debug.Assert(userPresence?.Status != UserStatus.Offline); return Clients.Group(ONLINE_PRESENCE_WATCHERS_GROUP).UserPresenceUpdated(userId, userPresence); } + + private bool shouldBroadcastPresentToOtherUsers(MetadataClientState state) + { + if (state.UserStatus == null) + return false; + + switch (state.UserStatus.Value) + { + case UserStatus.Offline: + return false; + + case UserStatus.DoNotDisturb: + case UserStatus.Online: + return true; + + default: + throw new ArgumentOutOfRangeException(); + } + } } } From 26fb4fd5665d6c5a99edf45463e3b5fed598937b Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Wed, 8 Jan 2025 20:21:56 +0900 Subject: [PATCH 4/4] Fix typo in method name --- osu.Server.Spectator/Hubs/Metadata/MetadataHub.cs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/osu.Server.Spectator/Hubs/Metadata/MetadataHub.cs b/osu.Server.Spectator/Hubs/Metadata/MetadataHub.cs index b2b36fc9..8fd335c2 100644 --- a/osu.Server.Spectator/Hubs/Metadata/MetadataHub.cs +++ b/osu.Server.Spectator/Hubs/Metadata/MetadataHub.cs @@ -115,7 +115,7 @@ public async Task UpdateActivity(UserActivity? activity) usage.Item.UserActivity = activity; - if (shouldBroadcastPresentToOtherUsers(usage.Item)) + if (shouldBroadcastPresenceToOtherUsers(usage.Item)) await broadcastUserPresenceUpdate(usage.Item.UserId, usage.Item.ToUserPresence()); } } @@ -212,7 +212,7 @@ public async Task EndWatchingMultiplayerRoom(long id) protected override async Task CleanUpState(MetadataClientState state) { await base.CleanUpState(state); - if (shouldBroadcastPresentToOtherUsers(state)) + if (shouldBroadcastPresenceToOtherUsers(state)) await broadcastUserPresenceUpdate(state.UserId, null); await scoreProcessedSubscriber.UnregisterFromAllMultiplayerRoomsAsync(state.UserId); } @@ -225,7 +225,7 @@ private Task broadcastUserPresenceUpdate(int userId, UserPresence? userPresence) return Clients.Group(ONLINE_PRESENCE_WATCHERS_GROUP).UserPresenceUpdated(userId, userPresence); } - private bool shouldBroadcastPresentToOtherUsers(MetadataClientState state) + private bool shouldBroadcastPresenceToOtherUsers(MetadataClientState state) { if (state.UserStatus == null) return false;