diff --git a/cmd/soundtouch-cli/cmd_group.go b/cmd/soundtouch-cli/cmd_group.go index 82ae61a..3649819 100644 --- a/cmd/soundtouch-cli/cmd_group.go +++ b/cmd/soundtouch-cli/cmd_group.go @@ -3,6 +3,7 @@ package main import ( "fmt" "net" + "sync" "github.com/gesellix/bose-soundtouch/pkg/client" "github.com/gesellix/bose-soundtouch/pkg/models" @@ -37,7 +38,10 @@ func getGroupStatus(c *cli.Context) error { return nil } -// createGroup forms a stereo pair on the LEFT speaker, which becomes the master. +// createGroup forms a stereo pair by POSTing /addGroup to both speakers in +// parallel. LEFT is the master. Addressing each speaker directly (instead of +// only the master and letting it propagate via marge) sidesteps the +// inter-device round-trip that surfaced as client timeouts in #252. func createGroup(c *cli.Context) error { leftIP := c.String("left") rightIP := c.String("right") @@ -80,6 +84,7 @@ func createGroup(c *cli.Context) error { {DeviceID: rightInfo.DeviceID, Role: "RIGHT", IPAddress: rightIP}, }, }, + SenderIPAddress: leftIP, } leftClient, err := clientForHost(c, leftIP) @@ -88,18 +93,95 @@ func createGroup(c *cli.Context) error { return err } - result, err := leftClient.AddGroup(req) + rightClient, err := clientForHost(c, rightIP) if err != nil { - PrintError(fmt.Sprintf("Failed to create group: %v", err)) + PrintError(fmt.Sprintf("Failed to create client for RIGHT: %v", err)) return err } - PrintSuccess(fmt.Sprintf("Stereo pair created (id=%s)", result.ID)) - printGroup(result) + leftOut, rightOut := propagateAddGroup(leftClient, rightClient, leftIP, rightIP, req) + + if leftOut.err != nil { + PrintError(fmt.Sprintf("LEFT (%s) /addGroup failed: %v", leftIP, leftOut.err)) + } + + if rightOut.err != nil { + PrintError(fmt.Sprintf("RIGHT (%s) /addGroup failed: %v", rightIP, rightOut.err)) + } + + if leftOut.err != nil || rightOut.err != nil { + if (leftOut.err == nil) != (rightOut.err == nil) { + succeeded := leftIP + if leftOut.err != nil { + succeeded = rightIP + } + + PrintError(fmt.Sprintf("Partial group state on %s — clean up with `soundtouch-cli --host %s group remove`", succeeded, succeeded)) + } + + return fmt.Errorf("/addGroup propagation failed") + } + + // The LEFT (master) response carries the assigned group ID; use it for display. + PrintSuccess(fmt.Sprintf("Stereo pair created (id=%s)", leftOut.group.ID)) + printGroup(leftOut.group) return nil } +// addGroupOutcome is the per-speaker result of a parallel /addGroup call. +type addGroupOutcome struct { + host string + group *models.Group + err error +} + +// propagateAddGroup POSTs /addGroup to both speakers concurrently and returns +// the (LEFT, RIGHT) outcomes. A non-GROUP_OK Status in the response is +// reported as an error so callers don't have to re-inspect the body. +func propagateAddGroup(left, right *client.Client, leftIP, rightIP string, req *models.Group) (addGroupOutcome, addGroupOutcome) { + var ( + wg sync.WaitGroup + leftOut, rightOut addGroupOutcome + ) + + wg.Add(2) + + go func() { + defer wg.Done() + + leftOut = postAddGroup(left, leftIP, req) + }() + + go func() { + defer wg.Done() + + rightOut = postAddGroup(right, rightIP, req) + }() + + wg.Wait() + + return leftOut, rightOut +} + +func postAddGroup(cli *client.Client, host string, req *models.Group) addGroupOutcome { + out := addGroupOutcome{host: host} + + g, err := cli.AddGroup(req) + if err != nil { + out.err = err + return out + } + + out.group = g + + if g != nil && g.Status != "" && g.Status != "GROUP_OK" { + out.err = fmt.Errorf("device returned status %q (want GROUP_OK)", g.Status) + } + + return out +} + // renameGroup updates the name of the existing stereo pair. The device // requires the full structure on every update, so we fetch the current // state first. diff --git a/cmd/soundtouch-cli/cmd_group_test.go b/cmd/soundtouch-cli/cmd_group_test.go new file mode 100644 index 0000000..9e13245 --- /dev/null +++ b/cmd/soundtouch-cli/cmd_group_test.go @@ -0,0 +1,172 @@ +package main + +import ( + "encoding/xml" + "io" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/gesellix/bose-soundtouch/pkg/client" + "github.com/gesellix/bose-soundtouch/pkg/models" +) + +// happyAddGroupServer fakes a speaker's /addGroup that echoes the request +// with an assigned ID and GROUP_OK status, matching real hardware behaviour. +func happyAddGroupServer(t *testing.T, assignedID string) (*httptest.Server, *[]string) { + t.Helper() + + bodies := make([]string, 0) + + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path != "/addGroup" || r.Method != http.MethodPost { + t.Errorf("unexpected request: %s %s", r.Method, r.URL.Path) + http.NotFound(w, r) + + return + } + + body, _ := io.ReadAll(r.Body) + bodies = append(bodies, string(body)) + + var got models.Group + if err := xml.Unmarshal(body, &got); err != nil { + t.Fatalf("decode request body: %v", err) + } + + got.ID = assignedID + got.Status = "GROUP_OK" + + w.Header().Set("Content-Type", "application/xml") + + enc, _ := xml.Marshal(&got) + _, _ = w.Write(enc) + })) + + return srv, &bodies +} + +func newTestGroupClient(serverURL string) *client.Client { + return client.NewClientFromHost(serverURL) +} + +func sampleGroupRequest(leftIP, rightIP string) *models.Group { + return &models.Group{ + Name: "Living Room", + MasterDeviceID: "9070658C9D4A", + Roles: models.GroupRoles{ + Roles: []models.GroupRole{ + {DeviceID: "9070658C9D4A", Role: "LEFT", IPAddress: leftIP}, + {DeviceID: "F45EAB3115DA", Role: "RIGHT", IPAddress: rightIP}, + }, + }, + SenderIPAddress: leftIP, + } +} + +func TestPropagateAddGroup_BothSucceed(t *testing.T) { + leftSrv, leftBodies := happyAddGroupServer(t, "9999999") + defer leftSrv.Close() + + rightSrv, rightBodies := happyAddGroupServer(t, "9999999") + defer rightSrv.Close() + + leftClient := newTestGroupClient(leftSrv.URL) + rightClient := newTestGroupClient(rightSrv.URL) + + req := sampleGroupRequest("192.168.1.131", "192.168.1.134") + + leftOut, rightOut := propagateAddGroup(leftClient, rightClient, "192.168.1.131", "192.168.1.134", req) + + if leftOut.err != nil { + t.Errorf("LEFT err = %v, want nil", leftOut.err) + } + + if rightOut.err != nil { + t.Errorf("RIGHT err = %v, want nil", rightOut.err) + } + + if leftOut.group == nil || leftOut.group.ID != "9999999" || leftOut.group.Status != "GROUP_OK" { + t.Errorf("LEFT group = %+v, want id=9999999 status=GROUP_OK", leftOut.group) + } + + if rightOut.group == nil || rightOut.group.Status != "GROUP_OK" { + t.Errorf("RIGHT group = %+v, want status=GROUP_OK", rightOut.group) + } + + // Both speakers must have received the same payload, including senderIPAddress. + for _, bodies := range []*[]string{leftBodies, rightBodies} { + if len(*bodies) != 1 { + t.Fatalf("expected exactly one POST, got %d", len(*bodies)) + } + + body := (*bodies)[0] + for _, want := range []string{"LEFT", "RIGHT", "192.168.1.131"} { + if !strings.Contains(body, want) { + t.Errorf("body missing %q\nbody:\n%s", want, body) + } + } + } +} + +func TestPropagateAddGroup_RightFails(t *testing.T) { + leftSrv, _ := happyAddGroupServer(t, "9999999") + defer leftSrv.Close() + + rightSrv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + http.Error(w, "boom", http.StatusInternalServerError) + })) + defer rightSrv.Close() + + leftClient := newTestGroupClient(leftSrv.URL) + rightClient := newTestGroupClient(rightSrv.URL) + + req := sampleGroupRequest("192.168.1.131", "192.168.1.134") + + leftOut, rightOut := propagateAddGroup(leftClient, rightClient, "192.168.1.131", "192.168.1.134", req) + + if leftOut.err != nil { + t.Errorf("LEFT err = %v, want nil", leftOut.err) + } + + if rightOut.err == nil { + t.Error("RIGHT err = nil, want non-nil") + } +} + +func TestPostAddGroup_StatusOtherThanGroupOKIsError(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/xml") + _, _ = w.Write([]byte(`GROUP_NOT_READY`)) + })) + defer srv.Close() + + out := postAddGroup(newTestGroupClient(srv.URL), "test", sampleGroupRequest("1.1.1.1", "2.2.2.2")) + + if out.err == nil { + t.Fatal("expected error for non-GROUP_OK status") + } + + if !strings.Contains(out.err.Error(), "GROUP_NOT_READY") { + t.Errorf("error %q does not mention returned status", out.err) + } +} + +func TestPostAddGroup_EmptyStatusIsAccepted(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/xml") + _, _ = w.Write([]byte(`n`)) + })) + defer srv.Close() + + out := postAddGroup(newTestGroupClient(srv.URL), "test", sampleGroupRequest("1.1.1.1", "2.2.2.2")) + + if out.err != nil { + t.Errorf("err = %v, want nil for empty status (some firmware omits it)", out.err) + } + + if out.group == nil || out.group.ID != "42" { + t.Errorf("group = %+v, want id=42", out.group) + } +}