From 64473b136d43368569bcb6f39445d755d7c9108b Mon Sep 17 00:00:00 2001 From: Timothy Date: Sat, 11 Jul 2026 15:08:58 +0200 Subject: [PATCH 1/2] fix(scheduling): stop silently resetting fill-group state + invalidate deco edits (#251, #252) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #252: ReplaceProgramScheduleItems deleted and re-inserted every item on every save (even a no-op PUT-back), and PlayoutScheduleItemFillGroupIndex.ProgramScheduleItemId is OnDelete(Cascade) — so every schedule save silently wiped persisted fill-group/shuffle enumerator progression for all playouts using the schedule. Switch to a positional in-place reconcile: for a same-typed slot, copy scalars via CurrentValues.SetValues (BuildItem stays the single source of item construction, so no field is dropped) and rebuild the watermark/graphics join rows, keeping the item id — and with it the fill-group index. Subtype change / surplus falls back to delete+insert for that slot only. The request DTO carries no stable item id, so position is the only key here; true content-aware stable identity is deferred to the shared concurrency/round-trip contract in #253. #251: deco / deco-template CONTENT edits (and default-deco assignment) only take effect on a playout Reset build — deco/break/default-filler content is applied during Reset, a Continue keeps the frozen filler items, and BlockKey change-detection has no deco dimension to self-heal. The editors enqueued nothing (a commented-out TODO in ReplaceDecoTemplateItemsHandler), so filler/break content stayed stale indefinitely until a manual Reset. Enqueue BuildPlayout(Reset) for exactly the affected playouts: - ReplaceDecoTemplateItemsHandler: playouts via PlayoutTemplate.DecoTemplateId - UpdateDecoHandler: playouts via Playout.DecoId and via deco-template items - UpdateDefaultDecoHandler: the reassigned playout (adjacent same-class fix) Post-commit enqueues use CancellationToken.None (audit #22 policy). Tests: DecoInvalidationTests + ReplaceProgramScheduleItemsReconcileTests, each proven non-vacuous against a negative control (inverted the primitive, verified 0 CS errors so the --no-build run used a fresh dll). Full ErsatzTV.Tests suite green (1067). Co-Authored-By: Claude Opus 4.8 (1M context) --- .../ReplaceProgramScheduleItemsHandler.cs | 84 ++++- .../ReplaceDecoTemplateItemsHandler.cs | 30 +- .../Scheduling/Commands/UpdateDecoHandler.cs | 35 ++- .../Commands/UpdateDefaultDecoHandler.cs | 15 +- ...placeProgramScheduleItemsReconcileTests.cs | 292 ++++++++++++++++++ .../Scheduling/DecoInvalidationTests.cs | 185 +++++++++++ 6 files changed, 626 insertions(+), 15 deletions(-) create mode 100644 ErsatzTV.Tests/Application/ProgramSchedules/ReplaceProgramScheduleItemsReconcileTests.cs create mode 100644 ErsatzTV.Tests/Application/Scheduling/DecoInvalidationTests.cs diff --git a/ErsatzTV.Application/ProgramSchedules/Commands/ReplaceProgramScheduleItemsHandler.cs b/ErsatzTV.Application/ProgramSchedules/Commands/ReplaceProgramScheduleItemsHandler.cs index b24a8a038..5e52685ef 100644 --- a/ErsatzTV.Application/ProgramSchedules/Commands/ReplaceProgramScheduleItemsHandler.cs +++ b/ErsatzTV.Application/ProgramSchedules/Commands/ReplaceProgramScheduleItemsHandler.cs @@ -38,12 +38,56 @@ public class ReplaceProgramScheduleItemsHandler( ProgramSchedule programSchedule, CancellationToken cancellationToken) { - dbContext.RemoveRange(programSchedule.Items); - - // reset index starting with zero - programSchedule.Items = []; + // Positional in-place reconcile (rather than delete-and-reinsert): a schedule item owns the + // persisted fill-group/shuffle enumerator state via PlayoutScheduleItemFillGroupIndex, whose + // ProgramScheduleItemId FK is OnDelete(Cascade). Removing and re-inserting every item — as the + // original handler did on every save, including a no-op PUT-back — cascade-deleted that state for + // all playouts using the schedule (#252). Reusing the existing item row for the same-typed slot + // keeps its id, so the cascade never fires and progression survives. The request DTO carries no + // stable item id, so position is the only key available here; true content-aware stable identity + // is deferred to the shared concurrency/round-trip contract in #253. var orderedItems = request.Items.OrderBy(i => i.Index).ToList(); - for (var i = 0; i < orderedItems.Count; i++) + List existingItems = programSchedule.Items.OrderBy(i => i.Index).ToList(); + + // load the watermark/graphics join rows for the existing items so they can be rebuilt in place + await dbContext.Entry(programSchedule) + .Collection(ps => ps.Items) + .Query() + .Include(i => i.ProgramScheduleItemWatermarks) + .Include(i => i.ProgramScheduleItemGraphicsElements) + .LoadAsync(cancellationToken); + + int commonCount = Math.Min(existingItems.Count, orderedItems.Count); + for (var i = 0; i < commonCount; i++) + { + ProgramScheduleItem existing = existingItems[i]; + ProgramScheduleItem built = BuildItem(programSchedule, i, orderedItems[i]); + if (existing.GetType() == built.GetType()) + { + // same TPH subtype: copy all scalar values in place (BuildItem is the single source of + // item construction, so no field is silently dropped) and rebuild the join rows, keeping + // the item's id — and with it the fill-group index that would otherwise cascade away. + built.Id = existing.Id; + dbContext.Entry(existing).CurrentValues.SetValues(built); + RebuildChildren(existing, orderedItems[i]); + } + else + { + // EF can't change a TPH row's type in place; this slot must be replaced (its fill-group + // index resets, which is acceptable — the item fundamentally changed). + dbContext.Remove(existing); + programSchedule.Items.Add(built); + } + } + + // remove surplus existing items + for (int i = commonCount; i < existingItems.Count; i++) + { + dbContext.Remove(existingItems[i]); + } + + // add surplus incoming items + for (int i = commonCount; i < orderedItems.Count; i++) { programSchedule.Items.Add(BuildItem(programSchedule, i, orderedItems[i])); } @@ -68,6 +112,36 @@ public class ReplaceProgramScheduleItemsHandler( return persisted.Map(ProjectToViewModel).ToList(); } + // Rebuild the watermark/graphics join rows for an item updated in place. Clearing severs the required + // relationship so EF deletes the orphaned join rows (same net effect the delete-and-reinsert path had), + // then re-add from the request. Only the parent item row is preserved — the join rows carry no state. + private static void RebuildChildren(ProgramScheduleItem item, ReplaceProgramScheduleItem request) + { + item.ProgramScheduleItemWatermarks ??= []; + item.ProgramScheduleItemWatermarks.Clear(); + foreach (int watermarkId in request.WatermarkIds) + { + item.ProgramScheduleItemWatermarks.Add( + new ProgramScheduleItemWatermark + { + ProgramScheduleItem = item, + WatermarkId = watermarkId + }); + } + + item.ProgramScheduleItemGraphicsElements ??= []; + item.ProgramScheduleItemGraphicsElements.Clear(); + foreach (int graphicsElementId in request.GraphicsElementIds) + { + item.ProgramScheduleItemGraphicsElements.Add( + new ProgramScheduleItemGraphicsElement + { + ProgramScheduleItem = item, + GraphicsElementId = graphicsElementId + }); + } + } + private static async Task> Validate( TvContext dbContext, ReplaceProgramScheduleItems request, diff --git a/ErsatzTV.Application/Scheduling/Commands/ReplaceDecoTemplateItemsHandler.cs b/ErsatzTV.Application/Scheduling/Commands/ReplaceDecoTemplateItemsHandler.cs index 42035aaa2..1188be323 100644 --- a/ErsatzTV.Application/Scheduling/Commands/ReplaceDecoTemplateItemsHandler.cs +++ b/ErsatzTV.Application/Scheduling/Commands/ReplaceDecoTemplateItemsHandler.cs @@ -1,12 +1,17 @@ +using System.Threading.Channels; +using ErsatzTV.Application.Playouts; using ErsatzTV.Core; using ErsatzTV.Core.Domain.Scheduling; +using ErsatzTV.Core.Scheduling; using ErsatzTV.Infrastructure.Data; using ErsatzTV.Infrastructure.Extensions; using Microsoft.EntityFrameworkCore; namespace ErsatzTV.Application.Scheduling; -public class ReplaceDecoTemplateItemsHandler(IDbContextFactory dbContextFactory) +public class ReplaceDecoTemplateItemsHandler( + IDbContextFactory dbContextFactory, + ChannelWriter channel) : IRequestHandler>> { public async Task>> Handle( @@ -18,7 +23,7 @@ public class ReplaceDecoTemplateItemsHandler(IDbContextFactory dbCont return await validation.Apply(ps => Persist(dbContext, request, ps, cancellationToken)); } - private static async Task> Persist( + private async Task> Persist( TvContext dbContext, ReplaceDecoTemplateItems request, DecoTemplate decoTemplate, @@ -33,11 +38,22 @@ public class ReplaceDecoTemplateItemsHandler(IDbContextFactory dbCont await dbContext.SaveChangesAsync(cancellationToken); - // TODO: refresh any playouts that use this schedule - // foreach (Playout playout in programSchedule.Playouts) - // { - // await _channel.WriteAsync(new BuildPlayout(playout.Id, PlayoutBuildMode.Refresh)); - // } + // Deco/break/default-filler content is only (re)applied during a Reset build (a Continue keeps the + // frozen DecoDefault filler items), and BlockKey change-detection has no deco dimension, so nothing + // self-heals a deco-template edit — the editor returned 200 but built filler stayed stale until a + // manual Reset (#251). Enqueue a Reset for every playout that references this deco template. + // Post-commit enqueue uses CancellationToken.None so a late cancellation can't drop the rebuild after + // the data has already changed (audit #22 policy). + List playoutIds = await dbContext.PlayoutTemplates + .Where(pt => pt.DecoTemplateId == decoTemplate.Id) + .Select(pt => pt.PlayoutId) + .Distinct() + .ToListAsync(cancellationToken); + + foreach (int playoutId in playoutIds) + { + await channel.WriteAsync(new BuildPlayout(playoutId, PlayoutBuildMode.Reset), CancellationToken.None); + } await dbContext.Entry(decoTemplate) .Collection(t => t.Items) diff --git a/ErsatzTV.Application/Scheduling/Commands/UpdateDecoHandler.cs b/ErsatzTV.Application/Scheduling/Commands/UpdateDecoHandler.cs index 2b8537929..584d5420a 100644 --- a/ErsatzTV.Application/Scheduling/Commands/UpdateDecoHandler.cs +++ b/ErsatzTV.Application/Scheduling/Commands/UpdateDecoHandler.cs @@ -1,13 +1,18 @@ +using System.Threading.Channels; +using ErsatzTV.Application.Playouts; using ErsatzTV.Core; using ErsatzTV.Core.Domain; using ErsatzTV.Core.Domain.Scheduling; +using ErsatzTV.Core.Scheduling; using ErsatzTV.Infrastructure.Data; using ErsatzTV.Infrastructure.Extensions; using Microsoft.EntityFrameworkCore; namespace ErsatzTV.Application.Scheduling; -public class UpdateDecoHandler(IDbContextFactory dbContextFactory) +public class UpdateDecoHandler( + IDbContextFactory dbContextFactory, + ChannelWriter channel) : IRequestHandler> { public async Task> Handle(UpdateDeco request, CancellationToken cancellationToken) @@ -17,7 +22,7 @@ public class UpdateDecoHandler(IDbContextFactory dbContextFactory) return await validation.Apply(ps => ApplyUpdateRequest(dbContext, ps, request, cancellationToken)); } - private static async Task ApplyUpdateRequest( + private async Task ApplyUpdateRequest( TvContext dbContext, Deco existing, UpdateDeco request, @@ -177,6 +182,32 @@ public class UpdateDecoHandler(IDbContextFactory dbContextFactory) await dbContext.SaveChangesAsync(cancellationToken); + // Deco content (break/default filler/etc.) is only (re)applied during a Reset build, and BlockKey + // change-detection has no deco dimension, so editing a deco left already-built filler/break content + // stale until a manual Reset (#251). Enqueue a Reset for every playout that references this deco — + // directly (Playout.DecoId) or through a deco template that includes it. Post-commit enqueue uses + // CancellationToken.None so a late cancellation can't drop the rebuild (audit #22 policy). + List directPlayoutIds = await dbContext.Playouts + .Where(p => p.DecoId == request.DecoId) + .Select(p => p.Id) + .ToListAsync(cancellationToken); + + List decoTemplateIds = await dbContext.DecoTemplateItems + .Where(i => i.DecoId == request.DecoId) + .Select(i => i.DecoTemplateId) + .Distinct() + .ToListAsync(cancellationToken); + + List templatePlayoutIds = await dbContext.PlayoutTemplates + .Where(pt => pt.DecoTemplateId.HasValue && decoTemplateIds.Contains(pt.DecoTemplateId.Value)) + .Select(pt => pt.PlayoutId) + .ToListAsync(cancellationToken); + + foreach (int playoutId in directPlayoutIds.Concat(templatePlayoutIds).Distinct()) + { + await channel.WriteAsync(new BuildPlayout(playoutId, PlayoutBuildMode.Reset), CancellationToken.None); + } + return Unit.Default; } diff --git a/ErsatzTV.Application/Scheduling/Commands/UpdateDefaultDecoHandler.cs b/ErsatzTV.Application/Scheduling/Commands/UpdateDefaultDecoHandler.cs index 9ef7ccafc..c57e30b14 100644 --- a/ErsatzTV.Application/Scheduling/Commands/UpdateDefaultDecoHandler.cs +++ b/ErsatzTV.Application/Scheduling/Commands/UpdateDefaultDecoHandler.cs @@ -1,10 +1,15 @@ +using System.Threading.Channels; +using ErsatzTV.Application.Playouts; using ErsatzTV.Core; +using ErsatzTV.Core.Scheduling; using ErsatzTV.Infrastructure.Data; using Microsoft.EntityFrameworkCore; namespace ErsatzTV.Application.Scheduling; -public class UpdateDefaultDecoHandler(IDbContextFactory dbContextFactory) +public class UpdateDefaultDecoHandler( + IDbContextFactory dbContextFactory, + ChannelWriter channel) : IRequestHandler> { public async Task> Handle(UpdateDefaultDeco request, CancellationToken cancellationToken) @@ -17,6 +22,14 @@ public class UpdateDefaultDecoHandler(IDbContextFactory dbContextFact .Where(p => p.Id == request.PlayoutId) .ExecuteUpdateAsync(u => u.SetProperty(p => p.DecoId, p => request.DecoId), cancellationToken); + // Changing a playout's default deco only takes effect on a Reset build (deco content is applied + // during Reset, and BlockKey has no deco dimension to self-heal) — same stale-until-Reset class + // as #251. Enqueue a Reset for the affected playout. CancellationToken.None so the rebuild isn't + // dropped after the assignment has already been persisted. + await channel.WriteAsync( + new BuildPlayout(request.PlayoutId, PlayoutBuildMode.Reset), + CancellationToken.None); + return Option.None; } catch (Exception ex) diff --git a/ErsatzTV.Tests/Application/ProgramSchedules/ReplaceProgramScheduleItemsReconcileTests.cs b/ErsatzTV.Tests/Application/ProgramSchedules/ReplaceProgramScheduleItemsReconcileTests.cs new file mode 100644 index 000000000..d73eb0698 --- /dev/null +++ b/ErsatzTV.Tests/Application/ProgramSchedules/ReplaceProgramScheduleItemsReconcileTests.cs @@ -0,0 +1,292 @@ +using System.Threading.Channels; +using ErsatzTV.Application; +using ErsatzTV.Application.ProgramSchedules; +using ErsatzTV.Core; +using ErsatzTV.Core.Domain; +using ErsatzTV.Core.Scheduling; +using ErsatzTV.Infrastructure.Data; +using ErsatzTV.Tests.Support; +using LanguageExt; +using Microsoft.EntityFrameworkCore; +using NUnit.Framework; +using Shouldly; + +namespace ErsatzTV.Tests.Application.ProgramSchedules; + +/// +/// Regression tests for #252: the ReplaceProgramScheduleItems write path used to delete every existing +/// item and re-insert with fresh ids, which cascade-deleted +/// (FK OnDelete(Cascade)) — silently resetting persisted fill-group/shuffle progression on every save, +/// even a no-op PUT-back. The handler now reconciles in place, keeping the item id (and its fill-group +/// index) for a same-typed slot. The primary assertion is on the mechanism — item ids stay stable across +/// a no-op save — because the in-memory harness runs with foreign_keys=OFF, so an assertion on the cascade +/// alone would be FK-pragma dependent. +/// +[TestFixture] +public class ReplaceProgramScheduleItemsReconcileTests +{ + private InMemoryTvContext _db = null!; + private ChannelWriter _worker = null!; + + [SetUp] + public async Task SetUp() + { + _db = await InMemoryTvContext.CreateAsync(); + _worker = System.Threading.Channels.Channel.CreateUnbounded().Writer; + } + + [TearDown] + public async Task TearDown() => await _db.DisposeAsync(); + + [Test] + public async Task NoOp_Replace_Should_Preserve_Item_Ids_And_FillGroup_State() + { + int scheduleId = await SeedSchedule(); + var handler = new ReplaceProgramScheduleItemsHandler(_db.Factory, _worker); + + var items = new List + { + MakeItem(0, PlayoutMode.Multiple, "group") with { FillWithGroupMode = FillWithGroupMode.FillWithShuffledGroups }, + MakeItem(1, PlayoutMode.One, "news") + }; + + (await handler.Handle(new ReplaceProgramScheduleItems(scheduleId, items), CancellationToken.None)) + .IsRight.ShouldBeTrue(); + + List originalIds; + int fillGroupItemId; + await using (TvContext ctx = _db.CreateContext()) + { + List persisted = await ctx.ProgramScheduleItems + .Where(i => i.ProgramScheduleId == scheduleId) + .OrderBy(i => i.Index) + .ToListAsync(); + originalIds = persisted.Select(i => i.Id).ToList(); + fillGroupItemId = persisted[0].Id; + + // persisted fill-group enumerator state pointing at item 0 (foreign_keys=OFF, so no Playout row needed) + ctx.Add(new PlayoutScheduleItemFillGroupIndex + { + PlayoutId = 1, + ProgramScheduleItemId = fillGroupItemId, + EnumeratorState = new CollectionEnumeratorState { Seed = 12345, Index = 7 } + }); + await ctx.SaveChangesAsync(); + } + + // Act: PUT the identical items back — a no-op save. + (await handler.Handle(new ReplaceProgramScheduleItems(scheduleId, items), CancellationToken.None)) + .IsRight.ShouldBeTrue(); + + await using (TvContext ctx = _db.CreateContext()) + { + List afterIds = await ctx.ProgramScheduleItems + .Where(i => i.ProgramScheduleId == scheduleId) + .OrderBy(i => i.Index) + .Select(i => i.Id) + .ToListAsync(); + afterIds.ShouldBe(originalIds); + + PlayoutScheduleItemFillGroupIndex fillGroup = await ctx.Set() + .Include(x => x.EnumeratorState) + .SingleAsync(); + fillGroup.ProgramScheduleItemId.ShouldBe(fillGroupItemId); + fillGroup.EnumeratorState.Seed.ShouldBe(12345); + fillGroup.EnumeratorState.Index.ShouldBe(7); + } + } + + [Test] + public async Task Reorder_Should_Preserve_Untouched_Item_Id_And_FillGroup_State() + { + int scheduleId = await SeedSchedule(); + var handler = new ReplaceProgramScheduleItemsHandler(_db.Factory, _worker); + + var items = new List + { + MakeItem(0, PlayoutMode.One, "a"), + MakeItem(1, PlayoutMode.One, "b"), + MakeItem(2, PlayoutMode.Multiple, "c") with { FillWithGroupMode = FillWithGroupMode.FillWithOrderedGroups } + }; + (await handler.Handle(new ReplaceProgramScheduleItems(scheduleId, items), CancellationToken.None)) + .IsRight.ShouldBeTrue(); + + List originalIds; + int untouchedItemId; + await using (TvContext ctx = _db.CreateContext()) + { + List persisted = await ctx.ProgramScheduleItems + .Where(i => i.ProgramScheduleId == scheduleId) + .OrderBy(i => i.Index) + .ToListAsync(); + originalIds = persisted.Select(i => i.Id).OrderBy(id => id).ToList(); + untouchedItemId = persisted[2].Id; // the item at index 2 ("c") is not reordered below + + ctx.Add(new PlayoutScheduleItemFillGroupIndex + { + PlayoutId = 1, + ProgramScheduleItemId = untouchedItemId, + EnumeratorState = new CollectionEnumeratorState { Seed = 99, Index = 4 } + }); + await ctx.SaveChangesAsync(); + } + + // Act: swap the first two items; the third ("c") is unchanged and stays at index 2. + var reordered = new List + { + items[1] with { Index = 0 }, + items[0] with { Index = 1 }, + items[2] + }; + (await handler.Handle(new ReplaceProgramScheduleItems(scheduleId, reordered), CancellationToken.None)) + .IsRight.ShouldBeTrue(); + + await using (TvContext ctx = _db.CreateContext()) + { + // no item row was cascade-deleted: the full id set is intact + List afterIds = await ctx.ProgramScheduleItems + .Where(i => i.ProgramScheduleId == scheduleId) + .Select(i => i.Id) + .OrderBy(id => id) + .ToListAsync(); + afterIds.ShouldBe(originalIds); + + // the untouched item kept its id and its fill-group state + PlayoutScheduleItemFillGroupIndex fillGroup = await ctx.Set() + .Include(x => x.EnumeratorState) + .SingleAsync(); + fillGroup.ProgramScheduleItemId.ShouldBe(untouchedItemId); + fillGroup.EnumeratorState.Index.ShouldBe(4); + } + } + + [Test] + public async Task InPlace_Update_Should_Still_Apply_Watermark_Edits() + { + int scheduleId = await SeedSchedule(); + await using (TvContext ctx = _db.CreateContext()) + { + // the response projection hydrates watermark navs, so the referenced rows must exist + ctx.Add(new ChannelWatermark { Id = 1, Name = "WM One" }); + ctx.Add(new ChannelWatermark { Id = 2, Name = "WM Two" }); + await ctx.SaveChangesAsync(); + } + + var handler = new ReplaceProgramScheduleItemsHandler(_db.Factory, _worker); + + var initial = new List { MakeItem(0, PlayoutMode.One, "a") with { WatermarkIds = [1] } }; + (await handler.Handle(new ReplaceProgramScheduleItems(scheduleId, initial), CancellationToken.None)) + .IsRight.ShouldBeTrue(); + + int itemId; + await using (TvContext ctx = _db.CreateContext()) + { + itemId = await ctx.ProgramScheduleItems.Where(i => i.ProgramScheduleId == scheduleId).Select(i => i.Id).SingleAsync(); + } + + // change only the watermark set; same slot, same subtype -> in-place update + var edited = new List { MakeItem(0, PlayoutMode.One, "a") with { WatermarkIds = [2] } }; + (await handler.Handle(new ReplaceProgramScheduleItems(scheduleId, edited), CancellationToken.None)) + .IsRight.ShouldBeTrue(); + + await using (TvContext ctx = _db.CreateContext()) + { + ProgramScheduleItem persisted = await ctx.ProgramScheduleItems + .Include(i => i.ProgramScheduleItemWatermarks) + .Where(i => i.ProgramScheduleId == scheduleId) + .SingleAsync(); + persisted.Id.ShouldBe(itemId); // in-place: id preserved + persisted.ProgramScheduleItemWatermarks.Select(w => w.WatermarkId).ShouldBe([2]); // edit applied, old removed + } + } + + [Test] + public async Task Subtype_Change_Should_Replace_The_Item() + { + int scheduleId = await SeedSchedule(); + var handler = new ReplaceProgramScheduleItemsHandler(_db.Factory, _worker); + + var initial = new List { MakeItem(0, PlayoutMode.One, "a") }; + (await handler.Handle(new ReplaceProgramScheduleItems(scheduleId, initial), CancellationToken.None)) + .IsRight.ShouldBeTrue(); + + int originalId; + await using (TvContext ctx = _db.CreateContext()) + { + originalId = await ctx.ProgramScheduleItems.Where(i => i.ProgramScheduleId == scheduleId).Select(i => i.Id).SingleAsync(); + } + + // EF cannot change a TPH row's type in place, so a mode change replaces the item (new id). + var changed = new List + { + MakeItem(0, PlayoutMode.Duration, "a") with { PlayoutDuration = TimeSpan.FromMinutes(30), DiscardToFillAttempts = 0 } + }; + (await handler.Handle(new ReplaceProgramScheduleItems(scheduleId, changed), CancellationToken.None)) + .IsRight.ShouldBeTrue(); + + await using (TvContext ctx = _db.CreateContext()) + { + ProgramScheduleItem persisted = await ctx.ProgramScheduleItems + .Where(i => i.ProgramScheduleId == scheduleId) + .SingleAsync(); + persisted.ShouldBeOfType(); + persisted.Id.ShouldNotBe(originalId); + } + } + + private async Task SeedSchedule() + { + await using TvContext ctx = _db.CreateContext(); + var schedule = new ProgramSchedule + { + Name = "Reconcile", + Items = [], + Playouts = [], + ProgramScheduleAlternates = [] + }; + ctx.ProgramSchedules.Add(schedule); + await ctx.SaveChangesAsync(); + return schedule.Id; + } + + private static ReplaceProgramScheduleItem MakeItem(int index, PlayoutMode mode, string searchQuery) => + new( + index, + StartType.Dynamic, + StartTime: null, + FixedStartTimeBehavior: null, + mode, + CollectionType.SearchQuery, + CollectionId: null, + MultiCollectionId: null, + SmartCollectionId: null, + RerunCollectionId: null, + MediaItemId: null, + PlaylistId: null, + SearchTitle: searchQuery, + SearchQuery: searchQuery, + PlaybackOrder.Shuffle, + MarathonGroupBy.None, + MarathonShuffleGroups: false, + MarathonShuffleItems: false, + MarathonBatchSize: null, + FillWithGroupMode.None, + MultipleMode.Count, + MultipleCount: "1", + PlayoutDuration: null, + TailMode.None, + DiscardToFillAttempts: null, + CustomTitle: null, + GuideMode.Normal, + PreRollFillerId: null, + MidRollFillerId: null, + PostRollFillerId: null, + TailFillerId: null, + FallbackFillerId: null, + WatermarkIds: [], + GraphicsElementIds: [], + PreferredAudioLanguageCode: null, + PreferredAudioTitle: null, + PreferredSubtitleLanguageCode: null, + SubtitleMode: null); +} diff --git a/ErsatzTV.Tests/Application/Scheduling/DecoInvalidationTests.cs b/ErsatzTV.Tests/Application/Scheduling/DecoInvalidationTests.cs new file mode 100644 index 000000000..36823540c --- /dev/null +++ b/ErsatzTV.Tests/Application/Scheduling/DecoInvalidationTests.cs @@ -0,0 +1,185 @@ +using System.Threading.Channels; +using ErsatzTV.Application; +using ErsatzTV.Application.Playouts; +using ErsatzTV.Application.Scheduling; +using ErsatzTV.Core; +using ErsatzTV.Core.Domain; +using ErsatzTV.Core.Domain.Scheduling; +using ErsatzTV.Core.Scheduling; +using ErsatzTV.Infrastructure.Data; +using ErsatzTV.Tests.Support; +using LanguageExt; +using NUnit.Framework; +using Shouldly; + +namespace ErsatzTV.Tests.Application.Scheduling; + +/// +/// Regression tests for #251: deco / deco-template CONTENT edits (and default-deco assignment) only take +/// effect on a playout Reset build — deco/break/default-filler content is applied during Reset, a Continue +/// keeps the frozen filler items, and BlockKey change-detection has no deco dimension to self-heal. The +/// editors previously enqueued nothing, so filler/break content stayed stale indefinitely. These tests +/// assert the handlers now enqueue a BuildPlayout(Reset) for exactly the affected playouts. +/// +[TestFixture] +public class DecoInvalidationTests +{ + private InMemoryTvContext _db = null!; + private Channel _channel = null!; + + [SetUp] + public async Task SetUp() + { + _db = await InMemoryTvContext.CreateAsync(); + _channel = System.Threading.Channels.Channel.CreateUnbounded(); + } + + [TearDown] + public async Task TearDown() => await _db.DisposeAsync(); + + private List DrainResetEnqueues() + { + var result = new List(); + while (_channel.Reader.TryRead(out IBackgroundServiceRequest? req)) + { + if (req is BuildPlayout bp) + { + result.Add(bp); + } + } + + return result; + } + + [Test] + public async Task ReplaceDecoTemplateItems_Should_Reset_Playouts_Using_That_Deco_Template() + { + await using (TvContext ctx = _db.CreateContext()) + { + ctx.Add(new Deco { Id = 1, DecoGroupId = 1, Name = "D" }); + ctx.Add(new DecoTemplate { Id = 1, DecoTemplateGroupId = 1, Name = "T", Items = [] }); + ctx.Add(new Playout { Id = 100, ChannelId = 1, Items = [] }); + ctx.Add(new Playout { Id = 200, ChannelId = 1, Items = [] }); + ctx.Add(MakePlayoutTemplate(playoutId: 100, decoTemplateId: 1)); + ctx.Add(MakePlayoutTemplate(playoutId: 200, decoTemplateId: null)); // control: no deco template + await ctx.SaveChangesAsync(); + } + + var handler = new ReplaceDecoTemplateItemsHandler(_db.Factory, _channel.Writer); + Either> result = await handler.Handle( + new ReplaceDecoTemplateItems( + DecoTemplateId: 1, + DecoTemplateGroupId: 1, + Name: "T", + Items: [new ReplaceDecoTemplateItem(DecoId: 1, StartTime: TimeSpan.Zero, EndTime: TimeSpan.FromHours(1))]), + CancellationToken.None); + + result.IsRight.ShouldBeTrue(result.LeftToSeq().HeadOrNone().Match(e => e.Value, () => "unknown")); + + List reset = DrainResetEnqueues(); + reset.Select(bp => bp.PlayoutId).ShouldBe([100]); + reset.ShouldAllBe(bp => bp.Mode == PlayoutBuildMode.Reset); + } + + [Test] + public async Task UpdateDeco_Should_Reset_Playouts_Referencing_The_Deco_Directly_And_By_Template() + { + await using (TvContext ctx = _db.CreateContext()) + { + ctx.Add(new Deco + { + Id = 1, + DecoGroupId = 1, + Name = "D", + BreakContent = [], + DecoWatermarks = [], + DecoGraphicsElements = [] + }); + + // direct reference + ctx.Add(new Playout { Id = 100, ChannelId = 1, DecoId = 1, Items = [] }); + + // reference via a deco template that includes this deco + ctx.Add(new DecoTemplate + { + Id = 1, + DecoTemplateGroupId = 1, + Name = "T", + Items = [new DecoTemplateItem { DecoTemplateId = 1, DecoId = 1, StartTime = TimeSpan.Zero, EndTime = TimeSpan.FromHours(1) }] + }); + ctx.Add(new Playout { Id = 200, ChannelId = 1, Items = [] }); + ctx.Add(MakePlayoutTemplate(playoutId: 200, decoTemplateId: 1)); + + // control: unrelated playout + ctx.Add(new Playout { Id = 300, ChannelId = 1, Items = [] }); + await ctx.SaveChangesAsync(); + } + + var handler = new UpdateDecoHandler(_db.Factory, _channel.Writer); + Either result = await handler.Handle(MakeUpdateDeco(decoId: 1), CancellationToken.None); + + result.IsRight.ShouldBeTrue(result.LeftToSeq().HeadOrNone().Match(e => e.Value, () => "unknown")); + + List reset = DrainResetEnqueues(); + reset.Select(bp => bp.PlayoutId).OrderBy(id => id).ShouldBe([100, 200]); + reset.ShouldAllBe(bp => bp.Mode == PlayoutBuildMode.Reset); + } + + [Test] + public async Task UpdateDefaultDeco_Should_Reset_The_Affected_Playout() + { + await using (TvContext ctx = _db.CreateContext()) + { + ctx.Add(new Playout { Id = 100, ChannelId = 1, Items = [] }); + await ctx.SaveChangesAsync(); + } + + var handler = new UpdateDefaultDecoHandler(_db.Factory, _channel.Writer); + Option result = await handler.Handle(new UpdateDefaultDeco(PlayoutId: 100, DecoId: 1), CancellationToken.None); + + result.IsNone.ShouldBeTrue(); + + List reset = DrainResetEnqueues(); + reset.Select(bp => bp.PlayoutId).ShouldBe([100]); + reset.ShouldAllBe(bp => bp.Mode == PlayoutBuildMode.Reset); + } + + private static PlayoutTemplate MakePlayoutTemplate(int playoutId, int? decoTemplateId) => + new() + { + PlayoutId = playoutId, + TemplateId = 1, + DecoTemplateId = decoTemplateId, + Index = 0, + DaysOfWeek = [], + DaysOfMonth = [], + MonthsOfYear = [] + }; + + private static UpdateDeco MakeUpdateDeco(int decoId) => + new( + decoId, + DecoGroupId: 1, + Name: "D", + WatermarkMode: DecoMode.Inherit, + WatermarkIds: [], + UseWatermarkDuringFiller: false, + GraphicsElementsMode: DecoMode.Inherit, + GraphicsElementIds: [], + UseGraphicsElementsDuringFiller: false, + BreakContentMode: DecoMode.Inherit, + BreakContent: [], + DefaultFillerMode: DecoMode.Inherit, + DefaultFillerCollectionType: CollectionType.Collection, + DefaultFillerCollectionId: null, + DefaultFillerMediaItemId: null, + DefaultFillerMultiCollectionId: null, + DefaultFillerSmartCollectionId: null, + DefaultFillerTrimToFit: false, + DeadAirFallbackMode: DecoMode.Inherit, + DeadAirFallbackCollectionType: CollectionType.Collection, + DeadAirFallbackCollectionId: null, + DeadAirFallbackMediaItemId: null, + DeadAirFallbackMultiCollectionId: null, + DeadAirFallbackSmartCollectionId: null); +} -- 2.47.3 From 51ac11e5aab6a5a145b25cf38e6ff669d76d08f3 Mon Sep 17 00:00:00 2001 From: Timothy Date: Sat, 11 Jul 2026 15:18:38 +0200 Subject: [PATCH 2/2] fix(review): close post-commit cancellation window + guard default-deco enqueue (Codex Medium/Low) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Independent Codex review of the fix diff surfaced two real findings the fork pass missed: - Medium: the #251 affected-playout QUERIES in ReplaceDecoTemplateItemsHandler and UpdateDecoHandler still ran on the request `cancellationToken`, so a cancellation landing after SaveChanges committed but before those queries executed would throw before the CancellationToken.None enqueue — the edit committed but no playout Reset, re-opening the stale-content bug in that window. Run the entire post-commit invalidation (queries + enqueue) on CancellationToken.None so the side effect can't be half-aborted once the data has changed. - Low: UpdateDefaultDecoHandler enqueued a Reset for request.PlayoutId even when ExecuteUpdateAsync matched 0 rows (nonexistent playout), creating a background build request for an id that isn't there. Guard the enqueue on rows-updated > 0 so the enqueued set equals the affected set. Added a regression test. Also corrected the ReplaceProgramScheduleItemsHandler comments: the schedule-item hierarchy is TPT (table-per-type), not TPH — the SetValues reconcile is safe either way (same-runtime-type guard; no discriminator to corrupt), Codex confirmed. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../ReplaceProgramScheduleItemsHandler.cs | 4 ++-- .../Commands/ReplaceDecoTemplateItemsHandler.cs | 9 +++++---- .../Scheduling/Commands/UpdateDecoHandler.cs | 12 +++++++----- .../Commands/UpdateDefaultDecoHandler.cs | 17 +++++++++++------ .../Scheduling/DecoInvalidationTests.cs | 11 +++++++++++ 5 files changed, 36 insertions(+), 17 deletions(-) diff --git a/ErsatzTV.Application/ProgramSchedules/Commands/ReplaceProgramScheduleItemsHandler.cs b/ErsatzTV.Application/ProgramSchedules/Commands/ReplaceProgramScheduleItemsHandler.cs index 5e52685ef..0ffc63e05 100644 --- a/ErsatzTV.Application/ProgramSchedules/Commands/ReplaceProgramScheduleItemsHandler.cs +++ b/ErsatzTV.Application/ProgramSchedules/Commands/ReplaceProgramScheduleItemsHandler.cs @@ -64,7 +64,7 @@ public class ReplaceProgramScheduleItemsHandler( ProgramScheduleItem built = BuildItem(programSchedule, i, orderedItems[i]); if (existing.GetType() == built.GetType()) { - // same TPH subtype: copy all scalar values in place (BuildItem is the single source of + // same TPT subtype: copy all scalar values in place (BuildItem is the single source of // item construction, so no field is silently dropped) and rebuild the join rows, keeping // the item's id — and with it the fill-group index that would otherwise cascade away. built.Id = existing.Id; @@ -73,7 +73,7 @@ public class ReplaceProgramScheduleItemsHandler( } else { - // EF can't change a TPH row's type in place; this slot must be replaced (its fill-group + // EF can't change a TPT row's type in place; this slot must be replaced (its fill-group // index resets, which is acceptable — the item fundamentally changed). dbContext.Remove(existing); programSchedule.Items.Add(built); diff --git a/ErsatzTV.Application/Scheduling/Commands/ReplaceDecoTemplateItemsHandler.cs b/ErsatzTV.Application/Scheduling/Commands/ReplaceDecoTemplateItemsHandler.cs index 1188be323..52fccc29d 100644 --- a/ErsatzTV.Application/Scheduling/Commands/ReplaceDecoTemplateItemsHandler.cs +++ b/ErsatzTV.Application/Scheduling/Commands/ReplaceDecoTemplateItemsHandler.cs @@ -41,14 +41,15 @@ public class ReplaceDecoTemplateItemsHandler( // Deco/break/default-filler content is only (re)applied during a Reset build (a Continue keeps the // frozen DecoDefault filler items), and BlockKey change-detection has no deco dimension, so nothing // self-heals a deco-template edit — the editor returned 200 but built filler stayed stale until a - // manual Reset (#251). Enqueue a Reset for every playout that references this deco template. - // Post-commit enqueue uses CancellationToken.None so a late cancellation can't drop the rebuild after - // the data has already changed (audit #22 policy). + // manual Reset (#251). Enqueue a Reset for every playout that references this deco template. This + // whole post-commit invalidation runs with CancellationToken.None (audit #22 policy): once the edit + // is committed, a late request cancellation must not be able to abort the affected-playout query OR + // the enqueue and leave content stale. List playoutIds = await dbContext.PlayoutTemplates .Where(pt => pt.DecoTemplateId == decoTemplate.Id) .Select(pt => pt.PlayoutId) .Distinct() - .ToListAsync(cancellationToken); + .ToListAsync(CancellationToken.None); foreach (int playoutId in playoutIds) { diff --git a/ErsatzTV.Application/Scheduling/Commands/UpdateDecoHandler.cs b/ErsatzTV.Application/Scheduling/Commands/UpdateDecoHandler.cs index 584d5420a..95fe7a55c 100644 --- a/ErsatzTV.Application/Scheduling/Commands/UpdateDecoHandler.cs +++ b/ErsatzTV.Application/Scheduling/Commands/UpdateDecoHandler.cs @@ -185,23 +185,25 @@ public class UpdateDecoHandler( // Deco content (break/default filler/etc.) is only (re)applied during a Reset build, and BlockKey // change-detection has no deco dimension, so editing a deco left already-built filler/break content // stale until a manual Reset (#251). Enqueue a Reset for every playout that references this deco — - // directly (Playout.DecoId) or through a deco template that includes it. Post-commit enqueue uses - // CancellationToken.None so a late cancellation can't drop the rebuild (audit #22 policy). + // directly (Playout.DecoId) or through a deco template that includes it. This whole post-commit + // invalidation runs with CancellationToken.None (audit #22 policy): once the edit is committed, a + // late request cancellation must not be able to abort either the affected-playout queries OR the + // enqueue and leave content stale — the entire side effect has to complete. List directPlayoutIds = await dbContext.Playouts .Where(p => p.DecoId == request.DecoId) .Select(p => p.Id) - .ToListAsync(cancellationToken); + .ToListAsync(CancellationToken.None); List decoTemplateIds = await dbContext.DecoTemplateItems .Where(i => i.DecoId == request.DecoId) .Select(i => i.DecoTemplateId) .Distinct() - .ToListAsync(cancellationToken); + .ToListAsync(CancellationToken.None); List templatePlayoutIds = await dbContext.PlayoutTemplates .Where(pt => pt.DecoTemplateId.HasValue && decoTemplateIds.Contains(pt.DecoTemplateId.Value)) .Select(pt => pt.PlayoutId) - .ToListAsync(cancellationToken); + .ToListAsync(CancellationToken.None); foreach (int playoutId in directPlayoutIds.Concat(templatePlayoutIds).Distinct()) { diff --git a/ErsatzTV.Application/Scheduling/Commands/UpdateDefaultDecoHandler.cs b/ErsatzTV.Application/Scheduling/Commands/UpdateDefaultDecoHandler.cs index c57e30b14..de31cfd49 100644 --- a/ErsatzTV.Application/Scheduling/Commands/UpdateDefaultDecoHandler.cs +++ b/ErsatzTV.Application/Scheduling/Commands/UpdateDefaultDecoHandler.cs @@ -18,17 +18,22 @@ public class UpdateDefaultDecoHandler( { await using TvContext dbContext = await dbContextFactory.CreateDbContextAsync(cancellationToken); - await dbContext.Playouts + int updated = await dbContext.Playouts .Where(p => p.Id == request.PlayoutId) .ExecuteUpdateAsync(u => u.SetProperty(p => p.DecoId, p => request.DecoId), cancellationToken); // Changing a playout's default deco only takes effect on a Reset build (deco content is applied // during Reset, and BlockKey has no deco dimension to self-heal) — same stale-until-Reset class - // as #251. Enqueue a Reset for the affected playout. CancellationToken.None so the rebuild isn't - // dropped after the assignment has already been persisted. - await channel.WriteAsync( - new BuildPlayout(request.PlayoutId, PlayoutBuildMode.Reset), - CancellationToken.None); + // as #251. Enqueue a Reset for the affected playout, but only if a row was actually updated: a + // request for a nonexistent playout must not queue a build for an id that isn't there (keeps the + // enqueued set equal to the affected set). CancellationToken.None so the rebuild isn't dropped + // after the assignment has already been persisted. + if (updated > 0) + { + await channel.WriteAsync( + new BuildPlayout(request.PlayoutId, PlayoutBuildMode.Reset), + CancellationToken.None); + } return Option.None; } diff --git a/ErsatzTV.Tests/Application/Scheduling/DecoInvalidationTests.cs b/ErsatzTV.Tests/Application/Scheduling/DecoInvalidationTests.cs index 36823540c..034fbac85 100644 --- a/ErsatzTV.Tests/Application/Scheduling/DecoInvalidationTests.cs +++ b/ErsatzTV.Tests/Application/Scheduling/DecoInvalidationTests.cs @@ -144,6 +144,17 @@ public class DecoInvalidationTests reset.ShouldAllBe(bp => bp.Mode == PlayoutBuildMode.Reset); } + [Test] + public async Task UpdateDefaultDeco_Should_Not_Enqueue_For_A_Nonexistent_Playout() + { + // no playout seeded — ExecuteUpdateAsync matches 0 rows + var handler = new UpdateDefaultDecoHandler(_db.Factory, _channel.Writer); + Option result = await handler.Handle(new UpdateDefaultDeco(PlayoutId: 999, DecoId: 1), CancellationToken.None); + + result.IsNone.ShouldBeTrue(); + DrainResetEnqueues().ShouldBeEmpty(); // enqueued set must equal the affected (empty) set + } + private static PlayoutTemplate MakePlayoutTemplate(int playoutId, int? decoTemplateId) => new() { -- 2.47.3