Closes https://github.com/ppy/osu/issues/26035.
`submitOnFailOrQuit()`, as the name suggests, can be called both when
the player has failed, or when the player screen is being exited from.
Notably, when perfect mod with auto-retry is active, the two happen
almost simultaneously.
This double call exposes a data race in `submitScore()` concerning the
handling of `scoreSubmissionSource`. The race could be experimentally
confirmed by applying the following patch:
diff --git a/osu.Game/Screens/Play/SubmittingPlayer.cs b/osu.Game/Screens/Play/SubmittingPlayer.cs
index 83adf1f960..76dd29bbdb 100644
--- a/osu.Game/Screens/Play/SubmittingPlayer.cs
+++ b/osu.Game/Screens/Play/SubmittingPlayer.cs
@@ -228,6 +228,7 @@ private Task submitScore(Score score)
return Task.CompletedTask;
}
+ Logger.Log($"{nameof(scoreSubmissionSource)} is {(scoreSubmissionSource == null ? "null" : "not null")}");
if (scoreSubmissionSource != null)
return scoreSubmissionSource.Task;
@@ -237,6 +238,7 @@ private Task submitScore(Score score)
Logger.Log($"Beginning score submission (token:{token.Value})...");
+ Logger.Log($"creating new {nameof(scoreSubmissionSource)}");
scoreSubmissionSource = new TaskCompletionSource<bool>();
var request = CreateSubmissionRequest(score, token.Value);
which would result in the following log output:
[runtime] 2024-01-02 09:54:13 [verbose]: scoreSubmissionSource is null
[runtime] 2024-01-02 09:54:13 [verbose]: scoreSubmissionSource is null
[runtime] 2024-01-02 09:54:13 [verbose]: Beginning score submission (token:36780)...
[runtime] 2024-01-02 09:54:13 [verbose]: creating new scoreSubmissionSource
[runtime] 2024-01-02 09:54:13 [verbose]: Beginning score submission (token:36780)...
[runtime] 2024-01-02 09:54:13 [verbose]: creating new scoreSubmissionSource
[network] 2024-01-02 09:54:13 [verbose]: Performing request osu.Game.Online.Solo.SubmitSoloScoreRequest
[network] 2024-01-02 09:54:14 [verbose]: Request to https://dev.ppy.sh/api/v2/beatmaps/869310/solo/scores/36780 successfully completed!
[network] 2024-01-02 09:54:14 [verbose]: SubmitSoloScoreRequest finished with response size of 639 bytes
[network] 2024-01-02 09:54:14 [verbose]: Performing request osu.Game.Online.Solo.SubmitSoloScoreRequest
[runtime] 2024-01-02 09:54:14 [verbose]: Score submission completed! (token:36780 id:20247)
[network] 2024-01-02 09:54:14 [verbose]: Request to https://dev.ppy.sh/api/v2/beatmaps/869310/solo/scores/36780 successfully completed!
[network] 2024-01-02 09:54:14 [verbose]: SubmitSoloScoreRequest finished with response size of 639 bytes
[runtime] 2024-01-02 09:54:14 [error]: An unhandled error has occurred.
[runtime] 2024-01-02 09:54:14 [error]: System.InvalidOperationException: An attempt was made to transition a task to a final state when it had already completed.
[runtime] 2024-01-02 09:54:14 [error]: at osu.Game.Screens.Play.SubmittingPlayer.<>c__DisplayClass30_0.<submitScore>b__0(MultiplayerScore s) in /home/dachb/Documents/opensource/osu/osu.Game/Screens/Play/SubmittingPlayer.cs:line 250
The intention of the submission logic was to only ever create one
`scoreSubmissionSource`, and then reuse this one if a redundant
submission request was made. However, because of the temporal proximity
of fail and quit in this particular case, combined with the fact that
the calls to `submitScore()` are taking place on TPL threads, means that
there is a read-write data race on `scoreSubmissionSource`, wherein the
source can be actually created twice.
This leads to two concurrent score submission requests, which, upon
completion, attempt to transition only _the second_
`scoreSubmissionSource` to a final state (this is because the API
success/failure request callbacks capture `this`, i.e. the entire
`SubmittingPlayer` instance, rather than the `scoreSubmissionSource`
reference specifically).
To fix, ensure correct synchronisation on the read-write critical
section, which should prevent the `scoreSubmissionSource` from being
created multiple times.
When a `SubmittingPlayer` gameplay session ends with the successful
completion of a beatmap, `PrepareScoreForResultsAsync()` ensures that
the score submission request is sent to and responded to by osu-web
before calling `ISpectatorClient.EndPlaying()`.
While previously this was mostly an implementation detail, this becomes
important when considering that more and more server-side flows (replay
upload, notifying about score processing completion) hook into
`EndPlaying()`, and assume that by the point that message arrives at
osu-spectator-server, the score has already been submitted and has been
assigned a score ID that corresponds to the score submission token.
As it turns out, in the early-exit path (when the user exits the play
midway through, retries, or just fails), the same ordering guarantees
were not provided. The score's submission ran concurrently to the
spectator client `EndPlaying()` call, therefore creating a network
race. osu-server-spectator components that implciitly relied on the
ordering provided by the happy path, could therefore fail to unmap the
score submission token to a score ID.
Note that as written, the osu-server-spectator replay upload flow is
not really affected by this, as it self-corrects by essentially polling
the database and trying to unmap the score submission token to a score
ID for up to 30 seconds. However, this change would have the benefit of
reducing the polls required in such cases to just one DB retrieval.
As it turns out, in current `master`, if a gameplay session ends
normally (i.e. by the player completing the beatmap in full), then
the spectator server `EndPlaying()` method will not be called until
`SubmittingPlayer.OnExiting()`, which in practice turns out to be
the moment where the user exits from the post-gameplay results screen
back to song select.
There is seemingly no reasonable cause for not calling this earlier. In
fact the solo spectator flow looks more broken without this call than
with, because without it the spectator view just hangs until the
spectated player exits gameplay, and *only then* shows results, rather
than do it upon normal gameplay completion.
