fix(player): mark sources stale on repeated failures, not on one

A failed /sources read went through CompleteFieldPoll, so it consumed the
field generation. That let a newer failed read discard an older, still
in-flight read that had succeeded, and since the player renders
disabled=${sourcesStale}, one transient hiccup could disable every source
button until the next fully successful poll, up to 30s later, even though a
valid inventory had just arrived.

A failure carries no inventory, so there is nothing to order and no reason
to spend the generation on it. ApplySourcesRead now splits the two:

  - a success is still fenced by generation, so two successful reads keep
    their ordering and an older one cannot overwrite a newer one, and it
    always clears the marker;
  - a failure is counted instead, and only staleSourcesFailureThreshold in
    a row marks the inventory unusable, matching how
    offlineFailureThreshold already debounces connectivity in this file.

A genuinely unreachable speaker is therefore stale one poll cycle later
than before, and a single dropped read costs nothing.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

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)

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

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)
 	}
 }

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