fix(service): address code-review findings on PR #652's startup retry

Fixes correctness issues found reviewing the bounded device-seed retry
loop before merging: a datastore read failure could make the readiness
check trivially pass; stale-host pruning only considered hosts inserted
in the current attempt and only ran inside the retry loop, not the
plain SeedExtraDevices path; the retry loop and a devices-changed-hook
seed could probe the same offline host concurrently; and a zero-change
startup window silently dropped the previously-unconditional device-list
broadcast. Also makes the retry interval/window configurable instead of
hardcoded, following the existing discovery-interval flag pattern.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

pkg/service/soundtouchweb/discovery.go

@@ -123,7 +123,8 @@ func (app *WebApp) addDeviceByHost(host string, port int, source string) *webtyp
 }
 
 // SeedExtraDevices registers any devices reported by the ExtraDeviceHosts hook
-// (if set) via AddDeviceByHost. Idempotent: already-known hosts are skipped.
+// (if set) via AddDeviceByHost, and prunes any previously-seeded host that no
+// longer appears in that set. Idempotent: already-known hosts are skipped.
 // Used by the embedded build to surface the service datastore's devices even
 // when network discovery is disabled; a no-op for standalone soundtouch-player.
 //
@@ -132,8 +133,14 @@ func (app *WebApp) addDeviceByHost(host string, port int, source string) *webtyp
 // datastore would otherwise stall the whole seed for 10 s, serially. Fanning
 // out bounds the cost to roughly a single timeout regardless of how many
 // devices are offline. AddDeviceByHost is registry-safe under concurrency.
