timothy/ersatztv
1
0
Fork 0
Code Issues 58 Pull Requests 8 Actions Packages Projects Releases 9 Wiki Activity

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
timothy commented 2026-07-25 15:37:35 +02:00
Owner
Copy Link
Copy Source

fixes #491

GetOrAddFolder is a check-then-insert on LibraryFolder(LibraryPathId, Path) with no unique index, so racing callers can both insert. Option 1 from the issue (index + tolerate the violation), but it needed a design change to be shippable.

The naive index does not work on MySQL

LibraryFolder.Path maps to longtext, which cannot be indexed without a prefix length (error 1170). HasMaxLength would truncate existing paths and cap folder depth, and a prefix index inherits MySQL's case-insensitive default collation, false-colliding sibling folders that differ only in case — legal on a case-sensitive filesystem.

Resolution follows the codebase's own MediaFile.Path/PathHash precedent: a nullable varchar(64) LibraryFolder.PathHash (SHA-256 via existing PathUtils.GetPathHash), unique index on (LibraryPathId, PathHash). Path keeps its unbounded column and stays the query key. Legacy rows keep a null hash — nulls are distinct in a unique index, so the index applies cleanly to any DB — and GetOrAddFolder heals each row on first touch, avoiding the stop-migrate-run-SQL-continue hack Add_MediaFilePathHash needed (SQLite has no sha2()).

The catch-and-tolerate half reuses the existing TvContext.IsUniqueConstraintViolation provider delegate from #308 rather than adding a second detector.

Migrations (both providers, dedupe before index)

Keep MIN(Id) per (LibraryPathId, Path); repoint MediaFile.LibraryFolderId and LibraryFolder.ParentId (both Restrict); ImageFolderDuration is 1:1 with its own unique index, so keep the keeper's row else promote exactly one duplicate's; then delete losers. MySQL collates every Path comparison utf8mb4_bin to preserve case-differing siblings. Keeper's Etag is cleared, and a self-parent guard nulls ParentId = Id.

Caught by real-MySQL replay: EF scaffolds DropIndex(IX_LibraryFolder_LibraryPathId) before creating the composite, and InnoDB refuses — "needed in a foreign key constraint". Both migrations and both Downs are reordered to create-then-drop. Invisible on SQLite; CI's fresh-DB apply would have caught it only after a push.

Review — first pass BLOCKED on two defects, both serious

B1 — the fix was inert in the only process that runs it, and was a net regression. TvContext.IsUniqueConstraintViolation is assigned only in ErsatzTV/Startup.cs — the host. ErsatzTV.Scanner is a separate executable launched as a child process, and its Program.cs wired 2 of 4 provider statics, omitting this one. Every production caller of GetOrAddFolder/SetEtag lives in the scanner. So the exception filter evaluated false there, the catch never ran, and the new index would have converted today's silent duplicate row into an unhandled DbUpdateException → scanner exits non-zero → the library scan fails.

Every test passed because SharedCacheTvContext, InMemoryTvContext and the migration fixture all assign the classifier in their own constructors — the harness supplied exactly the wiring production lacked.

Fixed by wiring it in both provider branches of the scanner, plus ProviderStaticsWiringTests asserting both composition roots wire the same set of TvContext statics, so the next divergence fails rather than relying on memory. IsSqlite is a genuine remaining divergence but inert (host-only readers), recorded as an exemption with its reason.

H1 — case-insensitive lookup vs case-sensitive hash. The hash is case-sensitive, but GetFolder still filtered f.Path == folder on a MySQL longtext under case-insensitive collation, while the migration deduped under utf8mb4_bin. Constraint and lookup semantics disagreed. Verified empirically on real MySQL 8.4: WHERE Path='/x/foo' matched both /x/Foo and /x/foo, and with LIMIT 1 (what FirstOrDefaultAsync emits) returned the wrong row — so the heal would have stamped one path's hash onto a different path's row, a permanent identity lie, and the next scan would hit a duplicate-key error that recurs forever.

Fixed both halves: GetFolder narrows in SQL, orders by Id, materializes, then settles identity with string.Equals(..., StringComparison.Ordinal) — and it is the single resolution point for the lookup, the post-violation re-read and SetEtag. The heal moved off raw Dapper onto EF (a raw UPDATE throws a bare MySqlException 1062 no DbUpdateException filter can see), guarded and wrapped so it declines rather than aborting a scan.

Re-review verdict MERGEABLE @ ee10f932; a final polish commit then closed the remaining Lows, including DbUpdateConcurrencyException in the heal's filter (the comment claimed the heal can never abort a scan, and that path could), an assertion that PRAGMA foreign_keys is actually 1 in the migration test, and lifting the ordinal settle into a testable pure static so the collation half is pinned by CI rather than by hand-run evidence.

Verification

ErsatzTV.Tests 1887, Scanner.Tests 1488, Infrastructure.Tests 111, Core.Tests 652 (+1 skipped), Architecture.Tests 7 — all 0 failed after rebasing on origin/main. has-pending-model-changes clean on both providers post-rebase, and every migration applies to a fresh SQLite DB. Negative controls: without the index 6 of 7 concurrency tests fail; removing the scanner wiring fails both parity tests; inverting the ordinal match and the FK pragma fails 4. Vacuity guards assert the SavingChanges interceptor fired and that threads genuinely collided. decisions_validate OK, BOM clean, format gate clean.

Known limit, stated not papered over: the MySQL data-mutating path has no automated coverage. CI's migrations job applies to a fresh, empty DB, so it executes zero rows of the dedupe logic; that half was hand-verified against a temporary mysql:8.4 container. The collation half is inherently MySQL-only, though the ordinal decision itself is now unit-tested provider-independently. Prod runs SQLite, which bounds the exposure.

Follow-up filed as #608 (GetParentFolderId still bypasses GetFolder and keeps case-insensitive resolution — pre-existing, same defect class).

