Fix intermittent beatmap recommendations test

[smoogipoo]

This has been failing for a while now and it's really annoying.

What I speculate (more on this later...) is:

1. The test asserts the correct beatmap is selected based on the game's beatmap.
2. Song select does not finish loading prior to either of the two beatmaps being presented - the global beatmap/ruleset bindable being changed by OsuGame.PresentBeatmap().
3. After the second beatmap is presented, the carousel finishes loading and triggers carouselBeatmapsLoaded() and somehow loads the first presented beatmap.
4. Sometimes the test passes because song select loads in time. Sometimes the test passes because song select never finishes loading.

Here's some annotated logs from https://github.com/ppy/osu/actions/runs/11699524005/job/32581685084?pr=30525:

[verbose]: 🔸 Step #8 all sets imported

----- Presenting first beatmap: 89457bfa (osu!).

[verbose]: 🔸 Step #9 present beatmap
[verbose]: Beginning PresentBeatmap with beatmap Some Artist 3 - Some Song (set id 7527) 89457bfa-d0ef-4a31-8194-c1b8b7945cca (Some Guy 5)
[verbose]: MainMenu completing PresentBeatmap with beatmap Some Artist 3 - Some Song (set id 7527) 89457bfa-d0ef-4a31-8194-c1b8b7945cca (Some Guy 5) [SR1] ruleset osu!
[verbose]: Game-wide working beatmap updated to Some Artist 3 - Some Song (set id 7527) 89457bfa-d0ef-4a31-8194-c1b8b7945cca (Some Guy 5) [SR1]
[verbose]: 📺 OsuScreenStack#303(depth:3) suspended MainMenu#510 (waiting on PlaySongSelect#553)
[verbose]: 📺 OsuScreenStack#303(depth:4) loading PlaySongSelect#553
[verbose]: decoupled ruleset transferred ("" -> "osu!")

----- Song select is now loading and has transferred the ruleset (happens during its BDL).

[verbose]: 🔸 Step #10 wait for song select

----- Even though we've "waited for song select", it is still not loaded.

[verbose]: 🔸 Step #11 recommended beatmap displayed
[verbose]: 📺 OsuScreenStack#303(depth:4) entered PlaySongSelect#553

----- First assertion passes, song select is loaded, but the carousel has not finished loading yet.
----- Presenting second beatmap: 13b38cfd (osu!catch).

[verbose]: 🔸 Step #12 present beatmap
[verbose]: Beginning PresentBeatmap with beatmap Some Artist 4 - Some Song (set id 7528) 13b38cfd-12bf-47c9-aeaa-0998aff2e4bf (Some Guy 7)
[verbose]: Game-wide working beatmap updated to Some Artist 4 - Some Song (set id 7528) 13b38cfd-12bf-47c9-aeaa-0998aff2e4bf (Some Guy 7) [SR2]
[verbose]: Completing PresentBeatmap with beatmap Some Artist 4 - Some Song (set id 7528) 13b38cfd-12bf-47c9-aeaa-0998aff2e4bf (Some Guy 7) ruleset osu!catch

----- Weirdly, it manages to transfer the ruleset here.
      I assume this is happening via LogoArriving(). 

[verbose]: decoupled ruleset transferred ("osu!" -> "osu!catch")
[verbose]: 🔸 Step #13 wait for song select

----- Carousel now finishes loading and selects a random beatmap via carouselBeatmapsLoaded().
      This ends up in 89457bfa (osu!).

[verbose]: Song select updating selection with beatmap: Some Artist 3 - Some Song (set id 7527) 89457bfa-d0ef-4a31-8194-c1b8b7945cca (Some Guy 5) [SR1] 027a2e93-5a40-47b3-924d-3df2082948be ruleset:fruits
[verbose]: Song select changing beatmap from "Some Artist 4 - Some Song (set id 7528) 13b38cfd-12bf-47c9-aeaa-0998aff2e4bf (Some Guy 7) [SR2]" to "Some Artist 3 - Some Song (set id 7527) 89457bfa-d0ef-4a31-8194-c1b8b7945cca (Some Guy 5) [SR1]"
[verbose]: Game-wide working beatmap updated to Some Artist 3 - Some Song (set id 7527) 89457bfa-d0ef-4a31-8194-c1b8b7945cca (Some Guy 5) [SR1]

