fix(500): dedup incoming metadata collections so a duplicate name inserts once #602
No Assignees
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: timothy/ersatztv#602
Reference in New Issue
Block a user
Closes #500.
The remove-stale + add-new reconcile idiom materializes its add set with
.ToList()before the loop mutates the existing collection, so the add filter is evaluated against a snapshot. Two identically-named incoming entries whose name is not yet on the existing item therefore both passed and both inserted — a duplicate row.Dedup the incoming set on the same key the filter compares (
Name;Guidfor guids) in both copies of the idiom:PlexMovieLibraryScanner.UpdateMetadata(7 collections) andJellyfinMusicVideoLibraryScanner.Reconcile{Genres,Tags,Studios,Artists}.Plex
Actorsare the exception and get an artwork-preferring dedup hoisted out and shared with the remove filter, whose key is(Name, artwork-presence)— that filter is the mechanism that drops an artwork-less actor so the add loop can re-add it with artwork. A bareDistinctBy(a => a.Name)keeps the FIRST duplicate, so Plex listing the artwork-less copy first discarded the artwork; worse, the remove filter would still see the artwork-less duplicate, making its upgrade clause false, so the stale row was never removed and the artwork never arrived on any later scan either.Actoralso carriesRole/Order, which first-wins would silently drop. This was caught by the cold adversarial review of the first version of this branch — it was a new permanent data-loss path, not a pre-existing one.Tests: duplicate-collapse for both paths, both Actors cases, and a positive control proving distinct entries are still all added and stale ones still removed (without it, a mis-keyed dedup that collapsed genuinely different entries would pass every other assertion). Each proven non-vacuous by reverting the fix.
Scope / follow-up: the same idiom is copied into 8 further scanners and repositories that this PR deliberately does not touch (the issue scoped it to two paths). Filed as #600 so the class of bug is tracked rather than left silently in the majority of its instances; the dedup rule is recorded under
scan.musicvideo-reconciliation.Verification:
ErsatzTV.Scanner.Tests1488 pass,ErsatzTV.Tests1868 pass, format gate clean, decisions guard + catalog check clean.The remove-stale + add-new reconcile idiom materializes its add set with .ToList() BEFORE the loop mutates the existing collection, so the add filter (`incoming.All(x2 => x2.Name != x.Name)`) is evaluated against a snapshot. Two identically-named incoming entries whose name is not yet on the existing item therefore BOTH passed the filter and BOTH inserted — a duplicate row. Deduplicate the incoming set on the same key the filter compares (Name; Guid for Guids), in both copies of the idiom: - PlexMovieLibraryScanner.UpdateMetadata (the original) — genres, studios, actors, directors, writers, guids, tags. - JellyfinMusicVideoLibraryScanner.Reconcile{Genres,Tags,Studios,Artists} (added in #497, mirrors the Plex pattern verbatim). Plex ACTORS are the exception and get an artwork-preferring dedup hoisted out and shared with the remove filter, because that filter is keyed on (Name, artwork-presence) — it is the mechanism that drops an artwork-less actor so the add loop can re-add it WITH artwork. A bare DistinctBy(a => a.Name) there keeps the FIRST duplicate, so Plex listing the artwork-less copy first discarded the artwork; worse, the remove filter would still see the artwork-less duplicate, making its upgrade clause false, so the stale row was never removed and the artwork never arrived on ANY later scan either. Actor also carries Role/Order, which first-wins would silently drop too. Caught by the cold review of the first version of this commit. For the Jellyfin scanner the dedup sits at the incoming-list declaration, which also covers the remove filter — safe because all four of those filters only ask "is this name present at all", an answer duplicates cannot change. Tests: duplicate-collapse for both paths, the two Actors cases above, and a POSITIVE CONTROL proving distinct entries are still all added and stale ones still removed (without it, a mis-keyed dedup that collapsed genuinely different entries would pass every other assertion). Each proven non-vacuous. The Plex tests drive the protected UpdateMetadata through a minimal test-only subclass, as MediaServerMovieLibraryScannerTests already does. Low likelihood in practice (a media server emitting two identically-named genres for one item is unusual); this is defensive, with no observed occurrence. The same idiom is copied into 8 further scanners/repositories that this change deliberately does not touch (the issue scoped it to two paths) — filed as #600 so the class of bug is tracked rather than silently left in the majority of its instances. The dedup rule is recorded under scan.musicvideo-reconciliation. fixes #500Independent cold adversarial review, two rounds.
Round 1 (first version of this branch): BLOCKED — the reviewer caught that deduping Plex Actors by Name alone was a new permanent data-loss path, strictly worse than the duplicate row it replaced. Because Actors' remove filter is keyed on (Name, artwork-presence), first-wins could keep the artwork-less duplicate AND the undeduped remove filter kept its upgrade clause false, so the stale row was never replaced and the artwork never arrived on any later scan. Fixed by hoisting an artwork-preferring deduped list shared by both filters, plus two dedicated tests and the positive control the reviewer also asked for.
Round 2 (
2f2bcca, this head): MERGEABLE, no new blocking findings. The reviewer independently re-ran its own two-scan probe against this head and additionally asserted idempotency (RemoveActor 0 calls, AddActor exactly 1 across two scans — no re-delete/re-insert churn); audited the rewritten control flow and proved deduping the set feeding the remove filter can only flip All false->true, i.e. strictly more removals, each necessarily paired with a re-add since the add filter is materialized after the remove loop; confirmed non-duplicate input is bit-identical to pre-change (GroupBy preserves first-occurrence order); and verified non-vacuity by reverting the fix (both actor tests plus its own probe fail, while the duplicate-collapse and positive-control tests still pass). It also sabotaged the positive control's key to DistinctBy(g => 1) and confirmed it fails, so that control is genuinely load-bearing. Suites re-measured independently: Scanner.Tests 1488/1488, ErsatzTV.Tests 1868/1868, format gate clean.Accepted as out of scope, with the reviewer agreeing: two same-name actors who BOTH carry artwork cannot keep distinct Role/Order. The add filter's identity is Name-only, so this idiom structurally cannot hold two stable rows per name — keeping both IS the duplicate-row bug. Re-keying on (Name, Role) is #600-scale work. The resulting rule: one row per name, artwork-bearing wins, Plex order breaks remaining ties.
Reviewer note applied outside this diff: #600's table described JellyfinTelevisionLibraryScanner as show/season/episode; it is show + episode only (Jellyfin season reconcile lives in JellyfinTelevisionRepository, already its own row). Corrected in the issue.
Noted for #600: once the shared helper lands, this dedup rule stops being music-video-specific and should get its own decision key rather than growing scan.musicvideo-reconciliation's Mechanics.
Pre-existing, not introduced here: the actor upgrade path does RemoveActor then AddActor with no transaction, so a failure between them loses the actor until the next scan. The non-duplicate upgrade path already had this.
Review-verdict: MERGEABLE @
2f2bcca