Same fix as -service and -player, for consistency: newGroupCoordinator
proactively pushed group cleanup/rename to an external Marge backend
the CLI doesn't own. A speaker's own firmware already self-reports
that 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.
Extracted the wiring into cliStereoPairGenerationPersistence (mirrors
the -service/-player equivalents): cleanup and rename are now no-ops,
and preflight's read-only dangling-generation check stays but its
failure no longer blocks Create -- it's printed as a warning instead.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Same reasoning as the -service fix: NewWebApp's default generation-
lifecycle wiring (used by the standalone player, and by embedded
-service until SetStereoPairGenerationPersistence overrides it)
proactively pushed group cleanup/rename to an external Marge backend
the player doesn't own. A speaker's own firmware already self-reports
that 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.
Extracted the wiring into playerStereoPairGenerationPersistence
(mirroring cmd/soundtouch-service's own testable helper): cleanup and
rename are now no-ops, and preflight's read-only dangling-generation
check stays but its failure no longer blocks Create.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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 <noreply@anthropic.com>
A speaker migrated at the DNS level keeps reporting its own MargeURL as
the literal Bose cloud hostname (e.g. https://streaming.bose.com) --
DNS migration only changes how that hostname resolves on the network,
never the device's own advertised URL. embeddedStereoPairGenerationPersistence's
isLocal() only matched against this service's own advertised server
URL, so it misclassified such a speaker as "external" and sent the
stereo-pair Create preflight's generation-conflict check out over the
real internet instead of checking the local datastore. Bose's cloud is
still live enough to answer (just not to authenticate us), so the
check failed with HTTP 401 and hard-blocked Create for an otherwise
perfectly normal DNS-migrated setup.
Added a DNS-hijack-aware check: if this service's own DNS hijack is
active and the reported MargeURL's host is one of the known redirected
Bose hostnames (discovery.InterceptedBoseHosts), treat it as local too.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
#672 added pkg/service/soundtouchweb/frontend_test/*.test.mjs (Node's
built-in test runner) but never hooked them into anything: not the
Makefile, not any GitHub Actions workflow. They only ran if someone
happened to invoke `node --test` manually, so CI would stay green even
if isSoundTouch10StereoPair or the DeviceDetail notice markup broke.
Added `make test-frontend` and a step in browser-tests.yml (which
already runs the Go-side chromedp player tests) to run them on every
push/PR, matching the pinned actions/setup-node version already used
by update-static-deps.yml.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Constrain device and playback metadata across narrow layouts while retaining complete values through tooltips and a touch-friendly details disclosure, including RAOP tracks.
The test's own comment said "we point the LAN host at 127.0.0.1", but
the resolver stub actually returned 1.2.3.4 -- a real, internet-
routable address, not 127.0.0.1 -- since the test was first written.
Probing an arbitrary internet destination's reachability depends on
the tester's own network path: a transparent proxy, a DPI middlebox,
or "known test IP" sinkholing can all make 1.2.3.4:443 appear
reachable, failing the test's core assumption outside a sandboxed CI
network. See #683.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
pkg/stereopair's sameRoles and pkg/service/datastore's
sameGroupGenerationTopology independently reimplemented the same
Role-keyed topology comparison, but normalized IP addresses
differently (net.ParseIP-only vs. plain string equality) -- exactly
the class of disagreement models.SameGroup was already created to fix
for order-sensitivity. Both now delegate their per-role comparison to
a new models.SameGroupRoles, which treats equal-but-differently-
formatted IPs as a match without regressing the common
both-addresses-unset case either implementation relied on.
models.SameGroup and datastore's sameStereoPair stay distinct
(commented why): both are intentionally ID/IP-agnostic for reasons
unrelated to this consolidation.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
completeStereoPairMutation's refreshStereoPairMembersAsync ran after
applyStereoPairProjection, and its UpdateDeviceStatus call always
minted a strictly newer group generation via BeginGroupRefresh -- so
ApplyPolledGroup's staleness guard could never reject it, even if its
/getGroup read raced a slower path and was stale relative to the
mutation that had already completed. The refresh now snapshots each
member's post-projection generation and only applies its own read via
ApplyPolledGroupIfBaseline, which requires nothing else (no other
event or poll) to have changed group state in the meantime. Other
UpdateDeviceStatus callers keep their existing always-newer semantics.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
applyDissolve tore down LEFT/RIGHT as independent goroutines with no
compensation path: a partial failure (one member's RemoveGroup
succeeds, the other never verifies empty even after the existing
reverification retries) only ever produced StatusDegraded, requiring
manual operator intervention. compensateDissolve retries RemoveGroup
on any member still unverified, giving it the same chance to converge
that Create's compensateCreate already gets on its own partial
failures.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
HandleMargeDeleteAccountGroups (DELETE /streaming/account/{id}/group/,
no group ID) had become a pure acknowledgement, leaving
DeleteAllGroupsForAccount dead code. This is the exact request real
firmware sends on factory reset/teardown; skipping the delete leaves a
stale Group_*.xml behind, permanently rejecting the next legitimate
Create for those devices via EnsureNoGroupsForDevices with no operator
remedy.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
validateCreateCandidates rejected LEFT/RIGHT pairs whenever their
MargeAccountUUID differed, even though no hardware or design
requirement calls for it: the pre-lifecycle CLI's direct /addGroup
calls never checked Marge accounts, and the existing read/display
projection has no account awareness either. Confirmed via live
hardware testing that this newly rejects a real, previously-working
cross-account stereo pair.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Gorilla's default same-origin CheckOrigin compares Origin and Host as
raw strings, port included. This repo's own documented nginx reverse-
proxy config forwards a portless Host header (nginx's $host never
includes the port, unlike $http_host) regardless of what public port
the proxy listens on. That's harmless on the scheme's default port
(the browser's Origin also omits it there), but on a non-default
public port (e.g. :8443, a realistic multi-service-hosting shape) the
browser's Origin keeps the port while the forwarded Host doesn't --
gorilla's strict compare then 403s every WebSocket handshake, silently
breaking the player's live updates in a deployment topology the docs
actively recommend.
Add checkWebSocketOrigin/sameHostIgnoringPort: gorilla's own default
policy, but comparing hostname only. Same-origin and cross-hostname
behavior is unchanged; only a port mismatch on an otherwise-matching
hostname is now tolerated. Also extracted newTestWebSocketServer,
shared by dialTestWebSocket and the origin-policy test, instead of the
origin test re-implementing the same httptest scaffolding inline.
Found in code review of PR #669 (findings #1, #2).
BeginStatusPoll/ApplySpeakerEvent/CompleteStatusPoll gated an entire
poll's merge (NowPlaying/Volume/Presets/Sources/Bass/IsConnected) behind
one shared speakerEventGeneration counter. Any unrelated push event
during the poll's flight discarded the whole result -- not just the
field that event touched. Sources has no push event at all, so it could
go stale indefinitely under ordinary event traffic, defeating both call
sites that depend on this poll (the 30s fallback poll and the
post-reconnect refresh).
Replace it with StatusField + BeginFieldPoll/CompleteFieldPoll/
ApplyFieldEvent: the same two-counter (issued/applied) pattern already
used for Group, generalized to one instance per independently-racing
field via a small fixed-size array. A poll or event for one field can
now only ever supersede that same field, never a different one. This
also removes the map-based generation bookkeeping the old mechanism
needed (issue/lookup/prune per poll) and the unreachable
"unknown generation" branch it required.
applyGroupUpdatedEvent now shares the same queueBroadcastIfChanged
helper as the other five event types, instead of duplicating the
"broadcast if changed" check inline.
Found in code review of PR #666 (findings #1, #2, #3, #4).
Two tests from PR #666 simulated "the browser WS write path is busy" by
holding the global webSocketWriteMu, which #665's fix removed. Adapt
them to the per-connection replacement: register a connection and hold
its own lock directly (same technique as the #665 test suite), rather
than a lock that no longer exists.
webSocketWriteMu serialized writes across ALL browser WebSocket
connections, not just the single connection gorilla actually requires.
HandleDeleteDevice's synchronous BroadcastDeviceList call, and every
other client's own periodic update, all contended on one lock -- directly
contradicting the PR's own goal that a stalled client cannot block
healthy ones.
Replace it with a per-connection *sync.Mutex stored in WSClients
(withConnWrite). Registration is fully decoupled from discovery-status
publication: a new connection reads whatever discoveryStatus.Load()
currently returns and is never blocked by an in-flight publication,
which stays safe because Store() always commits before a publication
takes its client snapshot. BroadcastDeviceList/BroadcastDiscoveryStatus
now write each client under only that client's own lock.
Found in code review of PR #665 (finding #1).
newStereoPairView emitted raw, un-normalized role.DeviceID/role.Role
while validMasterGroup/registeredMembersAgree/sameGroupClaim trim and
uppercase those same fields for internal comparison. Normalize before
assigning so a future frontend feature reading member.Role/.DeviceID
directly doesn't need to re-normalize it itself.
Found in code review of PR #665 (finding #8).
The singular GET /api/control/devices/{id} bypassed the projection that
HandleAPIDevices and both WebSocket frames already apply. A hidden
stereo-pair member was absent from the list but still fully fetchable,
unprojected, by its own id. Add deviceViewForID and return 404 for a
hidden member's own id, consistent with it already being absent from
the list.
Found in code review of PR #665 (finding #3).
BeginGroupRefresh/ApplyPolledGroup keyed invalidation off "has any newer
poll started" via groupGeneration equality. A later poll that starts but
never applies (its own GetGroup fails) still discarded an earlier poll's
still-arriving successful result, even though nothing newer ever actually
landed. Add groupAppliedGeneration and gate on "strictly newer than the
last applied", not "equal to the latest issued".
Found in code review of PR #665 (finding #2).
The mock services in docker-compose.ci.yml were pinned to
golang:1.27.0-alpine, which is now too old to run against go.mod's
1.27.1 requirement (GOTOOLCHAIN=local makes this an immediate, silent
container crash: "go.mod requires go >= 1.27.1 (running go 1.27.0)").
Bump all three mock images to 1.27.1-alpine to match.
Also make `make test-http-client` dump docker compose logs (and tear
down) whenever `docker compose up --wait` itself fails, not only
after the .http test run — that path previously aborted with no
diagnostic output at all. Switch the post-run log dump to plain
`docker compose logs` (all services) instead of three hardcoded
per-service calls, which had silently omitted tunein-mock.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The default 20s wsURLReadTimeout in chromedp's exec allocator can be
too tight on a loaded shared CI runner spawning headless Chrome,
surfacing as an unrelated "websocket url timeout reached" test
failure. Raise it to 45s and widen the per-test context to match.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
TestPrintRoutes compares against a checked-in route list; the new
GET .../zone/candidates route (added for Zone.js's candidate source
fix) needs to be reflected there too.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
If the device selected in the Library tab disappeared from the
devices map (e.g. it just became a hidden stereo-pair member per
device_projection.go), the sync effect fell back to entries[0][0] --
whichever key happens to sort first in the map -- silently redirecting
the user's Library browsing session to an unrelated speaker.
Now checks first whether the vanished device reappears as a member of
some other device's stereoPair (the pair's master, which now
represents the same physical speaker for control purposes) and
follows it there. Only falls back to an arbitrary device when the
selection is gone for a genuinely unrelated reason (removed, discovery
gap), matching the prior behavior for that case.
No JS unit-test framework exists in this repo for component-level
logic (consistent with the rest of the client-side code), so this is
verified by manual trace rather than an automated regression test.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Zone.js derived its "add speaker" candidate list from the `devices`
prop, which is app.js's projected/collapsed device list. Once the
stereo-pair projection (device_projection.go) started hiding a pair's
non-master member from that list, it silently became impossible to
add that physical device to an unrelated multiroom zone, even though
the backend's HandleZoneAdd/HandleZoneRemove already operate on the
raw device registry directly and never cared about pairing at all.
Zone.js's own file wasn't touched by that change; its effective input
just changed underneath it.
Added GET /api/control/devices/{id}/zone/candidates, deliberately
bypassing deviceViewSnapshot's projection and deliberately not
excluding {id} itself -- which candidates to exclude is a caller
concern (Zone.js already does this via the existing zoneIps set,
which includes the zone master's own IP even when standalone, per
models.ZoneInfo.IsStandalone). Zone and Group are separate, unrelated
groupings and should stay that way.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
sameGroupClaim (device_projection.go, used to validate a member's
claim agrees with the master's) compared roles via a device-ID-keyed
map, making it order-insensitive. webtypes.replaceGroup's change
detection used reflect.DeepEqual on the whole *Group, which is
order-sensitive for Roles.Roles. Both the polled /getGroup response
and the pushed groupUpdated event populate Roles.Roles directly from
XML unmarshaling in wire order, so nothing guarantees a pair's roles
list in the same order across two reads -- DeepEqual could then report
a spurious "changed" for a pair that didn't actually change.
Extracted the order-insensitive comparison into models.SameGroup as
the single shared implementation (also handles the nil/nil case
correctly, unlike the old sameGroupClaim, which mattered for
replaceGroup's existing "no prior group" path). Both call sites now
use it; the duplicate sameGroupClaim is gone.
Added TestApplyGroupEventIgnoresRoleOrder, verified to fail against
the prior DeepEqual-based logic and pass with this fix.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
