From e6954eed60fd1cd9d321ab9bb86ba3f369f49d6e Mon Sep 17 00:00:00 2001 From: Tobias Gesellchen Date: Wed, 20 May 2026 21:29:02 +0200 Subject: [PATCH] fix(datastore): real account wins over "default" placeholder in dedup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ListAllDevices used to let an entry under accounts/default/devices/ replace the real-account entry for the same physical device whenever the default-side DeviceInfo.xml had a non-empty . The consistency check then reported the device under "account default" even while the speaker was happily POST/PUT'ing to its actually-paired account — the operator saw "preset slot 1 present on speaker but missing from service" for slots that very obviously did exist, just under the real account they couldn't see. The dedup now treats "default" as a fallback placeholder: sorts it to the back of the iteration, and never lets it replace a real-account entry. A default-only device (fresh discovery, never paired) is still returned exactly as before. Also adds an orphan-detection finding in the consistency check that walks accounts/default/devices/ directly and flags entries whose deviceID is also paired under a real account, with a copy-pasteable rm -rf hint. We don't delete automatically — destructive filesystem actions need explicit operator consent (CLAUDE.md). Co-Authored-By: Claude Sonnet 4.6 --- pkg/service/datastore/datastore.go | 82 ++++++++++---- .../list_all_devices_default_test.go | 107 ++++++++++++++++++ pkg/service/health/checks_consistency.go | 49 ++++++++ 3 files changed, 218 insertions(+), 20 deletions(-) create mode 100644 pkg/service/datastore/list_all_devices_default_test.go diff --git a/pkg/service/datastore/datastore.go b/pkg/service/datastore/datastore.go index e44902c..eb8e89f 100644 --- a/pkg/service/datastore/datastore.go +++ b/pkg/service/datastore/datastore.go @@ -618,19 +618,47 @@ func (ds *DataStore) ListAllDevices() ([]models.ServiceDeviceInfo, error) { } devices := []models.ServiceDeviceInfo{} - seenIDs := make(map[string]bool) + + type seenEntry struct { + index int + account string + } + + seenIDs := make(map[string]seenEntry) for _, dir := range dirs { - accounts, err := os.ReadDir(dir) + entries, err := os.ReadDir(dir) if err != nil { continue } - for _, acc := range accounts { - if !acc.IsDir() { - continue + // Sort "default" to the back so a real-account entry always + // wins the first-seen race. "default" exists as a pre-pair + // placeholder; once the speaker pairs with a real account it + // becomes orphan state. Sort returns the same list for tests + // because os.ReadDir is already sorted, and digits sort before + // "default" in ASCII — but be defensive. + accounts := make([]os.DirEntry, 0, len(entries)) + for _, e := range entries { + if e.IsDir() { + accounts = append(accounts, e) + } + } + + sort.SliceStable(accounts, func(i, j int) bool { + ai, aj := accounts[i].Name(), accounts[j].Name() + if ai == accountIDDefault && aj != accountIDDefault { + return false } + if aj == accountIDDefault && ai != accountIDDefault { + return true + } + + return ai < aj + }) + + for _, acc := range accounts { accDevices := ds.listDevicesInAccount(dir, acc.Name()) for i := range accDevices { info := accDevices[i] @@ -641,21 +669,28 @@ func (ds *DataStore) ListAllDevices() ([]models.ServiceDeviceInfo, error) { key = info.IPAddress } - if !seenIDs[key] || info.Name != "" { - if seenIDs[key] && info.Name != "" { - // Replace previous empty-named entry with one that has a name - for j := range devices { - existing := &devices[j] - if (existing.DeviceID != "" && existing.DeviceID == info.DeviceID) || - (existing.IPAddress != "" && existing.IPAddress == info.IPAddress) { - devices[j] = info - break - } - } - } else if !seenIDs[key] { - devices = append(devices, info) - seenIDs[key] = true - } + entry, alreadySeen := seenIDs[key] + if !alreadySeen { + devices = append(devices, info) + seenIDs[key] = seenEntry{index: len(devices) - 1, account: info.AccountID} + + continue + } + + // Don't let a "default" entry replace a real-account + // one. The speaker pairs to a real account; any + // remaining "default" record is orphan state from + // before pairing. + if info.AccountID == accountIDDefault { + continue + } + + // Allow real-account → real-account replacement only + // when the prior was the placeholder "default" or had + // no name (less authoritative). + if entry.account == accountIDDefault || (devices[entry.index].Name == "" && info.Name != "") { + devices[entry.index] = info + seenIDs[key] = seenEntry{index: entry.index, account: info.AccountID} } } } @@ -664,6 +699,13 @@ func (ds *DataStore) ListAllDevices() ([]models.ServiceDeviceInfo, error) { return devices, nil } +// accountIDDefault is the placeholder account id assigned to records +// that exist before a speaker has been paired with a real Marge +// account. Treated as fallback in ListAllDevices and skipped in +// consistency checks when a real-account entry exists for the same +// device. +const accountIDDefault = "default" + func (ds *DataStore) getPossibleDataDirs() []string { dirs := []string{} // Check primary data directory diff --git a/pkg/service/datastore/list_all_devices_default_test.go b/pkg/service/datastore/list_all_devices_default_test.go new file mode 100644 index 0000000..dae3422 --- /dev/null +++ b/pkg/service/datastore/list_all_devices_default_test.go @@ -0,0 +1,107 @@ +package datastore + +import ( + "os" + "path/filepath" + "testing" +) + +// TestListAllDevices_RealAccountWinsOverDefault reproduces the operator's +// observation: the same physical device exists on disk under both +// accounts/default/devices// (leftover pre-pair state) and +// accounts//devices// (active pairing). The dedupe loop used to +// let "default" replace the real-account entry whenever the default +// directory's DeviceInfo.xml had a non-empty , which made +// consistency checks report the device under "default" while the speaker +// was happily POST/PUT'ing to the real account. +// +// "default" must be treated as a fallback placeholder, never as +// authoritative when a real account also has the device. +func TestListAllDevices_RealAccountWinsOverDefault(t *testing.T) { + tempDir, err := os.MkdirTemp("", "datastore-default-orphan-*") + if err != nil { + t.Fatalf("tempdir: %v", err) + } + defer func() { _ = os.RemoveAll(tempDir) }() + + deviceID := "AABBCCDDEEFF" + + // Default-account entry: stale pre-pair state with a name. + defaultDir := filepath.Join(tempDir, "accounts", "default", "devices", deviceID) + if err := os.MkdirAll(defaultDir, 0755); err != nil { + t.Fatalf("mkdir default: %v", err) + } + + if err := os.WriteFile(filepath.Join(defaultDir, "DeviceInfo.xml"), []byte(` +Discovered Device`), 0644); err != nil { + t.Fatalf("write default info: %v", err) + } + + // Real-account entry: the speaker is paired here. + realDir := filepath.Join(tempDir, "accounts", "1111111", "devices", deviceID) + if err := os.MkdirAll(realDir, 0755); err != nil { + t.Fatalf("mkdir real: %v", err) + } + + if err := os.WriteFile(filepath.Join(realDir, "DeviceInfo.xml"), []byte(` +Living Room SoundTouch`), 0644); err != nil { + t.Fatalf("write real info: %v", err) + } + + ds := NewDataStore(tempDir) + + devices, err := ds.ListAllDevices() + if err != nil { + t.Fatalf("ListAllDevices: %v", err) + } + + if len(devices) != 1 { + t.Fatalf("expected exactly 1 deduped device, got %d: %+v", len(devices), devices) + } + + if devices[0].AccountID != "1111111" { + t.Errorf("expected real account 1111111 to win dedup, got AccountID=%q (default leaked through)", devices[0].AccountID) + } + + if devices[0].Name != "Living Room SoundTouch" { + t.Errorf("expected real-account Name to survive dedup, got %q", devices[0].Name) + } +} + +// TestListAllDevices_DefaultOnlyDeviceStillSeen confirms the dedup +// preference is one-way: a device that *only* exists under "default" +// (e.g. fresh discovery, never paired) is still returned. We just +// refuse to let "default" override a real account. +func TestListAllDevices_DefaultOnlyDeviceStillSeen(t *testing.T) { + tempDir, err := os.MkdirTemp("", "datastore-default-only-*") + if err != nil { + t.Fatalf("tempdir: %v", err) + } + defer func() { _ = os.RemoveAll(tempDir) }() + + deviceID := "112233445566" + defaultDir := filepath.Join(tempDir, "accounts", "default", "devices", deviceID) + if err := os.MkdirAll(defaultDir, 0755); err != nil { + t.Fatalf("mkdir: %v", err) + } + + if err := os.WriteFile(filepath.Join(defaultDir, "DeviceInfo.xml"), []byte(` +Newly Discovered`), 0644); err != nil { + t.Fatalf("write: %v", err) + } + + ds := NewDataStore(tempDir) + + devices, err := ds.ListAllDevices() + if err != nil { + t.Fatalf("ListAllDevices: %v", err) + } + + if len(devices) != 1 { + t.Fatalf("expected 1 device, got %d", len(devices)) + } + + if devices[0].AccountID != "default" { + t.Errorf("expected default-only device to be returned with AccountID=default, got %q", devices[0].AccountID) + } +} diff --git a/pkg/service/health/checks_consistency.go b/pkg/service/health/checks_consistency.go index 4e32f03..14f4e9d 100644 --- a/pkg/service/health/checks_consistency.go +++ b/pkg/service/health/checks_consistency.go @@ -4,6 +4,7 @@ import ( "context" "encoding/xml" "fmt" + "os" "time" "github.com/gesellix/bose-soundtouch/pkg/models" @@ -84,6 +85,12 @@ func runPresetsConsistencyCheck(ds *datastore.DataStore) []Finding { var findings []Finding + // Surface orphan "default"-account entries for devices that are + // also paired under a real account. The speaker pairs to one + // account at a time; the leftover "default" record from the + // pre-pair phase is stale state the operator can safely remove. + findings = append(findings, detectOrphanDefaultEntries(ds, devices)...) + for i := range devices { dev := &devices[i] if dev.AccountID == "" || dev.DeviceID == "" { @@ -96,6 +103,48 @@ func runPresetsConsistencyCheck(ds *datastore.DataStore) []Finding { return findings } +// detectOrphanDefaultEntries walks the on-disk account directories +// directly (not through ListAllDevices, which already dedupes "default" +// out) and flags any device that exists under "default" *and* under a +// real account. The fix is to delete accounts/default/devices// — +// we don't do it automatically because filesystem deletions need +// explicit operator consent (CLAUDE.md "destructive actions" rule). +func detectOrphanDefaultEntries(ds *datastore.DataStore, paired []models.ServiceDeviceInfo) []Finding { + pairedReal := map[string]string{} // deviceID -> real accountID + + for i := range paired { + if paired[i].AccountID != "" && paired[i].AccountID != "default" && paired[i].DeviceID != "" { + pairedReal[paired[i].DeviceID] = paired[i].AccountID + } + } + + defaultDevicesDir := ds.AccountDevicesDir("default") + + entries, err := os.ReadDir(defaultDevicesDir) + if err != nil { + return nil + } + + var findings []Finding + + for _, e := range entries { + if !e.IsDir() { + continue + } + + deviceID := e.Name() + if realAccount, ok := pairedReal[deviceID]; ok { + findings = append(findings, Finding{ + Severity: SeverityWarning, + Target: Target{Account: "default", Device: deviceID}, + Message: "Orphan \"default\" account entry: device " + deviceID + " is also paired under account " + realAccount + ". The default entry is leftover state from before pairing and is masked from consistency checks. Safe to delete: rm -rf /accounts/default/devices/" + deviceID, + }) + } + } + + return findings +} + func checkOneDeviceConsistency(ds *datastore.DataStore, account, deviceID, ipAddress string) []Finding { target := Target{Account: account, Device: deviceID}