fix(496): review fixes — thread the replaced local path, scope identity per library, sweep legacy rows [decisions-edit]
Independent cold review (Codex) returned BLOCKED. Findings 1, 3 and 4 are fixed here;
each has a regression test proven non-vacuous by a negative control.
1. Blocker — the 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, adoption hashed the
server-side path, missed the existing row, ALSO slipped past MediaFileAlreadyExists
(which hashes that same wrong string) and inserted a duplicate row under a server path,
leaving the original collection-linked row identity-less forever. The test harness hid
this because its path-replacement stub was an identity function.
→ GetOrAdd now takes localPath explicitly and never reads the projection's path;
BuildPathReplacement takes a real mapping and the new test genuinely replaces.
3. Medium — GetByItemId matched on ItemId alone, so two media sources presenting the same
item id (cloned Jellyfin DB) resolved to each other's row, letting one library repoint
another's. → filtered by LibraryPath.LibraryId.
4. Medium — a row predating the identity that the server had ALREADY stopped reporting was
never adopted (adoption only runs for an incoming item) and carried no identity, so the
itemId diff could not see it either: it sat Normal and schedulable forever, strictly
worse than the hard delete it replaced. → GetExistingLegacyMusicVideoPaths +
FlagFileNotFoundByPaths reconcile legacy rows by local path, and they are counted into
the #477 empty-fetch guard (on the first scan after this ships they ARE the whole
library, so a guard counting only identity rows would sweep all of them on a transient
empty fetch).
Finding 2 (the issue's Done-when #2) is a scope question, not a defect, and is unchanged:
one file path is still one MediaItem row globally, so this lands music videos at parity
with movies rather than eliminating shared-row trashing. Recorded honestly in the decision
record; raised for an explicit call before the issue is closed.
fixes #496
This commit is contained in:
+18
-5
@@ -3858,7 +3858,7 @@ This is a **second, independent** refusal on the same guard, plus a decision not
|
||||
`key: scan.musicvideo-server-identity` · `status: active` · `since: 2026-07-25` · `supersedes: scan.musicvideo-reconciliation@2026-07-20` · `superseded-by: none`
|
||||
**Rule:** Jellyfin music videos carry a per-library server identity (`JellyfinMusicVideo : MusicVideo` with `ItemId`/`Etag`, TPT table + ItemId index), so `JellyfinMusicVideoLibraryScanner` folds onto a shared `MediaServerMusicVideoLibraryScanner` base that diffs the **server item id** and soft-trashes (`FlagFileNotFound`) instead of diffing local paths and hard-deleting. Rows predating the identity are **adopted in place** — the identity row is inserted against the same `MediaItem` id, scoped to the scanned library's own `LibraryPath` — never deleted and re-added.
|
||||
**Signals:** music-video server identity, JellyfinMusicVideo ItemId/Etag, itemId diff, soft FileNotFound trash, adoption of pre-identity rows, cross-library false-trash, path-replaced PathHash · paths: `JellyfinMusicVideo`, `JellyfinMusicVideoRepository`, `IMediaServerMusicVideoRepository`, `MediaServerMusicVideoLibraryScanner`, `JellyfinMusicVideoLibraryScanner` · issues: #496, #494, #477, #488, #497, #500
|
||||
**Mechanics:** dual-provider migration `Add_JellyfinMusicVideo`; adoption probe joins `MediaFile.PathHash` + `NOT EXISTS (JellyfinMusicVideo)` filtered to the library's `LibraryPath`; `AddMusicVideo` normalizes `Path`/`PathHash` to the path-REPLACED local path; `ScanLibrary_Should_Adopt_PreExisting_MusicVideo_Preserving_Identity_And_Collections`, `ScanLibrary_Should_Not_Adopt_A_MusicVideo_Owned_By_Another_LibraryPath`, `ScanLibrary_Should_Not_Flag_MusicVideos_Owned_By_Another_Library`
|
||||
**Mechanics:** dual-provider migration `Add_JellyfinMusicVideo`; `GetOrAdd` takes `localPath` explicitly and never reads the projection's path; adoption probe joins `MediaFile.PathHash` + `NOT EXISTS (JellyfinMusicVideo)` filtered to the library's `LibraryPath`; `GetByItemId` filters `LibraryPath.LibraryId`; legacy (identity-less) rows reconciled by `GetExistingLegacyMusicVideoPaths` + `FlagFileNotFoundByPaths` and counted into the #477 guard; `ScanLibrary_Should_Adopt_Using_The_PathReplaced_Local_Path`, `ScanLibrary_Should_Adopt_PreExisting_MusicVideo_Preserving_Identity_And_Collections`, `ScanLibrary_Should_Not_Adopt_A_MusicVideo_Owned_By_Another_LibraryPath`, `ScanLibrary_Should_Not_Resolve_An_ItemId_Owned_By_Another_Library`, `ScanLibrary_Should_Flag_A_Legacy_Row_The_Server_No_Longer_Reports`
|
||||
|
||||
#494 gave music videos a trash sweep but had to key it on `(LibraryPathId, path)` and hard-delete, because
|
||||
music videos carried no server identity. That left the known limitation this issue is named for: a file served
|
||||
@@ -3883,10 +3883,23 @@ owner's sweep **destroyed** the row the other library still served. This is the
|
||||
be hijacked into this library's identity.
|
||||
- **The adopted row is written with an empty etag** so the ordinary "etag changed ⇒ refresh" path picks it up
|
||||
once, rather than needing a second adoption-specific update path.
|
||||
- **`Path`/`PathHash` are normalized to the path-REPLACED local path on add.** The projection fills them from
|
||||
the path Jellyfin reported, but music videos have always stored the replaced local path. Storing the
|
||||
projection's hash would break every later `PathHash` lookup — `MediaFileAlreadyExists` and the adoption probe
|
||||
above. Caught by the #488 integration test failing on a `NOT NULL`/`UNIQUE` `PathHash` constraint.
|
||||
- **The path-replaced local path is the ONLY path this seam may match on or store.** `GetOrAdd` takes
|
||||
`localPath` as an explicit argument; the `item` it also receives still carries the path the media server
|
||||
reported, and on any install with path replacements configured those are different strings. An earlier cut of
|
||||
this change let the repository derive the path from the projection, which hashed the wrong string, missed the
|
||||
row adoption was meant to find, slipped past `MediaFileAlreadyExists` (which hashes that same wrong string),
|
||||
and inserted a duplicate row under a server-side path while the original collection-linked row stayed
|
||||
identity-less forever. The identity path-replacement stub in the test harness hid it; the regression test now
|
||||
uses a mapping that genuinely replaces.
|
||||
- **Identity lookup is scoped to the scanning library, not just the item id.** Two media sources can present the
|
||||
same item id (a cloned Jellyfin database); a global match would let one library silently repoint another
|
||||
library's row.
|
||||
- **Legacy (identity-less) rows are reconciled by local path until adopted.** A row that predates the identity
|
||||
AND is already gone from the server is never adopted — adoption only ever runs for an *incoming* item — and
|
||||
carries no identity, so the itemId diff cannot see it either. Without a residual path diff it would sit
|
||||
`Normal` and schedulable forever, strictly worse than the hard delete it replaced. Legacy rows are also
|
||||
counted into the #477 empty-fetch guard: on the first scan after this ships they ARE the whole library, so a
|
||||
guard that counted only identity rows would see "0 existing" and sweep all of them on a transient empty fetch.
|
||||
- **Scope honesty: this is parity, not a total fix.** 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*
|
||||
|
||||
Reference in New Issue
Block a user