fix(setup): require confirmation before Sync would shrink stored data

SyncDeviceData's syncPresets/syncRecents unconditionally overwrote the datastore with whatever the speaker's live :8090 API returned at that instant, with no check against what's already stored. If the speaker's own local cache was stale or incomplete at that moment (e.g. right after a burst of preset writes, or shortly after a reboot before the speaker resyncs with Marge), Sync would silently persist that bad snapshot over good data. A reporter's fresh #614 repro showed the account's /full response dropping from 6 to 5 presets right after a Sync click, consistent with this mechanism. SyncDeviceData now diffs a fresh live fetch against what's stored before writing anything; if applying would shrink either list, it returns the diff (via the new SyncResourceDiff/SyncResult types) without writing unless the caller passes confirmed=true. HandleInitialSync surfaces this as a 409 with the diff JSON; every call (confirmed or not) re-fetches live from the speaker, so a confirmed retry re-checks reality rather than replaying a stale snapshot. Sources sync is left unconditional, as before -- lower risk in practice and out of scope for this fix. fetchLivePresets/fetchLiveRecents are extracted pure-fetch helpers; syncPresets/syncRecents keep their unconditional-apply behavior (used directly by existing tests) since the button-driven path now goes through the diff/confirm guard instead. Adds TestSyncDeviceData_DestructiveSyncRequiresConfirmation covering both the refusal and the confirmed-retry path. Frontend wiring (script.js's startSync + real per-resource result rendering, replacing the current hardcoded "OK" text) is a follow-up commit on this branch. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
2026-08-24 14:47:23 +00:00 · 2026-08-23 14:19:40 +02:00
parent d4f4b4fb80
commit 2bac4fb208
6 changed files with 370 additions and 29 deletions
@@ -1274,7 +1274,16 @@ func (s *Server) HandleTestDNSRedirection(w http.ResponseWriter, r *http.Request
 	}
 }