+//
+// A hook read failure is logged and otherwise swallowed here; callers that
+// need to distinguish "read failed" from "converged" (the bounded startup
+// retry) should call seedExtraDevices directly instead.
 func (app *WebApp) SeedExtraDevices() {
-	app.seedExtraDevices()
+	if _, _, _, err := app.seedExtraDevices(); err != nil {
+		log.Printf("SeedExtraDevices: failed to read extra device hosts: %v", err)
+	}
 }
 
 type seededExtraDevice struct {
@@ -141,17 +148,40 @@ type seededExtraDevice struct {
 	conn *webtypes.DeviceConnection
 }
 
-func (app *WebApp) seedExtraDevices() []seededExtraDevice {
+// seedExtraDevices probes any ExtraDeviceHosts hosts that aren't already
+// registered, and prunes any registered host that's no longer in the current
+// desired set. Pruning is safe to apply to the whole registry (not just hosts
+// this call inserted) because ExtraDeviceHosts is the only inserter into this
+// registry for the embedded build: discoveryService is nil there, so the
+// mDNS/UPnP insertion path in DiscoverDevices is never reached.
+//
+// Runs are serialized via seedMu so the bounded startup retry loop
+// (SeedExtraDevicesUntilReady) and a devices-changed-hook-triggered
+// SeedExtraDevices call never issue concurrent probes to the same offline
+// host.
+//
+// A non-nil error means the hook itself failed (e.g. a datastore glitch);
+// callers must treat that as "unknown state, don't prune, don't declare
+// ready" rather than as an empty desired set.
+func (app *WebApp) seedExtraDevices() (inserted []seededExtraDevice, removed int, desired map[string]struct{}, err error) {
 	if app.ExtraDeviceHosts == nil {
-		return nil
+		return nil, 0, nil, nil
 	}
 
-	hosts := app.extraDeviceHosts()
-	inserted := make(chan seededExtraDevice, len(hosts))
+	app.seedMu.Lock()
+	defer app.seedMu.Unlock()
 
-	var wg sync.WaitGroup
+	desired, err = app.extraDeviceHostSet()
+	if err != nil {
+		return nil, 0, nil, err
+	}
 
-	for _, host := range hosts {
+	var (
+		mu sync.Mutex
+		wg sync.WaitGroup
+	)
+
+	for host := range desired {
 		if _, ok := app.GetDevice(host); ok {
 			continue
 		}
@@ -161,21 +191,32 @@ func (app *WebApp) seedExtraDevices() []seededExtraDevice {
 		go func(h string) {
 			defer wg.Done()
 
-			if conn := app.addDeviceByHost(h, 8090, "service-store"); conn != nil {
-				inserted <- seededExtraDevice{host: h, conn: conn}
+			conn := app.addDeviceByHost(h, 8090, "service-store")
+			if conn == nil {
+				return
 			}
+
+			mu.Lock()
+
+			inserted = append(inserted, seededExtraDevice{host: h, conn: conn})
+
+			mu.Unlock()
 		}(host)
 	}
 
 	wg.Wait()
-	close(inserted)
 
-	added := make([]seededExtraDevice, 0, len(inserted))
-	for device := range inserted {
-		added = append(added, device)
+	for _, entry := range app.DeviceSnapshot() {
+		if _, ok := desired[entry.ID]; ok {
+			continue
+		}
+
+		if app.removeDeviceIfMatch(entry.ID, entry.Device) {
+			removed++
+		}
 	}
 
-	return added
+	return inserted, removed, desired, nil
 }
 
 // SeedExtraDevicesUntilReady retries only the hosts returned by
@@ -185,32 +226,34 @@ func (app *WebApp) seedExtraDevices() []seededExtraDevice {
 // while the service is starting.
 func (app *WebApp) SeedExtraDevicesUntilReady(ctx context.Context, retryInterval time.Duration) {
 	retryUntilReady(ctx, retryInterval, func() bool {
-		inserted := app.seedExtraDevices()
-		desired := app.extraDeviceHostSet()
-
-		for _, device := range inserted {
-			if _, ok := desired[device.host]; !ok {
-				app.removeDeviceIfMatch(device.host, device.conn)
-			}
+		inserted, removed, desired, err := app.seedExtraDevices()
+		if err != nil {
+			log.Printf("SeedExtraDevicesUntilReady: failed to read extra device hosts, will retry: %v", err)
+			return false
 		}
 
-		if len(inserted) > 0 {
+		if len(inserted) > 0 || removed > 0 {
 			app.BroadcastDeviceList()
 		}
 
-		return app.extraDeviceHostsPresent(app.extraDeviceHostSet())
+		return app.extraDeviceHostsPresent(desired)
 	})
 }
 
-func (app *WebApp) extraDeviceHosts() []string {
+func (app *WebApp) extraDeviceHosts() ([]string, error) {
 	if app.ExtraDeviceHosts == nil {
-		return nil
+		return nil, nil
 	}
 
-	hosts := make([]string, 0)
-	seen := make(map[string]struct{})
+	rawHosts, err := app.ExtraDeviceHosts()
+	if err != nil {
+		return nil, err
+	}
 
-	for _, host := range app.ExtraDeviceHosts() {
+	hosts := make([]string, 0, len(rawHosts))
+	seen := make(map[string]struct{}, len(rawHosts))
+
+	for _, host := range rawHosts {
 		if host == "" {
 			continue
 		}
@@ -223,18 +266,22 @@ func (app *WebApp) extraDeviceHosts() []string {
 		hosts = append(hosts, host)
 	}
 
-	return hosts
+	return hosts, nil
 }
 
-func (app *WebApp) extraDeviceHostSet() map[string]struct{} {
-	hosts := app.extraDeviceHosts()
+func (app *WebApp) extraDeviceHostSet() (map[string]struct{}, error) {
+	hosts, err := app.extraDeviceHosts()
+	if err != nil {
+		return nil, err
+	}
+
 	desired := make(map[string]struct{}, len(hosts))
 
 	for _, host := range hosts {
 		desired[host] = struct{}{}
 	}
 
-	return desired
+	return desired, nil
 }
 
 func (app *WebApp) extraDeviceHostsPresent(desired map[string]struct{}) bool {

pkg/service/soundtouchweb/discovery_test.go

@@ -2,6 +2,7 @@ package soundtouchweb
 
 import (
 	"context"
+	"errors"
 	"net/http"
 	"net/http/httptest"
 	"strings"
@@ -131,9 +132,12 @@ func TestRetryUntilReadyStopsAfterContextCancellation(t *testing.T) {
 
 func TestExtraDeviceHostsPresent(t *testing.T) {
 	app := NewWebApp()
-	app.ExtraDeviceHosts = func() []string { return []string{"known", "", "known", "missing"} }
+	app.ExtraDeviceHosts = func() ([]string, error) { return []string{"known", "", "known", "missing"}, nil }
 	app.AddDevice("known", &webtypes.DeviceConnection{})
-	desired := app.extraDeviceHostSet()
+	desired, err := app.extraDeviceHostSet()
+	if err != nil {
+		t.Fatalf("extraDeviceHostSet() error = %v", err)
+	}
 
 	if len(desired) != 2 {
 		t.Fatalf("desired host count = %d, want 2", len(desired))
@@ -148,9 +152,19 @@ func TestExtraDeviceHostsPresent(t *testing.T) {
 	}
 }
 
+func TestExtraDeviceHostSetPropagatesHookError(t *testing.T) {
+	app := NewWebApp()
+	wantErr := errors.New("datastore glitch")
+	app.ExtraDeviceHosts = func() ([]string, error) { return nil, wantErr }
+
+	if _, err := app.extraDeviceHostSet(); !errors.Is(err, wantErr) {
+		t.Fatalf("extraDeviceHostSet() error = %v, want %v", err, wantErr)
+	}
+}
+
 func TestSeedExtraDevicesSkipsKnownHosts(t *testing.T) {
 	app := NewWebApp()
-	app.ExtraDeviceHosts = func() []string { return []string{"known"} }
+	app.ExtraDeviceHosts = func() ([]string, error) { return []string{"known"}, nil }
 	lastSeen := time.Unix(123, 0)
 	conn := &webtypes.DeviceConnection{LastSeen: lastSeen}
 	app.AddDevice("known", conn)
@@ -162,6 +176,117 @@ func TestSeedExtraDevicesSkipsKnownHosts(t *testing.T) {
 	}
 }
 
+// TestSeedExtraDevicesUntilReadyRetriesOnHookError covers the code-review
+// finding that a hook error (e.g. a transient datastore read failure) must
+// not be treated as "zero hosts persisted", which would make the readiness
+// check trivially pass and end the retry window immediately.
+func TestSeedExtraDevicesUntilReadyRetriesOnHookError(t *testing.T) {
+	var calls atomic.Int32
+
+	app := NewWebApp()
+	app.ExtraDeviceHosts = func() ([]string, error) {
+		n := calls.Add(1)
+		if n < 3 {
+			return nil, errors.New("datastore glitch")
+		}
+
+		return nil, nil
+	}
+
+	ctx, cancel := context.WithTimeout(context.Background(), time.Second)
+	defer cancel()
+
+	app.SeedExtraDevicesUntilReady(ctx, time.Millisecond)
+
+	if got := calls.Load(); got < 3 {
+		t.Fatalf("hook call count = %d, want at least 3 (kept retrying past the errors)", got)
+	}
+}
+
+// TestSeedExtraDevicesPrunesHostRemovedAfterEarlierAttempt covers the
+// code-review finding that pruning must consider the whole registry, not
+// only hosts inserted during the current call: a host registered by an
+// earlier seed call that later falls out of ExtraDeviceHosts must still be
+// pruned by a later call, and this must hold for the plain SeedExtraDevices
+// path too, not just the bounded retry loop.
+func TestSeedExtraDevicesPrunesHostRemovedAfterEarlierAttempt(t *testing.T) {
+	server := httptest.NewTestServer(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		w.Header().Set("Content-Type", "application/xml")
+		_, _ = w.Write([]byte(`<info deviceID="TESTDEVICE"><name>Stale speaker</name><type>SoundTouch 10</type></info>`))
+	}))
+	server.Start()
+
+	host := strings.TrimPrefix(server.URL, "http://")
+
+	var stillDesired atomic.Bool
+	stillDesired.Store(true)
+
+	app := NewWebApp()
+	app.ExtraDeviceHosts = func() ([]string, error) {
+		if stillDesired.Load() {
+			return []string{host}, nil
+		}
+
+		return nil, nil
+	}
+
+	// First call registers the host.
+	app.SeedExtraDevices()
+	if _, ok := app.GetDevice(host); !ok {
+		t.Fatalf("host %s was not registered on the first seed call", host)
+	}
+
+	// Simulate the device being removed from the datastore in between calls.
+	stillDesired.Store(false)
+
+	// A later plain SeedExtraDevices call (as triggered by
+	// SetDevicesChangedHook or the manual /api/control/discover route) must
+	// still prune it, not just the bounded retry loop.
+	app.SeedExtraDevices()
+	if _, ok := app.GetDevice(host); ok {
+		t.Fatal("stale host was not pruned by a later SeedExtraDevices call")
+	}
+}
+
+// TestSeedExtraDevicesSerializesConcurrentRuns covers the code-review finding
+// that the bounded startup retry loop and a devices-changed-hook-triggered
+// SeedExtraDevices call must not issue concurrent probes to the same
+// still-offline host.
+func TestSeedExtraDevicesSerializesConcurrentRuns(t *testing.T) {
+	var infoRequests atomic.Int32
+	release := make(chan struct{})
+
+	server := httptest.NewTestServer(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		infoRequests.Add(1)
+		<-release // block until the test lets the handler respond
+
+		w.Header().Set("Content-Type", "application/xml")
+		_, _ = w.Write([]byte(`<info deviceID="TESTDEVICE"><name>Slow speaker</name><type>SoundTouch 10</type></info>`))
+	}))
+	server.Start()
+
+	host := strings.TrimPrefix(server.URL, "http://")
+
+	app := NewWebApp()
+	app.ExtraDeviceHosts = func() ([]string, error) { return []string{host}, nil }
+
+	done := make(chan struct{}, 2)
+	go func() { app.SeedExtraDevices(); done <- struct{}{} }()
+	go func() { app.SeedExtraDevices(); done <- struct{}{} }()
+
+	// Give both goroutines a moment to reach the handler if they were going
+	// to run concurrently, then let the handler(s) respond.
+	time.Sleep(50 * time.Millisecond)
+	close(release)
+
+	<-done
+	<-done
+
+	if got := infoRequests.Load(); got != 1 {
+		t.Fatalf("/info request count = %d, want 1 (concurrent seeds were not serialized)", got)
+	}
+}
+
 func TestRemoveDeviceIfMatchKeepsReplacement(t *testing.T) {
 	app := NewWebApp()
 	original := webtypes.NewDeviceConnection(nil, &models.DeviceInfo{})

pkg/service/soundtouchweb/handler.go

@@ -62,7 +62,12 @@ type WebApp struct {
 	// soundtouch-service points it at the service datastore's known devices so
 	// the UI shows manually-added speakers even when network discovery is
 	// disabled. Standalone soundtouch-player leaves it nil.
-	ExtraDeviceHosts func() []string
+	//
+	// A non-nil error means the underlying read failed (e.g. a datastore
+	// glitch), which callers must NOT treat the same as "zero hosts
+	// persisted" -- doing so would make a transient read failure look like
+	// every persisted device is already registered.
+	ExtraDeviceHosts func() ([]string, error)
 
 	// TriggerDiscovery, when set, runs an external discovery sweep instead of
 	// this app's own mDNS/UPnP. The embedded build wires it to the host
@@ -78,6 +83,11 @@ type WebApp struct {
 	// removal only prunes the in-memory registry).
 	RemoveDeviceHook func(deviceID string) error
 
+	// seedMu serializes seedExtraDevices runs so the bounded startup retry
+	// loop (SeedExtraDevicesUntilReady) and a devices-changed-hook-triggered
+	// SeedExtraDevices never probe the same still-offline host concurrently.
+	seedMu sync.Mutex
+
 	discoveryStatus atomic.Value // stores *webtypes.DiscoveryStatus
 }