JellyfinMusicVideoLibraryScanner crashes immediately: ArgumentNullException from null LibraryPath.LibraryFolders (feature has never worked) #488

New Issue

2026-07-20T00:26:07+02:00

timothy commented

2026-07-20 00:26:07 +02:00

Found while working #474 on prod (2026-07-20). A Jellyfin library correctly typed musicvideos is discovered, mapped and dispatched fine — and then the scan crashes on the very first item, every time. Zero items ingested.

The fork's Jellyfin music-video support has, as far as the evidence goes, never successfully run — not in prod, not in CI, not locally.

Repro

Jellyfin library with content type Music videos (prod: id 15 → /data/music, 30 artist folders).
POST /api/v1/media-sources/jellyfin/2/refresh-libraries → appears as MediaKind = MusicVideos ✅
PUT /api/v1/media-sources/jellyfin/2/libraries → shouldSyncItems: true ✅
POST /api/v1/libraries/15/scan → 202, scanner process dies immediately.

Artist = 0, MusicVideo = 0 after the scan.

Stack

Unhandled exception: System.ArgumentNullException: Value cannot be null. (Parameter 'source')
   at System.Linq.Enumerable.Where[TSource](IEnumerable`1 source, Func`2 predicate)
   at LanguageExt.List.filter[T](IEnumerable`1 list, Func`2 predicate)
   at ListExtensions.Filter[T](IEnumerable`1 list, Func`2 predicate)
   at LibraryRepository.GetOrAddFolder(...) in ErsatzTV.Infrastructure/Data/Repositories/LibraryRepository.cs:line 173
   at LibraryRepository.GetOrAddFolder(...) in …LibraryRepository.cs:line 198
   at JellyfinMusicVideoLibraryScanner.ProcessMusicVideo(...) in …/JellyfinMusicVideoLibraryScanner.cs:line 130
   at JellyfinMusicVideoLibraryScanner.ScanLibrary(...) line 99 → 80 → 59
   at SynchronizeJellyfinLibraryByIdHandler.Synchronize(...) line 77

Root cause

LibraryRepository.GetOrAddFolder (line 173) reads the navigation collection off the passed entity:

LibraryFolder knownFolder = await libraryPath.LibraryFolders
    .Filter(f => f.Path == folder && f.LibraryPathId == libraryPath.Id)
    .HeadOrNone()
    .IfNoneAsync(CreateNewFolder(libraryPath, maybeParentFolder, folder));

It assumes LibraryFolders is loaded. That assumption holds for every local folder scanner, because the local scan path loads the library through LibraryRepository.GetLibrary(int):

return await dbContext.Libraries
    .Include(l => l.Paths)
    .ThenInclude(p => p.LibraryFolders)      // <-- the reason the local scanners work
    .ThenInclude(lf => lf.ImageFolderDuration)

The Jellyfin sync path does not. SynchronizeJellyfinLibraryByIdHandler validates via JellyfinLibraryMustExist, and JellyfinMusicVideoLibraryScanner.ScanLibrary then takes its LibraryPath straight off that entity:

Option<LibraryPath> maybeLibraryPath = Optional(library.Paths).Flatten().HeadOrNone();

Paths is populated; Paths[].LibraryFolders is null. Filter → Enumerable.Where(null, …) → ArgumentNullException('source').

So the defect is an implicit, undocumented loading contract on GetOrAddFolder — "caller must have eager-loaded LibraryFolders" — that is satisfied by exactly one of its two call-site families.

JellyfinMusicVideoLibraryScanner is the only remote scanner that calls GetOrAddFolder; every other caller is a local *FolderScanner. That is precisely why no other remote scanner trips it.

Fix — needs a decision, not just a null guard

Do not simply coerce null → empty. GetOrAddFolder falls through to CreateNewFolder when the lookup misses, so treating a null (= unknown) collection as an empty (= known-absent) one would make every scan create a duplicate LibraryFolder row for a path that already exists. Null and empty are not the same thing here, and there is no unique constraint standing behind it.

Two candidate shapes:

Fix the caller — load the LibraryPath with LibraryFolders included on the Jellyfin sync path. Smallest, matches how the local path already works, leaves the implicit contract intact (and therefore leaves the next remote scanner free to trip it again).
Fix the repository — make GetOrAddFolder self-sufficient: query LibraryFolder by (LibraryPathId, Path) from the DB instead of trusting a navigation collection. Removes the implicit contract entirely and is correct for every caller, at the cost of a query per folder on the local hot path (currently served from memory).

My inclination is (2) with the in-memory collection kept as a fast path when it is non-null, but that is a real design call and shouldn't be made unilaterally.

