Merge pull request #17715 from Shadowghost/fix-people-cleanup

Delete credits nothing maps to and bound item-by-name folder names

14 files changed, 231 insertions, 25 deletions

diff --git a/Emby.Server.Implementations/Library/LibraryManager.cs b/Emby.Server.Implementations/Library/LibraryManager.cs
index 6c5ff6e8c7..dd8c883684 100644
--- a/Emby.Server.Implementations/Library/LibraryManager.cs
+++ b/Emby.Server.Implementations/Library/LibraryManager.cs
@@ -3581,6 +3581,12 @@ namespace Emby.Server.Implementations.Library
         }
 
         /// <inheritdoc/>
+        public int DeleteOrphanedCredits()
+        {
+            return _peopleRepository.DeleteOrphanedCredits();
+        }
+
+        /// <inheritdoc/>
         public IReadOnlyDictionary<Guid, IReadOnlyList<string>> GetPeopleNamesByItems(IReadOnlyList<Guid> itemIds, IReadOnlyList<string> personTypes)
         {
             return _peopleRepository.GetPeopleNamesByItems(itemIds, personTypes);
diff --git a/Emby.Server.Implementations/Library/Validators/PeopleValidator.cs b/Emby.Server.Implementations/Library/Validators/PeopleValidator.cs
index dacef102dd..078a0b921d 100644
--- a/Emby.Server.Implementations/Library/Validators/PeopleValidator.cs
+++ b/Emby.Server.Implementations/Library/Validators/PeopleValidator.cs
@@ -49,6 +49,14 @@ public class PeopleValidator
     /// <returns>Task.</returns>
     public async Task ValidatePeople(CancellationToken cancellationToken, IProgress<double> progress)
     {
+        // Before the refresh below walks them: a credit no item maps to any more stands for nothing,
+        // and while it is there the person it names cannot reach the dead-person sweep either.
+        var numOrphaned = _libraryManager.DeleteOrphanedCredits();
+        if (numOrphaned > 0)
+        {
+            _logger.LogDebug("Deleted {Amount} credits no item maps to", numOrphaned);
+        }
+
         var people = _libraryManager.GetPeopleNames(new InternalPeopleQuery());
 
         var numComplete = 0;
@@ -115,6 +123,6 @@ public class PeopleValidator
 
         progress.Report(100);
 
-        _logger.LogInformation("People validation complete");
+        _logger.LogInformation("People validation complete, deleted {Orphaned} orphaned credits", numOrphaned);
     }
 }
diff --git a/Jellyfin.Server.Implementations/Item/PeopleRepository.cs b/Jellyfin.Server.Implementations/Item/PeopleRepository.cs
index aaa363b046..da2ad033ec 100644
--- a/Jellyfin.Server.Implementations/Item/PeopleRepository.cs
+++ b/Jellyfin.Server.Implementations/Item/PeopleRepository.cs
@@ -194,13 +194,45 @@ public class PeopleRepository(IDbContextFactory<JellyfinDbContext> dbProvider, I
             listOrder++;
         }
 
+        var droppedCredits = existingMaps.Select(e => e.PeopleId).Distinct().ToArray();
         context.PeopleBaseItemMap.RemoveRange(existingMaps);
 
         context.SaveChanges();
+
+        // Nothing else ever deletes a credit row, so one left without a single mapping outlives the
+        // credit it stood for: it keeps a person of that name off the dead-person sweep, which only
+        // sees items no credit names, and keeps the name in every by-name list. That is how a credit
+        // a provider dropped, or one a broken provider result invented, becomes impossible to clean up.
+        DeleteCreditsWithoutMapping(context, droppedCredits);
+
+        context.SaveChanges();
         transaction.Commit();
     }
 
     /// <inheritdoc/>
