timothy/ersatztv
1
0
Fork 0
Code Issues 57 Pull Requests 6 Actions Packages Projects Releases 9 Wiki Activity

Scheduling: an unhandled PlaybackOrder fails silently at five of six dispatch sites #403

New Issue
Closed
opened 2026-07-17 12:59:29 +02:00 by timothy · 3 comments
timothy commented 2026-07-17 12:59:29 +02:00
Owner
Copy Link
Copy Source

Split out of #70 as an explicit non-goal of PR #402, and confirmed independently by that PR's adversarial review.

Problem

Adding a new PlaybackOrder value is unsafe by construction: five of the six sites that turn the enum into an enumerator mishandle an unknown value silently, and two of them substitute a different working order rather than failing.

Engine Site Unhandled value →
Classic PlayoutBuilder.cs default: arm Silently returns RandomizedMediaCollectionEnumerator. Carries the comment // TODO: handle this error case differently?
Playlist PlaylistEnumerator.cs:166-207 — no default: arm at all Enumerator stays null → :210 if (enumerator is not null) → the item is silently dropped from the playlist. Aggravating factor: null is a legitimate state at :201 (SeasonEpisode with Count == 0), so nothing can flag it
Block BlockPlayoutBuilder.cs:54-61 + :166-169 allow-list filter Silently continues — the block item never airs, before the :367 switch is even reached
YAML EnumeratorCache.cs → Option.None Soft-fails; callers read None as "no content"
Scripted SchedulingEngine.EnumeratorForContent → Option.None (5 foreach call sites) Soft-fails; content silently absent
— MultiCollectionGroup.cs:41-43 The only loud site (throw new NotSupportedException)

Also: three reverse-mappings (enumerator type → PlaybackOrder) fall back to _ => PlaybackOrder.None (SchedulingEngine.cs, BlockPlayoutEnumerator.cs, YamlPlayoutApplyHistoryHandler.cs), so a new enumerator class silently reverse-maps to None when writing PlayoutHistory.

Latent fragility spotted while surveying: Random is in Block's allow-list but has no arm in the :367 switch — it works only by coincidence, via the _ => Random fallback.

Why it matters

A distribution order degrading to unweighted random is undetectable by inspection — the output is supposed to look arbitrary. #70 worked around this by refusing WeightedShuffle at the write path for engines that can't handle it (PR #402), which closes that feature's exposure but leaves the trap armed for the next order added. #176 and any future ordering work walk into the same hole.

Scope

