fix(264): record library-level LastScan after a successful local scan #408

Merged

timothy merged 3 commits from fix/264-library-lastscan into main 2026-07-17 15:00:58 +02:00

Conversation 2 Commits 3 Files Changed 3

+217 -5

timothy commented 2026-07-17 14:18:57 +02:00

Owner

Copy Link

Copy Source

fixes #264

Problem

Local libraries permanently showed "Never scanned" in the SPA libraries hub regardless of how many successful scans ran — while the item count updated correctly, which is what made it read as a display glitch rather than a missing write.

Root cause

ScanLocalLibraryHandler wrote only the path-level LibraryPath.LastScan — the value that gates the per-path refresh interval — and never the library-level Library.LastScan. The read API GetAllMediaSourcesForApiHandler populates the hub's scan-time badge from Library.LastScan, so that column stayed NULL forever.

This is local-library-specific: all three remote scanners (SynchronizeJellyfinLibraryByIdHandler, Emby, Plex) already set the library-level value on success. Both the read API and this scanner predate #202; the #202 SPA hub merely made the missing value visible, so this is not a #202 regression.

I enumerated every producer of LastScan rather than trusting the issue's cited line (the "enumerate every producer" rule from #235): 4 write sites total — 3 remote (library-level, correct) + 1 local (path-level only, the bug). Confirmed the diagnosis still holds on current main before acting on it.

Fix

Mirror the remote scanners' semantics — record the library-level scan time only when the scan actually ran and every path that ran succeeded:

• a skipped scan (unforced, refresh interval not elapsed) records nothing — there is no new scan to report;
• a partially failed scan records nothing at the library level, but successful paths still get their path-level timestamp (preserving the existing refresh-interval behaviour). A partially-scanned library must not claim a successful scan time.

Tests

New ErsatzTV.Scanner.Tests/Application/MediaSources/ScanLocalLibraryHandlerTests.cs (4 tests), following the existing SynchronizeJellyfinLibraryByIdHandlerTests pattern: success sets it, single-path failure doesn't, a later path failing doesn't (while the earlier good path still gets its path-level write), and a skipped scan doesn't.

Proven non-vacuous via negative control: inverting the guard to if (!scanned && anyFailed) makes Should_Set_Library_LastScan_After_Successful_Scan fail, with Build succeeded / 0 Error(s) confirmed so the run isn't a stale-dll false pass (the #250 trap — an if (true) stub would have left the flag unread → CS0219 → warnings-as-errors → silently stale dll).

Local verification, all green:

• ErsatzTV.Scanner.Tests — 1471 passed
• ErsatzTV.Tests — 1648 passed
• ErsatzTV.Core.Tests — 548 passed, 1 skipped

Notes

