ppy / osu-server-spectator Goto Github PK

Basically we need an implementation similar to osu-web to ensure correct mods are chosen by the client.

This is already enforced client side but we shouldn't be relying on that forever.

We need a way to stop older clients from connecting. This is already possible on score token retrieval (via checks osu-web side) but we don't have a flow in place for this server.

See ppy/osu#11686 for an example of an issue caused by old versions connecting to the server.

Opening for tracking purposes. Haven't figure out the cause yet..

2022-03-27 14:16:58 [verbose]: [user:14613516] [room:162666] Editing playlist item 752460 for beatmap 1581930
2022-03-27 14:16:58 [verbose]: [user:27352453] [room:162666] User state changed from Ready to Idle
2022-03-27 14:16:58 [error]: [user:14613516] Failed to invoke hub method: EditPlaylistItem(osu.Game.Online.Rooms.MultiplayerPlaylistItem)
2022-03-27 14:16:58 [error]: System.ObjectDisposedException: Cannot access a disposed object.
2022-03-27 14:16:58 [error]: Object name: 'MultiplayerHub'.
2022-03-27 14:16:58 [error]: at Microsoft.AspNetCore.SignalR.Hub.CheckDisposed()
2022-03-27 14:16:58 [error]: at osu.Server.Spectator.Hubs.MultiplayerHub.changeAndBroadcastUserState(ServerMultiplayerRoom room, MultiplayerRoomUser user, MultiplayerUserState state) in /Users/dean/Projects/osu-server-spectator/osu.Server.Spectator/Hubs/MultiplayerHub.cs:line 739
2022-03-27 14:16:58 [error]: at osu.Server.Spectator.Hubs.MultiplayerHub.unreadyAllUsers(ServerMultiplayerRoom room) in /Users/dean/Projects/osu-server-spectator/osu.Server.Spectator/Hubs/MultiplayerHub.cs:line 982
2022-03-27 14:16:58 [error]: at osu.Server.Spectator.Hubs.MultiplayerHub.OnPlaylistItemChanged(ServerMultiplayerRoom room, MultiplayerPlaylistItem item) in /Users/dean/Projects/osu-server-spectator/osu.Server.Spectator/Hubs/MultiplayerHub.cs:line 939
2022-03-27 14:16:58 [error]: at osu.Server.Spectator.Hubs.MultiplayerQueue.EditItem(MultiplayerPlaylistItem item, MultiplayerRoomUser user) in /Users/dean/Projects/osu-server-spectator/osu.Server.Spectator/Hubs/MultiplayerQueue.cs:line 175
2022-03-27 14:16:58 [error]: at osu.Server.Spectator.Hubs.MultiplayerHub.EditPlaylistItem(MultiplayerPlaylistItem item) in /Users/dean/Projects/osu-server-spectator/osu.Server.Spectator/Hubs/MultiplayerHub.cs:line 500
2022-03-27 14:16:58 [error]: at Microsoft.AspNetCore.SignalR.Internal.DefaultHubDispatcher`1.ExecuteMethod(ObjectMethodExecutor methodExecutor, Hub hub, Object[] arguments)
2022-03-27 14:16:58 [error]: at osu.Server.Spectator.LoggingHubFilter.InvokeMethodAsync(HubInvocationContext invocationContext, Func`2 next) in /Users/dean/Projects/osu-server-spectator/osu.Server.Spectator/LoggingHubFilter.cs:line 37
2022-03-27 14:16:59 [verbose]: [user:10872216] User disconnected
2022-03-27 14:16:59 [verbose]: [user:12176833] Connected

We should likely be tracking which instance is responsible for rooms at a database level, and perform cleanup on non-running instances (when a new instance is spawned).

This will cover the scenario of a crashed instance, where currently rooms are left with ended_at = null but in an un-joinable state.

(posting this from my personal todo list, not super high priority)

The current method of ensuring state consistency across (potentially) multiple connects and disconnects by the same user is to clean up any state which is owned by a previous user before allowing a new connection on the server.

This works fine.. until the same user connects twice to the server, not taking out a state either time. You now have a situation where there are two connections from the same user potentially sharing state data.

It shouldn't have any limit. The 3 beatmap limit is intended for other queue modes.

As brought up in ppy/osu#18590 (reply in thread), we need to report counts of users per release so it can be shown in places like the changelog header.

