diff options
4 files changed, 294 insertions, 14 deletions
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<MusicAlbum, Albu if (!string.IsNullOrWhiteSpace(artistMusicBrainzId)) { - var releaseSearchResults = await query.FindReleasesAsync($"\"{searchInfo.Name}\" AND arid:{artistMusicBrainzId}", null, null, false, cancellationToken) + var releaseSearchResults = await query.FindReleasesWithRetryAsync($"\"{searchInfo.Name}\" AND arid:{artistMusicBrainzId}", _logger, cancellationToken) .ConfigureAwait(false); if (releaseSearchResults.Results.Count > 0) @@ -83,7 +83,7 @@ public class MusicBrainzAlbumProvider : IRemoteMetadataProvider<MusicAlbum, Albu // I'm sure there is a better way but for now it resolves search for 12" Mixes var queryName = searchInfo.Name.Replace("\"", string.Empty, StringComparison.Ordinal); - var releaseSearchResults = await query.FindReleasesAsync($"\"{queryName}\" AND artist:\"{searchInfo.GetAlbumArtist()}\"c", null, null, false, cancellationToken) + var releaseSearchResults = await query.FindReleasesWithRetryAsync($"\"{queryName}\" AND artist:\"{searchInfo.GetAlbumArtist()}\"c", _logger, cancellationToken) .ConfigureAwait(false); if (releaseSearchResults.Results.Count > 0) @@ -200,13 +200,13 @@ public class MusicBrainzAlbumProvider : IRemoteMetadataProvider<MusicAlbum, Albu if (!string.IsNullOrEmpty(artistMusicBrainzId)) { - var releaseSearchResults = await query.FindReleasesAsync($"\"{info.Name}\" AND arid:{artistMusicBrainzId}", null, null, false, cancellationToken) + var releaseSearchResults = await query.FindReleasesWithRetryAsync($"\"{info.Name}\" AND arid:{artistMusicBrainzId}", _logger, cancellationToken) .ConfigureAwait(false); releaseResult = releaseSearchResults.Results.Count > 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<MusicArtist, Ar return []; } - var artistSearchResults = await query.FindArtistsAsync($"\"{searchInfo.Name}\"", null, null, false, cancellationToken) + var artistSearchResults = await query.FindArtistsWithRetryAsync($"\"{searchInfo.Name}\"", _logger, cancellationToken) .ConfigureAwait(false); if (artistSearchResults.Results.Count > 0) { @@ -70,7 +70,7 @@ public class MusicBrainzArtistProvider : IRemoteMetadataProvider<MusicArtist, Ar if (searchInfo.Name.HasDiacritics()) { // Try again using the search with an accented characters query - var artistAccentsSearchResults = await query.FindArtistsAsync($"artistaccent:\"{searchInfo.Name}\"", null, null, false, cancellationToken) + var artistAccentsSearchResults = await query.FindArtistsWithRetryAsync($"artistaccent:\"{searchInfo.Name}\"", _logger, cancellationToken) .ConfigureAwait(false); if (artistAccentsSearchResults.Results.Count > 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; @@ -15,6 +16,15 @@ namespace MediaBrowser.Providers.Plugins.MusicBrainz; internal static class MusicBrainzQueryExtensions { /// <summary> + /// The number of extra attempts made when MusicBrainz reports a transient failure. + /// </summary> + private const int MaxRetries = 2; + + private static readonly TimeSpan _minimumRetryDelay = TimeSpan.FromSeconds(1); + + private static readonly TimeSpan _maximumRetryDelay = TimeSpan.FromSeconds(15); + + /// <summary> /// Parses a MusicBrainz identifier, which may come from user-supplied tags or NFO files and is therefore not /// guaranteed to be a valid GUID. /// </summary> @@ -49,10 +59,11 @@ internal static class MusicBrainzQueryExtensions /// <returns>The release, or <see langword="null"/> if MusicBrainz does not have it.</returns> public static Task<IRelease?> 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); /// <summary> /// Looks up a release group, treating an unknown identifier as missing data rather than an error. @@ -65,10 +76,11 @@ internal static class MusicBrainzQueryExtensions /// <returns>The release group, or <see langword="null"/> if MusicBrainz does not have it.</returns> public static Task<IReleaseGroup?> 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); /// <summary> /// Looks up an artist, treating an unknown identifier as missing data rather than an error. @@ -81,10 +93,141 @@ internal static class MusicBrainzQueryExtensions /// <returns>The artist, or <see langword="null"/> if MusicBrainz does not have it.</returns> public static Task<IArtist?> 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); + + /// <summary> + /// Searches for artists, retrying when the MusicBrainz server is too busy to answer. + /// </summary> + /// <param name="query">The MusicBrainz query client.</param> + /// <param name="searchQuery">The search query.</param> + /// <param name="logger">The logger.</param> + /// <param name="cancellationToken">The cancellation token.</param> + /// <returns>The search results.</returns> + public static Task<ISearchResults<ISearchResult<IArtist>>> FindArtistsWithRetryAsync(this Query query, string searchQuery, ILogger logger, CancellationToken cancellationToken) + => RetryOnTransientErrorAsync( + token => query.FindArtistsAsync(searchQuery, null, null, false, token), + "artist search", + logger, + cancellationToken); + + /// <summary> + /// Searches for releases, retrying when the MusicBrainz server is too busy to answer. + /// </summary> + /// <param name="query">The MusicBrainz query client.</param> + /// <param name="searchQuery">The search query.</param> + /// <param name="logger">The logger.</param> + /// <param name="cancellationToken">The cancellation token.</param> + /// <returns>The search results.</returns> + public static Task<ISearchResults<ISearchResult<IRelease>>> FindReleasesWithRetryAsync(this Query query, string searchQuery, ILogger logger, CancellationToken cancellationToken) + => RetryOnTransientErrorAsync( + token => query.FindReleasesAsync(searchQuery, null, null, false, token), + "release search", + logger, + cancellationToken); + + /// <summary> + /// 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. + /// </summary> + /// <typeparam name="T">The type of the request result.</typeparam> + /// <param name="request">The request to run.</param> + /// <param name="operation">The operation being performed, used for logging.</param> + /// <param name="logger">The logger.</param> + /// <param name="cancellationToken">The cancellation token.</param> + /// <returns>The request result.</returns> + internal static async Task<T> RetryOnTransientErrorAsync<T>(Func<CancellationToken, Task<T>> 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); + } + } + } + + /// <summary> + /// 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. + /// </summary> + /// <param name="status">The status returned by MusicBrainz.</param> + /// <returns>Whether the request should be retried.</returns> + private static bool IsTransient(HttpStatusCode status) + => status is HttpStatusCode.TooManyRequests + or HttpStatusCode.BadGateway + or HttpStatusCode.ServiceUnavailable + or HttpStatusCode.GatewayTimeout; + + /// <summary> + /// 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. + /// </summary> + /// <param name="error">The error returned by MusicBrainz.</param> + /// <param name="attempt">The number of the attempt that just failed.</param> + /// <returns>The time to wait before the next attempt.</returns> + 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; + } /// <summary> /// Runs a lookup, mapping a "not found" response to <see langword="null"/>. Identifiers stored on a library item @@ -95,13 +238,14 @@ internal static class MusicBrainzQueryExtensions /// <param name="entityType">The type of entity being looked up, used for logging.</param> /// <param name="id">The identifier being looked up, used for logging.</param> /// <param name="logger">The logger.</param> + /// <param name="cancellationToken">The cancellation token.</param> /// <returns>The entity, or <see langword="null"/> if MusicBrainz does not have it.</returns> - private static async Task<T?> NotFoundAsNullAsync<T>(Func<Task<T>> lookup, string entityType, Guid id, ILogger logger) + private static async Task<T?> NotFoundAsNullAsync<T>(Func<CancellationToken, Task<T>> 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<HttpError>(() => MusicBrainzQueryExtensions.RetryOnTransientErrorAsync<string>( + 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<HttpError>(() => MusicBrainzQueryExtensions.RetryOnTransientErrorAsync<string>( + 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<HttpError> CreateErrorAsync(HttpStatusCode status, TimeSpan? retryAfter, Action<HttpResponseHeaders>? 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); + } +} |