Backport pull request #17947 from jellyfin/release-12.z

Don't record a failed refresh as a completed one

Original-merge: 8453ff2359ce1c35883226035eeb4c4694bf0890

Merged-by: crobibero <cody@robibe.ro>

Backported-by: Cody Robibero <cody@robibe.ro>

6 files changed, 343 insertions, 2 deletions

diff --git a/MediaBrowser.Providers/Manager/ItemImageProvider.cs b/MediaBrowser.Providers/Manager/ItemImageProvider.cs
index 727f481b65..ce5468c5c4 100644
--- a/MediaBrowser.Providers/Manager/ItemImageProvider.cs
+++ b/MediaBrowser.Providers/Manager/ItemImageProvider.cs
@@ -254,6 +254,7 @@ namespace MediaBrowser.Providers.Manager
             }
             catch (Exception ex)
             {
+                result.Failures++;
                 result.ErrorMessage = ex.Message;
                 _logger.LogError(ex, "Error in {Provider} for {Item}", provider.Name, item.Path ?? item.Name);
             }
@@ -338,6 +339,7 @@ namespace MediaBrowser.Providers.Manager
             }
             catch (Exception ex)
             {
+                result.Failures++;
                 result.ErrorMessage = ex.Message;
                 _logger.LogError(ex, "Error in {Provider} for {Item}", provider.Name, item.Path ?? item.Name);
             }
diff --git a/MediaBrowser.Providers/Manager/MetadataService.cs b/MediaBrowser.Providers/Manager/MetadataService.cs
index fe5285bf65..05c542337e 100644
--- a/MediaBrowser.Providers/Manager/MetadataService.cs
+++ b/MediaBrowser.Providers/Manager/MetadataService.cs
@@ -192,9 +192,13 @@ namespace MediaBrowser.Providers.Manager
                 }
             }
 
-            // Next run remote image providers, but only if local image providers didn't throw an exception
-            if (!localImagesFailed && refreshOptions.ImageRefreshMode > MetadataRefreshMode.ValidationOnly)
+            if (localImagesFailed)
             {
+                hasRefreshedImages = false;
+            }
+            else if (refreshOptions.ImageRefreshMode > MetadataRefreshMode.ValidationOnly)
+            {
+                // Next run remote image providers, now that local image providers didn't throw
                 var providers = GetNonLocalImageProviders(item, allImageProviders, refreshOptions).ToList();
 
                 if (providers.Count > 0)
@@ -937,6 +941,7 @@ namespace MediaBrowser.Providers.Manager
             }
             catch (Exception ex)
             {
+                refreshResult.Failures++;
                 refreshResult.ErrorMessage = ex.Message;
                 Logger.LogError(ex, "Error in {Provider} for {Item}", provider.Name, logName);
             }
diff --git a/MediaBrowser.Providers/MediaInfo/ProbeProvider.cs b/MediaBrowser.Providers/MediaInfo/ProbeProvider.cs
index 221c6bff5e..5d78cdd0be 100644
--- a/MediaBrowser.Providers/MediaInfo/ProbeProvider.cs
+++ b/MediaBrowser.Providers/MediaInfo/ProbeProvider.cs
@@ -142,6 +142,12 @@ namespace MediaBrowser.Providers.MediaInfo
                 }
             }
 
+            if (IsMissingMediaInfo(item))
+            {
+                _logger.LogDebug("Refreshing {ItemPath} because it has no media information.", item.Path);
+                return true;
+            }
+
             if (video is not null
                 && item.SupportsLocalMetadata
                 && !video.IsPlaceHolder)
@@ -175,6 +181,25 @@ namespace MediaBrowser.Providers.MediaInfo
             return false;
         }
 
+        private static bool IsMissingMediaInfo(BaseItem item)
+        {
+            if (item.RunTimeTicks.HasValue
+                || item.TotalBitrate.HasValue
+                || item.IsVirtualItem
+                || item.IsShortcut
+                || !item.IsFileProtocol)
+            {
+                return false;
+            }
+
+            return item switch
+            {
+                Video video => !video.IsPlaceHolder && video.IsCompleteMedia,
+                Audio => true,
+                _ => false
+            };
+        }
+
         /// <inheritdoc />
         public Task<ItemUpdateType> FetchAsync(Episode item, MetadataRefreshOptions options, CancellationToken cancellationToken)
         {
diff --git a/tests/Jellyfin.Providers.Tests/Manager/ItemImageProviderTests.cs b/tests/Jellyfin.Providers.Tests/Manager/ItemImageProviderTests.cs
index 1ec859223e..459973acba 100644
--- a/tests/Jellyfin.Providers.Tests/Manager/ItemImageProviderTests.cs
+++ b/tests/Jellyfin.Providers.Tests/Manager/ItemImageProviderTests.cs
@@ -557,6 +557,47 @@ namespace Jellyfin.Providers.Tests.Manager
             Assert.Equal(expectedToUpdate, result.UpdateType.HasFlag(ItemUpdateType.ImageUpdate));
         }
 
