From 0f34c86afa4633f556a02c65e31f59cae62f8632 Mon Sep 17 00:00:00 2001 From: Timothy Date: Sat, 18 Jul 2026 18:23:21 +0200 Subject: [PATCH 1/2] feat(403): make unsupported PlaybackOrder loud at build time + tripwire MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adding a new PlaybackOrder was unsafe by construction: three build-time dispatch sites turned an unknown value into an enumerator silently. Classic substituted RandomizedMediaCollectionEnumerator (the // TODO default arm), PlaylistEnumerator had no default arm so the item was dropped, and BlockPlayoutBuilder's allow-list continue skipped it. (#70 already made YAML/Scripted log a warning and MultiCollectionGroup throws.) - each silent site now logs a Warning naming the order + engine + the fallback taken; the fallback itself is preserved so a live channel never goes dark on one misconfigured item and scheduler goldens do not move. - PlaylistEnumerator.Create gained an optional Option (it was static with no logger -- why the drop was unreportable); loggered callers pass it. - BlockPlayoutBuilder gained an explicit Random arm (it previously reached an enumerator only via the coincidental _ => fallback) and a loud defensive fallback. - new PlaybackOrderSupport matrix (per SchedulingEngineKind) + tripwire PlaybackOrderSupportTests: Supported ∪ Unsupported must partition the enum for every engine, so a new order fails the test until classified. BlockPlayoutBuilder consumes the matrix for its allow-list. - write-path rejection left unchanged (#70 closed the persistence hole; the perimeter has been wrong three times per decisions.md); reverse _ => None mappings reviewed and deferred (different axis; making them loud would warn on legit enumerator types). docs/decisions.md updated. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../Scheduling/PlaybackOrderSupportTests.cs | 79 +++++++++++ .../Scheduling/PlaylistEnumeratorTests.cs | 85 +++++++++++ .../BlockScheduling/BlockPlayoutBuilder.cs | 38 +++-- .../Scheduling/Engine/SchedulingEngine.cs | 3 +- .../Scheduling/PlaybackOrderSupport.cs | 134 ++++++++++++++++++ .../Scheduling/PlaylistEnumerator.cs | 17 ++- ErsatzTV.Core/Scheduling/PlayoutBuilder.cs | 10 +- .../Scheduling/SchedulingEngineKind.cs | 23 +++ .../YamlScheduling/EnumeratorCache.cs | 3 +- docs/decisions.md | 47 ++++++ 10 files changed, 422 insertions(+), 17 deletions(-) create mode 100644 ErsatzTV.Core.Tests/Scheduling/PlaybackOrderSupportTests.cs create mode 100644 ErsatzTV.Core/Scheduling/PlaybackOrderSupport.cs create mode 100644 ErsatzTV.Core/Scheduling/SchedulingEngineKind.cs diff --git a/ErsatzTV.Core.Tests/Scheduling/PlaybackOrderSupportTests.cs b/ErsatzTV.Core.Tests/Scheduling/PlaybackOrderSupportTests.cs new file mode 100644 index 000000000..541e9da47 --- /dev/null +++ b/ErsatzTV.Core.Tests/Scheduling/PlaybackOrderSupportTests.cs @@ -0,0 +1,79 @@ +using ErsatzTV.Core.Domain; +using ErsatzTV.Core.Scheduling; +using NUnit.Framework; +using Shouldly; + +namespace ErsatzTV.Core.Tests.Scheduling; + +[TestFixture] +public class PlaybackOrderSupportTests +{ + private static readonly PlaybackOrder[] AllOrders = Enum.GetValues(); + + // The tripwire (#403): every engine must classify every PlaybackOrder value as either supported or + // explicitly unsupported. Adding a new order without classifying it here fails this test, which forces the + // author to wire it into (or deliberately reject it from) each dispatch site instead of letting it degrade + // silently. + [Test] + public void EveryOrder_IsClassified_ForEveryEngine() + { + foreach (SchedulingEngineKind engine in PlaybackOrderSupport.Engines) + { + foreach (PlaybackOrder order in AllOrders) + { + PlaybackOrderSupport.IsClassified(engine, order).ShouldBeTrue( + $"PlaybackOrder.{order} is not classified for {engine}. Add it to PlaybackOrderSupport " + + "(supported or unsupported) AND wire it into that engine's dispatch switch (#403)."); + } + } + } + + // The two sets must partition the enum: no order both supported and unsupported, and together they cover + // exactly the enum (no stale entry for a removed value, no missing value). + [Test] + public void SupportedAndUnsupported_ArePartition_ForEveryEngine() + { + var all = AllOrders.ToHashSet(); + + foreach (SchedulingEngineKind engine in PlaybackOrderSupport.Engines) + { + IReadOnlySet supported = PlaybackOrderSupport.SupportedBy(engine); + IReadOnlySet unsupported = PlaybackOrderSupport.UnsupportedBy(engine); + + supported.Intersect(unsupported).ShouldBeEmpty( + $"{engine}: an order is listed as both supported and unsupported"); + + var union = supported.Concat(unsupported).ToHashSet(); + union.ShouldBe(all, ignoreOrder: true, + $"{engine}: supported ∪ unsupported does not equal the PlaybackOrder enum"); + } + } + + // Guards the specific fragility called out in #403: Random is in Block's allow-list, so it must be + // supported by Block (it previously worked only via the switch's coincidental Random fallback). + [Test] + public void Block_Supports_Random() + { + PlaybackOrderSupport.IsSupported(SchedulingEngineKind.Block, PlaybackOrder.Random).ShouldBeTrue(); + } + + // WeightedShuffle (#70) is Classic-only; the other engines must classify it as unsupported so the + // write-path guards and this matrix agree. + [Test] + public void WeightedShuffle_IsClassicOnly() + { + PlaybackOrderSupport.IsSupported(SchedulingEngineKind.Classic, PlaybackOrder.WeightedShuffle) + .ShouldBeTrue(); + + foreach (SchedulingEngineKind engine in PlaybackOrderSupport.Engines) + { + if (engine == SchedulingEngineKind.Classic) + { + continue; + } + + PlaybackOrderSupport.IsSupported(engine, PlaybackOrder.WeightedShuffle).ShouldBeFalse( + $"{engine} must not support WeightedShuffle (#70 is Classic-only)"); + } + } +} diff --git a/ErsatzTV.Core.Tests/Scheduling/PlaylistEnumeratorTests.cs b/ErsatzTV.Core.Tests/Scheduling/PlaylistEnumeratorTests.cs index 78ccf74ed..05e3958b4 100644 --- a/ErsatzTV.Core.Tests/Scheduling/PlaylistEnumeratorTests.cs +++ b/ErsatzTV.Core.Tests/Scheduling/PlaylistEnumeratorTests.cs @@ -1,6 +1,7 @@ using ErsatzTV.Core.Domain; using ErsatzTV.Core.Interfaces.Repositories; using ErsatzTV.Core.Scheduling; +using Microsoft.Extensions.Logging; using NSubstitute; using NUnit.Framework; using Shouldly; @@ -433,6 +434,67 @@ public class PlaylistEnumeratorTests items.ShouldBe([11, 12, 10, 21, 22, 20, 12, 10, 11, 22, 20, 21]); } + // #403: an order the playlist engine doesn't handle must be dropped LOUDLY (a warning), not silently. + [Test] + public async Task Test_UnsupportedOrder_Drops_Item_And_Logs_Warning() + { + IMediaCollectionRepository repo = Substitute.For(); + var logger = new RecordingLogger(); + + var playlistItemMap = new Dictionary> + { + { + new PlaylistItem + { + Id = 1, + PlaybackOrder = PlaybackOrder.Chronological, + PlayAll = false, + CollectionType = CollectionType.Collection, + CollectionId = 1 + }, + [FakeMovie(10), FakeMovie(11)] + }, + { + // WeightedShuffle (#70) is Classic-only; the playlist switch has no arm for it. + new PlaylistItem + { + Id = 2, + PlaybackOrder = PlaybackOrder.WeightedShuffle, + PlayAll = false, + CollectionType = CollectionType.Collection, + CollectionId = 2 + }, + [FakeMovie(20), FakeMovie(21)] + } + }; + + PlaylistEnumerator enumerator = await PlaylistEnumerator.Create( + repo, + playlistItemMap, + new CollectionEnumeratorState(), + shufflePlaylistItems: false, + batchSize: Option.None, + CancellationToken.None, + logger); + + // the unsupported item (20, 21) is dropped; only the chronological item (10, 11) cycles + var items = new List(); + for (var i = 0; i < 4; i++) + { + items.AddRange(enumerator.Current.Map(mi => mi.Id)); + enumerator.MoveNext(Option.None); + } + + items.ShouldContain(10); + items.ShouldContain(11); + items.ShouldNotContain(20); + items.ShouldNotContain(21); + + // and it said so, rather than dropping silently + logger.Entries.ShouldContain( + e => e.Level == LogLevel.Warning && e.Message.Contains("not supported by playlist")); + } + private static Movie FakeMovie(int id) => new() { Id = id, @@ -445,4 +507,27 @@ public class PlaylistEnumeratorTests } ] }; + + private sealed class RecordingLogger : ILogger + { + public List<(LogLevel Level, string Message)> Entries { get; } = []; + + public IDisposable BeginScope(TState state) where TState : notnull => NullScope.Instance; + + public bool IsEnabled(LogLevel logLevel) => true; + + public void Log( + LogLevel logLevel, + EventId eventId, + TState state, + Exception exception, + Func formatter) => + Entries.Add((logLevel, formatter(state, exception))); + + private sealed class NullScope : IDisposable + { + public static readonly NullScope Instance = new(); + public void Dispose() { } + } + } } diff --git a/ErsatzTV.Core/Scheduling/BlockScheduling/BlockPlayoutBuilder.cs b/ErsatzTV.Core/Scheduling/BlockScheduling/BlockPlayoutBuilder.cs index aa79c8555..a14990848 100644 --- a/ErsatzTV.Core/Scheduling/BlockScheduling/BlockPlayoutBuilder.cs +++ b/ErsatzTV.Core/Scheduling/BlockScheduling/BlockPlayoutBuilder.cs @@ -51,15 +51,6 @@ public class BlockPlayoutBuilder( referenceData.Channel.Number, referenceData.Channel.Name); - List allowedPlaybackOrders = - [ - PlaybackOrder.Chronological, - PlaybackOrder.SeasonEpisode, - PlaybackOrder.Shuffle, - PlaybackOrder.Random, - PlaybackOrder.RandomRotation - ]; - int daysToBuild = await GetDaysToBuild(cancellationToken); // get blocks to schedule @@ -163,8 +154,14 @@ public class BlockPlayoutBuilder( foreach (BlockItem blockItem in effectiveBlock.Block.Items.OrderBy(i => i.Index)) { // TODO: support other playback orders - if (!allowedPlaybackOrders.Contains(blockItem.PlaybackOrder)) + if (!PlaybackOrderSupport.IsSupported(SchedulingEngineKind.Block, blockItem.PlaybackOrder)) { + // Skipping the item silently means it never airs and nothing says why (#403). + logger.LogWarning( + "Playback order {PlaybackOrder} is not supported by block scheduling; " + + "block item {BlockItemId} will be skipped", + blockItem.PlaybackOrder, + blockItem.Id); continue; } @@ -394,14 +391,31 @@ public class BlockPlayoutBuilder( referenceData.PlayoutHistory.Append(result.AddedHistory).ToList(), blockItem, historyKey), - _ => new RandomizedMediaCollectionEnumerator( + // Random is in Block's allow-list; give it an explicit arm rather than relying on the fallback + // below (it previously worked only by coincidence -- #403). + PlaybackOrder.Random => new RandomizedMediaCollectionEnumerator( collectionItems, - new CollectionEnumeratorState { Seed = new Random().Next(), Index = 0 }) + new CollectionEnumeratorState { Seed = new Random().Next(), Index = 0 }), + _ => UnsupportedBlockOrderFallback(blockItem.PlaybackOrder, collectionItems) }; return enumerator; } + // Defensive: the allow-list in Build already skips unsupported orders, so this should be unreachable. If a + // supported-but-unhandled order ever lands here, be loud instead of silently rotating Random (#403). + private IMediaCollectionEnumerator UnsupportedBlockOrderFallback( + PlaybackOrder playbackOrder, + List collectionItems) + { + logger.LogWarning( + "Playback order {PlaybackOrder} reached the block enumerator without a handler; falling back to Random", + playbackOrder); + return new RandomizedMediaCollectionEnumerator( + collectionItems, + new CollectionEnumeratorState { Seed = new Random().Next(), Index = 0 }); + } + private static PlayoutBuildResult CleanUpHistory( PlayoutReferenceData referenceData, DateTimeOffset start, diff --git a/ErsatzTV.Core/Scheduling/Engine/SchedulingEngine.cs b/ErsatzTV.Core/Scheduling/Engine/SchedulingEngine.cs index 359ffdf24..9a29ddcd9 100644 --- a/ErsatzTV.Core/Scheduling/Engine/SchedulingEngine.cs +++ b/ErsatzTV.Core/Scheduling/Engine/SchedulingEngine.cs @@ -257,7 +257,8 @@ public class SchedulingEngine( state, shufflePlaylistItems: false, batchSize: Option.None, - CancellationToken.None); + CancellationToken.None, + Optional((ILogger)logger)); string historyKey = HistoryDetails.KeyForSchedulingContent(key, PlaybackOrder.None); var details = new EnumeratorDetails(enumerator, historyKey, PlaybackOrder.None); diff --git a/ErsatzTV.Core/Scheduling/PlaybackOrderSupport.cs b/ErsatzTV.Core/Scheduling/PlaybackOrderSupport.cs new file mode 100644 index 000000000..c966c99cf --- /dev/null +++ b/ErsatzTV.Core/Scheduling/PlaybackOrderSupport.cs @@ -0,0 +1,134 @@ +using ErsatzTV.Core.Domain; +using OrderSet = System.Collections.Generic.HashSet; + +namespace ErsatzTV.Core.Scheduling; + +/// +/// The scheduling engine families that turn a into an enumerator, and which +/// orders each one actually handles. This is the single declared support matrix for #403. +/// +/// +/// Each engine keeps BOTH a Supported and an Unsupported set, maintained by hand ON PURPOSE: +/// PlaybackOrderSupportTests asserts the two sets partition every value +/// (union is total, intersection empty), so adding a new order fails that test until it is consciously +/// classified here. Deriving Unsupported as "everything not supported" would let a new order fall +/// through silently — the very defect #403 exists to kill — so it is spelled out instead. +/// +/// Membership here mirrors the executable dispatch in each builder (PlayoutBuilder, +/// PlaylistEnumerator, BlockPlayoutBuilder, EnumeratorCache [YAML], +/// SchedulingEngine.EnumeratorForContent [Scripted]); the builders remain the real logic. Only +/// consumes this table at runtime (its allow-list), so the table +/// is not merely test scaffolding. When you add a case to one of those switches, update the matching set +/// here. +/// +public static class PlaybackOrderSupport +{ + private sealed record EngineSupport( + IReadOnlySet Supported, + IReadOnlySet Unsupported); + + private static readonly IReadOnlyDictionary Matrix = + new Dictionary + { + // PlayoutBuilder.GetMediaCollectionEnumerator switch; default arm falls back to Random (now loud). + [SchedulingEngineKind.Classic] = new EngineSupport( + new OrderSet + { + PlaybackOrder.Chronological, + PlaybackOrder.SeasonEpisode, + PlaybackOrder.Random, + PlaybackOrder.Shuffle, + PlaybackOrder.ShuffleInOrder, + PlaybackOrder.MultiEpisodeShuffle, + PlaybackOrder.Marathon, + PlaybackOrder.WeightedShuffle + }, + new OrderSet { PlaybackOrder.None, PlaybackOrder.RandomRotation }), + + // PlaylistEnumerator.Create switch; no default arm meant a null enumerator -> silent drop (now loud). + [SchedulingEngineKind.Playlist] = new EngineSupport( + new OrderSet + { + PlaybackOrder.Chronological, + PlaybackOrder.SeasonEpisode, + PlaybackOrder.Random, + PlaybackOrder.Shuffle, + PlaybackOrder.ShuffleInOrder, + PlaybackOrder.MultiEpisodeShuffle + }, + new OrderSet + { + PlaybackOrder.None, + PlaybackOrder.RandomRotation, + PlaybackOrder.Marathon, + PlaybackOrder.WeightedShuffle + }), + + // BlockPlayoutBuilder allow-list (this very set) + the GetEnumerator switch. + [SchedulingEngineKind.Block] = new EngineSupport( + new OrderSet + { + PlaybackOrder.Chronological, + PlaybackOrder.SeasonEpisode, + PlaybackOrder.Shuffle, + PlaybackOrder.Random, + PlaybackOrder.RandomRotation + }, + new OrderSet + { + PlaybackOrder.None, + PlaybackOrder.ShuffleInOrder, + PlaybackOrder.MultiEpisodeShuffle, + PlaybackOrder.Marathon, + PlaybackOrder.WeightedShuffle + }), + + // EnumeratorCache (YAML, non-playlist path); unsupported orders LogWarning + None. + [SchedulingEngineKind.Yaml] = new EngineSupport( + new OrderSet { PlaybackOrder.Chronological, PlaybackOrder.Shuffle }, + new OrderSet + { + PlaybackOrder.None, + PlaybackOrder.Random, + PlaybackOrder.SeasonEpisode, + PlaybackOrder.ShuffleInOrder, + PlaybackOrder.MultiEpisodeShuffle, + PlaybackOrder.RandomRotation, + PlaybackOrder.Marathon, + PlaybackOrder.WeightedShuffle + }), + + // SchedulingEngine.EnumeratorForContent (Scripted); unsupported orders LogWarning + None. + [SchedulingEngineKind.Scripted] = new EngineSupport( + new OrderSet { PlaybackOrder.Chronological, PlaybackOrder.Shuffle }, + new OrderSet + { + PlaybackOrder.None, + PlaybackOrder.Random, + PlaybackOrder.SeasonEpisode, + PlaybackOrder.ShuffleInOrder, + PlaybackOrder.MultiEpisodeShuffle, + PlaybackOrder.RandomRotation, + PlaybackOrder.Marathon, + PlaybackOrder.WeightedShuffle + }) + }; + + public static IReadOnlyCollection Engines => Matrix.Keys.ToList(); + + /// The orders can turn into an enumerator. + public static IReadOnlySet SupportedBy(SchedulingEngineKind engine) => Matrix[engine].Supported; + + /// The orders explicitly known NOT to be handled by . + public static IReadOnlySet UnsupportedBy(SchedulingEngineKind engine) => Matrix[engine].Unsupported; + + public static bool IsSupported(SchedulingEngineKind engine, PlaybackOrder order) => + Matrix[engine].Supported.Contains(order); + + /// + /// True when appears in either set for . A new enum + /// value is classified in neither until a human adds it — which is what the tripwire test asserts. + /// + public static bool IsClassified(SchedulingEngineKind engine, PlaybackOrder order) => + Matrix[engine].Supported.Contains(order) || Matrix[engine].Unsupported.Contains(order); +} diff --git a/ErsatzTV.Core/Scheduling/PlaylistEnumerator.cs b/ErsatzTV.Core/Scheduling/PlaylistEnumerator.cs index a93cead90..c4c6e9e4a 100644 --- a/ErsatzTV.Core/Scheduling/PlaylistEnumerator.cs +++ b/ErsatzTV.Core/Scheduling/PlaylistEnumerator.cs @@ -3,6 +3,7 @@ using ErsatzTV.Core.Domain; using ErsatzTV.Core.Extensions; using ErsatzTV.Core.Interfaces.Repositories; using ErsatzTV.Core.Interfaces.Scheduling; +using Microsoft.Extensions.Logging; namespace ErsatzTV.Core.Scheduling; @@ -121,7 +122,8 @@ public class PlaylistEnumerator : IMediaCollectionEnumerator CollectionEnumeratorState state, bool shufflePlaylistItems, Option batchSize, - CancellationToken cancellationToken) + CancellationToken cancellationToken, + Option logger = default) { var result = new PlaylistEnumerator { @@ -203,6 +205,19 @@ public class PlaylistEnumerator : IMediaCollectionEnumerator break; case PlaybackOrder.Random: enumerator = new RandomizedMediaCollectionEnumerator(items, initState); + break; + default: + // An order the playlist engine doesn't handle (#403). Leaving the enumerator null drops + // this item from the playlist silently -- and null is a legitimate state above + // (SeasonEpisode with Count == 0), so nothing downstream can flag it. Say so here. + foreach (ILogger log in logger) + { + log.LogWarning( + "Playback order {PlaybackOrder} is not supported by playlist scheduling; " + + "this item will be dropped from the playlist", + playlistItem.PlaybackOrder); + } + break; } } diff --git a/ErsatzTV.Core/Scheduling/PlayoutBuilder.cs b/ErsatzTV.Core/Scheduling/PlayoutBuilder.cs index 609c29e35..a7e66db0b 100644 --- a/ErsatzTV.Core/Scheduling/PlayoutBuilder.cs +++ b/ErsatzTV.Core/Scheduling/PlayoutBuilder.cs @@ -1324,7 +1324,8 @@ public class PlayoutBuilder : IPlayoutBuilder state, marathonShuffleGroups, batchSize: Option.None, - cancellationToken); + cancellationToken, + Optional((ILogger)_logger)); } } @@ -1458,7 +1459,12 @@ public class PlayoutBuilder : IPlayoutBuilder goto default; default: - // TODO: handle this error case differently? + // An order the classic engine doesn't handle (#403). Say so instead of silently substituting a + // different, arbitrary-looking order; keep airing on the Random fallback so the channel does not + // go dark on one misconfigured item. + _logger.LogWarning( + "Playback order {PlaybackOrder} is not supported by classic scheduling; falling back to Random", + playbackOrder); return new RandomizedMediaCollectionEnumerator(mediaItems, state); } } diff --git a/ErsatzTV.Core/Scheduling/SchedulingEngineKind.cs b/ErsatzTV.Core/Scheduling/SchedulingEngineKind.cs new file mode 100644 index 000000000..1a2ae9836 --- /dev/null +++ b/ErsatzTV.Core/Scheduling/SchedulingEngineKind.cs @@ -0,0 +1,23 @@ +namespace ErsatzTV.Core.Scheduling; + +/// +/// The scheduling engine families that dispatch a into an +/// enumerator. Used to key the matrix (#403). +/// +public enum SchedulingEngineKind +{ + /// Classic schedules — PlayoutBuilder. + Classic, + + /// Playlist items — PlaylistEnumerator. + Playlist, + + /// Block scheduling — BlockPlayoutBuilder. + Block, + + /// YAML (sequential) scheduling — EnumeratorCache. + Yaml, + + /// Scripted scheduling — SchedulingEngine.EnumeratorForContent. + Scripted +} diff --git a/ErsatzTV.Core/Scheduling/YamlScheduling/EnumeratorCache.cs b/ErsatzTV.Core/Scheduling/YamlScheduling/EnumeratorCache.cs index 1f1f575bf..c5b1b04a0 100644 --- a/ErsatzTV.Core/Scheduling/YamlScheduling/EnumeratorCache.cs +++ b/ErsatzTV.Core/Scheduling/YamlScheduling/EnumeratorCache.cs @@ -175,7 +175,8 @@ public class EnumeratorCache(IMediaCollectionRepository mediaCollectionRepositor state, shufflePlaylistItems: false, batchSize: Option.None, - cancellationToken); + cancellationToken, + Optional(logger)); } var parsedOrder = Enum.Parse(content.Order, true); diff --git a/docs/decisions.md b/docs/decisions.md index 559a70a85..716c2be07 100644 --- a/docs/decisions.md +++ b/docs/decisions.md @@ -96,6 +96,7 @@ in-file entries. - [2026-07-18 — SmartCollection rule builder: compile-only closed subset, no stored AST, one-level nesting (#176)](#2026-07-18--smartcollection-rule-builder-compile-only-closed-subset-no-stored-ast-one-level-nesting-176) - [2026-07-18 — Auto-Tune per-source weights ride #70's MultiCollection machinery; created at tune time, not a post-hoc PUT (#425)](#2026-07-18--auto-tune-per-source-weights-ride-70s-multicollection-machinery-created-at-tune-time-not-a-post-hoc-put-425) - [2026-07-18 — Search all-items is paged to cap DoS exposure; SPA add-all pages to completeness (#293)](#2026-07-18--search-all-items-is-paged-to-cap-dos-exposure-spa-add-all-pages-to-completeness-293) +- [2026-07-18 — Unsupported PlaybackOrder is loud at build time; a declared support matrix and tripwire test make new orders safe by construction (#403)](#2026-07-18--unsupported-playbackorder-is-loud-at-build-time-a-declared-support-matrix-and-tripwire-test-make-new-orders-safe-by-construction-403) --- @@ -1837,3 +1838,49 @@ icon rail and (b) collapsible accordions per **labeled** nav group (Media, Syste - **Group keys are explicit + stable** (`SidebarNavGroupDefinition.key`: `'media'`, `'system'`) rather than derived from the label, so renaming a label doesn't silently orphan persisted state. - No route/screen was added or redirected (shell-chrome only), so no `blazor-route-parity.md` change. + +## 2026-07-18 — Unsupported PlaybackOrder is loud at build time; a declared support matrix and tripwire test make new orders safe by construction (#403) + +`#70` closed the *persistence* hole for `WeightedShuffle` (the write path rejects it on the engines that +can't handle it) and made **YAML + Scripted** log a warning; `MultiCollectionGroup` already threw. It left +the three still-**silent** build-time dispatch sites — the ones this issue names — as an explicit non-goal. +`#403` makes those loud and adds the by-construction net. + +- **Loud, but NON-FATAL, at every build site.** On an order it doesn't handle, each site now logs a + `Warning` (naming the order + engine + the fallback taken) instead of silently substituting/dropping/ + skipping: Classic's `PlayoutBuilder` `default:` arm (was `// TODO`, silently returned Random), the + `PlaylistEnumerator.Create` switch (had **no** `default:` arm → null enumerator → item dropped), and + `BlockPlayoutBuilder`'s allow-list `continue` (silent skip). The actual fallback is **preserved** — a + live channel airing wrong-but-something beats going dark on one misconfigured item — so scheduler goldens + do not move. This matches the log-and-continue posture `#70` already shipped for YAML/Scripted. + - `PlaylistEnumerator.Create` was `static` with **no logger**, which is *why* the playlist drop was + unreportable. It gained an optional `Option logger = default` (mirrors the existing + `GetStartTimeAfter(..., Option.None)` pattern); the three callers that have a logger pass it, + the rest default to `None`. + - `BlockPlayoutBuilder`'s `GetEnumerator` switch gained an explicit `PlaybackOrder.Random` arm. Random is + in Block's allow-list but previously reached an enumerator only via the switch's coincidental `_ =>` + Random fallback; the fallback is now a loud helper (defensive — the allow-list should make it + unreachable). + +- **Safe-by-construction net: a declared support matrix + a tripwire test.** `PlaybackOrderSupport` + (`ErsatzTV.Core/Scheduling/`) declares, per `SchedulingEngineKind` (Classic / Playlist / Block / Yaml / + Scripted), BOTH a `Supported` and an `Unsupported` set. `PlaybackOrderSupportTests` asserts the two sets + **partition** every `PlaybackOrder` value (union total, intersection empty) for every engine — so adding a + new enum value lands in neither set and **fails the test until it is consciously classified** and wired + into the matching dispatch switch. Both sets are hand-maintained on purpose: deriving `Unsupported` as + "everything not supported" would let a new order fall through silently, which is the exact defect being + killed. The matrix is not pure test scaffolding — `BlockPlayoutBuilder` consumes it at runtime for its + allow-list (replacing the previously duplicated inline list). + +- **Write-path rejection is deliberately UNCHANGED.** `#70`'s per-engine guards already close the real + persistence exposure, and the decisions log records that the "which order reaches which engine" perimeter + has been wrong three times — always by enumerating from a list instead of grepping every writer. + Broadening write-path rejection here would risk newly-`400`ing configs that currently save-and-fall-back, + for no safety gain, so it was left alone. This PR hardens the *build-time* sites and adds the tripwire. + +- **Reverse-mappings deferred (a different axis).** The three `_ => PlaybackOrder.None` arms that map an + enumerator *class* back to a `PlaybackOrder` for `PlayoutHistory` were left alone: that path is reached in + normal operation by legitimately-supported enumerator types (`ShuffleInOrderCollectionEnumerator`, + `SeasonEpisodeMediaCollectionEnumerator`) that simply aren't reverse-mapped, so making it "loud" would + emit false-positive warnings. Completing that reverse map is a separate concern from "an unsupported + *order* degrades silently" and is not part of #403's scope. -- 2.47.3 From 1d95cbfee6d833f1ba2e27696c4d1c144888799d Mon Sep 17 00:00:00 2001 From: Timothy Date: Sat, 18 Jul 2026 18:32:14 +0200 Subject: [PATCH 2/2] fix(403): accurate Classic fallback message + engine-coverage tripwire Review follow-ups (cold review of PR #457): - Marathon can reach PlayoutBuilder's default arm via `goto default` when its enumerator can't be built; the new warning claimed "Marathon is not supported by classic scheduling", which is false. Distinguish supported-but-failed-to-build (logs "could not build") from genuinely unsupported using PlaybackOrderSupport, which also makes Classic a runtime consumer of the matrix. - PlaybackOrderSupportTests now asserts every SchedulingEngineKind has a matrix entry, so a new engine kind fails the test instead of throwing KeyNotFoundException at runtime. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../Scheduling/PlaybackOrderSupportTests.cs | 14 +++++++++++ ErsatzTV.Core/Scheduling/PlayoutBuilder.cs | 23 ++++++++++++++----- 2 files changed, 31 insertions(+), 6 deletions(-) diff --git a/ErsatzTV.Core.Tests/Scheduling/PlaybackOrderSupportTests.cs b/ErsatzTV.Core.Tests/Scheduling/PlaybackOrderSupportTests.cs index 541e9da47..7c64bafef 100644 --- a/ErsatzTV.Core.Tests/Scheduling/PlaybackOrderSupportTests.cs +++ b/ErsatzTV.Core.Tests/Scheduling/PlaybackOrderSupportTests.cs @@ -28,6 +28,20 @@ public class PlaybackOrderSupportTests } } + // Every SchedulingEngineKind must have a matrix entry, or Matrix[engine] throws KeyNotFoundException at + // runtime instead of failing here. This is the engine-axis counterpart to the order tripwire. + [Test] + public void EveryEngineKind_HasAMatrixEntry() + { + var classified = PlaybackOrderSupport.Engines.ToHashSet(); + + foreach (SchedulingEngineKind engine in Enum.GetValues()) + { + classified.ShouldContain(engine, + $"SchedulingEngineKind.{engine} has no PlaybackOrderSupport matrix entry (#403)."); + } + } + // The two sets must partition the enum: no order both supported and unsupported, and together they cover // exactly the enum (no stale entry for a removed value, no missing value). [Test] diff --git a/ErsatzTV.Core/Scheduling/PlayoutBuilder.cs b/ErsatzTV.Core/Scheduling/PlayoutBuilder.cs index a7e66db0b..4ae9bd4f9 100644 --- a/ErsatzTV.Core/Scheduling/PlayoutBuilder.cs +++ b/ErsatzTV.Core/Scheduling/PlayoutBuilder.cs @@ -1459,12 +1459,23 @@ public class PlayoutBuilder : IPlayoutBuilder goto default; default: - // An order the classic engine doesn't handle (#403). Say so instead of silently substituting a - // different, arbitrary-looking order; keep airing on the Random fallback so the channel does not - // go dark on one misconfigured item. - _logger.LogWarning( - "Playback order {PlaybackOrder} is not supported by classic scheduling; falling back to Random", - playbackOrder); + // Say so instead of silently substituting a different, arbitrary-looking order; keep airing on + // the Random fallback so the channel does not go dark on one misconfigured item (#403). A + // supported order can also land here via `goto default` (Marathon that couldn't build its + // enumerator) -- distinguish the two so the log doesn't claim a supported order is unsupported. + if (PlaybackOrderSupport.IsSupported(SchedulingEngineKind.Classic, playbackOrder)) + { + _logger.LogWarning( + "Could not build a {PlaybackOrder} enumerator for classic scheduling; falling back to Random", + playbackOrder); + } + else + { + _logger.LogWarning( + "Playback order {PlaybackOrder} is not supported by classic scheduling; falling back to Random", + playbackOrder); + } + return new RandomizedMediaCollectionEnumerator(mediaItems, state); } } -- 2.47.3