+    public int DeleteOrphanedCredits()
+    {
+        using var context = _dbProvider.CreateDbContext();
+
+        return DeleteCreditsWithoutMapping(context, null);
+    }
+
+    // A null candidate list sweeps every credit, anything else only the ones just unmapped.
+    private int DeleteCreditsWithoutMapping(JellyfinDbContext context, IReadOnlyList<Guid>? candidates)
+    {
+        if (candidates is not null && candidates.Count == 0)
+        {
+            return 0;
+        }
+
+        var credits = candidates is null
+            ? context.Peoples.AsQueryable()
+            : context.Peoples.WhereOneOrMany(candidates, e => e.Id);
+
+        return credits.Where(e => !context.PeopleBaseItemMap.Any(f => f.PeopleId == e.Id)).ExecuteDelete();
+    }
+
+    /// <inheritdoc/>
     public IReadOnlyDictionary<Guid, IReadOnlyList<string>> GetPeopleNamesByItems(IReadOnlyList<Guid> itemIds, IReadOnlyList<string> personTypes)
     {
         using var context = _dbProvider.CreateDbContext();
diff --git a/MediaBrowser.Controller/Entities/Audio/MusicArtist.cs b/MediaBrowser.Controller/Entities/Audio/MusicArtist.cs
index c25694aba5..1e2d94d2a4 100644
--- a/MediaBrowser.Controller/Entities/Audio/MusicArtist.cs
+++ b/MediaBrowser.Controller/Entities/Audio/MusicArtist.cs
@@ -173,10 +173,7 @@ namespace MediaBrowser.Controller.Entities.Audio
 
         public static string GetPath(string name, bool normalizeName)
         {
-            // Trim the period at the end because windows will have a hard time with that
-            var validName = normalizeName ?
-                FileSystem.GetValidFilename(name).Trim().TrimEnd('.') :
-                name;
+            var validName = normalizeName ? GetItemByNameFolderName(name) : name;
 
             return System.IO.Path.Combine(ConfigurationManager.ApplicationPaths.ArtistsPath, validName);
         }
diff --git a/MediaBrowser.Controller/Entities/Audio/MusicGenre.cs b/MediaBrowser.Controller/Entities/Audio/MusicGenre.cs
index 65669e6804..23b3341dbc 100644
--- a/MediaBrowser.Controller/Entities/Audio/MusicGenre.cs
+++ b/MediaBrowser.Controller/Entities/Audio/MusicGenre.cs
@@ -80,10 +80,7 @@ namespace MediaBrowser.Controller.Entities.Audio
 
         public static string GetPath(string name, bool normalizeName)
         {
-            // Trim the period at the end because windows will have a hard time with that
-            var validName = normalizeName ?
-                FileSystem.GetValidFilename(name).Trim().TrimEnd('.') :
-                name;
+            var validName = normalizeName ? GetItemByNameFolderName(name) : name;
 
             return System.IO.Path.Combine(ConfigurationManager.ApplicationPaths.MusicGenrePath, validName);
         }
diff --git a/MediaBrowser.Controller/Entities/BaseItem.cs b/MediaBrowser.Controller/Entities/BaseItem.cs
index 28f40cb7fa..d030c8f420 100644
--- a/MediaBrowser.Controller/Entities/BaseItem.cs
+++ b/MediaBrowser.Controller/Entities/BaseItem.cs
@@ -48,6 +48,10 @@ namespace MediaBrowser.Controller.Entities
 
         public const string ThemeSongFileName = "theme";
 
+        // Well below the 255 byte limit of the common Linux filesystems and the 255 character limit
+        // of Windows, so the files inside the folder still fit within MAX_PATH.
+        private const int MaxItemByNameFolderNameBytes = 128;
+
         /// <summary>
         /// The supported image extensions.
         /// </summary>
@@ -942,6 +946,43 @@ namespace MediaBrowser.Controller.Entities
         }
 
         /// <summary>
