fix(marge): never persist or serve sources without a resolvable provider id

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 <sourceproviderid> — 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) <noreply@anthropic.com>

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 <sourceproviderid> 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 <username>, keeping that assertion meaningful.
 	sourcesXML := `
 		<sources>
-			<source id="SRC1" type="Audio" createdOn="2024-01-01T00:00:00Z" updatedOn="2024-01-01T00:00:00Z" displayName="Source1" secret="TOKEN1" secretType="token" sourceProviderId="2" sourceName="SourceName1">
-				<sourceKey type="NOT_TUNEIN" account="User1"/>
+			<source id="SRC1" type="Audio" createdOn="2024-01-01T00:00:00Z" updatedOn="2024-01-01T00:00:00Z" displayName="Source1" secret="TOKEN1" secretType="token" sourceName="SourceName1">
+				<sourceKey type="PANDORA" account="User1"/>
 			</source>
 		</sources>`
 	_ = 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{
 		"<sources>",
 		"<source id=\"SRC1\" type=\"Audio\"",

pkg/service/marge/i334_sources_roundtrip_test.go

@@ -0,0 +1,200 @@
+package marge
+
+import (
+	"encoding/xml"
+	"os"
+	"path/filepath"
+	"testing"
+
+	"github.com/gesellix/bose-soundtouch/pkg/models"
+	"github.com/gesellix/bose-soundtouch/pkg/service/constants"
+	"github.com/gesellix/bose-soundtouch/pkg/service/datastore"
+)
+
+// configuredFromSpeakerSources mirrors the HTTP /sources import branch of
+// setup.(*Manager).syncSources (cmd path: SyncDeviceData -> syncSources,
+// pkg/service/setup/setup.go). It is a verbatim field copy: the speaker's
+// <sourceItem source="X"> 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 <sources> 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)
+		}
+	})
+}

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

pkg/service/marge/repro_test.go

@@ -446,6 +446,12 @@ func TestSyncSourcesAggregation(t *testing.T) {
 		t.Fatal(err)
 	}
 
+	// Sources include valid <sourceproviderid> 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 := `<?xml version="1.0" encoding="UTF-8" standalone="yes"?>
 <account id="agg_account">
     <devices>
@@ -454,6 +460,7 @@ func TestSyncSourcesAggregation(t *testing.T) {
                 <preset buttonNumber="1">
                     <name>Preset Source</name>
                     <source id="src_preset" type="Audio">
+                        <sourceproviderid>11</sourceproviderid>
                         <name>Preset Source Name</name>
                     </source>
                 </preset>
@@ -464,6 +471,7 @@ func TestSyncSourcesAggregation(t *testing.T) {
                         <itemName>Recent Source</itemName>
                     </contentItem>
                     <source id="src_recent" type="Audio">
+                        <sourceproviderid>25</sourceproviderid>
                         <name>Recent Source Name</name>
                     </source>
                 </recent>
@@ -472,6 +480,7 @@ func TestSyncSourcesAggregation(t *testing.T) {
     </devices>
     <sources>
         <source id="src_account" type="Audio">
+            <sourceproviderid>2</sourceproviderid>
             <name>Account Source Name</name>
         </source>
     </sources>

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 <sourceproviderid>, 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)
 	}
 }

pkg/service/marge/testdata/i334_speaker_sources.xml

@@ -0,0 +1,32 @@
+<?xml version="1.0" encoding="UTF-8" ?>
+<!--
+  Real /sources capture from a SoundTouch 20 in issue #334 (v-tron's
+  bundle), sanitized: deviceID, the Spotify account, the StoredMusic GUID
+  and the owner e-mail are replaced with placeholders. The Bose built-in
+  placeholder account strings (QPlay1UserName, UPnPUserName, ...) are
+  preserved verbatim because they are structural, not PII, and matter to
+  the round-trip under test.
+
+  Note what this list does NOT contain: any INVALID_SOURCE entry. Every
+  source here is a legitimate, speaker-reported source. The round-trip
+  test feeds this clean list through the real import + /full build to see
+  whether AfterTouch itself emits something a speaker would reject.
+-->
+<sources deviceID="DEVICEID01">
+  <sourceItem source="AUX" sourceAccount="AUX" status="READY" isLocal="true" multiroomallowed="true">AUX IN</sourceItem>
+  <sourceItem source="STORED_MUSIC" sourceAccount="00000000-0000-0000-0000-000000000000/0" status="UNAVAILABLE" isLocal="false" multiroomallowed="true">Meine Musikbibliothek</sourceItem>
+  <sourceItem source="NOTIFICATION" status="UNAVAILABLE" isLocal="false" multiroomallowed="true" />
+  <sourceItem source="QPLAY" sourceAccount="QPlay1UserName" status="UNAVAILABLE" isLocal="true" multiroomallowed="true">QPlay1UserName</sourceItem>
+  <sourceItem source="QPLAY" sourceAccount="QPlay2UserName" status="UNAVAILABLE" isLocal="true" multiroomallowed="true">QPlay2UserName</sourceItem>
+  <sourceItem source="UPNP" sourceAccount="UPnPUserName" status="UNAVAILABLE" isLocal="false" multiroomallowed="true">UPnPUserName</sourceItem>
+  <sourceItem source="SPOTIFY" sourceAccount="spotifyacct01" status="READY" isLocal="false" multiroomallowed="true">user@example.com</sourceItem>
+  <sourceItem source="ALEXA" status="READY" isLocal="false" multiroomallowed="true" />
+  <sourceItem source="SPOTIFY" sourceAccount="SpotifyConnectUserName" status="UNAVAILABLE" isLocal="false" multiroomallowed="true">SpotifyConnectUserName</sourceItem>
+  <sourceItem source="SPOTIFY" sourceAccount="SpotifyAlexaUserName" status="UNAVAILABLE" isLocal="false" multiroomallowed="true">SpotifyAlexaUserName</sourceItem>
+  <sourceItem source="STORED_MUSIC_MEDIA_RENDERER" sourceAccount="StoredMusicUserName" status="UNAVAILABLE" isLocal="false" multiroomallowed="true">StoredMusicUserName</sourceItem>
+  <sourceItem source="AIRPLAY" sourceAccount="AirPlay2DefaultUserName" status="UNAVAILABLE" isLocal="false" multiroomallowed="false">AirPlay2DefaultUserName</sourceItem>
+  <sourceItem source="BLUETOOTH" status="UNAVAILABLE" isLocal="true" multiroomallowed="true" />
+  <sourceItem source="TUNEIN" status="READY" isLocal="false" multiroomallowed="true" />
+  <sourceItem source="LOCAL_INTERNET_RADIO" status="READY" isLocal="false" multiroomallowed="true" />
+  <sourceItem source="RADIO_BROWSER" status="READY" isLocal="false" multiroomallowed="true" />
+</sources>

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
+			// <sourceproviderid> 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 <sourceproviderid>
+		// 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 <sourcesUpdated/> notification
 // to /notification on the device, mirroring the manual workaround
 // documented in issue #234. The device responds by re-evaluating its