From 720d2abc5c87cacab974f2bcf068ba5c4ff0b893 Mon Sep 17 00:00:00 2001 From: Tobias Gesellchen Date: Sat, 27 Jun 2026 20:16:10 +0200 Subject: [PATCH] fix(zone): remove a member via /removeZoneSlave instead of a /setZone rebuild (refs #511) Removing one member from a multi-member zone did nothing. The remove paths rebuilt the zone with /setZone and the remaining members, but /setZone is additive: it never drops a member that is simply absent from the list. It only "removed" when the resulting set was empty (equivalent to dissolve), which is why removing the last member worked but removing one of several did not. Switch all three remove paths to the dedicated /removeZoneSlave endpoint (already implemented as client.RemoveZoneSlave): - HandleZoneRemove (web UI "remove member") - HandleZoneLeave (web UI slave "leave zone") - RemoveFromZone (client lib, used by CLI `zone remove`) DissolveZone (setZone master-only) and HandleZoneAdd (additive setZone) are correct and unchanged. Adds handler regression tests for remove/leave and rewrites TestClient_RemoveFromZone to assert /removeZoneSlave (the old test removed one of two members but only checked that setZone was called, never that the member was dropped). Co-Authored-By: Claude Opus 4.8 --- pkg/client/client.go | 26 ++++- pkg/client/zone_test.go | 36 +++++-- pkg/service/soundtouchweb/handler.go | 38 ++++--- pkg/service/soundtouchweb/handler_test.go | 124 ++++++++++++++++++++++ 4 files changed, 193 insertions(+), 31 deletions(-) diff --git a/pkg/client/client.go b/pkg/client/client.go index 0431949..584485a 100644 --- a/pkg/client/client.go +++ b/pkg/client/client.go @@ -1338,7 +1338,13 @@ func (c *Client) AddToZone(deviceID, ipAddress string) error { return c.SetZone(zoneRequest) } -// RemoveFromZone removes a device from the current zone +// RemoveFromZone removes a device from the current zone. +// +// It uses the dedicated /removeZoneSlave endpoint rather than rebuilding the +// zone with /setZone and the remaining members: /setZone does not drop a member +// from a multi-member zone (the speaker only goes standalone when the resulting +// member set is empty), so a setZone rebuild silently fails to remove one of +// several members. See #511. func (c *Client) RemoveFromZone(deviceID string) error { // Get current zone configuration currentZone, err := c.GetZone() @@ -1346,11 +1352,21 @@ func (c *Client) RemoveFromZone(deviceID string) error { return fmt.Errorf("failed to get current zone: %w", err) } - // Convert to zone request and remove member - zoneRequest := currentZone.ToZoneRequest() - zoneRequest.RemoveMember(deviceID) + if currentZone.IsStandalone() { + return nil // nothing to remove + } - return c.SetZone(zoneRequest) + // Carry the member's IP (as the speaker expects) when we know it. + slaveIP := "" + + for i := range currentZone.Members { + if currentZone.Members[i].DeviceID == deviceID { + slaveIP = currentZone.Members[i].IP + break + } + } + + return c.RemoveZoneSlave(currentZone.Master, deviceID, slaveIP) } // DissolveZone dissolves the current zone, making all devices standalone diff --git a/pkg/client/zone_test.go b/pkg/client/zone_test.go index 4806fb9..fa72989 100644 --- a/pkg/client/zone_test.go +++ b/pkg/client/zone_test.go @@ -1,6 +1,7 @@ package client import ( + "io" "net/http" "net/http/httptest" "strings" @@ -295,12 +296,15 @@ func TestClient_RemoveFromZone(t *testing.T) { getZoneCalled := false setZoneCalled := false + var removeBody string + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.Header().Set("Content-Type", "application/xml") - if r.URL.Path == "/getZone" && r.Method == http.MethodGet { + switch { + case r.URL.Path == "/getZone" && r.Method == http.MethodGet: getZoneCalled = true - // Return existing zone with members + // Return existing zone with two members. response := ` EFGH5678IJKL @@ -309,11 +313,16 @@ func TestClient_RemoveFromZone(t *testing.T) { w.WriteHeader(http.StatusOK) _, _ = w.Write([]byte(response)) - } else if r.URL.Path == "/setZone" && r.Method == http.MethodPost { + case r.URL.Path == "/removeZoneSlave" && r.Method == http.MethodPost: + b, _ := io.ReadAll(r.Body) + removeBody = string(b) + + w.WriteHeader(http.StatusOK) + case r.URL.Path == "/setZone" && r.Method == http.MethodPost: setZoneCalled = true w.WriteHeader(http.StatusOK) - } else { + default: w.WriteHeader(http.StatusNotFound) } })) @@ -321,6 +330,9 @@ func TestClient_RemoveFromZone(t *testing.T) { client := createTestClient(server.URL) + // Removing one of two members must target that member via /removeZoneSlave, + // not rebuild the zone via /setZone (which does not drop a member from a + // multi-member zone). Regression for #511. err := client.RemoveFromZone("EFGH5678IJKL") if err != nil { t.Errorf("Expected no error, but got: %v", err) @@ -330,8 +342,20 @@ func TestClient_RemoveFromZone(t *testing.T) { t.Error("Expected GetZone to be called") } - if !setZoneCalled { - t.Error("Expected SetZone to be called") + if setZoneCalled { + t.Error("RemoveFromZone must not use /setZone to drop a member from a multi-member zone") + } + + if !strings.Contains(removeBody, "EFGH5678IJKL") { + t.Errorf("removeZoneSlave body should target the member, got: %s", removeBody) + } + + if !strings.Contains(removeBody, `master="ABCD1234EFGH"`) { + t.Errorf("removeZoneSlave body should name the master, got: %s", removeBody) + } + + if !strings.Contains(removeBody, `ipaddress="192.0.2.11"`) { + t.Errorf("removeZoneSlave body should carry the member IP from the zone, got: %s", removeBody) } } diff --git a/pkg/service/soundtouchweb/handler.go b/pkg/service/soundtouchweb/handler.go index 17f7896..0868f0b 100644 --- a/pkg/service/soundtouchweb/handler.go +++ b/pkg/service/soundtouchweb/handler.go @@ -998,22 +998,22 @@ func (app *WebApp) HandleZoneRemove(w http.ResponseWriter, r *http.Request) { return } - if masterConn.Client == nil || slaveConn.DeviceInfo == nil { + if masterConn.Client == nil || masterConn.DeviceInfo == nil || slaveConn.DeviceInfo == nil { app.sendError(w, "Device not ready", http.StatusInternalServerError) return } - zone, err := masterConn.Client.GetZone() - if err != nil { - app.sendError(w, err.Error(), http.StatusInternalServerError) - return - } - - zoneReq := zone.ToZoneRequest() - zoneReq.RemoveMember(slaveConn.DeviceInfo.DeviceID) + masterHwID := masterConn.DeviceInfo.DeviceID + slaveHwID := slaveConn.DeviceInfo.DeviceID + // Remove a single member with the dedicated /removeZoneSlave endpoint. + // Rebuilding the zone via /setZone with the remaining members does not + // reliably drop a member when the zone has more than one: the speaker only + // goes standalone when the resulting member set is empty, so removing one of + // several members appeared to do nothing (#511). /removeZoneSlave targets the + // specific member. w.Header().Set("Content-Type", "application/json") - app.sendControlResponse(w, masterConn.Client.SetZone(zoneReq), "Device removed from zone") + app.sendControlResponse(w, masterConn.Client.RemoveZoneSlave(masterHwID, slaveHwID, slaveIP), "Device removed from zone") } // HandleZoneDissolve dissolves the zone, making all devices standalone. @@ -1073,17 +1073,15 @@ func (app *WebApp) HandleZoneLeave(w http.ResponseWriter, r *http.Request) { return } - masterZone, err := masterConn.Client.GetZone() - if err != nil { - app.sendError(w, err.Error(), http.StatusInternalServerError) - return - } - - zoneReq := masterZone.ToZoneRequest() - zoneReq.RemoveMember(slaveConn.DeviceInfo.DeviceID) - + // Drop this slave with the dedicated /removeZoneSlave endpoint sent to the + // master. Rebuilding the zone via /setZone with the remaining members does + // not drop a member from a multi-member zone (the master only goes standalone + // when the resulting set is empty), so leaving a 3+ device zone did nothing + // (#511). zone.Master is the master's hwID. w.Header().Set("Content-Type", "application/json") - app.sendControlResponse(w, masterConn.Client.SetZone(zoneReq), "Left zone") + app.sendControlResponse(w, + masterConn.Client.RemoveZoneSlave(zone.Master, slaveConn.DeviceInfo.DeviceID, slaveIP), + "Left zone") } // HandleDeviceRecents returns recently played items for a device. diff --git a/pkg/service/soundtouchweb/handler_test.go b/pkg/service/soundtouchweb/handler_test.go index 35e4771..a556ead 100644 --- a/pkg/service/soundtouchweb/handler_test.go +++ b/pkg/service/soundtouchweb/handler_test.go @@ -769,3 +769,127 @@ func TestHandleSourceControl_ForwardsAccount(t *testing.T) { }) } } + +// TestHandleZoneRemove_UsesRemoveZoneSlave is the #511 regression: removing one +// member from a multi-member zone must target that member via /removeZoneSlave. +// The previous implementation rebuilt the zone with /setZone and the remaining +// members, which the speaker only honoured when the resulting member set was +// empty — so removing one of several members appeared to do nothing, while +// removing the last member (empty set == dissolve) worked. +func TestHandleZoneRemove_UsesRemoveZoneSlave(t *testing.T) { + var gotPath, gotBody string + + speaker := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + gotPath = r.URL.Path + b, _ := io.ReadAll(r.Body) + gotBody = string(b) + w.WriteHeader(http.StatusOK) + })) + defer speaker.Close() + + app := NewWebApp() + + // Registry is IP-keyed; use RFC-5737 documentation addresses. + master := webtypes.NewDeviceConnection( + client.NewClient(&client.Config{Host: speaker.URL}), + &models.DeviceInfo{Name: "Master", DeviceID: "MASTERHW01"}, + ) + master.SetStatus(&webtypes.DeviceStatus{IsConnected: true, LastActivity: time.Now()}) + app.AddDevice("192.0.2.10", master) + + slave := webtypes.NewDeviceConnection(nil, &models.DeviceInfo{Name: "Slave", DeviceID: "SLAVEHW02"}) + app.AddDevice("192.0.2.20", slave) + + req := httptest.NewRequest("POST", "/api/control/devices/192.0.2.10/zone/remove/192.0.2.20", nil) + req = withChiParams(req, map[string]string{"id": "192.0.2.10", "slaveId": "192.0.2.20"}) + w := httptest.NewRecorder() + + app.HandleZoneRemove(w, req) + + if w.Code != http.StatusOK { + t.Fatalf("expected 200, got %d: %s", w.Code, w.Body.String()) + } + + if gotPath != "/removeZoneSlave" { + t.Errorf("expected POST to /removeZoneSlave, got %q (a /setZone rebuild does not drop a member from a multi-member zone)", gotPath) + } + + if !strings.Contains(gotBody, "SLAVEHW02") { + t.Errorf("removeZoneSlave body should target the slave device ID, got: %s", gotBody) + } + + if !strings.Contains(gotBody, `master="MASTERHW01"`) { + t.Errorf("removeZoneSlave body should name the master, got: %s", gotBody) + } +} + +// TestHandleZoneLeave_UsesRemoveZoneSlave is the #511 regression for the slave's +// "Leave zone" path: it must drop the slave via /removeZoneSlave on the master, +// not rebuild the master's zone with /setZone (which leaves a 3+ device zone +// unchanged). The leaving slave's "id" is its IP; it carries the master's hwID +// in its /getZone, which we resolve to the master's registry entry. +func TestHandleZoneLeave_UsesRemoveZoneSlave(t *testing.T) { + var masterPath, masterBody string + + // Master speaker captures the /removeZoneSlave call. + masterSpeaker := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + masterPath = r.URL.Path + b, _ := io.ReadAll(r.Body) + masterBody = string(b) + w.WriteHeader(http.StatusOK) + })) + defer masterSpeaker.Close() + + // Slave speaker answers /getZone naming the master by hwID. + slaveSpeaker := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/getZone" { + w.Header().Set("Content-Type", "application/xml") + _, _ = w.Write([]byte(` + + SLAVEHW02 + SLAVEHW03 +`)) + return + } + w.WriteHeader(http.StatusOK) + })) + defer slaveSpeaker.Close() + + app := NewWebApp() + + master := webtypes.NewDeviceConnection( + client.NewClient(&client.Config{Host: masterSpeaker.URL}), + &models.DeviceInfo{Name: "Master", DeviceID: "MASTERHW01"}, + ) + master.SetStatus(&webtypes.DeviceStatus{IsConnected: true, LastActivity: time.Now()}) + app.AddDevice("192.0.2.10", master) + + slave := webtypes.NewDeviceConnection( + client.NewClient(&client.Config{Host: slaveSpeaker.URL}), + &models.DeviceInfo{Name: "Slave", DeviceID: "SLAVEHW02"}, + ) + slave.SetStatus(&webtypes.DeviceStatus{IsConnected: true, LastActivity: time.Now()}) + app.AddDevice("192.0.2.20", slave) + + req := httptest.NewRequest("POST", "/api/control/devices/192.0.2.20/zone/leave", nil) + req = withChiParams(req, map[string]string{"id": "192.0.2.20"}) + w := httptest.NewRecorder() + + app.HandleZoneLeave(w, req) + + if w.Code != http.StatusOK { + t.Fatalf("expected 200, got %d: %s", w.Code, w.Body.String()) + } + + if masterPath != "/removeZoneSlave" { + t.Errorf("expected POST to master's /removeZoneSlave, got %q", masterPath) + } + + if !strings.Contains(masterBody, "SLAVEHW02") { + t.Errorf("removeZoneSlave body should target the leaving slave, got: %s", masterBody) + } + + if !strings.Contains(masterBody, `master="MASTERHW01"`) { + t.Errorf("removeZoneSlave body should name the master, got: %s", masterBody) + } +}