Persist the refresh stamp so the people task stops redoing its work

3 files changed, 150 insertions, 35 deletions

diff --git a/Emby.Server.Implementations/ScheduledTasks/Tasks/PeopleValidationTask.cs b/Emby.Server.Implementations/ScheduledTasks/Tasks/PeopleValidationTask.cs
index bd73f63aa7..afb27ddf9e 100644
--- a/Emby.Server.Implementations/ScheduledTasks/Tasks/PeopleValidationTask.cs
+++ b/Emby.Server.Implementations/ScheduledTasks/Tasks/PeopleValidationTask.cs
@@ -177,56 +177,51 @@ public class PeopleValidationTask : IScheduledTask, IConfigurableScheduledTask
         var thirtyDaysAgo = DateTime.UtcNow.AddDays(-30);
         var personTypeName = _itemTypeLookup.BaseItemKindNames[BaseItemKind.Person];
 
+        List<Guid> peopleIds;
+
         var context = await _dbContextFactory.CreateDbContextAsync(cancellationToken).ConfigureAwait(false);
         await using (context.ConfigureAwait(false))
         {
-            const int PartitionSize = 100;
-
-            var numPeople = await context.BaseItems
+            // Read the candidates in one go rather than paging them. A refresh stamps the person and takes
+            // it out of this set, so a growing offset over a shrinking set walks past people it never visits.
+            peopleIds = await context.BaseItems
                 .AsNoTracking()
                 .Where(b => b.Type == personTypeName)
                 .Where(b => b.DateLastRefreshed == null || b.DateLastRefreshed < thirtyDaysAgo)
                 .Where(b =>
                     !b.Images!.Any(i => i.ImageType == ImageInfoImageType.Primary) ||
                     string.IsNullOrEmpty(b.Overview))
-                .CountAsync(cancellationToken)
+                .OrderBy(b => b.Id)
+                .Select(b => b.Id)
+                .ToListAsync(cancellationToken)
                 .ConfigureAwait(false);
+        }
 
-            _logger.LogDebug("Found {Count} people needing image/overview refresh", numPeople);
+        _logger.LogDebug("Found {Count} people needing image/overview refresh", peopleIds.Count);
 
-            if (numPeople == 0)
-            {
-                progress.Report(100);
-                return;
-            }
+        if (peopleIds.Count == 0)
+        {
+            progress.Report(100);
+            return;
+        }
 
-            var numComplete = 0;
-            var numRefreshed = 0;
+        var numComplete = 0;
+        var numRefreshed = 0;
 
-            await foreach (var entry in context.BaseItems
-                .AsNoTracking()
-                .Where(b => b.Type == personTypeName)
-                .Where(b => b.DateLastRefreshed == null || b.DateLastRefreshed < thirtyDaysAgo)
-                .Where(b =>
-                    !b.Images!.Any(i => i.ImageType == ImageInfoImageType.Primary) ||
-                    string.IsNullOrEmpty(b.Overview))
-                .OrderBy(b => b.Id)
-                .WithPartitionProgress(partition => _logger.LogDebug("Processing people partition {Partition}", partition))
-                .PartitionEagerAsync(PartitionSize, cancellationToken)
-                .WithCancellation(cancellationToken)
-                .ConfigureAwait(false))
-            {
-                if (await RefreshPersonAsync(entry.Id, cancellationToken).ConfigureAwait(false))
-                {
-                    numRefreshed++;
-                }
+        foreach (var personId in peopleIds)
+        {
+            cancellationToken.ThrowIfCancellationRequested();
 
-                numComplete++;
-                progress.Report(100.0 * numComplete / numPeople);
+            if (await RefreshPersonAsync(personId, cancellationToken).ConfigureAwait(false))
+            {
+                numRefreshed++;
             }
 
-            _logger.LogInformation("Refreshed metadata for {Count} people missing images or overview", numRefreshed);
+            numComplete++;
+            progress.Report(100.0 * numComplete / peopleIds.Count);
         }
+
+        _logger.LogInformation("Refreshed metadata for {Count} people missing images or overview", numRefreshed);
     }
 
     private async Task<bool> RefreshPersonAsync(Guid personId, CancellationToken cancellationToken)
diff --git a/MediaBrowser.Providers/Manager/MetadataService.cs b/MediaBrowser.Providers/Manager/MetadataService.cs
index 26dc8f9930..fe5285bf65 100644
--- a/MediaBrowser.Providers/Manager/MetadataService.cs
+++ b/MediaBrowser.Providers/Manager/MetadataService.cs
@@ -212,22 +212,30 @@ namespace MediaBrowser.Providers.Manager
             var attemptedFetch = refreshOptions.MetadataRefreshMode > MetadataRefreshMode.ValidationOnly
                 || refreshOptions.ImageRefreshMode > MetadataRefreshMode.ValidationOnly;
 
