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