Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure score submission completion before notifying spectator server when exiting play early #21753

Merged

Conversation

bdach
Copy link
Collaborator

@bdach bdach commented Dec 21, 2022

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():

await submitScore(score).ConfigureAwait(false);
spectatorClient.EndPlaying(GameplayState);

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 (note the lack of await):

submitScore(Score.DeepClone());
spectatorClient.EndPlaying(GameplayState);

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.

This change aims to bring both paths to parity by waiting for the score submission request even when early-exiting the player instance.


I considered adding test coverage for the ordering but it's not instantly doable. I'll do it on request if this is deemed to be the correct direction and test coverage is deemed required.

… server when exiting play early

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.
@smoogipoo smoogipoo merged commit 198567a into ppy:master Dec 22, 2022
@bdach bdach deleted the submission-early-exit-ordering-guarantees branch December 22, 2022 06:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants