diff --git a/pkg/service/marge/addsource_storedmusic_test.go b/pkg/service/marge/addsource_storedmusic_test.go new file mode 100644 index 0000000..4f3af9f --- /dev/null +++ b/pkg/service/marge/addsource_storedmusic_test.go @@ -0,0 +1,97 @@ +package marge + +import ( + "os" + "strconv" + "testing" + + "github.com/gesellix/bose-soundtouch/pkg/service/constants" + "github.com/gesellix/bose-soundtouch/pkg/service/datastore" +) + +// TestAddSource_MultipleStoredMusicServersCoexist is a regression test for the +// media-server eviction bug: AddSource deduped STORED_MUSIC by provider ID, so +// registering a second DLNA media server overwrote the first. The first then +// vanished from /full + /sources and the speaker dropped it, meaning only one +// media server could ever stay registered. +// +// Each media server is a separate account (username = "/0"), so two +// distinct servers must coexist, while re-adding the same server (same account) +// updates in place. +func TestAddSource_MultipleStoredMusicServersCoexist(t *testing.T) { + tempDir, err := os.MkdirTemp("", "addsource-storedmusic-*") + if err != nil { + t.Fatalf("temp dir: %v", err) + } + + defer func() { _ = os.RemoveAll(tempDir) }() + + ds := datastore.NewDataStore(tempDir) + account := "6919733" + device := "A81B6A536A98" + + if mkErr := os.MkdirAll(ds.AccountDeviceDir(account, device), 0o755); mkErr != nil { + t.Fatalf("mkdir device dir: %v", mkErr) + } + + sm := strconv.Itoa(constants.StoredMusicProviderID) + + const ( + fritzAcct = "fa095ecc-e13e-40e7-8e6c-e0286d5bc000/0" + testAcct = "4d696e69-444c-164e-9d41-72ecda78e4c1/0" + ) + + // Register two different media servers. + if _, err := AddSource(ds, account, fritzAcct, sm, "", "", "fritz"); err != nil { + t.Fatalf("add server 1: %v", err) + } + + if _, err := AddSource(ds, account, testAcct, sm, "", "", "AfterTouch Test Library"); err != nil { + t.Fatalf("add server 2: %v", err) + } + + // storedMusicAccounts returns the set of STORED_MUSIC source accounts the + // datastore would serve via /full + /sources. SourceKey.Account is the + // persisted identity (the display name lives on the speaker, set via + // setMusicServiceAccount, and does not round-trip here). + storedMusicAccounts := func() map[string]bool { + sources, gerr := ds.GetConfiguredSources(account, device) + if gerr != nil { + t.Fatalf("get sources: %v", gerr) + } + + out := map[string]bool{} + + for _, s := range sources { + if s.SourceProviderID == sm { + out[s.SourceKey.Account] = true + } + } + + return out + } + + got := storedMusicAccounts() + if len(got) != 2 { + t.Fatalf("expected 2 STORED_MUSIC sources, got %d: %+v", len(got), got) + } + + if !got[fritzAcct] { + t.Errorf("first media server was evicted (account %q missing)", fritzAcct) + } + + if !got[testAcct] { + t.Errorf("second media server not registered (account %q missing)", testAcct) + } + + // Re-adding the SAME server (same account) updates in place; it must not + // create a duplicate or drop the other server. + if _, err := AddSource(ds, account, fritzAcct, sm, "", "", "fritz (renamed)"); err != nil { + t.Fatalf("re-add server 1: %v", err) + } + + got = storedMusicAccounts() + if len(got) != 2 || !got[fritzAcct] || !got[testAcct] { + t.Fatalf("re-adding the same server should keep exactly both accounts; got %+v", got) + } +} diff --git a/pkg/service/marge/marge.go b/pkg/service/marge/marge.go index bf7ca8f..acb5378 100644 --- a/pkg/service/marge/marge.go +++ b/pkg/service/marge/marge.go @@ -4,6 +4,8 @@ package marge import ( "bytes" + "crypto/rand" + "encoding/hex" "encoding/xml" "fmt" "log" @@ -2322,11 +2324,27 @@ func RemoveSourceFromAccount(ds *datastore.DataStore, account, sourceID string) return nil } +// newSourceID returns a unique opaque source ID. SaveConfiguredSources dedups +// by ID, so it must be collision-free even for sources created in the same +// instant; it uses crypto/rand (64 bits) and falls back to a nanosecond +// timestamp only if the RNG ever fails. +func newSourceID() string { + var b [8]byte + if _, err := rand.Read(b[:]); err != nil { + return "SRC_" + strconv.FormatInt(time.Now().UnixNano(), 10) + } + + return "SRC_" + hex.EncodeToString(b[:]) +} + // AddSource adds a new music source to the account and returns the generated source ID. func AddSource(ds *datastore.DataStore, account, username, providerID, secret, secretType, sourceName string) (string, error) { now := time.Now() createdOn := FormatTime(now) - sourceID := "SRC_" + strconv.FormatInt(now.Unix(), 10) + // SaveConfiguredSources dedups by ID, so IDs must be unique even when two + // sources are added in the same instant. Use a random ID rather than a + // timestamp (which can collide on coarse clocks or rapid calls). + sourceID := newSourceID() // List accounts directly from the account directory to be sure we find them. devicesDir := ds.AccountDevicesDir(account) @@ -2368,11 +2386,24 @@ func AddSource(ds *datastore.DataStore, account, username, providerID, secret, s PrepareConfiguredSource(&newSrc) - // Update or append. If it's the same provider, we replace it. + // Update or append. Most providers are singletons (one account each), so + // the same provider replaces the existing entry. STORED_MUSIC is the + // exception: each DLNA media server is a separate account (username = + // "/0"), so it must only replace when the account also matches. + // Otherwise registering a second media server overwrites the first, which + // then vanishes from /full + /sources and the speaker drops it (only one + // media server could ever stay registered). replaced := false for i := range sources { - if sources[i].SourceProviderID == providerID || + sameProvider := sources[i].SourceProviderID == providerID + if providerID == strconv.Itoa(constants.StoredMusicProviderID) { + // Match on the persisted account identity (SourceKey.Account), + // not Username, which does not round-trip through the datastore. + sameProvider = sameProvider && sources[i].SourceKey.Account == username + } + + if sameProvider || (providerID == strconv.Itoa(constants.SpotifyProviderID) && sources[i].SourceKey.Type == constants.ProviderSpotify) { sources[i] = newSrc replaced = true