fix(308): idempotent concurrent Add*ToCollection instead of a composite-PK 500
Build ErsatzTV Image / CI image pin matches docker/ci (pull_request) Successful in 8s
Build ErsatzTV Image / API docs in sync (OpenAPI + endpoint index) (pull_request) Successful in 12s
Build ErsatzTV Image / Docs update reminder (pull_request) Successful in 8s
Build ErsatzTV Image / decisions.md append-only (pull_request) Successful in 40s
Build ErsatzTV Image / Formatting (changed .cs conform to .editorconfig) (pull_request) Successful in 7m25s
Build ErsatzTV Image / Functional E2E (curl contracts) (pull_request) Successful in 14m59s
Build ErsatzTV Image / EF migration integrity (SQLite + MySql) (pull_request) Successful in 19m3s
Build ErsatzTV Image / Build & test (.NET) (pull_request) Successful in 19m47s
Build ErsatzTV Image / Build & push image (amd64) (pull_request) Has been skipped
Build ErsatzTV Image / CI image pin matches docker/ci (pull_request) Successful in 8s
Build ErsatzTV Image / API docs in sync (OpenAPI + endpoint index) (pull_request) Successful in 12s
Build ErsatzTV Image / Docs update reminder (pull_request) Successful in 8s
Build ErsatzTV Image / decisions.md append-only (pull_request) Successful in 40s
Build ErsatzTV Image / Formatting (changed .cs conform to .editorconfig) (pull_request) Successful in 7m25s
Build ErsatzTV Image / Functional E2E (curl contracts) (pull_request) Successful in 14m59s
Build ErsatzTV Image / EF migration integrity (SQLite + MySql) (pull_request) Successful in 19m3s
Build ErsatzTV Image / Build & test (.NET) (pull_request) Successful in 19m47s
Build ErsatzTV Image / Build & push image (amd64) (pull_request) Has been skipped
Two concurrent adds of the same item both membership-check it absent, both insert the CollectionItem composite key, and the loser's SaveChangesForcingVersion threw an uncaught DbUpdateException (SQLite 19 / MySQL 1062) -> 500. Now the loser is an idempotent no-op. - ConcurrencyExtensions.TrySaveChangesForcingVersion: bool-returning sibling that catches only a classified unique/PK violation and returns false. - 10 single-item Add*ToCollection handlers: return Unit.Default (no-op, skip fan-out) on false — the racing winner already inserted + rotated + rebuilt. - Bulk AddItemsToCollection: retry on a fresh context against recomputed membership so a partial-overlap collision doesn't drop the non-colliding items (bounded loop; common no-collision path runs once). - Provider detection via a TvContext.IsUniqueConstraintViolation static delegate (matches the existing IsSqlite/LastInsertedRowId provider seam), wired from Startup to SqliteErrorClassifier / MySqlErrorClassifier. - Add*ToPlaylist is NOT affected (PlaylistItem has its own identity PK; a playlist may legitimately contain the same item more than once). Tests: a negative-control anchor proves the race genuinely throws a classified exception; end-to-end handler tests reproduce a real cross-connection race via a shared-cache SQLite harness + a SavingChanges interceptor (the single-conn in-memory fixture cannot). Every fix-dependent test verified to fail with the catch disabled. Docs: api-conventions.md §7a (idempotent insert under concurrency) + decisions/optimistic-concurrency.md. fixes #308 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -17,6 +17,7 @@ cross-editor ETag rotation). Refs #197.
|
||||
- [2026-07-12 (#269 — non-If-Match root writers force-write past a concurrent Version bump)](#2026-07-12-269--non-if-match-root-writers-force-write-past-a-concurrent-version-bump)
|
||||
- [2026-07-12 — Cross-editor ETag rotation completed for Collection/Playout config siblings (#269)](#2026-07-12--cross-editor-etag-rotation-completed-for-collectionplayout-config-siblings-269)
|
||||
- [2026-07-12 — If-Match evaluates per RFC 7232: valid-but-non-matching → 412, only grammar violations → 400 (#265)](#2026-07-12--if-match-evaluates-per-rfc-7232-valid-but-non-matching--412-only-grammar-violations--400-265)
|
||||
- [2026-07-18 — Concurrent same-item add is idempotent, not a 500: catch the unique-violation per provider (#308)](#2026-07-18--concurrent-same-item-add-is-idempotent-not-a-500-catch-the-unique-violation-per-provider-308)
|
||||
|
||||
---
|
||||
|
||||
@@ -254,3 +255,36 @@ worried about was already correct (each handler loads/validates → 404 before `
|
||||
first-party SPA only ever echoes the single canonical strong tag we emit, so no shipped client changes behavior;
|
||||
the change only makes a hand-written/tooling `If-Match` get the RFC-correct status. Docs: `api-conventions.md`
|
||||
§7a. Refs #265 #253 #197.
|
||||
|
||||
## 2026-07-18 — Concurrent same-item add is idempotent, not a 500: catch the unique-violation per provider (#308)
|
||||
|
||||
**Decision.** The `Add*ToCollection` family's membership pre-check (#269) is not atomic with the insert, so two
|
||||
*concurrent* adds of the same item both observe it absent and both stage the `CollectionItem` composite key; the
|
||||
loser's `SaveChangesForcingVersion` threw a unique/PK-violation `DbUpdateException` (SQLite error 19 / MySQL 1062)
|
||||
it did not catch → **500**. We now treat that loss as an **idempotent no-op**, not an error: the desired end state
|
||||
(the item is a member) already holds because the racing winner inserted it, rotated the ETag, and fanned out the
|
||||
rebuild.
|
||||
|
||||
**Mechanism.** A `bool`-returning sibling `ConcurrencyExtensions.TrySaveChangesForcingVersion` wraps
|
||||
`SaveChangesForcingVersion` and catches *only* a classified unique/PK violation, returning `false`. The 10
|
||||
single-item handlers return `Unit.Default` on `false` (skip the reindex/rebuild fan-out — the winner did it). The
|
||||
bulk `AddItemsToCollection` handler cannot no-op — that would silently drop the non-colliding items when a batch
|
||||
partially overlaps a concurrent add — so it **retries** on a fresh context against recomputed membership (bounded
|
||||
loop; the common no-collision path runs once).
|
||||
|
||||
**Provider seam.** Detection is provider-specific but the Application layer must not reference the provider
|
||||
packages, so it follows the existing `TvContext` static-provider-config idiom (`IsSqlite`, `LastInsertedRowId`): a
|
||||
settable `TvContext.IsUniqueConstraintViolation` delegate, pointed at `SqliteErrorClassifier` (extended codes 1555
|
||||
PK / 2067 UNIQUE) or `MySqlErrorClassifier` (`Number == 1062`) from `Startup.cs`, defaulting to a conservative
|
||||
"no" so an unwired provider never silently swallows a save failure. Chosen over DI to avoid threading a new
|
||||
service through 11 handlers, and because the provider discriminator already lives as a `TvContext` static.
|
||||
|
||||
**Scope boundary.** `Add*ToPlaylist` is deliberately **untouched**: `PlaylistItem` has its own identity PK and no
|
||||
unique index on `(PlaylistId, MediaItemId)` — a playlist may legitimately contain the same item more than once, so
|
||||
there is no constraint to violate.
|
||||
|
||||
**Tests.** A negative-control anchor proves the race genuinely throws a classified `DbUpdateException`; the fix's
|
||||
end-to-end handler tests reproduce a *real cross-connection* race via a shared-cache SQLite harness + a
|
||||
`SavingChanges` interceptor that inserts the conflicting row on another connection mid-save (the single-connection
|
||||
in-memory fixture cannot). Every fix-dependent test was verified to fail with the catch disabled. Mechanics:
|
||||
`api-conventions.md` §7a ("Idempotent insert under concurrency"). Refs #308 #269 #253.
|
||||
|
||||
Reference in New Issue
Block a user