`SubmittingPlayer` no longer guarantees calling `EndPlaying` before exit

frenzibyte commented 1 year ago

Type

Crash to desktop

Bug description

Coming from recurring test failures. Since https://github.com/ppy/osu/pull/21753, gameplay screens no longer guarantee calling EndPlaying before exit, as it now happens under a fired-and-forgotten async task. This means that if submission took long and a user decided to play again, then the user might crash to desktop due to gameplay calling BeginPlaying when EndPlaying hasn't been called yet pending previous score submission.

I plan to push a workaround for tests, but that aside, I'm opening this issue thread (in addition to discord) to further discuss this matter. While it's rare to encounter this sort of edge case, it's best to leave an issue thread open for tracking at least. We may want to consider allowing out-of-order BeginPlaying/EndPlaying calls, either by chaining them at a SpectatorClient level, or let the server handle it properly.

cc/ @ppy/area-client

Screenshots or videos

No response

Version

8f704249994fbc223fb5811f3cfdd0c7f9093237

Logs

-

peppy commented 1 year ago

Keeping it simple, my assumptions would be:

As soon as exiting the player, EndPlaying should be called. This should always be before the next BeginPlaying call. That should be the only requirement of Player.

Internally, SpectatorClient should ensure that all requests are sent to the server in the correct order. Previous play frame bundled -> end -> begin. Any kind of delivery reliability should also be ensured by that component.

So I'm not sure what you mean by "allowing out-of-order", because that doesn't make much sense. Is the main issue here not that TestPlayer is calling this on disposal which is not guaranteed to fire early enough? Can't that just be fixed?

bdach commented 1 year ago

Server flows (replay upload less so, score processing notification delivery more so) are relying on score submission to complete before notifying osu-server-spectator to unmap submission token to score ID. I don't know how to ensure correct delivery order given that.

If there was a higher-level component coordinating/queueing both submission and notifying spectator in the background, then I could see this working. But I'm not sure we want to go there.

frenzibyte commented 1 year ago

So I'm not sure what you mean by "allowing out-of-order", because that doesn't make much sense. Is the main issue here not that TestPlayer is calling this on disposal which is not guaranteed to fire early enough? Can't that just be fixed?

For screen navigation tests, they rely on a full game instance which means running on SoloPlayer instead of TestPlayer, so we have to manually handle that. But for other tests, yeah, disposal is async so it can't be guaranteed (both of which should be fixed in #22221).

Generally the logic in TestPlayer was handling the case a screen gets disposed without exiting it, meanwhile the problem here is that even if OnExiting is called, EndPlaying will still not be guaranteed.

peppy commented 1 year ago

Server flows (replay upload less so, score processing notification delivery more so) are relying on score submission to complete before notifying osu-server-spectator to unmap submission token to score ID. I don't know how to ensure correct delivery order given that.

If there was a higher-level component coordinating/queueing both submission and notifying spectator in the background, then I could see this working. But I'm not sure we want to go there.

I think this needs to be simplified and handled server side. We can't have the client responsible for order of delivery of events to the server. Even if the client sends in the correct order, they could arrive out of order due to different routes or packet loss. I'd need to go through exactly what is happening currently to assess how wrong it is, but ultimately we should either aim for

The client to send completion to both server components in any order and have it correctly handled server side (I believe this may be what @smoogipoo and I discussed last time this came up)
The client to only send completion to one server component, which should handle any further communications with other server-side components itself.

bdach commented 1 year ago

Even if the client sends in the correct order, they could arrive out of order due to different routes or packet loss

I'll just note that as far as I'm aware, the client will currently synchronously wait for a response to the submission request before proceeding to call EndPlaying(). I'm not sure how these can be reordered in such a case; the flow is synchronous. The only thing that could break down is a failure to deliver the response, but then client will timeout the submission request and call EndPlaying() after that. That was the main point of https://github.com/ppy/osu/pull/21753; if it is the wrong direction, then I was not aware of it.

The client to only send completion to one server component, which should handle any further communications with other server-side components itself.

This would be nice, but I don't know how that plays with the responsibilities of osu-server-spectator. I guess it's already a grab-bag of everything right now, but it will become even more so if it is to coordinate submission too.

smoogipoo commented 1 year ago

As soon as exiting the player, EndPlaying should be called

It should really be "as soon as gameplay is finished". I can't remember why it doesn't already do that.

frenzibyte commented 1 year ago

That's because it has changed since #21753, with clarification in the PR's description along with https://github.com/ppy/osu/issues/22220#issuecomment-1383148322.

smoogipoo commented 1 year ago

That PR didn't change that, it just made it so that the calls to the osu-web and osu-server-spectator APIs are run one after another.

That being said, there is something I previously wanted to mention in this issue that I forgot about, and that is this:

Server flows (replay upload less so, score processing notification delivery more so) are relying on score submission to complete before notifying osu-server-spectator to unmap submission token to score ID. I don't know how to ensure correct delivery order given that.

Can you expand on this a bit more? I had similar concerns initially regarding the ordering, but I'm not sure it's strictly required. Can you point to the code which unmaps the submission token? I implemented the replay uploader at least with the intent that it would wait for the score ID to exist by periodically querying the tokens table, but I don't remember any unmapping going on.

bdach commented 1 year ago

Can you expand on this a bit more? I had similar concerns initially regarding the ordering, but I'm not sure it's strictly required. Can you point to the code which unmaps the submission token?

I'm referring to these two calls specifically.

Granted, as you say, the score uploader does not need the token to be unmappable to a score ID at that particular instant, as it will indeed poll for the score ID for up to 30 seconds. This was also mentioned in https://github.com/ppy/osu/pull/21753; the ordering guarantee is a nice addition there, but not a necessary one.

The ScoreProcessedSubscriber call, however, is not resilient against this and does no retry, so it's a lot more dependent on this particular ordering. I could have made it wait, but after going through the trouble of not polling for the process status of the score via redis pubsub, it seemed like a huge waste of that effort to have to poll for the score ID anyways, which the submission-before-EndPlaying ordering guarantee helped me avoid.

ppy / osu