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).
This commit is contained in:
Tobias Gesellchen
2026-09-04 21:13:44 +02:00
parent 4b3b455d52
commit 78c847f929
2 changed files with 59 additions and 8 deletions
@@ -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"}})
+20 -8
View File
@@ -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)
}