fix(api): #269 review — force-write ErasePlayoutHistory + document boundaries

Independent adversarial review (cold fork + Codex) of the first cut converged
on one real miss and two boundaries to document.

- **ErasePlayoutHistoryHandler** (HIGH, both reviewers): modifies Playout ROOT
  scalars (Seed/Anchor/OnDemandCheckpoint) *without* bumping Version, inside an
  explicit transaction with no try/catch, so it 500s on a concurrent bump —
  reachable via POST /api/playouts/{id}/erase-items-and-history. My first sweep
  filtered on "Version-bumpers + deletes"; the true exposure surface is "any
  handler leaving a versioned root Modified/Deleted", so this slipped through.
  Now routes through SaveChangesForcingVersion (+ a non-vacuous through-handler
  test that exercises the explicit-transaction path). Re-swept with the correct
  filter: ErasePlayoutItems (AsNoTracking + ExecuteDelete children only) and
  ResetAllPlayouts (read-only + enqueue) are NOT exposed.

- **Background build/time-shift Playout-scalar writers** (BuildPlayout via
  PlayoutBuilder, PlayoutTimeShifter): token-guarded too, but intentionally left
  on plain save — they already catch (build-failure, not 500), and force-writing
  would persist output built from stale config (the concurrent config bump already
  enqueues a rebuild). Documented as a deliberate boundary, not a gap.

