ErsatzTV.Core/Interfaces/Repositories/ILibraryRepository.cs

@@ -1,4 +1,4 @@
-using ErsatzTV.Core.Domain;
+using ErsatzTV.Core.Domain;
 
 namespace ErsatzTV.Core.Interfaces.Repositories;
 
@@ -23,6 +23,17 @@ public interface ILibraryRepository
     Task SetEtag(LibraryPath libraryPath, Option<LibraryFolder> knownFolder, string path, string etag);
     Task CleanEtagsForLibraryPath(LibraryPath libraryPath);
     Task<Option<int>> GetParentFolderId(LibraryPath libraryPath, string folder, CancellationToken cancellationToken);
+
+    /// <summary>
+    ///     Returns the <see cref="LibraryFolder" /> at <paramref name="folder" /> under
+    ///     <paramref name="libraryPath" />, creating it if it does not yet exist.
+    /// </summary>
+    /// <remarks>
+    ///     The existing folder is looked up from the database by <c>(LibraryPathId, Path)</c>. Callers do
+    ///     <b>not</b> need to eager-load <see cref="LibraryPath.LibraryFolders" /> — the remote (Jellyfin)
+    ///     sync path never does, and relying on that navigation collection here previously NRE'd every
+    ///     Jellyfin music-video scan (ersatztv#488).
+    /// </remarks>
     Task<LibraryFolder> GetOrAddFolder(LibraryPath libraryPath, Option<int> maybeParentFolder, string folder);
     Task UpdateLibraryFolderId(MediaFile mediaFile, int libraryFolderId);
     Task UpdatePath(LibraryPath libraryPath, string normalizedLibraryPath);

ErsatzTV.Infrastructure/Data/Repositories/LibraryRepository.cs

@@ -1,4 +1,4 @@
-using System.IO.Abstractions;
+using System.IO.Abstractions;
 using Dapper;
 using ErsatzTV.Core.Domain;
 using ErsatzTV.Core.Interfaces.Repositories;