fixes #491 `GetOrAddFolder` is a check-then-insert on `LibraryFolder(LibraryPathId, Path)` with no unique index, so racing callers can both insert. Option 1 from the issue (index + tolerate the violation), but it needed a design change to be shippable. ## The naive index does not work on MySQL `LibraryFolder.Path` maps to `longtext`, which cannot be indexed without a prefix length (error 1170). `HasMaxLength` would truncate existing paths and cap folder depth, and a prefix index inherits MySQL's case-insensitive default collation, false-colliding sibling folders that differ only in case — legal on a case-sensitive filesystem. Resolution follows the codebase's own `MediaFile.Path`/`PathHash` precedent: a nullable `varchar(64)` `LibraryFolder.PathHash` (SHA-256 via existing `PathUtils.GetPathHash`), unique index on `(LibraryPathId, PathHash)`. `Path` keeps its unbounded column and stays the query key. Legacy rows keep a **null** hash — nulls are distinct in a unique index, so the index applies cleanly to any DB — and `GetOrAddFolder` heals each row on first touch, avoiding the stop-migrate-run-SQL-continue hack `Add_MediaFilePathHash` needed (SQLite has no `sha2()`). The catch-and-tolerate half reuses the existing `TvContext.IsUniqueConstraintViolation` provider delegate from #308 rather than adding a second detector. ## Migrations (both providers, dedupe before index) Keep `MIN(Id)` per `(LibraryPathId, Path)`; repoint `MediaFile.LibraryFolderId` and `LibraryFolder.ParentId` (both `Restrict`); `ImageFolderDuration` is 1:1 with its own unique index, so keep the keeper's row else promote exactly one duplicate's; then delete losers. MySQL collates every `Path` comparison `utf8mb4_bin` to preserve case-differing siblings. Keeper's `Etag` is cleared, and a self-parent guard nulls `ParentId = Id`. **Caught by real-MySQL replay:** EF scaffolds `DropIndex(IX_LibraryFolder_LibraryPathId)` *before* creating the composite, and InnoDB refuses — "needed in a foreign key constraint". Both migrations and both `Down`s are reordered to create-then-drop. Invisible on SQLite; CI's fresh-DB apply would have caught it only after a push. ## Review — first pass BLOCKED on two defects, both serious **B1 — the fix was inert in the only process that runs it, and was a net regression.** `TvContext.IsUniqueConstraintViolation` is assigned only in `ErsatzTV/Startup.cs` — the *host*. `ErsatzTV.Scanner` is a **separate executable** launched as a child process, and its `Program.cs` wired 2 of 4 provider statics, omitting this one. Every production caller of `GetOrAddFolder`/`SetEtag` lives in the scanner. So the exception filter evaluated false there, the catch never ran, and the new index would have converted today's silent duplicate row into an unhandled `DbUpdateException` → scanner exits non-zero → the library scan fails. Every test passed because `SharedCacheTvContext`, `InMemoryTvContext` and the migration fixture all assign the classifier in their own constructors — the harness supplied exactly the wiring production lacked. Fixed by wiring it in both provider branches of the scanner, plus `ProviderStaticsWiringTests` asserting both composition roots wire the same set of `TvContext` statics, so the next divergence fails rather than relying on memory. `IsSqlite` is a genuine remaining divergence but inert (host-only readers), recorded as an exemption **with its reason**. **H1 — case-insensitive lookup vs case-sensitive hash.** The hash is case-sensitive, but `GetFolder` still filtered `f.Path == folder` on a MySQL `longtext` under case-insensitive collation, while the migration deduped under `utf8mb4_bin`. Constraint and lookup semantics disagreed. Verified empirically on real MySQL 8.4: `WHERE Path='/x/foo'` matched **both** `/x/Foo` and `/x/foo`, and with `LIMIT 1` (what `FirstOrDefaultAsync` emits) returned the wrong row — so the heal would have stamped one path's hash onto a different path's row, a permanent identity lie, and the next scan would hit a duplicate-key error that recurs forever. Fixed both halves: `GetFolder` narrows in SQL, orders by `Id`, materializes, then settles identity with `string.Equals(..., StringComparison.Ordinal)` — and it is the single resolution point for the lookup, the post-violation re-read and `SetEtag`. The heal moved off raw Dapper onto EF (a raw `UPDATE` throws a bare `MySqlException` 1062 no `DbUpdateException` filter can see), guarded and wrapped so it declines rather than aborting a scan. Re-review verdict **MERGEABLE @ ee10f932**; a final polish commit then closed the remaining Lows, including `DbUpdateConcurrencyException` in the heal's filter (the comment claimed the heal can never abort a scan, and that path could), an assertion that `PRAGMA foreign_keys` is actually 1 in the migration test, and lifting the ordinal settle into a testable pure static so the collation half is pinned by CI rather than by hand-run evidence. ## Verification ErsatzTV.Tests **1887**, Scanner.Tests **1488**, Infrastructure.Tests **111**, Core.Tests **652** (+1 skipped), Architecture.Tests **7** — all 0 failed after rebasing on `origin/main`. `has-pending-model-changes` clean on both providers post-rebase, and every migration applies to a fresh SQLite DB. Negative controls: without the index 6 of 7 concurrency tests fail; removing the scanner wiring fails both parity tests; inverting the ordinal match and the FK pragma fails 4. Vacuity guards assert the `SavingChanges` interceptor fired and that threads genuinely collided. `decisions_validate` OK, BOM clean, format gate clean. **Known limit, stated not papered over:** the MySQL data-mutating path has no automated coverage. CI's `migrations` job applies to a **fresh, empty** DB, so it executes zero rows of the dedupe logic; that half was hand-verified against a temporary `mysql:8.4` container. The collation half is inherently MySQL-only, though the ordinal decision itself is now unit-tested provider-independently. Prod runs SQLite, which bounds the exposure. Follow-up filed as #608 (`GetParentFolderId` still bypasses `GetFolder` and keeps case-insensitive resolution — pre-existing, same defect class).
timothy commented 2026-07-25 15:42:34 +02:00
Author
Owner
Copy Link
Copy Source

Independent cold review-only pass, three rounds.

Round 1 returned BLOCKED on two defects, both of which would have made this a net regression:

  • B1 — TvContext.IsUniqueConstraintViolation is assigned only in the host's Startup.cs, but ErsatzTV.Scanner is a separate executable and every production caller of GetOrAddFolder/SetEtag lives there. The catch was dead in the only process that runs it, so the new index would have turned today's silent duplicate row into a scan-aborting unhandled exception. Every test passed because the test harness assigns the classifier in its own constructors — it supplied exactly the wiring production lacked.
  • H1 — case-insensitive lookup vs case-sensitive hash on MySQL, verified empirically on real MySQL 8.4 rather than by argument: WHERE Path='/x/foo' matched both /x/Foo and /x/foo, and LIMIT 1 returned the wrong row.

