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>
statusUpdated (which drives IsConnected) ORed in
"stereoCapable && groupErr == nil" alongside the five substantive
status fetches. Since GetGroup is gated to stereo-capable models and
trivially succeeds even when a device is struggling (an empty
<group/> is a near-guaranteed reply, per Client.GetGroup's doc
comment), a round where NowPlaying/Volume/Presets/Sources/Bass all
fail but GetGroup alone succeeds would still report the device
connected -- masking a real status-refresh failure specifically on
ST10 hardware.
Removed the extra OR term entirely: IsConnected now depends only on
the five substantive fetches, matching the comment's own stated
intent ("mirrors prior behaviour"). GetGroup's own success/failure
still drives whether Group gets refreshed (unchanged, see
ApplyPolledGroup below), just no longer feeds the connectivity signal.
Added TestUpdateDeviceStatusNotConnectedWhenOnlyGroupSucceeds,
verified to fail against the prior logic and pass with this fix.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Refines the previous commit's doc fix with more precise, hardware-
verified detail: the ST20 doesn't just silently drop the connection --
its own firmware ("AllegroWebserver") eventually returns an explicit
"AllegroWebserver timeout: /getGroup" plain-text error after an
internal delay of several+ seconds, well past what client.get()'s
timeout will tolerate.
Also documents a dead-end a future contributor might otherwise try:
the ST20's own /supportedURLs response lists /getGroup (and the other
group endpoints) despite not actually servicing it, confirmed against
the same real hardware. A supportedURLs-based capability probe would
not have caught this either -- the model-name check has to stay.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The exported GetGroup doc comment claimed non-ST10 devices reply to
/getGroup "harmlessly" with an empty group. Verified against real
hardware this is wrong: a SoundTouch 20 does not reply at all -- the
request hangs until the client's own timeout (10-30s depending on how
the Client was constructed) instead of returning quickly. Confirmed
by direct request against a real ST20 (curl, 8s timeout, zero bytes
back) and cross-checked against two actively-paired real ST10 units,
which both replied in ~30-40ms with full group data.
This matters beyond prose accuracy: the newer stereoPairCapable gate
in websocket.go's UpdateDeviceStatus is load-bearing, not an
optimization. A future contributor trusting the old (wrong, and more
prominent/exported) doc could reasonably "simplify" by removing that
gate, reintroducing a 10-30s hang on every poll cycle for every
SoundTouch 20/30 on the network. Rewrote the doc to state the real
behavior and point at the gate that depends on it; the websocket.go
comment now defers to this doc instead of independently (and
incorrectly worded) restating it.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
