From a21b4d71ae93dcbb0d76b87c83e9bfdf83a9cc01 Mon Sep 17 00:00:00 2001 From: Tobias Gesellchen Date: Sat, 29 Aug 2026 15:55:04 +0200 Subject: [PATCH] fix(service): address code-review findings on PR #652's startup retry Fixes correctness issues found reviewing the bounded device-seed retry loop before merging: a datastore read failure could make the readiness check trivially pass; stale-host pruning only considered hosts inserted in the current attempt and only ran inside the retry loop, not the plain SeedExtraDevices path; the retry loop and a devices-changed-hook seed could probe the same offline host concurrently; and a zero-change startup window silently dropped the previously-unconditional device-list broadcast. Also makes the retry interval/window configurable instead of hardcoded, following the existing discovery-interval flag pattern. Co-Authored-By: Claude Sonnet 5 --- cmd/soundtouch-service/main.go | 257 ++++++++++-------- cmd/soundtouch-service/main_test.go | 52 ++++ .../content/docs/guides/SOUNDTOUCH-SERVICE.md | 56 ++-- pkg/service/soundtouchweb/discovery.go | 113 +++++--- pkg/service/soundtouchweb/discovery_test.go | 131 ++++++++- pkg/service/soundtouchweb/handler.go | 12 +- 6 files changed, 446 insertions(+), 175 deletions(-) diff --git a/cmd/soundtouch-service/main.go b/cmd/soundtouch-service/main.go index 091fa62d..08a796c5 100644 --- a/cmd/soundtouch-service/main.go +++ b/cmd/soundtouch-service/main.go @@ -46,11 +46,6 @@ var ( repoURL = "https://github.com/gesellix/bose-soundtouch" ) -const ( - embeddedDeviceSeedRetryInterval = 30 * time.Second - embeddedDeviceSeedRetryWindow = 10 * time.Minute -) - func updateBuildInfo() { if info, ok := debug.ReadBuildInfo(); ok { if info.Main.Path != "" { @@ -305,6 +300,18 @@ var serviceFlags = []cli.Flag{ Value: "5m", EnvVars: []string{"DISCOVERY_INTERVAL"}, }, + &cli.StringFlag{ + Name: "device-seed-retry-interval", + Usage: "Interval between embedded-player startup retries for unreachable persisted devices", + Value: "30s", + EnvVars: []string{"DEVICE_SEED_RETRY_INTERVAL"}, + }, + &cli.StringFlag{ + Name: "device-seed-retry-window", + Usage: "Bounded window during which the embedded player retries unreachable persisted devices at startup", + Value: "10m", + EnvVars: []string{"DEVICE_SEED_RETRY_WINDOW"}, + }, &cli.BoolFlag{ Name: "update-check-enabled", Usage: "Periodically check GitHub for a newer release (opt-in; the only network call this makes beyond speaker/provider traffic)", @@ -661,7 +668,7 @@ func main() { } internalURL := "http://" + net.JoinHostPort(loopbackHost, config.port) - webApp := newEmbeddedWebApp(server, config.serverURL, internalURL, ds) + webApp := newEmbeddedWebApp(server, config.serverURL, internalURL, ds, config.deviceSeedRetryInterval, config.deviceSeedRetryWindow) r := setupRouter(server, stockholmHandler, webApp) @@ -720,55 +727,57 @@ func showVersionInfo(_ *cli.Context) error { } type serviceConfig struct { - port string - bindAddr string - addr string - dataDir string - hostname string - serverURL string - httpsServerURL string // effective (derived or overridden) - httpsOverride string // explicit override; "" = derive from serverURL - httpsPort string - httpsDefaultURL string // hostname-based fallback - httpsAddr string - redact bool - logBody bool - record bool - dnsEnabled bool - dnsUpstream string - dnsBind string - internalPaths []string - tlsExtraHosts []string - discoveryEnabled bool - discoveryInterval time.Duration - updateCheckEnabled bool - updateCheckInterval time.Duration - domains []string - spotifyClientID string - spotifyClientSecret string - spotifyRedirectURI string - spotifyTokenURL string - spotifyAPIBase string - amazonClientID string - amazonClientSecret string - amazonRedirectURI string - amazonTokenURL string - amazonProfileURL string - tuneInOpmlURL string - tuneInAPIURL string - mgmtUsername string - mgmtPassword string - ttsProvider string - ttsGoogleAPIKey string - ttsGoogleEndpoint string - ttsLanguage string - ttsVoice string - ttsAppKey string - ttsVolume int - migrationEnabled bool - migrationDryRun bool - stockholmDir string - stockholmBasePath string + port string + bindAddr string + addr string + dataDir string + hostname string + serverURL string + httpsServerURL string // effective (derived or overridden) + httpsOverride string // explicit override; "" = derive from serverURL + httpsPort string + httpsDefaultURL string // hostname-based fallback + httpsAddr string + redact bool + logBody bool + record bool + dnsEnabled bool + dnsUpstream string + dnsBind string + internalPaths []string + tlsExtraHosts []string + discoveryEnabled bool + discoveryInterval time.Duration + deviceSeedRetryInterval time.Duration + deviceSeedRetryWindow time.Duration + updateCheckEnabled bool + updateCheckInterval time.Duration + domains []string + spotifyClientID string + spotifyClientSecret string + spotifyRedirectURI string + spotifyTokenURL string + spotifyAPIBase string + amazonClientID string + amazonClientSecret string + amazonRedirectURI string + amazonTokenURL string + amazonProfileURL string + tuneInOpmlURL string + tuneInAPIURL string + mgmtUsername string + mgmtPassword string + ttsProvider string + ttsGoogleAPIKey string + ttsGoogleEndpoint string + ttsLanguage string + ttsVoice string + ttsAppKey string + ttsVolume int + migrationEnabled bool + migrationDryRun bool + stockholmDir string + stockholmBasePath string } // resolveFallbackHost picks the host used to guess a server URL when @@ -865,6 +874,24 @@ func loadConfig(c *cli.Context) (serviceConfig, error) { discoveryInterval = 5 * time.Minute } + deviceSeedRetryIntervalStr := c.String("device-seed-retry-interval") + + deviceSeedRetryInterval, err := time.ParseDuration(deviceSeedRetryIntervalStr) + if err != nil { + log.Printf("Warning: Failed to parse device seed retry interval %s, using default 30s: %v", sanitizeLog(deviceSeedRetryIntervalStr), err) + + deviceSeedRetryInterval = 30 * time.Second + } + + deviceSeedRetryWindowStr := c.String("device-seed-retry-window") + + deviceSeedRetryWindow, err := time.ParseDuration(deviceSeedRetryWindowStr) + if err != nil { + log.Printf("Warning: Failed to parse device seed retry window %s, using default 10m: %v", sanitizeLog(deviceSeedRetryWindowStr), err) + + deviceSeedRetryWindow = 10 * time.Minute + } + updateCheckEnabled := c.Bool("update-check-enabled") updateCheckIntervalStr := c.String("update-check-interval") @@ -903,55 +930,57 @@ func loadConfig(c *cli.Context) (serviceConfig, error) { stockholmBasePath := c.String("stockholm-base-path") return serviceConfig{ - port: port, - bindAddr: bindAddr, - addr: addr, - dataDir: dataDir, - hostname: fallbackHost, - serverURL: serverURL, - httpsServerURL: httpsServerURL, - httpsOverride: httpsOverride, - httpsPort: httpsPort, - httpsDefaultURL: httpsDefaultURL, - httpsAddr: httpsAddr, - redact: redact, - logBody: logBody, - record: record, - dnsEnabled: dnsEnabled, - dnsUpstream: dnsUpstream, - dnsBind: dnsBind, - internalPaths: internalPaths, - tlsExtraHosts: tlsExtraHosts, - discoveryEnabled: discoveryEnabled, - discoveryInterval: discoveryInterval, - updateCheckEnabled: updateCheckEnabled, - updateCheckInterval: updateCheckInterval, - domains: domains, - spotifyClientID: spotifyClientID, - spotifyClientSecret: spotifyClientSecret, - spotifyRedirectURI: spotifyRedirectURI, - spotifyTokenURL: spotifyTokenURL, - spotifyAPIBase: spotifyAPIBase, - amazonClientID: amazonClientID, - amazonClientSecret: amazonClientSecret, - amazonRedirectURI: amazonRedirectURI, - amazonTokenURL: amazonTokenURL, - amazonProfileURL: amazonProfileURL, - tuneInOpmlURL: tuneInOpmlURL, - tuneInAPIURL: tuneInAPIURL, - mgmtUsername: mgmtUsername, - mgmtPassword: mgmtPassword, - ttsProvider: ttsProvider, - ttsGoogleAPIKey: ttsGoogleAPIKey, - ttsGoogleEndpoint: ttsGoogleEndpoint, - ttsLanguage: ttsLanguage, - ttsVoice: ttsVoice, - ttsAppKey: ttsAppKey, - ttsVolume: ttsVolume, - migrationEnabled: migrationEnabled, - migrationDryRun: migrationDryRun, - stockholmDir: stockholmDir, - stockholmBasePath: stockholmBasePath, + port: port, + bindAddr: bindAddr, + addr: addr, + dataDir: dataDir, + hostname: fallbackHost, + serverURL: serverURL, + httpsServerURL: httpsServerURL, + httpsOverride: httpsOverride, + httpsPort: httpsPort, + httpsDefaultURL: httpsDefaultURL, + httpsAddr: httpsAddr, + redact: redact, + logBody: logBody, + record: record, + dnsEnabled: dnsEnabled, + dnsUpstream: dnsUpstream, + dnsBind: dnsBind, + internalPaths: internalPaths, + tlsExtraHosts: tlsExtraHosts, + discoveryEnabled: discoveryEnabled, + discoveryInterval: discoveryInterval, + deviceSeedRetryInterval: deviceSeedRetryInterval, + deviceSeedRetryWindow: deviceSeedRetryWindow, + updateCheckEnabled: updateCheckEnabled, + updateCheckInterval: updateCheckInterval, + domains: domains, + spotifyClientID: spotifyClientID, + spotifyClientSecret: spotifyClientSecret, + spotifyRedirectURI: spotifyRedirectURI, + spotifyTokenURL: spotifyTokenURL, + spotifyAPIBase: spotifyAPIBase, + amazonClientID: amazonClientID, + amazonClientSecret: amazonClientSecret, + amazonRedirectURI: amazonRedirectURI, + amazonTokenURL: amazonTokenURL, + amazonProfileURL: amazonProfileURL, + tuneInOpmlURL: tuneInOpmlURL, + tuneInAPIURL: tuneInAPIURL, + mgmtUsername: mgmtUsername, + mgmtPassword: mgmtPassword, + ttsProvider: ttsProvider, + ttsGoogleAPIKey: ttsGoogleAPIKey, + ttsGoogleEndpoint: ttsGoogleEndpoint, + ttsLanguage: ttsLanguage, + ttsVoice: ttsVoice, + ttsAppKey: ttsAppKey, + ttsVolume: ttsVolume, + migrationEnabled: migrationEnabled, + migrationDryRun: migrationDryRun, + stockholmDir: stockholmDir, + stockholmBasePath: stockholmBasePath, }, nil } @@ -1432,7 +1461,7 @@ func runUpdateCheckTick(checker *updatecheck.Checker, lastLoggedVersion string) // TriggerDiscovery runs the service sweep on a UI-initiated "discover", and the // devices-changed hook re-syncs the UI registry whenever the service's // discovery or a manual add changes the set. -func newEmbeddedWebApp(server *handlers.Server, serverURL, internalURL string, ds *datastore.DataStore) *soundtouchweb.WebApp { +func newEmbeddedWebApp(server *handlers.Server, serverURL, internalURL string, ds *datastore.DataStore, deviceSeedRetryInterval, deviceSeedRetryWindow time.Duration) *soundtouchweb.WebApp { webApp := soundtouchweb.NewWebApp() webApp.Version = version webApp.Commit = commit @@ -1449,11 +1478,10 @@ func newEmbeddedWebApp(server *handlers.Server, serverURL, internalURL string, d // stream URLs the speaker fetches and the UI displays it. webApp.InternalServiceURL = internalURL - webApp.ExtraDeviceHosts = func() []string { + webApp.ExtraDeviceHosts = func() ([]string, error) { devices, listErr := ds.ListAllDevices() if listErr != nil { - log.Printf("web UI: failed to list devices from datastore: %v", listErr) - return nil + return nil, fmt.Errorf("web UI: failed to list devices from datastore: %w", listErr) } hosts := make([]string, 0, len(devices)) @@ -1463,7 +1491,7 @@ func newEmbeddedWebApp(server *handlers.Server, serverURL, internalURL string, d } } - return hosts + return hosts, nil } // UI "discover" runs the service's sweep, not a second mDNS stack. @@ -1487,10 +1515,17 @@ func newEmbeddedWebApp(server *handlers.Server, serverURL, internalURL string, d // service can start before persisted speaker addresses are routable, so // retry only those known addresses for a bounded startup window. The // devices-changed hook and explicit discovery keep it current afterwards. - ctx, cancel := context.WithTimeout(context.Background(), embeddedDeviceSeedRetryWindow) + ctx, cancel := context.WithTimeout(context.Background(), deviceSeedRetryWindow) defer cancel() - webApp.SeedExtraDevicesUntilReady(ctx, embeddedDeviceSeedRetryInterval) + webApp.SeedExtraDevicesUntilReady(ctx, deviceSeedRetryInterval) + + // Unconditional: a WebSocket client connected during a window where + // every attempt inserted or removed nothing (e.g. no persisted devices + // at all, or every persisted host stayed unreachable for the whole + // window) must still see the current, converged device list once this + // goroutine's work is done. + webApp.BroadcastDeviceList() }() return webApp diff --git a/cmd/soundtouch-service/main_test.go b/cmd/soundtouch-service/main_test.go index 49344113..8f245daf 100644 --- a/cmd/soundtouch-service/main_test.go +++ b/cmd/soundtouch-service/main_test.go @@ -6,6 +6,7 @@ import ( "path/filepath" "strings" "testing" + "time" "github.com/gesellix/bose-soundtouch/pkg/service/datastore" "github.com/urfave/cli/v2" @@ -69,6 +70,57 @@ func TestResolveFallbackHost(t *testing.T) { } } +func TestLoadConfig_DeviceSeedRetryTuning(t *testing.T) { + t.Run("defaults", func(t *testing.T) { + config, err := loadConfig(newTestServiceContext(t)) + if err != nil { + t.Fatalf("loadConfig() error = %v", err) + } + + if config.deviceSeedRetryInterval != 30*time.Second { + t.Errorf("deviceSeedRetryInterval = %s, want 30s", config.deviceSeedRetryInterval) + } + + if config.deviceSeedRetryWindow != 10*time.Minute { + t.Errorf("deviceSeedRetryWindow = %s, want 10m", config.deviceSeedRetryWindow) + } + }) + + t.Run("flags override the defaults", func(t *testing.T) { + config, err := loadConfig(newTestServiceContext(t, + "--device-seed-retry-interval=5s", + "--device-seed-retry-window=1m")) + if err != nil { + t.Fatalf("loadConfig() error = %v", err) + } + + if config.deviceSeedRetryInterval != 5*time.Second { + t.Errorf("deviceSeedRetryInterval = %s, want 5s", config.deviceSeedRetryInterval) + } + + if config.deviceSeedRetryWindow != time.Minute { + t.Errorf("deviceSeedRetryWindow = %s, want 1m", config.deviceSeedRetryWindow) + } + }) + + t.Run("unparseable values fall back to the defaults", func(t *testing.T) { + config, err := loadConfig(newTestServiceContext(t, + "--device-seed-retry-interval=not-a-duration", + "--device-seed-retry-window=also-not-a-duration")) + if err != nil { + t.Fatalf("loadConfig() error = %v", err) + } + + if config.deviceSeedRetryInterval != 30*time.Second { + t.Errorf("deviceSeedRetryInterval = %s, want fallback 30s", config.deviceSeedRetryInterval) + } + + if config.deviceSeedRetryWindow != 10*time.Minute { + t.Errorf("deviceSeedRetryWindow = %s, want fallback 10m", config.deviceSeedRetryWindow) + } + }) +} + func TestLoadConfig_DeploymentMode(t *testing.T) { t.Run("on-device with no --server-url defaults to localhost", func(t *testing.T) { config, err := loadConfig(newTestServiceContext(t, "--deployment-mode=on-device", "--port=8000")) diff --git a/docs/content/docs/guides/SOUNDTOUCH-SERVICE.md b/docs/content/docs/guides/SOUNDTOUCH-SERVICE.md index 16e985a9..e830005d 100644 --- a/docs/content/docs/guides/SOUNDTOUCH-SERVICE.md +++ b/docs/content/docs/guides/SOUNDTOUCH-SERVICE.md @@ -156,33 +156,35 @@ The service supports multiple ways to configure its behavior. When multiple sour ### Configuration Options -| Variable | Flag | Description | Default | -|------------------------------------|----------------------------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|---------------------------------------------| -| `PORT` | `--port`, `-p` | HTTP port to bind the service to | `8000` | -| `BIND_ADDR` | `--bind` | Network interface to bind to | all (ipv4 and ipv6) | -| `DATA_DIR` | `--data-dir` | Directory for persistent data | `./data` | -| `SERVER_URL` | `--server-url`, `-s` | External URL of this service | `http://:8000` | -| `DEPLOYMENT_MODE` | `--deployment-mode` | Where this service runs: `on-device`, `private-network`, or `public-network`. Only changes behavior when `SERVER_URL` is *not* set: `on-device` defaults to `http://localhost:` instead of guessing a hostname (the speaker's own Linux hostname is never resolvable — see issue #546); `public-network` refuses to start rather than guess a publicly reachable address; unset/`private-network` keeps the previous hostname-guessing behavior, now with a startup warning. The on-device install script sets this automatically. | unset (legacy hostname guess, with warning) | -| `HTTPS_PORT` | `--https-port` | HTTPS port to bind the service to | `8443` | -| `HTTPS_SERVER_URL` | `--https-server-url`, `-S` | External HTTPS URL. An override: when empty it is derived from `SERVER_URL` (same host, `https`, on `HTTPS_PORT`), and can also be viewed/overridden in Settings. | derived from `SERVER_URL` | -| `PYTHON_BACKEND_URL`, `TARGET_URL` | `--target-url` | URL for Python-based service components (legacy) | `http://localhost:8001` | -| `REDACT_PROXY_LOGS` | `--redact-logs` | Redact sensitive data in proxy logs | `true` | -| `LOG_PROXY_BODY` | `--log-bodies` | Log full request/response bodies | `false` | -| `RECORD_INTERACTIONS` | `--record-interactions` | Record HTTP interactions to disk | `true` | -| `DISCOVERY_INTERVAL` | `--discovery-interval` | Device discovery interval | `5m` | -| `ENABLE_DNS_DISCOVERY` | `--dns-discovery` | Enable DNS discovery server | `false` | -| `DNS_UPSTREAM` | `--dns-upstream` | Upstream DNS server for non-Bose queries | `8.8.8.8` | -| `DNS_BIND_ADDR` | `--dns-bind` | Bind address for the DNS discovery server (standard port `:53` is required for DNS/DHCP migration) | `:53` | -| `INTERNAL_PATHS` | `--internal-paths` | Paths for internal requests to exclude from recording (e.g., `/setup/*`, `/web/*`) | `[]` | -| `DISCOVERY_DISABLED` | | Disable automated device discovery | `false` | -| `UPDATE_CHECK_ENABLED` | `--update-check-enabled` | Periodically check GitHub Releases for a newer version and show a dismissible notice in the admin UI and Player when one is found. **Opt-in**: this is the only network call AfterTouch makes beyond speaker/provider traffic when enabled, so it defaults off. One unauthenticated `GET` per interval to `api.github.com`, nothing else leaves the box. Also available as an "Update Check" toggle on the admin Settings page, which applies without a restart; the env var/flag is the seed value for a fresh install with no `settings.json` yet. | `false` | -| `UPDATE_CHECK_INTERVAL` | `--update-check-interval` | Update check interval. Also editable on the admin Settings page (applies without a restart). | `24h` | -| `MGMT_USERNAME` | `--mgmt-username` | Username for HTTP Basic Auth on the Management API (`/api/mgmt/*`, `/mgmt/*`) — Spotify/Amazon account linking, Local Accounts | `admin` | -| `MGMT_PASSWORD` | `--mgmt-password` | Password for the same Management API Basic Auth. **Change this if AfterTouch is reachable beyond a trusted LAN** — the default is published in this doc. | `change_me!` | -| `STOCKHOLM_DIR` | `--stockholm-dir` | Path to extracted Stockholm frontend directory — enables the Stockholm UI when set | *(disabled)* | -| `MARGE_URL` | | Streaming/marge base URL used when rewriting `stockholm/json/config.json`. Defaults to `SERVER_URL`. Set to `SERVER_URL/marge` only when using a soundcork backend. | *(same as `SERVER_URL`)* | -| `MARGE_AUTH_TOKEN` | | Pre-seeds the Stockholm `margeAuthToken` state (skips the login step for the first session) | *(empty)* | -| `MARGE_ACCOUNT_ID` | | Pre-seeds the Stockholm `margeAccountID` state (used to filter device-discovery results by account) | *(empty)* | +| Variable | Flag | Description | Default | +|------------------------------------|--------------------------------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|---------------------------------------------| +| `PORT` | `--port`, `-p` | HTTP port to bind the service to | `8000` | +| `BIND_ADDR` | `--bind` | Network interface to bind to | all (ipv4 and ipv6) | +| `DATA_DIR` | `--data-dir` | Directory for persistent data | `./data` | +| `SERVER_URL` | `--server-url`, `-s` | External URL of this service | `http://:8000` | +| `DEPLOYMENT_MODE` | `--deployment-mode` | Where this service runs: `on-device`, `private-network`, or `public-network`. Only changes behavior when `SERVER_URL` is *not* set: `on-device` defaults to `http://localhost:` instead of guessing a hostname (the speaker's own Linux hostname is never resolvable — see issue #546); `public-network` refuses to start rather than guess a publicly reachable address; unset/`private-network` keeps the previous hostname-guessing behavior, now with a startup warning. The on-device install script sets this automatically. | unset (legacy hostname guess, with warning) | +| `HTTPS_PORT` | `--https-port` | HTTPS port to bind the service to | `8443` | +| `HTTPS_SERVER_URL` | `--https-server-url`, `-S` | External HTTPS URL. An override: when empty it is derived from `SERVER_URL` (same host, `https`, on `HTTPS_PORT`), and can also be viewed/overridden in Settings. | derived from `SERVER_URL` | +| `PYTHON_BACKEND_URL`, `TARGET_URL` | `--target-url` | URL for Python-based service components (legacy) | `http://localhost:8001` | +| `REDACT_PROXY_LOGS` | `--redact-logs` | Redact sensitive data in proxy logs | `true` | +| `LOG_PROXY_BODY` | `--log-bodies` | Log full request/response bodies | `false` | +| `RECORD_INTERACTIONS` | `--record-interactions` | Record HTTP interactions to disk | `true` | +| `DISCOVERY_ENABLED` | `--discovery-enabled` | Enable periodic device discovery | `true` | +| `DISCOVERY_INTERVAL` | `--discovery-interval` | Device discovery interval | `5m` | +| `DEVICE_SEED_RETRY_INTERVAL` | `--device-seed-retry-interval` | Interval between embedded-player startup retries for persisted devices that failed their first probe (e.g. LAN not yet routable on a cold boot) | `30s` | +| `DEVICE_SEED_RETRY_WINDOW` | `--device-seed-retry-window` | Bounded window during which the embedded player retries those unreachable persisted devices at startup | `10m` | +| `ENABLE_DNS_DISCOVERY` | `--dns-discovery` | Enable DNS discovery server | `false` | +| `DNS_UPSTREAM` | `--dns-upstream` | Upstream DNS server for non-Bose queries | `8.8.8.8` | +| `DNS_BIND_ADDR` | `--dns-bind` | Bind address for the DNS discovery server (standard port `:53` is required for DNS/DHCP migration) | `:53` | +| `INTERNAL_PATHS` | `--internal-paths` | Paths for internal requests to exclude from recording (e.g., `/setup/*`, `/web/*`) | `[]` | +| `UPDATE_CHECK_ENABLED` | `--update-check-enabled` | Periodically check GitHub Releases for a newer version and show a dismissible notice in the admin UI and Player when one is found. **Opt-in**: this is the only network call AfterTouch makes beyond speaker/provider traffic when enabled, so it defaults off. One unauthenticated `GET` per interval to `api.github.com`, nothing else leaves the box. Also available as an "Update Check" toggle on the admin Settings page, which applies without a restart; the env var/flag is the seed value for a fresh install with no `settings.json` yet. | `false` | +| `UPDATE_CHECK_INTERVAL` | `--update-check-interval` | Update check interval. Also editable on the admin Settings page (applies without a restart). | `24h` | +| `MGMT_USERNAME` | `--mgmt-username` | Username for HTTP Basic Auth on the Management API (`/api/mgmt/*`, `/mgmt/*`) — Spotify/Amazon account linking, Local Accounts | `admin` | +| `MGMT_PASSWORD` | `--mgmt-password` | Password for the same Management API Basic Auth. **Change this if AfterTouch is reachable beyond a trusted LAN** — the default is published in this doc. | `change_me!` | +| `STOCKHOLM_DIR` | `--stockholm-dir` | Path to extracted Stockholm frontend directory — enables the Stockholm UI when set | *(disabled)* | +| `MARGE_URL` | | Streaming/marge base URL used when rewriting `stockholm/json/config.json`. Defaults to `SERVER_URL`. Set to `SERVER_URL/marge` only when using a soundcork backend. | *(same as `SERVER_URL`)* | +| `MARGE_AUTH_TOKEN` | | Pre-seeds the Stockholm `margeAuthToken` state (skips the login step for the first session) | *(empty)* | +| `MARGE_ACCOUNT_ID` | | Pre-seeds the Stockholm `margeAccountID` state (used to filter device-discovery results by account) | *(empty)* | ### Configuration Examples diff --git a/pkg/service/soundtouchweb/discovery.go b/pkg/service/soundtouchweb/discovery.go index 71e5681a..3dcbd077 100644 --- a/pkg/service/soundtouchweb/discovery.go +++ b/pkg/service/soundtouchweb/discovery.go @@ -123,7 +123,8 @@ func (app *WebApp) addDeviceByHost(host string, port int, source string) *webtyp } // SeedExtraDevices registers any devices reported by the ExtraDeviceHosts hook -// (if set) via AddDeviceByHost. Idempotent: already-known hosts are skipped. +// (if set) via AddDeviceByHost, and prunes any previously-seeded host that no +// longer appears in that set. Idempotent: already-known hosts are skipped. // Used by the embedded build to surface the service datastore's devices even // when network discovery is disabled; a no-op for standalone soundtouch-player. // @@ -132,8 +133,14 @@ func (app *WebApp) addDeviceByHost(host string, port int, source string) *webtyp // datastore would otherwise stall the whole seed for 10 s, serially. Fanning // out bounds the cost to roughly a single timeout regardless of how many // devices are offline. AddDeviceByHost is registry-safe under concurrency. +// +// A hook read failure is logged and otherwise swallowed here; callers that +// need to distinguish "read failed" from "converged" (the bounded startup +// retry) should call seedExtraDevices directly instead. func (app *WebApp) SeedExtraDevices() { - app.seedExtraDevices() + if _, _, _, err := app.seedExtraDevices(); err != nil { + log.Printf("SeedExtraDevices: failed to read extra device hosts: %v", err) + } } type seededExtraDevice struct { @@ -141,17 +148,40 @@ type seededExtraDevice struct { conn *webtypes.DeviceConnection } -func (app *WebApp) seedExtraDevices() []seededExtraDevice { +// seedExtraDevices probes any ExtraDeviceHosts hosts that aren't already +// registered, and prunes any registered host that's no longer in the current +// desired set. Pruning is safe to apply to the whole registry (not just hosts +// this call inserted) because ExtraDeviceHosts is the only inserter into this +// registry for the embedded build: discoveryService is nil there, so the +// mDNS/UPnP insertion path in DiscoverDevices is never reached. +// +// Runs are serialized via seedMu so the bounded startup retry loop +// (SeedExtraDevicesUntilReady) and a devices-changed-hook-triggered +// SeedExtraDevices call never issue concurrent probes to the same offline +// host. +// +// A non-nil error means the hook itself failed (e.g. a datastore glitch); +// callers must treat that as "unknown state, don't prune, don't declare +// ready" rather than as an empty desired set. +func (app *WebApp) seedExtraDevices() (inserted []seededExtraDevice, removed int, desired map[string]struct{}, err error) { if app.ExtraDeviceHosts == nil { - return nil + return nil, 0, nil, nil } - hosts := app.extraDeviceHosts() - inserted := make(chan seededExtraDevice, len(hosts)) + app.seedMu.Lock() + defer app.seedMu.Unlock() - var wg sync.WaitGroup + desired, err = app.extraDeviceHostSet() + if err != nil { + return nil, 0, nil, err + } - for _, host := range hosts { + var ( + mu sync.Mutex + wg sync.WaitGroup + ) + + for host := range desired { if _, ok := app.GetDevice(host); ok { continue } @@ -161,21 +191,32 @@ func (app *WebApp) seedExtraDevices() []seededExtraDevice { go func(h string) { defer wg.Done() - if conn := app.addDeviceByHost(h, 8090, "service-store"); conn != nil { - inserted <- seededExtraDevice{host: h, conn: conn} + conn := app.addDeviceByHost(h, 8090, "service-store") + if conn == nil { + return } + + mu.Lock() + + inserted = append(inserted, seededExtraDevice{host: h, conn: conn}) + + mu.Unlock() }(host) } wg.Wait() - close(inserted) - added := make([]seededExtraDevice, 0, len(inserted)) - for device := range inserted { - added = append(added, device) + for _, entry := range app.DeviceSnapshot() { + if _, ok := desired[entry.ID]; ok { + continue + } + + if app.removeDeviceIfMatch(entry.ID, entry.Device) { + removed++ + } } - return added + return inserted, removed, desired, nil } // SeedExtraDevicesUntilReady retries only the hosts returned by @@ -185,32 +226,34 @@ func (app *WebApp) seedExtraDevices() []seededExtraDevice { // while the service is starting. func (app *WebApp) SeedExtraDevicesUntilReady(ctx context.Context, retryInterval time.Duration) { retryUntilReady(ctx, retryInterval, func() bool { - inserted := app.seedExtraDevices() - desired := app.extraDeviceHostSet() - - for _, device := range inserted { - if _, ok := desired[device.host]; !ok { - app.removeDeviceIfMatch(device.host, device.conn) - } + inserted, removed, desired, err := app.seedExtraDevices() + if err != nil { + log.Printf("SeedExtraDevicesUntilReady: failed to read extra device hosts, will retry: %v", err) + return false } - if len(inserted) > 0 { + if len(inserted) > 0 || removed > 0 { app.BroadcastDeviceList() } - return app.extraDeviceHostsPresent(app.extraDeviceHostSet()) + return app.extraDeviceHostsPresent(desired) }) } -func (app *WebApp) extraDeviceHosts() []string { +func (app *WebApp) extraDeviceHosts() ([]string, error) { if app.ExtraDeviceHosts == nil { - return nil + return nil, nil } - hosts := make([]string, 0) - seen := make(map[string]struct{}) + rawHosts, err := app.ExtraDeviceHosts() + if err != nil { + return nil, err + } - for _, host := range app.ExtraDeviceHosts() { + hosts := make([]string, 0, len(rawHosts)) + seen := make(map[string]struct{}, len(rawHosts)) + + for _, host := range rawHosts { if host == "" { continue } @@ -223,18 +266,22 @@ func (app *WebApp) extraDeviceHosts() []string { hosts = append(hosts, host) } - return hosts + return hosts, nil } -func (app *WebApp) extraDeviceHostSet() map[string]struct{} { - hosts := app.extraDeviceHosts() +func (app *WebApp) extraDeviceHostSet() (map[string]struct{}, error) { + hosts, err := app.extraDeviceHosts() + if err != nil { + return nil, err + } + desired := make(map[string]struct{}, len(hosts)) for _, host := range hosts { desired[host] = struct{}{} } - return desired + return desired, nil } func (app *WebApp) extraDeviceHostsPresent(desired map[string]struct{}) bool { diff --git a/pkg/service/soundtouchweb/discovery_test.go b/pkg/service/soundtouchweb/discovery_test.go index 9492e7c0..909f2e7a 100644 --- a/pkg/service/soundtouchweb/discovery_test.go +++ b/pkg/service/soundtouchweb/discovery_test.go @@ -2,6 +2,7 @@ package soundtouchweb import ( "context" + "errors" "net/http" "net/http/httptest" "strings" @@ -131,9 +132,12 @@ func TestRetryUntilReadyStopsAfterContextCancellation(t *testing.T) { func TestExtraDeviceHostsPresent(t *testing.T) { app := NewWebApp() - app.ExtraDeviceHosts = func() []string { return []string{"known", "", "known", "missing"} } + app.ExtraDeviceHosts = func() ([]string, error) { return []string{"known", "", "known", "missing"}, nil } app.AddDevice("known", &webtypes.DeviceConnection{}) - desired := app.extraDeviceHostSet() + desired, err := app.extraDeviceHostSet() + if err != nil { + t.Fatalf("extraDeviceHostSet() error = %v", err) + } if len(desired) != 2 { t.Fatalf("desired host count = %d, want 2", len(desired)) @@ -148,9 +152,19 @@ func TestExtraDeviceHostsPresent(t *testing.T) { } } +func TestExtraDeviceHostSetPropagatesHookError(t *testing.T) { + app := NewWebApp() + wantErr := errors.New("datastore glitch") + app.ExtraDeviceHosts = func() ([]string, error) { return nil, wantErr } + + if _, err := app.extraDeviceHostSet(); !errors.Is(err, wantErr) { + t.Fatalf("extraDeviceHostSet() error = %v, want %v", err, wantErr) + } +} + func TestSeedExtraDevicesSkipsKnownHosts(t *testing.T) { app := NewWebApp() - app.ExtraDeviceHosts = func() []string { return []string{"known"} } + app.ExtraDeviceHosts = func() ([]string, error) { return []string{"known"}, nil } lastSeen := time.Unix(123, 0) conn := &webtypes.DeviceConnection{LastSeen: lastSeen} app.AddDevice("known", conn) @@ -162,6 +176,117 @@ func TestSeedExtraDevicesSkipsKnownHosts(t *testing.T) { } } +// TestSeedExtraDevicesUntilReadyRetriesOnHookError covers the code-review +// finding that a hook error (e.g. a transient datastore read failure) must +// not be treated as "zero hosts persisted", which would make the readiness +// check trivially pass and end the retry window immediately. +func TestSeedExtraDevicesUntilReadyRetriesOnHookError(t *testing.T) { + var calls atomic.Int32 + + app := NewWebApp() + app.ExtraDeviceHosts = func() ([]string, error) { + n := calls.Add(1) + if n < 3 { + return nil, errors.New("datastore glitch") + } + + return nil, nil + } + + ctx, cancel := context.WithTimeout(context.Background(), time.Second) + defer cancel() + + app.SeedExtraDevicesUntilReady(ctx, time.Millisecond) + + if got := calls.Load(); got < 3 { + t.Fatalf("hook call count = %d, want at least 3 (kept retrying past the errors)", got) + } +} + +// TestSeedExtraDevicesPrunesHostRemovedAfterEarlierAttempt covers the +// code-review finding that pruning must consider the whole registry, not +// only hosts inserted during the current call: a host registered by an +// earlier seed call that later falls out of ExtraDeviceHosts must still be +// pruned by a later call, and this must hold for the plain SeedExtraDevices +// path too, not just the bounded retry loop. +func TestSeedExtraDevicesPrunesHostRemovedAfterEarlierAttempt(t *testing.T) { + server := httptest.NewTestServer(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/xml") + _, _ = w.Write([]byte(`Stale speakerSoundTouch 10`)) + })) + server.Start() + + host := strings.TrimPrefix(server.URL, "http://") + + var stillDesired atomic.Bool + stillDesired.Store(true) + + app := NewWebApp() + app.ExtraDeviceHosts = func() ([]string, error) { + if stillDesired.Load() { + return []string{host}, nil + } + + return nil, nil + } + + // First call registers the host. + app.SeedExtraDevices() + if _, ok := app.GetDevice(host); !ok { + t.Fatalf("host %s was not registered on the first seed call", host) + } + + // Simulate the device being removed from the datastore in between calls. + stillDesired.Store(false) + + // A later plain SeedExtraDevices call (as triggered by + // SetDevicesChangedHook or the manual /api/control/discover route) must + // still prune it, not just the bounded retry loop. + app.SeedExtraDevices() + if _, ok := app.GetDevice(host); ok { + t.Fatal("stale host was not pruned by a later SeedExtraDevices call") + } +} + +// TestSeedExtraDevicesSerializesConcurrentRuns covers the code-review finding +// that the bounded startup retry loop and a devices-changed-hook-triggered +// SeedExtraDevices call must not issue concurrent probes to the same +// still-offline host. +func TestSeedExtraDevicesSerializesConcurrentRuns(t *testing.T) { + var infoRequests atomic.Int32 + release := make(chan struct{}) + + server := httptest.NewTestServer(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + infoRequests.Add(1) + <-release // block until the test lets the handler respond + + w.Header().Set("Content-Type", "application/xml") + _, _ = w.Write([]byte(`Slow speakerSoundTouch 10`)) + })) + server.Start() + + host := strings.TrimPrefix(server.URL, "http://") + + app := NewWebApp() + app.ExtraDeviceHosts = func() ([]string, error) { return []string{host}, nil } + + done := make(chan struct{}, 2) + go func() { app.SeedExtraDevices(); done <- struct{}{} }() + go func() { app.SeedExtraDevices(); done <- struct{}{} }() + + // Give both goroutines a moment to reach the handler if they were going + // to run concurrently, then let the handler(s) respond. + time.Sleep(50 * time.Millisecond) + close(release) + + <-done + <-done + + if got := infoRequests.Load(); got != 1 { + t.Fatalf("/info request count = %d, want 1 (concurrent seeds were not serialized)", got) + } +} + func TestRemoveDeviceIfMatchKeepsReplacement(t *testing.T) { app := NewWebApp() original := webtypes.NewDeviceConnection(nil, &models.DeviceInfo{}) diff --git a/pkg/service/soundtouchweb/handler.go b/pkg/service/soundtouchweb/handler.go index 6964de72..3591ef66 100644 --- a/pkg/service/soundtouchweb/handler.go +++ b/pkg/service/soundtouchweb/handler.go @@ -62,7 +62,12 @@ type WebApp struct { // soundtouch-service points it at the service datastore's known devices so // the UI shows manually-added speakers even when network discovery is // disabled. Standalone soundtouch-player leaves it nil. - ExtraDeviceHosts func() []string + // + // A non-nil error means the underlying read failed (e.g. a datastore + // glitch), which callers must NOT treat the same as "zero hosts + // persisted" -- doing so would make a transient read failure look like + // every persisted device is already registered. + ExtraDeviceHosts func() ([]string, error) // TriggerDiscovery, when set, runs an external discovery sweep instead of // this app's own mDNS/UPnP. The embedded build wires it to the host @@ -78,6 +83,11 @@ type WebApp struct { // removal only prunes the in-memory registry). RemoveDeviceHook func(deviceID string) error + // seedMu serializes seedExtraDevices runs so the bounded startup retry + // loop (SeedExtraDevicesUntilReady) and a devices-changed-hook-triggered + // SeedExtraDevices never probe the same still-offline host concurrently. + seedMu sync.Mutex + discoveryStatus atomic.Value // stores *webtypes.DiscoveryStatus }