diff --git a/docs/decisions.md b/docs/decisions.md index ee5631a1a..07873ccdf 100644 --- a/docs/decisions.md +++ b/docs/decisions.md @@ -207,6 +207,7 @@ another doc or an old issue comment should land here and then follow the link. - 2026-07-23 — Facet-value typeahead is a new endpoint, allow-listed to text fields, no caching (#434) — [`api.search-field-values`](decisions/records/api/search-field-values.md) - 2026-07-23 — Relative-date rule builder operators are a frontend-only mapping onto existing Lucene macros (#435) — [`rulebuilder.relative-date-macros`](decisions/records/rulebuilder/relative-date-macros.md) - 2026-07-25 — A media-server sweep also refuses when the api client silently dropped items whose projection threw; the ratio threshold is rejected (#484) — [`scan.projection-failure-sweep-guard`](decisions/records/scan/projection-failure-sweep-guard.md) +- 2026-07-25 — LibraryFolder identity is enforced by a unique index on `(LibraryPathId, PathHash)`, not an in-process lock (#491) — [`scan.libraryfolder-unique-identity`](decisions/records/scan/libraryfolder-unique-identity.md) - 2026-07-25 — Music videos carry a per-library server identity; reconciliation is an itemId diff + soft trash (#496) — [`scan.musicvideo-server-identity`](decisions/records/scan/musicvideo-server-identity.md) - 2026-07-25 — OKF evaluated and rejected as a replacement; two of its optional fields adopted (#603) — [`docs.decision-optional-provenance`](decisions/records/docs/decision-optional-provenance.md) - 2026-07-25 — Rule-builder group nesting is bounded-arbitrary depth (`MAX_GROUP_DEPTH`), not one level (#436) — [`spa.rulebuilder-nesting`](decisions/records/spa/rulebuilder-nesting.md) diff --git a/docs/decisions/README.md b/docs/decisions/README.md index 67e890272..83b55d094 100644 --- a/docs/decisions/README.md +++ b/docs/decisions/README.md @@ -115,6 +115,7 @@ the link for rationale. Superseded/retired history lives in `archive/`. Regenera | `scan.collections-scan-status` | `GET /api/v1/media-sources/collections-scan-status` reports a family-global (not per-source), boolean-only active-scan set read from `IEntityLocker`; the SPA reconciles authoritatively against it (with a grace-tick helper) instead of a fixed client-side timeout. | 2026-07-12 | [link](records/scan/collections-scan-status.md) | | `scan.getoraddfolder-db-lookup` | `ILibraryRepository.GetOrAddFolder` resolves the existing folder via a DB query on `(LibraryPathId, Path)`, not the caller's `LibraryPath.LibraryFolders` in-memory navigation, since that navigation is only eager-loaded on the local scan path and is null on remote (Jellyfin) callers. | 2026-07-20 | [link](records/scan/getoraddfolder-db-lookup.md) | | `scan.jellyfin-mixed-content-library` | A Jellyfin library whose collection type is `mixed` (or absent) maps to one ErsatzTV library of `LibraryMediaKind.Mixed`, scanned by running the movie/television/music-video scanners in sequence against that single library — a library is a place, not a media kind. | 2026-07-20 | [link](records/scan/jellyfin-mixed-content-library.md) | +| `scan.libraryfolder-unique-identity` | `LibraryFolder` uniqueness per `(LibraryPathId, Path)` is enforced by a database unique index over a SHA-256 `PathHash` (Path is unbounded and not portably indexable), and `LibraryRepository.GetOrAddFolder`/`SetEtag` tolerate the constraint violation by re-reading and adopting the winner's row. | 2026-07-25 | [link](records/scan/libraryfolder-unique-identity.md) | | `scan.musicvideo-server-identity` | 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. | 2026-07-25 | [link](records/scan/musicvideo-server-identity.md) | | `scan.projection-failure-sweep-guard` | `MediaServerReconciliationGuard` takes a per-enumeration projection-failure count and refuses the file-not-found sweep (logged loudly) whenever it is non-zero against a non-empty existing set — at all six sweeps, including the nested per-show season and per-season episode ones #477 left unguarded (via `ShouldFlagMissingDescendants`, which applies the failure refusal but not #477's empty-fetch branch). Deliberate guard-clause skips (STRM, virtual, unsupported type) are explicitly **not** failures and never suppress a sweep. The missing-fraction / ratio threshold floated by #477 is **rejected**, not deferred. | 2026-07-25 | [link](records/scan/projection-failure-sweep-guard.md) | | `scan.zero-item-fetch-guard` | A media-server library sweep refuses to flag missing items when a successful fetch returns zero incoming items against a non-empty existing set (`MediaServerReconciliationGuard.ShouldFlagMissing`), rather than treating an ambiguous empty result as a full-library deletion. | 2026-07-19 | [link](records/scan/zero-item-fetch-guard.md) | diff --git a/docs/decisions/records/scan/libraryfolder-unique-identity.md b/docs/decisions/records/scan/libraryfolder-unique-identity.md new file mode 100644 index 000000000..b273af931 --- /dev/null +++ b/docs/decisions/records/scan/libraryfolder-unique-identity.md @@ -0,0 +1,237 @@ +--- +key: scan.libraryfolder-unique-identity +title: '2026-07-25 — LibraryFolder identity is enforced by a unique index on `(LibraryPathId, PathHash)`, not an in-process lock (#491)' +status: active +since: '2026-07-25' +supersedes: none +superseded-by: none +rule: '`LibraryFolder` uniqueness per `(LibraryPathId, Path)` is enforced by a database unique index over a SHA-256 `PathHash` (Path is unbounded and not portably indexable), and `LibraryRepository.GetOrAddFolder`/`SetEtag` tolerate the constraint violation by re-reading and adopting the winner''s row.' +signals: 'GetOrAddFolder, SetEtag, LibraryFolder duplicate rows, check-then-insert race, unique index, PathHash, longtext index, dedupe migration, concurrent scan · paths: `LibraryRepository.GetOrAddFolder`, `LibraryFolderConfiguration`, `LibraryFolder.PathHash`, `Add_LibraryFolder_PathHash_UniqueIndex`, `LibraryFolderConcurrencyTests`, `LibraryFolderDedupeMigrationTests` · issues: #491, #488, #308' +mechanics: '`LibraryFolderConfiguration` (`HasIndex(f => new { f.LibraryPathId, f.PathHash }).IsUnique()`); dual-provider migration `Add_LibraryFolder_PathHash_UniqueIndex`' +--- + +**Storage constraint, not a lock.** `GetOrAddFolder` looks a folder up by `(LibraryPathId, Path)` and +inserts when absent (`scan.getoraddfolder-db-lookup`, #488). Lookup and insert are not atomic, so two +callers racing the same path both miss and both insert. The alternative on the table — an in-process +`SemaphoreSlim` keyed on the path — was rejected: it closes only the single-process window, leaves the +invariant undocumented in the schema, silently permits duplicates from any other writer, and cannot +clean the duplicates that already exist. The index makes the duplicate *impossible*; the repository's +catch turns the loser into an idempotent adopt. + +**Reuse the #308 provider seam — and wire it in BOTH composition roots.** Detection goes through the +existing `TvContext.IsUniqueConstraintViolation` delegate (`concurrency.idempotent-concurrent-add`), +pointed at `SqliteErrorClassifier` / `MySqlErrorClassifier` and defaulting to a conservative "no", so an +unwired provider never silently swallows a save failure. No second unique-violation detector. + +There are **two** composition roots, and #308 only needed one. `ErsatzTV/Startup.cs` is the host; +`ErsatzTV.Scanner/Program.cs` is a *separate executable* launched per scan by +`CallLibraryScannerHandler`, and it wires its own copy of the provider statics. #308's only consumer +(`ConcurrencyExtensions.TrySaveChangesForcingVersion`) runs solely in the host, so `Startup` sufficed +there. This record's consumer is the opposite: **every** production caller of `GetOrAddFolder`/`SetEtag` +lives in the scanner (the eight `*FolderScanner`s plus `JellyfinMusicVideoLibraryScanner`) and **none** +in the host. Wiring only `Startup` would leave the classifier at its `_ => false` default in the only +process that runs the code, so the catch would be inert and the `DbUpdateException` would escape and +fail the whole scan — strictly worse than the duplicate row it replaces. Both roots now wire it. + +The failure mode is "a static nobody assigned", which no unit test can see, because every test harness +wires the classifier itself — the #491 negative control (`IsUniqueConstraintViolation = _ => false`) +was in fact an accidental reproduction of the real scanner configuration. `ProviderStaticsWiringTests` +(architecture tests) therefore asserts at the **source** level that the scanner assigns every +`TvContext.*` static the host assigns, with a single documented exemption (`IsSqlite`, read only by +`DbInitializer`/`DatabaseMigratorService`, both host-only). Removing either scanner assignment fails it +with the offending name. + +**Index the hash, not the path — a portability trap.** The naive `HasIndex(f => new { f.LibraryPathId, +f.Path }).IsUnique()` does not work on both providers: `LibraryFolder.Path` maps to MySQL `longtext`, +which cannot be indexed without a prefix length (error 1170), and bounding it with `HasMaxLength` +would truncate existing long paths and cap folder depth. A prefix index would also inherit MySQL's +default *case-insensitive* collation and false-collide sibling folders differing only in case, which +are legal on a case-sensitive filesystem. So identity is carried by a `varchar(64)` SHA-256 hex +`PathHash` (`PathUtils.GetPathHash`) — byte-exact on both providers, fixed width, no length cap. This +is not a new pattern: `MediaFile` already carries exactly this `Path`/`PathHash` pair for the same +reason. `Path` keeps its unbounded column and stays the *query* key; the hash exists only to be +indexable. + +**Legacy rows keep a null hash and heal lazily.** Backfilling the hash cannot be done inside the +migration — SQLite has no `sha2()` — and the only in-repo alternative is the `DatabaseMigratorService` +stop-migrate-run-SQL-continue hack that `Add_MediaFilePathHash` needed. It is not needed here: a unique +index treats nulls as distinct on both providers, so the index applies cleanly to any existing +database, and pre-#491 rows are still found by the `Path` lookup, so no insert can race them in the +meantime. `GetOrAddFolder` writes the hash the first time a scan touches such a row, so a database +converges after one scan without a startup migration phase. + +**The lookup must be as case-sensitive as the hash.** `PathUtils.GetPathHash` hashes the exact bytes, but +`GetFolder`'s `Path == folder` is evaluated by the *server*, and on MySQL a `longtext` under the default +collation is case-**insensitive** — the same collation the codebase names in +`TvContext.CaseInsensitiveCollation`. Verified on MySQL 8.4: with rows `/x/Foo` (id 10) and `/x/foo` +(id 11), `WHERE Path = '/x/foo'` returns **both**, and an unordered `LIMIT 1` — what +`FirstOrDefaultAsync` emits — returns **id 10**. Constraint semantics and lookup semantics would +therefore disagree: a scan of `/x/foo` would resolve the `/x/Foo` row and the heal would stamp the wrong +path's hash onto it, corrupting the identity column with nothing able to detect or repair it. `GetFolder` +now treats the SQL equality as a *narrowing* filter only, orders by `Id` for determinism (an unordered +pick can flip as the plan changes — adding the composite index alone can do it, which would make the +heal non-idempotent and self-collide on the next scan), and settles identity in memory with an **ordinal** +comparison. The same function serves the post-violation re-read in `GetOrAddFolder` and `SetEtag`, so +neither can adopt a case-differing sibling as "the winner". + +This is a **deliberate behaviour change on MySQL**, worth stating: `GetFolder` now returns null for a +case-differing spelling, so the scanner *inserts* a second `LibraryFolder` row where it previously reused +the case-differing one. That is the correct semantics for a case-sensitive filesystem, and it makes MySQL +match SQLite's long-standing behaviour (SQLite's `=` on TEXT has always been binary) rather than +diverging by provider. The visible consequence is on a **case-insensitive** filesystem backed by MySQL: +re-casing a library path now yields a second root `LibraryFolder` row instead of reuse. The pure +in-memory settle is factored out as `LibraryRepository.ResolveExact` so this decision is pinned by a +unit test rather than resting only on hand-run MySQL evidence. + +**The heal must never be able to abort a scan.** It is opportunistic maintenance on a hot path, so it +goes through EF rather than the raw Dapper `UPDATE` it started as — a raw update throws a bare +`MySqlException` 1062 that no `DbUpdateException` filter can catch — and its unique-violation is caught +and dropped, leaving the row for the next scan. The filter also admits `DbUpdateConcurrencyException`: +it derives from `DbUpdateException` but carries no provider exception, so the classifier does **not** +recognize it, and a row deleted by a concurrent library edit between the heal's read and its save would +otherwise propagate and fail the scan — the exact invariant the comment above it claims. With the case-exact lookup above the collision is only +reachable via a legacy duplicate the migration's grouping cannot see (see the `NULL` caveat below), but +"declines to heal" is the only acceptable failure mode for a maintenance write. + +**Ship the cleanup with the constraint.** Duplicates are not merely theoretical — before #488 the +lookup read a scan-start in-memory snapshot, so a folder created earlier in the *same* scan was +invisible and inserted again. The migration therefore audits and collapses them before creating the +index: keep the lowest `Id` per `(LibraryPathId, Path)`, repoint `MediaFile.LibraryFolderId` and +`LibraryFolder.ParentId` (both `Restrict`, so the delete would otherwise fail) at the keeper, and — +because `ImageFolderDuration` is 1:1 with a unique index of its own and cannot hold two rows for one +folder — keep the keeper's setting, or promote exactly one duplicate's when it has none, dropping the +rest. Two helper tables keep the statements readable; they are dropped at the end and created with +`DROP TABLE IF EXISTS` first so a retry after a partial failure is safe (DDL implicitly commits on +MySQL, so the migration is not atomic there). The MySQL copy collates every `Path` comparison +`CONVERT(Path USING binary)` — **byte-exact, not merely case-exact**. This is the second axis, and it +cost a round: the first attempt used `COLLATE utf8mb4_bin`, which does fix case, but `utf8mb4_bin` is a +**PAD SPACE** collation (only the `utf8mb4_0900_*` family is NO PAD). Verified on 8.4: +`'/media/Foo' = '/media/Foo '` is TRUE under it, so `/media/Foo` and `/media/Foo ` — two distinct legal +directories — grouped together and the second was **deleted irreversibly**, even though +`PathUtils.GetPathHash` hashes them differently and the unique index would have accepted both. A dedupe +that destroys data the constraint never required it to destroy is a strictly worse bug than the one it +fixes. Binary comparison is NO PAD and byte-exact, matching the hash exactly; `utf8mb4_0900_bin` would +also work but carries a server-version floor. The grouping, the join predicate and the keeper selection +all use it, and it is the *only* path comparison in either migration — every other predicate keys off an +integer id — so there is no mix of padded and unpadded comparisons across the steps. + +The general lesson: **"case-exact" is not "byte-exact"**, and a same-provider fix for one collation axis +can silently leave another open. SQLite's `=` on TEXT is byte-exact with no padding behaviour, so the +SQLite migration was always correct and the divergence was invisible to a SQLite-only test — which is +structurally why a SQLite-only `LibraryFolderDedupeMigrationTests` could not catch it — now closed, see +"Both providers run the same fixture" below. The **runtime** path was never affected: `GetFolder`'s SQL +equality is only a superset narrowing (every collation quirk makes it more permissive, never less, so it +cannot miss a byte-exact match) and `ResolveExact` settles identity with `StringComparison.Ordinal`, +which compares length first and therefore separates trailing-space variants correctly. + +Be precise about which axis is guaranteed, because the two are not alike. The schema pins only the +`utf8mb4` **charset** on `LibraryFolder.Path`, never a collation, so the effective comparison is the +server default. Case-insensitivity holds under every plausible default (8.4's `utf8mb4_0900_ai_ci` and +the older `utf8mb4_general_ci` are both `_ci`) — that axis is always live. PAD SPACE is +**server-dependent**: `utf8mb4_general_ci` has it, `utf8mb4_0900_ai_ci` does *not* (it is NO PAD). +Verified on 8.4, where the column really does come out `utf8mb4_0900_ai_ci` and `WHERE Path = +'/media/Foo'` matches the case variant but not the trailing-space one. The *migration* bug was +independent of that default because the old code applied an **explicit** `COLLATE utf8mb4_bin`, which is +PAD SPACE on every server; the runtime must simply tolerate both, which the superset + ordinal shape +does. The +keeper's `Etag` is cleared (which duplicate the scanner was actually writing to was arbitrary, so +`MIN(Id)`'s etag may describe a stale view and would suppress the repairing rescan — one rescan is +strictly safe), and a final `ParentId = NULL WHERE ParentId = Id` removes the self-cycle a folder +parented on its own duplicate would otherwise acquire. + +**Limits of the cleanup, stated rather than implied.** (a) It is **not** an exhaustive sweep: rows with a +`NULL` `Path` are excluded by `NULL = NULL` and are left as-is — harmless, but it is why the heal's +collision catch is a real backstop rather than dead code. (b) It **deletes user data**, and `Down` only +restores the index — the deleted duplicate rows and the dropped `ImageFolderDuration` settings are +unrecoverable, so rollback is backup-restore, not `Down`. The `DROP TABLE IF EXISTS` guard makes the +migration safe to **re-run**, which is not the same as safe to **undo**. (c) Because the index is on a +column that is `NULL` for every pre-existing row, and `NULL`s are distinct, the deletion is not actually +*required* for the migration to apply — it is data hygiene, which is the safer posture for a destructive +statement. (d) CI's `migrations` job applies to a **fresh, empty** database, so it executes zero rows of +this logic; it proves the DDL orders correctly and nothing about the data mutation. + +**Create the composite index before dropping the one it replaces.** Adding an explicit index whose +leftmost column is `LibraryPathId` makes EF drop the FK-convention index `IX_LibraryFolder_LibraryPathId`, +and EF scaffolds that drop *first*. On MySQL that fails outright — `Cannot drop index +'IX_LibraryFolder_LibraryPathId': needed in a foreign key constraint` — because InnoDB will not leave a +foreign key without a backing index. Both migrations are hand-reordered to create the composite (which +then backs the FK by leftmost prefix) before dropping the single-column index, and `Down` mirrors it. +This is invisible on SQLite and was only caught by replaying the migration against a real MySQL 8.4 +server; the CI `migrations` job's fresh-DB apply would have caught it too, but only after the push. + +**Tests.** `LibraryFolderConcurrencyTests` reproduces a real cross-connection race through the actual +repository via the shared-cache SQLite harness + a `SavingChanges` interceptor that inserts the +conflicting row on another connection mid-save, and asserts the interceptor fired (the vacuity guard — +a concurrency test that never raced proves nothing). A separate 8-thread × 10-round barrier test +asserts one row per path *and* that the number of insert attempts exceeded the number of rounds, so a +run where the threads never collided fails rather than passing hollow. Negative controls: removing the +index fails 6 of the 7 tests, and inverting the classifier (`IsUniqueConstraintViolation = _ => false`) +makes the same race throw, proving the catch is load-bearing. +`LibraryFolderDedupeMigrationTests` drives the *real* Sqlite migration against a database seeded at the +previous migration, so the cleanup SQL is exercised rather than restated — and it runs the migration +with **foreign keys ON** (matching production; seeding, which writes partial graphs, keeps them off), so +`DELETE FROM LibraryFolder` is executed against the two `Restrict` foreign keys it must not violate +rather than with enforcement disabled. + +**Both providers run the same fixture, in CI.** `LibraryFolderDedupeMigrationTests` is parameterized +`[TestFixture(TestProvider.Sqlite)]` / `[TestFixture(TestProvider.MySql)]` over **one** fixture body: the +same seeded rows, the same expected survivor set (`1,4,5,6,7,9,10`), including both collation traps — a +case-differing sibling and a trailing-space sibling, each with its own dependent media file. A separate +MySQL-only copy was rejected: it would drift from the SQLite one and recreate the gap, whereas one +parameterized body makes "the two providers agree" a checked property. Assertions deliberately avoid +`WHERE Path = '…'`, which is itself collation-dependent and would silently mean something different per +provider; rows are read once and compared ordinally in memory. + +This closes a **real** hole rather than a theoretical one. CI's `migrations` job only ever applied +migrations to a fresh **empty** database, so it executed zero rows of dedupe logic — both MySQL-only +collation defects sailed through it and were caught only by hand-run containers that vanish with the +session. A new step in that same job (reusing its existing `mysql:8.4` service, on a per-test database +of its own) runs the fixture against the live server. It is red when the collation is wrong: restoring +`COLLATE utf8mb4_bin` makes the MySQL half fail with survivors `[1,4,5,6,7,9]` — row 10, the +trailing-space sibling, deleted — while the SQLite half stays green. + +The gate cannot silently no-op. MySQL needs `ETV_TEST_MYSQL_CONNECTION`; without it the fixture +**ignores** (a visible skip, so local runs need no MySQL), but CI also sets `ETV_REQUIRE_MYSQL_TESTS=1`, +which turns that skip into a hard failure — "connected to nothing and passed" is precisely the failure +mode being fixed, so it must not be reachable on the CI path. + +**A MySQL test fixture needs a fresh database per test AND an explicitly cleared connection pool — the +two are independent, and fixing only one of them breaks the other way.** This fixture failed in CI twice, +each time for a different half of that sentence, so both are recorded with their measurements (taken +against a real 8.4 server, not reasoned about): +- **A pooled session outlives `DROP DATABASE`.** Reopening the dropped database's connection string + *succeeds*, because MySqlConnector hands back the still-alive pooled session whose default schema no + longer exists. Whether a caller then sees success or `Unknown database` depends on whether the pool + reuses that session or has to open a fresh one — a fresh handshake names the dropped schema and fails + 1049. That was failure #1: intermittent `Unknown database 'etv491_…'` at *connect* time, green on the + immediately preceding commit. It is invisible in any single green run. +- **An uncleared pool leaks a server connection per test.** MySqlConnector keys pools by connection + string, so a per-test database name means a per-test pool; left uncleared it leaked ~1 server thread + per iteration and hammering it exhausted `max_connections` (151) outright. A saturated server also + inflated this fixture's runtime from ~1m20s to 13 minutes — "slow" and "flaky" were the same defect. + +The first fix collapsed to a single shared database to stop the leak. That was a **misdiagnosis worth +recording**: the leak came from never clearing pools, not from the names being unique, so the shared name +gave up per-test isolation to solve a problem pool-clearing already solved. It duly failed a different +way — failure #2, `Duplicate entry '1' for key 'LibraryPath.PRIMARY'` in 539 ms, the second test seeding +on top of the first's rows because the wipe silently did not happen and `MigrateAsync` no-opped against +an already-current `__EFMigrationsHistory`. A shared database makes correctness depend on a wipe +succeeding; a name that has never been used **cannot** contain another test's rows. + +The shape that satisfies both: a **fresh `etv491_` database per test**, never created out of band +(the test's own `MigrateAsync(PreviousMigration)` creates it, keeping EF the single owner of the schema), +dropped in `TearDown` via `EnsureDeletedAsync` (guarded — a no-op when absent, unlike a raw `DROP +DATABASE`) followed by `ClearPoolAsync` **on that exact connection string**. Measured: 0 leaked threads +over 30 iterations, so isolation costs nothing. The stale-session hazard needs the connection string to +be reused after the drop, which a never-repeated name already makes impossible; clearing the pool is the +belt to that brace. No `[Retry]` anywhere — a gate that is re-run on failure trains people to re-run it, +which is worse than no gate. + +Verified: 10 consecutive runs 10/10 green with server threads flat at 2 and zero leftover schemas; each +test run **alone** (twice each) and with the order **reversed** — order-independence is the specific +evidence failure #2 would have needed, since a contamination bug is invisible when a test runs first. + +Still uncovered by automation, and stated rather than assumed: the lookup-collation premises behind the +ordinal fix were verified by hand on 8.4 (`WHERE Path = '/x/foo'` matching both `/x/Foo` and `/x/foo` +with `LIMIT 1` returning the wrong one; `'/media/Foo' = '/media/Foo '` TRUE under `utf8mb4_bin`). +`ResolveExact` unit tests pin the in-memory decision for both axes with no provider at all. Refs #491 #488 #308.