+        /// Turns an item-by-name entity's name into a folder name every supported filesystem accepts.
+        /// </summary>
+        /// <param name="name">The entity's name.</param>
+        /// <returns>The folder name.</returns>
+        public static string GetItemByNameFolderName(string name)
+        {
+            // Trim the period at the end because windows will have a hard time with that
+            var validName = FileSystem.GetValidFilename(name).Trim().TrimEnd('.');
+
+            // Most Linux filesystems cap a path component at 255 bytes, so a name past that cannot be
+            // turned into a folder at all - and an entity with no folder can never be created, which
+            // leaves the credit behind it stuck: not refreshable, not deletable, retried on every scan.
+            // Only broken provider data gets this long, but it still has to resolve to something, so
+            // keep a readable prefix and let a hash of the whole name tell two of them apart.
+            if (Encoding.UTF8.GetByteCount(validName) <= MaxItemByNameFolderNameBytes)
+            {
+                return validName;
+            }
+
+            var suffix = "-" + validName.GetMD5().ToString("N", CultureInfo.InvariantCulture);
+            var budget = MaxItemByNameFolderNameBytes - suffix.Length;
+            var length = Math.Min(validName.Length, budget);
+            while (length > 0 && Encoding.UTF8.GetByteCount(validName.AsSpan(0, length)) > budget)
+            {
+                length--;
+            }
+
+            // Never cut a surrogate pair in half, the lone half is not a valid file name character.
+            if (length > 0 && char.IsHighSurrogate(validName[length - 1]))
+            {
+                length--;
+            }
+
+            return string.Concat(validName.AsSpan(0, length).TrimEnd().TrimEnd('.'), suffix);
+        }
+
+        /// <summary>
         /// Cleans a raw name into its sortable form by applying the configured sort rules.
         /// </summary>
         /// <param name="name">The raw name to clean.</param>
diff --git a/MediaBrowser.Controller/Entities/Genre.cs b/MediaBrowser.Controller/Entities/Genre.cs
index 6ec78a270e..ef8acaef92 100644
--- a/MediaBrowser.Controller/Entities/Genre.cs
+++ b/MediaBrowser.Controller/Entities/Genre.cs
@@ -83,10 +83,7 @@ namespace MediaBrowser.Controller.Entities
 
         public static string GetPath(string name, bool normalizeName)
         {
-            // Trim the period at the end because windows will have a hard time with that
-            var validName = normalizeName ?
-                FileSystem.GetValidFilename(name).Trim().TrimEnd('.') :
-                name;
+            var validName = normalizeName ? GetItemByNameFolderName(name) : name;
 
             return System.IO.Path.Combine(ConfigurationManager.ApplicationPaths.GenrePath, validName);
         }
