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

New Issue

2026-07-12T19:20:36+02:00

timothy commented

2026-07-12 19:20:36 +02:00

Split out of #269 (PR #307) — Codex F2, ratified by Fable, deferred as out-of-scope for the rotation PR.

Problem

The Add*ToCollectionHandler family (11 handlers) short-circuits a sequential idempotent re-add via a membership check (collection.MediaItems.Any(mi => mi.Id == …)) — added in #269. But two concurrent adds of the same item both load the collection, both observe the item absent, both pass the guard, and both insert the same CollectionItem composite key (CollectionConfiguration.cs:33). The second insert throws a DbUpdateException (unique/PK violation, SQLite error 19 / MySQL 1062), which SaveChangesForcingVersion does not catch (it only catches DbUpdateConcurrencyException) → 500.

Severity: low

Pre-existing — before #269 the handlers .Add-then-plain-saved the same insert with no membership check, so the identical concurrent race already 500'd (or dup-inserted). #269 fixed the sequential case, not this one.
Narrow window: two identical requests racing.
No corruption — the composite PK guarantees at most one row; worst case is one 500 on a request whose desired outcome already exists.

Fix options (pick when scheduled)

Catch the provider-specific unique-violation on the insert and translate to an idempotent success/no-op (needs SQLite-19 / MySQL-1062 detection, or an IsUniqueViolation abstraction).
Re-check membership inside a catch-and-retry (analogous to SaveChangesForcingVersion's concurrency retry).

Apply uniformly across the 11 Add*ToCollection handlers (and consider the Add*ToPlaylist family, which has the same shape). RemoveItems/Update* are unaffected (no unique-key insert).

Refs #269 #253 · docs/api-conventions.md §7a.

Done-when

Unique-violation detector implemented for both providers (SQLite 19 / MySQL 1062) with a test
All 11 Add*ToCollection handlers made idempotent under concurrent same-item add; Add*ToPlaylist audited + fixed if affected (audited → not affected: PlaylistItem has its own identity PK, no unique index on (PlaylistId, MediaItemId) — duplicates are legal)
Concurrency stress test green with a proven non-vacuous negative control
Live-E2E of an affected add-to-collection write path passes (40 + 30 concurrent adds → all 204, 6 real DB collisions caught, 0 500s)
docs/api-conventions.md §7a updated to describe the idempotent-insert contract
Adversarial review passed (Review-verdict: MERGEABLE @ 2281f2e7)
CI green

Split out of #269 (PR #307) — Codex F2, ratified by Fable, deferred as out-of-scope for the rotation PR. ## Problem The `Add*ToCollectionHandler` family (11 handlers) short-circuits a *sequential* idempotent re-add via a membership check (`collection.MediaItems.Any(mi => mi.Id == …)`) — added in #269. But two **concurrent** adds of the *same* item both load the collection, both observe the item absent, both pass the guard, and both insert the same `CollectionItem` composite key (`CollectionConfiguration.cs:33`). The second insert throws a `DbUpdateException` (unique/PK violation, SQLite error 19 / MySQL 1062), which `SaveChangesForcingVersion` does **not** catch (it only catches `DbUpdateConcurrencyException`) → **500**. ## Severity: low - **Pre-existing** — before #269 the handlers `.Add`-then-plain-saved the same insert with no membership check, so the identical concurrent race already 500'd (or dup-inserted). #269 fixed the sequential case, not this one. - Narrow window: two *identical* requests racing. - No corruption — the composite PK guarantees at most one row; worst case is one 500 on a request whose desired outcome already exists. ## Fix options (pick when scheduled) 1. Catch the provider-specific unique-violation on the insert and translate to an idempotent success/no-op (needs SQLite-19 / MySQL-1062 detection, or an `IsUniqueViolation` abstraction). 2. Re-check membership inside a catch-and-retry (analogous to `SaveChangesForcingVersion`'s concurrency retry). Apply uniformly across the 11 `Add*ToCollection` handlers (and consider the `Add*ToPlaylist` family, which has the same shape). `RemoveItems`/`Update*` are unaffected (no unique-key insert). Refs #269 #253 · `docs/api-conventions.md` §7a. ## Done-when - [x] Unique-violation detector implemented for both providers (SQLite 19 / MySQL 1062) with a test - [x] All 11 `Add*ToCollection` handlers made idempotent under concurrent same-item add; `Add*ToPlaylist` audited + fixed if affected (audited → **not affected**: `PlaylistItem` has its own identity PK, no unique index on `(PlaylistId, MediaItemId)` — duplicates are legal) - [x] Concurrency stress test green **with** a proven non-vacuous negative control - [x] Live-E2E of an affected add-to-collection write path passes (40 + 30 concurrent adds → all 204, 6 real DB collisions caught, 0 500s) - [x] `docs/api-conventions.md` §7a updated to describe the idempotent-insert contract - [x] Adversarial review passed (`Review-verdict: MERGEABLE @ 2281f2e7`) - [x] CI green

timothy added the bug priority: low labels 2026-07-12 19:20:36 +02:00

timothy referenced this issue from a commit

2026-07-12 19:22:12 +02:00

fix(api): #269 review fixes — rebase force-write delta so rotation survives a race (Codex F1/F3)

timothy referenced this issue

2026-07-12 19:35:56 +02:00

fix(api): #269 rotate aggregate ETag on Collection/Playout config siblings #307

timothy referenced this issue

2026-07-12 19:36:27 +02:00

#253 follow-up: same-root config sibling writers don't rotate the aggregate ETag (cross-editor invalidation) #269

timothy referenced this issue from a commit

2026-07-12 19:41:42 +02:00

fix(api): #269 review fixes — rebase force-write delta so rotation survives a race (Codex F1/F3)

timothy referenced this issue

2026-07-12 19:50:55 +02:00

#253 follow-up: same-root config sibling writers don't rotate the aggregate ETag (cross-editor invalidation) #269

timothy referenced this issue

2026-07-12 19:52:10 +02:00

ChicoryTV rewrite — queue tracker (goal, arc, session protocol) #237

timothy referenced this issue

2026-07-12 19:52:39 +02:00

ChicoryTV rewrite — queue tracker (goal, arc, session protocol) #237

timothy referenced this issue

2026-07-12 23:33:24 +02:00

Replace-all PUTs have no optimistic concurrency — stale two-tab overwrite; define a shared If-Match/version contract before more editors ossify #253

timothy added priority: medium and removed priority: low labels 2026-07-17 00:25:09 +02:00

timothy referenced this issue

2026-07-17 00:25:34 +02:00

from-lineup advanced overrides cannot express "clear to none" (null = inherit template) #135

timothy added the in-progress label 2026-07-18 12:29:05 +02:00

timothy commented

2026-07-18 12:29:16 +02:00

Claiming (Claude Code / Opus 4.8 orchestrator session). Selected off the priority: medium backlog tier — arc/gate/milestone/review tiers are empty; #308 is the lowest-numbered eligible medium, deps clear, unclaimed.

Plan

Add a provider-agnostic unique-violation detector (SQLite error 19 / MySQL 1062) — likely an IsUniqueConstraintViolation(DbUpdateException) helper on the Sqlite/MySql infra split, mirroring how the providers already differ.
In the 11 Add*ToCollection handlers, catch the unique-violation on insert and translate to an idempotent no-op success (option 1 from the issue). Prefer a single shared code path over editing 11 catch blocks by hand.
Audit the Add*ToPlaylist family for the identical shape and fix uniformly if present.
Confirm Remove*/Update* are unaffected (no unique-key insert).
Tests: concurrency stress loop (N threads × many rounds, two barriers/round) + a negative control proving the test catches the un-guarded code (per the #231 lesson); enumerate lazy LanguageExt returns.
Live-E2E the write path (mandatory for write-path handler changes).
Independent cold-context review (mandatory — write-path + concurrency).

Done-when (adding to the issue body for the merge-consent gate)

Unique-violation detector implemented for both providers (SQLite 19 / MySQL 1062) with a test
All 11 Add*ToCollection handlers made idempotent under concurrent same-item add; Add*ToPlaylist audited + fixed if affected
Concurrency stress test green with a proven non-vacuous negative control
Live-E2E of an affected add-to-collection write path passes
docs/api-conventions.md §7a updated to describe the idempotent-insert contract
Adversarial review passed (Review-verdict: MERGEABLE @ <head-sha>)
CI green

Claiming (Claude Code / Opus 4.8 orchestrator session). Selected off the `priority: medium` backlog tier — arc/gate/milestone/review tiers are empty; #308 is the lowest-numbered eligible medium, deps clear, unclaimed. **Plan** - Add a provider-agnostic unique-violation detector (SQLite error 19 / MySQL 1062) — likely an `IsUniqueConstraintViolation(DbUpdateException)` helper on the Sqlite/MySql infra split, mirroring how the providers already differ. - In the 11 `Add*ToCollection` handlers, catch the unique-violation on insert and translate to an idempotent no-op success (option 1 from the issue). Prefer a single shared code path over editing 11 catch blocks by hand. - Audit the `Add*ToPlaylist` family for the identical shape and fix uniformly if present. - Confirm `Remove*`/`Update*` are unaffected (no unique-key insert). - Tests: concurrency stress loop (N threads × many rounds, two barriers/round) + a **negative control** proving the test catches the un-guarded code (per the #231 lesson); enumerate lazy LanguageExt returns. - Live-E2E the write path (mandatory for write-path handler changes). - Independent cold-context review (mandatory — write-path + concurrency). **Done-when** (adding to the issue body for the merge-consent gate) - [ ] Unique-violation detector implemented for both providers (SQLite 19 / MySQL 1062) with a test - [ ] All 11 `Add*ToCollection` handlers made idempotent under concurrent same-item add; `Add*ToPlaylist` audited + fixed if affected - [ ] Concurrency stress test green **with** a proven non-vacuous negative control - [ ] Live-E2E of an affected add-to-collection write path passes - [ ] `docs/api-conventions.md` §7a updated to describe the idempotent-insert contract - [ ] Adversarial review passed (`Review-verdict: MERGEABLE @ <head-sha>`) - [ ] CI green

timothy referenced this issue from a commit

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

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

timothy referenced a pull request that will close this issue

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

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

timothy closed this issue

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

timothy commented

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

Closed — fixed in PR #441 (merged to `main`, `2281f2e7`)

Root cause. The #269 membership pre-check (collection.MediaItems.Any(mi => mi.Id == …)) is not atomic with the insert. Two concurrent adds of the same item both load the collection, both see the item absent, both pass the guard, and both stage the CollectionItem composite key (CollectionId, MediaItemId). The loser's insert throws a unique/PK-violation DbUpdateException (SQLite 19 / MySQL 1062) that SaveChangesForcingVersion doesn't catch (it only handles DbUpdateConcurrencyException, a rowcount-mismatch with no DB error) → an unhandled 500.

Fix (option 1 + option 2, each where it fits).

ConcurrencyExtensions.TrySaveChangesForcingVersion — a bool-returning sibling of SaveChangesForcingVersion that catches only a classified unique/PK violation and returns false.
The 10 single-item Add*ToCollection handlers return Unit.Default on false (idempotent no-op, skip the reindex/rebuild fan-out — the racing winner already did it).
The bulk AddItemsToCollection handler retries on a fresh context against recomputed membership (bounded loop) so a partial-overlap collision never drops the non-colliding items.
Provider detection follows the existing TvContext static-provider seam: a settable TvContext.IsUniqueConstraintViolation delegate wired from Startup.cs to SqliteErrorClassifier (extended codes 1555/2067) / MySqlErrorClassifier (1062), defaulting to a conservative "no".

Scope. Add*ToPlaylist audited and left 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's no constraint to violate.

Files changed. ConcurrencyExtensions.cs, the 11 Add*ToCollection handlers, TvContext.cs, Sqlite/MySqlErrorClassifier.cs (new), Startup.cs; tests AddToCollectionIdempotencyConcurrencyTests.cs + SharedCacheTvContext.cs (new); docs api-conventions.md §7a + decisions/optimistic-concurrency.md.

Verification. Full suite green (~3,800 tests). Every fix-dependent test was verified to fail with the catch disabled (non-vacuous). Independent cold review: MERGEABLE. Live-E2E (real SQLite WAL): 40 + 30 concurrent adds → all 204, exactly-one-row invariant held, 6 genuine UNIQUE constraint failed collisions logged and all caught (pre-fix each is a 500).

No follow-ups deferred. No migration (no model change) and no OpenAPI regen (no endpoint/DTO change) were needed.

Docs updated: api-conventions.md §7a (new "Idempotent insert under concurrency" paragraph), decisions/optimistic-concurrency.md (dated entry).

## Closed — fixed in PR #441 (merged to `main`, `2281f2e7`) **Root cause.** The #269 membership pre-check (`collection.MediaItems.Any(mi => mi.Id == …)`) is not atomic with the insert. Two concurrent adds of the same item both load the collection, both see the item absent, both pass the guard, and both stage the `CollectionItem` composite key `(CollectionId, MediaItemId)`. The loser's insert throws a unique/PK-violation `DbUpdateException` (SQLite 19 / MySQL 1062) that `SaveChangesForcingVersion` doesn't catch (it only handles `DbUpdateConcurrencyException`, a rowcount-mismatch with no DB error) → an unhandled 500. **Fix (option 1 + option 2, each where it fits).** - `ConcurrencyExtensions.TrySaveChangesForcingVersion` — a `bool`-returning sibling of `SaveChangesForcingVersion` that catches *only* a classified unique/PK violation and returns `false`. - The **10 single-item** `Add*ToCollection` handlers return `Unit.Default` on `false` (idempotent no-op, skip the reindex/rebuild fan-out — the racing winner already did it). - The **bulk** `AddItemsToCollection` handler retries on a fresh context against recomputed membership (bounded loop) so a partial-overlap collision never drops the non-colliding items. - Provider detection follows the existing `TvContext` static-provider seam: a settable `TvContext.IsUniqueConstraintViolation` delegate wired from `Startup.cs` to `SqliteErrorClassifier` (extended codes 1555/2067) / `MySqlErrorClassifier` (1062), defaulting to a conservative "no". **Scope.** `Add*ToPlaylist` audited and **left 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's no constraint to violate. **Files changed.** `ConcurrencyExtensions.cs`, the 11 `Add*ToCollection` handlers, `TvContext.cs`, `Sqlite/MySqlErrorClassifier.cs` (new), `Startup.cs`; tests `AddToCollectionIdempotencyConcurrencyTests.cs` + `SharedCacheTvContext.cs` (new); docs `api-conventions.md` §7a + `decisions/optimistic-concurrency.md`. **Verification.** Full suite green (~3,800 tests). Every fix-dependent test was verified to *fail* with the catch disabled (non-vacuous). Independent cold review: MERGEABLE. Live-E2E (real SQLite WAL): 40 + 30 concurrent adds → all 204, exactly-one-row invariant held, **6 genuine `UNIQUE constraint failed` collisions logged and all caught** (pre-fix each is a 500). **No follow-ups deferred.** No migration (no model change) and no OpenAPI regen (no endpoint/DTO change) were needed. **Docs updated:** `api-conventions.md` §7a (new "Idempotent insert under concurrency" paragraph), `decisions/optimistic-concurrency.md` (dated entry).

timothy removed the in-progress label 2026-07-18 13:30:26 +02:00

timothy referenced this issue

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

ChicoryTV rewrite — queue tracker (goal, arc, session protocol) #237

timothy referenced this issue

2026-07-18 13:35:27 +02:00

feat(293): paginate GET /api/v1/search/all-items to cap DoS exposure #442

timothy referenced this issue

2026-07-18 14:18:04 +02:00

Page/cap GET /api/search/all-items (DoS hardening — deferred from #285) #293

timothy referenced this issue

2026-07-21 21:40:23 +02:00