Find dead people and artists by id, not by name

6 files changed, 219 insertions, 15 deletions

diff --git a/Emby.Server.Implementations/Library/LibraryManager.cs b/Emby.Server.Implementations/Library/LibraryManager.cs
index 672a86eb8a..48b61b78a3 100644
--- a/Emby.Server.Implementations/Library/LibraryManager.cs
+++ b/Emby.Server.Implementations/Library/LibraryManager.cs
@@ -1204,6 +1204,12 @@ namespace Emby.Server.Implementations.Library
         }
 
         /// <inheritdoc />
+        public Guid GetPersonId(string name)
+        {
+            return GetItemByNameId<Person>(Person.GetPath(name));
+        }
+
+        /// <inheritdoc />
         public Person? GetPerson(string name)
         {
             var path = Person.GetPath(name);
diff --git a/Emby.Server.Implementations/Library/Validators/ArtistsValidator.cs b/Emby.Server.Implementations/Library/Validators/ArtistsValidator.cs
index fa7112eb90..690466be70 100644
--- a/Emby.Server.Implementations/Library/Validators/ArtistsValidator.cs
+++ b/Emby.Server.Implementations/Library/Validators/ArtistsValidator.cs
@@ -1,4 +1,5 @@
 using System;
+using System.Collections.Generic;
 using System.Globalization;
 using System.Linq;
 using System.Threading;
@@ -61,6 +62,9 @@ public class ArtistsValidator
         var count = names.Count;
         var refreshed = 0;
 
+        var liveIds = new HashSet<Guid>();
+        var unresolved = 0;
+
         foreach (var name in names)
         {
             try
@@ -73,13 +77,20 @@ public class ArtistsValidator
 
                 // Fall back to GetArtist if not found (creates new item if needed)
                 item ??= _libraryManager.GetArtist(name);
-                var isNew = !existingArtistIds.Contains(item.Id);
-                var neverRefreshed = item.DateLastRefreshed == default;
 
-                if (isNew || neverRefreshed)
+                // A name with no item is nothing to refresh, and nothing to keep alive either.
+                if (item is not null)
                 {
-                    await item.RefreshMetadata(cancellationToken).ConfigureAwait(false);
-                    refreshed++;
+                    liveIds.Add(item.Id);
+
+                    var isNew = !existingArtistIds.Contains(item.Id);
+                    var neverRefreshed = item.DateLastRefreshed == default;
+
+                    if (isNew || neverRefreshed)
+                    {
+                        await item.RefreshMetadata(cancellationToken).ConfigureAwait(false);
+                        refreshed++;
+                    }
                 }
             }
             catch (OperationCanceledException)
@@ -88,6 +99,7 @@ public class ArtistsValidator
             }
             catch (Exception ex)
             {
+                unresolved++;
                 _logger.LogError(ex, "Error refreshing {ArtistName}", name);
             }
 
@@ -101,13 +113,26 @@ public class ArtistsValidator
 
         _logger.LogInformation("Refreshed metadata for {RefreshedCount} new artists out of {TotalCount} total", refreshed, count);
 
+        // Every name that threw is a name whose artist is missing from the live set, and deleting against
+        // a live set with holes in it deletes artists the library still refers to. Leave the sweep to a
+        // run that got a clean read of them.
+        if (unresolved > 0)
+        {
+            _logger.LogWarning(
+                "Not removing dead artists: {Count} of {TotalCount} names could not be resolved this run",
+                unresolved,
+                count);
+
+            progress.Report(100);
+            return;
+        }
+
         var deadEntities = _libraryManager.GetItemList(new InternalItemsQuery
         {
             IncludeItemTypes = [BaseItemKind.MusicArtist],
-            IsDeadArtist = true,
             IsLocked = false
-        }).Cast<MusicArtist>()
-        .Where(item => item.IsAccessedByName)
+        }).OfType<MusicArtist>()
+        .Where(item => item.IsAccessedByName && !liveIds.Contains(item.Id))
         .ToList();
 
         foreach (var item in deadEntities)
diff --git a/Emby.Server.Implementations/Library/Validators/PeopleValidator.cs b/Emby.Server.Implementations/Library/Validators/PeopleValidator.cs
index 3c8806d549..7d53f40ce7 100644
--- a/Emby.Server.Implementations/Library/Validators/PeopleValidator.cs
+++ b/Emby.Server.Implementations/Library/Validators/PeopleValidator.cs
@@ -1,4 +1,5 @@
 using System;
+using System.Collections.Generic;
 using System.Globalization;
 using System.Linq;
 using System.Threading;
@@ -58,6 +59,8 @@ public class PeopleValidator
             IncludeItemTypes = [BaseItemKind.Person]
         }).ToHashSet();
 
+        var (newNames, deadIds) = PartitionCreditsByPersonId(names, _libraryManager.GetPersonId, existingPersonIds);
+
         var numComplete = 0;
         var count = names.Count;
         var refreshed = 0;
@@ -96,14 +99,18 @@ public class PeopleValidator
             progress.Report(percent);
         }
 