Round 2 over the fix returned MERGEABLE @ ee10f932, having attacked the new source-parsing architecture test specifically (rename/move, static rename, wiring extracted to a shared helper, reformatting — it fails loudly in each case) and confirmed the migration test now runs the destructive DELETE under the FK-enforcing context.

Round 3 was a delta pass over the polish commit: all six Low items close their findings with mechanisms rather than promises, ResolveExact confirmed genuinely pure and behaviour-preserving, the new unit tests confirmed to construct a candidate list SQLite can never produce (so the ordinal decision is pinned in CI, not by hand-run evidence), and the rebase confirmed to have preserved every #491 hunk and both decision records. Verdict MERGEABLE @ e9b836eb, with one new nit.

Delta since that sha: one commit, docs(491), fixing exactly that nit — ResolveExact had been inserted between GetFolder's <summary> and GetFolder itself, leaving ResolveExact with two summaries and GetFolder with none, so the H1 rationale ("the SQL equality is only a narrowing filter", "ordered by Id for determinism") was attached to a member that does neither. Comment text only, no code. Verified: BOM clean, format gate clean, 19 targeted LibraryFolder/LibraryRepository tests pass.

Reviewer-family note for the audit trail: process.independent-review-rubric prefers a different model family than the implementer. The Codex CLI is not installed in this environment, so the rubric's stated fallback was used — a fresh agent with no implementation role, cold-briefed. Recording the exemption rather than leaving it silent.

Review-verdict: MERGEABLE @ 1b4dd6d6

Independent cold review-only pass, three rounds. Round 1 returned BLOCKED on two defects, both of which would have made this a net regression: - **B1** — `TvContext.IsUniqueConstraintViolation` is assigned only in the host's `Startup.cs`, but `ErsatzTV.Scanner` is a separate executable and every production caller of `GetOrAddFolder`/`SetEtag` lives there. The catch was dead in the only process that runs it, so the new index would have turned today's silent duplicate row into a scan-aborting unhandled exception. Every test passed because the test harness assigns the classifier in its own constructors — it supplied exactly the wiring production lacked. - **H1** — case-insensitive lookup vs case-sensitive hash on MySQL, verified empirically on real MySQL 8.4 rather than by argument: `WHERE Path='/x/foo'` matched both `/x/Foo` and `/x/foo`, and `LIMIT 1` returned the wrong row. Round 2 over the fix returned MERGEABLE @ ee10f932, having attacked the new source-parsing architecture test specifically (rename/move, static rename, wiring extracted to a shared helper, reformatting — it fails loudly in each case) and confirmed the migration test now runs the destructive `DELETE` under the FK-enforcing context. Round 3 was a delta pass over the polish commit: all six Low items close their findings with mechanisms rather than promises, `ResolveExact` confirmed genuinely pure and behaviour-preserving, the new unit tests confirmed to construct a candidate list SQLite can never produce (so the ordinal decision is pinned in CI, not by hand-run evidence), and the rebase confirmed to have preserved every #491 hunk and both decision records. Verdict MERGEABLE @ e9b836eb, with one new nit. Delta since that sha: one commit, `docs(491)`, fixing exactly that nit — `ResolveExact` had been inserted between `GetFolder`'s `<summary>` and `GetFolder` itself, leaving `ResolveExact` with two summaries and `GetFolder` with none, so the H1 rationale ("the SQL equality is only a narrowing filter", "ordered by Id for determinism") was attached to a member that does neither. Comment text only, no code. Verified: BOM clean, format gate clean, 19 targeted `LibraryFolder`/`LibraryRepository` tests pass. Reviewer-family note for the audit trail: `process.independent-review-rubric` prefers a different model family than the implementer. The Codex CLI is not installed in this environment, so the rubric's stated fallback was used — a fresh agent with no implementation role, cold-briefed. Recording the exemption rather than leaving it silent. Review-verdict: MERGEABLE @ 1b4dd6d6
timothy commented 2026-07-25 16:48:54 +02:00
Author
Owner
Copy Link
Copy Source

Cross-family (Codex/GPT) cold review — and it found a data-loss defect both same-family passes missed.

BLOCKER: the MySQL dedupe can irreversibly delete distinct folders whose paths differ only by trailing spaces.

The migration groups and joins on Path COLLATE utf8mb4_bin, then deletes every loser. utf8mb4_bin is a PAD SPACE collation — the MySQL 8.0 utf8mb4_0900_* family is NO PAD, but the older utf8mb4_bin/utf8mb4_general_ci are not — so trailing spaces are insignificant and '/media/Foo' = '/media/Foo ' is TRUE under it.

Rows /media/Foo and /media/Foo are two distinct, legal directories on Linux. They group together, the second row's dependents are repointed to the first, and the second is deleted — even though PathUtils.GetPathHash hashes exact UTF-8 bytes, so the unique index being created would have permitted both rows. The migration destroys data the constraint never required it to destroy.

utf8mb4_bin was chosen to be case-exact, and it achieves that; it just isn't byte-exact, which is what the hash semantics demand. SQLite's = on TEXT is byte-exact with no padding behaviour, so the two provider migrations are not semantically equivalent — and LibraryFolderDedupeMigrationTests being SQLite-only means it structurally cannot detect the divergence. That is precisely the residual the earlier passes flagged as "hand-verified only", now shown to be hiding something real.

Fix in progress: byte-exact comparison throughout the MySQL migration, plus a trailing-space sibling pair in the fixture asserting both rows survive, verified against real MySQL.

The review's second finding — GetParentFolderId bypassing ordinal identity settlement and attaching a child to a case-differing sibling on MySQL — is the already-known L5, filed as #608 before this review ran. Independent rediscovery by a different model family is good corroboration that it is real, but it is pre-existing and stays out of scope here.

Process note: this supersedes the "different model family unavailable" exemption I recorded earlier in this session. That claim was wrong — I generalized from one transient failed dispatch — and this review is the concrete cost of it: two same-family passes returned MERGEABLE on a migration that could delete user rows.

Review-verdict: BLOCKED @ 1b4dd6d6