+        [Fact]
+        public async Task RefreshImages_ProviderDynamicThrows_CountsAFailure()
+        {
+            var item = GetItemWithImages(ImageType.Primary, 0, false);
+            var libraryOptions = GetLibraryOptions(item, ImageType.Primary, 1);
+
+            var dynamicProvider = new Mock<IDynamicImageProvider>(MockBehavior.Strict);
+            dynamicProvider.Setup(rp => rp.Name).Returns("MockDynamicProvider");
+            dynamicProvider.Setup(rp => rp.GetSupportedImages(item))
+                .Returns(new[] { ImageType.Primary });
+            dynamicProvider.Setup(rp => rp.GetImage(item, ImageType.Primary, It.IsAny<CancellationToken>()))
+                .ThrowsAsync(new InvalidOperationException("provider is broken"));
+
+            var itemImageProvider = GetItemImageProvider(null, new Mock<IFileSystem>());
+            var result = await itemImageProvider.RefreshImages(item, libraryOptions, new List<IImageProvider> { dynamicProvider.Object }, new ImageRefreshOptions(Mock.Of<IDirectoryService>()), CancellationToken.None);
+
+            // Without this the caller stamps DateLastRefreshed and never asks this provider again.
+            Assert.Equal(1, result.Failures);
+        }
+
+        [Fact]
+        public async Task RefreshImages_ProviderRemoteThrows_CountsAFailure()
+        {
+            var item = GetItemWithImages(ImageType.Primary, 0, false);
+            var libraryOptions = GetLibraryOptions(item, ImageType.Primary, 1);
+
+            var remoteProvider = new Mock<IRemoteImageProvider>(MockBehavior.Strict);
+            remoteProvider.Setup(rp => rp.Name).Returns("MockRemoteProvider");
+            remoteProvider.Setup(rp => rp.GetSupportedImages(item))
+                .Returns(new[] { ImageType.Primary });
+
+            var providerManager = new Mock<IProviderManager>(MockBehavior.Strict);
+            providerManager.Setup(pm => pm.GetAvailableRemoteImages(It.IsAny<BaseItem>(), It.IsAny<RemoteImageQuery>(), It.IsAny<CancellationToken>()))
+                .ThrowsAsync(new HttpRequestException("unreachable"));
+
+            var itemImageProvider = GetItemImageProvider(providerManager.Object, new Mock<IFileSystem>());
+            var result = await itemImageProvider.RefreshImages(item, libraryOptions, new List<IImageProvider> { remoteProvider.Object }, new ImageRefreshOptions(Mock.Of<IDirectoryService>()), CancellationToken.None);
+
+            Assert.Equal(1, result.Failures);
+        }
+
         private static ItemImageProvider GetItemImageProvider(IProviderManager? providerManager, Mock<IFileSystem>? mockFileSystem)
         {
             // strict to ensure this isn't accidentally used where a prepared mock is intended
diff --git a/tests/Jellyfin.Providers.Tests/Manager/MetadataServiceRefreshTests.cs b/tests/Jellyfin.Providers.Tests/Manager/MetadataServiceRefreshTests.cs
index 3b4d6fc9bb..acf8de4366 100644
--- a/tests/Jellyfin.Providers.Tests/Manager/MetadataServiceRefreshTests.cs
+++ b/tests/Jellyfin.Providers.Tests/Manager/MetadataServiceRefreshTests.cs
@@ -287,6 +287,161 @@ namespace Jellyfin.Providers.Tests.Manager
             }
         }
 