-        _logger.LogInformation("Refreshed metadata for {RefreshedCount} new people out of {TotalCount} total", refreshed, count);
+        _logger.LogInformation(
+            "Refreshed metadata for {RefreshedCount} people out of {TotalCount} total, {NewCount} of which had no item yet",
+            refreshed,
+            count,
+            newNames.Count);
 
-        var deadEntities = _libraryManager.GetItemList(new InternalItemsQuery
-        {
-            IncludeItemTypes = [BaseItemKind.Person],
-            IsDeadPerson = true,
-            IsLocked = false
-        });
+        // A person somebody locked is theirs, not ours, however little the library still credits them.
+        var deadEntities = deadIds
+            .Select(_libraryManager.GetItemById)
+            .OfType<Person>()
+            .Where(item => !item.IsLocked)
+            .ToList();
 
         foreach (var item in deadEntities)
         {
@@ -114,4 +121,39 @@ public class PeopleValidator
 
         progress.Report(100);
     }
+
+    /// <summary>
+    /// Splits the person items into the ones a credit still calls for and the ones nothing does.
+    /// </summary>
+    /// <param name="creditNames">Every name credited on an item, from the people table.</param>
+    /// <param name="getPersonId">Maps a credit name to the id its person item has.</param>
+    /// <param name="existingPersonIds">The ids of the person items that exist.</param>
+    /// <returns>The credits needing an item, and the ids of the items nothing credits.</returns>
+    internal static (List<string> NewNames, List<Guid> DeadIds) PartitionCreditsByPersonId(
+        IReadOnlyList<string> creditNames,
+        Func<string, Guid> getPersonId,
+        IReadOnlySet<Guid> existingPersonIds)
+    {
+        ArgumentNullException.ThrowIfNull(creditNames);
+        ArgumentNullException.ThrowIfNull(getPersonId);
+        ArgumentNullException.ThrowIfNull(existingPersonIds);
+
+        var newNames = new List<string>();
+        var liveIds = new HashSet<Guid>();
+
+        foreach (var name in creditNames)
+        {
+            var personId = getPersonId(name);
+
+            // Distinct credit names can normalize onto one id; only the first of them needs an item.
+            if (liveIds.Add(personId) && !existingPersonIds.Contains(personId))
+            {
+                newNames.Add(name);
+            }
+        }
+
+        var deadIds = existingPersonIds.Where(id => !liveIds.Contains(id)).ToList();
+
+        return (newNames, deadIds);
+    }
 }
diff --git a/MediaBrowser.Controller/Entities/InternalItemsQuery.cs b/MediaBrowser.Controller/Entities/InternalItemsQuery.cs
index 0e5a5047cd..eb2a3676ac 100644
--- a/MediaBrowser.Controller/Entities/InternalItemsQuery.cs
+++ b/MediaBrowser.Controller/Entities/InternalItemsQuery.cs
@@ -432,12 +432,18 @@ namespace MediaBrowser.Controller.Entities
 
         public string? HasNoSubtitleTrackWithLanguage { get; set; }
 
+        /// <summary>
+        /// Gets or sets a value indicating whether to return only items nothing names any more.
+        /// </summary>
         public bool? IsDeadArtist { get; set; }
 
         public bool? IsDeadStudio { get; set; }
 
         public bool? IsDeadGenre { get; set; }
 
+        /// <summary>
+        /// Gets or sets a value indicating whether to return only items nothing names any more.
+        /// </summary>
         public bool? IsDeadPerson { get; set; }
 
         /// <summary>
diff --git a/MediaBrowser.Controller/Library/ILibraryManager.cs b/MediaBrowser.Controller/Library/ILibraryManager.cs
index 71b3f054ff..9028b0d6b8 100644
--- a/MediaBrowser.Controller/Library/ILibraryManager.cs
+++ b/MediaBrowser.Controller/Library/ILibraryManager.cs
@@ -707,6 +707,14 @@ namespace MediaBrowser.Controller.Library
         /// <returns><c>true</c> if ignored, <c>false</c> otherwise.</returns>
         bool IgnoreFile(FileSystemMetadata file, BaseItem parent);
 
+        /// <summary>
+        /// Gets the id a <see cref="Person"/> item for the name would have, without looking it up
+        /// or creating it.
+        /// </summary>
+        /// <param name="name">The name of the person.</param>
+        /// <returns>The item id for the name.</returns>
+        Guid GetPersonId(string name);
+
         Guid GetStudioId(string name);
 
         Guid GetGenreId(string name);
