fix(player): stop transport-state observation from fencing FieldConnectivity polls

OnTransportState routed connect/disconnect through
applyConnectionStateEvent, which calls ApplyFieldEvent(FieldConnectivity,
...). That unconditionally bumps FieldConnectivity's applied generation,
so a transient WebSocket transport blip could invalidate a
concurrently-completing, genuinely successful HTTP poll's IsConnected
merge -- exactly the kind of cross-mechanism staleness bug the per-field
fencing was built to prevent. ObserveEventStreamTransport already derives
IsConnected (via applyConnectivityLocked) from the same transport signal,
so the extra call was redundant as well as unsafe.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
Tobias Gesellchen
2026-09-05 17:30:23 +02:00
co-authored by Claude Sonnet 5
parent d045812288
commit 4cb6044259
2 changed files with 38 additions and 6 deletions
+12 -6
View File
@@ -541,6 +541,7 @@ func (app *WebApp) ConnectDeviceWebSocket(deviceID string, conn *webtypes.Device
wsClient.OnVolumeUpdated(func(event *models.VolumeUpdatedEvent) {
activity := time.Now()
app.applyVolumeEvent(conn, &event.Volume)
conn.MarkEventStreamActivity(activity)
})
@@ -562,12 +563,14 @@ func (app *WebApp) ConnectDeviceWebSocket(deviceID string, conn *webtypes.Device
wsClient.OnPresetUpdated(func(event *models.PresetUpdatedEvent) {
activity := time.Now()
app.applyPresetEvent(conn, &event.Presets)
conn.MarkEventStreamActivity(activity)
})
wsClient.OnBassUpdated(func(event *models.BassUpdatedEvent) {
activity := time.Now()
app.applyBassEvent(conn, &event.Bass)
conn.MarkEventStreamActivity(activity)
})
@@ -582,16 +585,19 @@ func (app *WebApp) ConnectDeviceWebSocket(deviceID string, conn *webtypes.Device
})
wsClient.OnTransportState(func(connected bool, generation uint64) {
// ObserveEventStreamTransport already derives status.IsConnected
// (via applyConnectivityLocked, alongside Connectivity/
// HTTPReachable/WebSocketConnected) from this same call. Do NOT
// also route it through applyConnectionStateEvent/
// ApplyFieldEvent(FieldConnectivity, ...): that unconditionally
// bumps FieldConnectivity's applied generation past whatever an
// in-flight HTTP poll already reserved, so a transient transport
// blip would silently discard a concurrently-completing,
// genuinely successful poll's IsConnected=true merge.
if !conn.ObserveEventStreamTransport(generation, connected, time.Now()) {
return
}
// Drives the same FieldConnectivity fencing the old inline
// connect/disconnect sites used to update directly -- ordered by
// ObserveEventStreamTransport's own generation check above, so a
// reordered transport callback can no longer apply here either.
app.applyConnectionStateEvent(conn, connected)
if connected {
if generation > 1 {
log.Printf("WebSocket reconnected for device %s", sanitizeLog(deviceID))
@@ -596,6 +596,32 @@ func TestUnrelatedFieldEventDoesNotInvalidateInFlightPoll(t *testing.T) {
}
}
// TestTransportStateObservationDoesNotFenceFieldConnectivityPoll guards
// against routing ObserveEventStreamTransport through
// ApplyFieldEvent(FieldConnectivity, ...): that would unconditionally bump
// FieldConnectivity's applied generation past whatever an in-flight HTTP
// poll already reserved, so a transient WebSocket transport blip could
// silently discard a concurrently-completing, genuinely successful poll's
// IsConnected merge.
func TestTransportStateObservationDoesNotFenceFieldConnectivityPoll(t *testing.T) {
conn := NewDeviceConnection(nil, &models.DeviceInfo{Name: "test"})
connectivityPoll := conn.BeginFieldPoll(FieldConnectivity)
if !conn.ObserveEventStreamTransport(1, false, time.Now()) {
t.Fatal("first transport observation should be accepted")
}
if !conn.CompleteFieldPoll(FieldConnectivity, connectivityPoll, func(status *DeviceStatus) {
status.IsConnected = true
}) {
t.Fatal("a transport-state observation must not invalidate an in-flight FieldConnectivity poll")
}
if !conn.Status().IsConnected {
t.Fatal("FieldConnectivity poll result was discarded by an unrelated transport-state observation")
}
}
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