From e7d1b44587ec82a93e7a96ab70484310d95f8616 Mon Sep 17 00:00:00 2001 From: Tobias Gesellchen Date: Sun, 17 May 2026 10:56:16 +0200 Subject: [PATCH] refactor(soundtouch-web): encapsulate WebApp device registry behind methods MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Devices map on WebApp was written from the startup goroutine, the /api/discover POST handler, and addDevice, while being read from every HTTP handler and the WebSocket periodic-update loop — all without any mutex. The Go runtime panics with "fatal error: concurrent map writes" or "concurrent map read and map write" on any actual collision, so this was a latent crash, not a tearing issue. Hide the map behind a sync.RWMutex and a small API: GetDevice(id) (*DeviceConnection, bool) DeviceSnapshot() []DeviceEntry DeviceCount() int AddDevice(id, conn) bool // atomic insert-or-touch TouchDevice(id) bool // fast-path LastSeen bump Update every caller — handlers, websocket, main, tests — to go through the API. addDevice's existing-host fast path uses TouchDevice; the final insert uses AddDevice so a race with another writer is rejected cleanly instead of silently overwriting. Add a TestRegistryConcurrent stress test that runs 64 goroutines doing 12,800 operations across writers, touchers, and two reader patterns. It exists to give `-race` (already on in CI) a concrete shape to catch if the encapsulation ever leaks back out. Struct-field races on conn.Status.* are not addressed by this change; they need their own follow-up. Co-Authored-By: Claude Opus 4.7 (1M context) --- cmd/soundtouch-web/handlers/handlers.go | 135 ++++++++++--- cmd/soundtouch-web/handlers/handlers_test.go | 14 +- cmd/soundtouch-web/handlers/registry_test.go | 194 +++++++++++++++++++ cmd/soundtouch-web/handlers/websocket.go | 24 +-- cmd/soundtouch-web/main.go | 21 +- cmd/soundtouch-web/spa_test.go | 2 +- 6 files changed, 339 insertions(+), 51 deletions(-) create mode 100644 cmd/soundtouch-web/handlers/registry_test.go diff --git a/cmd/soundtouch-web/handlers/handlers.go b/cmd/soundtouch-web/handlers/handlers.go index b58572f..d41d2f9 100644 --- a/cmd/soundtouch-web/handlers/handlers.go +++ b/cmd/soundtouch-web/handlers/handlers.go @@ -9,6 +9,7 @@ import ( "strconv" "strings" "sync" + "time" "github.com/gesellix/bose-soundtouch/cmd/soundtouch-web/webtypes" "github.com/gesellix/bose-soundtouch/pkg/models" @@ -17,18 +18,34 @@ import ( "github.com/gorilla/websocket" ) -// WebApp holds the application state and dependencies +// WebApp holds the application state and dependencies. +// +// The device registry (devices map + devicesMu) is encapsulated: +// callers go through GetDevice / DeviceSnapshot / AddDevice / +// TouchDevice / DeviceCount instead of touching the map directly. +// This prevents the concurrent-map-read/write panic that would +// otherwise be reachable any time an HTTP handler runs while +// discovery or the /api/discover endpoint is registering devices. type WebApp struct { - Devices map[string]*webtypes.DeviceConnection + devicesMu sync.RWMutex + devices map[string]*webtypes.DeviceConnection + Upgrader websocket.Upgrader WSClients map[*websocket.Conn]bool WSMutex sync.RWMutex } +// DeviceEntry pairs a device id with its connection. Used by +// DeviceSnapshot so callers can iterate without holding the lock. +type DeviceEntry struct { + ID string + Device *webtypes.DeviceConnection +} + // NewWebApp creates a new WebApp instance for SPA mode func NewWebApp() *WebApp { return &WebApp{ - Devices: make(map[string]*webtypes.DeviceConnection), + devices: make(map[string]*webtypes.DeviceConnection), WSClients: make(map[*websocket.Conn]bool), Upgrader: websocket.Upgrader{ CheckOrigin: func(_ *http.Request) bool { return true }, @@ -36,17 +53,89 @@ func NewWebApp() *WebApp { } } +// GetDevice returns the device for id and whether it exists. +func (app *WebApp) GetDevice(id string) (*webtypes.DeviceConnection, bool) { + app.devicesMu.RLock() + defer app.devicesMu.RUnlock() + + device, ok := app.devices[id] + + return device, ok +} + +// DeviceSnapshot returns a list of (id, *DeviceConnection) pairs taken +// under a single read lock. Callers can iterate the result without +// holding any registry lock. Devices added or removed after the call +// are not reflected; the pointers themselves remain valid because +// nothing deletes from the underlying map today. +func (app *WebApp) DeviceSnapshot() []DeviceEntry { + app.devicesMu.RLock() + defer app.devicesMu.RUnlock() + + out := make([]DeviceEntry, 0, len(app.devices)) + for id, device := range app.devices { + out = append(out, DeviceEntry{ID: id, Device: device}) + } + + return out +} + +// DeviceCount returns the number of registered devices at call time. +func (app *WebApp) DeviceCount() int { + app.devicesMu.RLock() + defer app.devicesMu.RUnlock() + + return len(app.devices) +} + +// AddDevice atomically registers conn under id when id is not already +// known. If id existed, its LastSeen is bumped and AddDevice returns +// false (the caller should discard conn). Returns true if conn was +// inserted. +func (app *WebApp) AddDevice(id string, conn *webtypes.DeviceConnection) bool { + app.devicesMu.Lock() + defer app.devicesMu.Unlock() + + if existing, ok := app.devices[id]; ok { + existing.LastSeen = time.Now() + return false + } + + app.devices[id] = conn + + return true +} + +// TouchDevice bumps LastSeen for id if it exists; returns true if +// found. Use this as a fast-path check before doing the network work +// needed to construct a new DeviceConnection. +func (app *WebApp) TouchDevice(id string) bool { + app.devicesMu.Lock() + defer app.devicesMu.Unlock() + + existing, ok := app.devices[id] + if !ok { + return false + } + + existing.LastSeen = time.Now() + + return true +} + // HandleAPIDevices returns all devices as JSON func (app *WebApp) HandleAPIDevices(w http.ResponseWriter, _ *http.Request) { w.Header().Set("Content-Type", "application/json") // Return all devices as JSON - devices := make(map[string]interface{}) - for id, device := range app.Devices { - devices[id] = map[string]interface{}{ - "info": device.DeviceInfo, - "status": device.Status, - "lastSeen": device.LastSeen, + snapshot := app.DeviceSnapshot() + devices := make(map[string]interface{}, len(snapshot)) + + for _, entry := range snapshot { + devices[entry.ID] = map[string]interface{}{ + "info": entry.Device.DeviceInfo, + "status": entry.Device.Status, + "lastSeen": entry.Device.LastSeen, } } @@ -68,7 +157,7 @@ func (app *WebApp) HandleAPIDevice(w http.ResponseWriter, r *http.Request) { return } - device, exists := app.Devices[deviceID] + device, exists := app.GetDevice(deviceID) if !exists { app.sendError(w, "Device not found", http.StatusNotFound) return @@ -107,7 +196,7 @@ func (app *WebApp) HandleAPIControl(w http.ResponseWriter, r *http.Request) { return } - device, exists := app.Devices[deviceID] + device, exists := app.GetDevice(deviceID) if !exists { app.sendError(w, "Device not found", http.StatusNotFound) return @@ -318,7 +407,7 @@ func (app *WebApp) HandleDeviceKey(w http.ResponseWriter, r *http.Request) { deviceID := chi.URLParam(r, "id") key := chi.URLParam(r, "key") - device, exists := app.Devices[deviceID] + device, exists := app.GetDevice(deviceID) if !exists { app.sendError(w, "Device not found", http.StatusNotFound) return @@ -350,7 +439,7 @@ func (app *WebApp) HandleDirectVolumeControl(w http.ResponseWriter, r *http.Requ return } - device, exists := app.Devices[deviceID] + device, exists := app.GetDevice(deviceID) if !exists { app.sendError(w, "Device not found", http.StatusNotFound) return @@ -376,7 +465,7 @@ func (app *WebApp) HandleDirectVolumeControl(w http.ResponseWriter, r *http.Requ func (app *WebApp) HandleDevicePower(w http.ResponseWriter, r *http.Request) { deviceID := chi.URLParam(r, "id") - device, exists := app.Devices[deviceID] + device, exists := app.GetDevice(deviceID) if !exists { app.sendError(w, "Device not found", http.StatusNotFound) return @@ -403,7 +492,7 @@ func (app *WebApp) HandleDevicePower(w http.ResponseWriter, r *http.Request) { func (app *WebApp) HandleDevicePowerStatus(w http.ResponseWriter, r *http.Request) { deviceID := chi.URLParam(r, "id") - device, exists := app.Devices[deviceID] + device, exists := app.GetDevice(deviceID) if !exists { app.sendError(w, "Device not found", http.StatusNotFound) return @@ -444,12 +533,14 @@ func (app *WebApp) BroadcastDeviceList() { app.WSMutex.RLock() defer app.WSMutex.RUnlock() - devices := make(map[string]interface{}) - for id, device := range app.Devices { - devices[id] = map[string]interface{}{ - "info": device.DeviceInfo, - "status": device.Status, - "lastSeen": device.LastSeen, + snapshot := app.DeviceSnapshot() + devices := make(map[string]interface{}, len(snapshot)) + + for _, entry := range snapshot { + devices[entry.ID] = map[string]interface{}{ + "info": entry.Device.DeviceInfo, + "status": entry.Device.Status, + "lastSeen": entry.Device.LastSeen, } } @@ -598,7 +689,7 @@ func (app *WebApp) HandlePlayTuneIn(w http.ResponseWriter, r *http.Request) { return } - device, exists := app.Devices[deviceID] + device, exists := app.GetDevice(deviceID) if !exists { app.sendError(w, fmt.Sprintf("Device '%s' not found", deviceID), http.StatusNotFound) return diff --git a/cmd/soundtouch-web/handlers/handlers_test.go b/cmd/soundtouch-web/handlers/handlers_test.go index cb4c5cf..b0c6fcb 100644 --- a/cmd/soundtouch-web/handlers/handlers_test.go +++ b/cmd/soundtouch-web/handlers/handlers_test.go @@ -40,7 +40,7 @@ func createTestApp() *WebApp { }, } - app.Devices["test-device"] = device + app.AddDevice("test-device", device) return app } @@ -59,13 +59,9 @@ func TestNewWebApp(t *testing.T) { if app == nil { t.Fatal("NewWebApp returned nil") } - if app.Devices == nil { - t.Fatal("Devices map not initialized") - } - // At this point we know app and app.Devices are not nil - if len(app.Devices) != 0 { - t.Errorf("Expected empty devices map, got %d devices", len(app.Devices)) + if count := app.DeviceCount(); count != 0 { + t.Errorf("Expected empty device registry, got %d devices", count) } } @@ -549,11 +545,11 @@ func BenchmarkHandleAPIDevices(b *testing.B) { // Add more devices for realistic benchmarking for i := 0; i < 10; i++ { deviceID := "device-" + string(rune('0'+i)) - app.Devices[deviceID] = &webtypes.DeviceConnection{ + app.AddDevice(deviceID, &webtypes.DeviceConnection{ Client: &client.Client{}, DeviceInfo: &models.DeviceInfo{Name: "Test Device " + deviceID}, Status: webtypes.DeviceStatus{IsConnected: true}, - } + }) } req := httptest.NewRequest("GET", "/api/devices", nil) diff --git a/cmd/soundtouch-web/handlers/registry_test.go b/cmd/soundtouch-web/handlers/registry_test.go new file mode 100644 index 0000000..20d87b4 --- /dev/null +++ b/cmd/soundtouch-web/handlers/registry_test.go @@ -0,0 +1,194 @@ +// Package handlers contains tests for the device registry API on +// WebApp (GetDevice, AddDevice, TouchDevice, DeviceSnapshot, +// DeviceCount). +package handlers + +import ( + "fmt" + "sync" + "testing" + + "github.com/gesellix/bose-soundtouch/cmd/soundtouch-web/webtypes" + "github.com/gesellix/bose-soundtouch/pkg/models" +) + +func newRegistryDevice(name string) *webtypes.DeviceConnection { + return &webtypes.DeviceConnection{ + DeviceInfo: &models.DeviceInfo{Name: name}, + Status: webtypes.DeviceStatus{IsConnected: true}, + } +} + +func TestAddDevice_Inserts(t *testing.T) { + app := NewWebApp() + conn := newRegistryDevice("first") + + if !app.AddDevice("host-1", conn) { + t.Fatal("AddDevice returned false on first insert") + } + + got, ok := app.GetDevice("host-1") + if !ok { + t.Fatal("GetDevice did not find the device after AddDevice") + } + + if got != conn { + t.Errorf("GetDevice returned a different pointer than inserted") + } +} + +func TestAddDevice_RejectsDuplicateAndBumpsLastSeen(t *testing.T) { + app := NewWebApp() + original := newRegistryDevice("first") + app.AddDevice("host-1", original) + + originalSeen := original.LastSeen + replacement := newRegistryDevice("second") + + if app.AddDevice("host-1", replacement) { + t.Fatal("AddDevice returned true on duplicate; expected false") + } + + got, _ := app.GetDevice("host-1") + if got != original { + t.Error("Duplicate AddDevice replaced the existing device pointer") + } + + if !got.LastSeen.After(originalSeen) { + t.Error("Duplicate AddDevice did not bump LastSeen on existing device") + } +} + +func TestTouchDevice(t *testing.T) { + app := NewWebApp() + + if app.TouchDevice("missing") { + t.Error("TouchDevice returned true for unknown id") + } + + conn := newRegistryDevice("first") + app.AddDevice("host-1", conn) + seenBefore := conn.LastSeen + + if !app.TouchDevice("host-1") { + t.Fatal("TouchDevice returned false for known id") + } + + if !conn.LastSeen.After(seenBefore) { + t.Error("TouchDevice did not bump LastSeen") + } +} + +func TestDeviceSnapshotAndCount(t *testing.T) { + app := NewWebApp() + + if got := app.DeviceCount(); got != 0 { + t.Errorf("DeviceCount on empty app = %d; want 0", got) + } + + if snap := app.DeviceSnapshot(); len(snap) != 0 { + t.Errorf("DeviceSnapshot on empty app = %v; want []", snap) + } + + for i := 0; i < 5; i++ { + app.AddDevice(fmt.Sprintf("host-%d", i), newRegistryDevice(fmt.Sprintf("n%d", i))) + } + + if got := app.DeviceCount(); got != 5 { + t.Errorf("DeviceCount after 5 adds = %d; want 5", got) + } + + snap := app.DeviceSnapshot() + if len(snap) != 5 { + t.Errorf("DeviceSnapshot len = %d; want 5", len(snap)) + } + + // Spot-check that the snapshot ids match what we inserted. + seen := map[string]bool{} + for _, entry := range snap { + seen[entry.ID] = true + } + + for i := 0; i < 5; i++ { + id := fmt.Sprintf("host-%d", i) + if !seen[id] { + t.Errorf("DeviceSnapshot missing %s", id) + } + } +} + +// TestRegistryConcurrent exercises the registry from many goroutines +// at once. Before the introduction of devicesMu this would either +// panic with "fatal error: concurrent map read and map write" or be +// flagged by the race detector. The test runs under `go test -race` +// in CI so a future regression that re-exposes the underlying map +// without locking would be caught here. +func TestRegistryConcurrent(t *testing.T) { + app := NewWebApp() + + const workers = 16 + + const opsPerWorker = 200 + + var wg sync.WaitGroup + wg.Add(workers * 4) + + // Writers: insert distinct ids across workers. + for w := 0; w < workers; w++ { + go func(worker int) { + defer wg.Done() + + for i := 0; i < opsPerWorker; i++ { + id := fmt.Sprintf("w%d-%d", worker, i) + app.AddDevice(id, newRegistryDevice(id)) + } + }(w) + } + + // Touchers: bump LastSeen on a shared id (which may or may not + // exist yet — both branches are exercised). + for w := 0; w < workers; w++ { + go func() { + defer wg.Done() + + for i := 0; i < opsPerWorker; i++ { + app.TouchDevice("shared") + } + }() + } + + // Readers via snapshot. + for w := 0; w < workers; w++ { + go func() { + defer wg.Done() + + for i := 0; i < opsPerWorker; i++ { + _ = app.DeviceSnapshot() + } + }() + } + + // Readers via direct lookup. + for w := 0; w < workers; w++ { + go func() { + defer wg.Done() + + for i := 0; i < opsPerWorker; i++ { + _, _ = app.GetDevice("shared") + _ = app.DeviceCount() + } + }() + } + + wg.Wait() + + // Sanity check: every writer inserted opsPerWorker devices, plus + // the "shared" entry was never AddDevice'd so should be absent. + if got, want := app.DeviceCount(), workers*opsPerWorker; got != want { + t.Errorf("DeviceCount after concurrent inserts = %d; want %d", got, want) + } + + if _, ok := app.GetDevice("shared"); ok { + t.Error("shared device should not exist (only TouchDevice was called for it)") + } +} diff --git a/cmd/soundtouch-web/handlers/websocket.go b/cmd/soundtouch-web/handlers/websocket.go index e42b190..b03ca3e 100644 --- a/cmd/soundtouch-web/handlers/websocket.go +++ b/cmd/soundtouch-web/handlers/websocket.go @@ -35,12 +35,14 @@ func (app *WebApp) HandleWebSocket(w http.ResponseWriter, r *http.Request) { app.WSMutex.Unlock() // Send initial device list - devices := make(map[string]interface{}) - for id, device := range app.Devices { - devices[id] = map[string]interface{}{ - "info": device.DeviceInfo, - "status": device.Status, - "lastSeen": device.LastSeen, + snapshot := app.DeviceSnapshot() + devices := make(map[string]interface{}, len(snapshot)) + + for _, entry := range snapshot { + devices[entry.ID] = map[string]interface{}{ + "info": entry.Device.DeviceInfo, + "status": entry.Device.Status, + "lastSeen": entry.Device.LastSeen, } } @@ -88,12 +90,12 @@ func (app *WebApp) HandleWebSocket(w http.ResponseWriter, r *http.Request) { } // Send periodic status updates - for id, device := range app.Devices { - if device.Status.IsConnected { + for _, entry := range app.DeviceSnapshot() { + if entry.Device.Status.IsConnected { statusMessage := webtypes.WebSocketMessage{ Type: "status_update", - DeviceID: id, - Data: device.Status, + DeviceID: entry.ID, + Data: entry.Device.Status, } if err := conn.WriteJSON(statusMessage); err != nil { @@ -230,7 +232,7 @@ func (app *WebApp) HandleDeviceWebSocket(w http.ResponseWriter, r *http.Request) return } - device, exists := app.Devices[deviceID] + device, exists := app.GetDevice(deviceID) if !exists { http.Error(w, "Device not found", http.StatusNotFound) return diff --git a/cmd/soundtouch-web/main.go b/cmd/soundtouch-web/main.go index 72701c4..1606fac 100644 --- a/cmd/soundtouch-web/main.go +++ b/cmd/soundtouch-web/main.go @@ -103,7 +103,7 @@ func main() { ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) defer cancel() - webApp.BroadcastDiscoveryStatus("starting", len(webApp.Devices)) + webApp.BroadcastDiscoveryStatus("starting", webApp.DeviceCount()) for _, host := range manualHosts { addDevice(webApp, host, 8090, "manual") @@ -111,7 +111,7 @@ func main() { discoverDevices(ctx, webApp, discoveryService) - webApp.BroadcastDiscoveryStatus("completed", len(webApp.Devices)) + webApp.BroadcastDiscoveryStatus("completed", webApp.DeviceCount()) webApp.BroadcastDeviceList() }() @@ -217,8 +217,8 @@ func resolveBindAddr(bindAddr string) (string, error) { // mDNS/UPnP. If the host is already known, the existing entry's // LastSeen is bumped and the function returns without re-fetching. func addDevice(app *handlers.WebApp, host string, port int, source string) { - if existing, ok := app.Devices[host]; ok { - existing.LastSeen = time.Now() + // Fast path: skip the network call if we already know this host. + if app.TouchDevice(host) { return } @@ -243,7 +243,12 @@ func addDevice(app *handlers.WebApp, host string, port int, source string) { LastActivity: time.Now(), }, } - app.Devices[host] = conn + if !app.AddDevice(host, conn) { + // Lost a race — another goroutine inserted the same host + // between TouchDevice and AddDevice. AddDevice bumped LastSeen + // on the existing entry; discard our conn. + return + } go app.UpdateDeviceStatus(host, conn) @@ -280,12 +285,12 @@ func setupRoutes(app *handlers.WebApp, discoveryService *discovery.UnifiedDiscov defer cancel() // Broadcast discovery start - app.BroadcastDiscoveryStatus("starting", len(app.Devices)) + app.BroadcastDiscoveryStatus("starting", app.DeviceCount()) discoverDevices(ctx, app, discoveryService) // Broadcast discovery completion and updated device list - app.BroadcastDiscoveryStatus("completed", len(app.Devices)) + app.BroadcastDiscoveryStatus("completed", app.DeviceCount()) app.BroadcastDeviceList() }() }) @@ -321,7 +326,7 @@ func discoverDevices(ctx context.Context, app *handlers.WebApp, discoveryService devices, err := discoveryService.DiscoverDevices(ctx) if err != nil { log.Printf("Discovery failed: %v", err) - app.BroadcastDiscoveryStatus("failed", len(app.Devices)) + app.BroadcastDiscoveryStatus("failed", app.DeviceCount()) return } diff --git a/cmd/soundtouch-web/spa_test.go b/cmd/soundtouch-web/spa_test.go index 2231d3b..867d24d 100644 --- a/cmd/soundtouch-web/spa_test.go +++ b/cmd/soundtouch-web/spa_test.go @@ -256,7 +256,7 @@ func TestControlAPIValidation(t *testing.T) { LastSeen: time.Now(), Status: webtypes.DeviceStatus{IsConnected: true}, } - app.Devices["testdevice"] = mockDevice + app.AddDevice("testdevice", mockDevice) for _, tt := range tests { t.Run(tt.name, func(t *testing.T) {