From a3780574f503561567c2f0b0132076f372dc390a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Tue, 31 Mar 2026 09:41:42 +0200 Subject: [PATCH 1/2] Do not send replay frames to spectator server if initial begin play invocation failed 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 ignored as no-ops: https://github.com/ppy/osu-server-spectator/blob/7bab117e9d161455485368f63a0607a9e53f9f8a/osu.Server.Spectator/Hubs/Spectator/SpectatorHub.cs#L122-L125 https://github.com/ppy/osu-server-spectator/blob/7bab117e9d161455485368f63a0607a9e53f9f8a/osu.Server.Spectator/Hubs/Spectator/SpectatorHub.cs#L153-L157 - 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. --- .../Online/Spectator/OnlineSpectatorClient.cs | 13 ++-- osu.Game/Online/Spectator/SpectatorClient.cs | 60 +++++++++++++++---- .../Visual/Spectator/TestSpectatorClient.cs | 5 +- 3 files changed, 59 insertions(+), 19 deletions(-) diff --git a/osu.Game/Online/Spectator/OnlineSpectatorClient.cs b/osu.Game/Online/Spectator/OnlineSpectatorClient.cs index 29d174f8e3ca..22f8cfbf2b44 100644 --- a/osu.Game/Online/Spectator/OnlineSpectatorClient.cs +++ b/osu.Game/Online/Spectator/OnlineSpectatorClient.cs @@ -7,6 +7,7 @@ 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 @@ private void load(IAPIProvider api) } } - 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 @@ protected override async Task BeginPlayingInternal(long? scoreToken, SpectatorSt 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 f245e8cf3a27..8c998ce32661 100644 --- a/osu.Game/Online/Spectator/SpectatorClient.cs +++ b/osu.Game/Online/Spectator/SpectatorClient.cs @@ -10,6 +10,7 @@ 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 @@ public void BeginPlaying(long? scoreToken, GameplayState state, Score score) 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 @@ public void BeginPlaying(long? scoreToken, GameplayState state, Score score) 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).ContinueWith(t => + { + bool success = t.GetResultSafely(); - BeginPlayingInternal(currentScoreToken, currentState); + 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 @@ public void EndPlaying(GameplayState state) 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 @@ public void EndPlaying(GameplayState state) }); } + 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 @@ public void StopWatchingUser(int userId) }); } - 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); diff --git a/osu.Game/Tests/Visual/Spectator/TestSpectatorClient.cs b/osu.Game/Tests/Visual/Spectator/TestSpectatorClient.cs index 5d33afd2881a..0eb1e7c28575 100644 --- a/osu.Game/Tests/Visual/Spectator/TestSpectatorClient.cs +++ b/osu.Game/Tests/Visual/Spectator/TestSpectatorClient.cs @@ -159,14 +159,15 @@ void flush() } } - 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) From d3c820b166b3596405b6013893b5d6595d8d4b26 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Tue, 31 Mar 2026 12:27:19 +0200 Subject: [PATCH 2/2] Drop pending frames if spectator transitioned into not-playing state from begin play failure --- osu.Game/Online/Spectator/SpectatorClient.cs | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/osu.Game/Online/Spectator/SpectatorClient.cs b/osu.Game/Online/Spectator/SpectatorClient.cs index 8c998ce32661..9489fb4ad057 100644 --- a/osu.Game/Online/Spectator/SpectatorClient.cs +++ b/osu.Game/Online/Spectator/SpectatorClient.cs @@ -389,6 +389,16 @@ private void purgePendingFrames() 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);