From 393b31ad935aedb49c9ea76f5bcf099f91fd19e8 Mon Sep 17 00:00:00 2001 From: Tobias Gesellchen Date: Sun, 14 Jun 2026 20:46:37 +0200 Subject: [PATCH] fix(marge): stop recents move-to-front from dropping/duplicating entries Re-playing an existing recent could make it (and its list neighbour) vanish from the speaker's recents, even with the list well under the 10-item cap. Root cause is a slice-aliasing bug in updateOrCreateRecent's move-to-front branch: recentObj = &recents[i] recents = append([]ServiceRecent{*recentObj}, append(recents[:i], recents[i+1:]...)...) return recentObj, recents The inner append(recents[:i], recents[i+1:]...) shifts elements left in place in the shared backing array, overwriting slot i. The returned recentObj still points at &recents[i], so it leaks the neighbouring recent back to the speaker. Worse, Go does not specify evaluation order between the *recentObj dereference and the inner append call, so the front element written into the saved list can also read the overwritten slot, dropping the matched recent and duplicating its neighbour. The SaveRecents dedup-by-ID guard then collapses that duplicate into a clean loss. Verified against recorded interactions (a "White Water" replay returned the "Sand Castle" recent; both Spotify albums vanished from a 9-item list) and a live diagnostic export (6 persisted recents, no duplicates, both albums gone). Fix: copy the matched recent out first, rebuild into a fresh backing array, and return a pointer into the new slice. Adds a regression test that fails on the old code and passes now. Co-Authored-By: Claude Opus 4.8 (1M context) --- pkg/service/marge/marge.go | 19 ++++- pkg/service/marge/recent_movetofront_test.go | 81 ++++++++++++++++++++ 2 files changed, 96 insertions(+), 4 deletions(-) create mode 100644 pkg/service/marge/recent_movetofront_test.go diff --git a/pkg/service/marge/marge.go b/pkg/service/marge/marge.go index 58355d0..96f0746 100644 --- a/pkg/service/marge/marge.go +++ b/pkg/service/marge/marge.go @@ -2019,11 +2019,22 @@ func updateOrCreateRecent(recents []models.ServiceRecent, name string, matchingS if sourceMatch && r.Location == location { recents[i].UtcTime = strconv.FormatInt(utcTime, 10) recents[i].UpdatedOn = FormatTime(time.Now()) - recentObj = &recents[i] - // Move to front - recents = append([]models.ServiceRecent{*recentObj}, append(recents[:i], recents[i+1:]...)...) - return recentObj, recents + // Move the matched recent to the front. Copy the value out FIRST, + // then rebuild the slice into a fresh backing array. The previous + // in-place `append(recents[:i], recents[i+1:]...)` shuffle aliased + // the shared backing array and overwrote index i, which both + // corrupted the returned pointer (it pointed at the neighbour) and, + // because Go does not specify evaluation order between `*recentObj` + // and the inner append, could drop the matched recent and duplicate + // its neighbour in the saved list. + matched := recents[i] + reordered := make([]models.ServiceRecent, 0, len(recents)) + reordered = append(reordered, matched) + reordered = append(reordered, recents[:i]...) + reordered = append(reordered, recents[i+1:]...) + + return &reordered[0], reordered } } diff --git a/pkg/service/marge/recent_movetofront_test.go b/pkg/service/marge/recent_movetofront_test.go new file mode 100644 index 0000000..47a55f1 --- /dev/null +++ b/pkg/service/marge/recent_movetofront_test.go @@ -0,0 +1,81 @@ +package marge + +import ( + "testing" + + "github.com/gesellix/bose-soundtouch/pkg/models" +) + +// TestUpdateOrCreateRecent_MoveToFrontPreservesList is a regression test for the +// recents move-to-front corruption: when an existing recent is re-played, the +// in-place slice shuffle at the match branch could (a) return the wrong recent +// (a list neighbor) and (b) drop or duplicate entries in the saved list. +// +// Live evidence (account 6919733 / device A81B6A536A98, 2026-06-14): re-playing +// the Spotify album "White Water" returned the neighboring "Sand Castle Tapes" +// recent, and both Spotify recents subsequently vanished from a list that was +// well under the 10-item cap. +func TestUpdateOrCreateRecent_MoveToFrontPreservesList(t *testing.T) { + spotify := &models.ConfiguredSource{} + spotify.SourceKeyType = "SPOTIFY" + spotify.SourceKeyAccount = "gesellix" + + mk := func(id, name, loc string) models.ServiceRecent { + var r models.ServiceRecent + r.ID = id + r.Name = name + r.Source = "SPOTIFY" + r.SourceAccount = "gesellix" + r.Location = loc + + return r + } + + // Three distinct Spotify recents sharing the same source; they differ only + // by location (the discriminator in the match branch). + recents := []models.ServiceRecent{ + mk("1", "Sunday", "loc-sunday"), + mk("2", "White Water", "loc-white"), + mk("3", "Sand Castle", "loc-sand"), + } + + // Re-play "White Water" (the middle entry) -> it should move to front, + // the returned recent must BE White Water, and no entry may be lost. + recentObj, out := updateOrCreateRecent(recents, "White Water", spotify, "tracklisturl", "loc-white", "DEVICEID01", 12345) + + if recentObj.Name != "White Water" || recentObj.Location != "loc-white" { + t.Errorf("returned recent = %q (loc %q), want White Water/loc-white (neighbor leakage)", recentObj.Name, recentObj.Location) + } + + if len(out) != 3 { + t.Fatalf("recents count = %d, want 3 (entry lost/duplicated): %s", len(out), names(out)) + } + + seen := map[string]int{} + for i := range out { + seen[out[i].Location]++ + } + + for _, loc := range []string{"loc-sunday", "loc-white", "loc-sand"} { + if seen[loc] != 1 { + t.Errorf("location %q appears %d times, want 1: %s", loc, seen[loc], names(out)) + } + } + + if out[0].Location != "loc-white" { + t.Errorf("front entry = %q, want White Water moved to front: %s", out[0].Name, names(out)) + } +} + +func names(rs []models.ServiceRecent) string { + s := "[" + for i := range rs { + if i > 0 { + s += ", " + } + + s += rs[i].Name + "(" + rs[i].Location + ")" + } + + return s + "]" +}