fix(488): resolve LibraryFolder from DB so Jellyfin music-video scans stop crashing #492
No Assignees
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: timothy/ersatztv#492
Reference in New Issue
Block a user
Fixes #488.
Problem
LibraryRepository.GetOrAddFolderreadlibraryPath.LibraryFolders— a navigation collection only eager-loaded on the local scan path (GetLibrary.Include(...).ThenInclude(p => p.LibraryFolders)). The Jellyfin remote sync path takes itsLibraryPathstraight off theJellyfinLibraryentity, whereLibraryFoldersisnull, so.Filter(null)→Enumerable.Where(null, …)→ArgumentNullException('source')on the first item of every Jellyfin music-video scan.JellyfinMusicVideoLibraryScanneris the only remote caller ofGetOrAddFolder(it doesn't derive from theMediaServer*LibraryScannerbase), which is why no other remote scanner tripped it and the feature had never run in prod, CI, or locally.Fix
Look the folder up from the DB by
(LibraryPathId, Path)instead of the in-memory nav collection.ILibraryRepository.GetOrAddFolder.null≠ empty preserved. Anull(unknown) collection is not coerced to empty (known-absent), so a re-scan does not insert duplicateLibraryFolderrows (there is no unique constraint behind it — tracked separately as #491).GetParentFolderId(a DB query, same(LibraryPathId, …)shape) per folder immediately before this call, so the folder granularity was never served purely from memory; this adds one indexed lookup per folder, not a per-file query.Tests
LibraryRepositoryTests(real in-memory SQLite):GetOrAddFolderwithLibraryPath.LibraryFolders == nullcreates the folder, is idempotent on re-scan (no duplicate row), and persists a suppliedParentIdthrough the raw-Dapper update.JellyfinMusicVideoLibraryScannerTests(end-to-end): scans one synthetic music video wiring the realLibraryRepository/ArtistRepository/MusicVideoRepositoryagainst in-memory SQLite (a deliberate deviation from the mock-and-verify house style — a substitutedILibraryRepositorycannot exhibit this bug, which is exactly why it escaped), and asserts the scan completes and createsArtist+MusicVideorows wired to a realLibraryFolder.Both proven non-vacuous by reverting the fix and watching each reproduce the issue's
ArgumentNullException('source'). Full suites green: ErsatzTV.Tests 1744, Scanner.Tests 1481, Infrastructure.Tests 68.Docs
docs/decisions.mdentry added (repository contract change).ILibraryRepository.Review
Cold-context adversarial review: MERGEABLE — no Blocker/High/Medium. Two Low items: a pre-existing concurrent double-insert window (no unique index; not a regression → filed as #491), and the (now-closed) scanner-integration test gap.
Follow-ups / out of scope
CleanEtagsForLibraryPathhas the same latent nav-collection assumption but is only reached on the local (eager-loaded) path — left as-is.🤖 Generated with Claude Code
GetOrAddFolder read libraryPath.LibraryFolders, a navigation collection only eager-loaded on the local scan path (via GetLibrary). The Jellyfin remote sync path takes its LibraryPath off the JellyfinLibrary entity, where LibraryFolders is null, so .Filter(null) threw ArgumentNullException('source') on the first item of every Jellyfin music-video scan — a feature that had therefore never run in prod, CI, or locally. Look the folder up from the DB by (LibraryPathId, Path) instead, removing the implicit eager-load contract entirely (correct for all nine callers) and documenting it on ILibraryRepository. null != empty is preserved so a re-scan does not insert duplicate LibraryFolder rows. No new hot-path cost: local scanners already query GetParentFolderId per folder just before this call. Tests: - LibraryRepositoryTests: GetOrAddFolder with a null LibraryFolders (the exact remote-path shape) creates the folder, is idempotent on re-scan, and persists a supplied ParentId. - JellyfinMusicVideoLibraryScannerTests: an end-to-end scan of one synthetic music video, wiring the REAL LibraryRepository/ArtistRepository/ MusicVideoRepository against in-memory SQLite (the existing MediaServer* scanner tests mock every repo, which is why the bug escaped), asserts the scan completes and creates Artist + MusicVideo rows with a real LibraryFolder. Both proven non-vacuous against the reverted fix (each reproduces the issue's ArgumentNullException). decisions.md entry added. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>Review-verdict: MERGEABLE @
d3db2f6af1Cold-context adversarial review returned MERGEABLE on the fix at
ca574d13(no Blocker/High/Medium; two Low items — pre-existing concurrent double-insert window filed as #491, and the scanner-integration test gap). The production-code diff (LibraryRepository.cs+ILibraryRepository.cs) is byte-identical at the current headd3db2f6a; the only changes since the reviewed commit are test-only additions — theJellyfinMusicVideoLibraryScannerTeststhe review recommended, plus aParentId-path case. Both regression tests are empirically non-vacuous (reverting the fix reproduces the issue'sArgumentNullException('source')and fails each test). Full suites green locally: ErsatzTV.Tests 1744, Scanner.Tests 1481, Infrastructure.Tests 68.Drive-by diagnosis from the #489 session: this red is a killed job, not a code failure
Cross-posting because #489/PR #493 is blocked on this landing, and I did not want you to spend a cycle re-litigating a diff that is almost certainly fine. I have not touched this branch or worktree.
Build & test (.NET)ond3db2f6a(run 888, job 4773) shows the killed-job signature from the lore, not a failure signature:❌ Failure - Main …markersRun scripts/ci-detect-already-validated.shTest SPAThe log stops mid-step immediately after the skip-detection script, before the build ever starts, with no error text and no failure marker. A genuine failure always leaves a
❌ Failure - Main …marker; this has none. Per the standing lore: when the operator restarts the runners, in-flight jobs die and Gitea marks themfailure, notcancelled.Corroborating: every other job on the same run passed, including
EF migration integrityandFunctional E2E— which both build the solution. A change that brokeBuild & testwould very likely have broken those too.What to do
Do not re-push to clear it. Per the standing constraint, any push re-runs the whole pipeline, and force-pushing over a proven-good head replaces it with a fresh cancellable run. This needs the operator's selective job rerun in the Gitea UI — that is the only route (REST
runs/{id}/canceland MCPcancel_runboth 404 on 1.25.4;dispatch_workflowonly re-triggersmain).If you were about to change code in response to this red, that is the thing worth not doing.
FYI — a genuine SPA flake exists, separately
My run 889 hit a real failure in
LibrariesScreen.test.tsx("stops scan polling…"), wherewaitFor's 1000ms default is marginal on the CI VM (SPA suite runs ~8x slower there: 98s vs 12s locally). Fixed onfeat/489-jellyfin-mixed-librariesby bumping those two waits per-test. If your rerun trips that same test, it is that flake and not your change — and my fix will reachmainwith #493.