From 7bd2a3d9f572ee3278f4a5f6e2423d149cfa9b78 Mon Sep 17 00:00:00 2001 From: Tobias Gesellchen Date: Sat, 5 Sep 2026 19:36:46 +0200 Subject: [PATCH] perf(player): confirm a source with one light readback, not three heavy ones Selecting a source cost up to ~21 speaker requests. Two multipliers, both removed here. A match on the first readback only marked the command provisional and left the later deadlines running, so the happy path always spent all three. The reason to keep watching is real: /select answers 200 even for a source the speaker rejects seconds later, surfacing as a transition to an error source. But the event stream already reports that transition as it happens, and the effect watching nowPlayingUpdated already turns it into a failure. Polling on top is re-asking a question we are subscribed to the answer of. The remaining readbacks are now kept only when the readback itself reports no live event stream, which is the case they are actually needed for. Each readback also fetched the whole device, and HandleAPIDevice runs a full UpdateDeviceStatus: six sequential speaker calls plus /getGroup on a stereo-capable model, to answer one question, against a device the readback may be checking on precisely because it is slow. GET /devices/{id}/now-playing refreshes only /now_playing and returns the same shape, under FieldNowPlaying's generation so it still orders against push events and concurrent polls, and reporting to the health tracker like any other HTTP round. A confirmed selection on a speaker with a live event stream now costs one speaker request instead of about twenty-one. A speaker whose events are not arriving keeps the full three-readback window, at one request each. Co-Authored-By: Claude Opus 5 (1M context) --- .../testdata/router_routes.txt | 1 + .../browser_compatibility_test.go | 106 ++++++++++++++++-- pkg/service/soundtouchweb/handler.go | 53 +++++++++ pkg/service/soundtouchweb/mount.go | 4 + .../soundtouchweb/source_cache_test.go | 72 ++++++++++++ pkg/service/soundtouchweb/static/js/api.js | 3 + .../static/js/components/Sources.js | 20 +++- 7 files changed, 244 insertions(+), 15 deletions(-) diff --git a/cmd/soundtouch-service/testdata/router_routes.txt b/cmd/soundtouch-service/testdata/router_routes.txt index 1b0260a3..33b2ee3c 100644 --- a/cmd/soundtouch-service/testdata/router_routes.txt +++ b/cmd/soundtouch-service/testdata/router_routes.txt @@ -43,6 +43,7 @@ GET /api/control/devices/{id}/ soundtouch GET /api/control/devices/{id}/action/{action} soundtouchweb.(*WebApp).HandleAPIControl-fm GET /api/control/devices/{id}/library/browse soundtouchweb.(*WebApp).HandleLibraryBrowse-fm GET /api/control/devices/{id}/library/servers soundtouchweb.(*WebApp).HandleDeviceLibraryServers-fm +GET /api/control/devices/{id}/now-playing soundtouchweb.(*WebApp).HandleDeviceNowPlaying-fm GET /api/control/devices/{id}/power-status soundtouchweb.(*WebApp).HandleDevicePowerStatus-fm GET /api/control/devices/{id}/recents soundtouchweb.(*WebApp).HandleDeviceRecents-fm GET /api/control/devices/{id}/stereo-pair/ soundtouchweb.(*WebApp).HandleGetStereoPair-fm diff --git a/pkg/service/soundtouchweb/browser_compatibility_test.go b/pkg/service/soundtouchweb/browser_compatibility_test.go index f4b410ab..6bcf8a04 100644 --- a/pkg/service/soundtouchweb/browser_compatibility_test.go +++ b/pkg/service/soundtouchweb/browser_compatibility_test.go @@ -427,7 +427,7 @@ func TestSourceSelectionUsesOneWriteAndAbsoluteReadbacks(t *testing.T) { } _ = json.NewEncoder(w).Encode(webtypes.APIResponse{Success: true}) }) - r.Get("/api/control/devices/speaker", func(w http.ResponseWriter, _ *http.Request) { + r.Get("/api/control/devices/speaker/now-playing", func(w http.ResponseWriter, _ *http.Request) { mu.Lock() run := &runs[len(runs)-1] run.readTimes = append(run.readTimes, time.Since(run.startedAt)) @@ -504,6 +504,88 @@ func TestSourceSelectionUsesOneWriteAndAbsoluteReadbacks(t *testing.T) { } } +// TestSourceSelectionStopsReadbacksOnceTheEventStreamConfirms: when the +// speaker's event stream is live it will report a late rejection on its own, +// so a confirmed selection must not keep polling. This is the difference +// between one readback per source tap and three. +func TestSourceSelectionStopsReadbacksOnceTheEventStreamConfirms(t *testing.T) { + var mu sync.Mutex + reads := 0 + server := newPlayerFixtureServer(t, sourceFixtureScript, func(r chi.Router) { + r.Post("/api/control/devices/speaker/action/source", func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"success":true}`)) + }) + r.Get("/api/control/devices/speaker/now-playing", func(w http.ResponseWriter, _ *http.Request) { + mu.Lock() + reads++ + mu.Unlock() + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"success":true,"data":{"status":{"revision":9,"nowPlayingRevision":9,` + + `"webSocketConnected":true,"nowPlaying":{"Source":"AUX","SourceAccount":"AUX1"}}}}`)) + }) + }) + + ctx := newHeadlessChromeContext(t) + if err := chromedp.Run(ctx, + chromedp.Navigate(server.URL+"/fixture"), + chromedp.WaitVisible(`.source-btn`, chromedp.ByQuery), + chromedp.Click(`.source-btn:nth-child(1)`, chromedp.ByQuery), + chromedp.Poll(`document.querySelector('.source-command-status').textContent === 'Source selected'`, nil), + // Outlast the remaining readback deadlines (250ms and 500ms here). + chromedp.Sleep(900*time.Millisecond), + ); err != nil { + t.Fatalf("exercise event-stream-confirmed selection: %v", err) + } + + mu.Lock() + defer mu.Unlock() + if reads != 1 { + t.Errorf("readbacks with a live event stream = %d, want 1", reads) + } +} + +// TestSourceSelectionKeepsReadbacksWithoutAnEventStream is the other half: +// with no event stream to watch for a late rejection, the readbacks are the +// only watcher and must run to the end of their window. +func TestSourceSelectionKeepsReadbacksWithoutAnEventStream(t *testing.T) { + var mu sync.Mutex + reads := 0 + server := newPlayerFixtureServer(t, sourceFixtureScript, func(r chi.Router) { + r.Post("/api/control/devices/speaker/action/source", func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"success":true}`)) + }) + r.Get("/api/control/devices/speaker/now-playing", func(w http.ResponseWriter, _ *http.Request) { + mu.Lock() + reads++ + readCount := reads + mu.Unlock() + w.Header().Set("Content-Type", "application/json") + _, _ = fmt.Fprintf(w, `{"success":true,"data":{"status":{"revision":%d,"nowPlayingRevision":%d,`+ + `"webSocketConnected":false,"nowPlaying":{"Source":"AUX","SourceAccount":"AUX1"}}}}`, + readCount+8, readCount+8) + }) + }) + + ctx := newHeadlessChromeContext(t) + if err := chromedp.Run(ctx, + chromedp.Navigate(server.URL+"/fixture"), + chromedp.WaitVisible(`.source-btn`, chromedp.ByQuery), + chromedp.Click(`.source-btn:nth-child(1)`, chromedp.ByQuery), + chromedp.Poll(`document.querySelector('.source-command-status').textContent === 'Source selected'`, nil), + chromedp.Sleep(300*time.Millisecond), + ); err != nil { + t.Fatalf("exercise selection without an event stream: %v", err) + } + + mu.Lock() + defer mu.Unlock() + if reads != len([]int{100, 250, 500}) { + t.Errorf("readbacks without an event stream = %d, want 3", reads) + } +} + func TestSourceSelectionTreatsSelfAccountAsOmitted(t *testing.T) { const fixture = ` import { h, render } from 'preact'; @@ -533,7 +615,7 @@ render(h(Sources, { w.Header().Set("Content-Type", "application/json") _, _ = w.Write([]byte(`{"success":true}`)) }) - r.Get("/api/control/devices/speaker", func(w http.ResponseWriter, _ *http.Request) { + r.Get("/api/control/devices/speaker/now-playing", func(w http.ResponseWriter, _ *http.Request) { w.Header().Set("Content-Type", "application/json") _, _ = w.Write([]byte(`{"success":true,"data":{"status":{"revision":2,"nowPlayingRevision":2,"nowPlaying":{"Source":"AUX","SourceAccount":""}}}}`)) }) @@ -574,7 +656,7 @@ func TestSourceSelectionReadbacksDoNotWaitForSlowWriteResponse(t *testing.T) { w.Header().Set("Content-Type", "application/json") _, _ = w.Write([]byte(`{"success":true}`)) }) - r.Get("/api/control/devices/speaker", func(w http.ResponseWriter, _ *http.Request) { + r.Get("/api/control/devices/speaker/now-playing", func(w http.ResponseWriter, _ *http.Request) { mu.Lock() readTimes = append(readTimes, time.Since(startedAt)) readCount := len(readTimes) @@ -619,7 +701,7 @@ func TestSourceSelectionDefinitiveRefusalFailsImmediately(t *testing.T) { w.WriteHeader(http.StatusNotFound) _, _ = w.Write([]byte(`{"success":false,"error":"Device not found"}`)) }) - r.Get("/api/control/devices/speaker", func(w http.ResponseWriter, _ *http.Request) { + r.Get("/api/control/devices/speaker/now-playing", func(w http.ResponseWriter, _ *http.Request) { mu.Lock() reads++ mu.Unlock() @@ -663,7 +745,7 @@ func TestSourceSelectionLaterFirmwareErrorOverridesProvisionalConfirmation(t *te w.Header().Set("Content-Type", "application/json") _, _ = w.Write([]byte(`{"success":true}`)) }) - r.Get("/api/control/devices/speaker", func(w http.ResponseWriter, _ *http.Request) { + r.Get("/api/control/devices/speaker/now-playing", func(w http.ResponseWriter, _ *http.Request) { mu.Lock() reads++ read := reads @@ -714,7 +796,7 @@ func TestSourceSelectionKeepsPushConfirmationWhenReadbacksFail(t *testing.T) { _, _ = w.Write([]byte(`{"success":true}`)) }) // Every readback fails, so only the pushed status can confirm anything. - r.Get("/api/control/devices/speaker", func(w http.ResponseWriter, _ *http.Request) { + r.Get("/api/control/devices/speaker/now-playing", func(w http.ResponseWriter, _ *http.Request) { http.Error(w, "unavailable", http.StatusServiceUnavailable) }) }) @@ -748,7 +830,7 @@ func TestSourceSelectionRejectsReadbackWithOnlyNewerAggregateRevision(t *testing w.Header().Set("Content-Type", "application/json") _, _ = w.Write([]byte(`{"success":true}`)) }) - r.Get("/api/control/devices/speaker", func(w http.ResponseWriter, _ *http.Request) { + r.Get("/api/control/devices/speaker/now-playing", func(w http.ResponseWriter, _ *http.Request) { mu.Lock() reads++ mu.Unlock() @@ -782,7 +864,7 @@ func TestSourceSelectionTreatsFirmwareErrorSourceAsFailed(t *testing.T) { w.Header().Set("Content-Type", "application/json") _, _ = w.Write([]byte(`{"success":true}`)) }) - r.Get("/api/control/devices/speaker", func(w http.ResponseWriter, _ *http.Request) { + r.Get("/api/control/devices/speaker/now-playing", func(w http.ResponseWriter, _ *http.Request) { mu.Lock() reads++ read := reads @@ -829,7 +911,7 @@ func TestSourceSelectionLaterAuthoritativeSourceClearsFinalProjection(t *testing w.Header().Set("Content-Type", "application/json") _, _ = w.Write([]byte(`{"success":true}`)) }) - r.Get("/api/control/devices/speaker", func(w http.ResponseWriter, _ *http.Request) { + r.Get("/api/control/devices/speaker/now-playing", func(w http.ResponseWriter, _ *http.Request) { mu.Lock() reads++ revision := reads + 1 @@ -899,7 +981,7 @@ render(h(Fixture), document.getElementById('fixture')); w.Header().Set("Content-Type", "application/json") _, _ = w.Write([]byte(`{"success":true}`)) }) - r.Get("/api/control/devices/speaker", func(w http.ResponseWriter, _ *http.Request) { + r.Get("/api/control/devices/speaker/now-playing", func(w http.ResponseWriter, _ *http.Request) { mu.Lock() reads++ revision := reads + 1 @@ -948,7 +1030,7 @@ func TestSourceSelectionResetsWhenDeviceChanges(t *testing.T) { w.Header().Set("Content-Type", "application/json") _, _ = w.Write([]byte(`{"success":true}`)) }) - r.Get("/api/control/devices/speaker", func(w http.ResponseWriter, _ *http.Request) { + r.Get("/api/control/devices/speaker/now-playing", func(w http.ResponseWriter, _ *http.Request) { w.Header().Set("Content-Type", "application/json") _, _ = w.Write([]byte(`{"success":true}`)) }) @@ -990,7 +1072,7 @@ func TestSourceSelectionFencesOlderReadbackAndStatus(t *testing.T) { w.Header().Set("Content-Type", "application/json") _ = json.NewEncoder(w).Encode(webtypes.APIResponse{Success: true}) }) - r.Get("/api/control/devices/speaker", func(w http.ResponseWriter, _ *http.Request) { + r.Get("/api/control/devices/speaker/now-playing", func(w http.ResponseWriter, _ *http.Request) { mu.Lock() source := currentSource readRevision++ diff --git a/pkg/service/soundtouchweb/handler.go b/pkg/service/soundtouchweb/handler.go index 53ceae77..eec19d0c 100644 --- a/pkg/service/soundtouchweb/handler.go +++ b/pkg/service/soundtouchweb/handler.go @@ -411,6 +411,59 @@ func (app *WebApp) HandleAPIDevices(w http.ResponseWriter, _ *http.Request) { } } +// HandleDeviceNowPlaying refreshes ONLY /now_playing and returns the device in +// the same shape as HandleAPIDevice. +// +// It exists for the player's source-selection readback, which needs one +// question answered ("what is the speaker playing now?") and nothing else. +// Going through HandleAPIDevice for that costs a full UpdateDeviceStatus, six +// sequential speaker calls plus /getGroup on a stereo-capable model, against a +// device the readback may well be checking on precisely because it is slow. +// +// The refresh runs under FieldNowPlaying's generation, so it orders against +// push events and concurrent polls exactly like any other now-playing write, +// and it reports its outcome to the health tracker like any other HTTP round. +func (app *WebApp) HandleDeviceNowPlaying(w http.ResponseWriter, r *http.Request) { + deviceID := chi.URLParam(r, "id") + if deviceID == "" { + app.sendError(w, "Device ID required", http.StatusBadRequest) + return + } + + device, exists := app.GetDevice(deviceID) + if !exists { + app.sendError(w, "Device not found", http.StatusNotFound) + return + } + + if device.Client != nil { + generation := device.BeginFieldPoll(webtypes.FieldNowPlaying) + pollGeneration := device.BeginHTTPPoll() + + nowPlaying, err := device.Client.GetNowPlaying() + if err == nil { + device.CompleteFieldPoll(webtypes.FieldNowPlaying, generation, func(status *webtypes.DeviceStatus) { + status.NowPlaying = nowPlaying + status.LastActivity = time.Now() + }) + } + + device.CompleteHTTPPoll(pollGeneration, err == nil, time.Now(), nil) + } + + view, visible := app.deviceViewForID(deviceID) + if !visible { + app.sendError(w, "Device not found", http.StatusNotFound) + return + } + + w.Header().Set("Content-Type", "application/json") + + if err := json.NewEncoder(w).Encode(webtypes.APIResponse{Success: true, Data: view}); err != nil { + http.Error(w, "Failed to encode response", http.StatusInternalServerError) + } +} + // HandleAPIDevice returns a specific device as JSON func (app *WebApp) HandleAPIDevice(w http.ResponseWriter, r *http.Request) { deviceID := chi.URLParam(r, "id") diff --git a/pkg/service/soundtouchweb/mount.go b/pkg/service/soundtouchweb/mount.go index 7cc185f2..a50c73b7 100644 --- a/pkg/service/soundtouchweb/mount.go +++ b/pkg/service/soundtouchweb/mount.go @@ -69,6 +69,10 @@ func (app *WebApp) MountWeb(r chi.Router, discoveryService *discovery.UnifiedDis r.Post("/power", app.HandleDevicePower) r.Get("/power-status", app.HandleDevicePowerStatus) r.Get("/recents", app.HandleDeviceRecents) + // Now-playing-only refresh. The player's source-selection + // readback uses this instead of the full device fetch, which + // would poll every field to answer one question. + r.Get("/now-playing", app.HandleDeviceNowPlaying) // Low-level "play this ContentItem" primitive (not a provider). r.Post("/play", app.HandleDevicePlay) // Generic key / preset / source / bass actions. Source selection is diff --git a/pkg/service/soundtouchweb/source_cache_test.go b/pkg/service/soundtouchweb/source_cache_test.go index 49240b8d..ab6a90ec 100644 --- a/pkg/service/soundtouchweb/source_cache_test.go +++ b/pkg/service/soundtouchweb/source_cache_test.go @@ -4,6 +4,8 @@ import ( "encoding/json" "net/http" "net/http/httptest" + "reflect" + "sync" "testing" "github.com/gesellix/bose-soundtouch/pkg/client" @@ -169,3 +171,73 @@ func TestHandleAPIDevicePublishesCanonicalReadback(t *testing.T) { t.Fatalf("response did not publish canonical status: response=%+v status=%+v", response.Data.Status, canonical) } } + +// TestHandleDeviceNowPlayingPollsOnlyNowPlaying is the point of the endpoint: +// the source-selection readback needs one question answered, and going +// through the full device fetch would poll every field to answer it. +func TestHandleDeviceNowPlayingPollsOnlyNowPlaying(t *testing.T) { + var mu sync.Mutex + var paths []string + speaker := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + mu.Lock() + paths = append(paths, r.URL.Path) + mu.Unlock() + + if r.URL.Path != "/now_playing" { + http.NotFound(w, r) + + return + } + + w.Header().Set("Content-Type", "application/xml") + _, _ = w.Write([]byte(`PLAY_STATE`)) + })) + defer speaker.Close() + + app := NewWebApp() + conn := webtypes.NewDeviceConnection( + client.NewClient(&client.Config{Host: speaker.URL}), + &models.DeviceInfo{Name: "Speaker"}, + ) + conn.SetStatus(&webtypes.DeviceStatus{NowPlaying: &models.NowPlaying{Source: "STANDBY"}}) + baseline := conn.Status().NowPlayingRevision + app.AddDevice("speaker", conn) + + req := httptest.NewRequest(http.MethodGet, "/api/control/devices/speaker/now-playing", nil) + req = withChiParams(req, map[string]string{"id": "speaker"}) + w := httptest.NewRecorder() + app.HandleDeviceNowPlaying(w, req) + + if w.Code != http.StatusOK { + t.Fatalf("now-playing readback = %d: %s", w.Code, w.Body.String()) + } + + mu.Lock() + defer mu.Unlock() + if want := []string{"/now_playing"}; !reflect.DeepEqual(paths, want) { + t.Fatalf("speaker requests = %v, want only %v", paths, want) + } + + var response struct { + Success bool `json:"success"` + Data struct { + Status webtypes.DeviceStatus `json:"status"` + } `json:"data"` + } + if err := json.NewDecoder(w.Body).Decode(&response); err != nil { + t.Fatalf("decode now-playing readback: %v", err) + } + + canonical := conn.Status() + if !response.Success || canonical.NowPlaying.Source != "AUX" { + t.Fatalf("now-playing was not merged: response=%+v status=%+v", response, canonical) + } + // The readback confirms a source by comparing this revision, so it has to + // carry the same values the connection now holds. + if response.Data.Status.NowPlayingRevision != canonical.NowPlayingRevision || + response.Data.Status.Revision != canonical.Revision || + canonical.NowPlayingRevision <= baseline { + t.Fatalf("response did not publish canonical revisions: response=%+v status=%+v", + response.Data.Status, canonical) + } +} diff --git a/pkg/service/soundtouchweb/static/js/api.js b/pkg/service/soundtouchweb/static/js/api.js index 59a8bd82..a9c5d896 100644 --- a/pkg/service/soundtouchweb/static/js/api.js +++ b/pkg/service/soundtouchweb/static/js/api.js @@ -39,6 +39,9 @@ async function checkedReq(url, opts = {}) { export const api = { devices: () => req('/api/control/devices'), device: (id) => req(`/api/control/devices/${id}`), + // Refreshes only /now_playing. Used by the source-selection readback, + // which would otherwise poll every field to answer one question. + deviceNowPlaying: (id) => req(`/api/control/devices/${id}/now-playing`), removeDevice: (id) => req(`/api/control/devices/${id}`, { method: 'DELETE' }), discover: () => req('/api/control/discover', { method: 'POST' }), key: (id, key) => req(`/api/control/devices/${id}/key/${key}`, { method: 'POST' }), diff --git a/pkg/service/soundtouchweb/static/js/components/Sources.js b/pkg/service/soundtouchweb/static/js/components/Sources.js index 23ac8c7d..eb305889 100644 --- a/pkg/service/soundtouchweb/static/js/components/Sources.js +++ b/pkg/service/soundtouchweb/static/js/components/Sources.js @@ -152,7 +152,7 @@ export function Sources({ active.latestReadback = index; try { - const response = await api.device(deviceId); + const response = await api.deviceNowPlaying(deviceId); if (commandRef.current.active !== active || active.latestReadback !== index) return; const readbackStatus = response?.data?.status; @@ -174,15 +174,29 @@ export function Sources({ }); } else if (revisionIsNewer && nowPlaying?.Source === target.source && sourceAccountsMatch(target.source, nowPlaying?.SourceAccount, target.account)) { + // A match is not the end of the story: /select answers + // 200 even for a source the speaker goes on to reject a + // few seconds later, which surfaces as a transition to + // an error source. Something has to keep watching. + // + // The event stream is the better watcher when it is + // live: nowPlayingUpdated reports that transition as it + // happens, and the effect above already turns it into a + // failure. Polling on top of that only re-asks a + // question we are already subscribed to the answer of. + // So keep the remaining readbacks only as a fallback + // for a device whose events we are not receiving. const isFinalReadback = index === readbackDelays.length - 1; + const eventStreamWatching = readbackStatus?.webSocketConnected === true; + const settled = isFinalReadback || eventStreamWatching; setCommand({ ...target, generation, - outcome: isFinalReadback ? 'final-confirmed' : 'provisional-confirmed', + outcome: settled ? 'final-confirmed' : 'provisional-confirmed', startNowPlayingRevision: nowPlayingRevision, confirmedRevision: readbackRevision, }); - if (isFinalReadback) { + if (settled) { clearReadbacks(); commandRef.current.active = null; }