diff --git a/MediaBrowser.Controller/Entities/Person.cs b/MediaBrowser.Controller/Entities/Person.cs
index 14325d971a..bba5005eed 100644
--- a/MediaBrowser.Controller/Entities/Person.cs
+++ b/MediaBrowser.Controller/Entities/Person.cs
@@ -98,10 +98,7 @@ namespace MediaBrowser.Controller.Entities
 
         public static string GetPath(string name, bool normalizeName)
         {
-            // Trim the period at the end because windows will have a hard time with that
-            var validFilename = normalizeName ?
-                FileSystem.GetValidFilename(name).Trim().TrimEnd('.') :
-                name;
+            var validFilename = normalizeName ? GetItemByNameFolderName(name) : name;
 
             string subFolderPrefix = null;
 
diff --git a/MediaBrowser.Controller/Entities/Studio.cs b/MediaBrowser.Controller/Entities/Studio.cs
index 9103b09a95..a944b356c8 100644
--- a/MediaBrowser.Controller/Entities/Studio.cs
+++ b/MediaBrowser.Controller/Entities/Studio.cs
@@ -78,10 +78,7 @@ namespace MediaBrowser.Controller.Entities
 
         public static string GetPath(string name, bool normalizeName)
         {
-            // Trim the period at the end because windows will have a hard time with that
-            var validName = normalizeName ?
-                FileSystem.GetValidFilename(name).Trim().TrimEnd('.') :
-                name;
+            var validName = normalizeName ? GetItemByNameFolderName(name) : name;
 
             return System.IO.Path.Combine(ConfigurationManager.ApplicationPaths.StudioPath, validName);
         }
diff --git a/MediaBrowser.Controller/Entities/Year.cs b/MediaBrowser.Controller/Entities/Year.cs
index 37820296cc..03fb2156d3 100644
--- a/MediaBrowser.Controller/Entities/Year.cs
+++ b/MediaBrowser.Controller/Entities/Year.cs
@@ -85,10 +85,7 @@ namespace MediaBrowser.Controller.Entities
 
         public static string GetPath(string name, bool normalizeName)
         {
-            // Trim the period at the end because windows will have a hard time with that
-            var validName = normalizeName ?
-                FileSystem.GetValidFilename(name).Trim().TrimEnd('.') :
-                name;
+            var validName = normalizeName ? GetItemByNameFolderName(name) : name;
 
             return System.IO.Path.Combine(ConfigurationManager.ApplicationPaths.YearPath, validName);
         }
diff --git a/MediaBrowser.Controller/Library/ILibraryManager.cs b/MediaBrowser.Controller/Library/ILibraryManager.cs
index ca686fbd9d..2a6ea214b8 100644
--- a/MediaBrowser.Controller/Library/ILibraryManager.cs
+++ b/MediaBrowser.Controller/Library/ILibraryManager.cs
@@ -606,6 +606,12 @@ namespace MediaBrowser.Controller.Library
         IReadOnlyList<string> GetPeopleNames(InternalPeopleQuery query);
 
         /// <summary>
+        /// Deletes every credit that no item maps to any more.
+        /// </summary>
+        /// <returns>The number of credits that were deleted.</returns>
+        int DeleteOrphanedCredits();
+
+        /// <summary>
         /// Gets the distinct people names per item for multiple items.
         /// </summary>
         /// <param name="itemIds">The item IDs.</param>
diff --git a/MediaBrowser.Controller/Persistence/IPeopleRepository.cs b/MediaBrowser.Controller/Persistence/IPeopleRepository.cs
index 9811241d31..15183a8806 100644
--- a/MediaBrowser.Controller/Persistence/IPeopleRepository.cs
+++ b/MediaBrowser.Controller/Persistence/IPeopleRepository.cs
@@ -34,6 +34,12 @@ public interface IPeopleRepository
     IReadOnlyList<string> GetPeopleNames(InternalPeopleQuery filter);
 
     /// <summary>
+    /// Deletes every credit that no item maps to any more.
+    /// </summary>
+    /// <returns>The number of credits that were deleted.</returns>
+    int DeleteOrphanedCredits();
+
+    /// <summary>
     /// Gets the distinct people names per item for multiple items efficiently by querying from the mapping table.
     /// </summary>
     /// <param name="itemIds">The item IDs to get people for.</param>
diff --git a/tests/Jellyfin.Controller.Tests/Entities/BaseItemTests.cs b/tests/Jellyfin.Controller.Tests/Entities/BaseItemTests.cs
index e34eb0bda3..86bac4256a 100644
--- a/tests/Jellyfin.Controller.Tests/Entities/BaseItemTests.cs
+++ b/tests/Jellyfin.Controller.Tests/Entities/BaseItemTests.cs
@@ -3,6 +3,7 @@ using System.Collections.Generic;
 using System.IO;
 using System.Linq;
 using System.Reflection;
+using System.Text;
 using System.Threading;
 using System.Threading.Tasks;
 using Jellyfin.Database.Implementations.Entities;
@@ -27,6 +28,57 @@ namespace Jellyfin.Controller.Tests.Entities;
 
 public class BaseItemTests
 {
+    [Fact]
+    public void GetItemByNameFolderName_ShortName_IsKeptAsIs()
+    {
+        SetupPassThroughFileSystem();
+
+        Assert.Equal("Mairghread Scott", BaseItem.GetItemByNameFolderName("Mairghread Scott."));
+    }
+
+    [Fact]
+    public void GetItemByNameFolderName_OverlongName_FitsInAPathComponent()
+    {
+        SetupPassThroughFileSystem();
+
+        // What a provider result that concatenated a whole credit list into one name looks like.
+        var name = string.Join(", ", Enumerable.Repeat("Jerry Siegel (created by: Superman)", 20));
+
+        var folderName = BaseItem.GetItemByNameFolderName(name);
+
+        Assert.True(Encoding.UTF8.GetByteCount(folderName) <= 128);
+        Assert.StartsWith("Jerry Siegel (created by: Superman)", folderName, StringComparison.Ordinal);
+    }
+
+    [Fact]
+    public void GetItemByNameFolderName_OverlongNamesSharingAPrefix_StayApart()
+    {
+        SetupPassThroughFileSystem();
+
+        var prefix = new string('a', 200);
+
+        Assert.NotEqual(
+            BaseItem.GetItemByNameFolderName(prefix + "Joe Shuster"),
+            BaseItem.GetItemByNameFolderName(prefix + "Bob Kane"));
+    }
+
+    [Fact]
+    public void GetItemByNameFolderName_OverlongName_IsStable()
+    {
+        SetupPassThroughFileSystem();
+
+        var name = new string('a', 300);
+
+        Assert.Equal(BaseItem.GetItemByNameFolderName(name), BaseItem.GetItemByNameFolderName(name));
+    }
+
+    private static void SetupPassThroughFileSystem()
+    {
+        var fileSystem = new Mock<IFileSystem>();
+        fileSystem.Setup(x => x.GetValidFilename(It.IsAny<string>())).Returns((string name) => name);
+        BaseItem.FileSystem = fileSystem.Object;
+    }
+
     [Theory]
     [InlineData("", "")]
     [InlineData("1", "0000000001")]
diff --git a/tests/Jellyfin.Server.Implementations.Tests/Item/PeopleRepositoryUpdatePeopleTests.cs b/tests/Jellyfin.Server.Implementations.Tests/Item/PeopleRepositoryUpdatePeopleTests.cs
index 54565c5787..649458f733 100644
--- a/tests/Jellyfin.Server.Implementations.Tests/Item/PeopleRepositoryUpdatePeopleTests.cs
+++ b/tests/Jellyfin.Server.Implementations.Tests/Item/PeopleRepositoryUpdatePeopleTests.cs
@@ -142,6 +142,79 @@ public sealed class PeopleRepositoryUpdatePeopleTests : SqliteDbTestFixture
         Assert.Equal("Hero", map.Role);
     }
 
+    [Fact]
+    public void UpdatePeople_CreditDroppedByTheProvider_LeavesNoCreditRowBehind()
+    {
+        _repository.UpdatePeople(_itemId, [
+            CreatePerson("Person A", PersonKind.Actor, "Hero"),
+            CreatePerson("Person B", PersonKind.Actor, "Villain")
+        ]);
+
+        _repository.UpdatePeople(_itemId, [CreatePerson("Person A", PersonKind.Actor, "Hero")]);
+
+        using var ctx = CreateDbContext();
+        Assert.Equal(["Person A"], ctx.Peoples.Select(e => e.Name).ToArray());
+    }
+
+    [Fact]
+    public void UpdatePeople_CreditStillHeldByAnotherItem_IsKept()
+    {
+        _repository.UpdatePeople(_itemId, [CreatePerson("Person A", PersonKind.Actor, "Hero")]);
+        _repository.UpdatePeople(AddMovie("Other Movie"), [CreatePerson("Person A", PersonKind.Actor, "Hero")]);
+
+        _repository.UpdatePeople(_itemId, []);
+
+        using var after = CreateDbContext();
+        Assert.Single(after.Peoples);
+        Assert.Single(after.PeopleBaseItemMap);
+    }
+
+    [Fact]
+    public void DeleteOrphanedCredits_CreditNoItemMapsTo_IsDeleted()
+    {
+        _repository.UpdatePeople(_itemId, [CreatePerson("Person A", PersonKind.Actor, "Hero")]);
+        using (var ctx = CreateDbContext())
+        {
+            // The state a credit was left in before UpdatePeople cleaned up after itself.
+            ctx.PeopleBaseItemMap.RemoveRange(ctx.PeopleBaseItemMap);
+            ctx.SaveChanges();
+        }
+
+        Assert.Equal(1, _repository.DeleteOrphanedCredits());
+
+        using var after = CreateDbContext();
+        Assert.Empty(after.Peoples);
+    }
+
+    [Fact]
+    public void DeleteOrphanedCredits_CreditAnItemMapsTo_IsKept()
+    {
+        _repository.UpdatePeople(_itemId, [CreatePerson("Person A", PersonKind.Actor, "Hero")]);
+
+        Assert.Equal(0, _repository.DeleteOrphanedCredits());
+
+        using var after = CreateDbContext();
+        Assert.Single(after.Peoples);
+    }
+
+    private Guid AddMovie(string name)
+    {
+        var id = Guid.NewGuid();
+        using var ctx = CreateDbContext();
+        ctx.BaseItems.Add(new BaseItemEntity
+        {
+            Id = id,
+            Type = new ItemTypeLookup().BaseItemKindNames[BaseItemKind.Movie],
+            Name = name,
+            MediaType = "Video",
+            IsMovie = true,
+            IsFolder = false,
+            IsVirtualItem = false
+        });
+        ctx.SaveChanges();
+        return id;
+    }
+
     private static PersonInfo CreatePerson(string name, PersonKind type, string role)
     {
         return new PersonInfo