fix(484): guard the nested TV season and episode sweeps against projection failures
Review finding 1 (blocking). ScanSeasons' FlagFileNotFoundSeasons and ScanEpisodes' FlagFileNotFoundEpisodes had no guard at all — neither #477's nor #484's — so ProjectToSeason / ProjectToEpisode returning Failed() was computed and discarded. #477 scoped those out because "the blast radius is one show's seasons / one season's episodes", which holds for a per-parent EMPTY fetch but not for a projection failure: that is systematic by construction. One bad code path fires on every parent, so every season enumerates zero episodes, existing.Except([]) is the whole episode library, and EmptyTrashHandler deletes it permanently. Threads the counter into GetSeasonLibraryItems / GetEpisodeLibraryItems(WithoutPeople) for Jellyfin and Emby using the same optional-trailing-param shape, and guards both sweeps with MediaServerReconciliationGuard.ShouldFlagMissingDescendants — the same class and the same private failure predicate as ShouldFlagMissing, deliberately WITHOUT #477's empty-fetch branch so per-parent empty behaviour (and #476's cascade, which depends on it) is unchanged. Also from the review: - finding 3: tests now pin the same-instance JOIN at every level (movie, show, season, episode, music video) by driving the real ScanLibrary entry point and recording the failure from inside the enumeration, so a refactor handing the api client a fresh counter goes red. - finding 4: the missing-library Failed() branch is documented as defensive and unreachable. - finding 2: the mass-Skip residual (Emby's response-shape-dependent MediaSources guard, Plex's pre-projection filter) is stated as a known limitation in the decision record. - finding 5: the log-contract change (only the #484 message when both refusals apply) is noted. fixes #484
This commit is contained in:
+55
-9
@@ -3779,8 +3779,8 @@ measuring is real signal; it just needs its own actor convention and wasn't wort
|
||||
change.
|
||||
## 2026-07-25 — A media-server sweep also refuses when the api client silently dropped items whose projection threw; the ratio threshold is rejected (#484)
|
||||
`key: scan.projection-failure-sweep-guard` · `status: active` · `since: 2026-07-25` · `supersedes: none` · `superseded-by: none`
|
||||
**Rule:** `MediaServerReconciliationGuard.ShouldFlagMissing` takes a per-enumeration projection-failure count and refuses the file-not-found sweep (logged loudly) whenever it is non-zero against a non-empty existing set; deliberate guard-clause skips (STRM, virtual, unsupported type) are explicitly **not** failures and never suppress a sweep. The missing-fraction / ratio threshold floated by #477 is **rejected**, not deferred.
|
||||
**Signals:** projection failure, silently dropped item, deferred ratio threshold rejected, STRM skip vs failure, library sweep anti-nuke follow-up · paths: `MediaServerProjectionResult`, `MediaServerProjectionFailureCounter`, `MediaServerReconciliationGuard`, `JellyfinApiClient.GetPagedLibraryItems`, `EmbyApiClient.GetPagedLibraryContents` · issues: #484, #477, #476
|
||||
**Rule:** `MediaServerReconciliationGuard` takes a per-enumeration projection-failure count and refuses the file-not-found sweep (logged loudly) whenever it is non-zero against a non-empty existing set — at all six sweeps, including the nested per-show season and per-season episode ones #477 left unguarded (via `ShouldFlagMissingDescendants`, which applies the failure refusal but not #477's empty-fetch branch). Deliberate guard-clause skips (STRM, virtual, unsupported type) are explicitly **not** failures and never suppress a sweep. The missing-fraction / ratio threshold floated by #477 is **rejected**, not deferred.
|
||||
**Signals:** projection failure, silently dropped item, deferred ratio threshold rejected, STRM skip vs failure, library sweep anti-nuke follow-up, nested season/episode sweep guard · paths: `MediaServerProjectionResult`, `MediaServerProjectionFailureCounter`, `MediaServerReconciliationGuard`, `JellyfinApiClient.GetPagedLibraryItems`, `EmbyApiClient.GetPagedLibraryContents` · issues: #484, #477, #476
|
||||
**Mechanics:** `MediaServerReconciliationGuardTests` policy table · `JellyfinApiClientTests.ProjectionFailureCounter` · `MediaServerTelevisionLibraryScannerTests.Projection_Failure_Suppresses_The_Partial_Deletion_Sweep`
|
||||
|
||||
This resolves both items the `scan.zero-item-fetch-guard` record (#477) left deferred. That record's rule
|
||||
@@ -3820,19 +3820,53 @@ This is a **second, independent** refusal on the same guard, plus a decision not
|
||||
scanner reads the count through an **optional trailing parameter** on only the library-level methods
|
||||
that actually feed a sweep (`IJellyfinApiClient.GetMovieLibraryItems` /
|
||||
`GetMusicVideoLibraryItems` / `GetShowLibraryItemsWithoutPeople`, `IEmbyApiClient.GetMovieLibraryItems`
|
||||
/ `GetShowLibraryItems`), so every other call site is untouched. A library that cannot be resolved
|
||||
(`Option<TLibrary>.None` inside a mapper) is treated as a **failure**, not a skip — fail closed.
|
||||
/ `GetShowLibraryItems`) **and** the nested season/episode ones (`GetSeasonLibraryItems`,
|
||||
`GetEpisodeLibraryItems`, `GetEpisodeLibraryItemsWithoutPeople`), so every other call site is untouched.
|
||||
A library that cannot be resolved (`Option<TLibrary>.None` inside a mapper) is classed as a **failure**
|
||||
rather than a skip; that branch is defensive and currently **unreachable** (every counted path passes a
|
||||
concrete library, and the only `None` caller — collections — does not route through it), kept only so
|
||||
the default classification is right if a future caller does pass `None`.
|
||||
- **The counter is per-enumeration state, never ambient.** `MediaServerProjectionFailureCounter` is
|
||||
created by the scanner that owns the sweep, handed to the single enumeration whose result it will
|
||||
diff, and read only after that enumeration completes. It is never a field on an api client (those are
|
||||
long-lived and shared) and never static, so concurrent scans of different libraries cannot leak
|
||||
failures into each other's sweep decision; increments are interlocked so a paginator that ever fans
|
||||
out stays correct.
|
||||
- **Wired into the four real sweep call sites**, matching #477's scope exactly: the three library-level
|
||||
base scanners (`MediaServerMovieLibraryScanner`, `MediaServerTelevisionLibraryScanner`,
|
||||
`MediaServerOtherVideoLibraryScanner`) and `JellyfinMusicVideoLibraryScanner`. The nested TV
|
||||
season/episode sweeps stay unguarded for the reasons in `scan.zero-item-fetch-guard`, and
|
||||
- **Wired into all six sweeps, not #477's four.** The four library-level ones — the three base scanners
|
||||
(`MediaServerMovieLibraryScanner`, `MediaServerTelevisionLibraryScanner`,
|
||||
`MediaServerOtherVideoLibraryScanner`) plus `JellyfinMusicVideoLibraryScanner` — **and** the two nested
|
||||
TV ones, `ScanSeasons`' `FlagFileNotFoundSeasons` and `ScanEpisodes`' `FlagFileNotFoundEpisodes`.
|
||||
`ScanLibraryWithoutCleanup` (single-show rescan) passes no counter because it runs no sweep.
|
||||
- **The nested season/episode sweeps get the failure refusal but NOT #477's empty-fetch branch.**
|
||||
`scan.zero-item-fetch-guard` left them unguarded because "the blast radius is one show's seasons / one
|
||||
season's episodes, not the whole library." That reasoning is sound for a per-parent **empty fetch** — a
|
||||
plausible legitimate state, bounded to one parent — but it does **not** transfer to a projection
|
||||
failure, which is systematic by construction: one bad code path fires on every parent, so a
|
||||
`ProjectToEpisode` regression makes *every* season enumerate zero episodes and
|
||||
`existing.Except([])` sweeps the **entire** episode library in a single scan, which `EmptyTrashHandler`
|
||||
then deletes permanently. Same shape for `ProjectToSeason` plus #476's cascade. So the nested sweeps
|
||||
call `MediaServerReconciliationGuard.ShouldFlagMissingDescendants` — the **same class and the same
|
||||
private failure predicate** as `ShouldFlagMissing`, deliberately without the empty-fetch branch. A
|
||||
separate entry point rather than reusing `ShouldFlagMissing` wholesale, because importing #477's
|
||||
zero-count branch here would silently change per-parent behaviour and break #476's cascade, which
|
||||
depends on an emptied parent still sweeping. One class still owns the invariant; only the empty-fetch
|
||||
policy differs, and the difference is pinned by a test.
|
||||
- **Known residual: a mass *Skip* is not covered, by design, and one skip site is response-shape
|
||||
dependent.** The whole split rests on skips being permanent and item-intrinsic. Emby's
|
||||
`ProjectToMovie` guard (`MediaSources is null || Count == 0`) does not fully satisfy that: it is
|
||||
response-shape dependent, so a Refit/DTO drift or a `fields`-parameter regression would present as a
|
||||
mass **Skip**, which by construction leaves the sweep enabled. A *total* such regression is caught by
|
||||
#477's zero-incoming branch; a *partial* one (say 1,200 incoming vs 2,000 existing, zero failures)
|
||||
would sweep 800 healthy movies into `FileNotFound`. Plex's pre-projection
|
||||
`.Filter(m => m.Media.Count > 0 …)` is the same class. This is stated rather than engineered around:
|
||||
re-classifying shape-dependent skips as failures would reintroduce exactly the STRM-style permanent
|
||||
suppression the split exists to prevent. Treat it as the known next question if a mass-skip incident
|
||||
ever occurs.
|
||||
- **Log-contract note.** When both refusals apply, only the #484 "silently dropped …" warning is emitted,
|
||||
not #477's "returned zero items" line — the failure names the actual cause, and emitting both would
|
||||
imply two independent problems. Operator alerting that greps the #477 string will not fire in that
|
||||
case. The message property is `{Scope}` (e.g. `library Movies`, `show Keeper seasons`,
|
||||
`season 1 of show Keeper episodes`), not `{Library}`, so one alert pattern covers all six sweeps.
|
||||
- **Plex reports zero failures because it has none.** `PlexServerApiClient`'s movie/show/other-video
|
||||
projections return a bare entity with no `Option` and no `catch`, so a bad item throws and unwinds the
|
||||
whole scan — the pre-existing "protection by accident of control flow" that #477 named. The Plex
|
||||
@@ -3848,4 +3882,16 @@ This is a **second, independent** refusal on the same guard, plus a decision not
|
||||
instance keep separate counts. `MediaServerTelevisionLibraryScannerTests
|
||||
.Projection_Failure_Suppresses_The_Partial_Deletion_Sweep` is the integration pair to #476's cascade
|
||||
test: identical arrangement, only the counter differs, so the cascade test is the positive control
|
||||
that keeps the new test from passing vacuously.
|
||||
that keeps the new test from passing vacuously. The nested levels get
|
||||
`Season_Sweep_Respects_Projection_Failures` and `Episode_Sweep_Respects_Projection_Failures`, each
|
||||
parameterised with its own zero-failure positive control, plus guard tests asserting the descendant
|
||||
entry point still sweeps a per-parent **empty** fetch (the #477-scope invariant) and still sweeps when
|
||||
only deliberate skips shortened the incoming set.
|
||||
- **Tests pin the JOIN, not just the two halves.** A client test proving the counter fills, plus a
|
||||
scanner test proving the guard honours a counter it is handed, would both stay green if a refactor
|
||||
handed the api client a *fresh* counter while the sweep read the local one — silently killing the
|
||||
protection. `MediaServerMovieLibraryScannerTests.Projection_Failure_From_The_Api_Suppresses_The_Sweep_
|
||||
End_To_End`, `MediaServerTelevisionLibraryScannerTests.Public_ScanLibrary_Joins_The_Api_Counter_To_The_
|
||||
Sweep`, the two nested TV cases, and `JellyfinMusicVideoLibraryScannerTests
|
||||
.MusicVideo_Sweep_Respects_Projection_Failures` all drive the real `ScanLibrary` entry point and record
|
||||
the failure from *inside* the enumeration, so same-instance wiring is what makes them pass.
|
||||
|
||||
Reference in New Issue
Block a user