From 66eedeefd38cca9f885a468216aa00d6734bca46 Mon Sep 17 00:00:00 2001 From: Tobias Gesellchen Date: Sat, 5 Sep 2026 12:18:01 +0200 Subject: [PATCH] fix(stereo): stop pushing to external Marge, make its preflight read advisory A speaker's own firmware self-reports its group create/rename/teardown to whatever Marge backend it's configured with -- that's the entire reason HandleMargeAddGroup/HandleMargeModifyGroup/HandleMargeDeleteGroup exist, they're only ever called by speakers, never by us. Coordinator cleanup/rename proactively pushing the same update to an external (non-local) Marge target duplicated that self-report against a backend we generally can't authenticate to anyway (real Bose cloud, another instance, ...), for zero benefit. The one part of the external path with a real, distinct purpose -- preflight's read-only check for a dangling stale generation before a new Create -- stays, but its failure (network error, wrong credentials, an unreachable backend) no longer blocks Create. It's a best-effort safety net on top of the coordinator's own physical preflight (capability/zone/reachability checks against the live speakers), not the primary guard, and the live speakers' own state is the authoritative signal either way. Co-Authored-By: Claude Sonnet 5 --- cmd/soundtouch-service/main.go | 20 +++- .../stereo_pair_persistence_test.go | 111 +++++++++--------- 2 files changed, 75 insertions(+), 56 deletions(-) diff --git a/cmd/soundtouch-service/main.go b/cmd/soundtouch-service/main.go index b7fe8625..eda819d3 100644 --- a/cmd/soundtouch-service/main.go +++ b/cmd/soundtouch-service/main.go @@ -1598,7 +1598,12 @@ func embeddedStereoPairGenerationPersistence( return err } - return stereopair.DeleteMargeGroupGeneration(httpClient, ref) + // The speaker itself self-reports its own group teardown to + // whatever Marge backend it's configured with -- that's the entire + // reason HandleMargeDeleteGroup/HandleMargeDeleteAccountGroups + // exist, they're only ever called by speakers, never by us. + // Nothing for us to push to a backend we don't own. + return nil } preflight := func(refs []stereopair.GenerationRef) error { @@ -1621,7 +1626,14 @@ func embeddedStereoPairGenerationPersistence( } if len(externalRefs) > 0 { - return stereopair.EnsureMargeNoGroupGenerations(httpClient, externalRefs) + // Best-effort dangling-generation check against a backend we + // don't own (real Bose cloud, another AfterTouch/SoundCork + // instance, ...): attempt it, but never let it being + // unreachable or unauthenticated block a Create the + // coordinator's own physical preflight already verified safe. + if err := stereopair.EnsureMargeNoGroupGenerations(httpClient, externalRefs); err != nil { + log.Printf("Stereo-pair external generation preflight inconclusive, proceeding: %v", err) + } } return nil @@ -1637,7 +1649,9 @@ func embeddedStereoPairGenerationPersistence( return err } - return stereopair.RenameMargeGroupGeneration(httpClient, ref, name) + // See cleanup's comment above: the speaker self-reports its own + // rename to whatever Marge backend it's configured with. + return nil } return cleanup, preflight, rename diff --git a/cmd/soundtouch-service/stereo_pair_persistence_test.go b/cmd/soundtouch-service/stereo_pair_persistence_test.go index 2650a53a..2099515a 100644 --- a/cmd/soundtouch-service/stereo_pair_persistence_test.go +++ b/cmd/soundtouch-service/stereo_pair_persistence_test.go @@ -1,9 +1,7 @@ package main import ( - "encoding/xml" "errors" - "fmt" "net/http" "net/http/httptest" "strings" @@ -19,9 +17,18 @@ import ( // TestEmbeddedStereoPairPersistenceTreatsDNSHijackedBoseHostAsLocal). func neverDNSHijacked(string) bool { return false } -type rejectingRoundTripper struct{} +// rejectingRoundTripper errors on every request and counts how many it saw. +// A preflight failure is now logged and swallowed rather than propagated +// (see TestEmbeddedStereoPairPersistenceSkipsExternalWritesButAttemptsRead), +// so tests that need to prove an external dispatch actually happened check +// calls rather than the returned error. +type rejectingRoundTripper struct { + calls int +} + +func (r *rejectingRoundTripper) RoundTrip(*http.Request) (*http.Response, error) { + r.calls++ -func (rejectingRoundTripper) RoundTrip(*http.Request) (*http.Response, error) { return nil, errors.New("unexpected HTTP persistence request") } @@ -50,7 +57,7 @@ func TestEmbeddedStereoPairPersistenceUsesLocalDatastoreAcrossAccounts(t *testin ds, func() []string { return []string{localURL} }, neverDNSHijacked, - &http.Client{Transport: rejectingRoundTripper{}}, + &http.Client{Transport: &rejectingRoundTripper{}}, ) err = preflight([]stereopair.GenerationRef{{ @@ -93,7 +100,7 @@ func TestEmbeddedStereoPairCleanupMapsAmbiguousGenerationToConflict(t *testing.T ds, func() []string { return []string{localURL} }, neverDNSHijacked, - &http.Client{Transport: rejectingRoundTripper{}}, + &http.Client{Transport: &rejectingRoundTripper{}}, ) wrongTopology := persistenceTestGroup(groupID) wrongTopology.Roles.Roles[1].DeviceID = "SUBSTITUTE-RIGHT-ID" @@ -107,64 +114,54 @@ func TestEmbeddedStereoPairCleanupMapsAmbiguousGenerationToConflict(t *testing.T } } -func TestEmbeddedStereoPairPersistenceUsesExternalMargeBackend(t *testing.T) { - active := true - deleteCalls := 0 - postCalls := 0 - expected := persistenceTestGroup("7654321") +// TestEmbeddedStereoPairPersistenceSkipsExternalWritesButAttemptsRead covers +// an external (non-local) MargeURL: cleanup and rename must never push to a +// backend we don't own -- the speaker itself self-reports its own group +// teardown/rename to whatever Marge backend it's configured with, which is +// the entire reason HandleMargeDeleteGroup/HandleMargeModifyGroup exist +// (they're only ever called by speakers). Preflight still attempts its +// read-only dangling-generation check, but a failure there (network error, +// wrong credentials, real Bose cloud rejecting us, ...) must not block +// Create, since it's a best-effort check on top of the coordinator's own +// physical preflight, not the primary guard. +func TestEmbeddedStereoPairPersistenceSkipsExternalWritesButAttemptsRead(t *testing.T) { + getCalls := 0 server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - switch { - case r.Method == http.MethodGet && strings.HasSuffix(r.URL.Path, "/device/LEFT-ID/group"): - if !active { - _, _ = w.Write([]byte(``)) - return - } - _, _ = fmt.Fprintf(w, `%s%sLEFT-IDLEFT192.0.2.10RIGHT-IDRIGHT192.0.2.11`, expected.ID, expected.Name, expected.MasterDeviceID) - case r.Method == http.MethodDelete && strings.HasSuffix(r.URL.Path, "/group/"+expected.ID): - deleteCalls++ - active = false - w.WriteHeader(http.StatusOK) - case r.Method == http.MethodPost && strings.HasSuffix(r.URL.Path, "/group/"+expected.ID): - postCalls++ - var update models.Group - if err := xml.NewDecoder(r.Body).Decode(&update); err != nil { - http.Error(w, err.Error(), http.StatusBadRequest) - return - } - expected = &update - w.WriteHeader(http.StatusOK) - default: - http.NotFound(w, r) + if r.Method == http.MethodGet && strings.HasSuffix(r.URL.Path, "/device/LEFT-ID/group") { + getCalls++ + http.Error(w, "unauthorized", http.StatusUnauthorized) + + return } + + t.Fatalf("unexpected external %s %s: cleanup/rename must not write to a backend we don't own", r.Method, r.URL.Path) })) defer server.Close() - cleanup, _, rename := embeddedStereoPairGenerationPersistence( + cleanup, preflight, rename := embeddedStereoPairGenerationPersistence( datastore.NewDataStore(t.TempDir()), func() []string { return []string{"http://aftertouch.invalid:18000"} }, neverDNSHijacked, server.Client(), ) - if err := rename(stereopair.GenerationRef{ + + ref := stereopair.GenerationRef{ DeviceID: "LEFT-ID", AccountID: "ACCOUNT1", MargeURL: server.URL + "/marge", - GroupID: expected.ID, ExpectedGroup: persistenceTestGroup(expected.ID), - }, "Renamed living room"); err != nil { - t.Fatalf("rename: %v", err) - } - if postCalls != 1 || expected.Name != "Renamed living room" { - t.Fatalf("external POST calls = %d, name = %q; want 1, Renamed living room", postCalls, expected.Name) + GroupID: "7654321", ExpectedGroup: persistenceTestGroup("7654321"), } - if err := cleanup(stereopair.GenerationRef{ - DeviceID: "LEFT-ID", AccountID: "ACCOUNT1", MargeURL: server.URL + "/marge", - GroupID: expected.ID, ExpectedGroup: expected, - }); err != nil { - t.Fatalf("cleanup: %v", err) + if err := rename(ref, "Renamed living room"); err != nil { + t.Fatalf("rename = %v, want nil (speaker self-reports its own rename)", err) } - - if deleteCalls != 1 || active { - t.Fatalf("external DELETE calls = %d, active = %v; want 1, false", deleteCalls, active) + if err := cleanup(ref); err != nil { + t.Fatalf("cleanup = %v, want nil (speaker self-reports its own teardown)", err) + } + if err := preflight([]stereopair.GenerationRef{ref}); err != nil { + t.Fatalf("preflight = %v, want nil: an unauthenticated external check must not block Create", err) + } + if getCalls != 1 { + t.Fatalf("external GET calls = %d, want exactly 1 (preflight must still attempt the read)", getCalls) } } @@ -178,6 +175,7 @@ func TestEmbeddedStereoPairPersistenceReadsOneCurrentURLSnapshot(t *testing.T) { currentURL := "http://old.invalid:18000" providerCalls := 0 + transport := &rejectingRoundTripper{} _, preflight, _ := embeddedStereoPairGenerationPersistence( ds, func() []string { @@ -185,7 +183,7 @@ func TestEmbeddedStereoPairPersistenceReadsOneCurrentURLSnapshot(t *testing.T) { return []string{currentURL} }, neverDNSHijacked, - &http.Client{Transport: rejectingRoundTripper{}}, + &http.Client{Transport: transport}, ) currentURL = "http://new.invalid:18000" @@ -200,11 +198,18 @@ func TestEmbeddedStereoPairPersistenceReadsOneCurrentURLSnapshot(t *testing.T) { t.Fatalf("URL provider calls = %d, want one coherent snapshot", providerCalls) } + // The old URL no longer matches localMargeURLs()'s current snapshot, so + // this ref is external. Preflight still attempts the read (proven by the + // transport call count) but no longer propagates its failure -- an + // external check failing must not block Create. err = preflight([]stereopair.GenerationRef{{ DeviceID: "LEFT-ID", AccountID: "OLD-ACCOUNT", MargeURL: "http://old.invalid:18000", }}) - if err == nil || !strings.Contains(err.Error(), "unexpected HTTP persistence request") { - t.Fatalf("old URL preflight error = %v, want external HTTP dispatch", err) + if err != nil { + t.Fatalf("old URL preflight error = %v, want nil (external check failure must not block)", err) + } + if transport.calls != 1 { + t.Fatalf("external HTTP dispatch calls = %d, want exactly 1", transport.calls) } } @@ -228,7 +233,7 @@ func TestEmbeddedStereoPairPersistenceTreatsDNSHijackedBoseHostAsLocal(t *testin ds, func() []string { return []string{"https://aftertouch.invalid:18443"} }, func(margeURL string) bool { return strings.Contains(margeURL, "streaming.bose.com") }, - &http.Client{Transport: rejectingRoundTripper{}}, + &http.Client{Transport: &rejectingRoundTripper{}}, ) err = preflight([]stereopair.GenerationRef{{