----- Boom.

[verbose]: 🔸 Step #14 recommended beatmap displayed
[verbose]: 💥 Failed (on attempt 1,661)

I don't know how to write a practical test nor can I even test my solution, but here's how I've reproduced it (run only the single test):

diff --git a/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapRecommendations.cs b/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapRecommendations.cs
index 16c8bc1a6b..fa20b30559 100644
--- a/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapRecommendations.cs
+++ b/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapRecommendations.cs
@@ -10,6 +10,7 @@
 using NUnit.Framework;
 using osu.Framework.Allocation;
 using osu.Framework.Extensions.IEnumerableExtensions;
+using osu.Framework.Graphics;
 using osu.Framework.Testing;
 using osu.Game.Beatmaps;
 using osu.Game.Extensions;
@@ -18,6 +19,7 @@
 using osu.Game.Rulesets.Mania;
 using osu.Game.Rulesets.Osu;
 using osu.Game.Rulesets.Taiko;
+using osu.Game.Screens.Select;
 using osu.Game.Tests.Resources;
 using osu.Game.Users;
 
@@ -127,6 +129,7 @@ public void TestBestRulesetIsRecommended()
             presentAndConfirm(() => mixedSet, 3);
         }
 
+        [Solo]
         [Test]
         public void TestSecondBestRulesetIsRecommended()
         {
@@ -141,7 +144,7 @@ public void TestSecondBestRulesetIsRecommended()
             presentAndConfirm(() => osuSet, 1);
 
             // Present mixed difficulty set, expect ruleset with second highest star difficulty
-            presentAndConfirm(() => mixedSet, 2);
+            presentAndConfirm(() => mixedSet, 2, true);
         }
 
         [Test]
@@ -187,11 +190,20 @@ private BeatmapSetInfo importBeatmapSet(IEnumerable<RulesetInfo> difficultyRules
 
         private bool ensureAllBeatmapSetsImported(IEnumerable<BeatmapSetInfo> beatmapSets) => beatmapSets.All(set => set != null);
 
-        private void presentAndConfirm(Func<BeatmapSetInfo> getImport, int expectedDiff)
+        private void presentAndConfirm(Func<BeatmapSetInfo> getImport, int expectedDiff, bool broken = false)
         {
             AddStep("present beatmap", () => Game.PresentBeatmap(getImport()));
 
-            AddUntilStep("wait for song select", () => Game.ScreenStack.CurrentScreen is Screens.Select.SongSelect);
+            if (broken)
+            {
+                AddStep("allow load", () => BeatmapCarousel.AllowLoad.Set());
+                AddUntilStep("wait for song select",
+                    () => Game.ScreenStack.CurrentScreen is Screens.Select.SongSelect ss
+                          && ss.ChildrenOfType<BeatmapCarousel>().SingleOrDefault()?.IsLoaded == true);
+            }
+            else
+                AddUntilStep("wait for song select", () => Game.ScreenStack.CurrentScreen is Screens.Select.SongSelect);
+
             AddUntilStep("recommended beatmap displayed", () => Game.Beatmap.Value.BeatmapInfo.MatchesOnlineID(getImport().Beatmaps[expectedDiff - 1]));
         }
     }
diff --git a/osu.Game/Screens/Select/BeatmapCarousel.cs b/osu.Game/Screens/Select/BeatmapCarousel.cs
index 44f91b4df1..c949a5ec29 100644
--- a/osu.Game/Screens/Select/BeatmapCarousel.cs
+++ b/osu.Game/Screens/Select/BeatmapCarousel.cs
@@ -242,9 +242,15 @@ public BeatmapCarousel(FilterCriteria initialCriterial)
             activeCriteria = initialCriterial;
         }
 
