From 792ce4a391c524f8e20f676be2d6cab07e5a59c4 Mon Sep 17 00:00:00 2001 From: Shadowghost Date: Tue, 1 Sep 2026 07:36:53 +0200 Subject: Fix people validator not creating missing people --- MediaBrowser.Controller/Library/ILibraryManager.cs | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) (limited to 'MediaBrowser.Controller/Library') diff --git a/MediaBrowser.Controller/Library/ILibraryManager.cs b/MediaBrowser.Controller/Library/ILibraryManager.cs index c8cca1fa93..71b3f054ff 100644 --- a/MediaBrowser.Controller/Library/ILibraryManager.cs +++ b/MediaBrowser.Controller/Library/ILibraryManager.cs @@ -106,6 +106,13 @@ namespace MediaBrowser.Controller.Library /// Task{Person}. Person? GetPerson(string name); + /// + /// Gets a Person, creating and persisting it if no item exists for the name yet. + /// + /// The name of the person. + /// The person. + Person GetOrCreatePerson(string name); + /// /// Finds the by path. /// @@ -152,15 +159,6 @@ namespace MediaBrowser.Controller.Library /// Throws if year is invalid. Year GetYear(int value); - /// - /// Validate and refresh the People sub-set of the IBN. - /// The items are stored in the db but not loaded into memory until actually requested by an operation. - /// - /// The progress. - /// The cancellation token. - /// Task. - Task ValidatePeopleAsync(IProgress progress, CancellationToken cancellationToken); - /// /// Reloads the root media folder. /// -- cgit v1.2.3 From 50f08d41a411254f67cf2305fd7bb838119a1bc1 Mon Sep 17 00:00:00 2001 From: Shadowghost Date: Tue, 1 Sep 2026 19:49:25 +0200 Subject: Find dead people and artists by id, not by name --- .../Library/LibraryManager.cs | 6 ++ .../Library/Validators/ArtistsValidator.cs | 41 ++++++-- .../Library/Validators/PeopleValidator.cs | 56 ++++++++-- .../Entities/InternalItemsQuery.cs | 6 ++ MediaBrowser.Controller/Library/ILibraryManager.cs | 8 ++ .../Library/PeopleValidatorPartitionTests.cs | 117 +++++++++++++++++++++ 6 files changed, 219 insertions(+), 15 deletions(-) create mode 100644 tests/Jellyfin.Server.Implementations.Tests/Library/PeopleValidatorPartitionTests.cs (limited to 'MediaBrowser.Controller/Library') 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 @@ -1203,6 +1203,12 @@ namespace Emby.Server.Implementations.Library .FirstOrDefault(); } + /// + public Guid GetPersonId(string name) + { + return GetItemByNameId(Person.GetPath(name)); + } + /// public Person? GetPerson(string 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(); + 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() - .Where(item => item.IsAccessedByName) + }).OfType() + .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() + .Where(item => !item.IsLocked) + .ToList(); foreach (var item in deadEntities) { @@ -114,4 +121,39 @@ public class PeopleValidator progress.Report(100); } + + /// + /// Splits the person items into the ones a credit still calls for and the ones nothing does. + /// + /// Every name credited on an item, from the people table. + /// Maps a credit name to the id its person item has. + /// The ids of the person items that exist. + /// The credits needing an item, and the ids of the items nothing credits. + internal static (List NewNames, List DeadIds) PartitionCreditsByPersonId( + IReadOnlyList creditNames, + Func getPersonId, + IReadOnlySet existingPersonIds) + { + ArgumentNullException.ThrowIfNull(creditNames); + ArgumentNullException.ThrowIfNull(getPersonId); + ArgumentNullException.ThrowIfNull(existingPersonIds); + + var newNames = new List(); + var liveIds = new HashSet(); + + 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; } + /// + /// Gets or sets a value indicating whether to return only items nothing names any more. + /// public bool? IsDeadArtist { get; set; } public bool? IsDeadStudio { get; set; } public bool? IsDeadGenre { get; set; } + /// + /// Gets or sets a value indicating whether to return only items nothing names any more. + /// public bool? IsDeadPerson { get; set; } /// 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 /// true if ignored, false otherwise. bool IgnoreFile(FileSystemMetadata file, BaseItem parent); + /// + /// Gets the id a item for the name would have, without looking it up + /// or creating it. + /// + /// The name of the person. + /// The item id for the name. + 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; + +/// +/// 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. +/// +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 { 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 { 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()); + + Assert.Equal(["Wanted Person"], newNames); + Assert.Empty(deadIds); + } + + [Fact] + public void PartitionCreditsByPersonId_ItemNoCreditNames_IsDead() + { + var orphan = PersonId("Nobody Credits Me"); + var existing = new HashSet { 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()); + + 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(); + + 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); + } +} -- cgit v1.2.3