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) + } +}