diff --git a/pkg/service/soundtouchweb/source_cache_test.go b/pkg/service/soundtouchweb/source_cache_test.go index 19dee1e0..49240b8d 100644 --- a/pkg/service/soundtouchweb/source_cache_test.go +++ b/pkg/service/soundtouchweb/source_cache_test.go @@ -42,11 +42,12 @@ func TestUpdateDeviceStatusDoesNotRefreshNowPlayingRevisionOnFailure(t *testing. } } -// 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) { +// TestUpdateDeviceStatusMarksSourcesStaleOnRepeatedFailedReads: unlike every +// other field, a failed /sources read is still recorded. The last known +// inventory stays visible but goes unusable once reads keep failing, because +// offering source buttons the speaker no longer confirms is worse than +// offering none. +func TestUpdateDeviceStatusMarksSourcesStaleOnRepeatedFailedReads(t *testing.T) { sourcesOK := true speaker := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { if r.URL.Path == "/sources" { @@ -77,12 +78,20 @@ func TestUpdateDeviceStatusMarksSourcesStaleOnFailedRead(t *testing.T) { t.Fatalf("successful source read was not merged as actionable: %+v", fresh) } + // One failure is not enough: a single transient hiccup must not disable + // the whole source list. sourcesOK = false app.UpdateDeviceStatus("speaker", conn) + if single := conn.Status(); single.SourcesStale { + t.Fatalf("one failed source read marked the inventory stale: %+v", single) + } + + app.UpdateDeviceStatus("speaker", conn) + stale := conn.Status() if !stale.SourcesStale { - t.Fatalf("failed source read did not mark the inventory stale: %+v", stale) + t.Fatalf("consecutive failed source reads 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) diff --git a/pkg/service/soundtouchweb/websocket.go b/pkg/service/soundtouchweb/websocket.go index 1ed174ca..80bc51da 100644 --- a/pkg/service/soundtouchweb/websocket.go +++ b/pkg/service/soundtouchweb/websocket.go @@ -799,24 +799,11 @@ func (app *WebApp) updateDeviceStatus(_ string, conn *webtypes.DeviceConnection, anyFetchSucceeded = true } - // Unlike the other fields, a FAILED /sources read is merged too. The + // Unlike the other fields, a FAILED /sources read is recorded 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() - }) + // a list the speaker no longer confirms is worse than offering none. See + // ApplySourcesRead for why a failure is counted rather than fenced. + conn.ApplySourcesRead(sourcesGen, sources, sourcesErr) if bassErr == nil { anyFetchSucceeded = true diff --git a/pkg/service/soundtouchweb/webtypes/source_cache_test.go b/pkg/service/soundtouchweb/webtypes/source_cache_test.go index 361405d5..7c4ba5f0 100644 --- a/pkg/service/soundtouchweb/webtypes/source_cache_test.go +++ b/pkg/service/soundtouchweb/webtypes/source_cache_test.go @@ -2,108 +2,135 @@ package webtypes import ( "encoding/json" + "errors" "strings" "testing" "github.com/gesellix/bose-soundtouch/pkg/models" ) -// 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) { +// TestSourcesSurviveASingleFailedRead: one failed /sources read is not +// evidence the inventory is wrong. Marking it stale immediately would disable +// every source button in the player on a transient hiccup. +func TestSourcesSurviveASingleFailedRead(t *testing.T) { conn := NewDeviceConnection(nil, nil) - retained := &models.Sources{SourceItem: []models.SourceItem{{Source: "PRODUCT"}}} + inventory := &models.Sources{SourceItem: []models.SourceItem{{Source: "AUX"}}} - firstGeneration := conn.BeginFieldPoll(FieldSources) - secondGeneration := conn.BeginFieldPoll(FieldSources) + conn.ApplySourcesRead(conn.BeginFieldPoll(FieldSources), inventory, nil) + conn.ApplySourcesRead(conn.BeginFieldPoll(FieldSources), nil, errors.New("temporary read failure")) - conn.CompleteFieldPoll(FieldSources, secondGeneration, func(status *DeviceStatus) { - status.Sources = retained - status.SourcesStale = true - }) + status := conn.Status() + if status.SourcesStale { + t.Fatalf("one failed read marked the inventory stale: %+v", status) + } + if status.Sources != inventory { + t.Fatalf("failed read discarded the inventory: %+v", status) + } +} - 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") +func TestSourcesGoStaleAfterConsecutiveFailedReads(t *testing.T) { + conn := NewDeviceConnection(nil, nil) + inventory := &models.Sources{SourceItem: []models.SourceItem{{Source: "AUX"}}} + conn.ApplySourcesRead(conn.BeginFieldPoll(FieldSources), inventory, nil) + + for range staleSourcesFailureThreshold { + conn.ApplySourcesRead(conn.BeginFieldPoll(FieldSources), nil, errors.New("read failure")) } status := conn.Status() - if status.Sources != retained || !status.SourcesStale { - t.Fatalf("older success cleared the newer source failure: %+v", status) + if !status.SourcesStale { + t.Fatalf("inventory not marked stale after %d failed reads: %+v", staleSourcesFailureThreshold, status) + } + // Kept visible, just not actionable: the player still shows the list. + if status.Sources != inventory { + t.Fatalf("stale marking discarded the last known inventory: %+v", status) } } -// 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) { +// TestSourcesSuccessClearsStaleAndResetsTheCount: a success is the strongest +// evidence available that the list can be acted on again. +func TestSourcesSuccessClearsStaleAndResetsTheCount(t *testing.T) { conn := NewDeviceConnection(nil, nil) - conn.CompleteFieldPoll(FieldSources, conn.BeginFieldPoll(FieldSources), func(status *DeviceStatus) { - status.SourcesStale = true - }) - - conn.CompleteFieldPoll(FieldVolume, conn.BeginFieldPoll(FieldVolume), func(status *DeviceStatus) { - status.Volume = &models.Volume{ActualVolume: 35} - }) - + for range staleSourcesFailureThreshold { + conn.ApplySourcesRead(conn.BeginFieldPoll(FieldSources), nil, errors.New("read failure")) + } if !conn.Status().SourcesStale { - t.Fatal("an unrelated field merge cleared the source stale marker") + t.Fatal("precondition: inventory should be stale") + } + + fresh := &models.Sources{SourceItem: []models.SourceItem{{Source: "BLUETOOTH"}}} + conn.ApplySourcesRead(conn.BeginFieldPoll(FieldSources), fresh, nil) + + if status := conn.Status(); status.SourcesStale || status.Sources != fresh { + t.Fatalf("successful read did not restore actionability: %+v", status) + } + + // The count reset too, so the next single failure must not re-mark stale. + conn.ApplySourcesRead(conn.BeginFieldPoll(FieldSources), nil, errors.New("read failure")) + + if status := conn.Status(); status.SourcesStale { + t.Fatalf("failure count was not reset by the successful read: %+v", status) } } -// 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) { +// TestSourcesFailureDoesNotDiscardConcurrentSuccess is the finding this +// design answers: a failed read carries no inventory to order, so it must not +// consume the field generation and throw away a slower, successful read. +func TestSourcesFailureDoesNotDiscardConcurrentSuccess(t *testing.T) { conn := NewDeviceConnection(nil, nil) - conn.CompleteFieldPoll(FieldSources, conn.BeginFieldPoll(FieldSources), func(status *DeviceStatus) { - status.SourcesStale = true - }) + + slowSuccess := conn.BeginFieldPoll(FieldSources) + fastFailure := conn.BeginFieldPoll(FieldSources) + + conn.ApplySourcesRead(fastFailure, nil, errors.New("read failure")) + + inventory := &models.Sources{SourceItem: []models.SourceItem{{Source: "AUX"}}} + if !conn.ApplySourcesRead(slowSuccess, inventory, nil) { + t.Fatal("successful read was discarded by a concurrent failure") + } + + if status := conn.Status(); status.SourcesStale || status.Sources != inventory { + t.Fatalf("concurrent failure suppressed a successful read: %+v", status) + } +} + +// TestSourcesOlderSuccessCannotOverwriteNewer keeps the ordering that does +// still matter: two successful reads are ordered by generation. +func TestSourcesOlderSuccessCannotOverwriteNewer(t *testing.T) { + conn := NewDeviceConnection(nil, nil) + + older := conn.BeginFieldPoll(FieldSources) + newer := conn.BeginFieldPoll(FieldSources) + + current := &models.Sources{SourceItem: []models.SourceItem{{Source: "BLUETOOTH"}}} + conn.ApplySourcesRead(newer, current, nil) + + stale := &models.Sources{SourceItem: []models.SourceItem{{Source: "AUX"}}} + if conn.ApplySourcesRead(older, stale, nil) { + t.Fatal("older successful read was accepted after a newer one") + } + + if status := conn.Status(); status.Sources != current { + t.Fatalf("older read overwrote a newer inventory: %+v", status) + } +} + +func TestStaleSourcesWithoutInventoryIsExplicitInJSON(t *testing.T) { + conn := NewDeviceConnection(nil, nil) + for range staleSourcesFailureThreshold { + conn.ApplySourcesRead(conn.BeginFieldPoll(FieldSources), nil, errors.New("read failure")) + } status := conn.Status() if status.Sources != nil || !status.SourcesStale { - t.Fatalf("initial source failure was not represented without inventory: %+v", status) + t.Fatalf("initial source failures not represented without inventory: %+v", status) } encoded, err := json.Marshal(status) if err != nil { - t.Fatalf("marshal initial source failure: %v", err) + t.Fatalf("marshal stale sources: %v", err) } if got := string(encoded); !strings.Contains(got, `"sourcesStale":true`) { - t.Fatalf("initial source failure omitted stale state: %s", got) - } -} - -// 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) - conn.CompleteFieldPoll(FieldSources, conn.BeginFieldPoll(FieldSources), func(status *DeviceStatus) { - status.SourcesStale = true - }) - - fresh := &models.Sources{SourceItem: []models.SourceItem{{Source: "BLUETOOTH"}}} - conn.CompleteFieldPoll(FieldSources, conn.BeginFieldPoll(FieldSources), func(status *DeviceStatus) { - status.Sources = fresh - status.SourcesStale = false - }) - - 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) + t.Fatalf("stale state missing from JSON: %s", got) } } diff --git a/pkg/service/soundtouchweb/webtypes/types.go b/pkg/service/soundtouchweb/webtypes/types.go index d1979ffb..53b1eaa0 100644 --- a/pkg/service/soundtouchweb/webtypes/types.go +++ b/pkg/service/soundtouchweb/webtypes/types.go @@ -79,6 +79,11 @@ type DeviceConnection struct { speakerConnectionConnected bool speakerConnectionObserved time.Time + // sourcesFailuresMu guards consecutiveSourcesFailures, the count of + // /sources reads that have failed in a row. See ApplySourcesRead. + sourcesFailuresMu sync.Mutex + consecutiveSourcesFailures int + // fieldGenMu guards fieldGen, the per-field generation ordering used by // BeginFieldPoll/CompleteFieldPoll/ApplyFieldEvent. Each StatusField gets // its own (issued, applied) pair so an event or a poll completion for @@ -158,6 +163,13 @@ const ( offlineGracePeriod = 60 * time.Second ) +// staleSourcesFailureThreshold is how many /sources reads must fail in a row +// before the player stops offering the inventory. One failed read is not +// evidence the list is wrong, and marking it stale immediately would disable +// every source button on a single transient hiccup. Mirrors +// offlineFailureThreshold's reasoning for connectivity. +const staleSourcesFailureThreshold = 2 + // SpeakerConnectionState is the network state reported by the speaker. type SpeakerConnectionState struct { State string `json:"state"` @@ -485,6 +497,45 @@ func recordFieldRevision(status *DeviceStatus, field StatusField, generation uin } } +// ApplySourcesRead records the outcome of one /sources read. +// +// A successful read is merged under the field's generation ordering, so an +// older in-flight read cannot overwrite a newer one, and it always clears the +// stale marker: having just received an inventory from the speaker is the +// strongest evidence available that the list can be acted on. +// +// A failure is deliberately NOT fenced by generation. It carries no inventory +// to order, and gating it on the generation would let a single failed read +// discard a concurrent successful one, disabling every source button until the +// next fully successful poll. Instead failures are counted, and only +// staleSourcesFailureThreshold of them in a row marks the inventory unusable. +func (c *DeviceConnection) ApplySourcesRead(generation uint64, sources *models.Sources, err error) bool { + c.sourcesFailuresMu.Lock() + + if err == nil { + c.consecutiveSourcesFailures = 0 + } else { + c.consecutiveSourcesFailures++ + } + + stale := c.consecutiveSourcesFailures >= staleSourcesFailureThreshold + c.sourcesFailuresMu.Unlock() + + if err != nil { + c.UpdateStatus(func(status *DeviceStatus) { + status.SourcesStale = stale + }) + + return false + } + + return c.CompleteFieldPoll(FieldSources, generation, func(status *DeviceStatus) { + status.Sources = sources + status.SourcesStale = stale + status.LastActivity = time.Now() + }) +} + // UpdateStatus atomically applies mut to a copy of the current status // and stores the result. If another goroutine updates the status while // mut runs, UpdateStatus retries with the newer status — so concurrent