From 890745d1d4433527aeb37b853f22b4c13412512e Mon Sep 17 00:00:00 2001 From: Timothy Date: Sat, 25 Jul 2026 12:47:14 +0200 Subject: [PATCH 1/2] fix(460): write null LastScan on disable-sync instead of the MinValue sentinel [decisions-edit] MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit MediaSourceRepository's Plex/Jellyfin/Emby remove-and-recreate (disable-sync) flows stamped SystemTime.MinValueUtc into library.LastScan, so normal use kept minting 0001-01-01 sentinel rows. #409 fixed the READ side (the API coerces the sentinel to null and a migration cleaned the historical residue), so the wire contract was already correct — this stops new sentinel rows being written. Safe because every remaining LastScan reader either coalesces (LastScan ?? SystemTime.MinValueUtc) for its own non-nullable scan-comparison needs, or is the API read-boundary coercion itself — audited every reference under ErsatzTV{,.Application,.Core,.Infrastructure,.Scanner,.Mcp} plus web/. There is no Where/OrderBy/GroupBy on LastScan anywhere, so the SQL null-ordering divergence between SQLite and MySQL has no surface here. Scan-comparison behavior is unchanged. Deliberately ships NO second cleanup migration: rows written between #409's NullOutNeverScannedLastScan and this change keep the sentinel at rest, and the permanent read coercion — not a migration — is what keeps the contract honest for them (as it must be anyway for a restored or hand-edited DB). LibraryPath.LastScan is likewise left untouched: the flows re-add Paths with their original values, and its only readers are the local-library scan handlers, so it has no API surface and no remote-scan effect. Regression test per provider (MediaSourceRepositoryDisableSyncTests), proven non-vacuous: restoring the MinValue writes fails all three. [decisions-edit] — the media.lastscan-null-boundary record documented this write as an ONGOING sentinel source and rested its "the coercion is permanent" argument on it, so the rationale prose is corrected in the same PR per docs.decision-lifecycle. The Rule line is unchanged, so the generated decisions/README.md catalog is byte-identical. MediaSourceRepository.cs also loses its UTF-8 BOM (fix-as-you-touch, #311). fixes #460 --- .../Repositories/MediaSourceRepository.cs | 20 +++- .../MediaSourceRepositoryDisableSyncTests.cs | 107 ++++++++++++++++++ docs/decisions.md | 39 ++++--- docs/testing.md | 2 +- 4 files changed, 150 insertions(+), 18 deletions(-) create mode 100644 ErsatzTV.Tests/Integration/MediaSourceRepositoryDisableSyncTests.cs diff --git a/ErsatzTV.Infrastructure/Data/Repositories/MediaSourceRepository.cs b/ErsatzTV.Infrastructure/Data/Repositories/MediaSourceRepository.cs index a66b0d7cf..1282cd1be 100644 --- a/ErsatzTV.Infrastructure/Data/Repositories/MediaSourceRepository.cs +++ b/ErsatzTV.Infrastructure/Data/Repositories/MediaSourceRepository.cs @@ -1,4 +1,4 @@ -using Dapper; +using Dapper; using ErsatzTV.Core; using ErsatzTV.Core.Domain; using ErsatzTV.Core.Emby; @@ -477,7 +477,11 @@ public class MediaSourceRepository(IDbContextFactory dbContextFactory { library.Id = 0; library.ShouldSyncItems = false; - library.LastScan = SystemTime.MinValueUtc; + // ersatztv#460: "never scanned" is null, not the 0001-01-01 MinValue sentinel. Every read path + // either coalesces (`LastScan ?? SystemTime.MinValueUtc`) for its own non-nullable + // scan-comparison, or is the API read-boundary coercion (`NormalizeLastScan`, which maps the + // sentinel to null anyway) — so scan-comparison behavior is unchanged. + library.LastScan = null; } await dbContext.PlexLibraries.AddRangeAsync(libraries); @@ -608,7 +612,11 @@ public class MediaSourceRepository(IDbContextFactory dbContextFactory { library.Id = 0; library.ShouldSyncItems = false; - library.LastScan = SystemTime.MinValueUtc; + // ersatztv#460: "never scanned" is null, not the 0001-01-01 MinValue sentinel. Every read path + // either coalesces (`LastScan ?? SystemTime.MinValueUtc`) for its own non-nullable + // scan-comparison, or is the API read-boundary coercion (`NormalizeLastScan`, which maps the + // sentinel to null anyway) — so scan-comparison behavior is unchanged. + library.LastScan = null; } await dbContext.JellyfinLibraries.AddRangeAsync(libraries); @@ -973,7 +981,11 @@ public class MediaSourceRepository(IDbContextFactory dbContextFactory { library.Id = 0; library.ShouldSyncItems = false; - library.LastScan = SystemTime.MinValueUtc; + // ersatztv#460: "never scanned" is null, not the 0001-01-01 MinValue sentinel. Every read path + // either coalesces (`LastScan ?? SystemTime.MinValueUtc`) for its own non-nullable + // scan-comparison, or is the API read-boundary coercion (`NormalizeLastScan`, which maps the + // sentinel to null anyway) — so scan-comparison behavior is unchanged. + library.LastScan = null; } await dbContext.EmbyLibraries.AddRangeAsync(libraries); diff --git a/ErsatzTV.Tests/Integration/MediaSourceRepositoryDisableSyncTests.cs b/ErsatzTV.Tests/Integration/MediaSourceRepositoryDisableSyncTests.cs new file mode 100644 index 000000000..755b8e4f6 --- /dev/null +++ b/ErsatzTV.Tests/Integration/MediaSourceRepositoryDisableSyncTests.cs @@ -0,0 +1,107 @@ +using ErsatzTV.Core.Domain; +using ErsatzTV.Infrastructure.Data; +using ErsatzTV.Infrastructure.Data.Repositories; +using ErsatzTV.Tests.Support; +using Microsoft.EntityFrameworkCore; +using Microsoft.Extensions.Logging.Abstractions; +using NUnit.Framework; +using Shouldly; + +namespace ErsatzTV.Tests.Integration; + +// ersatztv#460: the disable-sync (remove-and-recreate) flows used to stamp the 0001-01-01 MinValue +// sentinel into Library.LastScan. #409 fixed the READ side (the API coerces the sentinel to null and a +// migration cleaned historical rows), so the wire contract was already correct — these tests pin the +// WRITE side, so the data is clean at rest for every provider and no new sentinel rows are created. +[TestFixture] +public class MediaSourceRepositoryDisableSyncTests +{ + private InMemoryTvContext _db = null!; + private MediaSourceRepository _repository = null!; + + [SetUp] + public async Task SetUp() + { + _db = await InMemoryTvContext.CreateAsync(); + _repository = new MediaSourceRepository(_db.Factory, NullLogger.Instance); + } + + [TearDown] + public async Task TearDown() => await _db.DisposeAsync(); + + [Test] + public async Task DisablePlexLibrarySync_Should_Null_LastScan_Not_Write_MinValue() + { + int libraryId = await Seed(new PlexLibrary + { + Name = "Movies", + MediaKind = LibraryMediaKind.Movies, + MediaSourceId = 1, + Key = "key1", + ShouldSyncItems = true, + LastScan = new DateTime(2026, 7, 1, 12, 0, 0, DateTimeKind.Utc), + Paths = [new LibraryPath { Path = "/data/movies" }] + }); + + await _repository.DisablePlexLibrarySync([libraryId]); + + await using TvContext context = _db.CreateContext(); + PlexLibrary library = await context.PlexLibraries.SingleAsync(l => l.Name == "Movies"); + library.LastScan.ShouldBeNull(); + library.ShouldSyncItems.ShouldBeFalse(); + } + + [Test] + public async Task DisableJellyfinLibrarySync_Should_Null_LastScan_Not_Write_MinValue() + { + int libraryId = await Seed(new JellyfinLibrary + { + Name = "Concerts", + MediaKind = LibraryMediaKind.MusicVideos, + MediaSourceId = 1, + ItemId = "item1", + ShouldSyncItems = true, + LastScan = new DateTime(2026, 7, 1, 12, 0, 0, DateTimeKind.Utc), + Paths = [new LibraryPath { Path = "/data/music" }], + PathInfos = [] + }); + + await _repository.DisableJellyfinLibrarySync([libraryId]); + + await using TvContext context = _db.CreateContext(); + JellyfinLibrary library = await context.JellyfinLibraries.SingleAsync(l => l.Name == "Concerts"); + library.LastScan.ShouldBeNull(); + library.ShouldSyncItems.ShouldBeFalse(); + } + + [Test] + public async Task DisableEmbyLibrarySync_Should_Null_LastScan_Not_Write_MinValue() + { + int libraryId = await Seed(new EmbyLibrary + { + Name = "Shows", + MediaKind = LibraryMediaKind.Shows, + MediaSourceId = 1, + ItemId = "item1", + ShouldSyncItems = true, + LastScan = new DateTime(2026, 7, 1, 12, 0, 0, DateTimeKind.Utc), + Paths = [new LibraryPath { Path = "/data/shows" }], + PathInfos = [] + }); + + await _repository.DisableEmbyLibrarySync([libraryId]); + + await using TvContext context = _db.CreateContext(); + EmbyLibrary library = await context.EmbyLibraries.SingleAsync(l => l.Name == "Shows"); + library.LastScan.ShouldBeNull(); + library.ShouldSyncItems.ShouldBeFalse(); + } + + private async Task Seed(Library library) + { + await using TvContext context = _db.CreateContext(); + await context.AddAsync(library); + await context.SaveChangesAsync(); + return library.Id; + } +} diff --git a/docs/decisions.md b/docs/decisions.md index e08c43f2c..ead57c816 100644 --- a/docs/decisions.md +++ b/docs/decisions.md @@ -2098,26 +2098,30 @@ Refs: #398 (closed), #420, PR #455. ## 2026-07-18 — Never-scanned `LastScan` surfaces as null at the API boundary, not the 0001-01-01 MinValue sentinel (#409) `key: media.lastscan-null-boundary` · `status: active` · `since: 2026-07-18` · `supersedes: none` · `superseded-by: none` **Rule:** A never-scanned `LastScan` surfaces as `null` at the API/MCP boundary, not the `0001-01-01` MinValue sentinel — enforced by an ongoing read-boundary coercion plus a one-time data migration cleanup. -**Signals:** LastScan, MinValue sentinel, API-boundary honesty · paths: `GetAllMediaSourcesForApiHandler.NormalizeLastScan`, `MediaSourceRepository` · issues: #409 -**Mechanics:** `NullOutNeverScannedLastScan` migration; `LibrariesScreen.tsx` `hasScanned` removal +**Signals:** LastScan, MinValue sentinel, API-boundary honesty · paths: `GetAllMediaSourcesForApiHandler.NormalizeLastScan`, `MediaSourceRepository` · issues: #409, #460 +**Mechanics:** `NullOutNeverScannedLastScan` migration; `LibrariesScreen.tsx` `hasScanned` removal; #460 write-side `null` `Library.LastScan` / `LibraryPath.LastScan` are `DateTime?`; a never-scanned library is `null` at -runtime for a freshly-created row. But the `0001-01-01 00:00:00` MinValue sentinel still appears in the -DB from **two** sources — and the second is ongoing, not historical: +runtime for a freshly-created row. But the `0001-01-01 00:00:00` MinValue sentinel appeared in the +DB from **two** sources — both now closed: 1. Old `Reset_*` migrations wrote it via raw SQL (`UPDATE Library SET LastScan = '0001-01-01 00:00:00'`). -2. **Live code still writes it today**: `MediaSourceRepository` sets `library.LastScan = - SystemTime.MinValueUtc` on the Plex/Jellyfin/Emby remove-and-recreate (disable-sync) flows - (`MediaSourceRepository.cs:480/611/976`). So the sentinel keeps being written during normal use. +2. Live code wrote it on the Plex/Jellyfin/Emby remove-and-recreate (disable-sync) flows in + `MediaSourceRepository`. **Closed by #460**: those three writes now set `null`, so normal use no longer + produces new sentinel rows. Safe because every remaining `LastScan` reader either coalesces + (`LastScan ?? SystemTime.MinValueUtc`) for its own non-nullable scan-comparison, or is the read-boundary + coercion below — scan-comparison behavior is unchanged. `GetAllMediaSourcesForApiHandler` projected `l.LastScan` straight onto its `DateTime?` DTO, so that sentinel leaked to API/MCP clients as a fake midnight timestamp — the SPA papered over it with a client-side year<1900 heuristic (#409 first pass). Decision: the API is the right place to be honest, so **never-scanned reports as null** for API/MCP parity with the UI, via two layers: -- **Read-boundary coercion — the load-bearing, ongoing guard** (provider/history-independent): - `GetAllMediaSourcesForApiHandler.NormalizeLastScan` maps any `< 2000-01-01` value to null. Because - source #2 above keeps writing the sentinel, this coercion is *permanent*, not a stopgap — a - migration-only fix would regress the next time a user toggles a library's sync off. +- **Read-boundary coercion — the load-bearing guard** (provider/history-independent): + `GetAllMediaSourcesForApiHandler.NormalizeLastScan` maps any `< 2000-01-01` value to null. It stays + *permanent* even after #460 closed source #2: it is what makes the contract hold for sentinel rows this + codebase is not going to rewrite — a restored or hand-edited DB, and any row written by source #2 + between #409's cleanup migration and #460 — and a migration-only fix would have regressed the next time + a user toggled a library's sync off. - **Data migration** (`NullOutNeverScannedLastScan`, dual-provider): a one-time cleanup of the historical residue — `UPDATE Library/LibraryPath SET LastScan = NULL WHERE LastScan IS NOT NULL AND LastScan < '2000-01-01'`. Data-only (empty `Up`/`Down` otherwise, both `TvContextModelSnapshot.cs` byte-identical). @@ -2132,8 +2136,17 @@ internal scanner code that coalesces to `SystemTime.MinValueUtc` for its own non `DateTimeOffset` scan-comparison needs and never serializes it to a client. The DTO field was already `DateTime?`, so the OpenAPI schema is unchanged (no regen). The SPA's `hasScanned` heuristic in `LibrariesScreen.tsx` was removed — both call sites revert to a plain null/truthy check now that the API -is honest. (Follow-up option, not done here: have `MediaSourceRepository` write `null` instead of -`MinValue` so the data is clean at rest too; the read coercion makes that non-urgent.) +is honest. + +**#460 (write side, 2026-07-25):** the follow-up this record deferred is done — the three disable-sync +writes set `null` instead of `MinValue`, so **no new** sentinel rows are created. Deliberately ships +**no** second cleanup migration: rows written by source #2 between #409's `NullOutNeverScannedLastScan` +and this change still hold the sentinel at rest, and the read coercion above (not a migration) is what +keeps the contract honest for them. Regression-tested per provider in +`MediaSourceRepositoryDisableSyncTests`. `LibraryPath.LastScan` is deliberately left untouched — the +disable-sync flows re-add `Paths` with their original values, and the only readers of the path-level +column are the local-library scan handlers, so it has no API surface and no remote-scan effect. This +*extends* the rule rather than reversing it: the read-boundary coercion is unchanged and authoritative. ## 2026-07-19 — WeightedShuffle SPA: weights edited on the multi-collection, order offered only on classic MultiCollection schedule items; fair-share is a reset not a mode (#404) `key: sched.weightedshuffle-editor` · `status: active` · `since: 2026-07-19` · `supersedes: none` · `superseded-by: none` diff --git a/docs/testing.md b/docs/testing.md index b224d2335..ccf0a69f4 100644 --- a/docs/testing.md +++ b/docs/testing.md @@ -7,7 +7,7 @@ before adding tests, not just `docs/contributing.md` §8 (which now just points | Project | Covers | Notes | |---|---|---| -| `ErsatzTV.Tests` | API controllers + MediatR handlers | In-memory SQLite fixture: a shared `SqliteConnection("Data Source=:memory:;Foreign Keys=False")` kept open + `EnsureCreatedAsync()` (**not** full migration replay) + `PRAGMA foreign_keys=OFF`, then seed; a tiny `IDbContextFactory` wraps `new TvContext(...)`. 828 tests currently. | +| `ErsatzTV.Tests` | API controllers + MediatR handlers | In-memory SQLite fixture: a shared `SqliteConnection("Data Source=:memory:;Foreign Keys=False")` kept open + `EnsureCreatedAsync()` (**not** full migration replay) + `PRAGMA foreign_keys=OFF`, then seed; a tiny `IDbContextFactory` wraps `new TvContext(...)`. ~1,870 tests (approximate on purpose — an exact count goes stale on every PR that adds one; the previous hardcoded 828 was off by over a thousand). | | `ErsatzTV.Core.Tests` | Domain logic, scheduling, IPTV/XMLTV generation | References `ErsatzTV.Application` directly — there is no separate `Application.Tests` project. 543 tests + 1 skipped under `TZ=UTC` (the Block playout golden additionally skips under a non-UTC `TZ`; see Golden-file nets). | | `ErsatzTV.Scanner.Tests` | Library scanning: scan handlers, folder scanners, NFO readers | Handler tests substitute the folder scanners + `ILibraryRepository` and assert the resulting repository writes (e.g. `ScanLocalLibraryHandlerTests` pins which `LastScan` levels a scan records — ersatztv#264). Fakes/`Testably` back the file-system-facing scanners. ~1,485 tests (approximate on purpose — an exact count goes stale on every PR that adds one). Additionally contains `Core/FFmpeg/TranscodingTests` — `[Explicit]` + `[Combinatorial]`, so it never runs in CI or a plain `dotnet test` (it needs real ffmpeg/hardware) and contributes 0 to that count; run it by name when touching the transcoding pipeline. | | `ErsatzTV.Architecture.Tests` | Layering rules via NetArchTest.eNhancedEdition | Core↛Infra/App/EF; FFmpeg↛all; App↛concrete providers. 5 tests. See `docs/contributing.md` §1. | -- 2.47.3 From 0081bed4b1e7ee4431e6a5095eb097920424ab59 Mon Sep 17 00:00:00 2001 From: Timothy Date: Sat, 25 Jul 2026 13:12:38 +0200 Subject: [PATCH 2/2] docs(460): drop the "clean at rest" overstatement from the test header MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to the cold review of 615e00a. The record was corrected to say only that no NEW sentinel rows are created, but the test file's header still carried the disclaimed "the data is clean at rest for every provider" claim — sitting on the very test that supposedly proved it. It now states what the tests actually pin, and names what they cannot speak to (rows written before this change, which keep the sentinel and are covered only by the read coercion). Also corrects docs/testing.md's ErsatzTV.Core.Tests count (543 -> ~650 measured; the neighbouring Scanner.Tests cell is corrected in #602 instead, to keep the two open PRs off the same line). --- .../Integration/MediaSourceRepositoryDisableSyncTests.cs | 4 +++- docs/testing.md | 2 +- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/ErsatzTV.Tests/Integration/MediaSourceRepositoryDisableSyncTests.cs b/ErsatzTV.Tests/Integration/MediaSourceRepositoryDisableSyncTests.cs index 755b8e4f6..c13aeff52 100644 --- a/ErsatzTV.Tests/Integration/MediaSourceRepositoryDisableSyncTests.cs +++ b/ErsatzTV.Tests/Integration/MediaSourceRepositoryDisableSyncTests.cs @@ -12,7 +12,9 @@ namespace ErsatzTV.Tests.Integration; // ersatztv#460: the disable-sync (remove-and-recreate) flows used to stamp the 0001-01-01 MinValue // sentinel into Library.LastScan. #409 fixed the READ side (the API coerces the sentinel to null and a // migration cleaned historical rows), so the wire contract was already correct — these tests pin the -// WRITE side, so the data is clean at rest for every provider and no new sentinel rows are created. +// WRITE side for every provider, so no NEW sentinel rows are created. They do not (and cannot) speak to +// rows already written before this change: those keep the sentinel at rest and are covered only by the +// read-boundary coercion, since this ships no second cleanup migration. [TestFixture] public class MediaSourceRepositoryDisableSyncTests { diff --git a/docs/testing.md b/docs/testing.md index ccf0a69f4..3b40281b8 100644 --- a/docs/testing.md +++ b/docs/testing.md @@ -8,7 +8,7 @@ before adding tests, not just `docs/contributing.md` §8 (which now just points | Project | Covers | Notes | |---|---|---| | `ErsatzTV.Tests` | API controllers + MediatR handlers | In-memory SQLite fixture: a shared `SqliteConnection("Data Source=:memory:;Foreign Keys=False")` kept open + `EnsureCreatedAsync()` (**not** full migration replay) + `PRAGMA foreign_keys=OFF`, then seed; a tiny `IDbContextFactory` wraps `new TvContext(...)`. ~1,870 tests (approximate on purpose — an exact count goes stale on every PR that adds one; the previous hardcoded 828 was off by over a thousand). | -| `ErsatzTV.Core.Tests` | Domain logic, scheduling, IPTV/XMLTV generation | References `ErsatzTV.Application` directly — there is no separate `Application.Tests` project. 543 tests + 1 skipped under `TZ=UTC` (the Block playout golden additionally skips under a non-UTC `TZ`; see Golden-file nets). | +| `ErsatzTV.Core.Tests` | Domain logic, scheduling, IPTV/XMLTV generation | References `ErsatzTV.Application` directly — there is no separate `Application.Tests` project. ~650 tests + 1 skipped under `TZ=UTC` (the Block playout golden additionally skips under a non-UTC `TZ`; see Golden-file nets). | | `ErsatzTV.Scanner.Tests` | Library scanning: scan handlers, folder scanners, NFO readers | Handler tests substitute the folder scanners + `ILibraryRepository` and assert the resulting repository writes (e.g. `ScanLocalLibraryHandlerTests` pins which `LastScan` levels a scan records — ersatztv#264). Fakes/`Testably` back the file-system-facing scanners. ~1,485 tests (approximate on purpose — an exact count goes stale on every PR that adds one). Additionally contains `Core/FFmpeg/TranscodingTests` — `[Explicit]` + `[Combinatorial]`, so it never runs in CI or a plain `dotnet test` (it needs real ffmpeg/hardware) and contributes 0 to that count; run it by name when touching the transcoding pipeline. | | `ErsatzTV.Architecture.Tests` | Layering rules via NetArchTest.eNhancedEdition | Core↛Infra/App/EF; FFmpeg↛all; App↛concrete providers. 5 tests. See `docs/contributing.md` §1. | | `ErsatzTV.FFmpeg.Tests` | FFmpeg command construction | Build a pipeline, assert the exact rendered arg string (`PipelineBuilderBaseTests.cs`). | -- 2.47.3