From f26176fad44e74df7b24dc70a09c50a467899c43 Mon Sep 17 00:00:00 2001 From: Tobias Gesellchen Date: Sat, 30 May 2026 23:07:41 +0200 Subject: [PATCH] fix(marge): never persist or serve sources without a resolvable provider id MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Root cause of #334's INVALID_SOURCE: a speaker reports device-local slots (STORED_MUSIC_MEDIA_RENDERER, UPNP) in /sources; AfterTouch imports them verbatim and re-serves them in /full. PrepareConfiguredSource fills sourceproviderid only for types in constants.StaticProviders, so these go out with an empty — a required protobuf field — and the speaker rejects them as INVALID_SOURCE, which then re-syncs back into the datastore. Fix, keyed on the principle (no hardcoded denylist in production): - HasResolvableProviderID(s): true if the source already carries a provider id, or its source-key type resolves via StaticProviders. - Serve-side guard in getAccountSources: drop any source whose resolved sourceproviderid is still empty (generalises the existing AUX/#195 skip). Heals already-polluted datastores on the next /full, no resync needed. - Import-side filter in syncConfiguredSources (marge) and both branches of syncSources (setup): drop unresolvable sources before persisting, stopping future pollution and the re-import loop. Tests: reproduction converted to regression test (TestI334FullOmitsSourcesWithoutProviderID) seeded from a sanitised real #334 /sources capture; explicit servable/non-servable tables in TestHasResolvableProviderID. Two pre-existing fixtures that relied on sources with no provider id were given valid ones. Co-Authored-By: Claude Opus 4.8 (1M context) --- pkg/service/handlers/handlers_marge_test.go | 15 +- .../marge/i334_sources_roundtrip_test.go | 200 ++++++++++++++++++ pkg/service/marge/marge.go | 53 ++++- pkg/service/marge/repro_test.go | 9 + pkg/service/marge/sync.go | 24 ++- .../marge/testdata/i334_speaker_sources.xml | 32 +++ pkg/service/setup/setup.go | 39 ++++ 7 files changed, 361 insertions(+), 11 deletions(-) create mode 100644 pkg/service/marge/i334_sources_roundtrip_test.go create mode 100644 pkg/service/marge/testdata/i334_speaker_sources.xml diff --git a/pkg/service/handlers/handlers_marge_test.go b/pkg/service/handlers/handlers_marge_test.go index c126d21..ef6aca1 100644 --- a/pkg/service/handlers/handlers_marge_test.go +++ b/pkg/service/handlers/handlers_marge_test.go @@ -422,11 +422,18 @@ func TestMargeAccountSources(t *testing.T) { deviceDir := filepath.Join(tempDir, "accounts", account, "devices", deviceID) _ = os.MkdirAll(deviceDir, 0755) - // Mock Sources.xml + // Mock Sources.xml. PANDORA is used because (a) it maps to a + // constants.StaticProviders entry, so HasResolvableProviderID accepts it + // and ensureSourceProviderID fills its providerid — sources with no + // resolvable providerid are now filtered before serving, since the speaker + // rejects an empty as INVALID_SOURCE (issue #334); and + // (b) it is not one of the username-blanking types + // (TUNEIN/INTERNET_RADIO/LOCAL_INTERNET_RADIO), so the serve path still + // emits , keeping that assertion meaningful. sourcesXML := ` - - + + ` _ = os.WriteFile(filepath.Join(deviceDir, "Sources.xml"), []byte(sourcesXML), 0644) @@ -456,7 +463,7 @@ func TestMargeAccountSources(t *testing.T) { t.Errorf("Response missing expected source ID: %s", bodyStr) } - // Verify current XML structure produced by marge.go + // Verify current XML structure produced by marge.go. expectedSnippets := []string{ "", " syncSources, +// pkg/service/setup/setup.go). It is a verbatim field copy: the speaker's +// becomes ConfiguredSource.SourceKey.Type=X with no +// ID, no protocol Type and no SourceProviderID. Replicated here (rather than +// imported) because the upstream is inline I/O code and the field mapping is +// the input to the real /full builder, not the thing under test. +func configuredFromSpeakerSources(srs models.Sources) []models.ConfiguredSource { + var out []models.ConfiguredSource + + for _, s := range srs.SourceItem { + cs := models.ConfiguredSource{DisplayName: s.DisplayName} + + if s.Status == models.SourceStatusReady { + cs.SecretType = constants.CredentialTypeToken + } + + if s.Source == constants.ProviderSpotify { + cs.SecretType = constants.CredentialTypeTokenV3 + } + + cs.SourceKey.Type = s.Source + cs.SourceKey.Account = s.SourceAccount + cs.SourceKeyType = s.Source + cs.SourceKeyAccount = s.SourceAccount + + out = append(out, cs) + } + + return out +} + +// TestI334FullOmitsSourcesWithoutProviderID guards the #334 fix: a speaker's +// legitimate /sources list (the fixture has zero INVALID_SOURCE entries) is +// imported, persisted, and then turned into the /full block by +// getAccountSources. After the fix: +// +// - Every emitted source must have a non-empty SourceProviderID; any source +// without one (STORED_MUSIC_MEDIA_RENDERER, UPNP, …) would be rejected by +// the speaker as INVALID_SOURCE, creating the feedback loop that #334 +// identified. +// - The two specific device-local slots from v-tron's datastore +// (StoredMusicUserName / STORED_MUSIC_MEDIA_RENDERER and UPnPUserName / +// UPNP) must be absent from the emitted set entirely. +// +// Precedent: AUX was excluded the same way for issue #195 (see the comment +// block immediately before the ProviderAux continue in getAccountSources). +// This test extends that pattern to all sources whose type has no entry in +// constants.StaticProviders. +func TestI334FullOmitsSourcesWithoutProviderID(t *testing.T) { + raw, err := os.ReadFile(filepath.Join("testdata", "i334_speaker_sources.xml")) + if err != nil { + t.Fatalf("read fixture: %v", err) + } + + var srs models.Sources + if err := xml.Unmarshal(raw, &srs); err != nil { + t.Fatalf("unmarshal fixture: %v", err) + } + + imported := configuredFromSpeakerSources(srs) + if len(imported) == 0 { + t.Fatal("fixture produced no sources") + } + + // Filter as the import path does: drop unresolvable sources before save. + var servable []models.ConfiguredSource + for _, s := range imported { + if HasResolvableProviderID(s) { + servable = append(servable, s) + } + } + + const ( + account = "ACCT01" + device = "DEVICEID01" + ) + + ds := datastore.NewDataStore(t.TempDir()) + if err := ds.SaveConfiguredSources(account, device, servable); err != nil { + t.Fatalf("save configured sources: %v", err) + } + + // Real /full source build: merge defaults + PrepareConfiguredSource + + // mapToFullResponseSource, exactly as AccountFullToXML invokes it. + full := getAccountSources(ds, account, device) + + t.Logf("%-26s | %-8s | %-13s | %s", "out.name", "out.type", "out.provider", "credential.type") + for _, s := range full { + t.Logf("%-26q | %-8q | %-13q | %q", s.Name, s.Type, s.SourceProviderID, s.Credential.Type) + } + + // Every emitted source must have a non-empty SourceProviderID. + for _, s := range full { + if s.SourceProviderID == "" { + t.Errorf("source name=%q was emitted with empty SourceProviderID — it would be rejected as INVALID_SOURCE (#334)", s.Name) + } + } + + // The two specific device-local slots from the fixture must be absent. + emittedNames := make(map[string]bool, len(full)) + for _, s := range full { + emittedNames[s.Name] = true + } + + for _, mustBeAbsent := range []string{"StoredMusicUserName", "UPnPUserName"} { + if emittedNames[mustBeAbsent] { + t.Errorf("source name=%q must not appear in /full (device-local slot with no resolvable sourceproviderid, issue #334)", mustBeAbsent) + } + } +} + +// TestHasResolvableProviderID verifies the HasResolvableProviderID predicate +// used to filter sources before persistence and before serving via /full. +func TestHasResolvableProviderID(t *testing.T) { + // Sources that MUST be servable (true): their SourceKey.Type maps to a + // constants.StaticProviders entry, so ensureSourceProviderID can fill the + // canonical providerid. + trueTable := []struct { + name string + typ string + }{ + {"TUNEIN", constants.ProviderTunein}, + {"SPOTIFY", constants.ProviderSpotify}, + {"RADIO_BROWSER", constants.ProviderRadioBrowser}, + {"LOCAL_INTERNET_RADIO", constants.ProviderLocalInternetRadio}, + {"STORED_MUSIC", constants.ProviderStoredMusic}, + {"INTERNET_RADIO", constants.ProviderInternetRadio}, + {"AUX", constants.ProviderAux}, + } + + for _, tc := range trueTable { + t.Run("true/"+tc.name, func(t *testing.T) { + s := models.ConfiguredSource{} + s.SourceKey.Type = tc.typ + + if !HasResolvableProviderID(s) { + t.Errorf("HasResolvableProviderID(%q) = false, want true", tc.typ) + } + }) + } + + // Sources that MUST NOT be servable (false): no StaticProviders entry, + // so ensureSourceProviderID would leave SourceProviderID empty and the + // speaker would reject the source as INVALID_SOURCE. + falseTable := []struct { + name string + typ string + }{ + {"STORED_MUSIC_MEDIA_RENDERER", "STORED_MUSIC_MEDIA_RENDERER"}, + {"UPNP", "UPNP"}, + {"INVALID_SOURCE", "INVALID_SOURCE"}, + {"empty type", ""}, + } + + for _, tc := range falseTable { + t.Run("false/"+tc.name, func(t *testing.T) { + s := models.ConfiguredSource{} + s.SourceKey.Type = tc.typ + + if HasResolvableProviderID(s) { + t.Errorf("HasResolvableProviderID(%q) = true, want false", tc.typ) + } + }) + } + + // A source that already carries a non-empty SourceProviderID must return + // true regardless of its type — the existing providerid wins. + t.Run("true/existing-providerid-overrides-type", func(t *testing.T) { + s := models.ConfiguredSource{SourceProviderID: "99"} + s.SourceKey.Type = "STORED_MUSIC_MEDIA_RENDERER" // normally false + + if !HasResolvableProviderID(s) { + t.Error("HasResolvableProviderID with non-empty SourceProviderID = false, want true") + } + }) + + // Legacy SourceKeyType field must also be checked when SourceKey.Type is empty. + t.Run("true/legacy-sourcekeytype", func(t *testing.T) { + s := models.ConfiguredSource{SourceKeyType: constants.ProviderTunein} + // SourceKey.Type intentionally left empty + + if !HasResolvableProviderID(s) { + t.Errorf("HasResolvableProviderID with SourceKeyType=%q (SourceKey.Type empty) = false, want true", constants.ProviderTunein) + } + }) +} diff --git a/pkg/service/marge/marge.go b/pkg/service/marge/marge.go index 8388a96..43a41d1 100644 --- a/pkg/service/marge/marge.go +++ b/pkg/service/marge/marge.go @@ -118,13 +118,45 @@ func ensureSourceType(s *models.ConfiguredSource) { } } +// canonicalProviderIDByName returns the canonical sourceproviderid for a +// symbolic source-key type ("TUNEIN", "SPOTIFY", ...) via constants.StaticProviders. +// Returns the provider ID as a decimal string and ok=true on match, ("", false) otherwise. +func canonicalProviderIDByName(name string) (string, bool) { + for _, p := range constants.StaticProviders { + if p.Name == name { + return strconv.Itoa(p.ID), true + } + } + + return "", false +} + +// HasResolvableProviderID reports whether s can be given a canonical +// sourceproviderid — it already carries one, or its source-key type maps to a +// constants.StaticProviders entry. Sources without one are device-local / +// transient slots (STORED_MUSIC_MEDIA_RENDERER, UPNP, the INVALID_SOURCE +// sentinel, ...) that a speaker rejects as INVALID_SOURCE (issue #334); they +// must not be persisted for, or served to, a speaker. +func HasResolvableProviderID(s models.ConfiguredSource) bool { + if s.SourceProviderID != "" { + return true + } + + if _, ok := canonicalProviderIDByName(s.SourceKey.Type); ok { + return true + } + + if _, ok := canonicalProviderIDByName(s.SourceKeyType); ok { + return true + } + + return false +} + func ensureSourceProviderID(s *models.ConfiguredSource) { if s.SourceProviderID == "" && s.SourceKey.Type != "" { - for _, p := range constants.StaticProviders { - if p.Name == s.SourceKey.Type { - s.SourceProviderID = strconv.Itoa(p.ID) - break - } + if id, ok := canonicalProviderIDByName(s.SourceKey.Type); ok { + s.SourceProviderID = id } } } @@ -1253,7 +1285,16 @@ func getAccountSources(ds *datastore.DataStore, account, lastDeviceID string) [] } PrepareConfiguredSource(&s) - fullSources = append(fullSources, mapToFullResponseSource(s)) + + fs := mapToFullResponseSource(s) + if fs.SourceProviderID == "" { + log.Printf("[Marge] /full: omitting source name=%q (sourceKey type %q) — no resolvable sourceproviderid; would be rejected as INVALID_SOURCE", + sanitizeLog(fs.Name), sanitizeLog(s.SourceKeyType)) + + continue + } + + fullSources = append(fullSources, fs) } return fullSources diff --git a/pkg/service/marge/repro_test.go b/pkg/service/marge/repro_test.go index 07b8cd1..c1b1d6f 100644 --- a/pkg/service/marge/repro_test.go +++ b/pkg/service/marge/repro_test.go @@ -446,6 +446,12 @@ func TestSyncSourcesAggregation(t *testing.T) { t.Fatal(err) } + // Sources include valid values so that HasResolvableProviderID + // accepts them on import. Sources without a resolvable providerid are filtered + // before persistence (issue #334) — bare type="Audio" sources with no providerid + // would be dropped, exactly as device-local transient slots are. Use real + // providerids (LOCAL_INTERNET_RADIO=11, INTERNET_RADIO=2, TUNEIN=25) here so + // the aggregation assertion covers the path that actually reaches SaveConfiguredSources. xmlData := ` @@ -454,6 +460,7 @@ func TestSyncSourcesAggregation(t *testing.T) { Preset Source + 11 Preset Source Name @@ -464,6 +471,7 @@ func TestSyncSourcesAggregation(t *testing.T) { Recent Source + 25 Recent Source Name @@ -472,6 +480,7 @@ func TestSyncSourcesAggregation(t *testing.T) { + 2 Account Source Name diff --git a/pkg/service/marge/sync.go b/pkg/service/marge/sync.go index 2bd0581..81b9f1f 100644 --- a/pkg/service/marge/sync.go +++ b/pkg/service/marge/sync.go @@ -169,7 +169,29 @@ func syncConfiguredSources(ds *datastore.DataStore, accountID, deviceID string, // 4. Add deduction based on local presets/recents ds.DeduceSourceIDs(accountID, deviceID, deviceSources) - if err := ds.SaveConfiguredSources(accountID, deviceID, deviceSources); err != nil { + // 5. Drop sources that cannot be assigned a canonical sourceproviderid. + // Device-local / transient slots (STORED_MUSIC_MEDIA_RENDERER, UPNP, + // INVALID_SOURCE, ...) have no StaticProviders entry; persisting them + // causes the serve path to emit an empty , which the + // speaker rejects and re-reports as INVALID_SOURCE — the #334 loop. + var servable []models.ConfiguredSource + + var dropped []string + + for i := range deviceSources { + if HasResolvableProviderID(deviceSources[i]) { + servable = append(servable, deviceSources[i]) + } else { + dropped = append(dropped, sanitizeLog(deviceSources[i].SourceKeyType)) + } + } + + if len(dropped) > 0 { + log.Printf("[SYNC] device %s: dropping %d unresolvable source(s) before save (types: %v)", + sanitizeLog(deviceID), len(dropped), dropped) + } + + if err := ds.SaveConfiguredSources(accountID, deviceID, servable); err != nil { log.Printf("[SYNC_ERR] Failed to save sources for %s: %v", sanitizeLog(deviceID), err) } } diff --git a/pkg/service/marge/testdata/i334_speaker_sources.xml b/pkg/service/marge/testdata/i334_speaker_sources.xml new file mode 100644 index 0000000..f45bcc4 --- /dev/null +++ b/pkg/service/marge/testdata/i334_speaker_sources.xml @@ -0,0 +1,32 @@ + + + + AUX IN + Meine Musikbibliothek + + QPlay1UserName + QPlay2UserName + UPnPUserName + user@example.com + + SpotifyConnectUserName + SpotifyAlexaUserName + StoredMusicUserName + AirPlay2DefaultUserName + + + + + diff --git a/pkg/service/setup/setup.go b/pkg/service/setup/setup.go index c56e295..83424e5 100644 --- a/pkg/service/setup/setup.go +++ b/pkg/service/setup/setup.go @@ -22,6 +22,7 @@ import ( "github.com/gesellix/bose-soundtouch/pkg/service/certmanager" "github.com/gesellix/bose-soundtouch/pkg/service/constants" "github.com/gesellix/bose-soundtouch/pkg/service/datastore" + "github.com/gesellix/bose-soundtouch/pkg/service/marge" "github.com/gesellix/bose-soundtouch/pkg/ssh" "github.com/gesellix/bose-soundtouch/pkg/telnet" ) @@ -2728,6 +2729,13 @@ func (m *Manager) syncSources(deviceIP, accountID, deviceID string) { s.SourceKeyAccount = s.SourceKey.Account } + // Drop device-local/transient sources without a resolvable + // sourceproviderid (e.g. STORED_MUSIC_MEDIA_RENDERER, UPNP). + // Persisting them causes /full to emit an empty + // which the speaker rejects as INVALID_SOURCE + // (#334). + srs.Sources = filterServableSources(srs.Sources, deviceID) + _ = m.DataStore.SaveConfiguredSources(accountID, deviceID, srs.Sources) return @@ -2774,10 +2782,41 @@ func (m *Manager) syncSources(deviceIP, accountID, deviceID string) { configuredSources = append(configuredSources, cs) } + // Drop device-local/transient sources without a resolvable + // sourceproviderid (e.g. STORED_MUSIC_MEDIA_RENDERER, UPNP). + // Persisting them causes /full to emit an empty + // which the speaker rejects as INVALID_SOURCE (#334). + configuredSources = filterServableSources(configuredSources, deviceID) + _ = m.DataStore.SaveConfiguredSources(accountID, deviceID, configuredSources) } } +// filterServableSources returns a copy of srcs containing only sources that +// have a resolvable sourceproviderid. Device-local / transient slots without +// one (STORED_MUSIC_MEDIA_RENDERER, UPNP, INVALID_SOURCE, …) are silently +// dropped and logged as a single summary line. +func filterServableSources(srcs []models.ConfiguredSource, deviceID string) []models.ConfiguredSource { + var servable []models.ConfiguredSource + + var dropped []string + + for i := range srcs { + if marge.HasResolvableProviderID(srcs[i]) { + servable = append(servable, srcs[i]) + } else { + dropped = append(dropped, sanitizeLog(srcs[i].SourceKeyType)) + } + } + + if len(dropped) > 0 { + log.Printf("[SYNC] device %s: dropping %d unresolvable source(s) on import (types: %v)", + sanitizeLog(deviceID), len(dropped), dropped) + } + + return servable +} + // notifySpeakerSourcesUpdated POSTs the notification // to /notification on the device, mirroring the manual workaround // documented in issue #234. The device responds by re-evaluating its