+        [Theory]
+        [InlineData(true)]
+        [InlineData(false)]
+        public async Task RefreshMetadata_CustomProviderThrew_LeavesRefreshDateAlone(bool providerThrows)
+        {
+            var item = new TestItem
+            {
+                Id = Guid.NewGuid(),
+                Name = "Test Item",
+                PreferredMetadataLanguage = "en",
+                PreferredMetadataCountryCode = "US",
+                DateLastRefreshed = DateTime.UtcNow.AddDays(-60),
+                DateLastSaved = DateTime.UtcNow.AddDays(-60)
+            };
+            item.PresentationUniqueKey = item.CreatePresentationUniqueKey();
+
+            var stampBefore = item.DateLastRefreshed;
+
+            // Stands in for the probe provider, whose only other change monitor is the file's
+            // modification time: if a throw is stamped as a completed refresh the item is never revisited.
+            var provider = new Mock<ICustomMetadataProvider<TestItem>>(MockBehavior.Loose);
+            provider.Setup(p => p.Name).Returns("Throwing Provider");
+            provider.Setup(p => p.FetchAsync(It.IsAny<TestItem>(), It.IsAny<MetadataRefreshOptions>(), It.IsAny<CancellationToken>()))
+                .Returns(providerThrows
+                    ? Task.FromException<ItemUpdateType>(new InvalidOperationException("probe failed"))
+                    : Task.FromResult(ItemUpdateType.None));
+
+            var libraryManager = new Mock<ILibraryManager>(MockBehavior.Loose);
+            libraryManager.Setup(l => l.GetLibraryOptions(It.IsAny<BaseItem>())).Returns(new LibraryOptions());
+
+            var providerManager = new Mock<IProviderManager>(MockBehavior.Loose);
+            providerManager.Setup(p => p.GetImageProviders(It.IsAny<BaseItem>(), It.IsAny<ImageRefreshOptions>()))
+                .Returns(Array.Empty<IImageProvider>());
+            providerManager.Setup(p => p.GetMetadataProviders<TestItem>(It.IsAny<BaseItem>(), It.IsAny<LibraryOptions>()))
+                .Returns(new[] { (IMetadataProvider<TestItem>)provider.Object });
+            providerManager.Setup(p => p.GetMetadataSavers(It.IsAny<BaseItem>(), It.IsAny<LibraryOptions>()))
+                .Returns(Array.Empty<IMetadataSaver>());
+
+            var itemRepository = new Mock<IItemRepository>(MockBehavior.Loose);
+            itemRepository.Setup(r => r.ItemExistsAsync(It.IsAny<Guid>())).ReturnsAsync(true);
+
+            var service = new TestItemMetadataService(libraryManager.Object, providerManager.Object, itemRepository.Object);
+
+            await service.RefreshMetadata(
+                item,
+                new MetadataRefreshOptions(Mock.Of<IDirectoryService>())
+                {
+                    MetadataRefreshMode = MetadataRefreshMode.FullRefresh,
+                    ImageRefreshMode = MetadataRefreshMode.FullRefresh
+                },
+                CancellationToken.None).ConfigureAwait(true);
+
+            Assert.Equal(providerThrows, item.DateLastRefreshed == stampBefore);
+        }
+
+        [Theory]
+        [InlineData(true)]
+        [InlineData(false)]
+        public async Task RefreshMetadata_ImageProviderThrew_LeavesRefreshDateAlone(bool providerThrows)
+        {
+            var item = NewStampedTestItem();
+            var stampBefore = item.DateLastRefreshed;
+
+            var imageProvider = new Mock<IDynamicImageProvider>(MockBehavior.Loose);
+            imageProvider.Setup(p => p.Name).Returns("Throwing Image Provider");
+            imageProvider.Setup(p => p.GetSupportedImages(It.IsAny<BaseItem>())).Returns(new[] { ImageType.Primary });
+            imageProvider.Setup(p => p.GetImage(It.IsAny<BaseItem>(), ImageType.Primary, It.IsAny<CancellationToken>()))
+                .Returns(providerThrows
+                    ? Task.FromException<DynamicImageResponse>(new InvalidOperationException("image fetch failed"))
+                    : Task.FromResult(new DynamicImageResponse { HasImage = false }));
+
+            var providerManager = NewProviderManager(imageProviders: [imageProvider.Object]);
+
+            var service = NewService(providerManager);
+
+            await service.RefreshMetadata(
+                item,
+                new MetadataRefreshOptions(Mock.Of<IDirectoryService>())
+                {
+                    MetadataRefreshMode = MetadataRefreshMode.FullRefresh,
+                    ImageRefreshMode = MetadataRefreshMode.FullRefresh
+                },
+                CancellationToken.None).ConfigureAwait(true);
+
+            Assert.Equal(providerThrows, item.DateLastRefreshed == stampBefore);
+        }
+
+        [Fact]
+        public async Task RefreshMetadata_LocalImageValidationThrew_LeavesRefreshDateAlone()
+        {
+            var item = NewStampedTestItem();
+            var stampBefore = item.DateLastRefreshed;
+
+            // A throw here skips the remote image stage altogether, so no image work happened at all.
+            var localImageProvider = new Mock<ILocalImageProvider>(MockBehavior.Loose);
+            localImageProvider.Setup(p => p.Name).Returns("Throwing Local Image Provider");
+            localImageProvider.Setup(p => p.GetImages(It.IsAny<BaseItem>(), It.IsAny<IDirectoryService>()))
+                .Throws(new UnauthorizedAccessException("metadata folder is not readable"));
+
+            var providerManager = NewProviderManager(imageProviders: [localImageProvider.Object]);
+
+            var service = NewService(providerManager);
+
+            await service.RefreshMetadata(
+                item,
+                new MetadataRefreshOptions(Mock.Of<IDirectoryService>())
+                {
+                    MetadataRefreshMode = MetadataRefreshMode.FullRefresh,
+                    ImageRefreshMode = MetadataRefreshMode.FullRefresh
+                },
+                CancellationToken.None).ConfigureAwait(true);
+
+            Assert.Equal(stampBefore, item.DateLastRefreshed);
+        }
+
+        private static TestItem NewStampedTestItem()
+        {
+            var item = new TestItem
+            {
+                Id = Guid.NewGuid(),
+                Name = "Test Item",
+                PreferredMetadataLanguage = "en",
+                PreferredMetadataCountryCode = "US",
+                DateLastRefreshed = DateTime.UtcNow.AddDays(-60),
+                DateLastSaved = DateTime.UtcNow.AddDays(-60)
+            };
+            item.PresentationUniqueKey = item.CreatePresentationUniqueKey();
+            return item;
+        }
+
+        private static Mock<IProviderManager> NewProviderManager(
+            IMetadataProvider<TestItem>[]? metadataProviders = null,
+            IImageProvider[]? imageProviders = null)
+        {
+            var providerManager = new Mock<IProviderManager>(MockBehavior.Loose);
+            providerManager.Setup(p => p.GetImageProviders(It.IsAny<BaseItem>(), It.IsAny<ImageRefreshOptions>()))
+                .Returns(imageProviders ?? Array.Empty<IImageProvider>());
+            providerManager.Setup(p => p.GetMetadataProviders<TestItem>(It.IsAny<BaseItem>(), It.IsAny<LibraryOptions>()))
+                .Returns(metadataProviders ?? Array.Empty<IMetadataProvider<TestItem>>());
+            providerManager.Setup(p => p.GetMetadataSavers(It.IsAny<BaseItem>(), It.IsAny<LibraryOptions>()))
+                .Returns(Array.Empty<IMetadataSaver>());
+            return providerManager;
+        }
+
+        private static TestItemMetadataService NewService(Mock<IProviderManager> providerManager)
+        {
+            var libraryManager = new Mock<ILibraryManager>(MockBehavior.Loose);
+            libraryManager.Setup(l => l.GetLibraryOptions(It.IsAny<BaseItem>())).Returns(new LibraryOptions());
+
+            var itemRepository = new Mock<IItemRepository>(MockBehavior.Loose);
+            itemRepository.Setup(r => r.ItemExistsAsync(It.IsAny<Guid>())).ReturnsAsync(true);
+
+            return new TestItemMetadataService(libraryManager.Object, providerManager.Object, itemRepository.Object);
+        }
+
         /// <summary>
         /// Stands in for a real item so the refresh stays off the shared BaseItem statics, which other
         /// test classes in this assembly overwrite while xUnit runs them in parallel.
