diff --git a/pkg/service/health/checks_consistency.go b/pkg/service/health/checks_consistency.go index 0260a31..98a861a 100644 --- a/pkg/service/health/checks_consistency.go +++ b/pkg/service/health/checks_consistency.go @@ -111,6 +111,10 @@ func checkOneDeviceConsistency(ds *datastore.DataStore, account, deviceID, ipAdd var findings []Finding + // Internal consistency is meaningful only on the service side — + // the speaker manages its own preset/source coherence locally, + // and its /sources list deliberately omits streaming sources + // (which would always trigger spurious "dangling" findings). findings = append(findings, issuesToFindings(target, CheckInternalConsistency(serviceView), SeverityWarning)...) if ipAddress == "" { @@ -136,12 +140,33 @@ func checkOneDeviceConsistency(ds *datastore.DataStore, account, deviceID, ipAdd return findings } - findings = append(findings, issuesToFindings(target, CheckInternalConsistency(speakerView), SeverityWarning)...) + // "Unsynced device" short-circuit: when the service has no + // presets / recents / sources for a device that the speaker + // clearly does have all three for, emit one consolidated + // finding instead of a torrent of per-slot mismatches. + if isServiceUnsynced(serviceView) && !isSpeakerEmpty(speakerView) { + findings = append(findings, Finding{ + Severity: SeverityWarning, + Target: target, + Message: "Device has speaker state (presets, recents, sources) but service has nothing for it — looks like a missed pair/sync. Click \"Sync\" in the device tab or factory-reset and re-pair.", + }) + + return findings + } + findings = append(findings, issuesToFindings(target, CheckCrossSide(speakerView, serviceView), SeverityWarning)...) return findings } +func isServiceUnsynced(v ConsistencyView) bool { + return len(v.Presets) == 0 && len(v.Recents) == 0 && len(v.Sources) == 0 +} + +func isSpeakerEmpty(v ConsistencyView) bool { + return len(v.Presets) == 0 && len(v.Recents) == 0 && len(v.Sources) == 0 +} + func issuesToFindings(target Target, issues []ConsistencyIssue, severity Severity) []Finding { if len(issues) == 0 { return nil @@ -177,10 +202,35 @@ func loadServiceView(ds *datastore.DataStore, account, deviceID string) (Consist view := ConsistencyView{Label: "service"} + // Build sourceID -> SourceKeyType index up front so we can resolve + // the *effective* source for each preset/recent. syncPresets and + // syncRecents in sync.go currently persist the upstream + // FullResponseSource.Type ("Audio") into ServicePreset.Source, + // which is meaningless next to the speaker's actual source name. + // Resolve via SourceID so the cross-side diff compares like with + // like; fall back to the persisted Source string when the SourceID + // is empty or doesn't resolve. + sourceByID := make(map[string]string, len(sources)) + for i := range sources { + if sources[i].ID != "" && sources[i].SourceKeyType != "" { + sourceByID[sources[i].ID] = sources[i].SourceKeyType + } + } + + resolveSource := func(persisted, sourceID string) string { + if sourceID != "" { + if resolved := sourceByID[sourceID]; resolved != "" { + return resolved + } + } + + return persisted + } + for i := range presets { view.Presets = append(view.Presets, ConsistencyPreset{ Slot: presets[i].ButtonNumber, - Source: presets[i].Source, + Source: resolveSource(presets[i].Source, presets[i].SourceID), SourceID: presets[i].SourceID, Location: presets[i].Location, Name: presets[i].Name, @@ -190,7 +240,7 @@ func loadServiceView(ds *datastore.DataStore, account, deviceID string) (Consist for i := range recents { view.Recents = append(view.Recents, ConsistencyRecent{ ID: recents[i].ID, - Source: recents[i].Source, + Source: resolveSource(recents[i].Source, recents[i].SourceID), SourceID: recents[i].SourceID, Location: recents[i].Location, Name: recents[i].Name, diff --git a/pkg/service/health/consistency.go b/pkg/service/health/consistency.go index 0653851..8d06204 100644 --- a/pkg/service/health/consistency.go +++ b/pkg/service/health/consistency.go @@ -71,13 +71,11 @@ type ConsistencyIssueKind string // Recognised ConsistencyIssueKind values. const ( - IssuePresetMismatch ConsistencyIssueKind = "preset_mismatch" - IssueRecentMismatch ConsistencyIssueKind = "recent_mismatch" - IssueSourceMismatch ConsistencyIssueKind = "source_mismatch" - IssueDanglingPreset ConsistencyIssueKind = "dangling_preset" - IssueDanglingRecent ConsistencyIssueKind = "dangling_recent" - IssueDuplicateSource ConsistencyIssueKind = "duplicate_source" - IssueMissingSpeakerSide ConsistencyIssueKind = "missing_speaker_side" + IssuePresetMismatch ConsistencyIssueKind = "preset_mismatch" + IssueRecentMismatch ConsistencyIssueKind = "recent_mismatch" + IssueDanglingPreset ConsistencyIssueKind = "dangling_preset" + IssueDanglingRecent ConsistencyIssueKind = "dangling_recent" + IssueDuplicateSource ConsistencyIssueKind = "duplicate_source" ) // CheckCrossSide compares speaker and service views for the same device @@ -155,38 +153,33 @@ func CheckCrossSide(speaker, service ConsistencyView) []ConsistencyIssue { } } - for id, sv := range serviceRecents { - if _, ok := speakerRecents[id]; !ok { - // Service-only recents are common (the speaker drops old - // entries faster than we do), so only flag if there are - // disproportionately many. - _ = sv + speakerOnly := 0 + + for id := range speakerRecents { + if _, ok := serviceRecents[id]; !ok { + speakerOnly++ } } - // Sources: compare by type (speaker lists by type, not by numeric ID). - speakerSourceTypes := sourceTypesFromView(speaker) - serviceSourceTypes := sourceTypesFromView(service) - - for t := range speakerSourceTypes { - if !serviceSourceTypes[t] { - issues = append(issues, ConsistencyIssue{ - Kind: IssueSourceMismatch, - Side: "speaker", - Detail: "source type " + safeQuote(t) + " present on speaker but not in service Sources.xml — service /full will not include presets referencing this source", - }) - } + // Speaker-only recents are normal: the speaker keeps a longer + // history than the service ingests. Surface a single summary line + // when the gap is large enough to look like a missed sync, instead + // of one finding per missing id (which used to drown the report). + if speakerOnly >= 5 { + issues = append(issues, ConsistencyIssue{ + Kind: IssueRecentMismatch, + Side: "speaker", + Detail: strconv.Itoa(speakerOnly) + " recent(s) present on speaker but missing from service — usually means service hasn't ingested the latest /recents notification; harmless unless the gap keeps growing", + }) } - for t := range serviceSourceTypes { - if !speakerSourceTypes[t] { - issues = append(issues, ConsistencyIssue{ - Kind: IssueSourceMismatch, - Side: "service", - Detail: "source type " + safeQuote(t) + " in service Sources.xml but not advertised by speaker /sources — preset references to this source may fail at play time", - }) - } - } + // Note: we deliberately do *not* compare speaker /sources types + // against service Sources.xml. Those two lists answer different + // questions: speaker /sources enumerates local I/O sources (AUX, + // BLUETOOTH, AIRPLAY, QPLAY, …) plus active-account ones (SPOTIFY), + // while service Sources.xml tracks credentialed streaming sources + // (TUNEIN, INTERNET_RADIO, …). They legitimately don't overlap on + // most types, so flagging the asymmetry was pure noise. sort.SliceStable(issues, func(i, j int) bool { return issues[i].Detail < issues[j].Detail @@ -201,6 +194,16 @@ func CheckCrossSide(speaker, service ConsistencyView) []ConsistencyIssue { // 10004" class (GH-269 NorbertBauer's restore case after a partial backup // import). // +// Only meaningful on the service side. The speaker's /sources lists local +// I/O sources (AUX, BLUETOOTH, AIRPLAY, ALEXA, QPLAY, UPNP, …) plus +// active-account ones; it does *not* enumerate streaming sources +// (TUNEIN, INTERNET_RADIO, …) — those are proxied through BMX. So a +// preset with source="TUNEIN" on the speaker side legitimately has no +// matching entry in speaker /sources, and flagging it as "dangling" +// would be a false positive. Callers should pass a service-side view +// here; speaker-side internal consistency is enforced by the firmware +// itself. +// // The side string is "speaker" or "service" — included verbatim in each // finding so the operator can tell which side is internally inconsistent. func CheckInternalConsistency(view ConsistencyView) []ConsistencyIssue { @@ -208,7 +211,8 @@ func CheckInternalConsistency(view ConsistencyView) []ConsistencyIssue { sourceIDs := map[string]bool{} sourceTypes := map[string]bool{} - dupCount := map[string]int{} + dupKey := map[string]int{} + dupLabel := map[string]string{} for _, s := range view.Sources { if s.ID != "" { @@ -217,16 +221,24 @@ func CheckInternalConsistency(view ConsistencyView) []ConsistencyIssue { if s.Type != "" { sourceTypes[s.Type] = true - dupCount[s.Type]++ + + // Multiple entries with the same source type + // but different sourceAccount are normal (e.g. Spotify + // Connect + Spotify Alexa, QPlay1 + QPlay2). Key dedup by + // type+account so we only flag *true* duplicates that + // would shadow each other in mapPresetsToFullResponse. + key := s.Type + "\x00" + s.Account + dupKey[key]++ + dupLabel[key] = s.Type + accountSuffix(s.Account) } } - for t, n := range dupCount { + for key, n := range dupKey { if n > 1 { issues = append(issues, ConsistencyIssue{ Kind: IssueDuplicateSource, Side: view.Label, - Detail: view.Label + ": source type " + safeQuote(t) + " has " + plural(n, "entry", "entries") + " in Sources.xml — mapPresetsToFullResponse picks the first match, the rest are inert", + Detail: view.Label + ": source " + safeQuote(dupLabel[key]) + " has " + plural(n, "entry", "entries") + " in Sources.xml — mapPresetsToFullResponse picks the first match, the rest are inert", }) } } @@ -268,6 +280,14 @@ func CheckInternalConsistency(view ConsistencyView) []ConsistencyIssue { return issues } +func accountSuffix(account string) string { + if account == "" { + return "" + } + + return " (account " + account + ")" +} + func indexPresetsBySlot(in []ConsistencyPreset) map[string]ConsistencyPreset { out := make(map[string]ConsistencyPreset, len(in)) for i := range in { @@ -290,18 +310,6 @@ func indexRecentsByID(in []ConsistencyRecent) map[string]ConsistencyRecent { return out } -func sourceTypesFromView(v ConsistencyView) map[string]bool { - out := map[string]bool{} - - for _, s := range v.Sources { - if s.Type != "" { - out[s.Type] = true - } - } - - return out -} - // presetFieldsCompatible is strict on Source attribute (GH-343 footprint // reproduces here as a cross-side mismatch). Location is compared only // when both sides provide it; the speaker's /presets always does, the diff --git a/pkg/service/health/consistency_test.go b/pkg/service/health/consistency_test.go index b23d285..2c433a9 100644 --- a/pkg/service/health/consistency_test.go +++ b/pkg/service/health/consistency_test.go @@ -68,6 +68,7 @@ func TestCheckInternalConsistency_DuplicateSourceType(t *testing.T) { view := ConsistencyView{ Label: "service", Sources: []ConsistencySource{ + // Same type + same (empty) account — true shadow duplicate. {ID: "10004", Type: "TUNEIN"}, {ID: "14774275", Type: "TUNEIN"}, }, @@ -88,6 +89,29 @@ func TestCheckInternalConsistency_DuplicateSourceType(t *testing.T) { } } +// TestCheckInternalConsistency_MultipleAccountsAreNotDuplicates locks in +// the dedup-by-type+account policy: two Spotify entries with different +// sourceAccount (Connect, Alexa) are legitimate, not shadow duplicates. +func TestCheckInternalConsistency_MultipleAccountsAreNotDuplicates(t *testing.T) { + view := ConsistencyView{ + Label: "speaker", + Sources: []ConsistencySource{ + {Type: "SPOTIFY", Account: "SpotifyConnectUserName"}, + {Type: "SPOTIFY", Account: "SpotifyAlexaUserName"}, + {Type: "QPLAY", Account: "QPlay1UserName"}, + {Type: "QPLAY", Account: "QPlay2UserName"}, + }, + } + + got := CheckInternalConsistency(view) + + for _, iss := range got { + if iss.Kind == IssueDuplicateSource { + t.Errorf("did not expect duplicate-source finding for distinct accounts; got: %s", iss.Detail) + } + } +} + func TestCheckCrossSide_PresetSourceMismatch_GH343(t *testing.T) { // Speaker reports TUNEIN; service reports RADIOPLAYER — the // GH-343 footprint, where /full rebinding silently rewrote the