From ca44d8233dc895516c627e1a03226e94d9600913 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Wed, 8 Apr 2026 20:40:32 +0200 Subject: [PATCH] Do not send replay frames to spectator server if initial begin play invocation failed (#37159) RFC Until now, if the initial `BeginPlaySession()` call failed, the client would continue operating as if it didn't - it would still continue to send frames and call `EndPlaySession()` at the end of a session. Server-side, two things generally can happen after this: - The sent frames and the `EndPlaySession()` call are [completely](https://github.com/ppy/osu-server-spectator/blob/7bab117e9d161455485368f63a0607a9e53f9f8a/osu.Server.Spectator/Hubs/Spectator/SpectatorHub.cs#L122-L125) [ignored](https://github.com/ppy/osu-server-spectator/blob/7bab117e9d161455485368f63a0607a9e53f9f8a/osu.Server.Spectator/Hubs/Spectator/SpectatorHub.cs#L153-L157) as no-ops, or - A hub filter (like `ClientVersionChecker`) that failed the initial `BeginPlaySession()` call continues to fail the calls to `SendFrameData()` and `EndPlaySession()`, all the while creating a storm in logs, because it needs to throw `HubException`s to communicate to users that they need to update their game, and the exceptions can't be silenced from logs because they look like every other failure. To that end, this has two goals: reduce useless network traffic, and reduce noise in spectator server logs after the client version checks were recently reactivated. Probably needs tests, but unsure if everyone's going to be on board with this to begin with to be quite frank, so I'm leaving tests for when I'm told this needs tests. --- .../Online/Spectator/OnlineSpectatorClient.cs | 13 ++-- osu.Game/Online/Spectator/SpectatorClient.cs | 70 +++++++++++++++---- .../Visual/Spectator/TestSpectatorClient.cs | 5 +- 3 files changed, 69 insertions(+), 19 deletions(-) diff --git a/osu.Game/Online/Spectator/OnlineSpectatorClient.cs b/osu.Game/Online/Spectator/OnlineSpectatorClient.cs index 29d174f8e3..22f8cfbf2b 100644 --- a/osu.Game/Online/Spectator/OnlineSpectatorClient.cs +++ b/osu.Game/Online/Spectator/OnlineSpectatorClient.cs @@ -7,6 +7,7 @@ using System.Threading.Tasks; using Microsoft.AspNetCore.SignalR.Client; using osu.Framework.Allocation; using osu.Framework.Bindables; +using osu.Framework.Logging; using osu.Game.Online.API; using osu.Game.Online.Multiplayer; @@ -51,16 +52,17 @@ namespace osu.Game.Online.Spectator } } - protected override async Task BeginPlayingInternal(long? scoreToken, SpectatorState state) + protected override async Task BeginPlayingInternal(long? scoreToken, SpectatorState state) { if (!IsConnected.Value) - return; + return false; Debug.Assert(connection != null); try { await connection.InvokeAsync(nameof(ISpectatorServer.BeginPlaySession), scoreToken, state).ConfigureAwait(false); + return true; } catch (Exception exception) { @@ -69,11 +71,14 @@ namespace osu.Game.Online.Spectator Debug.Assert(connector != null); await connector.Reconnect().ConfigureAwait(false); - await BeginPlayingInternal(scoreToken, state).ConfigureAwait(false); + return await BeginPlayingInternal(scoreToken, state).ConfigureAwait(false); } // Exceptions can occur if, for instance, the locally played beatmap doesn't have a server-side counterpart. - // For now, let's ignore these so they don't cause unobserved exceptions to appear to the user (and sentry). + // For now, let's ignore these so they don't cause unobserved exceptions to appear to the user (and sentry), + // but log to disk for diagnostic purposes. + Logger.Log($"{nameof(OnlineSpectatorClient)}.{nameof(BeginPlayingInternal)} failed: {exception.Message}", LoggingTarget.Network); + return false; } } diff --git a/osu.Game/Online/Spectator/SpectatorClient.cs b/osu.Game/Online/Spectator/SpectatorClient.cs index f245e8cf3a..9489fb4ad0 100644 --- a/osu.Game/Online/Spectator/SpectatorClient.cs +++ b/osu.Game/Online/Spectator/SpectatorClient.cs @@ -10,6 +10,7 @@ using JetBrains.Annotations; using osu.Framework.Allocation; using osu.Framework.Bindables; using osu.Framework.Development; +using osu.Framework.Extensions; using osu.Framework.Graphics; using osu.Framework.Logging; using osu.Game.Beatmaps; @@ -216,8 +217,6 @@ namespace osu.Game.Online.Spectator if (isPlaying) throw new InvalidOperationException($"Cannot invoke {nameof(BeginPlaying)} when already playing"); - isPlaying = true; - // transfer state at point of beginning play currentState.BeatmapID = score.ScoreInfo.BeatmapInfo!.OnlineID; currentState.RulesetID = score.ScoreInfo.RulesetID; @@ -225,12 +224,27 @@ namespace osu.Game.Online.Spectator currentState.State = SpectatedUserState.Playing; currentState.MaximumStatistics = state.ScoreProcessor.MaximumStatistics; - currentBeatmap = state.Beatmap; - currentScore = score; - currentScoreToken = scoreToken; - currentScoreProcessor = state.ScoreProcessor; + setStateForScore(scoreToken, state, score); - BeginPlayingInternal(currentScoreToken, currentState); + BeginPlayingInternal(currentScoreToken, currentState).ContinueWith(t => + { + bool success = t.GetResultSafely(); + + if (!success) + { + Logger.Log($"Clearing {nameof(SpectatorClient)} state due to failed {nameof(BeginPlayingInternal)} call."); + Schedule(() => + { + clearScoreState(); + + currentState.BeatmapID = null; + currentState.RulesetID = null; + currentState.Mods = []; + currentState.State = SpectatedUserState.Idle; + currentState.MaximumStatistics = []; + }); + } + }); }); } @@ -278,11 +292,7 @@ namespace osu.Game.Online.Spectator if (pendingFrames.Count > 0) purgePendingFrames(); - isPlaying = false; - currentBeatmap = null; - currentScore = null; - currentScoreProcessor = null; - currentScoreToken = null; + clearScoreState(); if (state.HasPassed) currentState.State = SpectatedUserState.Passed; @@ -295,6 +305,26 @@ namespace osu.Game.Online.Spectator }); } + private void setStateForScore(long? scoreToken, GameplayState state, Score score) + { + isPlaying = true; + + currentBeatmap = state.Beatmap; + currentScore = score; + currentScoreToken = scoreToken; + currentScoreProcessor = state.ScoreProcessor; + } + + private void clearScoreState() + { + isPlaying = false; + + currentBeatmap = null; + currentScore = null; + currentScoreProcessor = null; + currentScoreToken = null; + } + public virtual void WatchUser(int userId) { Debug.Assert(ThreadSafety.IsUpdateThread); @@ -326,7 +356,11 @@ namespace osu.Game.Online.Spectator }); } - protected abstract Task BeginPlayingInternal(long? scoreToken, SpectatorState state); + /// + /// Contains the actual implementation of the "begin play" operation. + /// + /// Whether the server-side invocation to start play succeeded. + protected abstract Task BeginPlayingInternal(long? scoreToken, SpectatorState state); protected abstract Task SendFramesInternal(FrameDataBundle bundle); @@ -355,6 +389,16 @@ namespace osu.Game.Online.Spectator if (pendingFrames.Count == 0) return; + if (!isPlaying) + { + // it is possible for this to happen if the `BeginPlayingInternal()` call takes a long time, + // the client accumulates a purgeable bundle of frames in the meantime, + // and then `BeginPlayingInternal()` finally fails and `clearScoreState()` is called to abort the streaming session. + Logger.Log($"{nameof(SpectatorClient)} dropping pending frames as the user is no longer considered to be playing."); + pendingFrames.Clear(); + return; + } + Debug.Assert(currentScore != null); Debug.Assert(currentScoreProcessor != null); diff --git a/osu.Game/Tests/Visual/Spectator/TestSpectatorClient.cs b/osu.Game/Tests/Visual/Spectator/TestSpectatorClient.cs index 5d33afd288..0eb1e7c285 100644 --- a/osu.Game/Tests/Visual/Spectator/TestSpectatorClient.cs +++ b/osu.Game/Tests/Visual/Spectator/TestSpectatorClient.cs @@ -159,14 +159,15 @@ namespace osu.Game.Tests.Visual.Spectator } } - protected override Task BeginPlayingInternal(long? scoreToken, SpectatorState state) + protected override async Task BeginPlayingInternal(long? scoreToken, SpectatorState state) { // Track the local user's playing beatmap ID. Debug.Assert(state.BeatmapID != null); userBeatmapDictionary[api.LocalUser.Value.Id] = state.BeatmapID.Value; userModsDictionary[api.LocalUser.Value.Id] = state.Mods.ToArray(); - return ((ISpectatorClient)this).UserBeganPlaying(api.LocalUser.Value.Id, state); + await ((ISpectatorClient)this).UserBeganPlaying(api.LocalUser.Value.Id, state).ConfigureAwait(false); + return true; } protected override Task SendFramesInternal(FrameDataBundle bundle)