fix(496): per-library server identity for music videos — itemId diff + soft trash #607

Merged

timothy merged 5 commits from fix/496-musicvideo-identity into main 2026-07-25 18:08:13 +02:00

Conversation 3 Commits 5 Files Changed 22

+16352 -657

timothy commented 2026-07-25 15:21:29 +02:00

Owner

Copy Link

Copy Source

fixes #496

Implements #494's deferred "option 2". Music videos carried no server identity, so JellyfinMusicVideoLibraryScanner reconciled by a (LibraryPathId, path) diff and hard-deleted the remainder — destroying a row a second library might still serve, along with its collection membership and playout references, irreversibly.

What changed

Area	Change
Entity	JellyfinMusicVideo : MusicVideo (ItemId/Etag), mirroring JellyfinMovie — TPT table, varchar(36), ItemId index
Migration	Add_JellyfinMusicVideo, both providers via scripts/add-migration.sh
Repository	IMediaServerMusicVideoRepository + JellyfinMusicVideoRepository — itemId-keyed lookup and Flag{Normal,Unavailable,FileNotFound}, every query scoped per library via LibraryPath.LibraryId
Scanner	New generic MediaServerMusicVideoLibraryScanner base; the Jellyfin scanner folds onto it, keeping the #177/#488/#497/#500 metadata-reconcile logic verbatim
Behavior	The sweep soft-trashes (FileNotFound) instead of deleting — reversible and EmptyTrash-governed
Upgrade	Pre-identity rows are adopted in place: the identity row is inserted against the same MediaItem id, scoped to the scanned library's own LibraryPath

Why adoption, not delete-and-re-add

Every music video on an existing install has no identity row and so can never be found by item id. A delete/re-add would mint a new MediaItem id and silently drop CollectionItem membership — and MediaItemRepository.MediaFileAlreadyExists would in fact block the re-add outright, leaving the item erroring on every scan forever. On prod that is ~1437 music videos and a 204-member collection. Adoption is deliberately scoped to the scanned library's own LibraryPath, because the local MusicVideoFolderScanner writes the same MusicVideo table and its rows must never be hijacked.

Scope: parity, not a total fix

#496's Done-when #2 was amended (see the issue) rather than ticked as literally written. One file path is still one MediaItem row globally — MediaFileAlreadyExists is a global path-hash guard — so a second library serving the same file still gets no row of its own, exactly as for movies/TV. What changes is that the first library's sweep now flags rather than destroys the shared row: the unrecoverable loss is gone. The residual limitation is a whole-app property affecting every media kind and is tracked as #606. The independent review confirmed this diagnosis.

Review

Independent cold-context adversarial review by a different model family (Codex), three rounds:

