From 9f61d00b2d8eb417c6f9fef9f4fdd1acd78cfbda Mon Sep 17 00:00:00 2001 From: Tobias Gesellchen Date: Mon, 10 Aug 2026 22:53:46 +0200 Subject: [PATCH] feat(admin): live Settings-page toggle for the opt-in update check Follow-up to #591: UpdateCheckEnabled/UpdateCheckInterval are now persisted, live-reloaded Settings fields (mirroring the discovery enabled/interval pattern), editable from the admin Settings page without a restart. The env var/CLI flag remains the seed value for a fresh install with no settings.json yet. The background goroutine now always runs and polls the live settings every minute (updateCheckPollTick), instead of being started only if enabled at process launch, so flipping the toggle takes effect within a minute rather than requiring a restart. --- cmd/soundtouch-service/main.go | 92 +++++++--- cmd/soundtouch-service/update_check_test.go | 54 +++++- .../content/docs/guides/SOUNDTOUCH-SERVICE.md | 4 +- pkg/service/datastore/datastore.go | 2 + pkg/service/datastore/datastore_test.go | 16 +- pkg/service/handlers/handlers_setup.go | 66 ++++++- pkg/service/handlers/handlers_setup_test.go | 165 ++++++++++++++++++ pkg/service/handlers/server.go | 48 ++++- pkg/service/handlers/web/index.html | 17 ++ pkg/service/handlers/web/js/script.js | 8 + 10 files changed, 422 insertions(+), 50 deletions(-) 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,