- **Item-add index collision** under force-write: documented as an accepted
  Phase-1 effect (non-corrupting, self-correcting; reload-recompute refinement
  is a #197 candidate).

Also corrects the docs' "every Version bumper" framing to the true filter and the
test docstring's over-broad non-vacuity claim. Full ErsatzTV.Tests green (1483).

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

ErsatzTV.Application/Scheduling/Commands/ErasePlayoutHistoryHandler.cs

@@ -31,7 +31,11 @@ public class ErasePlayoutHistoryHandler(IDbContextFactory<TvContext> dbContextFa
 
             playout.OnDemandCheckpoint = null;
 
-            await dbContext.SaveChangesAsync(cancellationToken);
+            // These are Playout ROOT-scalar writes, so once #253 made Playout.Version an
+            // IsConcurrencyToken the UPDATE is guarded by `WHERE Version=@orig`. Force-write past a
+            // concurrent config bump (alt-schedule/template edit) instead of throwing an unhandled
+            // DbUpdateConcurrencyException → 500 — an unconditional erase should win (#269).
+            await dbContext.SaveChangesForcingVersion(cancellationToken);
 
             await dbContext.PlayoutItems
                 .Where(pi => pi.PlayoutId == playout.Id)

ErsatzTV.Tests/Application/Concurrency/RootWriterForceVersionTests.cs

@@ -26,15 +26,21 @@ namespace ErsatzTV.Tests.Application.Concurrency;
 ///     or an item add/remove bumper such as <c>Add*ToPlaylist</c> / <c>Add|DeleteProgramScheduleItem</c>) that
 ///     saves via plain
 ///     <c>SaveChangesAsync</c> throws an unhandled <see cref="DbUpdateConcurrencyException" /> → 500 when a
-///     replace-all editor bumps the row in the load→save window. These writers now save via
+///     replace-all editor bumps the row in the load→save window. Note the exposure is "any handler that
+///     leaves a versioned root <c>Modified</c>/<c>Deleted</c>", NOT only <c>Version</c>-bumpers —
+///     <see cref="ErasePlayoutHistoryHandler" /> writes Playout scalars <b>without</b> bumping and is
+///     exposed too. These writers now save via
 ///     <see cref="ConcurrencyExtensions.SaveChangesForcingVersion" /> (Phase-1 force-write): the concurrent
 ///     bump is adopted and the write succeeds instead of 500-ing.
 ///
 ///     The handler tests are non-vacuous by construction — they reproduce the race THROUGH the handler by
 ///     handing it a context whose root is already tracked at the stale version (EF identity resolution keeps
-///     the tracked scalar), then bumping the DB row from a second context. Revert any handler to plain
+///     the tracked scalar), then bumping the DB row from a second context. Revert any of the handlers
+///     exercised below (a delete, a bump+update, and the non-bumping scalar-write erase) to plain
 ///     <c>SaveChangesAsync</c> and its test throws <see cref="DbUpdateConcurrencyException" /> instead of
-///     returning success — see the explicit negative control at the bottom.
+///     returning success — see the explicit negative control at the bottom. The remaining routed handlers
+///     (the other deletes and the 7 item add/remove bumpers) are structurally identical one-line swaps to
+///     this same helper.
 /// </summary>
 [TestFixture]
 public class RootWriterForceVersionTests
@@ -130,6 +136,41 @@ public class RootWriterForceVersionTests
         (await verify.ProgramSchedules.AnyAsync(p => p.Id == 1)).ShouldBeFalse();
     }
 
+    [Test]
+    public async Task ErasePlayoutHistory_Should_Force_Write_Its_Scalar_Edits_Past_A_Concurrent_Version_Bump()
+    {
+        // ErasePlayoutHistory modifies Playout ROOT scalars (Seed/Anchor/OnDemandCheckpoint) WITHOUT
+        // bumping Version, so it fell outside a "Version-bumper" sweep yet is still token-guarded and 500s
+        // on a concurrent bump (review of PR #302). It runs its save inside an explicit transaction with
+        // no try/catch, so this exercises force-write in that path too.
+        await using TvContext seed = _db.CreateContext();
+        seed.Playouts.Add(new Playout
+        {
+            Id = 1,
+            ChannelId = 1,
+            Version = 1,
+            ScheduleKind = PlayoutScheduleKind.Block,
+            Seed = 0,
+            Items = []
+        });
+        await seed.SaveChangesAsync();
+
+        TvContext handlerContext = _db.CreateContext();
+        _ = await handlerContext.Playouts.SingleAsync(p => p.Id == 1);
+
+        // A concurrent alt-schedule/template edit bumps Playout.Version to 2.
+        await BumpVersion(c => c.Playouts, 1);
+
+        var handler = new ErasePlayoutHistoryHandler(new PreTrackedFactory(handlerContext));
+
+        // Must not throw (would be a 500 on plain SaveChangesAsync); the erase force-writes.
+        await handler.Handle(new ErasePlayoutHistory(1), CancellationToken.None);
+
+        await using TvContext verify = _db.CreateContext();
+        (await verify.Playouts.AnyAsync(p => p.Id == 1)).ShouldBeTrue();
+        (await verify.Playouts.Where(p => p.Id == 1).Select(p => p.Version).SingleAsync()).ShouldBe(2);
+    }
+
     [Test]
     public async Task UpdateProgramSchedule_Should_Force_Write_Past_A_Concurrent_Version_Bump()
     {

docs/api-conventions.md

@@ -438,14 +438,26 @@ makes EF append `WHERE Version=@orig` to *every* UPDATE **and DELETE** of the ro
 `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). Covered (17 writers, found by
-grepping *every* `Version` bumper + every root delete): the **9 aggregate delete handlers** (a delete has no
-ETag to rotate, so it needs only the force-write, not a bump); the **item add/remove bumpers**
-(`AddProgramScheduleItem`/`DeleteProgramScheduleItem`, `Add{Items,Movie,Show,Season,Episode}ToPlaylist` —
-each bumps its root on an item add/remove); `UpdateProgramScheduleHandler` (bumps then saves — the
-ProgramSchedule case); and, from PR3, the Playout settings/`ScheduleFile`/on-demand-checkpoint writers 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
+it needs only the force-write, not a bump); the **item add/remove bumpers**
+(`AddProgramScheduleItem`/`DeleteProgramScheduleItem`, `Add{Items,Movie,Show,Season,Episode}ToPlaylist`);
+`UpdateProgramScheduleHandler`; `ErasePlayoutHistoryHandler` (root-scalar erase inside an explicit
+transaction); and, from PR3, the Playout settings/`ScheduleFile`/on-demand-checkpoint writers and
 `UpdateCollectionHandler`. (The If-Match replace handlers keep the 412 `SaveChangesWithConcurrencyGuard`;
-`UpdateDefaultDeco`'s bulk `ExecuteUpdate` can't throw, so it needs neither.) **Still deferred to #197
+`UpdateDefaultDeco`'s bulk `ExecuteUpdate` can't throw, so it needs neither.)
+
+**Two deliberate boundaries.** (1) The **background build/time-shift** Playout-scalar writers
+(`BuildPlayoutHandler` via `PlayoutBuilder`, `PlayoutTimeShifter`) are token-guarded too but stay on plain
+save on purpose: they already `catch` (→ build-failure, not a 500), and force-writing would persist output
+built from *stale* config — the concurrent config bump already enqueues a rebuild, so failing and rebuilding
+with fresh config is correct. (2) **Item-add force-write can leave a duplicate/gap `Index`** (accepted
+Phase-1 effect): the index is computed from the handler's stale child list, so a concurrent replace-all that
+grew the list makes the add land at a colliding index (no unique constraint). Non-corrupting, self-correcting
+on next edit, strictly better than the pre-#269 500; a reload-and-recompute-on-conflict refinement is a #197
+candidate. **Still deferred to #197
 (cross-editor ETag rotation only):** the non-bumping config siblings
 (`RemoveItemsFromCollectionHandler`, `UpdatePlayoutHandler`'s `DailyRebuildTime`, and the repository-mediated
 `Add*ToCollection` family, shared with the scanner hot path) don't bump `Version`, so editing through them

docs/decisions.md

@@ -1091,9 +1091,10 @@ UPDATE **and DELETE** of that row with `WHERE Version=@orig` — so any writer t
 If-Match contract but still saves via plain `SaveChangesAsync` throws an unhandled
 `DbUpdateConcurrencyException`→**500** if a replace-all editor bumps the row in its narrow load→save window.
 PR3 already force-wrote the exposed *UPDATE* siblings (Playout settings/`ScheduleFile`/checkpoint,
-`UpdateCollectionHandler`); a completeness sweep for #269 (grep **every** `Version` bumper + every
-root delete, not just the handlers PR3's close note named) found the gap was wider than reported —
-**17 writers** in total, all on plain `SaveChangesAsync`:
+`UpdateCollectionHandler`); a completeness sweep for #269 found the gap was wider than reported —
+**18 writers** in total, all on plain `SaveChangesAsync`. **The correct exposure filter is "any handler
+that leaves a versioned root `Modified` or `Deleted`", NOT just `Version`-bumpers + deletes** — an early
+sweep used the narrower filter and a review of PR #302 caught what it missed (`ErasePlayoutHistory` below):
 - the **nine versioned-root delete handlers** (`DeletePlayout`/`DeleteCollection`/`DeleteMultiCollection`/
   `DeleteRerunCollection`/`DeletePlaylist`/`DeleteBlock`/`DeleteTemplate`/`DeleteDecoTemplate`/
   `DeleteProgramSchedule`) — a DELETE is now token-guarded too;
@@ -1101,11 +1102,24 @@ root delete, not just the handlers PR3's close note named) found the gap was wid
   *suspected*);
 - the **seven item add/remove bumpers** that PR2 wired to bump their root but left on plain save —
   `AddProgramScheduleItem`/`DeleteProgramScheduleItem` and the five
-  `Add{Items,Movie,Show,Season,Episode}ToPlaylist` handlers.
+  `Add{Items,Movie,Show,Season,Episode}ToPlaylist` handlers;
+- **`ErasePlayoutHistoryHandler`** — modifies Playout root **scalars** (`Seed`/`Anchor`/`OnDemandCheckpoint`)
+  **without** bumping `Version`, inside an explicit transaction with no try/catch → the one the bumper-only
+  filter missed; reachable via `POST /api/playouts/{id}/erase-items-and-history`.
 
-All now save through `ConcurrencyExtensions.SaveChangesForcingVersion`. The final sweep confirms every
-`Version` bumper in the app saves via Forcing (these 8) or the If-Match Guard (the 10 replace handlers) or
-bulk `ExecuteUpdate` (`UpdateDefaultDeco`), and every versioned-root delete force-writes. Decision:
+All now save through `ConcurrencyExtensions.SaveChangesForcingVersion`.
+
+**Two deliberate boundaries (documented, not gaps):** (1) the background build/time-shift Playout-scalar
+writers (`BuildPlayoutHandler` via `PlayoutBuilder`'s `Anchor`/`Seed`; `PlayoutTimeShifter`'s
+`OnDemandCheckpoint`) are token-guarded too but **intentionally left on plain save** — they already `catch`
+(→ a build-failure `BaseError`, not a 500), and force-writing would be *wrong*: a concurrent config edit
+that bumped `Version` also enqueues a rebuild, so failing the in-flight build and letting the rebuild redo
+it with fresh config is correct (force-writing would persist output built from stale config). (2)
+Item-add force-write can leave a duplicate/gap `Index` (accepted Phase-1 effect): the handler computes the
+new index from its stale child list, so if a concurrent replace-all grew the list the item lands at a
+now-colliding index (no unique constraint on `PlaylistItem.Index`/`ProgramScheduleItem.Index`) — non-
+corrupting, self-correcting on the next edit, still strictly better than the pre-#269 500; a
+reload-and-recompute-on-conflict refinement is a candidate for #197. Decision:
 **force-write, not 412** — these endpoints take no `If-Match` (an unconditional DELETE/settings-edit should
 win over a concurrent editor), matching the Phase-1 force-write posture. A delete has no ETag to rotate, so
 it needs only the force-write, not a `Version` bump. A genuine row-deletion race (two concurrent deletes)

web/node_modules

@@ -0,0 +1 @@
+/Users/timothy/ersatztv/web/node_modules