Merge pull request #18095 from Shadowghost/fix/18084-musicbrainz-retry

Retry MusicBrainz requests when the server is busy

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;
             }

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)
             {

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;
 /// </summary>
 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.
@@ -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)
         {

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);
+    }
+}