bvandeusen/minstrel
1
0
Fork 0
Code Issues 2 Pull Requests Actions Packages Projects Releases 52 Wiki Activity

docs: server comments say what the code does, not how it came to be (#6277)

Browse Source 
Comment blocks in cmd/minstrel and 30 smaller internal packages, rewritten
to rule 32's standard with each citation read in context: task, milestone
and rule ids go, and so do the stories they were attached to ("used to",
"Before #N", measurements from one install, quotes, pointers to Fabled
Librarian's records and to docs/superpowers files that are not in the
repo). Where a story wrapped a real constraint, the constraint stays as a
present-tense fact.

Comments that had gone stale are corrected:
- taste.Config: the tuned values are overlaid by internal/recsettings;
  it is no longer "a later follow-up".
- StorageConfig: the data dir holds art, caches and the stream secret,
  not only playlist covers.
- the Subsonic album list and getUser comments describe what they do
  today rather than a milestone that would change it.

Checked: the non-comment token stream of every changed file matches
HEAD, and gofmt -l is empty.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
bvandeusen
2026-10-23 12:15:05 -04:00
co-authored by Claude Opus 5.5
parent 9f9609591d
commit d191a28217
107 changed files with 485 additions and 597 deletions
Download Patch File Download Diff File Expand all files Collapse all files
internal/playevents/writer.go
+21 -34
View File
@@ -33,7 +33,7 @@ type Writer struct {
}
// NewWriter constructs a writer. timeout is the play-session window; the two
// remaining args are the skip rule thresholds from spec §6.
// remaining args are the skip rule's thresholds.
func NewWriter(
pool *pgxpool.Pool,
logger *slog.Logger,
@@ -78,23 +78,12 @@ func strPtrOrNil(s string) *string {
}
// systemPlaylistSources are the play_events.source values that count
// against a system playlist's rotation (Fable #415). Mirrors the
// `playlists_kind_variant_consistent` CHECK in migration
// 0028_discovery_mix_variants.up.sql — every variant in that CHECK
// list that ships as a refreshable system mix needs to be here so
// the rotation reporter sees Android + web plays from that surface.
//
// Drift #563: this map drifted behind the migrations. It had only
// for_you + discover but migrations 0021 + 0028 added 6 more
// variants. Plays from Rediscover / Deep Cuts / Songs Like X /
// New for You / On This Day / First Listens didn't advance the
// per-user rotation, so "unplayed first" ordering staled on those
// mixes — same tracks kept surfacing.
//
// Future variant additions should update this map AND the CHECK in
// the matching migration in the same change; the
// `db_check_constraint_for_new_variants` standing rule covers the
// CHECK half.
// against a system playlist's rotation. Every variant in the
// `playlists_kind_variant_consistent` CHECK that ships as a refreshable
// system mix must be here: a variant missing from this map never advances
// its per-user rotation, so "unplayed first" goes stale and the same tracks
// keep surfacing. A new variant updates this map and that CHECK (in its
// migration) in the same change.
var systemPlaylistSources = map[string]bool{
"for_you": true,
"discover": true,
@@ -107,10 +96,10 @@ var systemPlaylistSources = map[string]bool{
}
// lookupSystemPickKind resolves a system-playlist play's pick_kind
// (taste/fresh, Discover's bucket, or a rule-#131 tier) from the user's
// (taste/fresh, Discover's bucket, or an eligibility tier) from the user's
// live snapshot of that variant at ingestion time. Attribution must be
// frozen now — snapshots rebuild daily, so it can't be reconstructed at
// metrics-read time (#1249, generalized in #1270). Source strings
// metrics-read time. Source strings
// double as system_variant values (see systemPlaylistSources). Returns
// nil (unattributed) when the track isn't in the current snapshot —
// plays predating the feature, or an offline replay arriving after the
@@ -140,8 +129,8 @@ func lookupSystemPickKind(
// tag identifying which surface the play came from. When source is a
// known system-playlist kind the track is appended to that user's
// rotation set so shuffle-on-play can prefer the unplayed tail on a
// re-play within the day. Empty source → behaves exactly like the
// pre-#415 path (NULL column, no rotation write).
// re-play within the day. An empty source behaves exactly like
// RecordPlayStarted (NULL column, no rotation write).
func (w *Writer) RecordPlayStartedWithSource(
ctx context.Context,
userID, trackID pgtype.UUID,
@@ -209,7 +198,7 @@ func (w *Writer) RecordPlayStartedWithSource(
}
// RecordPlayEnded closes a play_event with duration + computed completion
// ratio. Applies the spec §6 skip rule (AND of completion < threshold AND
// ratio. Applies the skip rule (AND of completion < threshold AND
// duration_played < threshold) — both conditions must hold for the row to
// be marked was_skipped=true. If skipped, also writes a skip_events row.
func (w *Writer) RecordPlayEnded(
@@ -232,7 +221,7 @@ func (w *Writer) RecordPlayEnded(
// row is already closed, re-applying the close is harmless (the
// UPDATE is last-write-wins) but a second skip_events insert must
// NOT fire — a lost-response retry would otherwise double the
// skip signal. See the 2026-06-11 contract audit.
// skip signal.
alreadyEnded := ev.EndedAt.Valid
ratio := 0.0
if track.DurationMs > 0 {
@@ -382,17 +371,15 @@ func (w *Writer) RecordSyntheticCompletedPlay(
}
// RecordOfflinePlay writes a complete start+end play in one call from
// a caller-supplied timestamp + duration. This is the replay path for
// the Flutter offline mutation queue (#426 part B): a play that
// happened with no/flaky connectivity is captured locally as a
// completed unit and replayed here once back online, preserving the
// original `at`.
// a caller-supplied timestamp + duration. This is the replay path for a
// client's offline queue: a play that happened with no or flaky
// connectivity is captured locally as a completed unit and replayed here
// once back online, preserving the original `at`.
//
// Generalizes RecordSyntheticCompletedPlay: instead of assuming full
// completion it applies the spec §6 skip rule from the real
// durationPlayed vs track length (same AND-of-thresholds as
// RecordPlayEnded), and threads `source` so #415 rotation advances
// for system-playlist plays just like the live path.
// completion it applies the skip rule from the real durationPlayed vs
// track length (same AND-of-thresholds as RecordPlayEnded), and threads
// `source` so system-playlist rotation advances just like the live path.
//
// durationPlayedMs is clamped to [0, track duration] to absorb
// client clock / position drift.
@@ -414,7 +401,7 @@ func (w *Writer) RecordOfflinePlay(
// after commit makes the client re-fire the same play. The replayed
// payload carries the original `at`, so an exact (user, track,
// started_at) match means we already recorded it — no-op instead of a
// duplicate history row. See the 2026-06-11 contract audit.
// duplicate history row.
var dupID pgtype.UUID
err = tx.QueryRow(ctx,
`SELECT id FROM play_events
internal/playevents/writer_test.go
+4 -4
View File
@@ -220,8 +220,8 @@ func TestRecordPlayEnded_DurationOver50Percent_NotSkip(t *testing.T) {
}
// A lost-response retry of an offline play (same user/track/started_at)
// must not create a second history row. Guards the at-least-once replay
// path added in the 2026-06-11 audit.
// must not create a second history row: the offline replay is
// at-least-once.
func TestRecordOfflinePlay_DedupsByUserTrackStartedAt(t *testing.T) {
f := newFixture(t, 200_000)
ctx := context.Background()
@@ -491,7 +491,7 @@ func TestRecordPlayStarted_NoRotationWithoutSource(t *testing.T) {
// seedSystemSnapshot creates a system playlist of the given variant
// containing the fixture track with the given pick_kind, mimicking
// what the builder persists (#1249, generalized #1270).
// what the builder persists.
func seedSystemSnapshot(t *testing.T, f fixture, variant, pickKind string) {
t.Helper()
pl, err := f.q.CreateSystemPlaylist(context.Background(), dbq.CreateSystemPlaylistParams{
@@ -567,7 +567,7 @@ func TestRecordPlayStartedWithSource_CrossVariant_PickKindNull(t *testing.T) {
}
func TestRecordPlayStartedWithSource_StampsDiscoverBucket(t *testing.T) {
// Discover stamps its candidate bucket (#1270); a discover play whose
// Discover stamps its candidate bucket; a discover play whose
// track is in the live snapshot freezes that bucket onto the play.
f := newFixture(t, 200_000)
seedSystemSnapshot(t, f, "discover", "dormant")