From 522c6b8cb6c3496df8c4da0249ac3afa80480780 Mon Sep 17 00:00:00 2001 From: Tobias Gesellchen Date: Sun, 30 Aug 2026 21:55:30 +0200 Subject: [PATCH] fix(models): make group-equality order-insensitive everywhere sameGroupClaim (device_projection.go, used to validate a member's claim agrees with the master's) compared roles via a device-ID-keyed map, making it order-insensitive. webtypes.replaceGroup's change detection used reflect.DeepEqual on the whole *Group, which is order-sensitive for Roles.Roles. Both the polled /getGroup response and the pushed groupUpdated event populate Roles.Roles directly from XML unmarshaling in wire order, so nothing guarantees a pair's roles list in the same order across two reads -- DeepEqual could then report a spurious "changed" for a pair that didn't actually change. Extracted the order-insensitive comparison into models.SameGroup as the single shared implementation (also handles the nil/nil case correctly, unlike the old sameGroupClaim, which mattered for replaceGroup's existing "no prior group" path). Both call sites now use it; the duplicate sameGroupClaim is gone. Added TestApplyGroupEventIgnoresRoleOrder, verified to fail against the prior DeepEqual-based logic and pass with this fix. Co-Authored-By: Claude Sonnet 5 --- pkg/models/group.go | 42 ++++++++++++++++++- .../soundtouchweb/device_projection.go | 22 +--------- .../soundtouchweb/webtypes/status_test.go | 33 +++++++++++++++ pkg/service/soundtouchweb/webtypes/types.go | 3 +- 4 files changed, 76 insertions(+), 24 deletions(-) diff --git a/pkg/models/group.go b/pkg/models/group.go index eab4c6bb..cc0074b6 100644 --- a/pkg/models/group.go +++ b/pkg/models/group.go @@ -1,6 +1,9 @@ package models -import "encoding/xml" +import ( + "encoding/xml" + "strings" +) // Group represents a stereo pair of two ST10 SoundTouch speakers. type Group struct { @@ -32,3 +35,40 @@ type GroupRole struct { Role string `xml:"role"` IPAddress string `xml:"ipAddress,omitempty"` } + +// SameGroup reports whether left and right describe the same stereo-pair +// configuration, comparing role assignments by device ID rather than by +// slice order. The device's own /getGroup response and its groupUpdated +// WebSocket event both populate Roles.Roles directly from XML unmarshaling +// in wire order, so a polled read and a pushed event for the identical pair +// are not guaranteed to list roles in the same order -- comparing with +// reflect.DeepEqual (order-sensitive) would then report a spurious change +// even though nothing about the pair actually changed. Two nil Groups are +// equal; exactly one nil is not. +func SameGroup(left, right *Group) bool { + if left == nil && right == nil { + return true + } + + if left == nil || right == nil { + return false + } + + if left.ID != right.ID || left.MasterDeviceID != right.MasterDeviceID || + len(left.Roles.Roles) != len(right.Roles.Roles) { + return false + } + + rightRoles := make(map[string]string, len(right.Roles.Roles)) + for _, role := range right.Roles.Roles { + rightRoles[strings.TrimSpace(role.DeviceID)] = strings.ToUpper(strings.TrimSpace(role.Role)) + } + + for _, role := range left.Roles.Roles { + if rightRoles[strings.TrimSpace(role.DeviceID)] != strings.ToUpper(strings.TrimSpace(role.Role)) { + return false + } + } + + return true +} diff --git a/pkg/service/soundtouchweb/device_projection.go b/pkg/service/soundtouchweb/device_projection.go index 1b47dc4f..cbb5fc99 100644 --- a/pkg/service/soundtouchweb/device_projection.go +++ b/pkg/service/soundtouchweb/device_projection.go @@ -204,27 +204,7 @@ func registeredMembersAgree(group *models.Group, byDeviceID map[string][]deviceP continue } - if entries[0].Status == nil || !sameGroupClaim(group, entries[0].Status.Group) { - return false - } - } - - return true -} - -func sameGroupClaim(left, right *models.Group) bool { - if left == nil || right == nil || left.ID != right.ID || left.MasterDeviceID != right.MasterDeviceID || - len(left.Roles.Roles) != len(right.Roles.Roles) { - return false - } - - rightRoles := make(map[string]string, len(right.Roles.Roles)) - for _, role := range right.Roles.Roles { - rightRoles[strings.TrimSpace(role.DeviceID)] = strings.ToUpper(strings.TrimSpace(role.Role)) - } - - for _, role := range left.Roles.Roles { - if rightRoles[strings.TrimSpace(role.DeviceID)] != strings.ToUpper(strings.TrimSpace(role.Role)) { + if entries[0].Status == nil || !models.SameGroup(group, entries[0].Status.Group) { return false } } diff --git a/pkg/service/soundtouchweb/webtypes/status_test.go b/pkg/service/soundtouchweb/webtypes/status_test.go index fbeae144..7c77db79 100644 --- a/pkg/service/soundtouchweb/webtypes/status_test.go +++ b/pkg/service/soundtouchweb/webtypes/status_test.go @@ -176,6 +176,39 @@ func TestEmptyGroupClearsCurrentClaim(t *testing.T) { } } +// TestApplyGroupEventIgnoresRoleOrder guards replaceGroup's change-detection +// against a spurious "changed" report when the same pair's roles simply +// arrive in a different order -- a polled /getGroup response and a pushed +// groupUpdated event both populate Roles.Roles straight from XML unmarshal +// in wire order, so nothing guarantees they list LEFT/RIGHT the same way +// every time for the identical pair. +func TestApplyGroupEventIgnoresRoleOrder(t *testing.T) { + conn := NewDeviceConnection(nil, &models.DeviceInfo{Name: "test"}) + + leftFirst := &models.Group{ + ID: "pair-1", + MasterDeviceID: "master", + Roles: models.GroupRoles{Roles: []models.GroupRole{ + {DeviceID: "master", Role: "LEFT"}, + {DeviceID: "member", Role: "RIGHT"}, + }}, + } + conn.SetStatus(&DeviceStatus{Group: leftFirst}) + + rightFirst := &models.Group{ + ID: "pair-1", + MasterDeviceID: "master", + Roles: models.GroupRoles{Roles: []models.GroupRole{ + {DeviceID: "member", Role: "RIGHT"}, + {DeviceID: "master", Role: "LEFT"}, + }}, + } + + if conn.ApplyGroupEvent(rightFirst, time.Now()) { + t.Fatal("reordered roles for the same pair must not report a change") + } +} + func TestStatusSnapshotIsolation(t *testing.T) { // A snapshot returned by Status() must NOT change when a later // UpdateStatus replaces a pointer field. This proves the atomic diff --git a/pkg/service/soundtouchweb/webtypes/types.go b/pkg/service/soundtouchweb/webtypes/types.go index 5f2bbba7..9eaaac99 100644 --- a/pkg/service/soundtouchweb/webtypes/types.go +++ b/pkg/service/soundtouchweb/webtypes/types.go @@ -2,7 +2,6 @@ package webtypes import ( - "reflect" "sync" "sync/atomic" "time" @@ -190,7 +189,7 @@ func (c *DeviceConnection) ApplyGroupEvent(group *models.Group, activity time.Ti } func (c *DeviceConnection) replaceGroup(group *models.Group, activity time.Time) bool { - changed := !reflect.DeepEqual(c.Status().Group, group) + changed := !models.SameGroup(c.Status().Group, group) c.UpdateStatus(func(s *DeviceStatus) { s.Group = group if !activity.IsZero() {