diff --git a/pkg/service/soundtouchweb/browser_compatibility_test.go b/pkg/service/soundtouchweb/browser_compatibility_test.go index c39ad20d..ac9ff30a 100644 --- a/pkg/service/soundtouchweb/browser_compatibility_test.go +++ b/pkg/service/soundtouchweb/browser_compatibility_test.go @@ -246,6 +246,42 @@ window.revisionChecks = { sourcesStale: false, }) === stale, unknownRejected: mergeStatusUpdate(newer, 'unknown', { revision: 99 }) === newer, + // A fresh DeviceConnection for the same id restarts revisions at 0. Without + // the epoch check this frame loses the revision comparison and the tab stays + // pinned to the old status forever. + newerEpochAcceptedDespiteLowerRevision: (() => { + const epoched = mergeStatusUpdate(newer, 'speaker', { + epoch: 100, + revision: 40, + nowPlaying: { Track: 'epoch one' }, + }); + const reconnected = mergeStatusUpdate(epoched, 'speaker', { + epoch: 101, + revision: 0, + nowPlaying: { Track: 'epoch two' }, + }); + return reconnected !== epoched && + reconnected.speaker.status.nowPlaying.Track === 'epoch two'; + })(), + olderEpochRejected: (() => { + const epochOne = mergeStatusUpdate(newer, 'speaker', { + epoch: 100, + revision: 40, + nowPlaying: { Track: 'epoch one' }, + }); + const epochTwo = mergeStatusUpdate(epochOne, 'speaker', { + epoch: 101, + revision: 0, + nowPlaying: { Track: 'epoch two' }, + }); + // A frame still in flight from the replaced connection, carrying a high + // revision from its own sequence, must not win. + return mergeStatusUpdate(epochTwo, 'speaker', { + epoch: 100, + revision: 99, + nowPlaying: { Track: 'late frame from the old connection' }, + }) === epochTwo; + })(), adversarialIDsAreOwnDataProperties: Object.prototype.hasOwnProperty.call(constructorUpdated, '__proto__') && Object.prototype.hasOwnProperty.call(constructorUpdated, 'constructor'), diff --git a/pkg/service/soundtouchweb/static/js/app.js b/pkg/service/soundtouchweb/static/js/app.js index 2ec09881..bf7ffcd5 100644 --- a/pkg/service/soundtouchweb/static/js/app.js +++ b/pkg/service/soundtouchweb/static/js/app.js @@ -26,11 +26,27 @@ function statusRevision(status) { return Number.isSafeInteger(revision) && revision >= 0 ? revision : null; } +function statusEpoch(status) { + const epoch = status?.epoch; + return Number.isSafeInteger(epoch) ? epoch : null; +} + // The server advances DeviceStatus.revision on every projection, so a frame // carrying a revision no newer than what we already hold is stale -- a slow // `devices` snapshot overtaken by a `status_update` delta, or a REST refresh // overtaken by either. A device we have never seen is always accepted. +// +// Revisions are only comparable within one epoch. A device id backed by a new +// DeviceConnection, or a restarted service, restarts its revisions at 0, and +// without the epoch check the browser would reject every later frame for that +// id and display a status frozen at whatever it last held. function acceptsNewerStatus(current, incoming) { + const currentEpoch = statusEpoch(current); + const incomingEpoch = statusEpoch(incoming); + if (currentEpoch !== null && incomingEpoch !== null && incomingEpoch !== currentEpoch) { + return incomingEpoch > currentEpoch; + } + const currentRevision = statusRevision(current); const incomingRevision = statusRevision(incoming); if (currentRevision === null) return true; diff --git a/pkg/service/soundtouchweb/webtypes/revision_test.go b/pkg/service/soundtouchweb/webtypes/revision_test.go index 14946448..367ace24 100644 --- a/pkg/service/soundtouchweb/webtypes/revision_test.go +++ b/pkg/service/soundtouchweb/webtypes/revision_test.go @@ -95,6 +95,46 @@ func TestNowPlayingRevisionAdvancesForPollAndEvent(t *testing.T) { } } +// TestStatusEpochDistinguishesConnections: Revision restarts at 0 for every +// new DeviceConnection, so a browser holding a high revision for a device id +// would reject the replacement connection's frames forever. Epoch is what +// makes the two sequences distinguishable, and it must strictly increase. +func TestStatusEpochDistinguishesConnections(t *testing.T) { + first := NewDeviceConnection(nil, nil) + second := NewDeviceConnection(nil, nil) + + firstEpoch := first.Status().Epoch + secondEpoch := second.Status().Epoch + + if firstEpoch <= 0 || secondEpoch <= firstEpoch { + t.Fatalf("epochs = %d then %d, want strictly increasing and non-zero", firstEpoch, secondEpoch) + } + + // Both entry points must stamp it: a status that lost its epoch would be + // indistinguishable from one produced before epochs existed. + first.UpdateStatus(func(status *DeviceStatus) { + status.NowPlaying = &models.NowPlaying{Source: "AUX"} + }) + if got := first.Status().Epoch; got != firstEpoch { + t.Errorf("epoch after UpdateStatus = %d, want %d", got, firstEpoch) + } + + first.SetStatus(&DeviceStatus{}) + if got := first.Status().Epoch; got != firstEpoch { + t.Errorf("epoch after SetStatus = %d, want %d", got, firstEpoch) + } +} + +// TestStatusEpochStaysWithinJavaScriptSafeIntegers: the browser compares this +// value as a JSON number, so it must not exceed Number.MAX_SAFE_INTEGER. +func TestStatusEpochStaysWithinJavaScriptSafeIntegers(t *testing.T) { + const maxSafeInteger = int64(1)<<53 - 1 + + if got := NewDeviceConnection(nil, nil).Status().Epoch; got > maxSafeInteger { + t.Fatalf("epoch = %d, exceeds Number.MAX_SAFE_INTEGER (%d)", got, maxSafeInteger) + } +} + func TestDeviceStatusRevisionIsMonotonicWithConcurrentProjections(t *testing.T) { conn := NewDeviceConnection(nil, nil) const projections = 64 diff --git a/pkg/service/soundtouchweb/webtypes/types.go b/pkg/service/soundtouchweb/webtypes/types.go index 3e229ffd..d1979ffb 100644 --- a/pkg/service/soundtouchweb/webtypes/types.go +++ b/pkg/service/soundtouchweb/webtypes/types.go @@ -54,6 +54,9 @@ type DeviceConnection struct { deviceName atomic.Pointer[string] status atomic.Pointer[DeviceStatus] + // epoch stamps every status this connection publishes. See nextStatusEpoch. + epoch int64 + webSocketMu sync.RWMutex webSocketLoopRunning atomic.Bool @@ -121,6 +124,10 @@ type DeviceStatus struct { IsConnected bool `json:"isConnected"` LastActivity time.Time `json:"lastActivity"` + // Epoch identifies the DeviceConnection that produced this status. + // Revision restarts at 0 for every new connection, so revisions from + // different epochs are not comparable; a client must compare Epoch first. + Epoch int64 `json:"epoch"` // Revision is a per-connection monotonic counter advanced by every // successful UpdateStatus. It lets the browser order a full `devices` // snapshot against a `status_update` delta -- without it the two frames @@ -157,6 +164,33 @@ type SpeakerConnectionState struct { Signal string `json:"signal,omitempty"` } +// statusEpochClock hands out strictly increasing epochs, seeded from the wall +// clock so they keep increasing across a service restart too. A plain counter +// would restart at 0 on restart and a plain timestamp could collide for two +// connections created in the same millisecond; both would leave a browser +// unable to tell a newer connection's revision sequence from an older one. +var statusEpochClock atomic.Int64 + +func nextStatusEpoch() int64 { + // Milliseconds, not nanoseconds: this value is compared in the browser, + // where a nanosecond timestamp exceeds Number.MAX_SAFE_INTEGER and would + // lose precision as a JSON number. + now := time.Now().UnixMilli() + + for { + previous := statusEpochClock.Load() + + next := now + if next <= previous { + next = previous + 1 + } + + if statusEpochClock.CompareAndSwap(previous, next) { + return next + } + } +} + // StatusField identifies one independently-racing field of DeviceStatus for // BeginFieldPoll/CompleteFieldPoll/ApplyFieldEvent's generation ordering. // FieldConnectivity covers IsConnected, which is derived by a poll (from @@ -187,11 +221,13 @@ func NewDeviceConnection(c *client.Client, info *models.DeviceInfo) *DeviceConne DeviceInfo: info, LastSeen: time.Now(), done: make(chan struct{}), + epoch: nextStatusEpoch(), } conn.status.Store(&DeviceStatus{ Connectivity: ConnectivityOffline, IsConnected: false, LastActivity: time.Now(), + Epoch: conn.epoch, }) if info != nil { @@ -360,6 +396,7 @@ func (c *DeviceConnection) SetStatus(s *DeviceStatus) { for { old := c.status.Load() next := *s + next.Epoch = c.epoch next.Revision = old.Revision + 1 next.NowPlayingRevision = nowPlayingGeneration @@ -468,6 +505,7 @@ func (c *DeviceConnection) UpdateStatus(mut func(*DeviceStatus)) { old := c.status.Load() next := *old mut(&next) + next.Epoch = c.epoch next.Revision = old.Revision + 1 if c.status.CompareAndSwap(old, &next) {