diff --git a/pkg/service/soundtouchweb/browser_compatibility_test.go b/pkg/service/soundtouchweb/browser_compatibility_test.go index 0d6e38dc..a01753bf 100644 --- a/pkg/service/soundtouchweb/browser_compatibility_test.go +++ b/pkg/service/soundtouchweb/browser_compatibility_test.go @@ -232,8 +232,15 @@ window.revisionChecks = { equalRejected: equal === snapshot, olderRejected: older === equal, newerAccepted: newer !== older && newer.speaker.status.revision === 6 && newer.speaker.status.nowPlaying.Track === 'newest', - derivedStaleAccepted: stale !== newer && stale.speaker.status.sourcesStale === true && - stale.speaker.status.nowPlaying.Track === 'newest', + staleAtEqualRevisionRejected: stale === newer, + staleAtNewerRevisionAccepted: (() => { + const applied = mergeStatusUpdate(newer, 'speaker', { + revision: 7, + sourcesStale: true, + nowPlaying: { Track: 'newest' }, + }); + return applied !== newer && applied.speaker.status.sourcesStale === true; + })(), staleCannotClearAtEqualRevision: mergeStatusUpdate(stale, 'speaker', { revision: 6, sourcesStale: false, @@ -267,7 +274,10 @@ window.revisionChecks = { } } -func TestDerivedSourceExpiryDisablesCommandsAtEqualRevision(t *testing.T) { +// TestSourceExpiryDisablesCommandsOnNewerRevision: a stale marker pushed over +// the socket must reach the buttons and make them unclickable, without the +// projection ever showing the source as selected. +func TestSourceExpiryDisablesCommandsOnNewerRevision(t *testing.T) { const sourceExpiryFixture = ` import { h, render } from 'preact'; import { mergeStatusUpdate } from '/app/static/js/app.js'; @@ -293,11 +303,11 @@ function redraw() { } window.expireSources = () => { devices = mergeStatusUpdate(devices, 'speaker', { - revision: 5, + revision: 6, nowPlayingRevision: 5, sourcesStale: true, sources: { SourceItem: ready }, - nowPlaying: { Source: 'AUX', SourceAccount: 'AUX1' }, + nowPlaying: { Source: 'STANDBY', SourceAccount: '' }, }); redraw(); }; @@ -333,7 +343,7 @@ redraw(); mu.Lock() defer mu.Unlock() if writes != 0 || trackSource != "STANDBY" { - t.Fatalf("derived expiry writes=%d projected source=%q, want 0 and STANDBY", writes, trackSource) + t.Fatalf("stale source expiry writes=%d projected source=%q, want 0 and STANDBY", writes, trackSource) } } diff --git a/pkg/service/soundtouchweb/source_cache_test.go b/pkg/service/soundtouchweb/source_cache_test.go index a2ab86aa..19dee1e0 100644 --- a/pkg/service/soundtouchweb/source_cache_test.go +++ b/pkg/service/soundtouchweb/source_cache_test.go @@ -2,11 +2,9 @@ package soundtouchweb import ( "encoding/json" - "errors" "net/http" "net/http/httptest" "testing" - "time" "github.com/gesellix/bose-soundtouch/pkg/client" "github.com/gesellix/bose-soundtouch/pkg/models" @@ -44,38 +42,57 @@ func TestUpdateDeviceStatusDoesNotRefreshNowPlayingRevisionOnFailure(t *testing. } } -// TestUpdateSourcesCacheRetainsFailureAndRefreshesSuccess: a failed refresh -// keeps the previous inventory and its read time but marks it stale; the next -// success replaces both and restores actionability. -func TestUpdateSourcesCacheRetainsFailureAndRefreshesSuccess(t *testing.T) { - conn := webtypes.NewDeviceConnection(nil, nil) - // Read times must sit inside sourceCacheTTL of now: Status() derives - // staleness against the wall clock, so fixed calendar dates would read - // back as expired no matter what this test does. - firstRead := time.Now() - oldSources := &models.Sources{SourceItem: []models.SourceItem{{Source: "AUX"}}} +// TestUpdateDeviceStatusMarksSourcesStaleOnFailedRead: unlike every other +// field, a FAILED /sources read is still merged -- the last known inventory +// stays visible but is marked unusable, because offering source buttons the +// speaker no longer confirms is worse than offering none. +func TestUpdateDeviceStatusMarksSourcesStaleOnFailedRead(t *testing.T) { + sourcesOK := true + speaker := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/sources" { + if !sourcesOK { + http.Error(w, "unavailable", http.StatusServiceUnavailable) - updateSourcesCache(conn, conn.BeginFieldPoll(webtypes.FieldSources), oldSources, nil, firstRead) + return + } - if !updateSourcesCache(conn, conn.BeginFieldPoll(webtypes.FieldSources), nil, - errors.New("temporary read failure"), firstRead.Add(time.Second)) { - t.Fatal("failed source readback was not recorded") + w.Header().Set("Content-Type", "application/xml") + _, _ = w.Write([]byte(`Aux 1`)) + + return + } + + w.Header().Set("Content-Type", "application/xml") + _, _ = w.Write([]byte(`3535false`)) + })) + defer speaker.Close() + + app := NewWebApp() + conn := webtypes.NewDeviceConnection(client.NewClient(&client.Config{Host: speaker.URL}), nil) + + app.UpdateDeviceStatus("speaker", conn) + + fresh := conn.Status() + if fresh.SourcesStale || fresh.Sources == nil || len(fresh.Sources.SourceItem) != 1 { + t.Fatalf("successful source read was not merged as actionable: %+v", fresh) } - status := conn.Status() - if status.Sources != oldSources || !status.SourcesReadAt.Equal(firstRead) || !status.SourcesStale { - t.Fatalf("failed source readback changed the cache: %+v", status) + sourcesOK = false + app.UpdateDeviceStatus("speaker", conn) + + stale := conn.Status() + if !stale.SourcesStale { + t.Fatalf("failed source read did not mark the inventory stale: %+v", stale) + } + if stale.Sources != fresh.Sources { + t.Fatalf("failed source read discarded the last known inventory: %+v", stale) } - secondRead := time.Now() - newSources := &models.Sources{SourceItem: []models.SourceItem{{Source: "PRODUCT"}}} - if !updateSourcesCache(conn, conn.BeginFieldPoll(webtypes.FieldSources), newSources, nil, secondRead) { - t.Fatal("successful source readback was not reported as an update") - } + sourcesOK = true + app.UpdateDeviceStatus("speaker", conn) - status = conn.Status() - if status.Sources != newSources || !status.SourcesReadAt.Equal(secondRead) || status.SourcesStale { - t.Fatalf("successful source readback did not refresh the cache: %+v", status) + if recovered := conn.Status(); recovered.SourcesStale { + t.Fatalf("successful source read did not clear staleness: %+v", recovered) } } diff --git a/pkg/service/soundtouchweb/static/js/app.js b/pkg/service/soundtouchweb/static/js/app.js index 3308ab8b..e0d4f9cc 100644 --- a/pkg/service/soundtouchweb/static/js/app.js +++ b/pkg/service/soundtouchweb/static/js/app.js @@ -37,20 +37,6 @@ function acceptsNewerStatus(current, incoming) { return incomingRevision !== null && incomingRevision > currentRevision; } -// The server derives sourcesStale at read time, so two reads at the same -// revision can disagree about it. Carry that one derived bit forward without -// letting an otherwise-stale frame replace the canonical state. -function mergeDerivedStatus(current, incoming) { - const currentRevision = statusRevision(current); - const incomingRevision = statusRevision(incoming); - if (currentRevision === null || incomingRevision !== currentRevision || - current?.sourcesStale === true || incoming?.sourcesStale !== true) { - return current; - } - - return { ...current, sourcesStale: true }; -} - export function mergeDevicesSnapshot(previous, snapshot) { return Object.fromEntries(Object.entries(snapshot || {}).map(([deviceId, incoming]) => { const current = Object.prototype.hasOwnProperty.call(previous, deviceId) @@ -58,10 +44,10 @@ export function mergeDevicesSnapshot(previous, snapshot) { if (!current || acceptsNewerStatus(current.status, incoming?.status)) { return [deviceId, incoming]; } - return [deviceId, { - ...incoming, - status: mergeDerivedStatus(current.status, incoming?.status), - }]; + // Keep the newer status we already hold, but take the rest of the + // incoming entry: info/stereoPair travel with the snapshot, not with + // the status revision. + return [deviceId, { ...incoming, status: current.status }]; })); } @@ -79,13 +65,7 @@ export function mergeStatusUpdate(previous, deviceId, status) { // check despite not being a real, known device. if (!Object.prototype.hasOwnProperty.call(previous, deviceId) || !acceptsNewerStatus(previous[deviceId]?.status, status)) { - const current = previous[deviceId]?.status; - const merged = mergeDerivedStatus(current, status); - if (merged === current) return previous; - return replaceDevice(previous, deviceId, { - ...previous[deviceId], - status: merged, - }); + return previous; } return replaceDevice(previous, deviceId, { ...previous[deviceId], diff --git a/pkg/service/soundtouchweb/websocket.go b/pkg/service/soundtouchweb/websocket.go index 50dcba96..1ed174ca 100644 --- a/pkg/service/soundtouchweb/websocket.go +++ b/pkg/service/soundtouchweb/websocket.go @@ -752,7 +752,6 @@ func (app *WebApp) updateDeviceStatus(_ string, conn *webtypes.DeviceConnection, volume, volumeErr := conn.Client.GetVolume() presets, presetsErr := conn.Client.GetPresets() sources, sourcesErr := conn.Client.GetSources() - sourcesReadAt := time.Now() bass, bassErr := conn.Client.GetBass() var ( @@ -800,11 +799,24 @@ func (app *WebApp) updateDeviceStatus(_ string, conn *webtypes.DeviceConnection, anyFetchSucceeded = true } - // Unlike the other fields, a FAILED /sources read is merged too: the last - // known inventory stays visible but is marked stale so it cannot be acted - // on. Running through CompleteFieldPoll is what makes a newer failure - // fence an older, still-in-flight success. - updateSourcesCache(conn, sourcesGen, sources, sourcesErr, sourcesReadAt) + // Unlike the other fields, a FAILED /sources read is merged too. The + // inventory drives which source buttons the player offers, and acting on + // an inventory the speaker no longer confirms is worse than offering + // nothing: the last known list stays visible, but SourcesStale disables + // it until a read succeeds again. Running through CompleteFieldPoll (not + // a bare UpdateStatus) is what makes a newer failure fence an older, + // still-in-flight success rather than being silently overwritten by it. + conn.CompleteFieldPoll(webtypes.FieldSources, sourcesGen, func(s *webtypes.DeviceStatus) { + if sourcesErr != nil { + s.SourcesStale = true + + return + } + + s.Sources = sources + s.SourcesStale = false + s.LastActivity = time.Now() + }) if bassErr == nil { anyFetchSucceeded = true @@ -851,30 +863,6 @@ func (app *WebApp) updateDeviceStatus(_ string, conn *webtypes.DeviceConnection, } } -// updateSourcesCache records a source refresh, successful or not. A failure -// keeps the previous inventory and its read time, but marks it stale so the -// player stops offering it until a read succeeds again. -func updateSourcesCache( - conn *webtypes.DeviceConnection, - generation uint64, - sources *models.Sources, - err error, - readAt time.Time, -) bool { - return conn.CompleteFieldPoll(webtypes.FieldSources, generation, func(s *webtypes.DeviceStatus) { - if err != nil { - s.SourcesStale = true - - return - } - - s.Sources = sources - s.SourcesReadAt = readAt - s.SourcesStale = false - s.LastActivity = time.Now() - }) -} - func (app *WebApp) applyGroupUpdatedEvent( conn *webtypes.DeviceConnection, event *models.GroupUpdatedEvent, diff --git a/pkg/service/soundtouchweb/webtypes/source_cache_test.go b/pkg/service/soundtouchweb/webtypes/source_cache_test.go index 39a1a569..361405d5 100644 --- a/pkg/service/soundtouchweb/webtypes/source_cache_test.go +++ b/pkg/service/soundtouchweb/webtypes/source_cache_test.go @@ -4,52 +4,63 @@ import ( "encoding/json" "strings" "testing" - "time" "github.com/gesellix/bose-soundtouch/pkg/models" ) -func TestSourceCacheStatusAtTTLBoundary(t *testing.T) { - readAt := time.Date(2026, time.August, 30, 12, 0, 0, 0, time.UTC) - sources := &models.Sources{SourceItem: []models.SourceItem{{Source: "AUX"}}} - status := &DeviceStatus{ - Sources: sources, - SourcesReadAt: readAt, +// TestSourcesFailureFencesOlderOverlappingSuccess is the ordering property +// that makes the stale marker trustworthy: a /sources read that FAILED must +// not be undone by an older, still-in-flight read that happens to succeed +// after it. Both go through CompleteFieldPoll(FieldSources, ...), so the +// failure's newer generation wins. +func TestSourcesFailureFencesOlderOverlappingSuccess(t *testing.T) { + conn := NewDeviceConnection(nil, nil) + retained := &models.Sources{SourceItem: []models.SourceItem{{Source: "PRODUCT"}}} + + firstGeneration := conn.BeginFieldPoll(FieldSources) + secondGeneration := conn.BeginFieldPoll(FieldSources) + + conn.CompleteFieldPoll(FieldSources, secondGeneration, func(status *DeviceStatus) { + status.Sources = retained + status.SourcesStale = true + }) + + older := &models.Sources{SourceItem: []models.SourceItem{{Source: "AUX"}}} + if conn.CompleteFieldPoll(FieldSources, firstGeneration, func(status *DeviceStatus) { + status.Sources = older + status.SourcesStale = false + }) { + t.Fatal("older successful source poll was accepted after the newer failure") } - fresh := sourceCacheStatusAt(status, readAt.Add(sourceCacheTTL-time.Nanosecond), sourceCacheTTL) - if fresh.SourcesStale { - t.Fatal("source cache became stale before its TTL elapsed") - } - - stale := sourceCacheStatusAt(status, readAt.Add(sourceCacheTTL), sourceCacheTTL) - if !stale.SourcesStale { - t.Fatal("source cache was not stale at its TTL boundary") - } - if stale.Sources != sources { - t.Fatal("stale projection did not retain the last successful source list") - } - - refreshed := *stale - refreshed.SourcesReadAt = readAt.Add(sourceCacheTTL + time.Second) - refreshed.SourcesStale = false - - got := sourceCacheStatusAt(&refreshed, refreshed.SourcesReadAt, sourceCacheTTL) - if got.SourcesStale { - t.Fatal("successful source readback did not clear staleness immediately") + status := conn.Status() + if status.Sources != retained || !status.SourcesStale { + t.Fatalf("older success cleared the newer source failure: %+v", status) } } -func TestSourceCacheWithoutSuccessfulReadIsNotStale(t *testing.T) { - status := &DeviceStatus{Sources: &models.Sources{}} +// TestSourcesStaleSurvivesAnUnrelatedFieldMerge guards the reason staleness is +// stored rather than derived per read: an unrelated field's merge copies the +// status, and must carry the marker along. +func TestSourcesStaleSurvivesAnUnrelatedFieldMerge(t *testing.T) { + conn := NewDeviceConnection(nil, nil) + conn.CompleteFieldPoll(FieldSources, conn.BeginFieldPoll(FieldSources), func(status *DeviceStatus) { + status.SourcesStale = true + }) - got := sourceCacheStatusAt(status, time.Now().Add(time.Hour), time.Nanosecond) - if got.SourcesStale { - t.Fatal("source cache without a recorded successful read was marked stale") + conn.CompleteFieldPoll(FieldVolume, conn.BeginFieldPoll(FieldVolume), func(status *DeviceStatus) { + status.Volume = &models.Volume{ActualVolume: 35} + }) + + if !conn.Status().SourcesStale { + t.Fatal("an unrelated field merge cleared the source stale marker") } } -func TestSourceCacheFailureWithoutInventoryIsExplicit(t *testing.T) { +// TestSourcesFailureWithoutInventoryIsExplicit covers the first-poll case: the +// player has no inventory at all AND cannot trust one, and the browser has to +// see that in the JSON. +func TestSourcesFailureWithoutInventoryIsExplicit(t *testing.T) { conn := NewDeviceConnection(nil, nil) conn.CompleteFieldPoll(FieldSources, conn.BeginFieldPoll(FieldSources), func(status *DeviceStatus) { status.SourcesStale = true @@ -69,45 +80,30 @@ func TestSourceCacheFailureWithoutInventoryIsExplicit(t *testing.T) { } } -// TestSourceCacheFailureFencesOlderOverlappingSuccess: a /sources read that -// FAILED must not be undone by an older, still-in-flight read that happens to -// succeed after it. Both go through CompleteFieldPoll(FieldSources, ...), so -// the failure's newer generation wins. -func TestSourceCacheFailureFencesOlderOverlappingSuccess(t *testing.T) { +// TestSourcesSuccessClearsStale is the recovery half: once a read succeeds the +// inventory is actionable again, with no TTL to wait out. +func TestSourcesSuccessClearsStale(t *testing.T) { conn := NewDeviceConnection(nil, nil) - readAt := time.Date(2026, time.August, 30, 12, 0, 0, 0, time.UTC) - retained := &models.Sources{SourceItem: []models.SourceItem{{Source: "AUX"}}} - - firstGeneration := conn.BeginFieldPoll(FieldSources) - secondGeneration := conn.BeginFieldPoll(FieldSources) - - conn.CompleteFieldPoll(FieldSources, secondGeneration, func(status *DeviceStatus) { - status.Sources = retained - status.SourcesReadAt = readAt + conn.CompleteFieldPoll(FieldSources, conn.BeginFieldPoll(FieldSources), func(status *DeviceStatus) { status.SourcesStale = true }) - older := &models.Sources{SourceItem: []models.SourceItem{{Source: "PRODUCT"}}} - if conn.CompleteFieldPoll(FieldSources, firstGeneration, func(status *DeviceStatus) { - status.Sources = older - status.SourcesStale = false - }) { - t.Fatal("older success was accepted after newer failure") - } - - if status := conn.Status(); !status.SourcesStale || status.Sources != retained { - t.Fatalf("older success cleared newer failure: %+v", status) - } - - newer := &models.Sources{SourceItem: []models.SourceItem{{Source: "BLUETOOTH"}}} - newerRead := time.Now() + fresh := &models.Sources{SourceItem: []models.SourceItem{{Source: "BLUETOOTH"}}} conn.CompleteFieldPoll(FieldSources, conn.BeginFieldPoll(FieldSources), func(status *DeviceStatus) { - status.Sources = newer - status.SourcesReadAt = newerRead + status.Sources = fresh status.SourcesStale = false }) - if status := conn.Status(); status.SourcesStale || status.Sources != newer { - t.Fatalf("newer source success did not restore actionability: %+v", status) + status := conn.Status() + if status.SourcesStale || status.Sources != fresh { + t.Fatalf("successful source readback did not restore actionability: %+v", status) + } + + encoded, err := json.Marshal(status) + if err != nil { + t.Fatalf("marshal refreshed sources: %v", err) + } + if got := string(encoded); strings.Contains(got, "sourcesStale") { + t.Fatalf("cleared stale marker should be omitted from JSON: %s", got) } } diff --git a/pkg/service/soundtouchweb/webtypes/types.go b/pkg/service/soundtouchweb/webtypes/types.go index eef637d8..3e229ffd 100644 --- a/pkg/service/soundtouchweb/webtypes/types.go +++ b/pkg/service/soundtouchweb/webtypes/types.go @@ -112,7 +112,6 @@ type DeviceStatus struct { Presets *models.Presets `json:"presets,omitempty"` Sources *models.Sources `json:"sources,omitempty"` SourcesStale bool `json:"sourcesStale,omitempty"` - SourcesReadAt time.Time `json:"-"` Bass *models.Bass `json:"bass,omitempty"` Group *models.Group `json:"group,omitempty"` Connectivity Connectivity `json:"connectivity"` @@ -152,10 +151,6 @@ const ( offlineGracePeriod = 60 * time.Second ) -// sourceCacheTTL bounds how long a successfully-read source inventory stays -// actionable without a fresh confirmation from the speaker. -const sourceCacheTTL = 30 * time.Second - // SpeakerConnectionState is the network state reported by the speaker. type SpeakerConnectionState struct { State string `json:"state"` @@ -265,20 +260,7 @@ func (c *DeviceConnection) Info() *models.DeviceInfo { // mutated. Use UpdateStatus or SetStatus to apply changes. Never returns // nil for connections built via NewDeviceConnection. func (c *DeviceConnection) Status() *DeviceStatus { - return sourceCacheStatusAt(c.status.Load(), time.Now(), sourceCacheTTL) -} - -func sourceCacheStatusAt(status *DeviceStatus, now time.Time, ttl time.Duration) *DeviceStatus { - stale := status.SourcesStale || status.Sources != nil && !status.SourcesReadAt.IsZero() && - !now.Before(status.SourcesReadAt.Add(ttl)) - if stale == status.SourcesStale { - return status - } - - next := *status - next.SourcesStale = stale - - return &next + return c.status.Load() } // Done returns a channel that is closed when the connection is removed