+            var refreshStampNeedsSaving = false;
+
             if (hasRefreshedMetadata && hasRefreshedImages && attemptedFetch)
             {
                 item.DateLastRefreshed = DateTime.UtcNow;
                 updateType |= item.OnMetadataChanged();
+
+                // A full refresh queries every provider whether or not anything looks stale. When they all
+                // come back empty the stamp is the only thing that changed, and without it nothing records
+                // that the lookup happened, so the next pass repeats the same fruitless queries forever.
+                refreshStampNeedsSaving = refreshOptions.MetadataRefreshMode == MetadataRefreshMode.FullRefresh
+                    || refreshOptions.ImageRefreshMode == MetadataRefreshMode.FullRefresh;
             }
 
-            updateType = await SaveInternal(item, refreshOptions, updateType, isFirstRefresh, requiresRefresh, metadataResult, cancellationToken).ConfigureAwait(false);
+            updateType = await SaveInternal(item, refreshOptions, updateType, isFirstRefresh, requiresRefresh, refreshStampNeedsSaving, metadataResult, cancellationToken).ConfigureAwait(false);
 
             await AfterMetadataRefresh(itemOfType, refreshOptions, cancellationToken).ConfigureAwait(false);
 
             return updateType;
 
-            async Task<ItemUpdateType> SaveInternal(BaseItem item, MetadataRefreshOptions refreshOptions, ItemUpdateType updateType, bool isFirstRefresh, bool requiresRefresh, MetadataResult<TItemType> metadataResult, CancellationToken cancellationToken)
+            async Task<ItemUpdateType> SaveInternal(BaseItem item, MetadataRefreshOptions refreshOptions, ItemUpdateType updateType, bool isFirstRefresh, bool requiresRefresh, bool refreshStampNeedsSaving, MetadataResult<TItemType> metadataResult, CancellationToken cancellationToken)
             {
                 // Save if changes were made, or it's never been saved before
-                if (refreshOptions.ForceSave || updateType > ItemUpdateType.None || isFirstRefresh || refreshOptions.ReplaceAllMetadata || requiresRefresh)
+                if (refreshOptions.ForceSave || updateType > ItemUpdateType.None || isFirstRefresh || refreshOptions.ReplaceAllMetadata || requiresRefresh || refreshStampNeedsSaving)
                 {
                     if (item.IsFileProtocol)
                     {
diff --git a/tests/Jellyfin.Providers.Tests/Manager/MetadataServiceRefreshTests.cs b/tests/Jellyfin.Providers.Tests/Manager/MetadataServiceRefreshTests.cs
index 1d2fb2e760..465a032328 100644
--- a/tests/Jellyfin.Providers.Tests/Manager/MetadataServiceRefreshTests.cs
+++ b/tests/Jellyfin.Providers.Tests/Manager/MetadataServiceRefreshTests.cs
@@ -4,6 +4,7 @@ using System.Net.Http;
 using System.Threading;
 using System.Threading.Tasks;
 using Jellyfin.Data.Enums;
+using MediaBrowser.Controller;
 using MediaBrowser.Controller.Configuration;
 using MediaBrowser.Controller.Entities;
 using MediaBrowser.Controller.Entities.Movies;
@@ -11,8 +12,10 @@ using MediaBrowser.Controller.IO;
 using MediaBrowser.Controller.Library;
 using MediaBrowser.Controller.Persistence;
 using MediaBrowser.Controller.Providers;
+using MediaBrowser.Model.Configuration;
 using MediaBrowser.Model.Entities;
 using MediaBrowser.Model.IO;
+using MediaBrowser.Model.MediaInfo;
 using MediaBrowser.Providers.Manager;
 using Microsoft.Extensions.Logging.Abstractions;
 using Moq;
@@ -228,6 +231,100 @@ namespace Jellyfin.Providers.Tests.Manager
             Assert.Equal("nm0000123", mergedPerson.GetProviderId(MetadataProvider.Imdb));
         }
 
+        [Theory]
+        [InlineData(MetadataRefreshMode.FullRefresh, true)]
+        [InlineData(MetadataRefreshMode.Default, false)]
+        public async Task RefreshMetadata_ProvidersFoundNothing_PersistsRefreshDateOnFullRefresh(MetadataRefreshMode mode, bool expectSaved)
+        {
+            var peoplePath = System.IO.Path.Combine(System.IO.Path.GetTempPath(), "people");
+
+            var item = new Person
+            {
+                Id = Guid.NewGuid(),
+                Name = "Test Person",
+                Path = System.IO.Path.Combine(peoplePath, "T", "Test Person"),
+                PreferredMetadataLanguage = "en",
+                PreferredMetadataCountryCode = "US",
+                DateLastRefreshed = DateTime.UtcNow.AddDays(-60),
+                DateLastSaved = DateTime.UtcNow.AddDays(-60)
+            };
+            item.PresentationUniqueKey = item.CreatePresentationUniqueKey();
+
+            var stampBefore = item.DateLastRefreshed;
+
+            var provider = new Mock<IRemoteMetadataProvider<Person, PersonLookupInfo>>(MockBehavior.Loose);
+            provider.Setup(p => p.Name).Returns("Provider");
+            provider.Setup(p => p.GetMetadata(It.IsAny<PersonLookupInfo>(), It.IsAny<CancellationToken>()))
+                .ReturnsAsync(new MetadataResult<Person> { HasMetadata = false });
+
+            var libraryOptions = new LibraryOptions();
+
+            var libraryManager = new Mock<ILibraryManager>(MockBehavior.Loose);
+            libraryManager.Setup(l => l.GetLibraryOptions(It.IsAny<BaseItem>())).Returns(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<Person>(It.IsAny<BaseItem>(), It.IsAny<LibraryOptions>()))
+                .Returns(new[] { (IMetadataProvider<Person>)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 applicationPaths = new Mock<IServerApplicationPaths>(MockBehavior.Loose);
+            applicationPaths.Setup(a => a.PeoplePath).Returns(peoplePath);
+            var configurationManager = new Mock<IServerConfigurationManager>(MockBehavior.Loose);
+            configurationManager.Setup(c => c.ApplicationPaths).Returns(applicationPaths.Object);
+            configurationManager.Setup(c => c.Configuration).Returns(new ServerConfiguration());
+
+            var fileSystem = new Mock<IFileSystem>(MockBehavior.Loose);
+            fileSystem.Setup(f => f.GetFileSystemInfo(It.IsAny<string>())).Returns(new FileSystemMetadata { Exists = false });
+            fileSystem.Setup(f => f.GetValidFilename(It.IsAny<string>())).Returns<string>(name => name);
+
+            var mediaSourceManager = new Mock<IMediaSourceManager>(MockBehavior.Loose);
+            mediaSourceManager.Setup(m => m.GetPathProtocol(It.IsAny<string>())).Returns(MediaProtocol.File);
+
+            var previousLibraryManager = BaseItem.LibraryManager;
+            var previousConfigurationManager = BaseItem.ConfigurationManager;
+            var previousFileSystem = BaseItem.FileSystem;
+            var previousMediaSourceManager = BaseItem.MediaSourceManager;
+            BaseItem.LibraryManager = libraryManager.Object;
+            BaseItem.ConfigurationManager = configurationManager.Object;
+            BaseItem.FileSystem = fileSystem.Object;
+            BaseItem.MediaSourceManager = mediaSourceManager.Object;
+            try
+            {
+                var service = new TestPersonMetadataService(libraryManager.Object, providerManager.Object, itemRepository.Object, fileSystem.Object);
+
+                await service.RefreshMetadata(
+                    item,
+                    new MetadataRefreshOptions(Mock.Of<IDirectoryService>())
+                    {
+                        MetadataRefreshMode = mode,
+                        ImageRefreshMode = mode
+                    },
+                    CancellationToken.None).ConfigureAwait(true);
+            }
+            finally
+            {
+                BaseItem.LibraryManager = previousLibraryManager;
+                BaseItem.ConfigurationManager = previousConfigurationManager;
+                BaseItem.FileSystem = previousFileSystem;
+                BaseItem.MediaSourceManager = previousMediaSourceManager;
+            }
+
+            libraryManager.Verify(
+                l => l.UpdateItemAsync(item, It.IsAny<BaseItem>(), It.IsAny<ItemUpdateType>(), It.IsAny<CancellationToken>()),
+                expectSaved ? Times.Once() : Times.Never());
+
+            if (expectSaved)
+            {
+                Assert.True(item.DateLastRefreshed > stampBefore);
+            }
+        }
+
         private sealed class TestMetadataService : MetadataService<Movie, MovieInfo>
         {
             public TestMetadataService()
@@ -249,5 +346,20 @@ namespace Jellyfin.Providers.Tests.Manager
                 ICollection<IMetadataProvider> providers)
                 => RefreshWithProviders(metadata, id, options, providers, ImageProvider, false, CancellationToken.None);
         }
+
+        private sealed class TestPersonMetadataService : MetadataService<Person, PersonLookupInfo>
+        {
+            public TestPersonMetadataService(ILibraryManager libraryManager, IProviderManager providerManager, IItemRepository itemRepository, IFileSystem fileSystem)
+                : base(
+                    Mock.Of<IServerConfigurationManager>(),
+                    NullLogger<MetadataService<Person, PersonLookupInfo>>.Instance,
+                    providerManager,
+                    fileSystem,
+                    libraryManager,
+                    Mock.Of<IExternalDataManager>(),
+                    itemRepository)
+            {
+            }
+        }
     }
 }