From a1bd303cce2c6e101553aa2791a56e71a1d05cb3 Mon Sep 17 00:00:00 2001 From: Timothy Date: Sat, 11 Jul 2026 17:42:45 +0200 Subject: [PATCH 1/2] fix(app): post-commit side effects on CancellationToken.None + guide-xml/empty-list hardening (#254) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Backend mutation-hardening cluster from the 2026-07-11 mutation-safety audit sweep (adversarial-reviewer #22/#23), the parallel-safe backend-isolated slice. audit#22 F4 — standardize post-commit enqueues on CancellationToken.None: 20 command handlers under MediaCollections/, ProgramSchedules/, Playouts/, Channels/ threaded the request cancellationToken into work that runs AFTER SaveChangesAsync commits (WriteAsync rebuild/refresh enqueues, mediator.Publish, reindex, cache Refresh, and post-commit lookups that gate an enqueue). A late client-disconnect then turns an already-durable commit into a thrown request AND drops the side effect. Generalizes the #251 deco-handler fix. Excludes BuildPlayoutHandler (worker/background token, not a client-disconnect token), the config/FFmpeg multi-upsert handlers (partial-commit case, separate follow-up), and response-projection reloads (correctly keep the request token). audit#22 F2 — DeleteChannelHandler/DeletePlayoutHandler now delete the channel guide {number}.xml through IFileSystem.File.Delete (observable under MockFileSystem) and BEFORE the commit (a post-commit delete orphans the xml on a crash; the xml is regenerable on demand, so pre-commit delete is the safe order). audit#23 F4 — ReplacePlayoutAlternateScheduleItemsHandler rejects an empty item list in the handler (not only the controller pre-guard) so a direct caller can't trip the Max()-on-empty crash. Docs: api-conventions.md §7a (post-commit token convention + boundaries), decisions.md entry (rationale, sweep scope, #253 PR2-4 coordination note). Tests: guide-cache-delete-through-FS for both delete handlers, empty-list guard. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../Commands/BulkDeleteChannelsHandler.cs | 4 +- .../BulkMoveChannelsToGroupHandler.cs | 5 +- .../CreateChannelFromLineupHandler.cs | 9 ++-- .../Channels/Commands/DeleteChannelHandler.cs | 22 +++++---- .../Channels/Commands/UpdateChannelHandler.cs | 10 ++-- .../Commands/UpdateChannelNumbersHandler.cs | 6 ++- .../Commands/AddItemsToCollectionHandler.cs | 6 ++- .../RemoveItemsFromCollectionHandler.cs | 6 ++- .../Commands/UpdateCollectionHandler.cs | 4 +- .../Commands/UpdateMultiCollectionHandler.cs | 4 +- .../Commands/UpdateRerunCollectionHandler.cs | 4 +- .../Commands/UpdateSmartCollectionHandler.cs | 7 ++- .../Commands/UpdateTraktListHandler.cs | 4 +- .../Commands/CreateScriptedPlayoutHandler.cs | 9 ++-- .../CreateSequentialPlayoutHandler.cs | 9 ++-- .../Playouts/Commands/DeletePlayoutHandler.cs | 21 +++++---- ...acePlayoutAlternateScheduleItemsHandler.cs | 12 ++++- .../UpdateExternalJsonPlayoutHandler.cs | 4 +- .../Commands/UpdateScriptedPlayoutHandler.cs | 4 +- .../UpdateSequentialPlayoutHandler.cs | 4 +- .../Commands/AddProgramScheduleItemHandler.cs | 4 +- .../DeleteProgramScheduleItemHandler.cs | 4 +- .../ReplaceProgramScheduleItemsHandler.cs | 4 +- .../Channels/DeleteChannelHandlerTests.cs | 22 ++++++++- .../Playouts/PlayoutHandlerTests.cs | 47 +++++++++++++++++++ docs/api-conventions.md | 32 +++++++++++++ docs/decisions.md | 41 ++++++++++++++++ 27 files changed, 255 insertions(+), 53 deletions(-) diff --git a/ErsatzTV.Application/Channels/Commands/BulkDeleteChannelsHandler.cs b/ErsatzTV.Application/Channels/Commands/BulkDeleteChannelsHandler.cs index 58dff8705..9cd4369eb 100644 --- a/ErsatzTV.Application/Channels/Commands/BulkDeleteChannelsHandler.cs +++ b/ErsatzTV.Application/Channels/Commands/BulkDeleteChannelsHandler.cs @@ -55,7 +55,9 @@ public class BulkDeleteChannelsHandler( } } - await workerChannel.WriteAsync(new RefreshChannelList(), cancellationToken); + // post-commit side effect runs on CancellationToken.None so a late request cancellation + // can't abort it after the commit landed (#254) + await workerChannel.WriteAsync(new RefreshChannelList(), CancellationToken.None); return Right(Unit.Default); } diff --git a/ErsatzTV.Application/Channels/Commands/BulkMoveChannelsToGroupHandler.cs b/ErsatzTV.Application/Channels/Commands/BulkMoveChannelsToGroupHandler.cs index ede6c4506..1e78ae609 100644 --- a/ErsatzTV.Application/Channels/Commands/BulkMoveChannelsToGroupHandler.cs +++ b/ErsatzTV.Application/Channels/Commands/BulkMoveChannelsToGroupHandler.cs @@ -54,7 +54,10 @@ public class BulkMoveChannelsToGroupHandler( await transaction.CommitAsync(cancellationToken); searchTargets.SearchTargetsChanged(); - await workerChannel.WriteAsync(new RefreshChannelList(), cancellationToken); + + // post-commit side effect runs on CancellationToken.None so a late request cancellation + // can't abort it after the commit landed (#254) + await workerChannel.WriteAsync(new RefreshChannelList(), CancellationToken.None); return Right(Unit.Default); } diff --git a/ErsatzTV.Application/Channels/Commands/CreateChannelFromLineupHandler.cs b/ErsatzTV.Application/Channels/Commands/CreateChannelFromLineupHandler.cs index fd8773482..4127dc10f 100644 --- a/ErsatzTV.Application/Channels/Commands/CreateChannelFromLineupHandler.cs +++ b/ErsatzTV.Application/Channels/Commands/CreateChannelFromLineupHandler.cs @@ -68,19 +68,22 @@ public class CreateChannelFromLineupHandler( } searchTargets.SearchTargetsChanged(); + + // post-commit side effect runs on CancellationToken.None so a late request cancellation + // can't abort it after the commit landed (#254) await workerChannel.WriteAsync( new BuildPlayout(prepared.Playout.Id, PlayoutBuildMode.Reset), - cancellationToken); + CancellationToken.None); // Mirror CreateClassicPlayoutHandler: on-demand playouts must be time-shifted to "now" after build. if (prepared.Channel.PlayoutMode is ChannelPlayoutMode.OnDemand) { await workerChannel.WriteAsync( new TimeShiftOnDemandPlayout(prepared.Playout.Id, DateTimeOffset.Now, false), - cancellationToken); + CancellationToken.None); } - await workerChannel.WriteAsync(new RefreshChannelList(), cancellationToken); + await workerChannel.WriteAsync(new RefreshChannelList(), CancellationToken.None); return new CreateChannelFromLineupResponseModel( prepared.Channel.Id, diff --git a/ErsatzTV.Application/Channels/Commands/DeleteChannelHandler.cs b/ErsatzTV.Application/Channels/Commands/DeleteChannelHandler.cs index 0363cecab..bd673ef22 100644 --- a/ErsatzTV.Application/Channels/Commands/DeleteChannelHandler.cs +++ b/ErsatzTV.Application/Channels/Commands/DeleteChannelHandler.cs @@ -47,20 +47,24 @@ public class DeleteChannelHandler : IRequestHandler DoDeletion(TvContext dbContext, Channel channel, CancellationToken cancellationToken) { + // Delete the guide cache file through the filesystem abstraction (so it's observable under a + // MockFileSystem) and BEFORE the commit: deleting after commit orphans {number}.xml if the + // process crashes in between (nothing reaps it, and GetChannelGuideHandler serves everything + // in the cache folder). The guide xml is regenerable on demand, so losing it pre-commit is safe (#254). + string cacheFile = Path.Combine(FileSystemLayout.ChannelGuideCacheFolder, $"{channel.Number}.xml"); + if (_fileSystem.File.Exists(cacheFile)) + { + _fileSystem.File.Delete(cacheFile); + } + dbContext.Channels.Remove(channel); await dbContext.SaveChangesAsync(cancellationToken); _searchTargets.SearchTargetsChanged(); - // delete channel data from channel guide cache - string cacheFile = Path.Combine(FileSystemLayout.ChannelGuideCacheFolder, $"{channel.Number}.xml"); - if (_fileSystem.File.Exists(cacheFile)) - { - File.Delete(cacheFile); - } - - // refresh channel list to remove channel that has no playout - await _workerChannel.WriteAsync(new RefreshChannelList(), cancellationToken); + // refresh channel list to remove channel that has no playout — post-commit side effect runs on + // CancellationToken.None so a late request cancellation can't abort it after the delete committed (#254) + await _workerChannel.WriteAsync(new RefreshChannelList(), CancellationToken.None); return Unit.Default; } diff --git a/ErsatzTV.Application/Channels/Commands/UpdateChannelHandler.cs b/ErsatzTV.Application/Channels/Commands/UpdateChannelHandler.cs index bf3039a4e..39dfc1ad8 100644 --- a/ErsatzTV.Application/Channels/Commands/UpdateChannelHandler.cs +++ b/ErsatzTV.Application/Channels/Commands/UpdateChannelHandler.cs @@ -157,21 +157,23 @@ public class UpdateChannelHandler( searchTargets.SearchTargetsChanged(); + // post-commit side effect runs on CancellationToken.None so a late request cancellation + // can't abort it after the commit landed (#254) if (c.SubtitleMode != ChannelSubtitleMode.None) { Option maybePlayout = await dbContext.Playouts - .SelectOneAsync(p => p.ChannelId, p => p.ChannelId == c.Id, cancellationToken); + .SelectOneAsync(p => p.ChannelId, p => p.ChannelId == c.Id, CancellationToken.None); foreach (Playout playout in maybePlayout) { - await workerChannel.WriteAsync(new ExtractEmbeddedSubtitles(playout.Id), cancellationToken); + await workerChannel.WriteAsync(new ExtractEmbeddedSubtitles(playout.Id), CancellationToken.None); } } - await workerChannel.WriteAsync(new RefreshChannelList(), cancellationToken); + await workerChannel.WriteAsync(new RefreshChannelList(), CancellationToken.None); if (hasEpgChange) { - await workerChannel.WriteAsync(new RefreshChannelData(c.Number), cancellationToken); + await workerChannel.WriteAsync(new RefreshChannelData(c.Number), CancellationToken.None); } return ProjectToViewModel(c, c.Playouts?.Count ?? 0); diff --git a/ErsatzTV.Application/Channels/Commands/UpdateChannelNumbersHandler.cs b/ErsatzTV.Application/Channels/Commands/UpdateChannelNumbersHandler.cs index c1873c15f..3fdc321b8 100644 --- a/ErsatzTV.Application/Channels/Commands/UpdateChannelNumbersHandler.cs +++ b/ErsatzTV.Application/Channels/Commands/UpdateChannelNumbersHandler.cs @@ -81,10 +81,12 @@ public class UpdateChannelNumbersHandler( await transaction.CommitAsync(cancellationToken); // update channel list and xmltv - await workerChannel.WriteAsync(new RefreshChannelList(), cancellationToken); + // post-commit side effect runs on CancellationToken.None so a late request cancellation + // can't abort it after the commit landed (#254) + await workerChannel.WriteAsync(new RefreshChannelList(), CancellationToken.None); foreach (var channel in channelsToUpdate) { - await workerChannel.WriteAsync(new RefreshChannelData(channel.Number), cancellationToken); + await workerChannel.WriteAsync(new RefreshChannelData(channel.Number), CancellationToken.None); } return Option.None; diff --git a/ErsatzTV.Application/MediaCollections/Commands/AddItemsToCollectionHandler.cs b/ErsatzTV.Application/MediaCollections/Commands/AddItemsToCollectionHandler.cs index b21ce9f6f..475a1494d 100644 --- a/ErsatzTV.Application/MediaCollections/Commands/AddItemsToCollectionHandler.cs +++ b/ErsatzTV.Application/MediaCollections/Commands/AddItemsToCollectionHandler.cs @@ -81,13 +81,15 @@ public class AddItemsToCollectionHandler : if (await dbContext.SaveChangesAsync(cancellationToken) > 0) { - await _searchChannel.WriteAsync(new ReindexMediaItems(toAddIds.ToArray()), cancellationToken); + // post-commit side effect runs on CancellationToken.None so a late request cancellation + // can't abort it after the commit landed (#254) + await _searchChannel.WriteAsync(new ReindexMediaItems(toAddIds.ToArray()), CancellationToken.None); // refresh all playouts that use this collection foreach (int playoutId in await _mediaCollectionRepository .PlayoutIdsUsingCollection(request.CollectionId)) { - await _channel.WriteAsync(new BuildPlayout(playoutId, PlayoutBuildMode.Refresh), cancellationToken); + await _channel.WriteAsync(new BuildPlayout(playoutId, PlayoutBuildMode.Refresh), CancellationToken.None); } } diff --git a/ErsatzTV.Application/MediaCollections/Commands/RemoveItemsFromCollectionHandler.cs b/ErsatzTV.Application/MediaCollections/Commands/RemoveItemsFromCollectionHandler.cs index 58e267ae6..24af4aaae 100644 --- a/ErsatzTV.Application/MediaCollections/Commands/RemoveItemsFromCollectionHandler.cs +++ b/ErsatzTV.Application/MediaCollections/Commands/RemoveItemsFromCollectionHandler.cs @@ -63,14 +63,16 @@ public class RemoveItemsFromCollectionHandler : IRequestHandler 0) { + // post-commit side effect runs on CancellationToken.None so a late request cancellation + // can't abort it after the commit landed (#254) await _searchChannel.WriteAsync( new ReindexMediaItems(itemsToRemove.Select(mi => mi.Id).ToArray()), - cancellationToken); + CancellationToken.None); // refresh all playouts that use this collection foreach (int playoutId in await _mediaCollectionRepository.PlayoutIdsUsingCollection(collection.Id)) { - await _channel.WriteAsync(new BuildPlayout(playoutId, PlayoutBuildMode.Refresh), cancellationToken); + await _channel.WriteAsync(new BuildPlayout(playoutId, PlayoutBuildMode.Refresh), CancellationToken.None); } } diff --git a/ErsatzTV.Application/MediaCollections/Commands/UpdateCollectionHandler.cs b/ErsatzTV.Application/MediaCollections/Commands/UpdateCollectionHandler.cs index 15010dbe6..73ca500dd 100644 --- a/ErsatzTV.Application/MediaCollections/Commands/UpdateCollectionHandler.cs +++ b/ErsatzTV.Application/MediaCollections/Commands/UpdateCollectionHandler.cs @@ -61,11 +61,13 @@ public class UpdateCollectionHandler : IRequestHandler 0 && request.UseCustomPlaybackOrder.IsSome) { + // post-commit side effect runs on CancellationToken.None so a late request cancellation + // can't abort it after the commit landed (#254) // refresh all playouts that use this collection foreach (int playoutId in await _mediaCollectionRepository.PlayoutIdsUsingCollection( request.CollectionId)) { - await _channel.WriteAsync(new BuildPlayout(playoutId, PlayoutBuildMode.Refresh), cancellationToken); + await _channel.WriteAsync(new BuildPlayout(playoutId, PlayoutBuildMode.Refresh), CancellationToken.None); } } diff --git a/ErsatzTV.Application/MediaCollections/Commands/UpdateMultiCollectionHandler.cs b/ErsatzTV.Application/MediaCollections/Commands/UpdateMultiCollectionHandler.cs index d29987398..f4af1f30a 100644 --- a/ErsatzTV.Application/MediaCollections/Commands/UpdateMultiCollectionHandler.cs +++ b/ErsatzTV.Application/MediaCollections/Commands/UpdateMultiCollectionHandler.cs @@ -124,10 +124,12 @@ public class UpdateMultiCollectionHandler : IRequestHandler 0) { + // post-commit side effect runs on CancellationToken.None so a late request cancellation + // can't abort it after the commit landed (#254) // refresh all playouts that use this rerun collection foreach (int playoutId in await mediaCollectionRepository.PlayoutIdsUsingRerunCollection( request.RerunCollectionId)) { - await channel.WriteAsync(new BuildPlayout(playoutId, PlayoutBuildMode.Refresh), cancellationToken); + await channel.WriteAsync(new BuildPlayout(playoutId, PlayoutBuildMode.Refresh), CancellationToken.None); } } diff --git a/ErsatzTV.Application/MediaCollections/Commands/UpdateSmartCollectionHandler.cs b/ErsatzTV.Application/MediaCollections/Commands/UpdateSmartCollectionHandler.cs index e1cfbdc9d..df7e06246 100644 --- a/ErsatzTV.Application/MediaCollections/Commands/UpdateSmartCollectionHandler.cs +++ b/ErsatzTV.Application/MediaCollections/Commands/UpdateSmartCollectionHandler.cs @@ -69,12 +69,15 @@ public class if (await dbContext.SaveChangesAsync(cancellationToken) > 0) { _searchTargets.SearchTargetsChanged(); - await _smartCollectionCache.Refresh(cancellationToken); + + // post-commit side effects run on CancellationToken.None so a late request cancellation + // can't abort them after the commit landed (#254) + await _smartCollectionCache.Refresh(CancellationToken.None); // refresh all playouts that use this smart collection foreach (int playoutId in await _mediaCollectionRepository.PlayoutIdsUsingSmartCollection(request.Id)) { - await _channel.WriteAsync(new BuildPlayout(playoutId, PlayoutBuildMode.Refresh), cancellationToken); + await _channel.WriteAsync(new BuildPlayout(playoutId, PlayoutBuildMode.Refresh), CancellationToken.None); } } diff --git a/ErsatzTV.Application/MediaCollections/Commands/UpdateTraktListHandler.cs b/ErsatzTV.Application/MediaCollections/Commands/UpdateTraktListHandler.cs index 772d5ed62..f24e2805a 100644 --- a/ErsatzTV.Application/MediaCollections/Commands/UpdateTraktListHandler.cs +++ b/ErsatzTV.Application/MediaCollections/Commands/UpdateTraktListHandler.cs @@ -57,7 +57,9 @@ public class UpdateTraktListHandler( { try { - await workerChannel.WriteAsync(new MatchTraktListItems(traktList.Id), cancellationToken); + // post-commit side effect runs on CancellationToken.None so a late request + // cancellation can't abort it after the commit landed (#254) + await workerChannel.WriteAsync(new MatchTraktListItems(traktList.Id), CancellationToken.None); } catch { diff --git a/ErsatzTV.Application/Playouts/Commands/CreateScriptedPlayoutHandler.cs b/ErsatzTV.Application/Playouts/Commands/CreateScriptedPlayoutHandler.cs index 88dc41e20..8af0f2606 100644 --- a/ErsatzTV.Application/Playouts/Commands/CreateScriptedPlayoutHandler.cs +++ b/ErsatzTV.Application/Playouts/Commands/CreateScriptedPlayoutHandler.cs @@ -35,15 +35,18 @@ public class CreateScriptedPlayoutHandler( { await dbContext.Playouts.AddAsync(playout, cancellationToken); await dbContext.SaveChangesAsync(cancellationToken); - await channel.WriteAsync(new BuildPlayout(playout.Id, PlayoutBuildMode.Reset), cancellationToken); + + // post-commit side effect runs on CancellationToken.None so a late request cancellation + // can't abort it after the commit landed (#254) + await channel.WriteAsync(new BuildPlayout(playout.Id, PlayoutBuildMode.Reset), CancellationToken.None); if (playout.Channel.PlayoutMode is ChannelPlayoutMode.OnDemand) { await channel.WriteAsync( new TimeShiftOnDemandPlayout(playout.Id, DateTimeOffset.Now, false), - cancellationToken); + CancellationToken.None); } - await channel.WriteAsync(new RefreshChannelList(), cancellationToken); + await channel.WriteAsync(new RefreshChannelList(), CancellationToken.None); return new CreatePlayoutResponse(playout.Id); } diff --git a/ErsatzTV.Application/Playouts/Commands/CreateSequentialPlayoutHandler.cs b/ErsatzTV.Application/Playouts/Commands/CreateSequentialPlayoutHandler.cs index 267a22098..c0ce15b2e 100644 --- a/ErsatzTV.Application/Playouts/Commands/CreateSequentialPlayoutHandler.cs +++ b/ErsatzTV.Application/Playouts/Commands/CreateSequentialPlayoutHandler.cs @@ -33,15 +33,18 @@ public class CreateSequentialPlayoutHandler( { await dbContext.Playouts.AddAsync(playout, cancellationToken); await dbContext.SaveChangesAsync(cancellationToken); - await channel.WriteAsync(new BuildPlayout(playout.Id, PlayoutBuildMode.Reset), cancellationToken); + + // post-commit side effect runs on CancellationToken.None so a late request cancellation + // can't abort it after the commit landed (#254) + await channel.WriteAsync(new BuildPlayout(playout.Id, PlayoutBuildMode.Reset), CancellationToken.None); if (playout.Channel.PlayoutMode is ChannelPlayoutMode.OnDemand) { await channel.WriteAsync( new TimeShiftOnDemandPlayout(playout.Id, DateTimeOffset.Now, false), - cancellationToken); + CancellationToken.None); } - await channel.WriteAsync(new RefreshChannelList(), cancellationToken); + await channel.WriteAsync(new RefreshChannelList(), CancellationToken.None); return new CreatePlayoutResponse(playout.Id); } diff --git a/ErsatzTV.Application/Playouts/Commands/DeletePlayoutHandler.cs b/ErsatzTV.Application/Playouts/Commands/DeletePlayoutHandler.cs index c6434eaff..4831de8ce 100644 --- a/ErsatzTV.Application/Playouts/Commands/DeletePlayoutHandler.cs +++ b/ErsatzTV.Application/Playouts/Commands/DeletePlayoutHandler.cs @@ -28,20 +28,25 @@ public class DeletePlayoutHandler( foreach (Playout playout in maybePlayout) { - dbContext.Playouts.Remove(playout); - await dbContext.SaveChangesAsync(cancellationToken); - - // delete channel data from channel guide cache + // Delete the guide cache file through the filesystem abstraction (observable under a + // MockFileSystem) and BEFORE the commit: deleting after commit orphans {number}.xml if the + // process crashes in between. The guide xml is regenerable on demand, so losing it pre-commit + // is safe (#254). string cacheFile = Path.Combine(FileSystemLayout.ChannelGuideCacheFolder, $"{playout.Channel.Number}.xml"); if (fileSystem.File.Exists(cacheFile)) { - File.Delete(cacheFile); + fileSystem.File.Delete(cacheFile); } - // refresh channel list to remove channel that has no playout - await workerChannel.WriteAsync(new RefreshChannelList(), cancellationToken); + dbContext.Playouts.Remove(playout); + await dbContext.SaveChangesAsync(cancellationToken); - await mediator.Publish(new PlayoutUpdatedNotification(playout.Id, false), cancellationToken); + // post-commit side effects run on CancellationToken.None so a late request cancellation can't + // abort them after the delete committed (#254) + // refresh channel list to remove channel that has no playout + await workerChannel.WriteAsync(new RefreshChannelList(), CancellationToken.None); + + await mediator.Publish(new PlayoutUpdatedNotification(playout.Id, false), CancellationToken.None); } return maybePlayout diff --git a/ErsatzTV.Application/Playouts/Commands/ReplacePlayoutAlternateScheduleItemsHandler.cs b/ErsatzTV.Application/Playouts/Commands/ReplacePlayoutAlternateScheduleItemsHandler.cs index 0cf6490a6..434a0e439 100644 --- a/ErsatzTV.Application/Playouts/Commands/ReplacePlayoutAlternateScheduleItemsHandler.cs +++ b/ErsatzTV.Application/Playouts/Commands/ReplacePlayoutAlternateScheduleItemsHandler.cs @@ -20,7 +20,13 @@ public class ReplacePlayoutAlternateScheduleItemsHandler( ReplacePlayoutAlternateScheduleItems request, CancellationToken cancellationToken) { - // TODO: validate that items is not empty + // The handler reads the highest-index item as the default schedule (Max() below), so an empty + // list is invalid — reject it here rather than letting Max() throw. The controller pre-guards + // too, but a direct handler caller (MCP, test, reuse) must get a clean 422, not a raw crash (#254). + if (request.Items.Count == 0) + { + return BaseError.New("Playout alternate schedule items must not be empty"); + } try { @@ -152,9 +158,11 @@ public class ReplacePlayoutAlternateScheduleItemsHandler( existingValue.Name, schedule.Name); + // post-commit enqueue runs on CancellationToken.None: the schedule change is + // already committed, so a late cancellation must not drop the rebuild (#254) await channel.WriteAsync( new BuildPlayout(request.PlayoutId, PlayoutBuildMode.Refresh), - cancellationToken); + CancellationToken.None); break; } diff --git a/ErsatzTV.Application/Playouts/Commands/UpdateExternalJsonPlayoutHandler.cs b/ErsatzTV.Application/Playouts/Commands/UpdateExternalJsonPlayoutHandler.cs index 81eea96fd..c94770f57 100644 --- a/ErsatzTV.Application/Playouts/Commands/UpdateExternalJsonPlayoutHandler.cs +++ b/ErsatzTV.Application/Playouts/Commands/UpdateExternalJsonPlayoutHandler.cs @@ -42,7 +42,9 @@ public class if (await dbContext.SaveChangesAsync(cancellationToken) > 0) { - await _workerChannel.WriteAsync(new RefreshChannelData(playout.Channel.Number), cancellationToken); + // post-commit side effect runs on CancellationToken.None so a late request cancellation + // can't abort it after the commit landed (#254) + await _workerChannel.WriteAsync(new RefreshChannelData(playout.Channel.Number), CancellationToken.None); } return new PlayoutNameViewModel( diff --git a/ErsatzTV.Application/Playouts/Commands/UpdateScriptedPlayoutHandler.cs b/ErsatzTV.Application/Playouts/Commands/UpdateScriptedPlayoutHandler.cs index 329fc33b3..9ef7c1c58 100644 --- a/ErsatzTV.Application/Playouts/Commands/UpdateScriptedPlayoutHandler.cs +++ b/ErsatzTV.Application/Playouts/Commands/UpdateScriptedPlayoutHandler.cs @@ -37,7 +37,9 @@ public class if (await dbContext.SaveChangesAsync(cancellationToken) > 0) { - await workerChannel.WriteAsync(new RefreshChannelData(playout.Channel.Number), cancellationToken); + // post-commit side effect runs on CancellationToken.None so a late request cancellation + // can't abort it after the commit landed (#254) + await workerChannel.WriteAsync(new RefreshChannelData(playout.Channel.Number), CancellationToken.None); } return new PlayoutNameViewModel( diff --git a/ErsatzTV.Application/Playouts/Commands/UpdateSequentialPlayoutHandler.cs b/ErsatzTV.Application/Playouts/Commands/UpdateSequentialPlayoutHandler.cs index 1f50282ab..e81cb6ab9 100644 --- a/ErsatzTV.Application/Playouts/Commands/UpdateSequentialPlayoutHandler.cs +++ b/ErsatzTV.Application/Playouts/Commands/UpdateSequentialPlayoutHandler.cs @@ -42,7 +42,9 @@ public class if (await dbContext.SaveChangesAsync(cancellationToken) > 0) { - await _workerChannel.WriteAsync(new RefreshChannelData(playout.Channel.Number), cancellationToken); + // post-commit side effect runs on CancellationToken.None so a late request cancellation + // can't abort it after the commit landed (#254) + await _workerChannel.WriteAsync(new RefreshChannelData(playout.Channel.Number), CancellationToken.None); } return new PlayoutNameViewModel( diff --git a/ErsatzTV.Application/ProgramSchedules/Commands/AddProgramScheduleItemHandler.cs b/ErsatzTV.Application/ProgramSchedules/Commands/AddProgramScheduleItemHandler.cs index 1ec7ceb71..b8656406a 100644 --- a/ErsatzTV.Application/ProgramSchedules/Commands/AddProgramScheduleItemHandler.cs +++ b/ErsatzTV.Application/ProgramSchedules/Commands/AddProgramScheduleItemHandler.cs @@ -55,9 +55,11 @@ public class AddProgramScheduleItemHandler : ProgramScheduleItemCommandBase, await dbContext.SaveChangesAsync(cancellationToken); // refresh any playouts that use this schedule + // post-commit side effect runs on CancellationToken.None so a late request cancellation + // can't abort it after the commit landed (#254) foreach (Playout playout in programSchedule.Playouts) { - await _channel.WriteAsync(new BuildPlayout(playout.Id, PlayoutBuildMode.Refresh), cancellationToken); + await _channel.WriteAsync(new BuildPlayout(playout.Id, PlayoutBuildMode.Refresh), CancellationToken.None); } // reload with the full navigation graph before projecting: BuildItem creates the watermark/graphics diff --git a/ErsatzTV.Application/ProgramSchedules/Commands/DeleteProgramScheduleItemHandler.cs b/ErsatzTV.Application/ProgramSchedules/Commands/DeleteProgramScheduleItemHandler.cs index f54ccdd5f..017f64ac5 100644 --- a/ErsatzTV.Application/ProgramSchedules/Commands/DeleteProgramScheduleItemHandler.cs +++ b/ErsatzTV.Application/ProgramSchedules/Commands/DeleteProgramScheduleItemHandler.cs @@ -36,9 +36,11 @@ public class DeleteProgramScheduleItemHandler( dbContext.ProgramScheduleItems.Remove(item); await dbContext.SaveChangesAsync(cancellationToken); + // post-commit side effect runs on CancellationToken.None so a late request cancellation + // can't abort it after the commit landed (#254) foreach (Playout playout in playouts) { - await channel.WriteAsync(new BuildPlayout(playout.Id, PlayoutBuildMode.Refresh), cancellationToken); + await channel.WriteAsync(new BuildPlayout(playout.Id, PlayoutBuildMode.Refresh), CancellationToken.None); } return Unit.Default; diff --git a/ErsatzTV.Application/ProgramSchedules/Commands/ReplaceProgramScheduleItemsHandler.cs b/ErsatzTV.Application/ProgramSchedules/Commands/ReplaceProgramScheduleItemsHandler.cs index 0ffc63e05..f68ac5e73 100644 --- a/ErsatzTV.Application/ProgramSchedules/Commands/ReplaceProgramScheduleItemsHandler.cs +++ b/ErsatzTV.Application/ProgramSchedules/Commands/ReplaceProgramScheduleItemsHandler.cs @@ -95,9 +95,11 @@ public class ReplaceProgramScheduleItemsHandler( await dbContext.SaveChangesAsync(cancellationToken); // refresh any playouts that use this schedule + // post-commit side effect runs on CancellationToken.None so a late request cancellation + // can't abort it after the commit landed (#254) foreach (Playout playout in programSchedule.Playouts) { - await channel.WriteAsync(new BuildPlayout(playout.Id, PlayoutBuildMode.Refresh), cancellationToken); + await channel.WriteAsync(new BuildPlayout(playout.Id, PlayoutBuildMode.Refresh), CancellationToken.None); } // reload with the full navigation graph before projecting: BuildItem creates the watermark/graphics diff --git a/ErsatzTV.Tests/Application/Channels/DeleteChannelHandlerTests.cs b/ErsatzTV.Tests/Application/Channels/DeleteChannelHandlerTests.cs index 3fbb057dc..9b7142af0 100644 --- a/ErsatzTV.Tests/Application/Channels/DeleteChannelHandlerTests.cs +++ b/ErsatzTV.Tests/Application/Channels/DeleteChannelHandlerTests.cs @@ -14,7 +14,8 @@ namespace ErsatzTV.Tests.Application.Channels; [TestFixture] public class DeleteChannelHandlerTests : ChannelHandlerTestBase { - private DeleteChannelHandler MakeHandler() => new(Worker, Db.Factory, new MockFileSystem(), SearchTargets); + private DeleteChannelHandler MakeHandler(MockFileSystem? fileSystem = null) => + new(Worker, Db.Factory, fileSystem ?? new MockFileSystem(), SearchTargets); [Test] public async Task Should_Return_NotFoundError_When_Channel_Missing() @@ -39,6 +40,25 @@ public class DeleteChannelHandlerTests : ChannelHandlerTestBase exists.ShouldBeFalse(); } + [Test] + public async Task Should_Delete_Guide_Cache_File_Through_FileSystem_Abstraction() + { + await SeedChannel(1, "5"); + + var fileSystem = new MockFileSystem(); + string cacheFile = fileSystem.Path.Combine(FileSystemLayout.ChannelGuideCacheFolder, "5.xml"); + fileSystem.Directory.CreateDirectory(FileSystemLayout.ChannelGuideCacheFolder); + await fileSystem.File.WriteAllTextAsync(cacheFile, ""); + fileSystem.File.Exists(cacheFile).ShouldBeTrue(); + + Either result = + await MakeHandler(fileSystem).Handle(new DeleteChannel(1), CancellationToken.None); + + result.IsRight.ShouldBeTrue(); + // routed through the abstraction (not static File.Delete), so the MockFileSystem observes the removal + fileSystem.File.Exists(cacheFile).ShouldBeFalse(); + } + private static BaseError LeftOf(Either either) => either.Match(Left: e => e, Right: _ => throw new AssertionException("Expected a Left result")); } diff --git a/ErsatzTV.Tests/Application/Playouts/PlayoutHandlerTests.cs b/ErsatzTV.Tests/Application/Playouts/PlayoutHandlerTests.cs index 5377658e5..76a6fd8cc 100644 --- a/ErsatzTV.Tests/Application/Playouts/PlayoutHandlerTests.cs +++ b/ErsatzTV.Tests/Application/Playouts/PlayoutHandlerTests.cs @@ -9,6 +9,7 @@ using ErsatzTV.Infrastructure.Data; using ErsatzTV.Tests.Support; using LanguageExt; using MediatR; +using Microsoft.Extensions.Logging.Abstractions; using NSubstitute; using NUnit.Framework; using Shouldly; @@ -71,6 +72,52 @@ public class PlayoutHandlerTests LeftOf(result).ShouldBeOfType(); } + [Test] + public async Task Delete_Should_Remove_Guide_Cache_File_Through_FileSystem_Abstraction() + { + int channelId = await SeedChannel(); + int playoutId; + await using (TvContext context = _db.CreateContext()) + { + var playout = new Playout { ChannelId = channelId, Items = [] }; + context.Playouts.Add(playout); + await context.SaveChangesAsync(); + playoutId = playout.Id; + } + + var fileSystem = new MockFileSystem(); + string cacheFile = fileSystem.Path.Combine(FileSystemLayout.ChannelGuideCacheFolder, "101.xml"); + fileSystem.Directory.CreateDirectory(FileSystemLayout.ChannelGuideCacheFolder); + await fileSystem.File.WriteAllTextAsync(cacheFile, ""); + + var handler = new DeletePlayoutHandler( + _worker, + _db.Factory, + fileSystem, + Substitute.For()); + + Either result = await handler.Handle(new DeletePlayout(playoutId), CancellationToken.None); + + result.IsRight.ShouldBeTrue(); + // routed through the abstraction (not static File.Delete), so the MockFileSystem observes the removal + fileSystem.File.Exists(cacheFile).ShouldBeFalse(); + } + + [Test] + public async Task ReplaceAlternateScheduleItems_Should_Reject_Empty_List() + { + var handler = new ReplacePlayoutAlternateScheduleItemsHandler( + _db.Factory, + _worker, + NullLogger.Instance); + + Either result = await handler.Handle( + new ReplacePlayoutAlternateScheduleItems(1, []), + CancellationToken.None); + + LeftOf(result).Value.ShouldContain("must not be empty"); + } + private static BaseError LeftOf(Either either) => either.Match(Left: e => e, Right: _ => throw new AssertionException("Expected a Left result")); diff --git a/docs/api-conventions.md b/docs/api-conventions.md index 69f99086c..33d0f7baa 100644 --- a/docs/api-conventions.md +++ b/docs/api-conventions.md @@ -258,6 +258,38 @@ via `ProgramScheduleItemQueryExtensions.IncludeScheduleItemDetails()` (the one i the NRE; the controller's `.ToList()`/serialization does (regression: `ScheduleItemWriteProjectionTests`). GET handlers that feed an ordered list must also `.OrderBy(i => i.Index)` — id order is not index order. +### 7a. Post-commit side effects run on `CancellationToken.None` + +Once a command handler's `await dbContext.SaveChangesAsync(cancellationToken)` (or repository upsert) +has **committed**, everything that runs afterwards to complete that mutation's side effect — +`channel.WriteAsync(new BuildPlayout(...))` / other worker-channel enqueues, `mediator.Publish(...)`, +`ISearchIndex`/reindex enqueues, a cache `Refresh(...)`, and any post-commit **lookup that gates one +of those enqueues** — must be passed **`CancellationToken.None`**, not the request `cancellationToken`. + +Rationale (audit #22, issues #251 → #254): the request token is cancelled when the HTTP client +disconnects. If it's threaded into a post-commit enqueue, a late disconnect turns an *already-durable* +commit into a thrown request **and drops the side effect** (e.g. the rebuild is never queued → the +persisted change silently never takes visible effect). The commit is the point of no return: past it, +the compensating side effect must not be half-abortable. Exemplar idiom: +`ErsatzTV.Application/Scheduling/Commands/UpdateDecoHandler.cs` (the affected-playout queries **and** the +`WriteAsync(BuildPlayout..., CancellationToken.None)` enqueue all use `None`). + +Two boundaries: +- **Response projection is NOT a side effect.** The post-commit reload that builds the *returned* view + model (§7 above) legitimately keeps the request `cancellationToken` — if the client disconnected, we + don't need to compute a response nobody will read, and the durable work (commit + `None`-enqueue) has + already happened. Only the *side effect* chain gets `None`. +- **Background-job handlers keep their token.** `BuildPlayoutHandler` and other handlers invoked by the + worker (not by an HTTP request) receive the *worker's* shutdown token, not a client-disconnect token — + their downstream enqueues correctly stay on that token so a shutdown stops enqueuing more work. + +Pre-commit reads/validation and the `SaveChangesAsync` call itself keep the request token (cancelling +*before* the commit safely aborts with nothing persisted). A handler that passes **no** token to a +post-commit `WriteAsync()` is already behaviorally correct (`default` == `CancellationToken.None`); +making it explicit is optional cleanup, not required. Config handlers that commit via several sequential +`IConfigElementRepository.Upsert` calls are a distinct partial-commit-under-cancellation case not covered +by this rule (tracked separately). + ## 8. Known API warts (don't "fix" without discussion — they're deliberate synthesized rows) `GET /api/blocks` and `GET /api/templates` (via `GetAllBlocksHandler` / `GetAllTemplatesHandler` in diff --git a/docs/decisions.md b/docs/decisions.md index 1a9c1f7d7..e3e6ea615 100644 --- a/docs/decisions.md +++ b/docs/decisions.md @@ -563,3 +563,44 @@ action of the Step 2 deletion PR merge** (not before — `main` moves until then cut. Not cut this session — `main` still carries Blazor and will advance before the removal PR. + +## 2026-07-11 — Post-commit side effects run on `CancellationToken.None` (generalized from #251 to #254) + +Audit #22 (adversarial-reviewer) found ~20 command handlers threading the request `cancellationToken` +into work that runs **after** `SaveChangesAsync` commits — the post-commit `WriteAsync` enqueue that +rebuilds/refreshes the affected entity, `mediator.Publish`, search-index reindex, cache refresh. A late +HTTP-client disconnect cancels that token, so the *already-committed* mutation throws on the way out +**and silently drops its side effect** (the playout rebuild is never queued → the persisted edit never +takes visible effect until a manual Reset). #251 fixed this for the deco handlers; #254 generalizes the +policy across the codebase. + +**Decision.** Once a mutation has committed, the *entire* compensating side effect — enqueues, publishes, +reindexes, cache refreshes, and any post-commit lookup that **gates** one of those enqueues — runs on +`CancellationToken.None`. The commit is the point of no return; past it the side effect must not be +half-abortable. Full convention + the two boundaries in `docs/api-conventions.md` §7a. + +**Scope of the #254 sweep (this PR).** Swept the single-`SaveChanges` handlers under `MediaCollections/`, +`ProgramSchedules/`, `Playouts/`, `Channels/` (20 handlers). Deliberately **excluded**: +- **`BuildPlayoutHandler`** — a background/worker handler; its token is the worker shutdown token, not a + client-disconnect token, so its downstream enqueues *correctly* honor cancellation. +- **`UpdateFFmpegSettingsHandler` + the two `Configuration/` settings handlers** — they commit via several + sequential `IConfigElementRepository.Upsert` calls with an interleaved enqueue; "when is it committed" + is a partial-commit-under-cancellation question broader than the clean single-`SaveChanges` F4 pattern. + Left for a separate follow-up. +- **Response-projection reloads** (`ReplaceProgramScheduleItemsHandler` / `AddProgramScheduleItemHandler` + post-commit graph reload that builds the *returned* view model) keep the request token — a cancelled + response after a durable commit + `None`-enqueue loses nothing. +- Handlers a no-token `WriteAsync()` already makes behaviorally correct (`default` == `None`) were left + alone (explicit-`None` there is cosmetic). + +Also folded in the two other #254 items on the same handlers: the channel-guide `{number}.xml` delete in +`DeleteChannelHandler`/`DeletePlayoutHandler` now routes through `IFileSystem.File.Delete` (observable +under `MockFileSystem`) **before** the commit (a post-commit delete orphans the xml on a crash; the guide +xml is regenerable on demand, so a pre-commit delete is the safe ordering), and +`ReplacePlayoutAlternateScheduleItemsHandler` now rejects an empty item list in the handler (not only at +the controller pre-guard) so a direct caller can't trip the `Max()`-on-empty crash. + +**Coordination note for #253 PR2–PR4.** Those PRs add `Version++` (pre-commit) to the mutating handlers of +the versioned aggregates — several of which this sweep also touched (post-commit token, a different line +region). Low git-conflict risk, but merge `main` in and expect to see the `CancellationToken.None` +convention already present on the post-commit enqueues. -- 2.47.3 From 6055bd368738e82c2e5ccdd259a45940cb51209f Mon Sep 17 00:00:00 2001 From: Timothy Date: Sat, 11 Jul 2026 17:49:16 +0200 Subject: [PATCH 2/2] fix(app): fold Create/DeleteSmartCollection into the post-commit None sweep (#254 review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Independent adversarial review of PR #266 found two sibling handlers with the identical post-commit `_smartCollectionCache.Refresh(cancellationToken)` pattern that the sweep missed (only UpdateSmartCollection was caught). Same audit#22 F4 class: a late client-disconnect after the commit lands would throw and leave the in-memory smart-collection cache stale vs the committed DB. → CancellationToken.None. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../Commands/CreateSmartCollectionHandler.cs | 5 ++++- .../Commands/DeleteSmartCollectionHandler.cs | 5 ++++- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/ErsatzTV.Application/MediaCollections/Commands/CreateSmartCollectionHandler.cs b/ErsatzTV.Application/MediaCollections/Commands/CreateSmartCollectionHandler.cs index 5a9783c45..33ed35131 100644 --- a/ErsatzTV.Application/MediaCollections/Commands/CreateSmartCollectionHandler.cs +++ b/ErsatzTV.Application/MediaCollections/Commands/CreateSmartCollectionHandler.cs @@ -42,7 +42,10 @@ public class CreateSmartCollectionHandler : await dbContext.SmartCollections.AddAsync(smartCollection, cancellationToken); await dbContext.SaveChangesAsync(cancellationToken); _searchTargets.SearchTargetsChanged(); - await _smartCollectionCache.Refresh(cancellationToken); + + // post-commit side effect runs on CancellationToken.None so a late request cancellation + // can't abort it after the commit landed (#254) + await _smartCollectionCache.Refresh(CancellationToken.None); return ProjectToViewModel(smartCollection); } diff --git a/ErsatzTV.Application/MediaCollections/Commands/DeleteSmartCollectionHandler.cs b/ErsatzTV.Application/MediaCollections/Commands/DeleteSmartCollectionHandler.cs index 7fdea22d6..2b6e849a8 100644 --- a/ErsatzTV.Application/MediaCollections/Commands/DeleteSmartCollectionHandler.cs +++ b/ErsatzTV.Application/MediaCollections/Commands/DeleteSmartCollectionHandler.cs @@ -48,7 +48,10 @@ public class DeleteSmartCollectionHandler : IRequestHandler