Implement creating rooms for queued beatmaps for "daily challenge" system #11204

bdach · 2024-05-09T14:45:02Z

RFC. Opening this for early feedback because I want to continue building on this going forward (and because this is likely to go through a few review rounds because I suck at php).

This implements the queueing mechanism described in point (1) of the execution plan of the design document for the new "daily challenge" system. The queueing is based around a new database table:

MySQL [osu]> describe daily_challenge_queue;
+---------------------+--------------------+------+-----+---------+----------------+
| Field               | Type               | Null | Key | Default | Extra          |
+---------------------+--------------------+------+-----+---------+----------------+
| id                  | bigint unsigned    | NO   | PRI | NULL    | auto_increment |
| beatmap_id          | mediumint unsigned | NO   |     | NULL    |                |
| ruleset_id          | smallint unsigned  | NO   |     | NULL    |                |
| allowed_mods        | json               | YES  |     | NULL    |                |
| required_mods       | json               | YES  |     | NULL    |                |
| order               | smallint unsigned  | YES  |     | NULL    |                |
| multiplayer_room_id | bigint unsigned    | YES  |     | NULL    |                |
+---------------------+--------------------+------+-----+---------+----------------+

The table is designed to serve the MVP first and foremost (beatmap_id and ruleset_id), but also has some future-proofing built in (allowed and required_mods, also order for being able to e.g. swap the items if need be).

Items will be added to this table externally (most likely via ASS, cc @peppy for confirm). There is no selection automation in place.

This PR introduces a new artisan command - daily-challenge:queue-next - which takes the item with the lowest outstanding order (or id, if tied), and creates a new playlists room based on the parameters stored in the queue (beatmap id / ruleset id / mods). The playlist is assumed to run until midnight of the current day.

This is not hooked up to cron yet, because the full flow isn't yet ready. I'll PR a change adding it to cron when the dust has settled on all parts and we have a flow working end-to-end, between web, spectator server, and client.

Possible contention points

Naming - willing to listen to suggestions.

Creating a room requires a user because both the room itself and the playlist items are presumed to be owned by a user. In this case I used BanchoBot, or user w/ ID $GLOBALS['cfg']['osu']['legacy']['bancho_bot_user_id'], as a replacement for a user.

This is especially a bit dodgy due to restrictions placed on users with respect to the number of allowable rooms open at a time:

osu-web/app/Models/Multiplayer/Room.php

Lines 644 to 646 in 8a37441

    
           if ($query->count() >= $max) { 
        
               throw new InvariantException('number of simultaneously active rooms reached'); 
        
           }

That is why I wrote the command to die if another "daily challenge" room was already open when it was ran, so that check above won't be hit in practice, but yeah. I briefly considered adding a carve-out for the check for BanchoBot specifically but that felt really dodgy so I didn't.

I imagine this may be considered suboptimal, especially given the legacy name in that key lookup there. We could probably try to decouple the multiplayer room model from always needing to have a user although that may be a operation that requires care to a degree that I'm not necessarily sure I'm able to perform it confidently.

Structurally this reuses the whole $room->startGame() logic even though many validation checks are technically superfluous in there. It feels alright to me but it could be argued that this is a server-side flow and thus one might want to refactor that startPlay() method into smaller chunks and reuse those. Dunno.
The created room uses a new category - daily_challenge. I'm pretty sure this will be needed, at least for client-side filtering (because we won't want to show these in open room listings, just directly from main menu), but if not, then we can probably talk about alternatives.