• Round 1 → BLOCKED. Found a genuine Blocker: the unreplaced Jellyfin path was threaded into GetOrAdd, so on any install with path replacements configured, adoption would hash the server-side path, miss the existing row, slip past the global duplicate guard (which hashes the same wrong string), and insert a duplicate row under a server path — leaving the original collection-linked row identity-less forever. The test harness hid it because its path-replacement stub was an identity function. Plus two Mediums: identity lookup not scoped per library (a cloned Jellyfin DB could let one library repoint another's row), and pre-identity rows the server had already stopped reporting being invisible to both the itemId diff and the sweep — they would have sat Normal and schedulable forever, strictly worse than the hard delete they replaced.
• Round 2 → MERGEABLE, with one Low MySQL-collation edge in the new legacy sweep.
• Round 3 confirms the hardening (match the indexed PathHash, restate the NOT EXISTS guard so the two flag passes are disjoint by construction).

Verification

• 4,226 tests pass across all 7 test projects.
• 16 tests in the music-video fixture; every new #496 test proven non-vacuous by a negative control (reverting the fix makes exactly that test fail).
• dotnet format whitespace --folder and the BOM check clean; decisions_validate.py OK.
• Live-E2E not applicable: no API write-path handler or UI surface changes — this is scanner/repository internals.

Docs

scan.musicvideo-reconciliation relocated to docs/decisions/archive/scan.md as superseded; new active record scan.musicvideo-server-identity in docs/decisions.md; catalog regenerated.

🤖 Generated with Claude Code

fixes #496 Implements #494's deferred "option 2". Music videos carried no server identity, so `JellyfinMusicVideoLibraryScanner` reconciled by a `(LibraryPathId, path)` diff and **hard-deleted** the remainder — destroying a row a second library might still serve, along with its collection membership and playout references, irreversibly. ## What changed | Area | Change | |---|---| | Entity | `JellyfinMusicVideo : MusicVideo` (`ItemId`/`Etag`), mirroring `JellyfinMovie` — TPT table, `varchar(36)`, `ItemId` index | | Migration | `Add_JellyfinMusicVideo`, both providers via `scripts/add-migration.sh` | | Repository | `IMediaServerMusicVideoRepository` + `JellyfinMusicVideoRepository` — itemId-keyed lookup and `Flag{Normal,Unavailable,FileNotFound}`, every query scoped per library via `LibraryPath.LibraryId` | | Scanner | New generic `MediaServerMusicVideoLibraryScanner` base; the Jellyfin scanner folds onto it, keeping the #177/#488/#497/#500 metadata-reconcile logic verbatim | | Behavior | The sweep soft-trashes (`FileNotFound`) instead of deleting — reversible and `EmptyTrash`-governed | | Upgrade | Pre-identity rows are **adopted in place**: the identity row is inserted against the same `MediaItem` id, scoped to the scanned library's own `LibraryPath` | ## Why adoption, not delete-and-re-add Every music video on an existing install has no identity row and so can never be found by item id. A delete/re-add would mint a new `MediaItem` id and silently drop `CollectionItem` membership — and `MediaItemRepository.MediaFileAlreadyExists` would in fact block the re-add outright, leaving the item erroring on every scan forever. On prod that is ~1437 music videos and a 204-member collection. Adoption is deliberately scoped to the scanned library's own `LibraryPath`, because the local `MusicVideoFolderScanner` writes the same `MusicVideo` table and its rows must never be hijacked. ## Scope: parity, not a total fix #496's Done-when #2 was **amended** (see the issue) rather than ticked as literally written. One file path is still one `MediaItem` row globally — `MediaFileAlreadyExists` is a global path-hash guard — so a second library serving the same file still gets no row of its own, exactly as for movies/TV. What changes is that the first library's sweep now **flags** rather than **destroys** the shared row: the unrecoverable loss is gone. The residual limitation is a whole-app property affecting every media kind and is tracked as **#606**. The independent review confirmed this diagnosis. ## Review Independent cold-context adversarial review by a different model family (Codex), three rounds: - **Round 1 → BLOCKED.** Found a genuine **Blocker**: the *unreplaced* Jellyfin path was threaded into `GetOrAdd`, so on any install with path replacements configured, adoption would hash the server-side path, miss the existing row, slip past the global duplicate guard (which hashes the same wrong string), and insert a **duplicate** row under a server path — leaving the original collection-linked row identity-less forever. The test harness hid it because its path-replacement stub was an identity function. Plus two Mediums: identity lookup not scoped per library (a cloned Jellyfin DB could let one library repoint another's row), and pre-identity rows the server had *already* stopped reporting being invisible to both the itemId diff and the sweep — they would have sat `Normal` and schedulable forever, strictly worse than the hard delete they replaced. - **Round 2 → MERGEABLE**, with one Low MySQL-collation edge in the new legacy sweep. - **Round 3** confirms the hardening (match the indexed `PathHash`, restate the `NOT EXISTS` guard so the two flag passes are disjoint by construction). ## Verification - 4,226 tests pass across all 7 test projects. - 16 tests in the music-video fixture; every new #496 test proven **non-vacuous** by a negative control (reverting the fix makes exactly that test fail). - `dotnet format whitespace --folder` and the BOM check clean; `decisions_validate.py` OK. - Live-E2E not applicable: no API write-path handler or UI surface changes — this is scanner/repository internals. ## Docs `scan.musicvideo-reconciliation` relocated to `docs/decisions/archive/scan.md` as `superseded`; new active record `scan.musicvideo-server-identity` in `docs/decisions.md`; catalog regenerated. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

timothy force-pushed fix/496-musicvideo-identity from 5116c5dac0 to 040020f3ba 2026-07-25 15:26:41 +02:00 Compare

timothy commented 2026-07-25 15:27:02 +02:00

Author

Owner

Copy Link

Copy Source

Independent review — three rounds, different model family (Codex)

Review-verdict: MERGEABLE @ 040020f3

Carried from the reviewer's MERGEABLE @ 5116c5da. 040020f3 is that same tree rebased onto abeef63f (main had moved 14 commits). The rebase is content-identical for every reviewed file: the only conflict was an append-vs-append in docs/decisions.md, resolved by keeping both records, and the file overlap between this branch and main's 14 commits is docs/decisions.md + the generated docs/decisions/README.md only — no C# overlap at all. The reviewer independently confirmed the reviewed blob is byte-identical to the post-rebase tree. Full local gate re-run after the rebase: 4,226 tests green, format/BOM clean, decisions_validate.py OK.

Round 1 → BLOCKED

• Blocker — the path-replaced local path was discarded. The scanner computed localPath but GetOrAdd only received incoming, so the repository re-derived the path from the unreplaced projection. On any install with path replacements configured, adoption would hash the server-side path, miss the existing row, also slip past MediaFileAlreadyExists (which hashes that same wrong string), and insert a duplicate row under a server path — leaving the original, collection-linked row identity-less forever. Exactly the failure this PR exists to prevent. Fixed by threading localPath explicitly; the harness's identity-function path-replacement stub was what hid it.
• Medium — GetByItemId matched on ItemId alone, so two media sources presenting the same item id (cloned Jellyfin DB) resolved to each other's row. Fixed by filtering LibraryPath.LibraryId.
• Medium — pre-identity rows the server had already stopped reporting were invisible to both the itemId diff and the sweep: never adopted (adoption only runs for an incoming item), no identity to diff on. They would have sat Normal and schedulable forever — strictly worse than the hard delete they replaced. Fixed with GetExistingLegacyMusicVideoPaths + FlagFileNotFoundByPaths, counted into the #477 empty-fetch guard.

Round 2 → MERGEABLE, one Low

MySQL's case-insensitive default collation could let MF.Path IN @LocalPaths match a row the ordinal C#-side Except had excluded. Taken rather than shipped: the query now matches the indexed PathHash and re-states the NOT EXISTS (JellyfinMusicVideo) guard, so the identity pass and legacy pass are disjoint by construction.

Round 3 → confirms the hardening

PathUtils.GetPathHash round-trip is sound (same helper used at insert; no trimming/case-folding/Unicode normalization on the read path), Distinct() is semantics-preserving, and the NOT EXISTS is correctly correlated to the joined MusicVideo M.Id while retaining LibraryPath.LibraryId scoping.

Non-vacuity

Each of the three new regression tests was proven non-vacuous by a negative control — reverting the corresponding fix makes exactly that test, and only that test, fail.

Scope

Done-when #2 was amended on #496 rather than ticked as written; the residual shared-path limitation is a whole-app property tracked as #606. The reviewer independently confirmed that diagnosis.

## Independent review — three rounds, different model family (Codex) Review-verdict: MERGEABLE @ 040020f3 Carried from the reviewer's `MERGEABLE @ 5116c5da`. `040020f3` is that same tree rebased onto `abeef63f` (main had moved 14 commits). The rebase is content-identical for every reviewed file: the only conflict was an append-vs-append in `docs/decisions.md`, resolved by keeping both records, and the file overlap between this branch and main's 14 commits is `docs/decisions.md` + the generated `docs/decisions/README.md` **only** — no C# overlap at all. The reviewer independently confirmed the reviewed blob is byte-identical to the post-rebase tree. Full local gate re-run after the rebase: 4,226 tests green, format/BOM clean, `decisions_validate.py` OK. ### Round 1 → BLOCKED - **Blocker — the path-replaced local path was discarded.** The scanner computed `localPath` but `GetOrAdd` only received `incoming`, so the repository re-derived the path from the *unreplaced* projection. On any install with path replacements configured, adoption would hash the server-side path, miss the existing row, **also** slip past `MediaFileAlreadyExists` (which hashes that same wrong string), and insert a duplicate row under a server path — leaving the original, collection-linked row identity-less forever. Exactly the failure this PR exists to prevent. Fixed by threading `localPath` explicitly; the harness's identity-function path-replacement stub was what hid it. - **Medium — `GetByItemId` matched on `ItemId` alone**, so two media sources presenting the same item id (cloned Jellyfin DB) resolved to each other's row. Fixed by filtering `LibraryPath.LibraryId`. - **Medium — pre-identity rows the server had already stopped reporting** were invisible to both the itemId diff and the sweep: never adopted (adoption only runs for an *incoming* item), no identity to diff on. They would have sat `Normal` and schedulable forever — strictly worse than the hard delete they replaced. Fixed with `GetExistingLegacyMusicVideoPaths` + `FlagFileNotFoundByPaths`, counted into the #477 empty-fetch guard. ### Round 2 → MERGEABLE, one Low MySQL's case-insensitive default collation could let `MF.Path IN @LocalPaths` match a row the ordinal C#-side `Except` had excluded. Taken rather than shipped: the query now matches the indexed `PathHash` and re-states the `NOT EXISTS (JellyfinMusicVideo)` guard, so the identity pass and legacy pass are disjoint by construction. ### Round 3 → confirms the hardening `PathUtils.GetPathHash` round-trip is sound (same helper used at insert; no trimming/case-folding/Unicode normalization on the read path), `Distinct()` is semantics-preserving, and the `NOT EXISTS` is correctly correlated to the joined `MusicVideo M.Id` while retaining `LibraryPath.LibraryId` scoping. ### Non-vacuity Each of the three new regression tests was proven non-vacuous by a negative control — reverting the corresponding fix makes exactly that test, and only that test, fail. ### Scope Done-when #2 was **amended** on #496 rather than ticked as written; the residual shared-path limitation is a whole-app property tracked as #606. The reviewer independently confirmed that diagnosis.

timothy force-pushed fix/496-musicvideo-identity from 040020f3ba to 44da214e9a 2026-07-25 16:37:39 +02:00 Compare

timothy commented 2026-07-25 16:37:54 +02:00

Author

Owner

Copy Link

Copy Source

Review-verdict: MERGEABLE @ 44da214e

Rebase-only update — main advanced again (#603's decision-record metadata) and produced the usual append-vs-append conflict in docs/decisions.md, resolved by keeping both records. Carried forward from the reviewer's MERGEABLE @ 5116c5da / @ 040020f3.

Equivalence evidence for the new head:

• git diff 040020f3 44da214e -- '*.cs' is empty — not one line of reviewed C# changed.
• #603 touched only docs/** and scripts/decisions_* / scripts/tests/**; no production code overlap with this branch.
• scripts/build_decisions_catalog.py + scripts/decisions_validate.py re-run under #603's new validator: OK, catalog regenerated with no diff.
• Music-video fixture re-run post-rebase: 16/16 green.

Scheduling the merge on green (merge_when_checks_succeed) rather than hand-merging, since this is the third time main has moved underneath this branch mid-verification.

Review-verdict: MERGEABLE @ 44da214e Rebase-only update — `main` advanced again (#603's decision-record metadata) and produced the usual append-vs-append conflict in `docs/decisions.md`, resolved by keeping both records. Carried forward from the reviewer's `MERGEABLE @ 5116c5da` / `@ 040020f3`. Equivalence evidence for the new head: - `git diff 040020f3 44da214e -- '*.cs'` is **empty** — not one line of reviewed C# changed. - #603 touched only `docs/**` and `scripts/decisions_*` / `scripts/tests/**`; no production code overlap with this branch. - `scripts/build_decisions_catalog.py` + `scripts/decisions_validate.py` re-run under #603's **new** validator: OK, catalog regenerated with no diff. - Music-video fixture re-run post-rebase: 16/16 green. Scheduling the merge on green (`merge_when_checks_succeed`) rather than hand-merging, since this is the third time main has moved underneath this branch mid-verification.

timothy scheduled this pull request to auto merge when all checks succeed 2026-07-25 16:38:04 +02:00

timothy force-pushed fix/496-musicvideo-identity from 44da214e9a to 4bdad4bf52 2026-07-25 17:28:03 +02:00 Compare

timothy added 1 commit 2026-07-25 17:36:36 +02:00

test(496,484): record projection failures during enumeration, not eagerly [decisions-edit] ... b5b6e7f636

Re-review Low. The substitute incremented the failure counter inside .Returns(...), i.e. when
the enumerable was handed out, while the real paginator records from ProjectToMusicVideo's catch
DURING enumeration. A refactor that snapshotted Count before the enumeration completed would
then break production while both replacement tests kept passing — exactly the regression the
guard exists to prevent.

Moves the recording into an async iterator, and corrects the decision record to describe #484's
removed music-video test accurately and name its two replacements.

timothy commented 2026-07-25 17:36:57 +02:00

Author

Owner

Copy Link

Copy Source

Review-verdict: MERGEABLE @ b5b6e7f6

Round 4 reviewed 4bdad4bf — the #484 projection-failure integration — and returned MERGEABLE with one Low, which b5b6e7f6 fixes exactly as prescribed. Carrying the verdict forward on that basis; the delta is test-and-docs only (git diff 4bdad4bf..b5b6e7f6 touches JellyfinMusicVideoLibraryScannerTests.cs and docs/decisions.md + the generated catalog — no production code).

Round 4 findings (all verified by the reviewer at file:line)

The rebase onto #612 exposed a silent gap, not a conflict: #484's projectionFailureCount is an optional parameter defaulting to 0, so the new music-video scanner compiled unchanged while opting out of the sweep-refusal entirely — and ProjectToMusicVideo has exactly the swallowing catch #484 defends against. It would have shipped as the only unguarded media-server sweep.

Confirmed sound:

• Counter lifetime — created per ScanLibrary call, passed to both the enumeration and the reconciliation, .Count read strictly after the await foreach completes. Not static, not shared across concurrent library scans.
• Skipped vs Failed — the non-FileSystem and .strm guards return Skipped(); only the real catch returns Failed(), and the paginator increments only on IsFailure. (Getting this backwards would permanently disable reconciliation for any library holding one STRM file.)
• Rebase integrity — no markers remain and all three earlier fixes are intact: localPath threading, the LibraryPath.LibraryId filter in GetByItemId, and the PathHash + NOT EXISTS hardening in FlagFileNotFoundByPaths.
• Coverage — main's failure=1 / failure=0 cases are both still covered, plus the new legacy×failure interaction.

The Low, now fixed

The substitute incremented the counter inside .Returns(...) — i.e. when the enumerable was handed out — while the real paginator records during enumeration. A refactor that snapshotted Count before enumeration completed would have broken production while both replacement tests kept passing. Recording now happens inside an async iterator, and the decision record describes #484's removed test accurately and names its two replacements.

4,277 tests green; format/BOM clean; decisions validator OK under #603's schema.

Review-verdict: MERGEABLE @ b5b6e7f6 Round 4 reviewed `4bdad4bf` — the #484 projection-failure integration — and returned **MERGEABLE** with one Low, which `b5b6e7f6` fixes exactly as prescribed. Carrying the verdict forward on that basis; the delta is test-and-docs only (`git diff 4bdad4bf..b5b6e7f6` touches `JellyfinMusicVideoLibraryScannerTests.cs` and `docs/decisions.md` + the generated catalog — no production code). ### Round 4 findings (all verified by the reviewer at file:line) The rebase onto #612 exposed a **silent gap, not a conflict**: #484's `projectionFailureCount` is an *optional* parameter defaulting to 0, so the new music-video scanner compiled unchanged while opting out of the sweep-refusal entirely — and `ProjectToMusicVideo` has exactly the swallowing `catch` #484 defends against. It would have shipped as the only unguarded media-server sweep. Confirmed sound: - **Counter lifetime** — created per `ScanLibrary` call, passed to both the enumeration and the reconciliation, `.Count` read strictly after the `await foreach` completes. Not static, not shared across concurrent library scans. - **Skipped vs Failed** — the non-`FileSystem` and `.strm` guards return `Skipped()`; only the real catch returns `Failed()`, and the paginator increments only on `IsFailure`. (Getting this backwards would permanently disable reconciliation for any library holding one STRM file.) - **Rebase integrity** — no markers remain and all three earlier fixes are intact: `localPath` threading, the `LibraryPath.LibraryId` filter in `GetByItemId`, and the `PathHash` + `NOT EXISTS` hardening in `FlagFileNotFoundByPaths`. - **Coverage** — main's `failure=1` / `failure=0` cases are both still covered, plus the new legacy×failure interaction. ### The Low, now fixed The substitute incremented the counter inside `.Returns(...)` — i.e. when the enumerable was handed out — while the real paginator records *during* enumeration. A refactor that snapshotted `Count` before enumeration completed would have broken production while both replacement tests kept passing. Recording now happens inside an async iterator, and the decision record describes #484's removed test accurately and names its two replacements. 4,277 tests green; format/BOM clean; decisions validator OK under #603's schema.

timothy merged commit 54ed75624a into main 2026-07-25 18:08:13 +02:00

timothy deleted branch fix/496-musicvideo-identity 2026-07-25 18:08:13 +02:00

timothy referenced this issue from a commit 2026-07-25 18:08:15 +02:00

Merge pull request 'fix(496): per-library server identity for music videos — itemId diff + soft trash' (#607) from fix/496-musicvideo-identity into main

timothy referenced this pull request 2026-07-25 18:09:40 +02:00

Music videos have no per-library identity — global GetOrAdd path dedup can cross-library false-trash #496

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#607