From b1787cbc36d44d7feccd7e9adfadf739a56cc190 Mon Sep 17 00:00:00 2001 From: Shadowghost Date: Sun, 27 Sep 2026 16:30:42 -0400 Subject: Backport pull request #18095 from jellyfin/release-12.z Retry MusicBrainz requests when the server is busy Original-merge: 1478011dfc67c46d52d22b89a49e4519ec42bdb8 Merged-by: crobibero Backported-by: Cody Robibero --- .../MusicBrainz/MusicBrainzAlbumProvider.cs | 8 +- .../MusicBrainz/MusicBrainzArtistProvider.cs | 4 +- .../MusicBrainz/MusicBrainzQueryExtensions.cs | 160 +++++++++++++++++++-- .../Music/MusicBrainzQueryExtensionsTests.cs | 136 ++++++++++++++++++ 4 files changed, 294 insertions(+), 14 deletions(-) create mode 100644 tests/Jellyfin.Providers.Tests/Music/MusicBrainzQueryExtensionsTests.cs diff --git a/MediaBrowser.Providers/Plugins/MusicBrainz/MusicBrainzAlbumProvider.cs b/MediaBrowser.Providers/Plugins/MusicBrainz/MusicBrainzAlbumProvider.cs index 7dee5fd31d..55913aa3fc 100644 --- a/MediaBrowser.Providers/Plugins/MusicBrainz/MusicBrainzAlbumProvider.cs +++ b/MediaBrowser.Providers/Plugins/MusicBrainz/MusicBrainzAlbumProvider.cs @@ -70,7 +70,7 @@ public class MusicBrainzAlbumProvider : IRemoteMetadataProvider 0) @@ -83,7 +83,7 @@ public class MusicBrainzAlbumProvider : IRemoteMetadataProvider 0) @@ -200,13 +200,13 @@ public class MusicBrainzAlbumProvider : IRemoteMetadataProvider 0 ? releaseSearchResults.Results[0].Item : null; } else if (!string.IsNullOrEmpty(info.GetAlbumArtist())) { - var releaseSearchResults = await query.FindReleasesAsync($"\"{info.Name}\" AND artist:{info.GetAlbumArtist()}", null, null, false, cancellationToken) + var releaseSearchResults = await query.FindReleasesWithRetryAsync($"\"{info.Name}\" AND artist:{info.GetAlbumArtist()}", _logger, cancellationToken) .ConfigureAwait(false); releaseResult = releaseSearchResults.Results.Count > 0 ? releaseSearchResults.Results[0].Item : null; } diff --git a/MediaBrowser.Providers/Plugins/MusicBrainz/MusicBrainzArtistProvider.cs b/MediaBrowser.Providers/Plugins/MusicBrainz/MusicBrainzArtistProvider.cs index c3d13ed42c..eb189f1258 100644 --- a/MediaBrowser.Providers/Plugins/MusicBrainz/MusicBrainzArtistProvider.cs +++ b/MediaBrowser.Providers/Plugins/MusicBrainz/MusicBrainzArtistProvider.cs @@ -60,7 +60,7 @@ public class MusicBrainzArtistProvider : IRemoteMetadataProvider 0) { @@ -70,7 +70,7 @@ public class MusicBrainzArtistProvider : IRemoteMetadataProvider 0) { diff --git a/MediaBrowser.Providers/Plugins/MusicBrainz/MusicBrainzQueryExtensions.cs b/MediaBrowser.Providers/Plugins/MusicBrainz/MusicBrainzQueryExtensions.cs index f3df41e942..620a0b3c45 100644 --- a/MediaBrowser.Providers/Plugins/MusicBrainz/MusicBrainzQueryExtensions.cs +++ b/MediaBrowser.Providers/Plugins/MusicBrainz/MusicBrainzQueryExtensions.cs @@ -5,6 +5,7 @@ using System.Threading.Tasks; using MetaBrainz.Common; using MetaBrainz.MusicBrainz; using MetaBrainz.MusicBrainz.Interfaces.Entities; +using MetaBrainz.MusicBrainz.Interfaces.Searches; using Microsoft.Extensions.Logging; namespace MediaBrowser.Providers.Plugins.MusicBrainz; @@ -14,6 +15,15 @@ namespace MediaBrowser.Providers.Plugins.MusicBrainz; /// internal static class MusicBrainzQueryExtensions { + /// + /// The number of extra attempts made when MusicBrainz reports a transient failure. + /// + private const int MaxRetries = 2; + + private static readonly TimeSpan _minimumRetryDelay = TimeSpan.FromSeconds(1); + + private static readonly TimeSpan _maximumRetryDelay = TimeSpan.FromSeconds(15); + /// /// Parses a MusicBrainz identifier, which may come from user-supplied tags or NFO files and is therefore not /// guaranteed to be a valid GUID. @@ -49,10 +59,11 @@ internal static class MusicBrainzQueryExtensions /// The release, or if MusicBrainz does not have it. public static Task LookupReleaseOrNullAsync(this Query query, Guid releaseId, Include include, ILogger logger, CancellationToken cancellationToken) => NotFoundAsNullAsync( - () => query.LookupReleaseAsync(releaseId, include, cancellationToken), + token => query.LookupReleaseAsync(releaseId, include, token), "release", releaseId, - logger); + logger, + cancellationToken); /// /// Looks up a release group, treating an unknown identifier as missing data rather than an error. @@ -65,10 +76,11 @@ internal static class MusicBrainzQueryExtensions /// The release group, or if MusicBrainz does not have it. public static Task LookupReleaseGroupOrNullAsync(this Query query, Guid releaseGroupId, Include include, ILogger logger, CancellationToken cancellationToken) => NotFoundAsNullAsync( - () => query.LookupReleaseGroupAsync(releaseGroupId, include, null, cancellationToken), + token => query.LookupReleaseGroupAsync(releaseGroupId, include, null, token), "release group", releaseGroupId, - logger); + logger, + cancellationToken); /// /// Looks up an artist, treating an unknown identifier as missing data rather than an error. @@ -81,10 +93,141 @@ internal static class MusicBrainzQueryExtensions /// The artist, or if MusicBrainz does not have it. public static Task LookupArtistOrNullAsync(this Query query, Guid artistId, Include include, ILogger logger, CancellationToken cancellationToken) => NotFoundAsNullAsync( - () => query.LookupArtistAsync(artistId, include, null, null, cancellationToken), + token => query.LookupArtistAsync(artistId, include, null, null, token), "artist", artistId, - logger); + logger, + cancellationToken); + + /// + /// Searches for artists, retrying when the MusicBrainz server is too busy to answer. + /// + /// The MusicBrainz query client. + /// The search query. + /// The logger. + /// The cancellation token. + /// The search results. + public static Task>> FindArtistsWithRetryAsync(this Query query, string searchQuery, ILogger logger, CancellationToken cancellationToken) + => RetryOnTransientErrorAsync( + token => query.FindArtistsAsync(searchQuery, null, null, false, token), + "artist search", + logger, + cancellationToken); + + /// + /// Searches for releases, retrying when the MusicBrainz server is too busy to answer. + /// + /// The MusicBrainz query client. + /// The search query. + /// The logger. + /// The cancellation token. + /// The search results. + public static Task>> FindReleasesWithRetryAsync(this Query query, string searchQuery, ILogger logger, CancellationToken cancellationToken) + => RetryOnTransientErrorAsync( + token => query.FindReleasesAsync(searchQuery, null, null, false, token), + "release search", + logger, + cancellationToken); + + /// + /// Runs a request, retrying it when MusicBrainz reports a transient failure. MusicBrainz sheds load with + /// HTTP 503 when its servers are busy, which is not specific to this client and succeeds when retried, so + /// failing the whole refresh on the first one would leave items without metadata for no good reason. + /// + /// The type of the request result. + /// The request to run. + /// The operation being performed, used for logging. + /// The logger. + /// The cancellation token. + /// The request result. + internal static async Task RetryOnTransientErrorAsync(Func> request, string operation, ILogger logger, CancellationToken cancellationToken) + { + for (var attempt = 1; ; attempt++) + { + try + { + return await request(cancellationToken).ConfigureAwait(false); + } + catch (HttpError ex) when (attempt <= MaxRetries && IsTransient(ex.Status)) + { + var delay = GetRetryDelay(ex, attempt); + logger.LogDebug( + ex, + "MusicBrainz {Operation} failed with {Status}, retrying in {Delay} (attempt {Attempt} of {Attempts})", + operation, + ex.Status, + delay, + attempt, + MaxRetries + 1); + + await Task.Delay(delay, cancellationToken).ConfigureAwait(false); + } + } + } + + /// + /// Determines whether a response status is worth retrying. These are all cases of the server being unable to + /// answer right now rather than of the request itself being wrong. + /// + /// The status returned by MusicBrainz. + /// Whether the request should be retried. + private static bool IsTransient(HttpStatusCode status) + => status is HttpStatusCode.TooManyRequests + or HttpStatusCode.BadGateway + or HttpStatusCode.ServiceUnavailable + or HttpStatusCode.GatewayTimeout; + + /// + /// Works out how long to wait before retrying. MusicBrainz reports when its current rate limit window ends and + /// retrying before then is documented to fail, so that hint wins over the exponential backoff when it is longer. + /// + /// The error returned by MusicBrainz. + /// The number of the attempt that just failed. + /// The time to wait before the next attempt. + internal static TimeSpan GetRetryDelay(HttpError error, int attempt) + { + var delay = TimeSpan.FromSeconds(Math.Pow(2, attempt - 1)); + var hint = GetServerHint(error); + if (hint > delay) + { + delay = hint.Value; + } + + if (delay < _minimumRetryDelay) + { + return _minimumRetryDelay; + } + + return delay > _maximumRetryDelay ? _maximumRetryDelay : delay; + } + + private static TimeSpan? GetServerHint(HttpError error) + { + var headers = error.ResponseHeaders; + if (headers is null) + { + return null; + } + + var retryAfter = headers.RetryAfter; + if (retryAfter?.Delta is { } delta) + { + return delta; + } + + if (retryAfter?.Date is { } date) + { + return date - DateTimeOffset.UtcNow; + } + + var rateLimit = new RateLimitInfo(headers); + if (rateLimit.ResetIn is { } resetIn) + { + return TimeSpan.FromSeconds(resetIn); + } + + return rateLimit.ResetAt - rateLimit.LastRequest; + } /// /// Runs a lookup, mapping a "not found" response to . Identifiers stored on a library item @@ -95,13 +238,14 @@ internal static class MusicBrainzQueryExtensions /// The type of entity being looked up, used for logging. /// The identifier being looked up, used for logging. /// The logger. + /// The cancellation token. /// The entity, or if MusicBrainz does not have it. - private static async Task NotFoundAsNullAsync(Func> lookup, string entityType, Guid id, ILogger logger) + private static async Task NotFoundAsNullAsync(Func> lookup, string entityType, Guid id, ILogger logger, CancellationToken cancellationToken) where T : class { try { - return await lookup().ConfigureAwait(false); + return await RetryOnTransientErrorAsync(lookup, entityType + " lookup", logger, cancellationToken).ConfigureAwait(false); } catch (HttpError ex) when (ex.Status == HttpStatusCode.NotFound) { diff --git a/tests/Jellyfin.Providers.Tests/Music/MusicBrainzQueryExtensionsTests.cs b/tests/Jellyfin.Providers.Tests/Music/MusicBrainzQueryExtensionsTests.cs new file mode 100644 index 0000000000..6b0d3a2ab2 --- /dev/null +++ b/tests/Jellyfin.Providers.Tests/Music/MusicBrainzQueryExtensionsTests.cs @@ -0,0 +1,136 @@ +using System; +using System.Globalization; +using System.Net; +using System.Net.Http; +using System.Net.Http.Headers; +using System.Threading; +using System.Threading.Tasks; +using MediaBrowser.Providers.Plugins.MusicBrainz; +using MetaBrainz.Common; +using Microsoft.Extensions.Logging.Abstractions; +using Xunit; + +namespace Jellyfin.Providers.Tests.Music; + +public static class MusicBrainzQueryExtensionsTests +{ + [Fact] + public static async Task RetryOnTransientErrorAsync_ServerBusy_RetriesAndSucceeds() + { + var attempts = 0; + + var result = await MusicBrainzQueryExtensions.RetryOnTransientErrorAsync( + async _ => + { + attempts++; + if (attempts == 1) + { + throw await CreateErrorAsync(HttpStatusCode.ServiceUnavailable, TimeSpan.Zero); + } + + return "found"; + }, + "test", + NullLogger.Instance, + CancellationToken.None); + + Assert.Equal("found", result); + Assert.Equal(2, attempts); + } + + [Fact] + public static async Task RetryOnTransientErrorAsync_ServerStaysBusy_GivesUp() + { + var attempts = 0; + + var error = await Assert.ThrowsAsync(() => MusicBrainzQueryExtensions.RetryOnTransientErrorAsync( + async _ => + { + attempts++; + throw await CreateErrorAsync(HttpStatusCode.ServiceUnavailable, TimeSpan.Zero); + }, + "test", + NullLogger.Instance, + CancellationToken.None)); + + Assert.Equal(HttpStatusCode.ServiceUnavailable, error.Status); + Assert.Equal(3, attempts); + } + + [Fact] + public static async Task RetryOnTransientErrorAsync_NotFound_DoesNotRetry() + { + var attempts = 0; + + await Assert.ThrowsAsync(() => MusicBrainzQueryExtensions.RetryOnTransientErrorAsync( + async _ => + { + attempts++; + throw await CreateErrorAsync(HttpStatusCode.NotFound, null); + }, + "test", + NullLogger.Instance, + CancellationToken.None)); + + Assert.Equal(1, attempts); + } + + [Fact] + public static async Task GetRetryDelay_NoHint_BacksOffExponentially() + { + var error = await CreateErrorAsync(HttpStatusCode.ServiceUnavailable, null); + + Assert.Equal(TimeSpan.FromSeconds(1), MusicBrainzQueryExtensions.GetRetryDelay(error, 1)); + Assert.Equal(TimeSpan.FromSeconds(2), MusicBrainzQueryExtensions.GetRetryDelay(error, 2)); + } + + [Fact] + public static async Task GetRetryDelay_RetryAfterZero_WaitsMinimum() + { + var error = await CreateErrorAsync(HttpStatusCode.ServiceUnavailable, TimeSpan.Zero); + + Assert.Equal(TimeSpan.FromSeconds(1), MusicBrainzQueryExtensions.GetRetryDelay(error, 1)); + } + + [Fact] + public static async Task GetRetryDelay_RetryAfterLongerThanBackoff_UsesRetryAfter() + { + var error = await CreateErrorAsync(HttpStatusCode.ServiceUnavailable, TimeSpan.FromSeconds(10)); + + Assert.Equal(TimeSpan.FromSeconds(10), MusicBrainzQueryExtensions.GetRetryDelay(error, 1)); + } + + [Fact] + public static async Task GetRetryDelay_LongRetryAfter_IsCapped() + { + var error = await CreateErrorAsync(HttpStatusCode.ServiceUnavailable, TimeSpan.FromHours(1)); + + Assert.Equal(TimeSpan.FromSeconds(15), MusicBrainzQueryExtensions.GetRetryDelay(error, 1)); + } + + [Fact] + public static async Task GetRetryDelay_RateLimitWindow_WaitsForReset() + { + var error = await CreateErrorAsync( + HttpStatusCode.ServiceUnavailable, + null, + headers => headers.TryAddWithoutValidation("X-RateLimit-Reset", DateTimeOffset.UtcNow.AddSeconds(8).ToUnixTimeSeconds().ToString(CultureInfo.InvariantCulture))); + + var delay = MusicBrainzQueryExtensions.GetRetryDelay(error, 1); + + Assert.InRange(delay, TimeSpan.FromSeconds(6), TimeSpan.FromSeconds(8)); + } + + private static async Task CreateErrorAsync(HttpStatusCode status, TimeSpan? retryAfter, Action? configureHeaders = null) + { + using var response = new HttpResponseMessage(status); + if (retryAfter is not null) + { + response.Headers.RetryAfter = new RetryConditionHeaderValue(retryAfter.Value); + } + + configureHeaders?.Invoke(response.Headers); + + return await HttpError.FromResponseAsync(response); + } +} -- cgit v1.2.3