Whichever is chosen, the loading contract should be stated on ILibraryRepository.GetOrAddFolder rather than left implicit.

Why nothing caught this

Worth recording, because the gap is structural rather than an oversight on one PR:

No test exists for this scanner. ErsatzTV.Scanner.Tests covers MediaServerMovieLibraryScanner, MediaServerTelevisionLibraryScanner, MediaServerOtherVideoLibraryScanner and MovieFolderScanner — and JellyfinMusicVideoLibraryScanner is a standalone class that does not derive from the MediaServer*LibraryScanner base, so it inherits none of that coverage. There is no JellyfinMusicVideoLibraryScannerTests.
The one test that touches GetOrAddFolder mocks it away: MovieFolderScannerTests.cs:88 — _libraryRepository.GetOrAddFolder(Arg.Any<LibraryPath>(), …). A substituted repository cannot exhibit a null-navigation bug, so no local-scanner test could ever have caught it either.
E2E never exercises a remote source. scripts/e2e-local.sh seeds a local TV library (tiny ffmpeg testsrc MKVs + LibraryPath rows + scan) per docs/e2e-local.md. There is no Jellyfin in the E2E harness, so no remote scanner runs at all.
Prod could not have surfaced it. No Jellyfin library was ever typed musicvideos (the one on disk was mixed, which JellyfinApiClient.Project drops silently — see #474), so this code path had never been reached on the live system either.

Net: an unused, untested, un-E2E'd code path that fails on its first line of real work. The instructive part is that it was reachable only by a configuration change, so no amount of diff review would have found it.

Resolution (PR #492)

Went with option 2 (repository self-sufficient), but as a pure DB lookup rather than the hybrid with an in-memory fast path. Rationale: the reporter's hesitation about (2) — "a query per folder on the local hot path (currently served from memory)" — does not actually hold. Every local *FolderScanner already calls GetParentFolderId (a DB query with the same (LibraryPathId, …) shape) once per folder immediately before GetOrAddFolder, so folder granularity was never served purely from memory; the fix adds one indexed lookup per folder, not a per-file query. The pure lookup also removes the implicit contract with one code path (no is not null branch) and is more correct — it sees folders created earlier in the same scan, which a load-time in-memory snapshot cannot. Full reasoning in the docs/decisions.md entry.

Done-when

A Jellyfin musicvideos library scans to completion and creates Artist + MusicVideo rows — proven by JellyfinMusicVideoLibraryScannerTests (end-to-end scan of a synthetic music video wiring the real LibraryRepository/ArtistRepository/MusicVideoRepository). Live re-enable of prod library 15 sync is the operator's follow-up step (see Prod state below).
Re-scanning is idempotent — no duplicate LibraryFolder or MusicVideo rows (LibraryFolder idempotency pinned by LibraryRepositoryTests; MusicVideo GetOrAdd de-dupes by path, unchanged behavior).
The GetOrAddFolder loading contract is decided, implemented, and documented on the interface (ILibraryRepository.GetOrAddFolder remark).
Regression test covering JellyfinMusicVideoLibraryScanner with a non-included LibraryFolders (proven non-vacuous by reverting the fix) — both JellyfinMusicVideoLibraryScannerTests and LibraryRepositoryTests reproduce the ArgumentNullException('source') when the fix is reverted.
docs/decisions.md entry (repository contract changed).
Adversarial review passed — cold-context review returned MERGEABLE (see PR #492 Review-verdict).

Prod state

Sync on library 15 has been set back to shouldSyncItems: false so the cadence scan does not crash-loop. The library row remains. Main container unaffected — restarts=0, the scanner is a separate process. Re-enable when this is fixed.

Blocks #474 (and transitively #487).

Follow-ups

#491 — LibraryFolder has no unique index on (LibraryPathId, Path); the pre-existing concurrent double-insert window (not a regression).
CleanEtagsForLibraryPath reads libraryPath.LibraryFolders the same way, but is only reached on the local (eager-loaded) path, so it is not affected; left as-is.

Found while working #474 on prod (2026-07-20). A Jellyfin library correctly typed `musicvideos` is discovered, mapped and dispatched fine — and then the scan **crashes on the very first item**, every time. Zero items ingested. The fork's Jellyfin music-video support has, as far as the evidence goes, **never successfully run** — not in prod, not in CI, not locally. ## Repro 1. Jellyfin library with content type `Music videos` (prod: id 15 → `/data/music`, 30 artist folders). 2. `POST /api/v1/media-sources/jellyfin/2/refresh-libraries` → appears as `MediaKind = MusicVideos` ✅ 3. `PUT /api/v1/media-sources/jellyfin/2/libraries` → `shouldSyncItems: true` ✅ 4. `POST /api/v1/libraries/15/scan` → **202**, scanner process dies immediately. `Artist` = 0, `MusicVideo` = 0 after the scan. ## Stack ``` Unhandled exception: System.ArgumentNullException: Value cannot be null. (Parameter 'source') at System.Linq.Enumerable.Where[TSource](IEnumerable`1 source, Func`2 predicate) at LanguageExt.List.filter[T](IEnumerable`1 list, Func`2 predicate) at ListExtensions.Filter[T](IEnumerable`1 list, Func`2 predicate) at LibraryRepository.GetOrAddFolder(...) in ErsatzTV.Infrastructure/Data/Repositories/LibraryRepository.cs:line 173 at LibraryRepository.GetOrAddFolder(...) in …LibraryRepository.cs:line 198 at JellyfinMusicVideoLibraryScanner.ProcessMusicVideo(...) in …/JellyfinMusicVideoLibraryScanner.cs:line 130 at JellyfinMusicVideoLibraryScanner.ScanLibrary(...) line 99 → 80 → 59 at SynchronizeJellyfinLibraryByIdHandler.Synchronize(...) line 77 ``` ## Root cause `LibraryRepository.GetOrAddFolder` (line 173) reads the **navigation collection** off the passed entity: ```csharp LibraryFolder knownFolder = await libraryPath.LibraryFolders .Filter(f => f.Path == folder && f.LibraryPathId == libraryPath.Id) .HeadOrNone() .IfNoneAsync(CreateNewFolder(libraryPath, maybeParentFolder, folder)); ``` It assumes `LibraryFolders` is loaded. That assumption holds for every **local** folder scanner, because the local scan path loads the library through `LibraryRepository.GetLibrary(int)`: ```csharp return await dbContext.Libraries .Include(l => l.Paths) .ThenInclude(p => p.LibraryFolders) // <-- the reason the local scanners work .ThenInclude(lf => lf.ImageFolderDuration) ``` The **Jellyfin** sync path does not. `SynchronizeJellyfinLibraryByIdHandler` validates via `JellyfinLibraryMustExist`, and `JellyfinMusicVideoLibraryScanner.ScanLibrary` then takes its `LibraryPath` straight off that entity: ```csharp Option<LibraryPath> maybeLibraryPath = Optional(library.Paths).Flatten().HeadOrNone(); ``` `Paths` is populated; `Paths[].LibraryFolders` is **null**. `Filter` → `Enumerable.Where(null, …)` → `ArgumentNullException('source')`. So the defect is an **implicit, undocumented loading contract** on `GetOrAddFolder` — "caller must have eager-loaded `LibraryFolders`" — that is satisfied by exactly one of its two call-site families. `JellyfinMusicVideoLibraryScanner` is the **only** remote scanner that calls `GetOrAddFolder`; every other caller is a local `*FolderScanner`. That is precisely why no other remote scanner trips it. ## Fix — needs a decision, not just a null guard Do **not** simply coerce null → empty. `GetOrAddFolder` falls through to `CreateNewFolder` when the lookup misses, so treating a null (= *unknown*) collection as an empty (= *known-absent*) one would make every scan create a duplicate `LibraryFolder` row for a path that already exists. Null and empty are not the same thing here, and there is no unique constraint standing behind it. Two candidate shapes: 1. **Fix the caller** — load the `LibraryPath` with `LibraryFolders` included on the Jellyfin sync path. Smallest, matches how the local path already works, leaves the implicit contract intact (and therefore leaves the next remote scanner free to trip it again). 2. **Fix the repository** — make `GetOrAddFolder` self-sufficient: query `LibraryFolder` by `(LibraryPathId, Path)` from the DB instead of trusting a navigation collection. Removes the implicit contract entirely and is correct for every caller, at the cost of a query per folder on the local hot path (currently served from memory). My inclination is **(2)** with the in-memory collection kept as a fast path when it is non-null, but that is a real design call and shouldn't be made unilaterally. Whichever is chosen, the loading contract should be **stated** on `ILibraryRepository.GetOrAddFolder` rather than left implicit. ## Why nothing caught this Worth recording, because the gap is structural rather than an oversight on one PR: - **No test exists for this scanner.** `ErsatzTV.Scanner.Tests` covers `MediaServerMovieLibraryScanner`, `MediaServerTelevisionLibraryScanner`, `MediaServerOtherVideoLibraryScanner` and `MovieFolderScanner` — and `JellyfinMusicVideoLibraryScanner` is a **standalone class that does not derive from the `MediaServer*LibraryScanner` base**, so it inherits none of that coverage. There is no `JellyfinMusicVideoLibraryScannerTests`. - **The one test that touches `GetOrAddFolder` mocks it away**: `MovieFolderScannerTests.cs:88` — `_libraryRepository.GetOrAddFolder(Arg.Any<LibraryPath>(), …)`. A substituted repository cannot exhibit a null-navigation bug, so no local-scanner test could ever have caught it either. - **E2E never exercises a remote source.** `scripts/e2e-local.sh` seeds a **local** TV library (tiny ffmpeg testsrc MKVs + `LibraryPath` rows + scan) per `docs/e2e-local.md`. There is no Jellyfin in the E2E harness, so no remote scanner runs at all. - **Prod could not have surfaced it.** No Jellyfin library was ever typed `musicvideos` (the one on disk was `mixed`, which `JellyfinApiClient.Project` drops silently — see #474), so this code path had never been reached on the live system either. Net: an unused, untested, un-E2E'd code path that fails on its first line of real work. The instructive part is that it was **reachable only by a configuration change**, so no amount of diff review would have found it. ## Resolution (PR #492) Went with **option 2** (repository self-sufficient), but as a **pure DB lookup** rather than the hybrid with an in-memory fast path. Rationale: the reporter's hesitation about (2) — "a query per folder on the local hot path (currently served from memory)" — does not actually hold. Every local `*FolderScanner` already calls `GetParentFolderId` (a DB query with the same `(LibraryPathId, …)` shape) once per folder immediately before `GetOrAddFolder`, so folder granularity was never served purely from memory; the fix adds one indexed lookup per folder, not a per-file query. The pure lookup also removes the implicit contract with one code path (no `is not null` branch) and is *more* correct — it sees folders created earlier in the same scan, which a load-time in-memory snapshot cannot. Full reasoning in the `docs/decisions.md` entry. ## Done-when - [x] A Jellyfin `musicvideos` library scans to completion and creates `Artist` + `MusicVideo` rows — proven by `JellyfinMusicVideoLibraryScannerTests` (end-to-end scan of a synthetic music video wiring the real `LibraryRepository`/`ArtistRepository`/`MusicVideoRepository`). Live re-enable of prod library 15 sync is the operator's follow-up step (see Prod state below). - [x] Re-scanning is idempotent — no duplicate `LibraryFolder` or `MusicVideo` rows (`LibraryFolder` idempotency pinned by `LibraryRepositoryTests`; `MusicVideo` `GetOrAdd` de-dupes by path, unchanged behavior). - [x] The `GetOrAddFolder` loading contract is decided, implemented, and **documented on the interface** (`ILibraryRepository.GetOrAddFolder` remark). - [x] Regression test covering `JellyfinMusicVideoLibraryScanner` with a non-included `LibraryFolders` (proven non-vacuous by reverting the fix) — both `JellyfinMusicVideoLibraryScannerTests` and `LibraryRepositoryTests` reproduce the `ArgumentNullException('source')` when the fix is reverted. - [x] `docs/decisions.md` entry (repository contract changed). - [x] Adversarial review passed — cold-context review returned MERGEABLE (see PR #492 `Review-verdict`). ## Prod state Sync on library 15 has been set back to `shouldSyncItems: false` so the cadence scan does not crash-loop. The library row remains. Main container unaffected — `restarts=0`, the scanner is a separate process. Re-enable when this is fixed. Blocks #474 (and transitively #487). ## Follow-ups - #491 — `LibraryFolder` has no unique index on `(LibraryPathId, Path)`; the pre-existing concurrent double-insert window (not a regression). - `CleanEtagsForLibraryPath` reads `libraryPath.LibraryFolders` the same way, but is only reached on the local (eager-loaded) path, so it is not affected; left as-is.

timothy added the bug priority: high labels 2026-07-20 00:26:07 +02:00

timothy added a new dependency 2026-07-20 00:26:14 +02:00

#474 Music channels dead: `Music Videos` library has no LibraryPath, so /data/music (30 folders) is never scanned — 8+ collections empty

JellyfinMusicVideoLibraryScanner crashes immediately: ArgumentNullException from null LibraryPath.LibraryFolders (feature has never worked) #488

Repro

Stack

Root cause

Fix — needs a decision, not just a null guard

Why nothing caught this

Resolution (PR #492)

Done-when

Prod state

Follow-ups

What was done

Root cause

Files changed

Docs updated

Follow-ups filed

Operator follow-up (not code)