• Formatting: de-BOM'd the touched handler per the fix-as-you-touch charset gate (#311). Both touched files verified clean; the remaining legacy BOM files in the project are untouched and out of scope.
• Docs: no update required — no /api/* surface, route, DTO, or convention changes. (Stated exemption per the docs-update rule.)
• Live-E2E: docs/e2e-local.md scopes the mandatory live-E2E to API write-path handlers (a POST/PUT/DELETE /api/* command that mutates then reloads through the read path, whose lazy-enumeration blind spot unit tests share). This is a background scanner handler persisting via a plain Dapper UPDATE Library SET LastScan, with no lazy LanguageExt materialisation on the write path — so it falls outside that rule. Verification of the real scan → badge loop is noted separately below.

fixes #264 ## Problem Local libraries permanently showed **"Never scanned"** in the SPA libraries hub regardless of how many successful scans ran — while the item count updated correctly, which is what made it read as a display glitch rather than a missing write. ## Root cause `ScanLocalLibraryHandler` wrote only the **path-level** `LibraryPath.LastScan` — the value that gates the per-path refresh interval — and never the **library-level** `Library.LastScan`. The read API `GetAllMediaSourcesForApiHandler` populates the hub's scan-time badge from `Library.LastScan`, so that column stayed `NULL` forever. This is local-library-specific: all three remote scanners (`SynchronizeJellyfinLibraryByIdHandler`, `Emby`, `Plex`) already set the library-level value on success. Both the read API and this scanner predate #202; the #202 SPA hub merely made the missing value visible, so this is **not a #202 regression**. I enumerated every producer of `LastScan` rather than trusting the issue's cited line (the "enumerate every producer" rule from #235): 4 write sites total — 3 remote (library-level, correct) + 1 local (path-level only, the bug). Confirmed the diagnosis still holds on current `main` before acting on it. ## Fix Mirror the remote scanners' semantics — record the library-level scan time only when the scan **actually ran** and **every path that ran succeeded**: - a **skipped** scan (unforced, refresh interval not elapsed) records nothing — there is no new scan to report; - a **partially failed** scan records nothing at the library level, but successful paths still get their path-level timestamp (preserving the existing refresh-interval behaviour). A partially-scanned library must not claim a successful scan time. ## Tests New `ErsatzTV.Scanner.Tests/Application/MediaSources/ScanLocalLibraryHandlerTests.cs` (4 tests), following the existing `SynchronizeJellyfinLibraryByIdHandlerTests` pattern: success sets it, single-path failure doesn't, a *later* path failing doesn't (while the earlier good path still gets its path-level write), and a skipped scan doesn't. **Proven non-vacuous via negative control**: inverting the guard to `if (!scanned && anyFailed)` makes `Should_Set_Library_LastScan_After_Successful_Scan` fail, with `Build succeeded / 0 Error(s)` confirmed so the run isn't a stale-dll false pass (the #250 trap — an `if (true)` stub would have left the flag unread → CS0219 → warnings-as-errors → silently stale dll). Local verification, all green: - `ErsatzTV.Scanner.Tests` — 1471 passed - `ErsatzTV.Tests` — 1648 passed - `ErsatzTV.Core.Tests` — 548 passed, 1 skipped ## Notes - **Formatting**: de-BOM'd the touched handler per the fix-as-you-touch charset gate (#311). Both touched files verified clean; the remaining legacy BOM files in the project are untouched and out of scope. - **Docs**: no update required — no `/api/*` surface, route, DTO, or convention changes. (Stated exemption per the docs-update rule.) - **Live-E2E**: `docs/e2e-local.md` scopes the mandatory live-E2E to **API write-path handlers** (a `POST`/`PUT`/`DELETE` `/api/*` command that mutates then reloads through the read path, whose lazy-enumeration blind spot unit tests share). This is a background scanner handler persisting via a plain Dapper `UPDATE Library SET LastScan`, with no lazy LanguageExt materialisation on the write path — so it falls outside that rule. Verification of the real scan → badge loop is noted separately below.

timothy added 1 commit 2026-07-17 14:18:58 +02:00

fix(264): record library-level LastScan after a successful local scan ... 5c8dd64acf

Local libraries permanently showed "Never scanned" in the SPA libraries
hub regardless of successful scans, even as item counts updated.

Root cause: ScanLocalLibraryHandler wrote only the path-level
LibraryPath.LastScan (which gates the per-path refresh interval) and
never the library-level Library.LastScan. The read API
(GetAllMediaSourcesForApiHandler) populates the hub's scan-time badge
from Library.LastScan, so that value stayed null forever. The three
remote scanners (Jellyfin/Emby/Plex) already set the library-level
value; only the local scanner did not. Both sides predate #202 — the
SPA hub merely made the missing value visible.

Mirror the remote scanners' semantics: record the library-level scan
time only when the scan actually ran and every path that ran succeeded,
so a skipped (unforced, interval not elapsed) or partially-failed scan
does not claim a successful scan time.

Also de-BOM the touched handler per the fix-as-you-touch charset gate
(#311).

fixes #264

timothy added 1 commit 2026-07-17 14:27:56 +02:00

fix(264): log local path scan errors (review finding) + map Scanner.Tests ... 986ccfaf6c

Adversarial review (MERGEABLE, no blockers) raised two items worth
folding in rather than deferring:

Medium — ScanLocalLibraryHandler silently swallowed path scan errors,
while the three remote scanners it mirrors all log result.LeftToSeq().
That mattered less when a failed path only skipped the path-level
LastScan, but the previous commit makes a failed path suppress the
library-level scan time too — so the user would see exactly the #264
symptom ("Never scanned") with nothing in the log explaining why. That
is a diagnosis dead-end of the same class as the bug being fixed, so
log it here rather than file a follow-up. No test: the sibling
Synchronize*LibraryByIdHandlerTests don't assert on logging either, and
LogError is an extension method that NSubstitute can't cleanly verify.

Low — docs/testing.md bills itself as the authoritative map of what each
test project covers but omitted ErsatzTV.Scanner.Tests entirely (1471
pre-existing tests). This PR adds a file to that project, so add the row
and include it in the per-PR verification gate.

timothy referenced this pull request 2026-07-17 14:31:29 +02:00

Never-scanned local libraries render "Last scan 12:00 AM" instead of "Never scanned" (LastScan seeded as DateTime.MinValue, not NULL) #409

timothy commented 2026-07-17 14:31:48 +02:00

Author

Owner

Copy Link

Copy Source

Live-E2E: ran it anyway, and it earned its keep

The PR body notes this change is outside docs/e2e-local.md's mandatory live-E2E rule (that rule targets API write-path handlers with the lazy-enumeration blind spot; this is a background scanner writing via plain Dapper SQL). I ran the full loop regardless, because the entire bug is an observable-badge bug — claiming a display fix works without looking at the display is exactly the gap the rule exists to close.

Recipe: scripts/e2e-local.sh on a fresh config dir + the docs/e2e-local.md seeding recipe (two 2-second testsrc MKVs under one show), LibraryPath row attached to the built-in Shows library (Id=2), scan triggered through the real endpoint.

Before the scan:

GET /api/v1/media-sources → Shows lastScan='0001-01-01T00:00:00' itemCount=0
sqlite> SELECT LastScan FROM Library WHERE Id=2;  →  0001-01-01 00:00:00

Scan: POST /api/v1/libraries/2/scan → HTTP 202, 2 episodes scanned.

After the scan:

sqlite> Library.LastScan      = 2026-07-17 12:29:55.99406      ← the fix; previously never written
sqlite> LibraryPath.LastScan  = 2026-07-17 12:29:55.992212     ← pre-existing behaviour, unchanged
GET /api/v1/media-sources → Shows lastScan='2026-07-17T12:29:55.99406' itemCount=4

The library-level value is written and round-trips through the read path the SPA actually consumes. Done-when box ticked.

It also corrected the issue's premise, and surfaced a second bug → #409

#264 asserts the value "stays null" and that local libraries "permanently show Never scanned". Live state says otherwise: a never-scanned local library's LastScan is DateTime.MinValue, not NULL — a fresh DB seeds six of the seven built-in local libraries with 0001-01-01 00:00:00 and only Remote Streams with NULL.

That matters because LibrariesScreen.tsx:397 branches on truthiness, and "0001-01-01T00:00:00" is a truthy string. So before a first scan the badge doesn't say "Never scanned" — it renders "Last scan 12:00 AM", asserting a scan that never happened.

This does not change this PR: the underlying defect (the local scanner never wrote library-level LastScan) was real, is confirmed, and is fixed and verified above. The residual display bug is pre-existing, orthogonal, and now filed as #409 with three candidate fix directions rather than scope-crept into this diff.

Worth recording as a lesson: the issue's stated symptom was written from a plausible reading of the code, not from the DB — and only live E2E on a non-mandatory path caught it.

## Live-E2E: ran it anyway, and it earned its keep The PR body notes this change is **outside** `docs/e2e-local.md`'s mandatory live-E2E rule (that rule targets API write-path handlers with the lazy-enumeration blind spot; this is a background scanner writing via plain Dapper SQL). I ran the full loop regardless, because the entire bug *is* an observable-badge bug — claiming a display fix works without looking at the display is exactly the gap the rule exists to close. **Recipe**: `scripts/e2e-local.sh` on a fresh config dir + the `docs/e2e-local.md` seeding recipe (two 2-second `testsrc` MKVs under one show), `LibraryPath` row attached to the built-in Shows library (Id=2), scan triggered through the real endpoint. **Before** the scan: ``` GET /api/v1/media-sources → Shows lastScan='0001-01-01T00:00:00' itemCount=0 sqlite> SELECT LastScan FROM Library WHERE Id=2; → 0001-01-01 00:00:00 ``` **Scan**: `POST /api/v1/libraries/2/scan` → `HTTP 202`, 2 episodes scanned. **After** the scan: ``` sqlite> Library.LastScan = 2026-07-17 12:29:55.99406 ← the fix; previously never written sqlite> LibraryPath.LastScan = 2026-07-17 12:29:55.992212 ← pre-existing behaviour, unchanged GET /api/v1/media-sources → Shows lastScan='2026-07-17T12:29:55.99406' itemCount=4 ``` The library-level value is written and round-trips through the read path the SPA actually consumes. **Done-when box ticked.** ### It also corrected the issue's premise, and surfaced a second bug → #409 #264 asserts the value "stays null" and that local libraries "permanently show *Never scanned*". Live state says otherwise: a never-scanned local library's `LastScan` is **`DateTime.MinValue`, not `NULL`** — a fresh DB seeds six of the seven built-in local libraries with `0001-01-01 00:00:00` and only Remote Streams with `NULL`. That matters because `LibrariesScreen.tsx:397` branches on **truthiness**, and `"0001-01-01T00:00:00"` is a truthy string. So before a first scan the badge doesn't say "Never scanned" — it renders **"Last scan 12:00 AM"**, asserting a scan that never happened. This does **not** change this PR: the underlying defect (the local scanner never wrote library-level `LastScan`) was real, is confirmed, and is fixed and verified above. The residual display bug is pre-existing, orthogonal, and now filed as **#409** with three candidate fix directions rather than scope-crept into this diff. Worth recording as a lesson: the issue's stated symptom was written from a plausible reading of the code, not from the DB — and only live E2E on a *non-mandatory* path caught it.

timothy added 1 commit 2026-07-17 14:35:19 +02:00

test(264): pin the path-scan-error log; correct testing.md accuracy ... 71708633f5

Re-review of the previous fix commit (986ccfaf, verdict MERGEABLE)
raised one fair hit and one claim that did not survive checking.

Fair hit — that commit's message asserted "LogError is an extension
method that NSubstitute can't cleanly verify" as the reason for adding
no test. That is FALSE, and this repo disproves it:
ErsatzTV.Tests' ReleaseMemoryHandlerTests.ShouldHaveLogged asserts on
ILogger via ReceivedCalls(), which works precisely because the LogError
extension dispatches to the substituted ILogger.Log. Reusing that idiom
here costs 4 lines, so pin the log the previous commit added. Verified
non-vacuous: asserting a message the handler never logs fails the test.

Not applied — the same review called the row's "1471 tests" misleading
on the grounds that TranscodingTests is [Explicit] and contributes most
of that count. TranscodingTests is indeed [Explicit], but filtering it
out yields exactly 1471, so 1471 is already the runnable count and the
row was accurate. Kept the number; documented TranscodingTests as
[Explicit]/opt-in instead, since it was a genuine omission from a table
that claims to be authoritative.

Also rewrap the verification-gate paragraph the previous commit left
over-long, and name the two always-run projects instead of "both".

Deferred (filed separately): a canceled local scan now logs at ERROR
per path via this log. It mirrors the remote handlers exactly, so
diverging here would be the inconsistency, not the fix.

timothy referenced this pull request 2026-07-17 14:35:39 +02:00

Scan cancellation is logged at ERROR ("Scan was canceled") across all four scan handlers #410

timothy commented 2026-07-17 14:40:37 +02:00

Author

Owner

Copy Link

Copy Source

Independent review — three passes, clean at current head

Review-verdict: MERGEABLE @ 71708633f5

Per the "re-review the fix commit, not just the initial diff" rule (#242), each commit got its own cold-context pass rather than one review of the opening diff:

Commit	Scope	Verdict
5c8dd64a	the fix + 4 tests	MERGEABLE — no defects
986ccfaf	error logging + testing.md row	MERGEABLE — 1 fair hit, 1 disputed claim
71708633	logging test + doc accuracy	MERGEABLE — no blockers

What review changed

Conceded (review was right, I was wrong). 986ccfaf added the error log with no test, justified as "LogError is an extension method that NSubstitute can't cleanly verify." That is false, and this repo disproves it: ErsatzTV.Tests/Application/Maintenance/ReleaseMemoryHandlerTests.cs:45-48 already asserts on ILogger via ReceivedCalls() — it works precisely because the extension dispatches to the substituted ILogger.Log. 71708633 adopts that idiom. Verified non-vacuous by mutation: deleting the LogError call (keeping the build valid, so no stale-dll false pass) fails exactly one test — the right one:

Failed Should_Not_Set_Library_LastScan_When_A_Path_Scan_Fails [21 ms]
Failed! - Failed: 1, Passed: 3, Skipped: 0, Total: 4

Declined (review was wrong — verified, not asserted). The 986ccfaf pass called the row's "1471 tests" inflated by ~1379 non-running [Explicit] TranscodingTests permutations. Checked both ways:

dotnet test ErsatzTV.Scanner.Tests                                    → Passed: 1471, Skipped: 0
dotnet test ... --filter "FullyQualifiedName!~TranscodingTests"       → Passed: 1471, Skipped: 0

Identical. Fixture-level [Explicit] means those tests never enter the plan at all, so they contribute 0 to both the total and the skip count — had the number been inflated, the exclude-filter run would have dropped below 1471. It didn't move. The number stands; 71708633 instead documents TranscodingTests as opt-in, which was the genuine omission underneath the wrong finding. The final pass independently adjudicated this and confirmed the row's claims.

Verified in the final pass

• ElementAtOrDefault(2) correctly targets state in ILogger.Log<TState>(logLevel, eventId, state, exception, formatter); cannot throw; does not false-positive against the other log on that path (LogDebug "Scan of library {Name} completed" renders differently).
• No test-hygiene leak — _logger is re-created per test in [SetUp].
• Both touched files BOM-free; the #311 fix-as-you-touch gate is satisfied (the CHARSET errors dotnet format reports are pre-existing legacy files this PR doesn't touch).

Accepted as non-blocking

• Low — ShouldHaveLogged matches the formatted state only, so it wouldn't catch LogError being downgraded to LogDebug. Inherited verbatim from the cited ReleaseMemoryHandlerTests precedent; tightening it here would diverge from the established idiom for marginal gain.
• Low → filed as #410 — a canceled local scan now logs at ERROR per path (ScanCanceled is an ordinary BaseError). Deliberately not fixed here: this PR made the local handler consistent with the three remote handlers, which have logged cancellation this way all along. Diverging in this PR would be the inconsistency; #410 fixes all four together.

Also filed #409 from live-E2E — never-scanned local libraries render "Last scan 12:00 AM" (MinValue is truthy in JS), the mirror image of this bug.

## Independent review — three passes, clean at current head Review-verdict: MERGEABLE @ 71708633f565e6b9f08602a9431b61e39e31cf97 Per the "re-review the fix commit, not just the initial diff" rule (#242), each commit got its own cold-context pass rather than one review of the opening diff: | Commit | Scope | Verdict | |---|---|---| | `5c8dd64a` | the fix + 4 tests | MERGEABLE — no defects | | `986ccfaf` | error logging + testing.md row | MERGEABLE — 1 fair hit, 1 disputed claim | | `71708633` | logging test + doc accuracy | **MERGEABLE — no blockers** | ### What review changed **Conceded (review was right, I was wrong).** `986ccfaf` added the error log with no test, justified as *"LogError is an extension method that NSubstitute can't cleanly verify."* That is false, and this repo disproves it: `ErsatzTV.Tests/Application/Maintenance/ReleaseMemoryHandlerTests.cs:45-48` already asserts on `ILogger` via `ReceivedCalls()` — it works precisely *because* the extension dispatches to the substituted `ILogger.Log`. `71708633` adopts that idiom. Verified non-vacuous by mutation: deleting the `LogError` call (keeping the build valid, so no stale-dll false pass) fails exactly one test — the right one: ``` Failed Should_Not_Set_Library_LastScan_When_A_Path_Scan_Fails [21 ms] Failed! - Failed: 1, Passed: 3, Skipped: 0, Total: 4 ``` **Declined (review was wrong — verified, not asserted).** The `986ccfaf` pass called the row's "1471 tests" inflated by ~1379 non-running `[Explicit]` `TranscodingTests` permutations. Checked both ways: ``` dotnet test ErsatzTV.Scanner.Tests → Passed: 1471, Skipped: 0 dotnet test ... --filter "FullyQualifiedName!~TranscodingTests" → Passed: 1471, Skipped: 0 ``` Identical. Fixture-level `[Explicit]` means those tests never enter the plan at all, so they contribute 0 to both the total *and* the skip count — had the number been inflated, the exclude-filter run would have dropped below 1471. It didn't move. The number stands; `71708633` instead documents `TranscodingTests` as opt-in, which was the genuine omission underneath the wrong finding. The final pass independently adjudicated this and confirmed the row's claims. ### Verified in the final pass - `ElementAtOrDefault(2)` correctly targets `state` in `ILogger.Log<TState>(logLevel, eventId, state, exception, formatter)`; cannot throw; does not false-positive against the other log on that path (`LogDebug "Scan of library {Name} completed"` renders differently). - No test-hygiene leak — `_logger` is re-created per test in `[SetUp]`. - Both touched files BOM-free; the #311 fix-as-you-touch gate is satisfied (the CHARSET errors `dotnet format` reports are pre-existing legacy files this PR doesn't touch). ### Accepted as non-blocking - **Low** — `ShouldHaveLogged` matches the formatted state only, so it wouldn't catch `LogError` being downgraded to `LogDebug`. Inherited verbatim from the cited `ReleaseMemoryHandlerTests` precedent; tightening it here would diverge from the established idiom for marginal gain. - **Low → filed as #410** — a *canceled* local scan now logs at ERROR per path (`ScanCanceled` is an ordinary `BaseError`). Deliberately not fixed here: this PR made the local handler consistent with the three remote handlers, which have logged cancellation this way all along. Diverging in this PR would be the inconsistency; #410 fixes all four together. Also filed **#409** from live-E2E — never-scanned local libraries render "Last scan 12:00 AM" (MinValue is truthy in JS), the mirror image of this bug.

timothy referenced this pull request 2026-07-17 14:40:58 +02:00

Local libraries always show "Never scanned" — local scanner sets path-level LastScan but not library.LastScan #264

timothy merged commit 5ac49e553e into main 2026-07-17 15:00:58 +02:00

timothy deleted branch fix/264-library-lastscan 2026-07-17 15:00:59 +02:00

timothy referenced this issue from a commit 2026-07-17 15:00:59 +02:00

Merge pull request 'fix(264): record library-level LastScan after a successful local scan' (#408) from fix/264-library-lastscan into main

timothy referenced this pull request 2026-07-17 15:03:21 +02:00

Local libraries always show "Never scanned" — local scanner sets path-level LastScan but not library.LastScan #264

Sign in to join this conversation.

Reviewers

No Reviewers

Labels

Clear labels

ad-hoc

One-off / ad-hoc work not tracked by a dedicated issue

api

REST API / HTTP endpoints

bug

Something isn't working

ci-cd

Build, test, deploy pipeline

content

Channel content / schedules / playlists

dependencies

Dependency updates (Renovate)

enhancement

New feature or improvement

frontend

ChicoryTV React SPA frontend

in-progress

Claimed by an active session — do not pick up

jellyfin

Jellyfin tuner / IPTV integration

parked

Excluded from automatic queue pickup; work only when explicitly selected

priority: high

priority: low

priority: medium

review

Adversarial review finding

security

Security / vulnerability fix

No labels

Milestone

No items

No Milestone

Projects

Clear projects

No projects

Assignees

Clear assignees

renovate timothy (Timothy)

No Assignees

1 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: timothy/ersatztv#408