diff --git a/tests/Jellyfin.Providers.Tests/MediaInfo/ProbeProviderTests.cs b/tests/Jellyfin.Providers.Tests/MediaInfo/ProbeProviderTests.cs
new file mode 100644
index 0000000000..cd7ae0f525
--- /dev/null
+++ b/tests/Jellyfin.Providers.Tests/MediaInfo/ProbeProviderTests.cs
@@ -0,0 +1,113 @@
+using Emby.Naming.Common;
+using MediaBrowser.Controller.Chapters;
+using MediaBrowser.Controller.Configuration;
+using MediaBrowser.Controller.Entities;
+using MediaBrowser.Controller.Entities.Audio;
+using MediaBrowser.Controller.Entities.TV;
+using MediaBrowser.Controller.Library;
+using MediaBrowser.Controller.LiveTv;
+using MediaBrowser.Controller.Lyrics;
+using MediaBrowser.Controller.MediaEncoding;
+using MediaBrowser.Controller.Persistence;
+using MediaBrowser.Controller.Providers;
+using MediaBrowser.Controller.Subtitles;
+using MediaBrowser.Model.Globalization;
+using MediaBrowser.Model.IO;
+using MediaBrowser.Model.MediaInfo;
+using MediaBrowser.Providers.MediaInfo;
+using Microsoft.Extensions.Logging.Abstractions;
+using Moq;
+using Xunit;
+
+namespace Jellyfin.Providers.Tests.MediaInfo;
+
+public class ProbeProviderTests
+{
+    private readonly ProbeProvider _probeProvider;
+
+    public ProbeProviderTests()
+    {
+        var mediaSourceManager = new Mock<IMediaSourceManager>();
+        mediaSourceManager.Setup(m => m.GetPathProtocol(It.IsAny<string>()))
+            .Returns(MediaProtocol.File);
+
+        // prep BaseItem and Video for calls made that expect managers
+        BaseItem.MediaSourceManager = mediaSourceManager.Object;
+        Video.RecordingsManager = Mock.Of<IRecordingsManager>();
+
+        _probeProvider = new ProbeProvider(
+            mediaSourceManager.Object,
+            Mock.Of<IMediaEncoder>(),
+            Mock.Of<IBlurayExaminer>(),
+            Mock.Of<ILocalizationManager>(),
+            Mock.Of<IChapterManager>(),
+            Mock.Of<IServerConfigurationManager>(),
+            Mock.Of<ISubtitleManager>(),
+            Mock.Of<ILibraryManager>(),
+            Mock.Of<IFileSystem>(),
+            NullLoggerFactory.Instance,
+            new NamingOptions(),
+            Mock.Of<ILyricManager>(),
+            Mock.Of<IMediaAttachmentRepository>(),
+            Mock.Of<IMediaStreamRepository>());
+    }
+
+    [Fact]
+    public void HasChanged_NeverProbedVideo_ReturnsTrue()
+    {
+        // A probe that threw leaves the item like this while the refresh is stamped as done, and the
+        // file's modification time never changes afterwards, so nothing else would ask for a retry.
+        var item = new Episode { Path = "/media/show/S01E01.mkv" };
+
+        Assert.True(_probeProvider.HasChanged(item, Mock.Of<IDirectoryService>()));
+    }
+
+    [Fact]
+    public void HasChanged_NeverProbedAudio_ReturnsTrue()
+    {
+        var item = new Audio { Path = "/media/music/track.flac" };
+
+        Assert.True(_probeProvider.HasChanged(item, Mock.Of<IDirectoryService>()));
+    }
+
+    [Theory]
+    [InlineData(12345L, null)]
+    [InlineData(null, 2480000)]
+    [InlineData(12345L, 2480000)]
+    public void HasChanged_ProbedVideo_ReturnsFalse(long? runTimeTicks, int? totalBitrate)
+    {
+        var item = new Episode
+        {
+            Path = "/media/show/S01E01.mkv",
+            RunTimeTicks = runTimeTicks,
+            TotalBitrate = totalBitrate
+        };
+
+        Assert.False(_probeProvider.HasChanged(item, Mock.Of<IDirectoryService>()));
+    }
+
+    [Fact]
+    public void HasChanged_VirtualItemWithoutMediaInfo_ReturnsFalse()
+    {
+        var item = new Episode { Path = "/media/show/S01E01.mkv", IsVirtualItem = true };
+
+        Assert.False(_probeProvider.HasChanged(item, Mock.Of<IDirectoryService>()));
+    }
+
+    [Fact]
+    public void HasChanged_PlaceHolderWithoutMediaInfo_ReturnsFalse()
+    {
+        var item = new Episode { Path = "/media/show/S01E01.disc", IsPlaceHolder = true };
+
+        Assert.False(_probeProvider.HasChanged(item, Mock.Of<IDirectoryService>()));
+    }
+
+    [Fact]
+    public void HasChanged_ShortcutWithoutMediaInfo_ReturnsFalse()
+    {
+        // A .strm is only probed when remote content probing is enabled, so an empty one is expected.
+        var item = new Episode { Path = "/media/show/S01E01.strm", IsShortcut = true };
+
+        Assert.False(_probeProvider.HasChanged(item, Mock.Of<IDirectoryService>()));
+    }
+}