diff --git a/cmd/soundtouch-service/main.go b/cmd/soundtouch-service/main.go index d9e3adf..1cae83f 100644 --- a/cmd/soundtouch-service/main.go +++ b/cmd/soundtouch-service/main.go @@ -537,6 +537,7 @@ func main() { server.SetExpectedHosts(config.domains) server.SetVersionInfo(version, commit, date, repoURL) server.SetDiscoverySettings(config.discoveryInterval, config.discoveryEnabled) + server.SetUpdateCheckSettings(config.updateCheckInterval, config.updateCheckEnabled) server.SetDNSSettings(persisted.DNSEnabled, strings.Join(persisted.DNSUpstream, ","), persisted.DNSBindAddr) server.SetInternalPaths(persisted.InternalPaths) server.SetSpotifyConfig(config.spotifyClientID, config.spotifyClientSecret, config.spotifyRedirectURI) @@ -616,7 +617,7 @@ func main() { updateChecker := updatecheck.NewChecker(ds, "gesellix/Bose-SoundTouch", version) server.SetUpdateChecker(updateChecker) - startUpdateCheck(updateChecker, config.updateCheckEnabled, config.updateCheckInterval) + startUpdateCheck(server, updateChecker) var stockholmHandler *stockholm.Handler @@ -1021,6 +1022,22 @@ func applyPersistedSettings(ds *datastore.DataStore, config *serviceConfig) data } } + // Installs upgraded from a build that predates the Settings-page toggle + // have no update_check_* keys at all, and an absent JSON bool decodes as + // false — taking it at face value would silently switch the check off for + // everyone who had opted in via UPDATE_CHECK_ENABLED. The interval is + // always written together with the flag (createDefaultSettings and + // HandleUpdateSettings both set both, and a time.Duration never + // stringifies to ""), so a non-empty interval is the marker for + // "settings.json genuinely carries an update-check preference". + if persisted.UpdateCheckInterval != "" { + config.updateCheckEnabled = persisted.UpdateCheckEnabled + + if d, durErr := time.ParseDuration(persisted.UpdateCheckInterval); durErr == nil { + config.updateCheckInterval = d + } + } + config.redact = persisted.RedactLogs config.logBody = persisted.LogBodies config.record = persisted.RecordInteractions @@ -1138,10 +1155,15 @@ func createDefaultSettings(ds *datastore.DataStore, config serviceConfig) datast RecordInteractions: config.record, DiscoveryEnabled: config.discoveryEnabled, DiscoveryInterval: config.discoveryInterval.String(), - DNSEnabled: config.dnsEnabled, - DNSUpstream: strings.Split(config.dnsUpstream, ","), - DNSBindAddr: config.dnsBind, - InternalPaths: config.internalPaths, + // Seed the update-check preference from the CLI/env flags so a fresh + // install's settings.json matches what the operator asked for (and so + // the Settings page shows it) instead of silently reverting to off. + UpdateCheckEnabled: config.updateCheckEnabled, + UpdateCheckInterval: config.updateCheckInterval.String(), + DNSEnabled: config.dnsEnabled, + DNSUpstream: strings.Split(config.dnsUpstream, ","), + DNSBindAddr: config.dnsBind, + InternalPaths: config.internalPaths, Shortcuts: map[string]int{ "/.well-known/appspecific/com.chrome.devtools.json": http.StatusNotFound, "/sw.js": http.StatusNotFound, @@ -1230,16 +1252,45 @@ func startDeviceDiscovery(server *handlers.Server) { }() } -// startUpdateCheck runs the opt-in periodic check against GitHub Releases -// in the background (#591, _/i591/design-update-check.md). Unlike -// discovery, enabled/interval are fixed at startup (env-only for v1, no -// Settings-tab control — see the design doc's answer on that trade-off), -// so they're plain arguments, not read live from Settings each tick. -func startUpdateCheck(checker *updatecheck.Checker, enabled bool, interval time.Duration) { - if !enabled { - return - } +// updateCheckPollTick is how often the background update-check goroutine +// re-reads the live settings. It is deliberately much shorter than the +// check interval itself: sleeping a full (possibly 24h) interval between +// reads would make flipping the Settings-page toggle on appear to do +// nothing for up to a day. +const updateCheckPollTick = time.Minute +// shouldRunUpdateCheckNow reports whether the background goroutine should +// perform a real GitHub request on this poll tick. Pure/testable — no +// sleeping, no I/O. +// +// A zero interval is treated as "don't check": with interval 0 every tick +// would look due (shouldCheckImmediately), so an enabled check would hit +// GitHub once a minute forever. HandleUpdateSettings also refuses to keep +// the check enabled with a zero interval; this is the same guard for values +// that arrive via the CLI flag or a hand-edited settings.json. +func shouldRunUpdateCheckNow( + enabled bool, + lastCheckedAt time.Time, + interval time.Duration, + lastErrorAt, now time.Time, +) bool { + return enabled && + interval > 0 && + shouldCheckImmediately(lastCheckedAt, interval, now) && + !shouldSkipDueToBackoff(lastErrorAt, now) +} + +// startUpdateCheck runs the opt-in periodic check against GitHub Releases in +// the background (#591, _/i591/design-update-check.md). Unlike the original +// v1 design, enabled/interval are now live-reloadable from Settings (see +// handlers.Server.SetUpdateCheckSettings) — so this goroutine always runs, +// mirroring startDeviceDiscovery's live-settings pattern, and re-reads the +// current settings every updateCheckPollTick. It only performs the actual +// GitHub request when the check is enabled and the configured interval has +// elapsed since the last check, so "always running" does not mean "always +// talking to GitHub": with the check disabled it does nothing but wake up +// once a minute and go back to sleep. +func startUpdateCheck(server *handlers.Server, checker *updatecheck.Checker) { go func() { time.Sleep(randomJitter(5 * time.Minute)) @@ -1248,18 +1299,15 @@ func startUpdateCheck(checker *updatecheck.Checker, enabled bool, interval time. var lastErrorAt time.Time - if shouldCheckImmediately(lastResult.CheckedAt, interval, time.Now()) { - lastErrorAt, lastLoggedVersion = runUpdateCheckTick(checker, lastLoggedVersion) - } - for { - time.Sleep(interval) + interval, enabled := server.GetUpdateCheckSettings() - if shouldSkipDueToBackoff(lastErrorAt, time.Now()) { - continue + if shouldRunUpdateCheckNow(enabled, lastResult.CheckedAt, interval, lastErrorAt, time.Now()) { + lastErrorAt, lastLoggedVersion = runUpdateCheckTick(checker, lastLoggedVersion) + lastResult = checker.LastResult() } - lastErrorAt, lastLoggedVersion = runUpdateCheckTick(checker, lastLoggedVersion) + time.Sleep(updateCheckPollTick) } }() } diff --git a/cmd/soundtouch-service/update_check_test.go b/cmd/soundtouch-service/update_check_test.go index 04253a4..f330f06 100644 --- a/cmd/soundtouch-service/update_check_test.go +++ b/cmd/soundtouch-service/update_check_test.go @@ -104,9 +104,53 @@ func TestRandomJitter(t *testing.T) { } } -func TestStartUpdateCheck_DisabledIsANoOp(t *testing.T) { - // Must return immediately without spawning anything that could touch a - // nil-repo Checker or block — disabled is the default, so this path - // runs on every install that hasn't opted in. - startUpdateCheck(nil, false, time.Hour) +// There is deliberately no test for startUpdateCheck itself, matching +// startDeviceDiscovery (its equally untested sibling): both are thin, +// forever-looping goroutine wrappers whose only decisions live in pure +// helpers, which is what the tests above and below cover. The former +// TestStartUpdateCheck_DisabledIsANoOp asserted a contract that no longer +// exists — the goroutine now always starts, precisely so that enabling the +// check from the Settings page takes effect without a restart, and an +// early return for "disabled" would defeat that. +func TestShouldRunUpdateCheckNow(t *testing.T) { + now := time.Date(2026, 8, 10, 12, 0, 0, 0, time.UTC) + interval := 24 * time.Hour + stale := now.Add(-25 * time.Hour) + fresh := now.Add(-1 * time.Hour) + + cases := []struct { + name string + enabled bool + lastCheckedAt time.Time + interval time.Duration + lastErrorAt time.Time + want bool + }{ + {"disabled, never checked", false, time.Time{}, interval, time.Time{}, false}, + {"disabled, due", false, stale, interval, time.Time{}, false}, + {"enabled, never checked", true, time.Time{}, interval, time.Time{}, true}, + {"enabled, due", true, stale, interval, time.Time{}, true}, + {"enabled, not due yet", true, fresh, interval, time.Time{}, false}, + {"enabled and due, but in error backoff", true, stale, interval, now.Add(-30 * time.Minute), false}, + {"enabled and due, backoff expired", true, stale, interval, now.Add(-2 * time.Hour), true}, + // A zero interval must not turn every poll tick into a GitHub request. + {"enabled with a zero interval", true, stale, 0, time.Time{}, false}, + } + + for _, tc := range cases { + got := shouldRunUpdateCheckNow(tc.enabled, tc.lastCheckedAt, tc.interval, tc.lastErrorAt, now) + if got != tc.want { + t.Errorf("%s: shouldRunUpdateCheckNow() = %v, want %v", tc.name, got, tc.want) + } + } +} + +// TestUpdateCheckPollTickIsShorterThanTheDefaultInterval guards the property +// that makes the Settings-page toggle feel live: the goroutine must re-read +// the settings far more often than the check interval itself, otherwise +// switching the check on would appear to do nothing for up to a day. +func TestUpdateCheckPollTickIsShorterThanTheDefaultInterval(t *testing.T) { + if updateCheckPollTick >= 24*time.Hour { + t.Errorf("updateCheckPollTick = %v, want well below the 24h default interval", updateCheckPollTick) + } } diff --git a/docs/content/docs/guides/SOUNDTOUCH-SERVICE.md b/docs/content/docs/guides/SOUNDTOUCH-SERVICE.md index 19b2817..fb21853 100644 --- a/docs/content/docs/guides/SOUNDTOUCH-SERVICE.md +++ b/docs/content/docs/guides/SOUNDTOUCH-SERVICE.md @@ -174,8 +174,8 @@ The service supports multiple ways to configure its behavior. When multiple sour | `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. | `false` | -| `UPDATE_CHECK_INTERVAL` | `--update-check-interval` | Update check interval | `24h` | +| `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)* | diff --git a/pkg/service/datastore/datastore.go b/pkg/service/datastore/datastore.go index db80b2a..3f1bb55 100644 --- a/pkg/service/datastore/datastore.go +++ b/pkg/service/datastore/datastore.go @@ -2594,6 +2594,8 @@ type Settings struct { RecordInteractions bool `json:"record_interactions"` DiscoveryInterval string `json:"discovery_interval,omitempty"` DiscoveryEnabled bool `json:"discovery_enabled"` + UpdateCheckInterval string `json:"update_check_interval,omitempty"` + UpdateCheckEnabled bool `json:"update_check_enabled"` DNSEnabled bool `json:"dns_enabled"` DNSUpstream []string `json:"dns_upstream,omitempty"` DNSBindAddr string `json:"dns_bind_addr,omitempty"` diff --git a/pkg/service/datastore/datastore_test.go b/pkg/service/datastore/datastore_test.go index 3ae806b..b2d5b55 100644 --- a/pkg/service/datastore/datastore_test.go +++ b/pkg/service/datastore/datastore_test.go @@ -428,10 +428,12 @@ func TestSettingsPersistence(t *testing.T) { ds := NewDataStore(tempDir) settings := Settings{ - ServerURL: "http://myserver:8000", - LogBodies: true, - DiscoveryInterval: "10m", - DiscoveryEnabled: true, + ServerURL: "http://myserver:8000", + LogBodies: true, + DiscoveryInterval: "10m", + DiscoveryEnabled: true, + UpdateCheckInterval: "12h", + UpdateCheckEnabled: true, } err = ds.SaveSettings(settings) @@ -456,6 +458,12 @@ func TestSettingsPersistence(t *testing.T) { if loaded.DiscoveryEnabled != settings.DiscoveryEnabled { t.Errorf("Expected DiscoveryEnabled %v, got %v", settings.DiscoveryEnabled, loaded.DiscoveryEnabled) } + if loaded.UpdateCheckInterval != settings.UpdateCheckInterval { + t.Errorf("Expected UpdateCheckInterval %s, got %s", settings.UpdateCheckInterval, loaded.UpdateCheckInterval) + } + if loaded.UpdateCheckEnabled != settings.UpdateCheckEnabled { + t.Errorf("Expected UpdateCheckEnabled %v, got %v", settings.UpdateCheckEnabled, loaded.UpdateCheckEnabled) + } } // TestUpdateCheckState_MissingFileReturnsZeroValue verifies a fresh install diff --git a/pkg/service/handlers/handlers_setup.go b/pkg/service/handlers/handlers_setup.go index aaf409e..8de9ae0 100644 --- a/pkg/service/handlers/handlers_setup.go +++ b/pkg/service/handlers/handlers_setup.go @@ -167,6 +167,11 @@ func (s *Server) HandleGetSettings(w http.ResponseWriter, _ *http.Request) { httpsOverride := s.httpsOverride discoveryInterval := s.discoveryInterval.String() discoveryEnabled := s.discoveryEnabled + // Read the update-check fields directly rather than via + // GetUpdateCheckSettings(): that getter takes s.mu.RLock itself, and Go's + // sync.RWMutex is not reentrant-safe against a concurrent writer. + updateCheckInterval := s.updateCheckInterval.String() + updateCheckEnabled := s.updateCheckEnabled dnsEnabled := s.dnsEnabled dnsUpstream := s.dnsUpstream dnsBindAddr := s.dnsBindAddr @@ -248,6 +253,8 @@ func (s *Server) HandleGetSettings(w http.ResponseWriter, _ *http.Request) { "https_443_lan_host": probe443.LANHost, "discovery_interval": discoveryInterval, "discovery_enabled": discoveryEnabled, + "update_check_interval": updateCheckInterval, + "update_check_enabled": updateCheckEnabled, "dns_enabled": dnsEnabled, "dns_running": dnsRunning, "dns_actual_bind": actualBind, @@ -300,6 +307,40 @@ func parseDNSUpstreamList(dnsUpstream string) []string { return upstreamList } +// parseOptionalDuration parses a duration string that the client is allowed to +// omit. An empty value yields a zero duration and no error, so callers can +// treat "field omitted" as "keep the current value" while still rejecting a +// value that was supplied but is unparseable. +func parseOptionalDuration(value string) (time.Duration, error) { + if value == "" { + return 0, nil + } + + return time.ParseDuration(value) +} + +// resolvePeriodicSetting computes the new (interval, enabled) pair for one of +// the background pollers (device discovery, update check) from a settings +// request. When the request omitted the interval, the current one is kept. A +// zero interval always forces the task off: both pollers treat zero as +// "always due", so leaving the task enabled would make their poll tick the +// work rate. +func resolvePeriodicSetting( + currentInterval, requestedInterval time.Duration, + requestedIntervalProvided, requestedEnabled bool, +) (time.Duration, bool) { + interval := currentInterval + if requestedIntervalProvided { + interval = requestedInterval + } + + if interval == 0 { + return interval, false + } + + return interval, requestedEnabled +} + // HandleUpdateSettings updates the service settings. func (s *Server) HandleUpdateSettings(w http.ResponseWriter, r *http.Request) { var settings struct { @@ -307,6 +348,8 @@ func (s *Server) HandleUpdateSettings(w http.ResponseWriter, r *http.Request) { HTTPSServerURLOverride *string `json:"https_server_url_override"` DiscoveryInterval string `json:"discovery_interval"` DiscoveryEnabled bool `json:"discovery_enabled"` + UpdateCheckInterval string `json:"update_check_interval"` + UpdateCheckEnabled bool `json:"update_check_enabled"` DNSEnabled bool `json:"dns_enabled"` DNSUpstream string `json:"dns_upstream"` DNSBindAddr string `json:"dns_bind_addr"` @@ -370,12 +413,18 @@ func (s *Server) HandleUpdateSettings(w http.ResponseWriter, r *http.Request) { return } - interval, err := time.ParseDuration(settings.DiscoveryInterval) - if err != nil && settings.DiscoveryInterval != "" { + interval, err := parseOptionalDuration(settings.DiscoveryInterval) + if err != nil { http.Error(w, "Invalid discovery interval: "+err.Error(), http.StatusBadRequest) return } + updateCheckInterval, err := parseOptionalDuration(settings.UpdateCheckInterval) + if err != nil { + http.Error(w, "Invalid update check interval: "+err.Error(), http.StatusBadRequest) + return + } + s.mu.Lock() // Guard rail: refuse to enable the admin-area gate while the Management @@ -396,14 +445,11 @@ func (s *Server) HandleUpdateSettings(w http.ResponseWriter, r *http.Request) { // the Target Domain (which the derived URL follows) may have changed. s.applyHTTPSOverrideLocked(settings.HTTPSServerURLOverride) - s.discoveryEnabled = settings.DiscoveryEnabled - if settings.DiscoveryInterval != "" { - s.discoveryInterval = interval - } + s.discoveryInterval, s.discoveryEnabled = resolvePeriodicSetting( + s.discoveryInterval, interval, settings.DiscoveryInterval != "", settings.DiscoveryEnabled) - if s.discoveryInterval == 0 { - s.discoveryEnabled = false - } + s.updateCheckInterval, s.updateCheckEnabled = resolvePeriodicSetting( + s.updateCheckInterval, updateCheckInterval, settings.UpdateCheckInterval != "", settings.UpdateCheckEnabled) s.dnsEnabled = settings.DNSEnabled s.dnsUpstream = parseDNSUpstreamList(settings.DNSUpstream) @@ -469,6 +515,8 @@ func (s *Server) HandleUpdateSettings(w http.ResponseWriter, r *http.Request) { persisted.RecordInteractions = currentRecord persisted.DiscoveryInterval = s.discoveryInterval.String() persisted.DiscoveryEnabled = s.discoveryEnabled + persisted.UpdateCheckInterval = s.updateCheckInterval.String() + persisted.UpdateCheckEnabled = s.updateCheckEnabled persisted.DNSEnabled = s.dnsEnabled persisted.DNSUpstream = s.dnsUpstream persisted.DNSBindAddr = s.dnsBindAddr diff --git a/pkg/service/handlers/handlers_setup_test.go b/pkg/service/handlers/handlers_setup_test.go index 55242f3..81573a6 100644 --- a/pkg/service/handlers/handlers_setup_test.go +++ b/pkg/service/handlers/handlers_setup_test.go @@ -10,6 +10,7 @@ import ( "path/filepath" "strings" "testing" + "time" "github.com/gesellix/bose-soundtouch/pkg/models" "github.com/gesellix/bose-soundtouch/pkg/service/certmanager" @@ -443,6 +444,170 @@ func TestAdminAreaAuthRoundTrip(t *testing.T) { } } +// TestResolvePeriodicSetting covers the shared interval/enabled resolution +// used by both background pollers (device discovery, update check). +func TestResolvePeriodicSetting(t *testing.T) { + cases := []struct { + name string + current time.Duration + requested time.Duration + provided bool + enabled bool + wantInterval time.Duration + wantEnabledState bool + }{ + {"interval omitted keeps the current one", 24 * time.Hour, 0, false, true, 24 * time.Hour, true}, + {"interval supplied replaces the current one", 24 * time.Hour, 6 * time.Hour, true, true, 6 * time.Hour, true}, + {"disabling keeps the interval", 24 * time.Hour, 0, false, false, 24 * time.Hour, false}, + {"a zero interval forces it off", 24 * time.Hour, 0, true, true, 0, false}, + {"a zero current interval forces it off too", 0, 0, false, true, 0, false}, + } + + for _, tc := range cases { + gotInterval, gotEnabled := resolvePeriodicSetting(tc.current, tc.requested, tc.provided, tc.enabled) + if gotInterval != tc.wantInterval || gotEnabled != tc.wantEnabledState { + t.Errorf("%s: resolvePeriodicSetting() = %v/%v, want %v/%v", + tc.name, gotInterval, gotEnabled, tc.wantInterval, tc.wantEnabledState) + } + } +} + +// TestParseOptionalDuration verifies an omitted duration is not an error, +// while a supplied-but-invalid one is. +func TestParseOptionalDuration(t *testing.T) { + if d, err := parseOptionalDuration(""); err != nil || d != 0 { + t.Errorf("parseOptionalDuration(\"\") = %v/%v, want 0/nil", d, err) + } + + if d, err := parseOptionalDuration("90m"); err != nil || d != 90*time.Minute { + t.Errorf("parseOptionalDuration(\"90m\") = %v/%v, want 1h30m0s/nil", d, err) + } + + if _, err := parseOptionalDuration("nope"); err == nil { + t.Error("parseOptionalDuration(\"nope\") = nil error, want a parse error") + } +} + +// TestUpdateCheckSettingsRoundTrip covers the Settings-page control for the +// opt-in update check (#591 follow-up): POST /setup/settings must update the +// live values the background poller reads, persist them, and hand them back +// on GET so the UI reflects what was saved. +func TestUpdateCheckSettingsRoundTrip(t *testing.T) { + tempDir, err := os.MkdirTemp("", "update-check-settings-roundtrip-test") + if err != nil { + t.Fatalf("Failed to create temp dir: %v", err) + } + defer os.RemoveAll(tempDir) + + ds := datastore.NewDataStore(tempDir) + _ = ds.Initialize() + + r, server := setupRouter("http://127.0.0.1:8000", ds) + ts := httptest.NewServer(r) + defer ts.Close() + + // Default state: opted out, with a nonzero interval so enabling it later + // doesn't need an interval to be supplied. + if interval, enabled := server.GetUpdateCheckSettings(); enabled || interval == 0 { + t.Fatalf("Expected the check to default to disabled with a nonzero interval, got %v/%v", interval, enabled) + } + + enableBody, err := json.Marshal(map[string]interface{}{ + "server_url": "http://127.0.0.1:8000", + "update_check_enabled": true, + "update_check_interval": "6h", + }) + if err != nil { + t.Fatalf("Failed to marshal request body: %v", err) + } + + res, err := http.Post(ts.URL+"/setup/settings", "application/json", bytes.NewBuffer(enableBody)) + if err != nil { + t.Fatal(err) + } + res.Body.Close() + + if res.StatusCode != http.StatusOK { + t.Fatalf("POST /setup/settings (enable): expected 200, got %v", res.Status) + } + + interval, enabled := server.GetUpdateCheckSettings() + if !enabled || interval != 6*time.Hour { + t.Errorf("Expected live settings 6h/true, got %v/%v", interval, enabled) + } + + persisted, err := ds.GetSettings() + if err != nil { + t.Fatalf("Failed to reload settings: %v", err) + } + if !persisted.UpdateCheckEnabled || persisted.UpdateCheckInterval != "6h0m0s" { + t.Errorf("Expected persisted 6h0m0s/true, got %q/%v", + persisted.UpdateCheckInterval, persisted.UpdateCheckEnabled) + } + + res, err = http.Get(ts.URL + "/setup/settings") + if err != nil { + t.Fatal(err) + } + defer res.Body.Close() + + var got map[string]interface{} + if err := json.NewDecoder(res.Body).Decode(&got); err != nil { + t.Fatalf("Failed to decode GET /setup/settings: %v", err) + } + if got["update_check_enabled"] != true { + t.Errorf("GET /setup/settings: expected update_check_enabled true, got %+v", got["update_check_enabled"]) + } + if got["update_check_interval"] != "6h0m0s" { + t.Errorf("GET /setup/settings: expected update_check_interval 6h0m0s, got %+v", got["update_check_interval"]) + } + + // An unparseable interval must be rejected before anything is applied. + badBody, err := json.Marshal(map[string]interface{}{ + "server_url": "http://127.0.0.1:8000", + "update_check_enabled": true, + "update_check_interval": "not-a-duration", + }) + if err != nil { + t.Fatalf("Failed to marshal request body: %v", err) + } + + res, err = http.Post(ts.URL+"/setup/settings", "application/json", bytes.NewBuffer(badBody)) + if err != nil { + t.Fatal(err) + } + res.Body.Close() + + if res.StatusCode != http.StatusBadRequest { + t.Errorf("POST /setup/settings (bad interval): expected 400, got %v", res.Status) + } + + // A zero interval must force the check off rather than leave the poller + // hitting GitHub on every tick. + zeroBody, err := json.Marshal(map[string]interface{}{ + "server_url": "http://127.0.0.1:8000", + "update_check_enabled": true, + "update_check_interval": "0s", + }) + if err != nil { + t.Fatalf("Failed to marshal request body: %v", err) + } + + res, err = http.Post(ts.URL+"/setup/settings", "application/json", bytes.NewBuffer(zeroBody)) + if err != nil { + t.Fatal(err) + } + res.Body.Close() + + if res.StatusCode != http.StatusOK { + t.Fatalf("POST /setup/settings (zero interval): expected 200, got %v", res.Status) + } + + if _, enabled := server.GetUpdateCheckSettings(); enabled { + t.Error("Expected a zero interval to disable the update check") + } +} + // TestHandleGetVersionInfo_IncludesAbsoluteDataDir verifies /api/setup/version // reports the actual data directory in use, resolved to an absolute path — // added so operators running the service locally (not in Docker, where the diff --git a/pkg/service/handlers/server.go b/pkg/service/handlers/server.go index 94380ba..05cee45 100644 --- a/pkg/service/handlers/server.go +++ b/pkg/service/handlers/server.go @@ -52,6 +52,8 @@ type Server struct { recordEnabled bool discoveryInterval time.Duration discoveryEnabled bool + updateCheckInterval time.Duration // live update-check interval; see SetUpdateCheckSettings + updateCheckEnabled bool // live update-check opt-in; defaults off (#591) dnsEnabled bool dnsUpstream []string dnsBindAddr string @@ -71,7 +73,7 @@ type Server struct { mgmtPassword string adminAreaAuth string // "" (unset) / "enabled" / "disabled" — see datastore.Settings.AdminAreaAuth dismissedAnnouncements map[string]time.Time // announcement id -> most recent dismissal; see RecordDismissal - updateChecker *updatecheck.Checker // nil unless SetUpdateChecker was called (opt-in, see #591) + updateChecker *updatecheck.Checker // the HTTP-checking object; nil unless SetUpdateChecker was called spotifyClientID string spotifyClientSecret string spotifyRedirectURI string @@ -141,10 +143,14 @@ func NewServer(ds *datastore.DataStore, sm *setup.Manager, serverURL string, red recordEnabled: recordEnabled, discoveryInterval: 5 * time.Minute, discoveryEnabled: true, - peerObserver: newPeerObserver(), - healthRegistry: health.NewRegistry(), - authProbes: newAuthProbeRegistry(defaultAuthProbeTTL), - deprecatedRoutes: newDeprecatedRouteTracker(), + // The update check is opt-in (#591): only the interval gets a default, + // updateCheckEnabled stays false so no install starts making outbound + // GitHub calls without an explicit yes. + updateCheckInterval: 24 * time.Hour, + peerObserver: newPeerObserver(), + healthRegistry: health.NewRegistry(), + authProbes: newAuthProbeRegistry(defaultAuthProbeTTL), + deprecatedRoutes: newDeprecatedRouteTracker(), } health.RegisterSourcesXMLPresent(s.healthRegistry, ds) @@ -583,6 +589,28 @@ func (s *Server) SetDiscoverySettings(interval time.Duration, enabled bool) { s.discoveryEnabled = enabled } +// SetUpdateCheckSettings sets the live update-check settings for the server. +// +// Kept adjacent to its getter (rather than next to GetDiscoverySettings +// further down) so the pair reads as one unit; the background goroutine in +// soundtouch-service re-reads them on every poll, which is what makes the +// Settings-page toggle take effect without a restart. +func (s *Server) SetUpdateCheckSettings(interval time.Duration, enabled bool) { + s.mu.Lock() + defer s.mu.Unlock() + + s.updateCheckInterval = interval + s.updateCheckEnabled = enabled +} + +// GetUpdateCheckSettings returns the current update-check interval and enabled state. +func (s *Server) GetUpdateCheckSettings() (time.Duration, bool) { + s.mu.RLock() + defer s.mu.RUnlock() + + return s.updateCheckInterval, s.updateCheckEnabled +} + // SetDevicesChangedHook registers a callback fired after the known device set // changes (a discovery sweep or a manual add). The embedded web UI uses it to // re-sync its registry from the shared datastore — the single source of truth — @@ -1128,9 +1156,13 @@ func (s *Server) IsAnnouncementDismissed(id string) bool { return ok } -// SetUpdateChecker registers the opt-in periodic update checker (#591). Not -// called at all unless UPDATE_CHECK_ENABLED — leave nil otherwise; -// UpdateCheckResult handles that safely. +// SetUpdateChecker registers the update checker (#591). The checker itself is +// always constructed and registered, regardless of whether the periodic check +// is enabled, so /api/setup/version and the Announcements banner can read +// LastResult() (e.g. a result persisted by an earlier run) even before the +// periodic check has ever run. Only the periodic background check is gated by +// the live enabled setting — see SetUpdateCheckSettings. Callers that leave +// this nil are still safe: UpdateCheckResult returns the zero value. func (s *Server) SetUpdateChecker(c *updatecheck.Checker) { s.mu.Lock() defer s.mu.Unlock() diff --git a/pkg/service/handlers/web/index.html b/pkg/service/handlers/web/index.html index 52b72de..68cf00e 100644 --- a/pkg/service/handlers/web/index.html +++ b/pkg/service/handlers/web/index.html @@ -311,6 +311,23 @@ +
+ Update Check: +
+ +
+ + +
+
+ Makes one unauthenticated GET request to api.github.com per interval when enabled. + No other data leaves this install. Applies live, no restart needed — takes effect + within a minute (worst case). +
+
+
DNS Discovery:
diff --git a/pkg/service/handlers/web/js/script.js b/pkg/service/handlers/web/js/script.js index 7baaffc..db85079 100644 --- a/pkg/service/handlers/web/js/script.js +++ b/pkg/service/handlers/web/js/script.js @@ -354,6 +354,12 @@ async function fetchSettings() { if (settings.discovery_enabled !== undefined) { document.getElementById("discovery-enabled").checked = settings.discovery_enabled; } + if (settings.update_check_interval) { + document.getElementById("update-check-interval").value = settings.update_check_interval; + } + if (settings.update_check_enabled !== undefined) { + document.getElementById("update-check-enabled").checked = settings.update_check_enabled; + } if (settings.default_landing) { document.getElementById("default-landing").value = settings.default_landing; } @@ -504,6 +510,8 @@ async function updateSettings() { admin_area_auth: document.getElementById("admin-area-auth").value, discovery_interval: document.getElementById("discovery-interval").value, discovery_enabled: document.getElementById("discovery-enabled").checked, + update_check_interval: document.getElementById("update-check-interval").value, + update_check_enabled: document.getElementById("update-check-enabled").checked, dns_enabled: document.getElementById("dns-enabled").checked, dns_upstream: document.getElementById("dns-upstream").value, dns_bind_addr: document.getElementById("dns-bind").value,