diff --git a/pkg/service/health/checks_consistency.go b/pkg/service/health/checks_consistency.go index a56f260..469a680 100644 --- a/pkg/service/health/checks_consistency.go +++ b/pkg/service/health/checks_consistency.go @@ -4,7 +4,8 @@ import ( "context" "encoding/xml" "fmt" - "strconv" + "log" + "os" "time" "github.com/gesellix/bose-soundtouch/pkg/models" @@ -14,6 +15,11 @@ import ( // CheckIDPresetsConsistency is the registry id of the consistency check. const CheckIDPresetsConsistency = "presets_recents_sources_consistency" +// FixIDDeleteOrphanAccountEntry is the QuickFix that removes a stale +// account directory for a device after the operator has confirmed the +// speaker isn't currently targeting it. +const FixIDDeleteOrphanAccountEntry = "delete_orphan_account_entry" + // speakerPresetsConsistencyXML mirrors enough of :8090/presets to extract // slot id, source/location and itemName for cross-side comparison. type speakerPresetsConsistencyXML struct { @@ -68,6 +74,10 @@ func RegisterPresetsConsistencyCheck(r *Registry, ds *datastore.DataStore) { return runPresetsConsistencyCheck(ds) }, }) + + r.RegisterFix(CheckIDPresetsConsistency, FixIDDeleteOrphanAccountEntry, func(target Target) (string, error) { + return deleteOrphanAccountEntry(ds, target) + }) } func runPresetsConsistencyCheck(ds *datastore.DataStore) []Finding { @@ -108,10 +118,10 @@ func runPresetsConsistencyCheck(ds *datastore.DataStore) []Finding { // via the URL of every PUT it sends; any other account entry on disk // is leftover state from a previous pairing. The active account is // the one ListAllDevices' dedup currently exposes (with "default" -// already deprioritised); the stale ones get one finding each so the -// operator can see and clean them up. We don't delete automatically -// because filesystem deletions need explicit operator consent -// (CLAUDE.md "destructive actions" rule). +// already deprioritised); the stale ones each get a finding with a +// confirm-gated QuickFix so the operator can delete them one at a +// time after verifying via the service log which account the speaker +// is actually targeting. func detectOrphanDefaultEntries(ds *datastore.DataStore, paired []models.ServiceDeviceInfo) []Finding { activeAccount := map[string]string{} // deviceID -> the account ListAllDevices picked @@ -131,26 +141,28 @@ func detectOrphanDefaultEntries(ds *datastore.DataStore, paired []models.Service continue } - stale := make([]string, 0, len(allAccounts)-1) - for _, acc := range allAccounts { - if acc != active { - stale = append(stale, acc) + if acc == active { + continue } - } - if len(stale) == 0 { - continue + findings = append(findings, Finding{ + Severity: SeverityWarning, + Target: Target{Account: acc, Device: deviceID}, + Message: "Stale account entry: device " + deviceID + " also has state under account " + safeQuoteFinding(acc) + " — likely leftover from a previous pairing. The currently-active account is " + safeQuoteFinding(active) + ".", + Details: "Before deleting, verify the speaker isn't currently PUTting to account " + acc + " by checking the service log for /streaming/account/" + acc + "/device/" + deviceID + "/... entries.", + QuickFixes: []QuickFix{{ + ID: FixIDDeleteOrphanAccountEntry, + Label: "Delete stale entry", + Confirm: "Permanently delete /accounts/" + acc + "/devices/" + deviceID + "/? This removes Presets.xml, Recents.xml, Sources.xml and DeviceInfo.xml for this stale pairing. The active account " + active + " is not touched.", + }}, + ManualCommands: []ManualCommand{{ + Label: "Or remove from a shell:", + Command: "rm -rf /accounts/" + acc + "/devices/" + deviceID, + Hint: "Substitute with the service's actual data directory (typically /var/lib/soundtouch-service).", + }}, + }) } - - findings = append(findings, Finding{ - Severity: SeverityWarning, - Target: Target{Device: deviceID}, - Message: "Device " + deviceID + " has state under " + strconv.Itoa(len(allAccounts)) + - " account directories — likely leftover from earlier pairings. The active one (per ListAllDevices' dedup) is " + safeQuoteFinding(active) + - "; stale entries: " + joinAccounts(stale) + - ". Confirm which one the speaker currently PUTs to (check service log for /streaming/account//device/" + deviceID + "/...) and remove the others. Each stale dir lives at /accounts//devices/" + deviceID + "/.", - }) } return findings @@ -164,20 +176,41 @@ func safeQuoteFinding(s string) string { return `"` + s + `"` } -func joinAccounts(accounts []string) string { - out := "" - - for i, a := range accounts { - if i > 0 { - out += ", " - } - - out += `"` + a + `"` +// deleteOrphanAccountEntry removes accounts//devices//. +// Called only after the operator has clicked through the Confirm dialog +// that the QuickFix surfaces; the framework is the gatekeeper, so this +// just executes. Logs the action for auditability. +func deleteOrphanAccountEntry(ds *datastore.DataStore, target Target) (string, error) { + if target.Account == "" || target.Device == "" { + return "", fmt.Errorf("account and device are both required") } - return out + if target.Account == accountIDDefaultPlaceholder { + // Allowed — "default" is a frequent orphan source — but log + // the explicit case so misuse stands out. + log.Printf("[Health] deleteOrphanAccountEntry: deleting the \"default\" placeholder entry for device %s; this is normal after pairing completed", target.Device) + } + + path := ds.AccountDeviceDir(target.Account, target.Device) + if _, err := os.Stat(path); err != nil { + return "", fmt.Errorf("orphan directory %s no longer exists; nothing to do", path) + } + + if err := os.RemoveAll(path); err != nil { + return "", fmt.Errorf("delete %s: %w", path, err) + } + + log.Printf("[Health] Removed orphan account entry %s (account=%s device=%s) at operator request", + path, target.Account, target.Device) + + return fmt.Sprintf("Removed stale account entry %s for device %s.", target.Account, target.Device), nil } +// accountIDDefaultPlaceholder mirrors datastore.accountIDDefault for +// the health package; kept here to avoid widening the datastore +// package's exported surface. +const accountIDDefaultPlaceholder = "default" + func checkOneDeviceConsistency(ds *datastore.DataStore, account, deviceID, ipAddress string) []Finding { target := Target{Account: account, Device: deviceID} diff --git a/pkg/service/health/delete_orphan_test.go b/pkg/service/health/delete_orphan_test.go new file mode 100644 index 0000000..856f22d --- /dev/null +++ b/pkg/service/health/delete_orphan_test.go @@ -0,0 +1,105 @@ +package health + +import ( + "os" + "path/filepath" + "strings" + "testing" + + "github.com/gesellix/bose-soundtouch/pkg/service/datastore" +) + +// TestDeleteOrphanAccountEntry_RemovesOnlyTargetedDir confirms the +// QuickFix removes the exact stale account-device directory it was +// told to and leaves everything else (active account, other devices) +// intact. The framework gates this behind operator Confirm; this +// test only exercises the execution side. +func TestDeleteOrphanAccountEntry_RemovesOnlyTargetedDir(t *testing.T) { + tempDir, err := os.MkdirTemp("", "health-delete-orphan-*") + if err != nil { + t.Fatalf("tempdir: %v", err) + } + defer func() { _ = os.RemoveAll(tempDir) }() + + deviceID := "AABBCCDDEEFF" + staleAcc := "9569497" + activeAcc := "1111111" + + for _, acc := range []string{staleAcc, activeAcc} { + dir := filepath.Join(tempDir, "accounts", acc, "devices", deviceID) + if err := os.MkdirAll(dir, 0755); err != nil { + t.Fatalf("mkdir %s: %v", dir, err) + } + + if err := os.WriteFile(filepath.Join(dir, "DeviceInfo.xml"), []byte(""), 0644); err != nil { + t.Fatalf("write: %v", err) + } + } + + // And one unrelated device on the active account that must survive. + other := filepath.Join(tempDir, "accounts", activeAcc, "devices", "OTHERDEVICE01") + if err := os.MkdirAll(other, 0755); err != nil { + t.Fatalf("mkdir other: %v", err) + } + + if err := os.WriteFile(filepath.Join(other, "DeviceInfo.xml"), []byte(""), 0644); err != nil { + t.Fatalf("write other: %v", err) + } + + ds := datastore.NewDataStore(tempDir) + + msg, err := deleteOrphanAccountEntry(ds, Target{Account: staleAcc, Device: deviceID}) + if err != nil { + t.Fatalf("deleteOrphanAccountEntry: %v", err) + } + + if !strings.Contains(msg, staleAcc) { + t.Errorf("success message should name the deleted account; got %q", msg) + } + + if _, err := os.Stat(filepath.Join(tempDir, "accounts", staleAcc, "devices", deviceID)); !os.IsNotExist(err) { + t.Errorf("stale dir should be gone, stat err = %v", err) + } + + if _, err := os.Stat(filepath.Join(tempDir, "accounts", activeAcc, "devices", deviceID)); err != nil { + t.Errorf("active-account device dir should still exist, got err = %v", err) + } + + if _, err := os.Stat(other); err != nil { + t.Errorf("unrelated device dir should still exist, got err = %v", err) + } +} + +// TestDeleteOrphanAccountEntry_RejectsMissingTarget guards against +// fix-registry misuse: a caller that supplies an empty account or +// device should get a clear error, not silently no-op. +func TestDeleteOrphanAccountEntry_RejectsMissingTarget(t *testing.T) { + tempDir, _ := os.MkdirTemp("", "health-delete-empty-*") + defer func() { _ = os.RemoveAll(tempDir) }() + + ds := datastore.NewDataStore(tempDir) + + if _, err := deleteOrphanAccountEntry(ds, Target{Device: "A"}); err == nil { + t.Error("expected error for empty Account") + } + + if _, err := deleteOrphanAccountEntry(ds, Target{Account: "1"}); err == nil { + t.Error("expected error for empty Device") + } +} + +// TestDeleteOrphanAccountEntry_NotFoundIsExplicit returns an error +// pointing at the path rather than silently no-op'ing. If the +// operator clicks the fix twice or after manual cleanup, that's +// useful to surface. +func TestDeleteOrphanAccountEntry_NotFoundIsExplicit(t *testing.T) { + tempDir, _ := os.MkdirTemp("", "health-delete-missing-*") + defer func() { _ = os.RemoveAll(tempDir) }() + + ds := datastore.NewDataStore(tempDir) + + _, err := deleteOrphanAccountEntry(ds, Target{Account: "1111111", Device: "AABBCCDDEEFF"}) + if err == nil || !strings.Contains(err.Error(), "no longer exists") { + t.Errorf("expected 'no longer exists' error, got %v", err) + } +}