fix(308): idempotent concurrent Add*ToCollection instead of a composite-PK 500 #441

2026-07-18T13:02:49+02:00

timothy commented

2026-07-18 13:02:49 +02:00

Fixes #308.

Problem

The Add*ToCollection family's membership pre-check (#269) is not atomic with the insert. Two concurrent adds of the same item both observe it absent, both stage the CollectionItem composite key (CollectionId, MediaItemId), and the loser's SaveChangesForcingVersion throws a unique/PK-violation DbUpdateException (SQLite error 19 / MySQL 1062) that it does not catch → 500.

Fix

ConcurrencyExtensions.TrySaveChangesForcingVersion — a bool-returning sibling of SaveChangesForcingVersion that catches only a classified unique/PK violation and returns false.
10 single-item handlers (Add{Artist,Episode,Image,MediaItem,Movie,MusicVideo,OtherVideo,Season,Show,Song}ToCollection) return Unit.Default on false — an idempotent no-op that skips the reindex/rebuild fan-out (the racing winner already inserted the row, rotated the ETag, and rebuilt).
Bulk AddItemsToCollection can't no-op — that would silently drop the non-colliding items in a partially-overlapping batch — so it retries on a fresh context against recomputed membership (bounded loop; the common no-collision path runs the body once).
Provider detection follows the existing TvContext static-provider-config seam (IsSqlite, LastInsertedRowId): a settable TvContext.IsUniqueConstraintViolation delegate pointed at SqliteErrorClassifier (extended codes 1555 PK / 2067 UNIQUE) / 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.

Scope boundary

Add*ToPlaylist is not affected and 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

