diff --git a/ErsatzTV.Application/Scheduling/Commands/ErasePlayoutHistoryHandler.cs b/ErsatzTV.Application/Scheduling/Commands/ErasePlayoutHistoryHandler.cs index ec901e26c..a7fb77494 100644 --- a/ErsatzTV.Application/Scheduling/Commands/ErasePlayoutHistoryHandler.cs +++ b/ErsatzTV.Application/Scheduling/Commands/ErasePlayoutHistoryHandler.cs @@ -31,7 +31,11 @@ public class ErasePlayoutHistoryHandler(IDbContextFactory 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) diff --git a/ErsatzTV.Tests/Application/Concurrency/RootWriterForceVersionTests.cs b/ErsatzTV.Tests/Application/Concurrency/RootWriterForceVersionTests.cs index 6867fdb4d..d90fdfbd3 100644 --- a/ErsatzTV.Tests/Application/Concurrency/RootWriterForceVersionTests.cs +++ b/ErsatzTV.Tests/Application/Concurrency/RootWriterForceVersionTests.cs @@ -26,15 +26,21 @@ namespace ErsatzTV.Tests.Application.Concurrency; /// or an item add/remove bumper such as Add*ToPlaylist / Add|DeleteProgramScheduleItem) that /// saves via plain /// SaveChangesAsync throws an unhandled → 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 Modified/Deleted", NOT only Version-bumpers — +/// writes Playout scalars without bumping and is +/// exposed too. These writers now save via /// (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 /// SaveChangesAsync and its test throws 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. /// [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() { diff --git a/docs/api-conventions.md b/docs/api-conventions.md index 5c7062bfb..f114aaca8 100644 --- a/docs/api-conventions.md +++ b/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 diff --git a/docs/decisions.md b/docs/decisions.md index f6e49eb03..d70c7210d 100644 --- a/docs/decisions.md +++ b/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) diff --git a/web/node_modules b/web/node_modules new file mode 120000 index 000000000..0e66f862f --- /dev/null +++ b/web/node_modules @@ -0,0 +1 @@ +/Users/timothy/ersatztv/web/node_modules \ No newline at end of file