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

Independent Codex review (reconciled by Fable against a MERGEABLE fork verdict) found
SaveChangesForcingVersion silently DROPPED a pending Version++ under a concurrent
versioned-write race: on DbUpdateConcurrencyException it adopted the DB's current
Version verbatim (original = current = dbVersion), so a bumping sibling committed at
dbVersion instead of dbVersion+1. Net: an editor holding the concurrent writer's ETag
was never invalidated by the sibling's change — the exact lost-update the #253/#269
contract exists to close, lost under the very condition the helper handles.

F1 fix (shared helper, corrects all 25 bumpers incl. the pre-existing Add*ToPlaylist /
schedule-item writers): rebase the pending delta on top of the stored token —
  pendingDelta = current - original; original = dbVersion; current = dbVersion + pendingDelta
Bumpers (delta 1) advance to dbVersion+1; non-bumpers/deletes (delta 0, e.g.
ErasePlayoutHistory) still adopt the stored token unchanged, so RootWriterForceVersionTests
is unaffected. Idempotent across the bounded retry loop.

F3: the force-race tests now assert Version==3 (rebase), not just membership survival;
added the missing Playout force-race+rotate test. Negative-controlled: with the helper
fix reverted, both strengthened tests go red.

F2 (Medium, deferred → #308): two concurrent same-item Add*ToCollection can both pass the
membership check and the loser 500s on the composite-PK violation (DbUpdateException, which
the helper doesn't catch). Pre-existing and narrow (no corruption); doc claims softened to
name it. Filed #308.

Docs: api-conventions §7a + decisions.md prose corrected from "adopt the stored token" to
the rebase semantics.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

ErsatzTV.Application/ConcurrencyExtensions.cs

@@ -15,9 +15,10 @@ public static class ConcurrencyExtensions
     ///     UPDATE of that row with <c>WHERE Version=@orig</c>, so a concurrent bump from a replace-all
     ///     editor would otherwise surface as an unhandled <see cref="DbUpdateConcurrencyException" /> →
     ///     500 (issue #253 / #269). Phase-1 semantics for a missing <c>If-Match</c> is <b>force-write</b>,
-    ///     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.
     /// </summary>
     public static async Task<int> 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;
                 }
 

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]

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<TvContext>
+    {
+        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<IBackgroundServiceRequest> worker) =
+            MakeHandler(new PreTrackedFactory(handlerContext));
+
+        // Must not throw (plain SaveChangesAsync would 500 here) …
+        Either<BaseError, PlayoutNameViewModel> 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
+    }
 }

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.
 

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.