Negative-control anchor: proves the race genuinely throws a DbUpdateException the classifier recognizes (and that the classifier doesn't blanket-match unrelated failures) — so the fix's catch filter isn't a no-op.
Extension unit tests: TrySaveChangesForcingVersion swallows the duplicate (returns false, one row); and re-throws when the provider classifier says "not a unique violation" (guards over-broad catch).
End-to-end handler tests reproduce a real cross-connection race via a new shared-cache SQLite harness + a SavingChanges interceptor that inserts the conflicting row on another connection mid-save (the single-connection in-memory fixture can't). Single-item → no 500, one row, no fan-out; bulk → retries and keeps the non-colliding item.
Every fix-dependent test verified to fail with the catch disabled (per the #231 concurrency-test discipline). Full suite green (~3,800 tests).

Not needed

No /api/* endpoint/DTO change → no OpenAPI regeneration.
No TvContext model change (the detector is a static delegate, not a mapped column) → no migration.

Docs

docs/api-conventions.md §7a — new "Idempotent insert under concurrency (#308)" paragraph.
docs/decisions/optimistic-concurrency.md — dated decision entry.

Independent cold-context review + live-E2E (concurrent add) to follow.

Fixes #308. ## Problem The `Add*ToCollection` family's membership pre-check (#269) is not atomic with the insert. Two **concurrent** adds of the *same* item both observe it absent, both stage the `CollectionItem` composite key (`CollectionId, MediaItemId`), and the loser's `SaveChangesForcingVersion` throws a unique/PK-violation `DbUpdateException` (SQLite error 19 / MySQL 1062) that it does **not** catch → **500**. ## Fix - **`ConcurrencyExtensions.TrySaveChangesForcingVersion`** — a `bool`-returning sibling of `SaveChangesForcingVersion` that catches *only* a classified unique/PK violation and returns `false`. - **10 single-item handlers** (`Add{Artist,Episode,Image,MediaItem,Movie,MusicVideo,OtherVideo,Season,Show,Song}ToCollection`) return `Unit.Default` on `false` — an idempotent no-op that skips the reindex/rebuild fan-out (the racing winner already inserted the row, rotated the ETag, and rebuilt). - **Bulk `AddItemsToCollection`** can't no-op — that would silently drop the non-colliding items in a partially-overlapping batch — so it **retries** on a fresh context against recomputed membership (bounded loop; the common no-collision path runs the body once). - **Provider detection** follows the existing `TvContext` static-provider-config seam (`IsSqlite`, `LastInsertedRowId`): a settable `TvContext.IsUniqueConstraintViolation` delegate pointed at `SqliteErrorClassifier` (extended codes 1555 PK / 2067 UNIQUE) / `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. ## Scope boundary `Add*ToPlaylist` is **not** affected and 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 - **Negative-control anchor**: proves the race genuinely throws a `DbUpdateException` the classifier recognizes (and that the classifier doesn't blanket-match unrelated failures) — so the fix's catch filter isn't a no-op. - **Extension unit tests**: `TrySaveChangesForcingVersion` swallows the duplicate (returns `false`, one row); and *re-throws* when the provider classifier says "not a unique violation" (guards over-broad catch). - **End-to-end handler tests** reproduce a **real cross-connection race** via a new shared-cache SQLite harness + a `SavingChanges` interceptor that inserts the conflicting row on another connection mid-save (the single-connection in-memory fixture can't). Single-item → no 500, one row, no fan-out; bulk → retries and keeps the non-colliding item. - **Every fix-dependent test verified to fail with the catch disabled** (per the #231 concurrency-test discipline). Full suite green (~3,800 tests). ## Not needed - No `/api/*` endpoint/DTO change → no OpenAPI regeneration. - No `TvContext` model change (the detector is a static delegate, not a mapped column) → no migration. ## Docs - `docs/api-conventions.md` §7a — new "Idempotent insert under concurrency (#308)" paragraph. - `docs/decisions/optimistic-concurrency.md` — dated decision entry. Independent cold-context review + live-E2E (concurrent add) to follow.

timothy added 1 commit 2026-07-18 13:02:50 +02:00

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

Details

Build ErsatzTV Image / API docs in sync (OpenAPI + endpoint index) (pull_request) Successful in 12s

Details

Build ErsatzTV Image / Docs update reminder (pull_request) Successful in 8s

Details

Build ErsatzTV Image / decisions.md append-only (pull_request) Successful in 40s

Details

Build ErsatzTV Image / Formatting (changed .cs conform to .editorconfig) (pull_request) Successful in 7m25s

Details

Build ErsatzTV Image / Functional E2E (curl contracts) (pull_request) Successful in 14m59s

Details

Build ErsatzTV Image / EF migration integrity (SQLite + MySql) (pull_request) Successful in 19m3s

Details

Build ErsatzTV Image / Build & test (.NET) (pull_request) Successful in 19m47s

Details

Build ErsatzTV Image / Build & push image (amd64) (pull_request) Has been skipped

Details

2281f2e764

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>

timothy commented

2026-07-18 13:09:16 +02:00

Review-verdict: MERGEABLE @ 2281f2e764

Independent cold-context adversarial review (fresh Opus agent, no implementation role; cross-model Codex not routed this window — cold same-model per the skip rubric, stated). Verified all six risk areas plus the playlist-scope claim and test non-vacuity:

Try/Save × concurrency retry: a DB constraint violation is always a plain DbUpdateException, never the rowcount-mismatch DbUpdateConcurrencyException, so the inner retry never intercepts it and a genuine deleted-row conflict (null/non-provider inner) still propagates. Version++ and the CollectionItem insert are one transaction → no partial commit on the swallowed false.
Bulk retry: fresh context per attempt (clean tracker), membership recomputed, toAddIds strictly shrinks (no livelock), exhaustion returns a clean BaseError not a 500, Either<BaseError,bool> threading can't misread a Left as committed, reindex fan-out uses the per-attempt inserted set.
Classifier: SQLite narrowed to extended 1555/2067 (FK/NOT NULL/CHECK excluded), MySQL 1062 only; the handlers stage only a CollectionItem insert whose sole unique constraint is the composite PK — Collection.Name's unique index can't fire (no insert/rename). Mis-classification-→-silent-data-loss not reachable.
Static delegate: conservative _ => false default; no assembly-level parallelism so the one static-mutating test can't race.
Fan-out / CancellationToken: no-op skips reindex/rebuild (winner did it); post-commit on CancellationToken.None (§7b).
Tests: real cross-connection race via the SavingChanges interceptor (separate connection, Interlocked once, before EF opens its transaction — no SQLITE_BUSY), deterministic; negative control proves non-vacuous; bulk test genuinely exercises the retry.

Two Low/non-blocking notes (both acknowledged-intentional): the conservative default means a future unwired provider silently reverts to the 500 rather than failing loudly (a startup assertion would be defense-in-depth); and the test factory relies on CreateDbContextAsync's default sync-wrap. No action taken.

Live-E2E (local instance, real SQLite WAL) — the decisive confirmation: 40 concurrent same-item adds + 30 concurrent overlapping-batch adds all returned 204, exactly one row per item, zero unhandled 500s, and the server log recorded 6 genuine UNIQUE constraint failed: CollectionItem.CollectionId, MediaItemId collisions — i.e. the race actually fired and every one was caught. Pre-fix those six are 500s. Bulk overlap kept both items (retry dropped nothing).

Review-verdict: MERGEABLE @ 2281f2e764d74b6812ea0d4e79ea22ffdb78255e Independent cold-context adversarial review (fresh Opus agent, no implementation role; cross-model Codex not routed this window — cold same-model per the skip rubric, stated). Verified all six risk areas plus the playlist-scope claim and test non-vacuity: - **Try/Save × concurrency retry**: a DB constraint violation is always a plain `DbUpdateException`, never the rowcount-mismatch `DbUpdateConcurrencyException`, so the inner retry never intercepts it and a genuine deleted-row conflict (null/non-provider inner) still propagates. Version++ and the CollectionItem insert are one transaction → no partial commit on the swallowed `false`. - **Bulk retry**: fresh context per attempt (clean tracker), membership recomputed, `toAddIds` strictly shrinks (no livelock), exhaustion returns a clean `BaseError` not a 500, `Either<BaseError,bool>` threading can't misread a Left as committed, reindex fan-out uses the per-attempt inserted set. - **Classifier**: SQLite narrowed to extended 1555/2067 (FK/NOT NULL/CHECK excluded), MySQL 1062 only; the handlers stage only a `CollectionItem` insert whose sole unique constraint is the composite PK — `Collection.Name`'s unique index can't fire (no insert/rename). Mis-classification-→-silent-data-loss not reachable. - **Static delegate**: conservative `_ => false` default; no assembly-level parallelism so the one static-mutating test can't race. - **Fan-out / CancellationToken**: no-op skips reindex/rebuild (winner did it); post-commit on `CancellationToken.None` (§7b). - **Tests**: real cross-connection race via the `SavingChanges` interceptor (separate connection, `Interlocked` once, before EF opens its transaction — no SQLITE_BUSY), deterministic; negative control proves non-vacuous; bulk test genuinely exercises the retry. Two Low/non-blocking notes (both acknowledged-intentional): the conservative default means a *future* unwired provider silently reverts to the 500 rather than failing loudly (a startup assertion would be defense-in-depth); and the test factory relies on `CreateDbContextAsync`'s default sync-wrap. No action taken. **Live-E2E** (local instance, real SQLite WAL) — the decisive confirmation: 40 concurrent same-item adds + 30 concurrent overlapping-batch adds all returned **204**, exactly one row per item, **zero unhandled 500s**, and the server log recorded **6 genuine `UNIQUE constraint failed: CollectionItem.CollectionId, MediaItemId`** collisions — i.e. the race actually fired and every one was caught. Pre-fix those six are 500s. Bulk overlap kept both items (retry dropped nothing).

timothy merged commit 8a808e6205 into main

2026-07-18 13:29:02 +02:00

timothy deleted branch feat/308-idempotent-collection-add

2026-07-18 13:29:02 +02:00

timothy referenced this issue from a commit

2026-07-18 13:29:03 +02:00

Merge pull request 'fix(308): idempotent concurrent Add*ToCollection instead of a composite-PK 500' (#441) from feat/308-idempotent-collection-add into main

timothy referenced this pull request

2026-07-18 13:30:20 +02:00

Concurrent same-item Add*ToCollection can 500 on composite-PK violation (pre-existing idempotency-under-concurrency gap) #308

timothy referenced this pull request

2026-07-18 13:32:56 +02:00