+        public static ManualResetEventSlim AllowLoad = new ManualResetEventSlim();
+
         [BackgroundDependencyLoader]
         private void load(OsuConfigManager config, AudioManager audio, CancellationToken? cancellationToken)
         {
+#pragma warning disable RS0030
+            AllowLoad.Wait();
+#pragma warning restore RS0030
+
             spinSample = audio.Samples.Get("SongSelect/random-spin");
             randomSelectSample = audio.Samples.Get(@"SongSelect/select-random");
 

Could this happen in practice? Maybe? I'm not sure what the resolution is - would likely need @peppy to take over.

[bdach]

CI seems to say "no" to the test being fixed, seemingly. Albeit it's only TestCorrectStarRatingIsUsed failing this time from a sample size of 1.

[smoogipoo]

The irony is not lost on me re: new CI failures. Can repro locally at least.

[smoogipoo]

Turns out that the tests actually all did not actually work correctly (only work by chance), because the beatmap is processed immediately upon import which in this case resets all star ratings to 0 (there's no hitobjects). I'm not sure why the Windows runs passed - likely pure luck.

Hopefully the path I've taken here of extracting BeatmapUpdater to an interface is deemed appropriate.

Breadcrumbs:

osu/osu.Game/Beatmaps/BeatmapImporter.cs

Lines 218 to 230 in 35397d9

	protected override void PostImport(BeatmapSetInfo model, Realm realm, ImportParameters parameters)
	{
	base.PostImport(model, realm, parameters);
	// Scores are stored separately from beatmaps, and persist even when a beatmap is modified or deleted.
	// Let's reattach any matching scores that exist in the database, based on hash.
	foreach (BeatmapInfo beatmap in model.Beatmaps)
	{
	beatmap.UpdateLocalScores(realm);
	}
	ProcessBeatmap?.Invoke(model, parameters.Batch ? MetadataLookupScope.LocalCacheFirst : MetadataLookupScope.OnlineFirst);
	}

osu/osu.Game/OsuGameBase.cs

Line 334 in 35397d9

BeatmapManager.ProcessBeatmap = (beatmapSet, scope) => beatmapUpdater.Process(beatmapSet, scope);

osu/osu.Game/Beatmaps/BeatmapUpdater.cs

Lines 58 to 86 in 35397d9

	public void Process(BeatmapSetInfo beatmapSet, MetadataLookupScope lookupScope = MetadataLookupScope.LocalCacheFirst) => beatmapSet.Realm!.Write(_ =>
	{
	// Before we use below, we want to invalidate.
	workingBeatmapCache.Invalidate(beatmapSet);
	if (lookupScope != MetadataLookupScope.None)
	metadataLookup.Update(beatmapSet, lookupScope == MetadataLookupScope.OnlineFirst);
	foreach (var beatmap in beatmapSet.Beatmaps)
	{
	difficultyCache.Invalidate(beatmap);
	var working = workingBeatmapCache.GetWorkingBeatmap(beatmap);
	var ruleset = working.BeatmapInfo.Ruleset.CreateInstance();
	Debug.Assert(ruleset != null);
	var calculator = ruleset.CreateDifficultyCalculator(working);
	beatmap.StarRating = calculator.Calculate().StarRating;
	beatmap.Length = working.Beatmap.CalculatePlayableLength();
	beatmap.BPM = 60000 / working.Beatmap.GetMostCommonBeatLength();
	beatmap.EndTimeObjectCount = working.Beatmap.HitObjects.Count(h => h is IHasDuration);
	beatmap.TotalObjectCount = working.Beatmap.HitObjects.Count;
	}
	// And invalidate again afterwards as re-fetching the most up-to-date database metadata will be required.
	workingBeatmapCache.Invalidate(beatmapSet);
	});