From aa32fd78dd68bf476df6d0ff464e15fa2ce85e5d Mon Sep 17 00:00:00 2001 From: Timothy Date: Sun, 19 Jul 2026 23:28:27 +0200 Subject: [PATCH] fix(477): guard media-server library sweeps against successful-but-empty fetches MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A successful fetch returning zero items made existing.Except([]) flag the ENTIRE library FileNotFound in one scan — feeding EmptyTrashHandler's permanent delete and emptying every affected collection (dead channels). Add a shared MediaServerReconciliationGuard that skips (and logs a Warning) the sweep when incoming==0 while items exist, wired into the three library-level sweeps (Television shows / Movie / OtherVideo). An empty incoming set is indistinguishable at scan time from a mid-restore / emptied-upstream error (both report a zero total), so this deliberately overrides #476's degenerate "last item removed => empty incoming => flag" case. #476's cascade still fires for partial deletions (survivors present); its characterization test moves from an empty incoming to a survivor+removed partial-deletion case. Tests: policy table (MediaServerReconciliationGuardTests) + per-scanner integration proving the wiring (empty incoming + non-empty existing flags/reindexes nothing). Proven non-vacuous by neutralizing the guard. Nested TV season/episode sweeps left unguarded (bounded blast radius); ratio-threshold + projection-failure detection deferred to a follow-up. docs/decisions.md updated. Fixes #477 Co-Authored-By: Claude Opus 4.8 (1M context) --- .../MediaServerMovieLibraryScannerTests.cs | 110 ++++++++++++++++++ ...ediaServerOtherVideoLibraryScannerTests.cs | 108 +++++++++++++++++ .../MediaServerReconciliationGuardTests.cs | 69 +++++++++++ ...ediaServerTelevisionLibraryScannerTests.cs | 69 ++++++++++- .../MediaServerMovieLibraryScanner.cs | 14 ++- .../MediaServerOtherVideoLibraryScanner.cs | 12 +- .../MediaServerReconciliationGuard.cs | 40 +++++++ .../MediaServerTelevisionLibraryScanner.cs | 4 +- docs/decisions.md | 46 ++++++++ 9 files changed, 460 insertions(+), 12 deletions(-) create mode 100644 ErsatzTV.Scanner.Tests/Core/Metadata/MediaServerMovieLibraryScannerTests.cs create mode 100644 ErsatzTV.Scanner.Tests/Core/Metadata/MediaServerOtherVideoLibraryScannerTests.cs create mode 100644 ErsatzTV.Scanner.Tests/Core/Metadata/MediaServerReconciliationGuardTests.cs create mode 100644 ErsatzTV.Scanner/Core/Metadata/MediaServerReconciliationGuard.cs diff --git a/ErsatzTV.Scanner.Tests/Core/Metadata/MediaServerMovieLibraryScannerTests.cs b/ErsatzTV.Scanner.Tests/Core/Metadata/MediaServerMovieLibraryScannerTests.cs new file mode 100644 index 000000000..e21e28312 --- /dev/null +++ b/ErsatzTV.Scanner.Tests/Core/Metadata/MediaServerMovieLibraryScannerTests.cs @@ -0,0 +1,110 @@ +using System.IO.Abstractions; +using ErsatzTV.Core; +using ErsatzTV.Core.Domain; +using ErsatzTV.Core.Interfaces.Metadata; +using ErsatzTV.Core.Interfaces.Repositories; +using ErsatzTV.Core.Jellyfin; +using ErsatzTV.Core.Metadata; +using ErsatzTV.Scanner.Core.Interfaces; +using ErsatzTV.Scanner.Core.Interfaces.Metadata; +using ErsatzTV.Scanner.Core.Metadata; +using Microsoft.Extensions.Logging; +using NSubstitute; +using NUnit.Framework; +using Shouldly; + +namespace ErsatzTV.Scanner.Tests.Core.Metadata; + +// #477: a successful-but-empty media-server fetch must not flag a non-empty movie library FileNotFound +// (which EmptyTrash could then permanently delete). Assert the sweep is skipped when nothing came in. +public class MediaServerMovieLibraryScannerTests +{ + [TestFixture] + public class CleanupFileNotFoundItems + { + [Test] + public async Task Empty_Incoming_With_Existing_Movies_Does_Not_Flag() + { + var movieRepository = Substitute.For(); + var scannerProxy = Substitute.For(); + + var library = new JellyfinLibrary { Id = 3, Name = "Movies" }; + + movieRepository.GetExistingMovies(library) + .Returns(new List + { + new() { ItemId = "movie-1", State = MediaItemState.Normal }, + new() { ItemId = "movie-2", State = MediaItemState.Normal } + }); + + var scanner = new TestMovieLibraryScanner(scannerProxy); + + Either result = await scanner.Scan( + movieRepository, + new JellyfinConnectionParameters("http://jellyfin.example", "abc", 7), + library); + + result.IsRight.ShouldBeTrue(); + + await movieRepository.DidNotReceive().FlagFileNotFound( + Arg.Any(), Arg.Any>()); + await scannerProxy.DidNotReceive().ReindexMediaItems( + Arg.Any(), Arg.Any()); + } + } + + // Minimal concrete subclass exposing the cleanup path. Driven with an empty incoming list, so none of + // the per-item metadata members below are ever invoked — they exist only to satisfy the contract. + private sealed class TestMovieLibraryScanner : MediaServerMovieLibraryScanner< + JellyfinConnectionParameters, JellyfinLibrary, JellyfinMovie, JellyfinItemEtag> + { + public TestMovieLibraryScanner(IScannerProxy scannerProxy) + : base( + scannerProxy, + Substitute.For(), + Substitute.For(), + Substitute.For(), + Substitute.For()) + { + } + + public Task> Scan( + IJellyfinMovieRepository movieRepository, + JellyfinConnectionParameters connectionParameters, + JellyfinLibrary library) => + ScanLibrary( + movieRepository, + connectionParameters, + library, + _ => string.Empty, + false, + CancellationToken.None); + + protected override IAsyncEnumerable> GetMovieLibraryItems( + JellyfinConnectionParameters connectionParameters, JellyfinLibrary library) => EmptyMovies(); + + protected override string MediaServerItemId(JellyfinMovie movie) => movie.ItemId; + protected override string MediaServerEtag(JellyfinMovie movie) => movie.Etag; + + protected override Task> GetFullMetadata( + JellyfinConnectionParameters connectionParameters, JellyfinLibrary library, + MediaItemScanResult result, JellyfinMovie incoming, bool deepScan) => + throw new NotSupportedException(); + + protected override Task>> GetFullMetadataAndStatistics( + JellyfinConnectionParameters connectionParameters, JellyfinLibrary library, + MediaItemScanResult result, JellyfinMovie incoming) => + throw new NotSupportedException(); + + protected override Task>> UpdateMetadata( + MediaItemScanResult result, MovieMetadata fullMetadata, + CancellationToken cancellationToken) => + throw new NotSupportedException(); + + private static async IAsyncEnumerable> EmptyMovies() + { + await Task.CompletedTask; + yield break; + } + } +} diff --git a/ErsatzTV.Scanner.Tests/Core/Metadata/MediaServerOtherVideoLibraryScannerTests.cs b/ErsatzTV.Scanner.Tests/Core/Metadata/MediaServerOtherVideoLibraryScannerTests.cs new file mode 100644 index 000000000..a13c0fb9d --- /dev/null +++ b/ErsatzTV.Scanner.Tests/Core/Metadata/MediaServerOtherVideoLibraryScannerTests.cs @@ -0,0 +1,108 @@ +using System.IO.Abstractions; +using ErsatzTV.Core; +using ErsatzTV.Core.Domain; +using ErsatzTV.Core.Interfaces.Metadata; +using ErsatzTV.Core.Interfaces.Repositories; +using ErsatzTV.Core.Metadata; +using ErsatzTV.Core.Plex; +using ErsatzTV.Scanner.Core.Interfaces; +using ErsatzTV.Scanner.Core.Interfaces.Metadata; +using ErsatzTV.Scanner.Core.Metadata; +using ErsatzTV.Scanner.Core.Plex; +using Microsoft.Extensions.Logging; +using NSubstitute; +using NUnit.Framework; +using Shouldly; + +namespace ErsatzTV.Scanner.Tests.Core.Metadata; + +// #477: a successful-but-empty media-server fetch must not flag a non-empty other-video library +// FileNotFound (which EmptyTrash could then permanently delete). Assert the sweep is skipped. +public class MediaServerOtherVideoLibraryScannerTests +{ + [TestFixture] + public class CleanupFileNotFoundItems + { + [Test] + public async Task Empty_Incoming_With_Existing_OtherVideos_Does_Not_Flag() + { + var otherVideoRepository = Substitute.For(); + var scannerProxy = Substitute.For(); + + var library = new PlexLibrary { Id = 9, Name = "Other Videos" }; + + otherVideoRepository.GetExistingOtherVideos(library) + .Returns(new List + { + new() { Key = "ov-1", State = MediaItemState.Normal }, + new() { Key = "ov-2", State = MediaItemState.Normal } + }); + + var scanner = new TestOtherVideoLibraryScanner(scannerProxy); + + Either result = await scanner.Scan(otherVideoRepository, library); + + result.IsRight.ShouldBeTrue(); + + await otherVideoRepository.DidNotReceive().FlagFileNotFound( + Arg.Any(), Arg.Any>()); + await scannerProxy.DidNotReceive().ReindexMediaItems( + Arg.Any(), Arg.Any()); + } + } + + // Minimal concrete subclass exposing the cleanup path. Driven with an empty incoming list, so none of + // the per-item metadata members below are ever invoked — they exist only to satisfy the contract. The + // connection parameters are likewise never dereferenced on the empty path (passed null! below). + private sealed class TestOtherVideoLibraryScanner : MediaServerOtherVideoLibraryScanner< + PlexConnectionParameters, PlexLibrary, PlexOtherVideo, PlexItemEtag> + { + public TestOtherVideoLibraryScanner(IScannerProxy scannerProxy) + : base( + scannerProxy, + Substitute.For(), + Substitute.For(), + Substitute.For(), + Substitute.For()) + { + } + + public Task> Scan( + IPlexOtherVideoRepository otherVideoRepository, + PlexLibrary library) => + ScanLibrary( + otherVideoRepository, + null!, + library, + _ => string.Empty, + false, + CancellationToken.None); + + protected override IAsyncEnumerable> GetOtherVideoLibraryItems( + PlexConnectionParameters connectionParameters, PlexLibrary library) => EmptyOtherVideos(); + + protected override string MediaServerItemId(PlexOtherVideo otherVideo) => otherVideo.Key; + protected override string MediaServerEtag(PlexOtherVideo otherVideo) => otherVideo.Etag; + + protected override Task> GetFullMetadata( + PlexConnectionParameters connectionParameters, PlexLibrary library, + MediaItemScanResult result, PlexOtherVideo incoming, bool deepScan) => + throw new NotSupportedException(); + + protected override Task>> GetFullMetadataAndStatistics( + PlexConnectionParameters connectionParameters, PlexLibrary library, + MediaItemScanResult result, PlexOtherVideo incoming) => + throw new NotSupportedException(); + + protected override Task>> UpdateMetadata( + MediaItemScanResult result, OtherVideoMetadata fullMetadata, + CancellationToken cancellationToken) => + throw new NotSupportedException(); + + private static async IAsyncEnumerable> EmptyOtherVideos() + { + await Task.CompletedTask; + yield break; + } + } +} diff --git a/ErsatzTV.Scanner.Tests/Core/Metadata/MediaServerReconciliationGuardTests.cs b/ErsatzTV.Scanner.Tests/Core/Metadata/MediaServerReconciliationGuardTests.cs new file mode 100644 index 000000000..41f6e3029 --- /dev/null +++ b/ErsatzTV.Scanner.Tests/Core/Metadata/MediaServerReconciliationGuardTests.cs @@ -0,0 +1,69 @@ +using ErsatzTV.Scanner.Core.Metadata; +using Microsoft.Extensions.Logging; +using NSubstitute; +using NUnit.Framework; +using Shouldly; + +namespace ErsatzTV.Scanner.Tests.Core.Metadata; + +// #477: the deterministic policy behind the media-server anti-nuke guard. An empty incoming set with +// existing items present is the only case that skips the sweep (and logs); every other combination +// reconciles normally. +public class MediaServerReconciliationGuardTests +{ + [Test] + public void Empty_Incoming_With_Existing_Items_Skips_And_Warns() + { + var logger = Substitute.For(); + + bool shouldFlag = MediaServerReconciliationGuard.ShouldFlagMissing(logger, "Movies", 0, 5); + + shouldFlag.ShouldBeFalse(); + logger.Received(1).Log( + LogLevel.Warning, + Arg.Any(), + Arg.Any(), + Arg.Any(), + Arg.Any>()); + } + + [Test] + public void Partial_Deletion_Still_Flags() + { + var logger = Substitute.For(); + + MediaServerReconciliationGuard.ShouldFlagMissing(logger, "Movies", 3, 5).ShouldBeTrue(); + + logger.DidNotReceive().Log( + LogLevel.Warning, + Arg.Any(), + Arg.Any(), + Arg.Any(), + Arg.Any>()); + } + + [Test] + public void Empty_Incoming_With_No_Existing_Items_Is_A_Noop_Sweep() + { + var logger = Substitute.For(); + + // nothing exists, so an empty incoming set flags nothing either way — allow the (empty) sweep + // rather than special-casing it, and do not emit the scary warning. + MediaServerReconciliationGuard.ShouldFlagMissing(logger, "Movies", 0, 0).ShouldBeTrue(); + + logger.DidNotReceive().Log( + LogLevel.Warning, + Arg.Any(), + Arg.Any(), + Arg.Any(), + Arg.Any>()); + } + + [Test] + public void Full_Fetch_Into_Empty_Library_Still_Flags() + { + var logger = Substitute.For(); + + MediaServerReconciliationGuard.ShouldFlagMissing(logger, "Movies", 3, 0).ShouldBeTrue(); + } +} diff --git a/ErsatzTV.Scanner.Tests/Core/Metadata/MediaServerTelevisionLibraryScannerTests.cs b/ErsatzTV.Scanner.Tests/Core/Metadata/MediaServerTelevisionLibraryScannerTests.cs index 49c43862a..0a955cc69 100644 --- a/ErsatzTV.Scanner.Tests/Core/Metadata/MediaServerTelevisionLibraryScannerTests.cs +++ b/ErsatzTV.Scanner.Tests/Core/Metadata/MediaServerTelevisionLibraryScannerTests.cs @@ -17,6 +17,9 @@ namespace ErsatzTV.Scanner.Tests.Core.Metadata; // #476: a show/season gone from the media server is absent from the incoming list, so the per-parent // loop never visits it and the descendant sweeps never run for it. These tests assert the scanner // cascades the FileNotFound flag to descendants via the repository, using a substituted repository. +// #477: an empty incoming list is treated as a suspect (mid-restore / emptied) fetch and the sweep is +// skipped instead of nuking the whole library — so the #476 cascade is now exercised with a survivor +// present (a genuine partial deletion), and the empty case asserts nothing is flagged. public class MediaServerTelevisionLibraryScannerTests { [TestFixture] @@ -30,13 +33,24 @@ public class MediaServerTelevisionLibraryScannerTests var library = new JellyfinLibrary { Id = 42, Name = "TV Shows" }; - // one show exists in the DB but is NOT in the incoming (empty) list => it was removed upstream + // two shows exist; the survivor is still in the (non-empty) incoming list, so this is a + // genuine partial deletion — "show-6366" is gone upstream and must be flagged + cascaded. + // (#477: an EMPTY incoming would instead skip the sweep — see the guard test below.) televisionRepository.GetExistingShows(library, Arg.Any()) .Returns(new List { + new() { ItemId = "show-keep", State = MediaItemState.Normal }, new() { ItemId = "show-6366", State = MediaItemState.FileNotFound } }); + // the survivor short-circuits to Left so the per-item metadata path (unsupported in this + // harness) is never entered; it is still recorded as incoming, so it is not swept. + televisionRepository.GetOrAdd(library, Arg.Any(), Arg.Any()) + .Returns(Left>(BaseError.New("skip metadata in test"))); + + // the per-item loop reports progress; an unstubbed substitute returns false => ScanCanceled + scannerProxy.UpdateProgress(Arg.Any(), Arg.Any()).Returns(true); + televisionRepository .FlagFileNotFoundShows(library, Arg.Is>(l => l.Contains("show-6366")), Arg.Any()) @@ -57,7 +71,11 @@ public class MediaServerTelevisionLibraryScannerTests televisionRepository, new JellyfinConnectionParameters("http://jellyfin.example", "abc", 7), library, - EmptyShows()); + Shows(new JellyfinShow + { + ItemId = "show-keep", + ShowMetadata = new List { new() { Title = "Keeper" } } + })); result.IsRight.ShouldBeTrue(); @@ -78,11 +96,58 @@ public class MediaServerTelevisionLibraryScannerTests Arg.Any()); } + [Test] + public async Task Empty_Incoming_With_Existing_Shows_Does_Not_Flag() + { + var televisionRepository = Substitute.For(); + var scannerProxy = Substitute.For(); + + var library = new JellyfinLibrary { Id = 42, Name = "TV Shows" }; + + // shows exist locally, but a successful fetch returned ZERO items (server mid-restore or the + // library was emptied upstream). #477: flagging here would nuke the entire library, so the + // sweep must be skipped and nothing flagged or reindexed. + televisionRepository.GetExistingShows(library, Arg.Any()) + .Returns(new List + { + new() { ItemId = "show-1", State = MediaItemState.Normal }, + new() { ItemId = "show-2", State = MediaItemState.Normal } + }); + + var scanner = new TestTelevisionLibraryScanner(scannerProxy); + + Either result = await scanner.Scan( + televisionRepository, + new JellyfinConnectionParameters("http://jellyfin.example", "abc", 7), + library, + EmptyShows()); + + result.IsRight.ShouldBeTrue(); + + await televisionRepository.DidNotReceive().FlagFileNotFoundShows( + Arg.Any(), Arg.Any>(), Arg.Any()); + await televisionRepository.DidNotReceive().FlagFileNotFoundSeasonsForShows( + Arg.Any>(), Arg.Any()); + await televisionRepository.DidNotReceive().FlagFileNotFoundEpisodesForSeasons( + Arg.Any>(), Arg.Any()); + await scannerProxy.DidNotReceive().ReindexMediaItems( + Arg.Any(), Arg.Any()); + } + private static async IAsyncEnumerable> EmptyShows() { await Task.CompletedTask; yield break; } + + private static async IAsyncEnumerable> Shows(params JellyfinShow[] shows) + { + await Task.CompletedTask; + foreach (JellyfinShow show in shows) + { + yield return new Tuple(show, shows.Length); + } + } } // Minimal concrete subclass that exposes the abstract scanner's cleanup path. The incoming show list diff --git a/ErsatzTV.Scanner/Core/Metadata/MediaServerMovieLibraryScanner.cs b/ErsatzTV.Scanner/Core/Metadata/MediaServerMovieLibraryScanner.cs index cacedbf8a..e014aa745 100644 --- a/ErsatzTV.Scanner/Core/Metadata/MediaServerMovieLibraryScanner.cs +++ b/ErsatzTV.Scanner/Core/Metadata/MediaServerMovieLibraryScanner.cs @@ -1,4 +1,4 @@ -using System.Collections.Immutable; +using System.Collections.Immutable; using System.IO.Abstractions; using ErsatzTV.Core; using ErsatzTV.Core.Domain; @@ -203,11 +203,15 @@ public abstract class MediaServerMovieLibraryScanner ids = await movieRepository.FlagFileNotFound(library, fileNotFoundItemIds); - if (!await _scannerProxy.ReindexMediaItems(ids.ToArray(), cancellationToken)) + if (MediaServerReconciliationGuard.ShouldFlagMissing( + _logger, library.Name, incomingItemIds.Count, existingMovies.Count)) { - _logger.LogWarning("Failed to reindex media items from scanner process"); + var fileNotFoundItemIds = existingMovies.Keys.Except(incomingItemIds).ToList(); + List ids = await movieRepository.FlagFileNotFound(library, fileNotFoundItemIds); + if (!await _scannerProxy.ReindexMediaItems(ids.ToArray(), cancellationToken)) + { + _logger.LogWarning("Failed to reindex media items from scanner process"); + } } return Unit.Default; diff --git a/ErsatzTV.Scanner/Core/Metadata/MediaServerOtherVideoLibraryScanner.cs b/ErsatzTV.Scanner/Core/Metadata/MediaServerOtherVideoLibraryScanner.cs index 8442a42bb..18d90dcc2 100644 --- a/ErsatzTV.Scanner/Core/Metadata/MediaServerOtherVideoLibraryScanner.cs +++ b/ErsatzTV.Scanner/Core/Metadata/MediaServerOtherVideoLibraryScanner.cs @@ -210,11 +210,15 @@ public abstract class MediaServerOtherVideoLibraryScanner ids = await otherVideoRepository.FlagFileNotFound(library, fileNotFoundItemIds); - if (!await _scannerProxy.ReindexMediaItems(ids.ToArray(), cancellationToken)) + if (MediaServerReconciliationGuard.ShouldFlagMissing( + _logger, library.Name, incomingItemIds.Count, existingOtherVideos.Count)) { - _logger.LogWarning("Failed to reindex media items from scanner process"); + var fileNotFoundItemIds = existingOtherVideos.Keys.Except(incomingItemIds).ToList(); + List ids = await otherVideoRepository.FlagFileNotFound(library, fileNotFoundItemIds); + if (!await _scannerProxy.ReindexMediaItems(ids.ToArray(), cancellationToken)) + { + _logger.LogWarning("Failed to reindex media items from scanner process"); + } } return Unit.Default; diff --git a/ErsatzTV.Scanner/Core/Metadata/MediaServerReconciliationGuard.cs b/ErsatzTV.Scanner/Core/Metadata/MediaServerReconciliationGuard.cs new file mode 100644 index 000000000..fb5950f57 --- /dev/null +++ b/ErsatzTV.Scanner/Core/Metadata/MediaServerReconciliationGuard.cs @@ -0,0 +1,40 @@ +using Microsoft.Extensions.Logging; + +namespace ErsatzTV.Scanner.Core.Metadata; + +// #477: a media-server library sweep computes "gone upstream" as existing.Except(incoming) and flags the +// result FileNotFound. If a successful fetch returns ZERO items (the server is up but mid-restore / +// mid-rebuild, or the library was emptied upstream) then existing.Except([]) is EVERY existing item, so +// the whole library is flagged FileNotFound in one pass. That is data-loss-adjacent: EmptyTrashHandler +// deletes state:FileNotFound rows permanently, and PlayoutSkipMissingItems empties every affected +// collection. An empty incoming set is indistinguishable at scan time from a transient error (both report +// zero), so the safe policy is to refuse the sweep — logged loudly — rather than nuke the library. +// +// This deliberately overrides the degenerate "last item removed => empty incoming => flag" case that a +// partial-deletion sweep would otherwise handle (see #476's cascade, which still fires for the common +// case where survivors are present and only some items are gone). The cost of not flagging a genuinely +// emptied library (stale rows persist until an item returns or the library is removed) is far smaller +// than a one-scan permanent wipe. Ratio-thresholds and projection-failure detection are deferred — see +// docs/decisions.md and the #477 follow-up. +internal static class MediaServerReconciliationGuard +{ + public static bool ShouldFlagMissing( + ILogger logger, + string libraryName, + int incomingCount, + int existingCount) + { + if (incomingCount == 0 && existingCount > 0) + { + logger.LogWarning( + "Media server library {Library} returned zero items but {ExistingCount} exist locally; " + + "skipping the file-not-found sweep to avoid flagging the entire library as missing " + + "(expected if the server is mid-restore or the library was emptied upstream)", + libraryName, + existingCount); + return false; + } + + return true; + } +} diff --git a/ErsatzTV.Scanner/Core/Metadata/MediaServerTelevisionLibraryScanner.cs b/ErsatzTV.Scanner/Core/Metadata/MediaServerTelevisionLibraryScanner.cs index bc6a0619d..3b0f6cb2d 100644 --- a/ErsatzTV.Scanner/Core/Metadata/MediaServerTelevisionLibraryScanner.cs +++ b/ErsatzTV.Scanner/Core/Metadata/MediaServerTelevisionLibraryScanner.cs @@ -170,7 +170,9 @@ public abstract class MediaServerTelevisionLibraryScanner _logger.LogInformation("{Profile}", s)); } - if (cleanupFileNotFoundItems) + if (cleanupFileNotFoundItems && + MediaServerReconciliationGuard.ShouldFlagMissing( + _logger, library.Name, incomingItemIds.Count, existingShows.Count)) { // trash shows that are no longer present on the media server var fileNotFoundItemIds = existingShows.Map(s => s.MediaServerItemId).Except(incomingItemIds).ToList(); diff --git a/docs/decisions.md b/docs/decisions.md index 5ea8707c8..e4e4443af 100644 --- a/docs/decisions.md +++ b/docs/decisions.md @@ -2140,3 +2140,49 @@ server** returns the new `PlayoutItemNotAvailableFromMediaServer` error instead media servers — that is a reasonable prior but an *unverified* one. A HEAD-with-GET-fallback would avoid the side effect; deferred rather than guessed at, since it trades a known-working request for an untested one. + +## 2026-07-19 — A media-server library sweep refuses to flag when a successful fetch returns zero items, rather than nuking the whole library (#477) + +Each media-server scanner reconciles "gone upstream" as `existing.Except(incoming)` and flags the result +`FileNotFound`. If a *successful* fetch returns **zero** items — the server is up but mid-restore / +mid-rebuild, or the library was genuinely emptied upstream — then `existing.Except([])` is **every** item, +so one scan flags the entire library. That is data-loss-adjacent: `EmptyTrashHandler` permanently deletes +`state:FileNotFound` rows (a user clicking Empty Trash after a bad scan), and `PlayoutSkipMissingItems` +empties every affected collection (dead channels). There was no zero-count / ratio / server-total guard; +the only thing that stopped a mid-*pagination* failure was an exception unwinding past the flag step — +protection by accident of control flow, not by design. + +- **The guard is a single shared policy.** `MediaServerReconciliationGuard.ShouldFlagMissing(logger, + libraryName, incomingCount, existingCount)` returns false (and logs a Warning) **only** when + `incomingCount == 0 && existingCount > 0`; every other combination sweeps normally. Wired into the three + **library-level** sweeps — `MediaServerTelevisionLibraryScanner` (shows), `MediaServerMovieLibraryScanner`, + `MediaServerOtherVideoLibraryScanner`. One place owns the invariant so the policy can't drift between + scanners. +- **An empty incoming set is genuinely ambiguous, so we choose the non-destructive branch.** "User removed + every item" and "server returned empty erroneously" are **indistinguishable** at scan time — both report + a total of zero (the paginator computes `pages` from `TotalRecordCount`, so a 0 total is a clean empty + enumeration, not an error). Given the blast radius, skipping wins: the cost of *not* flagging a + legitimately-emptied library (stale rows persist until an item returns or the library is removed by hand) + is far smaller than a one-scan permanent wipe of a live library. +- **This partially overrides #476 for the degenerate case, on purpose.** #476 cascades a removed show's + flag to its seasons/episodes. Its common path — some items removed while **survivors are present** + (incoming non-empty) — still flags and cascades exactly as before. Only the degenerate "the last item was + removed, so incoming is empty" case now skips instead of flagging. The #476 characterization test was + rewritten from an empty incoming to a survivor-plus-removed partial deletion so it exercises the cascade + without tripping the guard. +- **Scope: library-level sweeps only; the nested TV season/episode sweeps are deliberately left unguarded.** + Their blast radius is one show's seasons / one season's episodes (not the whole library), a per-parent + empty is a more plausible legitimate state there, and the #476 descendant cascade already handles a fully + removed parent. Guarding them would alter #476's per-parent behaviour for little safety gain. +- **Deferred — ratio threshold and projection-failure detection.** The issue also floated "skip if the + missing fraction exceeds a threshold" and "distinguish a silently-dropped projection failure from a real + deletion." A ratio threshold risks suppressing a legitimate bulk deletion and needs a tunable, telemetry- + backed policy; projection-failure detection needs a dropped-count threaded out of `JellyfinApiClient` + through to the scanner (a cross-layer change). The deterministic zero-count guard has **no** false + positives and covers the reported catastrophic case, so both are deferred to a follow-up rather than + guessed at here. +- **Tests.** `MediaServerReconciliationGuardTests` pins the policy table (only `(0, N>0)` skips-and-warns; + `(0,0)`, `(3,5)`, `(3,0)` all sweep). Per-scanner integration tests + (`MediaServer{Television,Movie,OtherVideo}LibraryScannerTests`) prove the wiring: empty incoming + + non-empty existing flags nothing and reindexes nothing. Proven non-vacuous by neutralizing the guard and + watching all four anti-nuke assertions fail while the `(0,0)` no-op case stays green.