Status was a value-typed DeviceStatus field on DeviceConnection,
written from the periodic poller (UpdateDeviceStatus) and from four
WebSocket event handlers (OnNowPlaying, OnVolumeUpdated,
OnConnectionState, OnPresetUpdated) while being read from every HTTP
handler and the WebSocket broadcaster. The struct was 8+ words wide
with time.Time and string members, so concurrent readers could
observe torn fields or mixed-update snapshots. The map-level race was
fixed in the previous commit; this one closes the per-connection
struct race.
Hide the field behind atomic.Pointer[DeviceStatus]:
Status() // returns current snapshot
SetStatus(*DeviceStatus) // wholesale replace
UpdateStatus(func(*DeviceStatus)) // CAS retry loop
NewDeviceConnection constructs a connection with the atomic pointer
pre-initialised, so Status() never returns nil for callers that go
through the constructor (the old struct-literal pattern is no longer
possible because the status field is now private).
UpdateDeviceStatus runs network fetches into local vars first, then
batches them into a single UpdateStatus call so the CAS loop only
retries the merge — not the slow IO. WebSocket event handlers and
the connect/disconnect transitions each use UpdateStatus, so any
ordering of poller + event delivery converges to a consistent
status.
The UpdateStatus docstring is explicit about the shallow-copy
contract: nested pointer fields (NowPlaying, Volume, Bass, Presets,
Sources) MUST be replaced, not mutated through, because the copy
mut receives shares those pointers with the prior snapshot. All
production callers already follow this pattern (every value comes
fresh from the device API).
Tests:
- types_test.go: migrated literal struct to NewDeviceConnection +
SetStatus, switched reads to Status().
- status_test.go (new): six tests covering constructor init,
SetStatus replacement semantics, UpdateStatus mutator
application, field preservation across UpdateStatus, snapshot
isolation (old snapshot stable under later writes), and a
concurrent stress test (16 writers + 32 readers x 200 ops) that
runs under -race.
- handlers_test.go, registry_test.go, spa_test.go: migrated to
constructor.
Not addressed by this commit:
- DeviceConnection.WebSocket (set once in ConnectDeviceWebSocket,
read elsewhere). Word-sized pointer, atomic at the hardware
level on amd64/arm64; race detector may still flag.
- DeviceConnection.LastSeen (written under devicesMu by the
registry, read outside that lock via DeviceSnapshot consumers).
time.Time is non-atomic but the read is cosmetic.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The Devices map on WebApp was written from the startup goroutine, the
/api/discover POST handler, and addDevice, while being read from every
HTTP handler and the WebSocket periodic-update loop — all without any
mutex. The Go runtime panics with "fatal error: concurrent map writes"
or "concurrent map read and map write" on any actual collision, so this
was a latent crash, not a tearing issue.
Hide the map behind a sync.RWMutex and a small API:
GetDevice(id) (*DeviceConnection, bool)
DeviceSnapshot() []DeviceEntry
DeviceCount() int
AddDevice(id, conn) bool // atomic insert-or-touch
TouchDevice(id) bool // fast-path LastSeen bump
Update every caller — handlers, websocket, main, tests — to go through
the API. addDevice's existing-host fast path uses TouchDevice; the
final insert uses AddDevice so a race with another writer is rejected
cleanly instead of silently overwriting.
Add a TestRegistryConcurrent stress test that runs 64 goroutines doing
12,800 operations across writers, touchers, and two reader patterns.
It exists to give `-race` (already on in CI) a concrete shape to catch
if the encapsulation ever leaks back out.
Struct-field races on conn.Status.* are not addressed by this change;
they need their own follow-up.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`addManualDevice` and the per-device branch of `discoverDevices` were
~40 lines of near-identical client setup, info fetch, connection
build, and map write — differing only in log wording. Extract a
shared `addDevice(app, host, port, source)` helper used by both
paths.
Side effects of consolidating:
- Duplicate-host guard (LastSeen bump) now applies to both paths, so
passing `--devices 1.2.3.4` twice is idempotent and matches how
discovery treats repeat sightings.
- Map write happens before the UpdateDeviceStatus goroutine launch,
so a concurrent GET /api/devices sees the device with
`IsConnected: false` instead of racing the status update.
- Log wording is consistent: "Failed to fetch device info from <host>
(<source>): <err>" and "Added <source> device <name> (<type>) at
<host>:<port>".
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
When the user passes --bind <iface> and doesn't set --interface,
discovery now reuses the same interface name instead of auto-picking.
Common single-interface setups stop needing to repeat the flag, while
the two flags remain independent for the cases that legitimately want
HTTP and discovery on different interfaces.
Update the --interface help text to document the default. The --bind
text is unchanged: it still describes the HTTP listener address.
Refs #264
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The previous implementation silently returned the literal interface
name when the interface existed but had no IPv4 address (or when
listing addresses failed). That reproduces the exact error from #264
("listen tcp: lookup eth103 on ...: no such host") for users on
IPv6-only or admin-down interfaces, so the fix only worked for the
happy path.
Return an explicit error for those cases and fatal in main with a
message that identifies the offending --bind value. Add an IPv6
fallback (single non-link-local address, bracketed) and treat any
ambiguity -- multiple IPv4 or multiple IPv6 addresses on the same
interface -- as an error rather than picking one silently. Log when an
interface name was resolved to an IP so the indirection is visible.
Update the --bind flag help text to reflect the supported inputs.
Add a test covering the pass-through cases (host, IP, empty, unknown
name) and a portable loopback-interface test that skips cleanly when
the loopback isn't in a single-IPv4 configuration.
Refs #264
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
On a multi-homed host the discovery layer used to walk net.Interfaces()
and pick the first non-loopback IPv4 NIC, while UPnP/SSDP bound a
wildcard UDP socket and let the kernel route the multicast send. That
meant --bind on soundtouch-web only moved the HTTP listener; the
discovery still went out whatever interface the kernel preferred (often
the wrong one on hosts where the speakers sit behind a secondary NIC).
Introduce a separate DiscoveryInterface knob:
* pkg/config: DiscoveryInterface field + DISCOVERY_INTERFACE env var.
* pkg/discovery/mdns: NewMDNSDiscoveryServiceWithInterface; the
interface resolver now honours an explicit name and validates it
has a usable IPv4 address before handing it to hashicorp/mdns.
* pkg/discovery/upnp: when an interface is configured, bind the UDP
socket's source IP to the NIC's IPv4 and call
ipv4.PacketConn.SetMulticastInterface so M-SEARCH leaves the right
NIC. Without an interface, behaviour is unchanged.
* cmd/soundtouch-web: new --interface flag (DISCOVERY_INTERFACE env)
plumbed into the config before the discovery service is built.
go.mod/go.sum reflect promoting golang.org/x/net from indirect to a
direct dependency (now imported for ipv4.PacketConn).
Refs #264.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The flag's value is concatenated with ":PORT" and passed to
http.ListenAndServe, so it has always been a host/IP. The previous help
text invited users to pass an interface name like "eth0", which then
failed with a confusing DNS lookup error.
Refs #264.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
