diff --git a/ErsatzTV.Application/ConcurrencyExtensions.cs b/ErsatzTV.Application/ConcurrencyExtensions.cs index cd64f2d03..e98cdabd9 100644 --- a/ErsatzTV.Application/ConcurrencyExtensions.cs +++ b/ErsatzTV.Application/ConcurrencyExtensions.cs @@ -15,9 +15,10 @@ public static class ConcurrencyExtensions /// UPDATE of that row with WHERE Version=@orig, so a concurrent bump from a replace-all /// editor would otherwise surface as an unhandled → /// 500 (issue #253 / #269). Phase-1 semantics for a missing If-Match is force-write, - /// so on a concurrency failure we adopt the stored token as both original (the retry's WHERE then - /// matches) and current (so we don't revert the concurrent bump) and retry — a client-wins merge - /// scoped to the token; our own modified scalars still win. Bounded to avoid a livelock; if the row + /// so on a concurrency failure we rebase onto the stored token: original becomes the stored value + /// (the retry's WHERE then matches) and current becomes stored + our pending delta (a bumper's ++ + /// still advances the ETag past the concurrent writer's value — #269 rotation; a non-bumper adopts + /// it unchanged) and retry; our own modified scalars still win. Bounded to avoid a livelock; if the row /// was deleted out from under us, that's a genuine conflict and rethrows. /// public static async Task SaveChangesForcingVersion( @@ -48,9 +49,16 @@ public static class ConcurrencyExtensions } PropertyEntry version = entry.Property(nameof(IVersionedAggregate.Version)); - object currentVersion = databaseValues[nameof(IVersionedAggregate.Version)]!; - version.OriginalValue = currentVersion; - version.CurrentValue = currentVersion; + int dbVersion = (int)databaseValues[nameof(IVersionedAggregate.Version)]!; + + // Rebase our pending delta on top of the stored token instead of adopting it verbatim: + // a Version-bumping sibling (pending current = original + 1) must still advance the + // ETag PAST the concurrent writer's value, or an editor holding that writer's ETag is + // never invalidated by our change (#269 rotation silently lost under race). Non-bumpers + // (delta 0, e.g. ErasePlayoutHistory) still adopt the stored token unchanged. + int pendingDelta = (int)version.CurrentValue! - (int)version.OriginalValue!; + version.OriginalValue = dbVersion; + version.CurrentValue = dbVersion + pendingDelta; resolvedAny = true; } diff --git a/ErsatzTV.Tests/Application/MediaCollections/CollectionEtagRotationTests.cs b/ErsatzTV.Tests/Application/MediaCollections/CollectionEtagRotationTests.cs index bfb6b1a8f..6d5abb8eb 100644 --- a/ErsatzTV.Tests/Application/MediaCollections/CollectionEtagRotationTests.cs +++ b/ErsatzTV.Tests/Application/MediaCollections/CollectionEtagRotationTests.cs @@ -163,6 +163,11 @@ public class CollectionEtagRotationTests result.IsRight.ShouldBeTrue(); await using TvContext verify = _db.CreateContext(); (await verify.CollectionItems.AnyAsync(ci => ci.CollectionId == 1 && ci.MediaItemId == 10)).ShouldBeTrue(); + + // The force-write must rebase our bump ON TOP of the concurrent bump (1 →(other) 2 →(ours) 3), not + // adopt the stored token verbatim — otherwise an editor holding the concurrent writer's ETag ("2") + // is never invalidated by this add and the #269 rotation is silently lost under race. + (await ReadVersion(1)).ShouldBe(3); } [Test] diff --git a/ErsatzTV.Tests/Application/Playouts/PlayoutScheduleFileEtagRotationTests.cs b/ErsatzTV.Tests/Application/Playouts/PlayoutScheduleFileEtagRotationTests.cs index d15b04339..1501e8e87 100644 --- a/ErsatzTV.Tests/Application/Playouts/PlayoutScheduleFileEtagRotationTests.cs +++ b/ErsatzTV.Tests/Application/Playouts/PlayoutScheduleFileEtagRotationTests.cs @@ -102,4 +102,43 @@ public class PlayoutScheduleFileEtagRotationTests (await ReadVersion(1)).ShouldBe(1); // no bump worker.Reader.Count.ShouldBe(0); // no spurious refresh } + + private sealed class PreTrackedFactory(TvContext context) : IDbContextFactory + { + public TvContext CreateDbContext() => context; + } + + [Test] + public async Task Schedule_File_Change_Should_Force_Write_Past_A_Concurrent_Version_Bump_And_Still_Rotate() + { + await SeedPlayout(1, version: 1, scheduleFile: "before.yml"); + + // The handler's context has loaded the playout at version 1 (tracked via identity map). + TvContext handlerContext = _db.CreateContext(); + _ = await handlerContext.Playouts.SingleAsync(p => p.Id == 1); + + // A concurrent replace-all editor bumps the same row to version 2 in the DB. + await using (TvContext other = _db.CreateContext()) + { + Playout p = await other.Playouts.SingleAsync(x => x.Id == 1); + p.Version++; + await other.SaveChangesAsync(); + } + + (UpdateSequentialPlayoutHandler handler, Channel worker) = + MakeHandler(new PreTrackedFactory(handlerContext)); + + // Must not throw (plain SaveChangesAsync would 500 here) … + Either result = await handler.Handle( + new UpdateSequentialPlayout(1, "after.yml"), + CancellationToken.None); + + result.IsRight.ShouldBeTrue(); + + // … and the force-write must rebase our bump ON TOP of the concurrent bump (1 →(other) 2 →(ours) 3): + // adopting the stored token verbatim would leave the concurrent writer's ETag ("2") valid and the + // #269 rotation silently lost under race. + (await ReadVersion(1)).ShouldBe(3); + worker.Reader.Count.ShouldBe(1); // the genuine change still refreshes the channel + } } diff --git a/docs/api-conventions.md b/docs/api-conventions.md index fe94ade3e..68f790d05 100644 --- a/docs/api-conventions.md +++ b/docs/api-conventions.md @@ -446,8 +446,10 @@ exception, so it needs no guard). makes EF append `WHERE Version=@orig` to *every* UPDATE **and DELETE** of the root — so a plain `SaveChangesAsync` writer that is *not* part of the If-Match contract throws an unhandled `DbUpdateConcurrencyException`→**500** when a replace-all editor bumps the row in its load→save window. Every -such writer now saves through `SaveChangesForcingVersion(ct)` (Phase-1 force-write: adopt the stored token and -retry; rethrow only if the row was genuinely deleted out from under it). **The exposure filter is "any +such writer now saves through `SaveChangesForcingVersion(ct)` (Phase-1 force-write: rebase onto the stored +token — original = stored, current = stored + the pending delta, so a `Version`-bumper's rotation still +advances the ETag *past* the concurrent writer's value instead of silently adopting it — and retry; rethrow +only if the row was genuinely deleted out from under it). **The exposure filter is "any handler that leaves a versioned root `Modified` or `Deleted` on plain `SaveChangesAsync`", NOT just `Version`-bumpers + deletes** — `ErasePlayoutHistoryHandler` modifies Playout scalars *without* bumping and is exposed too. Covered (18 writers): the **9 aggregate delete handlers** (a delete has no ETag to rotate, so @@ -481,8 +483,10 @@ API-layer concern and the scanner's separate membership-write path is unaffected rotate the editor ETag). **No-op idempotence (the trap):** these handlers gate their reindex/`BuildPlayout` fan-out on `SaveChanges() > 0`; an *unconditional* bump makes that gate always-true, so an idempotent re-add / same-value re-submit would fire spurious rebuilds. Each therefore short-circuits a genuine no-op **before** the -bump — the Add handlers by an explicit membership check (also fixing a latent duplicate-`CollectionItem` -insert), the scalar writers by `ChangeTracker.HasChanges()` — so a no-op neither bumps nor rebuilds. This is an +bump — the Add handlers by an explicit membership check (also fixing the latent duplicate-`CollectionItem` +insert on a *sequential* re-add; two *concurrent* adds of the same item can still both pass the check and the +loser 500s on the composite-PK violation — a narrow, pre-existing race, tracked as #308), the scalar +writers by `ChangeTracker.HasChanges()` — so a no-op neither bumps nor rebuilds. This is an invalidation-completeness refinement; the primary endpoints' own bump+guard already covered the two-tab lost-update the contract targets. diff --git a/docs/decisions.md b/docs/decisions.md index e414412ba..3799bdcf6 100644 --- a/docs/decisions.md +++ b/docs/decisions.md @@ -1436,8 +1436,11 @@ Decisions frozen (ratified with Fable before implementation, feeding the #197 co - **No-op idempotence — the trap Fable caught.** These handlers gate their reindex/`BuildPlayout` fan-out on `SaveChanges() > 0`. An *unconditional* bump makes that gate always-true, so an idempotent re-add / same-value re-submit would fire spurious rebuilds across every playout using the aggregate. Fix: short-circuit a genuine - no-op **before** the bump — the Add handlers by an explicit membership check (which also fixes a latent - duplicate-`CollectionItem` insert on a re-add), the scalar writers (`UpdateCollection`, `UpdatePlayout`, the + no-op **before** the bump — the Add handlers by an explicit membership check (which also fixes the latent + duplicate-`CollectionItem` insert on a *sequential* re-add; two *concurrent* same-item adds can still both + pass the check and the loser 500s on the composite-PK unique violation — `SaveChangesForcingVersion` catches + only `DbUpdateConcurrencyException`, not `DbUpdateException`. That race is narrow and pre-existing, deferred + to #308), the scalar writers (`UpdateCollection`, `UpdatePlayout`, the three `ScheduleFile` writers) by `ChangeTracker.HasChanges()`. A no-op neither bumps nor rebuilds nor rotates the ETag — which is itself correct (nothing changed). - **The `Add*ToCollection` family is not repository-mediated.** #269's original framing ("repository-mediated, @@ -1445,6 +1448,15 @@ Decisions frozen (ratified with Fable before implementation, feeding the #197 co the `Collection` into its own `dbContext` and writes directly. So the rotation bump is a pure API-layer concern and the scanner's separate membership-write path is untouched — a background scan does **not** rotate the editor ETag (correct: background indexing is not an editor action). +- **Force-write rebases the bump, never adopts the stored token verbatim (Codex review of this PR).** + `SaveChangesForcingVersion` originally resolved a conflict by setting current=original=stored — which + silently *discarded* a sibling's pending `Version++` when a versioned writer committed in its load→save + window (sibling loads 1, bumps to pending 2, concurrent PUT commits 2 → retry wrote 2, so the concurrent + writer's ETag "2" stayed valid and the rotation was lost under exactly the race it exists for). Fixed in + this PR (it affects all 25 bumpers routed through the helper, including the pre-existing playlist/schedule + ones): the retry now rebases — original = stored, current = stored + (pending current − pending original) — + so a bumper lands at stored+1 and a non-bumper (delta 0, e.g. `ErasePlayoutHistory`) adopts stored unchanged. + The race tests assert the post-race Version (3, not 2) and fail against the verbatim-adopt implementation. - **No new status codes.** These endpoints take no `If-Match` and force-write, so they never 412; no `[ProducesResponseType(...412...)]` and no OpenAPI regen (response types unchanged). Only §7a prose changes.