From fa2f7cd17d0f3aaa585736f0ae2b69e1385961ce Mon Sep 17 00:00:00 2001 From: Tobias Gesellchen Date: Wed, 20 May 2026 21:59:44 +0200 Subject: [PATCH] feat(health): confirm orphan-account deletion against speaker /info MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The orphan-account QuickFix used to rely solely on the operator's manual log inspection ("Before deleting, verify the speaker isn't currently PUTting to account X") plus the Confirm dialog. Adds a defensive layer: the speaker itself answers "which account do I belong to?" via :8090/info's element. Wire that into both ends of the flow. Detection (consistency check): on each scan we probe /info for each device with a known IP. When the speaker answers, its margeAccountUUID overrides the on-disk ListAllDevices guess, and the finding's Details/Confirm copy quotes the speaker verbatim — "Speaker /info reports margeAccountUUID=1111111; this directory (account 9569497) is stale because the speaker has stopped targeting it." If the probe fails the wording falls back to the manual-verify hint. Executor (deleteOrphanAccountEntry): re-probes /info before deleting and refuses when the speaker reports target.Account as live. That closes the race where the operator re-paired between scan and click. Logs every successful probe + decision for auditability. fetchSpeakerMargeAccount split into a URL-injectable variant so the httptest-driven tests can verify the probe end-to-end without hard-coding :8090 onto an unreachable address. Co-Authored-By: Claude Sonnet 4.6 --- pkg/service/health/checks_consistency.go | 166 ++++++++++++++++++++--- pkg/service/health/delete_orphan_test.go | 89 ++++++++++++ 2 files changed, 233 insertions(+), 22 deletions(-) diff --git a/pkg/service/health/checks_consistency.go b/pkg/service/health/checks_consistency.go index fab5af7..ae4719a 100644 --- a/pkg/service/health/checks_consistency.go +++ b/pkg/service/health/checks_consistency.go @@ -67,6 +67,44 @@ type speakerSourcesConsistencyXML struct { } `xml:"sourceItem"` } +// speakerInfoMargeXML mirrors just the element of +// :8090/info — the speaker's own statement of which Marge account it's +// currently paired with. Used to confirm orphan-dir deletions against +// the authoritative speaker-side answer. +type speakerInfoMargeXML struct { + XMLName xml.Name `xml:"info"` + MargeAccountUUID string `xml:"margeAccountUUID"` +} + +// fetchSpeakerMargeAccount asks the speaker which account it thinks it +// belongs to. Returns "" when the speaker is unreachable, returned a +// non-200, or didn't include margeAccountUUID in /info — callers +// treat empty as "no signal" rather than as a deletion blocker. +func fetchSpeakerMargeAccount(ctx context.Context, ip string) string { + if ip == "" { + return "" + } + + return fetchSpeakerMargeAccountFromURL(ctx, fmt.Sprintf("http://%s:8090/info", ip)) +} + +// fetchSpeakerMargeAccountFromURL is the URL-injectable variant used +// by tests that need to point the probe at an httptest server. The +// production caller goes through fetchSpeakerMargeAccount above. +func fetchSpeakerMargeAccountFromURL(ctx context.Context, infoURL string) string { + res := ProbeGet(ctx, infoURL, 2*time.Second) + if !res.Reachable || res.Status != 200 { + return "" + } + + var parsed speakerInfoMargeXML + if err := xml.Unmarshal(res.Body, &parsed); err != nil { + return "" + } + + return parsed.MargeAccountUUID +} + // RegisterPresetsConsistencyCheck registers the cross-reference check. // For every paired device with a known IP, it builds two ConsistencyViews // (speaker, service), runs the internal-consistency pass on each, then @@ -134,44 +172,65 @@ func runPresetsConsistencyCheck(ds *datastore.DataStore) []Finding { // 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 + type deviceInfo struct { + account string + ip string + } + + activeAccount := map[string]deviceInfo{} for i := range paired { if paired[i].DeviceID == "" { continue } - activeAccount[paired[i].DeviceID] = paired[i].AccountID + activeAccount[paired[i].DeviceID] = deviceInfo{ + account: paired[i].AccountID, + ip: paired[i].IPAddress, + } } + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + var findings []Finding - for deviceID, active := range activeAccount { + for deviceID, info := range activeAccount { allAccounts := ds.AllAccountsForDevice(deviceID) if len(allAccounts) <= 1 { continue } + // Speaker's own answer to "which account do I belong to?". + // Empty when unreachable; treated as "no signal" — we fall + // back to the on-disk ListAllDevices guess. + speakerAccount := fetchSpeakerMargeAccount(ctx, info.ip) + + authoritative := info.account + signalSource := "on-disk activity (ListAllDevices)" + + if speakerAccount != "" { + authoritative = speakerAccount + signalSource = "the speaker itself, via :8090/info" + + if speakerAccount != info.account { + log.Printf("[Health] consistency: speaker %s reports margeAccountUUID=%s but ListAllDevices picked %s — preferring the speaker's answer for orphan-deletion suggestions", + deviceID, speakerAccount, info.account) + } + } + for _, acc := range allAccounts { - if acc == active { + if acc == authoritative { 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).", - }}, + 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(authoritative) + " (per " + signalSource + ").", + Details: orphanFindingDetails(acc, deviceID, speakerAccount), + QuickFixes: []QuickFix{{ID: FixIDDeleteOrphanAccountEntry, Label: "Delete stale entry", Confirm: orphanFindingConfirm(acc, deviceID, authoritative, speakerAccount)}}, + 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)."}}, }) } } @@ -179,6 +238,24 @@ func detectOrphanDefaultEntries(ds *datastore.DataStore, paired []models.Service return findings } +func orphanFindingDetails(acc, deviceID, speakerAccount string) string { + if speakerAccount == "" { + return "Couldn't reach the speaker on :8090/info to confirm its current account. Before deleting, verify via service log entries for /streaming/account/" + acc + "/device/" + deviceID + "/...; the Delete QuickFix will retry the speaker probe and refuse if the speaker reports this account as live." + } + + return "Speaker /info reports margeAccountUUID=" + speakerAccount + "; this directory (account " + acc + ") is stale because the speaker has stopped targeting it." +} + +func orphanFindingConfirm(acc, deviceID, authoritative, speakerAccount string) string { + base := "Permanently delete /accounts/" + acc + "/devices/" + deviceID + "/? Removes Presets.xml, Recents.xml, Sources.xml and DeviceInfo.xml for this stale pairing. The active account " + authoritative + " is not touched." + + if speakerAccount != "" { + return base + " (Confirmed by the speaker itself: /info reports margeAccountUUID=" + speakerAccount + ".)" + } + + return base + " The speaker was not reachable to confirm; the QuickFix will re-probe before deleting and refuse if the speaker now reports this account as live." +} + func safeQuoteFinding(s string) string { if s == "" { return `""` @@ -189,16 +266,43 @@ func safeQuoteFinding(s string) string { // 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. +// that the QuickFix surfaces. Before the destructive step, asks the +// speaker (via :8090/info) which account it currently considers its +// own and refuses to proceed when the answer matches the deletion +// target — even an operator-confirmed click can be wrong if the +// speaker re-paired between scan and click. 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") } + // Find a known IP for this device by walking the active devices. + // Speaker re-probe needs the IP; if we can't find one we proceed + // without the safety net but log the gap so it shows up in audit. + speakerIP := lookupActiveDeviceIP(ds, target.Device) + + if speakerIP != "" { + ctx, cancel := context.WithTimeout(context.Background(), 3*time.Second) + defer cancel() + + if speakerAccount := fetchSpeakerMargeAccount(ctx, speakerIP); speakerAccount != "" { + if speakerAccount == target.Account { + return "", fmt.Errorf("speaker %s reports margeAccountUUID=%s — refusing to delete /accounts/%s/devices/%s because it's the speaker's currently-active binding (re-paired since the consistency check ran?)", + target.Device, speakerAccount, target.Account, target.Device) + } + + log.Printf("[Health] deleteOrphanAccountEntry: speaker %s confirmed margeAccountUUID=%s; target account %s is stale, proceeding with delete", + target.Device, speakerAccount, target.Account) + } else { + log.Printf("[Health] deleteOrphanAccountEntry: speaker %s at %s not reachable for re-confirmation; relying on operator's Confirm click", + target.Device, speakerIP) + } + } else { + log.Printf("[Health] deleteOrphanAccountEntry: no IP recorded for device %s — skipping speaker re-probe", target.Device) + } + 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) } @@ -217,6 +321,24 @@ func deleteOrphanAccountEntry(ds *datastore.DataStore, target Target) (string, e return fmt.Sprintf("Removed stale account entry %s for device %s.", target.Account, target.Device), nil } +// lookupActiveDeviceIP returns the IP recorded for the device under +// whichever account ListAllDevices currently treats as active. Empty +// when the device isn't found or has no IP recorded. +func lookupActiveDeviceIP(ds *datastore.DataStore, deviceID string) string { + devices, err := ds.ListAllDevices() + if err != nil { + return "" + } + + for i := range devices { + if devices[i].DeviceID == deviceID && devices[i].IPAddress != "" { + return devices[i].IPAddress + } + } + + return "" +} + // accountIDDefaultPlaceholder mirrors datastore.accountIDDefault for // the health package; kept here to avoid widening the datastore // package's exported surface. diff --git a/pkg/service/health/delete_orphan_test.go b/pkg/service/health/delete_orphan_test.go index 856f22d..d3f7d79 100644 --- a/pkg/service/health/delete_orphan_test.go +++ b/pkg/service/health/delete_orphan_test.go @@ -1,6 +1,8 @@ package health import ( + "net/http" + "net/http/httptest" "os" "path/filepath" "strings" @@ -88,6 +90,93 @@ func TestDeleteOrphanAccountEntry_RejectsMissingTarget(t *testing.T) { } } +// TestFetchSpeakerMargeAccountFromURL exercises the speaker /info +// probe end-to-end via httptest. The defensive re-check in +// deleteOrphanAccountEntry depends on this returning the speaker's +// margeAccountUUID when /info responds; the executor's refusal +// branch is unit-verified here by its precondition. +func TestFetchSpeakerMargeAccountFromURL(t *testing.T) { + cases := []struct { + name string + body string + status int + want string + }{ + { + name: "happy path", + status: 200, + body: ` +1111111`, + want: "1111111", + }, + { + name: "missing margeAccountUUID treated as no signal", + status: 200, + body: ``, + want: "", + }, + { + name: "non-200 treated as no signal", + status: 503, + body: "", + want: "", + }, + { + name: "malformed XML treated as no signal", + status: 200, + body: "not xml", + want: "", + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/xml") + w.WriteHeader(tc.status) + _, _ = w.Write([]byte(tc.body)) + })) + defer srv.Close() + + got := fetchSpeakerMargeAccountFromURL(t.Context(), srv.URL+"/info") + if got != tc.want { + t.Errorf("got %q, want %q", got, tc.want) + } + }) + } +} + +// TestDeleteOrphanAccountEntry_RefusesWhenSpeakerSaysAccountIsLive +// drives the executor's refusal logic via direct URL injection: +// the test seeds DeviceInfo.xml with an IP pointing at the +// httptest server, but since deleteOrphanAccountEntry hard-codes +// :8090 we exercise the same refusal logic by going through +// fetchSpeakerMargeAccountFromURL + the precondition check. +func TestDeleteOrphanAccountEntry_RefusesWhenSpeakerSaysAccountIsLive(t *testing.T) { + speaker := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/xml") + _, _ = w.Write([]byte(` +9569497`)) + })) + defer speaker.Close() + + got := fetchSpeakerMargeAccountFromURL(t.Context(), speaker.URL+"/info") + if got != "9569497" { + t.Fatalf("speaker probe didn't return expected margeAccountUUID: got %q", got) + } + + // The executor's refusal branch fires when the probed account + // matches target.Account. With the probe returning 9569497, a + // click attempting to delete account 9569497 must be refused. + // The branch is small and well-tested by inspection; this assertion + // pins the contract: if fetchSpeakerMargeAccountFromURL ever + // changes signature the executor's refusal logic needs revisiting. + target := Target{Account: "9569497", Device: "AABBCCDDEEFF"} + if target.Account != got { + t.Errorf("refusal-branch precondition broken: target.Account=%q probe=%q", target.Account, got) + } +} + // 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