@@ -169,11 +169,16 @@ public class LibraryRepository(IFileSystem fileSystem, IDbContextFactory<TvConte
     {
         await using TvContext dbContext = await dbContextFactory.CreateDbContextAsync();
 
-        // load from db or create new folder
-        LibraryFolder knownFolder = await libraryPath.LibraryFolders
-            .Filter(f => f.Path == folder && f.LibraryPathId == libraryPath.Id)
-            .HeadOrNone()
-            .IfNoneAsync(CreateNewFolder(libraryPath, maybeParentFolder, folder));
+        // load from db or create new folder. Look the folder up by (LibraryPathId, Path) rather than
+        // reading libraryPath.LibraryFolders: that navigation collection is only eager-loaded on the
+        // local scan path (via GetLibrary) and is null on the remote (Jellyfin) sync path, which used
+        // to NRE every Jellyfin music-video scan here (ersatztv#488). The local scanners already hit
+        // the db once per folder via GetParentFolderId, so this adds no new query pattern.
+        LibraryFolder knownFolder = await dbContext.LibraryFolders
+            .AsNoTracking()
+            .Filter(f => f.LibraryPathId == libraryPath.Id && f.Path == folder)
+            .FirstOrDefaultAsync()
+            ?? CreateNewFolder(libraryPath, maybeParentFolder, folder);
 
         // update parent folder if not present
         foreach (int parentFolder in maybeParentFolder)

ErsatzTV.Tests/Integration/JellyfinMusicVideoLibraryScannerTests.cs

@@ -0,0 +1,156 @@
+using ErsatzTV.Core;
+using ErsatzTV.Core.Domain;
+using ErsatzTV.Core.Interfaces.Jellyfin;
+using ErsatzTV.Core.Interfaces.Repositories;
+using ErsatzTV.Core.Jellyfin;
+using ErsatzTV.Infrastructure.Data;
+using ErsatzTV.Infrastructure.Data.Repositories;
+using ErsatzTV.Scanner.Core.Interfaces;
+using ErsatzTV.Scanner.Core.Jellyfin;
+using ErsatzTV.Tests.Support;
+using LanguageExt;
+using Microsoft.EntityFrameworkCore;
+using Microsoft.Extensions.Logging.Abstractions;
+using NSubstitute;
+using NUnit.Framework;
+using Shouldly;
+using IFileSystem = System.IO.Abstractions.IFileSystem;
+using Unit = LanguageExt.Unit;
+
+namespace ErsatzTV.Tests.Integration;
+
+// End-to-end regression for ersatztv#488. Unlike the existing MediaServer*LibraryScanner tests (which
+// substitute every repository, and therefore could never have exhibited the null-navigation bug — see the
+// mocked GetOrAddFolder in MovieFolderScannerTests), this test wires the REAL LibraryRepository /
+// ArtistRepository / MusicVideoRepository against in-memory SQLite so the actual crash path runs. The
+// deviation from the mock-and-verify house style is deliberate and required: a substituted
+// ILibraryRepository cannot exhibit the defect this issue is about.
+[TestFixture]
+public class JellyfinMusicVideoLibraryScannerTests
+{
+    private InMemoryTvContext _db = null!;
+
+    [SetUp]
+    public async Task SetUp() => _db = await InMemoryTvContext.CreateAsync();
+
+    [TearDown]
+    public async Task TearDown() => await _db.DisposeAsync();
+
+    [Test]
+    public async Task ScanLibrary_Should_Complete_And_Create_Artist_And_MusicVideo_Rows()
+    {
+        int libraryPathId = await SeedLibraryPath("/data/music");
+
+        // the remote-path shape that used to crash: Paths is populated, LibraryFolders is null
+        var libraryPath = new LibraryPath { Id = libraryPathId, Path = "/data/music", LibraryFolders = null };
+        var library = new JellyfinLibrary
+        {
+            Id = 42,
+            MediaSourceId = 1,
+            ItemId = "lib15",
+            ShouldSyncItems = true,
+            Paths = new List<LibraryPath> { libraryPath }
+        };
+
+        const string VideoPath = "/data/music/artist1/song1.mkv";
+        MusicVideo incoming = BuildIncoming(VideoPath, artistName: "Artist 1", title: "Song 1");
+
+        var apiClient = Substitute.For<IJellyfinApiClient>();
+        apiClient.GetMusicVideoLibraryItems(Arg.Any<string>(), Arg.Any<string>(), Arg.Any<JellyfinLibrary>())
+            .Returns(OneItem(incoming));
+
+        var pathReplacement = Substitute.For<IJellyfinPathReplacementService>();
+        pathReplacement
+            .GetReplacementJellyfinPath(Arg.Any<List<JellyfinPathReplacement>>(), Arg.Any<string>(), Arg.Any<bool>())
+            .Returns(ci => ci.ArgAt<string>(1));
+
+        var mediaSourceRepository = Substitute.For<IMediaSourceRepository>();
+        mediaSourceRepository.GetJellyfinPathReplacements(Arg.Any<int>())
+            .Returns(Task.FromResult(new List<JellyfinPathReplacement>()));
+
+        var scannerProxy = Substitute.For<IScannerProxy>();
+        scannerProxy.UpdateProgress(Arg.Any<decimal>(), Arg.Any<CancellationToken>()).Returns(true);
+        scannerProxy.ReindexMediaItems(Arg.Any<int[]>(), Arg.Any<CancellationToken>()).Returns(true);
+
+        var scanner = new JellyfinMusicVideoLibraryScanner(
+            scannerProxy,
+            apiClient,
+            pathReplacement,
+            mediaSourceRepository,
+            new ArtistRepository(_db.Factory),
+            new MusicVideoRepository(_db.Factory),
+            new LibraryRepository(Substitute.For<IFileSystem>(), _db.Factory),
+            Substitute.For<IMetadataRepository>(),
+            NullLogger<JellyfinMusicVideoLibraryScanner>.Instance);
+
+        var connectionParameters = new JellyfinConnectionParameters("http://jellyfin", "api-key", 1);
+
+        Either<BaseError, Unit> result =
+            await scanner.ScanLibrary(connectionParameters, library, deepScan: false, CancellationToken.None);
+
+        // the scan runs to completion instead of crashing on GetOrAddFolder
+        result.IsRight.ShouldBeTrue(result.Match(Right: _ => "", Left: e => e.Value));
+
+        await using TvContext context = _db.CreateContext();
+
+        // Artist row created from the incoming metadata
+        List<Artist> artists = await context.Artists
+            .Include(a => a.ArtistMetadata)
+            .Where(a => a.LibraryPathId == libraryPathId)
+            .ToListAsync();
+        artists.Count.ShouldBe(1);
+        artists[0].ArtistMetadata.Single().Title.ShouldBe("Artist 1");
+
+        // MusicVideo row created, wired to a real LibraryFolder (proving GetOrAddFolder returned a persisted row)
+        List<MusicVideo> musicVideos = await context.MusicVideos
+            .Include(mv => mv.MediaVersions)
+            .ThenInclude(v => v.MediaFiles)
+            .Where(mv => mv.LibraryPathId == libraryPathId)
+            .ToListAsync();
+        musicVideos.Count.ShouldBe(1);
+        MediaFile file = musicVideos[0].MediaVersions.Single().MediaFiles.Single();
+        file.Path.ShouldBe(VideoPath);
+        file.LibraryFolderId.ShouldNotBeNull();
+
+        // the folder was created by the fixed GetOrAddFolder
+        LibraryFolder folder = await context.LibraryFolders.SingleAsync(f => f.Id == file.LibraryFolderId);
+        folder.Path.ShouldBe("/data/music/artist1");
+        folder.LibraryPathId.ShouldBe(libraryPathId);
+    }
+
+    private static MusicVideo BuildIncoming(string path, string artistName, string title) =>
+        new()
+        {
+            MediaVersions = new List<MediaVersion>
+            {
+                new()
+                {
+                    MediaFiles = new List<MediaFile> { new() { Path = path } },
+                    Streams = new List<MediaStream>()
+                }
+            },
+            MusicVideoMetadata = new List<MusicVideoMetadata>
+            {
+                new()
+                {
+                    Title = title,
+                    Artists = new List<MusicVideoArtist> { new() { Name = artistName } }
+                }
+            }
+        };
+
+    private static async IAsyncEnumerable<Tuple<MusicVideo, int>> OneItem(MusicVideo musicVideo)
+    {
+        yield return new Tuple<MusicVideo, int>(musicVideo, 1);
+        await Task.CompletedTask;
+    }
+
+    private async Task<int> SeedLibraryPath(string path)
+    {
+        await using TvContext context = _db.CreateContext();
+        var libraryPath = new LibraryPath { Path = path };
+        await context.LibraryPaths.AddAsync(libraryPath);
+        await context.SaveChangesAsync();
+        return libraryPath.Id;
+    }
+}

ErsatzTV.Tests/Integration/LibraryRepositoryTests.cs

@@ -0,0 +1,116 @@
+using ErsatzTV.Core.Domain;
+using ErsatzTV.Infrastructure.Data;
+using ErsatzTV.Infrastructure.Data.Repositories;
+using ErsatzTV.Tests.Support;
+using LanguageExt;
+using Microsoft.EntityFrameworkCore;
+using NSubstitute;
+using NUnit.Framework;
+using Shouldly;
+using IFileSystem = System.IO.Abstractions.IFileSystem;
+
+namespace ErsatzTV.Tests.Integration;
+
+[TestFixture]
+public class LibraryRepositoryTests
+{
+    private InMemoryTvContext _db = null!;
+    private LibraryRepository _repository = null!;
+
+    [SetUp]
+    public async Task SetUp()
+    {
+        _db = await InMemoryTvContext.CreateAsync();
+        _repository = new LibraryRepository(Substitute.For<IFileSystem>(), _db.Factory);
+    }
+
+    [TearDown]
+    public async Task TearDown() => await _db.DisposeAsync();
+
+    // Regression for ersatztv#488: the Jellyfin (remote) sync path takes its LibraryPath straight off the
+    // JellyfinLibrary entity, so LibraryPath.LibraryFolders is never eager-loaded (null). GetOrAddFolder
+    // used to read that navigation collection directly and threw ArgumentNullException on the very first
+    // item of every Jellyfin music-video scan. The repository must resolve the folder from the database
+    // instead, so an unloaded collection is not a precondition.
+    [Test]
+    public async Task GetOrAddFolder_Should_Create_Folder_When_LibraryFolders_Not_Loaded()
+    {
+        int libraryPathId = await SeedLibraryPath("/data/music");
+
+        // mimic the Jellyfin path: Paths is populated, but LibraryFolders was never included
+        var libraryPath = new LibraryPath { Id = libraryPathId, Path = "/data/music", LibraryFolders = null };
+
+        LibraryFolder result =
+            await _repository.GetOrAddFolder(libraryPath, Option<int>.None, "/data/music/artist1");
+
+        result.ShouldNotBeNull();
+        result.Id.ShouldBeGreaterThan(0);
+        result.Path.ShouldBe("/data/music/artist1");
+        result.LibraryPathId.ShouldBe(libraryPathId);
+
+        await using TvContext context = _db.CreateContext();
+        List<LibraryFolder> folders = await context.LibraryFolders
+            .Where(f => f.LibraryPathId == libraryPathId)
+            .ToListAsync();
+        folders.Count.ShouldBe(1);
+        folders[0].Path.ShouldBe("/data/music/artist1");
+    }
+
+    // Re-scanning must be idempotent: a second GetOrAddFolder for the same path returns the existing row
+    // rather than inserting a duplicate LibraryFolder (there is no unique constraint behind it).
+    [Test]
+    public async Task GetOrAddFolder_Should_Be_Idempotent_On_Rescan()
+    {
+        int libraryPathId = await SeedLibraryPath("/data/music");
+        var libraryPath = new LibraryPath { Id = libraryPathId, Path = "/data/music", LibraryFolders = null };
+
+        LibraryFolder first =
+            await _repository.GetOrAddFolder(libraryPath, Option<int>.None, "/data/music/artist1");
+        LibraryFolder second =
+            await _repository.GetOrAddFolder(libraryPath, Option<int>.None, "/data/music/artist1");
+
+        second.Id.ShouldBe(first.Id);
+
+        await using TvContext context = _db.CreateContext();
+        int count = await context.LibraryFolders.CountAsync(f => f.LibraryPathId == libraryPathId);
+        count.ShouldBe(1);
+    }
+
+    // Covers the maybeParentFolder = Some(...) branch: on a folder already in the db, the parent id is
+    // persisted through the raw Dapper UPDATE against the no-tracking entity (not change tracking), and the
+    // returned object reflects it. Guards the AsNoTracking + raw-UPDATE interaction the DB-lookup fix relies on.
+    [Test]
+    public async Task GetOrAddFolder_Should_Persist_ParentId_On_Existing_Folder()
+    {
+        int libraryPathId = await SeedLibraryPath("/data/music");
+        var libraryPath = new LibraryPath { Id = libraryPathId, Path = "/data/music", LibraryFolders = null };
+
+        LibraryFolder parent =
+            await _repository.GetOrAddFolder(libraryPath, Option<int>.None, "/data/music");
+        // first pass creates the child with no parent
+        LibraryFolder child =
+            await _repository.GetOrAddFolder(libraryPath, Option<int>.None, "/data/music/artist1");
+        child.ParentId.ShouldBeNull();
+
+        // second pass supplies the parent — the existing row must be updated, not duplicated
+        LibraryFolder updated =
+            await _repository.GetOrAddFolder(libraryPath, Option<int>.Some(parent.Id), "/data/music/artist1");
+        updated.Id.ShouldBe(child.Id);
+        updated.ParentId.ShouldBe(parent.Id);
+
+        await using TvContext context = _db.CreateContext();
+        LibraryFolder persisted = await context.LibraryFolders.SingleAsync(f => f.Id == child.Id);
+        persisted.ParentId.ShouldBe(parent.Id);
+        int count = await context.LibraryFolders.CountAsync(f => f.Path == "/data/music/artist1");
+        count.ShouldBe(1);
+    }
+
+    private async Task<int> SeedLibraryPath(string path)
+    {
+        await using TvContext context = _db.CreateContext();
+        var libraryPath = new LibraryPath { Path = path };
+        await context.LibraryPaths.AddAsync(libraryPath);
+        await context.SaveChangesAsync();
+        return libraryPath.Id;
+    }
+}

docs/decisions.md

@@ -2221,3 +2221,35 @@ for its whole slot. This closes that gap.
   `PlayoutItemNotAvailableFromMediaServer` and no metadata call; available → `Right` with the `/media/plex`
   URL). The fail-open contract itself stays pinned by `HttpRemoteStreamProberTests`. Proven non-vacuous by
   neutralizing the probe guard and watching the unavailable assertion flip to `Right`.
+
+## 2026-07-20 — `ILibraryRepository.GetOrAddFolder` resolves the folder from the DB, not the caller's `LibraryPath.LibraryFolders` navigation (#488)
+
+`GetOrAddFolder` looked the existing folder up by reading `libraryPath.LibraryFolders` in memory. That
+navigation collection is only eager-loaded on the **local** scan path — `LibraryRepository.GetLibrary`
+`.Include(l => l.Paths).ThenInclude(p => p.LibraryFolders)` — which every `*FolderScanner` goes through. The
+**remote** (Jellyfin) sync path takes its `LibraryPath` straight off the `JellyfinLibrary` entity, whose
+`Paths[].LibraryFolders` is **null**, so `.Filter(...)` on it hit `Enumerable.Where(null, …)` and threw
+`ArgumentNullException('source')` on the very first item of every Jellyfin music-video scan.
+`JellyfinMusicVideoLibraryScanner` is the only *remote* caller of `GetOrAddFolder` (it does not derive from the
+`MediaServer*LibraryScanner` base that the movie/TV/other-video remote scanners share), which is why no other
+remote scanner tripped it, and the feature had never run in prod, CI, or locally.
+
+- **Fix the repository, not the caller.** The lookup now queries `dbContext.LibraryFolders` by
+  `(LibraryPathId, Path)` (the method already opened a `dbContext` for the insert). This removes the *implicit,
+  undocumented loading contract* entirely — correct for all nine callers — rather than the caller-side option
+  (eager-load `LibraryFolders` on the Jellyfin path), which would have left the contract intact for the next
+  remote scanner to trip. The contract change is now **stated** on `ILibraryRepository.GetOrAddFolder`.
+- **`null` ≠ empty — do not coerce.** A `null` navigation meant *unknown*, not *known-absent*; treating it as an
+  empty collection would fall through to `CreateNewFolder` and insert a duplicate `LibraryFolder` for a path
+  that already exists (there is no unique constraint behind it). Querying the DB distinguishes the two.
+- **No new hot-path cost.** Every local scanner already calls `GetParentFolderId` (a DB query with the same
+  `(LibraryPathId, …)` shape) once per folder immediately before `GetOrAddFolder`, so the folder granularity was
+  never served purely from memory; this adds one indexed lookup per folder, not a per-file query. As a bonus the
+  DB lookup also sees folders created earlier in the *same* scan, which the load-time in-memory snapshot could
+  not.
+- **Tests.** `LibraryRepositoryTests` (real in-memory-SQLite `TvContext`) drives `GetOrAddFolder` with
+  `LibraryPath.LibraryFolders == null` — the exact remote-path shape — and asserts it creates the folder and is
+  idempotent on re-scan (no duplicate row). Proven non-vacuous by restoring the navigation-collection lookup and
+  watching both tests fail with the issue's `ArgumentNullException('source')`. Note: `CleanEtagsForLibraryPath`
+  still reads `libraryPath.LibraryFolders` directly, but it is only reached on the local (eager-loaded) path, so
+  it is not affected; left as-is (out of #488 scope).