From a4177ec6f2a32a7e897387b4a2c1f1386bde7a76 Mon Sep 17 00:00:00 2001 From: Tobias Gesellchen Date: Sat, 5 Sep 2026 19:10:07 +0200 Subject: [PATCH] fix(player): keep verifying a source write unless the refusal is definitive Selecting a source posts once and then confirms by bounded readback. When the POST itself failed, the write error was captured into active.writeError and never surfaced: a rejected command reported nothing for the full 10s readback window and then a bare "Source selection unverified". Surfacing it needs a distinction the API layer did not make. checkedReq collapsed every failure into one Error, but the two cases differ: - 4xx: every 4xx on these endpoints is produced before AfterTouch calls the speaker (unknown device, unparseable body, empty source, unknown action), so the command provably never went out. Nothing can confirm it; report the failure at once, with the server's reason. - 5xx and transport errors: handleSourceControl reports a failed Client.SelectSource through sendControlResponse, which maps any speaker-call error to 500. A request that timed out after the speaker already switched is indistinguishable from one it never received, so the readbacks must keep running and the reason is carried into whatever outcome they reach. checkedReq now tags thrown errors with `definitive`, and only a definitive refusal cancels the readbacks. Outcome text appends the reason when there is one, so a firmware rejection also names the error source it saw. Co-Authored-By: Claude Opus 5 (1M context) --- .../browser_compatibility_test.go | 58 ++++++++++++++++++- .../soundtouchweb/frontend_test/api.test.mjs | 38 +++++++++--- pkg/service/soundtouchweb/static/js/api.js | 21 ++++++- .../static/js/components/Sources.js | 28 ++++++++- 4 files changed, 130 insertions(+), 15 deletions(-) diff --git a/pkg/service/soundtouchweb/browser_compatibility_test.go b/pkg/service/soundtouchweb/browser_compatibility_test.go index ac1c3253..54c4a5a5 100644 --- a/pkg/service/soundtouchweb/browser_compatibility_test.go +++ b/pkg/service/soundtouchweb/browser_compatibility_test.go @@ -370,6 +370,10 @@ func TestSourceSelectionUsesOneWriteAndAbsoluteReadbacks(t *testing.T) { w.Header().Set("Content-Type", "application/json") if body.Source == "SPOTIFY" { + // 500 is how a failed Client.SelectSource surfaces. It does not + // prove the speaker ignored the command, so the readbacks below + // must still run. + w.WriteHeader(http.StatusInternalServerError) _ = json.NewEncoder(w).Encode(webtypes.APIResponse{Success: false, Error: "source rejected"}) return } @@ -406,6 +410,7 @@ func TestSourceSelectionUsesOneWriteAndAbsoluteReadbacks(t *testing.T) { chromedp.Poll(`document.querySelector('.source-command-status').textContent === 'Source selection unverified'`, nil), chromedp.Click(`.source-btn:nth-child(3)`, chromedp.ByQuery), chromedp.Poll(`document.querySelector('.source-btn:nth-child(3)').classList.contains('unverified')`, nil), + chromedp.Poll(`document.querySelector('.source-command-status').textContent === 'Source selection unverified: source rejected'`, nil), ); err != nil { t.Fatalf("exercise source commands: %v", err) } @@ -553,6 +558,55 @@ func TestSourceSelectionReadbacksDoNotWaitForSlowWriteResponse(t *testing.T) { } } +// TestSourceSelectionDefinitiveRefusalFailsImmediately: a 4xx is produced +// before AfterTouch ever calls the speaker, so the command provably never went +// out. There is nothing for the readbacks to confirm; report it at once, with +// the server's reason, instead of polling for the full readback window. +func TestSourceSelectionDefinitiveRefusalFailsImmediately(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.WriteHeader(http.StatusNotFound) + _, _ = w.Write([]byte(`{"success":false,"error":"Device not found"}`)) + }) + r.Get("/api/control/devices/speaker", 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":2,"nowPlayingRevision":2,"nowPlaying":{"Source":"STANDBY","SourceAccount":""}}}}`)) + }) + }) + + ctx := newHeadlessChromeContext(t) + var statusText string + 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-btn').classList.contains('failed')`, nil), + chromedp.Text(`.source-command-status`, &statusText, chromedp.ByQuery), + // Well past every readback deadline in this fixture (100/250/500ms), so + // a zero read count means the failure came from the write itself. + chromedp.Sleep(700*time.Millisecond), + ); err != nil { + t.Fatalf("exercise definitively refused source write: %v", err) + } + + if !strings.Contains(statusText, "Source selection failed") || + !strings.Contains(statusText, "Device not found") { + t.Errorf("status = %q, want the failure and the server's reason", statusText) + } + + mu.Lock() + defer mu.Unlock() + if reads != 0 { + t.Errorf("readbacks after a definitive refusal = %d, want 0", reads) + } +} + func TestSourceSelectionLaterFirmwareErrorOverridesProvisionalConfirmation(t *testing.T) { var mu sync.Mutex reads := 0 @@ -586,7 +640,7 @@ func TestSourceSelectionLaterFirmwareErrorOverridesProvisionalConfirmation(t *te chromedp.Click(`.source-btn:nth-child(1)`, chromedp.ByQuery), chromedp.Poll(`document.querySelector('.source-command-status').textContent === 'Source selected, confirming'`, nil), chromedp.Evaluate(`document.querySelector('.source-btn:nth-child(1)').classList.contains('provisional-confirmed')`, &provisional), - chromedp.Poll(`document.querySelector('.source-command-status').textContent === 'Source selection failed'`, nil), + chromedp.Poll(`document.querySelector('.source-command-status').textContent === 'Source selection failed: INVALID_SOURCE'`, nil), ); err != nil { t.Fatalf("exercise provisional source rejection: %v", err) } @@ -658,7 +712,7 @@ func TestSourceSelectionTreatsFirmwareErrorSourceAsFailed(t *testing.T) { 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 selection failed'`, nil), + chromedp.Poll(`document.querySelector('.source-command-status').textContent === 'Source selection failed: INVALID_SOURCE'`, nil), ); err != nil { t.Fatalf("exercise firmware source rejection: %v", err) } diff --git a/pkg/service/soundtouchweb/frontend_test/api.test.mjs b/pkg/service/soundtouchweb/frontend_test/api.test.mjs index 99b43a37..1ed3a8ba 100644 --- a/pkg/service/soundtouchweb/frontend_test/api.test.mjs +++ b/pkg/service/soundtouchweb/frontend_test/api.test.mjs @@ -23,22 +23,44 @@ test('selectSource posts the exact source and account body', async () => { assert.deepEqual(JSON.parse(requests[1].options.body), { source: 'PRODUCT', account: '' }); }); -test('source selection rejects non-2xx responses', async () => { - globalThis.fetch = async () => new Response('{"success":false,"error":"offline"}', { - status: 503, +// A 4xx is produced before any speaker call (unknown device, unparseable +// body, empty source), so the command provably never went out. +test('source selection reports a 4xx as definitive', async () => { + globalThis.fetch = async () => new Response('{"success":false,"error":"Device not found"}', { + status: 404, headers: { 'Content-Type': 'application/json' }, }); - await assert.rejects(api.selectSource('speaker', 'AUX', 'AUX1'), /offline/); + await assert.rejects(api.selectSource('speaker', 'AUX', 'AUX1'), error => { + assert.match(error.message, /Device not found/); + assert.equal(error.definitive, true); + return true; + }); }); -test('source selection rejects application failures on 2xx responses', async () => { - globalThis.fetch = async () => new Response('{"success":false,"error":"rejected"}', { - status: 200, +// A 5xx is how a failed Client.SelectSource surfaces, and a request that timed +// out after the speaker already switched is indistinguishable from one it never +// received. The caller must keep verifying by readback. +test('source selection reports a 5xx as non-definitive', async () => { + globalThis.fetch = async () => new Response('{"success":false,"error":"speaker unreachable"}', { + status: 500, headers: { 'Content-Type': 'application/json' }, }); - await assert.rejects(api.selectSource('speaker', 'AUX', 'AUX1'), /rejected/); + await assert.rejects(api.selectSource('speaker', 'AUX', 'AUX1'), error => { + assert.match(error.message, /speaker unreachable/); + assert.equal(error.definitive, false); + return true; + }); +}); + +test('source selection reports an unreadable body as non-definitive on 5xx', async () => { + globalThis.fetch = async () => new Response('gateway timeout', { status: 504 }); + + await assert.rejects(api.selectSource('speaker', 'AUX', 'AUX1'), error => { + assert.equal(error.definitive, false); + return true; + }); }); test('legacy API consumers retain response-level error handling', async () => { diff --git a/pkg/service/soundtouchweb/static/js/api.js b/pkg/service/soundtouchweb/static/js/api.js index 747b075c..59a8bd82 100644 --- a/pkg/service/soundtouchweb/static/js/api.js +++ b/pkg/service/soundtouchweb/static/js/api.js @@ -5,16 +5,33 @@ async function req(url, opts = {}) { return r.json(); } +// checkedReq turns a failed request into a thrown Error, tagging whether the +// failure is DEFINITIVE, meaning proof the command never reached the speaker. +// +// Only 4xx qualifies. Every 4xx on the control endpoints is produced before +// any speaker call (missing/unknown device, unparseable body, empty source, +// unknown action), so nothing was sent onward. A 5xx is NOT proof of anything: +// handleSourceControl reports a failed Client.SelectSource through +// sendControlResponse, which maps any speaker-call error to 500, and a request +// that timed out after the speaker already switched looks exactly like one it +// never received. Transport errors are ambiguous for the same reason. +// +// Callers that verify by readback must keep verifying unless the failure is +// definitive. async function checkedReq(url, opts = {}) { const r = await fetch(url, opts); + const definitive = r.status >= 400 && r.status < 500; let response; try { response = await r.json(); } catch (_) { - throw new Error(`Request failed (${r.status})`); + throw Object.assign(new Error(`Request failed (${r.status})`), { definitive }); } if (!r.ok || response?.success === false) { - throw new Error(response?.error || `Request failed (${r.status})`); + throw Object.assign( + new Error(response?.error || `Request failed (${r.status})`), + { definitive }, + ); } return response; } diff --git a/pkg/service/soundtouchweb/static/js/components/Sources.js b/pkg/service/soundtouchweb/static/js/components/Sources.js index e75d269f..015a6133 100644 --- a/pkg/service/soundtouchweb/static/js/components/Sources.js +++ b/pkg/service/soundtouchweb/static/js/components/Sources.js @@ -190,7 +190,23 @@ export function Sources({ try { await api.selectSource(deviceId, target.source, target.account); } catch (error) { - if (commandRef.current.active === active) active.writeError = error; + if (commandRef.current.active !== active) return; + // A definitive refusal (4xx) means the speaker never saw the + // command, so there is nothing for the readbacks to confirm and + // reporting it now beats waiting out the readback window. Anything + // else stays pending: a 5xx or a transport error does not tell us + // whether the speaker acted, so we keep verifying and carry the + // reason into whatever outcome the readbacks reach. + if (!error?.definitive) { + active.writeError = error; + + return; + } + clearReadbacks(); + commandRef.current.active = null; + setCommand(previous => previous?.generation === generation + ? { ...previous, outcome: 'failed', error: error?.message } + : previous); } } @@ -209,6 +225,12 @@ export function Sources({ failed: 'Source selection failed', }; + function commandMessage(cmd) { + if (!cmd) return ''; + const text = outcomeText[cmd.outcome]; + return cmd.error ? `${text}: ${cmd.error}` : text; + } + return html`

Sources

@@ -225,7 +247,7 @@ export function Sources({ class="source-btn ${isActive ? 'active' : ''} ${src.IsLocal ? 'local' : ''} ${outcome}" onClick=${() => select(src)} disabled=${sourcesStale} - title=${availabilityMessage || (outcome ? outcomeText[outcome] : src.Source)} + title=${availabilityMessage || (outcome ? commandMessage(command) : src.Source)} aria-describedby=${availabilityId} aria-busy=${outcome === 'pending' || outcome === 'provisional-confirmed' ? 'true' : null} > @@ -241,7 +263,7 @@ export function Sources({ role="status" aria-live="polite" > - ${availabilityMessage || (command ? outcomeText[command.outcome] : '')} + ${availabilityMessage || commandMessage(command)}
`;