Here's bancho's implementation of this for reference. The important part is the UPDATE statement (the INSERT one is not relevant for the newer implemetation) but I've included the whole stats method for context.

        private static bool isUpdatingStats;

        private static void updateStats()
        {
            if (isUpdatingStats || !ReportClientCounts) return;

            var countOsu = UserManager.CountOsu;

            // avoid reporting low stats by accident
            if (countOsu < 32) return;

            isUpdatingStats = true;

            RunThread(() =>
            {
                try
                {
                    Database.RunNonQueryLowPriority(
                        "INSERT INTO osu_banchostats (users_irc, users_osu, multiplayer_games) VALUES (@irc, @osu, @mp)",
                        new MySqlParameter("irc", UserManager.CountIrc),
                        new MySqlParameter("osu", countOsu),
                        new MySqlParameter("mp", Lobby.Matches.Count));

                    SortedDictionary<string, int> versionCounts = new SortedDictionary<string, int>();

                    foreach (Client c in UserManager.Clients)
                    {
                        NetClient nc = c as NetClient;
                        if (nc == null) continue;

                        string version = Regex.Replace(nc.Version, "[^0-9.]", string.Empty);

                        if (versionCounts.ContainsKey(version))
                            versionCounts[version]++;
                        else
                            versionCounts[version] = 1;
                    }

                    foreach (var v in versionCounts)
                    {
                        Database.RunNonQueryLowPriority("UPDATE osu_builds SET users = @users WHERE version = @version",
                            new MySqlParameter("users", v.Value),
                            new MySqlParameter("version", v.Key)
                        );
                    }
                }
                catch (Exception e)
                {
                    Bacon.WriteLine("Database connection failed!");
                    Bacon.WriteLine(e.ToString());
                }

                isUpdatingStats = false;
            });
        }

@ThePooN discovered during some infra work that a Redis connection failure will bring down spectator flows completely with

As is, redis definitely wasn't supposed to be this service-critical as its only purpose was delivering user statistics updates which should not be bringing down the entirety of spectator flows. It was definitely not intended, and is only happening due to DI behaviour foibles combined with signalr design (tl;dr: hubs are instantiated per client RPC call, which means that a service failing to resolve - such as IConnectionMultiplexer - will cause all of the operations that touch that hub to fail).

We probably need to put some kind of backstop against that happening in place. Or possibly even reconsider osu-server-spectator middlemanning in this process whatsoever and take a look at something like https://centrifugal.dev/ instead.

In theory, we should be able to do something like this:

diff --git a/osu.Game/Screens/Play/SubmittingPlayer.cs b/osu.Game/Screens/Play/SubmittingPlayer.cs
index 5fa6508a31..663ea9ab81 100644
--- a/osu.Game/Screens/Play/SubmittingPlayer.cs
+++ b/osu.Game/Screens/Play/SubmittingPlayer.cs
@@ -131,7 +131,8 @@ protected override async Task PrepareScoreForResultsAsync(Score score)
             score.ScoreInfo.Date = DateTimeOffset.Now;
 
             await submitScore(score).ConfigureAwait(false);
-            spectatorClient.EndPlaying(GameplayState);
+            long onlineId = score.ScoreInfo.OnlineID;
+            spectatorClient.EndPlaying(GameplayState, onlineId);
         }
 
         [Resolved]

This would hopefully avoid the polling resolution process we have in place for replay uploads. We should be able to trust the client's returned ID as we can cross-check with the solo_score_tokens table.

There are various cases where we retrieve a user state but don't set the state against it due to an error or otherwise early exit:

This seems fine at first glance, but looking at state clean-up code, the user state will not be removed completely if the item is null:

This leads to states remaining in a "stuck" state until the user next connects. A non-disconnect will be able to reuse the state on next connect, so this isn't a critical issue, but one we should still resolve.

Proposals:

• Destroy a state usage on dispose if the item is null
• Somehow enforce non-null on the item in a different way
• Have StatefulUserHub track connectionId outside of ClientState

First option being the simplest, if we are happy with that direction.