Cross-family (Codex/GPT) cold review — and it found a data-loss defect both same-family passes missed. **BLOCKER: the MySQL dedupe can irreversibly delete distinct folders whose paths differ only by trailing spaces.** The migration groups and joins on `Path COLLATE utf8mb4_bin`, then deletes every loser. `utf8mb4_bin` is a **PAD SPACE** collation — the MySQL 8.0 `utf8mb4_0900_*` family is NO PAD, but the older `utf8mb4_bin`/`utf8mb4_general_ci` are not — so trailing spaces are insignificant and `'/media/Foo' = '/media/Foo '` is TRUE under it. Rows `/media/Foo` and `/media/Foo ` are two distinct, legal directories on Linux. They group together, the second row's dependents are repointed to the first, and the second is deleted — even though `PathUtils.GetPathHash` hashes exact UTF-8 bytes, so the unique index being created would have permitted both rows. The migration destroys data the constraint never required it to destroy. `utf8mb4_bin` was chosen to be case-exact, and it achieves that; it just isn't *byte*-exact, which is what the hash semantics demand. SQLite's `=` on TEXT is byte-exact with no padding behaviour, so **the two provider migrations are not semantically equivalent** — and `LibraryFolderDedupeMigrationTests` being SQLite-only means it structurally cannot detect the divergence. That is precisely the residual the earlier passes flagged as "hand-verified only", now shown to be hiding something real. Fix in progress: byte-exact comparison throughout the MySQL migration, plus a trailing-space sibling pair in the fixture asserting both rows survive, verified against real MySQL. The review's second finding — `GetParentFolderId` bypassing ordinal identity settlement and attaching a child to a case-differing sibling on MySQL — is the already-known L5, filed as #608 before this review ran. Independent rediscovery by a different model family is good corroboration that it is real, but it is pre-existing and stays out of scope here. Process note: this supersedes the "different model family unavailable" exemption I recorded earlier in this session. That claim was wrong — I generalized from one transient failed dispatch — and this review is the concrete cost of it: two same-family passes returned MERGEABLE on a migration that could delete user rows. Review-verdict: BLOCKED @ 1b4dd6d6
timothy commented 2026-07-25 17:07:53 +02:00
Author
Owner
Copy Link
Copy Source

Cross-family re-review of the PAD SPACE fix: confirmed correct and complete.

It independently verified that CONVERT(Path USING binary) is byte-exact for the LONGTEXT column, that the predicate enumeration is right (both path comparisons converted; every downstream mutation — repoints, ImageFolderDuration, delete — keys off integer IDs only, so no padded predicate remains), that the SQLite migration was already byte-exact and the two are now genuinely equivalent, that the shared fixture would fail under the old collation, and that the runtime ResolveExact argument holds (the SQL narrowing can only be a superset; StringComparison.Ordinal then filters on length and bytes).

It nonetheless returned BLOCKED on one ground, and I agree rather than overriding it.

The MySQL dedupe DML has zero automated coverage. LibraryFolderDedupeMigrationTests is SQLite-only, and CI's MySQL job applies migrations to a fresh, empty database — so not one row of dedupe logic ever executes there. Two real MySQL-only collation defects have now escaped that same gate in a single session (the case half, then the padding half), both caught only by a hand-run container that vanishes with the session. On an irreversible, destructive path that is not a residual to note in the record; it is a demonstrated blind spot, and a future edit to this migration — or a new dedupe migration copied from it — would have no backstop at all.

The infrastructure to close it already exists: the migrations job runs a mysql:8.4 service. Adding coverage now, with the requirement that it runs the same shared fixture against both providers (so they cannot silently diverge again) and that it is demonstrated to go red when the collation is reverted — an assertion that cannot fail is what got us here twice.

Also correcting a nit the reviewer caught: a new comment claims the MySQL server default is PAD SPACE, but the schema pins only the utf8mb4 charset and MySQL 8.4's default collation is utf8mb4_0900_ai_ci, which is NO PAD. The code's reasoning is right; the claim about the default is not, and on the second collation bug in this file a comment that teaches something false is worth fixing.

The reviewer's other finding (GetParentFolderId bypassing ordinal settlement) remains out of scope and tracked as #608 — it independently rediscovered it, which is good corroboration that it is real.

Review-verdict: BLOCKED @ ec0f291c

Cross-family re-review of the PAD SPACE fix: **confirmed correct and complete.** It independently verified that `CONVERT(Path USING binary)` is byte-exact for the `LONGTEXT` column, that the predicate enumeration is right (both path comparisons converted; every downstream mutation — repoints, `ImageFolderDuration`, delete — keys off integer IDs only, so no padded predicate remains), that the SQLite migration was already byte-exact and the two are now genuinely equivalent, that the shared fixture would fail under the old collation, and that the runtime `ResolveExact` argument holds (the SQL narrowing can only be a superset; `StringComparison.Ordinal` then filters on length and bytes). It nonetheless returned BLOCKED on one ground, and I agree rather than overriding it. **The MySQL dedupe DML has zero automated coverage.** `LibraryFolderDedupeMigrationTests` is SQLite-only, and CI's MySQL job applies migrations to a **fresh, empty** database — so not one row of dedupe logic ever executes there. Two real MySQL-only collation defects have now escaped that same gate in a single session (the case half, then the padding half), both caught only by a hand-run container that vanishes with the session. On an irreversible, destructive path that is not a residual to note in the record; it is a demonstrated blind spot, and a future edit to this migration — or a new dedupe migration copied from it — would have no backstop at all. The infrastructure to close it already exists: the `migrations` job runs a `mysql:8.4` service. Adding coverage now, with the requirement that it runs the **same shared fixture** against both providers (so they cannot silently diverge again) and that it is demonstrated to go **red** when the collation is reverted — an assertion that cannot fail is what got us here twice. Also correcting a nit the reviewer caught: a new comment claims the MySQL server default is PAD SPACE, but the schema pins only the `utf8mb4` charset and MySQL 8.4's default collation is `utf8mb4_0900_ai_ci`, which is NO PAD. The code's reasoning is right; the claim about the default is not, and on the second collation bug in this file a comment that teaches something false is worth fixing. The reviewer's other finding (`GetParentFolderId` bypassing ordinal settlement) remains out of scope and tracked as #608 — it independently rediscovered it, which is good corroboration that it is real. Review-verdict: BLOCKED @ ec0f291c
timothy force-pushed fix/491-libraryfolder-unique-index from 1b4dd6d60f to fc977b842c 2026-07-25 17:31:58 +02:00 Compare
timothy force-pushed fix/491-libraryfolder-unique-index from fc977b842c to 4c320e6fea 2026-07-25 18:36:57 +02:00 Compare
timothy commented 2026-07-25 18:37:17 +02:00
Author
Owner
Copy Link
Copy Source

MySQL coverage landed, and the gate is verified to actually run — not merely green.

