feat(70): weighted / fair-share content distribution (WeightedShuffle playback order) #402
No Assignees
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: timothy/ersatztv#402
Reference in New Issue
Block a user
Closes #70 (slice 1: backend + API, Classic engine only).
Design verdict + rationale: #70's design-verdict comment. Decision record:
docs/decisions.md2026-07-17.What this does
Adds
PlaybackOrder.WeightedShuffle = 9. It picks a source by smooth weighted round-robin, then takes that source's next item, so a source'sWeightis its share of airtime. Weights 3:1 emitA A B A(spread, not drained). Fair-share is the equal-weights default, so one order covers both behaviors the issue asked for.The finding that justifies the issue existing
Recon initially concluded fair-share was already shipped as
ShuffleInOrder— which would have collapsed #70 to a docs-only issue (the #77 precedent). That was wrong, and the frontier design pass caught it:ShuffleInOrder's balanced shuffle (keyj) pads every source to the longest withOption<MediaItem>.Nonespacers — but spacers emit nothing. One cycle therefore plays every item exactly once, so a 200-episode show still takes 10x the airtime of a 20-episode one. It buys anti-clumping, not fair-share. PlainShuffleis likewise implicitly weighted by collection size.That distinction is the entire justification for this feature, and it read the other way round, so it is now written into
decisions.mdand thedomain-model.mdglossary rather than left to be rediscovered.Design highlights
WeightedShufflewith weights left at 1; the UI's "fair-share toggle" needs no second mechanism. A separate non-enum "distribution" setting was rejected (it would add an axis every dispatch site must also consult); retrofitting weights ontoShuffleInOrderwas rejected (it would silently change shipped output).ShuffleSourceBuilder.GetCollectionItemsForShuffleInOrder— the schedule-entity-free entry point that PR #391's decision entry explicitly reserved for "#70 distribution". No signature change, no god-factory, engines stay separate.MultiCollectionGroupercollapses sources intoGroupedMediaItem+.Distinct(), so on theShufflepath which source an item came from is gone. OnlyCollectionWithItems(ShuffleInOrder-shaped path) can carry a per-source weight.Seed,Index) — restores by replay like its siblings, no per-source counters (CollectionEnumeratorStatehas nowhere to put them). Ratios are data, not state: aResetbuild re-randomizes the seed but ratios hold.int WeightonMultiCollectionItemandMultiCollectionSmartItem; omitting the smart mirror un-weights smart-collection members silently. DB default is 1, not 0 — a 0-weight source is filtered out by the enumerator, so a 0 backfill would drop every pre-existing member from a rotation.The silent-fallback problem, and why validation instead
An unhandled
PlaybackOrderfails silently at five of six dispatch sites: Classic substitutesRandomizedMediaCollectionEnumerator;PlaylistEnumeratorhas nodefault:arm so the item is dropped from the playlist (and null is legitimate there forSeasonEpisodewithCount == 0, so nothing flags it);BlockPlayoutBuilderfilters against an allow-list andcontinues past the rest; YAML/Scripted returnNone, read downstream as "no content". OnlyMultiCollectionGroupthrows.A weighted order degrading to unweighted random is the worst case here — the output is supposed to look arbitrary, so nobody would notice.
So this PR closes the exposure at the write path:
ReplacePlaylistItems/ReplaceBlockItemsrejectWeightedShufflenaming where it is available. If it can't be persisted where it isn't handled, the silent sites never see it. YAML/Scripted address orders by name (Enum.Parseaccepts it regardless), so they get log warnings on unhandled fallthrough.Explicit non-goal: making those pre-existing fallbacks loud. Real defect class, wider than this feature, needs its own review — filed as #403.
A bug the non-vacuity control found
Inverting the WRR pick to prove the tests bite didn't fail — it hung. That exposed a real defect in my own code:
MoveNextretried the rebuild on wrap to avoid an immediate repeat, unbounded.ShuffleInOrdercan do that because its reshuffle randomizes the lead item; this order's lead is decided by weight, so the heaviest source always leads — and when it holds a single item the lead never changes and the retry never terminates. Two single-item collections with unequal weights would have wedged the playout build. Now bounded, with a regression test under a timeout.Verification
Build succeeded. 0 Error(s),error CScount 0.if(true), which trips CS0219 under warnings-as-errors and silently serves a stale dll to--no-build) failed 5 of 10 tests on a clean build. Reverted; green.check:apiexit 0 —v1.json/v1.d.ts/endpoint-indexregenerated, never hand-edited.weightis emitted optional, so/api/v1stays additive under the freeze.main23a615ecand re-verified green.Deviation from the design pass (reviewer, please weigh in)
The design specified per-source inner order mirroring
ShuffleInOrder'sOrderItems(chronological). I shuffle within-source instead (custom-ordered collections still honored). Reason: with chronological inner order and deterministic source selection, every rotation would be byte-identical — the reseed on wrap would change nothing and the repeat-avoidance retry could never succeed. "Weighted in order" would be a separate order if wanted. This is my call, not the design authority's, so it is flagged rather than buried.Deferred (not in this PR)
classic-weightedgolden inPlayoutBuildGoldenTests.blocked-bythis issue) — needs its own carrier: an auto-tuned channel is backed by one live SmartCollection, so its "sources" are members of a single collection, and aPlaylistItemcannot reference a search query. Nothing here forecloses it.Docs (same PR, per the repo rule)
decisions.md(new entry, incl. the ShuffleInOrder correction) +domain-model.md(new Playback order row contrasting the three orders; multi-collection row notes the join-entity mirror) + regeneratedv1.json/endpoint-index.md.🤖 Generated with Claude Code
Review round 1 — BLOCKED, fixed in
2c0be689The cold adversarial review returned BLOCKED. It could not break the algorithm — it probed the stateless-restore claim across rotation wraps (indices 12/13/20/37, all held) and measured the clamp preserving a 1000:1 ratio exactly. What it broke was the perimeter, and it was right on every count.
CreateChannelFromLineupis a THIRD writer ofPlaylistItem.PlaybackOrder; its guard only covered MultiCollection entries, so a ≥2-entry lineup of plain collections persistedWeightedShufflestraight through toPlaylistEnumerator's null-drop. Mydecisions.mdclaim that "the silent sites never see it" was false as written.WeightedShufflefor a multi-item lineup; allows it for multi collections (M1). 2 regression tests. Claim corrected indecisions.mdwith the lesson recorded.Weighthad no validation, and create/update disagreed on the same input. EF'sHasDefaultValue(1)substitutes 1 for a0on INSERT (0 reads as "not set") but an UPDATE writes the 0 through — and a 0-weight source is filtered out of the rotation, deleting it from the channel silently. The doc reasons carefully about this for the migration backfill and left the API wide open.MultiCollectionItemWeight(1..1000) validated on both paths so they can't drift; clamped again in the enumerator for rows predating the gate.Sum(weights)is checked arithmetic →OverflowExceptionfrom inside a playout build. Reachable via the API because of B2.longsum. Test.ScheduleAsGroupsilently ignored;MinimumDurationcached over a rotation that is a strict subset under the clamp; retry guard on the wrong quantity.defaultValue, despite a comment claiming otherwise.Verification after fixes:
Build succeeded. 0 Error(s)· Core.Tests 565 passed · ErsatzTV.Tests 1653 passed · 0 failed ·check:apiexit 0 · rebased ontoed4b0d45.The B1 lesson is the one worth carrying: I gated the two obvious writers and recorded the perimeter as complete. The composite handler is the one that gets missed — the same shape as the repo's existing "enumerate every producer" lesson (#235/#267), which I'd read and still walked into.
1fd21e3645to2c0be68905Review round 2 — re-review of the fix commit
2c0be689Per the #242 convention (re-review the fix commit, not just the initial diff — the fix is new code no reviewer has seen).
Verdict: MERGEABLE-WITH-NITS @
2c0be689, with three must-fix items. All three are now addressed in48b270e8.What the re-review confirmed independently
PlaylistItem.PlaybackOrder←ReplacePlaylistItemsHandler(gated) +CreateChannelFromLineupHandler(now gated);Add*ToPlaylistandTraktCommandBasehardcode their order; the two Preview handlers build in memory without persisting.BlockItem.PlaybackOrder←ReplaceBlockItemsHandler(gated). No fourth persisting writer. It also checked the two gate branches compose (a 2-entry lineup containing a MultiCollection passes the first check and is caught by themultiItemcheck).EffectiveWeightand watching 3 tests fail.MinimumDurationover_collectionsisn't a divergence fromShuffleInOrder— it restores the sibling's actual semantics, sinceShuffleInOrder's_shuffledalready contains every item once. It's also consumed as a cheap "can anything fit" early-out, so under-reporting costs a wasted loop iteration while over-reporting would truncate — the fix errs safe.The three must-fixes — all mine, all fair
1. B2 shipped with zero test coverage. B2's root cause was untested divergence between create and update, and my fix re-established agreement with nothing pinning it. Now:
MultiCollectionWeightValidationTestsdrives both paths from one shared case list (0, -1, -5, 1001, int.MaxValuerejected;1, 2, 500, 1000accepted), covers the smart-item mirror, and asserts create and update agree on every case — the per-path tests would both have passed while the two diverged, which is exactly how the bug existed. Non-vacuity proven: inverting only the update path's validation fails 10 of 20 on a clean build (0 errors, so not a stale-dll pass), and the agreement test is among the failures.2. My own clamp made four rationale statements false — two authored by the same commit, including the stated justification for
Minimum = 1inMultiCollectionItemWeight.EffectiveWeightremoved the weight filter, yet the prose still said a 0-weight source "is filtered out". As the reviewer noted, a future agent could read that and delete the clamp or the floor as redundant. Corrected to describe what the code actually does: the gate refuses input that means nothing on a share-of-airtime scale; the clamp protects rows predating the gate; neither is redundant.3. I miscounted the writers in the very bullet whose lesson is "grep every writer of the field".
ReplaceBlockItemswritesBlockItem.PlaybackOrder, notPlaylistItem.PlaybackOrder— I conflated the two fields. There are two persisting writers ofPlaylistItem.PlaybackOrder. The lesson now reads: grep every writer of each field separately. That the correction itself repeated the original error's shape is recorded indecisions.mdrather than quietly fixed.Verification on
48b270e8Build succeeded. 0 Error(s)· Core.Tests 565 passed · ErsatzTV.Tests 1673 passed · 0 failed · non-vacuity control 10/20 fail-on-inversion, reverted.Outstanding nit (not fixed, tracked)
v1.jsonpublishesweightwithdefault: 1but nominimum/maximum, so clients discover the 1..1000 bound only via the error response. Worth a[Range]on the request DTO, but it changes the validation surface (ModelState 400 vs the handler'sBaseError), so it isn't a drive-by.Still open on #70's Done-when (3 of 10)
classic-weightedgolden · live-E2E · a clean verdict on this commit. This PR is not ready to merge, and the H6 gate correctly blocks it.Review-verdict: MERGEABLE-WITH-NITS @
2c0be68905CI green on
d4f9ac82Run #744 — every job success (image push skipped as expected on a PR):
AddMultiCollectionItemWeightto a fresh DBBoth jobs that were red on the previous head are now green:
.editorconfigforbids (charset=utf-8). Self-inflicted: the patches that edited those legacy files wrote them back asutf-8-sigto "preserve the existing style", but the #311 fix-as-you-touch gate exists precisely so a touched file gets normalized. Fixed in27ecaedb, verified encoding-only (every hunk is-namespace→+namespace).v1.json/v1.d.ts/endpoint-index.mdreports in-sync, and it passes here on a healthy host.Two corrections to notes I was carrying, recorded so the next session doesn't inherit them:
dotnet format --includedoes NOT no-op on this Mac. It reproduced CI exactly (error CHARSET, exit 2) and fixed the files in place. The memory note claiming otherwise is wrong for this invocation.odpattern was wrong. Reading the first three bytes directly found them. A detector that can only return "ok" is worse than none.Done-when: 9/10
Remaining: an independent review verdict against this head (H10 — the prior
MERGEABLE-WITH-NITSreferences2c0be689, which is stale). In flight; it covers the weight-gate tests, the doc corrections, the BOM commit, and theclassic-weightedgolden.The PR is not merge-ready until that lands, and the H6 gate correctly blocks it.
Review round 3 — BLOCKED @
d4f9ac82; fixed in3bca1f6dThe reviewer was right, and the root cause it named is the useful part:
Fixing the list is not fixing the class. That is the same failure as B1 (the gate covered the two writers already in hand and missed
CreateChannelFromLineup) — repeated while writing the correction whose stated lesson is "grep every writer of the field". Third round, same shape.What it caught
CreateMultiCollectionHandler.cs:95A_Non_Positive_Weight_Does_Not_Delete_The_Source,An_Enormous_Weight_Does_Not_Overflow_The_Rotation).decisions.md:1306EffectiveWeightclamps before every sum andCycleLengthwidens tolong. Same file I claimed to have fixed.Add*ToPlaylistandTraktCommandBasedo persist the field (hardcoded). The true claim is two writers that persist a caller-supplied order. Also missedEngine/PlaylistHelperas a third in-memory writer.What it verified rather than took on faith
ValidateWeightsis dropped from either path. It also correctly notesUpdate_And_Create_Agree_On_Every_Caseis tautological in isolation (it detects divergence, not absence) — but honestly scoped, with the per-path suites carrying the absence case.TZ=UTC,Asia/Kolkata,Pacific/Kiritimati; no fixture collision; missing golden hard-fails.Fixed in
3bca1f6d— by grep, not by listRe-grepped the whole behavior class (
filtered out|removes the source|overflow∩weight|rotation|sum) rather than working the four cited sites. Every surviving hit is now an accurate statement (what the clamp prevents, a test name, or an explicit historical note).The ceiling rationale now says what is actually true: it is the floor's argument, not an arithmetic guard — a billion is not a share of airtime any more than 0 is, so it is refused at the boundary; the clamp is what makes the arithmetic safe, which is exactly why a row predating the gate is still safe.
The writer bullet now classifies the full set (persists-caller-value / persists-hardcoded / in-memory) and records that it has been wrong three times in the same shape — a lesson that keeps being re-learned is worth recording as a pattern rather than a fact.
Verification: Build 0 errors · Core.Tests 566 · ErsatzTV.Tests 1673 · 0 failed · format verify exit 0 · decisions.md +90/−0 (append-only green).
The pre-push BOM check caught this very commit re-adding a BOM to the one file patched with
utf-8-sig— the same trap, an hour after writing it down. That is the argument for the mechanical check over the memory of it.Re-review of
3bca1f6ddispatched: the last two rounds each found real defects in what I claimed was complete, so my say-so is not the evidence.Review round 5 — MERGEABLE @
c556bbd3The stale-claim class is closed. The reviewer ran its own exhaustive sweep (every added weight/enumerator comment across all 36 non-generated changed files, plus an independent grep of
Scheduling/,Data/,MediaCollections/,docs/forfilter|drop|remove|vanish|delet|exclud|omit|skipnearweight) rather than reusing my method — and classified every hit:MultiCollectionConfiguration.cs:37) and itsdecisions.mdtwin are now mutually consistent — the twin pattern that recurred every round is closed.PlaylistEnumerator, which has nodefault:arm and gates onenumerator is not nullat :210) verified as true, not stale — they are about orders, not weights.Sanity: Build 0/0. Weighted+golden filter 21 passed (incl. the non-vacuous
classic-weightedgolden and theA_Non_Positive_Weight_Does_Not_Delete_The_Source/An_Enormous_Weight_Does_Not_Overflow_The_Rotationguards). Weight+lineup filter 46 passed. No BOM on touched non-generated.cs.decisions.mdinsertion-only.Five rounds, and rounds 3–5 were entirely about prose contradicting code — the code was correct after round 2. Recorded honestly because it's the real cost of this PR: I repeatedly fixed the sites a reviewer named instead of the class, and the fix for stale comments is discipline, not a gate (unlike the BOM class, which got #413).
Review-verdict: MERGEABLE @ c556bbd3
c556bbd3a4to35e4b29a8fRebased onto current
main(was 6 behind). Clean rebase —git range-diff origin/main c556bbd3 35e4b29ashows all 12 commits as=(identical), so the round-5 review ofc556bbd3applies to35e4b29averbatim; no content changed, only the replay base. Re-verified on the new head: Build 0/0, Core.Tests 566, ErsatzTV.Tests 1673,check:apiexit 0, BOM-clean.Review-verdict: MERGEABLE @ 35e4b29a
35e4b29a8fto4620dc91aeRebased again onto current
main(was 8 behind; #413's BOM guard is now among the base commits). One conflict — the usualdecisions.mdappend-collision with a parallel session's entry — resolved by keeping both entries (content-preserving;git range-diffshows 11/12 commits identical to the reviewed35e4b29a, the 12th being only the re-resolveddecisions.md). Re-verified on4620dc91: Build 0/0, Core.Tests 566, ErsatzTV.Tests 1673,check:apiexit 0, BOM-clean (now enforced by the merged #413 guard).Review-verdict: MERGEABLE @ 4620dc91
4620dc91aeto4693aa14c3Rebased onto latest
main— #405 (channel-health, #72) landed and touches the same MultiCollection files, so this rebase carried real overlap, not just thedecisions.mdappend (git merged the disjoint regions cleanly; only the append collided, resolved keeping both). Re-verified fully because of that overlap: Build 0/0, Core.Tests 566, ErsatzTV.Tests 1676 (main brought new tests),check:apiexit 0, BOM-clean (guard from #413 now enforcing). Head4693aa14.Review-verdict: MERGEABLE @
4693aa14