From c668c732dfb129b80608d9f7f683b154fe0d9c19 Mon Sep 17 00:00:00 2001 From: Tobias Gesellchen Date: Sun, 17 May 2026 11:33:05 +0200 Subject: [PATCH] fix(#308): handle placeholder presets without panicking The ST10's /presets response after a factory reset emits self-closing entries with no ContentItem child. cmd/soundtouch-cli's getPresets() handled the missing ContentItem in GetDisplayName() but then dereferenced preset.ContentItem.Source on the next line, panicking with "invalid memory address or nil pointer dereference" the moment the loop reached the first empty entry. A second placeholder shape was observed on healthy devices that were never reset: . ContentItem is non-nil here, so the previous "ContentItem != nil" guard at other call sites still let these placeholders through into listings and into the AfterTouch datastore. Fix shape: pkg/models/presets.go - extend Preset.IsEmpty() to recognise both shapes (ContentItem == nil, OR Source == "" / "INVALID_SOURCE"). HasPresets, GetEmptyPresetSlots and GetUsedPresetSlots become honest about which slots actually carry playable content. cmd/soundtouch-cli/cmd_info.go (the crash site) - filter the slice via IsEmpty before the print loop, and switch the still-printed fields to the existing nil-safe Get* helpers. pkg/service/setup/setup.go - upgrade syncPresets's "ContentItem == nil" continue-guard to IsEmpty so Shape B placeholders don't get persisted in the AfterTouch datastore and then surface as junk rows in the admin web UI. cmd/soundtouch-cli/cmd_events.go, cmd/websocket-demo/main.go - same nil-guard upgrade. These already nil-checked so were crash-safe; the change is for consistency and to stop printing "Preset 0: (INVALID_SOURCE)" demo lines. examples/preset-management/main.go - had the same latent crash as cmd_info.go; same fix shape. Regression tests in pkg/models/presets_test.go cover both shapes using the exact XML observed in the wild: the reporter's three placeholders plus the three INVALID_SOURCE entries from a live device. The reporter XML test walks every preset through the same accessor path the CLI used and asserts no panic. The soundtouch-web Go code does not deref preset.ContentItem.X anywhere - presets flow through as JSON - so no separate crash trap exists there. The web frontend will pick up the cleaner data once syncPresets stops persisting placeholders. Closes #308 Co-Authored-By: Claude Opus 4.7 (1M context) --- cmd/soundtouch-cli/cmd_events.go | 8 +- cmd/soundtouch-cli/cmd_info.go | 27 +++-- cmd/websocket-demo/main.go | 8 +- examples/preset-management/main.go | 28 ++++- pkg/models/presets.go | 20 ++- pkg/models/presets_test.go | 188 +++++++++++++++++++++++++++++ pkg/service/setup/setup.go | 7 +- 7 files changed, 264 insertions(+), 22 deletions(-) create mode 100644 pkg/models/presets_test.go diff --git a/cmd/soundtouch-cli/cmd_events.go b/cmd/soundtouch-cli/cmd_events.go index 86df8a1..baedb9b 100644 --- a/cmd/soundtouch-cli/cmd_events.go +++ b/cmd/soundtouch-cli/cmd_events.go @@ -413,9 +413,11 @@ func handlePresetEvent(event *models.PresetUpdatedEvent, verbose bool) { for _, preset := range presets.Preset { fmt.Printf(" 📻 Preset %d:", preset.ID) - if preset.ContentItem != nil { - fmt.Printf(" %s", preset.ContentItem.ItemName) - fmt.Printf(" (%s)", preset.ContentItem.Source) + // IsEmpty catches both and INVALID_SOURCE + // placeholders; using the nil-safe helpers below means the + // inner Printf never dereferences a nil ContentItem. + if !preset.IsEmpty() { + fmt.Printf(" %s (%s)", preset.GetDisplayName(), preset.GetSource()) } fmt.Println() diff --git a/cmd/soundtouch-cli/cmd_info.go b/cmd/soundtouch-cli/cmd_info.go index 18174d0..5d80d0a 100644 --- a/cmd/soundtouch-cli/cmd_info.go +++ b/cmd/soundtouch-cli/cmd_info.go @@ -177,23 +177,36 @@ func getPresets(c *cli.Context) error { fmt.Printf("Device Presets:\n") - if len(presets.Preset) == 0 { + // Filter out placeholder presets the firmware emits for unconfigured + // slots (issue #308): self-closing after factory reset, + // or on healthy devices. + // IsEmpty covers both shapes; accessing fields like ContentItem.Source + // directly on the first shape panics. + configured := make([]models.Preset, 0, len(presets.Preset)) + + for _, p := range presets.Preset { + if !p.IsEmpty() { + configured = append(configured, p) + } + } + + if len(configured) == 0 { fmt.Printf(" No presets configured\n") return nil } fmt.Printf(" Configured Presets:\n") - for _, preset := range presets.Preset { + for _, preset := range configured { fmt.Printf(" %d. %s\n", preset.ID, preset.GetDisplayName()) - fmt.Printf(" Source: %s\n", preset.ContentItem.Source) + fmt.Printf(" Source: %s\n", preset.GetSource()) - if preset.ContentItem.SourceAccount != "" && preset.ContentItem.SourceAccount != preset.ContentItem.Source { - fmt.Printf(" Account: %s\n", preset.ContentItem.SourceAccount) + if account := preset.GetSourceAccount(); account != "" && account != preset.GetSource() { + fmt.Printf(" Account: %s\n", account) } - if preset.ContentItem.Location != "" { - fmt.Printf(" Location: %s\n", preset.ContentItem.Location) + if location := preset.GetLocation(); location != "" { + fmt.Printf(" Location: %s\n", location) } // Show preset creation time if available diff --git a/cmd/websocket-demo/main.go b/cmd/websocket-demo/main.go index 4ddd32e..13a4eaa 100644 --- a/cmd/websocket-demo/main.go +++ b/cmd/websocket-demo/main.go @@ -355,9 +355,11 @@ func handlePreset(event *models.PresetUpdatedEvent, verbose bool) { for _, preset := range presets.Preset { fmt.Printf(" 📻 Preset %d:", preset.ID) - if preset.ContentItem != nil { - fmt.Printf(" %s", preset.ContentItem.ItemName) - fmt.Printf(" (%s)", preset.ContentItem.Source) + // IsEmpty catches both and INVALID_SOURCE + // placeholders; using the nil-safe helpers below means the + // inner Printf never dereferences a nil ContentItem. + if !preset.IsEmpty() { + fmt.Printf(" %s (%s)", preset.GetDisplayName(), preset.GetSource()) } fmt.Println() diff --git a/examples/preset-management/main.go b/examples/preset-management/main.go index d935e33..2e18f03 100644 --- a/examples/preset-management/main.go +++ b/examples/preset-management/main.go @@ -96,22 +96,38 @@ func showCurrentPresets(c *client.Client) error { return err } - if len(presets.Preset) == 0 { + // Filter out placeholder presets (issue #308): self-closing + // entries from a factory-reset device and + // INVALID_SOURCE placeholders from healthy devices both panic if + // their fields are dereferenced directly. + configured := make([]models.Preset, 0, len(presets.Preset)) + + for _, p := range presets.Preset { + if !p.IsEmpty() { + configured = append(configured, p) + } + } + + if len(configured) == 0 { fmt.Println(" 📭 No presets configured") return nil } - fmt.Printf(" 📻 Found %d configured presets:\n", len(presets.Preset)) - for _, preset := range presets.Preset { + fmt.Printf(" 📻 Found %d configured presets:\n", len(configured)) + + for _, preset := range configured { fmt.Printf(" %d. %s\n", preset.ID, preset.GetDisplayName()) - fmt.Printf(" Source: %s\n", preset.ContentItem.Source) - if preset.ContentItem.Location != "" { - fmt.Printf(" Location: %s\n", preset.ContentItem.Location) + fmt.Printf(" Source: %s\n", preset.GetSource()) + + if location := preset.GetLocation(); location != "" { + fmt.Printf(" Location: %s\n", location) } + if preset.CreatedOn != nil && *preset.CreatedOn != 0 { createdTime := time.Unix(*preset.CreatedOn, 0) fmt.Printf(" Created: %s\n", createdTime.Format("2006-01-02 15:04:05")) } + fmt.Println() } diff --git a/pkg/models/presets.go b/pkg/models/presets.go index 0723f76..000af75 100644 --- a/pkg/models/presets.go +++ b/pkg/models/presets.go @@ -71,9 +71,25 @@ func (p *Preset) IsSpotifyPreset() bool { return p.ContentItem != nil && p.ContentItem.Source == "SPOTIFY" } -// IsEmpty returns true if the preset has no content +// IsEmpty returns true if the preset has no playable content. Two +// placeholder shapes are observed in the wild and both count as empty: +// +// - (or ) — no ContentItem child at all. +// Emitted by some firmware after a factory reset (issue #308). +// - +// — a placeholder ContentItem the firmware uses for unconfigured +// slots, observed on FW 27.0.6 even on devices that were never +// reset. +// +// Treating both as empty keeps GetEmptyPresetSlots, GetUsedPresetSlots +// and HasPresets honest, and lets callers safely skip placeholders +// before formatting a preset for display. func (p *Preset) IsEmpty() bool { - return p.ContentItem == nil + if p.ContentItem == nil { + return true + } + + return p.ContentItem.Source == "" || p.ContentItem.Source == "INVALID_SOURCE" } // GetSource returns the source of the preset content diff --git a/pkg/models/presets_test.go b/pkg/models/presets_test.go new file mode 100644 index 0000000..721150c --- /dev/null +++ b/pkg/models/presets_test.go @@ -0,0 +1,188 @@ +package models + +import ( + "encoding/xml" + "testing" +) + +// reporterXML is the /presets response captured from the speaker that +// crashed the CLI in issue #308 (ST10 post factory reset, FW 27.0.6). +// Two configured presets followed by three self-closing +// placeholders. The original crash happened on the first : +// GetDisplayName() handled the nil ContentItem, but the very next +// line dereferenced ContentItem.Source unconditionally. +const reporterXML = ` + + +MDR JUMP + + + + + +SUNSHINE LIVE + +http://cdn-profiles.tunein.com/s10637/images/logog.png?t=637791086340000000 + + + + + + +` + +// invalidSourceXML is the second placeholder shape observed in the +// wild (gesellix's ST10/ST20 on FW 27.0.6, never factory-reset). The +// firmware here populates ContentItem with source="INVALID_SOURCE" +// for unconfigured slots — non-nil but useless, so the old IsEmpty +// (== nil only) returned false and the placeholders polluted listings. +const invalidSourceXML = `` + + `` + + `` + + `` + + `Sand Castle Tapes` + + `Unpluggedhttps://example.com/art.jpg` + + `SMOOTH JAZZhttps://example.com/logo.png` + + `` + +func TestIsEmpty_NoContentItem(t *testing.T) { + // Shape A: — ContentItem == nil. This is the shape + // behind the issue #308 crash. + p := Preset{} + if !p.IsEmpty() { + t.Error("IsEmpty() should be true when ContentItem is nil") + } +} + +func TestIsEmpty_InvalidSourcePlaceholder(t *testing.T) { + // Shape B: ContentItem present but Source == "INVALID_SOURCE". + // Observed on devices that never had a factory reset. + p := Preset{ + ContentItem: &ContentItem{Source: "INVALID_SOURCE", IsPresetable: true}, + } + if !p.IsEmpty() { + t.Error("IsEmpty() should be true when ContentItem has INVALID_SOURCE") + } +} + +func TestIsEmpty_EmptySource(t *testing.T) { + // A ContentItem with no Source can't drive playback. Treat it + // as empty too — defensive, not tied to a single observed shape. + p := Preset{ContentItem: &ContentItem{}} + if !p.IsEmpty() { + t.Error("IsEmpty() should be true when ContentItem.Source is empty") + } +} + +func TestIsEmpty_RealPreset(t *testing.T) { + p := Preset{ + ContentItem: &ContentItem{ + Source: "TUNEIN", + ItemName: "MDR JUMP", + }, + } + if p.IsEmpty() { + t.Error("IsEmpty() should be false for a configured preset") + } +} + +func TestReporterXML_DoesNotPanicAndFiltersEmpty(t *testing.T) { + // Reproducer for issue #308: simulate the loop that crashed the + // CLI. The fix is two-fold: IsEmpty now recognises , + // and callers use the nil-safe Get* accessors. Walking every + // preset through the same paths the CLI uses must not panic on + // any entry. + var presets Presets + + if err := xml.Unmarshal([]byte(reporterXML), &presets); err != nil { + t.Fatalf("Failed to unmarshal reporter XML: %v", err) + } + + if got := len(presets.Preset); got != 5 { + t.Fatalf("Expected 5 preset entries (2 configured + 3 empty), got %d", got) + } + + emptyCount := 0 + configuredCount := 0 + + for _, p := range presets.Preset { + // The CLI now skips empty presets before dereferencing + // anything on ContentItem. The IsEmpty call must catch all + // three entries. + if p.IsEmpty() { + emptyCount++ + continue + } + + configuredCount++ + + // These calls would have panicked pre-fix on the empty + // entries; here they exercise the still-printed paths for + // the real ones. + _ = p.GetDisplayName() + _ = p.GetSource() + _ = p.GetSourceAccount() + _ = p.GetLocation() + } + + if emptyCount != 3 { + t.Errorf("Expected 3 empty presets, got %d", emptyCount) + } + + if configuredCount != 2 { + t.Errorf("Expected 2 configured presets, got %d", configuredCount) + } + + // HasPresets should reflect "there are real presets" — not + // confused by the placeholders. + if !presets.HasPresets() { + t.Error("HasPresets() should be true (2 real presets present)") + } + + if got := presets.GetUsedPresetSlots(); len(got) != 2 { + t.Errorf("GetUsedPresetSlots() = %v; want 2 entries", got) + } +} + +func TestInvalidSourceXML_PlaceholdersFilteredOut(t *testing.T) { + // Second-shape reproducer: three INVALID_SOURCE placeholders + // preceding three real presets. Before the IsEmpty extension, + // listings printed "0. Preset 0 / Source: INVALID_SOURCE" three + // times before the real entries — annoying, not crashing. + var presets Presets + + if err := xml.Unmarshal([]byte(invalidSourceXML), &presets); err != nil { + t.Fatalf("Failed to unmarshal invalid-source XML: %v", err) + } + + if got := len(presets.Preset); got != 6 { + t.Fatalf("Expected 6 preset entries, got %d", got) + } + + configured := 0 + + for _, p := range presets.Preset { + if !p.IsEmpty() { + configured++ + } + } + + if configured != 3 { + t.Errorf("Expected 3 configured presets (after filtering INVALID_SOURCE placeholders), got %d", + configured) + } + + // The three placeholders all carry id="0", so used-slot + // reporting should ignore them and show only the real ids. + used := presets.GetUsedPresetSlots() + if len(used) != 3 { + t.Fatalf("GetUsedPresetSlots() = %v; want 3 entries", used) + } + + wantIDs := map[int]bool{1: true, 2: true, 6: true} + for _, id := range used { + if !wantIDs[id] { + t.Errorf("Unexpected used slot id %d; want one of %v", id, []int{1, 2, 6}) + } + } +} diff --git a/pkg/service/setup/setup.go b/pkg/service/setup/setup.go index 8cd00be..7d799b2 100644 --- a/pkg/service/setup/setup.go +++ b/pkg/service/setup/setup.go @@ -2588,7 +2588,12 @@ func (m *Manager) syncPresets(deviceIP, accountID, deviceID string) { var servicePresets []models.ServicePreset for _, p := range ps.Preset { - if p.ContentItem == nil { + // IsEmpty catches both placeholder shapes a SoundTouch device + // can emit: self-closing (issue #308) and + // . Neither carries + // real playable data and persisting them would surface as + // junk entries in the admin web UI. + if p.IsEmpty() { continue }