From 78c847f929dfa11d11fd993ab439d924ea19f873 Mon Sep 17 00:00:00 2001 From: Tobias Gesellchen Date: Fri, 4 Sep 2026 20:27:29 +0200 Subject: [PATCH] fix(service): invalidate polled Group by completion order, not start order BeginGroupRefresh/ApplyPolledGroup keyed invalidation off "has any newer poll started" via groupGeneration equality. A later poll that starts but never applies (its own GetGroup fails) still discarded an earlier poll's still-arriving successful result, even though nothing newer ever actually landed. Add groupAppliedGeneration and gate on "strictly newer than the last applied", not "equal to the latest issued". Found in code review of PR #665 (finding #2). --- .../soundtouchweb/webtypes/status_test.go | 39 +++++++++++++++++++ pkg/service/soundtouchweb/webtypes/types.go | 28 +++++++++---- 2 files changed, 59 insertions(+), 8 deletions(-) diff --git a/pkg/service/soundtouchweb/webtypes/status_test.go b/pkg/service/soundtouchweb/webtypes/status_test.go index 7c77db79..c3da39e7 100644 --- a/pkg/service/soundtouchweb/webtypes/status_test.go +++ b/pkg/service/soundtouchweb/webtypes/status_test.go @@ -163,6 +163,45 @@ func TestGroupEventSupersedesInFlightPoll(t *testing.T) { } } +// TestPolledGroupAppliesAfterNewerRefreshStartFailsToApply guards against +// invalidating by start order: a second poll starting (and never applying, +// e.g. its own GetGroup failed) must not discard an earlier poll's +// still-arriving successful result. +func TestPolledGroupAppliesAfterNewerRefreshStartFailsToApply(t *testing.T) { + conn := NewDeviceConnection(nil, &models.DeviceInfo{Name: "test"}) + genA := conn.BeginGroupRefresh() + _ = conn.BeginGroupRefresh() // a second poll starts but never calls ApplyPolledGroup + + if !conn.ApplyPolledGroup(genA, &models.Group{ID: "pair-1", MasterDeviceID: "master"}) { + t.Fatal("an older poll's successful result must still apply when nothing newer ever actually applied") + } + + if got := conn.Status().Group; got == nil || got.ID != "pair-1" { + t.Fatalf("Group = %+v, want pair-1 applied", got) + } +} + +// TestOlderPolledGroupRejectedAfterNewerPollApplies guards the original +// protection this mechanism exists for: a genuinely newer successful poll +// must not be clobbered by an older one arriving late. +func TestOlderPolledGroupRejectedAfterNewerPollApplies(t *testing.T) { + conn := NewDeviceConnection(nil, &models.DeviceInfo{Name: "test"}) + genA := conn.BeginGroupRefresh() + genB := conn.BeginGroupRefresh() + + if !conn.ApplyPolledGroup(genB, &models.Group{ID: "pair-new", MasterDeviceID: "master"}) { + t.Fatal("newer poll result should apply") + } + + if conn.ApplyPolledGroup(genA, &models.Group{ID: "pair-stale", MasterDeviceID: "master"}) { + t.Fatal("an older poll's result arriving after a newer one already applied must be rejected") + } + + if got := conn.Status().Group; got == nil || got.ID != "pair-new" { + t.Fatalf("Group = %+v, want pair-new preserved", got) + } +} + func TestEmptyGroupClearsCurrentClaim(t *testing.T) { conn := NewDeviceConnection(nil, &models.DeviceInfo{Name: "test"}) conn.SetStatus(&DeviceStatus{Group: &models.Group{ID: "pair-1"}}) diff --git a/pkg/service/soundtouchweb/webtypes/types.go b/pkg/service/soundtouchweb/webtypes/types.go index 9eaaac99..5f71b983 100644 --- a/pkg/service/soundtouchweb/webtypes/types.go +++ b/pkg/service/soundtouchweb/webtypes/types.go @@ -48,10 +48,17 @@ type DeviceConnection struct { status atomic.Pointer[DeviceStatus] // groupMu orders polled /getGroup responses against real-time - // groupUpdated events. Starting a newer refresh or receiving an event - // invalidates any older in-flight poll. - groupMu sync.Mutex - groupGeneration uint64 + // groupUpdated events. groupGeneration is the highest generation + // issued (by BeginGroupRefresh or ApplyGroupEvent); + // groupAppliedGeneration is the highest generation whose result was + // actually applied. Invalidation keys off completion order via + // groupAppliedGeneration, not merely "a newer refresh has started" — + // otherwise a later poll that starts but never applies (e.g. its own + // GetGroup fails) would still discard an earlier poll's still-arriving + // successful result. + groupMu sync.Mutex + groupGeneration uint64 + groupAppliedGeneration uint64 // done is closed by Close when the device is removed from the // registry, signalling its background goroutines (the status poller @@ -164,26 +171,31 @@ func (c *DeviceConnection) BeginGroupRefresh() uint64 { return c.groupGeneration } -// ApplyPolledGroup stores a /getGroup result only when no newer poll or -// groupUpdated event superseded it. Empty groups clear the current claim. +// ApplyPolledGroup stores a /getGroup result only if no strictly newer +// result (poll or event) has already applied. Empty groups clear the +// current claim. func (c *DeviceConnection) ApplyPolledGroup(generation uint64, group *models.Group) bool { c.groupMu.Lock() defer c.groupMu.Unlock() - if generation != c.groupGeneration { + if generation <= c.groupAppliedGeneration { return false } + c.groupAppliedGeneration = generation + return c.replaceGroup(normalizeGroup(group), time.Time{}) } // ApplyGroupEvent stores the newest groupUpdated event and invalidates all -// in-flight /getGroup requests. Empty teardown events clear the current claim. +// in-flight /getGroup requests, including ones that have not started yet. +// Empty teardown events clear the current claim. func (c *DeviceConnection) ApplyGroupEvent(group *models.Group, activity time.Time) bool { c.groupMu.Lock() defer c.groupMu.Unlock() c.groupGeneration++ + c.groupAppliedGeneration = c.groupGeneration return c.replaceGroup(normalizeGroup(group), activity) }