fix(460): write null LastScan on disable-sync instead of the MinValue sentinel #601

2026-07-25T13:06:41+02:00

timothy commented

2026-07-25 13:06:41 +02:00

Closes #460.

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, so the wire contract was already correct — this stops new sentinel rows being written.

Safety audit. Every remaining LastScan reader either coalesces (LastScan ?? SystemTime.MinValueUtc) for its own non-nullable scan-comparison, or is the API read-boundary coercion. Audited every reference under ErsatzTV{,.Application,.Core,.Infrastructure,.Scanner,.Mcp} plus web/. There is no Where/OrderBy/GroupBy on LastScan anywhere, so the SQLite-vs-MySQL SQL null-ordering divergence has no surface here. Scan-comparison behavior is unchanged.

Deliberately not done: no second cleanup migration (rows written between #409's migration and this change keep the sentinel at rest; the permanent read coercion is what keeps the contract honest for them, as it must be anyway for a restored/hand-edited DB). LibraryPath.LastScan is left untouched — the flows re-add Paths with original values and its only readers are the local-library scan handlers, so it has no API surface.

Tests: MediaSourceRepositoryDisableSyncTests, one per provider, against the real repository over in-memory SQLite. Non-vacuous — restoring the MinValue writes fails all three.

Docs: 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 ([decisions-edit] per docs.decision-lifecycle). The Rule: line is unchanged, so the generated catalog is byte-identical. Also de-BOMed MediaSourceRepository.cs (fix-as-you-touch, #311) and replaced testing.md's hardcoded ErsatzTV.Tests count, which was off by over a thousand.

Verification: ErsatzTV.Tests 1870 pass, ErsatzTV.Infrastructure.Tests 111 pass, format gate clean, decisions guard + catalog check clean. An independent cold adversarial review found the missing [decisions-edit] token (blocking CI) and three accuracy overstatements; all are folded in.

Closes #460. `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, so the wire contract was already correct — this stops new sentinel rows being written. **Safety audit.** Every remaining `LastScan` reader either coalesces (`LastScan ?? SystemTime.MinValueUtc`) for its own non-nullable scan-comparison, or is the API read-boundary coercion. Audited every reference under `ErsatzTV{,.Application,.Core,.Infrastructure,.Scanner,.Mcp}` plus `web/`. There is **no** `Where`/`OrderBy`/`GroupBy` on `LastScan` anywhere, so the SQLite-vs-MySQL SQL null-ordering divergence has no surface here. Scan-comparison behavior is unchanged. **Deliberately not done:** no second cleanup migration (rows written between #409's migration and this change keep the sentinel at rest; the *permanent* read coercion is what keeps the contract honest for them, as it must be anyway for a restored/hand-edited DB). `LibraryPath.LastScan` is left untouched — the flows re-add `Paths` with original values and its only readers are the local-library scan handlers, so it has no API surface. **Tests:** `MediaSourceRepositoryDisableSyncTests`, one per provider, against the real repository over in-memory SQLite. Non-vacuous — restoring the MinValue writes fails all three. **Docs:** 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 (`[decisions-edit]` per `docs.decision-lifecycle`). The `Rule:` line is unchanged, so the generated catalog is byte-identical. Also de-BOMed `MediaSourceRepository.cs` (fix-as-you-touch, #311) and replaced `testing.md`'s hardcoded `ErsatzTV.Tests` count, which was off by over a thousand. **Verification:** `ErsatzTV.Tests` 1870 pass, `ErsatzTV.Infrastructure.Tests` 111 pass, format gate clean, decisions guard + catalog check clean. An independent cold adversarial review found the missing `[decisions-edit]` token (blocking CI) and three accuracy overstatements; all are folded in.

timothy referenced this pull request

2026-07-25 13:08:34 +02:00

MediaSourceRepository writes MinValue LastScan on disable-sync — normalize to null (#409 follow-up) #460

timothy commented

2026-07-25 13:15:09 +02:00

Independent cold adversarial review, two rounds.

Round 1 (615e00a, first version): BLOCKED — 4 findings. One was a real blocking CI failure (a rationale-prose edit to the media.lastscan-null-boundary record without the [decisions-edit] token, reproduced locally against the exact CI command); one was a factual overstatement ("clean at rest" — this ships no second cleanup migration, so pre-existing sentinel rows remain); two were accuracy nits in the doc and the inline comment. All fixed.

Round 2 (2dae450e, this head): MERGEABLE, no new findings. The reviewer verified the decisions guard and catalog check pass against the real PR base (0952078c, not just HEAD~1), re-ran the full ErsatzTV.Tests suite (1870/1870), re-confirmed the format gate and BOM state, and confirmed the "clean at rest" claim is now absent repo-wide.

The reviewer also refuted part of my own reasoning, which is worth recording: I had claimed SynchronizeJellyfinLibraryByIdHandlerTests already pins the null-LastScan path. It does not — every test in that fixture passes ForceScan: true with a 0 refresh interval, so the coalesced value gates nothing and the test would still pass if the default were changed to DateTime.UtcNow. The null path is executed, not pinned. Done-when box 2 is ticked on the strength of the code audit (independently reproduced by the reviewer: 5 coalescing sites, zero divergent null branch, zero Where/OrderBy/GroupBy on LastScan), not on that test. Adding a handler test that genuinely pins it was judged not worth it for a 3-line data fix; noted as the one remaining coverage gap.

Known follow-through: keep the [decisions-edit] token in whatever commit message survives the merge.

Review-verdict: MERGEABLE @ 2dae450e

Independent cold adversarial review, two rounds. Round 1 (615e00a, first version): BLOCKED — 4 findings. One was a real blocking CI failure (a rationale-prose edit to the media.lastscan-null-boundary record without the [decisions-edit] token, reproduced locally against the exact CI command); one was a factual overstatement ("clean at rest" — this ships no second cleanup migration, so pre-existing sentinel rows remain); two were accuracy nits in the doc and the inline comment. All fixed. Round 2 (2dae450e, this head): MERGEABLE, no new findings. The reviewer verified the decisions guard and catalog check pass against the real PR base (0952078c, not just HEAD~1), re-ran the full ErsatzTV.Tests suite (1870/1870), re-confirmed the format gate and BOM state, and confirmed the "clean at rest" claim is now absent repo-wide. The reviewer also refuted part of my own reasoning, which is worth recording: I had claimed SynchronizeJellyfinLibraryByIdHandlerTests already pins the null-LastScan path. It does not — every test in that fixture passes ForceScan: true with a 0 refresh interval, so the coalesced value gates nothing and the test would still pass if the default were changed to DateTime.UtcNow. The null path is executed, not pinned. Done-when box 2 is ticked on the strength of the code audit (independently reproduced by the reviewer: 5 coalescing sites, zero divergent null branch, zero Where/OrderBy/GroupBy on LastScan), not on that test. Adding a handler test that genuinely pins it was judged not worth it for a 3-line data fix; noted as the one remaining coverage gap. Known follow-through: keep the [decisions-edit] token in whatever commit message survives the merge. Review-verdict: MERGEABLE @ 2dae450e

timothy force-pushed fix/460-lastscan-null from 2dae450e07 to 0081bed4b1

2026-07-25 13:48:30 +02:00

Compare

timothy commented

2026-07-25 13:52:04 +02:00

Round 3 — rebased onto main after PR #602 (the #500 sibling) merged, so the previous verdict at 2dae450e is superseded.

PR #602 merging first made this PR unmergeable. Rebased onto 6f2898d7 (never merged main in, per release.format-as-you-touch-rebase). One conflict, in docs/testing.md, and it was my error: I had claimed the two PRs were kept off the same line to avoid exactly this, but the ErsatzTV.Core.Tests and ErsatzTV.Scanner.Tests rows are ADJACENT, so git's 3-line default context put them in one hunk and they conflicted anyway. Splitting by line is not sufficient — avoiding a conflict requires non-adjacent lines, or one PR owning the whole table. Resolved by taking each side's intended half (Core ~650 from here, Scanner ~1,485 from #602).

The reviewer verified on this head, independently rather than on my word:

The [decisions-edit] token survived the rebase (it lives in a commit message, not the tree) and the guard passes against the REAL base, not HEAD~1: merge-base == origin/main, so CI's mb..head range sees exactly these two commits. decisions-validate OK, catalog --check up to date. This was the specific fragility flagged in round 2, so it was worth confirming.
docs/decisions.md rebased clean and BOTH PRs' record edits coexist — #602's scan.musicvideo-reconciliation Signals/Mechanics edit is present exactly once, and this PR's media.lastscan-null-boundary edits are ~380 lines away. Neither won at the other's expense.
Line-by-line md5 against origin/main proves the testing.md resolution took exactly the two intended halves: the Scanner.Tests and Architecture.Tests rows are byte-identical to main, so no #602 content was lost and no row is half-applied. Every cell in the table was then measured and is accurate.
Every file #602 touched is byte-identical between origin/main and this head, so none of #500's C# is even touched by this branch — a silent revert is impossible by construction, not merely unobserved. The artwork-preferring actor block is intact.
Combined-state suites, the real integration check now that both changes coexist: ErsatzTV.Tests 1871/1871 (1868 on main + 3 here), Scanner.Tests 1488/1488, Core.Tests 653 + 1 skipped, Architecture 5/5, format gate clean.

No new findings. The only open item is the coverage gap agreed in round 2 (null-LastScan scan-comparison is executed but not pinned by any test; Done-when box 2 ticked on the code audit, which the reviewer independently reproduced).

Review-verdict: MERGEABLE @ 0081bed4

Round 3 — rebased onto main after PR #602 (the #500 sibling) merged, so the previous verdict at 2dae450e is superseded. PR #602 merging first made this PR unmergeable. Rebased onto 6f2898d7 (never merged main in, per release.format-as-you-touch-rebase). One conflict, in docs/testing.md, and it was my error: I had claimed the two PRs were kept off the same line to avoid exactly this, but the ErsatzTV.Core.Tests and ErsatzTV.Scanner.Tests rows are ADJACENT, so git's 3-line default context put them in one hunk and they conflicted anyway. Splitting by line is not sufficient — avoiding a conflict requires non-adjacent lines, or one PR owning the whole table. Resolved by taking each side's intended half (Core ~650 from here, Scanner ~1,485 from #602). The reviewer verified on this head, independently rather than on my word: - The [decisions-edit] token survived the rebase (it lives in a commit message, not the tree) and the guard passes against the REAL base, not HEAD~1: merge-base == origin/main, so CI's mb..head range sees exactly these two commits. decisions-validate OK, catalog --check up to date. This was the specific fragility flagged in round 2, so it was worth confirming. - docs/decisions.md rebased clean and BOTH PRs' record edits coexist — #602's scan.musicvideo-reconciliation Signals/Mechanics edit is present exactly once, and this PR's media.lastscan-null-boundary edits are ~380 lines away. Neither won at the other's expense. - Line-by-line md5 against origin/main proves the testing.md resolution took exactly the two intended halves: the Scanner.Tests and Architecture.Tests rows are byte-identical to main, so no #602 content was lost and no row is half-applied. Every cell in the table was then measured and is accurate. - Every file #602 touched is byte-identical between origin/main and this head, so none of #500's C# is even touched by this branch — a silent revert is impossible by construction, not merely unobserved. The artwork-preferring actor block is intact. - Combined-state suites, the real integration check now that both changes coexist: ErsatzTV.Tests 1871/1871 (1868 on main + 3 here), Scanner.Tests 1488/1488, Core.Tests 653 + 1 skipped, Architecture 5/5, format gate clean. No new findings. The only open item is the coverage gap agreed in round 2 (null-LastScan scan-comparison is executed but not pinned by any test; Done-when box 2 ticked on the code audit, which the reviewer independently reproduced). Review-verdict: MERGEABLE @ 0081bed4

timothy merged commit e87285c33d into main

2026-07-25 14:11:35 +02:00

timothy deleted branch fix/460-lastscan-null

2026-07-25 14:11:35 +02:00

timothy referenced this issue from a commit

2026-07-25 14:11:37 +02:00

Merge pull request 'fix(460): write null LastScan on disable-sync instead of the MinValue sentinel' (#601) from fix/460-lastscan-null into main

timothy referenced this pull request

2026-07-25 14:12:09 +02:00

MediaSourceRepository writes MinValue LastScan on disable-sync — normalize to null (#409 follow-up) #460

Sign in to join this conversation.