User 7010695 ending play session (Beatmap:2643109 Mods: Ruleset:0) fail: Microsoft.AspNetCore.SignalR.Internal.DefaultHubDispatcher[8] Failed to invoke hub method 'JoinRoom'. MySqlConnector.MySqlException (0x80004005): Duplicate entry '38413-12918535' for key 'multiplayer_rooms_high.multiplayer_rooms_high_room_id_user_id_unique' ---> MySqlConnector.MySqlException (0x80004005): Duplicate entry '38413-12918535' for key 'multiplayer_rooms_high.multiplayer_rooms_high_room_id_user_id_unique' at MySqlConnector.Core.ServerSession.ReceiveReplyAsyncAwaited(ValueTask`1 task) in /_/src/MySqlConnector/Core/ServerSession.cs:line 817 at MySqlConnector.Core.ResultSet.ReadResultSetHeaderAsync(IOBehavior ioBehavior) in /_/src/MySqlConnector/Core/ResultSet.cs:line 49 at MySqlConnector.MySqlDataReader.ActivateResultSet(CancellationToken cancellationToken) in /_/src/MySqlConnector/MySqlDataReader.cs:line 118 at MySqlConnector.MySqlDataReader.CreateAsync(CommandListPosition commandListPosition, ICommandPayloadCreator payloadCreator, IDictionary`2 cachedProcedures, IMySqlCommand command, CommandBehavior behavior, IOBehavior ioBehavior, CancellationToken cancellationToken) in /_/src/MySqlConnector/MySqlDataReader.cs:line 437 at MySqlConnector.Core.CommandExecutor.ExecuteReaderAsync(IReadOnlyList`1 commands, ICommandPayloadCreator payloadCreator, CommandBehavior behavior, IOBehavior ioBehavior, CancellationToken cancellationToken) in /_/src/MySqlConnector/Core/CommandExecutor.cs:line 60 at MySqlConnector.MySqlCommand.ExecuteNonQueryAsync(IOBehavior ioBehavior, CancellationToken cancellationToken) in /_/src/MySqlConnector/MySqlCommand.cs:line 264 at Dapper.SqlMapper.ExecuteImplAsync(IDbConnection cnn, CommandDefinition command, Object param) in /_/Dapper/SqlMapper.Async.cs:line 654 at osu.Server.Spectator.Hubs.MultiplayerHub.UpdateDatabaseParticipants(MultiplayerRoom room) in /Users/dean/Projects/osu-server-spectator/osu.Server.Spectator/Hubs/MultiplayerHub.cs:line 422 at osu.Server.Spectator.Hubs.MultiplayerHub.JoinRoom(Int64 roomId) in /Users/dean/Projects/osu-server-spectator/osu.Server.Spectator/Hubs/MultiplayerHub.cs:line 94 at lambda_method(Closure , Object ) at Microsoft.AspNetCore.SignalR.Internal.DefaultHubDispatcher`1.ExecuteHubMethod(ObjectMethodExecutor methodExecutor, THub hub, Object[] arguments) at Microsoft.AspNetCore.SignalR.Internal.DefaultHubDispatcher`1.<>c**DisplayClass13_0.<

Looking into this, it would seem that the case is where a user reconnects to the multiplayer hub before their previous connection times out (and triggers a disconnection server-side). This means the user state (which is stored against only user_id) may be carried across to the new user as it is only removed on disconnection.

It also means that the reverse can happen: a user can reconnect and start using the state, only to have their previous connection disconnect and clear it.

This is a pretty critical oversight.

As mentioned in ppy/osu#23159

Currently we are only conveying change information when a user is responsible for making a change to a beatmap. Eventually we'll want to change this to also include automated (or manual) changes to beatmap status which aren't triggered by a BSS change.

Relevant code:

https://sentry.ppy.sh/organizations/ppy/issues/34904/

I don't think we're tracking this anywhere, and it's quite important. Likely a simple change.

Apparently this is still happening even though the intention behind #89 was for new users to come after the first items from existing users. So if the queue looks like:

User1
User2
User1

When User3 adds their item, the queue would turn into

User1
User2
User3
User1

Now that we are doing rolling deploys, it is possible that users may be in a multiplayer room waiting for new players without being aware that all new players are going to be joining a server that prohibits them from joining the room.

To resolve this, users with existing multiplayer rooms should be made aware of a server shutdown. A countdown timer using the existing flow seems like the perfect way to do this, notifying the osu! client of an imminent shutdown and allowing the user to take action as they wish.

Will require support at the osu-side as well.

Disregarding all discussion above, a first point to address is the ability for a user to reconnect to osu-server-spectator. Regardless of whether we move responsibilities around, we need to ensure that a dropped websocket connection is not the end of the world for multiplayer, spectator, and (currently) replay storage.

Once we have this in place, we can decide whether responsibilities need to be shuffled.

Originally posted by @peppy in ppy/osu#24609 (comment)

The key part for the issue above is spectating flows, as they are the ones that implicitly participate in submission to upload replays. This will likely require a reconciliation flow which ensures that the client resends any frames the server might not have received.

The rest is also important for other reasons, but strictly speaking not for the aforementioned issue.

Describe the bug: Having the same user connected on multiple devices will cause the multiplayer server to give the user an invalid state.

Reproduction Steps
Upon testing, device A was a PC while device B was an Android.

1. Create a multiplayer lobby on device A
2. Launch osu! on device B
3. Try updating settings (such as beatmap) on device A's lobby (an error should throw saying the user is not in a multiplayer lobby)

At this point, device A can just simply create a new lobby to get back to a correct state. I can't get device B to create a lobby however even after restarting (probably a different bug regarding creating lobbies on Android devices).

Screenshots or videos showing encountered issue:

osu!lazer version:
2021.815.0-lazer

Logs:
N/A

Currently when a gameplay session finishes, all (non-expired) playlist order values are updated, causing one PlaylistItemChanged even to be fired for each queued item. This could be avoided if order values were not updated unless required.