-// HandleInitialSync fetches presets, recents and sources from the device and saves them to the datastore.
+// HandleInitialSync fetches presets, recents and sources from the device
+// and saves them to the datastore.
+//
+// If applying the fetched presets/recents would shrink what's already
+// stored, the sync is not applied — the response comes back 409 with the
+// diff describing what would be removed — unless the caller passes
+// ?confirmed=true, in which case it's applied unconditionally. Every call
+// re-fetches live from the speaker at that moment (see
+// setup.SyncDeviceData), so a confirmed retry re-checks current reality
+// rather than replaying a possibly-stale earlier response.
 func (s *Server) HandleInitialSync(w http.ResponseWriter, r *http.Request) {
 	deviceID := chi.URLParam(r, "deviceId")
 	if deviceID == "" {
@@ -1288,13 +1297,25 @@ func (s *Server) HandleInitialSync(w http.ResponseWriter, r *http.Request) {
 		return
 	}

-	if err := s.sm.SyncDeviceData(deviceIP); err != nil {
+	confirmed := r.URL.Query().Get("confirmed") == "true"
+
+	result, err := s.sm.SyncDeviceData(deviceIP, confirmed)
+	if err != nil {
 		http.Error(w, err.Error(), http.StatusInternalServerError)
 		return
 	}

-	w.WriteHeader(http.StatusOK)
-	_, _ = w.Write([]byte(`{"ok": true}`))
+	w.Header().Set("Content-Type", "application/json")
+
+	if !result.Applied {
+		w.WriteHeader(http.StatusConflict)
+	} else {
+		w.WriteHeader(http.StatusOK)
+	}
+
+	if encodeErr := json.NewEncoder(w).Encode(result); encodeErr != nil {
+		log.Printf("HandleInitialSync: failed to encode result for device %s: %s", sanitizeLog(deviceID), sanitizeErr(encodeErr))
+	}
 }

 // HandleRebootDevice reboots a device.
@@ -123,7 +123,7 @@ func TestIssue234_FactoryResetSpeakerSyncsReducedSources(t *testing.T) {
 	// SyncDeviceData derives accountID/deviceID from /info; with
 	// an empty margeAccountUUID the account falls through to
 	// "default".
-	if err := m.SyncDeviceData(deviceIP); err != nil {
+	if _, err := m.SyncDeviceData(deviceIP, false); err != nil {
 		t.Fatalf("SyncDeviceData: %v", err)
 	}

@@ -2635,12 +2635,46 @@ func (m *Manager) resolveIP(host string, client SSHClient) (string, error) {
 		ErrResolvedFromServiceOnly, host, resolved)
 }

-// SyncDeviceData fetches presets, recents and sources from the device and saves them to the datastore.
-func (m *Manager) SyncDeviceData(deviceIP string) error {
+// SyncResourceDiff describes what a Data Sync would change for one
+// datastore resource (presets or recents): what's currently stored versus
+// what the speaker's own live :8090 API returned just now.
+type SyncResourceDiff struct {
+	Resource      string   `json:"resource"`
+	CurrentCount  int      `json:"currentCount"`
+	IncomingCount int      `json:"incomingCount"`
+	Removed       []string `json:"removed,omitempty"`
+	Destructive   bool     `json:"destructive"`
+}
+
+// SyncResult is the outcome of a SyncDeviceData call: whether it actually
+// wrote anything, and the per-resource diff that led to that decision.
+type SyncResult struct {
+	Applied     bool               `json:"applied"`
+	Destructive bool               `json:"destructive"`
+	Diffs       []SyncResourceDiff `json:"diffs"`
+}
+
+// SyncDeviceData fetches presets, recents and sources from the device and
+// saves them to the datastore.
+//
+// Presets and recents are fetched live from the speaker's own :8090 API and
+// would previously overwrite the datastore unconditionally — including with
+// an empty or shrunk list if the speaker's own local cache happened to be
+// stale or incomplete at that exact moment (e.g. right after a burst of
+// preset writes, or shortly after a reboot before the speaker has resynced
+// with Marge). That's a real, confirmed mechanism for #614's "Sync wipes my
+// presets" reports. Now: if applying would shrink either list relative to
+// what's already stored, SyncDeviceData does NOT write — it reports the
+// diff instead — unless confirmed is true. There is no cached "preview"
+// state: every call (confirmed or not) re-fetches live from the speaker at
+// that moment, so confirming re-checks reality rather than replaying a
+// possibly-stale earlier snapshot. Sources are left unconditional, as
+// before — a source-list change is comparatively low-risk and self-healing.
+func (m *Manager) SyncDeviceData(deviceIP string, confirmed bool) (SyncResult, error) {
 	// 1. Fetch info to get Serial Number (account identifier)
 	info, err := m.GetLiveDeviceInfo(deviceIP)
 	if err != nil {
-		return fmt.Errorf("failed to get device info: %w", err)
+		return SyncResult{}, fmt.Errorf("failed to get device info: %w", err)
 	}

 	log.Printf("Starting sync for device at %s: Name='%s', DeviceID='%s', SerialNumber='%s'",
@@ -2652,7 +2686,7 @@ func (m *Manager) SyncDeviceData(deviceIP string) error {
 	deviceID := info.DeviceID
 	if deviceID == "" {
 		log.Printf("No deviceID found in /info response for device '%s' at %s", sanitizeLog(info.Name), sanitizeLog(deviceIP))
-		return fmt.Errorf("no deviceID found in /info response for device at %s - cannot sync without canonical device identifier", deviceIP)
+		return SyncResult{}, fmt.Errorf("no deviceID found in /info response for device at %s - cannot sync without canonical device identifier", deviceIP)
 	}

 	log.Printf("Using deviceID '%s' for sync operations (MAC address from /info)", sanitizeLog(deviceID))
@@ -2676,11 +2710,39 @@ func (m *Manager) SyncDeviceData(deviceIP string) error {
 		accountID = "default"
 	}

-	// 2. Fetch Presets from :8090
-	m.syncPresets(deviceIP, accountID, deviceID)
+	// 2. Diff presets and recents against a fresh live fetch, before writing
+	// anything.
+	presetDiff, incomingPresets, presetErr := m.presetSyncDiff(deviceIP, accountID, deviceID)
+	if presetErr != nil {
+		log.Printf("[SYNC_ERR] Failed to fetch presets for %s: %v", sanitizeLog(deviceIP), presetErr)
+	}

-	// 3. Fetch Recents from :8090
-	m.syncRecents(deviceIP, accountID, deviceID)
+	recentDiff, incomingRecents, recentErr := m.recentSyncDiff(deviceIP, accountID, deviceID)
+	if recentErr != nil {
+		log.Printf("[SYNC_ERR] Failed to fetch recents for %s: %v", sanitizeLog(deviceIP), recentErr)
+	}
+
+	result := SyncResult{
+		Diffs:       []SyncResourceDiff{presetDiff, recentDiff},
+		Destructive: presetDiff.Destructive || recentDiff.Destructive,
+	}
+
+	if result.Destructive && !confirmed {
+		log.Printf("[SYNC] Sync for %s would shrink stored data (presets %d->%d, recents %d->%d) — awaiting confirmation, not writing anything",
+			sanitizeLog(deviceIP), presetDiff.CurrentCount, presetDiff.IncomingCount, recentDiff.CurrentCount, recentDiff.IncomingCount)
+
+		return result, nil
+	}
+
+	// 3. Apply presets/recents (skip whichever one failed to fetch, leaving
+	// the existing stored data untouched rather than wiping it).
+	if presetErr == nil {
+		_ = m.DataStore.SavePresets(accountID, deviceID, incomingPresets)
+	}
+
+	if recentErr == nil {
+		_ = m.DataStore.SaveRecents(accountID, deviceID, incomingRecents)
+	}

 	// 4. Fetch Sources
 	m.syncSources(deviceIP, accountID, deviceID)
@@ -2697,28 +2759,113 @@ func (m *Manager) SyncDeviceData(deviceIP string) error {
 	// 6. Create off-device backup of system configuration
 	_ = m.BackupConfigOffDevice(deviceIP)

-	return nil
+	result.Applied = true
+
+	return result, nil
 }

-func (m *Manager) syncPresets(deviceIP, accountID, deviceID string) {
+// presetSyncDiff fetches the live preset list from the speaker and compares
+// it against what's currently stored, without writing anything.
+func (m *Manager) presetSyncDiff(deviceIP, accountID, deviceID string) (SyncResourceDiff, []models.ServicePreset, error) {
+	current, _ := m.DataStore.GetPresets(accountID, deviceID)
+
+	incoming, err := m.fetchLivePresets(deviceIP)
+	if err != nil {
+		return SyncResourceDiff{Resource: "presets", CurrentCount: len(current), IncomingCount: len(current)}, nil, err
+	}
+
+	return diffPresets(current, incoming), incoming, nil
+}
+
+// recentSyncDiff fetches the live recents list from the speaker and
+// compares it against what's currently stored, without writing anything.
+func (m *Manager) recentSyncDiff(deviceIP, accountID, deviceID string) (SyncResourceDiff, []models.ServiceRecent, error) {
+	current, _ := m.DataStore.GetRecents(accountID, deviceID)
+
+	incoming, err := m.fetchLiveRecents(deviceIP)
+	if err != nil {
+		return SyncResourceDiff{Resource: "recents", CurrentCount: len(current), IncomingCount: len(current)}, nil, err
+	}
+
+	return diffRecents(current, incoming), incoming, nil
+}
+
+// diffPresets compares a stored preset list against a freshly-fetched one.
+// Removed lists the names of presets present in current but absent (by
+// button/slot ID) from incoming — this is what tells an operator "Sync
+// would remove preset 6: Ici Roussillon" instead of just a bare count.
+func diffPresets(current, incoming []models.ServicePreset) SyncResourceDiff {
+	incomingIDs := make(map[string]bool, len(incoming))
+	for i := range incoming {
+		if incoming[i].ID != "" {
+			incomingIDs[incoming[i].ID] = true
+		}
+	}
+
+	var removed []string
+
+	for i := range current {
+		if current[i].ID != "" && current[i].Name != "" && !incomingIDs[current[i].ID] {
+			removed = append(removed, current[i].Name)
+		}
+	}
+
+	return SyncResourceDiff{
+		Resource:      "presets",
+		CurrentCount:  len(current),
+		IncomingCount: len(incoming),
+		Removed:       removed,
+		Destructive:   len(incoming) < len(current),
+	}
+}
+
+// diffRecents compares a stored recents list against a freshly-fetched one.
+// Recents have no stable per-entry ID the way presets do (they're an
+// ordered, time-sorted, size-capped list), so entries are matched by
+// content Location instead.
+func diffRecents(current, incoming []models.ServiceRecent) SyncResourceDiff {
+	incomingLocations := make(map[string]bool, len(incoming))
+	for i := range incoming {
+		if incoming[i].Location != "" {
+			incomingLocations[incoming[i].Location] = true
+		}
+	}
+
+	var removed []string
+
+	for i := range current {
+		if current[i].Location != "" && current[i].Name != "" && !incomingLocations[current[i].Location] {
+			removed = append(removed, current[i].Name)
+		}
+	}
+
+	return SyncResourceDiff{
+		Resource:      "recents",
+		CurrentCount:  len(current),
+		IncomingCount: len(incoming),
+		Removed:       removed,
+		Destructive:   len(incoming) < len(current),
+	}
+}
+
+// fetchLivePresets fetches the current preset list straight from the
+// speaker's own local :8090 API. It does not touch the datastore.
+func (m *Manager) fetchLivePresets(deviceIP string) ([]models.ServicePreset, error) {
 	presetsURL := fmt.Sprintf("http://%s:8090/presets", deviceIP)
 	if _, _, splitErr := net.SplitHostPort(deviceIP); splitErr == nil {
 		presetsURL = fmt.Sprintf("http://%s/presets", deviceIP)
 	}

-	log.Printf("[SYNC] Syncing presets for %s", sanitizeLog(deviceIP))
-
 	resp, err := m.HTTPGet(presetsURL)
 	if err != nil {
-		log.Printf("[SYNC_ERR] Failed to fetch presets for %s: %v", sanitizeLog(deviceIP), err)
-		return
+		return nil, err
 	}

 	defer func() { _ = resp.Body.Close() }()

 	var ps models.Presets
 	if decodeErr := xml.NewDecoder(resp.Body).Decode(&ps); decodeErr != nil {
-		return
+		return nil, decodeErr
 	}

 	var servicePresets []models.ServicePreset
@@ -2762,10 +2909,28 @@ func (m *Manager) syncPresets(deviceIP, accountID, deviceID string) {
 		})
 	}

-	_ = m.DataStore.SavePresets(accountID, deviceID, servicePresets)
+	return servicePresets, nil
 }

-func (m *Manager) syncRecents(deviceIP, accountID, deviceID string) {
+// syncPresets fetches the live preset list and unconditionally persists it.
+// Used directly by tests exercising the raw fetch+save behaviour; the
+// button-driven path goes through SyncDeviceData's diff/confirm guard
+// instead.
+func (m *Manager) syncPresets(deviceIP, accountID, deviceID string) {
+	log.Printf("[SYNC] Syncing presets for %s", sanitizeLog(deviceIP))
+
+	presets, err := m.fetchLivePresets(deviceIP)
+	if err != nil {
+		log.Printf("[SYNC_ERR] Failed to fetch presets for %s: %v", sanitizeLog(deviceIP), err)
+		return
+	}
+
+	_ = m.DataStore.SavePresets(accountID, deviceID, presets)
+}
+
+// fetchLiveRecents fetches the current recents list straight from the
+// speaker's own local :8090 API. It does not touch the datastore.
+func (m *Manager) fetchLiveRecents(deviceIP string) ([]models.ServiceRecent, error) {
 	recentsURL := fmt.Sprintf("http://%s:8090/recents", deviceIP)
 	if _, _, splitErr := net.SplitHostPort(deviceIP); splitErr == nil {
 		recentsURL = fmt.Sprintf("http://%s/recents", deviceIP)
@@ -2773,14 +2938,14 @@ func (m *Manager) syncRecents(deviceIP, accountID, deviceID string) {

 	resp, err := m.HTTPGet(recentsURL)
 	if err != nil {
-		return
+		return nil, err
 	}

 	defer func() { _ = resp.Body.Close() }()

 	var rr models.RecentsResponse
 	if decodeErr := xml.NewDecoder(resp.Body).Decode(&rr); decodeErr != nil {
-		return
+		return nil, decodeErr
 	}

 	var serviceRecents []models.ServiceRecent
@@ -2807,7 +2972,20 @@ func (m *Manager) syncRecents(deviceIP, accountID, deviceID string) {
 		})
 	}

-	_ = m.DataStore.SaveRecents(accountID, deviceID, serviceRecents)
+	return serviceRecents, nil
+}
+
+// syncRecents fetches the live recents list and unconditionally persists
+// it. Used directly by tests exercising the raw fetch+save behaviour; the
+// button-driven path goes through SyncDeviceData's diff/confirm guard
+// instead.
+func (m *Manager) syncRecents(deviceIP, accountID, deviceID string) {
+	recents, err := m.fetchLiveRecents(deviceIP)
+	if err != nil {
+		return
+	}
+
+	_ = m.DataStore.SaveRecents(accountID, deviceID, recents)
 }

 func (m *Manager) syncSources(deviceIP, accountID, deviceID string) {
@@ -0,0 +1,142 @@
+package setup
+
+import (
+	"fmt"
+	"net/http"
+	"net/http/httptest"
+	"os"
+	"testing"
+
+	"github.com/gesellix/bose-soundtouch/pkg/models"
+	"github.com/gesellix/bose-soundtouch/pkg/service/datastore"
+)
+
+// TestSyncDeviceData_DestructiveSyncRequiresConfirmation is a regression
+// test for #614's 2026-08-23 finding: SyncDeviceData used to overwrite the
+// datastore unconditionally with whatever the speaker's live :8090 API
+// returned, even if that snapshot had fewer presets than what was already
+// stored — e.g. because the speaker's own local cache was stale or
+// incomplete at that exact moment. This is a real, confirmed mechanism for
+// "Sync wipes my presets" reports.
+//
+// A device already has 3 stored presets. The mock speaker's live /presets
+// only reports 1. The first (unconfirmed) sync must NOT write anything and
+// must report the shrink; a confirmed retry must apply it.
+func TestSyncDeviceData_DestructiveSyncRequiresConfirmation(t *testing.T) {
+	const (
+		accountID = "1234567"
+		deviceID  = "AABBCCDDEEFF"
+	)
+
+	mockDevice := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		switch r.URL.Path {
+		case "/info":
+			w.Header().Set("Content-Type", "application/xml")
+			fmt.Fprintf(w, `<?xml version="1.0" encoding="UTF-8"?>
+<info deviceID="%s">
+    <name>Test Device</name>
+    <type>SoundTouch 20</type>
+    <margeAccountUUID>%s</margeAccountUUID>
+</info>`, deviceID, accountID)
+		case "/presets":
+			// Only one preset survived on the speaker's own live cache —
+			// the datastore already has three (seeded below).
+			w.Header().Set("Content-Type", "application/xml")
+			fmt.Fprint(w, `<?xml version="1.0" encoding="UTF-8"?>
+<presets>
+    <preset id="1">
+        <ContentItem source="LOCAL_INTERNET_RADIO" type="stationurl" location="/custom/v1/playback/station1" isPresetable="true">
+            <itemName>Station 1</itemName>
+        </ContentItem>
+    </preset>
+</presets>`)
+		case "/recents":
+			w.Header().Set("Content-Type", "application/xml")
+			fmt.Fprint(w, `<?xml version="1.0" encoding="UTF-8"?><recents></recents>`)
+		default:
+			w.WriteHeader(http.StatusNotFound)
+		}
+	}))
+	defer mockDevice.Close()
+
+	tempDir, err := os.MkdirTemp("", "sync-destructive-guard-*")
+	if err != nil {
+		t.Fatalf("tempdir: %v", err)
+	}
+	defer func() { _ = os.RemoveAll(tempDir) }()
+
+	ds := datastore.NewDataStore(tempDir)
+
+	seeded := []models.ServicePreset{
+		{ID: "1", ButtonNumber: "1", ServiceContentItem: models.ServiceContentItem{Name: "Station 1"}},
+		{ID: "2", ButtonNumber: "2", ServiceContentItem: models.ServiceContentItem{Name: "Station 2"}},
+		{ID: "3", ButtonNumber: "3", ServiceContentItem: models.ServiceContentItem{Name: "Station 3"}},
+	}
+	if err := ds.SavePresets(accountID, deviceID, seeded); err != nil {
+		t.Fatalf("seed SavePresets: %v", err)
+	}
+
+	m := NewManager("http://localhost:8000", ds, nil)
+	deviceIP := mockDevice.Listener.Addr().String()
+
+	// Unconfirmed: must refuse to write and report the shrink.
+	result, err := m.SyncDeviceData(deviceIP, false)
+	if err != nil {
+		t.Fatalf("SyncDeviceData(confirmed=false): %v", err)
+	}
+
+	if result.Applied {
+		t.Fatal("expected unconfirmed destructive sync to NOT apply")
+	}
+
+	if !result.Destructive {
+		t.Fatal("expected result.Destructive=true for a 3->1 preset shrink")
+	}
+
+	var presetDiff *SyncResourceDiff
+	for i := range result.Diffs {
+		if result.Diffs[i].Resource == "presets" {
+			presetDiff = &result.Diffs[i]
+		}
+	}
+
+	if presetDiff == nil {
+		t.Fatal("expected a presets diff in the result")
+	}
+
+	if presetDiff.CurrentCount != 3 || presetDiff.IncomingCount != 1 {
+		t.Errorf("expected presets diff 3->1, got %d->%d", presetDiff.CurrentCount, presetDiff.IncomingCount)
+	}
+
+	if len(presetDiff.Removed) != 2 {
+		t.Errorf("expected 2 removed preset names (slots 2 and 3), got %v", presetDiff.Removed)
+	}
+
+	presetsAfterRefusal, err := ds.GetPresets(accountID, deviceID)
+	if err != nil {
+		t.Fatalf("GetPresets after refused sync: %v", err)
+	}
+
+	if len(presetsAfterRefusal) != 3 {
+		t.Fatalf("expected the original 3 presets to survive an unconfirmed destructive sync, got %d", len(presetsAfterRefusal))
+	}
+
+	// Confirmed: must re-check fresh state and apply.
+	result, err = m.SyncDeviceData(deviceIP, true)
+	if err != nil {
+		t.Fatalf("SyncDeviceData(confirmed=true): %v", err)
+	}
+
+	if !result.Applied {
+		t.Fatal("expected confirmed destructive sync to apply")
+	}
+
+	presetsAfterConfirm, err := ds.GetPresets(accountID, deviceID)
+	if err != nil {
+		t.Fatalf("GetPresets after confirmed sync: %v", err)
+	}
+
+	if len(presetsAfterConfirm) != 1 {
+		t.Fatalf("expected confirmed sync to shrink to 1 preset, got %d", len(presetsAfterConfirm))
+	}
+}
@@ -64,7 +64,7 @@ func TestSyncDeviceData_UsesDeviceID(t *testing.T) {
 	manager := NewManager("http://localhost:8000", ds, cm)

 	// Test SyncDeviceData
-	err := manager.SyncDeviceData(serverHost)
+	_, err := manager.SyncDeviceData(serverHost, false)
 	if err != nil {
 		t.Fatalf("SyncDeviceData failed: %v", err)
 	}
@@ -130,7 +130,7 @@ func TestSyncDeviceData_NoDeviceID_ShouldFail(t *testing.T) {
 	manager := NewManager("http://localhost:8000", ds, cm)

 	// Test SyncDeviceData - should fail
-	err := manager.SyncDeviceData(serverHost)
+	_, err := manager.SyncDeviceData(serverHost, false)
 	if err == nil {
 		t.Fatal("SyncDeviceData should have failed when deviceID is empty")
 	}
@@ -221,7 +221,7 @@ func TestSyncDeviceData_FallbackToExistingDeviceMapping(t *testing.T) {
 	manager := NewManager("http://localhost:8000", ds, cm)

 	// Sync should work and use MAC address
-	err := manager.SyncDeviceData(serverHost)
+	_, err := manager.SyncDeviceData(serverHost, false)
 	if err != nil {
 		t.Fatalf("SyncDeviceData failed: %v", err)
 	}
@@ -279,7 +279,7 @@ func TestSyncSources_Format(t *testing.T) {
 	deviceIP := mockDevice.Listener.Addr().String()
 	accountID := "1234567"
 	deviceID := "001122334455"
-	err = m.SyncDeviceData(deviceIP)
+	_, err = m.SyncDeviceData(deviceIP, false)
 	if err != nil {
 		t.Fatalf("SyncDeviceData failed: %v", err)
 	}