docs(491): migrate the decision record to the per-file corpus format
Build ErsatzTV Image / Formatting (changed .cs conform to .editorconfig) (pull_request) Successful in 15s
PR Gates / CI image pin matches docker/ci (pull_request) Successful in 18s
PR Gates / Docs update reminder (pull_request) Successful in 21s
PR Gates / decisions lifecycle (pull_request) Successful in 36s
Build ErsatzTV Image / API docs in sync (OpenAPI + endpoint index) (pull_request) Successful in 1m27s
Build ErsatzTV Image / Functional E2E (curl + UI contracts) (pull_request) Successful in 17m30s
Build ErsatzTV Image / Build & test (.NET) (pull_request) Successful in 22m1s
Build ErsatzTV Image / EF migration integrity (SQLite + MySql) (pull_request) Failing after 22m24s
Build ErsatzTV Image / Build & push image (amd64) (pull_request) Has been skipped
Build ErsatzTV Image / Formatting (changed .cs conform to .editorconfig) (pull_request) Successful in 15s
PR Gates / CI image pin matches docker/ci (pull_request) Successful in 18s
PR Gates / Docs update reminder (pull_request) Successful in 21s
PR Gates / decisions lifecycle (pull_request) Successful in 36s
Build ErsatzTV Image / API docs in sync (OpenAPI + endpoint index) (pull_request) Successful in 1m27s
Build ErsatzTV Image / Functional E2E (curl + UI contracts) (pull_request) Successful in 17m30s
Build ErsatzTV Image / Build & test (.NET) (pull_request) Successful in 22m1s
Build ErsatzTV Image / EF migration integrity (SQLite + MySql) (pull_request) Failing after 22m24s
Build ErsatzTV Image / Build & push image (amd64) (pull_request) Has been skipped
#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.
This commit is contained in:
@@ -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)
|
||||
|
||||
@@ -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) |
|
||||
|
||||
@@ -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_<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 `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.
|
||||
Reference in New Issue
Block a user