diff --git a/tests/Jellyfin.Server.Implementations.Tests/Library/PeopleValidatorPartitionTests.cs b/tests/Jellyfin.Server.Implementations.Tests/Library/PeopleValidatorPartitionTests.cs
new file mode 100644
index 0000000000..30f7bed208
--- /dev/null
+++ b/tests/Jellyfin.Server.Implementations.Tests/Library/PeopleValidatorPartitionTests.cs
@@ -0,0 +1,117 @@
+using System;
+using System.Collections.Generic;
+using Emby.Server.Implementations.Library.Validators;
+using Xunit;
+
+namespace Jellyfin.Server.Implementations.Tests.Library;
+
+/// <summary>
+/// Tests for how the people validator decides which credits need a person item and which person items
+/// nothing credits any more. Keying either half on the item's name rather than its id put the two halves
+/// in a loop that created, refreshed and deleted the same people on every run, so these pin the id.
+/// </summary>
+public class PeopleValidatorPartitionTests
+{
+    // Stands in for the real item-by-name id: derived from the credit name, case-insensitively, and
+    // from nothing else. The property that matters is that it does not depend on the item's own name.
+    private static Guid PersonId(string creditName)
+    {
+#pragma warning disable CA5351 // Do Not Use Broken Cryptographic Algorithms
+        var hash = System.Security.Cryptography.MD5.HashData(
+            System.Text.Encoding.Unicode.GetBytes(creditName.ToLowerInvariant()));
+#pragma warning restore CA5351 // Do Not Use Broken Cryptographic Algorithms
+        return new Guid(hash);
+    }
+
+    [Fact]
+    public void PartitionCreditsByPersonId_ProviderRenamedThePerson_KeepsThemAndCreatesNothing()
+    {
+        // The credit still says "AURORA"; the item it made has been renamed to "Aurora" by the provider
+        // that refreshed it. Nothing about the library changed, so nothing should be created or deleted.
+        var credits = new[] { "AURORA" };
+        var existing = new HashSet<Guid> { PersonId("AURORA") };
+
+        var (newNames, deadIds) = PeopleValidator.PartitionCreditsByPersonId(credits, PersonId, existing);
+
+        Assert.Empty(newNames);
+        Assert.Empty(deadIds);
+    }
+
+    [Theory]
+    // Every shape of rename seen in the wild on a real library.
+    [InlineData("AURORA")]
+    [InlineData("Amir AboulEla")]
+    [InlineData("Miguel Ángel Fuentes")]
+    [InlineData("a‐ha")]
+    [InlineData("윤현민")]
+    public void PartitionCreditsByPersonId_CreditWithAnItem_IsNeverBothCreatedAndDeleted(string creditName)
+    {
+        var existing = new HashSet<Guid> { PersonId(creditName) };
+
+        var (newNames, deadIds) = PeopleValidator.PartitionCreditsByPersonId([creditName], PersonId, existing);
+
+        Assert.Empty(newNames);
+        Assert.Empty(deadIds);
+    }
+
+    [Fact]
+    public void PartitionCreditsByPersonId_CreditWithNoItem_IsCreated()
+    {
+        var (newNames, deadIds) = PeopleValidator.PartitionCreditsByPersonId(
+            ["Wanted Person"],
+            PersonId,
+            new HashSet<Guid>());
+
+        Assert.Equal(["Wanted Person"], newNames);
+        Assert.Empty(deadIds);
+    }
+
+    [Fact]
+    public void PartitionCreditsByPersonId_ItemNoCreditNames_IsDead()
+    {
+        var orphan = PersonId("Nobody Credits Me");
+        var existing = new HashSet<Guid> { PersonId("Credited"), orphan };
+
+        var (newNames, deadIds) = PeopleValidator.PartitionCreditsByPersonId(["Credited"], PersonId, existing);
+
+        Assert.Empty(newNames);
+        Assert.Equal([orphan], deadIds);
+    }
+
+    [Fact]
+    public void PartitionCreditsByPersonId_CreditsNormalizingOntoOneId_CreateOneItem()
+    {
+        // "AURORA" and "Aurora" are one person as far as the item-by-name id is concerned, so exactly
+        // one of them should create the item and neither should end up dead.
+        var (newNames, deadIds) = PeopleValidator.PartitionCreditsByPersonId(
+            ["AURORA", "Aurora", "aurora"],
+            PersonId,
+            new HashSet<Guid>());
+
+        Assert.Single(newNames);
+        Assert.Empty(deadIds);
+    }
+
+    [Fact]
+    public void PartitionCreditsByPersonId_SecondRunAfterCreating_AsksForNothingFurther()
+    {
+        // The churn showed up as a run that never settled, so drive two rounds: whatever round one
+        // created must leave round two with nothing to do.
+        string[] credits = ["AURORA", "Amir AboulEla", "Miguel Ángel Fuentes"];
+        var existing = new HashSet<Guid>();
+
+        var (firstNames, firstDead) = PeopleValidator.PartitionCreditsByPersonId(credits, PersonId, existing);
+        Assert.Equal(3, firstNames.Count);
+        Assert.Empty(firstDead);
+
+        foreach (var created in firstNames)
+        {
+            existing.Add(PersonId(created));
+        }
+
+        var (secondNames, secondDead) = PeopleValidator.PartitionCreditsByPersonId(credits, PersonId, existing);
+
+        Assert.Empty(secondNames);
+        Assert.Empty(secondDead);
+    }
+}