fix(datastore): treat empty/0-byte/unparseable XML as missing → serve defaults (#458)

A power-cut on the speaker's NAND can leave a datastore file present but 0-byte (a not-yet-flushed atomicWriteFile write). The read paths now treat empty/0-byte/ unparseable Presets/Recents/Sources the same as missing: GetConfiguredSources serves the managed defaults (so /full self-heals instead of wiping the speaker), GetPresets/GetRecents return an empty list (no more HTTP 500 on the device-level endpoints), and HasConfiguredSources reports a 0-byte file as absent (so the create_default_sources health quick fix is offered again). Read-side resilience only; the write-side durability fix (fsync in atomicWriteFile) follows in a separate PR. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2026-08-18 00:26:29 +00:00 · 2026-06-04 19:23:09 +02:00
parent c297a90be3
commit d7c3976684
2 changed files with 197 additions and 10 deletions
@@ -962,6 +962,16 @@ func (ds *DataStore) readPresetsLocked(account, device string) ([]models.Service
 		return nil, false, err
 	}

+	// An empty / 0-byte Presets.xml (e.g. truncated by an unclean power-cut on
+	// the speaker's NAND) is treated as "no presets" rather than a hard parse
+	// error, so the device-level /presets endpoint returns an empty list
+	// instead of HTTP 500. See #458.
+	if len(bytes.TrimSpace(data)) == 0 {
+		log.Printf("[Datastore] readPresetsLocked: empty/0-byte Presets.xml at %s — treating as no presets (#458)", sanitizeLog(path))
+
+		return []models.ServicePreset{}, false, nil
+	}
+
 	var presetsWrap struct {
 		Presets []struct {
 			ID          string `xml:"id,attr"`
@@ -989,7 +999,9 @@ func (ds *DataStore) readPresetsLocked(account, device string) ([]models.Service
 	needsRewrite := !bytes.Equal(normalized, data)

 	if err := xml.Unmarshal(normalized, &presetsWrap); err != nil {
-		return nil, false, fmt.Errorf("malformed presets XML at %s: %w", path, err)
+		log.Printf("[Datastore] readPresetsLocked: malformed Presets.xml at %s (%v) — treating as no presets (#458)", sanitizeLog(path), err)
+
+		return []models.ServicePreset{}, false, nil
 	}

 	presets := []models.ServicePreset{}
@@ -1225,6 +1237,16 @@ func (ds *DataStore) GetRecents(account, device string) ([]models.ServiceRecent,
 		return nil, err
 	}

+	// An empty / 0-byte Recents.xml (e.g. truncated by an unclean power-cut) is
+	// treated as "no recents" rather than a hard parse error, so the
+	// device-level /recents endpoint returns an empty list instead of HTTP 500.
+	// See #458.
+	if len(bytes.TrimSpace(data)) == 0 {
+		log.Printf("[Datastore] GetRecents: empty/0-byte Recents.xml at %s — treating as no recents (#458)", sanitizeLog(path))
+
+		return []models.ServiceRecent{}, nil
+	}
+
 	type RecentXML struct {
 		DeviceID    string `xml:"deviceID,attr,omitempty"`
 		UtcTime     string `xml:"utcTime,attr,omitempty"`
@@ -1252,7 +1274,9 @@ func (ds *DataStore) GetRecents(account, device string) ([]models.ServiceRecent,

 	var wrap RecentsXML
 	if err := xml.Unmarshal(data, &wrap); err != nil {
-		return nil, fmt.Errorf("malformed recents XML at %s: %w", path, err)
+		log.Printf("[Datastore] GetRecents: malformed Recents.xml at %s (%v) — treating as no recents (#458)", sanitizeLog(path), err)
+
+		return []models.ServiceRecent{}, nil
 	}

 	recents := make([]models.ServiceRecent, 0, len(wrap.Recents))
@@ -1794,18 +1818,34 @@ func (ds *DataStore) GetConfiguredSources(account, device string) ([]models.Conf

 	path := filepath.Join(ds.AccountDeviceDir(account, device), constants.SourcesFile)

+	// defaultSources is the fallback used whenever there is no usable
+	// Sources.xml: file missing (normal for a fresh device) or present but
+	// empty / 0-byte / unparseable. The latter happens when an unclean
+	// power-cut truncates a not-yet-flushed datastore write on the speaker's
+	// NAND; treating it like "missing" lets /full re-serve the managed defaults
+	// so the speaker self-heals instead of dropping all its sources. See #458.
+	defaultSources := func() []models.ConfiguredSource {
+		sources := ds.getInitialSources()
+		ds.DeduceSourceIDs(account, device, sources)
+
+		return sources
+	}
+
 	data, err := ds.rootReadFile(path)
 	if err != nil {
 		if os.IsNotExist(err) {
-			sources := ds.getInitialSources()
-			ds.DeduceSourceIDs(account, device, sources)
-
-			return sources, nil
+			return defaultSources(), nil
 		}

 		return nil, err
 	}

+	if len(bytes.TrimSpace(data)) == 0 {
+		log.Printf("[Datastore] GetConfiguredSources: empty/0-byte Sources.xml at %s — treating as missing, serving defaults (#458)", sanitizeLog(path))
+
+		return defaultSources(), nil
+	}
+
 	type persistentSource struct {
 		DisplayName      string `xml:"displayName,attr,omitempty"`
 		ID               string `xml:"id,attr,omitempty"`
@@ -1830,7 +1870,9 @@ func (ds *DataStore) GetConfiguredSources(account, device string) ([]models.Conf
 	}

 	if err := xml.Unmarshal(data, &sourcesWrap); err != nil {
-		return nil, fmt.Errorf("malformed sources XML at %s: %w", path, err)
+		log.Printf("[Datastore] GetConfiguredSources: malformed Sources.xml at %s (%v) — treating as missing, serving defaults (#458)", sanitizeLog(path), err)
+
+		return defaultSources(), nil
 	}

 	sources := make([]models.ConfiguredSource, len(sourcesWrap.Sources))
@@ -2359,12 +2401,22 @@ func (ds *DataStore) GetETagForPresets(account, device string) int64 {
 	return info.ModTime().UnixNano() / int64(time.Millisecond)
 }

-// HasConfiguredSources reports whether a Sources.xml file exists for the given account and device.
+// HasConfiguredSources reports whether a non-empty Sources.xml file exists for
+// the given account and device. A present-but-0-byte file (truncated by an
+// unclean power-cut) counts as absent. See #458.
 func (ds *DataStore) HasConfiguredSources(account, device string) bool {
 	path := filepath.Join(ds.AccountDeviceDir(account, device), constants.SourcesFile)
-	_, err := ds.rootStat(path)

-	return err == nil
+	data, err := ds.rootReadFile(path)
+	if err != nil {
+		return false
+	}
+
+	// An existing but empty / 0-byte Sources.xml (e.g. truncated by an unclean
+	// power-cut on the speaker's NAND) must not count as "present": otherwise it
+	// hides the sources_xml_present health check and its create_default_sources
+	// quick fix, leaving the device with no managed sources. See #458.
+	return len(bytes.TrimSpace(data)) > 0
 }

 // GetETagForSources returns the ETag (modification time) for the sources file for a specific device.
@@ -0,0 +1,135 @@
+package datastore
+
+import (
+	"os"
+	"path/filepath"
+	"testing"
+
+	"github.com/gesellix/bose-soundtouch/pkg/service/constants"
+)
+
+// These tests cover #458: an unclean power-cut on the speaker's NAND can leave
+// a datastore file present but 0-byte (truncated, not-yet-flushed write). The
+// read paths must treat empty / 0-byte / unparseable files the same as
+// "missing" — serve defaults for sources, return empty lists for presets/recents
+// — instead of advertising nothing on /full (which wipes the speaker) or
+// returning HTTP 500 on the device-level endpoints.
+
+func newTestStore(t *testing.T) (*DataStore, string, string) {
+	t.Helper()
+
+	tempDir, err := os.MkdirTemp("", "st-empty-test-*")
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	t.Cleanup(func() { _ = os.RemoveAll(tempDir) })
+
+	ds := NewDataStore(tempDir)
+	account := "1234567"
+	device := "001122334455"
+
+	dir := ds.AccountDeviceDir(account, device)
+	if err := os.MkdirAll(dir, 0755); err != nil {
+		t.Fatal(err)
+	}
+
+	return ds, account, device
+}
+
+func writeDeviceFile(t *testing.T, ds *DataStore, account, device, name string, content []byte) {
+	t.Helper()
+
+	path := filepath.Join(ds.AccountDeviceDir(account, device), name)
+	if err := os.WriteFile(path, content, 0644); err != nil {
+		t.Fatal(err)
+	}
+}
+
+func TestGetConfiguredSources_EmptyFile_ServesDefaults(t *testing.T) {
+	ds, account, device := newTestStore(t)
+	writeDeviceFile(t, ds, account, device, constants.SourcesFile, []byte{})
+
+	sources, err := ds.GetConfiguredSources(account, device)
+	if err != nil {
+		t.Fatalf("GetConfiguredSources returned error for 0-byte file: %v", err)
+	}
+
+	if len(sources) == 0 {
+		t.Fatal("expected default sources for a 0-byte Sources.xml, got none")
+	}
+
+	types := map[string]bool{}
+	for i := range sources {
+		types[sources[i].SourceKeyType] = true
+	}
+
+	for _, want := range []string{constants.ProviderTunein, constants.ProviderLocalInternetRadio} {
+		if !types[want] {
+			t.Errorf("expected default sources to include %q (ding/radio need it); got %v", want, types)
+		}
+	}
+}
+
+func TestGetConfiguredSources_MalformedFile_ServesDefaults(t *testing.T) {
+	ds, account, device := newTestStore(t)
+	writeDeviceFile(t, ds, account, device, constants.SourcesFile, []byte("<sources><not-closed"))
+
+	sources, err := ds.GetConfiguredSources(account, device)
+	if err != nil {
+		t.Fatalf("GetConfiguredSources returned error for malformed file: %v", err)
+	}
+
+	if len(sources) == 0 {
+		t.Fatal("expected default sources for a malformed Sources.xml, got none")
+	}
+}
+
+func TestGetPresets_EmptyFile_NoError(t *testing.T) {
+	ds, account, device := newTestStore(t)
+	writeDeviceFile(t, ds, account, device, constants.PresetsFile, []byte{})
+
+	presets, err := ds.GetPresets(account, device)
+	if err != nil {
+		t.Fatalf("GetPresets returned error for 0-byte file (would surface as HTTP 500): %v", err)
+	}
+
+	if len(presets) != 0 {
+		t.Errorf("expected no presets for a 0-byte Presets.xml, got %d", len(presets))
+	}
+}
+
+func TestGetRecents_EmptyFile_NoError(t *testing.T) {
+	ds, account, device := newTestStore(t)
+	writeDeviceFile(t, ds, account, device, constants.RecentsFile, []byte{})
+
+	recents, err := ds.GetRecents(account, device)
+	if err != nil {
+		t.Fatalf("GetRecents returned error for 0-byte file (would surface as HTTP 500): %v", err)
+	}
+
+	if len(recents) != 0 {
+		t.Errorf("expected no recents for a 0-byte Recents.xml, got %d", len(recents))
+	}
+}
+
+func TestHasConfiguredSources_EmptyFile_False(t *testing.T) {
+	ds, account, device := newTestStore(t)
+
+	// 0-byte file present must NOT count as "has sources" — otherwise the
+	// sources_xml_present health check stays green and hides the
+	// create_default_sources quick fix.
+	writeDeviceFile(t, ds, account, device, constants.SourcesFile, []byte{})
+
+	if ds.HasConfiguredSources(account, device) {
+		t.Error("HasConfiguredSources returned true for a 0-byte Sources.xml; want false")
+	}
+
+	// A populated file must still count as present.
+	writeDeviceFile(t, ds, account, device, constants.SourcesFile,
+		[]byte(`<?xml version="1.0"?><sources><source><sourceKey type="TUNEIN" account=""/></source></sources>`))
+
+	if !ds.HasConfiguredSources(account, device) {
+		t.Error("HasConfiguredSources returned false for a populated Sources.xml; want true")
+	}
+}