Skip to content

Do not cache null values in beatmap lookup cache#37456

Merged
smoogipoo merged 1 commit intoppy:masterfrom
bdach:beatmap-cache-do-not-cache-nulls
Apr 21, 2026
Merged

Do not cache null values in beatmap lookup cache#37456
smoogipoo merged 1 commit intoppy:masterfrom
bdach:beatmap-cache-do-not-cache-nulls

Conversation

@bdach
Copy link
Copy Markdown
Collaborator

@bdach bdach commented Apr 21, 2026

RFC.

The reason I'm bringing this up is #37383.

In this case, the retrieval of the beatmap failed on a timeout:

2026-04-19 02:43:10 [verbose]: Request to https://osu.ppy.sh/api/v2/beatmaps/?ids[]=5090069 failed with System.Net.WebException: Request to https://osu.ppy.sh/api/v2/beatmaps/?ids[]=5090069 timed out after 10 seconds idle (read 0 bytes, retried 0 times)..
2026-04-19 02:43:10 [verbose]: Failing request osu.Game.Online.API.Requests.GetBeatmapsRequest (System.Net.WebException: Request to https://osu.ppy.sh/api/v2/beatmaps/?ids[]=5090069 timed out after 10 seconds idle (read 0 bytes, retried 0 times).)
2026-04-19 02:43:10 [error]: Failed to load beatmap 5090069 for playlistItem 0.
2026-04-19 02:43:10 [verbose]: ⚠️ Failed to load beatmap 5090069 for playlistItem 0.

This fails at

if (beatmap == null)
{
Logger.Log($"Failed to load beatmap {playlistItem.BeatmapID} for playlistItem {playlistItem.ID}.", level: LogLevel.Error);
return;
}

Just this failing wouldn't cause the game to soft-lock; when I suppress the lookup locally, the game continues as normal. However, suppressing the lookup is not the same as the lookup failing, because a failed lookup will write a null to the cache, which means that when a beatmap download is initiated later in

// In a perfect world we'd use BeatmapAvailability, but there's no event-driven flow for when a selection changes.
// ie. if selection changes from "not downloaded" to another "not downloaded" we wouldn't get a value changed raised.
beatmapLookupCache
.GetBeatmapAsync(item.BeatmapID, (downloadCheckCancellation = new CancellationTokenSource()).Token)
.ContinueWith(resolved => Schedule(() =>
{
APIBeatmapSet? beatmapSet = resolved.GetResultSafely()?.BeatmapSet;
if (beatmapSet == null)
return;
beatmapDownloader.Download(beatmapSet, config.Get<bool>(OsuSetting.PreferNoVideo));
}));

it'll just do nothing and the user will be stuck, even though they could very well attempt to download the beatmap here if the lookup were to succeed on the second go.

To a degree changing the whole cache for this could be viewed as the tail wagging the dog, but I think in general caching nulls here seems pretty anti-user. From within the client I would generally not expect very many complete misses when looking up beatmaps.

Note that this wouldn't soft-lock the match anymore since ppy/osu-server-spectator#471. It's just another step in making degradation less impactful.

RFC.

The reason I'm bringing this up is
ppy#37383.

In this case, the retrieval of the beatmap failed on a timeout:

```
2026-04-19 02:43:10 [verbose]: Request to https://osu.ppy.sh/api/v2/beatmaps/?ids[]=5090069 failed with System.Net.WebException: Request to https://osu.ppy.sh/api/v2/beatmaps/?ids[]=5090069 timed out after 10 seconds idle (read 0 bytes, retried 0 times)..
2026-04-19 02:43:10 [verbose]: Failing request osu.Game.Online.API.Requests.GetBeatmapsRequest (System.Net.WebException: Request to https://osu.ppy.sh/api/v2/beatmaps/?ids[]=5090069 timed out after 10 seconds idle (read 0 bytes, retried 0 times).)
```

```
2026-04-19 02:43:10 [error]: Failed to load beatmap 5090069 for playlistItem 0.
2026-04-19 02:43:10 [verbose]: ⚠️ Failed to load beatmap 5090069 for playlistItem 0.
```

This fails at

https://github.com/ppy/osu/blob/0e9664bfdfa69b4b26ff9cf84615c4b83a195a0e/osu.Game/Screens/OnlinePlay/Matchmaking/RankedPlay/Card/RankedPlayCard.cs#L167-L171

Just this failing wouldn't cause the game to soft-lock; when I suppress
the lookup locally, the game continues as normal. *However*, suppressing
the lookup is not the same as the lookup *failing*, because a failed
lookup will write a null to the cache, which means that when a beatmap
download is initiated later in

https://github.com/ppy/osu/blob/1b488949e13568c23faa0d88b18c8036f4a7dbc8/osu.Game/Screens/OnlinePlay/Matchmaking/Match/ScreenMatchmaking.cs#L308-L320

it'll just do nothing and the user will be stuck, even though they could
very well attempt to download the beatmap here if the lookup were to
succeed on the second go.

To a degree changing the whole cache for this could be viewed as the
tail wagging the dog, but I think in general caching nulls here seems
pretty anti-user. From within the client I would generally not expect
very many complete misses when looking up beatmaps.

Note that this wouldn't soft-lock the match anymore since
ppy/osu-server-spectator#471. It's just another
step in making degradation less impactful.
@bdach bdach self-assigned this Apr 21, 2026
@bdach bdach added the area:online functionality Deals with online fetching / sending but don't change much on a surface UI level. label Apr 21, 2026
@bdach bdach moved this from Inbox to Pending Review in osu! team task tracker Apr 21, 2026
@smoogipoo smoogipoo merged commit 553c203 into ppy:master Apr 21, 2026
7 of 9 checks passed
@github-project-automation github-project-automation Bot moved this from Pending Review to Done in osu! team task tracker Apr 21, 2026
@bdach bdach deleted the beatmap-cache-do-not-cache-nulls branch April 21, 2026 13:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:online functionality Deals with online fetching / sending but don't change much on a surface UI level. size/XS

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

2 participants