Make the unsupported-order path loud rather than silent. Options to weigh (design call):

  • A shared PlaybackOrderSupport predicate per engine family, enforced at the write path centrally instead of per-handler (PR #402 had to patch handlers one at a time — and missed one, CreateChannelFromLineup, which its review caught).
  • Throw / return an error at the dispatch sites instead of substituting Random or dropping.
  • At minimum, log a warning at each fallback (PR #402 did this for YAML + Scripted only).

Changing shipped fallback behavior is a real behavior change in the riskiest subsystem — hence its own issue, its own review, and it should land on the #163/#380 golden net.

Notes

  • Related: #70 (introduced WeightedShuffle and the write-path workaround), #380 (the ShuffleSourceBuilder seam), #176.
  • decisions.md 2026-07-17 (#70 entry) documents the full site-by-site table.

Done-when

  • Unsupported-order handling is loud (error or logged) at every dispatch site, or structurally prevented at the write path by a shared predicate
  • Block's Random-in-allow-list-but-not-in-switch coincidence is resolved
  • Reverse-mapping _ => PlaybackOrder.None fallbacks reviewed
  • Golden/characterization coverage proves no behavior drift for existing orders
  • Adversarial review passed
  • decisions.md updated
Split out of #70 as an explicit non-goal of PR #402, and confirmed independently by that PR's adversarial review. ## Problem Adding a new `PlaybackOrder` value is unsafe by construction: five of the six sites that turn the enum into an enumerator mishandle an unknown value **silently**, and two of them substitute a *different working order* rather than failing. | Engine | Site | Unhandled value → | |---|---|---| | Classic | `PlayoutBuilder.cs` `default:` arm | **Silently** returns `RandomizedMediaCollectionEnumerator`. Carries the comment `// TODO: handle this error case differently?` | | Playlist | `PlaylistEnumerator.cs:166-207` — **no `default:` arm at all** | Enumerator stays null → `:210 if (enumerator is not null)` → the item is **silently dropped from the playlist**. Aggravating factor: null is a *legitimate* state at `:201` (SeasonEpisode with `Count == 0`), so nothing can flag it | | Block | `BlockPlayoutBuilder.cs:54-61` + `:166-169` allow-list filter | **Silently `continue`s** — the block item never airs, before the `:367` switch is even reached | | YAML | `EnumeratorCache.cs` → `Option.None` | Soft-fails; callers read `None` as "no content" | | Scripted | `SchedulingEngine.EnumeratorForContent` → `Option.None` (5 `foreach` call sites) | Soft-fails; content silently absent | | — | `MultiCollectionGroup.cs:41-43` | The **only** loud site (`throw new NotSupportedException`) | Also: three reverse-mappings (enumerator type → `PlaybackOrder`) fall back to `_ => PlaybackOrder.None` (`SchedulingEngine.cs`, `BlockPlayoutEnumerator.cs`, `YamlPlayoutApplyHistoryHandler.cs`), so a new enumerator *class* silently reverse-maps to `None` when writing `PlayoutHistory`. Latent fragility spotted while surveying: `Random` **is** in Block's allow-list but has **no arm** in the `:367` switch — it works only by coincidence, via the `_ =>` Random fallback. ## Why it matters **A distribution order degrading to unweighted random is undetectable by inspection — the output is *supposed* to look arbitrary.** #70 worked around this by refusing `WeightedShuffle` at the write path for engines that can't handle it (PR #402), which closes *that* feature's exposure but leaves the trap armed for the next order added. #176 and any future ordering work walk into the same hole. ## Scope Make the unsupported-order path **loud** rather than silent. Options to weigh (design call): - A shared `PlaybackOrderSupport` predicate per engine family, enforced at the write path centrally instead of per-handler (PR #402 had to patch handlers one at a time — and **missed one**, `CreateChannelFromLineup`, which its review caught). - Throw / return an error at the dispatch sites instead of substituting Random or dropping. - At minimum, log a warning at each fallback (PR #402 did this for YAML + Scripted only). Changing shipped fallback behavior is a real behavior change in the riskiest subsystem — hence its own issue, its own review, and it should land on the #163/#380 golden net. ## Notes - Related: #70 (introduced `WeightedShuffle` and the write-path workaround), #380 (the `ShuffleSourceBuilder` seam), #176. - `decisions.md` 2026-07-17 (#70 entry) documents the full site-by-site table. ## Done-when - [x] Unsupported-order handling is loud (error or logged) at every dispatch site, or structurally prevented at the write path by a shared predicate - [x] Block's `Random`-in-allow-list-but-not-in-switch coincidence is resolved - [x] Reverse-mapping `_ => PlaybackOrder.None` fallbacks reviewed - [x] Golden/characterization coverage proves no behavior drift for existing orders - [x] Adversarial review passed - [x] `decisions.md` updated
timothy added the enhancementpriority: medium labels 2026-07-17 12:59:30 +02:00
timothy added the in-progress label 2026-07-18 17:44:13 +02:00
timothy commented 2026-07-18 17:44:25 +02:00
Author
Owner
Copy Link
Copy Source

Claiming (Claude Code orchestrator session, 2026-07-18).

Selected as the next eligible priority: medium backlog issue after #350: #350 is already claimed by a parallel session (instrumentation shipped via PR #446, remaining optimization deferred/data-gated), so it is excluded. #403 is the lowest-numbered unclaimed tier-5 candidate; #404 is next.

Plan: make the unsupported-PlaybackOrder path loud rather than silent across the six dispatch sites, guarded by the scheduler golden net. Will decide the exact mechanism (shared per-engine support predicate at the write path vs. loud throws/logs at each site) after reading the current sites + docs, and report the design call here before implementing widely.

Claiming (Claude Code orchestrator session, 2026-07-18). Selected as the next eligible `priority: medium` backlog issue after #350: #350 is already claimed by a parallel session (instrumentation shipped via PR #446, remaining optimization deferred/data-gated), so it is excluded. #403 is the lowest-numbered unclaimed tier-5 candidate; #404 is next. Plan: make the unsupported-`PlaybackOrder` path **loud** rather than silent across the six dispatch sites, guarded by the scheduler golden net. Will decide the exact mechanism (shared per-engine support predicate at the write path vs. loud throws/logs at each site) after reading the current sites + docs, and report the design call here before implementing widely.
timothy commented 2026-07-18 17:51:25 +02:00
Author
Owner
Copy Link
Copy Source

Design call (before implementing widely)

Grounded in docs/decisions.md (the #70 entry, lines ~1447-1459) which explicitly deferred this to #403: #70 closed the persistence hole (write-path rejects WeightedShuffle where unhandled) and made YAML + Scripted loud (LogWarning); MultiCollectionGroup already throws. The still-silent build-time sites are the three this issue targets:

  • Classic PlayoutBuilder.cs:1460 default: -> silently substitutes RandomizedMediaCollectionEnumerator (the // TODO arm).
  • Playlist PlaylistEnumerator.cs -> no default: arm; enumerator stays null -> item silently dropped. (Create is static with no logger -> that's why it can't report; I'll add an optional Option<ILogger> param, matching the existing GetStartTimeAfter(..., Option<ILogger>.None) pattern.)
  • Block BlockPlayoutBuilder.cs:166-169 allow-list continue -> silent skip; plus the :367 _ => silent-Random, and Random is allow-listed but has no explicit arm (works only by coincidence).

Approach

  1. Loud but NON-FATAL at every build site. On an unsupported order, log Error/Warning (order + engine + fallback taken) instead of silently substituting/dropping/skipping. The actual fallback is preserved -> scheduler goldens do not move. A channel airing wrong-but-something beats a channel going dark on one misconfigured item (same posture as #70's YAML/Scripted log-and-skip). Also give Block an explicit Random arm to kill the coincidence.
  2. Safe-by-construction: a single declared support matrix ErsatzTV.Core/Scheduling/PlaybackOrderSupport.cs (per engine family) + a tripwire test asserting supported ∪ unsupported == Enum.GetValues<PlaybackOrder>() for each engine. Adding a new PlaybackOrder value then fails the test until it is consciously classified — that is the by-construction guarantee the issue asks for.
  3. Reverse-mappings (_ => PlaybackOrder.None in SchedulingEngine, BlockPlayoutEnumerator, YamlPlayoutApplyHistoryHandler): made loud where a logger is in reach.

Deliberately OUT of scope

Not broadening the write-path rejection beyond #70's per-engine guards. The decisions doc records that this exact 'which order reaches which engine' perimeter has been wrong three times, always by enumerating from a list instead of grepping every writer — and #70 already closed the real persistence exposure. Generalizing it now risks newly-400ing configs that currently save-and-fall-back, for no safety gain. So write-path validation is unchanged; this PR makes the build-time fallbacks loud + adds the tripwire.

Touches the scheduler, so: golden net stays green + mandatory independent (cold/cross-model) review before merge. Will report progress here.

## Design call (before implementing widely) Grounded in `docs/decisions.md` (the #70 entry, lines ~1447-1459) which explicitly deferred this to #403: #70 closed the *persistence* hole (write-path rejects `WeightedShuffle` where unhandled) and made **YAML + Scripted** loud (`LogWarning`); `MultiCollectionGroup` already throws. The still-**silent** build-time sites are the three this issue targets: - **Classic** `PlayoutBuilder.cs:1460` `default:` -> silently substitutes `RandomizedMediaCollectionEnumerator` (the `// TODO` arm). - **Playlist** `PlaylistEnumerator.cs` -> no `default:` arm; enumerator stays null -> item silently dropped. (`Create` is `static` with **no logger** -> that's *why* it can't report; I'll add an optional `Option<ILogger>` param, matching the existing `GetStartTimeAfter(..., Option<ILogger>.None)` pattern.) - **Block** `BlockPlayoutBuilder.cs:166-169` allow-list `continue` -> silent skip; plus the `:367` `_ =>` silent-Random, and `Random` is allow-listed but has **no explicit arm** (works only by coincidence). ### Approach 1. **Loud but NON-FATAL at every build site.** On an unsupported order, log Error/Warning (order + engine + fallback taken) instead of silently substituting/dropping/skipping. The actual fallback is **preserved** -> scheduler goldens do not move. A channel airing wrong-but-something beats a channel going dark on one misconfigured item (same posture as #70's YAML/Scripted log-and-skip). Also give Block an explicit `Random` arm to kill the coincidence. 2. **Safe-by-construction:** a single declared support matrix `ErsatzTV.Core/Scheduling/PlaybackOrderSupport.cs` (per engine family) + a **tripwire test** asserting `supported ∪ unsupported == Enum.GetValues<PlaybackOrder>()` for each engine. Adding a new `PlaybackOrder` value then **fails the test until it is consciously classified** — that is the by-construction guarantee the issue asks for. 3. **Reverse-mappings** (`_ => PlaybackOrder.None` in `SchedulingEngine`, `BlockPlayoutEnumerator`, `YamlPlayoutApplyHistoryHandler`): made loud where a logger is in reach. ### Deliberately OUT of scope **Not** broadening the write-path rejection beyond #70's per-engine guards. The decisions doc records that this exact 'which order reaches which engine' perimeter has been **wrong three times**, always by enumerating from a list instead of grepping every writer — and #70 already closed the real persistence exposure. Generalizing it now risks newly-400ing configs that currently save-and-fall-back, for no safety gain. So write-path validation is unchanged; this PR makes the *build-time* fallbacks loud + adds the tripwire. Touches the scheduler, so: golden net stays green + mandatory independent (cold/cross-model) review before merge. Will report progress here.
timothy removed the in-progress label 2026-07-18 19:10:49 +02:00
timothy commented 2026-07-18 19:11:14 +02:00
Author
Owner
Copy Link
Copy Source

Done — merged via PR #457

What was done

Made the unsupported-PlaybackOrder path loud but non-fatal at the three still-silent build-time dispatch sites, and added a by-construction net so the next order can't slip through:

  • Classic (PlayoutBuilder default: arm) — was // TODO silently returning Random; now logs a Warning (and correctly distinguishes a supported order that fell through via goto default, e.g. a Marathon that couldn't build, from a genuinely-unsupported order). Keeps the Random fallback.
  • Playlist (PlaylistEnumerator.Create) — had no default: arm → null enumerator → item silently dropped. Create was static with no logger (root cause of the un-reportable drop); added an optional Option<ILogger>, loggered callers pass it, new default: arm warns.
  • Block (BlockPlayoutBuilder) — silent allow-list continue now warns; added an explicit Random arm (the coincidence this issue flagged) + a loud defensive fallback.
  • PlaybackOrderSupport matrix (per SchedulingEngineKind) + tripwire PlaybackOrderSupportTests: Supported ∪ Unsupported must partition the enum for every engine, and every engine kind must have a matrix entry — so a new order (or engine) fails the test until consciously classified. Block consumes the matrix at runtime for its allow-list (removing a duplicated inline list).

Done-when

All six boxes ticked: loud at every site ✅, Block Random coincidence resolved ✅, reverse-mappings reviewed ✅ (deferred, see below), golden/characterization no-drift ✅, adversarial review passed ✅, decisions.md updated ✅.

Files changed

ErsatzTV.Core/Scheduling/: PlaybackOrderSupport.cs (new), SchedulingEngineKind.cs (new), PlayoutBuilder.cs, PlaylistEnumerator.cs, BlockScheduling/BlockPlayoutBuilder.cs, YamlScheduling/EnumeratorCache.cs, Engine/SchedulingEngine.cs. Tests: PlaybackOrderSupportTests.cs (new tripwire), PlaylistEnumeratorTests.cs (behavioral drop+warn test). Docs: docs/decisions.md.

Deliberately deferred (not follow-up issues — documented non-goals)

  • Write-path rejection left unchanged. #70 already closed the persistence hole; decisions.md records this perimeter has been wrong three times, so broadening it here is pure regression risk for no safety gain.
  • Reverse _ => PlaybackOrder.None mappings (SchedulingEngine, BlockPlayoutEnumerator, YamlPlayoutApplyHistoryHandler) reviewed and left alone: that path is the reverse axis (enumerator-class → order for history) and is hit by legitimately-supported enumerator types (ShuffleInOrder/SeasonEpisode) that simply aren't reverse-mapped, so making it loud would emit false-positive warnings. Completing that reverse map is a separate concern.

Verification

Full ErsatzTV.Core.Tests (586+) incl. scheduler goldens green (no behavior drift); infra + app scheduling tests green. Independent cold adversarial review (+ focused re-review of the fix commit): MERGEABLE @ 1d95cbfe — matrix accuracy, Block allow-list parity, byte-identical fallbacks, and non-vacuous tripwire all verified. CI green.

Docs updated

docs/decisions.md — appended the #403 design entry (loud build-time sites + support matrix/tripwire; write-path and reverse-map scope calls).

## Done — merged via PR #457 ### What was done Made the unsupported-`PlaybackOrder` path **loud but non-fatal** at the three still-silent build-time dispatch sites, and added a by-construction net so the next order can't slip through: - **Classic** (`PlayoutBuilder` `default:` arm) — was `// TODO` silently returning Random; now logs a Warning (and correctly distinguishes a supported order that fell through via `goto default`, e.g. a Marathon that couldn't build, from a genuinely-unsupported order). Keeps the Random fallback. - **Playlist** (`PlaylistEnumerator.Create`) — had **no** `default:` arm → null enumerator → item silently dropped. `Create` was `static` with no logger (root cause of the un-reportable drop); added an optional `Option<ILogger>`, loggered callers pass it, new `default:` arm warns. - **Block** (`BlockPlayoutBuilder`) — silent allow-list `continue` now warns; added an explicit `Random` arm (the coincidence this issue flagged) + a loud defensive fallback. - **`PlaybackOrderSupport`** matrix (per `SchedulingEngineKind`) + **tripwire** `PlaybackOrderSupportTests`: `Supported ∪ Unsupported` must partition the enum for every engine, and every engine kind must have a matrix entry — so a new order (or engine) fails the test until consciously classified. Block consumes the matrix at runtime for its allow-list (removing a duplicated inline list). ### Done-when All six boxes ticked: loud at every site ✅, Block `Random` coincidence resolved ✅, reverse-mappings reviewed ✅ (deferred, see below), golden/characterization no-drift ✅, adversarial review passed ✅, `decisions.md` updated ✅. ### Files changed `ErsatzTV.Core/Scheduling/`: `PlaybackOrderSupport.cs` (new), `SchedulingEngineKind.cs` (new), `PlayoutBuilder.cs`, `PlaylistEnumerator.cs`, `BlockScheduling/BlockPlayoutBuilder.cs`, `YamlScheduling/EnumeratorCache.cs`, `Engine/SchedulingEngine.cs`. Tests: `PlaybackOrderSupportTests.cs` (new tripwire), `PlaylistEnumeratorTests.cs` (behavioral drop+warn test). Docs: `docs/decisions.md`. ### Deliberately deferred (not follow-up issues — documented non-goals) - **Write-path rejection left unchanged.** #70 already closed the persistence hole; `decisions.md` records this perimeter has been wrong three times, so broadening it here is pure regression risk for no safety gain. - **Reverse `_ => PlaybackOrder.None` mappings** (`SchedulingEngine`, `BlockPlayoutEnumerator`, `YamlPlayoutApplyHistoryHandler`) reviewed and left alone: that path is the *reverse* axis (enumerator-class → order for history) and is hit by legitimately-supported enumerator types (ShuffleInOrder/SeasonEpisode) that simply aren't reverse-mapped, so making it loud would emit false-positive warnings. Completing that reverse map is a separate concern. ### Verification Full `ErsatzTV.Core.Tests` (586+) incl. scheduler **goldens** green (no behavior drift); infra + app scheduling tests green. Independent cold adversarial review (+ focused re-review of the fix commit): **MERGEABLE @ 1d95cbfe** — matrix accuracy, Block allow-list parity, byte-identical fallbacks, and non-vacuous tripwire all verified. CI green. ### Docs updated `docs/decisions.md` — appended the #403 design entry (loud build-time sites + support matrix/tripwire; write-path and reverse-map scope calls).
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
ad-hoc
One-off / ad-hoc work not tracked by a dedicated issue
 api
REST API / HTTP endpoints
 bug
Something isn't working
 ci-cd
Build, test, deploy pipeline
 content
Channel content / schedules / playlists
 dependencies
Dependency updates (Renovate)
 enhancement
New feature or improvement
 frontend
ChicoryTV React SPA frontend
 in-progress
Claimed by an active session — do not pick up
 jellyfin
Jellyfin tuner / IPTV integration
 parked
Excluded from automatic queue pickup; work only when explicitly selected
 priority: high
priority: low
priority: medium
review
Adversarial review finding
 security
Security / vulnerability fix
No labels enhancement priority: medium
Milestone
No items
No Milestone
Projects
Clear projects
No projects
Assignees
Clear assignees
renovate timothy (Timothy)
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: timothy/ersatztv#403