One thing that I already know is that this new category as added here will break old lazer clients due to serialisation woes (old versions won't know the new enum value). It is unfortunately too late to do anything about this, we'll just need to tell users to upgrade if this sticks.
af761d5 is a notable outlier in being a functional change to existing code rather than something completely new. It happened because I was having an issue when writing tests that rooms were getting out by the active() scope up to a second after having been opened. I'm not sure why that is - guessing it may be time comparison precision being set at 1 second or something - but it appears to me that a weak inequality doesn't really make sense there anyhow so I just made the change inline.

…reation Attempting to write tests for the command that creates 'beatmap of the day' multiplayer rooms was stymied by the fact that immediately weak inequality here was causing the room to not become 'active' up to a second after it was created.

cl8n · 2024-05-10T05:51:27Z

BeatmapOfTheDayQueueItem having most of the data of PlaylistItem makes me feel like it would be more straightforward to manage as orphaned playlist items (with room ID 0 or something), without a new table. the whole purpose of this queue is to eventually translate into playlist items anyway, right?

or even creating the entire room in advance to act as the "queue" (with starts_at set to the future)

bdach · 2024-05-10T06:15:51Z

BeatmapOfTheDayQueueItem having most of the data of PlaylistItem makes me feel like it would be more straightforward to manage as orphaned playlist items (with room ID 0 or something), without a new table

I really don't like this idea in general if I'm honest. It feels sloppy - I've really been historically bitten way too much by "special" in-band values like zero IDs having some special meaning like this, which then leads to pitfalls when somebody tries to follow them like a normal foreign key. It also closes off potential avenues of extension. For instance, imagine if this takes and a few months down the line we want to add something like collective goals attached to the beatmap of the day, like getting however many A ranks across the playerbase. At that point I dunno where you put this information if not into this table - some new table?

Although you could arguably say that if that is the eventual direction then the queue suffix needs to be dropped from the table. Which I guess is easy enough if we decide on that.

nanaya · 2024-05-10T07:58:36Z

Also mentioned above, how about creating the room with starts_at set to the future? Whatever the custom settings it'll have, I imagine it'll be linked to the room (or the respective playlist items) anyway.

bdach · 2024-05-10T08:43:14Z

Room with a future date sounds much cleaner from a design angle (no more weird detached playlist items whose existence must be kept in mind at all times), but also it implies that whatever will be responsible for queueing the beatmaps of the day will have to reimplement the logic responsible for creating the actual room, e.g. at least a decent subset of $room->startGame(). And there are ruleset/mod validity checks there, there is chat channel creation for the playlist, etc.

I dunno. I'm most likely not going to be the one implementing the enqueueing of items so from my perspective I can't pretend I can push back too hard on that but it does appear to me that if ASS is to create a full playlists room by itself it may become redundant (with respect to code duplication) and/or inconvenient. Unless ASS is going to just send a special api request to web for this or something, but at that point I don't know if things are that much simpler.

nanaya · 2024-05-10T09:08:03Z

Or can just implement an interop endpoint which accepts some set of parameters and then create the room...

bdach · 2024-05-10T09:11:29Z

Yep that would also work. Really need a shot-call from @peppy at this point to commit to either direction because I can't see into ASS source and don't have much of an insight as to what would be easiest/best here.

peppy · 2024-05-10T09:20:48Z

Hmm... if I was implementing this I too would have gone for a separate table as done here, just to implement cleanly and in an isolated manner. We've learnt the hard way from trying to make one model work for too many different usages, and that feels like it could be the case here.

As for whether we use a future starts_at, I guess it depends on whether we want the complexity at the queueing mechanism or the storage. I'm weakly leaning towards how it's done here.

I would see ASS being a very simple "queue more beatmap / mod criteria to queue table" and osu-web handling the actual room creation. If this was to change, it would be hard to orchestrate checks such as "queue is running low!" being broadcast to discord or otherwise, as ASS doesn't have any kind of cron concept.

I guess osu-web could still do the creation ahead of time and use a future starts_at if that makes timings better though? It would be a good tool to have in our arsenal to be able to schedule future rooms.

nanaya · 2024-05-10T09:28:56Z

well the table here is just an interim table before eventually transformed into rooms and playlists. Having an interop which creates the room immediately skips that additional step.

If the actual system doesn't use room/playlist at all then that'd make more sense I guess (although probably will come with its own complication).

bdach · 2024-05-10T09:44:10Z

I guess osu-web could still do the creation ahead of time and use a future starts_at if that makes timings better though? It would be a good tool to have in our arsenal to be able to schedule future rooms.

With a separate table scheduling future rooms is finicky (it'd be doable manually if anything). A remote procedure call would be more suited to scheduling future rooms because web then knows when a queue item is added; otherwise it would have to poll the queue on an interval I suppose. Or manual dispatch (which is doable but seems shoddy/counterproductive)

well the table here is just an interim table before eventually transformed into rooms and playlists. Having an interop which creates the room immediately skips that additional step.

It is that, yeah, but one other argument that I'll drop here in favour of having it interim is that delaying the room creation simplifies implementation of some manual interventions if required. For instance, reordering items is just a question of updating some rows' orders in the queue, you don't have to go find all of the other scheduled rooms and reorder their dates. Similarly, dropping a queued beatmap for whatever reason is just removing an item from the interim table, while if rooms were to be queued in advance, you'd have to go find all of the subsequent rooms for subsequent days (if any) and pull them back by a day again.

Dunno it's really a bit of a "see the future" thing as usual. Difficult to say how this will pan out in reality 🤷

peppy · 2024-05-10T11:35:00Z

With a separate table scheduling future rooms is finicky

Yeah I meant more for manual stuff in the future.

Having an interop which creates the room immediately skips that additional step.

It'll get a bit annoying if we need to make changes, especially to ordering. Like rather than having a cron task that runs once a day and pulls a new room in, we have all these future rooms already lined up, which potentially need date changes, deletion etc.

bdach · 2024-05-14T10:09:27Z

Blocking temporarily pending confirmation of rename of the feature (see https://discord.com/channels/188630481301012481/188630652340404224/1239804074355068959)

bdach · 2024-05-16T09:21:45Z

Rename of feature to "daily challenge" has been applied.

If anyone other than me has ran this on their local envs, first wipe all beatmap_of_the_day category rooms (or move them elsewhere), and then run artisan migrate:rollback --step=1 before pulling, then pull, then re-apply the migration from this PR with the naming changes again. Not sure if there's a better way to do this, but also not sure anybody except me cares.

nanaya · 2024-05-17T07:41:50Z

from what I gather, there will be a few other things to be run when the new room is open. How will the schedule be coordinated? Is it checking the rooms table and see if the new room has been created? What if the creation failed due to some reason (like db error)?

an alternative would be the table to be completely managed by ass and it just hit an interop endpoint to create the room when it starts maybe?

bdach · 2024-05-17T07:52:18Z

Currently the intended flow for this to work is as such:

"Daily challenge" beatmaps are queued manually by a selected team, via internal discord / ASS commands. This translates to insertion of daily_challenge_queue rows.
The command added here will be hooked up to a cron and will pick up the queue items on a daily basis and make rooms out of them, say every day on 00:05 UTC. (The five minute gap would be to avoid various funniness around the previous room not quite completing yet or something.)
osu-server-spectator will poll the db for open daily_challenge rooms on an interval. Something like once every five minutes. Once it detects a change in the active daily challenge room, it broadcasts this info to all connected clients.

from what I gather, there will be a few other things to be run when the new room is open

Not super sure what this is referring to.

How will the schedule be coordinated?

I'm not sure what the specific question is here but for now it is best-effort, hard-coded, daily. Rooms are created daily as long as there exist any queued beatmaps for the daily challenge thing. There's no coordination in the sense of "this beatmap must be played on this specific day". At most queued items can be reordered.

What if the creation failed due to some reason (like db error)?

The presumption is that step (2) will fail and osu-server-spectator will advertise nothing until we fix the job and manually re-run it.

an alternative would be the table to be completely managed by ass and it just hit an interop endpoint to create the room when it starts maybe?

I'm not sure what else to say to that other than what has been already said above. Truthfully I'm not really even understanding why there's such apparent pushback to this existing in web, which makes it difficult for me to suggest any kind of compromise.

nanaya · 2024-05-17T07:59:00Z

Not super sure what this is referring to.

the number 3 there.

I'm not sure what else to say to that other than what has been already said above.

it'll allow all the process to be run in sync by a single process, with its own failure handling. And possibly removes the need to poll the table

bdach · 2024-05-17T08:02:38Z

And possibly removes the need to poll the table

Unless ASS was to RPC spectator server too to tell it that there is a new room (which I don't think we currently even have the capability to do, on the spectator server side), it doesn't. Unless you're thinking that the interop endpoint could issue like a notifications server message, which, again, I'm neutral on this being the case, were it not for @peppy expressing clear opposition to using the notifications server for delivery to clients for this feature (which is understandable, since expansion of this feature with future capabilities will be much easier to do in spectator server if the expansion happens).

nanaya · 2024-05-17T08:08:25Z

well, I guess this is fine as long there's someone to handle rerunning/checking osu-web job in case of failure...

nanaya · 2024-05-17T08:23:09Z

app/Console/Commands/DailyChallengeQueueNext.php

+            $ownerId = $GLOBALS['cfg']['osu']['legacy']['bancho_bot_user_id'];
+
+            $room = (new Room())->startGame(
+                User::lookup($ownerId),


use findOrFail as it should be a simple id lookup and expected to fail if for some reason missing

nanaya · 2024-05-17T08:33:14Z

app/Console/Commands/DailyChallengeQueueNext.php

+    protected $signature = 'daily-challenge:queue-next';
+
+    protected $description = 'Creates a new "daily challenge" multiplayer room for the first item in queue.';


those are supposed to be sorted alphabetically nowadays

nanaya · 2024-05-17T08:34:33Z

tests/Commands/DailyChallengeQueueNextTest.php

+    {
+        parent::tearDown();
+
+        config_set('osu.legacy.bancho_bot_user_id', $this->originalUtilityUserId);


As it turned out the config is always reset between tests

(I probably didn't check how it actually behaves before)

nanaya · 2024-05-17T08:43:58Z

app/Console/Commands/DailyChallengeQueueNext.php

+
+class DailyChallengeQueueNext extends Command
+{
+    protected $signature = 'daily-challenge:queue-next';


is it queue-next or create-next

nanaya · 2024-05-17T08:54:06Z

app/Console/Commands/DailyChallengeQueueNext.php

+            );
+            $room->update(['category' => 'daily_challenge']);
+
+            $nextQueueItem->multiplayer_room_id = $room->id;


using ->getKey() is preferred over ->id

nanaya · 2024-05-17T08:55:01Z

app/Models/Multiplayer/DailyChallengeQueueItem.php

+
+    public function multiplayerRoom()
+    {
+        return $this->hasOne(Room::class, 'id', 'multiplayer_room_id');


This should be belongsTo just like beatmap (and the extra parameters aren't needed)

nanaya · 2024-05-17T09:04:03Z

tests/Commands/DailyChallengeQueueNextTest.php

+        $this->artisan('daily-challenge:queue-next')
+            ->expectsOutputToContain('"Daily challenge" queue is empty')
+            ->assertFailed();
+        $this->assertSame(0, Room::all()->count());


use expectCountChange instead

nanaya · 2024-05-17T09:27:40Z

app/Console/Commands/DailyChallengeQueueNext.php

+
+        DB::transaction(function () use ($nextQueueItem) {
+            $startTime = today();
+            $ownerId = $GLOBALS['cfg']['osu']['legacy']['bancho_bot_user_id'];


I think it's more like host than owner

Yeah there's "room host" but also "playlist item owner". Mixed these up a bit 😅

nanaya

seems working although should be remembered that any failure (including transient errors like db temporarily blipping) on the cron job means it'll need to be manually rerun by someone who has access to osu-web console as I don't think it's got any retry mechanism

(either that or schedule the cron multiple times or something)

nanaya · 2024-05-17T13:52:27Z

oh yeah, the title should probably be adjusted because it's not queuing anything but more like creating queued room.

bdach added 5 commits May 9, 2024 11:54

Add database migration for basic "beatmap of the day" queueing system

e8955a3

Add model class for beatmap of the day queue items

7189dc7

Implement command for creating room for next queued beatmap of the day

5414fcf

Add testing for command

8127d20

bdach added the area:multiplayer label May 9, 2024

bdach added the blocked label May 14, 2024

Rename feature to "daily challenge"

7de32c8

bdach removed the blocked label May 16, 2024

bdach changed the title ~~Implement queueing for "beatmap of the day" system~~ Implement queueing for "daily challenge" system May 16, 2024

This was referenced May 17, 2024

Implement "daily challenge" system ppy/osu#28136

Open

Add client/server models & operations for "daily challenge" feature ppy/osu#28194

Merged

Announce currently active "daily challenge" playlist to clients ppy/osu-server-spectator#234

Merged

nanaya requested changes May 17, 2024

View reviewed changes

bdach self-assigned this May 17, 2024

bdach added 8 commits May 17, 2024 13:27

Use findOrFail() instead

88d709c

Apply alphabetic sort

ca08d68

Remove redundant config restore

72263fa

Rename DailyChallenge{Queue -> Create}Next

1539aee

Prefer ->getKey() to ->id

b6f7258

Fix & simplify relation definition

d2b62f1

Use expectCountChange() instead

40cdeb5

Rename variable

4ea720b

nanaya approved these changes May 17, 2024

View reviewed changes

bdach changed the title ~~Implement queueing for "daily challenge" system~~ Implement creating rooms for queued beatmaps for "daily challenge" system May 17, 2024

peppy self-requested a review May 22, 2024 05:12

peppy approved these changes May 22, 2024

View reviewed changes

peppy merged commit 7031cbf into ppy:master May 22, 2024
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement creating rooms for queued beatmaps for "daily challenge" system #11204

Implement creating rooms for queued beatmaps for "daily challenge" system #11204

bdach commented May 9, 2024 •

edited

cl8n commented May 10, 2024

bdach commented May 10, 2024

nanaya commented May 10, 2024

bdach commented May 10, 2024 •

edited

nanaya commented May 10, 2024

bdach commented May 10, 2024

peppy commented May 10, 2024

nanaya commented May 10, 2024 •

edited

bdach commented May 10, 2024 •

edited

peppy commented May 10, 2024

bdach commented May 14, 2024

bdach commented May 16, 2024

nanaya commented May 17, 2024 •

edited

bdach commented May 17, 2024 •

edited

nanaya commented May 17, 2024

bdach commented May 17, 2024 •

edited

nanaya commented May 17, 2024 •

edited

nanaya May 17, 2024

nanaya May 17, 2024

nanaya May 17, 2024

nanaya May 17, 2024

nanaya May 17, 2024

nanaya May 17, 2024

nanaya May 17, 2024

nanaya May 17, 2024

bdach May 17, 2024

nanaya left a comment

nanaya commented May 17, 2024

	if ($query->count() >= $max) {
	throw new InvariantException('number of simultaneously active rooms reached');
	}

		protected $signature = 'daily-challenge:queue-next';

		protected $description = 'Creates a new "daily challenge" multiplayer room for the first item in queue.';

Implement creating rooms for queued beatmaps for "daily challenge" system #11204

Implement creating rooms for queued beatmaps for "daily challenge" system #11204

Conversation

bdach commented May 9, 2024 • edited

Possible contention points

cl8n commented May 10, 2024

bdach commented May 10, 2024

nanaya commented May 10, 2024

bdach commented May 10, 2024 • edited

nanaya commented May 10, 2024

bdach commented May 10, 2024

peppy commented May 10, 2024

nanaya commented May 10, 2024 • edited

bdach commented May 10, 2024 • edited

peppy commented May 10, 2024

bdach commented May 14, 2024

bdach commented May 16, 2024

nanaya commented May 17, 2024 • edited

bdach commented May 17, 2024 • edited

nanaya commented May 17, 2024

bdach commented May 17, 2024 • edited

nanaya commented May 17, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nanaya left a comment

Choose a reason for hiding this comment

nanaya commented May 17, 2024

bdach commented May 9, 2024 •

edited

bdach commented May 10, 2024 •

edited

nanaya commented May 10, 2024 •

edited

bdach commented May 10, 2024 •

edited

nanaya commented May 17, 2024 •

edited

bdach commented May 17, 2024 •

edited

bdach commented May 17, 2024 •

edited

nanaya commented May 17, 2024 •

edited