Measured, not assumed. I read the real CI log for the migrations job on this branch: the new step executed with ETV_REQUIRE_MYSQL_TESTS: 1 and reported Failed: 0, Passed: 4, Skipped: 0. Zero skipped is the load-bearing number — both provider fixtures genuinely ran against live MySQL in CI, so the gate is not a silent no-op. (Worth noting for anyone re-checking: the commit-status target_url carries the UI run number, and feeding that number back to the API resolves to a different, older run that returns a perfectly plausible log. Match on head_sha from /actions/runs instead, and sanity-check the log's date.)

Independent review of the coverage delta: MERGEABLE. It confirmed the fixture is genuinely one shared body — the only per-provider branching is how to introspect engine catalogs (PRAGMA foreign_keys vs @@SESSION.foreign_key_checks, sqlite_master vs information_schema), with identical expected values, which is the right kind of difference and leaves the divergence class closed. It also verified the fail-closed mechanics end to end (unset + required ⇒ hard fail; unreachable server ⇒ unhandled throw, never a silent pass; the step shares the same docs-only/revalidate gating as every other step in the job), found no leak path in the per-test etv491_<guid> database, and checked the corrected collation claims against the actual migration SQL.

Its one finding — newly-introduced nullable warnings (CS8600/8604/8625/8618) inconsistent with this file's own = null! late-init pattern — is fixed in the head commit. Build now clean.

Reviewer-family note, stated plainly: the cross-family (Codex) reviewer caught the PAD SPACE blocker on the previous commit, but its run stalled and never returned a verdict for this final delta. That last commit is test/CI/docs-only, touches no production or migration code, and was cold-reviewed same-family instead. Recording the gap rather than implying cross-family coverage it does not have.

Rebased onto 54ed7562; docs/decisions.md conflicts were append/append again — all records kept, catalog regenerated. Re-verified at head: ErsatzTV.Tests 1897 passed / 0 failed / 2 visibly skipped (the MySQL pair, correctly ignored locally), decisions_validate OK, BOM and format clean.

Review-verdict: MERGEABLE @ 4c320e6f

MySQL coverage landed, and the gate is verified to actually run — not merely green. **Measured, not assumed.** I read the real CI log for the migrations job on this branch: the new step executed with `ETV_REQUIRE_MYSQL_TESTS: 1` and reported `Failed: 0, Passed: 4, Skipped: 0`. Zero skipped is the load-bearing number — both provider fixtures genuinely ran against live MySQL in CI, so the gate is not a silent no-op. (Worth noting for anyone re-checking: the commit-status `target_url` carries the **UI** run number, and feeding that number back to the API resolves to a *different, older* run that returns a perfectly plausible log. Match on `head_sha` from `/actions/runs` instead, and sanity-check the log's date.) **Independent review of the coverage delta:** MERGEABLE. It confirmed the fixture is genuinely one shared body — the only per-provider branching is *how* to introspect engine catalogs (`PRAGMA foreign_keys` vs `@@SESSION.foreign_key_checks`, `sqlite_master` vs `information_schema`), with identical expected values, which is the right kind of difference and leaves the divergence class closed. It also verified the fail-closed mechanics end to end (unset + required ⇒ hard fail; unreachable server ⇒ unhandled throw, never a silent pass; the step shares the same docs-only/revalidate gating as every other step in the job), found no leak path in the per-test `etv491_<guid>` database, and checked the corrected collation claims against the actual migration SQL. Its one finding — newly-introduced nullable warnings (CS8600/8604/8625/8618) inconsistent with this file's own `= null!` late-init pattern — is fixed in the head commit. Build now clean. Reviewer-family note, stated plainly: the cross-family (Codex) reviewer caught the PAD SPACE blocker on the previous commit, but its run stalled and never returned a verdict for this final delta. That last commit is test/CI/docs-only, touches no production or migration code, and was cold-reviewed same-family instead. Recording the gap rather than implying cross-family coverage it does not have. Rebased onto `54ed7562`; `docs/decisions.md` conflicts were append/append again — all records kept, catalog regenerated. Re-verified at head: `ErsatzTV.Tests` 1897 passed / 0 failed / 2 visibly skipped (the MySQL pair, correctly ignored locally), `decisions_validate` OK, BOM and format clean. Review-verdict: MERGEABLE @ 4c320e6f
timothy scheduled this pull request to auto merge when all checks succeed 2026-07-25 18:37:26 +02:00
timothy commented 2026-07-25 19:02:40 +02:00
Author
Owner
Copy Link
Copy Source

Retracting the MERGEABLE verdict — the new MySQL gate went red in CI on head 4c320e6f, and it is a real defect in the fixture rather than a flake. Auto-merge is correctly blocked.

Failed Migration_Collapses_Duplicate_Folders_And_Repoints_Their_Dependents [4 s]
  MySqlConnector.MySqlException : Unknown database 'etv491_4811dd03231e4555b8db96569742744c'
  Server Error Code: 1049   SqlState: 42000
Failed!  -  Failed: 1, Passed: 3, Skipped: 0, Total: 4

(run 1426, job 6267, with the ❌ Failure - Main marker on the dedupe step — so a genuine step failure, not a killed job.)

The per-test database did not exist when the EF context opened its connection; the throw is at connect time, not inside the migration.

It is non-deterministic, which is what makes it worth stopping for. The identical fixture passed Failed: 0, Passed: 4, Skipped: 0 on the previous head fc977b84. The only delta between those heads is a nullable-annotation commit (string → string?, = null! on two Dapper row properties) and a rebase — neither can change runtime behaviour. So the fixture carries a latent ordering/lifetime bug that simply won the race the first time. I ruled out the obvious candidate: the fixture is [NonParallelizable] and there is no assembly-level Parallelizable.

Being explicit about the irony, because it is the actual lesson: this gate was added because two data-destroying collation defects escaped an inadequate test. Shipping it in a state where it intermittently fails would be worse than not adding it — an intermittently-red gate trains everyone to re-run rather than read, which is exactly how the original escapes happened. So it gets fixed deterministically, with no [Retry], and the fix must be demonstrated over repeated consecutive runs rather than one green one. The red-when-broken control also has to be re-confirmed afterwards, since the most likely bad fix is one that quietly neuters the assertion.

Diagnosis in progress — candidate causes include eager ServerVersion.AutoDetect connecting at configuration time, and MySqlConnector's connection pool handing back a session bound to a database a prior DROP DATABASE removed (pools are keyed by connection string, so a ClearPool after the drop may be required).

Review-verdict: BLOCKED @ 4c320e6f

Retracting the MERGEABLE verdict — the new MySQL gate went **red in CI** on head `4c320e6f`, and it is a real defect in the fixture rather than a flake. Auto-merge is correctly blocked. ``` Failed Migration_Collapses_Duplicate_Folders_And_Repoints_Their_Dependents [4 s] MySqlConnector.MySqlException : Unknown database 'etv491_4811dd03231e4555b8db96569742744c' Server Error Code: 1049 SqlState: 42000 Failed! - Failed: 1, Passed: 3, Skipped: 0, Total: 4 ``` (run 1426, job 6267, with the `❌ Failure - Main` marker on the dedupe step — so a genuine step failure, not a killed job.) The per-test database did not exist when the EF context opened its connection; the throw is at *connect* time, not inside the migration. **It is non-deterministic, which is what makes it worth stopping for.** The identical fixture passed `Failed: 0, Passed: 4, Skipped: 0` on the previous head `fc977b84`. The only delta between those heads is a nullable-annotation commit (`string` → `string?`, `= null!` on two Dapper row properties) and a rebase — neither can change runtime behaviour. So the fixture carries a latent ordering/lifetime bug that simply won the race the first time. I ruled out the obvious candidate: the fixture is `[NonParallelizable]` and there is no assembly-level `Parallelizable`. Being explicit about the irony, because it is the actual lesson: this gate was added *because* two data-destroying collation defects escaped an inadequate test. Shipping it in a state where it intermittently fails would be worse than not adding it — an intermittently-red gate trains everyone to re-run rather than read, which is exactly how the original escapes happened. So it gets fixed deterministically, with no `[Retry]`, and the fix must be demonstrated over repeated consecutive runs rather than one green one. The red-when-broken control also has to be re-confirmed afterwards, since the most likely bad fix is one that quietly neuters the assertion. Diagnosis in progress — candidate causes include eager `ServerVersion.AutoDetect` connecting at configuration time, and MySqlConnector's connection pool handing back a session bound to a database a prior `DROP DATABASE` removed (pools are keyed by connection string, so a `ClearPool` after the drop may be required). Review-verdict: BLOCKED @ 4c320e6f
timothy force-pushed fix/491-libraryfolder-unique-index from 4c320e6fea to 1e65810454 2026-07-25 20:04:58 +02:00 Compare
timothy commented 2026-07-25 20:05:16 +02:00
Author
Owner
Copy Link
Copy Source

Fixture flake fixed, and the root cause was found by measurement rather than inspection — a standalone harness replayed the fixture's exact database lifecycle against real MySQL 8.4, without the slow migration chain.

Two mechanisms, both confirmed empirically.

  1. A pooled session outlives DROP DATABASE. Reopening a dropped database's connection string succeeds when MySqlConnector hands back the still-alive pooled session — so the outcome depends on whether the pool reuses a session or has to open a fresh one, and a fresh handshake names the dropped schema and fails 1049. That is the non-determinism, and it explains why the throw was at ServerSession.ConnectAsync rather than inside the migration. It is invisible in any single green run. The obvious suspect was also ruled out empirically: ServerVersion.AutoDetect strips the database from the connection string, so it never validates existence and could not have thrown.
  2. A per-test database name leaked a connection pool per test. MySqlConnector keys pools by connection string, so etv491_<guid> created a new pool every test, never cleared — measured at ~1 leaked pool per iteration, dying at Too many connections after 400 iterations. Striking secondary symptom: a run against a leak-saturated server took 13 minutes versus 1m21s clean. "Slow" and "flaky" were the same defect.

Fix is structural, no retries. One fixed database name (etv491_dedupe) ⇒ exactly one pool. EF owns the lifecycle via a guarded EnsureDeletedAsync through the same connection string it migrates with; the pool is explicitly cleared after every drop, and the drop moved to the start of a test, so no test ever leaves a live pool pointing at a database it just destroyed. TearDown no longer touches MySQL. Design validated in the harness first: leak/iteration 1.0 → 0.08.

Determinism demonstrated over 10 consecutive runs, all Failed: 0, Passed: 4, Skipped: 0 at ~1m20s, with server threads flat at 2 throughout — the leak is gone, not merely smaller, and there is no CI cost.

Negative control re-confirmed after the change — this was the failure mode I was most worried about, since the tempting bad fix is one that quietly neuters the assertion. Restoring COLLATE utf8mb4_bin on the fixed fixture still fails with survivors [1,4,5,6,7,9] instead of [1,4,5,6,7,9,10]; row 10, the trailing-space sibling, is still detected as deleted. Migration restored and verified afterwards (both CONVERT(Path USING binary) sites present).

Constraints held: one shared fixture body across providers, no [Retry], still fail-closed under ETV_REQUIRE_MYSQL_TESTS.

Rebased onto 798ecd1f (append/append docs/decisions.md again; all records kept, catalog regenerated, no drift). At head: ErsatzTV.Tests 1897 passed / 0 failed / 2 visibly skipped locally; Scanner 1504, Infrastructure 114, Core 652 (+1 skipped), Architecture 7. decisions_validate OK, BOM and format clean.

The decision record now carries the diagnosis, so the next person writing a MySQL-backed fixture inherits the rule instead of rediscovering it: don't drop your own database, and don't vary its name.

Review-verdict: MERGEABLE @ 1e658104

Fixture flake fixed, and the root cause was found by measurement rather than inspection — a standalone harness replayed the fixture's exact database lifecycle against real MySQL 8.4, without the slow migration chain. **Two mechanisms, both confirmed empirically.** 1. **A pooled session outlives `DROP DATABASE`.** Reopening a dropped database's connection string *succeeds* when MySqlConnector hands back the still-alive pooled session — so the outcome depends on whether the pool **reuses** a session or has to open a **fresh** one, and a fresh handshake names the dropped schema and fails 1049. That is the non-determinism, and it explains why the throw was at `ServerSession.ConnectAsync` rather than inside the migration. It is invisible in any single green run. The obvious suspect was also ruled out empirically: `ServerVersion.AutoDetect` strips the database from the connection string, so it never validates existence and could not have thrown. 2. **A per-test database name leaked a connection pool per test.** MySqlConnector keys pools by connection string, so `etv491_<guid>` created a new pool every test, never cleared — measured at ~1 leaked pool per iteration, dying at `Too many connections` after 400 iterations. Striking secondary symptom: a run against a leak-saturated server took **13 minutes** versus 1m21s clean. "Slow" and "flaky" were the same defect. **Fix is structural, no retries.** One fixed database name (`etv491_dedupe`) ⇒ exactly one pool. EF owns the lifecycle via a guarded `EnsureDeletedAsync` through the same connection string it migrates with; the pool is explicitly cleared after every drop, and the drop moved to the **start** of a test, so no test ever leaves a live pool pointing at a database it just destroyed. `TearDown` no longer touches MySQL. Design validated in the harness first: leak/iteration 1.0 → 0.08. **Determinism demonstrated over 10 consecutive runs**, all `Failed: 0, Passed: 4, Skipped: 0` at ~1m20s, with server threads **flat at 2** throughout — the leak is gone, not merely smaller, and there is no CI cost. **Negative control re-confirmed *after* the change** — this was the failure mode I was most worried about, since the tempting bad fix is one that quietly neuters the assertion. Restoring `COLLATE utf8mb4_bin` on the fixed fixture still fails with survivors `[1,4,5,6,7,9]` instead of `[1,4,5,6,7,9,10]`; row 10, the trailing-space sibling, is still detected as deleted. Migration restored and verified afterwards (both `CONVERT(Path USING binary)` sites present). Constraints held: one shared fixture body across providers, no `[Retry]`, still fail-closed under `ETV_REQUIRE_MYSQL_TESTS`. Rebased onto `798ecd1f` (append/append `docs/decisions.md` again; all records kept, catalog regenerated, no drift). At head: `ErsatzTV.Tests` 1897 passed / 0 failed / 2 visibly skipped locally; Scanner 1504, Infrastructure 114, Core 652 (+1 skipped), Architecture 7. `decisions_validate` OK, BOM and format clean. The decision record now carries the diagnosis, so the next person writing a MySQL-backed fixture inherits the rule instead of rediscovering it: **don't drop your own database, and don't vary its name.** Review-verdict: MERGEABLE @ 1e658104
timothy force-pushed fix/491-libraryfolder-unique-index from 1e65810454 to 8e83183b8b 2026-07-25 21:16:55 +02:00 Compare
timothy commented 2026-07-25 21:17:16 +02:00
Author
Owner
Copy Link
Copy Source

Fixture determinism fixed properly this time, and the decision record migrated to the new corpus format.

Root cause of failure #2, verified rather than assumed. The previous fix bundled two changes and only one was needed. The leak was caused by never clearing pools, not by unique names — measured directly: unique database name per iteration with ClearPoolAsync gives threads before=2 after=2 over 30 iters → leak/iter = 0. Collapsing to a single fixed name therefore gave up per-test isolation to solve a problem pool-clearing had already solved, and that lost isolation is what produced Duplicate entry '1' for key 'LibraryPath.PRIMARY' — the second test seeded on top of the first's rows in 539ms, never re-migrating, because MigrateAsync no-opped against an already-current __EFMigrationsHistory.

The deeper point, which is now in the record: a shared database makes correctness depend on a wipe succeeding, and when the wipe silently doesn't you get cross-test contamination rather than a loud failure. A database name that has never been used cannot contain another test's rows — the class is gone by construction instead of by a wipe working.

Shape now: fresh etv491_<guid> per test, never created out of band (the test's own MigrateAsync(PreviousMigration) creates it, so EF solely owns the schema), dropped in TearDown via guarded EnsureDeletedAsync followed by ClearPoolAsync on that exact connection string.

Evidence — and note the dimension that was missing before. Ten consecutive runs, all Failed: 0, Passed: 4, threads flat at 2 and zero leftover etv491% schemas every time. Plus, newly, order independence: each test alone twice, and both together with the order reversed — all green. That is precisely the dimension that would have caught failure #2, which was invisible whenever the affected test ran first. Both prior fixes were verified in one dimension only; that is the actual lesson here, more than either bug.

Negative control re-confirmed after the change: restoring COLLATE utf8mb4_bin still drops row 10 ([1,4,5,6,7,9] vs [1,4,5,6,7,9,10]), then restored and green. No [Retry], one shared fixture body, still fail-closed under ETV_REQUIRE_MYSQL_TESTS.

Corpus migration. #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; catalog regenerated, decisions_validate OK. The record carries both fixture failures and the corrected rule, including that the "fixed name" conclusion was wrong and why.

At head: ErsatzTV.Tests 1897 passed / 0 failed / 2 visibly skipped locally (1899 / 0 skipped with MySQL reachable); Scanner 1504, Infrastructure 114, Core 652 (+1 skipped), Architecture 7. Migration files unchanged and verified (both CONVERT(Path USING binary) sites intact).

Review-verdict: MERGEABLE @ 8e83183b

Fixture determinism fixed properly this time, and the decision record migrated to the new corpus format. **Root cause of failure #2, verified rather than assumed.** The previous fix bundled two changes and only one was needed. The leak was caused by *never clearing pools*, not by unique names — measured directly: unique database name per iteration **with** `ClearPoolAsync` gives `threads before=2 after=2 over 30 iters → leak/iter = 0`. Collapsing to a single fixed name therefore gave up per-test isolation to solve a problem pool-clearing had already solved, and that lost isolation is what produced `Duplicate entry '1' for key 'LibraryPath.PRIMARY'` — the second test seeded on top of the first's rows in 539ms, never re-migrating, because `MigrateAsync` no-opped against an already-current `__EFMigrationsHistory`. The deeper point, which is now in the record: a shared database makes correctness depend on a wipe *succeeding*, and when the wipe silently doesn't you get cross-test contamination rather than a loud failure. A database name that has never been used cannot contain another test's rows — the class is gone by construction instead of by a wipe working. **Shape now:** fresh `etv491_<guid>` per test, never created out of band (the test's own `MigrateAsync(PreviousMigration)` creates it, so EF solely owns the schema), dropped in `TearDown` via guarded `EnsureDeletedAsync` followed by `ClearPoolAsync` on that exact connection string. **Evidence — and note the dimension that was missing before.** Ten consecutive runs, all `Failed: 0, Passed: 4`, threads flat at 2 and **zero leftover `etv491%` schemas** every time. Plus, newly, **order independence**: each test alone twice, and both together with the order reversed — all green. That is precisely the dimension that would have caught failure #2, which was invisible whenever the affected test ran first. Both prior fixes were verified in one dimension only; that is the actual lesson here, more than either bug. Negative control re-confirmed *after* the change: restoring `COLLATE utf8mb4_bin` still drops row 10 (`[1,4,5,6,7,9]` vs `[1,4,5,6,7,9,10]`), then restored and green. No `[Retry]`, one shared fixture body, still fail-closed under `ETV_REQUIRE_MYSQL_TESTS`. **Corpus migration.** #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; catalog regenerated, `decisions_validate` OK. The record carries both fixture failures and the corrected rule, including that the "fixed name" conclusion was wrong and why. At head: `ErsatzTV.Tests` 1897 passed / 0 failed / 2 visibly skipped locally (1899 / 0 skipped with MySQL reachable); Scanner 1504, Infrastructure 114, Core 652 (+1 skipped), Architecture 7. Migration files unchanged and verified (both `CONVERT(Path USING binary)` sites intact). Review-verdict: MERGEABLE @ 8e83183b
timothy added 1 commit 2026-07-25 21:43:20 +02:00
ci(491): split the MySql dedupe fixture out of CI, tracked by #627
PR Gates / Docs update reminder (pull_request) Successful in 18s
Details
PR Gates / CI image pin matches docker/ci (pull_request) Successful in 21s
Details
PR Gates / decisions lifecycle (pull_request) Successful in 36s
Details
Build ErsatzTV Image / API docs in sync (OpenAPI + endpoint index) (pull_request) Successful in 1m29s
Details
Build ErsatzTV Image / Formatting (changed .cs conform to .editorconfig) (pull_request) Successful in 1m30s
Details
Build ErsatzTV Image / Build & test (.NET) (pull_request) Successful in 8m34s
Details
Build ErsatzTV Image / Functional E2E (curl + UI contracts) (pull_request) Successful in 16m27s
Details
Build ErsatzTV Image / EF migration integrity (SQLite + MySql) (pull_request) Successful in 17m17s
Details
Build ErsatzTV Image / Build & push image (amd64) (pull_request) Has been skipped
Details
960145348b 
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]
timothy commented 2026-07-25 21:43:37 +02:00
Author
Owner
Copy Link
Copy Source

Third CI failure on the fixture, so I'm taking the cap I set earlier: splitting the MySQL CI coverage out and merging the production fix without it. Tracked as #627.

The third failure was failure #1's mode returning by a different route — Unknown database 'etv491_<guid>', this time with TearDown reporting the database never existed at all, so something opens a connection before MigrateAsync creates it. Three attempts, three distinct root causes, all in the test harness:

  1. Stale pooled session surviving DROP DATABASE (plus a pool leak per test — ~1/iteration, and a saturated server turned a 1m21s run into 13 minutes).
  2. Lost isolation from collapsing to a shared database name — Duplicate entry '1', second test seeding on the first's rows.
  3. Connect-before-create on the restored unique names.

Why split rather than keep iterating. The production change — unique index, repository fix, dual-provider migration, scanner classifier wiring — has been reviewed-green for several rounds, including a cross-family pass that caught the PAD SPACE data-loss bug. Every failure since has been in the harness around it. And by my own argument for adding the gate, shipping it unreliable would be self-defeating: an intermittently-red gate trains people to re-run instead of read, which is exactly how the two collation defects escaped originally. A gate that cries wolf is worse than a documented gap.

What ships: the fixture stays, parameterized over both providers from one shared body, opt-in via ETV_TEST_MYSQL_CONNECTION with a visible skip and ETV_REQUIRE_MYSQL_TESTS=1 still turning that skip into a hard failure for whoever re-arms it. Only the CI wiring is removed, with a note in the migrations job where it belongs pointing at #627.

I also corrected the decision record, which described the CI step as if it existed — on a file that has now had two collation bugs, a record teaching something false is a real cost. It now states plainly that the MySQL half has no automated coverage, that this is tracked, why the gate was removed, and that the fixture should be run by hand against a real server before touching this migration.

Removing the CI step does not touch the migration or repository code; both CONVERT(Path USING binary) sites verified intact, and the migrations job's original SQLite/MySQL drift-and-apply checks are unchanged.

Review-verdict: MERGEABLE @ 96014534

Third CI failure on the fixture, so I'm taking the cap I set earlier: **splitting the MySQL CI coverage out** and merging the production fix without it. Tracked as #627. The third failure was failure #1's mode returning by a different route — `Unknown database 'etv491_<guid>'`, this time with TearDown reporting the database **never existed at all**, so something opens a connection before `MigrateAsync` creates it. Three attempts, three distinct root causes, all in the test harness: 1. Stale pooled session surviving `DROP DATABASE` (plus a pool leak per test — ~1/iteration, and a saturated server turned a 1m21s run into 13 minutes). 2. Lost isolation from collapsing to a shared database name — `Duplicate entry '1'`, second test seeding on the first's rows. 3. Connect-before-create on the restored unique names. **Why split rather than keep iterating.** The production change — unique index, repository fix, dual-provider migration, scanner classifier wiring — has been reviewed-green for several rounds, including a cross-family pass that caught the PAD SPACE data-loss bug. Every failure since has been in the harness *around* it. And by my own argument for adding the gate, shipping it unreliable would be self-defeating: an intermittently-red gate trains people to re-run instead of read, which is exactly how the two collation defects escaped originally. A gate that cries wolf is worse than a documented gap. **What ships:** the fixture stays, parameterized over both providers from one shared body, opt-in via `ETV_TEST_MYSQL_CONNECTION` with a visible skip and `ETV_REQUIRE_MYSQL_TESTS=1` still turning that skip into a hard failure for whoever re-arms it. Only the CI wiring is removed, with a note in the `migrations` job where it belongs pointing at #627. I also corrected the decision record, which described the CI step as if it existed — on a file that has now had two collation bugs, a record teaching something false is a real cost. It now states plainly that the MySQL half has no automated coverage, that this is tracked, why the gate was removed, and that the fixture should be run by hand against a real server before touching this migration. Removing the CI step does not touch the migration or repository code; both `CONVERT(Path USING binary)` sites verified intact, and the `migrations` job's original SQLite/MySQL drift-and-apply checks are unchanged. Review-verdict: MERGEABLE @ 96014534
timothy merged commit e03c36e371 into main 2026-07-25 22:00:45 +02:00
timothy deleted branch fix/491-libraryfolder-unique-index 2026-07-25 22:00:46 +02:00
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
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: timothy/ersatztv#613