From 83f753b211e2a6fedcbb8555c34afb8d90d2beb5 Mon Sep 17 00:00:00 2001 From: Timothy Date: Sun, 12 Jul 2026 18:58:07 +0200 Subject: [PATCH 1/2] fix(api): #269 rotate aggregate ETag on Collection/Playout config siblings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Complete the #253 optimistic-concurrency contract's cross-editor ETag rotation tail. The non-If-Match config siblings mutated editor-visible state without bumping Version, so a concurrent editor of the same root never invalidated. Now the Collection Add*/Remove handlers bump Collection.Version, and UpdateCollection / UpdatePlayout / the three ScheduleFile writers (which already force-wrote past a concurrent bump) now bump too — all via SaveChangesForcingVersion (no If-Match → force write, never 412/500). No-op idempotence (Fable-caught trap): these gate reindex/BuildPlayout fan-out on SaveChanges()>0, so an unconditional bump would fire spurious rebuilds on an idempotent re-add / same-value re-submit. Each now short-circuits a genuine no-op before the bump — Add handlers by an explicit membership check (also fixing a latent duplicate-CollectionItem insert), scalar writers by ChangeTracker.HasChanges(). Corrects #269's framing: the Add*ToCollection family is not repository-mediated (IMediaCollectionRepository is read-only); each handler writes via its own dbContext, so the scanner's separate membership path is unaffected (a background scan does not rotate the editor ETag). Tests: CollectionEtagRotationTests + PlayoutScheduleFileEtagRotationTests (rotation, no-op-without-bump-or-rebuild, force-write-past-concurrent-bump), no-op guard proven non-vacuous by inverting the membership check. Docs: api-conventions §7a + decisions.md. No new status codes / no OpenAPI change (these endpoints take no If-Match, never 412). The #265 RFC-7232 If-Match parser refinement is a separate PR. fixes #269 Co-Authored-By: Claude Opus 4.8 (1M context) --- .../Commands/AddArtistToCollectionHandler.cs | 29 ++- .../Commands/AddEpisodeToCollectionHandler.cs | 29 ++- .../Commands/AddImageToCollectionHandler.cs | 29 ++- .../Commands/AddItemsToCollectionHandler.cs | 33 ++- .../AddMediaItemToCollectionHandler.cs | 29 ++- .../Commands/AddMovieToCollectionHandler.cs | 29 ++- .../AddMusicVideoToCollectionHandler.cs | 29 ++- .../AddOtherVideoToCollectionHandler.cs | 29 ++- .../Commands/AddSeasonToCollectionHandler.cs | 29 ++- .../Commands/AddShowToCollectionHandler.cs | 29 ++- .../Commands/AddSongToCollectionHandler.cs | 29 ++- .../RemoveItemsFromCollectionHandler.cs | 33 ++- .../Commands/UpdateCollectionHandler.cs | 26 +- .../UpdateExternalJsonPlayoutHandler.cs | 10 +- .../Playouts/Commands/UpdatePlayoutHandler.cs | 12 +- .../Commands/UpdateScriptedPlayoutHandler.cs | 10 +- .../UpdateSequentialPlayoutHandler.cs | 10 +- .../CollectionEtagRotationTests.cs | 243 ++++++++++++++++++ .../PlayoutScheduleFileEtagRotationTests.cs | 105 ++++++++ docs/api-conventions.md | 24 +- docs/decisions.md | 44 ++++ 21 files changed, 700 insertions(+), 140 deletions(-) create mode 100644 ErsatzTV.Tests/Application/MediaCollections/CollectionEtagRotationTests.cs create mode 100644 ErsatzTV.Tests/Application/Playouts/PlayoutScheduleFileEtagRotationTests.cs diff --git a/ErsatzTV.Application/MediaCollections/Commands/AddArtistToCollectionHandler.cs b/ErsatzTV.Application/MediaCollections/Commands/AddArtistToCollectionHandler.cs index d5027c5b9..df3c4c829 100644 --- a/ErsatzTV.Application/MediaCollections/Commands/AddArtistToCollectionHandler.cs +++ b/ErsatzTV.Application/MediaCollections/Commands/AddArtistToCollectionHandler.cs @@ -42,17 +42,28 @@ public class AddArtistToCollectionHandler : private async Task ApplyAddArtistRequest(TvContext dbContext, Parameters parameters) { - parameters.Collection.MediaItems.Add(parameters.Artist); - if (await dbContext.SaveChangesAsync() > 0) + // No-op on an idempotent re-add: don't rotate the ETag or fan out rebuilds for an item that is + // already a member (also avoids a duplicate CollectionItem row) — #269. + if (parameters.Collection.MediaItems.Any(mi => mi.Id == parameters.Artist.Id)) { - await _searchChannel.WriteAsync(new ReindexMediaItems([parameters.Artist.Id])); + return Unit.Default; + } - // refresh all playouts that use this collection - foreach (int playoutId in await _mediaCollectionRepository - .PlayoutIdsUsingCollection(parameters.Collection.Id)) - { - await _channel.WriteAsync(new BuildPlayout(playoutId, PlayoutBuildMode.Refresh)); - } + parameters.Collection.MediaItems.Add(parameters.Artist); + + // Rotate the collection ETag so an open custom-order editor's If-Match invalidates (#269); + // force-write past a concurrent replace-all Version bump — this add takes no If-Match, so a + // benign race must not 500 (#253/#269 §7a). Post-commit enqueues run on CancellationToken.None. + parameters.Collection.Version++; + await dbContext.SaveChangesForcingVersion(CancellationToken.None); + + await _searchChannel.WriteAsync(new ReindexMediaItems([parameters.Artist.Id]), CancellationToken.None); + + // refresh all playouts that use this collection + foreach (int playoutId in await _mediaCollectionRepository + .PlayoutIdsUsingCollection(parameters.Collection.Id)) + { + await _channel.WriteAsync(new BuildPlayout(playoutId, PlayoutBuildMode.Refresh), CancellationToken.None); } return Unit.Default; diff --git a/ErsatzTV.Application/MediaCollections/Commands/AddEpisodeToCollectionHandler.cs b/ErsatzTV.Application/MediaCollections/Commands/AddEpisodeToCollectionHandler.cs index 3ce2bfc16..b46405fc7 100644 --- a/ErsatzTV.Application/MediaCollections/Commands/AddEpisodeToCollectionHandler.cs +++ b/ErsatzTV.Application/MediaCollections/Commands/AddEpisodeToCollectionHandler.cs @@ -44,17 +44,28 @@ public class AddEpisodeToCollectionHandler : private async Task ApplyAddTelevisionEpisodeRequest(TvContext dbContext, Parameters parameters) { - parameters.Collection.MediaItems.Add(parameters.Episode); - if (await dbContext.SaveChangesAsync() > 0) + // No-op on an idempotent re-add: don't rotate the ETag or fan out rebuilds for an item that is + // already a member (also avoids a duplicate CollectionItem row) — #269. + if (parameters.Collection.MediaItems.Any(mi => mi.Id == parameters.Episode.Id)) { - await _searchChannel.WriteAsync(new ReindexMediaItems([parameters.Episode.Id])); + return Unit.Default; + } - // refresh all playouts that use this collection - foreach (int playoutId in await _mediaCollectionRepository - .PlayoutIdsUsingCollection(parameters.Collection.Id)) - { - await _channel.WriteAsync(new BuildPlayout(playoutId, PlayoutBuildMode.Refresh)); - } + parameters.Collection.MediaItems.Add(parameters.Episode); + + // Rotate the collection ETag so an open custom-order editor's If-Match invalidates (#269); + // force-write past a concurrent replace-all Version bump — this add takes no If-Match, so a + // benign race must not 500 (#253/#269 §7a). Post-commit enqueues run on CancellationToken.None. + parameters.Collection.Version++; + await dbContext.SaveChangesForcingVersion(CancellationToken.None); + + await _searchChannel.WriteAsync(new ReindexMediaItems([parameters.Episode.Id]), CancellationToken.None); + + // refresh all playouts that use this collection + foreach (int playoutId in await _mediaCollectionRepository + .PlayoutIdsUsingCollection(parameters.Collection.Id)) + { + await _channel.WriteAsync(new BuildPlayout(playoutId, PlayoutBuildMode.Refresh), CancellationToken.None); } return Unit.Default; diff --git a/ErsatzTV.Application/MediaCollections/Commands/AddImageToCollectionHandler.cs b/ErsatzTV.Application/MediaCollections/Commands/AddImageToCollectionHandler.cs index d41255a2f..2eba28437 100644 --- a/ErsatzTV.Application/MediaCollections/Commands/AddImageToCollectionHandler.cs +++ b/ErsatzTV.Application/MediaCollections/Commands/AddImageToCollectionHandler.cs @@ -41,17 +41,28 @@ public class AddImageToCollectionHandler : IRequestHandler ApplyAddImageRequest(TvContext dbContext, Parameters parameters) { - parameters.Collection.MediaItems.Add(parameters.Image); - if (await dbContext.SaveChangesAsync() > 0) + // No-op on an idempotent re-add: don't rotate the ETag or fan out rebuilds for an item that is + // already a member (also avoids a duplicate CollectionItem row) — #269. + if (parameters.Collection.MediaItems.Any(mi => mi.Id == parameters.Image.Id)) { - await _searchChannel.WriteAsync(new ReindexMediaItems([parameters.Image.Id])); + return Unit.Default; + } - // refresh all playouts that use this collection - foreach (int playoutId in await _mediaCollectionRepository - .PlayoutIdsUsingCollection(parameters.Collection.Id)) - { - await _channel.WriteAsync(new BuildPlayout(playoutId, PlayoutBuildMode.Refresh)); - } + parameters.Collection.MediaItems.Add(parameters.Image); + + // Rotate the collection ETag so an open custom-order editor's If-Match invalidates (#269); + // force-write past a concurrent replace-all Version bump — this add takes no If-Match, so a + // benign race must not 500 (#253/#269 §7a). Post-commit enqueues run on CancellationToken.None. + parameters.Collection.Version++; + await dbContext.SaveChangesForcingVersion(CancellationToken.None); + + await _searchChannel.WriteAsync(new ReindexMediaItems([parameters.Image.Id]), CancellationToken.None); + + // refresh all playouts that use this collection + foreach (int playoutId in await _mediaCollectionRepository + .PlayoutIdsUsingCollection(parameters.Collection.Id)) + { + await _channel.WriteAsync(new BuildPlayout(playoutId, PlayoutBuildMode.Refresh), CancellationToken.None); } return Unit.Default; diff --git a/ErsatzTV.Application/MediaCollections/Commands/AddItemsToCollectionHandler.cs b/ErsatzTV.Application/MediaCollections/Commands/AddItemsToCollectionHandler.cs index 475a1494d..e325e8588 100644 --- a/ErsatzTV.Application/MediaCollections/Commands/AddItemsToCollectionHandler.cs +++ b/ErsatzTV.Application/MediaCollections/Commands/AddItemsToCollectionHandler.cs @@ -73,24 +73,35 @@ public class AddItemsToCollectionHandler : .ToList(); var toAddIds = allItems.Where(item => collection.MediaItems.All(mi => mi.Id != item)).ToList(); + + // No-op when every requested item is already a member: don't rotate the ETag or fan out + // rebuilds for an idempotent re-add — #269. + if (toAddIds.Count == 0) + { + return Unit.Default; + } + List toAdd = await dbContext.MediaItems .Filter(mi => toAddIds.Contains(mi.Id)) .ToListAsync(cancellationToken); collection.MediaItems.AddRange(toAdd); - if (await dbContext.SaveChangesAsync(cancellationToken) > 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(toAddIds.ToArray()), CancellationToken.None); + // Rotate the collection ETag so an open custom-order editor's If-Match invalidates (#269); + // force-write past a concurrent replace-all Version bump — this add takes no If-Match, so a + // benign race must not 500 (#253/#269 §7a). + collection.Version++; + await dbContext.SaveChangesForcingVersion(cancellationToken); - // 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.None); - } + // 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.None); } return Unit.Default; diff --git a/ErsatzTV.Application/MediaCollections/Commands/AddMediaItemToCollectionHandler.cs b/ErsatzTV.Application/MediaCollections/Commands/AddMediaItemToCollectionHandler.cs index 187db312d..195e6cff9 100644 --- a/ErsatzTV.Application/MediaCollections/Commands/AddMediaItemToCollectionHandler.cs +++ b/ErsatzTV.Application/MediaCollections/Commands/AddMediaItemToCollectionHandler.cs @@ -42,17 +42,28 @@ public class AddMediaItemToCollectionHandler : private async Task ApplyAddMediaItemRequest(TvContext dbContext, Parameters parameters) { - parameters.Collection.MediaItems.Add(parameters.MediaItem); - if (await dbContext.SaveChangesAsync() > 0) + // No-op on an idempotent re-add: don't rotate the ETag or fan out rebuilds for an item that is + // already a member (also avoids a duplicate CollectionItem row) — #269. + if (parameters.Collection.MediaItems.Any(mi => mi.Id == parameters.MediaItem.Id)) { - await _searchChannel.WriteAsync(new ReindexMediaItems([parameters.MediaItem.Id])); + return Unit.Default; + } - // refresh all playouts that use this collection - foreach (int playoutId in await _mediaCollectionRepository - .PlayoutIdsUsingCollection(parameters.Collection.Id)) - { - await _channel.WriteAsync(new BuildPlayout(playoutId, PlayoutBuildMode.Refresh)); - } + parameters.Collection.MediaItems.Add(parameters.MediaItem); + + // Rotate the collection ETag so an open custom-order editor's If-Match invalidates (#269); + // force-write past a concurrent replace-all Version bump — this add takes no If-Match, so a + // benign race must not 500 (#253/#269 §7a). Post-commit enqueues run on CancellationToken.None. + parameters.Collection.Version++; + await dbContext.SaveChangesForcingVersion(CancellationToken.None); + + await _searchChannel.WriteAsync(new ReindexMediaItems([parameters.MediaItem.Id]), CancellationToken.None); + + // refresh all playouts that use this collection + foreach (int playoutId in await _mediaCollectionRepository + .PlayoutIdsUsingCollection(parameters.Collection.Id)) + { + await _channel.WriteAsync(new BuildPlayout(playoutId, PlayoutBuildMode.Refresh), CancellationToken.None); } return Unit.Default; diff --git a/ErsatzTV.Application/MediaCollections/Commands/AddMovieToCollectionHandler.cs b/ErsatzTV.Application/MediaCollections/Commands/AddMovieToCollectionHandler.cs index 0a2bd8269..603d4d49a 100644 --- a/ErsatzTV.Application/MediaCollections/Commands/AddMovieToCollectionHandler.cs +++ b/ErsatzTV.Application/MediaCollections/Commands/AddMovieToCollectionHandler.cs @@ -42,17 +42,28 @@ public class AddMovieToCollectionHandler : private async Task ApplyAddMovieRequest(TvContext dbContext, Parameters parameters) { - parameters.Collection.MediaItems.Add(parameters.Movie); - if (await dbContext.SaveChangesAsync() > 0) + // No-op on an idempotent re-add: don't rotate the ETag or fan out rebuilds for an item that is + // already a member (also avoids a duplicate CollectionItem row) — #269. + if (parameters.Collection.MediaItems.Any(mi => mi.Id == parameters.Movie.Id)) { - await _searchChannel.WriteAsync(new ReindexMediaItems([parameters.Movie.Id])); + return Unit.Default; + } - // refresh all playouts that use this collection - foreach (int playoutId in await _mediaCollectionRepository - .PlayoutIdsUsingCollection(parameters.Collection.Id)) - { - await _channel.WriteAsync(new BuildPlayout(playoutId, PlayoutBuildMode.Refresh)); - } + parameters.Collection.MediaItems.Add(parameters.Movie); + + // Rotate the collection ETag so an open custom-order editor's If-Match invalidates (#269); + // force-write past a concurrent replace-all Version bump — this add takes no If-Match, so a + // benign race must not 500 (#253/#269 §7a). Post-commit enqueues run on CancellationToken.None. + parameters.Collection.Version++; + await dbContext.SaveChangesForcingVersion(CancellationToken.None); + + await _searchChannel.WriteAsync(new ReindexMediaItems([parameters.Movie.Id]), CancellationToken.None); + + // refresh all playouts that use this collection + foreach (int playoutId in await _mediaCollectionRepository + .PlayoutIdsUsingCollection(parameters.Collection.Id)) + { + await _channel.WriteAsync(new BuildPlayout(playoutId, PlayoutBuildMode.Refresh), CancellationToken.None); } return Unit.Default; diff --git a/ErsatzTV.Application/MediaCollections/Commands/AddMusicVideoToCollectionHandler.cs b/ErsatzTV.Application/MediaCollections/Commands/AddMusicVideoToCollectionHandler.cs index 05d194073..39b2b21f4 100644 --- a/ErsatzTV.Application/MediaCollections/Commands/AddMusicVideoToCollectionHandler.cs +++ b/ErsatzTV.Application/MediaCollections/Commands/AddMusicVideoToCollectionHandler.cs @@ -44,17 +44,28 @@ public class AddMusicVideoToCollectionHandler : private async Task ApplyAddMusicVideoRequest(TvContext dbContext, Parameters parameters) { - parameters.Collection.MediaItems.Add(parameters.MusicVideo); - if (await dbContext.SaveChangesAsync() > 0) + // No-op on an idempotent re-add: don't rotate the ETag or fan out rebuilds for an item that is + // already a member (also avoids a duplicate CollectionItem row) — #269. + if (parameters.Collection.MediaItems.Any(mi => mi.Id == parameters.MusicVideo.Id)) { - await _searchChannel.WriteAsync(new ReindexMediaItems([parameters.MusicVideo.Id])); + return Unit.Default; + } - // refresh all playouts that use this collection - foreach (int playoutId in await _mediaCollectionRepository - .PlayoutIdsUsingCollection(parameters.Collection.Id)) - { - await _channel.WriteAsync(new BuildPlayout(playoutId, PlayoutBuildMode.Refresh)); - } + parameters.Collection.MediaItems.Add(parameters.MusicVideo); + + // Rotate the collection ETag so an open custom-order editor's If-Match invalidates (#269); + // force-write past a concurrent replace-all Version bump — this add takes no If-Match, so a + // benign race must not 500 (#253/#269 §7a). Post-commit enqueues run on CancellationToken.None. + parameters.Collection.Version++; + await dbContext.SaveChangesForcingVersion(CancellationToken.None); + + await _searchChannel.WriteAsync(new ReindexMediaItems([parameters.MusicVideo.Id]), CancellationToken.None); + + // refresh all playouts that use this collection + foreach (int playoutId in await _mediaCollectionRepository + .PlayoutIdsUsingCollection(parameters.Collection.Id)) + { + await _channel.WriteAsync(new BuildPlayout(playoutId, PlayoutBuildMode.Refresh), CancellationToken.None); } return Unit.Default; diff --git a/ErsatzTV.Application/MediaCollections/Commands/AddOtherVideoToCollectionHandler.cs b/ErsatzTV.Application/MediaCollections/Commands/AddOtherVideoToCollectionHandler.cs index a4a0a78d2..c76fcfa48 100644 --- a/ErsatzTV.Application/MediaCollections/Commands/AddOtherVideoToCollectionHandler.cs +++ b/ErsatzTV.Application/MediaCollections/Commands/AddOtherVideoToCollectionHandler.cs @@ -44,17 +44,28 @@ public class AddOtherVideoToCollectionHandler : private async Task ApplyAddOtherVideoRequest(TvContext dbContext, Parameters parameters) { - parameters.Collection.MediaItems.Add(parameters.OtherVideo); - if (await dbContext.SaveChangesAsync() > 0) + // No-op on an idempotent re-add: don't rotate the ETag or fan out rebuilds for an item that is + // already a member (also avoids a duplicate CollectionItem row) — #269. + if (parameters.Collection.MediaItems.Any(mi => mi.Id == parameters.OtherVideo.Id)) { - await _searchChannel.WriteAsync(new ReindexMediaItems([parameters.OtherVideo.Id])); + return Unit.Default; + } - // refresh all playouts that use this collection - foreach (int playoutId in await _mediaCollectionRepository - .PlayoutIdsUsingCollection(parameters.Collection.Id)) - { - await _channel.WriteAsync(new BuildPlayout(playoutId, PlayoutBuildMode.Refresh)); - } + parameters.Collection.MediaItems.Add(parameters.OtherVideo); + + // Rotate the collection ETag so an open custom-order editor's If-Match invalidates (#269); + // force-write past a concurrent replace-all Version bump — this add takes no If-Match, so a + // benign race must not 500 (#253/#269 §7a). Post-commit enqueues run on CancellationToken.None. + parameters.Collection.Version++; + await dbContext.SaveChangesForcingVersion(CancellationToken.None); + + await _searchChannel.WriteAsync(new ReindexMediaItems([parameters.OtherVideo.Id]), CancellationToken.None); + + // refresh all playouts that use this collection + foreach (int playoutId in await _mediaCollectionRepository + .PlayoutIdsUsingCollection(parameters.Collection.Id)) + { + await _channel.WriteAsync(new BuildPlayout(playoutId, PlayoutBuildMode.Refresh), CancellationToken.None); } return Unit.Default; diff --git a/ErsatzTV.Application/MediaCollections/Commands/AddSeasonToCollectionHandler.cs b/ErsatzTV.Application/MediaCollections/Commands/AddSeasonToCollectionHandler.cs index 549163fcf..4613010eb 100644 --- a/ErsatzTV.Application/MediaCollections/Commands/AddSeasonToCollectionHandler.cs +++ b/ErsatzTV.Application/MediaCollections/Commands/AddSeasonToCollectionHandler.cs @@ -42,17 +42,28 @@ public class AddSeasonToCollectionHandler : private async Task ApplyAddSeasonRequest(TvContext dbContext, Parameters parameters) { - parameters.Collection.MediaItems.Add(parameters.Season); - if (await dbContext.SaveChangesAsync() > 0) + // No-op on an idempotent re-add: don't rotate the ETag or fan out rebuilds for an item that is + // already a member (also avoids a duplicate CollectionItem row) — #269. + if (parameters.Collection.MediaItems.Any(mi => mi.Id == parameters.Season.Id)) { - await _searchChannel.WriteAsync(new ReindexMediaItems([parameters.Season.Id])); + return Unit.Default; + } - // refresh all playouts that use this collection - foreach (int playoutId in await _mediaCollectionRepository - .PlayoutIdsUsingCollection(parameters.Collection.Id)) - { - await _channel.WriteAsync(new BuildPlayout(playoutId, PlayoutBuildMode.Refresh)); - } + parameters.Collection.MediaItems.Add(parameters.Season); + + // Rotate the collection ETag so an open custom-order editor's If-Match invalidates (#269); + // force-write past a concurrent replace-all Version bump — this add takes no If-Match, so a + // benign race must not 500 (#253/#269 §7a). Post-commit enqueues run on CancellationToken.None. + parameters.Collection.Version++; + await dbContext.SaveChangesForcingVersion(CancellationToken.None); + + await _searchChannel.WriteAsync(new ReindexMediaItems([parameters.Season.Id]), CancellationToken.None); + + // refresh all playouts that use this collection + foreach (int playoutId in await _mediaCollectionRepository + .PlayoutIdsUsingCollection(parameters.Collection.Id)) + { + await _channel.WriteAsync(new BuildPlayout(playoutId, PlayoutBuildMode.Refresh), CancellationToken.None); } return Unit.Default; diff --git a/ErsatzTV.Application/MediaCollections/Commands/AddShowToCollectionHandler.cs b/ErsatzTV.Application/MediaCollections/Commands/AddShowToCollectionHandler.cs index 24911678a..f5bbc9fa0 100644 --- a/ErsatzTV.Application/MediaCollections/Commands/AddShowToCollectionHandler.cs +++ b/ErsatzTV.Application/MediaCollections/Commands/AddShowToCollectionHandler.cs @@ -42,17 +42,28 @@ public class AddShowToCollectionHandler : private async Task ApplyAddShowRequest(TvContext dbContext, Parameters parameters) { - parameters.Collection.MediaItems.Add(parameters.Show); - if (await dbContext.SaveChangesAsync() > 0) + // No-op on an idempotent re-add: don't rotate the ETag or fan out rebuilds for an item that is + // already a member (also avoids a duplicate CollectionItem row) — #269. + if (parameters.Collection.MediaItems.Any(mi => mi.Id == parameters.Show.Id)) { - await _searchChannel.WriteAsync(new ReindexMediaItems([parameters.Show.Id])); + return Unit.Default; + } - // refresh all playouts that use this collection - foreach (int playoutId in await _mediaCollectionRepository - .PlayoutIdsUsingCollection(parameters.Collection.Id)) - { - await _channel.WriteAsync(new BuildPlayout(playoutId, PlayoutBuildMode.Refresh)); - } + parameters.Collection.MediaItems.Add(parameters.Show); + + // Rotate the collection ETag so an open custom-order editor's If-Match invalidates (#269); + // force-write past a concurrent replace-all Version bump — this add takes no If-Match, so a + // benign race must not 500 (#253/#269 §7a). Post-commit enqueues run on CancellationToken.None. + parameters.Collection.Version++; + await dbContext.SaveChangesForcingVersion(CancellationToken.None); + + await _searchChannel.WriteAsync(new ReindexMediaItems([parameters.Show.Id]), CancellationToken.None); + + // refresh all playouts that use this collection + foreach (int playoutId in await _mediaCollectionRepository + .PlayoutIdsUsingCollection(parameters.Collection.Id)) + { + await _channel.WriteAsync(new BuildPlayout(playoutId, PlayoutBuildMode.Refresh), CancellationToken.None); } return Unit.Default; diff --git a/ErsatzTV.Application/MediaCollections/Commands/AddSongToCollectionHandler.cs b/ErsatzTV.Application/MediaCollections/Commands/AddSongToCollectionHandler.cs index d00339ffa..aea054ed2 100644 --- a/ErsatzTV.Application/MediaCollections/Commands/AddSongToCollectionHandler.cs +++ b/ErsatzTV.Application/MediaCollections/Commands/AddSongToCollectionHandler.cs @@ -42,17 +42,28 @@ public class AddSongToCollectionHandler : private async Task ApplyAddSongRequest(TvContext dbContext, Parameters parameters) { - parameters.Collection.MediaItems.Add(parameters.Song); - if (await dbContext.SaveChangesAsync() > 0) + // No-op on an idempotent re-add: don't rotate the ETag or fan out rebuilds for an item that is + // already a member (also avoids a duplicate CollectionItem row) — #269. + if (parameters.Collection.MediaItems.Any(mi => mi.Id == parameters.Song.Id)) { - await _searchChannel.WriteAsync(new ReindexMediaItems([parameters.Song.Id])); + return Unit.Default; + } - // refresh all playouts that use this collection - foreach (int playoutId in await _mediaCollectionRepository - .PlayoutIdsUsingCollection(parameters.Collection.Id)) - { - await _channel.WriteAsync(new BuildPlayout(playoutId, PlayoutBuildMode.Refresh)); - } + parameters.Collection.MediaItems.Add(parameters.Song); + + // Rotate the collection ETag so an open custom-order editor's If-Match invalidates (#269); + // force-write past a concurrent replace-all Version bump — this add takes no If-Match, so a + // benign race must not 500 (#253/#269 §7a). Post-commit enqueues run on CancellationToken.None. + parameters.Collection.Version++; + await dbContext.SaveChangesForcingVersion(CancellationToken.None); + + await _searchChannel.WriteAsync(new ReindexMediaItems([parameters.Song.Id]), CancellationToken.None); + + // refresh all playouts that use this collection + foreach (int playoutId in await _mediaCollectionRepository + .PlayoutIdsUsingCollection(parameters.Collection.Id)) + { + await _channel.WriteAsync(new BuildPlayout(playoutId, PlayoutBuildMode.Refresh), CancellationToken.None); } return Unit.Default; diff --git a/ErsatzTV.Application/MediaCollections/Commands/RemoveItemsFromCollectionHandler.cs b/ErsatzTV.Application/MediaCollections/Commands/RemoveItemsFromCollectionHandler.cs index 24af4aaae..135cbe251 100644 --- a/ErsatzTV.Application/MediaCollections/Commands/RemoveItemsFromCollectionHandler.cs +++ b/ErsatzTV.Application/MediaCollections/Commands/RemoveItemsFromCollectionHandler.cs @@ -59,21 +59,30 @@ public class RemoveItemsFromCollectionHandler : IRequestHandler collection.MediaItems.Remove(m)); - if (itemsToRemove.Count != 0 && await dbContext.SaveChangesAsync(cancellationToken) > 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.None); + // Rotate the collection ETag so an open custom-order editor's If-Match invalidates (#269); + // force-write past a concurrent replace-all Version bump — this remove takes no If-Match, so a + // benign race must not 500 (#253/#269 §7a). + collection.Version++; + await dbContext.SaveChangesForcingVersion(cancellationToken); - // 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.None); - } + // 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.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.None); } return Unit.Default; diff --git a/ErsatzTV.Application/MediaCollections/Commands/UpdateCollectionHandler.cs b/ErsatzTV.Application/MediaCollections/Commands/UpdateCollectionHandler.cs index 5b0830eb6..52ad0ddf2 100644 --- a/ErsatzTV.Application/MediaCollections/Commands/UpdateCollectionHandler.cs +++ b/ErsatzTV.Application/MediaCollections/Commands/UpdateCollectionHandler.cs @@ -59,17 +59,25 @@ public class UpdateCollectionHandler : IRequestHandler 0 && request.UseCustomPlaybackOrder.IsSome) + // Only rotate the ETag when the name/flag actually changed — a no-op re-submit must not bump the + // Version (spurious rebuilds / editor invalidation) — #269. When it did change, force-write past a + // concurrent bump from the custom-order editor (this writer takes no If-Match, so a benign race + // must not 500) — #253/#269 §7a. + if (dbContext.ChangeTracker.HasChanges()) { - // 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)) + c.Version++; + await dbContext.SaveChangesForcingVersion(cancellationToken); + + if (request.UseCustomPlaybackOrder.IsSome) { - await _channel.WriteAsync(new BuildPlayout(playoutId, PlayoutBuildMode.Refresh), CancellationToken.None); + // 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.None); + } } } diff --git a/ErsatzTV.Application/Playouts/Commands/UpdateExternalJsonPlayoutHandler.cs b/ErsatzTV.Application/Playouts/Commands/UpdateExternalJsonPlayoutHandler.cs index 2ea547c66..dc1fbb597 100644 --- a/ErsatzTV.Application/Playouts/Commands/UpdateExternalJsonPlayoutHandler.cs +++ b/ErsatzTV.Application/Playouts/Commands/UpdateExternalJsonPlayoutHandler.cs @@ -40,10 +40,14 @@ public class { playout.ScheduleFile = request.ScheduleFile; - // Force-write past a concurrent Version bump from a replace-all editor (schedule-file writer - // doesn't participate in If-Match, so an active token must not 500 a benign race) — #253/#269. - if (await dbContext.SaveChangesForcingVersion(cancellationToken) > 0) + // Rotate the playout ETag only when the schedule-file actually changed — a no-op re-submit must + // not bump (#269). When it changed, force-write past a concurrent Version bump from a replace-all + // editor (this writer takes no If-Match, so a benign race must not 500) — #253/#269 §7a. + if (dbContext.ChangeTracker.HasChanges()) { + playout.Version++; + await dbContext.SaveChangesForcingVersion(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); diff --git a/ErsatzTV.Application/Playouts/Commands/UpdatePlayoutHandler.cs b/ErsatzTV.Application/Playouts/Commands/UpdatePlayoutHandler.cs index 3ede2fdb6..f7d43279d 100644 --- a/ErsatzTV.Application/Playouts/Commands/UpdatePlayoutHandler.cs +++ b/ErsatzTV.Application/Playouts/Commands/UpdatePlayoutHandler.cs @@ -35,9 +35,15 @@ public class UpdatePlayoutHandler : IRequestHandler 0) + // Rotate the playout ETag only when the schedule-file actually changed — a no-op re-submit must + // not bump (#269). When it changed, force-write past a concurrent Version bump from a replace-all + // editor (this writer takes no If-Match, so a benign race must not 500) — #253/#269 §7a. + if (dbContext.ChangeTracker.HasChanges()) { + playout.Version++; + await dbContext.SaveChangesForcingVersion(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); diff --git a/ErsatzTV.Application/Playouts/Commands/UpdateSequentialPlayoutHandler.cs b/ErsatzTV.Application/Playouts/Commands/UpdateSequentialPlayoutHandler.cs index 55bd00319..0f3798537 100644 --- a/ErsatzTV.Application/Playouts/Commands/UpdateSequentialPlayoutHandler.cs +++ b/ErsatzTV.Application/Playouts/Commands/UpdateSequentialPlayoutHandler.cs @@ -40,10 +40,14 @@ public class { playout.ScheduleFile = request.ScheduleFile; - // Force-write past a concurrent Version bump from a replace-all editor (schedule-file writer - // doesn't participate in If-Match, so an active token must not 500 a benign race) — #253/#269. - if (await dbContext.SaveChangesForcingVersion(cancellationToken) > 0) + // Rotate the playout ETag only when the schedule-file actually changed — a no-op re-submit must + // not bump (#269). When it changed, force-write past a concurrent Version bump from a replace-all + // editor (this writer takes no If-Match, so a benign race must not 500) — #253/#269 §7a. + if (dbContext.ChangeTracker.HasChanges()) { + playout.Version++; + await dbContext.SaveChangesForcingVersion(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); diff --git a/ErsatzTV.Tests/Application/MediaCollections/CollectionEtagRotationTests.cs b/ErsatzTV.Tests/Application/MediaCollections/CollectionEtagRotationTests.cs new file mode 100644 index 000000000..bfb6b1a8f --- /dev/null +++ b/ErsatzTV.Tests/Application/MediaCollections/CollectionEtagRotationTests.cs @@ -0,0 +1,243 @@ +using System.Threading.Channels; +using ErsatzTV.Application; +using ErsatzTV.Application.MediaCollections; +using ErsatzTV.Application.Search; +using ErsatzTV.Core; +using ErsatzTV.Core.Domain; +using ErsatzTV.Core.Interfaces.Repositories; +using ErsatzTV.Infrastructure.Data; +using ErsatzTV.Tests.Support; +using LanguageExt; +using Microsoft.EntityFrameworkCore; +using NSubstitute; +using NUnit.Framework; +using Shouldly; +using Unit = LanguageExt.Unit; +using static LanguageExt.Prelude; + +namespace ErsatzTV.Tests.Application.MediaCollections; + +/// +/// #269: the non-If-Match config-sibling writers of a Collection (name/flag edit, membership +/// add/remove) rotate the aggregate ETag by bumping Collection.Version, so an open +/// custom-order editor's stale If-Match invalidates. Two invariants beyond the bump: an +/// idempotent no-op (re-add an existing member / re-submit the same name) must NOT bump the Version +/// or fan out playout rebuilds (the SaveChanges() > 0 side-effect gate would otherwise fire +/// spuriously once the bump makes it always-true — Fable D1), and the bump must force-write past a +/// concurrent replace-all bump rather than 500 (these writers take no If-Match). +/// +[TestFixture] +public class CollectionEtagRotationTests +{ + private InMemoryTvContext _db = null!; + + [SetUp] + public async Task SetUp() => _db = await InMemoryTvContext.CreateAsync(); + + [TearDown] + public async Task TearDown() => await _db.DisposeAsync(); + + private sealed class PreTrackedFactory(TvContext context) : IDbContextFactory + { + public TvContext CreateDbContext() => context; + } + + private async Task SeedCollection(int id, int version, bool withMovie, string name = "Manual") + { + await using TvContext context = _db.CreateContext(); + + var library = new LocalLibrary + { + Id = 1, + Name = "Library", + MediaKind = LibraryMediaKind.Movies, + Paths = [] + }; + var path = new LibraryPath { Id = 1, Path = "/media", Library = library, LibraryFolders = [], MediaItems = [] }; + library.Paths.Add(path); + + context.Movies.Add(new Movie + { + Id = 10, + LibraryPath = path, + Collections = [], + CollectionItems = [], + TraktListItems = [], + MovieMetadata = + [ + new MovieMetadata + { + Title = "Movie", SortTitle = "Movie", Artwork = [], Genres = [], Tags = [], Studios = [], + Actors = [], Guids = [], Subtitles = [], Directors = [], Writers = [] + } + ], + MediaVersions = [new MediaVersion { Duration = TimeSpan.FromMinutes(30) }] + }); + + context.Collections.Add(new Collection + { + Id = id, + Name = name, + Version = version, + MediaItems = [], + CollectionItems = withMovie ? [new CollectionItem { MediaItemId = 10, CustomIndex = null }] : [] + }); + + await context.SaveChangesAsync(); + } + + private async Task ReadVersion(int id) + { + await using TvContext context = _db.CreateContext(); + return await context.Collections.Where(c => c.Id == id).Select(c => c.Version).SingleAsync(); + } + + private static (IMediaCollectionRepository repo, Channel worker, + ChannelWriter search) Deps() + { + IMediaCollectionRepository repo = Substitute.For(); + // A single playout uses the collection, so a genuine mutation enqueues exactly one rebuild. + repo.PlayoutIdsUsingCollection(Arg.Any()).Returns([1]); + Channel worker = System.Threading.Channels.Channel.CreateUnbounded(); + ChannelWriter search = + System.Threading.Channels.Channel.CreateUnbounded().Writer; + return (repo, worker, search); + } + + [Test] + public async Task AddItem_To_Collection_Should_Bump_Version_And_Enqueue_Rebuild() + { + await SeedCollection(1, version: 1, withMovie: false); + (IMediaCollectionRepository repo, Channel worker, + ChannelWriter search) = Deps(); + + var handler = new AddMovieToCollectionHandler(_db.Factory, repo, worker.Writer, search); + Either result = await handler.Handle(new AddMovieToCollection(1, 10), CancellationToken.None); + + result.IsRight.ShouldBeTrue(); + (await ReadVersion(1)).ShouldBe(2); + worker.Reader.Count.ShouldBe(1); // rebuild fanned out to the one playout using the collection + } + + [Test] + public async Task Idempotent_ReAdd_Should_Be_A_NoOp_Without_Bump_Or_Rebuild() + { + // The item is already a member — Fable D1: an unconditional bump here would make the + // SaveChanges()>0 gate always-true and fire a spurious rebuild on every duplicate add. + await SeedCollection(1, version: 1, withMovie: true); + (IMediaCollectionRepository repo, Channel worker, + ChannelWriter search) = Deps(); + + var handler = new AddMovieToCollectionHandler(_db.Factory, repo, worker.Writer, search); + Either result = await handler.Handle(new AddMovieToCollection(1, 10), CancellationToken.None); + + result.IsRight.ShouldBeTrue(); + (await ReadVersion(1)).ShouldBe(1); // no bump + worker.Reader.Count.ShouldBe(0); // no spurious rebuild + } + + [Test] + public async Task AddItem_Should_Force_Write_Past_A_Concurrent_Version_Bump() + { + await SeedCollection(1, version: 1, withMovie: false); + + // The handler's context has loaded the collection at version 1 (tracked via identity map). + TvContext handlerContext = _db.CreateContext(); + _ = await handlerContext.Collections.Include(c => c.MediaItems).SingleAsync(c => c.Id == 1); + + // A concurrent replace-all editor bumps the same row to version 2 in the DB. + await using (TvContext other = _db.CreateContext()) + { + Collection c = await other.Collections.SingleAsync(x => x.Id == 1); + c.Version++; + await other.SaveChangesAsync(); + } + + (IMediaCollectionRepository repo, Channel worker, + ChannelWriter search) = Deps(); + var handler = new AddMovieToCollectionHandler(new PreTrackedFactory(handlerContext), repo, worker.Writer, search); + + // Must not throw (plain SaveChangesAsync would 500 here); the add force-writes past the bump. + Either result = await handler.Handle(new AddMovieToCollection(1, 10), CancellationToken.None); + + result.IsRight.ShouldBeTrue(); + await using TvContext verify = _db.CreateContext(); + (await verify.CollectionItems.AnyAsync(ci => ci.CollectionId == 1 && ci.MediaItemId == 10)).ShouldBeTrue(); + } + + [Test] + public async Task RemoveItem_From_Collection_Should_Bump_Version() + { + await SeedCollection(1, version: 1, withMovie: true); + (IMediaCollectionRepository repo, Channel worker, + ChannelWriter search) = Deps(); + + var handler = new RemoveItemsFromCollectionHandler(_db.Factory, repo, worker.Writer, search); + Either result = await handler.Handle( + new RemoveItemsFromCollection(1) { MediaItemIds = [10] }, + CancellationToken.None); + + result.IsRight.ShouldBeTrue(); + (await ReadVersion(1)).ShouldBe(2); + worker.Reader.Count.ShouldBe(1); + } + + [Test] + public async Task RemoveItem_With_Nothing_To_Remove_Should_Be_A_NoOp_Without_Bump() + { + await SeedCollection(1, version: 1, withMovie: true); + (IMediaCollectionRepository repo, Channel worker, + ChannelWriter search) = Deps(); + + var handler = new RemoveItemsFromCollectionHandler(_db.Factory, repo, worker.Writer, search); + Either result = await handler.Handle( + new RemoveItemsFromCollection(1) { MediaItemIds = [] }, + CancellationToken.None); + + result.IsRight.ShouldBeTrue(); + (await ReadVersion(1)).ShouldBe(1); + worker.Reader.Count.ShouldBe(0); + } + + [Test] + public async Task UpdateCollection_Name_Change_Should_Bump_Version() + { + await SeedCollection(1, version: 1, withMovie: false, name: "Before"); + + IMediaCollectionRepository repo = Substitute.For(); + repo.PlayoutIdsUsingCollection(Arg.Any()).Returns([]); + var handler = new UpdateCollectionHandler( + _db.Factory, + repo, + System.Threading.Channels.Channel.CreateUnbounded().Writer, + Substitute.For()); + + Either result = await handler.Handle( + new UpdateCollection(1, "After"), + CancellationToken.None); + + result.IsRight.ShouldBeTrue(); + (await ReadVersion(1)).ShouldBe(2); + } + + [Test] + public async Task UpdateCollection_Same_Name_Should_Be_A_NoOp_Without_Bump() + { + await SeedCollection(1, version: 1, withMovie: false, name: "Same"); + + IMediaCollectionRepository repo = Substitute.For(); + repo.PlayoutIdsUsingCollection(Arg.Any()).Returns([]); + var handler = new UpdateCollectionHandler( + _db.Factory, + repo, + System.Threading.Channels.Channel.CreateUnbounded().Writer, + Substitute.For()); + + Either result = await handler.Handle( + new UpdateCollection(1, "Same"), + CancellationToken.None); + + result.IsRight.ShouldBeTrue(); + (await ReadVersion(1)).ShouldBe(1); + } +} diff --git a/ErsatzTV.Tests/Application/Playouts/PlayoutScheduleFileEtagRotationTests.cs b/ErsatzTV.Tests/Application/Playouts/PlayoutScheduleFileEtagRotationTests.cs new file mode 100644 index 000000000..d15b04339 --- /dev/null +++ b/ErsatzTV.Tests/Application/Playouts/PlayoutScheduleFileEtagRotationTests.cs @@ -0,0 +1,105 @@ +using System.Threading.Channels; +using ErsatzTV.Application; +using ErsatzTV.Application.Channels; +using ErsatzTV.Application.Playouts; +using ErsatzTV.Core; +using ErsatzTV.Core.Domain; +using ErsatzTV.Infrastructure.Data; +using ErsatzTV.Tests.Support; +using LanguageExt; +using Microsoft.EntityFrameworkCore; +using NUnit.Framework; +using Shouldly; + +namespace ErsatzTV.Tests.Application.Playouts; + +/// +/// #269: the Playout schedule-file writer rotates the aggregate ETag (bumps Playout.Version) +/// when the schedule-file actually changes, so an open alt-schedule/template editor's stale +/// If-Match invalidates — but a no-op re-submit (same schedule-file) must NOT bump the Version +/// or enqueue a channel refresh (the SaveChanges() > 0 side-effect gate would otherwise fire +/// spuriously once the bump makes it always-true). The bump force-writes past a concurrent replace-all +/// bump rather than 500 (this writer takes no If-Match). +/// +[TestFixture] +public class PlayoutScheduleFileEtagRotationTests +{ + private InMemoryTvContext _db = null!; + + [SetUp] + public async Task SetUp() => _db = await InMemoryTvContext.CreateAsync(); + + [TearDown] + public async Task TearDown() => await _db.DisposeAsync(); + + private async Task SeedPlayout(int id, int version, string scheduleFile) + { + await using TvContext context = _db.CreateContext(); + var channel = new ErsatzTV.Core.Domain.Channel(Guid.NewGuid()) + { + Id = 1, + Number = "1", + Name = "Channel", + Group = "", + Categories = "", + Playouts = [], + Artwork = [] + }; + context.Channels.Add(channel); + context.Playouts.Add(new Playout + { + Id = id, + ChannelId = 1, + Channel = channel, + Version = version, + ScheduleKind = PlayoutScheduleKind.Sequential, + ScheduleFile = scheduleFile, + Seed = 0, + Items = [] + }); + await context.SaveChangesAsync(); + } + + private async Task ReadVersion(int id) + { + await using TvContext context = _db.CreateContext(); + return await context.Playouts.Where(p => p.Id == id).Select(p => p.Version).SingleAsync(); + } + + private static (UpdateSequentialPlayoutHandler handler, Channel worker) MakeHandler( + IDbContextFactory factory) + { + Channel worker = System.Threading.Channels.Channel.CreateUnbounded(); + return (new UpdateSequentialPlayoutHandler(factory, worker.Writer), worker); + } + + [Test] + public async Task Changing_The_Schedule_File_Should_Bump_Version_And_Refresh() + { + await SeedPlayout(1, version: 1, scheduleFile: "before.yml"); + (UpdateSequentialPlayoutHandler handler, Channel worker) = MakeHandler(_db.Factory); + + Either result = await handler.Handle( + new UpdateSequentialPlayout(1, "after.yml"), + CancellationToken.None); + + result.IsRight.ShouldBeTrue(); + (await ReadVersion(1)).ShouldBe(2); + worker.Reader.Count.ShouldBe(1); // channel refresh enqueued + } + + [Test] + public async Task Resubmitting_The_Same_Schedule_File_Should_Be_A_NoOp_Without_Bump_Or_Refresh() + { + await SeedPlayout(1, version: 1, scheduleFile: "same.yml"); + (UpdateSequentialPlayoutHandler handler, Channel worker) = MakeHandler(_db.Factory); + + Either result = await handler.Handle( + new UpdateSequentialPlayout(1, "same.yml"), + CancellationToken.None); + + result.IsRight.ShouldBeTrue(); + (await ReadVersion(1)).ShouldBe(1); // no bump + worker.Reader.Count.ShouldBe(0); // no spurious refresh + } +} diff --git a/docs/api-conventions.md b/docs/api-conventions.md index c61b24f7f..fe94ade3e 100644 --- a/docs/api-conventions.md +++ b/docs/api-conventions.md @@ -467,12 +467,24 @@ config is correct. (2) **Item-add force-write can leave a duplicate/gap `Index`* 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 -doesn't rotate an open editor's ETag. That is an invalidation-completeness refinement — not a lost-update or a -500; the primary endpoints' own bump+guard fully cover the two-tab lost-update this contract targets. +candidate. + +**Cross-editor ETag rotation — completed (#269).** The remaining non-bumping config siblings now rotate the +aggregate ETag so a concurrent editor of the *same root* invalidates on its next save: the Collection +`Add*ToCollection` family (all 11 handlers) and `RemoveItemsFromCollectionHandler` bump `Collection.Version`, +and `UpdateCollectionHandler` (name/flag), `UpdatePlayoutHandler` (`DailyRebuildTime`), and the three +`ScheduleFile` writers — which already force-wrote — now also bump. All rotate via `SaveChangesForcingVersion` +(they take no `If-Match`, so a concurrent bump force-writes, never 412/500). **Correction:** the +`Add*ToCollection` family is *not* repository-mediated — each handler loads the `Collection` into its own +`dbContext` and writes directly (`IMediaCollectionRepository` is read-only), so the rotation bump is purely an +API-layer concern and the scanner's separate membership-write path is unaffected (a background scan does not +rotate the editor ETag). **No-op idempotence (the trap):** these handlers gate their reindex/`BuildPlayout` +fan-out on `SaveChanges() > 0`; an *unconditional* bump makes that gate always-true, so an idempotent re-add / +same-value re-submit would fire spurious rebuilds. Each therefore short-circuits a genuine no-op **before** the +bump — the Add handlers by an explicit membership check (also fixing a latent duplicate-`CollectionItem` +insert), the scalar writers by `ChangeTracker.HasChanges()` — so a no-op neither bumps nor rebuilds. This is an +invalidation-completeness refinement; the primary endpoints' own bump+guard already covered the two-tab +lost-update the contract targets. ## 7b. Post-commit side effects run on `CancellationToken.None` diff --git a/docs/decisions.md b/docs/decisions.md index 955f66b73..e414412ba 100644 --- a/docs/decisions.md +++ b/docs/decisions.md @@ -75,6 +75,7 @@ keep append-only from accreting stale, contradictory, or unreadably-large histor - [2026-07-12 — Blocking CI gate for API-contract artifacts (#303 H4/H5)](#2026-07-12--blocking-ci-gate-for-api-contract-artifacts-303-h45) - [2026-07-12 — decisions.md is append-only, enforced; root-screenshot guard (#303 H9/H3)](#2026-07-12--decisionsmd-is-append-only-enforced-root-screenshot-guard-303-h9h3) - [2026-07-12 — Review-verdict merge-gate: latest commit must be reviewed (#303 H10)](#2026-07-12--review-verdict-merge-gate-latest-commit-must-be-reviewed-303-h10) +- [2026-07-12 — Cross-editor ETag rotation completed for Collection/Playout config siblings (#269)](#2026-07-12--cross-editor-etag-rotation-completed-for-collectionplayout-config-siblings-269) --- @@ -1409,3 +1410,46 @@ Scope: the Claude PreToolUse gate on the Gitea merge tool only. A direct `git pu PR comments to check, so the `.husky/pre-push` backstop is not extended for H10 (the merge tool is the real merge path; docs-only PRs remain exempt via H6's file-set exemption). Rationale, as with the whole Wave-1/2/3 hook set: make the process rule a derivation/hook, not prose to remember (#303). +--- + +## 2026-07-12 — Cross-editor ETag rotation completed for Collection/Playout config siblings (#269) + +The #253 optimistic-concurrency contract (§7a) had a documented tail: the non-If-Match config-sibling +writers of a versioned root mutated editor-visible state **without** bumping `Version`, so editing through +them did not rotate an open editor's ETag (a cross-editor invalidation gap — never a lost-update or a 500, +which the primary endpoints' bump+guard already cover). #269's first slice (PR #302) removed the 500 exposure +by routing those writers through `SaveChangesForcingVersion`; this slice completes the **rotation**. + +Handlers now bumping `Version` (all via `SaveChangesForcingVersion`, since they take no `If-Match` → a +concurrent replace-all bump force-writes, never 412/500): the Collection `Add*ToCollection` family (11 +handlers) and `RemoveItemsFromCollectionHandler` bump `Collection.Version`; `UpdateCollectionHandler` +(name/flag), `UpdatePlayoutHandler` (`DailyRebuildTime`), and the three `ScheduleFile` writers +(`UpdateSequential`/`UpdateScripted`/`UpdateExternalJsonPlayout`) — which already force-wrote — now also bump. + +Decisions frozen (ratified with Fable before implementation, feeding the #197 contract freeze): + +- **Rotate on every editor-visible config change, no per-aggregate carve-outs.** §7a's config-only boundary + ("every mutating handler of a root's editor-visible config bumps `Version`") already held for Playlist + `Add*`/schedule item writers; the Collection/Playout siblings were an inconsistency, not a judgment call. A + membership add rotating an open custom-order editor's ETag (→ 412 → reload) is correct: its list is genuinely + stale. Blast radius of the aggressive-but-safe rotation is a reload, never data loss. +- **No-op idempotence — the trap Fable caught.** These handlers gate their reindex/`BuildPlayout` fan-out on + `SaveChanges() > 0`. An *unconditional* bump makes that gate always-true, so an idempotent re-add / same-value + re-submit would fire spurious rebuilds across every playout using the aggregate. Fix: short-circuit a genuine + no-op **before** the bump — the Add handlers by an explicit membership check (which also fixes a latent + duplicate-`CollectionItem` insert on a re-add), the scalar writers (`UpdateCollection`, `UpdatePlayout`, the + three `ScheduleFile` writers) by `ChangeTracker.HasChanges()`. A no-op neither bumps nor rebuilds nor rotates + the ETag — which is itself correct (nothing changed). +- **The `Add*ToCollection` family is not repository-mediated.** #269's original framing ("repository-mediated, + shared with the scanner hot path") was wrong: `IMediaCollectionRepository` is read-only; each handler loads + the `Collection` into its own `dbContext` and writes directly. So the rotation bump is a pure API-layer + concern and the scanner's separate membership-write path is untouched — a background scan does **not** rotate + the editor ETag (correct: background indexing is not an editor action). +- **No new status codes.** These endpoints take no `If-Match` and force-write, so they never 412; no + `[ProducesResponseType(...412...)]` and no OpenAPI regen (response types unchanged). Only §7a prose changes. + +Tests: `CollectionEtagRotationTests` (rotation + no-op-without-bump-or-rebuild + force-write-past-concurrent-bump +for Add/Remove/Update) and `PlayoutScheduleFileEtagRotationTests` (ScheduleFile rotation + no-op-without-refresh), +the no-op guard proven non-vacuous by inverting the membership check. The `#265` RFC-7232 If-Match parser +refinement (valid-but-non-matching/weak/list → 412 not 400) is a **separate** PR (disjoint surface: the shared +parser + `CheckVersion`, not the handler saves). Refs #253 #269 #197 · `api-conventions.md` §7a. -- 2.47.3 From 7a9b30de714d87d102dd8cdbb96f00fa748e9a49 Mon Sep 17 00:00:00 2001 From: Timothy Date: Sun, 12 Jul 2026 19:22:08 +0200 Subject: [PATCH 2/2] =?UTF-8?q?fix(api):=20#269=20review=20fixes=20?= =?UTF-8?q?=E2=80=94=20rebase=20force-write=20delta=20so=20rotation=20surv?= =?UTF-8?q?ives=20a=20race=20(Codex=20F1/F3)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Independent Codex review (reconciled by Fable against a MERGEABLE fork verdict) found SaveChangesForcingVersion silently DROPPED a pending Version++ under a concurrent versioned-write race: on DbUpdateConcurrencyException it adopted the DB's current Version verbatim (original = current = dbVersion), so a bumping sibling committed at dbVersion instead of dbVersion+1. Net: an editor holding the concurrent writer's ETag was never invalidated by the sibling's change — the exact lost-update the #253/#269 contract exists to close, lost under the very condition the helper handles. F1 fix (shared helper, corrects all 25 bumpers incl. the pre-existing Add*ToPlaylist / schedule-item writers): rebase the pending delta on top of the stored token — pendingDelta = current - original; original = dbVersion; current = dbVersion + pendingDelta Bumpers (delta 1) advance to dbVersion+1; non-bumpers/deletes (delta 0, e.g. ErasePlayoutHistory) still adopt the stored token unchanged, so RootWriterForceVersionTests is unaffected. Idempotent across the bounded retry loop. F3: the force-race tests now assert Version==3 (rebase), not just membership survival; added the missing Playout force-race+rotate test. Negative-controlled: with the helper fix reverted, both strengthened tests go red. F2 (Medium, deferred → #308): two concurrent same-item Add*ToCollection can both pass the membership check and the loser 500s on the composite-PK violation (DbUpdateException, which the helper doesn't catch). Pre-existing and narrow (no corruption); doc claims softened to name it. Filed #308. Docs: api-conventions §7a + decisions.md prose corrected from "adopt the stored token" to the rebase semantics. Co-Authored-By: Claude Opus 4.8 (1M context) --- ErsatzTV.Application/ConcurrencyExtensions.cs | 20 +++++++--- .../CollectionEtagRotationTests.cs | 5 +++ .../PlayoutScheduleFileEtagRotationTests.cs | 39 +++++++++++++++++++ docs/api-conventions.md | 12 ++++-- docs/decisions.md | 16 +++++++- 5 files changed, 80 insertions(+), 12 deletions(-) diff --git a/ErsatzTV.Application/ConcurrencyExtensions.cs b/ErsatzTV.Application/ConcurrencyExtensions.cs index cd64f2d03..e98cdabd9 100644 --- a/ErsatzTV.Application/ConcurrencyExtensions.cs +++ b/ErsatzTV.Application/ConcurrencyExtensions.cs @@ -15,9 +15,10 @@ public static class ConcurrencyExtensions /// UPDATE of that row with WHERE Version=@orig, so a concurrent bump from a replace-all /// editor would otherwise surface as an unhandled → /// 500 (issue #253 / #269). Phase-1 semantics for a missing If-Match is force-write, - /// so on a concurrency failure we adopt the stored token as both original (the retry's WHERE then - /// matches) and current (so we don't revert the concurrent bump) and retry — a client-wins merge - /// scoped to the token; our own modified scalars still win. Bounded to avoid a livelock; if the row + /// so on a concurrency failure we rebase onto the stored token: original becomes the stored value + /// (the retry's WHERE then matches) and current becomes stored + our pending delta (a bumper's ++ + /// still advances the ETag past the concurrent writer's value — #269 rotation; a non-bumper adopts + /// it unchanged) and retry; our own modified scalars still win. Bounded to avoid a livelock; if the row /// was deleted out from under us, that's a genuine conflict and rethrows. /// public static async Task SaveChangesForcingVersion( @@ -48,9 +49,16 @@ public static class ConcurrencyExtensions } PropertyEntry version = entry.Property(nameof(IVersionedAggregate.Version)); - object currentVersion = databaseValues[nameof(IVersionedAggregate.Version)]!; - version.OriginalValue = currentVersion; - version.CurrentValue = currentVersion; + int dbVersion = (int)databaseValues[nameof(IVersionedAggregate.Version)]!; + + // Rebase our pending delta on top of the stored token instead of adopting it verbatim: + // a Version-bumping sibling (pending current = original + 1) must still advance the + // ETag PAST the concurrent writer's value, or an editor holding that writer's ETag is + // never invalidated by our change (#269 rotation silently lost under race). Non-bumpers + // (delta 0, e.g. ErasePlayoutHistory) still adopt the stored token unchanged. + int pendingDelta = (int)version.CurrentValue! - (int)version.OriginalValue!; + version.OriginalValue = dbVersion; + version.CurrentValue = dbVersion + pendingDelta; resolvedAny = true; } diff --git a/ErsatzTV.Tests/Application/MediaCollections/CollectionEtagRotationTests.cs b/ErsatzTV.Tests/Application/MediaCollections/CollectionEtagRotationTests.cs index bfb6b1a8f..6d5abb8eb 100644 --- a/ErsatzTV.Tests/Application/MediaCollections/CollectionEtagRotationTests.cs +++ b/ErsatzTV.Tests/Application/MediaCollections/CollectionEtagRotationTests.cs @@ -163,6 +163,11 @@ public class CollectionEtagRotationTests result.IsRight.ShouldBeTrue(); await using TvContext verify = _db.CreateContext(); (await verify.CollectionItems.AnyAsync(ci => ci.CollectionId == 1 && ci.MediaItemId == 10)).ShouldBeTrue(); + + // The force-write must rebase our bump ON TOP of the concurrent bump (1 →(other) 2 →(ours) 3), not + // adopt the stored token verbatim — otherwise an editor holding the concurrent writer's ETag ("2") + // is never invalidated by this add and the #269 rotation is silently lost under race. + (await ReadVersion(1)).ShouldBe(3); } [Test] diff --git a/ErsatzTV.Tests/Application/Playouts/PlayoutScheduleFileEtagRotationTests.cs b/ErsatzTV.Tests/Application/Playouts/PlayoutScheduleFileEtagRotationTests.cs index d15b04339..1501e8e87 100644 --- a/ErsatzTV.Tests/Application/Playouts/PlayoutScheduleFileEtagRotationTests.cs +++ b/ErsatzTV.Tests/Application/Playouts/PlayoutScheduleFileEtagRotationTests.cs @@ -102,4 +102,43 @@ public class PlayoutScheduleFileEtagRotationTests (await ReadVersion(1)).ShouldBe(1); // no bump worker.Reader.Count.ShouldBe(0); // no spurious refresh } + + private sealed class PreTrackedFactory(TvContext context) : IDbContextFactory + { + public TvContext CreateDbContext() => context; + } + + [Test] + public async Task Schedule_File_Change_Should_Force_Write_Past_A_Concurrent_Version_Bump_And_Still_Rotate() + { + await SeedPlayout(1, version: 1, scheduleFile: "before.yml"); + + // The handler's context has loaded the playout at version 1 (tracked via identity map). + TvContext handlerContext = _db.CreateContext(); + _ = await handlerContext.Playouts.SingleAsync(p => p.Id == 1); + + // A concurrent replace-all editor bumps the same row to version 2 in the DB. + await using (TvContext other = _db.CreateContext()) + { + Playout p = await other.Playouts.SingleAsync(x => x.Id == 1); + p.Version++; + await other.SaveChangesAsync(); + } + + (UpdateSequentialPlayoutHandler handler, Channel worker) = + MakeHandler(new PreTrackedFactory(handlerContext)); + + // Must not throw (plain SaveChangesAsync would 500 here) … + Either result = await handler.Handle( + new UpdateSequentialPlayout(1, "after.yml"), + CancellationToken.None); + + result.IsRight.ShouldBeTrue(); + + // … and the force-write must rebase our bump ON TOP of the concurrent bump (1 →(other) 2 →(ours) 3): + // adopting the stored token verbatim would leave the concurrent writer's ETag ("2") valid and the + // #269 rotation silently lost under race. + (await ReadVersion(1)).ShouldBe(3); + worker.Reader.Count.ShouldBe(1); // the genuine change still refreshes the channel + } } diff --git a/docs/api-conventions.md b/docs/api-conventions.md index fe94ade3e..68f790d05 100644 --- a/docs/api-conventions.md +++ b/docs/api-conventions.md @@ -446,8 +446,10 @@ exception, so it needs no guard). makes EF append `WHERE Version=@orig` to *every* UPDATE **and DELETE** of the root — so a plain `SaveChangesAsync` writer that is *not* part of the If-Match contract throws an unhandled `DbUpdateConcurrencyException`→**500** when a replace-all editor bumps the row in its load→save window. Every -such writer now saves through `SaveChangesForcingVersion(ct)` (Phase-1 force-write: adopt the stored token and -retry; rethrow only if the row was genuinely deleted out from under it). **The exposure filter is "any +such writer now saves through `SaveChangesForcingVersion(ct)` (Phase-1 force-write: rebase onto the stored +token — original = stored, current = stored + the pending delta, so a `Version`-bumper's rotation still +advances the ETag *past* the concurrent writer's value instead of silently adopting it — and retry; rethrow +only if the row was genuinely deleted out from under it). **The exposure filter is "any handler that leaves a versioned root `Modified` or `Deleted` on plain `SaveChangesAsync`", NOT just `Version`-bumpers + deletes** — `ErasePlayoutHistoryHandler` modifies Playout scalars *without* bumping and is exposed too. Covered (18 writers): the **9 aggregate delete handlers** (a delete has no ETag to rotate, so @@ -481,8 +483,10 @@ API-layer concern and the scanner's separate membership-write path is unaffected rotate the editor ETag). **No-op idempotence (the trap):** these handlers gate their reindex/`BuildPlayout` fan-out on `SaveChanges() > 0`; an *unconditional* bump makes that gate always-true, so an idempotent re-add / same-value re-submit would fire spurious rebuilds. Each therefore short-circuits a genuine no-op **before** the -bump — the Add handlers by an explicit membership check (also fixing a latent duplicate-`CollectionItem` -insert), the scalar writers by `ChangeTracker.HasChanges()` — so a no-op neither bumps nor rebuilds. This is an +bump — the Add handlers by an explicit membership check (also fixing the latent duplicate-`CollectionItem` +insert on a *sequential* re-add; two *concurrent* adds of the same item can still both pass the check and the +loser 500s on the composite-PK violation — a narrow, pre-existing race, tracked as #308), the scalar +writers by `ChangeTracker.HasChanges()` — so a no-op neither bumps nor rebuilds. This is an invalidation-completeness refinement; the primary endpoints' own bump+guard already covered the two-tab lost-update the contract targets. diff --git a/docs/decisions.md b/docs/decisions.md index e414412ba..3799bdcf6 100644 --- a/docs/decisions.md +++ b/docs/decisions.md @@ -1436,8 +1436,11 @@ Decisions frozen (ratified with Fable before implementation, feeding the #197 co - **No-op idempotence — the trap Fable caught.** These handlers gate their reindex/`BuildPlayout` fan-out on `SaveChanges() > 0`. An *unconditional* bump makes that gate always-true, so an idempotent re-add / same-value re-submit would fire spurious rebuilds across every playout using the aggregate. Fix: short-circuit a genuine - no-op **before** the bump — the Add handlers by an explicit membership check (which also fixes a latent - duplicate-`CollectionItem` insert on a re-add), the scalar writers (`UpdateCollection`, `UpdatePlayout`, the + no-op **before** the bump — the Add handlers by an explicit membership check (which also fixes the latent + duplicate-`CollectionItem` insert on a *sequential* re-add; two *concurrent* same-item adds can still both + pass the check and the loser 500s on the composite-PK unique violation — `SaveChangesForcingVersion` catches + only `DbUpdateConcurrencyException`, not `DbUpdateException`. That race is narrow and pre-existing, deferred + to #308), the scalar writers (`UpdateCollection`, `UpdatePlayout`, the three `ScheduleFile` writers) by `ChangeTracker.HasChanges()`. A no-op neither bumps nor rebuilds nor rotates the ETag — which is itself correct (nothing changed). - **The `Add*ToCollection` family is not repository-mediated.** #269's original framing ("repository-mediated, @@ -1445,6 +1448,15 @@ Decisions frozen (ratified with Fable before implementation, feeding the #197 co the `Collection` into its own `dbContext` and writes directly. So the rotation bump is a pure API-layer concern and the scanner's separate membership-write path is untouched — a background scan does **not** rotate the editor ETag (correct: background indexing is not an editor action). +- **Force-write rebases the bump, never adopts the stored token verbatim (Codex review of this PR).** + `SaveChangesForcingVersion` originally resolved a conflict by setting current=original=stored — which + silently *discarded* a sibling's pending `Version++` when a versioned writer committed in its load→save + window (sibling loads 1, bumps to pending 2, concurrent PUT commits 2 → retry wrote 2, so the concurrent + writer's ETag "2" stayed valid and the rotation was lost under exactly the race it exists for). Fixed in + this PR (it affects all 25 bumpers routed through the helper, including the pre-existing playlist/schedule + ones): the retry now rebases — original = stored, current = stored + (pending current − pending original) — + so a bumper lands at stored+1 and a non-bumper (delta 0, e.g. `ErasePlayoutHistory`) adopts stored unchanged. + The race tests assert the post-race Version (3, not 2) and fail against the verbatim-adopt implementation. - **No new status codes.** These endpoints take no `If-Match` and force-write, so they never 412; no `[ProducesResponseType(...412...)]` and no OpenAPI regen (response types unchanged). Only §7a prose changes. -- 2.47.3