fix(491): unique index on LibraryFolder(LibraryPathId, PathHash) + tolerate concurrent insert #613

Merged

timothy merged 11 commits from fix/491-libraryfolder-unique-index into main 2026-07-25 22:00:45 +02:00

Conversation 8 Commits 11 Files Changed 18

+16316 -26

11 Commits

Author	SHA1	Message	Date
timothy	960145348b	ci(491): split the MySql dedupe fixture out of CI, tracked by #627 ... The gate went red three times in CI with three distinct root causes (stale pooled session after a drop, lost isolation from a shared database name, connect-before-create). An intermittently-red gate is worse than none: it trains everyone to re-run instead of read, which is how the two collation defects escaped in the first place. The production fix is reviewed and green, so it should not stay blocked behind test-harness reliability. The fixture is kept and stays opt-in via ETV_TEST_MYSQL_CONNECTION (visible skip without it); only the CI wiring is removed, with a note where it belongs. Decision record corrected — it described a CI step that no longer exists. [decisions-edit]	2026-07-25 21:43:17 +02:00
timothy	8e83183b8b	docs(491): migrate the decision record to the per-file corpus format ... #610 split docs/decisions.md into one YAML-frontmatter file per record while this branch was open, so the inline record could not be merged -- it had to be converted. Same content and metadata, now at docs/decisions/records/scan/libraryfolder-unique-identity.md with an index entry and a regenerated catalog.	2026-07-25 21:16:51 +02:00
timothy	04ea2abb40	test(491): restore per-test databases; keep the pool clear (fixture determinism) ... Second CI failure of this fixture, and my previous fix caused it: collapsing to one shared database traded isolation for a wipe that has to succeed, and when it silently did not, the second test seeded onto the first's rows and failed with 'Duplicate entry 1 for key LibraryPath.PRIMARY' in 539ms - too fast to have re-run the migration chain, i.e. MigrateAsync no-opped against an already-current __EFMigrationsHistory. The coordinator's read was right and I verified it rather than assuming: the leak came from never clearing pools, not from names being unique. Harness against a real 8.4 server, unique database name per iteration WITH ClearPoolAsync on that connection string: 0 leaked threads over 30 iterations. So isolation costs nothing and the shared name was solving a problem pool-clearing already solved. Restore a fresh etv491_<guid> 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 the guarded EnsureDeletedAsync and followed by ClearPoolAsync on that exact connection string. The stale-session hazard from the first failure needs the connection string to be REUSED after the drop, which a never-repeated name makes impossible; clearing the pool is the belt to that brace and closes the leak. Verified: 10 consecutive runs 10/10 green, server threads flat at 2 and zero leftover schemas throughout; each test run ALONE twice; both tests with the order REVERSED - order-independence being the evidence this failure would have needed, since contamination is invisible when a test runs first. Negative control re-confirmed after the change: restoring COLLATE utf8mb4_bin still fails with survivors [1,4,5,6,7,9] vs [1,4,5,6,7,9,10], then restored and green again. No Retry anywhere. One shared fixture body across providers; still fail-closed under ETV_REQUIRE_MYSQL_TESTS. Refs #488 #308 fix #491	2026-07-25 21:13:31 +02:00
timothy	cdb67e26f2	test(491): make the MySql dedupe fixture deterministic (pool + database lifetime) ... The new MySql coverage failed intermittently in CI with 'Unknown database etv491_...' at connect time, and passed on the immediately preceding commit - a latent race in the fixture, not a regression. Diagnosed against a real 8.4 server rather than by inspection; two measured mechanisms: 1. A pooled session outlives DROP DATABASE. Reopening the dropped database's connection string SUCCEEDS, because MySqlConnector hands back the still-alive session whose default schema no longer exists. Whether a caller sees success or 1049 then depends on whether the pool reuses that session or opens a fresh one - a fresh handshake names the dropped schema and fails. That is the non-determinism, and it is invisible in any single green run. 2. A per-test database name leaks a pool per test. MySqlConnector keys pools by connection string, so each test built a new pool that was never cleared, leaking ~1 server thread per iteration (measured); hammering it exhausted max_connections outright, and a saturated server also inflated this fixture from ~1m20s to 13 minutes. Fix is structural, not a retry: ONE fixed database name (hence one pool), EF owns the lifecycle via EnsureDeletedAsync (guarded, unlike a raw DROP), the pool is cleared after every drop, and the drop moves to the START of a test so no test leaves a live pool pointing at a database it just destroyed. ServerVersion is resolved once per SetUp instead of twice. Determinism verified: 10 consecutive runs, 10/10 green, server threads flat at 2 throughout. Negative control re-confirmed after the change - restoring COLLATE utf8mb4_bin still fails with survivors [1,4,5,6,7,9] vs [1,4,5,6,7,9,10]. Both providers still share one fixture body; still fail-closed under ETV_REQUIRE_MYSQL_TESTS. Refs #488 #308 fix #491	2026-07-25 21:13:31 +02:00
timothy	b587f80f43	test(491): clear the nullable warnings in the dedupe fixture ... Review follow-up. The new MySql-half members introduced CS8600/8604/8625/8618 and were inconsistent with this same file's established late-init pattern (_databasePath = null!). Nullable-annotate the genuinely-nullable ones and use null! for the Dapper-populated row properties.	2026-07-25 21:13:31 +02:00
timothy	83cd36e0de	test(491): run the dedupe fixture against MySql in CI; correct the collation claim ... The dedupe DML had zero automated coverage on MySql: the migrations job only applies migrations to a fresh EMPTY database, so no dedupe row ever executed there. Two MySql-only collation defects escaped that gate in this session and were caught only by hand-run containers. Parameterize LibraryFolderDedupeMigrationTests over both providers from ONE fixture body - same seeded rows, same expected survivors - rather than adding a MySql-only copy that would drift and recreate the gap. Assertions no longer use WHERE Path = '...', which is itself collation-dependent and would quietly mean something different per provider; rows are read once and compared ordinally in memory. A new step in the existing migrations job runs it against that job's mysql:8.4 service, on a per-test database of its own. Proven red when the collation is wrong: restoring COLLATE utf8mb4_bin fails the MySql half with survivors [1,4,5,6,7,9] - the trailing-space sibling deleted - while SQLite stays green. Proven non-skippable: without ETV_TEST_MYSQL_CONNECTION the fixture ignores visibly, and with ETV_REQUIRE_MYSQL_TESTS=1 (which CI sets) that skip becomes a hard failure, so it cannot pass having connected to nothing. Local runs need no MySql. Also correct an overstated comment. The schema pins only the utf8mb4 charset, never a collation, so the effective comparison is the server default: always case-insensitive, but PAD SPACE only on utf8mb4_general_ci - 8.4's default utf8mb4_0900_ai_ci is NO PAD, verified on the real column. The migration bug was independent of that because the old code applied an EXPLICIT utf8mb4_bin, which is PAD SPACE everywhere; the runtime simply tolerates both. Refs #488 #308 fix #491	2026-07-25 21:13:31 +02:00
timothy	48d41b9235	fix(491): make the MySql dedupe byte-exact, not just case-exact (PAD SPACE) ... Cross-family review of 1b4dd6d6 found that utf8mb4_bin - chosen to keep the dedupe case-exact - is a PAD SPACE collation, so trailing spaces are insignificant under it. Verified on MySQL 8.4: '/media/Foo' = '/media/Foo ' is TRUE, while case correctly compares unequal. Two distinct legal directories therefore grouped together and the second was DELETED irreversibly, even though PathUtils.GetPathHash hashes them differently and the unique index about to be created would have accepted both. The dedupe destroyed data the constraint never required it to destroy. Group and join on CONVERT(Path USING binary) instead - NO PAD and byte-exact, matching the hash. utf8mb4_0900_bin is also NO PAD but carries a server-version floor. This 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 keeper-selection, repoint and delete steps. SQLite's = on TEXT is byte-exact with no padding, so that migration was already correct - which is exactly why a SQLite-only test could not see the divergence. The two providers are now semantically equivalent, and the dedupe fixture is shared: same rows, same expected survivors (1,4,5,6,7,9,10), asserted by the SQLite test and reproduced by hand on MySQL 8.4. Runtime was never affected, and this is now stated and tested rather than assumed: GetFolder's SQL equality is a superset narrowing (both collation quirks make it more permissive, never less, so it cannot miss a byte-exact match) and ResolveExact settles identity with StringComparison.Ordinal, which compares length first. Added ResolveExact coverage for the trailing-space axis. Refs #488 #308 fix #491	2026-07-25 21:13:31 +02:00
timothy	50eff83628	docs(491): reattach the case-exact lookup rationale to GetFolder ... Review follow-up. ResolveExact was inserted between GetFolder's summary and GetFolder itself, leaving ResolveExact with two summaries and GetFolder with none -- so the H1 rationale (SQL equality is only a narrowing filter; OrderBy(Id) for determinism) described neither of the things ResolveExact does. On a fix that turns on exactly that reasoning, a misattached explanation is what misleads the next reader.	2026-07-25 21:13:31 +02:00
timothy	ecb763ea58	fix(491): review polish — heal cannot abort a scan, ordinal settle unit-tested ... Final low-severity items from the re-review of ee10f932. L2: DbUpdateConcurrencyException derives from DbUpdateException but carries no provider exception, so IsUniqueConstraintViolation does not classify it. A row deleted by a concurrent library edit between the heal's read and its save would propagate and fail the scan, contradicting the invariant stated directly above it. Admit it in the filter. L1: lift the in-memory ordinal settle into LibraryRepository.ResolveExact and unit-test it with both spellings in the candidate list. No SQLite-backed test can exercise it (SQLite's = on TEXT is already binary), so this converts the half that rested on hand-run MySQL evidence into automated coverage. The end-to-end companion test's comment no longer claims to be provider-independent. L4: assert PRAGMA foreign_keys is 1 before migrating, so the enforcement guard cannot silently degrade into the weak pre-fix form it was added to replace. L3: detach the failed heal, matching the insert path. N3: the heal's inner predicate now matches its IsNullOrEmpty outer guard, so a PathHash = '' row cannot enter the branch and silently never heal. N4: record that GetFolder returning null for a case-differing spelling makes MySQL insert a second row where it used to reuse one — correct, and now matching SQLite, but a real behaviour change on a case-insensitive filesystem. Refs #488 #308 fix #491	2026-07-25 21:13:30 +02:00
timothy	14e9b03433	fix(491): wire the unique-violation classifier in the scanner; make folder lookup case-exact ... Review of 491f5099 found the fix inert in the only process that runs it, plus a MySQL collation defect in the lookup. B1 — TvContext.IsUniqueConstraintViolation was assigned only in ErsatzTV/ Startup.cs, but ErsatzTV.Scanner is a separate executable and every production caller of GetOrAddFolder/SetEtag lives there. The classifier kept its '_ => false' default, so the catch never ran and the DbUpdateException failed the whole scan - worse than the duplicate row it replaced. Wire both provider branches in ErsatzTV.Scanner/Program.cs, and add ProviderStaticsWiringTests (architecture) asserting the scanner assigns every TvContext static the host assigns, with IsSqlite documented as the one exemption. H1 — GetFolder's 'Path == folder' is case-insensitive on MySQL while PathHash is case-sensitive, and FirstOrDefault was unordered: a scan of '/x/foo' could resolve the '/x/Foo' row and stamp the wrong hash onto it (verified on MySQL 8.4: the WHERE matches both, LIMIT 1 returns the wrong one). Treat the SQL equality as a narrowing filter, order by Id, and settle identity ordinally. Route the heal through EF and drop a classified violation, so an opportunistic maintenance write can never abort a scan. Also: run the dedupe migration test with foreign keys ON (matching prod), clear the keeper's etag, null out a self-parent, and document the cleanup's limits (NULL paths excluded, Down does not restore deleted rows, CI's fresh-DB apply covers none of the data mutation). Refs #488 #308 fix #491	2026-07-25 21:13:30 +02:00
timothy	9a4f3e832d	fix(491): unique index on LibraryFolder(LibraryPathId, PathHash) + tolerate concurrent insert ... GetOrAddFolder was a check-then-insert with no unique constraint behind it, so two callers racing the same folder could both miss the lookup and both insert. Enforce identity in the schema and make the loser adopt the winner. - LibraryFolder gains a SHA-256 PathHash (the MediaFile.Path/PathHash precedent): Path is MySQL longtext, which cannot be indexed without a prefix length and collates case-insensitively, so the unique index is on (LibraryPathId, PathHash) instead. - GetOrAddFolder and SetEtag catch a classified unique violation via the existing TvContext.IsUniqueConstraintViolation seam (#308) and re-read. - Dual-provider migration audits and collapses pre-existing duplicates (repointing MediaFile, ParentId and ImageFolderDuration) before creating the index; legacy rows keep a null hash and heal on the next scan. - Tests: deterministic cross-connection race, 8x10 barrier stress with an insert-attempt vacuity guard, classifier-inversion negative control, and a real-migration dedupe test. Refs #488 #308 fix #491	2026-07-25 21:13:22 +02:00