fix(cli): POST /addGroup to both speakers in parallel for stereo pair

createGroup used to POST only to the LEFT (master) speaker and rely on the master to propagate the group to the slave via marge. That round- trip is the source of the "context deadline exceeded" failures reported in #252 — the master blocks waiting for marge while the CLI times out client-side. SoundCork's working ST10 implementation addresses each speaker directly, which avoids the inter-device coordination entirely. Changes: * Build the group request with senderIPAddress = master IP (the fhem wiki documents this field; SoundCork sets it; we previously omitted it). * propagateAddGroup() POSTs the same payload to both speakers concurrently via a sync.WaitGroup and returns per-side outcomes. * postAddGroup() flags a non-GROUP_OK response Status as an error so the caller doesn't have to re-parse the body. * On partial failure (one side succeeded), surface a remove command the user can run to clean up. Tests cover the happy path (both succeed, payload shape correct), the right-side-fails path, the non-GROUP_OK response, and an empty-status response (some firmware omits Status entirely on a successful echo). Refs #252. Optimistic fix — still pending feedback from BirdyBA's two-curl test on real ST10s before we're confident. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2026-08-18 08:36:13 +00:00 · 2026-05-15 09:05:14 +02:00
parent 5fd7e8c0ba
commit f89b2243c2
2 changed files with 259 additions and 5 deletions
@@ -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.
@@ -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{"<role>LEFT</role>", "<role>RIGHT</role>", "<senderIPAddress>192.168.1.131</senderIPAddress>"} {
+			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><status>GROUP_NOT_READY</status></group>`))
+	}))
+	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(`<group id="42"><name>n</name></group>`))
+	}))
+	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)
+	}
+}